diff --git a/packages/studio/src/components/nle/useTimelineEditCallbacks.test.tsx b/packages/studio/src/components/nle/useTimelineEditCallbacks.test.tsx index cb1d80f72..2218cc488 100644 --- a/packages/studio/src/components/nle/useTimelineEditCallbacks.test.tsx +++ b/packages/studio/src/components/nle/useTimelineEditCallbacks.test.tsx @@ -16,11 +16,11 @@ const mocks = vi.hoisted(() => ({ handleGsapMoveKeyframeToPlayhead: vi.fn(), handleGsapMoveKeyframe: vi.fn().mockResolvedValue(true), handleGsapResizeKeyframedTween: vi.fn().mockResolvedValue(true), - handleGsapUpdateMeta: vi.fn(), + handleGsapUpdateMeta: vi.fn().mockResolvedValue(true), handleGsapAddKeyframe: vi.fn(), handleGsapAddKeyframeBatch: vi.fn().mockResolvedValue(undefined), handleGsapConvertToKeyframes: vi.fn(), - handleGsapRemoveAllKeyframes: vi.fn(), + handleGsapRemoveAllKeyframes: vi.fn().mockResolvedValue(true), handleGsapDeleteAnimation: vi.fn(), buildDomSelectionForTimelineElement: vi.fn(), }, @@ -116,6 +116,19 @@ function renderCallbacks(): { callbacks: TimelineEditCallbacks; unmount: () => v return { callbacks, unmount: () => act(() => root.unmount()) }; } +// One selection PER element, so a callback that resolves the selection for the +// wrong element gets a visibly different object. A single mockResolvedValue +// hands every element the same selection, which passes just as happily when the +// write is committed through whatever happens to be selected. +function selectionForElement(el: TimelineElement): { + id: string; + selector: string; + sourceFile: string; +} { + if (el.id === "box") return mocks.selection; + return { id: el.id, selector: `#${el.id}`, sourceFile: el.sourceFile ?? "index.html" }; +} + function arrangeClickedCircle(): { circle: TimelineElement; selection: { id: string; selector: string; sourceFile: string }; @@ -128,19 +141,19 @@ function arrangeClickedCircle(): { domId: "circle", sourceFile: "scenes/main.html", }; - const selection = { id: "circle", selector: "#circle", sourceFile: "scenes/main.html" }; usePlayerStore.setState({ elements: [element, circle], gsapAnimations: new Map([[elementKey, [otherKeyframedAnimation]]]), }); - mocks.actions.buildDomSelectionForTimelineElement.mockResolvedValue(selection); - return { circle, selection }; + return { circle, selection: selectionForElement(circle) }; } beforeEach(() => { vi.clearAllMocks(); mocks.animations = [flatAnimation]; - mocks.actions.buildDomSelectionForTimelineElement.mockResolvedValue(mocks.selection); + mocks.actions.buildDomSelectionForTimelineElement.mockImplementation((el: TimelineElement) => + Promise.resolve(selectionForElement(el)), + ); usePlayerStore.setState({ currentTime: 0.5, elements: [element], @@ -212,6 +225,27 @@ describe("useTimelineEditCallbacks — flat tween keyframe lanes", () => { view.unmount(); }); + it("reports an unsettled flat-boundary retime as uncommitted", async () => { + mocks.actions.handleGsapUpdateMeta.mockResolvedValueOnce(false); + const view = renderCallbacks(); + + // The diamond snaps back on `false`. Answering `true` the moment update-meta + // was dispatched left a rejected boundary drag rendered at its drop position. + await expect( + view.callbacks.onMoveKeyframe?.( + "box", + { + percentage: 0, + propertyGroup: "position", + tweenPercentage: 0, + animationId: flatAnimation.id, + }, + 25, + ), + ).resolves.toBe(false); + view.unmount(); + }); + it("refuses a non-selected element flat boundary instead of deleting the tween", async () => { const circle: TimelineElement = { ...element, @@ -242,7 +276,7 @@ describe("useTimelineEditCallbacks — flat tween keyframe lanes", () => { otherFlatAnimation.id, 0, undefined, - mocks.selection, + selectionForElement(circle), ); expect(mocks.actions.handleGsapDeleteAnimation).not.toHaveBeenCalled(); view.unmount(); @@ -276,7 +310,7 @@ describe("useTimelineEditCallbacks — flat tween keyframe lanes", () => { otherKeyframedAnimation.id, 100, undefined, - mocks.selection, + selectionForElement(circle), ); expect(mocks.actions.handleGsapDeleteAnimation).not.toHaveBeenCalled(); view.unmount(); @@ -298,6 +332,65 @@ describe("useTimelineEditCallbacks — flat tween keyframe lanes", () => { view.unmount(); }); + it("deletes all keyframes on every keyframed tween of the layer, not just the first", async () => { + const opacityAnimation: GsapAnimation = { + ...otherKeyframedAnimation, + id: "circle-to-0-visual", + propertyGroup: "visual", + }; + const { circle } = arrangeClickedCircle(); + usePlayerStore.setState({ + gsapAnimations: new Map([ + ["scenes/main.html#circle", [otherKeyframedAnimation, opacityAnimation]], + ]), + }); + const view = renderCallbacks(); + + await act(async () => { + view.callbacks.onDeleteAllKeyframes?.(circle); + await Promise.resolve(); + await Promise.resolve(); + await Promise.resolve(); + }); + + expect(mocks.actions.handleGsapRemoveAllKeyframes.mock.calls.map((call) => call[0])).toEqual([ + otherKeyframedAnimation.id, + opacityAnimation.id, + ]); + view.unmount(); + }); + + it("aborts every mutation when the clicked element resolves no selection", async () => { + const { circle } = arrangeClickedCircle(); + mocks.actions.buildDomSelectionForTimelineElement.mockResolvedValue(null); + const view = renderCallbacks(); + + await act(async () => { + view.callbacks.onDeleteAllKeyframes?.(circle); + view.callbacks.onMoveKeyframeToPlayhead?.(circle, { + percentage: 100, + propertyGroup: "position", + tweenPercentage: 100, + animationId: otherKeyframedAnimation.id, + }); + view.callbacks.onDeleteKeyframe?.("scenes/main.html#circle", { + percentage: 100, + propertyGroup: "position", + tweenPercentage: 100, + animationId: otherKeyframedAnimation.id, + }); + await Promise.resolve(); + await Promise.resolve(); + }); + + // No selection for the clicked element means there is nothing safe to write + // to: falling back to the current selection would edit a different file. + expect(mocks.actions.handleGsapRemoveAllKeyframes).not.toHaveBeenCalled(); + expect(mocks.actions.handleGsapMoveKeyframeToPlayhead).not.toHaveBeenCalled(); + expect(mocks.actions.handleGsapRemoveKeyframe).not.toHaveBeenCalled(); + view.unmount(); + }); + it("moves a keyframe to the playhead through the clicked non-selected element's identity", async () => { const { circle, selection } = arrangeClickedCircle(); const view = renderCallbacks(); @@ -398,7 +491,7 @@ describe("useTimelineEditCallbacks — flat tween keyframe lanes", () => { otherFlatAnimation.id, 0, undefined, - mocks.selection, + selectionForElement(circle), ); 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 635f31675..4b689f025 100644 --- a/packages/studio/src/components/nle/useTimelineEditCallbacks.ts +++ b/packages/studio/src/components/nle/useTimelineEditCallbacks.ts @@ -194,10 +194,18 @@ export function useTimelineEditCallbacks({ // than deleting the whole animation — deleting strands a stale GSAP base // that the next drag adds to, flinging the element off-screen. const elementKey = getTimelineElementIdentity(element); - const anim = resolveElementAnimations(elementKey).find((animation) => animation.keyframes); - if (!anim) return; - void buildDomSelectionForTimelineElement(element).then((selection) => { - if (selection) handleGsapRemoveAllKeyframes(anim.id, selection); + // Every keyframed tween on the layer, not just the first: a layer with + // position AND opacity keyframes left the second one keyframed, so + // "Delete All Keyframes" visibly did half the job. + const anims = resolveElementAnimations(elementKey).filter( + (animation) => animation.keyframes, + ); + if (anims.length === 0) return; + void buildDomSelectionForTimelineElement(element).then(async (selection) => { + if (!selection) return; + // Serial: each removal rewrites the same source file, so dispatching + // them together would have the later writes read a pre-edit document. + for (const anim of anims) await handleGsapRemoveAllKeyframes(anim.id, selection); }); }, onDeleteKeyframe: (elId, keyframe) => { @@ -287,12 +295,14 @@ export function useTimelineEditCallbacks({ // keyframes form as a side effect of a pure position/duration change, so // dispatch update-meta and leave the tween as the author wrote it. if (decision.pctRemap.length === 0) { - handleGsapUpdateMeta( + // Report the write's real settlement, like every other branch here: + // answering `true` while the meta update is still in flight tells the + // diamond the retime landed, so a rejected write never snaps back. + return handleGsapUpdateMeta( target.animId, { position: decision.position, duration: decision.duration }, sel, ); - return true; } return handleGsapResizeKeyframedTween( target.animId, diff --git a/packages/studio/src/hooks/gsapDragPositionCommit.ts b/packages/studio/src/hooks/gsapDragPositionCommit.ts index 28c437710..794e335cc 100644 --- a/packages/studio/src/hooks/gsapDragPositionCommit.ts +++ b/packages/studio/src/hooks/gsapDragPositionCommit.ts @@ -210,8 +210,13 @@ async function commitFlatViaKeyframes( { label: "Convert to keyframes for drag", skipReload: true, coalesceKey }, ); const fresh = callbacks.fetchAnimations ? await callbacks.fetchAnimations() : []; + // By id first: a target with several tweens (two `to`s on the same selector) + // matches the selector lookup on whichever one happens to be first, and the + // extend-and-add below would then rewrite a tween the drag never touched. const converted = - fresh.find((a) => a.targetSelector === anim.targetSelector && a.keyframes) ?? anim; + fresh.find((a) => a.id === anim.id && a.keyframes) ?? + fresh.find((a) => a.targetSelector === anim.targetSelector && a.keyframes) ?? + anim; const convertedStart = resolveTweenStart(converted) ?? ts; const convertedDur = resolveTweenDuration(converted) || td; await extendTweenAndAddKeyframe( diff --git a/packages/studio/src/hooks/useGsapSelectionHandlers.test.tsx b/packages/studio/src/hooks/useGsapSelectionHandlers.test.tsx index 6dc247b36..b20423876 100644 --- a/packages/studio/src/hooks/useGsapSelectionHandlers.test.tsx +++ b/packages/studio/src/hooks/useGsapSelectionHandlers.test.tsx @@ -4,6 +4,7 @@ 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 { usePlayerStore } from "../player/store/playerStore"; import { useGsapSelectionHandlers } from "./useGsapSelectionHandlers"; (globalThis as unknown as { IS_REACT_ACT_ENVIRONMENT: boolean }).IS_REACT_ACT_ENVIRONMENT = true; @@ -80,7 +81,11 @@ describe("useGsapSelectionHandlers save failures", () => { makeParams({ updateGsapMeta: vi.fn().mockRejectedValue(error), showToast }), ); - act(() => rendered.handlers().handleGsapUpdateMeta("anim-1", { duration: 2 })); + // Braces, not a bare arrow: the handler returns its settlement promise now, + // and returning a thenable from act() turns it into an un-awaited async act. + act(() => { + void rendered.handlers().handleGsapUpdateMeta("anim-1", { duration: 2 }); + }); await flushRejection(); expect(showToast).toHaveBeenCalledWith("Couldn't save animation: write failed", "error"); @@ -135,12 +140,24 @@ describe("useGsapSelectionHandlers selection override", () => { 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; + // The passed tween runs 2s→6s, so the playhead at 3s is 25% into IT. Without + // the animation the handler falls back to the selection's own element window + // (0s→1s here), which reads the same playhead as 100%. Asserting the exact + // 25 is what separates the two; `expect.any(Number)` even accepts the NaN a + // missing window would produce. + const animation = { + id: "anim-1", + position: 2, + resolvedStart: 2, + duration: 4, + keyframes: { keyframes: [] }, + } as unknown as GsapAnimation; + usePlayerStore.setState({ currentTime: 3 }); const rendered = renderHandlers(makeParams({ moveKeyframe, selectedGsapAnimations: [] })); rendered.handlers().handleGsapMoveKeyframeToPlayhead("anim-1", 50, selection, animation); - expect(moveKeyframe).toHaveBeenCalledWith(selection, "anim-1", 50, expect.any(Number)); + expect(moveKeyframe).toHaveBeenCalledWith(selection, "anim-1", 50, 25); rendered.unmount(); }); }); @@ -163,20 +180,3 @@ describe("useGsapSelectionHandlers retime settlement", () => { withSelection.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(); - }); -}); diff --git a/packages/studio/src/hooks/useGsapSelectionHandlers.ts b/packages/studio/src/hooks/useGsapSelectionHandlers.ts index 7247433d4..c7579219e 100644 --- a/packages/studio/src/hooks/useGsapSelectionHandlers.ts +++ b/packages/studio/src/hooks/useGsapSelectionHandlers.ts @@ -150,12 +150,24 @@ export function useGsapSelectionHandlers({ [showToast], ); + // Resolves to whether the mutation landed. Callers that only fire-and-forget + // can ignore it (the rejection is always handled here), but a caller that + // reports a commit result to the UI has to await the real settlement instead + // of assuming success the moment it dispatched. const observeGsapMutation = useCallback( - (mutation: Promise, selection: DomEditSelection, mutationType: string, label: string) => { - void mutation.catch((error) => { - trackGsapHandlerFailure(error, selection, mutationType, label); - }); - }, + ( + mutation: Promise, + selection: DomEditSelection, + mutationType: string, + label: string, + ): Promise => + mutation.then( + () => true, + (error: unknown) => { + trackGsapHandlerFailure(error, selection, mutationType, label); + return false; + }, + ), [trackGsapHandlerFailure], ); @@ -174,8 +186,8 @@ export function useGsapSelectionHandlers({ selectionOverride?: DomEditSelection | null, ) => { const sel = resolveWriteSelection(selectionOverride); - if (!sel) return; - observeGsapMutation( + if (!sel) return Promise.resolve(false); + return observeGsapMutation( updateGsapMeta(sel, animId, updates), sel, "update-meta", @@ -419,8 +431,8 @@ export function useGsapSelectionHandlers({ const handleGsapRemoveAllKeyframes = useCallback( (animId: string, selectionOverride?: DomEditSelection | null) => { const selection = resolveWriteSelection(selectionOverride); - if (!selection) return; - observeGsapMutation( + if (!selection) return Promise.resolve(false); + return observeGsapMutation( removeAllKeyframes(selection, animId), selection, "remove-all-keyframes", diff --git a/packages/studio/src/player/components/LayerDisclosureRow.tsx b/packages/studio/src/player/components/LayerDisclosureRow.tsx index 596abd197..2f56f0aa8 100644 --- a/packages/studio/src/player/components/LayerDisclosureRow.tsx +++ b/packages/studio/src/player/components/LayerDisclosureRow.tsx @@ -1,6 +1,6 @@ import { CaretRight } from "@phosphor-icons/react"; import type { TimelineElement } from "../store/playerStore"; -import { LABEL_COL_W, TRACK_H } from "./timelineLayout"; +import { TRACK_H } from "./timelineLayout"; import { TrackClipCount } from "./TrackClipCount"; // Layer row (Figma order: disclosure ▸/▾, diamond, name) — the disclosure lives @@ -10,6 +10,8 @@ export function LayerDisclosureRow({ clipCount, isExpanded, gutterBackground, + columnWidth, + lanesId, onToggleClipExpanded, children, }: { @@ -17,6 +19,11 @@ export function LayerDisclosureRow({ clipCount: number; isExpanded: boolean; gutterBackground: string; + /** Same adaptive width the lane rows use: a narrowed header column must not + * leave this row hanging over the clips it labels. */ + columnWidth: number; + /** Id of the element holding the lanes this row's caret expands. */ + lanesId: string; onToggleClipExpanded: () => void; /** Trailing controls that act on the LAYER (the visibility eye), not on a lane. */ children?: React.ReactNode; @@ -26,7 +33,7 @@ export function LayerDisclosureRow({
event.stopPropagation()} onClick={(event) => { event.stopPropagation(); diff --git a/packages/studio/src/player/components/TimelineClipDiamonds.test.tsx b/packages/studio/src/player/components/TimelineClipDiamonds.test.tsx index 11c83585b..aff08c082 100644 --- a/packages/studio/src/player/components/TimelineClipDiamonds.test.tsx +++ b/packages/studio/src/player/components/TimelineClipDiamonds.test.tsx @@ -310,6 +310,89 @@ describe("TimelineClipDiamonds", () => { act(() => root.unmount()); }); + it("leaves the selection alone when a stale retime fails after a newer drag", async () => { + const onClickKeyframe = vi.fn(); + let failFirstDrag: (() => void) | undefined; + const onMoveKeyframe = vi + .fn() + .mockImplementationOnce( + () => + new Promise((resolve) => { + failFirstDrag = () => resolve(false); + }), + ) + .mockResolvedValue(true); + const host = document.createElement("div"); + document.body.append(host); + const root = createRoot(host); + act(() => { + root.render( + , + ); + }); + const diamond = host.querySelector('button[title="50%"]'); + + // Two drags back to back; the first one's commit is still in flight. + act(() => { + diamond!.dispatchEvent( + pointerEvent("pointerdown", { bubbles: true, button: 0, clientX: 100 }), + ); + diamond!.dispatchEvent(pointerEvent("pointerup", { bubbles: true, button: 0, clientX: 150 })); + diamond!.dispatchEvent( + pointerEvent("pointerdown", { bubbles: true, button: 0, clientX: 150 }), + ); + diamond!.dispatchEvent(pointerEvent("pointerup", { bubbles: true, button: 0, clientX: 170 })); + }); + onClickKeyframe.mockClear(); + + await act(async () => { + failFirstDrag?.(); + await Promise.resolve(); + }); + + // The stale failure must not drag the selection back to the first drag's + // source: the second retime, which the user can see, owns it now. + expect(onClickKeyframe).not.toHaveBeenCalled(); + act(() => root.unmount()); + }); + it("composes a rapid second retime from the pending position", () => { const onMoveKeyframe = vi.fn().mockResolvedValue(true); const host = document.createElement("div"); diff --git a/packages/studio/src/player/components/TimelineClipDiamonds.tsx b/packages/studio/src/player/components/TimelineClipDiamonds.tsx index 47e17b22d..2f6ec31b8 100644 --- a/packages/studio/src/player/components/TimelineClipDiamonds.tsx +++ b/packages/studio/src/player/components/TimelineClipDiamonds.tsx @@ -63,6 +63,10 @@ export const TimelineDiamondLane = memo(function TimelineDiamondLane({ // the first away, once per mounted lane. pendingRetimeRef.current ??= new Map(); const pendingRetimes = pendingRetimeRef.current; + // The most recent retime dispatched from this lane, whichever diamond it came + // from. Selection is lane-wide, so "is my revert still relevant" is a lane-wide + // question, not a per-keyframe one. + const latestRetimeRef = useRef<{ clipPct: number; tweenPct: number } | null>(null); useEffect(() => { // Clear a pending entry once the authoritative cache reflects THAT keyframe // at ~its destination. Match by tolerance, not equality: cache writers round @@ -354,6 +358,7 @@ export const TimelineDiamondLane = memo(function TimelineDiamondLane({ : target; const pending = { clipPct: res.toClipPct, tweenPct: newTweenPct }; pendingRetimes.set(kfKey, pending); + latestRetimeRef.current = pending; const clearPending = () => { if (pendingRetimes.get(kfKey) === pending) { pendingRetimes.delete(kfKey); @@ -365,8 +370,13 @@ export const TimelineDiamondLane = memo(function TimelineDiamondLane({ // position strands the playhead + selection on a keyframe that does // not exist there. const revertRetime = () => { + // Only the newest gesture owns the selection. A rejected first drag + // whose commit settles after a second one started would otherwise + // park the selection back on ITS source keyframe, undoing a retime + // the user has already made and moving the playhead with it. + const isLatest = latestRetimeRef.current === pending; clearPending(); - onClickKeyframe?.(fromTarget); + if (isLatest) onClickKeyframe?.(fromTarget); }; void onMoveKeyframe?.(fromTarget, res.toClipPct).then((committed) => { if (!committed) revertRetime(); @@ -386,7 +396,11 @@ export const TimelineDiamondLane = memo(function TimelineDiamondLane({ return (