feat(lyrics-plus): enhance Musixmatch integration with translation status and language handling #3562
Conversation
…atus and language handling
WalkthroughAdds multi-language Musixmatch translation support: new translation-prefix persistence, translation-source normalization, extraction of available translation statuses from API responses, trackId-based translation fetching, UI options for Musixmatch languages, state fields and caching, and config/localStorage synchronization. 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: update 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 track_lyrics_translation_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) | ⏱️ ~50 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)
🧰 Additional context used🧬 Code graph analysis (1)CustomApps/lyrics-plus/Settings.js (1)
🔇 Additional comments (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.
2 participants
Added dynamic language selection for Musixmatch translations. The dropdown now reflects available languages per track.
Summary by CodeRabbit
New Features
Bug Fixes