From d87310b4401594b7d1e63997a8aa43e318db5c70 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 28 Oct 2026 03:35:16 +0000 Subject: [PATCH] fix(app-shell): package-door permission save merges onto the pending draft The package door's save re-read the published body as its merge base, so a pending draft's non-authored keys (description, packageId, any key another door wrote) were replaced on the next Studio edit. Read the pending draft through readDraftForSave at the address the save writes and merge onto it when present; a rejected draft read refuses the save like the layered re-read does. Claude-Session: https://claude.ai/code/session_01JQwxba8E9EE7oBM24GbdNZ Co-Authored-By: Claude --- ...Editor.packageDoorDraftBase-26837.test.tsx | 218 ++++++++++++++++++ .../metadata-admin/PermissionMatrixEditor.tsx | 63 +++-- 2 files changed, 263 insertions(+), 18 deletions(-) create mode 100644 ui/packages/app-shell/src/views/metadata-admin/PermissionMatrixEditor.packageDoorDraftBase-26837.test.tsx diff --git a/ui/packages/app-shell/src/views/metadata-admin/PermissionMatrixEditor.packageDoorDraftBase-26837.test.tsx b/ui/packages/app-shell/src/views/metadata-admin/PermissionMatrixEditor.packageDoorDraftBase-26837.test.tsx new file mode 100644 index 00000000000..be79bdf52d0 --- /dev/null +++ b/ui/packages/app-shell/src/views/metadata-admin/PermissionMatrixEditor.packageDoorDraftBase-26837.test.tsx @@ -0,0 +1,218 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * The package door's save merges onto the PENDING DRAFT when one exists, not + * onto the published body (objectstack#26837). + * + * ## The defect these pins reach + * + * `doSave`'s package-door arm re-reads the set and merges this package's slice + * onto that read with `mergePermissionSlice`, which takes only the facets the + * editor can author from the edited body and everything else from the base. + * The base was `client.layered(...).effective` — the PUBLISHED body — while the + * save itself writes the package DRAFT. So a draft another door wrote (the + * card's reproduction: `PUT …?mode=draft&package=P` with `description` and + * `packageId`) lost those keys on the next Studio edit: the load displayed the + * draft, one checkbox autosaved, and the PUT carried the published values (or + * none) in their place. 200, no error. + * + * ## Why the pin drives the REAL editor + * + * `mergePermissionSlice` is correct for any base it is handed — its own pins + * (`permission-slice.authoredKeys.test.ts`, `permission-slice.facetDrift.test.ts`) + * call it directly and could never see this. The defect is the CHOICE of base, + * which only `doSave` makes, so these pins run the real page, the real load and + * the real autosave (objectui#11787: the package door has no Save button). + * + * ## The arms, and what each one's absence is read against + * + * 1. A pending draft whose non-authored keys differ from the published body: + * every one of them is on the wire byte-for-byte, out-of-scope rows come + * from the draft, and the edited row lands. + * 2. No pending draft: the base is still the published `effective`, so the + * published body's own keys and other packages' rows are on the wire — + * the control that the draft arm is not "the edited body, wholesale". + * 3. The save-time DRAFT read rejects: the save is refused on the same + * channel as objectui#9420's layered refusal, `client.save` is never + * called, and the control `getDraftCalls === 2` proves the read ran. + */ + +import '@testing-library/jest-dom/vitest'; +import { afterEach, describe, expect, it, vi } from 'vitest'; +import { cleanup, fireEvent, render, screen, waitFor, within } from '@testing-library/react'; +import { MemoryRouter } from 'react-router-dom'; + +/** + * The published record. `a_account` is the door's own package; `b_order` and + * `b_order.total` are another package's grants. + */ +const PUBLISHED = { + name: 'sales_perms', + label: 'Sales', + description: 'Published description', + objects: { + a_account: { allowRead: true, allowCreate: true }, + b_order: { allowRead: true }, + }, + fields: { 'b_order.total': { readable: true, editable: false } }, +}; + +/** + * The pending package draft another door wrote — every key the editor cannot + * author differs from {@link PUBLISHED}, and so does an out-of-scope row, so a + * value taken from the wrong base cannot pass for the right one. + */ +const DRAFT = { + name: 'sales_perms', + label: 'Sales', + description: 'Set through the API', + packageId: 'app.a', + objects: { + a_account: { allowRead: true, allowCreate: true }, + b_order: { allowRead: true, allowEdit: true, viewAllRecords: true }, + }, + fields: { 'b_order.total': { readable: true, editable: true } }, +}; + +/** The facets `EDITOR_AUTHORED_KEYS` does not name — the ones the base supplies. */ +const NON_AUTHORED = ['description', 'packageId'] as const; + +/** Distinctive slice of the refusal the author sees on the page's error strip. */ +const REFUSAL = /could not be re-read/; + +interface Server { + saved: Array>; + savedOpts: Array | undefined>; + /** `getDraft` calls seen so far — #1 is the load, #2 is `doSave`'s re-read. */ + getDraftCalls: number; +} + +function freshServer(): Server { + return { saved: [], savedOpts: [], getDraftCalls: 0 }; +} + +function makeClient(server: Server, opts: { draft: Record | null; rejectSaveTimeDraft?: boolean }) { + return { + layered: async () => ({ effective: PUBLISHED, code: null, overlay: null, overlayScope: null }), + getDraft: async () => { + server.getDraftCalls += 1; + if (opts.rejectSaveTimeDraft && server.getDraftCalls > 1) throw new Error('draft read failed'); + return opts.draft ? { type: 'permission', name: 'sales_perms', item: opts.draft, version: 'v1' } : null; + }, + // The package declares `a_account` only. + list: async (type: string) => (type === 'object' ? [{ item: { name: 'a_account' } }] : []), + get: async () => null, + save: async ( + _type: string, + _name: string, + payload: Record, + saveOpts?: Record, + ) => { + server.saved.push(payload); + server.savedOpts.push(saveOpts); + return payload; + }, + } as any; +} + +let clientImpl: any; + +vi.mock('./useMetadata', () => ({ + useMetadataClient: () => clientImpl, + useMetadataTypes: () => ({ + loading: false, + error: null, + entries: [{ type: 'permission', label: 'Permission', allowEnvironmentOverlay: true }], + }), +})); + +vi.mock('./AssignedUsersSection', () => ({ AssignedUsersSection: () => null })); + +import { PermissionMatrixEditPage } from './PermissionMatrixEditor'; + +afterEach(cleanup); + +function renderPackageDoor() { + return render( + + + , + ); +} + +/** One ordinary edit: revoke every grant on the door's own `a_account` row. */ +function editAccountRow() { + const row = screen.getByText('a_account').closest('tr')!; + fireEvent.click(within(row).getByRole('button', { name: 'None' })); +} + +/** The package door autosaves after the shared pause (1.5s). */ +const AUTOSAVE = { timeout: 4000 }; + +describe('PermissionMatrixEditPage — the PACKAGE door saves onto the pending draft (objectstack#26837)', () => { + it('keeps every key the editor does not author from the pending draft, byte-for-byte', async () => { + const server = freshServer(); + clientImpl = makeClient(server, { draft: DRAFT }); + renderPackageDoor(); + await screen.findByText('a_account'); + + editAccountRow(); + + await waitFor(() => expect(server.saved).toHaveLength(1), AUTOSAVE); + const body = server.saved[0] as any; + + // The edited row lands… + expect(body.objects.a_account).toEqual({}); + // …and the draft's non-authored keys survive, not the published ones. + for (const key of NON_AUTHORED) expect(body[key]).toEqual(DRAFT[key]); + // Out-of-scope rows come from the draft too: it is what this save replaces. + expect(body.objects.b_order).toEqual(DRAFT.objects.b_order); + expect(body.fields).toEqual(DRAFT.fields); + // CONTROL: the save-time draft read ran (#1 is the load). + expect(server.getDraftCalls).toBe(2); + expect(server.savedOpts[0]).toMatchObject({ mode: 'draft', packageId: 'app.a' }); + expect(screen.queryByText(REFUSAL)).toBeNull(); + }); + + it('CONTROL — with no pending draft the base is still the published body', async () => { + const server = freshServer(); + clientImpl = makeClient(server, { draft: null }); + renderPackageDoor(); + await screen.findByText('a_account'); + + editAccountRow(); + + await waitFor(() => expect(server.saved).toHaveLength(1), AUTOSAVE); + const body = server.saved[0] as any; + + expect(body.objects.a_account).toEqual({}); + expect(body.description).toBe(PUBLISHED.description); + expect('packageId' in body).toBe(false); + expect(body.objects.b_order).toEqual(PUBLISHED.objects.b_order); + expect(body.fields).toEqual(PUBLISHED.fields); + expect(screen.queryByText(REFUSAL)).toBeNull(); + }); + + it('REFUSES the save when the save-time draft read rejects, rather than fall back to the published body', async () => { + const server = freshServer(); + clientImpl = makeClient(server, { draft: DRAFT, rejectSaveTimeDraft: true }); + renderPackageDoor(); + await screen.findByText('a_account'); + + editAccountRow(); + + // Settle on either arm — the refusal on screen, or a PUT landing. + await waitFor( + () => expect(server.saved.length > 0 || screen.queryByText(REFUSAL) !== null).toBe(true), + AUTOSAVE, + ); + + // CONTROL: the save-time draft read really ran and really rejected. + expect(server.getDraftCalls).toBe(2); + expect(server.saved).toEqual([]); + expect(server.savedOpts).toEqual([]); + // The same error strip objectui#9420's layered refusal renders into. + expect(screen.getByText(REFUSAL)).toBeInTheDocument(); + expect(screen.queryByTestId('perm-autosaving')).toBeNull(); + }); +}); diff --git a/ui/packages/app-shell/src/views/metadata-admin/PermissionMatrixEditor.tsx b/ui/packages/app-shell/src/views/metadata-admin/PermissionMatrixEditor.tsx index 3fb2f78251f..ae1632ba59e 100644 --- a/ui/packages/app-shell/src/views/metadata-admin/PermissionMatrixEditor.tsx +++ b/ui/packages/app-shell/src/views/metadata-admin/PermissionMatrixEditor.tsx @@ -266,6 +266,19 @@ function isArtifactBackedLayer( ); } +/** + * The editable body of a pending package draft, as `readDraftForSave` served + * it (the `{ type, name, item }` envelope, or a bare body), or `null` for "no + * draft". Read decorations come off here, at the unwrap, so neither the load + * nor the save's merge base carries `_diagnostics` / `_draft` (objectui#8181). + */ +function pendingDraftBody( + answer: { item?: PermissionSetDraft } | PermissionSetDraft | null, +): PermissionSetDraft | null { + if (!answer) return null; + return stripReadDecorations((answer as { item?: PermissionSetDraft }).item ?? answer) as PermissionSetDraft; +} + /** Localized short label for an OWD value; falls back to the raw value. */ function owdLabel(t: (k: string) => string, value: string): string { const key: Record = { @@ -867,13 +880,10 @@ export function PermissionMatrixEditPage({ type, name, packageId, onDraftSaved, // below re-bases on a fresh RAW `layered` read, which drops them — but // its `.catch(() => null)` arm falls back to this very body and // spreads it into `client.save`, so a failed layered read used to put - // `_diagnostics` / `_draft` on the wire. Strip at the unwrap, which is - // the one place the served envelope becomes an editable draft. - const draftBody = pendingDraft - ? (stripReadDecorations( - (pendingDraft as any).item ?? pendingDraft, - ) as PermissionSetDraft) - : null; + // `_diagnostics` / `_draft` on the wire. Strip at the unwrap, + // `pendingDraftBody`, the one function through which a served envelope + // becomes an editable body — here and in `doSave`'s merge base. + const draftBody = pendingDraftBody(pendingDraft); // Draft wins over the published baseline for display (D6). const effective: PermissionSetDraft = (draftBody ?? lay?.effective ?? lay?.code ?? { name, objects: {} }) as PermissionSetDraft; @@ -1280,19 +1290,36 @@ export function PermissionMatrixEditPage({ type, name, packageId, onDraftSaved, // purpose: a set that exists only as a package draft (what the Studio // Access pillar's "+ New" creates) has no published rows for anyone to // lose, and refusing there would block its first save. + // + // objectstack#26837 — and this door's save WRITES the pending draft, so + // when one exists it is the base, not the published body: the facets + // the editor cannot author (`description`, `packageId`, any key another + // door wrote) come from the draft, exactly as the load displayed it. + // Read through the load's own door, at the address this save writes. + // The pin the load recorded is left alone, so a draft that moved since + // the load is still refused as a version conflict. `readDraftForSave` + // resolves a REJECTED read as no answer AND no pin, while a read that + // found no draft pins `If-None-Match: *` — so the pair tells a failure + // apart from "no draft", and a failure refuses exactly like the + // layered re-read: falling back to the published body would drop the + // draft's values with a 200. let rereadFailed = false; - const fresh = await client - .layered(type, payload.name) - // Kept on `.catch` so the rejection can never escape as an unhandled - // one; the refusal is raised below, inside this `try`'s own body, and - // reaches the author on the same error channel a failed - // `client.save` already uses. - .catch(() => { - rereadFailed = true; - return null; - }); + const [fresh, draftRead] = await Promise.all([ + client + .layered(type, payload.name) + // Kept on `.catch` so the rejection can never escape as an + // unhandled one; the refusal is raised below, inside this `try`'s + // own body, and reaches the author on the same error channel a + // failed `client.save` already uses. + .catch(() => { + rereadFailed = true; + return null; + }), + readDraftForSave<{ item?: PermissionSetDraft } | PermissionSetDraft>(client, type, payload.name, packageId), + ]); + if (draftRead.answer === null && draftRead.pin === null) rereadFailed = true; if (rereadFailed) throw new Error(t('perm.save.rereadFailed')); - const base = (fresh?.effective ?? payload) as PermissionSetDraft; + const base = (pendingDraftBody(draftRead.answer) ?? fresh?.effective ?? payload) as PermissionSetDraft; toSave = mergePermissionSlice(base, payload, scope); } // ADR-0086 P2 (D6/D7). Package door → the set is metadata: write a DRAFT