Skip to content

Commit b896749

Browse files
os-zhuangclaude
andauthored
fix(app-shell): guard unsaved OWD overview rows in the Access rail and Studio header nav (#2600 follow-up) (#2610)
PackageOwdOverviewPanel tracked its unsaved per-object OWD rows internally (rowState/dirtyCount) but reported them to no one, so both unsaved-edit guards discarded them silently: the Access rail's confirm (confirmDiscardMatrixEdits checked only matrixDirty) and the Studio header's surface-level pillarDirty guard from PR #2606. - PackageOwdOverviewPanel grows onDirtyChange — same contract as PermissionMatrixEditPage (ref-stabilized transitions, false on unmount). - AccessPillar folds it in as owdDirty alongside matrixDirty: the rail confirm gates on the combined bit (the two surfaces never coexist), and the combined bit mirrors up to the surface via an effect (replacing the reportMatrixDirty wrapper). - Re-clicking the open OWD rail entry is now a no-op (nothing remounts, so no confirm), mirroring the set re-click. - The create flow now lands on the new set's matrix even from the OWD overview (setOwdOpen(false) in doCreate), so the up-front gate's discard warning is truthful. Co-authored-by: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 parent 5933a41 commit b896749

5 files changed

Lines changed: 230 additions & 41 deletions

File tree

‎packages/app-shell/src/views/studio-design/PackageOwdOverviewPanel.test.tsx‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -177,3 +177,40 @@ describe('PackageOwdOverviewPanel — deep-link highlight', () => {
177177
expect(row.className).toMatch(/bg-primary/);
178178
});
179179
});
180+
181+
describe('PackageOwdOverviewPanel — onDirtyChange contract (objectui#2600)', () => {
182+
it('reports dirty transitions and resets to clean on unmount', async () => {
183+
const onDirtyChange = vi.fn();
184+
const { unmount } = renderPanel(freshServer(), { onDirtyChange });
185+
await screen.findByTestId('owd-row-crm_contact');
186+
expect(onDirtyChange).toHaveBeenLastCalledWith(false);
187+
188+
fireEvent.change(screen.getByTestId('owd-internal-crm_contact'), { target: { value: 'public_read' } });
189+
expect(onDirtyChange).toHaveBeenLastCalledWith(true);
190+
191+
// Reverting the edit back to the baseline reports clean again.
192+
fireEvent.change(screen.getByTestId('owd-internal-crm_contact'), { target: { value: 'private' } });
193+
expect(onDirtyChange).toHaveBeenLastCalledWith(false);
194+
195+
// Dirty again, then unmount — a deliberately-discarded panel must clear
196+
// the host's guard by itself (same contract as PermissionMatrixEditPage).
197+
fireEvent.change(screen.getByTestId('owd-internal-crm_contact'), { target: { value: 'public_read' } });
198+
expect(onDirtyChange).toHaveBeenLastCalledWith(true);
199+
unmount();
200+
expect(onDirtyChange).toHaveBeenLastCalledWith(false);
201+
});
202+
203+
it('reports clean after a successful save (edits become the new baseline)', async () => {
204+
const onDirtyChange = vi.fn();
205+
const server = freshServer();
206+
renderPanel(server, { onDirtyChange });
207+
await screen.findByTestId('owd-row-crm_contact');
208+
209+
fireEvent.change(screen.getByTestId('owd-internal-crm_contact'), { target: { value: 'public_read' } });
210+
expect(onDirtyChange).toHaveBeenLastCalledWith(true);
211+
212+
fireEvent.click(screen.getByTestId('owd-save'));
213+
await waitFor(() => expect(server.saved.length).toBe(1));
214+
await waitFor(() => expect(onDirtyChange).toHaveBeenLastCalledWith(false));
215+
});
216+
});

