From 0c3b4ac919e9fe441123aa86bc28916844f4274b Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 13 Aug 2026 09:17:36 +0000 Subject: [PATCH] fix(data-objectstack): deleteView removes every home the view has (#4479) A view has two homes -- the pending per-item draft (DELETE /api/v1/meta/view/:name?state=draft) and the published overlay (DELETE /api/v1/meta/view/:name). deleteView addressed only the second, unqualified, so deleting a draft-only view fired at the published overlay, the server answered 200 reset:false "nothing to delete", the draft survived and the tab was back after reload. Not the mechanical mirror of #4139: a draft-first-ONLY delete would discard just the draft on a published+draft pair, silently downgrading Delete view into Discard draft (an operation that already exists, discardRuntimeDraft). persistRuntimeMetadata stages every runtime edit as a draft, so pairs are routine. Both homes are now deleted, draft first, so a mid-operation fault leaves the published overlay intact and the delete cleanly retryable. Two blind calls, no probe: measured against the framework's deleteMetaItem, a missing home answers 200 reset:false, never 404. One transport: both halves go through MetadataClient.reset, which issues the byte-identical request for the published half and collapses two error shapes into one. Receipt widened additively with optional per-home outcomes; deleted is true only when no home remains and at least one held a row. A published-half failure after the draft was discarded throws with the partial state on the error's outcome rather than rounding to true. invalidateViewKeys moves into a finally so it fires once on every outcome including the throw. Red-first: 11 new pins red against unfixed code, 4 must-not-change assertions green throughout; reverse-verified by restoring the unqualified delete. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_017Qqyix2QcnpUC9XeYVDzx3 --- .../delete-view-removes-every-home-4479.md | 23 ++ .../src/deleteView.homes.test.ts | 389 ++++++++++++++++++ packages/data-objectstack/src/index.ts | 161 +++++++- .../src/viewCacheInvalidation.pin.test.ts | 12 + 4 files changed, 577 insertions(+), 8 deletions(-) create mode 100644 .changeset/delete-view-removes-every-home-4479.md create mode 100644 packages/data-objectstack/src/deleteView.homes.test.ts diff --git a/.changeset/delete-view-removes-every-home-4479.md b/.changeset/delete-view-removes-every-home-4479.md new file mode 100644 index 000000000..761eca3f1 --- /dev/null +++ b/.changeset/delete-view-removes-every-home-4479.md @@ -0,0 +1,23 @@ +--- +'@object-ui/data-objectstack': minor +--- + +`deleteView` removes every home the view has — deleting a draft-only saved view no longer silently no-ops + +A view has two possible homes: the pending per-item **draft** (`DELETE /api/v1/meta/view/:name?state=draft`) and the **published** overlay (`DELETE /api/v1/meta/view/:name`). `deleteView` addressed only the second, unqualified. Deleting a view that existed only as a draft therefore fired the delete at the published overlay, the server answered `200 {"success":true,"reset":false,"message":"No view '…' found — nothing to delete."}`, the draft survived untouched, and the tab was still there after a reload — while the receipt reported `{ deleted: false }` and nothing surfaced the refusal to the user. + +That is not a corner case. ADR-0034's `persistRuntimeMetadata` (app-shell) stages **every** runtime edit as a draft, and a view created from the `+` tab lives ONLY as a draft until an explicit Publish — so both "a view you just made" and "a published view you have since edited" are routinely draft-carrying. + +**Why this is not the mechanical mirror of #4139.** `updateView` probes the draft first and writes back to whichever home the read resolved; that is right for an update in all cases. Copying it here would have been wrong in one: on a published+draft pair a draft-first-*only* delete discards the draft and leaves the published row still serving the view. That is not Delete view, it is **Discard draft** — a deliberately different operation that already exists (`discardRuntimeDraft`, documented as "the published overlay is untouched"). The asymmetry has a clean statement: for an update, one home is the right home; for a delete, "remove this view" is satisfied only when *no home is left serving it*. + +So both homes are now deleted, **draft first**. The order is load-bearing on the failure path: a fault between the two calls leaves the published overlay intact, so the view is still served and the delete is cleanly retryable. The reverse order would strand a draft-only view — precisely the bug above. + +**Two blind calls, no probe.** Measured against the framework's `deleteMetaItem`: a missing home is reported as a **200** carrying `reset:false` (`"No pending draft for view/x."` / `"No view 'x' found — nothing to delete."`), never a 404. There is nothing for a probe to protect against, and `updateView`'s probe exists for a different reason — its read must resolve the row the merge writes back to — which has no counterpart for a delete. + +**One transport, one error contract.** Both halves now go through `MetadataClient.reset()`, the transport that can express the `?state=` qualifier and the one `updateView`'s draft half already uses. The published half previously went through `client.meta.deleteItem`; measured, that issues the byte-identical request (this adapter configures no environment scoping), so routing it here changes no addressing and collapses two error shapes into one `MetadataError`. + +The receipt is widened **additively**: `{ deleted }` gains optional `draft` and `published` outcomes (`removed`, plus the server's `reset` / `message`). `deleted` is true only when no home is left serving the view *and* at least one actually held a row — a view that existed in neither home still answers `false`, unchanged. A failure of the published half after the draft was discarded now throws (matching `updateView`'s convention of surfacing a fault rather than degrading) carrying the partial state on the error's `outcome`: "draft gone, overlay left" is exactly what the old `{ deleted: boolean }` could not express, and it is never rounded up to `true`. + +Cache invalidation moves into a `finally`, so `invalidateViewKeys` fires exactly once per call on **every** outcome including the throw. After a half-failure the draft row really is gone, and objectui#4363's asymmetry decides it: an unnecessary invalidation costs one refetch, a missed one costs the cache's full 5-minute TTL of stale overrides. + +Minor rather than patch: this moves published behavior for existing callers and adds two exported types, the same grading objectui#4271's `get()` unwrap and objectui#4495's `find()` resolve→reject took. The `.d.ts` diff is additive only — `deleteView`'s return widens from an inline `{ deleted: boolean }` to the new `DeleteViewResult`, which still carries `deleted: boolean` — so no consumer needs a code edit to keep compiling. A repo-wide census found one call site (app-shell's `ObjectView` delete handler), which awaits the call and does not read the receipt. diff --git a/packages/data-objectstack/src/deleteView.homes.test.ts b/packages/data-objectstack/src/deleteView.homes.test.ts new file mode 100644 index 000000000..6885a8e0c --- /dev/null +++ b/packages/data-objectstack/src/deleteView.homes.test.ts @@ -0,0 +1,389 @@ +/** + * 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'; +import { MetadataClient } from './metadata-client'; + +/** + * `deleteView` removes every home the view has (#4479). + * + * A view has two possible homes — the pending per-item DRAFT + * (`DELETE /meta/view/:name?state=draft`) and the PUBLISHED overlay + * (`DELETE /meta/view/:name`) — and `deleteView` used to address only the + * published one. The three cases, and what each used to do: + * + * | case | before #4479 | + * |--------------------|-----------------------------------------------------| + * | draft-only view | BUG — hits the published overlay, `reset:false`, the | + * | | draft survives and the tab is back after reload | + * | published-only | correct — `reset:true`, tab gone | + * | published + draft | ACCIDENTALLY correct — the published row goes, so | + * | | the tab goes; the orphan draft is left behind | + * + * The third row is why the mirror of #4139 is NOT mechanical. + * `persistRuntimeMetadata` (app-shell) stages **every** runtime edit as a + * draft, so "publish a view, then edit it" routinely produces a pair — and a + * naive draft-FIRST-only mirror would have discarded just the draft there, + * silently downgrading **Delete view** into **Discard draft**, which is a + * different operation that already exists (`discardRuntimeDraft`). + * + * The ruling on #4479: delete BOTH homes, draft first. Draft first because a + * failure between the two calls then leaves the PUBLISHED overlay intact and + * the operation cleanly retryable — the reverse order strands a draft-only + * view, which is this card's original bug shape. + * + * ## The wire these pins model + * + * Every receipt below is the framework's own, read verbatim from + * `packages/metadata-protocol/src/protocol.ts` (`deleteMetaItem`) — the + * no-row branch and the delete-ful branch. Both no-row answers are a **200** + * carrying `reset:false`, NOT a 404: + * + * - no draft pending -> `{success:true, reset:false, message:"No pending draft for view/x."}` + * - no published row -> `{success:true, reset:false, message:"No view 'x' found — nothing to delete."}` + * + * That measurement is what makes a blind two-call implementation legitimate + * here: neither "missing home" answer is an error, so there is nothing for a + * probe to protect against and the probe `updateView` needs (its read must + * resolve the row it is about to merge onto) has no counterpart for a delete. + * + * Nothing here mocks `deleteView`. The adapter is real and drives a + * wire-shaped `fetch`; the SDK stand-in on `ds.client.meta.deleteItem` + * mirrors the real client's transport (same URL, same `unwrapResponse`) so + * that the pins address the REQUEST, not the transport that issued it. + */ + +const VIEW = 'account.my_pipeline'; + +/** Framework receipt: a draft row existed and was discarded. */ +const DRAFT_DISCARDED = { + success: true, + reset: true, + seq: 41, + message: `Draft discarded — view/${VIEW}. [seq=41]`, +}; +/** Framework receipt: nothing pending. 200, not 404. */ +const NO_DRAFT = { + success: true, + reset: false, + message: `No pending draft for view/${VIEW}.`, +}; +/** Framework receipt: a runtime-only published row existed and is gone. */ +const PUBLISHED_DELETED = { + success: true, + reset: true, + seq: 42, + message: `Deleted view '${VIEW}' — it no longer exists. [seq=42]`, +}; +/** Framework receipt: no published row. 200, not 404 — the answer #4479 quoted. */ +const NO_PUBLISHED = { + success: true, + reset: false, + message: `No view '${VIEW}' found — nothing to delete.`, +}; + +interface Call { + /** `'draft'` when the request carried `?state=draft`, else `'published'`. */ + home: 'draft' | 'published'; + method: string; + url: string; +} + +interface Harness { + ds: any; + /** Every DELETE against `/meta/view/...`, in issue order. */ + deletes: Call[]; + /** Just the home of each DELETE, in issue order — the ordering pin. */ + order: () => Array<'draft' | 'published'>; + /** Keys passed to `metadataCache.invalidate`, in order. */ + invalidated: string[]; + baseUrl: string; + fetchImpl: any; +} + +/** + * Adapter whose two delete addresses answer independently. + * + * @param opts.draft receipt (or Error/status) for `DELETE ...?state=draft` + * @param opts.published receipt (or Error/status) for `DELETE ...` (active) + */ +function makeDS(opts: { + draft?: unknown | { failStatus: number }; + published?: unknown | { failStatus: number }; +} = {}): Harness { + const deletes: Call[] = []; + const invalidated: string[] = []; + const baseUrl = 'http://test.local'; + + const json = (body: unknown, status = 200) => + new Response(JSON.stringify(body), { + status, + headers: { 'Content-Type': 'application/json' }, + }); + + const answer = (spec: unknown) => { + if (spec && typeof spec === 'object' && 'failStatus' in (spec as any)) { + const status = (spec as any).failStatus as number; + return json({ error: { message: 'boom', code: 'server_error' } }, status); + } + return json(spec); + }; + + const fetchImpl = vi.fn(async (input: RequestInfo | URL, init?: RequestInit) => { + const url = String(input); + const method = init?.method ?? 'GET'; + if (url.includes('/meta/view/')) { + if (method === 'DELETE') { + const home = url.includes('state=draft') ? 'draft' : 'published'; + deletes.push({ home, method, url }); + return answer(home === 'draft' ? opts.draft ?? NO_DRAFT : opts.published ?? NO_PUBLISHED); + } + return json({ error: 'not found' }, 404); + } + return json({ success: true, data: { capabilities: {}, routes: {} } }); + }); + + const ds: any = new ObjectStackAdapter({ baseUrl, fetch: fetchImpl }); + ds.connected = true; + ds.connectionState = 'connected'; + + ds.metadataCache = { + get: async (_key: string, loader: () => Promise) => loader(), + invalidate: (key: string) => { + invalidated.push(key); + }, + getCachedSync: () => undefined, + getStats: () => ({}), + }; + + // SDK transport stand-in. NOT a stub of the behaviour under test: it issues + // the same real request the shipped `client.meta.deleteItem` issues + // (`${baseUrl}/api/v1/meta/:type/:name`, DELETE) and unwraps the + // `{success, data}` envelope exactly as `unwrapResponse` does, so the + // published half is observed at the WIRE whichever transport carries it. + ds.client = { + meta: { + deleteItem: async (type: string, name: string) => { + const res = await fetchImpl( + `${baseUrl}/api/v1/meta/${encodeURIComponent(type)}/${encodeURIComponent(name)}`, + { method: 'DELETE' }, + ); + const body: any = await res.json(); + if (!res.ok) { + throw Object.assign(new Error(body?.error?.message ?? 'request failed'), { + httpStatus: res.status, + }); + } + if (body && typeof body.success === 'boolean' && 'data' in body) return body.data; + return body; + }, + getItem: vi.fn(async () => { + throw Object.assign(new Error('Metadata item not found'), { httpStatus: 404 }); + }), + saveItem: vi.fn(async () => ({ success: true })), + getItems: vi.fn(async () => ({ items: [] })), + }, + }; + + return { + ds, + deletes, + order: () => deletes.map((c) => c.home), + invalidated, + baseUrl, + fetchImpl, + }; +} + +describe('deleteView removes every home the view has (#4479)', () => { + // ── Row 1: draft-only — the reported bug ──────────────────────────────── + describe('draft-only view (the #4479 bug row)', () => { + it('deletes the draft home, so the view is actually gone', async () => { + const h = makeDS({ draft: DRAFT_DISCARDED, published: NO_PUBLISHED }); + + const receipt = await h.ds.deleteView('account', VIEW); + + // The user-visible symptom first: before #4479 the only request issued + // was the unqualified one, the server answered `reset:false` / + // "nothing to delete", and this receipt came back `{deleted:false}` + // with the draft untouched and the tab back after reload. + expect(receipt.deleted).toBe(true); + // ...because the draft home was never addressed. + expect(h.order()).toContain('draft'); + }); + + it('reports the draft home as the one that had the row', async () => { + const h = makeDS({ draft: DRAFT_DISCARDED, published: NO_PUBLISHED }); + + const receipt = await h.ds.deleteView('account', VIEW); + + expect(receipt.draft?.removed).toBe(true); + expect(receipt.published?.removed).toBe(false); + }); + }); + + // ── Row 2: published-only — correct before, must stay correct ─────────── + describe('published-only view (correct before #4479)', () => { + it('still deletes the published home and still reports deleted', async () => { + const h = makeDS({ draft: NO_DRAFT, published: PUBLISHED_DELETED }); + + const receipt = await h.ds.deleteView('account', VIEW); + + expect(h.order()).toContain('published'); + // Unchanged from before #4479 — this half was never broken. + expect(receipt.deleted).toBe(true); + }); + + it('addresses both homes even when only one answers with a row', async () => { + const h = makeDS({ draft: NO_DRAFT, published: PUBLISHED_DELETED }); + + const receipt = await h.ds.deleteView('account', VIEW); + + expect(h.order()).toEqual(['draft', 'published']); + expect(receipt.draft?.removed).toBe(false); + expect(receipt.published?.removed).toBe(true); + }); + }); + + // ── Row 3: the pair — accidentally correct before; MUST NOT REGRESS ───── + describe('published + pending draft pair (must-not-change + the anti-mirror guard)', () => { + it('MUST NOT CHANGE: the published home is still deleted, so the tab still goes', async () => { + const h = makeDS({ draft: DRAFT_DISCARDED, published: PUBLISHED_DELETED }); + + const receipt = await h.ds.deleteView('account', VIEW); + + // This is the assertion a naive draft-first mirror of #4139 would have + // broken: it would have discarded the draft and returned, leaving the + // published row serving the view and the tab present after reload. + expect(h.order()).toContain('published'); + expect(receipt.deleted).toBe(true); + }); + + it('also discards the orphan draft, so a republish cannot resurrect the view', async () => { + const h = makeDS({ draft: DRAFT_DISCARDED, published: PUBLISHED_DELETED }); + + const receipt = await h.ds.deleteView('account', VIEW); + + expect(h.order()).toEqual(['draft', 'published']); + expect(receipt.draft?.removed).toBe(true); + expect(receipt.published?.removed).toBe(true); + }); + + it('invalidates both view cache keys exactly once (#4363 rule, unchanged)', async () => { + const h = makeDS({ draft: DRAFT_DISCARDED, published: PUBLISHED_DELETED }); + + await h.ds.deleteView('account', VIEW); + + // Two homes deleted, still ONE ordered pair: the #4363 rule is per + // WRITE METHOD, not per request. + expect(h.invalidated).toEqual([`view:account:${VIEW}`, 'view-overrides:account']); + }); + }); + + // ── Row 4: neither home — the control ─────────────────────────────────── + it('MUST NOT CHANGE: a view with no home at all reports deleted:false', async () => { + const h = makeDS({ draft: NO_DRAFT, published: NO_PUBLISHED }); + + const receipt = await h.ds.deleteView('account', VIEW); + + // Both homes answer 200 `reset:false`. Nothing was removed and the + // receipt has never claimed otherwise — that answer is preserved. + expect(receipt.deleted).toBe(false); + }); + + // ── Ordering ──────────────────────────────────────────────────────────── + it('addresses the draft home strictly before the published one', async () => { + const h = makeDS({ draft: DRAFT_DISCARDED, published: PUBLISHED_DELETED }); + + await h.ds.deleteView('account', VIEW); + + expect(h.order()).toEqual(['draft', 'published']); + expect(h.deletes[0].url).toContain('state=draft'); + expect(h.deletes[1].url).not.toContain('state=draft'); + }); + + // ── One transport, one error contract ─────────────────────────────────── + it('sends both deletes to the same `/api/v1/meta/view/:name` route', async () => { + const h = makeDS({ draft: DRAFT_DISCARDED, published: PUBLISHED_DELETED }); + + await h.ds.deleteView('account', VIEW); + + const base = `${h.baseUrl}/api/v1/meta/view/${encodeURIComponent(VIEW)}`; + expect(h.deletes[0].url).toBe(`${base}?state=draft`); + expect(h.deletes[1].url).toBe(base); + }); + + // ── Failure paths — updateView's convention: throw, never degrade ─────── + describe('partial failure', () => { + it('a failing draft delete throws and leaves the published home untouched', async () => { + const h = makeDS({ draft: { failStatus: 500 }, published: PUBLISHED_DELETED }); + + await expect(h.ds.deleteView('account', VIEW)).rejects.toThrow(); + + // The whole reason the ruling put the draft FIRST: a mid-operation + // failure must leave the published overlay serving the view, so the + // user retries a delete rather than being left with the draft-only + // shape that is this card's original bug. + expect(h.order()).toEqual(['draft']); + }); + + it('a failing published delete throws rather than reporting deleted:true', async () => { + const h = makeDS({ draft: DRAFT_DISCARDED, published: { failStatus: 500 } }); + + await expect(h.ds.deleteView('account', VIEW)).rejects.toThrow(); + + expect(h.order()).toEqual(['draft', 'published']); + }); + + it('the published-half failure carries the partial outcome, never rounds it to true', async () => { + const h = makeDS({ draft: DRAFT_DISCARDED, published: { failStatus: 500 } }); + + const err: any = await h.ds.deleteView('account', VIEW).catch((e: any) => e); + + // "draft gone, overlay left" is exactly what the old + // `{ deleted: boolean }` could not express. + expect(err.outcome?.deleted).toBe(false); + expect(err.outcome?.draft?.removed).toBe(true); + expect(err.outcome?.published?.removed).toBe(false); + expect(err.cause).toBeDefined(); + }); + + it('still invalidates the view cache keys when a half fails', async () => { + const h = makeDS({ draft: DRAFT_DISCARDED, published: { failStatus: 500 } }); + + await h.ds.deleteView('account', VIEW).catch(() => undefined); + + // The draft row IS gone, so a cache still holding it is stale for the + // full TTL. #4363's asymmetry decides it: an unnecessary invalidation + // costs one refetch, a missed one costs five minutes of stale overrides. + expect(h.invalidated).toEqual([`view:account:${VIEW}`, 'view-overrides:account']); + }); + }); + + // ── discardRuntimeDraft stays a DIFFERENT operation ───────────────────── + it('stays distinct from Discard draft, which touches only the draft home', async () => { + // `discardRuntimeDraft` (app-shell `runtime-metadata-persistence.ts`) is + // documented as "the published overlay is untouched" and is exactly this + // one call. Driven here through the same primitive rather than imported, + // because app-shell is downstream of this package — the point is that the + // two operations issue DIFFERENT request sets against the same wire. + const h = makeDS({ draft: DRAFT_DISCARDED, published: PUBLISHED_DELETED }); + + await new MetadataClient({ baseUrl: h.baseUrl, fetch: h.fetchImpl }).reset('view', VIEW, { + state: 'draft', + }); + expect(h.order()).toEqual(['draft']); + + // Same wire, same view: Delete view addresses BOTH homes. If these two + // ever issue the same request set, one of the operations has been lost. + const fresh = makeDS({ draft: DRAFT_DISCARDED, published: PUBLISHED_DELETED }); + await fresh.ds.deleteView('account', VIEW); + expect(fresh.order()).toEqual(['draft', 'published']); + }); +}); diff --git a/packages/data-objectstack/src/index.ts b/packages/data-objectstack/src/index.ts index 45eaee6b1..0681d3b55 100644 --- a/packages/data-objectstack/src/index.ts +++ b/packages/data-objectstack/src/index.ts @@ -1053,6 +1053,61 @@ function unwrapViewDraft(resp: unknown): Record | null { return Object.keys(spec).length > 0 ? (spec as Record) : null; } +/** + * Outcome of {@link ObjectStackAdapter.deleteView}'s delete against ONE of a + * view's two homes — the pending draft, or the published overlay (#4479). + */ +export interface ViewHomeDeleteOutcome { + /** + * True when this home held a row and the server removed it. False is the + * "there was nothing here" answer, which is a success, not a failure: the + * framework reports a missing home as a 200 carrying `reset:false`. + */ + removed: boolean; + /** The server receipt's `reset` flag, when it sent one. */ + reset?: boolean; + /** The server receipt's human-readable message, when it sent one. */ + message?: string; +} + +/** + * Receipt for {@link ObjectStackAdapter.deleteView} (#4479). + * + * The per-home fields are ADDITIVE over the original `{ deleted: boolean }`: + * a caller that only reads `deleted` is unaffected, and one that needs to tell + * "draft gone, overlay left" from "both gone" now can. + */ +export interface DeleteViewResult { + /** + * True only when no home is left serving the view AND at least one home + * actually held a row. A view that existed in neither home answers `false` + * — the same answer that shape has always given. + */ + deleted: boolean; + /** Outcome against the pending draft (`?state=draft`). */ + draft?: ViewHomeDeleteOutcome; + /** Outcome against the published overlay. */ + published?: ViewHomeDeleteOutcome; +} + +/** + * Read one delete receipt into a {@link ViewHomeDeleteOutcome}. + * + * The `deleted ?? reset ?? true` ladder is carried over verbatim from the + * single-call `deleteView` this replaced, so a server that answers a shape + * with neither key is read exactly as it was before (#4479): the framework + * sends `reset`, the SDK's typed metadata shape names `deleted`, and the + * final `true` is the "it answered 2xx and said nothing" default. + */ +function readViewDeleteReceipt(result: unknown): ViewHomeDeleteOutcome { + const r = (result ?? undefined) as Record | undefined; + return { + removed: !!(r?.deleted ?? r?.reset ?? true), + ...(typeof r?.reset === 'boolean' ? { reset: r.reset } : {}), + ...(typeof r?.message === 'string' ? { message: r.message } : {}), + }; +} + /** * Merge a partial view patch onto the CURRENT view document. * @@ -3457,22 +3512,112 @@ export class ObjectStackAdapter implements DataSource { } /** - * Delete an overlay view (reset to artifact default if one exists, or - * remove entirely if it was a user-created view). Routes to - * `DELETE /api/v1/meta/view/:name`. + * Delete an overlay view — from **every home it has**. + * + * A view has two possible homes, the same two {@link updateView} addresses: + * the pending per-item **draft** (`DELETE /meta/view/:name?state=draft`) and + * the **published** overlay (`DELETE /meta/view/:name`). This method used to + * issue only the second, and the three cases came out like this (#4479): + * + * | case | before | + * |--------------------|-----------------------------------------------------| + * | draft-only view | BUG — the published delete answered `reset:false` / | + * | | "nothing to delete", the draft survived, and the tab | + * | | was still there after reload | + * | published-only | correct — `reset:true`, tab gone | + * | published + draft | ACCIDENTALLY correct — the published row went, so | + * | | the tab went; the orphan draft stayed behind | + * + * **Why this is not the mechanical mirror of #4139.** `updateView` probes + * the draft first and writes back to whichever home the read resolved, and + * that is right for an update in all three cases. Copying it here would be + * wrong in the third: `persistRuntimeMetadata` (app-shell) stages EVERY + * runtime edit as a draft, so "publish a view, then edit it" routinely + * produces a pair — and a draft-first-only delete on a pair discards the + * draft and leaves the published row serving the view. That is not Delete + * view, it is **Discard draft**, a deliberately different operation that + * already exists (`discardRuntimeDraft`, documented as "the published + * overlay is untouched"). The clean statement of the asymmetry: for an + * update, one home is the right home; for a delete, "remove this view" is + * satisfied only when NO home is left serving it. + * + * So both homes are deleted, **draft first**. The order is load-bearing on + * the failure path: a fault between the two calls leaves the PUBLISHED + * overlay intact, so the view is still served and the operation is cleanly + * retryable. The reverse order would strand a draft-only view — which is + * precisely the bug shape above. + * + * **Two blind calls, no probe.** The framework's `deleteMetaItem` answers a + * missing home with a **200** carrying `reset:false` (`"No pending draft + * for view/x."` / `"No view 'x' found — nothing to delete."`), never a 404, + * so there is nothing for a probe to protect against. `updateView` needs its + * probe for a different reason — its read must resolve the row the merge + * writes back to — and that reason has no counterpart for a delete. + * + * **One transport, one error contract.** Both halves go through + * {@link MetadataClient} (`reset`), the transport that can express the + * `?state=` qualifier and the one `updateView`'s draft half already uses. + * The published half used to go through `client.meta.deleteItem`; measured, + * that issues the byte-identical request (`DELETE + * {baseUrl}/api/v1/meta/view/:name`, no environment scoping is configured on + * this adapter), so routing it here costs no addressing change and buys a + * single `MetadataError` shape across both calls instead of two. + * + * @returns `deleted` is true only when no home is left serving the view AND + * at least one actually held a row. A view that existed in neither home + * still answers `false`, unchanged. The per-home outcomes are additive: + * a partial result is observable rather than rounded up to `true`. + * @throws when either delete fails, matching {@link updateView}'s + * convention of surfacing the fault rather than degrading. A failure of + * the PUBLISHED half after the draft was discarded carries the partial + * state on the error's `outcome` — "draft gone, overlay left" is exactly + * what the old `{ deleted: boolean }` could not express. * * Invalidates through {@link invalidateViewKeys}: the deleted row leaves the * batch override map too, and a ghost entry there is what the object page - * would keep applying (objectui#4363). + * would keep applying (objectui#4363). Fired in a `finally`, so it happens + * once per call on EVERY outcome including the throw — after a half-failure + * the draft row really is gone, and #4363's asymmetry decides it: an + * unnecessary invalidation costs one refetch, a missed one costs the cache's + * full 5-minute TTL of stale overrides. */ async deleteView( objectName: string, viewName: string, - ): Promise<{ deleted: boolean }> { + ): Promise { await this.connect(); - const result: any = await this.client.meta.deleteItem('view', viewName); - this.invalidateViewKeys(objectName, viewName); - return { deleted: !!(result?.deleted ?? result?.reset ?? true) }; + const metaClient = this.metadataClient(); + try { + // ── Draft home, first ──────────────────────────────────────────────── + const draft = readViewDeleteReceipt( + await metaClient.reset('view', viewName, { state: 'draft' }), + ); + + // ── Published home ─────────────────────────────────────────────────── + let published: ViewHomeDeleteOutcome; + try { + published = readViewDeleteReceipt(await metaClient.reset('view', viewName)); + } catch (err) { + const outcome: DeleteViewResult = { + deleted: false, + draft, + published: { removed: false }, + }; + throw Object.assign( + new Error( + `deleteView: view "${viewName}" on object "${objectName}" is NOT fully removed` + + ` — the draft home was ${draft.removed ? 'discarded' : 'already absent'},` + + ' but deleting the published overlay failed. The published row is still' + + ' serving the view; retry the delete.', + ), + { cause: err, outcome }, + ); + } + + return { deleted: draft.removed || published.removed, draft, published }; + } finally { + this.invalidateViewKeys(objectName, viewName); + } } diff --git a/packages/data-objectstack/src/viewCacheInvalidation.pin.test.ts b/packages/data-objectstack/src/viewCacheInvalidation.pin.test.ts index 829c7916b..f912ba84d 100644 --- a/packages/data-objectstack/src/viewCacheInvalidation.pin.test.ts +++ b/packages/data-objectstack/src/viewCacheInvalidation.pin.test.ts @@ -87,6 +87,18 @@ function makeDS(opts: { }); if (url.includes('/meta/view/')) { if ((init?.method ?? 'GET') === 'PUT') return json({ success: true, version: 2 }); + // #4479 — `deleteView` addresses BOTH homes over this wire, so the + // harness has to answer DELETE. The framework never 404s a missing + // home: it reports one with a 200 carrying `reset:false`. Modelled + // faithfully (a home "has a row" exactly when this harness was told to + // serve one) so the pins below stay pins on INVALIDATION rather than + // becoming accidental assertions about the transport. + if ((init?.method ?? 'GET') === 'DELETE') { + const hasRow = url.includes('state=draft') + ? opts.draft != null + : opts.published != null && !(opts.published instanceof Error); + return json({ success: true, reset: hasRow }); + } if (url.includes('state=draft')) { if (opts.draft == null) return json({ error: 'not found' }, 404); return json({ type: 'view', name: opts.draft.name, item: opts.draft });