Skip to content

Commit d87310b

Browse files
committed
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 <noreply@anthropic.com>
1 parent 3230bdb commit d87310b

2 files changed

Lines changed: 263 additions & 18 deletions

File tree

Lines changed: 218 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,218 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* The package door's save merges onto the PENDING DRAFT when one exists, not
5+
* onto the published body (objectstack#26837).
6+
*
7+
* ## The defect these pins reach
8+
*
9+
* `doSave`'s package-door arm re-reads the set and merges this package's slice
10+
* onto that read with `mergePermissionSlice`, which takes only the facets the
11+
* editor can author from the edited body and everything else from the base.
12+
* The base was `client.layered(...).effective` — the PUBLISHED body — while the
13+
* save itself writes the package DRAFT. So a draft another door wrote (the
14+
* card's reproduction: `PUT …?mode=draft&package=P` with `description` and
15+
* `packageId`) lost those keys on the next Studio edit: the load displayed the
16+
* draft, one checkbox autosaved, and the PUT carried the published values (or
17+
* none) in their place. 200, no error.
18+
*
19+
* ## Why the pin drives the REAL editor
20+
*
21+
* `mergePermissionSlice` is correct for any base it is handed — its own pins
22+
* (`permission-slice.authoredKeys.test.ts`, `permission-slice.facetDrift.test.ts`)
23+
* call it directly and could never see this. The defect is the CHOICE of base,
24+
* which only `doSave` makes, so these pins run the real page, the real load and
25+
* the real autosave (objectui#11787: the package door has no Save button).
26+
*
27+
* ## The arms, and what each one's absence is read against
28+
*
29+
* 1. A pending draft whose non-authored keys differ from the published body:
30+
* every one of them is on the wire byte-for-byte, out-of-scope rows come
31+
* from the draft, and the edited row lands.
32+
* 2. No pending draft: the base is still the published `effective`, so the
33+
* published body's own keys and other packages' rows are on the wire —
34+
* the control that the draft arm is not "the edited body, wholesale".
35+
* 3. The save-time DRAFT read rejects: the save is refused on the same
36+
* channel as objectui#9420's layered refusal, `client.save` is never
37+
* called, and the control `getDraftCalls === 2` proves the read ran.
38+
*/
39+
40+
import '@testing-library/jest-dom/vitest';
41+
import { afterEach, describe, expect, it, vi } from 'vitest';
42+
import { cleanup, fireEvent, render, screen, waitFor, within } from '@testing-library/react';
43+
import { MemoryRouter } from 'react-router-dom';
44+
45+
/**
46+
* The published record. `a_account` is the door's own package; `b_order` and
47+
* `b_order.total` are another package's grants.
48+
*/
49+
const PUBLISHED = {
50+
name: 'sales_perms',
51+
label: 'Sales',
52+
description: 'Published description',
53+
objects: {
54+
a_account: { allowRead: true, allowCreate: true },
55+
b_order: { allowRead: true },
56+
},
57+
fields: { 'b_order.total': { readable: true, editable: false } },
58+
};
59+
60+
/**
61+
* The pending package draft another door wrote — every key the editor cannot
62+
* author differs from {@link PUBLISHED}, and so does an out-of-scope row, so a
63+
* value taken from the wrong base cannot pass for the right one.
64+
*/
65+
const DRAFT = {
66+
name: 'sales_perms',
67+
label: 'Sales',
68+
description: 'Set through the API',
69+
packageId: 'app.a',
70+
objects: {
71+
a_account: { allowRead: true, allowCreate: true },
72+
b_order: { allowRead: true, allowEdit: true, viewAllRecords: true },
73+
},
74+
fields: { 'b_order.total': { readable: true, editable: true } },
75+
};
76+
77+
/** The facets `EDITOR_AUTHORED_KEYS` does not name — the ones the base supplies. */
78+
const NON_AUTHORED = ['description', 'packageId'] as const;
79+
80+
/** Distinctive slice of the refusal the author sees on the page's error strip. */
81+
const REFUSAL = /could not be re-read/;
82+
83+
interface Server {
84+
saved: Array<Record<string, unknown>>;
85+
savedOpts: Array<Record<string, unknown> | undefined>;
86+
/** `getDraft` calls seen so far — #1 is the load, #2 is `doSave`'s re-read. */
87+
getDraftCalls: number;
88+
}
89+
90+
function freshServer(): Server {
91+
return { saved: [], savedOpts: [], getDraftCalls: 0 };
92+
}
93+
94+
function makeClient(server: Server, opts: { draft: Record<string, unknown> | null; rejectSaveTimeDraft?: boolean }) {
95+
return {
96+
layered: async () => ({ effective: PUBLISHED, code: null, overlay: null, overlayScope: null }),
97+
getDraft: async () => {
98+
server.getDraftCalls += 1;
99+
if (opts.rejectSaveTimeDraft && server.getDraftCalls > 1) throw new Error('draft read failed');
100+
return opts.draft ? { type: 'permission', name: 'sales_perms', item: opts.draft, version: 'v1' } : null;
101+
},
102+
// The package declares `a_account` only.
103+
list: async (type: string) => (type === 'object' ? [{ item: { name: 'a_account' } }] : []),
104+
get: async () => null,
105+
save: async (
106+
_type: string,
107+
_name: string,
108+
payload: Record<string, unknown>,
109+
saveOpts?: Record<string, unknown>,
110+
) => {
111+
server.saved.push(payload);
112+
server.savedOpts.push(saveOpts);
113+
return payload;
114+
},
115+
} as any;
116+
}
117+
118+
let clientImpl: any;
119+
120+
vi.mock('./useMetadata', () => ({
121+
useMetadataClient: () => clientImpl,
122+
useMetadataTypes: () => ({
123+
loading: false,
124+
error: null,
125+
entries: [{ type: 'permission', label: 'Permission', allowEnvironmentOverlay: true }],
126+
}),
127+
}));
128+
129+
vi.mock('./AssignedUsersSection', () => ({ AssignedUsersSection: () => null }));
130+
131+
import { PermissionMatrixEditPage } from './PermissionMatrixEditor';
132+
133+
afterEach(cleanup);
134+
135+
function renderPackageDoor() {
136+
return render(
137+
<MemoryRouter>
138+
<PermissionMatrixEditPage type="permission" name="sales_perms" packageId="app.a" />
139+
</MemoryRouter>,
140+
);
141+
}
142+
143+
/** One ordinary edit: revoke every grant on the door's own `a_account` row. */
144+
function editAccountRow() {
145+
const row = screen.getByText('a_account').closest('tr')!;
146+
fireEvent.click(within(row).getByRole('button', { name: 'None' }));
147+
}
148+
149+
/** The package door autosaves after the shared pause (1.5s). */
150+
const AUTOSAVE = { timeout: 4000 };
151+
152+
describe('PermissionMatrixEditPage — the PACKAGE door saves onto the pending draft (objectstack#26837)', () => {
153+
it('keeps every key the editor does not author from the pending draft, byte-for-byte', async () => {
154+
const server = freshServer();
155+
clientImpl = makeClient(server, { draft: DRAFT });
156+
renderPackageDoor();
157+
await screen.findByText('a_account');
158+
159+
editAccountRow();
160+
161+
await waitFor(() => expect(server.saved).toHaveLength(1), AUTOSAVE);
162+
const body = server.saved[0] as any;
163+
164+
// The edited row lands…
165+
expect(body.objects.a_account).toEqual({});
166+
// …and the draft's non-authored keys survive, not the published ones.
167+
for (const key of NON_AUTHORED) expect(body[key]).toEqual(DRAFT[key]);
168+
// Out-of-scope rows come from the draft too: it is what this save replaces.
169+
expect(body.objects.b_order).toEqual(DRAFT.objects.b_order);
170+
expect(body.fields).toEqual(DRAFT.fields);
171+
// CONTROL: the save-time draft read ran (#1 is the load).
172+
expect(server.getDraftCalls).toBe(2);
173+
expect(server.savedOpts[0]).toMatchObject({ mode: 'draft', packageId: 'app.a' });
174+
expect(screen.queryByText(REFUSAL)).toBeNull();
175+
});
176+
177+
it('CONTROL — with no pending draft the base is still the published body', async () => {
178+
const server = freshServer();
179+
clientImpl = makeClient(server, { draft: null });
180+
renderPackageDoor();
181+
await screen.findByText('a_account');
182+
183+
editAccountRow();
184+
185+
await waitFor(() => expect(server.saved).toHaveLength(1), AUTOSAVE);
186+
const body = server.saved[0] as any;
187+
188+
expect(body.objects.a_account).toEqual({});
189+
expect(body.description).toBe(PUBLISHED.description);
190+
expect('packageId' in body).toBe(false);
191+
expect(body.objects.b_order).toEqual(PUBLISHED.objects.b_order);
192+
expect(body.fields).toEqual(PUBLISHED.fields);
193+
expect(screen.queryByText(REFUSAL)).toBeNull();
194+
});
195+
196+
it('REFUSES the save when the save-time draft read rejects, rather than fall back to the published body', async () => {
197+
const server = freshServer();
198+
clientImpl = makeClient(server, { draft: DRAFT, rejectSaveTimeDraft: true });
199+
renderPackageDoor();
200+
await screen.findByText('a_account');
201+
202+
editAccountRow();
203+
204+
// Settle on either arm — the refusal on screen, or a PUT landing.
205+
await waitFor(
206+
() => expect(server.saved.length > 0 || screen.queryByText(REFUSAL) !== null).toBe(true),
207+
AUTOSAVE,
208+
);
209+
210+
// CONTROL: the save-time draft read really ran and really rejected.
211+
expect(server.getDraftCalls).toBe(2);
212+
expect(server.saved).toEqual([]);
213+
expect(server.savedOpts).toEqual([]);
214+
// The same error strip objectui#9420's layered refusal renders into.
215+
expect(screen.getByText(REFUSAL)).toBeInTheDocument();
216+
expect(screen.queryByTestId('perm-autosaving')).toBeNull();
217+
});
218+
});

‎ui/packages/app-shell/src/views/metadata-admin/PermissionMatrixEditor.tsx‎

Lines changed: 45 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -266,6 +266,19 @@ function isArtifactBackedLayer(
266266
);
267267
}
268268

269+
/**
270+
* The editable body of a pending package draft, as `readDraftForSave` served
271+
* it (the `{ type, name, item }` envelope, or a bare body), or `null` for "no
272+
* draft". Read decorations come off here, at the unwrap, so neither the load
273+
* nor the save's merge base carries `_diagnostics` / `_draft` (objectui#8181).
274+
*/
275+
function pendingDraftBody(
276+
answer: { item?: PermissionSetDraft } | PermissionSetDraft | null,
277+
): PermissionSetDraft | null {
278+
if (!answer) return null;
279+
return stripReadDecorations((answer as { item?: PermissionSetDraft }).item ?? answer) as PermissionSetDraft;
280+
}
281+
269282
/** Localized short label for an OWD value; falls back to the raw value. */
270283
function owdLabel(t: (k: string) => string, value: string): string {
271284
const key: Record<string, string> = {
@@ -867,13 +880,10 @@ export function PermissionMatrixEditPage({ type, name, packageId, onDraftSaved,
867880
// below re-bases on a fresh RAW `layered` read, which drops them — but
868881
// its `.catch(() => null)` arm falls back to this very body and
869882
// spreads it into `client.save`, so a failed layered read used to put
870-
// `_diagnostics` / `_draft` on the wire. Strip at the unwrap, which is
871-
// the one place the served envelope becomes an editable draft.
872-
const draftBody = pendingDraft
873-
? (stripReadDecorations(
874-
(pendingDraft as any).item ?? pendingDraft,
875-
) as PermissionSetDraft)
876-
: null;
883+
// `_diagnostics` / `_draft` on the wire. Strip at the unwrap,
884+
// `pendingDraftBody`, the one function through which a served envelope
885+
// becomes an editable body — here and in `doSave`'s merge base.
886+
const draftBody = pendingDraftBody(pendingDraft);
877887
// Draft wins over the published baseline for display (D6).
878888
const effective: PermissionSetDraft = (draftBody ?? lay?.effective ??
879889
lay?.code ?? { name, objects: {} }) as PermissionSetDraft;
@@ -1280,19 +1290,36 @@ export function PermissionMatrixEditPage({ type, name, packageId, onDraftSaved,
12801290
// purpose: a set that exists only as a package draft (what the Studio
12811291
// Access pillar's "+ New" creates) has no published rows for anyone to
12821292
// lose, and refusing there would block its first save.
1293+
//
1294+
// objectstack#26837 — and this door's save WRITES the pending draft, so
1295+
// when one exists it is the base, not the published body: the facets
1296+
// the editor cannot author (`description`, `packageId`, any key another
1297+
// door wrote) come from the draft, exactly as the load displayed it.
1298+
// Read through the load's own door, at the address this save writes.
1299+
// The pin the load recorded is left alone, so a draft that moved since
1300+
// the load is still refused as a version conflict. `readDraftForSave`
1301+
// resolves a REJECTED read as no answer AND no pin, while a read that
1302+
// found no draft pins `If-None-Match: *` — so the pair tells a failure
1303+
// apart from "no draft", and a failure refuses exactly like the
1304+
// layered re-read: falling back to the published body would drop the
1305+
// draft's values with a 200.
12831306
let rereadFailed = false;
1284-
const fresh = await client
1285-
.layered<PermissionSetDraft>(type, payload.name)
1286-
// Kept on `.catch` so the rejection can never escape as an unhandled
1287-
// one; the refusal is raised below, inside this `try`'s own body, and
1288-
// reaches the author on the same error channel a failed
1289-
// `client.save` already uses.
1290-
.catch(() => {
1291-
rereadFailed = true;
1292-
return null;
1293-
});
1307+
const [fresh, draftRead] = await Promise.all([
1308+
client
1309+
.layered<PermissionSetDraft>(type, payload.name)
1310+
// Kept on `.catch` so the rejection can never escape as an
1311+
// unhandled one; the refusal is raised below, inside this `try`'s
1312+
// own body, and reaches the author on the same error channel a
1313+
// failed `client.save` already uses.
1314+
.catch(() => {
1315+
rereadFailed = true;
1316+
return null;
1317+
}),
1318+
readDraftForSave<{ item?: PermissionSetDraft } | PermissionSetDraft>(client, type, payload.name, packageId),
1319+
]);
1320+
if (draftRead.answer === null && draftRead.pin === null) rereadFailed = true;
12941321
if (rereadFailed) throw new Error(t('perm.save.rereadFailed'));
1295-
const base = (fresh?.effective ?? payload) as PermissionSetDraft;
1322+
const base = (pendingDraftBody(draftRead.answer) ?? fresh?.effective ?? payload) as PermissionSetDraft;
12961323
toSave = mergePermissionSlice(base, payload, scope);
12971324
}
12981325
// ADR-0086 P2 (D6/D7). Package door → the set is metadata: write a DRAFT

0 commit comments

Comments
 (0)