fix(studio): make inspector commits transactional

[miguel-heygen]

Stack 2/2 after GSAP ownership. Adds atomic preflight, rollback, and stale-generation protection to inspector and grouped transform commits. 321 changed lines. Split from #2984.

[vanceingalls]

APPROVE — Robust generation-counter design correctly handles concurrent-gesture races (traced 4 scenarios including sourceValue advancing via a successful prior commit while a later commit is in flight — the sourceRef.current = sourceValue resync on every render covers the tight window). Return-value propagation through propertyPanel3dTransform.tsx and propertyPanelPrimitives.tsx correctly turns fire-and-forget void calls into awaitable chains the transaction can catch. Tests exercise the exact races the PR claims to fix.

Three P2 (no blockers):

• packages/studio/src/hooks/useAnimatedPropertyCommit.ts commitStaticSet (~L155-215, commitSetProps L143-149) — "Transactional" claim is oversold. The loop for (const [targetWrite, batch] of byTargetWrite) { await commitSetProps(...) } (then a second loop for newSetBatches) is not atomic: a rejection on the second-or-later mutation leaves earlier writes persisted server-side. Field-level rollback + bumpGsapCache() restore eventual consistency, not atomicity. Scenario: grouped-transform commit spanning two property groups (position + size on element with legacy mixed writes) — server 500 on second write → position persisted, size not, inspector shows baseline while server has mixed state until cache refetch. Consider narrowing the PR body ("adds error detection + field rollback + stale-generation guard") or wrapping the loop in a server-side batch endpoint.
• packages/studio/src/components/editor/useInspectorGestureTransaction.ts:73-75 — useEffect(() => { generationRef.current += 1; }, [sourceValue]) uses an effect purely to sync a ref against a prop change. AGENTS.md rule "No useEffect for state syncing" applies. Rewrite as a render-time detector: const lastSourceRef = useRef(sourceValue); if (!Object.is(lastSourceRef.current, sourceValue)) { lastSourceRef.current = sourceValue; generationRef.current += 1; }. Also strictly-mode-safe.
• packages/studio/src/components/editor/propertyPanelTransformCommit.ts:71-98 + gsapEditOutcome.ts — when onCommitAnimatedProperty throws GsapEditBlockedError (rich .message + .action), the transform handlers let the promise reject with no showToast(err.message) at this layer. The field silently rolls back and the user sees a value snap-back with no explanation of why. Since the error carries actionable copy specifically for user display, either catch-and-toast at the handler boundary or verify an upstream boundary already does. Otherwise, silent no-op UX from the X-user's original complaint persists for the "no-selector"/"unroll-required"/"source-uneditable" paths at the transform-commit boundary.

