Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@ describe("AnimationCard ease editing", () => {

expect(onUpdateKeyframeEase).toHaveBeenCalledExactlyOnceWith(animation.id, 50, ease);
expect(trackStudioSegmentEaseEdit).toHaveBeenCalledExactlyOnceWith({
action: "commit",
ease,
});
act(() => view.root.unmount());
Expand Down
2 changes: 1 addition & 1 deletion packages/studio/src/components/editor/AnimationCard.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -267,7 +267,7 @@ export const AnimationCard = memo(function AnimationCard({
onToggle={setExpandedKfPct}
onEaseCommit={(pct, ease) => {
onUpdateKeyframeEase(animation.id, pct, ease);
trackStudioSegmentEaseEdit({ ease });
trackStudioSegmentEaseEdit({ action: "commit", ease });
}}
onApplyAll={
onSetAllKeyframeEases
Expand Down
4 changes: 2 additions & 2 deletions packages/studio/src/hooks/gsapDragCommit.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import { usePlayerStore } from "../player/store/playerStore";
import { readRuntimeKeyframes, scanAllRuntimeKeyframes } from "./gsapRuntimeKeyframes";
import { resolveTweenStart, resolveTweenDuration } from "../utils/globalTimeCompiler";
import { roundTo3 } from "../utils/rounding";
import { computeElementPercentage } from "./gsapShared";
import { computeElementPercentage, idSelector } from "./gsapShared";
import { computeDraggedGsapPosition } from "./draggedGsapPosition";
import type { RuntimeTweenChange } from "./gsapRuntimePatch";
import { isGestureTransactionCommit, runGestureTransaction } from "./gestureTransaction";
Expand Down Expand Up @@ -117,7 +117,7 @@ export async function materializeIfDynamic(
const allScanned = scanAllRuntimeKeyframes(iframe);
if (allScanned.size === 0) return;
const allElements = Array.from(allScanned.entries()).map(([id, data]) => ({
selector: `#${id}`,
selector: idSelector(id),
keyframes: data.keyframes,
easeEach: data.easeEach,
}));
Expand Down
96 changes: 95 additions & 1 deletion packages/studio/src/hooks/gsapKeyframeCacheHelpers.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ const animWithKeyframes = (id: string): GsapAnimation => ({
});

beforeEach(() => {
usePlayerStore.setState({ keyframeCache: new Map(), elements: [] });
usePlayerStore.setState({ keyframeCache: new Map(), gsapAnimations: new Map(), elements: [] });
});

describe("clearKeyframeCacheForElement", () => {
Expand Down Expand Up @@ -98,6 +98,41 @@ describe("clearKeyframeCacheForFile", () => {
});

describe("updateKeyframeCacheFromParsed", () => {
it("serializes a multi-keyframe tween with a stable shape and animation identity", () => {
const animation: GsapAnimation = {
...animWithKeyframes("hero"),
duration: 2,
resolvedStart: 3,
keyframes: {
format: "percentage",
keyframes: [
{ percentage: 0, properties: { x: 0 } },
{ percentage: 50, properties: { x: 100 }, ease: "power1.inOut" },
{ percentage: 100, properties: { x: 200 } },
],
easeEach: "power1.inOut",
},
};
usePlayerStore.setState({
elements: [
{
id: "hero-clip",
domId: "hero",
tag: "div",
start: 2,
duration: 4,
track: 0,
},
],
});

updateKeyframeCacheFromParsed([animation], "scene.html", "hero", {});

expect(JSON.stringify(cache().get("scene.html#hero"))).toBe(
'{"format":"percentage","keyframes":[{"percentage":25,"properties":{"x":0},"tweenPercentage":0,"propertyGroup":"position","animationId":"hero"},{"percentage":50,"properties":{"x":100},"ease":"power1.inOut","tweenPercentage":50,"propertyGroup":"position","animationId":"hero"},{"percentage":75,"properties":{"x":200},"tweenPercentage":100,"propertyGroup":"position","animationId":"hero"}],"easeEach":"power1.inOut"}',
);
});

it("clears the bare key when the selected element no longer has keyframes", () => {
// Element previously had keyframes, so a bare entry exists (writes set both).
seed("index.html#box");
Expand All @@ -118,4 +153,63 @@ describe("updateKeyframeCacheFromParsed", () => {
expect(cache().has("index.html#hero")).toBe(true);
expect(cache().has("hero")).toBe(true);
});

it("caches flat tweens as clip-relative start and end keyframes", () => {
const animation: GsapAnimation = {
id: "flat-box",
targetSelector: "#box",
method: "to",
position: 1,
properties: { x: 420 },
duration: 2,
resolvedStart: 1,
ease: "power2.out",
propertyGroup: "position",
};
usePlayerStore.setState({
elements: [{ id: "box-clip", domId: "box", tag: "div", start: 1, duration: 2, track: 0 }],
});

updateKeyframeCacheFromParsed([animation], "scene.html", "box", {});

expect(cache().get("scene.html#box")).toEqual({
format: "percentage",
keyframes: [
{
percentage: 0,
properties: { x: 0 },
tweenPercentage: 0,
propertyGroup: "position",
animationId: "flat-box",
},
{
percentage: 100,
properties: { x: 420 },
ease: "power2.out",
tweenPercentage: 100,
propertyGroup: "position",
animationId: "flat-box",
},
],
});
expect(usePlayerStore.getState().gsapAnimations.get("scene.html#box")).toEqual([animation]);
});

it("does not cache a flat tween without animatable numeric properties", () => {
const animation: GsapAnimation = {
id: "flat-box",
targetSelector: "#box",
method: "to",
position: 0,
properties: { backgroundColor: "#fff" },
duration: 1,
propertyGroup: "visual",
};

updateKeyframeCacheFromParsed([animation], "scene.html", "box", {});

expect(cache().has("scene.html#box")).toBe(false);
expect(cache().has("box")).toBe(false);
expect(usePlayerStore.getState().gsapAnimations.has("scene.html#box")).toBe(false);
});
});
60 changes: 50 additions & 10 deletions packages/studio/src/hooks/gsapKeyframeCacheHelpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import type { GsapAnimation } from "@hyperframes/core/gsap-parser";
import { usePlayerStore, type KeyframeCacheEntry } from "../player/store/playerStore";
import { toAbsoluteTime } from "./gsapShared";
import { synthesizeFlatTweenKeyframes } from "./gsapTweenSynth";

export function updateKeyframeCacheFromParsed(
animations: GsapAnimation[],
Expand All @@ -15,10 +16,16 @@ export function updateKeyframeCacheFromParsed(
const { setKeyframeCache, elements } = usePlayerStore.getState();
const idsWithKeyframes = new Set<string>();
const merged = new Map<string, KeyframeCacheEntry>();
const sourceAnimations = new Map<string, GsapAnimation[]>();
for (const anim of animations) {
const id = anim.targetSelector.match(/^#([\w-]+)/)?.[1];
if (!id || !anim.keyframes) continue;
const kfSource =

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟑 id extraction regex /^#([\w-]+)/ isn't paired with the new idSelector attribute-selector output

for (const anim of animations) {
  const id = anim.targetSelector.match(/^#([\w-]+)/)?.[1];
  const kfSource =
    anim.keyframes?.keyframes ?? synthesizeFlatTweenKeyframes(anim)?.keyframes ?? [];
  if (!id || kfSource.length === 0) continue;

This PR introduces idSelector (gsapShared.ts:81) which emits [id="01-hook-hero-word"] for digit-leading / dotted / spaced ids and is now used by gsapDragCommit.materializeIfDynamic, gsapScriptCommitHelpers.ensureElementAddressable, useGestureCommit.commit, and selectorFromSelection. When such a selector is written into the source file, the next parse returns anim.targetSelector = '[id="01-hook-hero-word"]'. The ^#([\w-]+) regex on :21 (and again on :91 for the mutation target) returns undefined for that shape, so the animation is silently skipped and the keyframe cache is never updated for that element. Same regex is unchanged, but the new idSelector primitive is a fresh source of selectors it can't parse β€” the two changes together create a latent bug for exactly the case idSelector is meant to make safe.

Fix: Extract id via a helper handling both shapes: sel.match(/^#([\w-]+)/)?.[1] ?? sel.match(/^\[id="((?:[^"\\]|\\.)+)"\]/)?.[1]?.replace(/\\(["\\])/g, '$1'). Cover the mutation-target site (:91) too.

β€” Review by Rames D Jusso

anim.keyframes?.keyframes ?? synthesizeFlatTweenKeyframes(anim)?.keyframes ?? [];
if (!id || kfSource.length === 0) continue;
idsWithKeyframes.add(id);
if (anim.propertyGroup) {
sourceAnimations.set(id, [...(sourceAnimations.get(id) ?? []), anim]);
}

// Convert tween-relative percentages to clip-relative so diamonds
// render at the correct position within the timeline clip.
Expand All @@ -29,7 +36,7 @@ export function updateKeyframeCacheFromParsed(
);
const elStart = timelineEl?.start ?? 0;
const elDuration = timelineEl?.duration ?? 1;
const clipKeyframes = anim.keyframes.keyframes.map((kf) => {
const clipKeyframes = kfSource.map((kf) => {
const absTime = toAbsoluteTime(tweenPos, tweenDur, kf.percentage);
const clipPct =
elDuration > 0 ? Math.round(((absTime - elStart) / elDuration) * 1000) / 10 : kf.percentage;
Expand All @@ -38,6 +45,7 @@ export function updateKeyframeCacheFromParsed(
percentage: clipPct,
tweenPercentage: kf.percentage,
propertyGroup: anim.propertyGroup,
animationId: anim.id,
};
});

Expand All @@ -48,20 +56,36 @@ export function updateKeyframeCacheFromParsed(
const prev = byPct.get(kf.percentage);
if (prev) {
prev.properties = { ...prev.properties, ...kf.properties };
// Mirror deduplicateKeyframes: a same-% collision across different

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟑 Ambiguity-flag logic is inlined identically in two places β€” contract drift risk across foundation slice

if (
  prev.animationId !== undefined &&
  kf.animationId !== undefined &&
  prev.animationId !== kf.animationId
) {
  prev.easeAmbiguous = true;
}
if (kf.ease) prev.ease = kf.ease;

Same 6-line ambiguity check appears in deduplicateKeyframes (gsapTweenSynth.ts:14-22). B2-B8 will add more consumers that merge cross-lane keyframes; implementation drift (tightening in one place, not the other) would silently show/hide the inline ease button inconsistently. Invariant is once ambiguous within a batch, always ambiguous β€” good, but it should be encoded once, not twice.

Fix: Extract a mergeKeyframeIntoExisting(existing, incoming) helper in gsapTweenSynth.ts that owns both the property-spread AND the ambiguity check, then reuse from both call sites. Also worth adding to the KeyframeCacheEntry TSDoc that easeAmbiguous is monotonic within a merge pass.

β€” Review by Rames D Jusso

// source animations is an ambiguous merged segment (the button can
// only target one arbitrary animation). Flag it so the collapsed row
// suppresses the inline ease button there.
if (
prev.animationId !== undefined &&
kf.animationId !== undefined &&
prev.animationId !== kf.animationId
) {
prev.easeAmbiguous = true;
}
if (kf.ease) prev.ease = kf.ease;
} else {
byPct.set(kf.percentage, { ...kf, properties: { ...kf.properties } });
}
}
existing.keyframes = Array.from(byPct.values()).sort((a, b) => a.percentage - b.percentage);
} else {
merged.set(id, { ...anim.keyframes, keyframes: clipKeyframes });
merged.set(id, {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟑 Synthesized flat-tween's top-level ease is dropped when written into keyframeCache

} else {
  merged.set(id, {
    ...anim.keyframes,
    format: anim.keyframes?.format ?? "percentage",
    keyframes: clipKeyframes,
  });
}

For a flat tween, anim.keyframes is undefined, so ...anim.keyframes is a no-op and only format + keyframes are set β€” the flat tween's ease (which synthesizeFlatTweenKeyframes returns at BOTH top level AND on the 100% keyframe, gsapTweenSynth.ts:88-92) is dropped from the entry-level ease. useGsapAnimationsForElement's parallel write (:400-405) DOES carry ease at the entry level. Contract mismatch: two writers, one produces { ease: 'power2.out', keyframes: [...] } for a flat tween, the other produces { keyframes: [...] } with the ease only on the 100% kf. B2-B8 consumers reading entry.ease will see it sometimes and not others.

Fix: Compute the source shape once: const src = anim.keyframes ?? synthesizeFlatTweenKeyframes(anim) above the kfSource extraction, then merged.set(id, { ...src, format: src?.format ?? 'percentage', keyframes: clipKeyframes }) for consistency with the useGsapTweenCache write.

β€” Review by Rames D Jusso

...anim.keyframes,
format: anim.keyframes?.format ?? "percentage",
keyframes: clipKeyframes,
});
}
}
for (const [id, entry] of merged) {
setKeyframeCache(`${targetPath}#${id}`, entry);
setKeyframeCache(id, entry);
if (targetPath !== "index.html") setKeyframeCache(`index.html#${id}`, entry);
writeGsapAnimationsForElement(targetPath, id, sourceAnimations.get(id));
}
const targetId =
(mutation as { targetSelector?: string }).targetSelector?.match(/^#([\w-]+)/)?.[1] ??
Expand All @@ -84,13 +108,12 @@ export function updateKeyframeCacheFromParsed(
* a new cache map and re-render every subscriber.
*/
export function clearKeyframeCacheForElement(sourceFile: string, elementId: string): void {
const { keyframeCache, setKeyframeCache } = usePlayerStore.getState();
const keys =
sourceFile === "index.html"
? [`index.html#${elementId}`, elementId]
: [`${sourceFile}#${elementId}`, `index.html#${elementId}`, elementId];
const { keyframeCache, setKeyframeCache, gsapAnimations, setGsapAnimations } =
usePlayerStore.getState();
const keys = elementCacheKeys(sourceFile, elementId);
for (const key of keys) {
if (keyframeCache.has(key)) setKeyframeCache(key, undefined);
if (gsapAnimations.has(key)) setGsapAnimations(key, undefined);
}
}

Expand All @@ -102,11 +125,11 @@ export function clearKeyframeCacheForElement(sourceFile: string, elementId: stri
* absent from the re-scan) leaves no stale bare entry behind.
*/
export function clearKeyframeCacheForFile(sourceFile: string): void {
const { keyframeCache } = usePlayerStore.getState();
const { keyframeCache, gsapAnimations } = usePlayerStore.getState();
const sfPrefix = `${sourceFile}#`;
const fallbackPrefix = "index.html#";
const ids = new Set<string>();
for (const key of keyframeCache.keys()) {
for (const key of [...keyframeCache.keys(), ...gsapAnimations.keys()]) {
const matchesFile =
key.startsWith(sfPrefix) || (sourceFile !== "index.html" && key.startsWith(fallbackPrefix));
if (!matchesFile) continue;
Expand All @@ -118,6 +141,23 @@ export function clearKeyframeCacheForFile(sourceFile: string): void {
}
}

function elementCacheKeys(sourceFile: string, elementId: string): string[] {
return sourceFile === "index.html"
? [`index.html#${elementId}`, elementId]
: [`${sourceFile}#${elementId}`, `index.html#${elementId}`, elementId];
}

export function writeGsapAnimationsForElement(
sourceFile: string,
elementId: string,
animations: GsapAnimation[] | undefined,
): void {
const { setGsapAnimations } = usePlayerStore.getState();
for (const key of elementCacheKeys(sourceFile, elementId)) {
setGsapAnimations(key, animations);
}
}

function buildCacheKey(sourceFile: string, elementId: string): string {
return `${sourceFile}#${elementId}`;
}
Expand Down
5 changes: 3 additions & 2 deletions packages/studio/src/hooks/gsapScriptCommitHelpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,13 @@ import { findUnsafeDomPatchValues } from "@hyperframes/core/studio-api/finite-mu
import type { DomEditSelection } from "../components/editor/domEditingTypes";

export { PROPERTY_DEFAULTS } from "./gsapShared";
import { idSelector } from "./gsapShared";

export function ensureElementAddressable(selection: DomEditSelection): {
selector: string;
autoId?: string;
} {
if (selection.id) return { selector: `#${selection.id}` };
if (selection.id) return { selector: idSelector(selection.id) };
if (selection.selector) return { selector: selection.selector };

const el = selection.element;
Expand All @@ -20,7 +21,7 @@ export function ensureElementAddressable(selection: DomEditSelection): {
id = `${tag}-${n}`;
}
el.setAttribute("id", id);
return { selector: `#${id}`, autoId: id };
return { selector: idSelector(id), autoId: id };
}

export class GsapMutationHttpError extends Error {
Expand Down
31 changes: 30 additions & 1 deletion packages/studio/src/hooks/gsapShared.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { describe, it, expect } from "vitest";
import type { GsapAnimation } from "@hyperframes/core/gsap-parser";
import { isInstantHold, parsePercentageKeyframes } from "./gsapShared";
import { idSelector, isInstantHold, parsePercentageKeyframes } from "./gsapShared";

describe("isInstantHold", () => {
const animation = (method: GsapAnimation["method"], duration?: number) =>
Expand Down Expand Up @@ -74,3 +74,32 @@ describe("parsePercentageKeyframes", () => {
expect(parsePercentageKeyframes({})).toBeNull();
});
});

describe("idSelector", () => {
it("uses #id for valid CSS identifiers", () => {
expect(idSelector("hero-word")).toBe("#hero-word");
expect(idSelector("el_1")).toBe("#el_1");
});

it("uses an attribute selector for ids that #id can't address (digit-leading, dots, spaces)", () => {
// #01-... / #a.b / #a b throw a SyntaxError in querySelector / GSAP, crashing
// the preview when such a target is committed (e.g. dragging the element).
expect(idSelector("01-hook-hero-word")).toBe('[id="01-hook-hero-word"]');
expect(idSelector("my.class")).toBe('[id="my.class"]');
expect(idSelector("1box")).toBe('[id="1box"]');
});

it("escapes quotes and backslashes in the attribute selector value", () => {
expect(idSelector('1"x')).toBe('[id="1\\"x"]');
});

it("only ever emits #id for ids that can't break querySelector", () => {
// Every id resolves to either a plain #id (only when safe) or an attribute
// selector β€” never a #id that would throw a SyntaxError.
for (const id of ["hero-word", "01-hook", "a.b", "a b", "1", "--x", '1"q']) {
const sel = idSelector(id);
if (sel.startsWith("#")) expect(sel).toBe(`#${id}`);
else expect(sel.startsWith('[id="')).toBe(true);
}
});
});
Loading
Loading