Skip to content

feat(studio): clip drag engine — preview and commit (unwired)#2206

Open
ukimsanov wants to merge 1 commit into
studio-dnd/pr14-glue-api-coexistencefrom
studio-dnd/pr15-clip-drag-engine
Open

feat(studio): clip drag engine — preview and commit (unwired)#2206
ukimsanov wants to merge 1 commit into
studio-dnd/pr14-glue-api-coexistencefrom
studio-dnd/pr15-clip-drag-engine

Conversation

@ukimsanov

@ukimsanov ukimsanov commented Jul 11, 2026

Copy link
Copy Markdown
Collaborator

What

the NLE clip-drag engine core: timelineClipDragPreview (zone-aware preview placement; keeps the legacy resolveDragPreviewPlacement export until the timeline swap), timelineClipDragCommit (drop → move/insert commit, stacking-synced) and timelineElementsMove (batched ripple/insert move with GSAP position shifting), with test suites.

Why

the drag-drop brain, reviewable as functions before any component mounts it.

How

new files plus one authored intermediate (preview file = final content + legacy export the old engine still imports); consumes the collision/ snapping/zones/stacking modules and the coexistence layer.

Test plan

bunx vitest run on both test suites; tsc --noEmit; fallow audit clean.


Stack position 15/25 — studio NLE overhaul (CapCut-parity timeline + canvas). Graphite manages bases; merge from #2192 upward. Temporary TEMP(studio-dnd) fallow entries (unwired-component windows) are all removed by #2213 (app-shell swap), whose tree is byte-identical to the fully-verified integration branch.

ukimsanov commented Jul 11, 2026

Copy link
Copy Markdown
Collaborator Author

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

@miga-heygen miga-heygen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed as part of the 25-PR NLE overhaul stack. Full stack review on #2205 (the keystone). No blockers on this PR. — Miga

@vanceingalls vanceingalls left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

R1 Batch-D deep-dive — #2206 clip-drag engine

