Skip to content

Commit 0858267

Browse files
fix(app-shell): a blank dataset Field box writes no field key; a field-less dimension holds Save (objectui#11402) (#11408)
Fixes #11402 Clause-②: no ## What changed `DatasetDefaultInspector` no longer writes `field: ''` for a dataset row whose Field box is blank. - **New rows carry no `field` key.** "Add dimension" seeds `{ name: '', type: 'string' }` and "Add measure" seeds `{ name: '', aggregate: 'sum' }`. Neither writes an `id` default or any other field on the author's behalf. - **A blank Field value removes the key.** Both Field combos write through `writeRowField`, which rebuilds the row without `field` when the value is blank. It does not store `''` or `field: undefined`, so the held draft and the saved body agree. A `count` measure with no field saves the one spelling the spec has for count(*): the key is absent. - **A dimension with a blank Field box is held as incomplete.** The inspector reports each dimension whose `field` is absent or blank on its `onBlockingIssuesChange` channel. That count includes a stored `field: ''`. The host already reads this channel for the default-inspector family (objectui#4527, objectui#6900): it holds the Save button, the autosave timer and the shortcut until a field is picked. While the box is blank, that row's Field label also shows the designer's required marker (`RequiredMarker`, objectui#10948). A complete row shows the plain label. Measures are not held: a measure's `field` is optional, and whether its aggregate may go without one is the spec's verdict at save. ## Rulings → where each lands (triage `5941542449`) | Ruling | Where | |---|---| | New rows are seeded without a `field` key. | the two `onAdd` seeds; pins `seeds a new measure row …`, `seeds a new dimension row …` | | A measure with a blank Field box writes no `field`. | `writeRowField` via `pickMeasureField`; pin `⭐ a measure added and saved with a blank Field box carries no field, and the saved dataset parses under DatasetSchema` | | A dimension with a blank Field box shows as incomplete and is not saved with `''`. | the blocking count plus the state-bound marker; pins under `a blank-Field dimension is held as incomplete` | | ⛔ No `id` default. | the seeds; the dimension seed pin uses `toStrictEqual` | ## Which "incomplete" mechanism this reuses, and why it is licensed here I measured what this inspector family has for "incomplete": the `onBlockingIssuesChange` Save gate, the static `RequiredMarker`, and the host's live Zod pass. The Zod pass does not exist for `dataset`: `clientValidation.ts` registers no loader for it. Nothing new was invented. The hold reuses the Save gate, and the marker reuses `RequiredMarker`. A reviewer should weigh one point. The host's note above `blockingReport` in `ResourceEditPage` describes this channel as carrying faults the server does **not** catch, and it keeps schema verdicts advisory. The reason is that a client gate *stricter* than the server would wedge Save on a body the server accepts. - **A field-less dimension.** The installed spec (`@objectstack/spec` 17.5.0, `DatasetDimensionSchema.field` is a required string) refuses it as well. So the hold is never stricter than the spec. A new pin, `the hold is never stricter than the spec …`, asserts `DatasetSchema` refuses that dimension at `dimensions.0.field` and accepts it once a field is picked. It goes red if the key ever turns optional. - **A stored `field: ''`.** This is the one row the hold refuses that 17.5.0 accepts. The ruling covers it ("not saved with `''`"), and the Field box on screen repairs it, so it cannot wedge. - **Without the hold**, dropping the `''` seed alone would send every freshly added dimension to the server without a field. Autosave would then draw a 422 on every "Add dimension" until a field is picked. This is inferred, not measured against a live server: `DatasetDimensionSchema.field` is required on 17.5.0, and `dataset` has no client validator. ## Inherited vs re-run (this run resumed a killed one) - **Inherited:** `93f85530` (the pin) and `200e91d8` (the fix). I did not take either on trust. - **Re-run, red first:** with the inspector put back to its `7728c67c` bytes and the pin file unchanged, 8 tests failed and 1 passed (9 total) at `44fa23cf`. Each failure is on the asserted fact: the seeds carry `field: ''`, the held row has a `field` key, nothing is reported on the blocking channel, and `writeRowField` is absent. The one pass is the spec control, which asserts the spec rather than the inspector. The restore used `git checkout HEAD -- PATH`. I proved it by comparing hashes: the file's blob equals the HEAD blob `725de67b`, and `git diff HEAD` is empty. - **Found and repaired with new commits** (objectui forbids force-push): - `93c6598c`: the inherited pin did not compile under `tsc -p tsconfig.test.json`, the second leg of the package's `type-check`. A field-less literal passed to `writeRowField` read as a weak type and drew TS2353. The rows are now typed. - `44fa23cf`: the inherited fix put the marker on every dimension's Field label. That changed the label text on complete rows and turned `inspectorStrings.i18n-10696`'s lit control red, because it finds the Field combo by the label's exact text. The marker now renders only while the box is blank. That is also closer to the ruling's "shows as incomplete" than a static "required" mark on complete rows. - `e98eb867`: the spec-control pin described above. - **New here:** the changeset (`c5648ec6`). ## Tests and gates — all read at `44fa23cf` Node v22.22.2 from `/opt/nvm` (the container's v22.22.0 is refused by the engine-strict install). - `pnpm --filter @object-ui/app-shell type-check && pnpm --filter @object-ui/app-shell build` exited 0. Both `tsc` legs echoed, and the build printed `✓ dist completeness: 1 package(s) complete`. `tsc -p tsconfig.test.json --listFiles` lists the pin file. I built the dependency closure first with `turbo run build --filter=@object-ui/app-shell... --concurrency=2`: 29 of 29 tasks succeeded. - `pnpm exec vitest run --maxWorkers=2` over every suite in `packages/app-shell/src/views/metadata-admin/inspectors/`, plus the dataset readers `previews/DatasetPreview*`, `DatasetPreview.dimensionLabels-8187` and `dataPreviews.i18n-10862-s3`: `Test Files 125 passed (125)`, `Tests 1727 passed | 1 skipped (1728)`. The skip is an existing `skipIf` on a spec version in `flow-node-config.spec-reconciliation`. - These gates each exited 0: - `pnpm check:control-bytes` - `pnpm check:new-line-citations` (`VERDICT … 0 new citation(s)`) - `node scripts/check-changeset-presence.mjs` - `pnpm changeset:check` - `check:vi-mock-specifiers`, `check:vi-mock-inherit` and `check:vi-mock-override-shape` (the pin mocks `./useDatasetFields`) - `check:test-path-roots` - `check:i18n-keys` - `check:changeset-claims` - `check:pending-changeset-literals` - **Narrowed lint, a measurement rather than a skip.** 1. The population is the package's `eslint .` under the root `eslint.config.js`. Both changed files are linted, not ignored. 2. `--format json` reports 2 files, 0 errors and 4 warnings. One warning is new: `react-refresh/only-export-components` on the `writeRowField` export, the same rule the file's existing `objectChangePatch` export already draws. `lint.yml` sets no `--max-warnings`. 3. Invariance: `eslint.config.js` enables no type-aware linting, and no rule under `eslint-rules/` reads the disk. So this diff cannot move a verdict on an untouched file. - The diff is not on the governed surface: `check-governed-queue-guard.mjs --test` reports `NOT GOVERNED`. ## Readers of these rows (checked for an absent `field`) `DatasetPreview` reads `String(x?.field ?? '')` and skips an empty one. `ReportView`'s drill fallback reads `dimDef?.field || dim`. `missingRelationship` takes `field: string | undefined`. `useDatasetCatalog` never reads `field`. None of them breaks. ## Acceptance notes (observations, not filed) - **A legacy measure stored with `field: ''`** (written by this inspector before this change) is re-sent unchanged unless the author edits it. The card records that a `count` measure carrying it answered 500 on the ObjectQL strategy (the objectstack dev's measurement, not re-taken here). Under objectstack-ai/objectstack#21240 it is refused at `measures.N.field`. The combo has no "clear" affordance, so the in-UI repair is the Source tab. Repairing stored data was not asked for here. - **Clearing a filled Field box.** `InspectorComboField` offers no way to clear a picked value, so the inspector has no UI path to clear a measure's Field box back to count(*). `writeRowField` covers a blank value if one ever arrives. - **Host comments are now narrower than the channel.** `ResourceEditPage`'s Save-button comment and `inspectorBlocking` note name CEL faults and the objectui#6900 refusal as what the channel carries. The dataset dimension hold is now a third kind. --- _Generated by [Claude Code](https://claude.ai/code/session_01JG2jy8a9su7ia4Hx7zxv42)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 9547063 commit 0858267

3 files changed

Lines changed: 311 additions & 20 deletions

File tree

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
---
2+
'@object-ui/app-shell': patch
3+
---
4+
5+
The dataset designer no longer writes `field: ''` for a row whose Field box is blank (objectui#11402).
6+
7+
New dimension and measure rows used to be seeded with `field: ''`, so a Studio author who added a plain row-count measure and left the Field box blank saved `field: ''`. That value means nothing to any reader: the analytics query answered 500 on the ObjectQL strategy for a `count` measure carrying it, and the narrowed dataset schema (objectstack-ai/objectstack#21240) refuses it at save.
8+
9+
- New rows are seeded without a `field` key, and a blank Field value removes the key instead of storing `''`. A `count` measure with no field therefore saves the one spelling the spec has for count(*): the key absent.
10+
- A dimension needs a field. A dimension whose Field box is blank (including a stored `field: ''`) is reported to the editor's Save gate, the same channel that holds Save, autosave and the shortcut for a CEL expression that does not parse, until a field is picked. Its Field label carries the designer's required marker.
11+
- Nothing is filled in on the author's behalf: no `id` or other default field is written.
Lines changed: 213 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,213 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* objectui#11402 — a blank Field box writes no `field` key.
5+
*
6+
* The dataset inspector used to seed every new row with `field: ''`, so a
7+
* Studio author who added a plain row-count measure and left the Field box
8+
* blank saved `field: ''`. The spec has one spelling for "no field" on a
9+
* measure — the key is ABSENT (`DatasetMeasureSchema.field` is optional, and
10+
* only `count` may omit it) — and a dimension's `field` is required, so an
11+
* empty string is a value no reader can use: the analytics door answered 500
12+
* on the ObjectQL strategy for a `count` measure with `field: ''`, and the
13+
* narrowed spec (objectstack-ai/objectstack#21240) refuses it at save.
14+
*
15+
* The rulings this file pins (triage on objectui#11402):
16+
* - new rows are seeded without a `field` key;
17+
* - a measure with a blank Field box writes no `field`;
18+
* - a dimension with a blank Field box shows as incomplete and is not saved
19+
* with `''` — it is reported on the inspector's blocking-issue channel, the
20+
* one the host's Save gate already reads (objectui#4527 / objectui#6900);
21+
* - no `id` default is written on the author's behalf.
22+
*
23+
* The "saved" body below is the draft as the host holds it — patches merged
24+
* shallowly, exactly as `ResourceEditPage` applies `onPatch` — sent through a
25+
* JSON round trip, because the `dataset` type declares no `fromDraft`
26+
* serialiser, so that draft IS the body the save sends.
27+
*/
28+
29+
import * as React from 'react';
30+
import { describe, it, expect, vi, afterEach } from 'vitest';
31+
import { render, screen, cleanup, fireEvent, waitFor } from '@testing-library/react';
32+
import { DatasetSchema } from '@objectstack/spec/ui';
33+
34+
// Stub the catalog hooks so the inspector renders without a MetadataClient /
35+
// network — the sibling suites' stubs, plus two catalog fields to pick from.
36+
vi.mock('./useDatasetFields', () => ({
37+
useObjectOptions: () => ({ options: [], loading: false }),
38+
useDatasetFieldCatalog: () => ({
39+
relationships: [],
40+
fieldOptions: [
41+
{ value: 'region', label: 'Region', type: 'text' },
42+
{ value: 'amount', label: 'Amount', type: 'currency' },
43+
],
44+
loading: false,
45+
}),
46+
useDatasetUsage: () => ({ reports: 0, dashboards: 0, loading: false }),
47+
fieldTypeToDimensionType: (t: string) => (t === 'date' ? 'date' : 'string'),
48+
}));
49+
50+
import { DatasetDefaultInspector, writeRowField } from './DatasetDefaultInspector';
51+
52+
afterEach(cleanup);
53+
54+
type Draft = Record<string, unknown> & {
55+
dimensions?: Array<Record<string, unknown>>;
56+
measures?: Array<Record<string, unknown>>;
57+
};
58+
59+
const BASE: Draft = { name: 'sales', label: 'Sales', object: 'opportunity', dimensions: [], measures: [] };
60+
61+
/** A host that holds the draft and merges each patch the way `ResourceEditPage` does. */
62+
function mountHost(initial: Draft) {
63+
const seen: { draft: Draft; blocking: number[] } = { draft: initial, blocking: [] };
64+
function Host() {
65+
const [draft, setDraft] = React.useState<Draft>(initial);
66+
seen.draft = draft;
67+
return (
68+
<DatasetDefaultInspector
69+
type="dataset"
70+
name="sales"
71+
locale="en-US"
72+
readOnly={false}
73+
draft={draft}
74+
onPatch={(patch) => setDraft((d) => ({ ...d, ...patch }))}
75+
onBlockingIssuesChange={(count) => seen.blocking.push(count)}
76+
/>
77+
);
78+
}
79+
render(<Host />);
80+
return seen;
81+
}
82+
83+
/** The body the save sends: the held draft through the JSON wire. */
84+
const savedBody = (draft: Draft): Draft => JSON.parse(JSON.stringify(draft));
85+
86+
async function pickSelectOption(label: string, option: string) {
87+
fireEvent.keyDown(screen.getByRole('combobox', { name: label }), { key: 'ArrowDown' });
88+
await waitFor(() => expect(screen.queryAllByRole('option').length).toBeGreaterThan(0));
89+
fireEvent.click(screen.getByRole('option', { name: option }));
90+
}
91+
92+
async function pickComboOption(label: string, value: string) {
93+
fireEvent.click(screen.getByRole('combobox', { name: label }));
94+
await waitFor(() => expect(screen.queryAllByRole('option').length).toBeGreaterThan(0));
95+
const item = screen.getAllByRole('option').find((o) => o.textContent?.startsWith(value));
96+
expect(item, `the catalog option ${value}`).toBeTruthy();
97+
fireEvent.click(item!);
98+
}
99+
100+
describe('DatasetDefaultInspector — new rows carry no `field` key (objectui#11402)', () => {
101+
it('seeds a new measure row with no `field` key', () => {
102+
const onPatch = vi.fn();
103+
render(<DatasetDefaultInspector type="dataset" name="sales" locale="en-US" readOnly={false} draft={BASE} onPatch={onPatch} />);
104+
fireEvent.click(screen.getByText('Add measure'));
105+
// `toStrictEqual`, not `toEqual`: a key present with the value `undefined`
106+
// must fail here too — the row has to be built without it.
107+
expect(onPatch.mock.calls[0][0].measures[0]).toStrictEqual({ name: '', aggregate: 'sum' });
108+
});
109+
110+
it('seeds a new dimension row with no `field` key — and no `id` default either', () => {
111+
const onPatch = vi.fn();
112+
render(<DatasetDefaultInspector type="dataset" name="sales" locale="en-US" readOnly={false} draft={BASE} onPatch={onPatch} />);
113+
fireEvent.click(screen.getByText('Add dimension'));
114+
expect(onPatch.mock.calls[0][0].dimensions[0]).toStrictEqual({ name: '', type: 'string' });
115+
});
116+
});
117+
118+
describe('DatasetDefaultInspector — a blank Field box on a measure writes no `field` (objectui#11402)', () => {
119+
it('⭐ a measure added and saved with a blank Field box carries no `field`, and the saved dataset parses under DatasetSchema', async () => {
120+
const seen = mountHost(BASE);
121+
fireEvent.click(screen.getByText('Add measure'));
122+
fireEvent.change(screen.getByPlaceholderText('e.g. revenue'), { target: { value: 'row_count' } });
123+
await pickSelectOption('Aggregate', 'count');
124+
125+
const held = seen.draft.measures![0];
126+
expect(Object.hasOwn(held, 'field'), 'the held row has no `field` key at all').toBe(false);
127+
const body = savedBody(seen.draft);
128+
expect(body.measures![0]).toStrictEqual({ name: 'row_count', aggregate: 'count' });
129+
130+
const parsed = DatasetSchema.safeParse(body);
131+
expect(parsed.success, JSON.stringify(parsed.error?.issues)).toBe(true);
132+
// Control: the same parse refuses a field-less NON-count measure, so the
133+
// green above is a verdict the schema could have withheld.
134+
const control = DatasetSchema.safeParse({ ...body, measures: [{ name: 'row_count', aggregate: 'sum' }] });
135+
expect(control.success).toBe(false);
136+
});
137+
138+
it('a field-less measure is not held: only the dimension side is incomplete without a field', () => {
139+
const seen = mountHost({
140+
...BASE,
141+
dimensions: [{ name: 'region', field: 'region', type: 'string' }],
142+
measures: [{ name: 'row_count', aggregate: 'count' }],
143+
});
144+
expect(seen.blocking.at(-1)).toBe(0);
145+
});
146+
147+
it('a cleared Field value is written as an absent key, never as `\'\'`', () => {
148+
// Typed as the inspector's own row shape: a field-less literal shares no
149+
// key with `{ field?: string }`, so left to inference it reads as a weak
150+
// type and `tsc -p tsconfig.test.json` refuses its other keys.
151+
type Row = { name: string; aggregate: string; field?: string };
152+
const filled: Row = { name: 'n', aggregate: 'sum', field: 'amount' };
153+
const fieldless: Row = { name: 'n', aggregate: 'sum' };
154+
expect(writeRowField(filled, '')).toStrictEqual({ name: 'n', aggregate: 'sum' });
155+
expect(writeRowField(filled, ' ')).toStrictEqual({ name: 'n', aggregate: 'sum' });
156+
expect(writeRowField(fieldless, 'amount')).toStrictEqual({ name: 'n', aggregate: 'sum', field: 'amount' });
157+
});
158+
});
159+
160+
describe('DatasetDefaultInspector — a blank-Field dimension is held as incomplete (objectui#11402)', () => {
161+
it('⭐ a dimension added with a blank Field box is reported to the Save gate until a field is picked', async () => {
162+
const seen = mountHost(BASE);
163+
expect(seen.blocking.at(-1), 'nothing to hold before the row exists').toBe(0);
164+
165+
fireEvent.click(screen.getByText('Add dimension'));
166+
expect(seen.blocking.at(-1), 'the field-less dimension is a blocking issue').toBe(1);
167+
expect(Object.hasOwn(seen.draft.dimensions![0], 'field'), 'and it is not held as `field: \'\'`').toBe(false);
168+
// Shown as incomplete: while the box is blank, the Field label carries the
169+
// designer's required marker (objectui#10948) — the spec requires a
170+
// dimension's field.
171+
const markerOnFieldLabel = () => screen.getByText('Field').closest('label')?.querySelector('[data-required-marker="true"]');
172+
expect(markerOnFieldLabel(), 'the required marker on the blank Field label').toBeTruthy();
173+
174+
await pickComboOption('Field', 'region');
175+
expect(seen.draft.dimensions![0]).toMatchObject({ name: 'region', field: 'region', type: 'string' });
176+
expect(seen.blocking.at(-1), 'a picked field releases the hold').toBe(0);
177+
expect(markerOnFieldLabel(), 'a complete row shows the plain label').toBeFalsy();
178+
});
179+
180+
it('the hold is never stricter than the spec: DatasetSchema refuses the field-less dimension it holds, and takes it once a field is picked', () => {
181+
// The host keeps a verdict the server also returns advisory, because a
182+
// client gate STRICTER than the server would wedge Save on a body the
183+
// server takes (`ResourceEditPage`, the note above `blockingReport`). This
184+
// hold is safe on that count only while the spec itself refuses a
185+
// dimension without a `field` — if `DatasetDimensionSchema.field` ever
186+
// turns optional, this goes red and the hold has to be revisited. (A
187+
// stored `field: ''` is the one row it holds that this spec still takes:
188+
// that one is ruled — not saved with `''` — and the Field box on screen
189+
// repairs it, so it cannot wedge.)
190+
const held = DatasetSchema.safeParse({ ...BASE, dimensions: [{ name: 'region', type: 'string' }] });
191+
expect(held.success).toBe(false);
192+
expect(held.error?.issues.map((i) => i.path.join('.'))).toContain('dimensions.0.field');
193+
const picked = DatasetSchema.safeParse({ ...BASE, dimensions: [{ name: 'region', field: 'region', type: 'string' }] });
194+
expect(picked.success, JSON.stringify(picked.error?.issues)).toBe(true);
195+
});
196+
197+
it('a stored dimension whose field is `\'\'` is held too, so it is not saved again as `\'\'`', () => {
198+
const seen = mountHost({ ...BASE, dimensions: [{ name: 'region', field: '', type: 'string' }] });
199+
expect(seen.blocking.at(-1)).toBe(1);
200+
});
201+
202+
it('counts each incomplete dimension once', () => {
203+
const seen = mountHost({
204+
...BASE,
205+
dimensions: [
206+
{ name: 'a', type: 'string' },
207+
{ name: 'b', field: 'region', type: 'string' },
208+
{ name: 'c', field: '', type: 'string' },
209+
],
210+
});
211+
expect(seen.blocking.at(-1)).toBe(2);
212+
});
213+
});

0 commit comments

Comments
 (0)