fix: dragging a multi-selection only moved the grabbed clip - #10
fix: dragging a multi-selection only moved the grabbed clip#10GinoongFlores wants to merge 1 commit into
Conversation
- add MoveElementsCommand: batch-moves several elements as one undo step, each staying on its own track (no cross-track drop for group drags) - use-element-interaction: snapshot every selected clip's startTime at drag start when the grabbed clip is part of a multi-selection, shift them all by the same delta on drop, block the move if it would overlap a non-selected clip - new-track drops still fall back to single-element move (out of scope)
![vorflux[bot]](https://avatars.githubusercontent.com/in/2483280?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
Reviewed — found 4 issue(s). This PR adds group-drag support for multi-selected clips via a new MoveElementsCommand and logic in use-element-interaction.ts. The approach is sound — it reuses the same snapshot-undo pattern as the existing single-element command. However, the new command is missing enforceMainTrackStart, which can leave gaps at the start of the main video track, and per-element Math.max(0, …) clamping can cause undetected intra-group overlaps.
Findings
apps/web/src/lib/commands/timeline/element/move-elements.ts
-
Missing
enforceMainTrackStartcall inexecute(). The existingMoveElementCommand.execute()callsenforceMainTrackStartto ensure the main video track always starts at time 0.MoveElementsCommand.execute()never does. If a selected element on the main track is dragged left far enough, it can land at a non-zero time, leaving a gap at the track start and violating the invariant. See inline comment for fix suggestion. -
Silently skips missing elements. When a
trackId:elementIdpair isn't found in the current tracks, the command silently continues — unlikeMoveElementCommandwhich throws. This makes debugging harder if stale element IDs slip in. See inline comment. -
No unit tests. The new
MoveElementsCommandand group-drag logic inuse-element-interaction.tshave zero test coverage. The project already uses Bun for testing (keyframe-aware-commands.test.tsas reference); tests should cover execute/undo/redo, empty updates, and main-track pinning.
apps/web/src/hooks/timeline/element/use-element-interaction.ts
Math.max(0, …)clamping can cause undetected intra-group overlaps. Each element's new start is clamped individually tomax(0, start + delta). If the delta is negative enough to clamp one element on a track to 0 but not another, their relative positions shift and they can overlap — but both are excluded from the collision check. See inline comment.
Verdict
MoveElementsCommand as a blocking precondition.
| execute(): void { | ||
| const editor = EditorCore.getInstance(); | ||
| this.savedState = editor.timeline.getTracks(); | ||
|
|
||
| const newStartTimeByKey = new Map( | ||
| this.updates.map((update) => [ | ||
| `${update.trackId}:${update.elementId}`, | ||
| update.newStartTime, | ||
| ]), | ||
| ); | ||
|
|
||
| const updatedTracks = this.savedState.map((track): TimelineTrack => { | ||
| if (!("elements" in track)) return track; | ||
| let changed = false; | ||
| const elements = track.elements.map((element) => { | ||
| const newStartTime = newStartTimeByKey.get(`${track.id}:${element.id}`); | ||
| if (newStartTime === undefined) return element; | ||
| changed = true; | ||
| return { ...element, startTime: newStartTime }; | ||
| }); | ||
| return changed ? ({ ...track, elements } as typeof track) : track; | ||
| }); | ||
|
|
||
| editor.timeline.updateTracks(updatedTracks); | ||
| } |
There was a problem hiding this comment.
Missing enforceMainTrackStart — main track invariant can be violated.
The existing MoveElementCommand.execute() (above this class) calls enforceMainTrackStart to ensure the main video track always starts at time 0. MoveElementsCommand.execute() never calls it.
If a selected element lives on the main track and the group is dragged left far enough, that element can land at a non-zero time while still being the earliest element on the track — leaving a gap at the track start.
Suggested fix: For each update targeting the main track, call enforceMainTrackStart after computing the updated tracks. Note that enforceMainTrackStart takes a single excludeElementId, so you may need to iteratively enforce or extend it to accept a set of excluded IDs.
| const updatedTracks = this.savedState.map((track): TimelineTrack => { | ||
| if (!("elements" in track)) return track; | ||
| let changed = false; | ||
| const elements = track.elements.map((element) => { | ||
| const newStartTime = newStartTimeByKey.get(`${track.id}:${element.id}`); | ||
| if (newStartTime === undefined) return element; | ||
| changed = true; | ||
| return { ...element, startTime: newStartTime }; | ||
| }); | ||
| return changed ? ({ ...track, elements } as typeof track) : track; | ||
| }); |
There was a problem hiding this comment.
Silently skips missing elements — no error or warning.
When a trackId:elementId key isn't found in the current tracks (newStartTime === undefined), the command silently continues without marking changed = true. The existing MoveElementCommand throws "Source track or element not found" in the same situation.
In practice this shouldn't happen during normal drag, but it means stale element IDs from a bug elsewhere would produce silent no-ops — no error, toast, or warning. Consider logging a warning (at minimum) or throwing to match the existing command's behavior.
| const updates = groupDrag.map((g) => ({ | ||
| trackId: g.trackId, | ||
| elementId: g.elementId, | ||
| newStartTime: Math.max(0, g.startTime + delta), | ||
| })); |
There was a problem hiding this comment.
Per-element Math.max(0, …) clamping can cause undetected intra-group overlaps.
Each element's new start is clamped individually (Math.max(0, g.startTime + delta)), but the collision check (hasCollision) filters out all group members via movingElementIds. If the delta is negative enough to clamp one element on a track to 0 but not another on the same track, their relative positions shift and they can overlap — undetected.
Example: track has selected elements A at time=2, B at time=12. Delta=-5. A → max(0, -3) = 0, B → max(0, 7) = 7. If A.duration > 7, they overlap.
Suggested fix: After computing updates, re-check pairwise overlaps among moved elements that share the same track. Or clamp the entire delta uniformly so no element goes negative: clampedDelta = max(0 - minStartTime, delta).
| groupDragRef.current = selectedElements | ||
| .map((selected) => { | ||
| const selectedTrack = tracks.find((t) => t.id === selected.trackId); | ||
| const selectedElement = selectedTrack?.elements.find( | ||
| (el) => el.id === selected.elementId, | ||
| ); | ||
| return selectedElement | ||
| ? { | ||
| trackId: selected.trackId, | ||
| elementId: selected.elementId, | ||
| startTime: selectedElement.startTime, | ||
| } | ||
| : null; | ||
| }) | ||
| .filter((entry) => entry !== null); |
There was a problem hiding this comment.
groupDragRef snapshot silently filters out deleted elements.
At mousedown, groupDragRef.current maps selectedElements through tracks.find() and filters out null entries. If elements from the selection no longer exist in tracks (stale selection from a prior bug), the ref becomes empty ([]) and the group-drag branch is skipped — only the grabbed clip moves, not the multi-selection.
This is a hard-to-hit edge case, but a defensive console.warn when entries are filtered out would aid debugging.
Risk AssessmentThe PR adds group-drag support with a well-structured approach, but the new Risk score: 4/10 (threshold: 3) — above the configured risk threshold Concerns
Verdict
|
1 participant
Selecting several clips and dragging one only moved that one clip; the rest stayed put.
Root cause: the drag hook only ever tracked a single element (
dragState.elementId/trackId), regardless of how many clips were selected.MoveElementsCommand: batch-moves several elements as one undo step, each staying on its own trackuse-element-interaction: snapshot every selected clip's startTime at drag start when the grabbed clip is part of a multi-selection, shift them all by the same delta on drop, block the move if it would overlap a non-selected clip