Skip to content

Commit 012816a

Browse files
committed
fix(studio): scope timeline context targets
1 parent 6f6e9ac commit 012816a

14 files changed

Lines changed: 455 additions & 57 deletions

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

Lines changed: 78 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -317,17 +317,44 @@ describe("useTimelineEditCallbacks — flat tween keyframe lanes", () => {
317317
});
318318

319319
it("deletes all keyframes through the clicked non-selected element's identity", async () => {
320-
const { circle, selection } = arrangeClickedCircle();
320+
const circle: TimelineElement = {
321+
...element,
322+
id: "circle",
323+
key: "scenes/main.html#circle",
324+
domId: "circle",
325+
sourceFile: "scenes/main.html",
326+
};
327+
const circleSelection = { id: "circle", selector: "#circle", sourceFile: "scenes/main.html" };
328+
const scaleAnimation: GsapAnimation = {
329+
...otherKeyframedAnimation,
330+
id: "circle-to-0-scale",
331+
properties: {},
332+
propertyGroup: "scale",
333+
keyframes: {
334+
format: "percentage",
335+
keyframes: [
336+
{ percentage: 0, properties: { scale: 1 } },
337+
{ percentage: 100, properties: { scale: 2 } },
338+
],
339+
},
340+
};
341+
usePlayerStore.setState({
342+
elements: [element, circle],
343+
gsapAnimations: new Map([
344+
["scenes/main.html#circle", [otherKeyframedAnimation, scaleAnimation]],
345+
]),
346+
});
347+
mocks.actions.buildDomSelectionForTimelineElement.mockResolvedValue(circleSelection);
321348
const view = renderCallbacks();
322349

323350
await act(async () => {
324-
view.callbacks.onDeleteAllKeyframes?.(circle);
351+
view.callbacks.onDeleteAllKeyframes?.(circle, scaleAnimation.id);
325352
await Promise.resolve();
326353
});
327354

328355
expect(mocks.actions.handleGsapRemoveAllKeyframes).toHaveBeenCalledWith(
329-
otherKeyframedAnimation.id,
330-
selection,
356+
scaleAnimation.id,
357+
circleSelection,
331358
);
332359
view.unmount();
333360
});
@@ -360,6 +387,19 @@ describe("useTimelineEditCallbacks — flat tween keyframe lanes", () => {
360387
view.unmount();
361388
});
362389

