Add Timeline Snap Guides#515
Conversation
📝 WalkthroughWalkthroughSnap guides fire during timeline item drag/resize. TimelineEditor routes zoom/trim/speed spans as hard constraints and annotation/blur as soft snap targets, plus playhead and keyframes. TimelineWrapper snaps live spans, shows an amber guide at the snap point, applies snapping before overlap/min-duration checks, and hides overlays on end. ChangesSnap Guide Timeline Interactions
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
can you rework this based on new ui, add video of demo when you mark it ready for review? |
|
@siddharthvaddem definitely, sounds good to me. Allow me some time (1/2 days) to get back to this and I'll have it ready for review |
|
Also, taking this little chat to mention - amazing project! love it so far! |
AbhinRustagi
force-pushed
the
feature/timeline-handle-snap
branch
from
9294b1f to
d802473
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9294b1f79d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/components/video-editor/timeline/TimelineWrapper.tsx (2)
435-468: 💤 Low valuenit: cleaner — extract the screenX calc
The
event.activatorEvent && "clientX" in event.activatorEvent ? ... : undefinedblock is identical inonDragMoveandonResizeMove. Tiny helper would tidy it up — totally skippable though, it's 5 lines.♻️ Optional helper
+ const getScreenX = useCallback( + (event: { activatorEvent: Event | null; delta?: { x: number } }): number | undefined => + event.activatorEvent && "clientX" in event.activatorEvent + ? (event.activatorEvent as PointerEvent).clientX + (event.delta?.x ?? 0) + : undefined, + [], + );Then call
getScreenX(event)in both move handlers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/video-editor/timeline/TimelineWrapper.tsx` around lines 435 - 468, Both onDragMove and onResizeMove duplicate the same screenX calculation; extract it to a small helper (e.g. getScreenX) and call it from both handlers. Create getScreenX(event: DragMoveEvent | ResizeMoveEvent) that checks event.activatorEvent && "clientX" in event.activatorEvent and returns (event.activatorEvent as PointerEvent).clientX + (event.delta?.x ?? 0) or undefined, then replace the inline ternary in onDragMove and onResizeMove with const screenX = getScreenX(event) and keep existing calls to showTooltip(span, screenX).
199-297: 💤 Low valuesnap math looks solid; tiny note on tie-breaks + per-move allocations
Threshold scaling with visible range is the right call, and the resize branches correctly bail when snapping would shrink below
minItemDurationMs— good guardrail.Two small things worth a glance:
findNearestuses<=, so when two targets are equidistant, the later-inserted one wins. Given the insertion order (0/totalMs→ hard regions → soft regions → playhead → keyframes), ties resolve to keyframes > playhead > soft > hard > bounds. Probably fine, but if you expected playhead/timeline-bounds to have priority on ties, flipping to<would do it.targetSetis rebuilt on every pointer move. For most projects this is nothing, but on a heavy timeline (lots of regions + many keyframes) it's allocating aSetand anArraypermousemove. The static portion (bounds + non-active region edges + keyframes) could be precomputed in auseMemokeyed on the same deps and just filtered for the active id at call time. Defer if not visible in profiling.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/video-editor/timeline/TimelineWrapper.tsx` around lines 199 - 297, The snapSpanToTargets logic needs two small fixes: (1) change the tie-break in findNearest from "<=" to "<" so earlier-inserted targets win ties (flip from <= to < in the comparison inside findNearest), and (2) avoid rebuilding targetSet on every pointer move by memoizing the static targets (bounds, allRegionSpans/softSnapSpans edges, keyframeTimesMs, currentTimeMs) with useMemo and then filter out the activeItemId inside snapSpanToTargets before calling findNearest; reference functions/ids: snapSpanToTargets, findNearest, targetSet, allRegionSpans, softSnapSpans, keyframeTimesMs, useMemo.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/components/video-editor/timeline/TimelineWrapper.tsx`:
- Around line 435-468: Both onDragMove and onResizeMove duplicate the same
screenX calculation; extract it to a small helper (e.g. getScreenX) and call it
from both handlers. Create getScreenX(event: DragMoveEvent | ResizeMoveEvent)
that checks event.activatorEvent && "clientX" in event.activatorEvent and
returns (event.activatorEvent as PointerEvent).clientX + (event.delta?.x ?? 0)
or undefined, then replace the inline ternary in onDragMove and onResizeMove
with const screenX = getScreenX(event) and keep existing calls to
showTooltip(span, screenX).
- Around line 199-297: The snapSpanToTargets logic needs two small fixes: (1)
change the tie-break in findNearest from "<=" to "<" so earlier-inserted targets
win ties (flip from <= to < in the comparison inside findNearest), and (2) avoid
rebuilding targetSet on every pointer move by memoizing the static targets
(bounds, allRegionSpans/softSnapSpans edges, keyframeTimesMs, currentTimeMs)
with useMemo and then filter out the activeItemId inside snapSpanToTargets
before calling findNearest; reference functions/ids: snapSpanToTargets,
findNearest, targetSet, allRegionSpans, softSnapSpans, keyframeTimesMs, useMemo.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 76ee3694-d13d-4dc7-9014-c24878491696
📒 Files selected for processing (2)
src/components/video-editor/timeline/TimelineEditor.tsxsrc/components/video-editor/timeline/TimelineWrapper.tsx
|
@siddharthvaddem This is ready for review |
|
@AbhinRustagi - I saw the video, thanks for referencing the issue I created. Does it also snap to the timeline cursor? or only to edges of other clips? I use the snap to cursor a lot in Capcut and it's great for my editing workflow. Esp. after you find a frame, I place my cursor there, and then drag clips in other layer to the same frame, or sound effects etc. |
3 participants
Pull Request Template
Description
Adds magnetic snap behavior to timeline handles. When dragging or resizing a trim, zoom, speed, annotation, or blur region, its edges snap to nearby region edges, the playhead, keyframes, and timeline bounds. A live amber guideline shows the active snap target during interaction; the drag tooltip previews the snapped values; on release, the region commits to the snapped position.
Motivation
Aligning two regions on the timeline is eyeball work right now
Type of Change
Related Issue(s)
Closes #514 and partially #511
Screenshots / Video
Screenshot (if applicable):
snap.mp4
Testing
playhead, so the green playback cursor isn't doubled.)
Checklist
Thank you for contributing!
Summary by CodeRabbit