Watch settings file external changes - #98
Merged
heavyrubberslave merged 17 commits intoAug 2, 2026
Merged
Conversation
|
Warning Review limit reached
Next review available in: 43 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughChangesThe application now watches settings files and reloads valid external changes. Device detection uses branded Settings and device lifecycle
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Application
participant SettingsManager
participant SettingsFile
participant ObservableSettings
Application->>SettingsManager: startWatching()
SettingsFile-->>SettingsManager: external file change
SettingsManager->>SettingsFile: read and validate content
SettingsManager->>ObservableSettings: apply valid settings
SettingsManager-->>Application: emit settingsChanged
sequenceDiagram
participant DeviceProvider
participant SharedObserver
participant SerialPortObserver
participant SynchronousSerialPort
DeviceProvider->>SharedObserver: start()
SharedObserver->>SerialPortObserver: onSubsequentStart()
SerialPortObserver-->>DeviceProvider: re-announce managed devices
DeviceProvider->>SharedObserver: stop()
SerialPortObserver->>SynchronousSerialPort: close()
SynchronousSerialPort-->>SerialPortObserver: resolve close promise
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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
force-pushed
the
feat/watch-settings-file-external-changes
branch
from
July 25, 2026 07:30
b821854 to
408e8f1
Compare
Watches settings.json with chokidar so external edits (manual changes, other tooling) trigger the same reload pipeline as changes made through the API - device providers and devices get re-applied without a restart. Own writes from save()/replace() are recognized via a content comparison against the last content we wrote, so they don't trigger a redundant reload loop. Invalid JSON or schema-invalid content from an external edit is logged and ignored, keeping the previously loaded settings.
SerialPortObserver.onLastStop() never cleared managedDevices, so a provider restarting the shared observer after a full stop (all users gone) had discoverSerialDevices() treat every already-plugged port as "already managed" and silently skip re-announcing it - devices that disappeared when their source got disabled never came back on re-enable. onLastStop() now revokes every still-tracked device through DeviceManager before clearing managedDevices, mirroring what discoverSerialDevices() already does per-port when a device disappears.
…es when joining an already-running observer
- SynchronousSerialPort.close() now calls the SerialPortStream's own
close() (which releases the OS-level handle) instead of generic
stream end()/destroy(), and returns a promise that resolves only
once the port is actually released. Without this, a device source
disabled then re-enabled could fail to reopen the same port path
('Port is locked').
- Fixed a re-entrancy bug this exposed: destroying the writer/reader
after close() re-triggers their 'close' event, which looped back
into the device's close callback a second time while the original
close() was still in flight. handleClose() now recognizes a
self-initiated close and skips re-invoking the callback.
- SharedObserver gained an onSubsequentStart() hook, invoked when a
provider joins an observer that's already running (as opposed to
the very first joiner, including concurrent first-time joins).
SerialPortObserver implements it by re-announcing managed devices
that aren't currently connected. Root cause: all serial protocol
providers (SlvCtrl+, zc95, estim2b) share one SerialPortObserver
instance, so disabling a single device source doesn't stop the
observer while other sources keep it running - the observer never
reruns discovery, so a re-enabled provider never got a chance at
devices the observer already knew about.
Tests: unit coverage for SharedObserver's subsequent-start hook and
SerialPortObserver's catch-up announce; new integration test file
covering the reconnect scenario with all three serial providers
sharing an observer, matching the real multi-source configuration
that exposed the bug.
… re-enabled DeviceManager.applySettingsChange() closed a connected device whose known device just got disabled, but - unlike offerDevice()'s reject-at-connect-time path - never registered a pending retry for it. So re-enabling that specific known device (without touching its device source) never re-announced it: the device's provider never stops/restarts in this scenario, so nothing else would trigger a retry either. DeviceManager now retains each connected device's original DeviceDetectionInfo (connectedDeviceInfos), so a device closed for this reason can be registered in detectedDisabledDevices for retry the same way an offerDevice()-rejected one already is. Ported forward from the now-superseded claiming-arbitration branch, adapted onto the DetectedDeviceOfferQueue-based design merged in #99.
heavyrubberslave
force-pushed
the
feat/watch-settings-file-external-changes
branch
from
August 2, 2026 06:24
af47618 to
3f079be
Compare
DeviceManager.connectedDevices was the only collection field taking
external input via the constructor - every other internal collection
(detectedDisabledDevices, settingsChangeQueue, the offer queue) already
owns its state internally. That inconsistency also let a caller seed
connectedDevices directly (several existing unit tests did), bypassing
offerDevice()/registerDevice() and their DeviceDetectionInfo bookkeeping
entirely - so the two could start out-of-sync from construction,
independent of any internal code path.
Removed the constructor parameter entirely (no production caller ever
needed the same Map instance back - deviceServiceProvider.ts always
passed a fresh empty one, and no test read the injected map after
construction either), and merged the previously separate
connectedDeviceInfos map into connectedDevices as a single map of
{ device, deviceDetectionInfo } entries. registerDevice() is now the
only way to populate it, which structurally guarantees a connected
device always has its detection info available - removing the
'if (undefined !== deviceDetectionInfo)' guard applySettingsChange()
needed previously.
Updated the two unit tests that seeded connectedDevices directly to go
through the connectDevice() test helper instead, and dropped the
now-unused constructor argument at all other call sites.
Ported forward from the now-superseded claiming-arbitration branch
(af47618), adapted onto the DetectedDeviceOfferQueue-based design
merged in #99.
A device's preliminary/raw id at the point it's detected can differ from its final, canonical DeviceId - some protocols only learn the real one during a handshake (e.g. zc95's firmware serial number). Both were previously typed as plain DeviceId, so nothing stopped one from being passed where the other was meant - which had already hidden a real bug (see below). - deviceId.ts: add a DetectionId branded type alongside DeviceId, plus explicit DeviceId.fromDetectionId()/DetectionId.fromDeviceId() escape hatches for the places that deliberately treat one as the other. - deviceManager.ts: DeviceDetectionInfo.detectionId is now a DetectionId; detectedDisabledDevices is keyed by DetectionId (it always was, just mistyped as DeviceId); connectedDevices is keyed by DeviceId and getConnectedDevice() takes DeviceId, matching its only production caller (ConnectedDeviceRepository) which already required it. - fix: announceDetectedDevice()/offerDevice()'s already-connected check used to look up connectedDevices (keyed by canonical id) directly by detection id, which only worked by coincidence when the two are equal. For a device whose canonical id is only learned during a handshake (e.g. zc95 with fw >=2.0, whose canonical id comes from its firmware serial number rather than the serial port's) this silently never matched, so an already-connected device could be re-announced. Replaced with isDetectedDeviceAlreadyConnected(), which scans connected devices by their stored detection id instead of guessing a canonical id. - serialPortObserver.ts: onSubsequentStart() had the exact same bug via getConnectedDevice(). Since announceDetectedDevice() now correctly no-ops for an already-connected device on its own, the check here was redundant - removed entirely rather than fixed twice. - detectedDeviceOfferQueue.ts: keyed and parameterized by DetectionId. - serialPortObserver.ts, bleObserver.ts, virtualDeviceProvider.ts, buttplugIoWebsocketDeviceProvider.ts: construct detectionId via DetectionId.create()/fromDeviceId() instead of DeviceId.create(). - serializedTypes.ts: SerializedDevice.deviceId is now DeviceId instead of a plain string, matching WebSocket/HTTP payloads that always carried the canonical id. Ported forward from the now-superseded claiming-arbitration branch (484f3dd), separated from the parts of that commit already superseded by the DetectedDeviceOfferQueue-based design merged in #99.
…as the canonical device id Providers for protocols with no way to derive a better canonical id (estim2b, slvCtrlPlus, airotic, buttplugIo) were casting their detection id to DeviceId themselves via DeviceId.fromDetectionId() before calling their factory's create() - a decision providers have no real basis to make, since identity derivation is protocol-specific knowledge that belongs with whatever constructs the device. zc95 already got this right: its factory takes the detection id as a fallback and prefers a firmware-reported serial number when available (fw >=2.0). Applied the same shape to the other four factories, whose create() now takes a DetectionId instead of a pre-cast DeviceId and converts it internally - currently always falling back to the detection id itself, since none of these protocols' handshakes carry a better per-device identifier today. If one of them ever gains a real handshake-derived id, only that factory needs to change. virtualDeviceFactory is unaffected - it already takes a KnownDevice directly, with no detection/canonical id split to begin with.
SettingsManager.load()/startWatching() were called as a side effect
inside SettingsServiceProvider's DI factory, the only service in the
codebase that self-starts this way - every other stateful service
(health.metricsCollector, device.provider.manager, automation.scriptRuntime)
is constructed inertly by its provider and explicitly started/stopped
from createApp()/shutdown().
That made settings watching start implicitly, whenever something first
happened to call container.get('settings.manager') (currently
configureWebsocket(), incidentally) - and created an asymmetry with
stopWatching(), which was already called explicitly in shutdown().
Both load() and startWatching() are idempotent (no-op if already
loaded/watching), so moving them to explicit calls in createApp(),
before anything that depends on settings runs, is behavior-preserving.
heavyrubberslave
marked this pull request as ready for review
August 2, 2026 10:49
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@package.json`:
- Line 13: Update the Docker Compose development service’s Node image to a Node
20-or-newer image, ensuring the local runtime supports chokidar 5 and aligns
with the package and CI Node requirements. Locate the compose service
configuration rather than changing the chokidar dependency declaration.
In `@src/app.ts`:
- Around line 269-270: Guard the settings manager cleanup call in shutdown() by
wrapping await container.get('settings.manager').stopWatching() in try/catch and
reporting failures through the existing logError pattern. Ensure an exception
does not abort subsequent shutdown steps such as stopProviders(), metrics
shutdown, websocket closure, or server closure.
In `@src/device/transport/serialPortObserver.ts`:
- Around line 63-77: Prevent stale serial discovery from repopulating
managedDevices after shutdown by adding an active/generation cancellation guard
in the observer class. Set or advance the guard in onLastStop() before revoking
devices, and have discoverSerialDevices() validate the captured guard after
awaiting SerialPort.list() and before writing or announcing entries; preserve
normal discovery and restart behavior by invalidating only the canceled
discovery generation.
In `@src/settings/settingsManager.ts`:
- Around line 62-72: Update load() to wrap JSON.parse(fileContent) in a
try/catch, logging parse failures with logError using the same invalid-settings
context as the existing transformPlainToSettings handler, then rethrow the
original error. Keep the subsequent transformation and lastWrittenContent flow
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 Plus
Run ID: 61956f8b-cd9b-4a74-8ef6-378e377332d1
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (28)
package.jsonsrc/app.tssrc/device/detectedDeviceOfferQueue.tssrc/device/deviceId.tssrc/device/deviceManager.tssrc/device/protocol/airotic/airoticDeviceFactory.tssrc/device/protocol/buttplugIo/buttplugIoDeviceFactory.tssrc/device/protocol/buttplugIo/buttplugIoWebsocketDeviceProvider.tssrc/device/protocol/estim2b/estim2bDeviceFactory.tssrc/device/protocol/slvCtrlPlus/slvCtrlPlusDeviceFactory.tssrc/device/protocol/virtual/virtualDeviceProvider.tssrc/device/protocol/zc95/zc95DeviceFactory.tssrc/device/serializedTypes.tssrc/device/transport/bleObserver.tssrc/device/transport/serialPortObserver.tssrc/device/transport/sharedObserver.tssrc/serial/synchronousSerialPort.tssrc/serviceProvider/deviceServiceProvider.tssrc/serviceProvider/settingsServiceProvider.tssrc/settings/settingsManager.tstests/integration/devices/sharedSerialObserver.spec.tstests/unit/device/detectedDeviceOfferQueue.spec.tstests/unit/device/deviceManager.spec.tstests/unit/device/protocol/zc95/zc95DeviceFactory.spec.tstests/unit/device/provider/deviceProvider.spec.tstests/unit/device/transport/serialPortObserver.spec.tstests/unit/device/transport/sharedObserver.spec.tstests/unit/settings/settingsManager.spec.ts
💤 Files with no reviewable changes (1)
- src/serviceProvider/deviceServiceProvider.ts
…rial devices after stop SerialPortObserver.onLastStop() revoked and cleared managedDevices without waiting for or cancelling a rescan that was still awaiting SerialPort.list() (scheduled by the debounced USB event handler). Once that stale call resolved, it saw managedDevices as empty and treated every still-plugged-in port as newly discovered, writing them back and re-announcing them - even though the observer had already fully stopped. A later restart would then skip re-announcing those ports, since discoverSerialDevices() saw them as already managed. - util/latestOnlyTaskQueue.ts: exported (was unused/untracked), and given a cancel() passthrough to SequentialTaskQueue.cancel() - needed for the stop-and-stay-stopped case, where nothing will call run() again to supersede the stale task. - serialPortObserver.ts: the debounced rescan now runs through a discoveryQueue (LatestOnlyTaskQueue), so a later rescan trigger always supersedes a still-running one instead of racing it, and onLastStop() cancels it outright if nothing supersedes it first. discoverSerialDevices() takes an optional CancellationToken and bails out - without touching managedDevices - if it's been cancelled by the time SerialPort.list() resolves. - onFirstStart()'s own (non-debounced) discovery call intentionally bypasses the queue: SharedObserver keeps activeUsers at 0 until it resolves, so onLastStop() can't run concurrently with it. - bumped @timesplinter/sequential-task-queue to 1.4.0, whose CancellablePromiseLike now extends Promise instead of PromiseLike, so the debounced rescan's error handling can use .catch() directly instead of wrapping in Promise.resolve() first. Found by CodeRabbit on PR #98.
SynchronousSerialPort.close()/handleClose() only called queue.cancel(), which clears pending/in-flight tasks but leaves the queue open for new ones. In close(), that left a real window: after cancel() but before writer.close()'s callback actually destroys the streams, a racing write()/writeAndExpect() call would still be queued and reach the still-open writer. close(true) sets the queue's closed flag synchronously in the same call, so a racing write() now throws immediately instead. Also enabled checkThenables on @typescript-eslint/no-floating-promises, since it was previously never checking values typed as the (structural) PromiseLike our task queue used to return instead of a real Promise - now that the queue returns real Promises (see previous commit's dependency bump), plain 'warn' would have started flagging the queue.cancel()/close() calls above. Fixed the resulting warnings with void, and dropped the now-unnecessary Promise.resolve() wrap in DetectedDeviceOfferQueue.offer(), which existed for the same PromiseLike-vs-Promise reason.
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 `@docker/dev/node/Dockerfile`:
- Line 1: Update the Dockerfile to ensure the container runs as a non-root user:
make /app writable for the intended user, then add a USER instruction switching
away from root while preserving the existing Node image setup.
🪄 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 Plus
Run ID: 9344238c-938e-444a-ae7a-586d08c34af1
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
docker/dev/node/Dockerfileeslint.config.tspackage.jsonsrc/app.tssrc/device/detectedDeviceOfferQueue.tssrc/device/transport/serialPortObserver.tssrc/serial/synchronousSerialPort.tssrc/settings/settingsManager.tssrc/util/latestOnlyTaskQueue.tstests/unit/device/transport/serialPortObserver.spec.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- package.json
- src/app.ts
- src/device/detectedDeviceOfferQueue.ts
- src/settings/settingsManager.ts
…n any close
SynchronousSerialPort.onClose() registered an independent listener/closure
per call, sharing one 'this.closed' flag - so when called more than once on
the same instance (PeripheralDevice and MessageResponseHandler both do),
whichever registered first would flip that flag before the next one's own
check ran, and only the first-registered subscriber's callback ever actually
fired on a real disconnect. For zc95 devices (the only ones combining both),
this meant PeripheralDevice's own onClose callback - the one that sets
device state to closed and emits deviceDisconnected, which DeviceManager
depends on for cleanup - never fired on a physical disconnect.
- writer/reader now share a single 'close' listener registered once in the
constructor; onClose() just appends to a closeSubscribers list, notified
unconditionally whenever the port closes, for any reason - not just
physical disconnects. That makes the class's own contract simple and
self-contained ('closed' means closed, regardless of cause) instead of
encoding assumptions about what a caller does with the notification.
- close()/doClose() now also destroy() both streams even if only one of
them fired 'close' (e.g. a parser-level error killing the reader while the
physical port is still fine) - previously only the stream that closed
itself got cleaned up, the other was neither released nor negotiated
closed.
- close() still negotiates a real writer.close() before destroying if the
writer is still open, since destroy() alone never releases the OS-level
port handle (SerialPortStream has no _destroy() override - only its own
close() calls the binding's close()).
Notifying subscribers on every close (not just disconnects) exposed a
pre-existing re-entrancy bug in Device.close(): its only guard
(state === closed) is set in a finally block, so a second call arriving
while the first is still in-flight wasn't blocked, and PeripheralDevice's
own onClose callback (which calls this.close()) would run again from
inside SynchronousSerialPort's own close chain, causing deviceDisconnected
to fire twice - or, once Device.close() was given a proper closePromise
memoization guard, deadlock entirely, since the re-entrant call would await
the very promise whose completion it was blocking.
- Device.close() now memoizes its own closePromise, same pattern as
SynchronousSerialPort, so concurrent/re-entrant calls all resolve
together once actually done instead of re-running doClose().
- Added DeviceState.closing, set for the duration of close(); refresh()
and DeviceManager's refresh-interval skip both treat it like closed.
- PeripheralDevice's onClose callback no longer awaits its own close()
call - it's dispatched fire-and-forget, since awaiting it can mean
awaiting the very close chain that's currently invoking it.
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/serial/synchronousSerialPort.ts`:
- Around line 28-30: Prevent unhandled rejections from fire-and-forget close
operations: update handleStreamClose in src/serial/synchronousSerialPort.ts
(lines 28-30) to attach a catch handler using the existing logError helper and
this.logger, while preserving the non-awaited close to avoid the circular wait;
update the transport.onClose callback in src/device/peripheralDevice.ts (lines
38-40) to attach a catch handler that logs or otherwise surfaces close errors
instead of discarding the rejection.
🪄 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 Plus
Run ID: 82f8eaa4-f747-45a9-97de-e9402915d0c6
📒 Files selected for processing (5)
src/device/device.tssrc/device/deviceManager.tssrc/device/deviceState.tssrc/device/peripheralDevice.tssrc/serial/synchronousSerialPort.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/device/deviceManager.ts
…viceSimulator The simulator's write() override schedules the device's response via setImmediate(), a later macrotask. If the port gets closed (a disconnect racing the response) before that callback fires, MockPortBinding.emitData() throws 'Port must be open to pretend to receive data' as an uncaught exception - the source of the intermittent 'Port must be open...' flake seen across integration test runs. Guard the deferred call with an isOpen check; if the port's already closed, nothing is listening for the response anymore anyway.
…base Device CodeRabbit (PR #98): SynchronousSerialPort.handleStreamClose() and PeripheralDevice's onClose callback both use 'void this.close()' - deliberately not awaited, to avoid a circular wait (each runs from within the very close chain that awaiting would depend on). But a bare void discards the promise entirely: any eventual rejection becomes an unhandled promise rejection, which terminates the Node process by default. Neither currently rejects in practice, but that's incidental to the code as written today, not something actually enforced - not worth staking process uptime on. Fixed both with .catch(), which doesn't reintroduce the circular-wait problem (it doesn't await anything, same as void) but does give any eventual rejection somewhere to go. Device had no Logger to log to, so this also plumbs one into the base class: - Device gets a new required logger: Logger constructor param, and scopes it itself via logger.child({ name: new.target.name }) - new.target (not this.constructor, which isn't available until every constructor in the chain has run) is the actual concrete class new was called on, e.g. AiroticDevice - before storing it as a protected readonly logger field. - Every subclass already received logger as its own last constructor param; some stored it locally raw (SlvCtrlPlusDevice, EStim2bDevice, VirtualDevice), some additionally scoped it themselves first (Zc95Device, BleDevice, ButtplugIoDevice). All of them now just forward the raw logger straight to super() and rely on the inherited, uniformly self-scoping field instead of duplicating the scoping decision themselves. Zc95Device's scoping used to be more specific than just the class name (it included the transport's device identifier, so multiple connected zc95 devices had distinguishable log lines) - traded that away deliberately for a single, uniform mechanism. - No public constructor signature changed for any concrete device class, since logger was already their last param; only Device's and PeripheralDevice's own signatures grew it, rippling through each subclass's super() call. - Test-only Device subclasses (TestDevice, StubDevice) pass a mock<Logger>() internally rather than taking it as a new param, so none of their many existing call sites needed updating. The bleDeviceTransport.ts console.error → logError (same CodeRabbit thread, different file - BleUartDeviceTransport isn't a Device subclass and has no logger of its own) is left for a separate follow-up.
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 `@tests/unit/automation/scriptRuntime.spec.ts`:
- Line 31: Configure Logger.child() on the mock<Logger>() fixtures before
constructing both Device instances: update StubDevice in
tests/unit/automation/scriptRuntime.spec.ts:31-31 and TestDevice in
tests/unit/device/testDevice.ts:16-16 so child() returns a Logger mock,
preserving valid logger initialization in each fixture.
🪄 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 Plus
Run ID: 633c194c-2d70-49db-8df6-a920221d2e60
📒 Files selected for processing (12)
src/device/bleDevice.tssrc/device/device.tssrc/device/peripheralDevice.tssrc/device/protocol/buttplugIo/buttplugIoDevice.tssrc/device/protocol/estim2b/estim2bDevice.tssrc/device/protocol/slvCtrlPlus/slvCtrlPlusDevice.tssrc/device/protocol/virtual/virtualDevice.tssrc/device/protocol/zc95/zc95Device.tssrc/serial/synchronousSerialPort.tstests/integration/helpers/slvCtrlPlusDeviceSimulator.tstests/unit/automation/scriptRuntime.spec.tstests/unit/device/testDevice.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/device/device.ts
CodeRabbit (PR #98): Device's constructor calls logger.child(...) to build its own scoped logger. TestDevice and StubDevice both passed a bare mock<Logger>() with child() left unconfigured, which vitest-mock-extended returns undefined from by default - so this.logger ended up undefined for every device built through either fixture, despite its declared type being non-nullable. Doesn't break anything today since nothing in Device's base methods or either fixture currently calls a method on it, but any future logging call added to Device would immediately fail every test using these fixtures with a confusing 'Cannot read properties of undefined' instead of pointing at the real cause. Fixed by configuring child() to return a mock logger, matching the pattern already used for a different Logger instance elsewhere in scriptRuntime.spec.ts.
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