Skip to content

Commit 0c06609

Browse files
vanceingallsclaude
andcommitted
fix(core): gsap writer — keyframe ease routing, convert preserves delay, addLabel dedup (R3 #7/#8/#12)
- #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) <noreply@anthropic.com>
1 parent d47f34e commit 0c06609

3 files changed

Lines changed: 109 additions & 33 deletions

File tree

packages/core/src/parsers/gsapWriter.reviewFixes.test.ts

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,8 @@ import {
1515
updateArcSegmentInScript,
1616
splitAnimationsInScript,
1717
unrollDynamicAnimations,
18+
updateAnimationInScript,
19+
convertToKeyframesFromScript,
1820
} from "./gsapWriterAcorn.js";
1921
import { parseGsapScriptAcornForWrite } from "./gsapParserAcorn.js";
2022

@@ -283,3 +285,33 @@ tl.to("#h", { x: -120, y: -40, duration: 1 }, 0);`;
283285
expect(disabled).not.toContain("motionPath");
284286
});
285287
});
288+
289+
// ── #7 — updating ease on a keyframe tween routes to easeEach, not top-level ──
290+
291+
describe("#7 — ease update on a keyframe tween targets keyframes.easeEach", () => {
292+
const KF = `var tl = gsap.timeline({ paused: true });
293+
tl.to(".a", { keyframes: { "0%": { x: 0 }, "100%": { x: 100 } }, duration: 1, ease: "none" }, 0);`;
294+
295+
it("writes easeEach (per-keyframe), not a no-op top-level ease", () => {
296+
const id = parseGsapScriptAcornForWrite(KF)?.located[0]?.id ?? "";
297+
const out = updateAnimationInScript(KF, id, { ease: "power2.inOut" });
298+
expect(out).toContain('easeEach: "power2.inOut"');
299+
// The original top-level `ease: "none"` is untouched (no second top-level ease).
300+
expect((out.match(/ease: "power2.inOut"/g) ?? []).length).toBe(0);
301+
});
302+
});
303+
304+
// ── #8 — convertToKeyframes preserves builtin vars like `delay` ──
305+
306+
describe("#8 — convertToKeyframes keeps delay (was dropped, shifting start time)", () => {
307+
const DELAY = `var tl = gsap.timeline({ paused: true });
308+
tl.to(".a", { x: 100, duration: 1, delay: 0.3 }, 0);`;
309+
310+
it("preserves delay on the converted vars object", () => {
311+
const id = parseGsapScriptAcornForWrite(DELAY)?.located[0]?.id ?? "";
312+
const out = convertToKeyframesFromScript(DELAY, id);
313+
expect(out).toContain("keyframes:");
314+
expect(out).toContain("delay: 0.3"); // was lost → tween started 0.3s early
315+
expect(out).toContain("duration: 1");
316+
});
317+
});

packages/core/src/parsers/gsapWriterAcorn.ts

Lines changed: 68 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,12 @@ function findPropertyNode(varsArgNode: any, key: string): any | undefined {
9090
return undefined;
9191
}
9292

93+
/** The `keyframes` property's ObjectExpression value, or null when not a keyframe tween. */
94+
function keyframesObjectNode(varsNode: any): any | null {
95+
const kfProp = findPropertyNode(varsNode, "keyframes");
96+
return kfProp?.value?.type === "ObjectExpression" ? kfProp.value : null;
97+
}
98+
9399
function findEnclosingExpressionStatement(ancestors: any[]): any | null {
94100
for (let i = ancestors.length - 2; i >= 0; i--) {
95101
if (ancestors[i]?.type === "ExpressionStatement") return ancestors[i];
@@ -315,7 +321,12 @@ export function updateAnimationInScript(
315321
upsertProp(ms, call.varsArg, "duration", updates.duration);
316322
}
317323
if (updates.ease !== undefined) {
318-
upsertProp(ms, call.varsArg, "ease", updates.ease);
324+
// For a keyframe tween, easing lives at keyframes.easeEach (per-keyframe),
325+
// not a top-level ease. Writing top-level ease would leave the per-keyframe
326+
// easing unchanged — the user's edit would silently do nothing.
327+
const kfNode = keyframesObjectNode(call.varsArg);
328+
if (kfNode) upsertProp(ms, kfNode, "easeEach", updates.ease);
329+
else upsertProp(ms, call.varsArg, "ease", updates.ease);
319330
}
320331
if (updates.extras) {
321332
for (const [key, value] of Object.entries(updates.extras)) {
@@ -1055,18 +1066,18 @@ function buildKeyframesVarsCode(
10551066
animation: GsapAnimation,
10561067
fromProps: Record<string, number | string>,
10571068
toProps: Record<string, number | string>,
1069+
varsNode: any,
1070+
source: string,
10581071
): string {
10591072
const fromEntries = Object.entries(fromProps).map(([k, v]) => `${safeKey(k)}: ${valueToCode(v)}`);
10601073
const toEntries = Object.entries(toProps).map(([k, v]) => `${safeKey(k)}: ${valueToCode(v)}`);
10611074
const easeEntry = animation.ease ? `, easeEach: ${JSON.stringify(animation.ease)}` : "";
10621075
const kfCode = `{ "0%": { ${fromEntries.join(", ")} }, "100%": { ${toEntries.join(", ")} }${easeEntry} }`;
1063-
const parts: string[] = [`keyframes: ${kfCode}`];
1064-
if (animation.duration !== undefined) parts.push(`duration: ${valueToCode(animation.duration)}`);
1076+
// Preserve every non-editable key (duration/delay/callbacks/stagger/yoyo/…)
1077+
// verbatim from source — rebuilding from the animation object alone dropped
1078+
// `delay` (not a GsapAnimation field), shifting the tween's start time.
1079+
const parts: string[] = [`keyframes: ${kfCode}`, ...preservedVarsEntries(varsNode, source)];
10651080
if (animation.ease) parts.push(`ease: "none"`);
1066-
for (const [k, v] of Object.entries(animation.extras ?? {})) {
1067-
if (typeof v === "number" || typeof v === "string")
1068-
parts.push(`${safeKey(k)}: ${valueToCode(v)}`);
1069-
}
10701081
return `{ ${parts.join(", ")} }`;
10711082
}
10721083

@@ -1096,7 +1107,11 @@ export function convertToKeyframesFromScript(
10961107
if (call.method === "fromTo" && call.fromArg) {
10971108
ms.remove(call.fromArg.start, call.varsArg.start);
10981109
}
1099-
overwriteVarsArg(ms, call, buildKeyframesVarsCode(animation, fromProps, toProps));
1110+
overwriteVarsArg(
1111+
ms,
1112+
call,
1113+
buildKeyframesVarsCode(animation, fromProps, toProps, call.varsArg, script),
1114+
);
11001115

11011116
return ms.toString();
11021117
}
@@ -1363,10 +1378,54 @@ export function splitIntoPropertyGroupsFromScript(
13631378

13641379
// ── Label write ops ───────────────────────────────────────────────────────────
13651380

1381+
/** True when `expr` is `tl.<method>(…)` rooted at the timeline var. */
1382+
function isTimelineMethodCall(expr: any, timelineVar: string, method: string): boolean {
1383+
return (
1384+
expr?.type === "CallExpression" &&
1385+
expr.callee?.type === "MemberExpression" &&
1386+
isTimelineRooted(expr.callee.object, timelineVar) &&
1387+
expr.callee.property?.name === method
1388+
);
1389+
}
1390+
1391+
/** True when `expr` is `tl.addLabel("<name>", …)` rooted at the timeline var. */
1392+
function isAddLabelCall(expr: any, timelineVar: string, name: string): boolean {
1393+
const firstArg = expr?.arguments?.[0];
1394+
return (
1395+
isTimelineMethodCall(expr, timelineVar, "addLabel") &&
1396+
firstArg?.type === "Literal" &&
1397+
firstArg.value === name
1398+
);
1399+
}
1400+
1401+
/** Every `tl.addLabel("<name>", …)` ExpressionStatement in the script. */
1402+
function findLabelStatements(parsed: ParsedGsapAcornForWrite, name: string): any[] {
1403+
const targets: any[] = [];
1404+
acornWalk.simple(parsed.ast, {
1405+
ExpressionStatement(node: any) {
1406+
if (isAddLabelCall(node.expression, parsed.timelineVar, name)) targets.push(node);
1407+
},
1408+
});
1409+
return targets;
1410+
}
1411+
13661412
export function addLabelToScript(script: string, name: string, position: number): string {
13671413
const parsed = parseGsapScriptAcornForWrite(script);
13681414
if (!parsed) return script;
13691415

1416+
// If the label already exists, MOVE it (overwrite its position) rather than
1417+
// appending a duplicate. Two same-named addLabel statements make removeLabel
1418+
// over-remove — it deletes every match, including a pre-existing label the
1419+
// user never touched.
1420+
const existing = findLabelStatements(parsed, name)[0];
1421+
if (existing) {
1422+
const ms = new MagicString(script);
1423+
const posArg = existing.expression.arguments?.[1];
1424+
if (posArg) ms.overwrite(posArg.start, posArg.end, valueToCode(position));
1425+
else ms.appendLeft(existing.expression.end - 1, `, ${valueToCode(position)}`);
1426+
return ms.toString();
1427+
}
1428+
13701429
const insertionPoint = findInsertionPoint(parsed);
13711430
if (insertionPoint === null) return script;
13721431

@@ -1380,24 +1439,7 @@ export function removeLabelFromScript(script: string, name: string): string {
13801439
const parsed = parseGsapScriptAcornForWrite(script);
13811440
if (!parsed) return script;
13821441

1383-
const targets: any[] = [];
1384-
acornWalk.simple(parsed.ast, {
1385-
// fallow-ignore-next-line complexity
1386-
ExpressionStatement(node: any) {
1387-
const expr = node.expression;
1388-
if (
1389-
expr?.type === "CallExpression" &&
1390-
expr.callee?.type === "MemberExpression" &&
1391-
isTimelineRooted(expr.callee.object, parsed.timelineVar) &&
1392-
expr.callee.property?.name === "addLabel" &&
1393-
expr.arguments?.[0]?.type === "Literal" &&
1394-
expr.arguments[0].value === name
1395-
) {
1396-
targets.push(node);
1397-
}
1398-
},
1399-
});
1400-
1442+
const targets = findLabelStatements(parsed, name);
14011443
if (!targets.length) return script;
14021444

14031445
const ms = new MagicString(script);

packages/core/src/parsers/gsapWriterParity.corpus.test.ts

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -616,17 +616,19 @@ describe("correctness — addLabelToScript / removeLabelFromScript", () => {
616616
expect(removeLabelFromScript(SYN_SINGLE, "nope")).toBe(SYN_SINGLE);
617617
});
618618

619-
it("adding the same label twice yields two addLabel calls (no dedup contract)", () => {
619+
it("adding the same label twice MOVES it instead of duplicating (dedup contract)", () => {
620+
// A second addLabel for an existing name must not append a duplicate —
621+
// duplicates make removeLabel over-remove. It moves the label's position.
620622
const once = addLabelToScript(SYN_SINGLE, "mid", 1.0);
621623
const twice = addLabelToScript(once, "mid", 2.0);
622-
expect(labelCallCount(twice, "mid")).toBe(2);
624+
expect(labelCallCount(twice, "mid")).toBe(1);
625+
expect(twice).toContain('tl.addLabel("mid", 2)');
623626
});
624627

625-
it("removeLabel deletes ALL matching addLabel calls for the name", () => {
626-
const once = addLabelToScript(SYN_SINGLE, "mid", 1.0);
627-
const twice = addLabelToScript(once, "mid", 2.0);
628-
const cleared = removeLabelFromScript(twice, "mid");
629-
expect(labelCallCount(cleared, "mid")).toBe(0);
628+
it("removeLabel deletes ALL matching addLabel calls for the name (hand-authored dups)", () => {
629+
const dup = `var tl = gsap.timeline({ paused: true });\ntl.addLabel("mid", 1);\ntl.addLabel("mid", 2);\nwindow.__timelines["t"] = tl;`;
630+
expect(labelCallCount(dup, "mid")).toBe(2);
631+
expect(labelCallCount(removeLabelFromScript(dup, "mid"), "mid")).toBe(0);
630632
});
631633

632634
it("the added label is observable by the parser when a tween references it", () => {

0 commit comments

Comments
 (0)