Add BLE support - #75
Merged
Merged
Conversation
|
Warning Review limit reached
Next review available in: 9 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughIntroduce branded DeviceId and BLE support (observer, transport, BleDevice), add Airotic protocol/provider, refactor protocol message types and attribute key/value typing, make JSON schema validator generic, normalize thrown errors, migrate many APIs to DeviceId, and bump linting/dependency/tooling. Changes
Sequence Diagram(s)sequenceDiagram
participant BleObserver as BleObserver
participant DeviceManager as DeviceManager
participant AiroticProvider as AiroticProvider
participant BleTransport as BleUartTransport
participant AiroticProtocol as AiroticProtocol
participant AiroticDevice as AiroticDevice
Note over BleObserver,DeviceManager: BLE discovery and provisioning
BleObserver->>DeviceManager: announceDetectedDevice(BleDeviceInfo{id: DeviceId, peripheral})
DeviceManager->>AiroticProvider: deviceDetected(BleDeviceInfo)
AiroticProvider->>DeviceManager: acquireDetectedDevice(DeviceId)
DeviceManager-->>AiroticProvider: AcquireResult(success)
AiroticProvider->>BleTransport: BleUartDeviceTransport.create(peripheral, rxUuid, txUuid)
BleTransport-->>AiroticProvider: transport
AiroticProvider->>AiroticProtocol: send(createHelloMessage()) (handshake)
AiroticProtocol-->>AiroticProvider: hello-response
AiroticProvider->>AiroticDevice: new AiroticDevice(transport, protocol, DeviceId)
AiroticProvider->>DeviceManager: addDevice(AiroticDevice)
AiroticProvider->>DeviceManager: claimDetectedDevice(DeviceId)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
✨ 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 |
Member
Author
|
@coderabbitai please review. I also have an issue with a the Airtonic device that after a automatic reconnect (noble connection drop) is stuck with |
🧩 Analysis chain🏁 Script executed: Length of output: 1837 🏁 Script executed: Length of output: 18954 🏁 Script executed: Length of output: 6326
🔴 Root Cause: Stuck Reconnect After Noble DropThe deadlock occurs because of a race condition between two competing reconnect paths combined with a missing state check in The sequence that causes the hang:The peripheral object from noble's Identified Issues1.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/device/attribute/intRangeDeviceAttribute.ts (1)
72-81:⚠️ Potential issue | 🟡 MinorMissing branded type conversion - raw number cast bypasses
Intvalidation.Unlike
IntDeviceAttribute.fromString()which correctly usesInt.from(num)to brand the parsed value, this method casts a rawnumberdirectly toT. This bypasses theIntbranding and could allow non-integer values (from floating-point edge cases inparseInt) to be treated asInt.🔧 Proposed fix to use branded Int
public fromString(value: string): T { const res = parseInt(value, 10); if (isNaN(res)) { throw new Error(`Could not convert '${value}' to a valid value for ${this.constructor.name}`); } // eslint-disable-next-line `@typescript-eslint/consistent-type-assertions` - return res as T; + return Int.from(res) as T; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/device/attribute/intRangeDeviceAttribute.ts` around lines 72 - 81, The fromString method in IntRangeDeviceAttribute currently casts the parsed number directly (return res as T), bypassing the branded Int type; change it to parse the integer then pass the numeric value through Int.from(...) (the same branding used in IntDeviceAttribute.fromString) before returning so the result preserves the Int brand and validation (update the fromString implementation to call Int.from(res) and return that branded value).src/device/protocol/zc95/zc95Device.ts (1)
155-181:⚠️ Potential issue | 🟠 MajorDon’t silently succeed when channel power values are still uninitialized.
If the first power-status frame has not arrived yet, this method returns early, but
setAttribute()still updates the refresh timestamp and returns the requested value as if the write succeeded. That drops the command and leaves both local and device state unchanged.🛑 Proposed fix
private async setAttributePowerChannel( attribute: DeviceAttributeOf<Zc95DevicePowerChannelAttributes>, value: number ): Promise<void> { if (!this.allPowerChannelValuesDefined(this.attributes)) { - return; + throw new Error('Cannot set power channel before all power channel values have been initialized'); } const tmpData: { [K in keyof Zc95DevicePowerChannelAttributes]-?: InitializedIntRangeDeviceAttribute['value'] } = { powerChannel1: this.attributes.powerChannel1.value, powerChannel2: this.attributes.powerChannel2.value,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/device/protocol/zc95/zc95Device.ts` around lines 155 - 181, The method setAttributePowerChannel currently returns early when allPowerChannelValuesDefined(this.attributes) is false, causing callers (e.g., setAttribute) to believe the write succeeded while the command was dropped; change this to surface an error instead of silently returning: inside setAttributePowerChannel, replace the early return with throwing a descriptive Error (or rejecting) indicating "power channel values uninitialized" so callers won’t update refresh timestamps or local values; keep using allPowerChannelValuesDefined to detect the condition and preserve the existing logic that builds the tmpData, calls msgFactory.createSetPower, uses messageResponseHandler.send and assertOkResponse when values are present.
🟡 Minor comments (8)
src/automation/scriptRuntime.ts-114-117 (1)
114-117:⚠️ Potential issue | 🟡 MinorRemove
console.errorfrom the VM error path (lint warning at Line 115).The new catch path introduces an ESLint
no-consolewarning. Use the existing runtime log/event flow only.🔧 Suggested patch
} catch (e: unknown) { const error = BaseError.normalize(e); - console.error(`VM stdout: ${error.message}`); - void this.log(error.message); - this.eventEmitter.emit(AutomationEventType.consoleLog, error.toString()); + const formattedError = error.toString(); + void this.log(formattedError); + this.eventEmitter.emit(AutomationEventType.consoleLog, formattedError); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/automation/scriptRuntime.ts` around lines 114 - 117, Remove the console.error call in the VM error catch path to eliminate the ESLint no-console warning; after normalizing the error with BaseError.normalize(e) keep using the runtime logging and event flow by calling this.log(error.message) (already present) and emitting the console log via this.eventEmitter.emit(AutomationEventType.consoleLog, error.toString()), so delete the console.error invocation and rely on BaseError.normalize, this.log, and eventEmitter.emit to surface the error.src/device/transport/bleDeviceTransport.ts-35-35 (1)
35-35:⚠️ Potential issue | 🟡 MinorReplace
console.errorwith the project's logger.Static analysis flags this as an unexpected console statement. Consider injecting a logger or using a logging utility consistent with the rest of the codebase (e.g.,
logErrorfrom../util/error.js).💡 Proposed fix
+import { logError } from '../../util/error.js'; +import Logger from '../../logging/Logger.js'; export default class BleUartDeviceTransport implements DeviceTransport { + private readonly logger: Logger; // ... in constructor or create(): - this.peripheral.on('connect', asyncHandler(async () => await this.subscribe(), console.error)); + this.peripheral.on('connect', asyncHandler( + async () => await this.subscribe(), + (e: unknown) => logError(this.logger, 'Error during BLE subscription', e) + ));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/device/transport/bleDeviceTransport.ts` at line 35, The handler registered on this.peripheral for 'connect' uses console.error (this.peripheral.on('connect', asyncHandler(async () => await this.subscribe(), console.error))) which violates the project's no-console rule; replace console.error with the project's logging utility (e.g., import and use logError from ../util/error.js or use an injected logger) so asyncHandler reports errors via the project logger instead of console, and update the import/constructor to provide the logger if necessary.src/device/device.ts-17-24 (1)
17-24:⚠️ Potential issue | 🟡 Minor
AttributeValueOfresolves attribute values against the baseDeviceAttributestype rather than the generic parameter.The type
AttributeValueOf<K>always infers fromDeviceAttributes[K](which isRecord<string, DeviceAttribute | undefined>), not from the generic type parameter passed toDeviceData<T>. WhenDeviceData<T extends DeviceAttributes>usesAttributeValueOf<K>with concrete types likeZc95DeviceAttributesthat define more specific value types (InitializedListDeviceAttribute<Int, string>,IntRangeDeviceAttribute, etc.), the resulting mapped type loses this specificity and resolves all values to the base type's genericDeviceAttribute | undefined. This causes type information loss wheneverDeviceData<T>is used with concrete device attribute types that have specialized attribute value types.To preserve concrete attribute types,
AttributeValueOfwould need to accept the attribute container type as a generic parameter (e.g.,AttributeValueOf<K, A extends DeviceAttributes>looking up inA[K]rather thanDeviceAttributes[K]).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/device/device.ts` around lines 17 - 24, AttributeValueOf currently looks up attribute types from the base DeviceAttributes instead of the generic container, losing concrete attribute specifics; change its signature to accept the container type (e.g., AttributeValueOf<K, A extends DeviceAttributes>) and resolve via InferAttributeValue<A[K]> (or similar) and then update all usages (notably in DeviceData<T>) to pass the concrete attribute type parameter so attribute lookups use the provided T rather than the global DeviceAttributes. Ensure AttributeKeyOf/DeviceAttributeOf remain compatible or are similarly parameterized if needed.src/settings/settings.ts-8-37 (1)
8-37:⚠️ Potential issue | 🟡 MinorRemove redundant
requiredarrays from Type.Object options.TypeBox automatically generates
requiredarrays in the JSON Schema based on which properties are wrapped withType.Optional(). The explicitrequiredarrays at lines 20, 31, and 36 are unnecessary—TypeBox does not recognize arequiredoption for Type.Object configuration. Properties are required by default unless wrapped withType.Optional(), which your schema already uses correctly (e.g.,serialNoat line 13).Remove the options objects containing
additionalProperties: falseandrequiredarrays, keeping only{ additionalProperties: false }. This aligns with how other schemas in the codebase (e.g.,piperVirtualDeviceConfigSchema,randomGeneratorVirtualDeviceConfigSchema) are defined.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/settings/settings.ts` around lines 8 - 37, The SettingsSchema currently passes explicit required arrays in the Type.Object options for the knownDevices entry object, the deviceSources entry object, and the top-level SettingsSchema; remove the redundant "required" keys and leave only "{ additionalProperties: false }" in each Type.Object options object so TypeBox's implicit required handling (properties are required unless wrapped with Type.Optional) is used; update the Type.Object calls for the knownDevices value object, the deviceSources value object, and the outer SettingsSchema to drop the required arrays while keeping additionalProperties: false.src/schemaValidation/JsonSchemaValidatorFactory.ts-19-22 (1)
19-22:⚠️ Potential issue | 🟡 MinorUnsafe type assertion on parsed JSON.
JSON.parse()returnsunknown, and casting directly toT(aTSchema) bypasses runtime validation. If the file contains malformed or unexpected JSON, this could cause subtle runtime errors downstream.Consider adding runtime validation or documenting this as a trusted-input-only API.
💡 Possible improvement
public createFromFile<T extends TSchema>(schemaFilePath: string): JsonSchemaValidator<T> { - const schemaData: T = JSON.parse(fs.readFileSync(schemaFilePath, 'utf-8')); + const schemaData = JSON.parse(fs.readFileSync(schemaFilePath, 'utf-8')) as T; + // Note: Assumes schemaFilePath contains a valid TypeBox-compatible schema return new JsonSchemaValidator(this.ajv, schemaData); }If this is only called with trusted internal schema files, consider adding a JSDoc comment clarifying the assumption.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/schemaValidation/JsonSchemaValidatorFactory.ts` around lines 19 - 22, The method createFromFile in JsonSchemaValidatorFactory unsafely casts JSON.parse result to T, bypassing runtime checks; change it to parse into unknown, then validate the parsed object with AJV (use this.ajv.validateSchema(schemaData) or this.ajv.compile if appropriate) and throw a clear error if validation fails before constructing JsonSchemaValidator<T>, or—if the method is guaranteed to only load trusted internal schema files—add a JSDoc note on that assumption; reference createFromFile, JsonSchemaValidator<T>, JsonSchemaValidatorFactory and this.ajv when making the change.src/device/protocol/airotic/airoticDevice.ts-45-46 (1)
45-46:⚠️ Potential issue | 🟡 MinorRemove the debug
console.log.This is tripping the current
no-consolewarning and bypasses the structured logger already available on the device. Usethis.logger.debug(...)if you still need the trace.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/device/protocol/airotic/airoticDevice.ts` around lines 45 - 46, Remove the debug console.log in the setAttribute method and use the device's structured logger instead: replace the console.log in public async setAttribute<K extends AttributeKeyOf<AiroticDeviceAttributes>, V extends AttributeValueOf<K>>(attributeName: K, value: V) with this.logger.debug(...) (or this.logger.info if appropriate), emitting a structured message that includes attributeName and value; ensure you import or reference the existing this.logger on the class and keep the same message content for traceability.src/device/protocol/airotic/airoticDeviceProvider.ts-77-77 (1)
77-77:⚠️ Potential issue | 🟡 MinorRemove the unused
protocolparameter to clear the lint failure.CI is already failing on Line 93 for
@typescript-eslint/no-unused-vars. Drop the parameter fromcreateDevice()and the extra argument at Line 77.🧹 Proposed fix
- this.deviceManager.addDevice(this.createDevice(deviceInfo, deviceInfo.peripheral, messageResponseHandler, protocol)); + this.deviceManager.addDevice(this.createDevice(deviceInfo, deviceInfo.peripheral, messageResponseHandler)); ... private createDevice( deviceInfo: BleDeviceInfo, peripheral: Peripheral, - messageResponseHandler: MessageResponseHandler<AiroticProtocol>, - protocol: AiroticProtocol + messageResponseHandler: MessageResponseHandler<AiroticProtocol> ): AiroticDeviceAlso applies to: 89-94
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/device/protocol/airotic/airoticDeviceProvider.ts` at line 77, The createDevice function currently accepts a protocol parameter that is unused and causes an eslint no-unused-vars error; remove the protocol parameter from the createDevice function signature and drop the extra protocol argument from all calls (e.g., the deviceManager.addDevice(this.createDevice(deviceInfo, deviceInfo.peripheral, messageResponseHandler, protocol)) invocation and the other createDevice(...) calls in the same file) so createDevice only receives the actual used args (deviceInfo, peripheral, messageResponseHandler); update any related references to match the new signature.src/device/protocol/messageResponseHandler.ts-91-97 (1)
91-97:⚠️ Potential issue | 🟡 MinorRemove the ad-hoc
console.logcalls from the send path.Lines 92 and 94 are already flagged by eslint in CI, and they bypass the structured logger this class already carries.
🧹 Proposed fix
if (false === this.isMessageWithResponse(msg)) { return promiseWithTimeout(new Promise<InferResponse<MR>>((resolve, reject) => { - console.log(`Sending message without expected response: ${encodedMsg.toString('utf-8')}`); this.transport.send(encodedMsg).then(() => { - console.log(`Message sent without expected response: ${encodedMsg.toString('utf-8')}`); // eslint-disable-next-line `@typescript-eslint/consistent-type-assertions` resolve(undefined as InferResponse<MR>); }).catch(reject); }), timeoutMs); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/device/protocol/messageResponseHandler.ts` around lines 91 - 97, Replace the ad-hoc console.log calls in the send path with the class's structured logger: remove the two console.log(...) lines inside the Promise in messageResponseHandler.ts (the block that calls this.transport.send(encodedMsg)) and replace them with the appropriate this.logger.debug or this.logger.info calls (match surrounding log level conventions) so logging uses the existing structured logger rather than console.
🧹 Nitpick comments (5)
src/device/deviceManager.ts (2)
50-52: Remove commented-out dead code.The commented-out condition
/* || this.connectedDevices.has(deviceInfo.id)*/is superseded by the active check on lines 54-57 which does the same thing with proper logging. The commented code should be removed for clarity.♻️ Proposed fix
- if (this.detectedDeviceAcquireQueue.has(deviceInfo.id)/* || this.connectedDevices.has(deviceInfo.id)*/) { + if (this.detectedDeviceAcquireQueue.has(deviceInfo.id)) { return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/device/deviceManager.ts` around lines 50 - 52, Remove the dead commented condition from the early-return check in the device acquisition path: delete the commented fragment "/* || this.connectedDevices.has(deviceInfo.id)*/" inside the if that checks this.detectedDeviceAcquireQueue.has(deviceInfo.id). The active connectedDevices check is already implemented later with proper logging (see symbols detectedDeviceAcquireQueue and connectedDevices), so just remove the commented code to improve clarity without changing behavior.
38-38: Consider usingDeviceIdas the Map key type for consistency.The
detectedDeviceAcquireQueueusesstringas its key type while the API methods now acceptDeviceId. IfDeviceIdis a branded string type, this works at runtime but loses type safety. Updating toMap<DeviceId, ...>would maintain consistency with the broaderDeviceIdmigration.- private readonly detectedDeviceAcquireQueue: Map<string, { resolve: (value: AcquireResult) => void }[]> = new Map(); + private readonly detectedDeviceAcquireQueue: Map<DeviceId, { resolve: (value: AcquireResult) => void }[]> = new Map();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/device/deviceManager.ts` at line 38, The Map detectedDeviceAcquireQueue is typed with string keys but the rest of the API uses DeviceId, so change its declaration to Map<DeviceId, { resolve: (value: AcquireResult) => void }[]> and update any usages (look for accesses to detectedDeviceAcquireQueue.get/set/has/delete) to accept DeviceId instead of string; import or reference the DeviceId type where declared and adjust any places that currently pass plain strings (cast or convert to DeviceId as appropriate) so key types remain consistent and type-safe across the class.src/device/protocol/virtual/virtualDevice.ts (1)
66-87: Consider simplifying with async/await.The
setAttributemethod wraps synchronous logic in aPromiseconstructor. Since there's no actual async operation, this could be simplified usingasync/awaitwith direct returns/throws, matching the pattern in other device implementations.♻️ Simplified version
public async setAttribute< K extends AttributeKeyOf<ExtractAttributes<TLogic>> - >(attributeName: K, value: AttributeValueOf<K>): Promise<AttributeValueOf<K>> { - return new Promise<AttributeValueOf<K>>((resolve, reject) => { - this.state = DeviceState.busy; - - const attribute = this.attributes[attributeName]; - - if (undefined === attribute || null === attribute) { - reject(new Error( - `Attribute named "${attributeName}" does not exist for device with id "${this.deviceId}"` - )); - return; - } - - attribute.value = value; - - this.state = DeviceState.ready; - - resolve(value); - }); + >(attributeName: K, value: AttributeValueOf<K>): Promise<AttributeValueOf<K>> { + this.state = DeviceState.busy; + + const attribute = this.attributes[attributeName]; + + if (undefined === attribute || null === attribute) { + throw new Error( + `Attribute named "${attributeName}" does not exist for device with id "${this.deviceId}"` + ); + } + + attribute.value = value; + + this.state = DeviceState.ready; + + return value; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/device/protocol/virtual/virtualDevice.ts` around lines 66 - 87, The setAttribute method wraps synchronous logic in a new Promise; convert it to a plain async function that throws on missing attribute and returns the value directly: change the signature of setAttribute to be async, look up the attribute via this.attributes[attributeName], if attribute is null/undefined throw new Error(`Attribute named "${attributeName}" does not exist for device with id "${this.deviceId}"`), set this.state = DeviceState.busy before mutation, assign attribute.value = value, set this.state = DeviceState.ready, and finally return value (preserving the generic return type AttributeValueOf<K>); keep references to AttributeKeyOf, AttributeValueOf, DeviceState, attributes, and deviceId to locate the code.src/device/transport/bleDeviceTransport.ts (1)
86-88: Stubbed method will throw at runtime if called.
sendAndAwaitReceivethrows'Method not implemented.'. If this transport is used in a context that expects this method to work, it will fail. Consider either implementing it or documenting it's intentionally unsupported for this transport type.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/device/transport/bleDeviceTransport.ts` around lines 86 - 88, The stubbed sendAndAwaitReceive method currently throws synchronously and will crash callers; replace the throw with a proper Promise rejection that clearly states the method is unsupported on this transport (e.g. return Promise.reject(new Error('sendAndAwaitReceive not supported by BleDeviceTransport'))) or, if BLE request/response semantics are required, implement it inside the BleDeviceTransport class by using the existing send(...) method and wiring up the BLE notification/response listener (subscribe to the response characteristic, send(data), await the response with a timeout, then resolve the Promise) so callers get a proper async rejection or a resolved Buffer instead of a runtime throw.src/device/provider/genericDeviceProviderFactory.ts (1)
4-13: Constructor parameter types are erased.
ConcreteCtor<T>fixes parameters toany[], soConstructorParameters<ConcreteCtor<DP>>also becomesany[]. This allows invalid provider registrations to type-check. Make the factory generic over the constructor type itself to preserve and enforce parameter types:export default class GenericDeviceProviderFactory<C extends new (...args: any[]) => DP, DP extends DeviceProvider> implements DeviceProviderFactory<DP> { public constructor(private readonly ctor: C, private readonly args: ConstructorParameters<C>) {} public create(): DP { return new this.ctor(...this.args); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/device/provider/genericDeviceProviderFactory.ts` around lines 4 - 13, The constructor currently uses ConcreteCtor<T> which erases parameter types to any[], so ConstructorParameters<ConcreteCtor<DP>> becomes any[] and allows invalid registrations; make the class generic over the concrete constructor type instead (e.g., introduce a type parameter C extends new (...args: any[]) => DP) and change the ctor and args fields and constructor signature to use C and ConstructorParameters<C>, then adjust create() to instantiate via new this.ctor(...this.args) so parameter types are preserved and enforced; update references to GenericDeviceProviderFactory, ConcreteCtor, ctor, args, and the constructor accordingly.
🤖 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/controller/patchDeviceController.ts`:
- Around line 34-39: In the catch block of patchDeviceController (where
BaseError.normalize(e) is used), fix the broken response by replacing the
incorrect chaining res.send(error.message).sendStatus(500) with a single
response call that sets status and body together (e.g.,
res.status(500).send(error.message) or res.status(500).json({ error:
error.message })), and add an immediate return after sending the error so
execution does not reach the subsequent res.sendStatus(202).
In `@src/device/bleDevice.ts`:
- Around line 48-65: The reconnect handler (reconnectHandler) races with
BleObserver re-discovery: when promiseWithTimeout(peripheral.connectAsync())
times out we call await this.close() and remove the device from connectedDevices
while the peripheral may still be in 'connecting' state, allowing BleObserver to
re-announce and BleUartDeviceTransport.subscribe() to hang. Fix by either
removing this internal reconnectHandler and letting BleObserver handle
reconnection, or implement an isReconnecting boolean on the BLE device instance
that is set true before starting the reconnect attempt and false after
success/failure; when isReconnecting is true, have BleObserver skip
re-announcing this peripheral (or have subscribe() early-return), ensure you
clear the flag and call peripheral.off('disconnect', reconnectHandler) and
clearInterval(rssiInterval) only after final failure, and normalize all error
handling around BaseError.normalize(e) so state transitions are atomic and
race-free.
- Around line 43-65: The rssiInterval and reconnectHandler are only removed on
failed reconnect, leaving the interval and listener active after normal close;
make them instance-scoped and ensure they're cleaned up in the device close path
(doClose() or close()). Concretely: assign the interval and handler to
properties (e.g., this.rssiInterval and this.reconnectHandler) instead of local
vars, use peripheral.off('disconnect', this.reconnectHandler) and
clearInterval(this.rssiInterval) during doClose()/close(), and ensure any
existing listener/interval are guarded/cleared before reassigning to avoid
duplicates; reference rssiInterval, reconnectHandler, requestRssiUpdate, and the
class's close()/doClose() to locate where to change.
In `@src/device/protocol/airotic/airoticDevice.ts`:
- Around line 48-53: Validate and normalize the RGB input before sending BLE
commands: in the restColor branch (and the other color branch around the same
area), parse the string into exactly three components, ensure each component is
an integer between 0 and 255 (reject or throw/return early for malformed inputs
like wrong count, non-numeric, or out-of-range values), then call
this.messageResponseHandler.send(AiroticProtocol.createSelectRestColorMessage())
and await sleep(100) and
this.messageResponseHandler.send(AiroticProtocol.createSetColorMessage(r,g,b));
only after both sends succeed assign the original valid string (or a normalized
"r,g,b" form) into this.attributes['restColor'] (or the corresponding attribute
for the other branch) so in-memory state is updated; do not send
createSetColorMessage with NaN/undefined values.
In `@src/device/protocol/airotic/airoticDeviceProvider.ts`:
- Around line 62-66: The BleUartDeviceTransport.create call can hang; wrap the
create call in the same timeout protection used by the reconnect path so it
rejects if rediscovery stalls. Specifically, replace the direct call to
BleUartDeviceTransport.create(deviceInfo.peripheral,
AiroticDeviceProvider.UART_RX_CHAR_UUID,
AiroticDeviceProvider.UART_TX_CHAR_UUID) with the project’s timeout-wrapped
promise (the same helper used in the reconnect logic), ensure the wrapper
rejects on timeout, and keep the existing claimDetectedDevice() /
releaseDetectedDevice() flow (so releaseDetectedDevice() still runs if the
create times out).
- Around line 62-85: When handshake fails (handshakeResult false) or in the
catch block you must cleanly close the opened BLE session before releasing
ownership; after creating transport via BleUartDeviceTransport.create and/or
MessageResponseHandler.create, ensure you call the
transport/messageResponseHandler shutdown API (e.g. await transport.close() or
transport.disconnect(), and/or messageResponseHandler.close()/dispose() as
available) guarded by a null check so you only close when created, and catch/log
any error from the close; perform that close prior to calling
this.deviceManager.releaseDetectedDevice(deviceInfo.id).
- Around line 57-66: The handler currently ignores the ownership result from
deviceManager.acquireDetectedDevice(deviceInfo.id) and always proceeds to
connect; change it to capture the returned boolean (e.g., const successful =
await this.deviceManager.acquireDetectedDevice(deviceInfo.id)) and if successful
is false return early (abort the handler) so you do not attempt
BleUartDeviceTransport.create when ownership wasn't granted; ensure this check
is placed before calling BleUartDeviceTransport.create and any further connect
logic.
In `@src/device/protocol/slvCtrlPlus/slvCtrlProtocol.ts`:
- Around line 38-39: In isResponseMatchingMessage, the comparison uses
this.encode(message.message).toString() which preserves line terminators and can
fail to match echoed replies; replace the raw toString() usage by encoding the
message into a buffer (via this.encode(message.message)), converting with
toString('utf-8') and trimming trailing terminators (e.g., .trimEnd()) before
comparing to response.command so the echoed full command including args matches
correctly.
In `@src/device/protocol/zc95/zc95Device.ts`:
- Around line 134-138: The isNaN check is inverted: when extracting menuItemId
from patternDetailAttr.name (using parseInt with
Zc95Device.patternAttributePrefix removed) valid numeric IDs currently throw;
change the guard to throw only when menuItemId is NaN. Specifically, in the
block that computes const menuItemId = parseInt(...), replace the condition if
(!isNaN(menuItemId)) throw ... with if (isNaN(menuItemId)) throw ... so only
invalid names (non-numeric IDs) raise the error and valid names like
patternAttribute3 proceed.
In `@src/device/transport/bleDeviceTransport.ts`:
- Around line 41-56: The subscribe() method incorrectly only checks for
peripheral.state === 'disconnected', so when the peripheral is in 'connecting'
or other non-'connected' intermediate states the code proceeds to call
discoverSomeServicesAndCharacteristicsAsync() and can hang; change the guard to
ensure the peripheral is fully connected before service discovery (e.g., if
(this.peripheral.state !== 'connected') await this.peripheral.connectAsync() or
wait for the state to become 'connected'), keeping the isSubscribing flag
behavior and preserving error handling around connectAsync() and
discoverSomeServicesAndCharacteristicsAsync() in the subscribe() method.
In `@src/device/transport/bleObserver.ts`:
- Around line 50-51: onDiscover is re-announcing every advertisement because
announcedDevices bookkeeping was commented out; restore and use the
announcedDevices map to suppress duplicates: before calling
deviceManager.announceDetectedDevice(peripheral) check
announcedDevices.has(peripheral.id) and only announce if absent, then
immediately do announcedDevices.set(peripheral.id, peripheral). Also ensure
announcedDevices entries are cleared when a device is removed or after a
completed reconnect attempt (adjust the disconnect/reconnect handling where
devices are dropped) so future legitimate re-announces can proceed.
In `@src/index.ts`:
- Line 132: The two fire-and-forget calls serialPortObserver.init() and
bleObserver.init() swallow rejections; change them to handle errors explicitly
by either awaiting them inside the surrounding async startup function or
attaching .catch handlers that log and surface failures (e.g., use await
serialPortObserver.init() / await bleObserver.init() in the async init, or
serialPortObserver.init().catch(err => logger.error(...)) and likewise for
bleObserver) so any initialization rejection is logged and can trigger
appropriate shutdown/retry logic.
In `@src/util/async.ts`:
- Around line 97-100: The Promise.race call only clears timeoutHandle on
resolve, leaving the timer active if promise rejects; update the
Promise.race([promise, timeoutPromise]) handling to clearTimeout(timeoutHandle)
in both success and failure paths (e.g., use .finally or attach both .then and
.catch) so timeoutHandle is always cleared when either promise settles,
referencing the existing promise, timeoutPromise, timeoutHandle and the
Promise.race call.
---
Outside diff comments:
In `@src/device/attribute/intRangeDeviceAttribute.ts`:
- Around line 72-81: The fromString method in IntRangeDeviceAttribute currently
casts the parsed number directly (return res as T), bypassing the branded Int
type; change it to parse the integer then pass the numeric value through
Int.from(...) (the same branding used in IntDeviceAttribute.fromString) before
returning so the result preserves the Int brand and validation (update the
fromString implementation to call Int.from(res) and return that branded value).
In `@src/device/protocol/zc95/zc95Device.ts`:
- Around line 155-181: The method setAttributePowerChannel currently returns
early when allPowerChannelValuesDefined(this.attributes) is false, causing
callers (e.g., setAttribute) to believe the write succeeded while the command
was dropped; change this to surface an error instead of silently returning:
inside setAttributePowerChannel, replace the early return with throwing a
descriptive Error (or rejecting) indicating "power channel values uninitialized"
so callers won’t update refresh timestamps or local values; keep using
allPowerChannelValuesDefined to detect the condition and preserve the existing
logic that builds the tmpData, calls msgFactory.createSetPower, uses
messageResponseHandler.send and assertOkResponse when values are present.
---
Minor comments:
In `@src/automation/scriptRuntime.ts`:
- Around line 114-117: Remove the console.error call in the VM error catch path
to eliminate the ESLint no-console warning; after normalizing the error with
BaseError.normalize(e) keep using the runtime logging and event flow by calling
this.log(error.message) (already present) and emitting the console log via
this.eventEmitter.emit(AutomationEventType.consoleLog, error.toString()), so
delete the console.error invocation and rely on BaseError.normalize, this.log,
and eventEmitter.emit to surface the error.
In `@src/device/device.ts`:
- Around line 17-24: AttributeValueOf currently looks up attribute types from
the base DeviceAttributes instead of the generic container, losing concrete
attribute specifics; change its signature to accept the container type (e.g.,
AttributeValueOf<K, A extends DeviceAttributes>) and resolve via
InferAttributeValue<A[K]> (or similar) and then update all usages (notably in
DeviceData<T>) to pass the concrete attribute type parameter so attribute
lookups use the provided T rather than the global DeviceAttributes. Ensure
AttributeKeyOf/DeviceAttributeOf remain compatible or are similarly
parameterized if needed.
In `@src/device/protocol/airotic/airoticDevice.ts`:
- Around line 45-46: Remove the debug console.log in the setAttribute method and
use the device's structured logger instead: replace the console.log in public
async setAttribute<K extends AttributeKeyOf<AiroticDeviceAttributes>, V extends
AttributeValueOf<K>>(attributeName: K, value: V) with this.logger.debug(...) (or
this.logger.info if appropriate), emitting a structured message that includes
attributeName and value; ensure you import or reference the existing this.logger
on the class and keep the same message content for traceability.
In `@src/device/protocol/airotic/airoticDeviceProvider.ts`:
- Line 77: The createDevice function currently accepts a protocol parameter that
is unused and causes an eslint no-unused-vars error; remove the protocol
parameter from the createDevice function signature and drop the extra protocol
argument from all calls (e.g., the
deviceManager.addDevice(this.createDevice(deviceInfo, deviceInfo.peripheral,
messageResponseHandler, protocol)) invocation and the other createDevice(...)
calls in the same file) so createDevice only receives the actual used args
(deviceInfo, peripheral, messageResponseHandler); update any related references
to match the new signature.
In `@src/device/protocol/messageResponseHandler.ts`:
- Around line 91-97: Replace the ad-hoc console.log calls in the send path with
the class's structured logger: remove the two console.log(...) lines inside the
Promise in messageResponseHandler.ts (the block that calls
this.transport.send(encodedMsg)) and replace them with the appropriate
this.logger.debug or this.logger.info calls (match surrounding log level
conventions) so logging uses the existing structured logger rather than console.
In `@src/device/transport/bleDeviceTransport.ts`:
- Line 35: The handler registered on this.peripheral for 'connect' uses
console.error (this.peripheral.on('connect', asyncHandler(async () => await
this.subscribe(), console.error))) which violates the project's no-console rule;
replace console.error with the project's logging utility (e.g., import and use
logError from ../util/error.js or use an injected logger) so asyncHandler
reports errors via the project logger instead of console, and update the
import/constructor to provide the logger if necessary.
In `@src/schemaValidation/JsonSchemaValidatorFactory.ts`:
- Around line 19-22: The method createFromFile in JsonSchemaValidatorFactory
unsafely casts JSON.parse result to T, bypassing runtime checks; change it to
parse into unknown, then validate the parsed object with AJV (use
this.ajv.validateSchema(schemaData) or this.ajv.compile if appropriate) and
throw a clear error if validation fails before constructing
JsonSchemaValidator<T>, or—if the method is guaranteed to only load trusted
internal schema files—add a JSDoc note on that assumption; reference
createFromFile, JsonSchemaValidator<T>, JsonSchemaValidatorFactory and this.ajv
when making the change.
In `@src/settings/settings.ts`:
- Around line 8-37: The SettingsSchema currently passes explicit required arrays
in the Type.Object options for the knownDevices entry object, the deviceSources
entry object, and the top-level SettingsSchema; remove the redundant "required"
keys and leave only "{ additionalProperties: false }" in each Type.Object
options object so TypeBox's implicit required handling (properties are required
unless wrapped with Type.Optional) is used; update the Type.Object calls for the
knownDevices value object, the deviceSources value object, and the outer
SettingsSchema to drop the required arrays while keeping additionalProperties:
false.
---
Nitpick comments:
In `@src/device/deviceManager.ts`:
- Around line 50-52: Remove the dead commented condition from the early-return
check in the device acquisition path: delete the commented fragment "/* ||
this.connectedDevices.has(deviceInfo.id)*/" inside the if that checks
this.detectedDeviceAcquireQueue.has(deviceInfo.id). The active connectedDevices
check is already implemented later with proper logging (see symbols
detectedDeviceAcquireQueue and connectedDevices), so just remove the commented
code to improve clarity without changing behavior.
- Line 38: The Map detectedDeviceAcquireQueue is typed with string keys but the
rest of the API uses DeviceId, so change its declaration to Map<DeviceId, {
resolve: (value: AcquireResult) => void }[]> and update any usages (look for
accesses to detectedDeviceAcquireQueue.get/set/has/delete) to accept DeviceId
instead of string; import or reference the DeviceId type where declared and
adjust any places that currently pass plain strings (cast or convert to DeviceId
as appropriate) so key types remain consistent and type-safe across the class.
In `@src/device/protocol/virtual/virtualDevice.ts`:
- Around line 66-87: The setAttribute method wraps synchronous logic in a new
Promise; convert it to a plain async function that throws on missing attribute
and returns the value directly: change the signature of setAttribute to be
async, look up the attribute via this.attributes[attributeName], if attribute is
null/undefined throw new Error(`Attribute named "${attributeName}" does not
exist for device with id "${this.deviceId}"`), set this.state = DeviceState.busy
before mutation, assign attribute.value = value, set this.state =
DeviceState.ready, and finally return value (preserving the generic return type
AttributeValueOf<K>); keep references to AttributeKeyOf, AttributeValueOf,
DeviceState, attributes, and deviceId to locate the code.
In `@src/device/provider/genericDeviceProviderFactory.ts`:
- Around line 4-13: The constructor currently uses ConcreteCtor<T> which erases
parameter types to any[], so ConstructorParameters<ConcreteCtor<DP>> becomes
any[] and allows invalid registrations; make the class generic over the concrete
constructor type instead (e.g., introduce a type parameter C extends new
(...args: any[]) => DP) and change the ctor and args fields and constructor
signature to use C and ConstructorParameters<C>, then adjust create() to
instantiate via new this.ctor(...this.args) so parameter types are preserved and
enforced; update references to GenericDeviceProviderFactory, ConcreteCtor, ctor,
args, and the constructor accordingly.
In `@src/device/transport/bleDeviceTransport.ts`:
- Around line 86-88: The stubbed sendAndAwaitReceive method currently throws
synchronously and will crash callers; replace the throw with a proper Promise
rejection that clearly states the method is unsupported on this transport (e.g.
return Promise.reject(new Error('sendAndAwaitReceive not supported by
BleDeviceTransport'))) or, if BLE request/response semantics are required,
implement it inside the BleDeviceTransport class by using the existing send(...)
method and wiring up the BLE notification/response listener (subscribe to the
response characteristic, send(data), await the response with a timeout, then
resolve the Promise) so callers get a proper async rejection or a resolved
Buffer instead of a runtime throw.
🪄 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: 1c3bf691-890c-4713-a483-ef1bbe3a79db
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (80)
eslint.config.tspackage.jsonsrc/automation/scriptRuntime.tssrc/controller/automation/createScriptController.tssrc/controller/automation/deleteScriptController.tssrc/controller/automation/getLogController.tssrc/controller/automation/getScriptController.tssrc/controller/automation/runScriptController.tssrc/controller/getDeviceController.tssrc/controller/patchDeviceController.tssrc/controller/settings/putSettingsController.tssrc/device/attribute/boolDeviceAttribute.tssrc/device/attribute/floatDeviceAttribute.tssrc/device/attribute/intDeviceAttribute.tssrc/device/attribute/intRangeDeviceAttribute.tssrc/device/attribute/listDeviceAttribute.tssrc/device/attribute/strDeviceAttribute.tssrc/device/bleDevice.tssrc/device/device.tssrc/device/deviceId.tssrc/device/deviceManager.tssrc/device/peripheralDevice.tssrc/device/protocol/airotic/airoticDevice.tssrc/device/protocol/airotic/airoticDeviceProvider.tssrc/device/protocol/airotic/airtonicProtocol.tssrc/device/protocol/buttplugIo/buttplugIoDevice.tssrc/device/protocol/buttplugIo/buttplugIoDeviceFactory.tssrc/device/protocol/buttplugIo/buttplugIoWebsocketDeviceProvider.tssrc/device/protocol/deviceProtocol.tssrc/device/protocol/estim2b/estim2bDevice.tssrc/device/protocol/estim2b/estim2bDeviceFactory.tssrc/device/protocol/estim2b/estim2bProtocol.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/slvCtrlPlus/slvCtrlProtocol.tssrc/device/protocol/slvCtrlPlus/slvCtrlProtocolLegacy.tssrc/device/protocol/slvCtrlPlus/slvCtrlProtocolV1.tssrc/device/protocol/virtual/audio/piperVirtualDeviceLogic.tssrc/device/protocol/virtual/genericVirtualDeviceFactory.tssrc/device/protocol/virtual/virtualDevice.tssrc/device/protocol/virtual/virtualDeviceProvider.tssrc/device/protocol/zc95/zc95Device.tssrc/device/protocol/zc95/zc95DeviceFactory.tssrc/device/protocol/zc95/zc95MessageFactory.tssrc/device/protocol/zc95/zc95Protocol.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/bleDeviceTransport.tssrc/device/transport/bleObserver.tssrc/device/transport/serialPortObserver.tssrc/factory/uuidFactory.tssrc/index.tssrc/logging/PinoLogger.tssrc/repository/automationScriptRepository.tssrc/schemaValidation/JsonSchemaValidator.tssrc/schemaValidation/JsonSchemaValidatorFactory.tssrc/serial/synchronousSerialPort.tssrc/serialization/classToPlainSerializer.tssrc/serialization/discriminator/deviceDiscriminator.tssrc/serviceMap.tssrc/serviceProvider/deviceServiceProvider.tssrc/serviceProvider/factoryServiceProvider.tssrc/serviceProvider/settingsServiceProvider.tssrc/settings/knownDevice.tssrc/settings/settings.tssrc/settings/settingsManager.tssrc/socket/deviceUpdateHandler.tssrc/util/async.tssrc/util/createMapTransformFn.tssrc/util/expressUtils.tssrc/util/numbers.tssrc/util/objects.ts
Member
Author
|
@coderabbitai review. I improved the situation a lot, but sometimes I end up with when I want to send over the BleDevcieTransport. Basically isConnected is false, but peripheralState is 'connected'. Can you spot a place where I miss to updated isConnected? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 8801
What happens
There's no retry and the FixIn private async subscribe(): Promise<void> {
if (this.isSubscribing || this.isConnected) {
return;
}
this.isSubscribing = true;
let weSelfConnected = false;
try {
if (this.peripheral.state === 'disconnected') {
weSelfConnected = true;
await this.peripheral.connectAsync();
} else if (this.peripheral.state !== 'connected') {
throw new Error(`Peripheral in unexpected state: ${this.peripheral.state}`);
}
const { characteristics } = await this.peripheral.discoverSomeServicesAndCharacteristicsAsync(
[],
[this.uartRxCharacteristicUuid, this.uartTxCharacteristicUuid],
);
const rx = characteristics.find((c) => c.uuid === this.uartRxCharacteristicUuid);
const tx = characteristics.find((c) => c.uuid === this.uartTxCharacteristicUuid);
if (!rx || !tx) {
throw new Error('Missing UART RX/TX characteristics on device.');
}
this.rx = rx;
for (const subscriber of this.onReceiveSubscribers) {
this.tx?.removeListener('data', subscriber);
}
this.tx = tx;
await this.tx.subscribeAsync();
for (const subscriber of this.onReceiveSubscribers) {
this.tx.on('data', subscriber);
}
this.isConnected = true;
} catch (e) {
// If the peripheral is still physically connected but subscribe() failed
// (e.g. characteristic discovery error), force a disconnect so the
// reconnectHandler in BleDevice can restart the full cycle.
if (!weSelfConnected && this.peripheral.state === 'connected') {
try {
await this.peripheral.disconnectAsync();
} catch {
// ignore disconnect errors — reconnect handler will deal with it
}
}
throw e;
} finally {
this.isSubscribing = false;
}
}The key idea: track whether you called 🧠 Learnings used |
heavyrubberslave
marked this pull request as ready for review
March 31, 2026 05:33
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
src/device/protocol/airotic/airoticDevice.ts (1)
45-77:⚠️ Potential issue | 🟠 MajorAttribute values not persisted after successful write.
The
setAttributemethod returns the value but doesn't update the actual attribute object (e.g.,this.attributes.restColor.value = value). This means subsequent reads will return stale values, and serialization will reflect the old state.🐛 Proposed fix to persist attribute values
public async setAttribute<K extends AttributeKeyOf<AiroticDeviceAttributes>, V extends AttributeValueOf<K>>(attributeName: K, value: V): Promise<V> { if (attributeName === 'restColor' && value !== null && typeof value === 'string') { const { r, g, b } = this.parseColor(value); await this.messageResponseHandler.send(AiroticProtocol.createSelectRestColorMessage()); await sleep(100); await this.messageResponseHandler.send(AiroticProtocol.createSetColorMessage(r, g, b)); + this.attributes.restColor.value = value; return value; } if (attributeName === 'breathInColor' && value !== null && typeof value === 'string') { const { r, g, b } = this.parseColor(value); await this.messageResponseHandler.send(AiroticProtocol.createSelectBreathInColorMessage()); await sleep(100); await this.messageResponseHandler.send(AiroticProtocol.createSetColorMessage(r, g, b)); + this.attributes.breathInColor.value = value; return value; } if (attributeName === 'resetColors' && typeof value === 'boolean') { if (value) { await this.messageResponseHandler.send(AiroticProtocol.createResetColorsMessage()); } + this.attributes.resetColors.value = value; return value; } if (attributeName === 'reboot' && typeof value === 'boolean') { if (value) { await this.messageResponseHandler.send(AiroticProtocol.createRebootMessage()); await sleep(500); await this.close(); } + this.attributes.reboot.value = value; return value; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/device/protocol/airotic/airoticDevice.ts` around lines 45 - 77, The setAttribute method (setAttribute) sends commands but never updates the in-memory attribute store, so writes aren't persisted; update this.attributes for each handled key after successful command sends (e.g., set this.attributes.restColor.value = value for 'restColor', this.attributes.breathInColor.value = value for 'breathInColor', this.attributes.resetColors.value = value for 'resetColors', and this.attributes.reboot.value = value for 'reboot' where appropriate) ensuring you only assign after awaited sends/close complete and keep types consistent with AttributeValueOf; locate these updates in the branches that call parseColor, messageResponseHandler.send, and close to persist the new values.src/device/protocol/airotic/airoticDeviceProvider.ts (1)
98-102:⚠️ Potential issue | 🟠 MajorTransport not closed in catch block when error occurs after transport creation.
If an exception is thrown after
BleUartDeviceTransport.create()succeeds (e.g., during handshake), the catch block disconnects the peripheral but doesn't close the transport. This leaves the transport's event handlers attached and may cause resource leaks.🐛 Proposed fix to close transport in catch block
+ let transport: BleUartDeviceTransport | undefined; + try { this.logger.debug(`Requesting to acquire device: ${deviceInfo.id}`); const acquireResult = await this.deviceManager.acquireDetectedDevice(deviceInfo.id); if (!acquireResult.successful) { this.logger.debug(`Could not acquire device: ${acquireResult.reason}`); return; } - const transport = await promiseWithTimeout(BleUartDeviceTransport.create( + transport = await promiseWithTimeout(BleUartDeviceTransport.create( deviceInfo.peripheral, AiroticDeviceProvider.UART_RX_CHAR_UUID, AiroticDeviceProvider.UART_TX_CHAR_UUID ), 5000, `Timed out while creating BLE transport for device ${deviceInfo.id}`); // ... rest of try block ... } catch (e: unknown) { logError(this.logger, `Error while connecting to device`, e); this.deviceManager.releaseDetectedDevice(deviceInfo.id); + if (transport) { + await transport.close(); + } await this.disconnectDevice(deviceInfo.peripheral); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/device/protocol/airotic/airoticDeviceProvider.ts` around lines 98 - 102, The catch block currently releases the device and disconnects the peripheral but doesn't close the transport if BleUartDeviceTransport.create() succeeded, leaving event handlers attached; update the function to declare a transport variable outside the try, assign it from BleUartDeviceTransport.create(), and in the catch block check for the transport and await closing it (e.g., await transport.close() or transport.dispose() depending on the transport API) before calling this.deviceManager.releaseDetectedDevice(deviceInfo.id) and await this.disconnectDevice(deviceInfo.peripheral); ensure this change references BleUartDeviceTransport.create, the transport variable, this.disconnectDevice, and this.deviceManager.releaseDetectedDevice so the transport is always cleaned up on error.
🧹 Nitpick comments (4)
tests/unit/device/deviceManager.spec.ts (1)
13-18: Consider extracting common test setup into a helper.This repeated arrangement is a good candidate for a tiny factory to keep tests lean as coverage expands.
♻️ Optional cleanup
+function createDeviceManagerTestContext() { + const mockedDeviceManagerEventEmitter = mock<EventEmitter>(); + const mockedLogger = mock<Logger>(); + mockedLogger.child.mockReturnValue(mockedLogger); + const connectedDevices = new Map<string, Device>(); + const deviceManager = new DeviceManager(mockedDeviceManagerEventEmitter, connectedDevices, mockedLogger); + return { mockedDeviceManagerEventEmitter, mockedLogger, connectedDevices, deviceManager }; +}Also applies to: 43-48, 68-73
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/device/deviceManager.spec.ts` around lines 13 - 18, Extract the repeated test setup that constructs mockedDeviceManagerEventEmitter, mockedLogger (with child mockReturnValue), and the DeviceManager instance into a small factory/helper function (e.g., createDeviceManagerTestFixture) and replace duplicated blocks in this spec (and the other occurrences) with calls to that helper; the helper should return the mocks and the DeviceManager (or a tuple/object containing {deviceManager, mockedDeviceManagerEventEmitter, mockedLogger}) so tests can access and reuse the same setup across lines 13-18, 43-48, and 68-73.src/device/bleDevice.ts (1)
51-68: Reconnect handler may attempt connection on a peripheral already connecting.When
peripheral.state === 'connecting', the conditionperipheral.state !== 'connected'is true, soconnectAsync()is called on a peripheral that's already mid-connection. This could cause unexpected behavior or duplicate connection attempts.🛠️ Proposed fix to handle 'connecting' state explicitly
this.reconnectHandler = asyncHandler( async () => { this.logger.info(`BLE Device ${this.deviceId} disconnected, trying to reconnect`); try { - if (peripheral.state !== 'connected') { + if (peripheral.state === 'disconnected') { await promiseWithTimeout(peripheral.connectAsync(), 3000, `Timed out (>3s) while reconnecting to device ${this.deviceId}`); this.logger.info(`BLE Device ${this.deviceId} reconnected successfully`); + } else if (peripheral.state === 'connected') { + this.logger.warn(`BLE Device ${this.deviceId} disconnect event fired but peripheral is already connected`); } else { - this.logger.warn(`BLE Device ${this.deviceId} is not in disconnected state, current state: ${peripheral.state}`); + this.logger.warn(`BLE Device ${this.deviceId} is in '${peripheral.state}' state, skipping reconnect`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/device/bleDevice.ts` around lines 51 - 68, The reconnectHandler currently calls peripheral.connectAsync whenever peripheral.state !== 'connected', which can trigger a duplicate connect when state === 'connecting'; update the logic in reconnectHandler to explicitly handle the 'connecting' state (e.g., if peripheral.state === 'connecting' then skip calling peripheral.connectAsync — log and optionally await a short wait/timeout or return early) and only call promiseWithTimeout(peripheral.connectAsync(), ...) when state is neither 'connected' nor 'connecting'; keep existing error handling (BaseError.normalize, this.close) and the outer asyncHandler/logError wrapper intact.src/device/protocol/airotic/airoticDeviceProvider.ts (1)
170-176: Inconsistent provider name: hardcoded'airotic'vsAiroticDeviceProvider.providerName.Line 174 uses a hardcoded string while the class defines a constant at line 24. This could lead to inconsistencies if the provider name changes.
♻️ Proposed fix
// Create a new device and return if not yet known (new serial number) return new KnownDevice( deviceId, deviceName, - 'airotic', + AiroticDeviceProvider.providerName, provider );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/device/protocol/airotic/airoticDeviceProvider.ts` around lines 170 - 176, Replace the hardcoded provider string 'airotic' passed into the KnownDevice constructor with the canonical provider name constant defined on the provider class (AiroticDeviceProvider.providerName) to avoid mismatches; locate the return new KnownDevice(...) call and swap the third argument to use AiroticDeviceProvider.providerName (or the provider instance's providerName static/instance property) instead of the literal.src/device/transport/bleDeviceTransport.ts (1)
38-38: Replaceconsole.errorwith injected logger.Static analysis flagged this
console.error. The transport doesn't have a logger instance, but error handling in the connect handler should use proper logging for consistency with the rest of the codebase.♻️ Proposed fix to inject logger
+import Logger from '../../logging/Logger.js'; + export default class BleUartDeviceTransport implements DeviceTransport { private readonly peripheral: Peripheral; private rx?: Characteristic; private tx?: Characteristic; private readonly uartRxCharacteristicUuid: string; private readonly uartTxCharacteristicUuid: string; private isConnected: boolean = false; private isSubscribing: boolean = false; private onCloseSubscribers: (() => Promise<void>)[] = []; private onReceiveSubscribers: ((data: Buffer) => void)[] = []; private readonly connectHandler: (err: Error) => void; private readonly disconnectHandler: (err: Error) => void; + private readonly logger: Logger; public static async create( peripheral: Peripheral, uartRxCharacteristicUuid: string, - uartTxCharacteristicUuid: string + uartTxCharacteristicUuid: string, + logger: Logger ): Promise<BleUartDeviceTransport> { - const transport = new this(peripheral, uartRxCharacteristicUuid, uartTxCharacteristicUuid); + const transport = new this(peripheral, uartRxCharacteristicUuid, uartTxCharacteristicUuid, logger); await transport.subscribe(); return transport; } - private constructor(peripheral: Peripheral, uartRxCharacteristicUuid: string, uartTxCharacteristicUuid: string) { + private constructor(peripheral: Peripheral, uartRxCharacteristicUuid: string, uartTxCharacteristicUuid: string, logger: Logger) { this.peripheral = peripheral; this.uartRxCharacteristicUuid = uartRxCharacteristicUuid; this.uartTxCharacteristicUuid = uartTxCharacteristicUuid; + this.logger = logger.child({ name: BleUartDeviceTransport.name }); - this.connectHandler = asyncHandler(async (err: Error) => { if (null !== err) { return; } await this.subscribe() }, console.error); + this.connectHandler = asyncHandler( + async (err: Error) => { if (null !== err) { return; } await this.subscribe() }, + (e: unknown) => this.logger.error('Error in connect handler', e) + ); this.disconnectHandler = (): void => { this.isConnected = false; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/device/transport/bleDeviceTransport.ts` at line 38, The connectHandler currently passes console.error to asyncHandler; replace that with the transport's injected logger: add a Logger (or reuse existing this.logger) to the BleDeviceTransport class (constructor parameter and a private field), ensure the instance is assigned, and then pass a lambda that calls the logger's error method (e.g., err => this.logger.error(err)) to asyncHandler in the connectHandler assignment; keep the existing asyncHandler signature and retain the current err/null check and subscribe invocation in the connectHandler body.
🤖 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/genericDeviceUpdater.ts`:
- Around line 39-41: The warn log in genericDeviceUpdater.ts claims
"disconnecting" but no disconnect is performed; update the behavior so the log
accurately reflects actions: either remove the "disconnecting" phrase from
this.logger.warn(`Device ${device.getDeviceId} has more than 10 failed update
attempts, disconnecting it...`) or add an explicit call to the appropriate
disconnect/revoke routine (e.g., device.disconnect(), device.revokeConnection(),
or invoking the module-level revokeDevice/removeDevice function used elsewhere)
immediately after the failed attempt check; ensure you reference the same device
identity via device.getDeviceId and keep the log message consistent with the
actual action taken.
- Around line 12-13: The Map failedMessageCountPerDevice is never pruned and
setting counts to 0 leaves stale device IDs in the singleton; change the reset
logic to remove entries with this.failedMessageCountPerDevice.delete(deviceId)
instead of setting to 0, and add a small eviction strategy (e.g., cap the Map
size and drop oldest entries or run a periodic cleanup/TTL) to prevent unbounded
growth; update all places that currently set counts to 0 (the reset branch
around the current line 33) and ensure any new eviction maintains the same
semantics for get/increment operations.
- Around line 32-37: The failure counter is being updated per-attribute which
resets on any succeeding attribute (device.setAttribute) and increments on any
single attribute failure, breaking per-update semantics; change the logic in the
update routine that calls device.setAttribute and uses
this.failedMessageCountPerDevice and device.getDeviceId so that you compute
success/failure for the whole update cycle (e.g., track a boolean or failure
count while iterating attributes), then after processing all attributes: if all
succeeded reset this.failedMessageCountPerDevice.set(device.getDeviceId, 0),
otherwise increment it once
(this.failedMessageCountPerDevice.set(device.getDeviceId, (…)+1)) and call
logError(this.logger, `${deviceLogMsg} -> failed`, e) with the aggregated
error/context; ensure logger.info(`${deviceLogMsg} -> done`) is only emitted
when the whole update succeeded.
In `@src/device/protocol/airotic/airoticDevice.ts`:
- Around line 82-93: parseColor currently only validates each parsed channel but
not the number of components, so add an explicit check after splitting the input
string to ensure exactly 3 parts and throw a clear Error if not; keep the
existing per-channel parsing/validation logic (parseInt(c, 10) and range checks)
and then destructure the resulting numeric array into r, g, b and return { r, g,
b } from the parseColor method to prevent undefined values or silently ignored
extra components.
In `@src/device/protocol/zc95/zc95Device.ts`:
- Around line 115-118: The code is mistakenly treating setAttributePowerChannel
as always successful; change setAttribute to check the result of
setAttributePowerChannel (call it, await its return) and if it indicates failure
or undefined (meaning powerChannel* values are missing) do not call
updateLastRefresh or return success; instead either throw an explicit error from
setAttribute (e.g., new Error('power channel snapshot missing')) or attempt to
hydrate the missing snapshot (invoke the existing snapshot/hydration routine)
and retry the set once before throwing; ensure this logic is applied where
isPowerChannelAttribute(...) triggers setAttributePowerChannel (and likewise in
the other occurrence around lines 155-160) so callers get a failure and can
retry.
In `@tests/unit/device/protocol/buttplugIo/buttplugIoDevice.spec.ts`:
- Line 12: The test imports EventEmitter incorrectly from 'stream'; update the
import statement that references EventEmitter (used in buttplugIoDevice.spec.ts)
to import EventEmitter from 'node:events' instead of 'stream' so it uses the
Node.js EventEmitter API and avoids type/import errors.
---
Duplicate comments:
In `@src/device/protocol/airotic/airoticDevice.ts`:
- Around line 45-77: The setAttribute method (setAttribute) sends commands but
never updates the in-memory attribute store, so writes aren't persisted; update
this.attributes for each handled key after successful command sends (e.g., set
this.attributes.restColor.value = value for 'restColor',
this.attributes.breathInColor.value = value for 'breathInColor',
this.attributes.resetColors.value = value for 'resetColors', and
this.attributes.reboot.value = value for 'reboot' where appropriate) ensuring
you only assign after awaited sends/close complete and keep types consistent
with AttributeValueOf; locate these updates in the branches that call
parseColor, messageResponseHandler.send, and close to persist the new values.
In `@src/device/protocol/airotic/airoticDeviceProvider.ts`:
- Around line 98-102: The catch block currently releases the device and
disconnects the peripheral but doesn't close the transport if
BleUartDeviceTransport.create() succeeded, leaving event handlers attached;
update the function to declare a transport variable outside the try, assign it
from BleUartDeviceTransport.create(), and in the catch block check for the
transport and await closing it (e.g., await transport.close() or
transport.dispose() depending on the transport API) before calling
this.deviceManager.releaseDetectedDevice(deviceInfo.id) and await
this.disconnectDevice(deviceInfo.peripheral); ensure this change references
BleUartDeviceTransport.create, the transport variable, this.disconnectDevice,
and this.deviceManager.releaseDetectedDevice so the transport is always cleaned
up on error.
---
Nitpick comments:
In `@src/device/bleDevice.ts`:
- Around line 51-68: The reconnectHandler currently calls
peripheral.connectAsync whenever peripheral.state !== 'connected', which can
trigger a duplicate connect when state === 'connecting'; update the logic in
reconnectHandler to explicitly handle the 'connecting' state (e.g., if
peripheral.state === 'connecting' then skip calling peripheral.connectAsync —
log and optionally await a short wait/timeout or return early) and only call
promiseWithTimeout(peripheral.connectAsync(), ...) when state is neither
'connected' nor 'connecting'; keep existing error handling (BaseError.normalize,
this.close) and the outer asyncHandler/logError wrapper intact.
In `@src/device/protocol/airotic/airoticDeviceProvider.ts`:
- Around line 170-176: Replace the hardcoded provider string 'airotic' passed
into the KnownDevice constructor with the canonical provider name constant
defined on the provider class (AiroticDeviceProvider.providerName) to avoid
mismatches; locate the return new KnownDevice(...) call and swap the third
argument to use AiroticDeviceProvider.providerName (or the provider instance's
providerName static/instance property) instead of the literal.
In `@src/device/transport/bleDeviceTransport.ts`:
- Line 38: The connectHandler currently passes console.error to asyncHandler;
replace that with the transport's injected logger: add a Logger (or reuse
existing this.logger) to the BleDeviceTransport class (constructor parameter and
a private field), ensure the instance is assigned, and then pass a lambda that
calls the logger's error method (e.g., err => this.logger.error(err)) to
asyncHandler in the connectHandler assignment; keep the existing asyncHandler
signature and retain the current err/null check and subscribe invocation in the
connectHandler body.
In `@tests/unit/device/deviceManager.spec.ts`:
- Around line 13-18: Extract the repeated test setup that constructs
mockedDeviceManagerEventEmitter, mockedLogger (with child mockReturnValue), and
the DeviceManager instance into a small factory/helper function (e.g.,
createDeviceManagerTestFixture) and replace duplicated blocks in this spec (and
the other occurrences) with calls to that helper; the helper should return the
mocks and the DeviceManager (or a tuple/object containing {deviceManager,
mockedDeviceManagerEventEmitter, mockedLogger}) so tests can access and reuse
the same setup across lines 13-18, 43-48, and 68-73.
🪄 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: 5ab99c23-fd95-41e9-99a7-d913a8d09160
📒 Files selected for processing (13)
src/controller/patchDeviceController.tssrc/device/bleDevice.tssrc/device/genericDeviceUpdater.tssrc/device/protocol/airotic/airoticDevice.tssrc/device/protocol/airotic/airoticDeviceProvider.tssrc/device/protocol/messageResponseHandler.tssrc/device/protocol/zc95/zc95Device.tssrc/device/transport/bleDeviceTransport.tssrc/device/transport/bleObserver.tssrc/index.tssrc/util/async.tstests/unit/device/deviceManager.spec.tstests/unit/device/protocol/buttplugIo/buttplugIoDevice.spec.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/controller/patchDeviceController.ts
- src/device/transport/bleObserver.ts
- src/util/async.ts
- src/device/protocol/messageResponseHandler.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/device/protocol/zc95/zc95Device.ts (2)
267-275: Type assertion onchannel.Channelrelies on runtime guard for safety.Line 269 asserts
channelAttrNameasZc95DevicePowerChannelAttributesKey, butchannel.Channelcomes from external device data. If the device reports a channel outside1 | 2 | 3 | 4, the type assertion would be incorrect, though the runtime guard at line 273-275 (if (!channelAttr) { continue; }) handles this gracefully.If the ZC95 protocol guarantees channels 1-4 only, consider documenting this. Otherwise, a type-safe alternative would be to validate before asserting:
♻️ Optional: Validate channel before type assertion
private processPowerStatusMessage(msg: PowerStatusMsgResponse): void { for (const channel of msg.Channels) { + if (channel.Channel < 1 || channel.Channel > 4) { + continue; + } const channelAttrName: Zc95DevicePowerChannelAttributesKey = `${Zc95Device.powerChannelAttributePrefix}${channel.Channel}`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/device/protocol/zc95/zc95Device.ts` around lines 267 - 275, processPowerStatusMessage uses a type assertion on channel.Channel when building channelAttrName (Zc95Device.powerChannelAttributePrefix -> Zc95DevicePowerChannelAttributesKey) but channel.Channel comes from external device data; validate that channel.Channel is one of the allowed values (1|2|3|4) before forming the attribute key or asserting the type, e.g., check msg.Channels / channel.Channel against the permitted set and bail/continue on invalid values so the assertion is safe; update the code in processPowerStatusMessage to perform that guard and then access this.attributes[channelAttrName].
244-254: Consider narrowingchannelIndextype for stronger guarantees.The parameter
channelIndex: numbercould be typed as1 | 2 | 3 | 4to match theZc95DevicePowerChannelAttributesKeypattern. Since this method is private and only called with literal values, the current typing is safe in practice.♻️ Optional: Narrow channel index type
- private getChannelPowerAttribute(channelIndex: number): IntRangeDeviceAttribute { + private getChannelPowerAttribute(channelIndex: 1 | 2 | 3 | 4): IntRangeDeviceAttribute {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/device/protocol/zc95/zc95Device.ts` around lines 244 - 254, Change the parameter type of getChannelPowerAttribute from a broad number to the narrowed union 1 | 2 | 3 | 4 to match Zc95DevicePowerChannelAttributesKey and provide stronger type safety; update the method signature for getChannelPowerAttribute(channelIndex: 1 | 2 | 3 | 4) and ensure any internal uses (calls to getChannelPowerAttribute and references to powerChannelAttributePrefix) pass one of those literal values or are narrowed/cast accordingly so the compiler can validate correct channel indices.
🤖 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/transport/bleObserver.ts`:
- Around line 31-47: The init() method can race with the stateChange handler
causing concurrent observe() calls; add a scanning guard (e.g., a private
boolean like isObserving or scanningInProgress) on the class and use it in
init(), in the stateChange async handler, and at the start of observe() to
skip/return immediately if a scan is already starting; set the flag before
invoking startScanningAsync(), clear it in a finally block inside observe()
(also when catching errors that call stopScanningAsync()) so stopScanningAsync()
won't cancel a legitimate scan started by a concurrent caller.
---
Nitpick comments:
In `@src/device/protocol/zc95/zc95Device.ts`:
- Around line 267-275: processPowerStatusMessage uses a type assertion on
channel.Channel when building channelAttrName
(Zc95Device.powerChannelAttributePrefix -> Zc95DevicePowerChannelAttributesKey)
but channel.Channel comes from external device data; validate that
channel.Channel is one of the allowed values (1|2|3|4) before forming the
attribute key or asserting the type, e.g., check msg.Channels / channel.Channel
against the permitted set and bail/continue on invalid values so the assertion
is safe; update the code in processPowerStatusMessage to perform that guard and
then access this.attributes[channelAttrName].
- Around line 244-254: Change the parameter type of getChannelPowerAttribute
from a broad number to the narrowed union 1 | 2 | 3 | 4 to match
Zc95DevicePowerChannelAttributesKey and provide stronger type safety; update the
method signature for getChannelPowerAttribute(channelIndex: 1 | 2 | 3 | 4) and
ensure any internal uses (calls to getChannelPowerAttribute and references to
powerChannelAttributePrefix) pass one of those literal values or are
narrowed/cast accordingly so the compiler can validate correct channel indices.
🪄 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: f9402fbe-0c99-417f-8338-256dc6a2bd05
📒 Files selected for processing (5)
src/device/genericDeviceUpdater.tssrc/device/protocol/airotic/airoticDevice.tssrc/device/protocol/zc95/zc95Device.tssrc/device/transport/bleObserver.tstests/unit/device/protocol/buttplugIo/buttplugIoDevice.spec.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/unit/device/protocol/buttplugIo/buttplugIoDevice.spec.ts
- src/device/protocol/airotic/airoticDevice.ts
- src/device/genericDeviceUpdater.ts
Member
Author
|
@coderabbitai please review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== bleDeviceTransport.ts ==="
cat src/device/transport/bleDeviceTransport.ts
echo "=== bleDevice.ts ==="
cat src/device/bleDevice.ts
echo "=== airoticDeviceProvider.ts ==="
cat src/device/protocol/airotic/airoticDeviceProvider.ts
echo "=== bleObserver.ts ==="
cat src/device/transport/bleObserver.ts
echo "=== deviceManager.ts ==="
sed -n '1,250p' src/device/deviceManager.tsLength of output: 27769 🏁 Script executed: #!/bin/bash
fd -e ts "bleDeviceProvider" --exec cat {}
echo "=== device.ts (relevant parts) ==="
sed -n '1,50p' src/device/device.tsLength of output: 6131 🏁 Script executed: #!/bin/bash
rg -n "revokeDetectedDevice" --type=tsLength of output: 358 🏁 Script executed: #!/bin/bash
rg -n "revokeDetectedDevice"Length of output: 348 🏁 Script executed: #!/bin/bash
grep -n "revokeDetectedDevice" src/device/deviceManager.ts src/device/transport/bleObserver.ts
echo "---"
grep -rn "revokeDetectedDevice" src/ || echo "no matches"Length of output: 624
✅ Fixed since last review
One small leftover: if (this.detectedDeviceAcquireQueue.has(deviceInfo.id)/* || this.connectedDevices.has(deviceInfo.id)*/) {immediately followed by the real 🔴 Still open — the "stuck
|
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/device/bleDevice.ts (2)
35-79: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReconnect-handler log message contradicts its own condition.
At Line 66-68, the
elsebranch (only reached whenperipheral.state === 'connected') logs"is not in disconnected state"— the message is worded backwards and will mislead whoever debugs a future reconnect issue (this exact class of confusion is what caused the original hang report).🐛 Proposed fix
} else { - this.logger.warn(`BLE Device ${this.deviceId} is not in disconnected state, current state: ${peripheral.state}`); + this.logger.warn(`BLE Device ${this.deviceId} is already connected, skipping reconnect attempt`); }🤖 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/bleDevice.ts` around lines 35 - 79, The reconnect handler in BLEDevice has a misleading log message in the `else` branch of the `peripheral.state` check: it says the device “is not in disconnected state” even though that branch runs when the peripheral is already connected. Update the message in `reconnectHandler` to accurately describe the current state and action taken, keeping the condition and the logged state aligned so debugging `BLEDevice` reconnect behavior is not confusing.
98-117: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winKeep cleanup on the cleaned-up disconnect path.
returnhere skipsclearInterval(this.rssiInterval)andthis.peripheral.off('disconnect', this.reconnectHandler), so closing a device after the BLE manager is gone can leave the RSSI timer running and the reconnect listener attached. The special case should log/ignore the disconnect error, then continue to cleanup;tests/unit/device/bleDevice.spec.tsdoes not cover this branch.🤖 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/bleDevice.ts` around lines 98 - 117, The special-case branch in doClose for the “BLEManager has already been cleaned up” disconnect error is returning too early and skipping cleanup. Update doClose in BLEDevice so that this error is logged/ignored but execution continues to clearInterval(this.rssiInterval) and remove the disconnect listener with this.peripheral.off('disconnect', this.reconnectHandler), matching the normal cleanup path; make sure the disconnect error handling still stays inside the connected-state branch.src/device/transport/bleDeviceTransport.ts (1)
46-108: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAbort in-flight connects with
cancelConnect()
subscribe()can throw whileperipheral.state === 'connecting';disconnectAsync()won’t stop that attempt. Cancel the pending connect there, and keepdisconnectAsync()for the already-connected case so the reconnect handler can restart the cycle.🤖 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/bleDeviceTransport.ts` around lines 46 - 108, The BLE reconnect path in subscribe() only forces disconnect when the peripheral is already connected, so a failure during an in-flight connect can leave the connect attempt running. Update the catch block in BLEDeviceTransport.subscribe() to call cancelConnect() when the peripheral is in the connecting state, and keep disconnectAsync() only for the connected case so BleDevice.reconnectHandler can recover cleanly.
🧹 Nitpick comments (13)
src/serialization/classToPlainSerializer.ts (1)
12-13: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOverload still permits caller-asserted, unverified return type.
Replacing
result as Vwith an overload that returnsTOut extends Record<string, unknown>removes the explicit cast, but the implementation still always returns a bareRecord<string, unknown>; callers specifyingtransform<SerializedDevice>(...)get no runtime guarantee the shape matches. This is functionally equivalent to the prior unsafe cast, just relocated to the call site's generic argument — acceptable if intentional (similar toJSON.parse<T>()patterns), but worth being aware it's not a genuine type-safety improvement.🤖 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/serialization/classToPlainSerializer.ts` around lines 12 - 13, The new transform overload in classToPlainSerializer still lets callers assert an unchecked generic return type, so it does not provide real runtime type safety. Either keep the API explicitly as a typed assertion-style helper or narrow the signature in transform so it only returns Record<string, unknown> unless the shape is actually validated, and ensure the implementation matches the intended contract for TOut and TypeOptions.src/device/provider/deviceProviderManager.ts (1)
42-46: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
startProviders()isn't resilient to a single provider failing.Unlike
stopProviders(), which catches per-provider errors and continues,startProviders()lets anyprovider.init()rejection abort the loop, so providers ordered after a failing one never start. A single flaky provider (e.g., BLE) could prevent unrelated providers (serial, virtual, etc.) from initializing.♻️ Suggested fix mirroring stopProviders()' error handling
public async startProviders(): Promise<void> { - for (const provider of this.providers) { - await provider.init(); - } + const errors: unknown[] = []; + + for (const provider of this.providers) { + try { + await provider.init(); + } catch (error: unknown) { + errors.push(error); + this.logger.error('Failed to start device provider', error); + } + } + + if (errors.length > 0) { + throw new Error(`Failed to start ${errors.length} device provider(s)`); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/device/provider/deviceProviderManager.ts` around lines 42 - 46, startProviders() in DeviceProviderManager is not resilient to a single provider.init() failure, so one rejected init aborts later providers from starting. Update DeviceProviderManager.startProviders() to mirror stopProviders() by wrapping each provider.init() call in per-provider error handling, logging the failure with the provider identity, and continuing the loop so unrelated providers still initialize.src/device/deviceManager.ts (1)
144-146: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the
DeviceIdbrand on manager lookups.Line 144 still accepts any
string, while callers likeConnectedDeviceRepository.getById()now passDeviceId. Tightening this preserves the new identity contract at the manager boundary.- public getConnectedDevice(deviceId: string): Device|null + public getConnectedDevice(deviceId: DeviceId): Device|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/deviceManager.ts` around lines 144 - 146, The DeviceManager lookup API is still too loose because getConnectedDevice currently accepts a plain string instead of the branded DeviceId. Update the getConnectedDevice method signature to use DeviceId so the manager boundary preserves the identity contract, and make sure any internal uses of connectedDevices.get continue to work with that branded type.tests/unit/automation/scriptRuntime.spec.ts (1)
1-472: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding coverage for the
deviceNotificationevent variant.All tests dispatch
deviceConnected/deviceDisconnected(emptyargs: []).SupportedDeviceEvent'sdeviceNotificationbranch is the only one with a non-emptyargstuple exercising thehandler(device, ...args)spread path in__dispatchEvent. A test dispatching{ type: DeviceManagerEvent.deviceNotification, device, args: [notification] }and asserting the handler receives the notification payload would close a coverage gap on the new dispatch mechanism.🤖 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 `@tests/unit/automation/scriptRuntime.spec.ts` around lines 1 - 472, Add a test that covers the `SupportedDeviceEvent` `deviceNotification` path in `ScriptRuntime`. The current spec only exercises `deviceConnected` and `deviceDisconnected` with empty `args`, so it misses the `handler(device, ...args)` spread behavior in `__dispatchEvent`. In `tests/unit/automation/scriptRuntime.spec.ts`, dispatch a `DeviceManagerEvent.deviceNotification` event with a notification payload in `args` and assert the `onEvent('deviceNotification', ...)` handler receives that payload. Use the existing `dispatchAndCollect`, `runtime.runForEvent`, and `deviceA`/`StubDevice` setup to keep the test consistent.src/util/async.ts (1)
92-106: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument that the wrapped operation keeps running after timeout.
promiseWithTimeoutrejects the wrapper only; it does not cancelpromise. Since call sites use it around side-effectful BLE setup, make the cleanup/cancellation responsibility explicit.📝 Proposed API documentation
+/** + * Rejects if `promise` does not settle within `timeoutMs`. + * + * Note: this does not cancel the underlying promise. Callers wrapping + * side-effectful operations must cancel or clean up that work themselves. + */ export const promiseWithTimeout = <T>(Based on learnings,
Promise.racetimeouts do not cancel the underlying callback and this limitation should be recorded in code comments/API docs.🤖 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/util/async.ts` around lines 92 - 106, Document in promiseWithTimeout that it only rejects the wrapper on timeout and does not cancel the underlying promise or side effects. Update the API docs/comments near promiseWithTimeout to make cleanup/cancellation responsibility explicit for callers, especially where it is used for BLE setup. Refer to promiseWithTimeout and the Promise.race timeout behavior so future users understand the wrapped operation continues running after timeout.Source: Learnings
src/device/transport/deviceBidirectionalTransport.ts (1)
4-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename the exported interface to
DeviceBidirectionalTransport.The default export still uses the legacy
DeviceTransportname, which leaks into diagnostics and declaration output even though this module now models the bidirectional transport contract.♻️ Proposed rename
-export default interface DeviceTransport extends DeviceReadableTransport, DeviceWritableTransport +export default interface DeviceBidirectionalTransport extends DeviceReadableTransport, DeviceWritableTransport🤖 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/deviceBidirectionalTransport.ts` around lines 4 - 11, Rename the default exported interface from DeviceTransport to DeviceBidirectionalTransport in the deviceBidirectionalTransport module, and update the interface declaration so the exported contract name matches the bidirectional transport role. Make sure any references within this module that rely on the interface symbol are updated consistently so diagnostics and generated declarations no longer expose the legacy DeviceTransport name.tests/unit/device/protocol/slvCtrlPlus/genericSlvCtrlPlusDevice.spec.ts (1)
188-339: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider reusing the
createDevicehelper instead of duplicating the constructor call.All three new tests repeat the same 12-argument
GenericSlvCtrlPlusDeviceconstruction verbatim (differing only inattrs). ExtendingcreateDeviceto optionally accept/return the logger mock would remove this duplication.♻️ Example refactor sketch
- function createDevice( - attrs: SlvCtrlPlusDeviceAttributes, - protocol: SlvCtrlProtocol, - transport: DeviceBidirectionalTransport, - ): GenericSlvCtrlPlusDevice { + function createDevice( + attrs: SlvCtrlPlusDeviceAttributes, + protocol: SlvCtrlProtocol, + transport: DeviceBidirectionalTransport, + logger: Logger = mock<Logger>(), + ): GenericSlvCtrlPlusDevice { const fwVersion = 10000; const deviceUuid = DeviceId.create('foo-bar-baz'); const deviceName = 'Aston Martin'; const model = 'et312'; const protocolVersion = 10000; const provider = 'dummy'; return new GenericSlvCtrlPlusDevice( fwVersion, deviceUuid, deviceName, model, provider, new Date(), protocol, transport, protocolVersion, attrs, - mock<EventEmitter>(), mock<Logger>(), + mock<EventEmitter>(), logger, ); }🤖 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 `@tests/unit/device/protocol/slvCtrlPlus/genericSlvCtrlPlusDevice.spec.ts` around lines 188 - 339, The three new refresh tests duplicate the same GenericSlvCtrlPlusDevice setup, so refactor them to reuse the existing createDevice helper instead of repeating the 12-argument constructor. Update createDevice to accept the attrs override and optionally return or accept the Logger mock if needed, then use it in the bool/str, empty-string, and unknown-attribute tests to keep only the per-test protocol/transport stubbing inline.tests/unit/device/protocol/zc95/zc95Device.spec.ts (1)
685-714: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExercise the clamp path or simplify this case.
processPowerStatusMessageoverwrites the clamped value withMaxOutputPower, so this over-limit setup still ends at50and doesn’t distinguish clamp vs. no-clamp behavior.🤖 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 `@tests/unit/device/protocol/zc95/zc95Device.spec.ts` around lines 685 - 714, The test case in processPowerStatusMessage is not actually exercising the clamp behavior because the final assignment to MaxOutputPower overwrites the earlier clamped value, so the over-limit setup does not prove a distinct code path. Update the spec around getOnReceiveCallback and device.getAttribute('powerChannel1') to either use a scenario that reaches the clamp logic without being overwritten, or simplify the assertion to match the current behavior and avoid implying clamp-specific coverage.tests/unit/device/bleDevice.spec.ts (1)
80-86: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for
reconnectHandlerbehavior.Only the
disconnectlistener registration is tested. Consider capturing the handler passed tomockPeripheral.on('disconnect', ...)and exercising: successfulconnectAsync(), the already-'connected'branch, and the failure path that callsthis.close(). This is the exact logic implicated in the PR's reported reconnect hang.🤖 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 `@tests/unit/device/bleDevice.spec.ts` around lines 80 - 86, The current constructor spec only verifies that `BleDevice` registers the disconnect listener, but it does not cover the `reconnectHandler` logic passed to `mockPeripheral.on('disconnect', ...)`. Update the `bleDevice.spec.ts` constructor test to capture that handler and exercise all branches in `reconnectHandler`: a successful `connectAsync()` reconnect, the already `'connected'` early-return path, and the error path that triggers `this.close()`. Use the existing `createDevice`, `mockPeripheral`, and `connectAsync`/`close` mocks to assert the expected behavior.tests/unit/device/transport/serialPortObserver.spec.ts (1)
118-127: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest doesn't exercise a second discovery run.
SerialPortObserveronly rescans in response tousbconnect/disconnectevents (debounced viasetTimeout), per the observer source in the relevant snippets. This test advances fake timers by 3000ms but never dispatches ausbevent, sodiscoverSerialDevices()is never invoked a second time — the assertion (toHaveBeenCalledOnce) is trivially true even without any dedup logic working. Other tests in this file (e.g. "revokes a device that disappears...") correctly callobserver.discoverSerialDevices()directly to exercise a second run; consider doing the same here to actually validate the "already managed" dedup behavior.🤖 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 `@tests/unit/device/transport/serialPortObserver.spec.ts` around lines 118 - 127, The test for SerialPortObserver deduplication is only checking the initial scan and never triggers a second discovery pass, so it does not verify the “already managed” behavior. Update the spec to invoke SerialPortObserver.discoverSerialDevices() again after the first start/scan, using the existing createObserver and mockDeviceManager.announceDetectedDevice setup, so the assertion truly validates that a previously managed device is not re-announced on a subsequent discovery run.src/app.ts (1)
184-204: 🔒 Security & Privacy | 🔵 TrivialConsider adding Helmet for baseline security headers.
Static analysis flags the Express app as lacking Helmet-provided headers (X-Content-Type-Options, X-Frame-Options, etc.). Not blocking for this PR's BLE scope, but worth tracking.
🤖 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/app.ts` around lines 184 - 204, The Express app setup in createApp is missing Helmet-based baseline security headers, which static analysis flagged. Update the middleware chain in createApp to include Helmet alongside the existing cors, contentTypeMiddleware, express.json, and express.text setup, keeping the current PNA preflight handling intact. Use the createApp symbol as the place to add the security middleware so the app gets standard headers like X-Content-Type-Options and X-Frame-Options by default.Source: Linters/SAST tools
tests/unit/device/transport/bleObserver.spec.ts (1)
130-193: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing test coverage for
BleObserver.stop().
stop()is invoked on app shutdown (src/app.tscallsdevice.observer.ble.stop()) but has no dedicated test here (listener removal,stopScanningAsynccall when scanning,noble.stop()call, and the no-op path when not scanning).🤖 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 `@tests/unit/device/transport/bleObserver.spec.ts` around lines 130 - 193, Add dedicated tests for BleObserver.stop() in bleObserver.spec.ts using the existing createObserver and noble mocks. Cover the shutdown path where stop() is called while scanning: verify it removes the noble listeners, awaits stopScanningAsync, and calls noble.stop(). Also add a no-op case when scanning is already inactive so stop() does not try to stop scanning again. Keep the assertions centered on the BleObserver.stop method and the relevant noble/mockLogger interactions.tests/unit/device/protocol/airotic/airoticProtocol.spec.ts (1)
2-2: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSource filename typo:
airtonicProtocol.tsvs.airoticconvention.Every other Airotic file/directory uses the "airotic" spelling (
airoticDevice.ts,airoticDeviceProvider.ts,airotic/folder), but the protocol module and this import are namedairtonicProtocol. Worth renaming for consistency.🤖 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 `@tests/unit/device/protocol/airotic/airoticProtocol.spec.ts` at line 2, The Airotic protocol module name is misspelled and inconsistent with the rest of the Airotic symbols, so rename the protocol file/module from the current airtonic spelling to the airotic convention and update the import in AiroticProtocol spec accordingly. Make sure the exported default referenced by AiroticProtocol and any related module paths under the airotic folder all use the same airotic naming so the test and source imports stay aligned.
🤖 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 424-439: The processQueue error handling in scriptRuntime uses
different formatting for the same exception between logging and console
emission. Update the catch block in processQueue so the consoleLog event uses
the same normalized msg value already derived from e (via Error.message or
String(e)) instead of String(e), keeping AutomationEventType.consoleLog
consistent with logger.error and this.log output.
- Around line 24-47: Update the bootstrap JSDoc in scriptRuntime.ts to match the
current event API: the onEvent(handler) contract and __dispatchEvent
implementation use handler(device, ...args), not an event wrapper object. Remove
or rewrite the stale event.* bullets (event.type, event.device.getDeviceId,
event.device.getAttribute, etc.) and document the actual device argument and
payload shape using the onEvent and __dispatchEvent symbols as the source of
truth.
- Around line 101-115: The Device proxy’s setAttribute method resolves
immediately instead of reflecting the host write, so await on it can finish
before the underlying update completes. Update __createDeviceProxy’s
setAttribute to return the actual promise from __setAttribute/applySync path (or
wrap it so failures reject), and make sure the caller in the host-side device
handling around dev.setAttribute propagates the async result instead of only
logging .catch(). This will let scripts wait for the write and handle errors
from setAttribute properly.
In `@src/device/deviceManager.ts`:
- Around line 167-175: The reset() method in DeviceManager stops early if any
device.close() rejects, leaving later connected devices and
detectedDeviceAcquireQueue entries uncleared. Update reset() so it always
attempts to close every device and drain every pending acquire queue entry,
using the existing connectedDevices and clearDetectedDeviceAcquireQueue paths,
and make sure one close failure does not prevent the rest of the cleanup from
running.
In `@src/device/protocol/virtual/virtualDeviceProvider.ts`:
- Around line 42-57: The virtual device discovery flow in
virtualDeviceProvider’s init/discoverVirtualDevices path can still add devices
after stop() because an in-flight async scan is not guarded. Add a
shutdown/lifecycle check in discoverVirtualDevices and around the
deviceFactory.create/deviceManager.addDevice sequence so any scan that resumes
after stop() exits without registering devices, and make stop() set that guard
before clearing discoveryInterval and removing connected devices.
In `@src/device/provider/bleDeviceProvider.ts`:
- Around line 77-89: The disconnectPeripheral cleanup only handles peripherals
in connected state, so a timed-out connect can leave the peripheral stuck in
connecting. Update disconnectPeripheral in bleDeviceProvider to mirror
BleDevice.doClose by handling both connected and connecting states, and when the
peripheral is connecting call cancelConnect() before/alongside disconnectAsync.
Keep the existing timeout and logError handling intact.
---
Outside diff comments:
In `@src/device/bleDevice.ts`:
- Around line 35-79: The reconnect handler in BLEDevice has a misleading log
message in the `else` branch of the `peripheral.state` check: it says the device
“is not in disconnected state” even though that branch runs when the peripheral
is already connected. Update the message in `reconnectHandler` to accurately
describe the current state and action taken, keeping the condition and the
logged state aligned so debugging `BLEDevice` reconnect behavior is not
confusing.
- Around line 98-117: The special-case branch in doClose for the “BLEManager has
already been cleaned up” disconnect error is returning too early and skipping
cleanup. Update doClose in BLEDevice so that this error is logged/ignored but
execution continues to clearInterval(this.rssiInterval) and remove the
disconnect listener with this.peripheral.off('disconnect',
this.reconnectHandler), matching the normal cleanup path; make sure the
disconnect error handling still stays inside the connected-state branch.
In `@src/device/transport/bleDeviceTransport.ts`:
- Around line 46-108: The BLE reconnect path in subscribe() only forces
disconnect when the peripheral is already connected, so a failure during an
in-flight connect can leave the connect attempt running. Update the catch block
in BLEDeviceTransport.subscribe() to call cancelConnect() when the peripheral is
in the connecting state, and keep disconnectAsync() only for the connected case
so BleDevice.reconnectHandler can recover cleanly.
---
Nitpick comments:
In `@src/app.ts`:
- Around line 184-204: The Express app setup in createApp is missing
Helmet-based baseline security headers, which static analysis flagged. Update
the middleware chain in createApp to include Helmet alongside the existing cors,
contentTypeMiddleware, express.json, and express.text setup, keeping the current
PNA preflight handling intact. Use the createApp symbol as the place to add the
security middleware so the app gets standard headers like X-Content-Type-Options
and X-Frame-Options by default.
In `@src/device/deviceManager.ts`:
- Around line 144-146: The DeviceManager lookup API is still too loose because
getConnectedDevice currently accepts a plain string instead of the branded
DeviceId. Update the getConnectedDevice method signature to use DeviceId so the
manager boundary preserves the identity contract, and make sure any internal
uses of connectedDevices.get continue to work with that branded type.
In `@src/device/provider/deviceProviderManager.ts`:
- Around line 42-46: startProviders() in DeviceProviderManager is not resilient
to a single provider.init() failure, so one rejected init aborts later providers
from starting. Update DeviceProviderManager.startProviders() to mirror
stopProviders() by wrapping each provider.init() call in per-provider error
handling, logging the failure with the provider identity, and continuing the
loop so unrelated providers still initialize.
In `@src/device/transport/deviceBidirectionalTransport.ts`:
- Around line 4-11: Rename the default exported interface from DeviceTransport
to DeviceBidirectionalTransport in the deviceBidirectionalTransport module, and
update the interface declaration so the exported contract name matches the
bidirectional transport role. Make sure any references within this module that
rely on the interface symbol are updated consistently so diagnostics and
generated declarations no longer expose the legacy DeviceTransport name.
In `@src/serialization/classToPlainSerializer.ts`:
- Around line 12-13: The new transform overload in classToPlainSerializer still
lets callers assert an unchecked generic return type, so it does not provide
real runtime type safety. Either keep the API explicitly as a typed
assertion-style helper or narrow the signature in transform so it only returns
Record<string, unknown> unless the shape is actually validated, and ensure the
implementation matches the intended contract for TOut and TypeOptions.
In `@src/util/async.ts`:
- Around line 92-106: Document in promiseWithTimeout that it only rejects the
wrapper on timeout and does not cancel the underlying promise or side effects.
Update the API docs/comments near promiseWithTimeout to make
cleanup/cancellation responsibility explicit for callers, especially where it is
used for BLE setup. Refer to promiseWithTimeout and the Promise.race timeout
behavior so future users understand the wrapped operation continues running
after timeout.
In `@tests/unit/automation/scriptRuntime.spec.ts`:
- Around line 1-472: Add a test that covers the `SupportedDeviceEvent`
`deviceNotification` path in `ScriptRuntime`. The current spec only exercises
`deviceConnected` and `deviceDisconnected` with empty `args`, so it misses the
`handler(device, ...args)` spread behavior in `__dispatchEvent`. In
`tests/unit/automation/scriptRuntime.spec.ts`, dispatch a
`DeviceManagerEvent.deviceNotification` event with a notification payload in
`args` and assert the `onEvent('deviceNotification', ...)` handler receives that
payload. Use the existing `dispatchAndCollect`, `runtime.runForEvent`, and
`deviceA`/`StubDevice` setup to keep the test consistent.
In `@tests/unit/device/bleDevice.spec.ts`:
- Around line 80-86: The current constructor spec only verifies that `BleDevice`
registers the disconnect listener, but it does not cover the `reconnectHandler`
logic passed to `mockPeripheral.on('disconnect', ...)`. Update the
`bleDevice.spec.ts` constructor test to capture that handler and exercise all
branches in `reconnectHandler`: a successful `connectAsync()` reconnect, the
already `'connected'` early-return path, and the error path that triggers
`this.close()`. Use the existing `createDevice`, `mockPeripheral`, and
`connectAsync`/`close` mocks to assert the expected behavior.
In `@tests/unit/device/protocol/airotic/airoticProtocol.spec.ts`:
- Line 2: The Airotic protocol module name is misspelled and inconsistent with
the rest of the Airotic symbols, so rename the protocol file/module from the
current airtonic spelling to the airotic convention and update the import in
AiroticProtocol spec accordingly. Make sure the exported default referenced by
AiroticProtocol and any related module paths under the airotic folder all use
the same airotic naming so the test and source imports stay aligned.
In `@tests/unit/device/protocol/slvCtrlPlus/genericSlvCtrlPlusDevice.spec.ts`:
- Around line 188-339: The three new refresh tests duplicate the same
GenericSlvCtrlPlusDevice setup, so refactor them to reuse the existing
createDevice helper instead of repeating the 12-argument constructor. Update
createDevice to accept the attrs override and optionally return or accept the
Logger mock if needed, then use it in the bool/str, empty-string, and
unknown-attribute tests to keep only the per-test protocol/transport stubbing
inline.
In `@tests/unit/device/protocol/zc95/zc95Device.spec.ts`:
- Around line 685-714: The test case in processPowerStatusMessage is not
actually exercising the clamp behavior because the final assignment to
MaxOutputPower overwrites the earlier clamped value, so the over-limit setup
does not prove a distinct code path. Update the spec around getOnReceiveCallback
and device.getAttribute('powerChannel1') to either use a scenario that reaches
the clamp logic without being overwritten, or simplify the assertion to match
the current behavior and avoid implying clamp-specific coverage.
In `@tests/unit/device/transport/bleObserver.spec.ts`:
- Around line 130-193: Add dedicated tests for BleObserver.stop() in
bleObserver.spec.ts using the existing createObserver and noble mocks. Cover the
shutdown path where stop() is called while scanning: verify it removes the noble
listeners, awaits stopScanningAsync, and calls noble.stop(). Also add a no-op
case when scanning is already inactive so stop() does not try to stop scanning
again. Keep the assertions centered on the BleObserver.stop method and the
relevant noble/mockLogger interactions.
In `@tests/unit/device/transport/serialPortObserver.spec.ts`:
- Around line 118-127: The test for SerialPortObserver deduplication is only
checking the initial scan and never triggers a second discovery pass, so it does
not verify the “already managed” behavior. Update the spec to invoke
SerialPortObserver.discoverSerialDevices() again after the first start/scan,
using the existing createObserver and mockDeviceManager.announceDetectedDevice
setup, so the assertion truly validates that a previously managed device is not
re-announced on a subsequent discovery run.
🪄 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: 8b032485-44a5-4302-a008-af0368800890
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (90)
.gitignoreeslint.config.tspackage.jsonsrc/app.tssrc/automation/scriptRuntime.tssrc/controller/automation/deleteScriptController.tssrc/controller/automation/getScriptController.tssrc/controller/automation/runScriptController.tssrc/controller/getDeviceController.tssrc/controller/patchDeviceController.tssrc/device/attribute/intRangeDeviceAttribute.tssrc/device/attribute/strDeviceAttribute.tssrc/device/bleDevice.tssrc/device/device.tssrc/device/deviceId.tssrc/device/deviceManager.tssrc/device/peripheralDevice.tssrc/device/protocol/airotic/airoticDevice.tssrc/device/protocol/airotic/airoticDeviceProvider.tssrc/device/protocol/buttplugIo/buttplugIoDevice.tssrc/device/protocol/buttplugIo/buttplugIoDeviceFactory.tssrc/device/protocol/buttplugIo/buttplugIoWebsocketDeviceProvider.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/virtualDevice.tssrc/device/protocol/virtual/virtualDeviceLogic.tssrc/device/protocol/virtual/virtualDeviceProvider.tssrc/device/protocol/zc95/zc95Device.tssrc/device/protocol/zc95/zc95DeviceFactory.tssrc/device/protocol/zc95/zc95SerialDeviceProvider.tssrc/device/provider/bleDeviceProvider.tssrc/device/provider/deviceProvider.tssrc/device/provider/deviceProviderManager.tssrc/device/provider/serialDeviceProvider.tssrc/device/transport/bleDeviceTransport.tssrc/device/transport/bleObserver.tssrc/device/transport/deviceBidirectionalTransport.tssrc/device/transport/deviceReadableTransport.tssrc/device/transport/deviceWritableTransport.tssrc/device/transport/serialDeviceTransport.tssrc/device/transport/serialPortObserver.tssrc/device/types.d.tssrc/device/webSocketEvent.tssrc/index.tssrc/repository/connectedDeviceRepository.tssrc/serial/synchronousSerialPort.tssrc/serialization/classToPlainSerializer.tssrc/serviceMap.tssrc/serviceProvider/deviceServiceProvider.tssrc/serviceProvider/settingsServiceProvider.tssrc/socket/types.tssrc/types.d.tssrc/util/async.tssrc/util/color.tssrc/util/expressUtils.tssrc/util/objects.tstests/integration/automationScripts.spec.tstests/integration/deviceEvents.spec.tstests/integration/devices/airoticDevice.spec.tstests/integration/helpers/airoticDeviceSimulator.tstests/integration/helpers/appHelper.tstests/unit/automation/scriptRuntime.spec.tstests/unit/device/bleDevice.spec.tstests/unit/device/deviceManager.spec.tstests/unit/device/protocol/airotic/airoticDevice.spec.tstests/unit/device/protocol/airotic/airoticProtocol.spec.tstests/unit/device/protocol/buttplugIo/buttplugIoDevice.spec.tstests/unit/device/protocol/estim2b/estim2bDevice.spec.tstests/unit/device/protocol/estim2b/estim2bProtocol.spec.tstests/unit/device/protocol/slvCtrlPlus/genericSlvCtrlPlusDevice.spec.tstests/unit/device/protocol/slvCtrlPlus/slvCtrlProtocolLegacy.spec.tstests/unit/device/protocol/slvCtrlPlus/slvCtrlProtocolV1.spec.tstests/unit/device/protocol/virtual/display/displayVirtualDevice.spec.tstests/unit/device/protocol/zc95/zc95Device.spec.tstests/unit/device/protocol/zc95/zc95Protocol.spec.tstests/unit/device/testDevice.tstests/unit/device/testDeviceProvider.tstests/unit/device/transport/bleObserver.spec.tstests/unit/device/transport/serialPortObserver.spec.tstsconfig.jsonvitest.config.integration.tsvitest.config.tsvitest.config.unit.ts
✅ Files skipped from review due to trivial changes (6)
- .gitignore
- vitest.config.unit.ts
- src/device/attribute/intRangeDeviceAttribute.ts
- src/util/objects.ts
- src/device/transport/deviceWritableTransport.ts
- tsconfig.json
🚧 Files skipped from review as they are similar to previous changes (31)
- src/device/provider/deviceProvider.ts
- src/serial/synchronousSerialPort.ts
- src/serviceProvider/settingsServiceProvider.ts
- src/util/expressUtils.ts
- src/device/deviceId.ts
- eslint.config.ts
- src/device/protocol/buttplugIo/buttplugIoWebsocketDeviceProvider.ts
- src/controller/getDeviceController.ts
- package.json
- src/controller/automation/runScriptController.ts
- src/device/protocol/zc95/zc95DeviceFactory.ts
- src/serviceMap.ts
- src/device/protocol/estim2b/estim2bDeviceFactory.ts
- src/controller/automation/getScriptController.ts
- src/controller/automation/deleteScriptController.ts
- src/device/protocol/virtual/virtualDevice.ts
- src/device/protocol/slvCtrlPlus/genericSlvCtrlPlusDevice.ts
- src/device/transport/bleObserver.ts
- tests/unit/device/deviceManager.spec.ts
- src/device/transport/serialPortObserver.ts
- src/device/protocol/buttplugIo/buttplugIoDeviceFactory.ts
- src/device/protocol/estim2b/estim2bSerialDeviceProvider.ts
- src/device/protocol/slvCtrlPlus/slvCtrlPlusDeviceFactory.ts
- src/device/protocol/zc95/zc95SerialDeviceProvider.ts
- src/serviceProvider/deviceServiceProvider.ts
- src/device/protocol/messageResponseHandler.ts
- src/device/protocol/slvCtrlPlus/slvCtrlPlusSerialDeviceProvider.ts
- src/device/provider/serialDeviceProvider.ts
- src/controller/patchDeviceController.ts
- tests/unit/device/protocol/buttplugIo/buttplugIoDevice.spec.ts
- src/device/protocol/zc95/zc95Device.ts
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/device/bleDevice.ts (2)
35-79: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReconnect-handler log message contradicts its own condition.
At Line 66-68, the
elsebranch (only reached whenperipheral.state === 'connected') logs"is not in disconnected state"— the message is worded backwards and will mislead whoever debugs a future reconnect issue (this exact class of confusion is what caused the original hang report).🐛 Proposed fix
} else { - this.logger.warn(`BLE Device ${this.deviceId} is not in disconnected state, current state: ${peripheral.state}`); + this.logger.warn(`BLE Device ${this.deviceId} is already connected, skipping reconnect attempt`); }🤖 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/bleDevice.ts` around lines 35 - 79, The reconnect handler in BLEDevice has a misleading log message in the `else` branch of the `peripheral.state` check: it says the device “is not in disconnected state” even though that branch runs when the peripheral is already connected. Update the message in `reconnectHandler` to accurately describe the current state and action taken, keeping the condition and the logged state aligned so debugging `BLEDevice` reconnect behavior is not confusing.
98-117: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winKeep cleanup on the cleaned-up disconnect path.
returnhere skipsclearInterval(this.rssiInterval)andthis.peripheral.off('disconnect', this.reconnectHandler), so closing a device after the BLE manager is gone can leave the RSSI timer running and the reconnect listener attached. The special case should log/ignore the disconnect error, then continue to cleanup;tests/unit/device/bleDevice.spec.tsdoes not cover this branch.🤖 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/bleDevice.ts` around lines 98 - 117, The special-case branch in doClose for the “BLEManager has already been cleaned up” disconnect error is returning too early and skipping cleanup. Update doClose in BLEDevice so that this error is logged/ignored but execution continues to clearInterval(this.rssiInterval) and remove the disconnect listener with this.peripheral.off('disconnect', this.reconnectHandler), matching the normal cleanup path; make sure the disconnect error handling still stays inside the connected-state branch.src/device/transport/bleDeviceTransport.ts (1)
46-108: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAbort in-flight connects with
cancelConnect()
subscribe()can throw whileperipheral.state === 'connecting';disconnectAsync()won’t stop that attempt. Cancel the pending connect there, and keepdisconnectAsync()for the already-connected case so the reconnect handler can restart the cycle.🤖 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/bleDeviceTransport.ts` around lines 46 - 108, The BLE reconnect path in subscribe() only forces disconnect when the peripheral is already connected, so a failure during an in-flight connect can leave the connect attempt running. Update the catch block in BLEDeviceTransport.subscribe() to call cancelConnect() when the peripheral is in the connecting state, and keep disconnectAsync() only for the connected case so BleDevice.reconnectHandler can recover cleanly.
🧹 Nitpick comments (13)
src/serialization/classToPlainSerializer.ts (1)
12-13: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOverload still permits caller-asserted, unverified return type.
Replacing
result as Vwith an overload that returnsTOut extends Record<string, unknown>removes the explicit cast, but the implementation still always returns a bareRecord<string, unknown>; callers specifyingtransform<SerializedDevice>(...)get no runtime guarantee the shape matches. This is functionally equivalent to the prior unsafe cast, just relocated to the call site's generic argument — acceptable if intentional (similar toJSON.parse<T>()patterns), but worth being aware it's not a genuine type-safety improvement.🤖 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/serialization/classToPlainSerializer.ts` around lines 12 - 13, The new transform overload in classToPlainSerializer still lets callers assert an unchecked generic return type, so it does not provide real runtime type safety. Either keep the API explicitly as a typed assertion-style helper or narrow the signature in transform so it only returns Record<string, unknown> unless the shape is actually validated, and ensure the implementation matches the intended contract for TOut and TypeOptions.src/device/provider/deviceProviderManager.ts (1)
42-46: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
startProviders()isn't resilient to a single provider failing.Unlike
stopProviders(), which catches per-provider errors and continues,startProviders()lets anyprovider.init()rejection abort the loop, so providers ordered after a failing one never start. A single flaky provider (e.g., BLE) could prevent unrelated providers (serial, virtual, etc.) from initializing.♻️ Suggested fix mirroring stopProviders()' error handling
public async startProviders(): Promise<void> { - for (const provider of this.providers) { - await provider.init(); - } + const errors: unknown[] = []; + + for (const provider of this.providers) { + try { + await provider.init(); + } catch (error: unknown) { + errors.push(error); + this.logger.error('Failed to start device provider', error); + } + } + + if (errors.length > 0) { + throw new Error(`Failed to start ${errors.length} device provider(s)`); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/device/provider/deviceProviderManager.ts` around lines 42 - 46, startProviders() in DeviceProviderManager is not resilient to a single provider.init() failure, so one rejected init aborts later providers from starting. Update DeviceProviderManager.startProviders() to mirror stopProviders() by wrapping each provider.init() call in per-provider error handling, logging the failure with the provider identity, and continuing the loop so unrelated providers still initialize.src/device/deviceManager.ts (1)
144-146: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the
DeviceIdbrand on manager lookups.Line 144 still accepts any
string, while callers likeConnectedDeviceRepository.getById()now passDeviceId. Tightening this preserves the new identity contract at the manager boundary.- public getConnectedDevice(deviceId: string): Device|null + public getConnectedDevice(deviceId: DeviceId): Device|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/deviceManager.ts` around lines 144 - 146, The DeviceManager lookup API is still too loose because getConnectedDevice currently accepts a plain string instead of the branded DeviceId. Update the getConnectedDevice method signature to use DeviceId so the manager boundary preserves the identity contract, and make sure any internal uses of connectedDevices.get continue to work with that branded type.tests/unit/automation/scriptRuntime.spec.ts (1)
1-472: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding coverage for the
deviceNotificationevent variant.All tests dispatch
deviceConnected/deviceDisconnected(emptyargs: []).SupportedDeviceEvent'sdeviceNotificationbranch is the only one with a non-emptyargstuple exercising thehandler(device, ...args)spread path in__dispatchEvent. A test dispatching{ type: DeviceManagerEvent.deviceNotification, device, args: [notification] }and asserting the handler receives the notification payload would close a coverage gap on the new dispatch mechanism.🤖 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 `@tests/unit/automation/scriptRuntime.spec.ts` around lines 1 - 472, Add a test that covers the `SupportedDeviceEvent` `deviceNotification` path in `ScriptRuntime`. The current spec only exercises `deviceConnected` and `deviceDisconnected` with empty `args`, so it misses the `handler(device, ...args)` spread behavior in `__dispatchEvent`. In `tests/unit/automation/scriptRuntime.spec.ts`, dispatch a `DeviceManagerEvent.deviceNotification` event with a notification payload in `args` and assert the `onEvent('deviceNotification', ...)` handler receives that payload. Use the existing `dispatchAndCollect`, `runtime.runForEvent`, and `deviceA`/`StubDevice` setup to keep the test consistent.src/util/async.ts (1)
92-106: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument that the wrapped operation keeps running after timeout.
promiseWithTimeoutrejects the wrapper only; it does not cancelpromise. Since call sites use it around side-effectful BLE setup, make the cleanup/cancellation responsibility explicit.📝 Proposed API documentation
+/** + * Rejects if `promise` does not settle within `timeoutMs`. + * + * Note: this does not cancel the underlying promise. Callers wrapping + * side-effectful operations must cancel or clean up that work themselves. + */ export const promiseWithTimeout = <T>(Based on learnings,
Promise.racetimeouts do not cancel the underlying callback and this limitation should be recorded in code comments/API docs.🤖 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/util/async.ts` around lines 92 - 106, Document in promiseWithTimeout that it only rejects the wrapper on timeout and does not cancel the underlying promise or side effects. Update the API docs/comments near promiseWithTimeout to make cleanup/cancellation responsibility explicit for callers, especially where it is used for BLE setup. Refer to promiseWithTimeout and the Promise.race timeout behavior so future users understand the wrapped operation continues running after timeout.Source: Learnings
src/device/transport/deviceBidirectionalTransport.ts (1)
4-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename the exported interface to
DeviceBidirectionalTransport.The default export still uses the legacy
DeviceTransportname, which leaks into diagnostics and declaration output even though this module now models the bidirectional transport contract.♻️ Proposed rename
-export default interface DeviceTransport extends DeviceReadableTransport, DeviceWritableTransport +export default interface DeviceBidirectionalTransport extends DeviceReadableTransport, DeviceWritableTransport🤖 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/deviceBidirectionalTransport.ts` around lines 4 - 11, Rename the default exported interface from DeviceTransport to DeviceBidirectionalTransport in the deviceBidirectionalTransport module, and update the interface declaration so the exported contract name matches the bidirectional transport role. Make sure any references within this module that rely on the interface symbol are updated consistently so diagnostics and generated declarations no longer expose the legacy DeviceTransport name.tests/unit/device/protocol/slvCtrlPlus/genericSlvCtrlPlusDevice.spec.ts (1)
188-339: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider reusing the
createDevicehelper instead of duplicating the constructor call.All three new tests repeat the same 12-argument
GenericSlvCtrlPlusDeviceconstruction verbatim (differing only inattrs). ExtendingcreateDeviceto optionally accept/return the logger mock would remove this duplication.♻️ Example refactor sketch
- function createDevice( - attrs: SlvCtrlPlusDeviceAttributes, - protocol: SlvCtrlProtocol, - transport: DeviceBidirectionalTransport, - ): GenericSlvCtrlPlusDevice { + function createDevice( + attrs: SlvCtrlPlusDeviceAttributes, + protocol: SlvCtrlProtocol, + transport: DeviceBidirectionalTransport, + logger: Logger = mock<Logger>(), + ): GenericSlvCtrlPlusDevice { const fwVersion = 10000; const deviceUuid = DeviceId.create('foo-bar-baz'); const deviceName = 'Aston Martin'; const model = 'et312'; const protocolVersion = 10000; const provider = 'dummy'; return new GenericSlvCtrlPlusDevice( fwVersion, deviceUuid, deviceName, model, provider, new Date(), protocol, transport, protocolVersion, attrs, - mock<EventEmitter>(), mock<Logger>(), + mock<EventEmitter>(), logger, ); }🤖 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 `@tests/unit/device/protocol/slvCtrlPlus/genericSlvCtrlPlusDevice.spec.ts` around lines 188 - 339, The three new refresh tests duplicate the same GenericSlvCtrlPlusDevice setup, so refactor them to reuse the existing createDevice helper instead of repeating the 12-argument constructor. Update createDevice to accept the attrs override and optionally return or accept the Logger mock if needed, then use it in the bool/str, empty-string, and unknown-attribute tests to keep only the per-test protocol/transport stubbing inline.tests/unit/device/protocol/zc95/zc95Device.spec.ts (1)
685-714: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExercise the clamp path or simplify this case.
processPowerStatusMessageoverwrites the clamped value withMaxOutputPower, so this over-limit setup still ends at50and doesn’t distinguish clamp vs. no-clamp behavior.🤖 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 `@tests/unit/device/protocol/zc95/zc95Device.spec.ts` around lines 685 - 714, The test case in processPowerStatusMessage is not actually exercising the clamp behavior because the final assignment to MaxOutputPower overwrites the earlier clamped value, so the over-limit setup does not prove a distinct code path. Update the spec around getOnReceiveCallback and device.getAttribute('powerChannel1') to either use a scenario that reaches the clamp logic without being overwritten, or simplify the assertion to match the current behavior and avoid implying clamp-specific coverage.tests/unit/device/bleDevice.spec.ts (1)
80-86: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for
reconnectHandlerbehavior.Only the
disconnectlistener registration is tested. Consider capturing the handler passed tomockPeripheral.on('disconnect', ...)and exercising: successfulconnectAsync(), the already-'connected'branch, and the failure path that callsthis.close(). This is the exact logic implicated in the PR's reported reconnect hang.🤖 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 `@tests/unit/device/bleDevice.spec.ts` around lines 80 - 86, The current constructor spec only verifies that `BleDevice` registers the disconnect listener, but it does not cover the `reconnectHandler` logic passed to `mockPeripheral.on('disconnect', ...)`. Update the `bleDevice.spec.ts` constructor test to capture that handler and exercise all branches in `reconnectHandler`: a successful `connectAsync()` reconnect, the already `'connected'` early-return path, and the error path that triggers `this.close()`. Use the existing `createDevice`, `mockPeripheral`, and `connectAsync`/`close` mocks to assert the expected behavior.tests/unit/device/transport/serialPortObserver.spec.ts (1)
118-127: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest doesn't exercise a second discovery run.
SerialPortObserveronly rescans in response tousbconnect/disconnectevents (debounced viasetTimeout), per the observer source in the relevant snippets. This test advances fake timers by 3000ms but never dispatches ausbevent, sodiscoverSerialDevices()is never invoked a second time — the assertion (toHaveBeenCalledOnce) is trivially true even without any dedup logic working. Other tests in this file (e.g. "revokes a device that disappears...") correctly callobserver.discoverSerialDevices()directly to exercise a second run; consider doing the same here to actually validate the "already managed" dedup behavior.🤖 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 `@tests/unit/device/transport/serialPortObserver.spec.ts` around lines 118 - 127, The test for SerialPortObserver deduplication is only checking the initial scan and never triggers a second discovery pass, so it does not verify the “already managed” behavior. Update the spec to invoke SerialPortObserver.discoverSerialDevices() again after the first start/scan, using the existing createObserver and mockDeviceManager.announceDetectedDevice setup, so the assertion truly validates that a previously managed device is not re-announced on a subsequent discovery run.src/app.ts (1)
184-204: 🔒 Security & Privacy | 🔵 TrivialConsider adding Helmet for baseline security headers.
Static analysis flags the Express app as lacking Helmet-provided headers (X-Content-Type-Options, X-Frame-Options, etc.). Not blocking for this PR's BLE scope, but worth tracking.
🤖 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/app.ts` around lines 184 - 204, The Express app setup in createApp is missing Helmet-based baseline security headers, which static analysis flagged. Update the middleware chain in createApp to include Helmet alongside the existing cors, contentTypeMiddleware, express.json, and express.text setup, keeping the current PNA preflight handling intact. Use the createApp symbol as the place to add the security middleware so the app gets standard headers like X-Content-Type-Options and X-Frame-Options by default.Source: Linters/SAST tools
tests/unit/device/transport/bleObserver.spec.ts (1)
130-193: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing test coverage for
BleObserver.stop().
stop()is invoked on app shutdown (src/app.tscallsdevice.observer.ble.stop()) but has no dedicated test here (listener removal,stopScanningAsynccall when scanning,noble.stop()call, and the no-op path when not scanning).🤖 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 `@tests/unit/device/transport/bleObserver.spec.ts` around lines 130 - 193, Add dedicated tests for BleObserver.stop() in bleObserver.spec.ts using the existing createObserver and noble mocks. Cover the shutdown path where stop() is called while scanning: verify it removes the noble listeners, awaits stopScanningAsync, and calls noble.stop(). Also add a no-op case when scanning is already inactive so stop() does not try to stop scanning again. Keep the assertions centered on the BleObserver.stop method and the relevant noble/mockLogger interactions.tests/unit/device/protocol/airotic/airoticProtocol.spec.ts (1)
2-2: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSource filename typo:
airtonicProtocol.tsvs.airoticconvention.Every other Airotic file/directory uses the "airotic" spelling (
airoticDevice.ts,airoticDeviceProvider.ts,airotic/folder), but the protocol module and this import are namedairtonicProtocol. Worth renaming for consistency.🤖 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 `@tests/unit/device/protocol/airotic/airoticProtocol.spec.ts` at line 2, The Airotic protocol module name is misspelled and inconsistent with the rest of the Airotic symbols, so rename the protocol file/module from the current airtonic spelling to the airotic convention and update the import in AiroticProtocol spec accordingly. Make sure the exported default referenced by AiroticProtocol and any related module paths under the airotic folder all use the same airotic naming so the test and source imports stay aligned.
🤖 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 424-439: The processQueue error handling in scriptRuntime uses
different formatting for the same exception between logging and console
emission. Update the catch block in processQueue so the consoleLog event uses
the same normalized msg value already derived from e (via Error.message or
String(e)) instead of String(e), keeping AutomationEventType.consoleLog
consistent with logger.error and this.log output.
- Around line 24-47: Update the bootstrap JSDoc in scriptRuntime.ts to match the
current event API: the onEvent(handler) contract and __dispatchEvent
implementation use handler(device, ...args), not an event wrapper object. Remove
or rewrite the stale event.* bullets (event.type, event.device.getDeviceId,
event.device.getAttribute, etc.) and document the actual device argument and
payload shape using the onEvent and __dispatchEvent symbols as the source of
truth.
- Around line 101-115: The Device proxy’s setAttribute method resolves
immediately instead of reflecting the host write, so await on it can finish
before the underlying update completes. Update __createDeviceProxy’s
setAttribute to return the actual promise from __setAttribute/applySync path (or
wrap it so failures reject), and make sure the caller in the host-side device
handling around dev.setAttribute propagates the async result instead of only
logging .catch(). This will let scripts wait for the write and handle errors
from setAttribute properly.
In `@src/device/deviceManager.ts`:
- Around line 167-175: The reset() method in DeviceManager stops early if any
device.close() rejects, leaving later connected devices and
detectedDeviceAcquireQueue entries uncleared. Update reset() so it always
attempts to close every device and drain every pending acquire queue entry,
using the existing connectedDevices and clearDetectedDeviceAcquireQueue paths,
and make sure one close failure does not prevent the rest of the cleanup from
running.
In `@src/device/protocol/virtual/virtualDeviceProvider.ts`:
- Around line 42-57: The virtual device discovery flow in
virtualDeviceProvider’s init/discoverVirtualDevices path can still add devices
after stop() because an in-flight async scan is not guarded. Add a
shutdown/lifecycle check in discoverVirtualDevices and around the
deviceFactory.create/deviceManager.addDevice sequence so any scan that resumes
after stop() exits without registering devices, and make stop() set that guard
before clearing discoveryInterval and removing connected devices.
In `@src/device/provider/bleDeviceProvider.ts`:
- Around line 77-89: The disconnectPeripheral cleanup only handles peripherals
in connected state, so a timed-out connect can leave the peripheral stuck in
connecting. Update disconnectPeripheral in bleDeviceProvider to mirror
BleDevice.doClose by handling both connected and connecting states, and when the
peripheral is connecting call cancelConnect() before/alongside disconnectAsync.
Keep the existing timeout and logError handling intact.
---
Outside diff comments:
In `@src/device/bleDevice.ts`:
- Around line 35-79: The reconnect handler in BLEDevice has a misleading log
message in the `else` branch of the `peripheral.state` check: it says the device
“is not in disconnected state” even though that branch runs when the peripheral
is already connected. Update the message in `reconnectHandler` to accurately
describe the current state and action taken, keeping the condition and the
logged state aligned so debugging `BLEDevice` reconnect behavior is not
confusing.
- Around line 98-117: The special-case branch in doClose for the “BLEManager has
already been cleaned up” disconnect error is returning too early and skipping
cleanup. Update doClose in BLEDevice so that this error is logged/ignored but
execution continues to clearInterval(this.rssiInterval) and remove the
disconnect listener with this.peripheral.off('disconnect',
this.reconnectHandler), matching the normal cleanup path; make sure the
disconnect error handling still stays inside the connected-state branch.
In `@src/device/transport/bleDeviceTransport.ts`:
- Around line 46-108: The BLE reconnect path in subscribe() only forces
disconnect when the peripheral is already connected, so a failure during an
in-flight connect can leave the connect attempt running. Update the catch block
in BLEDeviceTransport.subscribe() to call cancelConnect() when the peripheral is
in the connecting state, and keep disconnectAsync() only for the connected case
so BleDevice.reconnectHandler can recover cleanly.
---
Nitpick comments:
In `@src/app.ts`:
- Around line 184-204: The Express app setup in createApp is missing
Helmet-based baseline security headers, which static analysis flagged. Update
the middleware chain in createApp to include Helmet alongside the existing cors,
contentTypeMiddleware, express.json, and express.text setup, keeping the current
PNA preflight handling intact. Use the createApp symbol as the place to add the
security middleware so the app gets standard headers like X-Content-Type-Options
and X-Frame-Options by default.
In `@src/device/deviceManager.ts`:
- Around line 144-146: The DeviceManager lookup API is still too loose because
getConnectedDevice currently accepts a plain string instead of the branded
DeviceId. Update the getConnectedDevice method signature to use DeviceId so the
manager boundary preserves the identity contract, and make sure any internal
uses of connectedDevices.get continue to work with that branded type.
In `@src/device/provider/deviceProviderManager.ts`:
- Around line 42-46: startProviders() in DeviceProviderManager is not resilient
to a single provider.init() failure, so one rejected init aborts later providers
from starting. Update DeviceProviderManager.startProviders() to mirror
stopProviders() by wrapping each provider.init() call in per-provider error
handling, logging the failure with the provider identity, and continuing the
loop so unrelated providers still initialize.
In `@src/device/transport/deviceBidirectionalTransport.ts`:
- Around line 4-11: Rename the default exported interface from DeviceTransport
to DeviceBidirectionalTransport in the deviceBidirectionalTransport module, and
update the interface declaration so the exported contract name matches the
bidirectional transport role. Make sure any references within this module that
rely on the interface symbol are updated consistently so diagnostics and
generated declarations no longer expose the legacy DeviceTransport name.
In `@src/serialization/classToPlainSerializer.ts`:
- Around line 12-13: The new transform overload in classToPlainSerializer still
lets callers assert an unchecked generic return type, so it does not provide
real runtime type safety. Either keep the API explicitly as a typed
assertion-style helper or narrow the signature in transform so it only returns
Record<string, unknown> unless the shape is actually validated, and ensure the
implementation matches the intended contract for TOut and TypeOptions.
In `@src/util/async.ts`:
- Around line 92-106: Document in promiseWithTimeout that it only rejects the
wrapper on timeout and does not cancel the underlying promise or side effects.
Update the API docs/comments near promiseWithTimeout to make
cleanup/cancellation responsibility explicit for callers, especially where it is
used for BLE setup. Refer to promiseWithTimeout and the Promise.race timeout
behavior so future users understand the wrapped operation continues running
after timeout.
In `@tests/unit/automation/scriptRuntime.spec.ts`:
- Around line 1-472: Add a test that covers the `SupportedDeviceEvent`
`deviceNotification` path in `ScriptRuntime`. The current spec only exercises
`deviceConnected` and `deviceDisconnected` with empty `args`, so it misses the
`handler(device, ...args)` spread behavior in `__dispatchEvent`. In
`tests/unit/automation/scriptRuntime.spec.ts`, dispatch a
`DeviceManagerEvent.deviceNotification` event with a notification payload in
`args` and assert the `onEvent('deviceNotification', ...)` handler receives that
payload. Use the existing `dispatchAndCollect`, `runtime.runForEvent`, and
`deviceA`/`StubDevice` setup to keep the test consistent.
In `@tests/unit/device/bleDevice.spec.ts`:
- Around line 80-86: The current constructor spec only verifies that `BleDevice`
registers the disconnect listener, but it does not cover the `reconnectHandler`
logic passed to `mockPeripheral.on('disconnect', ...)`. Update the
`bleDevice.spec.ts` constructor test to capture that handler and exercise all
branches in `reconnectHandler`: a successful `connectAsync()` reconnect, the
already `'connected'` early-return path, and the error path that triggers
`this.close()`. Use the existing `createDevice`, `mockPeripheral`, and
`connectAsync`/`close` mocks to assert the expected behavior.
In `@tests/unit/device/protocol/airotic/airoticProtocol.spec.ts`:
- Line 2: The Airotic protocol module name is misspelled and inconsistent with
the rest of the Airotic symbols, so rename the protocol file/module from the
current airtonic spelling to the airotic convention and update the import in
AiroticProtocol spec accordingly. Make sure the exported default referenced by
AiroticProtocol and any related module paths under the airotic folder all use
the same airotic naming so the test and source imports stay aligned.
In `@tests/unit/device/protocol/slvCtrlPlus/genericSlvCtrlPlusDevice.spec.ts`:
- Around line 188-339: The three new refresh tests duplicate the same
GenericSlvCtrlPlusDevice setup, so refactor them to reuse the existing
createDevice helper instead of repeating the 12-argument constructor. Update
createDevice to accept the attrs override and optionally return or accept the
Logger mock if needed, then use it in the bool/str, empty-string, and
unknown-attribute tests to keep only the per-test protocol/transport stubbing
inline.
In `@tests/unit/device/protocol/zc95/zc95Device.spec.ts`:
- Around line 685-714: The test case in processPowerStatusMessage is not
actually exercising the clamp behavior because the final assignment to
MaxOutputPower overwrites the earlier clamped value, so the over-limit setup
does not prove a distinct code path. Update the spec around getOnReceiveCallback
and device.getAttribute('powerChannel1') to either use a scenario that reaches
the clamp logic without being overwritten, or simplify the assertion to match
the current behavior and avoid implying clamp-specific coverage.
In `@tests/unit/device/transport/bleObserver.spec.ts`:
- Around line 130-193: Add dedicated tests for BleObserver.stop() in
bleObserver.spec.ts using the existing createObserver and noble mocks. Cover the
shutdown path where stop() is called while scanning: verify it removes the noble
listeners, awaits stopScanningAsync, and calls noble.stop(). Also add a no-op
case when scanning is already inactive so stop() does not try to stop scanning
again. Keep the assertions centered on the BleObserver.stop method and the
relevant noble/mockLogger interactions.
In `@tests/unit/device/transport/serialPortObserver.spec.ts`:
- Around line 118-127: The test for SerialPortObserver deduplication is only
checking the initial scan and never triggers a second discovery pass, so it does
not verify the “already managed” behavior. Update the spec to invoke
SerialPortObserver.discoverSerialDevices() again after the first start/scan,
using the existing createObserver and mockDeviceManager.announceDetectedDevice
setup, so the assertion truly validates that a previously managed device is not
re-announced on a subsequent discovery run.
🪄 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: 8b032485-44a5-4302-a008-af0368800890
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (90)
.gitignoreeslint.config.tspackage.jsonsrc/app.tssrc/automation/scriptRuntime.tssrc/controller/automation/deleteScriptController.tssrc/controller/automation/getScriptController.tssrc/controller/automation/runScriptController.tssrc/controller/getDeviceController.tssrc/controller/patchDeviceController.tssrc/device/attribute/intRangeDeviceAttribute.tssrc/device/attribute/strDeviceAttribute.tssrc/device/bleDevice.tssrc/device/device.tssrc/device/deviceId.tssrc/device/deviceManager.tssrc/device/peripheralDevice.tssrc/device/protocol/airotic/airoticDevice.tssrc/device/protocol/airotic/airoticDeviceProvider.tssrc/device/protocol/buttplugIo/buttplugIoDevice.tssrc/device/protocol/buttplugIo/buttplugIoDeviceFactory.tssrc/device/protocol/buttplugIo/buttplugIoWebsocketDeviceProvider.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/virtualDevice.tssrc/device/protocol/virtual/virtualDeviceLogic.tssrc/device/protocol/virtual/virtualDeviceProvider.tssrc/device/protocol/zc95/zc95Device.tssrc/device/protocol/zc95/zc95DeviceFactory.tssrc/device/protocol/zc95/zc95SerialDeviceProvider.tssrc/device/provider/bleDeviceProvider.tssrc/device/provider/deviceProvider.tssrc/device/provider/deviceProviderManager.tssrc/device/provider/serialDeviceProvider.tssrc/device/transport/bleDeviceTransport.tssrc/device/transport/bleObserver.tssrc/device/transport/deviceBidirectionalTransport.tssrc/device/transport/deviceReadableTransport.tssrc/device/transport/deviceWritableTransport.tssrc/device/transport/serialDeviceTransport.tssrc/device/transport/serialPortObserver.tssrc/device/types.d.tssrc/device/webSocketEvent.tssrc/index.tssrc/repository/connectedDeviceRepository.tssrc/serial/synchronousSerialPort.tssrc/serialization/classToPlainSerializer.tssrc/serviceMap.tssrc/serviceProvider/deviceServiceProvider.tssrc/serviceProvider/settingsServiceProvider.tssrc/socket/types.tssrc/types.d.tssrc/util/async.tssrc/util/color.tssrc/util/expressUtils.tssrc/util/objects.tstests/integration/automationScripts.spec.tstests/integration/deviceEvents.spec.tstests/integration/devices/airoticDevice.spec.tstests/integration/helpers/airoticDeviceSimulator.tstests/integration/helpers/appHelper.tstests/unit/automation/scriptRuntime.spec.tstests/unit/device/bleDevice.spec.tstests/unit/device/deviceManager.spec.tstests/unit/device/protocol/airotic/airoticDevice.spec.tstests/unit/device/protocol/airotic/airoticProtocol.spec.tstests/unit/device/protocol/buttplugIo/buttplugIoDevice.spec.tstests/unit/device/protocol/estim2b/estim2bDevice.spec.tstests/unit/device/protocol/estim2b/estim2bProtocol.spec.tstests/unit/device/protocol/slvCtrlPlus/genericSlvCtrlPlusDevice.spec.tstests/unit/device/protocol/slvCtrlPlus/slvCtrlProtocolLegacy.spec.tstests/unit/device/protocol/slvCtrlPlus/slvCtrlProtocolV1.spec.tstests/unit/device/protocol/virtual/display/displayVirtualDevice.spec.tstests/unit/device/protocol/zc95/zc95Device.spec.tstests/unit/device/protocol/zc95/zc95Protocol.spec.tstests/unit/device/testDevice.tstests/unit/device/testDeviceProvider.tstests/unit/device/transport/bleObserver.spec.tstests/unit/device/transport/serialPortObserver.spec.tstsconfig.jsonvitest.config.integration.tsvitest.config.tsvitest.config.unit.ts
✅ Files skipped from review due to trivial changes (6)
- .gitignore
- vitest.config.unit.ts
- src/device/attribute/intRangeDeviceAttribute.ts
- src/util/objects.ts
- src/device/transport/deviceWritableTransport.ts
- tsconfig.json
🚧 Files skipped from review as they are similar to previous changes (31)
- src/device/provider/deviceProvider.ts
- src/serial/synchronousSerialPort.ts
- src/serviceProvider/settingsServiceProvider.ts
- src/util/expressUtils.ts
- src/device/deviceId.ts
- eslint.config.ts
- src/device/protocol/buttplugIo/buttplugIoWebsocketDeviceProvider.ts
- src/controller/getDeviceController.ts
- package.json
- src/controller/automation/runScriptController.ts
- src/device/protocol/zc95/zc95DeviceFactory.ts
- src/serviceMap.ts
- src/device/protocol/estim2b/estim2bDeviceFactory.ts
- src/controller/automation/getScriptController.ts
- src/controller/automation/deleteScriptController.ts
- src/device/protocol/virtual/virtualDevice.ts
- src/device/protocol/slvCtrlPlus/genericSlvCtrlPlusDevice.ts
- src/device/transport/bleObserver.ts
- tests/unit/device/deviceManager.spec.ts
- src/device/transport/serialPortObserver.ts
- src/device/protocol/buttplugIo/buttplugIoDeviceFactory.ts
- src/device/protocol/estim2b/estim2bSerialDeviceProvider.ts
- src/device/protocol/slvCtrlPlus/slvCtrlPlusDeviceFactory.ts
- src/device/protocol/zc95/zc95SerialDeviceProvider.ts
- src/serviceProvider/deviceServiceProvider.ts
- src/device/protocol/messageResponseHandler.ts
- src/device/protocol/slvCtrlPlus/slvCtrlPlusSerialDeviceProvider.ts
- src/device/provider/serialDeviceProvider.ts
- src/controller/patchDeviceController.ts
- tests/unit/device/protocol/buttplugIo/buttplugIoDevice.spec.ts
- src/device/protocol/zc95/zc95Device.ts
🛑 Comments failed to post (6)
src/automation/scriptRuntime.ts (3)
24-47: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Stale JSDoc:
event.*API no longer matches handler signature.Lines 40-44 document
event.type,event.device.getDeviceId, etc., but the actualonEventhandler signature (per the updated line 39 and the__dispatchEventimplementation at lines 166-183) ishandler(device, ...args)— handlers receivedevicedirectly, not aneventwrapper object. This is leftover documentation from a prior API design.📝 Proposed doc fix
- * - onEvent(eventName, handler) – register handler for a specific event: (device, ...args) => void - * - event.type – string – current event type - * - event.device.getDeviceId – string - * - event.device.getDeviceName – string - * - event.device.getAttribute(name) – Promise<{ value, name, label, modifier, type } | undefined> - * - event.device.setAttribute(name, v) – Promise<void> + * - onEvent(eventName, handler) – register handler for a specific event: (device, ...args) => void + * - device.getDeviceId – string + * - device.getDeviceName – string + * - device.getAttribute(name) – Promise<{ value, name, label, modifier, type } | undefined> + * - device.setAttribute(name, v) – Promise<void>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements./** * Bootstrap code injected into the isolate context once on load, before the user script runs. * * Globals prefixed with `__` are set from the host via `jail.set()` before this runs: * - __log Reference – host console sink * - __getAttribute Reference – async: (deviceId, attrName) => JSON string|null * - __setAttribute Reference – async: (deviceId, attrName, value) => void * - __getDeviceJson Reference – sync: (deviceId) => JSON string|null * - __getDevicesJson Reference – sync: () => JSON string of [{id, name}] * - __lifecycleDone Callback – signals onStart/onStop completion (null = ok, string = error) * * Script-facing API: * - console.log(...) * - onStart(handler) – run once when script loads: () => void | Promise<void> * - onStop(handler) – run once when script stops: () => void | Promise<void> * - onEvent(eventName, handler) – register handler for a specific event: (device, ...args) => void * - device.getDeviceId – string * - device.getDeviceName – string * - device.getAttribute(name) – Promise<{ value, name, label, modifier, type } | undefined> * - device.setAttribute(name, v) – Promise<void> * - devices.getById(id) – Device | null * - devices.getAll() – Device[] */🤖 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 24 - 47, Update the bootstrap JSDoc in scriptRuntime.ts to match the current event API: the onEvent(handler) contract and __dispatchEvent implementation use handler(device, ...args), not an event wrapper object. Remove or rewrite the stale event.* bullets (event.type, event.device.getDeviceId, event.device.getAttribute, etc.) and document the actual device argument and payload shape using the onEvent and __dispatchEvent symbols as the source of truth.
101-115: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail # Locate the relevant file and nearby symbols git ls-files 'src/automation/scriptRuntime.ts' 'src/automation/*' | sed -n '1,20p' echo '--- outline ---' ast-grep outline src/automation/scriptRuntime.ts --view expanded || true echo '--- relevant ranges ---' nl -ba src/automation/scriptRuntime.ts | sed -n '1,340p'Repository: SlvCtrlPlus/slvctrlplus-server
Length of output: 2040
🏁 Script executed:
#!/bin/bash set -euo pipefail # Search for the callback and attribute-write paths referenced in the review comment rg -n "__setAttribute|setAttribute\\(|__done|logError|getAttribute|applySync|event dispatch" src/automation/scriptRuntime.ts src/automation -SRepository: SlvCtrlPlus/slvctrlplus-server
Length of output: 6528
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the full file with line numbers and search for the write path nl -ba src/automation/scriptRuntime.ts | sed -n '1,340p' echo '--- search ---' rg -n "__setAttribute|setAttribute\\(|__done|logError|getAttribute|applySync|event dispatch" src/automation/scriptRuntime.ts -SRepository: SlvCtrlPlus/slvctrlplus-server
Length of output: 208
🏁 Script executed:
#!/bin/bash set -euo pipefail sed -n '20,140p' src/automation/scriptRuntime.ts printf '\n---\n' sed -n '240,285p' src/automation/scriptRuntime.tsRepository: SlvCtrlPlus/slvctrlplus-server
Length of output: 7145
setAttributeneeds to await the host write Insrc/automation/scriptRuntime.ts:101-115, 260-264, the proxy returnsPromise.resolve()immediately while the host side only logsdev.setAttribute(...).catch(...), soawait device.setAttribute(...)finishes before the write completes and cannot surface failures. Return the host promise (or reject on error) so scripts can react to a failed write.🤖 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 101 - 115, The Device proxy’s setAttribute method resolves immediately instead of reflecting the host write, so await on it can finish before the underlying update completes. Update __createDeviceProxy’s setAttribute to return the actual promise from __setAttribute/applySync path (or wrap it so failures reject), and make sure the caller in the host-side device handling around dev.setAttribute propagates the async result instead of only logging .catch(). This will let scripts wait for the write and handle errors from setAttribute properly.
424-439: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
processQueueemits a different error string than what's logged.
msg(derived viae instanceof Error ? e.message : String(e)) is used for the logger and the log file, but theconsoleLogevent emitted to listeners usesString(e)— forErrorinstances this typically yields"Error: <message>"instead of the plain<message>. Listeners/UI consumingconsoleLogwill see inconsistent formatting compared to the automation log file.🩹 Proposed fix
} catch (e: unknown) { const msg = e instanceof Error ? e.message : String(e); this.logger.error(`VM error: ${msg}`); this.log(msg); - this.eventEmitter.emit(AutomationEventType.consoleLog, String(e)); + this.eventEmitter.emit(AutomationEventType.consoleLog, msg); }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.private async processQueue(): Promise<void> { while (this.eventQueue.length > 0) { const task = this.eventQueue.shift()!; try { await task(); } catch (e: unknown) { const msg = e instanceof Error ? e.message : String(e); this.logger.error(`VM error: ${msg}`); this.log(msg); this.eventEmitter.emit(AutomationEventType.consoleLog, msg); } } this.processQueuePromise = 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/automation/scriptRuntime.ts` around lines 424 - 439, The processQueue error handling in scriptRuntime uses different formatting for the same exception between logging and console emission. Update the catch block in processQueue so the consoleLog event uses the same normalized msg value already derived from e (via Error.message or String(e)) instead of String(e), keeping AutomationEventType.consoleLog consistent with logger.error and this.log output.src/device/deviceManager.ts (1)
167-175: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Ensure reset always drains devices and acquire queues.
If one
device.close()rejects, Line 170 abortsreset()before closing later devices and before resolving pending acquisition queues.Proposed fix
public async reset(): Promise<void> { + let closeError: unknown; + for (const [, device] of this.connectedDevices) { - await device.close(); + try { + await device.close(); + } catch (e: unknown) { + logError(this.logger, `device: ${device.getDeviceId} -> close during reset -> failed`, e); + if (undefined === closeError) { + closeError = e; + } + } } for (const [deviceId] of this.detectedDeviceAcquireQueue) { this.clearDetectedDeviceAcquireQueue(deviceId, 'Device manager reset'); } + + if (undefined !== closeError) { + throw closeError; + } }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.public async reset(): Promise<void> { let closeError: unknown; for (const [, device] of this.connectedDevices) { try { await device.close(); } catch (e: unknown) { logError(this.logger, `device: ${device.getDeviceId} -> close during reset -> failed`, e); if (undefined === closeError) { closeError = e; } } } for (const [deviceId] of this.detectedDeviceAcquireQueue) { this.clearDetectedDeviceAcquireQueue(deviceId, 'Device manager reset'); } if (undefined !== closeError) { throw closeError; } }🤖 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 167 - 175, The reset() method in DeviceManager stops early if any device.close() rejects, leaving later connected devices and detectedDeviceAcquireQueue entries uncleared. Update reset() so it always attempts to close every device and drain every pending acquire queue entry, using the existing connectedDevices and clearDetectedDeviceAcquireQueue paths, and make sure one close failure does not prevent the rest of the cleanup from running.src/device/protocol/virtual/virtualDeviceProvider.ts (1)
42-57: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Prevent in-flight discovery from adding devices after
stop().
clearIntervalonly stops future ticks. A discovery already awaitingdeviceFactory.create()can still resume afterstop()and calldeviceManager.addDevice(), leaving a virtual device connected after shutdown.🛡️ Proposed lifecycle guard
private discoveryInterval?: NodeJS.Timeout; + private stopped = true; public override async init(): Promise<void> { + this.stopped = false; this.discoveryInterval ??= setImmediateInterval(asyncHandler( this.discoverVirtualDevices.bind(this), (e: unknown) => this.logger.error('Error while scanning for new virtual devices', e) ), this.scanIntervalMs); } public override async stop(): Promise<void> { + this.stopped = true; if (this.discoveryInterval !== undefined) { clearInterval(this.discoveryInterval); this.discoveryInterval = undefined; } @@ private async discoverVirtualDevices(): Promise<void> { + if (this.stopped) { + return; + } + const settings = this.settingsManager.getSettings(); @@ for (const [k, v] of virtualDevices) { + if (this.stopped) { + return; + } + if (this.attemptedDevices.has(k) || this.connectedDevices.has(k)) { continue; } @@ try { const device = await this.deviceFactory.create(knowDevice, VirtualDeviceProvider.providerName); + if (this.stopped) { + await device.close(); + return; + } this.deviceManager.addDevice(device); this.connectedDevices.set(knowDevice.id, device);Also applies to: 89-97
🤖 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 42 - 57, The virtual device discovery flow in virtualDeviceProvider’s init/discoverVirtualDevices path can still add devices after stop() because an in-flight async scan is not guarded. Add a shutdown/lifecycle check in discoverVirtualDevices and around the deviceFactory.create/deviceManager.addDevice sequence so any scan that resumes after stop() exits without registering devices, and make stop() set that guard before clearing discoveryInterval and removing connected devices.src/device/provider/bleDeviceProvider.ts (1)
77-89: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail # Map relevant files and locate the referenced symbols. git ls-files 'src/device/provider/*' 'src/device/*' | sed -n '1,200p' echo '---' rg -n "disconnectPeripheral|doClose|cancelConnect|connectBleDevice|create\\(" src/device -SRepository: SlvCtrlPlus/slvctrlplus-server
Length of output: 13779
🏁 Script executed:
#!/bin/bash set -euo pipefail # Read the relevant file sections with line numbers. for f in src/device/provider/bleDeviceProvider.ts src/device/provider/bleDeviceProvider.ts src/device/device.ts; do if [ -f "$f" ]; then echo "### $f" wc -l "$f" echo '---' cat -n "$f" | sed -n '1,220p' echo fi doneRepository: SlvCtrlPlus/slvctrlplus-server
Length of output: 17295
🏁 Script executed:
#!/bin/bash set -euo pipefail # If the device/provider files are elsewhere, discover and inspect them. fd -a '.*(device|provider).*\.ts$' src | sed -n '1,200p'Repository: SlvCtrlPlus/slvctrlplus-server
Length of output: 5961
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect only the referenced symbols and surrounding logic. rg -n "class BleDevice|doClose\\(|cancelConnect\\(|disconnectPeripheral\\(|connectBleDevice\\(|BleUartDeviceTransport\\.create\\(|promiseWithTimeout\\(" src -S echo '--- bleDeviceProvider.ts ---' sed -n '1,180p' src/device/provider/bleDeviceProvider.ts echo '--- related device file(s) ---' fd -a 'bleDevice.*\.ts|*Device*.ts' src/device | sed -n '1,120p'Repository: SlvCtrlPlus/slvctrlplus-server
Length of output: 6562
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect BLE close/connect handling and the Airotic connection path. sed -n '1,150p' src/device/bleDevice.ts echo '---' sed -n '1,120p' src/device/protocol/airotic/airoticDeviceProvider.ts echo '---' sed -n '1,120p' src/device/transport/bleDeviceTransport.tsRepository: SlvCtrlPlus/slvctrlplus-server
Length of output: 15279
Handle
connectinghere too. IfconnectBleDevice()times out whileperipheral.connectAsync()is still in flight, this cleanup path leaves the peripheral stuck inconnecting; mirrorBleDevice.doClose()and callcancelConnect()here as well.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/device/provider/bleDeviceProvider.ts` around lines 77 - 89, The disconnectPeripheral cleanup only handles peripherals in connected state, so a timed-out connect can leave the peripheral stuck in connecting. Update disconnectPeripheral in bleDeviceProvider to mirror BleDevice.doClose by handling both connected and connecting states, and when the peripheral is connecting call cancelConnect() before/alongside disconnectAsync. Keep the existing timeout and logError handling intact.
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)
43-70: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winPreserve useful output for logged
Errorobjects.
JSON.stringify(new Error('x'))usually returns{}, soconsole.error(err)from scripts loses the message/stack. HandleErrorbefore the object-stringify path.Proposed fix
function __formatLogArg(arg) { if (typeof arg === 'string') { return arg; } + if (arg instanceof Error) { + return arg.stack || `${arg.name}: ${arg.message}`; + } + if (typeof arg === 'function') { return arg.toString(); }🤖 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 43 - 70, The __formatLogArg helper in scriptRuntime is serializing Error objects through the generic object path, which drops message and stack information. Update __formatLogArg to detect Error instances before the object/JSON.stringify branch and return a useful string representation that preserves the error’s message and stack, while keeping the existing handling for strings, functions, primitives, and circular objects.
🧹 Nitpick comments (1)
src/device/deviceManager.ts (1)
144-146: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
DeviceIdfor the connected-device lookup too.
acquireDetectedDevice,releaseDetectedDevice, andclaimDetectedDevicenow takeDeviceId; keeping this public lookup asstringleaves one path outside the typed-ID contract.Proposed fix
- public getConnectedDevice(deviceId: string): Device|null + public getConnectedDevice(deviceId: DeviceId): Device|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/deviceManager.ts` around lines 144 - 146, `getConnectedDevice` still accepts a plain `string`, which leaves the connected-device lookup outside the new typed-ID flow used by `acquireDetectedDevice`, `releaseDetectedDevice`, and `claimDetectedDevice`. Update the `DeviceManager` method signature and any matching call sites or map lookups to use `DeviceId` consistently, so the public API follows the same typed identifier contract as the rest of the detected-device methods.
🤖 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 43-70: The __formatLogArg helper in scriptRuntime is serializing
Error objects through the generic object path, which drops message and stack
information. Update __formatLogArg to detect Error instances before the
object/JSON.stringify branch and return a useful string representation that
preserves the error’s message and stack, while keeping the existing handling for
strings, functions, primitives, and circular objects.
---
Nitpick comments:
In `@src/device/deviceManager.ts`:
- Around line 144-146: `getConnectedDevice` still accepts a plain `string`,
which leaves the connected-device lookup outside the new typed-ID flow used by
`acquireDetectedDevice`, `releaseDetectedDevice`, and `claimDetectedDevice`.
Update the `DeviceManager` method signature and any matching call sites or map
lookups to use `DeviceId` consistently, so the public API follows the same typed
identifier contract as the rest of the detected-device methods.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 183a4889-dadd-4513-8f8c-fdbb1cfde977
📒 Files selected for processing (4)
src/automation/scriptRuntime.tssrc/device/deviceManager.tssrc/device/protocol/virtual/virtualDeviceProvider.tssrc/device/provider/bleDeviceProvider.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/device/provider/bleDeviceProvider.ts
This was referenced Jul 16, 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.
Summary by CodeRabbit
New Features
Bug Fixes / Improvements
Chores