desktop: zoom delete bug fix + mouse and drag#1213
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds drag-to-create and live-drag editing for zoom segments on the Timeline, tracks which sub-track is hovered, and moves hover state into the shared editor context. Creation enforces minimum width/duration, clamps to neighbors/totalDuration, and finalizes or defaults a 1s segment on mouseup. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ZoomTrack as ZoomTrack Component
participant Editor as Editor Context / State
participant Store as ZoomSegments Store
User->>ZoomTrack: mousedown (record start pos/time)
ZoomTrack->>Editor: read timeline.hoveredTrack
alt hoveredTrack === "zoom" (hovering existing segment)
ZoomTrack-->>User: ignore create (early return)
else not hovering
ZoomTrack->>ZoomTrack: set creatingSegmentViaDrag, store start
end
User->>ZoomTrack: mousemove (first significant move)
alt first move: create
ZoomTrack->>ZoomTrack: compute newSegmentDetails() (start,end,index,min)
ZoomTrack->>Store: createSegment(start,end,index)
else subsequent moves: update
ZoomTrack->>ZoomTrack: compute clamped end via neighbors & totalDuration
ZoomTrack->>Store: updateSegment(segmentId, newEnd)
end
User->>ZoomTrack: mouseup
alt movement occurred
ZoomTrack->>Store: finalize segment (keep created/updated)
else no movement
ZoomTrack->>Store: createSegment(default 1s)
end
Note over ZoomTrack,Editor: On segment deletion or zero segments\nreset/adjust hover state as needed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
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/ZoomTrack.tsx (1)
166-206: Clamp end-time to both min and max; prevent overlap with next segment or durationcreateSegment/updateSegment enforce only min width, so end can exceed the next segment or track end (especially when gap < min width). This causes overlaps during creation and drag. Clamp to [minEnd, maxEnd] and no-op creation if there isn’t enough space.
Apply this diff:
@@ - const minDuration = minPixelWidth * secsPerPixel(); - const minEndTime = startTime + minDuration; - - zoomSegments.splice(index, 0, { - start: startTime, - end: Math.max(minEndTime, endTime), - amount: 1.5, - mode: { - manual: { - x: 0.5, - y: 0.5, - }, - }, - }); - - createdSegmentIndex = index; + const minDuration = minPixelWidth * spp; + const minEndTime = startTime + minDuration; + const next = zoomSegments.find((s) => s.start > startTime); + const maxEndTime = next ? next.start - 0.1 : trackDuration - 0.1; + if (minEndTime > maxEndTime) { + // Not enough space to honor min width; skip insertion. + return; + } + const boundedEnd = Math.min(Math.max(endTime, minEndTime), maxEndTime); + zoomSegments.splice(index, 0, { + start: startTime, + end: boundedEnd, + amount: 1.5, + mode: { manual: { x: 0.5, y: 0.5 } }, + }); + createdSegmentIndex = index; + segmentCreated = true; @@ - }); - }); - }); - segmentCreated = true; + }); + }); + }); @@ - const updateSegment = (endTime: number) => { + const updateSegment = (endTime: number) => { if (!segmentCreated || createdSegmentIndex === -1) return; - - const minDuration = minPixelWidth * secsPerPixel(); - const minEndTime = startTime + minDuration; - - setProject( - "timeline", - "zoomSegments", - createdSegmentIndex, - "end", - Math.max(minEndTime, endTime), - ); + const minDuration = minPixelWidth * spp; + const minEndTime = startTime + minDuration; + const next = project.timeline?.zoomSegments?.find( + (s, idx) => idx !== createdSegmentIndex && s.start > startTime, + ); + const maxEndTime = next ? next.start - 0.1 : trackDuration - 0.1; + const boundedEnd = Math.min(Math.max(endTime, minEndTime), maxEndTime); + setProject("timeline", "zoomSegments", createdSegmentIndex, "end", boundedEnd); };Also applies to: 207-221
🧹 Nitpick comments (4)
apps/desktop/src/routes/editor/Timeline/ZoomTrack.tsx (4)
160-165: Snapshot secsPerPixel/duration at mousedown to keep math stable while draggingAvoid drift if zoom/timeline changes mid-drag.
- const minPixelWidth = 80; - let segmentCreated = false; + const minPixelWidth = 80; + const spp = secsPerPixel(); + const trackDuration = duration(); + let segmentCreated = false; let createdSegmentIndex = -1; const initialMouseX = e.clientX; const initialEndTime = startTime + 1;
222-251: Use snapshots during drag; keep bounds consistent with creation logicUse spp/trackDuration captured at mousedown.
- const deltaX = moveEvent.clientX - initialMouseX; - const deltaTime = deltaX * secsPerPixel(); + const deltaX = moveEvent.clientX - initialMouseX; + const deltaTime = deltaX * spp; const newEndTime = initialEndTime + deltaTime; @@ - const maxEndTime = nextSegment - ? nextSegment.start - 0.1 - : duration() - 0.1; + const maxEndTime = nextSegment ? nextSegment.start - 0.1 : trackDuration - 0.1; @@ - const minDuration = minPixelWidth * secsPerPixel(); + const minDuration = minPixelWidth * spp;
116-144: Use findIndex !== -1 (not undefined); remove dead checkfindIndex returns -1 on miss. Cleans up control flow; avoids confusing undefined checks.
- if (nextSegmentIndex !== undefined) { + if (nextSegmentIndex !== -1) { const prevSegmentIndex = nextSegmentIndex - 1; - - if (prevSegmentIndex === undefined) return;
43-69: Hover reset on deletion: LGTM; small simplification possibleSolid fix. Consider combining conditions to a single branch.
- // If segments were deleted (count decreased), reset hover state - if (currentCount < prevCount) { + // If segments were deleted or none exist, reset hover state + if (currentCount < prevCount || currentCount === 0) { setHoveringSegment(false); setHoveredTime(undefined); - } - - // If no segments exist, also reset - if (currentCount === 0) { - setHoveringSegment(false); - setHoveredTime(undefined); - } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/desktop/src/routes/editor/Timeline/ZoomTrack.tsx(4 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/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/ZoomTrack.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/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 (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Analyze (rust)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/desktop/src/routes/editor/Timeline/ZoomTrack.tsx (1)
156-269: LGTM! Well-structured segment creation with live dragging.The implementation correctly handles:
- Boundary detection and clamping to prevent overlaps
- Minimum duration enforcement (80px minimum width)
- Sorted insertion via dynamic index calculation
- Memory-safe event listener disposal
- Default 1-second segment when no drag occurs
Optional: Consider extracting repeated calculations.
The minimum duration/end time logic is calculated in three places (lines 187-188, 212-213, 237-238). While not a functional issue, extracting to a helper or calculating once could improve maintainability:
const getMinEndTime = () => { const minDuration = minPixelWidth * secsPerPixel(); return startTime + minDuration; };Additionally, magic numbers (80 for minPixelWidth, 0.1 for boundaries, 1 for default duration) could be named constants at the top of the component for easier tuning.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/desktop/src/routes/editor/Timeline/ZoomTrack.tsx(4 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/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/ZoomTrack.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/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). (4)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (rust)
🔇 Additional comments (3)
apps/desktop/src/routes/editor/Timeline/ZoomTrack.tsx (3)
43-44: LGTM! Clean state tracking for deletion detection.The signal provides the foundation for detecting when segments are removed, enabling proper hover state cleanup.
50-69: LGTM! Robust hover state management.The effect correctly handles both deletion scenarios (count decrease and empty state) and prevents stale hover states when segments are removed from the DOM.
147-153: LGTM! Proper guards prevent unwanted segment creation.Both guards are correctly implemented:
- Prevents creation while hovering existing segments
- Restricts creation to left-click only (addresses previous review feedback)
2 participants
Summary by CodeRabbit
Bug Fixes
Enhancements
UI