Skip to content

Commit bb563e9

Browse files
os-zhuangclaude
andauthored
fix(charts): say so when rows carry no category key, instead of drawing an empty axis (#3007)
Rows that carry the measures but NOT the key the chart was told to plot rendered an axis frame with ZERO marks — no throw, no console warning, no legend. Measured before changing anything: `data: [{count:2},{count:8}]` with `xAxisKey="issued"` produced an <svg> whose only text was the Y ticks ("02468") and 0 bar rectangles. That output is indistinguishable from "the query matched nothing", and it is the wrong conclusion. In framework#4033 an analytics bucket was grouped by but never projected, so every trend widget silently drew nothing while the numbers were right the whole time; the backend fix landed there, but nothing on this side would have caught the next producer that drops a column. framework#3701 was the same shape one layer over — a fieldless count keyed its value `undefined` and the chart plotted nothing. Twice is a pattern. The chart now renders an explanatory placeholder naming the missing key, and warns once with BOTH halves of the mismatch (the key expected, the keys the rows actually carry) — that pair is the whole diagnosis, and its absence is what made #4033 expensive to find. Deliberate choices: - Not a throw. A dashboard widget must not take the page down over its own data, and an honest empty state is more actionable than a stack trace. - Absence, not nullishness: `key in row` is false only when the producer never projected the column. A present-but-null value is a legitimate bucket (rows whose grouped dimension is empty collapse into one), and testing `== null` would condemn a working chart. Pinned by its own case. - Category-axis chart types only. scatter plots two measures; treemap/sankey read hierarchy/link fields — flagging those would be a false alarm. - The guard lives in a thin wrapper around the renderer. The condition depends on `data`, which arrives asynchronously, so an early return inside the renderer would change its hook count between renders. plugin-charts: 98/98. Refs objectstack-ai/objectstack#4033 Claude-Session: https://claude.ai/code/session_01TPxNwPnjcn599ujXpU3ibJ Co-authored-by: Claude <noreply@anthropic.com>
1 parent 07580d6 commit bb563e9

2 files changed

Lines changed: 217 additions & 1 deletion

File tree

Lines changed: 140 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,140 @@
1+
/**
2+
* ObjectUI
3+
* Copyright (c) 2024-present ObjectStack Inc.
4+
*
5+
* Unreadable-data guard (framework#4033).
6+
*
7+
* Rows that carry the measures but NOT the category key used to render an axis
8+
* frame with ZERO marks: no throw, no console warning, no legend. Measured, not
9+
* assumed — `data: [{count:2},{count:8}]` with `xAxisKey="issued"` produced an
10+
* `<svg>` containing only the Y tick text `"02468"` and `0` bar rectangles.
11+
*
12+
* That is indistinguishable from "the query matched nothing", and it is the
13+
* wrong conclusion: framework#4033's analytics bucket was grouped by but never
14+
* projected, so trend widgets drew nothing while the numbers were right the
15+
* whole time. framework#3701 was the same shape one layer over (a fieldless
16+
* count keyed its value `undefined` and "the chart plotted nothing"). Twice is
17+
* a pattern; the chart now says so instead of failing silently a third time.
18+
*/
19+
import React from 'react';
20+
import { describe, it, expect, afterEach, vi } from 'vitest';
21+
import { render, cleanup } from '@testing-library/react';
22+
23+
vi.mock('recharts', async () => {
24+
const actual = await vi.importActual<any>('recharts');
25+
return {
26+
...actual,
27+
ResponsiveContainer: ({ children }: any) =>
28+
React.cloneElement(children, { width: 480, height: 320 }),
29+
};
30+
});
31+
32+
import AdvancedChartImpl from './AdvancedChartImpl';
33+
34+
afterEach(cleanup);
35+
36+
/** The #4033 shape: measures present, category key never projected. */
37+
const UNREADABLE = [{ count: 2 }, { count: 8 }];
38+
const READABLE = [
39+
{ issued: '2026-01', count: 2 },
40+
{ issued: '2026-02', count: 8 },
41+
];
42+
const SERIES = [{ dataKey: 'count', label: 'Count' }];
43+
44+
const renderChart = (props: Record<string, unknown>) =>
45+
render(<AdvancedChartImpl series={SERIES as any} {...(props as any)} />);
46+
47+
describe('AdvancedChartImpl — unreadable-data guard (framework#4033)', () => {
48+
it('explains itself instead of drawing an empty axis', () => {
49+
const warn = vi.spyOn(console, 'warn').mockImplementation(() => {});
50+
const { container } = renderChart({
51+
chartType: 'bar', data: UNREADABLE, xAxisKey: 'issued',
52+
});
53+
54+
const placeholder = container.querySelector('[data-chart-error="missing-category-key"]');
55+
expect(placeholder, 'renders the explanatory placeholder').not.toBeNull();
56+
expect(placeholder?.textContent).toContain('issued');
57+
// The old behaviour: an <svg> axis frame with no marks. Anything that still
58+
// paints a plot here would be the silent failure this guard replaces.
59+
expect(container.querySelector('svg')).toBeNull();
60+
warn.mockRestore();
61+
});
62+
63+
it('warns once, naming both the expected key and the keys actually present', () => {
64+
const warn = vi.spyOn(console, 'warn').mockImplementation(() => {});
65+
renderChart({ chartType: 'bar', data: UNREADABLE, xAxisKey: 'issued' });
66+
67+
expect(warn).toHaveBeenCalledTimes(1);
68+
const msg = String(warn.mock.calls[0]?.[0] ?? '');
69+
// Both halves of the mismatch — without the pair the author is left diffing
70+
// a dataset against a chart spec by hand.
71+
expect(msg).toContain('issued');
72+
expect(msg).toContain('count');
73+
warn.mockRestore();
74+
});
75+
76+
it('stays out of the way when the category key IS present', () => {
77+
const warn = vi.spyOn(console, 'warn').mockImplementation(() => {});
78+
const { container } = renderChart({
79+
chartType: 'bar', data: READABLE, xAxisKey: 'issued',
80+
});
81+
82+
expect(container.querySelector('[data-chart-error="missing-category-key"]')).toBeNull();
83+
expect(container.querySelector('svg')).not.toBeNull();
84+
expect(container.textContent).toContain('2026-01');
85+
expect(warn).not.toHaveBeenCalled();
86+
warn.mockRestore();
87+
});
88+
89+
it('treats a present-but-NULL bucket as real data, not as breakage', () => {
90+
// A grouped query legitimately buckets rows whose dimension is empty. The
91+
// key is projected, the value is null — testing `== null` instead of
92+
// `key in row` would have condemned a working chart.
93+
const warn = vi.spyOn(console, 'warn').mockImplementation(() => {});
94+
const { container } = renderChart({
95+
chartType: 'bar',
96+
data: [{ issued: null, count: 3 }, { issued: '2026-02', count: 8 }],
97+
xAxisKey: 'issued',
98+
});
99+
100+
expect(container.querySelector('[data-chart-error="missing-category-key"]')).toBeNull();
101+
expect(warn).not.toHaveBeenCalled();
102+
warn.mockRestore();
103+
});
104+
105+
it('says nothing about an empty result set — that is a legitimate answer', () => {
106+
const warn = vi.spyOn(console, 'warn').mockImplementation(() => {});
107+
const { container } = renderChart({ chartType: 'bar', data: [], xAxisKey: 'issued' });
108+
109+
expect(container.querySelector('[data-chart-error="missing-category-key"]')).toBeNull();
110+
expect(warn).not.toHaveBeenCalled();
111+
warn.mockRestore();
112+
});
113+
114+
it('leaves scatter/treemap/sankey alone — they do not read a category axis', () => {
115+
const warn = vi.spyOn(console, 'warn').mockImplementation(() => {});
116+
for (const chartType of ['scatter', 'treemap', 'sankey'] as const) {
117+
const { container } = renderChart({ chartType, data: UNREADABLE, xAxisKey: 'issued' });
118+
expect(
119+
container.querySelector('[data-chart-error="missing-category-key"]'),
120+
`${chartType} must not be flagged`,
121+
).toBeNull();
122+
cleanup();
123+
}
124+
expect(warn).not.toHaveBeenCalled();
125+
warn.mockRestore();
126+
});
127+
128+
it('guards every category-axis chart type', () => {
129+
const warn = vi.spyOn(console, 'warn').mockImplementation(() => {});
130+
for (const chartType of ['bar', 'column', 'horizontal-bar', 'line', 'area', 'pie', 'donut', 'radar', 'funnel', 'combo'] as const) {
131+
const { container } = renderChart({ chartType, data: UNREADABLE, xAxisKey: 'issued' });
132+
expect(
133+
container.querySelector('[data-chart-error="missing-category-key"]'),
134+
`${chartType} must be flagged`,
135+
).not.toBeNull();
136+
cleanup();
137+
}
138+
warn.mockRestore();
139+
});
140+
});

packages/plugin-charts/src/AdvancedChartImpl.tsx

Lines changed: 77 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -184,6 +184,19 @@ export interface AdvancedChartImplProps {
184184
isAnimationActive?: boolean;
185185
}
186186

187+
/**
188+
* Chart types that plot a CATEGORY axis keyed by `xAxisKey`, and are therefore
189+
* unreadable when no row carries that key (see the guard in the component).
190+
*
191+
* `scatter` is excluded on purpose: both of its axes are numeric measures, so a
192+
* missing `xAxisKey` there is a different question. `treemap` and `sankey` are
193+
* excluded because they read hierarchy/link fields, not a category axis — a
194+
* guard that fired on them would be a false alarm on a working chart.
195+
*/
196+
const CATEGORY_AXIS_CHART_TYPES: ReadonlySet<string> = new Set([
197+
'bar', 'horizontal-bar', 'line', 'area', 'pie', 'donut', 'radar', 'funnel', 'combo',
198+
]);
199+
187200
/**
188201
* Treemap leaf cell — paints each leaf rect with its palette fill + label.
189202
* Hoisted to module scope so it is a stable component reference rather than one
@@ -228,7 +241,7 @@ function ChartFrame({ title, subtitle, children }: { title?: string; subtitle?:
228241
* AdvancedChartImpl - The heavy implementation that imports Recharts with full features
229242
* This component is lazy-loaded to avoid including Recharts in the initial bundle
230243
*/
231-
export default function AdvancedChartImpl({
244+
function AdvancedChartImplInner({
232245
chartType: rawChartType = 'bar',
233246
data: rawData = [],
234247
config = {},
@@ -255,6 +268,7 @@ export default function AdvancedChartImpl({
255268
// 'column' is the spec-level alias for vertical bars; 'horizontal-bar' stays as-is.
256269
const chartType = rawChartType === 'column' ? 'bar' : rawChartType;
257270
const data = Array.isArray(rawData) ? rawData : [];
271+
258272
// Only emit the prop when explicitly disabled, so the default (animated)
259273
// behavior is byte-for-byte unchanged for every existing caller.
260274
const animProps = isAnimationActive === false ? { isAnimationActive: false as const } : {};
@@ -1066,3 +1080,65 @@ export default function AdvancedChartImpl({
10661080
</ChartFrame>
10671081
);
10681082
}
1083+
1084+
1085+
/**
1086+
* Detect the framework#4033 shape from props alone: rows are present, the chart
1087+
* plots a category axis, and NOT ONE row carries the key it was told to plot.
1088+
*/
1089+
function hasNoCategoryKey(props: AdvancedChartImplProps): boolean {
1090+
const chartType = props.chartType === 'column' ? 'bar' : (props.chartType ?? 'bar');
1091+
const rows = Array.isArray(props.data) ? props.data : [];
1092+
const key = props.xAxisKey ?? 'name';
1093+
return (
1094+
CATEGORY_AXIS_CHART_TYPES.has(chartType) &&
1095+
rows.length > 0 &&
1096+
!rows.some((row) => row != null && typeof row === 'object' && key in row)
1097+
);
1098+
}
1099+
1100+
/**
1101+
* Public entry point. A THIN wrapper purely so the unreadable-data guard can
1102+
* short-circuit without breaking the rules of hooks: the condition depends on
1103+
* `data`, which arrives asynchronously, so an early return inside the renderer
1104+
* would change its hook count between renders. The wrapper's own hook list is
1105+
* fixed, and the renderer below is reached with data it can actually plot.
1106+
*/
1107+
export default function AdvancedChartImpl(props: AdvancedChartImplProps) {
1108+
const missingCategoryKey = hasNoCategoryKey(props);
1109+
const xAxisKey = props.xAxisKey ?? 'name';
1110+
const firstRowKeys = React.useMemo(
1111+
() => Object.keys((Array.isArray(props.data) ? props.data[0] : undefined) ?? {}),
1112+
[props.data],
1113+
);
1114+
1115+
React.useEffect(() => {
1116+
if (!missingCategoryKey) return;
1117+
// Names BOTH halves of the mismatch — the key the chart was told to plot and
1118+
// the keys the rows actually carry. That pair is the whole diagnosis;
1119+
// without it an author is left diffing a dataset against a chart spec by
1120+
// hand, which is exactly what made framework#4033 expensive to find.
1121+
console.warn(
1122+
`[chart] no row has the category key "${xAxisKey}" — rendering an explanatory ` +
1123+
`placeholder instead of an empty axis. Row keys: ${JSON.stringify(firstRowKeys)}. ` +
1124+
`A dataset query must PROJECT the dimension it groups by (framework#4033).`,
1125+
);
1126+
}, [missingCategoryKey, xAxisKey, firstRowKeys]);
1127+
1128+
if (missingCategoryKey) {
1129+
return (
1130+
<div
1131+
className={`flex h-full min-h-[120px] w-full items-center justify-center p-4 text-center text-sm text-muted-foreground ${props.className ?? ''}`}
1132+
role="status"
1133+
data-chart-error="missing-category-key"
1134+
>
1135+
<span>
1136+
This chart cannot plot its category axis: no row has a{' '}
1137+
<code className="font-mono">{xAxisKey}</code> field.
1138+
</span>
1139+
</div>
1140+
);
1141+
}
1142+
1143+
return <AdvancedChartImplInner {...props} />;
1144+
}

0 commit comments

Comments
 (0)