Uh oh!
There was an error while loading. Please reload this page.
Add piper virtual device - #59
Conversation
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds Piper TTS virtual device and TypeBox configs; refactors virtual-device abstractions to support per-type logic+config and validation; extends Device with config and errorInfo; adds process spawn and dev-null utilities; updates CI matrix, dependencies, and linting. Changes
Sequence Diagram(s)sequenceDiagram
actor Device as VirtualDevice
participant Logic as PiperVirtualDeviceLogic
participant Process as Piper (child_process)
participant Speaker as Speaker
Device->>Logic: refreshData()
activate Logic
Logic->>Logic: ensure Piper running
alt Piper not running
Logic->>Process: spawn piper (--model, --output-raw)
Process-->>Logic: ChildProcess (stdin/stdout/stderr)
Logic->>Speaker: instantiate Speaker(params)
Logic->>Process: stdout.pipe(Speaker)
end
Logic->>Logic: manage queue & cooldown, read text attribute
alt text available
Logic->>Process: stdin.write(text + '\n')
Logic->>Logic: update playback state
else stop requested
Logic->>Logic: stop playback / drain stdout
end
alt spawn or IO error
Logic->>Device: set state = ERROR, set errorInfo
end
deactivate Logic
sequenceDiagram
participant Factory as GenericVirtualDeviceFactory
participant Validator as JsonSchemaValidator
participant LogicCtor as DeviceLogicCtor
participant DeviceClass as VirtualDevice
Factory->>Factory: create(knownDevice)
alt validator exists
Factory->>Validator: validate(config)
alt invalid
Validator-->>Factory: errors → throw
end
end
Factory->>LogicCtor: new Logic(validatedConfig, logger)
LogicCtor-->>Factory: deviceLogic instance
Factory->>DeviceClass: construct VirtualDevice(..., deviceLogic)
DeviceClass-->>Factory: VirtualDevice instance
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/release.yml (1)
9-85: Race condition: Each matrix job creates the same tag and release concurrently.With the matrix strategy, all 5 jobs run in parallel and each attempts to:
- Create the same version tag (Line 36)
- Create/update the same GitHub release (Line 78)
This will cause race conditions - jobs may fail when trying to create an already-existing tag, or artifacts may be partially uploaded.
Consider splitting into two jobs: a single job to tag and create the release, followed by a matrix job (with
needs:) that builds and uploads artifacts.🔎 Suggested approach
jobs: create-release: runs-on: ubuntu-latestif: github.event.pull_request.merged && (...)outputs: tag: ${{ steps.set_tag.outputs.tag }}steps: - name: Checkout Codeuses: actions/checkout@v4 - name: Tagid: set_taguses: K-Phoen/semver-release-action@v1.3.2... - name: Create releaseuses: ncipollo/release-action@v1with: tag: ${{ steps.set_tag.outputs.tag }}...build-artifacts: needs: create-releasestrategy: matrix: include: - os: windows-latestarch: x64# ... other combinationsruns-on: ${{ matrix.os }}steps: # Build steps... - name: Upload artifact to releaseuses: ncipollo/release-action@v1with: tag: ${{ needs.create-release.outputs.tag }}allowUpdates: trueartifacts: "dist-${{ env.PLATFORM }}-${{ matrix.arch }}.tar.gz"
🤖 Fix all issues with AI Agents
In @package.json:
- Line 7: The package version for @sinclair/typebox is set to a non-existent
release (^0.34.46); update the dependency entry for "@sinclair/typebox" in
package.json to "^0.34.45" and then regenerate your lockfile (run npm install or
yarn install) so the lockfile reflects the valid version.
- Around line 31-34: Replace the GitHub fork reference in package.json
optionalDependencies by using the official npm packages: change "speaker":
"github:JasonBenn/node-speaker" to a valid npm spec (e.g., "speaker": "^0.5.5")
and keep "speaker-arm64": "^0.5.2"; alternatively, if you need JasonBenn’s
CoreAudio fixes, replace the GitHub URL with the npm-scoped package
"@mastra/node-speaker" instead of "github:JasonBenn/node-speaker" so both
"speaker" (or "@mastra/node-speaker") and "speaker-arm64" are installed from npm
for consistent security scanning and maintainability.
In @src/device/protocol/virtual/audio/piperVirtualDeviceLogic.ts:
- Around line 83-87: Add configurable sample rate support by extending
PiperVirtualDeviceConfig with a sampleRate property (default 22050) and use it
when creating the Speaker in piperVirtualDeviceLogic (replace the hardcoded
22050 in the this.speaker initialization). Alternatively, if model metadata is
available, derive the sample rate from the model info in the same initialization
path; ensure the constructor or factory that builds PiperVirtualDeviceConfig
populates sampleRate and update any callers to pass through or fall back to
22050.
In @src/device/protocol/virtual/virtualDeviceProvider.ts:
- Around line 53-58: The loop that removes devices from connectedDevices calls
removeDevice(v) but does not remove the device id from managedDevices, causing
re-added devices to be skipped; update the cleanup so that when a device is
removed you also delete its id from managedDevices (either by adding
managedDevices.delete(k) inside the removal loop that iterates
this.connectedDevices/virtualDevices or by updating the removeDevice method to
always remove the id from managedDevices), ensuring you reference the same id
key (k) used in the loop and keep removeDevice behavior consistent.
- Around line 61-69: The loop currently sets this.managedDevices.set(k, null)
then awaits addDevice(v) but never removes the key if addDevice rejects, causing
permanent skips; wrap the await this.addDevice(v) in a try/catch and on catch
call this.managedDevices.delete(k) (and log the error) so failed device
additions are retried on the next discovery cycle; alternatively move the
this.managedDevices.set(k, null) to after a successful addDevice(v) to only mark
devices as managed on success.
🧹 Nitpick comments (7)
src/util/numbers.ts (1)
8-8: Consider including the failing value in error messages for easier debugging.The error messages now use template literals but don't include the actual value that failed validation. Including the value would make debugging easier when these exceptions are thrown.
🔎 Proposed enhancement
- throw new Error(`Not an integer`);+ throw new Error(`Not an integer: ${value}`);- throw new Error(`Not a float`);+ throw new Error(`Not a float: ${value}`);Also applies to: 23-23
src/device/device.ts (1)
39-40: Consider adding a public getter for errorInfo.The
errorInfofield is properly exposed via@Expose()for serialization, but there's no public getter method like those forgetState(lines 102-104). If consumers need direct access to error information without relying on serialization, consider adding:publicgetgetErrorInfo(): DeviceError|undefined{returnthis.errorInfo;}src/device/protocol/virtual/audio/piperVirtualDeviceConfig.ts (1)
1-9: LGTM! Piper configuration schema is well-structured.The schema appropriately defines configuration for the Piper TTS device with an optional
binarypath (allowing for default/search behavior) and a requiredmodelfield. The export pattern is consistent with other config modules.Optional: Consider enhanced validation for future iteration
If stricter validation proves beneficial, you could add format constraints:
exportconstpiperVirtualDeviceConfigSchema=Type.Object({binary: Type.Optional(Type.String({minLength: 1})),model: Type.String({minLength: 1,pattern: '^[a-zA-Z0-9_-]+$'}),});This would ensure non-empty strings and (for model) a basic identifier pattern. However, the current implementation is adequate for initial functionality.
src/util/devNullStream.ts (1)
7-7: UnusedEventEmitterfield.The
eventsfield is instantiated but never used. The'idle'event on line 27 is emitted viathis.emit()(inherited fromWritable), not through this privateEventEmitter.Proposed fix
export default class DevNullStream extends Writable { private readonly timeoutMs: number; private timer?: NodeJS.Timeout; - private readonly events = new EventEmitter(); public constructor(timeoutMs: number = 500) {Also remove the unused import:
import {Writable} from "stream"; -import EventEmitter from "events";src/device/protocol/virtual/audio/piperVirtualDeviceLogic.ts (2)
54-56: Logging format may not interpolate as expected.The
%splaceholder in the log message won't be interpolated. Based on theLoggerinterface, the second argument is treated as context object, not a printf-style argument.Proposed fix
piperProcess.stderr.on('data', (data: Buffer) => { - this.logger.error('Piper stderr: %s', data.toString());+ this.logger.error(`Piper stderr: ${data.toString().trim()}`); });
97-107:DevNullStreamis not explicitly destroyed after draining.After the
'idle'event fires, theDevNullStreamis unpiped but not destroyed. While it will eventually be garbage collected, explicitly callingdevNull.destroy()in the idle handler would ensure timely cleanup of the internal timer.Proposed fix
const devNull = new DevNullStream(500); devNull.on('idle', () => { this.speakerCoolDown = false; this.piperProcess?.stdout.unpipe(); + devNull.destroy(); });src/device/protocol/virtual/virtualDevice.ts (1)
39-49: Error handling captures failures but could be more robust.The try/catch prevents unhandled rejections and sets the device to error state. However:
- The type assertion
(e as Error).messagemay fail if a non-Error is thrown (e.g., a string).- Once in error state, the device appears to stay there permanently since
PiperVirtualDeviceLogic.refreshDatareturns early on error state (line 117-119 of that file).More robust error extraction
} catch (e: unknown) { this.state = DeviceState.error; this.errorInfo = { - reason: (e as Error).message ?? 'Unknown error',+ reason: e instanceof Error ? e.message : String(e), occurredAt: new Date(), } }
📜 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 (25)
.github/workflows/release.ymlpackage.jsonsrc/device/anyDeviceConfig.tssrc/device/device.tssrc/device/deviceState.tssrc/device/protocol/virtual/audio/piperVirtualDeviceConfig.tssrc/device/protocol/virtual/audio/piperVirtualDeviceLogic.tssrc/device/protocol/virtual/audio/ttsVirtualDeviceConfig.tssrc/device/protocol/virtual/audio/ttsVirtualDeviceLogic.tssrc/device/protocol/virtual/display/displayVirtualDeviceLogic.tssrc/device/protocol/virtual/genericVirtualDeviceFactory.tssrc/device/protocol/virtual/randomGenerator/randomGeneratorVirtualDeviceConfig.tssrc/device/protocol/virtual/randomGenerator/randomGeneratorVirtualDeviceLogic.tssrc/device/protocol/virtual/virtualDevice.tssrc/device/protocol/virtual/virtualDeviceLogic.tssrc/device/protocol/virtual/virtualDeviceProvider.tssrc/schemaValidation/JsonSchemaValidator.tssrc/schemaValidation/JsonSchemaValidatorFactory.tssrc/serviceMap.tssrc/serviceProvider/deviceServiceProvider.tssrc/serviceProvider/schemaValidationServiceProvider.tssrc/serviceProvider/settingsServiceProvider.tssrc/util/devNullStream.tssrc/util/numbers.tssrc/util/process.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-30T08:02:18.388Z
Learnt from: heavyrubberslave
Repo: SlvCtrlPlus/slvctrlplus-server PR: 57
File: src/index.ts:89-108
Timestamp: 2025-12-30T08:02:18.388Z
Learning: Express 5 will automatically forward rejected promises and thrown errors from route handlers and middleware to your error-handling middleware, so you generally don’t need manual try/catch blocks or .catch(next). This applies to any route handler or middleware that returns a promise. Ensure you still have a proper error-handling middleware (err, req, res, next) in place and avoid relying on silent rejections. This guidance is applicable across TypeScript files in the project (src and beyond) and should be especially considered for routes and middleware that return promises.
Applied to files:
src/schemaValidation/JsonSchemaValidator.tssrc/device/device.tssrc/device/anyDeviceConfig.tssrc/device/protocol/virtual/audio/ttsVirtualDeviceConfig.tssrc/device/protocol/virtual/virtualDeviceLogic.tssrc/schemaValidation/JsonSchemaValidatorFactory.tssrc/device/protocol/virtual/genericVirtualDeviceFactory.tssrc/device/protocol/virtual/audio/piperVirtualDeviceConfig.tssrc/device/protocol/virtual/virtualDeviceProvider.tssrc/device/protocol/virtual/randomGenerator/randomGeneratorVirtualDeviceLogic.tssrc/util/devNullStream.tssrc/serviceProvider/settingsServiceProvider.tssrc/device/protocol/virtual/randomGenerator/randomGeneratorVirtualDeviceConfig.tssrc/util/process.tssrc/serviceProvider/schemaValidationServiceProvider.tssrc/device/deviceState.tssrc/device/protocol/virtual/display/displayVirtualDeviceLogic.tssrc/device/protocol/virtual/audio/piperVirtualDeviceLogic.tssrc/device/protocol/virtual/virtualDevice.tssrc/util/numbers.tssrc/serviceMap.tssrc/serviceProvider/deviceServiceProvider.tssrc/device/protocol/virtual/audio/ttsVirtualDeviceLogic.ts
🧬 Code graph analysis (11)
src/device/protocol/virtual/audio/ttsVirtualDeviceConfig.ts (3)
src/device/protocol/virtual/virtualDeviceFactory.ts (1)
VirtualDeviceFactory(4-9)src/settings/deviceSource.ts (1)
DeviceSource(5-33)src/device/protocol/virtual/delegatedVirtualDeviceFactory.ts (1)
DelegatedVirtualDeviceFactory(5-34)
src/device/protocol/virtual/virtualDeviceLogic.ts (2)
src/device/device.ts (1)
DeviceAttributes(6-6)src/device/anyDeviceConfig.ts (1)
AnyDeviceConfig(5-5)
src/schemaValidation/JsonSchemaValidatorFactory.ts (1)
src/schemaValidation/JsonSchemaValidator.ts (1)
JsonSchemaValidator(4-26)
src/device/protocol/virtual/genericVirtualDeviceFactory.ts (2)
src/schemaValidation/JsonSchemaValidator.ts (1)
JsonSchemaValidator(4-26)src/schemaValidation/JsonSchemaValidatorFactory.ts (1)
JsonSchemaValidatorFactory(7-24)
src/device/protocol/virtual/randomGenerator/randomGeneratorVirtualDeviceLogic.ts (3)
src/settings/deviceSource.ts (1)
config(30-32)src/settings/knownDevice.ts (1)
config(56-58)src/device/protocol/virtual/randomGenerator/randomGeneratorVirtualDeviceConfig.ts (1)
RandomGeneratorVirtualDeviceConfig(9-9)
src/util/devNullStream.ts (1)
src/logging/PinoLogger.ts (1)
error(35-37)
src/device/protocol/virtual/audio/piperVirtualDeviceLogic.ts (7)
src/device/attribute/strDeviceAttribute.ts (1)
StrDeviceAttribute(6-36)src/device/attribute/boolDeviceAttribute.ts (1)
BoolDeviceAttribute(6-36)src/device/protocol/virtual/virtualDeviceLogic.ts (1)
VirtualDeviceLogic(5-12)src/device/protocol/virtual/audio/piperVirtualDeviceConfig.ts (1)
PiperVirtualDeviceConfig(9-9)src/logging/Logger.ts (1)
Logger(9-19)src/util/process.ts (1)
spawnProcess(27-57)src/util/devNullStream.ts (1)
DevNullStream(4-29)
src/device/protocol/virtual/virtualDevice.ts (3)
src/device/device.ts (1)
DeviceAttributes(6-6)src/device/anyDeviceConfig.ts (1)
AnyDeviceConfig(5-5)src/device/protocol/virtual/virtualDeviceLogic.ts (1)
VirtualDeviceLogic(5-12)
src/serviceMap.ts (5)
src/device/protocol/virtual/genericVirtualDeviceFactory.ts (1)
GenericVirtualDeviceFactory(13-87)src/device/protocol/virtual/randomGenerator/randomGeneratorVirtualDeviceConfig.ts (1)
RandomGeneratorVirtualDeviceConfigSchema(8-8)src/device/anyDeviceConfig.ts (1)
AnyDeviceConfigSchema(4-4)src/device/protocol/virtual/audio/ttsVirtualDeviceConfig.ts (1)
TtsVirtualDeviceConfigSchema(7-7)src/device/protocol/virtual/audio/piperVirtualDeviceConfig.ts (1)
PiperVirtualDeviceConfigSchema(8-8)
src/serviceProvider/deviceServiceProvider.ts (6)
src/device/protocol/virtual/genericVirtualDeviceFactory.ts (1)
GenericVirtualDeviceFactory(13-87)src/device/protocol/virtual/randomGenerator/randomGeneratorVirtualDeviceConfig.ts (1)
randomGeneratorVirtualDeviceConfigSchema(3-6)src/device/anyDeviceConfig.ts (1)
anyDeviceConfigSchema(3-3)src/device/protocol/virtual/audio/ttsVirtualDeviceConfig.ts (1)
ttsVirtualDeviceConfigSchema(3-5)src/device/protocol/virtual/audio/piperVirtualDeviceLogic.ts (1)
PiperVirtualDeviceLogic(20-175)src/device/protocol/virtual/audio/piperVirtualDeviceConfig.ts (1)
piperVirtualDeviceConfigSchema(3-6)
src/device/protocol/virtual/audio/ttsVirtualDeviceLogic.ts (4)
src/device/protocol/virtual/audio/ttsVirtualDeviceConfig.ts (1)
TtsVirtualDeviceConfig(8-8)src/logging/Logger.ts (1)
Logger(9-19)src/settings/deviceSource.ts (1)
config(30-32)src/settings/knownDevice.ts (1)
config(56-58)
🔇 Additional comments (28)
src/util/numbers.ts (1)
4-4: Removing theno-redeclareESLint suppression is safe—the rule is not enabled.The ESLint configuration does not explicitly enable the
no-redeclarerule, so removing its suppression does not cause lint failures. The declaration merging pattern (typeInt+ constInt, typeFloat+ constFloat) is valid in TypeScript and does not requireno-redeclaresuppression. Thenaming-conventionsuppression that remains is correct and necessary.src/device/device.ts (1)
16-19: LGTM!The
DeviceErrortype is clean and well-structured. ThereasonandoccurredAtfields provide essential information for error tracking.src/schemaValidation/JsonSchemaValidator.ts (1)
1-2: LGTM!The formatting adjustment to add consistent spacing in the import statement improves code consistency.
src/device/deviceState.ts (1)
4-4: LGTM!The addition of the
errorstate to theDeviceStateenum follows the existing convention and enables proper error state tracking for devices.src/serviceProvider/settingsServiceProvider.ts (1)
19-19: Method exists with correct signature and is used appropriately.The
createFromFile(schemaFilePath: string)method exists inJsonSchemaValidatorFactoryand correctly accepts the file path string fromsettingsSchemaPath. This is semantically clearer than the alternativecreate()method which expects a schema object directly.src/device/protocol/virtual/display/displayVirtualDeviceLogic.ts (1)
16-18: LGTM! Getter naming improved.The rename from
getRefreshInterval()torefreshInterval()removes the redundant "get" prefix, aligning with idiomatic getter naming in TypeScript.src/serviceProvider/schemaValidationServiceProvider.ts (1)
3-3: LGTM! Proper type-only import optimization.Converting to
import typeis appropriate sinceAjvis only used for type annotation. The runtime dependency onAjv2020is correctly preserved.src/device/anyDeviceConfig.ts (1)
1-5: LGTM! Flexible config schema for generic devices.The
Record<string, unknown>schema provides the necessary flexibility for device configurations where the structure isn't predetermined. The export pattern (schema constant, type alias for schema, static type) is consistent with other config modules in this PR.src/device/protocol/virtual/audio/ttsVirtualDeviceConfig.ts (1)
1-8: LGTM! Clean TTS configuration schema.The schema appropriately defines an optional
voicefield for TTS configuration. The export pattern is consistent with other config modules, supporting the new typed factory approach.src/schemaValidation/JsonSchemaValidatorFactory.ts (1)
16-23: LGTM!Clean separation between in-memory schema compilation (
create) and file-based schema loading (createFromFile). The generic parameter enables type-safe schema validation.src/device/protocol/virtual/randomGenerator/randomGeneratorVirtualDeviceLogic.ts (1)
18-25: LGTM!The typed config simplifies the constructor by removing manual validation. Getter rename to
refreshIntervalfollows idiomatic TypeScript conventions.src/device/protocol/virtual/audio/ttsVirtualDeviceLogic.ts (2)
28-35: LGTM!Typed config integration is clean. The
voiceproperty access is now type-safe without explicit casting.
82-84: Getter rename is consistent with other device logic implementations.src/serviceProvider/deviceServiceProvider.ts (2)
136-152: LGTM!Piper factory wiring follows the established pattern for virtual device factories. The integration with the delegated factory is consistent with other device types.
112-134: Existing factories updated consistently.Good refactoring to use
GenericVirtualDeviceFactory.fromwith typed config schemas across all virtual device factories.src/device/protocol/virtual/virtualDeviceLogic.ts (1)
5-11: LGTM!The additional generic parameter
Cenables typed configuration throughout the device logic hierarchy. Defaults maintain backward compatibility with existing implementations.src/device/protocol/virtual/randomGenerator/randomGeneratorVirtualDeviceConfig.ts (1)
1-9: Clean TypeBox schema definition.The schema correctly uses TypeBox to define a typed configuration with
minandmaxnumeric fields. The exported types follow a consistent pattern with other config schemas in this PR.Consider whether runtime validation should enforce
min <= max— this could be done at the schema level usingType.Number({ maximum: ... })with a refinement, or in the device logic itself.src/util/devNullStream.ts (1)
15-28: Stream implementation looks correct.The
_writeand_finalmethods properly manage the idle timer lifecycle. The timeout-based idle detection pattern is appropriate for draining the Piper stdout buffer.src/device/protocol/virtual/audio/piperVirtualDeviceLogic.ts (3)
146-152: Text write and clear logic is correct.The logic properly checks stdin state before writing and clears the text attribute after sending. The guard against destroyed stdin prevents writes to a closed stream.
155-174: Attribute configuration and refresh interval look appropriate.The
textattribute as write-only andqueuingas read-write with a default offalsealign with the TTS use case. The 50ms refresh interval provides responsive text-to-speech handling.
117-119: The original review comment is incorrect.device.getStateis a public getter method defined in the Device class (line 102-104) that returns aDeviceStatevalue. The comparisonif (device.getState === DeviceState.error)is correct and follows the established pattern used elsewhere in the codebase (e.g., deviceProvider.ts line 31). No changes are needed.src/serviceMap.ts (2)
49-53: Imports correctly added for Piper support and typed configs.The new imports for
PiperVirtualDeviceLogicand the various config schema types align with the refactored factory pattern.
78-81: Factory type declarations properly parameterized.The
ServiceMapnow correctly types each virtual device factory with both its logic class and configuration schema, enabling type-safe factory creation and config validation.src/device/protocol/virtual/genericVirtualDeviceFactory.ts (2)
13-36: Well-structured generic factory with typed config validation.The private constructor with static
fromfactory pattern ensures consistent initialization. ThejsonSchemaValidatoris always assigned in the constructor, so the optional type (?) on line 21 is slightly misleading but harmless.
54-81: Config validation properly integrated into device creation.The validation step correctly throws with detailed error information when config is invalid. The thrown error inside the Promise constructor will properly reject the promise.
One minor note: since the code is entirely synchronous, the Promise wrapper could be simplified to
asyncwith direct return, but the current implementation works correctly.src/device/protocol/virtual/virtualDevice.ts (1)
8-19: Generic type parameterization is well-designed.The additional generic parameter
C extends AnyDeviceConfigwith a sensible default allows type-safe config handling while maintaining backward compatibility. ThedeviceLogictype correctly uses both generic parameters.src/util/process.ts (2)
14-25: Comprehensive TypeScript overloads for spawn.The overload signatures properly cover the common stdio configurations, providing strong typing for the returned
ChildProcessvariants. This enables type-safe access to stdin/stdout/stderr based on the stdio options.
36-55: Spawn promise implementation is solid.The
settledflag correctly prevents double resolution, and usingonce()ensures handlers are cleaned up. The error code translation covers the most common spawn failures.One minor note: the error message mutation (lines 42-45) modifies the original error object. This is acceptable here since the error is immediately rejected, but creating a new error would be more pure.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
eslint.config.ts (1)
176-184: Move the ESLint configuration change to a separate PR or revert it.The "spaced-comment" rule severity was downgraded from "error" to "warn" without any violations in the codebase to justify this change. The Piper TTS implementation follows proper comment spacing conventions, making this change unrelated to the PR's primary objective. Either revert this configuration change or move it to a dedicated PR focused on linting policy updates.
src/device/protocol/virtual/audio/ttsVirtualDeviceLogic.ts (1)
73-80: Fix incorrect callback error parameter type.The
say.speak()callback signature expects(err?: Error | null) => void, not(err: string). Update line 74 to type the error parameter asError | nullinstead ofstring.
🧹 Nitpick comments (2)
src/device/protocol/virtual/genericVirtualDeviceFactory.ts (2)
67-74: Unnecessary undefined check forjsonSchemaValidator.The
jsonSchemaValidatoris always assigned in the constructor (line 35) viajsonSchemaValidatorFactory.create(deviceConfigSchema), so theundefinedcheck is redundant. Consider removing the conditional.♻️ Suggested simplification
public create(knownDevice: KnownDevice, provider: string): Promise<VirtualDevice> { return new Promise<VirtualDevice>((resolve) => { - if (undefined !== this.jsonSchemaValidator) {- const isConfigValid = this.jsonSchemaValidator.validate(knownDevice.config);-- if (!isConfigValid) {- const validationErrors = this.jsonSchemaValidator.getValidationErrors();- throw new Error(`Config for device is not valid: ${JSON.stringify(validationErrors, null, 2)}`);- }+ const isConfigValid = this.jsonSchemaValidator.validate(knownDevice.config);++ if (!isConfigValid) {+ const validationErrors = this.jsonSchemaValidator.getValidationErrors();+ throw new Error(`Config for device is not valid: ${JSON.stringify(validationErrors, null, 2)}`); }Note: If future refactoring could make the validator optional, keep the check. Otherwise, removing it makes the code clearer and the field type can drop the
?.
64-91: Consider usingPromise.reject()or async/await instead of throwing in executor.Throwing inside a
new Promise()executor works but is less idiomatic. Since this is effectively synchronous, consider usingasync/awaitor returningPromise.resolve()/Promise.reject()directly.♻️ Alternative using async/await
- public create(knownDevice: KnownDevice, provider: string): Promise<VirtualDevice>- {- return new Promise<VirtualDevice>((resolve) => {- if (undefined !== this.jsonSchemaValidator) {- const isConfigValid = this.jsonSchemaValidator.validate(knownDevice.config);-- if (!isConfigValid) {- const validationErrors = this.jsonSchemaValidator.getValidationErrors();- throw new Error(`Config for device is not valid: ${JSON.stringify(validationErrors, null, 2)}`);- }- }-- const deviceLogic = new this.ctor(knownDevice.config as ExtractConfig<TLogic>, this.logger);-- const device = new VirtualDevice(- '1.0.0',- knownDevice.id,- knownDevice.name,- knownDevice.type,- provider,- this.dateFactory.now(),- knownDevice.config,- deviceLogic- );-- resolve(device);- });- }+ public async create(knownDevice: KnownDevice, provider: string): Promise<VirtualDevice>+ {+ const isConfigValid = this.jsonSchemaValidator.validate(knownDevice.config);++ if (!isConfigValid) {+ const validationErrors = this.jsonSchemaValidator.getValidationErrors();+ throw new Error(`Config for device is not valid: ${JSON.stringify(validationErrors, null, 2)}`);+ }++ const deviceLogic = new this.ctor(knownDevice.config as ExtractConfig<TLogic>, this.logger);++ return new VirtualDevice(+ '1.0.0',+ knownDevice.id,+ knownDevice.name,+ knownDevice.type,+ provider,+ this.dateFactory.now(),+ knownDevice.config,+ deviceLogic+ );+ }
📜 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 (12)
eslint.config.tspackage.jsonsrc/device/deviceConfig.tssrc/device/protocol/virtual/audio/piperVirtualDeviceLogic.tssrc/device/protocol/virtual/audio/ttsVirtualDeviceLogic.tssrc/device/protocol/virtual/genericVirtualDeviceFactory.tssrc/device/protocol/virtual/randomGenerator/randomGeneratorVirtualDeviceLogic.tssrc/device/protocol/virtual/virtualDevice.tssrc/device/protocol/virtual/virtualDeviceLogic.tssrc/serviceMap.tssrc/serviceProvider/deviceServiceProvider.tstslint.json
💤 Files with no reviewable changes (1)
- tslint.json
🚧 Files skipped from review as they are similar to previous changes (4)
- src/device/protocol/virtual/audio/piperVirtualDeviceLogic.ts
- src/serviceProvider/deviceServiceProvider.ts
- src/device/protocol/virtual/virtualDevice.ts
- package.json
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-30T08:02:18.388Z
Learnt from: heavyrubberslave
Repo: SlvCtrlPlus/slvctrlplus-server PR: 57
File: src/index.ts:89-108
Timestamp: 2025-12-30T08:02:18.388Z
Learning: Express 5 will automatically forward rejected promises and thrown errors from route handlers and middleware to your error-handling middleware, so you generally don’t need manual try/catch blocks or .catch(next). This applies to any route handler or middleware that returns a promise. Ensure you still have a proper error-handling middleware (err, req, res, next) in place and avoid relying on silent rejections. This guidance is applicable across TypeScript files in the project (src and beyond) and should be especially considered for routes and middleware that return promises.
Applied to files:
eslint.config.tssrc/device/protocol/virtual/genericVirtualDeviceFactory.tssrc/device/protocol/virtual/virtualDeviceLogic.tssrc/device/deviceConfig.tssrc/serviceMap.tssrc/device/protocol/virtual/audio/ttsVirtualDeviceLogic.tssrc/device/protocol/virtual/randomGenerator/randomGeneratorVirtualDeviceLogic.ts
🧬 Code graph analysis (5)
src/device/protocol/virtual/genericVirtualDeviceFactory.ts (6)
src/device/protocol/virtual/virtualDeviceLogic.ts (1)
VirtualDeviceLogic(5-14)src/settings/knownDevice.ts (1)
config(56-58)src/device/protocol/virtual/virtualDeviceFactory.ts (1)
VirtualDeviceFactory(4-9)src/factory/dateFactory.ts (1)
DateFactory(1-7)src/schemaValidation/JsonSchemaValidator.ts (1)
JsonSchemaValidator(4-26)src/schemaValidation/JsonSchemaValidatorFactory.ts (1)
JsonSchemaValidatorFactory(7-24)
src/device/protocol/virtual/virtualDeviceLogic.ts (2)
src/device/device.ts (1)
DeviceAttributes(6-6)src/device/deviceConfig.ts (2)
DeviceConfig(7-7)NoDeviceConfig(5-5)
src/serviceMap.ts (6)
src/device/protocol/virtual/genericVirtualDeviceFactory.ts (1)
GenericVirtualDeviceFactory(14-97)src/device/protocol/virtual/randomGenerator/randomGeneratorVirtualDeviceConfig.ts (1)
RandomGeneratorVirtualDeviceConfigSchema(8-8)src/device/deviceConfig.ts (1)
NoDeviceConfigSchema(4-4)src/device/protocol/virtual/audio/ttsVirtualDeviceConfig.ts (1)
TtsVirtualDeviceConfigSchema(7-7)src/device/protocol/virtual/audio/piperVirtualDeviceLogic.ts (1)
PiperVirtualDeviceLogic(20-177)src/device/protocol/virtual/audio/piperVirtualDeviceConfig.ts (1)
PiperVirtualDeviceConfigSchema(8-8)
src/device/protocol/virtual/audio/ttsVirtualDeviceLogic.ts (4)
src/device/protocol/virtual/virtualDeviceLogic.ts (1)
VirtualDeviceLogic(5-14)src/device/protocol/virtual/audio/ttsVirtualDeviceConfig.ts (1)
TtsVirtualDeviceConfig(8-8)src/settings/deviceSource.ts (1)
config(30-32)src/settings/knownDevice.ts (1)
config(56-58)
src/device/protocol/virtual/randomGenerator/randomGeneratorVirtualDeviceLogic.ts (4)
src/device/protocol/virtual/virtualDeviceLogic.ts (1)
VirtualDeviceLogic(5-14)src/device/protocol/virtual/randomGenerator/randomGeneratorVirtualDeviceConfig.ts (1)
RandomGeneratorVirtualDeviceConfig(9-9)src/settings/deviceSource.ts (1)
config(30-32)src/settings/knownDevice.ts (1)
config(56-58)
🔇 Additional comments (6)
src/device/deviceConfig.ts (1)
1-7: LGTM! Clean TypeBox schema foundation for device configs.The schema definitions follow TypeBox conventions properly. Using
additionalProperties: falseonnoDeviceConfigSchemaenforces strict validation for devices without configuration, whileDeviceConfig = Static<TObject>provides appropriate flexibility for typed device configs.src/device/protocol/virtual/virtualDeviceLogic.ts (1)
5-13: LGTM! Well-structured dual-generic interface.The addition of
TConfiggeneric with a sensible default (NoDeviceConfig) maintains backward compatibility while enabling type-safe configuration handling. The getter rename fromgetRefreshInterval()torefreshIntervalfollows proper accessor naming conventions.src/device/protocol/virtual/randomGenerator/randomGeneratorVirtualDeviceLogic.ts (1)
12-31: LGTM! Clean adoption of strongly-typed configuration.The direct access to
config.minandconfig.maxwithout runtime presence checks is appropriate here sinceGenericVirtualDeviceFactory.create()validates the config against the schema before instantiating the device logic. The type system guarantees these properties exist.src/serviceMap.ts (1)
78-81: LGTM! Service map properly updated for config-aware factories.The factory type definitions correctly pair each device logic with its corresponding config schema type. The new
piperentry aligns with the PR objective to add Piper TTS support.src/device/protocol/virtual/audio/ttsVirtualDeviceLogic.ts (1)
19-39: LGTM! Clean migration to strongly-typed configuration.The removal of the type assertion on
this.config.voice(line 65) is a good improvement - the type is now correctly inferred from the schema.src/device/protocol/virtual/genericVirtualDeviceFactory.ts (1)
39-62: LGTM! Clever type constraints ensure compile-time safety.The bidirectional type constraint on
deviceConfigSchema(lines 44-50) elegantly ensures that the schema's static type exactly matches the logic's expected config type, catching mismatches at compile time.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/device/protocol/virtual/virtualDevice.ts:
- Around line 42-50: The catch block in the refreshData path swallows exceptions
and is missing a semicolon: when awaiting this.deviceLogic.refreshData(this) you
set this.state = DeviceState.error and populate this.errorInfo but forgot the
trailing semicolon and do not re-throw the caught error; update the catch to add
the semicolon after the errorInfo assignment and then re-throw the original
error (the caught e) so callers of refreshData (e.g., in deviceProvider.ts)
receive the rejection as expected.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/device/deviceConfig.tssrc/device/protocol/virtual/virtualDevice.tssrc/device/protocol/virtual/virtualDeviceLogic.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-30T08:02:18.388Z
Learnt from: heavyrubberslave
Repo: SlvCtrlPlus/slvctrlplus-server PR: 57
File: src/index.ts:89-108
Timestamp: 2025-12-30T08:02:18.388Z
Learning: Express 5 will automatically forward rejected promises and thrown errors from route handlers and middleware to your error-handling middleware, so you generally don’t need manual try/catch blocks or .catch(next). This applies to any route handler or middleware that returns a promise. Ensure you still have a proper error-handling middleware (err, req, res, next) in place and avoid relying on silent rejections. This guidance is applicable across TypeScript files in the project (src and beyond) and should be especially considered for routes and middleware that return promises.
Applied to files:
src/device/protocol/virtual/virtualDevice.tssrc/device/deviceConfig.tssrc/device/protocol/virtual/virtualDeviceLogic.ts
🧬 Code graph analysis (2)
src/device/protocol/virtual/virtualDevice.ts (3)
src/device/device.ts (2)
DeviceAttributes(6-6)AttributeValue(8-8)src/device/deviceConfig.ts (1)
AnyDeviceConfig(7-7)src/device/protocol/virtual/virtualDeviceLogic.ts (1)
VirtualDeviceLogic(5-14)
src/device/protocol/virtual/virtualDeviceLogic.ts (2)
src/device/device.ts (1)
DeviceAttributes(6-6)src/device/deviceConfig.ts (2)
AnyDeviceConfig(7-7)NoDeviceConfig(5-5)
🔇 Additional comments (3)
src/device/deviceConfig.ts (1)
1-7: LGTM! Clean type definitions for device configuration.The TypeBox schemas and type exports are well-structured. The
additionalProperties: falseonnoDeviceConfigSchemaprovides strict validation, and the genericAnyDeviceConfigtype offers flexibility for various device configurations.src/device/protocol/virtual/virtualDeviceLogic.ts (1)
5-13: LGTM! Solid generic refactoring.The dual-generic interface design (
TAttributesandTConfig) with sensible defaults enables type-safe device configuration while maintaining backward compatibility. The getter syntax forrefreshIntervalis correctly aligned with its usage.src/device/protocol/virtual/virtualDevice.ts (1)
8-11: LGTM! Clean generic refactoring across the class.The dual-generic implementation is consistent and type-safe. The
TAttributesandTConfiggenerics are properly threaded through the class signature, constructor, and methods.Also applies to: 16-16, 21-21, 30-31, 54-54, 57-57
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @src/device/protocol/virtual/audio/piperVirtualDeviceLogic.ts:
- Around line 85-89: The Speaker is instantiated with a hardcoded sampleRate of
22050 which will misplay models trained at 16000; update
PiperVirtualDeviceConfig to include a sampleRate property (or, alternatively,
load the sample rate from the model's .onnx.json) and use that value when
creating the Speaker instead of the literal 22050. Locate the Speaker creation
in PiperVirtualDeviceLogic (the new Speaker({...}) call) and replace the fixed
sampleRate with config.sampleRate (or the parsed value from the model's
.onnx.json), provide a sensible default fallback (e.g., 22050) if the config or
model file is missing, and ensure any config parsing/loading code handles both
modelName->.onnx.json lookup and config overrides.
In @src/device/protocol/virtual/virtualDeviceProvider.ts:
- Around line 14-16: managedDevices currently uses a Map and is marked before
addDevice, and addDevice swallows errors while removeDevice never clears
managedDevices; change managedDevices to a Set<string> (replace Map<string,null>
with Set<string>), only add the device key to managedDevices after addDevice
completes successfully, modify addDevice to propagate or return failures (do not
swallow errors silently) so callers can clean up on failure, and ensure
removeDevice clears the key from managedDevices and clears deviceUpdaters entry
(NodeJS.Timeout) when removing a device; update all usages (e.g., where
managedDevices.set/get/delete, addDevice, removeDevice, deviceUpdaters)
accordingly.
🧹 Nitpick comments (3)
src/device/protocol/virtual/virtualDeviceLogicFactory.ts (1)
3-3: Consider exportingExtractConfigfor reuse.This type utility is duplicated in
genericVirtualDeviceLogicFactory.ts(line 5). Consider exporting it from this file or a shared types module to avoid duplication.♻️ Suggested change
-type ExtractConfig<T extends VirtualDeviceLogic<any, any>> = T extends VirtualDeviceLogic<any, infer C> ? C : never;+export type ExtractConfig<T extends VirtualDeviceLogic<any, any>> = T extends VirtualDeviceLogic<any, infer C> ? C : never;src/device/protocol/virtual/audio/piperVirtualDeviceLogic.ts (1)
94-114: Consider explicit cleanup of DevNullStream.The
DevNullStreamis created but relies on garbage collection for cleanup. While theidleevent handler unpipes, explicitly destroying the stream would be cleaner.♻️ Optional improvement
this.piperProcess.stdout.unpipe(); const devNull = new DevNullStream(500); devNull.on('idle', () => { this.speakerCoolDown = false; this.piperProcess?.stdout.unpipe(); + devNull.destroy(); });src/device/protocol/virtual/genericVirtualDeviceFactory.ts (1)
53-88: Preferasync create()+ align key toknownDevice.type; drop redundant checks.The current
new Promise(resolve => { ... throw ... })is harder to read, andif (undefined !== jsonSchemaValidator)(Line 64) is redundant givenJsonSchemaValidatorFactory.create()always returns a validator. Also consider caching compiled validators at registration time to avoid recompiling schemas per device.Proposed refactor (key by device type + async)
export default class GenericVirtualDeviceFactory implements VirtualDeviceFactory { @@ - public create(knownDevice: KnownDevice, provider: string): Promise<VirtualDevice> {- return new Promise<VirtualDevice>((resolve) => {- const factoryName = `${GenericVirtualDeviceFactory.capitalizeFirstLetter(knownDevice.type)}VirtualDeviceLogic`;- const factory = this.logicFactories.get(factoryName);+ public async create(knownDevice: KnownDevice, provider: string): Promise<VirtualDevice> {+ const factoryKey = knownDevice.type;+ const factory = this.logicFactories.get(factoryKey);- if (undefined === factory) {- throw new Error(`Could not find a factory for virtual device logic '${factoryName}'`);- }+ if (undefined === factory) {+ throw new Error(`Could not find a factory for virtual device type '${factoryKey}'`);+ }- const jsonSchemaValidator = this.jsonSchemaValidatorFactory.create(factory.deviceConfigSchema);+ const jsonSchemaValidator = this.jsonSchemaValidatorFactory.create(factory.deviceConfigSchema);- if (undefined !== jsonSchemaValidator) {- const isConfigValid = jsonSchemaValidator.validate(knownDevice.config);+ const isConfigValid = jsonSchemaValidator.validate(knownDevice.config);- if (!isConfigValid) {- const validationErrors = jsonSchemaValidator.getValidationErrors();- throw new Error(`Config for device is not valid: ${JSON.stringify(validationErrors, null, 2)}`);- }- }+ if (!isConfigValid) {+ const validationErrors = jsonSchemaValidator.getValidationErrors();+ throw new Error(`Config for device is not valid: ${JSON.stringify(validationErrors, null, 2)}`);+ }- const deviceLogic = factory.deviceLogicFactory.create(knownDevice.config as Static<typeof factory.deviceConfigSchema>);+ const deviceLogic = factory.deviceLogicFactory.create(knownDevice.config as Static<typeof factory.deviceConfigSchema>); @@ - resolve(device);- });+ return device; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/device/protocol/virtual/audio/piperVirtualDeviceLogic.tssrc/device/protocol/virtual/delegatedVirtualDeviceFactory.tssrc/device/protocol/virtual/genericVirtualDeviceFactory.tssrc/device/protocol/virtual/genericVirtualDeviceLogicFactory.tssrc/device/protocol/virtual/virtualDeviceFactory.tssrc/device/protocol/virtual/virtualDeviceLogicFactory.tssrc/device/protocol/virtual/virtualDeviceProvider.tssrc/device/protocol/virtual/virtualDeviceProviderFactory.tssrc/serviceMap.tssrc/serviceProvider/deviceServiceProvider.ts
💤 Files with no reviewable changes (1)
- src/device/protocol/virtual/delegatedVirtualDeviceFactory.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-30T08:02:18.388Z
Learnt from: heavyrubberslave
Repo: SlvCtrlPlus/slvctrlplus-server PR: 57
File: src/index.ts:89-108
Timestamp: 2025-12-30T08:02:18.388Z
Learning: Express 5 will automatically forward rejected promises and thrown errors from route handlers and middleware to your error-handling middleware, so you generally don’t need manual try/catch blocks or .catch(next). This applies to any route handler or middleware that returns a promise. Ensure you still have a proper error-handling middleware (err, req, res, next) in place and avoid relying on silent rejections. This guidance is applicable across TypeScript files in the project (src and beyond) and should be especially considered for routes and middleware that return promises.
Applied to files:
src/device/protocol/virtual/genericVirtualDeviceLogicFactory.tssrc/device/protocol/virtual/audio/piperVirtualDeviceLogic.tssrc/device/protocol/virtual/virtualDeviceProviderFactory.tssrc/device/protocol/virtual/virtualDeviceFactory.tssrc/device/protocol/virtual/virtualDeviceProvider.tssrc/serviceMap.tssrc/device/protocol/virtual/virtualDeviceLogicFactory.tssrc/device/protocol/virtual/genericVirtualDeviceFactory.tssrc/serviceProvider/deviceServiceProvider.ts
🧬 Code graph analysis (6)
src/device/protocol/virtual/genericVirtualDeviceLogicFactory.ts (3)
src/device/protocol/virtual/virtualDeviceLogic.ts (1)
VirtualDeviceLogic(5-14)src/logging/Logger.ts (1)
Logger(9-19)src/device/protocol/virtual/virtualDeviceLogicFactory.ts (1)
VirtualDeviceLogicFactory(5-10)
src/device/protocol/virtual/audio/piperVirtualDeviceLogic.ts (6)
src/device/attribute/strDeviceAttribute.ts (1)
StrDeviceAttribute(6-36)src/device/attribute/boolDeviceAttribute.ts (1)
BoolDeviceAttribute(6-36)src/device/protocol/virtual/virtualDeviceLogic.ts (1)
VirtualDeviceLogic(5-14)src/logging/Logger.ts (1)
Logger(9-19)src/util/process.ts (1)
spawnProcess(27-57)src/util/devNullStream.ts (1)
DevNullStream(4-29)
src/device/protocol/virtual/virtualDeviceProviderFactory.ts (4)
src/device/provider/deviceProviderFactory.ts (1)
DeviceProviderFactory(4-7)src/device/protocol/virtual/virtualDeviceFactory.ts (1)
VirtualDeviceFactory(4-6)src/settings/settingsManager.ts (1)
SettingsManager(19-127)src/logging/Logger.ts (1)
Logger(9-19)
src/serviceMap.ts (1)
src/device/protocol/virtual/virtualDeviceFactory.ts (1)
VirtualDeviceFactory(4-6)
src/device/protocol/virtual/virtualDeviceLogicFactory.ts (1)
src/device/protocol/virtual/virtualDeviceLogic.ts (1)
VirtualDeviceLogic(5-14)
src/device/protocol/virtual/genericVirtualDeviceFactory.ts (5)
src/device/protocol/virtual/virtualDeviceLogic.ts (1)
VirtualDeviceLogic(5-14)src/device/protocol/virtual/virtualDeviceLogicFactory.ts (1)
VirtualDeviceLogicFactory(5-10)src/device/protocol/virtual/virtualDeviceFactory.ts (1)
VirtualDeviceFactory(4-6)src/factory/dateFactory.ts (1)
DateFactory(1-7)src/schemaValidation/JsonSchemaValidatorFactory.ts (1)
JsonSchemaValidatorFactory(7-24)
🔇 Additional comments (10)
src/device/protocol/virtual/virtualDeviceFactory.ts (1)
1-6: LGTM!The simplified interface properly delegates device-type identification to
VirtualDeviceLogicFactory, maintaining clean separation of concerns. The unified factory pattern reduces boilerplate.src/device/protocol/virtual/virtualDeviceLogicFactory.ts (1)
5-10: LGTM!The factory interface is well-typed with proper generic constraints. The
forDeviceType()method appropriately moved here fromVirtualDeviceFactory.src/serviceMap.ts (1)
44-44: LGTM!The consolidation from multiple per-type factory entries to a single unified
device.virtual.factorysimplifies the service map and aligns with the new factory registration pattern.Also applies to: 68-68
src/device/protocol/virtual/virtualDeviceProviderFactory.ts (1)
7-7: LGTM!Clean migration from
DelegatedVirtualDeviceFactoryto the unifiedVirtualDeviceFactorytype. The change is consistent throughout the class.Also applies to: 12-12, 20-20
src/device/protocol/virtual/genericVirtualDeviceLogicFactory.ts (2)
8-33: LGTM!Well-structured generic factory with proper type constraints. The private constructor with static
from()factory method enforces controlled instantiation.
35-38: No action required.The codebase uses only TypeScript compilation (
tsc) without any minification, uglification, or mangling tools. The build script is"build": "tsc", there are no minification tools in devDependencies, and neither.babelrcnortsconfig.jsonconfigure any name mangling. Class names are preserved in production builds, sothis.ctor.namewill work correctly as a stable device type identifier.src/device/protocol/virtual/audio/piperVirtualDeviceLogic.ts (1)
157-176: LGTM!The attribute configuration is clean with proper modifiers (
writeOnlyfor text,readWritefor queuing). The 50ms refresh interval aligns with the PR objective of reducing latency for low-power devices.src/serviceProvider/deviceServiceProvider.ts (2)
112-140: LGTM!Clean factory registration pattern using method chaining. The unified
GenericVirtualDeviceFactoryapproach withaddLogicFactoryis extensible and keeps device-specific logic well-organized.
105-110: LGTM!The dependency on
device.virtual.factoryis correctly resolved through the DI container's lazy evaluation - the service definition order doesn't affect runtime resolution.src/device/protocol/virtual/genericVirtualDeviceFactory.ts (1)
38-51: No issue here — factory keys are consistent.The storage key uses
virtualDeviceLogicFactory.forDeviceType()which returns the class name (e.g.,RandomGeneratorVirtualDeviceLogic). The lookup key builds${capitalizeFirstLetter(knownDevice.type)}VirtualDeviceLogic, wherecapitalizeFirstLetter()only uppercases the first character while preserving the rest of the string. For camelCase device types like"randomGenerator", this correctly producesRandomGeneratorVirtualDeviceLogic—matching the stored class name exactly. All registered devices (randomGenerator,display,tts,piper) follow this pattern consistently.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/device/protocol/zc95/zc95Device.ts (1)
261-261: Missing semicolon.Minor style inconsistency - add semicolon for consistency with the rest of the codebase.
- channelAttr.value = Int.from(Math.floor(channel.MaxOutputPower * 0.1)) // or channel.OutputPower?+ channelAttr.value = Int.from(Math.floor(channel.MaxOutputPower * 0.1)); // or channel.OutputPower?
🤖 Fix all issues with AI agents
In @src/device/protocol/virtual/audio/piperVirtualDeviceLogic.ts:
- Around line 38-74: Add a cleanup routine to terminate the spawned Piper child
process to avoid leaks: implement a public async cleanup() (or
onStop()/destroy() override in VirtualDeviceLogic/VirtualDevice lifecycle) that
calls stopPlayback(), checks this.piperProcess, ends stdin, kills the process,
sets this.piperProcess = undefined, and logs termination; ensure callers or the
device lifecycle invoke this cleanup when the device is stopped or removed.
In @src/device/protocol/virtual/audio/ttsVirtualDeviceLogic.ts:
- Line 37: The refreshData method in TtsVirtualDeviceLogic currently declares
its parameter as VirtualDevice<TtsVirtualDeviceLogic>; change the signature to
match the abstract base class by using VirtualDevice<this> instead (i.e., public
async refreshData(device: VirtualDevice<this>): Promise<void>), keeping the
method body unchanged so it properly overrides VirtualDeviceLogic and preserves
polymorphic typing for subclasses.
In
@src/device/protocol/virtual/randomGenerator/randomGeneratorVirtualDeviceLogic.ts:
- Around line 20-24: The constructor RandomGeneratorVirtualDeviceLogic currently
assigns config.min and config.max without validating that min <= max, which can
cause a negative range in the random calculation (Math.floor(Math.random() *
(this.max - this.min + 1))). Add validation: in
RandomGeneratorVirtualDeviceLogic.constructor check if config.min <= config.max
and throw a clear Error (or normalize by swapping) when the invariant is
violated; alternatively, add TypeBox constraints to
RandomGeneratorVirtualDeviceConfig (e.g., set min and max bounds or a custom
validator ensuring min <= max) so invalid configs are rejected before
construction.
In @src/device/protocol/virtual/virtualDeviceProvider.ts:
- Line 14: The managedDevices Map is never cleared, blocking retries and
re-adds; update the createDevice and removeDevice flows to always remove the
device id from managedDevices on failure or removal. Specifically, in the method
that adds entries to managedDevices (managedDevices.set(...)) ensure you catch
initialization failures in createDevice (where connectedDevices is populated)
and call managedDevices.delete(deviceId) before returning/throwing; likewise, in
removeDevice ensure you call managedDevices.delete(deviceId) when a device is
removed. Make these changes around the createDevice and removeDevice functions
and any error paths that currently leave managedDevices populated.
🧹 Nitpick comments (7)
src/device/protocol/zc95/zc95DeviceFactory.ts (1)
54-65: Consider typing the config parameter explicitly.The empty object passed as config should be explicitly typed for consistency with the device config pattern introduced in this PR.
♻️ Proposed enhancement
Add the import at the top of the file:
+import { NoDeviceConfig } from '../../deviceConfig.js';Then update the constructor call:
return new Zc95Device( this.uuidFactory.create(), this.nameGenerator.generateName(), provider, this.dateFactory.now(), versionDetails.ZC95, transport, true, attributes, - {},+ {} as NoDeviceConfig, receiveQueue );src/device/protocol/virtual/audio/piperVirtualDeviceLogic.ts (1)
92-112: Verify the DevNullStream drain approach.The
stopPlayback()method uses aDevNullStreamwith a 500ms timeout to drain remaining Piper output before stopping the speaker. While this prevents data loss, consider whether:
- The 500ms timeout is sufficient for all scenarios (longer utterances may produce more buffered data)
- The
speakerCoolDownflag adequately prevents race conditions during the drain period- The second
unpipe()call on line 101 (in the idle handler) is necessary, given the unpipe on line 97Based on learnings, this pattern may benefit from verification or clarification in comments explaining the drain behavior and timeout choice.
src/device/protocol/virtual/genericVirtualDeviceFactory.ts (2)
53-88: UnnecessaryPromisewrapper for synchronous code.The
createmethod wraps synchronous code in anew Promise()constructor. Since all operations are synchronous (noawaitneeded), this adds unnecessary complexity. Consider simplifying toasync/awaitor just returning synchronously.♻️ Suggested refactor
- public create(knownDevice: KnownDevice, provider: string): Promise<VirtualDevice<any>> {- return new Promise<VirtualDevice<any>>((resolve) => {+ public create(knownDevice: KnownDevice, provider: string): Promise<VirtualDevice<any>> { const factoryName = `${GenericVirtualDeviceFactory.capitalizeFirstLetter(knownDevice.type)}VirtualDeviceLogic`; const factory = this.logicFactories.get(factoryName); if (undefined === factory) { throw new Error(`Could not find a factory for virtual device logic '${factoryName}'`); } const jsonSchemaValidator = this.jsonSchemaValidatorFactory.create(factory.deviceConfigSchema); - if (undefined !== jsonSchemaValidator) {- const isConfigValid = jsonSchemaValidator.validate(knownDevice.config);+ const isConfigValid = jsonSchemaValidator.validate(knownDevice.config);- if (!isConfigValid) {- const validationErrors = jsonSchemaValidator.getValidationErrors();- throw new Error(`Config for device is not valid: ${JSON.stringify(validationErrors, null, 2)}`);- }+ if (!isConfigValid) {+ const validationErrors = jsonSchemaValidator.getValidationErrors();+ throw new Error(`Config for device is not valid: ${JSON.stringify(validationErrors, null, 2)}`); } const deviceLogic = factory.deviceLogicFactory.create(knownDevice.config as Static<typeof factory.deviceConfigSchema>); const device = new VirtualDevice( '1.0.0', knownDevice.id, knownDevice.name, knownDevice.type, provider, this.dateFactory.now(), knownDevice.config, deviceLogic ); - resolve(device);- });+ return Promise.resolve(device); }
76-76: Hardcoded version string.The firmware version
'1.0.0'is hardcoded. Consider whether this should be configurable or derived from the device configuration, especially as devices evolve.src/device/protocol/zc95/zc95Device.ts (2)
43-43: Consider specifying both generic type parameters.The
Deviceclass now has two generic parameters:TAttributesandTConfig. WhileTConfighas a default ofNoDeviceConfig, explicitly specifying both improves clarity and consistency with other device implementations in this PR.-export default class Zc95Device extends Device<Zc95DeviceAttributes> {+export default class Zc95Device extends Device<Zc95DeviceAttributes, NoDeviceConfig> {
255-259: Simplify redundant attribute access.The code accesses
this.attributes[channelAttrName]multiple times whenchannelAttralready holds the same reference.♻️ Suggested simplification
if (undefined !== this.attributes[channelAttrName]?.value && - this.attributes[channelAttrName].value > percentagePowerLimit+ channelAttr.value !== undefined &&+ channelAttr.value > percentagePowerLimit ) { - this.attributes[channelAttrName].value = percentagePowerLimit;+ channelAttr.value = percentagePowerLimit; }src/device/protocol/virtual/virtualDeviceProvider.ts (1)
60-69: Consider concurrent device creation for faster discovery.Currently, devices are created sequentially (line 68). If device initialization is slow or times out, it blocks discovery of subsequent devices.
Consider using
Promise.all()orPromise.allSettled()to create devices concurrently:♻️ Proposed refactor
// Load all currently configured devices + const devicesToAdd: KnownDevice[] = []; for (const [k, v] of virtualDevices) { if (this.managedDevices.has(k) || this.connectedDevices.has(k)) { continue; } this.managedDevices.set(k, null); + devicesToAdd.push(v);+ }- await this.addDevice(v);- }+ // Create all devices concurrently+ await Promise.allSettled(devicesToAdd.map(device => this.addDevice(device))); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
src/device/device.tssrc/device/protocol/buttplugIo/buttplugIoDevice.tssrc/device/protocol/slvCtrlPlus/genericSlvCtrlPlusDevice.tssrc/device/protocol/slvCtrlPlus/slvCtrlPlusDevice.tssrc/device/protocol/virtual/audio/piperVirtualDeviceLogic.tssrc/device/protocol/virtual/audio/ttsVirtualDeviceLogic.tssrc/device/protocol/virtual/display/displayVirtualDeviceLogic.tssrc/device/protocol/virtual/genericVirtualDeviceFactory.tssrc/device/protocol/virtual/genericVirtualDeviceLogicFactory.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.tstests/unit/device/testDevice.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/device/protocol/virtual/genericVirtualDeviceLogicFactory.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-30T08:02:18.388Z
Learnt from: heavyrubberslave
Repo: SlvCtrlPlus/slvctrlplus-server PR: 57
File: src/index.ts:89-108
Timestamp: 2025-12-30T08:02:18.388Z
Learning: Express 5 will automatically forward rejected promises and thrown errors from route handlers and middleware to your error-handling middleware, so you generally don’t need manual try/catch blocks or .catch(next). This applies to any route handler or middleware that returns a promise. Ensure you still have a proper error-handling middleware (err, req, res, next) in place and avoid relying on silent rejections. This guidance is applicable across TypeScript files in the project (src and beyond) and should be especially considered for routes and middleware that return promises.
Applied to files:
src/device/protocol/virtual/display/displayVirtualDeviceLogic.tssrc/device/protocol/zc95/zc95DeviceFactory.tssrc/device/protocol/virtual/audio/piperVirtualDeviceLogic.tssrc/device/protocol/virtual/randomGenerator/randomGeneratorVirtualDeviceLogic.tssrc/device/protocol/virtual/genericVirtualDeviceFactory.tssrc/device/protocol/virtual/virtualDeviceFactory.tssrc/device/protocol/slvCtrlPlus/slvCtrlPlusDevice.tssrc/device/protocol/virtual/virtualDeviceLogic.tssrc/device/protocol/buttplugIo/buttplugIoDevice.tssrc/device/protocol/slvCtrlPlus/genericSlvCtrlPlusDevice.tssrc/device/protocol/virtual/virtualDevice.tssrc/device/protocol/virtual/virtualDeviceProvider.tssrc/device/device.tssrc/device/protocol/virtual/audio/ttsVirtualDeviceLogic.tssrc/device/protocol/zc95/zc95Device.tstests/unit/device/testDevice.ts
📚 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 (12)
src/device/protocol/virtual/display/displayVirtualDeviceLogic.ts (1)
tests/unit/device/protocol/virtual/display/displayVirtualDevice.spec.ts (3)
createDevice(6-42)device(37-41)createDevice(8-20)
src/device/protocol/virtual/audio/piperVirtualDeviceLogic.ts (6)
src/device/attribute/strDeviceAttribute.ts (1)
StrDeviceAttribute(6-36)src/device/attribute/boolDeviceAttribute.ts (1)
BoolDeviceAttribute(6-36)src/device/protocol/virtual/audio/piperVirtualDeviceConfig.ts (1)
PiperVirtualDeviceConfig(9-9)src/logging/Logger.ts (1)
Logger(9-19)src/util/process.ts (1)
spawnProcess(27-57)src/util/devNullStream.ts (1)
DevNullStream(4-29)
src/device/protocol/virtual/randomGenerator/randomGeneratorVirtualDeviceLogic.ts (5)
src/device/attribute/intDeviceAttribute.ts (1)
IntDeviceAttribute(8-42)src/device/protocol/virtual/randomGenerator/randomGeneratorVirtualDeviceConfig.ts (1)
RandomGeneratorVirtualDeviceConfig(9-9)src/settings/deviceSource.ts (1)
config(30-32)src/settings/knownDevice.ts (1)
config(56-58)src/serviceProvider/deviceServiceProvider.ts (1)
GenericVirtualDeviceFactory(105-109)
src/device/protocol/virtual/genericVirtualDeviceFactory.ts (5)
src/device/protocol/virtual/virtualDeviceLogic.ts (1)
ExtractConfig(10-13)src/device/protocol/virtual/virtualDeviceLogicFactory.ts (1)
VirtualDeviceLogicFactory(5-10)src/device/protocol/virtual/virtualDeviceFactory.ts (1)
VirtualDeviceFactory(4-6)src/factory/dateFactory.ts (1)
DateFactory(1-7)src/schemaValidation/JsonSchemaValidatorFactory.ts (1)
JsonSchemaValidatorFactory(7-24)
src/device/protocol/virtual/virtualDeviceLogic.ts (2)
src/device/device.ts (1)
DeviceAttributes(7-7)src/device/deviceConfig.ts (2)
AnyDeviceConfig(7-7)NoDeviceConfig(5-5)
src/device/protocol/buttplugIo/buttplugIoDevice.ts (2)
src/device/device.ts (1)
ExtractAttributeValue(9-9)src/device/attribute/deviceAttribute.ts (2)
value(50-52)value(54-56)
src/device/protocol/slvCtrlPlus/genericSlvCtrlPlusDevice.ts (1)
src/device/protocol/slvCtrlPlus/slvCtrlPlusDevice.ts (1)
SlvCtrlPlusDeviceAttributes(8-8)
src/device/protocol/virtual/virtualDevice.ts (3)
src/device/deviceConfig.ts (1)
AnyDeviceConfig(7-7)src/device/protocol/virtual/virtualDeviceLogic.ts (2)
ExtractAttributes(5-8)ExtractConfig(10-13)src/device/device.ts (1)
ExtractAttributeValue(9-9)
src/device/protocol/virtual/virtualDeviceProvider.ts (1)
src/device/protocol/virtual/virtualDeviceFactory.ts (1)
VirtualDeviceFactory(4-6)
src/device/device.ts (2)
src/device/deviceConfig.ts (2)
AnyDeviceConfig(7-7)NoDeviceConfig(5-5)src/device/attribute/deviceAttribute.ts (2)
value(50-52)value(54-56)
src/device/protocol/virtual/audio/ttsVirtualDeviceLogic.ts (2)
src/device/protocol/virtual/audio/ttsVirtualDeviceConfig.ts (1)
TtsVirtualDeviceConfig(8-8)src/logging/Logger.ts (1)
Logger(9-19)
tests/unit/device/testDevice.ts (1)
src/device/device.ts (2)
ExtractAttributeValue(9-9)DeviceAttributes(7-7)
🔇 Additional comments (27)
src/device/protocol/virtual/randomGenerator/randomGeneratorVirtualDeviceLogic.ts (4)
1-15: LGTM: Type safety improvements.The addition of the typed config import and generic type parameters strengthens type safety and aligns with the broader pattern of per-type device logic introduced in this PR.
26-28: LGTM: Cleaner getter pattern.The conversion from
getRefreshInterval()method torefreshIntervalgetter is more idiomatic for simple property access.
30-35: LGTM: Correct random number generation.The random number generation formula correctly produces integers in the inclusive range
[min, max], and the typed device parameter improves type safety.
37-45: LGTM: Well-structured attribute configuration.The
configureAttributes()method correctly creates a read-only integer attribute for the random value, following the expected pattern for virtual device configuration.src/device/protocol/virtual/virtualDeviceLogic.ts (2)
5-13: Well-designed type extraction utilities.The
ExtractAttributesandExtractConfigtype helpers provide a clean way to extract type information from device logic classes. This enables type-safe factory patterns and device handling throughout the codebase.
15-30: Solid refactor from interface to abstract class.The conversion to an abstract class with generics provides a robust foundation for virtual device implementations:
- Type-safe attribute and config handling through generics
- Shared config field and constructor reduce boilerplate
- Clear contract via abstract methods
This pattern enables the unified factory approach while maintaining type safety.
src/device/protocol/virtual/audio/piperVirtualDeviceLogic.ts (3)
20-36: LGTM: Clean class structure.The class properly extends
VirtualDeviceLogicwith appropriate type parameters, uses static constants for attribute names, and initializes a child logger for better observability.
155-174: LGTM: Appropriate attribute configuration.The attributes are well-designed:
textis write-only, allowing users to send text for synthesisqueuingdefaults to false with read-write access, giving users control over whether utterances queue or interruptThe 50ms refresh interval is appropriate for minimizing TTS latency, aligning with the PR objectives to reduce latency on low-power devices.
114-132: No changes needed. The code correctly usesgetStateas a property accessor, not a method call.getStateis defined as a getter in theDeviceclass (public get getState(): DeviceState), so the comparisondevice.getState === DeviceState.erroris valid and operates on the returned enum value, not a function reference. This pattern is consistently used throughout the codebase (e.g.,deviceProvider.ts).Regarding error handling:
startPiper()includes internal try-catch logic, and failures are gracefully handled via the undefined check on line 123. The current design pattern is appropriate.Likely an incorrect or invalid review comment.
src/device/protocol/virtual/display/displayVirtualDeviceLogic.ts (1)
10-32: No issues detected. The refactoring correctly implements the delegation pattern whereVirtualDevice.getRefreshIntervaldelegates tothis.deviceLogic.refreshInterval. The public API remains unchanged, and all existing code (tests,deviceProvider.ts) continues to work without modification.src/device/protocol/virtual/virtualDeviceFactory.ts (1)
4-6: No changes needed. The<any>return type inVirtualDeviceFactory.create()is appropriate for a polymorphic factory that creates different device types at runtime based on configuration. The actual device logic type is preserved internally, and all call sites use the device generically without requiring compile-time type information. Runtime config validation ensures type safety.src/device/protocol/virtual/audio/ttsVirtualDeviceLogic.ts (1)
71-79: LGTM!The
say.speakcallback properly handles errors by logging them, and the.catch()pattern for the nestedsetAttributepromise is appropriate here since this is within a callback context where async/await isn't directly usable.src/device/device.ts (3)
17-20: LGTM!The
DeviceErrortype is well-structured withreasonandoccurredAtfields, providing useful context for error tracking and debugging.
115-119: LGTM!The updated generic signatures for
getAttributeandsetAttributeprovide strong type safety while maintaining flexibility through theTAttributesandTConfigtype parameters. The use ofExtractAttributeValuehelper ensures correct value type inference.
57-58: No action needed. The config exposure does not present a security risk in the current implementation.The device config schemas (
randomGeneratorVirtualDeviceConfig,ttsVirtualDeviceConfig,piperVirtualDeviceConfig) contain only non-sensitive configuration values: numeric parameters, voice selection names, and file paths to models. None contain API keys, credentials, or sensitive tokens. The Device class uses a secure opt-in pattern with@Exclude()at the class level and explicit@Expose()decorators for each field, which properly restricts serialization to intended fields.Likely an incorrect or invalid review comment.
src/device/protocol/slvCtrlPlus/slvCtrlPlusDevice.ts (2)
11-14: LGTM!The class generics are well-structured, extending the base
Deviceclass with proper type constraints. The default types (SlvCtrlPlusDeviceAttributesandNoDeviceConfig) provide sensible fallbacks.
24-28: LGTM!Constructor correctly accepts the new
configparameter and passes it through to the base class, maintaining consistency with the updatedDeviceconstructor signature.src/device/protocol/slvCtrlPlus/genericSlvCtrlPlusDevice.ts (2)
34-34: LGTM!Passing an empty object
{}as the config argument is correct sinceGenericSlvCtrlPlusDeviceusesNoDeviceConfig, which is defined asStatic<NoDeviceConfigSchema>(an empty object type).
65-68: LGTM!The updated type constraint using
ExtractAttributeValue<SlvCtrlPlusDeviceAttributes[K]>aligns with the broader refactoring and provides correct type inference for attribute values.src/device/protocol/virtual/virtualDevice.ts (2)
36-46: Good error handling pattern.The try/catch wrapper properly captures errors from
deviceLogic.refreshData(), updates the device state toerror, and populateserrorInfowith context. This aligns with the PR's goal of improving error tracking.Minor: Missing semicolon after the
errorInfoobject literal on line 44.this.errorInfo = { reason: (e as Error).message ?? 'Unknown error', occurredAt: new Date(), - }+ };
52-74: LGTM!The
setAttributeimplementation correctly uses the new generic constraints withExtractAttributes<TLogic>andExtractAttributeValue. The defensive null/undefined check on line 61 is appropriate given that attributes could be partial.src/device/protocol/buttplugIo/buttplugIoDevice.ts (1)
3-3: LGTM! Type refactoring is consistent.The migration from
AttributeValuetoExtractAttributeValueand the addition of the config parameter align with the broader device type system refactoring.Also applies to: 36-36, 48-48
tests/unit/device/testDevice.ts (1)
1-1: LGTM! Test device updated correctly.The test device correctly adopts the new type system with
ExtractAttributeValueand the updated Device constructor signature.Also applies to: 11-11, 22-22
src/device/protocol/virtual/virtualDeviceProvider.ts (4)
33-41: LGTM! Proper error handling for async discovery.The initialization correctly sets up periodic device discovery with appropriate error handling.
1-9: LGTM! Factory refactoring is consistent.The migration from
DelegatedVirtualDeviceFactorytoVirtualDeviceFactoryaligns with the factory pattern improvements described in the PR objectives.Also applies to: 18-18, 24-24
44-49: LGTM! Good defensive programming.The early return when settings are undefined prevents errors during initialization and makes the code more robust.
53-58: LGTM! Device removal logic correctly implemented.The addition of device removal logic ensures that devices removed from configuration are properly cleaned up.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/device/protocol/virtual/virtualDevice.ts (1)
54-76: Fix stuck “busy” state when attribute is missing (and simplify Promise).Line 59 sets
busy, but Line 63-68 rejects without restoring state, so the device can remainbusyforever.Proposed fix
public async setAttribute< K extends keyof ExtractAttributes<TLogic>, V extends ExtractAttributeValue<ExtractAttributes<TLogic>[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 || null === 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);- });+ this.state = DeviceState.busy;+ try {+ const attribute = this.attributes[attributeName];++ if (undefined === attribute || null === 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;+ } catch (e: unknown) {+ this.state = DeviceState.error;+ const reason = e instanceof Error ? e.message : String(e);+ this.errorInfo = { reason: reason || 'Unknown error', occurredAt: new Date() };+ throw e;+ } }
🤖 Fix all issues with AI agents
In
@src/device/protocol/virtual/randomGenerator/randomGeneratorVirtualDeviceConfig.ts:
- Around line 3-17: Update randomGeneratorVirtualDeviceConfigSchema to enforce
integer bounds and correct the error text: change the schema fields in
Type.Object from Type.Number() to Type.Integer() for min and max, and modify the
Decode error message thrown in .Decode from "min (...) must be less than or
equal to max (...)" to "min (...) must be less than max (...)" to match the
existing check (value.min >= value.max) used to enforce min < max; leave .Encode
as-is.
In @src/device/protocol/virtual/virtualDeviceProvider.ts:
- Around line 43-70: The removal path doesn't fully clean up state: update
removeDevice(device: Device) to also delete the entry from this.deviceUpdaters
(clear any interval then this.deviceUpdaters.delete(device.id)) and remove the
id from this.managedDevices (this.managedDevices.delete(device.id)) so the maps
don't leak and the id can be re-added later; also apply the same cleanup
wherever devices are removed (e.g., the alternate removal logic around lines
~90-100) and ensure discoverVirtualDevices can re-add previously removed ids by
relying on managedDevices being cleared on removal.
🧹 Nitpick comments (2)
src/device/protocol/virtual/virtualDevice.ts (1)
36-48: Make refreshData() errorInfo robust for non-Error throws.
(e as Error).messageis undefined if someone throws a string/object. Preferinstanceof Error/String(e).Proposed fix
public async refreshData(): Promise<void> { try { return await this.deviceLogic.refreshData(this); } catch (e: unknown) { this.state = DeviceState.error; + const reason = e instanceof Error ? e.message : String(e); this.errorInfo = { - reason: (e as Error).message ?? 'Unknown error',+ reason: reason || 'Unknown error', occurredAt: new Date(), }; throw e; } }src/device/protocol/virtual/randomGenerator/randomGeneratorVirtualDeviceLogic.ts (1)
12-35: Good: refreshData() uses concrete VirtualDevice typing.This matches the project typing constraint around
device.setAttribute(...)inference. Based on learnings, this is the right pattern.
Also, since the config schema already enforcesmin < max, the constructor check is likely redundant.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/device/protocol/virtual/audio/piperVirtualDeviceLogic.tssrc/device/protocol/virtual/randomGenerator/randomGeneratorVirtualDeviceConfig.tssrc/device/protocol/virtual/randomGenerator/randomGeneratorVirtualDeviceLogic.tssrc/device/protocol/virtual/virtualDevice.tssrc/device/protocol/virtual/virtualDeviceProvider.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/device/protocol/virtual/audio/piperVirtualDeviceLogic.ts
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: heavyrubberslave
Repo: SlvCtrlPlus/slvctrlplus-server PR: 59
File: src/device/protocol/virtual/audio/ttsVirtualDeviceLogic.ts:37-37
Timestamp: 2026-01-11T11:29:34.943Z
Learning: In VirtualDeviceLogic implementations, the refreshData method should use the concrete class type (e.g., `VirtualDevice<TtsVirtualDeviceLogic>`) rather than `VirtualDevice<this>`, as the polymorphic this-type breaks TypeScript's conditional type inference for ExtractAttributes when calling device.setAttribute().
📚 Learning: 2025-12-30T08:02:18.388Z
Learnt from: heavyrubberslave
Repo: SlvCtrlPlus/slvctrlplus-server PR: 57
File: src/index.ts:89-108
Timestamp: 2025-12-30T08:02:18.388Z
Learning: Express 5 will automatically forward rejected promises and thrown errors from route handlers and middleware to your error-handling middleware, so you generally don’t need manual try/catch blocks or .catch(next). This applies to any route handler or middleware that returns a promise. Ensure you still have a proper error-handling middleware (err, req, res, next) in place and avoid relying on silent rejections. This guidance is applicable across TypeScript files in the project (src and beyond) and should be especially considered for routes and middleware that return promises.
Applied to files:
src/device/protocol/virtual/randomGenerator/randomGeneratorVirtualDeviceConfig.tssrc/device/protocol/virtual/virtualDevice.tssrc/device/protocol/virtual/randomGenerator/randomGeneratorVirtualDeviceLogic.tssrc/device/protocol/virtual/virtualDeviceProvider.ts
📚 Learning: 2026-01-11T11:29:34.943Z
Learnt from: heavyrubberslave
Repo: SlvCtrlPlus/slvctrlplus-server PR: 59
File: src/device/protocol/virtual/audio/ttsVirtualDeviceLogic.ts:37-37
Timestamp: 2026-01-11T11:29:34.943Z
Learning: In VirtualDeviceLogic implementations, use the concrete class type (e.g., VirtualDevice<TtsVirtualDeviceLogic>) instead of VirtualDevice<this> to preserve TypeScript's conditional type inference for ExtractAttributes when calling device.setAttribute(). Apply this pattern to all VirtualDeviceLogic implementations to maintain correct typing; target files under the virtual audio/video/device logic directories.
Applied to files:
src/device/protocol/virtual/randomGenerator/randomGeneratorVirtualDeviceConfig.tssrc/device/protocol/virtual/virtualDevice.tssrc/device/protocol/virtual/randomGenerator/randomGeneratorVirtualDeviceLogic.tssrc/device/protocol/virtual/virtualDeviceProvider.ts
📚 Learning: 2026-01-11T11:15:28.011Z
Learnt from: heavyrubberslave
Repo: SlvCtrlPlus/slvctrlplus-server PR: 59
File: src/device/protocol/virtual/virtualDeviceProvider.ts:61-69
Timestamp: 2026-01-11T11:15:28.011Z
Learning: In files matching virtualDeviceProvider.ts, ensure the internal maps (e.g., managedDevices or attemptedDevices) track devices for which initialization has been attempted, including both successes and failures. Do not retry initialization for devices that failed previously; keep them in the map to avoid repeated failed attempts during subsequent discovery cycles. This pattern helps prevent repeated, wasted work on misconfigured or unavailable devices and should be reviewed whenever a new provider implements similar initialization logic.
Applied to files:
src/device/protocol/virtual/virtualDeviceProvider.ts
🧬 Code graph analysis (3)
src/device/protocol/virtual/virtualDevice.ts (2)
src/device/deviceConfig.ts (1)
AnyDeviceConfig(7-7)src/device/protocol/virtual/virtualDeviceLogic.ts (2)
ExtractAttributes(5-8)ExtractConfig(10-13)
src/device/protocol/virtual/randomGenerator/randomGeneratorVirtualDeviceLogic.ts (1)
src/device/protocol/virtual/randomGenerator/randomGeneratorVirtualDeviceConfig.ts (1)
RandomGeneratorVirtualDeviceConfig(20-20)
src/device/protocol/virtual/virtualDeviceProvider.ts (2)
src/device/protocol/virtual/virtualDeviceFactory.ts (1)
VirtualDeviceFactory(4-6)src/settings/settingsManager.ts (1)
SettingsManager(19-127)
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/device/protocol/virtual/audio/piperVirtualDeviceLogic.ts:
- Around line 20-36: The Piper child process started in startPiper() is never
terminated, so add a cleanup override on PiperVirtualDeviceLogic to stop
playback and terminate the piperProcess: implement a public destroy(): void {
... } (or an appropriate lifecycle method if you add one to VirtualDeviceLogic)
that calls this.stopPlayback(), ends this.piperProcess.stdin if present, sends a
kill (SIGTERM) to this.piperProcess, and clears this.piperProcess = undefined;
ensure the method references PiperVirtualDeviceLogic, startPiper, stopPlayback,
and piperProcess so the process is always cleaned when the device is removed.
🧹 Nitpick comments (4)
src/device/protocol/virtual/audio/piperVirtualDeviceLogic.ts (4)
54-56: Logger format string won't be interpolated.The Logger interface uses
error(msg: string, context?: unknown)rather than printf-style formatting. The%splaceholder won't be replaced.Suggested fix
piperProcess.stderr.on('data', (data: Buffer) => { - this.logger.error('Piper stderr: %s', data.toString());+ this.logger.error(`Piper stderr: ${data.toString().trim()}`); });
83-87: Hardcoded sample rate may not match all Piper models.Piper voice models output audio at different sample rates (commonly 16000 or 22050 Hz). A mismatch will cause audio distortion or incorrect playback speed. Consider making
sampleRateconfigurable viaPiperVirtualDeviceConfig.Suggested approach
this.speaker = new Speaker({ channels: 1, bitDepth: 16, - sampleRate: 22050+ sampleRate: this.config.sampleRate ?? 22050 });This requires adding
sampleRateas an optional property to the config schema.
97-106: Consider cleaning up DevNullStream explicitly.The
DevNullStreaminstance is created without storing a reference, so it can't be explicitly destroyed. While it will eventually be garbage collected after theidleevent, callingdevNull.destroy()after unpipe in the idle handler would ensure cleaner resource management.Suggested improvement
this.piperProcess.stdout.unpipe(); const devNull = new DevNullStream(500); devNull.on('idle', () => { this.speakerCoolDown = false; this.piperProcess?.stdout.unpipe(); + devNull.destroy(); }); this.piperProcess.stdout.pipe(devNull);
146-154: Consider handling stdin backpressure.
stdin.write()returnsfalsewhen the internal buffer is full. With rapid text submissions, ignoring backpressure could lead to increased memory usage. For robustness, consider handling the'drain'event or logging when write returns false.Suggested improvement
if (!this.piperProcess.stdin.destroyed) { this.logger.debug(`Send to piper process: ${text}`); - this.piperProcess.stdin.write(text + '\n');+ const canWrite = this.piperProcess.stdin.write(text + '\n');+ if (!canWrite) {+ this.logger.warn('Piper stdin buffer is full, backpressure applied');+ } await device.setAttribute('text', undefined); } else {
📜 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 (3)
package.jsonsrc/device/protocol/virtual/audio/piperVirtualDeviceLogic.tssrc/device/protocol/virtual/randomGenerator/randomGeneratorVirtualDeviceConfig.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/device/protocol/virtual/randomGenerator/randomGeneratorVirtualDeviceConfig.ts
- package.json
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: heavyrubberslave
Repo: SlvCtrlPlus/slvctrlplus-server PR: 59
File: src/device/protocol/virtual/audio/ttsVirtualDeviceLogic.ts:37-37
Timestamp: 2026-01-11T11:29:34.943Z
Learning: In VirtualDeviceLogic implementations, the refreshData method should use the concrete class type (e.g., `VirtualDevice<TtsVirtualDeviceLogic>`) rather than `VirtualDevice<this>`, as the polymorphic this-type breaks TypeScript's conditional type inference for ExtractAttributes when calling device.setAttribute().
📚 Learning: 2026-01-11T11:29:34.943Z
Learnt from: heavyrubberslave
Repo: SlvCtrlPlus/slvctrlplus-server PR: 59
File: src/device/protocol/virtual/audio/ttsVirtualDeviceLogic.ts:37-37
Timestamp: 2026-01-11T11:29:34.943Z
Learning: In VirtualDeviceLogic implementations, use the concrete class type (e.g., VirtualDevice<TtsVirtualDeviceLogic>) instead of VirtualDevice<this> to preserve TypeScript's conditional type inference for ExtractAttributes when calling device.setAttribute(). Apply this pattern to all VirtualDeviceLogic implementations to maintain correct typing; target files under the virtual audio/video/device logic directories.
Applied to files:
src/device/protocol/virtual/audio/piperVirtualDeviceLogic.ts
📚 Learning: 2025-12-30T08:02:18.388Z
Learnt from: heavyrubberslave
Repo: SlvCtrlPlus/slvctrlplus-server PR: 57
File: src/index.ts:89-108
Timestamp: 2025-12-30T08:02:18.388Z
Learning: Express 5 will automatically forward rejected promises and thrown errors from route handlers and middleware to your error-handling middleware, so you generally don’t need manual try/catch blocks or .catch(next). This applies to any route handler or middleware that returns a promise. Ensure you still have a proper error-handling middleware (err, req, res, next) in place and avoid relying on silent rejections. This guidance is applicable across TypeScript files in the project (src and beyond) and should be especially considered for routes and middleware that return promises.
Applied to files:
src/device/protocol/virtual/audio/piperVirtualDeviceLogic.ts
🧬 Code graph analysis (1)
src/device/protocol/virtual/audio/piperVirtualDeviceLogic.ts (6)
src/device/attribute/strDeviceAttribute.ts (1)
StrDeviceAttribute(6-36)src/device/attribute/boolDeviceAttribute.ts (1)
BoolDeviceAttribute(6-36)src/device/protocol/virtual/audio/piperVirtualDeviceConfig.ts (1)
PiperVirtualDeviceConfig(9-9)src/logging/Logger.ts (1)
Logger(9-19)src/util/process.ts (1)
spawnProcess(27-57)src/util/devNullStream.ts (1)
DevNullStream(4-29)
🔇 Additional comments (3)
src/device/protocol/virtual/audio/piperVirtualDeviceLogic.ts (3)
157-172: LGTM!Attribute configuration is well-structured with appropriate modifiers and follows the established patterns.
174-176: LGTM!A 50ms refresh interval provides responsive text-to-speech processing with acceptable overhead.
117-119: No issues found. The code at line 117 is correct.
getStateis defined as a getter property (public get getState(): DeviceState) in the Device class. The comparisondevice.getState === DeviceState.errorcorrectly accesses the getter property and compares its value to the enum. This is the standard TypeScript pattern for using getters and does not require parentheses.Likely an incorrect or invalid review comment.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @package.json:
- Line 7: The package.json lists an invalid dependency version for
"@sinclair/typebox" (^0.34.47) which doesn't exist; update the dependency entry
for "@sinclair/typebox" to a published version such as "^0.34.45" (or another
available version), save package.json, and run your package manager
(npm/yarn/pnpm install) to verify the install succeeds.
📜 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 (1)
package.json
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: heavyrubberslave
Repo: SlvCtrlPlus/slvctrlplus-server PR: 59
File: src/device/protocol/virtual/audio/ttsVirtualDeviceLogic.ts:37-37
Timestamp: 2026-01-11T11:29:34.943Z
Learning: In VirtualDeviceLogic implementations, the refreshData method should use the concrete class type (e.g., `VirtualDevice<TtsVirtualDeviceLogic>`) rather than `VirtualDevice<this>`, as the polymorphic this-type breaks TypeScript's conditional type inference for ExtractAttributes when calling device.setAttribute().
🔇 Additional comments (1)
package.json (1)
26-26: GitHub tarball URL is currently accessible; consider supply-chain trade-offs.Using a GitHub release tarball for this native module fork is valid for prebuilt binaries, but be aware:
- URL availability: Currently accessible (verified via HEAD request), but GitHub release assets can be deleted in future.
- No npm audit: Security vulnerabilities in this fork won't appear in
npm audit—requires manual monitoring.- Manual updates: Unlike npm packages, this dependency won't auto-update; version bumps need explicit changes.
The pinned v0.1.0 is good for reproducibility.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/test.yml:
- Around line 48-52: The "Install ALSA runtime library" step's fallback shell
command is missing sudo on the second apt-get call, causing a permission error
if the first install fails; update the run command so the fallback uses sudo as
well (i.e., ensure the second command mirrors the first by prepending sudo to
apt-get install -y libasound2t64) in the step named "Install ALSA runtime
library".
🧹 Nitpick comments (2)
.github/workflows/test.yml (2)
32-37: Consider moving Checkout before the prebuild compatibility check.The
Checkout Codestep (line 83) occurs after the Linux pre-checks that download and execute external binaries. While the pre-checks don't require repo code, this ordering has implications:
- If pre-checks fail, the repo is never checked out, which could complicate debugging.
- Downloading and executing external
.nodebinaries before checkout is unconventional and could be a security consideration.If the pre-checks are meant to validate the environment before running tests, consider whether they should gate the checkout or run afterward as informational diagnostics.
Also applies to: 83-84
85-92: LGTM!The build steps are straightforward. Adding the coverage step is a good practice for CI.
Consider adding npm dependency caching (using
actions/cacheor the built-in caching inactions/setup-node) to speed up builds across the 15-job matrix, though this is optional.
📜 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 (1)
.github/workflows/test.yml
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: heavyrubberslave
Repo: SlvCtrlPlus/slvctrlplus-server PR: 59
File: src/device/protocol/virtual/audio/ttsVirtualDeviceLogic.ts:37-37
Timestamp: 2026-01-11T11:29:45.300Z
Learning: In VirtualDeviceLogic implementations, the refreshData method should use the concrete class type (e.g., `VirtualDevice<TtsVirtualDeviceLogic>`) rather than `VirtualDevice<this>`, as the polymorphic this-type breaks TypeScript's conditional type inference for ExtractAttributes when calling device.setAttribute().
🔇 Additional comments (2)
.github/workflows/test.yml (2)
53-82: Prebuild load failure will fail the workflow—is this intentional?Line 74 attempts to load the prebuild binary:
LD_DEBUG=libs node -e "require('./$PREBUILD_PATH')"If this fails (e.g., missing dependencies, ABI mismatch), the entire workflow step fails. This may be desirable as a gating check, but consider:
- If the intent is diagnostic-only, add
|| trueto prevent workflow failure.- If this is a required pre-flight check, the current behavior is correct—but consider adding a clearer error message before failure.
Additionally, when the prebuild is not found (lines 76-80), the step prints a message but doesn't exit with an error code, so the workflow continues. This asymmetry between "found but can't load" (fails) vs "not found" (continues) may be unintentional.
11-29: Matrix strategy is well-structured for cross-platform testing.The
fail-fast: falsesetting ensures all platform/version combinations run to completion, which is valuable for comprehensive compatibility testing. The nestedcomboobject cleanly associates runner labels with platform identifiers. Node.js 24.x is the current LTS line and production-ready, so the version matrix is appropriate.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/test.yml:
- Around line 48-52: The workflow step named "Install ALSA runtime library" uses
a fallback apt-get install without sudo which will fail due to permission
errors; update the fallback command to include sudo (i.e., change the second
`apt-get install -y libasound2t64` to `sudo apt-get install -y libasound2t64`)
so both install attempts run with elevated privileges and avoid failing on
systems where the primary package is absent.
🧹 Nitpick comments (3)
.github/workflows/test.yml (3)
38-47: ARM64 package name variant may be missed.The check for
libasound2:amd64won't match on ARM64 runners where the package is namedlibasound2:arm64. Consider using a more generic pattern.Suggested improvement
- elif dpkg -l | grep -q "libasound2:amd64"; then+ elif dpkg -l | grep -qE "libasound2:(amd64|arm64)"; thenOr simply check for
libasound2without the architecture suffix:- elif dpkg -l | grep -q "libasound2:amd64"; then+ elif dpkg -l | grep -q "libasound2[[:space:]]"; then
53-67: Redundantapt-get updatecall.Line 66 runs
sudo apt-get updateagain, but this was already executed on line 51. This adds unnecessary CI time.Proposed fix
EOF # Optional: Install alsa-utils to verify the configuration - sudo apt-get update && sudo apt-get install -y alsa-utils+ sudo apt-get install -y alsa-utils aplay -L | grep default
68-97: Hardcoded speaker version may drift from package.json.The prebuild check downloads
v0.1.0directly from GitHub releases. If the dependency version inpackage.jsonchanges, this check will become stale and potentially misleading.Consider extracting the version as a workflow variable or accepting that this is an intentional pinned compatibility check. If intentional, a comment clarifying this would help maintainability:
Suggested clarification
- name: Check node-speaker prebuild compatibility (Linux only) if: runner.os == 'Linux' run: | + # NOTE: Version pinned to match SlvCtrlPlus/node-speaker fork release echo "Downloading node-speaker tarball..." wget https://github.com/SlvCtrlPlus/node-speaker/releases/download/v0.1.0/speaker-v0.1.0.tgz
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/test.yml
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: heavyrubberslave
Repo: SlvCtrlPlus/slvctrlplus-server PR: 59
File: src/device/protocol/virtual/audio/ttsVirtualDeviceLogic.ts:37-37
Timestamp: 2026-01-11T11:29:45.300Z
Learning: In VirtualDeviceLogic implementations, the refreshData method should use the concrete class type (e.g., `VirtualDevice<TtsVirtualDeviceLogic>`) rather than `VirtualDevice<this>`, as the polymorphic this-type breaks TypeScript's conditional type inference for ExtractAttributes when calling device.setAttribute().
🔇 Additional comments (3)
.github/workflows/test.yml (3)
11-26: LGTM!The matrix strategy is well-structured with
fail-fast: falseto ensure all platform combinations are tested even if one fails. The use of acomboobject to groupruns-onandplatformvalues is a clean approach.
32-37: LGTM!Node.js setup before checkout is valid since
setup-nodedoesn't require repository files. The version print step is a useful diagnostic.
98-107: LGTM!The step ordering is correct: checkout → install → lint → coverage. The addition of the coverage step aligns with the PR's testing objectives.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/controller/patchDeviceController.ts (1)
29-35: Fix broken error handling: incorrect response chaining and missing return.Two issues here:
res.send(...).sendStatus(500)is incorrect —send()already sends headers, so chainingsendStatus()will throw "Cannot set headers after they are sent to the client."- Missing
returnafter the catch block causesres.sendStatus(202)to execute even after an error.🐛 Proposed fix
try { await this.deviceUpdater.update(device, req.body as DeviceData); } catch (err: unknown) { - res.send((err as Error).message).sendStatus(500);+ res.status(500).send((err as Error).message);+ return; } res.sendStatus(202);
🤖 Fix all issues with AI agents
In @.github/workflows/test.yml:
- Around line 38-43: The workflow step "Install ALSA runtime library" has a
fallback command missing sudo; update the run block so the fallback apt-get
install uses sudo as well (i.e., ensure the second command is "sudo apt-get
install -y libasound2t64" when the primary install of libasound2 fails) so both
install attempts run with elevated privileges in the "Install ALSA runtime
library" step.
- Around line 56-58: Remove the redundant "apt-get update" (it's already run
earlier) and make the aplay check non-failing: drop the second "sudo apt-get
update &&" and replace "aplay -L | grep default" with a tolerant check such as
'aplay -L | grep -iE "default|null" || true' so the step does not fail if
"default" is absent but a null device exists.
🧹 Nitpick comments (1)
.github/workflows/test.yml (1)
32-37: Consider reordering: checkout before setup-node.
actions/setup-node@v4is invoked beforeactions/checkout@v4. While this works, the conventional order is checkout first, then setup-node. This enables thecacheoption (e.g.,cache: 'npm') which requirespackage-lock.jsonfrom the repository.Suggested reorder
steps: + - name: Checkout Code+ uses: actions/checkout@v4 - name: Use Node.js ${{ matrix.node-version }} uses: actions/setup-node@v4 with: node-version: ${{ matrix.node-version }} + cache: 'npm' - name: Print Node.js version run: node --version - name: Install ALSA runtime library ... - - name: Checkout Code- uses: actions/checkout@v4Also applies to: 59-60
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/test.ymlsrc/controller/automation/createScriptController.tssrc/controller/automation/deleteScriptController.tssrc/controller/automation/getScriptController.tssrc/controller/getDeviceController.tssrc/controller/patchDeviceController.ts
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: heavyrubberslave
Repo: SlvCtrlPlus/slvctrlplus-server PR: 59
File: src/device/protocol/virtual/audio/ttsVirtualDeviceLogic.ts:37-37
Timestamp: 2026-01-11T11:29:45.300Z
Learning: In VirtualDeviceLogic implementations, the refreshData method should use the concrete class type (e.g., `VirtualDevice<TtsVirtualDeviceLogic>`) rather than `VirtualDevice<this>`, as the polymorphic this-type breaks TypeScript's conditional type inference for ExtractAttributes when calling device.setAttribute().
📚 Learning: 2025-12-30T08:02:18.388Z
Learnt from: heavyrubberslave
Repo: SlvCtrlPlus/slvctrlplus-server PR: 57
File: src/index.ts:89-108
Timestamp: 2025-12-30T08:02:18.388Z
Learning: Express 5 will automatically forward rejected promises and thrown errors from route handlers and middleware to your error-handling middleware, so you generally don’t need manual try/catch blocks or .catch(next). This applies to any route handler or middleware that returns a promise. Ensure you still have a proper error-handling middleware (err, req, res, next) in place and avoid relying on silent rejections. This guidance is applicable across TypeScript files in the project (src and beyond) and should be especially considered for routes and middleware that return promises.
Applied to files:
src/controller/automation/createScriptController.tssrc/controller/automation/deleteScriptController.tssrc/controller/automation/getScriptController.tssrc/controller/getDeviceController.tssrc/controller/patchDeviceController.ts
🧬 Code graph analysis (3)
src/controller/automation/createScriptController.ts (1)
src/automation/utils.ts (1)
isValidAutomationScriptFileName(3-4)
src/controller/automation/deleteScriptController.ts (1)
src/automation/utils.ts (1)
isValidAutomationScriptFileName(3-4)
src/controller/automation/getScriptController.ts (1)
src/automation/utils.ts (1)
isValidAutomationScriptFileName(3-4)
🔇 Additional comments (7)
src/controller/getDeviceController.ts (1)
22-22: LGTM with minor note on type assertion pattern.The
as stringcast is acceptable here since Express route parameters are strings when the route matches. The null check on line 24 handles the case where no device is found.src/controller/automation/getScriptController.ts (1)
20-25: LGTM!The string casts are consistent with the pattern applied across other controllers, and the validation check provides proper input sanitization before repository access.
src/controller/automation/createScriptController.ts (1)
27-32: LGTM!The explicit string casts align with the pattern in the other automation controllers and ensure type compatibility with the validation and repository methods.
src/controller/patchDeviceController.ts (1)
22-22: LGTM on the type cast.The explicit string cast for deviceId is consistent with the pattern applied across other controllers.
src/controller/automation/deleteScriptController.ts (1)
20-25: LGTM!The string casts are consistent with the other automation controllers, and the validation check ensures proper input handling before the delete operation.
.github/workflows/test.yml (2)
61-68: LGTM!The install, linting, and coverage steps follow standard Node.js CI patterns.
14-25: No action needed—all runners and Node.js versions are available. Node.js 24.x is released and GA (latest v24.12.0),ubuntu-24.04-armis available for public repos (GA since August 2025), andmacos-15-intelis documented in GitHub's hosted runners reference. The matrix configuration is valid.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/test.yml:
- Around line 38-53: The current ALSA config sets ctl.!default to "type hw card
0", which can fail on CI without card 0; replace that block so ctl.!default uses
a null control (e.g., ctl.!default { type null }) or otherwise avoid referencing
card 0 so ALSA control operations don't error during tests; update the snippet
that defines pcm.!default and ctl.!default in the workflow to use the null
control (or remove the ctl block entirely) so control operations are safely
no-ops.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/test.yml
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: heavyrubberslave
Repo: SlvCtrlPlus/slvctrlplus-server PR: 59
File: src/device/protocol/virtual/audio/ttsVirtualDeviceLogic.ts:37-37
Timestamp: 2026-01-11T11:29:45.300Z
Learning: In VirtualDeviceLogic implementations, the refreshData method should use the concrete class type (e.g., `VirtualDevice<TtsVirtualDeviceLogic>`) rather than `VirtualDevice<this>`, as the polymorphic this-type breaks TypeScript's conditional type inference for ExtractAttributes when calling device.setAttribute().
🔇 Additional comments (4)
.github/workflows/test.yml (4)
27-29: LGTM!Clear and descriptive job naming that will make it easy to identify failures in the GitHub Actions UI.
32-37: LGTM!Running
setup-nodebefore checkout is valid since it doesn't depend on repository content. The version print step aids debugging.
54-63: LGTM!The checkout placement after ALSA setup is appropriate since the virtual audio device configuration doesn't depend on repository content. The subsequent npm workflow steps are standard and unchanged.
11-26: Good matrix coverage across Node versions and platforms.The matrix strategy with
fail-fast: falseis well-configured, ensuring all 15 combinations (3 Node versions × 5 platforms) are tested even if some fail. This provides excellent cross-platform and version coverage. Node.js 24.x is available as Active LTS, and the virtual ALSA device setup for Linux is appropriately placed before the npm install step to ensure the prebuild binding compiles correctly in the headless CI environment.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/device/protocol/virtual/audio/piperVirtualDeviceLogic.ts`:
- Around line 174-179: In refreshData within PiperVirtualDeviceLogic, the code
compares the method reference device.getState to DeviceState.error instead of
invoking it; change the check to call device.getState() (i.e., if
(device.getState() === DeviceState.error) return;) so the actual state value is
compared; update any similar checks in this class that reference getState
without parentheses to ensure the method is invoked.
- Around line 92-122: In createSpeakerOptionsFromModelMetadata, don't treat
metadata.num_speakers as audio channels (Piper outputs mono) so always set
channels = 1 and remove use of num_speakers; convert sample_width to bits by
using bitDepth = metadata.sample_width * 8 (keep the existing fallback and
warning behavior); set sampleRate = metadata.audio.sample_rate directly (remove
the erroneous * 8) and keep the fallback/warning if missing; update the logger
messages to reflect these correct meanings (e.g., warn that num_speakers is not
used for channels if you choose to log it).
♻️ Duplicate comments (2)
src/device/protocol/virtual/audio/piperVirtualDeviceLogic.ts (2)
50-90: Add a cleanup method to terminate the Piper process.The
startPiper()method correctly spawns the process, but there's no corresponding cleanup method. When the device is destroyed or stopped, the child process will remain running as an orphan.♻️ Suggested cleanup method
publicdestroy(): void{ this.stopPlayback();if(this.piperProcess){this.piperProcess.stdin.end();this.piperProcess.kill('SIGTERM');this.piperProcess=undefined;this.logger.info('Piper process terminated');}}Ensure this method is called from the appropriate lifecycle hook in
VirtualDeviceorVirtualDeviceLogic.
206-214: Handle stdin failure more robustly.When
piperProcess.stdin.destroyedis true, the code logs an error but doesn't transition the device to an error state or clear the text attribute to prevent retry loops.♻️ Proposed enhancement
if (!this.piperProcess.stdin.destroyed) { this.logger.debug(`Send to piper process: ${text}`); this.piperProcess.stdin.write(text + '\n'); await device.setAttribute('text', undefined); } else { this.logger.error('Piper process stdin is not writable.'); + await device.setAttribute('text', undefined);+ device.setState(DeviceState.error); this.stopPlayback(); this.piperProcess = undefined; }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/device/protocol/virtual/audio/piperVirtualDeviceLogic.ts
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: heavyrubberslave
Repo: SlvCtrlPlus/slvctrlplus-server PR: 59
File: src/device/protocol/virtual/audio/ttsVirtualDeviceLogic.ts:37-37
Timestamp: 2026-01-11T11:29:45.300Z
Learning: In VirtualDeviceLogic implementations, the refreshData method should use the concrete class type (e.g., `VirtualDevice<TtsVirtualDeviceLogic>`) rather than `VirtualDevice<this>`, as the polymorphic this-type breaks TypeScript's conditional type inference for ExtractAttributes when calling device.setAttribute().
📚 Learning: 2026-01-11T11:29:34.943Z
Learnt from: heavyrubberslave
Repo: SlvCtrlPlus/slvctrlplus-server PR: 59
File: src/device/protocol/virtual/audio/ttsVirtualDeviceLogic.ts:37-37
Timestamp: 2026-01-11T11:29:34.943Z
Learning: In VirtualDeviceLogic implementations, use the concrete class type (e.g., VirtualDevice<TtsVirtualDeviceLogic>) instead of VirtualDevice<this> to preserve TypeScript's conditional type inference for ExtractAttributes when calling device.setAttribute(). Apply this pattern to all VirtualDeviceLogic implementations to maintain correct typing; target files under the virtual audio/video/device logic directories.
Applied to files:
src/device/protocol/virtual/audio/piperVirtualDeviceLogic.ts
📚 Learning: 2025-12-30T08:02:18.388Z
Learnt from: heavyrubberslave
Repo: SlvCtrlPlus/slvctrlplus-server PR: 57
File: src/index.ts:89-108
Timestamp: 2025-12-30T08:02:18.388Z
Learning: Express 5 will automatically forward rejected promises and thrown errors from route handlers and middleware to your error-handling middleware, so you generally don’t need manual try/catch blocks or .catch(next). This applies to any route handler or middleware that returns a promise. Ensure you still have a proper error-handling middleware (err, req, res, next) in place and avoid relying on silent rejections. This guidance is applicable across TypeScript files in the project (src and beyond) and should be especially considered for routes and middleware that return promises.
Applied to files:
src/device/protocol/virtual/audio/piperVirtualDeviceLogic.ts
🧬 Code graph analysis (1)
src/device/protocol/virtual/audio/piperVirtualDeviceLogic.ts (6)
src/device/attribute/strDeviceAttribute.ts (1)
StrDeviceAttribute(6-36)src/device/attribute/boolDeviceAttribute.ts (1)
BoolDeviceAttribute(6-36)src/device/protocol/virtual/audio/piperVirtualDeviceConfig.ts (1)
PiperVirtualDeviceConfig(9-9)src/logging/Logger.ts (1)
Logger(9-19)src/util/process.ts (1)
spawnProcess(27-57)src/util/devNullStream.ts (1)
DevNullStream(4-29)
🔇 Additional comments (7)
src/device/protocol/virtual/audio/piperVirtualDeviceLogic.ts (7)
1-29: LGTM!Imports and type definitions are appropriate. The eslint-disable for naming convention is correctly scoped to match Piper's JSON metadata format.
31-48: LGTM!Class structure follows the established
VirtualDeviceLogicpattern. Constructor properly initializes logger with class name binding.
124-138: LGTM!Good defensive handling of metadata file errors with fallback to undefined, allowing the caller to use default values.
140-150: LGTM!Guard clause correctly prevents creating multiple speaker instances.
152-172: LGTM!Good implementation using
DevNullStreamto drain buffered audio and the cooldown mechanism to prevent race conditions during playback transitions.
217-232: LGTM!Attributes are correctly configured:
textas write-only for input andqueuingas read-write with a sensible default.
234-236: LGTM!50ms refresh interval provides responsive text input handling.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Fixes#52
Summary by CodeRabbit
New Features
Improvements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.