Fix new recording flow list view #1235
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds an optional focused target parameter to the backend overlay command and forwards it from the frontend; frontend now opens/closes overlays explicitly at mode/selection points and resolves window icons directly from cursor state; Windows restore logic now preserves WINDOWPLACEMENT when restoring minimized windows. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend
participant Tauri
participant OS
User->>Frontend: click mode or select target
Frontend->>Frontend: derive focusedTarget (ScreenCaptureTarget or null)
Frontend->>Tauri: open_target_select_overlays(focusedTarget)
alt focusedTarget provided
Tauri->>Tauri: focusedTarget.display()/window()
else
Tauri->>Tauri: determine display/window from cursor/context
end
alt Windows && window is minimized
Tauri->>OS: IsIconic(hwnd)? / GetWindowPlacement
alt placement ok
Tauri->>OS: SetWindowPlacement(preserve pos/size)
else
Tauri->>OS: ShowWindow(SW_RESTORE)
end
end
Tauri->>OS: SetForegroundWindow(hwnd)
Tauri->>Frontend: overlays shown (focused context)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
apps/desktop/src/routes/(window-chrome)/new-main/index.tsx (4)
381-389: Avoid stale state: pass the newly selected display target to overlays.Reading rawOptions.captureTarget immediately after setOptions can be stale. Construct once and reuse for setOptions and the TAURI call.
Apply:
- setOptions( - "captureTarget", - reconcile({ variant: "display", id: target.id }), - ); - setOptions("targetMode", "display"); - commands.openTargetSelectOverlays(rawOptions.captureTarget); + const newTarget = { variant: "display", id: target.id } as const; + setOptions("captureTarget", reconcile(newTarget)); + setOptions("targetMode", "display"); + commands.openTargetSelectOverlays(newTarget);
392-400: Same issue for window selection; pass the just-selected target.Use the freshly built value to avoid race with reactive updates.
- setOptions( - "captureTarget", - reconcile({ variant: "window", id: target.id }), - ); - setOptions("targetMode", "window"); - commands.openTargetSelectOverlays(rawOptions.captureTarget); + const newTarget = { variant: "window", id: target.id } as const; + setOptions("captureTarget", reconcile(newTarget)); + setOptions("targetMode", "window"); + commands.openTargetSelectOverlays(newTarget);
648-655: Toggle overlay based on the next state, not the previous one.rawOptions.targetMode is read before setOptions applies; compute next and branch on it.
- setOptions("targetMode", (v) => - v === "display" ? null : "display", - ); - if (rawOptions.targetMode) - commands.openTargetSelectOverlays(null); - else commands.closeTargetSelectOverlays(); + const next = rawOptions.targetMode === "display" ? null : "display"; + setOptions("targetMode", next); + if (next) commands.openTargetSelectOverlays(null); + else commands.closeTargetSelectOverlays();
693-700: Same toggle correctness for Window.Branch on the computed next state.
- setOptions("targetMode", (v) => - v === "window" ? null : "window", - ); - if (rawOptions.targetMode) - commands.openTargetSelectOverlays(null); - else commands.closeTargetSelectOverlays(); + const next = rawOptions.targetMode === "window" ? null : "window"; + setOptions("targetMode", next); + if (next) commands.openTargetSelectOverlays(null); + else commands.closeTargetSelectOverlays();
🧹 Nitpick comments (2)
apps/desktop/src/routes/target-select-overlay.tsx (1)
69-78: Good: stable query key + enable flags.Keying by window id and guarding query solves unnecessary fetches. Minor nit: id is already a string; toString() is redundant.
- return await commands.getWindowIcon( - targetUnderCursor.window.id.toString(), - ); + return await commands.getWindowIcon(targetUnderCursor.window.id);apps/desktop/src-tauri/src/target_select_overlay.rs (1)
50-51: Focused-target support is wired correctly.Preferring focused_target for display/window and falling back to cursor-based detection is sound. Emitting every 50ms is okay; consider throttling to changes later if needed.
You can slightly simplify Option<Option> flow:
let display = focused_target .as_ref() .and_then(|v| v.display()) .or_else(|| scap_targets::Display::get_containing_cursor()); let window = focused_target .as_ref() .and_then(|v| v.window().and_then(|id| scap_targets::Window::from_id(&id))) .or_else(|| scap_targets::Window::get_topmost_at_cursor());Also applies to: 68-76
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/desktop/src-tauri/src/target_select_overlay.rs(4 hunks)apps/desktop/src/routes/(window-chrome)/new-main/index.tsx(7 hunks)apps/desktop/src/routes/target-select-overlay.tsx(4 hunks)apps/desktop/src/utils/tauri.ts(1 hunks)crates/recording/src/sources/screen_capture/mod.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Format Rust code usingrustfmtand ensure all Rust code passes workspace-level clippy lints.
Rust modules should be named with snake_case, and crate directories should be in kebab-case.
Files:
crates/recording/src/sources/screen_capture/mod.rsapps/desktop/src-tauri/src/target_select_overlay.rs
crates/*/src/**/*
📄 CodeRabbit inference engine (AGENTS.md)
Rust crates should place tests within the
src/and/or a siblingtests/directory for each crate insidecrates/*.
Files:
crates/recording/src/sources/screen_capture/mod.rs
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use a 2-space indent for TypeScript code.
Use Biome for formatting and linting TypeScript/JavaScript files by runningpnpm format.Use strict TypeScript and avoid any; leverage shared types
Files:
apps/desktop/src/utils/tauri.tsapps/desktop/src/routes/target-select-overlay.tsxapps/desktop/src/routes/(window-chrome)/new-main/index.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Use kebab-case for filenames for TypeScript/JavaScript modules (e.g.,user-menu.tsx).
Use PascalCase for React/Solid components.
Files:
apps/desktop/src/utils/tauri.tsapps/desktop/src/routes/target-select-overlay.tsxapps/desktop/src/routes/(window-chrome)/new-main/index.tsx
**/tauri.ts
📄 CodeRabbit inference engine (AGENTS.md)
Do not edit auto-generated files named
tauri.ts.NEVER EDIT auto-generated IPC bindings file: tauri.ts
Files:
apps/desktop/src/utils/tauri.ts
apps/desktop/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/desktop/src/**/*.{ts,tsx}: Desktop icons are auto-imported (unplugin-icons); do not import icons manually
Desktop IPC: Call generated tauri_specta commands/events; listen to generated events and use typed interfaces
Use @tanstack/solid-query for server state in the desktop app
Files:
apps/desktop/src/utils/tauri.tsapps/desktop/src/routes/target-select-overlay.tsxapps/desktop/src/routes/(window-chrome)/new-main/index.tsx
🧬 Code graph analysis (3)
crates/recording/src/sources/screen_capture/mod.rs (4)
apps/desktop/src/utils/tauri.ts (1)
WindowId(487-487)crates/scap-targets/src/platform/macos.rs (2)
id(49-49)id(302-304)crates/scap-targets/src/platform/win.rs (2)
id(131-131)id(424-426)crates/scap-targets/src/lib.rs (2)
id(28-30)id(118-120)
apps/desktop/src-tauri/src/target_select_overlay.rs (3)
apps/desktop/src/utils/tauri.ts (1)
ScreenCaptureTarget(459-459)crates/recording/src/sources/screen_capture/mod.rs (2)
display(67-73)window(75-80)crates/scap-targets/src/lib.rs (5)
display(146-148)id(28-30)id(118-120)from_id(32-34)from_id(122-124)
apps/desktop/src/routes/target-select-overlay.tsx (1)
apps/desktop/src/utils/tauri.ts (2)
commands(7-284)events(289-333)
⏰ 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: Typecheck
- 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 (8)
crates/recording/src/sources/screen_capture/mod.rs (1)
75-80: Accessor looks good; minimal and clear.Returning Option for only the Window variant is correct and aligns with downstream usage.
apps/desktop/src/routes/(window-chrome)/new-main/index.tsx (2)
198-200: UI class tweak is fine.No behavioral concerns here.
266-266: All callsites correctly pass ScreenCaptureTarget | null after signature change.Verification found 7 total invocations of
openTargetSelectOverlays:
- Lines 386, 397: Pass
rawOptions.captureTargetimmediately after user selection (display/window)- Lines 244, 419, 653, 698, 734: Pass
nullin non-selection contextsAll invocations are type-safe and semantically correct. The pattern matches the requirement: newly selected targets are passed post-selection, and
nullis used appropriately elsewhere.apps/desktop/src/routes/target-select-overlay.tsx (3)
128-133: Escape handling now closes overlays too.This prevents orphaned overlays. Looks good.
244-245: Opening overlays for area adjustment is consistent with the new flow.Works with explicit command-driven lifecycle.
Please confirm this path is the only place area-mode is entered from Window overlays, or update any other path to call openTargetSelectOverlays(null) as well.
781-784: Close button also closes overlays.Nice UX fix; avoids overlays lingering when returning.
apps/desktop/src-tauri/src/target_select_overlay.rs (1)
187-209: Windows focus restore logic avoids unwanted resize.Checking IsIconic and restoring via WINDOWPLACEMENT before SetForegroundWindow is the right approach for multi‑monitor/restore quirks.
Please test on:
- Minimized window restore (expect size/position unchanged).
- Maximized window (ensure it remains maximized after focus).
- Normal window on secondary monitor (ensure no jump/resize).
apps/desktop/src/utils/tauri.ts (1)
266-268: No issues found; file is properly auto-generated and all callsites correctly updated.Verification confirms:
apps/desktop/src/utils/tauri.tsis auto-generated (header verified)- Function signature
focusedTarget: ScreenCaptureTarget | nullcorrectly mirrors backendOption<ScreenCaptureTarget>- All 7 callsites pass valid arguments: either
rawOptions.captureTarget(typed asScreenCaptureTarget) or explicitnull- Types align correctly across imports and store definitions
- No manual edits detected; file integrity maintained
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 (1)
apps/desktop/src/routes/(window-chrome)/new-main/index.tsx (1)
647-655: Race condition: logic inverted due to stale state read.The code toggles
targetModeusing a callback, then immediately checksrawOptions.targetModeto decide overlay visibility. This reads the old value before the update propagates, inverting the intended logic:
- If
targetModewas"display", the callback changes it tonull- But the check sees
"display"(old value) and callsopenTargetSelectOverlays- The overlay should actually be closed when
targetModebecomesnullThe same issue exists in the Window button (lines 692-699) and Area button (lines 730-736).
Apply this diff to fix the logic:
onClick={() => { if (isRecording()) return; + const newMode = rawOptions.targetMode === "display" ? null : "display"; - setOptions("targetMode", (v) => - v === "display" ? null : "display", - ); + setOptions("targetMode", newMode); - if (rawOptions.targetMode) + if (newMode) commands.openTargetSelectOverlays(null); else commands.closeTargetSelectOverlays(); }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/desktop/src-tauri/src/target_select_overlay.rs(4 hunks)apps/desktop/src/routes/(window-chrome)/new-main/index.tsx(7 hunks)apps/desktop/src/routes/target-select-overlay.tsx(4 hunks)apps/desktop/src/utils/tauri.ts(1 hunks)crates/recording/src/sources/screen_capture/mod.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Format Rust code usingrustfmtand ensure all Rust code passes workspace-level clippy lints.
Rust modules should be named with snake_case, and crate directories should be in kebab-case.
Files:
crates/recording/src/sources/screen_capture/mod.rsapps/desktop/src-tauri/src/target_select_overlay.rs
crates/*/src/**/*
📄 CodeRabbit inference engine (AGENTS.md)
Rust crates should place tests within the
src/and/or a siblingtests/directory for each crate insidecrates/*.
Files:
crates/recording/src/sources/screen_capture/mod.rs
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use a 2-space indent for TypeScript code.
Use Biome for formatting and linting TypeScript/JavaScript files by runningpnpm format.Use strict TypeScript and avoid any; leverage shared types
Files:
apps/desktop/src/routes/target-select-overlay.tsxapps/desktop/src/utils/tauri.tsapps/desktop/src/routes/(window-chrome)/new-main/index.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Use kebab-case for filenames for TypeScript/JavaScript modules (e.g.,user-menu.tsx).
Use PascalCase for React/Solid components.
Files:
apps/desktop/src/routes/target-select-overlay.tsxapps/desktop/src/utils/tauri.tsapps/desktop/src/routes/(window-chrome)/new-main/index.tsx
apps/desktop/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/desktop/src/**/*.{ts,tsx}: Desktop icons are auto-imported (unplugin-icons); do not import icons manually
Desktop IPC: Call generated tauri_specta commands/events; listen to generated events and use typed interfaces
Use @tanstack/solid-query for server state in the desktop app
Files:
apps/desktop/src/routes/target-select-overlay.tsxapps/desktop/src/utils/tauri.tsapps/desktop/src/routes/(window-chrome)/new-main/index.tsx
**/tauri.ts
📄 CodeRabbit inference engine (AGENTS.md)
Do not edit auto-generated files named
tauri.ts.NEVER EDIT auto-generated IPC bindings file: tauri.ts
Files:
apps/desktop/src/utils/tauri.ts
🧬 Code graph analysis (3)
crates/recording/src/sources/screen_capture/mod.rs (3)
apps/desktop/src/utils/tauri.ts (1)
WindowId(487-487)crates/scap-targets/src/platform/macos.rs (2)
id(49-49)id(302-304)crates/scap-targets/src/lib.rs (2)
id(28-30)id(118-120)
apps/desktop/src/routes/target-select-overlay.tsx (1)
apps/desktop/src/utils/tauri.ts (2)
commands(7-284)events(289-333)
apps/desktop/src-tauri/src/target_select_overlay.rs (3)
apps/desktop/src/utils/tauri.ts (1)
ScreenCaptureTarget(459-459)crates/recording/src/sources/screen_capture/mod.rs (2)
display(67-73)window(75-80)crates/scap-targets/src/lib.rs (7)
display(146-148)get_containing_cursor(36-38)id(28-30)id(118-120)from_id(32-34)from_id(122-124)get_topmost_at_cursor(114-116)
⏰ 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 (10)
crates/recording/src/sources/screen_capture/mod.rs (1)
75-80: LGTM! Clean accessor following established patterns.The new
window()method mirrors thedisplay()pattern and provides a convenient way to extract the underlyingWindowIdfor Window targets without external pattern matching.apps/desktop/src/utils/tauri.ts (1)
266-267: Auto-generated file – no review needed.This file is generated by tauri-specta and should not be manually edited.
apps/desktop/src/routes/(window-chrome)/new-main/index.tsx (2)
198-200: Minor formatting adjustment – no functional impact.
380-389: Verify that state update completes before reading.The code calls
setOptionsto updatecaptureTargetand then immediately readsrawOptions.captureTargetto pass toopenTargetSelectOverlays. While SolidJS store updates are typically synchronous, the use ofreconcile()might introduce timing subtleties.apps/desktop/src/routes/target-select-overlay.tsx (4)
68-78: Good simplification – window icon now derived directly from cursor state.The query now uses
targetUnderCursor.window?.iddirectly, eliminating the previous multi-step window selection cache path. The 5-minute staleTime is reasonable for icon caching.
129-132: Good addition – escape now consistently closes overlays.Adding
closeTargetSelectOverlays()ensures overlay visibility stays synchronized with the target mode state.
236-248: Overlay open on area adjustment improves UX.Opening overlays when switching to area mode ensures the selection UI is visible.
781-784: Close button now properly closes overlays.Consistent with the escape handler, the close button now also closes overlays to maintain UI synchronization.
apps/desktop/src-tauri/src/target_select_overlay.rs (2)
47-75: Well-designed focused target integration.The addition of the
focused_targetparameter cleanly supports explicit targeting while maintaining backward compatibility through cursor-based fallback. The logic correctly usesScreenCaptureTarget.display()andScreenCaptureTarget.window()accessors.
180-209: Windows restoration logic improved to prevent unwanted resizing.The enhanced restoration sequence now:
- Only restores if the window is actually minimized (
IsIconic)- Preserves window size/position via
WINDOWPLACEMENT- Falls back gracefully if placement retrieval fails
- Always brings the window to the foreground
This addresses the PR's stated issue where "other applications' windows change size" on Windows in multi-monitor setups.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/desktop/src-tauri/src/target_select_overlay.rs (2)
68-75: Consider using.and_then()for flatter Option handling.The current logic is correct but could be more idiomatic. Since
display()andwindow()already returnOption, using.and_then()instead of.map()would avoid nested Options and improve readability.Apply this diff to simplify the Option handling:
- let display = focused_target - .as_ref() - .map(|v| v.display()) - .unwrap_or_else(|| scap_targets::Display::get_containing_cursor()); - let window = focused_target - .as_ref() - .map(|v| v.window().and_then(|id| scap_targets::Window::from_id(&id))) - .unwrap_or_else(|| scap_targets::Window::get_topmost_at_cursor()); + let display = focused_target + .as_ref() + .and_then(|v| v.display()) + .or_else(|| scap_targets::Display::get_containing_cursor()); + let window = focused_target + .as_ref() + .and_then(|v| v.window().and_then(|id| scap_targets::Window::from_id(&id))) + .or_else(|| scap_targets::Window::get_topmost_at_cursor());
180-209: Excellent improvement to window restoration logic.The new approach properly preserves window size and position when restoring minimized windows by using
WINDOWPLACEMENT, which directly addresses potential issues with multimonitor setups mentioned in the PR objectives.Consider adding error logging for
SetWindowPlacementto aid debugging if restoration fails:if GetWindowPlacement(hwnd, &mut wp).is_ok() { // Restore using the previous placement to avoid resizing wp.showCmd = SW_RESTORE.0 as u32; - SetWindowPlacement(hwnd, &wp); + if SetWindowPlacement(hwnd, &wp).is_err() { + // Log error but continue - SetForegroundWindow still executes + ShowWindow(hwnd, SW_RESTORE); + } } else {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/desktop/src-tauri/src/target_select_overlay.rs(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Format Rust code usingrustfmtand ensure all Rust code passes workspace-level clippy lints.
Rust modules should be named with snake_case, and crate directories should be in kebab-case.
Files:
apps/desktop/src-tauri/src/target_select_overlay.rs
🧬 Code graph analysis (1)
apps/desktop/src-tauri/src/target_select_overlay.rs (3)
apps/desktop/src/utils/tauri.ts (1)
ScreenCaptureTarget(459-459)crates/recording/src/sources/screen_capture/mod.rs (2)
display(67-73)window(75-80)crates/scap-targets/src/lib.rs (6)
display(146-148)id(28-30)id(118-120)from_id(32-34)from_id(122-124)get_topmost_at_cursor(114-116)
⏰ 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 (2)
apps/desktop/src-tauri/src/target_select_overlay.rs (2)
9-9: LGTM!The import is correctly added to support the new
focused_targetparameter functionality.
47-51: LGTM!The addition of the optional
focused_targetparameter enables proper targeting on multimonitor setups, directly addressing the PR objectives.
|
Works nice! |
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Refactor