desktop: multi-select in clip and scenes track & more #1295
Conversation
|
Warning Rate limit exceeded@ameer2468 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 17 minutes and 3 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThe pull request refactors the timeline selection system from single-index selections to multi-index selections. Changes include updating the selection type in editor context to use Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Track as Track Component
participant EditorState
Note over User,EditorState: Single Selection (Before)
User->>Track: Click on segment
Track->>EditorState: setSelection({ type, index })
Note over User,EditorState: Multi-Select (After)
User->>Track: Click + Ctrl/Cmd on segment
Track->>Track: Toggle index in existing indices
Track->>EditorState: setSelection({ type, indices: [...] })
Note over User,EditorState: Range-Select (After)
User->>Track: Shift+Click on segment
Track->>Track: Build range from lastIndex to current
Track->>EditorState: setSelection({ type, indices: [range] })
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 (5)
apps/desktop/src/routes/editor/Timeline/index.tsx (2)
199-203: Guard against undefined bounds when moving mouse
left!may be undefined early after mount; avoidNaNpreviewTime.- setEditorState( - "previewTime", - transform().position + secsPerPixel() * (e.clientX - left!), - ); + if (left != null) { + setEditorState( + "previewTime", + transform().position + secsPerPixel() * (e.clientX - left), + ); + }
234-238: Fix platform detection in wheel event handler (lines 234-238)The bug is confirmed:
platform()from@tauri-apps/plugin-osis asynchronous and returns"darwin"(not"macos") on macOS, so this synchronous comparison never matches.Use the pattern already established in the same codebase (
ClipTrack.tsxlines 384–386 andSceneTrack.tsxlines 236–237):- else if (platform() === "macos") { - delta = e.shiftKey ? e.deltaX : e.deltaY; - } else { - delta = e.deltaY; - } + else if (navigator.platform.toUpperCase().indexOf("MAC") >= 0) { + delta = e.shiftKey ? e.deltaX : e.deltaY; + } else { + delta = e.deltaY; + }This uses the synchronous
navigator.platformAPI (already proven in your Timeline subcomponents) and avoids the async/string mismatch.apps/desktop/src/routes/editor/ConfigSidebar.tsx (1)
705-712: Bug: Passing accessor instead of number to ZoomSegmentPreview
Index’sindexis an accessor. Passing it directly to anumberprop yieldsNaNwhen used arithmetically (e.g.,props.segmentIndex + 1).- <ZoomSegmentPreview - segment={item().segment} - segmentIndex={index} - /> + <ZoomSegmentPreview + segment={item().segment} + segmentIndex={index()} + />apps/desktop/src/routes/editor/context.ts (1)
106-116: Fix condition in deleteClipSegment (operator precedence bug)
!segment.recordingSegment === undefinedis always false; it should be a direct undefined check. This currently allows deletes you intended to block.- if ( - !segment || - !segment.recordingSegment === undefined || - project.timeline.segments.filter( - (s) => s.recordingSegment === segment.recordingSegment, - ).length < 2 - ) + if ( + !segment || + segment.recordingSegment === undefined || + project.timeline.segments.filter( + (s) => s.recordingSegment === segment.recordingSegment, + ).length < 2 + ) return;apps/desktop/src/routes/editor/Timeline/SceneTrack.tsx (1)
93-99: Use currentTarget for bounds, not target
e.targetmay be a child element;getBoundingClientRect()can be incorrect or missing. Use the track element.- const bounds = e.target.getBoundingClientRect()!; + const bounds = (e.currentTarget as Element).getBoundingClientRect();
🧹 Nitpick comments (8)
apps/desktop/src/routes/editor/Timeline/index.tsx (1)
144-156: Batch-delete helpers to reduce churn and ensure dedupeReverse-order loop is correct, but we already have a batched API for zoom. Mirror it for clips/scenes to dedupe, clear selection once, and minimize history churn.
Apply this refactor:
- Add
projectActions.deleteClipSegments(indices: number[])andprojectActions.deleteSceneSegments(indices: number[])(batched, descending, deduped).- Replace these loops with single calls:
- // Delete all selected clips in reverse order - [...selection.indices] - .sort((a, b) => b - a) - .forEach((idx) => { - projectActions.deleteClipSegment(idx); - }); + projectActions.deleteClipSegments(selection.indices);- // Delete all selected scenes in reverse order - [...selection.indices] - .sort((a, b) => b - a) - .forEach((idx) => { - projectActions.deleteSceneSegment(idx); - }); + projectActions.deleteSceneSegments(selection.indices);apps/desktop/src/routes/editor/Timeline/ZoomTrack.tsx (1)
399-402: Use loop index instead of findIndex for selectionYou’re already inside
<For each=...>{(_, i) => ...}; avoidfindIndexon every recompute.- const segmentIndex = project.timeline?.zoomSegments?.findIndex( - (s) => s.start === segment.start && s.end === segment.end, - ); - if (segmentIndex === undefined || segmentIndex === -1) return false; - return selection.indices.includes(segmentIndex); + const segmentIndex = i(); + return selection.indices.includes(segmentIndex);apps/desktop/src/routes/editor/ConfigSidebar.tsx (2)
749-795: Batch delete scenes instead of per-index loopAvoid per-index
deleteSceneSegmentcalls (each clears selection) and rely on a batched API for better performance and simpler state updates.- const indices = value().selection.indices; - // Delete segments in reverse order to maintain indices - [...indices].sort((a, b) => b - a).forEach((idx) => { - projectActions.deleteSceneSegment(idx); - }); + projectActions.deleteSceneSegments(value().selection.indices);Provide
deleteSceneSegmentsinprojectActions(dedupe, bounds check, descending, clear selection once).
815-861: Batch delete clips instead of per-index loopSame as scenes: do a single batched delete to dedupe/sort once and clear selection once.
- const indices = value().selection.indices; - [...indices].sort((a, b) => b - a).forEach((idx) => { - projectActions.deleteClipSegment(idx); - }); + projectActions.deleteClipSegments(value().selection.indices);apps/desktop/src/routes/editor/context.ts (1)
151-168: Add symmetric batched delete helpers for clips/scenesYou already provide
deleteZoomSegments. AdddeleteClipSegmentsanddeleteSceneSegmentsfor parity and use them from Timeline and ConfigSidebar.Add near
deleteZoomSegments:deleteClipSegments: (segmentIndices: number[]) => { if (!project.timeline) return; batch(() => { setProject( "timeline", "segments", produce((s) => { if (!s) return; const sorted = [...new Set(segmentIndices)] .filter((i) => Number.isInteger(i) && i >= 0 && i < s.length) .sort((a, b) => b - a); for (const i of sorted) s.splice(i, 1); }), ); setEditorState("timeline", "selection", null); }); }, deleteSceneSegments: (segmentIndices: number[]) => { batch(() => { setProject( "timeline", "sceneSegments", produce((s) => { if (!s) return; const sorted = [...new Set(segmentIndices)] .filter((i) => Number.isInteger(i) && i >= 0 && i < s.length) .sort((a, b) => b - a); for (const i of sorted) s.splice(i, 1); }), ); setEditorState("timeline", "selection", null); }); },Then update callers in
Timeline/index.tsxandConfigSidebar.tsxas suggested.apps/desktop/src/routes/editor/Timeline/ClipTrack.tsx (2)
245-248: Use loop index for selection membershipAvoid
findIndexper recompute and rely oni().- const segmentIndex = project.timeline?.segments?.findIndex( - (s) => s.start === segment.start && s.end === segment.end, - ); - if (segmentIndex === undefined || segmentIndex === -1) return false; - return selection.indices.includes(segmentIndex); + return selection.indices.includes(i());
376-451: Attach mouseup on window to avoid lost clicksIf the pointer leaves the segment before mouseup, the local target’s
mouseupmay not fire; selection won’t update.- createEventListener( - e.currentTarget, - "mouseup", - (upEvent) => { /* ... */ } - ); + createEventListenerMap(window, { + mouseup: (upEvent) => { /* same body */ }, + });apps/desktop/src/routes/editor/Timeline/SceneTrack.tsx (1)
350-353: Use loop index for selection membershipSame optimization as clips/zoom.
- const segmentIndex = project.timeline?.sceneSegments?.findIndex( - (s) => s.start === segment.start && s.end === segment.end, - ); - if (segmentIndex === undefined || segmentIndex === -1) return false; - return selection.indices.includes(segmentIndex); + return selection.indices.includes(i());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/desktop/src/routes/(window-chrome)/settings/recordings.tsx(3 hunks)apps/desktop/src/routes/editor/ConfigSidebar.tsx(2 hunks)apps/desktop/src/routes/editor/Timeline/ClipTrack.tsx(2 hunks)apps/desktop/src/routes/editor/Timeline/SceneTrack.tsx(2 hunks)apps/desktop/src/routes/editor/Timeline/ZoomTrack.tsx(1 hunks)apps/desktop/src/routes/editor/Timeline/index.tsx(1 hunks)apps/desktop/src/routes/editor/context.ts(1 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/index.tsxapps/desktop/src/routes/editor/Timeline/ZoomTrack.tsxapps/desktop/src/routes/editor/Timeline/ClipTrack.tsxapps/desktop/src/routes/editor/context.tsapps/desktop/src/routes/editor/Timeline/SceneTrack.tsxapps/desktop/src/routes/editor/ConfigSidebar.tsxapps/desktop/src/routes/(window-chrome)/settings/recordings.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/index.tsxapps/desktop/src/routes/editor/Timeline/ZoomTrack.tsxapps/desktop/src/routes/editor/Timeline/ClipTrack.tsxapps/desktop/src/routes/editor/context.tsapps/desktop/src/routes/editor/Timeline/SceneTrack.tsxapps/desktop/src/routes/editor/ConfigSidebar.tsxapps/desktop/src/routes/(window-chrome)/settings/recordings.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/index.tsxapps/desktop/src/routes/editor/Timeline/ZoomTrack.tsxapps/desktop/src/routes/editor/Timeline/ClipTrack.tsxapps/desktop/src/routes/editor/context.tsapps/desktop/src/routes/editor/Timeline/SceneTrack.tsxapps/desktop/src/routes/editor/ConfigSidebar.tsxapps/desktop/src/routes/(window-chrome)/settings/recordings.tsx
🧬 Code graph analysis (1)
apps/desktop/src/routes/editor/ConfigSidebar.tsx (1)
apps/desktop/src/routes/editor/ui.tsx (2)
Subfield(49-63)EditorButton(353-418)
⏰ 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). (5)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Lint (Biome)
- GitHub Check: Analyze (rust)
- GitHub Check: Analyze (actions)
This PR does the following:
Summary by CodeRabbit
New Features
Improvements