-
-
Notifications
You must be signed in to change notification settings - Fork 816
fix(lyrics-plus): fix issue with non-standard translations from Musixmatch providers #3596
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughTranslationMenu in Changes
Sequence Diagram(s)sequenceDiagram
participant UI as TranslationMenu
participant Lib as musixmatchDisplay
rect rgba(76,175,80,0.08)
UI->>Lib: call musixmatchDisplay.of(code)
alt returns value (non-nullish)
Lib-->>UI: label
UI->>UI: use label
else returns null/undefined
Lib-->>UI: null/undefined
UI->>UI: use code.toUpperCase()
else throws
Lib--x UI: throws error
UI->>UI: catch error
UI->>UI: use code.toUpperCase()
end
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
CustomApps/lyrics-plus/OptionsMenu.js (2)
130-130: Optional: Remove unnecessary empty string initialization.The
labelvariable is assigned in both the try and catch blocks, so the empty string initialization is redundant.Apply this diff:
- let label = ""; + let label; try { label = musixmatchDisplay.of(code); } catch (e) { label = code.toUpperCase(); }
133-135: Optional: Consider logging the exception for debugging.While silently falling back is correct for UX, logging the error could help identify problematic language codes during development.
Apply this diff:
} catch (e) { + console.warn(`[Lyrics Plus] Invalid language code for Musixmatch translation: ${code}`, e); label = code.toUpperCase(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
CustomApps/lyrics-plus/OptionsMenu.js(1 hunks)
🔇 Additional comments (1)
CustomApps/lyrics-plus/OptionsMenu.js (1)
130-135: I'll help you verify the review comment. Let me start by checking the Intl.DisplayNames API behavior and then examine the actual code context.
<function_calls>
<function_calls>
Intl.DisplayNames.of() throws exception invalid language code RangeError spec
</function_calls>Now let me examine the actual code in the file:
<function_calls>
find . -name "OptionsMenu.js" -path "lyrics-plus" 2>/dev/null | head -5
</function_calls>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CustomApps/lyrics-plus/OptionsMenu.js (1)
130-135: Good fix for the crash issue!The try-catch block correctly prevents the crash when
musixmatchDisplay.of(code)throws aRangeErrorfor non-standard language codes. The fallback tocode.toUpperCase()is appropriate.Consider logging the exception for debugging purposes to help identify problematic language codes in the future:
let label = ""; try { label = musixmatchDisplay.of(code) || code.toUpperCase(); } catch (e) { + console.warn(`Failed to get display name for language code "${code}":`, e); label = code.toUpperCase(); }This would make it easier to track down issues with invalid or non-standard language codes from Musixmatch providers.
|
fix issue #3592. |
In the case of the non-standard translation of the Musixmatch provider, the
DisplayNames.of()function would return an exception instead ofnullorundefined, causing Spotify to crash.Therefore, we modified this to handle exceptions using a try-catch statement.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.