fix(editor): show scissors cursor when clip tool is active #1217
Conversation
WalkthroughClipTrack component now supports split-mode for clip segments with integrated mouse-down handling that dispatches to either split or resize operations based on mode. Marker rendering was restructured for simplicity. Custom scissors cursor styling added to timeline CSS. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ClipTrack
participant projectActions
User->>ClipTrack: Mouse-down on clip handle
alt Split Mode Active
ClipTrack->>ClipTrack: Compute splitTime from click position
ClipTrack->>projectActions: splitClipSegment(splitTime)
projectActions-->>ClipTrack: Segment split
else Normal Mode
ClipTrack->>ClipTrack: Setup mouseup listener
ClipTrack->>ClipTrack: Open clip config via timeline.selection
User->>ClipTrack: Mouse-up
ClipTrack->>ClipTrack: Execute pending resize with bounds checks
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~35 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)
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: 3
🧹 Nitpick comments (4)
apps/desktop/src/routes/editor/Timeline/styles.css (1)
16-24: Cursor override robustnessTo ensure this beats Tailwind’s cursor utilities (e.g., cursor-col-resize) in all cascades, add !important to the descendant rule.
-.timeline-scissors-cursor * { - cursor: inherit; -} +.timeline-scissors-cursor * { + cursor: inherit !important; +}apps/desktop/src/routes/editor/Timeline/ClipTrack.tsx (3)
198-201: Clarify intent: rename helper toisSplitModeMinor readability nit. Avoid shadowing the common term “split” and make the predicate explicit.
-const split = () => { - return editorState.timeline.interactMode === "split"; -} +const isSplitMode = () => editorState.timeline.interactMode === "split";And update call sites accordingly.
416-461: Resize math likely mixes display seconds and clip units (timescale)
secsPerPixel()yields timeline/display seconds per pixel. You add that directly tostart(clip units) and clamp using values that are a mix of display units (availableTimelineDuration) and clip units (segment.end). This breaks whentimescale !== 1.Recommended approach:
- Convert pixel delta to display seconds: deltaDisplay = dx * secsPerPixel()
- Convert to clip units: deltaClip = deltaDisplay * (segment.timescale ?? 1)
- Compute bounds consistently in clip units:
- maxDurationClip = min(maxSegmentDuration, availableTimelineDuration) * (segment.timescale ?? 1)
- lower bound for start: max(prevEndIfSameClip, segment.end - maxDurationClip, 0)
- min length: segment.end - minLenSec * (segment.timescale ?? 1)
I can draft a concrete patch once you confirm the intended unit semantics of
segment.start/endanddisplay.duration.
501-510: Same timescale unit-mixing concerns for end handleThe end-handle path has the same mixing issues (e.g.,
segment.end + availableTimelineDurationadds display seconds to clip units). Please apply the same conversions as suggested for the start handle.Also applies to: 530-550
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/desktop/src/routes/editor/Timeline/ClipTrack.tsx(2 hunks)apps/desktop/src/routes/editor/Timeline/styles.css(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/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 (3)
SegmentRoot(49-87)SegmentHandle(103-125)SegmentContent(89-101)
⏰ 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 (1)
apps/desktop/src/routes/editor/Timeline/ClipTrack.tsx (1)
351-360: LGTM: cursor state applied only in split modeConditional application of the scissors cursor class is correct and localized to each segment.
| if (editorState.timeline.interactMode === "split") { | ||
| const rect = e.currentTarget.getBoundingClientRect(); | ||
| const fraction = (e.clientX - rect.left) / rect.width; | ||
|
|
||
| const splitTime = fraction * (segment.end - segment.start); | ||
|
|
||
| projectActions.splitClipSegment(prevDuration() + splitTime); | ||
| } else { | ||
| createRoot((dispose) => { | ||
| createEventListener(e.currentTarget, "mouseup", (e) => { | ||
| dispose(); | ||
|
|
||
| // // If there's only one segment, don't open the clip config panel | ||
| // // since there's nothing to configure - just let the normal click behavior happen | ||
| // const hasOnlyOneSegment = segments().length === 1; | ||
|
|
||
| // if (hasOnlyOneSegment) { | ||
| // // Clear any existing selection (zoom, layout, etc.) when clicking on a clip | ||
| // // This ensures the sidebar updates properly | ||
| // setEditorState("timeline", "selection", null); | ||
| // } else { | ||
| // When there are multiple segments, show the clip configuration | ||
| setEditorState("timeline", "selection", { | ||
| type: "clip", | ||
| index: i(), | ||
| }); | ||
|
|
||
| // } | ||
| props.handleUpdatePlayhead(e); | ||
| }); | ||
| }); | ||
| } | ||
| }} | ||
| > | ||
| <WaveformCanvas | ||
| micWaveform={micWaveform()} | ||
| systemWaveform={systemAudioWaveform()} | ||
| segment={segment} | ||
| secsPerPixel={secsPerPixel()} | ||
| /> | ||
|
|
||
| <Markings segment={segment} prevDuration={prevDuration()} /> | ||
|
|
||
| <SegmentHandle | ||
| position="start" | ||
| class="opacity-0 group-hover:opacity-100" | ||
| onMouseDown={(downEvent) => { | ||
| const start = segment.start; | ||
|
|
||
| const maxSegmentDuration = | ||
| editorInstance.recordings.segments[ | ||
| segment.recordingSegment ?? 0 | ||
| ].display.duration; | ||
|
|
||
| const availableTimelineDuration = | ||
| editorInstance.recordingDuration - | ||
| segments().reduce( | ||
| (acc, segment, segmentI) => | ||
| segmentI === i() | ||
| ? acc | ||
| : acc + | ||
| (segment.end - segment.start) / segment.timescale, | ||
| const rect = e.currentTarget.getBoundingClientRect(); | ||
| const fraction = (e.clientX - rect.left) / rect.width; | ||
|
|
||
| const splitTime = fraction * (segment.end - segment.start); | ||
|
|
||
| projectActions.splitClipSegment(prevDuration() + splitTime); | ||
| } else { |
There was a problem hiding this comment.
Split position ignores timescale (wrong cut point when timescale ≠ 1)
Width/pixels map to display duration, but splitTime uses raw (end-start) without dividing by timescale. Use display duration to compute the timeline time.
-const splitTime = fraction * (segment.end - segment.start);
+const displayDuration =
+ (segment.end - segment.start) / (segment.timescale ?? 1);
+const splitTime = fraction * displayDuration;This keeps prevDuration() + splitTime in the same (display) units as the timeline.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (editorState.timeline.interactMode === "split") { | |
| const rect = e.currentTarget.getBoundingClientRect(); | |
| const fraction = (e.clientX - rect.left) / rect.width; | |
| const splitTime = fraction * (segment.end - segment.start); | |
| projectActions.splitClipSegment(prevDuration() + splitTime); | |
| } else { | |
| createRoot((dispose) => { | |
| createEventListener(e.currentTarget, "mouseup", (e) => { | |
| dispose(); | |
| // // If there's only one segment, don't open the clip config panel | |
| // // since there's nothing to configure - just let the normal click behavior happen | |
| // const hasOnlyOneSegment = segments().length === 1; | |
| // if (hasOnlyOneSegment) { | |
| // // Clear any existing selection (zoom, layout, etc.) when clicking on a clip | |
| // // This ensures the sidebar updates properly | |
| // setEditorState("timeline", "selection", null); | |
| // } else { | |
| // When there are multiple segments, show the clip configuration | |
| setEditorState("timeline", "selection", { | |
| type: "clip", | |
| index: i(), | |
| }); | |
| // } | |
| props.handleUpdatePlayhead(e); | |
| }); | |
| }); | |
| } | |
| }} | |
| > | |
| <WaveformCanvas | |
| micWaveform={micWaveform()} | |
| systemWaveform={systemAudioWaveform()} | |
| segment={segment} | |
| secsPerPixel={secsPerPixel()} | |
| /> | |
| <Markings segment={segment} prevDuration={prevDuration()} /> | |
| <SegmentHandle | |
| position="start" | |
| class="opacity-0 group-hover:opacity-100" | |
| onMouseDown={(downEvent) => { | |
| const start = segment.start; | |
| const maxSegmentDuration = | |
| editorInstance.recordings.segments[ | |
| segment.recordingSegment ?? 0 | |
| ].display.duration; | |
| const availableTimelineDuration = | |
| editorInstance.recordingDuration - | |
| segments().reduce( | |
| (acc, segment, segmentI) => | |
| segmentI === i() | |
| ? acc | |
| : acc + | |
| (segment.end - segment.start) / segment.timescale, | |
| const rect = e.currentTarget.getBoundingClientRect(); | |
| const fraction = (e.clientX - rect.left) / rect.width; | |
| const splitTime = fraction * (segment.end - segment.start); | |
| projectActions.splitClipSegment(prevDuration() + splitTime); | |
| } else { | |
| if (editorState.timeline.interactMode === "split") { | |
| const rect = e.currentTarget.getBoundingClientRect(); | |
| const fraction = (e.clientX - rect.left) / rect.width; | |
| const displayDuration = | |
| (segment.end - segment.start) / (segment.timescale ?? 1); | |
| const splitTime = fraction * displayDuration; | |
| projectActions.splitClipSegment(prevDuration() + splitTime); | |
| } else { |
🤖 Prompt for AI Agents
In apps/desktop/src/routes/editor/Timeline/ClipTrack.tsx around lines 366-373,
the split calculation uses raw segment duration so the cut is wrong when
timescale ≠ 1; compute the display duration by dividing the raw duration by the
relevant timescale (e.g. displayDuration = (segment.end - segment.start) /
(segment.timescale || 1) or the timeline timescale used for rendering), then set
splitTime = fraction * displayDuration and call
projectActions.splitClipSegment(prevDuration() + splitTime); also guard against
zero timescale to avoid division by zero.
| createRoot((dispose) => { | ||
| createEventListener(e.currentTarget, "mouseup", (e) => { | ||
| dispose(); | ||
|
|
||
| // // If there's only one segment, don't open the clip config panel | ||
| // // since there's nothing to configure - just let the normal click behavior happen | ||
| // const hasOnlyOneSegment = segments().length === 1; | ||
|
|
||
| // if (hasOnlyOneSegment) { | ||
| // // Clear any existing selection (zoom, layout, etc.) when clicking on a clip | ||
| // // This ensures the sidebar updates properly | ||
| // setEditorState("timeline", "selection", null); | ||
| // } else { | ||
| // When there are multiple segments, show the clip configuration | ||
| setEditorState("timeline", "selection", { | ||
| type: "clip", | ||
| index: i(), | ||
| }); | ||
|
|
||
| // } | ||
| props.handleUpdatePlayhead(e); | ||
| }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Click vs drag: segment selection fires after handle resize
In non‑split mode, SegmentRoot installs a mouseup handler that always selects the clip and updates the playhead. When resizing via a handle, this will also run, causing unintended selection/playhead jumps.
Two minimal options:
- Stop propagation in handle
onMouseDown(see suggested diffs in handle blocks). - Or guard selection here by ignoring events that originated from a handle or had significant movement.
If you prefer a guard here, example:
createRoot((dispose) => {
- createEventListener(e.currentTarget, "mouseup", (e) => {
+ const downX = e.clientX, downY = e.clientY;
+ const isFromHandle = (target: Element | null) =>
+ !!(target as HTMLElement | null)?.closest('[data-segment-handle]');
+ createEventListenerMap(e.currentTarget, {
+ mouseup: (e) => {
+ if (isFromHandle(e.target) || Math.hypot(e.clientX - downX, e.clientY - downY) > 3) {
+ dispose();
+ return;
+ }
dispose();
// …Committable suggestion skipped: line range outside the PR's diff.
| <SegmentHandle | ||
| position="start" | ||
| class="opacity-0 group-hover:opacity-100" | ||
| onMouseDown={(downEvent) => { | ||
| const start = segment.start; | ||
|
|
||
| if (split()) return; | ||
| const maxSegmentDuration = | ||
| editorInstance.recordings.segments[ | ||
| segment.recordingSegment ?? 0 |
There was a problem hiding this comment.
Stop propagation in handle mousedown to prevent segment click-selection
Prevents the SegmentRoot mouseup-selection from being armed when starting a resize.
onMouseDown={(downEvent) => {
+ downEvent.stopPropagation();
const start = segment.start;
if (isSplitMode?.() ?? split?.()) return;Note: adjust isSplitMode/split name per your final choice.
Also applies to: 444-463
🤖 Prompt for AI Agents
In apps/desktop/src/routes/editor/Timeline/ClipTrack.tsx around lines 409-418
(and similarly for the other SegmentHandle block at 444-463), the SegmentHandle
onMouseDown handler needs to stop the mousedown from bubbling so the SegmentRoot
mouseup-selection isn't armed when beginning a resize; at the top of the handler
call event.stopPropagation() (and optionally event.preventDefault() if needed)
before any logic (e.g., before checking split/isSplitMode and reading
segment.start) to prevent the click-selection behavior during resize.
1 participant
Description
Shows a scissors cursor when split mode is active and disables clip resizing to prevent accidental edits.
2025-10-17.3.36.45.AM.mp4
Summary by CodeRabbit
New Features
Improvements