From 3f53c1577655e278bfd1b72858b513250810625c Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 27 Sep 2026 05:45:42 +0000 Subject: [PATCH 1/2] fix(app-shell): switching a report to joined clears the container binding it hides Studio's report inspector committed `{ type: 'joined' }` alone and, in the same render, hid its dataset / values / rows / columns / chart controls while the spec's own `reportForm` hid the whole "Dataset binding" section (`order` included) through `visibleWhen: "data.type != 'joined'"`. A report bound first and switched second kept every one of those keys invisibly; `ReportSchema`'s joined arm refuses a container `dataset` / `rows` / `columns` / `values` (objectstack PR #20160) and has always refused a container `order`, so the save was refused at a path no Properties-tab control could reach. The type picker now drops `dataset`, `values`, `rows`, `columns`, `chart` and `order` in the same patch that commits `type: 'joined'`, as `undefined`-valued keys (the spelling `commitChart` and the sibling inspectors already clear with); only keys the draft carries are named. `runtimeFilter` and `drilldown` are kept (the joined branch reads both), switching between non-joined types keeps the binding, switching away from `joined` restores nothing, and `blocks` is never touched. Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_014mXUNuFomfj24w7s1pZzhN --- .../10746-joined-report-clears-binding.md | 25 ++ ...spector.joinedClearsBinding-10746.test.tsx | 227 ++++++++++++++++++ .../inspectors/ReportDefaultInspector.tsx | 46 +++- 3 files changed, 297 insertions(+), 1 deletion(-) create mode 100644 .changeset/10746-joined-report-clears-binding.md create mode 100644 packages/app-shell/src/views/metadata-admin/inspectors/ReportDefaultInspector.joinedClearsBinding-10746.test.tsx diff --git a/.changeset/10746-joined-report-clears-binding.md b/.changeset/10746-joined-report-clears-binding.md new file mode 100644 index 0000000000..251dc650a3 --- /dev/null +++ b/.changeset/10746-joined-report-clears-binding.md @@ -0,0 +1,25 @@ +--- +'@object-ui/app-shell': patch +--- + +fix(app-shell): switching a report to `joined` clears the container binding it hides (objectui#10746) + +Studio's report inspector committed `{ type: 'joined' }` alone when the type picker moved +to `joined`, and in the same render hid its dataset / values / rows / columns / chart +controls, while the spec's own `reportForm` hid the whole "Dataset binding" section +(`order` included) through `visibleWhen: "data.type != 'joined'"`. A report that was +bound first and switched second kept every one of those keys invisibly. `ReportSchema`'s +joined arm refuses a container `dataset` / `rows` / `columns` / `values` (objectstack PR +#20160: "a `joined` report selects per block — move `KEY` onto `blocks[]`, or delete it") +and has always refused a container `order`, so the save was refused at a path no control +on the Properties tab could reach; only the JSON source tab could delete the key. + +The type picker now drops `dataset`, `values`, `rows`, `columns`, `chart` and `order` +in the same patch that commits `type: 'joined'` — an `undefined`-valued key, the same +spelling the inspector's own chart panel and its sibling inspectors already clear with, +which the host's shallow spread turns into an own key holding `undefined` and +`JSON.stringify` omits on the wire. Only keys the draft actually carries are named, so an +unbound report's switch stays the one-key patch it always was. `runtimeFilter` and +`drilldown` are kept: the joined branch reads both. Switching between two non-joined +types keeps the binding; switching away from `joined` restores nothing — the author +re-binds. `blocks` is never touched. diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/ReportDefaultInspector.joinedClearsBinding-10746.test.tsx b/packages/app-shell/src/views/metadata-admin/inspectors/ReportDefaultInspector.joinedClearsBinding-10746.test.tsx new file mode 100644 index 0000000000..b925584116 --- /dev/null +++ b/packages/app-shell/src/views/metadata-admin/inspectors/ReportDefaultInspector.joinedClearsBinding-10746.test.tsx @@ -0,0 +1,227 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * objectui#10746 — switching a bound report's type to `joined` CLEARS the + * container's selection keys in the same patch, instead of only hiding them. + * + * A joined report selects per block: each block binds its own `dataset` and + * picks its own `rows` / `columns` / `values`. `ReportSchema`'s joined arm + * therefore refuses those four keys on the container — objectstack PR #20160, + * `JOINED_CONTAINER_SELECTION_KEYS` in `packages/spec/src/ui/report.zod.ts`: + * "a `joined` report selects per block — move `KEY` onto `blocks[]`, or delete + * it; on the container it selects nothing." — and has always refused a + * container `order` ("a `joined` report orders per block — move `order` onto + * `blocks[]`."). `chart` is inert on a joined container (objectstack#20161). + * + * The type picker used to commit `{ type: 'joined' }` and nothing else. In the + * same render this inspector hides its dataset / values / rows / columns / + * chart controls (`datasetBound`), and the spec's own `reportForm` hides its + * whole "Dataset binding" section — `order` included — through + * `visibleWhen: "data.type != 'joined'"`. A report bound first and switched + * second kept every one of those keys INVISIBLY; its save was then refused at a + * path no control on the Properties tab could reach. + * + * What the installed `@objectstack/spec` can and cannot measure here: 17.4.0 at + * the time of writing predates PR #20160, so it already refuses a container + * `order` and still ACCEPTS the four selection keys. The parse leg below shows + * the `order` half of the refusal going away; for the four keys the pins assert + * ABSENCE, against the rule quoted above. ⛔ Never weaken those to "parses under + * the installed spec" — that read green on the defect too. + * + * ⚠️ Assertion spelling. `toHaveBeenCalledWith` and `toEqual` treat an + * `undefined`-valued key as absent, so `{ type: 'joined', dataset: undefined }` + * satisfies `toHaveBeenCalledWith({ type: 'joined' })` — the defect and the fix + * read alike under them. Every patch here is read with `toStrictEqual`, which + * tells an own key holding `undefined` from a missing one. + */ + +import { describe, it, expect, vi, afterEach } from 'vitest'; +import { render, screen, cleanup } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; +import { ReportSchema } from '@objectstack/spec/ui'; +import { ReportDefaultInspector } from './ReportDefaultInspector'; +import type { DatasetCatalogEntry } from '../previews/useDatasetCatalog'; + +afterEach(cleanup); + +// One fetch double at MODULE scope, never torn down — the same reason as in +// `ReportDefaultInspector.test.tsx` (objectui#7439 / #6640): `useDatasetSemantics` +// fires a fire-and-forget read that no test body awaits. +vi.stubGlobal( + 'fetch', + vi.fn(async () => new Response('null', { status: 404, headers: { 'content-type': 'application/json' } })), +); + +const catalog: DatasetCatalogEntry[] = [ + { + name: 'sales_metrics', + label: 'Sales metrics', + dimensions: [ + { name: 'stage', type: 'text' }, + { name: 'close_quarter', type: 'date' }, + ], + measures: [ + { name: 'total_amount', aggregate: 'sum' }, + { name: 'deal_count', aggregate: 'count' }, + ], + }, +]; + +const baseProps = { + type: 'report', + name: 'pipeline', + locale: 'en-US' as const, + onSelectionChange: vi.fn(), + datasetCatalogOverride: catalog, +}; + +/** The six keys the type switch clears, in the order the inspector names them. */ +const CLEARED = ['dataset', 'values', 'rows', 'columns', 'chart', 'order'] as const; +/** The four the spec's joined arm refuses on the container (objectstack PR #20160). */ +const SPEC_REFUSED_SELECTION = ['dataset', 'rows', 'columns', 'values'] as const; + +/** One dataset-bound block: where a joined report's data actually lives. */ +const block = { name: 'won_deals', dataset: 'sales_metrics', rows: ['stage'], values: ['total_amount'] }; + +/** + * A matrix report bound the way the Properties tab binds one: every curated + * control has written its key, the spec form has written `order`, and the two + * keys the joined branch READS (`runtimeFilter`, `drilldown`) are set so the + * pins can show them surviving the switch. + */ +const boundDraft = { + name: 'pipeline', + label: 'Pipeline', + type: 'matrix', + dataset: 'sales_metrics', + values: ['total_amount'], + rows: ['stage'], + columns: ['close_quarter'], + chart: { type: 'bar', xAxis: 'stage', yAxis: 'total_amount' }, + order: [{ by: 'total_amount' }], + runtimeFilter: { stage: 'won' }, + drilldown: false, +}; + +type Patch = Record; + +/** Mount on `draft`, pick `optionLabel` in the Report type control, hand back the one patch. */ +async function switchType(draft: Record, optionLabel: string): Promise { + const onPatch = vi.fn<(patch: Patch) => void>(); + render(); + await userEvent.click(screen.getByRole('combobox', { name: 'Report type' })); + await userEvent.click(await screen.findByRole('option', { name: optionLabel })); + expect(onPatch, 'the type picker commits exactly one patch').toHaveBeenCalledTimes(1); + return onPatch.mock.calls[0][0]; +} + +/** + * The host's merge, as both hosts spell it — `ResourceEditPage` passes + * `handleDraftChange((d) => ({ ...d, ...patch }))` and `ReportConfigPanel`'s + * `handlePatch` computes `{ ...draftRef.current, ...patch }`: a shallow spread. + */ +const merge = (draft: Record, patch: Patch): Record => ({ ...draft, ...patch }); + +/** What reaches the wire: the draft JSON-encoded, which is how `client.save` sends a body. */ +const serialised = (doc: Record): Record => JSON.parse(JSON.stringify(doc)); + +describe('ReportDefaultInspector — switching to `joined` clears the container binding (objectui#10746)', () => { + it('THE DEFECT: a bound report switched to `joined` loses dataset / values / rows / columns / chart / order in the SAME patch', async () => { + const patch = await switchType(boundDraft, 'Joined'); + expect(patch).toStrictEqual({ + type: 'joined', + dataset: undefined, + values: undefined, + rows: undefined, + columns: undefined, + chart: undefined, + order: undefined, + }); + }); + + it('a cleared key is an own key holding `undefined` on the draft — not `null`, not `""` — and is ABSENT once serialised', async () => { + const patch = await switchType(boundDraft, 'Joined'); + const committed = merge(boundDraft, patch); + for (const key of CLEARED) { + expect(Object.hasOwn(committed, key), `${key} is an own key after the spread`).toBe(true); + expect(committed[key], `${key} holds undefined, the one value JSON omits`).toBeUndefined(); + } + const document = serialised(committed); + for (const key of CLEARED) expect(document).not.toHaveProperty(key); + // The keys the joined branch READS travel through untouched. + expect(document).toStrictEqual({ + name: 'pipeline', + label: 'Pipeline', + type: 'joined', + runtimeFilter: { stage: 'won' }, + drilldown: false, + }); + }); + + it('the parse leg the installed spec can measure: the container `order` refusal goes away; the four selection keys are asserted absent against the spec rule', async () => { + const draft = { ...boundDraft, blocks: [block] }; + const patch = await switchType(draft, 'Joined'); + // What the defect committed: the type alone, every stale key kept. + const before = ReportSchema.safeParse(serialised(merge(draft, { type: 'joined' }))); + expect( + before.success, + 'INSTRUMENT CONTROL: the installed spec refuses a container `order` on a joined report', + ).toBe(false); + if (!before.success) { + const orderIssue = before.error.issues.find((i) => i.path[0] === 'order'); + expect(orderIssue?.message).toMatch(/^a `joined` report orders per block/); + } + const after = serialised(merge(draft, patch)); + for (const key of SPEC_REFUSED_SELECTION) { + expect( + after, + `\`${key}\` — "a \`joined\` report selects per block — move \`${key}\` onto \`blocks[]\`, or delete it"`, + ).not.toHaveProperty(key); + } + expect(after).not.toHaveProperty('order'); + const parsed = ReportSchema.safeParse(after); + expect(parsed.success, JSON.stringify(parsed.success ? null : parsed.error.issues)).toBe(true); + }); + + it('names only the keys the draft carries: a partially bound report yields exactly those', async () => { + const patch = await switchType( + { name: 'pipeline', label: 'Pipeline', type: 'summary', dataset: 'sales_metrics', values: ['total_amount'] }, + 'Joined', + ); + expect(patch).toStrictEqual({ type: 'joined', dataset: undefined, values: undefined }); + }); + + it('BOUNDARY: an unbound report switching to `joined` stays the one-key patch it always was', async () => { + const patch = await switchType({ name: 'pipeline', label: 'Pipeline', type: 'tabular' }, 'Joined'); + expect(patch).toStrictEqual({ type: 'joined' }); + }); + + it('CONTROL: switching between two non-joined types keeps the binding', async () => { + const patch = await switchType(boundDraft, 'Summary'); + expect(patch).toStrictEqual({ type: 'summary' }); + expect(serialised(merge(boundDraft, patch))).toMatchObject({ + dataset: 'sales_metrics', + values: ['total_amount'], + rows: ['stage'], + columns: ['close_quarter'], + chart: { type: 'bar', xAxis: 'stage', yAxis: 'total_amount' }, + order: [{ by: 'total_amount' }], + }); + }); + + it('CONTROL: `blocks` is untouched when the type becomes `joined`', async () => { + const draft = { ...boundDraft, blocks: [block] }; + const patch = await switchType(draft, 'Joined'); + expect(patch).not.toHaveProperty('blocks'); + expect(merge(draft, patch).blocks, 'the same array, not a copy').toBe(draft.blocks); + }); + + it('`joined` → non-joined restores nothing: the patch is the type alone, `blocks` stays, and the author re-binds', async () => { + const draft = { name: 'multi', label: 'Multi', type: 'joined', blocks: [block] }; + const patch = await switchType(draft, 'Summary'); + expect(patch).toStrictEqual({ type: 'summary' }); + const committed = merge(draft, patch); + expect(committed.blocks).toBe(draft.blocks); + for (const key of CLEARED) expect(committed).not.toHaveProperty(key); + }); +}); diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/ReportDefaultInspector.tsx b/packages/app-shell/src/views/metadata-admin/inspectors/ReportDefaultInspector.tsx index 8639b4b6cd..a9cb7e9c2f 100644 --- a/packages/app-shell/src/views/metadata-admin/inspectors/ReportDefaultInspector.tsx +++ b/packages/app-shell/src/views/metadata-admin/inspectors/ReportDefaultInspector.tsx @@ -71,6 +71,33 @@ const REPORT_CURATED_FIELDS = new Set([ 'chart', // dedicated Chart panel below (type + dataset-aware X/Y pickers) ]); +/** + * Top-level keys dropped from the container in the same patch that commits + * `type: 'joined'` (objectui#10746). A joined report selects per block — each + * block binds its own `dataset` and picks its own `rows` / `columns` / + * `values` — so `ReportSchema`'s joined arm refuses the four selection keys on + * the container ("a `joined` report selects per block — move `KEY` onto + * `blocks[]`, or delete it", objectstack PR #20160) and has always refused a + * container `order` ("a `joined` report orders per block"). `chart` is inert + * on a joined container (objectstack#20161) and is hidden by the same switch. + * + * Why CLEAR rather than un-hide: the moment the type becomes `joined` this + * inspector hides its dataset / values / rows / columns / chart controls + * (`datasetBound` below) and the spec's own `reportForm` hides its whole + * "Dataset binding" section — `order` included — through + * `visibleWhen: "data.type != 'joined'"`. A report that was bound first and + * switched second kept those keys INVISIBLY, and its save was then refused at + * a path no control on the Properties tab could reach. + * + * The clear is an `undefined`-valued key in the shallow patch — the spelling + * `commitChart` below and the sibling inspectors already clear with. The host + * spreads the patch over the draft, so the key becomes an own property holding + * `undefined`, which `JSON.stringify` omits on the wire and which the spec's + * refinement skips. `runtimeFilter` and `drilldown` are deliberately NOT here: + * the joined branch reads both. + */ +const JOINED_CONTAINER_CLEARED_KEYS = ['dataset', 'values', 'rows', 'columns', 'chart', 'order'] as const; + /** * Chart types offered in the curated Chart panel. A dataset-bound report plots * one measure (yAxis) across one dimension (xAxis), so we surface the families @@ -327,6 +354,23 @@ export function ReportDefaultInspector({ // the spec form's repeater) — the top-level binding only applies otherwise. const datasetBound = reportType !== 'joined'; + // objectui#10746 — see `JOINED_CONTAINER_CLEARED_KEYS`. Only keys the draft + // carries are named, so a host that mirrors each patched key to a live + // preview (`ReportConfigPanel`'s `onFieldChange`) sees no phantom clears and + // an unbound report's switch stays the one-key patch it always was. + // Switching AWAY from `joined` restores nothing: the binding was dropped when + // the type left, `onPatch` has no undo stack behind it, and the author + // re-binds. `blocks` is never touched in either direction. + const commitType = (nextType: string) => { + const patch: Record = { type: nextType }; + if (nextType === 'joined') { + for (const key of JOINED_CONTAINER_CLEARED_KEYS) { + if (draft[key] !== undefined) patch[key] = undefined; + } + } + onPatch(patch); + }; + // Graft any server-only top-level fields onto the bundled-spec form so they // are directly editable here even when the bundled `@objectstack/spec` lags // the running server (skew root-cure). @@ -388,7 +432,7 @@ export function ReportDefaultInspector({ label={tr('engine.inspector.report.type')} value={reportType} options={typeOptions} - onCommit={(v) => onPatch({ type: v })} + onCommit={commitType} disabled={readOnly} /> From d4945789f633b54a67d8a5b691936e368f05089e Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 27 Sep 2026 06:27:46 +0000 Subject: [PATCH 2/2] docs(app-shell): state the joined-clear fix's reach and name all three inspector hosts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-1 contract review on the PR: wording only. The changeset now states the limit of the clear — it holds for the patch and the save that follows it, while the metadata-admin editor's draft-over-`layered.effective` rebuild (on load, after each save and after publish) brings a PUBLISHED binding's keys back into the draft until publish and does not repair a report already saved `joined` with stale keys (objectui#10765, the host merge, not this card's). The pin file's merge helper comment names the third host, `StudioDesignSurface`, beside `ResourceEditPage` and `ReportConfigPanel`. Prose and a comment only; no source line changes. Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_014mXUNuFomfj24w7s1pZzhN --- .changeset/10746-joined-report-clears-binding.md | 8 +++++++- ...ortDefaultInspector.joinedClearsBinding-10746.test.tsx | 3 ++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/.changeset/10746-joined-report-clears-binding.md b/.changeset/10746-joined-report-clears-binding.md index 251dc650a3..239f022732 100644 --- a/.changeset/10746-joined-report-clears-binding.md +++ b/.changeset/10746-joined-report-clears-binding.md @@ -22,4 +22,10 @@ which the host's shallow spread turns into an own key holding `undefined` and unbound report's switch stays the one-key patch it always was. `runtimeFilter` and `drilldown` are kept: the joined branch reads both. Switching between two non-joined types keeps the binding; switching away from `joined` restores nothing — the author -re-binds. `blocks` is never touched. +re-binds. `blocks` is never touched. The clear holds for the patch and for the save that +follows it. The metadata-admin editor rebuilds its draft as the served draft spread over +`layered.effective` (on load, after each save and after publish), and `effective` is the +published layer, so a report whose PUBLISHED version was bound gets those keys back in the +draft after the first draft save until it is published, and a report already saved `joined` +with stale keys is not repaired on load. Both are the host's draft-over-baseline merge, +objectui#10765. diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/ReportDefaultInspector.joinedClearsBinding-10746.test.tsx b/packages/app-shell/src/views/metadata-admin/inspectors/ReportDefaultInspector.joinedClearsBinding-10746.test.tsx index b925584116..06ed49a3a7 100644 --- a/packages/app-shell/src/views/metadata-admin/inspectors/ReportDefaultInspector.joinedClearsBinding-10746.test.tsx +++ b/packages/app-shell/src/views/metadata-admin/inspectors/ReportDefaultInspector.joinedClearsBinding-10746.test.tsx @@ -116,7 +116,8 @@ async function switchType(draft: Record, optionLabel: string): } /** - * The host's merge, as both hosts spell it — `ResourceEditPage` passes + * The host's merge, as all three hosts spell it (`ResourceEditPage`, + * `ReportConfigPanel`, `StudioDesignSurface`) — `ResourceEditPage` passes * `handleDraftChange((d) => ({ ...d, ...patch }))` and `ReportConfigPanel`'s * `handlePatch` computes `{ ...draftRef.current, ...patch }`: a shallow spread. */