diff --git a/packages/core/src/parsers/gsapParser.ts b/packages/core/src/parsers/gsapParser.ts index e274ee1bb..0f383bc67 100644 --- a/packages/core/src/parsers/gsapParser.ts +++ b/packages/core/src/parsers/gsapParser.ts @@ -2170,9 +2170,16 @@ function buildMotionPathObjectCode(config: { }): string { const { waypoints, segments, autoRotate } = config; const hasExplicitControlPoints = segments.some((s) => s.cp1 && s.cp2); + // The simple `path` array supports only one scalar curviness for the whole + // path, so per-segment curviness must use the cubic form (curviness baked into + // each segment's control points). Without this, the simple branch serializes + // only segments[0].curviness and silently drops every other segment's curve. + const curvinessVaries = segments.some( + (s) => (s.curviness ?? 1) !== (segments[0]?.curviness ?? 1), + ); let pathEntries: string[]; - if (hasExplicitControlPoints && waypoints.length >= 2) { + if ((hasExplicitControlPoints || curvinessVaries) && waypoints.length >= 2) { // type: "cubic" — interleave control points: [anchor, cp1, cp2, anchor, ...] pathEntries = [`{x: ${waypoints[0]!.x}, y: ${waypoints[0]!.y}}`]; for (let i = 0; i < segments.length; i++) { diff --git a/packages/core/src/parsers/gsapSerialize.ts b/packages/core/src/parsers/gsapSerialize.ts index 28301a653..0ab7386a0 100644 --- a/packages/core/src/parsers/gsapSerialize.ts +++ b/packages/core/src/parsers/gsapSerialize.ts @@ -120,15 +120,20 @@ export function buildArcPath( autoRotate: boolean | number, isCubic: boolean, ): MotionPathShape | undefined { - if (coords.length < 2) return undefined; + const first = coords[0]; + if (coords.length < 2 || !first) return undefined; const segments: ArcPathSegment[] = []; let waypoints: Array<{ x: number; y: number }>; if (isCubic && coords.length >= 4) { // coords are [anchor, cp1, cp2, anchor, cp1, cp2, anchor, ...]. - waypoints = [coords[0]!]; + waypoints = [first]; for (let i = 1; i + 2 < coords.length; i += 3) { - waypoints.push(coords[i + 2]!); - segments.push({ curviness, cp1: coords[i]!, cp2: coords[i + 1]! }); + const cp1 = coords[i]; + const cp2 = coords[i + 1]; + const anchor = coords[i + 2]; + if (!cp1 || !cp2 || !anchor) continue; + waypoints.push(anchor); + segments.push({ curviness, cp1, cp2 }); } } else { waypoints = coords; @@ -527,10 +532,15 @@ function buildCubicPathEntries( waypoints: Array<{ x: number; y: number }>, segments: ArcPathSegment[], ): string[] { - const entries = [`{x: ${waypoints[0]!.x}, y: ${waypoints[0]!.y}}`]; + const first = waypoints[0]; + if (!first) return []; + const entries = [`{x: ${first.x}, y: ${first.y}}`]; for (let i = 0; i < segments.length; i++) { - const nextWp = waypoints[i + 1]!; - entries.push(...cubicControlPoints(segments[i]!, waypoints[i]!, nextWp)); + const seg = segments[i]; + const wp = waypoints[i]; + const nextWp = waypoints[i + 1]; + if (!seg || !wp || !nextWp) continue; + entries.push(...cubicControlPoints(seg, wp, nextWp)); entries.push(`{x: ${nextWp.x}, y: ${nextWp.y}}`); } return entries; @@ -543,7 +553,17 @@ export function buildMotionPathObjectCode(config: { }): string { const { waypoints, segments, autoRotate } = config; const arSuffix = autoRotateSuffix(autoRotate); - if (segments.some((s) => s.cp1 && s.cp2) && waypoints.length >= 2) { + // GSAP's simple `path` array supports only ONE scalar `curviness` for the whole + // path, so per-segment curviness can only be expressed in the cubic form (each + // segment's curviness baked into its control points). Emit cubic when segments + // carry explicit control points OR when their curviness values differ — the + // simple branch would otherwise serialize only segments[0].curviness and drop + // every other segment's curve. + const hasExplicitCp = segments.some((s) => s.cp1 && s.cp2); + const curvinessVaries = segments.some( + (s) => (s.curviness ?? 1) !== (segments[0]?.curviness ?? 1), + ); + if ((hasExplicitCp || curvinessVaries) && waypoints.length >= 2) { const pathStr = buildCubicPathEntries(waypoints, segments).join(", "); return `{ path: [${pathStr}], type: "cubic"${arSuffix} }`; } diff --git a/packages/core/src/parsers/gsapWriter.reviewFixes.test.ts b/packages/core/src/parsers/gsapWriter.reviewFixes.test.ts new file mode 100644 index 000000000..11ed3c460 --- /dev/null +++ b/packages/core/src/parsers/gsapWriter.reviewFixes.test.ts @@ -0,0 +1,317 @@ +// fallow-ignore-file code-duplication +/** + * Correctness regressions for the SDK-cutover review (PR #1539). + * + * Each test asserts the REAL-WORLD-CORRECT result of a write op — NOT mere + * agreement between the two writers. Several of these scenarios were cases where + * both writers were identically wrong (so the recast-vs-acorn parity suite stayed + * green); these tests pin the corrected behavior. + */ +import { describe, expect, it } from "vitest"; +import { + removeKeyframeFromScript, + updateKeyframeInScript, + setArcPathInScript, + updateArcSegmentInScript, + splitAnimationsInScript, + unrollDynamicAnimations, + updateAnimationInScript, + convertToKeyframesFromScript, +} from "./gsapWriterAcorn.js"; +import { parseGsapScriptAcornForWrite } from "./gsapParserAcorn.js"; + +// ── #2 — findKfPropByPct must hit the CLOSEST keyframe, not first-within-2% ── + +const KF_DENSE = `var tl = gsap.timeline({ paused: true }); +tl.to("#box", { keyframes: { "0%": { opacity: 0 }, "49%": { opacity: 0.4 }, "50%": { opacity: 0.5 }, "100%": { opacity: 1 } }, duration: 1 }, 0);`; +const KF_DENSE_ID = "#box-to-0-visual"; + +describe("#2 — keyframe ops target the closest percentage, not first-within-tolerance", () => { + it("removing 50% removes 50% and KEEPS the neighboring 49%", () => { + const result = removeKeyframeFromScript(KF_DENSE, KF_DENSE_ID, 50); + expect(result).not.toContain('"50%"'); + expect(result).toContain('"49%"'); + expect(result).toContain("opacity: 0.4"); // 49% body intact + expect(result).toContain('"0%"'); + expect(result).toContain('"100%"'); + }); + + it("updating ~50% overwrites 50% (closest), not 49%", () => { + const result = updateKeyframeInScript(KF_DENSE, KF_DENSE_ID, 51, { opacity: 0.99 }); + // 50% is closest to 51 (dist 1) vs 49 (dist 2) — 50% gets the new value. + expect(result).toContain('"50%": { opacity: 0.99 }'); + // 49% must be untouched. + expect(result).toContain('"49%": { opacity: 0.4 }'); + }); +}); + +// ── #4 — enableArcPath on an x/y-only tween must not produce '{}' ── + +describe("#4 — enableArcPath on x/y-only vars yields a real motionPath", () => { + const XY_ONLY = `var tl = gsap.timeline({ paused: true }); +tl.to("#h", { x: 100, y: 50 }, 0);`; + + it("emits a motionPath, drops x/y, and reparses with the arc enabled", () => { + const out = setArcPathInScript(XY_ONLY, "#h-to-0-position", { + enabled: true, + autoRotate: false, + segments: [], + }); + expect(out).toContain("motionPath"); + expect(out).toContain("path:"); + // The collision bug produced a bare '{}' (no motionPath, no x/y). + expect(out).not.toMatch(/\{\s*\}/); + + // x/y are folded into the motionPath waypoints, not left as top-level vars. + const reparsed = parseGsapScriptAcornForWrite(out); + const anim = reparsed?.located[0]?.animation; + expect(anim?.arcPath?.enabled).toBe(true); + expect("x" in (anim?.properties ?? {})).toBe(false); + expect("y" in (anim?.properties ?? {})).toBe(false); + }); +}); + +// ── #5 — split midpoint uses forward baseline (earlier tweens), not reverse ── + +describe("#5 — split-spanning midpoint interpolates from the forward baseline", () => { + // A ends at x:100 (t=0..1). B runs x:?→300 over t=1..3. Split at t=2 lands at + // B's 50% point: mid = 100 + (300-100)*0.5 = 200 (NOT 150 from a 0 baseline). + const TWO_TWEENS = `var tl = gsap.timeline({ paused: true }); +tl.to("#el", { x: 100, duration: 1 }, 0); +tl.to("#el", { x: 300, duration: 2 }, 1);`; + + it("computes midpoint x = 200 (100 + (300-100)*0.5), not 150", () => { + const { script } = splitAnimationsInScript(TWO_TWEENS, { + originalId: "el", + newId: "el2", + splitTime: 2, + }); + expect(script).toContain("x: 200"); + expect(script).not.toContain("x: 150"); + // The new element's second half starts from the midpoint, not from 0. + expect(script).toContain('tl.fromTo("#el2", { x: 200 }'); + }); +}); + +// ── #9 — unroll preserves non-target statements (tl.set) per iteration ── + +describe("#9 — unrollDynamicAnimations keeps sibling statements in the loop body", () => { + const LOOP = `var tl = gsap.timeline({ paused: true }); +for (let i = 0; i < 2; i++) { + tl.set(items[i], { autoAlpha: 0 }, 0); + tl.to(items[i], { opacity: 1, duration: 1 }, 0); +}`; + + it("the tl.set initial-state lines survive after unrolling the tl.to", () => { + const parsed = parseGsapScriptAcornForWrite(LOOP); + const targetId = parsed?.located.find((l) => l.animation.method === "to")?.id ?? ""; + const out = unrollDynamicAnimations(LOOP, targetId, [ + { + selector: "#a", + keyframes: [ + { percentage: 0, properties: { opacity: 0 } }, + { percentage: 100, properties: { opacity: 1 } }, + ], + }, + { + selector: "#b", + keyframes: [ + { percentage: 0, properties: { opacity: 0 } }, + { percentage: 100, properties: { opacity: 1 } }, + ], + }, + ]); + // Both tl.set lines must remain (one per iteration) — the blanket-overwrite + // bug destroyed every non-target statement in the loop body. + expect((out.match(/tl\.set\(/g) ?? []).length).toBe(2); + expect(out).toContain("autoAlpha: 0"); + // The for-loop itself is gone (unrolled). + expect(out).not.toContain("for ("); + // The target tween is unrolled to static selectors. + expect(out).toContain('tl.to("#a"'); + expect(out).toContain('tl.to("#b"'); + }); + + it("an empty element list is a no-op, not an animation-deleting overwrite", () => { + const parsed = parseGsapScriptAcornForWrite(LOOP); + const targetId = parsed?.located.find((l) => l.animation.method === "to")?.id ?? ""; + // Empty elements has no unrolled form — overwriting the loop with zero calls + // would silently delete the animation. Writer must return the script verbatim. + expect(unrollDynamicAnimations(LOOP, targetId, [])).toBe(LOOP); + }); +}); + +// ── R3 — unsafe sibling reproduction must refuse (no-op), never corrupt/drop ── + +const TWO_EL = [ + { selector: "#a", keyframes: [{ percentage: 100, properties: { opacity: 1 } }] }, + { selector: "#b", keyframes: [{ percentage: 100, properties: { opacity: 1 } }] }, +]; +function targetToId(script: string): string { + return ( + parseGsapScriptAcornForWrite(script)?.located.find((l) => l.animation.method === "to")?.id ?? "" + ); +} +function parses(src: string): boolean { + try { + new Function(src); + return true; + } catch { + return false; + } +} + +describe("R3 — unroll refuses (no-ops) when siblings can't be safely reproduced", () => { + // R2 carried a forEach WITH a sibling tl.set to the blanket overwrite, which + // dropped the tl.set (elements start visible instead of hidden). The numeric + // index a `for` loop provides isn't available, so we now refuse instead. + it("forEach with a sibling statement is left untouched, not flattened-and-dropped", () => { + const FOREACH = `var tl = gsap.timeline({ paused: true }); +items.forEach((item, i) => { + tl.set(item, { autoAlpha: 0 }, 0); + tl.to(item, { opacity: 1, duration: 1 }, 0); +});`; + expect(unrollDynamicAnimations(FOREACH, targetToId(FOREACH), TWO_EL)).toBe(FOREACH); + }); + + // R3 #1 — object shorthand { i }: substituting the value yields `{ 0 }` (invalid). + it("object shorthand using the index refuses rather than emit invalid `{ 0 }`", () => { + const SHORTHAND = `var tl = gsap.timeline({ paused: true }); +for (let i = 0; i < 2; i++) { + tl.set(items[i], { data: { i } }, 0); + tl.to(items[i], { opacity: 1, duration: 1 }, 0); +}`; + const out = unrollDynamicAnimations(SHORTHAND, targetToId(SHORTHAND), TWO_EL); + expect(out).toBe(SHORTHAND); + expect(parses(out)).toBe(true); + }); + + // R3 #2 — a sibling that re-declares the index (nested for / shadowing). + it("a sibling shadowing the index refuses rather than rewrite the inner binding", () => { + const SHADOW = `var tl = gsap.timeline({ paused: true }); +for (let i = 0; i < 2; i++) { + tl.set(items[i], { onStart() { for (let i = 0; i < 3; i++) log(i); } }, 0); + tl.to(items[i], { opacity: 1, duration: 1 }, 0); +}`; + const out = unrollDynamicAnimations(SHADOW, targetToId(SHADOW), TWO_EL); + expect(out).toBe(SHADOW); + expect(parses(out)).toBe(true); + }); + + // The safe for-loop sibling case must still unroll (regression guard). + it("a plain for-loop with an items[i] sibling still unrolls and preserves it", () => { + const SAFE = `var tl = gsap.timeline({ paused: true }); +for (let i = 0; i < 2; i++) { + tl.set(items[i], { autoAlpha: 0 }, 0); + tl.to(items[i], { opacity: 1, duration: 1 }, 0); +}`; + const out = unrollDynamicAnimations(SAFE, targetToId(SAFE), TWO_EL); + expect((out.match(/tl\.set\(/g) ?? []).length).toBe(2); + expect(out).not.toContain("for ("); + expect(parses(out)).toBe(true); + }); +}); + +// ── R2 #5 — index substitution is AST-based: string literals are never corrupted ── + +describe("R2 — unroll substitutes real index uses but not the index char in strings", () => { + const LOOP_STR = `var tl = gsap.timeline({ paused: true }); +for (let i = 0; i < 2; i++) { + tl.set(items[i], { id: "row-i" }, 0); + tl.to(items[i], { opacity: 1, duration: 1 }, 0); +}`; + + it('rewrites items[i] per iteration but leaves the "row-i" string intact', () => { + const parsed = parseGsapScriptAcornForWrite(LOOP_STR); + const targetId = parsed?.located.find((l) => l.animation.method === "to")?.id ?? ""; + const out = unrollDynamicAnimations(LOOP_STR, targetId, [ + { selector: "#a", keyframes: [{ percentage: 100, properties: { opacity: 1 } }] }, + { selector: "#b", keyframes: [{ percentage: 100, properties: { opacity: 1 } }] }, + ]); + // Real uses of the index are substituted… + expect(out).toContain("items[0]"); + expect(out).toContain("items[1]"); + // …but the literal "row-i" is untouched (the regex bug rewrote it to "row-0"). + expect(out).toContain('"row-i"'); + expect(out).not.toContain('"row-0"'); + }); +}); + +// ── #10 — per-segment curviness survives serialization ── + +describe("#10 — updateArcSegment on a non-first segment reflects its curviness", () => { + // 3-waypoint arc, uniform curviness 1.5 → change segment 1 to curviness 3. + const ARC = `var tl = gsap.timeline({ paused: true }); +tl.to("#h", { motionPath: { path: [{x: 0, y: 0}, {x: 100, y: 50}, {x: 200, y: 0}], curviness: 1.5 }, duration: 1 }, 0);`; + + it("does not drop the second segment's curve (no longer serializes only segments[0])", () => { + const parsed = parseGsapScriptAcornForWrite(ARC); + const id = parsed?.located[0]?.id ?? ""; + const out = updateArcSegmentInScript(ARC, id, 1, { curviness: 3 }); + + // With differing per-segment curviness, the only representation that carries + // both is the cubic form. The simple form (which only emits one scalar + // curviness) would silently drop segment 1's change. + expect(out).toContain('type: "cubic"'); + + // Compare against the SAME-shape arc left at uniform curviness 1.5: the + // segment-1 control points must DIFFER, proving curviness 3 took effect. + const uniformOut = updateArcSegmentInScript(ARC, id, 1, { curviness: 1.5 }); + expect(out).not.toBe(uniformOut); + }); +}); + +// ── #11 — disableArcPath recovers NEGATIVE destination coordinates ── + +describe("#11 — disableArcPath restores negative waypoint coords", () => { + it("restores x:-120, y:-40 on the flattened tween", () => { + const XY_NEG = `var tl = gsap.timeline({ paused: true }); +tl.to("#h", { x: -120, y: -40, duration: 1 }, 0);`; + const enabled = setArcPathInScript(XY_NEG, "#h-to-0-position", { + enabled: true, + autoRotate: false, + segments: [], + }); + const reEnabled = parseGsapScriptAcornForWrite(enabled); + const id = reEnabled?.located[0]?.id ?? ""; + const disabled = setArcPathInScript(enabled, id, { + enabled: false, + autoRotate: false, + segments: [], + }); + // The negative destination must come back — the UnaryExpression bug lost it. + expect(disabled).toContain("x: -120"); + expect(disabled).toContain("y: -40"); + expect(disabled).not.toContain("motionPath"); + }); +}); + +// ── #7 — updating ease on a keyframe tween routes to easeEach, not top-level ── + +describe("#7 — ease update on a keyframe tween targets keyframes.easeEach", () => { + const KF = `var tl = gsap.timeline({ paused: true }); +tl.to(".a", { keyframes: { "0%": { x: 0 }, "100%": { x: 100 } }, duration: 1, ease: "none" }, 0);`; + + it("writes easeEach (per-keyframe), not a no-op top-level ease", () => { + const id = parseGsapScriptAcornForWrite(KF)?.located[0]?.id ?? ""; + const out = updateAnimationInScript(KF, id, { ease: "power2.inOut" }); + expect(out).toContain('easeEach: "power2.inOut"'); + // The original top-level `ease: "none"` is untouched (no second top-level ease). + expect((out.match(/ease: "power2.inOut"/g) ?? []).length).toBe(0); + }); +}); + +// ── #8 — convertToKeyframes preserves builtin vars like `delay` ── + +describe("#8 — convertToKeyframes keeps delay (was dropped, shifting start time)", () => { + const DELAY = `var tl = gsap.timeline({ paused: true }); +tl.to(".a", { x: 100, duration: 1, delay: 0.3 }, 0);`; + + it("preserves delay on the converted vars object", () => { + const id = parseGsapScriptAcornForWrite(DELAY)?.located[0]?.id ?? ""; + const out = convertToKeyframesFromScript(DELAY, id); + expect(out).toContain("keyframes:"); + expect(out).toContain("delay: 0.3"); // was lost → tween started 0.3s early + expect(out).toContain("duration: 1"); + }); +}); diff --git a/packages/core/src/parsers/gsapWriterAcorn.ts b/packages/core/src/parsers/gsapWriterAcorn.ts index 0a996b686..cfd459cc7 100644 --- a/packages/core/src/parsers/gsapWriterAcorn.ts +++ b/packages/core/src/parsers/gsapWriterAcorn.ts @@ -28,6 +28,10 @@ import type { PropertyGroupName } from "./gsapConstants.js"; import type { SplitAnimationsOptions, SplitAnimationsResult } from "./gsapParser.js"; import * as acornWalk from "acorn-walk"; +// acorn ESTree nodes are structurally untyped here; mirror gsapParserAcorn.ts / +// gsapInline.ts rather than re-deriving the full ESTree union for every access. +type Node = any; + // ── Code generation helpers ────────────────────────────────────────────────── // Local serializer for the tween-statement path, which may carry boolean/object @@ -73,15 +77,15 @@ function buildTweenStatementCode(timelineVar: string, anim: Omit= 0; i--) { if (ancestors[i]?.type === "ExpressionStatement") return ancestors[i]; } @@ -98,11 +108,11 @@ function findEnclosingExpressionStatement(ancestors: any[]): any | null { } /** Find the VariableDeclaration statement for `tl = gsap.timeline(...)`. */ -function findTimelineDeclarationStatement(ast: any, timelineVar: string): any | null { - let found: any = null; +function findTimelineDeclarationStatement(ast: Node, timelineVar: string): Node | null { + let found: Node = null; acornWalk.simple(ast, { // fallow-ignore-next-line complexity - VariableDeclaration(node: any) { + VariableDeclaration(node: Node) { if (found) return; for (const decl of node.declarations ?? []) { if ( @@ -126,7 +136,7 @@ function findTimelineDeclarationStatement(ast: any, timelineVar: string): any | * Remove a property from a properties array, handling its comma. * `editableProps` must be the isObjectProperty-filtered subset in source order. */ -function removeProp(ms: MagicString, propNode: any, editableProps: any[]): void { +function removeProp(ms: MagicString, propNode: Node, editableProps: Node[]): void { const idx = editableProps.indexOf(propNode); if (idx === -1) return; if (editableProps.length === 1) { @@ -156,7 +166,7 @@ function overwriteVarsArg(ms: MagicString, call: TweenCallInfo, objCode: string) * Update a property value if it exists, or append a new key: val before the * closing `}`. Call with the full ObjectExpression node. */ -function upsertProp(ms: MagicString, objNode: any, key: string, value: unknown): void { +function upsertProp(ms: MagicString, objNode: Node, key: string, value: unknown): void { if (objNode?.type !== "ObjectExpression") return; const existing = findPropertyNode(objNode, key); if (existing) { @@ -208,7 +218,7 @@ function isEditableVarKey(key: string): boolean { * the entries plus the set of keys it kept, so callers can append new keys. */ function preservedEntries( - objNode: any, + objNode: Node, source: string, drop: (key: string) => boolean, overrides: Record, @@ -240,7 +250,7 @@ function preservedEntries( */ function reconcileEditableProps( ms: MagicString, - objNode: any, + objNode: Node, source: string, newProps: Record, nonEditableOverrides?: Record, @@ -260,7 +270,7 @@ function reconcileEditableProps( // ── Insertion helpers ───────────────────────────────────────────────────────── /** Traverse callee.object chain to check if a call ultimately roots at timelineVar. */ -function isTimelineRooted(node: any, timelineVar: string): boolean { +function isTimelineRooted(node: Node, timelineVar: string): boolean { if (node?.type === "Identifier") return node.name === timelineVar; if (node?.type === "CallExpression") return isTimelineRooted(node.callee?.object, timelineVar); return false; @@ -272,8 +282,9 @@ function isTimelineRooted(node: any, timelineVar: string): boolean { * not emit `tl.xxx()` calls in that case as `tl` would be undefined at render. */ function findInsertionPoint(parsed: ParsedGsapAcornForWrite): number | null { - if (parsed.located.length > 0) { - const lastCall = parsed.located[parsed.located.length - 1]!.call; + const lastLocated = parsed.located[parsed.located.length - 1]; + if (lastLocated) { + const lastCall = lastLocated.call; const exprStmt = findEnclosingExpressionStatement(lastCall.ancestors); return exprStmt?.end ?? lastCall.node.end; } @@ -314,7 +325,12 @@ export function updateAnimationInScript( upsertProp(ms, call.varsArg, "duration", updates.duration); } if (updates.ease !== undefined) { - upsertProp(ms, call.varsArg, "ease", updates.ease); + // For a keyframe tween, easing lives at keyframes.easeEach (per-keyframe), + // not a top-level ease. Writing top-level ease would leave the per-keyframe + // easing unchanged — the user's edit would silently do nothing. + const kfNode = keyframesObjectNode(call.varsArg); + if (kfNode) upsertProp(ms, kfNode, "easeEach", updates.ease); + else upsertProp(ms, call.varsArg, "ease", updates.ease); } if (updates.extras) { for (const [key, value] of Object.entries(updates.extras)) { @@ -527,7 +543,7 @@ function conversionEndpoints(animation: GsapAnimation): { } /** Collect preserved (non-editable) `key: value` entries from the original vars node. */ -function preservedVarsEntries(varsNode: any, source: string): string[] { +function preservedVarsEntries(varsNode: Node, source: string): string[] { const entries: string[] = []; if (varsNode?.type !== "ObjectExpression") return entries; for (const prop of varsNode.properties ?? []) { @@ -540,7 +556,7 @@ function preservedVarsEntries(varsNode: any, source: string): string[] { } /** Build the rebuilt vars-object code for a converted flat tween. */ -function buildConvertedVarsCode(animation: GsapAnimation, varsNode: any, source: string): string { +function buildConvertedVarsCode(animation: GsapAnimation, varsNode: Node, source: string): string { const { fromProps, toProps } = conversionEndpoints(animation); const easeEach = animation.ease; const easeEachEntry = easeEach ? `, easeEach: ${JSON.stringify(easeEach)}` : ""; @@ -554,8 +570,8 @@ function buildConvertedVarsCode(animation: GsapAnimation, varsNode: any, source: function convertMethodToTo( ms: MagicString, animation: GsapAnimation, - call: any, - varsNode: any, + call: Node, + varsNode: Node, ): void { if (animation.method !== "from" && animation.method !== "fromTo") return; const calleeProp = call.node.callee?.property; @@ -564,7 +580,7 @@ function convertMethodToTo( if (animation.method === "fromTo" && call.fromArg) ms.remove(call.fromArg.start, varsNode.start); } -function convertFlatTweenToKeyframes(script: string, target: any): string { +function convertFlatTweenToKeyframes(script: string, target: Node): string { const animation: GsapAnimation = target.animation; if (animation.keyframes || animation.method === "set") return script; const call = target.call; @@ -606,8 +622,8 @@ function recordToCode(record: Record): string { } /** Percentage-keyed property nodes of a keyframes ObjectExpression, in source order. */ -function percentagePropsOf(kfNode: any): any[] { - return (kfNode.properties ?? []).filter((p: any) => { +function percentagePropsOf(kfNode: Node): Node[] { + return (kfNode.properties ?? []).filter((p: Node) => { if (!isObjectProperty(p)) return false; const key = propKeyName(p); return typeof key === "string" && PERCENTAGE_KEY_RE.test(key); @@ -618,7 +634,7 @@ const LITERAL_NODE_TYPES = new Set(["Literal", "NumericLiteral", "StringLiteral" /** Read one value node: a number/string literal, a negative number, or raw source. */ // fallow-ignore-next-line complexity -function readValueNode(v: any, source: string): number | string { +function readValueNode(v: Node, source: string): number | string { if ( LITERAL_NODE_TYPES.has(v?.type) && (typeof v.value === "number" || typeof v.value === "string") @@ -641,7 +657,7 @@ function readValueNode(v: any, source: string): number | string { * preserved as `__raw:` so serializeValue round-trips it verbatim. * Keyframe values are literals in practice, so the raw fallback is rarely hit. */ -function valueNodeToRecord(valueNode: any, source: string): Record { +function valueNodeToRecord(valueNode: Node, source: string): Record { const record: Record = {}; if (valueNode?.type !== "ObjectExpression") return record; for (const prop of valueNode.properties ?? []) { @@ -666,7 +682,7 @@ function recordHasAuto(record: Record): boolean { * records (never a separate splice). */ function autoEndpointOverwrites( - kfNode: any, + kfNode: Node, source: string, percentage: number, properties: Record, @@ -675,7 +691,7 @@ function autoEndpointOverwrites( if (percentage <= 0 || percentage >= 100) return result; const pctProps = percentagePropsOf(kfNode); const allPcts = pctProps - .map((p: any) => percentageFromKey(propKeyName(p) ?? "")) + .map((p: Node) => percentageFromKey(propKeyName(p) ?? "")) .filter((n: number) => !Number.isNaN(n) && n !== percentage) .sort((a: number, b: number) => a - b); const leftNeighbor = allPcts.filter((p: number) => p < percentage).pop(); @@ -683,7 +699,7 @@ function autoEndpointOverwrites( for (const endPct of [0, 100]) { const isNeighbor = endPct === 0 ? leftNeighbor === 0 : rightNeighbor === 100; if (!isNeighbor) continue; - const endProp = pctProps.find((p: any) => percentageFromKey(propKeyName(p) ?? "") === endPct); + const endProp = pctProps.find((p: Node) => percentageFromKey(propKeyName(p) ?? "") === endPct); if (!endProp) continue; const rec = valueNodeToRecord(endProp.value, source); if (!recordHasAuto(rec)) continue; @@ -692,17 +708,27 @@ function autoEndpointOverwrites( return result; } -function findKfPropByPct(kfNode: any, percentage: number): { prop: any; idx: number } | null { +function findKfPropByPct(kfNode: Node, percentage: number): { prop: Node; idx: number } | null { + // Match the CLOSEST keyframe within tolerance, not the first one within range. + // Keyframes at e.g. 0/49/50/100 are all valid (the SDK dedups to a unique + // match at TOLERANCE=0.001 upstream); picking the first-within-PCT_TOLERANCE=2 + // would hit 49% when the caller meant 50%. Tie-break on the earliest index so + // the choice stays deterministic. const props = kfNode.properties ?? []; + let best: { prop: Node; idx: number } | null = null; + let bestDist = Number.POSITIVE_INFINITY; for (let i = 0; i < props.length; i++) { const prop = props[i]; if (!isObjectProperty(prop)) continue; const key = propKeyName(prop); - if (typeof key === "string" && Math.abs(percentageFromKey(key) - percentage) <= PCT_TOLERANCE) { - return { prop, idx: i }; + if (typeof key !== "string") continue; + const dist = Math.abs(percentageFromKey(key) - percentage); + if (dist <= PCT_TOLERANCE && dist < bestDist) { + best = { prop, idx: i }; + bestDist = dist; } } - return null; + return best; } export function updateKeyframeInScript( @@ -737,7 +763,7 @@ export function updateKeyframeInScript( * ease when the op omits one); otherwise it's just the new props. */ function buildTargetRecord( - existing: { prop: any; idx: number } | null, + existing: { prop: Node; idx: number } | null, source: string, properties: Record, ease: string | undefined, @@ -763,7 +789,7 @@ function buildTargetRecord( * nothing changes (so the caller emits no overwrite for it). */ function backfilledSiblingRecord( - valueNode: any, + valueNode: Node, source: string, newPropKeys: string[], backfillDefaults: Record, @@ -784,7 +810,7 @@ function backfilledSiblingRecord( function locateWithKeyframes( script: string, animationId: string, -): { script: string; parsed: ParsedGsapAcornForWrite; target: any; kfNode: any } | null { +): { script: string; parsed: ParsedGsapAcornForWrite; target: Node; kfNode: Node } | null { const parsed = parseGsapScriptAcornForWrite(script); if (!parsed) return null; // Converting from()/fromTo() to to() rewrites the content-derived id; match @@ -803,7 +829,7 @@ function locateWithKeyframes( function ensureKeyframesNode( script: string, animationId: string, -): { script: string; parsed: ParsedGsapAcornForWrite; target: any; kfNode: any } | null { +): { script: string; parsed: ParsedGsapAcornForWrite; target: Node; kfNode: Node } | null { const direct = locateWithKeyframes(script, animationId); if (direct) return direct; @@ -821,11 +847,11 @@ function ensureKeyframesNode( * target keyframe and any node already being overwritten as an `_auto` endpoint. */ function collectBackfillOverwrites( - kfNode: any, + kfNode: Node, src: string, properties: Record, backfillDefaults: Record | undefined, - skip: { existingProp: any; endpoints: Map }, + skip: { existingProp: Node; endpoints: Map }, ): Map> { const result = new Map>(); if (!backfillDefaults) return result; @@ -892,13 +918,13 @@ export function addKeyframeToScript( /** Insert a brand-new `"pct%": {...}` property in sorted order. */ function insertNewKeyframe( ms: MagicString, - kfNode: any, + kfNode: Node, percentage: number, pctKey: string, valueCode: string, ): void { - const allProps = (kfNode.properties ?? []).filter((p: any) => isObjectProperty(p)); - let insertBeforeProp: any = null; + const allProps = (kfNode.properties ?? []).filter((p: Node) => isObjectProperty(p)); + let insertBeforeProp: Node = null; for (const prop of allProps) { const key = propKeyName(prop); if (typeof key === "string" && percentageFromKey(key) > percentage) { @@ -924,7 +950,7 @@ function insertNewKeyframe( */ function collapseKeyframesToFlat( ms: MagicString, - varsNode: any, + varsNode: Node, source: string, remainingRecord: Record, ): void { @@ -962,12 +988,13 @@ export function removeKeyframeFromScript( // node — the two edits would overlap. const remaining = percentagePropsOf(kfNode).filter((p) => p !== match.prop); if (remaining.length < 2) { - const record = remaining.length === 1 ? valueNodeToRecord(remaining[0]!.value, script) : {}; + const sole = remaining[0]; + const record = sole ? valueNodeToRecord(sole.value, script) : {}; collapseKeyframesToFlat(ms, target.call.varsArg, script, record); return ms.toString(); } - const allProps = (kfNode.properties ?? []).filter((p: any) => isObjectProperty(p)); + const allProps = (kfNode.properties ?? []).filter((p: Node) => isObjectProperty(p)); removeProp(ms, match.prop, allProps); return ms.toString(); } @@ -987,7 +1014,7 @@ export function removePropertyFromAnimation( if (!objNode) return script; const propNode = findPropertyNode(objNode, property); if (!propNode) return script; - const allProps = (objNode.properties ?? []).filter((p: any) => isObjectProperty(p)); + const allProps = (objNode.properties ?? []).filter((p: Node) => isObjectProperty(p)); const ms = new MagicString(script); removeProp(ms, propNode, allProps); return ms.toString(); @@ -1007,24 +1034,35 @@ export function removeAllKeyframesFromScript(script: string, animationId: string if (!kfs || kfs.length === 0) return script; const sorted = [...kfs].sort((a, b) => a.percentage - b.percentage); - const collapse = target.call.method === "from" ? sorted[0]! : sorted[sorted.length - 1]!; + const collapse = target.call.method === "from" ? sorted[0] : sorted[sorted.length - 1]; + if (!collapse) return script; - // Flat vars = existing top-level props, then collapse-keyframe props (these - // win; skip the per-keyframe `ease` key), then duration/ease/extras. Drops - // keyframes + easeEach by reconstruction. - const flat: Record = { ...target.animation.properties }; + const ms = new MagicString(script); + overwriteVarsArg( + ms, + target.call, + buildVarsObjectCode(buildCollapsedFlatVars(target.animation, collapse)), + ); + return ms.toString(); +} + +// Flat vars for a tween collapsing its keyframes onto one stop: existing +// top-level props, then the collapse keyframe's props (skip per-keyframe +// `ease`), then duration/ease/extras. Drops keyframes + easeEach by omission. +function buildCollapsedFlatVars( + animation: GsapAnimation, + collapse: { properties: Record }, +): Record { + const flat: Record = { ...animation.properties }; for (const [k, v] of Object.entries(collapse.properties)) { if (k !== "ease") flat[k] = v; } - if (target.animation.duration !== undefined) flat.duration = target.animation.duration; - if (target.animation.ease) flat.ease = target.animation.ease; - for (const [k, v] of Object.entries(target.animation.extras ?? {})) { + if (animation.duration !== undefined) flat.duration = animation.duration; + if (animation.ease) flat.ease = animation.ease; + for (const [k, v] of Object.entries(animation.extras ?? {})) { if (typeof v === "number" || typeof v === "string") flat[k] = v; } - - const ms = new MagicString(script); - overwriteVarsArg(ms, target.call, buildVarsObjectCode(flat)); - return ms.toString(); + return flat; } /** Build the full replacement vars object for a tween being converted to keyframes. */ @@ -1032,18 +1070,18 @@ function buildKeyframesVarsCode( animation: GsapAnimation, fromProps: Record, toProps: Record, + varsNode: Node, + source: string, ): string { const fromEntries = Object.entries(fromProps).map(([k, v]) => `${safeKey(k)}: ${valueToCode(v)}`); const toEntries = Object.entries(toProps).map(([k, v]) => `${safeKey(k)}: ${valueToCode(v)}`); const easeEntry = animation.ease ? `, easeEach: ${JSON.stringify(animation.ease)}` : ""; const kfCode = `{ "0%": { ${fromEntries.join(", ")} }, "100%": { ${toEntries.join(", ")} }${easeEntry} }`; - const parts: string[] = [`keyframes: ${kfCode}`]; - if (animation.duration !== undefined) parts.push(`duration: ${valueToCode(animation.duration)}`); + // Preserve every non-editable key (duration/delay/callbacks/stagger/yoyo/…) + // verbatim from source — rebuilding from the animation object alone dropped + // `delay` (not a GsapAnimation field), shifting the tween's start time. + const parts: string[] = [`keyframes: ${kfCode}`, ...preservedVarsEntries(varsNode, source)]; if (animation.ease) parts.push(`ease: "none"`); - for (const [k, v] of Object.entries(animation.extras ?? {})) { - if (typeof v === "number" || typeof v === "string") - parts.push(`${safeKey(k)}: ${valueToCode(v)}`); - } return `{ ${parts.join(", ")} }`; } @@ -1073,7 +1111,11 @@ export function convertToKeyframesFromScript( if (call.method === "fromTo" && call.fromArg) { ms.remove(call.fromArg.start, call.varsArg.start); } - overwriteVarsArg(ms, call, buildKeyframesVarsCode(animation, fromProps, toProps)); + overwriteVarsArg( + ms, + call, + buildKeyframesVarsCode(animation, fromProps, toProps, call.varsArg, script), + ); return ms.toString(); } @@ -1116,6 +1158,9 @@ export function materializeKeyframesFromScript( easeEach?: string, resolvedSelector?: string, ): string { + // An empty keyframe list has no materialized form — rebuilding vars with an + // empty keyframes object would empty the animation. No-op instead. + if (keyframes.length === 0) return script; const parsed = parseGsapScriptAcornForWrite(script); if (!parsed) return script; const target = parsed.located.find((l) => l.id === animationId); @@ -1146,7 +1191,7 @@ export function materializeKeyframesFromScript( const eachProp = findPropertyNode(call.varsArg, "easeEach"); if (eachProp) { - const allProps = (call.varsArg.properties ?? []).filter((p: any) => isObjectProperty(p)); + const allProps = (call.varsArg.properties ?? []).filter((p: Node) => isObjectProperty(p)); removeProp(ms, eachProp, allProps); } @@ -1226,7 +1271,8 @@ function assignTransformOrigin(groupProps: Map): vo largestGroup = group; } } - if (largestGroup) groupProps.get(largestGroup)!.push("transformOrigin"); + const largest = largestGroup ? groupProps.get(largestGroup) : undefined; + if (largest) largest.push("transformOrigin"); } function filterGroupKeyframes( @@ -1336,10 +1382,54 @@ export function splitIntoPropertyGroupsFromScript( // ── Label write ops ─────────────────────────────────────────────────────────── +/** True when `expr` is `tl.(…)` rooted at the timeline var. */ +function isTimelineMethodCall(expr: Node, timelineVar: string, method: string): boolean { + return ( + expr?.type === "CallExpression" && + expr.callee?.type === "MemberExpression" && + isTimelineRooted(expr.callee.object, timelineVar) && + expr.callee.property?.name === method + ); +} + +/** True when `expr` is `tl.addLabel("", …)` rooted at the timeline var. */ +function isAddLabelCall(expr: Node, timelineVar: string, name: string): boolean { + const firstArg = expr?.arguments?.[0]; + return ( + isTimelineMethodCall(expr, timelineVar, "addLabel") && + firstArg?.type === "Literal" && + firstArg.value === name + ); +} + +/** Every `tl.addLabel("", …)` ExpressionStatement in the script. */ +function findLabelStatements(parsed: ParsedGsapAcornForWrite, name: string): Node[] { + const targets: Node[] = []; + acornWalk.simple(parsed.ast, { + ExpressionStatement(node: Node) { + if (isAddLabelCall(node.expression, parsed.timelineVar, name)) targets.push(node); + }, + }); + return targets; +} + export function addLabelToScript(script: string, name: string, position: number): string { const parsed = parseGsapScriptAcornForWrite(script); if (!parsed) return script; + // If the label already exists, MOVE it (overwrite its position) rather than + // appending a duplicate. Two same-named addLabel statements make removeLabel + // over-remove — it deletes every match, including a pre-existing label the + // user never touched. + const existing = findLabelStatements(parsed, name)[0]; + if (existing) { + const ms = new MagicString(script); + const posArg = existing.expression.arguments?.[1]; + if (posArg) ms.overwrite(posArg.start, posArg.end, valueToCode(position)); + else ms.appendLeft(existing.expression.end - 1, `, ${valueToCode(position)}`); + return ms.toString(); + } + const insertionPoint = findInsertionPoint(parsed); if (insertionPoint === null) return script; @@ -1353,24 +1443,7 @@ export function removeLabelFromScript(script: string, name: string): string { const parsed = parseGsapScriptAcornForWrite(script); if (!parsed) return script; - const targets: any[] = []; - acornWalk.simple(parsed.ast, { - // fallow-ignore-next-line complexity - ExpressionStatement(node: any) { - const expr = node.expression; - if ( - expr?.type === "CallExpression" && - expr.callee?.type === "MemberExpression" && - isTimelineRooted(expr.callee.object, parsed.timelineVar) && - expr.callee.property?.name === "addLabel" && - expr.arguments?.[0]?.type === "Literal" && - expr.arguments[0].value === name - ) { - targets.push(node); - } - }, - }); - + const targets = findLabelStatements(parsed, name); if (!targets.length) return script; const ms = new MagicString(script); @@ -1388,10 +1461,10 @@ export function removeLabelFromScript(script: string, name: string): string { * Remove a set of properties from an ObjectExpression in a single pass. * Groups consecutive marked props into blocks to avoid overlapping remove ranges. */ -function removePropsByKey(ms: MagicString, objNode: any, keys: Set): void { +function removePropsByKey(ms: MagicString, objNode: Node, keys: Set): void { if (objNode?.type !== "ObjectExpression") return; const allProps = (objNode.properties ?? []).filter(isObjectProperty); - const marked = allProps.map((p: any) => keys.has(propKeyName(p) ?? "")); + const marked = allProps.map((p: Node) => keys.has(propKeyName(p) ?? "")); let i = 0; while (i < allProps.length) { if (!marked[i]) { @@ -1404,7 +1477,11 @@ function removePropsByKey(ms: MagicString, objNode: any, keys: Set): voi } } -function blockRemoveRange(allProps: any[], blockStart: number, blockEnd: number): [number, number] { +function blockRemoveRange( + allProps: Node[], + blockStart: number, + blockEnd: number, +): [number, number] { if (blockStart === 0 && blockEnd === allProps.length) return [allProps[0].start, allProps[allProps.length - 1].end]; if (blockStart === 0) return [allProps[0].start, allProps[blockEnd].start]; @@ -1412,16 +1489,35 @@ function blockRemoveRange(allProps: any[], blockStart: number, blockEnd: number) } // fallow-ignore-next-line complexity -function readLastWaypointXY(mpVal: any): { x: number | null; y: number | null } { +function readLastWaypointXY(mpVal: Node): { x: number | null; y: number | null } { if (mpVal?.type !== "ObjectExpression") return { x: null, y: null }; const pathProp = findPropertyNode(mpVal, "path"); if (pathProp?.value?.type !== "ArrayExpression") return { x: null, y: null }; - const elems: any[] = pathProp.value.elements ?? []; + const elems: Node[] = pathProp.value.elements ?? []; const last = elems[elems.length - 1]; if (last?.type !== "ObjectExpression") return { x: null, y: null }; - const xRaw = findPropertyNode(last, "x")?.value?.value; - const yRaw = findPropertyNode(last, "y")?.value?.value; - return { x: typeof xRaw === "number" ? xRaw : null, y: typeof yRaw === "number" ? yRaw : null }; + return { + x: readNumericLiteralNode(findPropertyNode(last, "x")?.value), + y: readNumericLiteralNode(findPropertyNode(last, "y")?.value), + }; +} + +/** + * Read a numeric value node — a plain numeric literal or a unary-minus negative + * literal (e.g. `-120`). Returns null for anything non-numeric. Without the + * UnaryExpression branch, negative waypoint coords (parsed as a UnaryExpression + * with no `.value`) would be lost when disabling an arc path. + */ +function readNumericLiteralNode(v: Node): number | null { + if (LITERAL_NODE_TYPES.has(v?.type) && typeof v.value === "number") return v.value; + if ( + v?.type === "UnaryExpression" && + v.operator === "-" && + typeof v.argument?.value === "number" + ) { + return -v.argument.value; + } + return null; } function disableArcPath(ms: MagicString, call: TweenCallInfo): boolean { @@ -1442,7 +1538,7 @@ function disableArcPath(ms: MagicString, call: TweenCallInfo): boolean { return true; } -function stripXYFromKeyframes(ms: MagicString, kfPropNode: any): void { +function stripXYFromKeyframes(ms: MagicString, kfPropNode: Node): void { if (kfPropNode?.value?.type !== "ObjectExpression") return; const xyKeys = new Set(["x", "y"]); for (const pctProp of (kfPropNode.value.properties ?? []).filter(isObjectProperty)) { @@ -1470,7 +1566,20 @@ function enableArcPath( segments, autoRotate: config.autoRotate, }); - upsertProp(ms, call.varsArg, "motionPath", `__raw:${motionPathCode}`); + const vars = call.varsArg; + if (vars?.type !== "ObjectExpression") return false; + // Insert motionPath right after the opening `{` (appendRight at start+1) so the + // insertion point can never coincide with the end boundary of the x/y removal + // range. upsertProp would appendLeft at `end - 1`, which collides with a + // remove-range that ends at the same offset when x/y are the only props — + // MagicString then discards the append and the output loses everything. + const editable = (vars.properties ?? []).filter(isObjectProperty); + const survivesRemoval = editable.some((p: Node) => { + const k = propKeyName(p); + return k !== "x" && k !== "y"; + }); + const sep = survivesRemoval ? ", " : ""; + ms.appendRight(vars.start + 1, ` motionPath: ${motionPathCode}${sep}`); stripXYFromKeyframes(ms, findPropertyNode(call.varsArg, "keyframes")); removePropsByKey(ms, call.varsArg, new Set(["x", "y"])); return true; @@ -1507,9 +1616,10 @@ export function updateArcSegmentInScript( if (!animation.arcPath?.enabled) return script; const segments = [...animation.arcPath.segments]; - if (segmentIndex < 0 || segmentIndex >= segments.length) return script; + const existingSeg = segments[segmentIndex]; + if (segmentIndex < 0 || segmentIndex >= segments.length || !existingSeg) return script; - segments[segmentIndex] = { ...segments[segmentIndex]!, ...update }; + segments[segmentIndex] = { ...existingSeg, ...update }; const waypoints = extractArcWaypoints(animation); if (waypoints.length < 2) return script; @@ -1573,10 +1683,11 @@ function insertInheritedStateSetInScript( const code = `${parsed.timelineVar}.set(${JSON.stringify(selector)}, { ${props} }, ${position});`; const ms = new MagicString(script); const tlDecl = findTimelineDeclarationStatement(parsed.ast, parsed.timelineVar); + const firstLocated = parsed.located[0]; if (tlDecl) { ms.appendLeft(tlDecl.end, "\n" + code); - } else if (parsed.located.length > 0) { - const firstCall = parsed.located[0]!.call; + } else if (firstLocated) { + const firstCall = firstLocated.call; const exprStmt = findEnclosingExpressionStatement(firstCall.ancestors); const insertAt = exprStmt?.start ?? firstCall.node.start; ms.prependLeft(insertAt, code + "\n"); @@ -1586,7 +1697,147 @@ function insertInheritedStateSetInScript( return ms.toString(); } +/** + * Compute, in forward (timeline) order, the inherited-props baseline available + * BEFORE each matching tween, plus the final cumulative state at the split point. + * A tween contributes to later baselines when it ends at/before the split (full + * props or last keyframe), spans the split via keyframes (kfs at/before split), + * or spans the split as a flat tween (its interpolated midpoint). Decoupled from + * the reverse write loop so the spanning-tween midpoint reads earlier tweens. + */ // fallow-ignore-next-line complexity +function computeForwardBaselines( + matching: GsapAnimation[], + splitTime: number, +): { before: Array>; final: Record } { + const before: Array> = []; + const acc: Record = {}; + for (const anim of matching) { + before.push({ ...acc }); + const pos = typeof anim.position === "number" ? anim.position : 0; + const dur = anim.duration ?? 0; + const animEnd = pos + dur; + + if (anim.keyframes) { + const kfs = anim.keyframes.keyframes; + if (pos >= splitTime) { + // Moves wholly to the new element — contributes nothing to the baseline. + } else if (animEnd > splitTime) { + for (const kf of kfs) { + const kfTime = pos + (kf.percentage / 100) * dur; + if (kfTime <= splitTime) { + for (const [k, v] of Object.entries(kf.properties)) acc[k] = v; + } + } + } else { + const lastKf = kfs[kfs.length - 1]; + if (lastKf) { + for (const [k, v] of Object.entries(lastKf.properties)) acc[k] = v; + } + } + continue; + } + + if (animEnd <= splitTime) { + for (const [k, v] of Object.entries(anim.properties)) acc[k] = v; + continue; + } + + if (pos >= splitTime) continue; + + // Flat tween spanning the split — its midpoint becomes the inherited value. + const progress = dur > 0 ? (splitTime - pos) / dur : 0; + const fromSource = anim.fromProperties ?? acc; + for (const [k, v] of Object.entries(anim.properties)) { + if (typeof v !== "number") { + acc[k] = v; + continue; + } + const fromVal = typeof fromSource[k] === "number" ? (fromSource[k] as number) : 0; + acc[k] = fromVal + (v - fromVal) * progress; + } + } + return { before, final: { ...acc } }; +} + +// Split one tween that straddles the split point: trim the original to the +// first half (interpolated midpoint as its new end) and add a fromTo for the +// second half on the new element. `fromSource` is the forward baseline. +function buildSpanningSplit( + result: string, + anim: GsapAnimation, + pos: number, + dur: number, + fromSource: Record, + ctx: { splitTime: number; newSelector: string; newElementStart: number }, +): string { + const progress = dur > 0 ? (ctx.splitTime - pos) / dur : 0; + const midProps: Record = {}; + for (const [k, v] of Object.entries(anim.properties)) { + if (typeof v !== "number") { + midProps[k] = v; + continue; + } + const fromVal = typeof fromSource[k] === "number" ? (fromSource[k] as number) : 0; + midProps[k] = fromVal + (v - fromVal) * progress; + } + const trimmed = updateAnimationInScript(result, anim.id, { + duration: ctx.splitTime - pos, + properties: midProps, + }); + return addAnimationToScript(trimmed, { + targetSelector: ctx.newSelector, + method: "fromTo", + position: ctx.newElementStart, + duration: pos + dur - ctx.splitTime, + properties: { ...anim.properties }, + fromProperties: { ...midProps }, + ease: anim.ease, + extras: anim.extras, + }).script; +} + +type SplitCtx = { + splitTime: number; + originalSelector: string; + newSelector: string; + newElementStart: number; +}; + +// Decide what one matching tween does at the split point: move to the new +// element (wholly after), stay (wholly before / keyframes before), get skipped +// (keyframes spanning), or get interpolated in half (spanning). Returns the +// updated script; pushes any skip reason into `skippedSelectors`. +function applyTweenSplit( + result: string, + anim: GsapAnimation, + baselineBefore: Record, + ctx: SplitCtx, + skippedSelectors: string[], +): string { + const pos = typeof anim.position === "number" ? anim.position : 0; + const dur = anim.duration ?? 0; + const animEnd = pos + dur; + + if (anim.keyframes) { + if (pos >= ctx.splitTime) + return updateAnimationSelectorInScript(result, anim.id, ctx.newSelector); + if (animEnd > ctx.splitTime) { + skippedSelectors.push(`${ctx.originalSelector} (keyframes spanning split)`); + } + // Inherited-state for kf tweens is handled by computeForwardBaselines. + return result; + } + // Wholly before the split — kept on the original element. + if (animEnd <= ctx.splitTime) return result; + // Wholly after — move to the new element. + if (pos >= ctx.splitTime) + return updateAnimationSelectorInScript(result, anim.id, ctx.newSelector); + // Spans the split — interpolate the midpoint from the FORWARD baseline. + const fromSource = anim.fromProperties ?? baselineBefore; + return buildSpanningSplit(result, anim, pos, dur, fromSource, ctx); +} + export function splitAnimationsInScript( script: string, opts: SplitAnimationsOptions, @@ -1611,92 +1862,34 @@ export function splitAnimationsInScript( let result = script; const newElementStart = opts.splitTime; - const inheritedProps: Record = {}; + + // Forward pre-pass: compute the inherited-props baseline available BEFORE each + // matching tween, in source/timeline order. The write loop below runs in + // REVERSE (so updateAnimationSelectorInScript's selector edits can't shift the + // count-based IDs of not-yet-processed tweens), but the spanning-tween midpoint + // interpolation needs the baseline from EARLIER tweens — which a reverse + // accumulator hasn't seen yet. Decoupling the two fixes the wrong midpoint. + const { before: baselineBefore, final: finalInheritedProps } = computeForwardBaselines( + matching, + opts.splitTime, + ); // Reverse iteration: updateAnimationSelectorInScript mutates selectors which // can shift count-based ID suffixes for later animations. + const ctx = { splitTime: opts.splitTime, originalSelector, newSelector, newElementStart }; for (let i = matching.length - 1; i >= 0; i--) { - const anim = matching[i]!; - const pos = typeof anim.position === "number" ? anim.position : 0; - const dur = anim.duration ?? 0; - const animEnd = pos + dur; - - if (anim.keyframes) { - if (pos >= opts.splitTime) { - result = updateAnimationSelectorInScript(result, anim.id, newSelector); - } else if (animEnd > opts.splitTime) { - skippedSelectors.push(`${originalSelector} (keyframes spanning split)`); - const kfs = anim.keyframes.keyframes; - for (const kf of kfs) { - const kfTime = pos + (kf.percentage / 100) * dur; - if (kfTime <= opts.splitTime) { - for (const [k, v] of Object.entries(kf.properties)) { - inheritedProps[k] = v; - } - } - } - } else { - const kfs = anim.keyframes.keyframes; - if (kfs.length > 0) { - for (const [k, v] of Object.entries(kfs[kfs.length - 1]!.properties)) { - inheritedProps[k] = v; - } - } - } - continue; - } - - if (animEnd <= opts.splitTime) { - for (const [k, v] of Object.entries(anim.properties)) { - inheritedProps[k] = v; - } - continue; - } - - if (pos >= opts.splitTime) { - result = updateAnimationSelectorInScript(result, anim.id, newSelector); - continue; - } - - // Spans the split — linear interpolation to compute mid-values. - const progress = dur > 0 ? (opts.splitTime - pos) / dur : 0; - const fromSource = anim.fromProperties ?? inheritedProps; - const midProps: Record = {}; - for (const [k, v] of Object.entries(anim.properties)) { - if (typeof v !== "number") { - midProps[k] = v; - continue; - } - const fromVal = typeof fromSource[k] === "number" ? (fromSource[k] as number) : 0; - midProps[k] = fromVal + (v - fromVal) * progress; - } - - const firstHalfDuration = opts.splitTime - pos; - result = updateAnimationInScript(result, anim.id, { - duration: firstHalfDuration, - properties: midProps, - }); - - const secondHalfDuration = animEnd - opts.splitTime; - const addResult = addAnimationToScript(result, { - targetSelector: newSelector, - method: "fromTo", - position: newElementStart, - duration: secondHalfDuration, - properties: { ...anim.properties }, - fromProperties: { ...midProps }, - ease: anim.ease, - extras: anim.extras, - }); - result = addResult.script; - - for (const [k, v] of Object.entries(midProps)) { - inheritedProps[k] = v; - } + const anim = matching[i]; + if (!anim) continue; + result = applyTweenSplit(result, anim, baselineBefore[i] ?? {}, ctx, skippedSelectors); } - if (Object.keys(inheritedProps).length > 0) { - result = insertInheritedStateSetInScript(result, newSelector, newElementStart, inheritedProps); + if (Object.keys(finalInheritedProps).length > 0) { + result = insertInheritedStateSetInScript( + result, + newSelector, + newElementStart, + finalInheritedProps, + ); } return { script: result, skippedSelectors }; @@ -1704,7 +1897,7 @@ export function splitAnimationsInScript( // ── Unroll dynamic animations ──────────────────────────────────────────────── -function isLoopNode(node: any): boolean { +function isLoopNode(node: Node): boolean { const t = node?.type; return ( t === "ForStatement" || @@ -1714,7 +1907,7 @@ function isLoopNode(node: any): boolean { ); } -function isForEachStatement(node: any): boolean { +function isForEachStatement(node: Node): boolean { return ( node?.type === "ExpressionStatement" && node.expression?.type === "CallExpression" && @@ -1722,16 +1915,74 @@ function isForEachStatement(node: any): boolean { ); } -function findEnclosingLoop(ancestors: any[]): { start: number; end: number } | null { +/** The nearest enclosing loop / forEach AST node (not just its byte range). */ +function findEnclosingLoopNode(ancestors: Node[]): Node | null { for (let i = ancestors.length - 2; i >= 0; i--) { const node = ancestors[i]; - if (isLoopNode(node) || isForEachStatement(node)) { - return { start: node.start as number, end: node.end as number }; - } + if (isLoopNode(node) || isForEachStatement(node)) return node; + } + return null; +} + +/** Statements making up a loop's body block, or null when not a simple block. */ +function loopBodyStatements(loopNode: Node): Node[] | null { + let body: Node; + if (loopNode?.type === "ExpressionStatement") { + // forEach(cb): body is the callback's block. + const cb = loopNode.expression?.arguments?.[0]; + body = cb?.body; + } else { + body = loopNode?.body; + } + if (body?.type !== "BlockStatement") return null; + return (body.body ?? []).filter((s: Node) => s?.type === "ExpressionStatement"); +} + +/** The loop's index identifier name (`for (let i …)`), used for per-iteration substitution. */ +function loopIndexVarName(loopNode: Node): string | null { + if (loopNode?.type === "ForStatement") { + const decl = loopNode.init?.declarations?.[0]; + return typeof decl?.id?.name === "string" ? decl.id.name : null; } return null; } +/** + * Rewrite one body statement's source for iteration `idx`: replace USES of the + * loop index variable (AST Identifier nodes) with the literal index. AST-based, + * not a text regex, so the index name appearing inside a string literal (e.g. a + * selector ".row-i") or as a non-computed member/key (`obj.i`, `{ i: … }`) is + * left untouched — only real references to the variable are substituted. + */ +// An identifier in "binding position" is a name, not a value reference: a +// non-computed member property (`obj.i`) or object-literal key (`{ i: … }`). +// Those must NOT be substituted with the iteration index. +function isIndexBindingPosition(node: Node, parent: Node): boolean { + if (parent?.type === "MemberExpression") return parent.property === node && !parent.computed; + if (parent?.type === "Property" || parent?.type === "ObjectProperty") { + return parent.key === node && !parent.computed; + } + return false; +} + +function substituteLoopIndex(stmt: Node, indexVar: string, idx: number, script: string): string { + const base = stmt.start as number; + const src = script.slice(base, stmt.end as number); + const ranges: Array<[number, number]> = []; + acornWalk.ancestor(stmt, { + Identifier(node: Node, _state: unknown, ancestors: Node[]) { + if (node.name !== indexVar) return; + if (isIndexBindingPosition(node, ancestors[ancestors.length - 2])) return; + ranges.push([(node.start as number) - base, (node.end as number) - base]); + }, + }); + if (ranges.length === 0) return src; + ranges.sort((a, b) => b[0] - a[0]); + let out = src; + for (const [s, e] of ranges) out = out.slice(0, s) + String(idx) + out.slice(e); + return out; +} + function buildUnrollReplacement( timelineVar: string, animation: GsapAnimation, @@ -1759,29 +2010,189 @@ export type UnrollElement = { easeEach?: string; }; +/** Build one element's unrolled `tl.to(...)` call from the target animation. */ +function buildUnrollCallForElement( + timelineVar: string, + animation: GsapAnimation, + el: UnrollElement, +): string { + const duration = typeof animation.duration === "number" ? animation.duration : 8; + const ease = typeof animation.ease === "string" ? animation.ease : "none"; + const pos = animation.position ?? 0; + const posCode = typeof pos === "number" ? String(pos) : JSON.stringify(pos); + const sorted = [...el.keyframes].sort((a, b) => a.percentage - b.percentage); + const kfCode = buildKeyframeObjectCode(sorted, el.easeEach); + return `${timelineVar}.to(${JSON.stringify(el.selector)}, { keyframes: ${kfCode}, duration: ${duration}, ease: ${JSON.stringify(ease)} }, ${posCode});`; +} + +/** Sentinel: the unroll cannot safely reproduce the loop body — caller no-ops. */ +const REFUSE_UNROLL = Symbol("refuse-unroll"); + +/** Every statement in a loop's body block (unfiltered), or [] when not a block. */ +function loopBodyRawStatements(loopNode: Node): Node[] { + const body = + loopNode?.type === "ExpressionStatement" + ? loopNode.expression?.arguments?.[0]?.body + : loopNode?.body; + return body?.type === "BlockStatement" ? (body.body ?? []) : []; +} + +/** A node that re-binds `indexVar`: a re-declaration or a function param. */ +function rebindsIndex(node: Node, indexVar: string): boolean { + if (node.type === "VariableDeclarator") return node.id?.name === indexVar; + if ( + node.type === "FunctionExpression" || + node.type === "FunctionDeclaration" || + node.type === "ArrowFunctionExpression" + ) { + return (node.params ?? []).some((p: Node) => p?.name === indexVar); + } + return false; +} + +/** Object shorthand `{ i }` — substituting the value would yield invalid `{ 0 }`. */ +function isShorthandIndexUse(node: Node, indexVar: string): boolean { + return ( + (node.type === "Property" || node.type === "ObjectProperty") && + node.shorthand === true && + propKeyName(node) === indexVar + ); +} + +/** + * A sibling statement can't be safely index-substituted when it re-binds the + * loop index (shadowing — a nested `for (let i …)`, a callback param `i`) or + * uses it in object shorthand (`{ i }`, which would splice to the invalid + * `{ 0 }`). substituteLoopIndex has no scope analysis, so in these cases it + * would emit broken or wrong code — the unroll must refuse instead. + */ +function hasUnsafeLoopIndexUse(stmt: Node, indexVar: string): boolean { + let unsafe = false; + acornWalk.full(stmt, (node: Node) => { + if (!unsafe && (isShorthandIndexUse(node, indexVar) || rebindsIndex(node, indexVar))) { + unsafe = true; + } + }); + return unsafe; +} + +/** How to handle the loop body's non-target siblings when unrolling. */ +function unrollSiblingStrategy( + loopNode: Node, + targetStmt: Node, + stmts: Node[], + indexVar: string | null, +): "blanket" | "refuse" | "preserve" { + const siblings = stmts.filter((s) => s !== targetStmt); + // A sibling the filtered statement list doesn't model (non-ExpressionStatement) + // would be silently lost by either path — refuse if any exists. + const hasUnmodeledSibling = loopBodyRawStatements(loopNode).some( + (s) => s !== targetStmt && !stmts.includes(s), + ); + if (siblings.length === 0 && !hasUnmodeledSibling) return "blanket"; + if (hasUnmodeledSibling || !indexVar) return "refuse"; + return siblings.some((s) => hasUnsafeLoopIndexUse(s, indexVar)) ? "refuse" : "preserve"; +} + +/** Emit the per-iteration unrolled lines (target → static tl.to, siblings → index-substituted). */ +function emitUnrolledLines( + stmts: Node[], + targetStmt: Node, + elements: UnrollElement[], + timelineVar: string, + animation: GsapAnimation, + indexVar: string, + script: string, +): string { + const lines: string[] = []; + for (let idx = 0; idx < elements.length; idx++) { + const el = elements[idx]; + if (!el) continue; + for (const stmt of stmts) { + lines.push( + stmt === targetStmt + ? buildUnrollCallForElement(timelineVar, animation, el) + : substituteLoopIndex(stmt, indexVar, idx, script), + ); + } + } + return lines.join("\n "); +} + +/** + * Unroll the loop body, preserving every statement that is NOT the target tween. + * For each iteration, emit each non-target statement with the loop index + * substituted (e.g. `tl.set(items[i], …)` → `tl.set(items[0], …)`), and replace + * the target tween statement with that element's static `tl.to()` call. + * + * Returns null when a blanket overwrite is lossless (no sibling statements), and + * REFUSE_UNROLL when siblings exist but can't be safely reproduced — a non-`for` + * loop (no numeric index to splice), a statement we don't model, or an unsafe + * index use (shadowing / shorthand). Refusing no-ops the unroll, which is safe: + * the dynamic loop keeps rendering correctly, just un-flattened. + */ +function buildLoopUnrollPreserving( + script: string, + timelineVar: string, + animation: GsapAnimation, + elements: UnrollElement[], + loopNode: Node, + targetStmt: Node, +): string | null | typeof REFUSE_UNROLL { + const stmts = loopBodyStatements(loopNode); + if (!stmts || !stmts.includes(targetStmt)) return null; + const indexVar = loopIndexVarName(loopNode); + const strategy = unrollSiblingStrategy(loopNode, targetStmt, stmts, indexVar); + if (strategy === "blanket") return null; + if (strategy === "refuse" || !indexVar) return REFUSE_UNROLL; + return emitUnrolledLines(stmts, targetStmt, elements, timelineVar, animation, indexVar, script); +} + /** * Replace a dynamic loop that generates multiple tween calls with individual * static `tl.to()` calls — one per element. Finds the loop containing the - * animation and replaces the entire loop body with unrolled static calls. + * animation and replaces the loop with unrolled static calls, preserving every + * non-target statement in the loop body per iteration. */ export function unrollDynamicAnimations( script: string, animationId: string, elements: UnrollElement[], ): string { + // An empty element list has no unrolled form — replacing the loop/statement + // with zero calls would silently delete the animation. No-op instead. + if (elements.length === 0) return script; const parsed = parseGsapScriptAcornForWrite(script); if (!parsed) return script; const target = parsed.located.find((l) => l.id === animationId); if (!target) return script; - const replacement = buildUnrollReplacement(parsed.timelineVar, target.animation, elements); const ms = new MagicString(script); - const loop = findEnclosingLoop(target.call.ancestors); - if (loop) { - ms.overwrite(loop.start, loop.end, replacement); + const loopNode = findEnclosingLoopNode(target.call.ancestors); + if (loopNode) { + const targetStmt = findEnclosingExpressionStatement(target.call.ancestors); + const preserving = targetStmt + ? buildLoopUnrollPreserving( + script, + parsed.timelineVar, + target.animation, + elements, + loopNode, + targetStmt, + ) + : null; + // Siblings exist but can't be safely reproduced — leave the loop untouched + // rather than drop or corrupt them. The op no-ops (before === after). + if (preserving === REFUSE_UNROLL) return script; + // Fall back to the simple whole-body replacement when the body isn't a plain + // block of statements we can preserve. + const replacement = + preserving ?? buildUnrollReplacement(parsed.timelineVar, target.animation, elements); + ms.overwrite(loopNode.start as number, loopNode.end as number, replacement); } else { const stmt = findEnclosingExpressionStatement(target.call.ancestors); if (!stmt) return script; + const replacement = buildUnrollReplacement(parsed.timelineVar, target.animation, elements); ms.overwrite(stmt.start as number, stmt.end as number, replacement); } return ms.toString(); diff --git a/packages/core/src/parsers/gsapWriterParity.corpus.test.ts b/packages/core/src/parsers/gsapWriterParity.corpus.test.ts index 8270056ba..cb272c9f1 100644 --- a/packages/core/src/parsers/gsapWriterParity.corpus.test.ts +++ b/packages/core/src/parsers/gsapWriterParity.corpus.test.ts @@ -616,17 +616,19 @@ describe("correctness — addLabelToScript / removeLabelFromScript", () => { expect(removeLabelFromScript(SYN_SINGLE, "nope")).toBe(SYN_SINGLE); }); - it("adding the same label twice yields two addLabel calls (no dedup contract)", () => { + it("adding the same label twice MOVES it instead of duplicating (dedup contract)", () => { + // A second addLabel for an existing name must not append a duplicate — + // duplicates make removeLabel over-remove. It moves the label's position. const once = addLabelToScript(SYN_SINGLE, "mid", 1.0); const twice = addLabelToScript(once, "mid", 2.0); - expect(labelCallCount(twice, "mid")).toBe(2); + expect(labelCallCount(twice, "mid")).toBe(1); + expect(twice).toContain('tl.addLabel("mid", 2)'); }); - it("removeLabel deletes ALL matching addLabel calls for the name", () => { - const once = addLabelToScript(SYN_SINGLE, "mid", 1.0); - const twice = addLabelToScript(once, "mid", 2.0); - const cleared = removeLabelFromScript(twice, "mid"); - expect(labelCallCount(cleared, "mid")).toBe(0); + it("removeLabel deletes ALL matching addLabel calls for the name (hand-authored dups)", () => { + const dup = `var tl = gsap.timeline({ paused: true });\ntl.addLabel("mid", 1);\ntl.addLabel("mid", 2);\nwindow.__timelines["t"] = tl;`; + expect(labelCallCount(dup, "mid")).toBe(2); + expect(labelCallCount(removeLabelFromScript(dup, "mid"), "mid")).toBe(0); }); it("the added label is observable by the parser when a tween references it", () => { diff --git a/packages/core/src/parsers/htmlParser.test.ts b/packages/core/src/parsers/htmlParser.test.ts index 2d786a7d3..cd7543109 100644 --- a/packages/core/src/parsers/htmlParser.test.ts +++ b/packages/core/src/parsers/htmlParser.test.ts @@ -536,6 +536,29 @@ describe("removeElementFromHtml", () => { expect(updated).not.toContain('id="el1"'); expect(updated).toContain('id="el2"'); }); + + it("strips ALL gsap tweens for the removed element, not just the first", () => { + // Two tweens on the same element → count-based ids renumber when the first is + // removed, so a single up-front parse left the second tween orphaned. + const html = ` + +
+
box
+
+ +`; + + const updated = removeElementFromHtml(html, "box"); + + expect(updated).not.toContain('data-hf-id="box"'); + // Neither tween may survive — the orphaned second tl.to referenced a deleted element. + expect(updated).not.toContain("x: 100"); + expect(updated).not.toContain("x: 200"); + }); }); describe("validateCompositionHtml", () => { diff --git a/packages/core/src/parsers/htmlParser.ts b/packages/core/src/parsers/htmlParser.ts index df02653af..5a9082544 100644 --- a/packages/core/src/parsers/htmlParser.ts +++ b/packages/core/src/parsers/htmlParser.ts @@ -683,15 +683,24 @@ function selectorTargetsId(selector: string, id: string): boolean { } function stripGsapForId(script: string, elementId: string): string { - const parsed = parseGsapScriptAcornForWrite(script); - if (!parsed) return script; + // Re-parse after every removal. Animation ids are count-based (positional), so + // removing one tween renumbers the survivors — ids captured from a single + // up-front parse go stale and silently no-op, orphaning later tweens on the + // now-deleted element. Always remove the FIRST still-matching animation in a + // freshly-parsed script until none remain. let current = script; - for (const { id: animId, animation } of parsed.located) { - if (selectorTargetsId(animation.targetSelector, elementId)) { - current = removeAnimationFromScript(current, animId); - } + for (;;) { + const parsed = parseGsapScriptAcornForWrite(current); + if (!parsed) return current; + const match = parsed.located.find((l) => + selectorTargetsId(l.animation.targetSelector, elementId), + ); + if (!match) return current; + const updated = removeAnimationFromScript(current, match.id); + // Guard against a non-removing match (would otherwise loop forever). + if (updated === current) return current; + current = updated; } - return current; } function cascadeRemoveGsapById(doc: Document, elementId: string): void { diff --git a/packages/sdk/src/adapters/iframe.test.ts b/packages/sdk/src/adapters/iframe.test.ts index 879a96186..c01861900 100644 --- a/packages/sdk/src/adapters/iframe.test.ts +++ b/packages/sdk/src/adapters/iframe.test.ts @@ -212,6 +212,7 @@ interface FakeDomEl { "data-x": string | null; "data-y": string | null; style: FakeStyle; + isConnected: boolean; getAttribute(name: string): string | null; querySelector(sel: string): FakeDomEl | null; } @@ -234,6 +235,7 @@ function fakeDomEl(id: string, dataX: string | null, dataY: string | null): Fake "data-x": dataX, "data-y": dataY, style, + isConnected: true, getAttribute(name) { if (name === "data-x") return this["data-x"]; if (name === "data-y") return this["data-y"]; @@ -304,6 +306,25 @@ describe("IframePreviewAdapter draft / commit / cancel", () => { }); }); + it("applyDraft reuses the cached element across repeated calls (no re-query)", () => { + const el = fakeDomEl("hf-abc", "0", "0"); + let queryCount = 0; + const iframe = { + contentDocument: { + querySelector(_sel: string) { + queryCount++; + return el; + }, + }, + } as unknown as HTMLIFrameElement; + const adapter = createIframePreviewAdapter(iframe); + adapter.applyDraft("hf-abc", { dx: 1, dy: 1 }); + adapter.applyDraft("hf-abc", { dx: 2, dy: 2 }); + adapter.applyDraft("hf-abc", { dx: 3, dy: 3 }); + // Queried once on the first call; the next two reuse the connected cache. + expect(queryCount).toBe(1); + }); + it("commitPreview without a dispatch callback is a no-op", () => { const el = fakeDomEl("hf-abc", "0", "0"); const adapter = createIframePreviewAdapter(fakeIframe(el)); diff --git a/packages/sdk/src/adapters/iframe.ts b/packages/sdk/src/adapters/iframe.ts index 139f689cb..a0af067f9 100644 --- a/packages/sdk/src/adapters/iframe.ts +++ b/packages/sdk/src/adapters/iframe.ts @@ -102,7 +102,7 @@ class IframePreviewAdapter implements PreviewAdapter { /** Tracked id and element for the in-progress drag. */ private _draftId: string | null = null; - private _draftEl: Element | null = null; + private _draftEl: HTMLElement | null = null; constructor(iframe: HTMLIFrameElement, dispatch?: (op: EditOp) => void) { this.iframe = iframe; @@ -141,9 +141,14 @@ class IframePreviewAdapter implements PreviewAdapter { const doc = this.iframe.contentDocument; if (!doc) return; - const el = doc.querySelector( - `[data-hf-id="${id.replace(/\\/g, "\\\\").replace(/"/g, '\\"')}"]`, - ); + // Reuse the tracked element across the 60fps drag; only re-query when the id + // changes or the cached node detached (e.g. an iframe reload mid-drag). + const cached = id === this._draftId && this._draftEl?.isConnected ? this._draftEl : null; + const el = + cached ?? + doc.querySelector( + `[data-hf-id="${id.replace(/\\/g, "\\\\").replace(/"/g, '\\"')}"]`, + ); if (!el) return; this._draftId = id; @@ -167,11 +172,11 @@ class IframePreviewAdapter implements PreviewAdapter { return; } - const el = this._draftEl as HTMLElement; + const el = this._draftEl; const dx = parseFloat(el.style.getPropertyValue(VAR_DX) || "0") || 0; const dy = parseFloat(el.style.getPropertyValue(VAR_DY) || "0") || 0; - const dataX = (this._draftEl as Element).getAttribute("data-x"); - const dataY = (this._draftEl as Element).getAttribute("data-y"); + const dataX = el.getAttribute("data-x"); + const dataY = el.getAttribute("data-y"); const { x, y } = computeDraftPosition(dataX, dataY, dx, dy); this._dispatch({ type: "moveElement", target: this._draftId, x, y }); @@ -185,9 +190,8 @@ class IframePreviewAdapter implements PreviewAdapter { private _clearDraft(): void { if (this._draftEl) { - const el = this._draftEl as HTMLElement; - el.style.removeProperty(VAR_DX); - el.style.removeProperty(VAR_DY); + this._draftEl.style.removeProperty(VAR_DX); + this._draftEl.style.removeProperty(VAR_DY); } this._draftId = null; this._draftEl = null; diff --git a/packages/sdk/src/engine/mutate.gsap.test.ts b/packages/sdk/src/engine/mutate.gsap.test.ts index 8fc18d73a..5d60d6b59 100644 --- a/packages/sdk/src/engine/mutate.gsap.test.ts +++ b/packages/sdk/src/engine/mutate.gsap.test.ts @@ -90,8 +90,16 @@ describe("validateOp with GSAP script", () => { ).toBe(true); }); - it("removeGsapTween → ok:true", () => { - expect(validateOp(fresh(), { type: "removeGsapTween", animationId: "some-id" }).ok).toBe(true); + it("removeGsapTween → ok:true for a resolvable id", () => { + expect(validateOp(fresh(), { type: "removeGsapTween", animationId: TWEEN_ANIM_ID }).ok).toBe( + true, + ); + }); + + it("removeGsapTween → E_TARGET_NOT_FOUND for an unresolved id (can/apply agreement)", () => { + const r = validateOp(fresh(), { type: "removeGsapTween", animationId: "no-such-id" }); + expect(r.ok).toBe(false); + if (!r.ok) expect(r.code).toBe("E_TARGET_NOT_FOUND"); }); it("addLabel → ok:true", () => { @@ -400,13 +408,13 @@ describe("setGsapKeyframe", () => { }); describe("removeGsapKeyframe", () => { - it("removes keyframe at index 1 (50%)", () => { + it("removes keyframe at 50%", () => { const parsed = fresh(KF_SCRIPT); const animId = `[data-hf-id="hf-box"]-to-0-visual`; const result = applyOp(parsed, { type: "removeGsapKeyframe", animationId: animId, - keyframeIndex: 1, + percentage: 50, }); expect(result.forward).toHaveLength(1); const newScript = String(result.forward[0]?.value ?? ""); @@ -442,6 +450,26 @@ describe("removeAllKeyframes", () => { }); }); +// ─── materializeKeyframes ────────────────────────────────────────────────────── + +describe("materializeKeyframes", () => { + // dispatch bypasses validateOp, so the writer guard is the protection: an empty + // keyframe list must no-op rather than rebuild vars with an empty keyframes + // object (which would empty the animation). Uses the real anim id so the no-op + // is attributable to the empty list, not an unresolved id. + it("empty keyframe list no-ops on the dispatch path (writer guard)", () => { + const parsed = fresh(); + const before = getScript(parsed); + const result = applyOp(parsed, { + type: "materializeKeyframes", + animationId: TWEEN_ANIM_ID, + keyframes: [], + }); + expect(result.forward).toHaveLength(0); + expect(getScript(parsed)).toBe(before); + }); +}); + // ─── convertToKeyframes ──────────────────────────────────────────────────────── describe("convertToKeyframes", () => { @@ -630,6 +658,51 @@ window.__timelines["t"] = tl;`; }); }); +// ─── setTiming — per-tween GSAP shift/scale (review #3) ─────────────────────── + +describe("setTiming — GSAP sync shifts/scales each tween (not absolute)", () => { + // Two staggered tweens on ONE element: positions 2.0 and 5.0, clip [2, 7]. + const STAGGER_SCRIPT = `var tl = gsap.timeline({ paused: true }); +tl.to("[data-hf-id=\\"hf-box\\"]", { x: 100, duration: 1 }, 2); +tl.to("[data-hf-id=\\"hf-box\\"]", { x: 200, duration: 1 }, 5); +window.__timelines["t"] = tl;`; + + function freshStagger() { + return parseMutable(`
+
+ +
`); + } + + function gsapPatch(result: ReturnType): string { + const v = result.forward + .map((p) => p.value) + .find((val) => typeof val === "string" && val.includes("tl.")); + return typeof v === "string" ? v : ""; + } + + it("moving the clip +1 shifts BOTH tweens by the delta, preserving the stagger", () => { + const result = applyOp(freshStagger(), { type: "setTiming", target: "hf-box", start: 3 }); + const script = gsapPatch(result); + // 2.0 → 3.0 and 5.0 → 6.0 — NOT both collapsed onto the new absolute start. + expect(script).toContain("{ x: 100, duration: 1 }, 3)"); + expect(script).toContain("{ x: 200, duration: 1 }, 6)"); + // The stagger gap (3s) is preserved; durations are untouched. + expect(script).not.toContain("duration: 5"); + }); + + it("resizing the clip x2 scales each tween's duration by the ratio (not full clip)", () => { + // duration 5 → 10 (ratio 2); positions remap about the clip start (2). + const result = applyOp(freshStagger(), { type: "setTiming", target: "hf-box", duration: 10 }); + const script = gsapPatch(result); + // pos 2 (offset 0) stays 2; pos 5 → 2 + (5-2)*2 = 8. durations 1 → 2. + expect(script).toContain("{ x: 100, duration: 2 }, 2)"); + expect(script).toContain("{ x: 200, duration: 2 }, 8)"); + // The bug blew every duration up to the full clip duration (10). + expect(script).not.toContain("duration: 10"); + }); +}); + // ─── Label ops ──────────────────────────────────────────────────────────────── describe("addLabel", () => { @@ -737,6 +810,21 @@ window.__timelines["t"] = tl;`; expect(newScript).not.toContain("hf-box"); expect(newScript).toContain("hf-stage"); }); + + it("strips ALL tweens for the element, not just the first (positional-id renumber)", () => { + // Two tweens on the same element: removing the first renumbers the survivor's + // count-based id, so a single up-front parse left the second tween orphaned. + const twoOwnTweens = `var tl = gsap.timeline({ paused: true }); +tl.to("[data-hf-id=\\"hf-box\\"]", { x: 100, duration: 1 }, 0); +tl.to("[data-hf-id=\\"hf-box\\"]", { x: 200, duration: 1 }, 1); +window.__timelines["t"] = tl;`; + const parsed = fresh(twoOwnTweens); + const result = applyOp(parsed, { type: "removeElement", target: "hf-box" }); + const newScript = String(result.forward[1]?.value ?? ""); + expect(newScript).not.toContain("hf-box"); + expect(newScript).not.toContain("x: 100"); + expect(newScript).not.toContain("x: 200"); + }); }); // ─── GSAP ops on composition with no script block ──────────────────────────── @@ -857,3 +945,86 @@ describe("removeArcPath", () => { expect(getScript(parsed)).not.toContain("motionPath"); }); }); + +// ─── R3 #6 — validateOp rejects unappliable arc-segment edits ───────────────── + +describe("validateOp updateArcSegment (R3 #6)", () => { + it("E_ARC_NOT_ENABLED when the tween has no enabled arc path", () => { + const r = validateOp(freshArc(), { + type: "updateArcSegment", + animationId: ARC_ANIM_ID, + segmentIndex: 0, + update: { curviness: 2 }, + }); + expect(r.ok).toBe(false); + if (!r.ok) expect(r.code).toBe("E_ARC_NOT_ENABLED"); + }); + + it("E_INVALID_ARGS when the segment index is out of range", () => { + const parsed = freshArc(); + enableArc(parsed); + const r = validateOp(parsed, { + type: "updateArcSegment", + animationId: ARC_ANIM_ID, + segmentIndex: 9, + update: { curviness: 2 }, + }); + expect(r.ok).toBe(false); + if (!r.ok) expect(r.code).toBe("E_INVALID_ARGS"); + }); +}); + +// ─── R3 #13b — deleteAllForSelector matches across quote styles ──────────────── + +describe("deleteAllForSelector quote-insensitive match (R3 #13b)", () => { + it("removes a tween authored with double quotes when given a single-quoted selector", () => { + const html = `
+
+ +
`; + const parsed = parseMutable(html); + const result = applyOp(parsed, { + type: "deleteAllForSelector", + selector: `[data-hf-id='hf-box']`, + }); + expect(result.forward.length).toBeGreaterThan(0); + expect(getScript(parsed)).not.toContain("tl.to("); + }); +}); + +// ─── CF2 #15/#16 — handleSetTiming syncs #domId tweens + resizes data-duration ─ + +describe("handleSetTiming GSAP sync (CF2 #15/#16)", () => { + function timingDoc(attrs: string, tween: string) { + return parseMutable( + `
+
+ +
`, + ); + } + + it("#15: a #domId-targeted tween shifts when the clip moves", () => { + const parsed = timingDoc( + `data-start="2" data-end="5"`, + `tl.to("#box", { x: 100, duration: 1 }, 2);`, + ); + applyOp(parsed, { type: "setTiming", target: "hf-box", start: 5 }); + // position remapped 2 → 5 (delta +3); the bug left it at 2. + expect(getScript(parsed)).toMatch(/tl\.to\("#box",[^)]*\}, 5\)/); + }); + + it("#16: a data-duration clip updates data-duration and scales its tween", () => { + const parsed = timingDoc( + `data-start="2" data-duration="4"`, + `tl.to("#box", { x: 100, duration: 4 }, 2);`, + ); + applyOp(parsed, { type: "setTiming", target: "hf-box", duration: 8 }); + const el = parsed.document.querySelector('[data-hf-id="hf-box"]'); + // data-duration updated (not a stale value beside a fresh data-end). + expect(el?.getAttribute("data-duration")).toBe("8"); + expect(el?.getAttribute("data-end")).toBeNull(); + // tween duration scaled 4 → 8 (ratio 2). + expect(getScript(parsed)).toContain("duration: 8"); + }); +}); diff --git a/packages/sdk/src/engine/mutate.test.ts b/packages/sdk/src/engine/mutate.test.ts index 2ef850f4d..03b8f209d 100644 --- a/packages/sdk/src/engine/mutate.test.ts +++ b/packages/sdk/src/engine/mutate.test.ts @@ -492,6 +492,34 @@ describe("Phase 3b ops", () => { if (!r2.ok) expect(r2.code).toBe("E_NO_GSAP_SCRIPT"); }); + it("unrollDynamicAnimations rejects an empty element list (would delete the animation)", () => { + const parsed = parseMutable( + `
` + + ``, + ); + const r = validateOp(parsed, { + type: "unrollDynamicAnimations", + animationId: "#x-to-0-position", + elements: [], + }); + expect(r.ok).toBe(false); + if (!r.ok) expect(r.code).toBe("E_INVALID_ARGS"); + }); + + it("materializeKeyframes rejects an empty keyframe list (would empty the animation)", () => { + const parsed = parseMutable( + `
` + + ``, + ); + const r = validateOp(parsed, { + type: "materializeKeyframes", + animationId: "#x-to-0-position", + keyframes: [], + }); + expect(r.ok).toBe(false); + if (!r.ok) expect(r.code).toBe("E_INVALID_ARGS"); + }); + it("setClassStyle no longer throws — implemented in Phase 3b", () => { expect(() => applyOp(fresh(), { diff --git a/packages/sdk/src/engine/mutate.ts b/packages/sdk/src/engine/mutate.ts index 5968d2810..6b5051d7a 100644 --- a/packages/sdk/src/engine/mutate.ts +++ b/packages/sdk/src/engine/mutate.ts @@ -150,9 +150,7 @@ function dispatchRemoveGsapKeyframe( parsed: ParsedDocument, op: Extract, ): MutationResult { - return "percentage" in op - ? handleRemoveGsapKeyframeByPercentage(parsed, op.animationId, op.percentage) - : handleRemoveGsapKeyframe(parsed, op.animationId, op.keyframeIndex); + return handleRemoveGsapKeyframeByPercentage(parsed, op.animationId, op.percentage); } function applyGsapKeyframeOp(parsed: ParsedDocument, op: EditOp): MutationResult | undefined { @@ -397,11 +395,22 @@ function handleSetTiming( const oldStartStr = el.getAttribute("data-start"); const oldEndStr = el.getAttribute("data-end"); + const oldDurationStr = el.getAttribute("data-duration"); const oldTrackStr = el.getAttribute("data-track-index"); const oldStart = oldStartStr !== null ? parseFloat(oldStartStr) : null; const oldEnd = oldEndStr !== null ? parseFloat(oldEndStr) : null; - const oldDuration = oldStart !== null && oldEnd !== null ? oldEnd - oldStart : null; + const oldDurationAttr = oldDurationStr !== null ? parseFloat(oldDurationStr) : null; + // Prefer an explicit data-duration — the attribute clips are authored with and + // the runtime reads — falling back to data-end − data-start. Reading only + // data-end left oldDuration null for duration-authored clips, collapsing the + // GSAP duration-scale ratio to 1 and scaling nothing. + const oldDuration = + oldDurationAttr !== null + ? oldDurationAttr + : oldStart !== null && oldEnd !== null + ? oldEnd - oldStart + : null; const oldTrack = oldTrackStr !== null ? parseInt(oldTrackStr, 10) : null; const newStart = timing.start ?? oldStart; @@ -415,7 +424,20 @@ function handleSetTiming( el.setAttribute("data-start", String(newStart)); } - if ( + // Write to whichever timing attribute the clip actually uses. A data-duration + // clip updates data-duration only on a real resize (duration is invariant + // under a move); a data-end clip updates data-end whenever start or duration + // changes (end = start + duration). Writing a fresh data-end beside a stale + // data-duration had no playback effect. + if (oldDurationStr !== null) { + if (timing.duration !== undefined && newDuration !== null) { + const path = timingPath(id, "duration"); + const p = scalarChange(path, oldDurationAttr, newDuration); + result.forward.push(p.forward); + result.inverse.push(p.inverse); + el.setAttribute("data-duration", String(newDuration)); + } + } else if ( (timing.duration !== undefined || timing.start !== undefined) && newStart !== null && newDuration !== null @@ -442,16 +464,39 @@ function handleSetTiming( // Sync GSAP tween positions: the GSAP script is the source of truth at play time — // the timeline rebuilds from it on every seek. Without this, DOM attribute edits // have zero playback effect; the script's position/duration silently overrides them. - // Match against the resolved element's own data-hf-id (the canonical form - // tweens are stored under) so a comp-root target ("sub-1") whose tween lives - // at [data-hf-id="hf-host"] still syncs. - const matchId = el.getAttribute("data-hf-id") ?? id; - if (parsedGsap && currentScript) { + // Match against BOTH the element's data-hf-id (the canonical form) AND its DOM + // id: the Studio GSAP panel / ensureElementAddressable author tweens as + // `#domId`, which selectorMatchesId(hfId) never matched — so moving/resizing + // those clips left their tweens unsynced. + const matchHfId = el.getAttribute("data-hf-id") ?? id; + const matchDomId = el.getAttribute("id"); + if (parsedGsap && currentScript && oldStart !== null) { + // 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 + // the absolute newStart/newDuration onto every tween would collapse the + // stagger onto one point and blow each tween's duration to the full clip. + const startChanged = timing.start !== undefined && newStart !== null; + const durChanged = timing.duration !== undefined && newDuration !== null; + const ratio = + durChanged && oldDuration !== null && oldDuration > 0 && newDuration !== null + ? newDuration / oldDuration + : 1; + const remapStart = startChanged && newStart !== null ? newStart : oldStart; for (const { id: animId, animation } of parsedGsap.located) { - if (!selectorMatchesId(animation.targetSelector, matchId)) continue; + const matches = + selectorMatchesId(animation.targetSelector, matchHfId) || + (matchDomId !== null && selectorMatchesId(animation.targetSelector, matchDomId)); + if (!matches) continue; + if (typeof animation.position !== "number") continue; const updates: Partial = {}; - if (timing.start !== undefined && newStart !== null) updates.position = newStart; - if (timing.duration !== undefined && newDuration !== null) updates.duration = newDuration; + if (startChanged || durChanged) { + const shifted = remapStart + (animation.position - oldStart) * ratio; + updates.position = Math.max(0, Math.round(shifted * 1000) / 1000); + } + if (durChanged && typeof animation.duration === "number" && animation.duration > 0) { + updates.duration = Math.max(0.001, Math.round(animation.duration * ratio * 1000) / 1000); + } if (Object.keys(updates).length === 0) continue; currentScript = updateAnimationInScript(currentScript, animId, updates); } @@ -639,15 +684,20 @@ function collectSubtreeHfIds(el: Element): string[] { } function cascadeRemoveAnimations(script: string, id: HfId): string { - const parsedGsap = parseGsapScriptAcornForWrite(script); - if (!parsedGsap) return script; + // Re-parse after each removal: animation ids are positional, so removing one + // tween renumbers the survivors — ids from a single up-front parse go stale and + // no-op, orphaning later tweens on the removed element. Same fix as + // stripGsapForId in htmlParser.ts (R3 #3); this is its SDK-side twin. let current = script; - for (const { id: animId, animation } of parsedGsap.located) { - if (selectorMatchesId(animation.targetSelector, id)) { - current = removeAnimationFromScript(current, animId); - } + for (;;) { + const parsedGsap = parseGsapScriptAcornForWrite(current); + if (!parsedGsap) return current; + const match = parsedGsap.located.find((l) => selectorMatchesId(l.animation.targetSelector, id)); + if (!match) return current; + const next = removeAnimationFromScript(current, match.id); + if (next === current) return current; // guard against a non-removing match + current = next; } - return current; } // ─── setClassStyle handler ──────────────────────────────────────────────────── @@ -894,7 +944,13 @@ function handleDeleteAllForSelector(parsed: ParsedDocument, selector: string): M if (!script) return EMPTY; const parsedForWrite = parseGsapScriptAcornForWrite(script); if (!parsedForWrite) return EMPTY; - const matching = parsedForWrite.located.filter((l) => l.animation.targetSelector === selector); + // Compare quote-insensitively: [data-hf-id='x'] and [data-hf-id="x"] are the + // same selector. A strict === missed the alternate quote style and matched + // nothing while can() reported ok. + const wanted = selector.replace(/'/g, '"'); + const matching = parsedForWrite.located.filter( + (l) => l.animation.targetSelector.replace(/'/g, '"') === wanted, + ); if (matching.length === 0) return EMPTY; let newScript = script; for (const m of [...matching].reverse()) { @@ -913,8 +969,9 @@ function resolveKeyframe(parsed: ParsedDocument, animationId: string, keyframeIn const parsedForWrite = parseGsapScriptAcornForWrite(script); const located = parsedForWrite?.located.find((l) => l.id === animationId); const kfs = located?.animation.keyframes?.keyframes; - if (!kfs || keyframeIndex < 0 || keyframeIndex >= kfs.length) return null; - return { script, kf: kfs[keyframeIndex]!, kfs }; + const kf = kfs?.[keyframeIndex]; + if (!kfs || !kf || keyframeIndex < 0) return null; + return { script, kf, kfs }; } // fallow-ignore-next-line complexity @@ -995,26 +1052,9 @@ function handleRemoveGsapKeyframeByPercentage( // No-op on ambiguity: duplicate-percentage keyframes can't be disambiguated. const TOLERANCE = 0.001; const matches = kfs.filter((k) => Math.abs(k.percentage - percentage) <= TOLERANCE); - if (matches.length !== 1) return EMPTY; - const pct = matches[0]!.percentage; - const newScript = removeKeyframeFromScript(script, animationId, pct); - if (newScript === script) return EMPTY; - setGsapScript(parsed.document, newScript); - return gsapScriptChange(script, newScript); -} - -function handleRemoveGsapKeyframe( - parsed: ParsedDocument, - animationId: string, - keyframeIndex: number, -): MutationResult { - const resolved = resolveKeyframe(parsed, animationId, keyframeIndex); - if (!resolved) return EMPTY; - const { script, kf, kfs } = resolved; - const pct = kf.percentage; - // removeKeyframeFromScript matches by percentage; bail if two keyframes share - // the same percentage to avoid removing the wrong one. - if (kfs.filter((k) => k.percentage === pct).length > 1) return EMPTY; + const sole = matches[0]; + if (matches.length !== 1 || !sole) return EMPTY; + const pct = sole.percentage; const newScript = removeKeyframeFromScript(script, animationId, pct); if (newScript === script) return EMPTY; setGsapScript(parsed.document, newScript); @@ -1047,6 +1087,60 @@ function canErr(code: string, message: string, hint?: string): CanResult { return hint ? { ok: false, code, message, hint } : { ok: false, code, message }; } +/** E_NO_GSAP_SCRIPT CanResult when the composition has no GSAP script, else null. */ +function gsapScriptMissing(parsed: ParsedDocument): CanResult | null { + return getGsapScript(parsed.document) === null + ? canErr( + "E_NO_GSAP_SCRIPT", + "No GSAP script block found in the composition.", + "This composition does not use GSAP animations.", + ) + : null; +} + +/** The located GSAP animation for `animationId`, or undefined. */ +function locateGsapAnimation(parsed: ParsedDocument, animationId: string) { + const script = getGsapScript(parsed.document); + if (!script) return undefined; + return parseGsapScriptAcornForWrite(script)?.located.find((l) => l.id === animationId); +} + +/** + * E_TARGET_NOT_FOUND CanResult when no GSAP animation resolves to `animationId`, + * else null. Without this, can() returned ok for stale/positional ids that then + * no-op'd at apply — the caller believed the edit would land. + */ +function gsapAnimationMissing(parsed: ParsedDocument, animationId: string): CanResult | null { + if (getGsapScript(parsed.document) === null) return null; // reported by gsapScriptMissing + return locateGsapAnimation(parsed, animationId) + ? null + : canErr( + "E_TARGET_NOT_FOUND", + `No GSAP animation found with id "${animationId}".`, + "Animation ids are positional and shift after edits — re-read them from comp before dispatching.", + ); +} + +/** Validate updateArcSegment: the tween must have an enabled arc with that segment. */ +function validateArcSegment( + parsed: ParsedDocument, + op: Extract, +): CanResult { + const arc = locateGsapAnimation(parsed, op.animationId)?.animation.arcPath; + if (!arc?.enabled) + return canErr( + "E_ARC_NOT_ENABLED", + `Animation "${op.animationId}" has no enabled arc path.`, + "Call setArcPath({ enabled: true }) before updating a segment.", + ); + if (op.segmentIndex < 0 || op.segmentIndex >= arc.segments.length) + return canErr( + "E_INVALID_ARGS", + `Segment index ${op.segmentIndex} is out of range (0..${arc.segments.length - 1}).`, + ); + return CAN_OK; +} + /** Dry-run validation — returns CanResult for the given op against current document state. */ // fallow-ignore-next-line complexity export function validateOp(parsed: ParsedDocument, op: EditOp): CanResult { @@ -1108,22 +1202,44 @@ export function validateOp(parsed: ParsedDocument, op: EditOp): CanResult { case "removeGsapTween": case "removeAllKeyframes": case "convertToKeyframes": - case "materializeKeyframes": case "splitIntoPropertyGroups": - case "splitAnimations": case "setArcPath": - case "updateArcSegment": case "removeArcPath": - case "unrollDynamicAnimations": + return gsapScriptMissing(parsed) ?? gsapAnimationMissing(parsed, op.animationId) ?? CAN_OK; + case "updateArcSegment": + return ( + gsapScriptMissing(parsed) ?? + gsapAnimationMissing(parsed, op.animationId) ?? + validateArcSegment(parsed, op) + ); + case "splitAnimations": case "deleteAllForSelector": case "removeLabel": - if (getGsapScript(parsed.document) === null) - return canErr( - "E_NO_GSAP_SCRIPT", - "No GSAP script block found in the composition.", - "This composition does not use GSAP animations.", - ); - return CAN_OK; + return gsapScriptMissing(parsed) ?? CAN_OK; + case "unrollDynamicAnimations": + return ( + gsapScriptMissing(parsed) ?? + gsapAnimationMissing(parsed, op.animationId) ?? + (op.elements.length === 0 + ? canErr( + "E_INVALID_ARGS", + "unrollDynamicAnimations requires at least one element.", + "An empty element list would delete the animation; pass the resolved element list.", + ) + : CAN_OK) + ); + case "materializeKeyframes": + return ( + gsapScriptMissing(parsed) ?? + gsapAnimationMissing(parsed, op.animationId) ?? + (op.keyframes.length === 0 + ? canErr( + "E_INVALID_ARGS", + "materializeKeyframes requires at least one keyframe.", + "An empty keyframe list would empty the animation; pass the resolved keyframes.", + ) + : CAN_OK) + ); default: return canErr("E_UNKNOWN_OP", `Unknown op type: "${(op as EditOp).type}".`); } diff --git a/packages/sdk/src/engine/patches.ts b/packages/sdk/src/engine/patches.ts index a1776d928..8cfd4afca 100644 --- a/packages/sdk/src/engine/patches.ts +++ b/packages/sdk/src/engine/patches.ts @@ -5,7 +5,7 @@ * /elements/{hfId}/inlineStyles/{camelCaseProp} * /elements/{hfId}/text * /elements/{hfId}/attributes/{name} - * /elements/{hfId}/timing/{start|end|trackIndex} ← end = computed absolute data-end + * /elements/{hfId}/timing/{start|end|duration|trackIndex} ← end = computed absolute data-end * /elements/{hfId}/hold/{start|end|fill} * /elements/{hfId} ← whole subtree (removeElement) * /variables/{variableId} @@ -59,7 +59,7 @@ export function attrPath(id: string, name: string): string { return `/elements/${escapeIdForPath(id)}/attributes/${escapedName}`; } -export function timingPath(id: string, field: "start" | "end" | "trackIndex"): string { +export function timingPath(id: string, field: "start" | "end" | "duration" | "trackIndex"): string { return `/elements/${escapeIdForPath(id)}/timing/${field}`; } @@ -154,8 +154,9 @@ export function keyToPath(key: string): string | null { // the already-encoded attr segment. Reconstruct manually. if (attr?.[1] && attr[2]) return `/elements/${escapeIdForPath(attr[1])}/attributes/${attr[2]}`; - const timing = /^([^.]+)\.timing\.(start|end|trackIndex)$/.exec(key); - if (timing?.[1]) return timingPath(timing[1], timing[2] as "start" | "end" | "trackIndex"); + const timing = /^([^.]+)\.timing\.(start|end|duration|trackIndex)$/.exec(key); + if (timing?.[1]) + return timingPath(timing[1], timing[2] as "start" | "end" | "duration" | "trackIndex"); const hold = /^([^.]+)\.hold\.(start|end|fill)$/.exec(key); if (hold?.[1]) return holdPath(hold[1], hold[2] as "start" | "end" | "fill"); diff --git a/packages/sdk/src/session.test.ts b/packages/sdk/src/session.test.ts index d5a3b543a..e6e1ccc32 100644 --- a/packages/sdk/src/session.test.ts +++ b/packages/sdk/src/session.test.ts @@ -299,6 +299,41 @@ describe("override-set orphan cleanup on removeElement", () => { }); }); +describe("single-dispatch undo reverses the inverse patch list", () => { + // A single dispatch that emits order-dependent inverse patches (here a nested + // parent+child removeElement) must undo in reverse application order. Without + // the reverse, undo replays 'add child' before 'add parent' → the child has no + // parent to attach to and is dropped. + it("removeElement([child, parent]) undo restores both, child included", async () => { + const NESTED = `
+
x
+
`; + const comp = await openComposition(NESTED); + comp.dispatch({ type: "removeElement", target: ["hf-child", "hf-parent"] }); + expect(comp.getElement("hf-parent")).toBeNull(); + expect(comp.getElement("hf-child")).toBeNull(); + + comp.undo(); + expect(comp.getElement("hf-parent")).not.toBeNull(); + expect(comp.getElement("hf-child")).not.toBeNull(); + }); + + // Defense-in-depth: an aliased multi-target (the same element twice) makes the + // 2nd id capture the value the 1st already wrote; undo must replay the inverse + // in reverse to land on the ORIGINAL, not the intermediate. + it("setStyle with a duplicate target undoes to the original, not the intermediate", async () => { + const comp = await openComposition(BASE_HTML); + comp.dispatch({ + type: "setStyle", + target: ["hf-title", "hf-title"], + styles: { fontSize: "96px" }, + }); + expect(comp.getElement("hf-title")?.inlineStyles.fontSize).toBe("96px"); + comp.undo(); + expect(comp.getElement("hf-title")?.inlineStyles.fontSize).toBe("64px"); + }); +}); + // ─── setSelection / getSelection / selectionchange ─────────────────────────── describe("setSelection", () => { diff --git a/packages/sdk/src/session.ts b/packages/sdk/src/session.ts index 50cc5e680..b8d897f97 100644 --- a/packages/sdk/src/session.ts +++ b/packages/sdk/src/session.ts @@ -297,7 +297,13 @@ class CompositionImpl implements Composition { this.batchInverse.push(...inverse); if (!this.batchOpTypes.includes(op.type)) this.batchOpTypes.push(op.type); } else { - const event = buildPatchEvent(forward, inverse, origin, [op.type]); + // Reverse the inverse list (parity with batch() below): an op that emits + // multiple patches whose undo order matters — same path (reorderElements + // with a duplicate target), an aliased multi-target, or a nested + // parent+child removeElement — must undo in reverse application order, or + // undo lands on an intermediate value / drops a subtree. Harmless for the + // common single-patch / independent-path case. + const event = buildPatchEvent(forward, [...inverse].reverse(), origin, [op.type]); this.patchHandlers.forEach((h) => h(event)); this.changeHandlers.forEach((h) => h()); } @@ -501,12 +507,17 @@ export async function openComposition( const isEmbedded = opts?.overrides !== undefined; - if (!isEmbedded && opts?.history !== false) { - const history = createHistory(session, { - coalesceMs: opts?.coalesceMs ?? 300, - trackedOrigins: opts?.trackedOrigins, - }); - session.attachHistory(history); + if (!isEmbedded) { + // history:false opts out of the SDK undo stack ONLY. Persist (auto-save) is + // independent — gating it on the history flag too would silently drop every + // disk write for a caller that just wanted to disable undo (data loss). + if (opts?.history !== false) { + const history = createHistory(session, { + coalesceMs: opts?.coalesceMs ?? 300, + trackedOrigins: opts?.trackedOrigins, + }); + session.attachHistory(history); + } if (opts?.persist) { const pq = createPersistQueue(session, opts.persist, { diff --git a/packages/sdk/src/smoke.test.ts b/packages/sdk/src/smoke.test.ts index dbf69d746..c1d038df6 100644 --- a/packages/sdk/src/smoke.test.ts +++ b/packages/sdk/src/smoke.test.ts @@ -227,6 +227,20 @@ describe("persist adapter", () => { expect(content).toContain("color: #f00"); }); + it("still persists when history:false (undo opt-out must not disable auto-save)", async () => { + const adapter = createMemoryAdapter(); + const writeSpy = vi.spyOn(adapter, "write"); + + const comp = await openComposition(BASE_HTML, { persist: adapter, history: false }); + expect(comp.canUndo()).toBe(false); // undo is off… + comp.setStyle("hf-title", { color: "#f00" }); + await comp.flush(); + + expect(writeSpy).toHaveBeenCalled(); // …but the write still happened + const [, content] = writeSpy.mock.calls[0] as [string, string]; + expect(content).toContain("color: #f00"); + }); + it("surfaces persist errors via on('persist:error')", async () => { const adapter = createMemoryAdapter(); const errors: unknown[] = []; diff --git a/packages/sdk/src/types.ts b/packages/sdk/src/types.ts index 1abc560a3..66a453ad7 100644 --- a/packages/sdk/src/types.ts +++ b/packages/sdk/src/types.ts @@ -101,7 +101,6 @@ export type EditOp = position: number; value: Record; } - | { type: "removeGsapKeyframe"; animationId: string; keyframeIndex: number } | { type: "removeGsapKeyframe"; animationId: string; percentage: number } | { type: "removeGsapProperty"; animationId: string; property: string; from?: boolean } | { type: "removeGsapTween"; animationId: string } diff --git a/packages/studio/src/hooks/sdkSelfWriteRegistry.test.ts b/packages/studio/src/hooks/sdkSelfWriteRegistry.test.ts new file mode 100644 index 000000000..3f6ee6354 --- /dev/null +++ b/packages/studio/src/hooks/sdkSelfWriteRegistry.test.ts @@ -0,0 +1,67 @@ +import { describe, expect, it, beforeEach } from "vitest"; +import { + hashContent, + markSelfWrite, + isSelfWriteEcho, + resetSelfWriteRegistry, +} from "./sdkSelfWriteRegistry"; +import { shouldReloadOnFileChange } from "./useSdkSession"; + +describe("sdkSelfWriteRegistry (finding #14)", () => { + beforeEach(() => resetSelfWriteRegistry()); + + it("recognizes the echo of bytes we just wrote", () => { + markSelfWrite("/comp.html", "A"); + expect(isSelfWriteEcho("/comp.html", "A")).toBe(true); + }); + + it("does NOT match different content on the same path (an undo's reverted bytes)", () => { + markSelfWrite("/comp.html", "A"); + expect(isSelfWriteEcho("/comp.html", "REVERTED")).toBe(false); + }); + + it("is keyed per file — a self-write to one file can't mask a change to another", () => { + markSelfWrite("/a.html", "A"); + expect(isSelfWriteEcho("/b.html", "A")).toBe(false); + }); + + it("consumes a matched entry so a later genuine external write isn't suppressed", () => { + markSelfWrite("/comp.html", "A"); + expect(isSelfWriteEcho("/comp.html", "A")).toBe(true); + // A second arrival of identical bytes is NOT our echo — must reload. + expect(isSelfWriteEcho("/comp.html", "A")).toBe(false); + }); + + it("expires entries past the TTL so a stale self-write can't suppress forever", () => { + const t0 = 1_000_000; + markSelfWrite("/comp.html", "A", t0); + // 3 s later (> 2 s TTL) the entry is gone. + expect(isSelfWriteEcho("/comp.html", "A", t0 + 3000)).toBe(false); + }); + + it("hashes are stable and distinguish different content", () => { + expect(hashContent("x")).toBe(hashContent("x")); + expect(hashContent("x")).not.toBe(hashContent("y")); + }); +}); + +describe("shouldReloadOnFileChange (finding #14)", () => { + beforeEach(() => resetSelfWriteRegistry()); + + it("suppresses the reload when content matches a registered self-write (cutover echo)", () => { + markSelfWrite("/comp.html", "SELF"); + expect(shouldReloadOnFileChange("/comp.html", "SELF", true)).toBe(false); + }); + + it("reloads on an undo write even inside the suppress window (content differs)", () => { + // The cutover registered SELF; the undo writes REVERTED bytes within the + // same 2 s window. Time-only suppression dropped this; identity reloads it. + markSelfWrite("/comp.html", "SELF"); + expect(shouldReloadOnFileChange("/comp.html", "REVERTED", true)).toBe(true); + }); + + it("falls back to the time window only when content is unavailable", () => { + expect(shouldReloadOnFileChange("/comp.html", null, true)).toBe(false); + expect(shouldReloadOnFileChange("/comp.html", null, false)).toBe(true); + }); +}); diff --git a/packages/studio/src/hooks/sdkSelfWriteRegistry.ts b/packages/studio/src/hooks/sdkSelfWriteRegistry.ts new file mode 100644 index 000000000..3b6976d1f --- /dev/null +++ b/packages/studio/src/hooks/sdkSelfWriteRegistry.ts @@ -0,0 +1,77 @@ +/** + * Self-write identity registry — discriminates an SDK cutover ECHO from a genuine + * external write (notably undo/redo) in the file-change reload-suppression path. + * + * The old suppression was purely time-based: any file-change within 2 s of the + * shared `domEditSaveTimestampRef` was swallowed. But BOTH an SDK cutover + * self-write AND an undo write set that same timestamp, so the window could not + * tell "the echo of the bytes I just wrote" (suppress) from "the reverted bytes + * an undo just wrote" (must reload). An undo that landed inside the window was + * silently dropped, leaving the in-memory SDK doc on stale pre-undo content. + * + * Fix: tag each cutover self-write with the CONTENT it wrote (by hash). A + * file-change reload is suppressed only when the new on-disk content matches a + * recently-registered self-write hash — i.e. it is provably our own echo. Undo + * writes are never registered (they don't flow through persistSdkSerialize), so + * their content won't match and the reload always fires. Identity, not a clock. + */ + +const SELF_WRITE_TTL_MS = 2000; + +interface SelfWriteEntry { + hash: string; + at: number; +} + +// Module-scoped: the studio process has a single SDK session lifecycle at a time +// and persists are funnelled through one persistSdkSerialize. Keyed by file path +// so a self-write to one file can't mask a real external change to another. +const registry = new Map(); + +/** + * Stable 32-bit FNV-1a hash of content. Collisions only risk SUPPRESSING a real + * reload, and only within the 2 s TTL for the exact same file — negligible, and + * strictly safer than the prior time-only window it replaces. + */ +export function hashContent(content: string): string { + let h = 0x811c9dc5; + for (let i = 0; i < content.length; i++) { + h ^= content.charCodeAt(i); + h = Math.imul(h, 0x01000193); + } + return (h >>> 0).toString(16); +} + +function prune(entries: SelfWriteEntry[], now: number): SelfWriteEntry[] { + return entries.filter((e) => now - e.at < SELF_WRITE_TTL_MS); +} + +/** Record that WE wrote `content` to `path` (an SDK cutover self-write). */ +export function markSelfWrite(path: string, content: string, now: number = Date.now()): void { + const next = prune(registry.get(path) ?? [], now); + next.push({ hash: hashContent(content), at: now }); + registry.set(path, next); +} + +/** + * True when `content` matches a self-write registered for `path` within the TTL. + * Consumes the matched entry so a later genuinely-external write of identical + * bytes isn't suppressed forever. + */ +export function isSelfWriteEcho(path: string, content: string, now: number = Date.now()): boolean { + const entries = prune(registry.get(path) ?? [], now); + const hash = hashContent(content); + const idx = entries.findIndex((e) => e.hash === hash); + if (idx === -1) { + registry.set(path, entries); + return false; + } + entries.splice(idx, 1); + registry.set(path, entries); + return true; +} + +/** Test-only: drop all registered self-writes. */ +export function resetSelfWriteRegistry(): void { + registry.clear(); +} diff --git a/packages/studio/src/hooks/timelineEditingHelpers.ts b/packages/studio/src/hooks/timelineEditingHelpers.ts index 1fd7de1ac..b35f8ee39 100644 --- a/packages/studio/src/hooks/timelineEditingHelpers.ts +++ b/packages/studio/src/hooks/timelineEditingHelpers.ts @@ -97,6 +97,7 @@ export interface PersistTimelineEditInput { recordEdit: (input: RecordEditInput) => Promise; domEditSaveTimestampRef: React.MutableRefObject; pendingTimelineEditPathRef: React.MutableRefObject>; + coalesceKey?: string; } export async function persistTimelineEdit(input: PersistTimelineEditInput): Promise { @@ -119,6 +120,7 @@ export async function persistTimelineEdit(input: PersistTimelineEditInput): Prom projectId: input.projectId, label: input.label, kind: "timeline", + coalesceKey: input.coalesceKey, files: { [targetPath]: patchedContent }, readFile: async () => originalContent, writeFile: input.writeProjectFile, diff --git a/packages/studio/src/hooks/useGsapPropertyDebounce.test.ts b/packages/studio/src/hooks/useGsapPropertyDebounce.test.ts new file mode 100644 index 000000000..4c2a67fa9 --- /dev/null +++ b/packages/studio/src/hooks/useGsapPropertyDebounce.test.ts @@ -0,0 +1,70 @@ +// @vitest-environment happy-dom +import { describe, it, expect } from "vitest"; +import { openComposition } from "@hyperframes/sdk"; +import { createMemoryAdapter } from "@hyperframes/sdk/adapters/memory"; +import { parseGsapScriptAcorn } from "@hyperframes/core/gsap-parser-acorn"; +import { mergeTweenProperties } from "./useGsapPropertyDebounce"; +import { extractGsapScriptText } from "../utils/gsapSoftReload"; + +const HTML = ` +
+ +`; + +const FROMTO_HTML = ` +
+ +`; + +function tweenProps(comp: { serialize(): string }) { + const parsed = parseGsapScriptAcorn(extractGsapScriptText(comp.serialize()) ?? ""); + const anim = parsed.animations[0]; + return { id: anim?.id, properties: anim?.properties, fromProperties: anim?.fromProperties }; +} + +describe("setGsapTween replace semantics (finding #1)", () => { + it("REGRESSION: a single-key set drops the tween's other animated props", async () => { + // This documents the bug the merge fixes: setGsapTween REPLACES the property + // set, so sending only the edited key loses the siblings. + const comp = await openComposition(HTML, { persist: createMemoryAdapter() }); + const id = tweenProps(comp).id ?? ""; + comp.setGsapTween(id, { properties: { x: 200 } }); + const after = tweenProps(comp); + expect(after.properties).toEqual({ x: 200 }); + expect(after.properties).not.toHaveProperty("y"); + expect(after.properties).not.toHaveProperty("opacity"); + }); +}); + +describe("mergeTweenProperties (finding #1)", () => { + it("editing x preserves y and opacity through a real SDK write", async () => { + const comp = await openComposition(HTML, { persist: createMemoryAdapter() }); + const id = tweenProps(comp).id ?? ""; + // Mirror the send site: merge the single edited prop into the existing set. + const merged = mergeTweenProperties(comp, id, { x: 200 }, "to"); + expect(merged).toEqual({ x: 200, y: 50, opacity: 1 }); + comp.setGsapTween(id, { properties: merged }); + const after = tweenProps(comp); + expect(after.properties).toMatchObject({ x: 200, y: 50, opacity: 1 }); + }); + + it("editing a from-property preserves the other from-properties", async () => { + const comp = await openComposition(FROMTO_HTML, { persist: createMemoryAdapter() }); + const id = tweenProps(comp).id ?? ""; + const merged = mergeTweenProperties(comp, id, { x: 25 }, "from"); + expect(merged).toEqual({ x: 25, y: 0 }); + }); + + it("returns the single edit unchanged when the tween id is unknown", async () => { + const comp = await openComposition(HTML, { persist: createMemoryAdapter() }); + expect(mergeTweenProperties(comp, "no-such-id", { x: 5 }, "to")).toEqual({ x: 5 }); + }); +}); diff --git a/packages/studio/src/hooks/useGsapPropertyDebounce.ts b/packages/studio/src/hooks/useGsapPropertyDebounce.ts index 0d408e02b..60e031c81 100644 --- a/packages/studio/src/hooks/useGsapPropertyDebounce.ts +++ b/packages/studio/src/hooks/useGsapPropertyDebounce.ts @@ -1,16 +1,46 @@ import { useCallback, useEffect, useRef } from "react"; import type { Composition } from "@hyperframes/sdk"; +import { parseGsapScriptAcorn } from "@hyperframes/core/gsap-parser-acorn"; import type { DomEditSelection } from "../components/editor/domEditingTypes"; import { sdkGsapTweenPersist, sdkGsapRemovePropertyPersist, type CutoverDeps, } from "../utils/sdkCutover"; +import { extractGsapScriptText } from "../utils/gsapSoftReload"; import { PROPERTY_DEFAULTS } from "./gsapScriptCommitHelpers"; import type { SafeGsapCommitMutation } from "./gsapScriptCommitTypes"; const DEBOUNCE_MS = 150; +/** + * The SDK `setGsapTween` 'set' path REPLACES a tween's editable property set + * (engine `handleSetGsapTween` → `updateAnimationInScript`), so sending only the + * single edited key would silently drop the tween's other animated props. Mirror + * the legacy server path (`{ ...anim.properties, [property]: val }`): read the + * tween's CURRENT properties from the in-memory SDK doc and merge the one edit in, + * so REPLACE semantics preserve siblings. Returns the single-key map unchanged + * when the tween/script can't be found (best-effort; before===after then falls + * back to the server path). + */ +export function mergeTweenProperties( + sdkSession: Composition, + animationId: string, + edited: Record, + kind: "to" | "from", +): Record { + try { + const script = extractGsapScriptText(sdkSession.serialize()); + if (!script) return { ...edited }; + const anim = parseGsapScriptAcorn(script).animations.find((a) => a.id === animationId); + if (!anim) return { ...edited }; + const existing = kind === "from" ? (anim.fromProperties ?? {}) : anim.properties; + return { ...existing, ...edited }; + } catch { + return { ...edited }; + } +} + interface SdkPropertyDeps { sdkSession?: Composition | null; sdkDeps?: CutoverDeps | null; @@ -29,17 +59,32 @@ export function useGsapPropertyDebounce( } | null>(null); const debounceTimerRef = useRef | null>(null); + // The caller passes `sdk` as a fresh object literal every render. Keying any + // callback's deps on it (esp. flushPendingPropertyEdit, whose identity drives + // the unmount-flush cleanup effect) re-fires the cleanup on EVERY parent + // re-render — so a playhead tick mid-slider-drag would flush + record an undo + // entry per render. Hold the latest value in a ref instead so every callback + // reads current deps without re-subscribing on identity churn. + const sdkRef = useRef(sdk); + sdkRef.current = sdk; + const flushPendingPropertyEdit = useCallback(async () => { const pending = pendingPropertyEditRef.current; if (!pending) return; pendingPropertyEditRef.current = null; const { selection, animationId, property, value } = pending; - const { sdkSession, sdkDeps, activeCompPath } = sdk ?? {}; + const { sdkSession, sdkDeps, activeCompPath } = sdkRef.current ?? {}; if (sdkSession && sdkDeps) { const targetPath = selection.sourceFile || activeCompPath || "index.html"; const handled = await sdkGsapTweenPersist( targetPath, - { kind: "set", animationId, properties: { properties: { [property]: value } } }, + { + kind: "set", + animationId, + properties: { + properties: mergeTweenProperties(sdkSession, animationId, { [property]: value }, "to"), + }, + }, sdkSession, sdkDeps, { label: `Edit GSAP ${property}`, coalesceKey: `gsap:${animationId}:${property}` }, @@ -55,7 +100,7 @@ export function useGsapPropertyDebounce( softReload: true, }, ); - }, [commitMutationSafely, sdk]); + }, [commitMutationSafely]); const updateGsapProperty = useCallback( ( @@ -93,12 +138,23 @@ export function useGsapPropertyDebounce( const cs = el.ownerDocument.defaultView?.getComputedStyle(el); defaultValue = cs ? Number.parseFloat(cs.opacity) || 1 : 1; } - const { sdkSession, sdkDeps, activeCompPath } = sdk ?? {}; + const { sdkSession, sdkDeps, activeCompPath } = sdkRef.current ?? {}; if (sdkSession && sdkDeps) { const targetPath = selection.sourceFile || activeCompPath || "index.html"; const handled = await sdkGsapTweenPersist( targetPath, - { kind: "set", animationId, properties: { properties: { [property]: defaultValue } } }, + { + kind: "set", + animationId, + properties: { + properties: mergeTweenProperties( + sdkSession, + animationId, + { [property]: defaultValue }, + "to", + ), + }, + }, sdkSession, sdkDeps, { label: `Add GSAP ${property}` }, @@ -111,12 +167,12 @@ export function useGsapPropertyDebounce( { label: `Add GSAP ${property}` }, ); }, - [commitMutationSafely, sdk], + [commitMutationSafely], ); const removeProperty = useCallback( async (selection: DomEditSelection, animationId: string, property: string, from: boolean) => { - const { sdkSession, sdkDeps, activeCompPath } = sdk ?? {}; + const { sdkSession, sdkDeps, activeCompPath } = sdkRef.current ?? {}; if (sdkSession && sdkDeps) { const targetPath = selection.sourceFile || activeCompPath || "index.html"; const handled = await sdkGsapRemovePropertyPersist( @@ -148,7 +204,7 @@ export function useGsapPropertyDebounce( ); } }, - [commitMutationSafely, sdk], + [commitMutationSafely], ); const removeGsapProperty = useCallback( @@ -164,12 +220,23 @@ export function useGsapPropertyDebounce( property: string, value: number | string, ) => { - const { sdkSession, sdkDeps, activeCompPath } = sdk ?? {}; + const { sdkSession, sdkDeps, activeCompPath } = sdkRef.current ?? {}; if (sdkSession && sdkDeps) { const targetPath = selection.sourceFile || activeCompPath || "index.html"; const handled = await sdkGsapTweenPersist( targetPath, - { kind: "set", animationId, properties: { fromProperties: { [property]: value } } }, + { + kind: "set", + animationId, + properties: { + fromProperties: mergeTweenProperties( + sdkSession, + animationId, + { [property]: value }, + "from", + ), + }, + }, sdkSession, sdkDeps, { @@ -188,13 +255,13 @@ export function useGsapPropertyDebounce( }, ); }, - [commitMutationSafely, sdk], + [commitMutationSafely], ); const addGsapFromProperty = useCallback( async (selection: DomEditSelection, animationId: string, property: string) => { const defaultValue = PROPERTY_DEFAULTS[property] ?? 0; - const { sdkSession, sdkDeps, activeCompPath } = sdk ?? {}; + const { sdkSession, sdkDeps, activeCompPath } = sdkRef.current ?? {}; if (sdkSession && sdkDeps) { const targetPath = selection.sourceFile || activeCompPath || "index.html"; const handled = await sdkGsapTweenPersist( @@ -202,7 +269,14 @@ export function useGsapPropertyDebounce( { kind: "set", animationId, - properties: { fromProperties: { [property]: defaultValue } }, + properties: { + fromProperties: mergeTweenProperties( + sdkSession, + animationId, + { [property]: defaultValue }, + "from", + ), + }, }, sdkSession, sdkDeps, @@ -216,7 +290,7 @@ export function useGsapPropertyDebounce( { label: `Add GSAP from-${property}` }, ); }, - [commitMutationSafely, sdk], + [commitMutationSafely], ); const removeGsapFromProperty = useCallback( diff --git a/packages/studio/src/hooks/useGsapPropertyDebounceFlush.test.ts b/packages/studio/src/hooks/useGsapPropertyDebounceFlush.test.ts new file mode 100644 index 000000000..7890f0d00 --- /dev/null +++ b/packages/studio/src/hooks/useGsapPropertyDebounceFlush.test.ts @@ -0,0 +1,85 @@ +// @vitest-environment happy-dom +import React, { act, useState } from "react"; +import { createRoot } from "react-dom/client"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { useGsapPropertyDebounce } from "./useGsapPropertyDebounce"; +import type { DomEditSelection } from "../components/editor/domEditingTypes"; + +globalThis.IS_REACT_ACT_ENVIRONMENT = true; + +// The SDK path is gated on STUDIO_SDK_CUTOVER_ENABLED; keep it OFF so the flush +// routes through commitMutationSafely (the spy we count), keeping the test about +// flush TIMING, not the SDK write path. +vi.mock("../components/editor/manualEditingAvailability", () => ({ + STUDIO_SDK_CUTOVER_ENABLED: false, +})); +vi.mock("../utils/studioTelemetry", () => ({ trackStudioEvent: vi.fn() })); + +const selection = { sourceFile: "index.html" } as unknown as DomEditSelection; + +describe("useGsapPropertyDebounce flush stability (finding #7)", () => { + let container: HTMLDivElement; + beforeEach(() => { + vi.useFakeTimers(); + container = document.createElement("div"); + document.body.appendChild(container); + }); + afterEach(() => { + vi.useRealTimers(); + container.remove(); + }); + + it("re-rendering the parent while an edit is pending does NOT flush early or duplicate commits", () => { + const commitMutationSafely = vi.fn(); + let queueEdit: (() => void) | null = null; + let forceRerender: (() => void) | null = null; + + function Harness() { + const [tick, setTick] = useState(0); + forceRerender = () => setTick((t) => t + 1); + // A FRESH sdk wrapper literal every render — the exact churn that, before + // the ref-stabilization fix, re-fired the unmount-flush cleanup effect. + const ops = useGsapPropertyDebounce(commitMutationSafely, { + sdkSession: null, + sdkDeps: null, + activeCompPath: "index.html", + }); + queueEdit = () => ops.updateGsapProperty(selection, "tw-1", "x", tick + 1); + return React.createElement("div", null, String(tick)); + } + + const root = createRoot(container); + act(() => { + root.render(React.createElement(Harness)); + }); + + // Queue one pending edit. + act(() => { + queueEdit?.(); + }); + expect(commitMutationSafely).not.toHaveBeenCalled(); + + // Re-render the parent several times BEFORE the debounce elapses. The bug + // flushed (and recorded a commit) on every re-render via the cleanup effect. + act(() => { + forceRerender?.(); + }); + act(() => { + forceRerender?.(); + }); + act(() => { + forceRerender?.(); + }); + expect(commitMutationSafely).not.toHaveBeenCalled(); + + // The debounce fires exactly once. + act(() => { + vi.advanceTimersByTime(200); + }); + expect(commitMutationSafely).toHaveBeenCalledTimes(1); + + act(() => { + root.unmount(); + }); + }); +}); diff --git a/packages/studio/src/hooks/useGsapScriptCommits.ts b/packages/studio/src/hooks/useGsapScriptCommits.ts index 936e1fb8b..1c15af70d 100644 --- a/packages/studio/src/hooks/useGsapScriptCommits.ts +++ b/packages/studio/src/hooks/useGsapScriptCommits.ts @@ -115,6 +115,28 @@ export function useGsapScriptCommits({ projectIdRef, activeCompPath, previewIfra }, [previewIframeRef, reloadPreview, onCacheInvalidate], ); + // Reuse the SAME per-file serializer the legacy commitMutation path uses, so + // SDK gsap-write flushes serialize against legacy commits AND each other — + // overlapping same-file read-modify-writes can't interleave and lose an edit. + const serializeByFile = useCallback( + (key: string, task: () => Promise): Promise => serializerRef.current(key, task), + [], + ); + // Read the on-disk bytes of targetPath so the SDK GSAP persist captures the + // exact prior content as its undo `before` (matching the style/delete paths), + // instead of a normalized full-DOM re-emit that would reformat the whole file. + const readProjectFileContent = useCallback( + async (path: string): Promise => { + const pid = projectIdRef.current; + if (!pid) throw new Error("No active project"); + const res = await fetch(`/api/projects/${pid}/files/${encodeURIComponent(path)}`); + if (!res.ok) throw new Error(`Failed to read ${path}`); + const data = (await res.json()) as { content?: string }; + if (typeof data.content !== "string") throw new Error(`Missing file contents for ${path}`); + return data.content; + }, + [projectIdRef], + ); const sdkDeps = useMemo( () => writeProjectFile @@ -125,6 +147,8 @@ export function useGsapScriptCommits({ projectIdRef, activeCompPath, previewIfra domEditSaveTimestampRef, refresh: sdkRefresh, compositionPath: activeCompPath, + serialize: serializeByFile, + readProjectFile: readProjectFileContent, } : null, [ @@ -134,6 +158,8 @@ export function useGsapScriptCommits({ projectIdRef, activeCompPath, previewIfra domEditSaveTimestampRef, sdkRefresh, activeCompPath, + serializeByFile, + readProjectFileContent, ], ); diff --git a/packages/studio/src/hooks/useSdkSession.ts b/packages/studio/src/hooks/useSdkSession.ts index 1743814fb..e7b8c7e23 100644 --- a/packages/studio/src/hooks/useSdkSession.ts +++ b/packages/studio/src/hooks/useSdkSession.ts @@ -4,6 +4,7 @@ import { openComposition } from "@hyperframes/sdk"; import { createHttpAdapter } from "@hyperframes/sdk/adapters/http"; import type { Composition } from "@hyperframes/sdk"; import { readStudioFileChangePath } from "../components/editor/manualEdits"; +import { isSelfWriteEcho } from "./sdkSelfWriteRegistry"; /** * True when an external file-change payload targets the active composition and @@ -24,14 +25,40 @@ export function shouldReloadSdkSession(payload: unknown, activeCompPath: string * stale. The session has NO persist queue — Studio is the sole file writer; see * the open effect below. */ -// Time-window heuristic: suppress file-change reloads for 2 s after our own -// SDK cutover write, to avoid an echo-reload on the write we just committed. -// Footgun: if 2 s is too short (slow FS / network) the reload fires anyway; -// if too long it masks a legitimate external edit. The long-term shape is a -// sequence number or content hash threaded through the persist event so the -// comparison is exact rather than time-based. +// Reload-suppression baseline: a file-change within this window of our own SDK +// cutover write is a CANDIDATE echo, but the decision is content-identity based +// (isSelfWriteEcho) not time-only — so an undo write that lands inside the window +// still reloads (its reverted bytes were never registered as a self-write). The +// window only bounds how long a registered self-write stays suppressible. const SELF_WRITE_SUPPRESS_MS = 2000; +/** Best-effort read of the changed file's content from a file-change payload. */ +function readFileChangeContent(payload: unknown): string | null { + if (!payload || typeof payload !== "object") return null; + const record = payload as Record; + if (typeof record.content === "string") return record.content; + if ("data" in record) return readFileChangeContent(record.data); + return null; +} + +/** + * Decide whether a file-change for the active composition should reload the SDK + * session. `content` is the new on-disk bytes (from the payload or a re-read); + * pass null when unavailable. Content-identity wins: a change whose bytes match a + * registered self-write is our own echo (suppress). Without content we can't prove + * identity, so we fall back to the time window ONLY to suppress an echo — an undo + * write outside the window (or any non-self-write) still reloads. Exported for test. + */ +export function shouldReloadOnFileChange( + activeCompPath: string, + content: string | null, + withinSuppressWindow: boolean, +): boolean { + if (content != null) return !isSelfWriteEcho(activeCompPath, content); + // No content to compare — preserve the old time-window echo suppression. + return !withinSuppressWindow; +} + export interface SdkSessionHandle { session: Composition | null; /** @@ -53,15 +80,30 @@ export function useSdkSession( // ── Re-open on external change to the active composition ── useEffect(() => { if (!activeCompPath) return; + const compPath = activeCompPath; + const readAdapter = + projectId != null + ? createHttpAdapter({ projectFilesUrl: `/api/projects/${projectId}` }) + : null; const handler = (payload?: unknown) => { - if (!shouldReloadSdkSession(payload, activeCompPath)) return; - // Suppress reload triggered by our own SDK cutover write. - if ( - domEditSaveTimestampRef && - Date.now() - domEditSaveTimestampRef.current < SELF_WRITE_SUPPRESS_MS - ) + if (!shouldReloadSdkSession(payload, compPath)) return; + const withinWindow = + !!domEditSaveTimestampRef && + Date.now() - domEditSaveTimestampRef.current < SELF_WRITE_SUPPRESS_MS; + const decide = (content: string | null) => { + if (shouldReloadOnFileChange(compPath, content, withinWindow)) setReloadToken((t) => t + 1); + }; + const payloadContent = readFileChangeContent(payload); + // Prefer payload content; otherwise re-read so the decision is by IDENTITY + // (an undo's reverted bytes won't match a registered self-write → reload). + if (payloadContent != null || !readAdapter) { + decide(payloadContent); return; - setReloadToken((t) => t + 1); + } + readAdapter + .read(compPath) + .then((c) => decide(typeof c === "string" ? c : null)) + .catch(() => decide(null)); }; if (import.meta.hot) { import.meta.hot.on("hf:file-change", handler); @@ -72,7 +114,7 @@ export function useSdkSession( es.addEventListener("file-change", handler); return () => es.close(); // eslint-disable-next-line react-hooks/exhaustive-deps - }, [activeCompPath]); + }, [activeCompPath, projectId]); // ── Open / re-open the session ── useEffect(() => { diff --git a/packages/studio/src/hooks/useTimelineEditing.ts b/packages/studio/src/hooks/useTimelineEditing.ts index b9d85d4ca..688d4127a 100644 --- a/packages/studio/src/hooks/useTimelineEditing.ts +++ b/packages/studio/src/hooks/useTimelineEditing.ts @@ -29,6 +29,8 @@ import { readFileContent, applyPatchByTarget, formatTimelineAttributeNumber, + shiftGsapPositions, + scaleGsapPositions, } from "./timelineEditingHelpers"; import type { PersistTimelineEditInput } from "./timelineEditingHelpers"; import { sdkTimingPersist } from "../utils/sdkCutover"; @@ -91,6 +93,7 @@ export function useTimelineEditing({ element: TimelineElement, label: string, buildPatches: PersistTimelineEditInput["buildPatches"], + coalesceKey?: string, ): Promise => { if (isRecordingRef?.current) { showToast("Cannot edit timeline while recording", "error"); @@ -110,6 +113,7 @@ export function useTimelineEditing({ recordEdit, domEditSaveTimestampRef, pendingTimelineEditPathRef, + coalesceKey, }), ) .then(() => { @@ -154,6 +158,24 @@ export function useTimelineEditing({ value: String(updates.track), }); }; + // Server-path fallback (no SDK session): persist the attr patch, then + // shift GSAP tween positions on the server and reload the preview — the + // SDK path folds both into setTiming, but the fallback must do them + // explicitly or the clip moves while its GSAP tweens stay put + the + // preview never refreshes. coalesceKey mirrors the SDK branch so undo + // granularity is identical on either path. + const coalesceKey = `timeline-move:${element.hfId ?? element.id}`; + const moveFallback = () => + enqueueEdit(element, "Move timeline clip", buildMovePatches, coalesceKey).then(() => { + const pid = projectIdRef.current; + const delta = updates.start - element.start; + if (delta !== 0 && element.domId && pid) { + return shiftGsapPositions(pid, targetPath, element.domId, delta) + .then(() => reloadPreview()) + .catch((err) => console.error("[Timeline] Failed to shift GSAP positions", err)); + } + return reloadPreview(); + }); if (sdkSession && element.hfId) { return sdkTimingPersist( element.hfId, @@ -166,13 +188,16 @@ export function useTimelineEditing({ reloadPreview, domEditSaveTimestampRef, compositionPath: activeCompPath, + // Capture on-disk bytes as the undo `before` so undoing a timing move + // restores the file verbatim, not a normalized full-DOM re-emit. + readProjectFile: (path) => readFileContent(projectIdRef.current ?? "", path), }, - { label: "Move timeline clip", coalesceKey: `timeline-move:${element.hfId}` }, + { label: "Move timeline clip", coalesceKey }, ).then((handled) => { - if (!handled) return enqueueEdit(element, "Move timeline clip", buildMovePatches); + if (!handled) return moveFallback(); }); } - return enqueueEdit(element, "Move timeline clip", buildMovePatches); + return moveFallback(); }, [ previewIframeRef, @@ -193,10 +218,21 @@ export function useTimelineEditing({ element: TimelineElement, updates: Pick, ) => { - patchIframeDomTiming(previewIframeRef.current, element, [ + const liveAttrs: Array<[string, string]> = [ ["data-start", formatTimelineAttributeNumber(updates.start)], ["data-duration", formatTimelineAttributeNumber(updates.duration)], - ]); + ]; + // Patch the live playback-start/media-start attr too, or a resize that + // trims the playback start leaves the preview showing the old in-point + // until the next reload (the persisted patch handles it via pbs below). + if (updates.playbackStart != null) { + const liveAttr = + element.playbackStartAttr === "playback-start" + ? "data-playback-start" + : "data-media-start"; + liveAttrs.push([liveAttr, formatTimelineAttributeNumber(updates.playbackStart)]); + } + patchIframeDomTiming(previewIframeRef.current, element, liveAttrs); const targetPath = element.sourceFile || activeCompPath || "index.html"; const buildResizePatches: PersistTimelineEditInput["buildPatches"] = (original, target) => { const pbs = resolveResizePlaybackStart(original, target, element, updates); @@ -220,10 +256,38 @@ export function useTimelineEditing({ return patched; }; // SDK path: skip when a playback-start adjustment is needed (setTiming has no pbs field). - // Condition: no explicit pbs override AND (no start change OR element has no pbs attribute). + // The second clause fires because trimming the start of a clip that has a + // playback-start attribute implicitly shifts that in-point — which the SDK + // setTiming op can't express — so those resizes must take the server path. const hasPbsAdjustment = updates.playbackStart != null || (updates.start !== element.start && element.playbackStart != null); + // Server-path fallback: after persisting the attr patch, scale GSAP tween + // positions/durations on the server and reload the preview. The SDK path + // folds both into setTiming; the fallback must do them explicitly or the + // clip resizes while its GSAP tweens keep their old timing + the preview + // never refreshes. coalesceKey mirrors the SDK branch for undo parity. + const coalesceKey = `timeline-resize:${element.hfId ?? element.id}`; + const timingChanged = + updates.start !== element.start || updates.duration !== element.duration; + const resizeFallback = () => + enqueueEdit(element, "Resize timeline clip", buildResizePatches, coalesceKey).then(() => { + const pid = projectIdRef.current; + if (timingChanged && element.domId && pid) { + return scaleGsapPositions( + pid, + targetPath, + element.domId, + element.start, + element.duration, + updates.start, + updates.duration, + ) + .then(() => reloadPreview()) + .catch((err) => console.error("[Timeline] Failed to scale GSAP positions", err)); + } + return reloadPreview(); + }); if (sdkSession && element.hfId && !hasPbsAdjustment) { return sdkTimingPersist( element.hfId, @@ -236,13 +300,16 @@ export function useTimelineEditing({ reloadPreview, domEditSaveTimestampRef, compositionPath: activeCompPath, + // Capture on-disk bytes as the undo `before` so undoing a timing + // resize restores the file verbatim, not a normalized full-DOM re-emit. + readProjectFile: (path) => readFileContent(projectIdRef.current ?? "", path), }, - { label: "Resize timeline clip", coalesceKey: `timeline-resize:${element.hfId}` }, + { label: "Resize timeline clip", coalesceKey }, ).then((handled) => { - if (!handled) return enqueueEdit(element, "Resize timeline clip", buildResizePatches); + if (!handled) return resizeFallback(); }); } - return enqueueEdit(element, "Resize timeline clip", buildResizePatches); + return resizeFallback(); }, [ previewIframeRef, diff --git a/packages/studio/src/utils/sdkCutover.gate.test.ts b/packages/studio/src/utils/sdkCutover.gate.test.ts new file mode 100644 index 000000000..c8592dba8 --- /dev/null +++ b/packages/studio/src/utils/sdkCutover.gate.test.ts @@ -0,0 +1,60 @@ +import { describe, expect, it, vi } from "vitest"; + +// Dark-launch contract: with STUDIO_SDK_CUTOVER_ENABLED=false, EVERY cutover +// persist chokepoint must return false so the caller takes the legacy server +// path — even when a valid SDK session exists (one always does, for +// shadow/selection). This is the contract the prod flag-flip rests on; a future +// refactor of the gate guards that silently re-enables cutover on flag-off +// turns these red. (sdkCutover.test.ts mocks the flag TRUE; this is its sibling.) +vi.mock("../components/editor/manualEditingAvailability", () => ({ + STUDIO_SDK_CUTOVER_ENABLED: false, +})); +vi.mock("./studioTelemetry", () => ({ trackStudioEvent: vi.fn() })); + +import { sdkTimingPersist, sdkGsapTweenPersist, sdkDeletePersist } from "./sdkCutover"; + +const makeSession = () => + ({ + getElement: () => ({ inlineStyles: {} }), + serialize: () => "", + batch: (fn: () => void) => fn(), + setTiming: vi.fn(), + dispatch: vi.fn(), + }) as never; + +const makeDeps = () => + ({ + editHistory: { recordEdit: vi.fn().mockResolvedValue(undefined) }, + writeProjectFile: vi.fn().mockResolvedValue(undefined), + reloadPreview: vi.fn(), + domEditSaveTimestampRef: { current: 0 }, + }) as never; + +describe("dark-launch gate — STUDIO_SDK_CUTOVER_ENABLED=false ⇒ persist returns false", () => { + it("sdkTimingPersist falls back without writing", async () => { + const deps = makeDeps(); + expect(await sdkTimingPersist("hf-a", "/c.html", { start: 1 }, makeSession(), deps)).toBe( + false, + ); + expect( + (deps as unknown as { writeProjectFile: ReturnType }).writeProjectFile, + ).not.toHaveBeenCalled(); + }); + + it("sdkGsapTweenPersist (shared GSAP-op chokepoint) falls back", async () => { + expect( + await sdkGsapTweenPersist( + "/c.html", + { kind: "remove", animationId: "a" }, + makeSession(), + makeDeps(), + ), + ).toBe(false); + }); + + it("sdkDeletePersist falls back", async () => { + expect( + await sdkDeletePersist("hf-a", "", "/c.html", makeSession(), makeDeps()), + ).toBe(false); + }); +}); diff --git a/packages/studio/src/utils/sdkCutover.test.ts b/packages/studio/src/utils/sdkCutover.test.ts index 9e32d1f3c..17279a890 100644 --- a/packages/studio/src/utils/sdkCutover.test.ts +++ b/packages/studio/src/utils/sdkCutover.test.ts @@ -455,6 +455,153 @@ describe("sdkTimingPersist", () => { expect(result).toBe(false); expect(deps.writeProjectFile).not.toHaveBeenCalled(); }); + + // Finding #12: undo baseline must be the EXACT on-disk bytes (matching the + // style/delete paths), not a normalized SDK serialize() re-emit — otherwise + // undoing a timing edit reformats the whole file. + it("records the on-disk content (not serialize()) as the undo before when a reader is provided", async () => { + const deps = { + ...makeDeps(), + readProjectFile: vi.fn().mockResolvedValue("EXACT ON-DISK BYTES"), + }; + const session = makeSession(true); + await sdkTimingPersist("hf-clip", "/comp.html", { start: 3 }, session, deps); + expect(deps.readProjectFile).toHaveBeenCalledWith("/comp.html"); + expect(deps.editHistory.recordEdit).toHaveBeenCalledWith( + expect.objectContaining({ + files: { + "/comp.html": { before: "EXACT ON-DISK BYTES", after: "after" }, + }, + }), + ); + }); + + it("falls back to serialize() before when the reader throws", async () => { + const deps = { + ...makeDeps(), + readProjectFile: vi.fn().mockRejectedValue(new Error("read failed")), + }; + const session = makeSession(true); + await sdkTimingPersist("hf-clip", "/comp.html", { start: 3 }, session, deps); + expect(deps.editHistory.recordEdit).toHaveBeenCalledWith( + expect.objectContaining({ + files: { "/comp.html": { before: "before", after: "after" } }, + }), + ); + }); +}); + +describe("sdkGsapTweenPersist — undo baseline (finding #12)", () => { + const makeRef = (val: T): MutableRefObject => ({ current: val }); + const makeSession = () => + ({ + getElement: vi.fn().mockReturnValue({ id: "hf-box" }), + setGsapTween: vi.fn(), + serialize: vi + .fn() + .mockReturnValueOnce("serialized-before") + .mockReturnValue("after"), + batch: vi.fn((fn: () => void) => fn()), + }) as unknown as Parameters[2]; + + it("records the on-disk content as the undo before, not serialize()", async () => { + const deps = { + editHistory: { recordEdit: vi.fn().mockResolvedValue(undefined) }, + writeProjectFile: vi.fn().mockResolvedValue(undefined), + reloadPreview: vi.fn(), + domEditSaveTimestampRef: makeRef(0), + readProjectFile: vi.fn().mockResolvedValue("on-disk gsap bytes"), + }; + const session = makeSession(); + await sdkGsapTweenPersist( + "/comp.html", + { kind: "set", animationId: "tw-1", properties: { ease: "power3.in" } }, + session, + deps, + ); + expect(deps.editHistory.recordEdit).toHaveBeenCalledWith( + expect.objectContaining({ + files: { + "/comp.html": { before: "on-disk gsap bytes", after: "after" }, + }, + }), + ); + }); +}); + +describe("sdkGsapTweenPersist — per-file serialization (finding #8)", () => { + const makeRef = (val: T): MutableRefObject => ({ current: val }); + + it("routes the read-modify-write through the keyed serializer so same-file flushes can't interleave", async () => { + const order: string[] = []; + let writeResolve: (() => void) | null = null; + const deps = { + editHistory: { recordEdit: vi.fn().mockResolvedValue(undefined) }, + // First write blocks until we release it, so without serialization the + // second op's serialize()/dispatch would interleave ahead of it. + writeProjectFile: vi.fn().mockImplementation((_p: string, content: string) => { + order.push(`write-start:${content}`); + if (content === "after-1") { + return new Promise((res) => { + writeResolve = () => { + order.push(`write-done:${content}`); + res(); + }; + }); + } + order.push(`write-done:${content}`); + return Promise.resolve(); + }), + reloadPreview: vi.fn(), + domEditSaveTimestampRef: makeRef(0), + // A real per-key serializer: tasks under the same key run strictly in order. + serialize: (() => { + const inFlight = new Map>(); + return (key: string, task: () => Promise): Promise => { + const prior = inFlight.get(key) ?? Promise.resolve(); + const next = prior.then(task, task); + inFlight.set(key, next); + return next as Promise; + }; + })(), + }; + + let serializeCall = 0; + const session = { + getElement: vi.fn().mockReturnValue({ id: "hf-box" }), + setGsapTween: vi.fn(() => order.push("dispatch")), + serialize: vi.fn(() => { + serializeCall++; + // before-1, after-1, before-2, after-2 + return `${serializeCall % 2 === 1 ? "before" : "after"}-${Math.ceil(serializeCall / 2)}`; + }), + batch: vi.fn((fn: () => void) => fn()), + } as unknown as Parameters[2]; + + const p1 = sdkGsapTweenPersist( + "/comp.html", + { kind: "set", animationId: "tw-1", properties: { ease: "a" } }, + session, + deps, + ); + const p2 = sdkGsapTweenPersist( + "/comp.html", + { kind: "set", animationId: "tw-1", properties: { ease: "b" } }, + session, + deps, + ); + // Let the first op reach its (blocked) write before releasing it. + await Promise.resolve(); + await Promise.resolve(); + writeResolve?.(); + await Promise.all([p1, p2]); + + // The second op's write must NOT start before the first op's write completes. + const firstWriteDone = order.indexOf("write-done:after-1"); + const secondWriteStart = order.indexOf("write-start:after-2"); + expect(firstWriteDone).toBeGreaterThanOrEqual(0); + expect(secondWriteStart).toBeGreaterThan(firstWriteDone); + }); }); describe("sdkGsapTweenPersist", () => { diff --git a/packages/studio/src/utils/sdkCutover.ts b/packages/studio/src/utils/sdkCutover.ts index a6df2ce4b..09668be60 100644 --- a/packages/studio/src/utils/sdkCutover.ts +++ b/packages/studio/src/utils/sdkCutover.ts @@ -5,6 +5,7 @@ import type { EditHistoryKind } from "./editHistory"; import type { PatchOperation } from "./sourcePatcher"; import { STUDIO_SDK_CUTOVER_ENABLED } from "../components/editor/manualEditingAvailability"; import { trackStudioEvent } from "./studioTelemetry"; +import { markSelfWrite } from "../hooks/sdkSelfWriteRegistry"; const CUTOVER_OP_TYPES = new Set([ "inline-style", @@ -90,6 +91,42 @@ export interface CutoverDeps { * — otherwise we'd write the full active-comp serialization into that file. */ compositionPath?: string | null; + /** + * Optional per-key task serializer (the same `gsap-file:${file}` serializer the + * legacy `commitMutation` uses). When provided, every GSAP-op persist routes its + * read-serialize → dispatch → serialize → write through it so two concurrent + * same-file flushes can't interleave their read-modify-write and lose an edit. + * Absent (e.g. in unit tests) → ops run unserialized as before. + */ + serialize?: (key: string, task: () => Promise) => Promise; + /** + * Optional reader for the on-disk content of targetPath. Timing/GSAP persists + * use it to capture the EXACT prior bytes as the undo-history `before`, so undo + * restores the file verbatim instead of a normalized SDK re-emit (which would + * reformat the whole file). The style/delete paths already thread originalContent + * in explicitly; this gives timing/GSAP parity without touching every call site. + * Absent → falls back to the SDK's pre-edit serialize() (the prior behavior). + */ + readProjectFile?: (path: string) => Promise; +} + +/** + * Capture the undo-history `before` baseline for timing/GSAP persists: the exact + * on-disk bytes when a reader is available (so undo restores them verbatim), + * falling back to the SDK's pre-edit serialization when it isn't. Never throws — + * a failed read degrades to the serialized fallback rather than aborting the edit. + */ +async function captureOnDiskBefore( + deps: CutoverDeps, + targetPath: string, + serializedFallback: string, +): Promise { + if (!deps.readProjectFile) return serializedFallback; + try { + return await deps.readProjectFile(targetPath); + } catch { + return serializedFallback; + } } /** True when targetPath isn't the composition the SDK session models. */ @@ -115,6 +152,11 @@ async function persistSdkSerialize( options?: CutoverOptions, ): Promise { deps.domEditSaveTimestampRef.current = Date.now(); + // Tag this write with the exact content (by hash) so the file-change + // reload-suppression can recognize its own echo by IDENTITY, not just a 2 s + // clock — an undo write (different bytes, not registered here) then always + // reloads instead of being swallowed by the time window. + markSelfWrite(targetPath, after); await deps.writeProjectFile(targetPath, after); await deps.editHistory.recordEdit({ label: options?.label ?? "Edit layer", @@ -171,14 +213,22 @@ export async function sdkTimingPersist( deps: CutoverDeps, options?: CutoverOptions, ): Promise { + // Dark-launch gate: without this, timing cutover runs whenever an SDK session + // exists (it always does, for shadow/selection) — flipping the flag OFF would + // NOT disable it. Gate here so flag-off routes back to the legacy server path. + if (!STUDIO_SDK_CUTOVER_ENABLED) return false; if (!sdkSession || !sdkSession.getElement(hfId)) return false; if (wrongCompositionFile(deps, targetPath)) return false; try { - const before = sdkSession.serialize(); + const serializedBefore = sdkSession.serialize(); sdkSession.batch(() => sdkSession.setTiming(hfId, timingUpdate)); const after = sdkSession.serialize(); - if (after === before) return false; - await persistSdkSerialize(after, targetPath, before, deps, options); + if (after === serializedBefore) return false; + // Undo baseline = exact on-disk bytes (matching the style/delete paths), so + // undoing a timing edit restores the file verbatim instead of a normalized + // full-DOM re-emit. Falls back to serializedBefore when no reader is wired. + const undoBefore = await captureOnDiskBefore(deps, targetPath, serializedBefore); + await persistSdkSerialize(after, targetPath, undoBefore, deps, options); trackStudioEvent("sdk_cutover_success", { hfId, opCount: 1 }); return true; } catch (err) { @@ -199,6 +249,9 @@ export function sdkGsapTweenPersist( deps: CutoverDeps, options?: CutoverOptions, ): Promise { + // Leading dark-launch gate so flag-off does no SDK touch (getElement) at all — + // matches the other three chokepoints' discipline. + if (!STUDIO_SDK_CUTOVER_ENABLED) return Promise.resolve(false); if (op.kind === "add" && sdkSession && !sdkSession.getElement(op.target)) return Promise.resolve(false); // dispatchGsapOpAndPersist returns false on before===after — that catches stale @@ -224,20 +277,35 @@ async function dispatchGsapOpAndPersist( options: CutoverOptions | undefined, dispatch: (s: Composition) => void, ): Promise { + // Dark-launch gate (shared chokepoint for every GSAP-op cutover persist): + // flag OFF → return false → caller falls back to the legacy server path. + if (!STUDIO_SDK_CUTOVER_ENABLED) return false; if (!sdkSession) return false; if (wrongCompositionFile(deps, targetPath)) return false; - try { - const before = sdkSession.serialize(); - dispatch(sdkSession); - const after = sdkSession.serialize(); - if (after === before) return false; - await persistSdkSerialize(after, targetPath, before, deps, options); - trackStudioEvent("sdk_cutover_success", { opCount: 1 }); - return true; - } catch (err) { - trackStudioEvent("sdk_cutover_fallback", { error: String(err) }); - return false; - } + const session = sdkSession; + // Route the whole read-serialize → dispatch → serialize → write through the + // per-file serializer (when provided) so overlapping same-file flushes can't + // interleave their read-modify-write and drop an edit, matching the legacy + // commitMutation path's `gsap-file:${file}` serialization. + const run = async (): Promise => { + try { + const serializedBefore = session.serialize(); + dispatch(session); + const after = session.serialize(); + if (after === serializedBefore) return false; + // Undo baseline = exact on-disk bytes (matching the style/delete paths), so + // undoing a GSAP edit restores the file verbatim instead of a normalized + // full-DOM re-emit. Falls back to serializedBefore when no reader is wired. + const undoBefore = await captureOnDiskBefore(deps, targetPath, serializedBefore); + await persistSdkSerialize(after, targetPath, undoBefore, deps, options); + trackStudioEvent("sdk_cutover_success", { opCount: 1 }); + return true; + } catch (err) { + trackStudioEvent("sdk_cutover_fallback", { error: String(err) }); + return false; + } + }; + return deps.serialize ? deps.serialize(`gsap-file:${targetPath}`, run) : run(); } export function sdkGsapKeyframePersist( @@ -325,6 +393,8 @@ export async function sdkDeletePersist( sdkSession: Composition | null | undefined, deps: CutoverDeps, ): Promise { + // Dark-launch gate: flag OFF → legacy server delete path. + if (!STUDIO_SDK_CUTOVER_ENABLED) return false; if (!sdkSession || !sdkSession.getElement(hfId)) return false; if (wrongCompositionFile(deps, targetPath)) return false; try {