Refactors in windows.rs, Bump tauri and tauri plugin and objc2-* crate versions #1399
Conversation
|
Warning Rate limit exceeded@ItsEeleeya has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 42 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 (1)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughWorkspace updates Tauri and adds objc2; macOS platform code migrates from raw FFI/delegate implementations to objc2-based APIs and a WebviewWindowExt trait; window identifier/enums renamed (CapWindowId → CapWindowDef, ShowCapWindow → CapWindow); recording and timeline types extended. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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 |
…ndard levels for now.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/recording/src/output_pipeline/macos.rs (1)
123-139: ExistingAVFoundationMp4Muxer::send_audio_framealso lacks retry limit.The same unbounded loop issue exists in the original
AudioMuxerimplementation. Based on the retrieved learning, blocking is intentional for sequential processing, but indefinite blocking without timeout is still a reliability concern.
🟡 Minor comments (14)
apps/desktop/src-tauri/src/captions.rs-716-730 (1)
716-730: Potential silent word loss when timing data is missing.If
word_startisNonebutcurrent_wordis non-empty, the word is silently dropped. This can occur when all tokens in a word lack timing data (theelsebranch at line 706). Consider either logging a warning or falling back to the segment's overall timing for orphaned words.if !current_word.trim().is_empty() { if let Some(ws) = word_start { log::info!( " -> Final word: '{}' ({:.2}s - {:.2}s)", current_word.trim(), ws, word_end ); words.push(CaptionWord { text: current_word.trim().to_string(), start: ws, end: word_end, }); + } else { + log::warn!( + " -> Dropping word '{}' with no timing data", + current_word.trim() + ); } }apps/desktop/src/routes/editor/Player.tsx-309-311 (1)
309-311: Type error with MenuItem polymorphicasprop.Static analysis flags a type mismatch when using
as="div"onMenuItem. The component may not support this polymorphic pattern, or the type definitions need adjustment.Consider using a styled div directly instead of the MenuItem component for this disabled header:
-<MenuItem as="div" class="text-gray-11" data-disabled="true"> - Select preview quality -</MenuItem> +<div class="text-gray-11 px-2 py-1.5 text-sm" aria-disabled="true"> + Select preview quality +</div>apps/desktop/src/routes/editor/Player.tsx-292-299 (1)
292-299: Type error in KSelect.Value component.Static analysis indicates a type mismatch: the children function returns
stringbut the expected type is different. The nullable coalescing with??may need explicit typing.Consider updating the return type:
<KSelect.Value<{ label: string; value: PreviewQuality }> class="flex-1 text-left truncate" placeholder="Select preview quality" > - {(state) => - state.selectedOption()?.label ?? "Select preview quality" - } + {(state) => ( + <span>{state.selectedOption()?.label ?? "Select preview quality"}</span> + )} </KSelect.Value>apps/desktop/src/routes/editor/CaptionsTab.tsx-830-841 (1)
830-841: Number input onChange uses target.value without parsing validation.The
parseFloat(e.target.value)could produceNaNif the input is cleared or contains invalid characters. This could corrupt the segment data.Add validation before updating:
onChange={(e) => - updateSegment(segment.id, { - start: parseFloat(e.target.value), - }) + { + const value = parseFloat(e.target.value); + if (!Number.isNaN(value) && value >= 0) { + updateSegment(segment.id, { start: value }); + } + } }Also applies to: 847-858
apps/desktop/src/routes/editor/TextOverlay.tsx-86-88 (1)
86-88: Remove comments per coding guidelines.The file contains several comments that violate the coding guidelines which state: "Never add any form of comments to code... Code must be self-explanatory through naming, types, and structure."
Remove comments on lines 86-88, 114, 122-127, 263, 276-278, 287-288, and 301. The code is already self-explanatory.
- // Measurement Logic let hiddenMeasureRef: HTMLDivElement | undefined;- // Normalize to [0-1] const normalizedWidth = naturalWidth / deps.containerWidth; const normalizedHeight = naturalHeight / deps.containerHeight; const newFontSize = deps.fontSize; const newSizeX = normalizedWidth; const newSizeY = normalizedHeight; - // Logic simplified: Trust the measurement. - - // Update if significant difference to avoid loops const sizeXDiff = Math.abs(newSizeX - segment().size.x); const sizeYDiff = Math.abs(newSizeY - segment().size.y); - // const fontDiff = Math.abs(newFontSize - segment().fontSize); // We aren't changing font size anymoreAlso applies to: 114-127, 263-263, 276-278, 287-288, 301-301
apps/desktop/src/routes/editor/MaskOverlay.tsx-89-97 (1)
89-97: Fix the type error in Show component usage.The static analysis reports a type error on line 90. The issue is that when using
Showwith a callback child, the callback receives the resolvedwhenvalue as its parameter. The current compound expressionselectedMask() && maskState()makes the type ambiguous.Consider restructuring to use nested Show components or a single computed value:
- <Show when={selectedMask() && maskState()}> - {() => { - const state = () => maskState()!; + <Show when={selectedMask()}> + {(selected) => ( + <Show when={maskState()}> + {(state) => { const rect = () => { - const width = state().size.x * props.size.width; - const height = state().size.y * props.size.height; - const left = state().position.x * props.size.width - width / 2; - const top = state().position.y * props.size.height - height / 2; + const width = state().size.x * props.size.width; + const height = state().size.y * props.size.height; + const left = state().position.x * props.size.width - width / 2; + const top = state().position.y * props.size.height - height / 2; return { width, height, left, top }; }; // ... rest of component + }} + </Show> + )} + </Show>Alternatively, create a combined memo:
const overlayData = createMemo(() => { const selected = selectedMask(); const mask = maskState(); if (!selected || !mask) return null; return { selected, mask }; }); // Then use: <Show when={overlayData()}> {(data) => { /* use data().selected and data().mask */ }} </Show>apps/desktop/src/routes/editor/Editor.tsx-119-139 (1)
119-139: Event listeners not cleaned up on component unmount.The
handleTimelineResizeStartfunction addsmousemoveandmouseuplisteners towindow, but if the component unmounts during a drag operation, these listeners remain attached. Consider usingonCleanupto ensure removal.+ import { onCleanup } from "solid-js"; const handleTimelineResizeStart = (event: MouseEvent) => { if (event.button !== 0) return; event.preventDefault(); const startY = event.clientY; const startHeight = timelineHeight(); setIsResizingTimeline(true); const handleMove = (moveEvent: MouseEvent) => { const delta = moveEvent.clientY - startY; setStoredTimelineHeight(clampTimelineHeight(startHeight - delta)); }; const handleUp = () => { setIsResizingTimeline(false); window.removeEventListener("mousemove", handleMove); window.removeEventListener("mouseup", handleUp); }; window.addEventListener("mousemove", handleMove); window.addEventListener("mouseup", handleUp); + + onCleanup(() => { + window.removeEventListener("mousemove", handleMove); + window.removeEventListener("mouseup", handleUp); + }); };Committable suggestion skipped: line range outside the PR's diff.
crates/frame-converter/src/videotoolbox.rs-209-250 (1)
209-250: Buffer remains locked if frame creation or copy fails.In
copy_output_to_frame, if any operation afterCVPixelBufferLockBaseAddressfails (e.g., during the copy loop), the function returns an error without callingCVPixelBufferUnlockBaseAddress. This leaves the buffer in a locked state.Consider using a scope guard or restructuring to ensure unlock on all paths:
fn copy_output_to_frame( &self, pixel_buffer: CVPixelBufferRef, ) -> Result<frame::Video, ConvertError> { unsafe { let lock_status = CVPixelBufferLockBaseAddress(pixel_buffer, 0); if lock_status != K_CV_RETURN_SUCCESS { return Err(ConvertError::ConversionFailed(format!( "CVPixelBufferLockBaseAddress failed: {}", lock_status ))); } } - let mut output = - frame::Video::new(self.output_format, self.output_width, self.output_height); + let result = (|| { + let mut output = + frame::Video::new(self.output_format, self.output_width, self.output_height); - unsafe { - // ... copy logic ... - } + unsafe { + // ... copy logic ... + } - Ok(output) + Ok(output) + })(); + + unsafe { + CVPixelBufferUnlockBaseAddress(pixel_buffer, 0); + } + + result }crates/recording/examples/camera-benchmark.rs-158-159 (1)
158-159: Platform-specific null device path breaks Windows compatibility.
/dev/nullis Unix-specific. On Windows, this would fail. Consider using a cross-platform approach.- let mut output = - ffmpeg::format::output_as("/dev/null", "mp4").expect("Failed to create dummy output"); + let null_path = if cfg!(windows) { "NUL" } else { "/dev/null" }; + let mut output = + ffmpeg::format::output_as(null_path, "mp4").expect("Failed to create dummy output");apps/desktop/src/routes/editor/ExportDialog.tsx-1117-1118 (1)
1117-1118: Non-null assertion on potentially undefined value.
meta().sharing?.link!uses a non-null assertion after optional chaining. Ifsharingis undefined, the optional chain returnsundefined, and the!assertion would be incorrect.Add a guard or use nullish coalescing:
- navigator.clipboard.writeText( - meta().sharing?.link!, - ); + const link = meta().sharing?.link; + if (link) navigator.clipboard.writeText(link);apps/desktop/src/routes/editor/ExportDialog.tsx-1111-1130 (1)
1111-1130: Button has conflicting behaviors - opens link AND copies to clipboard.The button is wrapped in an anchor that opens the link, but the
onClickhandler also writes to clipboard. This creates a confusing UX where clicking performs two actions simultaneously.Consider separating these into distinct actions, or make the copy behavior explicit (e.g., a separate "Copy Link" button alongside "Open Link").
- <a - href={meta().sharing?.link} - target="_blank" - rel="noreferrer" - class="block" - > - <Button - onClick={() => { - setCopyPressed(true); - setTimeout(() => { - setCopyPressed(false); - }, 2000); - navigator.clipboard.writeText( - meta().sharing?.link!, - ); - }} - variant="dark" - class="flex gap-2 justify-center items-center" - > + <Button + onClick={() => { + window.open(meta().sharing?.link, "_blank"); + }} + variant="dark" + class="flex gap-2 justify-center items-center" + > + <IconCapLink class="size-4" /> + <p>Open Link</p> + </Button>Committable suggestion skipped: line range outside the PR's diff.
apps/desktop/src-tauri/src/lib.rs-2947-2948 (1)
2947-2948: Type mismatch: Using CapWindow instead of CapWindowDef.In
has_open_editor_window, the code usesCapWindow::from_strbut the pattern match usesCapWindow::Editor. Based on the refactoring pattern in this file, this should likely beCapWindowDef::from_strandCapWindowDef::Editorfor consistency with the rest of the codebase.fn has_open_editor_window(app: &AppHandle) -> bool { app.webview_windows() .keys() - .any(|label| matches!(CapWindow::from_str(label), Ok(CapWindow::Editor { .. }))) + .any(|label| matches!(CapWindowDef::from_str(label), Ok(CapWindowDef::Editor { .. }))) }crates/frame-converter/src/pool.rs-323-327 (1)
323-327: DropStrategy variants have identical behavior - DropOldest not implemented.Both
DropOldestandDropNewestsimply increment the dropped counter. To implementDropOldest, you would need to receive and discard an old frame from the output channel before sending the new one. Currently, the newest frame is effectively dropped in both cases sincetry_sendfails.If
DropOldestis intended to be different fromDropNewest, it needs additional logic:Err(flume::TrySendError::Full(_frame)) => match drop_strategy { - DropStrategy::DropOldest | DropStrategy::DropNewest => { + DropStrategy::DropOldest => { + let _ = output_tx.try_recv(); + if output_tx.try_send(ConvertedFrame { + frame: converted, + sequence, + submit_time, + conversion_duration, + }).is_err() { + stats.frames_dropped.fetch_add(1, Ordering::Relaxed); + } + } + DropStrategy::DropNewest => { stats.frames_dropped.fetch_add(1, Ordering::Relaxed); } },Alternatively, document that both strategies currently behave identically (dropping the newest).
Committable suggestion skipped: line range outside the PR's diff.
crates/recording/src/output_pipeline/win.rs-693-743 (1)
693-743: SysMemSlicePitch should specify the total NV12 buffer size, not 0.The row pitch calculation (
frame.width * bytes_per_pixel=frame.widthfor NV12) is actually correct. However,SysMemSlicePitchis incorrectly set to 0. For NV12 planar format in D3D11,SysMemSlicePitchmust be set to the total buffer size containing both Y and UV planes:width * (height + height/2)or equivalentlywidth * height * 3/2. Setting it to 0 may cause the D3D11 driver to misinterpret the buffer layout.
🧹 Nitpick comments (50)
crates/recording/src/output_pipeline/core.rs (1)
488-519: Consider extracting shared frame-processing logic to reduce duplication.The drain loop (lines 491-511) largely mirrors the main loop (lines 461-478). While the error handling differs intentionally (graceful
warn!+breakvs returningErr), the timestamp calculation and muxer call could be extracted into a helper closure to reduce repetition.+ let process_frame = |frame: TVideo::Frame, + first_tx: &mut Option<oneshot::Sender<Timestamp>>, + muxer: &Arc<Mutex<TMutex>>| async { + let timestamp = frame.timestamp(); + if let Some(tx) = first_tx.take() { + let _ = tx.send(timestamp); + } + let duration = timestamp + .checked_duration_since(timestamps) + .unwrap_or(Duration::ZERO); + muxer.lock().await.send_video_frame(frame, duration) + };This would allow both loops to call
process_frameand handle errors according to their respective policies.crates/recording/src/screenshot.rs (1)
195-199: Consider downgrading fast-capture size logging frominfotodebugin the futureLogging the final fast-captured image dimensions is helpful while validating the new area-capture behavior, but if screenshots are taken frequently this
info-level line might become noisy in production logs. Once you are confident in the behavior, consider moving this todebugor guarding it behind a more verbose log level/feature flag.apps/desktop/src/routes/target-select-overlay.tsx (1)
708-736: Remove debug logs; approve cropBounds refactor.The refactor to capture
crop()once intocropBounds(Line 708) and use it consistently throughout (Lines 723-724, 727-728) is excellent—this ensures atomic reads and prevents potential inconsistencies from multiple function calls.However, the debug
console.logstatements (Lines 710-716, 733-736) appear to be temporary debugging artifacts and should be removed before merging.Apply this diff to remove the debug logs while keeping the cropBounds refactor:
if (was && !interacting) { if (options.mode === "screenshot" && isValid()) { const cropBounds = crop(); - const displayInfo = areaDisplayInfo.data; - console.log("[Screenshot Debug] crop bounds:", cropBounds); - console.log("[Screenshot Debug] display info:", displayInfo); - console.log( - "[Screenshot Debug] window.innerWidth/Height:", - window.innerWidth, - window.innerHeight, - ); - const target: ScreenCaptureTarget = { variant: "area", screen: displayId(), bounds: { position: { x: cropBounds.x, y: cropBounds.y, }, size: { width: cropBounds.width, height: cropBounds.height, }, }, }; - console.log( - "[Screenshot Debug] target being sent:", - JSON.stringify(target, null, 2), - ); - try { const path = await invoke<string>("take_screenshot", { target,apps/desktop/src-tauri/src/recording.rs (1)
1228-1241: Consider consolidating duplicate Camera window closing.The Camera window is closed twice within the same code block:
- First at lines 1231-1233
- Again at lines 1239-1241
Unless there's a specific reason for this pattern (e.g., ensuring cleanup after feed removal), these could be consolidated into a single operation.
Apply this diff to consolidate:
if let Some(window) = CapWindowDef::Main.get(&handle) { window.unminimize().ok(); } else { - if let Some(v) = CapWindowDef::Camera.get(&handle) { - let _ = v.close(); - } let _ = app.mic_feed.ask(microphone::RemoveInput).await; let _ = app.camera_feed.ask(camera::RemoveInput).await; app.selected_mic_label = None; app.selected_camera_id = None; app.camera_in_use = false; if let Some(win) = CapWindowDef::Camera.get(&handle) { win.close().ok(); } }crates/export/src/mp4.rs (1)
193-224: Screenshot task behavior is sound; consider logging I/O failures instead of fully swallowing themThe move of screenshot generation into
spawn_blockinglooks correct and keeps blocking I/O off the async runtime, and panics inside the task are safely contained and logged via theJoinErrorbranch.Right now, failures from
create_dir_allandrgb_img.saveare silently ignored, which can make debugging “missing screenshot” reports harder. You can keep export non‑fatal while still logging these errors:- let screenshots_dir = project_path.join("screenshots"); - if std::fs::create_dir_all(&screenshots_dir).is_err() { - return; - } - - let screenshot_path = screenshots_dir.join("display.jpg"); - let _ = rgb_img.save(&screenshot_path); + let screenshots_dir = project_path.join("screenshots"); + if let Err(e) = std::fs::create_dir_all(&screenshots_dir) { + warn!( + "Failed to create screenshots directory at {}: {e}", + screenshots_dir.display() + ); + return; + } + + let screenshot_path = screenshots_dir.join("display.jpg"); + if let Err(e) = rgb_img.save(&screenshot_path) { + warn!( + "Failed to save screenshot to {}: {e}", + screenshot_path.display() + ); + }This keeps screenshot failures non‑fatal but provides useful diagnostics when they occur.
apps/desktop/src-tauri/src/captions.rs (1)
963-972: Consider handling potential NaN values in JSON serialization.
serde_json::Number::from_f64()returnsNoneforNaNorInfinityvalues. While unlikely, ifsegment.startorsegment.endever becomeNaN, theunwrap()will panic. Consider usingunwrap_orwith a fallback or validating values upstream.crates/rendering/src/layers/captions.rs (2)
131-155: Unused parameter_fade_opacityincalculate_bounce_offset.The first parameter
_fade_opacityis declared but never used in the function body. Consider removing it if not needed.fn calculate_bounce_offset( - _fade_opacity: f32, time_from_start: f32, time_to_end: f32, fade_duration: f32, ) -> f32 {Also update the call site at line 371-372:
- let bounce_offset = - calculate_bounce_offset(fade_opacity, time_from_start, time_to_end, fade_duration); + let bounce_offset = + calculate_bounce_offset(time_from_start, time_to_end, fade_duration);
707-715: Redundant scissor rect set at line 712.The scissor rect is set at line 708, then immediately set again to the same values at line 712 before
text_renderer.render(). The second call appears unnecessary.if let Some([x, y, width, height]) = self.background_scissor { pass.set_scissor_rect(x, y, width, height); pass.set_pipeline(&self.background_pipeline); pass.set_bind_group(0, &self.background_bind_group, &[]); pass.draw(0..6, 0..1); - pass.set_scissor_rect(x, y, width, height); } else if self.output_size.0 > 0 && self.output_size.1 > 0 { pass.set_scissor_rect(0, 0, self.output_size.0, self.output_size.1); }crates/frame-converter/src/d3d11.rs (1)
469-475: Limited format support inpixel_to_dxgi.Only NV12 and YUYV422 are supported. Other common formats like RGB/BGRA will fail with an error that incorrectly states the output format is NV12.
Consider improving the error message or extending format support:
fn pixel_to_dxgi(pixel: Pixel) -> Result<DXGI_FORMAT, ConvertError> { match pixel { Pixel::NV12 => Ok(DXGI_FORMAT_NV12), Pixel::YUYV422 => Ok(DXGI_FORMAT_YUY2), - _ => Err(ConvertError::UnsupportedFormat(pixel, Pixel::NV12)), + other => Err(ConvertError::UnsupportedFormat(other, other)), } }crates/enc-ffmpeg/src/video/h264.rs (1)
351-375: Consider validating pre-converted frame format and dimensions.
queue_preconverted_framelogs the frame details but doesn't validate that the frame matches expectedoutput_formatand dimensions. A mismatch could cause encoder errors or corrupt output.Consider adding validation:
pub fn queue_preconverted_frame( &mut self, mut frame: frame::Video, timestamp: Duration, output: &mut format::context::Output, ) -> Result<(), QueueFrameError> { + if frame.format() != self.output_format + || frame.width() != self.output_width + || frame.height() != self.output_height + { + return Err(QueueFrameError::Converter(ffmpeg::Error::InvalidData)); + } + trace!( "Encoding pre-converted frame: format={:?}, size={}x{}, expected={:?} {}x{}", frame.format(),crates/recording/src/benchmark.rs (4)
83-101: RwLock contention may impact performance under high frame rates.Each call to
record_frame_convertedandrecord_frame_encodedacquires a write lock on the timing vectors. At high frame rates (e.g., 60+ FPS), this could become a bottleneck.Consider using lock-free data structures or batching updates. For example, you could use a bounded concurrent queue and drain it periodically:
use crossbeam_channel::Sender; struct PipelineMetrics { conversion_times_tx: Sender<u64>, // ... }Alternatively, if this is acceptable for benchmark scenarios (which typically have lower performance requirements than production), document this tradeoff.
71-77: Unwrap on RwLock could panic if lock is poisoned.Using
.unwrap()on RwLock guards will panic if another thread panicked while holding the lock. Consider using.expect()with a descriptive message or handling the poisoned lock.pub fn start(&self) { - *self.start_time.write().unwrap() = Some(Instant::now()); + *self.start_time.write().expect("start_time lock poisoned") = Some(Instant::now()); } pub fn stop(&self) { - *self.end_time.write().unwrap() = Some(Instant::now()); + *self.end_time.write().expect("end_time lock poisoned") = Some(Instant::now()); }
387-395: Potential panic inpercentile_durationwith edge cases.When
pis 100.0, the index calculation((100.0 / 100.0) * (len - 1)).round()equalslen - 1, which is correct. However, ifp > 100.0, the index could exceed bounds.Add bounds clamping:
fn percentile_duration(times_ns: &[u64], p: f64) -> Option<Duration> { if times_ns.is_empty() { return None; } let mut sorted: Vec<u64> = times_ns.to_vec(); sorted.sort_unstable(); - let idx = ((p / 100.0) * (sorted.len() - 1) as f64).round() as usize; + let idx = ((p.clamp(0.0, 100.0) / 100.0) * (sorted.len() - 1) as f64).round() as usize; Some(Duration::from_nanos(sorted[idx])) }
463-466: Hardcoded GPU detection for macOS.
detect_macos_gpureturns a static string rather than actually detecting the GPU type.Consider using system APIs to detect the actual GPU, or document that this is a placeholder:
#[cfg(target_os = "macos")] fn detect_macos_gpu() -> String { - "Apple Silicon / Intel UHD".to_string() + // TODO: Use IOKit or Metal API to detect actual GPU + "Unknown (VideoToolbox)".to_string() }apps/desktop/src/store/captions.ts (1)
127-144: Consider spreading settings to avoid field drift.The
saveCaptionsmethod explicitly lists each setting field. If new fields are added toCaptionSettingsin the future, they could be missed here, causing data loss on save.Apply this diff to use spread and avoid missing new fields:
const captionsData = { segments: state.segments, - settings: { - enabled: state.settings.enabled, - font: state.settings.font, - size: state.settings.size, - color: state.settings.color, - backgroundColor: state.settings.backgroundColor, - backgroundOpacity: state.settings.backgroundOpacity, - position: state.settings.position, - bold: state.settings.bold, - italic: state.settings.italic, - outline: state.settings.outline, - outlineColor: state.settings.outlineColor, - exportWithSubtitles: state.settings.exportWithSubtitles, - highlightColor: state.settings.highlightColor, - fadeDuration: state.settings.fadeDuration, - }, + settings: { ...state.settings }, };crates/frame-converter/Cargo.toml (1)
20-21: Empty macOS dependencies section.The macOS target dependencies section is empty. If this is a placeholder for future additions, consider removing it until needed to reduce noise.
crates/rendering/src/mask.rs (1)
10-15: Consider caching sorted keyframes to avoid repeated sorting.Both
interpolate_vectorandinterpolate_scalarclone and sort the keyframes array on every call. Since this function is called per-segment per-frame during rendering, this could become a performance bottleneck for segments with many keyframes.Consider pre-sorting keyframes when segments are created/modified, or caching the sorted order.
Also applies to: 42-47
apps/desktop/src/routes/editor/Timeline/ZoomTrack.tsx (1)
404-416:isSelectedmay match wrong segment if multiple segments share same start/end times.The current implementation uses
findIndexcomparing bothstartandendto locate the segment. While unlikely in practice, if two segments had identical timing values, this could match the wrong one. Consider using the loop indexi()directly instead.const isSelected = createMemo(() => { const selection = editorState.timeline.selection; if (!selection || selection.type !== "zoom") return false; - const segmentIndex = project.timeline?.zoomSegments?.findIndex( - (s) => s.start === segment.start && s.end === segment.end, - ); - - // Support both single selection (index) and multi-selection (indices) - if (segmentIndex === undefined || segmentIndex === -1) return false; - - return selection.indices.includes(segmentIndex); + return selection.indices.includes(i()); });crates/recording/src/output_pipeline/async_camera.rs (1)
35-49: Consider documenting the rationale for default capacity values.The default values (worker_count: 4, input_capacity: 120, output_capacity: 90) appear well-chosen, but the reasoning isn't immediately clear. Consider adding a brief explanation in documentation or a related design document for future maintainers.
apps/desktop/src/routes/editor/Timeline/MaskTrack.tsx (3)
85-101: Segment length check may be too permissive.At Line 87, the check
if (length <= 0) returnonly guards against zero/negative length. IftotalDuration()is smaller thanminDuration(), the segment created will be shorter than the minimum visual threshold, potentially causing UI issues.const addSegmentAt = (time: number) => { - const length = Math.min(minDuration(), totalDuration()); - if (length <= 0) return; + const length = Math.min(minDuration(), totalDuration()); + if (length < minDuration() && totalDuration() >= minDuration()) return; + if (length <= 0) return;Alternatively, this may be intentional to allow small segments when the total duration is small. Consider whether this edge case needs handling.
103-111: Fallback time calculation uses raw pixel offset without bounds check.Line 109 calculates time from pixel position but doesn't guard against
timelineBounds.leftbeing undefined beyond the nullish coalescing. The formulasecsPerPixel() * (e.clientX - (timelineBounds.left ?? 0))could produce unexpected values if bounds aren't ready.Consider adding an early return if timeline bounds aren't fully initialized:
const handleBackgroundMouseDown = (e: MouseEvent) => { if (e.button !== 0) return; if ((e.target as HTMLElement).closest("[data-mask-segment]")) return; + if (timelineBounds.left === undefined) return; const timelineTime = editorState.previewTime ?? editorState.playbackTime ?? - secsPerPixel() * (e.clientX - (timelineBounds.left ?? 0)); + secsPerPixel() * (e.clientX - timelineBounds.left); addSegmentAt(timelineTime); };
282-288: Redundant sorting after each segment update.The pattern of calling
setProjectto update a segment property and then immediately callingsetProjectagain to sort appears three times (start handle, content drag, end handle). This triggers two separate state updates per drag event.Consider combining the update and sort into a single
producecall:-(e, value, initialMouseX) => { - const delta = (e.clientX - initialMouseX) * secsPerPixel(); - const next = Math.max( - value.minValue, - Math.min(value.maxValue, value.start + delta), - ); - setProject("timeline", "maskSegments", i(), "start", next); - setProject( - "timeline", - "maskSegments", - produce((items) => { - items.sort((a, b) => a.start - b.start); - }), - ); -}, +(e, value, initialMouseX) => { + const delta = (e.clientX - initialMouseX) * secsPerPixel(); + const next = Math.max( + value.minValue, + Math.min(value.maxValue, value.start + delta), + ); + setProject( + "timeline", + "maskSegments", + produce((items) => { + items[i()].start = next; + items.sort((a, b) => a.start - b.start); + }), + ); +},Also applies to: 320-326, 359-365
crates/rendering/src/layers/text.rs (1)
141-151: Error handling logs warnings but continues execution.Using
warn!for prepare/render failures is appropriate for a rendering layer where occasional failures shouldn't crash the application. However, consider whether upstream callers need to know about failures.If upstream code needs to react to rendering failures (e.g., skip compositing), consider returning a
Resultor a success boolean instead of silently logging.Also applies to: 154-161
apps/desktop/src/routes/editor/CaptionsTab.tsx (1)
380-418: Segment operations duplicate store logic.The
deleteSegment,updateSegment, andaddSegmentfunctions duplicate similar logic found inapps/desktop/src/store/captions.ts(referenced in relevant snippets). Consider using the store methods directly for consistency.Based on the relevant code snippets showing
addSegment,updateSegmentin the captions store, consider importing and using those methods instead of duplicating the logic here.crates/frame-converter/examples/benchmark.rs (2)
77-77: Clippy lint: preferwhile let Some(..)overwhile let Some(_).Using
..instead of_for unused bindings is more idiomatic and may satisfy stricter clippy configurations.-while let Some(_) = pool.try_recv() { +while let Some(..) = pool.try_recv() { received += 1; }
236-236: Same clippy lint: preferSome(..)pattern.-if let Some(_) = pool.recv_timeout(Duration::from_millis(100)) { +if let Some(..) = pool.recv_timeout(Duration::from_millis(100)) {apps/desktop/src/routes/editor/TextOverlay.tsx (2)
229-233: Unused variabledeltaPxXin corner resize logic.The variable
deltaPxXis computed but never used. The scaling uses onlyscaleY. If uniform scaling is intended, this is correct but the variable should be removed to avoid confusion.if (isCorner) { - const currentWidthPx = startSize.x * props.size.width; const currentHeightPx = startSize.y * props.size.height; - - const deltaPxX = dx * props.size.width * dirX; const deltaPxY = dy * props.size.height * dirY; const scaleY = (currentHeightPx + deltaPxY) / currentHeightPx; const scale = scaleY;
26-27: Consider extractingclampto a shared utility.The
clampfunction is commonly needed across components. It could be imported from a shared utilities module if one exists, or created for reuse.apps/desktop/src/routes/editor/Timeline/index.tsx (1)
49-80: Consider hoistingtrackDefinitionsoutside the component.The
trackDefinitionsarray is static and doesn't depend on component state or props. Moving it outside theTimelinefunction would avoid recreating the array on each render.+const trackDefinitions: TrackDefinition[] = [ + { + type: "clip", + label: "Clip", + icon: trackIcons.clip, + locked: true, + }, + // ... rest of definitions +]; + export function Timeline() { - const trackDefinitions: TrackDefinition[] = [ - // ... - ];apps/desktop/src/routes/editor/MaskOverlay.tsx (1)
154-158: Remove inline comments per coding guidelines.As per coding guidelines, comments should not be added to code. The code should be self-explanatory through naming and structure.
- {/* Border/Highlight */} <div class="absolute inset-0 rounded-md border-2 border-blue-9 bg-blue-9/10 cursor-move" /> - {/* Handles */} - {/* Corners */} <ResizeHandleAlso remove the
{/* Sides */}comment on line 176.crates/rendering/src/text.rs (2)
16-29: Consider supporting 8-character hex colors for alpha transparency.The
parse_colorfunction only handles 6-character hex codes (RGB), always returning alpha = 1.0. IfTextSegment.colorcan include alpha (e.g.,#RRGGBBAA), this would silently ignore it.fn parse_color(hex: &str) -> [f32; 4] { let color = hex.trim_start_matches('#'); - if color.len() == 6 { - if let (Ok(r), Ok(g), Ok(b)) = ( + if color.len() == 6 || color.len() == 8 { + if let (Ok(r), Ok(g), Ok(b)) = ( u8::from_str_radix(&color[0..2], 16), u8::from_str_radix(&color[2..4], 16), u8::from_str_radix(&color[4..6], 16), ) { - return [r as f32 / 255.0, g as f32 / 255.0, b as f32 / 255.0, 1.0]; + let a = if color.len() == 8 { + u8::from_str_radix(&color[6..8], 16).unwrap_or(255) + } else { + 255 + }; + return [r as f32 / 255.0, g as f32 / 255.0, b as f32 / 255.0, a as f32 / 255.0]; } } [1.0, 1.0, 1.0, 1.0] }
44-47: Consider using aHashSetforhidden_indicesif segment count grows.The
hidden_indices.contains(&i)lookup is O(n) per segment. For small segment counts this is fine, but if segment counts grow, switching toHashSet<usize>would make lookups O(1).crates/recording/examples/encoding-benchmark.rs (3)
133-142: Conversion duration measurement may be inaccurate.The
conversion_durationis measured asconversion_end.duration_since(receive_time), butreceive_timeis set beforepool.submit(). This includes the time spent insubmit()(which may block if queues are full), not just the actual conversion time. For accurate conversion timing, consider using the timestamp from when the frame was actually picked up by the worker.
147-163: Hardcoded conversion time during drain phase skews metrics.During the drain loop,
metrics.record_frame_converted(Duration::from_millis(1))uses a fixed 1ms value instead of actual conversion time. This underestimates the average conversion time in the final metrics.Consider tracking actual conversion time by storing submission timestamps in the pool's output frames, or simply not recording conversion time during drain.
407-419: Unknown--modevalues silently run full benchmark.The
"full" | _pattern means any unrecognized mode (e.g.,--mode typo) will run the full benchmark without warning. Consider printing a warning for unrecognized modes.match mode { "formats" => benchmark_conversion_formats(&config), "encode" => benchmark_encode_times(&config), "workers" => benchmark_worker_counts(&config), "resolutions" => benchmark_resolutions(&config), - "full" | _ => { + "full" => { + benchmark_conversion_formats(&config); + benchmark_encode_times(&config); + benchmark_worker_counts(&config); + benchmark_resolutions(&config); + run_full_benchmark(&config); + } + unknown => { + eprintln!("Unknown mode '{}', running full benchmark", unknown); benchmark_conversion_formats(&config); benchmark_encode_times(&config); benchmark_worker_counts(&config); benchmark_resolutions(&config); run_full_benchmark(&config); } }crates/recording/examples/recording-benchmark.rs (2)
218-221: Unsafeset_varis unsound in multi-threaded contexts.
std::env::set_varis marked unsafe in recent Rust editions because modifying environment variables is inherently racy in multi-threaded programs. Since this is within#[tokio::main], threads may already be spawned.Consider setting the environment variable before the tokio runtime starts, or use a safer initialization pattern:
-#[tokio::main] -async fn main() -> Result<(), Box<dyn std::error::Error>> { - unsafe { std::env::set_var("RUST_LOG", "info") }; - tracing_subscriber::fmt::init(); +fn main() -> Result<(), Box<dyn std::error::Error>> { + std::env::set_var("RUST_LOG", "info"); + tracing_subscriber::fmt::init(); + + tokio::runtime::Builder::new_multi_thread() + .enable_all() + .build()? + .block_on(async_main()) +} + +async fn async_main() -> Result<(), Box<dyn std::error::Error>> {
271-271: Catch-all pattern after explicit match arm is redundant.The pattern
"full" | _makes"full"unreachable since_already matches everything. If the intent is to treat unknown modes as "full", consider clarifying:- "full" | _ => { + _ => { println!("Mode: Full benchmark suite\n");Or add a warning for unrecognized modes before falling back to full.
crates/recording/examples/camera-benchmark.rs (2)
173-173: Silently ignoring write_header error may mask configuration issues.Using
.ok()discards any error fromwrite_header(). If the output context is misconfigured, this could lead to confusing downstream failures.Consider at least logging the error:
- output.write_header().ok(); + if let Err(e) = output.write_header() { + warn!("Failed to write output header: {}", e); + }
319-322: Same unsafe set_var issue as recording-benchmark.rs.See previous comment on recording-benchmark.rs regarding
unsafe { std::env::set_var(...) }in async context.Apply the same fix pattern as suggested for recording-benchmark.rs - set env var before runtime initialization.
crates/rendering/src/layers/mask.rs (1)
44-49: Bind group recreated on every render call - potential optimization opportunity.A new bind group is created for each
render()call. While this is correct, if the same mask is rendered repeatedly with only uniform changes, the bind group could be cached and only recreated when the texture view changes.This is a minor optimization that could be deferred. The current implementation is correct but creates allocation pressure for frequently updated masks.
apps/desktop/src/routes/editor/masks.ts (1)
76-79: Consider exportingsortByTimeandtimeMatchutilities.These helper functions could be useful for text segment keyframes or other timeline utilities. If they're intentionally private, this is fine.
crates/recording/src/output_pipeline/macos.rs (1)
142-193: Consider extracting common muxer setup logic.
AVFoundationCameraMuxerduplicates theMuxertrait implementation fromAVFoundationMp4Muxer(lines 46-82). Both have identicalsetupandfinishmethods. Consider extracting shared logic or using a generic wrapper.apps/desktop/src/routes/editor/Timeline/TextTrack.tsx (1)
280-286: Repeated sorting after each segment update.Each drag update calls
segments.sort()viaproduce. For many segments, this could be optimized by deferring sort until drag ends, or using binary insertion for single-segment moves.Also applies to: 318-324, 355-361
apps/desktop/src/routes/editor/ConfigSidebar.tsx (1)
2664-2719: Duplicated font weight options array.The weight options array is defined twice - once in the
KSelectoptions prop and again in theKSelect.Valuerender function. Extract to a constant:+const FONT_WEIGHT_OPTIONS = [ + { label: "Thin", value: 100 }, + { label: "Extra Light", value: 200 }, + { label: "Light", value: 300 }, + { label: "Normal", value: 400 }, + { label: "Medium", value: 500 }, + { label: "Semi Bold", value: 600 }, + { label: "Bold", value: 700 }, + { label: "Extra Bold", value: 800 }, + { label: "Black", value: 900 }, +]; <KSelect - options={[ - { label: "Thin", value: 100 }, - ... - ]} + options={FONT_WEIGHT_OPTIONS} ... > <KSelect.Value<any> class="truncate"> {(state) => { const selected = state.selectedOption(); if (selected) return selected.label; const weight = props.segment.fontWeight; - const option = [ - { label: "Thin", value: 100 }, - ... - ].find((o) => o.value === weight); + const option = FONT_WEIGHT_OPTIONS.find((o) => o.value === weight); return option ? option.label : weight.toString(); }} </KSelect.Value> </KSelect>crates/recording/src/output_pipeline/win.rs (3)
405-414: Consider handling unrecognized pixel formats explicitly.The fallback to
DXGI_FORMAT_NV12for unrecognized pixel formats may produce incorrect results silently. Consider returning an error or logging a warning for unsupported formats.pub fn dxgi_format(&self) -> DXGI_FORMAT { match self.pixel_format { cap_camera_windows::PixelFormat::NV12 => DXGI_FORMAT_NV12, cap_camera_windows::PixelFormat::YUYV422 => DXGI_FORMAT_YUY2, cap_camera_windows::PixelFormat::UYVY422 => DXGI_FORMAT_UYVY, - _ => DXGI_FORMAT_NV12, + other => { + tracing::warn!("Unsupported pixel format {:?}, defaulting to NV12", other); + DXGI_FORMAT_NV12 + } } }
567-618: First frame handling logic is complex - consider simplification.The
process_frameclosure capturesfirst_timestampby reference in an unusual pattern. The frame count check inside the encoder's input closure creates a non-obvious flow where the first frame is handled specially.Consider restructuring to process the first frame before entering the encoder loop to simplify the control flow.
609-615: Silently ignoring muxer write errors.The
write_sampleerror is captured but only converted to a string and discarded. This could hide real encoding issues.|output_sample| { let mut output = output.lock().unwrap(); - let _ = muxer + if let Err(e) = muxer .write_sample(&output_sample, &mut *output) - .map_err(|e| format!("WriteSample: {e}")); + { + tracing::warn!("WriteSample error: {e}"); + } Ok(()) },apps/desktop/src-tauri/src/windows.rs (1)
327-332: Consider extracting magic window level constant.The hardcoded value
50for the window level lacks context. Consider using a named constant or referencing an existingNSWindowLevelconstant for clarity, similar to how other window levels are handled inCapWindowDef::window_level().apps/desktop/src/routes/editor/context.ts (1)
130-147: Type assertions used for maskSegments/textSegments access.The code uses type assertions (
as TimelineConfiguration & { maskSegments?: MaskSegment[] }) to access these fields. This suggests the baseTimelineConfigurationtype from tauri.ts may not include these fields yet. Consider updating the generated types to includemaskSegmentsandtextSegmentsto eliminate these casts.crates/project/src/configuration.rs (1)
749-774: Consider usingCaptionPositionenum forCaptionSettings.position.A
CaptionPositionenum is defined at lines 749-759, butCaptionSettings.positionat line 773 is typed asString. This creates potential inconsistency and loses type safety. Consider updating the field to use the enum type directly.pub struct CaptionSettings { pub enabled: bool, pub font: String, pub size: u32, pub color: String, #[serde(alias = "backgroundColor")] pub background_color: String, #[serde(alias = "backgroundOpacity")] pub background_opacity: u32, #[serde(default)] - pub position: String, + pub position: CaptionPosition,Note: This would require updating
CaptionSettings::default()to useCaptionPosition::BottomCenterinstead of"bottom-center".to_string().
| fn objc2_nswindow(&self) -> &NSWindow { | ||
| // SAFETY: This cast is safe as the existence of the WebviewWindow means it's attached to an NSWindow | ||
| unsafe { | ||
| &*self | ||
| .ns_window() | ||
| .expect("WebviewWindow is always backed by NSWindow") | ||
| .cast() | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n apps/desktop/src-tauri/src/platform/macos/mod.rs | head -50Repository: CapSoftware/Cap
Length of output: 174
🌐 Web query:
tauri WebviewWindow ns_window safety guarantees macOS NSWindow lifetime
💡 Result:
Short answer: Tauri does not give a Rust lifetime guarantee for the raw NSWindow pointer returned by WebviewWindow::ns_window(); it exposes a raw pointer/opaque handle for use only in platform-specific, unsafe blocks (and many AppKit calls must be made on the main thread). You must treat the pointer as transient—do not store it expecting Rust to keep the Cocoa object alive—call into it only while you know the window is alive (e.g., inside setup/with_webview callbacks or synchronised to the app main thread) and avoid returning or holding that raw pointer across async/threads. See the WebviewWindow docs and macOS implementation notes. [1][2][3]
Why / practical rules:
- ns_window() exposes the platform NSWindow handle for platform extensions; it’s an unsafe, non-owning pointer (no Rust lifetime is enforced). Use it only in unsafe blocks and only while the window is known to exist. [1]
- AppKit UI work must run on the main thread / be dispatched to main; Tauri’s macOS implementation dispatches UI ops to main to be thread-safe. Don’t call Cocoa APIs from background threads. [3]
- There are macOS-specific behavior/compat issues (e.g. content-protection and ScreenCaptureKit) that are managed at the NSWindow level; those are platform changes you should handle via proper AppKit calls while the window is alive. [2]
Practical checklist:
- Only call window.ns_window() inside setup/with_webview or while you hold a valid WebviewWindow reference.
- Wrap Cocoa calls in unsafe { } and run them on the main thread (dispatch if needed).
- Don’t store or transmute the returned pointer as a long‑lived Rust reference; if you need longer-lived control, keep the Tauri Window/WebviewWindow handle and re-fetch ns_window when needed.
- When in doubt, use Tauri/tao high‑level APIs or open an issue/PR on the Tauri repo for a safe extension.
Sources:
[1] Tauri WebviewWindow docs (ns_window usage examples).
[2] Tauri GitHub issue discussing NSWindow/content protection behavior on macOS.
[3] tauri/tao macOS implementation notes re: AppKit thread-safety and ns_window extension.
🏁 Script executed:
git -C /tmp/Cap log --oneline -1 2>/dev/null || echo "Repo not available"Repository: CapSoftware/Cap
Length of output: 174
🏁 Script executed:
# Search for the file and examine its structure
fd "mod\.rs" -p "*macos*" 2>/dev/null | head -5Repository: CapSoftware/Cap
Length of output: 174
Remove comment and redesign method to avoid unsound pointer-to-reference conversion
This code violates two requirements:
-
Comment violation: The
// SAFETY:comment contradicts the no-comments guideline for Rust files. -
Unsound unsafe code: According to Tauri's contract,
ns_window()returns a non-owning, transient raw pointer with no Rust lifetime guarantee. Converting it to a stable reference (&NSWindow) is unsound—if theWebviewWindowis dropped or the window is deallocated, the returned reference becomes dangling. The pointer must not be stored or converted to a reference that outlives the window.
Required changes:
- Remove the
// SAFETY:comment. - Redesign this method to avoid creating a long-lived Rust reference from a transient pointer. Either:
- Return a raw pointer (
*const NSWindow) if callers need unsafe access, or - Provide a callback-based API where unsafe work happens synchronously while the window is known to be alive.
- Return a raw pointer (
- Ensure any AppKit calls dispatch to the main thread, per Tauri/tao requirements.
Consult Tauri's ns_window() documentation and macOS implementation notes to align with the actual pointer lifetime contract.
| <KSwitch class="relative" {...others}> | ||
| <KSwitch.Input class="peer absolute inset-0 opacity-0 cursor-pointer" /> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n apps/desktop/src/components/Toggle.tsxRepository: CapSoftware/Cap
Length of output: 174
Preserve relative positioning when consumers pass a class prop on Toggle
With <KSwitch class="relative" {...others}>, any class passed into Toggle via props will override "relative" (Solid merges props with later spreads taking precedence). This breaks the absolute input overlay, since absolute inset-0 requires the parent to be position: relative. If callers customize class, the root loses its positioning context, shrinking or misaligning the interactive area.
Extract class from props and concatenate it with the required "relative" class:
-const [local, others] = splitProps(props, ["size"]);
+const [local, others] = splitProps(props, ["size", "class"]);
@@
- return (
- <KSwitch class="relative" {...others}>
+ return (
+ <KSwitch class={`relative ${local.class ?? ""}`} {...others}>
<KSwitch.Input class="peer absolute inset-0 opacity-0 cursor-pointer" />
<KSwitch.Control class={toggleControlStyles({ size: local.size })}>
<KSwitch.Thumb class={toggleThumbStyles({ size: local.size })} />
</KSwitch.Control>
</KSwitch>
);This ensures the required positioning is always applied while still allowing external styling.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/desktop/src/components/Toggle.tsx around lines 43-44, the root <KSwitch
class="relative" {...others}> lets a consumer-provided class in {...others}
overwrite "relative", breaking the absolute-positioned input; extract the
incoming class from props (e.g. const { class: incomingClass, ...others } =
props) and supply a composed class that always includes "relative" plus the
consumer class (e.g. class={`relative ${incomingClass ?? ''}`} or via classList
merging) when rendering <KSwitch>, then spread the remaining props so external
styling is preserved without losing the required positioning.
| let frame = frame_renderer | ||
| .render( | ||
| current.segment_frames, | ||
| current.uniforms, | ||
| ¤t.cursor, | ||
| &mut layers, | ||
| ) | ||
| .await | ||
| .unwrap(); |
There was a problem hiding this comment.
Consider handling render errors gracefully instead of unwrapping.
The .unwrap() on line 153 will panic if frame_renderer.render() returns an error, potentially crashing the renderer task. Consider logging the error and continuing, or propagating it appropriately.
- let frame = frame_renderer
+ let frame = match frame_renderer
.render(
current.segment_frames,
current.uniforms,
¤t.cursor,
&mut layers,
)
.await
- .unwrap();
-
- (self.frame_cb)(frame);
+ {
+ Ok(frame) => frame,
+ Err(e) => {
+ eprintln!("Render error: {e}");
+ let _ = current.finished.send(());
+ continue;
+ }
+ };
+
+ (self.frame_cb)(frame);🤖 Prompt for AI Agents
In crates/editor/src/editor.rs around lines 145 to 153, the call to
frame_renderer.render(...).await currently uses .unwrap() which will panic on
error; change this to handle the Result instead — use match or if let to capture
Ok(frame) and Err(e), log the error with context (including e) and either
continue the loop (skip this frame) or return/propagate the error depending on
the surrounding control flow; ensure the code does not call unwrap() and that
the chosen path cleans up any state or yields a sensible fallback so the
renderer task won't crash.
| unsafe impl Send for SwscaleConverter {} | ||
| unsafe impl Sync for SwscaleConverter {} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for scaling::Context usage patterns
rg -n "scaling::Context" --type rust -A 3 -B 2
# Check for other unsafe Send/Sync implementations
rg -n "unsafe impl (Send|Sync)" --type rust
# Find the SwscaleConverter struct definition
fd -e rs swscale && cat -n crates/frame-converter/src/swscale.rs | head -80Repository: CapSoftware/Cap
Length of output: 174
🌐 Web query:
ffmpeg-next crate scaling Context thread safety Send Sync
💡 Result:
Short answer: treat ffmpeg-next's scaling Context as not thread-safe to share across threads. Create one Context per thread (or protect it with a Mutex) and avoid calling its methods from multiple threads simultaneously.
Why / sources:
- ffmpeg-next's scaling API is a thin wrapper around libswscale; libswscale historically requires care with concurrency and is safe when each SwsContext is used by a single thread (or you synchronize access). (stackoverflow.com)
- ffmpeg-next docs show the software::scaling::Context as the Rust wrapper around that functionality; the crate does not advertise it as Send/Sync (so you should not assume it is safe to share without synchronization). Use one Context per thread or protect it. (docs.rs)
- Recent libswscale work added a more dynamic API, but that doesn’t change the rule that contexts/state must be used safely across threads — prefer distinct contexts or explicit locking. (ffmpeg.org)
Practical recommendations:
- Create a Context::get per worker thread and call run()/run_into()/etc. from that thread only.
- If you must share a Context, wrap it in Arc<Mutex<...>> (or equivalent) and ensure only one thread uses it at a time.
- For high-throughput parallel scaling, spawn multiple contexts (one per thread) rather than serializing a single context.
If you want, I can check the exact ffmpeg-next version you use and inspect the crate source to confirm whether Context has explicit Send/Sync impls.
Citations:
- 1: https://stackoverflow.com/questions/16154197/how-to-use-ffmpegs-sws-scale-from-multiple-threads?utm_source=openai
- 2: https://docs.rs/crate/ffmpeg-next/latest?utm_source=openai
- 3: https://ffmpeg.org/pipermail/ffmpeg-devel/2024-October/334732.html?utm_source=openai
Remove or justify unsafe impl Send and unsafe impl Sync for SwscaleConverter.
The scaling::Context from ffmpeg-next is not marked as Send/Sync by the crate and requires explicit synchronization to share across threads. While the Mutex provides safe access serialization, declaring these traits unsafe asserts incorrect thread-safety guarantees. Either remove these implementations and let type inference restrict usage, or provide specific documentation explaining why scaling::Context is safe in this specific usage pattern.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR is being reviewed by Cursor Bugbot
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Bug: Screenshot editor path registration removed causing panic
The CapWindow::ScreenshotEditor path registration code was removed during refactoring, but the def() method at lines 833-836 still expects the path to exist in ScreenshotEditorWindowIds and calls .unwrap() on the lookup result. Since nothing adds entries to the registry anymore (the registration block that existed before this change was deleted), opening a screenshot editor window will panic when .unwrap() is called on None. Compare this to CapWindow::Editor which correctly preserves its registration logic at lines 264-277 before calling def().
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
apps/desktop/src-tauri/src/windows.rs (2)
388-391: Critical bug: undefined variablewindow_builder.On line 390,
window_builderis referenced but the variable is namedbuilder(defined on line 364). This will cause a compilation error.Apply this diff to fix:
#[cfg(windows)] { - builder = window_builder.inner_size(100.0, 100.0).position(0.0, 0.0); + builder = builder.inner_size(100.0, 100.0).position(0.0, 0.0); }
833-838: Potential panic: Screenshot editor path not registered before lookup.The
ScreenshotEditorvariant calls.unwrap()on the path lookup, but unlikeEditor(which registers the path at lines 264-277 before callingdef()), there's no corresponding registration logic forScreenshotEditorin theshow()method. This will panic when opening a screenshot editor.Either add registration logic similar to
Editorin theshow()method before line 460, or handle the missing path gracefully:CapWindow::ScreenshotEditor { path } => { let state = app.state::<ScreenshotEditorWindowIds>(); let s = state.ids.lock().unwrap(); - let id = s.iter().find(|(p, _)| p == path).unwrap().1; + let id = s + .iter() + .find(|(p, _)| p == path) + .map(|(_, id)| *id) + .unwrap_or_else(|| { + drop(s); + let id = state.counter.fetch_add(1, std::sync::atomic::Ordering::SeqCst); + state.ids.lock().unwrap().push((path.clone(), id)); + id + }); CapWindowDef::ScreenshotEditor { id } }Alternatively, add registration logic in the
ScreenshotEditorarm ofshow()similar to lines 264-277.
🧹 Nitpick comments (3)
apps/desktop/src-tauri/src/windows.rs (1)
197-211: Magic number 45 for window level could use a named constant.The hardcoded value
45forTargetSelectOverlayandCaptureAreawindow levels is unclear. WhileNSMainMenuWindowLevelandNSScreenSaverWindowLevelare well-documented constants,45lacks context.Consider defining a named constant or adding inline documentation:
+ // Level above most windows but below screen saver + // CGWindowLevelForKey(kCGMaximumWindowLevelKey) - 1 or similar Self::TargetSelectOverlay { .. } | Self::CaptureArea => Some(45),apps/desktop/src/routes/editor/Timeline/ZoomTrack.tsx (2)
290-398: HoistcreateMouseDownDragoutside the Index callback for better performance.The function is recreated for every segment in the array because it's defined inside the
Indexcallback. Since it doesn't require segment-specific closure state at definition time, consider hoisting it outside the callback and passingsegment,i, and other dependencies as parameters.Apply this pattern:
+const createMouseDownDrag = <T,>( + segmentAccessor: () => ZoomSegment, + segmentIndex: number, + setup: () => T, + _update: (e: MouseEvent, v: T, initialMouseX: number) => void, +) => { + return (downEvent: MouseEvent) => { + // ... existing logic, using segmentAccessor() and segmentIndex + }; +}; <Index each={project.timeline?.zoomSegments}> {(segment, i) => { - const createMouseDownDrag = <T,>( - setup: () => T, - _update: (e: MouseEvent, v: T, initialMouseX: number) => void, - ) => { - return (downEvent: MouseEvent) => { - // ... logic - }; - }; return ( <SegmentRoot onMouseDown={createMouseDownDrag( + segment, + i, () => { /* setup */ }, (e, value, initialMouseX) => { /* update */ }, )}
400-412: O(n) segment search inisSelectedmemo.The memo searches for the segment index by comparing
startandendvalues rather than using the provided indexi. This is O(n) per segment and could impact performance with many zoom segments.This approach appears intentional to handle array mutations (segments being reordered via sorting), as using
idirectly would break when indices change. However, this reveals an architectural fragility: storing indices in selection state is brittle.Consider assigning each segment a unique ID field and using IDs in the selection state instead of indices. This would make selection tracking more robust and efficient:
type ZoomSegment = { id: string; start: number; end: number; // ... }; const isSelected = () => { const selection = editorState.timeline.selection; if (!selection || selection.type !== "zoom") return false; return selection.ids.includes(segment().id); };For the current implementation, the value-based search is acceptable given the constraints.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
apps/desktop/src-tauri/src/lib.rs(22 hunks)apps/desktop/src-tauri/src/windows.rs(27 hunks)apps/desktop/src/routes/editor/Timeline/ZoomTrack.tsx(3 hunks)apps/desktop/src/utils/tauri.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (10)
**/*.{ts,tsx,js,jsx,rs,py,sh}
📄 CodeRabbit inference engine (CLAUDE.md)
Never add any form of comments to code (single-line //, multi-line /* /, documentation ///, //!, /* */, or any other comment syntax). Code must be self-explanatory through naming, types, and structure.
Files:
apps/desktop/src/routes/editor/Timeline/ZoomTrack.tsxapps/desktop/src-tauri/src/windows.rsapps/desktop/src-tauri/src/lib.rsapps/desktop/src/utils/tauri.ts
apps/desktop/src/**/*.{ts,tsx,solid}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/desktop/src/**/*.{ts,tsx,solid}: Use @tanstack/solid-query for server state in desktop app; call generated commands and events from tauri_specta
Never manually import desktop app icons; use unplugin-icons auto-import instead
Files:
apps/desktop/src/routes/editor/Timeline/ZoomTrack.tsxapps/desktop/src/utils/tauri.ts
**/*.{tsx,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use PascalCase for component names; use camelCase starting with 'use' for hook names
Use PascalCase for component names in TypeScript/JavaScript
Files:
apps/desktop/src/routes/editor/Timeline/ZoomTrack.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Enforce strict TypeScript; avoid 'any' type; leverage shared types from packages/utils and other shared packages
Files:
apps/desktop/src/routes/editor/Timeline/ZoomTrack.tsxapps/desktop/src/utils/tauri.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Biome for linting and formatting; run pnpm format and pnpm lint regularly and at the end of each coding session
**/*.{ts,tsx,js,jsx}: Use 2-space indentation in TypeScript files
Format all TypeScript and JavaScript code using Biome (viapnpm format)
Files:
apps/desktop/src/routes/editor/Timeline/ZoomTrack.tsxapps/desktop/src/utils/tauri.ts
**/*.{ts,tsx,js,jsx,rs}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx,rs}: Use kebab-case for file names (e.g.,user-menu.tsx)
Never add comments to code in any language (no//,/* */,///,//!,#, etc.). Code must be self-explanatory through naming, types, and structure
Files:
apps/desktop/src/routes/editor/Timeline/ZoomTrack.tsxapps/desktop/src-tauri/src/windows.rsapps/desktop/src-tauri/src/lib.rsapps/desktop/src/utils/tauri.ts
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Use Rust 2024 edition and run cargo fmt to format all Rust code with rustfmt
**/*.rs: Format all Rust code usingrustfmtand apply workspace clippy lints
Use snake_case for Rust module names and kebab-case for Rust crate names
Files:
apps/desktop/src-tauri/src/windows.rsapps/desktop/src-tauri/src/lib.rs
apps/desktop/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Follow architectural decision to use tauri_specta for strongly typed Rust-to-TypeScript IPC bindings; do not modify generated bindings
Files:
apps/desktop/src-tauri/src/windows.rsapps/desktop/src-tauri/src/lib.rs
apps/desktop/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Never edit auto-generated files: tauri.ts, queries.ts, and files under apps/desktop/src-tauri/gen/
Files:
apps/desktop/src/utils/tauri.ts
{**/tauri.ts,**/queries.ts,apps/desktop/src-tauri/gen/**}
📄 CodeRabbit inference engine (AGENTS.md)
Never edit auto-generated files including
**/tauri.ts,**/queries.ts, andapps/desktop/src-tauri/gen/**
Files:
apps/desktop/src/utils/tauri.ts
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-29T04:31:05.289Z
Learning: Applies to apps/desktop/**/*.rs : Follow architectural decision to use tauri_specta for strongly typed Rust-to-TypeScript IPC bindings; do not modify generated bindings
📚 Learning: 2025-11-29T04:31:05.289Z
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-29T04:31:05.289Z
Learning: Applies to apps/desktop/**/*.rs : Follow architectural decision to use tauri_specta for strongly typed Rust-to-TypeScript IPC bindings; do not modify generated bindings
Applied to files:
apps/desktop/src-tauri/src/windows.rs
🧬 Code graph analysis (1)
apps/desktop/src-tauri/src/lib.rs (2)
apps/desktop/src/utils/tauri.ts (2)
CapWindow(365-365)Camera(357-357)apps/desktop/src-tauri/src/windows.rs (12)
from_str(61-100)app(265-265)app(418-418)app(512-512)app(816-816)app(834-834)app(974-974)app(986-986)label(129-131)get(170-173)get(973-975)get(985-987)
⏰ 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: Clippy (aarch64-apple-darwin, macos-latest)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (11)
apps/desktop/src-tauri/src/lib.rs (6)
96-98: LGTM!Import changes correctly reflect the renamed types from the
windowsmodule.
493-498: LGTM!Correct migration from
ShowCapWindow::CameratoCapWindow::Camera.show().
1430-1445: LGTM!Correct usage of
CapWindowDeffor window label and lookup operations.
2153-2156: LGTM!The
show_windowcommand correctly usesCapWindowas the public API type for window display operations.
2512-2520: LGTM!Correct use of
CapWindowDefvariants for generating window labels in the state persistence denylist.
2837-2853: LGTM!New camera window cleanup handler properly releases resources when the camera window is destroyed, ensuring the camera preview is cleaned up and the camera input is removed when not actively recording.
apps/desktop/src-tauri/src/windows.rs (4)
771-808: LGTM!Good refactoring that centralizes window building configuration. The
window_buildermethod cleanly delegates toCapWindowDeffor labels, titles, and sizing, while applying platform-specific configuration consistently.
745-766: LGTM!Correct use of
run_on_main_threadfor macOSNSWindowconfiguration. The pattern properly applies window-level settings after window creation.
581-588: LGTM!Correct use of
dispatch2::run_on_mainfor setting macOS window collection behavior on the camera window.
39-101: LGTM!Clean enum definition with proper
FromStrimplementation. The legacy identifier handling for "in-progress-recording" maintains backward compatibility.apps/desktop/src/routes/editor/Timeline/ZoomTrack.tsx (1)
269-603: Refactor to direct Index iteration is sound, but see concerns below.The refactor simplifies segment rendering by directly mapping over
project.timeline.zoomSegmentswith<Index>. The overall structure preserves drag, selection, and split functionality correctly.
| s.sort((a, b) => a.start - b.start); | ||
| }), | ||
| ); |
There was a problem hiding this comment.
Selection state may become stale after sorting.
The drag handlers sort zoomSegments in place, which changes array indices. However, the selection state stores indices and is not updated after sorting. This means that after dragging a segment handle that triggers reordering, the selection may point to the wrong segment.
Example:
- Array:
[A, B, C], selection:{ indices: [1] }(segment B) - Drag B's start handle to make it earlier than A
- Array is sorted:
[B, A, C] - Selection remains
{ indices: [1] }, now pointing to A instead of B
The finish function (Line 310) only updates selection when !moved, so drag operations don't refresh selection state.
Consider one of these solutions:
Solution 1: Update selection indices after sorting:
setProject(
"timeline",
"zoomSegments",
produce((s) => {
const segmentIds = s.map((seg, idx) =>
selection?.indices.includes(idx) ? seg : null
);
s.sort((a, b) => a.start - b.start);
const newIndices = segmentIds
.map(seg => seg ? s.indexOf(seg) : -1)
.filter(idx => idx !== -1);
setEditorState("timeline", "selection", {
type: "zoom",
indices: newIndices,
});
}),
);Solution 2: Track segments by identity (start/end values or IDs) in selection state instead of indices, then map to current indices when needed.
Also applies to: 593-596
| .await; | ||
| app_state.camera_in_use = false; | ||
| } | ||
| }); |
There was a problem hiding this comment.
Bug: Camera cleanup misses double-execution guard
The new inline camera cleanup code in the Destroyed event handler is missing the camera_cleanup_done guard that exists in cleanup_camera_window. The CloseRequested event still calls cleanup_camera_window which checks and sets camera_cleanup_done, but the Destroyed event now runs inline code that lacks this check. When both events fire (which happens during normal window close), cleanup runs twice - calling on_window_close() and RemoveInput a second time after the guard was already set.
Additional Locations (1)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
apps/desktop/src-tauri/src/windows.rs (2)
377-407: Critical:window_builderis undefined in the Windows TargetSelectOverlay branch.In the
#[cfg(windows)]block,builder = window_builder.inner_size(...refers to a non‑existentwindow_buildervariable; onlybuilderis in scope. This will not compile and should usebuilderinstead. This was already noted in a previous review and still applies.- #[cfg(windows)] - { - builder = window_builder.inner_size(100.0, 100.0).position(0.0, 0.0); - } + #[cfg(windows)] + { + builder = builder.inner_size(100.0, 100.0).position(0.0, 0.0); + }
823-851: Avoid potential panics when resolving Editor/ScreenshotEditor IDs from registries.Both the
EditorandScreenshotEditorarms indef()usefind(...).unwrap(), which will panic if the path is missing from their respective registries. The currentshow()flow registers paths before callingdef(), but any future caller or inconsistent state would turn into a crash. A small change can make this more robust while preserving the same API surface:- CapWindow::Editor { project_path } => { - let state = app.state::<EditorWindowIds>(); - let s = state.ids.lock().unwrap(); - let id = s.iter().find(|(path, _)| path == project_path).unwrap().1; - CapWindowDef::Editor { id } - } + CapWindow::Editor { project_path } => { + let state = app.state::<EditorWindowIds>(); + let s = state.ids.lock().unwrap(); + let id = s + .iter() + .find(|(path, _)| path == project_path) + .map(|(_, id)| *id) + .unwrap_or_default(); + CapWindowDef::Editor { id } + } @@ - CapWindow::ScreenshotEditor { path } => { - let state = app.state::<ScreenshotEditorWindowIds>(); - let s = state.ids.lock().unwrap(); - let id = s.iter().find(|(p, _)| p == path).unwrap().1; - CapWindowDef::ScreenshotEditor { id } - } + CapWindow::ScreenshotEditor { path } => { + let state = app.state::<ScreenshotEditorWindowIds>(); + let s = state.ids.lock().unwrap(); + let id = s + .iter() + .find(|(p, _)| p == path) + .map(|(_, id)| *id) + .unwrap_or_default(); + CapWindowDef::ScreenshotEditor { id } + }This mirrors earlier feedback about these unwraps while avoiding a hard panic if the registry ever gets out of sync.
🧹 Nitpick comments (3)
apps/desktop/src-tauri/src/windows.rs (3)
264-281: Reduce duplicate scans when registering editor window IDs.The editor registration block scans
state.idstwice (once withany, once withfind). You can simplify and avoid the second search by deriving the ID from a singlefindresult and creating/pushing only onNone:- let window_id = { - let mut s = state.ids.lock().unwrap(); - if !s.iter().any(|(path, _)| path == project_path) { - let id = state - .counter - .fetch_add(1, std::sync::atomic::Ordering::SeqCst); - s.push((project_path.clone(), id)); - id - } else { - s.iter().find(|(path, _)| path == project_path).unwrap().1 - } - }; + let window_id = { + let mut s = state.ids.lock().unwrap(); + if let Some((_, id)) = s.iter().find(|(path, _)| path == project_path) { + *id + } else { + let id = state + .counter + .fetch_add(1, std::sync::atomic::Ordering::SeqCst); + s.push((project_path.clone(), id)); + id + } + };
212-224: Check Editor min_size vs explicit inner_size to avoid conflicting constraints.
CapWindowDef::min_sizeforEditoris(1275.0, 800.0), but theEditorbranch later calls.inner_size(1240.0, 800.0)on the builder. Sincewindow_builderalready sets bothinner_sizeandmin_inner_sizefrommin_size, the explicit.inner_size(1240.0, 800.0)may be clamped or overridden by the larger min size, depending on Tauri/winit behavior. Consider aligning these (either reduce themin_sizewidth or drop the explicitinner_size) so the initial size and minimum are consistent.Also applies to: 467-472, 789-803
197-211: Align RecordingsOverlay window level between CapWindowDef and NSPanel usage.
CapWindowDef::window_levelreturnsNSScreenSaverWindowLevelforRecordingsOverlay, but the NSPanel configuration still callspanel.set_level(NSMainMenuWindowLevel as i32). Since the post‑match macOS block also sets the NSWindow level fromdef.window_level(), whichever runs last wins; having two different levels for the same window is confusing and may be misleading for future changes. It would be clearer to pick one level forRecordingsOverlayand apply it consistently (either by adjustingwindow_level()or by updating/removing thepanel.set_levelcall).Also applies to: 724-735
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/desktop/src-tauri/src/windows.rs(28 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,js,jsx,rs,py,sh}
📄 CodeRabbit inference engine (CLAUDE.md)
Never add any form of comments to code (single-line //, multi-line /* /, documentation ///, //!, /* */, or any other comment syntax). Code must be self-explanatory through naming, types, and structure.
Files:
apps/desktop/src-tauri/src/windows.rs
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Use Rust 2024 edition and run cargo fmt to format all Rust code with rustfmt
**/*.rs: Format all Rust code usingrustfmtand apply workspace clippy lints
Use snake_case for Rust module names and kebab-case for Rust crate names
Files:
apps/desktop/src-tauri/src/windows.rs
apps/desktop/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Follow architectural decision to use tauri_specta for strongly typed Rust-to-TypeScript IPC bindings; do not modify generated bindings
Files:
apps/desktop/src-tauri/src/windows.rs
**/*.{ts,tsx,js,jsx,rs}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx,rs}: Use kebab-case for file names (e.g.,user-menu.tsx)
Never add comments to code in any language (no//,/* */,///,//!,#, etc.). Code must be self-explanatory through naming, types, and structure
Files:
apps/desktop/src-tauri/src/windows.rs
🧠 Learnings (1)
📚 Learning: 2025-11-29T04:31:05.289Z
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-29T04:31:05.289Z
Learning: Applies to apps/desktop/**/*.rs : Follow architectural decision to use tauri_specta for strongly typed Rust-to-TypeScript IPC bindings; do not modify generated bindings
Applied to files:
apps/desktop/src-tauri/src/windows.rs
⏰ 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). (1)
- GitHub Check: Clippy (aarch64-apple-darwin, macos-latest)
🔇 Additional comments (3)
apps/desktop/src-tauri/src/windows.rs (3)
40-56: CapWindowDef metadata wiring looks internally consistent.The enum variants,
FromStr/Displaylabel mapping, titles, dock-activation, window levels, and min sizes all line up and give a single source of truth for window identity and behavior, which should simplify future changes and avoid label/title drift.Also applies to: 58-101, 103-225
759-779: Centralized macOS window customization via CapWindowDef is a solid refactor.Using
def.disables_window_buttons(),def.disables_fullscreen(), anddef.window_level()from a singlerun_on_main_threadblock cleanly replaces scattered NSWindow logic and ties macOS‑specific behavior directly to the declarative window definition.
885-889: Content protection refresh now correctly uses CapWindowDef.Iterating over
webview_windows(), parsing labels withCapWindowDef::from_str, and feedingid.title()intoshould_protect_windowgives a consistent, type‑checked way to keep content protection in sync with the user’s exclusion list.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/desktop/src-tauri/src/windows.rs (1)
401-404: Critical bug: undefined variablewindow_builder.On line 403,
window_builderis referenced but the variable is namedbuilder(defined on line 377). This will cause a compilation error.Apply this diff to fix:
#[cfg(windows)] { - builder = window_builder.inner_size(100.0, 100.0).position(0.0, 0.0); + builder = builder.inner_size(100.0, 100.0).position(0.0, 0.0); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/desktop/src-tauri/src/windows.rs(28 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,js,jsx,rs,py,sh}
📄 CodeRabbit inference engine (CLAUDE.md)
Never add any form of comments to code (single-line //, multi-line /* /, documentation ///, //!, /* */, or any other comment syntax). Code must be self-explanatory through naming, types, and structure.
Files:
apps/desktop/src-tauri/src/windows.rs
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Use Rust 2024 edition and run cargo fmt to format all Rust code with rustfmt
**/*.rs: Format all Rust code usingrustfmtand apply workspace clippy lints
Use snake_case for Rust module names and kebab-case for Rust crate names
Files:
apps/desktop/src-tauri/src/windows.rs
apps/desktop/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Follow architectural decision to use tauri_specta for strongly typed Rust-to-TypeScript IPC bindings; do not modify generated bindings
Files:
apps/desktop/src-tauri/src/windows.rs
**/*.{ts,tsx,js,jsx,rs}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx,rs}: Use kebab-case for file names (e.g.,user-menu.tsx)
Never add comments to code in any language (no//,/* */,///,//!,#, etc.). Code must be self-explanatory through naming, types, and structure
Files:
apps/desktop/src-tauri/src/windows.rs
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-29T04:31:05.289Z
Learning: Applies to apps/desktop/**/*.rs : Follow architectural decision to use tauri_specta for strongly typed Rust-to-TypeScript IPC bindings; do not modify generated bindings
📚 Learning: 2025-11-29T04:31:05.289Z
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-29T04:31:05.289Z
Learning: Applies to apps/desktop/**/*.rs : Follow architectural decision to use tauri_specta for strongly typed Rust-to-TypeScript IPC bindings; do not modify generated bindings
Applied to files:
apps/desktop/src-tauri/src/windows.rs
⏰ 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). (1)
- GitHub Check: Clippy (aarch64-apple-darwin, macos-latest)
🔇 Additional comments (6)
apps/desktop/src-tauri/src/windows.rs (6)
10-56: LGTM!The enum refactor from
CapWindowIdtoCapWindowDefis clean and the new import additions support the macOS window management functionality introduced in this PR.
58-225: LGTM!The
FromStrandDisplayimplementations are correctly inverse operations, and the new window property methods are well-structured with appropriate const qualifiers and platform-specific gating.
436-676: LGTM!The window building logic is consistent across variants, and the macOS-specific NSWindow manipulation using
dispatch2::run_on_mainandobjc2_nswindow()properly ensures main-thread execution for AppKit operations.
758-779: LGTM!The consolidated post-build window configuration using
def-based methods (disables_window_buttons(),disables_fullscreen(),window_level()) with proper main-thread dispatch is a clean architectural improvement.
867-894: LGTM!The
should_protect_windowfunction andrefresh_window_content_protectioncommand correctly use the newCapWindowDefAPI to determine window protection based on title matching.
823-853: Verify safety of publicdef()method with unwraps.The
def()method is public and contains unwraps at lines 831 and 849 when looking up Editor and ScreenshotEditor paths. Whileshow()may ensure paths are registered before callingdef(), external callers could invokedef()directly without registration, causing a panic. Verify whetherdef()is called only fromshow()or from other contexts where path registration isn't guaranteed.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
apps/desktop/src-tauri/src/windows.rs (3)
260-297: Editor/ScreenshotEditor ID registration is sound; fmt::Write import can be droppedPre‑registering Editor and ScreenshotEditor paths in EditorWindowIds/ScreenshotEditorWindowIds before calling def() gives stable labels and enables the fast‑path to reuse existing windows, which is a solid pattern. The local
use std::fmt::Write;inside show() is now unused and can be removed to avoid unnecessary lints.- use std::fmt::Write; -
370-407: Fix undefinedwindow_builderin Windows TargetSelectOverlay branchIn the
TargetSelectOverlayarm, the Windows‑only block assigns tobuilder = window_builder..., but onlybuilderis in scope. This will fail to compile on Windows; the mutation should usebuilderitself.- #[cfg(windows)] - { - builder = window_builder.inner_size(100.0, 100.0).position(0.0, 0.0); - } + #[cfg(windows)] + { + builder = builder.inner_size(100.0, 100.0).position(0.0, 0.0); + }
823-852: Potential panic in def() when Editor/ScreenshotEditor IDs are missing
CapWindow::deflooks up Editor and ScreenshotEditor IDs using.iter().find(...).unwrap(). In the normal flow, show() pre‑registers paths so these unwraps succeed, but def() is public and could be called with a project_path/path that was never registered or whose entry was removed, which would panic. To harden against inconsistent state, consider either restricting def() to module‑private use or handling theNonecase explicitly (e.g. by returning a fallback CapWindowDef or signaling an error upstream instead of unwrapping).
🧹 Nitpick comments (2)
apps/desktop/src-tauri/src/windows.rs (2)
212-224: Align Editor min_size() with explicit .inner_size() to avoid conflicting constraintsCapWindowDef::min_size() returns
(1275.0, 800.0)for Editor and is applied in window_builder() as both inner_size and min_inner_size, but the Editor arm in show() later calls.inner_size(1240.0, 800.0). Depending on platform behavior, that smaller inner_size may just be clamped up to the min, but the two constraints are slightly contradictory. Consider either removing the explicit.inner_size(1240.0, 800.0)and relying on min_size(), or updating min_size() to reflect the intended initial Editor dimensions.Also applies to: 463-483
724-735: Centralized macOS NSWindow tweaks are good; reconcile duplicate level settings for RecordingsOverlayThe final
run_on_main_threadblock that toggles traffic lights, fullscreen behavior, and window level based on CapWindowDef centralizes macOS window policy nicely across all variants. One nuance: in the RecordingsOverlay arm you also callpanel.set_level(NSMainMenuWindowLevel as i32), whileCapWindowDef::window_level()returnsNSScreenSaverWindowLevelfor that variant, so whichever call happens last determines the actual level. It would be safer to pick one source of truth for that window’s level to avoid surprising differences between panel and raw NSWindow behavior.Also applies to: 758-779
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/desktop/src-tauri/src/windows.rs(28 hunks)apps/desktop/src/routes/screenshot-editor/Editor.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{ts,tsx,js,jsx,rs,py,sh}
📄 CodeRabbit inference engine (CLAUDE.md)
Never add any form of comments to code (single-line //, multi-line /* /, documentation ///, //!, /* */, or any other comment syntax). Code must be self-explanatory through naming, types, and structure.
Files:
apps/desktop/src/routes/screenshot-editor/Editor.tsxapps/desktop/src-tauri/src/windows.rs
apps/desktop/src/**/*.{ts,tsx,solid}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/desktop/src/**/*.{ts,tsx,solid}: Use @tanstack/solid-query for server state in desktop app; call generated commands and events from tauri_specta
Never manually import desktop app icons; use unplugin-icons auto-import instead
Files:
apps/desktop/src/routes/screenshot-editor/Editor.tsx
**/*.{tsx,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use PascalCase for component names; use camelCase starting with 'use' for hook names
Use PascalCase for component names in TypeScript/JavaScript
Files:
apps/desktop/src/routes/screenshot-editor/Editor.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Enforce strict TypeScript; avoid 'any' type; leverage shared types from packages/utils and other shared packages
Files:
apps/desktop/src/routes/screenshot-editor/Editor.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Biome for linting and formatting; run pnpm format and pnpm lint regularly and at the end of each coding session
**/*.{ts,tsx,js,jsx}: Use 2-space indentation in TypeScript files
Format all TypeScript and JavaScript code using Biome (viapnpm format)
Files:
apps/desktop/src/routes/screenshot-editor/Editor.tsx
**/*.{ts,tsx,js,jsx,rs}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx,rs}: Use kebab-case for file names (e.g.,user-menu.tsx)
Never add comments to code in any language (no//,/* */,///,//!,#, etc.). Code must be self-explanatory through naming, types, and structure
Files:
apps/desktop/src/routes/screenshot-editor/Editor.tsxapps/desktop/src-tauri/src/windows.rs
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Use Rust 2024 edition and run cargo fmt to format all Rust code with rustfmt
**/*.rs: Format all Rust code usingrustfmtand apply workspace clippy lints
Use snake_case for Rust module names and kebab-case for Rust crate names
Files:
apps/desktop/src-tauri/src/windows.rs
apps/desktop/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Follow architectural decision to use tauri_specta for strongly typed Rust-to-TypeScript IPC bindings; do not modify generated bindings
Files:
apps/desktop/src-tauri/src/windows.rs
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-29T04:31:05.289Z
Learning: Applies to apps/desktop/**/*.rs : Follow architectural decision to use tauri_specta for strongly typed Rust-to-TypeScript IPC bindings; do not modify generated bindings
📚 Learning: 2025-11-29T04:31:05.289Z
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-29T04:31:05.289Z
Learning: Applies to apps/desktop/**/*.rs : Follow architectural decision to use tauri_specta for strongly typed Rust-to-TypeScript IPC bindings; do not modify generated bindings
Applied to files:
apps/desktop/src-tauri/src/windows.rs
🧬 Code graph analysis (1)
apps/desktop/src-tauri/src/windows.rs (1)
apps/desktop/src/utils/tauri.ts (2)
Camera(357-357)CapWindow(365-365)
⏰ 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). (1)
- GitHub Check: Clippy (aarch64-apple-darwin, macos-latest)
🔇 Additional comments (4)
apps/desktop/src/routes/screenshot-editor/Editor.tsx (1)
350-351: Crop dialog visual change and hidden bounds – please confirm UXThe crop container no longer clips/rounds (removed rounded/overflow-hidden) and the Cropper now hides bounds (showBounds is false). Implementation looks fine, but this will noticeably change how the crop region is presented; please double-check that this matches the intended design and that selection handles remain clear in common cases.
Also applies to: 386-387
apps/desktop/src-tauri/src/windows.rs (3)
39-258: CapWindowDef/CapWindow enum refactor looks consistent and typesafeThe new CapWindowDef and CapWindow enums, plus FromStr/Display/label()/title()/activates_dock()/min_size(), line up consistently and match the CapWindow union shape on the TS side. This centralization of labels, titles, dock behavior, and sizing should make window handling less error‑prone going forward.
436-461: Settings window behavior and window-hiding logic look goodUsing
CapWindowDef::from_stron each webview window label and hiding Main, TargetSelectOverlay, and Camera when Settings opens is a clean way to prevent clashing UI layers. The subsequent builder configuration for Settings (resizable, centered, non‑maximized) is straightforward and consistent with that intent.
637-677: CaptureArea sizing/positioning and main-window minimization logic look correctThe CaptureArea window is sized and positioned from per‑display logical/physical bounds, and the call to Display::intersects with outer_position/outer_size/scale_factor to decide whether to minimize the Main window gives predictable behavior on multi‑monitor setups. This should reduce cases where the main window stays visible under a capture overlay on the same screen.
…e-cursor-option-to-settings-claude-4.5-opus-high-thinking-64f7 Add circle cursor option to settings
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
apps/desktop/src-tauri/Cargo.toml (1)
114-114: Remove the trailing comment.The trailing comment
#{ version = , features = ["http-proto", "reqwest-client"] }should be removed. As per coding guidelines, comments should not be added to code.apps/desktop/src-tauri/src/windows.rs (2)
262-262: Remove unused import.The
use std::fmt::Write;import is not used in this method. No write formatting operations are present that require this trait.pub async fn show(&self, app: &AppHandle<Wry>) -> tauri::Result<WebviewWindow> { - use std::fmt::Write; - if let Self::Editor { project_path } = &self {
401-404: Critical bug: undefined variablewindow_builder.On line 403,
window_builderis referenced but the variable is namedbuilder(defined on line 377). This will cause a compilation error.#[cfg(windows)] { - builder = window_builder.inner_size(100.0, 100.0).position(0.0, 0.0); + builder = builder.inner_size(100.0, 100.0).position(0.0, 0.0); }apps/desktop/src-tauri/src/lib.rs (2)
2970-2974: Critical bug:CapWindow::from_strdoes not exist.
CapWindowdoes not implementFromStr. TheFromStrtrait is implemented onCapWindowDef, notCapWindow. This will cause a compilation error.#[cfg(target_os = "windows")] fn has_open_editor_window(app: &AppHandle) -> bool { app.webview_windows() .keys() - .any(|label| matches!(CapWindow::from_str(label), Ok(CapWindow::Editor { .. }))) + .any(|label| matches!(CapWindowDef::from_str(label), Ok(CapWindowDef::Editor { .. }))) }
2860-2876: Camera cleanup missing double-execution guard.The inline camera cleanup in the
Destroyedevent handler is missing thecamera_cleanup_doneguard that exists incleanup_camera_window()(lines 588-603). When the camera window closes, bothCloseRequestedandDestroyedevents fire. TheCloseRequestedhandler callscleanup_camera_windowwhich sets the guard, but theDestroyedhandler runs its own inline cleanup without checking the guard, causingon_window_close()andRemoveInputto potentially execute twice.CapWindowDef::Camera => { let app = app.clone(); tokio::spawn(async move { let state = app.state::<ArcLock<App>>(); let mut app_state = state.write().await; + if app_state.camera_cleanup_done { + return; + } + app_state.camera_cleanup_done = true; + app_state.camera_preview.on_window_close(); if !app_state.is_recording_active_or_pending() { let _ = app_state .camera_feed .ask(feeds::camera::RemoveInput) .await; app_state.camera_in_use = false; } }); }
🧹 Nitpick comments (1)
apps/desktop/src-tauri/src/windows.rs (1)
10-10: Remove unusedf64import.The
f64type is a primitive and doesn't require explicit importing. This import appears to be dead code.use std::{ - f64, ops::Deref, path::PathBuf,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
Cargo.toml(2 hunks)apps/desktop/src-tauri/Cargo.toml(2 hunks)apps/desktop/src-tauri/src/lib.rs(23 hunks)apps/desktop/src-tauri/src/recording.rs(9 hunks)apps/desktop/src-tauri/src/screenshot_editor.rs(2 hunks)apps/desktop/src-tauri/src/tray.rs(5 hunks)apps/desktop/src-tauri/src/windows.rs(28 hunks)apps/desktop/src/routes/screenshot-editor/Editor.tsx(2 hunks)apps/desktop/src/utils/tauri.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/desktop/src/utils/tauri.ts
- apps/desktop/src-tauri/src/screenshot_editor.rs
🧰 Additional context used
📓 Path-based instructions (5)
**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
Use React Query hooks with Server Actions for mutations and perform precise cache updates using setQueryData/setQueriesData instead of broad invalidations
Files:
apps/desktop/src/routes/screenshot-editor/Editor.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Use strict TypeScript; avoid any type; leverage shared types from @cap/* packages
Follow camelCase naming for variables and functions; PascalCase for components; hooks must start with 'use' prefix
Use Biome for linting and formatting; match existing formatting conventions in the codebase
Use Tailwind CSS for styling in web components; stay consistent with spacing and tokens
Use static skeletons for loading states that mirror content; avoid bouncing animations
Memoize expensive work, code-split naturally, and use Next/Image for remote assets
**/*.{ts,tsx,js,jsx}: Use 2-space indent for TypeScript files; format with Biome usingpnpm format
Use Biome for code formatting and linting; runpnpm formatregularly
Use kebab-case for file names (e.g.,user-menu.tsx); use PascalCase for components
Files:
apps/desktop/src/routes/screenshot-editor/Editor.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Never add any form of comments to code (single-line //, multi-line /* /, JSDoc /* */, or any other comment syntax); code must be self-explanatory through naming, types, and structure
Directory naming must use lowercase-dashed convention
Files:
apps/desktop/src/routes/screenshot-editor/Editor.tsx
**/*.{ts,tsx,js,jsx,rs}
📄 CodeRabbit inference engine (AGENTS.md)
Never add comments to code (
//,/* */,///,//!,#, etc.); code must be self-explanatory through naming, types, and structure
Files:
apps/desktop/src/routes/screenshot-editor/Editor.tsxapps/desktop/src-tauri/src/recording.rsapps/desktop/src-tauri/src/tray.rsapps/desktop/src-tauri/src/windows.rsapps/desktop/src-tauri/src/lib.rs
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Never use dbg!() macro; use proper logging (tracing::debug!, etc.) instead
Never write let _ = async_fn() which silently drops futures; await or explicitly handle them
Use duration.saturating_sub(other) instead of duration - other to avoid panics on underflow
Merge nested if statements: write 'if a && b { }' instead of 'if a { if b { } }'
Don't call .clone() on Copy types (integers, bools, etc.); copy them directly
Use function references directly: iter.map(foo) instead of iter.map(|x| foo(x))
Accept &[T] or &str instead of &Vec or &String in function parameters for flexibility
Use .is_empty() instead of .len() == 0 or .len() > 0 / .len() != 0
Don't assign () to a variable: write foo(); instead of let _ = foo(); or let x = foo(); when return is unit
Use .unwrap_or(val) instead of .unwrap_or_else(|| val) when the default is a simple/cheap value
Use 'for item in &collection' or 'for (i, item) in collection.iter().enumerate()' instead of 'for i in 0..collection.len()'
Use value.clamp(min, max) instead of manual if chains or .min(max).max(min) patterns
Always handle Result/Option or types marked #[must_use]; never ignore them
**/*.rs: Userustfmtand workspace clippy lints for Rust code formatting and linting
Use snake_case for Rust module names and kebab-case for crate names
Never usedbg!()macro in Rust code; use proper logging instead (Clippy:dbg_macro= deny)
Always handleResult/Optionor types marked#[must_use]; never ignore them (Rust compiler lint:unused_must_use= deny)
Never writelet _ = async_fn()which silently drops futures; await or explicitly handle them (Clippy:let_underscore_future= deny)
Usesaturating_subinstead of-forDurationto avoid panics (Clippy:unchecked_duration_subtraction= deny)
Merge nestedifstatements: useif a && b { }instead ofif a { if b { } }(Clippy:collapsible_if= deny)
Don't call.clone()onCopytypes; just copy them directly (Clippy:clone_on_copy= deny)
U...
Files:
apps/desktop/src-tauri/src/recording.rsapps/desktop/src-tauri/src/tray.rsapps/desktop/src-tauri/src/windows.rsapps/desktop/src-tauri/src/lib.rs
🧠 Learnings (2)
📚 Learning: 2025-12-07T14:29:40.721Z
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T14:29:40.721Z
Learning: Applies to **/*.{ts,tsx,js,jsx,rs} : Never add comments to code (`//`, `/* */`, `///`, `//!`, `#`, etc.); code must be self-explanatory through naming, types, and structure
Applied to files:
apps/desktop/src-tauri/Cargo.toml
📚 Learning: 2025-12-07T14:29:19.166Z
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-07T14:29:19.166Z
Learning: Regenerate auto-generated Tauri bindings by restarting the dev server when Rust types change
Applied to files:
apps/desktop/src-tauri/Cargo.toml
🧬 Code graph analysis (2)
apps/desktop/src-tauri/src/recording.rs (2)
apps/desktop/src/utils/tauri.ts (1)
CapWindow(368-368)apps/desktop/src-tauri/src/windows.rs (11)
app(265-265)app(284-284)app(431-431)app(531-531)app(837-837)app(855-855)app(995-995)app(1007-1007)show(261-790)label(129-131)from_str(61-100)
apps/desktop/src-tauri/src/lib.rs (2)
apps/desktop/src/utils/tauri.ts (2)
CapWindow(368-368)Camera(360-360)apps/desktop/src-tauri/src/windows.rs (13)
from_str(61-100)app(265-265)app(284-284)app(431-431)app(531-531)app(837-837)app(855-855)app(995-995)app(1007-1007)label(129-131)get(170-173)get(994-996)get(1006-1008)
⏰ 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: Clippy (aarch64-apple-darwin, macos-latest)
- GitHub Check: Clippy (x86_64-pc-windows-msvc, windows-latest)
🔇 Additional comments (14)
Cargo.toml (1)
25-25: LGTM! Dependency updates align with the migration goals.The tauri bump to 2.9.3 and addition of
objc2 = "0.6.1"as a workspace dependency support the broader migration from legacy macOS FFI to the objc2 ecosystem.Also applies to: 62-62
apps/desktop/src/routes/screenshot-editor/Editor.tsx (1)
372-372: LGTM! Minor UI refinements.The removal of
rounded overflow-hiddenfrom the crop dialog wrapper and disablingshowBoundsare straightforward UI adjustments.Also applies to: 408-408
apps/desktop/src-tauri/Cargo.toml (2)
122-133: LGTM! Clean migration to objc2 ecosystem.The macOS dependencies have been properly restructured to use the objc2 family of crates with consistent versioning (0.3.2 for objc2-* crates, 0.6.2 for block2). The workspace-level objc2 reference ensures version consistency across the project.
30-46: LGTM! Tauri plugin versions aligned.The tauri-plugin-* dependencies are bumped consistently to the 2.3.x-2.5.x range, aligning with the tauri 2.9.3 workspace dependency.
apps/desktop/src-tauri/src/tray.rs (2)
4-4: LGTM! Import updated to new window type.The import change from
ShowCapWindowtoCapWindowaligns with the codebase-wide refactoring.
408-412: LGTM! Window invocations consistently updated.All tray window invocations have been properly migrated from
ShowCapWindowtoCapWindowvariants (ScreenshotEditor,Editor,Main,Settings) while preserving the existing behavior and.show(&app).awaitpattern.Also applies to: 430-431, 471-476, 511-511
apps/desktop/src-tauri/src/recording.rs (6)
67-67: LGTM! Import updated to new window types.The import correctly brings in both
CapWindow(for creating/showing windows) andCapWindowDef(for querying existing windows).
77-90: LGTM! Camera feed tracking added to both recording variants.The
camera_feed: Option<Arc<CameraFeedLock>>field is now consistently present in bothInstantandStudiovariants ofInProgressRecording, enabling proper camera lifecycle management during recordings.
479-481: LGTM! Window queries use CapWindowDef pattern correctly.The
CapWindowDef::Camera.get(&app)andCapWindowDef::Main.get(&app)calls correctly use the definition enum for querying existing windows.Also applies to: 599-604
564-577: LGTM! Window creation uses CapWindow pattern correctly.
CapWindow::WindowCaptureOccluderandCapWindow::InProgressRecordingare correctly used with.show(&app).awaitfor creating/displaying windows.Also applies to: 595-597
589-593: LGTM! Window label parsing updated to CapWindowDef.The filtering logic correctly uses
CapWindowDef::from_str(label)for parsing window labels and matching againstCapWindowDef::TargetSelectOverlay.
1519-1526: LGTM! Post-recording window handling updated.Both
CapWindow::EditorandCapWindow::RecordingsOverlayare correctly used for post-recording navigation based on user settings.apps/desktop/src-tauri/src/windows.rs (2)
854-859: Same potential panic for screenshot editor path lookup.Similar to the Editor variant, this
.unwrap()could panic if the path is not found.CapWindow::ScreenshotEditor { path } => { let state = app.state::<ScreenshotEditorWindowIds>(); let s = state.ids.lock().unwrap(); - let id = s.iter().find(|(p, _)| p == path).unwrap().1; + let id = s + .iter() + .find(|(p, _)| p == path) + .map(|(_, id)| *id) + .unwrap_or(0); CapWindowDef::ScreenshotEditor { id } }
836-841: Potential panic on missing editor path.The
.unwrap()at line 839 will panic ifproject_pathis not found in theEditorWindowIdsstate. While theshow()method registers the path before callingdef(), this could still fail if called from a different code path or if state becomes inconsistent. Consider using defensive error handling:CapWindow::Editor { project_path } => { let state = app.state::<EditorWindowIds>(); let s = state.ids.lock().unwrap(); - let id = s.iter().find(|(path, _)| path == project_path).unwrap().1; + let id = s + .iter() + .find(|(path, _)| path == project_path) + .map(|(_, id)| *id) + .unwrap_or(0); CapWindowDef::Editor { id } }Verify that
def()is only ever called after the path has been registered inEditorWindowIdsto ensure this is not a masking fix for a deeper state management issue.
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 (2)
apps/desktop/src-tauri/src/lib.rs (2)
1295-1307: Avoidunwrap()when parsing editor window labels intoCapWindowDefIn
create_editor_instance:let CapWindowDef::Editor { id } = CapWindowDef::from_str(window.label()).unwrap() else { return Err("Invalid window".to_string()); };If
window.label()is ever not parseable as aCapWindowDef(e.g. mislabelled window, unexpected call from a non‑editor window), this will panic before reaching theelsebranch.Safer pattern:
- let CapWindowDef::Editor { id } = CapWindowDef::from_str(window.label()).unwrap() else { - return Err("Invalid window".to_string()); - }; + let id = match CapWindowDef::from_str(window.label()) { + Ok(CapWindowDef::Editor { id }) => id, + _ => return Err("Invalid window".to_string()), + };This preserves the intended validation without risking a panic on malformed labels.
2882-2892:hide_dock_iconlogic can panic on unknown window labels viafrom_str(...).unwrap()In the
WindowEvent::Destroyedhandler you have:&& app .webview_windows() .keys() .all(|label| !CapWindowDef::from_str(label).unwrap().activates_dock())If any open window has a label that
CapWindowDef::from_strdoes not recognize (e.g."signin", which is explicitly referenced later inRunEvent::Reopen), this.unwrap()will panic during destruction.Consider treating unknown labels as non‑dock‑activating instead of panicking, for example:
- .all(|label| !CapWindowDef::from_str(label).unwrap().activates_dock()) + .all(|label| { + CapWindowDef::from_str(label) + .map(|id| !id.activates_dock()) + .unwrap_or(true) + })so unrecognized labels simply don’t block hiding the dock icon.
♻️ Duplicate comments (4)
apps/desktop/src-tauri/src/windows.rs (2)
374-404: TargetSelectOverlay builder fix correctly uses the existingbuildervariableThe refactored
TargetSelectOverlaypath now consistently mutates thebuilderreturned fromself.window_builder(...)on both macOS and Windows, resolving the previous undefinedwindow_buildervariable and ensuring platform‑specific sizing/positioning is applied before.build().
763-784: Avoid panics indef()for Editor/ScreenshotEditor window IDs
CapWindow::defcurrently does:let id = s.iter().find(|(path, _)| path == project_path).unwrap().1;and similarly for
ScreenshotEditor, which will panic if the ID registry is ever out of sync (e.g. def() called before registration, or after an entry was pruned). Prior reviews raised the same concern on these lookups.Consider making this resilient by handling the
Nonecase explicitly, for example:- CapWindow::Editor { project_path } => { - let state = app.state::<EditorWindowIds>(); - let s = state.ids.lock().unwrap(); - let id = s.iter().find(|(path, _)| path == project_path).unwrap().1; - CapWindowDef::Editor { id } - } + CapWindow::Editor { project_path } => { + let state = app.state::<EditorWindowIds>(); + let s = state.ids.lock().unwrap(); + let id = s + .iter() + .find(|(path, _)| path == project_path) + .map(|(_, id)| *id) + .unwrap_or(0); + CapWindowDef::Editor { id } + } ... - CapWindow::ScreenshotEditor { path } => { - let state = app.state::<ScreenshotEditorWindowIds>(); - let s = state.ids.lock().unwrap(); - let id = s.iter().find(|(p, _)| p == path).unwrap().1; - CapWindowDef::ScreenshotEditor { id } - } + CapWindow::ScreenshotEditor { path } => { + let state = app.state::<ScreenshotEditorWindowIds>(); + let s = state.ids.lock().unwrap(); + let id = s + .iter() + .find(|(p, _)| p == path) + .map(|(_, id)| *id) + .unwrap_or(0); + CapWindowDef::ScreenshotEditor { id } + }or by changing
def()to return aResult/Optionso callers can decide how to handle a missing mapping instead of panicking.Also applies to: 828-858
apps/desktop/src-tauri/src/lib.rs (2)
2972-2978:has_open_editor_windownow correctly uses CapWindowDef parsingReplacing the non‑existent
CapWindow::from_strcall withCapWindowDef::from_str(label)and matching onCapWindowDef::Editor { .. }fixes the earlier critical bug and matches how labels are parsed elsewhere.
2757-2759: Camera cleanup still runs twice (CloseRequested + Destroyed) without a shared guard
WindowEvent::CloseRequestedfor the camera window spawnscleanup_camera_window(app.clone()), which correctly usescamera_cleanup_doneto ensure idempotent cleanup. However,WindowEvent::Destroyedalso has aCapWindowDef::Cameraarm that runs inline cleanup (callingon_window_close()andRemoveInput) without consulting or settingcamera_cleanup_done.When both events fire (the normal close path), cleanup now runs twice again, matching the previously reported issue.
One straightforward fix is to route the Destroyed arm through the same helper:
- CapWindowDef::Camera => { - let app = app.clone(); - tokio::spawn(async move { - let state = app.state::<ArcLock<App>>(); - let mut app_state = state.write().await; - - app_state.camera_preview.on_window_close(); - - if !app_state.is_recording_active_or_pending() { - let _ = app_state - .camera_feed - .ask(feeds::camera::RemoveInput) - .await; - app_state.camera_in_use = false; - } - }); - } + CapWindowDef::Camera => { + let app = app.clone(); + tokio::spawn(cleanup_camera_window(app)); + }so both events share the same guarded logic.
Also applies to: 2861-2876
🧹 Nitpick comments (1)
apps/desktop/src-tauri/src/lib.rs (1)
426-545: Handle or consistently log errors fromCapWindow::showinstead of silently discarding themAcross several call sites (e.g.
set_camera_input,show_window, single‑instance handler, startup logic,RequestOpenRecordingPicker,RequestOpenSettings, macOSRunEvent::Reopen, andopen_project_from_path), the result ofCapWindow::... .show(&app).awaitis ignored vialet _ = ...or.ok(). This means window‑creation failures are effectively invisible to the JS side and often only loosely logged.Given that
show_windowand event handlers already returnResultin some cases, consider a more consistent pattern such as:CapWindow::Camera .show(&app_handle) .await .map_err(|err| { error!("Failed to show camera preview window: {err}"); err.to_string() })?;for commands that return
Result<_, String>, and at least logging the error (withoutlet _ =) for fire‑and‑forget event handlers. This will make diagnosing window‑creation failures much easier.Also applies to: 2166-2169, 2485-2490, 2681-2689, 2724-2727, 2732-2735, 2953-2957, 3249-3250
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
Cargo.toml(2 hunks)apps/desktop/src-tauri/src/lib.rs(23 hunks)apps/desktop/src-tauri/src/windows.rs(27 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Cargo.toml
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Never use dbg!() macro; use proper logging (tracing::debug!, etc.) instead
Never write let _ = async_fn() which silently drops futures; await or explicitly handle them
Use duration.saturating_sub(other) instead of duration - other to avoid panics on underflow
Merge nested if statements: write 'if a && b { }' instead of 'if a { if b { } }'
Don't call .clone() on Copy types (integers, bools, etc.); copy them directly
Use function references directly: iter.map(foo) instead of iter.map(|x| foo(x))
Accept &[T] or &str instead of &Vec or &String in function parameters for flexibility
Use .is_empty() instead of .len() == 0 or .len() > 0 / .len() != 0
Don't assign () to a variable: write foo(); instead of let _ = foo(); or let x = foo(); when return is unit
Use .unwrap_or(val) instead of .unwrap_or_else(|| val) when the default is a simple/cheap value
Use 'for item in &collection' or 'for (i, item) in collection.iter().enumerate()' instead of 'for i in 0..collection.len()'
Use value.clamp(min, max) instead of manual if chains or .min(max).max(min) patterns
Always handle Result/Option or types marked #[must_use]; never ignore them
**/*.rs: Userustfmtand workspace clippy lints for Rust code formatting and linting
Use snake_case for Rust module names and kebab-case for crate names
Never usedbg!()macro in Rust code; use proper logging instead (Clippy:dbg_macro= deny)
Always handleResult/Optionor types marked#[must_use]; never ignore them (Rust compiler lint:unused_must_use= deny)
Never writelet _ = async_fn()which silently drops futures; await or explicitly handle them (Clippy:let_underscore_future= deny)
Usesaturating_subinstead of-forDurationto avoid panics (Clippy:unchecked_duration_subtraction= deny)
Merge nestedifstatements: useif a && b { }instead ofif a { if b { } }(Clippy:collapsible_if= deny)
Don't call.clone()onCopytypes; just copy them directly (Clippy:clone_on_copy= deny)
U...
Files:
apps/desktop/src-tauri/src/windows.rsapps/desktop/src-tauri/src/lib.rs
**/*.{ts,tsx,js,jsx,rs}
📄 CodeRabbit inference engine (AGENTS.md)
Never add comments to code (
//,/* */,///,//!,#, etc.); code must be self-explanatory through naming, types, and structure
Files:
apps/desktop/src-tauri/src/windows.rsapps/desktop/src-tauri/src/lib.rs
🧬 Code graph analysis (1)
apps/desktop/src-tauri/src/lib.rs (2)
apps/desktop/src/utils/tauri.ts (2)
CapWindow(368-368)Camera(360-360)apps/desktop/src-tauri/src/windows.rs (13)
from_str(60-99)app(262-262)app(281-281)app(428-428)app(528-528)app(834-834)app(852-852)app(992-992)app(1004-1004)label(128-130)get(169-172)get(991-993)get(1003-1005)
⏰ 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: Clippy (aarch64-apple-darwin, macos-latest)
- GitHub Check: Clippy (x86_64-pc-windows-msvc, windows-latest)
🔇 Additional comments (14)
apps/desktop/src-tauri/src/windows.rs (7)
38-224: CapWindowDef enum and helper methods look consistent and well‑factoredThe FromStr/Display implementations, label/title, activates_dock, window_level, and min_size helpers appear internally consistent and keep window metadata centralized, which should simplify future changes to window behavior across platforms. No issues spotted here.
259-307: Editor/ScreenshotEditor window ID registration integrates cleanly with new CapWindowDef flowThe logic that assigns stable numeric IDs for
Editor { project_path }andScreenshotEditor { path }before computingCapWindowDeflabels and prewarming editor instances looks sound and should avoid the previous path‑registration issues. The lock/atomic usage is straightforward and appropriate for this low‑contention registry.
520-605: Camera window creation and macOS collection behavior wiring look correctThe camera window builder settings (always‑on‑top, workspace visibility, taskbar skip, transparent, hidden until initialized) and the macOS
dispatch2::run_on_maincall that addsFullScreenAuxiliarycollection behavior align with the intended “floating auxiliary camera preview” behavior. There are no obvious race conditions around the state lock or feed initialization in this block.
639-680: CaptureArea sizing/minimization behavior is coherent across platformsThe new
CaptureAreapath correctly derives bounds fromDisplay::from_id(screen_id)via logical/physical bounds per platform, then minimizes the main window only when its outer rect intersects the target display (usingMonitorExt::intersects). This seems like a sensible consolidation of logic and should avoid spurious minimization.
789-826: Centralizedwindow_buildercorrectly applies title, protection, min size, and decorationsThe new
window_builderhelper usesself.def(app)to assign labels, titles, min sizes, and content protection in one place, and applies macOS‑specific decorations/traffic‑light positions based onCapWindowDefflags. This is a solid simplification over per‑callsite configuration and should make future window variants easier to manage.
885-899:refresh_window_content_protectionnow safely uses CapWindowDef parsingUsing
CapWindowDef::from_str(&label)to recompute titles before re‑evaluatingshould_protect_windowis a good fit with the new enum; theif let Ok(id)guard means unknown/legacy labels will simply be skipped without panicking.
984-1005: Editor/ScreenshotEditorWindowIds helper structs are simple and appropriateWrapping the
(PathBuf, u32)registries and AtomicU32 counters in clonable structs withget(app: &AppHandle)helpers keeps the state access ergonomic while preserving synchronization viaArc<Mutex<...>>. No issues identified here.apps/desktop/src-tauri/src/lib.rs (7)
97-99: Windows module imports match the new CapWindow/CapWindowDef API surfaceImporting
CapWindow,CapWindowDef, the window ID registries, andset_window_transparentfromwindowscleanly aligns this module with the refactored window management API. No issues here.
1444-1470: Recordings overlay panel helpers correctly use CapWindowDef labelsBoth
close_recordings_overlay_windowandfocus_captures_panelnow derive the recordings overlay label viaCapWindowDef::RecordingsOverlay.label(), keeping the nspanel interactions in sync with the centralized label mapping. The non‑macOS path correctly falls back toCapWindowDef::RecordingsOverlay.get(&app).
2084-2089:is_camera_window_opencorrectly reuses CapWindowDef lookupUsing
CapWindowDef::Camera.get(&app).is_some()to implementis_camera_window_openis simple and consistent with the window‑management abstractions inwindows.rs. No issues here.
2526-2534: Window‑state denylist now driven by CapWindowDef labelsFeeding
CapWindowDef::<variant>.label().as_str()intotauri_plugin_window_state’swith_denylistkeeps these entries aligned with the canonical label mapping and avoids drifting string literals. This looks correct and future‑proof for these variants.
2570-2585: Camera feed disconnect handler cleanly closes the camera window when presentThe
OnFeedDisconnecthandler now usesCapWindowDef::Camera.get(&app)to look up and close the camera window if it exists. This is a good use of the centralized window lookup and ensures graceful UI cleanup when the camera feed drops.
2982-2993:reopen_main_windowcorrectly reuses CapWindowDef/CapWindow abstractionsThe Windows‑only
reopen_main_windowhelper now first tries to show/focus an existingCapWindowDef::Mainwindow viaget(&app), and only spawns a newCapWindow::Mainif none exists. This is a clean separation between definition and creation, consistent with the rest of the refactor.
3249-3261:open_project_from_pathuses CapWindow::Editor/Main consistently with new window APIWhen opening a studio recording, spawning
CapWindow::Editor { project_path }.show(&app)matches the new CapWindow surface; for instant recordings, closing an existingCapWindowDef::Mainwindow viaget(&app)is also consistent with the centralized window management. No issues found in this integration.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/desktop/src-tauri/src/windows.rs (1)
345-350: Handle theResultfromrun_on_main_thread.The
_ = window.run_on_main_thread(...)pattern silently drops theResult. Per coding guidelines,Resulttypes should be handled. Use.ok()to explicitly acknowledge ignoring the error.if new_recording_flow { - _ = window.run_on_main_thread({ + window.run_on_main_thread({ let window = window.clone(); move || window.objc2_nswindow().setLevel(50) - }); + }).ok(); }
🧹 Nitpick comments (10)
apps/desktop/src-tauri/src/windows.rs (10)
67-68: Remove the inline comment.As per coding guidelines, comments should not be added to code. The "legacy identifier" comment should be removed; the mapping itself is self-explanatory.
- // legacy identifier "in-progress-recording" => Self::RecordingControls,
116-116: Remove the inline comment.Same as above - remove the "legacy identifier" comment to comply with coding guidelines.
- Self::RecordingControls => write!(f, "in-progress-recording"), // legacy identifier + Self::RecordingControls => write!(f, "in-progress-recording"),
196-210: Consider extracting the magic window level constant.Line 204 uses a hardcoded value
45for the window level. While the other levels use named constants fromobjc2_app_kit, this one is a magic number. Consider defining a named constant for clarity, or add a brief reference to what this level represents.+ const TARGET_SELECT_WINDOW_LEVEL: objc2_app_kit::NSWindowLevel = 45; + match self { Self::RecordingControls => Some(NSMainMenuWindowLevel), - Self::TargetSelectOverlay { .. } | Self::CaptureArea => Some(45), + Self::TargetSelectOverlay { .. } | Self::CaptureArea => Some(TARGET_SELECT_WINDOW_LEVEL), Self::RecordingsOverlay | Self::WindowCaptureOccluder { .. } => { Some(NSScreenSaverWindowLevel) } _ => None, }
433-446: Remove the inline comment.The comment at line 434 violates coding guidelines. The code's intent is clear from the logic itself.
Self::Settings { page } => { - // Hide main window and target select overlays when settings window opens for (label, window) in app.webview_windows() {
487-518: Remove the inline comments.Lines 488 and 503 contain comments that violate coding guidelines. The code behavior is self-explanatory.
Self::Upgrade => { - // Hide main window when upgrade window opens if let Some(main) = CapWindowDef::Main.get(app) { let _ = main.hide(); } ... Self::ModeSelect => { - // Hide main window when mode select window opens if let Some(main) = CapWindowDef::Main.get(app) { let _ = main.hide();
564-567: Remove the inline comment.Line 565 contains a comment that violates coding guidelines.
- .visible(false); // We set this true in `CameraWindowState::init_window` + .visible(false);
669-677: Remove the inline comment.Line 669 contains a comment that violates coding guidelines.
- // Hide the main window if the target monitor is the same if let Some(main_window) = CapWindowDef::Main.get(app)
748-751: Remove the comment and track the issue properly.The comment indicates a known issue with the panel style mask. Per coding guidelines, comments should be removed. If this is a known limitation, consider creating a tracking issue instead of leaving a comment in code.
- // seems like this doesn't work properly -_- #[allow(non_upper_case_globals)] const NSWindowStyleMaskNonActivatingPanel: i32 = 1 << 7; panel.set_style_mask(NSWindowStyleMaskNonActivatingPanel);
763-784: Use.ok()instead oflet _ =for clarity.Similar to the earlier pattern, using
.ok()explicitly acknowledges ignoring theResultrather than silently dropping it withlet _ =.#[cfg(target_os = "macos")] - let _ = window.run_on_main_thread({ + window.run_on_main_thread({ let window = window.clone(); move || { // ... closure body ... } - }); + }).ok();
901-901: Consider removing the attribution comment.Per coding guidelines, comments should be avoided. If attribution is needed for licensing purposes, consider adding it to a NOTICES or LICENSE file instead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
Cargo.toml(2 hunks)apps/desktop/package.json(3 hunks)apps/desktop/src-tauri/Cargo.toml(2 hunks)apps/desktop/src-tauri/src/windows.rs(27 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/desktop/package.json
- Cargo.toml
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Never use dbg!() macro; use proper logging (tracing::debug!, etc.) instead
Never write let _ = async_fn() which silently drops futures; await or explicitly handle them
Use duration.saturating_sub(other) instead of duration - other to avoid panics on underflow
Merge nested if statements: write 'if a && b { }' instead of 'if a { if b { } }'
Don't call .clone() on Copy types (integers, bools, etc.); copy them directly
Use function references directly: iter.map(foo) instead of iter.map(|x| foo(x))
Accept &[T] or &str instead of &Vec or &String in function parameters for flexibility
Use .is_empty() instead of .len() == 0 or .len() > 0 / .len() != 0
Don't assign () to a variable: write foo(); instead of let _ = foo(); or let x = foo(); when return is unit
Use .unwrap_or(val) instead of .unwrap_or_else(|| val) when the default is a simple/cheap value
Use 'for item in &collection' or 'for (i, item) in collection.iter().enumerate()' instead of 'for i in 0..collection.len()'
Use value.clamp(min, max) instead of manual if chains or .min(max).max(min) patterns
Always handle Result/Option or types marked #[must_use]; never ignore them
**/*.rs: Userustfmtand workspace clippy lints for Rust code formatting and linting
Use snake_case for Rust module names and kebab-case for crate names
Never usedbg!()macro in Rust code; use proper logging instead (Clippy:dbg_macro= deny)
Always handleResult/Optionor types marked#[must_use]; never ignore them (Rust compiler lint:unused_must_use= deny)
Never writelet _ = async_fn()which silently drops futures; await or explicitly handle them (Clippy:let_underscore_future= deny)
Usesaturating_subinstead of-forDurationto avoid panics (Clippy:unchecked_duration_subtraction= deny)
Merge nestedifstatements: useif a && b { }instead ofif a { if b { } }(Clippy:collapsible_if= deny)
Don't call.clone()onCopytypes; just copy them directly (Clippy:clone_on_copy= deny)
U...
Files:
apps/desktop/src-tauri/src/windows.rs
**/*.{ts,tsx,js,jsx,rs}
📄 CodeRabbit inference engine (AGENTS.md)
Never add comments to code (
//,/* */,///,//!,#, etc.); code must be self-explanatory through naming, types, and structure
Files:
apps/desktop/src-tauri/src/windows.rs
🧠 Learnings (6)
📚 Learning: 2025-12-07T14:29:40.721Z
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T14:29:40.721Z
Learning: Applies to **/*.{ts,tsx,js,jsx,rs} : Never add comments to code (`//`, `/* */`, `///`, `//!`, `#`, etc.); code must be self-explanatory through naming, types, and structure
Applied to files:
apps/desktop/src-tauri/Cargo.toml
📚 Learning: 2025-12-07T14:29:19.166Z
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-07T14:29:19.166Z
Learning: Regenerate auto-generated Tauri bindings by restarting the dev server when Rust types change
Applied to files:
apps/desktop/src-tauri/Cargo.toml
📚 Learning: 2025-12-07T14:29:40.721Z
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T14:29:40.721Z
Learning: Applies to **/*.rs : Never write `let _ = async_fn()` which silently drops futures; await or explicitly handle them (Clippy: `let_underscore_future` = deny)
Applied to files:
apps/desktop/src-tauri/src/windows.rs
📚 Learning: 2025-12-07T14:29:19.165Z
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-07T14:29:19.165Z
Learning: Applies to **/*.rs : Never write let _ = async_fn() which silently drops futures; await or explicitly handle them
Applied to files:
apps/desktop/src-tauri/src/windows.rs
📚 Learning: 2025-12-07T14:29:40.721Z
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T14:29:40.721Z
Learning: Applies to **/*.rs : Always handle `Result`/`Option` or types marked `#[must_use]`; never ignore them (Rust compiler lint: `unused_must_use` = deny)
Applied to files:
apps/desktop/src-tauri/src/windows.rs
📚 Learning: 2025-12-07T14:29:40.721Z
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T14:29:40.721Z
Learning: Applies to **/*.rs : Don't assign `()` to a variable: write `foo();` instead of `let _ = foo();` when return is unit (Clippy: `let_unit_value` = deny)
Applied to files:
apps/desktop/src-tauri/src/windows.rs
⏰ 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: Clippy (aarch64-apple-darwin, macos-latest)
- GitHub Check: Clippy (x86_64-pc-windows-msvc, windows-latest)
🔇 Additional comments (4)
apps/desktop/src-tauri/src/windows.rs (3)
38-55: LGTM!The
CapWindowDefenum is well-structured with clear variant names. The use ofDisplayIdfor overlay-related variants provides type safety.
259-299: LGTM!The window ID registration logic for both
EditorandScreenshotEditorvariants correctly registers the path-to-ID mapping before callingdef(). This ensures thedef()method can find the ID when building the window label.
459-486: LGTM!The Editor and ScreenshotEditor window creation correctly closes Main and Camera windows before opening, preventing conflicting window states.
apps/desktop/src-tauri/Cargo.toml (1)
121-133: Verify objc2 workspace version compatibility with 0.3.2 crates.The objc2-* crates at version 0.3.2 require objc2 >=0.6.2 and <0.8.0. Confirm that the workspace-managed
objc2version falls within this range. Additionally, block2 0.6.2 satisfies the requirement (>=0.6.1, <0.8.0) for objc2-app-kit 0.3.2 and related crates.
| CapWindow::Editor { project_path } => { | ||
| let state = app.state::<EditorWindowIds>(); | ||
| let s = state.ids.lock().unwrap(); | ||
| let id = s.iter().find(|(path, _)| path == project_path).unwrap().1; | ||
| CapWindowId::Editor { id } | ||
| CapWindowDef::Editor { id } | ||
| } |
There was a problem hiding this comment.
Potential panic if def() is called without prior registration.
The def() method is public and uses .unwrap() when looking up Editor/ScreenshotEditor IDs. While show() registers the ID before calling def(), external callers of def() could trigger a panic if the path isn't registered.
Consider returning Option<CapWindowDef> or Result<CapWindowDef, _>, or documenting the precondition if this method should only be called after registration.
- CapWindow::Editor { project_path } => {
- let state = app.state::<EditorWindowIds>();
- let s = state.ids.lock().unwrap();
- let id = s.iter().find(|(path, _)| path == project_path).unwrap().1;
- CapWindowDef::Editor { id }
- }
+ CapWindow::Editor { project_path } => {
+ let state = app.state::<EditorWindowIds>();
+ let s = state.ids.lock().unwrap();
+ let id = s
+ .iter()
+ .find(|(path, _)| path == project_path)
+ .map(|(_, id)| *id)
+ .unwrap_or(0);
+ CapWindowDef::Editor { id }
+ }Also applies to: 851-856
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
apps/desktop/src-tauri/src/windows.rs (3)
833-838: Potential panic if path not found in registry.The
.unwrap()on line 836 can panic ifproject_pathis not found in the registry. Whileshow()registers the ID before callingdef(), this public method could be called externally without prior registration.Consider returning
Result<CapWindowDef, _>or documenting thatdef()must only be called aftershow():CapWindow::Editor { project_path } => { let state = app.state::<EditorWindowIds>(); let s = state.ids.lock().unwrap(); - let id = s.iter().find(|(path, _)| path == project_path).unwrap().1; + let id = s + .iter() + .find(|(path, _)| path == project_path) + .map(|(_, id)| *id) + .unwrap_or(0); CapWindowDef::Editor { id } }
851-856: Same potential panic for ScreenshotEditor path lookup.Similar to the Editor variant, the
.unwrap()on line 854 can panic if the path is not found in the registry.Apply the same fix as for Editor:
CapWindow::ScreenshotEditor { path } => { let state = app.state::<ScreenshotEditorWindowIds>(); let s = state.ids.lock().unwrap(); - let id = s.iter().find(|(p, _)| p == path).unwrap().1; + let id = s + .iter() + .find(|(p, _)| p == path) + .map(|(_, id)| *id) + .unwrap_or(0); CapWindowDef::ScreenshotEditor { id } }
346-350: Must handle Result fromrun_on_main_thread.Using
let _ = window.run_on_main_thread(...)violates coding guidelines that require handlingResult/#[must_use]types. The underscore assignment silently ignores the Result.Apply this diff to properly handle the Result:
#[cfg(target_os = "macos")] { if new_recording_flow { - let _ = window.run_on_main_thread({ + window.run_on_main_thread({ let window = window.clone(); move || window.objc2_nswindow().setLevel(50) - }); + }).ok(); }Based on coding guidelines: "Always handle
Result/Optionor types marked#[must_use]" and "Don't assign()to a variable".
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/desktop/src-tauri/src/windows.rs(27 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Never use dbg!() macro; use proper logging (tracing::debug!, etc.) instead
Never write let _ = async_fn() which silently drops futures; await or explicitly handle them
Use duration.saturating_sub(other) instead of duration - other to avoid panics on underflow
Merge nested if statements: write 'if a && b { }' instead of 'if a { if b { } }'
Don't call .clone() on Copy types (integers, bools, etc.); copy them directly
Use function references directly: iter.map(foo) instead of iter.map(|x| foo(x))
Accept &[T] or &str instead of &Vec or &String in function parameters for flexibility
Use .is_empty() instead of .len() == 0 or .len() > 0 / .len() != 0
Don't assign () to a variable: write foo(); instead of let _ = foo(); or let x = foo(); when return is unit
Use .unwrap_or(val) instead of .unwrap_or_else(|| val) when the default is a simple/cheap value
Use 'for item in &collection' or 'for (i, item) in collection.iter().enumerate()' instead of 'for i in 0..collection.len()'
Use value.clamp(min, max) instead of manual if chains or .min(max).max(min) patterns
Always handle Result/Option or types marked #[must_use]; never ignore them
**/*.rs: Userustfmtand workspace clippy lints for Rust code formatting and linting
Use snake_case for Rust module names and kebab-case for crate names
Never usedbg!()macro in Rust code; use proper logging instead (Clippy:dbg_macro= deny)
Always handleResult/Optionor types marked#[must_use]; never ignore them (Rust compiler lint:unused_must_use= deny)
Never writelet _ = async_fn()which silently drops futures; await or explicitly handle them (Clippy:let_underscore_future= deny)
Usesaturating_subinstead of-forDurationto avoid panics (Clippy:unchecked_duration_subtraction= deny)
Merge nestedifstatements: useif a && b { }instead ofif a { if b { } }(Clippy:collapsible_if= deny)
Don't call.clone()onCopytypes; just copy them directly (Clippy:clone_on_copy= deny)
U...
Files:
apps/desktop/src-tauri/src/windows.rs
**/*.{ts,tsx,js,jsx,rs}
📄 CodeRabbit inference engine (AGENTS.md)
Never add comments to code (
//,/* */,///,//!,#, etc.); code must be self-explanatory through naming, types, and structure
Files:
apps/desktop/src-tauri/src/windows.rs
🧠 Learnings (4)
📚 Learning: 2025-12-07T14:29:40.721Z
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T14:29:40.721Z
Learning: Applies to **/*.rs : Never write `let _ = async_fn()` which silently drops futures; await or explicitly handle them (Clippy: `let_underscore_future` = deny)
Applied to files:
apps/desktop/src-tauri/src/windows.rs
📚 Learning: 2025-12-07T14:29:19.165Z
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-07T14:29:19.165Z
Learning: Applies to **/*.rs : Never write let _ = async_fn() which silently drops futures; await or explicitly handle them
Applied to files:
apps/desktop/src-tauri/src/windows.rs
📚 Learning: 2025-12-07T14:29:40.721Z
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T14:29:40.721Z
Learning: Applies to **/*.rs : Always handle `Result`/`Option` or types marked `#[must_use]`; never ignore them (Rust compiler lint: `unused_must_use` = deny)
Applied to files:
apps/desktop/src-tauri/src/windows.rs
📚 Learning: 2025-12-07T14:29:40.721Z
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-07T14:29:40.721Z
Learning: Applies to **/*.rs : Don't assign `()` to a variable: write `foo();` instead of `let _ = foo();` when return is unit (Clippy: `let_unit_value` = deny)
Applied to files:
apps/desktop/src-tauri/src/windows.rs
⏰ 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). (1)
- GitHub Check: Clippy (x86_64-pc-windows-msvc, windows-latest)
| let _ = window.run_on_main_thread({ | ||
| let window = window.clone(); | ||
| move || { | ||
| if def.disables_window_buttons() { | ||
| window.set_traffic_lights_visible(false); | ||
| } | ||
|
|
||
| let nswindow = window.objc2_nswindow(); | ||
|
|
||
| if def.disables_fullscreen() { | ||
| nswindow.setCollectionBehavior( | ||
| nswindow.collectionBehavior() | ||
| | objc2_app_kit::NSWindowCollectionBehavior::FullScreenNone, | ||
| ); | ||
| } | ||
|
|
||
| if let Some(level) = def.window_level() { | ||
| nswindow.setLevel(level) | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
Must handle Result from run_on_main_thread.
Using let _ = window.run_on_main_thread(...) violates coding guidelines requiring proper handling of #[must_use] types.
Apply this diff:
#[cfg(target_os = "macos")]
- let _ = window.run_on_main_thread({
+ window.run_on_main_thread({
let window = window.clone();
move || {
if def.disables_window_buttons() {
window.set_traffic_lights_visible(false);
}
let nswindow = window.objc2_nswindow();
if def.disables_fullscreen() {
nswindow.setCollectionBehavior(
nswindow.collectionBehavior()
| objc2_app_kit::NSWindowCollectionBehavior::FullScreenNone,
);
}
if let Some(level) = def.window_level() {
nswindow.setLevel(level)
}
}
- });
+ }).ok();Based on coding guidelines: "Always handle Result/Option or types marked #[must_use]".
🤖 Prompt for AI Agents
In apps/desktop/src-tauri/src/windows.rs around lines 764 to 784, the call "let
_ = window.run_on_main_thread(...)" ignores a #[must_use] Result; replace the
discard with explicit handling: capture the Result, and on Err branch either
propagate the error (return/?) or log it and continue. Concretely, assign the
call to a variable and then use match or if let Err(e) = result to call an
appropriate logger (e.g., error!("run_on_main_thread failed: {:?}", e)) or
return the error from the enclosing function if that fits the function's
signature; ensure no #[must_use] value is silently dropped.
2 participants
Note
This is part of a preparation for bigger changes that are work-in-progress. These are smaller, but more frequent and should be done as early as possible to avoid future conflicts and unnecessary work.
This PR includes a small refactor in windows.rs and bumps some crate versions.
CapWindowId->CapWindowDefandShowCapWindow->CapWindow.We can now give the traffic light position to tauri instead of somewhat subclassing Tauri's NSWindow delegate. This is still not ideal for macOS 26+, that will be fixed with a later PR.
permissions.rs now uses objc2_application_services instead. This crate will be useful in the near future for determining the possible shape of a window.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.
Note
Refactors desktop windowing (CapWindowDef/CapWindow), switches macOS window/permissions to objc2, and upgrades Tauri/objc2 and plugins; also tweaks editor Zoom track and bundling metadata.
CapWindowId→CapWindowDefandShowCapWindow→CapWindow; centralize window building/levels/buttons/fullscreen inwindows.rs.traffic_light_positionand newWebviewWindowExtto accessNSWindowand toggle traffic lights.objc2_application_services(AXIsProcessTrusted(WithOptions)), update request flow.ZoomTrack.tsxsegment rendering/selection logic.tauri.conf.json.taurito2.9.3and multipletauri-*plugins; bumpobjc2*,block2,wry,tao,tauri-runtime(-wry), andserde; set@tauri-apps/clito^2.9.4.objc2-application-servicesand relatedobjc2-*crates; update lockfile accordingly.Written by Cursor Bugbot for commit a598262. This will update automatically on new commits. Configure here.