diff --git a/.changeset/updateview-draft-addressing-4139.md b/.changeset/updateview-draft-addressing-4139.md new file mode 100644 index 0000000000..7cd3775642 --- /dev/null +++ b/.changeset/updateview-draft-addressing-4139.md @@ -0,0 +1,13 @@ +--- +'@object-ui/data-objectstack': patch +--- + +Renaming a freshly-created view now persists — `updateView` reads and writes the same row, instead of reading the published overlay and losing the edit into a rejected partial write + +ADR-0034 stages every runtime-created view as a per-item **draft**: a view made from the `+` tab lives only in the draft row until an explicit Publish, and the UI reads it back through `?preview=draft`. `updateView` addressed neither half of that. Its read went to the published overlay (`client.meta.getItem`, no draft qualifier), which 404s for a draft-only view; a `catch {}` labelled "treat missing as create-equivalent" then substituted `current = {}`, so the read-merge-write cycle merged onto nothing. What went out was the fragment that merge produces — literally `{label, name, object}`, no `viewKind`, no `config` — which the server rejects as an invalid ViewItem (422). Nothing surfaced to the user, and the draft row still held the old label, so the rename simply did not happen. Create, pin and delete were unaffected: they never take this path. + +The read now probes the draft row first and, on a hit, merges onto that body and writes it straight back with `mode: 'draft'`. Whichever row the read resolved is the row the write updates, so the two halves agree by construction rather than by coincidence. Probing the draft **before** the published overlay is what makes it correct for a view that has both: writing the published row while a draft is pending would put the edit somewhere the draft shadows, and Publish would later overwrite it with the pre-edit body — losing the change a second time, further from the cause. A draft edit stays a draft, preserving ADR-0037's guarantee that nothing the preview shows goes live until Publish. Renaming a published view with no draft pending is unchanged, published read to published write. + +The silent catch is gone. A view that resolves in neither home now throws naming the view and the object (creating one is `createView`'s job — no caller of `updateView` relied on the create-equivalent behaviour), and a network, permission or server fault on either read propagates instead of degrading into the partial write that corrupted the row. This turns a class of failure that was previously invisible into an error the existing call sites already catch and surface. + +Set-default and reorder drive the same read-merge-write cycle with `{isDefault}` / `{sortOrder}` patches, so they were emitting the same partial write and are fixed by the same change. diff --git a/packages/data-objectstack/src/index.ts b/packages/data-objectstack/src/index.ts index a4248edcd0..fee753b2df 100644 --- a/packages/data-objectstack/src/index.ts +++ b/packages/data-objectstack/src/index.ts @@ -923,6 +923,54 @@ function viewItemObjectName(item: any): string | undefined { return spec?.data?.object ?? spec?.object ?? spec?.objectName; } +/** + * Unwrap a `?state=draft` view read into its bare body, or `null` when there + * is nothing pending (#4139). + * + * The framework answers draft reads in a `{type, name, item}` envelope while a + * published read is the bare body — an asymmetry `MetadataClient.getDraft` + * documents and deliberately preserves. An empty body is normalized to `null` + * so the caller's "is this view draft-backed?" test is a plain truthiness + * check. Mirrors app-shell's `unwrapDraftBody` (ADR-0034 seam); the two live + * apart because the seam sits above this adapter, not beside it. + */ +function unwrapViewDraft(resp: unknown): Record | null { + if (!resp || typeof resp !== 'object') return null; + const env = resp as Record; + const body = 'item' in env ? env.item : env; + if (!body || typeof body !== 'object') return null; + // Same `{list: {...}}` artifact wrapper the published read unwraps. + const spec = body.list ?? body; + if (!spec || typeof spec !== 'object') return null; + return Object.keys(spec).length > 0 ? (spec as Record) : null; +} + +/** + * Merge a partial view patch onto the CURRENT view document. + * + * ADR-0005 overlay rows store the *full* view document, so a partial update is + * a read-merge-write cycle and the merge must start from real current state — + * merging onto `{}` yields a `{label, name, object}` fragment the server + * rejects (422), which is exactly how a rename used to be lost (#4139). + * + * `name` is forced to the URL segment so the row key and `body.name` agree + * (#2767 P1), and `object` falls back through the two spellings a stored view + * may carry before defaulting to the caller's object. + */ +function mergeViewPatch( + current: Record, + partial: Record, + viewName: string, + objectName: string, +): Record { + return { + ...current, + ...partial, + name: viewName, + object: current?.object || current?.data?.object || objectName, + }; +} + /** * ObjectStack Data Source Adapter * @@ -3005,9 +3053,41 @@ export class ObjectStackAdapter implements DataSource { /** * Apply a partial update to an existing overlay view. Reads the current - * overlay (or seeds from artifact), merges, and writes back. ADR-0005 - * overlay rows store the *full* view document, so partial updates require - * a read-merge-write cycle. + * document, merges, and writes it back. ADR-0005 overlay rows store the + * *full* view document, so partial updates require a read-merge-write cycle. + * + * **Both halves address the same row (#4139).** A view has two possible + * homes and the read must resolve the one the write will target: + * + * - a pending per-item **draft** (`?state=draft` / `?mode=draft`) — where + * ADR-0034 stages every runtime-created view, so a view made from the `+` + * tab lives ONLY here until an explicit Publish; + * - the **published** overlay (`client.meta.getItem` / `saveItem`). + * + * The draft is probed FIRST, and a hit is merged and written straight back + * as a draft. Two things that ordering buys, both load-bearing: + * + * 1. A draft-only view is no longer invisible to the read. It used to 404, + * and a `catch {}` labelled "treat missing as create-equivalent" + * substituted `current = {}` — so a rename merged onto nothing and went + * out as a `{label, name, object}` partial the server rejects (422), + * while the draft row the UI reads back through `?preview=draft` kept the + * old label. The edit was lost with no error surfaced to the user. + * 2. A draft is never bypassed. Writing the published row while a draft is + * pending would put the edit somewhere the draft shadows — and Publish + * would then overwrite it with the pre-edit body, losing the change a + * second time, later, where nothing connects it to this call. + * + * A draft edit stays a draft: `mode: 'draft'` keeps ADR-0037's guarantee + * that nothing the preview shows goes live until Publish. Renaming a + * *published* view (no draft pending) is unchanged — it writes the + * published overlay, as before. + * + * @throws when the view resolves in neither home, or when either read fails + * for any other reason (network, permission). Both used to be swallowed + * and converted into the bad partial write above; a caller that wants a + * view created should call {@link createView}, which is the operation that + * actually means "create". */ async updateView( objectName: string, @@ -3015,21 +3095,47 @@ export class ObjectStackAdapter implements DataSource { partial: Record, ): Promise | void> { await this.connect(); - let current: any = {}; + + // ── Draft-addressed path ──────────────────────────────────────────── + // `MetadataClient.get` answers `null` on 404 (no draft pending) and throws + // on anything else, so a transport failure here is NOT read as "published". + const metaClient = this.metadataClient(); + const draft = unwrapViewDraft( + await metaClient.get('view', viewName, { state: 'draft' }), + ); + if (draft) { + const mergedDraft = mergeViewPatch(draft, partial, viewName, objectName); + await metaClient.save('view', viewName, mergedDraft, { mode: 'draft' }); + this.metadataCache.invalidate?.(`views:${objectName}`); + this.metadataCache.invalidate?.(`view:${objectName}:${viewName}`); + return mergedDraft; + } + + // ── Published-overlay path (unchanged addressing) ──────────────────── + let current: any; try { const r: any = await this.client.meta.getItem('view', viewName); current = (r && (r.item || r)) || {}; // Some endpoints return the bare item; others wrap as {type,name,item} if (current?.list) current = current.list; - } catch { - // Treat missing as create-equivalent + } catch (err) { + if (is404Error(err)) { + // Not a draft, not published, not an artifact — there is nothing to + // merge onto. Fail loudly instead of emitting the partial write. + throw Object.assign( + new Error( + `updateView: view "${viewName}" not found on object "${objectName}"` + + ' — no pending draft and no published overlay. Use createView() to create one.', + ), + { cause: err }, + ); + } + // Network / permission / server fault: surface it. Degrading to a + // create-equivalent write here is what corrupted the row before. + throw err; } - const merged = { - ...current, - ...partial, - name: viewName, - object: current?.object || (current as any)?.data?.object || objectName, - }; + + const merged = mergeViewPatch(current, partial, viewName, objectName); const result: any = await this.client.meta.saveItem('view', viewName, merged); this.metadataCache.invalidate?.(`views:${objectName}`); this.metadataCache.invalidate?.(`view:${objectName}:${viewName}`); diff --git a/packages/data-objectstack/src/updateView.draft.test.ts b/packages/data-objectstack/src/updateView.draft.test.ts new file mode 100644 index 0000000000..97cc998625 --- /dev/null +++ b/packages/data-objectstack/src/updateView.draft.test.ts @@ -0,0 +1,242 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +import { describe, it, expect, vi } from 'vitest'; +import { ObjectStackAdapter } from './index'; + +/** + * `updateView` read/write addressing (#4139). + * + * ADR-0034 routes a runtime-created view through the per-item DRAFT seam + * (`createRuntimeMetadata` → `metadataClient.save(..., { mode: 'draft' })`), + * so a view made from the `+` tab exists ONLY as a draft row until an + * explicit Publish. `updateView` used to read *and* write the published + * overlay on both halves: the read 404ed, a `catch {}` labelled + * "treat missing as create-equivalent" substituted `current = {}`, and the + * merge degenerated into a `{label, name, object}` partial that the server + * rejects (422) — while the draft row the UI actually reads back through + * `?preview=draft` kept the old label. The rename was silently lost. + * + * These pins fix the addressing on BOTH halves: whichever row the read + * resolved is the row the write updates. + */ + +/** The canonical ViewItem envelope `viewEnvelope()` stages as a draft. */ +const DRAFT_VIEW = { + name: 'crm_activity.kanban_board', + object: 'crm_activity', + viewKind: 'list', + label: 'Kanban Board', + config: { + type: 'kanban', + columns: ['subject', 'status', 'due_date'], + kanban: { columns: ['subject', 'status', 'due_date'], groupBy: 'status' }, + data: { provider: 'object', object: 'crm_activity' }, + }, +}; + +interface Harness { + ds: any; + /** Published-overlay writes (`client.meta.saveItem`) — the wrong address for a draft. */ + saveItem: ReturnType; + /** Draft-addressed writes: `[url, parsedBody]` per `PUT ...?mode=draft`. */ + draftPuts: Array<[string, any]>; + /** Every metadata URL the adapter fetched, in order. */ + urls: string[]; +} + +/** + * Build an adapter whose two metadata addresses answer independently. + * + * @param opts.draft body served at `GET /meta/view/:name?state=draft` + * (`null` → 404, i.e. nothing pending), wrapped in the + * `{type,name,item}` envelope the framework sends for + * draft reads. + * @param opts.published what `client.meta.getItem` (the published read) does: + * a body to resolve, or an Error to throw. + */ +function makeDS(opts: { draft: any | null; published: any | Error }): Harness { + const draftPuts: Array<[string, any]> = []; + const urls: string[] = []; + + const fetchImpl = vi.fn(async (input: RequestInfo | URL, init?: RequestInit) => { + const url = String(input); + const json = (body: unknown, status = 200) => + new Response(JSON.stringify(body), { + status, + headers: { 'Content-Type': 'application/json' }, + }); + if (url.includes('/meta/view/')) { + urls.push(url); + if ((init?.method ?? 'GET') === 'PUT') { + draftPuts.push([url, JSON.parse(String(init?.body ?? '{}'))]); + return json({ success: true, version: 2 }); + } + if (url.includes('state=draft')) { + if (opts.draft == null) return json({ error: 'not found' }, 404); + // Draft reads answer the `{type,name,item}` envelope (unlike the + // bare-body published read) — the asymmetry `unwrapDraftBody` exists for. + return json({ type: 'view', name: opts.draft.name, item: opts.draft }); + } + return json({ error: 'not found' }, 404); + } + return json({ success: true, data: { capabilities: {}, routes: {} } }); + }); + + const ds: any = new ObjectStackAdapter({ baseUrl: 'http://test.local', fetch: fetchImpl }); + ds.connected = true; + ds.connectionState = 'connected'; + const saveItem = vi.fn(async () => ({ success: true })); + ds.client = { + meta: { + getItem: vi.fn(async () => { + if (opts.published instanceof Error) throw opts.published; + return { item: opts.published }; + }), + saveItem, + }, + }; + return { ds, saveItem, draftPuts, urls }; +} + +/** A published read that 404s, decorated the way the SDK client decorates. */ +function notFound(): Error { + return Object.assign(new Error('Metadata item not found'), { httpStatus: 404 }); +} + +describe('ObjectStackDataSource.updateView — draft addressing (#4139)', () => { + it('renames a draft-only view by merging onto the DRAFT body and writing the draft back', async () => { + const { ds, saveItem, draftPuts } = makeDS({ draft: DRAFT_VIEW, published: notFound() }); + + await ds.updateView('crm_activity', DRAFT_VIEW.name, { label: 'Pipeline Board' }); + + // The write must be draft-addressed — the published overlay is the wrong + // row, and writing it would publish a view the user never published. + expect(saveItem).not.toHaveBeenCalled(); + expect(draftPuts).toHaveLength(1); + const [url, body] = draftPuts[0]; + expect(url).toContain('mode=draft'); + expect(url).toContain(encodeURIComponent(DRAFT_VIEW.name)); + + // …and it must carry the FULL merged document, not the + // `{label, name, object}` partial the create-equivalent path produced. + expect(body.label).toBe('Pipeline Board'); + expect(body.viewKind).toBe('list'); + expect(body.name).toBe(DRAFT_VIEW.name); + expect(body.object).toBe('crm_activity'); + expect(body.config).toEqual(DRAFT_VIEW.config); + // The pre-fix partial had exactly these three keys — pin the shape so a + // regression cannot pass by carrying the label alone. + expect(Object.keys(body).sort()).toEqual( + ['config', 'label', 'name', 'object', 'viewKind'].sort(), + ); + }); + + it('probes the draft BEFORE the published overlay, so a pending draft is never shadowed', async () => { + // A view with both a published overlay and a pending draft: the edit + // belongs to the draft. Writing the published row instead would be + // overwritten at publish time — the rename would vanish on Publish. + const published = { ...DRAFT_VIEW, label: 'Old Published Label' }; + const { ds, saveItem, draftPuts } = makeDS({ draft: DRAFT_VIEW, published }); + + await ds.updateView('crm_activity', DRAFT_VIEW.name, { label: 'Pipeline Board' }); + + expect(saveItem).not.toHaveBeenCalled(); + expect(draftPuts).toHaveLength(1); + expect(draftPuts[0][1].label).toBe('Pipeline Board'); + }); + + it('carries the full document for a set-default patch too, not just renames', async () => { + // `handleSetDefaultView` drives the same read-merge-write cycle with + // `{isDefault}`, so it degraded into the same partial write (#4139). + const { ds, draftPuts } = makeDS({ draft: DRAFT_VIEW, published: notFound() }); + + await ds.updateView('crm_activity', DRAFT_VIEW.name, { isDefault: true }); + + expect(draftPuts).toHaveLength(1); + const body = draftPuts[0][1]; + expect(body.isDefault).toBe(true); + expect(body.label).toBe('Kanban Board'); + expect(body.config).toEqual(DRAFT_VIEW.config); + }); + + it('CONTROL — a published view with no pending draft still updates the published overlay', async () => { + const published = { + name: 'crm_activity.all', + object: 'crm_activity', + viewKind: 'list', + label: 'All Activities', + config: { type: 'grid', data: { object: 'crm_activity' } }, + }; + const { ds, saveItem, draftPuts } = makeDS({ draft: null, published }); + + await ds.updateView('crm_activity', 'crm_activity.all', { label: 'Everything' }); + + // No draft row → the published path is unchanged. + expect(draftPuts).toHaveLength(0); + expect(saveItem).toHaveBeenCalledTimes(1); + const [type, name, body] = saveItem.mock.calls[0]; + expect(type).toBe('view'); + expect(name).toBe('crm_activity.all'); + expect(body.label).toBe('Everything'); + expect(body.viewKind).toBe('list'); + expect(body.config).toEqual(published.config); + }); + + it('surfaces a genuinely-missing view instead of writing a create-equivalent partial', async () => { + const { ds, saveItem, draftPuts } = makeDS({ draft: null, published: notFound() }); + + await expect( + ds.updateView('crm_activity', 'crm_activity.ghost', { label: 'Nope' }), + ).rejects.toThrow(/crm_activity\.ghost/); + + // The whole point: no partial write is emitted on the way out. + expect(saveItem).not.toHaveBeenCalled(); + expect(draftPuts).toHaveLength(0); + }); + + it('surfaces a transport failure on the published read rather than degrading to a partial write', async () => { + const boom = Object.assign(new Error('Internal Server Error'), { httpStatus: 500 }); + const { ds, saveItem, draftPuts } = makeDS({ draft: null, published: boom }); + + await expect( + ds.updateView('crm_activity', 'crm_activity.all', { label: 'Nope' }), + ).rejects.toThrow('Internal Server Error'); + + expect(saveItem).not.toHaveBeenCalled(); + expect(draftPuts).toHaveLength(0); + }); + + it('surfaces a transport failure on the DRAFT probe rather than falling through to the published row', async () => { + // A 500 on the draft probe is not "no draft pending" — falling through + // would write the published overlay while a draft may still shadow it. + const fetchImpl = vi.fn(async (input: RequestInfo | URL) => { + const url = String(input); + if (url.includes('/meta/view/') && url.includes('state=draft')) { + return new Response(JSON.stringify({ error: 'boom' }), { + status: 503, + headers: { 'Content-Type': 'application/json' }, + }); + } + return new Response(JSON.stringify({ success: true, data: {} }), { + status: 200, + headers: { 'Content-Type': 'application/json' }, + }); + }); + const ds: any = new ObjectStackAdapter({ baseUrl: 'http://test.local', fetch: fetchImpl }); + ds.connected = true; + ds.connectionState = 'connected'; + const saveItem = vi.fn(async () => ({ success: true })); + ds.client = { meta: { getItem: vi.fn(async () => ({ item: DRAFT_VIEW })), saveItem } }; + + await expect( + ds.updateView('crm_activity', DRAFT_VIEW.name, { label: 'Nope' }), + ).rejects.toThrow(); + expect(saveItem).not.toHaveBeenCalled(); + }); +});