From 1dd499571478f920eefc6795d6c79a16cd70aabd Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 25 Aug 2026 23:54:08 +0000 Subject: [PATCH] fix(app-shell): converge both confirm runtimes on one close reset shape MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `app-shell` mounts two confirm runtimes into one `ActionConfirmDialog`. They agreed on open and disagreed on close: `useConsoleActionRuntime` replaced the whole state (`{ open: false, message: '' }`), blanking `message` and dropping the `options` / `resolve` keys, while `RecordDetailView` flipped one flag and kept every field. Converge on the field-preserving shape. Radix holds `AlertDialogContent` mounted through its exit animation (`data-[state=closed]:animate-out … duration-200`), so `ActionConfirmDialog` goes on reading `state.message` into the description and `state.options` into the title and both button labels for the whole fade-out — blanking rewrote the dialog's visible text mid-fade. The `resolve` this retains is inert: the dialog settles the promise before it asks for the close, and the open path replaces the whole state object. Extend the existing parity pin to the close path rather than adding a second pin — two pins for one relationship can disagree, which is the defect class the two runtimes had. The fixture is multi-field on purpose (message + a full options bag) so "blanked" and "preserved" are distinguishable, and a non-degeneracy test asserts the two shapes really do differ on this fixture. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_011SfZeFWrhGLHmfq61xbz4q --- .../confirm-runtime-close-parity-6034.md | 15 ++ .../src/hooks/useConsoleActionRuntime.tsx | 19 ++- ...ailView.confirmRuntimeParity-5835.test.tsx | 140 +++++++++++++++++- 3 files changed, 172 insertions(+), 2 deletions(-) create mode 100644 .changeset/confirm-runtime-close-parity-6034.md diff --git a/.changeset/confirm-runtime-close-parity-6034.md b/.changeset/confirm-runtime-close-parity-6034.md new file mode 100644 index 0000000000..94766b7b4a --- /dev/null +++ b/.changeset/confirm-runtime-close-parity-6034.md @@ -0,0 +1,15 @@ +--- +'@object-ui/app-shell': patch +--- + +Console action runtime: closing an action confirm dialog now keeps the dialog's +text instead of blanking it mid-fade. + +`useConsoleActionRuntime` reset its confirm state by replacing the whole object +(`{ open: false, message: '' }`), which cleared `message` and dropped `options`. +Radix keeps `AlertDialogContent` mounted through its exit animation, so the +dialog's description went blank and its title and button labels reverted to +their defaults while it was still fading out. It now flips only `open` and keeps +every field, matching `RecordDetailView`'s second confirm runtime, which already +closed this way. Both runtimes feed one `ActionConfirmDialog`; the parity pin now +covers the close path as well as the open path. diff --git a/packages/app-shell/src/hooks/useConsoleActionRuntime.tsx b/packages/app-shell/src/hooks/useConsoleActionRuntime.tsx index 171a9787ca..fb54c32240 100644 --- a/packages/app-shell/src/hooks/useConsoleActionRuntime.tsx +++ b/packages/app-shell/src/hooks/useConsoleActionRuntime.tsx @@ -692,8 +692,25 @@ export function useConsoleActionRuntime(opts: ConsoleActionRuntimeOptions): Cons const dialogs = ( <> + {/* + Close = flip `open` and KEEP every other field (objectui#6034). Radix + holds `AlertDialogContent` mounted through its exit animation + (`data-[state=closed]:animate-out … duration-200`), so + `ActionConfirmDialog` goes on reading `state.message` into the + description and `state.options` into the title and both button labels + for the whole fade-out. The `{ open: false, message: '' }` this replaced + blanked the description and reverted the labels to their i18n defaults + mid-fade. The retained `resolve` is inert — the dialog settles the + promise BEFORE it asks for the close, and the open path above replaces + the whole state object, so nothing stale survives a reopen. + + `RecordDetailView` mounts a SECOND runtime into this same dialog and + already closed this way; that both runtimes hand the dialog one field + set on open AND reset it one way on close is pinned by + `views/RecordDetailView.confirmRuntimeParity-5835.test.tsx`. + */} { - if (!open) setConfirmState({ open: false, message: '' }); + if (!open) setConfirmState(s => ({ ...s, open: false })); }} /> { if (!open) setParamState({ open: false, params: [] }); diff --git a/packages/app-shell/src/views/RecordDetailView.confirmRuntimeParity-5835.test.tsx b/packages/app-shell/src/views/RecordDetailView.confirmRuntimeParity-5835.test.tsx index b5bdbfcbb6..108cdbc36a 100644 --- a/packages/app-shell/src/views/RecordDetailView.confirmRuntimeParity-5835.test.tsx +++ b/packages/app-shell/src/views/RecordDetailView.confirmRuntimeParity-5835.test.tsx @@ -15,6 +15,12 @@ * - `views/RecordDetailView.tsx` does NOT consume that hook. It builds a second, * near-identical `confirmHandler` and renders its own ``. * + * This file pins BOTH halves of that relationship. The OPEN half is #5835's and + * comes first; the CLOSE half (objectui#6034) is the second `describe` at the + * bottom, with its own header explaining which reset shape won and why. They + * live in ONE file on purpose: two parity pins for one relationship could + * disagree with each other, which is the very defect the two runtimes had. + * * Merging the two is a larger refactor and is deliberately NOT this file's job. * What this file buys instead is the property the duplication threatens: the * dialog is one component with one set of reads, so whichever runtime opened it @@ -85,9 +91,29 @@ vi.mock('sonner', () => ({ * a per-path double could drift exactly the way the runtimes did. */ let dialogState: any = null; + +/** + * The CLOSE path's subject (objectui#6034). `dialogState` above only records + * while `open` is true, so it structurally cannot see what a runtime writes + * when the dialog CLOSES — the exact half these two runtimes disagreed on. + * + * - `dialogStates` records EVERY `state` the dialog is handed, so the object + * that arrives after the close is observable. + * - `closeViaRuntime` is the runtime's OWN `onOpenChange` prop — driving the + * real seam, not a re-spelling of it. A test that called `setConfirmState` + * itself would pin the test's idea of the reset shape, not the runtime's. + */ +let dialogStates: any[] = []; +const NO_DIALOG_RENDERED = () => { + throw new Error('ActionConfirmDialog never rendered — no runtime close seam to drive'); +}; +let closeViaRuntime: (open: boolean) => void = NO_DIALOG_RENDERED; + vi.mock('./ActionConfirmDialog', () => ({ - ActionConfirmDialog: ({ state }: any) => { + ActionConfirmDialog: ({ state, onOpenChange }: any) => { if (state?.open) dialogState = state; + dialogStates.push(state); + closeViaRuntime = onOpenChange; return null; }, })); @@ -244,10 +270,34 @@ async function confirmViaConsoleRuntime(...args: unknown[]) { return dialogState; } +/** + * Close path — hand the runtime its own `onOpenChange(false)` and return the + * `state` object it writes in response (objectui#6034). + * + * The call ORDER mirrors `ActionConfirmDialog.handleCancel` exactly: the dialog + * settles the promise FIRST and only then calls `onOpenChange(false)`. That + * order is the whole reason a `resolve` retained past the close is inert — a + * second call on a settled promise is a no-op — so a close-path pin that + * skipped the settle would be pinning a sequence production never runs. + */ +async function closeFromRuntime(openState: any) { + const before = dialogStates.length; + await act(async () => { + openState.resolve?.(false); + closeViaRuntime(false); + await Promise.resolve(); + }); + const written = dialogStates.slice(before); + expect(written.length).toBeGreaterThan(0); + return written[written.length - 1]; +} + beforeEach(() => { cleanup(); captured.length = 0; dialogState = null; + dialogStates = []; + closeViaRuntime = NO_DIALOG_RENDERED; vi.stubGlobal( 'fetch', vi.fn(async () => @@ -315,3 +365,91 @@ describe('app-shell — both confirm runtimes feed ActionConfirmDialog the same } }); }); + +/** + * ## The CLOSE half (objectui#6034) + * + * The block above is the OPEN half and it is #5835's. Its assertions stay green + * under either reset shape, so within this file they are **controls, not + * evidence** — they establish that both runtimes still reach the dialog at all, + * which is what makes a difference measured after the close attributable to the + * close handler. + * + * The divergence this half pins: `useConsoleActionRuntime` used to close with + * `setConfirmState({ open: false, message: '' })` — replacing the whole object, + * blanking `message` and dropping the `options` / `resolve` keys outright — + * while `RecordDetailView` closed with `setConfirmState(s => ({ ...s, open: + * false }))`, flipping one flag and keeping every field. + * + * **Field-preserving is the shape that won, because of who reads the state + * after the close.** `AlertDialogContent` carries `data-[state=closed]: + * animate-out … duration-200`, so Radix's `Presence` keeps the dialog MOUNTED + * through its exit animation — `ActionConfirmDialog` goes on reading + * `state.message` into `AlertDialogDescription` and `state.options` into the + * title and both button labels for the whole fade-out. Blanking rewrites the + * dialog's visible text mid-fade; preserving fades it out intact. The cost of + * preserving is a settled promise's `resolve` left reachable in state, which is + * inert: the dialog settles it before it ever asks for the close, and the open + * path replaces the entire state object, so nothing stale survives a reopen. + */ +describe('app-shell — both confirm runtimes reset ActionConfirmDialog the same way on CLOSE (objectui#6034)', () => { + it('RecordDetailView: the close flips `open` and keeps every other dialog-read field', async () => { + const opened = await confirmViaRecordDetailView(MESSAGE, BAG); + expect(opened.open).toBe(true); + + const closed = await closeFromRuntime(opened); + expect(closed.open).toBe(false); + // By NAME, not by count: a shape that drops keys is red on the key set, and + // a shape that blanks `message` is red on the value even if the key stays. + expect(Object.keys(closed).sort()).toEqual(DIALOG_READS); + expect(closed.message).toBe(MESSAGE); + expect(closed.options).toEqual(BAG); + expect(typeof closed.resolve).toBe('function'); + }); + + it('useConsoleActionRuntime: the close flips `open` and keeps every other dialog-read field', async () => { + const opened = await confirmViaConsoleRuntime(MESSAGE, BAG); + expect(opened.open).toBe(true); + + const closed = await closeFromRuntime(opened); + expect(closed.open).toBe(false); + expect(Object.keys(closed).sort()).toEqual(DIALOG_READS); + expect(closed.message).toBe(MESSAGE); + expect(closed.options).toEqual(BAG); + expect(typeof closed.resolve).toBe('function'); + }); + + it('the two runtimes write the same post-close state, field for field', async () => { + const viewClosed = await closeFromRuntime(await confirmViaRecordDetailView(MESSAGE, BAG)); + cleanup(); + const hookClosed = await closeFromRuntime(await confirmViaConsoleRuntime(MESSAGE, BAG)); + + expect(Object.keys(viewClosed).sort()).toEqual(Object.keys(hookClosed).sort()); + expect(Object.keys(viewClosed).sort()).toEqual(DIALOG_READS); + expect(viewClosed.open).toBe(hookClosed.open); + expect(viewClosed.message).toBe(hookClosed.message); + expect(viewClosed.options).toEqual(hookClosed.options); + expect(typeof viewClosed.resolve).toBe(typeof hookClosed.resolve); + }); + + it('the close-path fixture is multi-field, so the two reset shapes are distinguishable here', async () => { + // Non-degeneracy guard. With an empty or single-field confirm state, + // "blanked" and "preserved" produce the SAME object and every assertion + // above passes without measuring anything. Applied to THIS fixture, the two + // shapes app-shell actually shipped must disagree — on the key set AND on a + // value — or the pin above is decorative. + const opened = await confirmViaRecordDetailView(MESSAGE, BAG); + const blanked: any = { open: false, message: '' }; + const preserved: any = { ...opened, open: false }; + + expect(Object.keys(blanked).sort()).not.toEqual(Object.keys(preserved).sort()); + expect(blanked).not.toHaveProperty('options'); + expect(blanked).not.toHaveProperty('resolve'); + expect(blanked.message).not.toBe(preserved.message); + expect(preserved.options).toEqual(BAG); + + // …and the fixture itself is what makes those inequalities real. + expect(MESSAGE.length).toBeGreaterThan(0); + expect(Object.keys(BAG).length).toBeGreaterThan(1); + }); +});