Repository navigation
Conversation
…ys the source uses, and each locale against English
Parses src/ (tests excluded) with Babel and reads the key of every t() call
and i18nKey attribute: literal keys, every branch of a conditional or
fallback array, constants, and template or concatenated keys as patterns.
String literals that equal a key count too, for keys stored in config.
families.json declares the keys of the call sites whose pattern spans a
whole namespace ('map_project.' + id). Each locale is compared with English:
missing keys, blank values and values identical to English are listed apart.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Ensn3G9qDdhmuQ29FLiBMb
…s: 208 of 778 English keys unreferenced report.md is the audit's output for this tree; findings.md records the manual check of the unreferenced keys (all 208 stand), the five keys the code uses that English lacks, locale coverage (none missing or blank), and the assessment of the 228-of-752 and user.recent_activity* claims made in the review of #90. No catalog key is removed here. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ensn3G9qDdhmuQ29FLiBMb
… audit Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ensn3G9qDdhmuQ29FLiBMb
… checkout's families.json, and sorting ignores the locale With --root, the CLI read families.json from the other checkout, which an older commit doesn't have, so findings.md's 211-of-752 at 25e2f8a couldn't be repeated with the documented command; it now reads the declarations next to the script, and findings.md says how to repeat it. Call sites were ordered with localeCompare, so the report could differ between machines with different locales; they're ordered by code unit now, like the key lists. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ensn3G9qDdhmuQ29FLiBMb
paynejd
left a comment
There was a problem hiding this comment.
Codex review, pass 1 (codex-cli 0.162.1, commit ffe7465)
Clean.
findings.json
{
"reviewer": "codex-cli 0.162.1",
"perspective": "defects",
"mode": "full",
"commit": "ffe7465",
"verdict": "clean",
"prior": [],
"findings": []
}
Response to Codex pass 1No findings to answer. I found two more problems in my own review and fixed them in 18dcb23, each with a test that fails without the fix. Codex pass 2 checks them.
|
paynejd
left a comment
There was a problem hiding this comment.
Codex review, pass 2 (codex-cli 0.162.1, commit 18dcb23)
Clean.
findings.json
{
"reviewer": "codex-cli 0.162.1",
"perspective": "defects",
"mode": "verify",
"commit": "18dcb23",
"verdict": "clean",
"prior": [],
"findings": []
}
Response to Codex pass 2No findings to answer. Pass 2 reviewed 18dcb23, my two self-review fixes, along with the whole diff. With no P0 or P1 open, the defect passes stop here. Next is the simplification review. |
paynejd
left a comment
There was a problem hiding this comment.
Simplification review (commit 18dcb23)
8 items.
- S1 · delete · src/i18n/audit.js:92 · Two output fields are never read. One is
viaon each site: the addSite parameter and its 't()', 'i18nKey' and 'Trans' arguments. The other isevidence, copied onto each family at line 192. renderReport prints neither. The.mapprojection on opaqueSites (line 215) exists only to carryvia. The only reader is audit.test.js:90, which pins that implementation detail. → Nothing replaces them. Useconst addSite = path => {, drop theviakey from both site objects andevidencefrom families, writeopaqueSites: sites.filter(site => site.opaque), and delete audit.test.js:90. families.json keeps its evidence for reviewers. With S1, S2, S3, S5 and S6 applied together in a scratch copy, report.md came out byte-identical and all 18 tests passed. (-2) - S2 · shrink · src/i18n/audit.js:153 · Three sorts do the work of one. They are byFileAndLine over the merged sites (lines 153-154, used at 169),
places.sort()over literalSites (line 202), and the CLI's.sort()of the locale directories (audit.mjs:35), which auditKeys repeats at line 225. scanSource returns each file's sites and literals in source order, so sorting the sources once orders both lists. → Sort once at the top of auditKeys and keep the code-unit comment:const scanned = [...sources].sort((a, b) => (a.file < b.file ? -1 : 1)).map(({ file, code }) => ({ file, ...scanSource(code, file) })), thenconst sites = scanned.flatMap(scan => scan.sites). Delete byFileAndLine, line 202 and the.sort()at audit.mjs:35. Places for a literal then appear in line order, not string order (:98before:124). Checked in the scratch copy: report.md is byte-identical, and the code-unit and read-order tests still pass. (-3) - S3 · shrink · src/i18n/audit.js:242 · renderReport collects lines through a
linesbuffer and anaddhelper that only callslines.push. Each of the seven sections pays for that with anadd(/)pair and a separating blank line. → BuildproblemLinesfirst, then return a single array:return ['# i18n key audit', '', …, '## Needs attention', '', ...(problemLines.length ? problemLines : ['Nothing.']), '', …, ...locales.flatMap(l => [### ${l.locale}, '', …, '']), …, ''].join('\n'). Checked in the scratch copy: report.md is byte-identical and the function drops from 101 lines to 81. (-20) - S4 · yagni · src/i18n/audit.js:170 ·
wherehas one caller (line 173). Line 200 already builds the samefile:linestring inline. → Inline it at line 173:.push(${site.file}:${site.line}). (-1) - S5 · delete · src/i18n/audit.js:75 · The
'OptionalMemberExpression'check in isTranslate never fires. Babel parsesi18n?.t('k')andthis.props?.t('k')as an OptionalCallExpression (checked with @babel/core), and the CallExpression visitor at line 108 never visits that node type. The only form the check can match is(i18n?.t)('k'). No call in src is optional. → Replace it with(callee.type === 'MemberExpression' && !callee.computed && callee.property.name === 't'). Whether optional calls should be scanned is a coverage question for the defects pass, not a reason to keep the dead check. (+0) - S6 · stdlib · scripts/i18n-audit/audit.mjs:14 · The
optionargv helper reimplementsutil.parseArgsfrom node:util, which is stable on Node 22, the version the Dockerfile and CI use. The helper also ignores a mistyped flag, so--rot <dir>quietly audits this checkout instead. → Addimport { parseArgs } from 'node:util', thenconst { values: options } = parseArgs({ options: { root: { type: 'string' }, out: { type: 'string' } } })andpath.resolve(options.root || path.join(here, '..', '..')). Unknown flags then fail. Checked in the scratch copy: auditCli.test.js passes and the output is unchanged. (-3) - S7 · native · scripts/i18n-audit/audit.mjs:21 ·
--outdoes what shell redirection already does.npm run -s i18n:audit > scripts/i18n-audit/report.mdwrites a file byte-identical to the committed report.md (checked). → Dropout, and replace the if/else at lines 44-47 withprocess.stdout.write(report). Change README line 20 and the header comment to thenpm run -s … >form. Keep--outif it is worth four lines to write only after a successful run and to keep npm's banner out of the file when someone forgets-s. (-4) - S8 · delete · src/i18n/audit.js:45 · The
resolvingset stops patternsOf from following a const that resolves back to itself. That needs a binding initialised from itself through these expression forms, which throws a TDZ ReferenceError for const and let. The tree has no such binding: all 6 const resolutions were checked and none cycles. → Useconst patternsOf = path => {and dropresolvingfrom the recursive calls. The identifier branch becomesif(binding?.constant && declarator.isVariableDeclarator() && declarator.get('id').isIdentifier() && declarator.node.init) return patternsOf(declarator.get('init')). Confidence is low because the guard is cheap, and without it such a file would crash the audit with a stack overflow. (-1)
findings.json
{
"reviewer": "claude subagent",
"perspective": "simplify",
"mode": "full",
"commit": "18dcb23",
"verdict": "findings",
"prior": [],
"findings": [
{
"id": "S1",
"severity": null,
"tag": "delete",
"file": "src/i18n/audit.js",
"line": 92,
"claim": "Two output fields are never read. One is `via` on each site: the addSite parameter and its 't()', 'i18nKey' and 'Trans' arguments. The other is `evidence`, copied onto each family at line 192. renderReport prints neither. The `.map` projection on opaqueSites (line 215) exists only to carry `via`. The only reader is audit.test.js:90, which pins that implementation detail.",
"scenario": null,
"action": "Nothing replaces them. Use `const addSite = path => {`, drop the `via` key from both site objects and `evidence` from families, write `opaqueSites: sites.filter(site => site.opaque)`, and delete audit.test.js:90. families.json keeps its evidence for reviewers. With S1, S2, S3, S5 and S6 applied together in a scratch copy, report.md came out byte-identical and all 18 tests passed.",
"lines_delta": -2,
"confidence": "high"
},
{
"id": "S2",
"severity": null,
"tag": "shrink",
"file": "src/i18n/audit.js",
"line": 153,
"claim": "Three sorts do the work of one. They are byFileAndLine over the merged sites (lines 153-154, used at 169), `places.sort()` over literalSites (line 202), and the CLI's `.sort()` of the locale directories (audit.mjs:35), which auditKeys repeats at line 225. scanSource returns each file's sites and literals in source order, so sorting the sources once orders both lists.",
"scenario": null,
"action": "Sort once at the top of auditKeys and keep the code-unit comment: `const scanned = [...sources].sort((a, b) => (a.file < b.file ? -1 : 1)).map(({ file, code }) => ({ file, ...scanSource(code, file) }))`, then `const sites = scanned.flatMap(scan => scan.sites)`. Delete byFileAndLine, line 202 and the `.sort()` at audit.mjs:35. Places for a literal then appear in line order, not string order (`:98` before `:124`). Checked in the scratch copy: report.md is byte-identical, and the code-unit and read-order tests still pass.",
"lines_delta": -3,
"confidence": "medium"
},
{
"id": "S3",
"severity": null,
"tag": "shrink",
"file": "src/i18n/audit.js",
"line": 242,
"claim": "renderReport collects lines through a `lines` buffer and an `add` helper that only calls `lines.push`. Each of the seven sections pays for that with an `add(`/`)` pair and a separating blank line.",
"scenario": null,
"action": "Build `problemLines` first, then return a single array: `return ['# i18n key audit', '', …, '## Needs attention', '', ...(problemLines.length ? problemLines : ['Nothing.']), '', …, ...locales.flatMap(l => [`### ${l.locale}`, '', …, '']), …, ''].join('\\n')`. Checked in the scratch copy: report.md is byte-identical and the function drops from 101 lines to 81.",
"lines_delta": -20,
"confidence": "medium"
},
{
"id": "S4",
"severity": null,
"tag": "yagni",
"file": "src/i18n/audit.js",
"line": 170,
"claim": "`where` has one caller (line 173). Line 200 already builds the same `file:line` string inline.",
"scenario": null,
"action": "Inline it at line 173: `.push(`${site.file}:${site.line}`)`.",
"lines_delta": -1,
"confidence": "high"
},
{
"id": "S5",
"severity": null,
"tag": "delete",
"file": "src/i18n/audit.js",
"line": 75,
"claim": "The `'OptionalMemberExpression'` check in isTranslate never fires. Babel parses `i18n?.t('k')` and `this.props?.t('k')` as an OptionalCallExpression (checked with @babel/core), and the CallExpression visitor at line 108 never visits that node type. The only form the check can match is `(i18n?.t)('k')`. No call in src is optional.",
"scenario": null,
"action": "Replace it with `(callee.type === 'MemberExpression' && !callee.computed && callee.property.name === 't')`. Whether optional calls should be scanned is a coverage question for the defects pass, not a reason to keep the dead check.",
"lines_delta": 0,
"confidence": "medium"
},
{
"id": "S6",
"severity": null,
"tag": "stdlib",
"file": "scripts/i18n-audit/audit.mjs",
"line": 14,
"claim": "The `option` argv helper reimplements `util.parseArgs` from node:util, which is stable on Node 22, the version the Dockerfile and CI use. The helper also ignores a mistyped flag, so `--rot <dir>` quietly audits this checkout instead.",
"scenario": null,
"action": "Add `import { parseArgs } from 'node:util'`, then `const { values: options } = parseArgs({ options: { root: { type: 'string' }, out: { type: 'string' } } })` and `path.resolve(options.root || path.join(here, '..', '..'))`. Unknown flags then fail. Checked in the scratch copy: auditCli.test.js passes and the output is unchanged.",
"lines_delta": -3,
"confidence": "medium"
},
{
"id": "S7",
"severity": null,
"tag": "native",
"file": "scripts/i18n-audit/audit.mjs",
"line": 21,
"claim": "`--out` does what shell redirection already does. `npm run -s i18n:audit > scripts/i18n-audit/report.md` writes a file byte-identical to the committed report.md (checked).",
"scenario": null,
"action": "Drop `out`, and replace the if/else at lines 44-47 with `process.stdout.write(report)`. Change README line 20 and the header comment to the `npm run -s … >` form. Keep `--out` if it is worth four lines to write only after a successful run and to keep npm's banner out of the file when someone forgets `-s`.",
"lines_delta": -4,
"confidence": "low"
},
{
"id": "S8",
"severity": null,
"tag": "delete",
"file": "src/i18n/audit.js",
"line": 45,
"claim": "The `resolving` set stops patternsOf from following a const that resolves back to itself. That needs a binding initialised from itself through these expression forms, which throws a TDZ ReferenceError for const and let. The tree has no such binding: all 6 const resolutions were checked and none cycles.",
"scenario": null,
"action": "Use `const patternsOf = path => {` and drop `resolving` from the recursive calls. The identifier branch becomes `if(binding?.constant && declarator.isVariableDeclarator() && declarator.get('id').isIdentifier() && declarator.node.init) return patternsOf(declarator.get('init'))`. Confidence is low because the guard is cheap, and without it such a file would crash the audit with a stack overflow.",
"lines_delta": -1,
"confidence": "low"
}
]
}…view cuts, and optional t() calls are read
From the simplification review: drop the unread via and evidence fields,
sort the files once instead of sorting sites, places and locales apart,
build the report as one array, inline a one-use helper, and parse the CLI's
flags with util.parseArgs, which also rejects a mistyped flag. The
OptionalMemberExpression check in isTranslate never fired, because
i18n?.t('key') is an OptionalCallExpression; the visitor now visits those
too, so such a call isn't missed. report.md is unchanged.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Ensn3G9qDdhmuQ29FLiBMb
Response to the simplification review
Gate after the cuts: eslint clean, and |
…nst the final code The README names both test files and says what makes the report reproducible; findings.md no longer says all four unreadable call sites get keys from string literals, since MultiAlgoSelector's gets none today. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ensn3G9qDdhmuQ29FLiBMb
filiperochalopes
left a comment
There was a problem hiding this comment.
Looks like it's a deterministic report/audit script, and we need another round for actually removing them, right? Will it be LLM triggered and use the report as a reference?
|
@filiperochalopes thanks for the review. To your question: yes, removing the keys is a separate step, and it doesn't need an LLM. It's a plain edit from the list. We're closing this PR without merging it. The audit applies to every OCL web app, not only the Mapper, because oclweb3 and oclweb2 use the same i18next layout. So we've moved it into the core team's internal checks (private for now: ocl_online#523). From there it runs weekly against oclmap, oclweb3 and oclweb2, and flags any new gap: a key the code uses that English lacks, or a locale missing a key. Once it settles, we plan to publish it as a package contributors can run before opening a PR. The Mapper results are on OpenConceptLab/ocl_issues#2880. The follow-up is OpenConceptLab/ocl_issues#2917: remove the unused keys, and add the 4 keys the code uses that English lacks. |
|
Sounds good |
What changed and why
OpenConceptLab/ocl_issues#2880 asks for a verified inventory of unused and untranslated i18n keys, so the catalogs can be trimmed without removing strings the code builds at run time. This adds a reproducible audit and its results. No catalog key is removed: that's a separate, reviewed change.
npm run i18n:audit:src/i18n/audit.jsparsessrc/with Babel (tests excluded) and reads the key of everyt()/i18n.t()/this.props.t()call, optional calls likei18n?.t()included, and everyi18nKeyattribute. It takes each branch of a conditional or fallback array, follows constants, and turns template-literal and concatenated keys into patterns (map_project.view_*). A string literal that equals a key counts too, for keys stored in config (labelKey: 'common.id').scripts/i18n-audit/audit.mjsis the CLI.--rootaudits another checkout, such as an older commit.scripts/i18n-audit/families.jsondeclares the keys of the 7 call sites whose pattern spans a whole namespace ('map_project.' + id,`concept.${prop}`,`search.${resource}`), with the code that produces them. Without it, those patterns would hide every unused key in those namespaces. The report flags an undeclared namespace-wide family, a declared key the code can't produce or the catalog lacks, and a declaration whose call site has gone.scripts/i18n-audit/report.mdis the output for this tree, andfindings.mdthe review done with it.Results, on
mainat 0f6b0beID,JSON,AutoMatch). es has 2 keys English lacks.npm run i18n:audit -- --root <checkout of 25e2f8a>repeats it. 228 can't be reproduced exactly because its rules aren't stated, and simpler rule sets give anything from 155 to 307 at the same commit (table infindings.md). Of the first 35 keys in its list, the audit agrees on 33.common.actionandcommon.created_onare in use: column configs pass them tot()aslabelKey. Ignoring keys passed that way adds 19 keys to 211, which makes 230.user.recent_activity*: the claim that all three were unused is correct.user.recent_activitylost its only call site when c619b5e deletedEvents.jsx(2026-06-30). The other two were added (b05133c) and removed (ca4024e) on the pt-BR branch, and nothing ever used them. None of the three exists onmainnow.How I tested it
On Node 22.23.2, from the worktree root, running
pr.yml's steps in order:start-prod.shsecret-shaped variable check: passed.npm ci: OK..eslintrc.json("couldn't determine the plugin react uniquely"). I ran./node_modules/eslint/bin/eslint.js --no-eslintrc -c .eslintrc.json src --ext .jsx,.jsinstead, which reads only this tree's config, as CI's checkout does.npm run test:all: 466 unit tests (19 new, insrc/i18n/__tests__/audit.test.jsandauditCli.test.js) and 6 integration tests, all passing. Every test added during review fails without its fix.npm run i18n:auditprintsreport.mdbyte for byte. A test checks that the report doesn't depend on the order files are read in or on the locale.--rooton the trees of 25e2f8a (752 keys) and 06d8841 (the Portuguese (Brazil) Translation & Language Persistence #90 merge).Acceptance criteria
Requirements:
i18nKey,<Trans>, dynamic keys, concatenation and conditionals. Seesrc/i18n/audit.js; tests "scanSource reads literal keys from t(), i18n.t(), this.props.t() and i18nKey", "…reads optional calls", "…every branch of a conditional, a logical fallback and a fallback-key array", "…turns template literals and concatenation into key patterns", "…records a Trans without i18nKey".report.md"Unreferenced keys (208)", the manual check infindings.md, andREADME.md"Limits of static detection".report.md"Locales compared with English" and the test "compareLocale separates missing keys, blank values and values identical to English".user.recent_activity*claims checked against call sites. Seefindings.md.src/i18n/locales/.Acceptance criteria:
report.md, and the 7 namespace-wide ones are declared infamilies.json. Nothing appears under "Needs attention" about families. Tests: "auditKeys counts an undeclared namespace-wide family…", "auditKeys uses the declared keys…", "auditKeys flags declarations that no longer fit…", "…do not depend on the order files are read in", "…orders call sites by code unit", and the CLI test for--root.report.md).user.recent_activity*claims assessed with evidence and the discrepancy explained (findings.md).Review
--rootreadfamilies.jsonfrom the other checkout, so the 211-of-752 result couldn't be repeated.localeCompare, so the report could change with the locale.util.parseArgs).i18n?.t('key')is read.--outdoesn't emptyreport.mdwhen the audit fails.const key = key || 'x'.Docs
scripts/i18n-audit/README.md(how to run it, what counts as a reference, declaring families, limits),findings.md,report.md.README.md: a short pointer to the audit.Not done, on purpose
report.mdand remove each from all four locales at once.report.mdis current. It would fail every i18n change that doesn't regenerate it. The README gives the one-line check.Noticed, not fixed
common.none,common.savedandsearch.save_default_filtersrender as the raw key in every language.map_project.candidates_legacy_shapeandmap_project.target_repo_version_requiredshow their inline English default in every language. Fixing them needs wording and translations, outside an audit.ResultConstants.js, in the column sets for mappings, repos, orgs, users and the URL registry. The Mapper only lists concepts, so these are removal candidates too, along with those column sets (findings.md).ConceptHeader.jsxhas afalse && …block that never renders.closes OpenConceptLab/ocl_issues#2880
🤖 Generated with Claude Code
https://claude.ai/code/session_01Ensn3G9qDdhmuQ29FLiBMb
Written by an Octoad session worker (
worker-v0.10, recipelean-loop, flowpublic>public, runopenconceptlab-ocl-issues-2880-20261011-141447).