Uh oh!
There was an error while loading. Please reload this page.
fix(app-shell): converge both confirm runtimes on one close reset shape - #6432
Merged
os-support-ai merged 1 commit intoAug 26, 2026
Merged
Conversation
`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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011SfZeFWrhGLHmfq61xbz4qContributor
✅ Console Performance Budget
The eager closure is every chunk the entry reaches through static imports — what the browser fetches and parses before the app renders. The entry chunk on its own is a small fraction of it. 📦 Bundle Size Report
Size Limits
|
os-support-ai
marked this pull request as ready for review
August 26, 2026 00:09
Uh oh!
There was an error while loading. Please reload this page.
os-support-ai
deleted the
claude/issue-6034-confirm-runtime-close-parity
branch
August 26, 2026 00:22
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes#6034
app-shellmounts two confirm runtimes into oneActionConfirmDialog. They agreed on open and disagreed on close. This converges the close handlers on one reset shape and extends #5835's existing parity pin to cover the close path.Census, re-derived on
origin/main9602dc820packages/app-shell/src/hooks/useConsoleActionRuntime.tsx:193setConfirmState({ open: true, message, options, resolve }):696setConfirmState({ open: false, message: '' })— blanks the whole statepackages/app-shell/src/views/RecordDetailView.tsx:518— identical shape:2511setConfirmState(s => ({ ...s, open: false }))— preserves fieldsReproduces exactly as filed. #5835's pin covered open only.
Close-path consumer enumeration — every hop, both runtimes
Not inherited; derived here.
confirmStateappears on exactly four lines inpackages/*/src(twouseStatedeclarations, two<ActionConfirmDialog state={confirmState}>reads) and nowhere else, so the consumer chain is short and closed:ActionConfirmDialog(views/ActionConfirmDialog.tsx), one component serving both. It readsstate.open,state.message,state.options?.title,state.options?.confirmText,state.options?.cancelText,state.resolve.state.openreachesAlertDialog;AlertDialogContentcarriesdata-[state=closed]:animate-out … duration-200, soPresencekeeps the subtree mounted through the exit animation and hop 2 keeps reading for the whole fade-out.Mount sites, so the "fix three of four sites" trap can be checked rather than asserted away: the console runtime's dialog is mounted from four places (
console/ConsoleShell.tsx:83,views/ObjectView.tsx:1060,views/DeclaredActionsBar.tsx:463, andviews/PageView.tsxviaConsoleActionRuntimeProvider), plusRecordDetailView's own — five mount sites. None of them touchesconfirmState; each runtime owns its reset in exactly one place, so changing the hook converges four sites at once and the fifth was already the target shape. No third consumer at a second hop.plugin-designer/src/hooks/useConfirmDialog.tsexports a name-collidingConfirmDialogState, but it is an unrelated hook (isOpen/title/message, resolver in a ref) that never feedsActionConfirmDialog. Not a hop.Which shape won, and why: field-preserving
useConsoleActionRuntimenow closes withsetConfirmState(s => ({ ...s, open: false })), matchingRecordDetailView.The decision follows from hop 3. Because Radix holds the content mounted for the ~200 ms fade-out, the state written on close is what the user sees during the close. Blanking rewrote the dialog's visible text mid-fade — the description emptied, and the title and both button labels reverted from the caller's
optionsto their i18n defaults. Preserving fades the dialog out intact. That is the only observable difference in the consumer set, and it points one way.The cost of preserving is a
resolveleft reachable in state after close. It is inert, and provably so from the dialog's own code:ActionConfirmDialog.handleConfirm/handleCancelcallstate.resolve?.(…)beforeonOpenChange(false), so the promise is already settled when the reset runs; a later call on a settled promise is a no-op.{ open: true, message, options, resolve },optionspresent-but-undefinedat the runner arity), so no stale field can leak into a reopen.Secondary, same direction: the functional-updater form is also the safer one under a close/reopen race — a whole-object replacement discards whatever a newly-opened confirm has just written, including its
resolve, which would strand that action's promise forever.No close-path consumer depends on the divergent shape, so nothing needed to be reported and stopped on under that clause, and the divergence is not preserved behind a flag or an option.
The pin: extended, not duplicated
packages/app-shell/src/views/RecordDetailView.confirmRuntimeParity-5835.test.tsxgains a seconddescribefor the close path. #5835's file keeps ownership of this relationship — a parallel pin would leave two pins that can disagree, which is the same defect class as the two runtimes.Against the degenerating-pin trap, the close half:
MESSAGEplus the full{ title, confirmText, cancelText }bag — so "blanked" and "preserved" are different objects;Object.keys(...)against the dialog-read list,message,options,typeof resolve), not by count;onOpenChangeprop, inActionConfirmDialog.handleCancel's exact order (settle the promise, then close), so it pins the runtime's reset rather than the test's idea of it;Verification
All vitest runs from the repo root with root-relative paths (objectui#3378), under the container's shared heavy-verify lock.
Red before the fix — pin written first, source untouched at
9602dc820. Predicted 6 pass / 2 fail, with the console-runtime close test and the cross-runtime parity test red. Observed exactly that:That assertion text is the divergence itself: blanking drops the
optionsandresolvekeys outright.Green after —
Tests 8 passed (8).Ablation, post-commit (fix reverted to the blanked shape, restored by hash): mutation confirmed on disk by grepping the injected and the removed text (
0fixed-shape hits,1blanked-shape hit) and bygit hash-objectdiffering from theHEADblob; re-run went2 failed | 6 passed; restore undertrap … EXIT INT TERMwith absolute paths, verified by the restored blob hash equalling theHEADblob (fb54c32240712ba5355384823c96380ae6936fff) andgit diff HEADempty. Nodistis in this path — the pin imports../hooks/useConsoleActionRuntimerelatively, so vitest loads the mutated source directly and no rebuild leg applies.Every reading below is on final head
1dd499571:Test Files 1 passed (1)/Tests 8 passed (8)Test Files 5 passed (5)/Tests 67 passed (67)RecordDetailView.*.test.tsx(17 files)Test Files 17 passed (17)/Tests 134 passed (134)pnpm --filter @object-ui/app-shell type-check0, script echoed astsc --noEmit && tsc -p tsconfig.test.jsoncheck:control-bytes✅ check-control-bytes: OK (scanned 5313 tracked text file(s))check:vi-mock-specifiers✅ check-vi-mock-specifiers: OKcheck:action-forward-parity0check-changeset-presence.mjs✅ 2 source file(s) of 1 released package(s) changed, and this change declares 1 changeset(s)Type-check coverage was measured, not assumed.
tsc -p tsconfig.json --listFilescontainsuseConsoleActionRuntime.tsxbut not the.test.tsx;tsc -p tsconfig.test.json --listFilescontains both. The package'stype-checkscript runs both programs, so the second half is what puts the edited test file under the compiler. Dependency closure built first (pnpm --filter '@object-ui/app-shell^...' build), sotsc'sdist-resolved workspace deps were current.Lint was narrowed to
packages/app-shell, and the narrowing is measured: (1) the population comes from eslint's own config — the package has no localeslint.config.*, soeslint .resolves the root flat config, which is the same invocationturbo run lintuses for this package; (2)--format jsonreports 975 files inspected, 0 errors, 2684 pre-existing warnings, and both edited files are in that population witherrors=0; (3)eslint.config.jsdeclares noproject/projectService/parserOptions.project, so type-aware linting is off and a change insideapp-shellcannot move the verdict on a file in another package. CI runs the full farm regardless.Changeset
.changeset/confirm-runtime-close-parity-6034.md,patchon@object-ui/app-shell— a real behaviour change on close, stated in words. Notmajor(fixed group).Out of scope, filed
#6431 —
ActionParamDialogcarries the same close divergence between the same two runtimes (useConsoleActionRuntime.tsx:716blanks{ open: false, params: [] };RecordDetailView.tsx:2519preserves). Deliberately not fixed here: this card's fence is the confirm path, and the param call is not the same mechanical one —ParamDialogStatecarriesparamsand in-flight collection state, so blanking on close may be the deliberate choice there rather than the mid-fade defect. That needs a ruling. The third dialog in the same pair,ActionResultDialog, does not diverge (both close withsetResultDialogState({ open: false })), so the drift is two of three.Collision check
packages/app-shellis shared, so: this PR touchessrc/hooks/useConsoleActionRuntime.tsxandsrc/views/RecordDetailView.confirmRuntimeParity-5835.test.tsxonly. Open draft PR #6283 in the same package is onsrc/views/metadata-admin/previews/block-types.ts— no file-level overlap.⛔ Left as draft for the PM to land — not marked ready, not self-merged, no auto-merge.
Generated by Claude Code