MPDX-9934 - Ensure {{ name }} and not {name} in <Trans> - #1977
MPDX-9934 - Ensure {{ name }} and not {name} in <Trans>#1977zweatshirt wants to merge 2 commits into
{{ name }} and not {name} in <Trans>#1977Conversation
…single-brace interpolation
…proved i18n support
zweatshirt
added
the
Preview Environment
|
Preview branch generated at https://yarn-extract-errors.d3dytjb8adxkk5.amplifyapp.com |
zweatshirt
changed the title
zweatshirt
changed the title
<Trans> to always have a t={t] prop, ensure {{ name }} and not {name} in <Trans>
Bundle sizes [mpdx-react]Compared against b3f7971 No significant changes found |
zweatshirt
left a comment
zweatshirt
left a comment
There was a problem hiding this comment.
Multi-Agent Review — Verdict: BLOCKERS FOUND (1)
11 agents: 5 specialized reviewers, 1 dependency-impact, 2 gap reviewers, 3 cross-examination rounds. Risk score 10/10 mechanically, but that is inflated by file count — 24 of 29 files are a one-line t={t} addition. Effective risk is LOW–MEDIUM.
The blocker
finish.page.tsx:46-50 — the new <Trans> key can never resolve (severity 8.5, 3 agents). Three agents independently ran the real extractor and rendered the real react-i18next@11.18.6:
| key | |
|---|---|
yarn extract writes |
Congratulations!<1></1>You're all set! |
| react-i18next requests | Congratulations!<br/>You're all set! |
i18next-parser indexes a propless <br />; react-i18next keeps it as a tag. They can never match. Congratulations! and You're all set! are translated in 22 locales; the new composite key exists in none — so every non-English user finishing onboarding sees an English <h2> over translated body copy, permanently. public/locales/fr/translation.json:265 shows the Crowdin loop already ran on this construct and produced an unreachable key. It fails silently: yarn eslint on the file exits 0 and no test asserts the title.
The fix keeps all 22 translations and needs no Crowdin round-trip — see the inline comment.
What the debate changed
This was escalated and four other findings were withdrawn, so the count is smaller than the raw agent output:
- Escalated 7.5 → 8.5: UX originally called the regression temporary/self-healing. Architecture disproved it by seeding the translated extracted key into the
frbundle and re-rendering — still English. UX conceded. - Withdrawn:
Calculation.tsx:305<Trans>vst()— both agents who raised it withdrew after finding 7–29 pre-existing instances of the same form and confirming both produce a byte-identical key. - Revised 7.0 → 3.0: "regenerate
public/locales/en/translation.json". Only 9 of 228 added keys belong to this PR; ~95% is unrelated drift already onmain(3561 fresh vs 3403 committed). A blanket regeneration would attribute ~284 unrelated keys here — and would not fix the blocker anyway. - Revised 4.0 → 1.5: the childless
<Link/>inLimitedAccess.tsx. Rendering old vs new against six synthetic translations was byte-identical in every failure branch; the address was already inside the translatable key. No new failure mode. - Consolidated: nine separate rule-quality items became one finding — they share a single root cause.
What is verified good
- 41 of 41 missing-
tand single-brace violations fixed across 26 files; 0 remaining repo-wide (real ESLint 8.57.0 over 110<Trans>elements). - New test passes 10/10; 24 colocated suites / 169 tests green;
tsc --noEmitandyarn lint:ciclean (62 pre-existing warnings, 0 errors);prettier --checkclean. - No whitespace or punctuation regressions in any Prettier-rewrapped
<Trans>body — the highest-risk area of this PR. AboutForm.tsxDOM is byte-identical before/after (oldHtml === newHtml), and its oldvalues={{ boardDateFormatted, availableDateFormatted }}was dead — no matching placeholders — so this is a latent bug fix. Same for thegetLimitedTexttrio, whose old keys had 0 real translations in any locale.- No namespaced
useTranslationexists anywhere (675 bare call sites), so all 24t={t}additions are provable no-ops for key resolution. - No
overridesorextendsconfig redefinesno-restricted-syntax; nothing silently disabled. CI'stranslationsjob is pure verification and passes (exit 0, 2289 files, no warnings). - Both gap reviewers found the mechanical
t={t}files clean.
Cross-cutting: fix-one-fix-all
The single-brace and missing-t sweep is complete. But the <br/>/<strong>-in-children key-mismatch class has 7 sites repo-wide; this PR fixed 0 and added 1. The other 6 are pre-existing and outside this diff (EligibleDisplay.tsx:19,32, PartnerRemindersReport.tsx:200, NsoMpdQuestionnaire/.../Settings.tsx:19, RequestedSalaryCard.tsx:210, PersonalInformationSection.tsx:105) — worth a follow-up ticket.
Safeguard parity gap: the new rules cover instance-binding and single-brace children, but not the extract-vs-runtime serializer mismatch — the mechanism that actually breaks keys, and the one the blocker falls into. The risk is false confidence: "lint passes" now reads as "interpolation is safe." A third selector banning propless br/strong/i/p as <Trans> children would close it.
Also worth a follow-up ticket (pre-existing, not blocking)
SuggestedContactStatus.tsx calls five hooks below a conditional early return. A user with one contact selected who adds a second flips to the early-return path and renders fewer hooks — React throws and takes down the task modal. Predates this PR, and nothing lints it because .eslintrc.js does not extend plugin:react-hooks/recommended. Details inline.
All 11 agents ran read-only; working tree verified clean afterward (git status --porcelain empty, HEAD still 944a6da).
| title={ | ||
| <Trans> | ||
| {t('Congratulations!')} | ||
| <Trans t={t}> |
There was a problem hiding this comment.
Verified by execution, not inference — three agents each ran the real extractor into a scratchpad and rendered the real react-i18next@11.18.6:
yarn extractwrites:Congratulations!<1></1>You're all set!- react-i18next requests:
Congratulations!<br/>You're all set!
i18next-parser 9.0.2 serializes a propless <br /> as an indexed node; react-i18next keeps it as a tag (br is in the default transKeepBasicHtmlNodesFor, and nothing overrides it). The two strings can never be equal, so no state of the locale files reachable by Crowdin makes this render translated — proven by seeding the translated extracted key into the fr bundle and re-rendering: still English. public/locales/fr/translation.json:265 shows a human translator already localized inside this code artifact on the previous form, and it still never rendered.
Why the old code worked: {t('Congratulations!')} / {t("You're all set!")} were translated before Trans built its key, so the outer lookup missed and Trans fell back to rendering its own children — which were already translated. Removing the t() calls makes that fallback literal English.
Impact: both inner keys have real translations in 22 locales (en:862/:3349, fr:808/:3119, es-419 "¡Felicitaciones!"/"¡Está todo listo!", …). The composite key exists in none. Every non-English user reaching the last screen of onboarding sees an English <h2> over translated body copy. Silent: yarn eslint on this file exits 0, and finish.page.test.tsx never asserts the title.
Fix — satisfies both new lint rules, keeps every existing translation, no Crowdin round-trip needed:
// line 5: drop Trans from the import
import { useTranslation } from 'react-i18next';
// lines 45-51
title={
<>
{t('Congratulations!')}
<br />
{t("You're all set!")}
</>
}Worth adding the assertion that would have caught this, too:
it('renders the congratulations title', () => {
const { getByText } = render(<TestComponent />);
expect(getByText(/Congratulations!/)).toBeInTheDocument();
expect(getByText(/You're all set!/)).toBeInTheDocument();
});| * Identifier (e.g. {name}), MemberExpression (e.g. {user.name}), or CallExpression (e.g. {getName()}) | ||
| */ | ||
| selector: | ||
| ":matches(JSXElement[openingElement.name.name='Trans'], JSXElement[openingElement.name.name='Trans'] JSXElement) > JSXExpressionContainer > :matches(Identifier, MemberExpression, CallExpression)", |
There was a problem hiding this comment.
All reproduced against this exact config with the real ESLint 8.57.0 + @typescript-eslint/parser:
| probe | result |
|---|---|
components={{ bold: <strong>{count}</strong> }} |
wrongly flagged |
<Trans {...props}> (spread supplies t) |
wrongly flagged (rule 1) |
{user?.name} — optional chaining |
missed (ChainExpression breaks the > match) |
{`hi ${n}`}, {n + '!'}, {c ? a : b}, {[n]} |
missed ×4 |
{n as string}, {n!} |
missed ×2 |
<Trans t={t}><>{name}</></Trans> |
missed (JSXFragment is not JSXElement) |
t only on a nested element inside components |
missed (rule 1's :has() is a descendant match) |
controls: {name} / {' '} |
flagged / clean ✓ |
The components false positive is not latent in practice: no-restricted-syntax is never autofixable and .husky/pre-commit runs lint-staged → eslint --cache --fix, so it becomes a hard commit block with no autofix path — and it lands squarely on components, the escape hatch this PR itself adopts in getLimitedText.tsx. Optional chaining is the worst miss, since ?. is pervasive here and <Trans t={t}>Hello {contact?.name}</Trans> is exactly the bug this rule exists to prevent.
Verified drop-in replacement — passes all 16 probes and produces 0 violations across */**/*.{js,ts,tsx}, so the PR's headline invariant survives:
selector:
":matches(JSXElement[openingElement.name.name='Trans'], JSXElement[openingElement.name.name='Trans'] JSXElement:not(JSXAttribute JSXElement), JSXElement[openingElement.name.name='Trans'] JSXFragment) > JSXExpressionContainer > :not(Literal, JSXEmptyExpression, ObjectExpression, JSXElement, JSXFragment, LogicalExpression)",Three changes: :not(JSXAttribute JSXElement) excludes components subtrees; a JSXFragment branch closes the fragment escape; allowlist→denylist closes the seven missed node types at once. Literal must stay excluded to preserve the {' '} separator used in 30+ files; ObjectExpression preserves correct {{ name }}; LogicalExpression preserves {cond && <B/>}.
Rule 1 (line 70-71) is not fixable in esquery — :has(> JSXAttribute[name.name='t']) is a hard syntax error (no leading combinator inside :has()). That check needs a custom ESLint rule, or the two bad shapes need to stay on the manual review checklist.
| selector: | ||
| ":matches(JSXElement[openingElement.name.name='Trans'], JSXElement[openingElement.name.name='Trans'] JSXElement) > JSXExpressionContainer > :matches(Identifier, MemberExpression, CallExpression)", | ||
| message: | ||
| 'Single-brace {name} inside <Trans> becomes part of the extracted key, so the lookup never matches. Use {{ name }} which only typechecks as a direct child of <Trans>, so if this sits inside a nested element, move that element outside the <Trans> instead.', |
There was a problem hiding this comment.
"…so if this sits inside a nested element, move that element outside the <Trans> instead" mandates children-based <Trans>, whose key is reconstructed twice — once by i18next-parser at extract time, once by react-i18next at render time. Whenever a propless br/strong/i/p child is present those two disagree (<N></N> vs <tag/>), and the key becomes permanently unresolvable. That is exactly what happened at finish.page.tsx:46, and there are 7 such sites repo-wide.
The defaults="…" + components={{…}} form this same PR adopts in getLimitedText.tsx has no such failure mode: the author writes one string that both the parser and the runtime consume verbatim, so no two-serializer agreement is required. Confirming evidence from a fresh extract: the only keys carrying literal <i>/<p>/<strong> markup are authored-string keys, and all 7 unmatchable-key sites are children-based.
Two suggestions, in order of value:
- Lead with the safe form in this message —
defaults=+values=+components=handles interpolation inside nested markup without DOM surgery.AboutForm.tsx:93-112in this PR had to invert<Trans>/<Box>nesting and hoist a<Box>out just to satisfy the current advice, when a props-only fix existed. - Add a third selector banning propless
br/strong/i/pas<Trans>children. That class is mechanically checkable, it is only 7 sites to migrate, and it would have caught this PR's blocker at lint time.
| message: | ||
| 'Single-brace {name} inside <Trans> becomes part of the extracted key, so the lookup never matches. Use {{ name }} which only typechecks as a direct child of <Trans>, so if this sits inside a nested element, move that element outside the <Trans> instead.', | ||
| }, | ||
| ], |
There was a problem hiding this comment.
Suggested additions:
CLAUDE.md § Localization — use t() with interpolation by default; use <Trans> only when inline elements matter; inside <Trans> interpolate as {{ name }} and only as a direct child; prefer defaults= + values= over children; never put a propless basic HTML node (br/strong/i/p) in <Trans> children.
.claude/rules/code-review.md § Localization — annotate the existing t-prop item as machine-enforced, with the two shapes still needing a manual check (<Trans {...props}> false positive, and t supplied only on a nested element inside components — the latter unfixable in esquery). Add the single-brace item with its known misses, and add the propless-basic-HTML rule.
| }, | ||
| rules: { 'no-restricted-syntax': restrictedSyntax }, | ||
| }) | ||
| .filter((message) => message.ruleId === 'no-restricted-syntax') |
There was a problem hiding this comment.
1. Parse errors are swallowed. ESLint reports them with ruleId: null, which this filter discards, so any probe that fails to parse yields [] — exactly what the toEqual([]) tests assert. Proven with a deliberate syntax error:
probe: '<Trans t={t}>oops</Tran'
ALL msgs: [{"ruleId":null,"fatal":true,"msg":"Parsing error: Unexpected token )"}]
after filter: []
So accepts a <Trans> that is passed t, accepts double-brace interpolation, accepts a string literal child…, accepts an expression in a nested element attribute, and does not flag nested double-brace interpolation are all one typo away from green-but-meaningless.
2. Wrong parser. .eslintrc.js:3 sets parser: '@typescript-eslint/parser'; verify() here passes none, so it validates against espree's AST instead. Combined with defect 1 this is a blind spot the harness structurally cannot see:
plain {name} | espree: FLAG | tsParser: FLAG
TS as-cast {name as string} | espree: PARSE_ERR→[] | tsParser: [] (real miss)
TS non-null {name!} | espree: PARSE_ERR→[] | tsParser: [] (real miss)
Fix:
const linter = new Linter({ configType: 'eslintrc' });
linter.defineParser('ts', require('@typescript-eslint/parser'));
const lintTrans = (body: string): string[] => {
const messages = linter.verify(
`export const Probe = ({ name, url, t }) => (\n ${body}\n);`,
{
parser: 'ts',
parserOptions: {
ecmaVersion: 2020,
sourceType: 'module',
ecmaFeatures: { jsx: true },
},
rules: { 'no-restricted-syntax': restrictedSyntax },
},
);
// A parse error reports ruleId: null, which the filter below would drop,
// turning every toEqual([]) expectation into a false pass.
const fatal = messages.find((message) => message.fatal);
if (fatal) {
throw new Error(`Probe failed to parse: ${fatal.message}`);
}
return messages
.filter((message) => message.ruleId === 'no-restricted-syntax')
.map((message) => message.message);
};Untested branches worth adding (each verified against the real config): optional chaining {user?.name}, template literal, conditional expression, MemberExpression (claimed in the config comment, never asserted), spread-only attributes, and the components-prop case from the .eslintrc.js:83 comment. If you widen the selector as suggested there, the first four flip from missed to flagged.
canac
left a comment
canac
left a comment
There was a problem hiding this comment.
Incredible work on this! I know we're careful about translations, but it's great to be able to catch a lot of places that we still missed!
| ignoreMemberSort: false, | ||
| }, | ||
| ], | ||
| 'no-restricted-syntax': [ |
There was a problem hiding this comment.
I love that we can do this with the ESLint selectors instead of needing to write a custom code-based rule!
| selector: | ||
| ":matches(JSXElement[openingElement.name.name='Trans'], JSXElement[openingElement.name.name='Trans'] JSXElement) > JSXExpressionContainer > :matches(Identifier, MemberExpression, CallExpression)", | ||
| message: | ||
| 'Single-brace {name} inside <Trans> becomes part of the extracted key, so the lookup never matches. Use {{ name }} which only typechecks as a direct child of <Trans>, so if this sits inside a nested element, move that element outside the <Trans> instead.', |
There was a problem hiding this comment.
Use {{ name }} which only typechecks as a direct child of
From my research, upgrading our i18n libraries might fix this restriction.
| This is calculated from your {above} responses and is the | ||
| lower of the Annual Fair Rental Value or the Annual Cost of | ||
| Providing a Home. | ||
| <Trans t={t} values={{ above }}> |
There was a problem hiding this comment.
Can you research values more? It seems redundant, if not actively harmful. Maybe we want a follow-up lint rule to ensure that we don't pass values to Trans.
There was a problem hiding this comment.
Making a separate PR for disallowing both values and defaults
| }} | ||
| > | ||
| <Trans defaults="Add new {{page}}" values={{ page }} /> | ||
| <Trans t={t} defaults="Add new {{page}}" values={{ page }} /> |
There was a problem hiding this comment.
Another possible rule would be to reject defaults as well and require the text to be a child of <Trans>.
| content: ( | ||
| <Trans t={t}> | ||
| Something went wrong while loading your account information. Please | ||
| try again later. If the problem persists, please contact {link}. |
There was a problem hiding this comment.
question: So it doesn't work to interpolate {{ link }} directly if it's a ReactNode? Does that cause a TypeScript error? If so, I guess we can't flat-out reject defaults until we can get that error figured out (upgrading the i18n libraries may help).
There was a problem hiding this comment.
I haven't tested an upgrade (I probably should just do that anyway), but this seems to work in i18next.d.ts:
interface CustomTypeOptions {
allowObjectInHTMLChildren: true;
}
There was a problem hiding this comment.
@canac Curious if you have a source on how upgrading the i18n libraries would fix this issue?
There was a problem hiding this comment.
I believe Claude told me so when I was researching this a few months ago. I don't believe I got far enough to prove it correct or incorrect. If it doesn't seem like it will help, please ignore that suggestion, and sorry if it sent you down a rabbit trail. If allowObjectInHTMLChildren: true works, it's definitely a simpler solution!
The docs seem to indicate that TypeScript errors are expected on the latest version and that allowObjectInHTMLChildren works but has tradeoffs: https://react.i18next.com/latest/trans-component#typescript-usage
zweatshirt
changed the title
<Trans> to always have a t={t] prop, ensure {{ name }} and not {name} in <Trans>{{ name }} and not {name} in <Trans>
2 participants
Description
{{ name }}instead of{name}which is just a JSX expression.https://jira.cru.org/browse/MPDX-9933
https://jira.cru.org/browse/MPDX-9934
Testing
Links broke in particular. I have already tested this, but a second set of eyes can help.Checklist:
/quality:agent-reviewcommand locally and fixed any relevant suggestions