feat(desktop): add 15-language i18n with hybrid JSON+TS architecture - #69819
feat(desktop): add 15-language i18n with hybrid JSON+TS architecture#69819iaendi wants to merge 3 commits into
Conversation
Clean split from NousResearch#38846 — Desktop-i18n-only files: - 15 locale JSON catalogs (ar, de, en, es, fr, hi, it, ja, ko, pt-BR, ru, th, vi, zh-CN, zh-Hant) - sync-locales.py bridge: JSON source → TypeScript defineLocale() - Generated TS locale files with full type annotations - i18n store (i18n.tsx, use-locale-sync.ts) - Locale key parity test Architecture: - JSON files in src/locales/ are the canonical translation source - sync-locales.py validates keys against en.json and generates src/i18n/*.ts - defineLocale() merges partial translations with English fallback - 249 en.json active keys, 122 keys per locale (remaining fall back to en) Supersedes: NousResearch#38846 (522 files → 35 files, i18n-only)
|
@iaendi — heads-up on a Russian overlap, so we don't land a regression by accident. This PR's Both PRs add the same file, so whichever lands second will conflict on it — and if this one lands first, the shipped Russian drops from complete to ~5%. Suggestion that keeps this PR's breadth and the RU depth:
Happy to do the mechanical part either way — I can open a patch against this branch that swaps in the complete |
|
Russian consolidation update: the complete, current-main RU Desktop candidate is now #72250. It is the rebased continuation of #66833 with original contributor authorship preserved, exact To avoid a Russian coverage regression, the cleanest path remains either:
That keeps this PR's multi-language architecture work independent while preserving the reviewed Russian depth. |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for narrowing the earlier i18n work to a Desktop-only change. The additional locales are still absent from current main, but this branch needs substantial adaptation before it can be evaluated as a merge candidate.
Problems
apps/desktop/src/locales/locales.test.ts:61requires complete key parity, but the PR’s ownen.jsonhas 250 keys whilede.jsonhas 126. That contradicts the documented partial-catalog fallback model and makes the test fail.apps/desktop/scripts/sync-locales.py:226-228records every missing key as an error, then exits nonzero at:310; partial catalogs therefore cannot sync successfully despite the fallback message at:227.apps/desktop/src/store/i18n.tsx:23-92creates localStorage-backed locale state alongside current main’s config-backedI18nProviderinapps/desktop/src/i18n/context.tsx:95-191. The current provider persistsdisplay.language, updates runtime translations, applies RTL, and rolls back failed saves.- The Russian overlap remains unresolved: this PR adds
src/i18n/ru.ts, while the discussion identifies #72250 as the complete current-main Russian candidate.
Suggested changes
- Port locale registration and catalogs onto the existing
apps/desktop/src/i18n/provider; do not introduce the parallel store. - Make partial-catalog fallback non-fatal and test that behavior, or provide full catalogs.
- Drop
ruor consolidate it with #72250 before salvage.
Automated hermes-sweeper review.
| const missingKeys = englishKeys.filter(k => !localeKeys.includes(k)) | ||
|
|
||
| // Only fail on missing keys, not extra self-reference keys | ||
| expect(missingKeys, `${locale}: missing keys`).toEqual([]) |
There was a problem hiding this comment.
This asserts full catalog parity, but the PR's en.json has 250 keys and de.json has 126. It will fail for the intentionally partial catalogs and conflicts with the stated English-fallback model. Please test fallback behavior instead, or require complete translations.
|
|
||
| if missing: | ||
| print(f' ⚠️ {locale["json"]}: {len(missing)} keys missing (will fallback to en)') | ||
| issues += len(missing) |
There was a problem hiding this comment.
Counting missing keys as errors makes every partial locale fail at the sys.exit(1) on line 310, despite the fallback claim immediately above. Missing override keys must be non-fatal if partial defineLocale() catalogs are supported.
| 'ru': 'Русский', | ||
| 'pt-BR': 'Português (Brasil)', | ||
| } | ||
|
|
There was a problem hiding this comment.
Current main already owns locale persistence in src/i18n/context.tsx through display.language, including runtime synchronization, RTL handling, and rollback on failed saves. Do not add a separate localStorage locale authority; port the new locales through that provider.
… parallel store
P1: sync-locales.py — missing keys are now informational (fallback to en),
exit 0 on partial catalogs instead of exit 1
P2: locales.test.ts — accept partial catalogs as valid; extra-key check
remains but missing keys are expected
P3: Remove store/i18n.tsx, store/use-locale-sync.ts, store/i18n.test.ts —
dead code not imported by any consumer; all store files correctly
use @/i18n (upstream I18nProvider)
P4: Drop Russian locale (ru.json, ru.ts, KNOWN_LOCALES entry) —
NousResearch#72250 is the complete Russian candidate; removing to avoid
5% coverage regression on merge
iaendi
left a comment
There was a problem hiding this comment.
Thanks for the thorough review @teknium1. All four issues have been addressed in 0deeefe:
P1 — sync-locales.py partial-catalog fallback: Missing keys are now informational only (ℹ️ instead of ⚠️), and the script exits 0 on partial catalogs. The existing fallback-to-English message path is preserved — defining fewer keys than en.json is expected and no longer treated as a validation error.
P2 — locales.test.ts partial-catalog test: The test now validates that non-English catalogs are subsets of the English catalog (no keys that en.json does not define), rather than requiring full key parity. Missing keys are expected and fall back to English at runtime.
P3 — Parallel i18n store removed: store/i18n.tsx, store/use-locale-sync.ts, and store/i18n.test.ts have been deleted. They were dead code — no consumer imported from them. All store files that need i18n correctly import from @/i18n (the upstream I18nProvider in apps/desktop/src/i18n/context.tsx).
P4 — Russian locale dropped: ru.json, ru.ts, and all Russian entries in sync-locales.py have been removed. The complete Russian candidate lives in #72250 and should land independently to avoid the ~5% coverage regression.
The branch now has 14 locales and is ready for re-evaluation.
|
@timoncool thanks for catching this. I have dropped This PR now carries 14 languages; Russian is intentionally left to the dedicated candidate. |
Summary
Clean i18n-only PR split from #38846 per alt-glitch's re-triage request.
35 files changed, Desktop-i18n-only scope.
Architecture
Languages (15)
en, zh-CN, zh-Hant, ja, ko, de, es, fr, pt-BR, ar, hi, th, vi, it, ru
Key design decisions
Verification
Closes #38846