feat(lyrics): progressive fill on the word being sung - #492
Conversation
Word-by-word karaoke already worked; what it did was switch states at each word boundary. This sweeps inside the word instead, which is what the Apple Music look actually is (issue #491, idea from discussion #488). Two things decide whether this feels right or broken. player:position is throttled to 4 Hz by the decoder, so painting from positionMs would advance the fill in 250 ms steps -- visibly worse than the discrete highlight it replaces. Each event is treated as an anchor (position + performance.now() when it landed) and every frame extrapolates from it, scaled by playbackSpeed so the sweep still tracks at 0.5x/2x, and frozen while paused so it doesn't drift to the end of the word. Arithmetic checked against a table of boundary cases (start, mid, exact end, past end, before the word, paused, both speeds, and a clock that appears to go backwards). And nothing goes through React state: useTrackLyrics is shared with the side panel and the column renders every line, so a setState per frame would re-render both trees ~60x/s. The loop writes a CSS variable straight onto the element it owns; the ref is attached to the active word only, and moving it is what starts the next sweep. The sung layer is aria-hidden -- the base layer already carries the text, and without it a screen reader would announce each active word twice. Opacity sits on the two layers rather than their parent, since a parent's opacity applies to its whole subtree and would dim the overlay along with the text it's supposed to brighten. Falls back to the discrete highlight under prefers-reduced-motion, for a word with no forward-going endMs (last word of a track, zero-duration stamps), and in the side panel, which keeps the cheap version. Refs #491. Claude-Session: https://claude.ai/code/session_01N9WXrurarkoiT2jgimvyj1
Two gaps in the first cut, both found by re-reading it rather than by running it. The loop stopped once the word was full, but a seek backwards *inside* the same word changes neither the word bounds nor isPlaying -- so the fill would stay pinned at 100 % until the active word changed. It now keeps looping while playback runs; one callback writing one string per frame is cheaper than that bug. And positionMs is now a dependency of the loop effect. While paused no loop is running, and the anchor effect only mutates a ref, so a seek would not have repainted at all. It changes at 4 Hz, so re-running the effect on it is cheap. Refs #491. Claude-Session: https://claude.ai/code/session_01N9WXrurarkoiT2jgimvyj1
Name the effect by what it does — a continuous sweep across the word being sung — instead of by another player. Comment and prose only, no behaviour change. Claude-Session: https://claude.ai/code/session_01N9WXrurarkoiT2jgimvyj1
InstaZDLL
added
scope: frontend
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 23 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughLe changement ajoute le remplissage progressif des mots karaoke dans la vue immersive. Un hook utilise ChangesRemplissage progressif des mots karaoke
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant PlaybackPosition
participant useKaraokeWordFill
participant ImmersiveLyricsColumn
participant WordFillLayer
PlaybackPosition->>useKaraokeWordFill: player:position et playbackSpeed
useKaraokeWordFill->>useKaraokeWordFill: extrapole la position avec requestAnimationFrame
useKaraokeWordFill->>WordFillLayer: écrit --kw-fill
ImmersiveLyricsColumn->>WordFillLayer: rend le texte et la surcouche active
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption. 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/hooks/useKaraokeWordFill.ts`:
- Around line 49-54: Update the anchor-resetting useEffect in useKaraokeWordFill
so it also depends on isPlaying, causing anchorRef.current to be reinitialized
when playback resumes while preserving the existing positionMs behavior.
🪄 Autofix
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: ASSERTIVE
Plan: Pro Plus
Run ID: 670cf99a-2d7f-4fd6-8e7a-e78be0350a2d
📒 Files selected for processing (5)
CLAUDE.mddocs/features/integrations.mdsrc/app.csssrc/components/player/ImmersiveLyricsColumn.tsxsrc/hooks/useKaraokeWordFill.ts
The backend stops emitting player:position while paused, so the anchor's timestamp stayed dated from before the pause. On resume the loop measured the whole pause as elapsed playback: a 30 s pause mid-word slammed the fill from 50 % to 100 % until the next position event landed (≤ 250 ms) — a visible flash on every resume. Reproduced with the same arithmetic harness used for the boundary cases before fixing. Re-anchoring on isPlaying resets `at` to now, so elapsed restarts at zero with the position unchanged. Refs #491. Claude-Session: https://claude.ai/code/session_01N9WXrurarkoiT2jgimvyj1
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/hooks/useKaraokeWordFill.ts (2)
61-67: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRejeter les horodatages non finis.
src/lib/tauri/lyrics.tsdocumenteendMs = +∞pour le dernier mot. La conditionend > startaccepteInfinity. Pour une position finie, la ligne [87] produit alors un ratio égal à0, donc--kw-fillreste à0%au lieu d’utiliser le repli discret prévu pour les timings finaux.Validez la finitude des deux bornes avant d’activer l’animation.
Correction proposée
- const hasSpan = start >= 0 && end > start; + const hasSpan = + Number.isFinite(start) && + Number.isFinite(end) && + start >= 0 && + end > start;Ce constat s’appuie sur le contrat
LyricsWorddesrc/lib/tauri/lyrics.tset sur l’objectif de repli pour les timings finaux.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks/useKaraokeWordFill.ts` around lines 61 - 67, Update the hasSpan calculation in the karaoke word timing logic to require both start and end timestamps to be finite, in addition to start being nonnegative and end being greater than start. This must reject Infinity and preserve the discrete fallback for final-word timings.
90-95: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winArrêter la boucle lorsque le mot est entièrement rempli.
positionMsest déjà une dépendance à la ligne [109]. Une recherche arrière dans le même mot relance donc l’effet, même sistart,endetisPlayingrestent inchangés. La boucle n’a pas besoin de continuer lorsqueclampedvaut1. Elle écrit alors la même valeur à chaque frame pendant les intervalles avant la ligne suivante.Planifiez la frame suivante uniquement si
isPlaying && clamped < 1.Correction proposée
- if (isPlaying) raf = requestAnimationFrame(paint); + if (isPlaying && clamped < 1) { + raf = requestAnimationFrame(paint); + }Ce constat s’appuie sur la dépendance
positionMsdéclarée à la ligne [109] et sur l’objectif de limiter le coût par frame.Also applies to: 105-109
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks/useKaraokeWordFill.ts` around lines 90 - 95, Update the requestAnimationFrame scheduling in the paint loop of useKaraokeWordFill so the next frame is requested only when isPlaying is true and clamped is less than 1. Preserve the existing fill calculation and dependency behavior, including restarting correctly after a backward seek.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/hooks/useKaraokeWordFill.ts`:
- Around line 49-59: Update the anchor management in useKaraokeWordFill to store
the anchor’s playbackSpeed and extrapolate positionMs using the previous speed
before replacing the anchor. Preserve the effective extrapolated position across
speed changes and pauses; when isPlaying resumes, reset only the anchor
timestamp to exclude paused time, without re-anchoring to the potentially stale
event position. Do not simply add playbackSpeed as an effect dependency, and
keep the fill calculation aligned with the updated anchor state.
---
Outside diff comments:
In `@src/hooks/useKaraokeWordFill.ts`:
- Around line 61-67: Update the hasSpan calculation in the karaoke word timing
logic to require both start and end timestamps to be finite, in addition to
start being nonnegative and end being greater than start. This must reject
Infinity and preserve the discrete fallback for final-word timings.
- Around line 90-95: Update the requestAnimationFrame scheduling in the paint
loop of useKaraokeWordFill so the next frame is requested only when isPlaying is
true and clamped is less than 1. Preserve the existing fill calculation and
dependency behavior, including restarting correctly after a backward seek.
🪄 Autofix
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: ASSERTIVE
Plan: Pro Plus
Run ID: c7b46dc1-a383-4e81-8f70-e31aea67b486
📒 Files selected for processing (1)
src/hooks/useKaraokeWordFill.ts
Three review findings, all confirmed. The anchor now stores the speed in force from the moment it was taken, and paint() uses that rather than the live value. Between a speed change and the next position event (up to 250 ms) the old code applied the NEW speed to time that was played at the old one -- on a 500 ms word a 1x->2x switch mis-stated the fill by up to half its width. The anchor also carries the extrapolated position over when the trigger was a speed change or a play/pause, instead of snapping back to the last event's position. That was a visible backwards jump on every pause: the last emitted position can be up to 250 ms behind where the fill had actually reached. A resume still excludes the paused time, because `at` is reset -- which is what the previous commit was reaching for, done without discarding progress. hasSpan now requires finite bounds. `LyricsWord`'s contract allows +∞ for a final word, which passes `end > start` while making every ratio 0, so the word would silently never light up instead of falling back to the discrete highlight. The loop stops again at a full word. It was made unconditional to survive a backward seek inside the same word, but positionMs became a dependency of the effect since, so that case re-runs the effect and repaints -- and stopping saves holding a frame callback through a long word's tail. Replayed against the arithmetic harness: continuity across a 1x->2x switch, pause/resume after 30 s, a backend event overriding drift, and a backward seek. The original boundary table still passes. Refs #491. Claude-Session: https://claude.ai/code/session_01N9WXrurarkoiT2jgimvyj1
1 participant
Closes #491.
Word-by-word karaoke already worked — what it did was switch states at each word boundary. This sweeps inside the word instead, so the highlight advances continuously as the word is sung.
How it renders
Two stacked copies of each active word: the base layer carries the accessible text at unsung opacity, and an
aria-hiddenoverlay (the sung state) is clipped to a--kw-fillpercentage.aria-hiddenbecause the base layer already carries the text — without it, a screen reader would announce every active word twice.The two things that decide whether it feels right
1. The position only arrives at 4 Hz. The decoder throttles
player:positionto one event per 250 ms (POSITION_EMIT_INTERVAL). Painting straight frompositionMswould advance the fill in 250 ms steps — visibly worse than the discrete highlight it replaces. Each event is instead an anchor (position + theperformance.now()when it landed) and every frame extrapolates from it:playbackSpeed, so the sweep still tracks at 0.5× and 2×;Arithmetic verified against a table of boundary cases: start, mid-word, exact end, past the end, before the word, paused, both speed extremes, and a clock that appears to run backwards.
2. Nothing goes through React state.
useTrackLyricsis shared by the immersive column and the side panel, and the column renders every line — asetStateper frame would re-render both trees ~60×/s.useKaraokeWordFillwrites the CSS variable straight onto the one element it owns. The ref is attached to the active word only; moving it is what starts the next sweep.Fallbacks
All landing on the existing discrete highlight, unchanged:
prefers-reduced-motion;endMs— the last word of a track keeps-1, and sloppy sources can stamp two words at the same millisecond (no division by zero);LyricsPanel, which deliberately keeps the cheap version — far smaller surface, and the issue left the call open.Two bugs fixed before this was pushed
Both found by re-reading the code, not by running it:
isPlaying, so the fill would have stayed pinned at 100 % until the active word changed. It now loops while playback runs.positionMsis a dependency of the loop effect. While paused no loop runs, and the anchor effect only mutates a ref, so a seek would not have repainted at all.Checks
bun run typecheck✅ ·bun run lint✅ ·bun run build✅clip-path: inset(0 calc(100% - var(--kw-fill,0%)) 0 0)intact through Lightning CSS, default value included — checked explicitly, since this repo has had a CSS regression that only appeared in a release build.ImmersiveView,ImmersiveSidePanel) mount underPlayerProvider, which the new hook requires; the mini-player webview renders no lyrics.Not verified
The visual result. This is an animation and I can't watch it run — smoothness, how legible the sweep is against each theme, and whether the unsung/sung contrast reads well are all open. Worth a pass on a track with word-level lyrics before merge.
Also left out on purpose: the optional extras the issue listed under "decide how far to go" (glow on the active word, vertical spring per line, blur on inactive lines). The fill was the substance; those are cosmetic and are better judged by eye than specified blind.
Note
Right-to-left lyrics fill from the wrong edge — the clip reveals left-to-right. The word-level highlight already had that limitation, so this is not a regression; it deserves its own issue rather than a half-fix here.
Summary by CodeRabbit