Repository navigation
Fix minimal mode pane tab drag routing - #4290
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:
📝 WalkthroughWalkthroughShort-circuit tab-bar pass-through on Bonsplit registry hits; add a window-move suppression reason and sequencing for Bonsplit pane/tab drags; disable hosting-view mouse-down window moves; update NSWindow event wiring and drag-handle capture; add tests and bump vendor submodule. ChangesTab-Bar Registry Hit Pass-Through
Window-Move Suppression and Tests
Vendor
Sequence DiagramsequenceDiagram
participant User as Client
participant Window as NSWindow
participant DragHandle as WindowDragHandleView
participant Registry as BonsplitTabItemHitRegionRegistry
participant Host as WindowTerminalHostView
participant Hosted as GhosttySurfaceScrollView
User->>Window: mouseDown event
Window->>DragHandle: windowDragHandleShouldCaptureHit?
DragHandle->>Registry: containsWindowPoint?
Registry-->>DragHandle: hit / no-hit
alt Registry hit
DragHandle-->>Window: decline capture
Window->>Host: performHitTest -> asks registry
Host->>Registry: query registryHit
Registry-->>Host: registryHit == true
Host-->>Window: defer to tab-strip region
else No registry hit
DragHandle-->>Window: capture (start suppression sequence as needed)
Window->>Hosted: hit test hosted terminal
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ 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 |
Greptile SummaryThis PR fixes minimal-mode pane-tab drag routing by making the main window permanently
Confidence Score: 5/5Safe to merge; the changes are narrowly scoped to the mouse-event dispatch path and are well-covered by targeted regression tests. The fix addresses the root cause directly: the main window starts immovable, explicit drag chrome is the only path to movement, and the new sequence model keeps that invariant through the full down→drag→up lifecycle. The BonsplitTabItemHitRegionRegistry early exit in windowDragHandleShouldCaptureHit closes the remaining hit-capture escape hatch. Test coverage is thorough — sequence lifecycle, stale-sequence replacement, drag-handle rejection, empty-chrome availability, and terminal portal trust order are all exercised. No production logging, no concurrency primitives, and no user-facing strings were introduced. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant AppKit
participant sendEvent as NSWindow.sendEvent
participant Sequence as SuppressionSequence
participant DragHandle as WindowDragHandleView
participant Registry as BonsplitTabItemHitRegionRegistry
Note over User,Registry: Pane-tab click (bug fixed path)
User->>AppKit: leftMouseDown on tab
AppKit->>sendEvent: dispatch
sendEvent->>Registry: containsWindowPoint?
Registry-->>sendEvent: true - begin sequence
sendEvent->>Sequence: beginWindowMoveSuppressionSequence(.bonsplitPaneTabDrag)
Sequence-->>sendEvent: "isMovable=false, sequence active"
sendEvent->>AppKit: cmux_sendEvent (tab receives click)
User->>AppKit: leftMouseDragged
AppKit->>sendEvent: dispatch
sendEvent->>Sequence: continueSequence - ensureImmovable
sendEvent->>AppKit: cmux_sendEvent (drag goes to tab, not window)
User->>AppKit: leftMouseUp
AppKit->>sendEvent: dispatch
sendEvent->>Sequence: "defer - finishSequence - restore isMovable=false"
sendEvent->>AppKit: cmux_sendEvent
Note over User,Registry: Empty-chrome drag (window move path)
User->>AppKit: leftMouseDown on empty chrome
DragHandle->>Registry: containsWindowPoint? - false
DragHandle->>AppKit: withTemporaryWindowMovableEnabled
Note over DragHandle: isMovable=true temporarily
DragHandle->>AppKit: window.performDrag(with: event)
AppKit-->>User: window moves / tiling triggers
Note over DragHandle: defer restores isMovable=false
Reviews (12): Last reviewed commit: "Update Bonsplit minimal tab drag hit tes..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![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 (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
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/AppDelegate.swift (1)
14833-14873:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFinish the active suppression sequence even when the current event no longer maps to a suppression reason.
If a Bonsplit/tab drag starts inside the registered hit region and the
leftMouseUplands outside it,windowMoveSuppressionReasonForEvent(...)can returnnilon the terminating event. Lines 14833-14845 then bypassfinishWindowMoveSuppressionSequence(...), so the window can stay suppressed after the drag ends.Proposed fix
- guard let suppressionReason = windowMoveSuppressionReasonForEvent(window: self, event: event) else { + let suppressionReason = windowMoveSuppressionReasonForEvent(window: self, event: event) + let hasActiveSuppressionSequence = windowDragSuppressionDepth(window: self) > 0 + guard suppressionReason != nil || hasActiveSuppressionSequence else { `#if` DEBUG if event.type == .keyDown { folderGuardMs = (ProcessInfo.processInfo.systemUptime - folderGuardStart) * 1000.0 let originalDispatchStart = ProcessInfo.processInfo.systemUptime cmux_sendEvent(event) @@ - let shouldFinishSuppression = shouldFinishWindowMoveSuppressionSequenceAfterDispatch(window: self, event: event) + let shouldFinishSuppression = + hasActiveSuppressionSequence && + shouldFinishWindowMoveSuppressionSequenceAfterDispatch(window: self, event: event) @@ - cmuxDebugLog("window.sendEvent.\(finishedReason?.rawValue ?? suppressionReason.rawValue) finish nowMovable=\(isMovable)") + cmuxDebugLog("window.sendEvent.\(finishedReason?.rawValue ?? suppressionReason?.rawValue ?? "activeSequence") finish nowMovable=\(isMovable)") } else { - cmuxDebugLog("window.sendEvent.\(suppressionReason.rawValue) keepSuppressed nowMovable=\(isMovable)") + cmuxDebugLog("window.sendEvent.\(suppressionReason?.rawValue ?? "activeSequence") keepSuppressed nowMovable=\(isMovable)") } `#endif` } @@ - cmuxDebugLog("window.sendEvent.\(suppressionReason.rawValue) suppress=1 hit=\(hitDesc) movable=\(isMovable) depth=\(depth)") + cmuxDebugLog("window.sendEvent.\(suppressionReason?.rawValue ?? "activeSequence") suppress=1 hit=\(hitDesc) movable=\(isMovable) depth=\(depth)")🤖 Prompt for 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. In `@Sources/AppDelegate.swift` around lines 14833 - 14873, When windowMoveSuppressionReasonForEvent(window:event:) returns nil you still need to end any active suppression; in the early-return branch (the guard else) call shouldFinishWindowMoveSuppressionSequenceAfterDispatch(window:self, event:event) and if it returns true invoke finishWindowMoveSuppressionSequence(window: self) (and emit the same cmuxDebugLog path under DEBUG) before cmux_sendEvent(event) and return so suppression cannot remain active after the terminating event. Ensure you preserve the existing DEBUG timing code and logging behavior when adding this check.
🤖 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 `@cmuxTests/WindowAndDragTests.swift`:
- Around line 645-655: The test helper FakeBonsplitTabItemRegionView is
duplicated across WindowDragHandleHitTests and
WindowMoveSuppressionHitPathTests; extract this class into a single shared test
helper (e.g., a new file or a file-level extension) and replace both local
definitions with references to that single implementation. Locate the
FakeBonsplitTabItemRegionView declaration used in WindowDragHandleHitTests and
WindowMoveSuppressionHitPathTests, move it to a shared test-support location,
ensure it remains nonisolated(unsafe) with tabFrames,
containsBonsplitTabItemHit(localPoint:) and overridden hitTest(_:), and remove
the duplicate declaration from the other test file so both tests import/use the
shared helper.
---
Outside diff comments:
In `@Sources/AppDelegate.swift`:
- Around line 14833-14873: When
windowMoveSuppressionReasonForEvent(window:event:) returns nil you still need to
end any active suppression; in the early-return branch (the guard else) call
shouldFinishWindowMoveSuppressionSequenceAfterDispatch(window:self, event:event)
and if it returns true invoke finishWindowMoveSuppressionSequence(window: self)
(and emit the same cmuxDebugLog path under DEBUG) before cmux_sendEvent(event)
and return so suppression cannot remain active after the terminating event.
Ensure you preserve the existing DEBUG timing code and logging behavior when
adding this check.
🪄 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: 4de03d6f-529d-4e91-89c2-17a5db84c18f
📒 Files selected for processing (5)
Sources/App/ShortcutRoutingSupport.swiftSources/AppDelegate.swiftSources/WindowDragHandleView.swiftcmuxTests/WindowAndDragTests.swiftvendor/bonsplit
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?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/AppDelegate.swift`:
- Around line 14856-14861: The DEBUG block reimplements hit-testing against
contentView only, causing titlebar/theme-frame hits to be reported as nil and
misleading suppression diagnostics; replace the inline contentView hit-test in
the hitView initializer with the same hit-test helper used by cmux_sendEvent(_:)
(i.e., call the existing centralized hit-test function the event path uses) so
the DEBUG logging reflects the actual view resolution across the whole window
chrome, not just contentView. Ensure you reference the existing helper instead
of duplicating logic and keep the fallback nil behavior intact.
🪄 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: 8c2f130c-a54a-42be-ad46-8a0144598b3d
📒 Files selected for processing (1)
Sources/AppDelegate.swift
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Stale automated review from an older commit. The active suppression-sequence issue was fixed in 73583e5 and the latest CodeRabbit review did not report this as an open issue.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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/AppDelegate.swift (1)
15113-15160:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winStart the suppression sequence before dispatch.
Line 15113 computes a fresh suppression reason, but this path never begins/ensures the suppression sequence before
cmux_sendEvent(event). On the first Bonsplit/folder-drag event,finishWindowMoveSuppressionSequence(window:)has nothing to finish, so the window can still start moving while the drag is being dispatched. Call the begin/ensure helper as soon assuppressionReasonbecomes non-nil, before the active-sequence check.🤖 Prompt for 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. In `@Sources/AppDelegate.swift` around lines 15113 - 15160, The code computes suppressionReason via windowMoveSuppressionReasonForEvent but never starts the suppression sequence before dispatch, so on first folder-drag there is nothing for finishWindowMoveSuppressionSequence(window:) to finish; fix by calling the begin/ensure helper for the suppression sequence as soon as suppressionReason != nil (before checking activeWindowMoveSuppressionSequenceReason and before calling cmux_sendEvent), e.g. invoke your beginWindowMoveSuppressionSequence(window: self, reason: suppressionReason!) or ensureWindowMoveSuppressionSequence(window: self, reason: suppressionReason!) so the sequence is active for subsequent shouldFinishWindowMoveSuppressionSequenceAfterDispatch and finishWindowMoveSuppressionSequence logic.
🤖 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/WindowDragHandleView.swift`:
- Around line 299-366: The stale-recovery path still uses
clearWindowDragSuppression(...) which only decrements the suppression depth and
leaves the move-sequence associated object intact, so
activeWindowMoveSuppressionSequenceReason(window:) can remain set and keep a
window immovable; fix by unifying cleanup with
finishWindowMoveSuppressionSequence(window:): either have
clearWindowDragSuppression call finishWindowMoveSuppressionSequence(window:)
when a WindowMoveSuppressionSequenceState exists, or replicate
finishWindowMoveSuppressionSequence’s logic (read the
WindowMoveSuppressionSequenceState via objc_getAssociatedObject, clear that
associated object, call endWindowDragSuppression(window:), and call
restoreWindowDragging(window:previousMovableState:) with the saved
previousMovableState) so the associated reason is removed and window.isMovable
is restored.
---
Outside diff comments:
In `@Sources/AppDelegate.swift`:
- Around line 15113-15160: The code computes suppressionReason via
windowMoveSuppressionReasonForEvent but never starts the suppression sequence
before dispatch, so on first folder-drag there is nothing for
finishWindowMoveSuppressionSequence(window:) to finish; fix by calling the
begin/ensure helper for the suppression sequence as soon as suppressionReason !=
nil (before checking activeWindowMoveSuppressionSequenceReason and before
calling cmux_sendEvent), e.g. invoke your
beginWindowMoveSuppressionSequence(window: self, reason: suppressionReason!) or
ensureWindowMoveSuppressionSequence(window: self, reason: suppressionReason!) so
the sequence is active for subsequent
shouldFinishWindowMoveSuppressionSequenceAfterDispatch and
finishWindowMoveSuppressionSequence logic.
🪄 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: 4fb07899-b41c-4094-85b5-c0fabbec4443
📒 Files selected for processing (7)
Sources/App/CmuxMainWindow.swiftSources/App/ShortcutRoutingSupport.swiftSources/AppDelegate.swiftSources/ContentView.swiftSources/WindowDragHandleView.swiftcmuxTests/WindowAndDragTests.swiftvendor/bonsplit
Addressed in 2df920f: stale cleanup now routes active move-sequence state through finishWindowMoveSuppressionSequence before draining suppression depth, with coverage; the NSWindow event path now calls beginOrContinueWindowMoveSuppressionSequenceForEvent so sequence start/continuation is explicit before dispatch.
Fix pane tab width regression after #4290
This branch was successfully deployed
2 participants
Fixes #4289.
Summary
leftMouseDraggedand restoring only onleftMouseUp.f7a39b2b3037822b42c2cd149072279afbbaaadd, including near-titlebar tab-lane hit ownership and clear shared-backdrop active-tab fill.main, which includes the settings startup side-effect deferral that avoids recursive Ghostty/settings singleton initialization during managed appearance loading.Reproduction / validation
leftMouseDown, restoredwindow.isMovable, thenleftMouseDraggedmoved the window whiletab.dragStartwas active.main's deferred settings side-effect path.cloud-mac preflightis blocked on this machine by macfleet auth:No token stored for 'default'. Run macfleet login.Submodule note
mainmerge commitf7a39b2b3037822b42c2cd149072279afbbaaadd.Testing
CMUX_SKIP_ZIG_BUILD=1 ./scripts/reload.sh --tag issue-4289-minimal-mode-tab-drag