Simplify device obtain process - #99
Merged
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:
📝 WalkthroughWalkthroughDevice detection now uses per-detection offer queues. ChangesDevice offer acquisition flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant DeviceProvider
participant DeviceManager
participant DetectedDeviceOfferQueue
participant Device
DeviceProvider->>DeviceManager: offerDevice(detectionInfo, createDevice)
DeviceManager->>DetectedDeviceOfferQueue: enqueue offer
DetectedDeviceOfferQueue->>Device: create and evaluate device
Device-->>DetectedDeviceOfferQueue: device or error
DetectedDeviceOfferQueue-->>DeviceManager: OfferResult
DeviceManager-->>DeviceProvider: success or rejection
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 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 |
![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
🤖 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 263-283: Update runNextInQueue to remove the processed entry from
deviceQueue after resolving its offer, then advance to the next queued entry
when present. When the queue becomes empty, clear the corresponding
detectedDeviceAcquireQueue entry so announceDetectedDevice can schedule future
offers and disabled-device retries can be re-announced.
In `@src/device/provider/deviceProvider.ts`:
- Around line 108-118: Restore an isStopped() guard immediately after the await
in the offerDevice flow, before handling the result or registering
result.device. If the provider has been stopped while offerDevice was pending,
close or otherwise abort the newly offered device using the existing
stop/cleanup path and return without adding it to connectedDevices.
🪄 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: dfcb8218-1cc9-44f3-bb62-288ff3de4f81
📒 Files selected for processing (2)
src/device/deviceManager.tssrc/device/provider/deviceProvider.ts
…e gating - DeviceManager.offerDevice(): advance the acquire queue to the next waiter on every non-terminal offer outcome (undefined, thrown, disabled-rejected), not just on success. Previously a failed offer left the queue stuck, breaking the multi-provider fallback (e.g. trying multiple serial protocols against the same detected port) and permanently blocking re-announcement of that detection id. - Add DeviceOfferRejectedError to distinguish manager-level rejections (disabled, claimed elsewhere, revoked, unavailable) from a real connect failure, so DeviceProvider only runs onConnectFailed() for the latter. - DeviceProvider: restore the isStopped() guard (checked at offer execution time) so a device that connects after the provider was stopped gets closed instead of registered. - DeviceProvider: track a device in the provider's own connectedDevices map inside the offer closure, before handing it back to the manager, instead of after the awaited offerDevice() call resolves. addDevice() emits deviceConnected synchronously as part of that resolution, so the previous ordering left a window where external listeners of that event (e.g. VirtualDeviceProvider's settings-change discovery) could observe the device as connected before the owning provider's own bookkeeping reflected it - causing dynamic device removal handling to silently drop devices depending on scheduling. Also log connected device count immediately at each add/remove instead of reading the shared count later, avoiding redundant/misleading repeated log lines. - DeviceManager.announceDetectedDevice(): remove the early isDeviceEnabled(detectionId) gate. Detection id is preliminary and can be shared by multiple protocol providers computing distinct canonical ids (e.g. several serial protocols probing the same port); gating on it incorrectly blocked every provider whenever it happened to collide with an unrelated disabled known device. The actual disabled check now happens once via addDevice()'s canonical-id check after a provider connects, which already drives the pending-retry re-announce mechanism on its own. - Rewrite/extend unit tests for the new offerDevice() API and the above behaviors.
Replace .then()/.catch() chaining with a single try/await/catch block - same behavior and microtask timing, just more linear to read. advanceQueue() stays synchronous since it has no async work of its own; its recursive call into runNextInQueue() and the executor call site in offerDevice() are marked void, matching the existing fire-and-forget convention used elsewhere in the codebase.
- Inline registerPendingRetry() into addDevice(), its only caller now that announceDetectedDevice() no longer has its own disabled-gate call site. - Dedup the repeated resolve-failure+advance-queue pairing in runNextInQueue() into a local rejectAndAdvance() closure. - Drop the unnecessary async modifier from offerDevice() - its body just constructs and returns a Promise directly, no await needed. - Inline the trivial single-use refreshDevice()/removeDevice() helpers directly into their event listener registrations in addDevice(). - Reword advanceQueue()'s doc comment to drop the reference to the no-longer-existing releaseDetectedDevice(). - Rename leftover acquire/claim terminology to match the offer-based model: AcquireResult -> OfferResult, detectedDeviceAcquireQueue -> detectedDeviceOfferQueue, clearDetectedDeviceAcquireQueue -> clearDetectedDeviceOfferQueue, plus matching comment/log wording. - Remove the dead eventEmitter field from the DeviceProvider base class - assigned in the constructor but never read anywhere in the base class or any of its ~8 subclasses. Threaded the removal through every provider subclass, the two dedicated provider factories (ButtplugIoWebsocketDeviceProviderFactory, VirtualDeviceProviderFactory, whose own eventEmitter/eventEmitterFactory fields became dead in turn), the DI wiring in deviceServiceProvider.ts, and the local test provider classes in deviceProvider.spec.ts/deviceProviderManager.spec.ts.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/unit/device/provider/deviceProvider.spec.ts (1)
253-279: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFixed 10ms delay to flush the async offer chain is flaky-prone.
This is the only test relying on
setTimeoutto wait for the announce→offer→addDevice chain to settle, while sibling tests usevi.waitFor. Since it's asserting a negative (onConnectFailedCallsstays 0), consider spying on the created device'sclose()(called internally byaddDevice()for disabled devices) andvi.waitFor-ing on that spy before asserting, for a deterministic wait instead of a magic timeout.♻️ Suggested deterministic wait
- await provider.start(); - deviceManager.announceDetectedDevice({ type: 'test', detectionId: deviceId }); - - // Flush the announce -> offer -> addDevice -> resolve microtask chain before asserting - await new Promise((resolve) => setTimeout(resolve, 10)); + let createdDevice: AnyDevice | undefined; + const provider = new TrackingTestProvider( + deviceManager, + (deviceDetectionInfo) => { + createdDevice = new TestDevice(deviceDetectionInfo.detectionId, 'Foo', new Date(), false, new EventEmitter()); + return Promise.resolve(createdDevice); + } + ); + await provider.start(); + deviceManager.announceDetectedDevice({ type: 'test', detectionId: deviceId }); + + await vi.waitFor(() => expect(createdDevice?.getState).toBeDefined()); + const closeSpy = vi.spyOn(createdDevice!, 'close'); + await vi.waitFor(() => expect(closeSpy).toHaveBeenCalled());🤖 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/unit/device/provider/deviceProvider.spec.ts` around lines 253 - 279, Replace the fixed 10ms timeout in the disabled-device test with a spy on the created TestDevice instance’s close() method, then use vi.waitFor to await that spy being called before asserting the device remains disconnected and onConnectFailedCalls is zero.
🤖 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 252-313: Prevent stale in-flight offers from being registered
after clearDetectedDeviceOfferQueue has revoked or reset the queue. In
runNextInQueue, after await entry.deviceOffer() resolves and before addDevice,
verify that the entry is still current in detectedDeviceOfferQueue for
detectionId; if it was cleared or replaced, do not add the device or emit
connection events, and return while preserving the already-settled caller
result.
In `@src/device/provider/deviceProvider.ts`:
- Around line 103-147: The residual tracking issue is rooted in DeviceManager’s
stale-offer handling, not this provider callback. Update runNextInQueue and
clearDetectedDeviceOfferQueue so an offer that is revoked or reset while
awaiting the callback is rejected after the await, preventing
handleDeviceDetection from treating the stale result as successful and keeping
connectedDevices tracking consistent.
---
Nitpick comments:
In `@tests/unit/device/provider/deviceProvider.spec.ts`:
- Around line 253-279: Replace the fixed 10ms timeout in the disabled-device
test with a spy on the created TestDevice instance’s close() method, then use
vi.waitFor to await that spy being called before asserting the device remains
disconnected and onConnectFailedCalls is zero.
🪄 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: 393f34e5-3e6a-46a0-93ff-c9f59672b2a1
📒 Files selected for processing (17)
src/device/deviceManager.tssrc/device/deviceOfferRejectedError.tssrc/device/protocol/airotic/airoticDeviceProvider.tssrc/device/protocol/buttplugIo/buttplugIoWebsocketDeviceProvider.tssrc/device/protocol/buttplugIo/buttplugIoWebsocketDeviceProviderFactory.tssrc/device/protocol/estim2b/estim2bSerialDeviceProvider.tssrc/device/protocol/slvCtrlPlus/slvCtrlPlusSerialDeviceProvider.tssrc/device/protocol/virtual/virtualDeviceProvider.tssrc/device/protocol/virtual/virtualDeviceProviderFactory.tssrc/device/protocol/zc95/zc95SerialDeviceProvider.tssrc/device/provider/bleDeviceProvider.tssrc/device/provider/deviceProvider.tssrc/device/provider/serialDeviceProvider.tssrc/serviceProvider/deviceServiceProvider.tstests/unit/device/deviceManager.spec.tstests/unit/device/provider/deviceProvider.spec.tstests/unit/device/provider/deviceProviderManager.spec.ts
💤 Files with no reviewable changes (3)
- src/device/protocol/virtual/virtualDeviceProviderFactory.ts
- src/serviceProvider/deviceServiceProvider.ts
- src/device/protocol/buttplugIo/buttplugIoWebsocketDeviceProviderFactory.ts
Addresses CodeRabbit review threads on PR #99: - runNextInQueue(): after a device offer resolves, verify the manager's offer queue for this detection id still points at the same queue array before calling addDevice(). Without this, an offer that settles with a device *after* revokeDetectedDevice()/reset() already cleared the queue and resolved the caller with a rejection would still get registered via addDevice() - connecting a device for hardware that's already known to be gone, with nothing left to close it. If the queue was cleared/replaced, the device is now closed instead and no state is touched. - advanceQueue(): same staleness check, applied more broadly. Without it, a stale failure (undefined/thrown/disabled-rejected) settling after a revoke/reset could shift()/delete() a completely different, currently-legitimate queue for the same detection id (e.g. one created by a fresh re-announce), corrupting unrelated in-progress detections. - Two new deviceManager.spec.ts tests, both confirmed to fail without this fix: one proving a stale offer's device gets closed and never registered, another proving a stale advanceQueue() call doesn't wipe a fresh, still-pending queue for the same detection id.
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
- clearDetectedDeviceOfferQueue()'s parameter and reset()'s loop variable were named deviceId but actually hold a detection id (matching every other use of this concept in the file) - renamed to detectionId to avoid confusion with the canonical device id that deviceId means everywhere else (isDeviceEnabled(), getConnectedDevice()). - Standardized on deviceDetectionInfo for every DeviceDetectionInfo-typed symbol (was inconsistently deviceInfo in some places, deviceDetectionInfo in others): announceDetectedDevice(), revokeDetectedDevice(), addDevice(), the pendingRetries map's value field, applySettingsChange()'s destructuring, and the deviceDetected event's tuple label. - Renamed detectedDeviceOfferQueue to detectedDeviceOfferQueues (plural) - it's a map of one independent queue per detection id, not a single queue. All internal/private renames - no public signature changes, no test changes needed.
- QueueEntry -> PendingDetectedDeviceOffer: mirrors the already-renamed detectedDeviceOfferQueues field it lives in (a map of queues, each holding these entries). - pendingRetries -> detectedDisabledDevices: the old name didn't say what was pending or why; the map holds detections rejected because their known device is currently disabled, parked for re-announcement on re-enable. - closingDevice -> deviceReleased: it's a Promise<void> signaling when the device's underlying resource (serial port, BLE connection, etc.) has been released by the old instance, awaited before re-announcing to avoid a new connection attempt racing the old one's teardown - not literally "a closing device".
…allstack - Merge rejectAndAdvance closure into rejectCurrentOfferInQueueAndAdvance - Rename runNextInQueue/rejectOfferQueueHeadAndAdvance for clarity - Replace undefined-on-failure with throw-on-failure across the whole createDevice/connectSerialDevice/connectBleDevice/deviceOffer callstack, since every concrete provider either always resolves with a device or throws - undefined was never a real signal - Narrow D | undefined to D throughout DeviceProvider, BleDeviceProvider, SerialDeviceProvider and DeviceManager's offerDevice/PendingDetectedDeviceOffer - Update tests accordingly, dropping one test whose scenario became type-impossible
…rQueue - New DetectedDeviceOfferQueue class owns queue storage, enqueue/dequeue, hand-off on failure, and TOCTOU staleness protection - previously mixed into DeviceManager alongside device registry and disabled-device retry concerns - DeviceManager.addDevice() now returns DeviceOfferRejectedError | undefined instead of boolean, injected into the queue as a DeviceOfferAcceptor callback, preserving specific rejection messages - announceDetectedDevice/offerDevice/revokeDetectedDevice/reset become thin delegations to the queue plus their own non-queue concerns (connectedDevices checks, event emission, detectedDisabledDevices) - Move queue race/hand-off/staleness tests into a dedicated detectedDeviceOfferQueue.spec.ts testing the class in isolation with a mock acceptor; deviceManager.spec.ts keeps end-to-end and delegation tests - Update deviceOfferRejectedError.ts's stale doc comment reference to the since-removed undefined return path
- DetectedDeviceOfferQueue no longer takes a DeviceOfferAcceptor callback - it's now a pure race/hand-off/cancellation queue with no opinion on what 'accepted' means. deviceOffer may resolve with either the connected device or a DeviceOfferRejectedError value, checked via instanceof - DeviceManager.offerDevice() wraps the disabled-device check itself in the deviceOffer closure passed to the queue, and on a successful result calls the new registerDevice() (renamed from addDevice, now pure registration with no accept/reject decision-making) - Update detectedDeviceOfferQueue.spec.ts to match: rejection tests now resolve deviceOffer with a DeviceOfferRejectedError directly instead of injecting a mock acceptor
Prevents open() from replacing an already-active queue, which would orphan whatever offer is still running on the old instance. That orphaned offer keeps running to completion on its own and, if it later succeeds, unconditionally cancels whatever queue currently exists for that detection id - the fresh one just created. Add a regression test verifying a pending offer keeps its place in line across a redundant open() call.
…ueue - Critical fix: the success-path check used 'false === cancellationToken.cancelled', but cancellationToken.cancelled is undefined (not false) until actually cancelled, so the check never matched and every device offer was being treated as cancelled and closed right after connecting. Fixed to 'true !== cancellationToken.cancelled' - Extract the task callback passed to queue.push() into its own private runOffer() method, restructured with an early return for the success case instead of nesting the cancellation handling - Extract the deviceOffer callback signature into a named DeviceOffer<D> type
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
src/device/detectedDeviceOfferQueue.ts (2)
65-83: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTruncated comment on Line 68 and a redundant
Promise.resolvewrapper."Reject every other still-queued offer for this detection id without" ends mid-sentence.
task.then(...)already returns a promise, so the surroundingPromise.resolve(...)adds nothing.🤖 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/detectedDeviceOfferQueue.ts` around lines 65 - 83, Complete the truncated comment in the successful branch of the task.then handler so it clearly describes clearing other queued offers for the detection ID. Remove the redundant Promise.resolve wrapper around task.then while preserving the existing success and cancellation-reason handling.
49-52: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
clearReasonsentries are never removed except onopen().
discard()and the post-drain cleanup leave the reason behind, so the map grows for every detection id that is cleared and never reopened. Also, a reason set by a previousclear()can be observed by a stale cancellation for an id that was discarded. Deleting the reason indiscard()(and after the cancelled offers settle) keeps the two maps in sync.Also applies to: 113-123
🤖 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/detectedDeviceOfferQueue.ts` around lines 49 - 52, Update discard() to remove the detectionId from clearReasons alongside queues.delete(), and update the post-drain cleanup path to delete the corresponding clearReasons entry after cancelled offers settle. Keep both maps synchronized so discarded or fully drained detection IDs cannot retain or expose stale clear reasons.tests/unit/device/detectedDeviceOfferQueue.spec.ts (2)
50-54: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFixed 20 ms sleep makes this assertion timing-dependent.
setTimeout(20)is enough today but couples the test to the scheduler. Consider asserting the ordering via a deterministic signal (e.g.await vi.waitFor(() => expect(firstOfferStarted).toBe(true))before checkingsecondOfferFnwas not called).🤖 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/unit/device/detectedDeviceOfferQueue.spec.ts` around lines 50 - 54, Replace the fixed 20 ms setTimeout in the queue-ordering test with a deterministic wait for the first offer’s started signal, such as waiting until firstOfferStarted is true before asserting secondOfferFn has not been called. Preserve the existing assertion that the second offer remains blocked while the first offer is unresolved.
213-262: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing case:
clear()→open()where the old queue drains afterwards.This suite covers a stale offer failing after a clear, but not the
drainedlistener from the cancelled queue firing after the id was reopened — the path flagged insrc/device/detectedDeviceOfferQueue.tsLines 34-47. Adding that case would pin the identity guard. Want me to draft it?🤖 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/unit/device/detectedDeviceOfferQueue.spec.ts` around lines 213 - 262, The test suite needs coverage for a cancelled queue’s delayed drained event after the same detection id is reopened. Add a test alongside the existing stale-offer case that starts an offer, calls clear(), reopens the id, queues a fresh pending offer, then triggers or awaits the old queue’s drained listener and verifies the fresh queue and its pending offer remain intact, confirming the identity guard in DetectedDeviceOfferQueue.src/device/deviceManager.ts (1)
167-179: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueRe-announcing inside the iteration can be re-entered via the same map.
announceDetectedDevice()→ provider offer → disabled path can insert intodetectedDisabledDeviceswhile thisfor..ofis still running, and Map iteration picks up entries added during iteration. It's bounded in practice (the re-added entry is still disabled and skipped), but snapshotting with[...this.detectedDisabledDevices]makes the pass unambiguous.🤖 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 167 - 179, Snapshot detectedDisabledDevices before iterating in the re-announcement logic, using a copied entry list for the loop instead of iterating the Map directly. Keep the existing enabled-device filtering, deletion, release wait, and announceDetectedDevice flow unchanged.
🤖 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/detectedDeviceOfferQueue.ts`:
- Around line 34-47: Update the drained listener in
DetectedDeviceOfferQueue.open so it deletes detectionId only when this.queues
still maps that ID to the same queue instance that emitted the event; leave
newer queues installed by clear/open untouched.
In `@src/device/deviceManager.ts`:
- Around line 87-96: Update the announceDetectedDevice flow around
eventEmitter.emit and offerQueue.open so a subscribed provider that declines via
canHandleDeviceDetectionInfo does not leave an empty queue open. Track whether
any provider actually submits an offer, or discard the queue when the detection
receives no offer within the existing queue lifecycle, while preserving the
current discard behavior when no providers are subscribed.
In `@src/device/provider/serialDeviceProvider.ts`:
- Around line 69-80: Guard cleanup failures in both provider error paths: in
src/device/provider/serialDeviceProvider.ts lines 69-80, wrap the port.close()
await in try/catch, log any close failure, then rethrow the original e; in
src/device/protocol/airotic/airoticDeviceProvider.ts lines 47-50, similarly
guard transport.close() so the original Handshake failed error remains the
propagated failure.
---
Nitpick comments:
In `@src/device/detectedDeviceOfferQueue.ts`:
- Around line 65-83: Complete the truncated comment in the successful branch of
the task.then handler so it clearly describes clearing other queued offers for
the detection ID. Remove the redundant Promise.resolve wrapper around task.then
while preserving the existing success and cancellation-reason handling.
- Around line 49-52: Update discard() to remove the detectionId from
clearReasons alongside queues.delete(), and update the post-drain cleanup path
to delete the corresponding clearReasons entry after cancelled offers settle.
Keep both maps synchronized so discarded or fully drained detection IDs cannot
retain or expose stale clear reasons.
In `@src/device/deviceManager.ts`:
- Around line 167-179: Snapshot detectedDisabledDevices before iterating in the
re-announcement logic, using a copied entry list for the loop instead of
iterating the Map directly. Keep the existing enabled-device filtering,
deletion, release wait, and announceDetectedDevice flow unchanged.
In `@tests/unit/device/detectedDeviceOfferQueue.spec.ts`:
- Around line 50-54: Replace the fixed 20 ms setTimeout in the queue-ordering
test with a deterministic wait for the first offer’s started signal, such as
waiting until firstOfferStarted is true before asserting secondOfferFn has not
been called. Preserve the existing assertion that the second offer remains
blocked while the first offer is unresolved.
- Around line 213-262: The test suite needs coverage for a cancelled queue’s
delayed drained event after the same detection id is reopened. Add a test
alongside the existing stale-offer case that starts an offer, calls clear(),
reopens the id, queues a fresh pending offer, then triggers or awaits the old
queue’s drained listener and verifies the fresh queue and its pending offer
remain intact, confirming the identity guard in DetectedDeviceOfferQueue.
🪄 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: b63eb7cf-6d5a-4c17-b5bc-90d878719569
📒 Files selected for processing (16)
src/device/detectedDeviceOfferQueue.tssrc/device/deviceManager.tssrc/device/deviceOfferRejectedError.tssrc/device/protocol/airotic/airoticDeviceProvider.tssrc/device/protocol/buttplugIo/buttplugIoWebsocketDeviceProvider.tssrc/device/protocol/estim2b/estim2bSerialDeviceProvider.tssrc/device/protocol/slvCtrlPlus/slvCtrlPlusSerialDeviceProvider.tssrc/device/protocol/virtual/virtualDeviceProvider.tssrc/device/protocol/zc95/zc95SerialDeviceProvider.tssrc/device/provider/bleDeviceProvider.tssrc/device/provider/deviceProvider.tssrc/device/provider/serialDeviceProvider.tstests/unit/device/detectedDeviceOfferQueue.spec.tstests/unit/device/deviceManager.spec.tstests/unit/device/provider/deviceProvider.spec.tstests/unit/device/provider/deviceProviderManager.spec.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- src/device/deviceOfferRejectedError.ts
- tests/unit/device/provider/deviceProviderManager.spec.ts
- src/device/protocol/virtual/virtualDeviceProvider.ts
- src/device/protocol/zc95/zc95SerialDeviceProvider.ts
- src/device/protocol/slvCtrlPlus/slvCtrlPlusSerialDeviceProvider.ts
- src/device/protocol/buttplugIo/buttplugIoWebsocketDeviceProvider.ts
- src/device/provider/deviceProvider.ts
- tests/unit/device/provider/deviceProvider.spec.ts
…ection() Pulls the device-offer closure (create device, check isStopped(), wire disconnect listener, register in connectedDevices) into its own named private method, mirroring the same extraction done in DetectedDeviceOfferQueue.runOffer(). connectedDevices.set() and the isStopped() check stay together in their original order - moving connectedDevices bookkeeping later would race both the deviceConnected event (listeners could see it before the provider's own bookkeeping is updated) and stop()'s device-closing loop (a device could be registered with DeviceManager but never make it into connectedDevices in time to be closed on shutdown).
- detectedDeviceOfferQueue.ts: guard the drained listener in open() with an identity check before deleting from queues, so it can't wipe a fresh queue installed by a later clear()+open() cycle. Confirmed via source-reading and empirical testing against the installed sequential-task-queue version that this isn't currently reachable (cancellation always settles synchronously within cancel()'s own call stack), but the guard is cheap and matches the same defensive pattern used elsewhere in this class - serialDeviceProvider.ts / airoticDeviceProvider.ts: wrap cleanup awaits (port.close() / transport.close()) in their own try/catch in the failure paths, so a failing cleanup no longer silently replaces the original, more meaningful connection/handshake error - matches the log-and-swallow convention used everywhere else close() is called in this codebase
DeviceManager.offerDevice()'s wrapped closure made its enablement decision (and detectedDisabledDevices side effect) purely based on the raw connect result, with no way to know the offer had already been revoked/reset while connecting was still in flight. A slow connect to a disabled device's canonical id that finished after a revoke would still get parked in detectedDisabledDevices, undoing revokeDetectedDevice()'s own cleanup and causing a spurious re-announce attempt for hardware already known to be gone if later re-enabled. Thread the queue's own CancellationToken into the DeviceOffer callback (DetectedDeviceOfferQueue.runOffer() now passes it through) so the closure can check it before making any enablement decision, closing the device and returning a DeviceOfferRejectedError directly instead. Add a regression test verified to fail without the fix (spurious re-announce after re-enabling) and pass with it.
- detectedDeviceOfferQueue.ts: add hadOffers(detectionId), backed by an offeredQueues map storing the exact queue instance an offer was made against (not just a bare Set<string>), so a stale record from a superseded queue generation can never be mistaken for a fresh one - deviceManager.ts: announceDetectedDevice() now checks hadOffers() synchronously right after emit() returns - if listeners exist but none of them actually called offerDevice() (e.g. no subscribed provider's canHandleDeviceDetectionInfo() matched), discard the queue instead of leaving it open forever and permanently blocking future announces for that detection id. Distinct log message from the existing !hadListeners case - deviceManager.spec.ts: add a reactToDetection() helper that wires a mocked EventEmitter to synchronously call offerDevice() on deviceDetected, matching how a real DeviceProvider actually reacts; rework the connectDevice helper and several tests to use it instead of calling offerDevice() as a separate, disconnected statement after announceDetectedDevice() - several of these were previously passing for the wrong reason (accidentally short-circuiting through paths they weren't meant to exercise) No queueMicrotask deferral needed - traced the full real call chain and confirmed every DeviceProvider always calls offerDevice() synchronously within emit()'s own call stack, so the synchronous check is correct for production; an earlier microtask-based attempt was reverted as unnecessary test-shaped complexity leaking into production code
Switch from sequential-task-queue to the @TiMESPLiNTER fork, which propagates custom cancellation reasons natively via cancellationToken.reason - removing the clearReasons workaround map this queue previously needed. Make offer() open its own queue lazily (getOrCreateQueue()) instead of requiring announceDetectedDevice() to proactively open/discard one and track hadOffers() via a second map. A detection that nobody recognizes no longer creates any queue at all, so there is nothing to discard. Add revoke()/dropIfRevoked(): a revoked (physically disappeared) detection stays closed as a tombstone instead of being deleted, so a late offer arriving after the revoke sees it and rejects itself rather than reconnecting a device that's already gone. Fix the drained-event handler to not delete a closed queue on its own drain, since revoke's close(true, reason) triggers that same drain. Add a connectedDevices check to offerDevice() so a late offer against an already-claimed device is rejected immediately, and restore the has()-based reentrancy guard in announceDetectedDevice() to avoid re-emitting deviceDetected while a round is still in flight. Update detectedDeviceOfferQueue.spec.ts for the new lazy-open API and add coverage for revoke/dropIfRevoked. Fix two deviceManager.spec.ts tests whose assertions encoded the old proactive-open behavior.
DeviceOfferRejectedError messages previously repeated the detection id even though the only place that reads them (DeviceProvider's rejection log) already includes it in the surrounding text, and ran through BaseError.normalize() which wraps a plain Error into a "Error: <message>" string - producing lines like: Device offer for '<id>' was rejected: Error: Device '<id>' has been claimed by another provider Drop the redundant id from each message so it reads as a clause after the log line's own id, and read DeviceOfferRejectedError's .message directly instead of normalizing it (still normalized for genuine thrown failures, which aren't ours to pre-format). Reworded the log line itself to "Offer for device detection with id '<id>' ..." to match the actual DeviceDetectionInfo/detectionId terminology used throughout this code.
heavyrubberslave
marked this pull request as ready for review
August 1, 2026 12:09
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/device/deviceManager.ts (1)
74-96: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winCritical:
dropIfRevoked()is unreachable, so a revoked device can never be redetected.
announceDetectedDevice()checksthis.offerQueue.has(deviceDetectionInfo.detectionId)at Line 76 and returns early if true, before ever callingthis.offerQueue.dropIfRevoked(...)at Line 85.DetectedDeviceOfferQueue.has()returnstruefor both active queues and closed tombstones left behind byrevoke()(seesrc/device/detectedDeviceOfferQueue.tshas()), since it only checks map presence, notisClosed.Once
revokeDetectedDevice()runs (for example when a serial device physically disappears), the tombstone stays inthis.queuesforever. Every subsequentannounceDetectedDevice()call for that samedetectionIdhits the Line 76 check first, returns immediately, and never reachesdropIfRevoked()at Line 85.dropIfRevoked()can only remove a closed entry, but that entry is exactly the one the earlierhas()check has already used to bail out. The device can never be re-announced again through the normal detection flow, even after it physically reappears — only a fullreset()(viaclearAll()) clears it.This breaks the reconnection story the tombstone/
dropIfRevoked()mechanism was built for. Move thedropIfRevoked()call before thehas()gate so a closed tombstone is reconciled before it can block a genuine redetection.🐛 Proposed fix: reconcile revoked tombstones before gating on `has()`
public announceDetectedDevice(deviceDetectionInfo: DeviceDetectionInfo): void { + this.offerQueue.dropIfRevoked(deviceDetectionInfo.detectionId); + if (this.offerQueue.has(deviceDetectionInfo.detectionId)) { return; } if (this.connectedDevices.has(deviceDetectionInfo.detectionId)) { this.logger.debug(`Device with id '${deviceDetectionInfo.detectionId}' is already connected, not announcing it as detected`); return; } - this.offerQueue.dropIfRevoked(deviceDetectionInfo.detectionId); - this.logger.info(`Detected new device with id ${deviceDetectionInfo.detectionId}`);None of the tests in
tests/unit/device/deviceManager.spec.tscurrently callrevokeDetectedDevice()followed by a secondannounceDetectedDevice()for the samedeviceInfo, so this regression is not caught by the current suite. Consider adding that regression test alongside the fix.Do you want me to add this fix and a matching regression test, or open a new issue to track this?
🤖 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 74 - 96, Move the this.offerQueue.dropIfRevoked(deviceDetectionInfo.detectionId) call in announceDetectedDevice() before the offerQueue.has() early-return check, so closed tombstones are removed before gating redetection. Preserve the existing handling for active offers and connected devices, and add a regression test covering revokeDetectedDevice() followed by announcing the same device again.
🧹 Nitpick comments (3)
src/device/detectedDeviceOfferQueue.ts (2)
106-118: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument that
has()returns true for closed (tombstoned) queues.
has()returnsthis.queues.has(detectionId)with no distinction between an active queue and a closed tombstone left byrevoke(). This dual meaning is not obvious from the method name or signature, and a caller that only wants to know "is a live offer flow in progress" can easily misuse it as a blocking gate for a revoked detection id (see the companion comment onsrc/device/deviceManager.tsannounceDetectedDevice(), which does exactly that).Add a short doc comment on
has()clarifying it returns true for both active and closed/tombstoned entries, and that a caller wanting to know whether a fresh announcement is still blocked by revocation must calldropIfRevoked()first.🤖 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/detectedDeviceOfferQueue.ts` around lines 106 - 118, Add a concise doc comment to OfferQueue.has explaining that it returns true for both active and closed/tombstoned queues, and that callers checking whether a fresh announcement remains blocked by revocation must call dropIfRevoked first.
61-77: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFix the comment on the rejection branch; it understates when this code runs.
The comment states this branch is "only reached if the offer was cancelled while still queued, never even starting." In fact,
task.then(onFulfilled, onRejected)'sonRejectedbranch also runs wheneverdeviceOffer(cancellationToken)itself rejects or throws (see therunOffercall chain), not only on cancellation before start. The "hands off to the next queued offer when the first one throws" test intests/unit/device/detectedDeviceOfferQueue.spec.ts(Lines 63-76) exercises exactly this path through a genuine rejection, not a queued-cancellation.Update the comment to state that this branch handles both queued-cancellation and any unhandled
deviceOfferrejection, so future maintainers reasoning about this concurrency-sensitive code do not narrow the failure surface incorrectly.🤖 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/detectedDeviceOfferQueue.ts` around lines 61 - 77, Update the rejection-branch comment in the task.then call within runOffer to state that it handles both cancellation while queued and any rejection or thrown error from deviceOffer(cancellationToken). Preserve the existing behavior and clarify that the branch is not limited to pre-start cancellation.tests/unit/device/deviceManager.spec.ts (1)
203-231: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a regression test for redetection after
revokeDetectedDevice().This suite covers "no listeners" and "listeners but no offer" cases for re-announcing, but there is no test that calls
manager.revokeDetectedDevice(deviceInfo)and thenmanager.announceDetectedDevice(deviceInfo)again to confirm the device can be redetected. As described in the companion comment onsrc/device/deviceManager.tsannounceDetectedDevice()(Lines 74-96), the current check order makesdropIfRevoked()unreachable, so a revoked device can never be re-announced. A test in this style would have caught that:it('allows re-announcing after a device is revoked and reappears', async () => { const manager = new DeviceManager(mockedEventEmitter, new Map(), mockedSettingsManager, mockedLogger); manager.revokeDetectedDevice(deviceInfo); mockClear(mockedEventEmitter); mockedEventEmitter.emit.mockReturnValue(true); manager.announceDetectedDevice(deviceInfo); expect(mockedEventEmitter.emit).toHaveBeenCalledWith(DeviceManagerEvent.deviceDetected, deviceInfo); });Do you want me to add this test alongside the fix for the underlying issue?
🤖 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/unit/device/deviceManager.spec.ts` around lines 203 - 231, Add a regression test alongside the existing re-announcement tests that calls DeviceManager.revokeDetectedDevice(deviceInfo), clears the emitter mock, announces the same device again, and verifies deviceDetected is emitted with deviceInfo. This should confirm revoked devices can be detected again.
🤖 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/airotic/airoticDeviceProvider.ts`:
- Around line 49-54: Bound the cleanup await in connectBleDevice after a failed
handshake so transport.close cannot stall indefinitely. Add a timeout around
BleUartDeviceTransport.close, or update its close implementation to use the
existing promiseWithTimeout/disconnectAsync fallback, while preserving error
logging and rethrowing the handshake failure.
---
Outside diff comments:
In `@src/device/deviceManager.ts`:
- Around line 74-96: Move the
this.offerQueue.dropIfRevoked(deviceDetectionInfo.detectionId) call in
announceDetectedDevice() before the offerQueue.has() early-return check, so
closed tombstones are removed before gating redetection. Preserve the existing
handling for active offers and connected devices, and add a regression test
covering revokeDetectedDevice() followed by announcing the same device again.
---
Nitpick comments:
In `@src/device/detectedDeviceOfferQueue.ts`:
- Around line 106-118: Add a concise doc comment to OfferQueue.has explaining
that it returns true for both active and closed/tombstoned queues, and that
callers checking whether a fresh announcement remains blocked by revocation must
call dropIfRevoked first.
- Around line 61-77: Update the rejection-branch comment in the task.then call
within runOffer to state that it handles both cancellation while queued and any
rejection or thrown error from deviceOffer(cancellationToken). Preserve the
existing behavior and clarify that the branch is not limited to pre-start
cancellation.
In `@tests/unit/device/deviceManager.spec.ts`:
- Around line 203-231: Add a regression test alongside the existing
re-announcement tests that calls DeviceManager.revokeDetectedDevice(deviceInfo),
clears the emitter mock, announces the same device again, and verifies
deviceDetected is emitted with deviceInfo. This should confirm revoked devices
can be detected again.
🪄 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: e603c6e2-d130-4617-9fce-da36386cb488
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (11)
package.jsonsrc/device/detectedDeviceOfferQueue.tssrc/device/deviceManager.tssrc/device/protocol/airotic/airoticDeviceProvider.tssrc/device/provider/deviceProvider.tssrc/device/provider/deviceProviderManager.tssrc/device/provider/serialDeviceProvider.tssrc/device/updater/bufferedDeviceUpdater.tssrc/serial/synchronousSerialPort.tstests/unit/device/detectedDeviceOfferQueue.spec.tstests/unit/device/deviceManager.spec.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/device/provider/serialDeviceProvider.ts
- src/device/provider/deviceProvider.ts
announceDetectedDevice() checked offerQueue.has() before calling dropIfRevoked(), but has() can't distinguish a genuinely in-flight queue from a closed tombstone left by revoke() - both just look like "an entry exists" to it. So once a detection id was ever revoked, has() would return true forever and short-circuit before dropIfRevoked() got a chance to clear the stale tombstone, permanently blocking any future announce for that id. This was invisible to unit tests but broke every integration test that revokes and later reuses the same detection id (which the beforeEach hooks in tests/integration/devices/*.spec.ts do on every run, since SerialPortObserver's managedDevices map isn't reset by DeviceManager.reset() and revokes the previous test's port on the next discoverSerialDevices() call). Fix: run dropIfRevoked() first, before the has() guard, so a stale tombstone is cleared before it's ever consulted. Added a unit test that reproduces this exact sequence (announce, revoke, re-announce) and fails without the fix.
…it nitpicks
- close() (renamed from clear(), private), revoke(), and closeAll()
(via close()) now all consistently use queue.close(true, reason) as
the one "shut this queue down" primitive, instead of the old clear()
using the weaker cancel(reason). The only difference between them is
what happens to the map entry afterward: close() deletes it,
revoke() keeps it as a tombstone, closeAll() deletes all of them.
No behavior change for close()'s caller (offer()'s success path) -
runOffer() only ever reads cancellationToken.cancelled/.reason,
never queue.isClosed, and close() already calls cancel() internally
with identical mechanics.
- Dropped the this.queues.get(detectionId) === queue identity check
in the drained handler, keeping only !queue.isClosed. Traced the
scenario it was meant to guard against (a cancelled, still-running
task whose underlying deviceOffer() promise settles much later) and
found the sequential-task-queue library already prevents it via its
own `if (this.currentTask !== task) return;` guard in doneTask() -
a task cancelled while running immediately clears currentTask, so
its later, real settlement can never re-reach the emit('drained')
line a second time. Verified by running the regression test written
specifically for this scenario ("does not corrupt a fresh,
still-pending queue when a stale offer fails after a clear") and the
full suite 3x - all pass. The !queue.isClosed half stays, since
that's what makes revoke()'s tombstone survive its own drain (the
bug fixed in bc9414f). The drained handler as a whole stays too -
it's the only thing that cleans up a queue that completes naturally
(e.g. a single offer that fails on its own, never reaching
offer()'s close() call at all); without it that entry would block
the detectionId from ever being offered again.
- Made the old clear() private (deviceManager.ts never called it
directly anymore since revoke() took over the "device disappeared"
case) and renamed it to close() to match what it actually does now.
closeAll() (renamed from clearAll) goes back to looping detectionIds
and delegating to close() per entry, now that close() itself uses
the close() primitive.
- Fixed truncated/inaccurate comments in offer()'s .then() handlers,
added a doc comment to has() clarifying it can't distinguish a
genuinely active queue from a closed revoke() tombstone (the exact
ambiguity behind the dropIfRevoked()-ordering bug in bc9414f),
snapshotted detectedDisabledDevices before iterating in
applySettingsChange() (announceDetectedDevice() calls inside that
loop could otherwise append to the same map mid-iteration), and
replaced a fixed setTimeout(10) in deviceProvider.spec.ts's
disabled-device test with a deterministic wait on the device's
close() spy.
Test file updated: direct clear() calls now go through closeAll()
(functionally identical when only one queue is in play), folded into
a renamed describe('closeAll', ...) block. Kept two of those tests
intact rather than treating them as redundant with the "claimed by
another provider" test - they exercise the cancel-a-running-task path
directly, which that other test's queued-but-never-started offer
can't reach (the queue's own scheduler uses setImmediate, a macrotask,
while our .then() continuation runs as a microtask that always drains
first, so a queued sibling can only ever be cancelled before it starts).
Left the Promise.resolve(task.then(...)) wrapper in offer() as-is -
task.then() returns a PromiseLike, not a full Promise (per the fork's
CancellablePromiseLike<T> extends PromiseLike<T>), so the wrapper is
required to satisfy offer()'s Promise<OfferResult<D>> return type, not
redundant as originally flagged by CodeRabbit.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/detectedDeviceOfferQueue.ts`:
- Around line 139-144: Update revoke() and the queue retention logic around
getOrCreateQueue, close, and dropIfRevoked to apply a bounded cleanup policy for
closed revocation tombstones. Retain tombstones long enough to preserve
rejection of late offers and active acquisition flows, then remove expired or
otherwise inactive closed queues so queues cannot grow without bound; leave
active queues and same-ID late-offer rejection behavior unchanged.
In `@src/device/deviceManager.ts`:
- Line 241: Update reset() to call this.offerQueue.closeAll(new
DeviceOfferRejectedError('Device manager reset')) before the first await in the
connected-device closure loop, ensuring pending offers are cancelled before any
device closure can complete and register a new device.
🪄 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: 400b8e0f-72d7-44a2-abae-7772ebc225a4
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (13)
package.jsonsrc/device/detectedDeviceOfferQueue.tssrc/device/deviceManager.tssrc/device/deviceOfferRejectedError.tssrc/device/protocol/airotic/airoticDeviceProvider.tssrc/device/provider/deviceProvider.tssrc/device/provider/deviceProviderManager.tssrc/device/provider/serialDeviceProvider.tssrc/device/updater/bufferedDeviceUpdater.tssrc/serial/synchronousSerialPort.tstests/unit/device/detectedDeviceOfferQueue.spec.tstests/unit/device/deviceManager.spec.tstests/unit/device/provider/deviceProvider.spec.ts
💤 Files with no reviewable changes (1)
- src/device/deviceOfferRejectedError.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- src/serial/synchronousSerialPort.ts
- src/device/updater/bufferedDeviceUpdater.ts
- package.json
- src/device/provider/deviceProviderManager.ts
- src/device/provider/serialDeviceProvider.ts
- tests/unit/device/detectedDeviceOfferQueue.spec.ts
- tests/unit/device/provider/deviceProvider.spec.ts
- src/device/provider/deviceProvider.ts
- tests/unit/device/deviceManager.spec.ts
reset() closed all already-connected devices first, then cancelled in-flight offer-queue entries afterward - leaving a window where a not-yet-connected offer could still resolve successfully and get registered via registerDevice() while the close loop was still busy awaiting other devices, landing a new connected device mid-reset. closeAll() itself is fully synchronous and doesn't touch connectedDevices at all, so moving it to the first line of reset() cancels every pending offer before the function ever yields to the event loop, closing that window down to effectively zero instead of "however long the close loop takes".
heavyrubberslave
added a commit
that referenced
this pull request
Aug 2, 2026
… 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
added a commit
that referenced
this pull request
Aug 2, 2026
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.
heavyrubberslave
added a commit
that referenced
this pull request
Aug 2, 2026
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.
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