From 04ddd411ecf14123c13e4d766454c9385fa4b9f0 Mon Sep 17 00:00:00 2001 From: Miguel Angel Simon Sierra Date: Thu, 9 Jul 2026 16:15:14 -0400 Subject: [PATCH] fix(studio): harden group timeline edits (capabilities, rollback, snapping, marquee) Group move/resize rejects the gesture when any selected member forbids the op (e.g. a locked clip), so a group never edits a clip that individually cannot move, and a persist failure now propagates so the optimistic preview rolls back. Snapping excludes every moving member, not just the grabbed clip. The marquee hit-test uses the real pixels-per-second (was floored at 1, wrong below 1x zoom), and a sub-threshold marquee click scrubs the playhead like a plain lane click. --- .../src/hooks/useTimelineGroupEditing.ts | 19 ++++++----- .../studio/src/player/components/Timeline.tsx | 16 ++++----- .../components/timelineSnapTargets.test.ts | 27 +++++++++++++-- .../player/components/timelineSnapTargets.ts | 5 +-- .../components/useTimelineClipDrag.test.tsx | 34 +++++++++++++++++++ .../player/components/useTimelineClipDrag.ts | 15 +++++--- .../components/useTimelineClipGroupDrag.ts | 32 +++++++++++++---- .../useTimelineMarqueeSelection.test.tsx | 7 +++- .../components/useTimelineMarqueeSelection.ts | 19 +++++++++-- 9 files changed, 140 insertions(+), 34 deletions(-) diff --git a/packages/studio/src/hooks/useTimelineGroupEditing.ts b/packages/studio/src/hooks/useTimelineGroupEditing.ts index 77a6e814f..ab67deac3 100644 --- a/packages/studio/src/hooks/useTimelineGroupEditing.ts +++ b/packages/studio/src/hooks/useTimelineGroupEditing.ts @@ -99,17 +99,20 @@ export function useTimelineGroupEditing({ (label: string, operation: (projectId: string) => Promise): Promise => { if (isRecordingRef?.current) { showToast("Cannot edit timeline while recording", "error"); - return Promise.resolve(); + return Promise.reject(new Error(`${label}: blocked while recording`)); } const projectId = projectIdRef.current; - if (!projectId) return Promise.resolve(); - const queued = editQueueRef.current - .then(() => operation(projectId)) - .catch((error) => { + if (!projectId) return Promise.reject(new Error(`${label}: no active project`)); + const run = editQueueRef.current.then(() => operation(projectId)); + // Keep the shared edit queue from wedging on a rejection, but return the raw + // (rejecting) promise so the gesture owner can roll back on a real failure. + editQueueRef.current = run.then( + () => undefined, + (error) => { console.error(`[Timeline] Failed to persist: ${label}`, error); - }); - editQueueRef.current = queued; - return queued; + }, + ); + return run; }, [editQueueRef, isRecordingRef, projectIdRef, showToast], ); diff --git a/packages/studio/src/player/components/Timeline.tsx b/packages/studio/src/player/components/Timeline.tsx index 5e9d34a59..7557b2234 100644 --- a/packages/studio/src/player/components/Timeline.tsx +++ b/packages/studio/src/player/components/Timeline.tsx @@ -367,8 +367,14 @@ export const Timeline = memo(function Timeline({ disabled: activeTool === "razor", setShowPopover, setRangeSelectionRef, + seekFromX, }); setRangeSelectionRef.current = setRangeSelection; + // Pointer-up and lost-capture end a gesture identically (marquee-claims-first). + const releasePointer = (event: Parameters[0]) => { + if (handleMarqueePointerUp(event)) return; + handlePointerUp(); + }; const prevSelectedRef = useRef(selectedElementRef.current); // eslint-disable-next-line no-restricted-syntax, react-hooks/exhaustive-deps @@ -472,14 +478,8 @@ export const Timeline = memo(function Timeline({ if (handleMarqueePointerMove(e)) return; handlePointerMove(e); }} - onPointerUp={(e) => { - if (handleMarqueePointerUp(e)) return; - handlePointerUp(); - }} - onLostPointerCapture={(e) => { - if (handleMarqueePointerUp(e)) return; - handlePointerUp(); - }} + onPointerUp={releasePointer} + onLostPointerCapture={releasePointer} > { const targets = buildTimelineSnapTargets({ elements: [dragged, other], - draggedKey: "dragged-key", + excludedKeys: new Set(["dragged-key"]), playhead: 8, compDuration: 10, beats: [1.5], @@ -47,13 +47,36 @@ describe("buildTimelineSnapTargets", () => { expect(times).toContain(6); }); + it("excludes every moving group member, not just the grabbed clip", () => { + const a = timelineElement({ id: "a", start: 1, duration: 1 }); + const b = timelineElement({ id: "b", start: 3, duration: 1 }); + const other = timelineElement({ id: "other", start: 6, duration: 1 }); + + const targets = buildTimelineSnapTargets({ + elements: [a, b, other], + excludedKeys: new Set(["a", "b"]), + playhead: 9, + compDuration: 10, + beats: [], + }); + + const times = targets.map((target) => target.time); + // Both group members' edges are excluded; the non-member's edges remain. + expect(times).not.toContain(1); + expect(times).not.toContain(2); + expect(times).not.toContain(3); + expect(times).not.toContain(4); + expect(times).toContain(6); + expect(times).toContain(7); + }); + it("dedupes near-equal times from different sources", () => { const dragged = timelineElement({ id: "dragged", start: 2, duration: 2 }); const other = timelineElement({ id: "other", start: 0.0004, duration: 10 }); const targets = buildTimelineSnapTargets({ elements: [dragged, other], - draggedKey: "dragged", + excludedKeys: new Set(["dragged"]), playhead: 5, compDuration: 10, beats: [0.0002, 10.0002], diff --git a/packages/studio/src/player/components/timelineSnapTargets.ts b/packages/studio/src/player/components/timelineSnapTargets.ts index 10bdfa963..eb0daac54 100644 --- a/packages/studio/src/player/components/timelineSnapTargets.ts +++ b/packages/studio/src/player/components/timelineSnapTargets.ts @@ -43,7 +43,8 @@ function addTarget(targets: TimelineSnapTarget[], candidate: TimelineSnapTarget) export function buildTimelineSnapTargets(input: { elements: TimelineElement[]; - draggedKey: string; + /** Keys of every clip moving in this gesture (the whole group), excluded as targets. */ + excludedKeys: ReadonlySet; playhead: number; compDuration: number; beats: number[]; @@ -56,7 +57,7 @@ export function buildTimelineSnapTargets(input: { for (const element of input.elements) { const elementKey = element.key ?? element.id; - if (elementKey === input.draggedKey || element.id === input.draggedKey) continue; + if (input.excludedKeys.has(elementKey) || input.excludedKeys.has(element.id)) continue; addTarget(targets, { time: element.start, kind: "edge" }); addTarget(targets, { time: element.start + element.duration, kind: "edge" }); } diff --git a/packages/studio/src/player/components/useTimelineClipDrag.test.tsx b/packages/studio/src/player/components/useTimelineClipDrag.test.tsx index 98973e9a2..6fc750b2b 100644 --- a/packages/studio/src/player/components/useTimelineClipDrag.test.tsx +++ b/packages/studio/src/player/components/useTimelineClipDrag.test.tsx @@ -22,6 +22,7 @@ function timelineElement(input: { sourceDuration?: number; playbackStart?: number; playbackRate?: number; + timelineLocked?: boolean; }): TimelineElement { return { id: input.id, @@ -39,6 +40,7 @@ function timelineElement(input: { compositionAncestors: ["root"], sourceFile: "index.html", timingSource: "authored", + timelineLocked: input.timelineLocked, }; } @@ -265,6 +267,38 @@ describe("useTimelineClipDrag", () => { harness.unmount(); }); + it("does not form a group when a selected member is locked (grabbed clip moves alone)", async () => { + const first = timelineElement({ id: "first", track: 0, zIndex: 1, start: 1, duration: 2 }); + const locked = timelineElement({ + id: "locked", + track: 1, + zIndex: 1, + start: 4, + duration: 2, + timelineLocked: true, + }); + const harness = renderDragHarness([first, locked]); + act(() => { + usePlayerStore.getState().setSelection(["first", "locked"], "first"); + }); + + harness.startDrag(first, 0); + harness.movePointer(200, 0); + await harness.dropPointer(); + + // The locked member forbids the op, so no group forms: the grabbed clip moves + // alone (single-clip path) and the locked clip is never touched. + expect(harness.onMoveElements).not.toHaveBeenCalled(); + expect(harness.onMoveElement).toHaveBeenCalledTimes(1); + expect(harness.onMoveElement).toHaveBeenCalledWith( + first, + expect.objectContaining({ start: 3 }), + ); + expect(harness.storeElements().find((el) => el.id === "locked")?.start).toBe(4); + + harness.unmount(); + }); + it("allows right-edge resize past the current composition duration", async () => { const clip = timelineElement({ id: "clip", track: 0, zIndex: 1, start: 6, duration: 2 }); const harness = renderDragHarness([clip]); diff --git a/packages/studio/src/player/components/useTimelineClipDrag.ts b/packages/studio/src/player/components/useTimelineClipDrag.ts index 9850bff8a..b9faa4be1 100644 --- a/packages/studio/src/player/components/useTimelineClipDrag.ts +++ b/packages/studio/src/player/components/useTimelineClipDrag.ts @@ -154,14 +154,21 @@ export function useTimelineClipDrag({ compositionDurationRef.current = compositionDuration; const buildSnapTargets = useCallback( - (element: TimelineElement) => - buildTimelineSnapTargets({ + (element: TimelineElement) => { + const draggedKey = element.key ?? element.id; + const selected = selectedElementIdsRef.current; + // In a group drag every selected clip moves together, so none of them may act + // as a snap target for the others; exclude the whole set, not just the grabbed clip. + const excludedKeys = + selected.size > 1 && selected.has(draggedKey) ? selected : new Set([draggedKey]); + return buildTimelineSnapTargets({ elements: timelineElementsRef.current, - draggedKey: element.key ?? element.id, + excludedKeys, playhead: playheadRef.current, compDuration: compositionDurationRef.current, beats: isMusicTrack(element) ? EMPTY_BEAT_TIMES : beatTimesRef.current, - }), + }); + }, [timelineElementsRef], ); diff --git a/packages/studio/src/player/components/useTimelineClipGroupDrag.ts b/packages/studio/src/player/components/useTimelineClipGroupDrag.ts index 469ab80d3..6bc6bc9ec 100644 --- a/packages/studio/src/player/components/useTimelineClipGroupDrag.ts +++ b/packages/studio/src/player/components/useTimelineClipGroupDrag.ts @@ -5,6 +5,7 @@ import type { } from "../../hooks/useTimelineGroupEditing"; import type { TimelineElement } from "../store/playerStore"; import { + getTimelineEditCapabilities, resolveTimelineGroupMove, resolveTimelineGroupResize, type TimelineGroupResizeEdge, @@ -81,14 +82,20 @@ function selectedMembers( selectedElementIdsInput: Set, timelineElements: readonly TimelineElement[], mapMember: (element: TimelineElement) => GroupTimingMember, + canEdit: (element: TimelineElement) => boolean, ): GroupTimingMember[] | null { const selectedElementIds = selectedElementSet(selectedElementIdsInput); const grabbedKey = elementKey(grabbedElement); if (selectedElementIds.size <= 1 || !selectedElementIds.has(grabbedKey)) return null; - const members = timelineElements - .filter((element) => selectedElementIds.has(elementKey(element))) - .map(mapMember); + const elements = timelineElements.filter((element) => + selectedElementIds.has(elementKey(element)), + ); + // A group edit must not touch a member that individually forbids this operation + // (e.g. a locked or implicitly-timed clip). If any member can't take it, don't form + // a group; the gesture degrades to a normal single-clip edit of the grabbed clip. + if (!elements.every(canEdit)) return null; + const members = elements.map(mapMember); return members.length > 1 ? members : null; } @@ -122,7 +129,13 @@ function createMoveSession( selectedElementIds: Set, timelineElements: readonly TimelineElement[], ): MoveSession | null { - const members = selectedMembers(element, selectedElementIds, timelineElements, moveMember); + const members = selectedMembers( + element, + selectedElementIds, + timelineElements, + moveMember, + (candidate) => getTimelineEditCapabilities(candidate).canMove, + ); if (!members) return null; return { grabbedKey: elementKey(element), @@ -138,8 +151,15 @@ function createResizeSession( timelineElements: readonly TimelineElement[], edge: TimelineGroupResizeEdge, ): ResizeSession | null { - const members = selectedMembers(element, selectedElementIds, timelineElements, (member) => - resizeMember(edge, member), + const members = selectedMembers( + element, + selectedElementIds, + timelineElements, + (member) => resizeMember(edge, member), + (candidate) => { + const caps = getTimelineEditCapabilities(candidate); + return edge === "start" ? caps.canTrimStart : caps.canTrimEnd; + }, ); if (!members) return null; return { diff --git a/packages/studio/src/player/components/useTimelineMarqueeSelection.test.tsx b/packages/studio/src/player/components/useTimelineMarqueeSelection.test.tsx index 966212ab6..525be9560 100644 --- a/packages/studio/src/player/components/useTimelineMarqueeSelection.test.tsx +++ b/packages/studio/src/player/components/useTimelineMarqueeSelection.test.tsx @@ -57,6 +57,7 @@ function renderMarqueeHarness(layers: StackingTimelineLayer[]) { const layerOrder = layers.map((item) => item.id); const setShowPopover = () => {}; const setRangeSelection = () => {}; + const seekedX: number[] = []; function Harness() { const scrollRef = React.useRef(null); @@ -67,6 +68,7 @@ function renderMarqueeHarness(layers: StackingTimelineLayer[]) { timelineLayersRef: { current: layers }, setShowPopover, setRangeSelectionRef: { current: setRangeSelection }, + seekFromX: (clientX: number) => seekedX.push(clientX), }); return (
("[data-clip]")!, unmount() { act(() => root.unmount()); @@ -152,7 +155,7 @@ describe("useTimelineMarqueeSelection", () => { harness.unmount(); }); - it("treats a sub-threshold empty-lane drag as a clear click", () => { + it("treats a sub-threshold empty-lane drag as a clear click that also seeks", () => { usePlayerStore.getState().setSelection(["selected"]); const harness = renderMarqueeHarness([layer("lane-0", [element("selected", 0, 1, 0)])]); @@ -162,6 +165,8 @@ describe("useTimelineMarqueeSelection", () => { expect(usePlayerStore.getState().selectedElementIds.size).toBe(0); expect(harness.host.querySelector("[data-marquee]")).toBeNull(); + // A sub-threshold press still scrubs the playhead to the click, like a plain lane click. + expect(harness.seekedX).toEqual([GUTTER + 20]); harness.unmount(); }); diff --git a/packages/studio/src/player/components/useTimelineMarqueeSelection.ts b/packages/studio/src/player/components/useTimelineMarqueeSelection.ts index 520865677..bab176315 100644 --- a/packages/studio/src/player/components/useTimelineMarqueeSelection.ts +++ b/packages/studio/src/player/components/useTimelineMarqueeSelection.ts @@ -44,6 +44,8 @@ interface UseTimelineMarqueeSelectionInput { disabled?: boolean; setShowPopover: (show: boolean) => void; setRangeSelectionRef: RefObject<((sel: null) => void) | null>; + /** Canonical playhead seek, used to keep empty-lane clicks scrubbing the playhead. */ + seekFromX: (clientX: number) => void; } function getCanvasPoint(scroll: HTMLDivElement, clientX: number, clientY: number) { @@ -89,6 +91,12 @@ function buildSelectionRect( const overlayRight = Math.max(overlayLeft, right); const overlayBottom = Math.max(overlayTop, bottom); + // Hit-test must use the SAME pixels-per-second the overlay is drawn at, or the + // selected time span diverges from the visible box at low zoom (pps < 1). Guard + // only against a non-finite/zero pps (would yield NaN/Infinity), never floor it. + const safePps = Number.isFinite(pps) && pps > 0 ? pps : 0; + const timeFromX = (x: number) => (safePps > 0 ? Math.max(0, (x - GUTTER) / safePps) : 0); + return { overlay: { left: overlayLeft, @@ -97,8 +105,8 @@ function buildSelectionRect( height: overlayBottom - overlayTop, }, selection: { - startTime: Math.max(0, (left - GUTTER) / Math.max(pps, 1)), - endTime: Math.max(0, (right - GUTTER) / Math.max(pps, 1)), + startTime: timeFromX(left), + endTime: timeFromX(right), top, bottom, }, @@ -113,6 +121,7 @@ export function useTimelineMarqueeSelection({ disabled = false, setShowPopover, setRangeSelectionRef, + seekFromX, }: UseTimelineMarqueeSelectionInput) { const activeRef = useRef(null); const pointerRef = useRef<{ clientX: number; clientY: number } | null>(null); @@ -231,7 +240,11 @@ export function useTimelineMarqueeSelection({ setMarqueeRect(null); if (!active.started) { + // A press that never crossed the marquee threshold is a plain empty-lane + // click: clear the selection AND scrub the playhead, matching the seek that + // the range/playhead handler would have run had the marquee not claimed it. usePlayerStore.getState().clearSelection(); + seekFromX(active.anchorClientX); return true; } @@ -249,7 +262,7 @@ export function useTimelineMarqueeSelection({ usePlayerStore.getState().setSelection(selectedIds); return true; }, - [ppsRef, scrollRef, stopAutoScroll, timelineLayersRef, trackOrderRef], + [ppsRef, scrollRef, seekFromX, stopAutoScroll, timelineLayersRef, trackOrderRef], ); useEffect(() => stopAutoScroll, [stopAutoScroll]);