feat(i18n): ratchet new code to require t()/<Trans> everywhere (#2105)
Co-authored-by: Kandev Agent <agent@kandev.dev>
This commit is contained in:
João Salavisa
@@ -155,6 +155,34 @@ jobs:
|
||||
- name: Run i18n checks
|
||||
run: pnpm --filter @kandev/web i18n:check
|
||||
|
||||
# New-code ratchet. The eslint literal guard is an error only on migrated
|
||||
# paths, which leaves new files under un-migrated directories unguarded.
|
||||
# These two judge the CHANGE instead of the FILE: an added file must be
|
||||
# clean outright, a modified file only on the lines it touched, and the
|
||||
# guard allowlist may not shrink. Same model as
|
||||
# `golangci-lint --new-from-rev` for Go. Base SHA is resolved the same way
|
||||
# as the Prettier step above.
|
||||
- name: Run i18n new-code ratchet
|
||||
shell: bash
|
||||
# The job defaults to `apps`; these scripts live one level deeper.
|
||||
working-directory: apps/web
|
||||
run: |
|
||||
set -euo pipefail
|
||||
|
||||
if [[ "${{ github.event_name }}" == "pull_request" ]]; then
|
||||
BASE_SHA="${{ github.event.pull_request.base.sha }}"
|
||||
else
|
||||
BASE_SHA="${{ github.event.before }}"
|
||||
fi
|
||||
|
||||
if [[ -z "${BASE_SHA}" || "${BASE_SHA}" == "0000000000000000000000000000000000000000" ]]; then
|
||||
echo "No usable base SHA; skipping the new-code ratchet."
|
||||
exit 0
|
||||
fi
|
||||
|
||||
node scripts/check-new-code-i18n.mjs --base "${BASE_SHA}"
|
||||
node scripts/check-guard-allowlist.mjs --base "${BASE_SHA}"
|
||||
|
||||
- name: Run tests
|
||||
run: pnpm --filter @kandev/web test
|
||||
|
||||
|
||||
@@ -51,3 +51,14 @@ repos:
|
||||
language: system
|
||||
files: ^apps/web/.*\.(ts|tsx|js|jsx)$
|
||||
pass_filenames: true
|
||||
|
||||
# New user-facing copy must go through t()/<Trans>, even in a directory the
|
||||
# i18n migration has not reached. Judges the change, not the file: added
|
||||
# files whole, modified files only on touched lines — so it never asks you
|
||||
# to migrate code you did not write. Mirrors the go-lint hook above.
|
||||
- id: i18n-new-code
|
||||
name: i18n guard (new code only)
|
||||
entry: bash -c 'unset GIT_DIR GIT_WORK_TREE; cd apps/web && node scripts/check-new-code-i18n.mjs && node scripts/check-guard-allowlist.mjs'
|
||||
language: system
|
||||
files: ^apps/web/(components|app|hooks|lib|src)/.*\.tsx?$|^apps/web/eslint\.i18n\.options\.mjs$
|
||||
pass_filenames: false
|
||||
|
||||
@@ -74,12 +74,17 @@ The web UI is localized with i18next (`namespace:key`). **All new user-facing co
|
||||
must go through `t()` / `<Trans>`** — never a hardcoded literal — regardless of
|
||||
which directory you are in.
|
||||
|
||||
The migration of existing strings is in progress, **one directory per PR**. The
|
||||
lint guard (`i18next/no-literal-string`) is an error, but only for the paths in
|
||||
`i18nGuardFiles` (`apps/web/eslint.i18n.options.mjs`), so unrelated PRs are never
|
||||
blocked by strings in code nobody has migrated. **When you migrate a directory,
|
||||
append it to that list in the same PR**; never remove an entry to make a build
|
||||
pass.
|
||||
This is enforced, not just requested. Two ratchets, both of which only tighten:
|
||||
|
||||
- **New code, everywhere** — `pnpm run i18n:ratchet` (pre-commit + CI) fails on a
|
||||
hardcoded string in a file you added, or on a line you changed, regardless of
|
||||
directory. Untouched lines are never judged, so you are never asked to migrate
|
||||
code you did not write.
|
||||
- **Migrated paths, whole file** — `i18next/no-literal-string` is a lint error for
|
||||
the paths in `i18nGuardFiles` (`apps/web/eslint.i18n.options.mjs`). The
|
||||
migration of existing strings proceeds **one directory per PR**: when you
|
||||
migrate one, append it to that list in the same PR. Never remove an entry to
|
||||
make a build pass — a check rejects that.
|
||||
|
||||
Three rules that cause silent, hard-to-find bugs when broken: never translate a
|
||||
string compared with `===` (type-to-confirm tokens become impossible to type);
|
||||
@@ -87,9 +92,11 @@ never call `t()` at module scope (it freezes at the boot locale and the
|
||||
pseudo-locale cannot detect it); never pass an English plural ending as a value —
|
||||
use `count` with `_one`/`_other`. `pnpm run i18n:check` enforces the last two.
|
||||
|
||||
A clean lint is not proof a file is done — the rule only sees plain literals in
|
||||
JSX. The pseudo-locale (Settings → General → Appearance, dev/e2e builds) is the
|
||||
completeness check. Full guide: [`docs/i18n.md`](docs/i18n.md).
|
||||
A clean lint is not proof a file is done. The rule only sees literals in JSX, and
|
||||
it **skips anything assigned to a SCREAMING_CASE identifier** — so a
|
||||
`const ROWS = [{ label: "Disk usage" }]` config table passes silently. Review
|
||||
those by eye. The pseudo-locale (Settings → General → Appearance, dev/e2e builds)
|
||||
is the completeness check. Full guide: [`docs/i18n.md`](docs/i18n.md).
|
||||
|
||||
### Knowledge
|
||||
- **Public docs:** Website-ready user documentation lives in `docs/public/**`. Use `/docs-maintainer` when a change affects CLI commands, config keys, install/deploy flows, workflows, executors, public APIs, screenshots, or user-facing terminology.
|
||||
@@ -112,7 +119,7 @@ For multiline Markdown issue or PR bodies, write the body to a file and pass it
|
||||
with the relevant `gh ... --body-file <path>` option. Do not send escaped
|
||||
newlines through `--body`; GitHub will render them literally.
|
||||
|
||||
For PR review/fixup workflows, prefer the repo helpers before manually querying GitHub/GraphQL: `scripts/pr-state --summary <PR>` for checks and unresolved-thread state, `scripts/pr-state --comment <comment_id>` for a full review-comment body, `scripts/pr-resolve list <PR>` for actionable unresolved review threads, and `scripts/pr-resolve reply <PR> <comment_id> <thread_id> --body-file <path>` to reply, resolve, and react in one call. Use `--body-file` whenever a reply has Markdown or shell-sensitive characters (especially backticks, `$`, or quotes); it prevents shell substitution from corrupting the GitHub reply.
|
||||
For PR review/fixup workflows, prefer the repo helpers before manually querying GitHub/GraphQL: `scripts/pr-state --summary <PR>` for checks and unresolved-thread state, `scripts/pr-state --comment <comment_id>` for a full review-comment body, `scripts/pr-resolve list <PR>` for actionable unresolved review threads, and `scripts/pr-resolve reply <PR> <comment_id> <thread_id> "<body>"` to reply, resolve, and react in one call.
|
||||
|
||||
When a Kandev system message references an MCP tool that is not visible in the active tool list, use the runtime's tool discovery mechanism, such as `tool_search` when available, before falling back to a less specific workflow. Some task messaging and platform helpers are exposed on demand.
|
||||
|
||||
|
||||
+1
-21
@@ -275,27 +275,6 @@ Every long-running goroutine must have a single owner with explicit start and st
|
||||
|
||||
You may still list the column in the `CREATE TABLE` so fresh DBs get it inline, but the migration is the source of truth for evolution and must stand alone. New columns also need: the struct field in `models/`, the DTO field + `ToAPI` in `pkg/api/v1/`, and every `CreateX`/`UpdateX`/bulk write in the repo that should set it.
|
||||
|
||||
## Internationalization
|
||||
|
||||
`internal/i18n` covers only what Go renders **directly to a browser**, which today
|
||||
is just the SPA-unavailable error pages. Everything else stays English by design —
|
||||
the ~1,100 `http.Error` / `gin.H{"error": …}` strings are diagnostics the SPA maps
|
||||
onto its own translated copy, and log lines, agent/ACP output, and CLI output are
|
||||
not display copy. The shared-task artifacts (`share.html`, gist README and
|
||||
description) are server-rendered and in scope for localization, but not done yet.
|
||||
|
||||
- `i18n.T(locale, key)` for lookup, `i18n.FromRequest(r)` at the HTTP boundary.
|
||||
- The package has no placeholder interpolation and no context-carried locale.
|
||||
Both are needed for the share artifacts; add them with that work rather than
|
||||
ahead of it.
|
||||
- Catalogs are embedded JSON (`internal/i18n/locales/<locale>.json`). `pseudo` is
|
||||
**generated** — run `pnpm run i18n:pseudo` from `apps/web`, which writes both
|
||||
the frontend and backend catalogs. A per-package test fails if an `en` key is
|
||||
missing from `pseudo`.
|
||||
- **For new user-facing output, prefer returning a stable error code** the
|
||||
frontend translates. Reach for `i18n.T` only when Go itself writes the bytes a
|
||||
user reads.
|
||||
|
||||
## Code-quality limits
|
||||
|
||||
Enforced by `apps/backend/.golangci.yml` (errors on new code only):
|
||||
@@ -317,4 +296,5 @@ golangci-lint run ./... --new-from-rev="<base-sha>" --timeout=5m
|
||||
- `internal/agentctl/AGENTS.md` — agentctl server route groups, adapter model, ACP protocol
|
||||
- `internal/agentctl/server/api/AGENTS.md` — reverse-proxy body rewriting (`Accept-Encoding`), iframe-blocking header stripping
|
||||
- `internal/integrations/AGENTS.md` — playbook for adding a new third-party integration (Jira/Linear pattern)
|
||||
- `docs/i18n.md` ("Backend") — `internal/i18n` covers only what Go renders straight to a browser (today: the SPA-unavailable error pages). Everything else stays English by design; for new user-facing output prefer a stable error code the frontend translates.
|
||||
- `cmd/mock-agent/AGENTS.md` — predefined `/e2e:<name>` scenarios vs inline `e2e:...` scripts, recipe for adding a scenario, and the rebuild-before-e2e requirement
|
||||
|
||||
+10
-20
@@ -26,7 +26,6 @@ import { Dialog } from "@kandev/ui/dialog";
|
||||
- Use `useTouchDrawer` when a hover/popover disclosure needs a coarse-pointer `Drawer` alternative. Width-based phone composition and pointer-based disclosure behavior are related but not interchangeable.
|
||||
- Existing Radix DropdownMenu and ContextMenu surfaces receive inset, safe-area-aware bottom-sheet treatment below 640px in `app/globals.css`. Reuse those primitives for contextual actions and add focused coverage for long or nested menus instead of creating a parallel mobile menu.
|
||||
- Mobile capability parity does not require desktop layout parity. Load `/mobile-parity` for the Kandev surface decision guide, mobile design contract, and verification requirements.
|
||||
- For container-query layouts, test just below and at the breakpoint plus the component's minimum supported width; preserve action order and prove clipped controls remain hit-target reachable.
|
||||
|
||||
## Data Flow Pattern (Critical)
|
||||
|
||||
@@ -69,19 +68,6 @@ lib/api/domains/ # API clients
|
||||
|
||||
**Hydration:** Go injects `window.__KANDEV_BOOT_PAYLOAD__` into the SPA shell before React mounts. `lib/state/hydration/merge-strategies.ts` has `deepMerge()`, `mergeSessionMap()`, `mergeLoadingState()` to avoid overwriting live client state. Pass `activeSessionId` to protect active sessions.
|
||||
|
||||
**Same-resource refreshes:** Preserve an already-loaded snapshot while
|
||||
refreshing the same workspace or resource. Surface the refresh separately for
|
||||
the local spinner, busy control, or disabled action; clear data only for the
|
||||
initial load or an identity change. A deferred hook test should show stale
|
||||
content with `loading=true`, and desktop/mobile E2E should keep the content
|
||||
visible during refresh.
|
||||
|
||||
**Live update provenance:** A WebSocket snapshot may omit optional fields that
|
||||
the boot payload or hydrated store already established. Merge those fields in
|
||||
provenance order—live payload, persisted session metadata, then existing store
|
||||
value—rather than replacing known immutable metadata with `undefined`. Add a
|
||||
handler regression test for an omitted-field update.
|
||||
|
||||
For rebasing or finishing PRs written against the old Next.js runtime, follow
|
||||
[`docs/nextjs-spa-migration.md`](../../docs/nextjs-spa-migration.md).
|
||||
|
||||
@@ -233,9 +219,16 @@ drifting back. Never delete an entry to make a build pass. Use
|
||||
`pnpm run lint:i18n <path>` to preview the guard on a path that is not on the
|
||||
list yet.
|
||||
|
||||
The rule also **only sees plain literals in JSX** — template literals,
|
||||
`confirm()` arguments and copy in plain `.ts` helpers are invisible to it, so a
|
||||
clean lint is not proof a file is done. `pnpm run i18n:check` gates key/catalog
|
||||
Separately, `pnpm run i18n:ratchet` (pre-commit + CI) guards **new code
|
||||
everywhere**, regardless of the allowlist: a file you added must be clean outright,
|
||||
and a file you modified is judged on the lines you touched. Untouched literals are
|
||||
never reported, so it cannot ask you to migrate code you did not write — the same
|
||||
contract as `golangci-lint --new-from-rev` for Go.
|
||||
|
||||
The rule **only sees literals in JSX** — `confirm()` arguments and copy in plain
|
||||
`.ts` helpers are invisible to it — and it **skips anything assigned to a
|
||||
SCREAMING_CASE identifier**, so `const ROWS = [{ label: "Disk usage" }]` passes
|
||||
silently. A clean lint is not proof a file is done. `pnpm run i18n:check` gates key/catalog
|
||||
drift, `<Trans>` tag indices, inline plurals and module-scope `t()`, and the
|
||||
**pseudo-locale** (Settings → General → Appearance, dev/e2e) is the completeness
|
||||
check — any plain-English text under it was never externalized. The tooling needs
|
||||
@@ -273,6 +266,3 @@ When you hit a limit, extract a helper function, custom hook, or sub-component.
|
||||
solely because `mouseenter` or `pointerMove` failed in jsdom.
|
||||
- In Playwright tests, avoid strict locators that assume only one `terminal-panel` or `.xterm` exists. Mobile and dockview layouts can mount multiple terminal instances; scope to the active panel or use `.first()` / `.last()` deliberately with a comment or helper.
|
||||
- Shared E2E helpers that inspect mounted React/DOM internals must be scoped to the active panel/container, not global selectors, because hidden or stale mounted panels can coexist in dock/mobile layouts.
|
||||
- When seeded data can create multiple tasks or repositories, scope actions to
|
||||
the seeded/API ID or an exact labelled container. Do not use `.first()` on a
|
||||
generic action locator unless list order is the tested contract.
|
||||
|
||||
@@ -14,7 +14,12 @@ export const noLiteralStringOptions = {
|
||||
// now checked, so the `words.exclude` list below has to carry the
|
||||
// weight of separating copy from prop enum values.
|
||||
mode: "jsx-only",
|
||||
"should-validate-template": false,
|
||||
// Template literals in JSX ARE copy: `Select task ${title}`, `${label} tasks,
|
||||
// over WIP limit`. Leaving them unchecked was the largest hole in the guard, and
|
||||
// turning it on measured cheap — +2 to +5 findings per un-migrated directory,
|
||||
// every one of them a real string, and zero new findings across the allowlist
|
||||
// (className templates are already covered by the Tailwind patterns below).
|
||||
"should-validate-template": true,
|
||||
// Brand/proper nouns and symbol-only strings are not translatable copy.
|
||||
//
|
||||
// NOTE: the plugin wraps each pattern as `^<pattern>$`
|
||||
@@ -62,6 +67,14 @@ export const noLiteralStringOptions = {
|
||||
".*(calc|env|url|var)\\(.*",
|
||||
"\\{.*\\}",
|
||||
"[A-Z][A-Z0-9_]*\\.[a-z]{2,4}",
|
||||
// Static chunks of an interpolated DOM id. With
|
||||
// `should-validate-template` on, the plugin checks each literal chunk of a
|
||||
// template separately, so `startup-page-${v}-label` arrives as
|
||||
// "startup-page-" and "-label" — kebab/snake fragments with a dangling
|
||||
// separator, which the token patterns above do not cover. Copy chunks
|
||||
// ("Select task ", " tasks, over WIP limit") carry a capital, a space or
|
||||
// punctuation and so still get flagged.
|
||||
"[a-z0-9]+(?:[-_][a-z0-9]*)*|[-_][a-z0-9]+(?:[-_][a-z0-9]*)*",
|
||||
// Single lowercase/camel/kebab tokens are prop enum values,
|
||||
// classnames, and identifiers (variant="ghost", side="top",
|
||||
// value="work-items") — never display copy, which is capitalized
|
||||
|
||||
@@ -24,7 +24,8 @@
|
||||
"e2e:capture": "cross-env CAPTURE_PR_ASSETS=true playwright test --config e2e/playwright.config.ts && tsx e2e/scripts/convert-assets.ts",
|
||||
"licenses:gen": "tsx scripts/generate-licenses.ts",
|
||||
"i18n:pseudo": "node scripts/generate-pseudo-locale.mjs",
|
||||
"i18n:check": "node scripts/check-i18n-keys.mjs && node scripts/check-trans-indices.mjs && node scripts/check-inline-plurals.mjs && node scripts/check-module-scope-t.mjs"
|
||||
"i18n:check": "node scripts/check-i18n-keys.mjs && node scripts/check-trans-indices.mjs && node scripts/check-inline-plurals.mjs && node scripts/check-module-scope-t.mjs",
|
||||
"i18n:ratchet": "node scripts/run-i18n-ratchet.mjs"
|
||||
},
|
||||
"dependencies": {
|
||||
"@base-ui/react": "^1.0.0",
|
||||
|
||||
@@ -0,0 +1,99 @@
|
||||
#!/usr/bin/env node
|
||||
/**
|
||||
* The i18n guard allowlist may only grow.
|
||||
*
|
||||
* `i18nGuardFiles` in eslint.i18n.options.mjs is what makes migrated code stay
|
||||
* migrated: once a path is listed, `i18next/no-literal-string` is an error there
|
||||
* forever. That protection has one obvious failure mode — someone hits the error,
|
||||
* deletes the entry, and the build goes green with the migration silently undone.
|
||||
* docs/i18n.md says never to do that; this makes it mechanical.
|
||||
*
|
||||
* Removing an entry is legitimate exactly once: when a file is deleted or renamed.
|
||||
* Both are detectable, so the check only complains when the path still exists.
|
||||
*
|
||||
* Usage: node scripts/check-guard-allowlist.mjs [--base <ref-or-sha>]
|
||||
*/
|
||||
import fs from "node:fs";
|
||||
import os from "node:os";
|
||||
import path from "node:path";
|
||||
|
||||
import { git, REPO_ROOT, resolveBase, WEB_DIR } from "./lib/git-base.mjs";
|
||||
|
||||
const OPTIONS_PATH = "apps/web/eslint.i18n.options.mjs";
|
||||
|
||||
const resolved = resolveBase();
|
||||
if (resolved.skip) {
|
||||
console.log(`⚠ guard allowlist check skipped — ${resolved.skip}`);
|
||||
process.exit(0);
|
||||
}
|
||||
const { base } = resolved;
|
||||
|
||||
let baseSource;
|
||||
try {
|
||||
// stderr silenced: "exists on disk, but not in <rev>" is the expected result
|
||||
// for any branch forked before the options module landed, and printing a
|
||||
// `fatal:` line there reads as a failure in CI logs when it is not one.
|
||||
baseSource = git(["show", `${base}:${OPTIONS_PATH}`], { quiet: true });
|
||||
} catch {
|
||||
// The options module does not exist at the base commit, so nothing can have
|
||||
// been removed from it. True for any branch forked before i18n landed.
|
||||
console.log("✓ guard allowlist — no baseline to compare (options module is new)");
|
||||
process.exit(0);
|
||||
}
|
||||
|
||||
/**
|
||||
* The options module is a dependency-free set of exports, so it imports cleanly
|
||||
* from a temp path. Parsing the array out with a regex would break the moment
|
||||
* someone reformats it.
|
||||
*/
|
||||
async function guardFilesFrom(source) {
|
||||
const file = path.join(
|
||||
fs.mkdtempSync(path.join(os.tmpdir(), "kandev-i18n-guard-")),
|
||||
"options.mjs",
|
||||
);
|
||||
fs.writeFileSync(file, source);
|
||||
try {
|
||||
const module = await import(`file://${file}`);
|
||||
return module.i18nGuardFiles ?? [];
|
||||
} finally {
|
||||
fs.rmSync(path.dirname(file), { recursive: true, force: true });
|
||||
}
|
||||
}
|
||||
|
||||
const before = await guardFilesFrom(baseSource);
|
||||
const after = await guardFilesFrom(fs.readFileSync(path.join(REPO_ROOT, OPTIONS_PATH), "utf8"));
|
||||
|
||||
const afterSet = new Set(after);
|
||||
/**
|
||||
* An entry may be removed once its path is genuinely gone — a deleted or renamed
|
||||
* file, or a migrated directory that was later deleted wholesale. Only a removal
|
||||
* whose target still EXISTS un-protects live code, so that is all we flag.
|
||||
*
|
||||
* Globs are resolved rather than assumed live: flagging every removed glob would
|
||||
* block the legitimate flow of migrating `components/foo/**`, listing it, then
|
||||
* deleting `foo/` entirely.
|
||||
*/
|
||||
const removed = before.filter((entry) => {
|
||||
if (afterSet.has(entry)) return false;
|
||||
if (/[*?[\]{}]/.test(entry)) {
|
||||
return fs.globSync(entry, { cwd: WEB_DIR }).length > 0;
|
||||
}
|
||||
return fs.existsSync(path.join(WEB_DIR, entry));
|
||||
});
|
||||
|
||||
if (removed.length > 0) {
|
||||
console.error(
|
||||
`\n✖ ${removed.length} entr(ies) removed from i18nGuardFiles:\n\n` +
|
||||
removed.map((entry) => ` ${entry}`).join("\n") +
|
||||
`\n\n Those paths are still present, so removing them un-protects copy that` +
|
||||
`\n was already migrated. If a literal there is failing lint, externalize it` +
|
||||
`\n — do not drop the entry. See docs/i18n.md ("The guard is scoped").\n`,
|
||||
);
|
||||
process.exit(1);
|
||||
}
|
||||
|
||||
const added = after.filter((entry) => !new Set(before).has(entry));
|
||||
console.log(
|
||||
`✓ guard allowlist intact — ${after.length} entr(ies)` +
|
||||
(added.length ? `, ${added.length} added` : ""),
|
||||
);
|
||||
@@ -0,0 +1,129 @@
|
||||
#!/usr/bin/env node
|
||||
/**
|
||||
* Ratchet: NEW user-facing copy must go through `t()` / `<Trans>`, everywhere.
|
||||
*
|
||||
* The eslint guard in eslint.config.mjs is an error only on `i18nGuardFiles` —
|
||||
* paths already migrated — because a repo-wide error on a half-migrated codebase
|
||||
* breaks every unrelated PR that adds a label. That leaves a hole this closes:
|
||||
* a brand-new component under an un-migrated directory is currently unguarded.
|
||||
*
|
||||
* The fix is to judge the CHANGE rather than the FILE:
|
||||
*
|
||||
* - a file the change ADDED must be clean outright. It carries no legacy debt,
|
||||
* so requiring zero literals costs nothing.
|
||||
* - a file the change MODIFIED is judged only on the lines it touched. Existing
|
||||
* literals elsewhere in the file are somebody else's migration, not this PR's
|
||||
* problem — which is exactly why this can be repo-wide without a treadmill.
|
||||
*
|
||||
* This mirrors `golangci-lint run --new-from-rev` in .pre-commit-config.yaml, so
|
||||
* it is the same contract the Go side already enforces.
|
||||
*
|
||||
* NOTE: this raises the floor, it does not seal the box. The rule only sees plain
|
||||
* literals and template literals in JSX — `confirm()` arguments and copy returned
|
||||
* from plain `.ts` helpers stay invisible. The pseudo-locale remains the
|
||||
* completeness check (docs/i18n.md).
|
||||
*
|
||||
* Usage:
|
||||
* node scripts/check-new-code-i18n.mjs [--base <ref-or-sha>]
|
||||
*
|
||||
* With no --base it uses `git merge-base HEAD origin/main`. CI passes the PR's
|
||||
* base SHA explicitly, matching the Prettier step in frontend-tests.yml.
|
||||
*/
|
||||
import path from "node:path";
|
||||
|
||||
import { ESLint } from "eslint";
|
||||
|
||||
import { lineInRanges, parseAddedLineRanges } from "./lib/diff-ranges.mjs";
|
||||
import { git, REPO_ROOT, resolveBase, toPosixPath, WEB_DIR } from "./lib/git-base.mjs";
|
||||
|
||||
const WEB_PREFIX = "apps/web/";
|
||||
|
||||
/** Only UI source. Tests build fixtures out of literals on purpose. */
|
||||
function isCandidate(repoPath) {
|
||||
if (!repoPath.startsWith(WEB_PREFIX)) return false;
|
||||
const rel = repoPath.slice(WEB_PREFIX.length);
|
||||
if (!/\.tsx?$/.test(rel)) return false;
|
||||
if (/\.(test|spec)\.tsx?$/.test(rel)) return false;
|
||||
if (rel.startsWith("e2e/") || rel.startsWith("scripts/")) return false;
|
||||
return /^(components|app|hooks|lib|src)\//.test(rel);
|
||||
}
|
||||
|
||||
function changedFiles(base, filter) {
|
||||
// --find-renames explicitly: rename detection defaults on since Git 2.9, but a
|
||||
// repo with diff.renames=false would report a moved file as ADDED, and an
|
||||
// added file is judged whole — demanding a full migration for a plain git mv.
|
||||
return git(["diff", "--name-only", "--find-renames", `--diff-filter=${filter}`, base])
|
||||
.split("\n")
|
||||
.map((line) => line.trim())
|
||||
.filter(isCandidate);
|
||||
}
|
||||
|
||||
const resolved = resolveBase();
|
||||
if (resolved.skip) {
|
||||
console.log(`⚠ i18n new-code ratchet skipped — ${resolved.skip}`);
|
||||
process.exit(0);
|
||||
}
|
||||
const { base } = resolved;
|
||||
|
||||
const added = changedFiles(base, "A");
|
||||
// Renames and copies are judged like modifications, NOT like additions: moving a
|
||||
// legacy file must not suddenly demand that its whole contents be migrated. A
|
||||
// pure rename adds no lines and so reports nothing; a rename-plus-edit is judged
|
||||
// on the edited lines, same as any other modification.
|
||||
const modified = changedFiles(base, "MRC");
|
||||
const targets = [...added, ...modified];
|
||||
|
||||
if (targets.length === 0) {
|
||||
console.log("✓ i18n new-code ratchet — no UI source added or modified");
|
||||
process.exit(0);
|
||||
}
|
||||
|
||||
// Line attribution only matters for modified files; added files are judged whole.
|
||||
//
|
||||
// Scoped to apps/web rather than to the individual files, because narrowing the
|
||||
// pathspec to only the NEW path of a rename hides the matching deletion and git
|
||||
// then reports the whole file as added — which would demand a full migration for
|
||||
// a pure `git mv`. Keeping both sides in scope lets rename detection pair them,
|
||||
// so a pure rename yields no hunks and reports nothing.
|
||||
const addedRanges =
|
||||
modified.length === 0
|
||||
? new Map()
|
||||
: parseAddedLineRanges(git(["diff", "--unified=0", "--find-renames", base, "--", WEB_PREFIX]));
|
||||
|
||||
const eslint = new ESLint({
|
||||
cwd: WEB_DIR,
|
||||
overrideConfigFile: path.join(WEB_DIR, "eslint.i18n.config.mjs"),
|
||||
});
|
||||
const results = await eslint.lintFiles(targets.map((f) => path.join(REPO_ROOT, f)));
|
||||
|
||||
const addedSet = new Set(added);
|
||||
const violations = [];
|
||||
for (const result of results) {
|
||||
const repoPath = toPosixPath(path.relative(REPO_ROOT, result.filePath));
|
||||
const whole = addedSet.has(repoPath);
|
||||
const ranges = addedRanges.get(repoPath);
|
||||
for (const message of result.messages) {
|
||||
if (message.ruleId !== "i18next/no-literal-string") continue;
|
||||
if (!whole && !lineInRanges(ranges, message.line)) continue;
|
||||
violations.push({ repoPath, whole, line: message.line, text: message.message });
|
||||
}
|
||||
}
|
||||
|
||||
if (violations.length === 0) {
|
||||
console.log(
|
||||
`✓ i18n new-code ratchet — ${added.length} added + ${modified.length} modified file(s) clean`,
|
||||
);
|
||||
process.exit(0);
|
||||
}
|
||||
|
||||
console.error(
|
||||
`\n✖ ${violations.length} hardcoded user-facing string(s) in new code.\n` +
|
||||
` New copy must go through t() / <Trans> even in a directory that has not\n` +
|
||||
` been migrated yet. See docs/i18n.md ("TL;DR for adding a string").\n`,
|
||||
);
|
||||
for (const violation of violations) {
|
||||
const scope = violation.whole ? "new file" : "changed line";
|
||||
console.error(` ${violation.repoPath}:${violation.line} [${scope}] ${violation.text}`);
|
||||
}
|
||||
console.error("");
|
||||
process.exit(1);
|
||||
@@ -0,0 +1,67 @@
|
||||
/**
|
||||
* Parse `git diff --unified=0` into the set of line numbers a change ADDED, so a
|
||||
* lint finding can be attributed to the change that introduced it rather than to
|
||||
* the file it happens to live in.
|
||||
*
|
||||
* This is what lets the i18n literal guard apply repo-wide to new code without
|
||||
* demanding that the surrounding legacy file be migrated first — the same model
|
||||
* as `golangci-lint --new-from-rev`, which this repo already uses for Go.
|
||||
*/
|
||||
|
||||
/** Hunk header: `@@ -oldStart[,oldCount] +newStart[,newCount] @@ optional context` */
|
||||
const HUNK = /^@@ -\d+(?:,\d+)? \+(\d+)(?:,(\d+))? @@/;
|
||||
|
||||
/**
|
||||
* Map of repo-relative path -> array of inclusive `[start, end]` line ranges that
|
||||
* the diff added, in the NEW file's numbering.
|
||||
*
|
||||
* Deleted files and pure-deletion hunks contribute nothing: there is no new line
|
||||
* to attribute a finding to.
|
||||
*
|
||||
* @param {string} diffText output of `git diff --unified=0 <base>...<head>`
|
||||
* @returns {Map<string, Array<[number, number]>>}
|
||||
*/
|
||||
export function parseAddedLineRanges(diffText) {
|
||||
const ranges = new Map();
|
||||
let current = null;
|
||||
|
||||
for (const line of diffText.split("\n")) {
|
||||
// Every file entry starts here, so reset first. Entries that emit no `+++`
|
||||
// header at all — a 100%-similarity rename, a binary file, a mode-only
|
||||
// change — would otherwise leave the PREVIOUS file selected, and any hunk
|
||||
// that followed would be attributed to it. Not reachable with today's git
|
||||
// output, but it is a one-line invariant instead of a standing assumption.
|
||||
if (line.startsWith("diff --git ")) {
|
||||
current = null;
|
||||
continue;
|
||||
}
|
||||
if (line.startsWith("+++ ")) {
|
||||
const target = line.slice(4).trim();
|
||||
// `+++ /dev/null` marks a deletion; `+++ b/path` is the new file.
|
||||
current = target === "/dev/null" ? null : target.replace(/^b\//, "");
|
||||
if (current && !ranges.has(current)) ranges.set(current, []);
|
||||
continue;
|
||||
}
|
||||
if (!current || !line.startsWith("@@")) continue;
|
||||
|
||||
const match = HUNK.exec(line);
|
||||
if (!match) continue;
|
||||
const start = Number(match[1]);
|
||||
// A missing count means exactly one line; an explicit 0 means deletion-only.
|
||||
const count = match[2] === undefined ? 1 : Number(match[2]);
|
||||
if (count > 0) ranges.get(current).push([start, start + count - 1]);
|
||||
}
|
||||
|
||||
return ranges;
|
||||
}
|
||||
|
||||
/**
|
||||
* Whether `line` falls inside any of `ranges`.
|
||||
*
|
||||
* @param {Array<[number, number]>} ranges
|
||||
* @param {number} line
|
||||
*/
|
||||
export function lineInRanges(ranges, line) {
|
||||
if (!ranges) return false;
|
||||
return ranges.some(([start, end]) => line >= start && line <= end);
|
||||
}
|
||||
@@ -0,0 +1,136 @@
|
||||
import { describe, expect, it } from "vitest";
|
||||
|
||||
import { lineInRanges, parseAddedLineRanges } from "./diff-ranges.mjs";
|
||||
|
||||
const OLD_A = "--- a/a.tsx";
|
||||
const NEW_A = "+++ b/a.tsx";
|
||||
|
||||
describe("parseAddedLineRanges", () => {
|
||||
it("reads a single-line hunk with no explicit count", () => {
|
||||
const diff = [OLD_A, NEW_A, "@@ -4 +4 @@", "-old", "+new"].join("\n");
|
||||
expect(parseAddedLineRanges(diff).get("a.tsx")).toEqual([[4, 4]]);
|
||||
});
|
||||
|
||||
it("reads a multi-line hunk", () => {
|
||||
const diff = [OLD_A, NEW_A, "@@ -10,0 +11,3 @@", "+x", "+y", "+z"].join("\n");
|
||||
expect(parseAddedLineRanges(diff).get("a.tsx")).toEqual([[11, 13]]);
|
||||
});
|
||||
|
||||
it("ignores deletion-only hunks, which add no attributable line", () => {
|
||||
const diff = [OLD_A, NEW_A, "@@ -5,2 +4,0 @@", "-gone", "-also"].join("\n");
|
||||
expect(parseAddedLineRanges(diff).get("a.tsx")).toEqual([]);
|
||||
});
|
||||
|
||||
it("ignores deleted files", () => {
|
||||
const diff = ["--- a/gone.tsx", "+++ /dev/null", "@@ -1,2 +0,0 @@", "-a", "-b"].join("\n");
|
||||
expect(parseAddedLineRanges(diff).has("gone.tsx")).toBe(false);
|
||||
});
|
||||
|
||||
it("keeps hunks separate across several files", () => {
|
||||
const diff = [
|
||||
"diff --git a/a.tsx b/a.tsx",
|
||||
OLD_A,
|
||||
NEW_A,
|
||||
"@@ -1 +1 @@",
|
||||
"+a",
|
||||
"diff --git a/b.tsx b/b.tsx",
|
||||
"--- a/b.tsx",
|
||||
"+++ b/b.tsx",
|
||||
"@@ -20,0 +21,2 @@",
|
||||
"+b",
|
||||
"+c",
|
||||
].join("\n");
|
||||
const ranges = parseAddedLineRanges(diff);
|
||||
expect(ranges.get("a.tsx")).toEqual([[1, 1]]);
|
||||
expect(ranges.get("b.tsx")).toEqual([[21, 22]]);
|
||||
});
|
||||
|
||||
it("collects multiple hunks in one file", () => {
|
||||
const diff = [OLD_A, NEW_A, "@@ -1 +1 @@", "+a", "@@ -50,0 +51,2 @@", "+b", "+c"].join("\n");
|
||||
expect(parseAddedLineRanges(diff).get("a.tsx")).toEqual([
|
||||
[1, 1],
|
||||
[51, 52],
|
||||
]);
|
||||
});
|
||||
|
||||
it("does not mistake a '+++' inside added content for a file header", () => {
|
||||
// A line of added source that begins with "+++" arrives as "++++...".
|
||||
const diff = [OLD_A, NEW_A, "@@ -1,0 +1,1 @@", "++++ not a header"].join("\n");
|
||||
const ranges = parseAddedLineRanges(diff);
|
||||
expect([...ranges.keys()]).toEqual(["a.tsx"]);
|
||||
expect(ranges.get("a.tsx")).toEqual([[1, 1]]);
|
||||
});
|
||||
|
||||
it("returns nothing for an empty diff", () => {
|
||||
expect(parseAddedLineRanges("").size).toBe(0);
|
||||
});
|
||||
|
||||
it("ignores a pure rename, which emits no headers or hunks", () => {
|
||||
// A plain `git mv` must cost nothing: attributing the moved file's existing
|
||||
// lines would demand a full migration for a file nobody edited.
|
||||
const diff = [
|
||||
"diff --git a/old.tsx b/new.tsx",
|
||||
"similarity index 100%",
|
||||
"rename from old.tsx",
|
||||
"rename to new.tsx",
|
||||
].join("\n");
|
||||
expect(parseAddedLineRanges(diff).size).toBe(0);
|
||||
});
|
||||
|
||||
it("does not attribute a later hunk to the file before a headerless entry", () => {
|
||||
// A binary entry emits no `+++`, so without resetting on `diff --git` the
|
||||
// previous file would stay selected and absorb the next hunk.
|
||||
const diff = [
|
||||
"diff --git a/a.tsx b/a.tsx",
|
||||
OLD_A,
|
||||
NEW_A,
|
||||
"@@ -1 +1 @@",
|
||||
"+a",
|
||||
"diff --git a/img.png b/img.png",
|
||||
"Binary files a/img.png and b/img.png differ",
|
||||
"@@ -99,0 +99,3 @@",
|
||||
"+not really a hunk",
|
||||
].join("\n");
|
||||
const ranges = parseAddedLineRanges(diff);
|
||||
expect(ranges.get("a.tsx")).toEqual([[1, 1]]);
|
||||
expect(ranges.has("img.png")).toBe(false);
|
||||
});
|
||||
|
||||
it("keeps a rename-plus-edit scoped to the edited lines only", () => {
|
||||
const diff = [
|
||||
"diff --git a/old.tsx b/new.tsx",
|
||||
"similarity index 92%",
|
||||
"rename from old.tsx",
|
||||
"rename to new.tsx",
|
||||
"--- a/old.tsx",
|
||||
"+++ b/new.tsx",
|
||||
"@@ -2,0 +3 @@",
|
||||
"+const added = 1;",
|
||||
].join("\n");
|
||||
expect(parseAddedLineRanges(diff).get("new.tsx")).toEqual([[3, 3]]);
|
||||
});
|
||||
});
|
||||
|
||||
describe("lineInRanges", () => {
|
||||
const ranges: Array<[number, number]> = [
|
||||
[3, 5],
|
||||
[10, 10],
|
||||
];
|
||||
|
||||
it("matches inclusive bounds", () => {
|
||||
expect(lineInRanges(ranges, 3)).toBe(true);
|
||||
expect(lineInRanges(ranges, 5)).toBe(true);
|
||||
expect(lineInRanges(ranges, 10)).toBe(true);
|
||||
});
|
||||
|
||||
it("rejects lines outside every range", () => {
|
||||
expect(lineInRanges(ranges, 2)).toBe(false);
|
||||
expect(lineInRanges(ranges, 6)).toBe(false);
|
||||
expect(lineInRanges(ranges, 11)).toBe(false);
|
||||
});
|
||||
|
||||
it("treats a missing range list as no match", () => {
|
||||
// Reached when a modified file produced no attributable added lines.
|
||||
expect(lineInRanges(undefined as unknown as Array<[number, number]>, 1)).toBe(false);
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,65 @@
|
||||
/**
|
||||
* Shared git plumbing for the i18n ratchet checks.
|
||||
*
|
||||
* Both `check-new-code-i18n.mjs` and `check-guard-allowlist.mjs` need the same
|
||||
* fork point and the same fail-closed policy about it, so it lives here rather
|
||||
* than being duplicated and drifting.
|
||||
*/
|
||||
import { execFileSync } from "node:child_process";
|
||||
import path from "node:path";
|
||||
|
||||
export const WEB_DIR = path.resolve(import.meta.dirname, "..", "..");
|
||||
export const REPO_ROOT = path.resolve(WEB_DIR, "..", "..");
|
||||
|
||||
export function git(args, { quiet = false } = {}) {
|
||||
return execFileSync("git", args, {
|
||||
cwd: REPO_ROOT,
|
||||
encoding: "utf8",
|
||||
maxBuffer: 64 * 1024 * 1024,
|
||||
stdio: ["ignore", "pipe", quiet ? "ignore" : "inherit"],
|
||||
});
|
||||
}
|
||||
|
||||
/**
|
||||
* Git reports paths with `/` on every platform; `path.relative` returns `\` on
|
||||
* Windows. Comparing the two silently matches nothing, which would make the
|
||||
* ratchet discard every finding and report "clean" — the worst failure mode for
|
||||
* a guard, because it looks like success.
|
||||
*/
|
||||
export function toPosixPath(value) {
|
||||
return value.split(path.sep).join("/");
|
||||
}
|
||||
|
||||
/**
|
||||
* The fork point to judge against, or `null` when the caller explicitly allows
|
||||
* running without one.
|
||||
*
|
||||
* Fail-closed: if the caller passed `--base`, or we are in CI, an unresolvable
|
||||
* base means a broken checkout (usually a shallow clone that never fetched the
|
||||
* base), and skipping would turn that into a silent pass. Only a bare local run
|
||||
* — a developer with no `origin/main`, e.g. offline or in a fresh init repo —
|
||||
* is allowed to skip, because failing there would block unrelated commits for
|
||||
* an environment reason.
|
||||
*
|
||||
* @returns {{ base: string } | { skip: string }}
|
||||
*/
|
||||
export function resolveBase(argv = process.argv) {
|
||||
const flag = argv.indexOf("--base");
|
||||
const explicit = flag !== -1 && argv[flag + 1] ? argv[flag + 1] : null;
|
||||
const requested = explicit ?? "origin/main";
|
||||
|
||||
try {
|
||||
return { base: git(["merge-base", requested, "HEAD"], { quiet: true }).trim() };
|
||||
} catch {
|
||||
if (explicit || process.env.CI) {
|
||||
console.error(
|
||||
`\n✖ cannot resolve a base to compare against (tried "${requested}").\n` +
|
||||
` In CI this means the checkout is incomplete — a shallow clone that\n` +
|
||||
` never fetched the base branch. Refusing to pass silently.\n` +
|
||||
` Fetch the base ref, or pass --base <sha>.\n`,
|
||||
);
|
||||
process.exit(1);
|
||||
}
|
||||
return { skip: `no base ref (tried "${requested}"); pass --base <sha> to check explicitly` };
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,18 @@
|
||||
#!/usr/bin/env node
|
||||
/**
|
||||
* Runs both ratchet checks, forwarding CLI args to each.
|
||||
*
|
||||
* `pnpm run i18n:ratchet --base <sha>` used to append the flag to the last token
|
||||
* of a `&&` chain, so only the second script saw it and the two disagreed about
|
||||
* which base they were judging. A tiny runner keeps them in step.
|
||||
*/
|
||||
import { spawnSync } from "node:child_process";
|
||||
import path from "node:path";
|
||||
|
||||
const args = process.argv.slice(2);
|
||||
for (const script of ["check-new-code-i18n.mjs", "check-guard-allowlist.mjs"]) {
|
||||
const result = spawnSync(process.execPath, [path.join(import.meta.dirname, script), ...args], {
|
||||
stdio: "inherit",
|
||||
});
|
||||
if (result.status !== 0) process.exit(result.status ?? 1);
|
||||
}
|
||||
+57
-7
@@ -7,9 +7,18 @@ generated **pseudo-locale** is used for QA. Requirements live in
|
||||
|
||||
The migration is **in progress, one directory per PR**. The runtime, the gates,
|
||||
and Settings → General → Appearance are done; everything else still holds
|
||||
English literals. See [Migrating a directory](#migrating-a-directory) for how to
|
||||
convert one, and [The guard is scoped](#the-guard-is-scoped-on-purpose) for why
|
||||
new code elsewhere is not blocked.
|
||||
English literals.
|
||||
|
||||
Two enforcement layers, and it matters which is which:
|
||||
|
||||
- **New code is guarded everywhere.** Copy you add — in a new file, or on a line
|
||||
you changed — must go through `t()` / `<Trans>` whatever directory it is in.
|
||||
See [the new-code ratchet](#but-new-code-is-guarded-everywhere).
|
||||
- **Whole files are guarded only once migrated.** The eslint literal rule errors
|
||||
across an entire file only for the paths in `i18nGuardFiles`, so you are never
|
||||
asked to migrate lines you did not touch. See
|
||||
[The guard is scoped](#the-guard-is-scoped-on-purpose) and
|
||||
[Migrating a directory](#migrating-a-directory).
|
||||
|
||||
## TL;DR for adding a string
|
||||
|
||||
@@ -149,6 +158,7 @@ cd apps/web
|
||||
pnpm run i18n:check # the four repo-wide gates (runs in CI: Frontend Tests)
|
||||
pnpm run i18n:pseudo # regenerate the pseudo catalog from en
|
||||
pnpm run lint:i18n <path> # preview the guard on a path that is NOT yet migrated
|
||||
pnpm run i18n:ratchet # new-code guard + allowlist-only-grows (pre-commit + CI)
|
||||
|
||||
# Codemods — always inspect the diff; each declines what it cannot prove safe.
|
||||
node scripts/externalize-strings.mjs --write components app hooks lib
|
||||
@@ -205,7 +215,29 @@ Scoping inverts it. The guard can only ever tighten:
|
||||
|
||||
Entries are globs, so use a file glob while a directory is partly migrated and
|
||||
collapse to `dir/**` once it is done. **Never remove an entry** to make a build
|
||||
pass — that silently un-protects migrated copy.
|
||||
pass — that silently un-protects migrated copy. `check-guard-allowlist.mjs`
|
||||
enforces this: it fails if an entry disappears while its path still exists.
|
||||
|
||||
### …but new code is guarded everywhere
|
||||
|
||||
Scoping the whole-file guard leaves a hole: a brand-new component under an
|
||||
un-migrated directory would be unchecked. `check-new-code-i18n.mjs` closes it by
|
||||
judging the **change** rather than the file:
|
||||
|
||||
| | Judged on |
|
||||
|---|---|
|
||||
| A file the change **added** | the whole file — it carries no legacy debt |
|
||||
| A file the change **modified** | only the lines it touched |
|
||||
|
||||
Existing literals elsewhere in a modified file are somebody else's migration, so
|
||||
this can be repo-wide without recreating the treadmill. It is the same contract
|
||||
`golangci-lint --new-from-rev` already enforces for Go in
|
||||
`.pre-commit-config.yaml`, and it runs in both pre-commit and CI.
|
||||
|
||||
So there are two independent ratchets, and both only tighten:
|
||||
|
||||
- **`i18nGuardFiles`** — whole files, migrated paths. Grows one directory per PR.
|
||||
- **the new-code ratchet** — new lines, everywhere. Always on.
|
||||
|
||||
## Migrating a directory
|
||||
|
||||
@@ -232,9 +264,27 @@ pnpm run lint:i18n components/<area> # 5. clean?
|
||||
|
||||
### The guard has blind spots
|
||||
|
||||
`i18next/no-literal-string` only inspects **plain string literals in JSX**. It
|
||||
cannot see a template literal (``aria-label={`Remove ${x}`}``), a `confirm()`
|
||||
argument, or copy returned from a plain `.ts` helper. `externalize-strings.mjs`
|
||||
`i18next/no-literal-string` only inspects **literals in JSX**. Template literals
|
||||
*are* now checked (`should-validate-template: true` — measured at +2 to +5
|
||||
findings per un-migrated directory, all of them real copy), but it still cannot
|
||||
see a `confirm()` argument or copy returned from a plain `.ts` helper.
|
||||
|
||||
It has one more blind spot worth knowing, because it is the shape config tables
|
||||
take: **a literal assigned to a SCREAMING_CASE identifier is skipped entirely.**
|
||||
|
||||
```tsx
|
||||
const HEADER = <span>Archive board</span>; // NOT flagged
|
||||
const header = <span>Archive board</span>; // flagged
|
||||
const ROWS = [{ label: "Disk usage" }]; // NOT flagged
|
||||
```
|
||||
|
||||
That is why module-scope config objects survived the sweep, and why the
|
||||
new-code ratchet inherits the same hole. `check-module-scope-t.mjs` catches the
|
||||
adjacent case (a module-scope `t()` call, frozen at the boot locale) but not
|
||||
module-scope literal copy. Reviewing a new `const TABLE = [...]` of labels by eye
|
||||
is still required.
|
||||
|
||||
`externalize-strings.mjs`
|
||||
covers those shapes — it walks `.ts` as well as `.tsx` and handles template
|
||||
literals and dialog/toast arguments — but **a zero from `pnpm lint` does not by
|
||||
itself mean a file is fully externalized**. The pseudo-locale is the real
|
||||
|
||||
Reference in New Issue
Block a user