desktop app: improve trimming behaviour #1334
Conversation
WalkthroughIntroduces a shared drag-tracking state to coordinate dragging across timeline segments, clamps start/end to avoid overlaps, adjusts z-index during active drags, and tightens zoom-segment proposal guards to only return proposals when minimum duration and next-segment bounds permit. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ClipTrack as ClipTrack Component
participant ActiveState as activeDragState (shared)
participant PrevSegment as Previous Segment
participant Renderer
Note over User,ClipTrack: Start dragging a segment handle
User ->> ClipTrack: pointerdown(start handle)
ClipTrack ->> ClipTrack: compute local offset, set startHandleDrag
ClipTrack ->> ActiveState: setActiveDragState({ index, offset })
ActiveState -->> ClipTrack: shared state stored
Note over ClipTrack,PrevSegment: During drag updates
User ->> ClipTrack: pointermove(delta)
ClipTrack ->> ClipTrack: compute proposedStart = base + localOffset + prevOffset
ClipTrack ->> ClipTrack: clampedStart = clamp(proposedStart, min, prevEnd)
ClipTrack ->> Renderer: render segment with clampedStart (z-50 if active)
Note over ClipTrack,ActiveState: On drag end/cleanup
User ->> ClipTrack: pointerup
ClipTrack ->> ClipTrack: clear startHandleDrag
ClipTrack ->> ActiveState: setActiveDragState(null)
ClipTrack ->> Renderer: render final state
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (2)**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧬 Code graph analysis (1)apps/desktop/src/routes/editor/Timeline/ClipTrack.tsx (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (4)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/desktop/src/routes/editor/Timeline/ZoomTrack.tsx (1)
119-133: Redundant space check when next segment exists.When
nextSegmentexists (lines 111-125), the code checksspaceToNext < minDurationat line 121 and then again checksmax - previewTime < minDurationat line 133 (wheremax === nextSegment.start). These are identical checks. The guard at line 133 is only necessary when there's no next segment.Consider restructuring to avoid the duplicate check:
if (Option.isSome(nextSegment)) { if (Option.isSome(prevSegment)) { const availableTime = nextSegment.value[0].start - prevSegment.value[0].end; if (availableTime < minDuration) return; } - // Check if there's space between preview time and next segment - const spaceToNext = nextSegment.value[0].start - previewTime; - if (spaceToNext < minDuration) { - // If there's not enough space for minimum duration, don't show proposed zoom - return; - } } const max = nextSegment.pipe( Option.map(([s]) => s.start), Option.getOrElse(() => totalDuration()), ); // Check if there's enough space before returning if (max - previewTime < minDuration) return;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/desktop/src/routes/editor/Timeline/ClipTrack.tsx(5 hunks)apps/desktop/src/routes/editor/Timeline/ZoomTrack.tsx(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use a 2-space indent for TypeScript code.
Use Biome for formatting and linting TypeScript/JavaScript files by runningpnpm format.
Files:
apps/desktop/src/routes/editor/Timeline/ClipTrack.tsxapps/desktop/src/routes/editor/Timeline/ZoomTrack.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Use kebab-case for filenames for TypeScript/JavaScript modules (e.g.,user-menu.tsx).
Use PascalCase for React/Solid components.
Files:
apps/desktop/src/routes/editor/Timeline/ClipTrack.tsxapps/desktop/src/routes/editor/Timeline/ZoomTrack.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (9)
apps/desktop/src/routes/editor/Timeline/ClipTrack.tsx (5)
200-204: LGTM! Good use of shared state for drag coordination.The
activeDragStatesignal appropriately coordinates dragging across multiple timeline segments, enabling the behavior described in the PR objectives where trimming the first clip no longer causes it to go underneath the following clip.
231-256: LGTM! Offset propagation logic correctly implements the improved trimming behavior.The calculation properly propagates drag offsets from previous segments to following segments, preventing clips from going underneath each other during trimming operations. The use of
Math.maxensures the start position remains non-negative.
406-406: LGTM! Appropriate z-index stacking during drag.The conditional
z-50class ensures the segment being dragged appears on top of other segments, providing clear visual feedback during the interaction.
566-577: LGTM! Correct offset calculation and state synchronization.The offset is properly computed as the difference between the clamped start position and the initial start position. Both local and shared drag states are updated consistently, enabling coordinated behavior across segments.
590-595: LGTM! Proper cleanup of drag state.The cleanup correctly resets both local (
startHandleDrag) and shared (activeDragState) drag states, preventing stale state from affecting subsequent drag operations.apps/desktop/src/routes/editor/Timeline/ZoomTrack.tsx (4)
119-124: LGTM! Appropriate guard against insufficient space.This guard correctly prevents proposing a new zoom segment when there isn't enough space between the preview time and the next segment, improving the boundary handling as described in the PR objectives.
127-130: LGTM! Good refactor to compute max boundary once.Computing the
maxvalue once and reusing it improves clarity and eliminates duplicate logic. The fallback tototalDuration()when there's no next segment is correct.
139-139: LGTM! Consistent use of computed max boundary.Using the pre-computed
maxvalue ensures consistency with the boundary checks above and improves code clarity.
425-425: LGTM! Proper z-index stacking for visual clarity.The z-index adjustments ensure actual zoom segments (
z-10) appear above the placeholder segment (z-0), providing clear visual feedback during timeline interactions. This coordinates with thez-50applied in ClipTrack.tsx for dragged segments.Also applies to: 616-616
This PR: previously if you trim the first clip or segment, it would go under the next one. This PR prevents that.
Screen.Recording.2025-10-31.at.16.43.11.mov
Summary by CodeRabbit
Bug Fixes
UI