Skip to content

Commit 783fa4a

Browse files
miguel-heygenmiguel-heygen
committed
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.
1 parent 614bb6a commit 783fa4a

8 files changed

Lines changed: 166 additions & 95 deletions

File tree

‎packages/studio/src/components/nle/useTimelineEditCallbacks.test.tsx‎

Lines changed: 98 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -167,7 +167,16 @@ describe("useTimelineEditCallbacks — flat tween keyframe lanes", () => {
167167
const view = renderCallbacks();
168168

169169
act(() => {
170-
view.callbacks.onMoveKeyframe?.("box", 0, 25, "position", 0, flatAnimation.id);
170+
view.callbacks.onMoveKeyframe?.(
171+
"box",
172+
{
173+
percentage: 0,
174+
propertyGroup: "position",
175+
tweenPercentage: 0,
176+
animationId: flatAnimation.id,
177+
},
178+
25,
179+
);
171180
});
172181

173182
expect(mocks.actions.handleGsapMoveKeyframe).not.toHaveBeenCalled();
@@ -189,13 +198,12 @@ describe("useTimelineEditCallbacks — flat tween keyframe lanes", () => {
189198
const view = renderCallbacks();
190199

191200
await act(async () => {
192-
view.callbacks.onDeleteKeyframe?.(
193-
"scenes/main.html#circle",
194-
0,
195-
"position",
196-
0,
197-
otherFlatAnimation.id,
198-
);
201+
view.callbacks.onDeleteKeyframe?.("scenes/main.html#circle", {
202+
percentage: 0,
203+
propertyGroup: "position",
204+
tweenPercentage: 0,
205+
animationId: otherFlatAnimation.id,
206+
});
199207
await Promise.resolve();
200208
await Promise.resolve();
201209
});
@@ -223,13 +231,12 @@ describe("useTimelineEditCallbacks — flat tween keyframe lanes", () => {
223231
const view = renderCallbacks();
224232

225233
await act(async () => {
226-
view.callbacks.onDeleteKeyframe?.(
227-
"scenes/main.html#circle",
228-
100,
229-
"position",
230-
100,
231-
otherKeyframedAnimation.id,
232-
);
234+
view.callbacks.onDeleteKeyframe?.("scenes/main.html#circle", {
235+
percentage: 100,
236+
propertyGroup: "position",
237+
tweenPercentage: 100,
238+
animationId: otherKeyframedAnimation.id,
239+
});
233240
await Promise.resolve();
234241
await Promise.resolve();
235242
});
@@ -287,7 +294,7 @@ describe("useTimelineEditCallbacks — flat tween keyframe lanes", () => {
287294
const view = renderCallbacks();
288295

289296
act(() => {
290-
view.callbacks.onMoveKeyframeToPlayhead?.("scenes/main.html#circle", 100);
297+
view.callbacks.onMoveKeyframeToPlayhead?.("scenes/main.html#circle", { percentage: 100 });
291298
});
292299

293300
expect(mocks.actions.handleGsapMoveKeyframeToPlayhead).toHaveBeenCalledWith(
@@ -301,7 +308,12 @@ describe("useTimelineEditCallbacks — flat tween keyframe lanes", () => {
301308
const view = renderCallbacks();
302309

303310
act(() => {
304-
view.callbacks.onDeleteKeyframe?.("box", 0, "position", 0, flatAnimation.id);
311+
view.callbacks.onDeleteKeyframe?.("box", {
312+
percentage: 0,
313+
propertyGroup: "position",
314+
tweenPercentage: 0,
315+
animationId: flatAnimation.id,
316+
});
305317
});
306318

307319
expect(mocks.actions.handleGsapDeleteAnimation).toHaveBeenCalledWith(flatAnimation.id);
@@ -371,24 +383,87 @@ describe("useTimelineEditCallbacks — flat tween keyframe lanes", () => {
371383
const view = renderCallbacks();
372384

373385
act(() => {
374-
view.callbacks.onDeleteKeyframe?.("box", 50, "position", 50, flatAnimation.id);
386+
view.callbacks.onDeleteKeyframe?.("box", {
387+
percentage: 50,
388+
propertyGroup: "position",
389+
tweenPercentage: 50,
390+
animationId: flatAnimation.id,
391+
});
375392
});
376393

377394
expect(mocks.actions.handleGsapRemoveKeyframe).toHaveBeenCalledWith(flatAnimation.id, 50);
378395
expect(mocks.actions.handleGsapDeleteAnimation).not.toHaveBeenCalled();
379396
view.unmount();
380397
});
381398

382-
it("keeps an authored interior drag on the per-keyframe move path", () => {
383-
mocks.animations = [authoredInteriorAnimation()];
399+
it("keeps an authored interior drag on the per-keyframe move path", async () => {
400+
const authored = authoredInteriorAnimation();
401+
mocks.animations = [authored];
402+
usePlayerStore.setState({ gsapAnimations: new Map([["box", [authored]]]) });
384403
const view = renderCallbacks();
385404

386-
act(() => {
387-
view.callbacks.onMoveKeyframe?.("box", 50, 75, "position", 50, flatAnimation.id);
405+
await act(async () => {
406+
await view.callbacks.onMoveKeyframe?.(
407+
"box",
408+
{
409+
percentage: 50,
410+
propertyGroup: "position",
411+
tweenPercentage: 50,
412+
animationId: authored.id,
413+
},
414+
75,
415+
);
388416
});
389417

390-
expect(mocks.actions.handleGsapMoveKeyframe).toHaveBeenCalledWith(flatAnimation.id, 50, 75);
418+
expect(mocks.actions.handleGsapMoveKeyframe).toHaveBeenCalledWith(
419+
authored.id,
420+
50,
421+
75,
422+
mocks.selection,
423+
);
391424
expect(mocks.actions.handleGsapResizeKeyframedTween).not.toHaveBeenCalled();
392425
view.unmount();
393426
});
427+
428+
// A drag starts on whatever diamond the pointer is over, which need not be the
429+
// selected element. Resolving against the selection would retime the selected
430+
// element's tween and commit it through the selected element's file.
431+
it("retimes a non-selected element's keyframe through that element's own selection", async () => {
432+
const circle: TimelineElement = {
433+
...element,
434+
id: "circle",
435+
key: "scenes/main.html#circle",
436+
domId: "circle",
437+
sourceFile: "scenes/main.html",
438+
};
439+
const circleSelection = { id: "circle", selector: "#circle", sourceFile: "scenes/main.html" };
440+
const circleAnimation = { ...authoredInteriorAnimation(), id: "circle-to-0-position" };
441+
usePlayerStore.setState({
442+
elements: [element, circle],
443+
gsapAnimations: new Map([["scenes/main.html#circle", [circleAnimation]]]),
444+
});
445+
mocks.actions.buildDomSelectionForTimelineElement.mockResolvedValue(circleSelection);
446+
const view = renderCallbacks();
447+
448+
await act(async () => {
449+
await view.callbacks.onMoveKeyframe?.(
450+
"scenes/main.html#circle",
451+
{
452+
percentage: 50,
453+
propertyGroup: "position",
454+
tweenPercentage: 50,
455+
animationId: circleAnimation.id,
456+
},
457+
75,
458+
);
459+
});
460+
461+
expect(mocks.actions.handleGsapMoveKeyframe).toHaveBeenCalledWith(
462+
circleAnimation.id,
463+
50,
464+
75,
465+
circleSelection,
466+
);
467+
view.unmount();
468+
});
394469
});

‎packages/studio/src/components/nle/useTimelineEditCallbacks.ts‎

Lines changed: 36 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,8 @@ import { resolveClipTimingBasis } from "../../hooks/useGsapTweenCache";
1414
import { resolveKeyframeRetime } from "../editor/keyframeRetime";
1515
import type { DomEditSelection } from "../editor/domEditingTypes";
1616
import type { TimelineMoveOperation } from "../../hooks/timelineMoveAdapter";
17+
import { splitTimelineElementKey } from "../../player/lib/timelineElementHelpers";
18+
import type { TimelineKeyframeTarget } from "../../player/components/timelineKeyframeIdentity";
1719

1820
export interface TimelineEditCallbackDeps {
1921
handleTimelineElementMove: (
@@ -117,14 +119,12 @@ export function useTimelineEditCallbacks({
117119
const resolveElementAnimations = useCallback(
118120
(elementKey: string): GsapAnimation[] => {
119121
const { gsapAnimations } = usePlayerStore.getState();
120-
const hashIndex = elementKey.lastIndexOf("#");
121-
const elementId = hashIndex === -1 ? elementKey : elementKey.slice(hashIndex + 1);
122-
const sourceFile =
123-
hashIndex === -1 ? (activeCompPath ?? "index.html") : elementKey.slice(0, hashIndex);
122+
const { sourceFile, domId } = splitTimelineElementKey(elementKey);
123+
const scope = sourceFile ?? activeCompPath ?? "index.html";
124124
return (
125-
gsapAnimations.get(`${sourceFile}#${elementId}`) ??
126-
gsapAnimations.get(`index.html#${elementId}`) ??
127-
gsapAnimations.get(elementId) ??
125+
gsapAnimations.get(`${scope}#${domId}`) ??
126+
gsapAnimations.get(`index.html#${domId}`) ??
127+
gsapAnimations.get(domId) ??
128128
[]
129129
);
130130
},
@@ -136,28 +136,24 @@ export function useTimelineEditCallbacks({
136136
// diamond reports a clip-% but the script ops key on the tween-%. Prefers the
137137
// anim in the keyframe's property group, falling back to the first keyframed one.
138138
const resolveKeyframeTarget = useCallback(
139-
// fallow-ignore-next-line complexity
140139
(
141-
pct: number,
142-
propertyGroup?: string,
143-
tweenPercentage?: number,
144-
animationId?: string,
140+
target: TimelineKeyframeTarget,
145141
animations: GsapAnimation[] = selectedGsapAnimations,
146142
elementKey?: string,
147143
): { animId: string; tweenPct: number } | null => {
148-
const explicitTarget =
149-
propertyGroup !== undefined || tweenPercentage !== undefined || animationId !== undefined
150-
? [{ percentage: pct, propertyGroup, tweenPercentage, animationId }]
151-
: undefined;
144+
const carriesIdentity =
145+
target.propertyGroup !== undefined ||
146+
target.tweenPercentage !== undefined ||
147+
target.animationId !== undefined;
152148
// The clicked element's own cache when the caller knows it: the diamond
153149
// context menu can open on an element that is not the selected one, and
154150
// reading the selection's cache there resolves against the wrong element.
155151
const cached = usePlayerStore
156152
.getState()
157153
.keyframeCache.get(elementKey ?? domEditSelection?.id ?? "");
158154
return resolveTimelineKeyframeTarget(
159-
pct,
160-
explicitTarget ?? cached?.keyframes ?? [],
155+
target.percentage,
156+
carriesIdentity ? [target] : (cached?.keyframes ?? []),
161157
animations,
162158
);
163159
},
@@ -202,9 +198,9 @@ export function useTimelineEditCallbacks({
202198
if (!anim) return;
203199
handleGsapRemoveAllKeyframes(anim.id);
204200
},
205-
onDeleteKeyframe: (elId, pct, group, tweenPct, animationId) => {
201+
onDeleteKeyframe: (elId, keyframe) => {
206202
const animations = resolveElementAnimations(elId);
207-
const target = resolveKeyframeTarget(pct, group, tweenPct, animationId, animations, elId);
203+
const target = resolveKeyframeTarget(keyframe, animations, elId);
208204
if (!target) return;
209205
const element = usePlayerStore.getState().elements.find((el) => (el.key ?? el.id) === elId);
210206
if (!element) {
@@ -219,15 +215,8 @@ export function useTimelineEditCallbacks({
219215
});
220216
},
221217
// Retime the keyframe to the playhead, preserving its value + ease.
222-
onMoveKeyframeToPlayhead: (elId, pct, group, tweenPct, animationId) => {
223-
const target = resolveKeyframeTarget(
224-
pct,
225-
group,
226-
tweenPct,
227-
animationId,
228-
resolveElementAnimations(elId),
229-
elId,
230-
);
218+
onMoveKeyframeToPlayhead: (elId, keyframe) => {
219+
const target = resolveKeyframeTarget(keyframe, resolveElementAnimations(elId), elId);
231220
if (target) handleGsapMoveKeyframeToPlayhead(target.animId, target.tweenPct);
232221
},
233222
// Drag-to-retime. The diamond reports clip-%s; resolveKeyframeTarget gives
@@ -238,11 +227,17 @@ export function useTimelineEditCallbacks({
238227
// resizes the tween — position/duration grow so the dragged keyframe lands at
239228
// the drop while every other keyframe keeps its absolute time (value+ease too).
240229
// fallow-ignore-next-line complexity
241-
onMoveKeyframe: async (_elId, fromClipPct, toClipPct, group, tweenPct, animationId) => {
242-
const target = resolveKeyframeTarget(fromClipPct, group, tweenPct, animationId);
243-
const sel = domEditSelection;
244-
if (!target || !sel) return false;
245-
const anim = selectedGsapAnimations.find((a) => a.id === target.animId);
230+
onMoveKeyframe: async (elId, keyframe, toClipPct) => {
231+
const animations = resolveElementAnimations(elId);
232+
const target = resolveKeyframeTarget(keyframe, animations, elId);
233+
if (!target) return false;
234+
// The dragged diamond's OWN element, not the selected one: a drag on a
235+
// non-selected clip has to read that clip's animations and commit
236+
// through that clip's selection, or it retimes whatever is selected.
237+
const element = usePlayerStore.getState().elements.find((el) => (el.key ?? el.id) === elId);
238+
const sel = element ? await buildDomSelectionForTimelineElement(element) : domEditSelection;
239+
if (!sel) return false;
240+
const anim = animations.find((a) => a.id === target.animId);
246241
const tweenStart = anim ? resolveTweenStart(anim) : null;
247242
if (!anim || tweenStart === null) return false;
248243
// Synthesized flat endpoints are clip boundaries, not authored keyframes.
@@ -267,7 +262,7 @@ export function useTimelineEditCallbacks({
267262
dropAbsTime,
268263
});
269264
if (decision.kind === "move" && decision.toTweenPct != null) {
270-
handleGsapMoveKeyframe(target.animId, target.tweenPct, decision.toTweenPct);
265+
handleGsapMoveKeyframe(target.animId, target.tweenPct, decision.toTweenPct, sel);
271266
} else if (
272267
decision.kind === "resize" &&
273268
decision.pctRemap &&
@@ -280,15 +275,17 @@ export function useTimelineEditCallbacks({
280275
decision.position,
281276
decision.duration,
282277
decision.pctRemap,
278+
sel,
283279
);
284280
} else {
285281
// resize-keyframed-tween requires an authored `keyframes` AST node
286282
// and intentionally no-ops for a flat tween. Update its real tween
287283
// window through the metadata writer (and SDK cutover path) instead.
288-
handleGsapUpdateMeta(target.animId, {
289-
position: decision.position,
290-
duration: decision.duration,
291-
});
284+
handleGsapUpdateMeta(
285+
target.animId,
286+
{ position: decision.position, duration: decision.duration },
287+
sel,
288+
);
292289
}
293290
} else {
294291
return false;

‎packages/studio/src/player/components/TimelineClipDiamonds.tsx‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -48,12 +48,13 @@ interface TimelineClipDiamondsProps {
4848
onShiftClickKeyframe?: (elementId: string, percentage: number) => void;
4949
onContextMenuKeyframe?: (e: React.MouseEvent, elementId: string, percentage: number) => void;
5050
/** Drag-to-retime: move a keyframe to a new time, preserving its value + ease.
51-
* Both percentages are clip-relative: `fromClipPercentage` identifies the
52-
* dragged keyframe, `toClipPercentage` is the neighbour-clamped drop position.
53-
* The handler decides move (within the tween) vs resize (past its boundary). */
51+
* `keyframe` identifies the dragged keyframe (clip-relative percentage plus
52+
* whatever animation identity the row carries); `toClipPercentage` is the
53+
* neighbour-clamped drop position, also clip-relative. The handler decides
54+
* move (within the tween) vs resize (past its boundary). */
5455
onMoveKeyframe?: (
5556
elementId: string,
56-
fromClipPercentage: number,
57+
keyframe: TimelineKeyframeTarget,
5758
toClipPercentage: number,
5859
) => Promise<boolean>;
5960
/** Open the segment ease editor for the hovered mid-point button — available on
@@ -549,7 +550,7 @@ export const TimelineClipDiamonds = memo(function TimelineClipDiamonds(
549550
onMoveKeyframe={
550551
props.onMoveKeyframe
551552
? (target, toClipPercentage) =>
552-
props.onMoveKeyframe?.(props.elementId, target.percentage, toClipPercentage) ??
553+
props.onMoveKeyframe?.(props.elementId, target, toClipPercentage) ??
553554
Promise.resolve(false)
554555
: undefined
555556
}

‎packages/studio/src/player/components/TimelineLanes.tsx‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import { Eye, EyeSlash } from "@phosphor-icons/react";
33
import { BeatStrip, BeatBackgroundLines } from "./BeatStrip";
44
import { TimelineClip } from "./TimelineClip";
55
import { TimelineClipDiamonds } from "./TimelineClipDiamonds";
6+
import type { TimelineKeyframeTarget } from "./timelineKeyframeIdentity";
67
import type { MusicBeatAnalysis } from "@hyperframes/core/beats";
78
import { getTimelineEditCapabilities, resolveBlockedTimelineEditIntent } from "./timelineEditing";
89
import type { TimelineTheme } from "./timelineTheme";
@@ -76,7 +77,7 @@ export interface TimelineLaneBaseProps {
7677
onContextMenuKeyframe?: (e: React.MouseEvent, elementId: string, percentage: number) => void;
7778
onMoveKeyframe?: (
7879
elementId: string,
79-
fromClipPercentage: number,
80+
keyframe: TimelineKeyframeTarget,
8081
toClipPercentage: number,
8182
) => Promise<boolean>;
8283
onContextMenuClip?: (e: React.MouseEvent, element: TimelineElement) => void;

‎packages/studio/src/player/components/TimelineOverlays.tsx‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -106,12 +106,12 @@ export function TimelineOverlays({
106106
<KeyframeDiamondContextMenu
107107
state={kfContextMenu}
108108
onClose={() => setKfContextMenu(null)}
109-
onDelete={(elId, pct) => onDeleteKeyframe?.(elId, pct)}
109+
onDelete={(elId, pct) => onDeleteKeyframe?.(elId, { percentage: pct })}
110110
onDeleteAll={(elId) => onDeleteAllKeyframes?.(elId)}
111111
onChangeEase={(elId, pct, ease) => onChangeKeyframeEase?.(elId, pct, ease)}
112112
onMoveToPlayhead={
113113
onMoveKeyframeToPlayhead
114-
? (elId, pct) => onMoveKeyframeToPlayhead(elId, pct)
114+
? (elId, pct) => onMoveKeyframeToPlayhead(elId, { percentage: pct })
115115
: undefined
116116
}
117117
onCopyProperties={(elId, pct) => {

0 commit comments

Comments
 (0)