Repository navigation
Fix #5099: keep right-sidebar titlebar buttons clickable (stop reparenting interactive controls) - #5101
Conversation
`titlebarInteractiveControl()` previously wrapped each control in a nested `NSHostingView` (`TitlebarInteractiveHostingView`) to win window-drag, resize-drag, and double-click-zoom routing over the control. Reparenting a SwiftUI control into a nested hosting view works for controls hosted in a real `NSTitlebarAccessoryViewController` (the sidebar toggle / notifications / new-tab / focus-history / update pill), but silently drops mouse-downs for controls that live in the full-size-content titlebar band — the right-sidebar mode bar (Files / Search / Feed / Vault), its close + open-as-pane buttons, and the session-index header controls. The keyboard path was unaffected, matching the report. Fix the shared modifier instead of special-casing one row: stop reparenting. `titlebarInteractiveControl()` now applies a transparent `.background(...)` marker (`TitlebarInteractiveControlRegion`) that registers the control's region with `MinimalModeTitlebarControlHitRegionRegistry` and returns `nil` from hitTest. Every titlebar drag/double-click surface already consults that registry via `isMinimalModeTitlebarControlHit` and yields over registered regions, so the control keeps receiving clicks in place while staying immune to window drag/resize/zoom. This is the same proven mechanism the right-sidebar buttons used before PR #5005, generalized to every current and future titlebar control. The drag handle's registry point-check runs before its sibling-walk, so the now-removed reparenting identifier fallback (`windowDragHandleHitBelongsToTitlebarInteractiveControl`) is unnecessary; the reparenting infrastructure (`TitlebarInteractiveControlHost`, `TitlebarInteractiveHostingView`) is deleted so it can't be reached again. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughReplace the SwiftUI/AppKit hosting-wrapper approach with a transparent ChangesTitlebar interactive control architecture refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning, 1 inconclusive)
✅ Passed checks (15 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 |
![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 e162d54. Configure here.
…nting Addresses Cursor Bugbot: dropping the nested NSHostingView also dropped its acceptsFirstMouse(for:), so an inactive-window click on a titlebar control would only activate the window. Restore that behavior generally via the registry: MainWindowHostingView.acceptsFirstMouse now returns true exactly when the click lands in a registered MinimalModeTitlebarControlHitRegionRegistry region (the regions titlebarInteractiveControl() registers), and keeps the default (false) for all other content. No reparenting — so active-window clicks keep working in the full-size-content titlebar band. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![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 6 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="Sources/WindowDragHandleView.swift">
<violation number="1" location="Sources/WindowDragHandleView.swift:550">
P2: This transparent marker never becomes the hit view (`hitTest` returns `nil`), so the previous `acceptsFirstMouse` click-through guarantee is removed for titlebar controls. On an inactive window, affected controls can require a second click because the first click only activates the window.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| } | ||
| } | ||
|
|
||
| override func hitTest(_ point: NSPoint) -> NSView? { nil } |
There was a problem hiding this comment.
P2: This transparent marker never becomes the hit view (hitTest returns nil), so the previous acceptsFirstMouse click-through guarantee is removed for titlebar controls. On an inactive window, affected controls can require a second click because the first click only activates the window.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/WindowDragHandleView.swift, line 550:
<comment>This transparent marker never becomes the hit view (`hitTest` returns `nil`), so the previous `acceptsFirstMouse` click-through guarantee is removed for titlebar controls. On an inactive window, affected controls can require a second click because the first click only activates the window.</comment>
<file context>
@@ -534,6 +521,50 @@ enum MinimalModeTitlebarControlHitRegionRegistry {
+ }
+ }
+
+ override func hitTest(_ point: NSPoint) -> NSView? { nil }
+
+ override var mouseDownCanMoveWindow: Bool { false }
</file context>
There was a problem hiding this comment.
Fixed in 221150e. You're right the marker can't grant first-mouse (it's hitTest=nil by design, so it never blocks the control's own clicks). The first-mouse guarantee is instead restored on the content host: MainWindowHostingView.acceptsFirstMouse(for:) now returns true exactly when the click lands in a registered MinimalModeTitlebarControlHitRegionRegistry region (every titlebarInteractiveControl()), and the default (false) elsewhere. Subclassing the SwiftUI content NSHostingView to govern acceptsFirstMouse is the standard way to make hosted SwiftUI controls first-mouse-clickable — SwiftUI routes the button's mouse-down through the hosting view, so the hosting view's answer governs. This recovers single-click-on-inactive-window for all these controls without reparenting (reparenting is what dropped active-window clicks in the full-size-content titlebar band, the bug this PR fixes).
— Claude Code
There was a problem hiding this comment.
Thanks for the feedback! I've saved this as a new learning to improve future reviews.
Greptile SummaryFixes dead-clicks on right-sidebar titlebar buttons (#5099) by replacing the reparenting-into-nested-
Confidence Score: 5/5Safe to merge. The change removes a footgun (reparenting NSHostingView) and replaces it with a simpler, registry-based marker that is already used by adjacent code; the fix is targeted and all changed paths are well-tested. The root cause is correctly identified and eliminated. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant AppKit
participant MainWindowHostingView
participant RegisteredView
participant Registry as MinimalModeTitlebarControlHitRegionRegistry
participant DragHandle as WindowDragHandleView
participant Control as SwiftUI Control
Note over RegisteredView,Registry: On view mount (viewDidMoveToWindow)
RegisteredView->>Registry: register(self)
Note over User,Control: Mouse click on titlebar control (active window)
User->>AppKit: leftMouseDown
AppKit->>DragHandle: hitTest(point)
DragHandle->>Registry: isMinimalModeTitlebarControlHit?
Registry-->>DragHandle: true (registered region hit)
DragHandle-->>AppKit: nil (yields)
AppKit->>Control: delivers mouseDown ✓
Note over User,Control: Mouse click on titlebar control (inactive window)
User->>AppKit: leftMouseDown (window inactive)
AppKit->>MainWindowHostingView: acceptsFirstMouse(for: event)?
MainWindowHostingView->>Registry: isMinimalModeTitlebarControlHit?
Registry-->>MainWindowHostingView: true
MainWindowHostingView-->>AppKit: true
AppKit->>Control: activate + deliver mouseDown ✓
Note over User,DragHandle: Mouse click on empty titlebar chrome
User->>AppKit: leftMouseDown
AppKit->>DragHandle: hitTest(point)
DragHandle->>Registry: isMinimalModeTitlebarControlHit?
Registry-->>DragHandle: false
DragHandle-->>AppKit: self (captures)
AppKit->>DragHandle: mouseDown → performDrag ✓
Reviews (1): Last reviewed commit: "fix: grant first-mouse to registered tit..." | Re-trigger Greptile |
This branch was successfully deployed
1 participant
Fixes #5099.
Symptom
The right sidebar's top button row (Files / Search / Feed / Vault), plus its close and open-as-pane buttons, rendered normally but dead-clicked — mouse clicks were not registered (no hover/press, no panel switch). The keyboard shortcut still toggled/switched the sidebar, which isolated the regression to buttons receiving mouse events, not the
RightSidebarModeaction path.Root cause (bisected to PR #5005,
03c937d6f/4cdf383c3)PR #5005 ("protect titlebar controls from resize drags", #5003) replaced each right-sidebar mode button's
.background(MinimalModeTitlebarControlHitRegionView())(a zero-impact sibling that only registered the control's hit region, leaving the button a normal SwiftUI control) with.titlebarInteractiveControl(). That modifier reparented the control into a nestedNSHostingView(TitlebarInteractiveHostingView) to setmouseDownCanMoveWindow = false+acceptsFirstMouseand to claim drag/double-click immunity.Reparenting works for controls hosted in a real
NSTitlebarAccessoryViewController(the sidebar toggle / notifications / new-tab / focus-history / update pill — what #5003 actually targeted). But the right-sidebar mode bar lives in the full-size-content titlebar band (part of the SwiftUI content hierarchy, overlapping the window titlebar). Moving those controls into a nested hosting view dropped their mouse-downs there — visible-but-dead. The same modifier is also applied to the session-index header controls (SessionIndexView), which had the same latent break.Fix — general, not a special-case (per
rethink-architecturally)The general class is "interactive titlebar controls reparented into a nested
NSHostingViewlose clicks in the full-size-content band." Fix the shared modifier instead of patching one row:titlebarInteractiveControl()no longer reparents. It applies a transparent.background(...)marker,TitlebarInteractiveControlRegion, that registers the control's frame withMinimalModeTitlebarControlHitRegionRegistryand returnsnilfromhitTest(zero impact on the control's own clicks —.backgrounddoes not change layout).isMinimalModeTitlebarControlHitand yields over registered regions. So the control keeps receiving clicks in place while staying immune to window drag, resize-drag (Sidebar toggle button in title bar sometimes starts a window resize-drag instead of toggling #5003), and double-click zoom/minimize.windowDragHandleShouldCaptureHitchecks the registry before its sibling-walk, so the reparenting-identifier fallback (windowDragHandleHitBelongsToTitlebarInteractiveControl) is redundant and is removed. The reparenting infrastructure (TitlebarInteractiveControlHost,TitlebarInteractiveHostingView) is deleted so the footgun can't be reused.This is the same registry mechanism the right-sidebar buttons used and worked with before #5005, now generalized to every current and future titlebar control. #5003's resize-drag protection is preserved because it is the registry that makes the drag handle yield, not the reparenting.
First-mouse (inactive-window single click) — preserved generally
The nested hosting view also set
acceptsFirstMouse, so dropping it would otherwise mean a click on a titlebar control while the window is inactive only activates the window (needs a second click). Restored generally without reparenting:MainWindowHostingView.acceptsFirstMouse(for:)now returnstrueexactly when the click lands in a registeredMinimalModeTitlebarControlHitRegionRegistryregion (everytitlebarInteractiveControl()), and keeps the default (false) for all other content. Subclassing the SwiftUI contentNSHostingViewto governacceptsFirstMouseis the standard way to make hosted SwiftUI controls first-mouse-clickable. (Addresses Cursor Bugbot + cubic P2.)Tests
Hit-testing/event-routing in the full-size-content titlebar band depends on the real
NSWindowtheme-frame and is not cleanly unit-testable headlessly, so there is no red/green repro of the dead-click itself (stating this plainly rather than faking one).TitlebarInteractiveControlTestsis rewritten to assert the protection contract behaviorally on the new mechanism:windowDragHandleShouldCaptureHityield (click reaches the control) while empty chrome stays draggable;isMinimalModeTitlebarControlHit) so the synthetic double-click zoom is suppressed;hitTest == nil) and does not move the window.The existing
WindowAndDragTestspassive-host/sibling-walk and double-click tests are unaffected.Validation
python3 scripts/normalize-pbxproj.py cmux.xcodeproj/project.pbxproj+./scripts/check-pbxproj.shpass (two source files removed from the project).git diff --checkclean.🤖 Generated with Claude Code