fix(charts): a spec series[].type draws, and a spec-shape series plots at all (#2945) - #3004
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
github-actions
Bot
added
documentation
Contributor
✅ Console Performance Budget
📦 Bundle Size Report
Size Limits
|
…plots at all (#2945) the spec ALREADY models a combo chart, per-series, and its own field comment says so — `ChartSeries.type`, "Series type override (combo charts)" — exactly as it models stacking with `ChartSeries.stack` rather than a `stacked-bar` family. So `combo` is not a name an author should reach for; it is what "the series disagree about their family" looks like from the renderer's side. `effectiveChartFamily` derives it. Chasing that turned up two live bugs, both silent, on the path a spec author takes. 1. The per-series override was parsed, carried, and then DROPPED. Only the `chartType === 'combo'` branch read `series[].chartType`, so `{ type: 'bar', series: [{ name: 'revenue' }, { name: 'margin', type: 'line' }] }` drew `margin` as a bar. A unit test even asserted the value was carried. 2. A spec-shape `series` rendered NOTHING AT ALL. `series` is the one binding both shapes spell alike, so ChartRenderer's blanket "internal props win" rule let the author's `[{ name }]` shadow the normalized `[{ dataKey }]` and reach a renderer that reads `dataKey`. Every other spec binding has a distinct name (`xAxis` vs `xAxisKey`), which is why only this one broke — and why the isolated normalization tests all passed over a dead path. (2) is fixed here rather than filed, because (1) alone would have been a fix no spec author could reach. `ChartRendererProps.series` now accepts both shapes too: it was typed internal-only, so writing the protocol correctly was a type error on code that works. Also found in passing: the combo branch had an `area` arm inside a `BarChart` container, and Recharts draws an `<Area>` child of `BarChart` as nothing — so an authored combo with an `area` series rendered a blank series. The container is now `ComposedChart`, which is what Recharts provides for mixed marks. Widening only. One family stays one family, an explicit `combo` is untouched, and a family with no per-series meaning (`pie`, `horizontal-bar`) is never widened. A derived combo binds to the left axis unless a series asks for `yAxis: 'right'` — the spec's own default — so widening changes the mark and not the scale; the bar→left/line→right guess is kept for an authored `combo`, where it was the only way to reach a second axis. Guards: the two helpers are unit-tested over the family matrix; the DOM tests assert the MARKS rather than the derived family, since the carry was already covered and the drawing was what broke; and `spec-derived-unions.test.ts` asserts `combo` is absent from the spec's `ChartTypeSchema`, so the day it is adopted upstream the derivation is named as the thing to retire. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
xuyushun441-sys
force-pushed
the
refactor/2945-combo-series-type
branch
from
July 30, 2026 09:50
90fef1e to
d61cbb4
Compare
Contributor
✅ Console Performance Budget
📦 Bundle Size Report
Size Limits
|
Contributor
Author
Verified in a real browserThe #2901 audit's own caveat is "Nothing was exercised at runtime, so symptoms are read off the code path, not observed." This PR's claims were in that category, so I observed them: a throwaway Vite entry mounting A/B by checking the three source files out at
The two "before" blanks are worth looking at directly: the chart renders its grid, both axes and the legend, and zero data marks. That is indistinguishable from an empty dataset — the same Tier-2 signature #3008 fixed for single-value families, which is why this could sit in a shipped renderer without a bug report. One finding about the harness, not the rendererMy first pass measured zero marks for all three combo cases even with the fix. The cause was my own page: Worth recording because it is a real trap for anyone embedding a combo chart in an auto-width container, and because it is invisible to the vitest suite: those tests mock |
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
1 participant
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.
Next item on #2945's additions-only list:
combo(plugin-charts), listed as renderer-local dialect to "promote or delete".Neither — and the check was one grep. The spec already models a combo chart, per-series, and its own field comment says so:
Same reasoning the spec applies to stacking —
ChartSeries.stackrather than astacked-barfamily:So
combois not a name an author should have to reach for. It is what "the series disagree about their family" looks like from the renderer's side, andeffectiveChartFamilynow derives it. It stays a documented renderer-local marker because internal callers pass it explicitly today (DatasetPreview's mixed-scale branch).Chasing it turned up two live bugs, both silent
Both sit on the path a spec author takes, and neither is visible from an enum-coverage angle.
1. The per-series override was parsed, carried, and then dropped. Only the
chartType === 'combo'branch readseries[].chartType, sodrew
marginas a bar. Nothing was wrong at any layer but the last — an existing unit test even asserted the value was carried.2. A spec-shape
seriesrendered nothing at all.seriesis the one binding both shapes spell with the same key, soChartRenderer's blanket "internal props win" rule let the author's[{ name }]shadow the normalized[{ dataKey }]and reach a renderer that readsdataKey. Blank chart. Every other spec binding has a distinct name (xAxisvsxAxisKey), which is why only this one broke — and why the isolatednormalizeChartSchematests could all pass over a dead path. I found it by probing the realChartRendererpath instead of trusting those tests.I fixed (2) here rather than filing it: shipping (1) alone would have been a fix no spec author could reach. The raw array is now preferred only when it already speaks the internal shape, so every internal caller is byte-for-byte unchanged, and a mixed array works too (
normalizeSeriesreadsdataKey ?? nameper entry).ChartRendererProps.seriesalso now accepts both shapes. It was typed internal-only, soseries: [{ name: 'total' }]was a type error on an author writing the protocol correctly — the same trapActionType-missing-formwas in #2985. My tests dropped theiras anycasts as a result, which is the proof the contract widened.3. Found in passing: the combo branch had an
areaarm inside aBarChartcontainer, and Recharts renders an<Area>child ofBarChartas nothing — so an authored combo with anareaseries drew a blank series, silently. The arm was unreachable. The container is nowComposedChart, which is what Recharts provides for mixed marks.Widening only
chartType: 'combo'pie/donut/funnel/radar/scatter/treemap/sankey/horizontal-barA derived combo binds series to the left axis unless one asks for
yAxis: 'right'— the spec's ownChartSeries.yAxisdefault — so widening a bar chart changes the series' mark and not its scale. Thebar→left / line→rightguess is kept only for an authoredcombo, where historically it was the sole way to reach a second axis.Guards
effectiveChartFamily/comboBaseFamilyunit-tested across the family matrix.spec-derived-unions.test.tsassertscombois absent from the spec'sChartTypeSchema— the same inverse tripwire refactor(actions):navigationbecomes a declared alias ofurl, sharing one navigator (#2944) #2994 added fornavigation, so the day it is adopted upstream the derivation is named as the thing to retire.Verification
plugin-chartstestsplugin-charts+typeseslintThe new tests bite. Reverting only the two source files with the tests left in place fails 8 of 11 — and the 3 that still pass are exactly the unchanged-behavior controls (plain bar chart, authored combo, Tremor
categories).One test file (
components/html-page-lazy-blocks) hit abeforeAlltimeout onawait import('../renderers')during the full run, unrelated to this change and with a zero test-failure count; it passes standalone (3/3). That is the known load-induced mode, not a logic failure.Refs #2945, #2901. Follows #2994.
🤖 Generated with Claude Code