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

* fix(sdk,studio): R5 cutover review fixes — fromTo dest, timing sync, parity

Confirmed correctness findings from the R5 review of the SDK cutover stack,
applied on top of #1539:

- fromTo add via cutover dropped its destination: handleAddGsapTween read only
  `toProperties`; now falls back to `properties` like every other method.
- handleSetTiming GSAP sync: a clip with no data-start skipped the shift (now
  treats start as 0, matching the server path) and a blank/non-numeric
  data-start wrote position: NaN (now sanitized).
- handleSetTiming no longer appends an absolute position to an auto-sequenced
  (implicit-position) tween, which collapsed staggers.
- handleSetTiming keeps data-end in sync when a clip carries BOTH data-duration
  and data-end (a stale data-end inverted the clip).
- string/relative tween positions ("+=0.5", "<") documented as a known ceiling.
- opacity/autoAlpha property seed no longer falsy-zero (`|| 1`): an element at
  opacity 0 seeds 0, not 1.
- optimistic add-keyframe cache tolerance aligned to the writer's PCT_TOLERANCE
  (2%) so a near-neighbour keyframe no longer shows then vanishes on reload.
- DOM-patch finiteness validation runs before the SDK cutover path.
- attribute ops mapping to a reserved data-* name decline the cutover up front
  instead of throwing inside dispatch.

Regression tests added for each SDK-side fix.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* fix(studio): close two gaps in the reserved-attr cutover gate

- Lowercase the mapped attribute name before the reserved check, matching the
  SDK's validateSetAttribute (which lowercases), so a case-variant reserved
  name is declined up front instead of throwing inside dispatch.
- Also gate `html-attribute` ops (raw, non-prefixed names), not just bare
  `attribute` ops. Both the emitter and the gate now derive the name via one
  shared `sdkAttrName` helper so they can't drift.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* fix(studio): match keyframe remove-path tolerance to the writer (mirror of add)

