Repository navigation
[machine] 사용자 기기 히스토리 모달 연결 - #40
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
리뷰 결과
변경된 9개 파일을 검토했습니다. 인라인 관찰 3개를 게시했습니다.
| 심각도 | 개수 |
|---|---|
| 치명적 | 0 |
| 높음 | 0 |
| 중간 | 3 |
| 낮음 | 0 |
유형: 필수 수정 0, 수정 권장 2, 제안 0, 확인 질문 1, 좋았던 점 0
주요 관찰
- 수정 권장 · MEDIUM 추가한 테스트가 현재 실행 경로에 포함되지 않습니다
- 수정 권장 · MEDIUM 응답은 있지만 표시 가능한 그룹이 없으면 모달이 빈 화면이 됩니다
- 확인 질문 · MEDIUM 일반 사용자에게 다른 사용자의 호실을 공개해도 되는 정책인가요?
검토한 head: 34d0926a272e · 최신 head는 /review로 검토하세요.
| @@ -0,0 +1,90 @@ | |||
| import assert from "node:assert/strict"; | |||
| import test from "node:test"; | |||
| import { getRecentMachineHistoryRange } from "../src/widgets/user-machine-list/lib/machineHistory.ts"; | |||
There was a problem hiding this comment.
🔷 수정 권장 · MEDIUM
추가한 테스트가 현재 실행 경로에 포함되지 않습니다
새 테스트는 TypeScript 소스 파일을 직접 가져오지만, 제공된 Node 20 환경은 별도 로더 없이 해당 TypeScript 구문을 실행할 수 없습니다. 또한 package.json에 test 스크립트가 없고 CI도 lint, 타입 검사, 빌드만 수행하므로 이 테스트들은 회귀를 막지 못합니다.
이유: 테스트가 존재해도 자동으로 실행되지 않으면 변경된 동작에 대한 보호 효과가 없습니다.
제안: TS 지원 테스트 환경을 마련한 뒤 CI에서 명시적으로 실행되는지 확인하세요.
There was a problem hiding this comment.
확인했습니다. 현재 저장소의 package.json에는 test 스크립트가 없고, CI도 lint·typecheck·build만 실행하며 Node 20을 사용하고 있습니다. 이번 PR에서는 테스트 인프라 변경을 범위에 포함하지 않고, 저장소 공통 설정을 별도 PR로 분리하겠습니다.
이번 PR의 테스트는 Node 22.23.2에서 node --test --experimental-strip-types tests/*.test.mjs로 51개 통과를 확인했습니다.
| aria-labelledby={`history-${group.label}`} | ||
| > | ||
| <h3 | ||
| id={`history-${group.label}`} |
There was a problem hiding this comment.
🔷 수정 권장 · MEDIUM
응답은 있지만 표시 가능한 그룹이 없으면 모달이 빈 화면이 됩니다
빈 상태는 원본 items 길이로 판단하지만 목록은 groups가 있을 때만 렌더링합니다. 따라서 날짜 문자열을 해석하지 못하거나 조회 범위 밖 날짜로 분류되어 모든 항목이 그룹화에서 제외되면 로딩, 오류, 빈 상태, 목록이 모두 렌더링되지 않습니다.
이유: 응답 형식이나 날짜 경계의 작은 이상이 사용자에게 설명 없는 빈 화면으로 나타납니다.
제안: 빈 상태 조건을 실제 표시 항목 수와 일치시키고 제외 항목 처리 방식을 정하세요.
There was a problem hiding this comment.
확인 후 수정했습니다. 응답 항목 수가 아니라 실제로 화면에 표시할 수 있는 날짜 그룹 수를 기준으로 빈 상태를 판단하도록 변경했습니다. 날짜 파싱 실패나 오늘·어제 그룹으로 분류되지 않는 항목이 와도 설명 없는 빈 화면 대신 빈 상태가 표시됩니다.
회귀 테스트도 추가했고, 수정 커밋은 f6e0f2d입니다.
| <p className="mt-1 flex items-center gap-1 text-xs text-muted-foreground"> | ||
| <Home className="h-3.5 w-3.5" /> | ||
| {item.userRoomNumber}호 | ||
| </p> |
There was a problem hiding this comment.
🔷 확인 질문 · MEDIUM
일반 사용자에게 다른 사용자의 호실을 공개해도 되는 정책인가요?
이 모달은 일반 사용자 홈의 기기 목록에서 열리며 각 이용 기록의 호실을 그대로 표시합니다. 해당 API가 현재 사용자의 기록만 반환한다는 보장이 있는지, 아니면 모든 기기 이용자의 호실 공개가 제품·개인정보 정책상 허용되는지 확인이 필요합니다.
이유: 호실과 시간대 정보는 개인을 식별하거나 생활 패턴을 추정하는 데 사용될 수 있습니다.
제안: API의 사용자별 접근 범위와 호실 공개 정책을 확인하고, 허용되지 않는다면 서버 권한 제한과 UI 마스킹을 적용하세요.
There was a problem hiding this comment.
확인 결과 이 API는 현재 사용자 예약만 반환하는 API가 아니라, 인증된 사용자가 특정 기기의 이용 이력을 조회하는 API입니다. 백엔드의 GET /api/v2/machines/{id}/history는 기기와 기간으로 조회하고 userRoomNumber를 응답하며, 이름·학번은 반환하지 않습니다.
이번 화면의 목적도 개인 예약 이력이 아닌 기기별 이용 이력 확인이므로 다른 호실의 기록과 정확한 호실 표시는 유지했습니다. v1 사용자 웹과 앱의 기존 히스토리 화면도 예약 호실을 표시하고 있습니다. 이름·학번을 추가 노출하는 변경은 하지 않았습니다.
jjhhss100906
left a comment
There was a problem hiding this comment.
히스토리 모달 연결 확인했습니다. 한국 시간 기준 어제 00:00 ~ 오늘 23:59:59 범위 계산, 기존 renderActions 슬롯을 활용한 히스토리 버튼 연결, 추가 조회 실패 시 기존 기록을 유지하는 처리까지 좋습니다. feat/user-machine-history 기준 로컬에서 테스트 50개 통과도 확인했습니다.
봇 코멘트와 관련해 의견 남깁니다.
-
빈 화면 가능성 (인라인 참고)
봇 지적에 동의합니다. 수정이 간단해서 반영하면 좋을 것 같습니다. -
테스트가 CI에서 실행되지 않는 문제
맞는 지적이지만 이 PR만의 문제는 아닌 것 같습니다. 저장소에test스크립트가 없고 CI가 Node 20이라.ts를 직접 import하는 기존 테스트(tests/*.test.mjs)도 모두 실행되지 않고 있습니다.--experimental-strip-types는 Node 22.6 이상에서 동작하니, 별도 PR로package.json에test스크립트를 추가하고 CI를 Node 22로 올려 테스트 단계를 넣는 방향을 제안드립니다. 필요하면 제가 올리겠습니다. -
다른 사용자 호실 공개 (봇 확인 질문)
v1 사용자 웹의 히스토리 모달도 "예약 호실"을 표시했고, 백엔드GET /api/v2/machines/{id}/history도 로그인한 사용자에게userRoomNumber를 내려주고 있어서 v1과 같은 정책으로 보입니다. -
(확인) 디자인
v1 히스토리 모달은 예약 생성 · 작업 시작 · 완료 타임라인 형태였는데, 이 PR은 오늘 / 어제로 묶은 새 디자인이라 의도하신 건지 확인 부탁드립니다. 최근 2일만 보여주는 정책이라면 지금 구성이 더 자연스러워 보이긴 합니다. -
(사소) import 경로
machineHistoryPresentation.ts가@/entities/machine대신../../../entities/machine/model/types.ts로 import하고 있습니다. 타입 import라 Node 테스트에서도 지워져서 별칭을 써도 될 것 같습니다.
참고로 제 쪽에 고장 신고 모달 작업이 있는데, 이 PR이 머지되면 히스토리 버튼 옆에 고장 신고 버튼을 함께 붙이는 PR로 올리겠습니다.
|
|
||
| {!historyQuery.isLoading && | ||
| errorState === null && | ||
| items.length === 0 && ( |
There was a problem hiding this comment.
봇 지적에 동의합니다. 빈 상태는 원본 items 길이로 판단하는데 목록은 groups가 있을 때만 그려서, 응답에 항목이 있지만 날짜 해석에 실패했거나 오늘 · 어제 범위 밖으로 분류되면 로딩 · 오류 · 빈 상태 · 목록이 모두 렌더링되지 않습니다.
여기 조건을 groups.length === 0으로 바꾸면 표시할 항목이 없을 때 항상 빈 상태가 보여서 설명 없는 빈 화면을 막을 수 있을 것 같습니다.
There was a problem hiding this comment.
반영했습니다. 실제 표시 항목이 없을 때 groups.length를 기준으로 빈 상태를 보여주도록 수정했고, 관련 회귀 테스트를 추가했습니다.
수정 커밋: f6e0f2d
|
리뷰 내용 확인 후 다음과 같이 반영했습니다.
|
jjhhss100906
left a comment
There was a problem hiding this comment.
빈 상태를 표시 가능한 날짜 그룹 기준으로 판단하도록 수정(f6e0f2d)하고 회귀 테스트까지 추가된 것 확인했습니다. feat/user-machine-history 기준 로컬 테스트 51개 통과, CI도 통과 확인했습니다. 테스트 CI는 별도 인프라 PR로 분리하는 것에 동의합니다. 수고하셨습니다!
개요 💡
일반 사용자 웹에서 각 세탁기·건조기의 최근 2일 이용 히스토리를 확인할 수 있도록 연결합니다.
작업내용 ⌨️
machineId기준/api/v2/machines/{machineId}/history연동확인사항