Add integration tests - #85
Closed
heavyrubberslave wants to merge 19 commits into
Closed
heavyrubberslave wants to merge 19 commits into
heavyrubberslave wants to merge 19 commits into
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:
📝 WalkthroughWalkthroughCentralize app startup in createApp with validated env and dataPath; refactor health metrics caching and ScriptRuntime queue/stop; add provider stop/loader shutdown and DeviceManager reset; parameterize virtual scan interval; add integration test configs, helpers, and suites; update CI to run integration tests. ChangesApp Factory Extraction and Entrypoint Refactor
Provider, Device, Settings, and Runtime Lifecycle
Integration Testing Infrastructure
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 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. ✨ 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 |
heavyrubberslave
marked this pull request as draft
June 6, 2026 20:08
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
src/app.ts (2)
45-45: 💤 Low valueConsider converting to arrow function per project linting rules.
The static analysis tools flag a preference for arrow functions. This is a style-only change.
♻️ Proposed fix
-export function createApp(options: AppOptions = {}): AppInstance { +export const createApp = (options: AppOptions = {}): AppInstance => {🤖 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` at line 45, The function declaration "export function createApp(options: AppOptions = {}): AppInstance {" should be converted to an exported arrow function to satisfy lint rules; replace the named function declaration with a const exported arrow like "export const createApp = (options: AppOptions = {}): AppInstance => {" and keep the existing body and return behavior unchanged, ensuring any hoisting-sensitive usage is adjusted if necessary.Source: Linters/SAST tools
137-157: ⚖️ Poor tradeoffEvent listeners are not cleaned up when the app instance is stopped.
The
createAppfunction registers event listeners ondeviceManager,settingsManager, andscriptRuntime, butAppInstanceprovides no way to remove them. In test scenarios wherecreateAppis called multiple times, this could lead to duplicate event handlers and memory leaks since these managers may be shared or persist across instances.Consider adding a cleanup method to
AppInstance:export interface AppInstance { expressApp: express.Application; container: Pimple<ServiceMap>; httpServer: http.Server; httpsServer: https.Server | undefined; cleanup: () => void; }Then store the handler references and remove them in
cleanup().🤖 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 137 - 157, createApp currently registers listeners on deviceManager, settingsManager and scriptRuntime but never removes them; add a cleanup function to the AppInstance interface (e.g., cleanup: () => void) and, when you attach handlers for DeviceManagerEvent.deviceConnected/deviceDisconnected/deviceRefreshed, SettingsEventType.changed and AutomationEventType.consoleLog, assign each listener to a named function or const handler reference and store those references on the returned AppInstance; implement cleanup to call the appropriate removal API (e.g., deviceManager.off/removeListener(...), settingsManager.off/removeListener(...), scriptRuntime.off/removeListener(...)) for each stored handler so repeated createApp calls do not accumulate listeners.src/device/provider/deviceProviderLoader.ts (1)
37-56: ⚖️ Poor tradeoffConsider cleanup on partial initialization failure.
If
provider.init()throws on line 53, providers already instartedProvidersremain initialized with their intervals running, but the error propagates without cleanup. While the learning notes that provider lifetime is process-tied and a restart is required for changes, tests or graceful degradation scenarios could benefit from cleanup on partial failure.♻️ Optional refactor to add try-finally cleanup
public async loadFromSettings(): Promise<void> { const configuredDeviceSources = this.settings.getDeviceSources(); this.logger.debug(`Found ${configuredDeviceSources.size} configured device source(s)`); + try { for (const [id, deviceSource] of configuredDeviceSources) { const factory = this.factories.get(deviceSource.type) if (undefined === factory) { this.logger.info(`Device source with id ${id} and type ${deviceSource.type} is not supported`); continue; } const provider = factory.create(deviceSource.config); await provider.init(); this.startedProviders.push(provider); } + } catch (error) { + this.logger.error('Error during provider initialization, stopping already-started providers', error); + this.stop(); + throw error; + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/device/provider/deviceProviderLoader.ts` around lines 37 - 56, The loadFromSettings method can leave already-initialized providers running if provider.init() throws; wrap the per-provider init in try/catch (or the whole loop in try/finally) so that on any init failure you iterate the existing this.startedProviders and call their cleanup API (e.g. stop(), shutdown(), or dispose()—use the actual method name implemented on your provider interface) to stop intervals/cleanup resources, await those calls, then rethrow the original error; update loadFromSettings to ensure partial initialization is cleaned up before propagating the exception.Source: Learnings
tests/integration/app.spec.ts (4)
117-126: ⚡ Quick winClean up event listener on timeout.
The listener registered on Line 120 is not removed if the timeout on Line 118 fires. This can cause test flakiness or leave dangling listeners in the test environment.
🧹 Proposed fix to clean up listener on timeout
const newDeviceConnected = new Promise<void>((resolve, reject) => { - const timeout = setTimeout(() => reject(new Error('Timed out waiting for new device to connect')), 1000); + const listener = (device: any) => { + if (device.getDeviceId === NEW_DEVICE_ID) { + clearTimeout(timeout); + deviceManager.off(DeviceManagerEvent.deviceConnected, listener); + resolve(); + } + }; + const timeout = setTimeout(() => { + deviceManager.off(DeviceManagerEvent.deviceConnected, listener); + reject(new Error('Timed out waiting for new device to connect')); + }, 1000); - deviceManager.on(DeviceManagerEvent.deviceConnected, (device) => { - if (device.getDeviceId === NEW_DEVICE_ID) { - clearTimeout(timeout); - resolve(); - } - }); + deviceManager.on(DeviceManagerEvent.deviceConnected, listener); });🤖 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/app.spec.ts` around lines 117 - 126, The promise newDeviceConnected installs a listener with deviceManager.on for DeviceManagerEvent.deviceConnected but never removes it on timeout or after resolve; change it to register a named handler (e.g., const handler = (device) => { ... }) and call deviceManager.off(DeviceManagerEvent.deviceConnected, handler) (or the appropriate remove method) both when you clear the timeout/resolve and inside the timeout reject path so the listener is always cleaned up; ensure the handler still checks device.getDeviceId === NEW_DEVICE_ID before resolving.
187-194: ⚡ Quick winClean up event listener on timeout.
The listener on Line 190 is not removed if the timeout fires.
🧹 Proposed fix to clean up listener on timeout
const disconnected = new Promise<void>((resolve, reject) => { - const timeout = setTimeout(() => reject(new Error('Timed out waiting for device disconnection')), 2000); + const listener = () => { + clearTimeout(timeout); + deviceManager.off(DeviceManagerEvent.deviceDisconnected, listener); + resolve(); + }; + const timeout = setTimeout(() => { + deviceManager.off(DeviceManagerEvent.deviceDisconnected, listener); + reject(new Error('Timed out waiting for device disconnection')); + }, 2000); - deviceManager.on(DeviceManagerEvent.deviceDisconnected, () => { - clearTimeout(timeout); - resolve(); - }); + deviceManager.on(DeviceManagerEvent.deviceDisconnected, listener); });🤖 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/app.spec.ts` around lines 187 - 194, The device disconnected Promise attaches an anonymous listener to deviceManager.on(DeviceManagerEvent.deviceDisconnected) but never removes it if the timeout fires; refactor the Promise in tests/integration/app.spec.ts to register a named handler (e.g., const handler = () => { clearTimeout(timeout); deviceManager.off(DeviceManagerEvent.deviceDisconnected, handler); resolve(); }) and in the timeout rejection path call deviceManager.off(DeviceManagerEvent.deviceDisconnected, handler) before rejecting (or use removeListener if that API is used) so the listener is always cleaned up whether the event fires or the timeout occurs.
148-157: ⚡ Quick winClean up event listeners on timeout.
Similar to previous tests, the listeners registered on Lines 151 and 170 are not removed if their respective timeouts fire. This pattern repeats across multiple tests and can cause flakiness.
🧹 Proposed fix to clean up listeners on timeout
Apply the same cleanup pattern as suggested in earlier comments. For Line 148-157:
const newDeviceConnected = new Promise<void>((resolve, reject) => { - const timeout = setTimeout(() => reject(new Error('Timed out waiting for new device to connect')), 1000); + const listener = (device: any) => { + if (device.getDeviceId === NEW_DEVICE_ID) { + clearTimeout(timeout); + deviceManager.off(DeviceManagerEvent.deviceConnected, listener); + resolve(); + } + }; + const timeout = setTimeout(() => { + deviceManager.off(DeviceManagerEvent.deviceConnected, listener); + reject(new Error('Timed out waiting for new device to connect')); + }, 1000); - deviceManager.on(DeviceManagerEvent.deviceConnected, (device) => { - if (device.getDeviceId === NEW_DEVICE_ID) { - clearTimeout(timeout); - resolve(); - } - }); + deviceManager.on(DeviceManagerEvent.deviceConnected, listener); });Apply similar changes to Lines 167-176.
Also applies to: 167-176
🤖 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/app.spec.ts` around lines 148 - 157, The newDeviceConnected promise registers a listener with deviceManager.on(DeviceManagerEvent.deviceConnected, ...) but does not remove that listener when the timeout fires, risking leaked listeners; refactor the promise to assign the callback to a const handler = (device) => { ... } and pass that to deviceManager.on, then in both the success path (after clearTimeout) and the timeout reject path call deviceManager.off(DeviceManagerEvent.deviceConnected, handler) (or deviceManager.removeListener if off is not available) so the listener is always cleaned up; apply the same pattern for the other promise around Lines 167-176.
91-98: ⚡ Quick winClean up event listener on timeout and filter events by device.
The listener registered on Line 94 is not removed if the timeout fires, and it doesn't validate which device was refreshed. This can cause flaky tests or memory leaks in the test environment.
🧹 Proposed fix to clean up listener and filter events
const secondValue = await new Promise<Int | undefined>((resolve, reject) => { const timeout = setTimeout(() => reject(new Error('Timed out waiting for device refresh')), 2000); - instance.container.get('device.manager').on(DeviceManagerEvent.deviceRefreshed, async () => { + const listener = async (refreshedDevice: any) => { + if (refreshedDevice.getDeviceId !== device.getDeviceId) return; clearTimeout(timeout); + deviceManager.off(DeviceManagerEvent.deviceRefreshed, listener); resolve((await device.getAttribute('value'))?.value); - }); + }; + deviceManager.on(DeviceManagerEvent.deviceRefreshed, listener); });🤖 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/app.spec.ts` around lines 91 - 98, The test registers a listener with instance.container.get('device.manager').on for DeviceManagerEvent.deviceRefreshed but never removes it on timeout and doesn't check which device fired the event; fix by creating a named handler that first checks the event's device id (or unique identifier) matches the test's device, and if so clears the timeout, removes the handler (via the manager's off/removeListener API) and resolves with (await device.getAttribute('value'))?.value; in the timeout branch call the manager's off/removeListener with the same handler before rejecting to ensure cleanup.
🤖 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/protocol/virtual/virtualDeviceProviderFactory.ts`:
- Around line 41-54: The create method in VirtualDeviceProviderFactory currently
only checks typeof config.scanIntervalMs === 'number' but allows 0, negative,
NaN, or Infinity; update VirtualDeviceProviderFactory.create to validate that
scanIntervalMs is a finite positive integer (or at least > 0) before using it,
and fall back to VirtualDeviceProviderFactory.DEFAULT_SCAN_INTERVAL_MS when the
value is missing or invalid; use Number.isFinite and a > 0 check on
config.scanIntervalMs (referencing scanIntervalMs,
VirtualDeviceProviderFactory.create and DEFAULT_SCAN_INTERVAL_MS) so the new
VirtualDeviceProvider(...) always receives a safe scanIntervalMs.
In `@tests/integration/app.spec.ts`:
- Around line 52-57: The test waits forever on the deviceConnected promise;
change the setup to race the existing deviceConnected listener
(DeviceManagerEvent.deviceConnected on deviceManager) against a timeout so the
test fails fast if the device never connects; use the same symbols
(deviceManager, DeviceManagerEvent.deviceConnected, the deviceConnected Promise
and the call to
instance.container.get('device.provider.loader').loadFromSettings()) and
implement a Promise.race with a short timeout (e.g., 3–10s) that rejects with a
clear error message so await will throw instead of hanging.
---
Nitpick comments:
In `@src/app.ts`:
- Line 45: The function declaration "export function createApp(options:
AppOptions = {}): AppInstance {" should be converted to an exported arrow
function to satisfy lint rules; replace the named function declaration with a
const exported arrow like "export const createApp = (options: AppOptions = {}):
AppInstance => {" and keep the existing body and return behavior unchanged,
ensuring any hoisting-sensitive usage is adjusted if necessary.
- Around line 137-157: createApp currently registers listeners on deviceManager,
settingsManager and scriptRuntime but never removes them; add a cleanup function
to the AppInstance interface (e.g., cleanup: () => void) and, when you attach
handlers for
DeviceManagerEvent.deviceConnected/deviceDisconnected/deviceRefreshed,
SettingsEventType.changed and AutomationEventType.consoleLog, assign each
listener to a named function or const handler reference and store those
references on the returned AppInstance; implement cleanup to call the
appropriate removal API (e.g., deviceManager.off/removeListener(...),
settingsManager.off/removeListener(...), scriptRuntime.off/removeListener(...))
for each stored handler so repeated createApp calls do not accumulate listeners.
In `@src/device/provider/deviceProviderLoader.ts`:
- Around line 37-56: The loadFromSettings method can leave already-initialized
providers running if provider.init() throws; wrap the per-provider init in
try/catch (or the whole loop in try/finally) so that on any init failure you
iterate the existing this.startedProviders and call their cleanup API (e.g.
stop(), shutdown(), or dispose()—use the actual method name implemented on your
provider interface) to stop intervals/cleanup resources, await those calls, then
rethrow the original error; update loadFromSettings to ensure partial
initialization is cleaned up before propagating the exception.
In `@tests/integration/app.spec.ts`:
- Around line 117-126: The promise newDeviceConnected installs a listener with
deviceManager.on for DeviceManagerEvent.deviceConnected but never removes it on
timeout or after resolve; change it to register a named handler (e.g., const
handler = (device) => { ... }) and call
deviceManager.off(DeviceManagerEvent.deviceConnected, handler) (or the
appropriate remove method) both when you clear the timeout/resolve and inside
the timeout reject path so the listener is always cleaned up; ensure the handler
still checks device.getDeviceId === NEW_DEVICE_ID before resolving.
- Around line 187-194: The device disconnected Promise attaches an anonymous
listener to deviceManager.on(DeviceManagerEvent.deviceDisconnected) but never
removes it if the timeout fires; refactor the Promise in
tests/integration/app.spec.ts to register a named handler (e.g., const handler =
() => { clearTimeout(timeout);
deviceManager.off(DeviceManagerEvent.deviceDisconnected, handler); resolve(); })
and in the timeout rejection path call
deviceManager.off(DeviceManagerEvent.deviceDisconnected, handler) before
rejecting (or use removeListener if that API is used) so the listener is always
cleaned up whether the event fires or the timeout occurs.
- Around line 148-157: The newDeviceConnected promise registers a listener with
deviceManager.on(DeviceManagerEvent.deviceConnected, ...) but does not remove
that listener when the timeout fires, risking leaked listeners; refactor the
promise to assign the callback to a const handler = (device) => { ... } and pass
that to deviceManager.on, then in both the success path (after clearTimeout) and
the timeout reject path call
deviceManager.off(DeviceManagerEvent.deviceConnected, handler) (or
deviceManager.removeListener if off is not available) so the listener is always
cleaned up; apply the same pattern for the other promise around Lines 167-176.
- Around line 91-98: The test registers a listener with
instance.container.get('device.manager').on for
DeviceManagerEvent.deviceRefreshed but never removes it on timeout and doesn't
check which device fired the event; fix by creating a named handler that first
checks the event's device id (or unique identifier) matches the test's device,
and if so clears the timeout, removes the handler (via the manager's
off/removeListener API) and resolves with (await
device.getAttribute('value'))?.value; in the timeout branch call the manager's
off/removeListener with the same handler before rejecting to ensure cleanup.
🪄 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: 152e47af-ae09-488d-8dd7-1957c8fb14ff
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (13)
.github/workflows/test.ymlpackage.jsonsrc/app.tssrc/device/protocol/virtual/randomGenerator/randomGeneratorVirtualDeviceLogic.tssrc/device/protocol/virtual/virtualDeviceProvider.tssrc/device/protocol/virtual/virtualDeviceProviderFactory.tssrc/device/provider/deviceProvider.tssrc/device/provider/deviceProviderLoader.tssrc/index.tssrc/serviceProvider/settingsServiceProvider.tstests/integration/app.spec.tsvitest.config.integration.tsvitest.config.ts
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/deviceManager.ts`:
- Around line 160-166: The reset() implementation currently calls device.close()
directly and then detectedDeviceAcquireQueue.clear(), which drops
acquireDetectedDevice() promises and stops cleanup if close() rejects; change
reset() to first call device.close() for all connectedDevices but use
Promise.allSettled on the close() calls so failures are non-blocking, then
iterate detectedDeviceAcquireQueue and call clearDetectedDeviceAcquireQueue(id)
for each queued id (instead of raw clear()) to resolve/reject waiting acquire
promises, and finally clear the internal map; reference the reset(),
connectedDevices, device.close(), detectedDeviceAcquireQueue.clear(), and
clearDetectedDeviceAcquireQueue(...) symbols when making these changes.
In `@src/env.ts`:
- Around line 7-8: parseEnv currently allows SSL_CERT_FILE and SSL_KEY_FILE to
be set independently, which permits a partial TLS config; update parseEnv to
validate these paired env vars (SSL_CERT_FILE and SSL_KEY_FILE) so that if one
is provided the other must be present, and throw or return a clear error if only
one is set. Locate the SSL_CERT_FILE and SSL_KEY_FILE entries in the Type
definitions and add a runtime check in parseEnv (or the function that consumes
the parsed env) to enforce the pair, producing a descriptive failure message
referencing both SSL_CERT_FILE and SSL_KEY_FILE when the check fails. Ensure the
check runs before downstream code that builds sslConfig so it never silently
falls back to HTTP.
In `@tests/integration/automationScripts.spec.ts`:
- Around line 60-66: The test-created Promises for scriptStarted and
scriptStopped attach listeners via scriptRuntime.on but never remove them,
causing listener leakage across tests; update the Promise factories (the
scriptStarted and the analogous scriptStopped block around
AutomationEventType.scriptStopped) to capture the handler callback in a const,
register it with scriptRuntime.on, and ensure you call
scriptRuntime.off(handler) (or
scriptRuntime.off(AutomationEventType.scriptStarted, handler) if that API
exists) in both the success path before resolve() and the timeout/reject path
(clearTimeout then remove the handler) so listeners are always unsubscribed when
the Promise settles.
In `@tests/integration/deviceEvents.spec.ts`:
- Around line 53-70: The promise-based wait in the test registers a
DeviceManagerEvent.deviceRefreshed handler via deviceManager.on but never
consistently removes it, causing listener buildup and flakes; fix each such
Promise (the one around deviceManager.on and the similar blocks at the other
occurrences) by capturing the handler in a variable, registering it with
deviceManager.on, and ensuring you remove it (deviceManager.off or equivalent)
and clear the timeout in both resolve and reject paths (or use a try/finally
inside an async wrapper / deviceManager.once if available) so the listener is
always unregistered when the Promise finishes or times out.
- Around line 87-90: The assertions rely on the order of the devices array
(devices[0]/devices[1]) which can be flaky; change the check to validate the set
of device IDs instead: map devices to their getDeviceId() values (using the
existing devices and getDeviceId symbol), and assert that the resulting array or
set contains both TEST_DEVICE_ID and NEW_DEVICE_ID (e.g., compare sets or use an
order-insensitive matcher like arrayContaining) rather than asserting by index.
In `@tests/integration/helpers/appHelper.ts`:
- Around line 78-92: The allGone Promise in resetTestApp registers a listener
for DeviceManagerEvent.deviceDisconnected but does not remove it if the timeout
fires; modify the timeout handler to remove the listener (call
deviceManager.off(DeviceManagerEvent.deviceDisconnected, listener)) before
rejecting so the listener is always cleaned up, keeping the existing
clearTimeout/resolve path unchanged.
🪄 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: 9b9bbfc7-59dc-4253-8aed-fcb111e4ee73
📒 Files selected for processing (15)
.env.example.github/workflows/test.ymleslint.config.tssrc/app.tssrc/automation/scriptRuntime.tssrc/device/deviceManager.tssrc/device/protocol/virtual/virtualDeviceProviderFactory.tssrc/env.tssrc/index.tstests/integration/api.spec.tstests/integration/automationScripts.spec.tstests/integration/deviceEvents.spec.tstests/integration/helpers/appHelper.tstests/unit/automation/scriptRuntime.spec.tsvitest.config.integration.ts
✅ Files skipped from review due to trivial changes (2)
- tests/unit/automation/scriptRuntime.spec.ts
- tests/integration/api.spec.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- vitest.config.integration.ts
- .github/workflows/test.yml
- src/device/protocol/virtual/virtualDeviceProviderFactory.ts
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai please review |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (5)
tests/integration/deviceEvents.spec.ts (3)
131-138:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRemove listener in both success and timeout paths.
The listener registered at line 134 is never removed. Every test run accumulates another orphaned listener on
deviceManager, causing cross-test interference and false positives.🔧 Proposed fix
const disconnected = new Promise<void>((resolve, reject) => { - const timeout = setTimeout(() => reject(new Error('Timed out waiting for device disconnection')), 2000); - - deviceManager.on(DeviceManagerEvent.deviceDisconnected, () => { + const listener = () => { clearTimeout(timeout); + deviceManager.off(DeviceManagerEvent.deviceDisconnected, listener); resolve(); - }); + }; + const timeout = setTimeout(() => { + deviceManager.off(DeviceManagerEvent.deviceDisconnected, listener); + reject(new Error('Timed out waiting for device disconnection')); + }, 2000); + deviceManager.on(DeviceManagerEvent.deviceDisconnected, listener); });🤖 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/deviceEvents.spec.ts` around lines 131 - 138, The promise that waits for disconnection registers a listener on deviceManager (DeviceManagerEvent.deviceDisconnected) but never removes it on success or timeout; update the Promise executor to store the listener function in a const, call deviceManager.removeListener(DeviceManagerEvent.deviceDisconnected, listener) (or deviceManager.off(...)) inside the success handler before resolve and inside the timeout handler before reject, and still clearTimeout(timeout) appropriately so the listener is removed in both paths.
54-73:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove listener when timeout fires.
If the device value doesn't change within 1000ms, the timeout rejects but leaves the
listenerattached todeviceManager, causing accumulation across tests and potential flaky behavior.🔧 Proposed fix
await new Promise<void>((resolve, reject) => { - const timeout = setTimeout(() => reject(new Error('Timed out waiting for device value to change')), 1000); - const listener = async () => { const value = (await device.getAttribute('value'))?.value; if (undefined === observedValue) { observedValue = value; return; } if (value !== observedValue) { changedValue = value; clearTimeout(timeout); deviceManager.off(DeviceManagerEvent.deviceRefreshed, listener); resolve(); } }; + const timeout = setTimeout(() => { + deviceManager.off(DeviceManagerEvent.deviceRefreshed, listener); + reject(new Error('Timed out waiting for device value to change')); + }, 1000); deviceManager.on(DeviceManagerEvent.deviceRefreshed, listener); });🤖 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/deviceEvents.spec.ts` around lines 54 - 73, The timeout handler currently rejects the promise but doesn't remove the registered listener, causing listeners to accumulate; update the timeout callback so it also calls deviceManager.off(DeviceManagerEvent.deviceRefreshed, listener) before rejecting, and keep the existing clearTimeout(timeout) in the success path (listener) to avoid stray timers; locate the Promise block where deviceManager.on(DeviceManagerEvent.deviceRefreshed, listener) is added and remove the listener in the timeout branch to ensure cleanup.
104-115:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove listener when timeout fires.
If
NEW_DEVICE_IDnever disconnects, the timeout rejects but leaves thelistenerattached todeviceManager, causing listener accumulation and test interference.🔧 Proposed fix
const deviceDisconnected = new Promise<void>((resolve, reject) => { - const timeout = setTimeout(() => reject(new Error('Timed out waiting for device to disconnect')), 1000); - const listener = (device: Device) => { if (device.getDeviceId === NEW_DEVICE_ID) { clearTimeout(timeout); deviceManager.off(DeviceManagerEvent.deviceDisconnected, listener); resolve(); } }; + const timeout = setTimeout(() => { + deviceManager.off(DeviceManagerEvent.deviceDisconnected, listener); + reject(new Error('Timed out waiting for device to disconnect')); + }, 1000); deviceManager.on(DeviceManagerEvent.deviceDisconnected, listener); });🤖 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/deviceEvents.spec.ts` around lines 104 - 115, The timeout rejection path in the deviceDisconnected promise leaves the listener attached; update the promise so the timeout handler also removes the listener by calling deviceManager.off(DeviceManagerEvent.deviceDisconnected, listener) before rejecting, ensuring the listener is cleaned up if NEW_DEVICE_ID never disconnects; keep the existing clearTimeout call in the success path and use the same listener reference so both paths remove it.src/device/protocol/virtual/virtualDeviceProviderFactory.ts (1)
41-44:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidation still allows
Infinityto pass.The current check on line 42 validates that
scanIntervalMsis a number and greater than zero, which correctly rejectsNaN, butInfinitywould still pass both conditions and causesetIntervalto effectively disable scanning.🛡️ Proposed fix to add finite check
- const scanIntervalMs = typeof config.scanIntervalMs === 'number' && config.scanIntervalMs > 0 + const scanIntervalMs = typeof config.scanIntervalMs === 'number' + && Number.isFinite(config.scanIntervalMs) + && config.scanIntervalMs > 0 ? config.scanIntervalMs : VirtualDeviceProviderFactory.DEFAULT_SCAN_INTERVAL_MS;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/device/protocol/virtual/virtualDeviceProviderFactory.ts` around lines 41 - 44, The validation in VirtualDeviceProviderFactory.create incorrectly allows Infinity for config.scanIntervalMs; update the conditional that computes scanIntervalMs (in the create method) to also require Number.isFinite(config.scanIntervalMs) (or isFinite) so only finite, positive numbers are accepted, otherwise fall back to VirtualDeviceProviderFactory.DEFAULT_SCAN_INTERVAL_MS; ensure this change affects the same variable used later when calling setInterval so scanning is not effectively disabled by Infinity.src/device/deviceManager.ts (1)
160-169:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
reset()can exit early ifdevice.close()throws, leaving remaining devices unclosed.Lines 166-168 correctly resolve pending acquire promises (improvement over past implementation), but lines 162-164 sequentially
awaiteachdevice.close()without error handling. If anyclose()rejects, the remaining devices inconnectedDeviceswon't be closed and queued acquires won't be cleared.🛡️ Proposed fix to make close errors non-blocking
public async reset(): Promise<void> { + const closeErrors: unknown[] = []; + for (const [, device] of this.connectedDevices) { - await device.close(); + try { + await device.close(); + } catch (e) { + closeErrors.push(e); + logError(this.logger, `device: ${device.getDeviceId} -> close -> failed during reset`, e); + } } for (const [deviceId] of this.detectedDeviceAcquireQueue) { this.clearDetectedDeviceAcquireQueue(deviceId, 'Device manager reset'); } + + if (closeErrors.length > 0) { + throw new Error(`Failed to close ${closeErrors.length} device(s) during 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 - 169, reset() currently awaits each device.close() sequentially so a rejection will abort the method and leave other devices unclosed and queued acquires uncleared; change the close loop to make close errors non-blocking (either use Promise.allSettled on Array.from(this.connectedDevices.values()).map(d => d.close()) or wrap each await device.close() in try/catch and log the error) and ensure the subsequent loop that calls clearDetectedDeviceAcquireQueue(deviceId, 'Device manager reset') still runs regardless of close errors; reference connectedDevices, device.close(), detectedDeviceAcquireQueue and clearDetectedDeviceAcquireQueue when making the change.
🧹 Nitpick comments (5)
tests/integration/helpers/appHelper.ts (2)
35-45: 💤 Low valueConsider adding cleanup on failure in
createTestApp.If
createApporloadFromSettingsthrows, the temporary directory will not be cleaned up. While test environments typically clean/tmpperiodically, explicit cleanup on failure would be better practice and aid debugging by preventing temp directory accumulation during test development.♻️ Proposed enhancement
export const createTestApp = async (): Promise<{ instance: AppInstance, tmpDir: string }> => { const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'slvctrlplus-test-')); - const dataPath = tmpDir + path.sep; - fs.writeFileSync(path.join(tmpDir, 'settings.json'), JSON.stringify(baseSettingsJson)); - - const instance = createApp({ dataPath }); - - await instance.container.get('device.provider.loader').loadFromSettings(); - - return { instance, tmpDir }; + try { + const dataPath = tmpDir + path.sep; + fs.writeFileSync(path.join(tmpDir, 'settings.json'), JSON.stringify(baseSettingsJson)); + + const instance = createApp({ dataPath }); + + await instance.container.get('device.provider.loader').loadFromSettings(); + + return { instance, tmpDir }; + } catch (error) { + fs.rmSync(tmpDir, { recursive: true }); + throw error; + } };🤖 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 35 - 45, The createTestApp helper currently creates a temp directory (tmpDir) and then calls createApp and await instance.container.get('device.provider.loader').loadFromSettings(); if either call throws, tmpDir is never removed; update createTestApp to perform cleanup on failure by wrapping the creation and setup steps in a try/catch/finally (or try with cleanup on catch) so that on any thrown error you remove the tmpDir (use fs.rmSync or fs.rmdirSync with recursive: true depending on Node version) before rethrowing the error; specifically modify createTestApp to ensure tmpDir is removed when createApp or loadFromSettings fail, referencing the tmpDir variable and the createApp and loadFromSettings calls.
47-60: ⚖️ Poor tradeoffConsider using try-finally to ensure cleanup in
teardownTestApp.If any cleanup step fails, subsequent steps (especially temp directory removal) won't execute. While test failures would surface this, ensuring all cleanup steps attempt to run would prevent resource leaks during test development.
♻️ Proposed enhancement
export const teardownTestApp = async (instance: AppInstance, tmpDir: string): Promise<void> => { - const scriptRuntime = instance.container.get('automation.scriptRuntime'); - if (scriptRuntime.isRunning()) { - await scriptRuntime.stop(); - } - - instance.container.get('device.provider.loader').stop(); - - await instance.container.get('device.manager').reset(); - - await new Promise<void>(resolve => instance.container.get('server.http').close(() => resolve())); - - fs.rmSync(tmpDir, { recursive: true }); + try { + const scriptRuntime = instance.container.get('automation.scriptRuntime'); + if (scriptRuntime.isRunning()) { + await scriptRuntime.stop(); + } + + instance.container.get('device.provider.loader').stop(); + + await instance.container.get('device.manager').reset(); + + await new Promise<void>(resolve => instance.container.get('server.http').close(() => resolve())); + } finally { + fs.rmSync(tmpDir, { recursive: true, force: true }); + } };Note: Added
force: truetormSyncto suppress errors if the directory doesn't exist.🤖 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 47 - 60, Wrap the teardown sequence in a try-finally inside teardownTestApp so every cleanup step is attempted even if an earlier one throws: perform the graceful actions (checking scriptRuntime.isRunning() -> scriptRuntime.stop(), device.provider.loader.stop(), await device.manager.reset(), await new Promise(resolve => server.http.close(() => resolve()))) inside the try and move fs.rmSync(tmpDir) into the finally block; also call fs.rmSync with the option { recursive: true, force: true } to suppress errors if the temp dir is missing.tests/integration/automationScripts.spec.ts (1)
185-206: 💤 Low valueConsider explicit
scriptRuntime.stop()after the test assertion.While
resetTestApplikely handles cleanup, explicitly stopping the runtime improves test clarity and consistency with other tests (lines 152, 177).♻️ Suggested addition
const logs = await logsPromise; + await scriptRuntime.stop(); + expect(logs).toContain(TEST_DEVICE_ID);🤖 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/automationScripts.spec.ts` around lines 185 - 206, After the assertion in the test, explicitly call scriptRuntime.stop() to shut down the loaded runtime; locate the test using the scriptRuntime variable in the 'onEvent is called with deviceDisconnected...' spec and add a scriptRuntime.stop() call after expect(logs).toContain(TEST_DEVICE_ID) so the runtime is cleaned up immediately instead of relying solely on resetTestApp.src/serviceProvider/settingsServiceProvider.ts (1)
28-34: ⚡ Quick winUse
path.joinfor cross-platform path construction.The current string concatenation
${dataPath}/settings.jsoncan produce double slashes whendataPathhas a trailing separator (e.g., from tests:tmpDir + path.sep), and on Windows it may mix\and/separators. Usepath.join(dataPath, 'settings.json')for correct cross-platform path handling.♻️ Proposed fix
const dataPath = this.dataPath ?? `${os.homedir()}/.slvctrlplus/`; if (false === fs.existsSync(dataPath)) { fs.mkdirSync(dataPath, { recursive: true }); } - const settingsFilePath = `${dataPath}/settings.json`; + const settingsFilePath = path.join(dataPath, 'settings.json');🤖 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/settingsServiceProvider.ts` around lines 28 - 34, The code builds settingsFilePath using string concatenation which can produce mixed or duplicate separators; update the logic that assigns settingsFilePath to use path.join(dataPath, 'settings.json') instead of `${dataPath}/settings.json`, and ensure the module imports/uses the Node "path" API (e.g., add or reuse the path import) so cross-platform separators are handled correctly; keep the existing dataPath and existsSync/mkdirSync checks but use path.join when constructing settingsFilePath.src/serviceProvider/repositoryServiceProvider.ts (1)
23-23: ⚡ Quick winUse
path.joinfor cross-platform path construction.String concatenation
${this.dataPath}/automation-scripts/can lead to double slashes or mixed separators on Windows whendataPathincludes a trailing separator. Usepath.join(this.dataPath, 'automation-scripts')instead.♻️ Proposed fix
container.set('repository.automationScript', () => { - const scriptsPath = `${this.dataPath}/automation-scripts/`; + const scriptsPath = path.join(this.dataPath ?? '', 'automation-scripts'); if (false === fs.existsSync(scriptsPath)) { fs.mkdirSync(scriptsPath);Note: You'll also need to import
pathat the top of the file:+import path from 'path';🤖 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` at line 23, Replace the string-concatenated scriptsPath assignment so it uses path.join to build a cross-platform path (e.g., change the usage of `${this.dataPath}/automation-scripts/` to path.join(this.dataPath, 'automation-scripts')); update the reference in repositoryServiceProvider where scriptsPath is set and ensure you add an import for the Node path module at the top of the file (import path from 'path' or const path = require('path') consistent with project style).
🤖 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/health/healthMetricsCollector.ts`:
- Around line 55-61: The start() method currently overwrites intervalHandle and
creates a new setIntervalAsync on each call, leaking previously scheduled
intervals; modify HealthMetricsCollector.start to check for an existing
this.intervalHandle and either return early or clear it before creating a new
interval, and ensure stop() clears and nulls this.intervalHandle so guards work
correctly — reference the start, stop, intervalHandle, setIntervalAsync, and
refresh symbols when making the change.
- Around line 57-60: The periodic call creating this.intervalHandle with
setIntervalAsync (which invokes the healthMetricsCollector.refresh method) lacks
an onError handler, so any exception in refresh can be rethrown and crash the
process; update the setIntervalAsync invocation in the constructor (or where
intervalHandle is assigned) to provide an onError callback that catches and logs
the error (using an injected logger field or fallback to console) and does not
rethrow, ensuring refresh failures are logged but do not crash the server.
In `@src/index.ts`:
- Line 9: The condition that defines allowedOrigins incorrectly checks `null !==
env.ALLOWED_ORIGINS.length`; update the boolean expression that constructs
allowedOrigins so it verifies that env.ALLOWED_ORIGINS is defined and
non-empty—e.g. check `typeof env.ALLOWED_ORIGINS !== 'undefined' &&
env.ALLOWED_ORIGINS.length > 0` or simply `env.ALLOWED_ORIGINS &&
env.ALLOWED_ORIGINS.length > 0`—so the code referencing env.ALLOWED_ORIGINS (the
allowedOrigins variable creation) properly detects an empty string instead of
performing the redundant null check on .length.
In `@src/serviceProvider/automationServiceProvider.ts`:
- Line 16: The logPath construction uses `${this.dataPath}/automation-logs/`
which yields "undefined/automation-logs/" when this.dataPath is missing; update
the AutomationServiceProvider constructor to validate that dataPath is provided
and throw a clear error if falsy, and change the logPath assignment (symbol:
logPath) to use a safe join (e.g., path.join(this.dataPath, 'automation-logs'))
so it never concatenates the string "undefined"; alternatively, if you prefer a
fallback, set this.dataPath = providedDataPath || process.cwd() in the
constructor before computing logPath and then compute logPath with
path.join(this.dataPath, 'automation-logs').
In `@tests/integration/automationScripts.spec.ts`:
- Around line 40-50: In waitForEvent, the timeout rejection path doesn't remove
the attached listener causing leaks; modify the timeout handler inside
waitForEvent (the function with params scriptRuntime and eventType) to call
scriptRuntime.off(eventType, listener) before rejecting so the listener is
removed on timeout, and keep the existing clearTimeout/resolve flow in the
success path to ensure the listener is also removed when the event fires.
---
Duplicate comments:
In `@src/device/deviceManager.ts`:
- Around line 160-169: reset() currently awaits each device.close() sequentially
so a rejection will abort the method and leave other devices unclosed and queued
acquires uncleared; change the close loop to make close errors non-blocking
(either use Promise.allSettled on
Array.from(this.connectedDevices.values()).map(d => d.close()) or wrap each
await device.close() in try/catch and log the error) and ensure the subsequent
loop that calls clearDetectedDeviceAcquireQueue(deviceId, 'Device manager
reset') still runs regardless of close errors; reference connectedDevices,
device.close(), detectedDeviceAcquireQueue and clearDetectedDeviceAcquireQueue
when making the change.
In `@src/device/protocol/virtual/virtualDeviceProviderFactory.ts`:
- Around line 41-44: The validation in VirtualDeviceProviderFactory.create
incorrectly allows Infinity for config.scanIntervalMs; update the conditional
that computes scanIntervalMs (in the create method) to also require
Number.isFinite(config.scanIntervalMs) (or isFinite) so only finite, positive
numbers are accepted, otherwise fall back to
VirtualDeviceProviderFactory.DEFAULT_SCAN_INTERVAL_MS; ensure this change
affects the same variable used later when calling setInterval so scanning is not
effectively disabled by Infinity.
In `@tests/integration/deviceEvents.spec.ts`:
- Around line 131-138: The promise that waits for disconnection registers a
listener on deviceManager (DeviceManagerEvent.deviceDisconnected) but never
removes it on success or timeout; update the Promise executor to store the
listener function in a const, call
deviceManager.removeListener(DeviceManagerEvent.deviceDisconnected, listener)
(or deviceManager.off(...)) inside the success handler before resolve and inside
the timeout handler before reject, and still clearTimeout(timeout) appropriately
so the listener is removed in both paths.
- Around line 54-73: The timeout handler currently rejects the promise but
doesn't remove the registered listener, causing listeners to accumulate; update
the timeout callback so it also calls
deviceManager.off(DeviceManagerEvent.deviceRefreshed, listener) before
rejecting, and keep the existing clearTimeout(timeout) in the success path
(listener) to avoid stray timers; locate the Promise block where
deviceManager.on(DeviceManagerEvent.deviceRefreshed, listener) is added and
remove the listener in the timeout branch to ensure cleanup.
- Around line 104-115: The timeout rejection path in the deviceDisconnected
promise leaves the listener attached; update the promise so the timeout handler
also removes the listener by calling
deviceManager.off(DeviceManagerEvent.deviceDisconnected, listener) before
rejecting, ensuring the listener is cleaned up if NEW_DEVICE_ID never
disconnects; keep the existing clearTimeout call in the success path and use the
same listener reference so both paths remove it.
---
Nitpick comments:
In `@src/serviceProvider/repositoryServiceProvider.ts`:
- Line 23: Replace the string-concatenated scriptsPath assignment so it uses
path.join to build a cross-platform path (e.g., change the usage of
`${this.dataPath}/automation-scripts/` to path.join(this.dataPath,
'automation-scripts')); update the reference in repositoryServiceProvider where
scriptsPath is set and ensure you add an import for the Node path module at the
top of the file (import path from 'path' or const path = require('path')
consistent with project style).
In `@src/serviceProvider/settingsServiceProvider.ts`:
- Around line 28-34: The code builds settingsFilePath using string concatenation
which can produce mixed or duplicate separators; update the logic that assigns
settingsFilePath to use path.join(dataPath, 'settings.json') instead of
`${dataPath}/settings.json`, and ensure the module imports/uses the Node "path"
API (e.g., add or reuse the path import) so cross-platform separators are
handled correctly; keep the existing dataPath and existsSync/mkdirSync checks
but use path.join when constructing settingsFilePath.
In `@tests/integration/automationScripts.spec.ts`:
- Around line 185-206: After the assertion in the test, explicitly call
scriptRuntime.stop() to shut down the loaded runtime; locate the test using the
scriptRuntime variable in the 'onEvent is called with deviceDisconnected...'
spec and add a scriptRuntime.stop() call after
expect(logs).toContain(TEST_DEVICE_ID) so the runtime is cleaned up immediately
instead of relying solely on resetTestApp.
In `@tests/integration/helpers/appHelper.ts`:
- Around line 35-45: The createTestApp helper currently creates a temp directory
(tmpDir) and then calls createApp and await
instance.container.get('device.provider.loader').loadFromSettings(); if either
call throws, tmpDir is never removed; update createTestApp to perform cleanup on
failure by wrapping the creation and setup steps in a try/catch/finally (or try
with cleanup on catch) so that on any thrown error you remove the tmpDir (use
fs.rmSync or fs.rmdirSync with recursive: true depending on Node version) before
rethrowing the error; specifically modify createTestApp to ensure tmpDir is
removed when createApp or loadFromSettings fail, referencing the tmpDir variable
and the createApp and loadFromSettings calls.
- Around line 47-60: Wrap the teardown sequence in a try-finally inside
teardownTestApp so every cleanup step is attempted even if an earlier one
throws: perform the graceful actions (checking scriptRuntime.isRunning() ->
scriptRuntime.stop(), device.provider.loader.stop(), await
device.manager.reset(), await new Promise(resolve => server.http.close(() =>
resolve()))) inside the try and move fs.rmSync(tmpDir) into the finally block;
also call fs.rmSync with the option { recursive: true, force: true } to suppress
errors if the temp dir is missing.
🪄 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: 8963cff5-338e-40bf-bb79-d8ad05e03484
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (29)
.env.example.github/workflows/test.ymleslint.config.tspackage.jsonresources/schemas/settings.schema.jsonsrc/app.tssrc/automation/scriptRuntime.tssrc/controller/automation/stopScriptController.tssrc/controller/healthController.tssrc/device/deviceManager.tssrc/device/protocol/virtual/randomGenerator/randomGeneratorVirtualDeviceLogic.tssrc/device/protocol/virtual/virtualDeviceProvider.tssrc/device/protocol/virtual/virtualDeviceProviderFactory.tssrc/device/provider/deviceProvider.tssrc/device/provider/deviceProviderLoader.tssrc/env.tssrc/health/healthMetricsCollector.tssrc/index.tssrc/serviceProvider/automationServiceProvider.tssrc/serviceProvider/repositoryServiceProvider.tssrc/serviceProvider/settingsServiceProvider.tstests/integration/api.spec.tstests/integration/automationScripts.spec.tstests/integration/deviceEvents.spec.tstests/integration/helpers/appHelper.tstests/unit/automation/scriptRuntime.spec.tstests/unit/controller/healthController.spec.tsvitest.config.integration.tsvitest.config.ts
💤 Files with no reviewable changes (1)
- resources/schemas/settings.schema.json
heavyrubberslave
marked this pull request as ready for review
June 9, 2026 19:52
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 139-140: The parseEnv validation currently allows DATA_PATH to be
an empty or whitespace string which then gets passed into createApp and causes
provider paths to resolve to root; update the parseEnv rule for DATA_PATH (the
Type.String declaration) to enforce non-empty trimmed input (e.g., minLength:1
plus trim/regex to reject all-whitespace) or, alternatively, normalize DATA_PATH
after parsing by trimming and treating '' as undefined before it is used in
src/index.ts or passed into createApp; change the DATA_PATH check in parseEnv or
the place that constructs the AppOptions so createApp and any providers see
either a valid non-empty path or undefined.
🪄 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: 52aa3106-e28b-4c4f-90f5-5bdbaa53a2fa
📒 Files selected for processing (7)
src/app.tssrc/health/healthMetricsCollector.tssrc/index.tssrc/serviceProvider/automationServiceProvider.tssrc/serviceProvider/healthServiceProvider.tssrc/serviceProvider/repositoryServiceProvider.tstests/integration/automationScripts.spec.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/serviceProvider/automationServiceProvider.ts
- src/serviceProvider/repositoryServiceProvider.ts
- tests/integration/automationScripts.spec.ts
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/integration/automationScripts.spec.ts (1)
93-104: ⚡ Quick winQuestionable cleanup call after failed request.
Line 103 calls
scriptRuntime.stop()after receiving a 400 response, but the controller (per context snippet 2) returns 400 early without loading the script. Ifstop()is safe to call when no script is running, this is defensive but potentially confusing; if it should throw, this might mask issues.💭 Consider removing unnecessary stop() call
expect(res.status).toBe(400); - - await scriptRuntime.stop(); });🤖 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/automationScripts.spec.ts` around lines 93 - 104, The test calls await scriptRuntime.stop() after a 400 response even though the controller returns early and never loads a script; remove the unnecessary cleanup call (await scriptRuntime.stop()) from the 'POST /automation/run returns 400 for non-text/plain content type' test (or replace it with a defensive check like only calling stop() if scriptRuntime.isRunning() exists and returns true) so the test no longer masks potential stop() errors and more accurately reflects the controller's early-return behavior.
🤖 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.
Nitpick comments:
In `@tests/integration/automationScripts.spec.ts`:
- Around line 93-104: The test calls await scriptRuntime.stop() after a 400
response even though the controller returns early and never loads a script;
remove the unnecessary cleanup call (await scriptRuntime.stop()) from the 'POST
/automation/run returns 400 for non-text/plain content type' test (or replace it
with a defensive check like only calling stop() if scriptRuntime.isRunning()
exists and returns true) so the test no longer masks potential stop() errors and
more accurately reflects the controller's early-return behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a407af86-e663-4c99-b521-36ed57703415
📒 Files selected for processing (8)
src/app.tssrc/env.tssrc/health/healthMetricsCollector.tssrc/index.tssrc/serviceProvider/automationServiceProvider.tssrc/serviceProvider/healthServiceProvider.tssrc/serviceProvider/repositoryServiceProvider.tstests/integration/automationScripts.spec.ts
✅ Files skipped from review due to trivial changes (1)
- src/serviceProvider/healthServiceProvider.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- src/env.ts
- src/index.ts
- src/serviceProvider/automationServiceProvider.ts
- src/health/healthMetricsCollector.ts
- src/serviceProvider/repositoryServiceProvider.ts
- src/app.ts
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores