web(i18n): one-way ceiling on isZh branching (#5519) - #5805
Conversation
The isZh migration was losing ground (12 -> 31 files over 90 days at filing; 28 today outside web/lib/i18n). Count the files that still branch on isZh outside the migration home and fail web CI if the number grows. Migrating branches lowers the count; then ratchet the CEILING. Signed-off-by: CodeWhale Bot <bot@codewhale.net>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_b1052a9b-3530-441f-9e2e-68b0d5f76d26) |
|
Claude finished @Hmbown's task in 3m 2s —— View job Code review: one-way
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
🟡 Changes recommended
The new test is currently prone to false positives and environment-dependent behavior (cwd + generated dirs), which can make the guard flaky and reduce its signal.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a Vitest regression guard intended to prevent the isZh migration from regressing by enforcing a one-way ceiling on isZh usage outside web/lib/i18n.
Changes:
- Introduces
web/lib/i18n/iszh-ceiling.test.tsthat walks theweb/tree, collects.js/.ts/.jsx/.tsxfiles, and fails if “offender” files exceed a fixed ceiling (28). - Reports offender paths in the assertion message to make regressions actionable.
File summaries
| File | Description |
|---|---|
| web/lib/i18n/iszh-ceiling.test.ts | Adds a filesystem-scanning Vitest guard to cap isZh usage outside lib/i18n. |
Review details
Suppressed comments (2)
web/lib/i18n/iszh-ceiling.test.ts:15
- The directory walk currently doesn’t exclude
.open-next,.wrangler, or.vercel(all common local build artifacts here). If they exist, this test will waste time scanning generated bundles and may become flaky if those bundles containisZh.
for (const entry of readdirSync(dir)) {
if (entry === "node_modules" || entry === ".next" || entry === ".git") continue;
const full = join(dir, entry);
web/lib/i18n/iszh-ceiling.test.ts:31
.includes("isZh")will count non-code mentions (e.g.,components/nav.tsxhas a comment that mentionsisZhwhile explicitly having noisZhbranch). Stripping comments before matching (and using a word-boundary regex) keeps this guard focused on real code usage and reduces false positives.
.filter(
(file) =>
!file.startsWith(MIGRATION_HOME) &&
!/\.test\.[jt]sx?$/.test(file) &&
readFileSync(join(ROOT, file), "utf8").includes("isZh"),
)
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| import { describe, expect, it } from "vitest"; | ||
| import { readdirSync, readFileSync, statSync } from "node:fs"; | ||
| import { join, relative } from "node:path"; | ||
|
|
||
| const ROOT = process.cwd(); | ||
| const MIGRATION_HOME = join("lib", "i18n"); |
![codewhale-agent[bot]](https://avatars.githubusercontent.com/u/101357273?s=60&v=4){kind=small}
There was a problem hiding this comment.
Codewhale review
Adds a Vitest one-way ceiling test for isZh branching outside web/lib/i18n. The approach is simple, but path resolution and text matching can make the count unstable or incorrect depending on invocation and repository layout.
Findings
- [WARNING] ROOT depends on process.cwd() (
web/lib/i18n/iszh-ceiling.test.ts:5)
The scan root isprocess.cwd(), so running the test from the repo root instead of the web package changes what is scanned. The test lives underweb/lib/i18nandMIGRATION_HOMEis relative toweb(lib/i18n), so from repo root the intended migration home is not excluded and the count is wrong. Derive ROOT from the test file location instead. - [WARNING] MIGRATION_HOME prefix check is not path-segment safe (
web/lib/i18n/iszh-ceiling.test.ts:28)
file.startsWith(MIGRATION_HOME)also excludes paths such aslib/i18n-utilsorlib/i18next, and it does not account for Windows path separators. Usefile.startsWith(MIGRATION_HOME + sep)or compare path segments. - [INFO] Text search counts any mention of "isZh", not actual branching (
web/lib/i18n/iszh-ceiling.test.ts:30)
The test reads each file and checks.includes("isZh"), so comments, strings, type names, or import references can count as branching. This may produce false positives and require ceiling adjustments for non-branching mentions. Consider using an AST or a more targeted regex for actualisZhconditional expressions if precision is desired. - [INFO] walk() only skips node_modules, .next, and .git (
web/lib/i18n/iszh-ceiling.test.ts:14)
Other generated or vendor directories such asdist,build,.turbo, orcoveragemay be walked and can contain transpiled files withisZh, causing false counts or slow runs. Add project-specific ignore patterns or use Vitest config root exclusions.
Suggestions
-
web/lib/i18n/iszh-ceiling.test.ts:3— Import the platform path separator so the migration-home prefix check is segment-safe.import { join, relative, sep } from "node:path"; -
web/lib/i18n/iszh-ceiling.test.ts:28— Compare with the platform separator so only actual children of lib/i18n are excluded and Windows paths work correctly.!file.startsWith(MIGRATION_HOME + sep) && -
web/lib/i18n/iszh-ceiling.test.ts:5— Derive ROOT from the test file location rather than process.cwd() so the test scans the web package even when vitest is invoked from the repo root. This requires importing fileURLToPath from node:url and resolving ../.. from this file.
Assessment
The test is a useful regression guard, but it should be made invocation-independent and path-segment-aware before relying on it in CI. The current text-matching and ignore-list limitations are acceptable for a coarse ceiling but should be documented.
Advisory review by Codewhale (codewhale review --pr 5805 --post, head 015e1770ef6fc91309f48d3addd8661abf95f36d). Line-specific findings are also posted as inline review comments; mechanical fixes arrive as committable suggestions you can apply from the Files tab. CODEOWNERS approval still governs merge.
| import { readdirSync, readFileSync, statSync } from "node:fs"; | ||
| import { join, relative } from "node:path"; | ||
|
|
||
| const ROOT = process.cwd(); |
There was a problem hiding this comment.
[WARNING] ROOT depends on process.cwd()
The scan root is process.cwd(), so running the test from the repo root instead of the web package changes what is scanned. The test lives under web/lib/i18n and MIGRATION_HOME is relative to web (lib/i18n), so from repo root the intended migration home is not excluded and the count is wrong. Derive ROOT from the test file location instead.
| .map((file) => relative(ROOT, file)) | ||
| .filter( | ||
| (file) => | ||
| !file.startsWith(MIGRATION_HOME) && |
There was a problem hiding this comment.
[WARNING] MIGRATION_HOME prefix check is not path-segment safe
file.startsWith(MIGRATION_HOME) also excludes paths such as lib/i18n-utils or lib/i18next, and it does not account for Windows path separators. Use file.startsWith(MIGRATION_HOME + sep) or compare path segments.
| (file) => | ||
| !file.startsWith(MIGRATION_HOME) && | ||
| !/\.test\.[jt]sx?$/.test(file) && | ||
| readFileSync(join(ROOT, file), "utf8").includes("isZh"), |
There was a problem hiding this comment.
[INFO] Text search counts any mention of "isZh", not actual branching
The test reads each file and checks .includes("isZh"), so comments, strings, type names, or import references can count as branching. This may produce false positives and require ceiling adjustments for non-branching mentions. Consider using an AST or a more targeted regex for actual isZh conditional expressions if precision is desired.
|
|
||
| function walk(dir: string, out: string[] = []): string[] { | ||
| for (const entry of readdirSync(dir)) { | ||
| if (entry === "node_modules" || entry === ".next" || entry === ".git") continue; |
There was a problem hiding this comment.
[INFO] walk() only skips node_modules, .next, and .git
Other generated or vendor directories such as dist, build, .turbo, or coverage may be walked and can contain transpiled files with isZh, causing false counts or slow runs. Add project-specific ignore patterns or use Vitest config root exclusions.
| @@ -0,0 +1,38 @@ | |||
| import { describe, expect, it } from "vitest"; | |||
| import { readdirSync, readFileSync, statSync } from "node:fs"; | |||
| import { join, relative } from "node:path"; | |||
There was a problem hiding this comment.
Import the platform path separator so the migration-home prefix check is segment-safe.
| import { join, relative } from "node:path"; | |
| import { join, relative, sep } from "node:path"; |
| .map((file) => relative(ROOT, file)) | ||
| .filter( | ||
| (file) => | ||
| !file.startsWith(MIGRATION_HOME) && |
There was a problem hiding this comment.
Compare with the platform separator so only actual children of lib/i18n are excluded and Windows paths work correctly.
| !file.startsWith(MIGRATION_HOME) && | |
| !file.startsWith(MIGRATION_HOME + sep) && |
| import { readdirSync, readFileSync, statSync } from "node:fs"; | ||
| import { join, relative } from "node:path"; | ||
|
|
||
| const ROOT = process.cwd(); |
There was a problem hiding this comment.
Derive ROOT from the test file location rather than process.cwd() so the test scans the web package even when vitest is invoked from the repo root. This requires importing fileURLToPath from node:url and resolving ../.. from this file.
Adds the ceiling test #5519 asked for: files branching on
isZhoutsideweb/lib/i18nmust never exceed 28 (the count on main today). Migration work lowers the count; the constant ratchets down with it.vitest run lib/i18n/iszh-ceiling.test.ts— 1 passed.Note
Low Risk
Adds a regression test only; no runtime or product behavior changes.
Overview
Adds a Vitest guard for the
isZh→lib/i18nmigration: it scans thewebtree for.js/.ts/.jsx/.tsxfiles that containisZhwhile excludinglib/i18nand test files, and fails if that count exceeds 28 (today’s baseline on main).The test is documented as a one-way ratchet—migration should shrink the offender list;
CEILINGis only lowered after branches move intolib/i18n, so new scatteredisZhbranching cannot creep back in.Reviewed by Cursor Bugbot for commit 015e177. Bugbot is set up for automated code reviews on this repo. Configure here.