fix(studio): address #1613 review — derive instantPatch from the mutation, patch both coalesced commits, wire onAsyncFailure

miguel-heygen · miguel-heygen · commit ff2be9df3a59 · 2026-06-20T17:17:58.000-04:00
- commitStaticGsapPosition/Rotation derive instantPatch.change.props from the actual update-property mutation(s) sent (one source of truth → findUnsafeMutationValues-validated values flow into the patch; can't drift). - Coalesced x/y: the intermediate x commit also carries instantPatch{x}, the y commit {x,y}, so a second-POST failure still leaves the preview patched for what persisted. - applyPreviewSync passes reloadPreview as onAsyncFailure (plugin-CDN load error → full reload); per U4 the synchronous false still does NOT escalate. - (channel disambiguation from #1612 verified end-to-end: {x,y}→position set, {rotation}→rotation set.)
diff --git a/packages/studio/src/hooks/gsapDragCommit.test.ts b/packages/studio/src/hooks/gsapDragCommit.test.ts
@@ -275,7 +275,7 @@ const existingRotationSet = (): GsapAnimation =>
 describe("commitStaticGsapPosition — instantPatch (value-only set)", () => {
   beforeEach(() => usePlayerStore.setState({ currentTime: 0, activeKeyframePct: null }));
 
-  it("attaches instantPatch {kind:set, props:{x,y}} to the FINAL coalesced commit only", async () => {
+  it("attaches an instantPatch to BOTH coalesced commits, each derived from its own mutation", async () => {
     const { commits, callbacks } = optionRecordingCallbacks();
 
     await commitStaticGsapPosition(
@@ -288,9 +288,14 @@ describe("commitStaticGsapPosition — instantPatch (value-only set)", () => {
     );
 
     expect(commits).toHaveLength(2);
-    // First (x) commit is the intermediate skipReload one — NO instantPatch.
+    // First (x) commit is the intermediate skipReload one — it now carries an
+    // instantPatch for just {x}, so if the SECOND POST fails the preview still
+    // reflects the x that DID persist (no reload, instant feedback).
     expect(commits[0].options.skipReload).toBe(true);
-    expect(commits[0].options.instantPatch).toBeUndefined();
+    expect(commits[0].options.instantPatch).toEqual({
+      selector: "#puck-a",
+      change: { kind: "set", props: { x: -50 } },
+    });
     // Final (y) commit triggers the reload and carries the full {x,y} patch.
     expect(commits[1].options.softReload).toBe(true);
     expect(commits[1].options.instantPatch).toEqual({
@@ -299,6 +304,34 @@ describe("commitStaticGsapPosition — instantPatch (value-only set)", () => {
     });
   });
 
+  it("derives each instantPatch's props from the value in the SAME mutation that's POSTed", async () => {
+    const { commits, callbacks } = optionRecordingCallbacks();
+
+    await commitStaticGsapPosition(
+      selection(),
+      { x: -50, y: 30 },
+      { x: 0, y: 0 },
+      "#puck-a",
+      existingPositionSet(),
+      callbacks,
+    );
+
+    // The patch values must equal the mutation values — they're read out of the
+    // same object, so a clean mutation can't ship alongside a stale patch.
+    const xMutation = commits[0].mutation as { property: string; value: number };
+    const yMutation = commits[1].mutation as { property: string; value: number };
+    const xPatch = commits[0].options.instantPatch as {
+      change: { props: Record<string, number> };
+    };
+    const yPatch = commits[1].options.instantPatch as {
+      change: { props: Record<string, number> };
+    };
+    expect(xPatch.change.props[xMutation.property]).toBe(xMutation.value);
+    expect(yPatch.change.props[yMutation.property]).toBe(yMutation.value);
+    // The y commit's combined patch also carries the x mutation's value.
+    expect(yPatch.change.props[xMutation.property]).toBe(xMutation.value);
+  });
+
   it("does NOT attach instantPatch when ADDING a new set (structural — new tween)", async () => {
     const { commits, callbacks } = optionRecordingCallbacks();
 
@@ -331,6 +364,12 @@ describe("commitStaticGsapRotation — instantPatch (value-only set)", () => {
       selector: "#puck-a",
       change: { kind: "set", props: { rotation: 42 } },
     });
+    // Patch value derived from the SAME mutation that's POSTed (one source).
+    const m = commits[0].mutation as { property: string; value: number };
+    const patch = commits[0].options.instantPatch as {
+      change: { props: Record<string, number> };
+    };
+    expect(patch.change.props[m.property]).toBe(m.value);
   });
 
   it("does NOT attach instantPatch when ADDING a new rotation set (structural)", async () => {
diff --git a/packages/studio/src/hooks/gsapDragCommit.ts b/packages/studio/src/hooks/gsapDragCommit.ts
@@ -10,7 +10,7 @@ import { resolveTweenStart, resolveTweenDuration } from "../utils/globalTimeComp
 import { roundTo3 } from "../utils/rounding";
 import { computeElementPercentage } from "./gsapShared";
 import { computeDraggedGsapPosition } from "./draggedGsapPosition";
-import type { RuntimeTweenChange } from "./gsapRuntimePatch";
+import type { RuntimeTweenChange, SetPatchProps } from "./gsapRuntimePatch";
 export interface GsapDragCommitCallbacks {
   commitMutation: (
     selection: DomEditSelection,
@@ -381,6 +381,37 @@ async function commitFlatViaKeyframes(
 // without importing the GSAP commit graph (store/runtime/core).
 export { computeDraggedGsapPosition };
 
+/** The shape of an `update-property` mutation a static-set nudge POSTs. */
+interface UpdatePropertyMutation {
+  type: "update-property";
+  animationId: string;
+  property: string;
+  value: number;
+}
+
+/**
+ * Build the `instantPatch` for a value-only `tl.set` from the SAME
+ * `update-property` mutation(s) that are POSTed — so the patch can never carry a
+ * value the source write didn't (one source of truth). Each mutation contributes
+ * its `{property: value}` channel to the patch's props.
+ */
+function setPatchFromUpdateProperties(
+  selector: string,
+  mutations: UpdatePropertyMutation[],
+): { selector: string; change: RuntimeTweenChange } {
+  const props: SetPatchProps = {};
+  for (const m of mutations) props[m.property as keyof SetPatchProps] = m.value;
+  return { selector, change: { kind: "set", props } };
+}
+
+/** Single-mutation convenience over {@link setPatchFromUpdateProperties}. */
+function setPatchFromUpdateProperty(
+  selector: string,
+  mutation: UpdatePropertyMutation,
+): { selector: string; change: RuntimeTweenChange } {
+  return setPatchFromUpdateProperties(selector, [mutation]);
+}
+
 /**
  * Find the studio position-hold `set` for a selector — a `tl.set("#el",{x,y})`
  * with no duration. This is what a static-element nudge writes/updates.
@@ -420,24 +451,41 @@ export async function commitStaticGsapPosition(
     // Update in place — two single-property mutations (the API updates one prop
     // per call). Coalesce them and reload only after the second lands.
     const coalesceKey = `gsap:set-nudge:${existingSet.id}`;
-    await callbacks.commitMutation(
-      selection,
-      { type: "update-property", animationId: existingSet.id, property: "x", value: newX },
-      { label: "Move layer", skipReload: true, coalesceKey },
-    );
-    await callbacks.commitMutation(
-      selection,
-      { type: "update-property", animationId: existingSet.id, property: "y", value: newY },
-      {
-        label: "Move layer",
-        softReload: true,
-        coalesceKey,
-        // Final commit of the coalesced x/y pair: carry the full {x,y} so the
-        // runtime `tl.set` is patched in place instantly (skips the soft reload
-        // when the helper can confidently apply it).
-        instantPatch: { selector, change: { kind: "set", props: { x: newX, y: newY } } },
-      },
-    );
+    // Build each mutation FIRST, then derive its instantPatch from the SAME
+    // object that's POSTed — so a future caller can't ship a clean mutation with
+    // a stale/malformed patch (the validated `value` flows straight into the
+    // patch). `findUnsafeMutationValues` validates the mutation upstream.
+    const xMutation = {
+      type: "update-property",
+      animationId: existingSet.id,
+      property: "x",
+      value: newX,
+    } as const;
+    const yMutation = {
+      type: "update-property",
+      animationId: existingSet.id,
+      property: "y",
+      value: newY,
+    } as const;
+    // Patch BOTH coalesced commits. If the SECOND POST fails server-side, the
+    // first (x) already persisted — patching its commit too means the live
+    // preview still reflects what DID persist. The x commit carries skipReload
+    // (no reload), so its instantPatch gives instant feedback without a reload;
+    // the y commit triggers the soft reload (skipped when the patch applies).
+    await callbacks.commitMutation(selection, xMutation, {
+      label: "Move layer",
+      skipReload: true,
+      coalesceKey,
+      instantPatch: setPatchFromUpdateProperty(selector, xMutation),
+    });
+    await callbacks.commitMutation(selection, yMutation, {
+      label: "Move layer",
+      softReload: true,
+      coalesceKey,
+      // Final commit of the coalesced x/y pair: carry both channels so the
+      // runtime `tl.set` lands the complete {x,y} pose in place.
+      instantPatch: setPatchFromUpdateProperties(selector, [xMutation, yMutation]),
+    });
     return;
   }
   await callbacks.commitMutation(
@@ -482,21 +530,21 @@ export async function commitStaticGsapRotation(
   callbacks: GsapDragCommitCallbacks,
 ): Promise<void> {
   if (existingSet) {
-    await callbacks.commitMutation(
-      selection,
-      {
-        type: "update-property",
-        animationId: existingSet.id,
-        property: "rotation",
-        value: newRotation,
-      },
-      {
-        label: "Rotate layer",
-        softReload: true,
-        // Value-only rotation set: patch the runtime `tl.set` rotation in place.
-        instantPatch: { selector, change: { kind: "set", props: { rotation: newRotation } } },
-      },
-    );
+    // Derive the instantPatch from the SAME mutation object that's POSTed (single
+    // source of truth — see commitStaticGsapPosition), so the validated `value`
+    // flows into the patch and the two can't drift.
+    const rotationMutation = {
+      type: "update-property",
+      animationId: existingSet.id,
+      property: "rotation",
+      value: newRotation,
+    } as const;
+    await callbacks.commitMutation(selection, rotationMutation, {
+      label: "Rotate layer",
+      softReload: true,
+      // Value-only rotation set: patch the runtime `tl.set` rotation in place.
+      instantPatch: setPatchFromUpdateProperty(selector, rotationMutation),
+    });
     return;
   }
   await callbacks.commitMutation(
diff --git a/packages/studio/src/hooks/gsapRuntimeBridge.test.ts b/packages/studio/src/hooks/gsapRuntimeBridge.test.ts
@@ -89,7 +89,7 @@ describe("tryGsapDragIntercept — stale-parse guard (no resurrection after dele
     expect(mutation.type).not.toBe("add-keyframe");
   });
 
-  it("forwards instantPatch {kind:set,x,y} on the final commit when updating an existing static set", async () => {
+  it("forwards instantPatch on BOTH coalesced commits when updating an existing static set", async () => {
     const commitMutation = vi.fn();
     const iframe = fakeIframe("puck-b", []); // runtime empty → STATIC path
     // An existing position-hold `set` for the selector → update-in-place (not add).
@@ -112,11 +112,15 @@ describe("tryGsapDragIntercept — stale-parse guard (no resurrection after dele
     );
 
     expect(handled).toBe(true);
-    // The coalesced update-property pair: the x commit is skipReload (no patch),
-    // the final y commit triggers the reload and carries the full {x,y} patch.
+    // The coalesced update-property pair both carry an instantPatch so a partial
+    // (second-POST) failure still leaves the preview patched for what persisted:
+    // the x commit patches {x}, the final y commit patches the full {x,y}.
     const updates = commitMutation.mock.calls.filter(([, m]) => m.type === "update-property");
     expect(updates).toHaveLength(2);
-    expect(updates[0][2].instantPatch).toBeUndefined();
+    expect(updates[0][2].instantPatch).toEqual({
+      selector: "#puck-b",
+      change: { kind: "set", props: { x: -50 } },
+    });
     expect(updates[1][2].instantPatch).toEqual({
       selector: "#puck-b",
       change: { kind: "set", props: { x: -50, y: 30 } },
diff --git a/packages/studio/src/hooks/useGsapScriptCommits.test.tsx b/packages/studio/src/hooks/useGsapScriptCommits.test.tsx
@@ -63,7 +63,7 @@ describe("applyPreviewSync", () => {
     expect(reloadPreview).not.toHaveBeenCalled();
   });
 
-  it("instantPatch + patch fails: falls back to the soft reload", () => {
+  it("instantPatch + patch fails: falls back to the soft reload, passing onAsyncFailure", () => {
     patchRuntimeTweenInPlace.mockReturnValue(false);
     applySoftReload.mockReturnValue(true);
     const reloadPreview = vi.fn();
@@ -79,11 +79,13 @@ describe("applyPreviewSync", () => {
       reloadPreview,
     );
 
-    expect(applySoftReload).toHaveBeenCalledWith(FAKE_IFRAME, "SCRIPT");
+    // reloadPreview is wired as onAsyncFailure (3rd arg) so a MotionPath-plugin
+    // CDN load failure escalates to a full reload — but it is NOT called eagerly.
+    expect(applySoftReload).toHaveBeenCalledWith(FAKE_IFRAME, "SCRIPT", reloadPreview);
     expect(reloadPreview).not.toHaveBeenCalled();
   });
 
-  it("instantPatch + patch fails + soft reload fails: escalates to full reload", () => {
+  it("instantPatch + patch fails + soft reload returns false: does NOT sync-escalate (U4)", () => {
     patchRuntimeTweenInPlace.mockReturnValue(false);
     applySoftReload.mockReturnValue(false);
     const reloadPreview = vi.fn();
@@ -99,10 +101,13 @@ describe("applyPreviewSync", () => {
       reloadPreview,
     );
 
-    expect(reloadPreview).toHaveBeenCalledTimes(1);
+    // U4: the synchronous false return means the soft reload couldn't run, NOT
+    // that the preview is broken — escalation happens only via onAsyncFailure.
+    expect(applySoftReload).toHaveBeenCalledWith(FAKE_IFRAME, "SCRIPT", reloadPreview);
+    expect(reloadPreview).not.toHaveBeenCalled();
   });
 
-  it("no instantPatch + softReload + scriptText: soft reloads (today's behavior)", () => {
+  it("no instantPatch + softReload + scriptText: soft reloads, passing onAsyncFailure", () => {
     applySoftReload.mockReturnValue(true);
     const reloadPreview = vi.fn();
 
@@ -114,11 +119,11 @@ describe("applyPreviewSync", () => {
     );
 
     expect(patchRuntimeTweenInPlace).not.toHaveBeenCalled();
-    expect(applySoftReload).toHaveBeenCalledWith(FAKE_IFRAME, "SCRIPT");
+    expect(applySoftReload).toHaveBeenCalledWith(FAKE_IFRAME, "SCRIPT", reloadPreview);
     expect(reloadPreview).not.toHaveBeenCalled();
   });
 
-  it("no instantPatch + softReload that fails: full reload (today's behavior)", () => {
+  it("no instantPatch + softReload that returns false: does NOT sync-escalate (U4)", () => {
     applySoftReload.mockReturnValue(false);
     const reloadPreview = vi.fn();
 
@@ -129,7 +134,9 @@ describe("applyPreviewSync", () => {
       reloadPreview,
     );
 
-    expect(reloadPreview).toHaveBeenCalledTimes(1);
+    // onAsyncFailure is wired, but the sync false return does not trigger it.
+    expect(applySoftReload).toHaveBeenCalledWith(FAKE_IFRAME, "SCRIPT", reloadPreview);
+    expect(reloadPreview).not.toHaveBeenCalled();
   });
 
   it("no instantPatch + no softReload: full reload (today's behavior)", () => {
@@ -264,7 +271,7 @@ describe("runCommit — instantPatch wiring", () => {
     });
 
     expect(fetch).toHaveBeenCalledTimes(1);
-    expect(applySoftReload).toHaveBeenCalledWith(FAKE_IFRAME, "SCRIPT");
+    expect(applySoftReload).toHaveBeenCalledWith(FAKE_IFRAME, "SCRIPT", deps.reloadPreview);
     expect(deps.reloadPreview).not.toHaveBeenCalled();
     expect(deps.onCacheInvalidate).toHaveBeenCalledTimes(1);
   });
@@ -279,7 +286,7 @@ describe("runCommit — instantPatch wiring", () => {
     });
 
     expect(patchRuntimeTweenInPlace).not.toHaveBeenCalled();
-    expect(applySoftReload).toHaveBeenCalledWith(FAKE_IFRAME, "SCRIPT");
+    expect(applySoftReload).toHaveBeenCalledWith(FAKE_IFRAME, "SCRIPT", deps.reloadPreview);
     expect(deps.reloadPreview).not.toHaveBeenCalled();
   });
 });
diff --git a/packages/studio/src/hooks/useGsapScriptCommits.ts b/packages/studio/src/hooks/useGsapScriptCommits.ts
@@ -68,7 +68,12 @@ export function applyPreviewSync(
     // Fall through to the soft/full reload path below.
   }
   if (options.softReload && result.scriptText) {
-    if (!applySoftReload(iframe, result.scriptText)) reloadPreview();
+    // Per U4, do NOT escalate on the synchronous `false` return (it means
+    // "soft-reload couldn't run; the value is unchanged on screen, not broken"
+    // — a full reload would re-flash the WebGL context for nothing). Only the
+    // async MotionPath-plugin load failure escalates, via `onAsyncFailure`,
+    // which fires after a soft reload that already returned true optimistically.
+    applySoftReload(iframe, result.scriptText, reloadPreview);
   } else {
     reloadPreview();
   }

Original file line number	Diff line number	Diff line change
`@@ -68,7 +68,12 @@ export function applyPreviewSync(`
`68`	`68`	`// Fall through to the soft/full reload path below.`
`69`	`69`	`}`
`70`	`70`	`if (options.softReload && result.scriptText) {`
`71`		`- if (!applySoftReload(iframe, result.scriptText)) reloadPreview();`
	`71`	+ // Per U4, do NOT escalate on the synchronous `false` return (it means
	`72`	`+ // "soft-reload couldn't run; the value is unchanged on screen, not broken"`
	`73`	`+ // — a full reload would re-flash the WebGL context for nothing). Only the`
	`74`	+ // async MotionPath-plugin load failure escalates, via `onAsyncFailure`,
	`75`	`+ // which fires after a soft reload that already returned true optimistically.`
	`76`	`+ applySoftReload(iframe, result.scriptText, reloadPreview);`
`72`	`77`	`} else {`
`73`	`78`	`reloadPreview();`
`74`	`79`	`}`