Add integration tests - #88
Merged
heavyrubberslave merged 40 commits intoJun 27, 2026
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR refactors the server entrypoint into ChangesApplication Bootstrap, Provider Lifecycle, and Test Harness Expansion
Sequence Diagram(s)sequenceDiagram
participant TestSuite
participant appHelper
participant createApp
participant DeviceProviderManager
participant VirtualDeviceProvider
participant DeviceManager
TestSuite->>appHelper: createTestApp(devices)
appHelper->>createApp: createApp(container, options)
createApp-->>appHelper: AppInstance
appHelper->>AppInstance: serve(port)
AppInstance->>DeviceProviderManager: loadFromSettings(settings)
DeviceProviderManager->>VirtualDeviceProvider: init()
VirtualDeviceProvider->>DeviceManager: emit deviceConnected
DeviceManager-->>TestSuite: deviceConnected event
TestSuite->>appHelper: resetTestApp(...)
appHelper->>DeviceProviderManager: stopProviders()
DeviceProviderManager->>VirtualDeviceProvider: stop()
appHelper->>DeviceManager: reset()
DeviceManager->>DeviceManager: close all devices + clear waiters
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Nitpick comments (3)
src/automation/scriptRuntime.ts (1)
363-386: 💤 Low valueRemove redundant non-null assertion.
Line 376 uses
this.dispatchRef!but there's already a null guard at line 364 that returns early. However, sincedispatchRefcould theoretically become null between the check at line 364 and usage at line 376 (ifstop()is called concurrently), the assertion masks this. Given the current code structure wherestop()is awaited and events are pushed synchronously, this is safe in practice, but the assertion is still redundant given the early return.♻️ Suggested simplification
this.eventQueue.push(() => new Promise<void>((resolve, reject) => { - if (this.dispatchRef === null) { + const ref = this.dispatchRef; + if (ref === null) { resolve(); return; } this.pendingEventDone = (errMsg: string | null): void => { if (errMsg !== null) { reject(new Error(errMsg)); } else { resolve(); } }; - void this.dispatchRef!.apply( + void ref.apply( undefined, [eventType, device.getDeviceId, device.getDeviceName], { arguments: { copy: true } } ); }));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/automation/scriptRuntime.ts` around lines 363 - 386, The non-null assertion operator (!) on this.dispatchRef used in the apply call is redundant because there is already an early return guard at the start of the block that checks if this.dispatchRef is null and returns immediately. Since execution only reaches the apply call if this.dispatchRef is not null, remove the non-null assertion operator from this.dispatchRef to simplify the code while maintaining the same safety guarantees.src/env.ts (1)
14-14: 💤 Low valueConsider exporting the
Envtype for downstream consumers.The
Envtype is currently private but returned by the exportedparseEnvfunction. Exporting it would help consumers that need to type variables holding the parsed environment.-type Env = Static<typeof EnvSchema>; +export type Env = Static<typeof EnvSchema>;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/env.ts` at line 14, The Env type created from Static<typeof EnvSchema> is currently not exported, which prevents downstream consumers from using it for type annotations even though the parseEnv function that returns this type is exported. Add the Env type to your module's exports so that consumers can properly type variables that hold the parsed environment configuration.src/serviceProvider/repositoryServiceProvider.ts (1)
9-13: ⚡ Quick winTighten
dataPathconstructor contract to non-optional.
dataPathis optional in the constructor, but Line 23 assumes it exists. If this provider is instantiated without args, it writes intoundefined/automation-scripts. Make the constructor requiredataPath: string(or add an explicit fallback).Suggested diff
-export default class RepositoryServiceProvider implements ServiceProvider<ServiceMap> -{ - private readonly dataPath: string | undefined; +export default class RepositoryServiceProvider implements ServiceProvider<ServiceMap> +{ + private readonly dataPath: string; - public constructor(dataPath?: string) { + public constructor(dataPath: string) { this.dataPath = dataPath; }Also applies to: 23-24
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/serviceProvider/repositoryServiceProvider.ts` around lines 9 - 13, The constructor parameter dataPath is optional (dataPath?: string) but the code at line 23-24 assumes it exists, which could result in writing to undefined paths. Make the dataPath parameter required by removing the optional marker (?) from the constructor parameter in RepositoryServiceProvider, and update the private field declaration from string | undefined to just string. This tightens the contract to ensure dataPath is always provided when instantiating the provider.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CLAUDE.md`:
- Line 21: Update the stale npm commands and Node version references in the
CLAUDE.md contributor documentation to match the current workflow. Replace the
outdated `npm run coverage` command with the correct current script name
`test:coverage`, and update any Node-version statements on line 86 to reflect
the current version matrix used in the CI workflow. Review both occurrences
mentioned in the comment to ensure the documentation accurately reflects the
actual scripts and versions currently in use.
In `@CONTEXT.md`:
- Around line 80-84: Remove the blank line between the two blockquote sections
in CONTEXT.md that are introduced with **Dev:** prompts. The first blockquote
section ending with the **Domain expert:** response about Device Manager and
Device Provider should be immediately followed by the second blockquote section
starting with **Dev:** about Automation Script, with no blank line separating
them, to comply with markdown lint rule MD028 which prohibits blank lines inside
blockquote blocks.
In `@src/app.ts`:
- Line 104: Remove the type assertion `as DeviceUpdateData` from the socket.on
call for WebSocketEvent.deviceUpdateReceived. Instead, either implement runtime
validation of the incoming data before passing it to deviceUpdateHandler.handle
by updating the handler to accept unknown type and validate internally, or use
Socket.IO's typed events interface to properly type the handler. This eliminates
the unsafe type cast and ensures untrusted websocket data is properly validated
before use.
In `@src/device/provider/deviceProviderManager.ts`:
- Around line 54-57: The stopProviders method currently uses fail-fast error
handling where a rejection from any provider.stop() call will break the loop and
prevent remaining providers from being stopped, potentially leaking resources.
Refactor this method to be best-effort by ensuring all providers attempt to stop
regardless of individual failures. Either wrap each await provider.stop() call
in a try-catch block that logs errors but continues to the next provider, or use
Promise.allSettled to execute all stop calls in parallel and handle any
rejections gracefully after all providers have been attempted. This ensures all
providers get a chance to clean up their resources even if some fail.
In `@src/device/provider/serialDeviceProvider.ts`:
- Around line 123-127: The SerialDeviceProvider class registers a long-lived
listener for DeviceManagerEvent.deviceDetected but inherits a no-op stop()
method, causing detection callbacks to fire even after stopProviders() is
called. Implement the stop() method in SerialDeviceProvider to properly detach
this listener by storing a reference to the deviceDetected listener callback
during initialization and then calling deviceManager.off() or the appropriate
unsubscribe method with that stored reference when stop() is invoked. This
ensures all active listeners are cleaned up when the provider is stopped.
In `@src/health/healthMetricsCollector.ts`:
- Around line 60-76: The healthMetricsCollector.start() method creates a
recurring interval with setIntervalAsync that is never cleaned up, causing
resource leaks and flaky tests. Ensure the stop() method is called to properly
clear the intervalHandle during application shutdown in src/app.ts and during
test teardown in tests/integration/helpers/appHelper.ts. This will guarantee
that the interval is properly cleared when the application shuts down or tests
complete, preventing active timer leaks.
In `@src/repository/automationScriptRepository.ts`:
- Line 30: The readFileSync call on line 30 and similar file operations on lines
42 and 47 directly concatenate user-controlled file names into filesystem paths
without sanitization, allowing path traversal attacks using sequences like ../.
Sanitize the name parameter by using path.basename() before constructing the
filesystem path to ensure only the filename is used and any directory traversal
sequences are stripped out. Apply this sanitization to all three affected
locations in the file to block path traversal for script file operations.
- Around line 29-33: The catch block in the try-catch statement around
fs.readFileSync uses a type assertion with the `as` keyword to cast the error to
NodeJS.ErrnoException. Replace this type assertion with proper type narrowing by
checking if the error object has a `code` property before accessing it, either
through an instanceof check or by verifying the property exists on the caught
error object.
In `@tests/integration/buttplugIoDevice.spec.ts`:
- Line 103: Remove the `as IntRangeDeviceAttribute` type assertion from the
variable declaration on line 103 where `device.getAttribute('Vibrate-0')` is
called, and do the same for the similar assertion on line 113. Keep the existing
`expect(...).toBeInstanceOf(IntRangeDeviceAttribute)` assertions which will
naturally narrow the type for TypeScript. When accessing type-specific
properties on the narrowed variable after the instanceof check, use optional
chaining or add explicit type guards if TypeScript cannot infer the narrowed
type.
In `@tests/integration/deviceEvents.spec.ts`:
- Around line 57-76: The event listener attached via
deviceManager.on(DeviceManagerEvent.deviceRefreshed, listener) is not removed
when the timeout rejects the promise, causing the listener to leak and
potentially interfere with subsequent tests. In the timeout callback (inside the
setTimeout function), before calling reject, ensure you remove the listener by
calling deviceManager.off(DeviceManagerEvent.deviceRefreshed, listener) to
guarantee cleanup occurs regardless of whether the promise resolves successfully
or times out.
- Line 52: The type assertion `as
VirtualDevice<RandomGeneratorVirtualDeviceLogic>` on the result of
`deviceManager.getConnectedDevices()[0]` violates TypeScript guidelines. Replace
this assertion with a proper type guard using an `instanceof` check to verify
the device is a VirtualDevice before accessing it, or create a dedicated helper
method that returns a properly typed VirtualDevice. This ensures type safety is
enforced at runtime rather than bypassed with an assertion.
In `@tests/integration/helpers/appHelper.ts`:
- Around line 130-150: The waitForNDevicesConnected function only listens for
future deviceConnected events registered after the listener is attached, so if
devices are already connected when this function is called, it will never reach
the target count and will timeout. Retrieve the current list of already
connected devices from the deviceManager before registering the event listener,
seed the connected array with these existing devices, and check if the
deviceCount requirement is already satisfied before starting the timeout timer.
Only set up the timeout and event listener if additional devices are still
needed.
- Around line 118-128: Remove type assertions from the test helpers to comply
with the project's TypeScript rules. In the getConnectedDevice function, replace
the type assertion `device as T` by adding a generic constraint `<T extends
Device>` to the function signature, establishing the relationship between T and
Device at the type level. Additionally, in the waitForNextWsEvent function
(around lines 158 and 162), type the wsEmitSpy parameter more specifically (such
as SpyInstance<[string, unknown]>) instead of casting mock.calls to the expected
tuple array format.
In `@tests/integration/helpers/buttplugIoServerSimulator.ts`:
- Line 135: Replace all `as` type assertions in the WebSocket message parsing
logic with proper type guard functions. At line 135 where JSON.parse() is cast
to ButtplugMessage[], create a type guard function to validate the parsed result
before type narrowing. For the payload field assertions in the range of lines
184-207, implement discriminated union validation or individual field type
guards to properly narrow types, ensuring validation happens before property
access rather than after (reverse the order at line 190 where runtime validation
currently follows the assertion). Apply this pattern especially to lines 205-207
where unvalidated assertions are directly used in string interpolation—validate
the message shape and required fields using type guards before referencing them
in any operations.
In `@tests/integration/helpers/mockSerialPortFactory.ts`:
- Around line 22-23: The setup function in mockSerialPortFactory.ts mutates the
global SerialPort.list property but the reset() method never restores the
original value, causing state leakage between tests. Store a reference to the
original SerialPort.list before overwriting it in the setup function, then
restore that original reference in the reset() method to ensure proper cleanup
and prevent order-dependent test failures.
In `@tests/integration/slvCtrlSerialDevice.spec.ts`:
- Line 63: Remove all type assertions using the `as` keyword and `any` types
throughout the slvCtrlSerialDevice.spec.ts integration spec file. For the filter
predicate on line 63 that casts to GenericSlvCtrlPlusDevice, replace the `as
GenericSlvCtrlPlusDevice` assertion with a proper typed predicate function or
ensure the type is correctly inferred through the callback parameter typing.
Apply the same approach to all other occurrences mentioned in the comment (lines
93-94, 98-99, 118-119, 123-124, and 419) by using typed payload interfaces or
type guard functions instead of assertions.
- Around line 212-219: The test is using a fixed 100ms sleep after calling
app.serve(testPort) to wait for server readiness, which is race-prone and causes
intermittent CI failures. Instead of relying on this arbitrary timeout,
implement a proper readiness check that waits for the server to actually be
listening on the port or uses a server readiness event. Consider refactoring the
server startup logic into a test setup fixture or using app.serve() with a
callback/promise that resolves when the server is ready to accept connections,
then await that instead of the fixed setTimeout delay. Also apply the same fix
to the other occurrence mentioned at lines 332-339.
---
Nitpick comments:
In `@src/automation/scriptRuntime.ts`:
- Around line 363-386: The non-null assertion operator (!) on this.dispatchRef
used in the apply call is redundant because there is already an early return
guard at the start of the block that checks if this.dispatchRef is null and
returns immediately. Since execution only reaches the apply call if
this.dispatchRef is not null, remove the non-null assertion operator from
this.dispatchRef to simplify the code while maintaining the same safety
guarantees.
In `@src/env.ts`:
- Line 14: The Env type created from Static<typeof EnvSchema> is currently not
exported, which prevents downstream consumers from using it for type annotations
even though the parseEnv function that returns this type is exported. Add the
Env type to your module's exports so that consumers can properly type variables
that hold the parsed environment configuration.
In `@src/serviceProvider/repositoryServiceProvider.ts`:
- Around line 9-13: The constructor parameter dataPath is optional (dataPath?:
string) but the code at line 23-24 assumes it exists, which could result in
writing to undefined paths. Make the dataPath parameter required by removing the
optional marker (?) from the constructor parameter in RepositoryServiceProvider,
and update the private field declaration from string | undefined to just string.
This tightens the contract to ensure dataPath is always provided when
instantiating the provider.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b2583a3e-8989-47f9-a9ae-a4604e0f07e8
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (50)
.env.example.github/workflows/test.ymlCLAUDE.mdCONTEXT.mdeslint.config.tspackage.jsonresources/schemas/settings.schema.jsonsrc/app.tssrc/automation/scriptRuntime.tssrc/controller/automation/stopScriptController.tssrc/controller/healthController.tssrc/device/deviceManager.tssrc/device/protocol/buttplugIo/buttplugIoDevice.tssrc/device/protocol/buttplugIo/buttplugIoDeviceFactory.tssrc/device/protocol/estim2b/estim2bSerialDeviceProvider.tssrc/device/protocol/slvCtrlPlus/slvCtrlPlusDevice.tssrc/device/protocol/slvCtrlPlus/slvCtrlPlusSerialDeviceProvider.tssrc/device/protocol/virtual/randomGenerator/randomGeneratorVirtualDeviceLogic.tssrc/device/protocol/virtual/virtualDeviceProvider.tssrc/device/protocol/virtual/virtualDeviceProviderFactory.tssrc/device/protocol/zc95/zc95SerialDeviceProvider.tssrc/device/provider/deviceProvider.tssrc/device/provider/deviceProviderManager.tssrc/device/provider/serialDeviceProvider.tssrc/env.tssrc/factory/serialPortFactory.tssrc/health/healthMetricsCollector.tssrc/index.tssrc/repository/automationScriptRepository.tssrc/serviceMap.tssrc/serviceProvider/automationServiceProvider.tssrc/serviceProvider/deviceServiceProvider.tssrc/serviceProvider/healthServiceProvider.tssrc/serviceProvider/repositoryServiceProvider.tssrc/serviceProvider/serverServiceProvider.tssrc/serviceProvider/settingsServiceProvider.tssrc/util/expressUtils.tstests/integration/api.spec.tstests/integration/automationScripts.spec.tstests/integration/buttplugIoDevice.spec.tstests/integration/deviceEvents.spec.tstests/integration/helpers/appHelper.tstests/integration/helpers/buttplugIoServerSimulator.tstests/integration/helpers/mockSerialPortFactory.tstests/integration/helpers/slvCtrlPlusDeviceSimulator.tstests/integration/slvCtrlSerialDevice.spec.tstests/unit/automation/scriptRuntime.spec.tstests/unit/controller/healthController.spec.tsvitest.config.integration.tsvitest.config.ts
💤 Files with no reviewable changes (2)
- src/serviceProvider/serverServiceProvider.ts
- resources/schemas/settings.schema.json
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/device/deviceManager.ts (1)
160-168:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle close failures in reset so cleanup always completes.
At Line 163, a single
device.close()rejection abortsreset(), so later devices are not closed and queue waiters are not rejected. That breaks reset/teardown reliability.Suggested fix
public async reset(): Promise<void> { - for (const [, device] of this.connectedDevices) { - await device.close(); - } + const devices = Array.from(this.connectedDevices.values()); + const results = await Promise.allSettled(devices.map((device) => device.close())); + results.forEach((result, index) => { + if (result.status === 'rejected') { + logError( + this.logger, + `Failed to close device '${devices[index]?.getDeviceId}' during reset`, + result.reason + ); + } + }); for (const [deviceId] of this.detectedDeviceAcquireQueue) { this.clearDetectedDeviceAcquireQueue(deviceId, 'Device manager reset'); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/device/deviceManager.ts` around lines 160 - 168, The reset() method will fail completely if any device.close() call throws an error, preventing subsequent devices from being closed and queue cleanup from running. Wrap the await device.close() call in the first for loop inside a try-catch block to catch and handle any errors, then continue iterating through remaining devices. This ensures all devices are attempted to be closed and the second for loop with clearDetectedDeviceAcquireQueue continues to execute regardless of individual close failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/app.ts`:
- Around line 235-238: The httpsServer assignment to serveResult.httpsServer is
happening inside the listen() callback, which executes asynchronously after the
serve() function has already returned. Move the assignment of
serveResult.httpsServer = httpsServer before the listen() callback starts,
similar to how httpServer is assigned to serveResult at line 218, so that
callers receive the httpsServer object immediately instead of undefined.
In `@tests/integration/buttplugIoDevice.spec.ts`:
- Around line 62-67: The server.close() call in the afterAll hook is not being
awaited, which allows teardownTestApp() and simulator.stop() to execute while
the HTTP server is still shutting down, causing intermittent test flakiness.
Wrap server.close() in a promise that resolves when the server has fully closed
(typically by passing a callback to close() or using a utility that promisifies
this operation), then await this promise before proceeding to teardownTestApp()
and simulator.stop().
- Around line 168-190: In the `device refreshes` test, the sensor is initialized
with `reading: 42` in the `simulator.addDevice` call and then
`simulator.setSensorReading` is called with the same value 42, which does not
actually validate that updates propagate. Change the initial `reading` value in
the sensor configuration to a different number (such as 0, 10, or any value
different from 42) so that the subsequent `simulator.setSensorReading` call with
42 represents an actual change that properly tests end-to-end refresh
propagation.
---
Outside diff comments:
In `@src/device/deviceManager.ts`:
- Around line 160-168: The reset() method will fail completely if any
device.close() call throws an error, preventing subsequent devices from being
closed and queue cleanup from running. Wrap the await device.close() call in the
first for loop inside a try-catch block to catch and handle any errors, then
continue iterating through remaining devices. This ensures all devices are
attempted to be closed and the second for loop with
clearDetectedDeviceAcquireQueue continues to execute regardless of individual
close failures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6c4e6f27-8a6d-422a-9ecd-179eb5537cd3
📒 Files selected for processing (7)
src/app.tssrc/device/deviceManager.tstests/integration/buttplugIoDevice.spec.tstests/integration/helpers/appHelper.tstests/integration/helpers/buttplugIoServerSimulator.tstests/integration/slvCtrlSerialDevice.spec.tstests/unit/device/testDeviceProvider.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/integration/slvCtrlSerialDevice.spec.ts
- tests/integration/helpers/appHelper.ts
- tests/integration/helpers/buttplugIoServerSimulator.ts
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/serial/synchronousSerialPort.ts (1)
64-70:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
close()should be awaitable to preserve shutdown ordering.Line 64 returns
void, but upstream callers alreadyawaitclose. That means teardown sequencing can continue beforewriter.end/destroy has completed.Suggested fix
- public close(): void { + public close(): Promise<void> { this.closed = true; this.queue.cancel(); - this.writer.end(() => { - this.writer.destroy(); - this.reader.destroy(); - }); + return new Promise((resolve) => { + this.writer.end(() => { + this.writer.destroy(); + this.reader.destroy(); + resolve(); + }); + }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/serial/synchronousSerialPort.ts` around lines 64 - 70, The close() method in synchronousSerialPort.ts is currently synchronous and returns void, but callers are awaiting it, which breaks teardown sequencing since the await completes before writer.end() and the destroy operations finish. Change the close() method to return a Promise<void> by making it async, and wrap the writer.end() callback and destroy operations in a way that the Promise doesn't resolve until all cleanup is actually complete, ensuring proper shutdown ordering for upstream callers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/device/transport/serialPortObserver.ts`:
- Around line 29-34: The onUsbEvent handler schedules a new scan on every USB
event, which can cause multiple concurrent discoverSerialDevices() calls during
rapid connect/disconnect sequences. Implement debouncing by tracking whether a
scan is already pending and resetting the timeout on each new event. Store a
reference to the setTimeout ID and clear it with clearTimeout before scheduling
a new one, ensuring only the last USB event in a burst triggers a single scan
after the 1-second delay has elapsed.
- Around line 36-37: The USB event listeners registered on the usb object using
addEventListener for 'connect' and 'disconnect' events are never unregistered,
causing memory leaks when the class is re-initialized. Add a cleanup method
(such as a destructor or lifecycle cleanup method) that removes these listeners
using removeEventListener with the same onUsbEvent handler and usb object to
ensure old instances can be garbage collected and prevent listener stacking on
re-initialization.
In `@tests/integration/devices/slvCtrlSerialDevice.spec.ts`:
- Around line 59-62: The afterAll teardown block in the
slvCtrlSerialDevice.spec.ts test file is not properly closing all resources
created in beforeAll, which can cause handle leaks across test runs. After the
teardownTestApp call, add explicit cleanup code to close the wsClient and await
the closing of the server object that were created in the beforeAll setup.
Ensure these resources are closed before mockSerialPortFactory.reset() is called
to guarantee proper resource cleanup.
---
Outside diff comments:
In `@src/serial/synchronousSerialPort.ts`:
- Around line 64-70: The close() method in synchronousSerialPort.ts is currently
synchronous and returns void, but callers are awaiting it, which breaks teardown
sequencing since the await completes before writer.end() and the destroy
operations finish. Change the close() method to return a Promise<void> by making
it async, and wrap the writer.end() callback and destroy operations in a way
that the Promise doesn't resolve until all cleanup is actually complete,
ensuring proper shutdown ordering for upstream callers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 46e4b451-f68b-444a-b00e-1274b7049c4c
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
package.jsonsrc/device/attribute/intRangeDeviceAttribute.tssrc/device/transport/serialPortObserver.tssrc/serial/synchronousSerialPort.tstests/integration/devices/buttplugIoDevice.spec.tstests/integration/devices/slvCtrlSerialDevice.spec.tstests/integration/helpers/mockSerialPortFactory.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- package.json
- tests/integration/helpers/mockSerialPortFactory.ts
✅ Action performedReview finished.
|
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 (3)
src/app.ts (1)
136-140: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winTrack provider startup promises so shutdown cannot race them.
startProviders()andserialPortObserver.start()are fire-and-forget, soshutdown()can resolve while startup work is still pending and may reopen resources after stop.🛠️ Proposed lifecycle fix
-const loadDeviceProviders = (container: Container<ServiceMap>): void => { +const loadDeviceProviders = (container: Container<ServiceMap>): Promise<void>[] => { const serialPortObserver = container.get('device.observer.serial'); const logger = container.get('logger.default'); const settings = container.get('settings'); const deviceProviderManager = container.get('device.provider.loader'); deviceProviderManager.loadFromSettings(settings); - deviceProviderManager + const providerStartup = deviceProviderManager .startProviders() .catch(e => logError(logger, `Loading device providers failed`, e)); - serialPortObserver.start().catch(e => logError(logger, `Initializing serial port observer failed`, e)); + const observerStartup = serialPortObserver + .start() + .catch(e => logError(logger, `Initializing serial port observer failed`, e)); + + return [providerStartup, observerStartup]; };- loadDeviceProviders(container); + const startupTasks = loadDeviceProviders(container);logger.info('Shutting down...'); + await Promise.allSettled(startupTasks); await container.get('automation.scriptRuntime').stop();Also applies to: 199-199, 245-248
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app.ts` around lines 136 - 140, The startup calls in app.ts are fire-and-forget, so shutdown can complete while deviceProviderManager.startProviders() and serialPortObserver.start() are still pending. Track these startup promises on the app lifecycle object, make shutdown() await or settle them before returning, and ensure the stop path in the startup/shutdown flow cannot reopen resources after close. Update the relevant lifecycle methods around startProviders, serialPortObserver.start, and shutdown to coordinate startup completion before teardown.src/health/healthMetricsCollector.ts (1)
48-53: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDo not remove unrelated listeners from the shared emitter.
EventEmitteris injected, soremoveAllListeners()can detach listeners owned by other components. Limit cleanup to the collector event.Proposed change
public stop(): void { this.intervalHandle?.clear(); this.intervalHandle = null; - this.eventEmitter.removeAllListeners(); + this.eventEmitter.removeAllListeners(HealthMetricsCollectorEvent.collected); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/health/healthMetricsCollector.ts` around lines 48 - 53, The stop() cleanup in HealthMetricsCollector is too broad because eventEmitter.removeAllListeners() removes listeners owned by other consumers of the shared EventEmitter. Update HealthMetricsCollector.stop() to remove only the listener(s) registered by this collector for its own event, while keeping the interval cleanup intact and leaving unrelated emitter listeners untouched.tests/integration/helpers/buttplugIoServerSimulator.ts (1)
61-63: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAvoid missing an already-completed client handshake.
If
RequestDeviceListarrives beforewaitForClientReady()is called, the resolver list has already been cleared and the wait times out.Proposed change
private clientConnectedResolvers: Array<() => void> = []; private clientReadyResolvers: Array<() => void> = []; + private clientReady = false; @@ public waitForClientReady(timeoutMs = 5000): Promise<void> { + if (this.clientReady) { + return Promise.resolve(); + } + return new Promise((resolve, reject) => { const timer = setTimeout(() => reject(new Error(`Timed out waiting for buttplug client to be ready (>${timeoutMs}ms)`)), timeoutMs); this.clientReadyResolvers.push(() => { clearTimeout(timer); resolve(); }); }); @@ - for (const resolve of this.clientReadyResolvers) resolve(); + this.clientReady = true; + for (const resolve of this.clientReadyResolvers) resolve(); this.clientReadyResolvers = [];Also applies to: 109-113, 178-179
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/helpers/buttplugIoServerSimulator.ts` around lines 61 - 63, The client handshake can be missed if RequestDeviceList arrives before waitForClientReady() starts waiting, because clientReadyResolvers is cleared after the event and the later wait times out. Update ButtplugIoServerSimulator so the ready state is tracked independently of the resolver queue, and have waitForClientReady() resolve immediately when the client is already ready instead of only relying on clientReadyResolvers. Make the same adjustment wherever the ready/connected resolver lists are managed, including the clientConnectedResolvers flow, so completed handshakes are not lost.
🧹 Nitpick comments (2)
tests/integration/helpers/appHelper.ts (1)
82-85: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant undefined check.
TestApp.httpServeris typed as a non-optionalhttp.Server, so the!== undefinedguard is dead. Harmless, but you can drop it for clarity.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/helpers/appHelper.ts` around lines 82 - 85, The `teardownTestApp` helper has a redundant `app.httpServer !== undefined` guard because `TestApp.httpServer` is already non-optional; remove the dead check and call `closeAllConnections` directly on `app.httpServer` to keep the cleanup logic in `teardownTestApp` clear.src/device/protocol/buttplugIo/buttplugIoWebsocketDeviceProvider.ts (1)
11-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPass the original error to
logError.
logErroralready normalizesunknown; passing onlye.messageloses the originalErrorstack/context and makes the new helper import unnecessary.Proposed change
-import { hasProperty } from '../../../util/objects.js'; @@ - logError(this.logger, `Could not connect to buttplug.io server (${url})`, hasProperty(e, 'message') ? e.message : 'unknown'); + logError(this.logger, `Could not connect to buttplug.io server (${url})`, e);Also applies to: 82-82
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/device/protocol/buttplugIo/buttplugIoWebsocketDeviceProvider.ts` at line 11, The error handling in buttplugIoWebsocketDeviceProvider should pass the original caught error object into logError instead of only e.message, so the helper can normalize unknown and preserve stack/context. Update the relevant catch blocks in the provider methods (and remove the now-unneeded hasProperty import if it is only used for this message extraction) so logError is called directly with the original error plus the existing context string.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/test.yml:
- Line 38: The workflow currently references the mutable actions/setup-node@v5
tag; update the setup-node step in the test workflow to use a full commit SHA
instead. Keep the same action usage and version behavior, but replace the tag
with the pinned SHA so the job remains locked to an immutable revision.
In `@src/app.ts`:
- Around line 249-251: The shutdown path in app.ts does not wait for the
HTTP/HTTPS server close operations to finish, so handles may still be alive when
shutdown() returns. Update the shutdown logic around websocketServer.close(),
serveResult.httpServer.close(), and serveResult.httpsServer?.close() to await
their completion using a promise-based wrapper or equivalent callback-to-promise
conversion, ensuring all servers are fully closed before exiting the function.
In `@src/index.ts`:
- Around line 31-38: Make the shutdown path idempotent in the shutdown handler
by guarding the app.shutdown() flow so it only runs once even if SIGTERM and
SIGINT arrive multiple times. Update the shutdown function in src/index.ts to
use a module-level “already shutting down” flag or equivalent, keep the existing
logError(logger, 'Error during shutdown', err) handling, and ensure subsequent
signal invocations return immediately without triggering duplicate cleanup or
multiple process.exit calls.
In `@tests/integration/helpers/appHelper.ts`:
- Around line 70-80: The WebSocket client setup in createWsClient leaves the 2s
timeout and failed wsClient alive on connect_error, so add a shared cleanup path
that clears the timer and disconnects the client before rejecting. Update the
promise handlers inside createWsClient to reuse one cleanup function for both
connect and connect_error, ensuring the timeout is always cleared and the socket
is torn down on the error path.
In `@tests/integration/helpers/buttplugIoServerSimulator.ts`:
- Around line 122-127: The stop() teardown in ButtplugIoServerSimulator can hang
when this.wss or this.server is null because the optional-chained close calls
never resolve. Update stop() to explicitly handle the null/not-started case for
both the wss and server shutdown paths, resolving immediately when either server
was never opened and only awaiting close() when the instance exists.
---
Outside diff comments:
In `@src/app.ts`:
- Around line 136-140: The startup calls in app.ts are fire-and-forget, so
shutdown can complete while deviceProviderManager.startProviders() and
serialPortObserver.start() are still pending. Track these startup promises on
the app lifecycle object, make shutdown() await or settle them before returning,
and ensure the stop path in the startup/shutdown flow cannot reopen resources
after close. Update the relevant lifecycle methods around startProviders,
serialPortObserver.start, and shutdown to coordinate startup completion before
teardown.
In `@src/health/healthMetricsCollector.ts`:
- Around line 48-53: The stop() cleanup in HealthMetricsCollector is too broad
because eventEmitter.removeAllListeners() removes listeners owned by other
consumers of the shared EventEmitter. Update HealthMetricsCollector.stop() to
remove only the listener(s) registered by this collector for its own event,
while keeping the interval cleanup intact and leaving unrelated emitter
listeners untouched.
In `@tests/integration/helpers/buttplugIoServerSimulator.ts`:
- Around line 61-63: The client handshake can be missed if RequestDeviceList
arrives before waitForClientReady() starts waiting, because clientReadyResolvers
is cleared after the event and the later wait times out. Update
ButtplugIoServerSimulator so the ready state is tracked independently of the
resolver queue, and have waitForClientReady() resolve immediately when the
client is already ready instead of only relying on clientReadyResolvers. Make
the same adjustment wherever the ready/connected resolver lists are managed,
including the clientConnectedResolvers flow, so completed handshakes are not
lost.
---
Nitpick comments:
In `@src/device/protocol/buttplugIo/buttplugIoWebsocketDeviceProvider.ts`:
- Line 11: The error handling in buttplugIoWebsocketDeviceProvider should pass
the original caught error object into logError instead of only e.message, so the
helper can normalize unknown and preserve stack/context. Update the relevant
catch blocks in the provider methods (and remove the now-unneeded hasProperty
import if it is only used for this message extraction) so logError is called
directly with the original error plus the existing context string.
In `@tests/integration/helpers/appHelper.ts`:
- Around line 82-85: The `teardownTestApp` helper has a redundant
`app.httpServer !== undefined` guard because `TestApp.httpServer` is already
non-optional; remove the dead check and call `closeAllConnections` directly on
`app.httpServer` to keep the cleanup logic in `teardownTestApp` clear.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8f61a1df-09d4-40a4-96a2-4c30bf4bfa76
📒 Files selected for processing (30)
.github/workflows/test.ymlsrc/app.tssrc/automation/scriptRuntime.tssrc/device/protocol/buttplugIo/buttplugIoWebsocketDeviceProvider.tssrc/device/provider/deviceProviderManager.tssrc/device/provider/serialDeviceProvider.tssrc/device/serializedTypes.tssrc/device/transport/serialPortObserver.tssrc/health/healthMetricsCollector.tssrc/health/serializedTypes.tssrc/index.tssrc/repository/automationScriptRepository.tssrc/serviceProvider/healthServiceProvider.tssrc/settings/serializedTypes.tssrc/socket/types.tssrc/util/objects.tstests/integration/api.spec.tstests/integration/automationScripts.spec.tstests/integration/deviceEvents.spec.tstests/integration/devices/buttplugIoDevice.spec.tstests/integration/devices/estim2bDevice.spec.tstests/integration/devices/slvCtrlSerialDevice.spec.tstests/integration/devices/zc95Device.spec.tstests/integration/helpers/appHelper.tstests/integration/helpers/buttplugIoServerSimulator.tstests/integration/helpers/estim2bDeviceSimulator.tstests/integration/helpers/mockSerialPortFactory.tstests/integration/helpers/zc95DeviceSimulator.tstests/unit/controller/healthController.spec.tsvitest.config.integration.ts
✅ Files skipped from review due to trivial changes (1)
- src/health/serializedTypes.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- vitest.config.integration.ts
- tests/unit/controller/healthController.spec.ts
- src/device/transport/serialPortObserver.ts
- src/device/provider/deviceProviderManager.ts
- src/serviceProvider/healthServiceProvider.ts
- src/device/provider/serialDeviceProvider.ts
- tests/integration/devices/buttplugIoDevice.spec.ts
- tests/integration/api.spec.ts
- src/automation/scriptRuntime.ts
- tests/integration/devices/slvCtrlSerialDevice.spec.ts
- tests/integration/deviceEvents.spec.ts
- tests/integration/automationScripts.spec.ts
heavyrubberslave
deleted the
feat/intergation-tests-serial-and-buttplugio
branch
June 27, 2026 10:54
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
SSL_KEY_FILE/SSL_CERT_FILE(optional, both required to enable).Improvements
/healthnow returns 204 when no metrics are available and serves periodically refreshed health data.Tests