Uh oh!
There was an error while loading. Please reload this page.
fix(app-shell): converge the third ActionParamDialog consumer on the field-preserving close - #6586
Conversation
…field-preserving close
`MetadataTypeActions` is the third consumer of `ActionParamDialog` in this
package, and the last one still writing `setParamState({ open: false, params:
[] })` on close. `DialogContent` carries `duration-200
data-[state=closed]:animate-out`, so Radix holds the content mounted through the
exit animation and the dialog renders off `state` for the whole fade-out: the
closing dialog re-titled itself from the action's own label to the generic
`actionDialog.title` and dropped every param row while it faded.
Close now flips `open` and keeps every other field, the shape
`useConsoleActionRuntime` and `RecordDetailView` converged on in #6431.
The pre-reset `paramState.resolve?.(null)` is dropped, on an enumeration rather
than on "resolving twice is a no-op": `onOpenChange` is reachable from exactly
three places, all inside `ActionParamDialog`, and each settles the promise
first — `handleSubmit`, `handleCancel`, and the Radix root handler that
delegates to `handleCancel` (the route Escape, an overlay click and the header X
all take). The new test drives all four routes and asserts the settle precedes
the callback, plus a census over `ActionParamDialog.tsx` so a fifth, unsettled
call site added later is red there rather than a promise pending forever.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011SfZeFWrhGLHmfq61xbz4qCo-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011SfZeFWrhGLHmfq61xbz4q
✅ 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
commented
Aug 26, 2026
ACCEPT on the diff at
The ⛔ exclusion held
The fix is at The sub-decision was decided the way the order required, and then improved on itThe order forbade "resolving twice is a no-op" as a reason and demanded an enumeration. It got one — every route into this Two things make this better than the enumeration I asked for:
Two corrections to this seat's order, both accepted(a) The card's line numbers, and this seat's praise of them. They were byte-exact for the target file ( <ActionParamDialogstate={paramState}onOpenChange={(open)=>{if(!open)setParamState(s=>({ ...s,open: false}));}}/>That is the convergence claim proved directly rather than argued: the third consumer now has the shape the second already had, byte-for-byte in the part that matters. (b) The acceptance bar named a field this consumer never supplies. It asked that the closing dialog keep "title, description and param rows". Measurement discipline worth namingThe eager-closure budget gauge is reported NOT MEASURED rather than green or red, quoting its own words: "No eager-closure report … so no chunk was weighed … this is a broken gauge, not 3 budgets that all passed." Prerequisite-not-met is neither pass nor fail, and it is not implicated by a diff that changes no imports. The test narrowing is declared and, more importantly, measured rather than guessed: the full 558-file package run hit the container's ~10-minute foreground cap ( Ablation: fix committed first, handler reverted in place, mutation proved on disk by anchored counts and blob hash ( Generated by Claude Code |
Uh oh!
There was an error while loading. Please reload this page.
Fixes#6473
packages/app-shell/src/views/metadata-admin/MetadataTypeActions.tsxis the third consumer ofActionParamDialogin this package, and after #6431 it was the only one still replacing the whole state object on close. Close now flipsopenand keeps every other field — the shapehooks/useConsoleActionRuntime.tsxandviews/RecordDetailView.tsxalready carry.DialogContentcarriesduration-200 data-[state=closed]:animate-out, so Radix holds the content mounted through its exit animation and the dialog goes on rendering offstatefor the whole fade-out. Under{ open: false, params: [] }the closing dialog re-titled itself from the action's own label to the genericactionDialog.titleand dropped every param row while it faded.Premise re-measured on
main@12402a9b8The card's line numbers were exact for the target file and stale for one sibling — re-measured before the first edit:
hooks/useConsoleActionRuntime.tsx:264:747-749(card said:748)views/RecordDetailView.tsx:562:2541-2546(card said:2519— now theHighlightFieldsProviderclose tag)views/metadata-admin/MetadataTypeActions.tsx:115:240-248(card said:245, the innersetParamState):97,:115and:245were byte-exact as the card and the dispatch claimed.One correction to the order. Acceptance says the closing dialog keeps its "title, description and param rows".
titleandparamsare real here —run()opens withaction.label ?? action.name.descriptionis not: this consumer never puts one inparamState, so the dialog shows the genericactionDialog.descriptionunder both shapes. It is not part of what the fix recovers, and the tests say so explicitly rather than letting a reader infer a parity that is not there.The
resolve?.(null)sub-decision: dropped, on an enumerationThe old close settled the promise itself before resetting. That line is gone — not because "resolving twice is a no-op", but because every path into this callback already settles.
onOpenChangeis reachable from exactly three places, all insideActionParamDialog(read-only for this card, unchanged):handleSubmit—state.resolve?.(serializeParamValues(...)), thenonOpenChange(false)(ActionParamDialog.tsx:280-281).handleCancel—state.resolve?.(null), thenonOpenChange(false)(:285-286).onOpenChange(:295-297), which delegates tohandleCancel. This is the single route taken by Escape, an overlay/outside click, and the header X button (DialogContentrenders aDialogPrimitive.Close, which flips the root).The host itself never calls the prop, and
setParamStateappears in only two places in the file (open at:115, close in the JSX). So no path arrives unsettled, and the removed line was a second settle on an already-settled promise — on the submit path it was also anullsettle over a values settle, i.e. the wrong intent had it ever been load-bearing.The enumeration is executable rather than asserted: the new test drives all four routes through the real component and asserts
resolveprecedesonOpenChange(false)in each, plus a census overActionParamDialog.tsx(exactly two directonOpenChange(calls, each preceded by a settle; exactly oneonOpenChange={prop, delegating tohandleCancel). A fifth call site added later without a settle is red there instead of leaving a promise pending forever.Test —
MetadataTypeActions.paramDialogClose-6473.test.tsx(11 cases)Modelled on
views/RecordDetailView.paramRuntimeParity-6431.test.tsx: it wraps the realActionParamDialograther than stubbing it, so the DOM half renders the genuine Radix presence machinery while the state half still sees every object handed to it. happy-dom has no CSS engine, soanimationNameis derived from the node's livedata-state— the conditionPresenceactually tests — and the model's premise is read off the shippedDialogContentrather than assumed.data-state="closed". A test that only checked the end state is green under both shapes and is not coverage.reasonblank,regionback tous-east) after the user typed over both. Those values live in the dialog's ownvalues, reseeded on everystate.openfalse→true edge — preservingparamson close must not make a reopen inherit the previous run.Cancel/Confirm/ the generic title) are measured, not hard-coded: the light DOM setup mounts no i18n provider, sot()returns raw keys. The file is red on the close handler, never on the test setup.Ablation — shown able to fail
The fix was committed first, then the handler was reverted to the pre-fix shape in place. Mutation proved on disk before the run (
grep -con both the injected and the removed text:injected=1 removed_remaining=0, blob8d6f21f9→291c0bc4), restore proved after (blob back to8d6f21f9,git diff HEADempty), with anEXIT INT TERMtrap on absolute paths. No rebuild leg:vitest.config.mtsaliases every@object-ui/*to the sibling package'ssrc/, and the subject module is imported from source, so nodistis in the loop.Predicted direction and observed direction agree: the two shape pins go red; the control, the four settle-order routes, the census, the exit-window premise and the blanking-shape evidence stay green (none of them read the host handler).
Gates — verdicts as each gate reported them, on the final commit
c3ec4a37cEvery heavy command ran through the container's shared verify lock; the verdict quoted is the gate's own line, not a bare
$?.pnpm --filter @object-ui/app-shell type-check(tsc --noEmit && tsc -p tsconfig.test.json)VERDICT command-exit 0tsc -p tsconfig.test.json --listFilespnpm --filter @object-ui/app-shell lint(eslint ., whole package, no narrowing)✖ 2761 problems (0 errors, 2761 warnings),VERDICT command-exit 0. The new file's 15 warnings are all@typescript-eslint/no-explicit-any, the same rule and shape as its siblings;lint.ymlsets no--max-warningspnpm exec vitest run <17 files> --maxWorkers=2Test Files 17 passed (17),Tests 175 passed (175),VERDICT command-exit 0node scripts/check-changeset-presence.mjs✅ 2 source file(s) of 1 released package(s) changed, and this change declares 1 changeset(s)check-changeset-*.mjs✅eachnode scripts/check-control-bytes.mjs✅ OK (scanned 5436 tracked text file(s); skipped 85 binary)node scripts/check-vi-mock-specifiers.mjs✅ OK (… 724 relative specifier(s) resolved …)— the new file adds fourvi.mockcallscheck-lint-coverage.mjs,check-type-check-coverage.mjs✅ 46/46,✅ 45/46 + 41/41 test projectsDeclared narrowing on the test run, and only there.
pnpm exec vitest run packages/app-shell/(558 files) was attempted and hit the container's ~10-minute foreground cap (exit 143) mid-run, so the run was narrowed to a measured radius rather than a guessed one: a transitive-importer closure overpackages/app-shell/srcseeded at the changed module (23 modules, 15 of them test files — the only tests that can observe a change to it), plus two declared extras that guard the shared dialog without importing the changed file (RecordDetailView.paramRuntimeParity-6431.test.tsx,useConsoleActionRuntime.test.tsx). 17 files, 175 tests, green. CI runs the full farm regardless.One gate not measured, stated as such:
check-eager-closure-budget.mjsexits 2 locally with "No eager-closure report atapps/console/dist/eager-closure.json, so no chunk was weighed … this is a broken gauge" — a missing prerequisite (it needs a console build), not a failure, and not implicated by this diff, which changes no source imports.Generated by Claude Code
Generated by Claude Code