Fix minimal UI terminal top-row hit testing#6073
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough
ChangesTitlebar pass-through and double-click deferral refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors, 1 warning)
✅ Passed checks (18 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 |
Contributor
Greptile SummaryThis PR fixes a dead-zone regression in Minimal UI where terminal top-row clicks were silently dropped because
Confidence Score: 5/5Safe to merge — the change is tightly scoped to the titlebar hit-test and double-click deferral paths, all three call sites are updated consistently, and four focused regression tests validate the fix and its boundary conditions. The fix correctly narrows the unconditional titlebar pass-through to only fire when no hosted terminal is under the cursor, with registered controls still taking priority. The lazy resolver avoids double-traversal in the common case. The new hasHostedTerminal recursive helper and the direct-subview hostedTerminalHitView serve two different callers with intentionally different search depths, and the tests exercise both paths. No production paths are left in an incorrect state. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant AppKit as AppKit Event Dispatch
participant WTHV as WindowTerminalHostView
participant Resolver as HitViewResolver (lazy)
participant TitlebarCheck as shouldPassThroughToTitlebar
participant TabBarCheck as shouldPassThroughToPaneTabBar
participant Terminal as GhosttySurfaceScrollView
AppKit->>WTHV: "hitTest(point)"
WTHV->>WTHV: "performHitTest(at:currentEvent:)"
WTHV->>Resolver: "hostedTerminalHitViewResolver(at: point)"
WTHV->>TitlebarCheck: "shouldPassThroughToTitlebar(at:hostedTerminalHitView:)"
alt "NOT in titlebar band"
TitlebarCheck-->>WTHV: "false - continue"
else "in titlebar band AND registered control hit"
TitlebarCheck-->>WTHV: "true - return nil (pass through to chrome)"
else "in titlebar band, no registered control"
TitlebarCheck->>Resolver: "invoke resolver (lazy eval)"
Resolver->>WTHV: "hostedTerminalHitView(at:)"
WTHV->>Terminal: "search direct subviews"
Terminal-->>Resolver: "NSView or nil"
alt "terminal found under cursor"
TitlebarCheck-->>WTHV: "false - terminal keeps click"
else "no terminal"
TitlebarCheck-->>WTHV: "true - pass through to titlebar"
end
end
WTHV->>TabBarCheck: "shouldPassThroughToPaneTabBar(at:eventType:hostedTerminalHitView:)"
alt "registry hit"
TabBarCheck-->>WTHV: "true - pass through"
else "no registry hit"
TabBarCheck->>Resolver: "invoke resolver (cached, no re-eval)"
Resolver-->>TabBarCheck: "cached hit view"
TabBarCheck-->>WTHV: "result based on terminal presence"
end
WTHV-->>AppKit: "hit view (terminal subview or nil)"
Reviews (8): Last reviewed commit: "perf: lazily resolve terminal titlebar h..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+3832
to
+3841
| XCTAssertGreaterThanOrEqual( | ||
| pointInWindow.y, | ||
| BonsplitTabBarPassThrough.titlebarInteractionBandMinY(in: window), | ||
| "The regression point must exercise the fixed-height titlebar pass-through band" | ||
| ) | ||
| assertHitFallsInsideHostedTerminal( | ||
| host.performHitTest(at: pointInHost, currentEvent: event), | ||
| hostedView: hostedView, | ||
| message: "Minimal-mode terminal content that extends into the titlebar band should keep receiving top-row mouse-downs" | ||
| ) |
Contributor
There was a problem hiding this comment.
XCTAssertGreaterThanOrEqual records a failure but does not stop execution. If the window geometry unexpectedly places pointInWindow.y below titlebarInteractionBandMinY, the guard assertion fires but assertHitFallsInsideHostedTerminal is still evaluated. Because the hosted terminal fills the entire host.bounds, a click anywhere on it would route to the terminal even without the fix — so the second assertion would likely pass on the old code as well, making the regression test give a false sense of coverage while only the guard shows as failing. Adding a return (or using XCTSkipIf) after the guard assertion ensures the test aborts rather than silently exercising the wrong scenario.
Suggested change
| XCTAssertGreaterThanOrEqual( | |
| pointInWindow.y, | |
| BonsplitTabBarPassThrough.titlebarInteractionBandMinY(in: window), | |
| "The regression point must exercise the fixed-height titlebar pass-through band" | |
| ) | |
| assertHitFallsInsideHostedTerminal( | |
| host.performHitTest(at: pointInHost, currentEvent: event), | |
| hostedView: hostedView, | |
| message: "Minimal-mode terminal content that extends into the titlebar band should keep receiving top-row mouse-downs" | |
| ) | |
| guard pointInWindow.y >= BonsplitTabBarPassThrough.titlebarInteractionBandMinY(in: window) else { | |
| XCTFail("The regression point must exercise the fixed-height titlebar pass-through band") | |
| return | |
| } | |
| assertHitFallsInsideHostedTerminal( |
Contributor
Author
There was a problem hiding this comment.
Fixed in b6198ed by converting the titlebar-band preconditions to guard/return checks in the new focused test file, so the assertions cannot continue on the wrong geometry.
— Claude Code
Contributor
Author
There was a problem hiding this comment.
Superseded in the current test file: the titlebar-band geometry check now uses Swift Testing , so the test exits before evaluating the hit assertion if the point is outside the fixed band.
— Claude Code
![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
🤖 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 `@Sources/TerminalWindowPortal.swift`:
- Line 126: The hit-test path is calling hostedTerminalHitView(at:) multiple
times redundantly - once within shouldPassThroughToTitlebar and again within
shouldPassThroughToPaneTabBar. To fix this, compute hostedTerminalHitView(at:)
once before calling these helper methods and cache the result. Then refactor
both shouldPassThroughToTitlebar and shouldPassThroughToPaneTabBar to accept the
precomputed hosted terminal view as an optional parameter instead of computing
it internally. Update all existing call sites of these helpers to pass either
the precomputed view or nil (for backward compatibility where the optimization
is not applicable) to eliminate the redundant scans in the pointer event hot
path.
🪄 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: 8f21802a-85d9-4388-8e2d-3a9b701ca368
📒 Files selected for processing (3)
Sources/TerminalWindowPortal.swiftcmux.xcodeproj/project.pbxprojcmuxTests/WindowTerminalHostViewTitlebarHitTests.swift
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 `@Sources/WindowTerminalHostViewHitTesting.swift`:
- Around line 4-34: The `hasHostedTerminal(at:)` method only checks direct
subviews, missing hosted terminals wrapped by intermediate views - refactor this
method to be recursive, so it also checks nested subviews that aren't
GhosttySurfaceScrollView instances. In the private static
`hasHostedTerminal(atWindowPoint:in:)` method, the recursion loop starting at
line 30 traverses every subview without checking whether the point is inside
them first, causing broad tree walks - add a bounds check for each subview
before recursing into it, ensuring only subviews that actually contain the point
are traversed to improve performance and properly bound the hit-test search.
🪄 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: 6ecdf5dc-7bff-4ce3-9b1b-5791bf12121a
📒 Files selected for processing (5)
Sources/WindowDragHandleView.swiftSources/WindowTerminalHostViewHitTesting.swiftcmux.xcodeproj/project.pbxprojcmuxTests/TitlebarInteractiveControlTests.swiftcmuxTests/WindowTerminalHostViewTitlebarHitTests.swift
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 `@Sources/TerminalWindowPortal.swift`:
- Around line 203-207: The `shouldPassThroughToTitlebar` function's default
parameter of `hostedTerminalHitView = nil` makes titlebar-band pass-through
unconditional when callers omit this argument, diverging from the overlap-aware
behavior in `performHitTest`. Update the calls to `shouldPassThroughToTitlebar`
at lines 239-242 to explicitly pass the appropriate `hostedTerminalHitView`
value instead of relying on the `nil` default, ensuring consistent titlebar
routing behavior over hosted terminal content.
🪄 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: 42f9339e-cb81-460a-b5d0-b64c91d590c8
📒 Files selected for processing (2)
Sources/TerminalWindowPortal.swiftcmux.xcodeproj/project.pbxproj
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #6066
Reproduction
on_clickevents.cmux-assets/issue-6066-minimal-ui-toolbar-hittest/local-repro/.Root Cause
WindowTerminalHostView.performHitTestunconditionally passed through points in the fixed titlebar interaction band. In minimal UI, terminal content can extend into that band, so top terminal rows were treated as titlebar/chrome instead of terminal content.Verification
reload.shper issue instructions; CI is the validation gate.Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Fixes #6066. Keeps terminal top-row clicks working in Minimal UI and defers synthetic titlebar double‑clicks when terminal content is under the cursor.
shouldPassThroughToChromewith the same logic.minimalModeTitlebarDoubleClickShouldDeferusing window‑scoped terminal detection; apply in titlebar monitors andWindowDecorationsController.WindowTerminalHostViewTitlebarHitTests; updateTitlebarInteractiveControlTests.Written for commit 0288a7f. Summary will update on new commits.
Summary by CodeRabbit