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.
This commit is contained in:
Miguel Angel Simon Sierra
2026-07-09 16:54:34 -04:00
parent 3c6c1d3f27
commit 04ddd411ec
9 changed files with 140 additions and 34 deletions
@@ -99,17 +99,20 @@ export function useTimelineGroupEditing({
(label: string, operation: (projectId: string) => Promise<void>): Promise<void> => {
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],
);
@@ -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<typeof handleMarqueePointerUp>[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}
>
<TimelineCanvas
major={major}
@@ -34,7 +34,7 @@ describe("buildTimelineSnapTargets", () => {
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],
@@ -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<string>;
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" });
}
@@ -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]);
@@ -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],
);
@@ -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<string>,
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<string>,
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 {
@@ -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<HTMLDivElement | null>(null);
@@ -67,6 +68,7 @@ function renderMarqueeHarness(layers: StackingTimelineLayer[]) {
timelineLayersRef: { current: layers },
setShowPopover,
setRangeSelectionRef: { current: setRangeSelection },
seekFromX: (clientX: number) => seekedX.push(clientX),
});
return (
<div
@@ -115,6 +117,7 @@ function renderMarqueeHarness(layers: StackingTimelineLayer[]) {
host,
scroll,
root,
seekedX,
clip: host.querySelector<HTMLElement>("[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();
});
@@ -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<ActiveMarqueeGesture | null>(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]);