Unite attributes values & enable strict mode#54
Conversation
📝 WalkthroughWalkthroughReplaces legacy GenericDeviceAttribute arrays with a typed DeviceAttribute map model, adds branded numeric types and utilities, converts device updater APIs and callers to async/Promise, and updates protocol implementations, virtual devices, factories, serializers, tests, and tooling to the new attribute-first design. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant HTTP as "HTTP Server"
participant Controller as "PatchDeviceController"
participant Updater as "GenericDeviceUpdater"
participant Device
participant Transport as "DeviceTransport"
Client->>HTTP: PATCH /device/:id (payload)
HTTP->>Controller: invoke execute(req,res)
activate Controller
Controller->>Updater: await update(device, rawData)
activate Updater
loop per attribute
Updater->>Device: await getAttribute(key)
Device-->>Updater: attribute or undefined
alt attribute exists & writable
Updater->>Device: await setAttribute(key, parsedValue)
Device->>Transport: send protocol command (if applicable)
Transport-->>Device: ack/response
Device-->>Updater: updated attribute.value
else
Updater-->>Updater: log skip or error
end
end
Updater-->>Controller: resolve
deactivate Updater
Controller-->>HTTP: send 200 OK
deactivate Controller
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code graph analysis (3)src/device/protocol/virtual/virtualDeviceLogic.ts (2)
src/device/protocol/virtual/genericVirtualDeviceFactory.ts (4)
src/device/protocol/virtual/audio/ttsVirtualDeviceLogic.ts (7)
🔇 Additional comments (6)
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 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/device/protocol/buttplugIo/buttplugIoDevice.ts (1)
46-56:sendValueis undefined forFloatGenericDeviceAttribute.The
ButtplugIoDeviceAttributestype includesFloatGenericDeviceAttribute, but thesendValuecalculation only handlesRangeGenericDeviceAttributeandBoolGenericDeviceAttribute. For Float attributes,sendValuewill beundefined, causing incorrect behavior when callingthis.send().if (attrDef instanceof RangeGenericDeviceAttribute) { sendValue = Number(value)/attrDef.max; } else if (attrDef instanceof BoolGenericDeviceAttribute) { sendValue = value ? 1 : 0; + } else if (attrDef instanceof FloatGenericDeviceAttribute) { + sendValue = Number(value); }
🧹 Nitpick comments (15)
src/device/genericDeviceUpdater.ts (1)
27-29: Consider using try/catch for consistency with async/await.Mixing
awaitwith.then()/.catch()works but is less idiomatic. For consistency with the async function style, consider using try/catch instead.🔎 Alternative implementation using try/catch
- await device.setAttribute(attrKey, attrStr) - .then(() => this.logger.info(`${deviceLogMsg} -> done`)) - .catch((e: Error) => this.logger.error(`${deviceLogMsg} -> failed: ${e.message}`, e)) + try { + await device.setAttribute(attrKey, attrStr); + this.logger.info(`${deviceLogMsg} -> done`); + } catch (e) { + this.logger.error(`${deviceLogMsg} -> failed: ${e.message}`, e); + }src/device/attribute/intGenericDeviceAttribute.ts (1)
9-11: Consider validating the numeric conversion.The
Number(value)conversion can returnNaNfor invalid inputs, which might propagate through the system unexpectedly. Consider adding validation to ensure valid integer values.🔎 Proposed validation
public fromString(value: string): number { - return Number(value); + const num = Number(value); + if (isNaN(num)) { + throw new Error(`Invalid integer value: ${value}`); + } + return num; }src/device/attribute/floatGenericDeviceAttribute.ts (1)
5-7: Consider validating the numeric conversion.Similar to
IntGenericDeviceAttribute, theNumber(value)conversion can returnNaNfor invalid inputs. Consider adding validation to ensure valid float values.🔎 Proposed validation
public fromString(value: string): number { - return Number(value); + const num = Number(value); + if (isNaN(num)) { + throw new Error(`Invalid float value: ${value}`); + } + return num; }src/device/protocol/virtual/delegatedVirtualDeviceFactory.ts (1)
9-21: Remove unnecessary Promise wrapper.The
factory.create(knownDevice, provider)already returns aPromise<VirtualDevice>, so wrapping it in another Promise constructor is redundant. Simply return the promise directly.🔎 Simplified implementation
public async create(knownDevice: KnownDevice, provider: string): Promise<VirtualDevice> { - return new Promise<VirtualDevice>((resolve) => { - const factoryName = `${DelegatedVirtualDeviceFactory.capitalizeFirstLetter(knownDevice.type)}VirtualDeviceLogic`; - if (!this.deviceFactories.has(factoryName)) { - throw new Error(`No factory defined for virtual device '${knownDevice.type}'`); - } - - const factory = this.deviceFactories.get(factoryName); - const device = factory.create(knownDevice, provider); - - resolve(device); - }) + const factoryName = `${DelegatedVirtualDeviceFactory.capitalizeFirstLetter(knownDevice.type)}VirtualDeviceLogic`; + if (!this.deviceFactories.has(factoryName)) { + throw new Error(`No factory defined for virtual device '${knownDevice.type}'`); + } + + const factory = this.deviceFactories.get(factoryName); + return factory.create(knownDevice, provider); }src/device/protocol/virtual/genericVirtualDeviceFactory.ts (1)
23-41: Simplify synchronous Promise construction.The entire operation is synchronous, so wrapping it in a Promise constructor is unnecessary. Consider using
Promise.resolve()or returning the device directly if the interface allows.🔎 Simplified implementation
public create(knownDevice: KnownDevice, provider: string): Promise<VirtualDevice> { - return new Promise<VirtualDevice>((resolve) => { - const deviceLogic = new this.ctor(knownDevice.config); - - const device = new VirtualDevice( - "1.0.0", - knownDevice.id, - knownDevice.name, - knownDevice.type, - provider, - this.dateFactory.now(), - knownDevice.config, - deviceLogic - ); - - resolve(device); - }); + const deviceLogic = new this.ctor(knownDevice.config); + + const device = new VirtualDevice( + "1.0.0", + knownDevice.id, + knownDevice.name, + knownDevice.type, + provider, + this.dateFactory.now(), + knownDevice.config, + deviceLogic + ); + + return Promise.resolve(device); }src/device/protocol/virtual/virtualDeviceProvider.ts (1)
70-82: Consider keeping type safety and removing redundant rethrow.Two observations:
Line 70: Removing the explicit
as VirtualDevicecast could cause type mismatches sinceconnectedDevicesis declared asMap<string, VirtualDevice>(line 15). If the factory returns aDevicebut not specifically aVirtualDevice, TypeScript should flag this.Lines 81-82: Rethrowing the error after logging is redundant in an async function. The error is already logged and will naturally propagate as a rejected promise without the explicit
throw.🔎 Proposed refinement
- const device = await this.deviceFactory.create(knowDevice, VirtualDeviceProvider.name); + const device = await this.deviceFactory.create(knowDevice, VirtualDeviceProvider.name) as VirtualDevice; const deviceStatusUpdaterInterval = this.initDeviceStatusUpdater(device); this.connectedDevices.set(knowDevice.id, device); @@ -78,8 +78,6 @@ this.logger.info('Connected virtual devices: ' + this.connectedDevices.size.toString()); } catch (e: unknown) { this.logger.error(`Could not initiate virtual device '${knowDevice.id}': ${(e as Error).message}`, e); - - throw e; } }src/device/attribute/listGenericDeviceAttribute.ts (1)
8-11: Consider documenting type assertion assumptions.The
fromStringmethod parses integers when possible and casts the result toK. While this works whenKincludes both string and number (union type), the type assertionas Kassumes the caller has validated that the parsed type matchesK.This is acceptable given the constraint
K extends string|number, but consider adding a comment explaining this assumption or validating against the actual K type if possible.💡 Optional: Add clarifying comment
public fromString(value: string): K { + // Parse as int if possible; K is constrained to string|number so cast is safe const parsedInt = parseInt(value, 10); return (isNaN(parsedInt) ? value : parsedInt) as K; }src/device/protocol/virtual/randomGenerator/randomGeneratorVirtualDeviceLogic.ts (1)
36-44: Consider initializing the attribute value.The
valueAttris created without an initialvalue. SincerefreshDatasets it immediately on the first refresh cycle, this is likely acceptable, but for consistency and to avoid potential undefined reads before the first refresh, consider initializing it:const valueAttr = new IntGenericDeviceAttribute(); valueAttr.name = 'value'; valueAttr.modifier = GenericDeviceAttributeModifier.readOnly; + valueAttr.value = 0;src/device/protocol/virtual/virtualDevice.ts (1)
47-57: Consider simplifying with async/await instead of Promise constructor.Wrapping synchronous operations in a
new Promise()constructor is an anti-pattern. Since the operations are synchronous, you can simplify:- public setAttribute<K extends keyof T>(attributeName: K, value: T[K]['value']): Promise<T[K]['value']> { - return new Promise<T[K]['value']>((resolve) => { - this.state = DeviceState.busy; - - this.attributes[attributeName].value = value; - - this.state = DeviceState.ready; - - resolve(value); - }); + public async setAttribute<K extends keyof T>(attributeName: K, value: T[K]['value']): Promise<T[K]['value']> { + this.state = DeviceState.busy; + this.attributes[attributeName].value = value; + this.state = DeviceState.ready; + return value; }src/device/protocol/slvCtrlPlus/genericSlvCtrlPlusDevice.ts (1)
74-76: Reassigningvalueparameter creates type confusion.Reassigning the
valueparameter frombooleantonumber(0 or 1) can cause type confusion since the original type isDeviceAttributes[K]['value']. Consider using a separate variable:+ let sendValue: string | number | boolean = value; if (value === true || value === false) { - value = value ? 1 : 0; + sendValue = value ? 1 : 0; } - const result = await this.send(`set-${attributeName.toString()} ${value}`); + const result = await this.send(`set-${attributeName.toString()} ${sendValue}`);src/device/protocol/buttplugIo/buttplugIoDevice.ts (1)
58-58: Unnecessary template literal.The template literal wrapping
attributeNameis redundant since it's already a string key:- this.attributes[`${attributeName}`].value = value; + this.attributes[attributeName].value = value;src/device/protocol/virtual/audio/ttsVirtualDeviceLogic.ts (2)
32-34: Consider using static attribute name constants consistently.The class defines static readonly constants for attribute names (lines 18-21), but
getAttributecalls use string literals. For consistency and refactoring safety, use the constants:- const text = (await device.getAttribute('text')).value; - const queuing = (await device.getAttribute('queuing')).value; - const speaking = (await device.getAttribute('speaking')).value; + const text = (await device.getAttribute(TtsVirtualDeviceLogic.textAttrName)).value; + const queuing = (await device.getAttribute(TtsVirtualDeviceLogic.queuingAttrName)).value; + const speaking = (await device.getAttribute(TtsVirtualDeviceLogic.speakingAttrName)).value;
73-96: Attributes created without initial values.The attributes are instantiated without initial
valueproperties. Consider initializing them to avoid potentialundefinedreads before the first refresh:const textAttr = new StrGenericDeviceAttribute(); textAttr.name = TtsVirtualDeviceLogic.textAttrName; textAttr.modifier = GenericDeviceAttributeModifier.writeOnly; + textAttr.value = null; // or empty string if null not allowed const speakingAttr = new BoolGenericDeviceAttribute(); speakingAttr.name = TtsVirtualDeviceLogic.speakingAttrName; speakingAttr.modifier = GenericDeviceAttributeModifier.readOnly; + speakingAttr.value = false; const queuingAttr = new BoolGenericDeviceAttribute(); queuingAttr.name = TtsVirtualDeviceLogic.queuingAttrName; queuingAttr.modifier = GenericDeviceAttributeModifier.readWrite; + queuingAttr.value = false; const queueLengthAttr = new IntGenericDeviceAttribute(); queueLengthAttr.name = TtsVirtualDeviceLogic.queueLengthAttrName; queueLengthAttr.modifier = GenericDeviceAttributeModifier.readOnly; + queueLengthAttr.value = 0;src/device/protocol/zc95/zc95Device.ts (2)
197-228: Async conversion looks correct.The Promise wrapper and loop logic safely process queued messages. The attribute access has been properly updated to use
this.attributes.Optional: Consider simplifying the Promise wrapper
Since there are no asynchronous operations within the loop, the method could be simplified:
-private processQueuedMessages(): Promise<void> { - return new Promise((resolve) => { - let queueMessagesProcessed = 0; - - while (this.receiveQueue.length > 0 && queueMessagesProcessed <= 10) { +private async processQueuedMessages(): Promise<void> { + let queueMessagesProcessed = 0; + + while (this.receiveQueue.length > 0 && queueMessagesProcessed <= 10) { + const msg = this.receiveQueue.shift(); + ++queueMessagesProcessed; // ... rest of logic ... - } - - resolve(); - }); + } }However, the current implementation is perfectly valid.
230-262: Consider reusing theattrNamevariable.The method correctly builds pattern attributes, but Line 258 reconstructs the attribute key instead of reusing the
attrNamevariable defined on Line 236.Suggested refactor to reduce redundancy
for (const menuItem of menuItems) { const attrName = `${Zc95Device.patternAttributePrefix}${menuItem.Id}`; if (this.isMinMaxMenuItem(menuItem)) { const rangeAttr = new RangeGenericDeviceAttribute(); rangeAttr.name = attrName; // ... rest of range setup ... patternAttributes[attrName as keyof Zc95DevicePatternAttributes] = rangeAttr; } else if (this.isMultiChoiceMenuItem(menuItem)) { const listAttr = new ListGenericDeviceAttribute<number, string>(); listAttr.name = attrName; // ... rest of list setup ... patternAttributes[attrName as keyof Zc95DevicePatternAttributes] = listAttr; } - patternAttributes[`${Zc95Device.patternAttributePrefix}${menuItem.Id}`].value = menuItem.Default; + patternAttributes[attrName as keyof Zc95DevicePatternAttributes].value = menuItem.Default; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (43)
src/controller/patchDeviceController.tssrc/device/attribute/boolGenericDeviceAttribute.tssrc/device/attribute/floatGenericDeviceAttribute.tssrc/device/attribute/genericDeviceAttribute.tssrc/device/attribute/intGenericDeviceAttribute.tssrc/device/attribute/listGenericDeviceAttribute.tssrc/device/attribute/rangeGenericDeviceAttribute.tssrc/device/attribute/strGenericDeviceAttribute.tssrc/device/device.tssrc/device/genericDeviceUpdater.tssrc/device/protocol/buttplugIo/buttplugIoDevice.tssrc/device/protocol/buttplugIo/buttplugIoDeviceFactory.tssrc/device/protocol/slvCtrlPlus/genericSlvCtrlPlusDevice.tssrc/device/protocol/slvCtrlPlus/slvCtrlPlusDevice.tssrc/device/protocol/slvCtrlPlus/slvCtrlPlusMessageParser.tssrc/device/protocol/virtual/audio/ttsVirtualDeviceLogic.tssrc/device/protocol/virtual/delegatedVirtualDeviceFactory.tssrc/device/protocol/virtual/display/displayVirtualDeviceLogic.tssrc/device/protocol/virtual/genericVirtualDeviceFactory.tssrc/device/protocol/virtual/randomGenerator/randomGeneratorVirtualDeviceLogic.tssrc/device/protocol/virtual/virtualDevice.tssrc/device/protocol/virtual/virtualDeviceFactory.tssrc/device/protocol/virtual/virtualDeviceLogic.tssrc/device/protocol/virtual/virtualDeviceProvider.tssrc/device/protocol/zc95/zc95Device.tssrc/device/protocol/zc95/zc95DeviceFactory.tssrc/device/provider/deviceProvider.tssrc/device/updater/abstractDeviceUpdater.tssrc/device/updater/bufferedDeviceUpdater.tssrc/device/updater/delegateDeviceUpdater.tssrc/device/updater/deviceUpdaterInterface.tssrc/index.tssrc/repository/connectedDeviceRepository.tssrc/repository/deviceRepositoryInterface.tssrc/serialization/classToPlainSerializer.tssrc/serviceProvider/deviceServiceProvider.tssrc/socket/deviceUpdateHandler.tstests/unit/controller/getDevicesController.spec.tstests/unit/device/protocol/buttplugIo/buttplugIoDevice.spec.tstests/unit/device/protocol/slvCtrlPlus/genericSlvCtrlPlusDevice.spec.tstests/unit/device/protocol/slvCtrlPlus/slvCtrlPlusMessageParser.spec.tstests/unit/device/protocol/virtual/display/displayVirtualDevice.spec.tstests/unit/device/testDevice.ts
💤 Files with no reviewable changes (1)
- src/device/updater/delegateDeviceUpdater.ts
🧰 Additional context used
🧬 Code graph analysis (26)
src/device/protocol/slvCtrlPlus/slvCtrlPlusDevice.ts (1)
src/device/device.ts (1)
DeviceAttributes(5-5)
src/device/protocol/virtual/delegatedVirtualDeviceFactory.ts (1)
src/device/protocol/virtual/virtualDeviceFactory.ts (1)
VirtualDeviceFactory(4-9)
src/device/updater/abstractDeviceUpdater.ts (1)
src/device/device.ts (1)
DeviceData(7-9)
src/device/updater/deviceUpdaterInterface.ts (1)
src/device/device.ts (1)
DeviceData(7-9)
src/socket/deviceUpdateHandler.ts (1)
src/socket/types.ts (1)
DeviceUpdateData(3-3)
src/controller/patchDeviceController.ts (2)
src/middleware/contentTypeMiddleware.ts (1)
req(3-18)src/device/device.ts (1)
DeviceData(7-9)
src/device/genericDeviceUpdater.ts (1)
src/device/device.ts (1)
DeviceData(7-9)
src/device/attribute/genericDeviceAttribute.ts (2)
src/device/protocol/buttplugIo/buttplugIoDevice.ts (1)
Exclude(10-78)src/device/protocol/slvCtrlPlus/genericSlvCtrlPlusDevice.ts (1)
Exclude(8-100)
tests/unit/device/protocol/slvCtrlPlus/genericSlvCtrlPlusDevice.spec.ts (2)
src/device/device.ts (1)
DeviceAttributes(5-5)src/device/transport/deviceTransport.ts (1)
DeviceTransport(1-26)
src/device/protocol/virtual/randomGenerator/randomGeneratorVirtualDeviceLogic.ts (2)
src/device/attribute/intGenericDeviceAttribute.ts (1)
IntGenericDeviceAttribute(4-12)src/device/protocol/virtual/virtualDeviceLogic.ts (1)
VirtualDeviceLogic(4-11)
tests/unit/device/protocol/buttplugIo/buttplugIoDevice.spec.ts (1)
src/device/protocol/buttplugIo/buttplugIoDevice.ts (1)
ButtplugIoDeviceAttributes(8-8)
src/device/protocol/virtual/audio/ttsVirtualDeviceLogic.ts (4)
src/device/attribute/strGenericDeviceAttribute.ts (1)
StrGenericDeviceAttribute(3-7)src/device/attribute/boolGenericDeviceAttribute.ts (1)
BoolGenericDeviceAttribute(3-8)src/device/attribute/intGenericDeviceAttribute.ts (1)
IntGenericDeviceAttribute(4-12)src/device/protocol/virtual/virtualDeviceLogic.ts (1)
VirtualDeviceLogic(4-11)
src/device/protocol/virtual/virtualDevice.ts (2)
src/device/device.ts (1)
DeviceAttributes(5-5)src/device/protocol/virtual/virtualDeviceLogic.ts (1)
VirtualDeviceLogic(4-11)
src/device/provider/deviceProvider.ts (1)
src/device/device.ts (1)
DeviceAttributes(5-5)
src/device/protocol/buttplugIo/buttplugIoDevice.ts (3)
src/device/attribute/rangeGenericDeviceAttribute.ts (1)
RangeGenericDeviceAttribute(4-20)src/device/attribute/boolGenericDeviceAttribute.ts (1)
BoolGenericDeviceAttribute(3-8)src/device/attribute/floatGenericDeviceAttribute.ts (1)
FloatGenericDeviceAttribute(3-8)
src/device/protocol/slvCtrlPlus/slvCtrlPlusMessageParser.ts (1)
src/device/device.ts (1)
DeviceAttributes(5-5)
src/device/protocol/zc95/zc95DeviceFactory.ts (2)
src/device/protocol/zc95/zc95Device.ts (1)
Zc95DeviceAttributes(32-33)src/device/attribute/listGenericDeviceAttribute.ts (1)
ListGenericDeviceAttribute(4-12)
src/device/protocol/slvCtrlPlus/genericSlvCtrlPlusDevice.ts (1)
src/device/device.ts (1)
DeviceAttributes(5-5)
src/device/protocol/zc95/zc95Device.ts (4)
src/device/attribute/listGenericDeviceAttribute.ts (1)
ListGenericDeviceAttribute(4-12)src/device/attribute/boolGenericDeviceAttribute.ts (1)
BoolGenericDeviceAttribute(3-8)src/device/attribute/rangeGenericDeviceAttribute.ts (1)
RangeGenericDeviceAttribute(4-20)src/device/protocol/zc95/Zc95Messages.ts (2)
MinMaxMenuItem(46-51)MultiChoiceMenuItem(53-55)
src/device/protocol/virtual/virtualDeviceLogic.ts (1)
src/device/device.ts (1)
DeviceAttributes(5-5)
src/device/protocol/buttplugIo/buttplugIoDeviceFactory.ts (2)
src/device/protocol/buttplugIo/buttplugIoDevice.ts (1)
ButtplugIoDeviceAttributes(8-8)src/device/attribute/floatGenericDeviceAttribute.ts (1)
FloatGenericDeviceAttribute(3-8)
src/serviceProvider/deviceServiceProvider.ts (1)
src/device/deviceManager.ts (1)
DeviceManager(7-64)
src/device/updater/bufferedDeviceUpdater.ts (2)
src/device/device.ts (1)
DeviceData(7-9)src/device/updater/deviceUpdaterInterface.ts (1)
DeviceUpdaterInterface(3-6)
src/device/protocol/virtual/display/displayVirtualDeviceLogic.ts (2)
src/device/attribute/strGenericDeviceAttribute.ts (1)
StrGenericDeviceAttribute(3-7)src/device/protocol/virtual/virtualDeviceLogic.ts (1)
VirtualDeviceLogic(4-11)
tests/unit/device/protocol/slvCtrlPlus/slvCtrlPlusMessageParser.spec.ts (6)
src/device/attribute/boolGenericDeviceAttribute.ts (1)
BoolGenericDeviceAttribute(3-8)src/device/attribute/rangeGenericDeviceAttribute.ts (1)
RangeGenericDeviceAttribute(4-20)src/device/attribute/intGenericDeviceAttribute.ts (1)
IntGenericDeviceAttribute(4-12)src/device/attribute/listGenericDeviceAttribute.ts (1)
ListGenericDeviceAttribute(4-12)src/device/attribute/strGenericDeviceAttribute.ts (1)
StrGenericDeviceAttribute(3-7)src/device/attribute/floatGenericDeviceAttribute.ts (1)
FloatGenericDeviceAttribute(3-8)
src/index.ts (1)
src/middleware/contentTypeMiddleware.ts (1)
req(3-18)
🔇 Additional comments (57)
src/serviceProvider/deviceServiceProvider.ts (2)
11-11: LGTM! Import style updated consistently.The change from a named import to a default import for Device is consistent with how other modules like DeviceManager are imported in this file.
61-61: LGTM! Type parameter correctly aligned with DeviceManager signature.The removal of the nested
<DeviceData>generic parameter correctly aligns the instantiation with DeviceManager's constructor signature, which expectsMap<string, Device>without the nested generic. This change is part of the broader refactor to simplify the device attribute type system.src/device/updater/deviceUpdaterInterface.ts (1)
5-5: LGTM! Appropriate async refactor.The change from synchronous to asynchronous (Promise) is appropriate for device update operations that likely involve I/O. The AI summary confirms this aligns with async implementations across the codebase.
src/device/provider/deviceProvider.ts (3)
3-3: LGTM! Consistent with attribute refactor.The import change from
DeviceDatatoDeviceAttributesaligns with the broader refactoring of the device attribute system.
21-26: LGTM! Method chaining enabled.Changing the return type from
DeviceProvidertothisenables fluent method chaining, which is a good design practice for builder-style APIs.
28-28: LGTM! Generic constraint updated.The constraint change from
T extends DeviceDatatoT extends DeviceAttributesis consistent with the attribute refactoring across the codebase.src/device/protocol/zc95/zc95DeviceFactory.ts (1)
65-81: LGTM! Consistent migration to typed attribute objects.The refactor from array-based attributes to a strongly-typed object structure (
Zc95DeviceAttributes) with named properties is well-executed. The use ofListGenericDeviceAttribute<number, string>with proper type parameters ensures type safety for the active pattern attribute.tests/unit/device/protocol/virtual/display/displayVirtualDevice.spec.ts (1)
32-32: LGTM! Test updated for new attribute structure.The assertion now correctly accesses the
.valueproperty of the attribute object returned bygetAttribute(), which aligns with the refactored attribute model where attributes are objects containing avaluefield.tests/unit/controller/getDevicesController.spec.ts (1)
21-21: LGTM! Test updated for object-based attributes.The constructor argument change from
[]to{}correctly reflects the migration from array-based to object-based device attributes.tests/unit/device/testDevice.ts (1)
11-11: LGTM! Test helper updated for new attribute model.The constructor call correctly changes the attributes parameter from an empty array
[]to an empty object{}, consistent with the migration to object-based device attributes.src/device/protocol/virtual/display/displayVirtualDeviceLogic.ts (3)
5-7: LGTM! Well-defined attribute type.The
DisplayVirtualDeviceAttributestype provides clear, strongly-typed structure for the display device's attributes.
9-9: LGTM! Generic type parameter added.Implementing
VirtualDeviceLogic<DisplayVirtualDeviceAttributes>ensures type safety across the virtual device logic interface.
20-28: LGTM! Consistent migration to typed attribute objects.The method now returns a strongly-typed object structure instead of an array, with the return value matching the
DisplayVirtualDeviceAttributestype. This provides better type safety and clearer attribute access patterns.src/index.ts (1)
104-107: LGTM! Proper async error handling.Wrapping the PATCH route with
asyncHandleris the correct approach now that the controller'sexecutemethod is asynchronous. This ensures any rejected promises are properly caught and passed to Express's error handling middleware.tests/unit/device/protocol/slvCtrlPlus/genericSlvCtrlPlusDevice.spec.ts (1)
1-37: LGTM! Well-structured test for attribute validation.The test correctly verifies that attempting to set a non-existent attribute results in a rejected promise with the appropriate error message. The test follows AAA pattern and properly uses mocking for the transport layer.
src/device/updater/abstractDeviceUpdater.ts (1)
14-14: LGTM! Async update contract established.Changing the abstract
updatemethod to returnPromise<void>properly establishes an asynchronous contract for all device updater implementations. This aligns with the broader refactor making device operations asynchronous.src/device/protocol/virtual/virtualDeviceFactory.ts (1)
2-6: LGTM! Improved type specificity.Refining the return type from
Promise<Device>toPromise<VirtualDevice>provides better type safety and more accurate interface documentation. This aligns with the generic attribute refactor across virtual device implementations.src/device/attribute/boolGenericDeviceAttribute.ts (1)
3-7: LGTM! Type-safe boolean attribute specialization.Extending
GenericDeviceAttribute<boolean>and narrowing thefromStringreturn type tobooleanprovides better type safety and aligns with the generic attribute model introduced across the codebase.src/controller/patchDeviceController.ts (2)
19-19: LGTM! Controller properly made async.The method signature correctly updated to
asyncand returnsPromise<void>, aligning with the asynchronous device updater contract.
30-30: LGTM! Update call properly awaited.The
deviceUpdater.updatecall is now correctly awaited, ensuring the asynchronous update operation completes before proceeding. This aligns with the newPromise<void>contract of the updater interface.src/device/attribute/strGenericDeviceAttribute.ts (1)
3-7: LGTM! Type-safe string attribute specialization.Extending
GenericDeviceAttribute<string>and implementingfromStringas an identity function provides proper type safety while maintaining consistency with other attribute type specializations in the codebase.src/socket/deviceUpdateHandler.ts (2)
24-24: LGTM! Handler properly made async.The method signature correctly updated to
asyncand returnsPromise<void>, enabling proper awaiting of the asynchronous device update operation.
33-33: LGTM! Update operation properly awaited.The
deviceUpdater.updatecall is now correctly awaited, ensuring the asynchronous update completes before the handler returns. Error handling is preserved and will properly catch any rejected promises.src/repository/deviceRepositoryInterface.ts (1)
1-8: LGTM!The type simplification from
Device<DeviceData>toDevicealigns well with the broader refactor to a generic attributes-based model. The changes are consistent across both methods.src/repository/connectedDeviceRepository.ts (1)
1-21: LGTM!The type updates correctly implement the interface changes from
DeviceRepositoryInterfaceand maintain consistency with the attribute model refactor.src/device/protocol/virtual/virtualDeviceLogic.ts (1)
4-8: All VirtualDeviceLogic implementations have been updated correctly.Verified that the three implementations (RandomGeneratorVirtualDeviceLogic, TtsVirtualDeviceLogic, DisplayVirtualDeviceLogic) all return their respective typed attributes matching the new interface signature that returns T instead of GenericDeviceAttribute[].
src/device/genericDeviceUpdater.ts (1)
19-19: The condition is correct and does not have a falsy value handling issue.The
getAttributemethod returnsPromise<T[K] | undefined>whereT[K]is an attribute object (not a primitive value). When an attribute exists, it returns the attribute object which is always truthy; when it doesn't exist, it returnsundefinedwhich is falsy. Therefore,!await device.getAttribute(attrKey)correctly checks attribute existence, not the truthiness of the attribute's value. The code properly skips only attributes that don't exist, regardless of their.valueproperty.Likely an incorrect or invalid review comment.
src/device/protocol/virtual/virtualDeviceProvider.ts (2)
9-9: LGTM: Import addition aligns with type refactoring.The Device import supports the type generalization in the removeDevice method.
86-86: LGTM: Type generalization improves flexibility.Changing the parameter type from
VirtualDevicetoDevicemakes the method more flexible while maintaining type safety.src/device/attribute/rangeGenericDeviceAttribute.ts (2)
4-4: LGTM: Generic type parameter aligns with attribute refactor.Extending
GenericDeviceAttribute<number>is consistent with other numeric attribute types (IntGenericDeviceAttribute, FloatGenericDeviceAttribute) and properly constrains the value type.
17-19: LGTM: fromString implementation is appropriate.The method correctly converts string input to number and aligns with similar implementations in other numeric attribute classes.
tests/unit/device/protocol/slvCtrlPlus/slvCtrlPlusMessageParser.spec.ts (3)
21-54: LGTM: Test updates correctly reflect object-based attribute access.The test assertions have been systematically updated to:
- Use
Object.keys(result).lengthfor counting attributes- Access attributes by name (e.g.,
result.connected) instead of array indices- Cast specific attribute types where needed
These changes correctly validate the new
DeviceAttributesobject-based structure.
77-90: LGTM: Edge case tests properly updated.The empty and malformed attribute tests correctly use the object-based API and verify expected behavior.
101-106: LGTM: Malformed attribute test correctly validates object access.The test properly verifies that valid attributes are accessible by name (
result.bar) while malformed ones are ignored.src/device/updater/bufferedDeviceUpdater.ts (1)
16-27: LGTM: Async refactor properly implemented.The update flow has been correctly converted to async/await:
- Public
updatemethod now properly awaits the queue operation- Private
handleUpdateawaits the nested updater call- Aligns with the
DeviceUpdaterInterfacesignature expectingPromise<void>This ensures proper completion tracking for buffered updates.
src/device/protocol/slvCtrlPlus/slvCtrlPlusDevice.ts (3)
2-2: LGTM: Import supports generic attribute typing.The
DeviceAttributesimport enables the generic constraint on the class.
6-6: LGTM: Generic parameter properly constrained with sensible default.The class declaration:
- Introduces generic parameter
A extends DeviceAttributes- Provides default type
DeviceAttributesfor backward compatibility- Extends
Device<A>to maintain type flowThis allows concrete device classes to specify their exact attribute structure while maintaining type safety.
17-17: LGTM: Constructor signature updated consistently.The
attributesparameter type correctly uses the genericA, ensuring type consistency throughout the inheritance chain.src/device/protocol/slvCtrlPlus/slvCtrlPlusMessageParser.ts (3)
8-8: LGTM: Import enables object-based attribute return type.The
DeviceAttributestype import supports the refactored return type.
20-22: LGTM: Method signature and initialization updated for object-based attributes.Changes properly reflect the shift from array to object:
- Return type now
DeviceAttributes(Record<string, GenericDeviceAttribute>)- Initialized as empty object
{} as DeviceAttributes
36-46: LGTM: Parsing logic correctly builds attribute object.The implementation properly:
- Renames loop variable to
attrDeffor clarity (definition string)- Creates attribute via
createAttributeFromValue- Assigns to object using attribute name as key:
attributeList[attr.name] = attrThis correctly builds the keyed attribute map.
tests/unit/device/protocol/buttplugIo/buttplugIoDevice.spec.ts (5)
4-6: LGTM: Import includes strongly-typed attribute container.The
ButtplugIoDeviceAttributestype import enables type-safe test construction.
13-24: LGTM: Test helper updated to use object-based attributes.The
createDevicefunction signature correctly usesButtplugIoDeviceAttributesparameter type, aligning with the new attribute model.
30-30: LGTM: Empty attributes object for error case.Passing an empty object
{}correctly tests the non-existing attribute error path.
49-62: LGTM: Boolean attribute test updated correctly.The test properly:
- Creates attribute object with keyed structure:
{'bool-1': boolAttr}- Accesses result value via
(await device.getAttribute(boolAttr.name)).value
74-93: LGTM: Range attribute test updated correctly.The test uses the same object-based pattern and properly validates the scaled scalar calculation.
src/device/attribute/listGenericDeviceAttribute.ts (1)
4-6: LGTM: Generic type parameters properly introduced.The class now uses:
- Generic parameters
<K extends string|number, V extends string|number>- Extends
GenericDeviceAttribute<K>for type safety- Properly typed
values: Map<K, V>This enables strongly-typed list attributes with specific key/value types.
src/device/attribute/genericDeviceAttribute.ts (2)
3-4: Well-designed generic abstraction for typed attributes.The introduction of
AttributeValueas a union type and makingGenericDeviceAttributegeneric with a constrained type parameter is a clean approach. MakingfromStringabstract ensures each concrete attribute type handles its own parsing logic.Also applies to: 12-12, 28-28
25-26: Thevalueproperty initialization follows an intentional pattern for this DTO class. While technically uninitialized, it's always populated before use throughplainToInstance()deserialization or direct assignment. The current design is appropriate for the class-transformer serialization pattern used throughout the codebase.Likely an incorrect or invalid review comment.
src/device/protocol/buttplugIo/buttplugIoDeviceFactory.ts (1)
55-86: Clean refactor from array to map-based attributes.The transition to returning
ButtplugIoDeviceAttributesas a keyed object is well-implemented. The explicit initialization ofattr.value = 0for each attribute addresses potential undefined value issues.src/device/device.ts (3)
93-102: Good defensive typing for getAttribute.The documentation explaining that
undefinedcan be returned when dealing withPartialtypes is helpful. The implementation correctly accesses the attributes map.
5-6: Well-structured generic refactor of the Device base class.The transition to a generic
DeviceAttributestype parameter with corresponding changes to the constructor and abstractsetAttributesignature provides strong typing throughout the device hierarchy.Also applies to: 12-12, 39-39, 47-47, 104-104
7-9: No changes needed. TheDeviceDatatype is actively used throughout the codebase as an exported type, imported and utilized in socket types, controllers, and device updater classes. This is the intended pattern for this type definition.src/device/protocol/zc95/zc95Device.ts (4)
13-34: Strong type system improvements!The new attribute type structure is well-designed:
- Clear separation between required and optional attributes
- Good use of template literal types for dynamic pattern attributes
- Type safety improvements over the previous array-based approach
69-166: Well-structured refactoring of setAttribute logic.The generic constraint
K extends keyof Zc95DeviceAttributesprovides excellent type safety, and the refactored setter methods correctly use the new attribute-based model. The recursive call at Line 142 is safe due to the guard condition at Line 149.
168-184: Helper methods properly refactored.All helper methods have been correctly updated:
getChannelPowerAttributesreturns a properly typed object structureremovePatternAttributesAndDatauses the type-safegetTypedKeyshelpergetTypedKeysis a useful utility for maintaining type safety during iteration- Type guard
isPowerChannelAttributesignature correctly updatedAlso applies to: 187-195, 264-268, 282-284
36-36: Breaking changes verified and properly implemented throughout the codebase.The class signature, constructor, and
setAttributemethod changes are all correctly implemented:
- The
Devicebase class supports the generic constraint withDevice<T extends DeviceAttributes>Zc95Deviceproperly extendsDevice<Zc95DeviceAttributes>with the correct generic parameter- The constructor accepts
attributes: Zc95DeviceAttributesand passes it to the parent classsetAttributeuses proper generic constraints:setAttribute<K extends keyof Zc95DeviceAttributes>(attributeName: K, value: Zc95DeviceAttributes[K]['value'])Zc95DeviceFactory.create()correctly instantiatesZc95Devicewith all parameters in the proper order, including the newZc95DeviceAttributestypeAll breaking changes have been consistently applied across the codebase.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/serial/SynchronousSerialPort.ts (1)
28-38: Fix race condition: promise resolves before write completes.The promise resolves immediately on line 36 without waiting for the write operation to complete. This creates a race where
resolve()is called before the write callback can potentially callreject(err), meaning the method may return successfully even if the write fails.🔎 Proposed fix
public async write(data: string): Promise<void> { return new Promise<void>((resolve, reject) => { this.writer.write(data, (err: Error | null | undefined) => { if (err) { reject(err); + } else { + resolve(); } }); - - resolve(); }); }src/automation/scriptRuntime.ts (2)
80-92: Inconsistent cleanup instop()method.The
stop()method setsvm,sandbox, andrunningSinceto null but leavesscriptCodeuntouched and doesn't nulllogWriterafter closing it. This creates ambiguous state after stopping.For consistent cleanup semantics, all nullable runtime resources should be handled uniformly. After
stop(), the runtime should be in the same state as beforeload()was called.🔎 Suggested fix for consistent cleanup
public stop(): void { + this.scriptCode = null; this.vm = null; this.sandbox = null; this.runningSince = null; if (null !== this.logWriter) { this.logWriter.close(); + this.logWriter = null; } this.eventEmitter.emit(AutomationEventType.scriptStopped); console.log('script stopped') }
47-78: Potential resource leak ifload()is called multiple times.The
load()method creates new resources (VMScript, NodeVM, WriteStream) without cleaning up existing ones. Ifload()is called again without callingstop()first, the old resources will be orphaned, particularly the WriteStream file handle.Consider adding a guard or calling cleanup logic at the start of
load()to prevent resource leaks.🔎 Suggested fix to prevent resource leaks
public load(scriptCode: string): void { + // Clean up existing resources before loading new ones + if (null !== this.logWriter) { + this.logWriter.close(); + } + this.scriptCode = new VMScript(scriptCode); this.sandbox = { event: { type: null, device: null }, devices: this.deviceRepository, context: {}, } this.vm = new NodeVM({ console: 'redirect', require: { external: true, root: './', }, sandbox: this.sandbox }); this.logWriter = fs.createWriteStream(`${this.logPath}/automation.log`) this.vm.on('console.log', (data: string) => { console.log(`VM stdout: ${data}`); void this.log(data); this.eventEmitter.emit(AutomationEventType.consoleLog, data); }); this.runningSince = new Date(); this.eventEmitter.emit(AutomationEventType.scriptStarted); console.log('script loaded') }
🧹 Nitpick comments (19)
src/schemaValidation/JsonSchemaValidator.ts (1)
23-25: Consider applying the same pattern for consistency.For consistency with line 20, you might consider using the nullish coalescing operator here as well, though
ajv.errorsText()typically handles undefined gracefully.🔎 Optional consistency improvement
public getValidationErrorsAsText(): string { - return this.ajv.errorsText(this.schemaValidator.errors); + return this.ajv.errorsText(this.schemaValidator.errors ?? []); }src/controller/automation/runScriptController.ts (1)
16-21: Content-Type validation logic is correct.The explicit check improves type safety for strict mode. The logic correctly rejects requests when the content type doesn't match.
💡 Optional: Simplify the condition
The condition could be more concise while maintaining the same behavior:
- const matchedContentType = req.is('text/plain'); - - if(false === matchedContentType || null === matchedContentType) { + if(!req.is('text/plain')) { res.status(400).send('Content-Type header must be text/plain'); return; }Since
req.is()returnsstring | false | null, the negation!req.is(...)will betruefor bothfalseandnull, andfalsefor any matching string.src/util/objects.ts (1)
3-5: LGTM! Idiomatic typed key accessor.This follows the standard TypeScript pattern for obtaining strongly-typed object keys. The implementation correctly wraps
Object.keys()with a type assertion to provide better type safety for downstream consumers, which aligns well with the strict mode typing goals mentioned in the commits.Optional: Consider adding JSDoc for API clarity
Since this is a utility function that will be used across the codebase, brief JSDoc documentation could help other developers understand its purpose:
+/** + * Returns the keys of an object with proper TypeScript typing. + * Only includes string keys (excludes symbols). + */ export const getTypedKeys = <T extends object>(obj: T): (StringKey<T>)[] => { return Object.keys(obj) as StringKey<T>[]; }src/controller/automation/createScriptController.ts (1)
17-22: LGTM! Explicit type validation improves clarity.The intermediate variable and explicit checks for both
falseandnullmake the validation logic clearer and align well with the PR's stricter typing approach. This correctly handles both content-type mismatch and missing body cases.Optional style refinements
If desired, consider these minor style adjustments:
- const matchedContentType = req.is('text/plain'); - - if(false === matchedContentType || null === matchedContentType) { + const matchedContentType = req.is('text/plain'); + + if (matchedContentType === false || matchedContentType === null) { res.status(400).send('Content-Type header must be text/plain'); return; }
- Add space after
ifkeyword- Use conventional comparison order (subject-verb-object vs. Yoda style)
These are purely stylistic and don't affect functionality.
src/serial/SynchronousSerialPort.ts (2)
45-46: Simplify eslint-disable comment.The comment includes both the TypeScript ESLint rule name and the base ESLint rule name. Since you're using TypeScript, only the
@typescript-eslint/no-empty-functionrule is necessary.🔎 Proposed simplification
- // eslint-disable-next-line @typescript-eslint/no-empty-function,no-empty-function + // eslint-disable-next-line @typescript-eslint/no-empty-function let removeListeners: () => void = () => {};
54-59: Remove redundant check.The check
if (undefined !== removeListeners)is now redundant sinceremoveListenersis always initialized to a function (either the no-op on line 46 or the cleanup function on line 61). The function is guaranteed to be defined at this point.🔎 Proposed fix
const dataHandler = (receivedData: string): void => { - if (undefined !== removeListeners) { - removeListeners(); - } + removeListeners(); resolve(receivedData); };src/device/protocol/buttplugIo/buttplugIoWebsocketDeviceProvider.ts (1)
42-43: Minor: URL is constructed twice.The WebSocket URL is built identically on lines 42 and 73. Consider extracting to a property or reusing the value.
Also applies to: 73-73
src/device/attribute/numberDeviceAttribute.ts (1)
5-5: Clarify theNumberAttributeValuetype definition.The type
NotJustUndefined<Int | Float | undefined>appears contradictory:NotJustUndefinedis intended to excludeundefined, yetundefinedis explicitly part of the unionInt | Float | undefined. This results inNumberAttributeValueresolving toInt | Float(withundefinedstripped), which may be the intended behavior but is not immediately clear.Consider simplifying to make the intent explicit:
🔎 Proposed simplification
-export type NumberAttributeValue = NotJustUndefined<Int | Float | undefined>; +export type NumberAttributeValue = Int | Float;Alternatively, if optional numeric values are needed in some contexts, document why
NotJustUndefinedis applied here.src/device/protocol/zc95/zc95DeviceFactory.ts (1)
66-79: Minor formatting: missing space after comma.On line 68, there's a missing space after the comma between
'Pattern'andDeviceAttributeModifier.readWrite.Otherwise, the migration to the new attribute system with
ListDeviceAttributeandBoolDeviceAttributefactories is correct, and the return shape properly aligns with theZc95DeviceAttributestype.🔎 Formatting fix
- 'activePattern', 'Pattern',DeviceAttributeModifier.readWrite, patterns, Int.ZERO + 'activePattern', 'Pattern', DeviceAttributeModifier.readWrite, patterns, Int.ZEROsrc/device/attribute/intRangeDeviceAttribute.ts (2)
72-80: Consider extracting shared parsing logic.The
fromStringimplementation duplicates the parsing logic fromIntDeviceAttribute. Consider extracting this to a shared utility or havingIntRangeDeviceAttributedelegate to a common parsing function to reduce duplication.Possible approach
Create a shared utility in
intDeviceAttribute.ts:export function parseIntValue(value: string, context: string): Int { const res = parseInt(value, 10); if (isNaN(res)) { throw new Error(`Could not convert '${value}' to a valid value for ${context}`); } return res as Int; }Then use it in both classes:
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}`); - } - - return res as T; + return parseIntValue(value, this.constructor.name) as T; }
19-80: Consider adding range validation for values.Neither the constructor nor
fromStringvalidates that values fall within[min, max]. This could allow invalid values to be set or parsed. Consider adding validation:🔎 Proposed validation
Add a private validation method:
private validateRange(value: Int): void { if (value < this._min || value > this._max) { throw new Error( `Value ${value} is out of range [${this._min}, ${this._max}] for ${this.name}` ); } }Then call it in the constructor (if
initialValueis notundefined) and infromString:public constructor(name: string, label: string | undefined, modifier: DeviceAttributeModifier, uom: string | undefined, min: Int, max: Int, incrementStep: Int, initialValue: T) { super(name, label, modifier, uom, initialValue); this._min = min; this._max = max; this._incrementStep = incrementStep; + if (initialValue !== undefined) { + this.validateRange(initialValue as 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}`); } + this.validateRange(res as Int); return res as T; }src/device/protocol/virtual/randomGenerator/randomGeneratorVirtualDeviceLogic.ts (1)
17-26: Consider usingObject.hasOwn()instead ofhasOwnProperty.
hasOwnPropertycan be shadowed if the object has a property with that name. Modern TypeScript prefersObject.hasOwn()or theinoperator.🔎 Proposed fix
public constructor(config: JsonObject) { - if (!config.hasOwnProperty('min')) { + if (!Object.hasOwn(config, 'min')) { throw new Error(`Config value 'min' missing`); } - if (!config.hasOwnProperty('max')) { + if (!Object.hasOwn(config, 'max')) { throw new Error(`Config value 'max' missing`); } this.min = config.min as number; this.max = config.max as number; }src/device/protocol/virtual/virtualDevice.ts (1)
47-66: Consider simplifying to async/await pattern.The explicit
new Promisewithrejectis verbose. Since this is already an async method, you could usethrowdirectly. However, the current implementation is functionally correct.🔎 Proposed simplification
- public async setAttribute<K extends keyof T, V extends AttributeValue<T[K]>>(attributeName: K, value: V): Promise<V> { - return new Promise<V>((resolve, reject) => { - this.state = DeviceState.busy; - - const attribute = this.attributes[attributeName]; - - if (undefined === attribute) { - reject(new Error( - `Attribute named "${attributeName.toString()}" does not exist for device with id "${this.deviceId}"` - )); - return; - } - - attribute.value = value; - - this.state = DeviceState.ready; - - resolve(value); - }); - } + public async setAttribute<K extends keyof T, V extends AttributeValue<T[K]>>(attributeName: K, value: V): Promise<V> { + this.state = DeviceState.busy; + + const attribute = this.attributes[attributeName]; + + if (undefined === attribute) { + this.state = DeviceState.ready; + throw new Error( + `Attribute named "${attributeName.toString()}" does not exist for device with id "${this.deviceId}"` + ); + } + + attribute.value = value; + this.state = DeviceState.ready; + + return value; + }src/device/attribute/deviceAttribute.ts (1)
14-17: Clarify the purpose of thetypefield.The
typefield is alwaysundefinedand marked as "only here to expose it explicitly." Consider adding a brief doc comment explaining how subclasses should set this (if at all) or whether this is for serialization discrimination purposes.🔎 Suggested documentation
@Exclude() export default abstract class DeviceAttribute<T extends AttributeValue = AttributeValue> { + /** + * Discriminator field for serialization. Subclasses should override + * this value for proper type identification during deserialization. + */ @Expose() private readonly type: string|undefined = undefined; // This field is only here to expose it explicitlysrc/device/attribute/listDeviceAttribute.ts (2)
54-57: Consider validating parsed values against the available keys.The
fromStringmethod parses the input but doesn't verify that the result exists in_values. This could allow invalid values if callers don't separately validate usingisValidValue.Consider adding validation or documenting that callers must validate the result:
🔎 Suggested improvement
public fromString(value: string): V { const parsedInt = parseInt(value, 10); - return (isNaN(parsedInt) ? value : parsedInt) as V; + const result = (isNaN(parsedInt) ? value : parsedInt) as V; + if (!this.isValidValue(result)) { + throw new Error(`Value '${value}' is not a valid key for attribute '${this.name}'`); + } + return result; }Alternatively, document that callers must validate using
isValidValueafter callingfromString.
67-72: Type safety consideration:Intvalidation.The
isValidValuemethod checkstypeof value === "number", butIKeyextendsstring|IntwhereIntis a branded type representing integers. This allows any number to pass the type check, not just integers.While this may work at runtime since Map will match the key, consider whether strict integer validation is needed:
public isValidValue(value: unknown): value is NotUndefined<V> { if (typeof value === "string") { return this._values.has(value as IKey); } if (typeof value === "number") { if (!Number.isInteger(value)) { return false; } return this._values.has(value as IKey); } return false; }This ensures only integers (not floats) are accepted when
IKeyisInt.src/device/protocol/buttplugIo/buttplugIoDevice.ts (1)
75-77: Validate attribute name parsing.The attribute name is split and parsed without validation. If the attribute name doesn't match the expected format
${ActuatorType}-${number}, this could lead to runtime errors.Consider adding validation:
🔎 Suggested improvement
const [actuatorType, index] = attributeName.split('-'); + +if (!actuatorType || !index) { + throw new Error(`Invalid attribute name format: ${attributeName}`); +} + +const parsedIndex = parseInt(index, 10); +if (isNaN(parsedIndex)) { + throw new Error(`Invalid index in attribute name: ${attributeName}`); +} -await this.send(actuatorType as ActuatorType, parseInt(index, 10), valueToSend); +await this.send(actuatorType as ActuatorType, parsedIndex, valueToSend);src/device/protocol/slvCtrlPlus/slvCtrlPlusMessageParser.ts (1)
106-115: Consider validation for range parsing.Lines 112-113 use
parseIntwithout checking forNaN. If the regex matches but the values aren't valid integers,Int.from()will throw an error.While the regex
(\d+)-(\d+)should ensure valid integers, consider adding explicit validation for robustness:🔎 Suggested improvement
} else if (null !== (result = reRange.exec(value))) { + const min = parseInt(result[1], 10); + const max = parseInt(result[2], 10); + if (isNaN(min) || isNaN(max)) { + throw new Error(`Invalid range values in attribute definition: ${value}`); + } attr = IntRangeDeviceAttribute.create( name, undefined, modifier, undefined, - Int.from(parseInt(result[1], 10)), - Int.from(parseInt(result[2], 10)), + Int.from(min), + Int.from(max), Int.from(1), );src/device/protocol/zc95/zc95Device.ts (1)
255-288: Redundant value assignment after initialization.Line 284 sets the attribute value to
menuItem.Default, but this was already done during initialization withcreateInitialized(lines 264 and 275). ThecreateInitializedfactory methods acceptinitialValueas the last parameter and set it during construction.Consider removing line 284:
🔎 Suggested cleanup
patternAttributes[attrName as keyof Zc95DevicePatternAttributes] = ListDeviceAttribute.createInitialized<Int, string>( attrName, menuItem.Title, DeviceAttributeModifier.readWrite, new Map(menuItem.Choices.map((choice) => [Int.from(choice.Id), choice.Name])), Int.from(menuItem.Default), ); } - - patternAttributes[`${Zc95Device.patternAttributePrefix}${menuItem.Id}`].value = Int.from(menuItem.Default); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (62)
.eslintrc.cjspackage.jsonsrc/automation/scriptRuntime.tssrc/controller/automation/createScriptController.tssrc/controller/automation/runScriptController.tssrc/device/attribute/boolDeviceAttribute.tssrc/device/attribute/boolGenericDeviceAttribute.tssrc/device/attribute/deviceAttribute.tssrc/device/attribute/floatDeviceAttribute.tssrc/device/attribute/floatGenericDeviceAttribute.tssrc/device/attribute/genericDeviceAttribute.tssrc/device/attribute/intDeviceAttribute.tssrc/device/attribute/intGenericDeviceAttribute.tssrc/device/attribute/intRangeDeviceAttribute.tssrc/device/attribute/listDeviceAttribute.tssrc/device/attribute/listGenericDeviceAttribute.tssrc/device/attribute/numberDeviceAttribute.tssrc/device/attribute/rangeGenericDeviceAttribute.tssrc/device/attribute/strDeviceAttribute.tssrc/device/attribute/strGenericDeviceAttribute.tssrc/device/device.tssrc/device/genericDeviceUpdater.tssrc/device/protocol/buttplugIo/buttplugIoDevice.tssrc/device/protocol/buttplugIo/buttplugIoDeviceFactory.tssrc/device/protocol/buttplugIo/buttplugIoWebsocketDeviceProvider.tssrc/device/protocol/buttplugIo/buttplugIoWebsocketDeviceProviderFactory.tssrc/device/protocol/slvCtrlPlus/genericSlvCtrlPlusDevice.tssrc/device/protocol/slvCtrlPlus/slvCtrlPlusDevice.tssrc/device/protocol/slvCtrlPlus/slvCtrlPlusDeviceFactory.tssrc/device/protocol/slvCtrlPlus/slvCtrlPlusMessageParser.tssrc/device/protocol/slvCtrlPlus/slvCtrlPlusSerialDeviceProvider.tssrc/device/protocol/virtual/audio/ttsVirtualDeviceLogic.tssrc/device/protocol/virtual/delegatedVirtualDeviceFactory.tssrc/device/protocol/virtual/display/displayVirtualDeviceLogic.tssrc/device/protocol/virtual/randomGenerator/randomGeneratorVirtualDeviceLogic.tssrc/device/protocol/virtual/virtualDevice.tssrc/device/protocol/virtual/virtualDeviceProvider.tssrc/device/protocol/zc95/Zc95Serial.tssrc/device/protocol/zc95/zc95Device.tssrc/device/protocol/zc95/zc95DeviceFactory.tssrc/device/protocol/zc95/zc95SerialDeviceProvider.tssrc/device/provider/deviceProviderLoader.tssrc/device/transport/serialDeviceTransport.tssrc/index.tssrc/logging/PinoLogger.tssrc/schemaValidation/JsonSchemaValidator.tssrc/serial/SynchronousSerialPort.tssrc/serialization/classToPlainSerializer.tssrc/serialization/discriminator/genericDeviceAttributeDiscriminator.tssrc/settings/deviceSource.tssrc/settings/knownDevice.tssrc/settings/settings.tssrc/settings/settingsManager.tssrc/util/numbers.tssrc/util/objects.tssrc/version.tstests/unit/device/protocol/buttplugIo/buttplugIoDevice.spec.tstests/unit/device/protocol/slvCtrlPlus/genericSlvCtrlPlusDevice.spec.tstests/unit/device/protocol/slvCtrlPlus/slvCtrlPlusMessageParser.spec.tstests/unit/device/protocol/virtual/display/displayVirtualDevice.spec.tstests/unit/device/testDevice.tstsconfig.json
💤 Files with no reviewable changes (9)
- src/device/attribute/floatGenericDeviceAttribute.ts
- src/device/protocol/buttplugIo/buttplugIoWebsocketDeviceProviderFactory.ts
- src/device/attribute/boolGenericDeviceAttribute.ts
- src/device/attribute/rangeGenericDeviceAttribute.ts
- package.json
- src/device/attribute/genericDeviceAttribute.ts
- src/device/attribute/intGenericDeviceAttribute.ts
- src/device/attribute/strGenericDeviceAttribute.ts
- src/device/attribute/listGenericDeviceAttribute.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- src/serialization/classToPlainSerializer.ts
- src/device/protocol/virtual/display/displayVirtualDeviceLogic.ts
- tests/unit/device/protocol/slvCtrlPlus/genericSlvCtrlPlusDevice.spec.ts
- tests/unit/device/protocol/virtual/display/displayVirtualDevice.spec.ts
- src/device/protocol/virtual/virtualDeviceProvider.ts
- src/index.ts
🧰 Additional context used
🧬 Code graph analysis (27)
src/logging/PinoLogger.ts (1)
src/logging/Logger.ts (1)
Logger(1-11)
src/device/protocol/zc95/Zc95Serial.ts (1)
src/device/protocol/zc95/Zc95Messages.ts (1)
MsgResponse(4-10)
src/device/genericDeviceUpdater.ts (2)
src/device/device.ts (1)
DeviceData(11-14)src/util/objects.ts (1)
getTypedKeys(3-5)
src/device/attribute/floatDeviceAttribute.ts (3)
src/device/attribute/deviceAttribute.ts (3)
NotJustUndefined(4-4)value(53-55)value(57-59)src/util/numbers.ts (2)
Float(17-17)Float(20-29)src/device/attribute/numberDeviceAttribute.ts (1)
uom(23-25)
src/controller/automation/createScriptController.ts (1)
src/middleware/contentTypeMiddleware.ts (1)
req(3-18)
src/device/attribute/intDeviceAttribute.ts (4)
src/device/attribute/deviceAttribute.ts (4)
NotJustUndefined(4-4)name(38-40)value(53-55)value(57-59)src/util/numbers.ts (2)
Int(2-2)Int(5-14)src/settings/knownDevice.ts (1)
name(43-45)src/device/attribute/numberDeviceAttribute.ts (1)
uom(23-25)
src/device/protocol/virtual/virtualDevice.ts (2)
src/device/protocol/virtual/virtualDeviceLogic.ts (1)
VirtualDeviceLogic(4-11)src/device/attribute/deviceAttribute.ts (3)
AttributeValue(6-6)value(53-55)value(57-59)
src/device/protocol/virtual/randomGenerator/randomGeneratorVirtualDeviceLogic.ts (3)
src/device/attribute/intDeviceAttribute.ts (1)
IntDeviceAttribute(8-38)src/device/protocol/virtual/virtualDeviceLogic.ts (1)
VirtualDeviceLogic(4-11)src/util/numbers.ts (2)
Int(2-2)Int(5-14)
src/serialization/discriminator/genericDeviceAttributeDiscriminator.ts (6)
src/device/attribute/boolDeviceAttribute.ts (1)
BoolDeviceAttribute(6-32)src/device/attribute/intDeviceAttribute.ts (1)
IntDeviceAttribute(8-38)src/device/attribute/floatDeviceAttribute.ts (1)
FloatDeviceAttribute(8-48)src/device/attribute/strDeviceAttribute.ts (1)
StrDeviceAttribute(6-32)src/device/attribute/intRangeDeviceAttribute.ts (1)
IntRangeDeviceAttribute(9-81)src/device/attribute/listDeviceAttribute.ts (1)
ListDeviceAttribute(11-73)
src/device/attribute/intRangeDeviceAttribute.ts (5)
src/util/numbers.ts (2)
Int(2-2)Int(5-14)src/device/attribute/intDeviceAttribute.ts (1)
IntAttributeValue(5-5)src/device/attribute/deviceAttribute.ts (5)
name(38-40)label(42-44)modifier(46-48)value(53-55)value(57-59)src/settings/knownDevice.ts (1)
name(43-45)src/device/attribute/numberDeviceAttribute.ts (1)
uom(23-25)
src/device/protocol/slvCtrlPlus/slvCtrlPlusDeviceFactory.ts (1)
src/settings/knownDevice.ts (1)
serialNo(39-41)
src/device/protocol/slvCtrlPlus/slvCtrlPlusMessageParser.ts (10)
src/device/protocol/slvCtrlPlus/slvCtrlPlusDevice.ts (1)
SlvCtrlPlusDeviceAttributes(7-7)src/device/attribute/deviceAttribute.ts (4)
value(53-55)value(57-59)name(38-40)modifier(46-48)src/settings/knownDevice.ts (2)
name(43-45)type(47-49)src/device/attribute/boolDeviceAttribute.ts (1)
BoolDeviceAttribute(6-32)src/device/attribute/intDeviceAttribute.ts (1)
IntDeviceAttribute(8-38)src/device/attribute/floatDeviceAttribute.ts (1)
FloatDeviceAttribute(8-48)src/device/attribute/strDeviceAttribute.ts (1)
StrDeviceAttribute(6-32)src/device/attribute/intRangeDeviceAttribute.ts (1)
IntRangeDeviceAttribute(9-81)src/util/numbers.ts (2)
Int(2-2)Int(5-14)src/device/attribute/listDeviceAttribute.ts (1)
ListDeviceAttribute(11-73)
tests/unit/device/protocol/buttplugIo/buttplugIoDevice.spec.ts (4)
src/device/protocol/buttplugIo/buttplugIoDevice.ts (2)
ButtplugIoDeviceAttributes(14-17)ButtplugIoDeviceAttributeKey(12-12)src/device/attribute/boolDeviceAttribute.ts (1)
BoolDeviceAttribute(6-32)src/device/attribute/intRangeDeviceAttribute.ts (1)
IntRangeDeviceAttribute(9-81)src/util/numbers.ts (2)
Int(2-2)Int(5-14)
src/settings/knownDevice.ts (2)
src/settings/deviceSource.ts (3)
id(21-23)type(25-27)config(29-31)src/device/attribute/deviceAttribute.ts (1)
name(38-40)
src/device/protocol/zc95/zc95DeviceFactory.ts (4)
src/util/numbers.ts (2)
Int(2-2)Int(5-14)src/device/protocol/zc95/zc95Device.ts (1)
Zc95DeviceAttributes(34-35)src/device/attribute/listDeviceAttribute.ts (1)
ListDeviceAttribute(11-73)src/device/attribute/boolDeviceAttribute.ts (1)
BoolDeviceAttribute(6-32)
src/device/protocol/zc95/zc95Device.ts (7)
src/device/attribute/listDeviceAttribute.ts (2)
InitializedListDeviceAttribute(6-9)ListDeviceAttribute(11-73)src/util/numbers.ts (2)
Int(2-2)Int(5-14)src/device/attribute/boolDeviceAttribute.ts (1)
InitializedBoolDeviceAttribute(4-4)src/device/attribute/intRangeDeviceAttribute.ts (2)
IntRangeDeviceAttribute(9-81)InitializedIntRangeDeviceAttribute(7-7)src/device/device.ts (1)
AttributeValue(8-8)src/util/objects.ts (1)
getTypedKeys(3-5)src/device/protocol/zc95/Zc95Messages.ts (2)
MinMaxMenuItem(46-51)MultiChoiceMenuItem(53-55)
src/device/attribute/listDeviceAttribute.ts (2)
src/util/numbers.ts (2)
Int(2-2)Int(5-14)src/device/attribute/deviceAttribute.ts (6)
name(38-40)label(42-44)modifier(46-48)value(53-55)value(57-59)NotUndefined(5-5)
src/device/protocol/buttplugIo/buttplugIoDeviceFactory.ts (5)
src/device/protocol/buttplugIo/buttplugIoDevice.ts (2)
ButtplugIoDeviceAttributes(14-17)ButtplugIoDeviceAttributeKey(12-12)src/device/attribute/intRangeDeviceAttribute.ts (1)
IntRangeDeviceAttribute(9-81)src/util/numbers.ts (2)
Int(2-2)Int(5-14)src/device/attribute/boolDeviceAttribute.ts (1)
BoolDeviceAttribute(6-32)src/device/attribute/intDeviceAttribute.ts (1)
IntDeviceAttribute(8-38)
src/device/protocol/slvCtrlPlus/slvCtrlPlusDevice.ts (1)
src/device/protocol/slvCtrlPlus/genericSlvCtrlPlusDevice.ts (1)
Exclude(9-118)
src/settings/deviceSource.ts (1)
src/settings/knownDevice.ts (3)
id(35-37)type(47-49)config(55-57)
src/controller/automation/runScriptController.ts (1)
src/middleware/contentTypeMiddleware.ts (1)
req(3-18)
src/device/attribute/strDeviceAttribute.ts (1)
src/device/attribute/deviceAttribute.ts (7)
NotJustUndefined(4-4)name(38-40)label(42-44)modifier(46-48)value(53-55)value(57-59)NotUndefined(5-5)
src/device/protocol/slvCtrlPlus/genericSlvCtrlPlusDevice.ts (4)
src/device/protocol/slvCtrlPlus/slvCtrlPlusDevice.ts (1)
SlvCtrlPlusDeviceAttributes(7-7)src/device/attribute/deviceAttribute.ts (3)
AttributeValue(6-6)value(53-55)value(57-59)src/device/device.ts (2)
AttributeValue(8-8)DeviceAttributes(6-6)src/device/attribute/boolDeviceAttribute.ts (1)
BoolDeviceAttribute(6-32)
src/device/device.ts (1)
src/device/attribute/deviceAttribute.ts (3)
AttributeValue(6-6)value(53-55)value(57-59)
src/automation/scriptRuntime.ts (1)
src/repository/deviceRepositoryInterface.ts (1)
DeviceRepositoryInterface(3-8)
src/device/protocol/virtual/delegatedVirtualDeviceFactory.ts (1)
src/device/protocol/virtual/virtualDeviceFactory.ts (1)
VirtualDeviceFactory(4-9)
tests/unit/device/testDevice.ts (1)
src/device/device.ts (2)
DeviceAttributes(6-6)AttributeValue(8-8)
🔇 Additional comments (72)
src/schemaValidation/JsonSchemaValidator.ts (1)
19-21: LGTM! Good defensive programming.The nullish coalescing operator ensures the method always returns an array, preventing potential runtime errors when callers expect array methods to be available.
src/util/objects.ts (1)
1-1: LGTM! Clean type helper.The
StringKey<T>helper correctly extracts string keys from an object type, properly filtering out symbols and numbers..eslintrc.cjs (3)
106-106: Excellent addition for type safety.The
@typescript-eslint/strict-boolean-expressionsrule prevents implicit truthiness checks on non-boolean types, requiring explicit comparisons (e.g.,value !== nullinstead ofif (value)). This aligns perfectly with the PR's goal of enabling strict mode and full typing.
152-157: Good adjustment for TypeScript conventions.Relaxing
no-underscore-dangleto allowthis._propertyaccommodates the common TypeScript pattern for private-like fields, which aligns with idiomatic TypeScript code while still preventing underscore dangles in other contexts.
162-166: Enforces explicit type conversions for clarity.The
no-implicit-coercionrule prevents shortcuts like!!valueor+value, requiring explicitBoolean(),Number(), andString()calls. While this adds some verbosity, it improves code clarity and prevents subtle coercion bugs, which is consistent with the strict typing approach in this PR.src/serial/SynchronousSerialPort.ts (1)
84-84: Manual verification required for type cast inqueue.push()return type.The cast
as Promise<string>cannot be verified automatically due to repository access issues. Verify thatSequentialTaskQueue.push()return type aligns withPromise<string>when the callback returnsPromise<string>, and confirm whether the type assertion is necessary or can be removed.src/automation/scriptRuntime.ts (3)
27-40: LGTM! Nullable field declarations align with strict typing.The nullable declarations properly represent the ScriptRuntime's lifecycle states (unloaded vs. loaded) and align with the PR's strict TypeScript mode initiative. The explicit null initialization and subsequent assignment in
load()make the state transitions clear.
94-126: LGTM! Proper guards and return type alignment.The execution guard in
runForEvent()correctly verifies all required runtime resources (vm, sandbox, scriptCode) are non-null before executing, preventing crashes if called beforeload()or afterstop().The
getRunningSince()return type change toDate|nullproperly aligns with the nullable field declaration.
128-133: LGTM! Defensive guard for null logWriter.The null check before writing prevents crashes if
log()is called beforeload()or afterstop(). The silent no-op behavior is appropriate for a logging method.src/device/protocol/slvCtrlPlus/slvCtrlPlusSerialDeviceProvider.ts (1)
102-109: LGTM!The type correction from
ErrortoError|nullproperly reflects the SerialPort callback signature whereerrisnullon successful open. The existing null-check logic handles this correctly.src/serialization/discriminator/genericDeviceAttributeDiscriminator.ts (1)
1-17: LGTM!The imports and discriminator map are correctly updated to reference the new attribute classes while preserving the same serialization keys for backward compatibility.
src/util/numbers.ts (1)
1-29: LGTM!The branded numeric types (
IntandFloat) are well-implemented using TypeScript's nominal typing pattern. The runtime validation infrom()methods correctly enforces constraints:
Int.from: Rejects non-integers viaNumber.isIntegerFloat.from: RejectsNaNand infinite values viaisNaNandNumber.isFiniteThe merged type-and-const pattern with eslint disables is an accepted idiom for this use case.
src/device/attribute/boolDeviceAttribute.ts (1)
25-27: VerifyfromStringparsing logic matches protocol expectations.The current implementation treats only
'1'astrueand everything else (including'true','TRUE','yes') asfalse. This may be intentional for a specific wire protocol, but could be surprising if string-based boolean inputs vary.src/device/protocol/zc95/Zc95Serial.ts (1)
16-16: LGTM!Initializing
pendingRecvMessagetonullis more explicit than leaving it undefined and aligns with therecv()return type ofPromise<MsgResponse | null>.src/device/protocol/zc95/zc95SerialDeviceProvider.ts (1)
65-68: LGTM!Good defensive check that fails fast with a clear error message rather than allowing a potential undefined access on
versionDetails.ZC95. The error is properly caught by the surrounding try-catch block.src/device/provider/deviceProviderLoader.ts (1)
41-46: LGTM!Good simplification - consolidating the
has()+get()pattern into a singleget()with an undefined check eliminates the redundant map lookup.tsconfig.json (1)
11-11: LGTM! Excellent type safety improvement.Enabling strict mode is a best practice that catches many potential runtime errors at compile time. This aligns well with the PR's objective of introducing a fully-typed attribute system.
src/device/protocol/slvCtrlPlus/slvCtrlPlusDeviceFactory.ts (1)
63-80: LGTM! Clean refactor to constructor-based initialization.The change from null-checking to undefined-checking aligns with strict mode, and using the parameterized
KnownDeviceconstructor improves immutability and encapsulation. The logic is preserved correctly.src/device/protocol/virtual/delegatedVirtualDeviceFactory.ts (1)
3-3: LGTM! Improved type specificity and cleaner retrieval pattern.The return type is now more specific (
VirtualDeviceinstead ofDevice), and the factory retrieval pattern is simplified by eliminating the redundanthas()check. The error handling is preserved.Also applies to: 9-23
src/settings/settingsManager.ts (3)
96-98: LGTM! Good defensive programming.The early return guard prevents attempting to save undefined settings, avoiding potential errors. This aligns well with the optional settings field change.
118-122: LGTM! Improved encapsulation with constructor usage.Using the
DeviceSourceconstructor instead of stepwise property assignment improves immutability and aligns with the encapsulation improvements across the codebase.
91-93: Unable to verify caller handling of potentially undefined return value fromgetSettings().The review comment identifies that
getSettings()now returnsSettings|undefinedinstead ofSettings, which is a breaking API change. However, verification of whether all callers properly handle the undefined case could not be completed due to repository access limitations.src/settings/deviceSource.ts (1)
6-31: LGTM! Excellent encapsulation with proper serialization mapping.The refactor from public fields to private readonly fields with getters improves encapsulation and immutability. The
@Exposedecorators correctly maintain serialization compatibility with the original field names, ensuring backward compatibility with persisted settings.src/device/attribute/numberDeviceAttribute.ts (1)
27-29: TheisValidValuecheck doesn't enforce brandedIntorFloattypes.The validation
typeof value === 'number'accepts any JavaScript number, including those not explicitly branded asIntorFloat. While this may be acceptable at runtime (since branded types are erased), it means the type guard doesn't enforce the stricter compile-time contract.If stricter validation is needed (e.g., checking that integers don't have fractional parts), consider adding runtime checks. Otherwise, this is acceptable.
src/settings/settings.ts (1)
11-20: LGTM! Constructor initialization and consistent use ofundefined.The explicit constructor properly initializes both maps, and the migration from
nulltoundefinedfor the return type ofgetKnownDeviceByIdaligns with TypeScript best practices.tests/unit/device/testDevice.ts (1)
1-25: LGTM! Test device correctly migrated to map-based attribute model.The changes properly align with the new typed attribute system:
- Empty object
{}replaces the empty array for attributes- Generic
setAttributesignature correctly constrains keys and values usingDeviceAttributesandAttributeValuesrc/device/protocol/slvCtrlPlus/slvCtrlPlusDevice.ts (1)
4-25: LGTM! Generic attribute typing enhances type safety.The introduction of generic type parameter
A extends SlvCtrlPlusDeviceAttributesprovides strong typing for device attributes while maintaining flexibility. The type aliasesSlvCtrlPlusDeviceAttributeKeyandSlvCtrlPlusDeviceAttributesclearly document the attribute structure.src/device/attribute/strDeviceAttribute.ts (1)
1-32: LGTM! String attribute implementation is clean and follows established patterns.The factory methods (
createInitializedandcreate) provide clear initialization patterns, and thefromStringandisValidValueimplementations are appropriate for string attributes.src/device/attribute/intDeviceAttribute.ts (1)
1-38: LGTM! Integer attribute implementation with proper parsing validation.The
fromStringmethod correctly usesparseIntwith radix 10 and throws a descriptive error on invalid input. The use ofthis.constructor.namein the error message aids debugging.src/device/protocol/virtual/randomGenerator/randomGeneratorVirtualDeviceLogic.ts (2)
7-11: Well-structured typed attribute definition.The
RandomGeneratorVirtualDeviceAttributestype provides clear typing for the device's attributes, aligning with the broader PR goal of uniting attribute values into a typed key-value structure.
32-45: Correct use ofInt.from()for type-safe integer values.The
refreshDatamethod properly wraps the random number withInt.from()to satisfy theIntDeviceAttributetype requirements. TheconfigureAttributesmethod returns a properly structured object matchingRandomGeneratorVirtualDeviceAttributes.src/device/protocol/virtual/virtualDevice.ts (1)
2-7: Good generic typing for flexible device attributes.The generic type parameter
T extends DeviceAttributes = DeviceAttributesproperly constrains the attribute types while providing a sensible default. The import from"../../device.js"correctly includes the.jsextension for ESM compatibility.tests/unit/device/protocol/buttplugIo/buttplugIoDevice.spec.ts (3)
27-40: Good test coverage for non-existing attribute error handling.The test properly verifies that setting a non-existent attribute throws the expected error with the attribute name in the message.
42-63: Test correctly uses the new attribute API.The test properly creates a
BoolDeviceAttributeusing the factory method and accesses the value via the.valueproperty aftergetAttribute().
65-100: Comprehensive test for range attribute handling.The test correctly:
- Creates an
IntRangeDeviceAttributewith properInttyped bounds- Uses
Int.from()for the new value- Verifies the scalar calculation uses
newValue/rangeAttr.maxsrc/device/attribute/deviceAttribute.ts (3)
4-6: Well-designed utility types for attribute value constraints.
NotJustUndefined<V>preventsVfrom being onlyundefined, whileNotUndefined<V>excludesundefinedfrom a union. This ensures attributes always have a meaningful value type while allowingundefinedas one option in a union.
61-63: Type guard implementation is correct.The
hasValue()method properly narrows the type. SinceTcould includeundefinedin the union (perAttributeValuedefinition), checking!== undefinedcorrectly identifies when a concrete value exists.
69-71: StaticisInstancetype guard is well-implemented.The pattern using
this: new (...args: any[]) => Uallows subclasses to callBoolDeviceAttribute.isInstance(attr)and get proper type narrowing toBoolDeviceAttribute.tests/unit/device/protocol/slvCtrlPlus/slvCtrlPlusMessageParser.spec.ts (2)
21-53: Tests properly verify the new dictionary-based attribute structure.The assertions correctly:
- Use
Object.keys(result).lengthto count attributes- Access attributes by key (e.g.,
result.connected,result.mode)- Verify
instanceoffor correct attribute class types- Cast to specific types when accessing subclass properties like
min/max
77-106: Edge case tests properly updated for new return type.The tests for empty attributes, malformed attributes, and empty status correctly use
Object.keys(result).lengthto verify the attribute count in the dictionary structure.src/device/protocol/slvCtrlPlus/genericSlvCtrlPlusDevice.ts (2)
46-63: Proper handling of empty attribute values.The use of
undefinedfor empty strings (line 62) aligns with theAttributeValuetype definition which includesundefined. This addresses previous concerns about assigningnullto attribute values.
87-94: Good use of type guard for boolean handling.Using
BoolDeviceAttribute.isInstance(attr)for runtime type checking and converting booleans to0/1for the SlvCtrl protocol is a clean pattern.src/device/protocol/buttplugIo/buttplugIoDeviceFactory.ts (4)
59-77: Correct attribute creation for ScalarCmd items.The factory properly:
- Creates composite keys from
ActuatorType-Index- Uses
IntRangeDeviceAttribute.createInitializedfor step counts > 2- Uses
BoolDeviceAttribute.createInitializedfor binary values- Applies
Int.from()andInt.ZEROfor type-safe integer bounds
80-105: Good fallback logic for SensorReadCmd attributes.The conditional check for
StepRange.length === 2with a fallback toIntDeviceAttributeis a reasonable approach when the range specification is non-standard.
117-131: Clean KnownDevice creation via constructor.The early return pattern for existing devices and the constructor-based creation for new devices aligns with the PR's migration to constructor-based initialization.
56-57: Type assertion on empty object may be unsafe.Casting
{}toButtplugIoDeviceAttributesassumes theRecordtype allows missing keys. IfButtplugIoDeviceAttributesexpects all keys to be present, this could cause runtime issues. Consider usingPartial<ButtplugIoDeviceAttributes>or verifying the type definition allows sparse records.#!/bin/bash # Check ButtplugIoDeviceAttributes type definition rg -n "ButtplugIoDeviceAttributes" --type=ts -A3 -B1 src/device/protocol/buttplugIo/src/settings/knownDevice.ts (1)
6-57: Clean encapsulation pattern with private fields and getters.The refactor to use private readonly fields with public getters aligns well with established patterns. The
@Exposedecorators correctly map internal field names to external API names for serialization.Verify that the
JsonObjecttype used on lines 22 and 25 is properly defined and imported. If this type is not globally declared (e.g., in a.d.tsfile), explicit imports should be added to avoid potential compilation issues in strict mode.src/device/attribute/listDeviceAttribute.ts (2)
1-17: LGTM! Clean type declarations and class structure.The generic type constraints and class declaration are well-designed. The
ListDeviceAttributeclass correctly extendsDeviceAttribute<V>and allows flexible key-value typing withstring|Intas the base type.
19-52: LGTM! Well-structured factory methods.The constructor and factory methods follow a consistent pattern with other attribute classes. The type narrowing in
createInitializedcorrectly ensures the value type isIKey(non-undefined), whilecreateallows undefined values.src/device/protocol/buttplugIo/buttplugIoDevice.ts (4)
10-17: LGTM! Well-structured type definitions.The template literal types for attribute keys and the
Record-based attribute container are correctly implemented. This provides strong typing for buttplug.io device attributes.
20-39: LGTM! Constructor correctly updated for typed attributes.The class declaration and constructor properly use the generic
Device<ButtplugIoDeviceAttributes>pattern, maintaining type safety throughout the attribute handling.
84-93: LGTM! Clean method signature and implementation.The
sendmethod has a clear signature with typed parameters and correctly constructs the buttplug.io command object.
41-46: Add array bounds check before accessingvalue[0].Line 44 accesses
value[0]without verifying the array is non-empty. The buttplug protocol documentation specifies that SensorReading.Data is an array of signed integers with varying value counts (referenced against SensorRange), but does not explicitly guarantee non-empty arrays. Add a length check to prevent passingundefinedtoInt.from():public async refreshData(): Promise<void> { for (const sensor of this.buttplugClientDevice.messageAttributes.SensorReadCmd ?? []) { const value = await this.buttplugClientDevice.sensorRead(sensor.Index, sensor.SensorType); if (value.length > 0) { this.attributes[`${sensor.SensorType}-${sensor.Index}`].value = Int.from(value[0]); } } }src/device/protocol/virtual/audio/ttsVirtualDeviceLogic.ts (3)
10-22: LGTM! Clean type definition.The
TtsVirtualDeviceAttributestype is well-structured with appropriate attribute types for each property. The class correctly implements the genericVirtualDeviceLogicinterface.
32-74: LGTM! Safe attribute handling with proper undefined checks.The
refreshDatamethod correctly uses the typed attribute model. The non-null assertions on lines 33-35 are safe because these attributes are required inTtsVirtualDeviceAttributes. The undefined handling fortextToSpeak(lines 61-64) properly prevents errors.Note: The previous review concern about setting
texttonullhas been resolved—the code now usesundefined(line 43), which is type-safe.
80-113: LGTM! Attribute configuration is correct.The
configureAttributesmethod properly initializes all attributes using the appropriate factory methods. The parameter ordering (e.g., line 103 withundefinedfor unit-of-measure andInt.ZEROfor initial value) is correct.src/device/protocol/slvCtrlPlus/slvCtrlPlusMessageParser.ts (3)
21-54: LGTM! Clean refactor to map-based attributes.The method correctly parses attributes into a keyed object structure. The early returns for invalid input and the validation of attribute parts (line 40) provide good error handling.
The type cast on line 23 is safe because
createAttributeFromValueensures only valid attributes are added, and line 50 uses the attribute's name as the key.
56-78: LGTM! Improved implementation with cleaner parsing.The refactored
parseStatusmethod uses array destructuring and modern JavaScript patterns effectively. The change fromnulltoundefinedfor the return type is more idiomatic for TypeScript.
127-137: LGTM! Clean enum mapping.The
getAttributeTypeFromStrmethod correctly maps string values toDeviceAttributeModifierenum values with proper error handling for invalid inputs.src/device/protocol/zc95/zc95Device.ts (8)
18-35: LGTM! Well-designed type hierarchy.The type definitions effectively model the ZC95 device's attribute structure with required base attributes and optional dynamic attributes for power channels and patterns. The use of
Partial<>andRequired<>provides the right level of flexibility.
52-69: LGTM! Clean constructor and refreshData implementation.The constructor correctly accepts and forwards the typed attributes, and
refreshDatais appropriately simplified to delegate to message processing.
71-95: LGTM! Well-structured setAttribute with type-safe routing.The
setAttributemethod correctly routes to specialized handlers based on attribute name patterns. The type casts are safe given the generic constraints, and the error handling provides clear feedback.
97-111: LGTM! Type-safe pattern detail handling.The method correctly uses type guards (
IntRangeDeviceAttribute.isInstanceandListDeviceAttribute.isInstance) along withisValidValuechecks before updating the attribute. This ensures type safety at runtime.
113-147: LGTM! Safe power channel handling with type guards.The
setAttributePowerChannelmethod uses theallPowerChannelValuesDefinedtype guard to ensure all power channel attributes have values before accessing them. This prevents runtime errors and provides proper type narrowing.
150-185: LGTM! Well-structured pattern state management.The methods correctly manage pattern state transitions with proper checks and attribute updates. The use of
Object.assignon line 175 appropriately merges dynamic attributes when a pattern starts.
187-217: LGTM! Clean attribute initialization and cleanup.The
getChannelPowerAttributesmethod correctly creates default power channel attributes, andremovePatternAttributesAndDatasafely removes dynamic attributes using the typed keys utility.
219-253: LGTM! Robust message queue processing.The
processQueuedMessagesmethod correctly processes power status messages with proper safeguards:
- Limits processing to 10 messages per call to prevent blocking
- Safely handles missing attributes with optional chaining
- Updates both the current value and the max limit based on device feedback
src/device/device.ts (5)
5-14: LGTM! Sophisticated and well-designed type system.The type definitions provide strong typing for the attribute-based device model:
DeviceAttributesallows optional attributes viaundefinedAttributeValue<A>extracts the inner value type from an attributeDeviceData<T>creates a mapped type that represents attribute values as dataThis provides excellent type safety for device implementations.
17-44: LGTM! Clean generic class declaration.The
Deviceclass is now properly generic withT extends DeviceAttributes, allowing subclasses to define their own strongly-typed attribute containers. The field type updates are consistent with this change.
46-61: LGTM! Constructor correctly updated for generic attributes.The constructor properly accepts the generic
attributes: Tparameter and assigns it to the instance field, maintaining consistency with the new attribute model.
98-107: LGTM! Well-documented getAttribute implementation.The
getAttributemethod correctly returns the attribute object (not just its value), with clear documentation explaining whenundefinedmight be returned. The generic typing ensures type safety for the attribute key.
109-109: LGTM! Type-safe setAttribute signature.The abstract
setAttributemethod signature uses sophisticated generic constraints to ensure type safety:
K extends keyof Tensures valid attribute keysV extends AttributeValue<T[K]>extracts and constrains the value typePromise<V>return type allows implementations to return the set valueThis provides compile-time safety for all device implementations.
heavyrubberslave
changed the title
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
src/device/protocol/buttplugIo/buttplugIoWebsocketDeviceProvider.ts (1)
83-100: LGTM!The guard condition on line 84 correctly returns early when the client is undefined OR not connected, ensuring discovery only proceeds when properly connected. The added guard on lines 93-95 checking
isScanningbefore stopping is a good defensive addition.src/logging/PinoLogger.ts (1)
12-17: Critical: Child logger must be wrapped in PinoLogger to maintain interface contract.This issue from the previous review remains unresolved. The method returns a raw
pino.Logger, but the Logger interface expects the parameter ordertrace(msg, context), while pino's native API usestrace(context, msg). Lines 19-41 show that PinoLogger adapts this parameter order, but child loggers returned directly fromthis.pino.child()will not have this adaptation, causing runtime errors when callers use the Logger interface methods.🔎 Proposed fix
public child(bindings?: ChildLoggerBindings, options?: ChildLoggerOptions): Logger { - return this.pino.child( + return new PinoLogger(this.pino.child( bindings ?? {} as pino.Bindings, undefined === options ? undefined : options as pino.ChildLoggerOptions - ); + )); }Based on past review comments.
🧹 Nitpick comments (5)
src/device/protocol/buttplugIo/buttplugIoWebsocketDeviceProvider.ts (2)
58-62: Consider storing interval handles for cleanup.The
setIntervalcalls create timers that are never cleaned up. If this provider is expected to live for the entire application lifetime, this is acceptable. However, if there's any scenario where the provider could be destroyed or re-initialized, these intervals would leak.🔎 Proposed fix
private connectedDevices: Map<number, ButtplugIoDevice> = new Map(); private deviceUpdaters: Map<number, NodeJS.Timeout> = new Map(); +private reconnectInterval?: NodeJS.Timeout; +private scanInterval?: NodeJS.Timeout; // In init(): -setInterval(() => { void this.connectToServer() }, 3000); +this.reconnectInterval = setInterval(() => { void this.connectToServer() }, 3000); if (this.autoScan) { - setInterval(() => { this.discoverButtplugIoDevices() }, 60000); + this.scanInterval = setInterval(() => { this.discoverButtplugIoDevices() }, 60000); }Then add a
destroy()method to clear these intervals when needed.
65-81: LGTM with minor suggestion.The connection logic and error handling are correct. The guard on lines 66-69 properly skips connection when already connected or uninitialized.
Minor nit: The URL is constructed twice (line 42 and line 73). Consider storing it as an instance field or reusing
this.websocketAddressdirectly in the log message to avoid duplication.src/device/genericDeviceUpdater.ts (1)
29-29: Missing semicolon for consistency.Line 29 is missing a semicolon after
setAttribute, while other statements in the file use semicolons consistently.🔎 Proposed fix
- await device.setAttribute(attrKey, attrStr) + await device.setAttribute(attrKey, attrStr);src/logging/PinoLogger.ts (1)
15-15: Simplify the redundant ternary operator.Since
optionsis already optional and pino'schildmethod accepts an optional second parameter, the explicitundefinedcheck is unnecessary.🔎 Proposed fix
return this.pino.child( bindings ?? {} as pino.Bindings, - undefined === options ? undefined : options as pino.ChildLoggerOptions + options as pino.ChildLoggerOptions | undefined );src/device/attribute/floatDeviceAttribute.ts (1)
1-1: Add.jsextension for ESM consistency.Line 1 is missing the
.jsextension in the import path, while other imports (lines 2-3) include it. For consistency with ESM module resolution used throughout the codebase:🔎 Proposed fix
-import {DeviceAttributeModifier, NotJustUndefined} from "./deviceAttribute"; +import {DeviceAttributeModifier, NotJustUndefined} from "./deviceAttribute.js";
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
src/device/attribute/floatDeviceAttribute.tssrc/device/attribute/intDeviceAttribute.tssrc/device/genericDeviceUpdater.tssrc/device/protocol/buttplugIo/buttplugIoWebsocketDeviceProvider.tssrc/device/protocol/slvCtrlPlus/genericSlvCtrlPlusDevice.tssrc/device/protocol/slvCtrlPlus/slvCtrlPlusDeviceFactory.tssrc/device/protocol/zc95/zc95Device.tssrc/device/protocol/zc95/zc95DeviceFactory.tssrc/device/protocol/zc95/zc95SerialDeviceProvider.tssrc/logging/Logger.tssrc/logging/PinoLogger.tstests/unit/controller/getDevicesController.spec.tstests/unit/device/protocol/buttplugIo/buttplugIoDevice.spec.tstests/unit/device/protocol/slvCtrlPlus/genericSlvCtrlPlusDevice.spec.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/unit/device/protocol/buttplugIo/buttplugIoDevice.spec.ts
- src/device/protocol/zc95/zc95SerialDeviceProvider.ts
- tests/unit/controller/getDevicesController.spec.ts
- src/device/protocol/zc95/zc95DeviceFactory.ts
🧰 Additional context used
🧬 Code graph analysis (9)
tests/unit/device/protocol/slvCtrlPlus/genericSlvCtrlPlusDevice.spec.ts (3)
src/device/protocol/slvCtrlPlus/slvCtrlPlusDevice.ts (1)
SlvCtrlPlusDeviceAttributes(7-7)src/device/transport/deviceTransport.ts (1)
DeviceTransport(1-26)tests/unit/device/testDevice.ts (1)
TestDevice(3-27)
src/logging/Logger.ts (1)
src/device/transport/serialPortObserver.ts (1)
constructor(20-26)
src/device/attribute/intDeviceAttribute.ts (2)
src/util/numbers.ts (2)
Int(2-2)Int(5-14)src/device/attribute/intGenericDeviceAttribute.ts (2)
IntGenericDeviceAttribute(4-12)fromString(9-11)
src/logging/PinoLogger.ts (2)
src/logging/Logger.ts (3)
ChildLoggerBindings(1-3)ChildLoggerOptions(5-7)Logger(9-19)src/serviceProvider/loggerServiceProvider.ts (1)
LoggerServiceProvider(7-24)
src/device/attribute/floatDeviceAttribute.ts (3)
src/device/attribute/deviceAttribute.ts (3)
NotJustUndefined(4-4)value(53-55)value(57-59)src/util/numbers.ts (2)
Float(17-17)Float(20-29)src/device/attribute/numberDeviceAttribute.ts (1)
uom(23-25)
src/device/protocol/slvCtrlPlus/genericSlvCtrlPlusDevice.ts (3)
src/device/protocol/slvCtrlPlus/slvCtrlPlusDevice.ts (1)
SlvCtrlPlusDeviceAttributes(7-7)src/device/device.ts (2)
AttributeValue(8-8)DeviceAttributes(6-6)src/device/attribute/boolDeviceAttribute.ts (1)
BoolDeviceAttribute(6-32)
src/device/protocol/zc95/zc95Device.ts (6)
src/device/attribute/listDeviceAttribute.ts (2)
InitializedListDeviceAttribute(6-9)ListDeviceAttribute(11-73)src/util/numbers.ts (2)
Int(2-2)Int(5-14)src/device/attribute/boolDeviceAttribute.ts (1)
InitializedBoolDeviceAttribute(4-4)src/device/attribute/intRangeDeviceAttribute.ts (2)
IntRangeDeviceAttribute(9-81)InitializedIntRangeDeviceAttribute(7-7)src/device/device.ts (1)
AttributeValue(8-8)src/util/objects.ts (1)
getTypedKeys(3-5)
src/device/protocol/slvCtrlPlus/slvCtrlPlusDeviceFactory.ts (2)
src/device/protocol/slvCtrlPlus/slvCtrlPlusMessageParser.ts (1)
SlvCtrlPlusMessageParser(13-138)src/settings/knownDevice.ts (1)
serialNo(39-41)
src/device/genericDeviceUpdater.ts (2)
src/device/device.ts (1)
DeviceData(11-14)src/util/objects.ts (1)
getTypedKeys(3-5)
🔇 Additional comments (27)
src/device/protocol/buttplugIo/buttplugIoWebsocketDeviceProvider.ts (2)
16-17: LGTM!Making these fields optional correctly reflects that they're initialized in
init()rather than the constructor, improving type safety.
123-131: LGTM!Good defensive addition. The guard prevents potential runtime errors when a device isn't found in the map, which could occur during rapid disconnect/reconnect scenarios or race conditions. The warning log aids debugging without causing a crash.
src/device/genericDeviceUpdater.ts (2)
17-35: Approve the async/await refactor with improved attribute handling.The refactor to async/await is well-executed and improves maintainability. The use of
getTypedKeysprovides better type safety, and the per-attribute try-catch ensures that one failing attribute won't block updates to others. The previous concern about falsy checks has been improved by using an explicitundefinedcomparison instead of a negation operator.
20-20: Current implementation is correct; null check is unnecessary.The
getAttribute()method has a return type ofPromise<T[K] | undefined>(seesrc/device/device.ts:104), meaning it returns either the attribute value orundefined— nevernull. The current check forundefined === await device.getAttribute(attrKey)is appropriate and correctly handles the method's actual contract.The previous improvement from a falsy check to an explicit
undefinedcheck was the right approach, properly distinguishing between non-existent attributes and falsy-but-valid attribute values like0,false, and empty strings.src/logging/Logger.ts (1)
1-11: LGTM! The new child logger API resolves the parameter order mismatch.The introduction of typed
ChildLoggerBindingsandChildLoggerOptionsprovides better type safety than the previous(config?: object, context?: unknown)signature, and the parameter order now matches between the Logger interface and PinoLogger implementation, resolving the critical issue from the previous review. The usage pattern insrc/device/transport/serialPortObserver.tsconfirms the API works as intended.Based on past review comments.
src/device/protocol/zc95/zc95Device.ts (8)
18-35: Well-structured type definitions for the new attribute model.The type hierarchy correctly models the device's attribute structure with required base attributes and optional dynamic attributes for power channels and patterns.
48-70: Constructor properly updated for typed attributes.The addition of
@Exposedecorator onfwVersionand the updated constructor signature align with the new attribute-centric model.
76-100: Type-safe setAttribute implementation.The generic signature ensures compile-time type safety for attribute names and values, and the branching logic correctly handles all attribute types.
102-116: Proper type-guarded attribute handling.The use of type guards ensures safe access to attribute-specific properties, and value updates occur after successful transport calls.
155-167: Correct pattern switching logic.The method properly handles state transitions by stopping the current pattern before switching and uses
Int.fromfor type safety.
169-190: Dynamic attribute management implemented correctly.The method properly fetches pattern details and dynamically adds attributes using
Object.assign, ensuring the attribute model reflects the active pattern's requirements.
213-222: Type-safe attribute cleanup.The use of
getTypedKeysensures type-safe iteration while dynamically removing pattern and power channel attributes.
295-312: Well-implemented type guards.The type guard functions correctly narrow types using runtime checks, enabling type-safe attribute and message handling.
src/device/attribute/intDeviceAttribute.ts (3)
1-7: LGTM!The imports and type definitions are well-structured and follow the established pattern for device attributes.
8-27: LGTM!The static factory methods provide a clean API for creating integer attributes with or without initial values, maintaining type safety throughout.
29-37: LGTM!The
fromStringimplementation correctly parses integers with proper validation. The use ofInt.from()ensures type-safe conversion to the brandedInttype, matching the pattern established in the codebase.src/device/attribute/floatDeviceAttribute.ts (3)
5-18: LGTM!The type definitions and constructor implementation are clean and follow the established attribute pattern.
20-37: LGTM!The static factory methods provide a clean API consistent with other attribute classes in this refactoring.
39-47: Excellent fix - proper type-safe Float conversion implemented!The previous review concern about bypassing Float validation has been resolved. Line 46 now correctly uses
Float.from(num), which validates that the number is finite and not NaN before returning the brandedFloattype. This ensures type safety and data integrity.tests/unit/device/protocol/slvCtrlPlus/genericSlvCtrlPlusDevice.spec.ts (2)
1-22: LGTM!The test setup with the
createDevicehelper function is well-structured and provides a clean way to instantiate devices for testing with different attribute configurations.
24-36: LGTM!The test correctly validates error handling when attempting to set a non-existent attribute. The test structure follows best practices with clear arrange/act/assert sections.
src/device/protocol/slvCtrlPlus/slvCtrlPlusDeviceFactory.ts (2)
38-61: LGTM!The device creation logic correctly handles the new numeric
fwVersiontype and map-baseddeviceAttrsstructure, aligning with the broader attribute refactoring.
63-80: LGTM!The refactoring to use the parameterized
KnownDeviceconstructor is cleaner and more maintainable. The change fromnulltoundefinedchecks is also more idiomatic for TypeScript.src/device/protocol/slvCtrlPlus/genericSlvCtrlPlusDevice.ts (4)
1-7: LGTM!All import statements now correctly include
.jsextensions, maintaining consistency with ESM module resolution throughout the codebase. The previous review concern has been addressed.
9-40: LGTM!The constructor signature correctly reflects the new numeric
fwVersiontype and map-basedSlvCtrlPlusDeviceAttributesstructure, maintaining consistency with the broader refactoring.
42-64: LGTM!The
refreshDatamethod now properly handles the map-based attributes with appropriateundefinedchecks and uses attribute-specificfromStringmethods for type-safe parsing. The previous concern aboutnullassignment has been resolved.
106-118: LGTM!The accessor methods are straightforward and correctly expose the device's attributes and configuration values.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/device/transport/serialDeviceTransport.ts (1)
24-38: Previous concern addressed; implementation looks solid.The undefined handling issue from the previous review has been resolved. The code now properly guards against undefined values by checking that all three optional fields (
vendorId,productId,locationId) are defined before constructing the composite identifier. This ensures no "undefined" strings appear in device identifiers.The three-tier fallback strategy (serialNumber → full composite → path) is sound and provides good coverage for various hardware/driver scenarios.
💡 Optional enhancement: partial composite fallback
Consider using a partial composite identifier when only some fields are available (e.g.,
vendorIdandproductIdbut notlocationId). This could provide more meaningful identifiers than falling back topathon systems where the path might change across reboots:public getDeviceIdentifier(): string { const portInfo = this.serialPort.getPortInfo(); if (undefined !== portInfo.serialNumber) { return portInfo.serialNumber; } // Fall back to some (hopefully) unique combination if serial number is missing if (undefined !== portInfo.vendorId && undefined !== portInfo.productId && undefined !== portInfo.locationId) { return `${portInfo.vendorId}-${portInfo.productId}-${portInfo.locationId}`; } + // Partial composite if we have vendorId and productId + if (undefined !== portInfo.vendorId && undefined !== portInfo.productId) { + return `${portInfo.vendorId}-${portInfo.productId}`; + } + // If only the bare minimum is available return portInfo.path; }Note: This is optional—the current implementation is correct and may be preferable if
pathstability is sufficient on your target platforms.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/device/protocol/slvCtrlPlus/genericSlvCtrlPlusDevice.ts (1)
96-100: Verify: Device response handling differs from past review recommendation.The past review recommended returning the input
valuedirectly (like other device implementations), but the current code parses the device response and returns that:const result = await this.send(`set-${attributeName.toString()} ${valueToSend.toString()}`); attr.value = attr.fromString(result); return attr.value as V;If the device echoes back a different format or value, this could cause inconsistency. Other device implementations (ButtplugIoDevice, Zc95Device, VirtualDevice) set the attribute to the input value and return it directly.
Verify whether the SlvCtrl protocol's
set-*response should be authoritative, or if the input value should be returned for consistency.#!/bin/bash # Compare setAttribute pattern across device implementations echo "=== GenericSlvCtrlPlusDevice pattern ===" rg -n "attr\.value.*=.*attr\.fromString" src/device/protocol/slvCtrlPlus/ --type=ts -B2 -A2 echo -e "\n=== Other device implementations ===" rg -n "setAttribute" src/device/protocol/virtual/virtualDevice.ts src/device/protocol/zc95/zc95Device.ts src/device/protocol/buttplugIo/buttplugIoDevice.ts --type=ts -A10 | head -80src/device/protocol/zc95/zc95Device.ts (1)
258-258: Resolve the field selection uncertainty.The comment
// or channel.OutputPower?indicates unresolved uncertainty about which field to use from the PowerStatusMessage. This should be clarified by consulting the device protocol specification or documentation, and the comment removed once the correct field is confirmed.
🧹 Nitpick comments (2)
src/device/attribute/deviceAttribute.ts (1)
62-65: Consider makinggetType()abstract for consistency.Lines 67 and 69 define
fromStringandisValidValueas abstract methods, which enforces implementation at compile time. ThegetType()method serves the same purpose (all concrete attribute classes override it), but currently throws at runtime instead of being abstract.🔎 Proposed refactor
@Expose({ name: "type" }) - public getType(): string { - throw new Error(`Not implemented`); - } + public abstract getType(): string;src/device/attribute/intRangeDeviceAttribute.ts (1)
72-80: UseInt.from()for consistency with the branded type pattern.Line 79 directly casts the parsed integer to
Twithout usingInt.from(), which is inconsistent withFloatDeviceAttribute(line 46) that usesFloat.from(). WhileparseIntguarantees an integer result, usingInt.from()ensures proper validation and maintains consistency with the branded type system.🔎 Proposed fix
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}`); } - return res as T; + return Int.from(res) as T; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
package.jsonsrc/device/attribute/boolDeviceAttribute.tssrc/device/attribute/deviceAttribute.tssrc/device/attribute/floatDeviceAttribute.tssrc/device/attribute/intDeviceAttribute.tssrc/device/attribute/intRangeDeviceAttribute.tssrc/device/attribute/listDeviceAttribute.tssrc/device/attribute/strDeviceAttribute.tssrc/device/protocol/slvCtrlPlus/genericSlvCtrlPlusDevice.tssrc/device/protocol/zc95/zc95Device.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/device/attribute/boolDeviceAttribute.ts
- src/device/attribute/intDeviceAttribute.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-26T21:24:04.798Z
Learnt from: heavyrubberslave
Repo: SlvCtrlPlus/slvctrlplus-server PR: 54
File: src/device/protocol/zc95/zc95Device.ts:192-211
Timestamp: 2025-12-26T21:24:04.798Z
Learning: In ZC95Device power channel attributes (src/device/protocol/zc95/zc95Device.ts), the minimum value is always 0 (Int.ZERO) by design, representing zero power output. Only the maximum value is updated from PowerStatusMessage. This asymmetric range initialization is intentional and correct.
Applied to files:
src/device/protocol/zc95/zc95Device.tssrc/device/attribute/intRangeDeviceAttribute.ts
🧬 Code graph analysis (5)
src/device/attribute/floatDeviceAttribute.ts (3)
src/device/attribute/deviceAttribute.ts (3)
NotJustUndefined(4-4)value(50-52)value(54-56)src/util/numbers.ts (2)
Float(17-17)Float(20-29)src/device/attribute/numberDeviceAttribute.ts (1)
uom(23-25)
src/device/attribute/deviceAttribute.ts (1)
src/util/numbers.ts (4)
Int(2-2)Int(5-14)Float(17-17)Float(20-29)
src/device/protocol/zc95/zc95Device.ts (6)
src/device/attribute/listDeviceAttribute.ts (2)
InitializedListDeviceAttribute(6-9)ListDeviceAttribute(11-77)src/util/numbers.ts (2)
Int(2-2)Int(5-14)src/device/attribute/boolDeviceAttribute.ts (1)
InitializedBoolDeviceAttribute(4-4)src/device/attribute/intRangeDeviceAttribute.ts (2)
IntRangeDeviceAttribute(9-85)InitializedIntRangeDeviceAttribute(7-7)src/device/device.ts (1)
AttributeValue(8-8)src/util/objects.ts (1)
getTypedKeys(3-5)
src/device/attribute/strDeviceAttribute.ts (1)
src/device/attribute/deviceAttribute.ts (7)
NotJustUndefined(4-4)name(35-37)label(39-41)modifier(43-45)value(50-52)value(54-56)NotUndefined(5-5)
src/device/attribute/intRangeDeviceAttribute.ts (3)
src/util/numbers.ts (2)
Int(2-2)Int(5-14)src/device/attribute/intDeviceAttribute.ts (1)
IntAttributeValue(5-5)src/device/attribute/deviceAttribute.ts (5)
name(35-37)label(39-41)modifier(43-45)value(50-52)value(54-56)
🔇 Additional comments (25)
src/device/protocol/slvCtrlPlus/genericSlvCtrlPlusDevice.ts (7)
1-7: Imports look good.All imports now include the
.jsextension for ESM compatibility, addressing the previous review comment.
19-19: LGTM: fwVersion type change.The type change from
stringtonumberis consistent across the property declaration, constructor parameter, and assignment.Also applies to: 25-25, 38-38
70-82: LGTM: Comprehensive validation logic.The attribute existence checks, null/undefined guards, and value validation provide good defensive programming with clear error messages.
87-94: LGTM: Boolean conversion for protocol.The boolean-to-numeric conversion (1=true, 0=false) is correctly implemented with appropriate type checking and a clear comment explaining the protocol requirement.
106-109: LGTM: Method renamed consistently.The
getAttributes()method properly exposes the device attributes, aligning with the broader refactoring from array-based to keyed attribute structure.
111-117: LGTM: Timeout and interval getters.The serial timeout and refresh interval methods are straightforward and correctly implemented.
62-62: No action needed on line 62.
AttributeValueis defined asNotJustUndefined<string | Int | Float | boolean | null | undefined>insrc/device/attribute/deviceAttribute.ts, which explicitly includesundefined. AssigningundefinedwhendataObj[attrKey]is an empty string is type-safe and valid.Likely an incorrect or invalid review comment.
src/device/attribute/strDeviceAttribute.ts (1)
1-36: LGTM! Clean implementation of string attribute type.The
StrDeviceAttributeclass follows the established attribute pattern with appropriate factory methods, type checking, and string handling. The implementation is straightforward and type-safe.src/device/attribute/deviceAttribute.ts (1)
1-74: Strong foundation for the attribute system.The base
DeviceAttributeclass provides a well-designed abstraction with:
- Clear type utilities (
NotJustUndefined,NotUndefined)- Appropriate serialization decorators
- Proper encapsulation with getters/setters
- Abstract contract for concrete implementations
src/device/attribute/floatDeviceAttribute.ts (1)
39-47: LGTM! Proper use ofFloat.from()for type-safe conversion.The
fromStringmethod correctly validates the parsed number and usesFloat.from(num)to ensure the value is finite and properly branded as aFloattype. This addresses the previous review concern and aligns with the branded type pattern.src/device/attribute/intRangeDeviceAttribute.ts (1)
1-85: Well-designed range attribute implementation.The
IntRangeDeviceAttributeclass provides comprehensive range management with:
- Proper serialization decorators for min/max/incrementStep
- Type-safe factory methods
- Clear getters/setters for range bounds
src/device/attribute/listDeviceAttribute.ts (1)
1-77: LGTM! Flexible and type-safe list attribute implementation.The
ListDeviceAttributeclass provides:
- Flexible support for both string and Int keys/values
- Proper validation via
isValidValueagainst the permitted values map- Type-safe factory methods with generics
- Clean serialization support
The
fromStringimplementation appropriately handles both integer and string inputs, with runtime validation inisValidValueensuring only permitted values are accepted.src/device/protocol/zc95/zc95Device.ts (13)
1-17: LGTM - Imports support attribute-centric refactor.The new imports properly support the refactoring from data-centric to attribute-centric model. The addition of
typeDetectfor error messages (used on line 121) andgetTypedKeysfor type-safe iteration (used on line 222) are good improvements.
19-36: LGTM - Well-structured attribute types.The type hierarchy properly models the device state with required base attributes and optional dynamic attributes (power channels and pattern attributes). The use of
PartialandRequiredcorrectly expresses the attribute lifecycle.
56-71: LGTM - Constructor properly updated.The constructor signature correctly reflects the new attribute-centric model. The addition of
fwVersionas a serializable field with@Expose()is appropriate.
73-77: LGTM - Correctly handles synchronous processing.The method appropriately calls the synchronous
processQueuedMessages()withoutawaitand returnsPromise.resolve()to satisfy thePromise<void>return type required by the parent class.
79-106: LGTM - Type-safe attribute setter.The generic signature properly constrains the value type based on the attribute key, ensuring type safety. The routing logic covers all attribute types with a clear error for unhandled cases.
108-124: LGTM - Robust pattern detail handling.The method properly validates attribute types and values before updating transport, with excellent error messages using
typeDetect. The flow correctly updates the attribute value after successful transport communication.
153-160: LGTM - Proper type guard implementation.The type predicate correctly narrows the partial power channel attributes to fully initialized attributes, ensuring type safety for operations that require all channels to be defined.
163-175: LGTM - Correct pattern switching logic.The method properly handles the pattern lifecycle by ensuring the current pattern is stopped before switching to a new one. The early return optimization avoids unnecessary operations.
177-198: LGTM - Comprehensive pattern lifecycle management.The method correctly handles both pattern start and stop operations, including dynamic attribute creation/removal and proper transport communication. The use of
Object.assignto merge attributes is appropriate.
221-230: LGTM - Type-safe attribute cleanup.The use of
getTypedKeysfor type-safe iteration over attribute keys is excellent. The cleanup logic correctly removes both pattern and power channel attributes.
232-262: Good refactor - Promise anti-pattern resolved.The method has been correctly refactored from the Promise constructor anti-pattern to a synchronous
voidreturn. However, the comment on line 258 still indicates uncertainty about field selection.
264-295: LGTM - Clean attribute creation from pattern details.The method properly creates typed attributes using
createInitialized, which sets initial values during construction. The previous issue with redundant value assignment has been resolved. Nice attention to detail with the unit of measure conversion on line 277 ('us'→'µs').
297-313: LGTM - Standard type guards.The type predicates follow standard TypeScript patterns and provide proper type narrowing for the various discriminated unions and attribute types.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/device/provider/deviceProviderFactory.ts (1)
4-7: The interface change is incomplete—three implementations do not match the updated signature.The interface now requires
create(config: JsonObject): DeviceProvider, but:
SlvCtrlPlusSerialDeviceProviderFactory.create()takes no parameterZc95SerialDeviceProviderFactory.create()takes no parameterVirtualDeviceProviderFactory.create()takes no parameterOnly
ButtplugIoWebsocketDeviceProviderFactoryhas the correct signature. Update the three mismatched implementations to accept theJsonObjectconfig parameter to match the interface and match the call site indeviceProviderLoader.ts(line 48).
♻️ Duplicate comments (1)
package.json (1)
29-29: Critical: vm2 remains deprecated despite version bump.Updating
vm2from^3.9.14to^3.10.0does not resolve the fundamental issue that the package has been deprecated by its maintainers due to multiple critical sandbox-escape vulnerabilities. Even if version 3.10.0 exists and patches some CVEs, the package is no longer maintained.The maintainers explicitly recommend migrating to
isolated-vmor other alternatives. Continuing to usevm2poses significant security risks.Verify the current status of vm2 3.10.0 and confirm the deprecation:
#!/bin/bash # Check vm2 npm registry data including deprecation status echo "=== vm2 package status ===" npm view vm2@3.10.0 version deprecated 2>/dev/null || echo "Version 3.10.0 not found" echo -e "\n=== Latest vm2 version and deprecation status ===" npm view vm2 dist-tags.latest deprecated echo -e "\n=== Recent vm2 versions ===" npm view vm2 versions --json | jq '.[-10:]' # Check for security advisories echo -e "\n=== Security advisories for vm2 ===" gh api graphql -f query=' { securityVulnerabilities(first: 10, ecosystem: NPM, package: "vm2") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }'Is vm2 npm package still deprecated in 2025? What is the latest version?
🧹 Nitpick comments (4)
src/schemaValidation/JsonSchemaValidator.ts (1)
24-26: Consider applying consistent null-safety pattern.Line 21 uses the nullish coalescing operator for safety, but line 25 passes
this.schemaValidator.errorsdirectly toerrorsText(). While AJV likely handles undefined gracefully, applying the same pattern would be more consistent.🔎 Suggested consistency improvement
public getValidationErrorsAsText(): string { - return this.ajv.errorsText(this.schemaValidator.errors); + return this.ajv.errorsText(this.schemaValidator.errors ?? []); }src/device/protocol/zc95/zc95Device.ts (1)
233-263: Method now correctly returns void, addressing previous review feedback.The Promise constructor anti-pattern has been resolved—the method now returns
voidand executes synchronously. The logic correctly processes queued power status messages and updates attribute values and limits.Optional clarification: Line 259's comment
// or channel.OutputPower?indicates uncertainty about the power field selection. If this is resolved, consider removing the comment or documenting whyMaxOutputPoweris the correct choice.🔎 Optional: Clarify power field selection
If
MaxOutputPoweris confirmed as the correct field:- channelAttr.value = Int.from(Math.floor(channel.MaxOutputPower * 0.1)) // or channel.OutputPower? + channelAttr.value = Int.from(Math.floor(channel.MaxOutputPower * 0.1))Or document why this field is used if there's a specific reason.
src/device/protocol/virtual/virtualDevice.ts (1)
48-67: Consider refactoring to use async/await instead of the Promise constructor.The
setAttributemethod wraps synchronous operations innew Promise, which is an anti-pattern when no actual asynchronous work is performed. This can be simplified using async/await syntax for better readability and maintainability.🔎 Proposed refactor using async/await
-public async setAttribute<K extends keyof T, V extends AttributeValue<T[K]>>(attributeName: K, value: V): Promise<V> { - return new Promise<V>((resolve, reject) => { - this.state = DeviceState.busy; - - const attribute = this.attributes[attributeName]; - - if (undefined === attribute) { - reject(new Error( - `Attribute named "${attributeName.toString()}" does not exist for device with id "${this.deviceId}"` - )); - return; - } - - attribute.value = value; - - this.state = DeviceState.ready; - - resolve(value); - }); -} +public async setAttribute<K extends keyof T, V extends AttributeValue<T[K]>>(attributeName: K, value: V): Promise<V> { + this.state = DeviceState.busy; + + const attribute = this.attributes[attributeName]; + + if (undefined === attribute) { + throw new Error( + `Attribute named "${attributeName.toString()}" does not exist for device with id "${this.deviceId}"` + ); + } + + attribute.value = value; + + this.state = DeviceState.ready; + + return value; +}src/device/protocol/virtual/audio/ttsVirtualDeviceLogic.ts (1)
38-38: Consider simplifying the null check.The condition checks for both
undefinedandnull, but based on theStrDeviceAttributetype, the value should bestring | undefined, making thenullcheck unnecessary. This might be legacy code from the previous implementation.🔎 Proposed simplification
- if (undefined !== text && null !== text) { + if (undefined !== text) {
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (13)
package.jsonsrc/controller/settings/putSettingsController.tssrc/device/protocol/virtual/audio/ttsVirtualDeviceLogic.tssrc/device/protocol/virtual/randomGenerator/randomGeneratorVirtualDeviceLogic.tssrc/device/protocol/virtual/virtualDevice.tssrc/device/protocol/zc95/zc95Device.tssrc/device/provider/deviceProviderFactory.tssrc/schemaValidation/JsonSchemaValidator.tssrc/schemaValidation/JsonSchemaValidatorFactory.tssrc/settings/deviceSource.tssrc/settings/knownDevice.tssrc/settings/settingsManager.tssrc/types.d.ts
✅ Files skipped from review due to trivial changes (1)
- src/controller/settings/putSettingsController.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/settings/deviceSource.ts
- src/device/protocol/virtual/randomGenerator/randomGeneratorVirtualDeviceLogic.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-26T21:24:04.798Z
Learnt from: heavyrubberslave
Repo: SlvCtrlPlus/slvctrlplus-server PR: 54
File: src/device/protocol/zc95/zc95Device.ts:192-211
Timestamp: 2025-12-26T21:24:04.798Z
Learning: In ZC95Device power channel attributes (src/device/protocol/zc95/zc95Device.ts), the minimum value is always 0 (Int.ZERO) by design, representing zero power output. Only the maximum value is updated from PowerStatusMessage. This asymmetric range initialization is intentional and correct.
Applied to files:
src/device/protocol/zc95/zc95Device.ts
🧬 Code graph analysis (4)
src/schemaValidation/JsonSchemaValidator.ts (2)
src/serviceProvider/schemaValidationServiceProvider.ts (2)
SchemaValidationServiceProvider(7-23)Ajv(10-15)src/serviceProvider/settingsServiceProvider.ts (1)
jsonSchemaValidatorFactory(13-20)
src/device/protocol/virtual/audio/ttsVirtualDeviceLogic.ts (5)
src/device/attribute/strDeviceAttribute.ts (1)
StrDeviceAttribute(6-36)src/device/attribute/boolDeviceAttribute.ts (1)
BoolDeviceAttribute(6-36)src/device/attribute/intDeviceAttribute.ts (1)
IntDeviceAttribute(8-42)src/device/protocol/virtual/virtualDeviceLogic.ts (1)
VirtualDeviceLogic(4-11)src/util/numbers.ts (2)
Int(2-2)Int(5-14)
src/device/protocol/virtual/virtualDevice.ts (3)
src/device/device.ts (2)
DeviceAttributes(6-6)AttributeValue(8-8)src/device/protocol/virtual/virtualDeviceLogic.ts (1)
VirtualDeviceLogic(4-11)src/device/attribute/deviceAttribute.ts (3)
AttributeValue(6-6)value(50-52)value(54-56)
src/device/protocol/zc95/zc95Device.ts (7)
src/device/attribute/listDeviceAttribute.ts (2)
InitializedListDeviceAttribute(6-9)ListDeviceAttribute(11-77)src/device/attribute/boolDeviceAttribute.ts (1)
InitializedBoolDeviceAttribute(4-4)src/device/attribute/intRangeDeviceAttribute.ts (2)
IntRangeDeviceAttribute(9-85)InitializedIntRangeDeviceAttribute(7-7)src/types.d.ts (1)
AllOrNone(11-11)src/device/protocol/zc95/Zc95Messages.ts (2)
Zc95Messages(75-282)MsgResponse(4-10)src/device/device.ts (1)
AttributeValue(8-8)src/util/objects.ts (1)
getTypedKeys(3-5)
🔇 Additional comments (21)
src/types.d.ts (1)
11-11: LGTM! Useful utility type.The
AllOrNone<T>type correctly implements an "all or nothing" constraint pattern, allowing either a complete T or an empty object.src/schemaValidation/JsonSchemaValidatorFactory.ts (1)
4-4: Good addition for type safety.The JsonObject import and usage at line 16 appropriately adds type information to the parsed schema data without changing runtime behavior.
Also applies to: 16-16
src/schemaValidation/JsonSchemaValidator.ts (2)
3-3: LGTM!The JsonObject import adds appropriate typing for the validate method parameter.
21-21: Good defensive programming.Using nullish coalescing ensures
getValidationErrors()always returns an array, even whenthis.schemaValidator.errorsis undefined.src/settings/settingsManager.ts (3)
12-12: LGTM!JsonObject import supports type-safe handling of parsed settings data.
119-123: LGTM! Simplified construction.The inline DeviceSource instantiation is more concise and maintains the same functionality.
23-23: The undefined handling is properly implemented across all callers.The refactor from
nulltoundefinedsemantics is correctly implemented.getSettings()returnsSettings|undefinedas expected, and the one identified caller insrc/device/protocol/virtual/virtualDeviceProvider.ts(line 44) properly checks for undefined before using the settings object. The internalsave()method also correctly guards against undefined values.src/settings/knownDevice.ts (2)
36-58: LGTM! Clean getter implementation.The public getters provide read-only access to all encapsulated fields, maintaining the expected API surface while enforcing immutability.
7-34: Good encapsulation with private readonly fields and explicit getters.The refactor to private readonly fields with explicit getters provides proper encapsulation and makes KnownDevice immutable after construction. The constructor enables clean initialization and the
@Exposedecorators maintain serialization compatibility. Verification confirms no code attempts to set properties after construction, so there are no breaking mutations.src/device/protocol/zc95/zc95Device.ts (10)
1-37: LGTM! Well-structured attribute type definitions.The imports and type definitions correctly model the attribute-centric refactoring. The use of
AllOrNonefor power channel attributes ensures they're either fully present or completely omitted, which aligns with the deferred initialization pattern used elsewhere in the code.
57-78: LGTM! Constructor and refreshData correctly structured.The constructor properly initializes the attribute-centric model, and
refreshDatacorrectly callsprocessQueuedMessages()synchronously (since it now returnsvoidrather thanPromise<void>), then returnsPromise.resolve()for interface consistency.
80-107: LGTM! Type-safe attribute setter with proper routing.The generic signature with
AttributeValue<Zc95DeviceAttributes[K]>ensures type safety across different attribute types. The routing logic correctly handles all attribute categories with appropriate type guards.
109-125: LGTM! Properly handles pattern detail attribute updates.The method correctly distinguishes between
IntRangeDeviceAttributeandListDeviceAttributetypes, validates values, and updates the transport before modifying the attribute state.
127-161: LGTM! Power channel updates correctly guard against partial initialization.The silent return when power channels aren't fully initialized (lines 131-133) is intentional per previous discussion. The type guard
allPowerChannelValuesDefinedproperly narrows the type, and the temporary data object ensures atomic transport updates.Based on learnings, the minimum value for power channels is always 0 by design.
164-199: LGTM! Pattern lifecycle methods properly orchestrated.The methods correctly handle state transitions:
setAttributeActivePatternstops any running pattern before switching, andsetAttributePatternStartedsynchronizes attribute state with transport operations. The use ofObject.assignto merge dynamically created attributes is appropriate for this use case.
201-220: LGTM! Power channel attribute initialization follows deferred pattern.The method creates uninitialized attributes (with
undefinedvalues) that are populated later viaprocessQueuedMessages(). The asymmetric range initialization (min alwaysInt.ZERO, max updated from messages) is intentional and correct per the design.Based on learnings, the minimum power value is always 0 by design, representing zero power output.
222-231: LGTM! Type-safe attribute cleanup.The use of
getTypedKeysensures type-safe iteration when removing dynamically created attributes. The logic correctly identifies and removes both pattern detail and power channel attributes.
265-296: LGTM! Pattern attributes correctly initialized from device menu items.The method properly creates initialized attributes for both range (min/max) and list (multi-choice) menu items. The redundant value assignment mentioned in previous review has been removed—values are now set only during
createInitializedcalls.The unit of measure conversion (
'us' → 'µs') is a nice touch for proper display.
298-314: LGTM! Type guards provide proper type narrowing.The type predicate methods are correctly implemented and enable type-safe handling of different attribute types, message types, and menu item types throughout the class.
src/device/protocol/virtual/audio/ttsVirtualDeviceLogic.ts (2)
67-74: Verify callback synchronization with async flow.The
say.speakcallback updates thespeakingattribute using a fire-and-forget pattern (void device.setAttribute(...)). This means the attribute update happens asynchronously after therefreshDatamethod completes, which could lead to race conditions ifrefreshDatais called again before the callback executes. Verify this is the intended behavior and won't cause state inconsistencies.
81-114: LGTM! Clean refactor to object-based attributes.The refactoring from array-based to object-based attributes with typed properties is well-implemented. The use of static factory methods and proper initialization provides good type safety and clarity.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/device/protocol/virtual/audio/ttsVirtualDeviceLogic.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/device/protocol/virtual/audio/ttsVirtualDeviceLogic.ts (5)
src/device/attribute/strDeviceAttribute.ts (1)
StrDeviceAttribute(6-36)src/device/attribute/boolDeviceAttribute.ts (1)
BoolDeviceAttribute(6-36)src/device/attribute/intDeviceAttribute.ts (1)
IntDeviceAttribute(8-42)src/device/protocol/virtual/virtualDeviceLogic.ts (1)
VirtualDeviceLogic(4-11)src/util/numbers.ts (2)
Int(2-2)Int(5-14)
🔇 Additional comments (6)
src/device/protocol/virtual/audio/ttsVirtualDeviceLogic.ts (6)
1-10: LGTM! Imports aligned with typed attribute system.The imports properly support the refactoring to strongly-typed device attributes, including the branded
Inttype and specific attribute classes.
11-16: LGTM! Type definition provides strong typing.The
TtsVirtualDeviceAttributestype properly constrains the attribute structure and enables type-safe access throughout the class.
34-36: LGTM! Previous unsafe attribute access resolved.The code now correctly uses optional chaining (
?.value) instead of non-null assertions, with appropriate default values for boolean attributes. This addresses the concern raised in previous reviews.
38-45: LGTM! Text clearing properly usesundefined.Setting the text attribute to
undefined(line 44) correctly clears the write-only attribute after processing. This resolves the previous review concern about incompatiblenullassignment.
60-65: Good defensive programming.The check for
undefinedaftershift()is good practice, even though the queue-empty check at lines 47-49 should typically prevent this case.
81-114: LGTM! Attribute configuration is well-structured.The method properly:
- Assigns appropriate modifiers (writeOnly for text, readOnly for speaking/queueLength, readWrite for queuing)
- Initializes attributes with sensible defaults (false for booleans, Int.ZERO for queue length)
- Returns a correctly typed object matching
TtsVirtualDeviceAttributes
Belongs to: SlvCtrlPlus/slvctrlplus-server#54 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added speed control UI for Striker Mk2 devices (debounced slider). * Added display for virtual random generator values. * **Bug Fixes** * Safer handling of missing/invalid device properties to avoid NaN/undefined issues. * Distance readings show a placeholder when unavailable. * Automation form now emits cancel; log viewer shows “currently not running” and emits close reliably. * **Improvements** * Dependency updates (Vuetify, Vite). * Charts handle gaps and tooltips more robustly. * Consolidated device attribute handling for more consistent UI. <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
1 participant
Fixes #53
Summary by CodeRabbit
Refactor
Bug Fixes / Reliability
New Features
✏️ Tip: You can customize this high-level summary in your review settings.