Overhaul camera preview #945
Conversation
WalkthroughReplaces procedural camera preview and feed code with an actor-based CameraFeed and a CameraPreviewManager that owns per-window renderer lifecycle; updates app wiring, recording inputs, camera crate capture handle semantics, TAURI frontend types, and multiple call-sites to use the new APIs. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as Camera Window (Webview)
participant App as Tauri App
participant CPM as CameraPreviewManager
participant Feed as CameraFeed Actor
participant Rend as Renderer (wgpu task)
UI->>App: open camera window
App->>CPM: init_window(window, actor) (async)
CPM->>Rend: spawn/init_wgpu (thread/task)
Feed-->>CPM: New RawCameraFrame (via AddSender -> channel)
CPM->>Rend: send frame / ReconfigureEvent::State
Rend-->>CPM: render / ack
App-->>CPM: on_window_resize(width,height)
CPM->>Rend: ReconfigureEvent::State (size)
App-->>CPM: on_window_close()
CPM->>Rend: ReconfigureEvent::Shutdown
Rend-->>CPM: cleanup complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Assessment against linked issues
Out-of-scope changes
Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (9)
crates/camera/src/macos.rs (1)
71-71: Add deprecated back-compat alias forNativeRecordingHandleWe’ve re-searched the entire codebase (including ignored files) and found no remaining references to
NativeRecordingHandle. To prevent downstream breakage, introduce a deprecated alias pointing to the new name.• File:
crates/camera/src/macos.rs
• Location: immediately after the existingNativeCaptureHandletypedefProposed patch:
pub type NativeCaptureHandle = AVFoundationRecordingHandle; + +// Transitional alias to avoid breakage in downstreams that still use the old name. +#[deprecated(note = "Use `NativeCaptureHandle` instead of `NativeRecordingHandle`.")] +pub type NativeRecordingHandle = NativeCaptureHandle;apps/desktop/src-tauri/src/recording.rs (1)
652-659: Duplicate camera window close; collapse to a single close after tearing down the feed.You close the Camera window twice: once on Line 652 and again on Lines 657–659. Keep a single close after
app.camera_feed.take()to avoid redundant calls and align with the intended shutdown ordering.Apply:
- if let Some(v) = CapWindowId::Camera.get(&handle) { - let _ = v.close(); - } let _ = app.mic_feed.ask(microphone::RemoveInput).await; app.camera_feed.take(); if let Some(win) = CapWindowId::Camera.get(&handle) { win.close().ok(); }crates/recording/src/feeds/camera.rs (1)
228-236: (If keeping explicit stop) Prefer Drop over explicit stop or convert handle toOptionandtake().If you want eager stop before breaking the loop, make
handleanOption<CaptureHandle>andtake()it on shutdown. This avoids relying on the timing of outer-scope drop.Example (illustrative, not a full diff):
struct SetupCameraState { handle: Option<cap_camera::CaptureHandle>, // ... } // on setup: let capture_handle = camera.start_capturing(/*...*/)?; let mut state = SetupCameraState { handle: Some(capture_handle), /*...*/ }; // on shutdown: if let Some(handle) = state.handle.take() { drop(handle); // triggers Drop immediately }apps/desktop/src-tauri/src/windows.rs (1)
363-367: Redundantunwrap_or_default()on an already defaulted valueThe
and_thenchain withunwrap_or_default()already provides a default value, making the outerunwrap_or_default()redundant.- let enable_native_camera_preview = GeneralSettingsStore::get(&app) - .ok() - .and_then(|v| v.map(|v| v.enable_native_camera_preview)) - .unwrap_or_default(); + let enable_native_camera_preview = GeneralSettingsStore::get(&app) + .ok() + .flatten() + .map(|v| v.enable_native_camera_preview) + .unwrap_or_default();crates/camera/src/lib.rs (2)
224-226: ImplementDebugforCaptureHandleThe
CaptureHandlestruct would benefit from aDebugimplementation for better diagnostics and logging, especially since it manages an optional native handle.+#[derive(Debug)] #[must_use = "must be held for the duration of the recording"] pub struct CaptureHandle { native: Option<NativeCaptureHandle>, }
237-242: Silent error handling inDropimplementationThe
Dropimplementation silently discards errors fromstop_capturing(). While this is typical for drop implementations, consider logging these errors for debugging purposes.impl Drop for CaptureHandle { fn drop(&mut self) { if let Some(feed) = self.native.take() { - feed.stop_capturing().ok(); + if let Err(e) = feed.stop_capturing() { + eprintln!("Failed to stop camera capture during drop: {}", e); + } } } }apps/desktop/src-tauri/src/camera.rs (3)
104-105: Use ofwhile let Ok(_)for draining channelThe pattern
while let Ok(_) = self.channel.1.try_recv() {}works but could be more idiomatic.- // Drain the channel so when the preview is opened it doesn't show an old frame. - while let Ok(_) = self.channel.1.try_recv() {} + // Drain the channel so when the preview is opened it doesn't show an old frame. + while self.channel.1.try_recv().is_ok() {}
602-611: GPU surface dimensions multiplied by scale factorThe GPU surface dimensions are multiplied by
GPU_SURFACE_SCALE(4) to improve anti-aliasing on curved edges. While this provides better visual quality, it increases memory usage by 16x. Consider making this configurable or adaptive based on system capabilities.+ // Scale factor for GPU surface resolution (higher = better quality but more memory) + // Consider making this configurable based on system capabilities self.surface_config.width = if window_width > 0 { window_width * GPU_SURFACE_SCALE } else { 1 };
432-433: Hardcoded aspect ratio calculationThe aspect ratio calculation uses a hardcoded value
1.7777778(approximately 16:9). Consider using a named constant for clarity.+ const DEFAULT_ASPECT_RATIO: f32 = 16.0 / 9.0; let output_width = 5; - let output_height = (5.0 * 1.7777778) as u32; // TODO + let output_height = (5.0 / DEFAULT_ASPECT_RATIO) as u32;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (10)
apps/desktop/src-tauri/src/camera.rs(9 hunks)apps/desktop/src-tauri/src/deeplink_actions.rs(2 hunks)apps/desktop/src-tauri/src/lib.rs(12 hunks)apps/desktop/src-tauri/src/recording.rs(1 hunks)apps/desktop/src-tauri/src/windows.rs(3 hunks)apps/desktop/src/routes/camera.tsx(0 hunks)crates/camera/src/lib.rs(1 hunks)crates/camera/src/macos.rs(1 hunks)crates/camera/src/windows.rs(1 hunks)crates/recording/src/feeds/camera.rs(1 hunks)
💤 Files with no reviewable changes (1)
- apps/desktop/src/routes/camera.tsx
🧰 Additional context used
📓 Path-based instructions (1)
apps/desktop/src-tauri/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Use tauri_specta on the Rust side for IPC (typed commands/events) and emit events via generated types
Files:
apps/desktop/src-tauri/src/recording.rsapps/desktop/src-tauri/src/deeplink_actions.rsapps/desktop/src-tauri/src/windows.rsapps/desktop/src-tauri/src/lib.rsapps/desktop/src-tauri/src/camera.rs
🧬 Code graph analysis (5)
apps/desktop/src-tauri/src/deeplink_actions.rs (1)
apps/desktop/src-tauri/src/lib.rs (5)
set_camera_input(247-331)app(1182-1183)app(2195-2195)app(2220-2220)app(2226-2226)
crates/camera/src/lib.rs (4)
crates/camera/src/macos.rs (2)
start_capturing_impl(87-147)stop_capturing(158-161)crates/camera/src/windows.rs (2)
start_capturing_impl(66-83)stop_capturing(90-92)crates/camera-windows/src/lib.rs (3)
new(457-475)new(501-521)stop_capturing(30-35)crates/camera-mediafoundation/src/lib.rs (2)
new(40-65)stop_capturing(374-376)
apps/desktop/src-tauri/src/windows.rs (2)
apps/desktop/src/utils/tauri.ts (2)
GeneralSettingsStore(536-558)Camera(410-420)apps/desktop/src-tauri/src/general_settings.rs (1)
get(177-188)
apps/desktop/src-tauri/src/lib.rs (1)
apps/desktop/src-tauri/src/camera.rs (2)
attach(102-107)new(64-72)
apps/desktop/src-tauri/src/camera.rs (1)
apps/desktop/src-tauri/src/lib.rs (8)
new(394-396)new(821-825)new(1452-1460)app(1182-1183)app(2195-2195)app(2220-2220)app(2226-2226)run(1874-2318)
🔇 Additional comments (8)
crates/camera/src/windows.rs (1)
64-65: Approve deprecated alias addition – no references toNativeRecordingHandlefoundRan the proposed sanity check across the entire repository (
rg -n '\bNativeRecordingHandle\b' --hidden) and confirmed there are zero occurrences. The new deprecated alias will not collide with existing code and is safe to introduce for downstream compatibility.crates/recording/src/feeds/camera.rs (1)
301-306: No compilation issue:stop_capturingstill exists onCaptureHandleI checked the implementation in
crates/camera/src/lib.rsand confirmed:
CaptureHandledefines apub fn stop_capturing(self) -> Result<(), String>method (lines 229–233).- There is also a
Dropimpl that calls.stop_capturing().ok()on drop (lines 237–242).This means calls like
state.handle.stop_capturing()will compile just fine. The original concern—that the method was removed—turns out to be incorrect. If you still want explicit shutdown semantics, keeping the call is valid. If you’d prefer to rely purely on RAII (and ignore any stop errors), you can remove the manual call, but that’s purely an optional refactor rather than a necessary fix.Likely an incorrect or invalid review comment.
apps/desktop/src-tauri/src/deeplink_actions.rs (2)
7-7: Import update looks correct for the new flow.Removing any CameraPreview references from imports and relying on the centralized manager is consistent with the PR direction.
117-117: Verified invocation ofset_camera_inputmatches updated APIThe call at
apps/desktop/src-tauri/src/deeplink_actions.rs:117correctly usescrate::set_camera_input(app.clone(), state.clone(), camera).await?;which aligns with the new signature in
lib.rs(AppHandle,MutableState<'_, App>,id). No discrepancies found.Optional – for deeper resilience testing, ensure that deep links specifying a camera gracefully handle initialization failures and surface a user-visible error (e.g., via a Tauri dialog).
apps/desktop/src-tauri/src/windows.rs (1)
418-430: macOS-specific collection behavior modificationThe code uses
unsafeto directly modify NSWindow collection behavior. This works correctly, but consider documenting whyFullScreenAuxiliarybehavior is needed for the camera window specifically.apps/desktop/src-tauri/src/lib.rs (1)
2223-2232: Async task spawned for camera window close eventThe camera window close handler spawns an async task to write to the app state. This is correct for avoiding blocking the event loop, but ensure proper error handling if the write lock acquisition fails.
apps/desktop/src-tauri/src/camera.rs (2)
126-129: Thread spawning with LocalSet for async runtimeThe code spawns a new thread with a dedicated runtime and LocalSet for the renderer. This is a valid approach for isolating GPU operations, but consider documenting why this isolation is necessary.
869-871: Good addition ofget_latest_keymethodThe addition of
get_latest_keymethod to theCachedstruct is a clean way to expose the current key without exposing the entire internal state.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/cursor-capture/src/position.rs (1)
208-355: Unit tests in this file are stale and won’t compile against the current API.
Tests referencecrop_position,crop_size,with_crop(position, size), and getters that no longer exist;NormalizedCursorPositionnow has a singlecrop: CursorCropBoundsandwith_crop(crop). They also assume a real display (Display::list()[0]), which will be flaky in CI.Recommended next steps:
- Update tests to construct
CursorCropBoundsand callwith_crop(crop). BecauseCursorCropBoundsfields are private and platform-specific ctors exist (new_macos,new_windows), either:
- Add a
#[cfg(test)]helper ctor in this module (e.g.,CursorCropBounds::from_xywh(x, y, w, h)) for test-only use, or- Gate tests per-OS and use the platform ctor.
- Avoid requiring a real display in unit tests. For pure math checks of
with_crop, prefer a test-only constructor forNormalizedCursorPositionthat takes a dummyDisplayor add a#[cfg(test)]path that doesn’t depend on hardware.Would you like me to push a patch that:
- Adds
#[cfg(test)] fn CursorCropBounds::from_xywh(...) -> Selfand- Rewrites these tests to the new API and to avoid real-display dependency?
🧹 Nitpick comments (18)
crates/scap-ffmpeg/src/cpal.rs (5)
12-14: Add a guard to ensure integral frame sizing.A malformed buffer would make
sample_counttruncate and later panics on copy. Add a cheap debug guard to catch this early.let sample_size = self.sample_format().sample_size(); - let sample_count = self.bytes().len() / (sample_size * config.channels as usize); + debug_assert_eq!( + self.bytes().len() % (sample_size * config.channels as usize), + 0, + "cpal data length must be divisible by (sample_size * channels)" + ); + let sample_count = self.bytes().len() / (sample_size * config.channels as usize);
28-36: Strengthen the planar copy with size assertion to avoid panic-by-surprise.
copy_from_slicewill panic if lengths differ. Assert expected sizes before copying to make the failure mode clearer.- ffmpeg_frame - .plane_data_mut(i as usize) - .copy_from_slice(&self.bytes()[base..base + plane_size]); + let dst = ffmpeg_frame.plane_data_mut(i as usize); + debug_assert_eq!( + dst.len(), + plane_size, + "unexpected plane size: dst={}, expected={}", + dst.len(), + plane_size + ); + dst.copy_from_slice(&self.bytes()[base..base + plane_size]);
37-39: Do the same for packed copy.Mirror the safety check for the packed path.
- ffmpeg_frame.data_mut(0).copy_from_slice(self.bytes()); + let dst = ffmpeg_frame.data_mut(0); + debug_assert_eq!( + dst.len(), + self.bytes().len(), + "unexpected packed buffer size: dst={}, src={}", + dst.len(), + self.bytes().len() + ); + dst.copy_from_slice(self.bytes());
10-11: Planar branch is currently unreachable; consider removing or makingformat_typmeaningful.
format_typis hardcoded toPacked, so the planar block never executes and may trip clippy (constant condition). If CPAL always provides interleaved PCM (usual), either:
- Remove the planar branch to reduce noise; or
- Compute
format_typfrom an input that can actually be planar (future-proofing), then keep both branches.Given the PR TODO includes “Clippy + error handling,” trimming unreachable code would help.
- if matches!(format_typ, sample::Type::Planar) { - for i in 0..config.channels { - let plane_size = sample_count * sample_size; - let base = (i as usize) * plane_size; - - ffmpeg_frame - .plane_data_mut(i as usize) - .copy_from_slice(&self.bytes()[base..base + plane_size]); - } - } else { - ffmpeg_frame.data_mut(0).copy_from_slice(self.bytes()); - } + ffmpeg_frame.data_mut(0).copy_from_slice(self.bytes());Also applies to: 28-37
16-23: Prefer a richer error over panic for unsupported sample formats.If this can be hit via external input, consider propagating a
Resultinstead of panicking. If it’s truly impossible, include the format in the message for faster diagnosis.- _ => panic!("Unsupported sample format"), + _ => panic!("Unsupported sample format: {:?}", self.sample_format()),crates/cpal-ffmpeg/src/lib.rs (5)
12-14: Guard against non-integral buffer sizes.Prevent silent truncation on
sample_countand later copy panics with a debug assertion.let sample_size = self.sample_format().sample_size(); - let sample_count = self.bytes().len() / (sample_size * config.channels as usize); + debug_assert_eq!( + self.bytes().len() % (sample_size * config.channels as usize), + 0, + "cpal data length must be divisible by (sample_size * channels)" + ); + let sample_count = self.bytes().len() / (sample_size * config.channels as usize);
28-36: Assert plane size before copy in planar path.Make the failure mode explicit if something upstream changes the buffer sizing.
- ffmpeg_frame - .plane_data_mut(i as usize) - .copy_from_slice(&self.bytes()[base..base + plane_size]); + let dst = ffmpeg_frame.plane_data_mut(i as usize); + debug_assert_eq!( + dst.len(), + plane_size, + "unexpected plane size: dst={}, expected={}", + dst.len(), + plane_size + ); + dst.copy_from_slice(&self.bytes()[base..base + plane_size]);
37-39: Mirror size check for packed data path.- ffmpeg_frame.data_mut(0).copy_from_slice(self.bytes()); + let dst = ffmpeg_frame.data_mut(0); + debug_assert_eq!( + dst.len(), + self.bytes().len(), + "unexpected packed buffer size: dst={}, src={}", + dst.len(), + self.bytes().len() + ); + dst.copy_from_slice(self.bytes());
10-11: Unreachable planar branch due to constantformat_typ = Packed.Same as the other file: either remove the branch for clarity or make
format_typreflect an actual planar input when/if that can occur.- if matches!(format_typ, sample::Type::Planar) { - for i in 0..config.channels { - let plane_size = sample_count * sample_size; - let base = (i as usize) * plane_size; - - ffmpeg_frame - .plane_data_mut(i as usize) - .copy_from_slice(&self.bytes()[base..base + plane_size]); - } - } else { - ffmpeg_frame.data_mut(0).copy_from_slice(self.bytes()); - } + ffmpeg_frame.data_mut(0).copy_from_slice(self.bytes());Also applies to: 28-37
16-23: Improve panic context or return aResult.Echoing the other file: include the actual format in the panic, or consider returning an error if this path is externally reachable.
- _ => panic!("Unsupported sample format"), + _ => panic!("Unsupported sample format: {:?}", self.sample_format()),crates/cursor-capture/src/position.rs (5)
41-46: Use a consistent return style across OS blocks.
Windows uses an explicitreturn, macOS uses an implicit return. Pick one style for both branches to reduce visual diff noise and future merge churn. I’d drop thereturnon Windows as well.Apply this diff inside the Windows branch:
@@ - return Some(Self { + Some(Self { x: raw.x - physical_bounds.position().x() as i32, y: raw.y - physical_bounds.position().y() as i32, display, - }); + })Also applies to: 52-57
36-58: No return path on unsupported targets (e.g., Linux) — add a fallback or gate the function.
If this crate is ever compiled for a target that is neither Windows nor macOS, both cfg-blocks are compiled out and the function body has no return expression, causing a compile error. Add a fallbackNoneor guard the entire impl behind a crate/feature cfg.Minimal, non-invasive fallback (returns
Noneon unsupported targets):@@ #[cfg(target_os = "macos")] { let logical_bounds = display.raw_handle().logical_bounds()?; Some(Self { x: raw.x - logical_bounds.position().x() as i32, y: raw.y - logical_bounds.position().y() as i32, display, }) } + +#[cfg(not(any(windows, target_os = "macos")))] +{ + // Unsupported target: no display coordinate system defined here. + // Avoid a compile error by returning None. + let _ = (raw, display); + None +}If Linux support is intentionally out of scope for this crate, alternatively guard the whole
implwith#[cfg(any(windows, target_os = "macos"))]and add a stub for other targets.
64-101: DRY: normalize() duplicates Windows/macOS branches that only differ in bounds accessor.
Both branches build the sameNormalizedCursorPosition; the only difference is whether you callphysical_bounds()orlogical_bounds(). A tiny helper can remove duplication.Sketch (outside selected range, for illustration):
#[cfg(windows)] fn display_size(d: &Display) -> Option<(f64, f64)> { let sz = d.raw_handle().physical_bounds()?.size(); Some((sz.width(), sz.height())) } #[cfg(target_os = "macos")] fn display_size(d: &Display) -> Option<(f64, f64)> { let sz = d.raw_handle().logical_bounds()?.size(); Some((sz.width(), sz.height())) } pub fn normalize(&self) -> Option<NormalizedCursorPosition> { let (w, h) = display_size(self.display())?; Some(NormalizedCursorPosition { x: self.x as f64 / w, y: self.y as f64 / h, crop: CursorCropBounds { x: 0.0, y: 0.0, width: w, height: h }, display: self.display, }) }
112-115: Typo: “opqaue” → “opaque”.
Tiny doc comment nit.-/// This type is opqaue on purpose as the logical/physical invariants need to hold +/// This type is opaque on purpose as the logical/physical invariants need to hold
184-196: with_crop: guard against zero/invalid crop dimensions to avoid division-by-zero.
Ifcrop.widthorcrop.heightis zero or non-finite,(raw_px - crop.xy) / crop.(w|h)will panic or yield NaN/Inf. Add a fast precondition (debug-assert plus an early return or documented behavior).Minimal, non-breaking guard:
pub fn with_crop(&self, crop: CursorCropBounds) -> Self { + debug_assert!( + crop.width() > 0.0 && crop.height() > 0.0 && crop.width().is_finite() && crop.height().is_finite(), + "crop width/height must be finite and > 0" + ); + let raw_px = ( self.x * self.crop.width + self.crop.x, self.y * self.crop.height + self.crop.y, ); Self { x: (raw_px.0 - crop.x) / crop.width, y: (raw_px.1 - crop.y) / crop.height, crop, display: self.display, } }If zero-size crops are possible, consider returning
Option<Self>instead (breaking change) or documenting clamping semantics.crates/rendering/src/layout.rs (3)
117-121: Extract the 0.05 “grace window” into a named constantThe magic number appears twice; giving it a name makes intent clear and centralizes tweaks.
Use:
@@ -pub const LAYOUT_TRANSITION_DURATION: f64 = 0.3; +pub const LAYOUT_TRANSITION_DURATION: f64 = 0.3; +pub const POST_SEGMENT_GRACE: f64 = 0.05; @@ - if cursor.time < prev_segment.end + 0.05 { + if cursor.time < prev_segment.end + POST_SEGMENT_GRACE { @@ - if cursor.time < prev_segment.end + 0.05 { + if cursor.time < prev_segment.end + POST_SEGMENT_GRACE {Also applies to: 126-130
135-139: Take LayoutMode by value in get_layout_values (it’s Copy) and simplify callsSince
LayoutModeisCopy, passing by value is clearer and removes needless referencing/dereferencing.@@ - let (start_camera_opacity, start_screen_opacity, start_camera_scale) = - Self::get_layout_values(¤t_mode); - let (end_camera_opacity, end_screen_opacity, end_camera_scale) = - Self::get_layout_values(&next_mode); + let (start_camera_opacity, start_screen_opacity, start_camera_scale) = + Self::get_layout_values(current_mode); + let (end_camera_opacity, end_screen_opacity, end_camera_scale) = + Self::get_layout_values(next_mode); @@ - fn get_layout_values(mode: &LayoutMode) -> (f64, f64, f64) { - match mode { + fn get_layout_values(mode: LayoutMode) -> (f64, f64, f64) { + match mode { LayoutMode::Default => (1.0, 1.0, 1.0), LayoutMode::CameraOnly => (1.0, 1.0, 1.0), LayoutMode::HideCamera => (0.0, 1.0, 1.0), } }Also applies to: 212-218
13-18: Add a debug-time assertion that segments are sorted by start
LayoutSegmentsCursor::next_segment()assumes ascending start times for correctness. A lightweight debug assert helps catch misordered inputs early.impl<'a> LayoutSegmentsCursor<'a> { pub fn new(time: f64, segments: &'a [LayoutSegment]) -> Self { + debug_assert!( + segments.windows(2).all(|w| w[0].start <= w[1].start), + "Layout segments must be sorted by start time" + ); match segments .iter()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
crates/cpal-ffmpeg/src/lib.rs(1 hunks)crates/cursor-capture/src/position.rs(1 hunks)crates/rendering/src/layout.rs(2 hunks)crates/scap-ffmpeg/src/cpal.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
crates/scap-ffmpeg/src/cpal.rs (2)
crates/audio/src/audio_data.rs (1)
sample_count(170-172)crates/media-info/src/lib.rs (1)
sample_size(92-94)
crates/cpal-ffmpeg/src/lib.rs (2)
crates/audio/src/audio_data.rs (1)
sample_count(170-172)crates/media-info/src/lib.rs (1)
sample_size(92-94)
crates/cursor-capture/src/position.rs (3)
crates/displays/src/platform/macos.rs (3)
logical_bounds(66-71)logical_bounds(334-356)display(461-481)crates/displays/src/lib.rs (1)
display(146-148)crates/displays/src/platform/win.rs (1)
display(1295-1302)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (6)
crates/scap-ffmpeg/src/cpal.rs (1)
30-31: Cast removal is correct; no semantic change.
sample_sizeis alreadyusize, so droppingas usizeis a safe cleanup.crates/cpal-ffmpeg/src/lib.rs (1)
30-31: Cast removal is correct; no semantic change.
sample_sizeis alreadyusize, so this is a tidy, safe cleanup.crates/cursor-capture/src/position.rs (1)
52-56: LGTM: macOS implicit return is a no-op semantic change and reads cleanly.
The switch fromreturn Some(...)to an implicitSome(...)in the macOS block doesn’t alter behavior and slightly improves readability.crates/rendering/src/layout.rs (3)
76-82: Replacing clone() with Copy moves for LayoutMode — solid cleanupGood removal of unnecessary
.clone()calls now thatLayoutModeis Copy. This reduces noise and avoids pointless allocations while preserving semantics.Also applies to: 88-96, 101-101, 108-115, 118-118, 199-202
125-133: Explicit fallback path when no current/next segment — niceThe added else-if chain ensures a complete path and avoids accidental None cases. Improves robustness when cursor is past the last segment.
1-4: Clamp progress and cache bezier easing closureTwo optimizations to reduce potential NaNs and per-frame allocations:
- Clamp the incoming
progressvalue to[0.0, 1.0]before applying the easing function to guard against floating-point overshoot or NaN at the boundaries.- Cache the easing closure returned by
bezier_easing(0.42, 0.0, 0.58, 1.0)in astaticso it isn’t re-allocated on every layout pass. Usestd::sync::OnceLockwhen your MSRV is ≥ 1.70, or fall back toonce_cell::sync::Lazyif you need to support older Rust toolchains.Please verify your MSRV (in
rust-toolchain, CI configs, or project docs) to confirm whetherstd::sync::OnceLockis available, and adjust toonce_cell::sync::Lazyif not.Proposed diff (in
crates/rendering/src/layout.rsaround your easing calls):use cap_project::{LayoutMode, LayoutSegment}; +// For Rust ≥1.70 +use std::sync::OnceLock; +// If MSRV <1.70, replace with: use once_cell::sync::Lazy; pub const LAYOUT_TRANSITION_DURATION: f64 = 0.3; + +// Cache the cubic-bezier easing closure to avoid per-frame allocations +static EASE_IN_OUT: OnceLock<Box<dyn Fn(f32) -> f32 + Send + Sync>> = OnceLock::new(); // inside your layout interpolation code: - let ease_in_out = bezier_easing::bezier_easing(0.42, 0.0, 0.58, 1.0).unwrap(); + let ease_in_out = EASE_IN_OUT.get_or_init(|| { + bezier_easing::bezier_easing(0.42, 0.0, 0.58, 1.0) + .expect("valid cubic-bezier (0.42, 0.0, 0.58, 1.0)") + }); + // Clamp t into [0,1], convert to f32, and apply easing + let ease = |t: f64| (**ease_in_out)(t.clamp(0.0, 1.0) as f32) as f64; // replace each `ease_in_out(progress as f32) as f64` with `ease(progress)`Applicable at lines 67, 82, 90, 98, and 115.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/recording/src/lib.rs (1)
44-49: Update RecordingBaseInputs initializations to include new camera_feed fieldThe
RecordingBaseInputsstruct now defines acamera_feedfield. The following call sites are missing this field and will fail to compile:
crates/recording/examples/recording-cli.rs(around theRecordingBaseInputs { … }on line 27)apps/cli/src/record.rs(around thecap_recording::RecordingBaseInputs { … }on line 65)Please update each initialization block to explicitly set
camera_feed, for example:RecordingBaseInputs { capture_target: …, capture_system_audio: …, mic_feed: …, + camera_feed: None, }If you’d like to supply an actual feed, replace
Nonewith an appropriateArc<CameraFeedLock>.apps/desktop/src-tauri/src/windows.rs (1)
677-679: Redundant camera window closureThe camera window is being closed twice - once at line 673 and again at lines 677-679. This appears to be duplicate cleanup logic.
Apply this diff to remove the duplicate closure:
if let Some(v) = CapWindowId::Camera.get(&handle) { let _ = v.close(); } let _ = app.mic_feed.ask(microphone::RemoveInput).await; let _ = app.camera_feed.ask(camera::RemoveInput).await; - if let Some(win) = CapWindowId::Camera.get(&handle) { - win.close().ok(); - }
♻️ Duplicate comments (4)
apps/desktop/src-tauri/src/recording.rs (1)
676-679: Duplicate camera window closureThis code attempts to close the camera window twice - once at line 673 (if Camera window exists) and again at lines 677-679. This appears to be redundant.
apps/desktop/src-tauri/src/camera.rs (2)
452-454: Remove or convert debug print statement to proper loggingThis debug print statement should be removed from production code or converted to use the tracing logging framework.
- let size = resize_window(&window, &default_state, aspect) - .context("Error resizing Tauri window")?; - - println!("1: {:?} {:?} {:?}", size.0, size.1, aspect); + let size = resize_window(&window, &default_state, aspect) + .context("Error resizing Tauri window")?; + + tracing::debug!("Initial window size: {}x{}, aspect: {}", size.0, size.1, aspect);
512-513: Remove another debug print statement from production codeAnother debug print statement that should be removed or converted to proper logging.
- let aspect_ratio = frame.frame.width() as f32 / frame.frame.height() as f32; - println!("2: {:?}", aspect_ratio); + let aspect_ratio = frame.frame.width() as f32 / frame.frame.height() as f32; + tracing::debug!("Camera aspect ratio: {}", aspect_ratio);apps/desktop/src-tauri/src/lib.rs (1)
1776-1786: TODO comment removed - camera preview readiness logic now implementedGood to see the TODO comment has been addressed. The
await_camera_preview_readyfunction now properly waits for the camera feed to be ready using a oneshot channel pattern.
🧹 Nitpick comments (14)
crates/recording/src/sources/audio_input.rs (3)
32-32: Avoid unnecessary clone on Copy type (Clippy warning).
AudioInfoimplementsCopy. Use deref instead ofclone()to silence Clippy and avoid extra method call.- audio_info: feed.audio_info().clone(), + audio_info: *feed.audio_info(),
118-129: Wrong error message refers to “camera feed” in microphone source.These logs/errors will mislead debugging. Replace “camera” with “microphone” (or “audio input”) here.
- Err(_) => { - error!("Lost connection with the camera feed"); - break Err("Lost connection with the camera feed".to_string()); - } + Err(_) => { + error!("Lost connection with the microphone feed"); + break Err("Lost connection with the microphone feed".to_string()); + }
49-70: Uncheckedunwrap()s on timestamp arithmetic can panic under clock/driver anomalies.If CPAL’s
StreamInstant::duration_sinceorSystemTime::checked_add/duration_sincefail, we panic. Consider saturating fallbacks or error propagation to avoid tearing down the pipeline due to transient timing issues.Please confirm CPAL’s
StreamInstantAPI exposes a saturating alternative (or returnsOptionvsResult). If it does, prefer a non-panicking path. Otherwise, handle theErr/Noneexplicitly and drop/skip the offending frame with a warning.apps/desktop/src-tauri/src/camera_legacy.rs (1)
18-41: Scaling always targets width 1280 and can upscale small frames or exceed 720px height.
- If format != RGBA but the frame is smaller than 1280x720, this code upscales to 1280-wide unnecessarily.
- For tall inputs (e.g., 1080x1920), the computed height can exceed 720 after scaling to width 1280.
Scale down only when needed, preserving aspect ratio under both max width and max height, and avoid upscaling.
- if frame.format() != Pixel::RGBA || frame.width() > 1280 || frame.height() > 720 { + if frame.format() != Pixel::RGBA || frame.width() > 1280 || frame.height() > 720 { let converter = match &mut converter { Some((format, converter)) if *format == frame.format() && converter.input().width == frame.width() && converter.input().height == frame.height() => { converter } _ => { - &mut converter - .insert(( - frame.format(), - ffmpeg::software::scaling::Context::get( - frame.format(), - frame.width(), - frame.height(), - Pixel::RGBA, - 1280, - (1280.0 / (frame.width() as f64 / frame.height() as f64)) - as u32, - ffmpeg::software::scaling::flag::Flags::FAST_BILINEAR, - ) - .unwrap(), - )) - .1 + let max_w = 1280u32; + let max_h = 720u32; + let in_w = frame.width(); + let in_h = frame.height(); + // Compute scale that respects both bounds and avoids upscaling. + let scale_w = max_w as f64 / in_w as f64; + let scale_h = max_h as f64 / in_h as f64; + let scale = scale_w.min(scale_h).min(1.0); + let out_w = ((in_w as f64) * scale).round() as u32; + let out_h = ((in_h as f64) * scale).round() as u32; + + &mut converter + .insert(( + frame.format(), + ffmpeg::software::scaling::Context::get( + frame.format(), + in_w, + in_h, + Pixel::RGBA, + out_w.max(1), + out_h.max(1), + ffmpeg::software::scaling::flag::Flags::FAST_BILINEAR, + ) + .unwrap(), + )) + .1 } };Optional: replace the
unwrap()s here with error handling and a warning, then continue by skipping the frame to avoid crashing the preview thread.crates/recording/src/lib.rs (1)
22-23: Use consistent import style forfeedspaths.This file mixes
use feeds::microphone::...anduse crate::feeds::camera::.... Prefer one style for consistency; since the file already uses the unqualifiedfeeds::..., mirror that here.-use crate::feeds::camera::CameraFeedLock; +use feeds::camera::CameraFeedLock;apps/desktop/src-tauri/src/deeplink_actions.rs (1)
65-66: Prefer structured logging overeprintln!.Use
tracing::error!for consistency with the rest of the file and to integrate with our logging pipeline.- eprintln!("Failed to handle deep link action: {e}"); + tracing::error!("Failed to handle deep link action: {e}");apps/desktop/src-tauri/src/windows.rs (1)
398-416: Avoid returning anyhow::Error from async command handlersReturning
anyhow!()error types from Tauri command handlers may not serialize properly for the frontend. Consider using a more specific error type or converting to a String.Apply this diff to return a proper Tauri error:
- return Err(anyhow!( - "Unable to initialize camera preview as one already exists!" - ) - .into()); + return Err(tauri::Error::Anyhow(anyhow!( + "Unable to initialize camera preview as one already exists!" + )));crates/recording/src/sources/camera.rs (2)
32-32: Remove unnecessary clone on Copy type
VideoInfoimplements theCopytrait, so cloning is unnecessary.Apply this diff to remove the unnecessary clone:
- video_info: feed.video_info().clone(), + video_info: *feed.video_info(),
107-111: Consider error handling for AddSenderThe error from
blocking_send()is being ignored withlet _. While this might be intentional if the feed is expected to be unavailable during shutdown, consider at least logging this error for debugging purposes.- let _ = self.feed.ask(camera::AddSender(tx)).blocking_send(); + if let Err(e) = self.feed.ask(camera::AddSender(tx)).blocking_send() { + tracing::warn!("Failed to add camera frame sender: {}", e); + }crates/recording/src/feeds/camera.rs (3)
76-76: Remove unused fieldidfrom AttachedStateThe
idfield inAttachedStateis never read and can be safely removed to reduce memory usage.Apply this diff to remove the unused field:
struct AttachedState { - id: DeviceOrModelID, handle: cap_camera::CaptureHandle, camera_info: cap_camera::CameraInfo, video_info: VideoInfo, }And update the construction at line 59:
self.attached = Some(AttachedState { - id, handle: data.handle, camera_info: data.camera_info, video_info: data.video_info, });
83-92: Consider adding Default implementation for CameraFeedSince
CameraFeed::new()takes no parameters and creates a default instance, implementing theDefaulttrait would be more idiomatic.Add the Default implementation:
impl Default for CameraFeed { fn default() -> Self { Self::new() } }
336-336: Remove unnecessary clones on Copy type VideoInfo
VideoInfoimplements theCopytrait, so cloning is unnecessary and less efficient.Apply these diffs:
At line 336:
- let _ = ready_tx.send(Ok((r.camera_info.clone(), r.video_info.clone()))); + let _ = ready_tx.send(Ok((r.camera_info.clone(), r.video_info)));At line 458:
- let video_info = attached.video_info.clone(); + let video_info = attached.video_info;Also applies to: 458-458
apps/desktop/src-tauri/src/camera.rs (2)
104-134: Consider adding timeout for actor communicationThe
init_windowmethod asks the camera feed actor to add a sender, but doesn't have a timeout. If the actor is unresponsive, this could hang indefinitely.Consider wrapping the actor call with a timeout:
- actor - .ask(feeds::camera::AddSender(camera_tx)) - .await - .context("Error attaching camera feed consumer")?; + tokio::time::timeout( + std::time::Duration::from_secs(5), + actor.ask(feeds::camera::AddSender(camera_tx)) + ) + .await + .context("Timeout waiting for camera feed actor")? + .context("Error attaching camera feed consumer")?;
425-450: Consider extracting fallback rendering to a separate methodThe fallback rendering logic for showing a blank color could be extracted to improve readability and reusability.
Consider extracting this to a dedicated method:
impl Renderer { fn render_fallback(&self) { if let Ok(surface) = self.surface.get_current_texture() .map_err(|err| error!("Error getting camera renderer surface texture: {err:?}")) { let (buffer, stride) = render_solid_frame( [0x11, 0x11, 0x11, 0xFF], // #111111 5, 5, ); PreparedTexture::init( self.device.clone(), self.queue.clone(), &self.sampler, &self.bind_group_layout, self.uniform_bind_group.clone(), self.render_pipeline.clone(), 5, 5, ) .render(&surface, &buffer, stride); surface.present(); } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (13)
apps/desktop/src-tauri/src/camera.rs(9 hunks)apps/desktop/src-tauri/src/camera_legacy.rs(1 hunks)apps/desktop/src-tauri/src/deeplink_actions.rs(2 hunks)apps/desktop/src-tauri/src/lib.rs(12 hunks)apps/desktop/src-tauri/src/recording.rs(4 hunks)apps/desktop/src-tauri/src/windows.rs(3 hunks)crates/cursor-capture/src/position.rs(1 hunks)crates/recording/src/feeds/camera.rs(3 hunks)crates/recording/src/feeds/mod.rs(1 hunks)crates/recording/src/lib.rs(2 hunks)crates/recording/src/sources/audio_input.rs(1 hunks)crates/recording/src/sources/camera.rs(3 hunks)crates/recording/src/studio_recording.rs(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/cursor-capture/src/position.rs
🧰 Additional context used
📓 Path-based instructions (1)
apps/desktop/src-tauri/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Use tauri_specta on the Rust side for IPC (typed commands/events) and emit events via generated types
Files:
apps/desktop/src-tauri/src/camera_legacy.rsapps/desktop/src-tauri/src/windows.rsapps/desktop/src-tauri/src/deeplink_actions.rsapps/desktop/src-tauri/src/recording.rsapps/desktop/src-tauri/src/lib.rsapps/desktop/src-tauri/src/camera.rs
🧬 Code graph analysis (5)
apps/desktop/src-tauri/src/windows.rs (3)
apps/desktop/src/utils/tauri.ts (2)
GeneralSettingsStore(547-569)Camera(416-426)apps/desktop/src-tauri/src/general_settings.rs (1)
get(176-187)apps/desktop/src-tauri/src/web_api.rs (1)
state(79-79)
crates/recording/src/studio_recording.rs (1)
crates/recording/src/cursor.rs (1)
spawn_cursor_recorder(38-188)
apps/desktop/src-tauri/src/recording.rs (2)
crates/camera/src/lib.rs (1)
list_cameras(42-44)crates/recording/src/feeds/camera.rs (9)
handle(296-351)handle(357-369)handle(375-377)handle(383-400)handle(406-424)handle(440-468)handle(474-499)handle(505-521)handle(527-540)
crates/recording/src/feeds/camera.rs (3)
apps/desktop/src/utils/tauri.ts (2)
CameraInfo(427-431)DeviceOrModelID(522-522)crates/camera/src/lib.rs (1)
device_id(29-31)crates/media-info/src/lib.rs (1)
from_raw_ffmpeg(183-191)
apps/desktop/src-tauri/src/camera.rs (2)
crates/recording/src/feeds/camera.rs (3)
oneshot(307-307)oneshot(309-309)new(83-92)apps/desktop/src-tauri/src/camera_legacy.rs (2)
flume(8-8)flume(9-9)
🪛 GitHub Check: Clippy
crates/recording/src/sources/audio_input.rs
[warning] 32-32: using clone on type AudioInfo which implements the Copy trait
warning: using clone on type AudioInfo which implements the Copy trait
--> crates/recording/src/sources/audio_input.rs:32:25
|
32 | audio_info: feed.audio_info().clone(),
| ^^^^^^^^^^^^^^^^^^^^^^^^^ help: try dereferencing it: *feed.audio_info()
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#clone_on_copy
crates/recording/src/sources/camera.rs
[warning] 32-32: using clone on type VideoInfo which implements the Copy trait
warning: using clone on type VideoInfo which implements the Copy trait
--> crates/recording/src/sources/camera.rs:32:25
|
32 | video_info: feed.video_info().clone(),
| ^^^^^^^^^^^^^^^^^^^^^^^^^ help: try dereferencing it: *feed.video_info()
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#clone_on_copy
crates/recording/src/feeds/camera.rs
[warning] 83-92: you should consider adding a Default implementation for CameraFeed
warning: you should consider adding a Default implementation for CameraFeed
--> crates/recording/src/feeds/camera.rs:83:5
|
83 | / pub fn new() -> Self {
84 | | Self {
85 | | state: State::Open(OpenState {
86 | | connecting: None,
... |
92 | | }
| |_____^
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#new_without_default
= note: #[warn(clippy::new_without_default)] on by default
help: try adding this
|
82 + impl Default for CameraFeed {
83 + fn default() -> Self {
84 + Self::new()
85 + }
86 + }
|
[warning] 76-76: field id is never read
warning: field id is never read
--> crates/recording/src/feeds/camera.rs:76:5
|
75 | struct AttachedState {
| ------------- field in this struct
76 | id: DeviceOrModelID,
| ^^
|
= note: #[warn(dead_code)] on by default
[warning] 458-458: using clone on type VideoInfo which implements the Copy trait
warning: using clone on type VideoInfo which implements the Copy trait
--> crates/recording/src/feeds/camera.rs:458:26
|
458 | let video_info = attached.video_info.clone();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try removing the clone call: attached.video_info
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#clone_on_copy
[warning] 336-336: using clone on type VideoInfo which implements the Copy trait
warning: using clone on type VideoInfo which implements the Copy trait
--> crates/recording/src/feeds/camera.rs:336:70
|
336 | let _ = ready_tx.send(Ok((r.camera_info.clone(), r.video_info.clone())));
| ^^^^^^^^^^^^^^^^^^^^ help: try removing the clone call: r.video_info
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#clone_on_copy
= note: #[warn(clippy::clone_on_copy)] on by default
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (13)
apps/desktop/src-tauri/src/camera_legacy.rs (1)
1-1: Import path update is correct and consistent with feeds module layout.Switching to
cap_recording::feeds::camera::RawCameraFramematches the new public module exposure infeeds::mod.rs. No concerns.apps/desktop/src-tauri/src/deeplink_actions.rs (3)
1-1: Import path update looks correct.Using
feeds::camera::DeviceOrModelIDmatches the new module layout. No functional concerns.
7-7: Removed legacy camera preview dependency: good cleanup.Dropping the preview handle import here reduces coupling to the preview layer and matches the actorified feed design.
117-117: Confirmset_camera_inputintegration
- Signature in
apps/desktop/src-tauri/src/lib.rs#[tauri::command] #[specta::specta] async fn set_camera_input( state: MutableState<'_, App>, id: Option<DeviceOrModelID>, ) -> Result<(), String> { … }- Call site in
apps/desktop/src-tauri/src/deeplink_actions.rscrate::set_camera_input(state.clone(), camera).await?;- Side effects
• Sendsfeeds::camera::RemoveInput/SetInput { id }toApp.camera_feedviaActorRef
• CameraPreviewManager is wired to the same actor (through itsAddSendersubscription) and will reflect the new input automatically—no direct call needed here- Event emission
•set_camera_inputdoes not emit any Tauri events; if the UI must react immediately to a device change (beyond the raw video frames), consider adding a tauri_specta-typed event (e.g.CameraInputChanged) after the actor callPlease verify that the preview manager behaves as expected when the feed changes and decide whether you need to emit an explicit event for the frontend.
apps/desktop/src-tauri/src/windows.rs (1)
446-455: Good error handling for camera preview initializationExcellent defensive programming - properly handling initialization errors by logging and closing the window on failure.
crates/recording/src/studio_recording.rs (1)
151-154: LGTM! Clean integration with new camera feed architectureThe camera feed logging correctly uses the new actor-based API via
base_inputs.camera_feed.apps/desktop/src-tauri/src/recording.rs (1)
378-382: LGTM! Consistent error handling for camera feed lockingThe camera feed locking follows the same pattern as microphone feed locking, properly handling the case when no input is available.
crates/recording/src/feeds/camera.rs (1)
280-283: Good timeout handling for camera initializationThe implementation properly handles camera initialization timeout, preventing the application from hanging if the camera fails to respond.
apps/desktop/src-tauri/src/camera.rs (2)
495-586: Good async/actor-based architecture implementationThe new async event-driven renderer with proper channel-based communication between the camera feed actor and the renderer is well-designed. The use of
tokio::select!for handling both frame reception and reconfiguration events is clean and avoids race conditions.
862-864: Good addition ofget_latest_keymethodThe addition of
get_latest_keymethod to theCachedstruct is a useful enhancement that allows retrieving the last used key without modifying the cache state.apps/desktop/src-tauri/src/lib.rs (3)
2167-2175: Properly handle camera preview cleanup on window closeThe window close handler correctly spawns an async task to call
camera_preview.on_window_close(), ensuring proper cleanup of the camera preview resources.
2249-2257: Consistent async handling for window resize eventsThe window resize event handler properly spawns an async task to handle the resize, maintaining consistency with other async operations in the codebase.
243-268: Well-structured camera input managementThe
set_camera_inputcommand properly handles both adding and removing camera inputs through the camera feed actor, with appropriate error handling and async/await patterns.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
crates/recording/src/feeds/camera.rs (2)
20-25: Fix typo: renamerefrence_timetoreference_time(duplicate of prior review).Spelling issue leaks into the public struct and will propagate to API/TS bindings. Rename the field and all references.
-#[derive(Clone)] -pub struct RawCameraFrame { - pub frame: frame::Video, - pub timestamp: Duration, - pub refrence_time: Instant, -} +#[derive(Clone)] +pub struct RawCameraFrame { + pub frame: frame::Video, + pub timestamp: Duration, + pub reference_time: Instant, +} @@ - .tell(NewFrame(RawCameraFrame { + .tell(NewFrame(RawCameraFrame { frame: ff_frame, timestamp: frame.timestamp, - refrence_time: frame.reference_time, + reference_time: frame.reference_time, }))
489-510: Replaceprintln!with structured logging (duplicate of prior review).Use
tracingmacros for consistency with the rest of the file.- println!("connected: {:?}", &id); + debug!("Camera connected: {:?}", &id);
🧹 Nitpick comments (11)
crates/recording/src/feeds/camera.rs (6)
83-95: AddDefaultforCameraFeedto satisfy clippy and ease construction.Clippy flagged
new_without_default. This is a trivial impl delegating tonew().+impl Default for CameraFeed { + fn default() -> Self { + Self::new() + } +}
76-81: Silence dead field warning or remove unusedidinAttachedState.
idisn’t read; rename to_idor remove it.-struct AttachedState { - id: DeviceOrModelID, +struct AttachedState { + _id: DeviceOrModelID, handle: cap_camera::CaptureHandle, camera_info: cap_camera::CameraInfo, video_info: VideoInfo, } @@ - self.attached = Some(AttachedState { - id, + self.attached = Some(AttachedState { + _id: id, handle: data.handle, camera_info: data.camera_info, video_info: data.video_info, });
338-343: Remove unnecessary clone ofVideoInfo(Copy type).
VideoInfoimplementsCopy. Avoid.clone().- let _ = ready_tx.send(Ok((r.camera_info.clone(), r.video_info.clone()))); + let _ = ready_tx.send(Ok((r.camera_info.clone(), r.video_info)));
474-477: Remove unnecessary clone ofVideoInfo(Copy type).Same here; prefer copy.
- let video_info = attached.video_info.clone(); + let video_info = attached.video_info;
18-18: Consider makingCAMERA_INIT_TIMEOUTconfigurable.4s might be tight on cold-start or high-latency backends. Consider reading from settings or env failpoint.
426-442: Avoid cloning full frames per sender; useArcto reduce copies.Cloning
frame::Videofor each sender can be expensive. Consider sendingArc<RawCameraFrame>and cloning theArc.Here’s the minimal shape of the change:
- senders: Vec<flume::Sender<RawCameraFrame>>, + senders: Vec<flume::Sender<Arc<RawCameraFrame>>>, @@ -struct NewFrame(RawCameraFrame); +struct NewFrame(Arc<RawCameraFrame>); @@ - if let Err(flume::TrySendError::Disconnected(_)) = sender.try_send(msg.0.clone()) { + if let Err(flume::TrySendError::Disconnected(_)) = sender.try_send(msg.0.clone()) { @@ setup_camera - .tell(NewFrame(RawCameraFrame { + .tell(NewFrame(Arc::new(RawCameraFrame { frame: ff_frame, timestamp: frame.timestamp, - reference_time: frame.reference_time, + reference_time: frame.reference_time, - })) + })))Note: This change ripples to consumers; evaluate impact before adopting.
apps/desktop/src-tauri/src/deeplink_actions.rs (1)
16-33: Optional: Gate deep link handling when camera permission is denied.If camera access isn’t permitted,
set_camera_inputwill error; consider early validation for a more user-friendly error path.apps/desktop/src-tauri/src/lib.rs (4)
241-275: Good:set_camera_inputawaits camera readiness and surfaces UI.
- Shows the Camera window prior to initialization.
- Awaits the actor’s readiness future, satisfying “Make recording await the camera being ready.”
Minor: Consider returning
(CameraInfo, VideoInfo)to the caller for telemetry/UX (you already have it at the call site).
1783-1793: Add timeout/error surface toawait_camera_preview_ready.As written, callers can hang indefinitely. Add a bounded timeout and propagate an error to TS.
async fn await_camera_preview_ready(app: MutableState<'_, App>) -> Result<bool, String> { let app = app.read().await.camera_feed.clone(); let (tx, rx) = oneshot::channel(); app.tell(feeds::camera::ListenForReady(tx)) .await .map_err(|err| format!("error registering ready listener: {err}"))?; - rx.await + tokio::time::timeout(std::time::Duration::from_secs(8), rx) + .await + .map_err(|_| "camera preview ready: timed out".to_string())? + .map_err(|err| format!("error receiving ready signal: {err}"))?; - .map_err(|err| format!("error receiving ready signal: {err}"))?; Ok(true) }
2038-2056: LGTM: Close camera window when feed disconnects.Good UX safety net. Consider also surfacing a toast/notification for better user feedback.
2193-2201: Optional: Consider removing camera input when the Camera window closes.Today you only call
camera_preview.on_window_close(). If the preview window controls the camera lifecycle, you may also want to:
- remove input (tell
RemoveInput) to release the camera device,- unless a recording is active or another consumer holds a lock.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
apps/desktop/src-tauri/src/deeplink_actions.rs(2 hunks)apps/desktop/src-tauri/src/lib.rs(13 hunks)crates/recording/src/feeds/camera.rs(3 hunks)crates/recording/src/sources/camera.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/recording/src/sources/camera.rs
🧰 Additional context used
📓 Path-based instructions (1)
apps/desktop/src-tauri/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Use tauri_specta on the Rust side for IPC (typed commands/events) and emit events via generated types
Files:
apps/desktop/src-tauri/src/lib.rsapps/desktop/src-tauri/src/deeplink_actions.rs
🧬 Code graph analysis (3)
crates/recording/src/feeds/camera.rs (4)
apps/desktop/src/utils/tauri.ts (2)
CameraInfo(427-431)DeviceOrModelID(522-522)apps/desktop/src-tauri/src/recording.rs (2)
id(147-152)list_cameras(189-191)crates/camera/src/lib.rs (1)
device_id(29-31)crates/media-info/src/lib.rs (1)
from_raw_ffmpeg(183-191)
apps/desktop/src-tauri/src/lib.rs (3)
crates/recording/src/feeds/camera.rs (13)
oneshot(311-311)oneshot(313-313)new(84-94)handle(300-355)handle(361-377)handle(383-385)handle(391-408)handle(414-420)handle(426-442)handle(458-486)handle(492-517)handle(523-539)handle(545-558)apps/desktop/src-tauri/src/windows.rs (6)
app(196-196)app(316-316)app(404-404)app(704-704)app(893-893)id(698-724)apps/desktop/src-tauri/src/camera.rs (1)
new(62-69)
apps/desktop/src-tauri/src/deeplink_actions.rs (1)
apps/desktop/src-tauri/src/lib.rs (5)
set_camera_input(243-275)app(1118-1119)app(2153-2153)app(2189-2189)app(2195-2195)
🪛 GitHub Check: Clippy
crates/recording/src/feeds/camera.rs
[warning] 84-94: you should consider adding a Default implementation for CameraFeed
warning: you should consider adding a Default implementation for CameraFeed
--> crates/recording/src/feeds/camera.rs:84:5
|
84 | / pub fn new() -> Self {
85 | | Self {
86 | | state: State::Open(OpenState {
87 | | connecting: None,
... |
94 | | }
| |_____^
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#new_without_default
= note: #[warn(clippy::new_without_default)] on by default
help: try adding this
|
83 + impl Default for CameraFeed {
84 + fn default() -> Self {
85 + Self::new()
86 + }
87 + }
|
[warning] 77-77: field id is never read
warning: field id is never read
--> crates/recording/src/feeds/camera.rs:77:5
|
76 | struct AttachedState {
| ------------- field in this struct
77 | id: DeviceOrModelID,
| ^^
|
= note: #[warn(dead_code)] on by default
[warning] 476-476: using clone on type VideoInfo which implements the Copy trait
warning: using clone on type VideoInfo which implements the Copy trait
--> crates/recording/src/feeds/camera.rs:476:26
|
476 | let video_info = attached.video_info.clone();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try removing the clone call: attached.video_info
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#clone_on_copy
[warning] 340-340: using clone on type VideoInfo which implements the Copy trait
warning: using clone on type VideoInfo which implements the Copy trait
--> crates/recording/src/feeds/camera.rs:340:70
|
340 | let _ = ready_tx.send(Ok((r.camera_info.clone(), r.video_info.clone())));
| ^^^^^^^^^^^^^^^^^^^^ help: try removing the clone call: r.video_info
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#clone_on_copy
= note: #[warn(clippy::clone_on_copy)] on by default
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (13)
crates/recording/src/feeds/camera.rs (6)
196-202: LGTM: Camera selection logic looks correct.Matching by DeviceID or ModelID is straightforward and robust.
252-294: LGTM: One-shot readiness signal and ffmpeg frame setup look good.
- PTS set from
timestampin microseconds matchesVideoInfo::time_base.- First-frame readiness via
oneshotcleanly initializesvideo_info.If you see intermittent timeouts, verify first-frame delivery from drivers that buffer N frames before invoking the callback.
315-355: LGTM:SetInputhandshake and error mapping are solid.The two-layer
oneshotpattern (internal vs external ready) and error mapping toBuildStreamCrashedis clean.
372-377: LGTM: Disconnect callbacks are invoked onRemoveInput.This enables window cleanup hooks from the app layer.
388-409: LGTM:ListenForReadyimmediate/queued semantics are correct.Ready is sent immediately when attached/locked, otherwise queued.
123-127: Use non-blockingtry_send()in Drop instead ofblocking_send()
Blocking in aDropimplementation can deadlock if the mailbox is full or the actor’s thread is busy. Switch to a best-effort, fire-and-forget send that fails fast when the mailbox is full or the actor is gone. (docs.rs, docs.page)impl Drop for CameraFeedLock { fn drop(&mut self) { - let _ = self.lock_tx.tell(Unlock).blocking_send(); + // Best-effort unlock: ignore if the mailbox is full or actor is shut down. + let _ = self.lock_tx.tell(Unlock).try_send(); } }apps/desktop/src-tauri/src/deeplink_actions.rs (2)
1-1: LGTM: Updated import path forDeviceOrModelID.Matches the new public surface under
feeds::camera.
117-118: Allset_camera_inputcall sites updated to the new signature
I’ve searched the entire codebase for any lingering 4-argument calls and found none—there are no remaining references to the old signature.apps/desktop/src-tauri/src/lib.rs (5)
104-118: LGTM: App now ownscamera_previewandcamera_feed.This aligns with the new actor-driven design and centralizes lifecycle.
1766-1777: LGTM: Persisting camera preview state via manager is clean.Error mapping is consistent; this will also play well with specta-typed IPC.
1935-1937: LGTM: CameraFeed spawn and sender hookup are correct.Legacy WS preview keeps working by wiring the new actor into the existing frame bus.
2153-2163: LGTM: Cleanup feeds on main window close when idle.Ensures camera/mic aren’t left running if no recording is active/pending.
2275-2283: LGTM: Resize events forwarded tocamera_preview.Non-blocking spawn avoids stalling the event loop.
There was a problem hiding this comment.
Actionable comments posted: 2
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/sources/camera.rs (1)
61-69: Potential panic on Duration/Instant arithmetic; switch to saturating operations
camera_frame.timestamp - first_frame_timestampcan underflow if frames arrive out of order. Likewise,(first_frame_instant + relative_timestamp) - self.start_instantwill panic ifself.start_instantis later. Use saturating variants to avoid runtime panics.Apply this diff:
- let relative_timestamp = camera_frame.timestamp - first_frame_timestamp; + let relative_timestamp = + camera_frame.timestamp.saturating_sub(first_frame_timestamp); @@ - (first_frame_instant + relative_timestamp - self.start_instant).as_secs_f64(), + (first_frame_instant + relative_timestamp) + .saturating_duration_since(self.start_instant) + .as_secs_f64(),
🧹 Nitpick comments (9)
crates/recording/src/sources/camera.rs (1)
107-112: Use tell() instead of ask() for fire-and-forget registrationRegistering a sender doesn’t require a reply. Using
ask(...).blocking_send()adds unnecessary overhead;tell(...).blocking_send()is simpler and conveys intent.Apply this diff:
- let frames = frames_rx.get_or_insert_with(|| { - let (tx, rx) = flume::bounded(5); - let _ = self.feed.ask(camera::AddSender(tx)).blocking_send(); - rx - }); + let frames = frames_rx.get_or_insert_with(|| { + let (tx, rx) = flume::bounded(5); + let _ = self.feed.tell(camera::AddSender(tx)).blocking_send(); + rx + });apps/desktop/src-tauri/src/camera.rs (3)
121-127: Avoid spawning a dedicated Tokio Runtime per preview; reuse the app runtimeCreating a per-preview
Runtimeand thread is heavier than needed and complicates shutdown. Prefertokio::spawn(renderer.run(...))on the existing runtime (Tauri already installs one). If you must pin to a thread, consider a single worker for all previews.
551-569: Double window resize/reconfigure in State event
sync_ratio_uniform_and_resize_window_to_italready resizes and reconfigures the surface; callingresize_window+reconfigure_gpu_surfaceagain duplicates work and can cause flicker. Consider removing one path.
783-841: Move write_texture before the render pass to avoid hazardsWriting the texture while a render pass is open is unusual and may risk synchronization issues on some backends. Move
queue.write_texture(...)beforebegin_render_pass.Apply this diff:
- { - let mut render_pass = encoder.begin_render_pass(&wgpu::RenderPassDescriptor { + { + // Upload texture data first + self.queue.write_texture( + wgpu::TexelCopyTextureInfo { + texture: &self.texture, + mip_level: 0, + origin: wgpu::Origin3d::ZERO, + aspect: wgpu::TextureAspect::All, + }, + buffer, + wgpu::TexelCopyBufferLayout { + offset: 0, + bytes_per_row: Some(stride), + rows_per_image: Some(self.height), + }, + wgpu::Extent3d { + width: self.width, + height: self.height, + depth_or_array_layers: 1, + }, + ); + + let mut render_pass = encoder.begin_render_pass(&wgpu::RenderPassDescriptor { label: None, color_attachments: &[Some(wgpu::RenderPassColorAttachment { view: &surface_view, resolve_target: None, ops: wgpu::Operations { load: wgpu::LoadOp::Clear(wgpu::Color { r: 0.0, g: 0.0, b: 0.0, a: 0.0, }), store: wgpu::StoreOp::Store, }, })], depth_stencil_attachment: None, timestamp_writes: None, occlusion_query_set: None, }); - - self.queue.write_texture( - wgpu::TexelCopyTextureInfo { - texture: &self.texture, - mip_level: 0, - origin: wgpu::Origin3d::ZERO, - aspect: wgpu::TextureAspect::All, - }, - buffer, - wgpu::TexelCopyBufferLayout { - offset: 0, - bytes_per_row: Some(stride), - rows_per_image: Some(self.height), - }, - wgpu::Extent3d { - width: self.width, - height: self.height, - depth_or_array_layers: 1, - }, - );crates/recording/src/feeds/camera.rs (4)
84-94: Add Default for CameraFeed to satisfy clippy and conventionClippy warns about
newwithoutDefault. ImplementingDefaultimproves ergonomics and silences the lint.Apply this diff:
impl CameraFeed { pub fn new() -> Self { Self { state: State::Open(OpenState { connecting: None, attached: None, }), senders: Vec::new(), on_ready: Vec::new(), on_disconnect: Vec::new(), } } } + +impl Default for CameraFeed { + fn default() -> Self { + Self::new() + } +}
340-342: Avoid clone on Copy type VideoInfo
VideoInfoimplementsCopy; remove.clone()to avoid lint and minor overhead.Apply this diff:
- let _ = ready_tx.send(Ok((r.camera_info.clone(), r.video_info.clone()))); + let _ = ready_tx.send(Ok((r.camera_info.clone(), r.video_info)));
476-476: Avoid clone on Copy type VideoInfo (again)Same rationale; use the value directly.
Apply this diff:
- let video_info = attached.video_info.clone(); + let video_info = attached.video_info;
76-81: Unused field id in AttachedState
idis never read after being set. Either remove it or add a comment/allow to justify future use. Dead fields tend to drift out of date.apps/desktop/src-tauri/src/lib.rs (1)
1935-1937: Consider reusing camera_tx across both legacy WS and actor-based preview sendersYou attach
camera_txto the newCameraFeedright after creating it, which is fine. If you observe backpressure, consider a larger buffer forcamera_txor per-consumer channels (already supported by the actor).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
apps/desktop/src-tauri/src/camera.rs(9 hunks)apps/desktop/src-tauri/src/lib.rs(12 hunks)apps/desktop/src/utils/tauri.ts(3 hunks)crates/recording/src/feeds/camera.rs(2 hunks)crates/recording/src/sources/camera.rs(5 hunks)crates/rendering/src/layout.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/rendering/src/layout.rs
🧰 Additional context used
📓 Path-based instructions (5)
**/tauri.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Never edit auto-generated IPC bindings file: tauri.ts
Files:
apps/desktop/src/utils/tauri.ts
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/desktop/**/*.{ts,tsx}: In the desktop app, rely on unplugin-icons auto-imports; do not manually import icon modules
Use generated tauri_specta commands/events (commands, events) in the desktop frontend; listen to generated events directly
Use @tanstack/solid-query for server state in the desktop app
Files:
apps/desktop/src/utils/tauri.ts
{apps/desktop,packages/ui-solid}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Component naming (Solid): components in PascalCase; hooks/utilities in camelCase starting with 'use' where applicable
Files:
apps/desktop/src/utils/tauri.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use strict TypeScript and avoid any; prefer shared types from packages
Files:
apps/desktop/src/utils/tauri.ts
apps/desktop/src-tauri/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Use tauri_specta on the Rust side for IPC (typed commands/events) and emit events via generated types
Files:
apps/desktop/src-tauri/src/camera.rsapps/desktop/src-tauri/src/lib.rs
🧬 Code graph analysis (3)
apps/desktop/src-tauri/src/camera.rs (2)
apps/desktop/src-tauri/src/lib.rs (8)
new(338-340)new(757-761)new(1388-1396)app(1118-1119)app(2153-2153)app(2189-2189)app(2195-2195)None(2387-2387)crates/recording/src/feeds/camera.rs (3)
oneshot(311-311)oneshot(313-313)new(84-94)
apps/desktop/src-tauri/src/lib.rs (4)
apps/desktop/src/utils/tauri.ts (4)
CameraPreviewState(435-439)DeviceOrModelID(522-522)ShowCapWindow(718-730)Camera(416-426)crates/recording/src/feeds/camera.rs (3)
oneshot(311-311)oneshot(313-313)new(84-94)apps/desktop/src-tauri/src/windows.rs (6)
app(196-196)app(316-316)app(404-404)app(704-704)app(893-893)id(698-724)apps/desktop/src-tauri/src/camera.rs (1)
new(62-69)
crates/recording/src/feeds/camera.rs (4)
apps/desktop/src/utils/tauri.ts (3)
CameraInfo(427-431)Video(770-776)DeviceOrModelID(522-522)apps/desktop/src-tauri/src/recording.rs (2)
id(147-152)list_cameras(189-191)crates/camera/src/lib.rs (1)
device_id(29-31)crates/media-info/src/lib.rs (1)
from_raw_ffmpeg(183-191)
🪛 GitHub Check: Clippy
crates/recording/src/feeds/camera.rs
[warning] 84-94: you should consider adding a Default implementation for CameraFeed
warning: you should consider adding a Default implementation for CameraFeed
--> crates/recording/src/feeds/camera.rs:84:5
|
84 | / pub fn new() -> Self {
85 | | Self {
86 | | state: State::Open(OpenState {
87 | | connecting: None,
... |
94 | | }
| |_____^
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#new_without_default
= note: #[warn(clippy::new_without_default)] on by default
help: try adding this
|
83 + impl Default for CameraFeed {
84 + fn default() -> Self {
85 + Self::new()
86 + }
87 + }
|
[warning] 77-77: field id is never read
warning: field id is never read
--> crates/recording/src/feeds/camera.rs:77:5
|
76 | struct AttachedState {
| ------------- field in this struct
77 | id: DeviceOrModelID,
| ^^
|
= note: #[warn(dead_code)] on by default
[warning] 476-476: using clone on type VideoInfo which implements the Copy trait
warning: using clone on type VideoInfo which implements the Copy trait
--> crates/recording/src/feeds/camera.rs:476:26
|
476 | let video_info = attached.video_info.clone();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try removing the clone call: attached.video_info
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#clone_on_copy
[warning] 340-340: using clone on type VideoInfo which implements the Copy trait
warning: using clone on type VideoInfo which implements the Copy trait
--> crates/recording/src/feeds/camera.rs:340:70
|
340 | let _ = ready_tx.send(Ok((r.camera_info.clone(), r.video_info.clone())));
| ^^^^^^^^^^^^^^^^^^^^ help: try removing the clone call: r.video_info
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#clone_on_copy
= note: #[warn(clippy::clone_on_copy)] on by default
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (11)
apps/desktop/src/utils/tauri.ts (3)
9-11: Signature change to setCameraInput aligns with backend command injectionThe new signature and return type look correct for the updated Rust command (AppHandle/State are injected by Tauri). Given this file is auto-generated, no manual edits are required.
225-227: CameraPreviewState wiring matches Rust side renameThe parameter type change to CameraPreviewState mirrors the backend rename (CameraWindowState → CameraPreviewState). As this file is generated, keep changes source-of-truth in Rust only.
435-440: New CameraPreviewState type exported as expectedThe fields (size, shape, mirrored) match the Rust struct. No issues.
crates/recording/src/sources/camera.rs (2)
27-39: Arc integration looks correctInitializing
video_infovia*feed.video_info()(Copy) and carryingstart_instantfor timeline alignment makes sense with the new actor model.
79-94: Draining frames on shutdown is a good guard against “stuck old frames”The explicit drain with per-frame processing ensures the final buffered frames are flushed deterministically.
apps/desktop/src-tauri/src/camera.rs (2)
407-440: Nice blank-frame fallback to hide init latencyRendering a neutral frame before the camera is ready avoids visual garbage. Good touch.
574-577: Resource cleanup path inconsistenciesYou explicitly call
self.device.destroy()only on the “feed completed” path; the Shutdown path returns early. Either remove the explicit destroy (wgpu resources drop cleanly) or ensure symmetric cleanup across both branches.Would you like me to submit a follow-up diff to harmonize cleanup?
crates/recording/src/feeds/camera.rs (1)
252-294: Startup handshake, timeout, and first-frame capture look solidUsing a oneshot to signal first-frame readiness with a 4s timeout helps de-flake initialization and supports
awaitCameraPreviewReady.apps/desktop/src-tauri/src/lib.rs (3)
241-275: set_camera_input flow looks correct with the new actor modelShowing the camera window before awaiting feed setup reduces flicker; the double-await to resolve the BoxFuture is precise.
2038-2056: Good: closing the camera window when the feed disconnectsRegistering
OnFeedDisconnectensures the preview lifecycle follows the feed. Nice touch.
2193-2200: Graceful preview shutdown on window destroyForwarding the event to
CameraPreviewManager::on_window_close()matches the new lifecycle API.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/desktop/src/routes/target-select-overlay.tsx (1)
664-665: Nit: Fix typo in local namegeneralSetings→generalSettingsfor consistency.Improves readability and aligns with the imported
generalSettingsStorename.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/desktop/src/routes/target-select-overlay.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/desktop/**/*.{ts,tsx}: In the desktop app, rely on unplugin-icons auto-imports; do not manually import icon modules
Use generated tauri_specta commands/events (commands, events) in the desktop frontend; listen to generated events directly
Use @tanstack/solid-query for server state in the desktop app
Files:
apps/desktop/src/routes/target-select-overlay.tsx
{apps/desktop,packages/ui-solid}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Component naming (Solid): components in PascalCase; hooks/utilities in camelCase starting with 'use' where applicable
Files:
apps/desktop/src/routes/target-select-overlay.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use strict TypeScript and avoid any; prefer shared types from packages
Files:
apps/desktop/src/routes/target-select-overlay.tsx
🧬 Code graph analysis (1)
apps/desktop/src/routes/target-select-overlay.tsx (1)
apps/desktop/src/store.ts (1)
generalSettingsStore(61-62)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (2)
apps/desktop/src/routes/target-select-overlay.tsx (2)
690-717: Nice UX touch adding "Off" to the countdown menu.Placing it first and mapping it to
0is intuitive and keeps parity with other options.
694-700: Narrow “Off” selection to explicit0The other countdown options all use a strict equality check, so we should do the same for “Off” to avoid it appearing selected before the real settings load.
File: apps/desktop/src/routes/target-select-overlay.tsx
Lines: 694–700- await CheckMenuItem.new({ - text: "Off", - action: () => generalSettingsStore.set({ recordingCountdown: 0 }), - checked: - !generalSetings.data?.recordingCountdown || - generalSetings.data?.recordingCountdown === 0, - }), + await CheckMenuItem.new({ + text: "Off", + action: () => generalSettingsStore.set({ recordingCountdown: 0 }), + checked: generalSetings.data?.recordingCountdown === 0, + }),
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/recording/src/feeds/microphone.rs (1)
118-123: Fix sample-rate filter: currently excludes most usable devices that support >48 kHzThe predicate only accepts configs whose max sample rate is <= 48 kHz. Many devices advertise max > 48 kHz while still supporting 48 kHz, so they’re incorrectly filtered out, leading to “no usable device” scenarios.
Change the predicate to “supports 48 kHz within its range.”
- .filter(|c| { - c.min_sample_rate().0 <= 48000 && c.max_sample_rate().0 <= 48000 - }) + .filter(|c| { + let min = c.min_sample_rate().0; + let max = c.max_sample_rate().0; + min <= 48_000 && 48_000 <= max + })
♻️ Duplicate comments (1)
crates/recording/src/feeds/camera.rs (1)
21-25: Typo fixed:reference_timeThe field rename is applied and consistent with prior feedback. Nice.
🧹 Nitpick comments (11)
crates/recording/src/feeds/microphone.rs (4)
6-15: Unify oneshot flavor with camera feed (tokio::sync::oneshot)Camera feed uses tokio::sync::oneshot; microphone uses futures::channel::oneshot. Mixing flavors complicates maintenance and increases deps. Tokio’s oneshot integrates better with the runtime you already spawn onto and is used elsewhere in this PR.
-use futures::{FutureExt, channel::oneshot, future::BoxFuture}; +use futures::{FutureExt, future::BoxFuture}; +use tokio::sync::oneshot;No other code changes are required;
.shared()works on anyFuture(the receiver future is compatible).
304-310: Don’t kill the actor on recoverable stream errors; gracefully detach insteadAbruptly calling
actor_ref.kill()on a stream error tears down the entire feed actor, which can cascade failures for dependents. Prefer gracefully removing the input so the actor remains usable.- move |e| { - error!("Microphone stream error: {e}"); - - let _ = error_sender.send(e).is_err(); - actor_ref.kill(); - }, + move |e| { + error!("Microphone stream error: {e}"); + let _ = error_sender.send(e).is_err(); + // Gracefully detach the stream; the actor stays alive. + let _ = actor_ref.tell(RemoveInput).try_send(); + },
399-404: Handle backpressure: log dropped frames when receivers are fullCurrently only Disconnected is handled; when queues are full, frames are silently dropped. Add trace-level logging (or metrics) to aid debugging under load.
- if let Err(TrySendError::Disconnected(_)) = sender.try_send(msg.clone()) { - warn!("Audio sender {} disconnected, will be removed", i); - to_remove.push(i); - }; + match sender.try_send(msg.clone()) { + Err(TrySendError::Disconnected(_)) => { + warn!("Audio sender {} disconnected, will be removed", i); + to_remove.push(i); + } + Err(TrySendError::Full(_)) => { + trace!("Audio sender {} queue full; dropping frame", i); + } + _ => {} + }
1-15: Channel variety adds cognitive loadThis module uses std::mpsc (done signal), flume (fanout), futures oneshot (ready/drop), while camera uses tokio oneshot. Consider standardizing on tokio + flume (or tokio only) to reduce the surface area.
Would you like a follow-up patch consolidating to tokio oneshot + flume across feeds?
crates/recording/src/feeds/camera.rs (7)
199-205: Make model-id comparison explicit to avoid type-mismatch surprisesComparing
Option<&ModelID>toOption<ModelID>relies on trait gymnastics. Useis_some_andfor clarity.- cap_camera::list_cameras().find(|c| match selected_camera { - DeviceOrModelID::DeviceID(device_id) => c.device_id() == device_id, - DeviceOrModelID::ModelID(model_id) => c.model_id() == Some(model_id), - }) + cap_camera::list_cameras().find(|c| match selected_camera { + DeviceOrModelID::DeviceID(device_id) => c.device_id() == device_id, + DeviceOrModelID::ModelID(model_id) => c.model_id().is_some_and(|m| m == model_id), + })
226-227: Prefer not to exclude 29.97 fps formatsFiltering at
>= 30.0skips 29.97, which is common. Consider a slightly lower threshold or rounding.- .filter(|f| f.frame_rate() >= 30.0 && f.width() < 2000 && f.height() < 2000) + .filter(|f| f.frame_rate() >= 29.0 && f.width() < 2000 && f.height() < 2000)Alternatively, round to nearest integer before comparison.
287-291: Improve timeout error to include camera id and durationThis makes diagnostics clearer in logs and UI.
- let video_info = tokio::time::timeout(CAMERA_INIT_TIMEOUT, ready_rx) - .await - .map_err(|e| SetInputError::Timeout(e.to_string()))? - .map_err(|_| SetInputError::Initialisation)?; + let video_info = tokio::time::timeout(CAMERA_INIT_TIMEOUT, ready_rx) + .await + .map_err(|_| SetInputError::Timeout(format!( + "Initialization timed out after {:?} for {:?}", + CAMERA_INIT_TIMEOUT, id + )))? + .map_err(|_| SetInputError::Initialisation)?;
341-347: Use tell instead of ask for InputConnected (you ignore the reply anyway)This avoids holding up the setup task on the reply path and matches your usage for
InputConnectFailed.- let _ = actor_ref.ask(InputConnected).await; + let _ = actor_ref.tell(InputConnected).await;
375-378: Decide whether disconnect callbacks are one-shot or persistentRight now, callbacks persist across disconnects (you iterate but don’t clear). If callers expect one-shot behavior (clear stale previews) this may accumulate duplicates. Consider draining like you do with
on_ready, or document the persistence.Option to make them one-shot:
- for cb in &self.on_disconnect { - (cb)(); - } + for cb in self.on_disconnect.drain(..) { + (cb)(); + }If persistence is desired, keep as-is and ensure clients register only once.
181-191: Error enum naming polish (optional)
BuildStreamCrashedreads like an internal implementation detail. ConsiderSetupTaskCanceledorSetupChannelClosedif you want a more descriptive external error.
13-15: Cross-crate consistency: oneshot flavorCamera uses tokio’s oneshot while microphone currently uses futures’ oneshot. For consistency and simpler mental model, consider standardizing on tokio across feeds.
I can prep a small patch that switches microphone to tokio oneshot and removes the futures channel import.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
crates/recording/src/feeds/camera.rs(2 hunks)crates/recording/src/feeds/microphone.rs(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
crates/recording/src/feeds/microphone.rs (2)
crates/recording/src/feeds/camera.rs (2)
oneshot(314-314)oneshot(316-316)crates/recording/src/capture_pipeline.rs (1)
tokio(152-152)
crates/recording/src/feeds/camera.rs (4)
apps/desktop/src/utils/tauri.ts (3)
CameraInfo(427-431)Video(770-776)DeviceOrModelID(522-522)crates/recording/src/feeds/microphone.rs (13)
try_as_open(40-46)handle_input_connected(55-66)new(82-92)deref(176-178)drop(182-186)handle(240-359)handle(365-377)handle(383-385)handle(391-411)handle(427-461)handle(467-479)handle(485-501)handle(507-520)crates/camera/src/lib.rs (1)
device_id(29-31)crates/media-info/src/lib.rs (1)
from_raw_ffmpeg(183-191)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (6)
crates/recording/src/feeds/microphone.rs (3)
181-186: Drop-to-unlock flow is clean and matches the camera feed patternThe lock uses a oneshot that’s triggered on Drop and a small task that relays
Unlockback to the actor. This keeps ownership and lifecycle tidy and avoids explicit unlock calls.Also applies to: 447-461
81-92: Device discovery and error reporting look solidGood use of logging,
IndexMapfor stable ordering, and fallbacks for default/iterated devices. Once the sample-rate predicate is fixed, this should be robust across platforms.
364-376: RemoveInput behavior is correct in Open state; confirm Locked-path expectations
RemoveInputintentionally errors when the feed is locked (viatry_as_open). That’s fine, as dropping the lock re-opens the feed. Ensure call-sites don’t try to remove while locked; otherwise, handle the error and retry after unlock.If helpful, I can scan call-sites to ensure we never attempt
RemoveInputwhile locked.crates/recording/src/feeds/camera.rs (3)
1-15: Good alignment with the new actor-based patternUsing tokio oneshot, explicit Open/Locked states, and drop-to-unlock mirrors the microphone feed and meets the PR goals around lifecycle clarity and race reduction.
391-411: Ready-listener behavior is well thought outImmediate fulfillment when already ready/locked and draining listeners on connect avoids races in the UI waiting for preview startup.
Also applies to: 522-525
485-496: Drop-to-unlock on the camera feed is solidThe oneshot +
Unlockrelay returns the actor to Open withattachedintact, which is exactly what we want for window/preview lifecycle.Also applies to: 552-568
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/routes/(window-chrome)/new-main/index.tsx (1)
368-368: Typo in user-facing text: “Singning In...” → “Signing In...”Visible UI copy should be correct.
- <span>Singning In...</span> + <span>Signing In...</span>
♻️ Duplicate comments (2)
crates/recording/src/feeds/camera.rs (2)
24-25: Fix typo in field name:reference_timefield name is correct nowThe typo has been fixed from
refrence_timetoreference_time. This addresses the previous review comment.
56-69: Potential resource leak when switching attachmentsWhen a new input connects while another is already attached, the previous
AttachedStateis replaced without explicitly stopping the old capture handle. This could lead to resource leaks if theDropimplementation doesn't properly clean up native resources.
🧹 Nitpick comments (3)
apps/desktop/src/routes/(window-chrome)/new-main/index.tsx (1)
296-296: Avoid drag region on the buttons cluster to guarantee clickabilityThe inner buttons container is also marked as a drag region. With the new macOS spacer providing ample drag surface, consider removing the drag attribute from the buttons group to reduce any chance of accidental window drags on click (varies by platform/window manager).
Proposed change:
- <div class="flex gap-1 items-center" data-tauri-drag-region> + <div class="flex gap-1 items-center">crates/recording/src/feeds/camera.rs (2)
337-371: Consider removing unnecessary Runtime creationCreating a new Tokio runtime in a spawned thread is unusual since the actor framework should already provide an async context. This pattern could lead to unnecessary overhead.
Consider refactoring to use the existing async context:
- let rt = Runtime::new().expect("Failed to get Tokio runtime!"); - std::thread::spawn(move || { - LocalSet::new().block_on(&rt, async move { + tokio::spawn(async move { let handle = match setup_camera(&id, new_frame_recipient).await { // ... rest of the logic - }) - }); + });
344-344: Remove unnecessary clone calls on Copy typesThe static analysis correctly identifies that
VideoInfoimplementsCopy, so cloning is unnecessary.Apply this diff to remove unnecessary clones:
- video_info: r.video_info.clone(), + video_info: r.video_info,- video_info: r.video_info.clone(), + video_info: r.video_info,Also applies to: 351-351
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
apps/desktop/src/routes/(window-chrome)/new-main/index.tsx(3 hunks)crates/recording/src/feeds/camera.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/desktop/**/*.{ts,tsx}: In the desktop app, rely on unplugin-icons auto-imports; do not manually import icon modules
Use generated tauri_specta commands/events (commands, events) in the desktop frontend; listen to generated events directly
Use @tanstack/solid-query for server state in the desktop app
Files:
apps/desktop/src/routes/(window-chrome)/new-main/index.tsx
{apps/desktop,packages/ui-solid}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Component naming (Solid): components in PascalCase; hooks/utilities in camelCase starting with 'use' where applicable
Files:
apps/desktop/src/routes/(window-chrome)/new-main/index.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use strict TypeScript and avoid any; prefer shared types from packages
Files:
apps/desktop/src/routes/(window-chrome)/new-main/index.tsx
🧬 Code graph analysis (1)
crates/recording/src/feeds/camera.rs (3)
crates/recording/src/feeds/microphone.rs (13)
try_as_open(40-46)handle_input_connected(55-66)new(82-92)deref(176-178)drop(182-186)handle(240-359)handle(365-377)handle(383-385)handle(391-411)handle(427-461)handle(467-479)handle(485-501)handle(507-520)apps/desktop/src-tauri/src/recording.rs (2)
id(147-152)list_cameras(189-191)crates/camera/src/lib.rs (1)
device_id(29-31)
🪛 GitHub Check: Clippy
crates/recording/src/feeds/camera.rs
[warning] 351-351: using clone on type VideoInfo which implements the Copy trait
warning: using clone on type VideoInfo which implements the Copy trait
--> crates/recording/src/feeds/camera.rs:351:45
|
351 | ... video_info: r.video_info.clone(),
| ^^^^^^^^^^^^^^^^^^^^ help: try removing the clone call: r.video_info
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#clone_on_copy
[warning] 344-344: using clone on type VideoInfo which implements the Copy trait
warning: using clone on type VideoInfo which implements the Copy trait
--> crates/recording/src/feeds/camera.rs:344:41
|
344 | ... video_info: r.video_info.clone(),
| ^^^^^^^^^^^^^^^^^^^^ help: try removing the clone call: r.video_info
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#clone_on_copy
= note: #[warn(clippy::clone_on_copy)] on by default
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (9)
apps/desktop/src/routes/(window-chrome)/new-main/index.tsx (3)
336-338: macOS drag spacer is a solid UX fixThe flex-1 spacer behind the traffic lights increases the draggable area without impacting interactive elements. Looks good.
352-352: Windows-specific left margin on the license pill — LGTMConditionally adding ml-2 on Windows aligns the badge with platform spacing conventions.
295-295: Removing the dir attribute: verify RTL/mirroring isn’t unintentionally affectedIf the previous dir attribute was compensating for platform-specific mirroring (especially on Windows) or future RTL locales, please sanity-check layout:
- Windows: header buttons order/spacing remains correct.
- macOS/Linux: no unexpected mirroring.
- Any RTL locale (if/when added): header content still reads left-to-right as intended.
If this attribute was only a workaround you no longer need, then all good—just confirming no regressions.
crates/recording/src/feeds/camera.rs (6)
28-34: LGTM! Clean actor-based architectureThe new actor-based design using Kameo provides better state management and concurrency control compared to the previous procedural approach. The state machine design with
State::OpenandState::Lockedis well-structured.
133-145: LGTM! Well-designed ID type with proper serializationThe
DeviceOrModelIDenum provides a clean abstraction for handling different camera identification methods, with proper serialization support for frontend integration.
218-302: LGTM! Robust camera setup with proper error handlingThe async
setup_camerafunction handles camera initialization well with timeout protection and comprehensive error handling. The format selection logic prioritizes ideal formats while falling back gracefully.
447-463: LGTM! Clean frame distribution with proper cleanupThe frame handling logic properly manages sender cleanup by removing disconnected senders, preventing memory leaks from accumulating dead channels.
479-516: LGTM! Well-implemented locking mechanismThe lock implementation properly handles the connecting state by awaiting readiness, and the automatic unlock via drop mechanism is elegant.
521-546: LGTM! Proper ready signal notificationThe
InputConnectedhandler correctly notifies all waiting listeners and clears the ready queue, ensuring proper synchronization.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
apps/desktop/src/routes/(window-chrome)/new-main/index.tsx (3)
342-344: Mac-only spacer looks good; ensures correct button cluster positioning on macOS.Conditionalizing the spacer fixes the Windows layout while preserving macOS spacing. Once the async OS resolution above is in place, switch to isMac() for consistency.
Suggested inline tweak for consistency:
- {ostype() === "macos" && ( + {isMac() && ( <div class="flex-1" data-tauri-drag-region /> )}
358-359: Use isWindows() instead of calling ostype() again; keep OS checks consistent.Minor consistency/readability improvement that also avoids re-invoking ostype().
- ostype() === "windows" && "ml-2", + isWindows() && "ml-2",
374-374: Fix user-facing typo: “Singning In...” → “Signing In...”.Small polish that users will see.
- <span>Singning In...</span> + <span>Signing In...</span>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/desktop/src/routes/(window-chrome)/new-main/index.tsx(3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/desktop/**/*.{ts,tsx}: In the desktop app, rely on unplugin-icons auto-imports; do not manually import icon modules
Use generated tauri_specta commands/events (commands, events) in the desktop frontend; listen to generated events directly
Use @tanstack/solid-query for server state in the desktop app
Files:
apps/desktop/src/routes/(window-chrome)/new-main/index.tsx
{apps/desktop,packages/ui-solid}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Component naming (Solid): components in PascalCase; hooks/utilities in camelCase starting with 'use' where applicable
Files:
apps/desktop/src/routes/(window-chrome)/new-main/index.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use strict TypeScript and avoid any; prefer shared types from packages
Files:
apps/desktop/src/routes/(window-chrome)/new-main/index.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (1)
apps/desktop/src/routes/(window-chrome)/new-main/index.tsx (1)
296-299: Suggestion Unneeded:ostype()Is Synchronous and Safe to Call InlineAccording to the official Tauri documentation, the JavaScript function
type()(aliased here asostype) returns anOsTypevalue synchronously—there is no Promise involved—so inline comparisons likeostype() === "macos"orostype() === "windows"are both correct and safe to execute repeatedly in JSX (tauri.app).
The original concern about “always-false comparisons” stems from a mistaken assumption that
ostype()returns a Promise; it does not.Performance-wise, calling a simple synchronous function like
ostype()multiple times incurs negligible cost; there’s no need to introduce async-safe resource logic.If you still prefer to centralize OS checks for consistency or readability, you can define a small utility in
src/utils/os.ts, for example:import { type as ostype } from "@tauri-apps/plugin-os"; export const isMac = () => ostype() === "macos"; export const isWindows = () => ostype() === "windows";and then use
isMac()/isWindows()inline—however, this is purely stylistic and not required for correctness.Likely an incorrect or invalid review comment.
Closes #909
TODO:
CameraFeedLockerror in the main windowSummary by CodeRabbit
New Features
Bug Fixes
Refactor
UX