finding(charts): ObjectChart's fieldOptionLabel ref workaround is removable once the i18n hook's memo holds without a provider

Observation-class finding, filed unassigned while implementing #5564. Not queued — this is a cleanup that only becomes correct after #5564 lands.

Blocked-by: #5564

What is there

packages/plugin-charts/src/ObjectChart.tsx:268-271 keeps a hand-rolled ref around one i18n resolver, with this comment:

Keep a stable ref to fieldOptionLabel — the i18n hook returns a fresh function reference on every render, which would otherwise invalidate fetchData's useCallback identity and trigger an infinite refetch loop.

That is an independent consumer diagnosing, and locally working around, exactly the defect #5564 measures: useSafeFieldLabel() returned a fresh object every render outside an i18next instance, so every closure on it was fresh too.

Why it is now removable

#5564 (PR #5585) stabilises useObjectLabel's memo on both paths, so fieldOptionLabel now has a stable identity with or without a provider. The ref indirection — fieldOptionLabelRef plus the useEffect that keeps it current — no longer buys anything, and it costs the usual price of a ref-hidden dependency: fetchData no longer re-runs when the resolver genuinely does change, which is now a real possibility rather than a permanent one.

Deliberately not removed in PR #5585: that PR's fence is packages/i18n, and removing a refetch-loop guard is a change that wants its own review and its own test rather than riding along in a diff about memo identity.

Suggested shape

Confirm the loop does not return: drop the ref, depend on fieldOptionLabel directly in fetchData's useCallback, and pin it with a test that counts fetches across re-renders both inside and outside a provider.
While there, packages/plugin-timeline/src/ObjectTimeline.tsx:25-37 carries a third hand-rolled wrapper (useSafeObjectLabel, a local useObjectLabel() ?? OBJECT_LABEL_FALLBACK with a 3-member fallback) duplicating useSafeFieldLabel from @object-ui/i18n. That one is a duplication question rather than a workaround question, so it may deserve a separate card — noting it here so the sweep is not lost.

Neither is urgent; both are dead weight that reads as load-bearing to the next person, which is the cost worth removing.

Triage (first-touch grading, 2026-08-21): finding → pm:blocked, type Task. The body already carries the machine-readable Blocked-by: #5564 line and the card is correct that the cleanup is only safe after that fix lands — removing a refetch-loop guard while the memo is still unstable would reintroduce the loop it guards. The unlock scan will return this to the queue when #5564 closes; on return, re-verify on the merged ref that PR #5585's memo stabilisation actually landed on both paths, then queue with the card's own test requirement (fetch-count pin across re-renders, inside and outside a provider).

