Repository navigation
Fix minimal tab drag window movement - #124
Conversation
cmux needs tab chrome zoom actions to run the same host-owned reconciliation used by shortcuts and context menus, instead of mutating Bonsplit zoom state directly from the SwiftUI tab item. Constraint: Tab header gesture ownership lives in Bonsplit while cmux owns portal visibility and focus reconciliation. Rejected: Toggle splitViewController.zoomedPaneId directly from double-click | it would bypass cmux workspace side effects. Confidence: medium Scope-risk: moderate Directive: Keep tab-chrome zoom requests routed through requestTabZoomToggle when hosts need side-effect ownership. Tested: git diff --check Not-tested: Local XCTest per cmux no-local-tests instruction
Tab chrome gestures are UI events and host zoom reconciliation runs on the main actor, so the public callback type now states that isolation explicitly. Constraint: Greptile review requested explicit confirmation of the Bonsplit-side actor annotation. Rejected: Rely only on BonsplitController class isolation | less clear at the stored closure boundary. Confidence: high Scope-risk: narrow Tested: git diff --check Not-tested: Local XCTest per cmux no-local-tests instruction
…ns-clip' into issue-3824-double-click-zoom-pane
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds an AppKit-scoped tab-item hit-region protocol and registry, threads tab-content width into TabBarLayout for split-button sizing, centralizes tab-scroll geometry updates, registers hit-region providers from tab items and background view, updates drag-zone cursor and hit-test behavior, and exposes a controller hook for tab-zoom requests. ChangesTab-item hit-region and split-button lane width
Sequence DiagramsequenceDiagram
participant Window
participant BonsplitTabItemHitRegionRegistry
participant TabBarBackgroundNSView
Window->>BonsplitTabItemHitRegionRegistry: containsWindowPoint(windowPoint, window)
BonsplitTabItemHitRegionRegistry->>TabBarBackgroundNSView: convertPointToLocal(windowPoint)
TabBarBackgroundNSView->>BonsplitTabItemHitRegionRegistry: containsBonsplitTabItemHit(localPoint) (Bool)
BonsplitTabItemHitRegionRegistry-->>Window: Bool (any provider hit)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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.
🧹 Nitpick comments (3)
Sources/Bonsplit/Internal/Views/TabBarView.swift (2)
54-98: 💤 Low valueConsider extracting shared registry logic.
BonsplitTabItemHitRegionRegistryduplicates nearly all ofBonsplitTabBarHitRegionRegistry(lines 5-48):register,unregister,snapshot,isVisibleInHierarchy, and the locking pattern. A generic base or shared helper could reduce this duplication.Given the localized scope and low immediate risk, this can be deferred.
🤖 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/Bonsplit/Internal/Views/TabBarView.swift` around lines 54 - 98, BonsplitTabItemHitRegionRegistry duplicates the locking/registry/snapshot/isVisibleInHierarchy logic found in BonsplitTabBarHitRegionRegistry; extract a shared helper (e.g., a BonsplitHitRegionRegistry type or protocol + concrete class) that owns the NSLock, NSHashTable<NSView>, and methods register(_:), unregister(_:), snapshot(), and isVisibleInHierarchy(_:), then replace the duplicated implementations in BonsplitTabItemHitRegionRegistry and BonsplitTabBarHitRegionRegistry to delegate to that shared helper (keep public containsWindowPoint(_:in:) behavior in each enum and cast to BonsplitTabItemHitRegionProviding / the bar equivalent as currently done).
2340-2340: 💤 Low value
nonisolated(unsafe)ontabFramesrelies on main-thread access pattern.This annotation bypasses Swift's isolation checking. The property is written from
updateNSView(main thread) and read fromcontainsBonsplitTabItemHit(also main thread via AppKit event handling). The pattern is safe in practice, but if the registry'scontainsWindowPointwere ever called from a background thread, this would race.Current usage appears correct.
🤖 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/Bonsplit/Internal/Views/TabBarView.swift` at line 2340, The use of nonisolated(unsafe) on tabFrames bypasses Swift concurrency checks and can race if accessed off the main thread; instead mark the property to enforce main-thread access (e.g., change the declaration to an `@MainActor` var tabFrames: [CGRect] = []) and remove nonisolated(unsafe), and ensure updateNSView and containsBonsplitTabItemHit (and any callers like registry.containsWindowPoint) access tabFrames on the main actor so reads/writes are serialized to the main thread.Tests/BonsplitTests/BonsplitTests.swift (1)
29-55: 💤 Low valueConsider immutability or document concurrency safety for the test double.
The
nonisolated(unsafe)annotation ontabFrames(line 31) allows concurrent access without synchronization. While this is required becausecontainsBonsplitTabItemHitisnonisolated, the mutable array creates a potential data race iftabFramesis modified while hit-testing runs concurrently.For test code with controlled access patterns this is acceptable, but consider either:
- Making
tabFrameseffectively immutable after initial setup (though this would require restructuring test setup)- Adding a comment documenting that tests must not mutate
tabFramesafter the view is registered- Using a thread-safe collection if concurrent access becomes necessary
🤖 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 `@Tests/BonsplitTests/BonsplitTests.swift` around lines 29 - 55, The test double FakeTabItemHitRegionView exposes a mutable nonisolated(unsafe) var tabFrames which can cause data races when containsBonsplitTabItemHit is called concurrently; change tabFrames to be immutable after setup (e.g., make it a let set once via initializer or a private(set) var that is only mutated before registering with BonsplitTabItemHitRegionRegistry) or add an explicit comment/documentation on FakeTabItemHitRegionView near tabFrames and in any test setup stating that tabFrames must not be mutated after the view is registered, so callers know the concurrency contract; ensure references to containsBonsplitTabItemHit and register/unregister usage remain consistent.
🤖 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.
Nitpick comments:
In `@Sources/Bonsplit/Internal/Views/TabBarView.swift`:
- Around line 54-98: BonsplitTabItemHitRegionRegistry duplicates the
locking/registry/snapshot/isVisibleInHierarchy logic found in
BonsplitTabBarHitRegionRegistry; extract a shared helper (e.g., a
BonsplitHitRegionRegistry type or protocol + concrete class) that owns the
NSLock, NSHashTable<NSView>, and methods register(_:), unregister(_:),
snapshot(), and isVisibleInHierarchy(_:), then replace the duplicated
implementations in BonsplitTabItemHitRegionRegistry and
BonsplitTabBarHitRegionRegistry to delegate to that shared helper (keep public
containsWindowPoint(_:in:) behavior in each enum and cast to
BonsplitTabItemHitRegionProviding / the bar equivalent as currently done).
- Line 2340: The use of nonisolated(unsafe) on tabFrames bypasses Swift
concurrency checks and can race if accessed off the main thread; instead mark
the property to enforce main-thread access (e.g., change the declaration to an
`@MainActor` var tabFrames: [CGRect] = []) and remove nonisolated(unsafe), and
ensure updateNSView and containsBonsplitTabItemHit (and any callers like
registry.containsWindowPoint) access tabFrames on the main actor so reads/writes
are serialized to the main thread.
In `@Tests/BonsplitTests/BonsplitTests.swift`:
- Around line 29-55: The test double FakeTabItemHitRegionView exposes a mutable
nonisolated(unsafe) var tabFrames which can cause data races when
containsBonsplitTabItemHit is called concurrently; change tabFrames to be
immutable after setup (e.g., make it a let set once via initializer or a
private(set) var that is only mutated before registering with
BonsplitTabItemHitRegionRegistry) or add an explicit comment/documentation on
FakeTabItemHitRegionView near tabFrames and in any test setup stating that
tabFrames must not be mutated after the view is registered, so callers know the
concurrency contract; ensure references to containsBonsplitTabItemHit and
register/unregister usage remain consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bc432a94-fc25-4e17-8ea0-cfc4157cf237
📒 Files selected for processing (2)
Sources/Bonsplit/Internal/Views/TabBarView.swiftTests/BonsplitTests/BonsplitTests.swift
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Greptile SummaryThis PR fixes the bug where clicking inside a real tab in minimal-mode windows would trigger implicit app-window dragging. It does so by introducing
Confidence Score: 4/5The core drag-suppression logic is correct and well-tested; the only concerns are concurrency annotations that mirror the pre-existing registry pattern. The functional change — checking tab frames before calling window.performDrag — is straightforward and the tests verify both the happy path and the hidden-provider edge case. The two concurrency observations (nonisolated(unsafe) on tabFrames and no @mainactor on containsWindowPoint) are real gaps but are consistent with how the existing BonsplitTabBarHitRegionRegistry is written; in practice all callers are AppKit event handlers on the main thread. Sources/Bonsplit/Internal/Views/TabBarView.swift — specifically BonsplitTabItemHitRegionRegistry.containsWindowPoint and TabBarBackgroundNSView.tabFrames regarding actor-isolation annotations. Important Files Changed
Sequence DiagramsequenceDiagram
participant SwiftUI as TabBarView (SwiftUI)
participant BG as TabBarBackgroundNSView
participant Reg as BonsplitTabItemHitRegionRegistry
participant Win as NSWindow
SwiftUI->>BG: updateNSView(tabFrames: [...])
Note over BG: tabFrames stored (nonisolated unsafe)
SwiftUI->>BG: viewDidMoveToWindow
BG->>Reg: register(self)
Win->>BG: mouseDown(event)
BG->>BG: convert(event.locationInWindow, from: nil) → localPoint
BG->>BG: containsBonsplitTabItemHit(localPoint)
alt localPoint inside a tab frame
BG->>BG: super.mouseDown(event) [no drag]
else empty chrome
BG->>Win: performDrag(with: event) [window drag]
end
Note over Reg: containsWindowPoint called by cmux
Reg->>BG: containsBonsplitTabItemHit(localPoint)
BG-->>Reg: true / false
Reviews (1): Last reviewed commit: "Fix minimal tab drag window movement" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| public static func containsWindowPoint(_ windowPoint: CGPoint, in window: NSWindow) -> Bool { | ||
| for view in snapshot() { | ||
| guard view.window === window, | ||
| isVisibleInHierarchy(view), | ||
| let provider = view as? BonsplitTabItemHitRegionProviding else { continue } | ||
| let localPoint = view.convert(windowPoint, from: nil) | ||
| if provider.containsBonsplitTabItemHit(localPoint: localPoint) { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
containsWindowPoint accesses @MainActor APIs without actor annotation
isVisibleInHierarchy and containsWindowPoint read view.window, view.superview, view.isHidden, view.alphaValue, and call view.convert(_:from:) — all @MainActor-isolated AppKit properties. The method carries no @MainActor annotation and is public, so callers in cmux or other consumers can invoke it from a non-main-actor context without a compiler warning. Combined with tabFrames being nonisolated(unsafe), a concurrent write during a background-thread call to containsWindowPoint would be an undetected data race. Adding @MainActor to the method (and to the parallel BonsplitTabBarHitRegionRegistry.containsWindowPoint) would close the gap without any behavioural change for existing callers, which are all AppKit event handlers.
| final class TabBarBackgroundNSView: NSView { | ||
| final class TabBarBackgroundNSView: NSView, BonsplitTabItemHitRegionProviding { | ||
| var isMinimalMode = false | ||
| nonisolated(unsafe) var tabFrames: [CGRect] = [] |
There was a problem hiding this comment.
nonisolated(unsafe) bypasses Swift concurrency safety for tabFrames
tabFrames is written from the main actor (via updateNSView, which runs on @MainActor) and read from nonisolated func containsBonsplitTabItemHit. The nonisolated(unsafe) annotation makes the Swift compiler accept this but provides no runtime protection. Since BonsplitTabItemHitRegionRegistry.containsWindowPoint is not @MainActor-constrained, a caller on a background actor could trigger a read while the main actor is updating tabFrames, producing a data race. Restricting containsWindowPoint to @MainActor (see sibling comment) would make this field safely accessible without the unsafe escape hatch.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Sources/Bonsplit/Internal/Views/TabBarView.swift (1)
107-109: 💤 Low valueConsider documenting the thread-safety assumption for
hitBounds.The
nonisolated(unsafe)annotation onhitBoundsrelies on the assumption thatcontainsBonsplitTabItemHitis only called from the main thread (via AppKit's hit testing). Consider adding a brief comment explaining this invariant for future maintainers.🤖 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/Bonsplit/Internal/Views/TabBarView.swift` around lines 107 - 109, The field hitBounds in RegionNSView is marked nonisolated(unsafe) which assumes containsBonsplitTabItemHit is only invoked on the main/AppKit thread; add a concise comment above the declaration of nonisolated(unsafe) private var hitBounds explaining this thread-safety invariant (that hitBounds is accessed only from the main thread via AppKit hit testing) and note any consequences or required call-site guarantees so future maintainers understand why the unsafe annotation is safe; reference RegionNSView, hitBounds, and containsBonsplitTabItemHit in the comment.
🤖 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.
Nitpick comments:
In `@Sources/Bonsplit/Internal/Views/TabBarView.swift`:
- Around line 107-109: The field hitBounds in RegionNSView is marked
nonisolated(unsafe) which assumes containsBonsplitTabItemHit is only invoked on
the main/AppKit thread; add a concise comment above the declaration of
nonisolated(unsafe) private var hitBounds explaining this thread-safety
invariant (that hitBounds is accessed only from the main thread via AppKit hit
testing) and note any consequences or required call-site guarantees so future
maintainers understand why the unsafe annotation is safe; reference
RegionNSView, hitBounds, and containsBonsplitTabItemHit in the comment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 93dd0149-8c7b-47d5-abef-caf1e8a23f22
📒 Files selected for processing (2)
Sources/Bonsplit/Internal/Views/TabBarView.swiftTests/BonsplitTests/BonsplitTests.swift
1 participant
Summary\n- Registers real Bonsplit tab frames separately from empty tab-bar chrome.\n- Prevents minimal-mode tab-bar background window dragging when the pointer is inside a real pane tab.\n- Adds coverage that tab frames suppress implicit app-window dragging while empty chrome stays draggable.\n\n## Testing\n- Not run locally; cmux policy runs tests in CI.\n\nNeeded by manaflow-ai/cmux#4290.
Summary by cubic
Fixes accidental window movement in minimal mode by blocking implicit window dragging over real tabs. Also routes tab‑header double‑click zoom through host-owned handling to keep workspace state in sync.
BonsplitTabItemHitRegionRegistry/BonsplitTabItemHitRegionProvidingand suppress minimal‑mode chrome dragging and double‑click over tabs, even before frame caches populate; hidden or unmounted providers are ignored.BonsplitController.requestTabZoomTogglewith optionalonTabZoomToggleRequest(@mainactor); single‑click selection stays instant.Written for commit 02db30f. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
New Features
Bug Fixes
Tests