Prewarm target select overlay #1286
Conversation
WalkthroughAdds asynchronous preload of target-select overlay windows at startup by spawning a background task, converts overlay open to concurrent initialization, changes overlay teardown to hide windows instead of closing them, and removes an unused helper from the React component. Changes
Sequence Diagram(s)sequenceDiagram
participant App as App Startup (lib.rs)
participant Init as target_select_overlay::init()
participant Displays as System Displays
participant Overlay as Overlay Window
participant Recording as start_recording()
App->>+Init: spawn async task (init(&app).await)
App->>App: continue startup (non-blocking)
Init->>+Displays: enumerate displays
Displays->>+Overlay: concurrently create & show overlays (join_all)
Overlay-->>-Displays: overlays shown (or log errors)
Overlay->>Overlay: hide overlays after show (preload hide)
Init-->>-App: init finished
Note right of Recording: During recording start
Recording->>Overlay: find overlay windows
Overlay-->>Recording: hide overlay windows (was close before)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
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: 0
🧹 Nitpick comments (1)
apps/desktop/src-tauri/src/target_select_overlay.rs (1)
75-89: Consider collecting and logging errors from concurrent overlay creation.The
join_allresults are silently discarded. If any overlay fails to display, the command succeeds without indication. For consistency with theinitfunction (which logs errors) and better observability, consider checking if any operations failed.- join_all( + let results = join_all( scap_targets::Display::list() .into_iter() .map(|d| d.id()) .map(|display_id| { let app = app.clone(); async move { ShowCapWindow::TargetSelectOverlay { display_id } .show(&app) .await } }), ) .await; + + for result in results { + result.map_err(|err| error!("Error showing target select overlay: {err}")).ok(); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/desktop/src-tauri/src/lib.rs(1 hunks)apps/desktop/src-tauri/src/target_select_overlay.rs(3 hunks)apps/desktop/src/routes/target-select-overlay.tsx(0 hunks)
💤 Files with no reviewable changes (1)
- apps/desktop/src/routes/target-select-overlay.tsx
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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.rsapps/desktop/src-tauri/src/target_select_overlay.rs
🧬 Code graph analysis (2)
apps/desktop/src-tauri/src/lib.rs (1)
apps/desktop/src-tauri/src/target_select_overlay.rs (2)
spawn(252-292)init(47-65)
apps/desktop/src-tauri/src/target_select_overlay.rs (5)
apps/desktop/src-tauri/src/general_settings.rs (1)
init(243-258)apps/desktop/src-tauri/src/fake_window.rs (2)
init(108-110)app(52-52)apps/desktop/src-tauri/src/hotkeys.rs (3)
init(84-131)app(103-103)app(187-187)apps/desktop/src-tauri/src/lib.rs (6)
app(1170-1171)app(2335-2335)app(2366-2366)app(2427-2427)app(2433-2433)app(2657-2658)apps/desktop/src/utils/tauri.ts (1)
ShowCapWindow(464-464)
⏰ 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 (3)
apps/desktop/src-tauri/src/target_select_overlay.rs (2)
10-10: LGTM: Appropriate import for concurrent operations.The
join_allimport enables concurrent window creation across multiple displays, which aligns with the PR's performance improvement objectives.
45-65: LGTM: Well-designed prewarming initialization.The async init function appropriately:
- Creates overlays concurrently using
join_allfor all displays- Logs errors without blocking app startup
- Follows non-blocking initialization patterns seen in other modules
This should address the Windows performance issue mentioned in the PR description.
apps/desktop/src-tauri/src/lib.rs (1)
2187-2192: LGTM: Appropriate async initialization for overlay prewarming.The fire-and-forget spawn pattern is well-suited for prewarming overlays without blocking app startup. This aligns with similar async initialization patterns elsewhere in the setup handler.
|
I'm closing this in favor of merging it into #1282. These will overlap a lot and it will be easier to prevent bugs if they are done together. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
apps/desktop/src-tauri/src/target_select_overlay.rs (2)
45-68: Consider clarifying comment and verifying window behavior.The comment states windows are "created hidden" but the implementation shows them first (
show()) then hides them (hide()). This might cause a brief visual flicker during startup.Additionally, all errors are silently logged without propagating failures, making it difficult to detect if prewarming failed. Consider returning a count of successfully initialized windows or at least logging a summary.
If you want to avoid potential flicker, verify whether Tauri allows creating windows in a hidden state without showing them first:
// Investigate if this is possible: let window = ShowCapWindow::TargetSelectOverlay { display_id } .create_hidden(&app) // hypothetical API .await?;Alternatively, document why the show-then-hide approach is necessary.
78-94: Error message is misleading in context.The error message says "Error initializing target select overlay" but this is within
open_target_select_overlays, not theinitfunction. This could confuse debugging efforts.async move { ShowCapWindow::TargetSelectOverlay { display_id } .show(&app) .await - .map_err(|err| error!("Error initializing target select overlay: {err}")) + .map_err(|err| error!("Error opening target select overlay: {err}")) .ok(); }apps/desktop/src-tauri/src/lib.rs (1)
2187-2192: Adding trace logging would improve observability, but is optional—init() already handles errors internally.The
target_select_overlay::init()function returns(), notResult, and has internal error handling via theerror!()macro (lines 59, 89 in target_select_overlay.rs). The fire-and-forget spawn pattern is appropriate here per Tauri best practices for background initialization.The suggestion to add trace logging is reasonable for development/monitoring purposes since the tracing infrastructure is already available in the codebase (imported at lib.rs:90). However, this is an optional enhancement rather than a correctness issue—the function already logs errors internally.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/desktop/src-tauri/src/lib.rs(2 hunks)apps/desktop/src-tauri/src/recording.rs(1 hunks)apps/desktop/src-tauri/src/target_select_overlay.rs(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/desktop/src-tauri/src/recording.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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.rsapps/desktop/src-tauri/src/target_select_overlay.rs
🧬 Code graph analysis (2)
apps/desktop/src-tauri/src/lib.rs (1)
apps/desktop/src-tauri/src/target_select_overlay.rs (2)
spawn(260-300)init(47-68)
apps/desktop/src-tauri/src/target_select_overlay.rs (4)
crates/scap-targets/src/platform/macos.rs (2)
list(32-38)list(237-269)crates/scap-targets/src/platform/win.rs (2)
list(97-120)list(283-313)crates/scap-targets/src/lib.rs (2)
list(16-18)list(103-105)apps/desktop/src/utils/tauri.ts (1)
ShowCapWindow(464-464)
⏰ 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 (3)
apps/desktop/src-tauri/src/target_select_overlay.rs (2)
153-156: Approve the hide-instead-of-close strategy.Hiding windows instead of closing them aligns with the prewarming optimization goal, keeping windows ready for reuse.
73-77: Review comment is incorrect.This function is a
#[tauri::command], which means it's invoked from the frontend via IPC bridge, not from Rust code. The codebase contains no direct Rust calls to this function. The parameter change from reference to owned (AppHandle) is intentional and necessary—the function clonesappfor use in async closures (join_all and tokio::spawn), which require owned values due to Rust's lifetime rules. This is the correct pattern for concurrent initialization and does not constitute a breaking change.Likely an incorrect or invalid review comment.
apps/desktop/src-tauri/src/lib.rs (1)
2361-2361: Approve alignment with hide-based lifecycle.Changing to
window.hide()is consistent with the new prewarming strategy where windows are reused rather than recreated.
It's crazy slow on Windows so this should make it actually not suck.
Summary by CodeRabbit