feat(i18n): auto-discover valid locales and harden language menu#425
feat(i18n): auto-discover valid locales and harden language menu#425imAaryash wants to merge 1 commit intosiddharthvaddem:mainfrom
Conversation
- discover locales from folder structure and validate required namespaces - keep incomplete locales out of selectors and system-language suggestion - auto-discover locale folders in i18n-check - fix launch language menu interaction, clipping, and scrolling
📝 WalkthroughWalkthroughThis PR transitions the app from static locale hardcoding to dynamic runtime locale discovery. It adds system language detection that prompts users to switch to their system locale on first visit, replaces the language select with an interactive menu UI, and validates locale completeness based on namespace availability. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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. 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 |
|
heyy @siddharthvaddem created a new PR for this #362 the older one got polluted |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2a15e5fd8e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/contexts/I18nContext.tsx (1)
101-111:⚠️ Potential issue | 🟡 MinorGuard
setLocalebefore persisting.Lowkey risky as-is:
Localeis juststring, so any caller can push an unsupported locale through here. The real danger is something likeVideoEditor.tsx:1707castinge.target.value as Localewithout validation—that goes straight to localStorage,document.documentElement.lang, and the Electron API.You already have
isSupportedLocale()as a type guard in this file. A quick check before persisting would keep the provider honest and match the pattern you're using elsewhere.nit: cleaner fix
const setLocale = useCallback((newLocale: Locale) => { + if (!isSupportedLocale(newLocale)) return; setLocaleState(newLocale); try { localStorage.setItem(LOCALE_STORAGE_KEY, newLocale);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/contexts/I18nContext.tsx` around lines 101 - 111, Validate the incoming locale with the existing isSupportedLocale() before applying side effects: in setLocale, call isSupportedLocale(newLocale) and only then call setLocaleState(newLocale), persist to localStorage (LOCALE_STORAGE_KEY), set document.documentElement.lang, and invoke window.electronAPI?.setLocale?.(newLocale); if the check fails, no-op (or optionally log/return) to avoid persisting/propagating unsupported strings. Ensure you reference the setLocale function and use the isSupportedLocale type guard already defined in this module.
🧹 Nitpick comments (2)
src/i18n/config.ts (1)
12-12: Consider a small locale-normalization helper at the boundary.
Locale = stringmakes sense for dynamic discovery, but it also makes invalid locale values easier to pass around. a tinynormalizeLocale/isAvailableLocalegate in the context boundary would keep this safer and cleaner.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/i18n/config.ts` at line 12, Replace the open-ended Locale = string usage by adding a small normalization/validation layer at the i18n boundary: implement a normalizeLocale(locale: string): Locale function that canonicalizes values (e.g., lower-case, underscore→dash, strip whitespace) and an isAvailableLocale(locale: string): boolean that checks against the discovered/allowed locales list; then update places that accept raw locale strings to call normalizeLocale and gate with isAvailableLocale (e.g., where the context is created or getLocaleFromRequest is used) so only normalized, validated Locale values propagate through the app.src/components/launch/LaunchWindow.tsx (1)
628-660:role="menu"withrole="menuitemradio"is basically making promises you're not keeping here.The semantics announce a full keyboard-accessible menu—arrow keys for navigation, Enter to select, Escape to close, focus movement into the menu and back out—but looking at the implementation, there's none of that. No arrow-key handlers, no focus trapping on open, no focus return to the trigger on close. Users who rely on keyboard nav are kinda out of luck here.
You've got two clean paths forward:
Add proper roving focus – Move focus into the menu when it opens (to first item), handle ArrowUp/ArrowDown/Home/End, trap focus while menu is open, return to the trigger on close. This is more work but the menu pattern would then be honest.
Switch to
role="radiogroup"orrole="listbox"– These patterns don't expect the full menu keyboard model, just arrow keys + Enter/Space, which is closer to what you're already doing with the click-to-select buttons.Pick one or the other; the current state is lowkey risky accessibility-wise since assistive tech users will trust the roles and get disappointed by missing interactions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/launch/LaunchWindow.tsx` around lines 628 - 660, The current menu uses role="menu" and role="menuitemradio" but lacks the required keyboard/focus behavior; either implement full roving-focus/menu keyboard handling or change the ARIA pattern to match current behavior. Option A (preferred quick fix): replace role="menu" with role="radiogroup" (or role="listbox") on the container (languageMenuPanelRef) and keep role="menuitemradio" on each button/option, ensure ArrowUp/ArrowDown and Enter/Space select the option by wiring keydown handlers on languageMenuPanelRef that call setLocale(loc) and resolveSystemLocaleSuggestion(), and ensure focus is set to the selected item when the panel opens (use isLanguageMenuOpen effect). Option B (full accessibility): keep role="menu" and implement roving focus and focus trapping when isLanguageMenuOpen is true (move focus into first/selected item, handle ArrowUp/ArrowDown/Home/End/Escape to navigate/close, and return focus to the trigger when closed). Use the existing identifiers availableLocales, setLocale, resolveSystemLocaleSuggestion, setIsLanguageMenuOpen, languageMenuPanelRef, and getLocaleName to locate and update the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/i18n/loader.ts`:
- Around line 70-79: The exported getters expose internal arrays directly
allowing callers to mutate module state; update getAvailableLocales and
getLocaleValidationErrors to return defensive copies instead of the original
arrays (e.g., return [DEFAULT_LOCALE] when empty, otherwise return a shallow
copy of availableLocales) and return a shallow copy of localeValidationErrors so
callers cannot push/splice into the module's internal arrays.
---
Outside diff comments:
In `@src/contexts/I18nContext.tsx`:
- Around line 101-111: Validate the incoming locale with the existing
isSupportedLocale() before applying side effects: in setLocale, call
isSupportedLocale(newLocale) and only then call setLocaleState(newLocale),
persist to localStorage (LOCALE_STORAGE_KEY), set document.documentElement.lang,
and invoke window.electronAPI?.setLocale?.(newLocale); if the check fails, no-op
(or optionally log/return) to avoid persisting/propagating unsupported strings.
Ensure you reference the setLocale function and use the isSupportedLocale type
guard already defined in this module.
---
Nitpick comments:
In `@src/components/launch/LaunchWindow.tsx`:
- Around line 628-660: The current menu uses role="menu" and
role="menuitemradio" but lacks the required keyboard/focus behavior; either
implement full roving-focus/menu keyboard handling or change the ARIA pattern to
match current behavior. Option A (preferred quick fix): replace role="menu" with
role="radiogroup" (or role="listbox") on the container (languageMenuPanelRef)
and keep role="menuitemradio" on each button/option, ensure ArrowUp/ArrowDown
and Enter/Space select the option by wiring keydown handlers on
languageMenuPanelRef that call setLocale(loc) and
resolveSystemLocaleSuggestion(), and ensure focus is set to the selected item
when the panel opens (use isLanguageMenuOpen effect). Option B (full
accessibility): keep role="menu" and implement roving focus and focus trapping
when isLanguageMenuOpen is true (move focus into first/selected item, handle
ArrowUp/ArrowDown/Home/End/Escape to navigate/close, and return focus to the
trigger when closed). Use the existing identifiers availableLocales, setLocale,
resolveSystemLocaleSuggestion, setIsLanguageMenuOpen, languageMenuPanelRef, and
getLocaleName to locate and update the code.
In `@src/i18n/config.ts`:
- Line 12: Replace the open-ended Locale = string usage by adding a small
normalization/validation layer at the i18n boundary: implement a
normalizeLocale(locale: string): Locale function that canonicalizes values
(e.g., lower-case, underscore→dash, strip whitespace) and an
isAvailableLocale(locale: string): boolean that checks against the
discovered/allowed locales list; then update places that accept raw locale
strings to call normalizeLocale and gate with isAvailableLocale (e.g., where the
context is created or getLocaleFromRequest is used) so only normalized,
validated Locale values propagate through the app.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b0672418-96f0-4213-8920-e5c08470f9e6
📒 Files selected for processing (12)
scripts/i18n-check.mjssrc/components/launch/LaunchWindow.module.csssrc/components/launch/LaunchWindow.tsxsrc/components/ui/dropdown-menu.tsxsrc/components/ui/select.tsxsrc/components/video-editor/VideoEditor.tsxsrc/contexts/I18nContext.tsxsrc/i18n/config.tssrc/i18n/loader.tssrc/i18n/locales/en/launch.jsonsrc/i18n/locales/es/launch.jsonsrc/i18n/locales/zh-CN/launch.json
|
Hey @siddharthvaddem, also a small suggestion you should create a GitHub organization for OpenScreen and transfer the repo to it. It'll give the project a more professional presence, help it reach more people, and let you assign maintainers. I'd love to contribute as a maintainer if you're open to it! 😊 |
1 participant
Description
This PR improves language onboarding and language switching in the Launch/HUD flow, with a focus on reliability in Electron.
It includes:
en,es, andzh-CNMotivation
The language selector and onboarding flow had interaction/clipping issues and required manual locale list maintenance.
This update improves usability and maintainability by:
Type of Change
Related Issue(s)
Fixes #356
Partially addresses #363
Summary by CodeRabbit