From 9fc00117037444711bac6963d35ee35ebc268da9 Mon Sep 17 00:00:00 2001 From: Miguel Angel Simon Sierra Date: Tue, 28 Jul 2026 00:12:45 +0200 Subject: [PATCH] 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. --- .../nle/useTimelineEditCallbacks.test.tsx | 24 +++++- .../nle/useTimelineEditCallbacks.ts | 80 +++++++++++++----- .../hooks/useGsapSelectionHandlers.test.tsx | 30 +++++++ .../src/hooks/useGsapSelectionHandlers.ts | 84 +++++++++++-------- 4 files changed, 158 insertions(+), 60 deletions(-) diff --git a/packages/studio/src/components/nle/useTimelineEditCallbacks.test.tsx b/packages/studio/src/components/nle/useTimelineEditCallbacks.test.tsx index dec72f7b1..ed83ac1ef 100644 --- a/packages/studio/src/components/nle/useTimelineEditCallbacks.test.tsx +++ b/packages/studio/src/components/nle/useTimelineEditCallbacks.test.tsx @@ -255,13 +255,16 @@ describe("useTimelineEditCallbacks — flat tween keyframe lanes", () => { // 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 // 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 = { ...element, id: "circle", key: "scenes/main.html#circle", domId: "circle", + sourceFile: "scenes/main.html", }; + const circleSelection = { id: "circle", selector: "#circle", sourceFile: "scenes/main.html" }; + mocks.actions.buildDomSelectionForTimelineElement.mockResolvedValue(circleSelection); usePlayerStore.setState({ elements: [element, circle], gsapAnimations: new Map([["scenes/main.html#circle", [otherKeyframedAnimation]]]), @@ -293,13 +296,18 @@ describe("useTimelineEditCallbacks — flat tween keyframe lanes", () => { }); const view = renderCallbacks(); - act(() => { + await act(async () => { 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( otherKeyframedAnimation.id, 100, + circleSelection, + otherKeyframedAnimation, ); 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(); 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(); view.unmount(); }); diff --git a/packages/studio/src/components/nle/useTimelineEditCallbacks.ts b/packages/studio/src/components/nle/useTimelineEditCallbacks.ts index f6e6f68f9..4669b99d0 100644 --- a/packages/studio/src/components/nle/useTimelineEditCallbacks.ts +++ b/packages/studio/src/components/nle/useTimelineEditCallbacks.ts @@ -171,12 +171,10 @@ export function useTimelineEditCallbacks({ ) => { const animation = animations.find((candidate) => candidate.id === animationId); if (animation && !animation.keyframes) { - if (selectionOverride === undefined) handleGsapDeleteAnimation(animationId); - else handleGsapDeleteAnimation(animationId, selectionOverride); + handleGsapDeleteAnimation(animationId, selectionOverride); return; } - if (selectionOverride === undefined) handleGsapRemoveKeyframe(animationId, percentage); - else handleGsapRemoveKeyframe(animationId, percentage, undefined, selectionOverride); + handleGsapRemoveKeyframe(animationId, percentage, undefined, selectionOverride); }, [handleGsapDeleteAnimation, handleGsapRemoveKeyframe], ); @@ -216,10 +214,25 @@ export function useTimelineEditCallbacks({ 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) => { - const target = resolveKeyframeTarget(keyframe, resolveElementAnimations(elId), elId); - if (target) handleGsapMoveKeyframeToPlayhead(target.animId, target.tweenPct); + const animations = resolveElementAnimations(elId); + 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 // the dragged keyframe's anim + tween-%. We convert the clip-% drop to an @@ -294,10 +307,19 @@ export function useTimelineEditCallbacks({ } return true; }, - onChangeKeyframeEase: (_elId: string, _pct: number, ease: string) => { - for (const anim of selectedGsapAnimations) { - if (anim.keyframes) handleGsapUpdateMeta(anim.id, { ease }); - } + onChangeKeyframeEase: (elId: string, _pct: number, ease: string) => { + // The edited element's own animations + selection, not the selection's: + // 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 onToggleKeyframeAtPlayhead: (el: TimelineElement) => { @@ -306,18 +328,34 @@ export function useTimelineEditCallbacks({ el.duration > 0 ? Math.max(0, Math.min(100, Math.round(((currentTime - el.start) / el.duration) * 100))) : 0; - const anim = selectedGsapAnimations.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); + // Same frame for read and write: the toggled element's animations decide + // add-vs-remove, and its selection is what the mutation commits through. + const animations = resolveElementAnimations(el.key ?? el.id); + void buildDomSelectionForTimelineElement(el).then((selection) => { + 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 { - 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) => { const selection = await buildDomSelectionForTimelineElement(element); diff --git a/packages/studio/src/hooks/useGsapSelectionHandlers.test.tsx b/packages/studio/src/hooks/useGsapSelectionHandlers.test.tsx index f1ae419ec..484adb94a 100644 --- a/packages/studio/src/hooks/useGsapSelectionHandlers.test.tsx +++ b/packages/studio/src/hooks/useGsapSelectionHandlers.test.tsx @@ -2,6 +2,7 @@ import { act } from "react"; import { createRoot } from "react-dom/client"; import { describe, expect, it, vi } from "vitest"; +import type { GsapAnimation } from "@hyperframes/core/gsap-parser"; import type { DomEditSelection } from "../components/editor/domEditingTypes"; import { useGsapSelectionHandlers } from "./useGsapSelectionHandlers"; @@ -114,3 +115,32 @@ describe("useGsapSelectionHandlers save failures", () => { 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(); + }); +}); diff --git a/packages/studio/src/hooks/useGsapSelectionHandlers.ts b/packages/studio/src/hooks/useGsapSelectionHandlers.ts index efde12caa..5a9dc1b68 100644 --- a/packages/studio/src/hooks/useGsapSelectionHandlers.ts +++ b/packages/studio/src/hooks/useGsapSelectionHandlers.ts @@ -118,6 +118,19 @@ export function useGsapSelectionHandlers({ const lastSelectionRef = useRef(null); 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( (error: unknown, selection: DomEditSelection, mutationType: string, label: string) => { trackStudioSaveFailure({ @@ -160,7 +173,7 @@ export function useGsapSelectionHandlers({ updates: { duration?: number; ease?: string; position?: number }, selectionOverride?: DomEditSelection | null, ) => { - const sel = selectionOverride ?? domEditSelection ?? lastSelectionRef.current; + const sel = resolveWriteSelection(selectionOverride); if (!sel) return; observeGsapMutation( updateGsapMeta(sel, animId, updates), @@ -169,16 +182,16 @@ export function useGsapSelectionHandlers({ "Edit GSAP animation", ); }, - [domEditSelection, observeGsapMutation, updateGsapMeta], + [resolveWriteSelection, observeGsapMutation, updateGsapMeta], ); const handleGsapDeleteAnimation = useCallback( (animId: string, selectionOverride?: DomEditSelection | null) => { - const sel = selectionOverride ?? domEditSelection ?? lastSelectionRef.current; + const sel = resolveWriteSelection(selectionOverride); if (!sel) return; observeGsapMutation(deleteGsapAnimation(sel, animId), sel, "delete", "Delete GSAP animation"); }, - [domEditSelection, deleteGsapAnimation, observeGsapMutation], + [resolveWriteSelection, deleteGsapAnimation, observeGsapMutation], ); const handleGsapDeleteAllForElement = useCallback( @@ -284,12 +297,12 @@ export function useGsapSelectionHandlers({ value: number | string, selectionOverride?: DomEditSelection | null, ) => { - const sel = selectionOverride ?? domEditSelection ?? lastSelectionRef.current; + const sel = resolveWriteSelection(selectionOverride); if (!sel) return; trackStudioEvent("keyframe", { action: "add", property }); addKeyframe(sel, animId, percentage, property, value); }, - [domEditSelection, addKeyframe], + [resolveWriteSelection, addKeyframe], ); const handleGsapAddKeyframeBatch = useCallback( @@ -300,7 +313,7 @@ export function useGsapSelectionHandlers({ commitOverrides?: Partial, selectionOverride?: DomEditSelection | null, ) => { - const sel = selectionOverride ?? domEditSelection ?? lastSelectionRef.current; + const sel = resolveWriteSelection(selectionOverride); if (!sel) return Promise.resolve(); return addKeyframeBatch(sel, animId, percentage, properties, commitOverrides).catch( (error) => { @@ -308,7 +321,7 @@ export function useGsapSelectionHandlers({ }, ); }, - [domEditSelection, addKeyframeBatch, trackGsapHandlerFailure], + [resolveWriteSelection, addKeyframeBatch, trackGsapHandlerFailure], ); const handleGsapRemoveKeyframe = useCallback( ( @@ -317,26 +330,34 @@ export function useGsapSelectionHandlers({ commitOverrides?: Partial, selectionOverride?: DomEditSelection | null, ) => { - const sel = selectionOverride ?? domEditSelection ?? lastSelectionRef.current; + const sel = resolveWriteSelection(selectionOverride); if (!sel) return; trackStudioEvent("keyframe", { action: "remove" }); removeKeyframe(sel, animId, percentage, commitOverrides); }, - [domEditSelection, removeKeyframe], + [resolveWriteSelection, removeKeyframe], ); 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; // Retime the keyframe to the playhead, preserving its value + ease. The - // playhead's tween-relative percentage is the move target. - const anim = selectedGsapAnimations.find((a) => a.id === animId); + // playhead's tween-relative percentage is the move target, and it has to + // 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); trackStudioEvent("keyframe", { action: "move_to_playhead" }); moveKeyframe(sel, animId, fromPercentage, toPercentage); }, - [domEditSelection, selectedGsapAnimations, moveKeyframe], + [resolveWriteSelection, selectedGsapAnimations, moveKeyframe], ); const handleGsapMoveKeyframe = useCallback( @@ -346,7 +367,7 @@ export function useGsapSelectionHandlers({ toPercentage: number, selectionOverride?: DomEditSelection | null, ) => { - const sel = selectionOverride ?? domEditSelection ?? lastSelectionRef.current; + const sel = resolveWriteSelection(selectionOverride); if (!sel) return; // Atomic retime: preserves the keyframe's value + per-keyframe ease. Both // percentages are tween-relative (the drag handler converts the drop @@ -355,7 +376,7 @@ export function useGsapSelectionHandlers({ trackStudioEvent("keyframe", { action: "retime" }); moveKeyframe(sel, animId, fromPercentage, toPercentage); }, - [domEditSelection, moveKeyframe], + [resolveWriteSelection, moveKeyframe], ); const handleGsapResizeKeyframedTween = useCallback( @@ -366,14 +387,14 @@ export function useGsapSelectionHandlers({ pctRemap: Array<{ from: number; to: number }>, selectionOverride?: DomEditSelection | null, ) => { - const sel = selectionOverride ?? domEditSelection ?? lastSelectionRef.current; + const sel = resolveWriteSelection(selectionOverride); if (!sel) return; // 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. trackStudioEvent("keyframe", { action: "retime_resize" }); resizeKeyframedTween(sel, animId, position, duration, pctRemap); }, - [domEditSelection, resizeKeyframedTween], + [resolveWriteSelection, resizeKeyframedTween], ); const handleGsapConvertToKeyframes = useCallback( @@ -382,24 +403,17 @@ export function useGsapSelectionHandlers({ resolvedFromValues?: Record, duration?: number, commitOverrides?: Partial, + selectionOverride?: DomEditSelection | null, ) => { - if (!domEditSelection) return Promise.resolve(); - return convertToKeyframes( - domEditSelection, - animId, - resolvedFromValues, - duration, - commitOverrides, - ).catch((error) => { - trackGsapHandlerFailure( - error, - domEditSelection, - "convert-to-keyframes", - "Convert to keyframes", - ); - }); + const sel = resolveWriteSelection(selectionOverride); + if (!sel) return Promise.resolve(); + return convertToKeyframes(sel, animId, resolvedFromValues, duration, commitOverrides).catch( + (error) => { + trackGsapHandlerFailure(error, sel, "convert-to-keyframes", "Convert to keyframes"); + }, + ); }, - [domEditSelection, convertToKeyframes, trackGsapHandlerFailure], + [resolveWriteSelection, convertToKeyframes, trackGsapHandlerFailure], ); const handleGsapRemoveAllKeyframes = useCallback(