From d61cbb4b3dbdcfd6b8762b05c75e8fe9de64508d Mon Sep 17 00:00:00 2001 From: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Date: Thu, 30 Jul 2026 16:55:47 +0800 Subject: [PATCH] fix(charts): a spec `series[].type` draws, and a spec-shape `series` plots at all (#2945) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 `` 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 --- .changeset/combo-derived-from-series.md | 63 +++++++ .../2901-spec-enum-renderer-coverage.md | 29 +++- ...AdvancedChartImpl.comboFromSeries.test.tsx | 155 ++++++++++++++++++ .../plugin-charts/src/AdvancedChartImpl.tsx | 52 ++++-- .../src/ChartRenderer.specSeries.test.tsx | 136 +++++++++++++++ packages/plugin-charts/src/ChartRenderer.tsx | 33 +++- .../src/normalizeChartSchema.test.ts | 82 ++++++++- .../plugin-charts/src/normalizeChartSchema.ts | 82 ++++++++- .../src/__tests__/spec-derived-unions.test.ts | 20 ++- 9 files changed, 627 insertions(+), 25 deletions(-) create mode 100644 .changeset/combo-derived-from-series.md create mode 100644 packages/plugin-charts/src/AdvancedChartImpl.comboFromSeries.test.tsx create mode 100644 packages/plugin-charts/src/ChartRenderer.specSeries.test.tsx diff --git a/.changeset/combo-derived-from-series.md b/.changeset/combo-derived-from-series.md new file mode 100644 index 0000000000..afe66381a6 --- /dev/null +++ b/.changeset/combo-derived-from-series.md @@ -0,0 +1,63 @@ +--- +"@object-ui/plugin-charts": minor +--- + +fix(charts): a spec `series[].type` override actually draws, and a spec-shape `series` plots at all (#2945) + +#2945 listed `combo` (`plugin-charts`) as renderer-local dialect to "promote or +delete". Neither: the spec **already models a combo chart**, per-series, and its +own field comment says so — + +```ts +// spec/src/ui/chart.zod.ts — ChartSeriesSchema +/** Series type override (combo charts) */ +type: ChartTypeSchema.optional().describe('Override chart type for this series'), +``` + +— 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` now derives it, and `combo` stays a documented +renderer-local marker (internal callers pass it directly today). + +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 +renderer's `chartType === 'combo'` branch read `series[].chartType`, so + +```ts +{ type: 'bar', series: [{ name: 'revenue' }, { name: 'margin', type: 'line' }] } +``` + +drew `margin` as a bar. Nothing was wrong at any layer but the last — 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 with the same key, so `ChartRenderer`'s blanket "internal props +win" rule let the author's `[{ name }]` shadow the normalized `[{ dataKey }]` and +reach a renderer that reads `dataKey`. Blank chart. 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. The raw array is now +preferred only when it already speaks the internal shape, so internal callers are +byte-for-byte unchanged and a mixed array works too. + +**Also fixed in passing:** the combo branch had an `area` arm under a `BarChart` +container, and Recharts renders an `` child of `BarChart` as nothing — so an +authored combo with an `area` series drew a blank series. The container is now +`ComposedChart`, which is what Recharts provides for mixed marks. + +Widening only. A chart whose series all resolve to one family keeps its own +family, an explicit `combo` is untouched, and a family with no per-series meaning +(`pie`, `horizontal-bar`, …) is never widened. A derived combo binds series to the +left axis unless one asks for `yAxis: 'right'` — the spec's own default — so +widening changes the series' mark and not its scale; the legacy bar→left/line→right +guess is kept for an authored `combo`, where it was historically the only way to +reach a second axis. + +Guards: `effectiveChartFamily` / `comboBaseFamily` are unit-tested over the whole +family matrix; DOM-level 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. diff --git a/docs/audits/2901-spec-enum-renderer-coverage.md b/docs/audits/2901-spec-enum-renderer-coverage.md index 030bd78f19..4999ad3296 100644 --- a/docs/audits/2901-spec-enum-renderer-coverage.md +++ b/docs/audits/2901-spec-enum-renderer-coverage.md @@ -166,7 +166,7 @@ value corruption rather than disclosure. | Name | Location | Call | |---|---|---| | `navigation` action type | [ActionRunner.ts](../../packages/core/src/actions/ActionRunner.ts) | ~~Promote~~ — **resolved as a declared alias instead.** See the correction below. | -| `combo` chart type | [normalizeChartSchema.ts:63](../../packages/plugin-charts/src/normalizeChartSchema.ts) | Drawn at `AdvancedChartImpl.tsx:819`. Promote or delete. | +| `combo` chart type | [normalizeChartSchema.ts](../../packages/plugin-charts/src/normalizeChartSchema.ts) | ~~Promote or delete~~ — **derived from the series instead.** See Correction 4; the spec models it as `ChartSeries.type`. | | `system` theme mode | ThemeProvider ×2 | Delete — the spec name is `auto`, and `react/ThemeContext.tsx` already gets it right. | | `scale-fade` preset | [useAnimation.ts:71](../../packages/react/src/hooks/useAnimation.ts) | Delete with the hyphen/underscore fix. | | `modal` notification type | [NotificationContext.tsx:23](../../packages/react/src/context/NotificationContext.tsx) | Inert — nothing reads `displayType`. | @@ -358,7 +358,7 @@ On a list whose only narrowing is that filter, the user sees rows they had asked to exclude. It is not a permission bypass — row-scoping `$and`-composes as a separate arm and survives — but it ranks with Tier 1, not Tier 2. -## Correction 4 — "promote `navigation` upstream" was the wrong call +## Correction 4 — "promote it upstream" was the wrong call twice (`navigation`, `combo`) Direction B told the reader to promote the `navigation` action type into the spec, on the grounds that it is "a real capability the spec doesn't name". The capability @@ -385,6 +385,28 @@ and delete — **alias to the spec name it duplicates** — and picking between three starts with asking whether the spec really lacks the concept or just spells it differently. +**`combo` is the same mistake, and the check is one grep.** The row above told the +reader to promote or delete it. `ChartSeriesSchema.type` already exists, and its +field comment reads *"Series type override (combo charts)"* — the spec models a +combo chart per-series, exactly as it models stacking with `ChartSeries.stack` +rather than a `stacked-bar` family. `combo` is therefore derived from the series, +not authored (`effectiveChartFamily`). + +Reading the row as written would also have missed what was actually broken. Two +live silent failures sat on the path a spec author takes: + +1. the per-series override was parsed and carried and then **dropped** — only the + `chartType === 'combo'` branch read `series[].chartType`, so `type: 'bar'` with + a `type: 'line'` series drew a bar; and +2. a spec-shape `series` **rendered nothing at all** — `series` is the one binding + both shapes spell alike, so `ChartRenderer`'s "internal props win" rule let the + author's `[{ name }]` shadow the normalized `[{ dataKey }]`. + +Neither is visible from an enum-coverage angle, which is worth noting about this +audit's method: **a dialect row can be the least interesting thing wrong at that +line.** The `combo` row asked whether a name belonged in the spec; the answer was +that the spec's own name for it had never worked. + ## Status of the items above | Item | Status | @@ -394,7 +416,8 @@ it differently. | `DensityModeSchema` inert | Resolved upstream (removed in spec 17) | | Packages lacking the `@objectstack/spec` devDep | **6** — `plugin-list` and `data-objectstack` gained it in #2974; `plugin-charts`, `plugin-dashboard`, `plugin-report`, `components`, `mobile` and `fields` remain. (The body above says 6 because it counted a 13-package "needs a guard" list that omitted `fields`; adding `fields` and removing the two now fixed leaves 6 either way.) | | The five shadowing forks | Fixed — #2985 (closes #2944) | -| `navigation` action type | Resolved as a declared alias, not promoted — see Correction 4 | +| `navigation` action type | Resolved as a declared alias, not promoted — #2994, see Correction 4 | +| `combo` chart type | Derived from `ChartSeries.type`, not promoted — see Correction 4. Uncovered two silent render bugs on the spec-shape path. | | Everything else | Open — #2941 (Tier 1), #2942 (Tier 2), #2943 (Tier 3), #2945 (vocabularies) | One method note for anyone repeating this: the original run read a checkout that diff --git a/packages/plugin-charts/src/AdvancedChartImpl.comboFromSeries.test.tsx b/packages/plugin-charts/src/AdvancedChartImpl.comboFromSeries.test.tsx new file mode 100644 index 0000000000..637a969000 --- /dev/null +++ b/packages/plugin-charts/src/AdvancedChartImpl.comboFromSeries.test.tsx @@ -0,0 +1,155 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * #2945 — a series that overrides its family actually draws that family. + * + * `ChartSeries.type` is the spec's own per-series family override; its field + * comment reads "Series type override (combo charts)". It was parsed by + * `normalizeChartSchema` and carried on `series[].chartType` — a unit test even + * asserted the carry — and then dropped, because only the renderer's + * `chartType === 'combo'` branch read it. So an author writing the protocol got + * their line series drawn as a bar, with nothing wrong at any layer but the last. + * + * These assert the MARKS in the DOM rather than the derived family, because the + * carry was already covered and the drawing is what was broken. + */ + +import React from 'react'; +import { describe, it, expect, vi, afterEach } from 'vitest'; +import { render, cleanup } from '@testing-library/react'; + +// Recharts' ResponsiveContainer measures via ResizeObserver, which reports 0×0 +// under the headless DOM, so nothing paints. Fix its size. +vi.mock('recharts', async () => { + const actual = await vi.importActual('recharts'); + return { + ...actual, + ResponsiveContainer: ({ children }: any) => + React.cloneElement(children, { width: 480, height: 320 }), + }; +}); + +import AdvancedChartImpl from './AdvancedChartImpl'; + +afterEach(cleanup); + +const DATA = [ + { month: 'Jan', revenue: 120, margin: 0.4 }, + { month: 'Feb', revenue: 80, margin: 0.9 }, +]; + +const base = { + data: DATA, + xAxisKey: 'month', + isAnimationActive: false, +}; + +const lines = (c: HTMLElement) => c.querySelectorAll('.recharts-line').length; +const bars = (c: HTMLElement) => c.querySelectorAll('.recharts-bar').length; +const areas = (c: HTMLElement) => c.querySelectorAll('.recharts-area').length; + +describe('AdvancedChartImpl — a combo derived from the series', () => { + it('draws a `type: line` series as a line on an otherwise-bar chart', () => { + // The regression: `margin` used to render as a second bar. + const { container } = render( + , + ); + expect(lines(container)).toBe(1); + expect(bars(container)).toBe(1); + }); + + it('draws a `type: bar` series as a bar on an otherwise-area chart', () => { + const { container } = render( + , + ); + expect(bars(container)).toBe(1); + // The un-annotated series takes the chart's own family, not `bar` — which + // is why the base has to come from the chart rather than a hardcoded guess. + expect(areas(container)).toBe(1); + }); + + it('leaves a chart whose series all agree as a plain bar chart', () => { + const { container } = render( + , + ); + expect(bars(container)).toBe(2); + expect(lines(container)).toBe(0); + }); + + it('binds a derived combo to one axis unless the series asks for two', () => { + // `ChartSeries.yAxis` defaults to 'left' in the spec, so widening a bar + // chart into a combo changes the series' MARK and nothing else. Two y-axes + // are always rendered by the combo branch; what matters is that the line + // is not silently moved onto the right one, which would rescale it. + const { container } = render( + , + ); + const line = container.querySelector('.recharts-line'); + expect(line).toBeTruthy(); + // Recharts places right-axis marks in the second y-axis' scale; the + // rendered path is the observable proxy, so assert the explicit opt-in + // changes it and the default does not. + const defaultPath = container.querySelector('.recharts-line-curve')?.getAttribute('d'); + + cleanup(); + const { container: right } = render( + , + ); + const rightPath = right.querySelector('.recharts-line-curve')?.getAttribute('d'); + expect(defaultPath).toBeTruthy(); + expect(rightPath).toBeTruthy(); + expect(rightPath).not.toBe(defaultPath); + }); + + it('draws an `area` series in a combo at all (it used to draw nothing)', () => { + // Found while widening this: the combo branch had an `area` arm, but its + // container was `BarChart`, under which a Recharts `` child renders + // nothing. The arm was unreachable and the series came out blank — silently. + // `ComposedChart` is the container built for mixed marks. + const { container } = render( + , + ); + expect(areas(container)).toBe(1); + expect(bars(container)).toBe(1); + }); + + it('still honours an explicitly authored `combo`', () => { + // `DatasetPreview` passes this directly for mixed-scale datasets, with + // series that declare nothing. The legacy index/family default applies: + // first series bar, the rest lines. + const { container } = render( + , + ); + expect(bars(container)).toBe(1); + expect(lines(container)).toBe(1); + }); +}); diff --git a/packages/plugin-charts/src/AdvancedChartImpl.tsx b/packages/plugin-charts/src/AdvancedChartImpl.tsx index 80d2f9a544..eb9207ef69 100644 --- a/packages/plugin-charts/src/AdvancedChartImpl.tsx +++ b/packages/plugin-charts/src/AdvancedChartImpl.tsx @@ -14,6 +14,7 @@ import { LineChart, Area, AreaChart, + ComposedChart, Pie, PieChart, Radar, @@ -47,7 +48,7 @@ import { ChartConfig } from './ChartContainerImpl'; import { mapScatterClick, mapTreemapClick, mapSankeyClick } from './chartDrillEvents'; -import { formatterFor, domainFor, ticksFor, RENDERABLE, SINGLE_VALUE_CHART_TYPES, TABULAR_CHART_TYPES, type NormalizedAxis, type NormalizedSeries } from './normalizeChartSchema'; +import { formatterFor, domainFor, ticksFor, RENDERABLE, SINGLE_VALUE_CHART_TYPES, TABULAR_CHART_TYPES, effectiveChartFamily, comboBaseFamily, type NormalizedAxis, type NormalizedSeries } from './normalizeChartSchema'; import { buildCategoryRank } from '@object-ui/core'; // Default color fallback for chart series @@ -96,6 +97,11 @@ const comparisonStyle = (s: any, kind: 'line' | 'area' | 'bar' | 'scatter') => { }; export interface AdvancedChartImplProps { + /** + * Chart family. `combo` is renderer-local and rarely needs to be passed: + * series declaring different families derive it (`effectiveChartFamily`), + * which is how `@objectstack/spec` expresses a combo chart. + */ chartType?: 'bar' | 'column' | 'horizontal-bar' | 'line' | 'area' | 'pie' | 'donut' | 'radar' | 'scatter' | 'funnel' | 'combo' | 'treemap' | 'sankey'; data?: Array>; config?: ChartConfig; @@ -266,7 +272,14 @@ function AdvancedChartImplInner({ }: AdvancedChartImplProps) { // Normalize 'column' → 'bar' (Recharts BarChart is already vertical). // 'column' is the spec-level alias for vertical bars; 'horizontal-bar' stays as-is. - const chartType = rawChartType === 'column' ? 'bar' : rawChartType; + const baseChartType = rawChartType === 'column' ? 'bar' : rawChartType; + // A chart whose series declare more than one family IS a combo, whatever its + // own family says — `ChartSeries.type` is how the spec expresses that, and + // reading it only under an explicit `combo` drew the overridden series as the + // base family instead (#2945). `comboSeriesBase` is what an un-annotated + // series draws, and `undefined` marks an explicitly-authored combo. + const chartType = effectiveChartFamily(baseChartType, series); + const comboSeriesBase = comboBaseFamily(baseChartType); const data = Array.isArray(rawData) ? rawData : []; // Only emit the prop when explicitly disabled, so the default (animated) @@ -364,9 +377,9 @@ function AdvancedChartImplInner({ radar: RadarChart, scatter: ScatterChart, funnel: FunnelChart as any, - combo: BarChart, - // treemap/sankey return from their own branches above; mapped here only so - // the index type stays exhaustive. + // combo/treemap/sankey return from their own branches above; mapped here + // only so the index type stays exhaustive. + combo: ComposedChart, treemap: BarChart, sankey: BarChart, }[chartType] || BarChart; @@ -877,12 +890,18 @@ function AdvancedChartImplInner({ ); } - // Combo chart (mixed bar + line on same chart) + // Combo chart (mixed families on one plot). Reached either by an explicit + // `chartType: 'combo'` or by series that declare different families — see + // `effectiveChartFamily`. if (chartType === 'combo') { return ( - + {/* `ComposedChart`, not `BarChart`, is the Recharts container built to + host mixed marks. Under `BarChart` an `` child renders nothing + at all, so the `seriesType === 'area'` arm below was unreachable — + an authored combo with an `area` series drew a blank series. */} + @@ -898,12 +917,21 @@ function AdvancedChartImplInner({ {brushEl} {series.map((s: any, index: number) => { const color = resolveColor(config[s.dataKey]?.color || DEFAULT_CHART_COLOR); - const seriesType = s.chartType || (index === 0 ? 'bar' : 'line'); - // An explicit spec `series[].yAxis` wins over the family-derived - // default (bar→left / line→right), which is only a guess. + // A derived combo knows what an un-annotated series is: the chart's + // own family. Only an explicitly-authored `combo` has no base, and + // there the index heuristic stands so those charts are unchanged. + const seriesType = s.chartType || comboSeriesBase || (index === 0 ? 'bar' : 'line'); + // An explicit spec `series[].yAxis` wins over any default. Where + // there is no `yAxis`, a DERIVED combo follows the spec, which + // defaults `ChartSeries.yAxis` to 'left' — so widening a bar chart + // into a combo changes the series' mark and nothing else. The + // bar→left / line→right guess is kept only for an authored `combo`, + // where historically it was the sole way to reach a second axis. const yAxisId = s.yAxis === 'right' || s.yAxis === 'left' ? s.yAxis - : (seriesType === 'bar' ? 'left' : 'right'); + : comboSeriesBase + ? 'left' + : (seriesType === 'bar' ? 'left' : 'right'); const cmp = comparisonStyle(s, seriesType as any); const stackProps = s.stack ? { stackId: String(s.stack) } : {}; const valueFormatter = formatterFor((yAxisId === 'right' ? secondaryY : primaryY)?.format); @@ -928,7 +956,7 @@ function AdvancedChartImplInner({ ); })} - + ); diff --git a/packages/plugin-charts/src/ChartRenderer.specSeries.test.tsx b/packages/plugin-charts/src/ChartRenderer.specSeries.test.tsx new file mode 100644 index 0000000000..ddf0991e1e --- /dev/null +++ b/packages/plugin-charts/src/ChartRenderer.specSeries.test.tsx @@ -0,0 +1,136 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * #2945 — a chart authored in the spec's `series` shape actually plots. + * + * `series` is the one binding the spec shape and the renderer's internal shape + * spell with the same key. `ChartRenderer` applied the blanket "internal props + * win" rule to it, so a spec author's `[{ name }]` shadowed the normalized + * `[{ dataKey }]` and reached a renderer that reads `dataKey` — the chart drew + * NOTHING. Every other spec binding has a distinct name (`xAxis` vs `xAxisKey`), + * which is why only this one broke, and why the isolated + * `normalizeChartSchema` unit tests could all pass while this path was dead. + * + * These go through `ChartRenderer` on purpose: the translation was already + * covered, the handoff was not. + */ + +import React from 'react'; +import { describe, it, expect, vi, afterEach } from 'vitest'; +import { render, cleanup, waitFor } from '@testing-library/react'; + +// Recharts' ResponsiveContainer measures via ResizeObserver, which reports 0×0 +// under the headless DOM, so nothing paints. Fix its size. +vi.mock('recharts', async () => { + const actual = await vi.importActual('recharts'); + return { + ...actual, + ResponsiveContainer: ({ children }: any) => + React.cloneElement(children, { width: 480, height: 320 }), + }; +}); + +import { ChartRenderer } from './ChartRenderer'; + +afterEach(cleanup); + +const DATA = [ + { month: 'Jan', revenue: 120, margin: 40 }, + { month: 'Feb', revenue: 80, margin: 90 }, +]; + +const marks = (c: HTMLElement) => ({ + bars: c.querySelectorAll('.recharts-bar').length, + lines: c.querySelectorAll('.recharts-line').length, +}); + +/** `AdvancedChartImpl` is lazy — wait for the real plot, not the skeleton. */ +const plotted = async (c: HTMLElement) => { + await waitFor(() => expect(c.querySelector('.recharts-surface')).toBeTruthy()); + return marks(c); +}; + +describe('ChartRenderer — the spec `series` shape', () => { + it('plots a spec-shape series (it used to render nothing at all)', async () => { + const { container } = render( + , + ); + expect(await plotted(container)).toEqual({ bars: 2, lines: 0 }); + }); + + it('honours a spec `series[].type` override end to end', async () => { + // The two bugs compound: the override was dropped at this handoff, and + // would have been ignored by the renderer even if it had survived. + const { container } = render( + , + ); + expect(await plotted(container)).toEqual({ bars: 1, lines: 1 }); + }); + + it('leaves an internal-shape series untouched', async () => { + const { container } = render( + , + ); + expect(await plotted(container)).toEqual({ bars: 1, lines: 1 }); + }); + + it('plots a mixed array, where neither shape alone would do', async () => { + const { container } = render( + , + ); + expect(await plotted(container)).toEqual({ bars: 2, lines: 0 }); + }); + + it('still adapts the Tremor-ish `categories` form', async () => { + const { container } = render( + , + ); + expect(await plotted(container)).toEqual({ bars: 2, lines: 0 }); + }); +}); diff --git a/packages/plugin-charts/src/ChartRenderer.tsx b/packages/plugin-charts/src/ChartRenderer.tsx index 9b4fec4f39..47e69bb16a 100644 --- a/packages/plugin-charts/src/ChartRenderer.tsx +++ b/packages/plugin-charts/src/ChartRenderer.tsx @@ -50,8 +50,20 @@ export interface ChartRendererProps { /** Internal binding. Authors write the spec `xAxis: { field }` (or the * report surface's bare string); `normalizeChartSchema` resolves both. */ xAxisKey?: string; - /** Internal binding. Authors write the spec `series: [{ name, … }]`. */ - series?: Array<{ dataKey: string; label?: string; variant?: 'current' | 'comparison'; opacity?: number; dashArray?: string; chartType?: 'bar' | 'line' | 'area'; stack?: string; yAxis?: 'left' | 'right'; color?: string }>; + /** + * Plotted series — **both shapes**, unlike the bindings below. + * + * `series` is the one binding the spec and the internal contract spell with + * the same key, so declaring only the internal shape here made + * `series: [{ name: 'total' }]` a type error on an author who was writing + * the protocol correctly — and, until #2945, one whose chart also rendered + * blank. `normalizeChartSchema` translates; an array that already speaks the + * internal shape passes through untouched. + */ + series?: Array< + | { dataKey: string; label?: string; variant?: 'current' | 'comparison'; opacity?: number; dashArray?: string; chartType?: 'bar' | 'line' | 'area'; stack?: string; yAxis?: 'left' | 'right'; color?: string } + | { name: string; label?: unknown; type?: string; variant?: 'current' | 'comparison' | 'primary'; opacity?: number; dashArray?: string; stack?: string; yAxis?: 'left' | 'right'; color?: string } + >; /** Spec `ChartConfig` shape — honored via `normalizeChartSchema` * (objectui#2880). Listed here so the author-facing contract type-checks; * the internal props above win when both are present. */ @@ -101,7 +113,22 @@ export const ChartRenderer: React.FC = ({ schema, onChartCli const props = React.useMemo(() => { const spec = normalizeChartSchema(schema); - let series: any[] | undefined = schema.series ?? spec.series; + // `series` is the one binding both shapes spell with the SAME key, so the + // blanket "internal props win" rule degenerated here: a spec author's + // `[{ name, type }]` shadowed the normalized `[{ dataKey, chartType }]` and + // reached a renderer that reads `dataKey` — so the chart rendered BLANK, and + // the per-series family override went with it (#2945). Every other spec + // binding has a distinct name (`xAxis` vs `xAxisKey`) and so was unaffected. + // + // Prefer the raw array only when it ALREADY speaks the internal shape, which + // keeps every internal caller byte-for-byte. Otherwise take the normalized + // one — `normalizeSeries` reads `dataKey ?? name` per entry, so it is also + // the right answer for an array that mixes the two. + const authored = Array.isArray(schema.series) ? schema.series : undefined; + const isInternalShaped = authored?.length + ? authored.every((s) => !!s && typeof s === 'object' && 'dataKey' in s) + : false; + let series: any[] | undefined = (isInternalShaped ? authored : spec.series) ?? authored; let xAxisKey = schema.xAxisKey ?? spec.xAxisKey; let config = schema.config; diff --git a/packages/plugin-charts/src/normalizeChartSchema.test.ts b/packages/plugin-charts/src/normalizeChartSchema.test.ts index 4bd249f2de..b399422a4a 100644 --- a/packages/plugin-charts/src/normalizeChartSchema.test.ts +++ b/packages/plugin-charts/src/normalizeChartSchema.test.ts @@ -10,7 +10,14 @@ * (DashboardRenderer / ObjectView / the dataset path pass it explicitly). */ import { describe, it, expect } from 'vitest'; -import { normalizeChartSchema, formatterFor, domainFor, ticksFor } from './normalizeChartSchema'; +import { + normalizeChartSchema, + formatterFor, + domainFor, + ticksFor, + effectiveChartFamily, + comboBaseFamily, +} from './normalizeChartSchema'; describe('normalizeChartSchema — spec shape', () => { it('resolves the ChartConfig axis + series shape', () => { @@ -117,6 +124,79 @@ describe('normalizeChartSchema — the `type` collision', () => { }); }); +/** + * `combo` is renderer-local — it is not a spec `ChartTypeSchema` value (the + * tripwire for that lives in `@object-ui/types`'s `spec-derived-unions.test.ts`, + * which already reads the spec enum). The spec expresses a combo chart + * per-series, via `ChartSeries.type`, and that override used to be parsed and + * carried and then dropped: only the renderer's `chartType === 'combo'` branch + * read it, so an author writing the protocol got the base family drawn instead. + */ +describe('effectiveChartFamily — a combo is derived from the series (#2945)', () => { + const s = (chartType?: 'bar' | 'line' | 'area') => (chartType ? { chartType } : {}); + + it('derives a combo when a series overrides the family', () => { + // The case that rendered silently wrong: `margin` drew as a bar. + expect(effectiveChartFamily('bar', [s(), s('line')])).toBe('combo'); + }); + + it('leaves a chart whose series all agree alone', () => { + expect(effectiveChartFamily('bar', [s(), s()])).toBe('bar'); + expect(effectiveChartFamily('bar', [s('bar'), s('bar')])).toBe('bar'); + // Agreeing with each other but not with the chart is still one family. + expect(effectiveChartFamily('area', [s('area'), s('area')])).toBe('area'); + }); + + it('reads the override against the chart\'s own family, not against `bar`', () => { + // On an area chart, `area` series are the base — not an override. + expect(effectiveChartFamily('area', [s(), s('area')])).toBe('area'); + expect(effectiveChartFamily('area', [s(), s('bar')])).toBe('combo'); + }); + + it('treats `column` as the bar spelling it is', () => { + expect(effectiveChartFamily('column', [s(), s('bar')])).toBe('column'); + expect(effectiveChartFamily('column', [s(), s('line')])).toBe('combo'); + }); + + it('returns an explicit `combo` untouched', () => { + // Internal-shape callers pass it directly (DatasetPreview's mixed-scale + // branch), with series that may declare nothing at all. + expect(effectiveChartFamily('combo', [s(), s()])).toBe('combo'); + expect(effectiveChartFamily('combo', undefined)).toBe('combo'); + }); + + it('does not widen a family that has no per-series meaning', () => { + // A `line` series inside a pie chart names nothing; switching the whole + // chart to a cartesian combo would be a worse answer than ignoring it. + for (const family of ['pie', 'donut', 'funnel', 'radar', 'scatter', 'treemap', 'sankey'] as const) { + expect(effectiveChartFamily(family, [s(), s('line')])).toBe(family); + } + // Same for horizontal bars — the combo renderer plots vertically, so + // deriving one would silently reorient the chart. + expect(effectiveChartFamily('horizontal-bar', [s(), s('line')])).toBe('horizontal-bar'); + }); + + it('needs two series to have a disagreement', () => { + expect(effectiveChartFamily('bar', [s('line')])).toBe('bar'); + expect(effectiveChartFamily('bar', [])).toBe('bar'); + expect(effectiveChartFamily(undefined, [s(), s('line')])).toBeUndefined(); + }); + + it('comboBaseFamily marks an authored combo by having no base', () => { + // The renderer uses exactly this to tell derived from authored: a derived + // combo defaults un-annotated series to the chart's family and binds them + // to the left axis (the spec default), an authored one keeps the legacy + // index/family guess. + expect(comboBaseFamily('bar')).toBe('bar'); + expect(comboBaseFamily('column')).toBe('bar'); + expect(comboBaseFamily('line')).toBe('line'); + expect(comboBaseFamily('area')).toBe('area'); + expect(comboBaseFamily('combo')).toBeUndefined(); + expect(comboBaseFamily('pie')).toBeUndefined(); + expect(comboBaseFamily(undefined)).toBeUndefined(); + }); +}); + describe('formatterFor', () => { it('formats currency', () => { expect(formatterFor('$0,0.00')!(1234.5)).toMatch(/1,234\.50/); diff --git a/packages/plugin-charts/src/normalizeChartSchema.ts b/packages/plugin-charts/src/normalizeChartSchema.ts index 83a5086290..e9f1919d0a 100644 --- a/packages/plugin-charts/src/normalizeChartSchema.ts +++ b/packages/plugin-charts/src/normalizeChartSchema.ts @@ -45,7 +45,25 @@ * widget's `chartConfig`) never collide and pass `type` through untouched. */ -/** A chart family this renderer actually draws. */ +/** + * A chart family this renderer actually draws. + * + * Every member is a `@objectstack/spec` `ChartTypeSchema` value except + * **`combo`**, which is renderer-local and is NOT a spec chart type (#2945). + * The spec models a combo chart per-series instead — `ChartSeries.type`, whose + * field comment reads *"Series type override (combo charts)"* — exactly as it + * models stacking with `ChartSeries.stack` rather than a `stacked-bar` family: + * + * > stacking is not a chart family, it is a property of the series … One `bar` + * > family plus a series-level stack group expresses all three without + * > multiplying the taxonomy. — `spec/src/ui/chart.zod.ts` + * + * So `combo` is 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, and + * {@link effectiveChartFamily} derives it. It stays in the union because + * internal-shape callers pass it explicitly today (`DatasetPreview`) and + * because dropping a name that is already authored breaks those charts. + */ export type ChartFamily = | 'bar' | 'column' | 'horizontal-bar' | 'line' | 'area' @@ -118,11 +136,20 @@ export interface NormalizedAxis { title?: string; } +/** The families that compose on one cartesian plot — see {@link SeriesFamily}. */ +export type SeriesFamily = 'bar' | 'line' | 'area'; + export interface NormalizedSeries { dataKey: string; label?: string; - /** Per-series family override (combo charts). */ - chartType?: 'bar' | 'line' | 'area'; + /** + * Per-series family override — spec `ChartSeries.type`. + * + * The spec types it as the full `ChartTypeSchema`, but only the cartesian + * families compose on one plot (a `pie` series inside a bar chart names + * nothing), so recognition stops at bar/line/area. + */ + chartType?: SeriesFamily; variant?: 'current' | 'comparison' | 'primary'; opacity?: number; dashArray?: string; @@ -310,6 +337,55 @@ export function normalizeChartSchema(schema: unknown): NormalizedChartSchema { return out; } +/** + * Which family an un-annotated series takes when the chart is drawn as a combo, + * or `undefined` when the chart's own family has no per-series meaning. + * + * `undefined` therefore doubles as "this combo was authored, not derived": + * only a cartesian family can be widened into a combo, so a chart that reaches + * the combo renderer with no base family got there by being named `combo`. + */ +export function comboBaseFamily(chartType: string | undefined): SeriesFamily | undefined { + if (chartType === 'bar' || chartType === 'column') return 'bar'; + if (chartType === 'line') return 'line'; + if (chartType === 'area') return 'area'; + return undefined; +} + +/** + * The family the renderer should actually draw. + * + * `ChartSeries.type` is the spec's own way to say "this series is a line on an + * otherwise-bar chart", and until now it was parsed, carried through + * normalization, and then dropped: only the renderer's `chartType === 'combo'` + * branch reads `series[].chartType`, so a chart authored in the spec shape + * + * ```ts + * { type: 'bar', series: [{ name: 'revenue' }, { name: 'margin', type: 'line' }] } + * ``` + * + * drew `margin` as a bar. Silently — the value was right at every layer except + * the last. That is the failure this widens: an author writing the protocol got + * the wrong picture unless they also knew to write objectui's non-spec `combo`. + * + * Only a disagreement derives a combo. A chart whose series all resolve to the + * same family keeps its own family, so nothing that renders correctly today + * changes; and an explicit `combo` is returned untouched. + * + * Pass the chart's EFFECTIVE family (defaults already applied) — the answer + * depends on what an un-annotated series would otherwise have drawn. + */ +export function effectiveChartFamily( + chartType: T, + series: readonly Pick[] | undefined, +): T | 'combo' { + if (chartType === 'combo') return 'combo'; + const base = comboBaseFamily(chartType); + if (!base || !series || series.length < 2) return chartType; + const resolved = series.map((s) => s.chartType ?? base); + return new Set(resolved).size > 1 ? 'combo' : chartType; +} + /** * Build a tick formatter from a spec `ChartAxis.format` string. * diff --git a/packages/types/src/__tests__/spec-derived-unions.test.ts b/packages/types/src/__tests__/spec-derived-unions.test.ts index bbdc25631d..a945e14cba 100644 --- a/packages/types/src/__tests__/spec-derived-unions.test.ts +++ b/packages/types/src/__tests__/spec-derived-unions.test.ts @@ -36,9 +36,10 @@ * only thing that fails CI, which is why the #4074 cases below are written as * identity and membership checks. * - * The last case is the inverse: `navigation` is a name objectui runs that the - * spec does NOT have. It is asserted absent from the spec so that the day the - * spec adopts it, this file fails and names the alias to retire. + * Two cases are the inverse: `navigation` and `combo` are names objectui uses + * that the spec does NOT have. Each is asserted absent from the spec, so that + * the day the spec adopts one, this file fails and names the local thing to + * retire. */ import { describe, it, expect } from 'vitest'; import { @@ -135,6 +136,19 @@ describe('unions derived from a spec vocabulary stay derived (#2944)', () => { expect([...OBJECTUI_LOCAL_ACTION_TYPES]).toEqual(['navigation']); }); + it('`combo` is not a spec chart type — the spec models it per-series', () => { + // `plugin-charts`'s `ChartFamily` carries `combo`, which #2945 listed as + // "promote or delete". Neither: `ChartSeries.type` already exists and its + // own field comment reads "Series type override (combo charts)", so the + // spec expresses a combo per-series, exactly as it expresses stacking with + // `ChartSeries.stack` rather than a `stacked-bar` family. `combo` stays a + // renderer-local marker that `effectiveChartFamily` DERIVES from the series. + // + // Tripwire, same shape as `navigation` above: if the spec ever adopts + // `combo`, this fails and the derivation is what to retire. + expect(optionsOf(SpecChartTypeSchema, 'ChartTypeSchema')).not.toContain('combo'); + }); + it('ReportType includes `joined` — the member the fork dropped', () => { expect(optionsOf(SpecReportType, 'ReportType')).toContain('joined'); });