Improve area selection #1236
Improve area selection #1236
Conversation
WalkthroughAdded a broadcast-channel dependency, passed an Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Tauri as Tauri Window
participant Overlay as Target Select Overlay
participant BC as Broadcast Channel
User->>Tauri: open target-select overlay
Tauri->>Tauri: determine cursor vs target display
Tauri->>Overlay: create window with ?isHoveredDisplay=(true|false)
Overlay->>Overlay: init bounds (DEFAULT or EMPTY)
rect rgba(245,250,255,0.9)
Note over User,Overlay: Area creation flow
User->>Overlay: mousedown (root)
Overlay->>BC: post reset
Overlay->>Overlay: set creating=true, record start
User->>Overlay: mousemove
Overlay->>Overlay: update live bounds, render occluders
User->>Overlay: mouseup
Overlay->>Overlay: finalize bounds, set hasArea if valid
end
rect rgba(245,255,245,0.9)
Note over User,Overlay: Post-creation interactions
User->>Overlay: interact with ResizeHandles / drag controls
Overlay->>Overlay: apply constrained transforms, update occluders & placement
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
apps/desktop/src/routes/target-select-overlay.tsx (5)
7-7: Derive hasArea from bounds; avoid writable memo to prevent desync on cross-window resets.Writable memo can get “stuck” when other events (e.g., broadcast reset) change bounds. A pure memo tied to bounds is safer; hide the box while creating via creating().
- import { createWritableMemo } from "@solid-primitives/memo"; + // (no longer needed) @@ - const [hasArea, setHasArea] = createWritableMemo( - () => bounds.size.width > 0 && bounds.size.height > 0, - ); + const hasArea = createMemo( + () => bounds.size.width > 0 && bounds.size.height > 0, + ); @@ - setCreating(true); - setHasArea(false); + setCreating(true); @@ - mouseup: () => { - setCreating(false); - // Only set hasArea if we created a meaningful area - if (bounds.size.width > 10 && bounds.size.height > 10) { - setHasArea(true); - } else { - // Reset to no area if too small - setBounds({ - position: { x: 0, y: 0 }, - size: { width: 0, height: 0 }, - }); - } - dispose(); - }, + mouseup: () => { + setCreating(false); + if (!(bounds.size.width > 10 && bounds.size.height > 10)) { + setBounds(structuredClone(EMPTY_BOUNDS)); + } + dispose(); + }, @@ - <Show when={hasArea()}> + <Show when={hasArea() && !creating()}> @@ - <Show when={!creating()}> + <Show when={!creating()}> <Show when={!hasArea()}> <p class="z-10 text-xl pointer-events-none text-white">Click and drag to select area</p> </Show> - <Show when={hasArea()}> + <Show when={hasArea()}> <p class="z-10 text-xl pointer-events-none text-white absolute bottom-4">Click and drag to create new area</p> </Show> </Show>Also add
createMemoto the Solid import list and remove thecreateWritableMemoimport.Also applies to: 285-291, 704-712, 796-807, 661-662, 684-697
42-51: Introduce an explicit Bounds type instead of using typeof bounds to improve typing at IPC boundaries.Relying on typeof bounds (a Store proxy type) is brittle and obscures the contract with tauri commands.
@@ -const EMPTY_BOUNDS = { +type Bounds = { + position: { x: number; y: number }; + size: { width: number; height: number }; +}; + +const EMPTY_BOUNDS: Bounds = { position: { x: 0, y: 0 }, size: { width: 0, height: 0 }, }; @@ -const DEFAULT_BOUNDS = { +const DEFAULT_BOUNDS: Bounds = { position: { x: 100, y: 100 }, size: { width: 400, height: 300 }, }; @@ -const [bounds, _setBounds] = createStore( - structuredClone(isPrimaryDisplay ? DEFAULT_BOUNDS : EMPTY_BOUNDS), -); +const [bounds, _setBounds] = createStore<Bounds>( + structuredClone(isPrimaryDisplay ? DEFAULT_BOUNDS : EMPTY_BOUNDS), +); @@ -const setBounds = (newBounds: typeof bounds) => { +const setBounds = (newBounds: Bounds) => { const clampedBounds: Bounds = { position: { x: Math.max(0, newBounds.position.x), y: Math.max(0, newBounds.position.y), }, size: { width: Math.max( 150, Math.min( window.innerWidth - Math.max(0, newBounds.position.x), newBounds.size.width, ), ), height: Math.max( 150, Math.min( window.innerHeight - Math.max(0, newBounds.position.y), newBounds.size.height, ), ), }, }; _setBounds(clampedBounds); };Also applies to: 112-115, 124-149
611-651: Occluders depend on window.innerWidth/innerHeight; consider making them reactive or using CSS calc.Unlikely to matter (overlay usually fixed), but a small tweak improves correctness on DPI/resize changes.
Example (Right occluder):
- style={{ - width: `${window.innerWidth - (bounds.size.width + bounds.position.x)}px`, - }} + style={{ + right: 0, + width: `calc(100vw - ${bounds.size.width + bounds.position.x}px)`, + }}Apply similarly for bottom occluder with 100vh.
65-70: Follow-up to Rust change: rename isPrimaryDisplay param to isCursorDisplay (or implement true primary).If you adopt the rename in Rust, update the search params and local variable here accordingly.
- const [params] = useSearchParams<{ displayId: DisplayId; isPrimaryDisplay: string }>(); - const isPrimaryDisplay = params.isPrimaryDisplay === "true"; + const [params] = useSearchParams<{ displayId: DisplayId; isCursorDisplay: string }>(); + const isPrimaryDisplay = params.isCursorDisplay === "true"; // or rename this local tooPlease confirm intended behavior: default box only on system primary display, or on the display under the cursor at overlay launch? Based on PR description, it sounds like “primary”.
42-51: Nit: centralize min size as a constant to keep behavior consistent across setBounds and all handles.You repeat 150 several times; a single MIN_SIZE constant reduces mistakes.
+const MIN_SIZE = 150; ... - width: Math.max(150, /*...*/) + width: Math.max(MIN_SIZE, /*...*/)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
apps/desktop/package.json(1 hunks)apps/desktop/src-tauri/src/windows.rs(2 hunks)apps/desktop/src/routes/target-select-overlay.tsx(5 hunks)packages/ui-solid/src/auto-imports.d.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{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:
packages/ui-solid/src/auto-imports.d.tsapps/desktop/src/routes/target-select-overlay.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:
packages/ui-solid/src/auto-imports.d.tsapps/desktop/src/routes/target-select-overlay.tsx
**/*.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/windows.rs
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.tsx
🧬 Code graph analysis (2)
apps/desktop/src-tauri/src/windows.rs (3)
crates/scap-targets/src/platform/macos.rs (2)
get_containing_cursor(78-92)display(519-539)crates/scap-targets/src/platform/win.rs (2)
get_containing_cursor(152-165)display(1044-1051)crates/scap-targets/src/lib.rs (2)
get_containing_cursor(36-38)display(146-148)
apps/desktop/src/routes/target-select-overlay.tsx (1)
apps/desktop/src/utils/tauri.ts (1)
DisplayId(386-386)
⏰ 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 (2)
packages/ui-solid/src/auto-imports.d.ts (1)
79-79: LGTM – new icon auto-imports.No issues; aligns with auto-imported icon pattern.
Also applies to: 83-83
apps/desktop/src/routes/target-select-overlay.tsx (1)
124-149: Guard setBounds against invalid inputs at call sites sourced from IPC.When calling setBounds(windowUnderCursor.bounds), ensure the shape matches Bounds to avoid runtime surprises from platform differences (e.g., fractional vs integer values).
Add a narrow zod schema check (dev-only) or a TypeScript type assertion where bounds come from Tauri.
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 (1)
apps/desktop/src-tauri/src/windows.rs (1)
303-317: Compute true OS primary display — the cursor-display comparison is semantically incorrect and causes wrong bounds initialization.The code currently assigns
is_primary_displayby checking if the cursor is over the current display, but the variable name and its usage in the TypeScript overlay initialization (controlling bounds) expect the OS primary display.scap_targets::Display::primary()exists and is public—use it instead.- let is_primary_display = scap_targets::Display::get_containing_cursor() - .map(|d| d.id()) - == Some(display.id()); + let is_primary_display = scap_targets::Display::primary().id() == display.id();This fix ensures the overlay uses DEFAULT_BOUNDS initialization on the actual OS primary display, rather than only when the cursor happens to be there.
🧹 Nitpick comments (3)
apps/desktop/src/routes/target-select-overlay.tsx (3)
124-149: Avoid magic numbers for min area; centralize to a constant.150 is repeated and hard-coded. Define a single MIN_AREA constant and reuse in clamps and handle math.
+const MIN_AREA = 150; ... - width: Math.max( - 150, + width: Math.max( + MIN_AREA, Math.min( window.innerWidth - Math.max(0, newBounds.position.x), newBounds.size.width, ), ), - height: Math.max( - 150, + height: Math.max( + MIN_AREA, Math.min( window.innerHeight - Math.max(0, newBounds.position.y), newBounds.size.height, ), ),Propagate MIN_AREA into ResizeHandles computations as well.
655-701: Emit “reset” when creation actually starts; drop global window mousedown emitter.Posting reset on every window mousedown can cause self-resets and unnecessary chatter. Emit once at the start of area creation here and remove the global emitter.
- onMouseDown={(downEvent) => { + onMouseDown={(downEvent) => { // Start creating a new area downEvent.preventDefault(); + postMessage?.({ type: "reset", senderId }); setCreating(true); setHasArea(false);And remove the earlier window-level mousedown post (see broadcast-channel comment).
679-699: Creation size clamps during drag may force 150px immediately.Because setBounds enforces MIN_AREA, the selection jumps to min size on the first move. If you want true “rubber band” UX, defer min-size enforcement to mouseup and allow smaller preview during creation. Optional.
Example approach:
- During creating, write directly via _setBounds without min clamp.
- On mouseup, if width/height < MIN_AREA, expand to MIN_AREA anchored to drag origin.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
apps/desktop/package.json(1 hunks)apps/desktop/src-tauri/src/windows.rs(2 hunks)apps/desktop/src/routes/target-select-overlay.tsx(5 hunks)packages/ui-solid/src/auto-imports.d.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{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:
packages/ui-solid/src/auto-imports.d.tsapps/desktop/src/routes/target-select-overlay.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:
packages/ui-solid/src/auto-imports.d.tsapps/desktop/src/routes/target-select-overlay.tsx
**/*.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/windows.rs
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.tsx
🧬 Code graph analysis (2)
apps/desktop/src-tauri/src/windows.rs (4)
apps/desktop/src-tauri/src/target_select_overlay.rs (1)
scap_targets(52-55)crates/scap-targets/src/platform/macos.rs (2)
get_containing_cursor(78-92)display(519-539)crates/scap-targets/src/platform/win.rs (2)
get_containing_cursor(152-165)display(1044-1051)crates/scap-targets/src/lib.rs (2)
get_containing_cursor(36-38)display(146-148)
apps/desktop/src/routes/target-select-overlay.tsx (1)
apps/desktop/src/utils/tauri.ts (1)
DisplayId(386-386)
🔇 Additional comments (2)
packages/ui-solid/src/auto-imports.d.ts (1)
79-79: LGTM! Auto-generated icon imports are correctly formatted.These icon declarations were automatically generated by
unplugin-auto-importand are correctly formatted and alphabetically ordered within the Lucide icons section. The newMaximizeandRatioicons align with the area selection improvements described in the PR.Also applies to: 83-83
apps/desktop/package.json (1)
24-24: No issues found — BroadcastChannel support and cleanup verified.Verification confirms BroadcastChannel is supported in Safari 15.4+ (macOS/WebKit) and WebView2 supports BroadcastChannel as a Chromium-based runtime. Your Tauri versions (2.3.0–2.5.0) have no compatibility issues.
The @solid-primitives/broadcast-channel library automatically cleans up the channel on owner cleanup, so no manual close/dispose calls are needed. The single usage in
target-select-overlay.tsx:116follows the correct pattern and requires no changes.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
apps/desktop/src/routes/target-select-overlay.tsx (2)
116-122: Critical: Global mousedown resets bounds on every click, breaking UI interactions and potentially causing zero-size recordings.The window-level mousedown listener broadcasts a reset on every click—including clicks on UI buttons like "Start Recording." The
onMessagehandler unconditionally clears bounds without filtering by message type. This can:
- Clear the selection area when clicking controls
- Start a recording with zero-size bounds if the reset happens mid-click
- Trigger resets from the sender's own messages
Apply this diff to fix:
const { postMessage, onMessage } = makeBroadcastChannel( "target_select_overlay", ); -createEventListener(window, "mousedown", () => - postMessage({ type: "reset" }), -); -onMessage(() => _setBounds(structuredClone(EMPTY_BOUNDS))); +const senderId = crypto.randomUUID(); +onMessage((msg: unknown) => { + const data = msg as { type?: string; senderId?: string }; + if (data?.type === "reset" && data?.senderId !== senderId) { + _setBounds(structuredClone(EMPTY_BOUNDS)); + } +}); +onCleanup(() => { + // Close broadcast channel on cleanup +});Then in the area creation mousedown handler (line 657), add:
postMessage({ type: "reset", senderId });
308-322: Resize listener leaks because createRoot is never disposed.The
createRoothere creates a disposal context, butdispose()is never invoked—onCleanupinside an undisposed root never executes. This leaks the window resize handler on every render.Apply this diff to fix:
-createRoot((dispose) => { - const onResize = () => { - const ctrlH = controlsEl?.offsetHeight ?? 64; - const margin = 16; - const wouldOverflow = - bounds.position.y + bounds.size.height + margin + ctrlH > - window.innerHeight; - setPlaceControlsAbove(wouldOverflow); - }; - window.addEventListener("resize", onResize); - onCleanup(() => { - window.removeEventListener("resize", onResize); - dispose(); - }); -}); +createEventListener(window, "resize", () => { + const ctrlH = controlsEl?.offsetHeight ?? 64; + const margin = 16; + const wouldOverflow = + bounds.position.y + bounds.size.height + margin + ctrlH > window.innerHeight; + setPlaceControlsAbove(wouldOverflow); +});
🧹 Nitpick comments (2)
apps/desktop/src/routes/target-select-overlay.tsx (2)
47-50: Consider making DEFAULT_BOUNDS display-aware.The hardcoded position (100, 100) and size (400, 300) may not work well on smaller displays or if the primary monitor isn't positioned at origin. Consider deriving these from
window.innerWidthandwindow.innerHeightto ensure the default box always fits within the visible viewport.
687-695: Consider extracting the minimum area threshold to a constant.The 10px threshold for meaningful area selection is hardcoded. Consider extracting to a named constant like
MIN_AREA_THRESHOLD = 10for clarity and maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/desktop/src-tauri/src/windows.rs(2 hunks)apps/desktop/src/routes/target-select-overlay.tsx(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src-tauri/src/windows.rs
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{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.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.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.tsx
🧬 Code graph analysis (1)
apps/desktop/src/routes/target-select-overlay.tsx (1)
apps/desktop/src/utils/tauri.ts (1)
DisplayId(386-386)
⏰ 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 (6)
apps/desktop/src/routes/target-select-overlay.tsx (6)
124-149: LGTM! Bounds clamping logic is correct.The clamping logic properly ensures bounds stay within the window and meet minimum size constraints (150px).
324-357: LGTM! Drag handler correctly manages lifecycle and uses requestAnimationFrame.The createRoot/dispose pattern here is correct:
dispose()is called on mouseup, properly cleaning up the event listeners.
359-609: LGTM! Resize handles correctly implement 8-directional resizing with proper constraints.All corner and edge handles properly enforce the 150px minimum size and adjust positions to compensate for clamped dimensions.
611-652: LGTM! Occluder overlay correctly covers non-selected screen areas.The four occluder divs properly partition the screen around the selected bounds.
816-975: LGTM! Recording controls properly handle authentication, mode selection, and recording initialization.The component correctly gates instant mode behind authentication and uses the typed mutation pattern.
997-1012: LGTM! ResizeHandle component correctly centers handles and accepts style overrides.The transform translation and prop spreading are implemented correctly.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
apps/desktop/src/routes/target-select-overlay.tsx (3)
66-70: Verify param name and semantics match the backend implementation.The param is named
isHoveredDisplay, but ensure this aligns with what the backend sends (see past comments suggestingisCursorDisplayor a trueisPrimaryDisplaycomputed in Rust). Confirm the parameter key and the derived boolean accurately reflect the backend's intent.Based on learnings from past review comments.
310-316: PrefercreateEventListenerfor the window resize handler.The current
createEffect+ manualaddEventListener+onCleanuppattern works, butcreateEventListener(already imported) provides automatic lifecycle management and is the idiomatic Solid primitive for event handling.Apply this diff:
-createEffect(() => { - setControlsHeight(controlsEl?.offsetHeight); - const onResize = () => setControlsHeight(controlsEl?.offsetHeight); - - window.addEventListener("resize", onResize); - onCleanup(() => window.removeEventListener("resize", onResize)); -}); +createEffect(() => { + setControlsHeight(controlsEl?.offsetHeight); +}); +createEventListener(window, "resize", () => { + setControlsHeight(controlsEl?.offsetHeight); +});Based on learnings from past review comments.
116-123: Critical: Broadcast reset on every window mousedown breaks UI interactions and lacks proper message filtering.The global
windowmousedown listener broadcasts reset on every click, including clicks on the Start Recording button, gear icon, and other controls. This clears bounds mid-interaction and can start recordings with zero-size areas. Additionally:
onMessagedoesn't filter bytype, so any broadcast triggers a reset.- No channel cleanup on unmount.
- No sender ID to prevent self-reset during multi-step interactions.
Fix:
- Remove the global window mousedown listener.
- Broadcast reset only when starting a new area selection (inside the overlay surface mousedown handler around line 651).
- Filter
onMessagebymsg?.type === "reset".- Add a sender ID to outgoing messages and ignore messages from self.
- Close the channel on cleanup.
Apply this pattern (based on past review guidance):
-const { postMessage, onMessage } = makeBroadcastChannel<{ type: "reset" }>( +const { postMessage, onMessage, close } = makeBroadcastChannel<{ type: "reset"; senderId?: string }>( "target_select_overlay", ); -createEventListener(window, "mousedown", () => - postMessage({ type: "reset" }), -); -onMessage(() => setBounds(structuredClone(EMPTY_BOUNDS))); +const senderId = crypto.randomUUID(); +onMessage((msg) => { + if (msg?.type === "reset" && msg?.senderId !== senderId) { + setBounds(structuredClone(EMPTY_BOUNDS)); + } +}); +onCleanup(() => close?.());Then in the overlay surface mousedown handler (around line 651), add:
onMouseDown={(downEvent) => { // Start creating a new area downEvent.preventDefault(); + postMessage({ type: "reset", senderId }); setState("creating"); setHasArea(false);Based on learnings from past review comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/desktop/src/routes/target-select-overlay.tsx(6 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{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.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.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.tsx
🧬 Code graph analysis (1)
apps/desktop/src/routes/target-select-overlay.tsx (1)
apps/desktop/src/utils/tauri.ts (1)
DisplayId(386-386)
⏰ 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: Analyze (rust)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
🔇 Additional comments (5)
apps/desktop/src/routes/target-select-overlay.tsx (5)
2-2: LGTM: Imports align with new broadcast channel and writable memo usage.The new imports are properly utilized throughout the refactored area selection flow.
Also applies to: 7-7
43-51: LGTM: Well-defined layout constants.
EMPTY_BOUNDSandDEFAULT_BOUNDSprovide clear initial states for display-aware initialization.
318-351: LGTM: Well-structured drag helper with proper cleanup.The
createOnMouseDownhelper correctly:
- Captures
startBoundsat interaction start.- Uses
createRootfor isolated scope with global listeners.- Throttles updates with
requestAnimationFrame.- Disposes listeners on
mouseup.
353-603: LGTM: Comprehensive resize handles with correct math and constraints.The
ResizeHandlescomponent properly implements:
- Eight handles (4 corners + 4 edges) with appropriate cursors.
- Correct positioning logic for each handle type.
- Minimum 150px size enforcement.
- Position compensation when size limits are hit.
- Event propagation control with
stopPropagation.
605-646: LGTM: Occluders and area creation/dragging logic are well-implemented.The implementation correctly:
- Darkens non-selected areas with properly calculated occluder dimensions.
- Handles bidirectional drag during creation (Math.abs for width/height, Math.min for position).
- Validates minimum area size (>10px) before committing.
- Clamps position explicitly during drag (lines 729-742) before using
_setBounds.- Shows appropriate visual feedback and instruction text based on state.
Also applies to: 698-804
There was a problem hiding this comment.
Actionable comments posted: 4
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/target-select-overlay.tsx (1)
1002-1006: Remove unused helper.getDisplayId isn’t referenced; drop it to reduce surface area.
♻️ Duplicate comments (2)
apps/desktop/src/routes/target-select-overlay.tsx (2)
120-127: Scope the broadcast reset, filter by type, ignore self, and close channel (remove global window mousedown).Global window mousedown broadcasts reset on every click; onMessage resets on any message. This wipes selection when clicking controls and can start with zero-size bounds. Add senderId, filter message.type, ignore self, and close on cleanup; only post when user starts a new selection on the overlay surface. This was previously flagged and remains unresolved.
Apply:
- const { postMessage, onMessage } = makeBroadcastChannel<{ type: "reset" }>( - "target_select_overlay", - ); - createEventListener(window, "mousedown", () => - postMessage({ type: "reset" }), - ); - onMessage(() => setBounds(EMPTY_BOUNDS)); + const { postMessage, onMessage, close } = makeBroadcastChannel< + { type: "reset"; senderId?: string } + >("target_select_overlay"); + const senderId = + (globalThis.crypto?.randomUUID?.() ?? + `${Date.now()}-${Math.random()}`).toString(); + onMessage((msg) => { + if (msg?.type === "reset" && msg?.senderId !== senderId) { + setBounds(structuredClone(EMPTY_BOUNDS)); + } + }); + onCleanup(() => close?.());
620-667: Emit typed reset with senderId only when starting creation on the overlay surface.Pair this with the filtered receiver above so other overlays clear, not the sender.
- // Start creating a new area + // Start creating a new area downEvent.preventDefault(); - postMessage({ type: "reset" }); + postMessage({ type: "reset", senderId });
🧹 Nitpick comments (2)
apps/desktop/src/routes/target-select-overlay.tsx (2)
258-267: Avoid mixed derived/mutable state for hasArea.createWritableMemo is derived from bounds but is also manually set, which is confusing. Prefer a pure derived memo or a plain signal. Given UI already derives “too small” from bounds, keep hasArea derived.
- const [hasArea, setHasArea] = createWritableMemo( - () => bounds.size.width > 0 && bounds.size.height > 0, - ); + const hasArea = createMemo( + () => bounds.size.width > 0 && bounds.size.height > 0, + ); - setHasArea(false); + // rely on bounds reset to compute hasArea=false - if (bounds.size.width > 10 && bounds.size.height > 10) { - setHasArea(true); - } else { + if (!(bounds.size.width > 10 && bounds.size.height > 10)) { setBounds(EMPTY_BOUNDS);Also applies to: 628-629, 654-656
577-618: Occluders also read window dimensions non-reactively.Use the same reactive viewport signals so occluders adjust on resize.
- width: `${window.innerWidth - (bounds.size.width + bounds.position.x)}px`, + width: `${viewportW() - (bounds.size.width + bounds.position.x)}px`, ... - height: `${window.innerHeight - (bounds.size.height + bounds.position.y)}px`, + height: `${viewportH() - (bounds.size.height + bounds.position.y)}px`,Add alongside viewportH:
+ const [viewportW, setViewportW] = createSignal(window.innerWidth); + createEventListener(window, "resize", () => setViewportW(window.innerWidth));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/desktop/src/routes/target-select-overlay.tsx(8 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{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.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.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.tsx
🧬 Code graph analysis (1)
apps/desktop/src/routes/target-select-overlay.tsx (1)
apps/desktop/src/utils/tauri.ts (1)
DisplayId(386-386)
⏰ 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 (1)
apps/desktop/src/routes/target-select-overlay.tsx (1)
69-75: Param rename to isHoveredDisplay looks consistent.Front-end now derives isHoveredDisplay from search params; aligns with the described backend change.
| const [controlsHeight, setControlsHeight] = createSignal< | ||
| number | undefined | ||
| >(undefined); | ||
| // Recompute placement when bounds change or window resizes | ||
| const placeControlsAbove = createMemo(() => { | ||
| const top = bounds.position.y; | ||
| const height = bounds.size.height; | ||
| // Measure controls height (fallback to 64px if not yet mounted) | ||
| const ctrlH = controlsHeight() ?? 64; | ||
| const margin = 16; | ||
|
|
||
| const wouldOverflow = | ||
| top + height + margin + ctrlH > window.innerHeight; | ||
| return wouldOverflow; | ||
| }); | ||
| onMount(() => setControlsHeight(controlsEl?.offsetHeight)); | ||
| createEventListener(window, "resize", () => | ||
| setControlsHeight(controlsEl?.offsetHeight), | ||
| ); | ||
|
|
There was a problem hiding this comment.
placeControlsAbove isn’t reactive to viewport size changes.
You read window.innerHeight in a memo without a reactive source; if controlsHeight doesn’t change on resize, placement won’t recompute. Track viewport height explicitly.
- const [controlsHeight, setControlsHeight] = createSignal<number | undefined>(undefined);
+ const [controlsHeight, setControlsHeight] = createSignal<number | undefined>(undefined);
+ const [viewportH, setViewportH] = createSignal(window.innerHeight);
- const placeControlsAbove = createMemo(() => {
+ const placeControlsAbove = createMemo(() => {
const top = bounds.position.y;
const height = bounds.size.height;
const ctrlH = controlsHeight() ?? 64;
const margin = 16;
- const wouldOverflow = top + height + margin + ctrlH > window.innerHeight;
+ const wouldOverflow = top + height + margin + ctrlH > viewportH();
return wouldOverflow;
});
- onMount(() => setControlsHeight(controlsEl?.offsetHeight));
- createEventListener(window, "resize", () => setControlsHeight(controlsEl?.offsetHeight));
+ onMount(() => {
+ setControlsHeight(controlsEl?.offsetHeight);
+ setViewportH(window.innerHeight);
+ });
+ createEventListener(window, "resize", () => {
+ setControlsHeight(controlsEl?.offsetHeight);
+ setViewportH(window.innerHeight);
+ });🤖 Prompt for AI Agents
In apps/desktop/src/routes/target-select-overlay.tsx around lines 270 to 289,
placeControlsAbove reads window.innerHeight directly so the memo won't rerun on
viewport changes; add a reactive viewport height signal (e.g., const
[viewportHeight, setViewportHeight] = createSignal(window.innerHeight)), set it
on mount and update it inside the existing resize listener
(setViewportHeight(window.innerHeight)), and then use viewportHeight() in the
memo instead of window.innerHeight so placement recomputes when the viewport is
resized; keep the existing controlsHeight updates as-is.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
apps/desktop/src/routes/target-select-overlay.tsx (3)
69-73: Verify param alignment with backend changes.Past reviews indicate the backend parameter naming may have changed (from
isPrimaryDisplaytoisCursorDisplayor similar). Ensure the front-end param key matches what the Rust backend actually sends inwindows.rs.
116-126: Critical: Broadcast channel issues remain unaddressed.The global window
mousedownlistener (line 123-125) and unfilteredonMessagehandler (line 126) create multiple issues already flagged in past reviews:
- Every window click (including UI button clicks) broadcasts a reset, clearing the area mid-interaction
- No message type filtering—any broadcast resets bounds
- No sender ID—windows can reset themselves
- No cleanup—channel remains open after unmount
See past review comments for the comprehensive fix involving message filtering, sender IDs, scoped reset calls, and cleanup.
270-288: placeControlsAbove won't recompute on viewport resize.The memo reads
window.innerHeightdirectly (line 282), which isn't a reactive signal. If the window resizes butcontrolsHeight()doesn't change, placement won't update.See past review comment for the fix: add a
viewportHeightsignal, update it in the resize listener, and use it in the memo.
🧹 Nitpick comments (1)
apps/desktop/src/routes/target-select-overlay.tsx (1)
46-49: EMPTY_BOUNDS as a function is an unusual pattern; prefer a constant with structuredClone at call sites.Defining
EMPTY_BOUNDSas a function that returns a new object each time is unconventional. Since you already usestructuredClone(EMPTY_BOUNDS())in line 126 andsetBounds(EMPTY_BOUNDS())elsewhere, you're creating nested object creation overhead.Apply this diff to use a constant instead:
-const EMPTY_BOUNDS = () => ({ +const EMPTY_BOUNDS = { position: { x: 0, y: 0 }, size: { width: 0, height: 0 }, -}); +};Then update call sites to clone:
- onMessage(() => setBounds(EMPTY_BOUNDS())); + onMessage(() => setBounds(structuredClone(EMPTY_BOUNDS))); - setBounds(EMPTY_BOUNDS()); + setBounds(structuredClone(EMPTY_BOUNDS));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/desktop/src/routes/target-select-overlay.tsx(8 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{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.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.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.tsx
🧬 Code graph analysis (1)
apps/desktop/src/routes/target-select-overlay.tsx (1)
apps/desktop/src/utils/tauri.ts (1)
DisplayId(386-386)
⏰ 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 (5)
apps/desktop/src/routes/target-select-overlay.tsx (5)
290-323: Good use of createRoot with proper disposal.The
createOnMouseDownhelper correctly creates an isolated reactive root and disposes it on mouseup (line 308), preventing memory leaks. TherequestAnimationFramethrottling is also appropriate for mousemove handlers.
325-575: Resize handles now correctly use MIN_WIDTH/MIN_HEIGHT constants.All resize handle calculations properly enforce the minimum dimensions using the constants (200×100), addressing the previous inconsistency with hardcoded
150values. The implementation is now consistent with theisAreaTooSmallvalidation.
623-664: Area creation correctly broadcasts reset on interaction start.The
postMessage({ type: "reset" })call on line 626 is properly scoped to when the user begins creating a new area, not on every window click. This is the correct placement per the architecture discussed in past reviews.However, note that the broadcast channel itself still needs the filtering/cleanup fixes flagged in the critical issue comment.
789-794: isAreaTooSmall correctly validates using shared constants.The validation function properly uses
MIN_WIDTHandMIN_HEIGHT, and now that the resize handles also use these constants (verified in lines 325-575), the UX is fully consistent—users cannot create or resize to dimensions that would be flagged as "too small."
860-959: Good UX: clear feedback for undersized areas.The conditional rendering with a descriptive fallback message (lines 863-875) provides helpful feedback, showing both the minimum required dimensions and the current selection size. This aligns well with the validation logic.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
apps/desktop/src/routes/target-select-overlay.tsx (2)
120-126: 🚨 CRITICAL: Global mousedown still broadcasts reset on every click, including UI interactions.Despite being marked as addressed in previous reviews, the global window mousedown listener (lines 123-125) still broadcasts a reset on every mouse click, including clicks on UI controls like "Start Recording". The
onMessagehandler (line 126) doesn't filter by message type and has no cleanup.Impact:
- Clicking "Start Recording" can reset bounds to zero before the command reads them
- Any UI interaction clears the selection
- The window resets its own selection during creation
Apply this diff to fix:
- const { postMessage, onMessage } = makeBroadcastChannel<{ type: "reset" }>( + const { postMessage, onMessage, close } = makeBroadcastChannel<{ type: "reset" }>( "target_select_overlay", ); - createEventListener(window, "mousedown", () => - postMessage({ type: "reset" }), - ); - onMessage(() => setBounds(EMPTY_BOUNDS())); + onMessage((msg) => { + if (msg?.type === "reset") setBounds(EMPTY_BOUNDS()); + }); + onCleanup(() => close?.());The reset broadcast now only happens at line 626 when actually starting a new area creation, which is the correct behavior.
270-288: placeControlsAbove reads window.innerHeight directly—not reactive to viewport resize.The memo at line 274 reads
window.innerHeight(line 282) without a reactive source. If the viewport resizes butcontrolsHeightdoesn't change, the placement won't recompute. This can cause controls to overflow or remain above when there's sufficient space below.Apply this diff to track viewport height reactively:
const [controlsHeight, setControlsHeight] = createSignal< number | undefined >(undefined); + const [viewportHeight, setViewportHeight] = createSignal(window.innerHeight); + // Recompute placement when bounds change or window resizes const placeControlsAbove = createMemo(() => { const top = bounds.position.y; const height = bounds.size.height; // Measure controls height (fallback to 64px if not yet mounted) const ctrlH = controlsHeight() ?? 64; const margin = 16; const wouldOverflow = - top + height + margin + ctrlH > window.innerHeight; + top + height + margin + ctrlH > viewportHeight(); return wouldOverflow; }); - onMount(() => setControlsHeight(controlsEl?.offsetHeight)); + onMount(() => { + setControlsHeight(controlsEl?.offsetHeight); + setViewportHeight(window.innerHeight); + }); createEventListener(window, "resize", () => - setControlsHeight(controlsEl?.offsetHeight), + { + setControlsHeight(controlsEl?.offsetHeight); + setViewportHeight(window.innerHeight); + } );Based on previous review comments.
🧹 Nitpick comments (1)
apps/desktop/src/routes/target-select-overlay.tsx (1)
51-54: Consider making DEFAULT_BOUNDS a function for consistency and safety.While
structuredCloneis used at line 117, makingDEFAULT_BOUNDSa function likeEMPTY_BOUNDSwould prevent accidental mutations and maintain consistency across the codebase.Apply this diff:
-const DEFAULT_BOUNDS = { +const DEFAULT_BOUNDS = () => ({ position: { x: 100, y: 100 }, size: { width: 400, height: 300 }, -}; +});Then update line 117:
- structuredClone(isHoveredDisplay ? DEFAULT_BOUNDS : EMPTY_BOUNDS()), + isHoveredDisplay ? DEFAULT_BOUNDS() : EMPTY_BOUNDS(),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/desktop/src/routes/target-select-overlay.tsx(8 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{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.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.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.tsx
🧬 Code graph analysis (1)
apps/desktop/src/routes/target-select-overlay.tsx (1)
apps/desktop/src/utils/tauri.ts (1)
DisplayId(386-386)
⏰ 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 (4)
apps/desktop/src/routes/target-select-overlay.tsx (4)
69-73: Verify param name matches backend implementation.Past reviews flagged a mismatch between frontend param naming (
isPrimaryDisplay) and backend behavior (cursor display vs OS primary). The current code usesisHoveredDisplay. Ensure this param name and semantics align with what the backend actually sends inwindows.rs.Based on previous review comments.
325-575: Excellent: Resize handles now consistently use MIN_WIDTH and MIN_HEIGHT.All 8 resize handles (4 corners + 4 edges) correctly enforce the minimum dimensions using the
MIN_WIDTHandMIN_HEIGHTconstants. This resolves the previous inconsistency where hardcoded150values caused UX confusion.
623-664: Area creation flow is well-structured.The click-and-drag creation UX is correctly implemented:
- Line 626: Broadcasts reset only when starting a new selection (not on every click)
- Lines 638-663: Proper lifecycle management with
createRootand cleanup on mouseup- Lines 654-659: Smart validation—only commits areas larger than 10×10 pixels
789-793: Good: Area size validation with clear user feedback.The
isAreaTooSmallhelper correctly validates againstMIN_WIDTHandMIN_HEIGHT, and the fallback UI (lines 864-874) provides clear, actionable feedback showing both the requirement and current dimensions.Also applies to: 861-876
Click and drag instead of having to move around the box around.
TODO:
Summary by CodeRabbit
New Features
Chores