fix(core,charts,dashboard): give a chart bucket an identity distinct from its display label - #4677
Merged
Merged
Conversation
…from its label (#4508) `buildChartSeries` used a bucket's DISPLAY string as the bucket's own key, so two pairs of genuinely different groups were conflated, and the segment click that drills a bar back to its records inherited both conflations: - the pivot branch keyed by `String(xRaw ?? '')`, which spells a null group and an empty-string group identically — two groups drew ONE bar and the later one's segment resolved to no row at all; - a record whose stored value literally spells the bucket label (`(None)`, or a localized `chart.nullCategory`) kept its own bucket, so two bars carried the same axis text and BOTH resolved to the first — a wrong drill, not a dead one. `chartBucketId` is the identity: the SAME encoder `buildPivot` already keys the pivot TABLE's buckets with (`pivotBucketId` over `pivotDimensionValue`), so the two surfaces stop answering one question about the same rows two ways. The pivot branch buckets by it. Where two DISTINCT buckets still paint the same axis text, the emitted row carries that identity under `CHART_BUCKET_ID_KEY` — the complete set of cases the display string cannot name, so an ordinary chart's rows are returned untouched and no renderer-internal key reaches an authoring surface. `findChartSeriesRow` takes it back as `options.bucketId` and treats it as authoritative. The renderers forward it: the drill event gains `categoryId` (`ChartSegmentClickEvent`, now declared once in core instead of inline in three packages), `AdvancedChartImpl` reads it off the clicked row on the cartesian, pie and funnel paths, and `DatasetWidget.handleChartDrill` hands it to the lookup. The identity is an ordinary enumerable property because recharts builds a pie sector's `payload` as a spread copy of the row. The reader's matching is exact now: a null category reads as its rendered bucket label and no longer also as `''`. That tolerance was justified as the drill layer's own spelling of "no group value", but no producer of this lookup's `category` writes it, while `''` IS the axis text a genuine empty-string group paints — so it was handing that group's bar a different group's records. The three cases objectui#4497 pinned as "measured limits, not correct" are updated here, in the commit that changed them, as their own note said they would have to be. A host that forwards no `categoryId` keeps its existing drill. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RnQd8iMMUwXQEV1crFmQiQ
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Contributor
✅ Console Performance Budget
📦 Bundle Size Report
Size Limits
|
yinlianghui
marked this pull request as ready for review
August 15, 2026 07:50
This was referenced Aug 15, 2026
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 #4508
target:v17board item. Implements the maintainer ruling of 2026-08-14 on the card (verbatim 「同意」 on the sentinel-identity direction):buildChartSerieswrites a bucket identity distinct from the display string,findChartSeriesRowreads it back, aligned with the distinct-bucket-id form the pivot TABLE (buildPivot) already uses over the same dataset rows. One change covering both collisions.The two collisions, and the one cause
The bucket's DISPLAY string served as the bucket's own key, so two pairs of genuinely different groups were conflated — and the segment click that drills a bar back to its records inherited both.
String(xRaw ?? ''), which spellsnulland''identically. The bar took its label from whichever row created the bucket, and the other group's segment resolved to no row — a visible bar whose click did nothing.(None)(or any localizedchart.nullCategory—(未指定)and the other nine packs) kept its own bucket, so two bars carried the same axis text and BOTH resolved to the first. That one is a wrong drill, not a dead one: clicking the null bucket's bar opened the drawer on another group's records.What changed
chartBucketId(@object-ui/core) is the identity — the SAME encoderbuildPivotkeys its buckets with (pivotBucketIdoverpivotDimensionValue), so the chart and the table stop answering one question two ways. The pivot branch buckets by it, which is what makes null and''two groups again.CHART_BUCKET_ID_KEYcarries that identity on an emitted row, written exactly where two DISTINCT buckets paint the same axis text — the complete set of cases where the display string cannot name what was clicked, given the reader's matching is now exact. An ordinary chart's rows are returned untouched (by identity), so no renderer-internal key reaches an authoring surface (dataon achartschema). Rows lacking the category key never get one — that shape stayshasNoCategoryKey's (framework#4033).findChartSeriesRowtakes it back asoptions.bucketIdand treats it as authoritative, ignoringcategoryentirely when present.categoryId, viaChartSegmentClickEvent— now declared once in core rather than as three inline literals acrossAdvancedChartImpl/ChartRenderer/ObjectChart, since a field added to one of three copies reaches the consumer asundefinedwith nothing red.AdvancedChartImplreads the identity off the clicked row on the cartesian, pie and funnel paths;DatasetWidget.handleChartDrillhands it to the lookup.Why the identity is an ordinary enumerable property
Measured, not assumed: recharts builds a pie sector's
payloadas{ ...row, ...cellProps }— a spread copy — so a symbol key or a non-enumerable property would arrive as "this bucket has no identity", the drill would fall back to the shared axis text, and every pure test would still be green. There is a DOM test that clicks a real sector for exactly this reason.On the cartesian path the clicked row is read out of our own
dataarray byactiveTooltipIndex, not out of the event: recharts 3 sends aMouseHandlerDataParamthat carries an index and no row, so nothing in that read can be a stale or copied payload.Behaviour change worth reviewing
An empty-string category no longer resolves to a null-valued row. That tolerance was justified in the code as the drill layer's own spelling of "no group value" (
computeDrillFilter), but re-measurement does not support it:computeDrillFilterruns downstream of a resolved drill and never produces this lookup'scategory, whose one production producer is a chart click — while''is the axis text a genuine empty-string group paints. So the tolerance was giving that group's bar a different group's records. A host that forwards nocategoryIdkeeps its existing drill unchanged.Pin updates
The three cases #4497 pinned in
chart-series.nullCategory.test.tsas "the measured limits of the bucket label" — explicitly as limits and not as correct — are flipped here, in the commit that changed them, each keeping its fixture and stating what it now asserts and what it asserted before. The two''-reads-as-null pins are updated with the re-measurement above. Every other pin in that file, including all the must-not-change ones, is untouched and green.Verification
All at
f3e14d59a(the final commit), gate union re-run after it.pnpm exec vitest run packages/core/ packages/plugin-charts/ packages/plugin-dashboard/— 170 files, 2463 tests, all passing.turbo run type-check lintfor the three packages with the dependency closure built — 19 tasks successful,tsc --noEmit && tsc -p tsconfig.test.jsonconfirmed executed, 0 lint errors (warnings are the packages' pre-existinganycounts).check:changeset-fixed,changeset-no-major,changeset-presence,control-bytes,spec-symbols,phantom-deps,i18n-keys,i18n-drift,lint-coverage,type-check-coverage,skills-paths— all OK.Cross-package type reverse check: renaming
ev.categoryIdto a non-existent field inDatasetWidgetfails aserror TS2339: Property 'categoryIdNoSuchField' does not exist on type 'ChartSegmentClickEvent'— the error names the type from the freshly built core.d.ts, proving plugin-dashboard type-checked against the rebuilt declaration rather than a cached one.Reverse verification, directions predicted before running, split into two legs so each collision is attributed to its own half:
categoryIdEach leg was restored byte-for-byte from the committed state before the next (
git checkoutof the commit sha, path-scoped — never a stash), and the final numbers above are from the restored tree.Out of scope, filed while measuring
activePayload, which recharts 3 does not send — every cartesian drill loses its series and value, killing the pivoted drill outright #4672 —AdvancedChartImpl's cartesian click readsactivePayload, which recharts 3 does not send, soseriesandvalueare alwaysundefined. This kills the pivoted drill outright (the lookup's pivot arm requires the series key), independently of this PR. It is why the pivot arm's end-to-end click cannot be demonstrated on a cartesian chart today; the pure and stubbed tests here exercise it directly instead. Not fixed here: different defect class, and the right value foractiveDataKeyunder a shared cartesian tooltip needs its own measurement. AdvancedChartImpl's cartesian click readsactivePayload, which recharts 3 does not send — every cartesian drill loses its series and value, killing the pivoted drill outright #4672 is not addressed by this PR.gId !== ''), so its measure is written into the bucket and never drawn. The A NULL-keyed group is dropped from a bar chart, leaving an axis with no marks and no empty state — the default first-boot state of System Overview's "Events by User" #4466 defect one dimension over, and it needs its own ruling because a series key is also a row key. buildChartSeries' pivot branch silently drops a null SECOND-dimension group: its measure is written to the bucket but never becomes a series, so the value vanishes from the chart #4673 remains open.Branch was cut from
b1119ece4;mainhas since advanced to541c72425(#4671), which touches none of this PR's files.Generated by Claude Code