Skip to content

fix: make locale switches atomic while lazy catalogs load - #2633

Open
Chris0Jeky wants to merge 2 commits into
mainfrom
issue-2003/pending-ui
Open

fix: make locale switches atomic while lazy catalogs load#2633
Chris0Jeky wants to merge 2 commits into
mainfrom
issue-2003/pending-ui

Conversation

@Chris0Jeky

@Chris0Jeky Chris0Jeky commented Sep 5, 2026

Copy link
Copy Markdown
Owner

Summary

  • Load a requested lazy locale catalog before committing the runtime locale and html.lang.
  • Expose pending and failed locale state with latest-request-wins protection for rapid choices.
  • Keep the prior language visible on failure and give the picker actionable, target-specific feedback.
  • Wait for persisted locale restoration before mounting so startup retains the no-flash guarantee.
  • Add catalog-parity copy and regression coverage for pending, failure, startup restore, and both race orderings.

Refs #2003

Scope

This PR implements only TD-2003-pending-ui. The service-worker preferred-locale warming contract and its browser/build proof remain separate follow-up work.

Verification

  • Red-first targeted assertions failed against the former flip-first implementation.
  • npx vitest --run --maxWorkers=2 src/tests/store/localeStore.failure.spec.ts src/tests/i18n/lazyLocales.spec.ts src/tests/views/AppearanceSettingsView.language.spec.ts src/tests/i18n/catalogs.spec.ts — 37 passed.
  • npm run test -- --maxWorkers=2 — 371 files passed; 5,728 passed, 3 skipped.
  • npm run typecheck — passed.
  • npm run lint — 0 errors, 8 existing warnings.
  • npm run build — passed; existing ineffective dynamic-import warning for toastStore.
  • git diff --check — passed.
  • Two coordinator review passes — no blocking findings.

Not verified

  • No real browser/Playwright, screen-reader, packaged-PWA, or installed-service-worker journey was run.
  • Hosted checks and automatic Codex review remain pending; the account's Codex review quota is currently exhausted.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 5, 2026

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

@Chris0Jeky Chris0Jeky mentioned this pull request Sep 5, 2026
Open
@Chris0Jeky
fix(i18n): bound the pre-mount locale restore wait and report a rejec…
7e9a64b 
…ted restore

Review round 1 (fresh-context, D-12 sweep): app.mount ran only in the finally of the locale
restore promise, so a lazy catalog request that stalled rather than failed held first paint
open indefinitely. Startup now races the restore against a 1500 ms budget and mounts in English
when the budget wins; the store still commits atomically if the catalog arrives later. A
rejected restore is logged through the existing error reporter instead of being discarded, the
garbage-stored-value test asserts preferredLocale (the validate-on-read guard it exists for),
and ADR-0054 section 7 records the commit-after-load semantics.
@Chris0Jeky

Chris0Jeky commented Sep 5, 2026

Copy link
Copy Markdown
Owner Author

Review gate (Codex review credits exhausted, SC-9; D-12 ruled 2026-09-05: the Codex session's PRs are reviewed and merged by the coordinator): one fresh-context read-only reviewer (Opus 5) on head 7e9a64b7b. Verdict: SHIP after round 1.

Round 1 reviewed head 6906c6a36: FIX-FIRST on one MEDIUM, fixed in 7e9a64b7b. The fix changes startup logic (a bounded race before mount), so a second read-only pass scoped to that diff follows below.

Checked by reading: both race orderings in localeStore.ts:93-162 (the module-level monotonic generation is incremented before every in-flight request and on the same-locale early return, so an obsolete resolution can neither commit nor clear the newer pending state); the failure path keeps locale, i18n.global.locale and <html lang> on the last committed value and names the failed target, while localStorage retains the preference by design; apply() cannot reject (the catalog load sits inside a .then() with .catch(() => false)), so mount was skipped only in the hang case; main.ts is the only apply() caller and AppearanceSettingsView.vue the only other consumer; loading and loadFailed exist in all three settings.ts catalogs with identical placeholders; no em dash in the six new strings; the four new store cases assert the opposite of the deleted flip-first assertions, so they fail on revert.

Findings and disposition:

  • MEDIUM, fixed: main.ts:36-56 mounted only in the .finally() of the restore promise, so a lazy catalog request that stalled rather than failed (a stale chunk hash after a deploy on a hanging connection) held first paint, telemetry and analytics open indefinitely. Startup now races the restore against a 1500 ms budget and mounts in English when the budget wins; the store still commits atomically if the catalog arrives later.
  • LOW, fixed alongside: the garbage-stored-value test asserted locale === 'en', which is hard-initialized and therefore trivially true; it now asserts preferredLocale, the validate-on-read guard it exists for.
  • LOW, fixed alongside: the .catch(() => undefined) swallowed a rejection; it now logs through the existing error reporter.
  • LOW, fixed alongside: ADR-0054 section 7 records the commit-after-load semantics. The STATUS line lands in the coordinator's next coordination subsection.
  • LOW, tracked on #2003: the role="status" live region is created by v-if together with its text, which screen readers commonly do not announce; render it always and toggle the text.

Issue disposition: slice of #2003 (TD-2003-pending-ui); the service-worker preferred-locale warming contract remains.

@Chris0Jeky

Chris0Jeky commented Sep 5, 2026

Copy link
Copy Markdown
Owner Author

Round 2 (scoped to the fix diff 6906c6a..7e9a64b only; one fresh-context read-only reviewer, Opus 5). Verdict: SHIP.

Checked by reading: Promise.race settles once and a single .finally is attached to it, so app.mount runs at most once; it cannot run zero times because the restore cannot reject (the new .catch returns void) and the budget always resolves, so the race always settles within 1500 ms; when the budget wins and the catalog arrives later, localeStore.ts:109-121 commits this.locale and applyLocale (i18n.global.locale plus html lang) in one synchronous body after setLocaleMessage has already registered the messages, so no raw keys are shown, only an English first paint that flips once; logError(context, ...args) exists at errorReporting.ts:78 and is the helper the installed handlers already use; preferredLocale is declared store state (localeStore.ts:71), so the spec change compiles and is the stronger assertion; no vitest spec imports src/main.ts (scripts/check-dev-entry-graph.mjs only transforms it; tests/run-vite-dev.spec.ts writes same-named fixtures), so the timer never runs under happy-dom today.

Findings and disposition (all LOW, tracked on #2003, none owed):

  • ADR-0054 section 7's trailing clause ("before mounting in English") can be read as the unconditional outcome; in the normal path the app mounts in the restored language. Wording only.
  • The startup budget has no test; main.ts is a side-effecting entry module with no harness, and adding a bootstrap() seam is a larger change than the fix.
  • The budget timer is never cleared when the restore wins; harmless in the browser.

Not verified by the reviewer: typecheck, build and the spec run (no shell); the hosted CI run on this head is the evidence for those.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

Taskdeck Execution
Status: Pending

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@Chris0Jeky