feat(lyrics-plus): convert musixmatch translation name from iso to language name #3672
Conversation
📝 WalkthroughWalkthroughAdds a cached Musixmatch language fetcher (getLanguages) and integrates its results into the lyrics-plus translation menu so language labels are resolved from Musixmatch names instead of raw ISO codes. Changes
Sequence DiagramsequenceDiagram
participant UI as TranslationMenu Component
participant Provider as ProviderMusixmatch
participant Cache as localStorage
participant API as Musixmatch API
UI->>UI: useEffect on mount
UI->>Provider: getLanguages()
Provider->>Cache: check cached map & version
alt cache hit & valid
Cache-->>Provider: return cached map (ISO→name)
else cache miss/stale
Provider->>API: fetch languages list
API-->>Provider: language payload
Provider->>Provider: build ISO(1/3)→name mapping
Provider->>Cache: store mapping with __version
end
Provider-->>UI: languageMap
UI->>UI: setState(languageMap) and re-render with labels
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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.
Co-authored-by: ririxi <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@CustomApps/lyrics-plus/OptionsMenu.js`:
- Around line 100-106: The effect using react.useEffect that calls
ProviderMusixmatch.getLanguages should first check that ProviderMusixmatch &&
typeof ProviderMusixmatch.getLanguages === "function", call it from an async
function (or IIFE), catch and handle any rejection, and avoid setting
languageMap after unmount by using a cancellation flag (e.g., let cancelled =
false; setLanguageMap only if !cancelled) or AbortController and cleaning up in
the effect's return; update the effect around languageMap/setLanguageMap and
ProviderMusixmatch.getLanguages to implement these guards and error handling.
2 participants
convert musixmatch translation from iso code to language name
Summary by CodeRabbit
New Features
Performance
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.