Repository navigation
Fix #5003: protect titlebar controls from resize drags - #5005
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:
📝 WalkthroughWalkthroughAdds TitlebarInteractiveHostingView plus an NSViewRepresentable and ViewModifier to mark SwiftUI controls as interactive titlebar controls, updates window-drag hit-testing to respect those markers, applies the modifier to multiple controls, and adds tests + project wiring. ChangesTitlebar Interactive Control System
Sequence DiagramsequenceDiagram
participant User
participant Button as Titlebar Button
participant Host as TitlebarInteractiveHostingView
participant WindowDragHandle
User->>Button: Click on control in titlebar
Button->>Host: mouseDown event delivered
Host->>Host: mouseDownCanMoveWindow = false
Host->>Host: acceptsFirstMouse = true
Host->>Button: Event reaches control logic
WindowDragHandle->>WindowDragHandle: windowDragHandleHitBelongsToTitlebarInteractiveControl?
WindowDragHandle->>WindowDragHandle: Walk superview chain for identifier
WindowDragHandle-->>WindowDragHandle: return false (not passive) if found
Button->>User: Control handles click, window does not drag
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 17 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (17 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 #5003 by replacing per-control
Confidence Score: 5/5Safe to merge — changes are confined to titlebar hit-testing and mouse-routing plumbing with no data, auth, or state-management paths affected. The new code paths (superview walk, registry registration, mouseDownCanMoveWindow override, acceptsFirstMouse) are well-scoped and the failure modes are limited to visual/interaction edge cases rather than data loss or security issues. The pre-existing deinit-calls-registry pattern is maintained consistently. Tests cover both drag-capture blocking and double-click suppression. No files require special attention; TitlebarInteractiveControlHost.swift has a minor redundant registry call on every SwiftUI update that could be cleaned up. Important Files Changed
Sequence DiagramsequenceDiagram
participant SUI as SwiftUI Body
participant Mod as titlebarInteractiveControl()
participant Host as TitlebarInteractiveControlHost
participant HV as TitlebarInteractiveHostingView
participant Reg as MinimalModeTitlebarControlHitRegionRegistry
participant DH as WindowDragHandleView
SUI->>Mod: .titlebarInteractiveControl()
Mod->>Host: makeNSView / updateNSView
Host->>HV: create + set identifier
HV->>Reg: register(self) via viewDidMoveToWindow
Note over DH: Mouse-down hit test
DH->>DH: windowDragHandleShouldCaptureHit(point)
DH->>DH: sibling.hitTest → hitView
DH->>DH: windowDragHandleShouldTreatTopHitAsPassiveHost(hitView)
DH->>DH: windowDragHandleHitBelongsToTitlebarInteractiveControl(hitView)
Note right of DH: Walk superview chain to contentView, find viewIdentifier
DH-->>SUI: "capture=false, control gets the click"
Note over DH: Synthetic double-click check
DH->>Reg: isMinimalModeTitlebarControlHit(window, point)
Reg->>Reg: containsWindowPoint finds HV
Reg-->>DH: true, suppress zoom/minimize
Reviews (6): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile |
| let titlebarInteractiveControlViewIdentifier = NSUserInterfaceItemIdentifier("cmux.titlebarInteractiveControl") | ||
|
|
||
| /// Hosts SwiftUI controls that must receive titlebar mouse-downs instead of moving the window. | ||
| @MainActor | ||
| final class TitlebarInteractiveHostingView<Content: View>: NSHostingView<Content> { | ||
| override var mouseDownCanMoveWindow: Bool { false } | ||
|
|
||
| override func acceptsFirstMouse(for event: NSEvent?) -> Bool { | ||
| true | ||
| } | ||
| } |
There was a problem hiding this comment.
The identifier is declared as a module-level
let constant, but WindowDragHandleView.viewIdentifier is namespaced as a static let inside the struct that owns it. Putting the identifier on the class that sets and reads it avoids polluting the module namespace and makes the ownership explicit.
| let titlebarInteractiveControlViewIdentifier = NSUserInterfaceItemIdentifier("cmux.titlebarInteractiveControl") | |
| /// Hosts SwiftUI controls that must receive titlebar mouse-downs instead of moving the window. | |
| @MainActor | |
| final class TitlebarInteractiveHostingView<Content: View>: NSHostingView<Content> { | |
| override var mouseDownCanMoveWindow: Bool { false } | |
| override func acceptsFirstMouse(for event: NSEvent?) -> Bool { | |
| true | |
| } | |
| } | |
| /// Hosts SwiftUI controls that must receive titlebar mouse-downs instead of moving the window. | |
| @MainActor | |
| final class TitlebarInteractiveHostingView<Content: View>: NSHostingView<Content> { | |
| static let viewIdentifier = NSUserInterfaceItemIdentifier("cmux.titlebarInteractiveControl") | |
| override var mouseDownCanMoveWindow: Bool { false } | |
| override func acceptsFirstMouse(for event: NSEvent?) -> Bool { | |
| true | |
| } | |
| } |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| private func windowDragHandleHitBelongsToTitlebarInteractiveControl(_ view: NSView) -> Bool { | ||
| var candidate: NSView? = view | ||
| while let current = candidate { | ||
| if current.identifier == titlebarInteractiveControlViewIdentifier { | ||
| return true | ||
| } | ||
| candidate = current.superview | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
The superview walk has no upper-bound guard: for every sibling hit that lands in empty chrome (and therefore has no
TitlebarInteractiveHostingView ancestor), it traverses all the way up to the root of the AppKit hierarchy before returning false. Stopping at the drag handle's own window content view makes the scope explicit and avoids crossing any unexpected hosting boundaries.
| private func windowDragHandleHitBelongsToTitlebarInteractiveControl(_ view: NSView) -> Bool { | |
| var candidate: NSView? = view | |
| while let current = candidate { | |
| if current.identifier == titlebarInteractiveControlViewIdentifier { | |
| return true | |
| } | |
| candidate = current.superview | |
| } | |
| return false | |
| } | |
| private func windowDragHandleHitBelongsToTitlebarInteractiveControl(_ view: NSView) -> Bool { | |
| let contentView = view.window?.contentView | |
| var candidate: NSView? = view | |
| while let current = candidate { | |
| if current.identifier == TitlebarInteractiveHostingView<AnyView>.viewIdentifier { | |
| return true | |
| } | |
| if let contentView, current === contentView { | |
| break | |
| } | |
| candidate = current.superview | |
| } | |
| return false | |
| } |
5e58fb5 to
835e9b7
Compare
835e9b7 to
655d8f1
Compare
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/RightSidebarPanelView.swift`:
- Around line 590-591: The titlebar control modifier order is inconsistent: in
this location the .titlebarInteractiveControl() is applied before
.background(MinimalModeTitlebarControlHitRegionView()), while in
openAsPaneButton and closeButton the background comes first; update this
occurrence in RightSidebarPanelView so the
.background(MinimalModeTitlebarControlHitRegionView()) is applied before
.titlebarInteractiveControl(), matching the ordering used in openAsPaneButton
and closeButton to improve consistency and readability.
🪄 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: 188ab046-a687-49a0-910c-df89edb654aa
📒 Files selected for processing (8)
Sources/RightSidebarPanelView.swiftSources/SessionIndexView.swiftSources/TitlebarInteractiveControlHost.swiftSources/TitlebarInteractiveControlModifier.swiftSources/TitlebarInteractiveHostingView.swiftSources/Update/UpdateTitlebarAccessory.swiftSources/WindowDragHandleView.swiftcmux.xcodeproj/project.pbxproj
A control wrapped with titlebarInteractiveControl() must register as a titlebar control hit region so the synthetic titlebar double-click monitors skip it instead of zooming/minimizing the window. This test fails until the host self-registers with MinimalModeTitlebarControlHitRegionRegistry. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
titlebarInteractiveControl() suppressed window drags (mouseDownCanMoveWindow = false) but not the synthetic titlebar double-click action, which is run by local-monitor surfaces (TitlebarDoubleClickMonitorView, the minimal-mode titlebar surfaces, the window drag handle, and WindowDecorationsController). Those surfaces all defer to MinimalModeTitlebarControlHitRegionRegistry via isMinimalModeTitlebarControlHit, but the interactive-control host never registered, so double-clicking a titlebar icon (e.g. the sidebar toggle) zoomed/minimized the window in addition to running the control's action. Make TitlebarInteractiveHostingView register itself with the registry so a single titlebarInteractiveControl() grants both drag- and double-click- immunity. This makes the per-call-site .background(MinimalModeTitlebarControlHitRegionView()) registrations redundant, so remove them and the now-unused view. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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 39f60a7. Configure here.
|
|
||
| let container = NSView(frame: NSRect(x: 0, y: 0, width: 240, height: 48)) | ||
| window.contentView = container | ||
|
|
||
| // Mirror how `titlebarInteractiveControl()` hosts a control: the host must | ||
| // register itself as a titlebar control hit region so the synthetic | ||
| // double-click monitors skip it instead of zooming/minimizing the window. | ||
| let host = TitlebarInteractiveHostingView(rootView: AnyView(Color.clear)) | ||
| host.identifier = TitlebarInteractiveHostingView<AnyView>.viewIdentifier | ||
| host.frame = NSRect(x: 12, y: 14, width: 24, height: 24) | ||
| container.addSubview(host) | ||
| host.layoutSubtreeIfNeeded() | ||
|
|
||
| let insideControl = NSPoint(x: host.frame.midX, y: host.frame.midY) | ||
| #expect( | ||
| isMinimalModeTitlebarControlHit(window: window, locationInWindow: insideControl), | ||
| "A double-click on a titlebarInteractiveControl must register as a control hit so the synthetic titlebar double-click (zoom/minimize) is suppressed." | ||
| ) | ||
|
|
||
| let emptyTitlebar = NSPoint(x: 220, y: 24) | ||
| #expect( | ||
| !isMinimalModeTitlebarControlHit(window: window, locationInWindow: emptyTitlebar), |
There was a problem hiding this comment.
Second test assertion will fail —
isMinimalModeTitlebarControlHit doesn't check TitlebarInteractiveHostingView
isMinimalModeTitlebarControlHit only queries MinimalModeTitlebarControlHitRegionRegistry.containsWindowPoint, which iterates over views explicitly registered via MinimalModeTitlebarControlHitRegionView.RegisteredView. TitlebarInteractiveHostingView never calls MinimalModeTitlebarControlHitRegionRegistry.register, so the #expect(isMinimalModeTitlebarControlHit(…) == true) assertion on line 91 will evaluate to false and fail CI.
This also exposes a production gap: in minimal mode, a double-click on any control that only receives .titlebarInteractiveControl() (e.g. TitlebarControlButton — sidebar toggle, notifications, new workspace, focus history — and the SessionIndexView scope/reload controls) would not suppress the synthetic titlebar double-click, so the window would still zoom or minimize on a double-click landing on those buttons.
The RightSidebarPanelView controls happen to have MinimalModeTitlebarControlHitRegionView() applied alongside .titlebarInteractiveControl(), so they are covered. But TitlebarControlButton has no MinimalModeTitlebarControlHitRegionView.
The simplest fix is to have TitlebarInteractiveHostingView self-register with the registry in viewDidMoveToWindow/deinit, matching the pattern used by MinimalModeTitlebarControlHitRegionView.RegisteredView.
Includes detachable SSH PTY persistence (manaflow-ai#4807), titlebar polish, Hermes session restore + pre_tool_call notif, omnibar lag fix, sidebar extension polish, blank sidebar icon fallback, browser extension pane integration, GitHub GraphQL rate-limit handling, sidebar resize-toggle drag, large diff viewer pre-render. Conflicts resolved: - Workspace.swift: keep fork's layoutTabs scaffolding while adopting upstream's snapshotWorkspaceId-aware restore signature, persistent SSH PTY restored session-id path, browser tab mute (manaflow-ai#5005), and the relayPort-aware reverse-relay control-master invocation. - AppDelegate.swift, ShortcutBareStartRouting.swift: combine fork's toggleQuickTerminal exception with upstream's browser-content shortcut filter. - UpdateTitlebarAccessory.swift: keep both fork's rightSidebarToggleControllers + sidebarTrailingEdgesByWindow and upstream's titlebar control changes. - TerminalControllerSocketSecurityTests.swift: keep both fork's v2 surface report-pwd remote workspace test and upstream's preserve_after_terminal_exit + remote PTY resize tests. - SessionBlueprintEncoder.swift: extend StableLayout encoder with the new PanelType.extensionBrowser case. - CmuxSettingsUI/SettingsWindowScene.swift: rename the local `anchor` binding so fork's host-injected extra-section path doesn't shadow upstream's new function-scope `anchor` parameter.
`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 manaflow-ai#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>
Fixes #5003.
Summary
WindowDragHandleView; it asserts the button blocks drag capture while empty titlebar chrome remains draggable.titlebarInteractiveControl()SwiftUI modifier backed by an AppKitNSHostingViewshim that opts out ofmouseDownCanMoveWindowand accepts first mouse.WindowDragHandleViewto treat hits inside that shim as interactive control hits, even when SwiftUI returns a private hosting subview fromhitTest.TitlebarControlButtonpath, which covers the sidebar toggle, notifications, new workspace, and focus-history controls, plus sibling right-sidebar titlebar/toolbar controls.Code Path
AppDelegate.makeWindowuses.titled,.resizable,.fullSizeContentView, transparent titlebar chrome, and cmux's explicit drag-handle behavior.UpdateTitlebarAccessoryController->TitlebarControlsAccessoryViewController->TitlebarControlsView.controlsGroup->TitlebarControlButton->AppDelegate.toggleSidebarInActiveMainWindow.Notes
WindowDragHandleView, so dragging the window remains available outside interactive controls.Validation
git diff --checkpython3 scripts/normalize-pbxproj.py --check cmux.xcodeproj/project.pbxproj./scripts/check-pbxproj.shLocal app reload/build and local test execution were intentionally not run per the issue instructions; CI should validate the executable paths.
Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Note
Low Risk
Localized AppKit/SwiftUI titlebar hit-testing and gesture routing; no auth, data, or core business-logic changes.
Overview
Fixes titlebar controls losing clicks to window drag and titlebar double-click (zoom/minimize) by replacing the old passive
MinimalModeTitlebarControlHitRegionViewbackground with a reusable.titlebarInteractiveControl()modifier.The modifier hosts the control in an AppKit
TitlebarInteractiveHostingViewthat opts out of window-drag on mouse down, accepts first mouse, registers withMinimalModeTitlebarControlHitRegionRegistry, and exposes a stable view identifier.WindowDragHandleViewnow treats hits under that host as real controls (not passiveNSHostingViewpass-through), so explicit drag capture does not swallow button clicks.The modifier is applied across shared titlebar UI (
TitlebarControlButton/ update accessory) and right-sidebar chrome (mode pills, close, open-as-pane, vault grouping, scope toggle, reload). Swift tests cover drag vs. empty titlebar chrome and double-click suppression on interactive controls.Reviewed by Cursor Bugbot for commit 42c4078. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Fixes #5003. Prevents window drag/resize and titlebar double-click from stealing clicks on titlebar controls by adding a SwiftUI modifier that marks controls as interactive. Empty titlebar space remains draggable and still responds to double-click.
.titlebarInteractiveControl()that hosts content in an AppKitNSHostingViewsubclass, disables window-drag on mouse down, accepts first mouse, sets a stable identifier, and registers as a titlebar-control hit region to suppress titlebar double-click actions.WindowDragHandleViewto skip drag capture for hits inside that host (even if a nested SwiftUI subview is the top hit)..background(MinimalModeTitlebarControlHitRegionView())with the new modifier and removed the old view.TitlebarControlButtonand right-sidebar controls: mode bar, grouping, scope toggle, reload, Open as Pane, and header controls.Written for commit 42c4078. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Tests