MPDX-9933 - Enforce t in <Trans> - #1986
Conversation
zweatshirt
added
the
Preview Environment
|
Preview branch generated at https://enforce-t-in-trans.d3dytjb8adxkk5.amplifyapp.com |
Bundle sizes [mpdx-react]Compared against e02be0d
|
zweatshirt
changed the title
<Trans>
zweatshirt
left a comment
zweatshirt
left a comment
There was a problem hiding this comment.
Multi-Agent Review — APPROVED WITH SUGGESTIONS
6 agents (Architecture, Testing & Quality, Standards, UX, Financial Reporting, Dependency Impact) · 0 blockers · 5 suggestions, highest severity 5.5 · Risk: MEDIUM
The PR does exactly what it claims, and the claim is machine-verifiable rather than asserted:
| Check | Result |
|---|---|
yarn lint:ci (read-only) |
0 errors, 2244 files |
yarn lint:ts (tsc) |
exit 0 |
eslintTransRules.test.ts |
11/11 pass |
| Component suites, all changed trees | 466 tests pass, no timeouts |
<Trans> missing t, repo-wide |
0 of 108 (63 files) |
| Rule-fires probe | <Trans>x</Trans> flagged; <Trans t={t}>x</Trans> clean |
"Fix one, fix all" passed — this is the strongest signal in the review. The PR adds an error-level rule and then satisfies it everywhere, so CI will not break on merge. Four already-compliant files outside the diff (AccountListRow, AccountListInvites, ContactRow, GoalCard) were correctly left alone.
Behavior-neutral, and slightly positive. No changed file uses a namespaced useTranslation('ns'), and keySeparator/nsSeparator are both false, so t={t} cannot shift key resolution. Bonus: EligibleDisplay, EffectiveDateBanner and SuggestedContactStatus previously had no useTranslation() at all and so never subscribed to languageChanged — they could stay stuck on English after a mid-session language switch. They now re-render.
Financial Reporting agent was triggered by HrTools/** + the keywords amount/total/value, and reports a false positive: no arithmetic, aggregation, currency, rounding, or date-window expression is added, removed, or modified anywhere in the diff.
Debate resolution — the one genuine disagreement
Three agents converged on the same defect (see the .eslintrc.js comment) but proposed conflicting fixes. Resolved against the real ESLint rather than by consensus:
| Proposal | Verdict |
|---|---|
:has(> JSXAttribute[name.name='t']) |
Invalid — esquery in ESLint 8.57 rejects a leading > inside :has(); throws SyntaxError at position 46 |
[parent.name.name='Trans'] |
Valid, 0 false positives across all 64 <Trans> files — but narrower than its author claimed |
Both forms were probed directly:
<Trans components={{ x: <Foo t={t}/> }}> -> fixed by [parent...] OK
<Trans components={{ x: <Trans t={t}/> }}> -> still missed --
The second escapes because the inner Trans's own t also satisfies [parent.name.name='Trans']. That form is pathological and absent from the repo; the first is the realistic one.
Pre-existing, informational — not part of this PR's scope
These sit in touched files but predate this PR and do not count toward the verdict. Listing them here rather than as inline comments, since they belong in separate tickets — not in an i18n conformance sweep.
SuggestedContactStatus.tsx:37-55— rules-of-hooks violation (8.0/10). Four hooks sit below an earlyreturn nullgated oncontactIds, a live Formik value. Going 1 → 2 selected contacts on a mounted modal throws "Rendered fewer hooks than expected." Nothing catches it:eslint-plugin-react-hooksis neither installed nor configured repo-wide. This PR's change here is correct — its newuseTranslation()is placed above the guard, leaving the hook-count delta unchanged. Worth its own ticket.transKeepBasicHtmlNodesForextraction/runtime mismatch. It is not configured anywhere in the repo, so the runtime keeps<strong>/<br>/<i>/<p>whilei18next-parserindexes them as<2>. Affected keys can never match, so those strings are untranslatable in production (users still see readable English via thedefaultValuefallback, so nothing is visibly broken). Seen atEligibleDisplay.tsx:19, and via the single-brace{above}/{after}form atRequestSummaryCard.tsx:88andAboutForm.tsx:93. If the deferred #1977 work doesn't cover this, it needs a ticket.EligibleDisplay.tsx:19-29— two adjacent<Trans>blocks render with no space:...button below.If you would like....
Explicitly cleared — not findings
Checked and dismissed so no reviewer re-raises them: two top-level describes in the test file (34/816 test files already do this); spread props being flagged (zero <Trans {... in repo); aliased/namespaced Trans escaping the selector (zero instances, and the pre-existing sibling i18nKey rule has the identical limitation).
Dependency impact
No breaking changes — every export/interface/React.FC line across all 23 files is byte-identical to merge-base e02be0d02. No CRITICAL-impact files; widest is Calculation.tsx (9 direct / 22 transitive). NullState.tsx is not the widely-imported one — that's the untouched NullStateBox.tsx; CreateButton is module-local and never exported.
Two things not verified first-hand, stated rather than papered over: whether yarn extract output changes for finish.page.tsx (locales are Crowdin-owned, so no extract was run), and two react-i18next v11 internals (namespace resolution order, nodesToString keep-vs-index) — inferred from library semantics plus corroborating extracted-key shapes, and both concerning pre-existing behavior.
Tooling note, unrelated to the diff: yarn lint is eslint ... --quiet --fix and mutates the working tree. Use yarn lint:ci when reviewing.
| {isContactType && ( | ||
| <Typography variant="body2"> | ||
| <Trans | ||
| t={t} |
There was a problem hiding this comment.
ContactPair.tsx and StickyConfirmButtons.tsx are the only 2 of 23 changed components without a colocated test. Their parents (MergeContacts.tsx, MergePeople.tsx) do have tests, but those assert none of the affected strings — grep for Showing / Source: in both test files returns nothing.
Low severity because the properties that matter here are already verified globally: tsc is clean (so t resolves — here it comes from the explicit t: TFunction prop on ContactItem at line 137, not a hook, which is correct pre-existing threading), and the sweep is provably behavior-neutral. Flagging only so you know this is where a rendering regression would go unnoticed.
| <Box> | ||
| <Typography> | ||
| <Trans | ||
| t={t} |
There was a problem hiding this comment.
There was a problem hiding this comment.
/dismiss out of scope
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI Review Auto-Approval
Risk Level: MEDIUM (4/10)
Verdict: APPROVED_WITH_SUGGESTIONS (suggestions posted, no blockers)
This PR was auto-approved because:
- The multi-agent AI review determined it is medium risk
- No blocking issues were found
- All suggestions have been posted as review comments for the developer to consider
If you believe this PR needs human review, dismiss this approval and request a review manually.
…corresponding tests
23b20d6 to
1acb149
Compare
…ation after early return
1 participant
Description
tprop{{ name }}and not{name}in<Trans>#1977 for this ticket since some of the work there is complicated and this can be deployed separatelyTesting
Checklist:
/quality:agent-reviewcommand locally and fixed any relevant suggestions