Cross-PR pattern (#2986 + #2987): the "explain WHY the edit was blocked" UX is inconsistent across paths. Same block reason surfaces as a toast on drag/resize/rotate, silent no-op on inspector 3D property, and silent snap-back on transform-commit. Worth unifying at a common boundary before shipping the X-user-facing fix.

Standards: Lint/preview-regression/player-perf green; Fallow ignores on pickBestAnimation and commitAnimatedProperties (complexity) justified.

— Review by Via

[miga-heygen]

R1 — Transactional inspector commits (head 7143d07)

Verdict: Approve (confirming Via's P2s)

This PR completes the error-surfacing story started by #2986. The key fix is changing the bare catch { bumpGsapCache(); } in commitAnimatedProperties to catch (error) { bumpGsapCache(); throw error; } — which unblocks the GsapEditBlockedError throws that #2986 added.

What was verified

Generation counter for stale-rejection detection: Both CommitField and useInspectorGestureTransaction use a generationRef that bumps on cancel, new input, new gesture, and source-value changes. Rejection handlers check generation !== generationRef.current before restoring. This prevents an old rejection from overwriting a newer draft — tested explicitly in both files.

Async commit propagation chain:

• onCommit signatures widened from void to void | Promise<void> through CommitField, FlatRow, MetricField, and useInspectorGestureTransaction. Return values are propagated with return onCommit(...) at each layer.
• propertyPanelTransformCommit.ts: void Promise.resolve(...) → await. .catch(() => undefined) removed. Errors now propagate.
• CommitField.commitDraft: rejection → setDraft(baseline) + onPreview?.(baseline) (snap-back). Generation check prevents stale snap-back.

Full error flow (with both #2986 + #2987):

1. Inspector 3D edit → commitAnimatedProperty wrapper (from #2986: toast + rethrow) → CommitField catch → snap-back
2. Drag/resize/rotate → outcome/throw → useGsapAwareEditing catch → toast → gestureTransaction rollback
3. All paths: toast + visual restoration. Consistent.

Cross-checked Via's findings

1. "Transactional" oversold — agreed as a naming concern. commitStaticSet loop is await-per-group, not atomic. A mid-loop failure leaves partial state. But the PR title refers to the field-level optimistic-update pattern (draft → commit → reject → snap-back), not database ACID. The field-level pattern IS transactional. The multi-property path is sequential-with-best-effort, which is the pre-existing contract.

2. useEffect generation bump — confirmed. Both CommitField and useInspectorGestureTransaction bump generationRef in effects, not render-time. The timing gap (render → effect) is sub-frame and hasn't caused issues, but a render-time useMemo or inline ref-write would be more correct. P2.

3. propertyPanelTransformCommit doesn't catch/toast — partially disagree. The error flows through commitAnimatedProperty (the #2986 wrapper), which DOES toast via trackGsapInteractionFailure before rethrowing. The CommitField then catches the rejection and snaps back. The user sees toast + snap-back, same as other paths. Via may have been analyzing the paths without the wrapper in scope.

Tests

Strong coverage: rejected commit → snap-back, stale rejection → no snap-back, useInspectorGestureTransaction rollback, commitAnimatedProperties rejection propagation, bumpGsapCache called on error.

---

Review by Miga

[james-russo-rames-d-jusso]

Reviewed at 7143d07d.

Solid intent — async plumb-through + rejection rollback + generation gating are the right primitives, and the sibling shape for generationRef matches the codebase (see useSdkSession.ts:165, 211, 254, 271). Peer R1 (Via) has APPROVE with three P2s; converging on two, adding four correctness findings peer missed, and escalating one item to blocker because it's the landing site for #2986's HITL regression fix.

BLOCKER — inspector swallowing catch lives here and this PR doesn't unswallow it. useAnimatedPropertyCommit.ts:395-534 wraps the entire property-routing block in try { … } catch { bumpGsapCache(); }. #2986 adds an aspirational handleGsapAwareCommitAnimatedProperty wrapper (useGsapAwareEditing.ts:345-366) that expects the raw impl to reject with GsapEditBlockedError — but this catch swallows it. Net: inspector-driven edit on a helper/runtime-owned tween silently no-ops while drag/resize/rotate on the same tween surface the block via toast. That's the exact HITL regression signature the parent #2984 committed to closing. This PR modifies useAnimatedPropertyCommit.ts (hunks at :24-30, 69-75, 179-184, 192-203, 258-264, 397-403, 406-435, 521-527, 551-560), so the fix belongs here: narrow the catch to if (isGsapEditBlockedError(e)) throw e; and add preflight to commitAnimatedProperties before the routing branches (using assertGsapAnimationDirectlyEditable from #2986). See #2986 BLOCKER A for the full failure scenario.

Four more concerns, all inline:

• commitStaticSet multi-batch loop is not atomic — useAnimatedPropertyCommit.ts:216-222 iterates commitSetProps per target-write with sequential await. Server 500 mid-loop leaves partial state (x=50 persisted, scale=1.2 still in-flight). PR body advertises "atomic preflight, rollback, and stale-generation protection to grouped transform commits" — this violates it. (Converging with Via's P2.)
• DOM mutation from applyStudioPathOffset is NOT rolled back on rejection — propertyPanelCommitField.tsx:82-88 restores draft in the catch, but applyStudioBoxSize / applyStudioPathOffset in useDomGeometryCommits.ts:54-58 synchronously mutate the DOM before the async persist. On persist rejection, the input reverts to baseline but the preview iframe still renders at the failed value. Real transaction gap.
• Settle-time preview flash to before widens under async commits — useInspectorGestureTransaction.ts:41-48 fires previewRef.current(active.before) at settle. Now that onCommit is genuinely async (returns a Promise on the network path), the parent value can't advance until the persist ACKs — every metric-field release paints stale-before-final for the full round-trip. useInspectorGestureDraft sidesteps this at :100-102; CommitField / propertyPanelColor do not. Miao's asset-flicker debt lens: user-visible.
• useEffect(() => generationRef.current += 1, [sourceValue]) gates legitimate rollback if a caller synchronously advances parent state. (Converging with Via's P2 flagging this as an AGENTS.md anti-pattern.) A caller doing optimistic-parent-update + async-persist would see the effect bump generation, then the persist rejection's catch would see generation !== generationRef.current and skip the transaction rollback — parent state permanently reflects the never-persisted latest.
• propertyPanelTransformCommit doesn't catch GsapEditBlockedError → silent snap-back — the transform-commit boundary drops the block on the floor. Same UX-inconsistency pattern Via's cross-PR item calls out. Converging.

Cross-PR UX pattern (endorsing Via): the "explain WHY the edit was blocked" surface is inconsistent — toast on drag/resize/rotate, silent no-op on inspector, silent snap-back on transform-commit. Same block reason, three different surfaces. Unifying at useGsapInteractionFailureTelemetry before shipping would close the gap the BLOCKER opens.

Small nit worth surfacing (body only, no inline): propertyPanelTypes.ts:74-76 declares onSetManualOffset / onSetManualSize / onSetManualRotation as => void while propertyPanelTransformCommit.ts does await Promise.resolve(...). Sibling handlers in the same file already use void | Promise<void>. Align the three to match runtime.

Test-coverage gaps — no test covers commitStaticSet partial rejection mid-loop, no test covers multi-group preflight where two animations share targetGroup but only one is unroll-required, no test covers commitAnimatedProperties({x, opacity}) where x's group passes preflight but opacity's group has a helper-authored tween. All three would have caught the concerns above.

— Review by Rames D Jusso

[miguel-heygen]

Browser verification on stacked head 66132faf (local CLI production build + real Chromium, design-panel-qa fixture):

• Direct GSAP X edit emitted gsap-mutations, persisted gsap.set(... { x: 20 }), and showed 20px after a full reload.
• Injected one-shot HTTP 500 for the next GSAP mutation: Inspector rolled back to 0px, preview DOM rolled back to translate(0px, 0px), disk retained the prior x: 20, and Studio showed both Couldn't save animation: e2e injected fault and Failed to save animated edit.
• rotationX: 10 persisted beside the fixture's separate 2D rotation tween and reloaded as 10°, exercising the exact-axis ownership filter.
• Replaced the direct tween in the scratch fixture with addTween(target, vars = {...}); addTween("#qa-tween-box"): attempting X=25 showed This motion comes from a helper or loop. Choose Unroll to edit it explicitly., sent no gsap-mutations request, rolled the field back to 0px, and left source unchanged.

The repository's older generic design-panel.mjs fixture can still select real elements, but its legacy data-panel-section field locators no longer match the current flat Inspector DOM; the assertions above were therefore driven directly through the current accessible UI and CDP mouse/keyboard events.

[miga-heygen]

R2 — Atomic inspector persistence (head 66132fa)

Verdict: Approve

R1 findings resolved:

1. Error propagation fixed — The bare catch pattern is now .catch((error) => { bumpGsapCache(); throw error; }) at all three mutation sites in commitAnimatedProperties. Errors propagate through the wrapper in useGsapAwareEditing → toast → rethrow → CommitField catch → snap-back.

2. Transform-commit test coverage — New propertyPanelTransformCommit.test.ts verifies the async error propagation through the transform commit chain.

3. useDomGeometryCommits — New file cleanly separates geometry commit wiring.

The merge-order concern from R1 is no longer critical since #2986's v2 moves intercepts to outcome-based returns (not throwing). Both PRs are safe to merge independently now, though landing together still gives the cleanest deployment.

No new findings.

---

Review by Miga

[miguel-heygen]

CI follow-up at 68cb244d: the full-suite Test job exposed two existing synchronous color-editor expectations after the async transaction rewrite (propertyPanelColor incomplete-hex settle and propertyPanelColorCurves release preview).

Fixed by separating transaction completion semantics:

• Promise-returning persistence keeps its optimistic preview while pending and rolls back only on rejection.
• Synchronous inspector consumers restore their preview immediately after persistence, preserving the established color-picker/curve behavior.
• A source acknowledgement for an earlier commit is tracked separately so it cannot cancel a newer active gesture.

Verification:

• 78/78 focused tests across transaction, CommitField/flat primitives, transform commit, animated-property commit, DOM geometry, color field, and color curves.
• Studio typecheck, format, lint, tracked-artifact gate, file-size gate, and Fallow all pass (inherited duplication remains warning-only).

[miga-heygen]

R3 — Sync/async commit split (head 68cb244)

Verdict: Approve

Targeted fix for CI-caught color editor compatibility failures. The R2 settle() wrapped all commit results in Promise.resolve(result).then(...), which deferred cleanup to the next microtask — breaking synchronous consumers (color pickers, curve editors) that expect the baseline preview to restore immediately after persist.

What was verified

Sync/async split: isPromiseCommit (duck-type thenable check) routes the two paths:

• Sync commits (void return): immediately clear pendingRef and restore baseline preview — matching historical behavior. Color picker closes, source value arrives in the next render.
• Async commits (Promise return): keep optimistic preview visible until resolution. No flash-back to baseline while the write is in flight.

awaitingSourceAckRef: Tracks the generation + value of a pending async commit. When the incoming sourceValue matches the awaited ack value, the render-time detector recognizes it as the commit's own propagation (not an external override) and doesn't bump the generation counter. Clean: cleared on ack, rollback, and cancel.

rollbackCommit extraction: Shared callback deduplicates the two error-handling blocks from R2 (catch block + promise rejection). Correctly clears pendingRef, awaitingSourceAckRef, restores source + preview.

Edge cases:

• Sync throw → catch → rollbackCommit → restore. Correct.
• Non-thenable truthy return → isPromiseCommit returns false → sync path. Correct.
• Stale rollback (generation mismatch) → no-op. Correct.

No new findings.

---

Review by Miga

[james-russo-rames-d-jusso]

Reviewed at 68cb244d — delta since 7143d07d, with a targeted sync-compat follow-up from 66132faf → 68cb244d.

All six R1 findings — the BLOCKER and all five concerns — addressed.

BLOCKER (useAnimatedPropertyCommit.ts swallowing catch) — narrowed to re-throw at :583-585, AND preflight hoisted to the top of commitAnimatedProperties at :439-444: assertGsapEditPersisted(directEditOutcomeForProperties(selectedGsapAnimations, new Set(propEntries.map(([property]) => property)))) runs before any try / routing. Test at useAnimatedPropertyCommit.test.tsx (rethrows a persistence failure to the telemetry wrapper) asserts rejects.toBe(failure) AND bumpGsapCache called exactly once. HITL regression path closed at the layer where the swallow lived.

Five concerns landed:

• commitStaticSet multi-batch atomicity — plans all calls first via planStaticSetCalls, then routes single-group calls through commit(...) OR multi-group through atomic commit.batch(calls, ...). Throws pre-flight if commit.batch is unavailable — zero partial-write. Tests cover single-call, atomic-batch, and missing-batch-preflight-throw paths.
• DOM mutation rollback on rejection — all three geometry commits (path offset, box size, rotation) now capture before-state via captureStudio*, and on rejection restore via restoreStudio* and rethrow. Test at useDomGeometryCommits.test.tsx (restores every optimistic geometry mutation when persistence rejects) verifies all three via real DOM read-back on readStudioPathOffset / readStudioBoxSize / readStudioRotation after rejection.
• Settle-time flash to before — at 66132faf the pre-commit previewRef.current(active.before) was removed and only the async rejection callback restores the baseline. 68cb244d refines further: an isPromiseCommit type-guard splits sync-vs-async paths so legacy sync inspector consumers (color pickers, curve editors) preserve their historical after-persist preview-restore, while async commits keep the optimistic preview through the network round-trip. My R1 concern was specifically about the flash widening under async; the async path is clean.
• generationRef unconditional bump on [sourceValue] — the useEffect(() => { generationRef.current += 1 }, [sourceValue]) is gone, replaced with a render-time guard: generation only bumps when the source-value change doesn't match activeRef.current.latest, pendingRef.current.latest, OR the new awaitingSourceAckRef.current.value. Parent echoing the committed value no longer aborts an in-flight rollback path. Test at propertyPanelFlatPrimitives.test.tsx (restores its durable value when an async commit rejects) re-renders parent with the optimistic value before the rejection fires and still asserts rollback.
• Transform-commit boundary swallow — solved via a cleaner mechanism than R1 proposed: instead of catching GsapEditBlockedError locally, all three transform handlers await and let rejection propagate up to CommitField's uniform rollback. Telemetry fires once at the useGsapAwareEditing seam (useGsapAwareEditing.ts:357 — trackGsapInteractionFailure(error, selection, "property", "Edit animated property"); throw error;). Test (propagates blocked ${axis} edits so the field can roll back) verifies all three handlers propagate GsapEditBlockedError AND the manual setters are not called during a blocked flow.

One small note on 68cb244d — the new sync-branch in settle() (else if (generation === generationRef.current) { pendingRef.current = null; previewRef.current(active.before); }) is not exercised by a dedicated test in useInspectorGestureTransaction.test.tsx (all three tests there are async). The color-editor suites (propertyPanelColor*.test.tsx) do cover it indirectly — which is what caught the earlier failures the compat fix addresses. Not blocking; a targeted sync-path unit test on the hook itself would guard against future regression of the split.

82 focused tests + full build/typecheck/lint/precommit + real-browser success/reload/fault paths green. Ready from my side, leaving as COMMENTED.

— Review by Rames D Jusso

[vanceingalls]

R2 APPROVE at 68cb244d. All 3 P2 findings + the cross-PR UX consistency callout are FIXED.

• F1 (atomicity oversold) — FIXED. A real batch primitive was added: commit.batch(calls) posts a single POST /projects/:id/gsap-mutations-batch/:file; server-side applyGsapMutations applies all mutations in memory then does ONE writeFileSync — errors return early with no bytes written. commitStaticSet now throws when commit.batch is unavailable rather than looping per-target-write. gsap-mutation-rollback endpoint added for higher-level orchestration. "Transactional" is now accurate.
• F2 (useEffect for state syncing anti-pattern) — FIXED. useInspectorGestureTransaction.ts:24-42 replaces the effect with a render-time detector (lastSourceValueRef + Object.is guard at the top of the hook body). Preserves optimistic-value ack semantics via awaitingSourceAckRef. Matches AGENTS.md.
• F3 (no toast on GsapEditBlockedError at transform-commit) — FIXED. Toast wiring landed at the correct layer: useGsapInteractionFailureTelemetry catches at the useGsapAwareEditing.commitAnimatedProperty(ies) wrapper and calls showToast(error.message, "error") when isGsapEditBlockedError(error), else "Failed to save animated edit." Then rethrows so useInspectorGestureTransaction.settle's rollback still restores the field. propertyPanelTransformCommit.test.ts asserts the error propagates to enable rollback.

Cross-PR UX consistency — FIXED at the right boundary. All five gesture surfaces (drag, group-drag, resize, rotate, inspector property/transform commit) now funnel through trackGsapInteractionFailure at the same code point in useGsapAwareEditing.ts — same error-copy source (GsapEditBlockedError.message from the shared COPY table in gsapEditOutcome.ts) and same UX (toast + rollback). Silent snap-back is gone across the stack.

No new findings. Standards: CI green modulo in-progress Windows tests, no blockers. Fallow audit SUCCESS. New tests added for batch atomicity + telemetry + geometry paths.

— Review by Via