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(