feat(lyrics-plus): convert musixmatch translation name from iso to language name #3672
Conversation
📝 WalkthroughWalkthroughAdds ProviderMusixmatch.getLanguages() to fetch and cache Musixmatch language mappings; OptionsMenu (TranslationMenu) loads that mapping into state and prefers it when resolving Musixmatch language labels. Also adjusts UI CSS to allow horizontal scrolling and spacing tweaks. Changes
Sequence DiagramsequenceDiagram
actor User
participant TM as TranslationMenu (OptionsMenu.js)
participant PM as ProviderMusixmatch
participant LS as localStorage
participant API as Musixmatch API
rect rgb(220,250,220)
note over TM,PM: Component mounts → request language map
TM->>PM: getLanguages()
activate PM
PM->>LS: read cached languages (versioned key)
alt cache valid
LS-->>PM: return cached mapping
else cache missing/invalid
PM->>API: fetch /languages
activate API
API-->>PM: languages JSON (codes + names)
deactivate API
PM->>PM: build ISO(1/3) → capitalized-name map
PM->>LS: write mapping with __version
LS-->>PM: stored
end
PM-->>TM: languageMap
deactivate PM
TM->>TM: setState(languageMap)
end
rect rgb(230,240,250)
note over TM: Render translation options
TM->>TM: build options array
TM->>TM: for each code prefer languageMap[code] else fallback (musixmatchDisplay.of / uppercase)
TM->>User: display updated labels
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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
🧹 Nitpick comments (3)
CustomApps/lyrics-plus/ProviderMusixmatch.js (2)
240-242: LGTM with minor concurrency consideration.The module-level cache and early return pattern work well for avoiding redundant API calls. However, if
getLanguages()is called multiple times concurrently beforelanguageMapis set, all calls will proceed to fetch from the API.Optional: Prevent duplicate concurrent API calls
If you want to prevent duplicate API calls during concurrent invocations, consider caching the promise itself:
let languageMap = null; +let languageMapPromise = null; async function getLanguages() { - if (languageMap) return languageMap; + if (languageMap) return languageMap; + if (languageMapPromise) return languageMapPromise; + + languageMapPromise = (async () => { + // existing fetch logic... + return languageMap; + })(); + + return languageMapPromise;This is a low-priority optimization and not necessary for correctness.
269-287: API integration looks good with minor capitalization consideration.The fetch logic, error handling, and dual ISO code mapping (lines 277-278) are well implemented. The function gracefully returns an empty object on failure, allowing the UI to fall back to alternative display methods.
One minor consideration: Line 276's capitalization logic (
charAt(0).toUpperCase() + slice(1)) only capitalizes the first letter. This works well for single-word language names but may not properly title-case multi-word names (e.g., "united states" becomes "United states" instead of "United States"). However, if the Musixmatch API consistently returns single-word names or already properly formatted names, this is fine.CustomApps/lyrics-plus/OptionsMenu.js (1)
102-106: Add error handling for the promise.The defensive check for
ProviderMusixmatchexistence is good. However, the promise returned bygetLanguages()lacks error handling. While the implementation returns an empty object on fetch failures, adding explicit error handling would make the code more robust and prevent potential unhandled promise rejections.Suggested fix
react.useEffect(() => { if (typeof ProviderMusixmatch !== "undefined") { - ProviderMusixmatch.getLanguages().then(setLanguageMap); + ProviderMusixmatch.getLanguages() + .then(setLanguageMap) + .catch((error) => { + console.error("Failed to load language map", error); + }); } }, []);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CustomApps/lyrics-plus/OptionsMenu.jsCustomApps/lyrics-plus/ProviderMusixmatch.js
🧰 Additional context used
🧬 Code graph analysis (2)
CustomApps/lyrics-plus/OptionsMenu.js (1)
CustomApps/lyrics-plus/ProviderMusixmatch.js (2)
languageMap(240-240)ProviderMusixmatch(1-291)
CustomApps/lyrics-plus/ProviderMusixmatch.js (2)
CustomApps/lyrics-plus/OptionsMenu.js (1)
languageMap(100-100)CustomApps/lyrics-plus/index.js (1)
CONFIG(42-108)
🔇 Additional comments (4)
CustomApps/lyrics-plus/ProviderMusixmatch.js (1)
290-290: LGTM!Properly exposes the new
getLanguagesfunction in the module's public API, consistent with existing patterns.CustomApps/lyrics-plus/OptionsMenu.js (3)
100-100: LGTM!Initializing
languageMapas an empty object is appropriate. The label resolution logic (lines 141-145) will gracefully fall back to other display methods until the async fetch completes.
138-151: Excellent fallback chain for label resolution.The three-tier fallback strategy is well implemented:
- Prefers
languageMap[code]from Musixmatch API (new feature)- Falls back to
Intl.DisplayNames.of(code)(browser native)- Final fallback to
code.toUpperCase()if all else failsThe try-catch block properly handles potential failures from the
IntlAPI. This ensures robust language name display across all scenarios.
234-243: LGTM!Adding
languageMapto the memoization dependencies ensures the translation options are recomputed when the language data is fetched, providing proper reactive behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
CustomApps/lyrics-plus/ProviderMusixmatch.js (3)
271-282: Consider adding defensive validation in the forEach loop.The code assumes each
itemhas alanguageproperty and that the language object is well-formed. If the API returns malformed data, accessingitem.languageor its properties could throw. While the outer try-catch will handle it, more granular validation would make debugging easier and prevent partial cache corruption.🔎 Suggested defensive validation
languageMap = {}; body.message.body.language_list.forEach((item) => { const lang = item.language; - if (lang.language_name) { + if (lang && lang.language_name) { const name = lang.language_name.charAt(0).toUpperCase() + lang.language_name.slice(1); if (lang.language_iso_code_1) languageMap[lang.language_iso_code_1] = name; if (lang.language_iso_code_3) languageMap[lang.language_iso_code_3] = name; } });
271-289: Operational consideration: Failed fetches will retry on every call.When the API fetch fails, the function returns an empty object but leaves
languageMapasnull. This means subsequent calls will retry the fetch, potentially hammering the API if it's experiencing issues. Consider caching the failure state temporarily to implement a backoff strategy, or accept the retry behavior as intentional for transient failures.
240-242: Consider returning a defensive copy instead of the cached reference.The function returns a direct reference to the module-level
languageMapobject. While currently safe since the only consumer (OptionsMenu.js) treats it as read-only, returning a shallow copy (return { ...languageMap }) would prevent accidental mutations from corrupting the cache and is a more robust defensive programming pattern.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
CustomApps/lyrics-plus/ProviderMusixmatch.js
🔇 Additional comments (4)
CustomApps/lyrics-plus/ProviderMusixmatch.js (4)
244-257: Excellent improvement over the previous cache validation approach.The version-based cache validation cleanly addresses the edge case limitations of the previous lowercase heuristic. The error handling with
console.warnalso improves debuggability compared to the silent catch block that was flagged in the previous review.
259-269: LGTM!The API URL construction follows the same pattern used consistently throughout the file for other Musixmatch endpoints.
292-292: LGTM!The new
getLanguagesfunction is correctly added to the module's public API alongside existing helpers.
278-278: This capitalization approach is appropriate for the Musixmatch API response format.The Musixmatch
languages.getAPI endpoint returns language names as lowercased single-word strings (e.g., "english"). ThecharAt(0).toUpperCase() + slice(1)logic correctly transforms these to title case, and handles the actual API response format as designed. Concerns about multi-word names, special rules, or pre-formatted input do not apply to this API's response structure.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
CustomApps/lyrics-plus/style.css
🔇 Additional comments (1)
CustomApps/lyrics-plus/style.css (1)
714-754: LGTM! Responsive spacing improvements align well.The progressive padding adjustments for split layouts across different breakpoints (base: 50px, 1024px: 75px, 1280px: 100px) are consistent with the existing margin patterns for lyrics lines and version selectors. This ensures a cohesive layout as the viewport expands.
1 participant
convert musixmatch translation from iso code to language name
Summary by CodeRabbit
New Features
Bug Fixes
Style
✏️ Tip: You can customize this high-level summary in your review settings.