Uh oh!
There was an error while loading. Please reload this page.
fix(app-shell): converge both action runtimes on a field-preserving ActionParamDialog close - #6476
Merged
Conversation
…ctionParamDialog close
`useConsoleActionRuntime` closed the param dialog with
`setParamState({ open: false, params: [] })` — replacing the whole state,
emptying `params` and dropping `title` / `description` / `resolve` — while
`RecordDetailView`, which mounts a second runtime into the same dialog, closed
with `setParamState(s => ({ ...s, open: false }))`.
Radix holds `DialogContent` mounted through its 200ms exit animation, so the
dialog renders off `state` for the whole fade-out: under the blanking shape a
just-filled params form re-titled itself to the generic `actionDialog.title`,
swapped in the generic description and dropped every param row. The console
runtime moves onto the field-preserving shape.
Re-measured on this dialog rather than inherited from the confirm pair
(objectui#6034): a params form could plausibly want its values dropped, but the
typed values are not in `paramState` at all — they live in the dialog's own
`values`, reseeded on every open — so a reopen starts blank either way and
nothing a user can observe outlives the fade.
Part of #6431Contributor
✅ 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
|
This was referenced Aug 26, 2026
os-support-ai
marked this pull request as ready for review
August 26, 2026 02:56
Uh oh!
There was an error while loading. Please reload this page.
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#6431
app-shellmounts two action runtimes into two copies ofActionParamDialog. They agreed on open and disagreed on close:useConsoleActionRuntimewrotesetParamState({ open: false, params: [] })— replacing the whole state object, emptyingparamsand droppingtitle/description/resolve— whileRecordDetailView, which re-implements the runtime rather than consuming the hook, wrotesetParamState(s => ({ ...s, open: false })).The behavioural diff is one line: the console runtime moves onto the field-preserving shape. Everything else in this PR is the measurement that chose it, and the pin that keeps the two from drifting apart again.
The measurement decided the shape — it was not inherited from the confirm pair
The triage ruling was explicit that #6034's answer transfers in form but not automatically in substance, and it named the two readings that had to be taken first. Both were taken, against the real dialog.
Reading 1 — what
ActionParamDialogrenders offstateafteropenflips false.DialogContentcarriesduration-200 data-[state=closed]:animate-out, so Radix keeps the content mounted through its exit animation and the dialog goes on rendering offstatefor the whole ~200ms. Driving the real dialog across that window:Create environmentEnvironment name*,RegionactionDialog.titleCreate environmentEnvironment name*,RegionA params form the user had just filled in re-titles itself to the generic label, swaps the action's description for the generic one, and empties out — strictly more visible damage than the confirm case, which only blanked one line of text.
Reading 2 — whether anything depends on
paramsNOT surviving the close. Nothing does, and the intuition that a params form wants its values dropped does not survive contact with the code: the typed values are not inparamStateat all. They live inActionParamDialog's ownvaluesstate, which itsuseEffectreseeds from the param defaults on everystate.openfalse→true edge. A full type → close → reopen round trip was run under both shapes; the reopened input is''either way. Measured, not reasoned:reopens blank under either reset shape — so no user-visible difference outlives the fade.So no product-semantics question emerged — nothing a user can observe beyond the fade-out frame differs between the two shapes — and this stayed a convergence rather than forking to a decision card. Field-preserving wins on this dialog's own evidence, which happens to agree with #6034.
The reason is recorded at the divergence site, per the ruling: the full rationale sits at the console runtime's close handler, with a pointer at
RecordDetailView's so the line there is not "cleaned up" later.ActionResultDialog, the non-divergence the card flagged as a control, is untouched.The instrument, and why it is not blind
jsdom has no CSS engine, so
getComputedStyle(node).animationNameis always'none', Radix'sPresenceunmounts synchronously, and the exit window does not exist unless it is modelled — a test that skipped this would assert on an unmounted tree and read as green for the wrong reason.animationNameis therefore derived from the node's own livedata-state, which is exactly what the two real classes do. That detail is load-bearing:Presencesuspends the unmount only when the mount-time and close-time names differ, so a constant stub silently fails to open the window (it did, on the first attempt — the dialog unmounted and every post-close assertion had nothing to read).The model's premise is measured rather than assumed:
the exit-animation window is real in production, not just modelled herereadsdata-[state=closed]:animate-out,data-[state=open]:animate-inandduration-200off the shippedDialogContent. If those classes ever go, that test fails before the pins start lying.Both runtimes are driven through their own real seams — the runtime's own
onOpenChangeprop, in the call orderhandleCancelactually uses (settle the promise, then ask for the close), so the retainedresolveis pinned as inert the way production makes it inert.Reverse verification
Predicted before the run, on the committed fix: reverting the console runtime's close handler should turn exactly three tests red — the DOM pin, the console state pin, and the cross-runtime equality — and leave the six controls green.
Mutation proven on disk in both directions (occurrence counts of the injected and the deleted text, plus the line read back off disk), never a diffstat and never an editor exit code:
Result, as predicted:
Restore proven byte-for-byte, not by an exit code — worktree blob
9f13fcfa…identical to theHEADblob,git diff HEADempty, counts back toblank=0 / preserving=1— under anEXIT/INT/TERMtrap holding absolute paths. The suite then returned to 9/9 green on the restored tree.Pins that are green in both directions, named rather than left to look load-bearing
Four of the nine do not distinguish the two shapes, and each is here for a stated reason:
RecordDetailView(×2) — that runtime was already correct, so these cannot be evidence for the ruling. They make a failure on the console side attributable: dialog, model and harness all behave for a runtime that was never broken, so a red above is the close handler and not the rig.the blanking shape … empties and re-titles the fading dialog— the negative half of the measurement, kept executable rather than written down. Green on either side of the fix by construction; it fails the dayActionParamDialogstops reading one of the fields the pin above targets, at which point that pin would be guarding nothing.the close-path fixture is multi-field— the anti-degeneracy guard itself. With a single-field param state the two shapes produce the same object and every parity assertion passes without measuring anything; this applies that requirement to this fixture.The ghost-assertion shape ("params were cleared" passing because nothing ever rendered) is closed by targeting the specific post-close state and only reaching it after
await screen.findByLabelText(/Environment name/)confirms a real render — a zero-render dialog times out there instead of sailing through.Verification
Local list, at final commit
fc9eb5558(worktree clean at the time of the run):Test Files 1 passed (1)/Tests 9 passed (9)Test Files 8 passed (8)/Tests 133 passed (133)@object-ui/app-shelltype-check0error TSlinescheck:vi-mock-specifiers✅ check-vi-mock-specifiers: OK (… 694 relative specifier(s) resolved …)check:control-bytes✅ check-control-bytes: OK (scanned 5346 tracked text file(s); skipped 85 binary)✅ 3 source file(s) … declares 1 changeset(s),✅ All workspace packages are in the changeset fixed group,✅ No changeset declares a major bumpTwo notes on how those were read:
type-checkthat excludes**/*.test.tsis a true statement about nothing;tsc -p tsconfig.test.json --listFilesputsparamRuntimeParity-6431.test.tsxin the program (3240 files total). The first typecheck run failed withTS2307: Cannot find module '@object-ui/fields'— the unbuilt dependency closure in a fresh worktree, not this diff;pnpm --workspace-concurrency=2 --filter '@object-ui/app-shell^...' build(exit 0) then took it to exit 0.eslint . --format jsonwas run over the wholeapp-shellpackage: 976 files selected by eslint's own config, 0 errors, 2716 warnings (pre-existing;no-explicit-anyandreact-refresh/only-export-componentsare warn-level across this package). The three touched files carry 0 errors; the new test file's 32 warnings are allno-explicit-any, matching the siblingconfirmRuntimeParity-5835.test.tsxstyle verbatim. Nothing outside that package can move:eslint.config.jsusestseslint.configs.recommendedwith noparserOptions.project/projectService, and no rule ineslint-rules/reads the filesystem — every rule is single-file-scoped, so a diff confined toapp-shellcannot change the verdict on a file it did not touch.Public surface
No public surface change. No
**/src/index.tsis touched, the diff adds and removes noexportline, and no published type gains a member — the behavioural change is one call site inside a component body. Stated explicitly because it is the answer the contract-review tier turns on.Out of scope, filed not fixed
There is a third
ActionParamDialogconsumer the card did not know about:views/metadata-admin/MetadataTypeActions.tsxholds its ownparamStateand still closes with the blanking shape. It is outside this card's declared file face, and converging it is not the same mechanical call — its close handler also callsparamState.resolve?.(null)itself, a second settle on an already-settled promise, and whether that line stays is a decision this card's measurement does not imply. Filed unassigned as #6473 with the evidence; deliberately not touched here.Generated by Claude Code