The optimistic remove-keyframe cache filtered with `> 0.001`, dropping only a
near-exact match, while the writer removes within PCT_TOLERANCE (2). Removing
at e.g. 49% dropped a 50% keyframe on disk but left it in the cache — a phantom
that vanished on reload, the inverted twin of the add-path tolerance fix.
Now filters with `> 2` to match.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
Vance Ingalls
2026-06-17 17:15:16 -07:00
committed by GitHub
co-authored by Claude Opus 4.8
parent 8c981a451a
commit e57e75b9b4
7 changed files with 195 additions and 29 deletions
@@ -184,6 +184,26 @@ describe("addGsapTween", () => {
expect(newScript).toContain("opacity: 0");
expect(newScript).toContain("opacity: 1");
});
it("R5 #1: fromTo destination supplied via `properties` (Studio add path) is not dropped", () => {
const parsed = fresh();
const result = applyOp(parsed, {
type: "addGsapTween",
target: "hf-box",
tween: {
method: "fromTo",
duration: 0.5,
fromProperties: { opacity: 0 },
// Studio's add path puts the destination in `properties`, not `toProperties`.
properties: { x: 400, opacity: 1 },
},
});
const newScript = String(result.forward[0]?.value ?? "");
expect(newScript).toContain("fromTo(");
// Regression: fromTo previously read only `toProperties` and wrote empty
// to-vars, so the destination vanished.
expect(newScript).toContain("x: 400");
});
});
// ─── Tween op test helpers ────────────────────────────────────────────────────
@@ -1027,4 +1047,49 @@ describe("handleSetTiming GSAP sync (CF2 #15/#16)", () => {
// tween duration scaled 4 → 8 (ratio 2).
expect(getScript(parsed)).toContain("duration: 8");
});
it("R5 #3: a start-less clip (no data-start) still shifts its tween (implicit start 0)", () => {
const parsed = timingDoc(`data-duration="4"`, `tl.to("#box", { x: 100, duration: 1 }, 2);`);
applyOp(parsed, { type: "setTiming", target: "hf-box", start: 3 });
// oldStart defaults to 0, so position remaps 2 → 3 + (2 0) = 5.
// The bug skipped the whole sync block when data-start was absent.
expect(getScript(parsed)).toMatch(/tl\.to\("#box",[^)]*\}, 5\)/);
});
it("R5 #3: a malformed data-start never writes position: NaN", () => {
const parsed = timingDoc(
`data-start="" data-duration="4"`,
`tl.to("#box", { x: 100, duration: 1 }, 2);`,
);
applyOp(parsed, { type: "setTiming", target: "hf-box", start: 3 });
const script = getScript(parsed);
expect(script).not.toContain("NaN");
expect(script).toMatch(/tl\.to\("#box",[^)]*\}, 5\)/);
});
it("R5 #2: an implicit-position tween is not collapsed to an absolute position on move", () => {
const parsed = timingDoc(
`data-start="2" data-end="5"`,
`tl.to("#box", { x: 100, duration: 1 });`,
);
applyOp(parsed, { type: "setTiming", target: "hf-box", start: 5 });
const script = getScript(parsed);
// The tween had no position arg (auto-sequenced); it must stay that way —
// appending an absolute position would collapse the stagger.
expect(script).toContain('tl.to("#box", { x: 100, duration: 1 })');
expect(script).not.toMatch(/tl\.to\("#box",[^)]*\}, \d/);
});
it("R5 #7: a clip with BOTH data-duration and data-end keeps data-end in sync on move", () => {
const parsed = timingDoc(
`data-start="1" data-duration="2" data-end="3"`,
`tl.to("#box", { x: 1, duration: 2 }, 1);`,
);
applyOp(parsed, { type: "setTiming", target: "hf-box", start: 5 });
const el = parsed.document.querySelector('[data-hf-id="hf-box"]');
expect(el?.getAttribute("data-start")).toBe("5");
expect(el?.getAttribute("data-duration")).toBe("2");
// data-end recomputed (5 + 2); the bug left it stale at 3 → inverted clip.
expect(el?.getAttribute("data-end")).toBe("7");
});
});
+34 -8
View File
@@ -437,6 +437,17 @@ function handleSetTiming(
result.inverse.push(p.inverse);
el.setAttribute("data-duration", String(newDuration));
}
// A clip carrying BOTH data-duration and data-end must keep data-end in
// sync (end = start + duration) on any start/duration change, else the
// stale data-end inverts the clip (end < start) for runtimes that read it.
if (oldEndStr !== null && newStart !== null && newDuration !== null) {
const newEnd = newStart + newDuration;
const endPath = timingPath(id, "end");
const ep = scalarChange(endPath, oldEnd, newEnd);
result.forward.push(ep.forward);
result.inverse.push(ep.inverse);
el.setAttribute("data-end", String(newEnd));
}
} else if (
(timing.duration !== undefined || timing.start !== undefined) &&
newStart !== null &&
@@ -470,7 +481,12 @@ function handleSetTiming(
// those clips left their tweens unsynced.
const matchHfId = el.getAttribute("data-hf-id") ?? id;
const matchDomId = el.getAttribute("id");
if (parsedGsap && currentScript && oldStart !== null) {
if (parsedGsap && currentScript) {
// A missing data-start means an implicit start of 0 (matching the server
// shiftGsapPositions path); a malformed attr parses to NaN. Sanitize to a
// finite number so a start-less/blank clip still shifts and never feeds
// NaN into the tween positions.
const oldStartNum = oldStart !== null && Number.isFinite(oldStart) ? oldStart : 0;
// Per-tween shift/scale (mirrors shiftGsapPositions/scaleGsapPositions): a
// multi-tween stagger maps each tween's own intra-clip position by the
// start DELTA and scales its duration by the clip-duration RATIO. Writing
@@ -482,16 +498,25 @@ function handleSetTiming(
durChanged && oldDuration !== null && oldDuration > 0 && newDuration !== null
? newDuration / oldDuration
: 1;
const remapStart = startChanged && newStart !== null ? newStart : oldStart;
const remapStart = startChanged && newStart !== null ? newStart : oldStartNum;
for (const { id: animId, animation } of parsedGsap.located) {
const matches =
selectorMatchesId(animation.targetSelector, matchHfId) ||
(matchDomId !== null && selectorMatchesId(animation.targetSelector, matchDomId));
if (!matches) continue;
// Skip tweens whose position is a label or relative string ("+=0.5",
// "<", ">"): relative positions already track their neighbours, and a
// string position can't be safely shifted by the clip delta here.
// ponytail: known ceiling — string positions are not re-synced on
// move/resize; numeric positions only.
if (typeof animation.position !== "number") continue;
const updates: Partial<GsapAnimation> = {};
if (startChanged || durChanged) {
const shifted = remapStart + (animation.position - oldStart) * ratio;
// Don't write an absolute position onto an auto-sequenced tween (no
// explicit position arg → parsed as implicitPosition): the writer would
// APPEND a position arg, collapsing the stagger onto one point. Duration
// still scales below.
if ((startChanged || durChanged) && animation.implicitPosition !== true) {
const shifted = remapStart + (animation.position - oldStartNum) * ratio;
updates.position = Math.max(0, Math.round(shifted * 1000) / 1000);
}
if (durChanged && typeof animation.duration === "number" && animation.duration > 0) {
@@ -771,10 +796,11 @@ function handleAddGsapTween(
if (tween.yoyo !== undefined) extras.yoyo = tween.yoyo;
if (tween.stagger !== undefined) extras.stagger = tween.stagger;
const toProps =
tween.method === "fromTo"
? ((tween.toProperties ?? {}) as Record<string, number | string>)
: ((tween.toProperties ?? tween.properties ?? {}) as Record<string, number | string>);
// A fromTo's destination may arrive as either `toProperties` or `properties`
// (the Studio add path sets `properties`). Fall back the same way for every
// method — the old fromTo-only branch read `toProperties` alone and wrote an
// empty to-vars object, so fromTo animations added via cutover animated to {}.
const toProps = (tween.toProperties ?? tween.properties ?? {}) as Record<string, number | string>;
// Scoped ids like "hf-host/hf-leaf" must use the bare leaf id in the GSAP
// selector — only the leaf part is written as data-hf-id on the DOM element.