From 530980c2c88eaf02e5b8a0a2496bac709a54573a Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Wed, 17 Jun 2026 11:54:43 -0700 Subject: [PATCH 01/17] fix(studio): restore timeline move/resize fallback parity (review #1466) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The §3.2 sdkTimingPersist rewrite regressed the non-SDK fallback path vs the pre-cutover behavior. Restored, on both fallback entry points (no-session and sdkTimingPersist-returned-unhandled): - Resize live DOM patch dropped the conditional data-playback-start/media-start attr — restored so a start-trim updates the preview's in-point immediately. - Move/resize fallback dropped the GSAP-position sync (shift/scaleGsapPositions) + reloadPreview — restored so server-path edits keep GSAP tweens in sync and refresh the preview (the SDK path folds both into setTiming). - Undo-coalesce drift: fallback enqueueEdit carried no coalesceKey while the SDK branch did — plumbed coalesceKey through persistTimelineEdit so undo granularity is identical on either path. - Documented the hasPbsAdjustment second clause + sdkTimingPersist before-capture transition limitation. Flag-off (dark launch) so this lands as one fix PR at the stack tip rather than restacking the mid-stack §3.2 commit. #1500 review items: parity-harness gap already closed at the tip (arc/unroll recast-vs-acorn parity added); blockRemoveRange flagged 'potential' but verified correct (no comma residue on any block position). Co-Authored-By: Claude Opus 4.8 (1M context) --- .../src/hooks/timelineEditingHelpers.ts | 2 + .../studio/src/hooks/useTimelineEditing.ts | 79 ++++++++++++++++--- packages/studio/src/utils/sdkCutover.ts | 5 ++ 3 files changed, 77 insertions(+), 9 deletions(-) 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/useTimelineEditing.ts b/packages/studio/src/hooks/useTimelineEditing.ts index b9d85d4ca..ffe5c7a45 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, @@ -167,12 +189,12 @@ export function useTimelineEditing({ domEditSaveTimestampRef, compositionPath: activeCompPath, }, - { 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 +215,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 +253,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, @@ -237,12 +298,12 @@ export function useTimelineEditing({ domEditSaveTimestampRef, compositionPath: activeCompPath, }, - { 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.ts b/packages/studio/src/utils/sdkCutover.ts index a6df2ce4b..0c5a55106 100644 --- a/packages/studio/src/utils/sdkCutover.ts +++ b/packages/studio/src/utils/sdkCutover.ts @@ -174,6 +174,11 @@ export async function sdkTimingPersist( if (!sdkSession || !sdkSession.getElement(hfId)) return false; if (wrongCompositionFile(deps, targetPath)) return false; try { + // `before` is the SDK's serialized state, which is the true pre-edit + // content only while every edit routes through this session. During the + // dark-launch transition (server still writes some paths) the in-memory + // SDK doc can drift from disk, so this `before` may not match the file's + // actual prior bytes. Acceptable for v1; revisit once cutover is always-on. const before = sdkSession.serialize(); sdkSession.batch(() => sdkSession.setTiming(hfId, timingUpdate)); const after = sdkSession.serialize(); From cd31bdf50e32ac946f525d5710cba059434bdc46 Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Wed, 17 Jun 2026 12:08:28 -0700 Subject: [PATCH 02/17] fix(sdk): retire duplicate removeGsapKeyframe keyframeIndex variant (review #1498) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit EditOp had two removeGsapKeyframe members with the same discriminant but different shapes (keyframeIndex vs percentage) — TS can't discriminate them and a handler could get the wrong shape. Per both reviewers (option 2): retire the keyframeIndex variant. It had no production caller (Studio dispatches percentage only); removed the dead by-index handleRemoveGsapKeyframe + simplified the dispatcher. resolveKeyframe stays (setGsapKeyframe still uses keyframeIndex). Converted the one by-index test to the percentage API. Co-Authored-By: Claude Opus 4.8 (1M context) --- packages/sdk/src/engine/mutate.gsap.test.ts | 4 ++-- packages/sdk/src/engine/mutate.ts | 22 +-------------------- packages/sdk/src/types.ts | 1 - 3 files changed, 3 insertions(+), 24 deletions(-) diff --git a/packages/sdk/src/engine/mutate.gsap.test.ts b/packages/sdk/src/engine/mutate.gsap.test.ts index 8fc18d73a..b1ea7e58f 100644 --- a/packages/sdk/src/engine/mutate.gsap.test.ts +++ b/packages/sdk/src/engine/mutate.gsap.test.ts @@ -400,13 +400,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 ?? ""); diff --git a/packages/sdk/src/engine/mutate.ts b/packages/sdk/src/engine/mutate.ts index 5968d2810..d7aaff516 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 { @@ -1003,24 +1001,6 @@ function handleRemoveGsapKeyframeByPercentage( 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 newScript = removeKeyframeFromScript(script, animationId, pct); - if (newScript === script) return EMPTY; - setGsapScript(parsed.document, newScript); - return gsapScriptChange(script, newScript); -} - function handleAddLabel(parsed: ParsedDocument, name: string, position: number): MutationResult { const script = getGsapScript(parsed.document); if (!script) return EMPTY; 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 } From 51d42f5bb8920fa5457d52d2a9b9a5b6fd64d430 Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Wed, 17 Jun 2026 12:15:29 -0700 Subject: [PATCH 03/17] =?UTF-8?q?fix(studio):=20gate=20ALL=20cutover=20per?= =?UTF-8?q?sist=20paths=20on=20the=20flag=20=E2=80=94=20true=20dark=20laun?= =?UTF-8?q?ch=20(review=20#1469=20finding=20#6)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Only sdkCutoverPersist (style/text/attr) checked STUDIO_SDK_CUTOVER_ENABLED. sdkTimingPersist, dispatchGsapOpAndPersist (every GSAP op) and sdkDeletePersist guarded only on `!sdkSession` — and useSdkSession opens a session by default for shadow/selection, so timing/GSAP/keyframe/delete cutover was ALWAYS live regardless of the flag. Flipping the flag OFF could not disable it, so the data-loss bugs in those paths (single-prop wipe, wrong-keyframe match, tween collapse, arc strip) ship LIVE on merge instead of being dark-launched. Added the flag guard at all three chokepoints → flag OFF returns false → callers fall back to the legacy server path. Makes the stack genuinely dark-launchable: merge is now a no-op in prod, and the remaining cutover correctness bugs become flip-prerequisites rather than merge-blockers. Co-Authored-By: Claude Opus 4.8 (1M context) --- packages/studio/src/utils/sdkCutover.ts | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/packages/studio/src/utils/sdkCutover.ts b/packages/studio/src/utils/sdkCutover.ts index 0c5a55106..e6f38dac8 100644 --- a/packages/studio/src/utils/sdkCutover.ts +++ b/packages/studio/src/utils/sdkCutover.ts @@ -171,6 +171,10 @@ 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 { @@ -229,6 +233,9 @@ 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 { @@ -330,6 +337,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 { From 06a47e3ced0ba0885d2122705ff338d9515ad550 Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Wed, 17 Jun 2026 12:43:57 -0700 Subject: [PATCH 04/17] fix(core,sdk): correct 8 GSAP write-path review findings (#1539) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Eight correctness bugs from the SDK-cutover review. Several were cases where BOTH writers were identically wrong, so the recast-vs-acorn parity suite stayed green; the new tests assert the real-world-correct result, not agreement. - #2 findKfPropByPct: match the CLOSEST keyframe within tolerance, not the first within 2% — removing/updating 50% on 0/49/50/100 no longer hits 49%. - #3 handleSetTiming: shift each tween by the start DELTA and scale duration by the clip-duration RATIO per-tween, instead of writing absolute newStart/ newDuration onto every tween (which collapsed staggers and blew durations). - #4 enableArcPath: insert motionPath via appendRight at the object start so the insertion can't collide with the x/y remove-range end (which made MagicString discard the append and emit '{}'). - #5 splitAnimationsInScript: compute the inherited baseline in a forward pre-pass so the split-spanning midpoint sees earlier tweens (the reverse write loop is kept for stable count-suffixed ids). - #9 unrollDynamicAnimations: preserve non-target loop-body statements (e.g. tl.set initial-state) per iteration instead of overwriting the whole loop. - #10 buildMotionPathObjectCode (both writers): emit the cubic form when segment curviness varies so per-segment curviness survives, not just segments[0]. - #11 readLastWaypointXY: handle UnaryExpression so negative destination coords are recovered when disabling an arc path. - #15 no-bang: removed every `!` non-null assertion in the touched files, replaced with guards/fallbacks. Tests: gsapWriter.reviewFixes.test.ts (#2/#4/#5/#9/#10/#11) and mutate.gsap.test.ts setTiming GSAP-sync block (#3). All fail on the base and pass after the fix; tsc + full core/sdk suites + parity stay green. Co-Authored-By: Claude Opus 4.8 (1M context) --- packages/core/src/parsers/gsapParser.ts | 9 +- packages/core/src/parsers/gsapSerialize.ts | 36 +- .../parsers/gsapWriter.reviewFixes.test.ts | 181 ++++++++++ packages/core/src/parsers/gsapWriterAcorn.ts | 316 +++++++++++++++--- packages/sdk/src/engine/mutate.gsap.test.ts | 45 +++ packages/sdk/src/engine/mutate.ts | 34 +- 6 files changed, 551 insertions(+), 70 deletions(-) create mode 100644 packages/core/src/parsers/gsapWriter.reviewFixes.test.ts 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..aba0a1ae2 --- /dev/null +++ b/packages/core/src/parsers/gsapWriter.reviewFixes.test.ts @@ -0,0 +1,181 @@ +// 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, +} 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"'); + }); +}); + +// ── #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"); + }); +}); diff --git a/packages/core/src/parsers/gsapWriterAcorn.ts b/packages/core/src/parsers/gsapWriterAcorn.ts index 0a996b686..385b2aa99 100644 --- a/packages/core/src/parsers/gsapWriterAcorn.ts +++ b/packages/core/src/parsers/gsapWriterAcorn.ts @@ -272,8 +272,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; } @@ -693,16 +694,26 @@ function autoEndpointOverwrites( } function findKfPropByPct(kfNode: any, percentage: number): { prop: any; 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: any; 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( @@ -962,7 +973,8 @@ 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(); } @@ -1007,7 +1019,8 @@ 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 @@ -1226,7 +1239,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( @@ -1419,9 +1433,28 @@ function readLastWaypointXY(mpVal: any): { x: number | null; y: number | null } const elems: any[] = 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: any): 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 { @@ -1470,7 +1503,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: any) => { + 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 +1553,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 +1620,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,6 +1634,69 @@ 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 } }; +} + // fallow-ignore-next-line complexity export function splitAnimationsInScript( script: string, @@ -1611,12 +1722,23 @@ 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. for (let i = matching.length - 1; i >= 0; i--) { - const anim = matching[i]!; + const anim = matching[i]; + if (!anim) continue; const pos = typeof anim.position === "number" ? anim.position : 0; const dur = anim.duration ?? 0; const animEnd = pos + dur; @@ -1626,30 +1748,15 @@ export function splitAnimationsInScript( 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; - } - } } + // Inherited-state accumulation for kf tweens is handled in the forward + // pre-pass (computeForwardBaselines). continue; } if (animEnd <= opts.splitTime) { - for (const [k, v] of Object.entries(anim.properties)) { - inheritedProps[k] = v; - } + // Wholly before the split — kept on the original element; its contribution + // to the inherited baseline is computed in the forward pre-pass. continue; } @@ -1658,9 +1765,10 @@ export function splitAnimationsInScript( continue; } - // Spans the split — linear interpolation to compute mid-values. + // Spans the split — linear interpolation to compute mid-values, using the + // FORWARD baseline (props from earlier tweens), not a reverse accumulator. const progress = dur > 0 ? (opts.splitTime - pos) / dur : 0; - const fromSource = anim.fromProperties ?? inheritedProps; + const fromSource = anim.fromProperties ?? baselineBefore[i] ?? {}; const midProps: Record = {}; for (const [k, v] of Object.entries(anim.properties)) { if (typeof v !== "number") { @@ -1689,14 +1797,15 @@ export function splitAnimationsInScript( extras: anim.extras, }); result = addResult.script; - - for (const [k, v] of Object.entries(midProps)) { - inheritedProps[k] = v; - } } - 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 }; @@ -1722,16 +1831,49 @@ 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: any[]): any | 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: any): any[] | null { + let body: any; + 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: any) => s?.type === "ExpressionStatement"); +} + +/** The loop's index identifier name (`for (let i …)`), used for per-iteration substitution. */ +function loopIndexVarName(loopNode: any): 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 whole-word + * occurrences of the loop index variable with the literal index. Keeps non-target + * statements (e.g. `tl.set(items[i], …)`) alive instead of discarding them. + */ +function substituteLoopIndex(stmtSource: string, indexVar: string | null, idx: number): string { + if (!indexVar) return stmtSource; + const re = new RegExp(`\\b${indexVar.replace(/[.*+?^${}()|[\]\\]/g, "\\$&")}\\b`, "g"); + return stmtSource.replace(re, String(idx)); +} + function buildUnrollReplacement( timelineVar: string, animation: GsapAnimation, @@ -1759,10 +1901,61 @@ 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});`; +} + +/** + * 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. Without + * this, a blanket overwrite of the loop body discards sibling statements such as + * an initial-state `tl.set(...)`. + */ +function buildLoopUnrollPreserving( + script: string, + timelineVar: string, + animation: GsapAnimation, + elements: UnrollElement[], + loopNode: any, + targetStmt: any, +): string | null { + const stmts = loopBodyStatements(loopNode); + if (!stmts || !stmts.includes(targetStmt)) return null; + const indexVar = loopIndexVarName(loopNode); + const lines: string[] = []; + for (let idx = 0; idx < elements.length; idx++) { + const el = elements[idx]; + if (!el) continue; + for (const stmt of stmts) { + if (stmt === targetStmt) { + lines.push(buildUnrollCallForElement(timelineVar, animation, el)); + } else { + const src = script.slice(stmt.start as number, stmt.end as number); + lines.push(substituteLoopIndex(src, indexVar, idx)); + } + } + } + return lines.join("\n "); +} + /** * 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, @@ -1774,14 +1967,29 @@ export function unrollDynamicAnimations( 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; + // 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/sdk/src/engine/mutate.gsap.test.ts b/packages/sdk/src/engine/mutate.gsap.test.ts index b1ea7e58f..23bd039a9 100644 --- a/packages/sdk/src/engine/mutate.gsap.test.ts +++ b/packages/sdk/src/engine/mutate.gsap.test.ts @@ -630,6 +630,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", () => { diff --git a/packages/sdk/src/engine/mutate.ts b/packages/sdk/src/engine/mutate.ts index d7aaff516..6809b1ae3 100644 --- a/packages/sdk/src/engine/mutate.ts +++ b/packages/sdk/src/engine/mutate.ts @@ -444,12 +444,30 @@ function handleSetTiming( // 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) { + 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; + 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); } @@ -911,8 +929,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 @@ -993,8 +1012,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 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); From ba17b937a9c1a921333758d08d5df3fd552da876 Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Wed, 17 Jun 2026 12:44:31 -0700 Subject: [PATCH 05/17] =?UTF-8?q?fix(studio):=20SDK=20cutover=20review=20f?= =?UTF-8?q?ixes=20=E2=80=94=20merge=20tween=20props,=20stabilize=20debounc?= =?UTF-8?q?e,=20serialize=20gsap=20writes,=20on-disk=20undo=20baseline,=20?= =?UTF-8?q?self-write=20identity?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses 5 SDK-cutover review findings (studio-only): - #1 useGsapPropertyDebounce: editing one GSAP tween property no longer drops the tween's other animated props. setGsapTween REPLACES the property set, so merge the single edit into the tween's CURRENT properties (read from the SDK doc) before dispatching, mirroring the legacy server merge. - #7 useGsapPropertyDebounce: stabilize the flush callback by reading sdk deps from a ref instead of an unmemoized literal, so a parent re-render mid-edit no longer tears down + flushes the debounce (one commit/undo entry per render). - #8 sdkCutover/useGsapScriptCommits: route SDK gsap-write persists through the same per-file keyed serializer the legacy commitMutation uses, so concurrent same-file read-modify-writes can't interleave and lose an edit. - #12 sdkCutover/useTimelineEditing: capture the exact on-disk bytes as the undo 'before' for timing/GSAP persists (matching the style/delete paths) instead of a normalized SDK serialize() re-emit that reformatted the whole file on undo. - #14 useSdkSession/sdkSelfWriteRegistry: discriminate a cutover echo from an undo write by CONTENT identity (registered self-write hash), not just the 2 s timestamp window — an undo write always reloads the SDK session. Tests: useGsapPropertyDebounce(.test), useGsapPropertyDebounceFlush.test, sdkSelfWriteRegistry.test, and new sdkCutover.test cases; each reproduces the review scenario and asserts the corrected behavior (verified red before fix). Co-Authored-By: Claude Opus 4.8 (1M context) --- .../src/hooks/sdkSelfWriteRegistry.test.ts | 67 ++++++++ .../studio/src/hooks/sdkSelfWriteRegistry.ts | 77 +++++++++ .../src/hooks/useGsapPropertyDebounce.test.ts | 70 +++++++++ .../src/hooks/useGsapPropertyDebounce.ts | 102 ++++++++++-- .../useGsapPropertyDebounceFlush.test.ts | 85 ++++++++++ .../studio/src/hooks/useGsapScriptCommits.ts | 26 ++++ packages/studio/src/hooks/useSdkSession.ts | 70 +++++++-- .../studio/src/hooks/useTimelineEditing.ts | 6 + packages/studio/src/utils/sdkCutover.test.ts | 147 ++++++++++++++++++ packages/studio/src/utils/sdkCutover.ts | 93 ++++++++--- 10 files changed, 695 insertions(+), 48 deletions(-) create mode 100644 packages/studio/src/hooks/sdkSelfWriteRegistry.test.ts create mode 100644 packages/studio/src/hooks/sdkSelfWriteRegistry.ts create mode 100644 packages/studio/src/hooks/useGsapPropertyDebounce.test.ts create mode 100644 packages/studio/src/hooks/useGsapPropertyDebounceFlush.test.ts 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/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 ffe5c7a45..688d4127a 100644 --- a/packages/studio/src/hooks/useTimelineEditing.ts +++ b/packages/studio/src/hooks/useTimelineEditing.ts @@ -188,6 +188,9 @@ 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 }, ).then((handled) => { @@ -297,6 +300,9 @@ 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 }, ).then((handled) => { 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 e6f38dac8..fcd95f12d 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", @@ -178,16 +220,15 @@ export async function sdkTimingPersist( if (!sdkSession || !sdkSession.getElement(hfId)) return false; if (wrongCompositionFile(deps, targetPath)) return false; try { - // `before` is the SDK's serialized state, which is the true pre-edit - // content only while every edit routes through this session. During the - // dark-launch transition (server still writes some paths) the in-memory - // SDK doc can drift from disk, so this `before` may not match the file's - // actual prior bytes. Acceptable for v1; revisit once cutover is always-on. - 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) { @@ -238,18 +279,30 @@ async function dispatchGsapOpAndPersist( 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( From 9a32c3064e9f743340fc2d09ef789202734863d1 Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Wed, 17 Jun 2026 12:55:32 -0700 Subject: [PATCH 06/17] refactor(core): extract split/collapse helpers to satisfy no-fallow-ignore rule MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The #5 (split) and #15 (no-bang guards) fixes pushed splitAnimationsInScript and removeAllKeyframesFromScript over fallow's complexity threshold, and a fallow-ignore had been added to splitAnimationsInScript. Per the hard rule (never ignore — fix), extracted buildSpanningSplit + applyTweenSplit (split) and buildCollapsedFlatVars (collapse), and removed the ignore. Both functions now under threshold; fallow new-only gate reports 0 new findings. Behavior unchanged — core 1811 green. Co-Authored-By: Claude Opus 4.8 (1M context) --- packages/core/src/parsers/gsapWriterAcorn.ts | 171 +++++++++++-------- 1 file changed, 101 insertions(+), 70 deletions(-) diff --git a/packages/core/src/parsers/gsapWriterAcorn.ts b/packages/core/src/parsers/gsapWriterAcorn.ts index 385b2aa99..4067b4f3e 100644 --- a/packages/core/src/parsers/gsapWriterAcorn.ts +++ b/packages/core/src/parsers/gsapWriterAcorn.ts @@ -1022,22 +1022,32 @@ export function removeAllKeyframesFromScript(script: string, animationId: string 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. */ @@ -1697,7 +1707,84 @@ function computeForwardBaselines( return { before, final: { ...acc } }; } -// fallow-ignore-next-line complexity +// 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, @@ -1736,67 +1823,11 @@ export function splitAnimationsInScript( // 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]; if (!anim) continue; - 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)`); - } - // Inherited-state accumulation for kf tweens is handled in the forward - // pre-pass (computeForwardBaselines). - continue; - } - - if (animEnd <= opts.splitTime) { - // Wholly before the split — kept on the original element; its contribution - // to the inherited baseline is computed in the forward pre-pass. - continue; - } - - if (pos >= opts.splitTime) { - result = updateAnimationSelectorInScript(result, anim.id, newSelector); - continue; - } - - // Spans the split — linear interpolation to compute mid-values, using the - // FORWARD baseline (props from earlier tweens), not a reverse accumulator. - const progress = dur > 0 ? (opts.splitTime - pos) / dur : 0; - const fromSource = anim.fromProperties ?? baselineBefore[i] ?? {}; - 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; + result = applyTweenSplit(result, anim, baselineBefore[i] ?? {}, ctx, skippedSelectors); } if (Object.keys(finalInheritedProps).length > 0) { From f9f534715210730cd0e4972cfa7432229220fd87 Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Wed, 17 Jun 2026 13:05:54 -0700 Subject: [PATCH 07/17] test(studio): pin dark-launch flag-gate contract (review #1539, Rames/Via) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit flag OFF ⇒ sdkTimingPersist / sdkGsapTweenPersist (GSAP-op chokepoint) / sdkDeletePersist all return false even with a valid session → legacy fallback. The prod flag-flip rests on this contract; sdkCutover.test.ts only mocks the flag TRUE, so a future gate refactor could silently re-enable cutover on flag-off without failing CI. This sibling file mocks it FALSE and locks the three guards. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../studio/src/utils/sdkCutover.gate.test.ts | 60 +++++++++++++++++++ 1 file changed, 60 insertions(+) create mode 100644 packages/studio/src/utils/sdkCutover.gate.test.ts 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); + }); +}); From e4fdb47d283852c1f8b32e0ebca5a6abcfd574ce Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Wed, 17 Jun 2026 13:07:26 -0700 Subject: [PATCH 08/17] fix(studio): leading flag-gate on sdkGsapTweenPersist (review #1539 nit, Via) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The add-op getElement existence check ran before the inner gate, so flag-off did an SDK touch before falling back. Lead with the flag guard to match the other three chokepoints — flag-off is now a clean no-op at every entry point. Co-Authored-By: Claude Opus 4.8 (1M context) --- packages/studio/src/utils/sdkCutover.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/studio/src/utils/sdkCutover.ts b/packages/studio/src/utils/sdkCutover.ts index fcd95f12d..09668be60 100644 --- a/packages/studio/src/utils/sdkCutover.ts +++ b/packages/studio/src/utils/sdkCutover.ts @@ -249,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 From 3f1bd63c0c1c1a167a341a5c38716fb279e6650d Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Wed, 17 Jun 2026 13:18:37 -0700 Subject: [PATCH 09/17] =?UTF-8?q?fix(core):=20unroll-preservation=20regres?= =?UTF-8?q?sions=20=E2=80=94=20non-for=20loops=20+=20AST=20index=20substit?= =?UTF-8?q?ution=20(review=20R2)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The #9 unroll-preservation fix had two confirmed regressions: - Non-for loops (forEach/for-of/for-in/while): loopIndexVarName returns null, so substitution no-op'd and preserved siblings kept a now-undefined loop variable (e.g. `item`) → ReferenceError at render. Now returns null for those forms → caller falls back to the blanket loop overwrite (drops siblings, valid code). The #9 fixture only used `for(let i…)` so it never caught this. - substituteLoopIndex did a \bvar\b regex over raw source including string literals, corrupting selectors like ".row-i" → ".row-0". Now AST-based: substitutes only real Identifier uses, skipping string literals and non-computed member/key positions (extracted isIndexBindingPosition helper to stay under the fallow complexity threshold — no ignore added). Two regression tests added (forEach no-dangling-var; for-loop string-literal intact). Co-Authored-By: Claude Opus 4.8 (1M context) --- .../parsers/gsapWriter.reviewFixes.test.ts | 51 +++++++++++++++++++ packages/core/src/parsers/gsapWriterAcorn.ts | 49 ++++++++++++++---- 2 files changed, 91 insertions(+), 9 deletions(-) diff --git a/packages/core/src/parsers/gsapWriter.reviewFixes.test.ts b/packages/core/src/parsers/gsapWriter.reviewFixes.test.ts index aba0a1ae2..ef60dc9a7 100644 --- a/packages/core/src/parsers/gsapWriter.reviewFixes.test.ts +++ b/packages/core/src/parsers/gsapWriter.reviewFixes.test.ts @@ -131,6 +131,57 @@ for (let i = 0; i < 2; i++) { }); }); +// ── R2 #1 — non-`for` loops must not leave preserved siblings with an unbound index var ── + +describe("R2 — unroll on a forEach does not emit an unbound loop variable", () => { + 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); +});`; + + it("falls back to the blanket overwrite (valid code, no dangling `item`)", () => { + const parsed = parseGsapScriptAcornForWrite(FOREACH); + const targetId = parsed?.located.find((l) => l.animation.method === "to")?.id ?? ""; + const out = unrollDynamicAnimations(FOREACH, targetId, [ + { selector: "#a", keyframes: [{ percentage: 100, properties: { opacity: 1 } }] }, + { selector: "#b", keyframes: [{ percentage: 100, properties: { opacity: 1 } }] }, + ]); + // The forEach (and its `item` param) is gone — no preserved sibling can + // reference a now-undefined `item` (the bug emitted `tl.set(item, …)` with + // `item` unbound → ReferenceError at render). + expect(out).not.toContain("item"); + expect(out).not.toContain("forEach"); + expect(out).toContain('tl.to("#a"'); + expect(out).toContain('tl.to("#b"'); + }); +}); + +// ── 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", () => { diff --git a/packages/core/src/parsers/gsapWriterAcorn.ts b/packages/core/src/parsers/gsapWriterAcorn.ts index 4067b4f3e..a8ec5c582 100644 --- a/packages/core/src/parsers/gsapWriterAcorn.ts +++ b/packages/core/src/parsers/gsapWriterAcorn.ts @@ -1895,14 +1895,39 @@ function loopIndexVarName(loopNode: any): string | null { } /** - * Rewrite one body statement's source for iteration `idx`: replace whole-word - * occurrences of the loop index variable with the literal index. Keeps non-target - * statements (e.g. `tl.set(items[i], …)`) alive instead of discarding them. + * 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. */ -function substituteLoopIndex(stmtSource: string, indexVar: string | null, idx: number): string { - if (!indexVar) return stmtSource; - const re = new RegExp(`\\b${indexVar.replace(/[.*+?^${}()|[\]\\]/g, "\\$&")}\\b`, "g"); - return stmtSource.replace(re, String(idx)); +// 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: any, parent: any): 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: any, 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: any, _state: unknown, ancestors: any[]) { + 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( @@ -1966,6 +1991,13 @@ function buildLoopUnrollPreserving( const stmts = loopBodyStatements(loopNode); if (!stmts || !stmts.includes(targetStmt)) return null; const indexVar = loopIndexVarName(loopNode); + // Only preserve siblings when we have a bound numeric index to substitute + // (`for (let i …)`). For forEach/for-of/for-in/while the iteration variable + // isn't a numeric index we can splice, so substituting would leave preserved + // siblings referencing a now-undefined loop variable (ReferenceError at + // render). Return null → caller falls back to the blanket loop overwrite, + // which drops the siblings but emits valid code. + if (!indexVar) return null; const lines: string[] = []; for (let idx = 0; idx < elements.length; idx++) { const el = elements[idx]; @@ -1974,8 +2006,7 @@ function buildLoopUnrollPreserving( if (stmt === targetStmt) { lines.push(buildUnrollCallForElement(timelineVar, animation, el)); } else { - const src = script.slice(stmt.start as number, stmt.end as number); - lines.push(substituteLoopIndex(src, indexVar, idx)); + lines.push(substituteLoopIndex(stmt, indexVar, idx, script)); } } } From 59029684a3931e4c53c95b9eb130a7367e37db58 Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Wed, 17 Jun 2026 14:39:30 -0700 Subject: [PATCH 10/17] fix(sdk,core): unrollDynamicAnimations rejects empty element list (R1 #1501b) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An empty `elements` array has no unrolled form — the writer would overwrite the loop/statement with zero tween calls, silently deleting the animation. - gsapWriterAcorn: unrollDynamicAnimations returns the script verbatim on an empty list (no-op instead of a destructive overwrite). - validateOp: reject unrollDynamicAnimations with empty elements as E_INVALID_ARGS so callers get a clean error rather than silent corruption. - Tests: writer no-op on []; validateOp E_INVALID_ARGS on []. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../src/parsers/gsapWriter.reviewFixes.test.ts | 8 ++++++++ packages/core/src/parsers/gsapWriterAcorn.ts | 3 +++ packages/sdk/src/engine/mutate.test.ts | 14 ++++++++++++++ packages/sdk/src/engine/mutate.ts | 15 ++++++++++++++- 4 files changed, 39 insertions(+), 1 deletion(-) diff --git a/packages/core/src/parsers/gsapWriter.reviewFixes.test.ts b/packages/core/src/parsers/gsapWriter.reviewFixes.test.ts index ef60dc9a7..7a465d1a2 100644 --- a/packages/core/src/parsers/gsapWriter.reviewFixes.test.ts +++ b/packages/core/src/parsers/gsapWriter.reviewFixes.test.ts @@ -129,6 +129,14 @@ for (let i = 0; i < 2; i++) { 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); + }); }); // ── R2 #1 — non-`for` loops must not leave preserved siblings with an unbound index var ── diff --git a/packages/core/src/parsers/gsapWriterAcorn.ts b/packages/core/src/parsers/gsapWriterAcorn.ts index a8ec5c582..474833f76 100644 --- a/packages/core/src/parsers/gsapWriterAcorn.ts +++ b/packages/core/src/parsers/gsapWriterAcorn.ts @@ -2024,6 +2024,9 @@ export function unrollDynamicAnimations( 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); diff --git a/packages/sdk/src/engine/mutate.test.ts b/packages/sdk/src/engine/mutate.test.ts index 2ef850f4d..0b002c3a3 100644 --- a/packages/sdk/src/engine/mutate.test.ts +++ b/packages/sdk/src/engine/mutate.test.ts @@ -492,6 +492,20 @@ 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: "tw-1", + elements: [], + }); + 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 6809b1ae3..d5b9e3e6e 100644 --- a/packages/sdk/src/engine/mutate.ts +++ b/packages/sdk/src/engine/mutate.ts @@ -1114,7 +1114,6 @@ export function validateOp(parsed: ParsedDocument, op: EditOp): CanResult { case "setArcPath": case "updateArcSegment": case "removeArcPath": - case "unrollDynamicAnimations": case "deleteAllForSelector": case "removeLabel": if (getGsapScript(parsed.document) === null) @@ -1124,6 +1123,20 @@ export function validateOp(parsed: ParsedDocument, op: EditOp): CanResult { "This composition does not use GSAP animations.", ); return CAN_OK; + case "unrollDynamicAnimations": + 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.", + ); + if (op.elements.length === 0) + return canErr( + "E_INVALID_ARGS", + "unrollDynamicAnimations requires at least one element.", + "An empty element list would delete the animation; pass the resolved element list.", + ); + return CAN_OK; default: return canErr("E_UNKNOWN_OP", `Unknown op type: "${(op as EditOp).type}".`); } From 854272cba482117703efdee68bfb6e544b4e8bf1 Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Wed, 17 Jun 2026 14:55:41 -0700 Subject: [PATCH 11/17] perf(sdk): cache draft element in applyDraft, drop HTMLElement casts (R1 #1490a) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit applyDraft runs at 60fps during a drag but re-ran doc.querySelector on every call — the _draftEl/_draftId fields were only consumed by commit/cancel, never to skip the query. Reuse the tracked element when the id matches and the node is still connected; re-query only on id change or detach (iframe reload). Retypes _draftEl to HTMLElement | null (only ever set from querySelector), which removes the `as HTMLElement` casts in commitPreview / _clearDraft. Test asserts a repeated same-id drag queries once. Co-Authored-By: Claude Opus 4.8 (1M context) --- packages/sdk/src/adapters/iframe.test.ts | 21 +++++++++++++++++++++ packages/sdk/src/adapters/iframe.ts | 24 ++++++++++++++---------- 2 files changed, 35 insertions(+), 10 deletions(-) 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; From 576cd3718f1889506c3b3ba2a321fd21d7b55aef Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Wed, 17 Jun 2026 15:13:34 -0700 Subject: [PATCH 12/17] =?UTF-8?q?fix(sdk,core):=20round-3=20correctness=20?= =?UTF-8?q?=E2=80=94=20unroll=20AST=20safety,=20single-dispatch=20undo,=20?= =?UTF-8?q?empty-arg=20guards,=20persist=20decouple?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the highest-severity round-3 review findings: - gsapWriterAcorn unroll (R3 #1/#2/#9): the round-2 AST-substitution fix emitted invalid GSAP for object shorthand `{ i }` (→ `{ 0 }`) and shadowed inner bindings (→ `for(let i=0;0<3;0++)`), and silently dropped sibling statements on non-`for` loops (forEach/for-of). The unroll now REFUSES (no-ops, leaving the dynamic loop intact) whenever siblings can't be safely reproduced — a non-`for` loop, an unmodeled statement, or an unsafe index use — instead of dropping or corrupting. Plain `for` loops with safe siblings still unroll. - session single-dispatch undo (R3 #5/#11): _dispatch now reverses the inverse patch list (parity with batch()). A single op emitting order-dependent inverse patches — a nested parent+child removeElement, an aliased multi-target — undid forward and dropped the child subtree / landed on an intermediate value. - materializeKeyframes empty-array (R3 #10): the unguarded twin of the just-fixed unrollDynamicAnimations. Writer no-ops on an empty keyframe list; validateOp rejects it as E_INVALID_ARGS (shared gsapScriptMissing helper). - history:false persist decouple (R3 #4): persist (auto-save) no longer lives inside the history-enable block, so opting out of SDK undo no longer silently disables all disk writes (data-loss trap for #1496's flag consumers). Tests: unroll refuse cases (shorthand/shadow/forEach) + safe-for-loop regression; nested removeElement undo; materializeKeyframes writer no-op + validateOp reject; history:false-still-persists. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../parsers/gsapWriter.reviewFixes.test.ts | 79 +++++++--- packages/core/src/parsers/gsapWriterAcorn.ts | 136 ++++++++++++++---- packages/sdk/src/engine/mutate.gsap.test.ts | 20 +++ packages/sdk/src/engine/mutate.test.ts | 14 ++ packages/sdk/src/engine/mutate.ts | 54 ++++--- packages/sdk/src/session.test.ts | 20 +++ packages/sdk/src/session.ts | 25 +++- packages/sdk/src/smoke.test.ts | 14 ++ 8 files changed, 293 insertions(+), 69 deletions(-) diff --git a/packages/core/src/parsers/gsapWriter.reviewFixes.test.ts b/packages/core/src/parsers/gsapWriter.reviewFixes.test.ts index 7a465d1a2..216536633 100644 --- a/packages/core/src/parsers/gsapWriter.reviewFixes.test.ts +++ b/packages/core/src/parsers/gsapWriter.reviewFixes.test.ts @@ -139,29 +139,74 @@ for (let i = 0; i < 2; i++) { }); }); -// ── R2 #1 — non-`for` loops must not leave preserved siblings with an unbound index var ── +// ── R3 — unsafe sibling reproduction must refuse (no-op), never corrupt/drop ── -describe("R2 — unroll on a forEach does not emit an unbound loop variable", () => { - const FOREACH = `var tl = gsap.timeline({ paused: true }); +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); + }); - it("falls back to the blanket overwrite (valid code, no dangling `item`)", () => { - const parsed = parseGsapScriptAcornForWrite(FOREACH); - const targetId = parsed?.located.find((l) => l.animation.method === "to")?.id ?? ""; - const out = unrollDynamicAnimations(FOREACH, targetId, [ - { selector: "#a", keyframes: [{ percentage: 100, properties: { opacity: 1 } }] }, - { selector: "#b", keyframes: [{ percentage: 100, properties: { opacity: 1 } }] }, - ]); - // The forEach (and its `item` param) is gone — no preserved sibling can - // reference a now-undefined `item` (the bug emitted `tl.set(item, …)` with - // `item` unbound → ReferenceError at render). - expect(out).not.toContain("item"); - expect(out).not.toContain("forEach"); - expect(out).toContain('tl.to("#a"'); - expect(out).toContain('tl.to("#b"'); + // 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); }); }); diff --git a/packages/core/src/parsers/gsapWriterAcorn.ts b/packages/core/src/parsers/gsapWriterAcorn.ts index 474833f76..88a0cedcb 100644 --- a/packages/core/src/parsers/gsapWriterAcorn.ts +++ b/packages/core/src/parsers/gsapWriterAcorn.ts @@ -1139,6 +1139,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); @@ -1972,13 +1975,111 @@ function buildUnrollCallForElement( 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: any): any[] { + 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: any, 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: any) => p?.name === indexVar); + } + return false; +} + +/** Object shorthand `{ i }` — substituting the value would yield invalid `{ 0 }`. */ +function isShorthandIndexUse(node: any, 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: any, indexVar: string): boolean { + let unsafe = false; + acornWalk.full(stmt, (node: any) => { + 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: any, + targetStmt: any, + stmts: any[], + 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: any[], + targetStmt: any, + 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. Without - * this, a blanket overwrite of the loop body discards sibling statements such as - * an initial-state `tl.set(...)`. + * 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, @@ -1987,30 +2088,14 @@ function buildLoopUnrollPreserving( elements: UnrollElement[], loopNode: any, targetStmt: any, -): string | null { +): string | null | typeof REFUSE_UNROLL { const stmts = loopBodyStatements(loopNode); if (!stmts || !stmts.includes(targetStmt)) return null; const indexVar = loopIndexVarName(loopNode); - // Only preserve siblings when we have a bound numeric index to substitute - // (`for (let i …)`). For forEach/for-of/for-in/while the iteration variable - // isn't a numeric index we can splice, so substituting would leave preserved - // siblings referencing a now-undefined loop variable (ReferenceError at - // render). Return null → caller falls back to the blanket loop overwrite, - // which drops the siblings but emits valid code. - if (!indexVar) return null; - const lines: string[] = []; - for (let idx = 0; idx < elements.length; idx++) { - const el = elements[idx]; - if (!el) continue; - for (const stmt of stmts) { - if (stmt === targetStmt) { - lines.push(buildUnrollCallForElement(timelineVar, animation, el)); - } else { - lines.push(substituteLoopIndex(stmt, indexVar, idx, script)); - } - } - } - return lines.join("\n "); + 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); } /** @@ -2046,6 +2131,9 @@ export function unrollDynamicAnimations( 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 = diff --git a/packages/sdk/src/engine/mutate.gsap.test.ts b/packages/sdk/src/engine/mutate.gsap.test.ts index 23bd039a9..0b5382dc3 100644 --- a/packages/sdk/src/engine/mutate.gsap.test.ts +++ b/packages/sdk/src/engine/mutate.gsap.test.ts @@ -442,6 +442,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", () => { diff --git a/packages/sdk/src/engine/mutate.test.ts b/packages/sdk/src/engine/mutate.test.ts index 0b002c3a3..c087fe8ab 100644 --- a/packages/sdk/src/engine/mutate.test.ts +++ b/packages/sdk/src/engine/mutate.test.ts @@ -506,6 +506,20 @@ describe("Phase 3b ops", () => { 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: "tw-1", + 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 d5b9e3e6e..e0d29a552 100644 --- a/packages/sdk/src/engine/mutate.ts +++ b/packages/sdk/src/engine/mutate.ts @@ -1047,6 +1047,17 @@ 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; +} + /** 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,7 +1119,6 @@ export function validateOp(parsed: ParsedDocument, op: EditOp): CanResult { case "removeGsapTween": case "removeAllKeyframes": case "convertToKeyframes": - case "materializeKeyframes": case "splitIntoPropertyGroups": case "splitAnimations": case "setArcPath": @@ -1116,27 +1126,29 @@ export function validateOp(parsed: ParsedDocument, op: EditOp): CanResult { case "removeArcPath": 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": - 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.", - ); - if (op.elements.length === 0) - return canErr( - "E_INVALID_ARGS", - "unrollDynamicAnimations requires at least one element.", - "An empty element list would delete the animation; pass the resolved element list.", - ); - return CAN_OK; + return ( + gsapScriptMissing(parsed) ?? + (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) ?? + (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/session.test.ts b/packages/sdk/src/session.test.ts index d5a3b543a..326d1dfcc 100644 --- a/packages/sdk/src/session.test.ts +++ b/packages/sdk/src/session.test.ts @@ -299,6 +299,26 @@ 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(); + }); +}); + // ─── 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[] = []; From d47f34e2a09727f450a68b3230ac4d5e0b0d9839 Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Wed, 17 Jun 2026 15:16:17 -0700 Subject: [PATCH 13/17] fix(core): stripGsapForId re-parses per removal so all tweens for a deleted element are stripped (R3 #3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Animation ids are count-based (positional), so removing one tween renumbers the survivors. stripGsapForId captured every matching id from a single up-front parse then removed against the mutating script — after the first removal the later ids were stale and silently no-op'd, leaving an orphaned tl.to() referencing the just-deleted element. Now re-parse after each removal and strip the first still-matching animation until none remain. Co-Authored-By: Claude Opus 4.8 (1M context) --- packages/core/src/parsers/htmlParser.test.ts | 23 ++++++++++++++++++++ packages/core/src/parsers/htmlParser.ts | 23 ++++++++++++++------ 2 files changed, 39 insertions(+), 7 deletions(-) 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 { From 0c06609c501dca426f9f3fbdb27ffe09458a55b1 Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Wed, 17 Jun 2026 15:31:47 -0700 Subject: [PATCH 14/17] =?UTF-8?q?fix(core):=20gsap=20writer=20=E2=80=94=20?= =?UTF-8?q?keyframe=20ease=20routing,=20convert=20preserves=20delay,=20add?= =?UTF-8?q?Label=20dedup=20(R3=20#7/#8/#12)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - #7: updateAnimationInScript routes an ease update on a keyframe tween to keyframes.easeEach (per-keyframe), not a top-level ease that GSAP ignores — the user's keyframe-easing edit was silently a no-op. - #8: convertToKeyframesFromScript now preserves every non-editable vars key (delay/callbacks/stagger/yoyo/…) verbatim via preservedVarsEntries instead of rebuilding from the GsapAnimation object, which had no `delay` field and dropped it — shifting the tween's start time. - #12: addLabelToScript moves an existing same-named label (overwrites its position) instead of appending a duplicate; duplicates made removeLabel over-remove (it deletes every match, including a pre-existing label). Tests: easeEach routing, delay preservation, addLabel move-not-duplicate + hand-authored-dup removal. Updated the old "no dedup contract" corpus test. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../parsers/gsapWriter.reviewFixes.test.ts | 32 +++++++ packages/core/src/parsers/gsapWriterAcorn.ts | 94 ++++++++++++++----- .../parsers/gsapWriterParity.corpus.test.ts | 16 ++-- 3 files changed, 109 insertions(+), 33 deletions(-) diff --git a/packages/core/src/parsers/gsapWriter.reviewFixes.test.ts b/packages/core/src/parsers/gsapWriter.reviewFixes.test.ts index 216536633..11ed3c460 100644 --- a/packages/core/src/parsers/gsapWriter.reviewFixes.test.ts +++ b/packages/core/src/parsers/gsapWriter.reviewFixes.test.ts @@ -15,6 +15,8 @@ import { updateArcSegmentInScript, splitAnimationsInScript, unrollDynamicAnimations, + updateAnimationInScript, + convertToKeyframesFromScript, } from "./gsapWriterAcorn.js"; import { parseGsapScriptAcornForWrite } from "./gsapParserAcorn.js"; @@ -283,3 +285,33 @@ tl.to("#h", { x: -120, y: -40, duration: 1 }, 0);`; 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 88a0cedcb..60006b7c3 100644 --- a/packages/core/src/parsers/gsapWriterAcorn.ts +++ b/packages/core/src/parsers/gsapWriterAcorn.ts @@ -90,6 +90,12 @@ function findPropertyNode(varsArgNode: any, key: string): any | undefined { return undefined; } +/** The `keyframes` property's ObjectExpression value, or null when not a keyframe tween. */ +function keyframesObjectNode(varsNode: any): any | null { + const kfProp = findPropertyNode(varsNode, "keyframes"); + return kfProp?.value?.type === "ObjectExpression" ? kfProp.value : null; +} + function findEnclosingExpressionStatement(ancestors: any[]): any | null { for (let i = ancestors.length - 2; i >= 0; i--) { if (ancestors[i]?.type === "ExpressionStatement") return ancestors[i]; @@ -315,7 +321,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)) { @@ -1055,18 +1066,18 @@ function buildKeyframesVarsCode( animation: GsapAnimation, fromProps: Record, toProps: Record, + varsNode: any, + 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(", ")} }`; } @@ -1096,7 +1107,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(); } @@ -1363,10 +1378,54 @@ export function splitIntoPropertyGroupsFromScript( // ── Label write ops ─────────────────────────────────────────────────────────── +/** True when `expr` is `tl.(…)` rooted at the timeline var. */ +function isTimelineMethodCall(expr: any, 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: any, 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): any[] { + const targets: any[] = []; + acornWalk.simple(parsed.ast, { + ExpressionStatement(node: any) { + 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; @@ -1380,24 +1439,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); 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", () => { From 3ec8f59c5e8ae36eadf8be6decb3b2480f0714f0 Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Wed, 17 Jun 2026 15:40:32 -0700 Subject: [PATCH 15/17] fix(sdk): handleSetTiming #domId + data-duration sync; validateOp resolves ids + arc/selector (R3 #6/#13, CF2 #15/#16) CF2 #15: handleSetTiming re-synced GSAP tweens only when the selector matched the element's hf-id. The common #domId-targeted tween (authored by the Studio panel) never matched, so moving/resizing a clip via the SDK timing path left its animations unsynced. Now match the tween selector against the DOM id too. CF2 #16: handleSetTiming read/wrote only data-end. Clips authored with data-duration (what the runtime prefers) got a fresh data-end beside a stale data-duration (no playback change) and oldDuration=null collapsed the GSAP duration-scale ratio to 1. Now read duration preferring data-duration, and write back to whichever attribute the clip uses (timingPath gains a "duration" field). R3 #13b: deleteAllForSelector compared selectors with strict === and missed the alternate quote style ([data-hf-id='x'] vs "x"); now quote-insensitive. R3 #6/#13a: validateOp now resolves the animationId for id-bearing GSAP ops (E_TARGET_NOT_FOUND instead of a misleading ok that no-ops at apply), and updateArcSegment validates the arc is enabled + the segment index is in range. Tests: #domId move sync, data-duration resize + scale, quote-insensitive delete, unresolved-id rejection, arc-segment preconditions. Updated the loose-can() test. Co-Authored-By: Claude Opus 4.8 (1M context) --- packages/sdk/src/engine/mutate.gsap.test.ts | 95 +++++++++++++++++- packages/sdk/src/engine/mutate.test.ts | 4 +- packages/sdk/src/engine/mutate.ts | 106 ++++++++++++++++++-- packages/sdk/src/engine/patches.ts | 2 +- 4 files changed, 192 insertions(+), 15 deletions(-) diff --git a/packages/sdk/src/engine/mutate.gsap.test.ts b/packages/sdk/src/engine/mutate.gsap.test.ts index 0b5382dc3..416214a19 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", () => { @@ -922,3 +930,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 c087fe8ab..03b8f209d 100644 --- a/packages/sdk/src/engine/mutate.test.ts +++ b/packages/sdk/src/engine/mutate.test.ts @@ -499,7 +499,7 @@ describe("Phase 3b ops", () => { ); const r = validateOp(parsed, { type: "unrollDynamicAnimations", - animationId: "tw-1", + animationId: "#x-to-0-position", elements: [], }); expect(r.ok).toBe(false); @@ -513,7 +513,7 @@ describe("Phase 3b ops", () => { ); const r = validateOp(parsed, { type: "materializeKeyframes", - animationId: "tw-1", + animationId: "#x-to-0-position", keyframes: [], }); expect(r.ok).toBe(false); diff --git a/packages/sdk/src/engine/mutate.ts b/packages/sdk/src/engine/mutate.ts index e0d29a552..3667752c7 100644 --- a/packages/sdk/src/engine/mutate.ts +++ b/packages/sdk/src/engine/mutate.ts @@ -395,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; @@ -413,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 @@ -440,10 +464,12 @@ 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; + // 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 @@ -458,7 +484,10 @@ function handleSetTiming( : 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 (startChanged || durChanged) { @@ -910,7 +939,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()) { @@ -1058,6 +1093,49 @@ function gsapScriptMissing(parsed: ParsedDocument): CanResult | null { : 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 { @@ -1120,16 +1198,23 @@ export function validateOp(parsed: ParsedDocument, op: EditOp): CanResult { case "removeAllKeyframes": case "convertToKeyframes": case "splitIntoPropertyGroups": - case "splitAnimations": case "setArcPath": - case "updateArcSegment": case "removeArcPath": + 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": return gsapScriptMissing(parsed) ?? CAN_OK; case "unrollDynamicAnimations": return ( gsapScriptMissing(parsed) ?? + gsapAnimationMissing(parsed, op.animationId) ?? (op.elements.length === 0 ? canErr( "E_INVALID_ARGS", @@ -1141,6 +1226,7 @@ export function validateOp(parsed: ParsedDocument, op: EditOp): CanResult { case "materializeKeyframes": return ( gsapScriptMissing(parsed) ?? + gsapAnimationMissing(parsed, op.animationId) ?? (op.keyframes.length === 0 ? canErr( "E_INVALID_ARGS", diff --git a/packages/sdk/src/engine/patches.ts b/packages/sdk/src/engine/patches.ts index a1776d928..02c0b7c47 100644 --- a/packages/sdk/src/engine/patches.ts +++ b/packages/sdk/src/engine/patches.ts @@ -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}`; } From 7017d89d6adaf11991d7a36a0078bb2e43b440fd Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Wed, 17 Jun 2026 15:43:56 -0700 Subject: [PATCH 16/17] refactor(core,sdk): name the acorn-node type alias; keyToPath round-trips timing.duration (R3 #14) - gsapWriterAcorn: replace the bare `: any` AST-node annotations with the named `type Node = any` alias, matching the established convention in gsapParserAcorn.ts / gsapInline.ts ("acorn ESTree nodes are structurally untyped"). Documents intent and is greppable; type-identical (zero runtime change). A full ESTree typing is a deliberate architecture decision the codebase has not taken and is out of scope here. - patches: keyToPath/timingPath now include the "duration" timing field added for the data-duration resize fix, so a timing.duration override round-trips on T3 replay instead of being dropped. Co-Authored-By: Claude Opus 4.8 (1M context) --- packages/core/src/parsers/gsapWriterAcorn.ts | 162 ++++++++++--------- packages/sdk/src/engine/patches.ts | 7 +- 2 files changed, 89 insertions(+), 80 deletions(-) diff --git a/packages/core/src/parsers/gsapWriterAcorn.ts b/packages/core/src/parsers/gsapWriterAcorn.ts index 60006b7c3..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]; } @@ -104,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 ( @@ -132,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) { @@ -162,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) { @@ -214,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, @@ -246,7 +250,7 @@ function preservedEntries( */ function reconcileEditableProps( ms: MagicString, - objNode: any, + objNode: Node, source: string, newProps: Record, nonEditableOverrides?: Record, @@ -266,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; @@ -539,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 ?? []) { @@ -552,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)}` : ""; @@ -566,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; @@ -576,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; @@ -618,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); @@ -630,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") @@ -653,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 ?? []) { @@ -678,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, @@ -687,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(); @@ -695,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; @@ -704,14 +708,14 @@ 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: any; idx: number } | null = null; + 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]; @@ -759,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, @@ -785,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, @@ -806,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 @@ -825,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; @@ -843,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; @@ -914,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) { @@ -946,7 +950,7 @@ function insertNewKeyframe( */ function collapseKeyframesToFlat( ms: MagicString, - varsNode: any, + varsNode: Node, source: string, remainingRecord: Record, ): void { @@ -990,7 +994,7 @@ export function removeKeyframeFromScript( 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(); } @@ -1010,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(); @@ -1066,7 +1070,7 @@ function buildKeyframesVarsCode( animation: GsapAnimation, fromProps: Record, toProps: Record, - varsNode: any, + varsNode: Node, source: string, ): string { const fromEntries = Object.entries(fromProps).map(([k, v]) => `${safeKey(k)}: ${valueToCode(v)}`); @@ -1187,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); } @@ -1379,7 +1383,7 @@ export function splitIntoPropertyGroupsFromScript( // ── Label write ops ─────────────────────────────────────────────────────────── /** True when `expr` is `tl.(…)` rooted at the timeline var. */ -function isTimelineMethodCall(expr: any, timelineVar: string, method: string): boolean { +function isTimelineMethodCall(expr: Node, timelineVar: string, method: string): boolean { return ( expr?.type === "CallExpression" && expr.callee?.type === "MemberExpression" && @@ -1389,7 +1393,7 @@ function isTimelineMethodCall(expr: any, timelineVar: string, method: string): b } /** True when `expr` is `tl.addLabel("", …)` rooted at the timeline var. */ -function isAddLabelCall(expr: any, timelineVar: string, name: string): boolean { +function isAddLabelCall(expr: Node, timelineVar: string, name: string): boolean { const firstArg = expr?.arguments?.[0]; return ( isTimelineMethodCall(expr, timelineVar, "addLabel") && @@ -1399,10 +1403,10 @@ function isAddLabelCall(expr: any, timelineVar: string, name: string): boolean { } /** Every `tl.addLabel("", …)` ExpressionStatement in the script. */ -function findLabelStatements(parsed: ParsedGsapAcornForWrite, name: string): any[] { - const targets: any[] = []; +function findLabelStatements(parsed: ParsedGsapAcornForWrite, name: string): Node[] { + const targets: Node[] = []; acornWalk.simple(parsed.ast, { - ExpressionStatement(node: any) { + ExpressionStatement(node: Node) { if (isAddLabelCall(node.expression, parsed.timelineVar, name)) targets.push(node); }, }); @@ -1457,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]) { @@ -1473,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]; @@ -1481,11 +1489,11 @@ 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 }; return { @@ -1500,7 +1508,7 @@ function readLastWaypointXY(mpVal: any): { x: number | null; y: number | null } * UnaryExpression branch, negative waypoint coords (parsed as a UnaryExpression * with no `.value`) would be lost when disabling an arc path. */ -function readNumericLiteralNode(v: any): number | null { +function readNumericLiteralNode(v: Node): number | null { if (LITERAL_NODE_TYPES.has(v?.type) && typeof v.value === "number") return v.value; if ( v?.type === "UnaryExpression" && @@ -1530,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)) { @@ -1566,7 +1574,7 @@ function enableArcPath( // 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: any) => { + const survivesRemoval = editable.some((p: Node) => { const k = propKeyName(p); return k !== "x" && k !== "y"; }); @@ -1889,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" || @@ -1899,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" && @@ -1908,7 +1916,7 @@ function isForEachStatement(node: any): boolean { } /** The nearest enclosing loop / forEach AST node (not just its byte range). */ -function findEnclosingLoopNode(ancestors: any[]): any | null { +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 node; @@ -1917,8 +1925,8 @@ function findEnclosingLoopNode(ancestors: any[]): any | null { } /** Statements making up a loop's body block, or null when not a simple block. */ -function loopBodyStatements(loopNode: any): any[] | null { - let body: any; +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]; @@ -1927,11 +1935,11 @@ function loopBodyStatements(loopNode: any): any[] | null { body = loopNode?.body; } if (body?.type !== "BlockStatement") return null; - return (body.body ?? []).filter((s: any) => s?.type === "ExpressionStatement"); + 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: any): string | null { +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; @@ -1949,7 +1957,7 @@ function loopIndexVarName(loopNode: any): string | null { // 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: any, parent: any): boolean { +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; @@ -1957,12 +1965,12 @@ function isIndexBindingPosition(node: any, parent: any): boolean { return false; } -function substituteLoopIndex(stmt: any, indexVar: string, idx: number, script: string): string { +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: any, _state: unknown, ancestors: any[]) { + 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]); @@ -2021,7 +2029,7 @@ function buildUnrollCallForElement( const REFUSE_UNROLL = Symbol("refuse-unroll"); /** Every statement in a loop's body block (unfiltered), or [] when not a block. */ -function loopBodyRawStatements(loopNode: any): any[] { +function loopBodyRawStatements(loopNode: Node): Node[] { const body = loopNode?.type === "ExpressionStatement" ? loopNode.expression?.arguments?.[0]?.body @@ -2030,20 +2038,20 @@ function loopBodyRawStatements(loopNode: any): any[] { } /** A node that re-binds `indexVar`: a re-declaration or a function param. */ -function rebindsIndex(node: any, indexVar: string): boolean { +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: any) => p?.name === indexVar); + return (node.params ?? []).some((p: Node) => p?.name === indexVar); } return false; } /** Object shorthand `{ i }` — substituting the value would yield invalid `{ 0 }`. */ -function isShorthandIndexUse(node: any, indexVar: string): boolean { +function isShorthandIndexUse(node: Node, indexVar: string): boolean { return ( (node.type === "Property" || node.type === "ObjectProperty") && node.shorthand === true && @@ -2058,9 +2066,9 @@ function isShorthandIndexUse(node: any, indexVar: string): boolean { * `{ 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: any, indexVar: string): boolean { +function hasUnsafeLoopIndexUse(stmt: Node, indexVar: string): boolean { let unsafe = false; - acornWalk.full(stmt, (node: any) => { + acornWalk.full(stmt, (node: Node) => { if (!unsafe && (isShorthandIndexUse(node, indexVar) || rebindsIndex(node, indexVar))) { unsafe = true; } @@ -2070,9 +2078,9 @@ function hasUnsafeLoopIndexUse(stmt: any, indexVar: string): boolean { /** How to handle the loop body's non-target siblings when unrolling. */ function unrollSiblingStrategy( - loopNode: any, - targetStmt: any, - stmts: any[], + loopNode: Node, + targetStmt: Node, + stmts: Node[], indexVar: string | null, ): "blanket" | "refuse" | "preserve" { const siblings = stmts.filter((s) => s !== targetStmt); @@ -2088,8 +2096,8 @@ function unrollSiblingStrategy( /** Emit the per-iteration unrolled lines (target → static tl.to, siblings → index-substituted). */ function emitUnrolledLines( - stmts: any[], - targetStmt: any, + stmts: Node[], + targetStmt: Node, elements: UnrollElement[], timelineVar: string, animation: GsapAnimation, @@ -2128,8 +2136,8 @@ function buildLoopUnrollPreserving( timelineVar: string, animation: GsapAnimation, elements: UnrollElement[], - loopNode: any, - targetStmt: any, + loopNode: Node, + targetStmt: Node, ): string | null | typeof REFUSE_UNROLL { const stmts = loopBodyStatements(loopNode); if (!stmts || !stmts.includes(targetStmt)) return null; diff --git a/packages/sdk/src/engine/patches.ts b/packages/sdk/src/engine/patches.ts index 02c0b7c47..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} @@ -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"); From f92bba7fed1871b25d01dcec9ef5ce696ff6ab61 Mon Sep 17 00:00:00 2001 From: Vance Ingalls Date: Wed, 17 Jun 2026 16:02:27 -0700 Subject: [PATCH 17/17] =?UTF-8?q?fix(sdk):=20cascadeRemoveAnimations=20re-?= =?UTF-8?q?parses=20per=20removal=20(R4=20=E2=80=94=20SDK=20twin=20of=20#3?= =?UTF-8?q?)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit cascadeRemoveAnimations captured every matching animation id from a single up-front parse, then removed against the mutating script — the SDK-side twin of the stripGsapForId bug (R3 #3). Animation ids are positional, so removing the first tween for an element renumbered the survivors and the stale later ids no-op'd, orphaning those tweens on the just-removed element. Now re-parse after each removal and strip the first still-matching animation until none remain. Also adds the reviewer's defense-in-depth test: an aliased multi-target setStyle (same id twice) undoes to the original, not the intermediate (exercises the single-dispatch inverse reversal from R3 #5/#11). Co-Authored-By: Claude Opus 4.8 (1M context) --- packages/sdk/src/engine/mutate.gsap.test.ts | 15 +++++++++++++++ packages/sdk/src/engine/mutate.ts | 19 ++++++++++++------- packages/sdk/src/session.test.ts | 15 +++++++++++++++ 3 files changed, 42 insertions(+), 7 deletions(-) diff --git a/packages/sdk/src/engine/mutate.gsap.test.ts b/packages/sdk/src/engine/mutate.gsap.test.ts index 416214a19..5d60d6b59 100644 --- a/packages/sdk/src/engine/mutate.gsap.test.ts +++ b/packages/sdk/src/engine/mutate.gsap.test.ts @@ -810,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 ──────────────────────────── diff --git a/packages/sdk/src/engine/mutate.ts b/packages/sdk/src/engine/mutate.ts index 3667752c7..6b5051d7a 100644 --- a/packages/sdk/src/engine/mutate.ts +++ b/packages/sdk/src/engine/mutate.ts @@ -684,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 ──────────────────────────────────────────────────── diff --git a/packages/sdk/src/session.test.ts b/packages/sdk/src/session.test.ts index 326d1dfcc..e6e1ccc32 100644 --- a/packages/sdk/src/session.test.ts +++ b/packages/sdk/src/session.test.ts @@ -317,6 +317,21 @@ describe("single-dispatch undo reverses the inverse patch list", () => { 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 ───────────────────────────