Reviewed as the NLE drag brain that the coexistence layer (#2205) makes typeable. SHA b7eee0d. Read all three new files at head; cross-checked against #2205's bivariant callback contract and #2212's legacy-export removal.

Consumer-invariant audit

Shape contract with #2205's TimelineGroupMoveEdit.
timelineClipDragCommit.ts:7-10 defines a local TimelineMoveEdit = { element, updates: Pick<TimelineElement, "start" | "track"> }. Structurally identical to #2205's TimelineGroupMoveEdit (timelineCallbacks.ts:80-83). The local declaration is used only inside timelineClipDragCommit.ts and timelineElementsMove.ts (also declared locally there as TimelineElementMoveEdit, line 22-25) — same shape three times. This is defensible under the "reviewable as functions before any component mounts it" framing (the engine files stand alone), but it's a watch-note for the swap PR (#2212): once wiring lands, the callback boundary flows through TimelineGroupMoveEdit from timelineCallbacks.ts — the three local structural clones should be replaced with the single canonical import so tsc catches drift if the shape ever widens (e.g. adding an optional stackingReorder field only to one). Miga's higher-level review noted a similar SSOT gap on overlapsInTime between #2198/#2199; this is the analogous smell in this PR.

Legacy export retention until the swap.
PR body claims: keeps resolveDragPreviewPlacement export until the timeline swap. Verified at timelineClipDragPreview.ts:290. Verified #2212 diff on the same file deletes exactly that export (-export function resolveDragPreviewPlacement(). Contract honored.

Interaction with Batch-A's math modules.

  • computeDragPreview (line 98) correctly consumes:
    • resolveTimelineMove from #2195 collision (line 3 import).
    • snapMoveToTargets + snapTimelineTime + TIMELINE_SNAP_PX from #2196 snapping (line 7-12).
    • resolveInsertRow + resolveZoneDropPlacement from #2199 zones (line 13).
    • clampGroupMoveDelta from #2197 multi-drag preview (line 14) — the rigid formation clamp.
  • commitDraggedClipMove (line 91) reuses normalizeToZones (#2199) after applying the drop, and computeStackingPatches (#2198) for lane↔z sync. The sync engagement gate is correct — if (!readZIndex || !onStackingPatches) return; (line 171) means outside the NLE (standalone <Timeline>) the z-sync is a no-op. Good coexistence hygiene.

Multi-selection handling.
Group drag: commitDraggedClipMove:98-99 engages the multi-drag ONLY when the dragged clip is in the selection AND selection size > 1 — the standard NLE gesture. Track changes apply to the dragged clip only (line 129 track: targetTrack on drag; line 130 preserves track: e.track on passengers). Time delta clamped at zero via movedStart (line 101-102: Math.max(0, round3(e.start + delta))) — matches the leftmost-at-0 constraint from #2197.

Fine but flag-worthy: edits.filter((e) => e.updates.start !== e.element.start) at line 111 drops passengers whose clamped start didn't change (e.g. group can't slide left of a passenger at 0). Good — no phantom no-op writes. However, the same filter is NOT applied on the topology-change branch (line 141): every clip normalized-to-a-new-track is persisted, including those that keep their old start. That's correct because normalize-to-zones may have relocated their lane, but readers of the persist path should know the two branches have different filter policies. Docstring at line 82-88 covers the intent; enough.

Three-phase persist (Miga's line-noted nit).
persistMoveEdits (line 49-65) is fire-and-forget with rollback on rejection (line 61-64). The catch handler is console.error only — the caller cannot observe the failure. Miga already noted this: worth documenting for downstream callers, but not a blocker since the optimistic UI is rolled back either way. My addition: the rollback restores { start, track } verbatim from prev — if a concurrent store mutation happened between the optimistic apply and the catch (e.g. from an unrelated undo), rollback overwrites that. Extremely narrow race; the store's operations are synchronous and the persist path awaits its promise synchronously enough that the window is tiny. Accepted as-is.

persistTimelineElementsMove atomicity (in timelineElementsMove.ts).
Three phases (patch-in-memory → atomic write → post-save preview sync) — Miga captured this well. My added observation: patchTimelineMoveGroups:113 short-circuits if (patched === original) continue; per-group. Correct — a no-op group is skipped from filesToSave. But a group with a subset of edits that individually no-op would still land in groupResults if ANY edit changed the patched string; the shift-computation phase 3 (line 141-146) then correctly filters shifts to updates.start - element.start !== 0. Correct end-to-end.

Test coverage on timelineElementsMove.test.ts (+157 lines) and timelineClipDragCommit.test.ts (+242 lines) is thorough on the pure-function boundary. Not a substitute for the deleted useTimelineEditing.test.tsx integration suite (Miga's #2213 nit) — that's a stack-level follow-up.

Overall

🟢 with two watch-notes for the swap PR (#2212):

  1. Consolidate the three structurally-identical local edit shapes (TimelineMoveEdit, TimelineElementMoveEdit, and the canonical TimelineGroupMoveEdit from #2205's timelineCallbacks.ts) to a single import once the engine is wired — prevents silent drift.
  2. Fire-and-forget error path in persistMoveEdits: consider a typed error callback on DragCommitDeps so a UI toast can surface the rollback (currently only console.error).

Neither is blocking at #2206. Solid module boundary, well-tested pure functions, cleanly consumes the coexistence-layer types.

R1 by Via

@ukimsanov ukimsanov force-pushed the studio-dnd/pr14-glue-api-coexistence branch from a2f4dd0 to 7d93612 Compare July 11, 2026 02:56
@ukimsanov ukimsanov force-pushed the studio-dnd/pr15-clip-drag-engine branch 2 times, most recently from 0d3d115 to de842df Compare July 11, 2026 08:18
@ukimsanov ukimsanov force-pushed the studio-dnd/pr14-glue-api-coexistence branch from 7d93612 to dbb7d62 Compare July 11, 2026 08:18
@ukimsanov ukimsanov marked this pull request as ready for review July 11, 2026 09:06
@ukimsanov ukimsanov force-pushed the studio-dnd/pr15-clip-drag-engine branch from de842df to 4500814 Compare July 11, 2026 10:49
What: the NLE clip-drag engine core: timelineClipDragPreview (zone-aware
preview placement; keeps the legacy resolveDragPreviewPlacement export until
the timeline swap), timelineClipDragCommit (drop → move/insert commit,
stacking-synced) and timelineElementsMove (batched ripple/insert move with
GSAP position shifting), with test suites.

Why: the drag-drop brain, reviewable as functions before any component
mounts it.

How: new files plus one authored intermediate (preview file = final content
+ legacy export the old engine still imports); consumes the collision/
snapping/zones/stacking modules and the coexistence layer.

Test plan: bunx vitest run on both test suites; tsc --noEmit; fallow audit
clean.
@ukimsanov ukimsanov force-pushed the studio-dnd/pr15-clip-drag-engine branch from 4500814 to 61bd37e Compare July 11, 2026 12:51
@ukimsanov ukimsanov force-pushed the studio-dnd/pr14-glue-api-coexistence branch from 0a4df4f to 4675cb9 Compare July 11, 2026 12:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants