feat(lyrics-plus): add idling indicator for pauses and UI enhancements#3726
feat(lyrics-plus): add idling indicator for pauses and UI enhancements#3726ianz56 wants to merge 6 commits intospicetify:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdded public Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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.
🧹 Nitpick comments (1)
CustomApps/lyrics-plus/Pages.js (1)
85-94: Gap detection only works for Musixmatch karaoke lyrics.The gap detection logic at lines 85-94 depends on
line.endTime, which is only provided by the Musixmatch karaoke provider (seeProviderMusixmatch.js:282-291). Synced lyrics from Spotify and Netease providers only havestartTime, so this code path will never execute for those sources.This isn't a bug since it's properly guarded with
line.endTime != null, but users might expect pause indicators to appear for long instrumental sections in all synced lyrics, not just Musixmatch karaoke.Consider adding a comment documenting this limitation, or estimating gaps using consecutive
startTimevalues whenendTimeis unavailable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CustomApps/lyrics-plus/Pages.js` around lines 85 - 94, The gap detection currently only runs when line.endTime is present, so LONG_PAUSE_THRESHOLD-based pause markers never appear for providers that only supply startTime (e.g., Spotify/Netease); update the logic in the block using line.endTime/nextLine.startTime (the code around the check with isPauseLine and result.push) to fall back to estimating the previous line's end by using the nextLine.startTime of the current line pair (e.g., treat previous endTime as the current line's startTime when endTime is null) and compare that estimated gap to LONG_PAUSE_THRESHOLD, or if you prefer the minimal change, add a clear comment above the existing if explaining the limitation and recommending the estimation approach for future work; ensure you reference LONG_PAUSE_THRESHOLD, isPauseLine, line.endTime, nextLine.startTime and the result.push call when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@CustomApps/lyrics-plus/Pages.js`:
- Around line 85-94: The gap detection currently only runs when line.endTime is
present, so LONG_PAUSE_THRESHOLD-based pause markers never appear for providers
that only supply startTime (e.g., Spotify/Netease); update the logic in the
block using line.endTime/nextLine.startTime (the code around the check with
isPauseLine and result.push) to fall back to estimating the previous line's end
by using the nextLine.startTime of the current line pair (e.g., treat previous
endTime as the current line's startTime when endTime is null) and compare that
estimated gap to LONG_PAUSE_THRESHOLD, or if you prefer the minimal change, add
a clear comment above the existing if explaining the limitation and recommending
the estimation approach for future work; ensure you reference
LONG_PAUSE_THRESHOLD, isPauseLine, line.endTime, nextLine.startTime and the
result.push call when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 57072133-afe3-428e-963e-a1cb98e781d9
📒 Files selected for processing (2)
CustomApps/lyrics-plus/Pages.jsCustomApps/lyrics-plus/style.css
There was a problem hiding this comment.
🧹 Nitpick comments (2)
CustomApps/lyrics-plus/Pages.js (2)
128-132: Consider using a separate accumulator variable instead of mutatingstartTimeThe
startTimeparameter is being reassigned within the loop to track cumulative word timing. While this works correctly (primitives are passed by value in JavaScript), mutating a function parameter can be confusing for maintainability. Using a separate variable likeaccumulatedTimewould clarify intent.♻️ Suggested refactor for clarity
return text.map(({ word, time }, i) => { const isRTL = isRTLText(typeof word === "string" ? word : ""); - const isWordActive = position >= startTime; - startTime += time; - const isWordComplete = isWordActive && position >= startTime; + const wordStartTime = startTime; + startTime += time; // accumulate for next iteration + const isWordActive = position >= wordStartTime; + const isWordComplete = isWordActive && position >= startTime;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CustomApps/lyrics-plus/Pages.js` around lines 128 - 132, The loop mutates the function parameter startTime inside text.map which reduces clarity; introduce a new local accumulator (e.g., accumulatedTime) initialized to startTime before the map and use/update accumulatedTime inside the map to compute isWordActive and isWordComplete and to add each word's time, leaving the original startTime parameter unchanged and maintaining the same comparisons in isWordActive/isWordComplete and subsequent logic (identify usages in the map callback where isWordActive, isWordComplete and the startTime increment occur).
43-54: Minor: Consider defensive type check before calling.trim()If
text?.props?.children?.[0]resolves to a non-string truthy value (e.g., a nested React element), calling.trim()on line 53 would throw a TypeError. Given that lyrics data comes from multiple providers, a defensive check could prevent edge-case crashes.♻️ Suggested defensive fix
const isPauseLine = (text) => { if (!text) return true; if (Array.isArray(text)) { const joined = text .map((w) => (typeof w === "object" ? w.word : w)) .join("") .trim(); return joined === "♪" || joined === ""; } const str = typeof text === "object" ? text?.props?.children?.[0] : text; - return !str || str.trim() === "♪" || str.trim() === ""; + return !str || (typeof str === "string" && (str.trim() === "♪" || str.trim() === "")); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CustomApps/lyrics-plus/Pages.js` around lines 43 - 54, The isPauseLine function may call .trim() on a non-string when text is an object (e.g., a React element); update the logic to defensively coerce or verify str is a string before trimming: compute str = typeof text === "object" ? text?.props?.children?.[0] : text, then if typeof str !== "string" convert it to a safe string (e.g., String(str) or extract inner text) or bail out (treat as pause) before calling str.trim(); apply this change inside isPauseLine so .trim() is only invoked on confirmed string values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@CustomApps/lyrics-plus/Pages.js`:
- Around line 128-132: The loop mutates the function parameter startTime inside
text.map which reduces clarity; introduce a new local accumulator (e.g.,
accumulatedTime) initialized to startTime before the map and use/update
accumulatedTime inside the map to compute isWordActive and isWordComplete and to
add each word's time, leaving the original startTime parameter unchanged and
maintaining the same comparisons in isWordActive/isWordComplete and subsequent
logic (identify usages in the map callback where isWordActive, isWordComplete
and the startTime increment occur).
- Around line 43-54: The isPauseLine function may call .trim() on a non-string
when text is an object (e.g., a React element); update the logic to defensively
coerce or verify str is a string before trimming: compute str = typeof text ===
"object" ? text?.props?.children?.[0] : text, then if typeof str !== "string"
convert it to a safe string (e.g., String(str) or extract inner text) or bail
out (treat as pause) before calling str.trim(); apply this change inside
isPauseLine so .trim() is only invoked on confirmed string values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 192e03ae-fd1b-468b-8f3f-2e3f4697f15c
📒 Files selected for processing (1)
CustomApps/lyrics-plus/Pages.js
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
CustomApps/lyrics-plus/Pages.js (1)
265-275: Extract duplicated pause-indicator construction into a helper.Both synced views build the same pause indicator block. Pulling this into one function reduces drift risk and keeps pause behavior consistent across compact/expanded modes.
Also applies to: 611-621
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CustomApps/lyrics-plus/Pages.js` around lines 265 - 275, Extract the duplicated pause-indicator construction into a single helper function (e.g., getPauseIndicator or buildPauseIndicator) that accepts the inputs used in the calculation (lyricWithEmptyLines, lineNumber, startTime, position, and isFocusedLine/isPause) and returns either the IdlingIndicator element or null; inside the helper reuse findNextLineStartTime to compute nextStart, pauseStart, pauseDuration, progress and delay (delay = pauseDuration / 3), and create the react.createElement(IdlingIndicator, { progress, delay }) when appropriate; replace both inline blocks (the current block around IdlingIndicator and the similar one at the other location) with calls to this helper so both synced views use the same logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CustomApps/lyrics-plus/Pages.js`:
- Around line 199-224: The window math is doing string concatenation because
CONFIG.visual["lines-before"] and ["lines-after"] may be strings; update the
calculations in the useMemo (where targetBefore and targetAfter are computed) to
coerce those config values to numbers (e.g., use
Number(CONFIG.visual["lines-before"]) or parseInt(...,10) before adding 1) so
targetBefore and targetAfter are numeric; also apply the same numeric coercion
where the same config values are used elsewhere (e.g., the related padding logic
that affects activeLines/activeElementIndex) to prevent the string + number bug.
- Around line 543-545: The scroll effect that reads initialScroll.current and
runs on activeLineIndex should also react to lyric content changes: update the
dependency array of the useEffect that handles scrolling (the effect referencing
activeLineIndex and initialScroll.current) to include lyricsId (or the stable
identifier for lyrics) in addition to activeLineIndex so the effect re-runs when
lyrics change even if activeLineIndex remains the same; locate the useEffect
that performs scrolling and adjust its deps from [activeLineIndex] to
[activeLineIndex, lyricsId].
---
Nitpick comments:
In `@CustomApps/lyrics-plus/Pages.js`:
- Around line 265-275: Extract the duplicated pause-indicator construction into
a single helper function (e.g., getPauseIndicator or buildPauseIndicator) that
accepts the inputs used in the calculation (lyricWithEmptyLines, lineNumber,
startTime, position, and isFocusedLine/isPause) and returns either the
IdlingIndicator element or null; inside the helper reuse findNextLineStartTime
to compute nextStart, pauseStart, pauseDuration, progress and delay (delay =
pauseDuration / 3), and create the react.createElement(IdlingIndicator, {
progress, delay }) when appropriate; replace both inline blocks (the current
block around IdlingIndicator and the similar one at the other location) with
calls to this helper so both synced views use the same logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dcfedbaf-df20-4098-81eb-e5b30b3050ea
📒 Files selected for processing (1)
CustomApps/lyrics-plus/Pages.js
There was a problem hiding this comment.
Pull request overview
Adds improved pause/idling visualization and RTL karaoke rendering to the Lyrics Plus custom app, alongside small UI/visibility tweaks to synced/unsynced lyric lines.
Changes:
- Introduces pause-line detection + long-gap insertion logic to drive idling indicators during lyric downtime.
- Adds per-word RTL support for karaoke (reversed gradient direction + hover behavior adjustments).
- Updates styling to support hidden pause lines and “past line” visual treatment.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| CustomApps/lyrics-plus/Pages.js | Adds pause-line processing, idling indicator logic, per-word RTL karaoke classes, and refactors performer rendering/scroll behavior. |
| CustomApps/lyrics-plus/style.css | Adds RTL karaoke gradient rules, hidden-line class, “past line” styling, and minor unsynced line hover/layout tweaks. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CustomApps/lyrics-plus/Pages.js`:
- Around line 214-239: The current useMemo computation for
activeLines/activeElementIndex reads CONFIG.visual["lines-before"] and
["lines-after"] with Number(...) which may produce NaN; update the parsing to
coerce to a finite integer with a safe default and clamp (e.g., parseInt or
Number then Number.isFinite check, fallback to 0 or a small max) before
assigning targetBefore and targetAfter and anywhere else those CONFIG values are
used (references: targetBefore, targetAfter, useMemo and the code around
activeLines/activeElementIndex and the other occurrence near line ~300); ensure
the final values are non-NaN integers so the while loops and padding logic
always behave deterministically.
- Around line 109-113: The code falls back endTime to line.startTime but then
checks endTime > line.startTime, which prevents inserting long-gap markers when
endTime is absent; update the conditional in the block that computes gap (around
the endTime/nextLine logic) so it uses nextLine.startTime > line.startTime (or
otherwise checks the gap relative to the original start rather than requiring a
non-null endTime) instead of endTime > line.startTime, keeping the existing gap
check (gap >= LONG_PAUSE_THRESHOLD) and the isPauseLine(nextLine.text) guard so
long-gap markers are inserted even when line.endTime is missing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fd9be0e9-58ed-4519-bbc0-26ba92998a03
📒 Files selected for processing (1)
CustomApps/lyrics-plus/Pages.js
Add an idling indicator for pause lines and long gaps between lyrics >8 seconds, fix RTL handling in karaoke lyrics, and UI enhancements.
Maybe solved this issues #3363
2026-03-07.22-04-05.mp4
Summary by CodeRabbit
New Features
Style