Uh oh!
There was an error while loading. Please reload this page.
fix(plugin-dashboard): draw an authored combo as a combo on the dataset path (#4229) - #4298
Merged
Merged
Conversation
…et path (#4229) A widget authoring the spec's combo shape — `series[].type` plus `series[].yAxis` and two `yAxis` entries — rendered as two bar series on one shared axis (measured: 2 bars / 0 lines / 1 axis where 1 bar + 1 line + 2 axes were authored). Two halves, both here, and fixing either alone is worse than neither: `CHART_TYPE_MAP` had no `combo` entry so the widget fell through its `?? 'bar'` default, and `chartConfigPresentation` forwarded no `series` / `xAxis` / `yAxis` at all. The ruled split: the dataset owns DATA (series membership, and the column each binding reads — `ChartSeries.name`, `ChartAxis.field`), the author owns PRESENTATION (per-series mark and axis binding, label/colour/stack, axis scale and chrome). Presentation merges onto the derived bindings by name/key with the explicit binding winning — one merge function, not a spread per attribute. This is #2880's S2 rule, landed in ObjectChart by #2883 and never carried to the dataset path. Dropping `ChartAxis.field` on the way through is what makes forwarding the axes safe structurally: it is the one key by which an authored axis could name a series, since the renderer synthesises series from `yAxis[].field` when a chart declares none. Also: a `compareTo` overlay now inherits its own measure's mark and axis, so a bar-on-the-left measure's comparison no longer draws as a line on the right once the chart becomes a combo. 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
marked this pull request as ready for review
August 11, 2026 12:12
Uh oh!
There was an error while loading. Please reload this page.
This was referenced Aug 11, 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 freeto 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#4229
Both halves of the card land here, as it asked: fixing either one alone leaves a worse state than fixing neither (a chart that is nominally a combo with no line and one axis, or a forwarded series array colliding with the dataset derivation).
The premise, re-verified at
origin/main(ee66e2ebf)Both halves still hold, unchanged:
CHART_TYPE_MAPhad nocomboentry, soCHART_TYPE_MAP[widgetType] ?? 'bar'sent everycombowidget to the bar family.chartConfigPresentationforwarded noseries/xAxis/yAxisat all, under a comment stating they are "DERIVED from the dataset selection".The ruling this implements
Quoted verbatim from the claim comment on #4229 (#4229#issuecomment-5252539356):
This is #2880's S2 — dual axes as
yAxis[].positionplusseries[].yAxis, explicit binding taking priority over the implicit per-series-type guess — which PR #2883 landed inObjectChartand which the dataset path never carried over.Where the split falls
The line runs through the two binding keys, not around whole objects:
ChartSeries.name;ChartAxis.field;xAxisKeyseries[].type,series[].yAxis,label,color,stack,variant,dashArray,opacity; axistitle/format/min/max/stepSize/showGridLines/position/logarithmicmergeAuthoredPresentationis the one place that merge happens — a single function carrying the match-by-name rule, not a spread per attribute. An authored entry naming a measure outside the dataset selection matches nothing and is dropped with its presentation; a derived series with no authored entry keeps the family default.Dropping
ChartAxis.fieldon the way through is what makes forwarding the axes safe structurally rather than by a guard: it is the one key by which an authored axis could name a series, becausenormalizeChartSchemasynthesises series out ofyAxis[].fieldwhen a chart declares none. The count of axis entries still travels, since that is what declares the secondary axis.CHART_TYPE_MAP.combomaps tocomborather than to a base family: the renderer draws it distinctly (mixed marks on aComposedChart, both y-axes), it has been aChartTypeSchemamember since spec 17.0.0-rc.1, andwidgetDispatchalready resolves acombowidget tochartType: 'combo'— so the two surfaces now agree.Two consequences worth flagging in review
type: 'line'series and get the combo the renderer already derived from disagreeing series types (Vocabulary consolidation (#2901): what shipped, and why Track C items 1–3 are not planned #2945). The widget's own family is unchanged; the renderer derives.compareTooverlay now inherits its own measure's mark and axis. Without this, my change would have introduced a new wrong state: once a chart becomes a combo, the overlay of a bar/left measure fell to the renderer's positional guess and drew as a line on the right axis. It inherits mark and axis only — neverstack, which would add the two periods together.Tests
New
DatasetWidget.comboPresentation.test.tsx(12 cases) drives the card's exact fixture —task_countbar/left,avg_progressline/right, twoyAxisentries — and asserts against the shape the renderer receives, running the emitted schema throughnormalizeChartSchema(the one translation layerChartRendererputs in front ofAdvancedChartImpl), imported read-only.Honest scope note on the DOM half. These do not count recharts marks. That needs
ResponsiveContainermocked to a measured box, andrechartsresolves insideplugin-chartsalone — avi.mock('recharts')inplugin-dashboardcannot even resolve the specifier, which is the same constraintDatasetWidget.chartConfig.dom.test.tsxalready records. The mark half is already pinned there against exactly this shape:AdvancedChartImpl.comboFromSeries.test.tsxdraws achartType: 'line'series as a line and bindsyAxis: 'right'to the right axis, and the combo branch renders both y-axes unconditionally. The card's measured table is therefore reconciled across the two files rather than re-measured here.Reverse verification, fix removed via
git checkout origin/main -- DatasetWidget.tsx, direction predicted first: 8 red, 4 green. The 4 that stay green are exactly the controls and the membership pins — an authored series outside the selection ignored, a non-cartesian per-seriestypedropped, a no-chartConfigwidget emitting a byte-identical derived series, a chrome-onlychartConfigleaving bindings alone. Green in both states is what makes them controls: they guard the invariant rather than testing the new code.DatasetWidget.chartConfig.test.tsxpinned the old belief and is updated: its refusal section now pins the data half (ChartAxis.field,ChartSeries.name) as hard as it pinned the whole keys before.typeandariastay refused, untouched.Green locally:
plugin-dashboard36 files / 313 tests (including #4273'srelabelDimensions/displayRowsand #4275'spivotDimensionValueregions, which this diff does not touch),plugin-charts22 files / 170 tests, roottype-check78/78 tasks,type-check:coverage,type-check:vitest-setup,check:control-bytes,check-changeset-presence,check-changeset-no-major, and eslint on the changed files (0 errors).Out of scope
The
kpi/gauge/solid-gauge/bulletcaveat was measured, not fixed, per ruling item 4 — filed as #4295. The answer: one shape, and deliberate, not #4229's shape. All four are routed to the single-value path by three coordinated places with no per-type differentiation anywhere, and the spec's ownChartTypeSchemacalls them "honest single-value variants pending a real dial/target renderer". The gap is one missing capability (no dial / arc / bullet-target renderer), not four defects.Generated by Claude Code