Improve macOS recording startup by sharing ScreenCaptureKit warm-up #1088
Conversation
WalkthroughAdds macOS ScreenCaptureKit prewarming and a shareable-content cache with public async APIs; registers a RequestScreenCapturePrewarm event and prewarmer state; threads shareable content through recording builders and screen-capture sources; implements desktop thumbnail capture/collection and updates platform exports and macOS deps. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Window
participant App as Tauri App
participant Prewarmer as ScreenCapturePrewarmer (macOS)
participant SC as sc_shareable_content Cache
participant Recorder
participant ScreenCapture
User->>Window: open/show
Window->>App: emit "request-screen-capture-prewarm" (force=false)
App->>Prewarmer: handle event
Prewarmer->>SC: prewarm_shareable_content()
alt cache present
SC-->>Prewarmer: immediate OK
else warmup needed
SC->>SC: spawn/coordinate warmup (deduped)
SC-->>Prewarmer: notify on completion
end
User->>Recorder: start recording
Recorder->>SC: get_shareable_content()
SC-->>Recorder: ShareableContent (retained)
Recorder->>ScreenCapture: create_screen_capture(..., shareable_content)
ScreenCapture-->>Recorder: initialized
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
crates/recording/src/pipeline/builder.rs (1)
125-129: Consider OS-specific or configurable launch timeoutRaising to 15s helps macOS warm-up, but it may slow failure feedback on Windows. Consider gating by OS or making it configurable.
Apply this diff to keep 15s on macOS and 5s elsewhere:
- tokio::time::timeout(Duration::from_secs(15), task.ready_signal.recv_async()) + let launch_timeout = if cfg!(target_os = "macos") { 15 } else { 5 }; + tokio::time::timeout(Duration::from_secs(launch_timeout), task.ready_signal.recv_async())apps/desktop/src-tauri/src/windows.rs (1)
278-289: Avoid double-triggering prewarmYou both call prewarmer.request(false) and emit RequestScreenCapturePrewarm, which likely invokes the same code path again. The prewarmer dedupes, but this causes redundant work/logs. Prefer one path (local request is enough here).
Apply this diff to remove the redundant emit:
- if let Err(error) = (RequestScreenCapturePrewarm { force: false }).emit(app) { - warn!(%error, "Failed to emit ScreenCaptureKit prewarm event"); - }Based on learnings
crates/scap-targets/src/platform/macos.rs (1)
205-239: Minor: avoid redundant Vec re-collection.windows is already a Vec; you can pass a slice directly to ns::Array.
Apply this diff:
- let windows = windows.into_iter().collect::<Vec<_>>(); - let excluded_windows = ns::Array::from_slice_retained(windows.as_slice()); + let excluded_windows = ns::Array::from_slice_retained(&windows);crates/scap-targets/src/platform/macos/cache.rs (3)
54-61: Simplify by returning the expression directly.The
letbinding is unnecessary here.Apply this diff to simplify:
- let result = warmup - .result - .lock() - .await - .clone() - .expect("ScreenCaptureKit warmup task missing result"); - - result + warmup + .result + .lock() + .await + .clone() + .expect("ScreenCaptureKit warmup task missing result")
93-97: Collapse nested if statements.These nested if statements can be collapsed into a single if-let expression with a guard.
Apply this diff to simplify:
- if let Some(current) = guard.as_ref() { - if Arc::ptr_eq(¤t.notify, &task.notify) { - *guard = None; - } - } + if let Some(current) = guard.as_ref() + && Arc::ptr_eq(¤t.notify, &task.notify) { + *guard = None; + }
22-22: Consider simplifying the complex type with a type alias.The nested generic type is complex and could benefit from a type alias for better readability.
Add a type alias at the module level:
type WarmupResult = Result<(), arc::R<ns::Error>>;Then simplify the struct field:
struct WarmupTask { notify: Arc<Notify>, - result: Arc<Mutex<Option<Result<(), arc::R<ns::Error>>>>>, + result: Arc<Mutex<Option<WarmupResult>>>, }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
apps/desktop/src-tauri/src/lib.rs(5 hunks)apps/desktop/src-tauri/src/windows.rs(2 hunks)apps/desktop/src/utils/tauri.ts(3 hunks)crates/recording/src/capture_pipeline.rs(1 hunks)crates/recording/src/pipeline/builder.rs(1 hunks)crates/scap-targets/Cargo.toml(1 hunks)crates/scap-targets/src/lib.rs(1 hunks)crates/scap-targets/src/platform/macos.rs(4 hunks)crates/scap-targets/src/platform/macos/cache.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
crates/**/src/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
For desktop IPC, use tauri_specta derive/macros on Rust types/events; do not hand-roll bindings
Files:
crates/scap-targets/src/lib.rscrates/recording/src/pipeline/builder.rscrates/scap-targets/src/platform/macos/cache.rscrates/recording/src/capture_pipeline.rscrates/scap-targets/src/platform/macos.rs
**/*.{ts,tsx,js,jsx,rs}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not add inline, block, or docstring comments in any language; code must be self-explanatory
Files:
crates/scap-targets/src/lib.rsapps/desktop/src/utils/tauri.tscrates/recording/src/pipeline/builder.rsapps/desktop/src-tauri/src/windows.rscrates/scap-targets/src/platform/macos/cache.rscrates/recording/src/capture_pipeline.rsapps/desktop/src-tauri/src/lib.rscrates/scap-targets/src/platform/macos.rs
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Format Rust code usingrustfmtand ensure all Rust code passes workspace-level clippy lints.
Rust modules should be named with snake_case, and crate directories should be in kebab-case.
Files:
crates/scap-targets/src/lib.rscrates/recording/src/pipeline/builder.rsapps/desktop/src-tauri/src/windows.rscrates/scap-targets/src/platform/macos/cache.rscrates/recording/src/capture_pipeline.rsapps/desktop/src-tauri/src/lib.rscrates/scap-targets/src/platform/macos.rs
crates/*/src/**/*
📄 CodeRabbit inference engine (AGENTS.md)
Rust crates should place tests within the
src/and/or a siblingtests/directory for each crate insidecrates/*.
Files:
crates/scap-targets/src/lib.rscrates/recording/src/pipeline/builder.rscrates/scap-targets/src/platform/macos/cache.rscrates/recording/src/capture_pipeline.rscrates/scap-targets/src/platform/macos.rs
**/tauri.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Never edit auto-generated IPC bindings file tauri.ts
Do not edit auto-generated files named
tauri.ts.
Files:
apps/desktop/src/utils/tauri.ts
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/desktop/**/*.{ts,tsx}: Do not manually import icons in the desktop app; rely on auto-imported icons
In the desktop app, use @tanstack/solid-query for server state management
Files:
apps/desktop/src/utils/tauri.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use strict TypeScript and avoid any; leverage shared types from packages
**/*.{ts,tsx}: Use a 2-space indent for TypeScript code.
Use Biome for formatting and linting TypeScript/JavaScript files by runningpnpm format.
Files:
apps/desktop/src/utils/tauri.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Use kebab-case for filenames for TypeScript/JavaScript modules (e.g.,user-menu.tsx).
Use PascalCase for React/Solid components.
Files:
apps/desktop/src/utils/tauri.ts
🧠 Learnings (1)
📚 Learning: 2025-09-22T14:17:47.407Z
Learnt from: CR
PR: CapSoftware/Cap#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-22T14:17:47.407Z
Learning: Applies to crates/**/src/**/*.rs : For desktop IPC, use tauri_specta derive/macros on Rust types/events; do not hand-roll bindings
Applied to files:
crates/scap-targets/src/lib.rsapps/desktop/src-tauri/src/windows.rs
🧬 Code graph analysis (6)
crates/scap-targets/src/lib.rs (2)
crates/scap-targets/src/platform/macos.rs (1)
prewarm_shareable_content(22-24)crates/scap-targets/src/platform/macos/cache.rs (1)
prewarm_shareable_content(31-62)
apps/desktop/src-tauri/src/windows.rs (2)
apps/desktop/src-tauri/src/lib.rs (6)
tauri_specta(1929-2040)app(1213-1214)app(2295-2295)app(2326-2326)app(2363-2363)app(2369-2369)apps/desktop/src/utils/tauri.ts (1)
RequestScreenCapturePrewarm(437-437)
crates/scap-targets/src/platform/macos/cache.rs (1)
crates/scap-targets/src/platform/macos.rs (1)
prewarm_shareable_content(22-24)
crates/recording/src/capture_pipeline.rs (2)
crates/scap-targets/src/platform/macos.rs (1)
prewarm_shareable_content(22-24)crates/scap-targets/src/platform/macos/cache.rs (1)
prewarm_shareable_content(31-62)
apps/desktop/src-tauri/src/lib.rs (3)
apps/desktop/src/utils/tauri.ts (1)
RequestScreenCapturePrewarm(437-437)crates/scap-targets/src/platform/macos.rs (1)
prewarm_shareable_content(22-24)crates/scap-targets/src/platform/macos/cache.rs (2)
state(27-29)prewarm_shareable_content(31-62)
crates/scap-targets/src/platform/macos.rs (1)
crates/scap-targets/src/platform/macos/cache.rs (5)
prewarm_shareable_content(31-62)get_display(100-135)display(206-208)get_window(137-172)window(210-212)
🪛 GitHub Check: Clippy
crates/scap-targets/src/platform/macos/cache.rs
[warning] 93-97: this if statement can be collapsed
warning: this if statement can be collapsed
--> crates/scap-targets/src/platform/macos/cache.rs:93:5
|
93 | / if let Some(current) = guard.as_ref() {
94 | | if Arc::ptr_eq(¤t.notify, &task.notify) {
95 | | *guard = None;
96 | | }
97 | | }
| |_____^
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#collapsible_if
= note: #[warn(clippy::collapsible_if)] on by default
help: collapse nested if block
|
93 ~ if let Some(current) = guard.as_ref()
94 ~ && Arc::ptr_eq(¤t.notify, &task.notify) {
95 | *guard = None;
96 ~ }
|
[warning] 61-61: returning the result of a let binding from a block
warning: returning the result of a let binding from a block
--> crates/scap-targets/src/platform/macos/cache.rs:61:5
|
54 | / let result = warmup
55 | | .result
56 | | .lock()
57 | | .await
58 | | .clone()
59 | | .expect("ScreenCaptureKit warmup task missing result");
| |_______________________________________________________________- unnecessary let binding
60 |
61 | result
| ^^^^^^
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#let_and_return
= note: #[warn(clippy::let_and_return)] on by default
help: return the expression directly
|
54 ~
55 |
56 ~ warmup
57 + .result
58 + .lock()
59 + .await
60 + .clone()
61 + .expect("ScreenCaptureKit warmup task missing result")
|
[warning] 22-22: very complex type used. Consider factoring parts into type definitions
warning: very complex type used. Consider factoring parts into type definitions
--> crates/scap-targets/src/platform/macos/cache.rs:22:13
|
22 | result: Arc<Mutex<Option<Result<(), arc::Rns::Error>>>>,
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#type_complexity
= note: #[warn(clippy::type_complexity)] 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). (4)
- GitHub Check: Vercel Agent Review
- 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 (22)
crates/scap-targets/src/lib.rs (1)
5-6: macOS-only re-export looks goodThe conditional public re-export is clean and keeps non-macOS builds unaffected.
crates/recording/src/capture_pipeline.rs (1)
790-804: Prewarm before capture: LGTMThe macOS-only prewarm with trace/warn logging is correct and safely non-fatal.
apps/desktop/src-tauri/src/windows.rs (1)
19-24: Imports update is fineEvent trait import and expanded tracing are appropriate; unused-imports are already allowed at file top.
apps/desktop/src/utils/tauri.ts (2)
293-296: LGTM: event wiring added correctly (auto-generated file; do not edit manually).requestScreenCapturePrewarm is mapped with the expected kebab-case name. No action needed. As per coding guidelines, this file is auto-generated and should not be edited manually.
Also applies to: 315-316
437-437: LGTM: TS payload matches Rust event shape.force?: boolean aligns with #[serde(default)] pub force: bool. No action needed.
apps/desktop/src-tauri/src/lib.rs (7)
82-84: macOS‑gated import is correct.tokio::sync::Mutex is only pulled on macOS. Good guard.
325-330: LGTM: New event payload is minimal and forward‑compatible.#[serde(default)] on force matches the frontend’s optional field.
338-350: LGTM: Simple prewarm state machine and default initialization.State enum + Mutex-wrapped holder are appropriate here.
Also applies to: 351-358
2023-2024: LGTM: Event registered with specta; keeps TS bindings in sync.
2163-2164: LGTM: Prewarmer added to app state on macOS only.
2293-2298: LGTM: Event handler invokes prewarm; executes off the UI thread.listen_any_spawn already spawns; awaiting request here is fine.
360-411: Fix tracing field usage; current macro call likely doesn’t record elapsed_ms.Use key=value (or %/? shorthand) in tracing! fields to avoid compile/log issues.
Apply this diff:
@@ - Ok(()) => { - let elapsed_ms = warm_start.elapsed().as_micros() as f64 / 1000.0; - *state = PrewarmState::Warmed; - trace!(elapsed_ms, "ScreenCaptureKit cache warmed"); - } + Ok(()) => { + let elapsed_ms = warm_start.elapsed().as_micros() as f64 / 1000.0; + *state = PrewarmState::Warmed; + trace!(elapsed_ms = elapsed_ms, "ScreenCaptureKit cache warmed"); + }Likely an incorrect or invalid review comment.
crates/scap-targets/src/platform/macos.rs (4)
18-24: LGTM: Introduces cache module and public prewarm entrypoint.Good placement; logging via tracing is consistent with the rest of the crate.
184-195: LGTM: Display resolution now goes through warmed cache with error logging.Gracefully degrades to None on cache errors.
229-236: LGTM: Content filter creation includes timing trace; helpful for perf telemetry.
490-501: LGTM: Window resolution now uses cache with warnings on failure.Matches the display path; consistent behavior.
crates/scap-targets/src/platform/macos/cache.rs (6)
31-62: LGTM! Clean implementation of warmup orchestration.The function correctly handles the deduplication of in-flight warmup tasks and properly awaits their completion. The trace logging provides good observability for debugging.
64-98: LGTM! Robust warmup execution with proper cleanup.The warmup task correctly populates the cache, measures performance, and ensures cleanup of the warmup state. The error handling and logging are comprehensive.
100-135: LGTM! Efficient display lookup with fallback to warmup.The function implements an efficient cache-first lookup pattern with automatic fallback to warmup on cache miss. The timing instrumentation is helpful for performance monitoring.
137-172: LGTM! Consistent window lookup implementation.The window lookup function mirrors the display lookup pattern consistently, maintaining the same efficient cache-first approach with warmup fallback.
185-204: LGTM! Efficient cache structure with pre-built lookup maps.The
ShareableContentCacheimplementation efficiently pre-builds HashMap lookups during construction, avoiding repeated linear searches. The use ofretained()ensures proper reference counting for the cached objects.
182-183: Verify thread safety of ShareableContentCache contentsConfirm that
arc::R<sc::ShareableContent>,arc::R<sc::Display>, andarc::R<sc::Window>(and the underlying types) are allSend + Syncbefore unsafely impl-ing them onShareableContentCache. I couldn’t find anySend/Syncimpls for these in the code—please add explicit proofs or documentation.
There was a problem hiding this comment.
Actionable comments posted: 5
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/sources/screen_capture/macos.rs (1)
4-4: Import tracing macros used belowwarn! and error! are used later but not imported; add them to avoid unresolved macro errors.
-use tracing::{debug, info, trace}; +use tracing::{debug, info, trace, warn, error};As per coding guidelines.
crates/recording/src/lib.rs (1)
42-49: Gate macOS‐onlyshareable_contentin RecordingBaseInputs
shareable_contentreferences a macOS‐only type but isn’t cfg-gated inRecordingBaseInputs, leading to non-macOS build failures. Wrap it in#[cfg(target_os = "macos")]to align with downstream usage and avoid compilation errors.Apply:
--- crates/recording/src/lib.rs @@ pub struct RecordingBaseInputs { - pub shareable_content: cidre::arc::R<cidre::sc::ShareableContent>, + #[cfg(target_os = "macos")] + pub shareable_content: cidre::arc::R<cidre::sc::ShareableContent>, }
🧹 Nitpick comments (1)
apps/desktop/src-tauri/src/platform/macos/sc_shareable_content.rs (1)
92-126: Consider panic safety for in-flight warmup.If the async warmup block panics before line 114,
task.resultis never set andnotify_waitersis never called, causing awaiting tasks to hang indefinitely. Consider wrapping the warmup logic withcatch_unwindor ensuring that panics in ScreenCaptureKit APIs are handled gracefully.Apply this pattern to ensure result is always set:
async fn run_warmup(task: WarmupTask) { - let result = async { + let result = std::panic::AssertUnwindSafe(async { let warm_start = Instant::now(); debug!("Populating ScreenCaptureKit shareable content cache"); let content = sc::ShareableContent::current().await?; let cache = ShareableContentCache::new(content); let elapsed_ms = warm_start.elapsed().as_micros() as f64 / 1000.0; let mut guard = state().cache.write().unwrap(); let replaced = guard.is_some(); *guard = Some(cache); info!( elapsed_ms, replaced, "ScreenCaptureKit shareable content cache populated" ); Ok::<(), arc::R<ns::Error>>(()) - } - .await; + }) + .catch_unwind() + .await + .unwrap_or_else(|_| Err(/* convert panic to error */)); { let mut res_guard = task.result.lock().await; *res_guard = Some(result); } task.notify.notify_waiters(); let mut guard = state().warmup.lock().await; if let Some(current) = guard.as_ref() && Arc::ptr_eq(¤t.notify, &task.notify) { *guard = None; } }Alternatively, document that panics in ScreenCaptureKit are not expected and accept the hang as an edge case.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
apps/desktop/src-tauri/src/lib.rs(5 hunks)apps/desktop/src-tauri/src/platform/macos/mod.rs(1 hunks)apps/desktop/src-tauri/src/platform/macos/sc_shareable_content.rs(1 hunks)apps/desktop/src-tauri/src/recording.rs(3 hunks)crates/recording/src/capture_pipeline.rs(2 hunks)crates/recording/src/instant_recording.rs(3 hunks)crates/recording/src/lib.rs(1 hunks)crates/recording/src/pipeline/builder.rs(1 hunks)crates/recording/src/sources/screen_capture/macos.rs(1 hunks)crates/recording/src/sources/screen_capture/mod.rs(4 hunks)crates/recording/src/studio_recording.rs(6 hunks)crates/scap-targets/src/platform/macos.rs(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- crates/recording/src/studio_recording.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/recording/src/pipeline/builder.rs
- crates/recording/src/capture_pipeline.rs
🧰 Additional context used
📓 Path-based instructions (4)
crates/**/src/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
For desktop IPC, use tauri_specta derive/macros on Rust types/events; do not hand-roll bindings
Files:
crates/recording/src/sources/screen_capture/macos.rscrates/scap-targets/src/platform/macos.rscrates/recording/src/sources/screen_capture/mod.rscrates/recording/src/instant_recording.rscrates/recording/src/lib.rs
**/*.{ts,tsx,js,jsx,rs}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not add inline, block, or docstring comments in any language; code must be self-explanatory
Files:
crates/recording/src/sources/screen_capture/macos.rsapps/desktop/src-tauri/src/platform/macos/mod.rsapps/desktop/src-tauri/src/recording.rscrates/scap-targets/src/platform/macos.rscrates/recording/src/sources/screen_capture/mod.rscrates/recording/src/instant_recording.rsapps/desktop/src-tauri/src/lib.rscrates/recording/src/lib.rsapps/desktop/src-tauri/src/platform/macos/sc_shareable_content.rs
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Format Rust code usingrustfmtand ensure all Rust code passes workspace-level clippy lints.
Rust modules should be named with snake_case, and crate directories should be in kebab-case.
Files:
crates/recording/src/sources/screen_capture/macos.rsapps/desktop/src-tauri/src/platform/macos/mod.rsapps/desktop/src-tauri/src/recording.rscrates/scap-targets/src/platform/macos.rscrates/recording/src/sources/screen_capture/mod.rscrates/recording/src/instant_recording.rsapps/desktop/src-tauri/src/lib.rscrates/recording/src/lib.rsapps/desktop/src-tauri/src/platform/macos/sc_shareable_content.rs
crates/*/src/**/*
📄 CodeRabbit inference engine (AGENTS.md)
Rust crates should place tests within the
src/and/or a siblingtests/directory for each crate insidecrates/*.
Files:
crates/recording/src/sources/screen_capture/macos.rscrates/scap-targets/src/platform/macos.rscrates/recording/src/sources/screen_capture/mod.rscrates/recording/src/instant_recording.rscrates/recording/src/lib.rs
🧬 Code graph analysis (6)
crates/recording/src/sources/screen_capture/macos.rs (2)
apps/desktop/src-tauri/src/platform/macos/sc_shareable_content.rs (1)
display(160-162)crates/recording/src/sources/screen_capture/mod.rs (1)
display(67-73)
apps/desktop/src-tauri/src/recording.rs (3)
apps/desktop/src-tauri/src/platform/macos/sc_shareable_content.rs (1)
get_shareable_content(63-90)crates/recording/src/instant_recording.rs (1)
builder(154-156)crates/recording/src/studio_recording.rs (1)
builder(129-131)
crates/scap-targets/src/platform/macos.rs (2)
apps/desktop/src-tauri/src/platform/macos/sc_shareable_content.rs (2)
display(160-162)window(164-166)crates/scap-targets/src/lib.rs (1)
display(146-148)
crates/recording/src/instant_recording.rs (1)
apps/desktop/src-tauri/src/recording.rs (1)
inputs(66-71)
apps/desktop/src-tauri/src/lib.rs (2)
apps/desktop/src/utils/tauri.ts (1)
RequestScreenCapturePrewarm(437-437)apps/desktop/src-tauri/src/platform/macos/sc_shareable_content.rs (2)
state(28-30)prewarm_shareable_content(32-61)
apps/desktop/src-tauri/src/platform/macos/sc_shareable_content.rs (3)
crates/recording/src/sources/screen_capture/mod.rs (1)
display(67-73)crates/scap-targets/src/platform/macos.rs (4)
display(466-486)Self(279-288)id(45-45)id(295-297)crates/scap-targets/src/lib.rs (3)
display(146-148)id(28-30)id(118-120)
⏰ 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: Vercel Agent Review
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (21)
apps/desktop/src-tauri/src/platform/macos/sc_shareable_content.rs (5)
1-11: LGTM!Imports are appropriate for implementing a cached, async-aware ScreenCaptureKit prewarming layer.
12-30: LGTM!The cache state and warmup coordination structures correctly enable single in-flight warmup with multiple awaiters via
Notify, and safe concurrent cache access viaRwLock.
139-167: LGTM!The
ShareableContentCacheimplementation correctly builds lookup tables for displays and windows, enabling efficient O(1) access by ID.
32-61: run_warmup sets result before notify
The code storesresultintask.resultunder lock before callingtask.notify.notify_waiters(), so theexpectinprewarm_shareable_contentcannot panic due to a missing result.
63-90: Remove stale concern about Ok(None) after prewarm
prewarm_shareable_content()only returnsOk(())oncerun_warmuphas populatedstate().cachevia*guard = Some(cache);, so the second read inget_shareable_content()can never yieldNoneafter a successful prewarm.Likely an incorrect or invalid review comment.
crates/scap-targets/src/platform/macos.rs (4)
177-183: LGTM!Refactoring
as_scto accept explicitcontentparameter enables reuse of prewarmed shareable content and avoids redundant ScreenCaptureKit queries.
185-191: LGTM!Delegating to
as_content_filter_excluding_windowswith an empty window list is correct and maintains backward compatibility.
193-208: LGTM!The refactored
as_content_filter_excluding_windowscorrectly resolves the display from the provided content and constructs the content filter with excluded windows.
458-464: LGTM!Refactoring
as_scforWindowImplto accept explicitcontentaligns with the display implementation and enables efficient reuse of cached shareable content.apps/desktop/src-tauri/src/platform/macos/mod.rs (1)
14-16: LGTM!The module declaration and public re-export correctly expose the shareable content prewarming API to the rest of the macOS platform codebase.
apps/desktop/src-tauri/src/recording.rs (2)
409-418: LGTM: Studio builder correctly forwards shareable_content under macOSPassing the extra param behind cfg matches the builder signature change and keeps other platforms unaffected.
382-387: LGTM: macOS-only content fetch is correctly gated and error-mapped
Confirm CI passes for non-macOS builds after the lib.rs cfg update.crates/recording/src/sources/screen_capture/mod.rs (1)
202-204: Approve macOS-only ShareableContent integrationThe cfg-gated field is properly propagated across init, clone, and invocation sites and is consumed in
src/sources/screen_capture/macos.rsviaShareableContent::current().await. No dead code remains after this refactor.crates/recording/src/instant_recording.rs (2)
186-202: LGTM! Shareable content parameter correctly gated and propagated.The macOS-only
shareable_contentparameter is properly added to thebuildsignature with#[cfg(target_os = "macos")], forwarded intoRecordingBaseInputs, and correctly threaded through the actor spawn path.
244-244: No action needed: the.retained()call returnscidre::arc::R<cidre::sc::ShareableContent>, matching thecreate_screen_captureparameter.apps/desktop/src-tauri/src/lib.rs (6)
82-83: LGTM! Conditional Mutex import for macOS.The
Muteximport fromtokio::syncis correctly gated with#[cfg(target_os = "macos")]for the prewarmer state machine.
325-329: LGTM! Event payload correctly defined with tauri_specta.The
RequestScreenCapturePrewarmstruct uses proper derive macros for Tauri IPC and includes aforcefield with serde default, aligning with the coding guidelines for desktop IPC.
2162-2163: LGTM! ScreenCapturePrewarmer managed in app setup.The
ScreenCapturePrewarmeris correctly initialized and managed in the Tauri app state, gated with#[cfg(target_os = "macos")].
2023-2023: LGTM! Event registered in specta.The
RequestScreenCapturePrewarmevent is correctly added to thecollect_events!macro for TypeScript type generation.
2293-2297: LGTM! Event handler correctly wired.The event handler retrieves the
ScreenCapturePrewarmerfrom app state and invokesrequest(event.force).await, properly integrating the prewarm trigger into the app lifecycle.
338-411: Clarify force flag and concurrent request semantics
force:trueresets only the localPrewarmState;prewarm_shareable_contentuses a sharedwarmupmutex to join any in-flight warmup rather than abandoning it.request(false)duringWarmingreturns immediately (non-blocking) by design. Callers that need to await the warmed cache should callrequest(true)or synchronize explicitly on the shared cache.
| #[derive(Debug)] | ||
| struct ShareableContentCache { | ||
| #[allow(dead_code)] | ||
| content: arc::R<sc::ShareableContent>, | ||
| displays: HashMap<CGDirectDisplayID, arc::R<sc::Display>>, | ||
| windows: HashMap<CGWindowID, arc::R<sc::Window>>, | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove incorrect #[allow(dead_code)] on content field.
The content field is used on line 72 (v.content.retained()), so the #[allow(dead_code)] attribute is incorrect.
Apply this diff:
#[derive(Debug)]
struct ShareableContentCache {
- #[allow(dead_code)]
content: arc::R<sc::ShareableContent>,
displays: HashMap<CGDirectDisplayID, arc::R<sc::Display>>,
windows: HashMap<CGWindowID, arc::R<sc::Window>>,
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[derive(Debug)] | |
| struct ShareableContentCache { | |
| #[allow(dead_code)] | |
| content: arc::R<sc::ShareableContent>, | |
| displays: HashMap<CGDirectDisplayID, arc::R<sc::Display>>, | |
| windows: HashMap<CGWindowID, arc::R<sc::Window>>, | |
| } | |
| #[derive(Debug)] | |
| struct ShareableContentCache { | |
| content: arc::R<sc::ShareableContent>, | |
| displays: HashMap<CGDirectDisplayID, arc::R<sc::Display>>, | |
| windows: HashMap<CGWindowID, arc::R<sc::Window>>, | |
| } |
🤖 Prompt for AI Agents
In apps/desktop/src-tauri/src/platform/macos/sc_shareable_content.rs around
lines 128 to 134, the ShareableContentCache.content field is incorrectly
annotated with #[allow(dead_code)] despite being used elsewhere
(v.content.retained() at line 72); remove the #[allow(dead_code)] attribute from
the content field so the field's usage is correctly recognized by the compiler
and linter.
| unsafe impl Send for ShareableContentCache {} | ||
| unsafe impl Sync for ShareableContentCache {} |
There was a problem hiding this comment.
🧩 Analysis chain
Verify arc::R is thread-safe before manual Send/Sync impls.
Manual unsafe impl Send and Sync for ShareableContentCache assume that arc::R<sc::ShareableContent>, arc::R<sc::Display>, and arc::R<sc::Window> are thread-safe. Verify that cidre::arc::R behaves like std::sync::Arc and is safe to send/share across threads.
🌐 Web query:
Is cidre::arc::R thread-safe and similar to std::sync::Arc in Rust?
💡 Result:
Short answer: No.
Details:
- In the cidre crate arc module, R is a type alias for Retained, a thin wrapper around a &'static mut T that calls Objective‑C retain/release (the module defines Retained/Allocated and pub type R = Retained). This is an Obj‑C ownership helper, not an atomically reference‑counted, thread‑safe pointer. [1]
- std::sync::Arc is an atomically reference‑counted, thread‑safe shared pointer (atomic refcounting; implements Send/Sync when T does). R in cidre does not provide that behavior. [2]
If you want thread‑safe shared ownership in Rust use std::sync::Arc (or wrap cidre types in synchronization primitives yourself).
Sources:
[1] cidre crate arc.rs (defines Retained/Allocated and pub type R = Retained). (docs.rs)
[2] std::sync::Arc documentation (thread‑safe, atomically reference counted). (doc.rust-lang.org)
Citations:
- 1: https://docs.rs/crate/cidre/latest/source/src/arc.rs
- 2: https://doc.rust-lang.org/std/sync/struct.Arc.html?utm_source=openai
Remove manual unsafe impl Send/Sync for ShareableContentCache (apps/desktop/src-tauri/src/platform/macos/sc_shareable_content.rs lines 136–137): cidre::arc::R is not thread-safe. Use std::sync::Arc or wrap the contents in proper synchronization primitives for cross-thread sharing.
🤖 Prompt for AI Agents
In apps/desktop/src-tauri/src/platform/macos/sc_shareable_content.rs around
lines 136–137, remove the manual unsafe impl Send/Sync for ShareableContentCache
because cidre::arc::R is not thread-safe; instead make the cache use thread-safe
primitives (for example replace cidre::arc::R with std::sync::Arc if the inner
type is Send + Sync, or wrap the inner data in std::sync::Mutex/RwLock/Arc to
ensure proper synchronization), delete the unsafe impls, and adjust types/usages
so ShareableContentCache is only shared across threads via these safe, standard
synchronization primitives.
| }; | ||
|
|
||
| use crate::bounds::{LogicalBounds, LogicalPosition, LogicalSize, PhysicalSize}; | ||
| use tracing::trace; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
❓ Verification inconclusive
Verify tracing::trace is used.
The trace import is added but no trace!() macro calls are visible in the provided code. Confirm this import is necessary or remove it if unused.
🏁 Script executed:
#!/bin/bash
# Description: Check for trace! macro usage in this file.
rg -n 'trace!' crates/scap-targets/src/platform/macos.rsLength of output: 78
🏁 Script executed:
#!/bin/bash
# Check for any trace!() macro calls in the macOS platform file
rg -nF 'trace!(' crates/scap-targets/src/platform/macos.rsLength of output: 0
Remove unused import
Drop use tracing::trace; from crates/scap-targets/src/platform/macos.rs (no trace!() calls present).
🤖 Prompt for AI Agents
In crates/scap-targets/src/platform/macos.rs around line 18, the file imports
tracing::trace but does not use trace!() anywhere; remove the unused import line
`use tracing::trace;` from the file to eliminate the dead import and any
unused-import lint warnings.
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
| let content = sc::ShareableContent::current() | ||
| .await | ||
| .map_err(|e| SourceError::CreateActor(e))?; |
There was a problem hiding this comment.
The screen capture source is still calling sc::ShareableContent::current() directly instead of using the cached shareable_content parameter, completely defeating the caching optimization.
View Details
📝 Patch Details
diff --git a/crates/recording/src/sources/screen_capture/macos.rs b/crates/recording/src/sources/screen_capture/macos.rs
index 98131d4c..7c0d2110 100644
--- a/crates/recording/src/sources/screen_capture/macos.rs
+++ b/crates/recording/src/sources/screen_capture/macos.rs
@@ -128,6 +128,7 @@ impl PipelineSourceTask for ScreenCaptureSource<CMSampleBufferCapture> {
let video_tx = self.video_tx.clone();
let audio_tx = self.audio_tx.clone();
let config = self.config.clone();
+ let shareable_content = self.shareable_content.clone();
self.tokio_handle
.block_on(async move {
@@ -137,9 +138,7 @@ impl PipelineSourceTask for ScreenCaptureSource<CMSampleBufferCapture> {
let display = Display::from_id(&config.display)
.ok_or_else(|| SourceError::NoDisplay(config.display))?;
- let content = sc::ShareableContent::current()
- .await
- .map_err(|e| SourceError::CreateActor(e))?;
+ let content = shareable_content;
let content_filter = display
.raw_handle()
Analysis
Screen capture bypasses SCK caching system, causing 10s recording startup delays
What fails: ScreenCaptureSource::run() method in crates/recording/src/sources/screen_capture/macos.rs calls sc::ShareableContent::current().await (lines 140-142) instead of using the cached self.shareable_content field, completely defeating the SCK caching optimization.
How to reproduce:
- Initialize
ScreenCaptureSourcewith a prewarmedshareable_contentparameter - Call
PipelineSourceTask::run()to start recording - Observe that the expensive SCK lookup happens every time instead of using the cached content
Result: Recording startup takes ~10 seconds each time due to ShareableContent::current().await call. The prewarmed cache in self.shareable_content is never used.
Expected: Should use the cached shareable_content field to achieve instant recording startup as intended by the caching system.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
crates/scap-targets/src/platform/macos.rs (1)
23-23: Remove unused import (duplicate).
use tracing::trace;is unused in this file. Drop it to satisfy clippy/rustc lints.-use tracing::trace;
🧹 Nitpick comments (2)
crates/scap-targets/src/platform/macos.rs (1)
190-196: Avoid moving a Vec; accept a slice for windows.Pass windows as a slice to avoid needless allocation/moves and make call sites cheaper.
- pub async fn as_content_filter( - &self, - content: &sc::ShareableContent, - ) -> Option<arc::R<sc::ContentFilter>> { - self.as_content_filter_excluding_windows(content, vec![]) + pub async fn as_content_filter( + &self, + content: &sc::ShareableContent, + ) -> Option<arc::R<sc::ContentFilter>> { + self.as_content_filter_excluding_windows(content, &[]) .await } - pub async fn as_content_filter_excluding_windows( + pub async fn as_content_filter_excluding_windows( &self, - content: &sc::ShareableContent, - windows: Vec<arc::R<sc::Window>>, + content: &sc::ShareableContent, + windows: &[arc::R<sc::Window>], ) -> Option<arc::R<sc::ContentFilter>> { let display = content .displays() .iter() .find(|display| display.display_id().0 == self.0.id) .map(|display| display.retained())?; Some(sc::ContentFilter::with_display_excluding_windows( display.as_ref(), - &ns::Array::from_slice_retained(&windows), + &ns::Array::from_slice_retained(windows), )) }Also applies to: 198-213
apps/desktop/src-tauri/src/windows.rs (1)
281-293: Avoid double prewarm; emit first, fallback to direct call on error.Currently you both start prewarm and emit an event that does the same. Keep one path to prevent redundant work.
- #[cfg(target_os = "macos")] - { - let app_handle = app.clone(); - tauri::async_runtime::spawn(async move { - let prewarmer = app_handle.state::<ScreenCapturePrewarmer>(); - prewarmer.request(false).await; - }); - - if let Err(error) = (RequestScreenCapturePrewarm { force: false }).emit(app) { - warn!(%error, "Failed to emit ScreenCaptureKit prewarm event"); - } - } + #[cfg(target_os = "macos")] + { + if let Err(error) = (RequestScreenCapturePrewarm { force: false }).emit(app) { + warn!(%error, "Failed to emit ScreenCaptureKit prewarm event; falling back"); + let app_handle = app.clone(); + tauri::async_runtime::spawn(async move { + let prewarmer = app_handle.state::<ScreenCapturePrewarmer>(); + prewarmer.request(false).await; + }); + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
apps/desktop/src-tauri/src/lib.rs(5 hunks)apps/desktop/src-tauri/src/recording.rs(3 hunks)apps/desktop/src-tauri/src/windows.rs(2 hunks)apps/desktop/src/utils/tauri.ts(3 hunks)crates/scap-targets/Cargo.toml(1 hunks)crates/scap-targets/src/platform/macos.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/scap-targets/Cargo.toml
- apps/desktop/src-tauri/src/recording.rs
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{ts,tsx,js,jsx,rs}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not add inline, block, or docstring comments in any language; code must be self-explanatory
Files:
apps/desktop/src-tauri/src/lib.rsapps/desktop/src/utils/tauri.tscrates/scap-targets/src/platform/macos.rsapps/desktop/src-tauri/src/windows.rs
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Format Rust code usingrustfmtand ensure all Rust code passes workspace-level clippy lints.
Rust modules should be named with snake_case, and crate directories should be in kebab-case.
Files:
apps/desktop/src-tauri/src/lib.rscrates/scap-targets/src/platform/macos.rsapps/desktop/src-tauri/src/windows.rs
**/tauri.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Never edit auto-generated IPC bindings file tauri.ts
Do not edit auto-generated files named
tauri.ts.
Files:
apps/desktop/src/utils/tauri.ts
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/desktop/**/*.{ts,tsx}: Do not manually import icons in the desktop app; rely on auto-imported icons
In the desktop app, use @tanstack/solid-query for server state management
Files:
apps/desktop/src/utils/tauri.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use strict TypeScript and avoid any; leverage shared types from packages
**/*.{ts,tsx}: Use a 2-space indent for TypeScript code.
Use Biome for formatting and linting TypeScript/JavaScript files by runningpnpm format.
Files:
apps/desktop/src/utils/tauri.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Use kebab-case for filenames for TypeScript/JavaScript modules (e.g.,user-menu.tsx).
Use PascalCase for React/Solid components.
Files:
apps/desktop/src/utils/tauri.ts
crates/**/src/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
For desktop IPC, use tauri_specta derive/macros on Rust types/events; do not hand-roll bindings
Files:
crates/scap-targets/src/platform/macos.rs
crates/*/src/**/*
📄 CodeRabbit inference engine (AGENTS.md)
Rust crates should place tests within the
src/and/or a siblingtests/directory for each crate insidecrates/*.
Files:
crates/scap-targets/src/platform/macos.rs
🧠 Learnings (1)
📚 Learning: 2025-09-22T14:17:47.407Z
Learnt from: CR
PR: CapSoftware/Cap#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-22T14:17:47.407Z
Learning: Applies to crates/**/src/**/*.rs : For desktop IPC, use tauri_specta derive/macros on Rust types/events; do not hand-roll bindings
Applied to files:
apps/desktop/src-tauri/src/windows.rs
🧬 Code graph analysis (3)
apps/desktop/src-tauri/src/lib.rs (2)
apps/desktop/src-tauri/src/platform/macos/sc_shareable_content.rs (2)
state(28-30)prewarm_shareable_content(32-61)apps/desktop/src-tauri/src/windows.rs (5)
app(215-215)app(357-357)app(447-447)app(747-747)app(936-936)
crates/scap-targets/src/platform/macos.rs (2)
apps/desktop/src-tauri/src/platform/macos/sc_shareable_content.rs (2)
display(160-162)window(164-166)crates/scap-targets/src/lib.rs (1)
display(146-148)
apps/desktop/src-tauri/src/windows.rs (1)
apps/desktop/src-tauri/src/lib.rs (6)
tauri_specta(1929-2043)app(1213-1214)app(2298-2298)app(2329-2329)app(2366-2366)app(2372-2372)
⏰ 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). (4)
- GitHub Check: Clippy
- 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 (3)
apps/desktop/src/utils/tauri.ts (1)
302-305: Event/type wiring looks correct; no action needed.New RequestScreenCapturePrewarm event and payload are correctly exported and mapped. Note: this file is auto-generated; do not edit manually.
Also applies to: 324-326, 448-449
crates/scap-targets/src/platform/macos.rs (1)
181-188: LGTM: content-context threading.Switching to content-parameterized lookups for displays/windows is clear and aligns with the shared ShareableContent cache.
Also applies to: 479-485
apps/desktop/src-tauri/src/lib.rs (1)
325-330: LGTM: event surface and wiring.New RequestScreenCapturePrewarm event, managed state, and listener hookup look correct and are macOS-gated appropriately.
Also applies to: 2026-2026, 2165-2167, 2296-2301
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
apps/cli/Cargo.toml(1 hunks)apps/cli/src/record.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx,rs}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not add inline, block, or docstring comments in any language; code must be self-explanatory
Files:
apps/cli/src/record.rs
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Format Rust code usingrustfmtand ensure all Rust code passes workspace-level clippy lints.
Rust modules should be named with snake_case, and crate directories should be in kebab-case.
Files:
apps/cli/src/record.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Clippy
- 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 (1)
apps/cli/Cargo.toml (1)
27-28: macOS-only dependency looks good.Target-scoping
cidrekeeps non-mac builds clean while enabling the new ScreenCaptureKit path. No concerns on my end.
| .build( | ||
| #[cfg(target_os = "macos")] | ||
| cidre::sc::ShareableContent::current().await.unwrap(), | ||
| ) | ||
| .await | ||
| .map_err(|e| e.to_string())?; |
There was a problem hiding this comment.
Propagate ShareableContent errors instead of panicking.
The unwrap() at line 66 will panic if ShareableContent::current() fails, crashing the CLI without user-friendly context. Since the outer await and map_err (lines 68-69) already handle build() errors, propagate the ShareableContent error through the same path.
Apply this diff:
.build(
#[cfg(target_os = "macos")]
- cidre::sc::ShareableContent::current().await.unwrap(),
+ cidre::sc::ShareableContent::current()
+ .await
+ .map_err(|e| format!("Failed to get shareable content: {}", e))?,
)
.await
.map_err(|e| e.to_string())?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .build( | |
| #[cfg(target_os = "macos")] | |
| cidre::sc::ShareableContent::current().await.unwrap(), | |
| ) | |
| .await | |
| .map_err(|e| e.to_string())?; | |
| .build( | |
| #[cfg(target_os = "macos")] | |
| cidre::sc::ShareableContent::current() | |
| .await | |
| .map_err(|e| format!("Failed to get shareable content: {}", e))?, | |
| ) | |
| .await | |
| .map_err(|e| e.to_string())?; |
🤖 Prompt for AI Agents
In apps/cli/src/record.rs around lines 64 to 69, the code uses unwrap() on
ShareableContent::current() which can panic; replace the unwrap with error
propagation so the ShareableContent error flows through the existing map_err
path. Concretely, change the macOS branch to call
cidre::sc::ShareableContent::current().await.map_err(|e| e.to_string())? (i.e.
await, map the error to a string and use ?), so the failing ShareableContent
creation is returned instead of panicking.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
apps/desktop/src-tauri/src/platform/macos/sc_shareable_content.rs (1)
136-137: Revert the unsafe Send/Sync impl; it’s unsound.
cidre::arc::Rintentionally does not implementSend/Sync, because the wrapped ScreenCaptureKit objects are only guaranteed to be safe on the thread they were created on. ForcingShareableContentCacheto beSend/Syncso it can survive inside atokio::spawnmoves those Objective‑C objects across the multi-threaded runtime and can trigger undefined behaviour at runtime. Please drop the unsafe impls and keep the warm-up future on the originating thread (e.g. runrun_warmupdirectly in the first caller, or switch to aspawn_local/main-thread executor) so no cross-thread transfer is needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/desktop/src-tauri/src/lib.rs(6 hunks)apps/desktop/src-tauri/src/platform/macos/sc_shareable_content.rs(1 hunks)apps/desktop/src-tauri/src/recording.rs(7 hunks)apps/desktop/src-tauri/src/thumbnails/mac.rs(1 hunks)apps/desktop/src-tauri/src/thumbnails/mod.rs(1 hunks)apps/desktop/src-tauri/src/thumbnails/windows.rs(1 hunks)apps/desktop/src-tauri/src/windows.rs(2 hunks)crates/recording/src/sources/screen_capture/macos.rs(2 hunks)crates/scap-targets/src/platform/macos.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src-tauri/src/lib.rs
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,js,jsx,rs}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not add inline, block, or docstring comments in any language; code must be self-explanatory
Files:
apps/desktop/src-tauri/src/thumbnails/windows.rsapps/desktop/src-tauri/src/windows.rsapps/desktop/src-tauri/src/thumbnails/mod.rscrates/recording/src/sources/screen_capture/macos.rscrates/scap-targets/src/platform/macos.rsapps/desktop/src-tauri/src/platform/macos/sc_shareable_content.rsapps/desktop/src-tauri/src/recording.rsapps/desktop/src-tauri/src/thumbnails/mac.rs
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Format Rust code usingrustfmtand ensure all Rust code passes workspace-level clippy lints.
Rust modules should be named with snake_case, and crate directories should be in kebab-case.
Files:
apps/desktop/src-tauri/src/thumbnails/windows.rsapps/desktop/src-tauri/src/windows.rsapps/desktop/src-tauri/src/thumbnails/mod.rscrates/recording/src/sources/screen_capture/macos.rscrates/scap-targets/src/platform/macos.rsapps/desktop/src-tauri/src/platform/macos/sc_shareable_content.rsapps/desktop/src-tauri/src/recording.rsapps/desktop/src-tauri/src/thumbnails/mac.rs
crates/**/src/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
For desktop IPC, use tauri_specta derive/macros on Rust types/events; do not hand-roll bindings
Files:
crates/recording/src/sources/screen_capture/macos.rscrates/scap-targets/src/platform/macos.rs
crates/*/src/**/*
📄 CodeRabbit inference engine (AGENTS.md)
Rust crates should place tests within the
src/and/or a siblingtests/directory for each crate insidecrates/*.
Files:
crates/recording/src/sources/screen_capture/macos.rscrates/scap-targets/src/platform/macos.rs
🧬 Code graph analysis (7)
apps/desktop/src-tauri/src/thumbnails/windows.rs (3)
apps/desktop/src-tauri/src/thumbnails/mac.rs (2)
capture_display_thumbnail(7-12)capture_window_thumbnail(14-20)crates/scap-direct3d/src/lib.rs (1)
stride(543-545)apps/desktop/src-tauri/src/thumbnails/mod.rs (1)
normalize_thumbnail_dimensions(38-81)
apps/desktop/src-tauri/src/windows.rs (2)
apps/desktop/src-tauri/src/lib.rs (6)
tauri_specta(1855-1969)app(1139-1140)app(2224-2224)app(2255-2255)app(2292-2292)app(2298-2298)apps/desktop/src/utils/tauri.ts (1)
RequestScreenCapturePrewarm(448-448)
apps/desktop/src-tauri/src/thumbnails/mod.rs (4)
crates/recording/src/sources/screen_capture/mod.rs (2)
list_displays(438-452)list_windows(454-492)apps/desktop/src/utils/tauri.ts (5)
CaptureDisplayWithThumbnail(365-365)DisplayId(381-381)CaptureWindowWithThumbnail(367-367)WindowId(477-477)LogicalBounds(410-410)apps/desktop/src-tauri/src/thumbnails/mac.rs (3)
new(284-290)capture_display_thumbnail(7-12)capture_window_thumbnail(14-20)apps/desktop/src-tauri/src/thumbnails/windows.rs (2)
capture_display_thumbnail(1-138)capture_window_thumbnail(140-277)
crates/scap-targets/src/platform/macos.rs (1)
apps/desktop/src-tauri/src/platform/macos/sc_shareable_content.rs (2)
display(160-162)window(164-166)
apps/desktop/src-tauri/src/platform/macos/sc_shareable_content.rs (2)
crates/scap-targets/src/platform/macos.rs (3)
display(490-510)id(50-50)id(303-305)crates/scap-targets/src/lib.rs (3)
display(146-148)id(28-30)id(118-120)
apps/desktop/src-tauri/src/recording.rs (5)
apps/desktop/src/utils/tauri.ts (2)
CaptureDisplayWithThumbnail(365-365)CaptureWindowWithThumbnail(367-367)apps/desktop/src-tauri/src/thumbnails/mod.rs (2)
collect_displays_with_thumbnails(83-99)collect_windows_with_thumbnails(101-149)apps/desktop/src-tauri/src/platform/macos/sc_shareable_content.rs (1)
get_shareable_content(63-90)crates/recording/src/instant_recording.rs (1)
builder(154-156)crates/recording/src/studio_recording.rs (1)
builder(129-131)
apps/desktop/src-tauri/src/thumbnails/mac.rs (2)
apps/desktop/src-tauri/src/platform/macos/sc_shareable_content.rs (1)
get_shareable_content(63-90)apps/desktop/src-tauri/src/thumbnails/mod.rs (1)
normalize_thumbnail_dimensions(38-81)
⏰ 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). (4)
- GitHub Check: Clippy
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Analyze (rust)
| pub fn list_displays_with_thumbnails() -> Result<Vec<CaptureDisplayWithThumbnail>, String> { | ||
| tauri::async_runtime::block_on(collect_displays_with_thumbnails()) | ||
| } | ||
|
|
||
| #[tauri::command(async)] | ||
| #[specta::specta] | ||
| pub async fn list_windows_with_thumbnails() -> Result<Vec<CaptureWindowWithThumbnail>, String> { | ||
| tokio::task::spawn_blocking(|| collect_windows_with_thumbnails()) | ||
| .await | ||
| .map_err(|err| err.to_string())? | ||
| } | ||
|
|
||
| fn collect_displays_with_thumbnails() -> Result<Vec<CaptureDisplayWithThumbnail>, String> { | ||
| tauri::async_runtime::block_on(async move { | ||
| let displays = screen_capture::list_displays(); | ||
|
|
||
| let mut results = Vec::new(); | ||
| for (capture_display, display) in displays { | ||
| let thumbnail = capture_display_thumbnail(&display).await; | ||
| results.push(CaptureDisplayWithThumbnail { | ||
| id: capture_display.id, | ||
| name: capture_display.name, | ||
| refresh_rate: capture_display.refresh_rate, | ||
| thumbnail, | ||
| }); | ||
| } | ||
|
|
||
| Ok(results) | ||
| }) | ||
| } | ||
|
|
||
| fn collect_windows_with_thumbnails() -> Result<Vec<CaptureWindowWithThumbnail>, String> { | ||
| tauri::async_runtime::block_on(async move { | ||
| let windows = screen_capture::list_windows(); | ||
|
|
||
| debug!(window_count = windows.len(), "Collecting window thumbnails"); | ||
| let mut results = Vec::new(); | ||
| for (capture_window, window) in windows { | ||
| let thumbnail = capture_window_thumbnail(&window).await; | ||
| let app_icon = window.app_icon().and_then(|bytes| { | ||
| if bytes.is_empty() { | ||
| None | ||
| } else { | ||
| Some(base64::Engine::encode( | ||
| &base64::engine::general_purpose::STANDARD, | ||
| bytes, | ||
| )) | ||
| } | ||
| }); | ||
|
|
||
| if thumbnail.is_none() { | ||
| warn!( | ||
| window_id = ?capture_window.id, | ||
| window_name = %capture_window.name, | ||
| owner_name = %capture_window.owner_name, | ||
| "Window thumbnail capture returned None", | ||
| ); | ||
| } else { | ||
| debug!( | ||
| window_id = ?capture_window.id, | ||
| window_name = %capture_window.name, | ||
| owner_name = %capture_window.owner_name, | ||
| "Captured window thumbnail", | ||
| ); | ||
| } | ||
|
|
||
| results.push(CaptureWindowWithThumbnail { | ||
| id: capture_window.id, | ||
| name: capture_window.name, | ||
| owner_name: capture_window.owner_name, | ||
| bounds: capture_window.bounds, | ||
| refresh_rate: capture_window.refresh_rate, | ||
| thumbnail, | ||
| app_icon, | ||
| }); | ||
| } | ||
|
|
||
| info!(windows = results.len(), "Collected window thumbnail data"); | ||
|
|
||
| Ok(results) | ||
| }) | ||
| tauri::async_runtime::block_on(collect_windows_with_thumbnails()) | ||
| } |
There was a problem hiding this comment.
Avoid blocking the Tauri async runtime in command handlers.
tauri::async_runtime::block_on is being invoked from within an async command, which risks panicking (“cannot block the current thread”) or stalling the runtime thread under Tokio. The command itself is already async, so we should just await the collectors directly.
-#[tauri::command(async)]
-pub fn list_displays_with_thumbnails() -> Result<Vec<CaptureDisplayWithThumbnail>, String> {
- tauri::async_runtime::block_on(collect_displays_with_thumbnails())
-}
+#[tauri::command(async)]
+pub async fn list_displays_with_thumbnails() -> Result<Vec<CaptureDisplayWithThumbnail>, String> {
+ collect_displays_with_thumbnails().await
+}
@@
pub async fn list_windows_with_thumbnails() -> Result<Vec<CaptureWindowWithThumbnail>, String> {
- tauri::async_runtime::block_on(collect_windows_with_thumbnails())
+ collect_windows_with_thumbnails().await
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/desktop/src-tauri/src/recording.rs around lines 197 to 205, the code
calls tauri::async_runtime::block_on inside command handlers (and a non-async
function), which can block or panic the runtime; change both functions to be
async and simply await the collector futures (e.g., make
list_displays_with_thumbnails async and remove block_on, and in
list_windows_with_thumbnails replace block_on with await), keep the existing
return types and attributes (add #[tauri::command(async)]/#[specta::specta] to
the displays function if needed), and remove the blocking call so the collectors
run on the async runtime without blocking the current thread.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/desktop/src-tauri/src/recording.rs (1)
195-213: Avoid nested blocking in async command handlers.Both
list_displays_with_thumbnailsandlist_windows_with_thumbnailsusespawn_blockingwrappingblock_on, which is an anti-pattern. Since these commands are already async-capable and the collector functions are async, the handlers should simply await them directly.This issue was previously flagged. Apply this diff to fix both functions:
-#[tauri::command] +#[tauri::command(async)] #[specta::specta] pub async fn list_displays_with_thumbnails() -> Result<Vec<CaptureDisplayWithThumbnail>, String> { - tokio::task::spawn_blocking(|| { - tauri::async_runtime::block_on(collect_displays_with_thumbnails()) - }) - .await - .map_err(|e| e.to_string())? + collect_displays_with_thumbnails().await } -#[tauri::command] +#[tauri::command(async)] #[specta::specta] pub async fn list_windows_with_thumbnails() -> Result<Vec<CaptureWindowWithThumbnail>, String> { - tokio::task::spawn_blocking( - || tauri::async_runtime::block_on(collect_windows_with_thumbnails()), - ) - .await - .map_err(|e| e.to_string())? + collect_windows_with_thumbnails().await }
🧹 Nitpick comments (2)
apps/desktop/src-tauri/src/recording.rs (2)
29-29: Consider explicit tracing imports for clarity.The wildcard
use tracing::*;import brings all tracing macros and functions into scope, which can reduce clarity about which specific tracing facilities are used. While common in Rust codebases, explicit imports likeuse tracing::{debug, error, info, trace, warn};improve code self-documentation.Apply this diff to use explicit imports:
-use tracing::*; +use tracing::{debug, error, info, trace, warn};
410-414: Consider more descriptive error messages for SCK failures.The error messages
"GetShareableContent: {e}"and"GetShareableContent/NotAvailable"are functional but could be more user-friendly for debugging. Consider adding context about why this matters (e.g., "Failed to initialize ScreenCaptureKit" or "ScreenCaptureKit not available on this system").Apply this diff for clearer diagnostics:
#[cfg(target_os = "macos")] let shareable_content = crate::platform::get_shareable_content() .await - .map_err(|e| format!("GetShareableContent: {e}"))? - .ok_or_else(|| format!("GetShareableContent/NotAvailable"))?; + .map_err(|e| format!("Failed to initialize ScreenCaptureKit: {e}"))? + .ok_or_else(|| "ScreenCaptureKit content unavailable".to_string())?;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/desktop/src-tauri/src/recording.rs(7 hunks)apps/desktop/src-tauri/src/thumbnails/windows.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx,rs}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not add inline, block, or docstring comments in any language; code must be self-explanatory
Files:
apps/desktop/src-tauri/src/thumbnails/windows.rsapps/desktop/src-tauri/src/recording.rs
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Format Rust code usingrustfmtand ensure all Rust code passes workspace-level clippy lints.
Rust modules should be named with snake_case, and crate directories should be in kebab-case.
Files:
apps/desktop/src-tauri/src/thumbnails/windows.rsapps/desktop/src-tauri/src/recording.rs
🧬 Code graph analysis (2)
apps/desktop/src-tauri/src/thumbnails/windows.rs (3)
apps/desktop/src-tauri/src/thumbnails/mac.rs (2)
capture_display_thumbnail(7-12)capture_window_thumbnail(14-20)crates/scap-direct3d/src/lib.rs (1)
stride(543-545)apps/desktop/src-tauri/src/thumbnails/mod.rs (1)
normalize_thumbnail_dimensions(38-81)
apps/desktop/src-tauri/src/recording.rs (3)
apps/desktop/src/utils/tauri.ts (2)
CaptureDisplayWithThumbnail(365-365)CaptureWindowWithThumbnail(367-367)apps/desktop/src-tauri/src/thumbnails/mod.rs (2)
collect_displays_with_thumbnails(83-99)collect_windows_with_thumbnails(101-149)apps/desktop/src-tauri/src/platform/macos/sc_shareable_content.rs (1)
get_shareable_content(63-90)
⏰ 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). (4)
- GitHub Check: Clippy
- 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 (3)
apps/desktop/src-tauri/src/thumbnails/windows.rs (1)
3-140: Function logic and error handling are sound.The comprehensive overflow checks, dimension validation, and structured error handling with early returns are well-implemented. The progressive validation approach (dimensions → stride → offsets → capacity → reconstruction) is robust.
apps/desktop/src-tauri/src/recording.rs (2)
437-446: LGTM! Studio actor build correctly threads shareable_content.The conditional macOS parameter passing is clean, and the error handling with descriptive logging is appropriate. This aligns with the PR's objective to prewarm and share SCK content across recording flows.
473-482: LGTM! Instant actor build correctly threads shareable_content.The conditional macOS parameter passing matches the studio recording branch pattern, maintaining consistency across both recording modes. The error handling appropriately logs and propagates failures.
| pub async fn capture_display_thumbnail(display: &scap_targets::Display) -> Option<String> { | ||
| use image::{ColorType, ImageEncoder, codecs::png::PngEncoder}; | ||
| use scap_direct3d::{Capturer, Settings}; | ||
| use std::io::Cursor; | ||
|
|
||
| let item = display.raw_handle().try_as_capture_item().ok()?; | ||
|
|
||
| let (tx, rx) = std::sync::mpsc::channel(); | ||
|
|
||
| let settings = Settings { | ||
| is_cursor_capture_enabled: Some(false), | ||
| pixel_format: scap_direct3d::PixelFormat::R8G8B8A8Unorm, | ||
| ..Default::default() | ||
| }; | ||
|
|
||
| let mut capturer = Capturer::new( | ||
| item, | ||
| settings.clone(), | ||
| move |frame| { | ||
| let _ = tx.send(frame); | ||
| Ok(()) | ||
| }, | ||
| || Ok(()), | ||
| None, | ||
| ) | ||
| .ok()?; | ||
|
|
||
| capturer.start().ok()?; | ||
|
|
||
| let frame = rx.recv_timeout(std::time::Duration::from_secs(2)).ok()?; | ||
| let _ = capturer.stop(); | ||
|
|
||
| let width = frame.width(); | ||
| let height = frame.height(); | ||
|
|
||
| if width == 0 || height == 0 { | ||
| return None; | ||
| } | ||
|
|
||
| let frame_buffer = frame.as_buffer().ok()?; | ||
| let data = frame_buffer.data(); | ||
| let stride = frame_buffer.stride() as usize; | ||
|
|
||
| let width_usize = width as usize; | ||
| let height_usize = height as usize; | ||
|
|
||
| let Some(row_bytes) = width_usize.checked_mul(4) else { | ||
| warn!( | ||
| frame_width = width, | ||
| "Windows display thumbnail row size overflowed" | ||
| ); | ||
| return None; | ||
| }; | ||
|
|
||
| if stride < row_bytes { | ||
| warn!( | ||
| frame_width = width, | ||
| frame_height = height, | ||
| stride, | ||
| expected_row_bytes = row_bytes, | ||
| "Windows display thumbnail stride smaller than row size" | ||
| ); | ||
| return None; | ||
| } | ||
|
|
||
| let rows_before_last = height_usize.saturating_sub(1); | ||
| let Some(last_row_start) = rows_before_last.checked_mul(stride) else { | ||
| warn!( | ||
| frame_width = width, | ||
| frame_height = height, | ||
| stride, | ||
| "Windows display thumbnail row offset overflowed" | ||
| ); | ||
| return None; | ||
| }; | ||
|
|
||
| let Some(required_len) = last_row_start.checked_add(row_bytes) else { | ||
| warn!( | ||
| frame_width = width, | ||
| frame_height = height, | ||
| stride, | ||
| required_row_bytes = row_bytes, | ||
| "Windows display thumbnail required length overflowed" | ||
| ); | ||
| return None; | ||
| }; | ||
|
|
||
| if data.len() < required_len { | ||
| warn!( | ||
| frame_width = width, | ||
| frame_height = height, | ||
| stride, | ||
| frame_data_len = data.len(), | ||
| expected_len = required_len, | ||
| "Windows display thumbnail frame buffer missing pixel data" | ||
| ); | ||
| return None; | ||
| } | ||
|
|
||
| let Some(rgba_capacity) = height_usize.checked_mul(row_bytes) else { | ||
| warn!( | ||
| frame_width = width, | ||
| frame_height = height, | ||
| total_row_bytes = row_bytes, | ||
| "Windows display thumbnail RGBA capacity overflowed" | ||
| ); | ||
| return None; | ||
| }; | ||
|
|
||
| let mut rgba_data = Vec::with_capacity(rgba_capacity); | ||
| for y in 0..height_usize { | ||
| let row_start = y * stride; | ||
| let row_end = row_start + row_bytes; | ||
| rgba_data.extend_from_slice(&data[row_start..row_end]); | ||
| } | ||
|
|
||
| let Some(img) = image::RgbaImage::from_raw(width, height, rgba_data) else { | ||
| warn!("Windows display thumbnail failed to construct RGBA image"); | ||
| return None; | ||
| }; | ||
| let thumbnail = normalize_thumbnail_dimensions(&img); | ||
|
|
||
| let mut png_data = Cursor::new(Vec::new()); | ||
| let encoder = PngEncoder::new(&mut png_data); | ||
| encoder | ||
| .write_image( | ||
| thumbnail.as_raw(), | ||
| thumbnail.width(), | ||
| thumbnail.height(), | ||
| ColorType::Rgba8.into(), | ||
| ) | ||
| .ok()?; | ||
|
|
||
| Some(base64::Engine::encode( | ||
| &base64::engine::general_purpose::STANDARD, | ||
| png_data.into_inner(), | ||
| )) | ||
| } | ||
|
|
||
| pub async fn capture_window_thumbnail(window: &scap_targets::Window) -> Option<String> { | ||
| use image::{ColorType, ImageEncoder, codecs::png::PngEncoder}; | ||
| use scap_direct3d::{Capturer, Settings}; | ||
| use std::io::Cursor; | ||
|
|
||
| let item = window.raw_handle().try_as_capture_item().ok()?; | ||
|
|
||
| let (tx, rx) = std::sync::mpsc::channel(); | ||
|
|
||
| let settings = Settings { | ||
| is_cursor_capture_enabled: Some(false), | ||
| pixel_format: scap_direct3d::PixelFormat::R8G8B8A8Unorm, | ||
| ..Default::default() | ||
| }; | ||
|
|
||
| let mut capturer = Capturer::new( | ||
| item, | ||
| settings.clone(), | ||
| move |frame| { | ||
| let _ = tx.send(frame); | ||
| Ok(()) | ||
| }, | ||
| || Ok(()), | ||
| None, | ||
| ) | ||
| .ok()?; | ||
|
|
||
| capturer.start().ok()?; | ||
|
|
||
| let frame = rx.recv_timeout(std::time::Duration::from_secs(2)).ok()?; | ||
| let _ = capturer.stop(); | ||
|
|
||
| let width = frame.width(); | ||
| let height = frame.height(); | ||
|
|
||
| if width == 0 || height == 0 { | ||
| return None; | ||
| } | ||
|
|
||
| let frame_buffer = frame.as_buffer().ok()?; | ||
| let data = frame_buffer.data(); | ||
| let stride = frame_buffer.stride() as usize; | ||
|
|
||
| let width_usize = width as usize; | ||
| let height_usize = height as usize; | ||
|
|
||
| let Some(row_bytes) = width_usize.checked_mul(4) else { | ||
| warn!( | ||
| frame_width = width, | ||
| "Windows window thumbnail row size overflowed" | ||
| ); | ||
| return None; | ||
| }; | ||
|
|
||
| if stride < row_bytes { | ||
| warn!( | ||
| frame_width = width, | ||
| frame_height = height, | ||
| stride, | ||
| expected_row_bytes = row_bytes, | ||
| "Windows window thumbnail stride smaller than row size" | ||
| ); | ||
| return None; | ||
| } | ||
|
|
||
| let rows_before_last = height_usize.saturating_sub(1); | ||
| let Some(last_row_start) = rows_before_last.checked_mul(stride) else { | ||
| warn!( | ||
| frame_width = width, | ||
| frame_height = height, | ||
| stride, | ||
| "Windows window thumbnail row offset overflowed" | ||
| ); | ||
| return None; | ||
| }; | ||
|
|
||
| let Some(required_len) = last_row_start.checked_add(row_bytes) else { | ||
| warn!( | ||
| frame_width = width, | ||
| frame_height = height, | ||
| stride, | ||
| required_row_bytes = row_bytes, | ||
| "Windows window thumbnail required length overflowed" | ||
| ); | ||
| return None; | ||
| }; | ||
|
|
||
| if data.len() < required_len { | ||
| warn!( | ||
| frame_width = width, | ||
| frame_height = height, | ||
| stride, | ||
| frame_data_len = data.len(), | ||
| expected_len = required_len, | ||
| "Windows window thumbnail frame buffer missing pixel data" | ||
| ); | ||
| return None; | ||
| } | ||
|
|
||
| let Some(rgba_capacity) = height_usize.checked_mul(row_bytes) else { | ||
| warn!( | ||
| frame_width = width, | ||
| frame_height = height, | ||
| total_row_bytes = row_bytes, | ||
| "Windows window thumbnail RGBA capacity overflowed" | ||
| ); | ||
| return None; | ||
| }; | ||
|
|
||
| let mut rgba_data = Vec::with_capacity(rgba_capacity); | ||
| for y in 0..height_usize { | ||
| let row_start = y * stride; | ||
| let row_end = row_start + row_bytes; | ||
| rgba_data.extend_from_slice(&data[row_start..row_end]); | ||
| } | ||
|
|
||
| let Some(img) = image::RgbaImage::from_raw(width, height, rgba_data) else { | ||
| warn!("Windows window thumbnail failed to construct RGBA image"); | ||
| return None; | ||
| }; | ||
| let thumbnail = normalize_thumbnail_dimensions(&img); | ||
|
|
||
| let mut png_data = Cursor::new(Vec::new()); | ||
| let encoder = PngEncoder::new(&mut png_data); | ||
| encoder | ||
| .write_image( | ||
| thumbnail.as_raw(), | ||
| thumbnail.width(), | ||
| thumbnail.height(), | ||
| ColorType::Rgba8.into(), | ||
| ) | ||
| .ok()?; | ||
|
|
||
| Some(base64::Engine::encode( | ||
| &base64::engine::general_purpose::STANDARD, | ||
| png_data.into_inner(), | ||
| )) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Extract shared thumbnail capture logic to eliminate duplication.
Both capture_display_thumbnail and capture_window_thumbnail duplicate ~136 lines of identical logic. The only differences are the capture item source (line 8 vs 147) and warning message prefixes.
Consider extracting the shared pipeline (capturer setup, frame capture, validation, RGBA reconstruction, PNG encoding) into a helper function that accepts the capture item and a label for warnings.
Example refactor:
async fn capture_thumbnail_from_item(
item: impl Into<scap_direct3d::CaptureItem>,
label: &str,
) -> Option<String> {
use image::{ColorType, ImageEncoder, codecs::png::PngEncoder};
use scap_direct3d::{Capturer, Settings};
use std::io::Cursor;
let (tx, rx) = std::sync::mpsc::channel();
let settings = Settings {
is_cursor_capture_enabled: Some(false),
pixel_format: scap_direct3d::PixelFormat::R8G8B8A8Unorm,
..Default::default()
};
let mut capturer = Capturer::new(
item.into(),
settings.clone(),
move |frame| {
let _ = tx.send(frame);
Ok(())
},
|| Ok(()),
None,
)
.ok()?;
capturer.start().ok()?;
let frame = rx.recv_timeout(std::time::Duration::from_secs(2)).ok()?;
let _ = capturer.stop();
let width = frame.width();
let height = frame.height();
if width == 0 || height == 0 {
return None;
}
let frame_buffer = frame.as_buffer().ok()?;
let data = frame_buffer.data();
let stride = frame_buffer.stride() as usize;
let width_usize = width as usize;
let height_usize = height as usize;
let Some(row_bytes) = width_usize.checked_mul(4) else {
warn!(frame_width = width, "{label} row size overflowed");
return None;
};
if stride < row_bytes {
warn!(
frame_width = width,
frame_height = height,
stride,
expected_row_bytes = row_bytes,
"{label} stride smaller than row size"
);
return None;
}
let rows_before_last = height_usize.saturating_sub(1);
let Some(last_row_start) = rows_before_last.checked_mul(stride) else {
warn!(
frame_width = width,
frame_height = height,
stride,
"{label} row offset overflowed"
);
return None;
};
let Some(required_len) = last_row_start.checked_add(row_bytes) else {
warn!(
frame_width = width,
frame_height = height,
stride,
required_row_bytes = row_bytes,
"{label} required length overflowed"
);
return None;
};
if data.len() < required_len {
warn!(
frame_width = width,
frame_height = height,
stride,
frame_data_len = data.len(),
expected_len = required_len,
"{label} frame buffer missing pixel data"
);
return None;
}
let Some(rgba_capacity) = height_usize.checked_mul(row_bytes) else {
warn!(
frame_width = width,
frame_height = height,
total_row_bytes = row_bytes,
"{label} RGBA capacity overflowed"
);
return None;
};
let mut rgba_data = Vec::with_capacity(rgba_capacity);
for y in 0..height_usize {
let row_start = y * stride;
let row_end = row_start + row_bytes;
rgba_data.extend_from_slice(&data[row_start..row_end]);
}
let Some(img) = image::RgbaImage::from_raw(width, height, rgba_data) else {
warn!("{label} failed to construct RGBA image");
return None;
};
let thumbnail = normalize_thumbnail_dimensions(&img);
let mut png_data = Cursor::new(Vec::new());
let encoder = PngEncoder::new(&mut png_data);
encoder
.write_image(
thumbnail.as_raw(),
thumbnail.width(),
thumbnail.height(),
ColorType::Rgba8.into(),
)
.ok()?;
Some(base64::Engine::encode(
&base64::engine::general_purpose::STANDARD,
png_data.into_inner(),
))
}
pub async fn capture_display_thumbnail(display: &scap_targets::Display) -> Option<String> {
let item = display.raw_handle().try_as_capture_item().ok()?;
capture_thumbnail_from_item(item, "Windows display thumbnail").await
}
pub async fn capture_window_thumbnail(window: &scap_targets::Window) -> Option<String> {
let item = window.raw_handle().try_as_capture_item().ok()?;
capture_thumbnail_from_item(item, "Windows window thumbnail").await
}🤖 Prompt for AI Agents
apps/desktop/src-tauri/src/thumbnails/windows.rs lines 3-279: the two functions
capture_display_thumbnail and capture_window_thumbnail duplicate the entire
capture/validation/encoding pipeline; extract the shared logic into a single
async helper like capture_thumbnail_from_item(item: impl
Into<scap_direct3d::CaptureItem>, label: &str) -> Option<String> that performs
capturer setup, frame recv/stop, buffer validation, RGBA reconstruction,
normalization and PNG/base64 encoding, and replace both functions with thin
wrappers that convert their raw handle to a capture item and call the helper
with the appropriate label (e.g. "Windows display thumbnail" / "Windows window
thumbnail"); keep existing warning keys and messages but interpolate the label
so behavior and diagnostics remain identical.
Currently in production the current timeout of 5s is too slow - solely adjusting this is not enough currently on my machine, because every subsequent recording will still take my machine ~10s because there's no prewarming of SCK.
Before (production app):
Screen.Recording.2025-09-26.at.22.20.19.mov
After (with SCK warmup, fresh app startup):
Screen.Recording.2025-09-27.at.13.52.10.mov
Summary by CodeRabbit
New Features
Bug Fixes
Chores