Area select fix button positions #1290
Conversation
WalkthroughReplaces top/bottom toggle with a memoized Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant O as TargetSelectOverlay
participant B as Browser / DOM
participant P as controlsPlacement (memo)
U->>O: open selection / resize
O->>B: measure controls on mount/resize
B-->>O: controlsWidth, controlsHeight
O->>P: compute placement (selection height, window space, control dims)
alt selection height >= 90% (full-height)
P-->>O: try -> right
alt right not feasible
P-->>O: try -> left
alt left not feasible
P-->>O: choose -> inside-bottom
end
end
else normal height
P-->>O: choose -> above or below based on overflow
end
O->>B: apply placement classes, width rules, and showBackground flag
B-->>U: controls rendered with chosen placement
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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: 0
🧹 Nitpick comments (4)
apps/desktop/src/routes/target-select-overlay.tsx (4)
277-308: Document magic numbers and verify fallback dimensions.The placement logic uses undocumented magic numbers:
300(line 285): Fallback width for controls0.9(line 289): Full-height threshold64(line 284): Fallback height for controlsThese fallback dimensions may cause incorrect initial placement before
onMountsets actual dimensions, potentially causing a visual flash.Consider:
- Extract these as named constants with explanatory comments
- Verify the 300px fallback matches typical control width
- Add JSDoc comments explaining the placement algorithm
Apply this diff:
+// Fallback dimensions for controls before they mount +const CONTROLS_FALLBACK_HEIGHT = 64; +const CONTROLS_FALLBACK_WIDTH = 300; +// Consider selection "full-height" if it spans 90% or more of window height +const FULL_HEIGHT_THRESHOLD = 0.9; + // Determine the best placement for controls const controlsPlacement = createMemo(() => { const top = bounds.position.y; const left = bounds.position.x; const width = bounds.size.width; const height = bounds.size.height; - // Measure controls dimensions (fallback if not yet mounted) - const ctrlH = controlsHeight() ?? 64; - const ctrlW = controlsWidth() ?? 300; + const ctrlH = controlsHeight() ?? CONTROLS_FALLBACK_HEIGHT; + const ctrlW = controlsWidth() ?? CONTROLS_FALLBACK_WIDTH; const margin = 16; - // Check if selection spans full height (or nearly full height) - const isFullHeight = height >= window.innerHeight * 0.9; + const isFullHeight = height >= window.innerHeight * FULL_HEIGHT_THRESHOLD;
314-317: Consider debouncing the resize handler.The resize event can fire very frequently during window resizing. Reading
offsetHeightandoffsetWidthsynchronously on every resize event may cause performance issues due to layout thrashing.Consider wrapping the dimension updates in
requestAnimationFrame:createEventListener(window, "resize", () => { - setControlsHeight(controlsEl?.offsetHeight); - setControlsWidth(controlsEl?.offsetWidth); + requestAnimationFrame(() => { + setControlsHeight(controlsEl?.offsetHeight); + setControlsWidth(controlsEl?.offsetWidth); + }); });
754-774: ExtractcontrolsPlacement()to avoid repeated calls.
controlsPlacement()is called multiple times in the JSX (lines 757-764 for classes, 768-773 for width). While memoized, extracting it once improves readability and reduces function call overhead.Apply this diff:
<div ref={controlsEl} + use:((el) => { + const placement = controlsPlacement(); + return ( <div - ref={controlsEl} class={cx( "flex absolute flex-col items-center m-2", - controlsPlacement().position === "above" && "bottom-full", - controlsPlacement().position === "below" && "top-full", - controlsPlacement().position === "right" && + placement.position === "above" && "bottom-full", + placement.position === "below" && "top-full", + placement.position === "right" && "left-full top-1/2 -translate-y-1/2", - controlsPlacement().position === "left" && + placement.position === "left" && "right-full top-1/2 -translate-y-1/2", - controlsPlacement().position === "inside-bottom" && + placement.position === "inside-bottom" && "bottom-2 left-1/2 -translate-x-1/2", )} style={{ width: - controlsPlacement().position === "right" || - controlsPlacement().position === "left" + placement.position === "right" || + placement.position === "left" ? "auto" - : controlsPlacement().position === "inside-bottom" + : placement.position === "inside-bottom" ? "auto" : `${bounds.size.width}px`, }} > + ); + })()}Alternative simpler approach: Destructure at the start of the JSX block:
+{(() => { + const placement = controlsPlacement(); + return ( <div ref={controlsEl} class={cx( "flex absolute flex-col items-center m-2", - controlsPlacement().position === "above" && "bottom-full", + placement.position === "above" && "bottom-full", // ... rest of the conditions using placement.position )} + ); +})()}
766-774: Simplify the width calculation logic.The nested ternary for width can be simplified. Currently, it checks
right || left→auto, theninside-bottom→auto, else → width. Since all three positions use"auto", this can be consolidated.style={{ width: - controlsPlacement().position === "right" || - controlsPlacement().position === "left" - ? "auto" - : controlsPlacement().position === "inside-bottom" - ? "auto" - : `${bounds.size.width}px`, + ["right", "left", "inside-bottom"].includes(controlsPlacement().position) + ? "auto" + : `${bounds.size.width}px`, }}Or if you extract placement as suggested in the previous comment:
style={{ width: - placement.position === "right" || - placement.position === "left" - ? "auto" - : placement.position === "inside-bottom" - ? "auto" - : `${bounds.size.width}px`, + ["right", "left", "inside-bottom"].includes(placement.position) + ? "auto" + : `${bounds.size.width}px`, }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/desktop/src/routes/target-select-overlay.tsx(2 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
⏰ 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 (1)
apps/desktop/src/routes/target-select-overlay.tsx (1)
273-275: LGTM!The
controlsWidthsignal follows the same pattern ascontrolsHeightand is correctly initialized.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/desktop/src/routes/target-select-overlay.tsx (2)
310-317: Consider debouncing the resize handler.The resize event can fire very frequently during window resizing, causing excessive recalculations of control dimensions and placement. This could impact performance during resize operations.
Consider using a debounced resize handler:
+import { debounce } from "@solid-primitives/scheduled"; + onMount(() => { setControlsHeight(controlsEl?.offsetHeight); setControlsWidth(controlsEl?.offsetWidth); }); -createEventListener(window, "resize", () => { +const updateDimensions = debounce(() => { setControlsHeight(controlsEl?.offsetHeight); setControlsWidth(controlsEl?.offsetWidth); -}); +}, 100); +createEventListener(window, "resize", updateDimensions);
766-774: Consider extracting width calculation for readability.The nested ternary for width calculation is correct but difficult to parse at a glance.
Consider extracting the width logic:
+const controlsWidth = createMemo(() => { + const pos = controlsPlacement().position; + if (pos === "right" || pos === "left" || pos === "inside-bottom") { + return "auto"; + } + return `${bounds.size.width}px`; +}); + <div ref={controlsEl} class={cx( "flex absolute flex-col items-center m-2", controlsPlacement().position === "above" && "bottom-full", controlsPlacement().position === "below" && "top-full", controlsPlacement().position === "right" && "left-full top-1/2 -translate-y-1/2", controlsPlacement().position === "left" && "right-full top-1/2 -translate-y-1/2", controlsPlacement().position === "inside-bottom" && "bottom-2 left-1/2 -translate-x-1/2", )} - style={{ - width: - controlsPlacement().position === "right" || - controlsPlacement().position === "left" - ? "auto" - : controlsPlacement().position === "inside-bottom" - ? "auto" - : `${bounds.size.width}px`, - }} + style={{ width: controlsWidth() }} >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/desktop/src/routes/target-select-overlay.tsx(5 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
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (rust)
🔇 Additional comments (4)
apps/desktop/src/routes/target-select-overlay.tsx (4)
273-275: LGTM!The
controlsWidthstate follows the same pattern ascontrolsHeightand is properly initialized.
782-784: LGTM!Correctly passes
showBackgroundwhen controls are positioned inside-bottom, ensuring the controls remain visible against the selection area.
814-818: LGTM!Good UX improvement—hiding the helper text when controls are positioned inside-bottom prevents visual clutter and overlap.
278-308: Verify fallback dimensions during initial render phase.The width handling reveals that the horizontal overflow concern is not applicable: for "above" and "below" positions, the controls container width is explicitly set to match the selection width (line 774), so no overflow occurs.
However, the fallback dimensions concern remains valid:
Fallback dimensions (line 285): The
ctrlW = 300pxfallback is used to determine placement before controls are measured. If the actual RecordingControls component has a different width (e.g., 250px or 400px), the initial placement decision could be suboptimal. The placement recalculates afteronMountwith the correct measured dimensions, but the initial render may show incorrect positioning.For right/left positions (where width is "auto"), the concern is less critical since width adapts to content. For above/below positions, the width matches the selection anyway, so fallback accuracy matters less there. The main risk is if the controls are significantly wider than 300px and need right/left placement—the initial decision could be wrong.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/desktop/src/routes/target-select-overlay.tsx (1)
999-1013: Fix conflicting hover opacity styles.Lines 1003 and 1005 both apply hover opacity utilities (
hover:opacity-80andhover:opacity-60) whenshowBackgroundis true, creating conflicting styles.Apply this diff to use a single hover opacity:
<button onClick={() => props.setToggleModeSelect?.(true)} class="cursor-pointer flex gap-1 items-center mb-5 transition-all duration-200" classList={{ - "bg-black/40 p-2 rounded-lg backdrop-blur-sm border border-white/10 hover:bg-black/50 hover:opacity-80": + "bg-black/40 p-2 rounded-lg backdrop-blur-sm border border-white/10 hover:bg-black/50": props.showBackground, - "hover:opacity-60": props.showBackground, }} >
🧹 Nitpick comments (1)
apps/desktop/src/routes/target-select-overlay.tsx (1)
767-774: Simplify nested ternary for better readability.The width calculation uses nested ternaries that can be flattened since both conditions return
"auto".Apply this diff:
style={{ width: controlsPlacement().position === "right" || - controlsPlacement().position === "left" + controlsPlacement().position === "left" || + controlsPlacement().position === "inside-bottom" ? "auto" - : controlsPlacement().position === "inside-bottom" - ? "auto" - : `${bounds.size.width}px`, + : `${bounds.size.width}px`, }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/desktop/src/routes/target-select-overlay.tsx(5 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
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Typecheck
- GitHub Check: Analyze (rust)
🔇 Additional comments (5)
apps/desktop/src/routes/target-select-overlay.tsx (5)
278-308: LGTM! Well-structured placement logic.The placement computation correctly handles both full-height and standard cases, with appropriate fallbacks and margin calculations. The 90% threshold for full-height detection is reasonable.
310-317: LGTM! Dimension tracking is correctly implemented.The onMount and resize handlers properly measure control dimensions, with fallback values in the memo handling cases where controls aren't yet rendered.
782-786: LGTM! Appropriate use of background styling.The
showBackgroundprop is correctly set totrueonly when controls are placed inside the selection, ensuring proper contrast.
815-820: LGTM! Hint text visibility aligns with placement.Hiding the hint when controls are inside the selection avoids visual clutter and improves UX.
833-833: LGTM! Prop addition is well-typed.The optional
showBackgroundprop is correctly typed and appropriately optional for flexible component usage.
Summary by CodeRabbit