From 8f66b06d12e9deb25329e7aefe5054763df297b7 Mon Sep 17 00:00:00 2001 From: Miguel Angel Simon Sierra Date: Sun, 26 Jul 2026 01:14:36 +0200 Subject: [PATCH] fix(studio): retime the dragged element's own keyframe Drag-to-retime resolved the dragged diamond against the selected element's animations and committed through the selected element's DOM selection, so dragging a diamond on a non-selected clip retimed the wrong tween. It now resolves against the clicked element's animations and commits through that element's selection, matching the delete path. The three diamond callbacks also take the TimelineKeyframeTarget they already had instead of five positional fields, and the two copies of the sourceFile#domId split share splitTimelineElementKey. --- .../nle/useTimelineEditCallbacks.test.tsx | 121 ++++++++++++++---- .../nle/useTimelineEditCallbacks.ts | 75 ++++++----- .../components/TimelineClipDiamonds.tsx | 11 +- .../src/player/components/TimelineLanes.tsx | 3 +- .../player/components/TimelineOverlays.tsx | 4 +- .../player/components/timelineCallbacks.ts | 24 +--- .../hooks/useExpandedTimelineElements.ts | 9 +- .../src/player/lib/timelineElementHelpers.ts | 14 ++ 8 files changed, 166 insertions(+), 95 deletions(-) diff --git a/packages/studio/src/components/nle/useTimelineEditCallbacks.test.tsx b/packages/studio/src/components/nle/useTimelineEditCallbacks.test.tsx index cf240afa9..dec72f7b1 100644 --- a/packages/studio/src/components/nle/useTimelineEditCallbacks.test.tsx +++ b/packages/studio/src/components/nle/useTimelineEditCallbacks.test.tsx @@ -167,7 +167,16 @@ describe("useTimelineEditCallbacks — flat tween keyframe lanes", () => { const view = renderCallbacks(); act(() => { - view.callbacks.onMoveKeyframe?.("box", 0, 25, "position", 0, flatAnimation.id); + view.callbacks.onMoveKeyframe?.( + "box", + { + percentage: 0, + propertyGroup: "position", + tweenPercentage: 0, + animationId: flatAnimation.id, + }, + 25, + ); }); expect(mocks.actions.handleGsapMoveKeyframe).not.toHaveBeenCalled(); @@ -189,13 +198,12 @@ describe("useTimelineEditCallbacks — flat tween keyframe lanes", () => { const view = renderCallbacks(); await act(async () => { - view.callbacks.onDeleteKeyframe?.( - "scenes/main.html#circle", - 0, - "position", - 0, - otherFlatAnimation.id, - ); + view.callbacks.onDeleteKeyframe?.("scenes/main.html#circle", { + percentage: 0, + propertyGroup: "position", + tweenPercentage: 0, + animationId: otherFlatAnimation.id, + }); await Promise.resolve(); await Promise.resolve(); }); @@ -223,13 +231,12 @@ describe("useTimelineEditCallbacks — flat tween keyframe lanes", () => { const view = renderCallbacks(); await act(async () => { - view.callbacks.onDeleteKeyframe?.( - "scenes/main.html#circle", - 100, - "position", - 100, - otherKeyframedAnimation.id, - ); + view.callbacks.onDeleteKeyframe?.("scenes/main.html#circle", { + percentage: 100, + propertyGroup: "position", + tweenPercentage: 100, + animationId: otherKeyframedAnimation.id, + }); await Promise.resolve(); await Promise.resolve(); }); @@ -287,7 +294,7 @@ describe("useTimelineEditCallbacks — flat tween keyframe lanes", () => { const view = renderCallbacks(); act(() => { - view.callbacks.onMoveKeyframeToPlayhead?.("scenes/main.html#circle", 100); + view.callbacks.onMoveKeyframeToPlayhead?.("scenes/main.html#circle", { percentage: 100 }); }); expect(mocks.actions.handleGsapMoveKeyframeToPlayhead).toHaveBeenCalledWith( @@ -301,7 +308,12 @@ describe("useTimelineEditCallbacks — flat tween keyframe lanes", () => { const view = renderCallbacks(); act(() => { - view.callbacks.onDeleteKeyframe?.("box", 0, "position", 0, flatAnimation.id); + view.callbacks.onDeleteKeyframe?.("box", { + percentage: 0, + propertyGroup: "position", + tweenPercentage: 0, + animationId: flatAnimation.id, + }); }); expect(mocks.actions.handleGsapDeleteAnimation).toHaveBeenCalledWith(flatAnimation.id); @@ -371,7 +383,12 @@ describe("useTimelineEditCallbacks — flat tween keyframe lanes", () => { const view = renderCallbacks(); act(() => { - view.callbacks.onDeleteKeyframe?.("box", 50, "position", 50, flatAnimation.id); + view.callbacks.onDeleteKeyframe?.("box", { + percentage: 50, + propertyGroup: "position", + tweenPercentage: 50, + animationId: flatAnimation.id, + }); }); expect(mocks.actions.handleGsapRemoveKeyframe).toHaveBeenCalledWith(flatAnimation.id, 50); @@ -379,16 +396,74 @@ describe("useTimelineEditCallbacks — flat tween keyframe lanes", () => { view.unmount(); }); - it("keeps an authored interior drag on the per-keyframe move path", () => { - mocks.animations = [authoredInteriorAnimation()]; + it("keeps an authored interior drag on the per-keyframe move path", async () => { + const authored = authoredInteriorAnimation(); + mocks.animations = [authored]; + usePlayerStore.setState({ gsapAnimations: new Map([["box", [authored]]]) }); const view = renderCallbacks(); - act(() => { - view.callbacks.onMoveKeyframe?.("box", 50, 75, "position", 50, flatAnimation.id); + await act(async () => { + await view.callbacks.onMoveKeyframe?.( + "box", + { + percentage: 50, + propertyGroup: "position", + tweenPercentage: 50, + animationId: authored.id, + }, + 75, + ); }); - expect(mocks.actions.handleGsapMoveKeyframe).toHaveBeenCalledWith(flatAnimation.id, 50, 75); + expect(mocks.actions.handleGsapMoveKeyframe).toHaveBeenCalledWith( + authored.id, + 50, + 75, + mocks.selection, + ); expect(mocks.actions.handleGsapResizeKeyframedTween).not.toHaveBeenCalled(); view.unmount(); }); + + // A drag starts on whatever diamond the pointer is over, which need not be the + // selected element. Resolving against the selection would retime the selected + // element's tween and commit it through the selected element's file. + it("retimes a non-selected element's keyframe through that element's own selection", 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" }; + const circleAnimation = { ...authoredInteriorAnimation(), id: "circle-to-0-position" }; + usePlayerStore.setState({ + elements: [element, circle], + gsapAnimations: new Map([["scenes/main.html#circle", [circleAnimation]]]), + }); + mocks.actions.buildDomSelectionForTimelineElement.mockResolvedValue(circleSelection); + const view = renderCallbacks(); + + await act(async () => { + await view.callbacks.onMoveKeyframe?.( + "scenes/main.html#circle", + { + percentage: 50, + propertyGroup: "position", + tweenPercentage: 50, + animationId: circleAnimation.id, + }, + 75, + ); + }); + + expect(mocks.actions.handleGsapMoveKeyframe).toHaveBeenCalledWith( + circleAnimation.id, + 50, + 75, + circleSelection, + ); + view.unmount(); + }); }); diff --git a/packages/studio/src/components/nle/useTimelineEditCallbacks.ts b/packages/studio/src/components/nle/useTimelineEditCallbacks.ts index 0ac9a3274..fa80561bb 100644 --- a/packages/studio/src/components/nle/useTimelineEditCallbacks.ts +++ b/packages/studio/src/components/nle/useTimelineEditCallbacks.ts @@ -14,6 +14,8 @@ import { resolveClipTimingBasis } from "../../hooks/useGsapTweenCache"; import { resolveKeyframeRetime } from "../editor/keyframeRetime"; import type { DomEditSelection } from "../editor/domEditingTypes"; import type { TimelineMoveOperation } from "../../hooks/timelineMoveAdapter"; +import { splitTimelineElementKey } from "../../player/lib/timelineElementHelpers"; +import type { TimelineKeyframeTarget } from "../../player/components/timelineKeyframeIdentity"; export interface TimelineEditCallbackDeps { handleTimelineElementMove: ( @@ -117,14 +119,12 @@ export function useTimelineEditCallbacks({ const resolveElementAnimations = useCallback( (elementKey: string): GsapAnimation[] => { const { gsapAnimations } = usePlayerStore.getState(); - const hashIndex = elementKey.lastIndexOf("#"); - const elementId = hashIndex === -1 ? elementKey : elementKey.slice(hashIndex + 1); - const sourceFile = - hashIndex === -1 ? (activeCompPath ?? "index.html") : elementKey.slice(0, hashIndex); + const { sourceFile, domId } = splitTimelineElementKey(elementKey); + const scope = sourceFile ?? activeCompPath ?? "index.html"; return ( - gsapAnimations.get(`${sourceFile}#${elementId}`) ?? - gsapAnimations.get(`index.html#${elementId}`) ?? - gsapAnimations.get(elementId) ?? + gsapAnimations.get(`${scope}#${domId}`) ?? + gsapAnimations.get(`index.html#${domId}`) ?? + gsapAnimations.get(domId) ?? [] ); }, @@ -136,19 +136,15 @@ export function useTimelineEditCallbacks({ // diamond reports a clip-% but the script ops key on the tween-%. Prefers the // anim in the keyframe's property group, falling back to the first keyframed one. const resolveKeyframeTarget = useCallback( - // fallow-ignore-next-line complexity ( - pct: number, - propertyGroup?: string, - tweenPercentage?: number, - animationId?: string, + target: TimelineKeyframeTarget, animations: GsapAnimation[] = selectedGsapAnimations, elementKey?: string, ): { animId: string; tweenPct: number } | null => { - const explicitTarget = - propertyGroup !== undefined || tweenPercentage !== undefined || animationId !== undefined - ? [{ percentage: pct, propertyGroup, tweenPercentage, animationId }] - : undefined; + const carriesIdentity = + target.propertyGroup !== undefined || + target.tweenPercentage !== undefined || + target.animationId !== undefined; // The clicked element's own cache when the caller knows it: the diamond // context menu can open on an element that is not the selected one, and // reading the selection's cache there resolves against the wrong element. @@ -156,8 +152,8 @@ export function useTimelineEditCallbacks({ .getState() .keyframeCache.get(elementKey ?? domEditSelection?.id ?? ""); return resolveTimelineKeyframeTarget( - pct, - explicitTarget ?? cached?.keyframes ?? [], + target.percentage, + carriesIdentity ? [target] : (cached?.keyframes ?? []), animations, ); }, @@ -202,9 +198,9 @@ export function useTimelineEditCallbacks({ if (!anim) return; handleGsapRemoveAllKeyframes(anim.id); }, - onDeleteKeyframe: (elId, pct, group, tweenPct, animationId) => { + onDeleteKeyframe: (elId, keyframe) => { const animations = resolveElementAnimations(elId); - const target = resolveKeyframeTarget(pct, group, tweenPct, animationId, animations, elId); + const target = resolveKeyframeTarget(keyframe, animations, elId); if (!target) return; const element = usePlayerStore.getState().elements.find((el) => (el.key ?? el.id) === elId); if (!element) { @@ -219,15 +215,8 @@ export function useTimelineEditCallbacks({ }); }, // Retime the keyframe to the playhead, preserving its value + ease. - onMoveKeyframeToPlayhead: (elId, pct, group, tweenPct, animationId) => { - const target = resolveKeyframeTarget( - pct, - group, - tweenPct, - animationId, - resolveElementAnimations(elId), - elId, - ); + onMoveKeyframeToPlayhead: (elId, keyframe) => { + const target = resolveKeyframeTarget(keyframe, resolveElementAnimations(elId), elId); if (target) handleGsapMoveKeyframeToPlayhead(target.animId, target.tweenPct); }, // Drag-to-retime. The diamond reports clip-%s; resolveKeyframeTarget gives @@ -238,11 +227,17 @@ export function useTimelineEditCallbacks({ // resizes the tween — position/duration grow so the dragged keyframe lands at // the drop while every other keyframe keeps its absolute time (value+ease too). // fallow-ignore-next-line complexity - onMoveKeyframe: async (_elId, fromClipPct, toClipPct, group, tweenPct, animationId) => { - const target = resolveKeyframeTarget(fromClipPct, group, tweenPct, animationId); - const sel = domEditSelection; - if (!target || !sel) return false; - const anim = selectedGsapAnimations.find((a) => a.id === target.animId); + onMoveKeyframe: async (elId, keyframe, toClipPct) => { + const animations = resolveElementAnimations(elId); + const target = resolveKeyframeTarget(keyframe, animations, elId); + if (!target) return false; + // The dragged diamond's OWN element, not the selected one: a drag on a + // non-selected clip has to read that clip's animations and commit + // through that clip's selection, or it retimes whatever is selected. + const element = usePlayerStore.getState().elements.find((el) => (el.key ?? el.id) === elId); + const sel = element ? await buildDomSelectionForTimelineElement(element) : domEditSelection; + if (!sel) return false; + const anim = animations.find((a) => a.id === target.animId); const tweenStart = anim ? resolveTweenStart(anim) : null; if (!anim || tweenStart === null) return false; // Synthesized flat endpoints are clip boundaries, not authored keyframes. @@ -267,7 +262,7 @@ export function useTimelineEditCallbacks({ dropAbsTime, }); if (decision.kind === "move" && decision.toTweenPct != null) { - handleGsapMoveKeyframe(target.animId, target.tweenPct, decision.toTweenPct); + handleGsapMoveKeyframe(target.animId, target.tweenPct, decision.toTweenPct, sel); } else if ( decision.kind === "resize" && decision.pctRemap && @@ -280,15 +275,17 @@ export function useTimelineEditCallbacks({ decision.position, decision.duration, decision.pctRemap, + sel, ); } else { // resize-keyframed-tween requires an authored `keyframes` AST node // and intentionally no-ops for a flat tween. Update its real tween // window through the metadata writer (and SDK cutover path) instead. - handleGsapUpdateMeta(target.animId, { - position: decision.position, - duration: decision.duration, - }); + handleGsapUpdateMeta( + target.animId, + { position: decision.position, duration: decision.duration }, + sel, + ); } } else { return false; diff --git a/packages/studio/src/player/components/TimelineClipDiamonds.tsx b/packages/studio/src/player/components/TimelineClipDiamonds.tsx index 1a066d5d0..09320bdc5 100644 --- a/packages/studio/src/player/components/TimelineClipDiamonds.tsx +++ b/packages/studio/src/player/components/TimelineClipDiamonds.tsx @@ -48,12 +48,13 @@ interface TimelineClipDiamondsProps { onShiftClickKeyframe?: (elementId: string, percentage: number) => void; onContextMenuKeyframe?: (e: React.MouseEvent, elementId: string, percentage: number) => void; /** Drag-to-retime: move a keyframe to a new time, preserving its value + ease. - * Both percentages are clip-relative: `fromClipPercentage` identifies the - * dragged keyframe, `toClipPercentage` is the neighbour-clamped drop position. - * The handler decides move (within the tween) vs resize (past its boundary). */ + * `keyframe` identifies the dragged keyframe (clip-relative percentage plus + * whatever animation identity the row carries); `toClipPercentage` is the + * neighbour-clamped drop position, also clip-relative. The handler decides + * move (within the tween) vs resize (past its boundary). */ onMoveKeyframe?: ( elementId: string, - fromClipPercentage: number, + keyframe: TimelineKeyframeTarget, toClipPercentage: number, ) => Promise; /** Open the segment ease editor for the hovered mid-point button — available on @@ -549,7 +550,7 @@ export const TimelineClipDiamonds = memo(function TimelineClipDiamonds( onMoveKeyframe={ props.onMoveKeyframe ? (target, toClipPercentage) => - props.onMoveKeyframe?.(props.elementId, target.percentage, toClipPercentage) ?? + props.onMoveKeyframe?.(props.elementId, target, toClipPercentage) ?? Promise.resolve(false) : undefined } diff --git a/packages/studio/src/player/components/TimelineLanes.tsx b/packages/studio/src/player/components/TimelineLanes.tsx index 13707c05c..8437bb64e 100644 --- a/packages/studio/src/player/components/TimelineLanes.tsx +++ b/packages/studio/src/player/components/TimelineLanes.tsx @@ -3,6 +3,7 @@ import { Eye, EyeSlash } from "@phosphor-icons/react"; import { BeatStrip, BeatBackgroundLines } from "./BeatStrip"; import { TimelineClip } from "./TimelineClip"; import { TimelineClipDiamonds } from "./TimelineClipDiamonds"; +import type { TimelineKeyframeTarget } from "./timelineKeyframeIdentity"; import type { MusicBeatAnalysis } from "@hyperframes/core/beats"; import { getTimelineEditCapabilities, resolveBlockedTimelineEditIntent } from "./timelineEditing"; import type { TimelineTheme } from "./timelineTheme"; @@ -76,7 +77,7 @@ export interface TimelineLaneBaseProps { onContextMenuKeyframe?: (e: React.MouseEvent, elementId: string, percentage: number) => void; onMoveKeyframe?: ( elementId: string, - fromClipPercentage: number, + keyframe: TimelineKeyframeTarget, toClipPercentage: number, ) => Promise; onContextMenuClip?: (e: React.MouseEvent, element: TimelineElement) => void; diff --git a/packages/studio/src/player/components/TimelineOverlays.tsx b/packages/studio/src/player/components/TimelineOverlays.tsx index bd0896e0b..44e312074 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, pct)} + onDelete={(elId, pct) => onDeleteKeyframe?.(elId, { percentage: pct })} onDeleteAll={(elId) => onDeleteAllKeyframes?.(elId)} onChangeEase={(elId, pct, ease) => onChangeKeyframeEase?.(elId, pct, ease)} onMoveToPlayhead={ onMoveKeyframeToPlayhead - ? (elId, pct) => onMoveKeyframeToPlayhead(elId, pct) + ? (elId, pct) => onMoveKeyframeToPlayhead(elId, { percentage: pct }) : undefined } onCopyProperties={(elId, pct) => { diff --git a/packages/studio/src/player/components/timelineCallbacks.ts b/packages/studio/src/player/components/timelineCallbacks.ts index ced618e71..204302e4b 100644 --- a/packages/studio/src/player/components/timelineCallbacks.ts +++ b/packages/studio/src/player/components/timelineCallbacks.ts @@ -4,6 +4,7 @@ import type { TimelineElement } from "../store/playerStore"; import type { TimelineMoveOperation } from "../../hooks/timelineMoveAdapter"; import type { BlockedTimelineEditIntent } from "./timelineEditing"; import type { PropertyGroupName } from "@hyperframes/core/gsap-parser"; +import type { TimelineKeyframeTarget } from "./timelineKeyframeIdentity"; export interface TimelinePropertyGroupKeyframeToggle { animationId: string; @@ -71,29 +72,16 @@ export interface TimelineEditCallbacks { onSplitElement?: (element: TimelineElement, splitTime: number) => Promise | void; onRazorSplit?: (element: TimelineElement, splitTime: number) => Promise | void; onRazorSplitAll?: (splitTime: number) => Promise | void; - onDeleteKeyframe?: ( - elementId: string, - percentage: number, - propertyGroup?: string, - tweenPercentage?: number, - animationId?: string, - ) => void; + onDeleteKeyframe?: (elementId: string, keyframe: TimelineKeyframeTarget) => void; onDeleteAllKeyframes?: (elementId: string) => void; onChangeKeyframeEase?: (elementId: string, percentage: number, ease: string) => void; - onMoveKeyframeToPlayhead?: ( - elementId: string, - percentage: number, - propertyGroup?: string, - tweenPercentage?: number, - animationId?: string, - ) => void; + onMoveKeyframeToPlayhead?: (elementId: string, keyframe: TimelineKeyframeTarget) => void; + /** Drag-to-retime: `keyframe` identifies the dragged keyframe (its percentage + * is clip-relative), `toClipPercentage` is the neighbour-clamped drop. */ onMoveKeyframe?: ( elementId: string, - fromClipPercentage: number, + keyframe: TimelineKeyframeTarget, toClipPercentage: number, - propertyGroup?: string, - tweenPercentage?: number, - animationId?: string, ) => Promise; onToggleKeyframeAtPlayhead?: (element: TimelineElement) => void; onTogglePropertyGroupKeyframe?: ( diff --git a/packages/studio/src/player/hooks/useExpandedTimelineElements.ts b/packages/studio/src/player/hooks/useExpandedTimelineElements.ts index a5e31e71b..d0ccaa268 100644 --- a/packages/studio/src/player/hooks/useExpandedTimelineElements.ts +++ b/packages/studio/src/player/hooks/useExpandedTimelineElements.ts @@ -2,7 +2,7 @@ import { useMemo } from "react"; import { usePlayerStore, type TimelineElement, type DomClipChild } from "../store/playerStore"; import type { ClipManifestClip } from "../lib/playbackTypes"; import { createTimelineElementFromManifestClip } from "../lib/timelineDOM"; -import { buildTimelineElementKey } from "../lib/timelineElementHelpers"; +import { buildTimelineElementKey, splitTimelineElementKey } from "../lib/timelineElementHelpers"; function findTopLevelAncestor(id: string, parentMap: Map): string | null { let current = parentMap.get(id); @@ -19,18 +19,13 @@ function findTopLevelAncestor(id: string, parentMap: Map): strin return current; } -function extractDomId(key: string): string { - const hashIdx = key.lastIndexOf("#"); - return hashIdx >= 0 ? key.slice(hashIdx + 1) : key; -} - function resolveRawId( selectedId: string | null, manifest: ClipManifestClip[], parentMap: Map, ): string | null { if (!selectedId) return null; - const rawId = extractDomId(selectedId); + const rawId = splitTimelineElementKey(selectedId).domId; if (parentMap.has(rawId)) return rawId; if (parentMap.has(selectedId)) return selectedId; const clip = manifest.find((c) => c.label === selectedId || c.label === rawId); diff --git a/packages/studio/src/player/lib/timelineElementHelpers.ts b/packages/studio/src/player/lib/timelineElementHelpers.ts index b3d6482ae..323248291 100644 --- a/packages/studio/src/player/lib/timelineElementHelpers.ts +++ b/packages/studio/src/player/lib/timelineElementHelpers.ts @@ -298,6 +298,20 @@ export function buildTimelineElementKey(params: { return `${scope}:${params.id}:${params.fallbackIndex}`; } +/** + * Inverse of {@link buildTimelineElementKey} for the `sourceFile#domId` form. + * A key with no `#` is a bare dom id and carries no source file of its own, so + * the caller supplies the scope it wants to look that id up in. + */ +export function splitTimelineElementKey(key: string): { + sourceFile: string | null; + domId: string; +} { + const hashIndex = key.lastIndexOf("#"); + if (hashIndex < 0) return { sourceFile: null, domId: key }; + return { sourceFile: key.slice(0, hashIndex), domId: key.slice(hashIndex + 1) }; +} + export function buildTimelineElementIdentity(params: { preferredId?: string | null; label: string;