Repository navigation
refactor(dashboard): drop ObjectPivotTable's i18n-resolver ref workaround - #5851
Merged
Merged
Conversation
…ound The metadata-derivation effect held `fieldLabel` / `fieldOptionLabel` behind refs so it would not have to declare them, because `useSafeFieldLabel()` returned a fresh object every render outside an i18next provider — a direct dependency would have re-run the effect on every render, and since it ends in two `setState` calls with freshly built objects, each run scheduled the next one. `useObjectLabel`'s memo now holds on both paths (objectui#5564), so the refs are dead weight. Removing them also restores the re-derivation the refs suppressed: a pivot rendered across a language switch now updates its header and option labels instead of serving the old resolver's output. Mirrors the same removal in `ObjectChart` (objectui#5587). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EuPCi56cnGyykygi3z9w4m
Contributor
✅ Console Performance Budget
The eager closure is every chunk the entry reaches through static imports — what the browser fetches and parses before the app renders. The entry chunk on its own is a small fraction of it. 📦 Bundle Size Report
Size Limits
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #5625
Removes the hand-rolled ref indirection around the two i18n resolvers in
ObjectPivotTable, mirroring the removal #5587 landed inObjectChart(PR #5628) one package over.
Why the refs were there, and why they no longer buy anything
The metadata-derivation effect held
fieldLabel/fieldOptionLabelbehindfieldLabelRef/fieldOptionLabelRefplus auseEffectkeeping them current,so it would not have to declare them. The stated reason was that the i18n hook
"returns fresh function identities each render" — true at the time:
useSafeFieldLabel()returned a fresh object on every render outside an i18nextprovider.
That made a direct dependency genuinely unsafe here, and in a sharper way than
in
ObjectChart. This effect ends in twosetStatecalls(
setFieldLabelMaps/setFieldNameLabels) with freshly built objects, soeach run scheduled a re-render, which produced fresh resolver identities, which
re-ran the effect: an unbounded derive loop rather than a merely wasteful one.
useObjectLabel's memo now pins both of its dependencies to module constantswhile no i18next instance is bound (#5564, PR #5585), so the resolvers have a
stable identity on both paths.
packages/i18n/src/useObjectLabel.tscarriesthat fix on
maintoday anduseObjectLabel-identity-5564.test.tsxpins it,including through
useSafeFieldLabelspecifically. Precondition re-verified oncurrent
mainbefore anything was removed.What the removal buys — the user-visible half
A ref-hidden dependency meant the derivation did not re-run when the
resolver genuinely changed. A pivot mounted before its
I18nProvider, orrendered across a language switch, kept showing its top-left header label and
its select-option cell labels as resolved by the old resolver, until some
unrelated dependency (the data source, the object name) happened to move. It
now re-derives exactly once on that transition.
The two cautions the card raised, as tests
ObjectPivotTable.i18nResolverDeps.test.tsx(new) asserts a derivationcount, not rendered output — nothing renders wrong under the loop, so an
output assertion is green against the defect:
getObjectSchemacall count across mount plus three forced re-renders, oncewith no provider (asserting the raw labels, so a leaked process-global
fails loudly instead of silently duplicating the other case) and once inside
one (asserting the localized labels, so the two cases really exercise the two
identity paths).
rowLabelsidentity (
fieldLabelMaps), one distinctrowFieldLabel(
fieldNameLabels), and a render count that stops growing over a quietwindow. The settle question the card flagged is answered: the maps do settle,
because the effect runs once per genuine resolver change.
The no-provider case runs first, because
createI18nregisters its instanceas react-i18next's process-global and that registration survives
cleanup().Reverse verification
Both legs on real commits, mutation confirmed on disk by grep rather than by an
editor's exit code, each leg driven by a script with a
trap … EXIT INT TERMrestore.
@object-ui/i18nis vitest-aliased topackages/i18n/src, so bothmutations are live from source with no rebuild step to get wrong.
ObjectPivotTable.tsx(git checkoutthe base rev;fieldLabelRefback to 3 occurrences, direct-deps line to 0)AssertionError: expected 'Stage (en)' to be 'Stage (ja)'. 1 failed, 2 passed.useObjectLabel.ts(NO_INSTANCE_T;to 0 occurrences,const t = boundT;to 1)AssertionError: expected 258 to be 1, i.e. 258 derivations instead of one: the loop returns. 1 failed, 2 passed.git diff HEAD --statempty.Legs that do NOT discriminate, named rather than implied: under the first
ablation the two counting cases stay green — the ref version also derives
exactly once, which was the whole point of the ref — so only the language-switch
case speaks for the source change. Under the second ablation the two
with-provider cases stay green, because that identity path was stable before
#5585 too; only the no-provider case speaks for it. Neither leg alone covers the
change; the pair does.
Verification, all at
b83dd427cpnpm exec vitest run packages/plugin-dashboard/ packages/i18n/— 129 testfiles, 1607 tests passed.
pnpm --filter @object-ui/plugin-dashboard type-check— exit 0, afterpnpm --filter '@object-ui/plugin-dashboard^...' build. Output echoed> tsc --noEmit && tsc -p tsconfig.test.json, so it was not a zero-matchno-op.
pnpm lint(repo-wideeslint . --no-inline-config) — 0 errors repo-wide.No
react-hooks/exhaustive-depsfinding on the changed files: the new deparray is honest and needed no suppression.
node scripts/check-changeset-presence.mjs— OK, 1 changeset declared.check:control-bytes(4870 files),check:phantom-deps,check:self-import,check:i18n-keys— all OK.No type was widened, no pin loosened, no assertion weakened, no exemption added.
Generated by Claude Code
Generated by Claude Code