Skip to content

Fix minimal tab drag window movement #124

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
austinywang merged 12 commits into from
May 19, 2026
Merged

Fix minimal tab drag window movement #124

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
ce88069
Route tab-header zoom requests through host ownership
austinywang May 12, 2026
2979b6f
Mark tab zoom hook main actor
austinywang May 12, 2026
c44cc83
Keep tab selection instant with zoom gesture
austinywang May 12, 2026
207186b
test: cover action lane using trailing whitespace
austinywang May 13, 2026
ff55eda
fix: let action lane use empty tab bar space
austinywang May 13, 2026
e1d9954
test: keep overflow fixture crowded
austinywang May 13, 2026
b5dd1f0
Merge remote-tracking branch 'origin/issue-4110-surface-tab-bar-butto…
austinywang May 18, 2026
e1a9e76
Fix minimal tab drag window movement
austinywang May 18, 2026
f8233a4
Show cursor over minimal tab drag chrome
austinywang May 18, 2026
ba80421
Protect tab items from minimal chrome dragging
austinywang May 18, 2026
dc78f5c
Merge commit 'b5dd1f03993b70d3aa4b079e347d557323844e67'
austinywang May 19, 2026
02db30f
Clarify minimal tab drag hit region
austinywang May 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
101 changes: 95 additions & 6 deletions Sources/Bonsplit/Internal/Views/TabBarView.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,56 @@ public enum BonsplitTabBarHitRegionRegistry {
}
}

public protocol BonsplitTabItemHitRegionProviding: AnyObject {
func containsBonsplitTabItemHit(localPoint: NSPoint) -> Bool
}

public enum BonsplitTabItemHitRegionRegistry {
private static let lock = NSLock()
private static let registeredViews = NSHashTable<NSView>.weakObjects()

static func register(_ view: NSView) {
lock.lock()
registeredViews.add(view)
lock.unlock()
}

static func unregister(_ view: NSView) {
lock.lock()
registeredViews.remove(view)
lock.unlock()
}

private static func snapshot() -> [NSView] {
lock.lock()
let views = registeredViews.allObjects
lock.unlock()
return views
}

private static func isVisibleInHierarchy(_ view: NSView) -> Bool {
var current: NSView? = view
while let candidate = current {
guard !candidate.isHidden, candidate.alphaValue > 0 else { return false }
current = candidate.superview
}
return true
}

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
}
Comment on lines +86 to +97

@greptile-apps greptile-apps Bot May 18, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 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.

}

