fix(sdk,studio): R5 cutover review fixes (on top of #1539)

packages/sdk/src/engine/mutate.gsap.test.ts

@@ -184,6 +184,26 @@ describe("addGsapTween", () => {
     expect(newScript).toContain("opacity: 0");
     expect(newScript).toContain("opacity: 1");
   });
+
+  it("R5 #1: fromTo destination supplied via `properties` (Studio add path) is not dropped", () => {
+    const parsed = fresh();
+    const result = applyOp(parsed, {
+      type: "addGsapTween",
+      target: "hf-box",
+      tween: {
+        method: "fromTo",
+        duration: 0.5,
+        fromProperties: { opacity: 0 },
+        // Studio's add path puts the destination in `properties`, not `toProperties`.
+        properties: { x: 400, opacity: 1 },
+      },
+    });
+    const newScript = String(result.forward[0]?.value ?? "");
+    expect(newScript).toContain("fromTo(");
+    // Regression: fromTo previously read only `toProperties` and wrote empty
+    // to-vars, so the destination vanished.
+    expect(newScript).toContain("x: 400");
+  });
 });
 
 // ─── Tween op test helpers ────────────────────────────────────────────────────
@@ -1027,4 +1047,49 @@ describe("handleSetTiming GSAP sync (CF2 #15/#16)", () => {
     // tween duration scaled 4 → 8 (ratio 2).
     expect(getScript(parsed)).toContain("duration: 8");
   });
+
+  it("R5 #3: a start-less clip (no data-start) still shifts its tween (implicit start 0)", () => {
+    const parsed = timingDoc(`data-duration="4"`, `tl.to("#box", { x: 100, duration: 1 }, 2);`);
+    applyOp(parsed, { type: "setTiming", target: "hf-box", start: 3 });
+    // oldStart defaults to 0, so position remaps 2 → 3 + (2 − 0) = 5.
+    // The bug skipped the whole sync block when data-start was absent.
+    expect(getScript(parsed)).toMatch(/tl\.to\("#box",[^)]*\}, 5\)/);
+  });
+
+  it("R5 #3: a malformed data-start never writes position: NaN", () => {
+    const parsed = timingDoc(
+      `data-start="" data-duration="4"`,
+      `tl.to("#box", { x: 100, duration: 1 }, 2);`,
+    );
+    applyOp(parsed, { type: "setTiming", target: "hf-box", start: 3 });
+    const script = getScript(parsed);
+    expect(script).not.toContain("NaN");
+    expect(script).toMatch(/tl\.to\("#box",[^)]*\}, 5\)/);
+  });
+
+  it("R5 #2: an implicit-position tween is not collapsed to an absolute position on move", () => {
+    const parsed = timingDoc(
+      `data-start="2" data-end="5"`,
+      `tl.to("#box", { x: 100, duration: 1 });`,
+    );
+    applyOp(parsed, { type: "setTiming", target: "hf-box", start: 5 });
+    const script = getScript(parsed);
+    // The tween had no position arg (auto-sequenced); it must stay that way —
+    // appending an absolute position would collapse the stagger.
+    expect(script).toContain('tl.to("#box", { x: 100, duration: 1 })');
+    expect(script).not.toMatch(/tl\.to\("#box",[^)]*\}, \d/);
+  });
+
+  it("R5 #7: a clip with BOTH data-duration and data-end keeps data-end in sync on move", () => {
+    const parsed = timingDoc(
+      `data-start="1" data-duration="2" data-end="3"`,
+      `tl.to("#box", { x: 1, duration: 2 }, 1);`,
+    );
+    applyOp(parsed, { type: "setTiming", target: "hf-box", start: 5 });
+    const el = parsed.document.querySelector('[data-hf-id="hf-box"]');
+    expect(el?.getAttribute("data-start")).toBe("5");
+    expect(el?.getAttribute("data-duration")).toBe("2");
+    // data-end recomputed (5 + 2); the bug left it stale at 3 → inverted clip.
+    expect(el?.getAttribute("data-end")).toBe("7");
+  });
 });

packages/sdk/src/engine/mutate.ts

@@ -437,6 +437,17 @@ function handleSetTiming(
         result.inverse.push(p.inverse);
         el.setAttribute("data-duration", String(newDuration));
       }
