Conversation
There was a problem hiding this comment.
Code Review
This pull request implements comprehensive room state management and playlist functionality, including the integration of a YouTube player and the ability for users to add tracks. It introduces the useRoomState hook to synchronize queue, participant, and playback data via WebSockets and standardizes room slug handling with a new normalization utility. The review feedback suggests improving the uniqueness of generated log IDs using crypto.randomUUID(), removing redundant normalization calls in WebSocket API functions, and ensuring that debug-heavy UI components are handled appropriately for production environments.
| setRoomEventLogs((prev) => | ||
| [ | ||
| { | ||
| id: `${Date.now()}-${event.type}`, |
There was a problem hiding this comment.
Date.now()와 event.type을 조합하여 ID를 생성하면 동일한 타입의 이벤트가 같은 밀리초에 도착했을 때 중복된 키가 생성될 수 있습니다. 이는 React 렌더링 시 문제를 일으킬 수 있습니다. 124라인의 Math.random()을 사용하는 경우도 마찬가지로 고유성을 완벽하게 보장하지 않습니다. crypto.randomUUID()와 같이 더 안정적인 고유 ID 생성기 사용을 고려해보세요.
| id: `${Date.now()}-${event.type}`, | |
| id: crypto.randomUUID(), |
| <div className="space-y-4 p-4"> | ||
| <div>room: {slug}</div> | ||
| <div>join: {status}</div> | ||
| <div>message: {message}</div> | ||
| <div>Error Code: {code}</div> | ||
| <div>last room event: {lastRoomEventType}</div> | ||
| <div>last room event time: {lastRoomEventTime || "-"}</div> | ||
| <YouTubePlayer | ||
| videoId={currentVideoId} | ||
| playbackStatus={playbackStatus?.status ?? null} | ||
| currentTimeMs={playbackStatus?.currentTime ?? null} | ||
| /> | ||
| {status === "joined" ? <AddTrackAction slug={slug} /> : null} | ||
| <div> | ||
| state loading: {isRoomStateLoading || isRoomStateFetching ? "yes" : "no"} | ||
| </div> | ||
| <div>state error: {roomStateError?.message ?? "-"}</div> | ||
| <div>participants count: {participants.length}</div> | ||
| <div>current entry id: {currentEntry?.entryId ?? "-"}</div> | ||
| <div>current track videoId: {currentEntry?.track.videoId ?? "-"}</div> | ||
| <div>playback status: {playbackStatus?.status ?? "-"}</div> | ||
| <div>playback currentTime(ms): {playbackStatus?.currentTime ?? "-"}</div> | ||
| <div>queue count: {queue.length}</div> | ||
| {participants.length ? ( | ||
| <ul> | ||
| {participants.map((participant, index) => ( | ||
| <li key={participant.participantId ?? participant.id ?? index}> | ||
| {participant.nickname ?? participant.participantId ?? "unknown"} | ||
| </li> | ||
| ))} | ||
| </ul> | ||
| ) : null} | ||
| {currentEntry ? ( | ||
| <div> | ||
| current entry: | ||
| <pre className="mt-2 overflow-x-auto whitespace-pre-wrap break-all text-xs text-gray-700"> | ||
| {JSON.stringify(currentEntry, null, 2)} | ||
| </pre> | ||
| </div> | ||
| ) : null} | ||
| <div> | ||
| queue entries: | ||
| </div> | ||
| {queue.length ? ( | ||
| <ul> | ||
| {queue.map((entry) => ( | ||
| <li key={entry.entryId}> | ||
| {entry.order}. {entry.entryId} ({entry.track.videoId}) | ||
| </li> | ||
| ))} | ||
| </ul> | ||
| ) : null} | ||
| <div className="space-y-2"> | ||
| <div className="font-semibold">room event logs</div> | ||
| {roomEventLogs.length ? ( | ||
| <ul className="space-y-2"> | ||
| {roomEventLogs.map((eventLog) => ( | ||
| <li | ||
| key={eventLog.id} | ||
| className="rounded-lg border border-gray-200 bg-gray-50 p-3" | ||
| > | ||
| <div className="text-sm font-medium text-gray-900"> | ||
| {eventLog.type} | ||
| </div> | ||
| <div className="text-xs text-gray-500"> | ||
| received at {eventLog.receivedAt} | ||
| </div> | ||
| <pre className="mt-2 overflow-x-auto whitespace-pre-wrap break-all text-xs text-gray-700"> | ||
| {eventLog.body} | ||
| </pre> | ||
| </li> | ||
| ))} | ||
| </ul> | ||
| ) : ( | ||
| <div>아직 받은 방 이벤트 없음</div> | ||
| )} | ||
| </div> | ||
| </div> |
| export function publishAddTrack(safeSlug: string, payload: AddTrackPayload) { | ||
| const client = getSocketClient(); | ||
| const normalizedSlug = normalizeRoomSlug(safeSlug); | ||
|
|
||
| client.publish({ | ||
| destination: `/app/room/${encodeURIComponent(normalizedSlug)}/playlist`, | ||
| body: JSON.stringify({ videoId: payload.videoId.trim() }), | ||
| }); | ||
| } |
There was a problem hiding this comment.
publishAddTrack 함수의 safeSlug 파라미터는 이미 정규화된 값을 전달받을 것으로 예상됩니다. 여기서 normalizeRoomSlug를 다시 호출하는 것은 중복으로 보입니다. 호출하는 쪽에서 정규화된 slug를 전달하도록 하고, 이 함수 내에서는 중복 호출을 제거하는 것이 좋겠습니다. 파라미터 이름을 normalizedSlug로 변경하여 의도를 더 명확하게 할 수 있습니다.
export function publishAddTrack(normalizedSlug: string, payload: AddTrackPayload) {
const client = getSocketClient();
client.publish({
destination: `/app/room/${encodeURIComponent(normalizedSlug)}/playlist`,
body: JSON.stringify({ videoId: payload.videoId.trim() }),
});
}| export function publishJoinRequest( | ||
| safeSlug: string, | ||
| payload: JoinRoomPayload, | ||
| ) { | ||
| const client = getSocketClient(); | ||
| const normalizedSlug = normalizeRoomSlug(safeSlug); | ||
|
|
||
| client.publish({ | ||
| destination: `/app/room/${encodeURIComponent(safeSlug)}/join`, | ||
| destination: `/app/room/${encodeURIComponent(normalizedSlug)}/join`, | ||
| body: JSON.stringify({ password: payload.password ?? null }), | ||
| }); | ||
| } |
There was a problem hiding this comment.
publishJoinRequest 함수의 safeSlug 파라미터는 joinRoom 함수에서 이미 normalizeRoomSlug를 통해 정규화된 값을 전달받습니다. 여기서 normalizeRoomSlug를 다시 호출하는 것은 중복입니다. 파라미터 이름을 normalizedSlug 등으로 변경하여 의도를 명확히 하고, 함수 내 중복 호출을 제거하는 것이 좋습니다.
export function publishJoinRequest(
normalizedSlug: string,
payload: JoinRoomPayload,
) {
const client = getSocketClient();
client.publish({
destination: `/app/room/${encodeURIComponent(normalizedSlug)}/join`,
body: JSON.stringify({ password: payload.password ?? null }),
});
}| export function subscribeRoomEvents( | ||
| safeSlug: string, | ||
| onMessage: (message: IMessage) => void, | ||
| ): StompSubscription { | ||
| const client = getSocketClient(); | ||
| const destination = `/topic/room/${encodeURIComponent(safeSlug)}/events`; | ||
| const destination = `/topic/room/${encodeURIComponent( | ||
| normalizeRoomSlug(safeSlug), | ||
| )}/events`; | ||
|
|
||
| return client.subscribe(destination, onMessage); | ||
| } |
There was a problem hiding this comment.
subscribeRoomEvents 함수의 safeSlug 파라미터는 이미 정규화된 값을 전달받을 것으로 예상됩니다. 여기서 normalizeRoomSlug를 다시 호출하는 것은 중복입니다. 파라미터 이름을 normalizedSlug 등으로 변경하여 의도를 명확히 하고, 함수 내 중복 호출을 제거하는 것이 좋습니다.
| export function subscribeRoomEvents( | |
| safeSlug: string, | |
| onMessage: (message: IMessage) => void, | |
| ): StompSubscription { | |
| const client = getSocketClient(); | |
| const destination = `/topic/room/${encodeURIComponent(safeSlug)}/events`; | |
| const destination = `/topic/room/${encodeURIComponent( | |
| normalizeRoomSlug(safeSlug), | |
| )}/events`; | |
| return client.subscribe(destination, onMessage); | |
| } | |
| export function subscribeRoomEvents( | |
| normalizedSlug: string, | |
| onMessage: (message: IMessage) => void, | |
| ): StompSubscription { | |
| const client = getSocketClient(); | |
| const destination = `/topic/room/${encodeURIComponent( | |
| normalizedSlug, | |
| )}/events`; | |
| return client.subscribe(destination, onMessage); | |
| } |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthrough룸 슬러그 정규화 도입과 함께 플레이리스트 및 재생 상태 조회/동기화, WebSocket 발행·구독 확장, YouTube 플레이어와 곡 추가 UI 및 관련 API·훅이 추가되어 클라이언트에서 실시간 큐·재생 관리를 수행합니다. (49 words) Changes
Sequence Diagram(s)sequenceDiagram
participant User as 사용자
participant AddTrack as AddTrackAction
participant Modal as AddTrackModal
participant Extract as extractYouTubeVideoId
participant WS as publishAddTrack (STOMP)
participant Server as 서버
participant Queue as Playlist Queue
participant RoomPage as Room Page
participant RoomStateHook as useRoomState
User->>AddTrack: 곡 추가 버튼 클릭
AddTrack->>Modal: 모달 오픈
User->>Modal: 유튜브 URL 입력
Modal->>Extract: videoId 추출 요청
Extract-->>Modal: videoId 또는 null
Modal->>WS: publishAddTrack(slug, {videoId})
WS->>Server: STOMP /app/room/{slug}/playlist (receipt)
Server->>Queue: 큐에 항목 추가 (서버측)
Server->>RoomPage: 룸 이벤트 발행
RoomPage->>RoomStateHook: refetchRoomState() 트리거
RoomStateHook->>Server: GET /api/v1/rooms/{slug}/state
Server-->>RoomStateHook: RoomStateSnapshot
RoomPage->>RoomPage: UI/Player 업데이트 (queue, currentEntry, playback)
sequenceDiagram
participant RoomPage as Room Page
participant useRoomState as useRoomState Hook
participant API as fetchRoomState/fetchRoomPlayback
participant Server as Room Server
participant YouTube as YouTubePlayer
participant YTApi as YouTube Iframe API
RoomPage->>useRoomState: (slug, password, enabled)
useRoomState->>API: fetchRoomState/fetchRoomPlayback 호출
API->>Server: GET /api/v1/rooms/{slug}/state or /playback
Server-->>API: RoomStateSnapshot / PlaybackSnapshot
API-->>useRoomState: 결과 반환
useRoomState-->>RoomPage: 상태 제공 (participants, queue, playback)
RoomPage->>YouTube: videoId, playbackStatus, currentTimeMs 전달
YouTube->>YTApi: Iframe API 로드/초기화
YTApi-->>YouTube: 플레이어 이벤트 발생
YouTube-->>RoomPage: onPlaybackStateChange 전달
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (8)
src/features/room/create/model/useCreateRoom.ts (1)
21-23: room 경로 생성 규칙이 분산되어 있어 변경 누락 위험이 큽니다.
encodeURIComponent(normalizeRoomSlug(...))조합이 여러 파일에 반복되고 있어 정책 변경 시 누락 가능성이 높습니다.buildRoomPath(slug)같은 단일 함수로 경계화하는 편이 유지보수에 유리합니다.리팩터링 예시
- router.push( - `/room/${encodeURIComponent(normalizeRoomSlug(result.slug))}`, - ); + router.push(buildRoomPath(result.slug));// src/shared/lib/buildRoomPath.ts import { normalizeRoomSlug } from "@/src/shared/lib/normalizeRoomSlug"; export function buildRoomPath(slug: string): string { return `/room/${encodeURIComponent(normalizeRoomSlug(slug))}`; }As per coding guidelines,
**/*: "유지보수/확장성 관점에서 모듈 경계(의존성 방향, 책임 분리)가 적절한지 최우선으로 확인."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/room/create/model/useCreateRoom.ts` around lines 21 - 23, The room path generation is duplicated (using encodeURIComponent(normalizeRoomSlug(...))) and should be centralized: add a new helper function buildRoomPath(slug: string) that returns `/room/${encodeURIComponent(normalizeRoomSlug(slug))}` (use normalizeRoomSlug), export it from a shared lib, and replace direct uses in code (e.g., the router.push call that currently uses encodeURIComponent(normalizeRoomSlug(result.slug))) to call buildRoomPath(result.slug) instead; update imports where needed and run a repo-wide search/replace for other occurrences to ensure consistency.src/entities/playlist/model/types.ts (1)
23-29: 도메인 타입이 너무 느슨해서 확장 시 결합도가 커질 수 있습니다.
[key: string]: unknown중심 타입은 안전망이 약합니다.entryId,participantId,status,role등 핵심 필드는 점진적으로 명시 타입(또는 branded ID)으로 좁히는 걸 권장합니다.As per coding guidelines "TypeScript에서 any/과도한 as 캐스팅을 지양하고, 도메인 타입(예: ProfileId 같은 ID 타입)으로 의미를 드러내는지 확인."
Also applies to: 43-52
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/entities/playlist/model/types.ts` around lines 23 - 29, PlaylistEntryStatus and PlaylistAddedBy are too permissive using index signatures; replace the {[key: string]: unknown} shapes with explicit domain fields (e.g., entryId, participantId, status, role) and narrow types (string literals or enums for status/role) and use branded ID types (e.g., PlaylistEntryId, ProfileId) for identifiers; update the corresponding types (PlaylistEntryStatus, PlaylistAddedBy and the similar types around lines 43-52) to declare those properties explicitly and remove the open index signature so callers get compile-time guarantees and avoid excessive unknown/any casting.src/features/playlist/add-track/model/extractYouTubeVideoId.ts (1)
27-33: YouTube videoId 형식 검증을 추가하는 편이 안전합니다.Line 27-33은 빈 문자열만 걸러서
invalid-id같은 값도 통과됩니다. 클라이언트에서 11자 포맷을 한 번 더 검증하면 요청 실패를 줄일 수 있습니다.제안 코드
+const YOUTUBE_VIDEO_ID_REGEX = /^[A-Za-z0-9_-]{11}$/; + export function extractYouTubeVideoId(input: string): string | null { @@ if (hostname === "youtube.com" && parsedUrl.pathname === "/watch") { const videoId = parsedUrl.searchParams.get("v")?.trim(); - return videoId || null; + return videoId && YOUTUBE_VIDEO_ID_REGEX.test(videoId) ? videoId : null; } if (hostname === "youtu.be") { const videoId = parsedUrl.pathname.split("/").filter(Boolean)[0]?.trim(); - return videoId || null; + return videoId && YOUTUBE_VIDEO_ID_REGEX.test(videoId) ? videoId : null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/playlist/add-track/model/extractYouTubeVideoId.ts` around lines 27 - 33, The extracted videoId from parsedUrl.searchParams.get("v") and from parsedUrl.pathname (in the hostname === "youtu.be" branch) should be validated against the expected YouTube ID format before returning; update the extractYouTubeVideoId logic to trim the candidate, then check it matches the canonical YouTube ID pattern (exactly 11 characters using [A-Za-z0-9_-]) and return null if the regex/length check fails, otherwise return the id; apply this check to both places where the local variable videoId is computed (referencing parsedUrl and videoId) so malformed values like "invalid-id" are rejected.src/app/room/[slug]/page.tsx (3)
283-286: 리스트 key 안정성 개선 권장
participant.participantId ?? participant.id ?? index로 key를 구성하고 있습니다.index를 fallback으로 사용하면 리스트 순서가 변경될 때 불필요한 DOM 재생성이 발생할 수 있습니다.모든 participant에 고유 ID가 있다면
indexfallback을 제거하고, 없을 경우 서버에서 ID 제공을 요청하는 것이 좋습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/room/`[slug]/page.tsx around lines 283 - 286, The list key uses a fragile fallback `participant.participantId ?? participant.id ?? index` inside the participants.map render; remove the `index` fallback so keys are always stable (use only `participant.participantId` or `participant.id`) and, if some participants lack a unique id, update the data source / API to return a stable identifier for every participant rather than falling back to `index`; locate the usage in the participants.map callback (the li key expression) and replace it accordingly.
239-245: 중복 refetch 가능성이 effect는
status === "joined"일 때slug변경 시refetchRoomState()를 호출합니다. 그러나useRoomState의 queryKey가["roomState", slug, password ?? null]로 구성되어 있어,slug가 변경되면 React Query가 자동으로 새 쿼리를 실행합니다.이 effect가 의도적으로 추가된 것이라면(예: 초기 마운트 시 강제 fetch), 해당 의도를 주석으로 명시하거나, 불필요하다면 제거를 고려해 주세요.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/room/`[slug]/page.tsx around lines 239 - 245, The useEffect mounting the call to refetchRoomState may cause duplicate fetches because useRoomState already uses the queryKey ["roomState", slug, password ?? null] and React Query will refetch when slug changes; either remove the effect entirely (delete the useEffect that calls refetchRoomState) or keep it but add a clear comment above the effect explaining the specific intentional behavior (e.g., "force initial fetch on mount despite React Query's automatic refetching") and ensure dependencies remain [refetchRoomState, slug, status]; reference useEffect, refetchRoomState, and useRoomState/queryKey in your change.
138-147: 이벤트 로그 key 중복 가능성
id가${Date.now()}-${event.type}으로 생성되는데, 동일 밀리초에 같은 타입의 이벤트가 연속으로 수신되면 key가 중복될 수 있습니다.♻️ 개선안
setRoomEventLogs((prev) => [ { - id: `${Date.now()}-${event.type}`, + id: `${Date.now()}-${Math.random().toString(36).slice(2)}`, type: event.type, receivedAt, body: JSON.stringify(event, null, 2), }, ...prev, ].slice(0, 20), );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/room/`[slug]/page.tsx around lines 138 - 147, The current event log id generation uses the template `${Date.now()}-${event.type}`, which can collide when multiple events of the same type arrive in the same millisecond; update the id generation inside the array creation (the object with properties id, type, receivedAt, body) to append a small unique suffix (for example use crypto.randomUUID(), globalThis.crypto?.randomUUID(), a short random hex via Math.random(), or an incrementing counter) so each id becomes something like Date.now()-event.type-<uniqueSuffix>, ensuring uniqueness even for same-millisecond events.src/features/playlist/player/ui/YouTubePlayer.tsx (1)
296-296: 콜백 props 변경 시 플레이어 재생성 가능성
useEffect의존성 배열에onPlaybackStateChange와onPlayerReady가 포함되어 있습니다. 호출 측에서 이 콜백들을 인라인 함수로 전달하면 매 렌더마다 플레이어가 재생성될 수 있습니다.현재
page.tsx에서는 이 콜백들을 전달하지 않으므로 즉시 문제는 없지만, 향후 사용 시 호출 측에서useCallback으로 감싸야 합니다. 또는 콜백을 ref로 저장하여 effect 의존성에서 제외하는 방법도 있습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/playlist/player/ui/YouTubePlayer.tsx` at line 296, The effect in YouTubePlayer (the useEffect that depends on applyDesiredPlayback, destroyPlayer, onPlaybackStateChange, onPlayerReady, videoId) can recreate the player every render if callers pass inline callbacks; to fix, either require callers to memoize those callbacks (wrap onPlaybackStateChange and onPlayerReady with useCallback where passed) or make YouTubePlayer resilient by storing them in refs: create refs for onPlaybackStateChange and onPlayerReady inside YouTubePlayer, update ref.current in a small effect when props change, and remove those callback props from the main player-creation effect dependencies so only stable values (applyDesiredPlayback, destroyPlayer, videoId) trigger player recreation.src/entities/playlist/model/useGetPlayList.ts (1)
8-14:useGetPlayList에 비밀번호 파라미터 누락 - API 패턴 불일치 확인 필요
useRoomState와useRoomPlayback는password?: string | null파라미터를 받아서 API 요청 시X-Room-Password헤더로 전달하는 반면,useGetPlayList에는 해당 파라미터가 없습니다.코드 내에
PlaylistProtectedRequestParams타입이 정의되어 있지만,getPlaylist는GetPlaylistParams를 사용하고 있어 의도적 설계인지 또는 미처 반영되지 않은 부분인지 확인이 필요합니다. 비밀번호가 설정된 방의 경우 state/playback 조회와 동일하게 playlist 조회 시에도 인증이 필요한지 백엔드 API 사양을 검토하고, 필요시 함수 서명을 통일해 주세요.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/entities/playlist/model/useGetPlayList.ts` around lines 8 - 14, The useGetPlayList hook is missing the optional password parameter used by useRoomState/useRoomPlayback; update useGetPlayList(slug: string | null, password?: string | null, enabled = true) to accept password and pass it into getPlaylist so the X-Room-Password header can be sent (use the existing PlaylistProtectedRequestParams or reconcile with GetPlaylistParams), update the queryKey to include password (e.g., ["playlist", slug, password]), and ensure enabled logic remains enabled && !!slug; also align the getPlaylist call signature in getPlaylist(...) to accept and forward the password when required.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/room/`[slug]/page.tsx:
- Around line 265-269: The YouTubePlayer is receiving seconds instead of
milliseconds via currentTimeMs: convert playbackStatus?.currentTime (which is in
seconds per PlaybackSyncData) to milliseconds before passing it into
<YouTubePlayer>; locate the JSX where YouTubePlayer is rendered and change the
prop to pass (playbackStatus?.currentTime != null ? playbackStatus.currentTime *
1000 : null) so the component receives milliseconds as expected (keep videoId
and playbackStatus props unchanged).
In `@src/entities/playlist/api/fetchRoomState.ts`:
- Around line 16-20: In fetchRoomState, normalize the incoming password by
calling trim() and only attach the "X-Room-Password" header when the trimmed
value is non-empty; update the conditional that currently uses password to use
trimmedPassword (or similar) so whitespace-only inputs don't create a header,
keeping fetch headers/credentials behavior consistent.
In `@src/entities/playlist/api/websocket/publishAddTrack.ts`:
- Around line 9-17: publishAddTrack currently fires a STOMP publish and returns
void, invalidating callers' try-catch; change publishAddTrack(safeSlug, payload)
to return Promise<void> that resolves on STOMP receipt and rejects on
ERROR/timeout. Specifically, use getSocketClient() and
normalizeRoomSlug(safeSlug) as before, but attach a unique receipt header to the
publish to `/app/room/${encodeURIComponent(normalizedSlug)}/playlist`, register
a one-shot receipt callback that resolves the promise, register an ERROR (or
receipt timeout) handler that rejects with the broker error, and ensure the
promise cleans up those callbacks on settle; this way callers (e.g.,
AddTrackAction.handleSubmit) can await and catch publish failures.
In `@src/entities/room/api/websocket/publishJoinRequest.ts`:
- Around line 11-15: The publishJoinRequest code is re-normalizing an
already-normalized slug which can double-decode percent-encodings and break
matching; remove the extra normalizeRoomSlug() call and use the existing
safeSlug directly when building the publish destination (still wrap with
encodeURIComponent for the URL) so the slug used in client.publish matches the
slug produced by joinRoom (see joinRoom at Line 78) and avoid timeouts from
mismatched filters.
In `@src/features/playlist/add-track/ui/AddTrackAction.tsx`:
- Line 21: The isLoggedIn calculation in AddTrackAction.tsx incorrectly flips to
false on refetch errors because it uses `Boolean(me) && !isError`; change the
logic to treat presence of cached user data (`me`) as authenticated even when
`isError` is true (i.e., consider the user logged in whenever `me` is truthy,
and only treat as not logged in when `me` is null/undefined), so update the
`isLoggedIn` const to rely primarily on `me` rather than `!isError`.
In `@src/features/playlist/add-track/ui/AddTrackModal.tsx`:
- Around line 46-63: The AddTrackModal UI relies on backdrop clicks to close and
uses an unlabeled input which hurts accessibility; update the AddTrackModal
component to add an explicit, keyboard-focusable close control (e.g., a button
with onClick that calls the same close handler used by backdrop clicks and also
listens for Escape) and give the URL input a proper label (either a <label
htmlFor="..."> paired with the input id or an aria-label/aria-labelledby
attribute tied to a visible label) so screen readers can announce it; ensure the
close button has an accessible name (aria-label="닫기" or visible text) and that
existing handlers (onChange, onSubmit, submitting, errorMessage) continue to
work with the new elements.
In `@src/shared/lib/normalizeRoomSlug.ts`:
- Around line 1-13: normalizeRoomSlug currently returns an empty string for
blank input which can lead to invalid routes; add a separate exported validation
util (e.g., isValidRoomSlug(slug: string): boolean and/or requireRoomSlug(slug:
string): string) that trims the input, rejects empty strings (return false or
throw), and documents intended behavior so callers must guard before using
normalizeRoomSlug; keep normalizeRoomSlug’s decoding/trim logic but ensure
callers use isValidRoomSlug or requireRoomSlug to prevent passing "" into
routing/API paths.
---
Nitpick comments:
In `@src/app/room/`[slug]/page.tsx:
- Around line 283-286: The list key uses a fragile fallback
`participant.participantId ?? participant.id ?? index` inside the
participants.map render; remove the `index` fallback so keys are always stable
(use only `participant.participantId` or `participant.id`) and, if some
participants lack a unique id, update the data source / API to return a stable
identifier for every participant rather than falling back to `index`; locate the
usage in the participants.map callback (the li key expression) and replace it
accordingly.
- Around line 239-245: The useEffect mounting the call to refetchRoomState may
cause duplicate fetches because useRoomState already uses the queryKey
["roomState", slug, password ?? null] and React Query will refetch when slug
changes; either remove the effect entirely (delete the useEffect that calls
refetchRoomState) or keep it but add a clear comment above the effect explaining
the specific intentional behavior (e.g., "force initial fetch on mount despite
React Query's automatic refetching") and ensure dependencies remain
[refetchRoomState, slug, status]; reference useEffect, refetchRoomState, and
useRoomState/queryKey in your change.
- Around line 138-147: The current event log id generation uses the template
`${Date.now()}-${event.type}`, which can collide when multiple events of the
same type arrive in the same millisecond; update the id generation inside the
array creation (the object with properties id, type, receivedAt, body) to append
a small unique suffix (for example use crypto.randomUUID(),
globalThis.crypto?.randomUUID(), a short random hex via Math.random(), or an
incrementing counter) so each id becomes something like
Date.now()-event.type-<uniqueSuffix>, ensuring uniqueness even for
same-millisecond events.
In `@src/entities/playlist/model/types.ts`:
- Around line 23-29: PlaylistEntryStatus and PlaylistAddedBy are too permissive
using index signatures; replace the {[key: string]: unknown} shapes with
explicit domain fields (e.g., entryId, participantId, status, role) and narrow
types (string literals or enums for status/role) and use branded ID types (e.g.,
PlaylistEntryId, ProfileId) for identifiers; update the corresponding types
(PlaylistEntryStatus, PlaylistAddedBy and the similar types around lines 43-52)
to declare those properties explicitly and remove the open index signature so
callers get compile-time guarantees and avoid excessive unknown/any casting.
In `@src/entities/playlist/model/useGetPlayList.ts`:
- Around line 8-14: The useGetPlayList hook is missing the optional password
parameter used by useRoomState/useRoomPlayback; update useGetPlayList(slug:
string | null, password?: string | null, enabled = true) to accept password and
pass it into getPlaylist so the X-Room-Password header can be sent (use the
existing PlaylistProtectedRequestParams or reconcile with GetPlaylistParams),
update the queryKey to include password (e.g., ["playlist", slug, password]),
and ensure enabled logic remains enabled && !!slug; also align the getPlaylist
call signature in getPlaylist(...) to accept and forward the password when
required.
In `@src/features/playlist/add-track/model/extractYouTubeVideoId.ts`:
- Around line 27-33: The extracted videoId from parsedUrl.searchParams.get("v")
and from parsedUrl.pathname (in the hostname === "youtu.be" branch) should be
validated against the expected YouTube ID format before returning; update the
extractYouTubeVideoId logic to trim the candidate, then check it matches the
canonical YouTube ID pattern (exactly 11 characters using [A-Za-z0-9_-]) and
return null if the regex/length check fails, otherwise return the id; apply this
check to both places where the local variable videoId is computed (referencing
parsedUrl and videoId) so malformed values like "invalid-id" are rejected.
In `@src/features/playlist/player/ui/YouTubePlayer.tsx`:
- Line 296: The effect in YouTubePlayer (the useEffect that depends on
applyDesiredPlayback, destroyPlayer, onPlaybackStateChange, onPlayerReady,
videoId) can recreate the player every render if callers pass inline callbacks;
to fix, either require callers to memoize those callbacks (wrap
onPlaybackStateChange and onPlayerReady with useCallback where passed) or make
YouTubePlayer resilient by storing them in refs: create refs for
onPlaybackStateChange and onPlayerReady inside YouTubePlayer, update ref.current
in a small effect when props change, and remove those callback props from the
main player-creation effect dependencies so only stable values
(applyDesiredPlayback, destroyPlayer, videoId) trigger player recreation.
In `@src/features/room/create/model/useCreateRoom.ts`:
- Around line 21-23: The room path generation is duplicated (using
encodeURIComponent(normalizeRoomSlug(...))) and should be centralized: add a new
helper function buildRoomPath(slug: string) that returns
`/room/${encodeURIComponent(normalizeRoomSlug(slug))}` (use normalizeRoomSlug),
export it from a shared lib, and replace direct uses in code (e.g., the
router.push call that currently uses
encodeURIComponent(normalizeRoomSlug(result.slug))) to call
buildRoomPath(result.slug) instead; update imports where needed and run a
repo-wide search/replace for other occurrences to ensure consistency.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 977543de-4b24-4aec-a00a-d857e7c40fe1
📒 Files selected for processing (24)
src/app/room/[slug]/page.tsxsrc/entities/playlist/api/fetchRoomPlayback.tssrc/entities/playlist/api/fetchRoomState.tssrc/entities/playlist/api/getPlaylist.tssrc/entities/playlist/api/websocket/publishAddTrack.tssrc/entities/playlist/model/types.tssrc/entities/playlist/model/useGetPlayList.tssrc/entities/playlist/model/useRoomPlayback.tssrc/entities/playlist/model/useRoomState.tssrc/entities/room/api/deleteRoom.tssrc/entities/room/api/fetchRoomMeta.tssrc/entities/room/api/joinRoom.tssrc/entities/room/api/websocket/publishJoinRequest.tssrc/entities/room/api/websocket/subscribeRoomEvents.tssrc/entities/room/ui/RoomInfo.module.csssrc/entities/room/ui/RoomInfo.tsxsrc/features/playlist/add-track/model/extractYouTubeVideoId.tssrc/features/playlist/add-track/ui/AddTrackAction.tsxsrc/features/playlist/add-track/ui/AddTrackModal.tsxsrc/features/playlist/player/ui/YouTubePlayer.tsxsrc/features/room/create/model/useCreateRoom.tssrc/features/room/join/ui/JoinRoomButton.tsxsrc/shared/lib/normalizeRoomSlug.tssrc/widgets/home/ui/HomeTopBar.tsx
| headers: password | ||
| ? { | ||
| "X-Room-Password": password, | ||
| } | ||
| : undefined, |
There was a problem hiding this comment.
비밀번호 헤더는 trim 후 조건부 전송이 더 안전합니다.
Line 16-20에서 공백 포함 입력이 그대로 헤더로 나갈 수 있습니다. 전송 전 trim() 정규화로 인증 실패 노이즈를 줄이는 편이 좋습니다.
제안 코드
export async function fetchRoomState({
slug,
password,
}: PlaylistProtectedRequestParams): Promise<RoomStateSnapshot> {
+ const normalizedPassword = password?.trim();
+
const res = await axiosInstance.get<ApiResponse<RoomStateSnapshot>>(
`/api/v1/rooms/${encodeURIComponent(normalizeRoomSlug(slug))}/state`,
{
- headers: password
+ headers: normalizedPassword
? {
- "X-Room-Password": password,
+ "X-Room-Password": normalizedPassword,
}
: undefined,
},
);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/entities/playlist/api/fetchRoomState.ts` around lines 16 - 20, In
fetchRoomState, normalize the incoming password by calling trim() and only
attach the "X-Room-Password" header when the trimmed value is non-empty; update
the conditional that currently uses password to use trimmedPassword (or similar)
so whitespace-only inputs don't create a header, keeping fetch
headers/credentials behavior consistent.
| <input | ||
| type="url" | ||
| value={value} | ||
| onChange={(event) => onChange(event.target.value)} | ||
| placeholder="유튜브 링크를 입력하세요" | ||
| className="w-full rounded-lg border border-gray-300 px-3 py-2 text-sm text-gray-900 outline-none focus:border-gray-900" | ||
| /> | ||
| {errorMessage ? ( | ||
| <p className="text-sm text-red-600">{errorMessage}</p> | ||
| ) : null} | ||
| <button | ||
| type="button" | ||
| onClick={onSubmit} | ||
| disabled={submitting} | ||
| className="w-full rounded-lg bg-black px-4 py-2 text-sm font-medium text-white disabled:cursor-not-allowed disabled:bg-gray-400" | ||
| > | ||
| {submitting ? "신청 중..." : "신청하기"} | ||
| </button> |
There was a problem hiding this comment.
모달 접근성: 닫기 경로와 입력 라벨을 명시해 주세요.
Line 56-63 기준으로 닫기 액션이 명시적 버튼 없이 backdrop click에만 의존합니다. 또한 Line 46-52 입력은 label이 없어 스크린리더 접근성이 떨어집니다.
제안 코드
<div className="mt-4 space-y-3">
+ <label htmlFor="youtube-url-input" className="sr-only">
+ 유튜브 링크
+ </label>
<input
+ id="youtube-url-input"
type="url"
value={value}
onChange={(event) => onChange(event.target.value)}
placeholder="유튜브 링크를 입력하세요"
className="w-full rounded-lg border border-gray-300 px-3 py-2 text-sm text-gray-900 outline-none focus:border-gray-900"
/>
@@
- <button
- type="button"
- onClick={onSubmit}
- disabled={submitting}
- className="w-full rounded-lg bg-black px-4 py-2 text-sm font-medium text-white disabled:cursor-not-allowed disabled:bg-gray-400"
- >
- {submitting ? "신청 중..." : "신청하기"}
- </button>
+ <div className="flex gap-2">
+ <button
+ type="button"
+ onClick={onClose}
+ disabled={submitting}
+ className="w-full rounded-lg border border-gray-300 px-4 py-2 text-sm font-medium text-gray-700 disabled:cursor-not-allowed disabled:bg-gray-100"
+ >
+ 닫기
+ </button>
+ <button
+ type="button"
+ onClick={onSubmit}
+ disabled={submitting}
+ className="w-full rounded-lg bg-black px-4 py-2 text-sm font-medium text-white disabled:cursor-not-allowed disabled:bg-gray-400"
+ >
+ {submitting ? "신청 중..." : "신청하기"}
+ </button>
+ </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/features/playlist/add-track/ui/AddTrackModal.tsx` around lines 46 - 63,
The AddTrackModal UI relies on backdrop clicks to close and uses an unlabeled
input which hurts accessibility; update the AddTrackModal component to add an
explicit, keyboard-focusable close control (e.g., a button with onClick that
calls the same close handler used by backdrop clicks and also listens for
Escape) and give the URL input a proper label (either a <label htmlFor="...">
paired with the input id or an aria-label/aria-labelledby attribute tied to a
visible label) so screen readers can announce it; ensure the close button has an
accessible name (aria-label="닫기" or visible text) and that existing handlers
(onChange, onSubmit, submitting, errorMessage) continue to work with the new
elements.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/room/`[slug]/page.tsx:
- Line 284: The label "playback currentTime(ms):" mismatches the actual unit
because playbackStatus?.currentTime is in seconds (the code multiplies by * 1000
elsewhere to get ms); update the UI to be consistent by either changing the
label to "(s)" to reflect seconds or convert the displayed value to milliseconds
(value * 1000) and keep "(ms)"; locate the JSX div rendering
playbackStatus?.currentTime and the spot where currentTime is transformed (the
existing * 1000 conversion) and apply one of these two fixes so label and value
match.
- Around line 239-245: The effect is re-triggering because refetchRoomState
(returned directly from useQuery in useRoomState) gets a new reference each
render; wrap the refetch logic in a stable function or ref and remove the
unstable refetchRoomState from the dependency array: either (A) memoize/refactor
the refetch inside useRoomState (e.g., export a useCallback-stable refetch
wrapper from useRoomState so refetchRoomState is stable), or (B) create a stable
ref inside the page component (useRef or useCallback) that calls the current
refetchRoomState and use that stable wrapper in the useEffect dependency array
alongside slug and status; ensure useEffect only depends on stableRef (or the
useCallback) plus slug and status, not the original refetchRoomState.
- Around line 102-156: The ensureRoomSubscription useCallback should not depend
on the changing refetchRoomState function; create a stable ref (e.g.,
refetchRoomStateRef) that you update whenever refetchRoomState changes, then
read refetchRoomStateRef.current inside ensureRoomSubscription instead of using
refetchRoomState directly so you can remove refetchRoomState from the
ensureRoomSubscription dependency array; keep roomSubscriptionRef,
cleanupRoomSubscription, subscribeRoomEvents and shouldRefetchRoomState usage
the same but replace calls to refetchRoomState() with
refetchRoomStateRef.current() to avoid recreating ensureRoomSubscription each
render.
In `@src/entities/playlist/api/websocket/publishAddTrack.ts`:
- Line 13: The current call to getSocketClient() in publishAddTrack.ts may
return a client that is not connected; before calling client.publish (in the
publishAddTrack function), check the client's connection state (e.g.,
client.connected or equivalent) and only call publish when connected; if not
connected, either queue the message for retry, await a
client.connect()/reconnect helper, or return a controlled failure/error path so
messages are not silently dropped—update publishAddTrack to guard against
disconnected clients and handle retry/err path accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ce4e89d8-e28c-46df-961e-4605d94893d7
📒 Files selected for processing (7)
src/app/room/[slug]/page.tsxsrc/entities/playlist/api/fetchRoomPlayback.tssrc/entities/playlist/api/websocket/publishAddTrack.tssrc/entities/room/api/websocket/publishJoinRequest.tssrc/entities/room/api/websocket/subscribeRoomEvents.tssrc/features/playlist/add-track/ui/AddTrackAction.tsxsrc/shared/lib/normalizeRoomSlug.ts
✅ Files skipped from review due to trivial changes (1)
- src/entities/room/api/websocket/publishJoinRequest.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/shared/lib/normalizeRoomSlug.ts
- src/entities/room/api/websocket/subscribeRoomEvents.ts
- src/features/playlist/add-track/ui/AddTrackAction.tsx
- src/entities/playlist/api/fetchRoomPlayback.ts
| const ensureRoomSubscription = useCallback( | ||
| (roomSlug: string) => { | ||
| if (roomSubscriptionRef.current?.slug === roomSlug) { | ||
| return; | ||
| } | ||
| if (roomSubscriptionRef.current?.slug === roomSlug) { | ||
| return; | ||
| } | ||
|
|
||
| cleanupRoomSubscription(); | ||
|
|
||
| cleanupRoomSubscription(); | ||
| roomSubscriptionRef.current = { | ||
| slug: roomSlug, | ||
| subscription: subscribeRoomEvents(roomSlug, ({ body }) => { | ||
| if (!body) return; | ||
|
|
||
| roomSubscriptionRef.current = { | ||
| slug: roomSlug, | ||
| subscription: subscribeRoomEvents(roomSlug, ({ body }) => { | ||
| if (!body) return; | ||
| const receivedAt = new Date().toLocaleTimeString(); | ||
|
|
||
| let event: WsEvent; | ||
| try { | ||
| event = JSON.parse(body) as WsEvent; | ||
| } catch { | ||
| return; | ||
| } | ||
| let event: WsEvent; | ||
| try { | ||
| event = JSON.parse(body) as WsEvent; | ||
| } catch { | ||
| setRoomEventLogs((prev) => | ||
| [ | ||
| { | ||
| id: `${Date.now()}-${Math.random()}`, | ||
| type: "INVALID_JSON", | ||
| receivedAt, | ||
| body, | ||
| }, | ||
| ...prev, | ||
| ].slice(0, 20), | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| setLastRoomEventType(event.type); | ||
| setLastRoomEventTime(new Date(event.timestamp).toLocaleTimeString()); | ||
| }), | ||
| }; | ||
| setLastRoomEventType(event.type); | ||
| setLastRoomEventTime(new Date(event.timestamp).toLocaleTimeString()); | ||
| setRoomEventLogs((prev) => | ||
| [ | ||
| { | ||
| id: `${Date.now()}-${event.type}`, | ||
| type: event.type, | ||
| receivedAt, | ||
| body: JSON.stringify(event, null, 2), | ||
| }, | ||
| ...prev, | ||
| ].slice(0, 20), | ||
| ); | ||
|
|
||
| if (shouldRefetchRoomState(event.type)) { | ||
| void refetchRoomState(); | ||
| } | ||
| }), | ||
| }; | ||
| }, | ||
| [cleanupRoomSubscription], | ||
| [cleanupRoomSubscription, refetchRoomState], | ||
| ); |
There was a problem hiding this comment.
ensureRoomSubscription 의존성 배열에 refetchRoomState 포함 문제
refetchRoomState가 매 렌더마다 새 참조를 가지므로, 이 useCallback도 매번 재생성됩니다. 이는 line 237의 useEffect가 ensureRoomSubscription을 의존성으로 갖고 있어 불필요한 cleanup/re-subscription 사이클을 유발할 수 있습니다.
위에서 제안한 useRef 패턴을 적용하여 refetchRoomState를 의존성에서 제거하세요.
제안 수정
+ const refetchRoomStateRef = useRef(refetchRoomState);
+ useEffect(() => {
+ refetchRoomStateRef.current = refetchRoomState;
+ });
const ensureRoomSubscription = useCallback(
(roomSlug: string) => {
// ... existing code ...
if (shouldRefetchRoomState(event.type)) {
- void refetchRoomState();
+ void refetchRoomStateRef.current();
}
}),
};
},
- [cleanupRoomSubscription, refetchRoomState],
+ [cleanupRoomSubscription],
);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/room/`[slug]/page.tsx around lines 102 - 156, The
ensureRoomSubscription useCallback should not depend on the changing
refetchRoomState function; create a stable ref (e.g., refetchRoomStateRef) that
you update whenever refetchRoomState changes, then read
refetchRoomStateRef.current inside ensureRoomSubscription instead of using
refetchRoomState directly so you can remove refetchRoomState from the
ensureRoomSubscription dependency array; keep roomSubscriptionRef,
cleanupRoomSubscription, subscribeRoomEvents and shouldRefetchRoomState usage
the same but replace calls to refetchRoomState() with
refetchRoomStateRef.current() to avoid recreating ensureRoomSubscription each
render.
| <div>current entry id: {currentEntry?.entryId ?? "-"}</div> | ||
| <div>current track videoId: {currentEntry?.track.videoId ?? "-"}</div> | ||
| <div>playback status: {playbackStatus?.status ?? "-"}</div> | ||
| <div>playback currentTime(ms): {playbackStatus?.currentTime ?? "-"}</div> |
There was a problem hiding this comment.
디버그 레이블 단위 불일치
playbackStatus?.currentTime은 초 단위인데(line 270에서 * 1000 변환 수행), 레이블은 (ms)로 표시되어 혼란을 줍니다.
제안 수정
- <div>playback currentTime(ms): {playbackStatus?.currentTime ?? "-"}</div>
+ <div>playback currentTime(sec): {playbackStatus?.currentTime ?? "-"}</div>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <div>playback currentTime(ms): {playbackStatus?.currentTime ?? "-"}</div> | |
| <div>playback currentTime(sec): {playbackStatus?.currentTime ?? "-"}</div> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/app/room/`[slug]/page.tsx at line 284, The label "playback
currentTime(ms):" mismatches the actual unit because playbackStatus?.currentTime
is in seconds (the code multiplies by * 1000 elsewhere to get ms); update the UI
to be consistent by either changing the label to "(s)" to reflect seconds or
convert the displayed value to milliseconds (value * 1000) and keep "(ms)";
locate the JSX div rendering playbackStatus?.currentTime and the spot where
currentTime is transformed (the existing * 1000 conversion) and apply one of
these two fixes so label and value match.
| safeSlug: string, | ||
| payload: AddTrackPayload, | ||
| ): Promise<void> { | ||
| const client = getSocketClient(); |
There was a problem hiding this comment.
연결되지 않은 클라이언트에서 publish 시도 가능
getSocketClient()는 연결 상태와 무관하게 클라이언트 인스턴스를 반환합니다(관련 코드 스니펫 참조). 클라이언트가 아직 연결되지 않았거나 재연결 중일 때 publish를 호출하면 메시지가 손실될 수 있습니다.
연결 상태 확인 또는 client.connected 체크를 추가하는 것을 고려하세요.
제안 수정
const client = getSocketClient();
+ if (!client.connected) {
+ return Promise.reject(new Error("WebSocket 연결이 끊어져 있습니다."));
+ }
const normalizedSlug = normalizeRoomSlug(safeSlug);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/entities/playlist/api/websocket/publishAddTrack.ts` at line 13, The
current call to getSocketClient() in publishAddTrack.ts may return a client that
is not connected; before calling client.publish (in the publishAddTrack
function), check the client's connection state (e.g., client.connected or
equivalent) and only call publish when connected; if not connected, either queue
the message for retry, await a client.connect()/reconnect helper, or return a
controlled failure/error path so messages are not silently dropped—update
publishAddTrack to guard against disconnected clients and handle retry/err path
accordingly.
e202286 to
2c8fa47
Compare
Summary by CodeRabbit
릴리스 노트
New Features
Improvements
Summary by CodeRabbit