Skip to content

Commit dc87dfb

Browse files
fix(app-shell): a package-door permission save keeps the pending draft's values (#27080)
<!-- claude-projects-attribution: {"github_login":"os-warren"} --> _Requested by **Warren** · [project thread](https://claude.ai/code/project/chan_01U5RdnAAmkSQmZbqh6FtqZ8?thread=cmsg_01U5RdnAAmkSQmZbqh6FtqZ8HDu4gcLm3Z4tJj8SMXXi7D)_ Fixes #26837 Clause-②: no Metadata-protocol: none ## 维护者速读 - 改了什么:Studio 包内权限集的保存,合并基底从「已发布版本」改为「当前待发布草稿」;没有草稿时仍用已发布版本。 - 为什么改:经 API 或其他入口写进草稿的 description、packageId 等编辑器不管的键,在 Studio 里勾一个格子就被已发布值覆盖,静默丢数据。 - 风险与回滚:保存前多读一次草稿;草稿读取失败时拒绝保存,提示与已发布重读失败相同(作者可再次编辑重试)。回滚即还原本 PR 的修复提交。 - 先看:`PermissionMatrixEditor.tsx` 的 `doSave` 包门分支(`base` 一行与草稿读失败判断);新测试 `PermissionMatrixEditor.packageDoorDraftBase-26837.test.tsx`;同文件新增的 `pendingDraftBody`。 - 两分钟验证:`pnpm --dir ui exec vitest run packages/app-shell/src/views/metadata-admin/PermissionMatrixEditor.packageDoorDraftBase-26837.test.tsx`,预期 3 passed。 - 需要你做的:无,席位落地。 ## What changes - `doSave`'s package-door arm now reads the pending draft through the load's own door, `readDraftForSave`, at the address the save writes (in parallel with the existing `layered` re-read), and merges this package's slice onto the draft when one exists. With no pending draft the base is the published `effective`, and the 404 shape keeps the `?? payload` base, both as before. - A REJECTED save-time draft read refuses the save with the same `perm.save.rereadFailed` text as the layered refusal (objectui#9420); it never falls back to the published body. `readDraftForSave` resolves a rejection as no answer and no pin, while a read that found no draft pins `If-None-Match: *`, so that pair is the failure signal. - `mergePermissionSlice` and `EDITOR_AUTHORED_KEYS` are unchanged: measured, the key list already takes only the authored facets from the edited body. The defect was the base only. - The unwrap of a served draft envelope (strip read decorations, objectui#8181) moves into one module-level helper, `pendingDraftBody`, used by the load and by the save base, so the two can't drift apart. - The environment door (no `packageId`) keeps its whole-record live save unchanged. ## Why The package door's save writes the package DRAFT, but its merge base was the PUBLISHED body. Keys the editor does not author (`description`, `packageId`, anything another door wrote) came from that base, so one Studio edit replaced a pending draft's values with published ones (or with none). The response was 200 and nothing showed an error. ## Tests New `PermissionMatrixEditor.packageDoorDraftBase-26837.test.tsx` drives the real page, load and autosave: 1. A pending draft whose `description`, `packageId` and an out-of-scope row differ from published: every non-authored key and the out-of-scope rows reach the wire equal to the draft, and the edited row lands. 2. Control, no pending draft: the base is the published body (its `description`, no `packageId`, its rows). 3. The save-time draft read rejects: `client.save` is never called, the refusal is on the page's error strip, and `getDraftCalls === 2` proves the read ran. ## Acceptance notes - The load-time version pin is left alone: the save-time draft read does not re-record it. A draft that moved after the load is still refused as a `409 METADATA_CONFLICT` and opens the conflict dialog, instead of being merged silently. - When a pending draft exists, out-of-scope rows (other packages' grants) now come from the draft as well, since the draft is the document this save replaces and Publish promotes. - `readDraftForSave` has no explicit failure flag. This PR reads failure from its documented answer/pin pair and stays inside `PermissionMatrixEditor.tsx`. An explicit flag would be a change to `DraftConflictDialog.tsx`, which is outside this card's file surface. - Serial work in the same file: PR #26999 and PR #27064 are both still open. Neither has a hunk in the save-base region. PR #27064 edits the load path near the `draftBody` line this PR changes, so whichever lands second may need a small textual merge. ## Verification Head `4dd4403f` (the fix commit `d87310b4` plus a merge of `origin/main` at `f708cf22`). <details><summary>Ablation (two legs, both restored to the HEAD blob)</summary> `node scripts/ablation-replace.mjs` under the verify lock, tests import the editor by relative path (no dist leg): - Base back on `fresh?.effective ?? payload`: anchor 1 → 0, blob ae1632b → 55370e9d; the draft-survival test went red with `expected 'Published description' to deeply equal 'Set through the API'` (1 failed, 2 passed); restored, blob equals HEAD, `git diff HEAD` empty. - The draft-read refusal line deleted: anchor 1 → 0; the refusal test went red with one PUT where none was expected (1 failed, 2 passed); restored, blob equals HEAD, `git diff HEAD` empty. </details> <details><summary>Tests and type-check</summary> - All 45 test files that reference the permission editor: 45/45 files, 321/321 tests passed on head `4dd4403f` (and on `d87310b4` before the merge). - 24 further app-shell test files naming the `permission` type (Studio Access pillar, list pages, previews, draft conflict): 24/24 files, 392/392 tests passed on `d87310b4`. - `pnpm --filter @object-ui/app-shell run type-check` (both `tsc` projects, tests included): exit 0. - `pnpm --dir ui run check:new-line-citations`: 0 new citations. </details> <details><summary>Derived gates (24 families, all exit 0)</summary> `node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack` at `4dd4403f` derived 24 families; each was run and its exit recorded before any pipe; `--ran` reconciliation: 24 derived, 24 run, 0 NOT-MEASURED, 0 UNRUN. `--tier`: no path-derived mandate. Changed lines 281, under the 3000 threshold. </details> No changeset: `@object-ui/app-shell` is private (`skip-changeset`). ## Human Minimum - [x] 1. Schemas start as Zod; `packages/spec` holds no runtime logic. (not touched) - [x] 2. Every name I added uses the casing for its kind. - [x] 3. No formatter ran; `pnpm lint` passes. (CI runs the repo lint) - [x] 4. One issue, one branch off an up-to-date `main`, one pull request. - [x] 5. A change that publishes carries a changeset, and both declaration lines are filled in here and in the changeset. (private package, no changeset) - [x] 6. `packages/spec` touched: its generated artifacts are regenerated and committed. (not touched) - [x] 7. No hand edits to `content/docs/references/`, `packages/*/CHANGELOG.md` or `content/docs/releases/`. - [x] 8. Title, body and comments are in English. (the 维护者速读 block is the os-dev order's required Chinese summary) --- _Generated by [Claude Code](https://claude.ai/code/session_01JQwxba8E9EE7oBM24GbdNZ)_ Co-authored-by: Claude <noreply@anthropic.com>
1 parent 19a2b61 commit dc87dfb

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
@@ -297,6 +297,19 @@ function isArtifactBackedLayer(
297297
);
298298
}
299299

