Allow enabling/disabling device sources and known devices#92
Open
heavyrubberslave wants to merge 31 commits into
Open
Allow enabling/disabling device sources and known devices#92heavyrubberslave wants to merge 31 commits into
heavyrubberslave wants to merge 31 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds enable/disable support for known devices and device sources, refactors device typing to generalized ChangesDevice Enablement and Provider Lifecycle
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant SettingsManager
participant App
participant DeviceManager
participant DeviceProviderManager
participant DeviceProvider
SettingsManager->>App: changed(settings)
App->>DeviceManager: onSettingsChanged()
DeviceManager->>DeviceManager: close disabled devices, re-announce pending enabled ones
App->>DeviceProviderManager: loadFromSettings(settings)
DeviceProviderManager->>DeviceProvider: init() / stop()
DeviceProvider->>DeviceManager: acquireDetectedDevice(detectionId)
DeviceProvider->>DeviceManager: addDevice(deviceInfo, device)
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
…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.
…protocols
Every other protocol factory (airotic, zc95, estim2b, slvCtrlPlus) takes the
already-computed deviceId as a plain parameter and only ButtplugIoDeviceFactory
independently recomputed the same id from the raw ButtplugClientDevice via its
own static computeDeviceId(). ButtplugIoDeviceFactory.create() now takes
(deviceId, buttplugDevice, provider) like its siblings, and the id computation
itself moved into buttplugIoWebsocketDeviceProvider's toDeviceInfo() - the one
place it was actually called from - since the separate computeDeviceId()
method was only ever a couple of lines wrapping that single call site.
Also fixes the buttplugIoWebsocketDeviceProvider class name's casing
(lowercase 'b' was a pre-existing typo against TS convention) and realigns
providerName with what main already uses ('buttplugIoWebsocket'), rather than
what this branch had reverted it to earlier.
A rejected device.close() previously bubbled straight out of the loop, skipping the close of any remaining removed devices and also skipping discovery of newly configured devices for that reconciliation pass entirely. Catch and log each close failure individually so one misbehaving device can't block cleanup/discovery for the rest.
toDeviceInfo() -> createDeviceDetectionInfo() to match the type it actually builds (ButtplugIoDeviceDetectionInfo), and ButtplugIoDeviceProviderConfig -> ButtplugIoWebsocketDeviceProviderConfig to match the provider class it configures.
…during stop() DeviceProviderManager deliberately keeps a provider recorded whose stop() rejected, on the assumption it may still be partially running. But DeviceProvider.stop() marks itself stopped and unsubscribes from device detection before awaiting each device's close() - both irreversible and necessary so in-flight detections can't survive stop(). If any device's close() then rejected, that exception propagated out of stop() itself, leaving the provider internally dead (stopped, unsubscribed) yet retained by the manager as if it might recover, permanently blocking a later re-enable of that source from ever creating a replacement. Catch and log each device's close() failure individually instead, matching the same pattern already used in VirtualDeviceProvider's reconciliation loop and DeviceManager.onSettingsChanged(). Device.close() already emits deviceDisconnected and transitions state in a finally block regardless of whether the underlying doClose() throws, so nothing is left dangling by catching here - stop() can now always complete, so DeviceProviderManager can always safely remove the provider and create a fresh one on the next reload. Also includes a few smaller cleanups: dropped an unused JsonObject import/ eslint-disable in VirtualDeviceProviderFactory, removed a stale docstring comment from BleDeviceProvider, and renamed DeviceProvider's DI type param to DDI.
…entialTaskQueue The manual Promise chain (this.operationQueue.then(operation, operation), re-assigned with a .catch(() => undefined) to keep the chain from wedging on a rejected operation) was reimplementing what SequentialTaskQueue already does out of the box: run pushed tasks strictly one at a time in FIFO order, and move on to the next queued task regardless of whether the previous one resolved or rejected. This is the same package already used for the same kind of serialization in synchronousSerialPort.ts and bufferedDeviceUpdater.ts. enqueueOperation() stays as a thin wrapper rather than calling push() at each call site directly, since it earns its keep on two fronts: SequentialTaskQueue.push() returns a CancellablePromiseLike<any> (only has .then(), not .catch()/.finally()), which app.ts relies on via .catch() after loadFromSettings() - awaiting it inside an async method is what normalizes it back into a real Promise<void> without an any/type-assertion. Its operation: () => Promise<void> parameter type is also a tighter contract than push()'s own untyped Function parameter, which accepts any callable regardless of arity or return type.
…server into SharedObserver Both observers are DI singletons shared across multiple DeviceProviders of the same transport, and both had near-identical activeUsers counting logic (plus matching doc comments) reimplementing the same 'only start on the first caller, only stop once every caller has also stopped' contract independently. Moved that bookkeeping into a new abstract SharedObserver base class: acquire()/release() own the counting and 'already running'/'still used' logging, and delegate the actual transport-specific work to abstract onFirstStart()/onLastStop() hooks. BleObserver and SerialPortObserver keep their existing public method names (init()/stop() and start()/stop() respectively) as thin wrappers around acquire()/release(), so nothing calling them (BleDeviceProvider, SerialDeviceProvider) needs to change. Also removed SerialPortObserver's 'public static readonly name = "serial"', which shadowed the class's built-in static name property for no reason beyond its own constructor/test - it wasn't used as a DI key, discriminator, or anywhere else. Its logger now shows 'SerialPortObserver' like every other class in the codebase that relies on the built-in name for its logger label, instead of being the one exception.
…r.transform()
KnownDevice/DeviceSource previously defended against a missing 'enabled' by
falling back to '?? true' in their getters, working around class-transformer
bypassing the constructor on plain-JSON deserialization. That fallback only
ever protected consumers of these two specific classes, and needed a
matching '_enabled'/toClassOnly + getter/toPlainOnly split just to keep the
serialized payload looking like a plain boolean.
PlainToClassSerializer.transform() now takes an optional TypeBox schema
alongside the target class: when given, it validates plain against it via
the shared Ajv instance (throwing a SchemaValidationError carrying Ajv's own
error objects on failure), then hydrates any schema-declared defaults (e.g.
enabled: true) into plain via Value.Default() before deserializing. Both
SettingsManager.load() and PutSettingsController now just pass SettingsSchema
into transform() instead of running their own separate JsonSchemaValidator
validate-then-transform dance, and PutSettingsController's structured 400
response now comes straight from the thrown error's validationErrors.
This let KnownDevice/DeviceSource drop the '?? true' workaround entirely -
'enabled' is now a plain field like every other property on these classes,
since anything that deserializes them through Settings is guaranteed to
already have it hydrated.
One TypeBox quirk had to be worked around: Value.Default() only recurses
into an *existing* Type.Record entry to apply that entry's own nested
defaults if the entry's own schema also carries a 'default' - otherwise it
leaves already-present-but-incomplete entries untouched. Added 'default: {}'
to the knownDevices/deviceSources per-entry object schemas to opt into that
recursion (verified empirically with a throwaway script before landing it).
Removed the now-fully-unused 'settings.schema.validator' DI registration
(JsonSchemaValidatorFactory/JsonSchemaValidator themselves stay - still used
independently by GenericVirtualDeviceFactory for per-device-type config
validation).
@typescript-eslint/no-floating-promises already treats a promise chain ending in .catch() as handled - confirmed empirically (no new lint warning without it). The void operator here wasn't doing anything the .catch() wasn't already covering.
heavyrubberslave
marked this pull request as ready for review
July 19, 2026 17:11
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/device/transport/bleObserver.ts (1)
88-106: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAvoid blocking
init()indefinitely if the BLE adapter is unavailable.
await noble.waitForPoweredOnAsync()will hang indefinitely if Bluetooth is turned off, unavailable, or lacking permissions. BecauseDeviceProviderManagerserializes provider operations, this hang will block the entire initialization queue, preventing unrelated providers (like serial or Buttplug) from loading.Additionally, this indefinite
awaitcreates a TOCTOU race condition: ifstop()is called while it is waiting,observe()will eventually resume and start scanning after the observer is supposed to be stopped.Since you already have a
stateChangelistener that triggersobserve()when the adapter powers on, you can safely drop the indefinite wait and rely on the state machine instead.🔒️ Proposed fix
private async observe(): Promise<void> { if (this.isScanning) { return; } try { - // Wait for Adapter poweredOn state - await noble.waitForPoweredOnAsync(); + if (noble.state !== 'poweredOn') { + this.logger.debug(`BLE adapter state is '${noble.state}', waiting for 'poweredOn' event`); + return; + } this.isScanning = true; await noble.startScanningAsync([BleObserver.UART_SERVICE_UUID], true); this.logger.info('Looking for BLE UART devices');🤖 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 88 - 106, Remove the blocking noble.waitForPoweredOnAsync() call from observe() so init() cannot hang when the adapter is unavailable and stop() cannot be bypassed after waiting. Let observe() proceed only when the existing adapter stateChange listener has detected a powered-on state, while preserving the current scanning, logging, and error-handling behavior in observe().
🧹 Nitpick comments (3)
src/device/transport/bleObserver.ts (1)
59-69: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider explicitly tracking and removing specific event listeners.
Calling
noble.removeAllListeners()without arguments clears every listener attached to the module, which can sometimes interfere with internal library state or other modules sharing the singleton. Retaining references to your bound callbacks and removing them explicitly vianoble.off(...)is a safer pattern.🤖 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 59 - 69, Update onLastStop to stop calling noble.removeAllListeners(). Retain references to this observer’s registered callbacks and remove only those listeners with noble.off(...), preserving listeners owned by noble or other modules while keeping the existing scan-stop and noble.stop behavior unchanged.src/device/protocol/buttplugIo/buttplugIoWebsocketDeviceProvider.ts (1)
163-172: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPrevent unhandled errors by verifying connection status before stopping the scan.
If the provider is stopped or the websocket disconnects while this 30-second scan window is open, the timeout callback will still fire. Calling
stopScanning()on a disconnected client typically throws an error.Consider checking
this.isStopped()or!this.buttplugClient.connectedto avoid unnecessary API calls and potential errors during teardown.♻️ Proposed fix
setTimeout(() => { - if (undefined === this.buttplugClient || !this.buttplugClient.isScanning) { + if (this.isStopped() || !this.buttplugClient.connected || !this.buttplugClient.isScanning) { return; } this.buttplugClient.stopScanning()🤖 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/buttplugIo/buttplugIoWebsocketDeviceProvider.ts` around lines 163 - 172, Update the delayed scan-stop callback in the ButtplugIoWebsocketDeviceProvider scan flow to return when the provider is stopped or the client is no longer connected, before calling stopScanning(). Preserve the existing undefined-client and isScanning checks, and keep stopScanning() and its logging behavior unchanged for active connected clients.src/device/transport/sharedObserver.ts (1)
13-22: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider tracking the startup promise to support concurrent
acquire()calls.While
DeviceProviderManagersafely serializes provider initialization in the current architecture, this reference-counting logic will fail ifacquire()is ever called concurrently. The second caller incrementsactiveUsersand returns immediately, believing the observer is fully started even though the first caller'sonFirstStart()promise is still pending.If future changes allow concurrent provider loading, consider caching the startup promise so subsequent callers wait for initialization to finish.
♻️ Proposed fix
+ private startupPromise: Promise<void> | null = null; + protected async acquire(): Promise<void> { this.activeUsers++; if (this.activeUsers > 1) { this.logger.debug(`Already running, now used by ${this.activeUsers} provider(s)`); + if (this.startupPromise) await this.startupPromise; return; } - await this.onFirstStart(); + this.startupPromise = this.onFirstStart(); + try { + await this.startupPromise; + } finally { + this.startupPromise = null; + } }🤖 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/sharedObserver.ts` around lines 13 - 22, Update SharedObserver.acquire to cache the in-flight onFirstStart startup promise when activeUsers transitions from zero, and have concurrent callers await that same promise before returning. Preserve the existing reference counting and debug behavior, while ensuring the cached promise is cleared appropriately after startup completes.
🤖 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.
Outside diff comments:
In `@src/device/transport/bleObserver.ts`:
- Around line 88-106: Remove the blocking noble.waitForPoweredOnAsync() call
from observe() so init() cannot hang when the adapter is unavailable and stop()
cannot be bypassed after waiting. Let observe() proceed only when the existing
adapter stateChange listener has detected a powered-on state, while preserving
the current scanning, logging, and error-handling behavior in observe().
---
Nitpick comments:
In `@src/device/protocol/buttplugIo/buttplugIoWebsocketDeviceProvider.ts`:
- Around line 163-172: Update the delayed scan-stop callback in the
ButtplugIoWebsocketDeviceProvider scan flow to return when the provider is
stopped or the client is no longer connected, before calling stopScanning().
Preserve the existing undefined-client and isScanning checks, and keep
stopScanning() and its logging behavior unchanged for active connected clients.
In `@src/device/transport/bleObserver.ts`:
- Around line 59-69: Update onLastStop to stop calling
noble.removeAllListeners(). Retain references to this observer’s registered
callbacks and remove only those listeners with noble.off(...), preserving
listeners owned by noble or other modules while keeping the existing scan-stop
and noble.stop behavior unchanged.
In `@src/device/transport/sharedObserver.ts`:
- Around line 13-22: Update SharedObserver.acquire to cache the in-flight
onFirstStart startup promise when activeUsers transitions from zero, and have
concurrent callers await that same promise before returning. Preserve the
existing reference counting and debug behavior, while ensuring the cached
promise is cleared appropriately after startup completes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1fce9863-c182-4c8e-9512-c4fecfec08ad
📒 Files selected for processing (31)
src/app.tssrc/controller/settings/putSettingsController.tssrc/device/deviceManager.tssrc/device/protocol/buttplugIo/buttplugIoDeviceFactory.tssrc/device/protocol/buttplugIo/buttplugIoWebsocketDeviceProvider.tssrc/device/protocol/buttplugIo/buttplugIoWebsocketDeviceProviderFactory.tssrc/device/protocol/virtual/virtualDeviceProvider.tssrc/device/protocol/virtual/virtualDeviceProviderFactory.tssrc/device/provider/bleDeviceProvider.tssrc/device/provider/deviceProvider.tssrc/device/provider/deviceProviderManager.tssrc/device/provider/serialDeviceProvider.tssrc/device/transport/bleObserver.tssrc/device/transport/serialPortObserver.tssrc/device/transport/sharedObserver.tssrc/schemaValidation/schemaValidationError.tssrc/serialization/plainToClassSerializer.tssrc/serviceMap.tssrc/serviceProvider/controllerServiceProvider.tssrc/serviceProvider/deviceServiceProvider.tssrc/serviceProvider/serializationServiceProvider.tssrc/serviceProvider/settingsServiceProvider.tssrc/settings/deviceSource.tssrc/settings/knownDevice.tssrc/settings/settings.tssrc/settings/settingsManager.tstests/integration/devices/buttplugIoDevice.spec.tstests/unit/device/transport/serialPortObserver.spec.tstests/unit/settings/deviceSource.spec.tstests/unit/settings/knownDevice.spec.tstests/unit/settings/settings.spec.ts
💤 Files with no reviewable changes (7)
- tests/unit/settings/knownDevice.spec.ts
- tests/unit/settings/deviceSource.spec.ts
- src/serviceProvider/controllerServiceProvider.ts
- src/app.ts
- src/device/provider/bleDeviceProvider.ts
- src/device/provider/serialDeviceProvider.ts
- src/serviceProvider/settingsServiceProvider.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- src/settings/settings.ts
- src/device/protocol/virtual/virtualDeviceProvider.ts
- src/device/transport/serialPortObserver.ts
- src/device/provider/deviceProviderManager.ts
- src/device/provider/deviceProvider.ts
- src/device/deviceManager.ts
…nify observer/provider naming Fixes for findings from PR review comments that were embedded in full review bodies (outside-diff/nitpick sections) rather than separate discussion threads, so an earlier per-thread pass missed them: - DeviceManager.onSettingsChanged() is invoked fire-and-forget from a settings-changed listener, so rapid successive settings changes could overlap and race on connectedDevices/pendingDisabledDevices across the await device.close() boundary. Serialized via the same SequentialTaskQueue pattern already used by DeviceProviderManager. - BleObserver.init() (now start()) awaited noble.waitForPoweredOnAsync() directly, which - while bounded to noble's own 10s default rather than truly indefinite - still meant a BLE adapter that's slow/never powers on delays this provider's start() by that long, and raced against stop() resolving mid-wait. Restructured to run the power-on wait as a background task instead of blocking start()/acquire() on it (see below). - ButtplugIoWebsocketDeviceProvider's delayed scan-stop callback didn't account for the connection having dropped or the provider having stopped during the scan window, since buttplugClient.isScanning isn't reset by a lost/closed connection - only by an explicit start/stopScanning call or a 'scanningfinished' message. Added connected()/isStopped() checks. - DeviceProvider.abortDetection() didn't guarantee releaseDetectedDevice() runs if onConnectFailed()/device.close() themselves threw, permanently leaking the acquire-queue claim for that device. Wrapped both in finally. (This was previously marked resolved by the review bot, but checking the referenced commit showed the finally-wrapping was never actually added.) BleObserver's power-on wait is now a genuinely backgrounded task (started fire-and-forget from onFirstStart(), not awaited) so a BLE-based device source with no adapter available can't stall DeviceProviderManager's whole reload pipeline behind it. It still uses noble's own recommended waitForPoweredOnAsync() (matching noble's documented usage), but re-issues it in 10-minute chunks rather than one huge timeout, since noble has no "wait forever" option and Node's setTimeout silently clamps any delay above ~24.8 days down to 1ms. stop() cancels the wait via a self-controlled Promise.race signal - though since noble exposes no cancellation API, the abandoned noble call's own internal timer keeps running regardless for up to that chunk size, which is also why it's bounded to a human-scale duration rather than something much larger. Separately, followed up on the earlier SharedObserver reference-counting fix: concurrent acquire() calls are now actually reachable, since DeviceProviderManager.doLoadFromSettings() parallelizes starting/stopping providers for different device sources via Promise.allSettled() instead of one at a time - so one slow/hung provider no longer delays every other, unrelated device source during a settings reload. Each task only ever touches its own id in the providers map, so nothing new to race there. Finally, two naming cleanups for consistency: - SharedObserver.start()/stop() are now concrete public methods on the base class itself (renamed from the protected acquire()/release()), since every subclass's public entry point was already a pure one-line passthrough to them with no added behavior - removes that redundant wrapper entirely from both BleObserver and SerialPortObserver, and BleObserver's own init()/stop() become the inherited start()/stop() directly. - DeviceProvider.init() renamed to start() across the whole provider hierarchy to match this same start()/stop() convention. Added tests/unit/helper/async.ts's waitTicks() for tests that need to let an already-pending promise chain progress a specific number of microtask turns - preferred over a real setTimeout()-based wait since it's fully deterministic and works the same regardless of whether the calling test uses vi.useFakeTimers() (serialPortObserver.spec.ts does).
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 (1)
src/device/provider/deviceProviderManager.ts (1)
81-105: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winProvider construction failures are silently swallowed.
factory.create(deviceSource.config)(line 89) runs outside thetrythat wrapsprovider.start(). Ifcreate()throws synchronously,Promise.allSettledabsorbs the rejection with no logging at all - unlikedoStopProviders(), which explicitly collects and logs rejected results. An operator would have no way to tell why a device source silently never started.🔧 Suggested fix
- const provider = factory.create(deviceSource.config); - try { + const provider = factory.create(deviceSource.config); + await provider.start(); // Only record the provider once it started successfully, so a failed start // doesn't leave a stuck entry that blocks all future retries for this source. this.providers.set(id, provider); + + return; } catch (error: unknown) { - logError(this.logger, `Failed to start device provider for device source '${id}'`, error); + logError(this.logger, `Failed to create/start device provider for device source '${id}'`, error); + return; + } +``` </details> <details> <summary>🤖 Prompt for AI Agents</summary>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/deviceProviderManager.tsaround lines 81 - 105, Move the
provider construction call in the startup flow around the Promise.allSettled
callback into the existing error-handling try block so synchronous failures from
factory.create(deviceSource.config) are caught and logged with the device source
context. Preserve the current start-success registration and half-started
provider cleanup behavior for providers that are created successfully.</details> <!-- cr-comment:v1:a933b225facc6108c13db8a7 --> </blockquote></details> </blockquote></details>🤖 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/provider/deviceProviderManager.ts`: - Line 23: Add an error listener for the SequentialTaskQueue instance initialized by operationQueue, ensuring rejected tasks from doStopProviders() are handled through the queue’s error event after its existing logging path. --- Outside diff comments: In `@src/device/provider/deviceProviderManager.ts`: - Around line 81-105: Move the provider construction call in the startup flow around the Promise.allSettled callback into the existing error-handling try block so synchronous failures from factory.create(deviceSource.config) are caught and logged with the device source context. Preserve the current start-success registration and half-started provider cleanup behavior for providers that are created successfully.🪄 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:
3894eb80-ffc5-4701-8345-3c900c4b5d5b📒 Files selected for processing (34)
src/app.tssrc/controller/settings/putSettingsController.tssrc/device/deviceManager.tssrc/device/protocol/buttplugIo/buttplugIoDeviceFactory.tssrc/device/protocol/buttplugIo/buttplugIoWebsocketDeviceProvider.tssrc/device/protocol/buttplugIo/buttplugIoWebsocketDeviceProviderFactory.tssrc/device/protocol/virtual/virtualDeviceProvider.tssrc/device/protocol/virtual/virtualDeviceProviderFactory.tssrc/device/provider/bleDeviceProvider.tssrc/device/provider/deviceProvider.tssrc/device/provider/deviceProviderManager.tssrc/device/provider/serialDeviceProvider.tssrc/device/transport/bleObserver.tssrc/device/transport/serialPortObserver.tssrc/device/transport/sharedObserver.tssrc/schemaValidation/schemaValidationError.tssrc/serialization/plainToClassSerializer.tssrc/serviceMap.tssrc/serviceProvider/controllerServiceProvider.tssrc/serviceProvider/deviceServiceProvider.tssrc/serviceProvider/serializationServiceProvider.tssrc/serviceProvider/settingsServiceProvider.tssrc/settings/deviceSource.tssrc/settings/knownDevice.tssrc/settings/settings.tssrc/settings/settingsManager.tstests/integration/devices/buttplugIoDevice.spec.tstests/unit/device/provider/deviceProviderManager.spec.tstests/unit/device/transport/bleObserver.spec.tstests/unit/device/transport/serialPortObserver.spec.tstests/unit/helper/async.tstests/unit/settings/deviceSource.spec.tstests/unit/settings/knownDevice.spec.tstests/unit/settings/settings.spec.ts💤 Files with no reviewable changes (5)
- tests/unit/settings/deviceSource.spec.ts
- tests/unit/settings/knownDevice.spec.ts
- src/serviceProvider/controllerServiceProvider.ts
- src/serviceProvider/settingsServiceProvider.ts
- src/app.ts
🚧 Files skipped from review as they are similar to previous changes (18)
- src/schemaValidation/schemaValidationError.ts
- src/settings/deviceSource.ts
- src/controller/settings/putSettingsController.ts
- tests/integration/devices/buttplugIoDevice.spec.ts
- tests/unit/settings/settings.spec.ts
- src/device/protocol/virtual/virtualDeviceProviderFactory.ts
- src/device/protocol/buttplugIo/buttplugIoWebsocketDeviceProviderFactory.ts
- src/device/protocol/virtual/virtualDeviceProvider.ts
- src/device/protocol/buttplugIo/buttplugIoDeviceFactory.ts
- src/serialization/plainToClassSerializer.ts
- src/settings/settingsManager.ts
- tests/unit/device/transport/serialPortObserver.spec.ts
- src/settings/settings.ts
- src/device/transport/serialPortObserver.ts
- src/device/provider/serialDeviceProvider.ts
- src/serviceMap.ts
- src/device/protocol/buttplugIo/buttplugIoWebsocketDeviceProvider.ts
- src/device/deviceManager.ts
heavyrubberslave
added
minor
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
New Features
Bug Fixes