‎packages/app-shell/src/views/studio-design/PackageOwdOverviewPanel.tsx‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,11 @@ export interface PackageOwdOverviewPanelProps {
9090
locale: SupportedLocale;
9191
/** Object to scroll to / highlight (deep-link from the permission matrix badge). */
9292
highlightObject?: string | null;
93+
/** objectui#2600 — mirrors the unsaved-row state up to the host pillar, whose
94+
* rail/header navigation unmounts this panel (SPA nav, so no beforeunload).
95+
* Reports `false` on unmount so a confirmed discard clears the host's guard —
96+
* same contract as PermissionMatrixEditPage's own onDirtyChange. */
97+
onDirtyChange?: (dirty: boolean) => void;
9398
}
9499

95100
export function PackageOwdOverviewPanel({
@@ -100,6 +105,7 @@ export function PackageOwdOverviewPanel({
100105
readOnly = false,
101106
locale,
102107
highlightObject,
108+
onDirtyChange,
103109
}: PackageOwdOverviewPanelProps): React.ReactElement {
104110
const [rows, setRows] = React.useState<OwdRow[]>([]);
105111
const [edits, setEdits] = React.useState<Record<string, OwdEdit>>({});
@@ -203,6 +209,25 @@ export function PackageOwdOverviewPanel({
203209
const dirtyCount = rowState.filter((s) => s.dirty).length;
204210
const hasInvalid = rowState.some((s) => s.invalid);
205211

212+
// Report dirty transitions to the host (see onDirtyChange). Ref-stabilized
213+
// so a non-memoized callback prop doesn't refire the effect; the unmount
214+
// cleanup reports `false` so a deliberately-discarded panel clears the
215+
// host's guard state.
216+
const isDirty = dirtyCount > 0;
217+
const onDirtyChangeRef = React.useRef(onDirtyChange);
218+
React.useEffect(() => {
219+
onDirtyChangeRef.current = onDirtyChange;
220+
});
221+
React.useEffect(() => {
222+
onDirtyChangeRef.current?.(isDirty);
223+
}, [isDirty]);
224+
React.useEffect(
225+
() => () => {
226+
onDirtyChangeRef.current?.(false);
227+
},
228+
[],
229+
);
230+
206231
const doSave = React.useCallback(async () => {
207232
const changed = rowState.filter((s) => s.dirty && !s.invalid);
208233
if (changed.length === 0) return;

‎packages/app-shell/src/views/studio-design/StudioDesignSurface.accessGuard.test.tsx‎

Lines changed: 85 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -6,14 +6,17 @@
66
* The permission matrix in the main panel is keyed by the selected set
77
* (`key={current}`) and unmounts entirely when the OWD overview swaps in, so
88
* before this guard existed a rail click silently REMOUNTED the matrix and
9-
* discarded any unsaved cell edits. The pillar now holds the editor's
10-
* reported dirty state (`onDirtyChange`) and gates every swap on a native
11-
* confirm:
9+
* discarded any unsaved cell edits. The OWD overview batch-editor holds
10+
* unsaved rows the same way and unmounts when a set swaps back in
11+
* (objectui#2600). The pillar now holds both editors' reported dirty state
12+
* (`onDirtyChange`) and gates every swap on a native confirm:
1213
*
1314
* • dirty + switch to another set → confirm; cancel keeps the edits,
1415
* • dirty + swap to the OWD overview → same confirm,
16+
* • dirty OWD rows + swap to a set / create flow → same confirm,
1517
* • clean switches never prompt,
16-
* • re-clicking the already-open set never prompts (nothing remounts),
18+
* • re-clicking the already-open set / overview never prompts (nothing
19+
* remounts),
1720
* • a confirmed discard clears the guard (the editor resets on unmount).
1821
*/
1922

@@ -38,9 +41,7 @@ vi.mock('../metadata-admin/useMetadata', async (importOriginal) => {
3841
});
3942

4043
// Rail siblings that are irrelevant to the guard — keep the render light.
41-
vi.mock('./PackageOwdOverviewPanel', () => ({
42-
PackageOwdOverviewPanel: () => <div data-testid="owd-overview" />,
43-
}));
44+
// (The OWD overview is NOT mocked: its dirty report is under test.)
4445
vi.mock('../../components/SuggestedBindingsPanel', () => ({
4546
SuggestedBindingsPanel: () => null,
4647
}));
@@ -112,8 +113,10 @@ function makeClient(server: Server) {
112113
getDraft: async () => null,
113114
get: async (type: string) =>
114115
type === 'object' ? { fields: [{ name: 'name', label: 'Name' }] } : null,
115-
save: async (_t: string, _n: string, payload: Record<string, unknown>) => {
116+
save: async (_t: string, name: string, payload: Record<string, unknown>) => {
116117
server.saved.push(payload);
118+
// Register the set so the post-create reload can list and open it.
119+
server.byName[name] = payload;
117120
return payload;
118121
},
119122
} as any;
@@ -211,3 +214,77 @@ describe('AccessPillar — unsaved matrix edits guard', () => {
211214
expect(within(row).getByRole('checkbox', { name: 'a_account Read' })).not.toBeChecked();
212215
});
213216
});
217+
218+
/** Swap the main panel to the (real) OWD overview and edit a row so the panel
219+
* reports dirty. Assumes the open matrix is clean, so the swap never prompts. */
220+
async function openAndDirtyTheOwd() {
221+
fireEvent.click(screen.getByRole('button', { name: 'Record sharing (OWD)' }));
222+
await screen.findByTestId('owd-internal-a_account');
223+
fireEvent.change(screen.getByTestId('owd-internal-a_account'), { target: { value: 'private' } });
224+
await screen.findByTestId('owd-dirty-a_account');
225+
}
226+
227+
const owdInternalValue = () =>
228+
(screen.getByTestId('owd-internal-a_account') as HTMLSelectElement).value;
229+
230+
describe('AccessPillar — unsaved OWD overview edits guard (#2600)', () => {
231+
it('asks before swapping a dirty overview for a set; cancel keeps the edits', async () => {
232+
await renderPillarOnSetA();
233+
await openAndDirtyTheOwd();
234+
235+
confirmSpy.mockReturnValueOnce(false);
236+
fireEvent.click(screen.getByRole('button', { name: 'Set A' }));
237+
expect(confirmSpy).toHaveBeenCalledTimes(1);
238+
// Still on the overview, edit intact.
239+
expect(owdInternalValue()).toBe('private');
240+
241+
// Confirming the same swap discards and mounts the set A matrix.
242+
confirmSpy.mockReturnValueOnce(true);
243+
fireEvent.click(screen.getByRole('button', { name: 'Set A' }));
244+
expect(confirmSpy).toHaveBeenCalledTimes(2);
245+
await screen.findByDisplayValue('set_a');
246+
247+
// The discarded panel reset the guard on unmount — swapping back to the
248+
// overview must NOT prompt, and it reloads the untouched baseline.
249+
fireEvent.click(screen.getByRole('button', { name: 'Record sharing (OWD)' }));
250+
expect(confirmSpy).toHaveBeenCalledTimes(2);
251+
await screen.findByTestId('owd-internal-a_account');
252+
expect(owdInternalValue()).toBe('');
253+
});
254+
255+
it('never prompts when re-clicking the OWD rail entry (nothing remounts)', async () => {
256+
await renderPillarOnSetA();
257+
await openAndDirtyTheOwd();
258+
259+
fireEvent.click(screen.getByRole('button', { name: 'Record sharing (OWD)' }));
260+
261+
expect(confirmSpy).not.toHaveBeenCalled();
262+
// Not remounted — the unsaved edit is still visible.
263+
expect(owdInternalValue()).toBe('private');
264+
});
265+
266+
it('gates the create-set flow, and creating lands on the new set matrix', async () => {
267+
await renderPillarOnSetA();
268+
await openAndDirtyTheOwd();
269+
270+
// Cancel keeps the overview (and its edits) — no dialog.
271+
confirmSpy.mockReturnValueOnce(false);
272+
fireEvent.click(screen.getByRole('button', { name: 'New permission set' }));
273+
expect(confirmSpy).toHaveBeenCalledTimes(1);
274+
expect(screen.queryByRole('dialog')).toBeNull();
275+
expect(owdInternalValue()).toBe('private');
276+
277+
// Confirm opens the creator; submitting swaps the overview out for the
278+
// new set's matrix (the discard the gate warned about).
279+
confirmSpy.mockReturnValueOnce(true);
280+
fireEvent.click(screen.getByRole('button', { name: 'New permission set' }));
281+
const dialog = await screen.findByRole('dialog');
282+
fireEvent.change(within(dialog).getByPlaceholderText('Display name (e.g. Sales permissions)'), {
283+
target: { value: 'Set C' },
284+
});
285+
fireEvent.click(within(dialog).getByRole('button', { name: 'Create' }));
286+
287+
await screen.findByDisplayValue('set_c');
288+
expect(screen.queryByTestId('owd-overview')).not.toBeInTheDocument();
289+
});
290+
});

‎packages/app-shell/src/views/studio-design/StudioDesignSurface.pillarNavGuard.test.tsx‎

Lines changed: 30 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,8 @@
1313
* • dirty + pillar switch → confirm; cancel keeps the pillar and its edits,
1414
* • dirty + Home → same confirm; cancel stays, confirm leaves,
1515
* • dirty + package switch → same confirm,
16+
* • dirty OWD overview rows + pillar switch → same confirm (the pillar
17+
* folds the overview's report into the same bit, objectui#2600 follow-up),
1618
* • clean navigation never prompts,
1719
* • re-clicking the open pillar never prompts (nothing unmounts),
1820
* • a confirmed discard clears the guard (the pillar resets on unmount).
@@ -55,10 +57,8 @@ vi.mock('./packages-io', async (importOriginal) => {
5557
};
5658
});
5759

58-
// Rail siblings / docks that are irrelevant to the guard — keep the render light.
59-
vi.mock('./PackageOwdOverviewPanel', () => ({
60-
PackageOwdOverviewPanel: () => <div data-testid="owd-overview" />,
61-
}));
60+
// Rail siblings / docks that are irrelevant to the guard — keep the render
61+
// light. (The OWD overview is NOT mocked: its dirty report is under test.)
6262
vi.mock('../../components/SuggestedBindingsPanel', () => ({
6363
SuggestedBindingsPanel: () => null,
6464
}));
@@ -217,6 +217,32 @@ describe('Studio header — unsaved pillar edits guard (#2600)', () => {
217217
await screen.findByDisplayValue('set_a');
218218
});
219219

220+
it('gates a pillar switch on dirty OWD overview rows (#2600 follow-up)', async () => {
221+
await renderSurfaceOnAccess();
222+
// Swap the main panel to the (real) OWD overview and edit a row so the
223+
// panel's dirty report crosses AccessPillar into the surface guard.
224+
fireEvent.click(screen.getByRole('button', { name: 'Record sharing (OWD)' }));
225+
await screen.findByTestId('owd-internal-a_account');
226+
fireEvent.change(screen.getByTestId('owd-internal-a_account'), { target: { value: 'private' } });
227+
228+
confirmSpy.mockReturnValueOnce(false);
229+
fireEvent.click(screen.getByRole('link', { name: 'Interfaces' }));
230+
expect(confirmSpy).toHaveBeenCalledTimes(1);
231+
// Still on the overview, edit intact.
232+
expect((screen.getByTestId('owd-internal-a_account') as HTMLSelectElement).value).toBe('private');
233+
234+
// Confirming discards and leaves; the unmounting pillar resets the guard,
235+
// so walking back to Access must NOT prompt again.
236+
confirmSpy.mockReturnValueOnce(true);
237+
fireEvent.click(screen.getByRole('link', { name: 'Interfaces' }));
238+
expect(confirmSpy).toHaveBeenCalledTimes(2);
239+
await screen.findByText('This package has no app yet');
240+
241+
fireEvent.click(screen.getByRole('link', { name: 'Access' }));
242+
expect(confirmSpy).toHaveBeenCalledTimes(2);
243+
await screen.findByDisplayValue('set_a');
244+
});
245+
220246
it('switches pillars without prompting when clean', async () => {
221247
await renderSurfaceOnAccess();
222248

0 commit comments

Comments
 (0)