300+
/**
301+
* The editable body of a pending package draft, as `readDraftForSave` served
302+
* it (the `{ type, name, item }` envelope, or a bare body), or `null` for "no
303+
* draft". Read decorations come off here, at the unwrap, so neither the load
304+
* nor the save's merge base carries `_diagnostics` / `_draft` (objectui#8181).
305+
*/
306+
function pendingDraftBody(
307+
answer: { item?: PermissionSetDraft } | PermissionSetDraft | null,
308+
): PermissionSetDraft | null {
309+
if (!answer) return null;
310+
return stripReadDecorations((answer as { item?: PermissionSetDraft }).item ?? answer) as PermissionSetDraft;
311+
}
312+
300313
/** Localized short label for an OWD value; falls back to the raw value. */
301314
function owdLabel(t: (k: string) => string, value: string): string {
302315
const key: Record<string, string> = {
@@ -910,13 +923,10 @@ export function PermissionMatrixEditPage({ type, name, packageId, onDraftSaved,
910923
// below re-bases on a fresh RAW `layered` read, which drops them — but
911924
// its `.catch(() => null)` arm falls back to this very body and
912925
// spreads it into `client.save`, so a failed layered read used to put
913-
// `_diagnostics` / `_draft` on the wire. Strip at the unwrap, which is
914-
// the one place the served envelope becomes an editable draft.
915-
const draftBody = pendingDraft
916-
? (stripReadDecorations(
917-
(pendingDraft as any).item ?? pendingDraft,
918-
) as PermissionSetDraft)
919-
: null;
926+
// `_diagnostics` / `_draft` on the wire. Strip at the unwrap,
927+
// `pendingDraftBody`, the one function through which a served envelope
928+
// becomes an editable body — here and in `doSave`'s merge base.
929+
const draftBody = pendingDraftBody(pendingDraft);
920930
// Draft wins over the published baseline for display (D6).
921931
const effective: PermissionSetDraft = (draftBody ?? lay?.effective ??
922932
lay?.code ?? { name, objects: {} }) as PermissionSetDraft;
@@ -1344,19 +1354,36 @@ export function PermissionMatrixEditPage({ type, name, packageId, onDraftSaved,
13441354
// purpose: a set that exists only as a package draft (what the Studio
13451355
// Access pillar's "+ New" creates) has no published rows for anyone to
13461356
// lose, and refusing there would block its first save.
1357+
//
1358+
// objectstack#26837 — and this door's save WRITES the pending draft, so
1359+
// when one exists it is the base, not the published body: the facets
1360+
// the editor cannot author (`description`, `packageId`, any key another
1361+
// door wrote) come from the draft, exactly as the load displayed it.
1362+
// Read through the load's own door, at the address this save writes.
1363+
// The pin the load recorded is left alone, so a draft that moved since
1364+
// the load is still refused as a version conflict. `readDraftForSave`
1365+
// resolves a REJECTED read as no answer AND no pin, while a read that
1366+
// found no draft pins `If-None-Match: *` — so the pair tells a failure
1367+
// apart from "no draft", and a failure refuses exactly like the
1368+
// layered re-read: falling back to the published body would drop the
1369+
// draft's values with a 200.
13471370
let rereadFailed = false;
1348-
const fresh = await client
1349-
.layered<PermissionSetDraft>(type, payload.name)
1350-
// Kept on `.catch` so the rejection can never escape as an unhandled
1351-
// one; the refusal is raised below, inside this `try`'s own body, and
1352-
// reaches the author on the same error channel a failed
1353-
// `client.save` already uses.
1354-
.catch(() => {
1355-
rereadFailed = true;
1356-
return null;
1357-
});
1371+
const [fresh, draftRead] = await Promise.all([
1372+
client
1373+
.layered<PermissionSetDraft>(type, payload.name)
1374+
// Kept on `.catch` so the rejection can never escape as an
1375+
// unhandled one; the refusal is raised below, inside this `try`'s
1376+
// own body, and reaches the author on the same error channel a
1377+
// failed `client.save` already uses.
1378+
.catch(() => {
1379+
rereadFailed = true;
1380+
return null;
1381+
}),
1382+
readDraftForSave<{ item?: PermissionSetDraft } | PermissionSetDraft>(client, type, payload.name, packageId),
1383+
]);
1384+
if (draftRead.answer === null && draftRead.pin === null) rereadFailed = true;
13581385
if (rereadFailed) throw new Error(t('perm.save.rereadFailed'));
1359-
const base = (fresh?.effective ?? payload) as PermissionSetDraft;
1386+
const base = (pendingDraftBody(draftRead.answer) ?? fresh?.effective ?? payload) as PermissionSetDraft;
13601387
toSave = mergePermissionSlice(base, payload, scope);
13611388
}
13621389
// ADR-0086 P2 (D6/D7). Package door → the set is metadata: write a DRAFT

0 commit comments

Comments
 (0)