New recording flow #933
New recording flow #933
Conversation
WalkthroughMigrates display/window dependencies from cap-displays to scap-targets across Rust crates and apps. Updates Tauri backend (new display_information command, overlay APIs, Windows sizing logic). Refactors frontend new recording flow into modular components and adds update/check logic. Overhauls target-select overlay UI to fetch display metadata. Minor UI/setting tweaks and one file removal. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor UI as Frontend (overlay)
participant Tauri as Tauri Command
participant Targets as scap_targets::Display
UI->>Tauri: display_information(display_id)
Tauri->>Targets: Display::from_id(parse(display_id))
Targets-->>Tauri: Display{name, physical_size, refresh_rate?}
Tauri-->>UI: DisplayInformation {name?, physical_size?, refresh_rate}
UI->>UI: Render monitor panel (fallbacks if null)
sequenceDiagram
autonumber
actor User
participant UI as Camera/Mic Selector
participant Hook as useRequestPermission
participant Cmd as Tauri commands
participant Query as Permissions Query
User->>UI: Click "Request Permission"
UI->>Hook: requestPermission(type)
Hook->>Cmd: reset{Type}Permissions()
Hook->>Cmd: requestPermission(type)
Hook->>Query: refetch(getPermissions)
Query-->>UI: permissions updated
UI->>UI: Update pill (On/Off or Request)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/displays/src/platform/win.rs (2)
124-131: Fix UB/compile error in GetDpiForMonitor call (cannot take &mut of a temporary).You're passing &mut 0 for the vertical DPI, which is invalid and won’t compile. Declare a second mutable variable instead.
- let dpi = unsafe { - let mut dpi_x = 0; - GetDpiForMonitor(self.0, MDT_EFFECTIVE_DPI, &mut dpi_x, &mut 0).ok()?; - dpi_x - }; + let dpi = unsafe { + let mut dpi_x = 0u32; + let mut dpi_y = 0u32; + GetDpiForMonitor(self.0, MDT_EFFECTIVE_DPI, &mut dpi_x, &mut dpi_y).ok()?; + dpi_x + };
838-846: Use mem::size_of and avoid &raw here; current code won’t compile on stable and size_of isn’t in scope.
- size_of::() isn’t imported; use mem::size_of::() given you already import std::mem as mem.
- The (&raw mut rect) form is unnecessary here and may require newer language features. A plain cast from &mut rect is sufficient.
- DwmGetWindowAttribute( - self.0, - DWMWA_EXTENDED_FRAME_BOUNDS, - (&raw mut rect).cast(), - size_of::<RECT>() as u32, - ) + DwmGetWindowAttribute( + self.0, + DWMWA_EXTENDED_FRAME_BOUNDS, + (&mut rect as *mut _ as *mut _), + mem::size_of::<RECT>() as u32, + ) .ok()?;Apply the same change to the corresponding call in physical_bounds.
Also applies to: 875-883
🧹 Nitpick comments (32)
crates/displays/src/platform/win.rs (4)
379-393: Don’t reorder candidates; EnumWindows already returns Z-order.The comment says “Sort candidates by Z-order,” but the sort only prioritizes WS_EX_TOPMOST and may produce incorrect ordering vs. actual Z-order. EnumWindows enumerates top-level windows in Z-order already; keep that order and pick the first valid candidate.
- // Sort candidates by Z-order (topmost first) - data.candidates.sort_by(|&a, &b| { - // Use GetWindowLong to check topmost status - let a_topmost = (GetWindowLongW(a, GWL_EXSTYLE) & WS_EX_TOPMOST.0 as i32) != 0; - let b_topmost = (GetWindowLongW(b, GWL_EXSTYLE) & WS_EX_TOPMOST.0 as i32) != 0; - - match (a_topmost, b_topmost) { - (true, false) => std::cmp::Ordering::Less, // a is more topmost - (false, true) => std::cmp::Ordering::Greater, // b is more topmost - _ => std::cmp::Ordering::Equal, // Same topmost level - } - }); - data.candidates.first().map(|&hwnd| Self(hwnd))
913-933: Return None on GetWindowTextW failure instead of empty string.If GetWindowTextW returns 0 (failure), you currently return Some(String::new()). That conflates “untitled window” with “API error.”
- if copied == 0 { - return Some(String::new()); - } + if copied == 0 { + return None; + }
963-967: Case-insensitive path check for SystemApps.starts_with("C:\Windows\SystemApps") is case-sensitive and may miss matches on Windows. Normalize case or compare components.
- if owner_process_path.starts_with("C:\\Windows\\SystemApps") { + if owner_process_path + .to_string_lossy() + .to_ascii_lowercase() + .starts_with("c:\\windows\\systemapps") + { return false; }
677-820: Icon extraction loop is heavy; add an early-exit threshold and prefer CreateDIBSection.You iterate many target sizes per icon and do a GetDIBits conversion per pass. This is expensive on large icons.
- Early-exit once you’ve captured a sufficiently large icon (e.g., >= 128 px).
- Prefer CreateDIBSection over CreateCompatibleBitmap + GetDIBits to avoid an extra copy.
Example early-exit:
- for &size in &sizes { + for &size in &sizes { ... - if draw_result.is_ok() { + if draw_result.is_ok() { ... - if best_result.is_none() || size > best_size { + if best_result.is_none() || size > best_size { best_result = Some((png_data, size)); best_size = size; + if best_size >= 128 { + break; // good enough + } }apps/desktop/src/routes/(window-chrome)/settings/experimental.tsx (1)
84-96: Ungating “New recording flow” toggle is good; streamline the scroll hack and fix the non‑standard behavior value.
- The toggle wiring and persistence look correct.
- The scroll workaround duplicates the same snippet used above and uses a non‑standard
behavior: "instant"(spec supports "auto" | "smooth"). Prefer a single helper andrequestAnimationFrame(orqueueMicrotask) to avoid jank.Apply this diff locally to the block to remove duplication and use a standard behavior:
- onChange={(value) => { - handleChange("enableNewRecordingFlow", value); - // This is bad code, but I just want the UI to not jank and can't seem to find the issue. - setTimeout( - () => window.scrollTo({ top: 0, behavior: "instant" }), - 5, - ); - }} + onChange={(value) => { + handleChange("enableNewRecordingFlow", value); + // Avoid layout jank: schedule after the next frame and use a standard behavior. + requestAnimationFrame(() => window.scrollTo({ top: 0, behavior: "auto" })); + }}If you want to DRY this with the “Auto zoom on clicks” toggle, extract a small
scrollTopSoon()utility and call it from both places.apps/desktop/src-tauri/src/general_settings.rs (1)
106-108: Defaulting the new recording flow to debug builds makes sense.This aligns with “enable by default for new configurations only” and keeps release builds stable. If you intend to dogfood in non‑debug CI builds, consider an opt‑in cargo feature or env override to flip this without touching code.
apps/desktop/src/routes/(window-chrome)/InfoPill.tsx (1)
1-17: Make class/type extensible and consistent with consumers.Spreading
...propsbeforetype/classforces your defaults and discards any caller-providedclass. Allow merging and let callers overridetypewhen needed.Apply this diff to merge classes and provide an overridable type:
-export default function InfoPill( - props: ComponentProps<"button"> & { variant: "blue" | "red" }, -) { - return ( - <button - {...props} - type="button" - class={cx( - "px-2 py-0.5 rounded-full text-white text-[11px]", - props.variant === "blue" ? "bg-blue-9" : "bg-red-9", - )} - /> - ); -} +export default function InfoPill({ + variant, + class: className, + type, + ...rest +}: ComponentProps<"button"> & { variant: "blue" | "red" }) { + return ( + <button + {...rest} + type={type ?? "button"} + class={cx( + "px-2 py-0.5 rounded-full text-white text-[11px]", + variant === "blue" ? "bg-blue-9" : "bg-red-9", + className, + )} + /> + ); +}Note: This component’s styles (bg-blue-9 with white text) differ from the prior inline InfoPill in (main).tsx (lighter background with dark text). Confirm this visual change is intentional across the app.
Would you like me to sweep for remaining inline “InfoPill”-like buttons and replace them with this component for consistency?
apps/desktop/src/utils/tauri.ts (1)
305-307: New TAURI commanddisplayInformationlooks correct.Name/case matches the Rust command (
display_information) and uses astringid compatible withDisplayId.If feasible in the specta surface, type the argument as
DisplayIdfor stronger intent (alias isstring, but improves readability).apps/desktop/src/routes/(window-chrome)/ChangeLogButton.tsx (2)
20-21: Prefer passing the fetcher directly tocreateResourcefor clarity.Minor style/readability improvement; avoids an extra closure allocation and matches common Solid patterns.
-const [currentVersion] = createResource(() => getVersion()); +const [currentVersion] = createResource(getVersion);
49-63: Avoid repeatedcurrentVersion()reads in the effect.Cache the value to prevent multiple reactive reads and ensure consistency within a single effect run.
-createEffect(() => { - if (changelogStatus.state === "ready" && currentVersion()) { - const hasUpdate = changelogStatus()?.hasUpdate || false; - if ( - hasUpdate === true && - changelogState.lastOpenedVersion !== currentVersion() - ) { - setChangelogState({ - hasUpdate: true, - lastOpenedVersion: currentVersion(), - changelogClicked: false, - }); - } - } -}); +createEffect(() => { + const v = currentVersion(); + if (changelogStatus.state === "ready" && v) { + const hasUpdate = changelogStatus()?.hasUpdate || false; + if (hasUpdate === true && changelogState.lastOpenedVersion !== v) { + setChangelogState({ + hasUpdate: true, + lastOpenedVersion: v, + changelogClicked: false, + }); + } + } +});apps/desktop/src-tauri/src/target_select_overlay.rs (3)
60-83: Reduce CPU/alloc churn: cache app icon base64 per window id.The loop emits every 50ms and re-encodes
w.app_icon()to base64 each tick, which is wasteful for unchanged topmost windows. Cache the icon string keyed byWindowIdand only re-encode when the topmost window changes.- let handle = tokio::spawn({ - let app = app.clone(); - async move { - loop { + let handle = tokio::spawn({ + let app = app.clone(); + async move { + let mut last_icon_cache: Option<(WindowId, String)> = None; + loop { let display = cap_displays::Display::get_containing_cursor(); - let window = cap_displays::Window::get_topmost_at_cursor(); + let window = cap_displays::Window::get_topmost_at_cursor(); let _ = TargetUnderCursor { display_id: display.map(|d| d.id()), - window: window.and_then(|w| { - Some(WindowUnderCursor { - id: w.id(), - bounds: w.display_relative_logical_bounds()?, - app_name: w.owner_name()?, - icon: w.app_icon().map(|bytes| { - format!("data:image/png;base64,{}", BASE64_STANDARD.encode(&bytes)) - }), - }) - }), + window: window.and_then(|w| { + let id = w.id(); + // Only (re)encode when window id changes or we have no cache. + if last_icon_cache.as_ref().map(|(wid, _)| wid != &id).unwrap_or(true) { + let encoded = w.app_icon().map(|bytes| { + format!("data:image/png;base64,{}", BASE64_STANDARD.encode(&bytes)) + }); + if let Some(encoded) = encoded { + last_icon_cache = Some((id.clone(), encoded)); + } else { + last_icon_cache = None; + } + } + Some(WindowUnderCursor { + id, + bounds: w.display_relative_logical_bounds()?, + app_name: w.owner_name()?, + icon: last_icon_cache.as_ref().and_then(|(wid, data)| { + // ensure cache matches current window id + if wid == &w.id() { Some(data.clone()) } else { None } + }), + }) + }), } .emit(&app); tokio::time::sleep(Duration::from_millis(50)).await; } } });
95-100: Fix misleading comment about registering the Escape shortcut.The
elsebranch runs when there was no previous task (i.e., first registration). The current comment says the opposite.- } else { - // If task is already set we know we have already registered this. + } else { + // First time starting the cursor tracking task; register the Escape shortcut.
117-131: Formatrefresh_rateto a human-friendly stringAfter inspecting
Display::refresh_rate, it returns a non-optionalf64. Rather than calling.to_string()(which produces a verbose float), round to the nearest integer and append “Hz” for clarity:apps/desktop/src-tauri/src/target_select_overlay.rs @@ pub async fn display_information(display_id: &str) -> Result<DisplayInformation, String> { - refresh_rate: display.refresh_rate().to_string(), + refresh_rate: format!("{:.0}Hz", display.refresh_rate()),apps/desktop/src-tauri/src/windows.rs (1)
278-280: Double-check cross-platform UX foralways_on_top(true)+content_protected(true)on TargetSelectOverlay.This will keep the overlay above all windows and hidden from screenshots on all platforms. That may be intended, but it can affect interactions with system UI (e.g., virtual desktops, UAC prompts on Windows).
Suggested quick manual checks:
- macOS: overlay sits above all apps but below your own “Main” window as managed by
WindowFocusManager; switching spaces behaves correctly.- Windows: UAC prompt doesn’t get obscured; overlay doesn’t trap focus unexpectedly; screen capture flows unaffected.
- Linux (if applicable): presence over full-screen apps and task switching.
If you observe issues, consider platform-conditional gating for
always_on_top.apps/desktop/src/routes/(window-chrome)/TargetSelectInfoPill.tsx (1)
1-33: Optional: add an accessible label.Since the pill content can be “On/Off/Request Permission”, consider adding
aria-labelfor screen readers to clarify which setting it controls (e.g., “Microphone permission: Request”). You can accept an optionalariaLabelprop and pass it through toInfoPill.apps/desktop/src/routes/(window-chrome)/TargetTypeButton.tsx (1)
12-28: Consider keyboard accessibility if this is interactiveIf the root is clickable, prefer a button or add role="button", tabIndex={0}, and key handlers for Enter/Space.
apps/desktop/src/routes/(window-chrome)/MicrophoneSelect.tsx (2)
45-51: Tauri event listener: minor clarity and cleanup safetyYou correctly clean up the listener. For readability, avoid shadowing dbs and name the promise descriptively.
-const result = events.audioInputLevelChange.listen((dbs) => { - if (!props.value) setDbs(); - else setDbs(dbs.payload); -}); - -onCleanup(() => result.then((unsub) => unsub())); +const unlistenPromise = events.audioInputLevelChange.listen((evt) => { + if (!props.value) setDbs(); + else setDbs(evt.payload); +}); + +onCleanup(() => unlistenPromise.then((unsub) => unsub()));
34-38: Type/guard inconsistency on props.optionsprops.options is typed as string[], yet guarded as if it could be undefined. Pick one:
- If it can be missing: change type to string[] | undefined and keep the guard.
- If it’s required: remove the if (!props.options) return; guard and rely on [] fallback where used.
apps/desktop/src/routes/(window-chrome)/CameraSelect.tsx (1)
25-35: Clarify onChange semantics and permission flow
- The parameter is named cameraLabel but is a CameraInfo; rename for clarity.
- If selecting a non-null camera while permission isn’t granted, consider prompting for permission (today you only prompt when clearing to null).
-const onChange = (cameraLabel: CameraInfo | null) => { - if (!cameraLabel && permissions?.data?.camera !== "granted") +const onChange = (camera: CameraInfo | null) => { + if (!camera && permissions?.data?.camera !== "granted") return requestPermission("camera"); - props.onChange(cameraLabel); + props.onChange(camera); trackEvent("camera_selected", { - camera_name: cameraLabel?.display_name ?? null, - enabled: !!cameraLabel, + camera_name: camera?.display_name ?? null, + enabled: !!camera, }); };Optionally request permission when enabling the camera too:
if (camera && permissions?.data?.camera !== "granted") { void requestPermission("camera"); }apps/desktop/src/routes/target-select-overlay.tsx (9)
106-109: Limit keyboard prevention to modifier shortcuts; current code blocks all keys (hurts a11y).Preventing every keydown disables Tab navigation, screen readers, etc. Restrict to Ctrl/Cmd combos (or specific keys) to avoid browser shortcuts without harming accessibility.
- createEventListener(document, "keydown", (e) => e.preventDefault()); + createEventListener(document, "keydown", (e) => { + // Only intercept modifier shortcuts (e.g., Ctrl/Cmd+P) + if (e.ctrlKey || e.metaKey) e.preventDefault(); + });
31-31: Remove unused import DisplayArt.This import is unused and should be removed to reduce bundle size.
-import DisplayArt from "../assets/illustrations/display.png";
136-137: Avoid non-null assertion on params.displayId; provide a safe fallback.If displayId is missing, the non-null assertion may crash. Either gate rendering or use a default/fallback value.
- <RecordingControls - target={{ variant: "display", id: params.displayId! }} - /> + <RecordingControls + target={{ variant: "display", id: params.displayId ?? "0" }} + />Alternatively, early-return from the display Match when params.displayId is undefined.
777-782: Dead code: getDisplayId is unused (and mismatched types).This helper isn’t referenced and returns a number, while DisplayId appears to be a string. Remove to avoid confusion.
-function getDisplayId(displayId: string | undefined) { - const id = Number(displayId); - if (Number.isNaN(id)) return 0; - return id; -}
675-706: Countdown menu actions are stubs (console.log); consider wiring to an option.The submenu currently logs only. If a pre-recording countdown is intended, add an option (e.g., countdownSeconds) and plumb it through startRecording.
Example action:
- action: () => { - console.log("Countdown 3 clicked"); - }, + action: () => setOptions({ countdownSeconds: 3 }),If you want, I can add countdownSeconds to the persisted options and update the backend call accordingly.
710-746: Use a semantic button for the primary action.The clickable div should be a button for accessibility (keyboard focus, role, ARIA). This also improves semantics for screen readers.
- <div + <button class="flex items-center px-4 py-2 rounded-full transition-colors cursor-pointer bg-blue-9 hover:bg-blue-10" onClick={() => { commands.startRecording({ capture_target: props.target, mode: rawOptions.mode, capture_system_audio: rawOptions.captureSystemAudio, }); }} -> + type="button"> ... -</div> +</button>
519-553: Make occluders resilient to window resizes without manual width math.These elements compute widths using window.innerWidth/Height, which won’t reactively update. Use CSS positioning with left/right/top/bottom to avoid JS width calculations.
Example for the right occluder:
- <div - class="absolute top-0 right-0 bottom-0 bg-black/50" - style={{ - width: `${window.innerWidth - (bounds.size.width + bounds.position.x)}px`, - }} - /> + <div + class="absolute top-0 right-0 bottom-0 bg-black/50" + style={{ + left: `${bounds.position.x + bounds.size.width}px`, + }} + />And similarly for the bottom occluder using style={{ top:
${bounds.position.y + bounds.size.height}px}}.
250-285: Mouse drag roots: ensure RAF cleanup is robust.You already cancelAnimationFrame on mouseup; good. Consider adding passive: false on listener if you ever need to call preventDefault during drag (touch support later). Not required now, just a heads-up.
647-756: Consistency: options context vs. local persisted options.RecordingControls uses createOptionsQuery whereas the new main flow uses RecordingOptionsProvider/useRecordingOptions. This divergence can lead to subtle desync if storage events race. Consider standardizing on the provider (with storage sync inside it).
I can refactor RecordingControls to consume useRecordingOptions for consistency if desired.
apps/desktop/src/routes/(window-chrome)/new-main.tsx (4)
138-140: Remove unused primaryMonitor read.The monitor value is fetched but never used. Drop it to avoid unnecessary async work.
- const monitor = await primaryMonitor(); - if (!monitor) return;
152-162: Prefer reactive effects over .promise for option reconciliation.Using cameras.promise/mics.promise runs once and may miss device list changes. A reactive effect on cameras.data/mics.data will keep options in sync.
- 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 ms = mics.data; + if (!ms) return; + if (rawOptions.micName && !ms.includes(rawOptions.micName)) { + setOptions("micName", null); + } + });
101-132: Window size enforcement: confirm UX requirement.Forcing size on mount, focus, and onResized prevents user resize entirely. If that’s intentional, fine; otherwise consider debouncing or only enforcing minimums.
I can switch to a min/max size policy via setMinSize/setMaxSize and remove the onResized handler if desired.
237-252: Camera selection initialization: await mutations to serialize device setup.Multiple mutate calls may race on mount. If the underlying command is sensitive, consider awaiting in sequence.
Example:
onMount(async () => { const id = rawOptions.cameraID; await setCamera.mutateAsync( !id ? null : "ModelID" in id ? { ModelID: id.ModelID } : { DeviceID: id.DeviceID }, ); });
📜 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 ignored due to path filters (5)
apps/desktop/src/assets/illustrations/display.pngis excluded by!**/*.pngpackages/ui-solid/icons/caret-down.svgis excluded by!**/*.svgpackages/ui-solid/icons/ellipsis-menu.svgis excluded by!**/*.svgpackages/ui-solid/icons/gear.svgis excluded by!**/*.svgpackages/ui-solid/icons/x.svgis excluded by!**/*.svg
📒 Files selected for processing (19)
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-tauri/src/windows.rs(1 hunks)apps/desktop/src/routes/(window-chrome)/CameraSelect.tsx(1 hunks)apps/desktop/src/routes/(window-chrome)/ChangeLogButton.tsx(1 hunks)apps/desktop/src/routes/(window-chrome)/InfoPill.tsx(1 hunks)apps/desktop/src/routes/(window-chrome)/MicrophoneSelect.tsx(1 hunks)apps/desktop/src/routes/(window-chrome)/SystemAudio.tsx(1 hunks)apps/desktop/src/routes/(window-chrome)/TargetSelectInfoPill.tsx(1 hunks)apps/desktop/src/routes/(window-chrome)/TargetTypeButton.tsx(1 hunks)apps/desktop/src/routes/(window-chrome)/new-main.tsx(6 hunks)apps/desktop/src/routes/(window-chrome)/settings/experimental.tsx(1 hunks)apps/desktop/src/routes/(window-chrome)/useRequestPermission.ts(1 hunks)apps/desktop/src/routes/target-select-overlay.tsx(6 hunks)apps/desktop/src/utils/tauri.ts(2 hunks)crates/displays/Cargo.toml(1 hunks)crates/displays/src/platform/win.rs(1 hunks)packages/ui-solid/src/auto-imports.d.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (12)
apps/desktop/src/routes/(window-chrome)/InfoPill.tsx (1)
apps/desktop/src/routes/(window-chrome)/(main).tsx (1)
InfoPill(1038-1053)
apps/desktop/src/routes/(window-chrome)/settings/experimental.tsx (1)
apps/desktop/src/routes/(window-chrome)/settings/Setting.tsx (1)
ToggleSetting(24-40)
apps/desktop/src/routes/(window-chrome)/CameraSelect.tsx (4)
apps/desktop/src/utils/tauri.ts (2)
CameraInfo(424-428)requestPermission(130-132)apps/desktop/src/utils/queries.ts (2)
createCurrentRecordingQuery(120-126)getPermissions(81-85)apps/desktop/src/routes/(window-chrome)/useRequestPermission.ts (1)
useRequestPermission(5-23)apps/desktop/src/routes/(window-chrome)/TargetSelectInfoPill.tsx (1)
TargetSelectInfoPill(3-33)
apps/desktop/src/routes/(window-chrome)/MicrophoneSelect.tsx (5)
apps/desktop/src/utils/queries.ts (2)
getPermissions(81-85)createCurrentRecordingQuery(120-126)apps/desktop/src/utils/tauri.ts (2)
requestPermission(130-132)events(312-354)apps/desktop/src/routes/(window-chrome)/useRequestPermission.ts (1)
useRequestPermission(5-23)apps/desktop/src/routes/(window-chrome)/TargetSelectInfoPill.tsx (1)
TargetSelectInfoPill(3-33)apps/desktop/src/routes/(window-chrome)/(main).tsx (1)
MicrophoneSelect(805-916)
apps/desktop/src/routes/(window-chrome)/useRequestPermission.ts (2)
apps/desktop/src/utils/tauri.ts (2)
requestPermission(130-132)commands(5-308)apps/desktop/src/utils/queries.ts (1)
getPermissions(81-85)
apps/desktop/src/routes/(window-chrome)/TargetSelectInfoPill.tsx (2)
apps/desktop/src/routes/(window-chrome)/InfoPill.tsx (1)
InfoPill(4-17)apps/desktop/src/routes/(window-chrome)/(main).tsx (1)
TargetSelectInfoPill(1005-1036)
apps/desktop/src/routes/(window-chrome)/SystemAudio.tsx (4)
apps/desktop/src/routes/(window-chrome)/OptionsContext.tsx (1)
useRecordingOptions(9-16)apps/desktop/src/utils/queries.ts (1)
createCurrentRecordingQuery(120-126)apps/desktop/src/routes/(window-chrome)/InfoPill.tsx (1)
InfoPill(4-17)apps/desktop/src/routes/(window-chrome)/(main).tsx (1)
SystemAudio(918-944)
apps/desktop/src/routes/(window-chrome)/ChangeLogButton.tsx (3)
apps/desktop/src/utils/web-api.ts (1)
apiClient(34-37)apps/desktop/src/utils/tauri.ts (1)
commands(5-308)apps/desktop/src/routes/(window-chrome)/(main).tsx (1)
ChangelogButton(1055-1127)
apps/desktop/src-tauri/src/lib.rs (1)
apps/desktop/src-tauri/src/target_select_overlay.rs (1)
display_information(119-130)
apps/desktop/src-tauri/src/target_select_overlay.rs (2)
apps/desktop/src/utils/tauri.ts (3)
DisplayId(520-520)DisplayInformation(521-525)PhysicalSize(642-642)crates/displays/src/lib.rs (3)
display(146-148)from_id(32-34)from_id(122-124)
apps/desktop/src/routes/target-select-overlay.tsx (2)
apps/desktop/src/utils/tauri.ts (2)
displayInformation(305-307)ScreenCaptureTarget(693-696)apps/desktop/src/utils/queries.ts (1)
createOptionsQuery(87-118)
apps/desktop/src/routes/(window-chrome)/new-main.tsx (7)
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)listAudioDevices(72-79)apps/desktop/src/utils/tauri.ts (2)
listAudioDevices(42-44)commands(5-308)apps/desktop/src/routes/(window-chrome)/Context.tsx (1)
WindowChromeHeader(29-41)apps/desktop/src/routes/(window-chrome)/CameraSelect.tsx (1)
CameraSelect(11-83)apps/desktop/src/routes/(window-chrome)/MicrophoneSelect.tsx (1)
MicrophoneSelect(12-118)apps/desktop/src/routes/(window-chrome)/SystemAudio.tsx (1)
SystemAudio(5-29)
⏰ 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 (8)
packages/ui-solid/src/auto-imports.d.ts (2)
6-6: Good call disabling Biome on a generated file.Prevents noisy lints where we shouldn’t hand‑edit.
14-15: Icon globals added are consistent with existing patterns.IconCapCaretDown, IconCapGear, and IconCapX declarations match other entries and should auto-import cleanly via unplugin. No issues.
Also applies to: 29-29, 60-60
apps/desktop/src-tauri/src/target_select_overlay.rs (1)
107-115: LGTM: simplified close API relies on Destroyed event to clean up.Closing overlay windows here will trigger the
Destroyedhandler inlib.rsto callWindowFocusManager::destroy, which unregisters Escape and cancels tasks. The flow is coherent.apps/desktop/src-tauri/src/lib.rs (1)
1986-1987: LGTM: command export fordisplay_information.The new Tauri command is correctly registered and will be included in Specta’s TS bindings.
apps/desktop/src/routes/(window-chrome)/TargetSelectInfoPill.tsx (1)
10-16: Pointer-down handling looks good.Stopping propagation only when permission is granted and a value exists matches the prior behavior and avoids blocking clicks that should fall through when the pill is effectively inactive.
apps/desktop/src/routes/(window-chrome)/SystemAudio.tsx (1)
18-18: No import required for IconPhMonitorBoldIconPhMonitorBold is declared in packages/ui-solid/src/auto-imports.d.ts (line 90) and auto-imported via your UI-Solid setup, so it’s globally available in TSX without an explicit import. You can safely ignore the missing import here.
apps/desktop/src/routes/(window-chrome)/MicrophoneSelect.tsx (1)
68-88: UX parity: requesting permission on button click (intentional change?)Previously, clicking the control requested microphone permission when not granted; now it always opens the menu and only the pill requests permission. Validate this UX change.
If you want the previous behavior, restore the early return:
onClick={() => { - Promise.all([ + if (!permissionGranted()) { + requestPermission("microphone"); + return; + } + Promise.all([ // ...apps/desktop/src/routes/(window-chrome)/CameraSelect.tsx (1)
64-64: IconCapCamera auto-import confirmed
TheIconCapCameracomponent is declared inpackages/ui-solid/src/auto-imports.d.ts(importing from"~icons/cap/camera.jsx"), so it’s provided globally via the existing auto-import setup. No explicit import is required here.
apps/desktop/src/routes/(window-chrome)/useRequestPermission.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
apps/desktop/src/routes/(window-chrome)/MicrophoneSelect.tsx (3)
67-71: Positioning fix: add relative/overflow-hidden to contain the absolute overlayThe blue level bar is absolutely positioned; without
relative overflow-hiddenon the button, it will misposition/bleed outside.- class="flex flex-row gap-2 items-center px-2 w-full h-9 rounded-lg transition-colors hover:bg-gray-3 bg-gray-2 disabled:text-gray-11 KSelect" + class="relative overflow-hidden flex flex-row gap-2 items-center px-2 w-full h-9 rounded-lg transition-colors hover:bg-gray-3 bg-gray-2 disabled:text-gray-11 KSelect"
60-63: Catch initialization errors to avoid unhandled promise rejectionsPrior discussion recommended awaiting/catching. Using
voiddrops errors.- void handleMicrophoneChange({ name: props.value }); + void handleMicrophoneChange({ name: props.value }, "auto") + .catch((err) => console.error("Failed to initialize microphone", err));
56-63: Thanks for restoring mount-time initializationThis fixes the regression noted earlier where live levels didn’t start for an already-selected mic.
🧹 Nitpick comments (6)
apps/desktop/src/routes/(window-chrome)/MicrophoneSelect.tsx (6)
53-55: Clamp audio level math to avoid NaN/negative valuesIf an event ever yields >0 dB, the current formula can produce a negative base for the square root →
NaN, resulting inright: "NaN%".- const audioLevel = () => - (1 - Math.max((dbs() ?? 0) + DB_SCALE, 0) / DB_SCALE) ** 0.5; + const audioLevel = () => { + // Normalize to [0,1] where 0 = loudest, 1 = silent (after transform) + const normalized = Math.min( + Math.max(((dbs() ?? -DB_SCALE) + DB_SCALE) / DB_SCALE, 0), + 1, + ); + return Math.sqrt(1 - normalized); + };If
audioInputLevelChangeguarantees <= 0 dB values, this is defensive but harmless; otherwise it prevents NaNs.
34-43: Differentiate telemetry for auto-init vs user actionMount-time initialization fires the same "microphone_selected" event as a user click, skewing analytics. Add an
originfield and default to"user".-const handleMicrophoneChange = async (item: Option | null) => { +const handleMicrophoneChange = async ( + item: Option | null, + origin: "user" | "auto" = "user", +) => { if (!props.options) return; props.onChange(item ? item.name : null); if (!item) setDbs(); trackEvent("microphone_selected", { microphone_name: item?.name ?? null, enabled: !!item, + origin, }); };Apply at mount (see Lines 60-63 below):
-void handleMicrophoneChange({ name: props.value }); +void handleMicrophoneChange({ name: props.value }, "auto");
71-89: Harden popup creation chain with error handlingAdd
voidand a terminal.catchto avoid unhandled promise rejections if menu creation/popup fails.- Promise.all([ + void Promise.all([ CheckMenuItem.new({ text: NO_MICROPHONE, checked: props.value === null, action: () => handleMicrophoneChange(null), }), PredefinedMenuItem.new({ item: "Separator" }), - ...(props.options ?? []).map((name) => + ...(props.options ?? []).map((name) => CheckMenuItem.new({ text: name, checked: name === props.value, action: () => handleMicrophoneChange({ name: name }), }), ), ]) .then((items) => Menu.new({ items })) .then((m) => { m.popup(); - }); + }) + .catch((err) => console.error("Failed to open mic menu", err));
50-50: Catch potential failure when unsubscribing event listenerIf
listenrejects orunsub()throws, you’ll get an unhandled rejection during cleanup.-onCleanup(() => result.then((unsub) => unsub())); +onCleanup(() => + result.then((unsub) => unsub()).catch((err) => { + console.error("Failed to unlisten audio level", err); + }), +);
35-36: Remove unnecessary guard that blocks clearing when options is empty
optionsis typed asstring[]; this check is unnecessary and could block clearing tonullin edge cases.- if (!props.options) return; props.onChange(item ? item.name : null);
45-48: Avoid name shadowing for readability in event callbackThe parameter name
dbsshadows thedbs()signal, which is confusing.-const result = events.audioInputLevelChange.listen((dbs) => { - if (!props.value) setDbs(); - else setDbs(dbs.payload); +const result = events.audioInputLevelChange.listen((evt) => { + if (!props.value) setDbs(); + else setDbs(evt.payload); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
apps/desktop/src/routes/(window-chrome)/MicrophoneSelect.tsx(1 hunks)apps/desktop/src/routes/target-select-overlay.tsx(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/routes/target-select-overlay.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
apps/desktop/src/routes/(window-chrome)/MicrophoneSelect.tsx (4)
apps/desktop/src/utils/queries.ts (2)
getPermissions(81-85)createCurrentRecordingQuery(120-126)apps/desktop/src/utils/tauri.ts (2)
requestPermission(130-132)events(312-354)apps/desktop/src/routes/(window-chrome)/useRequestPermission.ts (1)
useRequestPermission(5-23)apps/desktop/src/routes/(window-chrome)/TargetSelectInfoPill.tsx (1)
TargetSelectInfoPill(3-33)
⏰ 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: Analyze (javascript-typescript)
- GitHub Check: Analyze (actions)
- GitHub Check: Analyze (rust)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
🔇 Additional comments (2)
apps/desktop/src/routes/(window-chrome)/MicrophoneSelect.tsx (2)
78-84: Remove redundant fallback onprops.options
Sinceoptionsis declared asstring[]and every<MicrophoneSelect>usage already ensures a defined array (either[]or a non-undefined value via upstream fallbacks), the?? []in the.mapcall is unnecessary. You can simplify the code as follows:- ...(props.options ?? []).map((name) => + ...props.options.map((name) => CheckMenuItem.new({ text: name, checked: name === props.value, action: () => handleMicrophoneChange({ name }), }), ),This keeps the prop contract simple and makes the intent clearer.
1-9: Ignore manual import for IconCapMicrophone – it’s auto-importedSolid-Start is configured (in
packages/ui-solid/vite.js) to use unplugin-auto-import with an IconsResolver (prefix"Icon", custom collection"cap") alongside unplugin-icons loading thecapfolder. This setup automatically providesIconCapMicrophone(seepackages/ui-solid/src/auto-imports.d.ts), so no explicit import is needed and the code will compile as is.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/desktop/src/routes/target-select-overlay.tsx (1)
119-131: Fix: incorrect Solid Show usage for size and wrong unit label (should be Hz, not FPS).The inner Show callback receives the raw value, not an accessor. Calling size() will throw at runtime. Also guard refresh_rate and label it Hz. This was flagged previously and still applies.
- <Show when={display.physical_size}> - {(size) => ( - <span class="text-xs mb-2"> - {`${size().width}x${size().height} · ${display.refresh_rate}FPS`} - </span> - )} - </Show> + <Show when={display.physical_size} keyed> + {(size) => ( + <span class="text-xs mb-2"> + {`${size.width}x${size.height}${ + display.refresh_rate ? ` · ${display.refresh_rate}Hz` : "" + }`} + </span> + )} + </Show>
🧹 Nitpick comments (8)
apps/desktop/src/routes/target-select-overlay.tsx (8)
70-95: Windows DPI scaling: unify coordinate spaces (bounds vs. viewport) to fix position drift.Clamping/occluders/drag use window.innerWidth/innerHeight and bounds likely sourced from native events. On Windows with non-100% scaling, OS/native “logical” coordinates can diverge from CSS pixels, causing drift and misalignment. PR description lists this as a blocker.
Suggestions:
- Normalize all coordinates into the overlay’s CSS pixel space before clamping and rendering. Prefer the overlay window’s scale factor from Tauri instead of assuming devicePixelRatio:
- const sf = await getCurrentWindow().scaleFactor(); // per-monitor on Windows
- If native bounds are in physical pixels, divide by sf to get CSS px; if they’re logical, multiply accordingly. Keep it consistent everywhere (setBounds, occluders, drag, resize).
- Replace window.innerWidth/innerHeight with visualViewport.width/height or document.documentElement.clientWidth/Height to avoid edge cases with scrollbars and zoom, but the core fix is applying the same scale to both bounds and viewport.
- Consider reading the display’s scale (and/or logical/physical sizes) from your DisplayInformation payload and cache the chosen conversion alongside bounds.
To validate, instrument a quick overlay logger to print: { sf, viewportW/H, incomingBounds, cssBounds } and test on Windows at 100%, 125%, 150% across multiple monitors. If helpful, I can propose a focused patch after you confirm how targetUnderCursor.bounds are reported (physical vs logical).
Also applies to: 563-606, 516-556
151-156: Visual parity: apply the same “over” highlight for window targeting.The display-target path uses data-[over='true']:bg-blue-600/40 + transition-colors; the window-target container doesn’t, so highlight feedback is inconsistent.
- <div - data-over={targetUnderCursor.display_id === params.displayId} - class="relative w-screen h-screen bg-black/50" - > + <div + data-over={targetUnderCursor.display_id === params.displayId} + class="relative w-screen h-screen bg-black/50 data-[over='true']:bg-blue-600/40 transition-colors" + >
107-110: Don’t blanket-block all keydown defaults; narrowly prevent only problematic shortcuts.Preventing default for every key can interfere with accessibility and expected OS/browser behaviors. Target only combos like Ctrl/Cmd+P, S, W, etc.
-// This prevents browser keyboard shortcuts from firing. -// Eg. on Windows Ctrl+P would open the print dialog without this -createEventListener(document, "keydown", (e) => e.preventDefault()); +// Prevent only disruptive shortcuts (e.g., print/save) without blocking all keys. +createEventListener(document, "keydown", (e) => { + if (e.ctrlKey || e.metaKey) { + const k = e.key.toLowerCase(); + if (k === "p" || k === "s" || k === "w") { + e.preventDefault(); + } + } +});
703-751: Recording countdown UX: menus exist but countdown isn’t applied when starting.Menu updates generalSettingsStore.recordingCountdown, but Start Recording ignores it (noted as a blocker). Consider deferring commands.startRecording by the selected countdown and showing a brief inline countdown UI.
Minimal approach (pseudo-logic; wire to your store/getter accordingly):
onClick={() => { - commands.startRecording({ - capture_target: props.target, - mode: rawOptions.mode, - capture_system_audio: rawOptions.captureSystemAudio, - }); + const seconds = generalSettingsStore.get?.().recordingCountdown ?? 0; + if (seconds > 0) { + // Optionally: render a transient countdown overlay here + setTimeout(() => { + commands.startRecording({ + capture_target: props.target, + mode: rawOptions.mode, + capture_system_audio: rawOptions.captureSystemAudio ?? false, + }); + }, seconds * 1000); + } else { + commands.startRecording({ + capture_target: props.target, + mode: rawOptions.mode, + capture_system_audio: rawOptions.captureSystemAudio ?? false, + }); + } }}If you share the exact shape of generalSettingsStore (getter API), I can turn this into a precise patch and add a countdown display.
49-63: Solid Query nits: stabilize the key and reduce unnecessary refetches.Current key is ["displayId", params.displayId]; consider a more specific scope and disabling noisy refetches during overlay interactions.
-const displayInformation = createQuery(() => ({ - queryKey: ["displayId", params.displayId], +const displayInformation = createQuery(() => ({ + queryKey: ["displayInformation", 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); return null; } }, - enabled: - params.displayId !== undefined && rawOptions.targetMode === "display", + enabled: + params.displayId !== undefined && rawOptions.targetMode === "display", + refetchOnWindowFocus: false, + retry: 1, }))
251-286: Pointer events + capture simplify drag/resize and improve input device coverage.The current pattern adds/removes window mouse listeners per interaction. Pointer events with setPointerCapture reduce complexity, support pen/touch, and avoid global listeners.
Example for the draggable area:
- onMouseDown={(downEvent) => { - setDragging(true); - const startPosition = { ...bounds.position }; - createRoot((dispose) => { - createEventListenerMap(window, { - mousemove: (moveEvent) => { + onPointerDown={(downEvent) => { + (downEvent.currentTarget as HTMLElement).setPointerCapture(downEvent.pointerId); + setDragging(true); + const startPosition = { ...bounds.position }; + const onMove = (moveEvent: PointerEvent) => { const newPosition = { x: startPosition.x + moveEvent.clientX - downEvent.clientX, y: startPosition.y + moveEvent.clientY - downEvent.clientY, }; // clamp... _setBounds("position", newPosition); - }, - mouseup: () => { - setDragging(false); - dispose(); - }, - }); - }); + }; + const onUp = (upEvent: PointerEvent) => { + (upEvent.currentTarget as HTMLElement).releasePointerCapture(upEvent.pointerId); + setDragging(false); + downEvent.currentTarget.removeEventListener("pointermove", onMove as any); + downEvent.currentTarget.removeEventListener("pointerup", onUp as any); + }; + downEvent.currentTarget.addEventListener("pointermove", onMove as any); + downEvent.currentTarget.addEventListener("pointerup", onUp as any); }}You can apply the same pattern to ResizeHandles to eliminate the createOnMouseDown + window listeners.
Also applies to: 564-616
288-512: Reduce duplication in resize math with a single helper.All eight handles repeat similar limit/clamp logic. Extract a helper to compute next bounds from a corner/edge and the delta to improve maintainability and reduce bug surface.
Sketch:
type Corner = "nw" | "ne" | "sw" | "se" | "n" | "e" | "s" | "w"; function nextBoundsFromDrag(start: Bounds, delta: {x:number;y:number}, corner: Corner): Bounds { // compute new position/size, apply min-size constraints, return. }Then each onMouseDown can call setBounds(nextBoundsFromDrag(startBounds, delta, "nw")).
772-776: Remove dead code: getDisplayId() is unused.Nothing references this helper anymore.
-function getDisplayId(displayId: string | undefined) { - const id = Number(displayId); - if (Number.isNaN(id)) return 0; - return id; -} +// (removed) unused
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/desktop/src/routes/target-select-overlay.tsx(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/desktop/src/routes/target-select-overlay.tsx (3)
apps/desktop/src/utils/tauri.ts (3)
displayInformation(305-307)commands(5-308)ScreenCaptureTarget(693-696)apps/desktop/src/utils/queries.ts (1)
createOptionsQuery(87-118)apps/desktop/src/store.ts (1)
generalSettingsStore(61-62)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Analyze (rust)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/desktop/src-tauri/src/windows.rs (1)
284-288: Nit: document the “tiny placeholder size” rationale to aid future maintainersThe 100×100 at (0,0) is a pragmatic workaround to avoid wrong DPI sizing pre-move. Add a short comment so the intent is explicit (and not “optimized away” later).
#[cfg(windows)] { - window_builder = window_builder.inner_size(100.0, 100.0).position(0.0, 0.0); + // Start small and off to the side to avoid DPI mis-sizing before we can move the window + // to the target monitor and apply the correct scale factor (see Windows block post-build). + window_builder = window_builder.inner_size(100.0, 100.0).position(0.0, 0.0); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/desktop/src-tauri/src/windows.rs(1 hunks)
⏰ 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/windows.rs (3)
268-270: Overlay hardening (always_on_top + content_protected) is appropriate; please sanity-check UX on all OSesGood call to enforce both flags for the TargetSelectOverlay; this matches an overlay’s intent and reduces accidental capture. Please do a quick pass on:
- Windows: ensure overlay hover/click interactions remain responsive when other topmost windows are present (e.g., Teams, Zoom).
- macOS: confirm this doesn’t interfere with the level you set later (window level 45). They should compose, but worth verifying.
274-283: macOS: Pre-build logical geometry is correct for per-monitor DPIUsing the display’s logical_position and logical_size before build avoids initial flicker and ensures correct DPI. Looks good.
291-312: Apply Windows DPI refactor as proposedThe suggested changes correctly address the DPI‐scaling issues on Windows by:
- Moving the window via
set_positionbefore sizing so it adopts the target monitor’s scale factor.- Increasing the retry sleep from 5 ms to 16 ms to align with a 60 Hz frame interval.
- Verifying both width and height when comparing the expected vs. actual physical size, and reapplying the logical size on mismatch.
- Removing the now-unused
PhysicalSizeimport to keep imports clean.Once you’ve applied this refactor, confirm on Windows with a primary monitor at 100% scaling and a secondary at 150%/200% that the overlay covers the exact pixel area (no 1 px gaps) and that hit-testing at the edges is correct.
|
- plan indicator in header - fix some light/dark mode issues - add permanent dark overlay to display select - use default cursor instead of pointer
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (2)
apps/desktop/src/routes/target-select-overlay.tsx (2)
125-131: Fix incorrect Solid Show usage causing runtime errorInside the Show callback,
size()is called but the parameter is the raw value, not an accessor. This will cause a runtime error. The refresh rate should also be labeled as "Hz" instead of "FPS" and be conditionally rendered.Apply this diff:
- <Show when={display.physical_size}> - {(size) => ( - <span class="text-xs mb-2 text-white"> - {`${size().width}x${size().height} · ${display.refresh_rate}FPS`} - </span> - )} - </Show> + <Show when={display.physical_size} keyed> + {(size) => ( + <span class="text-xs mb-2 text-white"> + {`${size.width}x${size.height}${ + display.refresh_rate ? ` · ${display.refresh_rate}Hz` : "" + }`} + </span> + )} + </Show>
164-171: Fix incorrect Solid Show usage for window iconInside the Show callback,
icon()is called but the parameter is the raw value, not an accessor. This will cause a runtime error.Apply this diff:
- <Show when={windowUnderCursor.icon}> - {(icon) => ( - <img - src={icon()} - alt={`${windowUnderCursor.app_name} icon`} - class="mb-3 w-32 h-32 rounded-lg" - /> - )} - </Show> + <Show when={windowUnderCursor.icon} keyed> + {(icon) => ( + <img + src={icon} + alt={`${windowUnderCursor.app_name} icon`} + class="mb-3 w-32 h-32 rounded-lg" + /> + )} + </Show>
🧹 Nitpick comments (18)
apps/desktop/src/routes/(window-chrome).tsx (1)
82-85: Non-Windows header padding uses a magic number; verify intent with flex-row-reverse.You reduced the non-Windows leading padding to 4.2rem. With
flex-row-reverse, left padding may not visually correspond to the lead/trail edge. If intentional (to reserve space for traffic lights or draggable area), consider documenting/extracting the value to avoid regressions.Example:
- isWindows ? "flex-row" : "flex-row-reverse pl-[4.2rem]", + isWindows + ? "flex-row" + : cx("flex-row-reverse", NON_WINDOWS_TITLEBAR_LEADING_PAD) // 4.2rem reserved for platform controlsAnd define
const NON_WINDOWS_TITLEBAR_LEADING_PAD = "pl-[4.2rem]";near the component.Please quickly sanity-check on macOS/Linux that the reserved space is on the intended edge when using
flex-row-reverseand that dragging still works as expected.apps/desktop/src/routes/(window-chrome)/new-main/InfoPill.tsx (2)
4-16: Avoid leaking custom props to the DOM and allow class merging.
{...props}currently forwardsvariantandclassto the DOM.variantbecomes an invalid attribute on<button>, and externalclassis ignored because your explicitclasscomes after the spread. Destructure and merge instead.-export default function InfoPill( - props: ComponentProps<"button"> & { variant: "blue" | "red" }, -) { - return ( - <button - {...props} - type="button" - class={cx( - "px-2 py-0.5 rounded-full text-white text-[11px]", - props.variant === "blue" ? "bg-blue-9" : "bg-red-9", - )} - /> - ); -} +export default function InfoPill( + { variant, class: className, ...rest }: ComponentProps<"button"> & { variant: "blue" | "red" }, +) { + return ( + <button + {...rest} + type="button" + class={cx( + "px-2 py-0.5 rounded-full text-white text-[11px]", + variant === "blue" ? "bg-blue-9" : "bg-red-9", + className, + )} + /> + ); +}
12-14: Nit: consider accessible state if used as a toggle.If InfoPill represents an on/off state, consider optionally supporting
aria-pressedto improve screen reader UX.apps/desktop/src/routes/(window-chrome)/new-main/TargetTypeButton.tsx (3)
12-19: Prevent prop leakage to the DOM and support external class merging.
{...props}forwardsselected,Component, andnameto the<div>, producing invalid attributes. Destructure and only spread validdivprops; also merge consumer classes.-function TargetTypeButton( - props: { - selected: boolean; - Component: Component<ComponentProps<"svg">>; - name: string; - } & ComponentProps<"div">, -) { +function TargetTypeButton( + rawProps: { + selected: boolean; + Component: Component<ComponentProps<"svg">>; + name: string; + } & ComponentProps<"div">, +) { + const { selected, Component: Icon, name, class: className, ...divProps } = rawProps; return ( - <div - {...props} - class={cx( - "flex-1 text-center hover:bg-gray-4 bg-gray-3 flex flex-col ring-offset-gray-1 ring-offset-2 items-center justify-end gap-2 py-1.5 rounded-lg transition-all", - props.selected - ? "bg-gray-3 text-white ring-blue-9 ring-1" - : "ring-transparent ring-0", - )} - > - <props.Component + <div + {...divProps} + class={cx( + "flex-1 text-center hover:bg-gray-4 bg-gray-3 flex flex-col ring-offset-gray-1 ring-offset-2 items-center justify-end gap-2 py-1.5 rounded-lg transition-colors", + selected ? "text-white ring-blue-9 ring-1" : "ring-transparent ring-0", + className, + )} + > + <Icon class={cx( "size-6 transition-colors", - props.selected ? "text-gray-12" : "text-gray-9", + selected ? "text-gray-12" : "text-gray-9", )} /> - <p class="text-xs text-gray-12">{props.name}</p> + <p class="text-xs text-gray-12">{name}</p> </div> ); }
15-15: Prefer targeted transitions overtransition-all.
transition-allcan be costly;transition-colorsmatches the usage here.
4-10: Consider button semantics for accessibility.If this wrapper is clickable, prefer a
<button>or addrole="button"and keyboard handlers (Enter/Space) plustabIndex={0}for keyboard accessibility.apps/desktop/src/routes/(window-chrome)/new-main/ChangeLogButton.tsx (3)
22-34: Harden fetcher against network errors.If
apiClient.desktop.getChangelogStatusthrows, the resource enters an error state. Addtry/catchand return a safe fallback.- const [changelogStatus] = createResource( + const [changelogStatus] = createResource( () => currentVersion(), async (version) => { if (!version) { return { hasUpdate: false }; } - const response = await apiClient.desktop.getChangelogStatus({ - query: { version }, - }); - if (response.status === 200) return response.body; - return null; + try { + const response = await apiClient.desktop.getChangelogStatus({ query: { version } }); + if (response.status === 200) return response.body; + return { hasUpdate: false }; + } catch { + return { hasUpdate: false }; + } }, );
66-81: Add an accessible label to the icon-only button.Icon-only controls should expose an accessible name.
- <button + <button type="button" onClick={handleChangelogClick} - class="flex relative justify-center items-center size-5" + class="flex relative justify-center items-center size-5" + aria-label="Changelog" >
49-63: State naming may be misleading.
lastOpenedVersionis updated when an update is detected (not necessarily “opened”). Consider renaming tolastNotifiedVersion(no code change required in this PR) to reflect behavior and avoid future confusion.apps/desktop/src/routes/(window-chrome)/new-main/useRequestPermission.ts (1)
10-14: Reset only the relevant permission.Current branching is fine; consider a
switchfor readability and to guard future expansion.- if (type === "camera") { - await commands.resetCameraPermissions(); - } else if (type === "microphone") { - await commands.resetMicrophonePermissions(); - } + switch (type) { + case "camera": + await commands.resetCameraPermissions(); + break; + case "microphone": + await commands.resetMicrophonePermissions(); + break; + }apps/desktop/src/routes/(window-chrome)/new-main/SystemAudio.tsx (2)
16-16: Typo in class name prevents expected cursor styling.
curosr-default→cursor-default.- class="curosr-default flex flex-row gap-2 items-center px-2 w-full h-9 rounded-lg transition-colors bg-gray-3 disabled:text-gray-11 KSelect" + class="cursor-default flex flex-row gap-2 items-center px-2 w-full h-9 rounded-lg transition-colors bg-gray-3 disabled:text-gray-11 KSelect"
9-15: Prevent state toggles when disabled (defensive).Button is disabled, but it’s cheap to guard in the handler to avoid accidental toggles from programmatic clicks.
- onClick={() => { - if (!rawOptions) return; + onClick={() => { + if (!rawOptions || !!currentRecording.data) return; setOptions({ captureSystemAudio: !rawOptions.captureSystemAudio }); }}apps/desktop/src/routes/(window-chrome)/new-main/MicrophoneSelect.tsx (3)
34-38: Remove redundant guard that blocks clearing selection in edge cases.
if (!props.options) return;can prevent “No Microphone” from applying if options momentarily resolve to undefined.- const handleMicrophoneChange = async (item: Option | null) => { - if (!props.options) return; + const handleMicrophoneChange = async (item: Option | null) => { props.onChange(item ? item.name : null); if (!item) setDbs();
45-51: Attach input-level listener in onMount for symmetry with cleanup.Works as-is, but colocating subscribe/unsubscribe improves lifecycle clarity.
- const result = events.audioInputLevelChange.listen((dbs) => { - if (!props.value) setDbs(); - else setDbs(dbs.payload); - }); - - onCleanup(() => result.then((unsub) => unsub())); + onMount(() => { + const p = events.audioInputLevelChange.listen((dbs) => { + if (!props.value) setDbs(); + else setDbs(dbs.payload); + }); + onCleanup(() => p.then((unsub) => unsub())); + });
92-101: Show meter when level is 0 dB (edge visualization).
<Show when={dbs()}>hides the bar for falsy values; if 0 ever occurs, it won’t render. Considerwhen={dbs() !== undefined}.- <Show when={dbs()}> + <Show when={dbs() !== undefined}>apps/desktop/src/routes/(window-chrome)/new-main/CameraSelect.tsx (1)
68-79: Minor UX: don’t clear selection if already null.Early return helps avoid unnecessary renders.
onClick={(e) => { - if (!props.options) return; - if (props.value !== null) { + if (!props.options || props.value === null) return; e.stopPropagation(); props.onChange(null); - } }}apps/desktop/src/routes/(window-chrome)/new-main/index.tsx (2)
134-140: Continuous size enforcement on resize can create event loops and fight users.Setting size inside
onResizedwill re-trigger the event on many platforms. Consider removing this handler or debounce and only correct when dimensions deviate significantly.Example (debounced check):
let resizeTimer: number | undefined; const unlistenResize = currentWindow.onResized(() => { clearTimeout(resizeTimer); resizeTimer = window.setTimeout(async () => { const expected = getWindowSize(); const { width, height } = await currentWindow.innerSize(); if (width !== expected.width || height !== expected.height) { await currentWindow.setSize(new LogicalSize(expected.width, expected.height)); } }, 150); });
146-148: Remove unused primaryMonitor call.
monitorisn’t used; drop this block to avoid unnecessary async work.- const monitor = await primaryMonitor(); - if (!monitor) return;
📜 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 ignored due to path filters (1)
packages/ui-solid/icons/gear.svgis excluded by!**/*.svg
📒 Files selected for processing (12)
apps/desktop/src/routes/(window-chrome).tsx(1 hunks)apps/desktop/src/routes/(window-chrome)/callback.template.ts(0 hunks)apps/desktop/src/routes/(window-chrome)/new-main/CameraSelect.tsx(1 hunks)apps/desktop/src/routes/(window-chrome)/new-main/ChangeLogButton.tsx(1 hunks)apps/desktop/src/routes/(window-chrome)/new-main/InfoPill.tsx(1 hunks)apps/desktop/src/routes/(window-chrome)/new-main/MicrophoneSelect.tsx(1 hunks)apps/desktop/src/routes/(window-chrome)/new-main/SystemAudio.tsx(1 hunks)apps/desktop/src/routes/(window-chrome)/new-main/TargetSelectInfoPill.tsx(1 hunks)apps/desktop/src/routes/(window-chrome)/new-main/TargetTypeButton.tsx(1 hunks)apps/desktop/src/routes/(window-chrome)/new-main/index.tsx(1 hunks)apps/desktop/src/routes/(window-chrome)/new-main/useRequestPermission.ts(1 hunks)apps/desktop/src/routes/target-select-overlay.tsx(6 hunks)
💤 Files with no reviewable changes (1)
- apps/desktop/src/routes/(window-chrome)/callback.template.ts
🧰 Additional context used
🧬 Code graph analysis (10)
apps/desktop/src/routes/(window-chrome)/new-main/ChangeLogButton.tsx (4)
apps/desktop/src/utils/web-api.ts (1)
apiClient(34-37)apps/desktop/src/utils/tauri.ts (1)
commands(5-308)apps/desktop/src/routes/(window-chrome)/new-main.tsx (1)
ChangelogButton(658-730)apps/desktop/src/routes/(window-chrome)/(main).tsx (1)
ChangelogButton(1055-1127)
apps/desktop/src/routes/(window-chrome)/new-main/TargetSelectInfoPill.tsx (3)
apps/desktop/src/routes/(window-chrome)/new-main/InfoPill.tsx (1)
InfoPill(4-17)apps/desktop/src/routes/(window-chrome)/new-main.tsx (1)
TargetSelectInfoPill(609-639)apps/desktop/src/routes/(window-chrome)/(main).tsx (1)
TargetSelectInfoPill(1005-1036)
apps/desktop/src/routes/(window-chrome)/new-main/TargetTypeButton.tsx (1)
apps/desktop/src/routes/(window-chrome)/new-main.tsx (1)
TargetTypeButton(732-751)
apps/desktop/src/routes/(window-chrome)/new-main/index.tsx (7)
apps/desktop/src/utils/tauri.ts (5)
CameraInfo(424-428)DeviceOrModelID(519-519)commands(5-308)ScreenCaptureTarget(693-696)setMicInput(6-8)apps/desktop/src/store.ts (1)
generalSettingsStore(61-62)apps/desktop/src/routes/(window-chrome)/OptionsContext.tsx (2)
RecordingOptionsProvider(18-18)useRecordingOptions(9-16)apps/desktop/src/utils/queries.ts (5)
listScreens(41-46)listWindows(24-39)listVideoDevices(53-58)createCameraMutation(152-175)createLicenseQuery(128-150)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/(window-chrome)/new-main/SystemAudio.tsx (5)
apps/desktop/src/routes/(window-chrome)/OptionsContext.tsx (1)
useRecordingOptions(9-16)apps/desktop/src/utils/queries.ts (1)
createCurrentRecordingQuery(120-126)apps/desktop/src/routes/(window-chrome)/new-main/InfoPill.tsx (1)
InfoPill(4-17)apps/desktop/src/routes/(window-chrome)/new-main.tsx (1)
SystemAudio(581-607)apps/desktop/src/routes/(window-chrome)/(main).tsx (1)
SystemAudio(918-944)
apps/desktop/src/routes/(window-chrome)/new-main/CameraSelect.tsx (4)
apps/desktop/src/utils/tauri.ts (2)
CameraInfo(424-428)requestPermission(130-132)apps/desktop/src/utils/queries.ts (2)
createCurrentRecordingQuery(120-126)getPermissions(81-85)apps/desktop/src/routes/(window-chrome)/new-main/useRequestPermission.ts (1)
useRequestPermission(5-23)apps/desktop/src/routes/(window-chrome)/new-main/TargetSelectInfoPill.tsx (1)
TargetSelectInfoPill(3-33)
apps/desktop/src/routes/(window-chrome)/new-main/InfoPill.tsx (2)
apps/desktop/src/routes/(window-chrome)/new-main.tsx (1)
InfoPill(641-656)apps/desktop/src/routes/(window-chrome)/(main).tsx (1)
InfoPill(1038-1053)
apps/desktop/src/routes/target-select-overlay.tsx (3)
apps/desktop/src/utils/tauri.ts (2)
displayInformation(305-307)ScreenCaptureTarget(693-696)apps/desktop/src/utils/queries.ts (1)
createOptionsQuery(87-118)apps/desktop/src/store.ts (1)
generalSettingsStore(61-62)
apps/desktop/src/routes/(window-chrome)/new-main/useRequestPermission.ts (2)
apps/desktop/src/utils/tauri.ts (2)
requestPermission(130-132)commands(5-308)apps/desktop/src/utils/queries.ts (1)
getPermissions(81-85)
apps/desktop/src/routes/(window-chrome)/new-main/MicrophoneSelect.tsx (4)
apps/desktop/src/utils/queries.ts (2)
getPermissions(81-85)createCurrentRecordingQuery(120-126)apps/desktop/src/utils/tauri.ts (2)
requestPermission(130-132)events(312-354)apps/desktop/src/routes/(window-chrome)/new-main/useRequestPermission.ts (1)
useRequestPermission(5-23)apps/desktop/src/routes/(window-chrome)/new-main/TargetSelectInfoPill.tsx (1)
TargetSelectInfoPill(3-33)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (11)
apps/desktop/src/routes/(window-chrome)/new-main/ChangeLogButton.tsx (1)
72-72: Ignore manual import for IconLucideBell—it's auto-importedThe
IconLucideBellcomponent is already made available to all.tsxfiles via our icons unplugin (seepackages/ui-solid/src/auto-imports.d.ts, which mapsIconLucideBellto~icons/lucide/bell.jsx). You can confirm it’s used inapps/desktop/src/routes/(window-chrome)/(main).tsxwithout an explicit import, and it compiles successfully there.No change is needed in
ChangeLogButton.tsx. If you encounter a build error, please verify that your Vite/Rollup config in the apps/desktop project includes the icons plugin under the~iconsalias rather than adding a manual import fromlucide-solid.Likely an incorrect or invalid review comment.
apps/desktop/src/routes/(window-chrome)/new-main/index.tsx (1)
245-250: Handle setMicInput failures and keep state consistent.If the backend call fails, the UI will still update. Revert on error.
const setMicInput = createMutation(() => ({ mutationFn: async (name: string | null) => { - await commands.setMicInput(name); - setOptions("micName", name); + const before = rawOptions.micName ?? null; + setOptions("micName", name); + try { + await commands.setMicInput(name); + } catch (e) { + setOptions("micName", before); + throw e; + } }, }));Likely an incorrect or invalid review comment.
apps/desktop/src/routes/target-select-overlay.tsx (9)
48-62: Proper display information query setupThe display information query is well-structured with proper error handling, conditional enabling, and null return for missing display ID.
64-94: Improved bounds management with clampingThe bounds clamping logic correctly ensures minimum dimensions of 150px and prevents overflow beyond viewport boundaries. The implementation is solid and handles edge cases well.
220-231: Smart placement calculation for recording controlsThe dynamic placement logic to avoid bottom overflow is well-implemented, properly reading reactive dependencies and calculating the placement based on bounds and control height.
250-285: Improved drag handling with throttlingThe mouse event handling with requestAnimationFrame throttling is a good performance optimization. The cleanup logic and event handling are properly implemented.
287-513: Comprehensive resize handle implementationThe resize handles cover all corners and edges with proper cursor styles and mathematical calculations. The implementation correctly handles minimum size constraints and boundary calculations.
515-556: Occluders provide proper visual feedbackThe occluder divs create the proper "selection area" effect by covering the non-selected areas with a semi-transparent overlay. The calculations are correct for all four regions.
573-614: Draggable area with boundary constraintsThe drag implementation properly constrains the selection area within the viewport boundaries and uses proper event cleanup patterns with createRoot.
654-700: Menu integration for recording modes and countdownThe Tauri Menu integration for mode switching and countdown selection is well-structured, providing a clean UI for configuration options.
703-755: Enhanced recording controls UIThe recording controls provide a comprehensive interface with mode selection, close functionality, and settings access. The UI is well-structured with proper hover states and visual feedback.
| const onChange = (cameraLabel: CameraInfo | null) => { | ||
| if (!cameraLabel && permissions?.data?.camera !== "granted") | ||
| return requestPermission("camera"); | ||
|
|
||
| props.onChange(cameraLabel); | ||
|
|
||
| trackEvent("camera_selected", { | ||
| camera_name: cameraLabel?.display_name ?? null, | ||
| enabled: !!cameraLabel, | ||
| }); | ||
| }; |
There was a problem hiding this comment.
Permission gating is inverted — permission request won’t run when selecting a camera.
Currently, permission is requested only when selecting “No Camera”. It should request when enabling a camera and permission is not yet granted.
Apply this diff:
- const onChange = (cameraLabel: CameraInfo | null) => {
- if (!cameraLabel && permissions?.data?.camera !== "granted")
- return requestPermission("camera");
-
- props.onChange(cameraLabel);
-
- trackEvent("camera_selected", {
- camera_name: cameraLabel?.display_name ?? null,
- enabled: !!cameraLabel,
+ const onChange = (camera: CameraInfo | null) => {
+ if (camera && permissions?.data?.camera !== "granted") {
+ return requestPermission("camera");
+ }
+
+ props.onChange(camera);
+
+ trackEvent("camera_selected", {
+ camera_name: camera?.display_name ?? null,
+ enabled: !!camera,
});
};📝 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.
| const onChange = (cameraLabel: CameraInfo | null) => { | |
| if (!cameraLabel && permissions?.data?.camera !== "granted") | |
| return requestPermission("camera"); | |
| props.onChange(cameraLabel); | |
| trackEvent("camera_selected", { | |
| camera_name: cameraLabel?.display_name ?? null, | |
| enabled: !!cameraLabel, | |
| }); | |
| }; | |
| const onChange = (camera: CameraInfo | null) => { | |
| if (camera && permissions?.data?.camera !== "granted") { | |
| return requestPermission("camera"); | |
| } | |
| props.onChange(camera); | |
| trackEvent("camera_selected", { | |
| camera_name: camera?.display_name ?? null, | |
| enabled: !!camera, | |
| }); | |
| }; |
🤖 Prompt for AI Agents
In apps/desktop/src/routes/(window-chrome)/new-main/CameraSelect.tsx around
lines 25 to 35, the permission check is inverted: it currently requests
permission only when selecting "No Camera". Change the condition to request
permission when a camera is being enabled but camera permission isn't granted
(i.e., if (cameraLabel && permissions?.data?.camera !== "granted") return
requestPermission("camera");), then keep calling props.onChange(cameraLabel) and
tracking the event after permission handling.
| ...props.options.map((o) => | ||
| CheckMenuItem.new({ | ||
| text: o.display_name, | ||
| checked: o === props.value, | ||
| action: () => onChange(o), | ||
| }), | ||
| ), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid reference equality for checked menu state.
Comparing objects by reference is brittle across re-fetches. Prefer stable IDs.
- ...props.options.map((o) =>
+ ...props.options.map((o) =>
CheckMenuItem.new({
text: o.display_name,
- checked: o === props.value,
- action: () => onChange(o),
+ checked:
+ (o.model_id && props.value?.model_id
+ ? o.model_id === props.value.model_id
+ : o.device_id === props.value?.device_id),
+ action: () => onChange(o),
}),
),📝 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.
| ...props.options.map((o) => | |
| CheckMenuItem.new({ | |
| text: o.display_name, | |
| checked: o === props.value, | |
| action: () => onChange(o), | |
| }), | |
| ), | |
| ...props.options.map((o) => | |
| CheckMenuItem.new({ | |
| text: o.display_name, | |
| checked: | |
| (o.model_id && props.value?.model_id | |
| ? o.model_id === props.value.model_id | |
| : o.device_id === props.value?.device_id), | |
| action: () => onChange(o), | |
| }), | |
| ), |
| if (rawOptions.targetMode) commands.openTargetSelectOverlays(); | ||
| else commands.closeTargetSelectOverlays(); | ||
| }); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm overlay open/close lifecycle matches expected UX.
Effectually toggling overlays on every targetMode change is fine; verify overlays also close on countdown start to address the PR comment (“overlay doesn't close until after the countdown finishes”).
🏁 Script executed:
#!/bin/bash
# Find where overlays are opened/closed and whether we also close them on recording start/countdown.
rg -nP -C3 '(openTargetSelectOverlays|closeTargetSelectOverlays|recordingStarted|recordingEvent|countdown)' apps/desktopLength of output: 16554
Add overlay close on countdown start to match expected UX
The current effect in apps/desktop/src/routes/(window-chrome)/new-main/index.tsx only opens or closes the target-select overlays when rawOptions.targetMode toggles. To prevent the overlay from lingering during the pre-recording countdown, we need to close it as soon as the countdown begins.
• In new-main/index.tsx (around lines 151–153), keep the existing createEffect for targetMode.
• Add a listener for the recording countdown event and invoke commands.closeTargetSelectOverlays() when playback transitions into the countdown phase. For example:
import { commands, events, createTauriEventListener } from "~/utils/tauri";
+ import { createEffect } from "solid-js";
// existing effect toggles overlays based on targetMode
createEffect(() => {
if (rawOptions.targetMode) commands.openTargetSelectOverlays();
else commands.closeTargetSelectOverlays();
});
+ // close overlays immediately when countdown starts
+ createTauriEventListener(events.recordingEvent, (payload) => {
+ if (payload.variant === "Countdown") {
+ commands.closeTargetSelectOverlays();
+ }
+ });This ensures the overlay closes as soon as the countdown begins, addressing the issue where it previously remained visible until after the countdown finished.
📝 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.
| if (rawOptions.targetMode) commands.openTargetSelectOverlays(); | |
| else commands.closeTargetSelectOverlays(); | |
| }); | |
| import { commands, events, createTauriEventListener } from "~/utils/tauri"; | |
| import { createEffect } from "solid-js"; | |
| // existing effect toggles overlays based on targetMode | |
| createEffect(() => { | |
| if (rawOptions.targetMode) commands.openTargetSelectOverlays(); | |
| else commands.closeTargetSelectOverlays(); | |
| }); | |
| // close overlays immediately when countdown starts | |
| createTauriEventListener(events.recordingEvent, (payload) => { | |
| if (payload.variant === "Countdown") { | |
| commands.closeTargetSelectOverlays(); | |
| } | |
| }); |
🤖 Prompt for AI Agents
In apps/desktop/src/routes/(window-chrome)/new-main/index.tsx around lines
151–153, keep the existing createEffect that opens/closes overlays based on
rawOptions.targetMode, but also subscribe to the recording countdown playback
event and call commands.closeTargetSelectOverlays() when playback transitions
into the countdown/pre-recording state; add a listener (or hook into the
existing playback/recorder event emitter) that detects the countdown start and
invokes commands.closeTargetSelectOverlays(), then clean up the listener on
unmount to avoid leaks.
| dir={ostype() === "windows" ? "rtl" : "rtl"} | ||
| class="flex gap-1 items-center mx-2 w-full" |
There was a problem hiding this comment.
UI direction is always RTL; likely a typo.
dir={ostype() === "windows" ? "rtl" : "rtl"} forces RTL everywhere. Use LTR for non-Windows if that was the intent.
- dir={ostype() === "windows" ? "rtl" : "rtl"}
+ dir={ostype() === "windows" ? "rtl" : "ltr"}📝 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.
| dir={ostype() === "windows" ? "rtl" : "rtl"} | |
| class="flex gap-1 items-center mx-2 w-full" | |
| @@ apps/desktop/src/routes/(window-chrome)/new-main/index.tsx | |
| dir={ostype() === "windows" ? "rtl" : "ltr"} | |
| class="flex gap-1 items-center mx-2 w-full" |
🤖 Prompt for AI Agents
In apps/desktop/src/routes/(window-chrome)/new-main/index.tsx around lines
268-269, the dir prop is set to rtl for both branches (dir={ostype() ===
"windows" ? "rtl" : "rtl"}) which forces RTL everywhere; change the non-Windows
branch to "ltr" so it reads dir={ostype() === "windows" ? "rtl" : "ltr"} (or
simplify to default "ltr" and only set "rtl" when ostype() === "windows") to
restore correct text direction.
| onClick={(e) => { | ||
| if (!props.permissionGranted) { | ||
| props.requestPermission(); | ||
| return; | ||
| } | ||
|
|
||
| props.onClick(e); | ||
| }} |
There was a problem hiding this comment.
Clicking “Request Permission” may bubble and trigger parent handlers.
In the “no permission” branch, the previous inline implementation stopped propagation to avoid triggering parent click handlers. Restore e.stopPropagation() there.
- onClick={(e) => {
- if (!props.permissionGranted) {
- props.requestPermission();
- return;
- }
-
- props.onClick(e);
- }}
+ onClick={(e) => {
+ if (!props.permissionGranted) {
+ e.stopPropagation();
+ props.requestPermission();
+ return;
+ }
+ props.onClick(e);
+ }}📝 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.
| onClick={(e) => { | |
| if (!props.permissionGranted) { | |
| props.requestPermission(); | |
| return; | |
| } | |
| props.onClick(e); | |
| }} | |
| onClick={(e) => { | |
| if (!props.permissionGranted) { | |
| e.stopPropagation(); | |
| props.requestPermission(); | |
| return; | |
| } | |
| props.onClick(e); | |
| }} |
🤖 Prompt for AI Agents
In apps/desktop/src/routes/(window-chrome)/new-main/TargetSelectInfoPill.tsx
around lines 17 to 24, the "no permission" branch calls
props.requestPermission() but doesn't stop the click event from bubbling which
can trigger parent handlers; update the onClick handler so that when
!props.permissionGranted you first call e.stopPropagation(), then
props.requestPermission(), and then return to prevent further propagation and
avoid invoking parent click handlers.
| await commands.requestPermission(type); | ||
| await queryClient.refetchQueries(getPermissions); | ||
| } catch (error) { |
There was a problem hiding this comment.
Fix refetch to target the correct query key (v5 API).
queryClient.refetchQueries(getPermissions) passes a queryOptions object where QueryFilters are expected. Refetch by queryKey instead.
Apply this diff:
- await queryClient.refetchQueries(getPermissions);
+ await queryClient.refetchQueries({ queryKey: getPermissions.queryKey });Alternatively, invalidateQueries({ queryKey: getPermissions.queryKey }) is fine if you prefer a lazy refetch.
📝 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.
| await commands.requestPermission(type); | |
| await queryClient.refetchQueries(getPermissions); | |
| } catch (error) { | |
| await commands.requestPermission(type); | |
| await queryClient.refetchQueries({ queryKey: getPermissions.queryKey }); | |
| } catch (error) { |
🤖 Prompt for AI Agents
In apps/desktop/src/routes/(window-chrome)/new-main/useRequestPermission.ts
around lines 15 to 17, the call queryClient.refetchQueries(getPermissions) is
passing the function rather than a queryKey; replace it with
queryClient.refetchQueries({ queryKey: getPermissions.queryKey }) or,
alternatively, use queryClient.invalidateQueries({ queryKey:
getPermissions.queryKey }) to target the correct v5 queryKey for refetching.
| } catch (error) { | ||
| console.error(`Failed to get ${type} permission:`, error); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Bubble errors or return a status so callers can react.
Swallowing errors makes the UI assume success. Either rethrow or return a boolean.
Apply one of these diffs:
Option A — rethrow:
- } catch (error) {
- console.error(`Failed to get ${type} permission:`, error);
- }
+ } catch (error) {
+ console.error(`Failed to get ${type} permission:`, error);
+ throw error;
+ }Option B — return status:
- async function requestPermission(type: "camera" | "microphone") {
+ async function requestPermission(type: "camera" | "microphone"): Promise<boolean> {
try {
...
- await queryClient.refetchQueries({ queryKey: getPermissions.queryKey });
+ await queryClient.refetchQueries({ queryKey: getPermissions.queryKey });
+ return true;
} catch (error) {
console.error(`Failed to get ${type} permission:`, error);
+ return false;
}
}🤖 Prompt for AI Agents
In apps/desktop/src/routes/(window-chrome)/new-main/useRequestPermission.ts
around lines 17-19, the catch currently swallows errors which makes callers
assume success; change it to either rethrow the caught error (after logging) so
upstream can handle it, or change the function to return a status boolean and
return false from the catch (and true on success) — ensure the function
signature and all call sites reflect the Promise<boolean> return if you choose
the status approach, and keep the console.error but do not suppress the error
when rethrowing.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
crates/scap-targets/src/platform/win.rs (3)
124-132: Fix invalid GetDpiForMonitor call: cannot take &mut 0; missing dpi_y out paramThe call passes
&mut 0as the Y-DPI pointer, which won’t compile and is undefined behavior even if coerced. Create a mutabledpi_yand pass its address. Also, usedpi_xfor scale.Apply this diff:
- let dpi = unsafe { - let mut dpi_x = 0; - GetDpiForMonitor(self.0, MDT_EFFECTIVE_DPI, &mut dpi_x, &mut 0).ok()?; - dpi_x - }; + let dpi = unsafe { + let mut dpi_x: u32 = 0; + let mut dpi_y: u32 = 0; + GetDpiForMonitor(self.0, MDT_EFFECTIVE_DPI, &mut dpi_x, &mut dpi_y).ok()?; + dpi_x + };
838-846: Use stable pointer ops for DwmGetWindowAttribute and bring size_of into scope
(&raw mut rect).cast()is nightly-only syntax andsize_of::<RECT>()isn’t in scope (should bemem::size_of). Useaddr_of_mut!or a simple&mut rect as *mut _cast withmem::size_of.Apply this diff:
- DwmGetWindowAttribute( - self.0, - DWMWA_EXTENDED_FRAME_BOUNDS, - (&raw mut rect).cast(), - size_of::<RECT>() as u32, - ) + DwmGetWindowAttribute( + self.0, + DWMWA_EXTENDED_FRAME_BOUNDS, + (std::ptr::addr_of_mut!(rect)).cast(), + mem::size_of::<RECT>() as u32, + )
875-882: Same DwmGetWindowAttribute issue repeatedMirror the fix in physical_bounds(): avoid
&rawand qualifysize_of.Apply this diff:
- DwmGetWindowAttribute( - self.0, - DWMWA_EXTENDED_FRAME_BOUNDS, - (&raw mut rect).cast(), - size_of::<RECT>() as u32, - ) + DwmGetWindowAttribute( + self.0, + DWMWA_EXTENDED_FRAME_BOUNDS, + (std::ptr::addr_of_mut!(rect)).cast(), + mem::size_of::<RECT>() as u32, + )apps/desktop/src-tauri/src/fake_window.rs (1)
61-69: Fix coordinate-space mismatch (global vs window-local) causing incorrect hit-testing
cursor_position()returns a position relative to the window’s client area. Addingouter_position()(screen coords) mixes spaces and breaks comparisons, especially on Windows with DPI scaling. Convert the mouse position to logical window coords and compare directly againstLogicalBounds(which are logical). This likely explains the overlay interactivity being “wacky” and click-through not toggling as intended.Apply this diff:
- let (Ok(window_position), Ok(mouse_position), Ok(scale_factor)) = ( - window.outer_position(), - window.cursor_position(), - window.scale_factor(), - ) else { + let (Ok(mouse_position), Ok(scale_factor)) = ( + window.cursor_position(), + window.scale_factor(), + ) else { let _ = window.set_ignore_cursor_events(true); continue; }; @@ - for bounds in windows.values() { - let x_min = (window_position.x as f64) + bounds.position().x() * scale_factor; - let x_max = (window_position.x as f64) - + (bounds.position().x() + bounds.size().width()) * scale_factor; - let y_min = (window_position.y as f64) + bounds.position().y() * scale_factor; - let y_max = (window_position.y as f64) - + (bounds.position().y() + bounds.size().height()) * scale_factor; - - if mouse_position.x >= x_min - && mouse_position.x <= x_max - && mouse_position.y >= y_min - && mouse_position.y <= y_max - { + for bounds in windows.values() { + // Convert mouse to logical window coordinates + let mx = mouse_position.x / scale_factor; + let my = mouse_position.y / scale_factor; + + let x_min = bounds.position().x(); + let x_max = bounds.position().x() + bounds.size().width(); + let y_min = bounds.position().y(); + let y_max = bounds.position().y() + bounds.size().height(); + + if mx >= x_min && mx <= x_max && my >= y_min && my <= y_max { ignore = false; // ShowCapturesPanel.emit(&app).ok(); break; } }Also applies to: 72-89
apps/cli/src/record.rs (3)
46-55: Replace todo!() to avoid runtime panic when --camera is suppliedPassing
--cameracurrently triggers a panic viatodo!(). Fail fast with a clear error or explicitly ignore (with a warning). Returning an error is safer.Apply this diff to fail gracefully:
- let camera = if let Some(model_id) = self.camera { - let _model_id: ModelID = model_id - .try_into() - .map_err(|_| "Invalid model ID".to_string())?; - - todo!() - // Some(CameraFeed::init(model_id).await.unwrap()) - } else { - None - }; + let camera = if let Some(model_id) = self.camera { + // Validate format now; feature not yet implemented + ModelID::try_from(model_id.as_str()) + .map_err(|_| "Invalid model ID".to_string())?; + return Err("--camera is not supported in this build yet; please omit it for now.".into()); + } else { + None + };
78-81: Borrowing a temporary String across.awaitwill not compile
read_line(&mut String::new()).awaitborrows a temporary across an.await, which the compiler will reject. Allocate the buffer first.- tokio::io::BufReader::new(tokio::io::stdin()) - .read_line(&mut String::new()) - .await - .unwrap(); + let mut line = String::new(); + tokio::io::BufReader::new(tokio::io::stdin()) + .read_line(&mut line) + .await + .unwrap();
62-74: Unnecessary--micflag and brittle borrow of a temporaryIt looks like you’re parsing
--micintoself.micbut never actually using it—this will confuse users expecting microphone capture. At the same time, doingmic_feed: &Noneborrows a temporary that only lives for the duration of the call expression, which is safe today but easy to break if you ever change the async/await or borrowing patterns.– Use or remove the
--micflag. If you intend to support mic capture in the CLI, turn theu32ID into anAudioInputFeedbefore spawning the actor:let mic_feed: Option<AudioInputFeed> = if let Some(mic_id) = self.mic { // initialize the audio feed from the device ID Some(AudioInputFeed::init(mic_id).await.map_err(|e| e.to_string())?) } else { None };Then pass
&mic_feedintoRecordingBaseInputs. Otherwise, drop the#[arg(long)] mic: Option<u32>field entirely.– Avoid borrowing a temporary. Instead of
cap_recording::RecordingBaseInputs { …, mic_feed: &None }hoist a local variable so the reference clearly outlives the call:
let mic_feed: Option<AudioInputFeed> = None; // or initialized above let actor = spawn_studio_recording_actor( id, path, RecordingBaseInputs { capture_target, capture_system_audio, mic_feed: &mic_feed }, …, ) .await?;– Optional refactor: Consider changing
RecordingBaseInputs<'a>to own itsOption<AudioInputFeed>(mic_feed: Option<AudioInputFeed>) and let callers move in the value. That will remove any lifetime gymnastics around&Noneor&mic_feed.These changes will clear up the unused‐flag bug and eliminate the fragile borrow of a temporary.
crates/cursor-capture/src/position.rs (1)
208-356: Tests are out of sync with the new NormalizedCursorPosition APIThe tests still construct NormalizedCursorPosition with crop_position/crop_size and call with_crop(position, size), which no longer exist. The struct has a single crop: CursorCropBounds and with_crop(crop: CursorCropBounds).
Apply the following focused updates to the tests:
@@ mod tests { use super::*; - use scap_targets::Display; + use scap_targets::Display; @@ fn test_with_crop_no_change() { let display = mock_display(); let original_normalized = NormalizedCursorPosition { x: 0.5, y: 0.5, - crop_position: LogicalPosition::new(0.0, 0.0), - crop_size: LogicalSize::new(1.0, 1.0), + crop: CursorCropBounds { x: 0.0, y: 0.0, width: 1.0, height: 1.0 }, display, }; - let cropped_position = LogicalPosition::new(0.0, 0.0); - let cropped_size = LogicalSize::new(1.0, 1.0); - let new_normalized = original_normalized.with_crop(cropped_position, cropped_size); + let cropped = CursorCropBounds { x: 0.0, y: 0.0, width: 1.0, height: 1.0 }; + let new_normalized = original_normalized.with_crop(cropped); assert_eq!(new_normalized.x, 0.5); assert_eq!(new_normalized.y, 0.5); - assert_eq!( - new_normalized.crop_position(), - LogicalPosition::new(0.0, 0.0) - ); - assert_eq!(new_normalized.crop_size(), LogicalSize::new(1.0, 1.0)); + assert_eq!(new_normalized.crop(), cropped); } @@ fn test_with_crop_centered() { let display = mock_display(); let original_normalized = NormalizedCursorPosition { x: 0.5, y: 0.5, - crop_position: LogicalPosition::new(0.0, 0.0), - crop_size: LogicalSize::new(1.0, 1.0), + crop: CursorCropBounds { x: 0.0, y: 0.0, width: 1.0, height: 1.0 }, display, }; - let cropped_position = LogicalPosition::new(0.25, 0.25); - let cropped_size = LogicalSize::new(0.5, 0.5); - let new_normalized = original_normalized.with_crop(cropped_position, cropped_size); + let cropped = CursorCropBounds { x: 0.25, y: 0.25, width: 0.5, height: 0.5 }; + let new_normalized = original_normalized.with_crop(cropped); @@ - assert_eq!(new_normalized.crop_position(), cropped_position); - assert_eq!(new_normalized.crop_size(), cropped_size); + assert_eq!(new_normalized.crop(), cropped); } @@ fn test_with_crop_top_left_of_crop() { let display = mock_display(); - let cropped_position = LogicalPosition::new(0.25, 0.25); - let cropped_size = LogicalSize::new(0.5, 0.5); + let cropped = CursorCropBounds { x: 0.25, y: 0.25, width: 0.5, height: 0.5 }; let original_normalized_at_crop_tl = NormalizedCursorPosition { x: 0.25, y: 0.25, - crop_position: LogicalPosition::new(0.0, 0.0), - crop_size: LogicalSize::new(1.0, 1.0), + crop: CursorCropBounds { x: 0.0, y: 0.0, width: 1.0, height: 1.0 }, display, }; - let new_normalized = - original_normalized_at_crop_tl.with_crop(cropped_position, cropped_size); + let new_normalized = original_normalized_at_crop_tl.with_crop(cropped); @@ - assert_eq!(new_normalized.crop_position(), cropped_position); - assert_eq!(new_normalized.crop_size(), cropped_size); + assert_eq!(new_normalized.crop(), cropped); } @@ fn test_with_crop_bottom_right_of_crop() { let display = mock_display(); - let cropped_position = LogicalPosition::new(0.25, 0.25); - let cropped_size = LogicalSize::new(0.5, 0.5); + let cropped = CursorCropBounds { x: 0.25, y: 0.25, width: 0.5, height: 0.5 }; let original_normalized_at_crop_br = NormalizedCursorPosition { x: 0.75, y: 0.75, - crop_position: LogicalPosition::new(0.0, 0.0), - crop_size: LogicalSize::new(1.0, 1.0), + crop: CursorCropBounds { x: 0.0, y: 0.0, width: 1.0, height: 1.0 }, display, }; - let new_normalized = - original_normalized_at_crop_br.with_crop(cropped_position, cropped_size); + let new_normalized = original_normalized_at_crop_br.with_crop(cropped); @@ - assert_eq!(new_normalized.crop_position(), cropped_position); - assert_eq!(new_normalized.crop_size(), cropped_size); + assert_eq!(new_normalized.crop(), cropped); } @@ fn test_with_crop_from_existing_crop() { let display = mock_display(); let original_normalized = NormalizedCursorPosition { x: 0.5, // This 0.5 is within the first crop y: 0.5, // This 0.5 is within the first crop - crop_position: LogicalPosition::new(0.1, 0.1), - crop_size: LogicalSize::new(0.8, 0.8), + crop: CursorCropBounds { x: 0.1, y: 0.1, width: 0.8, height: 0.8 }, display, }; @@ - let second_crop_position = LogicalPosition::new(0.2, 0.2); - let second_crop_size = LogicalSize::new(0.6, 0.6); + let second_crop = CursorCropBounds { x: 0.2, y: 0.2, width: 0.6, height: 0.6 }; @@ - let new_normalized = original_normalized.with_crop(second_crop_position, second_crop_size); + let new_normalized = original_normalized.with_crop(second_crop); @@ - assert_eq!( - new_normalized.crop_position(), - LogicalPosition::new(0.2, 0.2) - ); - assert_eq!(new_normalized.crop_size(), LogicalSize::new(0.6, 0.6)); + assert_eq!(new_normalized.crop(), second_crop); }Note: These tests now treat crop coordinates in a unit-agnostic space (e.g., 0–1), which is sufficient for the math in with_crop and avoids platform-specific logical/physical differences in unit tests.
🧹 Nitpick comments (33)
crates/scap-targets/src/platform/win.rs (12)
379-393: Z-order fallback sort isn’t accurate; only checks WS_EX_TOPMOSTSorting candidates solely by
WS_EX_TOPMOSTdoesn’t produce a reliable frontmost order. Enumerations aren’t guaranteed Z-order. This can cause wrong window to be selected (“area select feels wacky”).Consider computing a Z-order index using
GW_HWNDPREVand sorting by that value:- // Sort candidates by Z-order (topmost first) - data.candidates.sort_by(|&a, &b| { - // Use GetWindowLong to check topmost status - let a_topmost = (GetWindowLongW(a, GWL_EXSTYLE) & WS_EX_TOPMOST.0 as i32) != 0; - let b_topmost = (GetWindowLongW(b, GWL_EXSTYLE) & WS_EX_TOPMOST.0 as i32) != 0; - - match (a_topmost, b_topmost) { - (true, false) => std::cmp::Ordering::Less, // a is more topmost - (false, true) => std::cmp::Ordering::Greater, // b is more topmost - _ => std::cmp::Ordering::Equal, // Same topmost level - } - }); + // Sort by true Z-order (frontmost first), with topmost as a tie-breaker + fn z_index(hwnd: HWND) -> i32 { + let mut idx = 0; + let mut h = hwnd; + loop { + let prev = GetWindow(h, GW_HWNDPREV).unwrap_or(HWND(std::ptr::null_mut())); + if prev.0 == 0 { break; } + idx += 1; + h = prev; + } + idx + } + data.candidates.sort_by(|&a, &b| { + let za = z_index(a); + let zb = z_index(b); + if za != zb { + return za.cmp(&zb); + } + // tie-breaker: topmost first + let a_topmost = (GetWindowLongW(a, GWL_EXSTYLE) & WS_EX_TOPMOST.0 as i32) != 0; + let b_topmost = (GetWindowLongW(b, GWL_EXSTYLE) & WS_EX_TOPMOST.0 as i32) != 0; + b_topmost.cmp(&a_topmost) + });This should improve consistency in window targeting and likely addresses some of the “inconsistent area select” feedback.
693-706: Icon capture tries extremely large render sizes (up to 2048+); high memory/CPU costRendering repeatedly at huge sizes risks allocating tens of MB per attempt and stalls UI. Cap to a smaller set and short-circuit once you obtain a “good enough” icon.
Apply this diff to reduce the search space and bail early when contentful result is found:
- // Always try for the highest resolution possible, starting with the largest sizes - let mut sizes = vec![2048, 1024, 512, 256, 128, 96, 64, 48, 32, 24, 16]; + // Reasonable sizes; prevent massive allocations + let mut sizes = vec![256, 128, 96, 64, 48, 32, 24, 16]; @@ - if best_result.is_none() || size > best_size { + if best_result.is_none() || size > best_size { best_result = Some((png_data, size)); best_size = size; } + // Early exit once we reach a decent size + if best_size >= 128 { + break; + }
421-427: Use GetWindowLongPtrW for consistency on 64-bitElsewhere you use
GetWindowLongPtrW; here you useGetWindowLongW. Mixing can be confusing; prefer the Ptr variant on 64-bit.Apply this diff:
- let ex_style = GetWindowLongW(self.0, GWL_EXSTYLE); + let ex_style = GetWindowLongPtrW(self.0, GWL_EXSTYLE) as i32;
913-933: Handle zero-length window titles cleanly (avoid pre-allocating vec for empty)For
len == 0, we can short-circuit toSome(String::new())without allocating a buffer.Apply this diff:
- let mut name = vec![0u16; usize::try_from(len).unwrap() + 1]; - if len >= 1 { + if len == 0 { + return Some(String::new()); + } + let mut name = vec![0u16; usize::try_from(len).unwrap() + 1]; + { let copied = unsafe { GetWindowTextW(self.0, &mut name) }; if copied == 0 { return Some(String::new()); } - } + }
206-243: Display name retrieval may return adapter string, not monitor friendly name
EnumDisplayDevicesW(device_name, 0, ...)often returns the adapter’sDeviceString. If UI expects a monitor-friendly name, consider enumerating monitors withEDD_GET_DEVICE_INTERFACE_NAMEand checkingDISPLAY_DEVICE_ACTIVE, or fallback to EDID parsing if available.I can draft a small helper that tries:
EnumDisplayDevicesW(device_name, iDevNum)loop for monitors, prefer those withDISPLAY_DEVICE_ACTIVE.- If that fails, fallback to current approach.
1015-1076: Layered/transparent-window filtering may hide legitimate windowsStrict alpha < 50 and
.TRANSPARENTchecks can exclude real app windows (e.g., custom chromeless apps, UWP). Consider relaxing thresholds or add a short whitelist for known window classes.If testers report “window select overlay isn’t showing,” capture a diagnostic dump of rejected windows at the cursor:
- if (ex_style & WS_EX_TRANSPARENT.0) != 0 || (ex_style & WS_EX_LAYERED.0) != 0 { + if (ex_style & WS_EX_TRANSPARENT.0) != 0 || (ex_style & WS_EX_LAYERED.0) != 0 { // Allow layered windows only if they have proper alpha if (ex_style & WS_EX_LAYERED.0) != 0 { let mut alpha = 0u8; let mut color_key = 0u32; let mut flags = 0u32; if GetLayeredWindowAttributes( hwnd, Some(&mut color_key as *mut u32 as *mut _), Some(&mut alpha), Some(&mut flags as *mut u32 as *mut _), ) .is_ok() { - if alpha < 50 { + if alpha == 0 { // Skip nearly transparent windows return false; } } } else { return false; // Skip fully transparent windows } }And optionally log rejections during a debug build to pinpoint problematic classes/styles.
471-515: File description extraction hardcodes US-English codepageUsing
\StringFileInfo\040904B0can fail on non-US locales. Consider querying\VarFileInfo\Translationfirst, then formatting the found language/codepage pair.I can provide a small helper to read the translation table and probe
FileDescriptionfor the first entry.
131-136: Nit: avoid recomputing scale per call
scale = dpi as f64 / 96.0could be factored into a tiny helper for reuse and tested separately. Low impact.Happy to propose a tiny
fn scale_from_dpi(dpi: u32) -> f64.
1-45: Unused imports auditLarge import surface increases compile time and risk of stale usages (e.g.,
DEVMODEW,BITMAPINFOHEADERetc. are used; but checkDEVMODEWis only used in refresh_rate). This is non-blocking.If desired, we can run
cargo +nightly udepslater to prune.
935-969: SystemApps filter may be too broadHard-excluding
C:\Windows\SystemAppshides Start, Taskbar, Settings—might be intended. If users try to capture these, selection won’t work. Consider making this behavior configurable.Add a feature flag or env var override to include system apps when debugging.
1131-1152: pid_to_exe_path: minor robustnessIf
CloseHandlefails, we drop the error. That’s fine; but log at trace to aid handle-leak investigations.No code changes required unless you want the log; keeping it quiet is also fine.
822-858: Ensure Logical Size Fallbacks to Physical Size on Non-PER_MONITOR DPI AwarenessCurrently, if
GetProcessDpiAwareness(None)returns an error or any value other thanPROCESS_PER_MONITOR_DPI_AWARE,logical_size()immediately returnsNone. That propagates into unwraps and?calls elsewhere (for example, inmain.rsand the screen‐capture modules), causing overlay blanking or capture initialization failures.Consider mapping those branches to a
physical_size()fallback so that callers always receive a valid size:unsafe { match GetProcessDpiAwareness(None) { - Err(e) => { - error!("Failed to get process DPI awareness: {e}"); - return None; - } + Err(e) => { + error!("Failed to get process DPI awareness: {e}; falling back to physical size"); + return self + .physical_size() + .map(|sz| LogicalSize::new(sz.width(), sz.height())); + } - Ok(v) => { - error!("Unsupported DPI awareness {v:?}"); - return None; - } + Ok(v) => { + error!("Unsupported DPI awareness {v:?}; falling back to physical size"); + return self + .physical_size() + .map(|sz| LogicalSize::new(sz.width(), sz.height())); + } } DwmGetWindowAttribute(…)• File:
crates/scap-targets/src/platform/win.rslines 822–858
• Physical size is already implemented viapub fn physical_size(&self) -> Option<PhysicalSize>in this file, so the fallback is availableThis optional refactor will keep your overlays and capture routines from breaking when per-monitor DPI awareness can’t be determined.
apps/desktop/src-tauri/src/fake_window.rs (3)
93-101: Avoid forcing window focus on hover; it can keep overlay “active” and annoy usersAuto-focusing on mere hover can cause z-order/focus thrash on Windows, and may contribute to overlays not closing promptly when countdown starts. Prefer focusing on intentional interaction (e.g., click) or remove focusing altogether for overlays.
Apply this diff to drop the implicit focus grab and redundant call:
- let focused = window.is_focused().unwrap_or(false); - if !ignore { - if !focused { - window.set_focus().ok(); - } - } else if focused { - window.set_ignore_cursor_events(ignore).ok(); - } + // Keep passive; don't steal focus on hover. + // If needed, focus can be triggered from the click handler where appropriate.
52-52: Minor readability: use explicit 50 ms instead of 1000/20Makes the intent obvious and avoids accidental integer math shenanigans.
- sleep(Duration::from_millis(1000 / 20)).await; + sleep(Duration::from_millis(50)).await;
46-46: Reduce redundant set_ignore_cursor_events callsYou call
set_ignore_cursor_eventsat initialization (Line 46), after each loop (Line 91), and again in the focused branch (Line 99). The last one is redundant; keep a single authoritative call each tick.- } else if focused { - window.set_ignore_cursor_events(ignore).ok(); - } + }Also applies to: 91-91, 99-99
crates/scap-direct3d/Cargo.toml (1)
7-22: Optional: gate dev-deps by platform to speed non-Windows buildsSince this crate is Windows-centric, you can optionally move
scap-targetsunder a Windows-only dev-deps table to avoid fetching/building on other platforms during tests/examples.Example:
[target.'cfg(windows)'.dev-dependencies] scap-targets = { path = "../scap-targets" }Also applies to: 31-33
crates/recording/examples/recording-cli.rs (2)
8-13: Don’t unwrap SetProcessDpiAwareness; it can fail with E_ACCESSDENIED if already setIn examples, prefer ignoring the error rather than panicking.
- unsafe { SetProcessDpiAwareness(PROCESS_PER_MONITOR_DPI_AWARE).unwrap() }; + // Ignore error if already set or restricted by context. + let _ = unsafe { SetProcessDpiAwareness(PROCESS_PER_MONITOR_DPI_AWARE) };
24-31: Optional: allow selecting a non-primary display via CLI argNot required here, but a small addition would make this example more helpful for debugging multi-monitor/DPI edge cases mentioned in the PR.
I can sketch a minimal
--displayarg if useful.apps/cli/src/record.rs (3)
32-44: Target resolution works; consider improving UX and guardrails
- Logic is fine given the clap ArgGroup. For a better CLI UX, consider making the "target" group required so clap errors early rather than at runtime with "No target specified".
- When not found, consider listing available IDs to aid users.
Example tweak (no behavior change required here):
// Add in your clap root if not already present: -#[derive(Args)] +#[derive(Args)] +#[group(required = true, multiple = false, id = "target")] struct RecordTargets { // … }And optionally enumerate available IDs in the error string when a match isn't found.
58-61: Avoid unwrap() on current_dir() to prevent panic; return a meaningful error instead
current_dir().unwrap()can panic in restricted environments. Prefer propagating an error.- let path = self - .path - .unwrap_or_else(|| current_dir().unwrap().join(format!("{id}.cap"))); + let path = match self.path { + Some(p) => p, + None => current_dir() + .map_err(|e| format!("Failed to resolve current directory: {e}"))? + .join(format!("{id}.cap")), + };
25-27:--fpsis documented but not enforced or usedYou advertise “max 60” but don’t clamp/validate or thread it through. Either enforce and pass it to the recording pipeline or remove the flag for now to avoid confusion.
Potential outline:
- Validate with
if let Some(fps) = self.fps { if fps == 0 || fps > 60 { return Err("fps must be 1..=60".into()); } }- Plumb into
RecordingBaseInputsor the actor if/when supported.crates/cursor-capture/src/main.rs (1)
5-12: Avoid potential panics and busy loop
Display::list()[1]will panic on single-display systems. Prefer safe indexing with a fallback.- The tight loop will peg a CPU core. Throttle a bit.
Add a small sleep to reduce CPU usage:
println!("{position:?}"); + std::thread::sleep(std::time::Duration::from_millis(16));And consider replacing
[1]with safe access (e.g.,.get(1).or_else(|| displays.get(0))) with appropriate ownership semantics forDisplay.crates/scap-ffmpeg/examples/cli.rs (2)
1-1: Prefer explicit import over wildcard for scap_targetsLimit what’s brought into scope for readability and to avoid future symbol collisions.
-use scap_targets::*; +use scap_targets::Display;
34-39: Avoid blocking the Tokio runtime; replace thread::sleep and block_on with awaitsThis example runs inside a Tokio runtime. Using std::thread::sleep blocks the runtime, and mixing futures::executor::block_on with Tokio is unnecessary and potentially risky. Prefer async sleeps and plain awaits.
- std::thread::sleep(Duration::from_secs(3)); + tokio::time::sleep(Duration::from_secs(3)).await; @@ - std::thread::sleep(Duration::from_secs(3)); + tokio::time::sleep(Duration::from_secs(3)).await; @@ - use futures::executor::block_on; use scap_screencapturekit::*; @@ - block_on(capturer.start()).expect("Failed to start capturing"); + capturer.start().await.expect("Failed to start capturing"); @@ - std::thread::sleep(Duration::from_secs(3)); + tokio::time::sleep(Duration::from_secs(3)).await; @@ - block_on(capturer.stop()).expect("Failed to stop capturing"); + capturer.stop().await.expect("Failed to stop capturing"); @@ - std::thread::sleep(Duration::from_secs(1)); + tokio::time::sleep(Duration::from_secs(1)).await;Also applies to: 41-81
crates/cursor-capture/src/position.rs (1)
112-120: Enable simple equality checks for crop bounds (used in tests and debugging)Deriving PartialEq/Eq on CursorCropBounds makes assertions and comparisons straightforward.
-#[derive(Clone, Copy, Debug)] +#[derive(Clone, Copy, Debug, PartialEq, Eq)] pub struct CursorCropBounds {crates/scap-targets/src/main.rs (4)
6-9: DPI awareness API: consider using the modern context-based callSetProcessDpiAwareness works but SetProcessDpiAwarenessContext(PROCESS_PER_MONITOR_DPI_AWARE_V2) is the recommended modern API and can yield better scaling behavior on recent Windows versions. Optional for this debug tool.
11-15: Don’t recompute display list; use Display::primary() instead of list().first()
- Computing “primary” from list().first() relies on enumeration order and may be wrong.
- Also avoids repeatedly calling Display::list() inside the loop.
- println!("=== Display Information ==="); - for (index, display) in scap_targets::Display::list().iter().enumerate() { + println!("=== Display Information ==="); + let displays = scap_targets::Display::list(); + let main_display_id = scap_targets::Display::primary().map(|d| d.id()); + for (index, display) in displays.iter().enumerate() { @@ - // Check if this is the main display - let main_display_id = scap_targets::Display::list().first().map(|d| d.id()); - - if let Some(main_id) = main_display_id { + // Check if this is the main display + if let Some(main_id) = main_display_id { if display.id() == main_id { println!(" Type: Primary Display"); } else { println!(" Type: Secondary Display"); } } else { println!(" Type: Unknown"); } println!(); }Also applies to: 39-49, 51-52
64-101: Surface window bounds consistently across platformsOnly macOS prints bounds. For parity and easier debugging of DPI issues, consider also printing physical bounds on Windows when available.
122-129: Public API vs. internal WindowImpl usageUsing WindowImpl here is fine within the crate, but it leaks implementation details into the example. If feasible, consider mirroring the “level” info on the public Window wrapper to keep examples on the stable API.
crates/recording/src/cursor.rs (3)
132-145: Guard against out-of-range normalized positions before recordingWhen the provided display doesn’t actually contain the cursor (or due to DPI mismatches), normalize() can yield values outside [0,1]. Clamp to keep events valid for downstream consumers.
- let position = (position != last_position).then(|| { + let position = (position != last_position).then(|| { last_position = position; let cropped_norm_pos = position .relative_to_display(display)? .normalize()? .with_crop(crop_bounds); - Some((cropped_norm_pos.x(), cropped_norm_pos.y())) + let x = cropped_norm_pos.x().clamp(0.0, 1.0); + let y = cropped_norm_pos.y().clamp(0.0, 1.0); + Some((x, y)) });
507-514: Clamp hotspot after trimming to stay within [0,1]Trimming can push computed hotspots slightly out of bounds due to rounding. Clamp before emitting.
// Adjust hotspot coordinates for the trimmed image hotspot_x = (hotspot_x * width as f64 - trim_min_x as f64) / trim_width as f64; hotspot_y = (hotspot_y * height as f64 - trim_min_y as f64) / trim_height as f64; + hotspot_x = hotspot_x.clamp(0.0, 1.0); + hotspot_y = hotspot_y.clamp(0.0, 1.0); @@ Some(CursorData { image: png_data, - hotspot: XY::new(hotspot_x, hotspot_y), + hotspot: XY::new(hotspot_x.clamp(0.0, 1.0), hotspot_y.clamp(0.0, 1.0)), shape: CursorShape::try_from(&cursor_info.hCursor).ok(), })Also applies to: 525-529
76-83: Consider reducing cursor shape extraction frequencyget_cursor_data() and image hashing every 10ms can be heavy (GDI calls, PNG encode). Consider:
- Sampling at a lower rate (e.g., 30–60ms) for shape changes.
- Debouncing cursor image saves (only when hash changes and after a small dwell).
Also applies to: 90-131
apps/desktop/src-tauri/src/windows.rs (1)
284-312: Document and Harden Windows DPI Scaling WorkaroundThis block in apps/desktop/src-tauri/src/windows.rs (lines 284–312) relies on a three-step resize (initial 100×100, logical resize + position, 5 ms sleep, optional second resize) to work around non-default DPI scaling on Windows. While it fixes the immediate issue, it’s brittle and opaque:
• Add a comment above this
#[cfg(windows)]block explaining:
– Why an initial tiny window (100×100) is required.
– That the subsequentset_size→set_position→sleep sequence forces Windows to apply DPI scaling before measuring the “real” inner size.
– Why the final conditional resize is needed when the first attempt still yields the wrong size.• Consider moving DPI awareness to the process level instead of per-window hacks:
– CallSetProcessDpiAwareness(PROCESS_PER_MONITOR_DPI_AWARE)at startup (e.g., inmain.rsor via app manifest) to ensure Windows maps logical sizes correctly.
– Alternatively, declare<dpiAware>true</dpiAware>or<dpiAwareness>PerMonitorV2</dpiAwareness>in the application’s manifest.• Explore a more robust approach via the Windows API:
– Handle theWM_DPICHANGEDmessage in the event loop to resize/reposition when DPI changes occur, eliminating arbitrary sleep delays.
– UseSetThreadDpiAwarenessContext(DPI_AWARENESS_CONTEXT_PER_MONITOR_AWARE_V2)for thread-level DPI context if needed.• Refactor this sequence into a dedicated helper (e.g.,
apply_per_monitor_dpi_correction(window, display)) to encapsulate and unit-test the workaround, and to make it obvious that it’s a Windows-only hack.
📜 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 ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (27)
apps/cli/Cargo.toml(1 hunks)apps/cli/src/record.rs(1 hunks)apps/desktop/src-tauri/Cargo.toml(1 hunks)apps/desktop/src-tauri/src/fake_window.rs(1 hunks)apps/desktop/src-tauri/src/lib.rs(3 hunks)apps/desktop/src-tauri/src/target_select_overlay.rs(6 hunks)apps/desktop/src-tauri/src/windows.rs(3 hunks)crates/cursor-capture/Cargo.toml(1 hunks)crates/cursor-capture/src/main.rs(1 hunks)crates/cursor-capture/src/position.rs(2 hunks)crates/media/Cargo.toml(1 hunks)crates/recording/Cargo.toml(1 hunks)crates/recording/examples/recording-cli.rs(1 hunks)crates/recording/examples/screen_capture.rs(1 hunks)crates/recording/src/cursor.rs(1 hunks)crates/recording/src/lib.rs(1 hunks)crates/recording/src/sources/screen_capture/macos.rs(0 hunks)crates/recording/src/sources/screen_capture/mod.rs(3 hunks)crates/scap-direct3d/Cargo.toml(1 hunks)crates/scap-direct3d/examples/cli.rs(1 hunks)crates/scap-ffmpeg/Cargo.toml(1 hunks)crates/scap-ffmpeg/examples/cli.rs(1 hunks)crates/scap-screencapturekit/Cargo.toml(1 hunks)crates/scap-screencapturekit/examples/cli.rs(1 hunks)crates/scap-targets/Cargo.toml(2 hunks)crates/scap-targets/src/main.rs(6 hunks)crates/scap-targets/src/platform/win.rs(1 hunks)
💤 Files with no reviewable changes (1)
- crates/recording/src/sources/screen_capture/macos.rs
✅ Files skipped from review due to trivial changes (1)
- crates/recording/examples/screen_capture.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src-tauri/src/lib.rs
🧰 Additional context used
🧬 Code graph analysis (13)
crates/recording/src/cursor.rs (6)
crates/cursor-capture/src/position.rs (2)
display(60-62)display(176-178)crates/recording/src/sources/screen_capture/mod.rs (1)
display(68-74)crates/scap-targets/src/platform/win.rs (1)
display(904-911)crates/scap-targets/src/platform/macos.rs (1)
display(461-481)crates/scap-targets/src/lib.rs (1)
display(146-148)apps/desktop/src-tauri/src/target_select_overlay.rs (1)
scap_targets(50-53)
crates/scap-direct3d/examples/cli.rs (2)
apps/desktop/src-tauri/src/target_select_overlay.rs (1)
scap_targets(50-53)crates/scap-targets/src/main.rs (1)
scap_targets(123-129)
crates/scap-ffmpeg/examples/cli.rs (2)
apps/desktop/src-tauri/src/target_select_overlay.rs (1)
scap_targets(50-53)crates/scap-targets/src/main.rs (1)
scap_targets(123-129)
crates/cursor-capture/src/main.rs (1)
apps/desktop/src-tauri/src/target_select_overlay.rs (1)
scap_targets(50-53)
crates/cursor-capture/src/position.rs (2)
apps/desktop/src-tauri/src/target_select_overlay.rs (1)
scap_targets(50-53)crates/scap-targets/src/main.rs (1)
scap_targets(123-129)
crates/recording/examples/recording-cli.rs (1)
apps/desktop/src-tauri/src/target_select_overlay.rs (1)
scap_targets(50-53)
crates/scap-screencapturekit/examples/cli.rs (2)
apps/desktop/src-tauri/src/target_select_overlay.rs (1)
scap_targets(50-53)crates/scap-targets/src/main.rs (1)
scap_targets(123-129)
apps/desktop/src-tauri/src/fake_window.rs (3)
apps/desktop/src-tauri/src/target_select_overlay.rs (1)
scap_targets(50-53)crates/scap-targets/src/main.rs (1)
scap_targets(123-129)apps/desktop/src/utils/tauri.ts (1)
LogicalBounds(601-601)
crates/scap-targets/src/main.rs (2)
crates/scap-targets/src/platform/win.rs (6)
windows(246-246)list(83-106)list(269-299)get_containing_cursor(138-151)list_containing_cursor(396-408)get_topmost_at_cursor(305-348)crates/scap-targets/src/lib.rs (5)
list(16-18)list(103-105)get_containing_cursor(36-38)list_containing_cursor(107-112)get_topmost_at_cursor(114-116)
crates/recording/src/lib.rs (3)
apps/desktop/src-tauri/src/target_select_overlay.rs (1)
scap_targets(50-53)crates/scap-targets/src/main.rs (1)
scap_targets(123-129)apps/desktop/src/utils/tauri.ts (1)
LogicalBounds(601-601)
apps/desktop/src-tauri/src/windows.rs (4)
apps/desktop/src-tauri/src/target_select_overlay.rs (1)
scap_targets(50-53)crates/scap-targets/src/platform/win.rs (5)
from_id(116-119)logical_size(121-136)logical_size(822-858)physical_size(175-177)physical_size(896-898)crates/scap-targets/src/platform/macos.rs (5)
from_id(43-46)logical_size(48-55)logical_size(358-360)physical_size(89-104)physical_size(362-372)crates/scap-targets/src/lib.rs (6)
from_id(32-34)from_id(122-124)logical_size(48-50)logical_size(130-132)physical_size(44-46)physical_size(126-128)
apps/desktop/src-tauri/src/target_select_overlay.rs (3)
apps/desktop/src/utils/tauri.ts (4)
DisplayId(520-520)WindowId(789-789)DisplayInformation(521-525)PhysicalSize(642-642)crates/scap-targets/src/platform/win.rs (5)
list(83-106)list(269-299)get_containing_cursor(138-151)get_topmost_at_cursor(305-348)from_id(116-119)crates/scap-targets/src/lib.rs (6)
list(16-18)list(103-105)get_containing_cursor(36-38)get_topmost_at_cursor(114-116)from_id(32-34)from_id(122-124)
crates/recording/src/sources/screen_capture/mod.rs (4)
apps/desktop/src-tauri/src/target_select_overlay.rs (1)
scap_targets(50-53)crates/scap-targets/src/platform/win.rs (2)
list(83-106)list(269-299)crates/scap-targets/src/platform/macos.rs (2)
list(27-33)list(223-255)crates/scap-targets/src/lib.rs (2)
list(16-18)list(103-105)
⏰ 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 (35)
crates/scap-ffmpeg/Cargo.toml (1)
24-24: Dependency rename looks correctDev-dep switch to
scap-targetsaligns with the crate migration. No issues spotted.crates/scap-direct3d/examples/cli.rs (1)
10-10: Import change to scap_targets is consistent with workspace migrationThe example now uses
Display::primary().raw_handle()withtry_as_capture_item(). Assuming the existingDisplaywrapper returnsDisplayImpl, this remains valid.crates/scap-screencapturekit/examples/cli.rs (1)
1-1: Import path update LGTM
use scap_targets::Display;matches the new crate. The rest of the example remains unchanged and coherent.crates/recording/Cargo.toml (1)
16-16: Recording crate now depends on scap-targetsMigration looks consistent with the rest of the workspace. No action needed.
apps/desktop/src-tauri/src/fake_window.rs (2)
8-22: Thread-safe state handling looks goodUse of
tokio::RwLockwith per-window maps is clean and avoids unnecessary contention. Early return on missing labels is also a nice touch.Also applies to: 24-43
1-1: Import Verified: Specta TS Type for LogicalBounds MatchesThe Specta-generated TypeScript definition confirms that
LogicalBoundsincludes bothpositionandsize, matching the Rust type and ensuring Specta exports remain compatible:
- apps/desktop/src/utils/tauri.ts line 601:
export type LogicalBounds = { position: LogicalPosition; size: LogicalSize };No further action required.
apps/cli/Cargo.toml (2)
17-17: LGTM: migrate CLI to scap-targetsDependency swap is straightforward and aligns with workspace changes.
6-29: No lingeringcap-displaysreferences detectedRan the suggested grep command against
apps/cli(excludingtarget/), and it returned zero matches—confirming that allcap-displaysreferences have been removed.crates/scap-direct3d/Cargo.toml (1)
24-26: Dev-dep migration looks correctSwapping to
scap-targetsis consistent with the project’s new display/window APIs.apps/desktop/src-tauri/Cargo.toml (2)
91-92: LGTM: desktop crate now depends on scap-targetsMatches the import changes in tauri sources (lib.rs, windows.rs, target_select_overlay.rs).
21-48: No lingeringcap-displaysreferences foundA repository-wide regex search returned zero matches for any
cap-displayorcap_displaysidentifiers. The migration appears complete.crates/recording/examples/recording-cli.rs (1)
3-3: LGTM: import path migrated to scap_targets::DisplayNo behavior change; consistent with the rest of the PR.
apps/cli/src/record.rs (1)
2-2: Import migration to scap_targets looks correctThe type switch to
scap_targets::{DisplayId, WindowId}aligns with the workspace-wide migration and preserves local semantics.crates/media/Cargo.toml (2)
20-20: Dependency migration to scap-targets is consistent with the PR directionSwitching from cap-displays to scap-targets at the media layer looks correct.
14-23: No stalecap-displaysreferences foundI ran a full search for both
cap-displaysandcap_displayswithin thecrates/mediadirectory and confirmed there are no lingering references. The migration appears clean—no further changes are needed here.crates/cursor-capture/src/main.rs (1)
2-2: Import path update LGTMUsing
scap_targets::Displayhere matches the workspace changes.crates/cursor-capture/Cargo.toml (1)
10-10: Manifest migration to scap-targets looks goodThe crate imports
Displayfromscap_targetsin main.rs, so this dependency aligns.crates/scap-screencapturekit/Cargo.toml (1)
23-23: Dev-dependency swap to scap-targets is appropriateThis keeps test/dev tooling consistent with the new targets crate.
crates/cursor-capture/src/position.rs (2)
2-2: LGTM on the import migrationImport path switched cleanly from cap_displays to scap_targets without behavior change.
36-58: Non-Windows/macOS build path returns nothing; confirm crate targetsBoth RelativeCursorPosition::from_raw and normalize are entirely behind cfg(windows)/cfg(macos). If this crate is built on Linux or other targets (e.g., CI), these functions will compile to empty bodies and fail to return.
If Linux builds are required, add a fallback returning None:
impl RelativeCursorPosition { pub fn from_raw(raw: RawCursorPosition, display: Display) -> Option<Self> { #[cfg(windows)] { ... } #[cfg(target_os = "macos")] { ... } + + #[cfg(not(any(windows, target_os = "macos")))] + { + None + } } }Also applies to: 64-101
crates/recording/src/cursor.rs (1)
40-40: LGTM on the migration to scap_targets::DisplaySignature aligns with the updated cursor-capture API; call sites should remain straightforward.
crates/recording/src/lib.rs (2)
11-12: Re-export reorder is fineNo functional impact; aligns with typical “spawn_*, types” ordering.
18-19: LogicalBounds serde shape matches frontend
- In
crates/scap-targets/src/bounds.rs,LogicalBoundsis annotated with#[derive(Serialize, Deserialize)]and exposes two fields—positionandsize—exactly as the struct definition:#[derive(Clone, Copy, Debug, Type, Serialize, Deserialize)] pub struct LogicalBounds { pub(crate) position: LogicalPosition, pub(crate) size: LogicalSize, }- In
apps/desktop/src/utils/tauri.ts, the TypeScript type mirrors this JSON shape precisely:export type LogicalBounds = { position: LogicalPosition; size: LogicalSize }; export type LogicalPosition = { x: number; y: number }; export type LogicalSize = { width: number; height: number };No mismatches were found—no renames or skips—and nested types (
LogicalPosition/LogicalSize) align with their Rust counterparts. You can safely proceed without changes.crates/scap-targets/Cargo.toml (1)
2-2: Package rename looks goodThe package rename from
cap-displaystoscap-targetsis properly reflected here.crates/recording/src/sources/screen_capture/mod.rs (2)
2-2: Import migration looks correctThe migration from
cap_displaystoscap_targetsis properly implemented with all necessary types imported.
405-405: Consistent usage of scap_targets APIThe updates to use
scap_targets::Display::list()andscap_targets::Window::list()are consistent with the migration pattern across the codebase.Also applies to: 421-421
apps/desktop/src-tauri/src/windows.rs (4)
4-4: Import path updated correctlyThe import change from
cap_displaystoscap_targetsis consistent with the broader migration.
255-255: Consistent Display API usageThe use of
scap_targets::Display::from_idfollows the new API correctly.
268-269: Content protection enhancementGood improvement adding
content_protected(true)to prevent screen recording of the overlay itself, enhancing security.
274-282: Platform-specific sizing properly handledThe macOS-specific positioning and sizing using logical coordinates is correctly implemented.
apps/desktop/src-tauri/src/target_select_overlay.rs (5)
11-14: Import updates are correctThe migration to
scap_targetstypes is properly implemented.
38-42: Good architectural improvement with DisplayInformationThe new
DisplayInformationstruct provides a clean API for fetching display metadata separately, improving separation of concerns.
50-50: API usage is consistentThe updated calls to
scap_targets::Display::list()and cursor-related methods follow the new API correctly.Also applies to: 64-65
107-107: Simplified function signatureGood simplification removing the unused
WindowFocusManagerparameter fromclose_target_select_overlays.
119-130: Well-designed display information commandThe new
display_informationcommand provides a clean interface for fetching display metadata with proper error handling.
| [target.'cfg(target_os = "windows")'.dependencies] | ||
| windows = { workspace = true, features = [ | ||
| "Win32_Foundation", | ||
| "Win32_System", | ||
| "Win32_System_Threading", | ||
| "Win32_System_Registry", | ||
| "Win32_System_Com", | ||
| "Win32_System_Wmi", | ||
| "Win32_UI_WindowsAndMessaging", | ||
| "Win32_UI_Shell", | ||
| "Win32_UI_HiDpi", | ||
| "Win32_Graphics_Dwm", | ||
| "Win32_Graphics_Gdi", | ||
| "Win32_Storage_FileSystem", | ||
| "Win32_Devices_Display", | ||
| "Graphics_Capture", | ||
| "Win32_System_WinRT_Graphics_Capture", | ||
| ] } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify Windows dependency removal impact
The removal of several Windows dependencies (Win32_System_Com, Win32_System_Wmi, Win32_Graphics_Dwm, etc.) may affect functionality. Please ensure that all Windows-specific features previously dependent on these crates are either no longer needed or have been reimplemented using the remaining dependencies.
🏁 Script executed:
#!/bin/bash
# Check if there are any lingering references to the removed Windows dependencies
rg -n "Win32_System_Com|Win32_System_Registry|Win32_System_Wmi|Win32_Graphics_Dwm|Win32_Devices_Display|Graphics_Capture|Win32_System_WinRT_Graphics_Capture" --type rustLength of output: 171
🏁 Script executed:
#!/bin/bash
# Search Rust source for direct module references to removed Windows components
rg -n 'Win32::System::Com|Win32::System::Registry|Win32::System::Wmi|Win32::Graphics::Dwm|Win32::Devices::Display|Graphics::Capture|Win32::System::WinRT::Graphics::Capture' --type rust
# Search manifests and documentation for any lingering feature flags or mentions
rg -n 'Win32_System_Com|Win32_System_Registry|Win32_System_Wmi|Win32_Graphics_Dwm|Win32_Devices_Display|Graphics_Capture|Win32_System_WinRT_Graphics_Capture' -g '*.toml' -g '*.md'Length of output: 1158
Re-add missing Windows “Capture” feature flags
The Windows-targeted scap-targets crate is still importing Windows Capture types but the corresponding feature flags were removed from its Cargo.toml, causing compilation errors:
• In crates/scap-targets/src/platform/win.rs you call
• Graphics::Capture::GraphicsCaptureItem
• WinRT::Graphics::Capture::IGraphicsCaptureItemInterop
• In crates/recording/src/sources/screen_capture/windows.rs you use
• windows::Graphics::Capture::GraphicsCaptureItem
These both require the Graphics_Capture and Win32_System_WinRT_Graphics_Capture features on the windows crate. Please restore them:
[target.'cfg(target_os = "windows")'.dependencies]
windows = { workspace = true, features = [
"Win32_Foundation",
"Win32_System_Threading",
"Win32_UI_WindowsAndMessaging",
"Win32_UI_Shell",
"Win32_UI_HiDpi",
"Win32_Graphics_Gdi",
"Win32_Storage_FileSystem",
+ "Graphics_Capture",
+ "Win32_System_WinRT_Graphics_Capture",
] }With these re-added, the references in your Windows-specific modules will resolve correctly.
📝 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.
| [target.'cfg(target_os = "windows")'.dependencies] | |
| windows = { workspace = true, features = [ | |
| "Win32_Foundation", | |
| "Win32_System", | |
| "Win32_System_Threading", | |
| "Win32_System_Registry", | |
| "Win32_System_Com", | |
| "Win32_System_Wmi", | |
| "Win32_UI_WindowsAndMessaging", | |
| "Win32_UI_Shell", | |
| "Win32_UI_HiDpi", | |
| "Win32_Graphics_Dwm", | |
| "Win32_Graphics_Gdi", | |
| "Win32_Storage_FileSystem", | |
| "Win32_Devices_Display", | |
| "Graphics_Capture", | |
| "Win32_System_WinRT_Graphics_Capture", | |
| ] } | |
| [target.'cfg(target_os = "windows")'.dependencies] | |
| windows = { workspace = true, features = [ | |
| "Win32_Foundation", | |
| "Win32_System_Threading", | |
| "Win32_UI_WindowsAndMessaging", | |
| "Win32_UI_Shell", | |
| "Win32_UI_HiDpi", | |
| "Win32_Graphics_Gdi", | |
| "Win32_Storage_FileSystem", | |
| "Graphics_Capture", | |
| "Win32_System_WinRT_Graphics_Capture", | |
| ] } |
🤖 Prompt for AI Agents
In crates/scap-targets/Cargo.toml around lines 22 to 31, the windows features
for Capture were removed causing unresolved types; restore the missing feature
flags by adding "Graphics_Capture" and "Win32_System_WinRT_Graphics_Capture" (as
entries in the windows = { workspace = true, features = [...] } list) so the
platform modules can resolve Graphics::Capture::GraphicsCaptureItem and related
WinRT capture interop types.
Replaces #874
Blocking:
Summary by CodeRabbit
New Features
Improvements