feat: lyrics style#70
Conversation
SafeDep Report Summary
No dependency changes detected. Nothing to scan. This report is generated by SafeDep Github App |
|
Caution Review failedThe pull request is closed. WalkthroughRefactors player into a tabbed "Main"/"Lyrics" UI, replaces TrackPlayer progress listeners with a sticky progress emitter, adds animated lyric rendering and async seek/coalescing, adjusts several component props/signatures, introduces new progress emitter and sticky-mitt subscribe API, bumps versionCode to 72 and updates docs. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PlayerPage
participant TabView
participant PlayerMainTab
participant PlayerLyrics
User->>PlayerPage: Open player
PlayerPage->>TabView: Render tabs (index state)
TabView->>PlayerMainTab: Render when index=0
PlayerMainTab->>PlayerMainTab: Load cover, controls, slider
PlayerMainTab->>User: Display main playback UI
User->>TabView: Switch to Lyrics tab (index=1)
TabView->>PlayerLyrics: Render lyrics
PlayerLyrics->>PlayerLyrics: Fetch track & lyrics, animate lines
PlayerLyrics->>User: Display animated lyrics and offset UI
sequenceDiagram
participant TrackPlayer as TrackPlayer (native)
participant Emitter as playerProgressEmitter
participant Hook as Hook (useLyricSync / usePlayerSlider / etc.)
TrackPlayer->>TrackPlayer: PlaybackProgressUpdated fires
TrackPlayer->>Emitter: Listener maps payload -> progress shape
Emitter->>Emitter: emitSticky('progress', data)
Hook->>Emitter: subscribe('progress', handler)
Emitter->>Hook: deliver latest or future progress
Hook->>Hook: update local state (position, lyric index, UI)
Hook->>Emitter: unsubscribe via returned function on cleanup
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas needing extra attention:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/features/player/hooks/useLyricSync.ts (1)
17-77: Prevent stale lyric highlight after overlapping seeks.When the user taps multiple lines quickly, each
handleJumpToLyriccall kicks off aseekTo. Those promises can resolve out of order, so an older request may finish last and revertcurrentLyricIndexto an obsolete value, making the highlight jump back even though playback has already moved on. Please guard the state update so only the latest jump wins (e.g., track a monotonically increasing request id or move the state update ahead of the await).diff @@ - const manualScrollTimeoutRef = useRef<number | null>(null) + const manualScrollTimeoutRef = useRef<number | null>(null) + const latestJumpRequestRef = useRef(0) @@ - async (index: number) => { + async (index: number) => { if (lyrics.length === 0) return if (!lyrics[index]) return - await seekTo(lyrics[index].timestamp) + const requestId = ++latestJumpRequestRef.current + await seekTo(lyrics[index].timestamp) + if (latestJumpRequestRef.current !== requestId) return setCurrentLyricIndex(index) if (manualScrollTimeoutRef.current) { clearTimeout(manualScrollTimeoutRef.current) manualScrollTimeoutRef.current = null }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
CHANGELOG.md(1 hunks)package.json(1 hunks)src/app/player.tsx(2 hunks)src/features/player/components/PlayerFunctionalMenu.tsx(2 hunks)src/features/player/components/PlayerHeader.tsx(2 hunks)src/features/player/components/PlayerLyrics.tsx(16 hunks)src/features/player/components/PlayerMainTab.tsx(1 hunks)src/features/player/components/PlayerTrackInfo.tsx(2 hunks)src/features/player/hooks/useLyricSync.ts(3 hunks)src/features/player/hooks/usePlayerSlider.ts(3 hunks)src/hooks/player/useAnimatedTrackProgress.ts(2 hunks)src/hooks/player/useTrackProgress.ts(3 hunks)src/hooks/stores/usePlayerStore.ts(2 hunks)src/lib/player/progressListener.ts(1 hunks)src/utils/sticky-mitt.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
src/features/player/components/PlayerFunctionalMenu.tsx (1)
src/hooks/stores/useModalStore.ts (2)
openModal(74-74)useModalStore(29-72)
src/features/player/components/PlayerMainTab.tsx (3)
src/features/player/components/PlayerTrackInfo.tsx (1)
TrackInfo(17-154)src/features/player/components/PlayerSlider.tsx (1)
PlayerSlider(71-115)src/features/player/components/PlayerControls.tsx (1)
PlayerControls(7-122)
src/features/player/components/PlayerLyrics.tsx (2)
src/hooks/queries/lyrics/index.ts (1)
useSmartFetchLyrics(14-36)src/hooks/stores/useModalStore.ts (1)
useModalStore(29-72)
src/features/player/hooks/useLyricSync.ts (1)
src/types/player/lyrics.ts (1)
LyricLine(3-16)
src/features/player/hooks/usePlayerSlider.ts (1)
src/hooks/stores/usePlayerStore.ts (1)
usePlayerStore(46-735)
src/app/player.tsx (1)
src/features/player/components/PlayerHeader.tsx (1)
PlayerHeader(6-52)
🔇 Additional comments (8)
package.json (1)
5-5: LGTM!Version code increment aligns with the PR's feature additions.
src/features/player/components/PlayerTrackInfo.tsx (1)
83-83: LGTM!Consistent border radius applied to both the gradient placeholder and cover image improves visual polish.
Also applies to: 104-104
src/hooks/stores/usePlayerStore.ts (1)
504-513: Verify progress fetch timing for accuracy.The current implementation fetches progress before seeking, then emits the new position with the old
durationandbufferedvalues. This provides immediate feedback but might be briefly inaccurate if these values change during the seek operation.Consider whether fetching progress after
TrackPlayer.seekTo(position)would better reflect the actual player state, or confirm that immediate emission with pre-seek values is intentional for responsiveness.src/hooks/player/useAnimatedTrackProgress.ts (1)
1-1: LGTM!Clean migration from TrackPlayer events to the centralized progress emitter. The unsubscribe pattern is correctly implemented.
Also applies to: 26-34
src/features/player/components/PlayerHeader.tsx (1)
6-12: LGTM!The refactor from
viewMode/trackTitleprops to a singleindexprop simplifies the component API while maintaining clear rendering logic for different tab states.Also applies to: 39-43
src/utils/sticky-mitt.ts (1)
90-103: LGTM!The
subscribemethod provides a clean convenience API that correctly inherits sticky behavior from the underlyingon()method and returns an unsubscribe function for easy cleanup.src/features/player/hooks/usePlayerSlider.ts (2)
12-12: Good type correction.Changing from
NodeJS.Timeouttonumbercorrectly reflects that React Native'ssetTimeoutreturns a number rather than a NodeJS Timeout object.
26-31: LGTM!The migration to the progress emitter and centralizing seek through
usePlayerStorealigns with the PR's architectural improvements, enabling consistent progress updates across the application.Also applies to: 33-33, 63-63
1 participant
Summary by CodeRabbit
New Features
Style
Chores
Documentation