Repository navigation
Fix #3856: honor focusPaneOnFirstClick for minimal-mode chrome and workspace sidebar - #3881
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds gated-first-mouse hosting views and an overlay for the workspace sidebar, updates two control overrides to respect PaneFirstClickFocusSettings, and expands tests with a sidebar harness that simulates active vs inactive first-click behavior. ChangesFirst-Click Focus Gating for Sidebar and Controls
Sequence DiagramsequenceDiagram
participant User
participant HostingOverlay as FirstMouseGatedHostingOverlay
participant PassThrough as FirstMouseGatedPassThroughHostingView
participant HostingView as FirstMouseGatedHostingView
participant Setting as PaneFirstClickFocusSettings
User->>PassThrough: mouse click on inactive window
PassThrough->>HostingView: hitTest(_:)
HostingView->>Setting: isEnabled() + window.isKeyWindow
Setting-->>HostingView: shouldCaptureInactiveFirstMouse result
HostingView-->>PassThrough: capture decision
alt Setting enabled or window active
PassThrough-->>PassThrough: return self (capture)
else Setting disabled and window inactive
PassThrough-->>User: return nil (pass through)
end
PassThrough->>PassThrough: acceptsFirstMouse(for:)
PassThrough-->>User: return setting.isEnabled()
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (13 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Greptile SummaryThis PR fixes #3856 by making the inactive first-click policy consistent across the workspace sidebar, minimal-mode sidebar controls, and PDF preview chrome — gating action delivery on
Confidence Score: 5/5The change is safe to merge. Hit-test coordinate conversion is correct, the overlay is correctly scoped to the sidebar, and the three acceptsFirstMouse gates are consistent with PaneFirstClickFocusSettings. All three production surfaces (sidebar overlay, PDF chrome, minimal-mode controls) now read from the same setting, the coordinate-space conversion in shouldCaptureInactiveFirstMouse correctly converts from superview space before the bounds check, and the split into FirstMouseGatedHostingView / FirstMouseGatedPassThroughHostingView eliminates the mutable-flag bad state. No incorrect data paths or activation regressions were found in the production code. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant User as User click
participant AppKit as AppKit hit-test
participant Overlay as FirstMouseGatedPassThroughHostingView
participant Sidebar as Sidebar NSViews
participant TM as TabManager
User->>AppKit: leftMouseDown (window inactive)
AppKit->>Overlay: hitTest(point)
alt focusPaneOnFirstClick disabled
Overlay->>Overlay: shouldCaptureInactiveFirstMouse → true
Overlay-->>AppKit: return self
AppKit->>Overlay: acceptsFirstMouse → false
AppKit->>AppKit: activate window only (no action dispatch)
Note over TM: workspace unchanged
else focusPaneOnFirstClick enabled
Overlay->>Overlay: shouldCaptureInactiveFirstMouse → false
Overlay-->>AppKit: return nil
AppKit->>Sidebar: hitTest(point) → underlying view
AppKit->>Sidebar: dispatch mouseDown
Sidebar->>TM: selectWorkspace(target)
end
Reviews (5): Last reviewed commit: "Honor inactive first-click policy in sid..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| private func shouldCaptureInactiveFirstMouse(at point: NSPoint) -> Bool { | ||
| window?.isKeyWindow != true && | ||
| !PaneFirstClickFocusSettings.isEnabled() && | ||
| bounds.contains(point) | ||
| } |
There was a problem hiding this comment.
hitTest(_:) receives point in the superview's coordinate system (per NSView docs), but bounds is in the receiver's own coordinate system. The two coincide only when frame.origin == (0, 0). For a SwiftUI overlay that can be placed at a non-zero y-origin — e.g., after ignoresSafeArea, safe-area insets, or any layout shift on the ZStack — bounds.contains(point) returns false for points that actually fall inside the view, causing the check to miss the gate and fall through to passesThroughWhenNotGating = true, silently delivering inactive first clicks to sidebar rows. The fix is to check against the frame (frame.contains(point)) or convert first (bounds.contains(convert(point, from: superview))).
There was a problem hiding this comment.
I verified this one against AppKit's hit-testing contract: NSView.hitTest(_:) receives the point in the receiver's coordinate space, so bounds.contains(point) is the correct check here. I added a source comment documenting that invariant.
— Claude Code
There was a problem hiding this comment.
Updated after re-checking the AppKit hitTest(_:) docs. The current code now converts from the superview into local bounds before checking containment.
— Claude Code
| var passesThroughWhenNotGating = false | ||
|
|
||
| override var intrinsicContentSize: NSSize { | ||
| NSSize(width: NSView.noIntrinsicMetric, height: NSView.noIntrinsicMetric) | ||
| } | ||
|
|
||
| override func hitTest(_ point: NSPoint) -> NSView? { | ||
| if shouldCaptureInactiveFirstMouse(at: point) { | ||
| return self | ||
| } | ||
| if passesThroughWhenNotGating { | ||
| return nil | ||
| } | ||
| return super.hitTest(point) | ||
| } | ||
|
|
||
| override func acceptsFirstMouse(for event: NSEvent?) -> Bool { | ||
| PaneFirstClickFocusSettings.isEnabled() | ||
| } | ||
|
|
||
| private func shouldCaptureInactiveFirstMouse(at point: NSPoint) -> Bool { | ||
| window?.isKeyWindow != true && | ||
| !PaneFirstClickFocusSettings.isEnabled() && | ||
| bounds.contains(point) | ||
| } | ||
| } |
There was a problem hiding this comment.
passesThroughWhenNotGating mutable flag leaves a bad default state representable
passesThroughWhenNotGating defaults to false, but the only consumer always immediately sets it to true. In the false state (the default before makeNSView/updateNSView assigns it), the view wraps EmptyView, so super.hitTest returns self for any point in bounds, silently intercepting every click. This matches the architectural-rethink rule's concern about new mutable flags that create a second owner for behavior already owned by a single path. The simplest resolution would be a custom initializer that takes passesThroughWhenNotGating as a constructor argument, or splitting into two distinct types so the bad state is unexpressible.
Rule Used: Flag Swift fixes that patch symptoms while leaving... (source)
There was a problem hiding this comment.
Fixed by splitting the pass-through overlay into a dedicated FirstMouseGatedPassThroughHostingView subclass. The mutable passesThroughWhenNotGating flag is gone, so the bad default state is no longer representable.
— Claude Code
| modifierFlags: [], | ||
| timestamp: ProcessInfo.processInfo.systemUptime, | ||
| windowNumber: window.windowNumber, | ||
| context: nil, | ||
| eventNumber: 1, | ||
| clickCount: 1, | ||
| pressure: 1 | ||
| )) | ||
| let hitView = try XCTUnwrap(host.hitTest(targetRowPoint)) |
There was a problem hiding this comment.
No click event is ever dispatched into the AppKit event system in this test — the NSApp.sendEvent path is inside an if hitView.acceptsFirstMouse(for: downEvent) block that the test has already asserted to be false. The final XCTAssertEqual(tabManager.selectedTabId, initialWorkspace.id) assertions therefore just verify the initial setup, not that an accepted click would have been blocked from switching workspaces. The test name promises end-to-end "does not switch workspace" coverage that isn't delivered; a future regression that lets clicks through would still pass all four assertions here.
There was a problem hiding this comment.
Fixed by expanding the sidebar coverage to prove both branches. The enabled-path test dispatches an accepted inactive first click and verifies the workspace switches; the disabled-path test verifies the click is rejected and the selection stays on the original workspace.
— Claude Code
28d07d9 to
9a438cb
Compare
| XCTAssertTrue(try dispatchInactiveFirstClick(in: harness)) | ||
| XCTAssertEqual(harness.tabManager.selectedTabId, harness.targetWorkspace.id) |
There was a problem hiding this comment.
dispatchInactiveFirstClick gates on hitView.acceptsFirstMouse(for: downEvent) before dispatching any event (line 201). When the setting is enabled, FirstMouseGatedPassThroughHostingView.hitTest returns nil and the hit-test descends into SwiftUI's internal NSView hierarchy. SwiftUI-generated views for list rows and buttons do not override acceptsFirstMouse, so it returns the NSView default of false. The guard short-circuits, events are never sent, and dispatchInactiveFirstClick returns false. XCTAssertTrue therefore fails and line 227's workspace-switch assertion is never reached.
The harness correctly proves the disabled path (overlay blocks the click). To prove the enabled path, the test needs a way to deliver the event that doesn't depend on acceptsFirstMouse; for example, calling window.makeKeyAndOrderFront(nil) first (making the window key) to simulate an already-active window, or driving the click directly through NSApp.sendEvent at the window level bypassing the acceptsFirstMouse gate.
There was a problem hiding this comment.
Fixed by changing the enabled/control path into an active-window row click. That validates the discovered row target without depending on SwiftUI internal views accepting first mouse; the inactive disabled test remains focused on the first-mouse gate.
— Claude Code
| window: window, | ||
| keyWindow: keyWindow, | ||
| host: host, | ||
| targetRowPoint: NSPoint(x: 48, y: frame.height - 88) |
There was a problem hiding this comment.
NSPoint(x: 48, y: frame.height - 88) = (48, 272) assumes the second workspace row lands at exactly that Y position after layout. displayIfNeeded() + layoutSubtreeIfNeeded() on a bare NSWindow without a real screen do not guarantee that SwiftUI's layout pass completes to the pixel-perfect position expected here. If the layout doesn't settle, host.hitTest returns the wrong view (or nil), making try XCTUnwrap fail in the enabled case or silently hit the wrong row in both cases.
There was a problem hiding this comment.
Fixed by removing the hardcoded pixel coordinate. The harness now finds the target workspace row through its sidebarWorkspace. accessibility identifier and uses the element's accessibility frame center.
— Claude Code
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 9a438cb. Configure here.
1db4c1f to
42fe0c0
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
42fe0c0 to
4161427
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmuxTests/InactivePaneFirstClickFocusTests.swift`:
- Around line 136-200: The test mounts VerticalTabsSidebar directly into an
NSHostingView in makeSidebarFirstClickHarness, bypassing the production
first-mouse gate/overlay; replace the raw NSHostingView usage with the
production first-mouse gated hosting wrapper (i.e., instantiate the
FirstMouseGatedHostingView or the app's equivalent gated hosting helper with
rootView: sidebar) so the test exercises the same overlay boundary as production
and verifies first-click behavior for VerticalTabsSidebar.
- Around line 283-300: The test currently returns early in
dispatchInactiveFirstClick when hitView.acceptsFirstMouse(for: downEvent) is
false, so it never exercises the inactive-click activation path; change
dispatchInactiveFirstClick (and the duplicate at the other range) to always
drive the inactive-click sequence: if acceptsFirstMouse is false, still send the
click event (use sendClick(at:in:) with the constructed downEvent/point) so the
window activation path runs, then assert the window became key
(window.isKeyWindow or appropriate API) while verifying selectedTabId remains
the same; keep the existing behavior when acceptsFirstMouse is true (send click
and return true), but ensure the test asserts window activation and unchanged
selectedTabId after the click in both branches.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 59e128c1-80a7-4c2a-b9be-69adb05a20e2
📒 Files selected for processing (6)
Sources/App/CmuxMainWindow.swiftSources/ContentView.swiftSources/Panels/FilePreviewPanel.swiftSources/Update/MinimalModeSidebarControls.swiftcmuxTests/InactivePaneFirstClickFocusTests.swiftcmuxTests/WindowAndDragTests.swift
4161427 to
1f4566c
Compare
1f4566c to
89be965
Compare
89be965 to
0826e48
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmuxTests/InactivePaneFirstClickFocusTests.swift`:
- Around line 329-348: Add a new test that verifies the enabled-path for
inactive-window first-click on the sidebar: create a test (e.g.,
testWorkspaceSidebarInactiveFirstClickSwitchesWorkspaceWhenSettingEnabled) that
sets UserDefaults.standard.set(true, forKey: settingsKey), creates the harness
via makeSidebarFirstClickHarness(), calls dispatchInactiveFirstClick(in:
harness), and asserts that result.acceptedFirstMouse is true, the
hitViewClassName does not contain "FirstMouseGated", and
harness.tabManager.selectedTabId equals harness.targetWorkspace.id (and is not
the initialWorkspace.id) to ensure the enabled behavior actually switches
workspaces.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f6008581-8494-435b-8082-83b197236a11
📒 Files selected for processing (6)
Sources/App/CmuxMainWindow.swiftSources/ContentView.swiftSources/Panels/FilePreviewPanel.swiftSources/Update/MinimalModeSidebarControls.swiftcmuxTests/InactivePaneFirstClickFocusTests.swiftcmuxTests/WindowAndDragTests.swift
Add failing behavior coverage for the first-click focus policy where chrome and SwiftUI sidebar surfaces bypass PaneFirstClickFocusSettings. The tests mirror the existing pane body assertions and exercise the workspace sidebar through a hosted runtime path instead of checking source text. The sidebar regression discovers the target row through its accessibility identifier, proves the coordinate with an active-window click, then verifies that an inactive first click with focusPaneOnFirstClick disabled is rejected before the workspace selection changes. Constraint: Do not run local tests; CI owns unit and UI verification for this repo. Rejected: Source-shape assertions for hardcoded acceptsFirstMouse | project policy requires runtime behavior tests. Confidence: medium Scope-risk: narrow Tested: git diff --check Not-tested: Local XCTest execution, by repo and user instruction.
Route minimal-mode controls, PDF chrome, and the SwiftUI workspace sidebar through PaneFirstClickFocusSettings. The sidebar gets a single AppKit hosting gate that captures inactive first clicks when the setting is off and otherwise passes through to normal SwiftUI hit testing. The pass-through overlay is a dedicated subclass instead of mutable configuration, so the default hosting gate cannot be left in a bad pass-through state. The gate converts hit-test points from the superview into local bounds before deciding whether to capture. Constraint: focusPaneOnFirstClick is the existing source of truth for pane first-click behavior. Rejected: Guard each workspace row action | duplicates the policy across SwiftUI action sites and misses future sidebar controls. Confidence: medium Scope-risk: moderate Directive: Keep first-mouse policy at AppKit boundaries; do not add per-row workarounds for inactive-window activation. Tested: git diff --check; source scan for acceptsFirstMouse overrides; file length check for touched budgeted files. Not-tested: Local XCTest/build execution, by repo and user instruction; CI will run on the PR.
0826e48 to
9cf33d8
Compare
Stale CodeRabbit change request on an older commit. Requested enabled-path regression was added in the current tests-only commit and the thread is resolved.
This branch was successfully deployed
1 participant
Fixes #3856.
This PR keeps PaneFirstClickFocusSettings as the single source of truth for inactive first-click behavior. It adds failing regression coverage first, then gates the minimal-mode sidebar controls, PDF chrome hosting view, and SwiftUI workspace sidebar first-mouse boundary so inactive first clicks only activate cmux when app.focusPaneOnFirstClick is false.
Local tests/build were not run per repo and task instructions; CI is the verification path before the required tagged reload.
Note
Medium Risk
Changes macOS first-mouse hit-testing/acceptance for several UI surfaces, which can subtly affect click/focus behavior across the app. Adds broad UI-event-driven test coverage, reducing risk but still sensitive to edge cases in AppKit/SwiftUI integration.
Overview
Ensures inactive-window first clicks consistently respect
PaneFirstClickFocusSettings(akafocusPaneOnFirstClick) across minimal-mode titlebar controls, PDF preview chrome, and the SwiftUI workspace sidebar.Adds a SwiftUI/AppKit boundary “first-mouse gate” overlay (
FirstMouseGatedHostingOverlay) on the sidebar to capture the initial click when the window is inactive and the setting is disabled, preventing accidental workspace switches.Updates/expands regression tests to cover these behaviors, including an integration-style sidebar harness that locates rows via accessibility IDs and dispatches real mouse events; also adjusts existing PDF chrome tests to set the setting explicitly.
Reviewed by Cursor Bugbot for commit 9cf33d8. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Fixes #3856 by honoring the inactive first‑click policy across the workspace sidebar, minimal‑mode controls, and PDF preview chrome. When
PaneFirstClickFocusSettingsis off, the first click on an inactive window only activates the app and does not switch panes or workspaces.FirstMouseGatedHostingView,FirstMouseGatedPassThroughHostingView, andFirstMouseGatedHostingOverlay; overlay (accessibility‑hidden) is applied toVerticalTabsSidebarto capture inactive first clicks at the SwiftUI↔AppKit boundary.acceptsFirstMouseinMinimalModeSidebarControlActionViewandFilePreviewPDFChromeHostingViewwithPaneFirstClickFocusSettings.isEnabled().FirstMouseGated*view when the setting is off, and updated PDF chrome tests to respect the setting.Written for commit 9cf33d8. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Tests