fix: localize macOS menu role labels - #184
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 1 minutes and 48 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds localized labels for Electron role-backed menu items: new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Code Review
This pull request introduces localized labels for Electron menu roles in English and Chinese, ensuring a consistent user interface across different locales. It adds a new menuRoleLabel utility to handle translations with application name placeholders and updates the menu template to explicitly set labels for role-based items. A critical issue was identified in the 'Window' menu: removing the windowMenu role on macOS disables the OS-managed dynamic list of open windows, so the role should be retained alongside the custom label.
a8cf99c to
d446cbd
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Code Review Summary
P1 (required): 1 issue
P2 (suggested): 2 issues
P3 (optional): 2 issues
Overall the implementation is solid, but a few areas need attention before merge.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
d446cbd to
0df56eb
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Review: fix: localize macOS menu role labels
Thorough nitpick review. The approach is sound but a few issues worth addressing.
P1 (1 issue): windowMenu role + custom submenu contradicts Electron docs
P2 (3 issues): redundant English labels, loose test assertions, silent submenu miss
P3 (3 issues): localization accuracy, readability, locale-coverage gap
Key concern: the windowMenu block sets both role and submenu. Electron docs say role takes precedence and other properties are ignored, which would silently discard the custom submenu and defeat the localization goal. Needs either a code comment documenting the tested behavior, or a restructure to avoid the conflict.
0df56eb to
92abb1d
Compare
Summary
Localizes macOS desktop menu role-backed items in Chinese while keeping Electron roles attached for native behavior.
Why
Closes #177. PawWork custom menu labels were already localized, but role-only Electron menu items still rendered in English in Chinese locale.
Related Issue
Closes #177
How To Verify
Screenshots or Recordings
Not captured. This change is covered by menu template tests that assert Chinese labels and preserved Electron roles; a manual macOS menu check is still useful before release.
Checklist
dev, and my PR title and commit messages use Conventional Commits in EnglishSummary by CodeRabbit
New Features
Bug Fixes
Tests