Repository navigation
Fix minimal-mode drag pass-through for new windows - #3194
Conversation
Add second-window portal-host regression coverage so CI can prove the minimal tab-strip drag path is missing for later-created windows before the fix lands. Constraint: Repo policy forbids running local tests Rejected: Fold tests into the fix commit | would hide whether the regression coverage fails on its own Confidence: high Scope-risk: narrow Reversibility: clean Directive: Keep later-window coverage alongside the first-window minimal drag tests when this area changes again Tested: Not run locally; intended to fail in CI before the fix Not-tested: Local XCTest execution per repo policy
Later-created minimal-mode windows can mount terminal or browser portal hosts over the Bonsplit tab strip, so their empty top chrome stops reaching the window drag surface. Share the tab-strip hit-test deferral and apply it in both portal hosts so every window resolves the same drag region. Constraint: Must preserve portal layering while restoring empty top-bar drag behavior Rejected: Toggle NSWindow background dragging globally | breaks tab reordering and other content gestures in minimal mode Confidence: medium Scope-risk: narrow Reversibility: clean Directive: Keep portal-host hit-testing aligned between terminal and browser hosts; later-window regressions tend to come from them drifting apart Tested: Built and launched with ./scripts/reload.sh --tag issue-3193-new-window-minimal-drag --launch Not-tested: Local XCTest execution per repo policy
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughCentralizes Bonsplit tab-strip pass-through logic and adds early-return hit-test routes in browser and terminal portal hosts so pointer events can defer per-window to the underlying minimal-mode tab strip; includes per-window regression tests validating pass-through below the titlebar interaction band. Changes
Sequence Diagram(s)sequenceDiagram
participant Ptr as Pointer Event
participant Host as Portal Host (hitTest / performHitTest)
participant Band as Titlebar Interaction Band
participant Reg as TabBar Registry
participant View as View Hierarchy
participant UI as Underlying Tab Strip UI
Ptr->>Host: Pointer event arrives
Host->>Band: Is point in titlebar interaction band?
alt In interaction band
Host->>Host: clear divider cursor
Host-->>UI: return nil (pass-through)
else Not in band
Host->>Reg: registry-based hit test for tab bar
alt Registry reports hit
Host-->>UI: return nil (pass-through)
else No registry hit
Host->>View: recursive detect underlying TabBarBackgroundNSView
alt Found underlying tab bar view
Host-->>UI: return nil (pass-through)
else Not found
Host->>Host: continue sidebar/divider hit-testing
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
![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
🧹 Nitpick comments (1)
Sources/BrowserWindowPortal.swift (1)
49-53: Trim fallback tree-walk work in the hothitTestpath.On Lines 49-53 and Lines 56-64, the recursive fallback (
hasUnderlyingBonsplitTabBarBackground) runs on every pointer-event registry miss, including high-frequency hover events. Consider adding a cheap early guard (for example, near-top-band Y filtering) before doing the full subtree scan.♻️ Lightweight guard example
static func passThroughDecision( at point: NSPoint, in portalHost: NSView, eventType: NSEvent.EventType? ) -> (windowPoint: NSPoint, result: Bool, registryHit: Bool)? { guard isPassThroughPointerEvent(eventType) else { return nil } let windowPoint = portalHost.convert(point, to: nil) + if let window = portalHost.window { + // Skip expensive fallback scans for points clearly outside top interaction area. + let topBandGuardMinY = titlebarInteractionBandMinY(in: window) - 96 + if windowPoint.y < topBandGuardMinY { + return (windowPoint, false, false) + } + } let decision = shouldPassThroughToPaneTabBar(windowPoint: windowPoint, below: portalHost) return (windowPoint, decision.result, decision.registryHit) }Also applies to: 56-64
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/BrowserWindowPortal.swift` around lines 49 - 53, The fallback subtree scan hasUnderlyingBonsplitTabBarBackground is being invoked in the hot hitTest path for every pointer miss; add a cheap early Y-band guard in hitTest that checks windowPoint.y against portalHost (or a fixed top-band threshold) and only calls hasUnderlyingBonsplitTabBarBackground when the point lies inside that narrow top region (apply the same guard to both call sites where hasUnderlyingBonsplitTabBarBackground is used) to avoid the expensive recursive walk on high-frequency hover events.
🤖 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/TerminalWindowPortal.swift`:
- Around line 148-158: The pass-through checks for titlebar/tab-bar are
currently executed when currentEvent == nil due to the condition using
"isPointerEvent || currentEvent == nil", which violates the documented
requirement to only run this work for pointer events; change the guard to only
use isPointerEvent (remove "|| currentEvent == nil") so
shouldPassThroughToTitlebar(...) and shouldPassThroughToPaneTabBar(...) run only
when isPointerEvent is true, keeping clearActiveDividerCursor(restoreArrow:)
behavior unchanged; update any tests that relied on nil currentEvent to supply a
real mouse pointer event instead of nil.
---
Nitpick comments:
In `@Sources/BrowserWindowPortal.swift`:
- Around line 49-53: The fallback subtree scan
hasUnderlyingBonsplitTabBarBackground is being invoked in the hot hitTest path
for every pointer miss; add a cheap early Y-band guard in hitTest that checks
windowPoint.y against portalHost (or a fixed top-band threshold) and only calls
hasUnderlyingBonsplitTabBarBackground when the point lies inside that narrow top
region (apply the same guard to both call sites where
hasUnderlyingBonsplitTabBarBackground is used) to avoid the expensive recursive
walk on high-frequency hover events.
🪄 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: 941c17f0-3f99-49bb-ac39-2c6072f5f0b7
📒 Files selected for processing (4)
Sources/BrowserWindowPortal.swiftSources/TerminalWindowPortal.swiftcmuxTests/BrowserPanelTests.swiftcmuxTests/TerminalAndGhosttyTests.swift
Greptile SummaryThis PR extracts the minimal-mode tab-strip drag pass-through logic into a shared Confidence Score: 4/5Safe to merge; the fix is well-scoped and the regression tests validate the core scenario. Only P2 findings: two unused The Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["hitTest(point) called\n(Browser or Terminal host)"] --> B{isPointerEvent\nor no current event?}
B -- No --> Z["super.hitTest / standard path"]
B -- Yes --> C{shouldPassThroughToTitlebar?}
C -- Yes --> R1["return nil\n(titlebar drag surface)"]
C -- No --> D{shouldPassThroughToPaneTabBar?}
D -- No --> E["other checks\n(sidebar, divider, drag targets…)"]
D -- Yes --> F{BonsplitTabBarPassThrough\n.passThroughDecision}
F --> G{isPassThroughPointerEvent?}
G -- No --> H["return nil from passThroughDecision\n→ tabStripPassThrough = false"]
G -- Yes --> I{Registry hit?\nBonsplitTabBarHitRegionRegistry}
I -- Yes --> R2["return nil\n(tab strip drag surface)"]
I -- No --> J{hasUnderlyingBonsplitTabBarBackground?\nwalk sibling views below host}
J -- Yes --> R2
J -- No --> E
Reviews (1): Last reviewed commit: "Restore minimal drag pass-through in lat..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| func testHostViewPassesThroughUnderlyingTabStripInSecondWindowBelowTitlebarBand() { |
There was a problem hiding this comment.
firstWindow is created but never used
firstWindow is allocated and deferred for cleanup but is never ordered front, never populated with views, and has no effect on the test outcome. Since the detection logic in hasUnderlyingBonsplitTabBarBackground inspects only secondWindow's view hierarchy, firstWindow's presence changes nothing. If the intent is to document the "second window" scenario, a comment would be clearer; if it was meant to interact with secondWindow (e.g., to exercise some AppKit multi-window state), the interaction is missing.
| let firstWindow = NSWindow( | ||
| contentRect: NSRect(x: 0, y: 0, width: 420, height: 260), |
There was a problem hiding this comment.
firstWindow is created but never used
Same as the browser test: firstWindow is allocated and torn down but plays no role in the assertion. The test exercises only secondWindow's view hierarchy, so firstWindow's existence has no observable effect on correctness.
| static func hasUnderlyingBonsplitTabBarBackground( | ||
| at windowPoint: NSPoint, | ||
| below portalHost: NSView | ||
| ) -> Bool { | ||
| if let container = portalHost.superview, | ||
| let hostIndex = container.subviews.firstIndex(of: portalHost) { | ||
| for sibling in container.subviews[..<hostIndex].reversed() { | ||
| guard !sibling.isHidden, sibling.alphaValue > 0 else { continue } | ||
| if hasBonsplitTabBarBackground(at: windowPoint, in: sibling) { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } | ||
|
|
||
| guard let window = portalHost.window, | ||
| let rootView = window.contentView else { | ||
| return false | ||
| } | ||
| return hasBonsplitTabBarBackground(at: windowPoint, in: rootView) | ||
| } |
There was a problem hiding this comment.
When portalHost.superview is nil (portal host not yet in the view hierarchy), hasUnderlyingBonsplitTabBarBackground falls back to searching the entire window.contentView tree. Unlike the sibling-search branch which only walks views at lower z-indices than the host (subviews[..<hostIndex]), this fallback will return true for any TabBarBackgroundNSView anywhere in the window — including ones rendered above the host. In practice this path is only reached before the host is inserted, so it may never fire in production, but it could produce a false-positive pass-through if the host is temporarily unparented while a tab bar view exists elsewhere in the window.
- Tighten typing-latency guard in WindowTerminalHostView.hitTest by removing the `currentEvent == nil` carve-out and routing tests through a new `performHitTest(at:currentEvent:)` seam that takes an explicit pointer event (CodeRabbit major). - Cap the recursive `hasUnderlyingBonsplitTabBarBackground` walk to the top 200pt band where the minimal tab strip can live, so high-frequency hover/cursor events skip the subtree scan once they fall outside the tab-strip Y range (CodeRabbit nitpick). - Remove the unparented-host fallback that walked the full content view; z-order can't be respected without a superview, so the safe answer is no pass-through (Greptile P2). - Make `firstWindow` load-bearing in the regression tests by asserting pass-through holds in BOTH windows, exercising the full bug repro rather than only the second window (Greptile P2).
…-3193-new-window-minimal-drag
Minimal mode overlays the sidebar, notification, and new-workspace controls in the hidden titlebar host. The icon buttons still need to receive clicks, but the empty spacing between them should behave like titlebar chrome so users can drag the window from that cluster. The fix adds a narrow AppKit hit-test layer for the hidden controls host. It returns nil over the button columns and performs a normal window drag only when the click lands in the inter-icon gaps. Constraint: Keep the fix scoped to the three-icon minimal-mode cluster; surrounding titlebar drag behavior already works. Rejected: Change the global drag-handle sibling hit testing | broader than the user-confirmed failure surface. Confidence: high Scope-risk: narrow Tested: git diff --check; ./scripts/reload.sh --tag issue-3193-drag --launch Not-tested: XCTest locally, per repo testing policy
The workflow guard compares current Swift file lengths against the checked-in budget. PR #3194 intentionally grows portal-host tests and the minimal titlebar control hit-test layer, so the accepted file-length budget needs to match the branch. Constraint: CI fails any tracked Swift file whose actual line count exceeds the checked-in budget. Rejected: Refactor unrelated earlier PR changes only to avoid the budget update | broader than the failing guard and outside the requested fix. Confidence: high Scope-risk: narrow Tested: python3 scripts/swift_file_length_budget.py --budget .github/swift-file-length-budget.tsv; ./tests/test_ci_swift_file_length_budget.sh; git diff --check Not-tested: XCTest locally, per repo testing policy
This branch was successfully deployed
1 participant
Summary
Root Cause
Later-created minimal-mode windows can mount browser/terminal portal hosts above the Bonsplit tab strip. Those window-level hosts were not deferring hits in the empty minimal tab-strip region, so drags in new windows stopped before they reached the window-drag surface.
Testing
==> reload succeeded in 28s
==> log: /tmp/cmux-reload-issue-3193-new-window-minimal-drag.log
App path:
/Users/austinwang/Library/Developer/Xcode/DerivedData/cmux-issue-3193-new-window-minimal-drag/Build/Products/Debug/cmux DEV issue-3193-new-window-minimal-drag.app
Dev web origin:
http://localhost:9550
CLI path:
/Users/austinwang/Library/Developer/Xcode/DerivedData/cmux-issue-3193-new-window-minimal-drag/Build/Products/Debug/cmux DEV issue-3193-new-window-minimal-drag.app/Contents/Resources/bin/cmux
CLI helpers:
/tmp/cmux-cli ...
/Users/austinwang/.local/bin/cmux-dev ...
/Users/austinwang/.codex/tmp/arg0/codex-arg0RadO8e/cmux ...
If your shell still resolves the old cmux, run: rehash
Closes #3193
Note
Medium Risk
Touches AppKit
hitTest/pointer routing and window-drag behavior for browser/terminal portal hosts and titlebar controls; regressions could break clicks, hover cursors, or dragging in key UI surfaces across windows.Overview
Fixes a minimal-mode regression where browser/terminal portal hosts could intercept pointer hits in the top tab-strip region (especially in later-created windows), preventing window/tab dragging.
This centralizes minimal tab-strip pass-through logic in
BonsplitTabBarPassThroughand applies it in bothWindowBrowserHostViewandWindowTerminalHostView(including a test seamperformHitTest(at:currentEvent:)for terminal). It also makes gaps between hidden titlebar control icons draggable via an overlayNSViewRepresentablewhile keeping the buttons themselves interactive.Adds regression tests covering tab-strip pass-through in both the first and second window instances for browser and terminal, plus hit-region tests for titlebar control gaps, and updates the Swift file length budget accordingly.
Reviewed by Cursor Bugbot for commit 6aac688. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Restores minimal-mode dragging in later-created windows by passing empty top-strip hits through to the underlying Bonsplit tab strip in both browser and terminal hosts, fixing #3193. Also makes the gaps between minimal titlebar icons draggable.
Bug Fixes
BonsplitTabBarPassThroughwith aBonsplitTabBarHitRegionRegistryfast path, a 200pt top-band scan cap, and top-band-only scan; removed the unparented-host fallback.performHitTest(at:currentEvent:), gated logic to pointer events, and preserved the typing-latency path.TitlebarControlsGapDragViewandTitlebarControlsHitRegionsso inter-icon gaps start a normal window drag while buttons still receive clicks.Dependencies
.github/swift-file-length-budget.tsvto reflect file growth and keep CI passing.Written for commit 6aac688. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
New Features
Bug Fixes
Tests