From eddc5e9224fc7236fdf6776c9cbafaff3d84b13b Mon Sep 17 00:00:00 2001 From: Miguel Angel Simon Sierra Date: Tue, 28 Jul 2026 00:25:41 +0200 Subject: [PATCH] fix(studio): pass the whole keyframe identity out of the diamond menu The context menu handed its actions loose positional arguments, so the overlay adapters forwarded the percentage alone and dropped the property group, tween percentage, and animation id the menu had resolved. The target then fell back to first-match-by-percentage and deleted or retimed the wrong animation whenever two collide at one percentage. Every action now carries a TimelineKeyframeTarget, which leaves no adapter shape that can drop it. --- .../components/editor/MotionPathOverlay.tsx | 8 ++- .../KeyframeDiamondContextMenu.test.tsx | 69 +++++++++++++++++++ .../components/KeyframeDiamondContextMenu.tsx | 43 ++++-------- .../player/components/TimelineOverlays.tsx | 4 +- 4 files changed, 92 insertions(+), 32 deletions(-) create mode 100644 packages/studio/src/player/components/KeyframeDiamondContextMenu.test.tsx diff --git a/packages/studio/src/components/editor/MotionPathOverlay.tsx b/packages/studio/src/components/editor/MotionPathOverlay.tsx index 81a10a8d1..8af9f3129 100644 --- a/packages/studio/src/components/editor/MotionPathOverlay.tsx +++ b/packages/studio/src/components/editor/MotionPathOverlay.tsx @@ -520,9 +520,13 @@ export const MotionPathOverlay = memo(function MotionPathOverlay({ setKfMenu(null)} - onDelete={(_elId, pct) => animId && handleGsapRemoveKeyframe(animId, pct)} + onDelete={(_elId, target) => + animId && handleGsapRemoveKeyframe(animId, target.percentage) + } onDeleteAll={() => animId && handleGsapRemoveAllKeyframes(animId)} - onMoveToPlayhead={(_elId, pct) => animId && handleGsapMoveKeyframeToPlayhead(animId, pct)} + onMoveToPlayhead={(_element, target) => + animId && handleGsapMoveKeyframeToPlayhead(animId, target.percentage) + } /> )} diff --git a/packages/studio/src/player/components/KeyframeDiamondContextMenu.test.tsx b/packages/studio/src/player/components/KeyframeDiamondContextMenu.test.tsx new file mode 100644 index 000000000..f802da680 --- /dev/null +++ b/packages/studio/src/player/components/KeyframeDiamondContextMenu.test.tsx @@ -0,0 +1,69 @@ +// @vitest-environment happy-dom +import { act } from "react"; +import { createRoot } from "react-dom/client"; +import { describe, expect, it, vi } from "vitest"; +import type { TimelineElement } from "../store/playerStore"; +import { + KeyframeDiamondContextMenu, + type KeyframeDiamondContextMenuState, +} from "./KeyframeDiamondContextMenu"; + +(globalThis as unknown as { IS_REACT_ACT_ENVIRONMENT: boolean }).IS_REACT_ACT_ENVIRONMENT = true; + +const element = { id: "box", start: 0, duration: 2, track: 0 } as unknown as TimelineElement; + +const state: KeyframeDiamondContextMenuState = { + x: 10, + y: 10, + element, + elementId: "box", + percentage: 50, + tweenPercentage: 25, + propertyGroup: "position", + animationId: "box-to-1-position", +}; + +function clickMenuItem(label: string, props: Partial>) { + const host = document.createElement("div"); + document.body.appendChild(host); + const root = createRoot(host); + act(() => + root.render( + {}} + onDelete={vi.fn()} + onDeleteAll={vi.fn()} + {...props} + />, + ), + ); + const button = Array.from(document.body.querySelectorAll("button")).find( + (candidate) => candidate.textContent === label, + ); + act(() => button?.click()); + act(() => root.unmount()); + host.remove(); +} + +describe("KeyframeDiamondContextMenu", () => { + // Two animations can carry a keyframe at the same clip percentage. Dropping the + // property group / tween percentage / animation id here sends the mutation back + // to first-match-by-percentage, which retimes or deletes the wrong tween. + it("hands every action the full keyframe identity, not just the percentage", () => { + const onDelete = vi.fn(); + const onMoveToPlayhead = vi.fn(); + + clickMenuItem("Delete Keyframe", { onDelete }); + clickMenuItem("Move to Playhead", { onMoveToPlayhead }); + + const target = { + percentage: 50, + tweenPercentage: 25, + propertyGroup: "position", + animationId: "box-to-1-position", + }; + expect(onDelete).toHaveBeenCalledWith("box", target); + expect(onMoveToPlayhead).toHaveBeenCalledWith(element, target); + }); +}); diff --git a/packages/studio/src/player/components/KeyframeDiamondContextMenu.tsx b/packages/studio/src/player/components/KeyframeDiamondContextMenu.tsx index 1e8275511..9dc70df9d 100644 --- a/packages/studio/src/player/components/KeyframeDiamondContextMenu.tsx +++ b/packages/studio/src/player/components/KeyframeDiamondContextMenu.tsx @@ -2,6 +2,7 @@ import { memo } from "react"; import { createPortal } from "react-dom"; import { useContextMenuDismiss } from "../../hooks/useContextMenuDismiss"; import type { TimelineElement } from "../store/playerStore"; +import type { TimelineKeyframeTarget } from "./timelineKeyframeIdentity"; export interface KeyframeDiamondContextMenuState { x: number; @@ -18,24 +19,12 @@ export interface KeyframeDiamondContextMenuState { interface KeyframeDiamondContextMenuProps { state: KeyframeDiamondContextMenuState; onClose: () => void; - onDelete: ( - elementId: string, - percentage: number, - propertyGroup?: string, - tweenPercentage?: number, - animationId?: string, - ) => void; + onDelete: (elementId: string, target: TimelineKeyframeTarget) => void; onDeleteAll: (element: TimelineElement) => void; onChangeEase?: (elementId: string, percentage: number, ease: string) => void; onCopyProperties?: (elementId: string, percentage: number) => void; /** Retime the keyframe to the current playhead, preserving its value + ease. */ - onMoveToPlayhead?: ( - element: TimelineElement, - fromPercentage: number, - propertyGroup?: string, - tweenPercentage?: number, - animationId?: string, - ) => void; + onMoveToPlayhead?: (element: TimelineElement, target: TimelineKeyframeTarget) => void; } export const KeyframeDiamondContextMenu = memo(function KeyframeDiamondContextMenu({ @@ -46,6 +35,16 @@ export const KeyframeDiamondContextMenu = memo(function KeyframeDiamondContextMe onMoveToPlayhead, }: KeyframeDiamondContextMenuProps) { const menuRef = useContextMenuDismiss(onClose); + // One target object for every action: passing the identity as loose positional + // arguments let an adapter forward the percentage alone, which drops the menu + // back to first-match-by-percentage and picks the wrong animation whenever two + // collide at the same percentage. + const target: TimelineKeyframeTarget = { + percentage: state.percentage, + tweenPercentage: state.tweenPercentage, + propertyGroup: state.propertyGroup, + animationId: state.animationId, + }; const menuWidth = 200; const menuHeight = onMoveToPlayhead ? 100 : 70; @@ -67,13 +66,7 @@ export const KeyframeDiamondContextMenu = memo(function KeyframeDiamondContextMe // Pass clip-% — resolveKeyframeTarget keys the cache lookup on clip-% // and returns the tween-% for the mutation. Passing tween-% here would // miss the lookup on any tween whose window is shorter than the clip. - onMoveToPlayhead( - state.element, - state.percentage, - state.propertyGroup, - state.tweenPercentage, - state.animationId, - ); + onMoveToPlayhead(state.element, target); onClose(); }} > @@ -86,13 +79,7 @@ export const KeyframeDiamondContextMenu = memo(function KeyframeDiamondContextMe type="button" className="w-full flex items-center gap-2 px-3 py-1.5 text-xs text-red-400 hover:bg-neutral-800 cursor-pointer text-left" onClick={() => { - onDelete( - state.elementId, - state.percentage, - state.propertyGroup, - state.tweenPercentage, - state.animationId, - ); + onDelete(state.elementId, target); onClose(); }} > diff --git a/packages/studio/src/player/components/TimelineOverlays.tsx b/packages/studio/src/player/components/TimelineOverlays.tsx index 44e312074..cfc2527b4 100644 --- a/packages/studio/src/player/components/TimelineOverlays.tsx +++ b/packages/studio/src/player/components/TimelineOverlays.tsx @@ -106,12 +106,12 @@ export function TimelineOverlays({ setKfContextMenu(null)} - onDelete={(elId, pct) => onDeleteKeyframe?.(elId, { percentage: pct })} + onDelete={(elId, target) => onDeleteKeyframe?.(elId, target)} onDeleteAll={(elId) => onDeleteAllKeyframes?.(elId)} onChangeEase={(elId, pct, ease) => onChangeKeyframeEase?.(elId, pct, ease)} onMoveToPlayhead={ onMoveKeyframeToPlayhead - ? (elId, pct) => onMoveKeyframeToPlayhead(elId, { percentage: pct }) + ? (element, target) => onMoveKeyframeToPlayhead(element, target) : undefined } onCopyProperties={(elId, pct) => {