Skip to content

Commit fc32921

Browse files
os-justinclaude
andauthored
fix(plugin-dashboard,core): bind an object-bound chart's series to the column the aggregate projects (#8272)
* fix(plugin-dashboard,core): bind an object-bound chart's series to the column the aggregate projects (objectui#8266) A dashboard chart widget with `aggregate: { function: 'count', groupBy: 'status' }` and no `field` rendered a plot frame with the category ticks drawn and zero marks in it — no error, no empty state. Both relays built the series binding as `aggregate?.field || (options.yField || 'value')`, resolving to `'value'`, while an object-bound fieldless count projects its value under the literal `'count'` (framework#3701). Neither renderer guard fires on that: the rows do carry the category key, and the series array is not empty. `chartMeasureKey` (new, `@object-ui/core`) delegates to the contract's own `chartAggregateValueKey` from `@objectstack/spec/ui`. Both relays and the row projection in `plugin-charts` now read it, so the series binding and the column it binds to cannot disagree again. The authored-rows branch in both relays keeps reading the raw `yField`: it is reached only when the widget's rows are a literal array, where there is no aggregate to consult and the author's key names a column of their own rows. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YBWFb5YgMU5dw8p2VKj16S * test(plugin-dashboard): drop the unused `vi` import from the series-key pin The recorder is a plain function component, so nothing in the file spies or mocks. `object-ui/no-unused-imports` is an error rule here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YBWFb5YgMU5dw8p2VKj16S --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 3963414 commit fc32921

10 files changed