390+
it("does not delete a different lane when an explicit animation identity is stale", async () => {
391+
const { circle } = arrangeClickedCircle();
392+
const view = renderCallbacks();
393+
394+
await act(async () => {
395+
view.callbacks.onDeleteAllKeyframes?.(circle, "missing-animation-id");
396+
await Promise.resolve();
397+
});
398+
399+
expect(mocks.actions.handleGsapRemoveAllKeyframes).not.toHaveBeenCalled();
400+
view.unmount();
401+
});
402+
363403
it("aborts every mutation when the clicked element resolves no selection", async () => {
364404
const { circle } = arrangeClickedCircle();
365405
mocks.actions.buildDomSelectionForTimelineElement.mockResolvedValue(null);
@@ -391,6 +431,40 @@ describe("useTimelineEditCallbacks — flat tween keyframe lanes", () => {
391431
view.unmount();
392432
});
393433

434+
it("does not delete a different keyframe when its explicit animation identity is stale", () => {
435+
const view = renderCallbacks();
436+
437+
act(() => {
438+
view.callbacks.onDeleteKeyframe?.("box", {
439+
percentage: 100,
440+
propertyGroup: "position",
441+
tweenPercentage: 100,
442+
animationId: "missing-animation-id",
443+
});
444+
});
445+
446+
expect(mocks.actions.handleGsapDeleteAnimation).not.toHaveBeenCalled();
447+
expect(mocks.actions.handleGsapRemoveKeyframe).not.toHaveBeenCalled();
448+
view.unmount();
449+
});
450+
451+
it("does not move a different keyframe when its explicit animation identity is stale", async () => {
452+
const view = renderCallbacks();
453+
454+
await act(async () => {
455+
view.callbacks.onMoveKeyframeToPlayhead?.(element, {
456+
percentage: 100,
457+
propertyGroup: "position",
458+
tweenPercentage: 100,
459+
animationId: "missing-animation-id",
460+
});
461+
await Promise.resolve();
462+
});
463+
464+
expect(mocks.actions.handleGsapMoveKeyframeToPlayhead).not.toHaveBeenCalled();
465+
view.unmount();
466+
});
467+
394468
it("moves a keyframe to the playhead through the clicked non-selected element's identity", async () => {
395469
const { circle, selection } = arrangeClickedCircle();
396470
const view = renderCallbacks();

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

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -189,17 +189,22 @@ export function useTimelineEditCallbacks({
189189
onSplitElement: handleTimelineElementSplit,
190190
onRazorSplit: handleRazorSplit,
191191
onRazorSplitAll: handleRazorSplitAll,
192-
onDeleteAllKeyframes: (element) => {
192+
onDeleteAllKeyframes: (element, animationId) => {
193193
// Hold the element where it is (collapse keyframes to a static set) rather
194194
// than deleting the whole animation — deleting strands a stale GSAP base
195195
// that the next drag adds to, flinging the element off-screen.
196196
const elementKey = getTimelineElementIdentity(element);
197-
// Every keyframed tween on the layer, not just the first: a layer with
198-
// position AND opacity keyframes left the second one keyframed, so
199-
// "Delete All Keyframes" visibly did half the job.
200-
const anims = resolveElementAnimations(elementKey).filter(
201-
(animation) => animation.keyframes,
202-
);
197+
// An explicit animation id scopes the delete to the lane whose menu was
198+
// opened; without one this is the layer-wide action, and that means
199+
// EVERY keyframed tween, not just the first. A layer with position AND
200+
// opacity keyframes used to leave the second one keyframed, so "Delete
201+
// All Keyframes" visibly did half the job. A stale id matches nothing
202+
// and deletes nothing, which is the point: it never falls back to a
203+
// lane the user did not click.
204+
const animations = resolveElementAnimations(elementKey);
205+
const anims = animationId
206+
? animations.filter((animation) => animation.id === animationId)
207+
: animations.filter((animation) => animation.keyframes);
203208
if (anims.length === 0) return;
204209
void buildDomSelectionForTimelineElement(element).then(async (selection) => {
205210
if (!selection) return;

packages/studio/src/hooks/useGsapKeyframeOps.test.tsx

Lines changed: 39 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,8 @@ import { afterEach, describe, expect, it, vi } from "vitest";
77
(globalThis as unknown as { IS_REACT_ACT_ENVIRONMENT: boolean }).IS_REACT_ACT_ENVIRONMENT = true;
88

99
import type { DomEditSelection } from "../components/editor/domEditingTypes";
10+
import type { KeyframeCacheEntry } from "../player/store/playerStore";
11+
import { usePlayerStore } from "../player/store/playerStore";
1012
import { useGsapKeyframeOps } from "./useGsapKeyframeOps";
1113

1214
type HookApi = ReturnType<typeof useGsapKeyframeOps>;
@@ -30,6 +32,7 @@ function successfulCommitMutation() {
3032

3133
function renderKeyframeOps(over: {
3234
commitMutation: (...args: unknown[]) => Promise<unknown>;
35+
commitMutationSafely?: (...args: unknown[]) => Promise<void>;
3336
trackGsapSaveFailure: (...args: unknown[]) => void;
3437
}) {
3538
const captured: { api: HookApi | null } = { api: null };
@@ -41,7 +44,7 @@ function renderKeyframeOps(over: {
4144
// eslint-disable-next-line @typescript-eslint/no-explicit-any -- test doubles
4245
commitMutation: over.commitMutation as any,
4346
// eslint-disable-next-line @typescript-eslint/no-explicit-any -- test doubles
44-
commitMutationSafely: (() => {}) as any,
47+
commitMutationSafely: (over.commitMutationSafely ?? (async () => {})) as any,
4548
// eslint-disable-next-line @typescript-eslint/no-explicit-any -- test doubles
4649
trackGsapSaveFailure: over.trackGsapSaveFailure as any,
4750
sdkSession: null,
@@ -203,6 +206,41 @@ describe("useGsapKeyframeOps — keyframe transaction options", () => {
203206
);
204207
});
205208

209+
it("lets the successful commit refresh own delete-all cache invalidation", async () => {
210+
let finishCommit: (() => void) | undefined;
211+
const commitMutationSafely = vi.fn(
212+
() =>
213+
new Promise<void>((resolve) => {
214+
finishCommit = resolve;
215+
}),
216+
);
217+
const api = renderKeyframeOps({
218+
commitMutation: successfulCommitMutation(),
219+
commitMutationSafely,
220+
trackGsapSaveFailure: vi.fn(),
221+
});
222+
const cached: KeyframeCacheEntry = {
223+
format: "percentage",
224+
keyframes: [
225+
{ percentage: 0, properties: { x: 0 } },
226+
{ percentage: 100, properties: { x: 200 } },
227+
],
228+
};
229+
usePlayerStore.setState({ keyframeCache: new Map([["index.html#box", cached]]) });
230+
231+
const pending = api.removeAllKeyframes(selection, "box-to-0-position");
232+
expect(commitMutationSafely).toHaveBeenCalledWith(
233+
selection,
234+
{ type: "remove-all-keyframes", animationId: "box-to-0-position" },
235+
{ label: "Remove all keyframes", softReload: true },
236+
);
237+
expect(usePlayerStore.getState().keyframeCache.get("index.html#box")).toBe(cached);
238+
239+
finishCommit?.();
240+
await pending;
241+
expect(usePlayerStore.getState().keyframeCache.get("index.html#box")).toBe(cached);
242+
});
243+
206244
it("threads one coalesce key through skipped convert reload and terminal batch edit", async () => {
207245
const commitMutation = successfulCommitMutation();
208246
const api = renderKeyframeOps({ commitMutation, trackGsapSaveFailure: vi.fn() });

packages/studio/src/hooks/useGsapKeyframeOps.ts

Lines changed: 2 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -15,12 +15,7 @@ import {
1515
} from "../utils/sdkCutover";
1616
import type { KeyframeCacheEntry } from "../player/store/playerStore";
1717
import { commitKeyframeAtTimeImpl } from "./gsapKeyframeCommit";
18-
import { idFromSelector } from "./gsapShared";
19-
import {
20-
clearKeyframeCacheForElement,
21-
readKeyframeSnapshot,
22-
writeKeyframeCache,
23-
} from "./gsapKeyframeCacheHelpers";
18+
import { readKeyframeSnapshot, writeKeyframeCache } from "./gsapKeyframeCacheHelpers";
2419
import type {
2520
CommitMutation,
2621
CommitMutationOptions,
@@ -336,11 +331,6 @@ export function useGsapKeyframeOps({
336331
const removeAllKeyframes = useCallback(
337332
async (selection: DomEditSelection, animationId: string) => {
338333
const targetPath = selection.sourceFile || activeCompPath || "index.html";
339-
// remove-all-keyframes collapses the tween to a static hold and the commit
340-
// path doesn't return parsed animations, so the keyframe cache is never
341-
// refreshed — clear it here so the timeline diamonds disappear immediately.
342-
const elementId = selection.id ?? idFromSelector(selection.selector);
343-
if (elementId) clearKeyframeCacheForElement(targetPath, elementId);
344334
if (sdkSession && sdkDeps) {
345335
const handled = await sdkGsapRemoveAllKeyframesPersist(
346336
targetPath,
@@ -351,7 +341,7 @@ export function useGsapKeyframeOps({
351341
);
352342
if (cutoverCommittedOrThrow(handled)) return;
353343
}
354-
commitMutationSafely(
344+
await commitMutationSafely(
355345
selection,
356346
{ type: "remove-all-keyframes", animationId },
357347
{ label: "Remove all keyframes", softReload: true },

packages/studio/src/player/components/KeyframeDiamondContextMenu.test.tsx

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
// @vitest-environment happy-dom
22
import { act } from "react";
33
import { createRoot } from "react-dom/client";
4-
import { describe, expect, it, vi } from "vitest";
4+
import { afterEach, describe, expect, it, vi } from "vitest";
55
import type { TimelineElement } from "../store/playerStore";
66
import {
77
KeyframeDiamondContextMenu,
@@ -10,6 +10,10 @@ import {
1010

1111
(globalThis as unknown as { IS_REACT_ACT_ENVIRONMENT: boolean }).IS_REACT_ACT_ENVIRONMENT = true;
1212

13+
afterEach(() => {
14+
document.body.innerHTML = "";
15+
});
16+
1317
const element = { id: "box", start: 0, duration: 2, track: 0 } as unknown as TimelineElement;
1418

1519
const state: KeyframeDiamondContextMenuState = {
@@ -87,4 +91,15 @@ describe("KeyframeDiamondContextMenu", () => {
8791
act(() => root.unmount());
8892
host.remove();
8993
});
94+
95+
// The layer-wide delete takes every keyframed tween. Opened from a diamond it
96+
// has to stay on that diamond's own lane, or right-clicking the opacity lane
97+
// silently clears position too.
98+
it("deletes all keyframes from the animation that opened the menu", () => {
99+
const onDeleteAll = vi.fn();
100+
101+
clickMenuItem("Delete All Keyframes", { onDeleteAll });
102+
103+
expect(onDeleteAll).toHaveBeenCalledExactlyOnceWith(element, "box-to-1-position");
104+
});
90105
});

packages/studio/src/player/components/KeyframeDiamondContextMenu.tsx

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,8 @@ import type { TimelineKeyframeTarget } from "./timelineKeyframeIdentity";
77
export interface KeyframeDiamondContextMenuState {
88
x: number;
99
y: number;
10+
/** Timeline project session that created this portaled target. */
11+
sessionEpoch?: number;
1012
element: TimelineElement;
1113
elementId: string;
1214
percentage: number;
@@ -23,7 +25,7 @@ interface KeyframeDiamondContextMenuProps {
2325
* floor in removeMotionPathPointInScript): an entry that silently no-ops is
2426
* worse than no entry. */
2527
onDelete?: (elementId: string, keyframe: TimelineKeyframeTarget) => void;
26-
onDeleteAll: (element: TimelineElement) => void;
28+
onDeleteAll: (element: TimelineElement, animationId?: string) => void;
2729
/** Retime the keyframe to the current playhead, preserving its value + ease. */
2830
onMoveToPlayhead?: (element: TimelineElement, keyframe: TimelineKeyframeTarget) => void;
2931
}
@@ -93,7 +95,7 @@ export const KeyframeDiamondContextMenu = memo(function KeyframeDiamondContextMe
9395
type="button"
9496
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"
9597
onClick={() => {
96-
onDeleteAll(state.element);
98+
onDeleteAll(state.element, state.animationId);
9799
onClose();
98100
}}
99101
>

packages/studio/src/player/components/Timeline.tsx

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ import { TimelineEmptyState } from "./TimelineEmptyState";
1212
import { TimelineCanvas } from "./TimelineCanvas";
1313
import { type KeyframeDiamondContextMenuState } from "./KeyframeDiamondContextMenu";
1414
import { useTimelineClipDrag } from "./useTimelineClipDrag";
15-
import { TimelineOverlays } from "./TimelineOverlays";
15+
import { TimelineOverlays, type ClipContextMenuState } from "./TimelineOverlays";
1616
import { useTimelineEditPinning } from "./useTimelineEditPinning";
1717
import { useTimelineStackingSync } from "./useTimelineStackingSync";
1818
import { useTimelineGeometry } from "./useTimelineGeometry";
@@ -141,11 +141,7 @@ export const Timeline = memo(function Timeline({
141141
const shiftHeld = useTimelineShiftModifier();
142142
const [showPopover, setShowPopover] = useState(false);
143143
const [kfContextMenu, setKfContextMenu] = useState<KeyframeDiamondContextMenuState | null>(null);
144-
const [clipContextMenu, setClipContextMenu] = useState<{
145-
x: number;
146-
y: number;
147-
element: TimelineElement;
148-
} | null>(null);
144+
const [clipContextMenu, setClipContextMenu] = useState<ClipContextMenuState | null>(null);
149145
const setContainerRef = useCallback((el: HTMLDivElement | null) => {
150146
containerRef.current = el;
151147
}, []);
@@ -557,7 +553,12 @@ export const Timeline = memo(function Timeline({
557553
setSelectedElementId(el.key ?? el.id);
558554
onSelectElement?.(el);
559555
dismissGapMenu();
560-
setClipContextMenu({ x: e.clientX, y: e.clientY, element: el });
556+
setClipContextMenu({
557+
x: e.clientX,
558+
y: e.clientY,
559+
element: el,
560+
sessionEpoch: usePlayerStore.getState().timelineSessionEpoch,
561+
});
561562
}}
562563
onContextMenuLane={(e, track, time) => {
563564
if (draggedClip?.started || resizingClip) return;
@@ -568,6 +569,8 @@ export const Timeline = memo(function Timeline({
568569
{activeTool === "razor" && razorGuideX !== null && <TimelineRazorGuide x={razorGuideX} />}
569570
</div>
570571
<TimelineOverlays
572+
elements={expandedElements}
573+
elementsRef={expandedElementsRef}
571574
theme={theme}
572575
showShortcutHint={showShortcutHint}
573576
showPopover={showPopover}

0 commit comments

Comments
 (0)