feat(i18n): add app language switching and Chinese docs#600
feat(i18n): add app language switching and Chinese docs#600guozhijian611 wants to merge 2 commits intoDimillian:mainfrom
Conversation
xkonjin
left a comment
There was a problem hiding this comment.
Nice direction overall, but I think this needs a bit more regression coverage before it lands.
The change touches a huge number of UI surfaces plus the Rust settings model, but the new tests only exercise the Display settings section. I don’t see a smoke test that proves the end-to-end wiring actually works when appLanguage is set to zh-CN, especially for the two new provider entry points (MainApp and the standalone AboutView).
The failure mode I’m worried about is subtle: a serialization mismatch, a missed provider boundary, or an untranslated portal/render path would still let the settings tests pass while the app ships partially untranslated. I’d strongly suggest adding at least one integration-style test that seeds appLanguage: "zh-CN" and asserts a non-settings surface renders translated text, plus one check for the standalone About window since it now bootstraps i18n separately.
xkonjin
left a comment
There was a problem hiding this comment.
Code Review: i18n Language Switching
Summary
Large, well-structured i18n migration. The dual API (t for named keys, tx for source strings) is pragmatic for incremental adoption.
Issues
1. Test coverage is too thin for the diff size
Only 30 lines of new tests (i18n.test.ts) for a ~12k line change. At minimum, add component-level tests verifying that:
I18nProvidercorrectly setsdocument.documentElement.lang- A sample component renders Chinese text when language is
zh-CN normalizeLanguagePreferencehandles edge cases (empty string, undefined, unexpected values)
2. Double-translation anti-pattern
SidebarBottomRail.tsx computes accountActionLabel = accountEmail ? tx("Switch account") : tx("Sign in"), then later renders {tx(accountActionLabel)}. This passes an already-translated string back into tx(). In zh-CN it happens to fall through to the fallback, but this is fragile. Store the raw key and translate only at render time.
3. Missing translation coverage check in CI
The coverage script in docs/i18n.md is useful but not wired to CI or pre-commit. New tx() calls will silently fall back to English for zh-CN users. Consider adding it to npm run test or a lint step.
4. Potential perf note
zhCNAdditionalTextTranslations is a massive eagerly-loaded object. For a desktop Tauri app this is acceptable, but monitor memory if more languages are added.
Security
No issues. String interpolation only replaces {key} with app state values.
Verdict
Approachable with tests added. The double-translation pattern should be cleaned up before merge.
xkonjin
left a comment
There was a problem hiding this comment.
Review: i18n language switching
Bugs / correctness
-
Double-translation in
SidebarBottomRail.tsx—accountActionLabelis already a translated string (computed viatx("Switch account")ortx("Sign in")), but it is later passed back intotx()at render:{tx(accountActionLabel)}. This creates a double-translation hazard: ifaccountActionLabelever resolves to a string that happens to match a different translation key, the UI will show the wrong text. Store the raw key instead and translate once at render time. -
AboutViewasync language load flash —AboutViewinitializeslanguagestate to"system", then fetches settings asynchronously. During the gap,I18nProviderrenders with"system"and then re-renders with the fetched value. If the user has a non-system preference, this causes a language flash on every About window open. Consider reading the language synchronously from Tauri storage or accepting it as a prop from the parent window. -
MainApppasses rawappSettings.appLanguagewithout normalizing —appLanguagecould theoretically be an unexpected value (empty string, malformed locale, or future language not yet in the provider).I18nProvidershould defend against this, but it would be safer to pipe it throughnormalizeLanguagePreferencebefore passing it down.
Test coverage gaps
- Only one new test (
SettingsDisplaySection.test.tsx+ 30 lines ini18n.test.ts) covers a 12k-line diff. At minimum, add:- A test proving
I18nProvidercorrectly setsdocument.documentElement.langwhen language changes. - A component-level test rendering a non-settings surface (e.g.,
SidebarBottomRail) withappLanguage: "zh-CN"and asserting a translated label appears. - Edge-case tests for
normalizeLanguagePreference(empty string,undefined, unexpected locale code).
- A test proving
Security
- No issues. String interpolation only replaces
{key}with known app-state values.
Verdict: approachable with the double-translation fix and broader test coverage added.
xkonjin
left a comment
There was a problem hiding this comment.
Review: i18n / language switching (PR #600)
Overall this is a solid, well-structured i18n foundation. The separation between t(key) for stable namespaced keys and tx(source) for inline source-string translation is pragmatic.
Bugs / Security
-
XSS risk in interpolated translations — tx("Version {version}", { version }) and similar calls interpolate raw strings into UI text. If version (or any other interpolated value) ever comes from untrusted input, this becomes an injection vector. The PR introduces ~400 tx() calls; most are static labels, but anywhere user-generated content (workspace names, thread titles, branch names, commit messages) is interpolated, it needs HTML escaping. Recommend auditing all {placeholder} usages and either:
- Escaping interpolated values in the tx implementation before returning, OR
- Documenting that tx returns raw strings and callers must escape.
-
workspaceDropText passed to tx() in Sidebar.tsx — workspaceDropText is a dynamic string ("Drop Project Here" / etc.). If this ever contains user input, it bypasses the intended translation mapping and could leak into the dictionary. The current code does tx(workspaceDropText) which means the entire raw string becomes a lookup key. This is fine for hardcoded values but risky if the source becomes dynamic.
-
Missing tx tests for edge cases — No tests visible for:
- Missing key fallback behavior
- Interpolation with missing placeholders
- HTML/JSX injection through placeholder values
- Locale resolution when system preference is set but system locale is unsupported
Test Coverage Gaps
-
I18nProvider SSR / hydration — I18nProvider sets document.documentElement.lang in a useEffect. In SSR or pre-rendered contexts, this causes a hydration mismatch because the server-rendered HTML won't have the correct lang attribute. Consider setting it synchronously during render or accepting an initial lang prop.
-
Rust app_language deserialization — The new app_language field in AppSettings has a default_app_language() of "system". If an older settings JSON is loaded and the field is missing, it defaults correctly, but there's no test for backward compatibility (loading settings serialized before this field existed).
-
No tests for normalizeLanguagePreference / resolveLocale — These are the core locale resolution functions. Edge cases like:
- system locale = zh-HK (should it fall back to zh-CN or en?)
- Invalid stored preference (e.g., user manually edits settings.json to "Klingon")
- Rapid language switching causing re-render thrash
Minor
-
README.zh-CN.md — The Chinese translation of the README is thorough, but the "iOS Support" section says "iOS support is still in progress" in Chinese while the English README was updated in a previous commit. Consider whether the two READMEs should be kept in sync mechanically rather than manually.
-
Translation dictionary size — zhCNTranslations and zhCNTextTranslations are large inline objects. At scale this will bloat the main bundle. Consider lazy-loading locale data (e.g., dynamic import), especially as more languages are added.
Verdict
Conditional approval once XSS escaping strategy for interpolated values is addressed and at least basic unit tests for tx() / I18nProvider are added. The structural approach is sound.
Summary
system,en, andzh-CNoptions.Validation
npm run typechecknpm run testcd src-tauri && cargo checkNote:
cargo checkonly reports existing dead-code warnings.