Repository navigation
Fix minimal-mode top-bar drag pass-through - #3121
austinywang wants to merge 2 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughEnhances hit-testing in browser and terminal window portals to defer pointer events to underlying tab bars and titlebar regions in minimal mode. Adds shared pass-through logic via Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 routes pointer hit-testing for the browser and terminal portal hosts through the underlying Bonsplit tab strip in minimal mode, restoring window-drag behaviour in the empty top bar. The shared
Confidence Score: 4/5Production logic is correct; both regression tests are structurally broken and will fail when run. The hit-test routing changes in the two portal source files look correct and well-structured. Score is 4 rather than 5 because both new tests have a definite failure mode (nil event type bypasses all tab-bar pass-through logic). The fragile class-name fallback is P2 only. cmuxTests/BrowserPanelTests.swift and cmuxTests/TerminalAndGhosttyTests.swift — both new tests need a synthesized NSEvent before calling hitTest. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["hitTest(point) called"] --> B{Is pointer event?}
B -- No --> Z["super.hitTest → return child or nil"]
B -- Yes --> C{In titlebar band?}
C -- Yes --> D["return nil (window drag)"]
C -- No --> E{BonsplitTabBarPassThrough shouldPassThrough?}
E -- "Registry hit (primary)" --> F["return nil (tab strip)"]
E -- "Class-name walk (fallback)" --> F
E -- No --> G{In sidebar resizer?}
G -- Yes --> H["return nil (resizer)"]
G -- No --> I{Split divider?}
I -- Yes --> J["return nil (divider)"]
I -- No --> K["return portal child view"]
Reviews (1): Last reviewed commit: "Pass portal hits through minimal tab str..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| XCTAssertNil( | ||
| host.hitTest(pointInHost), | ||
| "Browser portal should defer to the minimal tab strip even just below the native titlebar interaction band" | ||
| ) |
There was a problem hiding this comment.
passThroughDecision(at:in:eventType:) starts with guard isPassThroughPointerEvent(eventType) else { return nil }. When the test calls host.hitTest(pointInHost) directly — with no synthesized NSEvent — NSApp.currentEvent is nil, so isPassThroughPointerEvent(nil) falls to the default branch and returns false. passThroughDecision returns nil, shouldPassThroughToPaneTabBar returns false, the child CapturingView is hit, and XCTAssertNil fails.
The same pattern breaks the mirror test in TerminalAndGhosttyTests.swift. Existing divider and titlebar pass-through tests succeed because those guards are geometry-only and don't consult NSApp.currentEvent. To fix, synthesize a .leftMouseDown event before calling hitTest.
| let pointInHost = host.convert(pointInWindow, from: nil) | ||
|
|
||
| XCTAssertNil( | ||
| host.hitTest(pointInHost), | ||
| "Terminal portal should defer to the minimal tab strip even just below the native titlebar interaction band" |
There was a problem hiding this comment.
shouldPassThroughToPaneTabBar is only reachable inside the if isPointerEvent { } block. With no current event (NSApp.currentEvent == nil), isPointerEvent is false, the entire block is skipped, super.hitTest returns the child CapturingView, and XCTAssertNil fails. A synthesized .leftMouseDown event is needed (same fix as the browser portal test).
| } | ||
|
|
||
| for subview in view.subviews.reversed() { | ||
| if hasBonsplitTabBarBackground(at: windowPoint, in: subview) { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } | ||
|
|
||
| static func hasUnderlyingBonsplitTabBarBackground( | ||
| at windowPoint: NSPoint, | ||
| below portalHost: NSView | ||
| ) -> Bool { | ||
| if let container = portalHost.superview, |
There was a problem hiding this comment.
hasBonsplitTabBarBackground identifies the tab strip by checking NSStringFromClass(type(of: view)).contains("TabBarBackgroundNSView"). This is a string match against an internal Bonsplit class name with no public API contract. If Bonsplit renames that view, the fallback silently stops working with no compile-time signal. The primary BonsplitTabBarHitRegionRegistry path is robust; consider whether the fallback is worth keeping, or add a comment explaining the known fragility.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/TerminalWindowPortal.swift (1)
3-5:⚠️ Potential issue | 🔴 CriticalRemove
#if DEBUGguards fromimport Bonsplit.Lines 215 and 222 reference
BonsplitTabBarPassThroughin production code, but the import is currently wrapped in#if DEBUG. Release builds will fail to compile.Proposed fix
import AppKit import ObjectiveC -#if DEBUG import Bonsplit -#endif🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TerminalWindowPortal.swift` around lines 3 - 5, Remove the conditional compile guard around importing Bonsplit so the module is available in release builds: remove the surrounding `#if` DEBUG / `#endif` and keep a plain import Bonsplit at the top of TerminalWindowPortal.swift so references to BonsplitTabBarPassThrough (and any other Bonsplit symbols used at lines referencing BonsplitTabBarPassThrough) compile in production.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmuxTests/TerminalAndGhosttyTests.swift`:
- Around line 2114-2163: The test
testHostViewPassesThroughUnderlyingTabStripBelowTitlebarBand is falsely passing
because WindowTerminalHostView.hitTest only follows the titlebar pass-through
path when the current event is a pointer; add a synthetic mouse pointer event
before calling host.hitTest(pointInHost) so isPointerEvent is true and the
shouldPassThroughToTitlebar() branch is exercised. Create an NSEvent via
NSEvent.mouseEvent(...) positioned at pointInWindow (or converted appropriately)
and deliver it with NSApp.sendEvent(...) (or set NSApp.currentEvent) immediately
prior to the hitTest invocation in the test to ensure the titlebar routing logic
in WindowTerminalHostView.hitTest is actually tested.
---
Outside diff comments:
In `@Sources/TerminalWindowPortal.swift`:
- Around line 3-5: Remove the conditional compile guard around importing
Bonsplit so the module is available in release builds: remove the surrounding
`#if` DEBUG / `#endif` and keep a plain import Bonsplit at the top of
TerminalWindowPortal.swift so references to BonsplitTabBarPassThrough (and any
other Bonsplit symbols used at lines referencing BonsplitTabBarPassThrough)
compile in production.
🪄 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: b42f57ba-d6a2-4a15-af19-856f223565e7
📒 Files selected for processing (4)
Sources/BrowserWindowPortal.swiftSources/TerminalWindowPortal.swiftcmuxTests/BrowserPanelTests.swiftcmuxTests/TerminalAndGhosttyTests.swift
| func testHostViewPassesThroughUnderlyingTabStripBelowTitlebarBand() { | ||
| let window = NSWindow( | ||
| contentRect: NSRect(x: 0, y: 0, width: 420, height: 260), | ||
| styleMask: [.titled, .closable], | ||
| backing: .buffered, | ||
| defer: false | ||
| ) | ||
| defer { window.orderOut(nil) } | ||
| guard let contentView = window.contentView else { | ||
| XCTFail("Expected content view") | ||
| return | ||
| } | ||
| guard let container = contentView.superview else { | ||
| XCTFail("Expected content container") | ||
| return | ||
| } | ||
|
|
||
| let tabStripHeight: CGFloat = 44 | ||
| let tabStrip = FakeTabBarBackgroundNSView( | ||
| frame: NSRect( | ||
| x: 0, | ||
| y: contentView.bounds.maxY - tabStripHeight, | ||
| width: contentView.bounds.width, | ||
| height: tabStripHeight | ||
| ) | ||
| ) | ||
| tabStrip.autoresizingMask = [.width, .minYMargin] | ||
| contentView.addSubview(tabStrip) | ||
|
|
||
| let hostFrame = container.convert(contentView.bounds, from: contentView) | ||
| let host = WindowTerminalHostView(frame: hostFrame) | ||
| host.autoresizingMask = [.width, .height] | ||
| let child = CapturingView(frame: host.bounds) | ||
| child.autoresizingMask = [.width, .height] | ||
| host.addSubview(child) | ||
| container.addSubview(host, positioned: .above, relativeTo: contentView) | ||
|
|
||
| let titlebarBandHeight = max(28, min(72, window.frame.height - window.contentLayoutRect.height)) | ||
| let pointInContent = NSPoint( | ||
| x: contentView.bounds.midX, | ||
| y: contentView.bounds.maxY - titlebarBandHeight - 8 | ||
| ) | ||
| let pointInWindow = contentView.convert(pointInContent, to: nil) | ||
| let pointInHost = host.convert(pointInWindow, from: nil) | ||
|
|
||
| XCTAssertNil( | ||
| host.hitTest(pointInHost), | ||
| "Terminal portal should defer to the minimal tab strip even just below the native titlebar interaction band" | ||
| ) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the regression test calls hitTest without arranging a pointer currentEvent,
# while WindowTerminalHostView gates the pass-through path on NSApp.currentEvent.
rg -n -C5 'testHostViewPassesThroughUnderlyingTabStripBelowTitlebarBand|host\.hitTest|NSApp\.currentEvent|let isPointerEvent|NSEvent\.mouseEvent|sendEvent' \
cmuxTests/TerminalAndGhosttyTests.swift Sources/TerminalWindowPortal.swiftRepository: manaflow-ai/cmux
Length of output: 10461
🏁 Script executed:
#!/bin/bash
# Get the complete hitTest method implementation
sed -n '134,200p' Sources/TerminalWindowPortal.swiftRepository: manaflow-ai/cmux
Length of output: 2530
🏁 Script executed:
#!/bin/bash
# Get the rest of the hitTest method to see non-pointer path
sed -n '134,220p' Sources/TerminalWindowPortal.swiftRepository: manaflow-ai/cmux
Length of output: 3221
🏁 Script executed:
#!/bin/bash
# Check other hitTest calls in the same test class to see if they all have the same issue
rg -n 'func test.*\(\)' cmuxTests/TerminalAndGhosttyTests.swift | grep -A1 "hitTest" | head -20Repository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Find all test methods that call hitTest and check if they set up events
rg -B20 'host\.hitTest|splitView\.hitTest' cmuxTests/TerminalAndGhosttyTests.swift | grep -E '(func test|NSEvent\.mouseEvent|NSApp\.sendEvent|host\.hitTest)'Repository: manaflow-ai/cmux
Length of output: 587
Set NSApp.currentEvent to a pointer event before calling hitTest.
The test expects host.hitTest(pointInHost) to return nil via the shouldPassThroughToTitlebar() logic, but WindowTerminalHostView.hitTest() only executes that routing path when isPointerEvent is true (lines 150–193 of TerminalWindowPortal.swift). Without a pointer event in NSApp.currentEvent, the method skips directly to the non-pointer fallback (lines 195–197), which also happens to return nil—so the test passes for the wrong reason and doesn't actually verify the titlebar pass-through behavior.
Use NSEvent.mouseEvent() to create a pointer event and NSApp.sendEvent() to establish it as the current event, as shown in the proposed diff.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmuxTests/TerminalAndGhosttyTests.swift` around lines 2114 - 2163, The test
testHostViewPassesThroughUnderlyingTabStripBelowTitlebarBand is falsely passing
because WindowTerminalHostView.hitTest only follows the titlebar pass-through
path when the current event is a pointer; add a synthetic mouse pointer event
before calling host.hitTest(pointInHost) so isPointerEvent is true and the
shouldPassThroughToTitlebar() branch is exercised. Create an NSEvent via
NSEvent.mouseEvent(...) positioned at pointInWindow (or converted appropriately)
and deliver it with NSApp.sendEvent(...) (or set NSApp.currentEvent) immediately
prior to the hitTest invocation in the test to ensure the titlebar routing logic
in WindowTerminalHostView.hitTest is actually tested.
![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 4 files
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="cmuxTests/BrowserPanelTests.swift">
<violation number="1" location="cmuxTests/BrowserPanelTests.swift:512">
P2: This test invokes `hitTest` without a current mouse event, so pane-tab-bar pass-through is never evaluated and the child capture view may be returned. Set up a synthetic pointer event before the `hitTest` call so the assertion validates the real routing path.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| let pointInHost = host.convert(pointInWindow, from: nil) | ||
|
|
||
| XCTAssertNil( | ||
| host.hitTest(pointInHost), |
There was a problem hiding this comment.
P2: This test invokes hitTest without a current mouse event, so pane-tab-bar pass-through is never evaluated and the child capture view may be returned. Set up a synthetic pointer event before the hitTest call so the assertion validates the real routing path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cmuxTests/BrowserPanelTests.swift, line 512:
<comment>This test invokes `hitTest` without a current mouse event, so pane-tab-bar pass-through is never evaluated and the child capture view may be returned. Set up a synthetic pointer event before the `hitTest` call so the assertion validates the real routing path.</comment>
<file context>
@@ -457,6 +463,57 @@ final class WindowBrowserHostViewTests: XCTestCase {
+ let pointInHost = host.convert(pointInWindow, from: nil)
+
+ XCTAssertNil(
+ host.hitTest(pointInHost),
+ "Browser portal should defer to the minimal tab strip even just below the native titlebar interaction band"
+ )
</file context>
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
3 participants
Summary
Root Cause
In minimal mode, the browser and terminal portal hosts sat above the Bonsplit tab strip and continued to claim pointer hits just below the native titlebar interaction band. That prevented empty top-bar drags from reaching the minimal-mode window-drag handlers behind the portals.
Testing
./scripts/reload.sh --tag issue-3119-minimal-drag-sidebar --launchCloses #3119
Note
Medium Risk
Adjusts AppKit hit-testing in window-level browser/terminal portal hosts; mistakes could regress click/drag routing around the titlebar/tab strip and split dividers, but scope is limited to pointer-event routing.
Overview
Restores minimal-mode top-bar drag/click behavior by teaching the browser and terminal portal host views to yield pointer hit-testing not only to the native/custom titlebar band, but also to the underlying Bonsplit minimal tab strip.
This introduces shared
BonsplitTabBarPassThroughhelpers (including a registry-based hit check with a view-tree fallback) and adds regression tests asserting the portal hosts returnnilfor hits in the tab strip area just below the titlebar interaction band.Reviewed by Cursor Bugbot for commit bd54dad. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Fixes minimal-mode top-bar dragging by letting browser and terminal portal hosts pass pointer events through to the native titlebar band and the underlying Bonsplit minimal tab strip. Restores window drag behavior just below the titlebar. Closes #3119.
BonsplitTabBarPassThroughhelpers (registry check with view-tree fallback).Written for commit bd54dad. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests