feat(lyrics-plus): enhance Musixmatch integration with translation status and language handling #3562
Conversation
…atus and language handling
WalkthroughAdds multi-language Musixmatch translation support: extracts translation status from API responses, fetches translations by trackId, exposes available translations to the container, dynamically builds Musixmatch translation UI options, synchronizes selected language with CONFIG/localStorage, and adds container-level refresh/caching to apply translations per track and language. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant OptionsMenu
participant Settings
participant LyricsContainer
participant Providers
participant ProviderMusixmatch
participant MusixmatchAPI as API
User->>OptionsMenu: select Musixmatch language
OptionsMenu->>Settings: save translate:translated-lyrics-source + musixmatch-translation-language
Settings->>LyricsContainer: lyricContainerUpdate()
LyricsContainer->>LyricsContainer: resolveTranslationSource()
LyricsContainer->>LyricsContainer: refreshMusixmatchTranslation()
LyricsContainer->>Providers: request lyrics (include translation metadata)
Providers->>ProviderMusixmatch: forward request (adds track_lyrics_translation_status)
ProviderMusixmatch->>MusixmatchAPI: fetch lyrics + status
MusixmatchAPI-->>ProviderMusixmatch: lyrics with status
ProviderMusixmatch-->>Providers: return augmented body (__musixmatchTranslationStatus, __musixmatchTrackId)
Providers-->>LyricsContainer: provider result (with available translations)
alt selected language available
LyricsContainer->>ProviderMusixmatch: getTranslation(trackId)
ProviderMusixmatch->>MusixmatchAPI: fetch translation for trackId/language
MusixmatchAPI-->>ProviderMusixmatch: translation payload
ProviderMusixmatch-->>LyricsContainer: translation array
LyricsContainer->>LyricsContainer: apply translation mapping to lyrics
else unavailable
LyricsContainer->>LyricsContainer: clear or skip translation
end
LyricsContainer->>User: render lyrics (with/without translation)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CustomApps/lyrics-plus/ProviderMusixmatch.js (1)
205-238: Handle network errors in getTranslation to avoid unhandled rejectionsCosmosAsync.get can throw; without try/catch, refresh paths may break. Return null on errors.
- async function getTranslation(trackId) { + async function getTranslation(trackId) { if (!trackId) return null; @@ - let result = await Spicetify.CosmosAsync.get(finalURL, null, headers); + let result; + try { + result = await Spicetify.CosmosAsync.get(finalURL, null, headers); + } catch (e) { + console.error("Musixmatch translation request failed", e); + return null; + }
🧹 Nitpick comments (3)
CustomApps/lyrics-plus/OptionsMenu.js (1)
121-135: Avoid duplicate Musixmatch language optionsAfter pushing the active language, ensure uniqueness to prevent duplicate menu entries.
- const availableMusixmatchLanguages = Array.isArray(musixmatchLanguages) ? [...new Set(musixmatchLanguages.filter(Boolean))] : []; - const activeMusixmatchLanguage = + let availableMusixmatchLanguages = Array.isArray(musixmatchLanguages) ? [...new Set(musixmatchLanguages.filter(Boolean))] : []; + const activeMusixmatchLanguage = musixmatchSelectedLanguage && musixmatchSelectedLanguage !== "none" ? musixmatchSelectedLanguage : null; if (hasTranslation.musixmatch && activeMusixmatchLanguage) { - availableMusixmatchLanguages.push(activeMusixmatchLanguage); + availableMusixmatchLanguages = [...new Set([...availableMusixmatchLanguages, activeMusixmatchLanguage])]; }CustomApps/lyrics-plus/index.js (1)
297-379: Harden refreshMusixmatchTranslation against fetch errors and unmountsGreat race checks and cache updates. Add a try/catch around the translation fetch; optionally guard setState after unmount.
- const translation = await ProviderMusixmatch.getTranslation(trackId); + let translation = null; + try { + translation = await ProviderMusixmatch.getTranslation(trackId); + } catch (e) { + console.error("refreshMusixmatchTranslation failed", e); + translation = null; + }Optional (nice-to-have): track mount status to avoid setState after unmount.
class LyricsContainer extends react.Component { constructor() { super(); + this._mounted = false; @@ componentDidMount() { + this._mounted = true; @@ componentWillUnmount() { + this._mounted = false; @@ async refreshMusixmatchTranslation() { // ... - this.setState({ + if (!this._mounted) return; + this.setState({ musixmatchTranslation: null, musixmatchTranslationLanguage: null, }); @@ - this.setState({ + if (!this._mounted) return; + this.setState({ musixmatchTranslation: mappedTranslation, musixmatchTranslationLanguage: currentLanguage, });CustomApps/lyrics-plus/Settings.js (1)
700-709: Remove duplicate lyricContainerUpdate call; init order is safe via optional chainingThe duplicate call at lines 706 and 708 should be consolidated. Move
lyricContainerUpdate?.();outside the if-else block:// Reload Lyrics if translation language is changed if (name === "musixmatch-translation-language") { if (value === "none") { CONFIG.visual["translate:translated-lyrics-source"] = "none"; localStorage.setItem(`${APP_NAME}:visual:translate:translated-lyrics-source`, "none"); } } lyricContainerUpdate?.();Init order is already safe:
lyricContainerUpdateis assigned incomponentDidMount()before user interactions can occur, and optional chaining here guards against edge cases. The refactoring is sound.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
CustomApps/lyrics-plus/OptionsMenu.js(6 hunks)CustomApps/lyrics-plus/ProviderMusixmatch.js(5 hunks)CustomApps/lyrics-plus/Providers.js(2 hunks)CustomApps/lyrics-plus/Settings.js(1 hunks)CustomApps/lyrics-plus/index.js(14 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
CustomApps/lyrics-plus/Providers.js (2)
CustomApps/lyrics-plus/index.js (1)
CONFIG(40-106)CustomApps/lyrics-plus/ProviderMusixmatch.js (1)
ProviderMusixmatch(1-241)
CustomApps/lyrics-plus/ProviderMusixmatch.js (1)
CustomApps/lyrics-plus/Providers.js (6)
body(14-14)result(3-10)result(47-59)result(108-116)result(151-158)result(207-213)
CustomApps/lyrics-plus/OptionsMenu.js (1)
CustomApps/lyrics-plus/index.js (4)
react(6-6)react(7-7)CONFIG(40-106)APP_NAME(23-23)
CustomApps/lyrics-plus/index.js (2)
CustomApps/lyrics-plus/ProviderMusixmatch.js (1)
ProviderMusixmatch(1-241)CustomApps/lyrics-plus/Utils.js (1)
Utils(1-338)
CustomApps/lyrics-plus/Settings.js (1)
CustomApps/lyrics-plus/index.js (1)
lyricContainerUpdate(158-158)
🔇 Additional comments (12)
CustomApps/lyrics-plus/Providers.js (1)
54-57: New Musixmatch fields on result look goodAdding musixmatchAvailableTranslations, musixmatchTrackId, and musixmatchTranslationLanguage makes downstream state handling straightforward. LGTM.
CustomApps/lyrics-plus/ProviderMusixmatch.js (3)
7-35: Recursive translation-status extraction looks goodTraversal handles objects/arrays and returns early on first match. Works for current macro response shapes.
52-53: Including part=track_lyrics_translation_status is correctEnsures the macro response contains translation status; necessary for dynamic language options.
78-91: Non-enumerable augmentation for status and trackId is neatUsing Object.defineProperties to stash __musixmatchTranslationStatus and __musixmatchTrackId avoids polluting consumers’ iterations. LGTM.
CustomApps/lyrics-plus/OptionsMenu.js (2)
89-96: Prefix helper aligns with global prefix useFetching the prefix from window with a sane default decouples UI from internal constants. Good.
266-278: Syncing musixmatch-translation-language with source selection is correctThis keeps config/UI consistent when a Musixmatch-prefixed source is chosen or cleared.
CustomApps/lyrics-plus/index.js (6)
23-33: Globalizable translation prefix is well factoredConstants + window propagation enable cross-file access without tight coupling. LGTM.
125-142: Backward-compat upgrade path is solidMigrates legacy "musixmatchTranslation" to the new prefixed format and syncs musixmatch-translation-language. Nice touch.
146-156: Empty state extended with Musixmatch fieldsMatches downstream usage and avoids stale data when switching tracks. Looks good.
166-177: resolveTranslationSource keeps state normalizedCleanly maps prefixed sources to a key+language pair, reducing special-casing elsewhere.
483-505: Good UX: reset selection when chosen language isn’t availableAuto-clearing prefixed source and language avoids stuck states when a track lacks that translation.
593-606: Source-language sync on render path is correctKeeps CONFIG.visual in sync with resolved source; reduces drift between menu and runtime state.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CustomApps/lyrics-plus/Providers.js (1)
94-121: Excellent O(n) optimization addresses past review concerns!The translation mapping now uses a
Mapfor O(1) lookups instead of nestedfind(), resolving the O(n²) performance issue flagged in the previous review. The fallback normalization function appropriately handles cases whereUtilsis unavailable due to module loading order.However, consider adding defensive checks for the translation entries:
Apply this diff to guard against malformed API responses:
const translationMap = new Map(); for (const entry of translation) { + if (!entry || typeof entry.matchedLine !== "string" || typeof entry.translation !== "string") { + continue; + } const normalizedMatched = normalizeLyrics(entry.matchedLine); - if (!translationMap.has(normalizedMatched)) { + if (normalizedMatched && !translationMap.has(normalizedMatched)) { translationMap.set(normalizedMatched, entry.translation); } }This ensures:
- Entries with missing or non-string properties are skipped
- Empty normalized keys (from empty
matchedLine) don't create Map entries- The mapping remains robust against unexpected API responses
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
CustomApps/lyrics-plus/Providers.js(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
CustomApps/lyrics-plus/Providers.js (3)
CustomApps/lyrics-plus/Utils.js (2)
result(22-36)Utils(1-338)CustomApps/lyrics-plus/index.js (2)
CONFIG(40-106)value(19-19)CustomApps/lyrics-plus/ProviderMusixmatch.js (1)
ProviderMusixmatch(1-241)
🔇 Additional comments (3)
CustomApps/lyrics-plus/Providers.js (3)
54-56: LGTM! Clean field initialization.The new fields for translation support are properly initialized with appropriate default values (empty array for translations, null for trackId and language).
87-88: LGTM! Robust metadata extraction.The defensive checks ensure safe handling of the properties attached by
ProviderMusixmatch.findLyrics, with appropriate fallbacks for missing or malformed data.
90-92: LGTM! Effective translation request gating.The logic correctly validates that a translation language is configured and available before making the API request, preventing unnecessary calls.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
CustomApps/lyrics-plus/OptionsMenu.js (1)
89-95: Consider caching the prefix retrieval.The function reads from
windowon every call. Since the prefix is set once globally and doesn't change, consider retrieving it once at module initialization rather than recalculating it each render.Apply this diff to optimize:
+const MUSIXMATCH_TRANSLATION_PREFIX = + typeof window !== "undefined" && typeof window.__lyricsPlusMusixmatchTranslationPrefix === "string" + ? window.__lyricsPlusMusixmatchTranslationPrefix + : "musixmatchTranslation:"; + -function getMusixmatchTranslationPrefix() { - if (typeof window !== "undefined" && typeof window.__lyricsPlusMusixmatchTranslationPrefix === "string") { - return window.__lyricsPlusMusixmatchTranslationPrefix; - } - - return "musixmatchTranslation:"; -}Then replace
getMusixmatchTranslationPrefix()calls with the constant.CustomApps/lyrics-plus/index.js (2)
337-340: Clarify base lyrics selection.Line 337 uses nullish coalescing (
??) to prefersyncedoverunsynced. The early return at line 339 is correct, but the comment could clarify why the function returns early rather than clearing translation when base lyrics are unavailable.Add a comment:
const baseLyrics = this.state.synced ?? this.state.unsynced; if (!baseLyrics) { + // No base lyrics available to translate; preserve existing state return; }
509-530: Complex conditional logic—verify correctness.Lines 510-515 determine
shouldRefreshMusixmatchTranslationbased on trackId, selected language, available translations, and cached state. Lines 516-529 reset the translation source to"none"if the selected language is unavailable.This logic appears correct but is dense. Consider extracting helper functions for readability.
Example refactor:
const isMusixmatchLanguageAvailable = (language, availableTranslations) => { return language !== "none" && Array.isArray(availableTranslations) && availableTranslations.includes(language); };Then use it to simplify the conditionals.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CustomApps/lyrics-plus/OptionsMenu.js(6 hunks)CustomApps/lyrics-plus/index.js(14 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
CustomApps/lyrics-plus/OptionsMenu.js (1)
CustomApps/lyrics-plus/index.js (4)
react(6-6)react(7-7)CONFIG(42-108)APP_NAME(23-23)
🔇 Additional comments (19)
CustomApps/lyrics-plus/OptionsMenu.js (5)
97-98: LGTM!The props extension and prefix retrieval setup correctly support dynamic Musixmatch language options.
170-223: LGTM!The refactor from a direct return to constructing
configItemsenables dynamic dependency tracking. The expanded memoization array correctly includes all derived values that affect the menu structure.
235-235: Correct dependency addition.Including
itemsin theuseEffectdependency array ensures the custom event fires whenever the menu structure changes, which is necessary for the dynamic Musixmatch options.
262-278: Synchronization logic looks correct.The code extracts the language suffix from prefixed values, updates both CONFIG and localStorage, and maintains consistency. The fallback to
"none"is appropriate.
121-135: No upstream duplicates—the review comment's concern is not substantiated.The data flow shows that
__musixmatchTranslationStatusis created in ProviderMusixmatch.js (line 80) with explicit deduplication:[...new Set(translationStatus.map(...).filter(Boolean))]. This already-deduplicated array is passed unchanged through Providers.js to OptionsMenu, which then deduplicates it again (line 122, redundantly but safely) before the conditional push at lines 125–126.The review comment asks to confirm no callers pass duplicate-laden arrays. The script results confirm they don't—the immediate upstream source deduplicates at the origin. The redundant dedup in OptionsMenu.js is defensive programming, and the potential duplicate from the unconditional push at line 126 is harmless since the reduce at line 129 overwrites duplicate keys.
CustomApps/lyrics-plus/index.js (14)
24-35: LGTM!Constants are well-defined with clear naming. The prefix resolution checks
windowsafely and falls back to the default. Persisting the prefix to the global scope enables cross-module coordination.
127-154: Migration logic handles legacy state correctly.The code upgrades old
"musixmatchTranslation"values to prefixed format, synchronizesmusixmatch-translation-languagebidirectionally with the translation source, and enforces mutual exclusivity betweentranslateand translation providers.
164-167: State fields appropriately extended.Adding
musixmatchAvailableTranslations,musixmatchTrackId, andmusixmatchTranslationLanguagetoemptyStateensures proper reset when transitioning between tracks.
177-188: Helper function correctly resolves translation sources.The logic distinguishes prefixed Musixmatch sources from other translation keys and extracts the language code. The fallback for non-prefixed sources is appropriate.
209-212: Initialization looks correct.The constructor properly initializes Musixmatch state fields and
currentMusixmatchLanguagefrom CONFIG.Also applies to: 247-247
386-395: Translation mapping logic is sound.The code maps base lyrics to translated text by matching processed original text. Preserving
originalTextenables future re-translation. The fallback toline.textwhen no match is found prevents data loss.
575-597: State update callback correctly triggers refresh.The callback at line 591 ensures
currentMusixmatchLanguageis updated before invokingrefreshMusixmatchTranslation. This ordering is critical for the refresh logic to use the correct language.
601-606: Callback ordering matches initial load.Consistent with lines 591-596, this ensures proper sequencing when updating state after cache retrieval.
621-626: Translation source resolution integrated correctly.Using
resolveTranslationSourceto extract the language and synchronize CONFIG is appropriate. This keeps the Musixmatch language in sync with the UI selection.
894-897: Language change detection triggers refresh appropriately.Comparing
currentMusixmatchLanguagewith CONFIG detects user-driven changes and invokes the refresh. This is the correct place to trigger translation updates in response to UI interactions.
900-900: Binding ensures correct context.Binding
refreshMusixmatchTranslationto the instance and assigning it to the module-level variable enables external invocation while preservingthiscontext.
955-955: Cleanup prevents stale references.Nulling the module-level
refreshMusixmatchTranslationon unmount prevents dangling references and potential memory leaks.
1032-1033: Updated hasTranslation logic includes Musixmatch correctly.
hasMusixmatchLanguageschecks for available translations, andhasTranslationnow considers Musixmatch alongside Netease. This ensures the translation button appears when Musixmatch languages are available.
1126-1127: Props passed correctly to TranslationMenu.
musixmatchLanguagesandmusixmatchSelectedLanguageare sourced from state and CONFIG, providing the UI with the necessary data to render dynamic options.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
CustomApps/lyrics-plus/index.js (2)
127-153: Migration logic runs on every load.The CONFIG upgrade logic correctly handles migrating from the old
"musixmatchTranslation"format to the new prefixed format, and synchronizes themusixmatch-translation-languagesetting. However, this migration runs on every app initialization rather than once.Consider adding a version flag to localStorage to run this migration only once:
+const MIGRATION_VERSION = "1"; +const currentMigrationVersion = localStorage.getItem(`${APP_NAME}:migration-version`) || "0"; + +if (currentMigrationVersion < MIGRATION_VERSION) { if (CONFIG.visual["translate:translated-lyrics-source"] === "musixmatchTranslation") { // ... migration logic ... } // ... other migrations ... + localStorage.setItem(`${APP_NAME}:migration-version`, MIGRATION_VERSION); +}
602-633: LGTM - Proper async refresh after state updates.The callbacks (lines 618-623, 629-632) correctly trigger Musixmatch translation refresh after React has committed the state changes.
The callback logic is duplicated between lines 618-623 and 629-632. Consider extracting to a helper:
const afterStateUpdate = () => { this.currentMusixmatchLanguage = CONFIG.visual["musixmatch-translation-language"]; if (shouldRefreshMusixmatchTranslation) { this.refreshMusixmatchTranslation(); } }; this.setState({ ...tempState, ...translationOverrides }, afterStateUpdate);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
CustomApps/lyrics-plus/index.js(14 hunks)
🔇 Additional comments (11)
CustomApps/lyrics-plus/index.js (11)
24-35: LGTM - Global prefix persistence pattern.The use of a window global to persist the Musixmatch translation prefix across module reloads is appropriate, and the defensive
typeofchecks prevent errors in non-browser environments.
164-167: LGTM - emptyState extended for Musixmatch fields.The new fields properly initialize Musixmatch-related state and will be used for state resets.
171-171: Module-level binding follows existing pattern.The
refreshMusixmatchTranslationmodule-level variable follows the same pattern aslyricContainerUpdateandreloadLyrics(lines 169-170), enabling external references to instance methods.
177-188: LGTM - Clean abstraction for translation source resolution.The function correctly parses Musixmatch translation sources with language prefixes and handles edge cases (non-string sources, unprefixed sources).
209-212: LGTM - Proper state initialization.The Musixmatch-related state fields are correctly initialized, with
musixmatchAvailableTranslationsas an empty array and the request ID tracker for preventing race conditions.Also applies to: 247-248
309-432: Race condition protection implemented.The Symbol-based request tracking (lines 314-316) and
isLatestRequest()guards (lines 395, 418) properly address the concurrent fetch concern from the previous review.
536-558: Translation refresh logic is sound.The code correctly determines when Musixmatch translation needs refreshing (lines 537-542) and resets unavailable language selections to
"none"(lines 543-556). ThetranslationOverridespattern (line 557) appropriately clears stale translation data before the async refresh.
920-927: LGTM - Proper handling of external config changes.The language change detection (lines 921-924) correctly triggers translation refresh when settings are updated externally, and the module-level binding (line 927) enables coordinated cleanup.
982-982: LGTM - Proper cleanup of module-level reference.Clearing
refreshMusixmatchTranslationon unmount prevents memory leaks and follows the existing cleanup pattern.
1059-1060: LGTM - Correct translation availability detection.The logic properly determines translation availability by checking for existing translations or available language options, enabling the translation menu when appropriate.
1153-1154: LGTM - Appropriate fallback for selected language.The props correctly pass available languages and the selected language (with appropriate fallback to config when translation hasn't been fetched yet).
|
Address coderabbit's actionable review |
2 participants
Added dynamic language selection for Musixmatch translations. The dropdown now reflects available languages per track.
Summary by CodeRabbit
New Features
Bug Fixes