From aa16fb9c0c2bf21f2c182fd0a1e17e97473314c8 Mon Sep 17 00:00:00 2001 From: austinpower1258 Date: Fri, 7 Aug 2026 01:44:09 -0700 Subject: [PATCH 1/2] fix: make tab drag monitor teardown synchronous --- .../Controllers/TabDragLifecycleMonitor.swift | 86 +++++++++++-------- Tests/BonsplitTests/BonsplitTests.swift | 3 +- 2 files changed, 52 insertions(+), 37 deletions(-) diff --git a/Sources/Bonsplit/Internal/Controllers/TabDragLifecycleMonitor.swift b/Sources/Bonsplit/Internal/Controllers/TabDragLifecycleMonitor.swift index 1191736e..74a7beaf 100644 --- a/Sources/Bonsplit/Internal/Controllers/TabDragLifecycleMonitor.swift +++ b/Sources/Bonsplit/Internal/Controllers/TabDragLifecycleMonitor.swift @@ -1,18 +1,41 @@ import AppKit /// Ground-truth exits for a tab drag that never reaches a SwiftUI drop callback. -/// Event callbacks enqueue cancellation after returning so AppKit can finish -/// delivering a real drop first; the session generation rejects stale callbacks. +/// AppKit delivers event-monitor callbacks on the main thread, so one synchronous +/// bridge ends the matching generation without a deferred cleanup race. @MainActor final class TabDragLifecycleMonitor { + private struct Registrations { + var appResignObserver: (any NSObjectProtocol)? + var keyDownMonitor: Any? + var localMouseUpMonitor: Any? + var globalMouseUpMonitor: Any? + + mutating func removeAll() { + if let appResignObserver { + NotificationCenter.default.removeObserver(appResignObserver) + self.appResignObserver = nil + } + if let keyDownMonitor { + NSEvent.removeMonitor(keyDownMonitor) + self.keyDownMonitor = nil + } + if let localMouseUpMonitor { + NSEvent.removeMonitor(localMouseUpMonitor) + self.localMouseUpMonitor = nil + } + if let globalMouseUpMonitor { + NSEvent.removeMonitor(globalMouseUpMonitor) + self.globalMouseUpMonitor = nil + } + } + } + private static let escapeKeyCode: UInt16 = 53 private let generation: Int private let onRequestEnd: @MainActor (Int) -> Void - private var appResignObserver: (any NSObjectProtocol)? - private var keyDownMonitor: Any? - private var localMouseUpMonitor: Any? - private var globalMouseUpMonitor: Any? + private var registrations = Registrations() private var endRequested = false private var isStopped = false @@ -21,55 +44,48 @@ final class TabDragLifecycleMonitor { self.onRequestEnd = onRequestEnd } + isolated deinit { + registrations.removeAll() + } + func start() { - appResignObserver = NotificationCenter.default.addObserver( + registrations.appResignObserver = NotificationCenter.default.addObserver( forName: NSApplication.didResignActiveNotification, object: nil, queue: .main ) { [weak self] _ in - self?.requestEnd() + self?.requestEndFromMainThreadCallback() } - keyDownMonitor = NSEvent.addLocalMonitorForEvents(matching: .keyDown) { [weak self] event in + registrations.keyDownMonitor = NSEvent.addLocalMonitorForEvents(matching: .keyDown) { [weak self] event in if event.keyCode == Self.escapeKeyCode { - self?.requestEnd() + self?.requestEndFromMainThreadCallback() } return event } - localMouseUpMonitor = NSEvent.addLocalMonitorForEvents(matching: .leftMouseUp) { [weak self] event in - self?.requestEnd() + registrations.localMouseUpMonitor = NSEvent.addLocalMonitorForEvents(matching: .leftMouseUp) { [weak self] event in + self?.requestEndFromMainThreadCallback() return event } - globalMouseUpMonitor = NSEvent.addGlobalMonitorForEvents(matching: .leftMouseUp) { [weak self] _ in - self?.requestEnd() + registrations.globalMouseUpMonitor = NSEvent.addGlobalMonitorForEvents(matching: .leftMouseUp) { [weak self] _ in + self?.requestEndFromMainThreadCallback() } } func stop() { guard !isStopped else { return } isStopped = true - if let appResignObserver { - NotificationCenter.default.removeObserver(appResignObserver) - self.appResignObserver = nil - } - if let keyDownMonitor { - NSEvent.removeMonitor(keyDownMonitor) - self.keyDownMonitor = nil - } - if let localMouseUpMonitor { - NSEvent.removeMonitor(localMouseUpMonitor) - self.localMouseUpMonitor = nil - } - if let globalMouseUpMonitor { - NSEvent.removeMonitor(globalMouseUpMonitor) - self.globalMouseUpMonitor = nil - } + registrations.removeAll() } - private nonisolated func requestEnd() { - Task { @MainActor [weak self] in - guard let self, !self.isStopped, !self.endRequested else { return } - self.endRequested = true - self.onRequestEnd(self.generation) + private nonisolated func requestEndFromMainThreadCallback() { + MainActor.assumeIsolated { + requestEnd() } } + + private func requestEnd() { + guard !isStopped, !endRequested else { return } + endRequested = true + onRequestEnd(generation) + } } diff --git a/Tests/BonsplitTests/BonsplitTests.swift b/Tests/BonsplitTests/BonsplitTests.swift index d50c22e0..d8d3cead 100644 --- a/Tests/BonsplitTests/BonsplitTests.swift +++ b/Tests/BonsplitTests/BonsplitTests.swift @@ -1736,7 +1736,7 @@ final class BonsplitTests: XCTestCase { } @MainActor - func testAppResignClearsTabDragLifecycle() async { + func testAppResignClearsTabDragLifecycleSynchronously() { let controller = SplitViewController() let pane = controller.focusedPane! let tab = pane.selectedTab! @@ -1752,7 +1752,6 @@ final class BonsplitTests: XCTestCase { name: NSApplication.didResignActiveNotification, object: nil ) - await Task.yield() XCTAssertNil( controller.tabDragSession, From 6a9b467125e23705b92987145be61efb0b563b7a Mon Sep 17 00:00:00 2001 From: austinpower1258 Date: Fri, 7 Aug 2026 01:47:32 -0700 Subject: [PATCH 2/2] fix: avoid isolated deinit runtime dependency --- .../Controllers/TabDragLifecycleMonitor.swift | 82 +++++++++++-------- 1 file changed, 48 insertions(+), 34 deletions(-) diff --git a/Sources/Bonsplit/Internal/Controllers/TabDragLifecycleMonitor.swift b/Sources/Bonsplit/Internal/Controllers/TabDragLifecycleMonitor.swift index 74a7beaf..be1cb866 100644 --- a/Sources/Bonsplit/Internal/Controllers/TabDragLifecycleMonitor.swift +++ b/Sources/Bonsplit/Internal/Controllers/TabDragLifecycleMonitor.swift @@ -5,37 +5,17 @@ import AppKit /// bridge ends the matching generation without a deferred cleanup race. @MainActor final class TabDragLifecycleMonitor { - private struct Registrations { - var appResignObserver: (any NSObjectProtocol)? - var keyDownMonitor: Any? - var localMouseUpMonitor: Any? - var globalMouseUpMonitor: Any? - - mutating func removeAll() { - if let appResignObserver { - NotificationCenter.default.removeObserver(appResignObserver) - self.appResignObserver = nil - } - if let keyDownMonitor { - NSEvent.removeMonitor(keyDownMonitor) - self.keyDownMonitor = nil - } - if let localMouseUpMonitor { - NSEvent.removeMonitor(localMouseUpMonitor) - self.localMouseUpMonitor = nil - } - if let globalMouseUpMonitor { - NSEvent.removeMonitor(globalMouseUpMonitor) - self.globalMouseUpMonitor = nil - } - } - } - private static let escapeKeyCode: UInt16 = 53 private let generation: Int private let onRequestEnd: @MainActor (Int) -> Void - private var registrations = Registrations() + // Registration tokens are mutated only by main-actor start/stop. Deinit + // reads them after the monitor's last owner has released it, when no + // actor-isolated access can remain in flight. + private nonisolated(unsafe) var appResignObserver: (any NSObjectProtocol)? + private nonisolated(unsafe) var keyDownMonitor: Any? + private nonisolated(unsafe) var localMouseUpMonitor: Any? + private nonisolated(unsafe) var globalMouseUpMonitor: Any? private var endRequested = false private var isStopped = false @@ -44,29 +24,34 @@ final class TabDragLifecycleMonitor { self.onRequestEnd = onRequestEnd } - isolated deinit { - registrations.removeAll() + deinit { + Self.removeRegistrations( + appResignObserver: appResignObserver, + keyDownMonitor: keyDownMonitor, + localMouseUpMonitor: localMouseUpMonitor, + globalMouseUpMonitor: globalMouseUpMonitor + ) } func start() { - registrations.appResignObserver = NotificationCenter.default.addObserver( + appResignObserver = NotificationCenter.default.addObserver( forName: NSApplication.didResignActiveNotification, object: nil, queue: .main ) { [weak self] _ in self?.requestEndFromMainThreadCallback() } - registrations.keyDownMonitor = NSEvent.addLocalMonitorForEvents(matching: .keyDown) { [weak self] event in + keyDownMonitor = NSEvent.addLocalMonitorForEvents(matching: .keyDown) { [weak self] event in if event.keyCode == Self.escapeKeyCode { self?.requestEndFromMainThreadCallback() } return event } - registrations.localMouseUpMonitor = NSEvent.addLocalMonitorForEvents(matching: .leftMouseUp) { [weak self] event in + localMouseUpMonitor = NSEvent.addLocalMonitorForEvents(matching: .leftMouseUp) { [weak self] event in self?.requestEndFromMainThreadCallback() return event } - registrations.globalMouseUpMonitor = NSEvent.addGlobalMonitorForEvents(matching: .leftMouseUp) { [weak self] _ in + globalMouseUpMonitor = NSEvent.addGlobalMonitorForEvents(matching: .leftMouseUp) { [weak self] _ in self?.requestEndFromMainThreadCallback() } } @@ -74,7 +59,16 @@ final class TabDragLifecycleMonitor { func stop() { guard !isStopped else { return } isStopped = true - registrations.removeAll() + Self.removeRegistrations( + appResignObserver: appResignObserver, + keyDownMonitor: keyDownMonitor, + localMouseUpMonitor: localMouseUpMonitor, + globalMouseUpMonitor: globalMouseUpMonitor + ) + appResignObserver = nil + keyDownMonitor = nil + localMouseUpMonitor = nil + globalMouseUpMonitor = nil } private nonisolated func requestEndFromMainThreadCallback() { @@ -88,4 +82,24 @@ final class TabDragLifecycleMonitor { endRequested = true onRequestEnd(generation) } + + private nonisolated static func removeRegistrations( + appResignObserver: (any NSObjectProtocol)?, + keyDownMonitor: Any?, + localMouseUpMonitor: Any?, + globalMouseUpMonitor: Any? + ) { + if let appResignObserver { + NotificationCenter.default.removeObserver(appResignObserver) + } + if let keyDownMonitor { + NSEvent.removeMonitor(keyDownMonitor) + } + if let localMouseUpMonitor { + NSEvent.removeMonitor(localMouseUpMonitor) + } + if let globalMouseUpMonitor { + NSEvent.removeMonitor(globalMouseUpMonitor) + } + } }