Skip to content

Conversation

@oscartbeaumont
Copy link
Contributor

@oscartbeaumont oscartbeaumont commented Oct 17, 2025

Reverts some changes introduced in #1104

Summary by CodeRabbit

  • Refactor
    • Streamlined window overlay targeting logic to improve accuracy and reliability when selecting windows across multiple displays.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 17, 2025

Walkthrough

The window targeting overlay logic is simplified by removing intermediate computed state. The conditional gating now directly checks if the target is under the current display, and window content rendering directly uses targetUnderCursor.window instead of relying on a previously computed windowToShow() function.

Changes

Cohort / File(s) Summary
Window Targeting Logic Simplification
apps/desktop/src/routes/target-select-overlay.tsx
Simplified conditional gating from composite check to direct target-under-display verification; removed windowToShow() indirection and now directly references targetUnderCursor.window for window content rendering

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

The changes involve careful logic simplification in a single file. Review should verify that the simplified conditionals maintain equivalent behavior and don't inadvertently alter window selection or display targeting functionality.

Possibly related PRs

Poem

🐰 Where windows once were called by name,
Now cursors find their aim with grace,
Simplicity becomes the game,
Direct paths hold their rightful place! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The PR title "Fix Target Select Overlay window bounds on multimonitor setups" clearly and specifically describes the main change in the changeset. The changes simplify the window overlay conditional logic in the target-select-overlay component specifically to address issues with multimonitor display scenarios, which directly aligns with the title's focus on fixing window bounds on multimonitor setups. The title is concise, uses appropriate action language ("Fix"), and provides enough context that a developer reviewing the commit history would understand the primary purpose of the change without needing to inspect the actual code modifications.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix-tso

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@oscartbeaumont oscartbeaumont marked this pull request as ready for review October 17, 2025 14:21
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
apps/desktop/src/routes/target-select-overlay.tsx (1)

224-224: Simplified rendering logic - consider cleanup.

The change to directly use targetUnderCursor.window simplifies the rendering logic and aligns with the multimonitor fix.

However, this makes the selectedWindow query (lines 69-90) and the fallback logic in windowToShow() (lines 95-97) effectively unused, since the overlay now only renders when actively hovering over a window. Consider removing or refactoring this unused code to improve maintainability.

Apply this diff to remove the unused query and simplify windowToShow():

-	const selectedWindow = createQuery(() => ({
-		queryKey: ["selectedWindow", rawOptions.captureTarget],
-		queryFn: async () => {
-			if (rawOptions.captureTarget.variant !== "window") return null;
-			const windowId = rawOptions.captureTarget.id;
-
-			const windows = await commands.listCaptureWindows();
-			const window = windows.find((w) => w.id === windowId);
-
-			if (!window) return null;
-
-			return {
-				id: window.id,
-				app_name: window.owner_name || window.name || "Unknown",
-				bounds: window.bounds,
-			};
-		},
-		enabled:
-			rawOptions.captureTarget.variant === "window" &&
-			rawOptions.targetMode === "window",
-		staleTime: 5 * 1000,
-	}));
-
 	const windowToShow = () => {
-		const hoveredWindow = targetUnderCursor.window;
-		if (hoveredWindow) return hoveredWindow;
-		if (rawOptions.captureTarget.variant === "window") {
-			const selected = selectedWindow.data;
-			if (selected) return selected;
-		}
-		return hoveredWindow;
+		return targetUnderCursor.window;
 	};
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fcbb8fb and af7a48d.

📒 Files selected for processing (1)
  • apps/desktop/src/routes/target-select-overlay.tsx (1 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 running pnpm 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)

218-222: LGTM - Simplified multimonitor logic.

The condition now directly checks if the cursor is over the current display, removing the composite logic that could cause the overlay to appear on incorrect displays. This should fix the multimonitor issues mentioned in the PR.

@Brendonovich Brendonovich merged commit a0cc059 into main Oct 17, 2025
15 checks passed
@oscartbeaumont oscartbeaumont deleted the fix-tso branch October 17, 2025 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants