chore(i18n): index catalogs instead of branching on locale - #4824
Conversation
6245704 to
0f5f052
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed at 0f5f052. The six catalog conversions are exactly what the RFC asks for and I checked each renders byte-identical output to main (including String(true), 小時, and the bypass sentence). The en fast path in resolveUiMessageCatalog is a reference shortcut, not a locale leak, so leaving it with a comment is right. One P2 on the one behavior change, then smaller things.
P2: the provenance rule leaves the keyword ladder with exactly one input, and it maps that input wrong. BUNDLED_SKILL_CATALOG has a single entry, computer-use (bundled-skill-catalog.generated.ts:16). Its description says "prefer Browser tools for web pages", none of the earlier keyword groups match, so skill-status.ts:132 hits browser and renders 打开、检查、操作网页界面 / "Open, inspect, and operate web interfaces" for a skill whose whole point is native desktop apps. main has the same bug, but main at least uses the ladder for every non-CJK skill; after this PR the ladder's other five branches and their 21 localized strings are unreachable, and the premise in the body ("only bundled skills carry product copy") is really "only one skill carries product copy". Two coherent shapes: delete the ladder and give bundled skills a Record<bundledId, localized description> (one row today, no guessing), or keep this PR to the catalog conversions and argue the provenance rule separately. There is also a narrow regression: a bundled skill the user has edited keeps sourceType: 'bundled' (skills-governance.ts:148-160), so a user who rewrote computer-use's description in Chinese sees it today and gets the canned line after this PR.
Smaller:
requiresBypass.errorMessageistitle + descriptionretyped in three locales (tool-activity/copy.ts:194-197, 299-302, 404-407); the banner reads the parts, the tooltip reads the whole, and nothing keeps them in sync. Make the separator the catalog entry instead, or add one assertion thaterrorMessage === title + sep + description.astryxMessageOverridesnow returnsOverrides, so the?.atastryx-i18n.tsx:68is dead, and the thunk wrapper around three synchronous values can be a plainsatisfies UiCatalog<Overrides>table.RETRY_DELAY_UNITSis a second per-locale table in a file that already hasConversationCopy(comparegoalElapsedatconversation-copy.ts:607); put the units on the copy object.- The first case in
skill-status.test.tspasses onmaintoo ('Create and edit Word documents.'is non-CJK and hitswordeither way). Only the second case fails without the change.
User-visible, so before this merges someone needs to look at: computer-use's description in the skill library and inspector in zh-CN / zh-TW / en; a workspace skill with an English description under a Chinese UI (the declared behavior change); the bypass tooltip vs banner text in all three locales. CI test was still pending when I read this.
Evidence boundary: static read at 0f5f052 against cbeb1a93c7; the keyword match for computer-use was recomputed on the description string, not observed in the app. No build, no tests.
AI-assisted review: drafted with Maka; I verified the bundled catalog contents and the keyword match myself.
0f5f052 to
6a43bdd
Compare
bcb3c85 to
9a01375
Compare
Six presentation helpers still picked copy with locale comparisons, so a fourth UI locale would compile and silently render the wrong language. Each now indexes a UiCatalog, and bypass errors are complete per-locale messages instead of a title-and-description concatenation. Skill library descriptions stop sniffing the text for CJK: only bundled skills carry product copy, so a per-id table names what each does and a bundled skill the user has rewritten keeps the rewritten description. Generated-by: Claude Code Claude-Session: https://claude.ai/code/session_01AqdSkg56F2x55wEGRWvzcB
Generated-by: OpenCode
9a01375 to
127c870
Compare
Generated-by: Claude Code
723138d to
146c7a8
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Re-reviewed at 146c7a8. The P2 is closed the way I hoped: the keyword ladder and its 21 strings are gone, bundled skills render bundledDescription[skill.id], a user-edited bundled skill keeps its text (skill-status.ts:120), and the three screenshots show computer-use describing desktop apps in every locale. The smaller items are all done too: the tooltip is asserted to open with the banner title and end with its description in every locale, the Astryx overrides are a plain satisfies UiCatalog<Overrides> table with the dead ?. removed, the duration units live on each copy entry, and the first skill-status test now fails on main.
One P3, then approve-worthy:
P3: a bundled skill without a copy entry falls back to its raw English silently. bundledDescription is Partial<Record<string, string>> and ?? raw covers the miss, so the next entry added to BUNDLED_SKILL_CATALOG shows English under a Chinese UI with nothing failing. @maka/ui cannot import the catalog (it lives in @maka/runtime), but apps/desktop can: one test there that walks BUNDLED_SKILL_CATALOG × UI_LOCALES and asserts every id has an entry closes it without changing the type. Fine as a follow-up commit here or on its own.
Ordering: no shared files with the other four locale PRs. CI test was pending when I read this head.
Evidence boundary: static read of the delta since 0f5f052 against 03b5a2cc5a; no build, no suites.
AI-assisted review: drafted with Maka; I verified the ladder removal, the userModified branch and the Partial fallback myself.
Summary
Six presentation helpers outside the open locale PRs still chose copy with
locale === '…'comparisons, so adding a fourthUiLocalewould compile and render the wrong language: the client-settings confirmation dialog, the project picker title, the Astryx message overrides, the retry-delay formatter, and the bypass-required tool error.resolveUiMessageCatalogkeeps itsenfast path:enis the base every other locale merges over, so that branch is the fallback rule itself rather than a locale leak. Each now indexes aUiCatalog, which the type system enforces per locale.The bypass-required error is a complete
errorMessagein each locale catalog, independent of the banner title and description. This lets future translations choose their own sentence structure rather than fitting a title/separator/description formula; current wording is unchanged.formatSkillLibraryDescriptionalso decided whether a description was product copy by sniffing it for CJK characters, then guessed which canned line to show from keywords in the text. Only bundled skills carry product copy, and the catalog has exactly one,computer-use, whose description mentions "prefer Browser tools" and so landed on the browser line. Provenance and identity decide instead: a bundled skill renders the localized description filed under its id, a bundled skill the user rewrote keeps the rewritten text, and a skill the user authored renders its own description as written in every locale. The keyword ladder and its 21 unreachable strings are gone. Two user-visible changes:computer-usenow describes desktop apps rather than web pages in all three locales, and an English description on a workspace skill shows in English under a Chinese UI rather than a generic canned line.Refs #2672
Verification
In the running app (
MAKA_E2Efixture;computer-useinstalled from the built-in tab, thedocx-reviewworkspace skill and the bypass tool result seeded into the local fixture for this capture only).computer-usein the skill library and inspector, zh-CN / zh-TW / en, with the English-described workspace skill under the Chinese UI:The bypass banner in an expanded tool row, zh-CN / zh-TW / en. The error icon's
titletooltip in the same runs reads需要“绕过”模式。此操作会直接控制本机应用,无法在沙箱模式下执行。/需要“繞過”模式。此操作會直接控制本機應用,無法在沙箱模式下執行。/Bypass mode required. This action controls a local app directly and cannot run inside the sandbox.:npm run typecheck, core (824) / ui (374) / desktop main (2138) suites, the renderer architecture ratchet,format:check, andknippass locally. The locale-hygiene scan reports zerolocale-branch/cjk-sniffhits in the touched files.Follow-up verification: core and UI builds, UI typecheck, all 376 UI tests, repository-wide
format:check, and lint of the three changed files pass. The bypass rendering test checks the complete error-icon text in everyUI_LOCALESentry, including Traditional Chinese, and asserts the tooltip opens with the banner title and ends with the banner description so the two cannot drift.After review (rebased on
main): UI typecheck, all 365 UI tests from a cleandist,format:check, and biome on the changed files pass.AI use
Select exactly one:
Tool(s) and scope: Claude Code drafted the catalog conversions, the skill-status test, the review follow-ups (per-id bundled description table, retry units on the copy object, plain Astryx override table, tooltip/banner sync assertion), and this description; the provenance rule was decided by hand. OpenCode replaced sentence concatenation with complete bypass-error catalog messages, extended the rendering test, and updated this description.
Checklist
Does this PR entail a change in behavior?