Repository navigation
[Maintainer] i18n: string-extraction codemod + CI literal guard - #1167
Conversation
…ale audit - scripts/i18n-literals.ts: shared scanner for user-visible literals, a per-file ratchet (i18n/literal-baseline.json) wired into i18n:check, and report/update commands; exemptions via i18n/literal-allowlist.json or an i18n-ignore comment - scripts/i18n-codemod.ts: rewrites JSX text, UI props, conditional and template copy and notification payloads into i18n._ descriptors with the usePascalI18n hook; module-level and server copy is listed for a person - en-XA pseudo-locale compiled from English, registered by the app outside production, plus a Playwright audit that screenshots surfaces and lists copy that bypassed the catalog - i18n:check accepts AI drafts flagged fuzzy + Review: required (they are never compiled into packs); check --reviewed keeps the sign-off gate
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 993f983ec3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!/\p{L}{2}/u.test(trimmed)) return false | ||
| if (allow.has(trimmed)) return false | ||
| // Identifiers, icon names, CSS classes, paths and URLs. | ||
| if (/^[a-z0-9]+(?:[-_:./#][a-z0-9]+)+$/i.test(trimmed) && !/\s/.test(trimmed)) return false |
There was a problem hiding this comment.
Do not classify every separator-delimited token as a non-translatable identifier. Because this predicate is shared by JSX text and known UI attributes/properties, literals such as <button>Sign-in</button>, aria-label="zoom-in", or label: 'Read-only' match this regex and produce no candidate. New hardcoded copy in those forms therefore passes i18n:check, defeating the literal guard for a common class of labels.
Useful? React with 👍 / 👎.
| ts.isPropertyAssignment(property) && | ||
| ts.isIdentifier(property.name) && | ||
| property.name.text === 'description' | ||
| ) { | ||
| literalSink(property.initializer, 'notify') |
There was a problem hiding this comment.
Avoid emitting a second candidate for a notification option's description. For a TSX call such as toast.error('Failed', { description: 'Try again' }), this loop reports the description as notify, then normal AST traversal reports the same literal again through the generic UI_PROPERTIES branch. convert --apply consequently applies two edits to the identical source range and corrupts the generated file, while the check path also counts one literal twice.
Useful? React with 👍 / 👎.
| import '../lib/bootstrap' | ||
| import { createI18n, defaultI18n, PascalI18nProvider } from '@pascal-app/core' | ||
| import { registerBundledLocales } from '@pascal-app/editor/locales' | ||
| import { registerPseudoLocale } from '@pascal-app/editor/locales/pseudo' |
There was a problem hiding this comment.
Load the pseudo-locale module only through a production-excluded path rather than a static root-client import. The conditional suppresses registerPseudoLocale() but does not conditionally execute the ES module import; pseudo-locale.ts eagerly imports the generated en-XA/messages.ts, whose JSON.parse initializer is therefore part of the production client dependency graph. As the English catalog grows, every production editor load carries and evaluates a duplicate development-only catalog.
Useful? React with 👍 / 👎.
…rface helpers; codemod accepts explicit files
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
Tooling cross-review at b4cc700: CHANGES REQUESTED CI is green on this SHA (quality, cli-smoke, Changed paths, ci), and Bugbot is neutral. The codemod isn't applied anywhere, so no product copy changes. Locally, The guard catches new JSX text, P1: the review relaxation is a real weakening.
P2:
Security isn't needed: no auth, payments, secrets, infra, moderation, user data or schema files are touched. |
…view A translation that is Review: human on the base branch with the same English must stay reviewed. CI checks out the merge commit's first parent for the comparison, and release.yml runs check --reviewed before publish. The review rules are now a pure function so tests use fixtures.
The baseline lists each file's legacy copy by text, so new or swapped copy fails, and CI fails when the baseline gains entries relative to the base. i18n-ignore needs a reason; hyphenated labels and string constants rendered in JSX are copy; toast descriptions count once. The codemod keeps one-line bodies valid and converts explicitly named .ts modules. Enter, Space, Tab and Del leave the allowlist.
…rds; playwright-core 1.64.1
Scene theme names move to a shared helper used by both toolbars. 64 new messages; zh-CN/fr/es/ja drafts are fuzzy and await human review.
A translation that appears without a Source-Hash becomes a fuzzy draft unless a person already marked it Review: human, so pasted or tool-filled copy cannot reach the compiled packs without review.
They can render inside server components, where an inserted hook crashes the request.
A const is flagged only when the nearest declaration of the rendered name is that const string, so shadowing parameters, locals and same-named constants elsewhere in the file no longer swap the copy.
…eaks release.yml runs release-check instead of check --reviewed: it compares the committed catalogs with the last v* tag and fails when a reviewed entry with unchanged English became a draft, or when a compiled pack contains a draft. i18n:check runs the same checks against the PR base in CI. Both write per-locale draft counts to the job summary. Job-level tests run the command against temporary catalog copies.
Moved, split or extracted copy no longer fails: each text the baseline adds
must consume an equal text removed elsewhere, duplicates counted. JSX text is
recorded as it renders (whitespace, full HTML entities via entities,
{' '} joins), constants inside sentences become part of the sentence, and
concatenated copy is a candidate. The allowlist may not gain entries
relative to the base. Named callbacks and async functions are not hook
owners.
…dempotent --vars Copy is encoded so the compiled message renders the original text. Hooks are never placed in named callbacks or async functions, and editor package entry modules without 'use client' are left for a person. Descriptor mode stops at existing descriptors and i18n._ calls.
…ent.before A reviewed ID replaced by a new ID with the same English needs its translation carried over or reviewed again. On push, CI fetches and compares with the commit before the push.
…s for the new guards
…rror extract marks a translation whose English changed fuzzy + Review: required and keeps the old Source-Hash; that entry is never shipped, so ordinary CI counts it as awaiting review. A reviewed entry with a stale hash, and every stale entry under --reviewed, still fail.
|
Tooling cross-review at 285ce3c: CHANGES REQUESTED I checked this locally at /tmp, at the head commit. CI is green: quality, ci, cli-smoke, Changed paths and Bugbot all pass. Most of my earlier findings are fixed. One P1 is left in the release half of the gate. P1: the release regression check never compares against a real release, so it can't fail
Verified fixed
P2 (doesn't block)
Security isn't needed: there are no changes to auth, secrets or user data. |
Releases tag packages @pascal-app/<pkg>@<version>, so the v* lookup found v1.0.3, which has no catalogs, and the check could never fail. The base is now the latest @pascal-app/editor@* tag (the package that ships the catalogs); when that tag predates the catalogs it falls back to origin/main (another commit with catalogs), then to the commit that introduced them, and otherwise fails loudly. Tests run the default lookup in a temporary repository and fail on a reviewed-to-draft change.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 1032c0f. Configure here.
- Same-English matches between a removed reviewed ID and a new ID are a
warning (console and job summary), since they may be unrelated additions.
A rename declared with a renamed-from: <old id> descriptor comment is
carried over by i18n:extract and fails CI only if it arrives without the
reviewed translation.
- release-check fails on a --base that cannot be resolved or read.
- The allowlist applies to whole joined sentences; constants inside
concatenations count as copy; parenthesized named callbacks are not hook
owners.
- The codemod encodes adjacent static pieces together, keeps no-break
spaces, encodes text before a placeholder knowing a { follows, decodes
attribute entities, and escapes CR and line separators in emitted strings.
- The baseline format is version 3, since fingerprints now use rendered
text.
A previous-line ignore applies only when that line is a standalone comment, so an exemption written after one literal no longer hides a new literal on the following line.
|
Tooling cross-review at 53abaae: APPROVE I checked this locally at /tmp, at the head commit. CI is green: quality, ci, cli-smoke, Changed paths and Bugbot all pass. The P1 from 285ce3c is fixed. Release base (the earlier P1): fixed
Renamed IDs: weaker, but not silent
Spot checks
P2 (doesn't block)
Security isn't needed: there are no changes to auth, secrets or user data. |
What does this PR do?
Adds the tooling for the full localization migration (#621, after #1148 and #1162). It also migrates the viewer toolbar, so the new literal baseline starts lower than the scan rather than higher.
bun run i18n:literals -- convert [--apply] <path…>(scripts/i18n-codemod.ts) rewrites hardcoded JSX text,aria-*/title/placeholder/alt/label/tooltip… props, conditional and template-literal copy, andtoast/alert/confirm/setErrorpayloads into the existing runtime form,i18n._({ id, message, values }). It addsusePascalI18n()to the owning component (one-line bodies stay valid) and the import. Adjacent text and simple interpolations become one ICU message.--keys=… --vars=…turns module data into lazy/* i18n */descriptors (MessageDescriptor, now re-exported by@pascal-app/core).{follows.MANUALand never edited:'use client';.map(function …)callbacks (also parenthesized) and async functions;--varspass, changes nothing.i18n:check, already inbun run ci).i18n/literal-baseline.jsonlists each file's legacy copy by text: 2,884 candidates today. The baseline format is version 3, since fingerprints are now rendered text.Per file: adding or swapping a string fails, and a new file must have none.
The baseline may only shrink, measured across the repository: each text it adds must consume an equal text removed elsewhere, duplicates counted. Copy that moves with a
git mv, split or extraction passes afterupdate.The allowlist may not gain entries relative to the base either. One-off exemptions use
// i18n-ignore: <reason>; a barei18n-ignoreexempts nothing.Text is recorded as it renders: JSX whitespace rules, full HTML entity decoding (
entities, attributes included) and{' '}joins, so equivalent spellings match. A no-break space is content. The allowlist applies to the whole joined text.These count as copy:
conststrings rendered in JSX or inside a sentence (resolved through lexical scope);+, constants included.Toast descriptions are counted once.
fuzzy+Review: required. They are never compiled into packs, so users see English.bun scripts/i18n.ts release-checkcompares the committed catalogs with a base revision, never the working tree.i18n:check, depth-2 checkout). On push tomainit isgithub.event.before, which covers every commit of the push.release.ymlthe base is the latest@pascal-app/editor@<version>tag, since the editor package ships the catalogs.@pascal-app/editor@1.0.6), it falls back toorigin/mainwhen that is another commit with catalogs, then to the commit that introduced the catalogs, and otherwise fails loudly.--basethat cannot be resolved or read fails the job.renamed-from: <old id>descriptor comment.i18n:extractcarries the reviewed translation over when the English is unchanged. CI fails only if a declared rename arrives without it.Review: humanthemselves.check --reviewedremains a manual sign-off command; it is not in any workflow.extractnever approves a newly filled translation by itself.en-XA(accented, ~30 % longer, bracketed) is compiled from English and checked for drift.apps/editor/lib/dev-locales.tsregisters it only outside production, inside aNODE_ENVbranch that the bundler drops. A localnext buildhas noen-XAstrings in.next/staticor.next/server.bun run i18n:pseudo-audit(playwright-core 1.64.1, matching the tree, with the system Chrome; a dev tool, not a CI job) opens each surface ini18n/pseudo-surfaces.tsand saves full-viewport screenshots.inno longer hidesFindorPin.w-max min-w-60 max-w-80, so longer labels no longer wrap or truncate.i18n/README.mdcovers the guard, codemod, review states, how to declare a rename, release behaviour (including which tag the release compares with) and the audit. It includes "Releases ship drafts as English until they are reviewed."CONTRIBUTING.mdgets a short "User-visible text" section.How to test
bun run i18n:check: passes. It reports 2,884 legacy candidates and 64 drafts per locale awaiting review, and writes that table to$GITHUB_STEP_SUMMARYin CI.packages/editor/src/i18n/locales/fr/messages.po, markeditor.buildPanel.advancedas#, fuzzy+Review: requiredand rerun: it fails withfr: "editor.buildPanel.advanced" is reviewed on the base branch with the same English and must stay reviewed. I ran this locally.editor.buildPanel.advancedinbuild-panel.tsxtoeditor.buildPanel.advancedSection, withcomment: 'renamed-from: editor.buildPanel.advanced', then runbun run i18n:extract. The French entry arrives asAvancéwithReview: human, andi18n:checkpasses. I ran this locally and reverted.<p>Hello there</p>to a component, or change an existing literal's text: the check fails and names the file, line and text. Add{/* i18n-ignore: demo */}above it and the check passes; a bare{/* i18n-ignore */}does not.<p>Pascal{' '}</p>passes like<p>Pascal</p>.bun scripts/i18n.ts release-checkprints the base it compares with and passes;--base nonexistent-refexits 1.--base-dir, in a temporary git repository. It uses an@pascal-app/editor@*tag that has catalogs, ignoringv*tags. It falls back to the catalogs' first commit when the tag predates them. It fails loudly when no base exists. The first two exit 1 on a reviewed-to-draft change.Avancébecomes a draft with the same English, and when a draft is injected into the compiled French pack.bun test ./scripts/i18n.test.ts ./scripts/i18n-literals.test.ts ./packages/editor/src/i18n: 55 pass. The tests use fixtures, temp copies and temp repositories, never the real catalogs. They include:bun dev, thenbun run i18n:pseudo-audit -- --out .i18n-audit: screenshots land in.i18n-audit/.Follow-ups (not in this PR)
NEXT_PUBLIC_PASCAL_PSEUDO_LOCALE), so production-built Preview deployments can enable it.<html lang>(inherited from [Maintainer] i18n: locale registry, formatting and first locales #1148).memo as reactMemo) in the hook-owner check; aliased wrappers are currently refused conservatively.Screenshots / screen recording
The editor in
en-XAfrom the audit. The migrated top bar and toolbar are pseudo-localized; the tool tiles, tool dock and sidebar are later surface PRs:Editor in en-XA
Checklist
bun devbun checkto verify)mainbranchTo show artifacts inline, enable in settings.
Note
Medium Risk
Changes CI and release gates for translations plus a large baseline policy surface; the toolbar migration is localized UI but mistakes could block merges or releases.
Reviewed by Cursor Bugbot for commit 53abaae. Bugbot is set up for automated code reviews on this repo. Configure here.