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 3 commits intospicetify:mainfrom
Conversation
📝 WalkthroughWalkthroughExpanded IdlingIndicator props (adds className, style). Added pause-line utilities and pause-marker rendering, RTL detection, and per-word karaoke rendering changes. Reworked line visibility/active-state logic and performer rendering. Added CSS for past/hidden lines, unsynced layout tweaks, and RTL karaoke gradients. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
1 participant
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