Conversation
…menu bar on device state changes
…actor related methods for improved thread safety
…on handling and MainActor annotations for thread safety
|
There was a problem hiding this comment.
Pull request overview
Adds runtime detection of multitouch HID device connect/disconnect events (e.g., Bluetooth Magic Trackpad after login) so MiddleDrag can start/restart monitoring and refresh the menu bar UI when hardware availability changes.
Changes:
- Introduces
HIDDeviceWatcher(IOKit/IOHIDManager-based) to watch digitizer-class device arrivals/removals with debouncing. - Integrates the watcher into
MultitouchManagerand posts a new.deviceConnectionStateChangednotification for UI refresh. - Adds unit tests for the watcher and for
MultitouchManager’s HID watcher integration; updates the Xcode project to include the new tests.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| MiddleDrag/Managers/HIDDeviceWatcher.swift | New IOHIDManager-based device watcher with debouncing and initial-enumeration suppression. |
| MiddleDrag/Managers/MultitouchManager.swift | Starts/stops watcher, reacts to connect/disconnect, and posts notification on successful late-start. |
| MiddleDrag/AppDelegate.swift | Observes .deviceConnectionStateChanged and refreshes the menu bar UI. |
| MiddleDrag/UI/MenuBarController.swift | Adds the new notification name constant. |
| MiddleDrag/MiddleDragTests/HIDDeviceWatcherTests.swift | New tests for watcher lifecycle/delegate/debounce constant assumptions. |
| MiddleDrag/MiddleDragTests/MultitouchManagerHIDTests.swift | New integration-style tests simulating watcher callbacks to validate late-start/restart behavior. |
| MiddleDrag.xcodeproj/project.pbxproj | Registers the new test files and adjusts test-target membership exceptions. |
Comments suppressed due to low confidence (1)
MiddleDrag/Managers/MultitouchManager.swift:158
- Marking
start()as@MainActoris a breaking change for synchronous callers: existing call sites (including unit tests) that invokestart()from non-@MainActorcontexts will become compile errors unless updated to@MainActor/await MainActor.run. Either update all call sites accordingly, or avoid actor-isolating this synchronous API (e.g., keep it nonisolated and internally dispatch to the main actor/queue).
@MainActor func start() {
guard !isMonitoring else { return }
applyConfiguration()
let eventTapSuccess = eventTapSetupFactory()
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| isRunning = true | ||
| startTime = ProcessInfo.processInfo.systemUptime | ||
| Log.info("HID device watcher started", category: .device) |
There was a problem hiding this comment.
startTime is set after IOHIDManagerOpen, but IOHIDManager can deliver initial matching callbacks immediately after scheduling/opening. If a callback arrives before startTime is assigned, elapsed will be computed from the default 0 and the initial-enumeration suppression won’t work (causing restarts on launch). Set startTime before registering/opening the manager (or initialize it at the top of start()) so early callbacks are properly ignored.
|
|
||
| /// Stop monitoring | ||
| func stop() { | ||
| @MainActor func stop() { |
There was a problem hiding this comment.
Marking stop() as @MainActor is a breaking change for synchronous callers: existing call sites (including unit tests) that invoke stop() from non-@MainActor contexts will become compile errors unless updated to @MainActor/await MainActor.run. Either update all call sites accordingly, or avoid actor-isolating this synchronous API (e.g., keep it nonisolated and internally dispatch to the main actor/queue).
| @MainActor func stop() { | |
| func stop() { |
| // If the removed device was the only one, DeviceMonitor.start() will | ||
| // fail and we'll gracefully go back to waiting for a new device. | ||
| if isMonitoring { | ||
| restart() | ||
| } |
There was a problem hiding this comment.
On HID device removal, this code triggers restart() but never posts .deviceConnectionStateChanged. If the removed device was the last one, restart() can transition the manager to disabled/not monitoring, but the menu bar UI won’t refresh because notifications are only posted in the “fresh start after connection” path. Consider posting .deviceConnectionStateChanged on disconnections as well (or centralize state-change notifications when isEnabled/isMonitoring changes due to device availability).
| final class MultitouchManagerHIDTests: XCTestCase { | ||
|
|
||
| // MARK: - Initial Start Failure Path | ||
|
|
||
| func testStartWithNoDevicesDoesNotMonitor() { |
There was a problem hiding this comment.
This test case calls MultitouchManager.start() / stop(), which are now @MainActor. Unless the test runs in a @MainActor context, these calls will be compile errors. Consider annotating the test class as @MainActor (as done in MenuBarControllerTests) or wrapping calls in await MainActor.run { … }.
| addSleepWakeObservers() | ||
|
|
||
| isMonitoring = true | ||
| isEnabled = true |
There was a problem hiding this comment.
attemptStartAfterDeviceConnection() unconditionally sets isEnabled = true on a successful late start. This can override an explicit user “Disabled” choice made while no compatible hardware was present (since toggleEnabled() can be used even when isMonitoring == false). Consider preserving the previous enabled state (similar to how restart() preserves wasEnabled) instead of forcing enabled to true on device connection.
| isEnabled = true |
| let result = IOHIDManagerOpen(manager, IOOptionBits(kIOHIDOptionsTypeNone)) | ||
| if result != kIOReturnSuccess { | ||
| Log.warning("Failed to open IOHIDManager: \(result)", category: .device) | ||
| return | ||
| } |
There was a problem hiding this comment.
HIDDeviceWatcher.start() returns early when IOHIDManagerOpen fails, but leaves hidManager scheduled on the main run loop with callbacks registered while isRunning remains false. Because stop() guards on isRunning, a later stop()/deinit will not unschedule/close the manager, which can leak resources and potentially allow callbacks to fire after the watcher is deallocated. Consider cleaning up (unschedule + close + nil out hidManager) on the failure path and/or making stop() clean up whenever hidManager is non-nil (even if isRunning is false).
…otations and update Swift version to 6.0
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| /// Unlike restart(), this handles the case where monitoring was never | ||
| /// successfully started (e.g., no devices at launch). | ||
| private func attemptStartAfterDeviceConnection() { | ||
| if isMonitoring { |
There was a problem hiding this comment.
Bug: A race condition between restart() and attemptStartAfterDeviceConnection() can cause resource leaks due to missing lock acquisition when modifying shared state like deviceMonitor.
Severity: MEDIUM
Suggested Fix
Ensure that attemptStartAfterDeviceConnection() acquires the restartLock before reading or modifying shared state variables like isMonitoring and deviceMonitor. This will prevent it from interleaving with an in-progress restart() operation, ensuring state consistency and preventing resource leaks.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: MiddleDrag/Managers/MultitouchManager.swift#L415
Potential issue: A race condition can occur between `restart()` (triggered by system
wake) and `attemptStartAfterDeviceConnection()` (triggered by a new device). Both
operate on the main queue but can interleave. `attemptStartAfterDeviceConnection()`
reads and modifies shared state like `isMonitoring` and `deviceMonitor` without
acquiring the `restartLock` that `restart()` uses. This can lead to an inconsistent
state where a `deviceMonitor` instance is created by
`attemptStartAfterDeviceConnection()` only to be overwritten by `performRestart()`
moments later, causing the first monitor's resources to leak.
Did we get this right? 👍 / 👎 to inform future reviews.
| Log.debug("HID device watcher ignoring initial enumeration: \(productName)", category: .device) | ||
| return | ||
| } |
There was a problem hiding this comment.
Bug: A new device connecting within 2 seconds of app launch will be ignored, as the code incorrectly assumes it's part of the initial device enumeration.
Severity: MEDIUM
Suggested Fix
Instead of a fixed time window, a more robust approach would be to track the devices reported during the initial enumeration phase from IOHIDManager. Once the initial enumeration is complete, any subsequent connection callback can be treated as a genuinely new device. This avoids incorrectly ignoring legitimate, fast-connecting devices post-launch.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: MiddleDrag/Managers/HIDDeviceWatcher.swift#L152-L154
Potential issue: The `HIDDeviceWatcher` uses a fixed 2-second window after startup to
ignore device connection events, assuming they are from the initial enumeration of
already-connected devices. However, a genuinely new Bluetooth device, such as a Magic
Trackpad being woken up, can connect within this 2-second window immediately after the
app launches at login. When this happens, the connection is incorrectly discarded as an
'initial enumeration' event, and the `MultitouchManager` is never notified. The
application will fail to detect the new device until the app is restarted.
Did we get this right? 👍 / 👎 to inform future reviews.
|
Will refactor fixes on a fresh branch |
This pull request introduces robust support for detecting multitouch HID device (e.g., Bluetooth Magic Trackpad) connections and disconnections after app launch, ensuring MiddleDrag can dynamically respond to hardware changes—especially when devices connect after login. It achieves this by adding a new
HIDDeviceWatcherclass, integrating it intoMultitouchManager, updating UI in response to device state changes, and providing comprehensive unit tests for the new watcher.Device connection detection and management:
HIDDeviceWatcherclass, which uses IOKit to monitor for multitouch HID device connections/disconnections. It handles debouncing, ignores initial enumeration events, and notifies its delegate of device state changes.HIDDeviceWatcherintoMultitouchManager, starting and stopping the watcher as needed. When a new device is detected, the manager attempts to start or restart monitoring, ensuring the app responds to devices connected after launch. Also implemented the delegate methods for connection/disconnection events. [1] [2] [3] [4]AppDelegateto listen for.deviceConnectionStateChangednotifications and refresh the menu bar UI when the device state changes, providing immediate feedback to the user. [1] [2]Testing and project setup:
HIDDeviceWatcherinHIDDeviceWatcherTests.swift, covering lifecycle, delegate wiring, debouncing constants, and memory management.project.pbxproj) to include the new test file and ensure proper test target membership and folder exception handling. [1] [2] [3]