From 33f004ed043c0dd9d3e38d26dbf6b0d377a24b3e Mon Sep 17 00:00:00 2001 From: Maurice Date: Thu, 8 Oct 2026 12:25:56 +0200 Subject: [PATCH] fix(client): a wide template key needs a reviewed allow-list entry lint:i18n-keys let any template key through as long as its fixed parts matched one en key, and each interpolation could stand for any run of characters, dots included. `settings.${mode}` therefore reached all 574 settings keys and `help.guide.${id}.${part}` nearly 2000, so a wrong suffix behind such a prefix could never fail, and none of the 109 templates had been looked at. An interpolation now stands for one key segment, and a template passes by itself only when it reaches between 1 and MAX_IMPLICIT_MATCHES (30) en keys, the size of an enum such as the docsync error codes. A wider one fails unless DYNAMIC_ALLOWED has an entry for that file and template saying what bounds the value. The twelve wide templates in use today get one each: the help key builders (checked by the registry test), literal tuples beside the call, typed unions, the generated plugin permissions and the seeded photo provider fields. An entry whose file no longer uses the template, or whose template has become narrow enough to pass alone, fails, so the list stays short and current. Listing all 109 templates instead was considered and left out: the narrow ones would only restate their own prefix, and the bound already keeps a mistake inside one small namespace. --- client/CLAUDE.md | 2 +- client/scripts/i18n-keys.mjs | 152 +++++++++++++++--- client/tests/unit/i18n/i18nKeysScript.test.ts | 50 +++++- 3 files changed, 173 insertions(+), 31 deletions(-) diff --git a/client/CLAUDE.md b/client/CLAUDE.md index 23e0a4313..ccff8474f 100644 --- a/client/CLAUDE.md +++ b/client/CLAUDE.md @@ -64,7 +64,7 @@ The offline core is flagship work surrounded by a periphery that ignores it. New ## Big-picture pieces - **Maps** (`src/components/Map/`): two interchangeable renderers — Leaflet and Mapbox/MapLibre GL — chosen at runtime by `MapViewAuto.tsx`. Keep both in sync when changing map features, and keep `mapbox-gl` and `maplibre-gl` in separate chunks (`check:gl-split`). The raster prefetcher fetches tiles `no-cors` so custom tile providers without CORS headers keep working; the vector prefetcher deliberately uses `cors` because it has to read the responses — don't "align" one with the other. -- **i18n** (`src/i18n/TranslationContext.tsx`): `en` is bundled; every other locale is a dynamic `import('@trek/shared/i18n/')` so Vite emits one chunk per locale. Strings live in `shared/`, never here. `t()` returns an unknown key unchanged, so `lint:i18n-keys` (`scripts/i18n-keys.mjs`) resolves every literal key in `t()`, `tHtml()`, `tr()`, ``, `translateApiError` and `*Key` props against `shared/src/i18n/en`; a template key passes when its fixed parts match an en key, or through `DYNAMIC_ALLOWED` with a reason. Pass counts as numbers, and let a plural group pick the wording instead of switching keys on the count. +- **i18n** (`src/i18n/TranslationContext.tsx`): `en` is bundled; every other locale is a dynamic `import('@trek/shared/i18n/')` so Vite emits one chunk per locale. Strings live in `shared/`, never here. `t()` returns an unknown key unchanged, so `lint:i18n-keys` (`scripts/i18n-keys.mjs`) resolves every literal key in `t()`, `tHtml()`, `tr()`, ``, `translateApiError` and `*Key` props against `shared/src/i18n/en`; a template key is read with each interpolation as one key segment and passes by itself only when it reaches between 1 and `MAX_IMPLICIT_MATCHES` (30) en keys; a wider one, or one with no fixed dotted prefix, needs a `DYNAMIC_ALLOWED` entry keyed by file and template that says what bounds the value, and an entry the check no longer needs fails. Pass counts as numbers, and let a plural group pick the wording instead of switching keys on the count. - **Mobile shell** (`src/mobile/`): below the phone breakpoint (`useIsPhone`) `App.tsx` wraps routes in `MobileShell` and the `M*` screens under `mobile/screens/` take over. A UI change to a domain with an `M*` twin usually needs both — and the shared-hook rule below. - **Plugins UI** (`src/components/Plugins/`): third-party surfaces render only inside the sandboxed `PluginFrame` iframe (postMessage bridge, live theme-token sync). Host-rendered contribution points (widgets, schedule rows, map layers, planner columns/actions) render server-normalized primitives only — plugin markup never runs inline. Authoring is covered by the `trek-plugin-dev` skill. - **Managed installs** (`src/managed/index.tsx`): the attachment point for screens that only exist on a centrally administered install. Empty here by design — an operator replaces it at build time. Don't put features there. diff --git a/client/scripts/i18n-keys.mjs b/client/scripts/i18n-keys.mjs index a1bbfcfe1..820eebcbb 100644 --- a/client/scripts/i18n-keys.mjs +++ b/client/scripts/i18n-keys.mjs @@ -22,8 +22,16 @@ * A key exists when en declares it, or declares `key.other` (a plural group * whose general form is spelled out). A template literal key such as * t(`budget.category.${id}`), or a prefix with the rest appended - * (t('costs.filter.' + f)), cannot be resolved; it passes when its fixed - * parts match at least one en key, and otherwise only through DYNAMIC_ALLOWED. + * (t('costs.filter.' + f)), cannot be resolved. It is read as a pattern in + * which each interpolation stands for one key segment (no dot), and it passes + * on its own only when that pattern is narrow: it matches at least one en key + * and at most MAX_IMPLICIT_MATCHES of them, the size of an enum such as the + * docsync error codes. A pattern that matches nothing fails. A wider one + * (`settings.${mode}` reaches every settings key, so a wrong suffix would + * pass unseen) and one without a fixed dotted prefix need an entry in + * DYNAMIC_ALLOWED, keyed by file and template, saying what bounds the value. + * Listing every narrow template instead would be a hundred entries restating + * their own prefix; the bound is what keeps those honest. * A key held in a variable (`t(opt.label)`) is out of reach unless the table * names it in a *Key property. * @@ -44,19 +52,77 @@ const HERE = dirname(fileURLToPath(import.meta.url)) const SRC = join(HERE, '..', 'src') const SHARED_SCRIPTS = join(HERE, '..', '..', 'shared', 'scripts') +/** How many en keys a template may reach and still pass without an entry below. */ +export const MAX_IMPLICIT_MATCHES = 30 + /** - * Template literal keys whose fixed parts match no en key, each with the - * reason it still resolves and the keys it resolves to, which must exist. - * Keep this short: every entry is a key the check cannot follow. + * Template keys the check cannot bound by itself: wider than + * MAX_IMPLICIT_MATCHES, or without a fixed dotted prefix. Each names the file + * it sits in and why the interpolated value can only be a real key; one with + * no fixed prefix also lists the keys it resolves to, which must exist. An + * entry nothing uses any more fails, so the list cannot rot. */ export const DYNAMIC_ALLOWED = [ { + file: 'components/Trips/TripShareDialog.tsx', template: '${opt.label}Hint', because: 'TripShareDialog appends Hint to each SHARE_OPTIONS label (useTripShare.ts); share.optTravelOnlyHint ' + 'and share.optHideImagesHint exist, and the template has no fixed part to match them by', resolves: ['share.optTravelOnlyHint', 'share.optHideImagesHint'], }, + ...[ + 'help.ctx.${id}.${part}', + 'help.ctx.${id}.bullet.${n}', + 'help.guide.${id}.${part}', + 'help.guide.${id}.step.${n}', + 'help.guide.${id}.tip.${n}', + ].map((template) => ({ + file: 'help/registry.ts', + template, + because: + 'a key builder over the registered help contexts and guides; src/help/registry.test.ts resolves every ' + + 'key of every registered entry against en', + })), + { + file: 'mobile/screens/dashboard/MUserMenu.tsx', + template: 'settings.${mode}', + because: "mode is a ThemeMode from resolveMode(), 'light' | 'dark' | 'auto', and settings.light/dark/auto exist", + }, + ...['components/Studio/StudioElementsPanel.tsx', 'components/Studio/StudioInspector.tsx'].map((file) => ({ + file, + template: 'journey.studio.${key}', + because: 'key comes from an `as const` tuple of frame and stroke styles written beside the call, typed literals', + })), + ...['mobile/screens/trip/sheets/MReservationSheet.tsx', 'mobile/screens/trip/sheets/MTransportFormSheet.tsx'].map( + (file) => ({ + file, + template: 'reservations.${s}', + because: "s iterates the literal ['pending', 'confirmed'] as const; reservations.pending/confirmed exist", + }), + ), + ...['components/Admin/AdminPluginsPanel.tsx', 'mobile/screens/admin/MAdminPluginsPanel.tsx'].map((file) => ({ + file, + template: 'admin.plugins.perm.${perm}', + because: + 'PermLabel translates a permission only when PERM_KEYS (generated PLUGIN_PERMISSIONS) holds it and shows ' + + 'the raw code otherwise; the plugin-facts gate keeps the permission keys in step with en', + })), + ...['components/Settings/PhotoProvidersSection.tsx', 'mobile/screens/settings/MPhotoProvidersSection.tsx'].flatMap( + (file) => + ['memories.${field.label}', 'memories.${field.hint}'].map((template) => ({ + file, + template, + because: + 'field label and hint names come from the provider field rows PhotoProviderSeeder writes, each a ' + + 'memories.* key in en', + })), + ), + { + file: 'components/Tours/planner/TourPlannerPanels.tsx', + template: 'tours.planner.${role}', + because: "roleLabel takes role: 'start' | 'via' | 'end', and tours.planner.start/via/end exist", + }, ] const KEY_RE = /^[a-z][a-zA-Z0-9_]*(?:\.[a-zA-Z0-9_:-]+)+$/ @@ -121,12 +187,15 @@ export function literalsIn(arg) { return { quoted, templates } } -/** A template literal's key pattern: its fixed parts, anything in between. Null when it has no fixed dotted prefix. */ +/** + * A template literal's key pattern: its fixed parts, one key segment (no dot) + * for each interpolation. Null when it has no fixed dotted prefix. + */ export function templatePattern(template) { const parts = template.split(/\$\{[^}]*\}/) if (!/^[a-z][a-zA-Z0-9_]*\./.test(parts[0] ?? '')) return null const escape = (s) => s.replace(/[.*+?^${}()|[\]\\]/g, '\\$&') - return new RegExp(`^${parts.map(escape).join('.+')}$`) + return new RegExp(`^${parts.map(escape).join('[^.]+')}$`) } /** Every key reference in one file's source. */ @@ -174,27 +243,55 @@ export async function readEnKeys() { export const hasKey = (enKeys, key) => enKeys.has(key) || enKeys.has(`${key}.other`) +const base = (k) => k.replace(/\.(?:zero|one|two|few|many|other)$/, '') + +/** How many en keys a pattern reaches, a plural group counted once. */ +export function countMatches(pattern, enKeys) { + const reached = new Set() + for (const k of enKeys) { + if (pattern.test(k)) reached.add(k) + else if (pattern.test(base(k))) reached.add(base(k)) + } + return reached.size +} + /** * The verdict over a scan: literal keys en lacks, template keys no en key - * matches (unless allowed), allow-list entries nothing uses any more, and - * the en keys nothing reaches. + * matches, templates too wide to pass without an allow-list entry, + * allow-list entries that no file uses any more or that the check no longer + * needs, and the en keys nothing reaches. */ export function evaluate(scan, enKeys, allowed = DYNAMIC_ALLOWED) { const missing = scan.literal.filter(({ key }) => !hasKey(enKeys, key)) - const allowedTemplates = new Set(allowed.map((a) => a.template)) - const unmatched = scan.dynamic.filter( - ({ template, pattern }) => - !allowedTemplates.has(template) && !(pattern && [...enKeys].some((k) => pattern.test(k))), - ) - const usedTemplates = new Set(scan.dynamic.map((d) => d.template)) - const stale = allowed.filter((a) => !usedTemplates.has(a.template) || a.resolves.some((k) => !hasKey(enKeys, k))) - const reached = new Set([...scan.literal.map((l) => l.key), ...allowed.flatMap((a) => a.resolves)]) + const site = (file, template) => `${file}\n${template}` + const allowedSites = new Set(allowed.map((a) => site(a.file, a.template))) + const counts = new Map() + const reach = (template, pattern) => { + if (!counts.has(template)) counts.set(template, pattern ? countMatches(pattern, enKeys) : null) + return counts.get(template) + } + const unmatched = [] + const broad = [] + for (const d of scan.dynamic) { + const n = reach(d.template, d.pattern) + const isAllowed = allowedSites.has(site(d.file, d.template)) + if (n === 0 || (n === null && !isAllowed)) unmatched.push(d) + else if (n > MAX_IMPLICIT_MATCHES && !isAllowed) broad.push({ ...d, matches: n }) + } + const usedSites = new Set(scan.dynamic.map((d) => site(d.file, d.template))) + const stale = allowed.filter((a) => { + if (!usedSites.has(site(a.file, a.template))) return true + if ((a.resolves ?? []).some((k) => !hasKey(enKeys, k))) return true + // An entry for a template narrow enough to pass by itself only lengthens the list. + const n = reach(a.template, templatePattern(a.template)) + return n !== null && n <= MAX_IMPLICIT_MATCHES + }) + const reached = new Set([...scan.literal.map((l) => l.key), ...allowed.flatMap((a) => a.resolves ?? [])]) const patterns = scan.dynamic.map((d) => d.pattern).filter(Boolean) - const base = (k) => k.replace(/\.(?:zero|one|two|few|many|other)$/, '') const unused = [...enKeys].filter( (k) => !reached.has(k) && !reached.has(base(k)) && !patterns.some((p) => p.test(k) || p.test(base(k))), ) - return { missing, unmatched, stale, unused } + return { missing, unmatched, broad, stale, unused } } export function scanTree(root = SRC) { @@ -216,15 +313,24 @@ if (isCli) { try { const scan = scanTree() const enKeys = await readEnKeys() - const { missing, unmatched, stale, unused } = evaluate(scan, enKeys) + const { missing, unmatched, broad, stale, unused } = evaluate(scan, enKeys) for (const { file, line, key } of missing) { console.error(`FAIL ${file}:${line}: '${key}' is not a key in shared/src/i18n/en`) } for (const { file, line, template } of unmatched) { console.error(`FAIL ${file}:${line}: \`${template}\` matches no key in shared/src/i18n/en`) } - for (const { template } of stale) { - console.error(`FAIL DYNAMIC_ALLOWED lists \`${template}\`, which no file uses any more: remove the entry`) + for (const { file, line, template, matches } of broad) { + console.error( + `FAIL ${file}:${line}: \`${template}\` reaches ${matches} en keys, more than ${MAX_IMPLICIT_MATCHES}: ` + + 'narrow the prefix, or add a DYNAMIC_ALLOWED entry saying what bounds the value', + ) + } + for (const { file, template } of stale) { + console.error( + `FAIL DYNAMIC_ALLOWED lists \`${template}\` in ${file}, which that file no longer uses, whose keys en ` + + 'lacks, or which is narrow enough to pass without it: remove or fix the entry', + ) } if (process.argv.includes('--unused')) for (const key of unused.sort()) console.log(`unused ${key}`) console.log( @@ -232,7 +338,7 @@ if (isCli) { `reference(s) checked against ${enKeys.size} en keys; ${unused.length} en key(s) reached by neither ` + '(information, --unused lists them)', ) - if (missing.length || unmatched.length || stale.length) process.exit(1) + if (missing.length || unmatched.length || broad.length || stale.length) process.exit(1) } catch (err) { console.error(`FAIL ${err.message}`) process.exit(1) diff --git a/client/tests/unit/i18n/i18nKeysScript.test.ts b/client/tests/unit/i18n/i18nKeysScript.test.ts index b240d3180..2cd17348c 100644 --- a/client/tests/unit/i18n/i18nKeysScript.test.ts +++ b/client/tests/unit/i18n/i18nKeysScript.test.ts @@ -4,6 +4,8 @@ import { join } from 'node:path' import { describe, expect, it } from 'vitest' import { DYNAMIC_ALLOWED, + MAX_IMPLICIT_MATCHES, + countMatches, evaluate, firstArgument, readEnKeys, @@ -12,7 +14,7 @@ import { templatePattern, } from '../../../scripts/i18n-keys.mjs' -// FE-I18N-KEYS-001 to FE-I18N-KEYS-010: the client key check (scripts/i18n-keys.mjs). +// FE-I18N-KEYS-001 to FE-I18N-KEYS-013: the client key check (scripts/i18n-keys.mjs). const EN = new Set(['budget.title', 'places.count', 'places.count.one', 'trips.total.other', 'trips.total.one', 'costs.filter.all']) @@ -52,6 +54,8 @@ describe('i18n key check', () => { expect(dynamic.map((d) => d.template)).toEqual(['costs.filter.${f}', 'costs.filter.${…}', 'help.guide.${id}.title']) expect(templatePattern('costs.filter.${f}')!.test('costs.filter.all')).toBe(true) expect(templatePattern('${opt.label}Hint')).toBeNull() + // An interpolation is one key segment: costs.filter.${f} does not reach a nested costs.filter.x.y. + expect(templatePattern('costs.filter.${f}')!.test('costs.filter.x.y')).toBe(false) }) it('FE-I18N-KEYS-004: fails a literal key en lacks and accepts a plural group by its general form', () => { @@ -63,7 +67,7 @@ describe('i18n key check', () => { it('FE-I18N-KEYS-005: fails a template no en key matches unless it is allowed with the keys it resolves to', () => { const scan = scanSource('t(`costs.filter.${f}`); t(`gone.prefix.${x}`); t(`${opt.label}Hint`)') expect(evaluate(scan, EN, []).unmatched.map((u) => u.template)).toEqual(['gone.prefix.${x}', '${opt.label}Hint']) - const allowed = [{ template: '${opt.label}Hint', because: 'test', resolves: ['budget.title'] }] + const allowed = [{ file: '', template: '${opt.label}Hint', because: 'test', resolves: ['budget.title'] }] const result = evaluate(scan, EN, allowed) expect(result.unmatched.map((u) => u.template)).toEqual(['gone.prefix.${x}']) expect(result.stale).toEqual([]) @@ -71,10 +75,13 @@ describe('i18n key check', () => { it('FE-I18N-KEYS-006: refuses an allow-list entry nothing uses or whose keys en lacks', () => { const scan = scanSource('t(`${opt.label}Hint`)') - const unused = [{ template: '${gone}', because: 'test', resolves: ['budget.title'] }] + const unused = [{ file: '', template: '${gone}', because: 'test', resolves: ['budget.title'] }] expect(evaluate(scan, EN, unused).stale).toEqual(unused) - const wrong = [{ template: '${opt.label}Hint', because: 'test', resolves: ['share.nope'] }] + const wrong = [{ file: '', template: '${opt.label}Hint', because: 'test', resolves: ['share.nope'] }] expect(evaluate(scan, EN, wrong).stale).toEqual(wrong) + const elsewhere = [{ file: 'other.tsx', template: '${opt.label}Hint', because: 'test', resolves: ['budget.title'] }] + expect(evaluate(scan, EN, elsewhere).stale).toEqual(elsewhere) + expect(evaluate(scan, EN, elsewhere).unmatched.map((u) => u.template)).toEqual(['${opt.label}Hint']) }) it('FE-I18N-KEYS-007: reports en keys neither a literal nor a pattern reaches', () => { @@ -82,10 +89,11 @@ describe('i18n key check', () => { expect(evaluate(scan, EN, []).unused).toEqual(['trips.total.other', 'trips.total.one']) }) - it('FE-I18N-KEYS-008: every allow-list entry carries a reason and the keys it resolves to', () => { + it('FE-I18N-KEYS-008: every allow-list entry names its file and a reason, and a prefix-less one its keys', () => { for (const entry of DYNAMIC_ALLOWED) { + expect(entry.file).toMatch(/\.tsx?$/) expect(entry.because.length).toBeGreaterThan(20) - expect(entry.resolves.length).toBeGreaterThan(0) + if (templatePattern(entry.template) === null) expect('resolves' in entry && entry.resolves.length).toBeGreaterThan(0) } }) @@ -101,9 +109,37 @@ describe('i18n key check', () => { it('FE-I18N-KEYS-010: the client source names no key en lacks today', async () => { const enKeys = await readEnKeys() - const { missing, unmatched, stale } = evaluate(scanTree(), enKeys) + const { missing, unmatched, broad, stale } = evaluate(scanTree(), enKeys) expect(missing).toEqual([]) expect(unmatched).toEqual([]) + expect(broad).toEqual([]) expect(stale).toEqual([]) }) + + // A table of en keys wider than the implicit bound: wide.k0 to wide.k. + const WIDE = new Set([...EN, ...Array.from({ length: MAX_IMPLICIT_MATCHES + 1 }, (_, i) => `wide.k${i}`)]) + + it('FE-I18N-KEYS-011: counts a plural group once when measuring what a template reaches', () => { + expect(countMatches(templatePattern('trips.${x}')!, EN)).toBe(1) + expect(countMatches(templatePattern('wide.${x}')!, WIDE)).toBe(MAX_IMPLICIT_MATCHES + 1) + }) + + it('FE-I18N-KEYS-012: fails a template wider than the bound unless its file has an entry for it', () => { + const scan = scanSource('t(`wide.${x}`); t(`costs.filter.${f}`)', 'a.tsx') + const { broad, unmatched } = evaluate(scan, WIDE, []) + expect(broad.map((b) => [b.template, b.matches])).toEqual([['wide.${x}', MAX_IMPLICIT_MATCHES + 1]]) + expect(unmatched).toEqual([]) + const otherFile = [{ file: 'b.tsx', template: 'wide.${x}', because: 'test' }] + expect(evaluate(scan, WIDE, otherFile).broad).toHaveLength(1) + const ownFile = [{ file: 'a.tsx', template: 'wide.${x}', because: 'test' }] + expect(evaluate(scan, WIDE, ownFile)).toMatchObject({ broad: [], stale: [] }) + }) + + it('FE-I18N-KEYS-013: refuses an entry for a narrow template, and an allowed template that matches nothing', () => { + const scan = scanSource('t(`costs.filter.${f}`); t(`gone.prefix.${x}`)', 'a.tsx') + const narrow = [{ file: 'a.tsx', template: 'costs.filter.${f}', because: 'test' }] + expect(evaluate(scan, EN, narrow).stale).toEqual(narrow) + const gone = [{ file: 'a.tsx', template: 'gone.prefix.${x}', because: 'test' }] + expect(evaluate(scan, EN, gone).unmatched.map((u) => u.template)).toEqual(['gone.prefix.${x}']) + }) })