Skip to content

refactor(i18n): migrate UI catalogs to the resolver-based message format - #5921

Merged
me2seeks merged 1 commit into
apache:mainfrom
heeoneie:compat/ui-message-catalog-resolver
Oct 13, 2026
Merged

me2seeks merged 1 commit into
apache:mainfrom
heeoneie:compat/ui-message-catalog-resolver

Conversation

@heeoneie

@heeoneie heeoneie commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

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 defineShippedUiMessageCatalog to reject missing shipped keys and incompatible function parameters at compile time; only future locales may provide partial overlays. resolveUiMessageCatalog falls 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 ko stubs are added, and the language selector is unchanged.

Scope and verification

  • Head 82880665e61102517683fb2892d727d5b502ea8a, rebased onto main 32b1750487ad2ccee6da5928c5dc8f98851f5bca; merge conflicts preserve main's current delivery/recovery copy.
  • Clean npm run build:test and all workspace typechecks pass.
  • Core locale/redaction/time/quiet-preview, all shared UI, TUI catalogs/guidance/runner, and focused Desktop copy/recovery tests: 1166 passed, no failures or skips.
  • Renderer architecture checks and their 167 tests pass against main; locale hygiene, TUI copy boundaries, Biome and git diff --check pass.
  • AST comparison of 82 production copy files confirms string literals and template fragments match main exactly.
  • Forward compatibility: temporarily enabled ko in 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.
  • Runtime forward-compatibility probes confirm English fallback for all 15 TUI domains, session hover cards, Astryx and provider display copy; a partial Korean overlay preserves translated keys and falls back for missing keys.
  • Negative verification: deleting the existing Chinese network_error entry now fails Core compilation. Type regression cases also reject missing shipped catalogs/keys and wrong function arguments. Existing ICU-formatting tests remain intact.
  • Hosted CI passed on this exact head: https://github.com/apache/maka/actions/runs/38362465594.

Attribution

The commit preserves Co-authored-by: Heewon Chae <68272931+heeoneie@users.noreply.github.com> and Generated-by: Claude Code, and records Generated-by: OpenAI Codex for integration repairs and validation.

AI use

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Claude Code contributed the original migration; OpenAI Codex contributed rebase integration, complete compatibility coverage, shipped-language safeguards and verification.

Checklist

  • Regression tests cover the change
  • Local build, typechecks, affected tests and static checks pass
  • New-head hosted CI passes

Does this PR entail a change in behavior?

  • Yes — future locale resolution can fall back to English while shipped translation completeness remains enforced
  • No

@github-actions github-actions Bot added the effort/XL Under 2500 readable lines label Oct 2, 2026

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>> becomes defineUiMessageCatalog<Record<GeneralizedErrorClass, string>>()({ … }) (and resolveUiMessageCatalog(...) 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-locale is 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 Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Astro-Han

Copy link
Copy Markdown
Contributor

Correction to my review above, and the actual cause of the red test job (it is not e2e).

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:

  1. The failing step is Desktop typecheck, not e2e. I inferred the area from the step's attempt to upload apps/desktop/e2e/test-results/, which was the wrong signal.

  2. The migration does lose something. typecheck fails with TS7006 ("implicitly has an 'any' type") on the function-valued messages, 30 occurrences in that log, e.g. apps/desktop/src/renderer/locales/settings-data-copy.ts:52:

    connections: (created, overwritten, skipped) => `连接 新增${created}·覆盖${overwritten}·跳过${skipped}`,

    The parameters carry no annotations, and under the new form they no longer receive any: defineUiMessageCatalog<Expected>() (packages/core/src/ui-locale.ts:64-75) infers its Catalog from the argument, so the message functions are typed only by the constraint rather than contextually by the expected shape they way the satisfies UiCatalog<…> literal was. The result is that where the old form checked each interpolation parameter's type and arity, this form silently accepts any.

    So my earlier sentence — that each catalog keeping the same type argument means compilation still enforces the same thing — was too strong. Key presence is still constrained, but parameter typing for interpolated messages is not, which is both the build failure and the more interesting loss. The fix belongs in the helper: it needs to give the message functions their contextual types again (for instance by constraining each locale's values against the expected shape rather than only the catalog object), after which the annotations above can stay unnecessary.

This remains a P2: the head cannot land until test is green, and the point of this note is that the cause is a type-safety regression in the new form rather than a test-fixture problem.


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.

@heeoneie

heeoneie commented Oct 3, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for pinning this down — the cause was in the helper, as you suggested. DeepPartial and ExactMessageShape treated functions as plain objects and mapped them to {}, so function-valued messages in non-en locales lost their contextual signature. Fixed in ecb29e5 by passing functions through both helpers unchanged. Desktop typecheck goes from 30× TS7006 to 0 locally, and a deliberately wrong use (skipped.toFixed() > 0) now fails with TS2365, so parameter typing is enforced again. The test job is green on this head.

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 an invalid message for stale model/mode selections. The resolution adds invalid to the defineUiMessageCatalog<...> key type and keeps all three locale strings, now inside resolveUiMessageCatalog(...). executorSubmissionError still returns SUBMISSION_COPY[locale].invalid (:80). Correct.
  • packages/ui/src/executor-model-picker.tsx: main renamed selectionFailed to modeFailed and added mode. The PR's wrap now applies to main's ExecutorCopy unchanged (defineUiMessageCatalog<ExecutorCopy>()). Correct.
  • apps/desktop/src/renderer/locales/shell-copy.ts and settings-external-agents-copy.ts: every line main added since c7fa6bb6 (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-locale dependency-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:52 is still {...} satisfies UiCatalog<DiagnosticsCopy>. It arrived on main via #5892 (2026-10-02), likely after the PR's initial sweep. It was already unmigrated at ecb29e51. It compiles and behaves correctly as is, but the description says every satisfies UiCatalog<T> catalog is migrated. Its shape is a plain nested string object, with no self-locale keys like astryx-i18n.tsx and no UiCatalog<string> leaf like provider-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.

@heeoneie

heeoneie commented Oct 6, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @Astro-Han — the P3 is addressed in 7962619: diagnostics-copy.ts is now wrapped in resolveUiMessageCatalog(defineUiMessageCatalog<DiagnosticsCopy>()(...)), keeping the trailing satisfies UiCatalog<DiagnosticsCopy> like the other migrated catalogs. The renderer architecture check and tsc -p tsconfig.renderer.json pass locally, and diagnostics-owner.test is 16/16. The only old-form catalogs left are the two documented exclusions (astryx-i18n.tsx, provider-display-copy.ts).

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@me2seeks
me2seeks force-pushed the compat/ui-message-catalog-resolver branch from 7962619 to 8288066 Compare October 13, 2026 07:24

@me2seeks me2seeks left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@me2seeks
me2seeks merged commit 6c63fa0 into apache:main Oct 13, 2026
1 check passed
me2seeks added a commit that referenced this pull request Oct 13, 2026
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/XL Under 2500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants