feat(hardware): add DacMonitor with gesture and device state sync#116
feat(hardware): add DacMonitor with gesture and device state sync#116
Conversation
Introduces a persistent hardware polling loop that detects tap gestures and keeps device_state in sync with the hardware daemon. - DacMonitor: polls DEVICE_STATUS every 2 s, diffs tap counters to emit gesture:detected events; handles firmware-restart counter resets via silent re-baselining; isPollInFlight guard prevents concurrent polls - GestureActionHandler: executes DB-configured actions on gesture events (temperature adjustment, alarm snooze/dismiss, power toggle); snooze restarts are tracked and cancelled on shutdown via cleanup() - DeviceStateSync: upserts both sides of device_state on every status:updated event; isPowered derived from currentLevel != 0 - dacMonitor.instance: single-flight singleton wiring all three together; instance registered before start() to avoid duplicate creation on concurrent callers or failed starts - instrumentation: non-blocking DacMonitor init; await shutdown before DB close so snooze timers and listeners are cleaned up - health router: dacMonitor procedure exposes status, podVersion, gesturesSupported - 27 new tests across dacMonitor and gestureActionHandler suites
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughIntroduces comprehensive hardware monitoring and gesture detection system: DacMonitor class polls hardware daemon and emits lifecycle/status/gesture events; GestureActionHandler processes detected gestures to execute hardware actions; DeviceStateSync persists device state to database; integrates lifecycle management into server instrumentation with health endpoint. Changes
Sequence Diagram(s)sequenceDiagram
participant Server as Server Instance
participant Instr as Instrumentation
participant MonInst as DacMonitor Singleton
participant Monitor as DacMonitor
participant HClient as HardwareClient
participant Daemon as Hardware Daemon
Server->>Instr: Start server
Instr->>Instr: Initialize scheduler
Instr->>MonInst: getDacMonitor()
activate MonInst
MonInst->>Monitor: new DacMonitor(config)
MonInst->>Monitor: start()
activate Monitor
Monitor->>HClient: new HardwareClient()
Monitor->>HClient: connect()
HClient->>Daemon: Connect to socket
Daemon-->>HClient: Connected
Monitor->>Monitor: Begin polling at interval
Monitor-->>MonInst: Promise resolves
MonInst-->>Instr: Resolve shared instance
deactivate MonInst
loop Poll Interval
Monitor->>HClient: getStatus()
HClient->>Daemon: Query device status
Daemon-->>HClient: Return DeviceStatus
Monitor->>Monitor: Detect gestures from counters
Monitor->>Monitor: Emit status:updated
Monitor->>Monitor: Emit gesture:detected (if new)
end
Server->>Instr: Graceful shutdown
Instr->>MonInst: shutdownDacMonitor()
MonInst->>Monitor: stop()
activate Monitor
Monitor->>Monitor: Clear polling interval
Monitor->>HClient: disconnect()
HClient->>Daemon: Close connection
Monitor-->>MonInst: Resolved
deactivate Monitor
MonInst-->>Instr: Shutdown complete
Instr->>Instr: Close database
Instr->>Server: Shutdown complete
sequenceDiagram
participant Monitor as DacMonitor
participant Handler as GestureActionHandler
participant Deps as Gesture Deps
participant DB as Database
participant HClient as HardwareClient
participant Daemon as Hardware Daemon
Monitor->>Monitor: Poll detects gesture increment
Monitor->>Handler: Emit gesture:detected event
activate Handler
Handler->>Handler: handle(GestureEvent)
Handler->>Deps: findGestureConfig(side, tapType)
Deps->>DB: Query tapGestures row
DB-->>Deps: Return TapGestureRow
Deps-->>Handler: Config with action metadata
alt Temperature Action
Handler->>Deps: findDeviceState(side)
Deps->>DB: Query device state
DB-->>Deps: Return current targetTemperature
Handler->>Handler: Apply delta, clamp to bounds
Handler->>HClient: new HardwareClient()
Handler->>HClient: connect()
HClient->>Daemon: setTemperature(newTemp)
Daemon-->>HClient: Acknowledged
else Alarm Action
Handler->>Deps: findDeviceState(side)
Deps->>DB: Query isAlarmVibrating
alt Vibrating
alt Snooze
Handler->>HClient: clearAlarm()
Handler->>Handler: Schedule restart after duration
Handler->>HClient: setAlarm(pattern, duration)
else Dismiss
Handler->>HClient: clearAlarm()
end
else Not Vibrating
alt Power Toggle
Handler->>HClient: setPower(!isPowered)
end
end
end
deactivate Handler
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (4)
src/hardware/deviceStateSync.ts (2)
33-52: Twonew Date()calls produce slightly different timestamps.
new Date()is called once in.values()(line 41) and again in.onConflictDoUpdate()(line 50). On an insert-vs-update code path they'll differ by microseconds. Hoist a single timestamp for consistency:Suggested fix
private upsertSide = async (side: 'left' | 'right', status: DeviceStatus): Promise<void> => { const sideStatus = side === 'left' ? status.leftSide : status.rightSide + const now = new Date() await db .insert(deviceState) .values({ side, currentTemperature: sideStatus.currentTemperature, targetTemperature: sideStatus.targetTemperature, isPowered: sideStatus.currentLevel !== 0, waterLevel: status.waterLevel, - lastUpdated: new Date(), + lastUpdated: now, }) .onConflictDoUpdate({ target: deviceState.side, set: { currentTemperature: sideStatus.currentTemperature, targetTemperature: sideStatus.targetTemperature, isPowered: sideStatus.currentLevel !== 0, waterLevel: status.waterLevel, - lastUpdated: new Date(), + lastUpdated: now, }, }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hardware/deviceStateSync.ts` around lines 33 - 52, The code uses two separate new Date() calls for lastUpdated in the db.insert(values) and .onConflictDoUpdate(set) paths causing slightly different timestamps; fix this by creating a single timestamp variable (e.g., const now = new Date()) immediately before calling await db.insert(...) and replace both occurrences of new Date() in values and in onConflictDoUpdate.set with that now variable so both insert and update use the exact same timestamp; keep identifiers deviceState, db.insert(...).values(...), and .onConflictDoUpdate(...) unchanged.
9-23: Error handling silently swallows write failures.
sync()catches all errors and logs them but never re-throws. The caller indacMonitor.instance.ts:37already has its own.catch()wrapper, so this double-swallowing means a persistent DB failure (e.g., disk full, schema mismatch) will only produce console logs with no alerting or back-pressure. This is acceptable for now given the "sync only" contract, but consider emitting a metric or incrementing a counter for operational visibility if you have observability infrastructure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hardware/deviceStateSync.ts` around lines 9 - 23, DeviceStateSync.sync currently catches errors from await Promise.all([...this.upsertSide('left', status), this.upsertSide('right', status)]) and only logs them, which hides failures from upstream callers; update DeviceStateSync.sync to, after logging, either (a) re-throw the error so the caller's .catch can observe it, or (b) increment/emit an observability counter (e.g., metrics.increment('device_state_sync_fail')) and then re-throw — ensure the change is applied inside the catch block that surrounds the Promise.all call and keep references to the existing upsertSide('left'| 'right', status) calls and the sync method signature.src/hardware/tests/dacMonitor.test.ts (1)
148-163:updatesBeforeis always 0 — redundant variable.
updatesBeforeon line 156 is hardcoded to0and theupdatesarray is only created after this assignment, so the final assertion (updates.length === updatesBefore) is equivalent toupdates.length === 0. Consider simplifying:- const updatesBefore = 0 const updates: unknown[] = [] monitor.on('status:updated', s => updates.push(s)) await sleep(POLL_MS * 3) // No more polls after stop - expect(updates.length).toBe(updatesBefore) + expect(updates).toHaveLength(0)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hardware/tests/dacMonitor.test.ts` around lines 148 - 163, The test creates a redundant hardcoded updatesBefore variable (set to 0) and constructs the updates array after that, making the final assertion trivial; remove the updatesBefore variable and simplify the assertion by asserting directly that updates.length is 0 (after creating updates and subscribing to 'status:updated' on the monitor created via createMonitor(), which you start then stop via monitor.start() / monitor.stop()) so the test verifies no further 'status:updated' events are received.src/hardware/gestureActionHandler.ts (1)
126-142: Snooze restart uses hardcoded alarm parameters.The vibration intensity (50), pattern (
'rise'), and duration (180 s) are baked in. If the original alarm had different settings, the restarted alarm won't match. Consider sourcing these from the original alarm config or the gesture row so the snooze feels consistent to the user.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hardware/gestureActionHandler.ts` around lines 126 - 142, The snooze restart currently uses hardcoded alarm parameters (vibrationIntensity: 50, vibrationPattern: 'rise', duration: 180) inside the setTimeout in GestureActionHandler; change it to capture the original alarm settings from the gesture or event (e.g., gesture.alarmVibrationIntensity, gesture.alarmVibrationPattern, gesture.alarmDuration or the event.alarm config) before calling client.clearAlarm, then pass those captured values into restartClient.setAlarm so the restarted alarm matches the original; keep sensible fallbacks (existing gesture.alarmSnoozeDuration) and continue adding the timeoutId to this.snoozeTimeouts and cleaning it up as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@instrumentation.ts`:
- Around line 54-60: The shutdown can race with an in-flight
initializeDacMonitor() because shutdownDacMonitor() may return early if
monitorInstance is null; update shutdownDacMonitor in dacMonitor.instance.ts to
first await the shared monitorInitPromise (the promise set by
initializeDacMonitor) before checking/returning so any concurrent initialization
completes and sets monitorInstance, then proceed to stop the monitorInstance
normally; reference the existing symbols monitorInitPromise,
initializeDacMonitor(), getDacMonitor(), shutdownDacMonitor(), and
monitorInstance when making the change.
In `@src/hardware/dacMonitor.instance.ts`:
- Around line 29-55: The singleton is assigned before DacMonitor.start() so a
thrown start() leaves monitorInstance (and gestureHandlerInstance) pointing to a
non-started object; change the init flow so you either move the assignments of
monitorInstance and gestureHandlerInstance until after await monitor.start(), or
add a catch that clears monitorInstance and gestureHandlerInstance if start()
throws (ensure monitorInitPromise is still reset in the finally); update code
around monitorInitPromise, monitorInstance, gestureHandlerInstance and the
start() call in the async initializer used by getDacMonitor() /
DacMonitor.start() accordingly.
In `@src/hardware/dacMonitor.ts`:
- Around line 153-158: The catch block uses monitorStatus to decide whether to
emit 'connection:lost', which prevents emitting on the first failed poll after
an initial failed connect; add a dedicated boolean flag (e.g.,
connectionLostEmitted) on the class, set it to false when a successful
connect/reconnect occurs (inside the connect() success path), and in the catch
handler check connectionLostEmitted instead of monitorStatus to decide to emit
'connection:lost' once, then set connectionLostEmitted = true; keep emitting
'error' as before. This preserves the one-time "lost" emission contract while
still using monitorStatus for degraded state.
- Around line 172-195: The loop over TAP_TYPES currently only emits a single
gesture when currentCounts.l or .r > lastCounts, which drops extra gestures;
modify the logic in dacMonitor.ts (inside the for loop that reads currentCounts
and lastCounts and uses this.lastGestures / extractGestureCounters) to compute
the delta (e.g., deltaL = currentCounts.l - lastCounts.l, deltaR =
currentCounts.r - lastCounts.r) and for each positive delta either emit multiple
'gesture:detected' events (one per count) or emit a single event that includes
the delta in its payload (e.g., { side: 'left'|'right', tapType, count: delta,
timestamp }), ensuring both left and right sides are handled and you still
re-baseline on counter reset as before.
- Around line 111-160: The poll can still emit after stop() completes; update
stop() and poll() so in-flight poll results are discarded when shutdown occurs:
in stop() set this.monitorStatus = 'stopped' immediately and null out/replace
this.client (or mark a shutdown flag) before disconnecting/clearing interval so
polls see the stopped state, and in poll() set this.isPollInFlight = true at
start and false in a finally block, then before any emit or state update (e.g.,
this.lastStatus, this.lastGestures, detectGestures, emit('status:updated'),
emit('gesture:detected')) check that this.monitorStatus !== 'stopped' and
this.client is unchanged/non-null; if stopped or client changed, return early to
drop the in-flight results. Ensure isFirstPoll handling still sets lastGestures
but only when not stopped.
In `@src/hardware/gestureActionHandler.ts`:
- Around line 95-99: The current logic in gestureActionHandler (when computing
delta from gesture.temperatureChange) treats null the same as 'decrement',
causing unintended decreases; update the delta calculation used to compute
newTemp so it only adds amount for 'increment', subtracts amount for
'decrement', and uses 0 for null/undefined (e.g., set delta =
gesture.temperatureChange === 'increment' ? amount : gesture.temperatureChange
=== 'decrement' ? -amount : 0), keep using currentTemp from
this.deps.findDeviceState and clamp with MIN_TEMP/MAX_TEMP as before.
In `@src/server/routers/health.ts`:
- Around line 207-224: The health endpoint's dacMonitor procedure currently
calls getDacMonitor(), which may lazily create and start a DacMonitor (wiring
GestureActionHandler/DeviceStateSync and starting polling) as a side-effect;
change it to use a non-creating accessor (export and call
getDacMonitorIfRunning() from the DacMonitor instance module) so the health
check only reads the existing monitor instance and does not initialize it. If
getDacMonitorIfRunning() returns undefined/null, have dacMonitor return a
degraded/unavailable status (e.g., status: 'unavailable' or similar, podVersion
null, gesturesSupported false) instead of creating the monitor or throwing,
preserving the existing error handling for real runtime faults.
---
Nitpick comments:
In `@src/hardware/deviceStateSync.ts`:
- Around line 33-52: The code uses two separate new Date() calls for lastUpdated
in the db.insert(values) and .onConflictDoUpdate(set) paths causing slightly
different timestamps; fix this by creating a single timestamp variable (e.g.,
const now = new Date()) immediately before calling await db.insert(...) and
replace both occurrences of new Date() in values and in onConflictDoUpdate.set
with that now variable so both insert and update use the exact same timestamp;
keep identifiers deviceState, db.insert(...).values(...), and
.onConflictDoUpdate(...) unchanged.
- Around line 9-23: DeviceStateSync.sync currently catches errors from await
Promise.all([...this.upsertSide('left', status), this.upsertSide('right',
status)]) and only logs them, which hides failures from upstream callers; update
DeviceStateSync.sync to, after logging, either (a) re-throw the error so the
caller's .catch can observe it, or (b) increment/emit an observability counter
(e.g., metrics.increment('device_state_sync_fail')) and then re-throw — ensure
the change is applied inside the catch block that surrounds the Promise.all call
and keep references to the existing upsertSide('left'| 'right', status) calls
and the sync method signature.
In `@src/hardware/gestureActionHandler.ts`:
- Around line 126-142: The snooze restart currently uses hardcoded alarm
parameters (vibrationIntensity: 50, vibrationPattern: 'rise', duration: 180)
inside the setTimeout in GestureActionHandler; change it to capture the original
alarm settings from the gesture or event (e.g., gesture.alarmVibrationIntensity,
gesture.alarmVibrationPattern, gesture.alarmDuration or the event.alarm config)
before calling client.clearAlarm, then pass those captured values into
restartClient.setAlarm so the restarted alarm matches the original; keep
sensible fallbacks (existing gesture.alarmSnoozeDuration) and continue adding
the timeoutId to this.snoozeTimeouts and cleaning it up as before.
In `@src/hardware/tests/dacMonitor.test.ts`:
- Around line 148-163: The test creates a redundant hardcoded updatesBefore
variable (set to 0) and constructs the updates array after that, making the
final assertion trivial; remove the updatesBefore variable and simplify the
assertion by asserting directly that updates.length is 0 (after creating updates
and subscribing to 'status:updated' on the monitor created via createMonitor(),
which you start then stop via monitor.start() / monitor.stop()) so the test
verifies no further 'status:updated' events are received.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
instrumentation.tssrc/hardware/dacMonitor.instance.tssrc/hardware/dacMonitor.tssrc/hardware/deviceStateSync.tssrc/hardware/gestureActionHandler.deps.tssrc/hardware/gestureActionHandler.tssrc/hardware/tests/dacMonitor.test.tssrc/hardware/tests/gestureActionHandler.test.tssrc/server/routers/health.ts
| // Step 2: Shutdown DAC monitor | ||
| try { | ||
| await shutdownDacMonitor() | ||
| } | ||
| catch (error) { | ||
| console.error('Error shutting down DacMonitor:', error) | ||
| } |
There was a problem hiding this comment.
Shutdown race if DacMonitor initialization is still in-flight.
Because initializeDacMonitor() is fire-and-forget (line 218), a SIGTERM arriving before getDacMonitor() finishes creates a window where shutdownDacMonitor() may find monitorInstance still null (set at dacMonitor.instance.ts:44, before start() but inside the async IIFE). In that case, shutdown returns early while the in-flight initialization continues to create and start a monitor that nobody will stop.
The 10 s force-exit watchdog mitigates this in practice, but if you want a clean stop you could await monitorInitPromise in shutdownDacMonitor before bailing:
Suggested fix in dacMonitor.instance.ts
export const shutdownDacMonitor = async (): Promise<void> => {
+ // If initialization is in-flight, wait for it so we can shut it down cleanly
+ if (monitorInitPromise) {
+ try { await monitorInitPromise } catch { /* start failure is fine */ }
+ }
+
const monitor = monitorInstance
const gestureHandler = gestureHandlerInstance🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@instrumentation.ts` around lines 54 - 60, The shutdown can race with an
in-flight initializeDacMonitor() because shutdownDacMonitor() may return early
if monitorInstance is null; update shutdownDacMonitor in dacMonitor.instance.ts
to first await the shared monitorInitPromise (the promise set by
initializeDacMonitor) before checking/returning so any concurrent initialization
completes and sets monitorInstance, then proceed to stop the monitorInstance
normally; reference the existing symbols monitorInitPromise,
initializeDacMonitor(), getDacMonitor(), shutdownDacMonitor(), and
monitorInstance when making the change.
| catch (error) { | ||
| if (this.monitorStatus !== 'degraded') { | ||
| this.monitorStatus = 'degraded' | ||
| this.emit('connection:lost') | ||
| } | ||
| this.emit('error', error instanceof Error ? error : new Error(String(error))) |
There was a problem hiding this comment.
connection:lost can be skipped after initial connect failure
If the initial connect() fails, monitorStatus becomes 'degraded', and the first failed poll won’t emit connection:lost because the status check suppresses it. This breaks the “fires once on first failed poll” contract. Consider tracking a separate “lost emitted” flag that resets on successful reconnect.
💡 Suggested fix (track one-time lost emission)
export class DacMonitor extends EventEmitter {
private readonly config: Required<DacMonitorConfig>
private client: HardwareClient | null = null
private intervalHandle: ReturnType<typeof setInterval> | null = null
private monitorStatus: DacMonitorStatus = 'stopped'
private lastStatus: DeviceStatus | null = null
private lastGestures: GestureData | null = null
private isFirstPoll = true
private isPollInFlight = false
+ private hasEmittedConnectionLost = false
...
await this.client.connect()
this.monitorStatus = 'running'
+ this.hasEmittedConnectionLost = false
this.emit('connection:established')
...
if (this.monitorStatus === 'degraded') {
this.monitorStatus = 'running'
+ this.hasEmittedConnectionLost = false
this.emit('connection:established')
}
...
catch (error) {
- if (this.monitorStatus !== 'degraded') {
- this.monitorStatus = 'degraded'
- this.emit('connection:lost')
- }
+ if (!this.hasEmittedConnectionLost) {
+ this.emit('connection:lost')
+ this.hasEmittedConnectionLost = true
+ }
+ this.monitorStatus = 'degraded'
this.emit('error', error instanceof Error ? error : new Error(String(error)))
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/hardware/dacMonitor.ts` around lines 153 - 158, The catch block uses
monitorStatus to decide whether to emit 'connection:lost', which prevents
emitting on the first failed poll after an initial failed connect; add a
dedicated boolean flag (e.g., connectionLostEmitted) on the class, set it to
false when a successful connect/reconnect occurs (inside the connect() success
path), and in the catch handler check connectionLostEmitted instead of
monitorStatus to decide to emit 'connection:lost' once, then set
connectionLostEmitted = true; keep emitting 'error' as before. This preserves
the one-time "lost" emission contract while still using monitorStatus for
degraded state.
CodeRabbit feedback — addressed in 24449bdFixed (8 issues)
Pushed back (2 issues)
Tracked as issue (1)Hardcoded snooze alarm parameters — fixing this requires a DB schema change to |
- dacMonitor.instance: clear poisoned singleton (monitorInstance/gestureHandlerInstance) in catch block if start() throws so future callers can retry - dacMonitor.instance: await monitorInitPromise in shutdownDacMonitor to avoid race where in-flight init escapes graceful shutdown - dacMonitor.instance: export getDacMonitorIfRunning() — non-creating accessor for health checks - dacMonitor: set monitorStatus = 'stopped' before clearing interval/client in stop() so in-flight polls see the flag; drop poll results if monitorStatus changed or client was replaced after await - dacMonitor: emit gesture:detected once per delta count (loop deltaL/deltaR) instead of exactly once per poll, preventing missed gestures when counters jump by >1 - gestureActionHandler: guard for null temperatureChange to avoid silent decrement on misconfigured rows - health router: use getDacMonitorIfRunning() — health endpoint no longer lazily initialises the monitor as a side-effect; returns not_initialized when monitor hasn't started - deviceStateSync: hoist new Date() to a single `now` var so insert and onConflictDoUpdate use the same timestamp - dacMonitor.test: simplify redundant updatesBefore variable
|
🎉 This PR is included in version 1.1.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Summary
Scope Breakdown
Test plan
Generated with Claude Code
Summary by CodeRabbit
New Features
Tests