Refactors in windows.rs, Bump tauri and tauri plugin and objc2-* crate versions #1399
Conversation
|
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. WalkthroughRenames window types (ShowCapWindow → CapWindow, CapWindowId → CapWindowDef), upgrades Tauri/CLI and plugins, migrates macOS FFI to objc2-based crates and adds a WebviewWindowExt trait while removing legacy delegates.rs, extends recording/timeline/display/caption types, and refactors frontend zoom-segment rendering and screenshot editor styles. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Frontend
participant TauriCmd as Tauri backend
participant Windows as windows::CapWindow / CapWindowDef
participant Webview as tauri::WebviewWindow
participant macOS as objc2 / NSWindow
Frontend->>TauriCmd: invoke showWindow(CapWindow::Main|Settings|Editor|...)
TauriCmd->>Windows: resolve CapWindowDef and build/show window
Windows->>Webview: obtain WebviewWindow handle
Webview->>macOS: WebviewWindowExt::objc2_nswindow()
Webview->>macOS: set_traffic_lights_visible(visible)
Note right of macOS: NSWindow / CF / AX calls performed via objc2 crates (AXIsProcessTrustedWithOptions etc.)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
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().
| tracing-opentelemetry = "0.32.0" | ||
| opentelemetry = "0.31.0" | ||
| opentelemetry-otlp = "0.31.0" #{ version = , features = ["http-proto", "reqwest-client"] } | ||
| opentelemetry-otlp = "0.31.0" #{ version = , features = ["http-proto", "reqwest-client"] } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove the trailing comment.
As per coding guidelines, comments should not be added to code. The trailing comment #{ version = , features = ["http-proto", "reqwest-client"] } should be removed.
-opentelemetry-otlp = "0.31.0" #{ version = , features = ["http-proto", "reqwest-client"] }
+opentelemetry-otlp = "0.31.0"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| opentelemetry-otlp = "0.31.0" #{ version = , features = ["http-proto", "reqwest-client"] } | |
| opentelemetry-otlp = "0.31.0" |
🤖 Prompt for AI Agents
In apps/desktop/src-tauri/Cargo.toml around line 112, remove the trailing inline
comment "#{ version = , features = ["http-proto", "reqwest-client"] }" so the
dependency line reads only the valid TOML entry (opentelemetry-otlp = "0.31.0")
with no trailing comments; ensure no leftover characters remain and the file
still parses as valid TOML.
| 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.
| #[cfg(windows)] | ||
| { | ||
| window_builder = window_builder.inner_size(100.0, 100.0).position(0.0, 0.0); | ||
| builder = window_builder.inner_size(100.0, 100.0).position(0.0, 0.0); | ||
| } |
There was a problem hiding this comment.
Critical bug: undefined variable window_builder.
On line 382, window_builder is referenced but the variable is named builder (defined on line 356). 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);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[cfg(windows)] | |
| { | |
| window_builder = window_builder.inner_size(100.0, 100.0).position(0.0, 0.0); | |
| 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); | |
| } |
🤖 Prompt for AI Agents
In apps/desktop/src-tauri/src/windows.rs around lines 380 to 383, the
conditional block references an undefined variable `window_builder` but the
actual variable is named `builder` (declared earlier). Replace the
`window_builder` reference with `builder` inside the #[cfg(windows)] block so
the call becomes builder = builder.inner_size(100.0, 100.0).position(0.0, 0.0);
ensuring the existing builder variable is used and the code compiles.
| 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.
| PastFrames: 0, | ||
| FutureFrames: 0, | ||
| ppPastSurfaces: ptr::null_mut(), | ||
| pInputSurface: std::mem::transmute_copy(&input_view), |
There was a problem hiding this comment.
Use of transmute_copy is fragile and potentially unsafe.
std::mem::transmute_copy(&input_view) is used to convert the ID3D11VideoProcessorInputView to Option<ID3D11VideoProcessorInputView>. This relies on internal COM object representation details.
Consider using safer alternatives. The windows crate typically provides conversion methods. If pInputSurface expects an Option, you might be able to use Some(input_view) directly or check the field's expected type in the Windows API bindings.
let stream = D3D11_VIDEO_PROCESSOR_STREAM {
Enable: true.into(),
OutputIndex: 0,
InputFrameOrField: 0,
PastFrames: 0,
FutureFrames: 0,
ppPastSurfaces: ptr::null_mut(),
- pInputSurface: std::mem::transmute_copy(&input_view),
+ pInputSurface: Some(input_view.clone()),
ppFutureSurfaces: ptr::null_mut(),
ppPastSurfacesRight: ptr::null_mut(),
pInputSurfaceRight: None,
ppFutureSurfacesRight: ptr::null_mut(),
};Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
crates/frame-converter/src/d3d11.rs around line 405: the code uses
std::mem::transmute_copy(&input_view) to populate pInputSurface which is fragile
and relies on internal COM layout; replace this transmute with the correct, safe
conversion for the windows crate binding (e.g., provide an
Option<ID3D11VideoProcessorInputView> directly such as Some(input_view) or use
the binding's IntoParam/into() method or an explicit clone/raw pointer accessor
as required by the generated signature); inspect the generated type for
pInputSurface in the Windows bindings and use the supported conversion
(Some/None or IntoParam) to pass the COM object safely instead of
transmute_copy.
| unsafe fn copy_mapped_to_frame(src: *const u8, src_stride: usize, frame: &mut frame::Video) { | ||
| let height = frame.height() as usize; | ||
| let format = frame.format(); | ||
|
|
||
| match format { | ||
| Pixel::NV12 => { | ||
| for y in 0..height { | ||
| ptr::copy_nonoverlapping( | ||
| src.add(y * src_stride), | ||
| frame.data_mut(0).as_mut_ptr().add(y * frame.stride(0)), | ||
| frame.width() as usize, | ||
| ); | ||
| } | ||
| let uv_offset = height * src_stride; | ||
| for y in 0..height / 2 { | ||
| ptr::copy_nonoverlapping( | ||
| src.add(uv_offset + y * src_stride), | ||
| frame.data_mut(1).as_mut_ptr().add(y * frame.stride(1)), | ||
| frame.width() as usize, | ||
| ); | ||
| } | ||
| } | ||
| _ => {} | ||
| } | ||
| } |
There was a problem hiding this comment.
Asymmetric format handling between copy_frame_to_mapped and copy_mapped_to_frame.
copy_frame_to_mapped handles Pixel::NV12, Pixel::YUYV422, and Pixel::UYVY422, but copy_mapped_to_frame only handles Pixel::NV12. If the output format is YUYV422, the output frame will not be properly populated.
Add YUYV422/UYVY422 handling to copy_mapped_to_frame:
unsafe fn copy_mapped_to_frame(src: *const u8, src_stride: usize, frame: &mut frame::Video) {
let height = frame.height() as usize;
let format = frame.format();
match format {
Pixel::NV12 => {
for y in 0..height {
ptr::copy_nonoverlapping(
src.add(y * src_stride),
frame.data_mut(0).as_mut_ptr().add(y * frame.stride(0)),
frame.width() as usize,
);
}
let uv_offset = height * src_stride;
for y in 0..height / 2 {
ptr::copy_nonoverlapping(
src.add(uv_offset + y * src_stride),
frame.data_mut(1).as_mut_ptr().add(y * frame.stride(1)),
frame.width() as usize,
);
}
}
+ Pixel::YUYV422 | Pixel::UYVY422 => {
+ let row_bytes = frame.width() as usize * 2;
+ for y in 0..height {
+ ptr::copy_nonoverlapping(
+ src.add(y * src_stride),
+ frame.data_mut(0).as_mut_ptr().add(y * frame.stride(0)),
+ row_bytes,
+ );
+ }
+ }
_ => {}
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| unsafe fn copy_mapped_to_frame(src: *const u8, src_stride: usize, frame: &mut frame::Video) { | |
| let height = frame.height() as usize; | |
| let format = frame.format(); | |
| match format { | |
| Pixel::NV12 => { | |
| for y in 0..height { | |
| ptr::copy_nonoverlapping( | |
| src.add(y * src_stride), | |
| frame.data_mut(0).as_mut_ptr().add(y * frame.stride(0)), | |
| frame.width() as usize, | |
| ); | |
| } | |
| let uv_offset = height * src_stride; | |
| for y in 0..height / 2 { | |
| ptr::copy_nonoverlapping( | |
| src.add(uv_offset + y * src_stride), | |
| frame.data_mut(1).as_mut_ptr().add(y * frame.stride(1)), | |
| frame.width() as usize, | |
| ); | |
| } | |
| } | |
| _ => {} | |
| } | |
| } | |
| unsafe fn copy_mapped_to_frame(src: *const u8, src_stride: usize, frame: &mut frame::Video) { | |
| let height = frame.height() as usize; | |
| let format = frame.format(); | |
| match format { | |
| Pixel::NV12 => { | |
| for y in 0..height { | |
| ptr::copy_nonoverlapping( | |
| src.add(y * src_stride), | |
| frame.data_mut(0).as_mut_ptr().add(y * frame.stride(0)), | |
| frame.width() as usize, | |
| ); | |
| } | |
| let uv_offset = height * src_stride; | |
| for y in 0..height / 2 { | |
| ptr::copy_nonoverlapping( | |
| src.add(uv_offset + y * src_stride), | |
| frame.data_mut(1).as_mut_ptr().add(y * frame.stride(1)), | |
| frame.width() as usize, | |
| ); | |
| } | |
| } | |
| Pixel::YUYV422 | Pixel::UYVY422 => { | |
| let row_bytes = frame.width() as usize * 2; | |
| for y in 0..height { | |
| ptr::copy_nonoverlapping( | |
| src.add(y * src_stride), | |
| frame.data_mut(0).as_mut_ptr().add(y * frame.stride(0)), | |
| row_bytes, | |
| ); | |
| } | |
| } | |
| _ => {} | |
| } | |
| } |
🤖 Prompt for AI Agents
In crates/frame-converter/src/d3d11.rs around lines 547 to 571,
copy_mapped_to_frame only handles Pixel::NV12 causing YUYV422/UYVY422 outputs to
be left unpopulated; add match arms for Pixel::YUYV422 and Pixel::UYVY422 that
mirror the logic in copy_frame_to_mapped: for each scanline copy the packed
pixel bytes from src to the frame's plane 0 using ptr::copy_nonoverlapping with
the correct per-row source offset (y * src_stride) and destination offset (y *
frame.stride(0)), copying frame.width() as usize * 2 bytes (2 bytes per pixel)
and ensure you use the frame.height()/stride values exactly as with NV12 so
packed formats are fully copied.
| 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.
| fn has_open_editor_window(app: &AppHandle) -> bool { | ||
| app.webview_windows() | ||
| .keys() | ||
| .any(|label| matches!(CapWindowId::from_str(label), Ok(CapWindowId::Editor { .. }))) | ||
| .any(|label| matches!(CapWindow::from_str(label), Ok(CapWindow::Editor { .. }))) | ||
| } |
There was a problem hiding this comment.
Critical bug: CapWindow::from_str does not exist.
CapWindow does not implement FromStr. The FromStr trait is implemented on CapWindowDef, not CapWindow. This will cause a compilation error.
Apply this diff to fix:
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 { .. })))
}🤖 Prompt for AI Agents
In apps/desktop/src-tauri/src/lib.rs around lines 2947 to 2951, the code calls
CapWindow::from_str which doesn't exist because the FromStr impl is on
CapWindowDef; replace the call to use CapWindowDef::from_str and match against
the CapWindowDef::Editor variant (i.e. call CapWindowDef::from_str(label) and
check for Ok(CapWindowDef::Editor { .. })) so the code compiles and correctly
detects editor windows.
| 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.
| impl ShowCapWindow { | ||
| impl CapWindow { | ||
| pub async fn show(&self, app: &AppHandle<Wry>) -> tauri::Result<WebviewWindow> { | ||
| use std::fmt::Write; |
There was a problem hiding this comment.
Remove unused import.
The use std::fmt::Write; import doesn't appear to be used in the method body. No write formatting operations are present that would require this trait.
Apply this diff:
- use std::fmt::Write;
-📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| use std::fmt::Write; |
🤖 Prompt for AI Agents
In apps/desktop/src-tauri/src/windows.rs around line 262, the import "use
std::fmt::Write;" is unused; remove that import line to eliminate the
dead/unused dependency and keep the file imports tidy (ensure no other code
relies on the Write trait before deleting).
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.
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
UI
Data
✏️ 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.