Uh oh!
There was an error while loading. Please reload this page.
refactor(plugin-charts): ObjectChart consumes the shared analytics label-net helpers (#4405) - #4412
Merged
Merged
Conversation
…lpers (#4405) objectui#4389 (PR #4404) retired two copies of the analytics label-net glue into `@object-ui/core` + `@object-ui/react`. `ObjectChart` was a third copy, which that card did not name and its PR left out of scope: a local `translatorFor` closure logically identical to the two deleted ones (down to the comment), a longhand `buildDimensionLabelMap` loop, and its own base-object-read-then-walk composition. It now calls core's `dimensionOptionTranslator`, `deriveDimensionLabelMaps` and `loadDimensionFieldMeta`. Pure function swap — same reads in the same order, same best-effort fallback, same locale-applying memo boundary. The package's 22 test files / 170 assertions pass unchanged and are byte-identical. The two bug-fix properties stay stated locally and undisturbed: `apiFetch` is in the effect's deps (objectui#4121), the locale is NOT (objectui#4030 / PR #4324). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Qqyix2QcnpUC9XeYVDzx3
The latest updates on your projects. Learn more about Vercel for GitHub. |
Contributor
✅ Console Performance Budget
📦 Bundle Size Report
Size Limits
|
yinlianghui
commented
Aug 12, 2026
CollaboratorAuthor
ACCEPT — PM 复核 (session
Flipping ready + arming auto-merge. Generated by Claude Code |
yinlianghui
marked this pull request as ready for review
August 12, 2026 04:34
Uh oh!
There was an error while loading. Please reload this page.
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
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#4405
objectui#4389 (PR #4404) named two copies of the analytics label-net glue and retired both. There was a THIRD, which that card did not name and its PR deliberately left out of scope:
packages/plugin-charts/src/ObjectChart.tsx. This retires it.Step 1 — the pure-function swap (mandatory, done)
ObjectChartcarried three pieces of the glue longhand. All three now call the helpers PR #4404 landed in@object-ui/core:ObjectChart.tsxtranslatorFor(path)closure — logically identical to the two deleted copies, down to the comment about binding to the owning objectdimensionOptionTranslator(meta, fieldOptionLabel)buildDimensionLabelMaploop accumulating per-dimension mapsderiveDimensionLabelMaps(metaByPath, relabel, fieldOptionLabel)loadObjectSchema(objectName)thenresolveDimensionFieldMeta(...)loadDimensionFieldMeta(loadObjectSchema, objectName, paths)The third row goes slightly beyond the two swaps the ruling enumerates. It is the same "consume the landed helpers" mandate from the card title, it is strictly mechanical (two statements collapse into the one composition that is those two statements), and the read count is identical because
resolveDimensionFieldMetamemoizes per call and seeds the base schema either way. Flagged explicitly here so it is trivial to veto — dropping it changes nothing else in this PR.@object-ui/corewas already a declared dependency of@object-ui/plugin-charts(verified againstpackage.json, per the #4394 phantom-dep lesson — and that gate landed inmainmid-task, so it also ran green against this branch after merging).The two bug-fix properties are untouched and remain stated locally, exactly as the card predicted for a step-1-only landing:
apiFetchis still in the effect's deps (objectui#4121) and the locale is still NOT (objectui#4030 / PR #4324). The diff does not touch the effect's dependency array.Step 2 — the dataset read onto
useDatasetDimensionMeta: attempted, DECLINED on measurementThe blocker is not the
fieldByDimmap to ordered-array translation the card worried about — that part is clean, since the shared hook already takes both adimension name to field pathmap and the name list, so the adapter is justObject.keys(fieldByDim).The blocker is that ObjectChart's colour dimension is not one of the dimensions the surface renders, and the shared hook only resolves dimensions it will also relabel.
Measured, on
mainas it stands: a dataset chart that declares nodimensionsstill resolves its colour map, because ObjectChart resolves[fieldName, ...values(fieldByDim)]wherefieldNamecomes from the dataset DEFINITION's first dimension, not fromschema.dimensions. I pinned this in a scratch test and it is green today — 2 reads, colours applied:Routed through
useDatasetDimensionMeta, that case passesdimensions: [], and the hook's effect bails before fetching:So the read count would go 2 to 1 and the colour map would silently stop resolving — a behaviour change, in the exact best-effort-so-it-fails-quietly way this family keeps getting bitten by. Preserving it requires smuggling the colour path through as a synthetic dimension and filtering it back out of the derived maps before
relabelDimensionssees it, which is a contorted adapter that changes the hook's meaning rather than adapting a shape.Two further costs, for completeness:
objectanddimensionFieldsalready resolved, but on the dataset path both come from the/api/v1/meta/dataset/...read that today lives inside the same effect. Using the hook forces that read into its own effect writing a new state that the hook then keys on: one effect and one state become two of each, and the metadata settles in two render commits instead of one.loadObjectSchemaplusloadDimensionFieldMetacall regardless. Step 2 therefore splits the local code and leaves half behind while adding a state and a sentinel adapter — net local code grows.Residual, recorded rather than acted on: the clean version of step 2 would need the shared hook to grow a "resolve these extra paths but emit no relabel entry for them" concept. That is an edit to
@object-ui/react, a surface that just landed and whose naming/shape belongs to a fresh card — so per the dispatch constraints I stopped rather than reshaping it here.Verification
All runs are repo-root vitest, per AGENTS.md.
pnpm exec vitest run packages/plugin-charts/ --maxWorkers=2— 22 files / 170 tests passed, before and after, identical counts.pnpm --filter @object-ui/plugin-charts type-check(tsc --noEmit) — clean; packagebuild(dts emit) — clean. Dependency closure built first (--filter '@object-ui/plugin-charts^...' build), since@object-ui/reactresolves throughdistrather than a root tsconfig path alias.eslint packages/plugin-charts/src/ObjectChart.tsx— 0 errors; 57 warnings, byte-identical to the same file's warning count onorigin/main(all pre-existingreact-refresh/anywarnings).node scripts/check-control-bytes.mjs— OK; plus a targetedgrep -naPcontrol-byte self-scan of both touched files, clean.node scripts/check-phantom-dependencies.mjs(Phantom dependency:reactresolves frompackages/corealthough core declares none — a resolution-based layering check returns the wrong answer #4394, landed mid-task) — OK.node scripts/check-changeset-presence.mjs— OK, 1 changeset for 1 released package. Changeset ispatch, nevermajor, matching PR refactor(core,react,dashboard,report): the analytics label net's fetch-and-memo glue is written once (#4389) #4404's precedent for a behaviour-unchanged refactor.Pins unchanged (the acceptance evidence):
git diff --name-only origin/main...HEAD -- 'packages/plugin-charts/**/*.test.*'is empty. The whole branch touches exactly two files:Note the pin that actually covers this code is
packages/plugin-charts/src/ObjectChart.optionColors.test.tsx, which sits outside thesrc/__tests__/directory — the sweep above deliberately covers the whole package's test surface rather than that one directory.Reverse verification
Direction predicted before each run.
ObjectChart.tsxto the local closures, keep the suite. Predicted GREEN, got GREEN (22 files / 170 tests). This is a null result and is reported as such: for a behaviour-preserving swap it is the correct outcome, and it is the argument that the seam-level pins carry the guarantee — the same result and the same reasoning PR refactor(core,react,dashboard,report): the analytics label net's fetch-and-memo glue is written once (#4389) #4404's RV1 recorded.nullas the relabel argument. Predicted RED, got RED, on exactly the two dataset label pins:expected 'todo' to be 'To do'in resolves a dataset chart through its definition to the underlying object and routes BOTH dataset hops through the host apiFetch. The pins do observe this path, so run 1's green is meaningful.dimensionOptionTranslator(undefined, ...)). Predicted GREEN as a blind spot, got GREEN (7/7). Reported honestly rather than dressed up as coverage: these pins mount noI18nProvider, so the resolver returns the authored label and a bound translator is indistinguishable from an unbound one. The colour path's translator binding is therefore not pinned at this seam today — it is pinned one level down in core's own unit tests. Not a regression introduced here (the same gap existed before this PR), and not in scope to fix.Working tree clean after each; no
git stashused at any point.Generated by Claude Code