Fix trimming behavior #1272
Fix trimming behavior #1272
Conversation
WalkthroughAdds per-segment drag signals and computes a memoized relative segment (start/ end) that reflects drag offsets; updates rendering to use the relativeSegment; and extends the drag lifecycle with move/start/end handlers, listener disposal, and history resumption. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as ClipTrack UI
participant Signals as Drag Signals
participant Memo as relativeSegment Memo
User->>UI: mousedown on segment handle
UI->>Signals: set initialStart, isDragging = true
UI->>UI: attach move & end listeners
User->>UI: mousemove (drag)
UI->>Signals: update dragOffset (delta / timescale)
Signals->>Memo: invalidate / recompute
Memo->>UI: provide updated relativeSegment
UI->>UI: re-render segment position/width
User->>UI: mouseup / blur / mouseleave
UI->>Signals: isDragging = false, reset dragOffset
UI->>UI: remove listeners, resume history, call onHandleReleased
Memo->>UI: final relativeSegment
UI->>UI: final render
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/routes/editor/Timeline/ClipTrack.tsx (1)
452-474: Fix timescale handling in drag calculation.The mouse movement is converted to timeline seconds via
secsPerPixel(), butsegment.startis in recording seconds. Whentimescale !== 1, this causes incorrect drag behavior.Apply this diff to account for timescale:
function update(event: MouseEvent) { const newStart = start + - (event.clientX - downEvent.clientX) * secsPerPixel(); + (event.clientX - downEvent.clientX) * secsPerPixel() * segment.timescale; // Update visual offset in recording seconds (scaled in relativeSegment) setDragOffset(newStart - initialStart());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/desktop/src/routes/editor/Timeline/ClipTrack.tsx(7 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{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.Use strict TypeScript and avoid any; leverage shared types
Files:
apps/desktop/src/routes/editor/Timeline/ClipTrack.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.tsx
apps/desktop/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/desktop/src/**/*.{ts,tsx}: Desktop icons are auto-imported (unplugin-icons); do not import icons manually
Desktop IPC: Call generated tauri_specta commands/events; listen to generated events and use typed interfaces
Use @tanstack/solid-query for server state in the desktop app
Files:
apps/desktop/src/routes/editor/Timeline/ClipTrack.tsx
🧬 Code graph analysis (1)
apps/desktop/src/routes/editor/Timeline/ClipTrack.tsx (1)
apps/desktop/src/routes/editor/Timeline/Track.tsx (2)
useSegmentTranslateX(28-41)useSegmentWidth(43-47)
⏰ 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). (2)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
🔇 Additional comments (4)
apps/desktop/src/routes/editor/Timeline/ClipTrack.tsx (4)
11-11: LGTM: Import addition supports new drag state.The
createSignalimport is correctly added to support the per-segment drag state management introduced in this PR.
208-233: LGTM: Core fix for trimming behavior.The introduction of per-segment drag state with memoized relative positioning correctly addresses the reported issue. By computing the segment's timeline position from
prevDuration()instead ofsegment.start, the segment no longer shifts in the timeline when the start handle is dragged. The visual offset during dragging provides immediate feedback while maintaining the correct final position.
235-236: LGTM: Consistent use of relativeSegment for positioning.The positioning logic correctly uses
relativeSegmentwhile the waveform rendering (line 409) appropriately continues to use the originalsegmentwith recording timestamps.Also applies to: 379-379
528-602: ****The asymmetry between start and end handle drag state is intentional and correct. The
isDragging/dragOffsetmechanism in the start handle provides visual feedback by offsetting the segment's position during the drag. The end handle doesn't need this because it only modifies the segment's duration (end time), not its position—applying a position offset would be incorrect. Theblurandmouseleavehandlers correctly omit theonHandleReleased()call since they're cleanup handlers, not commit handlers;onHandleReleased()appropriately updates the timeline zoom state only on successful drag completion (mouseup).Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/desktop/src/routes/editor/Timeline/ClipTrack.tsx(7 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{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.Use strict TypeScript and avoid any; leverage shared types
Files:
apps/desktop/src/routes/editor/Timeline/ClipTrack.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.tsx
apps/desktop/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/desktop/src/**/*.{ts,tsx}: Desktop icons are auto-imported (unplugin-icons); do not import icons manually
Desktop IPC: Call generated tauri_specta commands/events; listen to generated events and use typed interfaces
Use @tanstack/solid-query for server state in the desktop app
Files:
apps/desktop/src/routes/editor/Timeline/ClipTrack.tsx
🧬 Code graph analysis (1)
apps/desktop/src/routes/editor/Timeline/ClipTrack.tsx (1)
apps/desktop/src/routes/editor/Timeline/Track.tsx (2)
useSegmentTranslateX(28-41)useSegmentWidth(43-47)
⏰ 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). (2)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
🔇 Additional comments (7)
apps/desktop/src/routes/editor/Timeline/ClipTrack.tsx (7)
11-11: LGTM! Import added correctly.The
createSignalimport is necessary for the new drag state management.
208-210: Good drag state management design.The per-segment signals correctly isolate drag state for each timeline segment, enabling independent drag interactions.
212-233: Excellent use of memoization for drag offset calculations.The
relativeSegmentmemo correctly handles timescale conversion:dragOffset(in segment time) is divided by timescale to get timeline time, then applied to both start and end to maintain duration. The zero-guard on line 225 prevents potential division errors.
235-236: Correct integration with position hooks.Passing
relativeSegmentto the position hooks ensures the UI reflects drag offsets in real-time.
420-421: Good initialization of drag state.Capturing
initialStartand settingisDraggingon mousedown provides the foundation for tracking drag deltas.
455-457: Correct timescale handling in drag calculation.Multiplying by
segment.timescale(line 455) correctly converts mouse delta from screen space to segment time, anddragOffsetproperly stores the difference for visual feedback viarelativeSegment.
483-502: Past review comment addressed for start handle.All three drag-end handlers (mouseup, blur, mouseleave) now properly call
onHandleReleased(), dispose listeners, resume history, and reset drag state. This ensures consistent cleanup regardless of how the drag ends.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/desktop/src/routes/editor/Timeline/ClipTrack.tsx (2)
452-472: Potential visual mismatch when start handle dragging is clamped.The
dragOffsetis computed from the unclampednewStart(line 457), butsegment.startis updated to a clamped value (lines 464-471). When clamping occurs (e.g., when dragging beyond segment boundaries), thedragOffsetand the actualsegment.startchange won't match, potentially causing a visual discrepancy between the rendered position (which usesdragOffset) and the underlying data.Consider computing
dragOffsetafter determining the final clamped value:const newStart = start + (event.clientX - downEvent.clientX) * secsPerPixel() * segment.timescale; -setDragOffset(newStart - initialStart()); -setProject( - "timeline", - "segments", - i(), - "start", - Math.min( - Math.max( - newStart, - prevSegmentIsSameClip ? prevSegment.end : 0, - segment.end - maxDuration, - ), - segment.end - 1, - ), -); +const clampedStart = Math.min( + Math.max( + newStart, + prevSegmentIsSameClip ? prevSegment.end : 0, + segment.end - maxDuration, + ), + segment.end - 1, +); + +setDragOffset(clampedStart - initialStart()); +setProject("timeline", "segments", i(), "start", clampedStart);
531-603: End handle doesn't manage drag state (isDragging/dragOffset).The start handle's drag logic sets
isDragging(true)and managesdragOffset(lines 420-421, 484-485, 492-493, 499-500), but the end handle's drag logic (lines 531-603) doesn't touch these signals. This creates an inconsistency: if you drag the end handle,isDragging()remainsfalseanddragOffset()remains0.If the end handle doesn't need drag offset visualization (because it doesn't shift the segment's timeline position, only changes width), this asymmetry is fine. However, if there's any rendering or logic that checks
isDragging()for the segment (e.g., visual feedback), it won't work when dragging the end handle.Consider either:
- Document why end handle doesn't need drag state (if intentional)
- Add drag state management to end handle for consistency:
onMouseDown={(downEvent) => { const end = segment.end; + setIsDragging(true); if (split()) return; // ... rest of the logic createEventListenerMap(window, { mousemove: update, mouseup: (e) => { dispose(); resumeHistory(); update(e); + setIsDragging(false); onHandleReleased(); }, blur: () => { dispose(); resumeHistory(); + setIsDragging(false); onHandleReleased(); }, mouseleave: () => { dispose(); resumeHistory(); + setIsDragging(false); onHandleReleased(); }, }); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/desktop/src/routes/editor/Timeline/ClipTrack.tsx(7 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{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.Use strict TypeScript and avoid any; leverage shared types
Files:
apps/desktop/src/routes/editor/Timeline/ClipTrack.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.tsx
apps/desktop/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/desktop/src/**/*.{ts,tsx}: Desktop icons are auto-imported (unplugin-icons); do not import icons manually
Desktop IPC: Call generated tauri_specta commands/events; listen to generated events and use typed interfaces
Use @tanstack/solid-query for server state in the desktop app
Files:
apps/desktop/src/routes/editor/Timeline/ClipTrack.tsx
🧬 Code graph analysis (1)
apps/desktop/src/routes/editor/Timeline/ClipTrack.tsx (1)
apps/desktop/src/routes/editor/Timeline/Track.tsx (2)
useSegmentTranslateX(28-41)useSegmentWidth(43-47)
⏰ 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). (2)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
🔇 Additional comments (2)
apps/desktop/src/routes/editor/Timeline/ClipTrack.tsx (2)
489-502: Start handle: blur and mouseleave handlers don't finalize drag position.The
mouseuphandler callsupdate(e)(line 482) to finalize the drag position before cleanup, but theblurandmouseleavehandlers skip this step. If the drag ends via blur or mouse leaving the window, the segment won't be updated to the final drag position.Apply this diff to ensure the drag position is finalized:
blur: () => { + // Note: We can't call update() here as we don't have the mouse event. + // The segment will remain at the last mousemove position. dispose(); resumeHistory(); setIsDragging(false); setDragOffset(0); onHandleReleased(); }, mouseleave: () => { + // Note: We can't call update() here as we don't have the mouse event. + // The segment will remain at the last mousemove position. dispose(); resumeHistory(); setIsDragging(false); setDragOffset(0); onHandleReleased(); },Alternatively, if this behavior is intentional (keeping the last mousemove position), the comments above clarify the design choice.
208-233: LGTM: Well-structured drag state management.The per-segment drag state (signals and memos) is well-designed:
prefixOffsetsandprevDurationcorrectly compute cumulative timeline positionsrelativeSegmentcleanly applies drag offset for visual rendering while preserving segment data- The reactive pattern ensures UI updates correctly during drag lifecycle
1 participant
Fixes #824
https://github.com/user-attachments/assets/e73d501a-0451-4cbe-9f74-78cd065b0ad1
Summary by CodeRabbit