Skip to content

test(i18n): guard against locale keys English has dropped - #71233

Open
joelbrilliant wants to merge 1 commit into
NousResearch:mainfrom
joelbrilliant:fix/i18n-locale-parity-guard
Open

test(i18n): guard against locale keys English has dropped#71233
joelbrilliant wants to merge 1 commit into
NousResearch:mainfrom
joelbrilliant:fix/i18n-locale-parity-guard

Conversation

@joelbrilliant

Copy link
Copy Markdown
Contributor

The gap

defineLocale() deep-merges a locale's overrides onto en and makes every key optional, so a partially translated locale falls back to English. That's deliberate and good.

What it hides is the opposite case: a locale key English no longer has. The stale override survives the merge and wins over the English value.

TypeScript can't catch it. Blocks like composer.commandDescs are typed Record<string, string>, so any key satisfies the type. Verified on this repo — re-adding a removed key to a locale leaves tsc --noEmit exiting 0.

Why it's worth a guard

This isn't hypothetical. #66646 removed terminal-only commands (/clear, /details, /copy, /quit) from the desktop quick-help drawer because the desktop refuses them. A locale that still defined those keys kept advertising commands that don't work — the exact bug that change fixed, staying live in one language while every other locale was corrected. Nothing failed.

Any long-lived branch touching i18n has this landmine, because a locale added after the branch was cut inherits English silently.

What it checks

Fails when a locale defines a key English doesn't have. Missing keys are not flagged — falling back is the design.

Two exemptions, both documented in the file, because the guard genuinely can't protect them:

  • Blocks English leaves deliberately emptymessaging.platformIntro is {}, an extension point locales populate.
  • Runtime contribution registriessidebar.nav is SIDEBAR_NAV_AREA in app/routes.ts; plugins register the ids, so extra locale labels there aren't evidence of a removal.

I kept that list to two and named the reason for each, since every entry is somewhere the guard stops working.

What it found

Running it on current main surfaced two genuinely dead keys in ar.ts, both removed here after checking the app for any remaining reference:

key why it's dead
keybinds.actions'view.closePreviewTab' no such action exists anywhere in the app
onboarding.flowSubtitles'loopback' desktop handles pkce, device_code, external only

Both would render to Arabic users as descriptions of things that don't exist.

Tests

The walker itself is unit-tested alongside the parity check: reports extra keys, reports nested keys with a dotted trail, does not report missing keys, skips empty blocks, skips registry paths, and ignores functions/arrays.

39 passed in src/i18n, tsc clean, lint clean.

@alt-glitch alt-glitch added type/test Test coverage or test infrastructure P3 Low — cosmetic, nice to have comp/desktop Electron desktop app (apps/desktop/*) area/i18n Localization, locales, translations labels Jul 25, 2026
defineLocale() deep-merges a locale's overrides onto en and makes every key
optional, so a partially translated locale falls back to English. That is
deliberate. What it hides is the opposite case: a locale key English NO
LONGER HAS. The stale override survives the merge and wins over English.

TypeScript cannot catch it. Blocks like composer.commandDescs are typed
Record<string, string>, so any key satisfies the type - verified by
re-adding a removed key and watching tsc --noEmit still exit 0.

That is not cosmetic. NousResearch#66646 removed terminal-only commands (/clear,
/details, /copy, /quit) from the desktop quick-help drawer because the
desktop refuses them, but a locale still defining those keys kept
advertising commands that do not work - the very bug the change fixed,
staying live in one language while every other locale was corrected.

The guard fails on exactly that: a key a locale defines that English does
not have. Missing keys are NOT flagged, since falling back is the design.

Two exemptions, both documented, because the guard cannot protect them:
- blocks English leaves deliberately empty (messaging.platformIntro is {})
- runtime contribution registries (sidebar.nav, SIDEBAR_NAV_AREA in
  app/routes.ts), where plugins register the ids

Running it on current main found two genuinely dead keys in ar.ts, both
removed here after checking the app for any remaining reference:
- keybinds.actions 'view.closePreviewTab' - no such action exists
- onboarding.flowSubtitles 'loopback' - desktop handles pkce, device_code
  and external only

39 passed in src/i18n, tsc clean, lint clean.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@joelbrilliant
joelbrilliant force-pushed the fix/i18n-locale-parity-guard branch from c5ca399 to 316007f Compare July 29, 2026 01:05
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for adding a behavioral invariant rather than a locale snapshot. Current main still has both stale Arabic overrides: view.closePreviewTab at apps/desktop/src/i18n/ar.ts:236 and loopback at apps/desktop/src/i18n/ar.ts:1963; English has no corresponding keys (apps/desktop/src/i18n/en.ts:2313-2317). defineLocale() retains arbitrary override keys during its merge (apps/desktop/src/i18n/define-locale.ts:25-40), so the guard addresses a real current defect. The PR is currently CLEAN and MERGEABLE according to gh pr view 71233.

Automated hermes-sweeper review.

Copy link
Copy Markdown
Contributor Author

Thanks for checking the stale-key premise and the behavioural guard against current main. The current head is 316007f63, the PR is clean and mergeable, and all required checks are green.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/i18n Localization, locales, translations comp/desktop Electron desktop app (apps/desktop/*) P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades type/test Test coverage or test infrastructure

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants