New recording flow improvements #874
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughUpdates default for enable_new_recording_flow to be debug-dependent, exposes a new Tauri command to fetch display information, refactors target select overlay types by removing embedded screen data, introduces a new DisplayInformation type/function, and updates frontend to fetch and render display info asynchronously with corresponding type changes. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as TargetSelectOverlay (frontend)
participant Utils as utils.commands
participant Tauri as Tauri bridge
participant Backend as target_select_overlay::display_information
participant Displays as cap_displays::Display
UI->>Utils: displayInformation(displayId)
Utils->>Tauri: invoke("display_information", { displayId })
Tauri->>Backend: display_information(display_id)
Backend->>Displays: Display::from_id(display_id)
Displays-->>Backend: Display
Backend-->>Tauri: DisplayInformation { name, physical_size, refresh_rate }
Tauri-->>Utils: DisplayInformation
Utils-->>UI: DisplayInformation
UI->>UI: Render overlay with display details
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
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: 0
🔭 Outside diff range comments (1)
apps/desktop/src-tauri/src/target_select_overlay.rs (1)
107-115: Critical: close_target_select_overlays no longer cleans up background tasks and the global Escape shortcutThis change drops WindowFocusManager state, so:
- The cursor-tracking task set in open_target_select_overlays is never aborted.
- Per-overlay focus tasks spawned via WindowFocusManager::spawn are never aborted.
- The global Escape shortcut may remain registered.
This will leak tasks, keep unnecessary work running, and may leave the Escape shortcut stolen from other apps.
Apply this diff to restore cleanup (aborting tasks and unregistering the shortcut) while closing the overlay windows:
-#[specta::specta] -#[tauri::command] -pub async fn close_target_select_overlays(app: AppHandle) -> Result<(), String> { +#[specta::specta] +#[tauri::command] +pub async fn close_target_select_overlays( + app: AppHandle, + state: tauri::State<'_, WindowFocusManager>, +) -> Result<(), String> { for (id, window) in app.webview_windows() { if let Ok(CapWindowId::TargetSelectOverlay { .. }) = CapWindowId::from_str(&id) { let _ = window.close(); } } + // Abort per-overlay focus tasks and clear the map + let handles = { + let mut tasks = state.tasks.lock().unwrap_or_else(PoisonError::into_inner); + tasks.drain().map(|(_, h)| h).collect::<Vec<_>>() + }; + for h in handles { + let _ = h.abort(); + } + + // Abort the cursor tracking task + if let Some(task) = state + .task + .lock() + .unwrap_or_else(PoisonError::into_inner) + .take() + { + task.abort(); + } + + // Unregister the global Escape shortcut + app.global_shortcut() + .unregister("Escape") + .map_err(|err| { + error!("Error unregistering global keyboard shortcut for Escape: {err}") + }) + .ok(); + Ok(()) }If the intent is to avoid the state parameter, add an internal cleanup API (e.g., WindowFocusManager::destroy_all(&AppHandle)) and call it here.
🧹 Nitpick comments (3)
apps/desktop/src/routes/target-select-overlay.tsx (1)
46-60: Consider adding proper error handling for display information query.The current implementation logs errors to the console but doesn't provide user feedback when the display information fetch fails. Consider showing an error state or fallback UI.
const displayInformation = createQuery(() => ({ queryKey: ["displayId", params.displayId], queryFn: async () => { if (!params.displayId) return null; try { const info = await commands.displayInformation(params.displayId); return info; } catch (error) { console.error("Failed to fetch screen information:", error); + // Consider showing a toast notification or error message to the user return null; } }, enabled: params.displayId !== undefined && rawOptions.targetMode === "screen", }));Additionally, you might want to handle the loading state for a better user experience:
<Show when={displayInformation.data} keyed fallback={ <Show when={displayInformation.isLoading}> <div class="w-screen h-screen flex items-center justify-center"> <span>Loading display information...</span> </div> </Show> }>apps/desktop/src-tauri/src/target_select_overlay.rs (2)
38-42: Prefer numeric refresh_rate; format in the UI instead of returning StringReturning refresh_rate as a String reduces type fidelity (e.g., cannot easily compare or format without parsing). Consider keeping it as f64 and formatting “XX Hz” in the UI.
Apply this diff in this block:
pub struct DisplayInformation { name: String, physical_size: PhysicalSize, - refresh_rate: String, + refresh_rate: f64, }And adjust the constructor accordingly (see suggestion in Lines 117-133).
117-133: Follow-up if you switch refresh_rate to f64If you adopt the earlier suggestion to use f64, update construction accordingly.
Apply this diff in this block:
- let physical_size = display.physical_size(); - let refresh_rate = display.refresh_rate().to_string(); + let physical_size = display.physical_size(); + let refresh_rate = display.refresh_rate(); @@ - physical_size, - refresh_rate, + physical_size, + refresh_rate,And update the frontend TS type accordingly:
// apps/desktop/src/utils/tauri.ts export type DisplayInformation = { name: string physical_size: PhysicalSize refresh_rate: number // was string }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/desktop/src-tauri/src/general_settings.rs(1 hunks)apps/desktop/src-tauri/src/lib.rs(1 hunks)apps/desktop/src-tauri/src/target_select_overlay.rs(4 hunks)apps/desktop/src/routes/(window-chrome)/settings/experimental.tsx(1 hunks)apps/desktop/src/routes/target-select-overlay.tsx(3 hunks)apps/desktop/src/utils/tauri.ts(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
apps/desktop/src/routes/(window-chrome)/settings/experimental.tsx (2)
apps/desktop/src/routes/(window-chrome)/settings/Setting.tsx (1)
ToggleSetting(24-40)apps/desktop/src/routes/(window-chrome)/settings/general.tsx (8)
value(183-185)value(200-202)value(211-213)value(241-243)value(255-257)value(270-272)value(289-291)value(308-310)
apps/desktop/src-tauri/src/lib.rs (1)
apps/desktop/src-tauri/src/target_select_overlay.rs (1)
display_information(119-133)
apps/desktop/src-tauri/src/target_select_overlay.rs (4)
apps/desktop/src/utils/tauri.ts (2)
DisplayId(360-360)DisplayInformation(361-361)crates/displays/src/platform/win.rs (5)
format(429-431)format(497-499)from_id(110-113)physical_size(167-201)refresh_rate(203-228)crates/displays/src/lib.rs (3)
from_id(27-29)physical_size(39-41)refresh_rate(43-45)crates/displays/src/platform/macos.rs (3)
from_id(38-41)physical_size(73-91)refresh_rate(93-104)
⏰ 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 (aarch64-apple-darwin, macos-latest)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
🔇 Additional comments (10)
apps/desktop/src/routes/(window-chrome)/settings/experimental.tsx (1)
84-98: LGTM! The feature toggle is now always visible.The removal of the
import.meta.env.DEVconditional check makes the "New recording flow" toggle available in all environments, which aligns with the default behavior change in the backend whereenable_new_recording_flowdefaults totruein debug builds.apps/desktop/src-tauri/src/general_settings.rs (1)
107-109: Good change to enable feature in development by default.Using
cfg!(debug_assertions)ensures the new recording flow is enabled automatically in debug builds while remaining disabled in release builds. This provides a better developer experience without affecting production users.apps/desktop/src-tauri/src/lib.rs (1)
1959-1959: LGTM! New command properly exposed.The
target_select_overlay::display_informationcommand is correctly added to the specta command list, making it available to the frontend.apps/desktop/src/routes/target-select-overlay.tsx (2)
100-119: Good refactoring to use asynchronous display information fetch.The change from using
targetUnderCursor.screento fetching display information via a dedicated query improves the architecture by separating concerns and reducing the payload size of theTargetUnderCursorevent.
103-103: Staticdata-over="true"in screen mode is intentional
The hardcoded attribute on line 103 ensures the screen overlay is always highlighted when in screen-target mode. Dynamic toggling is already applied in window mode (line 131), so no changes are needed here.Likely an incorrect or invalid review comment.
apps/desktop/src/utils/tauri.ts (2)
260-262: LGTM! New command and type definitions are properly implemented.The
displayInformationcommand andDisplayInformationtype are correctly added, matching the backend implementation.
361-361: Good architectural improvement: Removing screen data from TargetUnderCursor.The removal of the
screenfield fromTargetUnderCursorand the introduction of a separateDisplayInformationtype improves the separation of concerns. This change:
- Reduces the payload size of the frequently emitted
TargetUnderCursorevent- Allows display information to be fetched on-demand when needed
- Makes the data flow more explicit and easier to reason about
Also applies to: 428-428
apps/desktop/src-tauri/src/target_select_overlay.rs (3)
12-14: Importing Display is correct and necessary for new display_information APIThe added import of Display (alongside DisplayId, WindowId, and PhysicalSize) aligns with the new usage in display_information().
38-42: AI summary inconsistency: DisplayInformation still derives Serialize/Type/CloneThe AI summary claims DisplayInformation “no longer derives Serialize, Type, or Clone,” but the code derives all three. Please confirm which is intended and update the summary or code accordingly.
117-133: New display_information command: API and error handling look correctParsing DisplayId, resolving Display::from_id, and returning DisplayInformation are all sound. Error strings are concise and appropriate.
oscartbeaumont
force-pushed
the
new-recording-flow2
branch
from
e0fe42f to
6ba08c3
Compare
windowsfeatures oncap-displaysIs the Windowsz-indeximplementation good enough?Summary by CodeRabbit