Repository navigation
Conversation
zh.ts carries all 375 keys and joins SUPPORTED_LANGUAGES, which is the only registration the settings picker, the per-language client script and the language clamping need. Every locale gains the lang.zh and langLocal.zh names the picker shows, and the README feature line and the assets.ts comment (one script per language, built at module init) follow. Placeholder and key parity now iterate SUPPORTED_LANGUAGES instead of a hardcoded id/sv list, so this locale and any later one are checked the day they register. New assertions pin zh where the runtime does the work: Intl.DisplayNames resolves 美国, Intl.NumberFormat groups 1,234,567, toLocaleDateString renders 7月, the picker labels the other languages in Chinese, and a lang=zh page links and serves its own client script.
DennisAlund
left a comment
There was a problem hiding this comment.
The locale file is complete: key and placeholder parity hold, the two parity tests iterate SUPPORTED_LANGUAGES, the server-side formatters take zh through Intl, and no call site breaks on the widened SupportedLanguage union. Inline comments below cover what the PR exposes through existing composition code or leaves half-registered. Two findings sit in files outside this diff:
src/client.ts:971-978 (chart axis labels). fmtLabel glues the new month.* strings into a Latin Mon YY / Mon D composition. For a lang=zh viewer, monthly buckets on the all-time range render as 7月 26, which reads as July 26th rather than July 2026, and daily buckets render as 7月 16 instead of 7月16日. The server-side formatBucketLabel pinned in big-chart.test.ts is unaffected; this is the client-only path the month keys feed.
browser-extensions/src/i18n/index.ts:13 and browser-extensions/README.md:164. The extension's locale set and README ("Three languages, mirroring the admin app") stop mirroring the admin app once zh ships. A browser reporting zh-CN gets a Chinese dashboard and an English popup. Either the extension gains zh in a follow-up or the README sentence changes; the PR does neither and does not say which.
Not flagged after checking: <html lang="zh"> resolves to Simplified Chinese in browsers and ICU, matching the 简体中文 label; CJK text falls through the Manrope/Space Grotesk unicode-range subsets to system-ui with no new font; fmtNumber, countryName, formatBucketLabel, the widget cache key, the settings validation message and the per-language asset map all take zh through SUPPORTED_LANGUAGES.
| export type SupportedLanguage = (typeof SUPPORTED_LANGUAGES)[number]; | ||
|
|
||
| const translations: Record<string, Translations> = { en, id, sv }; | ||
| const translations: Record<string, Translations> = { en, id, sv, zh }; |
There was a problem hiding this comment.
The map is typed Record<string, Translations>, so a locale is registered in two places (SUPPORTED_LANGUAGES and this map) and forgetting the map entry compiles. The PR description's claim that SUPPORTED_LANGUAGES is the only registration is not true of the code.
A later locale added to SUPPORTED_LANGUAGES but not here passes isSupportedLanguage, getTranslations returns undefined, assets.ts builds var T = undefined; for it at module init, and createTranslateFn throws on the first t() call, which 500s every admin page for that viewer.
Type it Record<SupportedLanguage, Translations> (browser-extensions/src/i18n/index.ts:16 already does) or derive SUPPORTED_LANGUAGES from the map's keys so one registration exists.
There was a problem hiding this comment.
Fixed in 091145a. The map is Record<SupportedLanguage, Translations> now, so a language added to the array without a table here fails the typecheck. You were right that the PR description claimed SUPPORTED_LANGUAGES was the only registration: that was wrong, and the description now says the table is a second registration and what makes it safe.
| }); | ||
| }); | ||
|
|
||
| describe("Settings page language picker", () => { |
There was a problem hiding this comment.
The new picker option is pinned with a render assertion only. No Playwright step selects zh and asserts the reloaded page renders in Chinese, and the PR ran only yarn test.
CLAUDE.md, Testing: "New admin controls (chip, sort, paginator, selector, row action) get an e2e step that clicks them and asserts the URL and the marked state, alongside the vitest render assertions." The selector gains a new offered choice here and e2e/ has no language-picker step at all (only the perf.spec html lang truthiness check). A dead onchange, a reload that lands on English, or CJK text clipped in the fixed-width chart labels would ship unseen.
There was a problem hiding this comment.
You're right that the picker has no browser coverage, and the gap is older than this PR. #language-picker (src/pages/settings.tsx:45) shipped 2026-03-31 in 91c1aaa, the original en/id/sv commit, which touched no e2e/ file because the suite did not exist yet. e2e/ arrived 2026-08-27 in 317339a, the same commit that added the "new admin controls get an e2e step" line to CLAUDE.md. No language has a step today, not only the new one: the only lang assertion in e2e/ is perf.spec.ts:219, expect(a11y.lang).toBeTruthy(), and no commit has ever added a step that drives the picker.
So this PR adds a fourth option to a control that was already uncovered, and it does not reduce coverage. I am not adding the step here.
Driving the picker end to end is e2e-suite work rather than locale work. The spec has to select each offered language, follow the cookie and the PUT /_/admin/api/preferences and the reload that setLanguage (src/client.ts:151) performs, and assert both the rendered language and the marked option. That is a larger change than the locale that prompted it, and the shape is identical for en, id and sv, so it belongs in a change that covers the picker as a control. The same reasoning is why I read the rule as aimed at new controls: this PR adds an option to an existing one.
The part of your comment that is a defect rather than a coverage gap is the axis labels. fmtLabel composes a Latin Mon D / Mon YY string out of the month.* keys, so zh renders 7月 16 and 7月 26 where the locale wants 7月16日 and 2026年7月. That wants a locale-aware Intl.DateTimeFormat, the call fmtBucketDate already makes for tooltips, and I will take it as its own change.
| "links.allDisabled": "所有链接均已停用。选择“已停用”筛选即可查看。", | ||
| "links.noMatches": "没有符合当前筛选条件的链接。", | ||
| "links.noSearchMatches": "没有匹配“{query}”的链接。", | ||
| "links.noSearchMatchesInFilter": "在{filter}中没有匹配“{query}”的链接。", |
There was a problem hiding this comment.
links.noSearchMatchesInFilter substitutes the bare chip label into 在{filter}中, which reads as "in active" / "in disabled" with no noun. links.tsx:187 passes t(FILTER_LABEL[filter]) (活跃 or 已停用), so a viewer searching abc under the disabled chip reads 在已停用中没有匹配“abc”的链接。.
A form that names what the filter is, such as 在“{filter}”筛选下没有匹配“{query}”的链接。, keeps the placeholder set identical and reads as a sentence.
There was a problem hiding this comment.
Fixed in 21bda93. It reads 在“{filter}”筛选下没有匹配“{query}”的链接。 The placeholder set is unchanged.
| * are built once at module init and served from a map. | ||
| * language). The asset set is fixed for the life of the isolate, so each | ||
| * asset is built once at module init and served from a map: every language | ||
| * registered in SUPPORTED_LANGUAGES costs one script built here. |
There was a problem hiding this comment.
This comment promises one script for "every language registered in SUPPORTED_LANGUAGES", but SCRIPT_PATH (line 93) hardcodes a two-letter code, [a-z]{2}. A later zh-Hant or pt-BR entry builds its asset and serves it from byPath, but during a deploy an old-build document requesting /_/assets/client.zh-Hant.<hash>.js fails the regex in sameAssetFromThisBuild and gets a 404 instead of the no-store fallback this module exists to provide. The same [a-z]{2} is copied into the SCRIPT regex in src/__tests__/handler/admin-assets.test.ts:26.
Match [A-Za-z-]+ and let isSupportedLanguage decide, or state the two-letter constraint here.
There was a problem hiding this comment.
Fixed in 4ac4cb8. The segment is matched loosely as [A-Za-z-]+ and isSupportedLanguage stays the gate, which is the first option you offered. A registered subtagged code now gets the no-store fallback. The test copy at admin-assets.test.ts:26 is widened with it, and the branded 404 case gained client.zh-Hant.<hash>.js so the assertion pins that the refusal comes from the supported-language check rather than from the shape of the path.
| expect(out).toMatch(/1.234.567/); // any single separator character | ||
| }); | ||
|
|
||
| it("groups thousands for zh", () => { |
There was a problem hiding this comment.
This asserts 1,234,567, the exact string the en-US fallback it claims to guard against would also produce, so it pins nothing about zh. A runtime without zh ICU data falls back to en-US, formats the same output, the test passes, and the comment's "deliberate decision rather than a surprise" never triggers.
Assert something the fallback cannot satisfy, such as new Intl.NumberFormat("zh").resolvedOptions().locale starting with zh, or drop the comment and keep this as a plain output pin.
There was a problem hiding this comment.
Fixed in 88beabb. The case asserts new Intl.NumberFormat("zh").resolvedOptions().locale matches /^zh/ first, then the digits, so a runtime without zh ICU data fails here instead of passing on the fallback output.
| it("gives each language its own script URL", async () => { | ||
| const en = (await pageHtml("/_/admin/settings")).match(SCRIPT)![1]; | ||
| const sv = (await pageHtml("/_/admin/settings", { Cookie: "lang=sv" })).match(SCRIPT)![1]; | ||
| const zh = (await pageHtml("/_/admin/settings", { Cookie: "lang=zh" })).match(SCRIPT)![1]; |
There was a problem hiding this comment.
This local const zh holding a script URL shadows the zh locale import added at line 13, so zh is a URL in this test and the translation table in the next (zh["client.themeUpdated"] ten lines down). Name the URLs enUrl / svUrl / zhUrl, or import the table as zhStrings.
There was a problem hiding this comment.
Fixed in 4ac4cb8, renamed to zhUrl. One correction for the record: the table and the URL are in different it blocks, so the next test was never reading a URL; the name was a trap for the reader, not a live mix-up. The rename removes it either way.
| it.each([["id", id], ["sv", sv]] as const)("keeps every %s placeholder that en declares", (_name, locale) => { | ||
| // silently drop the interpolation the key exists for. Driven off | ||
| // SUPPORTED_LANGUAGES so a new locale is covered the day it is registered. | ||
| const translated = SUPPORTED_LANGUAGES.filter((lang) => lang !== DEFAULT_LANGUAGE); |
There was a problem hiding this comment.
translated (SUPPORTED_LANGUAGES minus the default) is computed here and again at line 159 in the key-parity describe. Two copies drift independently when the default language or the exclusion rule changes. Hoist a single const translated to module scope next to flattenKeys and placeholders, which both describes already share.
There was a problem hiding this comment.
Fixed in 88beabb. One module-scope translated next to flattenKeys and placeholders, read by both suites.
| @@ -148,12 +156,10 @@ describe("i18n", () => { | |||
|
|
|||
| describe("key parity across locales", () => { | |||
There was a problem hiding this comment.
The constants › lists all supported languages test just above (line 150) still checks only en, id and sv, while its sibling isSupportedLanguage test was extended with zh. Someone trimming SUPPORTED_LANGUAGES back to three entries sees that test stay green; only the derived parity tests notice, and they notice by running zero cases for zh rather than by failing. Add expect(SUPPORTED_LANGUAGES).toContain("zh") or assert the exact array.
There was a problem hiding this comment.
Fixed in 88beabb. It asserts [...SUPPORTED_LANGUAGES] equals ["en", "id", "sv", "zh"], so trimming the array fails here rather than leaving the parity cases to run one fewer silently.
| - **QR codes** as SVG for any slug, from the admin UI, the API, the SDKs and MCP. Scans are tracked separately from link clicks. | ||
| - **Edge redirects.** Slug lookups are cached in Workers KV in front of D1, and click recording runs after the redirect is sent. | ||
| - **Admin dashboard** in English, Indonesian and Swedish, with three themes, per-link and per-bundle analytics, API key management and settings. | ||
| - **Admin dashboard** in English, Indonesian, Swedish and Simplified Chinese, with three themes, per-link and per-bundle analytics, API key management and settings. |
There was a problem hiding this comment.
This extends a hand-maintained enumeration of admin languages whose source of truth is src/i18n/. CLAUDE.md, Documentation: "Never enumerate plugins, skills, dependencies, components, or any list backed by a file/folder source of truth. Reference the source directly." The next locale needs another README edit and the list drifts the first time one is forgotten. Phrase it as "in four languages (see src/i18n/)" or link the directory, or say in the PR that a reader-facing list is wanted here.
There was a problem hiding this comment.
Fixed in 42e4d3a. The line reads "in four languages (see src/i18n/ for the set)".
The map was typed Record<string, Translations>, so a language could join SUPPORTED_LANGUAGES with no table here and still compile: TypeScript was satisfied, isSupportedLanguage said yes, getTranslations returned undefined, the per-language client script was built as `var T = undefined;` at module init, and createTranslateFn threw on the first t() call. That 500s every admin page for the viewer who selected the language. Record<SupportedLanguage, Translations> turns the missing entry into a build failure. Raised in review; the same map in browser-extensions already uses the union.
SCRIPT_PATH hardcoded a two-letter code, [a-z]{2}, while the comment above
the module promised one script for every language registered in
SUPPORTED_LANGUAGES. A locale with a subtag (zh-Hant, pt-BR) would build
its asset and serve it from byPath, but a document from another build
asking for it during a deploy window missed the regex and got a 404
instead of the no-store fallback this module exists to provide.
The segment is matched loosely now, and isSupportedLanguage stays the gate
that decides which codes this build holds a script for. The test's copy of
the regex is widened with it, the `zh` URL locals that shadowed the zh
translation table are renamed, and the branded 404 case gains a subtagged
code so it pins that the refusal comes from the supported-language check
rather than from the shape of the path.
Three defects in the assertions this locale added or touched. format.test.ts asserted fmtNumber(1234567, "zh") === "1,234,567", the exact output a runtime without zh ICU data produces through the en-US fallback, so the case held whether or not zh resolved and could not catch the regression its comment claimed to guard. It asserts the locale the formatter resolved, then the digits. i18n.test.ts computed the list of translated languages twice, so a change to the default language or to the exclusion rule had to be made in two places. Both suites read one module-scope list. lists all supported languages checked three toContain calls while the parity suites iterate SUPPORTED_LANGUAGES, so a locale trimmed from the array left it green and the parity cases simply stopped running. It asserts the exact array.
"在{filter}中没有匹配“{query}”的链接。" substitutes the bare chip label,
so searching under the disabled chip read 在已停用中 with no noun: the
chip label is an adjective and the sentence needs what it modifies. The
placeholder set is unchanged.
The feature line enumerated the admin languages by hand, which needs an edit the next time a locale registers and drifts the first time one is forgotten. The set lives in src/i18n/.
|
Two findings from the review body are now their own changes, since neither is about the locale itself.
The picker step is the one finding this PR does not take, for the reasons in its thread. |
What
Adds Simplified Chinese as a fourth admin language, on the same path the existing locales take.
src/i18n/zh.tscarries all 375 keys and joinsSUPPORTED_LANGUAGES. That array is what the settings picker, the per-language client script and the language clamping read. The locale table is a second registration, now typedRecord<SupportedLanguage, Translations>so a language added to the array without a table fails the build instead of servingundefinedinto that language's client script.lang.zhandlangLocal.zhnames the picker shows.src/i18n/instead of listing the languages by hand.Placeholder and key parity iterate
SUPPORTED_LANGUAGESinstead of a hardcodedid/svlist, so this locale and any later one are checked the day they register.New assertions pin
zhwhere the runtime does the work:Intl.DisplayNamesresolves 美国,Intl.NumberFormatresolves azhlocale and groups 1,234,567,toLocaleDateStringrenders 7月, the picker labels the other languages in Chinese, and alang=zhpage links and serves its own client script.Tests
yarn test: 102 files, 1605 tests pass.yarn e2e: 32 tests pass.yarn tsc --noEmit: clean.Review
Commits 2 through 6 answer the inline review: the locale table is exhaustive over
SupportedLanguage, a language-shaped client script path is recognized even when the code carries a subtag, thezhempty-search string names the filter, and the assertions pin the resolved locale rather than the en-US fallback.Two findings from the review body are not in this PR. The axis-label composition in
src/client.ts(fmtLabel) composes a LatinMon D/Mon YYstring out of themonth.*keys, which is a defect for zh and wants a locale-aware formatter; that is its own change. The browser extension keeps its own language set and release track, so it follows separately.