feat: custom lyrics-plus#7
Open
ianz56 wants to merge 56 commits into
Open
Conversation
Merge remote-tracking branch 'origin/main' into feat(lyrics-plus)/ianz56-provider commit 4b0f1a9 Author: ianz56 <ianperdiansah05@gmail.com> Date: Sat Feb 21 20:37:17 2026 +0700 fix commit b7e3bdf Author: ianz56 <ianperdiansah05@gmail.com> Date: Sat Feb 21 20:23:15 2026 +0700 lint commit d49af46 Author: ianz56 <ianperdiansah05@gmail.com> Date: Sat Feb 21 20:08:22 2026 +0700 add ianz56 provider commit 353fce6 Author: ianz56 <ianperdiansah05@gmail.com> Date: Tue Feb 17 23:05:09 2026 +0700 refactor commit 4932fc0 Author: ianz56 <ianperdiansah05@gmail.com> Date: Tue Feb 17 22:58:53 2026 +0700 SyncedExpandedLyricsPage commit 772c465 Author: ianz56 <ianperdiansah05@gmail.com> Date: Tue Feb 17 22:53:31 2026 +0700 Restore non-active karaoke fallback for lines without end time commit a7e2a1c Author: ianz56 <ianperdiansah05@gmail.com> Date: Tue Feb 17 22:26:57 2026 +0700 fix past line and add writers info commit e17b4e9 Author: Ian P <ianperdiansah05@gmail.com> Date: Tue Feb 17 01:01:48 2026 +0700 Update CustomApps/lyrics-plus/ProviderApple.js Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> commit 5377061 Author: Ian P <ianperdiansah05@gmail.com> Date: Tue Feb 17 01:01:20 2026 +0700 Update CustomApps/lyrics-plus/ProviderApple.js Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> commit cc4ceb0 Author: Ian P <ianperdiansah05@gmail.com> Date: Tue Feb 17 01:01:06 2026 +0700 Update CustomApps/lyrics-plus/Pages.js Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> commit f27b7da Author: Ian P <ianperdiansah05@gmail.com> Date: Tue Feb 17 00:54:41 2026 +0700 Update CustomApps/lyrics-plus/index.js Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> commit 2f6f385 Author: ianz56 <ianperdiansah05@gmail.com> Date: Tue Feb 17 00:10:19 2026 +0700 Update index desc for apple commit a0a47f0 Author: ianz56 <ianperdiansah05@gmail.com> Date: Mon Feb 16 23:59:57 2026 +0700 fix rtl commit f669217 Author: ianz56 <ianperdiansah05@gmail.com> Date: Mon Feb 16 23:55:21 2026 +0700 feat(lyrics-plus): add apple music provider commit 8d12cbc Author: ianz56 <ianperdiansah05@gmail.com> Date: Mon Feb 16 22:47:36 2026 +0700 fix group performer tags in Unsynced mode commit e6a1c9f Merge: f7b9a2b c06fc8f Author: Ian P <ianperdiansah05@gmail.com> Date: Mon Feb 16 22:33:48 2026 +0700 Merge branch 'main' into feat/performer-tag commit f7b9a2b Author: ianz56 <ianperdiansah05@gmail.com> Date: Mon Feb 16 22:32:51 2026 +0700 remove unnecessary folder commit b038137 Author: ianz56 <ianperdiansah05@gmail.com> Date: Tue Feb 3 13:21:37 2026 +0700 filter unknown commit 72e19a4 Author: ianz56 <ianperdiansah05@gmail.com> Date: Tue Feb 3 13:04:58 2026 +0700 add guard clause commit e11046d Author: ianz56 <ianperdiansah05@gmail.com> Date: Tue Feb 3 12:41:29 2026 +0700 Add performer tag
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds Apple Music (Paxsenix) and ianz56 providers, provider registration, pause-line synthesis and Word Mode, karaoke/RTL rendering updates, background-translation plumbing, a settings overlay + options-menu trigger API, Musixmatch endpoint/header updates, UI/CSS changes, and assorted wiring across pages, utils, and settings. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Lyrics UI
participant Registry as Providers Registry
participant AppleAPI as Apple (iTunes / Paxsenix)
participant IanzRepo as ianz56 GitHub
participant Parser as Parser / Utils
UI->>Registry: requestLyrics(info, provider)
alt provider == Apple
Registry->>AppleAPI: search track (iTunes Search API) [timeout]
AppleAPI-->>Registry: track metadata / appleID
Registry->>AppleAPI: fetch synced lyrics (Paxsenix) [timeout, token optional]
AppleAPI-->>Registry: raw lyrics JSON
else provider == ianz56
Registry->>IanzRepo: fetch index.json (cached)
IanzRepo-->>Registry: index
Registry->>IanzRepo: fetch lyrics JSON (matched path)
IanzRepo-->>Registry: raw lyrics JSON
end
Registry->>Parser: parse/convert (karaoke, synced, unsynced, background)
Parser-->>Registry: structured lyrics result (incl. ianz56Translation)
Registry-->>UI: return lyrics result
UI->>UI: synthesize pause lines, merge background translations, apply RTL/karaoke rendering, update UI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ 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. Review rate limit: 0/1 reviews remaining, refill in 49 minutes and 48 seconds.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: 3
🧹 Nitpick comments (6)
CustomApps/lyrics-plus/ProviderApple.js (1)
94-98:Content-Typeheader unnecessary on a GET request.Line 97 sets
Content-Type: application/jsonfor the GET request to the lyrics API. This header is typically only meaningful for requests with a body (POST, PUT). It's harmless but unnecessary.♻️ Proposed fix
if (token) { url = `https://api.paxsenix.org/lyrics/applemusic?id=${id}`; headers["Authorization"] = `Bearer ${token}`; - headers["Content-Type"] = "application/json"; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CustomApps/lyrics-plus/ProviderApple.js` around lines 94 - 98, The headers object in ProviderApple.js unnecessarily sets "Content-Type" for a GET request; remove the line that assigns headers["Content-Type"] = "application/json" (or conditionally set Content-Type only for requests with a body) where the code builds the Apple lyrics request (the block that checks token, sets url = `https://api.paxsenix.org/lyrics/applemusic?id=${id}` and headers["Authorization"] = `Bearer ${token}`) so the GET request only includes the Authorization header.CustomApps/lyrics-plus/ProviderIanz56.js (1)
126-126: Remove or gateconsole.logbehind a debug flag.Line 126 logs every lyrics fetch URL to the console in production. Consider removing it or wrapping with a debug check.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CustomApps/lyrics-plus/ProviderIanz56.js` at line 126, The console.log call that prints "[ianz56] Fetching JSON lyrics from:" should not run in production; locate the console.log("[ianz56] Fetching JSON lyrics from:", url) in ProviderIanz56.js and either remove it or wrap it behind a debug flag (e.g., check an existing logger like this.logger.debug or use process.env.DEBUG_IANZ56/DEBUG) so the message only prints when debugging is enabled; ensure no unconditional console.* calls remain in the fetch path.CustomApps/lyrics-plus/ProviderMusixmatch.js (1)
165-215: Consider edge case: lookahead may skip unmatched snippets permanently.When a snippet at
queueCursordoesn't match but a snippet atqueueCursor + idoes,queueCursoradvances past the unmatched intermediate snippets (Line 190:queueCursor += matchFoundAtOffset). This means those skipped snippets are never revisited and their performer data is lost. This is likely intentional for robustness against misaligned data, but worth confirming it doesn't cause silent performer data loss for valid lyrics.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CustomApps/lyrics-plus/ProviderMusixmatch.js` around lines 165 - 215, The loop can permanently skip snippets when a match is found ahead (queueCursor += matchFoundAtOffset), losing performer data; in matchSequential update the logic so skipped intermediate snippets are not discarded: either (A) when matchFoundAtOffset > 0 also collect/merge performers from snippetQueue[queueCursor .. queueCursor+matchFoundAtOffset-1] into matchedPerformers before advancing, or (B) avoid advancing the global queue past those snippets (advance only by 1 so they can be reprocessed by subsequent lines). Apply the change inside matchSequential where queueCursor and matchedPerformers are updated (use variable names queueCursor, matchFoundAtOffset, matchedSnippet, matchedPerformers) so skipped snippets are preserved.CustomApps/lyrics-plus/Pages.js (2)
69-75: Redundant sub-expressions in the early-return guard.The condition on lines 70–73 simplifies to:
(!isActive && position > startTime) || (endTime != null && position > endTime)The inner
(position > startTime || (endTime != null && position > endTime))is always true whenposition > startTime(which is already required by the trailing&& position > startTime), making it a no-op.♻️ Suggested simplification
const KaraokeLine = ({ text, isActive, position, startTime, endTime }) => { - if ( - (!isActive && (position > startTime || (endTime != null && position > endTime)) && position > startTime) || - (endTime != null && position > endTime) - ) { + if ((!isActive && position > startTime) || (endTime != null && position > endTime)) { return text.map(({ word }, i) => (typeof word === "string" ? word : react.cloneElement(word, { key: i }))); }🤖 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 69 - 75, The early-return guard in KaraokeLine contains redundant sub-expressions; replace the complex condition in the if of KaraokeLine with the simplified equivalent "(!isActive && position > startTime) || (endTime != null && position > endTime)" so the logic and return behavior remain the same but the boolean expression is clearer and avoids duplicated checks; update only the if condition in the KaraokeLine component to use that simplified expression.
490-508:isPastandisActivecan both be true whenisFocused && position > endTime.When the focused line's
endTimehas elapsed but no subsequent line has started, both CSS classes (-activeand-past) will apply simultaneously at line 508. If the styles conflict (e.g., different opacities), verify the CSS specificity produces the intended visual. IfisPastshould take precedence in that situation:♻️ Optional: exclude isPast when isActive
- className: `lyrics-lyricsContainer-LyricsLine${isActive ? " lyrics-lyricsContainer-LyricsLine-active" : ""}${isPast ? " lyrics-lyricsContainer-LyricsLine-past" : ""}`, + className: `lyrics-lyricsContainer-LyricsLine${isActive ? " lyrics-lyricsContainer-LyricsLine-active" : ""}${isPast && !isActive ? " lyrics-lyricsContainer-LyricsLine-past" : ""}`,🤖 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 490 - 508, The logic allows both isActive and isPast to be true when the focused line has endTime elapsed (isFocused && position > endTime); update the boolean logic so a line can't be both active and past — for example compute isPast as ((endTime != null && position > endTime) || (!isFocused && startTime != null && position > startTime)) AND explicitly exclude when isActive (or compute isActive first from isFocused/isPlaying and then set isPast = isPast && !isActive) so the className for the div (constructed in the return block) never gets both "LyricsLine-active" and "LyricsLine-past".CustomApps/lyrics-plus/index.js (1)
748-812: Consider extracting duplicatedbgResultcomputation into a helper.The same pattern — guard on
lyric.background, join words, translate — is repeated verbatim across all four language branches (ja, ko, zh-hans, zh-hant). A small helper would reduce ~60 lines to ~10 and make future translator additions less error-prone:♻️ Example helper extraction
+const translateBackground = async (lyrics, translatorFn) => + Promise.all( + lyrics.map(async (lyric) => { + if (!lyric.background || !lyric.background.length) return null; + const bgText = lyric.background.map((w) => w.word).join(""); + return translatorFn(bgText); + }) + );Then each branch becomes, e.g.:
-bgResult = await Promise.all( - lyrics.map(async (lyric) => { - if (!lyric.background || !lyric.background.length) return null; - const bgText = lyric.background.map((w) => w.word).join(""); - return await this.translator.romajifyText(bgText, map[targetConvert].target, map[targetConvert].mode); - }) -); +bgResult = await translateBackground(lyrics, (text) => + this.translator.romajifyText(text, map[targetConvert].target, map[targetConvert].mode) +);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CustomApps/lyrics-plus/index.js` around lines 748 - 812, Duplicate bgResult logic across the ja/ko/zh-hans/zh-hant branches should be extracted into a helper to avoid repetition: add a method on the class (e.g., computeBgResult or translateBackgrounds) that takes (lyrics, translatorFn, ...translatorArgs) or a small wrapper like (lyrics, (text)=>this.translator.*(...)) and implements the guard for lyric.background, joins words, and calls the provided translator for each bgText with Promise.all; then replace each repeated bgResult = await Promise.all(...) block in the ja, ko, zh-hans, and zh-hant branches with a single await this.computeBgResult(lyrics, translatorWrapper) call (for ja/ko pass romajify/convertToRomaja wrappers, for zh pass convertChinese wrapper with appropriate map[targetConvert] args).
🤖 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/OptionsMenu.js`:
- Around line 186-191: The useMemo that builds the sourceOptions/items for
OptionsMenu omits hasTranslation.ianz56 from its dependency array, so the
conditional addition of ianz56Translation (the code that spreads into
sourceOptions when hasTranslation.ianz56 is true) won't trigger recompute;
update the dependency array for the useMemo that returns items/sourceOptions to
include hasTranslation.ianz56 so the memo reruns when that flag changes (refer
to the useMemo that constructs sourceOptions/items and the hasTranslation.ianz56
conditional).
In `@CustomApps/lyrics-plus/ProviderIanz56.js`:
- Around line 28-60: The outer async function fetchIndex contains a dead,
shadowing inner fetchIndex that is never called; remove the nested fetchIndex
function and keep the outer cache-check + fetch logic, or if you intended to use
Spicetify.CosmosAsync like fetchLyricsJson, wire that call into the outer
fetchIndex (call Spicetify.CosmosAsync(...) and fall back to fetch(indexUrl) on
failure) and then set indexCache and lastIndexFetch in the same outer function
so there is no duplicate/shadowed fetchIndex definition.
- Around line 95-112: The current index.find callback allows exact title matches
to succeed even when entryArtist is non-empty, causing false positives across
artists; update the else-branch in the match logic (the callback using
entryArtist, entryTitle, normalizedTitle) to also require the artist to match
(compare entryArtist to the query's normalizedArtist) before accepting an exact
title match so that matching requires both entryTitle === normalizedTitle and
entryArtist === normalizedArtist.
---
Nitpick comments:
In `@CustomApps/lyrics-plus/index.js`:
- Around line 748-812: Duplicate bgResult logic across the ja/ko/zh-hans/zh-hant
branches should be extracted into a helper to avoid repetition: add a method on
the class (e.g., computeBgResult or translateBackgrounds) that takes (lyrics,
translatorFn, ...translatorArgs) or a small wrapper like (lyrics,
(text)=>this.translator.*(...)) and implements the guard for lyric.background,
joins words, and calls the provided translator for each bgText with Promise.all;
then replace each repeated bgResult = await Promise.all(...) block in the ja,
ko, zh-hans, and zh-hant branches with a single await
this.computeBgResult(lyrics, translatorWrapper) call (for ja/ko pass
romajify/convertToRomaja wrappers, for zh pass convertChinese wrapper with
appropriate map[targetConvert] args).
In `@CustomApps/lyrics-plus/Pages.js`:
- Around line 69-75: The early-return guard in KaraokeLine contains redundant
sub-expressions; replace the complex condition in the if of KaraokeLine with the
simplified equivalent "(!isActive && position > startTime) || (endTime != null
&& position > endTime)" so the logic and return behavior remain the same but the
boolean expression is clearer and avoids duplicated checks; update only the if
condition in the KaraokeLine component to use that simplified expression.
- Around line 490-508: The logic allows both isActive and isPast to be true when
the focused line has endTime elapsed (isFocused && position > endTime); update
the boolean logic so a line can't be both active and past — for example compute
isPast as ((endTime != null && position > endTime) || (!isFocused && startTime
!= null && position > startTime)) AND explicitly exclude when isActive (or
compute isActive first from isFocused/isPlaying and then set isPast = isPast &&
!isActive) so the className for the div (constructed in the return block) never
gets both "LyricsLine-active" and "LyricsLine-past".
In `@CustomApps/lyrics-plus/ProviderApple.js`:
- Around line 94-98: The headers object in ProviderApple.js unnecessarily sets
"Content-Type" for a GET request; remove the line that assigns
headers["Content-Type"] = "application/json" (or conditionally set Content-Type
only for requests with a body) where the code builds the Apple lyrics request
(the block that checks token, sets url =
`https://api.paxsenix.org/lyrics/applemusic?id=${id}` and
headers["Authorization"] = `Bearer ${token}`) so the GET request only includes
the Authorization header.
In `@CustomApps/lyrics-plus/ProviderIanz56.js`:
- Line 126: The console.log call that prints "[ianz56] Fetching JSON lyrics
from:" should not run in production; locate the console.log("[ianz56] Fetching
JSON lyrics from:", url) in ProviderIanz56.js and either remove it or wrap it
behind a debug flag (e.g., check an existing logger like this.logger.debug or
use process.env.DEBUG_IANZ56/DEBUG) so the message only prints when debugging is
enabled; ensure no unconditional console.* calls remain in the fetch path.
In `@CustomApps/lyrics-plus/ProviderMusixmatch.js`:
- Around line 165-215: The loop can permanently skip snippets when a match is
found ahead (queueCursor += matchFoundAtOffset), losing performer data; in
matchSequential update the logic so skipped intermediate snippets are not
discarded: either (A) when matchFoundAtOffset > 0 also collect/merge performers
from snippetQueue[queueCursor .. queueCursor+matchFoundAtOffset-1] into
matchedPerformers before advancing, or (B) avoid advancing the global queue past
those snippets (advance only by 1 so they can be reprocessed by subsequent
lines). Apply the change inside matchSequential where queueCursor and
matchedPerformers are updated (use variable names queueCursor,
matchFoundAtOffset, matchedSnippet, matchedPerformers) so skipped snippets are
preserved.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
CustomApps/lyrics-plus/OptionsMenu.jsCustomApps/lyrics-plus/Pages.jsCustomApps/lyrics-plus/ProviderApple.jsCustomApps/lyrics-plus/ProviderIanz56.jsCustomApps/lyrics-plus/ProviderMusixmatch.jsCustomApps/lyrics-plus/Providers.jsCustomApps/lyrics-plus/Utils.jsCustomApps/lyrics-plus/index.jsCustomApps/lyrics-plus/manifest.jsonCustomApps/lyrics-plus/style.css
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1c5776ec8d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
CustomApps/lyrics-plus/ProviderIanz56.js (2)
28-60: Dead nestedfetchIndexfunction — previously flagged and still unaddressed.The inner
fetchIndex(Lines 35–50) duplicates the outer function and is never called.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CustomApps/lyrics-plus/ProviderIanz56.js` around lines 28 - 60, The outer async function fetchIndex contains a dead, duplicated inner fetchIndex that is never called; remove the nested function block (the inner fetchIndex definition and its duplicate body) and keep a single implementation that uses BASE_URL, indexUrl, indexCache, lastIndexFetch and CACHE_DURATION, preserving the cache check, fetch, response.ok error throw, JSON parsing and updating indexCache/lastIndexFetch before returning.
95-112: Third matching strategy may still return false positives — previously flagged and still unaddressed.When
entryArtistis non-empty, Line 108 accepts any entry whose normalized title matches exactly, regardless of artist.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CustomApps/lyrics-plus/ProviderIanz56.js` around lines 95 - 112, The current index.find callback allows exact title matches even when entryArtist is non-empty, causing false positives; update the matching logic to require artist agreement when the input artist is provided by computing the normalized input artist (e.g., normalizedArtist) and in the branch where entryArtist is non-empty require both entryTitle === normalizedTitle AND normalized(entry.artist) === normalizedArtist (or another appropriate artist-equality check); keep the legacy behavior of title-only matching only when the input artist is empty and entryArtist is empty.
🧹 Nitpick comments (2)
CustomApps/lyrics-plus/ProviderIanz56.js (2)
303-303: Remove or implement commented-outbackgroundfield.If background words are intentionally excluded from
ianz56Translation, remove the comment; if they're needed, implement the field.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CustomApps/lyrics-plus/ProviderIanz56.js` at line 303, The commented-out background field inside the ianz56Translation object should either be removed or implemented: locate the ianz56Translation object in ProviderIanz56.js and either delete the commented line "// background: backgroundWords.length ? backgroundWords : undefined," if backgroundWords are intentionally unused, or uncomment and ensure backgroundWords is defined/available and used (keeping the conditional expression or replacing with the correct value) so the background property is properly set on ianz56Translation.
354-358: Preserve original error message for debuggability.
eis swallowed into a generic string, which makes it hard to distinguish network failures from "no match found" from "no jsonPath" cases.♻️ Proposed improvement
- result.error = "Request error or lyrics not found"; + result.error = e?.message || "Request error or lyrics not found";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CustomApps/lyrics-plus/ProviderIanz56.js` around lines 354 - 358, The catch block currently swallows the original error by setting result.error to a generic string; keep the console.error("[ianz56] Error:", e) but also preserve the original error details in result.error (e.g., set result.error = `${e?.message ?? String(e)}` or JSON.stringify(e) alongside the generic text) so callers/getters can distinguish network, parsing, or no-match errors; update the catch that references the result variable and the catch parameter e in ProviderIanz56.js accordingly.
🤖 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/ProviderIanz56.js`:
- Around line 128-136: The current code uses Spicetify.CosmosAsync.get(url) as
the primary fetch path which will attach Spotify session credentials to an
external raw.githubusercontent.com request; change the logic in the block that
references url and response so you call fetch(url) first (await fetch, check
response.ok and return response.text()/json() as appropriate) and remove using
Spicetify.CosmosAsync.get for this external GitHub URL (or only use CosmosAsync
as a non-default fallback for internal Spotify URLs); also adjust the catch/warn
handling around the fetch call and remove or narrow the CosmosAsync.warn path to
avoid sending user cookies to external hosts.
- Around line 307-312: The unsynced array is left unsorted while karaoke, synced
and ianz56Translation are sorted by startTime; ensure unsynced is ordered too by
either (A) adding a startTime to unsynced entries and applying the same sort:
unsynced.sort((a,b)=>a.startTime-b.startTime), or (B) build/populate unsynced
after you sort karaoke/synced/ianz56Translation so it inherits the correct
sequence; update the code that pushes into unsynced (from the lines iteration)
to include a startTime if you choose option A, or move that push logic to use
the already-sorted arrays if you choose option B.
- Line 126: Remove the stray debug console.log call that prints "[ianz56]
Fetching JSON lyrics from:" and the url in ProviderIanz56.js (the temporary
debug inside the lyrics-fetching routine); either delete the console.log line or
replace it with the project's proper logging utility (e.g., debug/logger) used
elsewhere so no raw console output remains.
- Around line 252-253: The object construction is using falsy checks that turn
valid zero times into undefined; change the assignment for
backgroundStartTime/backgroundEndTime to preserve 0 by using a nullish check
instead (e.g., use the nullish coalescing logic or an explicit numeric check) so
backgroundStartTime and backgroundEndTime (computed from bgWords[0].begin * 1000
and bgWords[last].end * 1000) are only turned to undefined when truly
null/undefined rather than when equal to 0.
---
Duplicate comments:
In `@CustomApps/lyrics-plus/ProviderIanz56.js`:
- Around line 28-60: The outer async function fetchIndex contains a dead,
duplicated inner fetchIndex that is never called; remove the nested function
block (the inner fetchIndex definition and its duplicate body) and keep a single
implementation that uses BASE_URL, indexUrl, indexCache, lastIndexFetch and
CACHE_DURATION, preserving the cache check, fetch, response.ok error throw, JSON
parsing and updating indexCache/lastIndexFetch before returning.
- Around line 95-112: The current index.find callback allows exact title matches
even when entryArtist is non-empty, causing false positives; update the matching
logic to require artist agreement when the input artist is provided by computing
the normalized input artist (e.g., normalizedArtist) and in the branch where
entryArtist is non-empty require both entryTitle === normalizedTitle AND
normalized(entry.artist) === normalizedArtist (or another appropriate
artist-equality check); keep the legacy behavior of title-only matching only
when the input artist is empty and entryArtist is empty.
---
Nitpick comments:
In `@CustomApps/lyrics-plus/ProviderIanz56.js`:
- Line 303: The commented-out background field inside the ianz56Translation
object should either be removed or implemented: locate the ianz56Translation
object in ProviderIanz56.js and either delete the commented line "// background:
backgroundWords.length ? backgroundWords : undefined," if backgroundWords are
intentionally unused, or uncomment and ensure backgroundWords is
defined/available and used (keeping the conditional expression or replacing with
the correct value) so the background property is properly set on
ianz56Translation.
- Around line 354-358: The catch block currently swallows the original error by
setting result.error to a generic string; keep the console.error("[ianz56]
Error:", e) but also preserve the original error details in result.error (e.g.,
set result.error = `${e?.message ?? String(e)}` or JSON.stringify(e) alongside
the generic text) so callers/getters can distinguish network, parsing, or
no-match errors; update the catch that references the result variable and the
catch parameter e in ProviderIanz56.js accordingly.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
♻️ Duplicate comments (3)
CustomApps/lyrics-plus/ProviderIanz56.js (3)
287-292: Keepunsyncedordering deterministic, like other lyric arrays.
karaoke,synced, andianz56Translationare sorted, butunsyncedis not before return on Line 292.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CustomApps/lyrics-plus/ProviderIanz56.js` around lines 287 - 292, The unsynced lyric array is not being sorted before return while karaoke, synced, and ianz56Translation are; add a deterministic sort for unsynced (e.g., unsynced.sort((a, b) => a.startTime - b.startTime) or equivalent) in the same block so the return value uses consistent ordering for karaoke, synced, unsynced, and ianz56Translation in the function that prepares these arrays.
232-233: Preserve valid0ms background timing values.Using
|| undefinedon Lines 232-233 drops legitimate0values.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CustomApps/lyrics-plus/ProviderIanz56.js` around lines 232 - 233, The code is incorrectly using the logical OR fallback which converts valid 0 values for backgroundStartTime and backgroundEndTime into undefined; update the object construction that sets backgroundStartTime and backgroundEndTime (the properties named backgroundStartTime and backgroundEndTime in ProviderIanz56) to use a nullish-coalescing fallback (or an explicit check) instead of "|| undefined" so that 0 is preserved (e.g., use "backgroundStartTime ?? undefined" and "backgroundEndTime ?? undefined").
109-109: Remove raw debugconsole.logstatements from provider flow.
console.logon Line 109 and Line 317 should not ship in production provider paths.Also applies to: 317-317
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CustomApps/lyrics-plus/ProviderIanz56.js` at line 109, Remove raw console.log calls in ProviderIanz56.js (specifically the statements that print "[ianz56] Fetching JSON lyrics from:" and the other console.log at the provider flow). Replace them with the project logging utility or a debug-level logger (e.g., providerLogger.debug or similar) or simply remove them so no raw console output ships in production; locate the calls by searching for the literal string "[ianz56] Fetching JSON lyrics from:" and the second console.log in the provider flow and update those sites (in the provider function where JSON lyrics are fetched) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@CustomApps/lyrics-plus/ProviderIanz56.js`:
- Around line 287-292: The unsynced lyric array is not being sorted before
return while karaoke, synced, and ianz56Translation are; add a deterministic
sort for unsynced (e.g., unsynced.sort((a, b) => a.startTime - b.startTime) or
equivalent) in the same block so the return value uses consistent ordering for
karaoke, synced, unsynced, and ianz56Translation in the function that prepares
these arrays.
- Around line 232-233: The code is incorrectly using the logical OR fallback
which converts valid 0 values for backgroundStartTime and backgroundEndTime into
undefined; update the object construction that sets backgroundStartTime and
backgroundEndTime (the properties named backgroundStartTime and
backgroundEndTime in ProviderIanz56) to use a nullish-coalescing fallback (or an
explicit check) instead of "|| undefined" so that 0 is preserved (e.g., use
"backgroundStartTime ?? undefined" and "backgroundEndTime ?? undefined").
- Line 109: Remove raw console.log calls in ProviderIanz56.js (specifically the
statements that print "[ianz56] Fetching JSON lyrics from:" and the other
console.log at the provider flow). Replace them with the project logging utility
or a debug-level logger (e.g., providerLogger.debug or similar) or simply remove
them so no raw console output ships in production; locate the calls by searching
for the literal string "[ianz56] Fetching JSON lyrics from:" and the second
console.log in the provider flow and update those sites (in the provider
function where JSON lyrics are fetched) accordingly.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CustomApps/lyrics-plus/OptionsMenu.jsCustomApps/lyrics-plus/ProviderIanz56.js
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/ProviderIanz56.js`:
- Line 252: The unsynced array entries are missing their startTime property
causing the subsequent sort in sort((a,b)=>a.startTime-b.startTime) to be a
no-op; update the push that currently does unsynced.push({ text: valText }) to
include the corresponding start time (e.g., unsynced.push({ text: valText,
startTime: <computedStart> })) using the same source for start times used
elsewhere in this function (look for variables like start, currentStart,
timestamp, or the value used when creating synced entries) so the sort on
startTime works correctly.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
CustomApps/lyrics-plus/ProviderIanz56.js (1)
320-323: Generic error message loses debugging context.The catch block discards the original error and returns a generic message. Consider logging the actual error for debugging while still returning a user-friendly message.
🔧 Proposed improvement
} catch (e) { + console.warn("[ianz56] findLyrics error:", e); result.error = "Request error or lyrics not found"; return result; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CustomApps/lyrics-plus/ProviderIanz56.js` around lines 320 - 323, The catch block currently swallows the thrown error and returns a generic message; update the catch in the function that builds and returns result (the catch (e) { ... } block that sets result.error) to log the original error (e.g., console.error or the module logger) including e.message or e.stack for debugging, while still setting result.error to the user-friendly "Request error or lyrics not found" before returning; ensure you reference the same result variable and preserve the existing user-facing message.CustomApps/lyrics-plus/ProviderApple.js (2)
239-245:unsyncedentries lackstartTimeunlikeProviderIanz56.In
ProviderIanz56.js(lines 255, 259),unsyncedentries includestartTimefor sorting. Here,unsynced.push({ text: ... })omits it. While unsynced lyrics don't require timing for display, this inconsistency between providers could cause issues if downstream code expects a consistent shape.🔧 Proposed fix for consistency
if (isInline) { - if (combinedText) unsynced.push({ text: combinedText }); + if (combinedText) unsynced.push({ startTime: lineStartTime, text: combinedText }); } else { let valText = mainTextStr; if (bgTextStr) valText += ` (${bgTextStr})`; - if (valText) unsynced.push({ text: valText }); + if (valText) unsynced.push({ startTime: lineStartTime, text: valText }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CustomApps/lyrics-plus/ProviderApple.js` around lines 239 - 245, The unsynced entries pushed in the block using isInline/combinedText/mainTextStr/bgTextStr omit a startTime property, causing inconsistency with ProviderIanz56 which adds startTime for downstream sorting; update both push calls to include a startTime (e.g., startTime: 0 or another canonical sentinel) so each object pushed to unsynced has the same shape (refer to unsynced, isInline, combinedText, mainTextStr, bgTextStr and ProviderIanz56 for the expected structure).
220-220: Same implicit CONFIG dependency as ProviderIanz56.This provider also references the global
CONFIGobject without explicit import. This creates coupling but is consistent with the codebase pattern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CustomApps/lyrics-plus/ProviderApple.js` at line 220, The code in ProviderApple.js reads the global CONFIG (e.g., const isInline = CONFIG?.visual?.["synced-background-inline"]) causing an implicit dependency; update the provider to use an explicit CONFIG source—either add CONFIG to the module's imports/arguments or accept it via the provider's constructor/initialization function (matching the pattern used in ProviderIanz56), then replace direct global access in ProviderApple (reference to isInline/CONFIG) with the new explicit parameter so the provider no longer relies on a global.
🤖 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/ProviderApple.js`:
- Around line 239-245: The unsynced entries pushed in the block using
isInline/combinedText/mainTextStr/bgTextStr omit a startTime property, causing
inconsistency with ProviderIanz56 which adds startTime for downstream sorting;
update both push calls to include a startTime (e.g., startTime: 0 or another
canonical sentinel) so each object pushed to unsynced has the same shape (refer
to unsynced, isInline, combinedText, mainTextStr, bgTextStr and ProviderIanz56
for the expected structure).
- Line 220: The code in ProviderApple.js reads the global CONFIG (e.g., const
isInline = CONFIG?.visual?.["synced-background-inline"]) causing an implicit
dependency; update the provider to use an explicit CONFIG source—either add
CONFIG to the module's imports/arguments or accept it via the provider's
constructor/initialization function (matching the pattern used in
ProviderIanz56), then replace direct global access in ProviderApple (reference
to isInline/CONFIG) with the new explicit parameter so the provider no longer
relies on a global.
In `@CustomApps/lyrics-plus/ProviderIanz56.js`:
- Around line 320-323: The catch block currently swallows the thrown error and
returns a generic message; update the catch in the function that builds and
returns result (the catch (e) { ... } block that sets result.error) to log the
original error (e.g., console.error or the module logger) including e.message or
e.stack for debugging, while still setting result.error to the user-friendly
"Request error or lyrics not found" before returning; ensure you reference the
same result variable and preserve the existing user-facing message.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
CustomApps/lyrics-plus/ProviderApple.jsCustomApps/lyrics-plus/ProviderIanz56.jsCustomApps/lyrics-plus/Settings.jsCustomApps/lyrics-plus/index.jsCustomApps/lyrics-plus/style.css
There was a problem hiding this comment.
🧹 Nitpick comments (1)
CustomApps/lyrics-plus/style.css (1)
749-761: Scope.splitselectors to the unsynced lyrics page to avoid CSS bleed.Lines [749], [754], [759], [768], [773], [780], and [785] are global
.split ...selectors. If.splitis reused elsewhere, these styles can leak unexpectedly.Scoped selector diff
-.split .lyrics-lyricsContainer-LyricsLine { +.lyrics-lyricsContainer-UnsyncedLyricsPage .split .lyrics-lyricsContainer-LyricsLine { padding-left: 50px; padding-right: 50px; } -.split .lyrics-versionSelector { +.lyrics-lyricsContainer-UnsyncedLyricsPage .split .lyrics-versionSelector { margin-right: 50px; margin-left: 50px; } -.split .lyrics-versionSelector select { +.lyrics-lyricsContainer-UnsyncedLyricsPage .split .lyrics-versionSelector select { width: 100%; } `@media` (min-width: 1024px) { - .split .lyrics-lyricsContainer-LyricsLine { + .lyrics-lyricsContainer-UnsyncedLyricsPage .split .lyrics-lyricsContainer-LyricsLine { padding-left: 75px; padding-right: 75px; } - .split .lyrics-versionSelector { + .lyrics-lyricsContainer-UnsyncedLyricsPage .split .lyrics-versionSelector { margin-right: 75px; margin-left: 75px; } } `@media` (min-width: 1280px) { - .split .lyrics-lyricsContainer-LyricsLine { + .lyrics-lyricsContainer-UnsyncedLyricsPage .split .lyrics-lyricsContainer-LyricsLine { padding-left: 100px; padding-right: 100px; } - .split .lyrics-versionSelector { + .lyrics-lyricsContainer-UnsyncedLyricsPage .split .lyrics-versionSelector { margin-right: 100px; margin-left: 100px; } }Also applies to: 768-788
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CustomApps/lyrics-plus/style.css` around lines 749 - 761, The `.split` rules are too global and may leak; scope them to the unsynced lyrics page container by prefixing each selector with the page-specific wrapper (e.g. `.unsynced-lyrics-page .split .lyrics-lyricsContainer-LyricsLine`, `.unsynced-lyrics-page .split .lyrics-versionSelector`, `.unsynced-lyrics-page .split .lyrics-versionSelector select`) so only the unsynced lyrics page gets these styles; update every `.split ...` occurrence in this block (including the three shown: `.lyrics-lyricsContainer-LyricsLine`, `.lyrics-versionSelector`, and `.lyrics-versionSelector select`) to use the scoped prefix.
🤖 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/style.css`:
- Around line 749-761: The `.split` rules are too global and may leak; scope
them to the unsynced lyrics page container by prefixing each selector with the
page-specific wrapper (e.g. `.unsynced-lyrics-page .split
.lyrics-lyricsContainer-LyricsLine`, `.unsynced-lyrics-page .split
.lyrics-versionSelector`, `.unsynced-lyrics-page .split .lyrics-versionSelector
select`) so only the unsynced lyrics page gets these styles; update every
`.split ...` occurrence in this block (including the three shown:
`.lyrics-lyricsContainer-LyricsLine`, `.lyrics-versionSelector`, and
`.lyrics-versionSelector select`) to use the scoped prefix.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
CustomApps/lyrics-plus/ProviderApple.js (1)
99-103:Content-Typeheader is unnecessary for GET requests.The
Content-Typeheader typically applies to request bodies. For GET requests without a body, this header is ignored by most servers but could be removed for clarity.♻️ Suggested fix
if (token) { url = `https://api.paxsenix.org/lyrics/applemusic?id=${id}`; headers["Authorization"] = `Bearer ${token}`; - headers["Content-Type"] = "application/json"; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CustomApps/lyrics-plus/ProviderApple.js` around lines 99 - 103, The code in ProviderApple.js sets headers["Content-Type"] for a GET request inside the token branch; remove that header assignment so GET requests don't include an unnecessary Content-Type. Update the token handling block (the code around url = `https://api.paxsenix.org/lyrics/applemusic?id=${id}` and headers["Authorization"] = `Bearer ${token}`) to omit or conditionally set Content-Type only for requests with a body, leaving only the Authorization header for this GET call.
🤖 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/ProviderApple.js`:
- Around line 99-103: The code in ProviderApple.js sets headers["Content-Type"]
for a GET request inside the token branch; remove that header assignment so GET
requests don't include an unnecessary Content-Type. Update the token handling
block (the code around url =
`https://api.paxsenix.org/lyrics/applemusic?id=${id}` and
headers["Authorization"] = `Bearer ${token}`) to omit or conditionally set
Content-Type only for requests with a body, leaving only the Authorization
header for this GET call.
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 2 file(s) based on 7 unresolved review comments. A stacked PR containing fixes has been created.
Time taken: |
* fix: apply CodeRabbit auto-fixes Fixed 2 file(s) based on 7 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai> * lint --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: CodeRabbit <noreply@coderabbit.ai> Co-authored-by: ianz56 <ianperdiansah05@gmail.com>
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/index.js`:
- Around line 946-948: The onQueueChange handler currently returns early when
queue.current is falsy, leaving previous lyrics displayed; instead, when
queue.current is missing you should clear/reset the lyrics view/state before
returning. Modify the onQueueChange function to detect !queue.current, call the
component's lyrics-clear routine (for example invoke an existing
clearLyrics/clearLyricsView method or set the lyrics state to null via
this.setState/updateLyricsState), then return; ensure you change the branch
inside onQueueChange that checks queue.current so stale lyrics are removed
whenever queue.current disappears.
In `@CustomApps/lyrics-plus/Utils.js`:
- Around line 143-146: The replacement of line.background currently throws away
timing by setting a single token with time:0; instead preserve original timing
envelopes: when bgResult?.[index] exists, compute convertedBgText = typeof
bgResult[index] === "string" ? this.rubyTextToReact(bgResult[index]) :
bgResult[index] and then if the original line.background is an array of timed
tokens, map over it and produce tokens that keep each token's time and
isBackground properties but replace the word/content with convertedBgText (or,
if bgResult[index] itself is an array of timed tokens, prefer using its time
values and merge/align them with the original tokens), finally assign the mapped
array back to line.background; update code in Utils.js around the bgResult
handling (references: bgResult, rubyTextToReact, line.background) to implement
this mapping so timing is retained.
🪄 Autofix (Beta)
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: CHILL
Plan: Pro
Run ID: cac38ecb-7fe5-460a-b540-652012f22a22
📒 Files selected for processing (2)
CustomApps/lyrics-plus/Utils.jsCustomApps/lyrics-plus/index.js
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
CustomApps/lyrics-plus/Pages.js (2)
142-147: Remove unusedaccumulatedTimevariable.
accumulatedTimeis declared on line 142 but never read. The code directly mutates thestartTimeparameter instead. This appears to be leftover from a refactor.🧹 Proposed fix
- let accumulatedTime = startTime; return text.map(({ word, time }, i) => { const isRTL = isRTLText(typeof word === "string" ? word : ""); const isWordActive = position >= startTime; startTime += time;🤖 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 142 - 147, Remove the unused accumulatedTime variable declared in the lyrics mapping block: delete the line "let accumulatedTime = startTime;" and keep the existing logic that mutates startTime inside text.map (functions/isRTLText, isWordActive, isWordComplete remain unchanged); if the intention was to avoid mutating startTime, instead replace uses of startTime mutation with an accumulatedTime variable initialized to startTime and update that variable inside the map — but if not, simply remove accumulatedTime.
138-140: Add defensive check forreact.cloneElementon non-element values.If
wordis neither a string nor a valid React element (e.g., a plain object ornull),react.cloneElementwill throw. While current upstream code should always provide valid types, adding a guard prevents runtime errors from unexpected data shapes.🛡️ Proposed defensive fix
if ((endTime != null && position > endTime + KARA_DELAY) || (!isActive && position > startTime)) { - return text.map(({ word }, i) => (typeof word === "string" ? word : react.cloneElement(word, { key: i }))); + return text.map(({ word }, i) => { + if (typeof word === "string") return word; + if (react.isValidElement(word)) return react.cloneElement(word, { key: i }); + return word; // fallback for unexpected types + }); }🤖 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 138 - 140, The current early-return maps text items and blindly calls react.cloneElement on non-string values, which can throw if word is null or a plain object; update the map to only call react.cloneElement when react.isValidElement(word) is true and otherwise safely handle other types (e.g., return String(word) for primitives or null/empty string for null/undefined) so the mapping in the block that uses endTime, position, KARA_DELAY, isActive, startTime, text and word is defensive against unexpected shapes.
🤖 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`:
- Line 607: The line creating lyricsId directly from lyrics[0].text can throw if
lyrics is null/undefined or empty; update the assignment for lyricsId in
Pages.js to defensively access the array (mirror the approach used in
SyncedLyricsPage) by checking lyrics exists and has items before reading .text
(e.g., use a conditional/optional chaining and a fallback), so lyricsId is only
derived when lyrics[0] is present and otherwise set to a safe default.
- Line 187: The code accesses lyrics[0].text which will throw when lyrics is
empty; update the Pages.js logic that computes lyricsId to guard against empty
arrays (the variable lyricsId and the input prop/variable lyrics) — e.g., check
lyrics && lyrics.length > 0 before using lyrics[0].text and provide a safe
fallback (empty string, null, or early return/render placeholder) so consumers
of lyricsId never get a runtime exception.
---
Nitpick comments:
In `@CustomApps/lyrics-plus/Pages.js`:
- Around line 142-147: Remove the unused accumulatedTime variable declared in
the lyrics mapping block: delete the line "let accumulatedTime = startTime;" and
keep the existing logic that mutates startTime inside text.map
(functions/isRTLText, isWordActive, isWordComplete remain unchanged); if the
intention was to avoid mutating startTime, instead replace uses of startTime
mutation with an accumulatedTime variable initialized to startTime and update
that variable inside the map — but if not, simply remove accumulatedTime.
- Around line 138-140: The current early-return maps text items and blindly
calls react.cloneElement on non-string values, which can throw if word is null
or a plain object; update the map to only call react.cloneElement when
react.isValidElement(word) is true and otherwise safely handle other types
(e.g., return String(word) for primitives or null/empty string for
null/undefined) so the mapping in the block that uses endTime, position,
KARA_DELAY, isActive, startTime, text and word is defensive against unexpected
shapes.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d4767bd5-218c-4044-92f2-c4699378a1db
📒 Files selected for processing (4)
CustomApps/lyrics-plus/Pages.jsCustomApps/lyrics-plus/Settings.jsCustomApps/lyrics-plus/Utils.jsCustomApps/lyrics-plus/style.css
✅ Files skipped from review due to trivial changes (1)
- CustomApps/lyrics-plus/Settings.js
🚧 Files skipped from review as they are similar to previous changes (2)
- CustomApps/lyrics-plus/Utils.js
- CustomApps/lyrics-plus/style.css
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 1 file(s) based on 2 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 1 file(s) based on 2 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
* cek * fix lag * biome * apple line mode fix * dynamic before-after * fix overlap * add settings to tabbar and icon * Update CustomApps/lyrics-plus/Pages.js Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
CustomApps/lyrics-plus/index.js (1)
946-948:⚠️ Potential issue | 🟡 MinorClear the view when
queue.currentdisappears.This guard still leaves the previous track rendered when playback stops or the queue resets. Reset the lyric state before returning so the panel does not show stale content.
Proposed fix
this.onQueueChange = async ({ data: queue }) => { - if (!queue.current) return; + if (!queue.current) { + this.currentTrackUri = ""; + this.nextTrackUri = ""; + this.setState({ ...emptyState, uri: "", provider: "", isLoading: false, isCached: false }); + return; + } this.state.explicitMode = this.state.lockMode;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CustomApps/lyrics-plus/index.js` around lines 946 - 948, In the onQueueChange handler, when queue.current is falsy you must clear the component's lyric display state before returning so stale lyrics aren't shown; inside this.onQueueChange (the guard that checks if (!queue.current) return) call the lyric-clearing logic used elsewhere (e.g., reset any properties like currentLyric, lyricLines, activeTrackId or invoke the existing clearLyric/clearState method) and then return, ensuring the panel is emptied whenever the queue resets or playback stops.CustomApps/lyrics-plus/ProviderApple.js (1)
182-195:⚠️ Potential issue | 🟠 MajorUse background vocals when deriving Apple line timing.
lineStartTime/lineEndTimeare still pinned to the main vocal timestamps. WhenbackgroundTextstarts earlier or ends later, the separated background lane renders outside the line’s active window and drifts.CustomApps/lyrics-plus/ProviderIanz56.js:200-280already widens the window the wayPages.jsexpects.Proposed fix
lyricsJson.content.forEach((line) => { - const lineStartTime = line.timestamp; - const lineEndTime = line.endtime; + const bgWords = Array.isArray(line.backgroundText) ? line.backgroundText : []; + const firstBgStart = bgWords[0]?.timestamp; + const lastBgEnd = bgWords[bgWords.length - 1]?.endtime; + const lineStartTime = firstBgStart != null ? Math.min(line.timestamp ?? firstBgStart, firstBgStart) : line.timestamp; + const lineEndTime = lastBgEnd != null ? Math.max(line.endtime ?? line.timestamp ?? lastBgEnd, lastBgEnd) : line.endtime; if (lineStartTime == null) return; // Process Main Text🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CustomApps/lyrics-plus/ProviderApple.js` around lines 182 - 195, The current loop in lyricsJson.content.forEach uses only the main vocal timestamps (lineStartTime/lineEndTime) so backgroundText lanes can extend outside the line window; update the timing calculation before calling processWords: if line.backgroundText exists, compute the earliest start and latest end between the main line timestamps and the backgroundText items' timestamps/endtimes, then assign those expanded values to the variables used for processing. Locate the timing logic in the lyricsJson.content.forEach block (variables lineStartTime, lineEndTime) and adjust them using the backgroundText timestamps so processWords(...) is invoked with the widened window for both main and background words.
🤖 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 311-333: The effects that call updateSpacerRef.current() and
computeOffsetRef.current() only depend on activeLineIndex and lyricsId but must
also rerun when visual settings or the spacer height change; update the
dependency array of the react.useLayoutEffect (the one currently using
[activeLineIndex, lyricsId]) to include startLineIndex and the visual settings
keys CONFIG.visual["lines-before"] and CONFIG.visual["lines-after"] so the
spacer height and offset are recomputed on settings changes (this will
indirectly pick up changes to --lyrics-line-height when startLineIndex or the
visual window changes); reference updateSpacerRef, computeOffsetRef,
startLineIndex, CONFIG.visual["lines-before"], CONFIG.visual["lines-after"],
spacerRef, and lyricContainerEle when making the change.
---
Duplicate comments:
In `@CustomApps/lyrics-plus/index.js`:
- Around line 946-948: In the onQueueChange handler, when queue.current is falsy
you must clear the component's lyric display state before returning so stale
lyrics aren't shown; inside this.onQueueChange (the guard that checks if
(!queue.current) return) call the lyric-clearing logic used elsewhere (e.g.,
reset any properties like currentLyric, lyricLines, activeTrackId or invoke the
existing clearLyric/clearState method) and then return, ensuring the panel is
emptied whenever the queue resets or playback stops.
In `@CustomApps/lyrics-plus/ProviderApple.js`:
- Around line 182-195: The current loop in lyricsJson.content.forEach uses only
the main vocal timestamps (lineStartTime/lineEndTime) so backgroundText lanes
can extend outside the line window; update the timing calculation before calling
processWords: if line.backgroundText exists, compute the earliest start and
latest end between the main line timestamps and the backgroundText items'
timestamps/endtimes, then assign those expanded values to the variables used for
processing. Locate the timing logic in the lyricsJson.content.forEach block
(variables lineStartTime, lineEndTime) and adjust them using the backgroundText
timestamps so processWords(...) is invoked with the widened window for both main
and background words.
🪄 Autofix (Beta)
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: CHILL
Plan: Pro
Run ID: 1324b152-90bc-4d0e-a0d8-3bc745be21f8
📒 Files selected for processing (6)
CustomApps/lyrics-plus/OptionsMenu.jsCustomApps/lyrics-plus/Pages.jsCustomApps/lyrics-plus/ProviderApple.jsCustomApps/lyrics-plus/TabBar.jsCustomApps/lyrics-plus/index.jsCustomApps/lyrics-plus/style.css
✅ Files skipped from review due to trivial changes (1)
- CustomApps/lyrics-plus/TabBar.js
🚧 Files skipped from review as they are similar to previous changes (2)
- CustomApps/lyrics-plus/OptionsMenu.js
- CustomApps/lyrics-plus/style.css
There was a problem hiding this comment.
🧹 Nitpick comments (2)
CustomApps/lyrics-plus/Pages.js (2)
146-146: Remove unused variableaccumulatedTime.This variable is assigned but never referenced in the function body. The code directly mutates
startTimeinstead.🧹 Proposed fix
- let accumulatedTime = startTime; return text.map(({ word, time }, i) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CustomApps/lyrics-plus/Pages.js` at line 146, Remove the unused local variable accumulatedTime that is assigned from startTime but never read; locate the declaration "let accumulatedTime = startTime;" in the function (near uses of startTime) and delete that line so the code only uses and updates startTime directly (no other changes required).
767-781: Consider adding visual settings to effect dependencies for immediate re-centering.The effect reads
CONFIG.visual["lines-before"]andCONFIG.visual["lines-after"](lines 771-772) but only depends on[activeLineIndex, lyricsId]. When users change these settings, the focal point calculation changes but the scroll won't update until the next line transition.This is a minor UX consideration since the scroll will eventually correct itself.
♻️ Optional: Add visual settings to dependencies
+ const linesBefore = CONFIG.visual["lines-before"]; + const linesAfter = CONFIG.visual["lines-after"]; + const fontSize = CONFIG.visual["font-size"]; + react.useLayoutEffect(() => { if (activeLineRef.current && pageRef.current) { - const fontSize = Number(CONFIG.visual["font-size"]) || 32; + const fontSizeNum = Number(fontSize) || 32; - const lineHeight = fontSize + 12; // Matching UnsyncedLyricsPage line-height + const lineHeight = fontSizeNum + 12; - const linesBefore = CONFIG.visual["lines-before"]; - const linesAfter = CONFIG.visual["lines-after"]; const focalPoint = pageRef.current.clientHeight / 2 + (linesBefore - linesAfter) * (lineHeight / 2); pageRef.current.scrollTo({ top: activeLineRef.current.offsetTop - focalPoint + activeLineRef.current.clientHeight / 2, behavior: initialScroll.current ? "smooth" : "auto", }); initialScroll.current = true; } - }, [activeLineIndex, lyricsId]); + }, [activeLineIndex, lyricsId, linesBefore, linesAfter, fontSize]);🤖 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 767 - 781, The useLayoutEffect that recenters the page (useLayoutEffect block using activeLineRef, pageRef, initialScroll) reads CONFIG.visual["lines-before"], CONFIG.visual["lines-after"] and font-size but only lists [activeLineIndex, lyricsId] as dependencies, so changing those visual settings won't immediately re-run the effect; update the dependency array to include the derived visual values (e.g., CONFIG.visual["lines-before"], CONFIG.visual["lines-after"], and the computed fontSize or CONFIG.visual["font-size"]) or compute and memoize those values (via constants or useMemo) and include them in the dependency list so the focalPoint recalculation and scrollTo are triggered when visual settings 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`:
- Line 146: Remove the unused local variable accumulatedTime that is assigned
from startTime but never read; locate the declaration "let accumulatedTime =
startTime;" in the function (near uses of startTime) and delete that line so the
code only uses and updates startTime directly (no other changes required).
- Around line 767-781: The useLayoutEffect that recenters the page
(useLayoutEffect block using activeLineRef, pageRef, initialScroll) reads
CONFIG.visual["lines-before"], CONFIG.visual["lines-after"] and font-size but
only lists [activeLineIndex, lyricsId] as dependencies, so changing those visual
settings won't immediately re-run the effect; update the dependency array to
include the derived visual values (e.g., CONFIG.visual["lines-before"],
CONFIG.visual["lines-after"], and the computed fontSize or
CONFIG.visual["font-size"]) or compute and memoize those values (via constants
or useMemo) and include them in the dependency list so the focalPoint
recalculation and scrollTo are triggered when visual settings change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8b65d35f-bac7-4035-95e7-1c943b812bfd
📒 Files selected for processing (2)
CustomApps/lyrics-plus/Pages.jsCustomApps/lyrics-plus/style.css
🚧 Files skipped from review as they are similar to previous changes (1)
- CustomApps/lyrics-plus/style.css
Owner
Author
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
Status, support, documentation and community
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
CustomApps/lyrics-plus/Pages.js (1)
274-345: Spacer/offset bookkeeping LGTM after the settings-dep fix.The window/spacer/offset pipeline is consistent:
updateSpacerRefruns beforecomputeOffsetRefsooffsetTopis read after the spacer height update, and the layout effect deps now includestartLineIndex,lines-before,lines-after, so settings changes no longer leave stale spacer/offset.One small note:
for (let k = 0; k < startLineIndex; k++)(Lines 322–326) recomputes the non-pause prefix count every render. It's O(n) per update and negligible here, but if lyric lengths grow this could be cached alongside the cluster memo (carry a precomputed prefix sum inlyricWithEmptyLines).🤖 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 274 - 345, The loop that counts non-pause lines up to startLineIndex in updateSpacerRef (for (let k = 0; k < startLineIndex; k++) ... using lyricWithEmptyLines) is O(n) every update; compute and cache a prefix-sum array of non-pause counts alongside the memo that builds activeLines (the useMemo that returns activeLines/startLineIndex/activeElementIndex) — e.g., include nonPausePrefixCounts in that memo keyed to lyricWithEmptyLines and reuse it in updateSpacerRef by reading nonPausePrefixCounts[startLineIndex] instead of looping; update all references to expect the memo to return the extra prefix data and keep the updateSpacerRef logic otherwise the same.
🤖 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/ProviderIanz56.js`:
- Line 347: The current isInline computation in ProviderIanz56.js uses (typeof
CONFIG !== "undefined" && CONFIG?.visual?.["synced-background-inline"]) ?? false
which yields false when CONFIG is undefined and therefore contradicts the
app-wide default set to true in lyrics-plus/index.js; update the expression so
that when CONFIG is undefined or the visual key is missing it falls back to true
(the same default used in index.js) — locate the isInline assignment and change
it to explicitly return CONFIG.visual["synced-background-inline"] when present,
otherwise true.
- Around line 283-373: Add the missing TypeScript triple-slash reference by
inserting /// <reference path="../globals.d.ts" /> as the first line of
ProviderIanz56.js, and remove the dead isMainBackground variable and the
isBackground property from the karaoke objects (references: isMainBackground and
the isBackground field inside the karaoke.push call) so the karaoke entries no
longer include unused metadata.
---
Nitpick comments:
In `@CustomApps/lyrics-plus/Pages.js`:
- Around line 274-345: The loop that counts non-pause lines up to startLineIndex
in updateSpacerRef (for (let k = 0; k < startLineIndex; k++) ... using
lyricWithEmptyLines) is O(n) every update; compute and cache a prefix-sum array
of non-pause counts alongside the memo that builds activeLines (the useMemo that
returns activeLines/startLineIndex/activeElementIndex) — e.g., include
nonPausePrefixCounts in that memo keyed to lyricWithEmptyLines and reuse it in
updateSpacerRef by reading nonPausePrefixCounts[startLineIndex] instead of
looping; update all references to expect the memo to return the extra prefix
data and keep the updateSpacerRef logic otherwise the same.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
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: CHILL
Plan: Pro
Run ID: 0da4abcb-c2e4-44e4-88bc-78b9974d2dc8
📒 Files selected for processing (2)
CustomApps/lyrics-plus/Pages.jsCustomApps/lyrics-plus/ProviderIanz56.js
Comment on lines
+283
to
+373
| lines.forEach((line) => { | ||
| const mainStart = line.begin; | ||
| let bgStart = mainStart; | ||
|
|
||
| if (line.backgroundVocal?.words?.length > 0) { | ||
| bgStart = line.backgroundVocal.words[0].begin; | ||
| } | ||
|
|
||
| // The line should start at the earliest timestamp | ||
| const lineStartTime = Math.min(mainStart, bgStart); | ||
|
|
||
| // Process main vocals | ||
| const mainWords = processWords(line.words || [], lineStartTime, false); | ||
|
|
||
| // Process background vocals | ||
| let backgroundWords = []; | ||
| let backgroundStartTime = 0; | ||
| let backgroundEndTime = 0; | ||
| if (line.backgroundVocal?.words) { | ||
| const bgWords = line.backgroundVocal.words; | ||
| if (bgWords.length > 0) { | ||
| backgroundStartTime = bgWords[0].begin * 1000; // in ms | ||
| backgroundEndTime = bgWords[bgWords.length - 1].end * 1000; | ||
| backgroundWords = processWords(bgWords, lineStartTime, true); | ||
| } | ||
| } | ||
|
|
||
| const isMainBackground = (line.words || []).length > 0 && (line.words || []).every((w) => w.isBackground); | ||
|
|
||
| // Calculate the effective end time (max of main line end and background vocal end) | ||
| let lineEndTime = line.end; | ||
| if (line.backgroundVocal?.words && line.backgroundVocal.words.length > 0) { | ||
| const bgWords = line.backgroundVocal.words; | ||
| const bgEndTime = bgWords[bgWords.length - 1].end; | ||
| lineEndTime = Math.max(lineEndTime, bgEndTime); | ||
| } | ||
|
|
||
| karaoke.push({ | ||
| startTime: Math.round(lineStartTime * 1000), | ||
| endTime: Math.round(lineEndTime * 1000), | ||
| backgroundStartTime: backgroundWords.length > 0 ? backgroundStartTime : undefined, | ||
| backgroundEndTime: backgroundWords.length > 0 ? backgroundEndTime : undefined, | ||
| text: mainWords, | ||
| isBackground: isMainBackground, | ||
| // Separate background vocal track | ||
| background: backgroundWords.length > 0 ? backgroundWords : undefined, | ||
| }); | ||
|
|
||
| const mainTextStr = (line.text || "").trim(); | ||
| const bgTextStr = backgroundWords | ||
| .map((w) => w.word) | ||
| .join("") | ||
| .replace(/\s+/g, " ") | ||
| .trim(); | ||
|
|
||
| let combinedText = mainTextStr; | ||
| if (bgTextStr) { | ||
| if (bgStart < mainStart) { | ||
| combinedText = `(${bgTextStr}) ${combinedText}`.trim(); | ||
| } else { | ||
| combinedText = `${combinedText} (${bgTextStr})`.trim(); | ||
| } | ||
| } | ||
|
|
||
| const isInline = (typeof CONFIG !== "undefined" && CONFIG?.visual?.["synced-background-inline"]) ?? false; | ||
|
|
||
| synced.push({ | ||
| startTime: Math.round(lineStartTime * 1000), | ||
| endTime: Math.round(lineEndTime * 1000), | ||
| text: isInline ? combinedText : mainTextStr || "", | ||
| background: !isInline && backgroundWords.length ? backgroundWords : undefined, | ||
| }); | ||
|
|
||
| if (isInline) { | ||
| if (combinedText) unsynced.push({ startTime: Math.round(lineStartTime * 1000), text: combinedText }); | ||
| } else { | ||
| let valText = mainTextStr; | ||
| if (bgTextStr) valText += ` (${bgTextStr})`; | ||
| if (valText) unsynced.push({ startTime: Math.round(lineStartTime * 1000), text: valText }); | ||
| } | ||
|
|
||
| const translatedText = (line.translation || "").trim(); | ||
| if (translatedText) hasTranslation = true; | ||
|
|
||
| ianz56Translation.push({ | ||
| startTime: Math.round(lineStartTime * 1000), | ||
| endTime: Math.round(lineEndTime * 1000), | ||
| text: translatedText || combinedText, | ||
| originalText: combinedText, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP --type=js -C2 '\bisMainBackground\b'Repository: ianz56/cli
Length of output: 963
🏁 Script executed:
rg -n '\bisBackground\b' --type=js -C3 | head -50Repository: ianz56/cli
Length of output: 3453
🏁 Script executed:
head -20 CustomApps/lyrics-plus/ProviderIanz56.jsRepository: ianz56/cli
Length of output: 657
🏁 Script executed:
rg -n 'Pages\.js' --type=js -lRepository: ianz56/cli
Length of output: 36
🏁 Script executed:
head -30 CustomApps/lyrics-plus/ProviderIanz56.jsRepository: ianz56/cli
Length of output: 826
🏁 Script executed:
find CustomApps/lyrics-plus -name "*.js" -type f | head -20Repository: ianz56/cli
Length of output: 618
🏁 Script executed:
rg -n 'karaoke' --type=js -A2 -B2 | grep -E '(Pages|render|\.isBackground)' | head -30Repository: ianz56/cli
Length of output: 149
🏁 Script executed:
sed -n '463,478p' CustomApps/lyrics-plus/Pages.jsRepository: ianz56/cli
Length of output: 586
🏁 Script executed:
rg -n '\bkaraoke\[' --type=js -A5 -B2 CustomApps/lyrics-plus/Pages.js | head -80Repository: ianz56/cli
Length of output: 36
🏁 Script executed:
rg -n '\.isBackground' --type=js CustomApps/lyrics-plus/ -C3Repository: ianz56/cli
Length of output: 609
🏁 Script executed:
rg -n 'indent|tabs|width|trailing|comma' CustomApps/lyrics-plus/ProviderIanz56.js --type=js | head -20Repository: ianz56/cli
Length of output: 36
🏁 Script executed:
cat CustomApps/lyrics-plus/.biome.json 2>/dev/null || cat biome.json 2>/dev/null || echo "No biome config found"Repository: ianz56/cli
Length of output: 708
🏁 Script executed:
# Check for any TypeScript reference in any file in CustomApps
grep -r "/// <reference" CustomApps/ --include="*.js" | head -10Repository: ianz56/cli
Length of output: 380
Add missing TypeScript reference and remove dead isBackground metadata.
ProviderIanz56.js is missing the required /// <reference path="../globals.d.ts" /> at the top of the file, violating the coding guideline for CustomApps/**/*.js files. Add this reference as the first line.
Additionally, the isMainBackground variable (line 310) and resulting isBackground field in karaoke entries (line 326) are dead code—they're never consumed by the rendering path in Pages.js or elsewhere in the codebase. Remove both to eliminate unnecessary metadata.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CustomApps/lyrics-plus/ProviderIanz56.js` around lines 283 - 373, Add the
missing TypeScript triple-slash reference by inserting /// <reference
path="../globals.d.ts" /> as the first line of ProviderIanz56.js, and remove the
dead isMainBackground variable and the isBackground property from the karaoke
objects (references: isMainBackground and the isBackground field inside the
karaoke.push call) so the karaoke entries no longer include unused metadata.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
CustomApps/lyrics-plus/Settings.js (1)
466-499: Use a functional state updater and de-duplicate the toggle handler.Both the
<label onClick>and theButtonSVG onClickcompute!modesOn[mode]from the captured closure value, which can drop updates if multiple toggles fire in the same render and also duplicates the same logic twice.♻️ Suggested cleanup
- Object.keys(modesOn).map(mode => { + Object.keys(modesOn).map(mode => { const labelMap = { karaoke: "Kara", synced: "Sync", unsynced: "Unsync" }; const label = labelMap[mode] || mode.charAt(0).toUpperCase() + mode.slice(1); + const toggleMode = () => { + setModesOn((prev) => { + const next = { ...prev, [mode]: !prev[mode] }; + if (onModeToggle) onModeToggle(item.name, mode, next[mode]); + return next; + }); + }; return react.createElement( "div", { style: { display: "flex", alignItems: "center", gap: "8px" }, key: mode }, react.createElement( "label", { style: { fontSize: "13px", opacity: 0.8, cursor: "pointer", userSelect: "none" }, - onClick: () => { - const newState = !modesOn[mode]; - setModesOn({ ...modesOn, [mode]: newState }); - if (onModeToggle) onModeToggle(item.name, mode, newState); - } + onClick: toggleMode, }, label ), react.createElement(ButtonSVG, { icon: Spicetify.SVGIcons.check, active: modesOn[mode], - onClick: () => { - const newState = !modesOn[mode]; - setModesOn({ ...modesOn, [mode]: newState }); - if (onModeToggle) onModeToggle(item.name, mode, newState); - } + onClick: toggleMode, }) ); })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CustomApps/lyrics-plus/Settings.js` around lines 466 - 499, The toggle logic reads from the stale closure value and is duplicated in both the label and ButtonSVG onClick handlers; refactor by extracting a single toggleMode handler (e.g., function toggleMode(mode) { setModesOn(prev => ({ ...prev, [mode]: !prev[mode] })); if (onModeToggle) onModeToggle(item.name, mode, !prev[mode]); }) and use it for both the label onClick and ButtonSVG onClick, using the functional updater on setModesOn to avoid lost updates and eliminate duplicated logic; ensure you reference the existing symbols modesOn, setModesOn, onModeToggle, item.name, and mode when implementing.
🤖 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/index.js`:
- Around line 1416-1440: The settings button (element with className
"lyrics-config-button" rendered inside Spicetify.ReactComponent.TooltipWrapper
and using onClick handler triggerOpenLyricsPlusSettings) is missing an
aria-label; update the button props to include an appropriate aria-label (e.g.,
"Open Lyrics Plus settings") so screen readers receive an accessible name while
keeping the existing TooltipWrapper and onClick intact.
- Around line 508-512: The current gating with service.modesOn only nulls
data.karaoke/synced/unsynced but leaves provider translation objects (e.g.,
ianz56Translation, neteaseTranslation, musixmatchTranslation) that contain
startTime/endTime, causing untimed translations to be merged without their timed
anchors; after the existing modesOn block (and/or before the merge into
finalData) detect whether any timed lyric arrays survived (data.karaoke ||
data.synced || data.unsynced) and if none did, explicitly set timing-dependent
translation fields (ianz56Translation, neteaseTranslation,
musixmatchTranslation, etc.) to null so they cannot be merged into finalData
without their corresponding timed lyrics; update the same conditional logic used
for data.genius if needed and ensure the merge loop that produces finalData
skips translation objects that are null.
In `@CustomApps/lyrics-plus/Settings.js`:
- Around line 596-615: The overlay cleanup currently only removes
existingOverlay._keydownHandler (handleEscape) but leaves handleFocusTrap and
handleFocusIn attached, causing listener leaks; update the teardown to either
(A) persist all three handlers (e.g., overlay._keydownHandler,
overlay._focusTrapHandler, overlay._focusInHandler) when you attach them and
remove each with document.removeEventListener("keydown",
overlay._focusTrapHandler) and document.removeEventListener("focusin",
overlay._focusInHandler) in the existingOverlay cleanup block, or (B) implement
the module-level activeConfigOverlayCleanup pattern so that closeOverlay (and
its finally block) unregisters all attached handlers and clears
activeConfigOverlayCleanup, then call that cleanup here before removing the DOM
node; reference the existing functions/idents: existingOverlay, _keydownHandler,
handleEscape, handleFocusTrap, handleFocusIn, closeOverlay, and
activeConfigOverlayCleanup.
- Around line 808-818: The deferred focus timer can re-focus a removed panel;
update the setTimeout callback to bail out if the overlay is closing or the
panel is no longer in the document: inside the timeout check the isClosing flag
and verify the panel is still connected (e.g., panel.isConnected or
document.contains(panel)) before calling
getFocusableElements()/closeButton/panel.focus(); alternatively clear the timer
when closeOverlay runs—ensure you reference the existing setTimeout block,
getFocusableElements(), closeButton, panel, isClosing, closeOverlay, and
prevFocus when making the guard.
- Around line 800-806: The overlay mounting currently no-ops if
Spicetify.ReactDOM provides neither createRoot nor render; add an else branch
after the existing checks that logs a clear warning and fallback notice so the
failure is visible. Specifically, update the block around
Spicetify.ReactDOM.createRoot / Spicetify.ReactDOM.render to include an else
that logs a warning (e.g., console.warn) mentioning that configContainer was not
rendered into contentHost, and also call any available user-visible notifier
(e.g., Spicetify.showNotification or Spicetify.React?.toast) if present; keep
reactRoot, configContainer, and contentHost usage unchanged otherwise. Ensure
the message references the missing ReactDOM APIs (createRoot/render) so it's
easy to debug.
---
Nitpick comments:
In `@CustomApps/lyrics-plus/Settings.js`:
- Around line 466-499: The toggle logic reads from the stale closure value and
is duplicated in both the label and ButtonSVG onClick handlers; refactor by
extracting a single toggleMode handler (e.g., function toggleMode(mode) {
setModesOn(prev => ({ ...prev, [mode]: !prev[mode] })); if (onModeToggle)
onModeToggle(item.name, mode, !prev[mode]); }) and use it for both the label
onClick and ButtonSVG onClick, using the functional updater on setModesOn to
avoid lost updates and eliminate duplicated logic; ensure you reference the
existing symbols modesOn, setModesOn, onModeToggle, item.name, and mode when
implementing.
🪄 Autofix (Beta)
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: CHILL
Plan: Pro
Run ID: 52bd6973-ff9f-4f7c-aad8-ec513b704f09
📒 Files selected for processing (2)
CustomApps/lyrics-plus/Settings.jsCustomApps/lyrics-plus/index.js
* check * fix word and timing sylablle * fix container word * add bg * warp * overlap bg * fix bug * fix bg size scale * add idling * fix: apply CodeRabbit auto-fixes Fixed 3 file(s) based on 4 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai> * lint --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/index.js`:
- Around line 517-551: The current auto-mode logic returns immediately when
data.synced is present, which stops scanning later providers that might have
data.karaoke; change the behavior in the block guarded by mode === -1 so that
you only return early when data.karaoke is found (set
providerKaraoke/copyrightKaraoke and return), but when data.synced is true just
record providerSynced/copyrightSynced and continue scanning (update finalData as
a fallback if needed); ensure you still handle data.unsynced and data.genius the
same way (set providerUnsynced/providerGenius fields and use finalData as
fallback) so the loop only exits early on karaoke or after exhausting providers.
In `@CustomApps/lyrics-plus/Pages.js`:
- Around line 77-93: The pause branch pushes a shifted pause line but fails to
provide a finite endTime, which lets clustering treat it as spanning to
Infinity; update the object pushed into result inside the if (pauseDuration >=
LONG_PAUSE_THRESHOLD) block to include a finite endTime (use nextStart), e.g.
set endTime: Math.min(nextStart, line.endTime ?? nextStart) so you don’t extend
an existing endTime beyond the next line; this change affects the code paths
around isPauseLine, findNextLineStartTime, LONG_PAUSE_THRESHOLD and KARA_DELAY
and ensures pause rows won’t absorb subsequent lines during clustering.
In `@CustomApps/lyrics-plus/Settings.js`:
- Around line 75-82: The Spicetify.CosmosAsync.get call that requests the
Musixmatch token must be replaced with a standard fetch call: remove the
Spicetify.CosmosAsync.get invocation and use await
fetch("https://apic-appmobile.musixmatch.com/ws/1.1/token.get?app_id=mac-ios-v2.0",
{ method: "GET", headers: { authority: "...", "x-mxm-app-version": "10.1.1",
"User-Agent": "Musixmatch/2025120901 CFNetwork/3860.300.31 Darwin/25.2.0",
"Accept-Language": "en-US,en;q=0.9", Connection: "keep-alive", Accept:
"application/json" }, credentials: "omit" }) to ensure no Spotify auth context
is sent; then await response.json() and handle errors similarly to the original
flow (reject/report on non-2xx or JSON parse errors). Target the
Spicetify.CosmosAsync.get(...) call in Settings.js and replace its usage with
this fetch-based flow, preserving existing variable names and downstream logic
that consumes the token response.
🪄 Autofix (Beta)
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: CHILL
Plan: Pro
Run ID: 2deb1529-057b-4b4f-9701-5d13d5e30321
📒 Files selected for processing (6)
CustomApps/lyrics-plus/OptionsMenu.jsCustomApps/lyrics-plus/Pages.jsCustomApps/lyrics-plus/ProviderIanz56.jsCustomApps/lyrics-plus/Settings.jsCustomApps/lyrics-plus/index.jsCustomApps/lyrics-plus/style.css
Comment on lines
517
to
+551
| if (mode === -1) { | ||
| finalData = data; | ||
| return finalData; | ||
| // In auto mode, if we found karaoke or synced, we can just return immediately | ||
| if (data.karaoke || data.synced) { | ||
| if (data.karaoke) { | ||
| data.providerKaraoke = data.provider; | ||
| data.copyrightKaraoke = data.copyright; | ||
| } | ||
| if (data.synced) { | ||
| data.providerSynced = data.provider; | ||
| data.copyrightSynced = data.copyright; | ||
| } | ||
| if (data.unsynced) { | ||
| data.providerUnsynced = data.provider; | ||
| data.copyrightUnsynced = data.copyright; | ||
| } | ||
| if (data.genius) { | ||
| data.providerGenius = data.provider; | ||
| data.copyrightGenius = data.copyright; | ||
| } | ||
| finalData = data; | ||
| return finalData; | ||
| } | ||
| // Otherwise (only unsynced/genius), we save it but keep looking for a better mode | ||
| if (!finalData.provider) { | ||
| if (data.unsynced) { | ||
| data.providerUnsynced = data.provider; | ||
| data.copyrightUnsynced = data.copyright; | ||
| } | ||
| if (data.genius) { | ||
| data.providerGenius = data.provider; | ||
| data.copyrightGenius = data.copyright; | ||
| } | ||
| finalData = data; | ||
| } | ||
| continue; |
There was a problem hiding this comment.
Don't let auto mode stop at the first synced provider.
This makes provider order outrank lyric quality. If an earlier provider only has synced, auto mode returns before later providers can contribute karaoke, even though render still prefers karaoke when it exists. Keep synced as the current fallback, but keep scanning until you either find karaoke or exhaust the list.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CustomApps/lyrics-plus/index.js` around lines 517 - 551, The current
auto-mode logic returns immediately when data.synced is present, which stops
scanning later providers that might have data.karaoke; change the behavior in
the block guarded by mode === -1 so that you only return early when data.karaoke
is found (set providerKaraoke/copyrightKaraoke and return), but when data.synced
is true just record providerSynced/copyrightSynced and continue scanning (update
finalData as a fallback if needed); ensure you still handle data.unsynced and
data.genius the same way (set providerUnsynced/providerGenius fields and use
finalData as fallback) so the loop only exits early on karaoke or after
exhausting providers.
Comment on lines
+77
to
+93
| if (isPauseLine(line)) { | ||
| // Skip consecutive pause lines to consolidate them into one idling indicator | ||
| const lastLine = result[result.length - 1]; | ||
| if (lastLine && isPauseLine(lastLine)) { | ||
| continue; | ||
| } | ||
|
|
||
| const nextStart = findNextLineStartTime(lyrics, i); | ||
| const pauseStart = line.startTime || 0; | ||
| if (nextStart != null) { | ||
| const pauseDuration = nextStart - pauseStart; | ||
| if (pauseDuration >= LONG_PAUSE_THRESHOLD) { | ||
| result.push({ | ||
| ...line, | ||
| startTime: line.startTime + (isKara ? KARA_DELAY : 0), | ||
| }); | ||
| } |
There was a problem hiding this comment.
Pause rows need a finite endTime before they reach the clustering code.
When an existing pause line is kept here, it only gets a shifted startTime. Later in both synced renderers, clustering uses line.endTime ?? Infinity, so a pause row with no endTime can absorb every following line into one cluster and throw off focus/active-line selection after long gaps.
Suggested fix
if (isPauseLine(line)) {
// Skip consecutive pause lines to consolidate them into one idling indicator
const lastLine = result[result.length - 1];
if (lastLine && isPauseLine(lastLine)) {
continue;
}
const nextStart = findNextLineStartTime(lyrics, i);
const pauseStart = line.startTime || 0;
if (nextStart != null) {
const pauseDuration = nextStart - pauseStart;
if (pauseDuration >= LONG_PAUSE_THRESHOLD) {
result.push({
...line,
startTime: line.startTime + (isKara ? KARA_DELAY : 0),
+ endTime: nextStart,
});
}
}
} else {🤖 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 77 - 93, The pause branch
pushes a shifted pause line but fails to provide a finite endTime, which lets
clustering treat it as spanning to Infinity; update the object pushed into
result inside the if (pauseDuration >= LONG_PAUSE_THRESHOLD) block to include a
finite endTime (use nextStart), e.g. set endTime: Math.min(nextStart,
line.endTime ?? nextStart) so you don’t extend an existing endTime beyond the
next line; this change affects the code paths around isPauseLine,
findNextLineStartTime, LONG_PAUSE_THRESHOLD and KARA_DELAY and ensures pause
rows won’t absorb subsequent lines during clustering.
… accessibility and event cleanup, and fix default sync-background configuration
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements
Documentation