+      // A clip carrying BOTH data-duration and data-end must keep data-end in
+      // sync (end = start + duration) on any start/duration change, else the
+      // stale data-end inverts the clip (end < start) for runtimes that read it.
+      if (oldEndStr !== null && newStart !== null && newDuration !== null) {
+        const newEnd = newStart + newDuration;
+        const endPath = timingPath(id, "end");
+        const ep = scalarChange(endPath, oldEnd, newEnd);
+        result.forward.push(ep.forward);
+        result.inverse.push(ep.inverse);
+        el.setAttribute("data-end", String(newEnd));
+      }
     } else if (
       (timing.duration !== undefined || timing.start !== undefined) &&
       newStart !== null &&
@@ -470,7 +481,12 @@ function handleSetTiming(
     // those clips left their tweens unsynced.
     const matchHfId = el.getAttribute("data-hf-id") ?? id;
     const matchDomId = el.getAttribute("id");
-    if (parsedGsap && currentScript && oldStart !== null) {
+    if (parsedGsap && currentScript) {
+      // A missing data-start means an implicit start of 0 (matching the server
+      // shiftGsapPositions path); a malformed attr parses to NaN. Sanitize to a
+      // finite number so a start-less/blank clip still shifts and never feeds
+      // NaN into the tween positions.
+      const oldStartNum = oldStart !== null && Number.isFinite(oldStart) ? oldStart : 0;
       // Per-tween shift/scale (mirrors shiftGsapPositions/scaleGsapPositions): a
       // multi-tween stagger maps each tween's own intra-clip position by the
       // start DELTA and scales its duration by the clip-duration RATIO. Writing
@@ -482,16 +498,25 @@ function handleSetTiming(
         durChanged && oldDuration !== null && oldDuration > 0 && newDuration !== null
           ? newDuration / oldDuration
           : 1;
-      const remapStart = startChanged && newStart !== null ? newStart : oldStart;
+      const remapStart = startChanged && newStart !== null ? newStart : oldStartNum;
       for (const { id: animId, animation } of parsedGsap.located) {
         const matches =
           selectorMatchesId(animation.targetSelector, matchHfId) ||
           (matchDomId !== null && selectorMatchesId(animation.targetSelector, matchDomId));
         if (!matches) continue;
+        // Skip tweens whose position is a label or relative string ("+=0.5",
+        // "<", ">"): relative positions already track their neighbours, and a
+        // string position can't be safely shifted by the clip delta here.
+        // ponytail: known ceiling — string positions are not re-synced on
+        // move/resize; numeric positions only.
         if (typeof animation.position !== "number") continue;
         const updates: Partial<GsapAnimation> = {};
-        if (startChanged || durChanged) {
-          const shifted = remapStart + (animation.position - oldStart) * ratio;
+        // Don't write an absolute position onto an auto-sequenced tween (no
+        // explicit position arg → parsed as implicitPosition): the writer would
+        // APPEND a position arg, collapsing the stagger onto one point. Duration
+        // still scales below.
+        if ((startChanged || durChanged) && animation.implicitPosition !== true) {
+          const shifted = remapStart + (animation.position - oldStartNum) * ratio;
           updates.position = Math.max(0, Math.round(shifted * 1000) / 1000);
         }
         if (durChanged && typeof animation.duration === "number" && animation.duration > 0) {
@@ -771,10 +796,11 @@ function handleAddGsapTween(
   if (tween.yoyo !== undefined) extras.yoyo = tween.yoyo;
   if (tween.stagger !== undefined) extras.stagger = tween.stagger;
 
-  const toProps =
-    tween.method === "fromTo"
-      ? ((tween.toProperties ?? {}) as Record<string, number | string>)
-      : ((tween.toProperties ?? tween.properties ?? {}) as Record<string, number | string>);
+  // A fromTo's destination may arrive as either `toProperties` or `properties`
+  // (the Studio add path sets `properties`). Fall back the same way for every
+  // method — the old fromTo-only branch read `toProperties` alone and wrote an
+  // empty to-vars object, so fromTo animations added via cutover animated to {}.
+  const toProps = (tween.toProperties ?? tween.properties ?? {}) as Record<string, number | string>;
 
   // Scoped ids like "hf-host/hf-leaf" must use the bare leaf id in the GSAP
   // selector — only the leaf part is written as data-hf-id on the DOM element.

packages/studio/src/hooks/useDomEditCommits.ts

@@ -154,6 +154,20 @@ export function useDomEditCommits({
       }
 
       if (options?.shouldSave && !options.shouldSave()) return;
+
+      // Validate layout values BEFORE any persist path runs. The SDK cutover
+      // path (onTrySdkPersist) returns early on success, so leaving this check
+      // after it let invalid numeric values bypass the guard whenever the
+      // cutover flag was on.
+      const patchTarget = buildDomEditPatchTarget(selection);
+      const patchBody = { target: patchTarget, operations };
+      const unsafeFields = findUnsafeDomPatchValues(patchBody);
+      if (unsafeFields.length > 0) {
+        const fields = formatUnsafeFieldList(unsafeFields);
+        showToast("Couldn't save edit because it contains invalid layout values", "error");
+        throw new Error(`DOM patch contains unsafe values: ${fields}`);
+      }
+
       // Skip the SDK path when prepareContent is set (e.g. @font-face injection
       // for a custom font): sdkCutoverPersist serializes only the patched DOM
       // and would drop the injected content. Let the server path run prepareContent.
@@ -170,14 +184,6 @@ export function useDomEditCommits({
         // forceReload (that would echo-reload the session we just wrote).
         return;
       }
-      const patchTarget = buildDomEditPatchTarget(selection);
-      const patchBody = { target: patchTarget, operations };
-      const unsafeFields = findUnsafeDomPatchValues(patchBody);
-      if (unsafeFields.length > 0) {
-        const fields = formatUnsafeFieldList(unsafeFields);
-        showToast("Couldn't save edit because it contains invalid layout values", "error");
-        throw new Error(`DOM patch contains unsafe values: ${fields}`);
-      }
 
       // Mark the save timestamp before the file write so the SSE file-change
       // handler suppresses the reload even if the event arrives before the

packages/studio/src/hooks/useGsapKeyframeOps.ts

@@ -80,8 +80,12 @@ export function useGsapKeyframeOps({
         // appending a duplicate — matches addKeyframeToScript, which writes one
         // keyframe per percentage (merging properties).
         apply: (prev) => {
+          // Match addKeyframeToScript's merge tolerance (PCT_TOLERANCE = 2 in
+          // gsapWriterAcorn): a keyframe added within 2% of an existing one
+          // merges on disk, so the optimistic cache must merge it too — else the
+          // UI shows a phantom keyframe that vanishes on the next reload.
           const idx = prev.keyframes.findIndex(
-            (kf) => Math.abs((kf.tweenPercentage ?? kf.percentage) - percentage) < 0.001,
+            (kf) => Math.abs((kf.tweenPercentage ?? kf.percentage) - percentage) <= 2,
           );
           if (idx >= 0) {
             const keyframes = prev.keyframes.slice();
@@ -164,8 +168,13 @@ export function useGsapKeyframeOps({
         elementId: selection.id,
         apply: (prev) => ({
           ...prev,
+          // Match the writer's removal tolerance (PCT_TOLERANCE = 2 in
+          // gsapWriterAcorn): removing at e.g. 49% drops a keyframe at 50% on
+          // disk, so the optimistic cache must drop it too — else the stranded
+          // entry is a phantom that vanishes on the next reload (mirror of the
+          // add-path tolerance fix).
           keyframes: prev.keyframes.filter(
-            (kf) => Math.abs((kf.tweenPercentage ?? kf.percentage) - percentage) > 0.001,
+            (kf) => Math.abs((kf.tweenPercentage ?? kf.percentage) - percentage) > 2,
           ),
         }),
         persist: async () => {

packages/studio/src/hooks/useGsapPropertyDebounce.ts

@@ -136,7 +136,10 @@ export function useGsapPropertyDebounce(
         defaultValue = Math.round(property === "width" ? rect.width : rect.height);
       } else if (property === "opacity" || property === "autoAlpha") {
         const cs = el.ownerDocument.defaultView?.getComputedStyle(el);
-        defaultValue = cs ? Number.parseFloat(cs.opacity) || 1 : 1;
+        // Use `|| 1` only as a non-finite fallback, not a falsy fallback: an
+        // element currently at opacity 0 must seed 0, not 1.
+        const current = cs ? Number.parseFloat(cs.opacity) : Number.NaN;
+        defaultValue = Number.isFinite(current) ? current : 1;
       }
       const { sdkSession, sdkDeps, activeCompPath } = sdkRef.current ?? {};
       if (sdkSession && sdkDeps) {

packages/studio/src/utils/sdkCutover.test.ts

@@ -77,6 +77,26 @@ describe("shouldUseSdkCutover", () => {
     expect(shouldUseSdkCutover(true, true, "hf-abc", [htmlAttrOp("class", "foo")])).toBe(true);
   });
 
+  it("returns false for an attribute op that maps to a reserved data-* name", () => {
+    // {type:'attribute', property:'end'} → 'data-end', which the SDK's
+    // validateSetAttribute rejects. Decline the batch so it takes the server
+    // path cleanly instead of throwing inside dispatch and falling back per op.
+    expect(shouldUseSdkCutover(true, true, "hf-abc", [attrOp("end", "2")])).toBe(false);
+    expect(shouldUseSdkCutover(true, true, "hf-abc", [attrOp("data-start", "1")])).toBe(false);
+  });
+
+  it("declines a case-variant reserved attribute (SDK lowercases before checking)", () => {
+    // attribute op "END" → "data-END" → lower → "data-end" (reserved).
+    expect(shouldUseSdkCutover(true, true, "hf-abc", [attrOp("END", "2")])).toBe(false);
+  });
+
+  it("declines an html-attribute op whose raw name is reserved", () => {
+    // html-attribute ops aren't data-prefixed, so a raw reserved name must still
+    // be caught (the SDK throws on it just the same).
+    expect(shouldUseSdkCutover(true, true, "hf-abc", [htmlAttrOp("data-end", "3")])).toBe(false);
+    expect(shouldUseSdkCutover(true, true, "hf-abc", [htmlAttrOp("DATA-START", "1")])).toBe(false);
+  });
+
   it("returns true when ops mix all supported types", () => {
     expect(
       shouldUseSdkCutover(true, true, "hf-abc", [

packages/studio/src/utils/sdkCutover.ts

@@ -14,6 +14,46 @@ const CUTOVER_OP_TYPES = new Set<PatchOperation["type"]>([
   "html-attribute",
 ]);
 
+// Mirrors the SDK's RESERVED_ATTRS (mutate.ts): a bare `attribute` op is
+// force-prefixed `data-`, so e.g. property "end" → "data-end", which the SDK
+// rejects with a throw. Detect that up front and decline the whole batch so it
+// takes the server path cleanly, instead of throwing inside the dispatch and
+// silently falling back per op.
+// ponytail: small mirror of the SDK set; if the SDK adds a reserved attr, a new
+// op for it just reverts to the (working) throw→fallback path until synced.
+const RESERVED_CUTOVER_ATTRS = new Set<string>([
+  "data-hf-id",
+  "data-composition-id",
+  "data-width",
+  "data-height",
+  "data-start",
+  "data-end",
+  "data-track-index",
+  "data-hold-start",
+  "data-hold-end",
+  "data-hold-fill",
+]);
+
+// The attribute name the SDK setAttribute op carries for this patch op (or null
+// if the op isn't an attribute). Shared by patchOpsToSdkEditOps and the reserved
+// gate so the name they reason about can't drift: a bare `attribute` op is
+// force-prefixed `data-`; an `html-attribute` op keeps its raw name.
+function sdkAttrName(op: PatchOperation): string | null {
+  if (op.type === "attribute") {
+    return op.property.startsWith("data-") ? op.property : `data-${op.property}`;
+  }
+  if (op.type === "html-attribute") return op.property;
+  return null;
+}
+
+function mapsToReservedAttr(op: PatchOperation): boolean {
+  const name = sdkAttrName(op);
+  // Lowercase to match the SDK's validateSetAttribute (it lowercases before the
+  // reserved check), so "DATA-START" is declined up front too; covers both
+  // `attribute` (prefixed) and `html-attribute` (raw) ops.
+  return name !== null && RESERVED_CUTOVER_ATTRS.has(name.toLowerCase());
+}
+
 /**
  * Map Studio PatchOperations for a given hf-id to SDK EditOps.
  *
@@ -31,15 +71,11 @@ function patchOpsToSdkEditOps(hfId: string, ops: PatchOperation[]): EditOp[] {
       hasStyles = true;
     } else if (op.type === "text-content") {
       result.push({ type: "setText", target: hfId, value: op.value ?? "" });
-    } else if (op.type === "attribute") {
-      result.push({
-        type: "setAttribute",
-        target: hfId,
-        name: op.property.startsWith("data-") ? op.property : `data-${op.property}`,
-        value: op.value,
-      });
-    } else if (op.type === "html-attribute") {
-      result.push({ type: "setAttribute", target: hfId, name: op.property, value: op.value });
+    } else if (op.type === "attribute" || op.type === "html-attribute") {
+      const name = sdkAttrName(op);
+      if (name !== null) {
+        result.push({ type: "setAttribute", target: hfId, name, value: op.value });
+      }
     }
   }
 
@@ -61,7 +97,8 @@ export function shouldUseSdkCutover(
     hasSession &&
     !!hfId &&
     ops.length > 0 &&
-    ops.every((o) => CUTOVER_OP_TYPES.has(o.type))
+    ops.every((o) => CUTOVER_OP_TYPES.has(o.type)) &&
+    !ops.some(mapsToReservedAttr)
   );
 }