New recording flow #933
New recording flow #933
Conversation
|
Warning Rate limit exceeded@oscartbeaumont has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 14 minutes and 45 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughMigrates display/window types from cap-displays to scap-targets, adds a Tauri Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Frontend as UI
participant Tauri as Backend
participant Targets as scap_targets
UI->>Tauri: commands.display_information(displayId)
Tauri->>Targets: scap_targets::Display::from_id(displayId)
Targets-->>Tauri: Display { name?, physical_size?, refresh_rate? }
Tauri-->>UI: DisplayInformation { name?, physical_size?, refresh_rate }
note right of UI: overlay label and size/refresh_rate\nrendered using DisplayInformation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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: 3
♻️ Duplicate comments (14)
crates/scap-targets/src/platform/win.rs (1)
1045-1053: Duplicated window filtering logic should be consolidated.The window filtering logic is duplicated between
is_window_valid_for_enumerationandis_window_valid_for_topmost_selection. Consider extracting the common process filtering into a shared helper.As noted in lines 1076-1088, this duplicated process filtering logic should be extracted into a helper function.
packages/ui-solid/src/auto-imports.d.ts (2)
86-86: Use correct hyphenated Lucide slug for Volume 2.As noted in the past review comment and confirmed by web search, the canonical slug for the Lucide icon "Volume 2" is
volume-2(with a hyphen), notvolume2.
88-88: Correct Material Symbols icon slug to include hyphen.As noted in the past review comment, the canonical Material Symbols slug is
screenshot-frame-2-rounded(with hyphen between "frame" and "2").apps/desktop/src/routes/target-select-overlay.tsx (4)
135-139: Fix: incorrect Solid Show usage causes runtime error.Inside the inner Show, the callback parameter is the raw value (not an accessor), but it's invoked as
size(). This will throw a runtime error.
175-182: Fix: incorrect Solid Show usage for window icon.Inside Show, the parameter is the value, not an accessor. Calling
icon()will throw.
247-261: Memory leak: createRoot around window resize listener.The inner
createRootis unnecessary and thedispose()call inonCleanupcreates a circular/ineffective cleanup path.
729-730: Avoid raw string events; use generated specta events.Per desktop guidelines, prefer typed events from
eventsor directly trigger sign-in.apps/desktop/src/utils/tauri.ts (1)
524-528: Unify refresh-rate types (string vs number) via Rust struct change + regenerate TS bindings.DisplayInformation.refresh_rate is a string, while CaptureDisplay.refresh_rate and CaptureWindow.refresh_rate are numbers. This inconsistency will force ad‑hoc parsing or branching in the UI.
Given this file is auto-generated, don’t edit it directly. Instead:
- Change the Rust DisplayInformation to return both a numeric Hz and a label, then regenerate tauri.ts.
- Keep UI formatting concerns in the frontend, numeric logic on Hz.
Proposed Rust shape (in apps/desktop/src-tauri/src/target_select_overlay.rs or wherever DisplayInformation is defined):
#[derive(Serialize, specta::Type)] pub struct DisplayInformation { pub name: Option<String>, pub physical_size: Option<PhysicalSize>, pub refresh_rate_hz: u32, // numeric pub refresh_rate_label: String, // e.g. "144 Hz" }After regenerating, the TS type will match:
export type DisplayInformation = { name: string | null; physical_size: PhysicalSize | null; refresh_rate_hz: number; refresh_rate_label: string; };This aligns all refresh-rate shapes, removes parsing, and keeps the generated file untouched.
Also applies to: 471-475
apps/desktop/src/routes/(window-chrome)/new-main/index.tsx (6)
158-161: Close overlays at countdown start to match UX (“overlay shouldn't linger during countdown”).Subscribe to recordingEvent and close overlays when Countdown fires.
+ import { events } from "~/utils/tauri"; createEffect(() => { if (rawOptions.targetMode) commands.openTargetSelectOverlays(); else commands.closeTargetSelectOverlays(); }); + + // Close overlays immediately when the countdown begins + events.recordingEvent.listen(({ payload }) => { + if (payload.variant === "Countdown") { + commands.closeTargetSelectOverlays(); + } + });
296-296: UI direction forced to RTL; likely a typo.dir={ostype() === "windows" ? "rtl" : "rtl"} sets RTL everywhere.
- dir={ostype() === "windows" ? "rtl" : "rtl"} + dir={ostype() === "windows" ? "rtl" : "ltr"}
369-369: Typo in user-facing text: “Singning” → “Signing”.- <span>Singning In...</span> + <span>Signing In...</span>
4-4: Use Solid Query’s Solid API: replace useQuery with createQuery.@tanstack/solid-query for Solid exposes createQuery, not useQuery. Current code won’t type-check and will fail at runtime.
Apply:
- import { createMutation, useQuery } from "@tanstack/solid-query"; + import { createMutation, createQuery } from "@tanstack/solid-query";- const screens = useQuery(() => listScreens); - const windows = useQuery(() => listWindows); - const cameras = useQuery(() => listVideoDevices); - const mics = useQuery(() => listAudioDevices); + const screens = createQuery(() => listScreens); + const windows = createQuery(() => listWindows); + const cameras = createQuery(() => listVideoDevices); + const mics = createQuery(() => listAudioDevices);Also applies to: 163-167
168-178: Solid Query objects don’t expose .promise; use reactive effects on .data.Replace the .promise chains with createEffect blocks.
- cameras.promise.then((cameras) => { - if (rawOptions.cameraID && findCamera(cameras, rawOptions.cameraID)) { - setOptions("cameraLabel", null); - } - }); + createEffect(() => { + const cs = cameras.data; + if (!cs) return; + if (rawOptions.cameraID && findCamera(cs, rawOptions.cameraID)) { + setOptions("cameraLabel", null); + } + });- mics.promise.then((mics) => { - if (rawOptions.micName && !mics.includes(rawOptions.micName)) { - setOptions("micName", null); - } - }); + createEffect(() => { + const names = mics.data; + if (!names) return; + if (rawOptions.micName && !names.includes(rawOptions.micName)) { + setOptions("micName", null); + } + });
4-4: Fix remaininguseQueryand.promiseusagesThe automated search uncovered leftover React-style Solid Query patterns that must be replaced with Solid Query’s
createQueryand appropriate signal handling:• In
apps/desktop/src/utils/queries.tsat line 188:return useQuery(() => ({ … }))• In
apps/desktop/src/routes/(window-chrome)/new-main/index.tsxat lines 163–166:const screens = useQuery(() => listScreens); const windows = useQuery(() => listWindows); const cameras = useQuery(() => listVideoDevices); const mics = useQuery(() => listAudioDevices);• And the corresponding
.promiseaccesses at lines 168 and 174:cameras.promise.then((cameras) => { … }) mics.promise .then((mics) => { … })Please refactor these to use
createQuery(e.g.const screens = createQuery(() => listScreens())) and consume the returned data via Solid’s reactive signals or suspense, removing all.promisecalls.
🧹 Nitpick comments (6)
crates/scap-targets/src/platform/win.rs (3)
54-60: Static array is safer as a constant rather than a static reference.The current
static IGNORED_EXES: &'static [&str]pattern can be simplified and made safer. Consider using aconstarray instead of a static reference to an array.-static IGNORED_EXES: &'static [&str] = &[ +const IGNORED_EXES: &[&str] = &[ // As it's a system webview it isn't owned by the Cap process. "webview2", "msedgewebview2", // Just make sure, lol "cap", ];
1076-1088: Complex inline condition could benefit from extraction.The window filtering logic has become complex with multiple webview process checks. Consider extracting to a helper function for better readability and reusability.
- // Also skip WebView2 and Cap-related processes - if let Ok(exe_path) = pid_to_exe_path(process_id) { - if let Some(exe_name) = exe_path.file_name().and_then(|n| n.to_str()) { - let exe_name_lower = exe_name.to_lowercase(); - if exe_name_lower.contains("webview2") - || exe_name_lower.contains("msedgewebview2") - || exe_name_lower.contains("cap") - { - return false; - } - } - } + // Also skip WebView2 and Cap-related processes + if is_ignored_process(process_id) { + return false; + }And add a helper function:
fn is_ignored_process(process_id: u32) -> bool { if let Ok(exe_path) = unsafe { pid_to_exe_path(process_id) } { if let Some(exe_name) = exe_path.file_name().and_then(|n| n.to_str()) { let exe_name_lower = exe_name.to_lowercase(); // Check against static list if IGNORED_EXES.contains(&&*exe_name_lower) { return true; } // Additional pattern-based checks if exe_name_lower.contains("webview2") || exe_name_lower.contains("cap") { return true; } } } false }
527-528: Document rationale for GOOD_SIZE_THRESHOLD and make it configurableThe constant
GOOD_SIZE_THRESHOLDis hard-coded to 64px without explaining why that size was chosen or allowing callers to opt into a different cutoff. To improve clarity and flexibility:• Add a doc-comment on the constant describing the trade-off (e.g. “64px yields acceptable icon fidelity on Windows while avoiding unnecessary scaling”).
• Expose the threshold as a parameter or configuration option on the icon-loading API so downstream code can override it if needed.
• Double-check that each HDC and bitmap handle allocated in the search loop is properly dropped on both success and early exit—Rust’s RAII will normally free these, but an inline comment can help future maintainers confirm no leaks.apps/desktop/src/routes/(window-chrome)/new-main/index.tsx (3)
73-87: Name the default-exported component (PascalCase per guidelines).Components should be PascalCase. Name the default export for clearer stack traces and consistency.
-export default function () { +export default function NewMain() {
119-156: Minor: primaryMonitor() result is unused.If not needed, remove to avoid confusion; otherwise use it to place/center the window.
274-291: Use typed events via tauri_specta for “start-sign-in”It looks like we’re emitting and listening for
"start-sign-in"as a plain string in two places:
- apps/desktop/src/routes/target-select-overlay.tsx: onClick → emit("start-sign-in")
- apps/desktop/src/routes/(window-chrome)/new-main/index.tsx: listen("start-sign-in", …)
No matching generated event helper exists in the frontend bindings, so the suggestion still stands:
• Define a typed event in your Rust side (e.g. in src-tauri/src/events.rs) using tauri_specta’s#[specta]attribute.
• Re-run the binding generator to produce a TS helper (e.g.events.startSignIn) and replace both the string literalemit("start-sign-in")andlisten("start-sign-in",…)calls with the generated, type-safe helpers.This will keep the client and backend in sync and eliminate string-literal drift as your IPC surface grows.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
apps/desktop/src-tauri/src/lib.rs(10 hunks)apps/desktop/src-tauri/src/recording.rs(3 hunks)apps/desktop/src-tauri/src/target_select_overlay.rs(8 hunks)apps/desktop/src/routes/(window-chrome)/new-main/index.tsx(1 hunks)apps/desktop/src/routes/target-select-overlay.tsx(6 hunks)apps/desktop/src/utils/tauri.ts(7 hunks)crates/scap-targets/src/platform/win.rs(9 hunks)packages/ui-solid/src/auto-imports.d.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/desktop/**/*.{ts,tsx}: In the desktop app, rely on unplugin-icons auto-imports; do not manually import icon modules
Use generated tauri_specta commands/events (commands, events) in the desktop frontend; listen to generated events directly
Use @tanstack/solid-query for server state in the desktop app
Files:
apps/desktop/src/routes/(window-chrome)/new-main/index.tsxapps/desktop/src/routes/target-select-overlay.tsxapps/desktop/src/utils/tauri.ts
{apps/desktop,packages/ui-solid}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Component naming (Solid): components in PascalCase; hooks/utilities in camelCase starting with 'use' where applicable
Files:
apps/desktop/src/routes/(window-chrome)/new-main/index.tsxapps/desktop/src/routes/target-select-overlay.tsxpackages/ui-solid/src/auto-imports.d.tsapps/desktop/src/utils/tauri.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use strict TypeScript and avoid any; prefer shared types from packages
Files:
apps/desktop/src/routes/(window-chrome)/new-main/index.tsxapps/desktop/src/routes/target-select-overlay.tsxpackages/ui-solid/src/auto-imports.d.tsapps/desktop/src/utils/tauri.ts
apps/desktop/src-tauri/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Use tauri_specta on the Rust side for IPC (typed commands/events) and emit events via generated types
Files:
apps/desktop/src-tauri/src/target_select_overlay.rsapps/desktop/src-tauri/src/recording.rsapps/desktop/src-tauri/src/lib.rs
**/tauri.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Never edit auto-generated IPC bindings file: tauri.ts
Files:
apps/desktop/src/utils/tauri.ts
🧠 Learnings (4)
📚 Learning: 2025-08-25T10:58:06.134Z
Learnt from: CR
PR: CapSoftware/Cap#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-25T10:58:06.134Z
Learning: Applies to apps/desktop/**/*.{ts,tsx} : Use tanstack/solid-query for server state in the desktop app
Applied to files:
apps/desktop/src/routes/(window-chrome)/new-main/index.tsxapps/desktop/src/routes/target-select-overlay.tsx
📚 Learning: 2025-08-25T10:58:06.134Z
Learnt from: CR
PR: CapSoftware/Cap#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-25T10:58:06.134Z
Learning: Applies to apps/web/**/*.{ts,tsx} : Use TanStack Query v5 for client-side server state and data fetching in the web app
Applied to files:
apps/desktop/src/routes/(window-chrome)/new-main/index.tsx
📚 Learning: 2025-08-25T10:58:06.134Z
Learnt from: CR
PR: CapSoftware/Cap#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-25T10:58:06.134Z
Learning: Applies to **/queries.ts : Never edit auto-generated query bindings file: queries.ts
Applied to files:
apps/desktop/src/routes/(window-chrome)/new-main/index.tsx
📚 Learning: 2025-08-25T10:58:06.134Z
Learnt from: CR
PR: CapSoftware/Cap#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-25T10:58:06.134Z
Learning: Applies to apps/desktop/**/*.{ts,tsx} : In the desktop app, rely on unplugin-icons auto-imports; do not manually import icon modules
Applied to files:
packages/ui-solid/src/auto-imports.d.ts
🧬 Code graph analysis (5)
apps/desktop/src/routes/(window-chrome)/new-main/index.tsx (9)
apps/desktop/src/utils/tauri.ts (7)
CameraInfo(427-431)DeviceOrModelID(522-522)LogicalSize(606-606)commands(5-311)listAudioDevices(42-44)ScreenCaptureTarget(695-698)setMicInput(6-8)apps/desktop/src/store.ts (1)
generalSettingsStore(61-62)apps/desktop/src/routes/(window-chrome)/OptionsContext.tsx (1)
useRecordingOptions(9-16)apps/desktop/src/utils/queries.ts (4)
listScreens(41-46)listWindows(24-39)listVideoDevices(53-58)createCameraMutation(152-185)apps/desktop/src/utils/auth.ts (1)
createSignInMutation(13-32)apps/desktop/src/routes/(window-chrome)/Context.tsx (1)
WindowChromeHeader(29-41)apps/desktop/src/routes/(window-chrome)/new-main/CameraSelect.tsx (1)
CameraSelect(11-83)apps/desktop/src/routes/(window-chrome)/new-main/MicrophoneSelect.tsx (1)
MicrophoneSelect(12-120)apps/desktop/src/routes/(window-chrome)/new-main/SystemAudio.tsx (1)
SystemAudio(5-29)
apps/desktop/src/routes/target-select-overlay.tsx (3)
apps/desktop/src/utils/tauri.ts (3)
commands(5-311)displayInformation(305-307)ScreenCaptureTarget(695-698)apps/desktop/src/store.ts (2)
authStore(59-59)generalSettingsStore(61-62)apps/desktop/src/utils/queries.ts (1)
createOptionsQuery(87-118)
apps/desktop/src-tauri/src/target_select_overlay.rs (3)
apps/desktop/src/utils/tauri.ts (5)
DisplayId(523-523)WindowId(791-791)DisplayInformation(524-528)PhysicalSize(645-645)WindowUnderCursor(792-796)crates/scap-targets/src/platform/win.rs (8)
list(91-114)list(277-307)display(925-932)get_containing_cursor(146-159)get_topmost_at_cursor(313-356)owner_name(437-477)from_id(124-127)std(967-967)crates/scap-targets/src/lib.rs (9)
list(16-18)list(103-105)display(146-148)get_containing_cursor(36-38)get_topmost_at_cursor(114-116)owner_name(134-136)display_relative_logical_bounds(154-198)from_id(32-34)from_id(122-124)
apps/desktop/src-tauri/src/recording.rs (2)
apps/desktop/src/utils/tauri.ts (2)
RecordingMode(682-682)ShowCapWindow(718-730)apps/desktop/src-tauri/src/windows.rs (8)
id(664-690)app(195-195)app(315-315)app(397-397)app(670-670)app(859-859)label(112-114)from_str(52-84)
apps/desktop/src-tauri/src/lib.rs (7)
apps/desktop/src/utils/tauri.ts (10)
RecordingMode(682-682)DisplayId(523-523)WindowId(791-791)LogicalBounds(604-604)ScreenCaptureTarget(695-698)JsonValue(601-601)CurrentRecordingTarget(495-498)CurrentRecording(490-493)RecordingMetaWithMode(674-681)RecordingMeta(669-673)apps/desktop/src-tauri/src/target_select_overlay.rs (3)
scap_targets(49-52)display_information(138-149)get_window_icon(125-134)apps/desktop/src-tauri/src/recording.rs (3)
mode(125-130)capture_target(58-63)id(147-152)crates/scap-targets/src/platform/win.rs (3)
id(125-125)id(418-420)from_id(124-127)crates/scap-targets/src/platform/macos.rs (3)
id(44-44)id(288-290)from_id(43-46)apps/desktop/src-tauri/src/windows.rs (7)
id(664-690)app(195-195)app(315-315)app(397-397)app(670-670)app(859-859)label(112-114)crates/scap-targets/src/lib.rs (4)
id(28-30)id(118-120)from_id(32-34)from_id(122-124)
⏰ 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: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Analyze (rust)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (25)
crates/scap-targets/src/platform/win.rs (1)
1002-1009: Ensure IGNORED_EXES comparison is case-insensitive on Windows.The code converts the exe_name to lowercase for comparison against IGNORED_EXES, which is correct for Windows (case-insensitive filesystem). However, consider documenting this platform-specific behavior.
The case-insensitive comparison using
to_lowercase()is appropriate for Windows filesystem semantics.packages/ui-solid/src/auto-imports.d.ts (1)
14-14: LGTM: New Cap icon declarations added correctly.The three new icon declarations (
IconCapCaretDown,IconCapGear,IconCapX) follow the existing pattern and use correct hyphenated slugs.Also applies to: 29-29, 60-60
apps/desktop/src-tauri/src/recording.rs (4)
124-130: LGTM: Clean implementation of mode getter.The
mode()method provides a clear way to determine the recording mode from the enum variant.
310-313: LGTM: Correct parameter order for set_pending_recording.The change to pass both
modeandcapture_targetaligns with the updated state management API.
315-324: LGTM: Proper cleanup of target select overlays.The code correctly closes all
TargetSelectOverlaywindows before starting the countdown, ensuring a clean recording flow.
329-334: LGTM: Main window behavior correctly positioned after showing InProgressRecording.The main window start behavior is now triggered at the right time in the recording flow.
apps/desktop/src-tauri/src/target_select_overlay.rs (5)
11-14: LGTM: Clean migration to scap_targets.The imports correctly use the new
scap_targetsmodule, aligning with the PR's migration fromcap_displays.
36-41: LGTM: Well-structured DisplayInformation type.The new
DisplayInformationstruct properly encapsulates display metadata with appropriate optional fields.
125-134: LGTM: Well-implemented window icon retrieval.The
get_window_iconcommand properly validates the window ID, retrieves the icon, and returns it as a base64-encoded data URL.
137-149: LGTM: Clean display information retrieval.The
display_informationcommand properly validates the display ID and returns comprehensive display metadata.
196-196: Reevaluate the polling interval for focus updatesI only found a single instance of this polling delay:
- apps/desktop/src-tauri/src/target_select_overlay.rs line 196:
tokio::time::sleep(std::time::Duration::from_millis(400)).await;No other timing constants or “focus poll”–style logic appeared elsewhere in the codebase. Since this interval was bumped from 300 ms to 400 ms, please verify that the extra 100 ms of latency still meets your UI responsiveness requirements for focus management. You may want to:
- Exercise typical focus transitions in the desktop app and gauge perceived delay.
- Consider making the interval configurable or introducing a more event-driven notification if 400 ms proves too coarse.
apps/desktop/src/routes/target-select-overlay.tsx (4)
49-59: LGTM: Well-structured window icon query.The query is properly configured with appropriate caching (5 minutes) and conditional enabling based on window ID availability.
61-75: LGTM: Robust display information query with error handling.The query includes proper error handling and conditional enabling based on both display ID and target mode.
263-298: LGTM: Clean drag handler implementation with animation frame throttling.The use of
requestAnimationFramefor throttling mouse move events is a good performance optimization.
666-668: LGTM: Proper text capitalization utility.Simple and effective implementation for capitalizing mode names.
apps/desktop/src/utils/tauri.ts (4)
305-310: New commands look correct and match Rust exports.displayInformation and getWindowIcon bindings look consistent with the backend; good addition for the overlay flows.
492-493: Rename propagated: CurrentRecording now usesmode(nottype).Frontend type matches the backend change; this should prevent drift and confusion.
674-682: RecordingMetaWithMode type and list/get APIs are coherent.
- getRecordingMeta → RecordingMetaWithMode
- listRecordings → [string, RecordingMetaWithMode][]
These look consistent with the Rust side and avoid union narrowing in the UI.
Also applies to: 145-147, 154-156
562-563: Field rename ‘custom_cursor_capture2’ verified across backend and UIAll occurrences of the legacy
custom_cursor_capturekey have been updated tocustom_cursor_capture2. Grep results confirm that both data definitions and UI reads/writes use the new field:
- apps/desktop/src-tauri/src/general_settings.rs (serde rename and default)
- apps/desktop/src/utils/tauri.ts (type definition)
- apps/desktop/src/routes/(window-chrome)/settings/experimental.tsx (initial state, value binding, handleChange)
- apps/desktop/src/routes/(window-chrome)/settings/general.tsx (initial state)
- apps/desktop/src/routes/editor/ConfigSidebar.tsx (conditional disable)
No references to the old
custom_cursor_captureremain.apps/desktop/src-tauri/src/lib.rs (3)
1464-1479: Wrapper type for mode is clean and future‑proof.RecordingMetaWithMode.new derives RecordingMode from RecordingMetaInner. Flatting inner is convenient for the UI. LGTM.
1964-1966: Exporting display_information and get_window_icon commands is aligned with the new overlay flow.The generated bindings pick these up; good separation of concerns.
2235-2245: Nice UX: restore relevant windows on Settings/Upgrade/ModeSelect close.Showing TargetSelectOverlay and Main again avoids “lost window” states reported in the PR.
apps/desktop/src/routes/(window-chrome)/new-main/index.tsx (3)
253-259: Mutations for mic/camera look good; error handling restores UI state.The mic mutation updates the option after the backend call. The camera mutation (from utils/queries) already handles rollback on failure. Solid implementation.
Also applies to: 260-269
382-434: Options reconciliation logic reads well and handles fallbacks (window→display).The derived options and window→display fallback guard against empty sources. This should mitigate “window select overlay isn’t showing” edge-cases during data churn.
Also applies to: 185-236, 239-251
1-71: General: imports follow the Auto-Icons guideline and strict TS—nice.Icons are auto-imported, no any usage, and component naming elsewhere adheres to conventions.
| let (mode, capture_target) = match &state.recording_state { | ||
| RecordingState::None => return Ok(JsonValue::new(&None)), | ||
| RecordingState::Pending { mode, target } => (*mode, target), | ||
| RecordingState::Active(inner) => (inner.mode(), inner.capture_target()), | ||
| }; | ||
|
|
||
| let target = match capture_target { | ||
| ScreenCaptureTarget::Display { id } => CurrentRecordingTarget::Screen { id: id.clone() }, | ||
| ScreenCaptureTarget::Window { id } => CurrentRecordingTarget::Window { | ||
| id: id.clone(), | ||
| bounds: scap_targets::Window::from_id(&id) | ||
| .ok_or(())? | ||
| .display_relative_logical_bounds() | ||
| .ok_or(())?, | ||
| }, | ||
| ScreenCaptureTarget::Area { screen, bounds } => CurrentRecordingTarget::Area { | ||
| screen: screen.clone(), | ||
| bounds: bounds.clone(), | ||
| }, | ||
| }; | ||
|
|
||
| Ok(JsonValue::new(&Some(CurrentRecording { target, mode }))) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid erroring when a window target disappears; return None instead.
get_current_recording currently returns Err(()) if the WindowId no longer resolves (e.g., target window closed) or bounds aren’t available. This will reject the frontend invoke and can break polling/UI. Prefer degrading to Ok(JsonValue::new(&None)).
Suggested change:
- ScreenCaptureTarget::Window { id } => CurrentRecordingTarget::Window {
- id: id.clone(),
- bounds: scap_targets::Window::from_id(&id)
- .ok_or(())?
- .display_relative_logical_bounds()
- .ok_or(())?,
- },
+ ScreenCaptureTarget::Window { id } => {
+ if let Some(bounds) = scap_targets::Window::from_id(&id)
+ .and_then(|w| w.display_relative_logical_bounds())
+ {
+ CurrentRecordingTarget::Window { id: id.clone(), bounds }
+ } else {
+ // Target no longer valid; surface "no current recording" to the UI rather than erroring out.
+ return Ok(JsonValue::new(&None));
+ }
+ },This keeps the command shape stable and avoids spurious frontend errors during transient target churn.
📝 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.
| let (mode, capture_target) = match &state.recording_state { | |
| RecordingState::None => return Ok(JsonValue::new(&None)), | |
| RecordingState::Pending { mode, target } => (*mode, target), | |
| RecordingState::Active(inner) => (inner.mode(), inner.capture_target()), | |
| }; | |
| let target = match capture_target { | |
| ScreenCaptureTarget::Display { id } => CurrentRecordingTarget::Screen { id: id.clone() }, | |
| ScreenCaptureTarget::Window { id } => CurrentRecordingTarget::Window { | |
| id: id.clone(), | |
| bounds: scap_targets::Window::from_id(&id) | |
| .ok_or(())? | |
| .display_relative_logical_bounds() | |
| .ok_or(())?, | |
| }, | |
| ScreenCaptureTarget::Area { screen, bounds } => CurrentRecordingTarget::Area { | |
| screen: screen.clone(), | |
| bounds: bounds.clone(), | |
| }, | |
| }; | |
| Ok(JsonValue::new(&Some(CurrentRecording { target, mode }))) | |
| } | |
| let (mode, capture_target) = match &state.recording_state { | |
| RecordingState::None => return Ok(JsonValue::new(&None)), | |
| RecordingState::Pending { mode, target } => (*mode, target), | |
| RecordingState::Active(inner) => (inner.mode(), inner.capture_target()), | |
| }; | |
| let target = match capture_target { | |
| ScreenCaptureTarget::Display { id } => CurrentRecordingTarget::Screen { id: id.clone() }, | |
| ScreenCaptureTarget::Window { id } => { | |
| if let Some(bounds) = scap_targets::Window::from_id(&id) | |
| .and_then(|w| w.display_relative_logical_bounds()) | |
| { | |
| CurrentRecordingTarget::Window { id: id.clone(), bounds } | |
| } else { | |
| // Target no longer valid; surface "no current recording" to the UI rather than erroring out. | |
| return Ok(JsonValue::new(&None)); | |
| } | |
| }, | |
| ScreenCaptureTarget::Area { screen, bounds } => CurrentRecordingTarget::Area { | |
| screen: screen.clone(), | |
| bounds: bounds.clone(), | |
| }, | |
| }; | |
| Ok(JsonValue::new(&Some(CurrentRecording { target, mode }))) | |
| } |
| // Listen for resize events | ||
| const unlistenResize = currentWindow.onResized(() => { | ||
| const size = getWindowSize(); | ||
|
|
||
| currentWindow.setSize(new LogicalSize(size.width, size.height)); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Potential resize event loop: onResized calls setSize unconditionally.
Setting size inside onResized will re-trigger the event on some platforms, causing a loop/flicker. Guard the handler so it only sets size when it differs.
- const unlistenResize = currentWindow.onResized(() => {
- const size = getWindowSize();
- currentWindow.setSize(new LogicalSize(size.width, size.height));
- });
+ const unlistenResize = currentWindow.onResized(async ({ payload }) => {
+ const desired = getWindowSize();
+ // payload is PhysicalSize; convert to Logical via scale factor if needed
+ const scale = await currentWindow.scaleFactor();
+ const w = Math.round(payload.width / scale);
+ const h = Math.round(payload.height / scale);
+ if (w !== desired.width || h !== desired.height) {
+ currentWindow.setSize(new LogicalSize(desired.width, desired.height));
+ }
+ });Optionally, drop the onResized clamp altogether and enforce only on mount/focus if you don’t need hard clamping.
Also applies to: 131-139
🤖 Prompt for AI Agents
In apps/desktop/src/routes/(window-chrome)/new-main/index.tsx around lines
142-148 (and similarly lines 131-139), the onResized handler always calls
currentWindow.setSize which can re-trigger onResized and cause an event
loop/flicker; change the handler to compute the desired/clamped size from
getWindowSize, compare the desired width and height to the current window size,
and only call currentWindow.setSize(new LogicalSize(...)) if either dimension
differs; alternatively remove the onResized clamp and enforce sizing only on
mount/focus if hard clamping isn’t required.
Replaces #874
Blocking:
Summary by CodeRabbit
New Features
Improvements
Behavior