Fix minimal mode workspace and pane tab clicks#2650
Fix minimal mode workspace and pane tab clicks#2650austinywang wants to merge 7 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
This review could not be run because your cubic account has exceeded the monthly review limit. If you need help restoring access, please contact contact@cubic.dev. |
📝 WalkthroughWalkthroughChanges extend portal host view hit-testing to pass through pointer events to Bonsplit pane tab bars and window titlebar, preventing the portal from claiming those interactions. Adds tab-bar background detection helpers with view-hierarchy traversal. Includes comprehensive test coverage for the new detection logic. Updates vendored Bonsplit dependency. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 workspace tab interactions in minimal mode on Intel by overriding Confidence Score: 5/5Safe to merge — minimal one-line fix consistent with established codebase patterns; existing drag infrastructure is unaffected The change is a single property override matching the existing pattern in NonDraggableHostingView, TitlebarLeadingInsetPassthroughView, DraggableFolderNSView, and others. Window dragging is already handled exclusively through WindowDragHandleView with explicit performDrag calls, and the window is created with isMovable = false and isMovableByWindowBackground = false. No P1 or P0 findings. No files require special attention Important Files Changed
Sequence DiagramsequenceDiagram
participant U as User (Intel)
participant AK as AppKit
participant MWHV as MainWindowHostingView
participant DV as DraggableView (WindowDragHandle)
participant SUI as SwiftUI WorkspaceTab
Note over MWHV: mouseDownCanMoveWindow = false (new)
U->>AK: leftMouseDown on titlebar band
AK->>MWHV: mouseDownCanMoveWindow?
MWHV-->>AK: false — skip window drag
AK->>SUI: event propagates to SwiftUI
SUI-->>U: tab hover/click/right-click works
Note over DV: Explicit drag path (unchanged)
U->>AK: leftMouseDown on empty titlebar
AK->>DV: hitTest → captures hit
DV->>AK: window.performDrag(with: event)
AK-->>U: window drag works
Reviews (1): Last reviewed commit: "Fix minimal mode workspace tab clicks" | Re-trigger Greptile |
…-2633-minimal-mode-tab-switching
Pulls in manaflow-ai/bonsplit#91, which adds NonDraggableHostingView/NonDraggableHostingController and overrides mouseDownCanMoveWindow=false on the pane container chain so AppKit no longer treats split-pane tab clicks as window-drag intents in minimal mode (where the window has no titlebar drag region). The cmux app already overrides mouseDownCanMoveWindow=false on its top-level MainWindowHostingView, but Bonsplit's split layout creates its own nested NSHostingController instances that were never covered by that override — making split-pane tabs completely unclickable in minimal mode while terminal surfaces (which use a separate portal hosting path) kept working.
Pulls in manaflow-ai/bonsplit#92 which makes ThemedSplitView refuse window-drag promotion. Fixes left pane tab clicks in horizontal splits when minimal mode (no titlebar drag region) caused AppKit to consume the mouseUp before SwiftUI's tap gesture fired. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![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 (2)
Sources/BrowserWindowPortal.swift (1)
1257-1323: Extract the Bonsplit background scan before these copies drift.This recursive
TabBarBackgroundNSViewdetection is now effectively duplicated inSources/TerminalWindowPortal.swift. A shared helper would make future hit-test fixes much less likely to land in only one portal.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/BrowserWindowPortal.swift` around lines 1257 - 1323, The duplicated recursive detection logic for TabBarBackgroundNSView (functions hasBonsplitTabBarBackground and hasUnderlyingBonsplitTabBarBackground, plus their DEBUG testing wrappers hasBonsplitTabBarBackgroundForTesting and hasUnderlyingBonsplitTabBarBackgroundForTesting) should be extracted into a shared helper (e.g., a new PortalHitTest or WindowPortalUtils type) and the copies in Sources/BrowserWindowPortal.swift and Sources/TerminalWindowPortal.swift replaced to call that shared API; move the recursive implementation and any DEBUG-accessible wrappers into the new helper, keep the same function signatures (or provide thin forwarding functions) so callers (BrowserWindowPortal and TerminalWindowPortal) only delegate to the shared hasBonsplitTabBarBackground(...) and hasUnderlyingBonsplitTabBarBackground(...) helpers to avoid drift.scripts/build-intel-debug-artifact.sh (1)
370-397: Embedded metadata lacks tarball SHA (intentional but worth noting).The metadata embedded in the app bundle (line 391) is written before the tarball exists, so
tarballSHA256is empty. The external metadata file (line 424) gets the actual SHA. This is fine for debugging provenance, but if someone extracts and inspects the embeddedcmux-dev-build-metadata.json, the missing SHA could be confusing.Consider adding a comment near line 391 noting this is intentional, or using a placeholder like
"pending"instead of empty string to make the intent clearer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/build-intel-debug-artifact.sh` around lines 370 - 397, The embedded metadata currently writes an empty tarball SHA (the last argument to write_metadata_json is ""), which is confusing; update the call to write_metadata_json (symbol: write_metadata_json, METADATA_PATH, PACKAGED_APP_PATH, cmux-dev-build-metadata.json) to pass a clear placeholder like "pending" instead of "" for the tarball SHA, and/or add a one-line comment above the call explaining that the real tarball SHA is computed later and the embedded metadata intentionally contains a placeholder.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/build-ghostty-cli-helper.sh`:
- Around line 110-112: The case pattern using v[0-9]*.[0-9]*.[0-9]* allows
suffixes like -rc1; replace the glob-based check with a strict regex match
against exact_tag to require MAJOR.MINOR.PATCH only. Concretely, in the block
that currently uses case "$exact_tag" and the v[0-9]*.[0-9]*.[0-9]*) branch,
change to an if [[ "$exact_tag" =~ ^v[0-9]+\.[0-9]+\.[0-9]+$ ]] && return 0 (or
equivalent) so only pure release tags like v1.2.3 pass and tags with suffixes
(e.g., -rc1) do not.
In `@Sources/BrowserWindowPortal.swift`:
- Around line 701-728: The switch in shouldPassThroughToPaneTabBar currently
only allows mouse down/up types so hover and pointer updates are blocked; update
the eventType whitelist in shouldPassThroughToPaneTabBar to also include
.mouseMoved, .mouseEntered, .mouseExited, and .cursorUpdate (and optionally the
dragged variants if desired) so move/enter/exit/cursor updates are treated like
clicks and can pass through to the pane tab bar; keep the rest of the logic
(windowPoint conversion, BonsplitTabBarHitRegionRegistry check and debug
logging) unchanged.
In `@Sources/TerminalWindowPortal.swift`:
- Around line 223-250: The method shouldPassThroughToPaneTabBar currently only
allows mouse down/up events to pass through, which still swallows hover and
cursor events; update the switch on eventType in shouldPassThroughToPaneTabBar
to also allow hover-related event types (at minimum .mouseMoved, .mouseEntered,
.mouseExited, and .cursorUpdate, and optionally
.leftMouseDragged/.rightMouseDragged if relevant) to fall through to the
passthrough logic so registry checks
(BonsplitTabBarHitRegionRegistry.containsWindowPoint) and
Self.hasUnderlyingBonsplitTabBarBackground are used for those events too; keep
the existing registryHit/result computation and debug logging unchanged.
In `@vendor/bonsplit`:
- Line 1: The parent repo's submodule pointer in vendor/bonsplit references
commit 8316cbef which is not present on the remote main branch; open the
vendor/bonsplit repository, ensure commit 8316cbef is present locally, push that
commit to origin/main (or rebase/merge it onto origin/main) so origin/main
contains 8316cbef, then return to the parent repo and update the submodule
pointer (in vendor/bonsplit) to the now-pushed commit and commit that change to
the parent repository.
---
Nitpick comments:
In `@scripts/build-intel-debug-artifact.sh`:
- Around line 370-397: The embedded metadata currently writes an empty tarball
SHA (the last argument to write_metadata_json is ""), which is confusing; update
the call to write_metadata_json (symbol: write_metadata_json, METADATA_PATH,
PACKAGED_APP_PATH, cmux-dev-build-metadata.json) to pass a clear placeholder
like "pending" instead of "" for the tarball SHA, and/or add a one-line comment
above the call explaining that the real tarball SHA is computed later and the
embedded metadata intentionally contains a placeholder.
In `@Sources/BrowserWindowPortal.swift`:
- Around line 1257-1323: The duplicated recursive detection logic for
TabBarBackgroundNSView (functions hasBonsplitTabBarBackground and
hasUnderlyingBonsplitTabBarBackground, plus their DEBUG testing wrappers
hasBonsplitTabBarBackgroundForTesting and
hasUnderlyingBonsplitTabBarBackgroundForTesting) should be extracted into a
shared helper (e.g., a new PortalHitTest or WindowPortalUtils type) and the
copies in Sources/BrowserWindowPortal.swift and
Sources/TerminalWindowPortal.swift replaced to call that shared API; move the
recursive implementation and any DEBUG-accessible wrappers into the new helper,
keep the same function signatures (or provide thin forwarding functions) so
callers (BrowserWindowPortal and TerminalWindowPortal) only delegate to the
shared hasBonsplitTabBarBackground(...) and
hasUnderlyingBonsplitTabBarBackground(...) helpers to avoid drift.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 19ddf4a1-e380-4ead-b7e2-e6b9d7fb7177
📒 Files selected for processing (10)
Sources/AppDelegate.swiftSources/BrowserWindowPortal.swiftSources/TerminalWindowPortal.swiftcmuxTests/BrowserPanelTests.swiftcmuxTests/TerminalAndGhosttyTests.swiftscripts/build-ghostty-cli-helper.shscripts/build-intel-debug-artifact.shscripts/clang-xcodebuild-wrapper.shscripts/reload.shvendor/bonsplit
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/AppDelegate.swift
| case "$exact_tag" in | ||
| tip|v[0-9]*.[0-9]*.[0-9]*) | ||
| return 0 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for tag in tip v1.2.3 v1.2.3-rc1 v1.2.3+meta xcframework-deadbeef; do
case "$tag" in
tip|v[0-9]*.[0-9]*.[0-9]*) old="match" ;;
*) old="no-match" ;;
esac
if [[ "$tag" == "tip" || "$tag" =~ ^v[0-9]+\.[0-9]+\.[0-9]+$ ]]; then
new="match"
else
new="no-match"
fi
printf "%-22s old=%-8s new=%s\n" "$tag" "$old" "$new"
doneRepository: manaflow-ai/cmux
Length of output: 298
🏁 Script executed:
cd manaflow-ai/cmux && sed -n '100,125p' scripts/build-ghostty-cli-helper.shRepository: manaflow-ai/cmux
Length of output: 126
🏁 Script executed:
fd -t f "build-ghostty-cli-helper.sh" 2>/dev/null | head -5Repository: manaflow-ai/cmux
Length of output: 95
🏁 Script executed:
sed -n '103,125p' scripts/build-ghostty-cli-helper.shRepository: manaflow-ai/cmux
Length of output: 844
Tighten release-tag matching to avoid false positives.
On Line 111, v[0-9]*.[0-9]*.[0-9]* will also match non-release tags like v1.2.3-rc1, so the override path is skipped when it shouldn't be.
Proposed fix
- case "$exact_tag" in
- tip|v[0-9]*.[0-9]*.[0-9]*)
- return 0
- ;;
- esac
+ if [[ "$exact_tag" == "tip" ]]; then
+ return 0
+ fi
+
+ if [[ "$exact_tag" =~ ^v[0-9]+\.[0-9]+\.[0-9]+$ ]]; then
+ return 0
+ fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/build-ghostty-cli-helper.sh` around lines 110 - 112, The case pattern
using v[0-9]*.[0-9]*.[0-9]* allows suffixes like -rc1; replace the glob-based
check with a strict regex match against exact_tag to require MAJOR.MINOR.PATCH
only. Concretely, in the block that currently uses case "$exact_tag" and the
v[0-9]*.[0-9]*.[0-9]*) branch, change to an if [[ "$exact_tag" =~
^v[0-9]+\.[0-9]+\.[0-9]+$ ]] && return 0 (or equivalent) so only pure release
tags like v1.2.3 pass and tags with suffixes (e.g., -rc1) do not.
| private func shouldPassThroughToPaneTabBar( | ||
| at point: NSPoint, | ||
| eventType: NSEvent.EventType? | ||
| ) -> Bool { | ||
| switch eventType { | ||
| case .leftMouseDown, .leftMouseUp, | ||
| .rightMouseDown, .rightMouseUp, | ||
| .otherMouseDown, .otherMouseUp: | ||
| break | ||
| default: | ||
| return false | ||
| } | ||
|
|
||
| let windowPoint = convert(point, to: nil) | ||
| let registryHit = window.map { | ||
| BonsplitTabBarHitRegionRegistry.containsWindowPoint(windowPoint, in: $0) | ||
| } ?? false | ||
| let result = registryHit || Self.hasUnderlyingBonsplitTabBarBackground(at: windowPoint, below: self) | ||
| #if DEBUG | ||
| if eventType == .leftMouseDown { | ||
| dlog( | ||
| "portal.brwsr.passThroughTabBar wp=\(Int(windowPoint.x)),\(Int(windowPoint.y)) " + | ||
| "registry=\(registryHit ? 1 : 0) result=\(result ? 1 : 0)" | ||
| ) | ||
| } | ||
| #endif | ||
| return result | ||
| } |
There was a problem hiding this comment.
Hover still won’t reach pane tabs.
Because Line 705 only whitelists mouse down/up events, split-pane and left-pane tab bars under the browser portal still won’t receive move/enter/exit/cursor updates. Click/right-click will work again, but hover highlight and pointer-state changes stay intercepted by the host.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/BrowserWindowPortal.swift` around lines 701 - 728, The switch in
shouldPassThroughToPaneTabBar currently only allows mouse down/up types so hover
and pointer updates are blocked; update the eventType whitelist in
shouldPassThroughToPaneTabBar to also include .mouseMoved, .mouseEntered,
.mouseExited, and .cursorUpdate (and optionally the dragged variants if desired)
so move/enter/exit/cursor updates are treated like clicks and can pass through
to the pane tab bar; keep the rest of the logic (windowPoint conversion,
BonsplitTabBarHitRegionRegistry check and debug logging) unchanged.
| private func shouldPassThroughToPaneTabBar( | ||
| at point: NSPoint, | ||
| eventType: NSEvent.EventType? | ||
| ) -> Bool { | ||
| switch eventType { | ||
| case .leftMouseDown, .leftMouseUp, | ||
| .rightMouseDown, .rightMouseUp, | ||
| .otherMouseDown, .otherMouseUp: | ||
| break | ||
| default: | ||
| return false | ||
| } | ||
|
|
||
| let windowPoint = convert(point, to: nil) | ||
| let registryHit = window.map { | ||
| BonsplitTabBarHitRegionRegistry.containsWindowPoint(windowPoint, in: $0) | ||
| } ?? false | ||
| let result = registryHit || Self.hasUnderlyingBonsplitTabBarBackground(at: windowPoint, below: self) | ||
| #if DEBUG | ||
| if eventType == .leftMouseDown { | ||
| dlog( | ||
| "portal.term.passThroughTabBar wp=\(Int(windowPoint.x)),\(Int(windowPoint.y)) " + | ||
| "registry=\(registryHit ? 1 : 0) result=\(result ? 1 : 0)" | ||
| ) | ||
| } | ||
| #endif | ||
| return result | ||
| } |
There was a problem hiding this comment.
Pane-tab hover is still blocked here.
Line 227 only passes mouse down/up through to the underlying Bonsplit tab bar. That restores click/right-click, but hover/enter/exit/cursor updates for split-pane and left-pane tabs will still get swallowed by the terminal portal host.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/TerminalWindowPortal.swift` around lines 223 - 250, The method
shouldPassThroughToPaneTabBar currently only allows mouse down/up events to pass
through, which still swallows hover and cursor events; update the switch on
eventType in shouldPassThroughToPaneTabBar to also allow hover-related event
types (at minimum .mouseMoved, .mouseEntered, .mouseExited, and .cursorUpdate,
and optionally .leftMouseDragged/.rightMouseDragged if relevant) to fall through
to the passthrough logic so registry checks
(BonsplitTabBarHitRegionRegistry.containsWindowPoint) and
Self.hasUnderlyingBonsplitTabBarBackground are used for those events too; keep
the existing registryHit/result computation and debug logging unchanged.
| @@ -1 +1 @@ | |||
| Subproject commit 098d9fa00e2b1d4712f1a46b818ee7d53d4aa31f | |||
| Subproject commit 8316cbef7d66f3a98c6051241861abef04f95466 | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify bonsplit submodule commit is ancestor of origin/main
cd vendor/bonsplit
git fetch origin main
if git merge-base --is-ancestor HEAD origin/main; then
echo "✅ Submodule commit is on origin/main"
else
echo "❌ Submodule commit is NOT on origin/main — push before merging parent PR"
exit 1
fiRepository: manaflow-ai/cmux
Length of output: 238
Submodule commit must be pushed to origin/main before merging.
The submodule pointer targets commit 8316cbef, but this commit has not been pushed to the remote main branch of vendor/bonsplit. Per project guidelines, submodule commits must be pushed to their remote main branch before updating the parent repository pointer.
Push the submodule commit to origin/main in the vendor/bonsplit repository, then update the parent pointer.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vendor/bonsplit` at line 1, The parent repo's submodule pointer in
vendor/bonsplit references commit 8316cbef which is not present on the remote
main branch; open the vendor/bonsplit repository, ensure commit 8316cbef is
present locally, push that commit to origin/main (or rebase/merge it onto
origin/main) so origin/main contains 8316cbef, then return to the parent repo
and update the submodule pointer (in vendor/bonsplit) to the now-pushed commit
and commit that change to the parent repository.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 10 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="scripts/build-ghostty-cli-helper.sh">
<violation number="1" location="scripts/build-ghostty-cli-helper.sh:110">
P2: The release-tag matcher is too permissive and can misclassify non-semver tags as valid release tags, skipping the version override and potentially reintroducing build panics.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![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 e2e96fe. Configure here.
| ) -> Bool { | ||
| hasUnderlyingBonsplitTabBarBackground(at: windowPoint, below: portalHost) | ||
| } | ||
| #endif |
There was a problem hiding this comment.
Duplicated tab bar detection logic across portal classes
Low Severity
hasBonsplitTabBarBackground, hasUnderlyingBonsplitTabBarBackground, shouldPassThroughToPaneTabBar, and their testing wrappers are copy-pasted across WindowTerminalHostView and WindowBrowserHostView with identical logic (only debug log prefixes differ). A bug fix applied to one will likely be missed in the other. Since these are static methods operating on plain NSView hierarchies with no dependency on the host class, they could live in a shared extension or utility.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit e2e96fe. Configure here.
…-2633-minimal-mode-tab-switching # Conflicts: # scripts/reload.sh
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/AppDelegate.swift`:
- Around line 10330-10334: The click hit-testing is using event.locationInWindow
directly which is in window coordinates; update the clickInspectMonitor handler
(the closure assigned to clickInspectMonitor) to convert the point into the
contentView's coordinate system before calling hitTest: obtain
window.contentView, call its convert(_:from:) with event.locationInWindow and
from: nil (or use convert(event.locationInWindow, from: window) pattern used
elsewhere), then pass that converted point to hitView.hitTest(_: ) (and to any
subsequent hit-testing/logging) so the hit chain is computed in the view's own
coordinate space.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: b9663601-0fdf-416f-837a-d45e49f8798e
📒 Files selected for processing (2)
Sources/AppDelegate.swiftcmuxTests/TerminalAndGhosttyTests.swift
✅ Files skipped from review due to trivial changes (1)
- cmuxTests/TerminalAndGhosttyTests.swift
| clickInspectMonitor = NSEvent.addLocalMonitorForEvents(matching: [.leftMouseDown]) { event in | ||
| guard let window = event.window else { return event } | ||
| let windowPoint = event.locationInWindow | ||
| let hitView = window.contentView?.hitTest(windowPoint) | ||
| let hitClass = hitView.map { String(describing: type(of: $0)) } ?? "nil" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and read the code at the specified lines
sed -n '10325,10340p' Sources/AppDelegate.swiftRepository: manaflow-ai/cmux
Length of output: 828
🏁 Script executed:
# Also check the context around this function
sed -n '10320,10370p' Sources/AppDelegate.swift | head -60Repository: manaflow-ai/cmux
Length of output: 2219
🏁 Script executed:
# Search for other hitTest usage in the codebase to see common patterns
rg 'hitTest\(' Sources/ -A 2 -B 2Repository: manaflow-ai/cmux
Length of output: 16222
🌐 Web query:
NSView hitTest coordinate system AppKit macOS
💡 Result:
In AppKit on macOS, the NSView hitTest(:) method takes an NSPoint parameter that is expressed in the coordinate system of the view's superview, not the receiver's own coordinate system. This method finds the deepest descendant view (including itself) containing that point or returns nil if outside. It is typically invoked by NSWindow for mouse event routing and ignores hidden views. When overriding, convert points between coordinate systems using methods like convert(:from:) if needed.
Citations:
- 1: https://developer.apple.com/documentation/appkit/nsview/hittest(_:)
- 2: https://developer.apple.com/documentation/appkit/nsview/hittest(_:)?language=objc
- 3: https://developer.apple.com/documentation/appkit/nsview/hittest(_:)?changes=latest_m_5_4&language=objc
Fix coordinate-space mismatch in click hit-testing.
Line 10333 passes event.locationInWindow directly to hitTest(_:), but the method expects coordinates in the view's own coordinate system. All other hitTest calls in the codebase (AppDelegate, ContentView, BrowserPanelView, etc.) convert the point first using view.convert(event.locationInWindow, from: nil). Without conversion, the hit chain logged will be incorrect.
Suggested fix
private func installClickInspectMonitor() {
guard clickInspectMonitor == nil else { return }
clickInspectMonitor = NSEvent.addLocalMonitorForEvents(matching: [.leftMouseDown]) { event in
guard let window = event.window else { return event }
let windowPoint = event.locationInWindow
- let hitView = window.contentView?.hitTest(windowPoint)
+ let hitView = window.contentView.flatMap { contentView in
+ let contentPoint = contentView.convert(windowPoint, from: nil)
+ return contentView.hitTest(contentPoint)
+ }
let hitClass = hitView.map { String(describing: type(of: $0)) } ?? "nil"
var ancestorClasses: [String] = []
var current: NSView? = hitView🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/AppDelegate.swift` around lines 10330 - 10334, The click hit-testing
is using event.locationInWindow directly which is in window coordinates; update
the clickInspectMonitor handler (the closure assigned to clickInspectMonitor) to
convert the point into the contentView's coordinate system before calling
hitTest: obtain window.contentView, call its convert(_:from:) with
event.locationInWindow and from: nil (or use convert(event.locationInWindow,
from: window) pattern used elsewhere), then pass that converted point to
hitView.hitTest(_: ) (and to any subsequent hit-testing/logging) so the hit
chain is computed in the view's own coordinate space.
austinywang
changed the title
There was a problem hiding this comment.
2 issues found across 5 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="scripts/reload.sh">
<violation number="1">
P2: `xcodebuild` is now invoked without the clang wrapper env, removing the compiler-probe workaround path.</violation>
<violation number="2">
P1: Post-build copying of `ghostty/zig-out/bin/ghostty` can overwrite the Xcode-produced helper with a stale or wrong-arch binary.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
1 participant
Summary
vendor/bonsplitto the merged support inmanafow-ai/bonsplit#97Fixes #2633
Root Cause
Minimal mode renders both workspace and pane tabs into the titlebar band. On the affected setup, those clicks could be claimed before the tab views saw them: either by AppKit window-drag behavior or by the browser/terminal portal hosts that sit above the SwiftUI and AppKit tab chrome.
There was also a pane-tab-specific failure mode. In minimal mode, the visible tab strip can live outside the immediate sibling container bounds that the portal hosts would normally reason about, so pointer hits in that area were not being recognized as Bonsplit tab-bar hits.
Implementation
MainWindowHostingView.mouseDownCanMoveWindow = falseso titlebar-band SwiftUI tab chrome remains clickable in minimal modeWindowBrowserHostViewandWindowTerminalHostViewfor titlebar-band hits plus Bonsplit tab-bar registry/background detectionvendor/bonsplitto the merged tab-hit support frommanafow-ai/bonsplit#97Validation
./scripts/reload.sh --tag issue-2633-pr-cleanupNote
Medium Risk
Touches low-level AppKit hit-testing and window-drag behavior; mistakes could break click/drag routing or cursor behavior across the window chrome and portals.
Overview
Fixes minimal-mode click routing so workspace/pane tab interactions aren’t intercepted by window dragging or portal host views.
MainWindowHostingViewis made non-draggable (mouseDownCanMoveWindow = false), and bothWindowBrowserHostViewandWindowTerminalHostViewnow pass mouse down/up events through to underlying Bonsplit tab-bar regions (viaBonsplitTabBarHitRegionRegistryplus a fallback scan forTabBarBackgroundNSView, including sibling trees used when tabs render into the titlebar band). New unit tests cover tab-bar background hit detection and sibling-tree edge cases.Reviewed by Cursor Bugbot for commit 6c13568. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
Bug Fixes
Tests