Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughLink 컴포넌트에 onClick을 추가해 외부 클릭 처리를 허용하고, 플레이리스트 항목 클릭 시 해당 인덱스로 재생하도록 연결했습니다. CD 래퍼에 재생 상태 기반 회전 애니메이션을 추가했고, YouTube 플레이어는 ref/초기 로드 가드로 로드 흐름을 분리했습니다. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant PI as PlaylistInfo
participant PP as PlayerProvider
participant YT as YoutubePlayer
U->>PI: 트랙 Link 클릭(index)
PI->>PP: setPlaylist(currentPlaylist, index, 0)
note right of PP: 재생 인덱스/플레이리스트 업데이트
PP-->>YT: props로 videoId/상태 전달
YT->>YT: (초기 이후) loadVideoById(videoId)
sequenceDiagram
autonumber
participant PP as PlayerProvider
participant PL as PlaylistLayout
participant CD as CdWrapper
PP-->>PL: isPlaying 상태 전달
PL->>CD: <CdWrapper $isPlaying={isPlaying}>
alt isPlaying = true
CD->>CD: spin 애니메이션 재생
else isPlaying = false
CD->>CD: 애니메이션 일시정지
end
sequenceDiagram
autonumber
participant YT as YoutubePlayer
participant API as YouTube IFrame API
YT->>API: mount player (playerVars: mute:0, playsinline:1)
API-->>YT: onReady(event, player)
YT->>YT: player ref에 저장
loop videoId 변경(초기 제외)
YT->>API: loadVideoById(videoId)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 |
Summary of ChangesHello @maylh, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 이 PR은 플레이어의 전반적인 사용자 경험을 개선하기 위한 여러 변경 사항을 포함합니다. 재생 중인 CD 애니메이션 추가, 플레이리스트에서 특정 곡을 선택하여 재생하는 기능, 그리고 모바일 환경에서 음소거 해제 상태를 유지하는 기능 등을 통해 사용자가 더욱 직관적이고 편리하게 플레이어를 이용할 수 있도록 합니다. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
🎵 Storybook Link 🎵 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
안녕하세요. 코드 리뷰를 시작하겠습니다.
이번 PR은 플레이어 사용성 개선을 위한 변경 사항들이 포함되어 있네요. CD 회전 애니메이션, 특정 곡부터 재생 기능, 모바일 unmute 상태 유지 등 사용자 경험을 크게 향상시킬 수 있는 좋은 시도들입니다.
코드 리뷰 결과, 몇 가지 개선점을 발견했습니다.
PlaylistInfo.tsx에서 특정 곡을 클릭하여 재생을 시작하는 로직에 현재 재생 중인 플레이리스트를 사용하는 버그가 있어, 보고 있는 플레이리스트 기준으로 재생되도록 수정을 제안했습니다.YoutubePlayer.tsx에서 새로운 곡을 불러올 때 플레이어 상태(음소거 등)가 초기화될 수 있는 잠재적인 문제를 발견하여, 이를 방지하기 위한 수정을 제안했습니다.- 추가적으로 React의
keyprop 사용에 대한 작은 개선 사항도 포함했습니다.
자세한 내용은 각 파일의 주석을 참고해주세요.
| const handleClickTrack = (trackIndex: number) => { | ||
| if (!currentPlaylist) return | ||
| navigate(-1) | ||
| setPlaylist(currentPlaylist, trackIndex, 0) | ||
| } |
There was a problem hiding this comment.
플레이리스트 상세 페이지에서 특정 트랙을 클릭했을 때의 재생 로직에 치명적인 오류가 있습니다.
현재 코드는 usePlaylist에서 가져온 currentPlaylist를 사용하여 setPlaylist를 호출하고 있습니다. 이 경우, 사용자가 보고 있는 플레이리스트(playlistData)가 아닌 현재 재생 중인 플레이리스트의 트랙이 변경됩니다. 만약 아무것도 재생 중이지 않다면 currentPlaylist가 null이므로 아무 동작도 하지 않게 됩니다.
사용자가 보고 있는 playlistData를 기준으로 플레이리스트를 설정하도록 수정해야 합니다. playlistData의 타입(PlaylistDetailResponse)과 setPlaylist가 요구하는 타입(PlaylistInfo)이 다르므로, 필요한 형태로 객체를 변환하여 전달해야 합니다.
const handleClickTrack = (trackIndex: number) => {
if (!playlistData) return
const playlistToPlay: PlaylistInfo = {
...playlistData,
creator: {
creatorId: playlistData.creatorId,
creatorNickname: playlistData.creatorNickname,
},
}
navigate(-1)
setPlaylist(playlistToPlay, trackIndex, 0)
}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
src/shared/ui/Link.tsx (2)
9-9: onClick 타입 명시 + 키보드 접근성 보강
- 이벤트 객체를 받을 수 있도록 onClick 타입을 MouseEventHandler로 명시해주세요.
- div를 클릭 가능한 요소로 사용할 때는 role/tabIndex/키보드 핸들링이 필요합니다. Enter/Space로도 활성화되도록 onKeyDown을 추가하는 것을 권장합니다.
적용 diff:
- onClick?: () => void + onClick?: React.MouseEventHandler<HTMLDivElement>- <LinkBox $variant={variant} onClick={onClick}> + <LinkBox + $variant={variant} + onClick={onClick} + role="button" + tabIndex={0} + onKeyDown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault() + ;(e.currentTarget as HTMLElement).click() + } + }} + >추가 타입 import(선택):
import type { MouseEventHandler } from 'react'Also applies to: 14-15
29-33: 가능하면 button 요소로 전환 고려현재 LinkBox가 div라 기본 키보드 액션이 없습니다. styled.button(type="button")으로 전환하면 기본 a11y와 포커스/키보드 동작을 자연스럽게 확보할 수 있습니다. 스타일 초기화(appearance: none; background: transparent; border: 0; padding: 0; cursor: pointer;)만 추가하면 됩니다.
src/widgets/playlist/PlaylistInfo.tsx (2)
23-27: 네비게이션 순서 조정setPlaylist가 먼저, navigate(-1)가 다음이 더 안전합니다. 상태 업데이트가 UI 전환보다 먼저 반영되어 재생 화면으로 돌아갔을 때 즉시 올바른 트랙/시간으로 반영됩니다.
적용 diff:
- navigate(-1) - setPlaylist(currentPlaylist, trackIndex, 0) + setPlaylist(currentPlaylist, trackIndex, 0) + navigate(-1)
61-66: React key로 index 사용 지양가능하면 트랙의 고유 ID(예: youtubeId 또는 songId)를 key로 사용하세요. 재정렬/삽입 시 불필요한 리마운트/상태 손실을 줄일 수 있습니다.
src/widgets/playlist/PlaylistLayout.tsx (1)
133-147: keyframes 이름 충돌 방지 + 모션 감소 선호도 대응
- 전역 @Keyframes 'spin'은 충돌 소지가 있습니다. styled-components의 keyframes 헬퍼를 사용해 이름을 스코프화하세요.
- prefers-reduced-motion을 존중해 모션 감소 환경에서는 애니메이션을 비활성화하는 것이 좋습니다.
적용 diff(해당 블록 내):
- animation: spin 40s linear infinite; + animation: ${spin} 40s linear infinite; animation-play-state: ${(props) => (props.$isPlaying ? 'running' : 'paused')}; transform-origin: center; - @keyframes spin { - 0% { - transform: rotate(0deg); - } - 100% { - transform: rotate(360deg); - } - } + @media (prefers-reduced-motion: reduce) { + animation: none; + }블록 외 추가 코드:
// 상단 import 교체 import styled, { keyframes } from 'styled-components' // spin 정의 const spin = keyframes` 0% { transform: rotate(0deg); } 100% { transform: rotate(360deg); } `
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/shared/ui/Link.tsx(1 hunks)src/widgets/playlist/PlaylistInfo.tsx(3 hunks)src/widgets/playlist/PlaylistLayout.tsx(2 hunks)src/widgets/playlist/YoutubePlayer.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js,jsx}
⚙️ CodeRabbit configuration file
**/*.{ts,tsx,js,jsx}: ## 1. 일반적인 코딩 컨벤션포맷팅
.prettierrc설정에 따라 포맷팅 확인- 들여쓰기: 2칸 스페이스
- 최대 줄 길이: 100자
- 세미콜론 사용 안함
- 따옴표: 작은따옴표 사용
- 괄호 안 공백: 있음
- 화살표 함수 괄호: 항상 사용
- 줄바꿈: LF 사용
네이밍 컨벤션
- 컴포넌트: PascalCase (예: UserProfile)
- 유틸리티/훅/변수: camelCase (예: getUserData, useUserInfo)
- 상수: UPPER_SNAKE_CASE (예: API_BASE_URL)
- 이미지 파일: kebab-case (예: user-profile-icon.png)
주석 사용
- 복잡한 로직에만 주석 추가
- 불필요한 주석 지양 (코드로 설명 가능한 것)
- TODO/FIXME 형식:
// TODO: 설명 - 작성자가독성
- 매직 넘버 지양, 의미있는 상수 사용
- 함수는 하나의 책임만 가지도록 작성 (최대 20줄 권장)
- 중첩 깊이 최소화 (3단계 이하 권장)
2. React 모범 사례
컴포넌트 작성
- 최신 React hooks 사용 권장
- 컴포넌트는 단일 책임 원칙 준수
- Presentational/Container 컴포넌트 분리
- 성능 최적화: memo, useCallback, useMemo 적절히 사용
- 대용량 리스트는 가상화 라이브러리 사용 고려
상태 관리
- Zustand와 Tanstack Query를 일관되게 사용
- 상태 구조는 정규화된 형태로 관리
- 에러 처리: Error Boundary와 try-catch 또는 onError 콜백 활용
3. 스타일링
Styled Components
- Styled Components 일관되게 사용
- 스타일드 컴포넌트명은 의미있게 작성
- 동적 스타일링은 props나 CSS 변수 활용
- 테마 시스템 활용하여 글로벌 스타일 관리
- 재사용 가능한 스타일은 mixin이나 확장으로 관리
- CSS 포맷팅 가독성 유지
- 사용하지 않는 스타일이나 중복 스타일 제거
4. Vite 및 빌드 최적화
- 모듈 import 최적화 (tree-shaking 고려)
- 환경변수는 .env 파일로 관리
- vite.config.ts에서 빌드 성능 튜닝 (sourcemap 설정, 플러그인 최적화 등)
5. 아키텍처 및 개발 환경
폴더 구조
- Feature-Sliced Design (FSD) 구조 준수
- 레이어별 참조 규칙 엄격히 적용
타입스크립트
- strict 모드 사용
- 타입 명시적으로 작성 (any 사용 지양)
- path alias (@/_) 절대 경로 import 사용
Git 훅
- Husky 설정으로 pre-commit, commit-msg 린팅 확인
6. 기타 가이드라인
- 충분한 근거와 함께 리뷰 제공
- 정보 검증 후 답변
- 간결하고 명확한 응답
- 필요시 추가 컨텍스트 요청
- 검증되지 않은 주장 지양
- 가능한 경우 출처 명시
- 별도 언급 없으면 JavaScript 기준
- 한국어로 응답
- 대부분 브라우저에서 지원하는 ES6+ 기능 활용
- 코드 리뷰를 통한 유지보수성 향상에 적극 활용
Files:
src/shared/ui/Link.tsxsrc/widgets/playlist/PlaylistLayout.tsxsrc/widgets/playlist/PlaylistInfo.tsxsrc/widgets/playlist/YoutubePlayer.tsx
🧠 Learnings (2)
📓 Common learnings
Learnt from: maylh
PR: dnd-side-project/dnd-13th-8-frontend#55
File: src/widgets/playlist/BackgroundPlayer.tsx:55-83
Timestamp: 2025-08-20T05:59:48.729Z
Learning: BackgroundPlayer 컴포넌트에서 네비게이션 후 컨트롤 버튼이 작동하지 않는 버그는 주로 다음 원인들로 발생한다: 1) DOM ID 충돌 (고정값 'yt-player' 사용), 2) window.YT가 이미 존재할 때 새 플레이어를 생성하지 않음, 3) onStateChange 의존성으로 인한 useEffect 재실행과 전역 콜백 덮어쓰기, 4) 리마운트 시 ref 상태 불일치. 해결책으로는 고유 DOM ID 생성, initPlayer 함수 분리를 통한 즉시 초기화, 콜백 ref 사용으로 의존성 최소화, 플레이리스트 변경 시 상태 초기화 등이 필요하다.
📚 Learning: 2025-08-20T05:59:48.729Z
Learnt from: maylh
PR: dnd-side-project/dnd-13th-8-frontend#55
File: src/widgets/playlist/BackgroundPlayer.tsx:55-83
Timestamp: 2025-08-20T05:59:48.729Z
Learning: BackgroundPlayer 컴포넌트에서 네비게이션 후 컨트롤 버튼이 작동하지 않는 버그는 주로 다음 원인들로 발생한다: 1) DOM ID 충돌 (고정값 'yt-player' 사용), 2) window.YT가 이미 존재할 때 새 플레이어를 생성하지 않음, 3) onStateChange 의존성으로 인한 useEffect 재실행과 전역 콜백 덮어쓰기, 4) 리마운트 시 ref 상태 불일치. 해결책으로는 고유 DOM ID 생성, initPlayer 함수 분리를 통한 즉시 초기화, 콜백 ref 사용으로 의존성 최소화, 플레이리스트 변경 시 상태 초기화 등이 필요하다.
Applied to files:
src/widgets/playlist/YoutubePlayer.tsx
🧬 Code graph analysis (1)
src/widgets/playlist/PlaylistInfo.tsx (1)
src/app/providers/PlayerProvider.tsx (1)
usePlaylist(101-105)
🔇 Additional comments (2)
src/widgets/playlist/YoutubePlayer.tsx (2)
19-21: 모바일 자동재생 정책 점검 필요 (mute: 0 기본값)iOS/Safari 등에서 음소거 해제 상태(mute: 0)로 autoplay가 차단될 수 있습니다. 최초 로드만 mute: 1로 시작하고, 사용자 상호작용 후 unMute 상태를 유지하는 흐름을 실제 기기에서 확인해주세요.
테스트 권장 시나리오:
- iOS Safari/Chrome에서 최초 진입 자동재생 여부
- 첫 unmute 이후 다른 videoId로 전환 시 unmute 상태 유지 여부
41-44: onReady에서 playerRef 설정 LGTM인스턴스를 ref에 고정하고 상위 onReady에 정상 위임하고 있어 이후 제어가 일관됩니다.
| const handleClickTrack = (trackIndex: number) => { | ||
| if (!currentPlaylist) return | ||
| navigate(-1) | ||
| setPlaylist(currentPlaylist, trackIndex, 0) | ||
| } |
There was a problem hiding this comment.
선택한 트랙 재생 시 원본 플레이리스트 소스 확인 필요 (현재 로직은 currentPlaylist만 사용)
상세 화면에서 클릭한 트랙의 인덱스로 재생하려면, currentPlaylist가 상세의 playlistData와 동일하지 않은 경우에도 해당 상세 플레이리스트를 소스로 설정해야 합니다. 현 로직은 currentPlaylist가 없으면 아무 작업도 하지 않고, 다른 플레이리스트가 활성화된 상태에서는 잘못된 인덱스로 점프할 수 있습니다.
권장: currentPlaylist?.playlistId !== playlistData.playlistId인 경우 playlistData를 Player가 요구하는 타입으로 변환하여 setPlaylist에 전달하세요. 또한 setPlaylist 이후 navigate(-1) 호출 순서를 권장합니다.
예시(개념적):
const handleClickTrack = (trackIndex: number) => {
if (!playlistData) return
const source =
currentPlaylist?.playlistId === playlistData.playlistId
? currentPlaylist
: transformDetailToPlaylistInfo(playlistData) // 변환 함수 필요
setPlaylist(source, trackIndex, 0)
navigate(-1)
}🤖 Prompt for AI Agents
In src/widgets/playlist/PlaylistInfo.tsx around lines 23 to 27, the click
handler currently only uses currentPlaylist and returns early if it's missing,
which can cause wrong-track jumps when the detailed view's playlistData differs;
change the logic to first ensure playlistData exists, then choose the source: if
currentPlaylist?.playlistId === playlistData.playlistId use currentPlaylist,
otherwise convert playlistData into the Player's PlaylistInfo shape (implement a
transformDetailToPlaylistInfo or similar) and pass that to setPlaylist; call
setPlaylist(source, trackIndex, 0) before navigate(-1) so the player is set to
the correct track, and remove the early return on missing currentPlaylist in
favor of checking playlistData.
| useEffect(() => { | ||
| if (!isFirstLoad.current && playerRef.current && videoId) { | ||
| // 첫 로딩 제외하고 loadVideoById 호출 | ||
| try { | ||
| playerRef.current.loadVideoById(videoId) | ||
| } catch (e) { | ||
| console.error('loadVideoById 실패', e) | ||
| } | ||
| } | ||
| isFirstLoad.current = false | ||
| }, [videoId]) | ||
|
|
There was a problem hiding this comment.
videoId 변경 시 이중 load 위험 (react-youtube가 이미 처리함)
react-youtube는 videoId prop 변경 시 내부적으로 loadVideoById를 호출합니다. 현재 useEffect에서 동일 호출을 반복해 깜빡임/상태 경합이 생길 수 있습니다. 이 효과를 제거하는 것을 권장합니다.
적용 diff:
- useEffect(() => {
- if (!isFirstLoad.current && playerRef.current && videoId) {
- // 첫 로딩 제외하고 loadVideoById 호출
- try {
- playerRef.current.loadVideoById(videoId)
- } catch (e) {
- console.error('loadVideoById 실패', e)
- }
- }
- isFirstLoad.current = false
- }, [videoId])
+ // react-youtube가 videoId 변경을 내부적으로 처리하므로 별도의 load 호출은 불필요합니다.추가로 isFirstLoad ref는 불필요해집니다(정리 권장).
📝 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.
| useEffect(() => { | |
| if (!isFirstLoad.current && playerRef.current && videoId) { | |
| // 첫 로딩 제외하고 loadVideoById 호출 | |
| try { | |
| playerRef.current.loadVideoById(videoId) | |
| } catch (e) { | |
| console.error('loadVideoById 실패', e) | |
| } | |
| } | |
| isFirstLoad.current = false | |
| }, [videoId]) | |
| // react-youtube가 videoId 변경을 내부적으로 처리하므로 별도의 load 호출은 불필요합니다. |
🤖 Prompt for AI Agents
In src/widgets/playlist/YoutubePlayer.tsx around lines 24 to 35, remove the
useEffect that calls playerRef.current.loadVideoById on videoId changes because
react-youtube already handles loading when its videoId prop changes; delete the
entire effect block and also remove the isFirstLoad ref and any references to it
(cleanup its declaration and the isFirstLoad.current assignment) so we don't
duplicate behavior or keep unused state.
2 participants
🛰️ 관련 이슈
✨ 주요 변경 사항
11차 회의에서 결정된 플레이어 사용성 개선 사항을 반영했습니다.
🔍 테스트 방법 / 체크리스트
🗯️ PR 포인트
🚀 알게된 점
animation-play-state속성📖 참고 자료 (선택)
Summary by CodeRabbit
New Features
Refactor