Lines changed: 539 additions & 4 deletions
Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
---
2+
'@object-ui/core': patch
3+
'@object-ui/plugin-dashboard': patch
4+
'@object-ui/plugin-charts': patch
5+
---
6+
7+
Fix a dashboard chart widget with a FIELDLESS `count` aggregate plotting nothing
8+
(objectui#8266).
9+
10+
A widget bound to an object with `aggregate: { function: 'count', groupBy: 'status' }`
11+
and no `field` — the normal way to author "how many records per status" — rendered an
12+
empty chart. No error, no empty state: a plot frame with the category ticks drawn and
13+
not one mark in it, which reads exactly like "this object has no rows yet".
14+
15+
**Cause.** The two dashboard relays (`DashboardGridLayout`, `DashboardRenderer`) each
16+
built the series binding as `aggregate?.field || (options.yField || 'value')`, which for
17+
a fieldless count resolves to `'value'`. The rows an object-bound fieldless count
18+
returns are keyed `'count'` — the alias the engine projects `COUNT(*)` under, pinned
19+
since framework#3701. A `dataKey` naming a column no row carries plots nothing, and
20+
neither of the renderer's two guards fires on it: the rows DO carry the category key,
21+
and the series array is not empty.
22+
23+
**Fix.** `chartMeasureKey` is a new `@object-ui/core` export delegating to
24+
`chartAggregateValueKey` in `@objectstack/spec/ui` — the contract's own derivation of
25+
"the value column an object-bound aggregate produces". Both relays now consult it, and
26+
the row-projection side (`aggregateValueKey` in `@object-ui/plugin-charts`) is routed
27+
through the same function, so the two halves of the question cannot drift again.
28+
29+
**What moves on screen.** A chart that was blank now draws. Charts that already drew are
30+
unaffected: a field-bearing aggregate resolves to its raw field under both the old and
31+
the new reading, and a chart with no `aggregate` at all keeps the author's `yField`.
32+
One authored key changes meaning: a `yField` written on an object-bound chart that
33+
ALSO declares an aggregate no longer wins over the aggregate's own column — it named a
34+
record column that a grouped aggregate never returns, so it plotted nothing before.
35+
36+
**Not fixed here, and out of scope.** The same widget with no `options.xField` is
37+
refused by the category-axis guard naming `name`, a key the author never wrote (they
38+
wrote `aggregate.groupBy`). That is the category half of the same relay gap and is
39+
filed separately.

‎packages/core/src/index.ts‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,10 @@ export * from './utils/dashboard-filters.js';
7272
export * from './utils/merge-filters.js';
7373
export * from './utils/compare-to.js';
7474
export * from './utils/chart-series.js';
75+
// "Which result column carries the measure?" — one answer for the row
76+
// projection and the series binding alike, delegated to the spec's own
77+
// derivation so the two cannot drift (objectui#8266).
78+
export * from './utils/chart-measure-key.js';
7579
// The AUTHORED half of a dataset-bound chart (objectui#4229's data/presentation
7680
// split), shared by the dashboard widget and the report's embedded chart so the
7781
// same spec keys are lowered identically on both (objectui#4877).
Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,61 @@
1+
/**
2+
* ObjectUI
3+
* Copyright (c) 2024-present ObjectStack Inc.
4+
*
5+
* This source code is licensed under the MIT license found in the
6+
* LICENSE file in the root directory of this source tree.
7+
*/
8+
9+
/**
10+
* objectui#8266 — the ONE answer to "which result column carries the measure?".
11+
*
12+
* The regression this exists for is the FIELDLESS COUNT arm: it is the only
13+
* shape where the contract's answer and a `aggregate?.field || yField` read
14+
* disagree, and it is the normal way to author "how many records per status".
15+
* Every other arm agrees under both readings, which is precisely why the
16+
* disagreement survived in the tree — see the two dashboard relays, pinned in
17+
* `plugin-dashboard/src/__tests__/DashboardChart.countSeriesKey-8266.test.tsx`.
18+
*
19+
* The fallback arms are the other half: a floor that fires too eagerly would
20+
* take the caller's `yField` back over an aggregate the contract CAN answer for,
21+
* re-creating the bug through the fix.
22+
*/
23+
import { describe, it, expect } from 'vitest';
24+
import { chartMeasureKey } from './chart-measure-key';
25+
26+
describe('chartMeasureKey — the contract answers', () => {
27+
it('is the raw field for a field-bearing aggregate, never the caller floor', () => {
28+
expect(chartMeasureKey({ field: 'amount', function: 'sum' }, 'value')).toBe('amount');
29+
});
30+
31+
it('is the literal "count" for a fieldless count — NOT the caller floor', () => {
32+
// THE REGRESSION. `aggregate?.field || yField` answered 'value' here, and
33+
// the rows carry 'count', so the chart plotted nothing and said nothing.
34+
expect(chartMeasureKey({ function: 'count', groupBy: 'status' }, 'value')).toBe('count');
35+
});
36+
37+
it('still prefers an explicit field even for count', () => {
38+
expect(chartMeasureKey({ field: 'amount', function: 'count' }, 'value')).toBe('amount');
39+
});
40+
41+
it('ignores a caller floor the author chose, when the aggregate answers', () => {
42+
// An authored `yField` names a column an object-bound aggregate does not
43+
// project. Honouring it would plot nothing, exactly as 'value' did.
44+
expect(chartMeasureKey({ function: 'count', groupBy: 'status' }, 'total')).toBe('count');
45+
});
46+
});
47+
48+
describe('chartMeasureKey — the caller floor, only where the contract is silent', () => {
49+
it('falls back when there is no aggregate at all', () => {
50+
// A provider whose rows are raw records: the author's yField IS the key.
51+
expect(chartMeasureKey(undefined, 'value')).toBe('value');
52+
expect(chartMeasureKey(undefined, 'amount')).toBe('amount');
53+
});
54+
55+
it('falls back for an aggregate shape ChartAggregateSchema rejects', () => {
56+
// "only 'count' may omit field" — a fieldless sum is not a declaration the
57+
// spec admits, so the contract has no column to name.
58+
expect(chartMeasureKey({ function: 'sum' }, 'value')).toBe('value');
59+
expect(chartMeasureKey({}, 'value')).toBe('value');
60+
});
61+
});
Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,73 @@
1+
/**
2+
* ObjectUI
3+
* Copyright (c) 2024-present ObjectStack Inc.
4+
*
5+
* This source code is licensed under the MIT license found in the
6+
* LICENSE file in the root directory of this source tree.
7+
*/
8+
9+
/**
10+
* chart-measure-key — the ONE answer to "which result column carries the
11+
* measure?" for an object-bound chart (objectui#8266).
12+
*
13+
* ## The disagreement this replaces
14+
*
15+
* The question was answered independently in three places, and for a FIELDLESS
16+
* `count` — the normal way to author "how many records per status", and the
17+
* most common dashboard chart there is — two of them answered differently:
18+
*
19+
* - the ROW PROJECTION (`aggregateValueKey` / `aggregateRecords` /
20+
* `runAggregate`'s alias, all in `plugin-charts/src/ObjectChart.tsx`) keys
21+
* the value column `'count'`, the alias the engine projects `COUNT(*)`
22+
* under;
23+
* - the SERIES BINDING, spelled twice in `plugin-dashboard`
24+
* (`DashboardGridLayout.tsx` and `DashboardRenderer.tsx`), read
25+
* `aggregate.field || (options.yField || 'value')` and so bound `'value'`.
26+
*
27+
* A `dataKey` naming a column no row carries plots nothing, and nothing says
28+
* so: measured on `origin/main` `0fa7a9c83`, rows `[{status:'open',count:2},
29+
* {status:'paid',count:5}]` with `series:[{dataKey:'value'}]` rendered a
30+
* `.recharts-surface` with the category ticks "open"/"paid", **0 bars, 0
31+
* rectangles**, no refusal and no empty state — the same rows under
32+
* `dataKey:'count'` drew 2 rectangles. An empty chart reads as "no data yet",
33+
* which is exactly what an author with a genuinely empty object also sees.
34+
*
35+
* ## Why the answer is delegated rather than restated
36+
*
37+
* `chartAggregateValueKey` in `@objectstack/spec/ui` is the CONTRACT's own
38+
* derivation — "the VALUE column an object-bound aggregate produces … what a
39+
* chart's series / y-axis binding must name". Producers and checkers already
40+
* read it. Restating the rule here would make this file a fourth opinion of the
41+
* same question, which is the shape (objectui#5042 / #7544 / #8193 / #8168)
42+
* this module exists to end. So the rule lives upstream in the spec and this is
43+
* the seam objectui-side callers share, exactly as `humanizeLabel` (objectui#5444)
44+
* and `buildChartSeries` (ADR-0021) already are.
45+
*
46+
* ⛔ The inverse fix — making the row projection key a fieldless count
47+
* `'value'` — is not available: it contradicts a pin the tree already carries
48+
* (`ObjectChart.aggregateResultColumns.test.ts`) and renames the column every
49+
* other consumer reads, including the engine's own `COUNT(*)` alias.
50+
*/
51+
52+
import { chartAggregateValueKey, type ChartAggregateLike } from '@objectstack/spec/ui';
53+
54+
export type { ChartAggregateLike };
55+
56+
/**
57+
* The result column a chart's series must bind to.
58+
*
59+
* `fallback` is the caller's own floor, used ONLY when the contract has no
60+
* answer — a chart that declares no `aggregate` at all (its rows are raw
61+
* records or authored literals, where the author's `yField` is the right key),
62+
* or an aggregate shape `ChartAggregateSchema` already rejects. It is never a
63+
* second opinion about an aggregate the contract CAN answer for.
64+
*
65+
* @param aggregate the chart's inline aggregate, or `undefined`
66+
* @param fallback the key to bind when the contract has no answer
67+
*/
68+
export function chartMeasureKey(
69+
aggregate: ChartAggregateLike | undefined,
70+
fallback: string,
71+
): string {
72+
return chartAggregateValueKey(aggregate) ?? fallback;
73+
}

‎packages/plugin-charts/src/ObjectChart.aggregateResultColumns.test.ts‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,29 @@ describe('aggregateValueKey', () => {
3737
it('prefers an explicit field even for count', () => {
3838
expect(aggregateValueKey({ field: 'total', function: 'count' })).toBe('total');
3939
});
40+
41+
/**
42+
* objectui#8266 routed this function through `chartMeasureKey`
43+
* (`@object-ui/core`), which delegates to the contract's own
44+
* `chartAggregateValueKey`, so the SERIES binding the dashboard relays
45+
* compose cannot drift from the column this projects. The delegation is
46+
* behaviour-preserving, and these two arms are the only shapes where that
47+
* claim is not already covered above: the contract answers `undefined` for
48+
* them (`ChartAggregateSchema` refuses a fieldless non-count outright), so
49+
* everything after the delegation is this renderer's own floor.
50+
*
51+
* They are unreachable from validated metadata and reachable from
52+
* unvalidated. The floor exists so such a row keys a COLUMN rather than the
53+
* literal string "undefined", which is the regression the file below pins.
54+
*/
55+
it('floors a fieldless non-count on the function name, as before the delegation', () => {
56+
expect(aggregateValueKey({ function: 'sum' })).toBe('sum');
57+
expect(aggregateValueKey({ function: 'avg' })).toBe('avg');
58+
});
59+
60+
it('floors an empty aggregate on "count", as before the delegation', () => {
61+
expect(aggregateValueKey({})).toBe('count');
62+
});
4063
});
4164

4265
describe('aggregateRecords — result columns', () => {
Lines changed: 138 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,138 @@
1+
/**
2+
* ObjectUI
3+
* Copyright (c) 2024-present ObjectStack Inc.
4+
*
5+
* This source code is licensed under the MIT license found in the
6+
* LICENSE file in the root directory of this source tree.
7+
*/
8+
9+
/**
10+
* objectui#8266, the RENDER half — what a `dataKey` naming a column the rows do
11+
* not carry actually draws.
12+
*
13+
* `plugin-dashboard/src/__tests__/DashboardChart.countSeriesKey-8266.test.tsx`
14+
* pins which column the two dashboard relays name. That is necessary and not
15+
* sufficient: a seam assertion cannot tell a honoured binding from an ignored
16+
* one, and the whole claim of that card is that the mismatch is SILENT. So this
17+
* file renders the real chain — `ChartRenderer` → `normalizeChartSchema` →
18+
* `AdvancedChartImpl` — over the rows a fieldless count actually returns, once
19+
* under each key, and reads the DOM.
20+
*
21+
* It lives here because `recharts` resolves inside `plugin-charts` alone, so
22+
* this is the only package that can mock `ResponsiveContainer` to a measured
23+
* box and count marks at all (the same reason
24+
* `plugin-dashboard/src/__tests__/DatasetWidget.chartConfig.dom.test.tsx`
25+
* states for its own split).
26+
*
27+
* ## Measured on `origin/main` `0fa7a9c83`, before any fix
28+
*
29+
* dataKey 'value' (what the relays composed): surface drawn, x ticks
30+
* ["open","paid"], 0 `.recharts-bar`, 0 `.recharts-rectangle`, NO
31+
* `[data-chart-error]`, NO `chart-empty-state`.
32+
* dataKey 'count' (the column the rows carry): 1 `.recharts-bar`,
33+
* 2 `.recharts-rectangle`, y ticks 0..8.
34+
*
35+
* Same rows, same harness — which is what makes the zero a statement about the
36+
* binding rather than about the harness. The zero arm is kept as a pin
37+
* deliberately: it is the reason the seam file's assertions are worth making,
38+
* and if a future guard DOES start refusing this shape loudly, this is the test
39+
* that must be re-decided rather than a silent behaviour change nobody notices.
40+
*
41+
* ⚠️ Mark counts are harness-bound (`ResponsiveContainer` is fixed at 480x320
42+
* here); they are re-derived in this file and never carried in from another.
43+
*/
44+
45+
import React from 'react';
46+
import { describe, it, expect, vi, afterEach } from 'vitest';
47+
import { render, screen, cleanup, waitFor } from '@testing-library/react';
48+
49+
// Recharts measures via ResizeObserver, which reports 0x0 under the headless
50+
// DOM, so nothing paints. Fix its size — the shim every render test in this
51+
// package uses.
52+
vi.mock('recharts', async () => {
53+
const actual = await vi.importActual<any>('recharts');
54+
return {
55+
...actual,
56+
ResponsiveContainer: ({ children }: any) =>
57+
React.cloneElement(children, { width: 480, height: 320 }),
58+
};
59+
});
60+
61+
import { ChartRenderer } from './ChartRenderer';
62+
import { aggregateRecords } from './ObjectChart';
63+
64+
afterEach(cleanup);
65+
66+
/**
67+
* Not transcribed — produced by the very builder the row projection uses, so a
68+
* change to the projected column name lands in this fixture instead of leaving
69+
* the pin asserting against a shape the product stopped emitting.
70+
*/
71+
const COUNT_ROWS = aggregateRecords(
72+
[
73+
{ status: 'open' }, { status: 'open' },
74+
{ status: 'paid' }, { status: 'paid' }, { status: 'paid' }, { status: 'paid' }, { status: 'paid' },
75+
],
76+
{ function: 'count', groupBy: 'status' },
77+
);
78+
79+
const drawWith = async (dataKey: string) => {
80+
const { container } = render(
81+
<ChartRenderer
82+
schema={{
83+
chartType: 'bar',
84+
data: COUNT_ROWS,
85+
xAxisKey: 'status',
86+
series: [{ dataKey }],
87+
isAnimationActive: false,
88+
} as any}
89+
/>,
90+
);
91+
// `AdvancedChartImpl` is lazy — wait for the real outcome, not the skeleton.
92+
// Either terminal state satisfies this, so a refusal is never mistaken for a
93+
// timeout and a timeout is never read as "it drew nothing".
94+
await waitFor(() => {
95+
if (!container.querySelector('.recharts-surface') && !container.querySelector('[data-chart-error]')) {
96+
throw new Error('neither a plot nor a refusal');
97+
}
98+
}, { timeout: 5000 });
99+
return {
100+
marks: container.querySelectorAll('.recharts-rectangle').length,
101+
series: container.querySelectorAll('.recharts-bar').length,
102+
refusal: container.querySelector('[data-chart-error]')?.getAttribute('data-chart-error') ?? null,
103+
emptyState: !!screen.queryByTestId('chart-empty-state'),
104+
ticks: Array.from(container.querySelectorAll('.recharts-cartesian-axis-tick-value')).map((n) => n.textContent),
105+
};
106+
};
107+
108+
describe('a fieldless count projects its value under "count" (objectui#8266)', () => {
109+
it('is the shape the row builder emits — the premise the rest of the file rests on', () => {
110+
expect(COUNT_ROWS).toEqual([
111+
{ status: 'open', count: 2 },
112+
{ status: 'paid', count: 5 },
113+
]);
114+
});
115+
116+
it('draws its marks when the series names that column', async () => {
117+
const drawn = await drawWith('count');
118+
expect(drawn.refusal).toBeNull();
119+
expect(drawn.series).toBe(1);
120+
expect(drawn.marks).toBe(2);
121+
// The values really reached an axis, so "it drew" is not just a container.
122+
expect(drawn.ticks).toEqual(expect.arrayContaining(['open', 'paid', '0', '2', '4']));
123+
});
124+
125+
it('draws NOTHING, silently, when the series names "value" instead', async () => {
126+
const drawn = await drawWith('value');
127+
// The failure this card is about: a plot frame with the categories on it…
128+
expect(drawn.ticks).toEqual(['open', 'paid']);
129+
// …and not one mark in it.
130+
expect(drawn.marks).toBe(0);
131+
expect(drawn.series).toBe(0);
132+
// …and nothing anywhere says so. Both guards this renderer carries decline:
133+
// `hasNoCategoryKey` is satisfied (the rows DO have `status`) and
134+
// `hasNoPlottableSeries` keys on `series: []`, which this is not.
135+
expect(drawn.refusal).toBeNull();
136+
expect(drawn.emptyState).toBe(false);
137+
});
138+
});

‎packages/plugin-charts/src/ObjectChart.tsx‎

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ import React, { useState, useEffect, useContext, useCallback, useMemo } from 're
33
import { useDataScope, SchemaRendererContext, SchemaRenderer, useDrillNavigation, useFilterScope, ElementDataSourceGate, type ElementDataSourceMapping } from '@object-ui/react';
44
import { ChartRenderer } from './ChartRenderer';
55
import { normalizeChartSchema } from './normalizeChartSchema';
6-
import { ComponentRegistry, humanizeLabel, extractRecords, computeDrillFilter, isDrillEnabled, resolveDrillTitle, resolveFilterPlaceholders, resolveContextTokens, shiftFilterByCompareTo, compareToTrendLabelKey, buildChartSeries, buildOptionColorMap, deriveDimensionLabelMaps, dimensionOptionTranslator, loadDimensionFieldMeta, relabelDimensions, localizeFieldOptions, elementDataSourceBlock, type DimensionFieldMeta, type CompareToConfig, type DrillEvent, type ChartResultField, type ChartSegmentClickEvent } from '@object-ui/core';
6+
import { ComponentRegistry, chartMeasureKey, humanizeLabel, extractRecords, computeDrillFilter, isDrillEnabled, resolveDrillTitle, resolveFilterPlaceholders, resolveContextTokens, shiftFilterByCompareTo, compareToTrendLabelKey, buildChartSeries, buildOptionColorMap, deriveDimensionLabelMaps, dimensionOptionTranslator, loadDimensionFieldMeta, relabelDimensions, localizeFieldOptions, elementDataSourceBlock, type DimensionFieldMeta, type CompareToConfig, type DrillEvent, type ChartResultField, type ChartSegmentClickEvent } from '@object-ui/core';
77
import { Sheet, SheetContent, SheetHeader, SheetTitle, Dialog, DialogContent, DialogHeader, DialogTitle, RefreshIndicator, Button, ChartSkeleton, DataEmptyState } from '@object-ui/components';
88
import { AlertCircle, ArrowUpRight, Inbox } from 'lucide-react';
99
import { builtinAggregateLabels, useSafeFieldLabel, useSafeTranslate } from '@object-ui/i18n';
@@ -35,7 +35,17 @@ export { humanizeLabel };
3535
* agrees on one key instead of each re-deriving it.
3636
*/
3737
export function aggregateValueKey(aggregate: { field?: string; function?: string }): string {
38-
return aggregate.field || aggregate.function || 'count';
38+
// The contract's own derivation answers every aggregate `ChartAggregateSchema`
39+
// ADMITS; the tail is this renderer's floor for shapes it REJECTS (a
40+
// non-count aggregate that names no field, or an empty bag), which reach this
41+
// function only from unvalidated metadata and must still key a column rather
42+
// than the string "undefined". Byte-identical to the three-rung `field ||
43+
// function || 'count'` it replaces — the two extra rungs are unreachable for
44+
// any aggregate the spec answers for. Delegated rather than restated so this
45+
// cannot drift from the SERIES binding the dashboard relays compose, which is
46+
// exactly how a fieldless count came to project `'count'` and be plotted as
47+
// `'value'` (objectui#8266).
48+
return chartMeasureKey(aggregate, aggregate.function || 'count');
3949
}
4050

4151
/**

0 commit comments

Comments
 (0)