Allow enabling/disabling device sources and known devices#92
Draft
heavyrubberslave wants to merge 21 commits into
Draft
Allow enabling/disabling device sources and known devices#92heavyrubberslave wants to merge 21 commits into
heavyrubberslave wants to merge 21 commits into
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughDevice and device-source enablement is added to settings. Device detection, provider lifecycle, observer ownership, and application reload flows are generalized around detection metadata and ChangesDevice enablement and provider lifecycle
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
heavyrubberslave
commented
Jul 5, 2026
heavyrubberslave
commented
Jul 5, 2026
heavyrubberslave
commented
Jul 5, 2026
Adds an optional `enabled` flag (defaults to true) to both `deviceSources` and `knownDevices` entries in the settings JSON. - DeviceSource / KnownDevice gain an `isEnabled()` accessor. - DeviceProviderManager can now hot-reload: it starts/stops individual device source providers as they are enabled/disabled/added/removed, instead of only loading them once at startup. Reload calls are serialized internally to avoid races when settings change in quick succession (e.g. disable immediately followed by re-enable). - SettingsManager's 'settingsChanged' event now triggers DeviceProviderManager.reload(), so editing settings (e.g. via PUT /settings) dynamically starts/stops device sources. - SerialDeviceProvider/BleDeviceProvider/ButtplugIoWebsocketDeviceProvider skip connecting to newly detected devices that are known but disabled, and close already-connected devices that become disabled. Devices that were skipped while disabled are retried once they become enabled again (via the new DeviceProvider.onSettingsChanged hook). - VirtualDeviceProvider's periodic discovery loop now also honors the enabled flag of virtual known devices. Closes #70
heavyrubberslave
force-pushed
the
feat/device-source-enable-disable
branch
from
July 16, 2026 20:47
0ba9ffc to
2586e6f
Compare
DeviceManager is now the single authoritative place deciding whether a known device may connect. It owns the enabled check, the pending-retry map for disabled devices, and closing devices that get disabled at runtime, exposed via isDeviceEnabled(), addDevice(deviceInfo, device) and onSettingsChanged(). Providers no longer duplicate this: BLE, serial and buttplug.io all route detection through announceDetectedDevice()/deviceDetected and just react to addDevice()'s result. The per-provider onSettingsChanged() hook is gone. VirtualDeviceProvider now reacts to settings changes instead of polling, dropping the scanIntervalMs config entirely.
revokeDetectedDevice() now also drops the device from the pending-retry map, so a disabled device that physically disappears is not resurrected when its known device is later re-enabled. Buttplug's removeButtplugIoDevice now revokes not-yet-connected devices instead of just logging a warning. VirtualDeviceProvider's detection listener lifecycle now matches the serial provider (registered in init(), removed in stop()), so the stopped flag is only needed for the genuinely async device-creation window.
Extract the shared acquire -> create -> addDevice -> release flow that BLE, serial and buttplug.io all implemented near-identically into a new DetectedDeviceProvider base. Subclasses now only implement supportsDeviceInfo() and createDevice(), plus an optional onConnectFailed() hook, and get the connected-device bookkeeping and stop() for free. Buttplug.io keys its connected devices by their final DeviceId (via the factory's computeDeviceId) instead of the Intiface index, so it no longer needs a separate index map. Align ButtplugIoDevice.setAttribute with the single-type-parameter shape used by EStim2bDevice so the concrete device satisfies the base class' Device constraint.
…ries Add AnyDevice = Omit<Device, 'setAttribute'> & wide setAttribute. Concrete devices are not assignable to Device<...> because their narrowing setAttribute override trips method parameter bivariance; erasing that one method and re-adding a wide, string-keyed version restores assignability while still requiring the full remaining Device surface (so non-devices are rejected). Use AnyDevice everywhere a concrete device type is irrelevant - the device manager, repository, updater and automation runtime - which lets addDevice drop its <TAttrs, TNotifications, TConfig> generics and collapses the device providers from five type parameters to a single D extends AnyDevice. The concrete devices keep their strict setAttribute for their own call sites. Removes the now-unused Infer* helper families.
The value parameter and return of a device's setAttribute are always an AttributeValue, so use that in the erased AnyDevice signature rather than unknown. Concrete devices remain assignable and callers at the AnyDevice boundaries (automation runtime, generic updater) now get a real value type.
The AnyDevice erasure made all device families structurally identical (their transport members are private/protected, so Omit strips their nominal identity), which let a BleDeviceProvider be parameterized with a non-BLE device. Expose a distinguishing public member per family - getPeripheral() on BleDevice, getTransport() on PeripheralDevice - and define AnyBleDevice / AnyPeripheralDevice erasing setAttribute like AnyDevice but retaining that member. Constrain the two providers on those, so the wrong device family is rejected again while keeping the single-generic, bivariance-free flow.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (1)
src/device/protocol/virtual/virtualDeviceProvider.ts (1)
108-119: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRetain failed virtual-device attempts across discovery cycles.
Releasing the acquisition queue after factory failure lets every later settings event announce and initialize the same broken device again. Keep failed attempts tracked until their configuration is removed or deliberately reset.
Based on learnings, internal virtual-provider maps should track attempted devices, including failed initialization attempts, to prevent repeated work on later discovery cycles.
Also applies to: 164-166
🤖 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 `@src/device/protocol/virtual/virtualDeviceProvider.ts` around lines 108 - 119, Update the virtual-device tracking used by the discovery loop around virtualDevices and connectedDevices so every initialization attempt, including factory failures, remains marked as attempted. Prevent announceDetectedDevice from re-announcing attempted devices on later settings or discovery cycles, while removing the marker only when the device configuration is removed or explicitly reset.Source: Learnings
🤖 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 `@src/device/deviceManager.ts`:
- Around line 197-199: Update src/device/deviceManager.ts lines 197-199 in
registerPendingRetry to retain both the detected device ID and
device.getDeviceId(), and make onSettingsChanged check enablement using the
canonical ID. Update tests/unit/device/deviceManager.spec.ts lines 502-528 to
use distinct detected and canonical IDs and verify no retry occurs until the
canonical device is enabled.
In `@src/device/protocol/buttplugIo/buttplugIoWebsocketDeviceProvider.ts`:
- Around line 70-78: Update ButtplugIoWebsocketDeviceProvider.stop() to
disconnect the Buttplug websocket client and remove its connection listeners
before or during shutdown, preventing handleLostConnection() from reinitializing
the provider after stop. Preserve the existing interval cleanup and super.stop()
device cleanup.
In `@src/device/provider/detectedDeviceProvider.ts`:
- Around line 79-88: Update the connection-failure handling around
onConnectFailed so transport cleanup completes before another provider can claim
the device. Move releaseDetectedDevice into a finally block that runs after
onConnectFailed for both the caught-error and undefined-device paths, preserving
the existing failure notification and return behavior.
- Around line 44-50: Update DetectedDeviceProvider.stop and the device-detection
handler to coordinate shutdown with in-flight work: set a stopping flag before
removing the listener, and after each awaited acquisition or device-creation
step, check that flag and close/release the resulting resource instead of
registering it. Ensure stop closes all currently connected devices and clears
the map, preventing any handler that began before stop from leaving a device
active afterward.
- Around line 96-103: Cancel in-flight device creation before completed devices
are registered. In src/device/provider/detectedDeviceProvider.ts:96-103, require
an active acquisition claim before registering through addDevice and
connectedDevices. In
src/device/protocol/buttplugIo/buttplugIoWebsocketDeviceProvider.ts:169-176,
revoke or invalidate pending creation when the device is removed. In
src/device/protocol/virtual/virtualDeviceProvider.ts:101-119, track configured
devices being created and revoke them on removal; in
src/device/protocol/virtual/virtualDeviceProvider.ts:142-161, revalidate current
settings before adding the completed virtual device.
In `@src/device/provider/deviceProviderManager.ts`:
- Around line 74-80: Update the provider lifecycle methods around
doInitProviders and doStopProviders so provider-map mutations occur only after
successful lifecycle operations: add providers only after init() succeeds and
remove any failed initialization entry so later reloads retry it; in stop
handling, delete each provider only after its stop() succeeds, retain
failed-to-stop entries, and propagate the stop failure instead of clearing the
entire map.
In `@src/settings/serializedTypes.ts`:
- Around line 10-17: Make the enabled property optional in the serialized
payload type declarations, including SerializedDeviceSource and the nearby
serialized type with the same field. Keep the property’s boolean type while
allowing legacy serialized objects to omit it, so consumers handle missing
values explicitly.
In `@tests/unit/device/deviceManager.spec.ts`:
- Around line 502-528: Update the test around DeviceManager.addDevice and
onSettingsChanged to use different detected and canonical device IDs: keep
deviceInfo.id as the detected ID while assigning the TestDevice a distinct
canonical ID. Verify that changing unrelated settings does not re-announce or
retry the pending device while the canonical known device remains disabled, then
enable that canonical device and retain the expected deviceDetected assertion.
In `@tests/unit/device/provider/deviceProviderManager.spec.ts`:
- Around line 154-168: Update the test around
DeviceProviderManager.stopProviders to reload the existing manager after
stopping it, rather than creating manager2. Use a fresh provider factory setup
as needed, then assert the subsequent reload initializes a new provider, proving
stopProviders clears the original manager’s internal state.
---
Nitpick comments:
In `@src/device/protocol/virtual/virtualDeviceProvider.ts`:
- Around line 108-119: Update the virtual-device tracking used by the discovery
loop around virtualDevices and connectedDevices so every initialization attempt,
including factory failures, remains marked as attempted. Prevent
announceDetectedDevice from re-announcing attempted devices on later settings or
discovery cycles, while removing the marker only when the device configuration
is removed or explicitly reset.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3a6df50d-3922-4daf-99ed-bd10e65aa9db
📒 Files selected for processing (35)
src/app.tssrc/automation/scriptRuntime.tssrc/device/bleDevice.tssrc/device/device.tssrc/device/deviceManager.tssrc/device/genericDeviceUpdater.tssrc/device/peripheralDevice.tssrc/device/protocol/airotic/airoticDeviceProvider.tssrc/device/protocol/buttplugIo/buttplugIoDevice.tssrc/device/protocol/buttplugIo/buttplugIoDeviceFactory.tssrc/device/protocol/buttplugIo/buttplugIoWebsocketDeviceProvider.tssrc/device/protocol/virtual/virtualDeviceProvider.tssrc/device/protocol/virtual/virtualDeviceProviderFactory.tssrc/device/provider/bleDeviceProvider.tssrc/device/provider/detectedDeviceProvider.tssrc/device/provider/deviceProviderManager.tssrc/device/provider/serialDeviceProvider.tssrc/device/updater/abstractDeviceUpdater.tssrc/device/updater/bufferedDeviceUpdater.tssrc/device/updater/deviceUpdaterInterface.tssrc/entity/deviceList.tssrc/repository/connectedDeviceRepository.tssrc/repository/deviceRepositoryInterface.tssrc/serviceProvider/deviceServiceProvider.tssrc/settings/deviceSource.tssrc/settings/knownDevice.tssrc/settings/serializedTypes.tssrc/settings/settings.tssrc/settings/settingsManager.tstests/integration/deviceEvents.spec.tstests/integration/helpers/appHelper.tstests/unit/device/deviceManager.spec.tstests/unit/device/provider/deviceProviderManager.spec.tstests/unit/settings/deviceSource.spec.tstests/unit/settings/knownDevice.spec.ts
Now that AnyDevice.getAttribute()/setAttribute expose AttributeValue, the device-value locals in the deviceEvents integration test and the StubDevice setAttribute recorder can use AttributeValue instead of unknown.
- deviceManager: gate a pending-retry on the device's canonical id (known only after connecting) instead of the preliminary detection id, so a disabled device isn't retried on every unrelated settings change. - buttplug provider: stop() now removes the client's listeners and disconnects, so an in-flight disconnect can't reinitialize the provider after shutdown. - DetectedDeviceProvider: guard against a detection that completes after stop(), and run transport cleanup (onConnectFailed) before releasing the acquire claim so the next provider can't overlap the previous transport. - deviceProviderManager: only mutate the provider map after a lifecycle operation succeeds - keep failed-to-stop providers, drop half-started ones - without aborting the rest of the reload. - KnownDevice/DeviceSource: serialize the normalized 'enabled' getter (toPlainOnly) while still reading the raw field (toClassOnly), so the serialized payload always carries a real boolean for legacy entries. - Tests: exercise distinct detected/canonical ids and reuse the same manager to prove stopProviders() clears its state.
…gh it Now that VirtualDeviceProvider also discovers via announceDetectedDevice(), every provider goes through the detection pipeline, leaving DeviceProvider with a single subclass. Fold the two base classes into one: - VirtualDeviceProvider extends the common base and drops its duplicated detection flow (listener, handleDeviceDetection, connected-device map, stopped guard) - keeping only its settings-driven discovery. - DetectedDeviceProvider is removed; DeviceProvider now owns the pipeline. - No generic defaults anywhere in the provider hierarchy, so every subclass must deliberately declare the DeviceInfo/Device it supports. - Add AnyDeviceProvider = DeviceProvider<DeviceInfo, AnyDevice> for the type-agnostic boundaries (manager + factories). - Delete the unused TestDeviceProvider test double.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@src/device/protocol/virtual/virtualDeviceProvider.ts`:
- Around line 99-104: Update the virtual device announcement flow around the
virtualDevices iteration and initialization tracking to record every attempted
device ID, including failed creations, before announcing or retrying it. Prevent
unrelated settings changes from re-announcing already attempted IDs, while
preserving announcements for newly configured devices. Remove an ID from the
attempted set only when its configuration entry is removed.
- Around line 93-96: Update the reconciliation loop over getConnectedDevices in
the virtual device provider so each device.close failure is caught and logged,
allowing subsequent removed devices to be processed and newly configured device
discovery to continue. Keep the existing close behavior for successful devices
and preserve the loop’s device filtering.
In `@src/device/provider/deviceProvider.ts`:
- Around line 143-145: Ensure detection claims are always released: in
src/device/provider/deviceProvider.ts lines 143-145, update abortDetection() to
run releaseDetectedDevice() in a finally block around onConnectFailed(); in
src/device/provider/deviceProvider.ts lines 114-118, invoke abortDetection()
from a finally block around device.close() so cleanup rejection cannot bypass
release.
- Around line 65-73: Update DeviceProvider.stop so a device.close rejection does
not leave a retained provider permanently inactive: make the stopped state and
deviceDetectedListener detachment transactional or roll them back when cleanup
fails, while preserving cleanup of connected devices. Ensure
DeviceProviderManager can subsequently re-enable the source by either making
stop recoverable or removing terminal providers after failure.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 90dfb15b-4428-4b9b-961e-f156506718dd
📒 Files selected for processing (10)
src/device/protocol/buttplugIo/buttplugIoWebsocketDeviceProvider.tssrc/device/protocol/virtual/virtualDeviceProvider.tssrc/device/provider/bleDeviceProvider.tssrc/device/provider/deviceProvider.tssrc/device/provider/deviceProviderFactory.tssrc/device/provider/deviceProviderManager.tssrc/device/provider/genericDeviceProviderFactory.tssrc/device/provider/serialDeviceProvider.tstests/unit/device/provider/deviceProviderManager.spec.tstests/unit/device/testDeviceProvider.ts
💤 Files with no reviewable changes (1)
- tests/unit/device/testDeviceProvider.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/unit/device/provider/deviceProviderManager.spec.ts
- src/device/protocol/buttplugIo/buttplugIoWebsocketDeviceProvider.ts
- src/device/provider/deviceProviderManager.ts
Comment on lines
+93
to
96
| for (const device of [...this.getConnectedDevices()]) { | ||
| if (!virtualDevices.has(device.getDeviceId)) { | ||
| await device.close(); | ||
| this.attemptedDevices.delete(knowDevice.id); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not abort the entire reconciliation when one device fails to close.
A rejected device.close() prevents remaining removed devices from closing and skips discovery of newly configured devices. Catch and log each close failure, then continue processing.
Proposed fix
for (const device of [...this.getConnectedDevices()]) {
if (!virtualDevices.has(device.getDeviceId)) {
- await device.close();
+ try {
+ await device.close();
+ } catch (e: unknown) {
+ logError(this.logger, `Failed to close removed virtual device '${device.getDeviceId}'`, e);
+ }
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for (const device of [...this.getConnectedDevices()]) { | |
| if (!virtualDevices.has(device.getDeviceId)) { | |
| await device.close(); | |
| this.attemptedDevices.delete(knowDevice.id); | |
| return; | |
| } | |
| for (const device of [...this.getConnectedDevices()]) { | |
| if (!virtualDevices.has(device.getDeviceId)) { | |
| try { | |
| await device.close(); | |
| } catch (e: unknown) { | |
| logError(this.logger, `Failed to close removed virtual device '${device.getDeviceId}'`, e); | |
| } | |
| } |
🤖 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 `@src/device/protocol/virtual/virtualDeviceProvider.ts` around lines 93 - 96,
Update the reconciliation loop over getConnectedDevices in the virtual device
provider so each device.close failure is caught and logged, allowing subsequent
removed devices to be processed and newly configured device discovery to
continue. Keep the existing close behavior for successful devices and preserve
the loop’s device filtering.
Comment on lines
+99
to
+104
| // Announce all currently configured devices - the device manager takes care of skipping | ||
| // disabled ones (and re-announcing them once re-enabled) as well as ones already connected. | ||
| for (const knownDevice of virtualDevices.values()) { | ||
| const deviceInfo: VirtualDeviceInfo = { type: 'virtual', id: knownDevice.id, knownDevice }; | ||
|
|
||
| try { | ||
| await device.close(); | ||
| } finally { | ||
| this.connectedDevices.delete(deviceId); | ||
| this.attemptedDevices.delete(deviceId); | ||
| this.deviceManager.announceDetectedDevice(deviceInfo); |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
Retain attempted virtual devices after failed initialization.
When creation fails, the claim is released; every unrelated settings change then announces the same device and retries initialization. Track attempted IDs, including failures, and clear them only when the configuration entry is removed.
Based on learnings, virtual providers must retain both successful and failed initialization attempts to avoid repeated work.
🤖 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 `@src/device/protocol/virtual/virtualDeviceProvider.ts` around lines 99 - 104,
Update the virtual device announcement flow around the virtualDevices iteration
and initialization tracking to record every attempted device ID, including
failed creations, before announcing or retrying it. Prevent unrelated settings
changes from re-announcing already attempted IDs, while preserving announcements
for newly configured devices. Remove an ID from the attempted set only when its
configuration entry is removed.
Source: Learnings
Comment on lines
65
to
+73
| public async stop(): Promise<void> { | ||
| this.stopped = true; | ||
|
|
||
| this.deviceManager.off(DeviceManagerEvent.deviceDetected, this.deviceDetectedListener); | ||
|
|
||
| for (const device of this.connectedDevices.values()) { | ||
| await device.close(); | ||
| } | ||
| this.connectedDevices.clear(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Keep failed shutdowns recoverable.
Line 66 marks the provider stopped and Line 68 detaches detection before a device close can reject. DeviceProviderManager retains providers whose stop fails, so re-enabling that source finds a retained—but permanently inactive—provider and does not create a replacement.
Make shutdown transactional/recoverable, or align the manager contract so terminal providers are removed even when device cleanup reports errors.
🤖 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 `@src/device/provider/deviceProvider.ts` around lines 65 - 73, Update
DeviceProvider.stop so a device.close rejection does not leave a retained
provider permanently inactive: make the stopped state and deviceDetectedListener
detachment transactional or roll them back when cleanup fails, while preserving
cleanup of connected devices. Ensure DeviceProviderManager can subsequently
re-enable the source by either making stop recoverable or removing terminal
providers after failure.
…mers The provider previously ran two independent guessed timers: kick off a scan every 60s, and blindly stop it 30s later. Both numbers were arbitrary and ignored what the server was actually doing. A buttplug.io scan is a bounded operation - the server ends it on its own and reports back via the 'scanningfinished' event. Use that event to drive the loop: start a scan on connect, and when the server reports the scan finished, schedule the next one after a short cooldown. This removes the guessed scan duration entirely and syncs the cadence to real server state instead of a fixed clock. - Replace AUTO_SCAN_INTERVAL_MS/SCAN_DURATION_MS with a single RESCAN_COOLDOWN_MS, and name the connect-retry interval too. - Cooldown is injectable (defaulting to the constant) so tests can exercise the re-scan loop without waiting the full 30s. - Simulator tracks StartScanning requests and exposes waitForScanCount(); the buttplug lifecycle app now runs with autoScan enabled so the loop is actually covered.
…fixed timers" This reverts commit 00088ea.
Keep the existing StartScanning/StopScanning duty-cycle (it matches how the desktop websocket server actually behaves - it scans until told to stop), but pull the three magic numbers out into named constants so their intent is clear: CONNECT_RETRY_INTERVAL_MS, AUTO_SCAN_INTERVAL_MS and SCAN_DURATION_MS. The scan interval and duration are also injectable via the constructor (defaulting to those constants) and threaded through the provider factory config, so tests can shrink them without waiting the full 60s/30s. Adds a dedicated auto-scanning test that constructs the provider directly against the simulator - no full app, so it neither touches the shared BLE teardown nor lets its repeated scanning interfere with the lifecycle suite. The simulator now records StartScanning/StopScanning counts and no longer echoes ScanningFinished, modelling the desktop server that scans until stopped.
The dedicated auto-scan test surfaced a pre-existing flake in the 'device refreshes' test that is worth investigating on its own, so remove the new test (and the injectable interval/duration params + simulator scan-tracking that only it used) for now. The named scan-timing constants stay. The refresh flake will be looked at in a separate PR.
…anonical device ids The detection-time device id and the final/canonical device id (device.getDeviceId, which can differ for protocols that only learn their real id post-handshake, e.g. zc95 firmware >=2.0) were both loosely called 'id', with only the retry-bookkeeping field carrying a distinguishing name (enablementId) - backwards, since that field held the more authoritative of the two. Rename for clarity: - DeviceInfo -> DeviceDetectionInfo (and its BleDeviceInfo/SerialDeviceInfo/ ButtplugIoDeviceInfo/VirtualDeviceInfo variants -> ...DeviceDetectionInfo), matching the vocabulary already used elsewhere (announceDetectedDevice, acquireDetectedDevice, detectedDeviceAcquireQueue, deviceDetected event). - DeviceDetectionInfo.id -> detectionId, naming it for what it is: the id assigned at detection time, before canonical identity may be resolved. - enablementId -> canonicalId, naming it for what it is: the device's final, authoritative id. Pure identifier rename, no logic changes. The unrelated DeviceInfo type in slvCtrlProtocol.ts (wire-protocol handshake payload) is untouched.
…lves on removal BLE/serial observers were previously started unconditionally at app boot regardless of whether any matching device source was even configured, wasting BLE radio/USB polling resources. They're now started/stopped by the individual providers that need them (BleDeviceProvider/SerialDeviceProvider), reference-counted so multiple providers sharing the same observer (e.g. zc95Serial + estim2bSerial + slvCtrlPlusSerial all sharing one SerialPortObserver) don't double-start or stop it out from under each other. Fixed a latent bug this surfaced along the way: AiroticDeviceProvider.init() overrode the base method without calling super.init(), which would have silently skipped starting the BLE observer entirely. app.ts's loadDeviceProviders()/shutdown() no longer touch the observers directly - that's now fully driven by DeviceProviderManager calling each provider's init()/stop(). Separately, ButtplugIoDevice now closes itself when the buttplug.io server reports it removed (while the overall connection stays up), listening directly to its own ButtplugClientDevice's 'deviceremoved' event - mirroring how BleDevice/PeripheralDevice detect their own physical disconnect. The provider's connection-lost handling still closes every connected device in bulk, since that's the one case an individual device could never notice on its own (the buttplug protocol doesn't emit per-device removal messages once the connection itself is gone). Also: ButtplugIoDevice.getRefreshInterval now returns undefined for actuator-only devices instead of unconditionally polling sensors that don't exist - sensor values can only be polled (no push/subscribe API exists in the installed buttplug client), so there's nothing to refresh on a device with no sensors.
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 (2)
src/device/deviceManager.ts (1)
204-227: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
onSettingsChanged()isn't serialized against overlapping calls.
DeviceProviderManagerexplicitly serializesloadFromSettings()/stopProviders()because settings can change in rapid succession (e.g. a device source being disabled and immediately re-enabled), overlapping calls need to be serialized to avoid racing on that map — butDeviceManager.onSettingsChanged()(invoked without awaiting, fire-and-forget, fromapp.ts's settings-changed listener) has no equivalent guard despite mutating the same kind of shared state (connectedDevices,pendingDisabledDevices) across anawait device.close()boundary. Two settings changes in quick succession could interleave: the first call's loop is mid-close()on a device (still present inconnectedDevicessince removal happens via thedeviceDisconnectedlistener), and the second call's loop iterates and callsclose()on the same device again concurrently.Consider mirroring
DeviceProviderManager'senqueueOperationpattern here to serializeonSettingsChanged()invocations.🤖 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 `@src/device/deviceManager.ts` around lines 204 - 227, Serialize overlapping onSettingsChanged() invocations using the existing DeviceProviderManager enqueueOperation pattern or an equivalent operation queue. Ensure the full connectedDevices and pendingDisabledDevices processing, including awaited device.close() calls, runs sequentially so rapid settings changes cannot close the same device concurrently.src/device/transport/bleObserver.ts (1)
113-118: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPrevent orphaned BLE scans by checking
activeUsersafter awaiting power-on.There is a TOCTOU (time-of-check to time-of-use) race condition here. If a provider rapidly calls
init()and thenstop()(e.g. during a rapid settings reload):
init()triggersobserve(), which awaitsnoble.waitForPoweredOnAsync().stop()executes, decrementsactiveUsersto 0, and attempts to stop the scan. BecauseisScanningis stillfalse, it skips stopping the scan.waitForPoweredOnAsync()resolves. The method proceeds to setisScanning = trueand starts the scan.This results in the BLE adapter scanning indefinitely while
activeUsersis 0, ignoring subsequentstop()calls. Re-verifyingactiveUsersafter theawaitfixes the race.🔒️ Proposed fix
try { // Wait for Adapter poweredOn state await noble.waitForPoweredOnAsync(); + if (this.activeUsers === 0) { + return; + } + this.isScanning = true; await noble.startScanningAsync([BleObserver.UART_SERVICE_UUID], true);🤖 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 `@src/device/transport/bleObserver.ts` around lines 113 - 118, In the observe() startup flow, re-check activeUsers immediately after noble.waitForPoweredOnAsync() returns and before setting isScanning or calling noble.startScanningAsync. Abort the startup when no active users remain, preserving stop() behavior and preventing a scan from starting after the final consumer has stopped.
🤖 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 `@src/device/protocol/buttplugIo/buttplugIoDevice.ts`:
- Line 48: Replace the console.error callback in ButtplugIoDevice’s
deviceRemovedHandler with structured logging through an injected Logger. Update
ButtplugIoDeviceFactory to provide the logger, retain the existing async close
flow, and log failures with the “Error closing device” context via the project’s
logError helper.
---
Outside diff comments:
In `@src/device/deviceManager.ts`:
- Around line 204-227: Serialize overlapping onSettingsChanged() invocations
using the existing DeviceProviderManager enqueueOperation pattern or an
equivalent operation queue. Ensure the full connectedDevices and
pendingDisabledDevices processing, including awaited device.close() calls, runs
sequentially so rapid settings changes cannot close the same device
concurrently.
In `@src/device/transport/bleObserver.ts`:
- Around line 113-118: In the observe() startup flow, re-check activeUsers
immediately after noble.waitForPoweredOnAsync() returns and before setting
isScanning or calling noble.startScanningAsync. Abort the startup when no active
users remain, preserving stop() behavior and preventing a scan from starting
after the final consumer has stopped.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0e91598b-94ab-4553-9763-b0601237f41b
📒 Files selected for processing (29)
src/app.tssrc/device/bleDevice.tssrc/device/device.tssrc/device/deviceManager.tssrc/device/peripheralDevice.tssrc/device/protocol/airotic/airoticDeviceProvider.tssrc/device/protocol/buttplugIo/buttplugIoDevice.tssrc/device/protocol/buttplugIo/buttplugIoDeviceFactory.tssrc/device/protocol/buttplugIo/buttplugIoDeviceProvider.tssrc/device/protocol/buttplugIo/buttplugIoDeviceProviderFactory.tssrc/device/protocol/estim2b/estim2bSerialDeviceProvider.tssrc/device/protocol/slvCtrlPlus/slvCtrlPlusSerialDeviceProvider.tssrc/device/protocol/virtual/virtualDeviceProvider.tssrc/device/protocol/zc95/zc95SerialDeviceProvider.tssrc/device/provider/bleDeviceProvider.tssrc/device/provider/deviceProvider.tssrc/device/provider/deviceProviderManager.tssrc/device/provider/genericDeviceProviderFactory.tssrc/device/provider/serialDeviceProvider.tssrc/device/transport/bleObserver.tssrc/device/transport/serialPortObserver.tssrc/serviceMap.tssrc/serviceProvider/deviceServiceProvider.tstests/integration/devices/buttplugIoDevice.spec.tstests/unit/device/deviceManager.spec.tstests/unit/device/protocol/buttplugIo/buttplugIoDevice.spec.tstests/unit/device/provider/deviceProviderManager.spec.tstests/unit/device/transport/bleObserver.spec.tstests/unit/device/transport/serialPortObserver.spec.ts
💤 Files with no reviewable changes (2)
- src/device/bleDevice.ts
- src/device/peripheralDevice.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- src/device/provider/genericDeviceProviderFactory.ts
- src/device/protocol/buttplugIo/buttplugIoDeviceFactory.ts
- src/device/device.ts
- tests/unit/device/provider/deviceProviderManager.spec.ts
- src/device/protocol/virtual/virtualDeviceProvider.ts
- src/device/provider/deviceProvider.ts
- tests/unit/device/deviceManager.spec.ts
…ling stop() usb is a real, module-wide EventTarget - without mocking it, every addEventListener() call made by an observer under test was still registered against the real usb object when the next test ran, piling up across the whole file's run and eventually risking Node's MaxListenersExceededWarning. The afterEach hook was working around this by calling stop() twice on every created observer, on the assumption that no test's reference count would ever exceed 2 - a fragile magic number that would silently start leaking listeners again if a future test called start() a third time. Mock usb the same way bleObserver.spec.ts already mocks @stoprocent/noble: no real listeners are ever registered, so there's nothing to leak and nothing to work around.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds an optional
enabledflag (defaults to true) to bothdeviceSourcesandknownDevicesentries in the settings JSON.isEnabled()accessor.Closes #70
Summary by CodeRabbit
enabledcontrols for known devices and device sources.