Skip to content

Commit 23871f1

Browse files
os-zhuangclaude
andauthored
fix(spec-parity): render the six Tier-1 spec values right instead of silently wrong (#2941) (#2993)
Every row here validated at authoring time and produced output that looked correct and wasn't (#2901 Tier 1 — worse than a blank cell, because nobody asks why a plausible number is wrong): - PivotTable: an out-of-vocabulary aggregation (`count_distinct`, `array_agg`, `string_agg` — engine names with no client renderer) returned a SUM through the `default:` branch. It now refuses loudly with a visible notice; the implemented set is pinned to the spec's 5-name `ChartAggregateFunctionSchema`. - Report chart: 10 of 19 `ChartTypeSchema` values silently drew a bar. `planReportChart` now classifies all 19 — the 12 series families reach the generic chart verbatim (`horizontal-bar` stays horizontal), the 5 single-value families render the measure as a server-aggregated number (dimensionless dataset query — no client math), `table`/`pivot` add no duplicate chart, and out-of-spec values get a visible notice. - `selection.type: 'single'`: `selectable` was a bare truthy, so single rendered the full multi-select UX. The data-table now enforces replace-on-select with no select-all header, and ObjectGrid stops offering the cross-page "select all N matching" escalation in single mode. - Filter `type: 'select'`: the spec names both `select` and `multi-select`, so `select` is single-choice. It now renders radios and replaces the pick (badge × clears); restored/default multi-value selections clamp to one. Inferred (omitted) types keep the historical multi-check UX. - `addRecord.position: 'both'` collapsed to `top` through a binary ternary; both buttons now render. - `tabular` vs `summary` was resolved from whether `rows` was non-empty; the DECLARED type now picks the branch: summary (and degraded matrix) carries a server-computed grand-total footer (`totals: { groupings: [[]] }`, ADR-0021 red line respected), tabular is the same selection as a plain list. Each fix lands with a spec-parity guard per the #2897 template (`summary-spec-parity.test.ts`) plus behavior tests; `components`, `plugin-dashboard` and `plugin-report` gain the `@objectstack/spec` devDependency that makes those guards possible (`components` also pins `zod@^4` so the spec resolves to the same store instance as the rest of the repo instead of forking a second `zod@3` peer variant via shadcn's MCP SDK). Refs #2941, #2901 Co-authored-by: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 parent 9a13622 commit 23871f1

15 files changed

Lines changed: 1094 additions & 55 deletions

File tree

packages/components/package.json

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,7 @@
9090
"tailwindcss": "^4.2.1"
9191
},
9292
"devDependencies": {
93+
"@objectstack/spec": "^17.0.0-rc.0",
9394
"@tailwindcss/postcss": "^4.3.3",
9495
"@types/react": "19.2.17",
9596
"@types/react-dom": "19.2.3",
@@ -100,7 +101,8 @@
100101
"tailwindcss": "^4.3.3",
101102
"typescript": "^6.0.3",
102103
"vite": "^8.1.5",
103-
"vite-plugin-dts": "^5.0.3"
104+
"vite-plugin-dts": "^5.0.3",
105+
"zod": "^4.4.3"
104106
},
105107
"keywords": [
106108
"objectui",
Lines changed: 150 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,150 @@
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+
* Selection mode ↔ spec vocabulary parity + behavior (#2941).
11+
*
12+
* `SelectionConfigSchema.type` (`ui/view.zod.ts`) publishes three names:
13+
* `none | single | multiple`. The table used to treat `selectable` as a bare
14+
* truthy, so a view authored with `selection.type: 'single'` rendered the full
15+
* multi-select UX — per-row checkboxes that accumulate AND a select-all
16+
* header. The output looked right and wasn't: `single` was never
17+
* distinguished from `multiple`.
18+
*
19+
* Contract under test:
20+
* - parity: the table's declared mode set equals the spec enum, both ways;
21+
* - `single` offers no select-all and replaces the selection on each pick;
22+
* - `multiple` (and legacy `true`) keeps the accumulate + select-all UX.
23+
*/
24+
import { describe, it, expect, vi, beforeAll } from 'vitest';
25+
import { fireEvent } from '@testing-library/react';
26+
import '@testing-library/jest-dom';
27+
import { SelectionConfigSchema } from '@objectstack/spec/ui';
28+
import { renderComponent } from './test-utils';
29+
import { SUPPORTED_SELECTION_MODES } from '../renderers/complex/data-table';
30+
31+
beforeAll(async () => {
32+
await import('../renderers');
33+
}, 30000);
34+
35+
/** The spec's selection vocabulary, read through the `.default()` wrapper. */
36+
function specSelectionModes(): string[] {
37+
const typeSchema = (SelectionConfigSchema as unknown as { shape?: Record<string, unknown> })
38+
.shape?.type as { def?: { innerType?: { options?: readonly string[] } } } | undefined;
39+
const options = typeSchema?.def?.innerType?.options;
40+
return Array.isArray(options) ? [...options] : [];
41+
}
42+
43+
const baseSchema = {
44+
type: 'data-table' as const,
45+
columns: [{ header: 'Name', accessorKey: 'name' }],
46+
data: [
47+
{ id: '1', name: 'Alice' },
48+
{ id: '2', name: 'Bob' },
49+
{ id: '3', name: 'Carol' },
50+
],
51+
pagination: false,
52+
searchable: false,
53+
};
54+
55+
describe('data-table selection mode covers the spec selection vocabulary', () => {
56+
const specNames = specSelectionModes();
57+
58+
it('reads a non-empty enum from the spec', () => {
59+
expect(specNames, 'could not read SelectionConfigSchema.shape.type options from the spec').not.toEqual([]);
60+
});
61+
62+
it('implements every selection mode the spec accepts', () => {
63+
const unimplemented = specNames.filter((name) => !SUPPORTED_SELECTION_MODES.has(name));
64+
expect(
65+
unimplemented,
66+
'these pass schema validation but render an undistinguished selection UX — implement them in data-table',
67+
).toEqual([]);
68+
});
69+
70+
it('does not accept selection modes the spec rejects', () => {
71+
const extra = [...SUPPORTED_SELECTION_MODES].filter((name) => !specNames.includes(name));
72+
expect(
73+
extra,
74+
'these are renderer-local dialect — promote them into @objectstack/spec instead',
75+
).toEqual([]);
76+
});
77+
});
78+
79+
describe('data-table selection behavior per mode', () => {
80+
it("'single' renders no select-all header and replaces the selection on each pick", () => {
81+
const onSelectionChange = vi.fn();
82+
const { getAllByRole } = renderComponent({
83+
...baseSchema,
84+
selectable: 'single',
85+
onSelectionChange,
86+
} as any);
87+
88+
// 3 rows → exactly 3 checkboxes; a select-all header would make it 4.
89+
const checkboxes = getAllByRole('checkbox');
90+
expect(checkboxes).toHaveLength(3);
91+
92+
fireEvent.click(checkboxes[0]);
93+
expect(onSelectionChange).toHaveBeenLastCalledWith([expect.objectContaining({ name: 'Alice' })]);
94+
95+
// Picking Bob must REPLACE Alice, never accumulate to two rows.
96+
fireEvent.click(checkboxes[1]);
97+
expect(onSelectionChange).toHaveBeenLastCalledWith([expect.objectContaining({ name: 'Bob' })]);
98+
});
99+
100+
it("'single' allows deselecting the picked row", () => {
101+
const onSelectionChange = vi.fn();
102+
const { getAllByRole } = renderComponent({
103+
...baseSchema,
104+
selectable: 'single',
105+
onSelectionChange,
106+
} as any);
107+
108+
const checkboxes = getAllByRole('checkbox');
109+
fireEvent.click(checkboxes[0]);
110+
fireEvent.click(checkboxes[0]);
111+
expect(onSelectionChange).toHaveBeenLastCalledWith([]);
112+
});
113+
114+
it("'multiple' keeps the select-all header and accumulates picks", () => {
115+
const onSelectionChange = vi.fn();
116+
const { getAllByRole } = renderComponent({
117+
...baseSchema,
118+
selectable: 'multiple',
119+
onSelectionChange,
120+
} as any);
121+
122+
// 3 rows + the select-all header.
123+
const checkboxes = getAllByRole('checkbox');
124+
expect(checkboxes).toHaveLength(4);
125+
126+
// First checkbox is the header select-all; rows follow.
127+
fireEvent.click(checkboxes[1]);
128+
fireEvent.click(checkboxes[2]);
129+
expect(onSelectionChange).toHaveBeenLastCalledWith([
130+
expect.objectContaining({ name: 'Alice' }),
131+
expect.objectContaining({ name: 'Bob' }),
132+
]);
133+
});
134+
135+
it('legacy `selectable: true` still means multi-select', () => {
136+
const { getAllByRole } = renderComponent({
137+
...baseSchema,
138+
selectable: true,
139+
} as any);
140+
expect(getAllByRole('checkbox')).toHaveLength(4);
141+
});
142+
143+
it("'none' renders no selection column", () => {
144+
const { queryAllByRole } = renderComponent({
145+
...baseSchema,
146+
selectable: 'none',
147+
} as any);
148+
expect(queryAllByRole('checkbox')).toHaveLength(0);
149+
});
150+
});

packages/components/src/renderers/complex/data-table.tsx

Lines changed: 44 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -288,14 +288,39 @@ export const DataTableBuiltinRowActionItem: React.FC<{
288288
);
289289
};
290290

291+
/**
292+
* The selection modes this table implements — the renderer half of the spec's
293+
* `SelectionConfigSchema.type` vocabulary (`ui/view.zod.ts`). Kept as an
294+
* explicit export so a parity test can fail the moment either side moves
295+
* (#2941; template: plugin-grid `summary-spec-parity.test.ts`).
296+
*/
297+
export const SUPPORTED_SELECTION_MODES: ReadonlySet<string> = new Set(['none', 'single', 'multiple']);
298+
299+
type SelectionMode = 'none' | 'single' | 'multiple';
300+
301+
/**
302+
* Resolve the `selectable` prop — `boolean | 'single' | 'multiple'` (plus
303+
* `'none'` arriving verbatim from raw SDUI JSON) — to a selection mode.
304+
* `'single'` is a real mode (replace-on-select, no select-all), not a truthy
305+
* alias for `'multiple'`; collapsing the two rendered per-row checkboxes AND
306+
* a select-all header for `selection.type: 'single'` (#2941). Legacy `true`
307+
* and unrecognized truthy strings keep their historical multi-select meaning.
308+
*/
309+
function resolveSelectionMode(selectable: DataTableSchema['selectable'] | 'none'): SelectionMode {
310+
if (selectable === 'single') return 'single';
311+
if (selectable === 'none' || !selectable) return 'none';
312+
return 'multiple';
313+
}
314+
291315
/**
292316
* Enterprise-level data table component with Airtable-like features.
293317
*
294318
* Provides comprehensive table functionality including:
295319
* - Multi-column sorting (ascending/descending/none)
296320
* - Real-time search across all columns
297321
* - Pagination with configurable page sizes
298-
* - Row selection with persistence across pages
322+
* - Row selection with persistence across pages (multi-select), or
323+
* replace-on-select when the view declares `selection.type: 'single'`
299324
* - CSV export of filtered/sorted data
300325
* - Row action buttons (edit/delete)
301326
*
@@ -337,7 +362,7 @@ const DataTableRenderer = ({ schema }: { schema: DataTableSchema }) => {
337362
onPageChange,
338363
onPageSizeChange,
339364
searchable = true,
340-
selectable = false,
365+
selectable: selectableProp = false,
341366
showSelectionCount = true,
342367
selectionResetKey,
343368
sortable = true,
@@ -359,6 +384,11 @@ const DataTableRenderer = ({ schema }: { schema: DataTableSchema }) => {
359384
disableInnerScroll = false,
360385
} = schema;
361386

387+
// 'single' caps the selection at one row (replace-on-select) and drops the
388+
// select-all header; every truthy legacy value keeps meaning 'multiple'.
389+
const selectionMode = resolveSelectionMode(selectableProp);
390+
const selectable = selectionMode !== 'none';
391+
362392
// Ambient design-surface affordance: when a host (Studio) provides it, render
363393
// a trailing "+ add field" column header. `null` for every runtime table, so
364394
// existing tables render unchanged.
@@ -699,7 +729,9 @@ const DataTableRenderer = ({ schema }: { schema: DataTableSchema }) => {
699729
};
700730

701731
const handleSelectRow = (rowId: any, checked: boolean) => {
702-
const newSelected = new Set(selectedRowIds);
732+
// Single mode replaces the previous selection instead of accumulating —
733+
// the spec's 'single' must never hold two rows (#2941).
734+
const newSelected = new Set(selectionMode === 'single' ? [] : selectedRowIds);
703735
if (checked) {
704736
newSelected.add(rowId);
705737
} else {
@@ -1283,10 +1315,15 @@ const DataTableRenderer = ({ schema }: { schema: DataTableSchema }) => {
12831315
<TableRow ref={headerRowRef}>
12841316
{selectable && (
12851317
<TableHead className={cn("w-10 bg-background px-3", frozenColumns > 0 && "sticky left-0 z-20")}>
1286-
<Checkbox
1287-
checked={allPageRowsSelected ? true : somePageRowsSelected ? 'indeterminate' : false}
1288-
onCheckedChange={handleSelectAll}
1289-
/>
1318+
{/* Select-all is a multi-select affordance; a 'single' view
1319+
keeps the column (alignment) but offers no way to select
1320+
more than one row (#2941). */}
1321+
{selectionMode === 'multiple' && (
1322+
<Checkbox
1323+
checked={allPageRowsSelected ? true : somePageRowsSelected ? 'indeterminate' : false}
1324+
onCheckedChange={handleSelectAll}
1325+
/>
1326+
)}
12901327
</TableHead>
12911328
)}
12921329
{showRowNumbers && (

packages/plugin-dashboard/package.json

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@
4242
"react-grid-layout": "^2.2.0 || ^1.4.0"
4343
},
4444
"devDependencies": {
45+
"@objectstack/spec": "^17.0.0-rc.0",
4546
"@types/react-grid-layout": "^2.1.0",
4647
"@vitejs/plugin-react": "^6.0.4",
4748
"react-grid-layout": "^2.2.3",

packages/plugin-dashboard/src/PivotTable.tsx

Lines changed: 35 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,17 @@ function displayKey(key: string, labels?: Record<string, string>): string {
9797
return labels?.[key] ?? key;
9898
}
9999

100+
/**
101+
* The aggregations this pivot computes — the renderer half of the spec's
102+
* `ChartAggregateFunctionSchema` (`ui/chart.zod.ts`), the UI-side subset the
103+
* spec deliberately carved out of the engine's 8-name `AggregationFunction`.
104+
* Engine-level names (`count_distinct`, `array_agg`, `string_agg`) used to
105+
* fall into a `default:` branch that returned a SUM — a plausible wrong total
106+
* with no signal (#2941). They now short-circuit to a visible notice before
107+
* any cell is computed. Exported for the spec-parity test.
108+
*/
109+
export const PIVOT_AGGREGATIONS: ReadonlySet<string> = new Set(['sum', 'count', 'avg', 'min', 'max']);
110+
100111
/** Aggregate an array of numbers with the given function. */
101112
function aggregate(values: number[], fn: PivotAggregation): number {
102113
if (values.length === 0) return 0;
@@ -112,7 +123,10 @@ function aggregate(values: number[], fn: PivotAggregation): number {
112123
case 'max':
113124
return Math.max(...values);
114125
default:
115-
return values.reduce((a, b) => a + b, 0);
126+
// Unreachable: the component refuses to render out-of-vocabulary
127+
// aggregations. NaN (never a silent sum) is the tripwire if a new call
128+
// site skips that gate.
129+
return Number.NaN;
116130
}
117131
}
118132

@@ -228,6 +242,26 @@ export const PivotTable: React.FC<PivotTableProps> = ({ schema, className, rowLa
228242

229243
const fmt = (v: number) => formatValue(v, format);
230244

245+
// Out-of-vocabulary aggregation (e.g. the engine-level `count_distinct`
246+
// arriving through untyped SDUI JSON): refuse loudly instead of quietly
247+
// summing every cell (#2941). Placed after the hooks so their order stays
248+
// stable across renders.
249+
if (!PIVOT_AGGREGATIONS.has(aggregation)) {
250+
return (
251+
<div className={cn('overflow-auto', className)} data-testid="pivot-unsupported-aggregation">
252+
{title && (
253+
<h3 className="text-sm font-semibold mb-2">{title}</h3>
254+
)}
255+
<div role="alert" className="flex flex-col items-center justify-center py-8 text-destructive">
256+
<p className="text-xs">
257+
Unsupported aggregation &ldquo;{String(aggregation)}&rdquo; — this pivot table renders{' '}
258+
{[...PIVOT_AGGREGATIONS].join(', ')}.
259+
</p>
260+
</div>
261+
</div>
262+
);
263+
}
264+
231265
if (data.length === 0) {
232266
return (
233267
<div className={cn('overflow-auto', className)}>

0 commit comments

Comments
 (0)