From 00d963724c131c9bd025681f2540cd5588cdd258 Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Wed, 17 Jun 2026 16:48:19 -0700 Subject: [PATCH 1/3] =?UTF-8?q?fix(sdk,studio):=20R5=20cutover=20review=20?= =?UTF-8?q?fixes=20=E2=80=94=20fromTo=20dest,=20timing=20sync,=20parity?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Confirmed correctness findings from the R5 review of the SDK cutover stack, applied on top of #1539: - fromTo add via cutover dropped its destination: handleAddGsapTween read only `toProperties`; now falls back to `properties` like every other method. - handleSetTiming GSAP sync: a clip with no data-start skipped the shift (now treats start as 0, matching the server path) and a blank/non-numeric data-start wrote position: NaN (now sanitized). - handleSetTiming no longer appends an absolute position to an auto-sequenced (implicit-position) tween, which collapsed staggers. - handleSetTiming keeps data-end in sync when a clip carries BOTH data-duration and data-end (a stale data-end inverted the clip). - string/relative tween positions ("+=0.5", "<") documented as a known ceiling. - opacity/autoAlpha property seed no longer falsy-zero (`|| 1`): an element at opacity 0 seeds 0, not 1. - optimistic add-keyframe cache tolerance aligned to the writer's PCT_TOLERANCE (2%) so a near-neighbour keyframe no longer shows then vanishes on reload. - DOM-patch finiteness validation runs before the SDK cutover path. - attribute ops mapping to a reserved data-* name decline the cutover up front instead of throwing inside dispatch. Regression tests added for each SDK-side fix. Co-Authored-By: Claude Opus 4.8 (1M context) --- packages/sdk/src/engine/mutate.gsap.test.ts | 65 +++++++++++++++++++ packages/sdk/src/engine/mutate.ts | 42 +++++++++--- .../studio/src/hooks/useDomEditCommits.ts | 22 ++++--- .../studio/src/hooks/useGsapKeyframeOps.ts | 6 +- .../src/hooks/useGsapPropertyDebounce.ts | 5 +- packages/studio/src/utils/sdkCutover.test.ts | 8 +++ packages/studio/src/utils/sdkCutover.ts | 29 ++++++++- 7 files changed, 158 insertions(+), 19 deletions(-) diff --git a/packages/sdk/src/engine/mutate.gsap.test.ts b/packages/sdk/src/engine/mutate.gsap.test.ts index 5d60d6b59..3c839f6ed 100644 --- a/packages/sdk/src/engine/mutate.gsap.test.ts +++ b/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"); + }); }); diff --git a/packages/sdk/src/engine/mutate.ts b/packages/sdk/src/engine/mutate.ts index 6b5051d7a..8b4761053 100644 --- a/packages/sdk/src/engine/mutate.ts +++ b/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 = {}; - 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) - : ((tween.toProperties ?? tween.properties ?? {}) as Record); + // 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; // 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. diff --git a/packages/studio/src/hooks/useDomEditCommits.ts b/packages/studio/src/hooks/useDomEditCommits.ts index aae11d62d..6cc5c4659 100644 --- a/packages/studio/src/hooks/useDomEditCommits.ts +++ b/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 diff --git a/packages/studio/src/hooks/useGsapKeyframeOps.ts b/packages/studio/src/hooks/useGsapKeyframeOps.ts index 8ff6c2aa4..e50852b34 100644 --- a/packages/studio/src/hooks/useGsapKeyframeOps.ts +++ b/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(); diff --git a/packages/studio/src/hooks/useGsapPropertyDebounce.ts b/packages/studio/src/hooks/useGsapPropertyDebounce.ts index 60e031c81..3b59609ce 100644 --- a/packages/studio/src/hooks/useGsapPropertyDebounce.ts +++ b/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) { diff --git a/packages/studio/src/utils/sdkCutover.test.ts b/packages/studio/src/utils/sdkCutover.test.ts index 17279a890..bc6826e1e 100644 --- a/packages/studio/src/utils/sdkCutover.test.ts +++ b/packages/studio/src/utils/sdkCutover.test.ts @@ -77,6 +77,14 @@ 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("returns true when ops mix all supported types", () => { expect( shouldUseSdkCutover(true, true, "hf-abc", [ diff --git a/packages/studio/src/utils/sdkCutover.ts b/packages/studio/src/utils/sdkCutover.ts index 09668be60..cf21f6281 100644 --- a/packages/studio/src/utils/sdkCutover.ts +++ b/packages/studio/src/utils/sdkCutover.ts @@ -14,6 +14,32 @@ const CUTOVER_OP_TYPES = new Set([ "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([ + "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", +]); + +function mapsToReservedAttr(op: PatchOperation): boolean { + if (op.type !== "attribute") return false; + const name = op.property.startsWith("data-") ? op.property : `data-${op.property}`; + return RESERVED_CUTOVER_ATTRS.has(name); +} + /** * Map Studio PatchOperations for a given hf-id to SDK EditOps. * @@ -61,7 +87,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) ); } From 5d6738b10cf2f3f88b943461836b22d65479c327 Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Wed, 17 Jun 2026 17:00:17 -0700 Subject: [PATCH 2/3] fix(studio): close two gaps in the reserved-attr cutover gate - Lowercase the mapped attribute name before the reserved check, matching the SDK's validateSetAttribute (which lowercases), so a case-variant reserved name is declined up front instead of throwing inside dispatch. - Also gate `html-attribute` ops (raw, non-prefixed names), not just bare `attribute` ops. Both the emitter and the gate now derive the name via one shared `sdkAttrName` helper so they can't drift. Co-Authored-By: Claude Opus 4.8 (1M context) --- packages/studio/src/utils/sdkCutover.test.ts | 12 +++++++ packages/studio/src/utils/sdkCutover.ts | 34 +++++++++++++------- 2 files changed, 34 insertions(+), 12 deletions(-) diff --git a/packages/studio/src/utils/sdkCutover.test.ts b/packages/studio/src/utils/sdkCutover.test.ts index bc6826e1e..93e16411e 100644 --- a/packages/studio/src/utils/sdkCutover.test.ts +++ b/packages/studio/src/utils/sdkCutover.test.ts @@ -85,6 +85,18 @@ describe("shouldUseSdkCutover", () => { 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", [ diff --git a/packages/studio/src/utils/sdkCutover.ts b/packages/studio/src/utils/sdkCutover.ts index cf21f6281..03499a01f 100644 --- a/packages/studio/src/utils/sdkCutover.ts +++ b/packages/studio/src/utils/sdkCutover.ts @@ -34,10 +34,24 @@ const RESERVED_CUTOVER_ATTRS = new Set([ "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 { - if (op.type !== "attribute") return false; - const name = op.property.startsWith("data-") ? op.property : `data-${op.property}`; - return RESERVED_CUTOVER_ATTRS.has(name); + 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()); } /** @@ -57,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 }); + } } } From 047f00c07110c990fe2f191f6b1ae04f9563ee40 Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Wed, 17 Jun 2026 17:08:05 -0700 Subject: [PATCH 3/3] fix(studio): match keyframe remove-path tolerance to the writer (mirror of add) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The optimistic remove-keyframe cache filtered with `> 0.001`, dropping only a near-exact match, while the writer removes within PCT_TOLERANCE (2). Removing at e.g. 49% dropped a 50% keyframe on disk but left it in the cache — a phantom that vanished on reload, the inverted twin of the add-path tolerance fix. Now filters with `> 2` to match. Co-Authored-By: Claude Opus 4.8 (1M context) --- packages/studio/src/hooks/useGsapKeyframeOps.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/packages/studio/src/hooks/useGsapKeyframeOps.ts b/packages/studio/src/hooks/useGsapKeyframeOps.ts index e50852b34..fbb247274 100644 --- a/packages/studio/src/hooks/useGsapKeyframeOps.ts +++ b/packages/studio/src/hooks/useGsapKeyframeOps.ts @@ -168,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 () => {