fix(studio): let a failed DOM edit report that it failed - #3510
Conversation
`runDomEditCommit` catches a persist failure, reverts, fires `onError` and then resolves. That contract is deliberate and its docstring says so: the human path learns the write failed from the toast `onError` puts on screen, so a rejection would be redundant. It also means a caller awaiting `handleDomTextCommit` or `handleDomStyleCommit` cannot tell a landed write from a reverted one, because both resolve with `undefined`. The runner already offers `onSettled` as the way out. Text and style were the two commits that never got it wired. Add `runReportedDomEditCommit`, which owns `onSettled` (forwarding to a caller-supplied one rather than dropping it) and returns whether the write landed. Both handlers now return a tagged outcome, so the three preconditions that previously returned early and silently are each distinguishable: no selection, a manual-geometry property the style path refuses, and a selection that cannot edit styles. Same for text: no selection versus not text-editable. Human-facing behaviour is unchanged and the tests assert that: the toast still fires and the optimistic DOM change is still reverted. The callback props that carry these handlers ignore the result, so their declared type widens from `Promise<void>` to `Promise<unknown>`. That type is hand-copied in fourteen places; consolidating it is worth its own change. `useDomEditTextCommits.ts` is now 593 lines against the 600-line cap. The next change to it needs a split.
Two more commits that could not tell a caller they had failed. `useDomEditPositionPatchCommit` swallowed `DomEditSaveQueueOpenError` and resolved. The intent was right, a paused save queue already puts a banner on screen and one toast per blocked edit is noise, but swallowing it also skipped the caller's revert: `useDomGeometryCommits` only restores the optimistic offset, size or rotation from its `.catch`. So once the breaker opened, a drag left the element where the user dropped it while nothing reached the file, and the next reload snapped it back. It now rejects without toasting. The banner still does the telling; the caller gets to revert. `handleDomEditElementsDelete` caught everything and only toasted, so an unpatchable target and a completed delete were indistinguishable to a caller. It now returns an outcome, with `no-project` and `no-selection` separated from a failed write rather than all three sharing an early `return`. Adds the first test for `useDomEditPositionPatchCommit`, covering the paused queue, an ordinary failure, and success.
vanceingalls
left a comment
There was a problem hiding this comment.
Wires a caller-visible outcome onto handleDomTextCommit, handleDomStyleCommit, handleDomEditElementsDelete, and lets useDomEditPositionPatchCommit reject on a paused save queue so the caller's optimistic revert fires. Contract preservation looks solid: runDomEditCommit's resolve-always shape is unchanged; runReportedDomEditCommit layers reporting on top via onSettled, forwarding a caller-supplied callback rather than clobbering it. Human-facing behaviour is asserted preserved (toast fires, DOM reverts). Overall verdict: FINDINGS (P2/nits) — COMMENT pending stamp, no blockers. CI is all green at HEAD 6cbbac02.
Standards checklist
- (a) Types/typecheck: clean per PR;
Promise<void>→Promise<unknown>widening propagated to 14 sites; consolidation deferred by note. - (b) Tests: semantic assertions (
toEqual({ ok: false, reason: 'persist-failed' }), toast-fired, DOM-reverted). NewuseDomEditPositionPatchCommit.test.tsxcovers paused-queue, ordinary-fail, success. - (c) Edge cases: covered for text/style (no-selection, geometry-property, styles-not-editable, not-text-editable, persist-failed). Delete outcome shape is under-covered (see F1).
- (d) Contract invariants:
runDomEditCommitresolve-always preserved;onSettledfires exactly once per docstring;runReportedDomEditCommitrespects that. - (e) Telemetry:
trackStudioSaveFailurestill fires for ordinary position-patch failures; deliberately skipped on paused-queue (banner-only path). No change in classification. - (f) i18n: no user-facing strings touched.
Editor-UI parity lens (Studio)
- Mid-drag safety: geometry-commit
.catchinuseDomGeometryCommitsstill restores optimistic offset/size/rotation and rethrows — now the paused-queue path actually reaches it. Verified. - Selection state after mutation: delete-failure path preserves selection (unchanged from pre-diff).
- Error-swallowing: the failing test would have caught it —
handleDomStyleCommit/handleDomTextCommitreporting is asserted on the persist-failure path withoutcome.reason === 'persist-failed', not just presence.handleDomEditElementsDeleteoutcome asserted only via type, not test.
Findings
P2 — handleDomEditElementsDelete SDK-success path returns undefined, breaking the outcome contract this PR just introduced. packages/studio/src/hooks/useElementLifecycleOps.ts:145 bare-returns return; inside if (allHandled) { ... } while :212 returns { ok: true } as const for the REST-success path and :218 returns domEditCommitDeclined("persist-failed") for the catch. Callers checking outcome.ok === true see undefined for every SDK-cutover delete. Existing tests (useElementLifecycleOps.multiDelete.test.tsx) don't assert the return shape, so this passes CI silently. Suggest return { ok: true } as const; on line 145 and one test that asserts the SDK-success return shape.
P2 — Same bug class the PR fixes still lives in handleDomManualEditsReset. packages/studio/src/hooks/useDomGeometryCommits.ts (the reset handler around the manual-edits-reset callback) does void commitPositionPatchToHtml(...).catch(() => undefined); after already calling clearStudioPathOffset/BoxSize/Rotation on the live element. Under a paused save queue this now rejects (post-PR) but the caller swallows it — the preview stays cleared while nothing reached the file, next reload snaps the manual edits back. Same shape as the position-patch bug the PR describes in its Why. Out of the PR's stated scope but worth a follow-up ticket or a scope-widening line in the body.
Nit — console.error("rotate commit failed", …) / "resize commit failed" in useDomEditOverlayGestures.ts:411,495 now fire for DomEditSaveQueueOpenError too. Before this PR the swallow ate the queue-open error before the geometry commit's .catch ever saw it. Post-PR it propagates through and hits these console.errors, which is a wait condition, not an error. Consider if (!(error instanceof DomEditSaveQueueOpenError)) before logging — otherwise the paused-queue banner event ships a spurious console.error per blocked gesture. Small analytics/log-noise concern only.
Nit — Tagged-outcome docstring on DomEditCommitDeclineReason doesn't mention that persist-failed also covers capture/apply throwing before persist is reached. runReportedDomEditCommit initializes landed = false and only flips true from onSettled(true); if capture()/apply() throws synchronously, the exception propagates and runReportedDomEditCommit rejects rather than returning { ok: false, reason: 'persist-failed' }. Fine — matches runDomEditCommit's pre-existing behaviour — but the "resolves on persist failure by design" note conflates the two paths. Optional tightening.
— Via
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
🟠 Second-independent read on the delete-outcome contract gap — same P2 landing point as Via's review 5036548919. Two-reviewer convergence on the shape-consistency blocker; three additional differentiated concerns below. HEAD 6cbbac0.
Endorse (co-witness with Via)
Blocker — useElementLifecycleOps.ts:145 bare return; on the SDK-happy path breaks the outcome contract this PR establishes. Verified at HEAD: if (allHandled) short-circuits with showToast(...); return; while :212 returns { ok: true } as const on the REST-success path and :218 returns domEditCommitDeclined("persist-failed") in the catch. Callers checking outcome?.ok === true see undefined on every SDK-cutover delete — indistinguishable from {ok: false, reason: "persist-failed"} at truthy-check and untagged at the type level. Fix: return { ok: true } as const; on :145.
Two more of Via's findings I also traced and read the same way:
handleDomManualEditsResetresidual (useDomGeometryCommits.ts) — same bug-class,void commitPositionPatchToHtml(...).catch(() => undefined);after clearing offset/box/rotation on the live element; post-PR the paused-queue reject becomes a real signal that gets swallowed. Adjacent to this PR's stated scope; worth a follow-up ticket or scope-widen line.console.errorblast-radius nit atuseDomEditOverlayGestures.ts:411,495— aDomEditSaveQueueOpenErrornow reaches the log site as a spurious error per blocked gesture.if (!(error instanceof DomEditSaveQueueOpenError))guard is the right shape.
Additional findings (not in Via's review)
-
No test coverage for
handleDomEditElementsDelete's reporting channel. The PR body claims "reports instead of only toasting" for delete, but the new test file adds zero delete-outcome assertions. Given the blocker above, a "reports a successful SDK-path delete" test would have caught the shape drift. Add tests mirroring the text/style set — SDK-success, REST-success,no-project,no-selection,persist-failedfor both HTTP-non-ok and the "Nothing to delete" stale-preview throw. Adds ~50 lines but pins the contract on the biggest surface. -
"Nothing to delete — the preview was out of date"is bucketed aspersist-failed. In the REST path,removeData.changed === falsethrows a plainErrorthat the catch treats as persist-failed. Semantically it'spreview-stale— a programmatic caller retrying onpersist-failedwill spin, whereas apreview-stalereason signals "refresh first, then retry." Marginal today (no consumers), real once #3511's agent tools consume outcomes. -
runReportedDomEditCommitinferslandedviaonSettledflag (domEditCommitRunner.ts:97-99). If a future refactor ofrunDomEditCommitgrows a failure mode that bypassesonSettled(e.g. throws before the try/catch), the wrapper silently reports{ok: false, reason: "persist-failed"}— right shape, wrong reason. Not currently reachable, brittle. Wrapping the innerawait runDomEditCommit(...)in try/catch and rethrowing would make the invariant explicit.
What I didn't verify
- Whether any callsite up the chain from
useDomGeometryCommits's.catch(which now sees a rethrownDomEditSaveQueueOpenErrorwhere it used to see silence) has a further.catchthat swallows unknown error classes silently — the new rejection has to terminate somewhere. - Windows CI job passed (14m7s), but I did not scan the specific test files it hit to confirm the new position-patch test actually ran on Windows — shard names collapsed in
gh pr checks. - Whether #3511's
studio_look(the outcome consumer up the stack) actually readsoutcome.okyet — that wiring may live further in the 10-PR stack.
— Review by Rames D Jusso
miguel-heygen
left a comment
There was a problem hiding this comment.
Resolution for the #3510 findings.
SDK success returns undefined.
Fixed in 2278c11d4. useElementLifecycleOps.ts:138-145 now returns { ok: true }, with the SDK witness at useElementLifecycleOps.multiDelete.test.tsx:92-105.
Manual reset swallows a failed save.
Fixed in 2278c11d4. useDomGeometryCommits.ts:130-154 captures all three geometry states, restores them on rejection, and rethrows through the existing reporting path.
Save-queue waits now log as ordinary errors.
Fixed in 2278c11d4. useDomEditOverlayGestures.ts:61-65 excludes DomEditSaveQueueOpenError; anchoredResizeCommitFeedsOffset.test.ts:230-246 proves the wait is quiet while real failures still log.
Delete outcomes are untested, and stale preview is reported as persist-failed.
Fixed in 2278c11d4 and 2a0a034dd. Delete coverage is at useElementLifecycleOps.multiDelete.test.tsx:80-148; useElementLifecycleOps.ts:172-179 now returns preview-stale directly and uses the corrected toast copy.
The landed inference is brittle if a future failure bypasses onSettled.
This does not reproduce in the current contract. domEditCommitRunner.ts:40-52 runs capture and apply before the persist try, so those bugs reject through the wrapper. The only handled failure is persist, and both persist outcomes call onSettled. Lines 62-65 now state that boundary explicitly.
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
🟢 #3510 R2 verdict — cleared by shape at 2a0a034d. Every R1 concern (mine + Via's) verified fixed at the file:line Miguel cited; no new hazards introduced.
R1 concerns — verified fixed by shape:
- P2 blocker (bare
return;on SDK-happy delete path): PASS.useElementLifecycleOps.ts:145now returns{ ok: true } as const;. Contract shape now matches:213(REST-success) and:219(catch →persist-failed). - Delete-outcome test coverage: PASS.
useElementLifecycleOps.multiDelete.test.tsxnow pins the full matrix — SDK-success (:92-105, "reports a successful SDK delete as landed"), REST-success (:89),no-project/no-selection(:108-122),persist-failedon HTTP 500 (:124-132),preview-stale(:134-149). Would catch the blocker on regression. - "Nothing to delete" →
persist-failedmisclassification: PASS.useElementLifecycleOps.ts:172-179now returnsdomEditCommitDeclined("preview-stale")directly instead of throwing into the catch.preview-staleadded to the union atdomEditCommitRunner.ts:75. A programmatic caller retrying on persist-failed no longer spins on a stale preview. runReportedDomEditCommitlanded-via-onSettled brittleness: CLEARED by mechanism, not paved-over. Miguel's rebuttal holds —capture()/apply()run before thetryatdomEditCommitRunner.ts:40-42, so their throws propagate throughawait runDomEditCommit(...)inrunReportedDomEditCommit, which means the{ok: false, reason: "persist-failed"}return is genuinely gated on persist-only. The:62-65docstring now states this: "Capture and apply bugs still reject." Right-shape-right-reason invariant is now explicit. Not a carry-forward ticket.- Via's residual —
handleDomManualEditsResetrollback: PASS.useDomGeometryCommits.ts:130-154capturesbeforeOffset/beforeSize/beforeRotationup front, restores all three in.catch, then rethrows. Test atuseDomGeometryCommits.test.tsx:55asserts rollback on rejection. Return type widenedvoid → Promise<void>atuseDomEditWiring.ts:110anduseGsapSelectionHandlers.ts:114; the sole invocation atuseGsapSelectionHandlers.ts:233doesvoid handleDomManualEditsReset(...).catch(() => undefined)— intentional UI-boundary swallow, documented inline, safe because rollback lives inside and telemetry lives on the position commit. - Via's residual — save-queue
console.errorblast: PASS. New helperlogGestureCommitFailureatuseDomEditOverlayGestures.ts:63-66skipsDomEditSaveQueueOpenError; both call sites (rotate:418, resize:502) use it. Test atanchoredResizeCommitFeedsOffset.test.ts:229-247proves both directions — paused-queue is quiet, ordinary failures still log with("resize commit failed", failure).
What I didn't verify:
- CI is not yet fully green at HEAD —
Analyze/Build/Lint/Format/Typecheck/CodeQL/SDK unit+contract+smoke/Preflight/Studio: load smoke/Perf: *all pass, butregression-shards(shards 2/3/5/6/7/8),Tests on windows-latest,CLI smoke (required),Test,Smoke: global installare stillpending. One top-levelregressionwrapper showsfailat 4s — reads as a meta-check reflecting the still-pending shards rather than a real regression.mergeStateStatus=BLOCKED,mergeable=MERGEABLE. - Whether any additional consumer of the widened
Promise<unknown>outcome (14 hand-copied call-prop sites Miguel flagged as consolidation-worthy) actually readsoutcome.okyet — that wiring lives up the stack in #3511+ and is out of R2 scope.
Peer state: Via's R2 review 5036548919 (COMMENTED, 2026-08-27 01:46) was posted at pre-fix SHA 6cbbac0 — she flagged the SAME P2 blocker independently and raised the manual-reset + save-queue log concerns. Miguel's R2 push at 2a0a034d addresses her three findings AND my three differentiated findings in a single pass. Via has not yet re-reviewed at R2 HEAD.
Stack hygiene note (not a #3510 blocker): #3511's baseRefOid still points at 6cbbac02 (R1 HEAD of #3510) rather than R2 HEAD 2a0a034d, so #3511 hasn't been restacked onto Miguel's fixes yet. When it does restack, it'll pick up the new outcome-shape returns (delete-outcome + preview-stale union member) — worth remembering for the #3511 gate. #3511 + #3514 currently both CLEAN.
— Review by Rames D Jusso
vanceingalls
left a comment
There was a problem hiding this comment.
R2 delta re-verify (HEAD 2a0a034d)
Prior review head was 6cbbac02. All four claims verified by shape against the diff.
R1 findings — delta
-
P2 #1 — outcome-shape inconsistency in
handleDomEditElementsDelete: fixed.packages/studio/src/hooks/useElementLifecycleOps.ts:145— SDK-cutover success path nowreturn { ok: true } as constinstead of barereturn;.useElementLifecycleOps.ts:175— stale-preview path nowshowToast(...); return domEditCommitDeclined("preview-stale")in place of thethrow. Human-visible toast preserved; the outercatchis no longer the messenger for this case, which is what let the pointer close.useElementLifecycleOps.ts:210/:216— HTTP success returns{ ok: true } as const; catch returnsdomEditCommitDeclined("persist-failed"). New testsuseElementLifecycleOps.multiDelete.test.tsx:1051-1091assert the exact tagged shapes across SDK-commit / no-project / no-selection / persist-fail / stale-preview.
-
P2 #2 — same swallow-bug class in
useDomGeometryCommits.tshandleDomManualEditsReset: fixed.packages/studio/src/hooks/useDomGeometryCommits.ts:130-152— signature now returnsPromise<void>; the.catchrestores capturedbeforeOffset/beforeSize/beforeRotationvia therestoreStudio*helpers and rethrows.- Interface propagation checked:
useDomEditWiring.ts:110anduseGsapSelectionHandlers.ts:114both updated toPromise<void>. The only actual invocation site —useGsapSelectionHandlers.ts:233— usesvoid handleDomManualEditsReset(sel).catch(() => undefined)as the fire-and-forget UI boundary, with a comment explaining that position-commit already owns the user report and the reset owns rollback. useDomGeometryCommits.test.tsx:55extends the existing rollback assertion to cover the reset path.
-
Nit —
console.errorspam onDomEditSaveQueueOpenError: fixed.packages/studio/src/hooks/useDomEditPositionPatchCommit.ts:38-46— queue-error branch now rethrows without toasting or callingtrackStudioSaveFailure; the toast + telemetry stay scoped to the non-queue branch.useDomEditOverlayGestures.ts:61-66— newlogGestureCommitFailurehelper filtersDomEditSaveQueueOpenErrorfrom both therotate commit failedandresize commit failedpaths (:415and:499). TestsanchoredResizeCommitFeedsOffset.test.ts:113-130assert both: no spam on paused queue, ordinary failures still log.
Adjacent-defect check (6 axes at the fix boundary)
- Types: the outcome plumbing widened
onSetStyle/onCommitreturn types tovoid | Promise<unknown>acrosspropertyPanelCommitField.tsx:24, the flat panelpropertyPanelFlat*files,useInspectorGestureTransaction.ts:15,125, andDomEditSelectionChrome.tsx:126. This is a widening — existingPromise<void>callers still satisfy it. No caller I could find destructures the outcome. - Callers:
handleDomManualEditsResetgrep at HEAD returns one true invocation site (already audited above); the other hits are interface plumbing only. - Tests: outcomes are asserted by structural equality (
toEqual({ ok: false, reason: "..." })), so a future silent mutation of the reason string will fail the test rather than pass on a truthy shape. - Error paths: capture / apply throws still propagate as rejections from
runReportedDomEditCommit— documented in theDomEditCommitDeclineReasondoc-comment (domEditCommitRunner.ts:39-46). The docstring makes the two exit shapes explicit (reject vs.{ok:false}), which is what a caller mixing.then/.catchwill need to reason about. - Concurrency: the position-patch rethrow now feeds the caller's optimistic revert, closing the drag-persist race that was the reason this bug had a user-visible tail.
useDomEditPositionPatchCommit.test.tsx:22-58covers the three shapes (queue-paused → reject-no-toast, ordinary-failure → reject-with-toast, success → resolve). - Telemetry:
trackStudioSaveFailureis still called on ordinary save failures and correctly skipped onDomEditSaveQueueOpenError. No path lost a report.
Adversarial fingerprint
For handleDomStyleCommit, a manual-geometry property like left now returns { ok: false, reason: "geometry-property" } (useDomEditTextCommits.ts:190). A caller that only checks outcome.ok will treat this as "did not land" — but the style path deliberately refuses these because manual-geometry lives on a different code path, so this is intended and matches the docstring. Not a defect.
CI
gh pr checks 3510: 51 pass, 2 skipping, 5 pending, 1 fail. The one fail is the regression aggregation from run 33034712644, red because its shards were cancelled != success; the shards on the newer run 33034885989 are still pending and several have already gone green. Re-verify once shards settle.
Verdict
R1 findings resolved; ready for approval by another reviewer.
— Via
|
@vanceingalls @james-russo-rames-d-jusso Both R2 verdicts are green at 2a0a034, so this is a request for the formal APPROVED review, not another analysis pass. Via, there is no other reviewer assigned to this stack, and APPROVED is a review state you have used in this repo before. Please submit it yourself, or name the specific remaining reason you will not. Rames, please say whether GREEN is your terminal state or whether you can submit APPROVED; if you can, please stamp this PR. |
jrusso1020
left a comment
There was a problem hiding this comment.
APPROVED at 2a0a034dd5914a7e2aca732acd3f83d0825cbdfb.
Read the diff independently rather than stamping on the green R2s. The outcome contract holds: onSettled fires exactly once on both branches of runDomEditCommit (domEditCommitRunner.ts:46,52) and both precede the resync at :55, so landed is always set by the time runReportedDomEditCommit resolves. Every top-level path in all three producing handlers now returns an outcome — the :145 bare return that started this is gone.
One non-blocking finding: the contract is enforced nowhere in the type system.
DomEditCommitOutcome appears exactly three times in this 1202-line diff — the type declaration, domEditCommitDeclined's return type, and runReportedDomEditCommit's. It is never a handler's return type. All three producers (handleDomStyleCommit, handleDomTextCommit, handleDomEditElementsDelete) are bare async (…) => {…} relying on inference.
That means a future bare return; in any of them silently widens the inferred type to … | undefined rather than failing the build — which is exactly the bug this PR exists to remove, reintroduced with no compiler complaint. The delete test matrix pins today's paths; it can't pin a path that doesn't exist yet. An explicit : Promise<DomEditCommitOutcome> on each of the three is a one-line-each durable guard, and worth having before the write tools downstream start depending on the shape.
Two things I chased and cleared, so nobody reopens them:
capturebailing on a missing element does not make{ok: true}a lie. InhandleDomStyleCommitbothcaptureandapplyearly-return when the element isn't in the preview DOM, butpersistcallspersistDomEditOperations(domEditSelection, operations, …)— payload derived from the selection descriptor, not fromeditedElement. The file genuinely changes; only the optimistic in-preview paint is skipped. Pre-existing preview/file divergence, outside this fix boundary.- The bare
return;s remaining inside the handlers are in thecapture/apply/revertclosures, which are correctlyvoid. Not stragglers.
autoMergeRequest=null, so this stamp is a stamp and the merge stays yours. Not gating on the windows-latest red — CI isn't mine to hold a stamp on — but it is real and yours to clear before merge.
— Rames
|
@jrusso1020 @vanceingalls Fixed the approval finding in 69c4402. All three outcome-producing handlers now explicitly return Promise, so a future bare return is a compile error rather than a silently widened union. Witness proof: temporarily restoring the former bare return fails typecheck with TS2322, undefined is not assignable to DomEditCommitOutcome. Restored head passes typecheck, 13 focused tests, and every pre-commit check. Please approve the new head if clear. |
jrusso1020
left a comment
There was a problem hiding this comment.
APPROVED at 69c4402f74b38954a71e36e7180ed5a92615e531. Finding fixed, verified at source.
All three producers now carry the annotation — handleDomStyleCommit and handleDomTextCommit (useDomEditTextCommits.ts:189,273) and handleDomEditElementsDelete (useElementLifecycleOps.ts:88) — imported type-only, so nothing is added at runtime. The delta is 5 added and 4 removed across exactly the two files that hold them, with no drive-by changes riding along.
I checked the two things that could have made this cosmetic rather than real:
- The annotation actually bites.
packages/studio/tsconfig.jsonsets"strict": true, sostrictNullChecksis on andundefinedis not assignable toDomEditCommitOutcome. A barereturn;is now a type error, and so is a path that falls off the end — TS reports the missing ending return precisely because the declared type excludesundefined. Without strict mode this would have compiled and the guard would have been inert, which is the version of this fix that looks identical in a diff. - No consumer erases it. Nothing in
useDomEditWiring.tsorpropertyPanelTypes.tsre-declares these three with aPromise<void>signature that would flatten the contract back at the boundary.
So the type system is now the witness, and it will stay the witness without anyone maintaining a test for it. That's the durable version.
— Rames
vanceingalls
left a comment
There was a problem hiding this comment.
R3 delta re-verify (HEAD 69c4402f)
Prior verified HEAD was 2a0a034d. Miguel's follow-up addresses Rames's R2 non-blocking finding — that the outcome contract was runtime-only and would not stop a future bare return; from re-introducing the silent-swallow bug.
1. Delta scope
gh api compare 2a0a034d...69c4402f returns one commit ("fix(studio): enforce DOM edit outcome types") touching two files:
packages/studio/src/hooks/useDomEditTextCommits.ts(+3, −2)packages/studio/src/hooks/useElementLifecycleOps.ts(+2, −2)
No other files, no drive-by edits, no test changes in the delta. The 15 runtime-shape tests Rames and I both leaned on for R1/R2 are the pre-existing suite; the delta pulls its weight from the type system, not from new tests.
2. Type-witness claim — substantiated
Three producers now carry the annotation:
handleDomStyleCommit—useDomEditTextCommits.ts:189→async (property, value): Promise<DomEditCommitOutcome> => …handleDomTextCommit—useDomEditTextCommits.ts:273→ same shapehandleDomEditElementsDelete—useElementLifecycleOps.ts:88→ same shape
Imports are type-only in both files, so nothing is added at runtime.
The annotation bites — I verified two independent gates:
- Strictness.
packages/studio/tsconfig.jsonat69c4402fsets"strict": true, which turns onstrictNullChecks.undefinedis not assignable to{ ok: true } | { ok: false; reason: DomEditCommitDeclineReason }, so a future barereturn;is a compile error and so is a fall-through path — TS reports the missing return precisely because the declared type excludesundefined. Without strict, the annotation would be inert and the fix would look identical in a diff. - No consumer flattens it back. Grepping the delta and its neighbouring consumers, no
Promise<void>re-declaration re-widens the contract at the boundary. The widerPromise<unknown>types ononStyleCommitetc. in R2 accept the narrowerPromise<DomEditCommitOutcome>— they don't erase it at the producer.
Typecheck passed on this HEAD (Actions runs/33036722304/job/98400920649), which is the direct proof the annotation compiles.
3. Test discipline
Delta adds no tests. The pre-existing R2 tests (useDomEditTextCommits.test.tsx, useElementLifecycleOps.multiDelete.test.tsx, useDomEditPositionPatchCommit.test.tsx, anchoredResizeCommitFeedsOffset.test.ts) all assert runtime shape — expect(outcome).toEqual({ ok: false, reason: "persist-failed" }) and friends — not type existence. Runtime discipline stayed; this delta lets the type system replace the "will anyone remember to test the new path" fragility with a compiler check.
4. Adversarial — throws still bypass the type witness (accepted)
A Promise<T> type constrains the resolved value only; a rejection carries any thrown error regardless of T. So the annotation catches "handler falls through and resolves undefined" but not "handler throws before reaching a return".
Walking each producer to confirm the surviving throw paths are correctly routed to rejection rather than silent completion:
handleDomStyleCommit/handleDomTextCommit—runReportedDomEditCommitawaitsrunDomEditCommit, which by contract callsonSettled(ok)on both persist paths (domEditCommitRunner.ts:46,52) and swallows persist failures. Acapture/apply-shaped defect rejects — which is the intended signal (defect, not handled failure), distinct from{ ok: false }, and the caller can distinguish them by.catchvs. resolved outcome.handleDomEditElementsDelete— wraps its main path intry/catchand returnsdomEditCommitDeclined("persist-failed")on error, so even a throw resolves as a proper outcome; the type witness is genuinely total here.
Nothing to raise — this is the expected shape.
5. CI
Typecheck, Build, Lint, Format, Preflight, CLI smoke (required), Producer unit + integration, Studio load smoke + timeline viewport gate, Test: skills, Test: runtime contract, SDK unit+contract+smoke — all green at 69c4402f. Perf shards and one Analyze pass still in-progress at review time; no non-success conclusions anywhere on the run. windows-latest — the red Rames flagged at R2 — is not present at this HEAD.
Verdict
Fix substantiated at source. Type-witness is real, not cosmetic. Non-approving per protocol; deferring stamp to the human path.
— Via
What
Three places where a failed Studio edit reported success are now able to say they failed.
handleDomTextCommitandhandleDomStyleCommitreturn a tagged outcome instead ofundefined.handleDomEditElementsDeletereports instead of only toasting.useDomEditPositionPatchCommitno longer swallowsDomEditSaveQueueOpenError.Why
runDomEditCommitcatches a persist failure, reverts, firesonErrorand then resolves. That contract is deliberate and its own docstring says so: the human learns the write failed from the toastonErrorputs on screen, so a rejection would be redundant. The cost is that a caller awaiting one of these handlers cannot tell a landed write from a reverted one.The position-patch swallow had a real user-visible consequence.
useDomGeometryCommitsonly restores the optimistic offset, size or rotation from its.catch. Swallowing the paused-queue error skipped that revert, so once the save-queue breaker opened a drag left the element where the user dropped it while nothing reached the file, and the next reload snapped it back.How
runDomEditCommitalready offeredonSettledas the way out; text and style were the two commits that never got it wired.runReportedDomEditCommitowns that callback, forwards to a caller-supplied one rather than dropping it, and returns whether the write landed.The outcome is a tagged union rather than a boolean, so the three preconditions that previously returned early and silently stay distinguishable: no selection, a manual-geometry property the style path refuses, and a selection that cannot edit styles.
The paused-queue branch still does not toast, because the paused-save banner is already on screen and one toast per blocked edit is what that branch existed to prevent. It just rejects now, so the caller gets to revert.
Human-facing behaviour is unchanged throughout, and the tests assert that: the toast still fires and the optimistic DOM change is still reverted.
The callback props carrying these handlers ignore the result, so their declared type widens from
Promise<void>toPromise<unknown>. That type is hand-copied in fourteen places; consolidating it is worth its own change and is not attempted here.Test plan
Tests were written first and watched fail for the right reason (
undefinedwhere an outcome belonged) before the fix.useDomEditTextCommits.test.tsx: persist failure reports, success reports, and each decline reason is distinct, with assertions that the toast still fires and the DOM still reverts.useDomEditPositionPatchCommit.test.tsxis new: paused queue rejects without toasting, an ordinary failure toasts and rejects, success resolves.bunx tsc --noEmitclean. Typecheck caught two real consumer breaks that would otherwise have shipped.useDomEditTextCommits.tsis now 593 lines against the 600-line cap. The next change to it needs a split.Scope added during review