Repository navigation
refactor(i18n): migrate UI catalogs to the resolver-based message format - #5921
Conversation
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed f9428cb336205b703140f03823b594252f91188c (73 files, +927/−942, a single commit).
P2 — the test job fails on this head at the renderer architecture check, and the migration itself is what trips it.
Run 36976556208, step check:architecture: copy catalog validation failed: missing UiCatalog marker from @maka/core/ui-locale, reported across the migrated catalog files (shell-copy.ts, shell-remaining-copy.ts, settings-*-copy.ts, task-readiness-copy.ts, work-board-error-copy.ts, storage-usage-copy.ts, and more), together with dependencyPaths changed; expected {}, received {"@maka/core/ui-locale":1} for the same files. The migration replaces satisfies UiCatalog<T> with the new form, so the marker that the copy-catalog validation looks for is gone, and the committed architecture ledger no longer matches what the files import. The new form therefore needs to be either recognised by that check or accompanied by a regenerated ledger — a convention question worth settling on the PR rather than by hand-editing the ledger.
Some entries in the same failure are not attributable to this commit: src/renderer/platform/desktop/onboarding-snapshot-bridge.ts ("Desktop adapter imports unbudgeted renderer legacy code") and use-app-shell-session-list.ts (import counts) do not appear in this commit's diff, so they come from base-branch drift relative to the check's --base. Those should clear once the branch merges the current base.
On the three risks this review was asked about, the migration holds up.
- Key loss: mitigated by construction. Each migrated catalog keeps the same type argument:
satisfies UiCatalog<Record<GeneralizedErrorClass, string>>becomesdefineUiMessageCatalog<Record<GeneralizedErrorClass, string>>()({ … })(andresolveUiMessageCatalog(...)around it where the call site needs a resolved catalog). The interface that forced every locale to carry every key is still applied per locale, so a missing key remains a compile error rather than a runtime hole. - Placeholder and interpolation semantics: I checked the diff for changed message values rather than wrapper lines, and the message strings appear only as re-indentations of identical content, in all three locales. The type arguments are unchanged too, so a renamed or re-ordered placeholder that changed arity would have to show up in those types.
- The resolver itself is not modified here —
packages/core/src/ui-localeis untouched, so the resolution mechanism the catalogs migrate to is the existing one.
One change worth naming explicitly, because it is security-adjacent in a copy refactor: packages/core/src/redaction.ts is touched, but only its exported GENERALIZED_ERROR_COPY constant is re-wrapped into the new form with the identical strings and the identical type argument; the redaction logic itself is unchanged.
Gate on this head: test is red (the architecture step above); label is green.
What I could not judge
- I did not run the architecture checker or the workspace suites myself; the above is read from the failing job's per-file output plus the commit's diff.
- No renderer or Electron run, so the resolved output of the new form is taken from the type arguments and unchanged message data rather than observed.
I did not approve, request changes, or merge.
Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed 476cbe246acae79da589f48f1f53d62c1ac0c179. The delta since my previous review is a merge of main plus a large re-baseline of apps/desktop/renderer-architecture.json (164 lines).
The specific failure I reported is resolved; the test job is nevertheless red on this head for something else — 1×P2.
What I can verify directly: the previous run failed at check:architecture with copy catalog validation failed: missing UiCatalog marker from @maka/core/ui-locale plus dependencyPaths changed; expected {}, received {"@maka/core/ui-locale":1} across the migrated catalogs. Neither string appears in this head's failed log, the architecture ledger has been re-baselined, and the check now reports its rules passing (for example ✔ rejects unbudgeted legacy imports from every strict ownership zone). So the marker/ledger problem is closed.
What I cannot close: this head's test job still ends in failure. The retrieved log's failing step is the one that also attempts to upload apps/desktop/e2e/test-results/, which that step reports as not found ("No files were found with the provided path"), so the failure sits in the Desktop e2e area — but the log I have does not surface the failing spec or assertion, and I am not going to guess at it. The PR cannot land until that job is green on this revision, so this is a P2 on the gate rather than a claim about the migration.
Everything I verified about the migration itself still holds, since nothing in this delta touches it: each migrated catalog keeps the same type argument as the satisfies UiCatalog<…> it replaced, so per-locale key completeness is still enforced at compile time; the message values appear only as re-indentations of identical strings; and the resolver in packages/core/src/ui-locale is not modified here.
Gate on this head: test is red (as described); mergeable is true and the state is blocked.
What I could not judge
- The identity of the failing Desktop e2e case, since the log I retrieved does not contain it; whoever holds the job's report can settle it in one look.
- I did not run the workspace or e2e suites myself.
I did not approve, request changes, or merge.
Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
Astro-Han
left a comment
There was a problem hiding this comment.
Follow-up to the review above at the same head, identifying the test failure it could not pin down.
P2: Desktop typecheck fails on the migrated catalogs (run 36980276109, tsc -p tsconfig.renderer.json --noEmit). Every function-valued message in the migrated renderer catalogs now reports TS7006: Parameter '…' implicitly has an 'any' type, for example:
apps/desktop/src/renderer/locales/settings-data-copy.ts:52(created,overwritten,skipped),:56(label),:58(items),:67(error)settings-subagents-copy.ts:106(total),:113(name),:151(max)settings-daily-review-copy.ts:48,settings-web-search-copy.ts:43,:52
Under satisfies UiCatalog<T> these parameters were contextually typed from T. Through the new defineUiMessageCatalog<T>()({…}) / resolveUiMessageCatalog path they no longer are, so the compiler can no longer check that a message function's parameters match the catalog type. That weakens the guarantee the earlier review relied on: missing keys may still be caught, but a placeholder whose parameter was renamed or reordered would now type-check as any.
Fix: make the helper's parameter type flow into the object literal (for example, type the inner call's argument as T or UiMessageSource<T> rather than inferring it from the literal), then rerun npm run typecheck -w apps/desktop. Adding explicit parameter annotations at each call site would silence the errors but keep the weaker contract, so we would prefer the helper fix.
This is an automated review by Claude (Anthropic), run on behalf of the maintainer. It is not an independent human review.
|
Correction to my review above, and the actual cause of the red I said I would not guess at the failure, then went on to say the migration's type story still held. Two things need fixing, and the second is mine:
This remains a P2: the head cannot land until
|
|
Thanks for pinning this down — the cause was in the helper, as you suggested. |
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed ecb29e5199acbd1b363faa59e53bf9a436329597.
The P2 I raised — and publicly corrected myself about — is fixed, and the gate proves it. On this head the test job is completed/success, and that job is the one that previously failed at Desktop typecheck with thirty TS7006 implicit-any errors on the function-valued messages. Those errors are gone.
How it was fixed, read from the type machinery. ui-locale.ts now has an explicit MessageFunction notion and uses it in the shaping types: DeepPartial excludes message functions (so a function is no longer deep-partialled, which is what had destroyed its parameter types), and ExactMessageShape gained a MessageFunction branch that compares functions as functions rather than recursing into them. That is the right level to fix it: the loss was never in the catalogs, it was in the helper's shaping of function-valued entries, so the catalogs keep their unannotated arrow parameters and get their contextual types back.
So the sequence is: I reported the missing UiCatalog marker and the stale ledger, corrected my own claim when the real cause turned out to be the dropped contextual typing, and the fix has now landed at the level that cause pointed to. Nothing else about the migration changes — key presence is still constrained by the type argument, message values are unchanged re-indentations, and the resolver in packages/core is untouched.
Gate on this head: test completed successfully; no Grok involvement; mergeable is true.
What I could not judge
- I did not run the typecheck myself; the evidence is the job result plus the shaping types as written.
- I did not re-read the whole migration; this pass was scoped to the increment that addresses my finding.
I did not approve, request changes, or merge.
Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
Incremental re-review of exact head f12dbb18. The previous reviewed head was ecb29e51, which had become CONFLICTING with main. The increment is a single merge commit, f12dbb18f (merge of upstream/main). It moves the merge-base from c7fa6bb6 to 7c90bac2, pulling in 14 main commits. The PR is unchanged in size: +711/-669 across 74 files.
Isolating the real change. I compared diff(c7fa6bb6, ecb29e51) with diff(7c90bac2, f12dbb18). They touch the same 74 files. Ignoring index and hunk-offset lines, the patches differ only in the two files where main's new content met the PR's transform. Main also changed three more of the PR's files; their resolutions are checked below.
apps/desktop/src/renderer/features/conversation/model/executor-submission.ts:50-66: main added aninvalidmessage for stale model/mode selections. The resolution addsinvalidto thedefineUiMessageCatalog<...>key type and keeps all three locale strings, now insideresolveUiMessageCatalog(...).executorSubmissionErrorstill returnsSUBMISSION_COPY[locale].invalid(:80). Correct.packages/ui/src/executor-model-picker.tsx: main renamedselectionFailedtomodeFailedand addedmode. The PR's wrap now applies to main'sExecutorCopyunchanged (defineUiMessageCatalog<ExecutorCopy>()). Correct.apps/desktop/src/renderer/locales/shell-copy.tsandsettings-external-agents-copy.ts: every line main added sincec7fa6bb6(13 and 3 lines) is present at the new head, in the hoisted resolver form.apps/desktop/renderer-architecture.json: the PR's@maka/core/ui-localedependency-path additions re-apply cleanly on top of main's changes.
No main-side change was dropped, and no PR-side transform was lost in the resolution.
Findings
- P3, carry-over (not introduced by this merge): one catalog is still on the old form.
apps/desktop/src/renderer/features/diagnostics/locales/diagnostics-copy.ts:52is still{...} satisfies UiCatalog<DiagnosticsCopy>. It arrived on main via #5892 (2026-10-02), likely after the PR's initial sweep. It was already unmigrated atecb29e51. It compiles and behaves correctly as is, but the description says everysatisfies UiCatalog<T>catalog is migrated. Its shape is a plain nested string object, with no self-locale keys likeastryx-i18n.tsxand noUiCatalog<string>leaf likeprovider-display-copy.ts, so the same mechanical wrap applies. The only remaining old-form catalogs are this file and the two documented exclusions (packages/ui/src/astryx-i18n.tsx:102,apps/desktop/src/renderer/settings/provider-display-copy.ts:54). Either migrate it or note it in the description.
CI and merge state: test passes on this head. The PR is MERGEABLE again (the earlier conflict is resolved); it is BLOCKED only on required review.
Verdict: the conflict resolution is correct. Only the P3 completeness nit remains; nothing blocks.
|
Thanks @Astro-Han — the P3 is addressed in 7962619: |
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
Incremental re-review of exact head 7962619c. The previous reviewed head was f12dbb18. The increment is one commit, 7962619c9 ("migrate diagnostics-copy to the resolver-based message format"). The merge-base is unchanged (7c90bac2), so the PR diff grows by exactly that commit: +714/-672 across 75 files.
Prior P3: resolved. apps/desktop/src/renderer/features/diagnostics/locales/diagnostics-copy.ts:20,30,52 now uses resolveUiMessageCatalog(defineUiMessageCatalog<DiagnosticsCopy>()({...})) satisfies UiCatalog<DiagnosticsCopy>. That matches the wrap in capability-reason-copy.ts, onboarding-copy.ts and the other migrated catalogs. The three locale objects are byte-identical, and getDiagnosticsCopy is unchanged. At this head, the only satisfies UiCatalog<...> catalogs not wrapped in the resolver are the two documented exclusions (packages/ui/src/astryx-i18n.tsx:102, apps/desktop/src/renderer/settings/provider-display-copy.ts:54).
Current main. Main has moved four commits past the merge-base (to 3597abe8). Two of them touch files this PR changes: apps/desktop/renderer-architecture.json, and settings-provider-copy.ts from #5866, which adds 11 default-model keys per locale. A trial git merge-tree of this head against 3597abe8 is clean. The merged settings-provider-copy.ts keeps all of #5866's new strings in zh-CN, zh-TW and en, inside the PR's resolveUiMessageCatalog(...) wrap. No new old-form catalogs arrive from main.
Findings: none.
CI and merge state: test is still running on this head (pending at review time). The PR is MERGEABLE and BLOCKED only on required review.
Verdict: the last P3 is addressed and the delta is a mechanical, correct wrap. Nothing blocks once CI is green.
Require complete English, Simplified Chinese and Traditional Chinese catalogs while allowing partial overlays for future locales. Preserve function signatures and optional translated fields during resolution. Complete the TUI, shared UI and Desktop compatibility inventory, including locale-shaped Astryx/provider maps and locale parameters. Keep the current language set and all existing copy unchanged. Co-authored-by: Heewon Chae <68272931+heeoneie@users.noreply.github.com> Generated-by: Claude Code Generated-by: OpenAI Codex
7962619 to
8288066
Compare
me2seeks
left a comment
There was a problem hiding this comment.
Reviewed head 8288066. The compatibility migration now covers the omitted catalogs and locale parameters, retains complete English/Simplified Chinese/Traditional Chinese catalogs, and preserves current copy. Local build, all workspace typechecks, 1166 affected tests and architecture/static checks passed; temporary Korean-enablement and negative completeness probes passed as described in the PR. Exact-head hosted CI is green and there are no unresolved review threads. Integration repairs and validation were assisted by OpenAI Codex.
Enable Korean locale recognition and Intl formatting on the shipped-catalog compatibility layer from #5921. Preserve the reviewed Korean Core error, relative-time and quiet-preview copy, using English fallback for untranslated entries. Refresh the source onto current main, adapt relative-time tests to its extracted policy, and preserve complete shipped-language catalogs. Translation slices remain separate changes under the progressive rollout decision. Reviewed source: #5011 at 6dddd3d (same tree as 36a4377). Refs #3975 and #5075 Co-authored-by: heeoneie <68272931+heeoneie@users.noreply.github.com> Co-authored-by: me2seeks <me2seeks@gmail.com> Generated-by: Claude Code Generated-by: OpenAI Codex
Summary
Prepare existing UI catalogs for incremental locale rollout without changing
UI_LOCALES. This is the compatibility prerequisite requested on #5100: compatibility first, then Korean core enablement, then translation slices.English, Simplified Chinese and Traditional Chinese remain complete. Migrated catalogs use
defineShippedUiMessageCatalogto reject missing shipped keys and incompatible function parameters at compile time; only future locales may provide partial overlays.resolveUiMessageCatalogfalls back to English and preserves optional translated fields that English does not define.The migration now includes the previously omitted TUI catalogs, shared progress/session cards, Host resource dialogs, archive retention, workspace recovery and locale parameters in bot/daily-review helpers. Astryx overrides and provider display maps keep their locale-shaped data and use explicit English fallback rather than merging incompatible dictionary shapes.
Existing copy is unchanged. Korean is not enabled here, no English
kostubs are added, and the language selector is unchanged.Scope and verification
82880665e61102517683fb2892d727d5b502ea8a, rebased onto main32b1750487ad2ccee6da5928c5dc8f98851f5bca; merge conflicts preserve main's current delivery/recovery copy.npm run build:testand all workspace typechecks pass.git diff --checkpass.koin the core locale vocabulary/guards/Intl mapping, rebuilt Core and ran all workspace typechecks, including Desktop renderer and Storybook. All pass. This temporary enablement was restored before committing.network_errorentry now fails Core compilation. Type regression cases also reject missing shipped catalogs/keys and wrong function arguments. Existing ICU-formatting tests remain intact.Attribution
The commit preserves
Co-authored-by: Heewon Chae <68272931+heeoneie@users.noreply.github.com>andGenerated-by: Claude Code, and recordsGenerated-by: OpenAI Codexfor integration repairs and validation.AI use
Claude Code contributed the original migration; OpenAI Codex contributed rebase integration, complete compatibility coverage, shipped-language safeguards and verification.
Checklist
Does this PR entail a change in behavior?