Allow enabling/disabling device sources and known devices - #92
Merged
Merged
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 devices and sources, generalizes device and provider contracts, centralizes detection and lifecycle handling, serializes provider reloads, moves settings validation into serialization, and introduces an isolated automation VM abstraction. ChangesDevice enablement and contracts
Provider lifecycle and orchestration
Automation and settings validation
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant SettingsManager
participant App
participant DeviceProviderManager
participant DeviceManager
participant DeviceProvider
SettingsManager->>App: settings changed
App->>DeviceProviderManager: loadFromSettings(settings)
DeviceProviderManager->>DeviceProvider: start or stop provider
App->>DeviceManager: onSettingsChanged()
DeviceProvider->>DeviceManager: announceDetectedDevice(detectionInfo)
DeviceManager->>DeviceProvider: acquire and add device
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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.
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
- app.ts: reload device sources before applying device enable/disable changes on a settings change (was unordered, could re-announce a device before its provider reloaded); split the reactive settings- change handling out of configureWebsocket() into its own configureSettingsChangeHandling(), since it has nothing to do with WebSockets beyond the broadcast itself. - DeviceProvider: start()/stop() are now template methods calling subclass hooks doStart()/doStop(), matching SharedObserver's onFirstStart()/onLastStop() pattern. The deviceManager.on() subscription moved out of the constructor into start() (previously reactive before the provider was ever started). Providers now properly support a stop() -> start() restart cycle instead of throwing, collapsed into a single `running` field instead of two. - ButtplugIoWebsocketDeviceProvider: split reconnect() out of doStart() so a lost-connection reconnect no longer re-registers the buttplug client's listeners. - DeviceProviderManager: restart a provider when its device source's type/config changes instead of leaving it running with stale config. - SharedObserver: a failed onFirstStart() no longer permanently increments the refcount; reworked so a caller only counts as active once startup actually succeeds, removing the need for manual rollback on failure. - BleObserver: distinguish real adapter errors from a power-on timeout instead of swallowing both as "timeout"; fixed a start/stop race around the scanning flag. - PlainToClassSerializer: don't mutate the caller's input object when hydrating schema defaults. - Removed dead code (BleDevice.getPeripheral, PeripheralDevice.getTransport), routed ButtplugIoDevice's device-removed handler through its logger instead of console.error, extracted a shared WithUntypedAttributes<D> helper instead of duplicating the same Omit/intersection three times, renamed the stale device.provider.loader DI key to device.provider.manager, and unified deviceDetectionInfo/deviceInfo parameter naming across DeviceProvider and its subclasses. Tests: added coverage for SharedObserver's startup-failure/idempotency behaviour, DeviceProvider's restart cycle and listener bookkeeping, and DeviceProviderManager's config-change restart; fixed a flaky integration test that could observe a stale sensor reading.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/device/protocol/buttplugIo/buttplugIoWebsocketDeviceProvider.ts (1)
131-146: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUncaught
device.close()can permanently stall reconnection.
handleLostConnection()iterates connected devices and awaitsdevice.close()without a try/catch. If any device'sclose()rejects, the loop aborts,autoScanningIntervalRefis never cleared, and — since this now also gates the reconnect resumption —this.connectClient()at Line 145 never runs. The provider would stop attempting to reconnect to the buttplug.io server until manually restarted. This mirrors the exact hazard already fixed indeviceProvider.ts'sstop()(per-device try/catch), but is missing here.🩹 Proposed fix
private async handleLostConnection(url: string): Promise<void> { this.logger.info(`Lost connection to buttplug.io server (${url})`); for (const device of [...this.getConnectedDevices()]) { - await device.close(); + try { + await device.close(); + } catch (e: unknown) { + logError(this.logger, `Failed to close device '${device.getDeviceId}' after losing buttplug.io connection`, e); + } } clearInterval(this.autoScanningIntervalRef); this.autoScanningIntervalRef = undefined;🤖 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 131 - 146, Update handleLostConnection to close each connected device inside its own try/catch, matching the per-device handling in deviceProvider.ts stop(). Ensure a rejected device.close() is handled without aborting the loop, so interval cleanup and the subsequent connectClient() call still execute.
🤖 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/app.ts`:
- Around line 265-267: Guard the device provider shutdown call in shutdown() so
an error from device.provider.manager.stopProviders() is handled without
aborting the remaining graceful-shutdown steps. Ensure
health.metricsCollector.stop() and the subsequent websocket/HTTP(S) server close
operations still execute when an individual provider fails, while preserving the
existing stopProviders invocation.
In `@src/device/provider/deviceProvider.ts`:
- Around line 46-58: Guard the await of doStop() in DeviceProvider.stop() so a
subclass failure cannot prevent the remaining shutdown steps. Ensure
deviceDetected listener detachment and connectedDevices cleanup/close still
execute, while preserving the existing non-rejecting stop behavior and handling
the doStop error consistently with the device-close loop.
---
Outside diff comments:
In `@src/device/protocol/buttplugIo/buttplugIoWebsocketDeviceProvider.ts`:
- Around line 131-146: Update handleLostConnection to close each connected
device inside its own try/catch, matching the per-device handling in
deviceProvider.ts stop(). Ensure a rejected device.close() is handled without
aborting the loop, so interval cleanup and the subsequent connectClient() call
still execute.
🪄 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: 2e41bb83-a76a-4ae4-9495-011babc2334c
📒 Files selected for processing (28)
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/buttplugIoWebsocketDeviceProvider.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/serialDeviceProvider.tssrc/device/transport/bleObserver.tssrc/device/transport/sharedObserver.tssrc/serialization/plainToClassSerializer.tssrc/serviceMap.tssrc/serviceProvider/deviceServiceProvider.tstests/integration/devices/buttplugIoDevice.spec.tstests/unit/device/protocol/buttplugIo/buttplugIoDevice.spec.tstests/unit/device/provider/deviceProvider.spec.tstests/unit/device/provider/deviceProviderManager.spec.tstests/unit/device/transport/sharedObserver.spec.tstests/unit/settings/settings.spec.ts
🚧 Files skipped from review as they are similar to previous changes (19)
- tests/unit/settings/settings.spec.ts
- src/device/protocol/slvCtrlPlus/slvCtrlPlusSerialDeviceProvider.ts
- src/serviceProvider/deviceServiceProvider.ts
- src/device/transport/sharedObserver.ts
- src/device/peripheralDevice.ts
- src/serialization/plainToClassSerializer.ts
- tests/unit/device/protocol/buttplugIo/buttplugIoDevice.spec.ts
- src/device/protocol/estim2b/estim2bSerialDeviceProvider.ts
- src/device/protocol/buttplugIo/buttplugIoDeviceFactory.ts
- src/device/protocol/zc95/zc95SerialDeviceProvider.ts
- src/device/protocol/virtual/virtualDeviceProvider.ts
- src/device/protocol/airotic/airoticDeviceProvider.ts
- src/device/provider/bleDeviceProvider.ts
- src/device/protocol/buttplugIo/buttplugIoDevice.ts
- tests/unit/device/provider/deviceProviderManager.spec.ts
- src/device/transport/bleObserver.ts
- src/device/provider/serialDeviceProvider.ts
- src/device/deviceManager.ts
- src/device/provider/deviceProviderManager.ts
- shutdown() no longer aborts remaining cleanup steps if stopProviders() throws - DeviceProvider.stop() no longer skips listener detachment and device cleanup if doStop() throws Addresses CodeRabbit findings on PR #92
POST /automation/run resolved before the log file's write stream finished opening, so an immediate GET /automation/log could hit read-last-lines' fs.exists() check before the file existed on disk, causing a 500. load() now awaits the stream's open event before resolving. getLog() also treats a missing log file as an empty log instead of an error, covering the case of no script having run yet.
Member
Author
|
@coderabbitai please do a final review |
|
✅ Action performedFull review finished. |
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/transport/serialPortObserver.ts (1)
110-121: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winClear
managedDeviceswhen stopping the serial observer.Disabling/re-enabling a serial source runs
DeviceProviderManager.stopProviders()followed byloadFromSettings(), which creates a newSerialProvider, starts it, and shuts down the previous provider viaDeviceProvider.stop()(closing the connected device). BecauseSerialPortObserverkeeps entries inmanagedDevicesand only skips announcement while a device is already present, a still-plugged serial device is not re-announced on the restart. Clear this cache inonLastStop()so the next start reports present devices again.🛠️ Proposed fix
protected async onLastStop(): Promise<void> { if (this.rescanTimer !== undefined) { clearTimeout(this.rescanTimer); this.rescanTimer = undefined; } if (this.onUsbEventRef !== undefined) { usb.removeEventListener('connect', this.onUsbEventRef); usb.removeEventListener('disconnect', this.onUsbEventRef); this.onUsbEventRef = undefined; } + + // Drop detection cache so a subsequent start re-announces still-present devices + this.managedDevices.clear(); }🤖 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/serialPortObserver.ts` around lines 110 - 121, Clear the managedDevices cache in SerialPortObserver.onLastStop() during shutdown, alongside the existing timer and USB listener cleanup, so restarting the observer re-announces currently connected serial devices.
🤖 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/automation/scriptRuntime.ts`:
- Around line 302-312: Update the writer-open handling in load() so any
rejection from the promise waiting on writer open disposes the already-created
isolate and VM context and clears the captured dispatchRef/lifecycleRef before
rethrowing the error. Ensure the cleanup occurs before load() exits, while
preserving normal initialization and assignment of logWriter after a successful
open.
---
Outside diff comments:
In `@src/device/transport/serialPortObserver.ts`:
- Around line 110-121: Clear the managedDevices cache in
SerialPortObserver.onLastStop() during shutdown, alongside the existing timer
and USB listener cleanup, so restarting the observer re-announces currently
connected serial devices.
🪄 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 Plus
Run ID: 7e55bcba-0dfa-46ff-8ac4-b4a897f70890
📒 Files selected for processing (61)
src/app.tssrc/automation/scriptRuntime.tssrc/controller/settings/putSettingsController.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/buttplugIo/buttplugIoWebsocketDeviceProviderFactory.tssrc/device/protocol/estim2b/estim2bSerialDeviceProvider.tssrc/device/protocol/slvCtrlPlus/slvCtrlPlusSerialDeviceProvider.tssrc/device/protocol/virtual/virtualDeviceProvider.tssrc/device/protocol/virtual/virtualDeviceProviderFactory.tssrc/device/protocol/zc95/zc95SerialDeviceProvider.tssrc/device/provider/bleDeviceProvider.tssrc/device/provider/deviceProvider.tssrc/device/provider/deviceProviderFactory.tssrc/device/provider/deviceProviderManager.tssrc/device/provider/genericDeviceProviderFactory.tssrc/device/provider/serialDeviceProvider.tssrc/device/transport/bleObserver.tssrc/device/transport/serialPortObserver.tssrc/device/transport/sharedObserver.tssrc/device/updater/abstractDeviceUpdater.tssrc/device/updater/bufferedDeviceUpdater.tssrc/device/updater/deviceUpdaterInterface.tssrc/entity/deviceList.tssrc/repository/connectedDeviceRepository.tssrc/repository/deviceRepositoryInterface.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/serializedTypes.tssrc/settings/settings.tssrc/settings/settingsManager.tstests/integration/deviceEvents.spec.tstests/integration/devices/buttplugIoDevice.spec.tstests/integration/helpers/appHelper.tstests/unit/automation/scriptRuntime.spec.tstests/unit/device/deviceManager.spec.tstests/unit/device/protocol/buttplugIo/buttplugIoDevice.spec.tstests/unit/device/provider/deviceProvider.spec.tstests/unit/device/provider/deviceProviderManager.spec.tstests/unit/device/testDeviceProvider.tstests/unit/device/transport/bleObserver.spec.tstests/unit/device/transport/serialPortObserver.spec.tstests/unit/device/transport/sharedObserver.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 (3)
- src/serviceProvider/controllerServiceProvider.ts
- tests/unit/device/testDeviceProvider.ts
- src/serviceProvider/settingsServiceProvider.ts
Member
Author
|
@coderabbitai please do a final review |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/device/protocol/buttplugIo/buttplugIoWebsocketDeviceProvider.ts (1)
107-127: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMissing post-await
isStopped()check leaks a connection afterstop()races with an in-flight connect.
isStopped()is only checked beforeawait this.buttplugClient.connect(...). Ifstop()runs while that connect is pending,doStop()seesbuttplugClient.connected === falseand skips disconnecting, and already removed all listeners. When the pending connect then resolves,connectToServer()proceeds unconditionally - the websocket stays open with no listeners, and (ifautoScanis on) a freshautoScanningIntervalRefstarts drivingdiscoverButtplugIoDevices()indefinitely, even though the provider is stopped.🩹 Proposed fix
try { await this.buttplugClient.connect(this.buttplugConnector); + + if (this.isStopped()) { + // stop() ran while this connect attempt was in flight; listeners are already + // torn down, so just close the now-stale connection instead of proceeding + try { + await this.buttplugClient.disconnect(); + } catch (e: unknown) { + logError(this.logger, 'Could not disconnect stale connection after stop()', e); + } + return; + } + this.logger.info(`Successfully connected to buttplug.io server (${url})`);🤖 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 107 - 127, Add an isStopped() check immediately after the awaited buttplugClient.connect() in connectToServer(). If the provider was stopped while the connection was pending, disconnect the newly established client and return before logging success, clearing connection timers, or starting auto-scanning.
🤖 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/automation/scriptRuntime.ts`:
- Around line 226-243: Wrap the initialization after assigning `this.logWriter`
in cleanup handling covering isolate/context creation, script compilation, and
jail setup. On failure, close the writer, release any partially created isolate
or context, reset related state as needed, and rethrow the original error so
`stop()` is not relied on while `runningSince` remains null.
In `@src/device/provider/deviceProviderManager.ts`:
- Around line 84-110: Move factory.create(deviceSource.config) inside the try
block in DeviceProviderManager’s provider-start flow so creation failures are
passed to logError and do not escape silently; preserve cleanup only when a
provider was created. In
tests/unit/device/provider/deviceProviderManager.spec.ts lines 85-237, add
coverage where the factory’s create() throws, asserting logError/warn is invoked
and the source is retried on the next loadFromSettings() call.
---
Outside diff comments:
In `@src/device/protocol/buttplugIo/buttplugIoWebsocketDeviceProvider.ts`:
- Around line 107-127: Add an isStopped() check immediately after the awaited
buttplugClient.connect() in connectToServer(). If the provider was stopped while
the connection was pending, disconnect the newly established client and return
before logging success, clearing connection timers, or starting auto-scanning.
🪄 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 Plus
Run ID: 0ecf8733-2f5a-4165-8fe2-04dfa071d98c
📒 Files selected for processing (61)
src/app.tssrc/automation/scriptRuntime.tssrc/controller/settings/putSettingsController.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/buttplugIo/buttplugIoWebsocketDeviceProviderFactory.tssrc/device/protocol/estim2b/estim2bSerialDeviceProvider.tssrc/device/protocol/slvCtrlPlus/slvCtrlPlusSerialDeviceProvider.tssrc/device/protocol/virtual/virtualDeviceProvider.tssrc/device/protocol/virtual/virtualDeviceProviderFactory.tssrc/device/protocol/zc95/zc95SerialDeviceProvider.tssrc/device/provider/bleDeviceProvider.tssrc/device/provider/deviceProvider.tssrc/device/provider/deviceProviderFactory.tssrc/device/provider/deviceProviderManager.tssrc/device/provider/genericDeviceProviderFactory.tssrc/device/provider/serialDeviceProvider.tssrc/device/transport/bleObserver.tssrc/device/transport/serialPortObserver.tssrc/device/transport/sharedObserver.tssrc/device/updater/abstractDeviceUpdater.tssrc/device/updater/bufferedDeviceUpdater.tssrc/device/updater/deviceUpdaterInterface.tssrc/entity/deviceList.tssrc/repository/connectedDeviceRepository.tssrc/repository/deviceRepositoryInterface.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/serializedTypes.tssrc/settings/settings.tssrc/settings/settingsManager.tstests/integration/deviceEvents.spec.tstests/integration/devices/buttplugIoDevice.spec.tstests/integration/helpers/appHelper.tstests/unit/automation/scriptRuntime.spec.tstests/unit/device/deviceManager.spec.tstests/unit/device/protocol/buttplugIo/buttplugIoDevice.spec.tstests/unit/device/provider/deviceProvider.spec.tstests/unit/device/provider/deviceProviderManager.spec.tstests/unit/device/testDeviceProvider.tstests/unit/device/transport/bleObserver.spec.tstests/unit/device/transport/serialPortObserver.spec.tstests/unit/device/transport/sharedObserver.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 (3)
- tests/unit/device/testDeviceProvider.ts
- src/serviceProvider/controllerServiceProvider.ts
- src/serviceProvider/settingsServiceProvider.ts
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/automation/scriptRuntime.ts (1)
55-87: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard
load()against concurrent execution.
POST /automation/runcallsscriptRuntime.load()directly, and there’s no existingstop()/running-state check before starting a new script. Ifload()is entered while another script is already running, the previous VM isolate and log writer are replaced before teardown. Callstop()first or add a reentrancy guard that reuses/rejects the running script.🤖 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/automation/scriptRuntime.ts` around lines 55 - 87, Guard ScriptRuntime.load against concurrent execution by checking the existing running state before opening a new log writer or creating a VM. Reuse or reject the load request while a script is active, or invoke stop() first so the current VM and log writer are fully torn down before replacement; preserve the existing startup and failure cleanup behavior for non-concurrent loads.
🧹 Nitpick comments (1)
src/automation/scriptVmFactory.ts (1)
252-260: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a CPU/timeout limit to sandboxed script execution.
memoryLimit: 128bounds memory, butcompiledScript.run(vmContext, { promise: true })and the asyncReference.apply(...)calls inScriptVmcan execute arbitrary user code without atimeoutoption, so a synchronous infinite loop can leave the isolate thread pinning the awaiting caller. Pass a boundedtimeoutwhereisolated-vmpermits it for this execution path.🤖 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/automation/scriptVmFactory.ts` around lines 252 - 260, Add a bounded timeout to sandboxed user-code execution in the script factory and ScriptVm dispatch paths. Update compiledScript.run in the bootstrap flow and the async Reference.apply calls in ScriptVm to pass the timeout option supported by isolated-vm, while leaving bootstrap execution and existing promise behavior unchanged.
🤖 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/automation/scriptRuntime.ts`:
- Around line 55-87: Guard ScriptRuntime.load against concurrent execution by
checking the existing running state before opening a new log writer or creating
a VM. Reuse or reject the load request while a script is active, or invoke
stop() first so the current VM and log writer are fully torn down before
replacement; preserve the existing startup and failure cleanup behavior for
non-concurrent loads.
---
Nitpick comments:
In `@src/automation/scriptVmFactory.ts`:
- Around line 252-260: Add a bounded timeout to sandboxed user-code execution in
the script factory and ScriptVm dispatch paths. Update compiledScript.run in the
bootstrap flow and the async Reference.apply calls in ScriptVm to pass the
timeout option supported by isolated-vm, while leaving bootstrap execution and
existing promise behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8d257178-87e5-4aee-a543-b9371df9c6a1
📒 Files selected for processing (9)
src/automation/scriptRuntime.tssrc/automation/scriptVm.tssrc/automation/scriptVmFactory.tssrc/device/provider/deviceProviderManager.tssrc/serviceMap.tssrc/serviceProvider/automationServiceProvider.tssrc/serviceProvider/deviceServiceProvider.tstests/unit/automation/scriptRuntime.spec.tstests/unit/device/provider/deviceProviderManager.spec.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/unit/automation/scriptRuntime.spec.ts
- src/serviceProvider/deviceServiceProvider.ts
- tests/unit/device/provider/deviceProviderManager.spec.ts
- src/device/provider/deviceProviderManager.ts
…le-message prefix
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
This was referenced Jul 27, 2026
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