Second item noted on the card (ObjectTimeline's third hand-rolled wrapper duplicating useSafeFieldLabel) is a separate question — on unlock, the returning triage pass should decide whether it gets its own card rather than letting it ride silently.

Generated by Claude Code

Unlock (same triage round, minutes after the block was placed): #5564 closed during this round — fix PR #5585 is on origin/main (38a9568, "hold useObjectLabel's memo when no i18next instance is bound"), which is exactly the condition this card's Blocked-by: #5564 waited for. Both release checks pass (the blocked-transition comment above is the latest condition on this card; no newer merged PR landed on the card after it). pm:blocked → pm:queue.

Claim-time requirements stand as written in the grading comment: verify the memo holds on both paths on the merged ref, then remove the ref indirection with a fetch-count pin across re-renders inside and outside a provider. Size/model suggestion: S–M. The ObjectTimeline third-wrapper question noted on the card still wants its own card decision from whoever implements this.

Generated by Claude Code

Claim — domain:ui execution seat (#5560), round 8.

Session: session_012u2pRjcqAYtoEjgr3wwhnK
Branch: claude/issue-5587-objectchart-ref-workaround
Label: pm:queue → pm:dispatched

Unlock confirmed: #5564 is closed and PR #5585 is on origin/main at 38a9568, so the workaround's precondition is gone.

Scope fence for this dispatch: the ObjectTimeline third-wrapper question noted on the card is out of scope here and is not to be folded in. Per the triage note it wants its own card decision — the dev files it as a fresh card and links it back to this one rather than answering it inside this PR.

Generated by Claude Code

refactor(charts): drop ObjectChart's fieldOptionLabel ref workaround

chore(charts): changeset for the ObjectChart fieldOptionLabel ref rem…

{
  "issue": 5587,
  "status": "done",
  "premise_still_valid": true,
  "branch": "claude/issue-5587-objectchart-ref-workaround",
  "pr_url": "https://github.com/objectstack-ai/objectui/pull/5628",
  "head_sha": "337f55d81",
  "files_changed": [
    "packages/plugin-charts/src/ObjectChart.tsx",
    "packages/plugin-charts/src/__tests__/ObjectChart.fieldOptionLabelRefetch.test.tsx",
    ".changeset/objectchart-fieldoptionlabel-ref-5587.md"
  ],
  "what_changed": "Removed the ref indirection around fieldOptionLabel in ObjectChart: deleted the 3-line comment, fieldOptionLabelRef, and the useEffect keeping it current; rewrote the single read at line 650 to call fieldOptionLabel directly; added fieldOptionLabel to fetchData's useCallback dep array; dropped the now-unused useRef from the React import. The pre-existing exhaustive-deps disable stays (it covers older omissions on that list). Added a fetch-count pin test and a patch changeset.",
  "evidence": {
    "verified_line_numbers": "Card cited 268-271; located by code against origin/main 8c87f0583 the workaround is 269-275 plus one read at 650. Line 268 is the useSafeFieldLabel() call itself and is KEPT. 269-271 comment, 272 useRef, 273-275 useEffect. So the card's range starts one line early and stops four lines short of the useEffect it describes.",
    "pr5585_diff_why_precondition_gone": "useSafeFieldLabel() = useObjectLabel() ?? SAFE_FIELD_LABEL_FALLBACK, and useObjectLabel returns useMemo(() => {...}, [t, i18n]) (useObjectLabel.ts:116 .. :675). fieldOptionLabel is a closure built inside that memo, so its identity IS the memo's. Before #5585 the deps came straight from useObjectTranslation -> react-i18next useTranslation, which with no bound instance rebuilds its return from a fresh {} each render, so i18n was fresh every render and the memo never held — exactly the condition ObjectChart's comment names. PR #5585 (38a956877, verified ancestor of this branch via git merge-base --is-ancestor) inserted useObjectLabel.ts:108-110: `const bound = hasUsableI18nInstance(boundI18n); const t = bound ? boundT : NO_INSTANCE_T; const i18n = bound ? boundI18n : NO_INSTANCE_I18N;` where NO_INSTANCE_T (:72) and NO_INSTANCE_I18N (:77) are MODULE-LEVEL constants. Unbound, both deps are process-lifetime constants so the memo holds; bound, they are the live values which were already stable. One identity on both paths — precondition gone. Confirmed empirically by the ablation, not only read off the diff.",
    "ablation": "With this PR's ObjectChart.tsx in place, packages/i18n/src/useObjectLabel.ts was reverted to 38a9568~1 and the same vitest command re-run. Mutation proved ON DISK with anchored greps before measuring (hasUsableI18nInstance 0 hits, NO_INSTANCE_T 0 hits, pre-#5585 guard line 1 hit) — an editor exit code was not treated as proof. Result: `AssertionError: expected 2 to be 1`, `Test Files 1 failed (1)` / `Tests 1 failed | 1 passed (2)`, with 4x React 'Maximum update depth exceeded' in the same log. OBSERVED DIRECTION, not the template's: only the NO-PROVIDER case goes red; the with-provider case stays green, which is correct because #5585 only changed the unbound path. No rebuild was needed and none was skipped: vitest.config.mts aliases @object-ui/i18n to packages/i18n/src, so a root-launched run reads source, not dist — and the ablation flipping red is itself the proof (a dist-reading run would have stayed green). Restore leg ran from a `trap ... EXIT INT TERM`: git status clean, marker back (5 hits), git diff --quiet origin/main on that file reports identical.",
    "over_delete_sweep": "One sweep() function, whole repo, excluding node_modules/dist/.git/.turbo/coverage/build. PROBE `fieldOptionLabelRef` = 6 hits: 3 in ObjectChart.tsx (all deleted or rewritten here) and 3 in packages/plugin-dashboard/src/ObjectPivotTable.tsx, which is a SEPARATE module-local symbol of the same name, not a reader of ObjectChart's. grep -c on the edited file after the change = 0. Nothing else in the repo read what was deleted. useRef was checked the same way before removing it from the import (appeared only on the import line and line 272).",
    "control_probe": "IDENTICAL scope and flags to the probe — same sweep() function, same root, same --exclude-dir set — differing ONLY in the search pattern: `fieldOptionLabel` returned 102 hits, grep exit 0. That is what makes the probe's 6 readable as a measurement rather than a silently-empty search. (A control at a different scope cannot fail and would prove nothing.)",
    "dist_measurement": "Both legs built and measured at the REAL packages/plugin-charts/dist/ path (no renamed copies): dist/index.js 66269 -> 66201 = -68 B DIFFERENT; dist/index.umd.cjs 49613 -> 49549 = -64 B DIFFERENT; dist/index.d.ts 559 -> 559 = 0, IDENTICAL sha256. Leg B mutation proved on disk (fieldOptionLabelRef back to 3 hits, useRef import back to 1). dist rebuilt afterwards to leg A's hash 6a52ea21f99ba7a8.",
    "pr_body_revision": "PR #5628's body was revised once after the PM note to quote real code instead of describing it: the deleted ObjectChart.tsx:269-275 block verbatim in a fenced tsx block, the before/after of the line-650 read, the before/after of fetchData's dep array, and the two test wrappers as real JSX (`(chart) => chart` versus `<I18nProvider instance={instance} persistLanguage={false}>{chart}</I18nProvider>`). Both replacements were anchored and asserted to match exactly once before writing. Read back from the REST API afterwards: the JSX open tag, the closing `</I18nProvider>`, and `const fieldOptionLabelRef = useRef(fieldOptionLabel);` are all present in the stored bytes, so tag-shaped text does reach GitHub intact. PR is still draft, still open, not merged."
  },
  "new_issue_filed": {
    "number": 5623,
    "url": "https://github.com/objectstack-ai/objectui/issues/5623",
    "title": "finding(timeline): ObjectTimeline hand-rolls a third `useSafeFieldLabel`, with a 3-member fallback where the shared one has 5"
  },
  "changeset": {
    "present": true,
    "path": ".changeset/objectchart-fieldoptionlabel-ref-5587.md",
    "bump": "patch @object-ui/plugin-charts",
    "why": "check-changeset-presence.mjs is the authority and is decisive on its own; its verdict line: '2 source file(s) of 1 released package(s) changed, and this change declares 1 changeset(s): .changeset/objectchart-fieldoptionlabel-ref-5587.md.' Independently earned by behaviour: the ref-hidden dependency meant fetchData did NOT re-run when the resolver changed, so a chart mounted before its I18nProvider or rendered across a language switch kept stale groupBy labels until an unrelated dep moved; it now refetches once on that transition. Per the PM's mid-flight correction the zero .d.ts delta was NOT reasoned from — the .d.ts is byte-identical exactly as expected for an internal-ref deletion, while the emitted JS shrank, which is the signal that the deleted code was reachable."
  },
  "tests": {
    "command": "pnpm exec vitest run packages/plugin-charts/ --maxWorkers=2  (repo root; package-cwd vitest is refused by the repo guard)",
    "result": "Test Files 34 passed (34) / Tests 239 passed (239), re-run at final commit 337f55d81. New file: packages/plugin-charts/src/__tests__/ObjectChart.fieldOptionLabelRefetch.test.tsx, 2 cases counting ds.aggregate and ds.getObjectSchema calls across 3 forced re-renders, one with no provider and one inside I18nProvider with a bound instance. The no-provider case runs FIRST and asserts the RAW option label, because createI18n registers a react-i18next process-global — a leak makes that case fail rather than silently become a duplicate of the bound one. Type-check: pnpm --filter @object-ui/plugin-charts type-check -> tsc --noEmit, exit 0 (script name echoed, so not a zero-match no-op)."
  },
  "gates": [
    {
      "name": "check-changeset-presence",
      "status": "pass",
      "verdict": "2 source file(s) of 1 released package(s) changed, and this change declares 1 changeset(s)"
    },
    {
      "name": "check:control-bytes",
      "status": "pass",
      "verdict": "OK (scanned 4673 tracked text file(s); skipped 85 binary)"
    },
    {
      "name": "check:phantom-deps",
      "status": "pass",
      "verdict": "Every in-scope import is declared by the package that publishes it."
    },
    {
      "name": "check:self-import",
      "status": "pass",
      "verdict": "No package names itself inside its own src/."
    },
    {
      "name": "check:action-forward-parity",
      "status": "pass",
      "verdict": "exit 0 — note it CANNOT detect an over-delete; the sweep above is the real evidence"
    },
    {
      "name": "check:i18n-keys",
      "status": "pass",
      "verdict": "Every in-scope call-site key resolves against the en pack (2918 keys)"
    },
    {
      "name": "check:i18n-drift",
      "status": "pass",
      "verdict": "No en value changed in this range."
    },
    {
      "name": "check:spec-symbols",
      "status": "pass",
      "verdict": "spec symbol derivation: 1290 files scanned against 4912 spec export names"
    },
    {
      "name": "check:doc-types",
      "status": "pass",
      "verdict": "Every documented component type is registered."
    },
    {
      "name": "check:esm-specifiers",
      "status": "pass",
      "verdict": "no un-ledgered package emits an extensionless relative specifier"
    },
    {
      "name": "check:skills-paths",
      "status": "pass",
      "verdict": "OK (95/96 stated path(s) resolve; 1 baselined) — governed surface untouched"
    },
    {
      "name": "check:published-dist",
      "status": "pass",
      "verdict": "No published package's build output carries tooling material."
    },
    {
      "name": "check:eager-closure",
      "status": "broken-gauge",
      "verdict": "exit 2 — 'No eager-closure report at apps/console/dist/eager-closure.json ... This is a broken gauge, not a passing budget.' Pre-existing; needs a console build."
    },
    {
      "name": "check:doc-snippets",
      "status": "broken-gauge",
      "verdict": "exit 1 — 'The snippet program was NOT run: the packages it resolves against are not built'. Pre-existing."
    },
    {
      "name": "eslint (declared narrowing)",
      "status": "pass-narrowed",
      "verdict": "47 files linted, 0 errors, 273 pre-existing warnings in packages/plugin-charts, counted from --format json. CI's pnpm lint is turbo run lint, a per-package fan-out; the package task covering this diff ran in full. Invariance for untouched files: eslint.config.js declares no parserOptions.project/projectService (no type-aware linting) and no rule in eslint-rules/*.js reads the filesystem or holds cross-file state, so each file's verdict is a function of its own text plus the shared config; this diff touches two .tsx files inside plugin-charts plus one .changeset/*.md outside the **/*.{ts,tsx} glob."
    },
    {
      "name": "CI check runs",
      "status": "in_progress",
      "verdict": "Reported at draft-PR time per the standing contract; CI convergence is the PM's read, not a wait this seat performs."
    }
  ],
  "scope_fence_respected": {
    "objecttimeline_untouched": true,
    "note": "packages/plugin-timeline/src/ObjectTimeline.tsx is not in the diff (3 files changed, all listed above). Filed as #5623 instead, unassigned, labelled `finding` only."
  },
  "out_of_scope_findings": [
    "filed as #5623: ObjectTimeline hand-rolls a third useSafeFieldLabel with a 3-member `as any` fallback where the shared SAFE_FIELD_LABEL_FALLBACK has 5; @object-ui/react already re-exports the shared hook on an import the file already has (the fence-mandated card)",
    "filed as #5625: ObjectPivotTable (packages/plugin-dashboard/src/ObjectPivotTable.tsx:83-93) carries the SAME class of ref workaround with the same now-gone precondition — found by the over-delete sweep. Not folded in: different package, different suite, and the ref there hides the resolvers from a metadata-derivation effect rather than a fetch callback, so it changes different behaviour and wants its own before/after test (bounded-in-place-fix condition on gate family / verification surface fails)."
  ],
  "open_questions": [],
  "pm_notes_applied": {
    "changeset_correction": "Applied. The changeset does not rest on the .d.ts delta. check-changeset-presence.mjs is quoted as the authority, and the behavioural justification is stated independently. The zero .d.ts delta is reported as the expected result for an internal-ref deletion, not as a reason.",
    "marker_readback": "No action was needed and none was taken. The report comment was posted ONCE, and both the write and the read-back went through the REST API directly (curl), not the MCP body reader — so the sanitizing reader the note describes was never in the path. Read-back returned the marker as the literal first line with a body length of 11948 bytes, byte-identical to what was sent. No rewrite, no duplicate report comment exists.",
    "footer_duplication": "Confirmed on this PR and left alone. Immediately after creation the body carried 0 bare footers and 1 session-URL footer. After the body edit below it carries 1 bare + 1 session-URL — the server-side append, observed rather than assumed. Not touched, per the standing lane ruling. This also supersedes the footer question this report previously listed as open; it has been removed.",
    "jsx_not_paraphrased": "Applied to the PR body — see evidence.pr_body_revision. Verified in stored bytes that the angle brackets survived."
  },
  "report_delivery": "Posted once as issue comment 5374241066 on #5587 with `<!-- os-dev-report -->` as the literal first line, then updated in place (PATCH on the same comment id) to carry this pm_notes_applied section — deliberately an edit rather than a second comment, so exactly one marker-bearing report comment exists for the PM's scan."
}```


---
_Generated by [Claude Code](https://claude.ai/code)_

refactor(charts): drop ObjectChart's fieldOptionLabel ref workaround (o…

Navigation Menu

Repository navigation

finding(charts): ObjectChart's fieldOptionLabel ref workaround is removable once the i18n hook's memo holds without a provider #5587

What is there

Why it is now removable

Suggested shape

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

finding(charts): ObjectChart's fieldOptionLabel ref workaround is removable once the i18n hook's memo holds without a provider #5587

Description

What is there

Why it is now removable

Suggested shape

Activity

huangyiirene commented on Aug 21, 2026

huangyiirene commented on Aug 21, 2026

os-sales commented on Aug 21, 2026

claude commented on Aug 21, 2026

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions