fix(charts): a spec series[].type draws, and a spec-shape series plots at all (#2945)

.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 `<Area>` 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.

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

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<any>('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(
+      <AdvancedChartImpl
+        {...base}
+        chartType="bar"
+        series={[{ dataKey: 'revenue' }, { dataKey: 'margin', chartType: 'line' }]}
+      />,
+    );
+    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(
+      <AdvancedChartImpl
+        {...base}
+        chartType="area"
+        series={[{ dataKey: 'revenue' }, { dataKey: 'margin', chartType: 'bar' }]}
+      />,
+    );
+    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(
+      <AdvancedChartImpl
+        {...base}
+        chartType="bar"
+        series={[{ dataKey: 'revenue' }, { dataKey: 'margin' }]}
+      />,
+    );
+    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(
+      <AdvancedChartImpl
+        {...base}
+        chartType="bar"
+        series={[{ dataKey: 'revenue' }, { dataKey: 'margin', chartType: 'line' }]}
+      />,
+    );
+    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(
+      <AdvancedChartImpl
+        {...base}
+        chartType="bar"
+        series={[{ dataKey: 'revenue' }, { dataKey: 'margin', chartType: 'line', yAxis: 'right' }]}
+      />,
+    );
+    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 `<Area>` 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(
+      <AdvancedChartImpl
+        {...base}
+        chartType="combo"
+        series={[{ dataKey: 'revenue', chartType: 'bar' }, { dataKey: 'margin', chartType: 'area' }]}
+      />,
+    );
+    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(
+      <AdvancedChartImpl
+        {...base}
+        chartType="combo"
+        series={[{ dataKey: 'revenue' }, { dataKey: 'margin' }]}
+      />,
+    );
+    expect(bars(container)).toBe(1);
+    expect(lines(container)).toBe(1);
+  });
+});

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<Record<string, any>>;
   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 (
       <ChartFrame title={title} subtitle={subtitle}>
       <ChartContainer config={config} className={className} {...containerProps}>
-        <BarChart data={data}>
+        {/* `ComposedChart`, not `BarChart`, is the Recharts container built to
+            host mixed marks. Under `BarChart` an `<Area>` child renders nothing
+            at all, so the `seriesType === 'area'` arm below was unreachable —
+            an authored combo with an `area` series drew a blank series. */}
+        <ComposedChart data={data}>
           <CartesianGrid {...gridProps} />
           <XAxis dataKey={xAxisKey} {...xAxisCommonProps} />
           <YAxis yAxisId="left" tickLine={false} axisLine={false} tickFormatter={yTickFormatter} width={48} {...yAxisSpecProps(primaryY)} />
@@ -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({
               </Bar>
             );
           })}
-        </BarChart>
+        </ComposedChart>
       </ChartContainer>
       </ChartFrame>
     );