diff --git a/.changeset/resource-edit-reset-delete-verdict-4886.md b/.changeset/resource-edit-reset-delete-verdict-4886.md new file mode 100644 index 0000000000..dc3172d8bb --- /dev/null +++ b/.changeset/resource-edit-reset-delete-verdict-4886.md @@ -0,0 +1,37 @@ +--- +'@object-ui/app-shell': patch +--- + +The metadata editor's reset/delete button now renders the verb it executes, decided by the server's own verdict. + +One control, two independently hand-rolled artifact predicates. The render side +asked the page's two-tier `isArtifactItem` — which excludes the `sys_metadata` +save-path sentinel and ADR-0010 `provenance: 'org'` — while `doReset()` asked a +looser one of its own, `layered?.code != null`. For a published org-own entry +those disagree, because that entry's `code` layer IS its own rehydrated +`sys_metadata` row: the button drew a trash can titled "Delete", then asked +"Reset overlay for …?" and took the reset branch, leaving the operator on a page +for an entry the request had just destroyed. PR #4885 did not introduce this and +does not fix it, but it widened the population that can see the button. + +Both sides now read ONE value, and it is the one the server already computes and +ships on the layered envelope (`resolveLockState`: `resettable = artifactBacked`). +Icon, `title`, confirm text and the branch taken can no longer disagree about the +same entry. Two facts were measured before the change rather than assumed: for an +entry with no artifact baseline the `DELETE /meta/:type/:name` that this button +issues hard-deletes the `sys_metadata` row (there is nothing to reset *to*), and +for exactly that population the server answers `resettable: false` — so delete is +the honest verb, and the confirm dialog is the consent gate. This is the +maintainer's 2026-08-17 ruling on objectui#4886. + +Two consequences are behavior changes, stated because they are not merely fixes. +`resettable` is read as an honest tri-state: `undefined` means the server has no +opinion (a pre-ADR-0010 envelope), and instead of the old +`layered?.resettable !== false` collapse into "resettable" — which promised a +baseline that may not exist — the page falls back to its own conservative tier, +the same value the render side already used, so a legacy server keeps its legacy +rendering and both sides still move together. And the button's lock gate is now +`deletable` for both verbs: reset and delete are the same request, and the server +gates it once, through `evaluateLockForDelete`. A `_lock: 'no-delete'` artifact +item therefore stops offering a Reset button the server would answer with +`403 ITEM_LOCKED`. diff --git a/packages/app-shell/src/views/metadata-admin/ResourceEditPage.resetVerdict.test.tsx b/packages/app-shell/src/views/metadata-admin/ResourceEditPage.resetVerdict.test.tsx new file mode 100644 index 0000000000..79b18e6009 --- /dev/null +++ b/packages/app-shell/src/views/metadata-admin/ResourceEditPage.resetVerdict.test.tsx @@ -0,0 +1,317 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * The reset/delete control must render the verb it executes — objectui#4886. + * + * ## The defect + * + * One button, two independently hand-rolled artifact predicates. The render + * side asked the page's two-tier `isArtifactItem` (which excludes the + * `sys_metadata` sentinel and ADR-0010 `provenance: 'org'`); `doReset()` + * asked a looser one of its own, `layered?.code != null`. For a published + * org-own entry — whose `code` layer is its own rehydrated `sys_metadata` + * row — those disagree, so the button rendered `Trash2` with the title + * "Delete", then asked "Reset overlay for …?" and took the reset branch: + * refetch layered, stay on the page. A button that said delete, asked reset, + * and did reset. + * + * ## The fix this suite pins + * + * Both sides now read ONE value, and it is the server's: `resolveLockState` + * computes `resettable = artifactBacked` and ships it on the layered + * envelope. The page consumes that verdict instead of re-deriving it, and + * reads it as an honest tri-state — `layered?.resettable !== false` used to + * collapse "no opinion" into "true", promising a baseline that may not exist. + * + * Each case therefore asserts all THREE surfaces of the same entry together + * — icon + `title`, confirm text, and the branch actually taken. Asserting + * any one alone is what let the defect live: every individual surface was + * self-consistent, only the trio disagreed. + * + * ## Measured, not assumed (the two dispatch preconditions) + * + * 1. `client.reset()` is `DELETE /meta/:type/:name`. For an entry with no + * artifact baseline the server's `deleteMetaItem` routes to + * `SysMetadataRepository.delete`, which HARD-deletes the `sys_metadata` + * row, appends an `operation_type: 'delete'` tombstone, and retires the + * item's registry entry because no layer beneath it can serve the name. + * The row IS the entry, so the call destroys it — and the server says so + * itself: its receipt for this population reads `Deleted '' + * — it no longer exists.`, against `… reset to artifact default.` for the + * backed one. "Delete" is the honest verb, which is the maintainer's + * 2026-08-17 ruling on this card. + * 2. For that same population the server answers `resettable: false`: + * `resettable = artifactBacked = lookupArtifactItem(...) !== undefined`, + * and `SchemaRegistry.getArtifactItem` returns `undefined` for both the + * `sys_metadata` sentinel and `_provenance: 'org'`. The server is right; + * only these two client-side re-derivations were wrong. + */ + +import '@testing-library/jest-dom/vitest'; +import { describe, it, expect, vi, afterEach, beforeEach } from 'vitest'; +import { render, screen, cleanup, waitFor } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; +import { MemoryRouter, Route, Routes, useLocation } from 'react-router-dom'; + +/** Body of the entry under test, minus the provenance tags each case adds. */ +const objectDef = { + name: 'showcase_account', + label: 'Account', + fields: [{ name: 'title', label: 'Title', type: 'text' }], +}; + +const layeredImpl = { current: vi.fn() }; + +const mockClient = { + list: vi.fn(async () => []), + listDrafts: vi.fn(async () => []), + get: vi.fn(async () => null), + getDraft: vi.fn(async () => null), + references: vi.fn(async () => []), + layered: vi.fn(async (...args: unknown[]) => layeredImpl.current(...args)), + reset: vi.fn(async () => ({ success: true, reset: true })), +}; + +// Both tier flags are true so `canWrite` is satisfied on either side of the +// artifact/org split: this suite is about which VERB the control carries, not +// about the two-tier write gate (that one is pinned by +// `ResourceEditPage.artifactTier.test.tsx`). +vi.mock('./useMetadata', async (importOriginal) => { + const mod = await importOriginal(); + return { + ...mod, + useMetadataClient: () => mockClient, + useMetadataTypes: () => ({ + loading: false, + error: null, + entries: [ + { + type: 'object', + name: 'object', + label: 'Object', + allowOrgOverride: true, + allowRuntimeCreate: true, + schema: { + type: 'object', + properties: { name: { type: 'string' }, label: { type: 'string' } }, + }, + }, + ], + }), + }; +}); + +import { MetadataResourceEditPage } from './ResourceEditPage'; + +const START_PATH = '/metadata/object/showcase_account'; +/** Where the delete branch is supposed to land: the type's list view. */ +const LIST_PATH = '/metadata/object'; + +/** Renders the live pathname so "navigated back" is observed, not mocked. */ +function LocationProbe() { + const loc = useLocation(); + return {loc.pathname}; +} + +/** + * The editor mounted under the route it really lives on. A real `` + * matters here rather than a bare `MemoryRouter`: the delete branch navigates + * with `{ relative: 'path' }`, which resolves against the matched route — with + * no route to match, `../` lands on `/` and the assertion would be testing the + * harness instead of the page. + */ +function editorUnderRoute() { + return ( + + + + } + /> + list} /> + + + ); +} + +/** Captures what the confirm dialog actually asked, and answers yes. */ +const confirmMessages: string[] = []; + +beforeEach(() => { + confirmMessages.length = 0; + vi.stubGlobal('confirm', (msg?: string) => { + confirmMessages.push(String(msg ?? '')); + return true; + }); +}); + +afterEach(() => { + cleanup(); + vi.unstubAllGlobals(); + vi.clearAllMocks(); +}); + +/** + * Mount the editor over one layered envelope and return the control. + * Every fixture carries a non-null `overlay` — the button only renders for + * an entry that HAS a customization to reset or delete. + */ +async function mountWithEnvelope(layered: Record) { + layeredImpl.current = vi.fn(async () => layered); + render(editorUnderRoute()); + await waitFor(() => expect(mockClient.layered).toHaveBeenCalled()); + return await screen.findByTestId('reset-or-delete-button'); +} + +/** Current pathname, trailing slash normalised away (`navigate('../')`). */ +function pathname(): string { + const raw = screen.getByTestId('pathname').textContent ?? ''; + return raw.length > 1 ? raw.replace(/\/+$/, '') : raw; +} + +/** + * A published org-own entry as the server actually ships it: the `code` layer + * is the entry's own rehydrated `sys_metadata` row, so `code != null` — the + * exact shape the old `doReset()` predicate misread — while the server's + * verdict says there is no baseline behind it. + */ +const ORG_OWN_SENTINEL = { + code: { ...objectDef, _packageId: 'sys_metadata' }, + overlay: { ...objectDef, label: 'Account (customized)' }, + overlayScope: 'org', + effective: { ...objectDef, label: 'Account (customized)' }, + packageId: 'sys_metadata', + editable: true, + deletable: true, + resettable: false, +}; + +/** A genuine packaged artifact carrying an org overlay on top. */ +const PACKAGED_WITH_OVERLAY = { + code: { ...objectDef, _packageId: 'com.example.showcase', _provenance: 'package' }, + overlay: { ...objectDef, label: 'Account (customized)' }, + overlayScope: 'org', + effective: { ...objectDef, label: 'Account (customized)' }, + provenance: 'package', + packageId: 'com.example.showcase', + editable: true, + deletable: true, + resettable: true, +}; + +describe('ResourceEditPage — one server verdict drives icon, confirm and branch (#4886)', () => { + it('resettable:false — renders Delete, ASKS delete, and leaves the page', async () => { + // The card's population. `code != null` here, which is precisely why the + // old execute-side predicate said "artifact" and reset; the server says + // there is no baseline, so the DELETE this button issues destroys the + // entry and the current URL stops referring to anything. + const btn = await mountWithEnvelope(ORG_OWN_SENTINEL); + + expect(btn).toHaveAttribute('title', 'Delete'); + expect(screen.getByTestId('delete-icon')).toBeInTheDocument(); + expect(screen.queryByTestId('reset-icon')).toBeNull(); + + await userEvent.click(btn); + + await waitFor(() => expect(mockClient.reset).toHaveBeenCalledWith('object', 'showcase_account')); + expect(confirmMessages).toEqual([ + 'Delete object/showcase_account? This cannot be undone.', + ]); + await waitFor(() => expect(pathname()).toBe(LIST_PATH)); + }); + + it('resettable:true — renders Reset overlay, ASKS reset, and stays put', async () => { + // The other half of the same verdict: a baseline exists, so the overlay + // is peeled and the code default remains — the page must survive. + const btn = await mountWithEnvelope(PACKAGED_WITH_OVERLAY); + + expect(btn).toHaveAttribute('title', 'Reset overlay'); + expect(screen.getByTestId('reset-icon')).toBeInTheDocument(); + expect(screen.queryByTestId('delete-icon')).toBeNull(); + + await userEvent.click(btn); + + await waitFor(() => expect(mockClient.reset).toHaveBeenCalledWith('object', 'showcase_account')); + expect(confirmMessages).toEqual(['Reset overlay for object/showcase_account?']); + // The reset branch re-reads the layered envelope in place … + await waitFor(() => expect(mockClient.layered.mock.calls.length).toBeGreaterThan(1)); + // … and does NOT navigate away. + expect(pathname()).toBe(START_PATH); + }); + + it('no verdict + packaged code layer — falls back to the conservative reset reading', async () => { + // Pre-ADR-0010 server: the envelope carries no `resettable`. We do not + // invent one — the page falls back to its own artifact tier, which for a + // genuine packaged item is "reset", i.e. exactly the legacy rendering. + const { resettable: _omitted, ...noVerdict } = PACKAGED_WITH_OVERLAY; + const btn = await mountWithEnvelope(noVerdict); + + expect(btn).toHaveAttribute('title', 'Reset overlay'); + + await userEvent.click(btn); + + await waitFor(() => expect(mockClient.reset).toHaveBeenCalled()); + expect(confirmMessages).toEqual(['Reset overlay for object/showcase_account?']); + expect(pathname()).toBe(START_PATH); + }); + + it('no verdict + org-own code layer — "no opinion" is NOT collapsed into resettable', async () => { + // The tri-state that the old `layered?.resettable !== false` erased. With + // no verdict AND a sentinel code layer the conservative fallback is the + // page's own tier, which correctly reads this entry as org-own → delete. + // Under the old collapse this same entry rendered delete and executed + // reset; the point of the fallback is that both sides move together. + const { resettable: _omitted, ...noVerdict } = ORG_OWN_SENTINEL; + const btn = await mountWithEnvelope(noVerdict); + + expect(btn).toHaveAttribute('title', 'Delete'); + expect(screen.getByTestId('delete-icon')).toBeInTheDocument(); + + await userEvent.click(btn); + + await waitFor(() => expect(mockClient.reset).toHaveBeenCalled()); + expect(confirmMessages).toEqual([ + 'Delete object/showcase_account? This cannot be undone.', + ]); + await waitFor(() => expect(pathname()).toBe(LIST_PATH)); + }); + + it('server verdict outranks the client tier: provenance-org entry with resettable:true resets', async () => { + // The verdict is the authority, not a tie-breaker layered on top of the + // old heuristic. An entry the page's own tier would call org-own + // (`provenance: 'org'`) but the server says IS artifact-backed must + // render and execute reset — otherwise the client is still deciding. + const btn = await mountWithEnvelope({ + ...ORG_OWN_SENTINEL, + code: { ...objectDef, _packageId: 'com.example.base', _provenance: 'org' }, + provenance: 'org', + packageId: 'com.example.base', + resettable: true, + }); + + expect(btn).toHaveAttribute('title', 'Reset overlay'); + + await userEvent.click(btn); + + await waitFor(() => expect(mockClient.reset).toHaveBeenCalled()); + expect(confirmMessages).toEqual(['Reset overlay for object/showcase_account?']); + expect(pathname()).toBe(START_PATH); + }); + + it('deletable:false hides the control for BOTH verbs — the server gates one DELETE', async () => { + // Reset and delete are the same `DELETE /meta/:type/:name`, and the + // server gates it once (`assertLockAllowsDelete` → `evaluateLockForDelete`) + // regardless of artifact backing. Rendering a Reset button the server + // would answer 403 ITEM_LOCKED is the same "second copy of the contract" + // this card removes, so `deletable` gates both. + layeredImpl.current = vi.fn(async () => ({ + ...PACKAGED_WITH_OVERLAY, + lock: 'no-delete', + deletable: false, + })); + render(editorUnderRoute()); + await waitFor(() => expect(mockClient.layered).toHaveBeenCalled()); + expect(screen.queryByTestId('reset-or-delete-button')).toBeNull(); + }); +}); diff --git a/packages/app-shell/src/views/metadata-admin/ResourceEditPage.tsx b/packages/app-shell/src/views/metadata-admin/ResourceEditPage.tsx index 082cbe21af..9ed06f4708 100644 --- a/packages/app-shell/src/views/metadata-admin/ResourceEditPage.tsx +++ b/packages/app-shell/src/views/metadata-admin/ResourceEditPage.tsx @@ -996,6 +996,49 @@ function MetadataResourceEditPageImpl({ && (layered.code as { _packageId?: string } | null)?._packageId !== 'sys_metadata' && layered?.provenance !== 'org'; + // ── objectui#4886 — ONE verdict decides the reset/delete verb ────────── + // + // Which verb the destructive control carries is a server question, and the + // server already answers it: `resolveLockState` computes + // `resettable = artifactBacked` and ships it on the layered envelope. This + // page used to re-derive that answer TWICE, differently — the render side + // asked `isArtifactItem`, `doReset()` asked `layered?.code != null` — and + // the two disagree for a published org-own item, whose `code` layer is a + // rehydrated `sys_metadata` row: the button rendered `Trash2` / + // "Delete" and then executed the reset branch (refetch, stay on the page) + // after asking "Reset overlay for …?". A button that said delete, asked + // reset, and did reset. + // + // Honest tri-state. `resettable` is `boolean | undefined`: + // • `true` — a package baseline exists behind the overlay, so + // "Reset overlay" is the truthful verb: peel the overlay, + // the code default remains, stay on the page. + // • `false` — nothing to reset TO. The entry IS its `sys_metadata` + // row, so the DELETE the button issues removes it + // outright: `deleteMetaItem` hard-deletes the row, writes + // a `delete` tombstone, and retires the item's registry + // entry because no layer under it can serve the name. The + // server says so in its own receipt for exactly this + // population — `Deleted '' — it no longer + // exists.`, as against `… reset to artifact default.` for + // the backed one. "Delete" is the honest verb and the + // confirm dialog is the consent gate — the maintainer's + // 2026-08-17 ruling on this card. + // • `undefined` — the server has NO OPINION (pre-ADR-0010 envelope, or a + // transport that drops the flag). The old + // `layered?.resettable !== false` read collapsed this into + // `true`, i.e. it promised a baseline that may not exist. + // We do not guess in the server's name: fall back to this + // page's own conservative tier (`isArtifactItem`), which + // is exactly what the render side already used before this + // change — so a legacy server keeps its legacy rendering, + // and render + confirm + execute still read ONE value. + // + // `??` (not `||`) on purpose: `false` from the server is an ANSWER, and must + // not fall through to the client heuristic. + const resettableVerdict: boolean | undefined = layered?.resettable; + const isResetSemantic = !createMode && (resettableVerdict ?? isArtifactItem); + // Auto-enable design mode for designer-capable types. We do this once // per (type,name) navigation so the user lands in the productive // state instead of having to click "Edit". Truly read-only types @@ -1259,13 +1302,18 @@ function MetadataResourceEditPageImpl({ } async function doReset() { - // Two semantics: - // - artifact-backed item: "Reset overlay" — keep the code default. - // - DB-only item: "Delete" — the item disappears entirely (no - // artifact baseline to fall back to). Navigate back to the list - // since the current URL no longer refers to anything. - const itemIsArtifact = !createMode && layered?.code != null; - const confirmKey = itemIsArtifact + // Two semantics, ONE verdict — see `isResetSemantic` above. Both this + // handler and the button that triggers it read that single value, so the + // icon, the `title`, the confirm text and the branch taken here can no + // longer disagree about the same entry (objectui#4886). The wire call is + // the same `DELETE /meta/:type/:name` either way; what differs is what + // the server does with it and therefore what we must tell the user: + // - baseline present: the overlay row is dropped and the code default + // is what remains → "Reset overlay", refetch layered, stay put. + // - no baseline: the overlay row IS the entry, so the same DELETE + // destroys it → "Delete", and the current URL no longer refers to + // anything, so navigate back to the list. + const confirmKey = isResetSemantic ? 'engine.edit.resetConfirm' : 'engine.edit.deleteConfirm'; if (!confirm(tFormat(confirmKey, locale, { type, name: name ?? '' }))) { @@ -1275,7 +1323,7 @@ function MetadataResourceEditPageImpl({ setError(null); try { await client.reset(type, name); - if (itemIsArtifact) { + if (isResetSemantic) { const lay = await client.layered(type, name); setLayered(lay); const fresh = (lay.effective ?? lay.code ?? {}) as Record; @@ -1379,8 +1427,14 @@ function MetadataResourceEditPageImpl({ // ADR-0010 — server-computed lock flags. undefined means "no opinion" // (older server / non-lockable item) → preserve legacy behaviour. const lockEditable = layered?.editable !== false; + // `deletable` is the lock gate for BOTH verbs, not just the delete one: + // reset and delete are the same `DELETE /meta/:type/:name` call, and the + // server gates it once, through `evaluateLockForDelete` (`assertLockAllowsDelete` + // runs on every `deleteMetaItem`, artifact-backed or not). `resettable` is + // NOT a permission — it is `artifactBacked`, i.e. "is there a baseline to + // reset to", which is why it now drives the VERB (`isResetSemantic`) and no + // longer doubles as the button's lock gate (objectui#4886). const lockDeletable = layered?.deletable !== false; - const lockResettable = layered?.resettable !== false; const lockReason = layered?.lockReason; const isLocked = layered?.lock && layered.lock !== 'none'; const canWriteByType = createMode @@ -1700,23 +1754,24 @@ function MetadataResourceEditPageImpl({ )} )} - {!createMode && canWrite && layered?.overlay && (isArtifactItem ? lockResettable : lockDeletable) && ( + {!createMode && canWrite && layered?.overlay && lockDeletable && ( )}