Add BLE support#72
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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:
📝 WalkthroughWalkthroughCentralizes device discovery/claiming in DeviceManager, introduces EventEmitter-driven device lifecycle and DeviceEvent, makes providers/devices DeviceManager-aware and generic, changes transport API from receive→onReceive with onClose/isOpen/close, wires EventEmitterFactory and loggers, and adds three BLE deps to package.json. Changes
Sequence Diagram(s)sequenceDiagram
participant SerialPortObserver as SerialPortObserver
participant DeviceManager as DeviceManager
participant SerialDeviceProvider as SerialDeviceProvider
participant Device as Device
SerialPortObserver->>DeviceManager: announceDetectedDevice(serialDeviceInfo)
activate DeviceManager
DeviceManager->>DeviceManager: enqueue detection, emit deviceDetected
DeviceManager->>SerialDeviceProvider: notify / acquireDetectedDevice(deviceId)
deactivate DeviceManager
activate SerialDeviceProvider
SerialDeviceProvider->>SerialDeviceProvider: connectSerialDevice(port)
SerialDeviceProvider->>Device: instantiate Device(EventEmitter, Logger)
SerialDeviceProvider->>DeviceManager: addDevice(device)
deactivate SerialDeviceProvider
DeviceManager->>DeviceManager: mark claimed, emit deviceConnected
Device->>Device: periodic refresh() -> emit deviceRefreshed
Device->>DeviceManager: on close -> emit deviceDisconnected
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can scan for known vulnerabilities in your dependencies using OSV Scanner.OSV Scanner will automatically detect and report security vulnerabilities in your project's dependencies. No additional configuration is required. |
|
@coderabbitai review please |
|
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
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)
94-101:⚠️ Potential issue | 🟠 Major
runForEvent()signature accepts an event type whose payload is incompatible with the method parameter.The method signature
runForEvent(eventType: keyof DeviceManagerEvent, device: Device)advertises support for allDeviceManagerEventkeys, includingdeviceAvailable(which has payloadDeviceInfo), but the parameter is typed asDevice. While no current callers forwarddeviceAvailable, this creates a type safety gap—the method cannot correctly handle all event types it claims to accept.Since automation only runs for connected-device events (
deviceConnected,deviceDisconnected,deviceRefreshed), narrow the type signature to excludedeviceAvailable:Suggested fix
- public runForEvent(eventType: keyof DeviceManagerEvent, device: Device): void + public runForEvent( + eventType: Exclude<keyof DeviceManagerEvent, 'deviceAvailable'>, + device: Device + ): void🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/automation/scriptRuntime.ts` around lines 94 - 101, The method runForEvent declares eventType as keyof DeviceManagerEvent but accepts a Device parameter while some DeviceManagerEvent keys (e.g., deviceAvailable) use a different payload type; change the signature to only accept the connected-device event keys that map to Device (e.g., narrow eventType to the union 'deviceConnected' | 'deviceDisconnected' | 'deviceRefreshed' or use keyof Pick<DeviceManagerEvent, 'deviceConnected'|'deviceDisconnected'|'deviceRefreshed'>) so the event payload type matches the device parameter, and update any callers of runForEvent if needed to use one of these narrowed keys; keep the rest of runForEvent (sandbox.event assignment) unchanged and ensure TypeScript types now align.
🧹 Nitpick comments (3)
package.json (2)
13-13: Pin the GitHub dependency to a specific commit or tag for reproducibility.The
bluetooth-numbers-databasedependency references the repository without a commit SHA or tag. Whilepackage-lock.jsoncurrently pins it to commit85317212ef563adcc57e575793a24e679c81b724, fresh installs or lock regeneration could pull different commits, leading to inconsistent builds.📦 Suggested fix to pin the commit
- "bluetooth-numbers-database": "github:NordicSemiconductor/bluetooth-numbers-database", + "bluetooth-numbers-database": "github:NordicSemiconductor/bluetooth-numbers-database#85317212ef563adcc57e575793a24e679c81b724",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 13, The GitHub dependency "bluetooth-numbers-database" is not pinned to a commit/tag which can cause non-reproducible installs; update the package.json entry for "bluetooth-numbers-database" to include the known commit SHA or a stable tag (e.g. change "github:NordicSemiconductor/bluetooth-numbers-database" to "github:NordicSemiconductor/bluetooth-numbers-database#85317212ef563adcc57e575793a24e679c81b724"), then run your package manager to regenerate package-lock.json and commit both package.json and package-lock.json so installs remain reproducible.
8-8: Note:@stoprocent/noblerequires native Bluetooth dependencies.This library requires platform-specific Bluetooth stack dependencies (e.g.,
libbluetooth-devon Linux, Xcode command line tools on macOS). Ensure these are documented in setup/installation instructions for developers and deployment environments.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 8, The dependency "@stoprocent/noble" requires native Bluetooth stack libraries, so update the repository setup/docs to list platform-specific prerequisites and install commands (e.g., libbluetooth-dev and build-essential for Debian/Ubuntu, Xcode command line tools for macOS, appropriate Windows Bluetooth SDKs), add a short note near the dependency reference in README/INSTALL describing these requirements, include any apt/brew/choco install commands and CI/container setup tips (e.g., Docker image packages or actions) and mention failure symptoms and how to verify Bluetooth libs are present to help developers and deployment automation.src/device/provider/deviceProvider.ts (1)
40-46: Consider emittingdeviceRefreshedonly after successful refresh.The
deviceRefreshedevent is emitted synchronously (line 44) immediately after starting the asyncrefreshData()call, not after it completes. This means:
- The event fires even if refresh fails
- Subscribers receive the event before the device data is actually updated
If this is intentional (to signal "refresh cycle occurred"), the current behavior is fine. Otherwise, consider emitting inside the
.then()callback.💡 Suggested change to emit after successful refresh
device.refreshData().then(() => device.updateLastRefresh()).catch( (e: Error) => this.logger.error(`device: ${device.getDeviceId} -> refreshData -> failed: ${e.message}`) - ); - - this.eventEmitter.emit(DeviceProviderEvent.deviceRefreshed, device); + ).then(() => { + this.eventEmitter.emit(DeviceProviderEvent.deviceRefreshed, device); + this.logger.trace(`Device refreshed: ${device.getDeviceId}`) + }); - - this.logger.trace(`Device refreshed: ${device.getDeviceId}`)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/device/provider/deviceProvider.ts` around lines 40 - 46, The deviceRefreshed event is emitted before the async refresh completes; move the this.eventEmitter.emit(DeviceProviderEvent.deviceRefreshed, device) and the this.logger.trace(`Device refreshed: ${device.getDeviceId}`) inside the success path of the device.refreshData() promise (i.e., within the .then() after device.updateLastRefresh()) so the event and trace only occur after refreshData() resolves successfully; keep the existing .catch(...) for logging failures and do not emit the event on error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/device/deviceManager.ts`:
- Around line 63-71: freeClaimedDevice currently shifts and resolves the entry
returned by shift(), which wakes the current owner instead of the next waiter;
change freeClaimedDevice to remove the current claimant first (call shift() to
discard the owner) and then resolve the next pending claimant (peek
deviceQueue[0] or deviceQueue.shift() for the next element) only if it exists;
reference the deviceClaimQueue map and the freeClaimedDevice method and ensure
you do not call resolve() on the removed entry so the next waiter is the one
resumed.
- Around line 79-80: The async listener that calls claimAvailableDevice() in
serialDeviceProvider.ts must not allow rejected Promises to become unhandled:
update the listener (the async deviceAvailable handler that awaits
claimAvailableDevice()) to catch rejections (wrap the await in try/catch) and
return a clear non-exception result (e.g., resolve to false) or otherwise handle
the losing case, or alternatively change claimAvailableDevice() to return a
non-throwing result (Promise<boolean>) and adjust callers accordingly; ensure
this handles the rejection path created by addDevice() which rejects entries
stored in deviceClaimQueue and references device.getDeviceId so no unhandled
promise rejection occurs.
In `@src/device/deviceManagerEvent.ts`:
- Around line 4-9: The interface DeviceManagerEvents is currently type-only and
removes the runtime value used by tests; add a runtime export named
DeviceManagerEvent (singular) containing the event name strings
(deviceConnected, deviceDisconnected, deviceRefreshed, deviceAvailable) so
value-space consumers can reference DeviceManagerEvent.deviceConnected, etc.,
while keeping the existing DeviceManagerEvents interface for typing; ensure the
runtime object keys match the interface keys exactly.
In `@src/device/provider/serialDeviceProvider.ts`:
- Around line 32-46: The async event handler registered on this.deviceManager
('deviceAvailable' callback) lacks try/catch and can produce unhandled promise
rejections from calls like claimAvailableDevice, connectToDevice, or addDevice;
wrap the handler body in a try { ... } catch (err) { ... } block, call
freeClaimedDevice(deviceInfo.id) in the catch if the device was claimed (or
after a failed connect), and log or surface the error via the existing logging
utility before returning so the promise rejection is handled; keep the existing
checks using isSerialDeviceInfo(deviceInfo), and ensure connectToDevice and
addDevice failures clean up by invoking freeClaimedDevice when appropriate.
---
Outside diff comments:
In `@src/automation/scriptRuntime.ts`:
- Around line 94-101: The method runForEvent declares eventType as keyof
DeviceManagerEvent but accepts a Device parameter while some DeviceManagerEvent
keys (e.g., deviceAvailable) use a different payload type; change the signature
to only accept the connected-device event keys that map to Device (e.g., narrow
eventType to the union 'deviceConnected' | 'deviceDisconnected' |
'deviceRefreshed' or use keyof Pick<DeviceManagerEvent,
'deviceConnected'|'deviceDisconnected'|'deviceRefreshed'>) so the event payload
type matches the device parameter, and update any callers of runForEvent if
needed to use one of these narrowed keys; keep the rest of runForEvent
(sandbox.event assignment) unchanged and ensure TypeScript types now align.
---
Nitpick comments:
In `@package.json`:
- Line 13: The GitHub dependency "bluetooth-numbers-database" is not pinned to a
commit/tag which can cause non-reproducible installs; update the package.json
entry for "bluetooth-numbers-database" to include the known commit SHA or a
stable tag (e.g. change "github:NordicSemiconductor/bluetooth-numbers-database"
to
"github:NordicSemiconductor/bluetooth-numbers-database#85317212ef563adcc57e575793a24e679c81b724"),
then run your package manager to regenerate package-lock.json and commit both
package.json and package-lock.json so installs remain reproducible.
- Line 8: The dependency "@stoprocent/noble" requires native Bluetooth stack
libraries, so update the repository setup/docs to list platform-specific
prerequisites and install commands (e.g., libbluetooth-dev and build-essential
for Debian/Ubuntu, Xcode command line tools for macOS, appropriate Windows
Bluetooth SDKs), add a short note near the dependency reference in
README/INSTALL describing these requirements, include any apt/brew/choco install
commands and CI/container setup tips (e.g., Docker image packages or actions)
and mention failure symptoms and how to verify Bluetooth libs are present to
help developers and deployment automation.
In `@src/device/provider/deviceProvider.ts`:
- Around line 40-46: The deviceRefreshed event is emitted before the async
refresh completes; move the
this.eventEmitter.emit(DeviceProviderEvent.deviceRefreshed, device) and the
this.logger.trace(`Device refreshed: ${device.getDeviceId}`) inside the success
path of the device.refreshData() promise (i.e., within the .then() after
device.updateLastRefresh()) so the event and trace only occur after
refreshData() resolves successfully; keep the existing .catch(...) for logging
failures and do not emit the event on error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ecd6a43d-c923-4486-9c07-a49b103a32a2
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (22)
package.jsonsrc/automation/scriptRuntime.tssrc/device/device.tssrc/device/deviceManager.tssrc/device/deviceManagerEvent.tssrc/device/peripheralDevice.tssrc/device/protocol/buttplugIo/buttplugIoWebsocketDeviceProvider.tssrc/device/protocol/buttplugIo/buttplugIoWebsocketDeviceProviderFactory.tssrc/device/protocol/estim2b/estim2bDevice.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/deviceProvider.tssrc/device/provider/deviceProviderFactory.tssrc/device/provider/deviceProviderLoader.tssrc/device/provider/genericDeviceProviderFactory.tssrc/device/provider/serialDeviceProvider.tssrc/device/transport/serialPortObserver.tssrc/index.tssrc/serviceProvider/deviceServiceProvider.ts
💤 Files with no reviewable changes (1)
- src/device/provider/deviceProviderLoader.ts
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 12
♻️ Duplicate comments (1)
src/device/provider/serialDeviceProvider.ts (1)
29-44:⚠️ Potential issue | 🟠 MajorCatch failures inside the
deviceAvailablelistener.This
asyncEventEmitter handler returns a promise nobody observes. IfclaimAvailableDevice()races oraddDevice()throws, the rejection becomes unhandled and any successful claim is never released.🛡️ Proposed fix
- this.deviceManager.on(DeviceManagerEvent.deviceAvailable, async (deviceInfo: DeviceInfo) => { + this.deviceManager.on(DeviceManagerEvent.deviceAvailable, async (deviceInfo: DeviceInfo): Promise<void> => { if (!this.isSerialDeviceInfo(deviceInfo)) { return; } - await this.deviceManager.claimAvailableDevice(deviceInfo.id); - - const device = await this.connectToDevice(deviceInfo.portInfo); - - if (undefined === device) { - this.deviceManager.freeClaimedDevice(deviceInfo.id); - return; - } - - this.deviceManager.addDevice(device); + try { + await this.deviceManager.claimAvailableDevice(deviceInfo.id); + } catch { + return; + } + + try { + const device = await this.connectToDevice(deviceInfo.portInfo); + if (undefined === device) { + this.deviceManager.freeClaimedDevice(deviceInfo.id); + return; + } + + this.deviceManager.addDevice(device); + } catch (e: unknown) { + this.deviceManager.freeClaimedDevice(deviceInfo.id); + this.logger.error(`Failed to connect serial device '${deviceInfo.id}'`, e as Error); + } });In Node.js `events.EventEmitter`, are rejected promises from `async` listeners handled automatically, or is `captureRejections` required?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/device/provider/serialDeviceProvider.ts` around lines 29 - 44, The event listener passed to this.deviceManager.on is async and its promise can reject unobserved; wrap the listener body in a try/catch (or attach a .catch) to handle all errors, ensure any successful claim is always released on failure, and log the error. Concretely, inside the DeviceManagerEvent.deviceAvailable handler (the function that calls isSerialDeviceInfo, claimAvailableDevice, connectToDevice, freeClaimedDevice, addDevice) introduce a boolean like "claimed" set after claimAvailableDevice resolves, then try { const device = await connectToDevice(...); if (!device) { freeClaimedDevice(id); return; } addDevice(device); } catch (err) { if (claimed) freeClaimedDevice(id); /* log or surface err via processLogger / this.logger */ } so no rejection escapes the listener and claims are always cleaned up.
🧹 Nitpick comments (2)
src/device/protocol/estim2b/estim2bDevice.ts (1)
84-90: Avoid rebuilding the attribute map on every poll.Line 86 replaces the mode-dependent attributes every 175ms, even when
currentModeis unchanged. That adds steady allocation churn and can invalidate cached attribute references for no state change. Rebuild only when the mode actually changes.♻️ Possible tweak
public async refresh(): Promise<void> { const status = await this.send(this.protocol.createGetStatusCommand()); - this.attributes = this.setModeBasedAttributes(status); + if (this.attributes.mode.value !== Int.from(status.currentMode)) { + this.attributes = this.setModeBasedAttributes(status); + } this.updateAttributeValues(status); await super.refresh(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/device/protocol/estim2b/estim2bDevice.ts` around lines 84 - 90, The refresh() method rebuilds the mode-dependent attributes every poll which causes allocation churn; change it to call setModeBasedAttributes only when the device mode actually changes by comparing the new mode from the status (e.g., status.mode or the relevant field) with the currentMode stored on the device, updating this.attributes and currentMode only on change, while always calling updateAttributeValues(status) and await super.refresh(); use the existing setModeBasedAttributes, attributes and currentMode symbols to locate and gate the rebuild.src/device/device.ts (1)
124-127: The baserefresh()method is a silent no-op for actual polling logic, creating a maintenance risk.Currently, if a device subclass doesn't override
refresh(), it silently inherits onlyupdateLastRefresh()with no active polling. While this works for event-driven devices like Zc95Device, it could hide implementation bugs in future polling-based devices. Consider either:
- Making
refresh()abstract to enforce explicit override- Implementing a template-method pattern with a protected
executeRefresh()hook that subclasses implement, while the base class handlesupdateLastRefresh()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/device/device.ts` around lines 124 - 127, The current public refresh() is a silent no-op (only calls updateLastRefresh()), which risks subclasses forgetting to implement polling; change refresh() to a template method that enforces subclass behavior: declare a protected abstract executeRefresh(): Promise<void> (or protected async executeRefresh(): Promise<void> in non-abstract base if you prefer) and implement public async refresh(): Promise<void> to await executeRefresh() inside a try/finally that always calls updateLastRefresh(); alternatively, if you want compile-time enforcement, make refresh() abstract so subclasses must implement it (use the class' abstract modifier and remove the base implementation).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/device/device.ts`:
- Line 5: The import for EventEmitter is from the wrong module; update the
import statement that currently imports EventEmitter from 'stream' to instead
import it from 'events' so it matches other usages and TypeScript/Node types
(look for the EventEmitter import at the top of src/device/device.ts and replace
the module source 'stream' with 'events').
In `@src/device/deviceManager.ts`:
- Around line 121-125: removeDevice currently deletes the device from
connectedDevices but does not stop the manager-owned refresh interval created by
initDeviceRefresher, so you must perform the same cleanup the device-emitted
disconnect path does: locate where initDeviceRefresher stores the per-device
timer (e.g., a map like this.deviceRefresherMap or a timer stored alongside
entries in this.connectedDevices), clearInterval for that timer and remove the
timer entry, remove any listeners set for that device, then delete from
this.connectedDevices and emit 'deviceDisconnected'; apply the identical fix to
the other removal spot referenced (the code around the 171-177 block) so both
code paths stop the refresher and remove listeners consistently.
- Around line 54-60: The addAvailableDevice method resets claim state by
unconditionally calling this.deviceClaimQueue.set(deviceInfo.id, []), which
drops existing owners/waiters used by claimAvailableDevice; change
addAvailableDevice so it updates/sets this.availableDevices as before but only
initializes this.deviceClaimQueue for deviceInfo.id if it does not already exist
(i.e., do not overwrite an existing queue), preserving any pending
promises/owners; keep emitting the 'deviceAvailable' event as-is.
In `@src/device/protocol/buttplugIo/buttplugIoWebsocketDeviceProvider.ts`:
- Around line 111-113: The provider adds devices to this.connectedDevices and
registers them via this.deviceManager.addDevice(device) on connect, but the
disconnect path only removes from this.connectedDevices; update the
disconnect/remove code paths (the block paired with this.connectedDevices.set
and the other removal block at 122-135) to also unregister with the manager by
calling this.deviceManager.removeDevice(device) (or the manager's corresponding
unregister method) and trigger the device-disconnected lifecycle (e.g., call
device.deviceDisconnected() or this.deviceManager.deviceDisconnected(device) if
that’s the manager API) so DeviceManager state and lifecycle hooks stay in sync.
In `@src/device/protocol/messageResponseHandler.ts`:
- Line 38: The transport close path currently only wires transport.onReceive to
this.onResponse, leaving pendingEntries unresolved; add a handler for
transport.onClose (or equivalent close event) that iterates pendingEntries,
rejects each pending promise with a TransportClosedError (or a clear Error
including transport info), and then clears pendingEntries so in-flight requests
fail fast; ensure you reference transport.onClose, pendingEntries, and the
existing onResponse wiring and unregister any listeners if needed.
In `@src/device/protocol/virtual/virtualDeviceProvider.ts`:
- Around line 84-85: The provider's removeDevice() currently only clears
provider-local maps but doesn't unregister the device from the shared
DeviceManager, leaving the virtual device visible after deletion; update
removeDevice() to also call the DeviceManager removal API (e.g.,
deviceManager.removeDevice(...) with the same identifier/object used in
deviceManager.addDevice(device)) after the local maps are cleared so the central
DeviceManager no longer lists or refreshes the removed virtual device.
In `@src/device/transport/deviceTransport.ts`:
- Around line 20-36: Update the type signatures and event registration so async
handlers are supported and their rejections are observed: change the interface
methods onReceive and onClose to accept callbacks of type (data: Buffer) => void
| Promise<void> or (=> Promise<void>) (pick one consistent choice), then update
SerialDeviceTransport.onReceive and SerialDeviceTransport.onClose to accept that
async signature and wrap the provided handler before passing to Node streams
(e.g. const wrapped = (...args) => Promise.resolve(handler(...args)).catch(err
=> /* log/forward error */); this ensures async data handlers (used by
zc95Device, messageResponseHandler, peripheralDevice) are awaited/handled and
errors are not silently dropped). Ensure the wrapper is used for both data and
close/end/close events and that any logging uses the transport's logger.
In `@src/serial/synchronousSerialPort.ts`:
- Around line 39-41: The onClose method currently registers an async callback
(() => Promise<void>) directly with the EventEmitter via this.writer.on('close',
callback) which will not be awaited and can cause unhandled rejections and it
only listens to the writer; change onClose to accept a synchronous callback type
(() => void) or keep the async signature but wrap the callback when registering
to catch and log/reject safely (e.g., (/*evt*/) => { callback().catch(err =>
/*handle*/); }); and register that wrapped handler on both this.writer and
this.reader 'close' events so the callback fires when either stream closes;
update the onClose signature and usage of writer/reader accordingly (refer to
onClose, this.writer, this.reader, and the 'close' event).
- Around line 47-51: The close() method currently calls writer.end() and then
immediately writer.destroy() and reader.destroy(), which can truncate buffered
writes; change close() to either wait for the writer's 'finish' event after
calling writer.end() before destroying the streams (i.e., attach a one-time
'finish' listener to writer to then destroy writer and reader and cancel/clear
the queue) or, if you intentionally want a forceful shutdown, skip writer.end()
and only call writer.destroy() and reader.destroy() after first
cancelling/clearing the queue to avoid mid-flight task failures; update
references in close(), writer.end(), writer.destroy(), reader.destroy(), and the
queue cancellation logic accordingly and ensure any event listeners are cleaned
up.
In `@src/util/async.ts`:
- Around line 40-60: The timeout promise currently resolves the race and allows
loop() to schedule the next iteration even though the original fn(...args) may
still be running; change the logic in loop so that after Promise.race(promises)
returns you always await the original fn promise (e.g., track it as fnPromise
from the call to fn(...args)) before scheduling the next timer, and use the
timeout only to call options.onError or abort (if an AbortController is
supplied) but not to signal that iteration is complete; update symbols
timeoutHandle, promises, fnPromise (or create one), and loop so the next
setTimeout(loop, options.intervalMs) is only set after fnPromise settles (or
after it is aborted), preventing overlapping executions.
- Around line 41-50: The timeout callback currently calls resolve() and then
throws an Error (making the throw run after the promise is already settled);
change the Promise constructor to (resolve, reject) and do not call resolve() in
the timeout path—instead create the Error and call options.onError(error) if
present, then call reject(error) (or just reject(error) if no onError),
replacing the stray throw; update references to timeoutHandle and
options.timeoutMs accordingly so the timeout branch properly rejects the
promise.
- Around line 27-34: The loop() helper currently rethrows errors from
fn(...args) when options.onError is undefined, which leads to unhandled rejected
promises because loop() is invoked fire-and-forget via void loop() and
setTimeout(loop, ...); fix this by ensuring all rejections are consumed: inside
loop() replace the throw in the fn(...args).catch handler with a safe fallback
(e.g., call a default error handler, log via options.logger or swallow) so the
promise resolves, and when invoking loop() (the void loop() call and
setTimeout(loop, ...)) ensure any returned promise is handled by appending
.catch(...) or wrapping the call in an async IIFE that catches errors; reference
symbols: loop, fn, options.onError, void loop(), and setTimeout(loop, ...).
---
Duplicate comments:
In `@src/device/provider/serialDeviceProvider.ts`:
- Around line 29-44: The event listener passed to this.deviceManager.on is async
and its promise can reject unobserved; wrap the listener body in a try/catch (or
attach a .catch) to handle all errors, ensure any successful claim is always
released on failure, and log the error. Concretely, inside the
DeviceManagerEvent.deviceAvailable handler (the function that calls
isSerialDeviceInfo, claimAvailableDevice, connectToDevice, freeClaimedDevice,
addDevice) introduce a boolean like "claimed" set after claimAvailableDevice
resolves, then try { const device = await connectToDevice(...); if (!device) {
freeClaimedDevice(id); return; } addDevice(device); } catch (err) { if (claimed)
freeClaimedDevice(id); /* log or surface err via processLogger / this.logger */
} so no rejection escapes the listener and claims are always cleaned up.
---
Nitpick comments:
In `@src/device/device.ts`:
- Around line 124-127: The current public refresh() is a silent no-op (only
calls updateLastRefresh()), which risks subclasses forgetting to implement
polling; change refresh() to a template method that enforces subclass behavior:
declare a protected abstract executeRefresh(): Promise<void> (or protected async
executeRefresh(): Promise<void> in non-abstract base if you prefer) and
implement public async refresh(): Promise<void> to await executeRefresh() inside
a try/finally that always calls updateLastRefresh(); alternatively, if you want
compile-time enforcement, make refresh() abstract so subclasses must implement
it (use the class' abstract modifier and remove the base implementation).
In `@src/device/protocol/estim2b/estim2bDevice.ts`:
- Around line 84-90: The refresh() method rebuilds the mode-dependent attributes
every poll which causes allocation churn; change it to call
setModeBasedAttributes only when the device mode actually changes by comparing
the new mode from the status (e.g., status.mode or the relevant field) with the
currentMode stored on the device, updating this.attributes and currentMode only
on change, while always calling updateAttributeValues(status) and await
super.refresh(); use the existing setModeBasedAttributes, attributes and
currentMode symbols to locate and gate the rebuild.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8cd86dcc-aaf1-4b30-983c-ffca580c25f9
📒 Files selected for processing (36)
eslint.config.tssrc/automation/scriptRuntime.tssrc/device/device.tssrc/device/deviceManager.tssrc/device/deviceManagerEvent.tssrc/device/genericDeviceUpdater.tssrc/device/peripheralDevice.tssrc/device/protocol/buttplugIo/buttplugIoDevice.tssrc/device/protocol/buttplugIo/buttplugIoDeviceFactory.tssrc/device/protocol/buttplugIo/buttplugIoWebsocketDeviceProvider.tssrc/device/protocol/buttplugIo/buttplugIoWebsocketDeviceProviderFactory.tssrc/device/protocol/estim2b/estim2bDevice.tssrc/device/protocol/estim2b/estim2bDeviceFactory.tssrc/device/protocol/estim2b/estim2bSerialDeviceProvider.tssrc/device/protocol/messageResponseHandler.tssrc/device/protocol/slvCtrlPlus/genericSlvCtrlPlusDevice.tssrc/device/protocol/slvCtrlPlus/slvCtrlPlusDevice.tssrc/device/protocol/slvCtrlPlus/slvCtrlPlusDeviceFactory.tssrc/device/protocol/slvCtrlPlus/slvCtrlPlusSerialDeviceProvider.tssrc/device/protocol/virtual/audio/piperVirtualDeviceLogic.tssrc/device/protocol/virtual/audio/ttsVirtualDeviceLogic.tssrc/device/protocol/virtual/genericVirtualDeviceFactory.tssrc/device/protocol/virtual/virtualDevice.tssrc/device/protocol/virtual/virtualDeviceProvider.tssrc/device/protocol/virtual/virtualDeviceProviderFactory.tssrc/device/protocol/zc95/zc95Device.tssrc/device/protocol/zc95/zc95DeviceFactory.tssrc/device/protocol/zc95/zc95SerialDeviceProvider.tssrc/device/provider/deviceProvider.tssrc/device/provider/serialDeviceProvider.tssrc/device/transport/deviceTransport.tssrc/device/transport/serialDeviceTransport.tssrc/index.tssrc/serial/synchronousSerialPort.tssrc/serviceProvider/deviceServiceProvider.tssrc/util/async.ts
💤 Files with no reviewable changes (1)
- src/device/deviceManagerEvent.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/index.ts
|
@coderabbitai please review the latest commit and also check the open comments. If something has been solved of your open suggestions by this commit, please close the conversation. :-) |
🧩 Analysis chain
🏁 Script executed: Length of output: 154147
🏁 Script executed: Length of output: 6299
With The architectural change is clean and actually stronger than my original suggested fix (which tracked refreshers in a separate map). Making the methods private enforces the invariant at the type level. I also noticed the broader API cleanup in the latest commit is excellent:
I've triggered a review of the latest commit to check any remaining open items. 🐇 ✅ Actions performedReview triggered.
|
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/provider/serialDeviceProvider.ts (1)
60-96:⚠️ Potential issue | 🟠 MajorAwait the port close operation before releasing the claim to prevent concurrent access.
The
port.close()call at line 87 is fire-and-forget (callback-based, not awaited). SinceconnectToDevice()returns immediately and the caller then releases the device claim at line 46, the next provider can begin opening the same port path while the previous file descriptor is still closing. This mirrors the same pattern correctly applied toport.open()at line 73-75.Possible fix
if (undefined === result) { if (port.isOpen) { - port.close(); + try { + await new Promise<void>((resolve, reject) => { + port.close(err => err ? reject(err) : resolve()); + }); + } catch (closeErr) { + this.logger.warn( + `Could not fully close serial device '${portInfo.path}' after failed connect: ${BaseError.normalize(closeErr).message}` + ); + } } this.logger.info(`Could not connect to serial device '${portInfo.path}': ${attemptFailureReason}`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/device/provider/serialDeviceProvider.ts` around lines 60 - 96, connectToDevice currently calls port.close() fire-and-for-get, which can allow a new claimant to open the same serial path before the descriptor is actually closed; change the close call in connectToDevice to await the close operation (wrap port.close callback in a Promise like the port.open usage), only call it if port.isOpen, and handle/log rejection so the promise never leaves an unawaited callback—apply this to the close path where result === undefined and keep existing logging behavior in connectToDevice / preparePort / connectSerialDevice.
♻️ Duplicate comments (1)
src/util/async.ts (1)
40-60:⚠️ Potential issue | 🔴 CriticalThis still stops via an unhandled rejected promise when
onErroris omitted.
throw errrejectsloop(), but both launch sites (void loop()andsetTimeout(loop, ...)) ignore that promise. The interval does stop, but it stops through an unobserved rejection instead of a controlled failure path. Either requireonErroror catch everyloop()invocation and stop explicitly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/util/async.ts` around lines 40 - 60, The loop() promise can reject (when options.onError is absent) and both call sites ignore that rejection; change both invocations so they handle rejections instead of letting an unhandled rejection stop the interval: replace the direct call sites for loop (the immediate launch at options.runImmediately and the delayed call via setTimeout with timer) to call loop().catch(err => { if (options.onError) options.onError(err); clearTimeout(timer); /* stop scheduling further runs */ }), and for the setTimeout case invoke loop inside the timer callback and attach the same .catch handler; this ensures the interval is stopped in a controlled way when loop rejects even if onError is not provided.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/device/deviceManager.ts`:
- Around line 56-66: The bug is that availableDeviceClaimQueue retains an empty
array after the final claimant calls shift(), preventing addAvailableDevice from
re-emitting deviceAvailable for the same id; in the release/claimant-removal
code (the method that calls shift() / releases a claimant — review the code
around lines 88-104 and any function handling claim releases), after removing a
claimant with shift() check if the array is now empty and if so call
availableDeviceClaimQueue.delete(deviceId) so the entry is removed; this ensures
addAvailableDevice(deviceInfo) will not early-return on has(deviceInfo.id) and
will emit deviceAvailable on rediscovery/unplug-replug.
- Around line 106-127: The addDevice -> claimAvailableDevice sequence is
fragile: listener exceptions thrown during deviceConnected emission can prevent
claimAvailableDevice from running and leave the device stuck in
connectedDevices. Fix by (1) making addDevice resilient to listener errors: wrap
the this.eventEmitter.emit('deviceConnected', device) call in a try/catch and
log the listener error instead of letting it propagate, and (2) defensively
update the success path in serialDeviceProvider (the code that calls
addDevice(...) then claimAvailableDevice(...)) to use a try/finally so
claimAvailableDevice(deviceId) is always executed even if addDevice throws;
reference the addDevice and claimAvailableDevice methods and the success path in
serialDeviceProvider when applying the changes.
---
Outside diff comments:
In `@src/device/provider/serialDeviceProvider.ts`:
- Around line 60-96: connectToDevice currently calls port.close()
fire-and-for-get, which can allow a new claimant to open the same serial path
before the descriptor is actually closed; change the close call in
connectToDevice to await the close operation (wrap port.close callback in a
Promise like the port.open usage), only call it if port.isOpen, and handle/log
rejection so the promise never leaves an unawaited callback—apply this to the
close path where result === undefined and keep existing logging behavior in
connectToDevice / preparePort / connectSerialDevice.
---
Duplicate comments:
In `@src/util/async.ts`:
- Around line 40-60: The loop() promise can reject (when options.onError is
absent) and both call sites ignore that rejection; change both invocations so
they handle rejections instead of letting an unhandled rejection stop the
interval: replace the direct call sites for loop (the immediate launch at
options.runImmediately and the delayed call via setTimeout with timer) to call
loop().catch(err => { if (options.onError) options.onError(err);
clearTimeout(timer); /* stop scheduling further runs */ }), and for the
setTimeout case invoke loop inside the timer callback and attach the same .catch
handler; this ensures the interval is stopped in a controlled way when loop
rejects even if onError is not provided.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 52911bbb-3f5c-4a3e-ac01-76a01e1d096a
📒 Files selected for processing (19)
src/device/device.tssrc/device/deviceManager.tssrc/device/protocol/buttplugIo/buttplugIoDeviceFactory.tssrc/device/protocol/estim2b/estim2bDeviceFactory.tssrc/device/protocol/messageResponseHandler.tssrc/device/protocol/slvCtrlPlus/slvCtrlPlusDeviceFactory.tssrc/device/protocol/virtual/genericVirtualDeviceFactory.tssrc/device/protocol/virtual/virtualDeviceProviderFactory.tssrc/device/protocol/zc95/zc95DeviceFactory.tssrc/device/provider/serialDeviceProvider.tssrc/factory/eventEmitterFactory.tssrc/serial/synchronousSerialPort.tssrc/serviceMap.tssrc/serviceProvider/automationServiceProvider.tssrc/serviceProvider/deviceServiceProvider.tssrc/serviceProvider/factoryServiceProvider.tssrc/serviceProvider/settingsServiceProvider.tssrc/util/async.tssrc/util/devNullStream.ts
💤 Files with no reviewable changes (1)
- src/util/devNullStream.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/device/protocol/estim2b/estim2bDeviceFactory.ts
- src/device/device.ts
- src/serial/synchronousSerialPort.ts
- src/device/protocol/messageResponseHandler.ts
- src/device/protocol/virtual/virtualDeviceProviderFactory.ts
…es if connection is lost
|
@coderabbitai please check latest commit, it should fix another few of the open discussions. Check the changes against the open discussions and tell me which ones you close because the latest commit fixed them. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 114959 🏁 Script executed: Length of output: 26266 🏁 Script executed: Length of output: 8099
✅ Closed by the latest commit1. Mirror 2. Mirror
|
|
@coderabbitai please review the newest commit. If this commit solves some of the open discussions, please resolve them and write a summary to this comment here with all closed discussions due to the latest commit. |
🧩 Analysis chainLet me check the latest commit and the current state of open discussions. 🏁 Script executed: Length of output: 104458 Let me resolve the two threads that the latest commit addresses. 🏁 Script executed: Length of output: 184
✅ Closed by this commit1. Async callback not awaited in The new this.writer.on('close', asyncHandler(callback, (e) => logError(this.logger, `Error in writer close handler`, e)));
this.reader.on('close', asyncHandler(callback, (e) => logError(this.logger, `Error in reader close handler`, e)));Both of the original concerns are now fixed: rejections are caught and logged via 2. Type signature mismatch / async callbacks not observed in The introduction of 🔴 Still open3. In this.deviceManager.addDevice(device); // emits deviceConnected synchronously
this.deviceManager.claimAvailableDevice(deviceInfo.id); // never runs if addDevice throwsThe try {
this.deviceManager.addDevice(device);
} finally {
this.deviceManager.claimAvailableDevice(deviceInfo.id);
}✅ Actions performedReview triggered.
|
🧩 Analysis chain🏁 Script executed: Length of output: 104501
✅ Closed by this commitNone of the previously unresolved discussions are newly closed by this commit — all other threads had already been resolved by prior commits. What this commit does bringThis commit is a strong architectural improvement:
🔴 Still open
The // serialDeviceProvider.ts
this.deviceManager.addDevice(device); // emits deviceConnected synchronously
this.deviceManager.claimAvailableDevice(deviceInfo.id); // skipped if addDevice() throwsIf a synchronous A try {
this.deviceManager.addDevice(device);
} finally {
this.deviceManager.claimAvailableDevice(deviceInfo.id);
}✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 5
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/virtual/virtualDeviceProvider.ts (1)
91-101:⚠️ Potential issue | 🟠 MajorEnsure provider-local cleanup still runs when
device.close()fails.Right now a thrown close aborts map cleanup and disconnect emission, leaving stale entries in
connectedDevices/attemptedDevices.🧩 Suggested fix (cleanup in finally)
private async removeDevice(device: Device): Promise<void> { const deviceId = device.getDeviceId; - - await device.close(); - this.connectedDevices.delete(deviceId); - this.attemptedDevices.delete(deviceId); - - this.eventEmitter.emit(DeviceProviderEvent.deviceDisconnected, device); - - this.logger.info(`Device removed: ${deviceId} (${device.getDeviceName})`); - this.logger.info(`Connected devices: ${this.connectedDevices.size.toString()}`); + try { + await device.close(); + } catch (e: unknown) { + this.logger.error(`Could not close virtual device '${deviceId}': ${(e as Error).message}`, e); + } finally { + this.connectedDevices.delete(deviceId); + this.attemptedDevices.delete(deviceId); + this.eventEmitter.emit(DeviceProviderEvent.deviceDisconnected, device); + this.logger.info(`Device removed: ${deviceId} (${device.getDeviceName})`); + this.logger.info(`Connected devices: ${this.connectedDevices.size.toString()}`); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/device/protocol/virtual/virtualDeviceProvider.ts` around lines 91 - 101, The removeDevice method currently calls device.close() and if that throws it prevents local cleanup and the disconnect event; change removeDevice (function removeDevice) to wrap device.close() in try/catch and perform the connectedDevices.delete(deviceId), attemptedDevices.delete(deviceId), eventEmitter.emit(DeviceProviderEvent.deviceDisconnected, device) and logger.info calls in a finally block so provider-local cleanup and event emission always run; in the catch log the close error (including device.getDeviceId / device.getDeviceName for context) via this.logger.error before rethrowing or continuing as appropriate.
♻️ Duplicate comments (1)
src/device/deviceManager.ts (1)
107-119:⚠️ Potential issue | 🟠 MajorUncaught
deviceConnectedlistener errors can still strand claim finalization.
emit()is synchronous; if a listener throws here, provider code can skip the subsequentclaimAvailableDevice(...)step and leave queue state hanging.#!/bin/bash # Verify unresolved addDevice -> claimAvailableDevice atomicity in serial provider. echo "=== deviceManager emit path ===" rg -nP --type=ts -C3 "eventEmitter\\.emit\\('deviceConnected'|eventEmitter\\.emit\\(\"deviceConnected\"" src/device/deviceManager.ts echo -e "\n=== serial provider occurrences ===" rg -nP --type=ts -C8 "\\baddDevice\\s*\\(|\\bclaimAvailableDevice\\s*\\(" src/device/provider/serialDeviceProvider.ts echo -e "\n=== check for try/finally around success path ===" rg -nP --type=ts -C8 "\\btry\\b|\\bfinally\\b" src/device/provider/serialDeviceProvider.ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/device/deviceManager.ts` around lines 107 - 119, addDevice currently calls this.eventEmitter.emit('deviceConnected', device) synchronously which can throw and prevent callers (e.g., serial provider) from completing the subsequent claimAvailableDevice flow; wrap the emit call in a try/catch, and on error do not rethrow synchronously — instead log/report the error and rethrow asynchronously (e.g., setImmediate(() => { throw err }) or process.nextTick) so the addDevice method returns normally and callers can proceed to claimAvailableDevice; update the addDevice method (around eventEmitter.emit), keeping existing calls to this.initDeviceRefresher, device.on(DeviceEvent...), refreshDevice and removeDevice unchanged.
🧹 Nitpick comments (2)
src/device/protocol/buttplugIo/slvCtrlPlusButtplugWebsocketClientConnector.ts (1)
1-1: Remove file-level eslint disable and refactoropenlistener to use sync wrapper with internal async IIFE.Line 1 suppresses the rule for the entire file, but only Line 18's async event handler actually triggers the violation. Replace the async handler with a sync wrapper that contains an internal async IIFE.
Proposed refactor
-/* eslint-disable `@typescript-eslint/no-misused-promises` */ import { ButtplugNodeWebsocketClientConnector } from 'buttplug'; @@ - ws.addEventListener('open', async (): Promise<void> => { - this._ws = ws; - try { - await this.initialize(); - this._ws.addEventListener('message', (msg) => { - this.parseIncomingMessage(msg); - }); - this._ws.removeEventListener('close', onCloseCallback); - this._ws.removeEventListener('error', onErrorCallback); - this._ws.addEventListener('close', this.disconnect); - resolve(); - } catch (e) { - reject(e); - } - }); + ws.addEventListener('open', (): void => { + void (async (): Promise<void> => { + this._ws = ws; + try { + await this.initialize(); + this._ws.addEventListener('message', (msg) => { + this.parseIncomingMessage(msg); + }); + this._ws.removeEventListener('close', onCloseCallback); + this._ws.removeEventListener('error', onErrorCallback); + this._ws.addEventListener('close', this.disconnect); + resolve(); + } catch (e) { + reject(e); + } + })(); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/device/protocol/buttplugIo/slvCtrlPlusButtplugWebsocketClientConnector.ts` at line 1, Remove the file-level eslint disable and refactor the async event handler for the "open" listener in slvCtrlPlusButtplugWebsocketClientConnector: delete the top-line "/* eslint-disable `@typescript-eslint/no-misused-promises` */", replace the async callback passed to the "open" event with a synchronous wrapper that immediately starts an internal async IIFE (e.g., socket.on("open", () => { (async () => { ... })().catch(err => { /* handle */ }); })), and ensure any thrown errors are caught/handled inside the IIFE so the listener is synchronous and no rule is violated.src/device/provider/serialDeviceProvider.ts (1)
99-99: Stale TODO comment should be addressed or removed.The TODO
// todo: freigeben!("release!") appears to be a leftover. The release logic is already handled by the caller inhandleDeviceDetection(line 56). Either remove this comment or clarify its intent if additional cleanup is needed here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/device/provider/serialDeviceProvider.ts` at line 99, Remove the stale TODO comment "// todo: freigeben!" in serialDeviceProvider.ts or replace it with a short clarifying comment; since release/cleanup is already performed by the caller handleDeviceDetection, either delete the vestigial TODO or update it to state that resource release is handled by handleDeviceDetection so future readers won't expect release logic in the serial device provider.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/device/device.ts`:
- Around line 149-152: The close() method currently emits
DeviceEvent.deviceDisconnected every time it's called; make close() idempotent
by adding a boolean flag (e.g. private _closed = false on the Device class) and
at the start of close() check if _closed is true and return early, otherwise set
_closed = true and then emit DeviceEvent.deviceDisconnected (and perform any
other cleanup once). Update the Device class state (not just emit) so subsequent
close() calls are no-ops.
In `@src/device/protocol/buttplugIo/buttplugIoWebsocketDeviceProvider.ts`:
- Around line 146-157: The removal flow currently only calls
this.connectedDevices.delete(buttplugDevice.index) after await device.close(),
so a thrown error leaves a stale entry; ensure the provider-local entry is
always removed by deleting this.connectedDevices.delete(buttplugDevice.index)
regardless of close() outcome (e.g., call delete before awaiting device.close()
or perform the delete in a finally block), keep the logger.error path intact to
record close() failures, and continue to use device.getDeviceId and
buttplugDevice.name/index values for logging.
- Around line 85-96: The interval refs (this.connectionIntervalRef,
this.autoScanningIntervalRef) are not being cleared to undefined after
clearInterval, causing subsequent init() calls to skip re-creating intervals due
to the ??= checks; update connectToServer() to set this.connectionIntervalRef =
undefined immediately after clearInterval(...) in that method, and update
handleLostConnection() to set both this.autoScanningIntervalRef = undefined and
this.connectionIntervalRef = undefined immediately after their
clearInterval(...) calls so future init() runs will recreate the intervals as
intended (refer to methods connectToServer(), handleLostConnection(), and
properties connectionIntervalRef/autoScanningIntervalRef).
In `@src/device/provider/serialDeviceProvider.ts`:
- Around line 60-61: Wrap the addDevice + claimAvailableDevice sequence in a
try/finally to ensure claimAvailableDevice(deviceInfo.id) always runs even if
this.deviceManager.addDevice(device) throws; specifically, call
this.deviceManager.addDevice(device) inside a try block and move
this.deviceManager.claimAvailableDevice(deviceInfo.id) into the finally block,
preserving and rethrowing any caught exception so callers still see failures.
Use the existing deviceManager, addDevice, claimAvailableDevice and deviceInfo
symbols to locate the code in serialDeviceProvider.ts.
In `@src/serial/synchronousSerialPort.ts`:
- Around line 41-44: The onClose implementation currently registers the same
callback on both this.writer and this.reader so shutdown that closes both
streams calls the callback twice; modify onClose (and the similar block at
50-55) to wrap the provided callback in a one-time guard: create a local
"called" flag (or use a once wrapper) and register the same wrapper function
with asyncHandler for both this.writer.on('close', ...) and
this.reader.on('close', ...); inside the wrapper ensure it only invokes the
original callback once, and after the first invocation remove the other listener
(or no-op subsequent calls) to prevent duplicate cleanup; reference onClose,
this.writer, this.reader, asyncHandler, and logError/logger when making the
change.
---
Outside diff comments:
In `@src/device/protocol/virtual/virtualDeviceProvider.ts`:
- Around line 91-101: The removeDevice method currently calls device.close() and
if that throws it prevents local cleanup and the disconnect event; change
removeDevice (function removeDevice) to wrap device.close() in try/catch and
perform the connectedDevices.delete(deviceId),
attemptedDevices.delete(deviceId),
eventEmitter.emit(DeviceProviderEvent.deviceDisconnected, device) and
logger.info calls in a finally block so provider-local cleanup and event
emission always run; in the catch log the close error (including
device.getDeviceId / device.getDeviceName for context) via this.logger.error
before rethrowing or continuing as appropriate.
---
Duplicate comments:
In `@src/device/deviceManager.ts`:
- Around line 107-119: addDevice currently calls
this.eventEmitter.emit('deviceConnected', device) synchronously which can throw
and prevent callers (e.g., serial provider) from completing the subsequent
claimAvailableDevice flow; wrap the emit call in a try/catch, and on error do
not rethrow synchronously — instead log/report the error and rethrow
asynchronously (e.g., setImmediate(() => { throw err }) or process.nextTick) so
the addDevice method returns normally and callers can proceed to
claimAvailableDevice; update the addDevice method (around eventEmitter.emit),
keeping existing calls to this.initDeviceRefresher, device.on(DeviceEvent...),
refreshDevice and removeDevice unchanged.
---
Nitpick comments:
In
`@src/device/protocol/buttplugIo/slvCtrlPlusButtplugWebsocketClientConnector.ts`:
- Line 1: Remove the file-level eslint disable and refactor the async event
handler for the "open" listener in slvCtrlPlusButtplugWebsocketClientConnector:
delete the top-line "/* eslint-disable `@typescript-eslint/no-misused-promises`
*/", replace the async callback passed to the "open" event with a synchronous
wrapper that immediately starts an internal async IIFE (e.g., socket.on("open",
() => { (async () => { ... })().catch(err => { /* handle */ }); })), and ensure
any thrown errors are caught/handled inside the IIFE so the listener is
synchronous and no rule is violated.
In `@src/device/provider/serialDeviceProvider.ts`:
- Line 99: Remove the stale TODO comment "// todo: freigeben!" in
serialDeviceProvider.ts or replace it with a short clarifying comment; since
release/cleanup is already performed by the caller handleDeviceDetection, either
delete the vestigial TODO or update it to state that resource release is handled
by handleDeviceDetection so future readers won't expect release logic in the
serial device provider.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 56baeeba-d669-4861-a7d7-7cf4b1133a39
📒 Files selected for processing (20)
eslint.config.tssrc/device/attribute/numberDeviceAttribute.tssrc/device/device.tssrc/device/deviceManager.tssrc/device/peripheralDevice.tssrc/device/protocol/buttplugIo/buttplugIoWebsocketDeviceProvider.tssrc/device/protocol/buttplugIo/slvCtrlPlusButtplugWebsocketClientConnector.tssrc/device/protocol/messageResponseHandler.tssrc/device/protocol/slvCtrlPlus/slvCtrlPlusDeviceFactory.tssrc/device/protocol/virtual/virtualDeviceProvider.tssrc/device/protocol/zc95/Zc95SerialReader.tssrc/device/protocol/zc95/zc95Device.tssrc/device/provider/serialDeviceProvider.tssrc/device/transport/deviceTransport.tssrc/entity/automationScript.tssrc/serial/frameParser.tssrc/serial/synchronousSerialPort.tssrc/util/async.tssrc/util/devNullStream.tssrc/util/error.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- eslint.config.ts
- src/device/transport/deviceTransport.ts
- src/util/async.ts
| try { | ||
| await device.close(); | ||
| this.connectedDevices.delete(buttplugDevice.index); | ||
|
|
||
| this.connectedDevices.delete(buttplugDevice.index); | ||
| this.logger.info(`Device removed: ${device.getDeviceId} (${buttplugDevice.name}@${buttplugDevice.index})`); | ||
| } catch (e: unknown) { | ||
| const error = BaseError.normalize(e); | ||
| this.logger.error( | ||
| `Could not remove device '${device.getDeviceId}' (${buttplugDevice.name}@${buttplugDevice.index}): ${error.message}`, | ||
| e | ||
| ); | ||
| } |
There was a problem hiding this comment.
Always remove provider-local device entry even if close() fails.
If close throws, the device stays in connectedDevices, which leaves stale state and repeated failed removals.
🧹 Suggested fix
- try {
- await device.close();
- this.connectedDevices.delete(buttplugDevice.index);
+ try {
+ await device.close();
@@
- } catch (e: unknown) {
+ } catch (e: unknown) {
const error = BaseError.normalize(e);
this.logger.error(
`Could not remove device '${device.getDeviceId}' (${buttplugDevice.name}@${buttplugDevice.index}): ${error.message}`,
e
);
+ } finally {
+ this.connectedDevices.delete(buttplugDevice.index);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/device/protocol/buttplugIo/buttplugIoWebsocketDeviceProvider.ts` around
lines 146 - 157, The removal flow currently only calls
this.connectedDevices.delete(buttplugDevice.index) after await device.close(),
so a thrown error leaves a stale entry; ensure the provider-local entry is
always removed by deleting this.connectedDevices.delete(buttplugDevice.index)
regardless of close() outcome (e.g., call delete before awaiting device.close()
or perform the delete in a finally block), keep the logger.error path intact to
record close() failures, and continue to use device.getDeviceId and
buttplugDevice.name/index values for logging.
🧩 Analysis chain🏁 Script executed: Length of output: 110206
✅ Closed by this commitNone. This commit is a naming/cleanup improvement rather than a bug fix. Specifically, it:
All good improvements — but none target the open bug threads. 🔴 Still open (5 threads)1. // These two need a try/finally guard:
this.deviceManager.addDevice(device); // emits deviceConnected synchronously
this.deviceManager.claimAvailableDevice(deviceInfo.id); // skipped if addDevice throws2. 3. 4. 5. ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/device/provider/serialDeviceProvider.ts (1)
99-99: Address or remove the TODO comment.The comment
// todo: freigeben!("release!" in German) suggests cleanup was intended here but not implemented. ThereleaseDetectedDevicecall at line 56 already handles the failure path, so this TODO may be stale. Please verify and either implement the missing logic or remove the comment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/device/provider/serialDeviceProvider.ts` at line 99, The TODO comment "// todo: freigeben!" in serialDeviceProvider.ts is stale—either remove it or implement the intended release/cleanup; verify that the existing releaseDetectedDevice helper is invoked on all failure and exit paths inside the same function (ensure calls to releaseDetectedDevice occur where devices/errors are cleaned up), and if any success path also needs explicit releasing implement that call; otherwise simply delete the comment to avoid misleading TODOs.src/device/protocol/zc95/zc95SerialDeviceProvider.ts (1)
78-84: Remove the commented-out code.The
// clearInterval(deviceStatusUpdaterInterval);comment references logic that has been moved toDeviceManager.initDeviceRefresher. This dead comment is confusing and should be removed.♻️ Proposed fix
port.on('close', () => { - // clearInterval(deviceStatusUpdaterInterval); this.connectedDevices.delete(device.getDeviceId); this.logger.info('Lost serial device: ' + device.getDeviceId); this.logger.info('Connected ZC95 serial devices: ' + this.connectedDevices.size.toString()); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/device/protocol/zc95/zc95SerialDeviceProvider.ts` around lines 78 - 84, Remove the stale commented-out line referencing deviceStatusUpdaterInterval inside the port.on('close', ...) handler in zc95SerialDeviceProvider.ts; specifically edit the port.on('close', () => { ... }) block (which contains this.connectedDevices.delete(device.getDeviceId) and logger.info calls) and delete the line "// clearInterval(deviceStatusUpdaterInterval);" so the handler no longer contains the dead comment that was moved to DeviceManager.initDeviceRefresher.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/device/protocol/zc95/zc95SerialDeviceProvider.ts`:
- Around line 78-84: Remove the stale commented-out line referencing
deviceStatusUpdaterInterval inside the port.on('close', ...) handler in
zc95SerialDeviceProvider.ts; specifically edit the port.on('close', () => { ...
}) block (which contains this.connectedDevices.delete(device.getDeviceId) and
logger.info calls) and delete the line "//
clearInterval(deviceStatusUpdaterInterval);" so the handler no longer contains
the dead comment that was moved to DeviceManager.initDeviceRefresher.
In `@src/device/provider/serialDeviceProvider.ts`:
- Line 99: The TODO comment "// todo: freigeben!" in serialDeviceProvider.ts is
stale—either remove it or implement the intended release/cleanup; verify that
the existing releaseDetectedDevice helper is invoked on all failure and exit
paths inside the same function (ensure calls to releaseDetectedDevice occur
where devices/errors are cleaned up), and if any success path also needs
explicit releasing implement that call; otherwise simply delete the comment to
avoid misleading TODOs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ef30333c-ee32-459a-bc53-8c71546e0ecb
📒 Files selected for processing (7)
src/device/deviceManager.tssrc/device/protocol/virtual/virtualDeviceProvider.tssrc/device/protocol/zc95/zc95SerialDeviceProvider.tssrc/device/provider/deviceProvider.tssrc/device/provider/deviceProviderEvent.tssrc/device/provider/serialDeviceProvider.tssrc/device/transport/serialPortObserver.ts
💤 Files with no reviewable changes (1)
- src/device/provider/deviceProviderEvent.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/device/transport/serialPortObserver.ts
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 129002
✅ Closed by this commitNone. Commit
All good improvements — but none address the open bug threads. 🔴 Still open (5 threads)1. this.deviceManager.addDevice(device); // still no try/finally guard
this.deviceManager.claimAvailableDevice(deviceInfo.id);A synchronous exception inside try {
this.deviceManager.addDevice(device);
} finally {
this.deviceManager.claimAvailableDevice(deviceInfo.id);
}2.
3. After 4.
5. Both |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/device/provider/serialDeviceProvider.ts (1)
46-61:⚠️ Potential issue | 🟠 MajorAlways settle the detected-device queue after a successful acquire.
Once
acquireDetectedDevice()returns success, every exit path needs to end in eitherreleaseDetectedDevice()orclaimAvailableDevice(). Right now a synchronous throw fromconnectToDevice()oraddDevice()only gets logged byasyncHandler, so the current queue entry is left behind and the remaining providers never get a turn.🛠️ Minimal fix
- const device = await this.connectToDevice(deviceInfo.portInfo); - - if (undefined === device) { - this.deviceManager.releaseDetectedDevice(deviceInfo.id); - return; - } - - this.deviceManager.addDevice(device); - this.deviceManager.claimAvailableDevice(deviceInfo.id); + let device: D | undefined; + try { + device = await this.connectToDevice(deviceInfo.portInfo); + } catch (err) { + this.deviceManager.releaseDetectedDevice(deviceInfo.id); + throw err; + } + + if (undefined === device) { + this.deviceManager.releaseDetectedDevice(deviceInfo.id); + return; + } + + try { + this.deviceManager.addDevice(device); + } finally { + this.deviceManager.claimAvailableDevice(deviceInfo.id); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/device/provider/serialDeviceProvider.ts` around lines 46 - 61, After a successful acquireDetectedDevice() call you must always settle the detected-device queue by calling either releaseDetectedDevice(deviceInfo.id) or claimAvailableDevice(deviceInfo.id) on deviceManager; update the logic around connectToDevice(deviceInfo.portInfo) and deviceManager.addDevice(device) so synchronous throws don’t skip that settlement: wrap connectToDevice and addDevice in a try/catch/finally (or use a try { ... } finally { ... }) that ensures claimAvailableDevice(deviceInfo.id) is called on success and releaseDetectedDevice(deviceInfo.id) is called on error, referencing the existing deviceManager methods (acquireDetectedDevice, releaseDetectedDevice, claimAvailableDevice) and the functions connectToDevice and addDevice to locate the code to modify.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/device/provider/serialDeviceProvider.ts`:
- Around line 46-61: After a successful acquireDetectedDevice() call you must
always settle the detected-device queue by calling either
releaseDetectedDevice(deviceInfo.id) or claimAvailableDevice(deviceInfo.id) on
deviceManager; update the logic around connectToDevice(deviceInfo.portInfo) and
deviceManager.addDevice(device) so synchronous throws don’t skip that
settlement: wrap connectToDevice and addDevice in a try/catch/finally (or use a
try { ... } finally { ... }) that ensures claimAvailableDevice(deviceInfo.id) is
called on success and releaseDetectedDevice(deviceInfo.id) is called on error,
referencing the existing deviceManager methods (acquireDetectedDevice,
releaseDetectedDevice, claimAvailableDevice) and the functions connectToDevice
and addDevice to locate the code to modify.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c00d1633-6170-4b27-a45f-606b22d9baf5
📒 Files selected for processing (7)
src/device/deviceManager.tssrc/device/protocol/virtual/virtualDeviceProvider.tssrc/device/protocol/zc95/zc95SerialDeviceProvider.tssrc/device/provider/deviceProvider.tssrc/device/provider/deviceProviderEvent.tssrc/device/provider/serialDeviceProvider.tssrc/device/transport/serialPortObserver.ts
💤 Files with no reviewable changes (1)
- src/device/provider/deviceProviderEvent.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/device/protocol/virtual/virtualDeviceProvider.ts
|
@coderabbitai with my most recent commit, all the problems should be fixed. Can you check any open discussions and close the ones that are resolved with the current code base committed and make a summary of the ones that are still unaddressed. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 135765 🏁 Script executed: Length of output: 92
✅ Closed by this commit
Both interval refs are now correctly nulled after clearing:
The 🔴 Still open (1 thread)
In try {
await device.close();
this.connectedDevices.delete(buttplugDevice.index); // ← skipped if close() throws
...
} catch (e: unknown) {
...
// stale entry remains in connectedDevices
}If try {
await device.close();
this.logger.info(`Device removed: ${device.getDeviceId} (${buttplugDevice.name}@${buttplugDevice.index})`);
} catch (e: unknown) {
const error = BaseError.normalize(e);
this.logger.error(
`Could not remove device '${device.getDeviceId}' (${buttplugDevice.name}@${buttplugDevice.index}): ${error.message}`,
e
);
} finally {
this.connectedDevices.delete(buttplugDevice.index);
} |
|
Closed in favor of #74 |
1 participant
Summary by CodeRabbit
New Features
Refactor
Chores