`useSafeFieldLabel()` returned a fresh object on every render outside an
i18next provider, so a direct dependency on `fieldOptionLabel` made
`fetchData` fresh every render and the effect depending on it refetched
without bound. ObjectChart worked around that locally with a ref plus an
effect keeping it current.
`useObjectLabel`'s memo now holds on both paths, so the resolver has a
stable identity with or without a provider and the indirection buys
nothing. It costs something, though: a ref-hidden dependency means
`fetchData` does NOT re-run when the resolver genuinely changes, which is
now a real possibility rather than a permanent impossibility.
Depend on `fieldOptionLabel` directly and pin the result with a
fetch-count test across forced re-renders, inside and outside a provider.
Part of #5587
Fixes#5587
All measurements below were taken at
337f55d8, the head of this branch.What changed
packages/plugin-charts/src/ObjectChart.tsxheld itsfieldOptionLabelresolver behind a ref. That ref is gone;fetchDatanow depends on the resolver directly.fieldOptionLabelRef+ theuseEffectkeeping it currentfieldOptionLabelRef.current(...)→fieldOptionLabel(...)fieldOptionLabeltofetchData'suseCallbackdep array (the pre-existingexhaustive-depsdisable stays — it covers other, older omissions on that list, which this PR does not touch)useReffrom the React import; it was there for this ref aloneThe block that came out, verbatim (
ObjectChart.tsx:269-275; line 268, the hook call, stays):Its one read, at line 650, and what replaced it:
and the dependency that was hidden behind it is now declared:
Verified line numbers
The card cited
ObjectChart.tsx:268-271. Located by code againstorigin/mainat8c87f0583, the workaround is lines 269-275:const { fieldOptionLabel } = useSafeFieldLabel();— the hook call, keptconst fieldOptionLabelRef = useRef(fieldOptionLabel);useEffectwriting.currentfieldOptionLabelRef.current(schema.objectName, groupByField, value, fallback)So the card's range starts one line early (it includes the hook call, which stays) and stops four lines short of the
useEffectit describes.Why the precondition is gone — from PR #5585's diff, not from #5564 being closed
The workaround's own comment states the precondition: "the i18n hook returns a fresh function reference on every render". That is a claim about one memo, and PR #5585 (
38a956877, an ancestor of this branch — confirmed withgit merge-base --is-ancestor) is what falsified it.useSafeFieldLabel()isuseObjectLabel() ?? SAFE_FIELD_LABEL_FALLBACK, anduseObjectLabelreturnsuseMemo(() => {…}, [t, i18n])(useObjectLabel.ts:116…:675).fieldOptionLabelis a closure built inside that memo, so its identity is exactly the memo's identity.Before #5585,
useObjectLabelopened withconst { t, i18n } = useObjectTranslation()and passed those straight into the dep list.useObjectTranslationdelegates to react-i18next'suseTranslation, which with no bound instance rebuilds its return value from a fresh{}every render. Soi18narrived with a new identity each render, the memo never held, and every closure on it —fieldOptionLabelincluded — was fresh each render. That is precisely the condition ObjectChart's comment describes.After #5585, the same function reads (
useObjectLabel.ts:108-110):NO_INSTANCE_T(:72) andNO_INSTANCE_I18N(:77) are module-level constants, so while nothing is bound both memo dependencies carry one identity for the life of the process and the memo holds. When an instance is bound they are the live values, which were already stable. Both paths, one identity — the precondition is gone.This is asserted empirically rather than only read off the diff; see the ablation below.
The test that pins it
packages/plugin-charts/src/__tests__/ObjectChart.fieldOptionLabelRefetch.test.tsxcounts fetches across forced re-renders, not rendered output — nothing renders wrong when the loop is present, the chart just refetches forever, so an output assertion would be green against the defect.Two cases, one per identity path. Each mounts, waits for the first load, forces three more renders of the same tree, and asserts
ds.aggregateandds.getObjectSchemawere each called exactly once. The only difference between them is the wrapper:The no-provider case runs first and asserts the raw option label (
Won), becausecreateI18nregisters its instance as react-i18next's process-global; if one ever leaked into that case, the assertion fails rather than the case quietly becoming a second test of the bound path. The provider case asserts the localized label, so the two cases demonstrably exercise different paths.Ablation — the test fails against the pre-#5585 code
With this PR's
ObjectChart.tsxin place,packages/i18n/src/useObjectLabel.tswas reverted to38a9568~1and the same command re-run.Mutation confirmed on disk before measuring, anchored on the text #5585 added and removed (an editor's exit code proves nothing):
Result:
alongside 4 × React
Maximum update depth exceededin the same log — the refetch loop, caught by React's own guard, which is why the count reads 2 rather than climbing without bound.Direction, as observed rather than as predicted: only the no-provider case goes red. The with-provider case stays green, and that is correct — #5585 only changed the unbound path; the bound path's memo was already holding. So one case is the unlock pin and the other is a regression guard on the path that already worked.
No rebuild was needed for this ablation and none was skipped:
vitest.config.mtsaliases@object-ui/i18ntopackages/i18n/src, so a root-launched run reads that source file directly rather thandist/. The ablation flipping red is itself the proof of that — a run readingdist/would have stayed green. The restore leg ran from anEXIT INT TERMtrap and is verified:git statusclean, marker back (5 hits), andgit diff --quiet origin/main -- packages/i18n/src/useObjectLabel.tsreports identical.Over-delete sweep, with a same-scope control
check-action-forward-parityand gates like it derive their owed set from what the runtime reads, so deleting a read shrinks the set instead of failing. They cannot catch an over-delete, so the sweep is the evidence.One
sweep()function; probe and control differ only in the search pattern — same root, same--exclude-dirset, same flags:fieldOptionLabelRef→ 6 hits: 3 inObjectChart.tsx(all three deleted or rewritten here) and 3 inpackages/plugin-dashboard/src/ObjectPivotTable.tsx, which is a separate module-local symbol of the same name, not a reader of ObjectChart's.grep -con the edited file after the change: 0. Nothing else in the repo read what was deleted.fieldOptionLabel, identical scope and flags → 102 hits, exit 0.The control is what makes the probe's small result readable: the same command over the same tree does find things, so 6 is a measurement and not a silently-empty search.
useRefwas also checked before removing it from the import — it appeared only on the import line and on line 272.Changeset
.changeset/objectchart-fieldoptionlabel-ref-5587.md, patch to@object-ui/plugin-charts. The authority agrees:Both legs built and diffed at the real
packages/plugin-charts/dist/path:dist/index.jsdist/index.umd.cjsdist/index.d.tsThe emitted JS shrinks, which is what a deletion of reachable runtime code should do — had it not shrunk, the deleted code would have been unreachable or already bundled away and this PR would be claiming something different. The
.d.tsis byte-identical, as expected: an internal ref changes no exported type.The changeset is owed on the source-of-a-released-package rule and is independently earned by behaviour: a ref-hidden dependency meant
fetchDatadid not re-run when the resolver changed. A chart mounted before itsI18nProvider, or rendered across a language switch, kept serving groupBy labels resolved by the old resolver until some unrelated dependency happened to move. It now refetches once on that transition.Verification
pnpm exec vitest run packages/plugin-charts/(repo root)Test Files 34 passed (34)/Tests 239 passed (239)pnpm --filter @object-ui/plugin-charts type-checktsc --noEmit, exit 0check:control-bytes✅ OK (scanned 4673 tracked text file(s); skipped 85 binary)check:phantom-deps✅ Every in-scope import is declared by the package that publishes it.check:self-import✅ No package names itself inside its own src/.check:action-forward-paritycheck:i18n-keysEvery in-scope call-site key resolves against the en pack (2918 keys)…check:i18n-driftNo en value changed in this range.check:spec-symbols✅ spec symbol derivation: 1290 files scanned…check:doc-types✅ Every documented component type is registered.check:esm-specifiersno un-ledgered package emits an extensionless relative specifiercheck:skills-paths✅ OK (95/96 stated path(s) resolve…)— untouched, governed surfacecheck:published-dist✅ No published package's build output carries tooling material.check-changeset-presenceVitest ran from the repo root throughout; a package-cwd run is refused by the repo's own guard.
Broken gauges, unrelated to this diff — both report the documented "I did not run" state rather than a verdict:
check:eager-closureexits 2:❌ No eager-closure report at apps/console/dist/eager-closure.json … This is a broken gauge, not a passing budget.(needs a console build)check:doc-snippetsexits 1:The snippet program was NOT run: the packages it resolves against are not built…Declared narrowing — lint. CI's
pnpm lintisturbo run lint, a per-package fan-out ofeslint .; the package task covering this diff was run in full rather than the whole farm. Three pieces of evidence, so this reads as a measurement and not as "skipped":eslint.config.jswhosefilesglob is**/*.{ts,tsx}.--format json: 47 files linted, 0 errors, 273 warnings inpackages/plugin-charts. The warnings are pre-existingno-explicit-any; the 2 in the new test file arelet lastSchema: anyand theChartRenderermock's(props: any), the same two the siblingObjectChart.compareTo.test.tsxcarries.eslint.config.jsdeclares noparserOptions.project/projectService(grep empty) → no type-aware linting; and no rule ineslint-rules/*.jsreads the filesystem or holds cross-file state (grep forreadFileSync|globSync|process.cwd()empty) → each file's verdict is a function of its own text plus the shared config. This diff touches two.tsxfiles, both insidepackages/plugin-charts, plus one.changeset/*.mdthat falls outside thefilesglob entirely. No file outside that package can change verdict.Scope
ObjectTimelinewas not touched — per triage it wants its own card, filed as #5623 (third hand-rolleduseSafeFieldLabel, 3-member fallback where the shared one has 5).The over-delete sweep additionally turned up #5625:
ObjectPivotTablecarries the same class of ref workaround with the same now-gone precondition. Deliberately not folded in — different package, different suite, and the ref there hides the resolvers from a metadata-derivation effect rather than from a fetch callback, so it changes different behaviour and wants its own before/after test.Both are filed unassigned and labelled
findingonly.Generated by Claude Code
Generated by Claude Code