fix(studio): give every keyframe callback one mutation frame

Each timeline keyframe callback now reads and writes through the SAME
element: the clicked element's animations resolve the target, its selection
commits the mutation, and its animation computes the playhead percentage.
onMoveKeyframeToPlayhead previously took the percentage from the clicked
element and the animation plus selection from the current one, so a context
menu on a non-selected diamond retimed against one tween and wrote into
another file. onChangeKeyframeEase and onToggleKeyframeAtPlayhead had the
same split. An explicit null selection override now aborts the write instead
of falling back to the current selection.
This commit is contained in:
Miguel Angel Simon Sierra
2026-07-28 00:12:45 +02:00
parent a423c9300e
commit 9fc0011703
4 changed files with 158 additions and 60 deletions
@@ -255,13 +255,16 @@ describe("useTimelineEditCallbacks — flat tween keyframe lanes", () => {
// not belong to the selected element, and it passes no explicit target — so // not belong to the selected element, and it passes no explicit target — so
// the resolve falls back to the cache. Reading the SELECTED element's cache // the resolve falls back to the cache. Reading the SELECTED element's cache
// there resolves against the wrong element's keyframes. // there resolves against the wrong element's keyframes.
it("resolves a cache fallback against the clicked element, not the selected one", () => { it("resolves a cache fallback against the clicked element, not the selected one", async () => {
const circle: TimelineElement = { const circle: TimelineElement = {
...element, ...element,
id: "circle", id: "circle",
key: "scenes/main.html#circle", key: "scenes/main.html#circle",
domId: "circle", domId: "circle",
sourceFile: "scenes/main.html",
}; };
const circleSelection = { id: "circle", selector: "#circle", sourceFile: "scenes/main.html" };
mocks.actions.buildDomSelectionForTimelineElement.mockResolvedValue(circleSelection);
usePlayerStore.setState({ usePlayerStore.setState({
elements: [element, circle], elements: [element, circle],
gsapAnimations: new Map([["scenes/main.html#circle", [otherKeyframedAnimation]]]), gsapAnimations: new Map([["scenes/main.html#circle", [otherKeyframedAnimation]]]),
@@ -293,13 +296,18 @@ describe("useTimelineEditCallbacks — flat tween keyframe lanes", () => {
}); });
const view = renderCallbacks(); const view = renderCallbacks();
act(() => { await act(async () => {
view.callbacks.onMoveKeyframeToPlayhead?.("scenes/main.html#circle", { percentage: 100 }); view.callbacks.onMoveKeyframeToPlayhead?.("scenes/main.html#circle", { percentage: 100 });
await Promise.resolve();
}); });
// The retime target, the selection it commits through, and the animation the
// playhead percentage is computed against all come from the CLICKED element.
expect(mocks.actions.handleGsapMoveKeyframeToPlayhead).toHaveBeenCalledWith( expect(mocks.actions.handleGsapMoveKeyframeToPlayhead).toHaveBeenCalledWith(
otherKeyframedAnimation.id, otherKeyframedAnimation.id,
100, 100,
circleSelection,
otherKeyframedAnimation,
); );
view.unmount(); view.unmount();
}); });
@@ -316,7 +324,10 @@ describe("useTimelineEditCallbacks — flat tween keyframe lanes", () => {
}); });
}); });
expect(mocks.actions.handleGsapDeleteAnimation).toHaveBeenCalledWith(flatAnimation.id); expect(mocks.actions.handleGsapDeleteAnimation).toHaveBeenCalledWith(
flatAnimation.id,
undefined,
);
expect(mocks.actions.handleGsapRemoveKeyframe).not.toHaveBeenCalled(); expect(mocks.actions.handleGsapRemoveKeyframe).not.toHaveBeenCalled();
view.unmount(); view.unmount();
}); });
@@ -391,7 +402,12 @@ describe("useTimelineEditCallbacks — flat tween keyframe lanes", () => {
}); });
}); });
expect(mocks.actions.handleGsapRemoveKeyframe).toHaveBeenCalledWith(flatAnimation.id, 50); expect(mocks.actions.handleGsapRemoveKeyframe).toHaveBeenCalledWith(
flatAnimation.id,
50,
undefined,
undefined,
);
expect(mocks.actions.handleGsapDeleteAnimation).not.toHaveBeenCalled(); expect(mocks.actions.handleGsapDeleteAnimation).not.toHaveBeenCalled();
view.unmount(); view.unmount();
}); });
@@ -171,12 +171,10 @@ export function useTimelineEditCallbacks({
) => { ) => {
const animation = animations.find((candidate) => candidate.id === animationId); const animation = animations.find((candidate) => candidate.id === animationId);
if (animation && !animation.keyframes) { if (animation && !animation.keyframes) {
if (selectionOverride === undefined) handleGsapDeleteAnimation(animationId); handleGsapDeleteAnimation(animationId, selectionOverride);
else handleGsapDeleteAnimation(animationId, selectionOverride);
return; return;
} }
if (selectionOverride === undefined) handleGsapRemoveKeyframe(animationId, percentage); handleGsapRemoveKeyframe(animationId, percentage, undefined, selectionOverride);
else handleGsapRemoveKeyframe(animationId, percentage, undefined, selectionOverride);
}, },
[handleGsapDeleteAnimation, handleGsapRemoveKeyframe], [handleGsapDeleteAnimation, handleGsapRemoveKeyframe],
); );
@@ -216,10 +214,25 @@ export function useTimelineEditCallbacks({
removeKeyframeTarget(target.animId, target.tweenPct, animations, selection); removeKeyframeTarget(target.animId, target.tweenPct, animations, selection);
}); });
}, },
// Retime the keyframe to the playhead, preserving its value + ease. // Retime the keyframe to the playhead, preserving its value + ease. The
// clicked element owns the whole write: its animations resolve the target,
// its selection commits it, and its animation computes the playhead
// percentage. Mixing frames here retimed against the selected element's
// tween and wrote the result into the clicked element's file.
onMoveKeyframeToPlayhead: (elId, keyframe) => { onMoveKeyframeToPlayhead: (elId, keyframe) => {
const target = resolveKeyframeTarget(keyframe, resolveElementAnimations(elId), elId); const animations = resolveElementAnimations(elId);
if (target) handleGsapMoveKeyframeToPlayhead(target.animId, target.tweenPct); const target = resolveKeyframeTarget(keyframe, animations, elId);
const animation = target
? animations.find((candidate) => candidate.id === target.animId)
: undefined;
if (!target || !animation) return;
const element = usePlayerStore.getState().elements.find((el) => (el.key ?? el.id) === elId);
if (!element) return;
void buildDomSelectionForTimelineElement(element).then((selection) => {
if (selection) {
handleGsapMoveKeyframeToPlayhead(target.animId, target.tweenPct, selection, animation);
}
});
}, },
// Drag-to-retime. The diamond reports clip-%s; resolveKeyframeTarget gives // Drag-to-retime. The diamond reports clip-%s; resolveKeyframeTarget gives
// the dragged keyframe's anim + tween-%. We convert the clip-% drop to an // the dragged keyframe's anim + tween-%. We convert the clip-% drop to an
@@ -294,10 +307,19 @@ export function useTimelineEditCallbacks({
} }
return true; return true;
}, },
onChangeKeyframeEase: (_elId: string, _pct: number, ease: string) => { onChangeKeyframeEase: (elId: string, _pct: number, ease: string) => {
for (const anim of selectedGsapAnimations) { // The edited element's own animations + selection, not the selection's:
if (anim.keyframes) handleGsapUpdateMeta(anim.id, { ease }); // an ease change on a non-selected lane otherwise rewrote whichever
} // element happened to be selected, in whichever file it lives.
const animations = resolveElementAnimations(elId);
const element = usePlayerStore.getState().elements.find((el) => (el.key ?? el.id) === elId);
if (!element) return;
void buildDomSelectionForTimelineElement(element).then((selection) => {
if (!selection) return;
for (const anim of animations) {
if (anim.keyframes) handleGsapUpdateMeta(anim.id, { ease }, selection);
}
});
}, },
// fallow-ignore-next-line complexity // fallow-ignore-next-line complexity
onToggleKeyframeAtPlayhead: (el: TimelineElement) => { onToggleKeyframeAtPlayhead: (el: TimelineElement) => {
@@ -306,18 +328,34 @@ export function useTimelineEditCallbacks({
el.duration > 0 el.duration > 0
? Math.max(0, Math.min(100, Math.round(((currentTime - el.start) / el.duration) * 100))) ? Math.max(0, Math.min(100, Math.round(((currentTime - el.start) / el.duration) * 100)))
: 0; : 0;
const anim = selectedGsapAnimations.find((a) => a.keyframes); // Same frame for read and write: the toggled element's animations decide
if (anim?.keyframes) { // add-vs-remove, and its selection is what the mutation commits through.
const existing = anim.keyframes.keyframes.find((k) => Math.abs(k.percentage - pct) <= 1); const animations = resolveElementAnimations(el.key ?? el.id);
if (existing) { void buildDomSelectionForTimelineElement(el).then((selection) => {
handleGsapRemoveKeyframe(anim.id, existing.percentage); if (!selection) return;
const anim = animations.find((a) => a.keyframes);
if (anim?.keyframes) {
const existing = anim.keyframes.keyframes.find(
(k) => Math.abs(k.percentage - pct) <= 1,
);
if (existing) {
handleGsapRemoveKeyframe(anim.id, existing.percentage, undefined, selection);
} else {
handleGsapAddKeyframe(anim.id, pct, "x", 0, selection);
}
} else { } else {
handleGsapAddKeyframe(anim.id, pct, "x", 0); const flatAnim = animations.find((a) => !a.keyframes);
if (flatAnim) {
void handleGsapConvertToKeyframes(
flatAnim.id,
undefined,
undefined,
undefined,
selection,
);
}
} }
} else { });
const flatAnim = selectedGsapAnimations.find((a) => !a.keyframes);
if (flatAnim) handleGsapConvertToKeyframes(flatAnim.id);
}
}, },
onTogglePropertyGroupKeyframe: async (element, target) => { onTogglePropertyGroupKeyframe: async (element, target) => {
const selection = await buildDomSelectionForTimelineElement(element); const selection = await buildDomSelectionForTimelineElement(element);
@@ -2,6 +2,7 @@
import { act } from "react"; import { act } from "react";
import { createRoot } from "react-dom/client"; import { createRoot } from "react-dom/client";
import { describe, expect, it, vi } from "vitest"; import { describe, expect, it, vi } from "vitest";
import type { GsapAnimation } from "@hyperframes/core/gsap-parser";
import type { DomEditSelection } from "../components/editor/domEditingTypes"; import type { DomEditSelection } from "../components/editor/domEditingTypes";
import { useGsapSelectionHandlers } from "./useGsapSelectionHandlers"; import { useGsapSelectionHandlers } from "./useGsapSelectionHandlers";
@@ -114,3 +115,32 @@ describe("useGsapSelectionHandlers save failures", () => {
rendered.unmount(); rendered.unmount();
}); });
}); });
describe("useGsapSelectionHandlers selection override", () => {
it("aborts on an explicit null override instead of writing to the current selection", () => {
const removeKeyframe = vi.fn();
const rendered = renderHandlers(makeParams({ removeKeyframe }));
// Explicit null: the caller resolved a selection for its own element and
// found none, so the write must not land on the selected element.
rendered.handlers().handleGsapRemoveKeyframe("anim-1", 50, undefined, null);
expect(removeKeyframe).not.toHaveBeenCalled();
// Omitted override: falls back to the current selection as before.
rendered.handlers().handleGsapRemoveKeyframe("anim-1", 50);
expect(removeKeyframe).toHaveBeenCalledOnce();
rendered.unmount();
});
it("computes the playhead percentage from the passed animation, not the selection's", () => {
const moveKeyframe = vi.fn();
const selection = makeSelection();
const animation = { id: "anim-1", keyframes: { keyframes: [] } } as unknown as GsapAnimation;
const rendered = renderHandlers(makeParams({ moveKeyframe, selectedGsapAnimations: [] }));
rendered.handlers().handleGsapMoveKeyframeToPlayhead("anim-1", 50, selection, animation);
expect(moveKeyframe).toHaveBeenCalledWith(selection, "anim-1", 50, expect.any(Number));
rendered.unmount();
});
});
@@ -118,6 +118,19 @@ export function useGsapSelectionHandlers({
const lastSelectionRef = useRef<DomEditSelection | null>(null); const lastSelectionRef = useRef<DomEditSelection | null>(null);
if (domEditSelection) lastSelectionRef.current = domEditSelection; if (domEditSelection) lastSelectionRef.current = domEditSelection;
// `undefined` means the caller passed no override and accepts the current
// selection. An explicit `null` means the caller RESOLVED a selection for the
// element it is editing and there is none: falling back to domEditSelection
// there commits the edit onto whichever element happens to be selected, which
// is a different element's file. Only `undefined` may fall back.
const resolveWriteSelection = useCallback(
(selectionOverride?: DomEditSelection | null): DomEditSelection | null =>
selectionOverride === undefined
? (domEditSelection ?? lastSelectionRef.current)
: selectionOverride,
[domEditSelection],
);
const trackGsapHandlerFailure = useCallback( const trackGsapHandlerFailure = useCallback(
(error: unknown, selection: DomEditSelection, mutationType: string, label: string) => { (error: unknown, selection: DomEditSelection, mutationType: string, label: string) => {
trackStudioSaveFailure({ trackStudioSaveFailure({
@@ -160,7 +173,7 @@ export function useGsapSelectionHandlers({
updates: { duration?: number; ease?: string; position?: number }, updates: { duration?: number; ease?: string; position?: number },
selectionOverride?: DomEditSelection | null, selectionOverride?: DomEditSelection | null,
) => { ) => {
const sel = selectionOverride ?? domEditSelection ?? lastSelectionRef.current; const sel = resolveWriteSelection(selectionOverride);
if (!sel) return; if (!sel) return;
observeGsapMutation( observeGsapMutation(
updateGsapMeta(sel, animId, updates), updateGsapMeta(sel, animId, updates),
@@ -169,16 +182,16 @@ export function useGsapSelectionHandlers({
"Edit GSAP animation", "Edit GSAP animation",
); );
}, },
[domEditSelection, observeGsapMutation, updateGsapMeta], [resolveWriteSelection, observeGsapMutation, updateGsapMeta],
); );
const handleGsapDeleteAnimation = useCallback( const handleGsapDeleteAnimation = useCallback(
(animId: string, selectionOverride?: DomEditSelection | null) => { (animId: string, selectionOverride?: DomEditSelection | null) => {
const sel = selectionOverride ?? domEditSelection ?? lastSelectionRef.current; const sel = resolveWriteSelection(selectionOverride);
if (!sel) return; if (!sel) return;
observeGsapMutation(deleteGsapAnimation(sel, animId), sel, "delete", "Delete GSAP animation"); observeGsapMutation(deleteGsapAnimation(sel, animId), sel, "delete", "Delete GSAP animation");
}, },
[domEditSelection, deleteGsapAnimation, observeGsapMutation], [resolveWriteSelection, deleteGsapAnimation, observeGsapMutation],
); );
const handleGsapDeleteAllForElement = useCallback( const handleGsapDeleteAllForElement = useCallback(
@@ -284,12 +297,12 @@ export function useGsapSelectionHandlers({
value: number | string, value: number | string,
selectionOverride?: DomEditSelection | null, selectionOverride?: DomEditSelection | null,
) => { ) => {
const sel = selectionOverride ?? domEditSelection ?? lastSelectionRef.current; const sel = resolveWriteSelection(selectionOverride);
if (!sel) return; if (!sel) return;
trackStudioEvent("keyframe", { action: "add", property }); trackStudioEvent("keyframe", { action: "add", property });
addKeyframe(sel, animId, percentage, property, value); addKeyframe(sel, animId, percentage, property, value);
}, },
[domEditSelection, addKeyframe], [resolveWriteSelection, addKeyframe],
); );
const handleGsapAddKeyframeBatch = useCallback( const handleGsapAddKeyframeBatch = useCallback(
@@ -300,7 +313,7 @@ export function useGsapSelectionHandlers({
commitOverrides?: Partial<CommitMutationOptions>, commitOverrides?: Partial<CommitMutationOptions>,
selectionOverride?: DomEditSelection | null, selectionOverride?: DomEditSelection | null,
) => { ) => {
const sel = selectionOverride ?? domEditSelection ?? lastSelectionRef.current; const sel = resolveWriteSelection(selectionOverride);
if (!sel) return Promise.resolve(); if (!sel) return Promise.resolve();
return addKeyframeBatch(sel, animId, percentage, properties, commitOverrides).catch( return addKeyframeBatch(sel, animId, percentage, properties, commitOverrides).catch(
(error) => { (error) => {
@@ -308,7 +321,7 @@ export function useGsapSelectionHandlers({
}, },
); );
}, },
[domEditSelection, addKeyframeBatch, trackGsapHandlerFailure], [resolveWriteSelection, addKeyframeBatch, trackGsapHandlerFailure],
); );
const handleGsapRemoveKeyframe = useCallback( const handleGsapRemoveKeyframe = useCallback(
( (
@@ -317,26 +330,34 @@ export function useGsapSelectionHandlers({
commitOverrides?: Partial<CommitMutationOptions>, commitOverrides?: Partial<CommitMutationOptions>,
selectionOverride?: DomEditSelection | null, selectionOverride?: DomEditSelection | null,
) => { ) => {
const sel = selectionOverride ?? domEditSelection ?? lastSelectionRef.current; const sel = resolveWriteSelection(selectionOverride);
if (!sel) return; if (!sel) return;
trackStudioEvent("keyframe", { action: "remove" }); trackStudioEvent("keyframe", { action: "remove" });
removeKeyframe(sel, animId, percentage, commitOverrides); removeKeyframe(sel, animId, percentage, commitOverrides);
}, },
[domEditSelection, removeKeyframe], [resolveWriteSelection, removeKeyframe],
); );
const handleGsapMoveKeyframeToPlayhead = useCallback( const handleGsapMoveKeyframeToPlayhead = useCallback(
(animId: string, fromPercentage: number, selectionOverride?: DomEditSelection | null) => { (
const sel = selectionOverride ?? domEditSelection ?? lastSelectionRef.current; animId: string,
fromPercentage: number,
selectionOverride?: DomEditSelection | null,
animationOverride?: GsapAnimation,
) => {
const sel = resolveWriteSelection(selectionOverride);
if (!sel) return; if (!sel) return;
// Retime the keyframe to the playhead, preserving its value + ease. The // Retime the keyframe to the playhead, preserving its value + ease. The
// playhead's tween-relative percentage is the move target. // playhead's tween-relative percentage is the move target, and it has to
const anim = selectedGsapAnimations.find((a) => a.id === animId); // come from the SAME element the write lands on: reading the animation off
// the current selection while the percentage came from the clicked element
// computes the target against one tween and writes it into another.
const anim = animationOverride ?? selectedGsapAnimations.find((a) => a.id === animId);
const toPercentage = computeCurrentPercentage(sel, anim); const toPercentage = computeCurrentPercentage(sel, anim);
trackStudioEvent("keyframe", { action: "move_to_playhead" }); trackStudioEvent("keyframe", { action: "move_to_playhead" });
moveKeyframe(sel, animId, fromPercentage, toPercentage); moveKeyframe(sel, animId, fromPercentage, toPercentage);
}, },
[domEditSelection, selectedGsapAnimations, moveKeyframe], [resolveWriteSelection, selectedGsapAnimations, moveKeyframe],
); );
const handleGsapMoveKeyframe = useCallback( const handleGsapMoveKeyframe = useCallback(
@@ -346,7 +367,7 @@ export function useGsapSelectionHandlers({
toPercentage: number, toPercentage: number,
selectionOverride?: DomEditSelection | null, selectionOverride?: DomEditSelection | null,
) => { ) => {
const sel = selectionOverride ?? domEditSelection ?? lastSelectionRef.current; const sel = resolveWriteSelection(selectionOverride);
if (!sel) return; if (!sel) return;
// Atomic retime: preserves the keyframe's value + per-keyframe ease. Both // Atomic retime: preserves the keyframe's value + per-keyframe ease. Both
// percentages are tween-relative (the drag handler converts the drop // percentages are tween-relative (the drag handler converts the drop
@@ -355,7 +376,7 @@ export function useGsapSelectionHandlers({
trackStudioEvent("keyframe", { action: "retime" }); trackStudioEvent("keyframe", { action: "retime" });
moveKeyframe(sel, animId, fromPercentage, toPercentage); moveKeyframe(sel, animId, fromPercentage, toPercentage);
}, },
[domEditSelection, moveKeyframe], [resolveWriteSelection, moveKeyframe],
); );
const handleGsapResizeKeyframedTween = useCallback( const handleGsapResizeKeyframedTween = useCallback(
@@ -366,14 +387,14 @@ export function useGsapSelectionHandlers({
pctRemap: Array<{ from: number; to: number }>, pctRemap: Array<{ from: number; to: number }>,
selectionOverride?: DomEditSelection | null, selectionOverride?: DomEditSelection | null,
) => { ) => {
const sel = selectionOverride ?? domEditSelection ?? lastSelectionRef.current; const sel = resolveWriteSelection(selectionOverride);
if (!sel) return; if (!sel) return;
// Boundary drag-to-retime: grows/shifts the tween window + re-keys keyframes // Boundary drag-to-retime: grows/shifts the tween window + re-keys keyframes
// in place. Distinct telemetry action so resize is separable from in-window move. // in place. Distinct telemetry action so resize is separable from in-window move.
trackStudioEvent("keyframe", { action: "retime_resize" }); trackStudioEvent("keyframe", { action: "retime_resize" });
resizeKeyframedTween(sel, animId, position, duration, pctRemap); resizeKeyframedTween(sel, animId, position, duration, pctRemap);
}, },
[domEditSelection, resizeKeyframedTween], [resolveWriteSelection, resizeKeyframedTween],
); );
const handleGsapConvertToKeyframes = useCallback( const handleGsapConvertToKeyframes = useCallback(
@@ -382,24 +403,17 @@ export function useGsapSelectionHandlers({
resolvedFromValues?: Record<string, number | string>, resolvedFromValues?: Record<string, number | string>,
duration?: number, duration?: number,
commitOverrides?: Partial<CommitMutationOptions>, commitOverrides?: Partial<CommitMutationOptions>,
selectionOverride?: DomEditSelection | null,
) => { ) => {
if (!domEditSelection) return Promise.resolve(); const sel = resolveWriteSelection(selectionOverride);
return convertToKeyframes( if (!sel) return Promise.resolve();
domEditSelection, return convertToKeyframes(sel, animId, resolvedFromValues, duration, commitOverrides).catch(
animId, (error) => {
resolvedFromValues, trackGsapHandlerFailure(error, sel, "convert-to-keyframes", "Convert to keyframes");
duration, },
commitOverrides, );
).catch((error) => {
trackGsapHandlerFailure(
error,
domEditSelection,
"convert-to-keyframes",
"Convert to keyframes",
);
});
}, },
[domEditSelection, convertToKeyframes, trackGsapHandlerFailure], [resolveWriteSelection, convertToKeyframes, trackGsapHandlerFailure],
); );
const handleGsapRemoveAllKeyframes = useCallback( const handleGsapRemoveAllKeyframes = useCallback(