private struct SelectedTabFramePreferenceKey: PreferenceKey {
static let defaultValue: CGRect? = nil

Expand Down Expand Up @@ -329,6 +379,7 @@ enum TabBarStyling {
struct TabBarLayout: Equatable {
let barHeight: CGFloat
let availableWidth: CGFloat
let tabContentWidthExcludingSplitButtonLane: CGFloat?
let splitButtonCount: Int
let splitButtonLaneVisible: Bool
let reservesSplitButtonLane: Bool
Expand All @@ -337,13 +388,15 @@ struct TabBarLayout: Equatable {
init(
tabBarHeight: CGFloat,
availableWidth: CGFloat = 0,
tabContentWidthExcludingSplitButtonLane: CGFloat? = nil,
splitButtonCount: Int,
splitButtonLaneVisible: Bool,
reservesSplitButtonLane: Bool,
measuredSplitButtonLaneWidth: CGFloat = 0
) {
self.barHeight = max(1, tabBarHeight)
self.availableWidth = max(0, availableWidth)
self.tabContentWidthExcludingSplitButtonLane = tabContentWidthExcludingSplitButtonLane.map { max(0, $0) }
self.splitButtonCount = max(0, splitButtonCount)
self.splitButtonLaneVisible = splitButtonLaneVisible
self.reservesSplitButtonLane = reservesSplitButtonLane
Expand All @@ -362,7 +415,16 @@ struct TabBarLayout: Equatable {

var maximumSplitButtonLaneWidth: CGFloat {
guard availableWidth > 0 else { return fullSplitButtonLaneWidth }
return availableWidth * TabBarStyling.maximumSplitButtonLaneWidthFraction
let fractionLimit = availableWidth * TabBarStyling.maximumSplitButtonLaneWidthFraction
return max(fractionLimit, trailingWhitespaceBeforeSplitButtonLane)
}

var trailingWhitespaceBeforeSplitButtonLane: CGFloat {
guard availableWidth > 0,
let tabContentWidthExcludingSplitButtonLane else {
return 0
}
return max(0, availableWidth - tabContentWidthExcludingSplitButtonLane)
}

var visibleSplitButtonLaneWidth: CGFloat {
Expand Down Expand Up @@ -761,6 +823,7 @@ struct TabBarView: View {
@State private var dropLifecycle: TabDropLifecycle = .idle
@State private var scrollOffset: CGFloat = 0
@State private var contentWidth: CGFloat = 0
@State private var tabContentWidthExcludingSplitButtonLane: CGFloat?
@State private var containerWidth: CGFloat = 0
@State private var selectedTabFrameInBar: CGRect?
@State private var tabFramesInBar: [UUID: CGRect] = [:]
Expand Down Expand Up @@ -803,6 +866,7 @@ struct TabBarView: View {
TabBarLayout(
tabBarHeight: appearance.tabBarHeight,
availableWidth: containerWidth,
tabContentWidthExcludingSplitButtonLane: tabContentWidthExcludingSplitButtonLane,
splitButtonCount: visibleSplitButtons.count,
splitButtonLaneVisible: shouldShowSplitButtons,
reservesSplitButtonLane: showSplitButtons && !isMinimalMode,
Expand Down Expand Up @@ -944,13 +1008,11 @@ struct TabBarView: View {
GeometryReader { contentGeo in
Color.clear
.onChange(of: contentGeo.frame(in: .named("tabScroll"))) { _, newFrame in
scrollOffset = -newFrame.minX
contentWidth = newFrame.width
updateTabScrollContent(frame: newFrame)
}
.onAppear {
let frame = contentGeo.frame(in: .named("tabScroll"))
scrollOffset = -frame.minX
contentWidth = frame.width
updateTabScrollContent(frame: frame)
}
}
)
Expand Down Expand Up @@ -1047,6 +1109,7 @@ struct TabBarView: View {
}
.background(TabBarDragAndHoverView(
isMinimalMode: isMinimalMode,
tabFrames: Array(tabFramesInBar.values),
onDoubleClick: {
performNewTerminalSplitButtonAction()
},
Expand Down Expand Up @@ -1536,6 +1599,12 @@ struct TabBarView: View {
splitButtonContentWidth = frame.width
}

private func updateTabScrollContent(frame: CGRect) {
scrollOffset = -frame.minX
contentWidth = frame.width
tabContentWidthExcludingSplitButtonLane = max(0, frame.width - tabBarLayout.trailingTabContentInset)
}

@ViewBuilder
private var splitButtonRow: some View {
let tooltips = controller.configuration.appearance.splitButtonTooltips
Expand Down Expand Up @@ -2246,25 +2315,29 @@ private struct TabBarManualReorderTrackingView: NSViewRepresentable {
/// this view only receives hits in truly empty space.
private struct TabBarDragAndHoverView: NSViewRepresentable {
let isMinimalMode: Bool
let tabFrames: [CGRect]
let onDoubleClick: () -> Bool
let onHoverChanged: (Bool) -> Void

func makeNSView(context: Context) -> TabBarBackgroundNSView {
let view = TabBarBackgroundNSView()
view.isMinimalMode = isMinimalMode
view.tabFrames = tabFrames
view.onDoubleClick = onDoubleClick
view.onHoverChanged = onHoverChanged
return view
}

func updateNSView(_ nsView: TabBarBackgroundNSView, context: Context) {
nsView.isMinimalMode = isMinimalMode
nsView.tabFrames = tabFrames
nsView.onDoubleClick = onDoubleClick
nsView.onHoverChanged = onHoverChanged
}

final class TabBarBackgroundNSView: NSView {
final class TabBarBackgroundNSView: NSView, BonsplitTabItemHitRegionProviding {
var isMinimalMode = false
nonisolated(unsafe) var tabFrames: [CGRect] = []

@greptile-apps greptile-apps Bot May 18, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 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.

var onDoubleClick: (() -> Bool)?
var onHoverChanged: ((Bool) -> Void)?
private var hoverTrackingArea: NSTrackingArea?
Expand All @@ -2279,15 +2352,18 @@ private struct TabBarDragAndHoverView: NSViewRepresentable {
removeLocalMouseMonitor()
removeWindowObservers()
BonsplitTabBarHitRegionRegistry.unregister(self)
BonsplitTabItemHitRegionRegistry.unregister(self)
}

override func viewDidMoveToWindow() {
super.viewDidMoveToWindow()
BonsplitTabBarHitRegionRegistry.unregister(self)
BonsplitTabItemHitRegionRegistry.unregister(self)
removeWindowObservers()
if let window {
window.acceptsMouseMovedEvents = true
BonsplitTabBarHitRegionRegistry.register(self)
BonsplitTabItemHitRegionRegistry.register(self)
installWindowObservers()
installLocalMouseMonitorIfNeeded()
syncHoverStateToCurrentMouseLocation()
Expand All @@ -2301,9 +2377,15 @@ private struct TabBarDragAndHoverView: NSViewRepresentable {
super.viewDidMoveToSuperview()
if superview == nil {
BonsplitTabBarHitRegionRegistry.unregister(self)
BonsplitTabItemHitRegionRegistry.unregister(self)
}
}

nonisolated func containsBonsplitTabItemHit(localPoint: NSPoint) -> Bool {
let paddedFrames = tabFrames.map { $0.insetBy(dx: -2, dy: -2) }
return paddedFrames.contains { $0.contains(localPoint) }
}

override func updateTrackingAreas() {
super.updateTrackingAreas()
if let existing = hoverTrackingArea {
Expand Down Expand Up @@ -2338,6 +2420,13 @@ private struct TabBarDragAndHoverView: NSViewRepresentable {
super.mouseDown(with: event)
return
}
if containsBonsplitTabItemHit(localPoint: convert(event.locationInWindow, from: nil)) {
#if DEBUG
dlog("tab.bar.bg.mouseDown skipped reason=tabItem")
#endif
super.mouseDown(with: event)
return
}
if event.clickCount >= 2 {
if isMinimalMode {
let action = UserDefaults.standard.persistentDomain(forName: UserDefaults.globalDomain)?["AppleActionOnDoubleClick"] as? String
Expand Down
114 changes: 111 additions & 3 deletions Tests/BonsplitTests/BonsplitTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,34 @@ final class BonsplitTests: XCTestCase {
}
}

@MainActor
private final class FakeTabItemHitRegionView: NSView, BonsplitTabItemHitRegionProviding {
nonisolated(unsafe) var tabFrames: [CGRect] = []

deinit {
BonsplitTabItemHitRegionRegistry.unregister(self)
}

override func viewDidMoveToWindow() {
super.viewDidMoveToWindow()
BonsplitTabItemHitRegionRegistry.unregister(self)
if window != nil {
BonsplitTabItemHitRegionRegistry.register(self)
}
}

override func viewDidMoveToSuperview() {
super.viewDidMoveToSuperview()
if superview == nil {
BonsplitTabItemHitRegionRegistry.unregister(self)
}
}

nonisolated func containsBonsplitTabItemHit(localPoint: NSPoint) -> Bool {
tabFrames.contains { $0.contains(localPoint) }
}
}

@MainActor
private final class LayoutProbeView: NSView {
private(set) var sizeChangeCount = 0
Expand Down Expand Up @@ -422,6 +450,24 @@ final class BonsplitTests: XCTestCase {
XCTAssertTrue(layout.splitButtonLaneOverflowsViewport)
}

func testTabBarLayoutUsesTrailingWhitespaceBeforeClippingSplitButtons() {
let measuredWidth = TabBarStyling.splitButtonsBackdropWidth(buttonCount: 10)
let layout = TabBarLayout(
tabBarHeight: 28,
availableWidth: 930,
tabContentWidthExcludingSplitButtonLane: 300,
splitButtonCount: 10,
splitButtonLaneVisible: true,
reservesSplitButtonLane: true,
measuredSplitButtonLaneWidth: measuredWidth
)

XCTAssertEqual(layout.maximumSplitButtonLaneWidth, 630)
XCTAssertEqual(layout.visibleSplitButtonLaneWidth, measuredWidth)
XCTAssertEqual(layout.trailingTabContentInset, measuredWidth)
XCTAssertFalse(layout.splitButtonLaneOverflowsViewport)
}

func testTabBarLayoutKeepsMeasuredLaneWhenItFitsQuarterOfAvailableWidth() {
let layout = TabBarLayout(
tabBarHeight: 28,
Expand Down Expand Up @@ -736,6 +782,68 @@ final class BonsplitTests: XCTestCase {
)
}

@MainActor
func testTabItemHitRegionRegistryTracksOnlyRealTabFrames() {
let window = NSWindow(
contentRect: NSRect(x: 0, y: 0, width: 320, height: 200),
styleMask: [.titled, .closable],
backing: .buffered,
defer: false
)
defer { window.orderOut(nil) }
guard let contentView = window.contentView else {
XCTFail("Expected content view")
return
}

let tabBar = FakeTabItemHitRegionView(frame: NSRect(x: 20, y: 132, width: 220, height: 30))
tabBar.tabFrames = [
CGRect(x: 8, y: 0, width: 96, height: 30),
CGRect(x: 112, y: 0, width: 80, height: 30)
]
contentView.addSubview(tabBar)

let tabPoint = tabBar.convert(NSPoint(x: 32, y: 12), to: nil)
XCTAssertTrue(
BonsplitTabItemHitRegionRegistry.containsWindowPoint(tabPoint, in: window),
"Points inside actual tab frames should suppress implicit AppKit window dragging"
)

let emptyChromePoint = tabBar.convert(NSPoint(x: 204, y: 12), to: nil)
XCTAssertFalse(
BonsplitTabItemHitRegionRegistry.containsWindowPoint(emptyChromePoint, in: window),
"Empty tab-bar chrome should stay available for explicit app-window dragging"
)
}

@MainActor
func testTabItemHitRegionRegistryIgnoresHiddenProviders() {
let window = NSWindow(
contentRect: NSRect(x: 0, y: 0, width: 320, height: 200),
styleMask: [.titled, .closable],
backing: .buffered,
defer: false
)
defer { window.orderOut(nil) }
guard let contentView = window.contentView else {
XCTFail("Expected content view")
return
}

let tabBar = FakeTabItemHitRegionView(frame: NSRect(x: 20, y: 132, width: 220, height: 30))
tabBar.tabFrames = [CGRect(x: 8, y: 0, width: 96, height: 30)]
contentView.addSubview(tabBar)

let tabPoint = tabBar.convert(NSPoint(x: 32, y: 12), to: nil)
XCTAssertTrue(BonsplitTabItemHitRegionRegistry.containsWindowPoint(tabPoint, in: window))

tabBar.isHidden = true
XCTAssertFalse(
BonsplitTabItemHitRegionRegistry.containsWindowPoint(tabPoint, in: window),
"Hidden tab providers must not suppress app-window dragging"
)
}

@MainActor
func testConfigurationAcceptsCustomSplitButtonTooltips() {
let customTooltips = BonsplitConfiguration.SplitButtonTooltips(
Expand Down Expand Up @@ -3097,9 +3205,9 @@ final class BonsplitTests: XCTestCase {
showSplitButtons: true,
size: size,
configurePane: { pane in
let selected = TabItem(title: "", icon: nil)
pane.tabs = [selected]
pane.selectedTabId = selected.id
let tabs = (0..<8).map { _ in TabItem(title: "", icon: nil) }
pane.tabs = tabs
pane.selectedTabId = tabs.first?.id
}
) { hostingView in
maximumBrightness(
Expand Down
Loading