feat: add i18n support#96
Conversation
emreertunc
left a comment
There was a problem hiding this comment.
Thanks for the PR — solid work overall. Build (pnpm build incl. check-i18n), cargo test (21/21), locale key parity (253/253 incl. placeholders), settings backward compatibility (#[serde(default)]), and command registration (both lib.rs and web.rs) all check out. A few issues before I can approve:
Blocking
- "system" mode can resolve to different languages in the UI vs. native menus. The frontend resolves from navigator.languages (src/i18n/index.ts) while the backend resolves from sys_locale (src-tauri/src/i18n.rs), with the zh-CN/zh-SG/zh-Hans matching logic duplicated in both. When the two inputs disagree (e.g. WebView language list ["zh-TW", "zh-CN"], or macOS where WKWebView's navigator.language may not match the OS locale), the window renders in one language and the menus/tray in another. Suggestion: make the backend's resolved_code the single source of truth and have the frontend consume it (e.g. return it from get_app_language / include it in the language-changed payload), and drop the TS matcher.
- The English label for the system-language option is the lowercase literal "system" (locales/en-US/ui.json language.system, locales/en-US/native.json systemLanguage). It reads as an untranslated placeholder in both the dropdown and the native Settings > Language menu; zh-CN correctly says 跟随系统. Should be e.g. "System Default".
Minor (worth fixing here, but not deal-breakers)
- In AccountUsageStats.tsx, t was added to loadStats's dependency array, so every language switch re-fetches usage stats from the ChatGPT backend for each open panel. Previously fetches only happened on account/panel changes.
- Numbers lost thousands separators: formatNumber (Intl) was replaced with raw i18next interpolation for stats.runs / stats.days - 12345 now renders as "12345 run" instead of "12,345 runs".
- src/main.tsx / src/tray-main.tsx gate the first render on initializeI18n() (an IPC round-trip); if it hangs or i18next init rejects, the window stays blank (void renderApp() swallows the error). Since resources are bundled, consider rendering immediately and applying the language when it resolves.
Optional / follow-up
- tray.rs build_menu reads the settings file 2–3× per rebuild (which happens every 60s), and in "system" mode each t() call triggers a sys_locale OS query (~30 per menu build). Resolve the language once per rebuild and pass it down.
- Some TrayMenu.tsx callbacks capture t without listing it in deps → stale-language error text after a switch.
- Duplication that could be consolidated later: three near-identical reset/duration formatters (TrayMenu, UsageBar, AccountUsageStats), the "chatgpt_backend" sentinel hardcoded in 5 places, and dead English default dialog titles in platform.ts (all callers pass t() now).
|
Thanks for the detailed review. I've rebased onto the latest
|
emreertunc
left a comment
There was a problem hiding this comment.
Thanks, the original review items are addressed correctly. I found one remaining startup race introduced by rendering before async language initialization.
applyLanguage() only notifies listeners that are already registered, while App subscribes to the language preference in a passive useEffect. If the backend response arrives after the initial render but before that effect runs, the translations switch correctly but the language selector keeps its initial system value, even when the saved preference is an explicit language. I reproduced this timing in a production-mode React check.
Please make the subscription provide the current snapshot immediately, or use useSyncExternalStore for the language preference.
Summary
Implementation
i18nextandreact-i18nextto manage frontend translationslocalesand register them throughmanifest.jsonsystemmode that follows the operating system localecheck-i18n.mjsto validate locale manifests, resource files, and translation key structure during buildsTesting
pnpm buildcargo test