fix(i18n): resolve a deactivated language before the app renders - #1499
fix(i18n): resolve a deactivated language before the app renders#1499thomasbeaudry wants to merge 2 commits into
Conversation
gdevenyi
left a comment
There was a problem hiding this comment.
Solid diagnosis, and verified against libui source — the child-first effect ordering explains exactly why the sidebar and not the navbar strands, and reconciling in beforeLoad is the right place. Two items before merge:
apps/gateway/src/routers/root.router.ts:48-51still picks the language with its own inlineactiveLanguages.includes(...) ? ... : activeLanguages[0]— the same policy your newresolveActiveLanguagedoc comment calls "the one place that policy is decided". Please route it through the helper (requestedLanguage.success ? resolveActiveLanguage(requestedLanguage.data, activeLanguages) : activeLanguages[0]);assignment.activeLanguagesis already typedActiveLanguages, so it fits as-is, and the comment becomes accurate.reconcileInterfaceLanguagechanges the i18n singleton, but it lives inapps/web/src/utils/language.ts, andapps/web/AGENTS.mdreservessrc/utils/for pure helpers only — side-effect modules belong insrc/services/. Please move it intosrc/services/i18n.ts(and its tests tosrc/__tests__/), or amend that AGENTS.md line in the same commit so the docs and the code agree.
If you hand this to Claude Code, Opus 5 is the right size — the items are mechanical but span the gateway and web workspaces.
Reviewed at commit 4a0ce5c.
Deactivating the language a user is reading left the sidebar in it. The reconciliation added in #1442 runs from `useLanguageOptions`, inside `LanguageToggle`. It corrects `i18n.resolvedLanguage` and emits `languageChange` — but libui's `useTranslation` subscribes in an effect, and React runs effects child-first, so a tree that mounts already stranded fires the correction before the toggle's ancestors have subscribed. They never hear it and keep rendering the deactivated language. The sidebar is the casualty precisely because it renders the toggle. `Layout` mounts the navbar first, so the navbar subscribes in time and updates while the sidebar does not — which is why the symptom names one and not the other. Fixes it where the ordering cannot matter: `_app`'s `beforeLoad` reconciles before any component renders, so every `useTranslation` initialises from the corrected language instead of waiting to be told. The policy itself moves to `resolveActiveLanguage` in schemas/core, next to the authoring-language policy, so the pre-render pass and the live in-session effect decide the same thing rather than each carrying a copy. `useLanguageOptions` keeps its effect: it covers an admin deactivating a language mid-session, where every component is already subscribed and it demonstrably works. Its doc now says which case it can and cannot cover. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…de-effect to services Route gateway's language resolution through resolveActiveLanguage so the doc comment's "one place" claim is accurate. Move reconcileInterfaceLanguage from utils/ (pure helpers) to services/i18n.ts (side-effect singletons) per apps/web AGENTS.md. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
4a0ce5c to
d1560c0
Compare
joshunrau
left a comment
There was a problem hiding this comment.
The diagnosis holds up — I read libui's useTranslation and it does exactly what you describe:
useState(i18n.resolvedLanguage) at mount plus an effect subscription, so an ancestor that mounts
before the correction fires never hears it. Reconciling in _app's beforeLoad is the right place,
and pulling the policy into resolveActiveLanguage so the pre-render pass, the live effect and the
gateway all agree is a real improvement. Four things before this can merge:
-
pnpm lintpasses but rewrites two of your own files — run it and commit the result. It sorts
the@/services/i18nimport above@/storeinapps/web/src/routes/_app/route.tsx:9, and drops
the bareimport '@/services/i18n'on line 6 of
apps/web/src/__tests__/reconcile-interface-language.test.ts, which the named import on line 4
already covers. -
Nothing currently fails if the fix is removed. I restored
packages/react-core/src/hooks/useLanguageOptions.tsto the version onmainand all seven tests
inapps/web/src/__tests__/language-toggle.test.tsxstill passed — including the two new ones,
because by the time yourerender, the ancestor has already subscribed, which is the mid-session
path you say was working anyway. And nothing at all exercises
apps/web/src/routes/_app/route.tsx:22, so deleting that call leaves the whole suite green.
Please add a test that covers the wiring — drivingRoute.options.beforeLoadwith a stubbed
router context and assertingi18n.resolvedLanguagemoved would do it. -
In
testing/src/specs/admin-settings.spec.ts:68-90, the new block belongs in its owntest(...)
— the surrounding title is "should hide the language toggle once only one language is offered",
which is not what those lines assert. Also drop or reword thepage.reload()step at line 89:
libui'sTranslator.initcallschangeLanguage(defaultLanguage)on every load, so the language
is English after any reload and that assertion cannot fail, whereas the comment above it reads as
though it covers the fresh-mount case. -
reconcileInterfaceLanguagereturns a boolean that no production code reads —route.tsx
discards it, and the guard inside the function is already what prevents the redundant notify.
Either returnvoidor use the result at the call site.
If you hand this to Claude Code, Opus 5 is the right size — the work spans the web unit tests,
the Playwright spec and the helper's signature, and the test item is a design question rather than a
mechanical edit.
Reviewed at commit d1560c0.
The bug
Deactivating the language a user is reading leaves the sidebar in it.
Why
#1442 reconciles a stranded reader from
useLanguageOptions, which lives insideLanguageToggle. It setsi18n.resolvedLanguageand emitslanguageChange. But libui'suseTranslationsubscribes to that event in an effect (useTranslation.ts), and React runs effects child-first — so when a tree mounts already stranded, the correction fires before the toggle's ancestors have subscribed. They never hear it and keep rendering the deactivated language.Translator.changeLanguageemits only to handlers registered at that instant; a late subscriber gets nothing:This is why it is the sidebar and not the navbar.
Layoutmounts<Navbar />before<Sidebar />, and the sidebar is what renders the toggle. The navbar subscribes earlier and updates; the sidebar is the ancestor of the thing that fired the event, so its ownuseTranslation— including the one insideuseNavItems— registers too late.Reproduced directly: an ancestor rendering
LanguageTogglekeepsPanel de controlwhilei18n.resolvedLanguageis alreadyen.The fix
Reconcile where ordering cannot matter — in
_app'sbeforeLoad, before any component renders — so everyuseTranslationinitialises from the corrected language rather than having to be told afterwards.The policy moves to
resolveActiveLanguageinschemas/core, beside the existing authoring-language policy, so the pre-render pass and the live in-session effect decide the same thing instead of each carrying a copy.apps/webwraps it asreconcileInterfaceLanguage, which returns whether it moved anyone so a no-op does not notify every translated component on each route load.useLanguageOptionskeeps its effect. It covers an admin deactivating a language mid-session, where everything is already subscribed and it demonstrably works — I verified that path in a browser, with and without this change. Its doc now states which case it covers and which it cannot, rather than implying both.apps/gatewayneeds nothing:root.router.tsalready picks the language server-side from the same active set, so it cannot mount stranded.What I could and could not verify
Honest about the limits, because they affect how much this PR proves:
So the new e2e is a regression guard for the flow, not a demonstration of the fix — it passes either way. The unit tests are what fail without the reconciliation. Flagging this rather than implying the e2e proves more than it does.
Worth noting separately: the interface language does not survive a reload at all — nothing persists
resolvedLanguage. That is arguably its own bug, and fixing it would make this latent ordering hazard fire on every load, which is the main reason to land this first.Test plan
pnpm lint— 33/33 workspaces cleanpnpm test— 662 passed, 1 skipped (9 new)pnpm test:e2e— 142 passed; a later run flaked onuploadandstart-sessionunder firefox plus a teardown 429 from the login throttler, and all 22 pass in isolationLocal e2e needed a temporary port move: 5500/3500 were held by another process.
.envwas restored byte-for-byte afterwards.🤖 Generated with Claude Code