feat: event bus for writes — broadcast device state after mutations#245
Merged
feat: event bus for writes — broadcast device state after mutations#245
Conversation
…244) Remove dead claim_processing / activeClient / processingState code. Add broadcastMutationStatus() to device router so temperature, power, and alarm changes are pushed to all WS clients immediately. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
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:
📝 WalkthroughWalkthroughRemoves client/server claim/heartbeat ownership and in-memory processingState; adds fire-and-forget post-mutation deviceStatus broadcasts, a shared frame normalizer, tests, and client/UI adjustments to operate with a read-only WebSocket pub/sub model. Changes
Sequence DiagramsequenceDiagram
participant Client
participant tRPC as "tRPC API :3000"
participant HW as "Hardware (SequentialQueue)"
participant Dac as "DacMonitor (2s)"
participant Broad as "broadcastMutationStatus"
participant WS as "piezoStream WS :3001"
Client->>tRPC: POST /device/setTemperature
tRPC->>HW: Issue command (SequentialQueue)
HW-->>tRPC: Success
tRPC->>Dac: getLastStatus()
Dac-->>tRPC: lastStatus
tRPC->>Broad: broadcastMutationStatus(overlay)
Broad->>WS: broadcastFrame(deviceStatus)
WS-->>Client: deviceStatus (all subscribers)
par Scheduler / Polling backstop
Dac->>Broad: periodic poll -> broadcastFrame(deviceStatus)
Broad->>WS: deviceStatus (authority)
Note right of Dac: scheduled jobs also call Broad after success
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/hooks/useSensorStream.ts (1)
661-664: Clarify outgoing message scope in the hook docstringLine 663 currently implies only
subscribeis sent, but this hook also sendsseekandget_time_rangevia returned methods. Tightening this wording avoids protocol confusion.✏️ Suggested doc tweak
- * sends subscribe messages, and exposes typed sensor frames. + * sends subscribe/control messages (`subscribe`, `seek`, `get_time_range`), and exposes typed sensor frames.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useSensorStream.ts` around lines 661 - 664, Update the docstring for useSensorStream to clarify the hook sends more than just "subscribe" messages: mention that it also can send "seek" and "get_time_range" via the hook's returned methods (e.g., the seek and get_time_range functions) in addition to automatic subscribe messages, so callers understand the full set of outgoing protocol messages managed by useSensorStream.src/server/routers/device.ts (1)
58-60: Consider logging broadcast failures for debugging.The empty catch block silently swallows all errors. While fire-and-forget is appropriate, completely suppressing errors makes debugging difficult if broadcasts silently start failing.
Also, the ESLint brace-style error should be addressed.
💡 Suggested improvement
- } catch { - // Fire-and-forget — never block the mutation response + } catch (err) { + // Fire-and-forget — never block the mutation response + console.debug('broadcastMutationStatus failed:', err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/routers/device.ts` around lines 58 - 60, The empty catch block in src/server/routers/device.ts (the catch { ... } after the broadcast fire-and-forget) swallows all errors and triggers an ESLint brace-style complaint; update the block to capture the error (e.g., catch (err) { ... }) and log it using the existing logger (or console.error) with a clear context message like "broadcast failed" plus the error details, while keeping the fire-and-forget behavior (do not await the broadcast). Ensure you declare the error parameter (err) to satisfy brace-style rules and include the error object in the log for debugging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/server/routers/device.ts`:
- Around line 326-329: The broadcastMutationStatus call is currently adding
targetLevel: input.powered ? undefined : 0 which injects targetLevel: undefined
when powering ON and later overwrites a valid value; change the overlay so
targetLevel is only included when powering OFF (e.g. only spread { targetLevel:
0 } when input.powered is false), and remove any branch that explicitly sets
targetLevel to undefined so broadcastMutationStatus (and the DeviceStatusFrame)
never receives an undefined targetLevel.
---
Nitpick comments:
In `@src/hooks/useSensorStream.ts`:
- Around line 661-664: Update the docstring for useSensorStream to clarify the
hook sends more than just "subscribe" messages: mention that it also can send
"seek" and "get_time_range" via the hook's returned methods (e.g., the seek and
get_time_range functions) in addition to automatic subscribe messages, so
callers understand the full set of outgoing protocol messages managed by
useSensorStream.
In `@src/server/routers/device.ts`:
- Around line 58-60: The empty catch block in src/server/routers/device.ts (the
catch { ... } after the broadcast fire-and-forget) swallows all errors and
triggers an ESLint brace-style complaint; update the block to capture the error
(e.g., catch (err) { ... }) and log it using the existing logger (or
console.error) with a clear context message like "broadcast failed" plus the
error details, while keeping the fire-and-forget behavior (do not await the
broadcast). Ensure you declare the error parameter (err) to satisfy brace-style
rules and include the error object in the log for debugging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d35692b4-1343-4d71-8a1c-f68a5cd233d4
📒 Files selected for processing (5)
src/hooks/useSensorStream.tssrc/server/routers/biometrics.tssrc/server/routers/device.tssrc/streaming/piezoStream.tssrc/streaming/processingState.ts
💤 Files with no reviewable changes (2)
- src/streaming/processingState.ts
- src/server/routers/biometrics.ts
- README: add WS server + mutation→broadcast path to architecture diagram, replace outdated "Why not WebSocket" tradeoff with current design - project-info: add mutation→WS broadcast to data flow diagram and table - trpc-api-architecture: expand Device Status sequence diagram with mutation broadcast path, document broadcastMutationStatus() behavior Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
my-3 (24px) → mt-2 (8px) so the dial + controls fit in the viewport without scrolling, avoiding touch event interception by the dial. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
-mt-2 on the +/off/- row pulls it 8px closer to the dial on mobile, reclaiming vertical space to keep everything in viewport. sm: unchanged. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (3)
.claude/docs/trpc-api-architecture.md (2)
65-71: Consider clarifying the mutation overlay staleness window.The diagram shows that
broadcastMutationStatus()overlays mutation state ontodacMonitor.getLastStatus(). Between DacMonitor's 2-second polls, this baseline could be up to 2 seconds stale for non-mutated fields. While the 2s backstop ensures eventual consistency, it might be worth adding a brief note that the broadcast reflects the mutation immediately but other status fields may lag until the next poll.📝 Suggested clarification
Add a note below line 71:
Note over WS,UI: Broadcast overlays mutation onto last poll (may be <2s stale for other fields)Or add to the prose documentation around line 133.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/docs/trpc-api-architecture.md around lines 65 - 71, Add a short clarifying note that broadcastMutationStatus() overlays the immediate mutation onto dacMonitor.getLastStatus(), which may be up to the DacMonitor poll interval stale for other fields; update the diagram or nearby prose to include a line such as "Note over WS,UI: Broadcast overlays mutation onto last poll (may be <2s stale for other fields)" or similar, and mention the 2s poll backstop in the paragraph near dacMonitor.getLastStatus() / DacMonitor so readers know non-mutated fields will refresh on the next poll.
133-134: Consider documenting fire-and-forget broadcast failure implications.The documentation correctly states that broadcasts are "fire-and-forget" and "never block the HTTP response," which is excellent for performance. However, it might be worth briefly noting that this means broadcast failures are silent and that the 2s DacMonitor poll serves as the recovery mechanism if a broadcast is missed.
📝 Suggested addition
Extend line 133-134:
-**Event bus:** All mutation procedures (`setTemperature`, `setPower`, `setAlarm`, `clearAlarm`, `snoozeAlarm`) call `broadcastMutationStatus()` after hardware success. This overlays the mutation onto `dacMonitor.getLastStatus()` and broadcasts a `deviceStatus` frame to all WS clients. Fire-and-forget — never blocks the HTTP response. DacMonitor's 2s poll remains the authoritative consistency backstop. +**Event bus:** All mutation procedures (`setTemperature`, `setPower`, `setAlarm`, `clearAlarm`, `snoozeAlarm`) call `broadcastMutationStatus()` after hardware success. This overlays the mutation onto `dacMonitor.getLastStatus()` and broadcasts a `deviceStatus` frame to all WS clients. Fire-and-forget — never blocks the HTTP response. Broadcast failures are silent; DacMonitor's 2s poll remains the authoritative consistency backstop that ensures all clients eventually converge.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/docs/trpc-api-architecture.md around lines 133 - 134, Update the Event bus paragraph to explicitly state the implications of the fire-and-forget behavior: note that broadcastMutationStatus() failures are intentionally silent (do not retry/block the HTTP response) and clients may miss an intermediate deviceStatus frame, and emphasize that dacMonitor.getLastStatus() / DacMonitor's 2s polling acts as the authoritative recovery mechanism to reconcile missed broadcasts. Mention that broadcasts are best-effort only and that the 2s poll is the consistency backstop.README.md (1)
376-376: Update the ~200ms mutation broadcast latency claim. The documentation states mutations "broadcast immediately after success so all connected clients see changes within ~200ms," but no performance tests or benchmarks in the codebase support this figure. Either verify the latency through testing or update the text to indicate it as "typically ~200ms" or an estimated value rather than a measured SLA.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 376, The README makes an unsupported SLA claim that tRPC mutations broadcast and reach all clients "within ~200ms"; update this to reflect it's an estimate (e.g., "typically ~200ms" or "approximately 200ms under normal conditions") or remove the numeric latency unless you add benchmark tests; edit the sentence referencing piezoStream, DacMonitor, and tRPC mutations so it no longer asserts a measured 200ms guarantee and, if desired, add a note that latency may vary by network/clients or that benchmarks should be added to verify it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.claude/docs/trpc-api-architecture.md:
- Around line 65-71: Add a short clarifying note that broadcastMutationStatus()
overlays the immediate mutation onto dacMonitor.getLastStatus(), which may be up
to the DacMonitor poll interval stale for other fields; update the diagram or
nearby prose to include a line such as "Note over WS,UI: Broadcast overlays
mutation onto last poll (may be <2s stale for other fields)" or similar, and
mention the 2s poll backstop in the paragraph near dacMonitor.getLastStatus() /
DacMonitor so readers know non-mutated fields will refresh on the next poll.
- Around line 133-134: Update the Event bus paragraph to explicitly state the
implications of the fire-and-forget behavior: note that
broadcastMutationStatus() failures are intentionally silent (do not retry/block
the HTTP response) and clients may miss an intermediate deviceStatus frame, and
emphasize that dacMonitor.getLastStatus() / DacMonitor's 2s polling acts as the
authoritative recovery mechanism to reconcile missed broadcasts. Mention that
broadcasts are best-effort only and that the 2s poll is the consistency
backstop.
In `@README.md`:
- Line 376: The README makes an unsupported SLA claim that tRPC mutations
broadcast and reach all clients "within ~200ms"; update this to reflect it's an
estimate (e.g., "typically ~200ms" or "approximately 200ms under normal
conditions") or remove the numeric latency unless you add benchmark tests; edit
the sentence referencing piezoStream, DacMonitor, and tRPC mutations so it no
longer asserts a measured 200ms guarantee and, if desired, add a note that
latency may vary by network/clients or that benchmarks should be added to verify
it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4a781a35-907b-4f32-8521-6a178a5e8288
📒 Files selected for processing (3)
.claude/docs/project-info.md.claude/docs/trpc-api-architecture.mdREADME.md
✅ Files skipped from review due to trivial changes (1)
- .claude/docs/project-info.md
- ADR-0015: documents mutation broadcast architecture and claim_processing removal - Move trpc-api-architecture.md from .claude/docs/ to docs/ (project doc, not Claude-specific) - Fix tRPC doc: clarify WS-first with tRPC fallback (not "not polled via tRPC") - Fix broken ADR links in README (0004, 0010 filenames) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace all "Eight Sleep" with neutral phrasing (Pod, stock firmware). SleepyPod → sleepypod everywhere in docs. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Extract broadcastMutationStatus() to shared module so both the device router (user-initiated) and scheduler (automated jobs) broadcast via the same event bus. All hardware writers now push to WS clients immediately after success. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
All four docs (README, project-info, trpc-api-architecture, ADR-0015) now show the scheduler calling broadcastMutationStatus() after scheduled jobs, alongside the device router's user-initiated path. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Fix ReactFlow infinite render loop (React #185) by isolating DAG in memo'd zero-prop component with module-level static nodes/edges - Fix WS subscription bug: pendingSubscription=null (all sensors) was falsy, so subscribe message never sent on connect — only piezo-dual got through. Now calls recomputeAndSendSubscription() in onopen. - Fix frzHealth/frzTherm normalization: firmware nests as left.pump.rpm, left.tec.current, fan.top.rpm but normalizer expected flat keys - Extract normalizeFrame into shared module with typed Wire* interfaces for firmware payloads, add 19 tests with real captured fixtures - Wrap client.send() in try-catch in streaming loop and broadcastFrame - Add error boundary for sensors page - Remove MovementChart from sensors page - Reduce header margin/padding, crop dial SVG dead space - Add cursor-pointer to temp control buttons
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/TempScreen/TempScreen.tsx (1)
174-183:⚠️ Potential issue | 🟡 MinorPower button still shows pointer cursor when disabled
On Line 178,
cursor-pointeris always applied, but this button is disabled whilesetPowerMutation.isPending. That can show a clickable cursor in a disabled state.Suggested fix
className={clsx( - 'flex h-14 w-14 cursor-pointer items-center justify-center rounded-full transition-all duration-200 sm:h-16 sm:w-16', - 'active:scale-95', + 'flex h-14 w-14 cursor-pointer items-center justify-center rounded-full transition-all duration-200 sm:h-16 sm:w-16', + 'active:scale-95 disabled:cursor-default disabled:active:scale-100', isOn ? 'bg-sky-500/20 text-sky-400 border border-sky-500/30' : 'bg-zinc-900 text-zinc-600 border border-transparent', )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/TempScreen/TempScreen.tsx` around lines 174 - 183, The button always has 'cursor-pointer' which shows a clickable cursor even when disabled; update the button in TempScreen (the element using onClick={handlePowerToggle}, disabled={setPowerMutation.isPending}) to conditionally apply cursor classes based on setPowerMutation.isPending (or inverted: !setPowerMutation.isPending) so that when pending the button uses a non-clickable cursor like 'cursor-not-allowed' (or omits 'cursor-pointer') and when not pending it uses 'cursor-pointer'; adjust the clsx invocation around the existing class list and the isOn branch so the cursor class is added/removed appropriately.
🧹 Nitpick comments (10)
src/components/Sensors/SensorsScreen.tsx (1)
103-103: Fix indentation at Line 103 to unblock lint/CI.Line 103 currently violates the configured indentation rule (
@stylistic/indent), which will fail the lint step.Proposed fix
- <div className="-mt-1 space-y-3 pb-4"> + <div className="-mt-1 space-y-3 pb-4">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Sensors/SensorsScreen.tsx` at line 103, The JSX indentation around the div element with className "-mt-1 space-y-3 pb-4" in the SensorsScreen component is misaligned and triggers the `@stylistic/indent` lint rule; locate the SensorsScreen function (or its return JSX) and correct the indentation of that <div> and its child elements so they follow the project's JSX/indent style (align opening/closing tags and nested children consistently with surrounding JSX).app/[lang]/sensors/error.tsx (1)
12-12: Localize user-facing strings on a[lang]route.
Sensors crashedandRetryare hard-coded English strings; these should come from your i18n dictionary/provider for locale consistency.Also applies to: 20-20
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/`[lang]/sensors/error.tsx at line 12, The hard-coded English strings "Sensors crashed" and "Retry" in error.tsx must be replaced with localized values from the app's i18n provider; import and use the route-appropriate translation hook/function (e.g., useTranslations or t) at the top of the component in error.tsx and replace the literal strings in the <p className="text-sm text-red-400"> and the Retry button text with the corresponding translation keys (e.g., sensors.crashed and sensors.retry) so the text is rendered from the locale dictionary instead of hard-coded English.src/scheduler/jobManager.ts (1)
171-173: Inconsistency:schedulePowerOndoesn't broadcast power-on state completely.Unlike
device.ts:setPowerwhich attempts to includetargetLevel, this only conditionally spreadstargetTemperature. For consistency, consider whether power-on should broadcast any additional state (though thedevice.tsimplementation is currently buggy). Once thedevice.tsfix is applied, mirror the corrected pattern here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/scheduler/jobManager.ts` around lines 171 - 173, The broadcast in schedulePowerOn is incomplete: update the broadcastMutationStatus(sched.side, {...}) call to mirror the corrected pattern used in device.ts:setPower by including the same additional state fields (e.g., conditionally include targetLevel alongside targetTemperature) so the power-on broadcast carries the full intended state; ensure you use the same conditional-spread pattern (referencing sched.onTemperature and the corresponding targetLevel source) so both places stay consistent once device.ts is fixed.src/streaming/normalizeFrame.ts (2)
89-104: Duplication: helper functions replicated inpiezoStream.ts.
NO_SENSOR,isSentinel,safeNum, andcdToCare duplicated insrc/streaming/piezoStream.ts:249-265. Consider importing from this module to maintain a single source of truth for sentinel handling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/streaming/normalizeFrame.ts` around lines 89 - 104, Export the shared sentinel helpers (constant NO_SENSOR and functions isSentinel, safeNum, cdToC) from this module and remove the duplicated definitions from piezoStream.ts; update piezoStream.ts to import NO_SENSOR, isSentinel, safeNum, and cdToC from this module so there is a single source of truth for sentinel handling. Ensure the exported symbols have the same names and behavior and adjust any local references in piezoStream.ts to use the imported identifiers.
91-94: Minor: line-break style for logical OR.ESLint prefers the
||operator at the beginning of continuation lines.🔧 Proposed fix
function isSentinel(v: unknown): boolean { - return v === null || v === undefined || v === NO_SENSOR || - (typeof v === 'number' && Math.abs(v - NO_SENSOR) < 0.01) + return v === null + || v === undefined + || v === NO_SENSOR + || (typeof v === 'number' && Math.abs(v - NO_SENSOR) < 0.01) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/streaming/normalizeFrame.ts` around lines 91 - 94, The isSentinel function's multi-line boolean expression should follow ESLint style by placing the logical OR operators at the start of continuation lines; update the return expression in isSentinel to break after the first operand and put each subsequent "||" at the start of its line (referencing isSentinel and NO_SENSOR to locate the code).src/streaming/tests/normalizeFrame.test.ts (2)
125-180: Test coverage gap: no assertions forflowrateorbottomFan.The
frzHealthfixture includestemps.flowrateandfan.bottom.rpm(lines 42, 47, 49), but no tests verify their normalization. This aligns with the missing fields issue innormalizeFrame.ts. Once the normalizer includes these fields, add corresponding test assertions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/streaming/tests/normalizeFrame.test.ts` around lines 125 - 180, The frzHealth tests miss assertions for temps.flowrate and fan.bottom.rpm; update the normalizeFrame tests that use FIRMWARE_FIXTURES.frzHealth to assert that result.left.temps.flowrate (or result.left.flowrate if normalizer flattens it) equals the fixture value and that result.fan.bottom.rpm (or result.fan.bottomRpm if normalized) equals the fixture bottom fan rpm; add these two expectations near the other frzHealth assertions so they fail until normalizeFrame properly maps temps.flowrate and fan.bottom.rpm to the normalized output.
11-25: Stale comments: extraction is complete.These comments discuss a "better approach" to extract
normalizeFrameinto its own module, but that extraction has already been done (the import at line 28 confirms it). Remove or update this commentary.🔧 Proposed cleanup
-// normalizeFrame is not exported — we test it via a re-export helper. -// We'll import the module and call the function directly. -// Since it's a client-side module ('use client'), we import the raw function. - -// --------------------------------------------------------------------------- -// Extract normalizeFrame for testing -// --------------------------------------------------------------------------- - -// The normalizeFrame function lives in useSensorStream.ts. We can't import -// the whole module (it has browser deps). Instead, we duplicate the core -// logic in a testable helper and assert against the real types. -// -// Better approach: extract normalizeFrame into its own module. For now, -// we inline the test fixtures and expected outputs, and import the function -// from a small extraction. - -// We'll test via the extracted module: import { normalizeFrame } from '../normalizeFrame'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/streaming/tests/normalizeFrame.test.ts` around lines 11 - 25, Remove the stale comment block about extracting normalizeFrame: delete or update the multi-line commentary that starts with "normalizeFrame is not exported — we test it via a re-export helper..." and the paragraph stating "Better approach: extract normalizeFrame into its own module." and replace with a short note (or remove entirely) since normalizeFrame is already exported and imported (see normalizeFrame import in the test). Ensure references to useSensorStream.ts or duplication are removed so the test only documents current import of normalizeFrame.src/streaming/broadcastMutationStatus.ts (2)
18-35: API allows partial arguments that are silently ignored.When
sideOverlayis provided withoutside(or vice versa), the overlay is silently discarded (line 32 requires both). This is a subtle footgun for callers. Consider either:
- Making this explicit with an overloaded signature or a single object param
- Throwing/logging when only one is provided
Current callers in
device.tsandjobManager.tsappear to always pass both or neither, but the loose signature invites future misuse.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/streaming/broadcastMutationStatus.ts` around lines 18 - 35, The function broadcastMutationStatus currently accepts side and sideOverlay independently which lets callers pass one without the other; add an explicit runtime check at the start of broadcastMutationStatus to detect mismatched arguments (e.g., side && !sideOverlay or !side && sideOverlay) and handle it deterministically (either throw an Error or log a clear warning and return) so overlays are not silently discarded; update callers only if you choose to change the signature, otherwise keep the existing signature but enforce the validation for side and sideOverlay together to prevent future misuse.
50-52: Fix brace style per ESLint rules.The empty catch is intentional for fire-and-forget semantics, but the formatting triggers a linter error.
🔧 Proposed fix
- } catch { + } + catch { // Fire-and-forget — never block the mutation response }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/streaming/broadcastMutationStatus.ts` around lines 50 - 52, The empty catch block's formatting breaks the project's brace-style rule; update the catch to accept an unused identifier and use a single-line body with the intent comment to satisfy the linter. Specifically, in broadcastMutationStatus (in src/streaming/broadcastMutationStatus.ts) change "catch { /* comment on next line */ }" to "catch (_err) { // Fire-and-forget — never block the mutation response }" so the catch is non-empty and conforms to brace-style.src/streaming/piezoStream.ts (1)
751-751: Good robustness: wrappingclient.send()in try/catch.This correctly handles the race where a client disconnects between the
readyStatecheck and the actual send. The empty catch is appropriate here.Consider expanding to multiple lines per ESLint's
max-statements-per-lineandbrace-stylerules:🔧 Proposed formatting fix (line 751)
- try { client.send(payload) } catch { /* client gone between readyState check and send */ } + try { + client.send(payload) + } + catch { + // client gone between readyState check and send + }🔧 Proposed formatting fix (line 803)
- try { client.send(payload) } catch { /* client gone between readyState check and send */ } + try { + client.send(payload) + } + catch { + // client gone between readyState check and send + }Also applies to: 803-803
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/streaming/piezoStream.ts` at line 751, The one-line try/catch around client.send(payload) violates ESLint rules; change it to a multi-line block so the try and catch have their own lines and the send and comment are on separate lines (e.g., replace "try { client.send(payload) } catch { /* client gone between readyState check and send */ }" with a multi-line try { client.send(payload); } catch { /* client gone between readyState check and send */ }), and make the same multi-line formatting change for the other occurrence of the same try/catch around client.send(payload) in this file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/`[lang]/sensors/error.tsx:
- Around line 13-15: Replace the raw rendering of error.message in the error UI
with a user-safe message: change the JSX that currently renders {error.message}
to display a generic, non-sensitive string (e.g., "An unexpected error
occurred.") for production while still allowing detailed error output in
development only (use process.env.NODE_ENV === 'development' or Next.js runtime
check). Keep a developer-facing log of the error (console.error or a telemetry
call) using the same error variable so debugging info is preserved but not shown
to end users; update the component rendering around the <pre> block that
references error to implement this conditional display.
In `@src/components/TemperatureDial/TemperatureDial.tsx`:
- Around line 206-207: angleToTemp is still scaling the pointer Y coordinate
using VIEW_SIZE which breaks mapping after switching to a non-square viewBox;
update angleToTemp to use VIEW_HEIGHT (or derive centerY = VIEW_HEIGHT/2 and
scale pointerY by VIEW_HEIGHT) wherever VIEW_SIZE is currently used for Y math
so the pointer Y -> angle conversion uses the correct vertical scale and the
drag/selection mapping is accurate.
In `@src/streaming/normalizeFrame.ts`:
- Around line 137-156: The frzHealth branch in normalizeFrame.ts strips fields
UI needs — update the 'frzHealth' case (the code handling WireFrzHealth) to
include left.temps.flowrate, right.temps.flowrate and fan.bottom.rpm (or
fan.bottom with rpm/duty as in the wire shape) in the returned normalized object
so FreezerHealthCard.tsx no longer needs unsafe reconversion; ensure you read
these values from wire.left.temps.flowrate, wire.right.temps.flowrate and
wire.fan.bottom.rpm (with safe null coalescing to 0) alongside the existing
fields.
---
Outside diff comments:
In `@src/components/TempScreen/TempScreen.tsx`:
- Around line 174-183: The button always has 'cursor-pointer' which shows a
clickable cursor even when disabled; update the button in TempScreen (the
element using onClick={handlePowerToggle},
disabled={setPowerMutation.isPending}) to conditionally apply cursor classes
based on setPowerMutation.isPending (or inverted: !setPowerMutation.isPending)
so that when pending the button uses a non-clickable cursor like
'cursor-not-allowed' (or omits 'cursor-pointer') and when not pending it uses
'cursor-pointer'; adjust the clsx invocation around the existing class list and
the isOn branch so the cursor class is added/removed appropriately.
---
Nitpick comments:
In `@app/`[lang]/sensors/error.tsx:
- Line 12: The hard-coded English strings "Sensors crashed" and "Retry" in
error.tsx must be replaced with localized values from the app's i18n provider;
import and use the route-appropriate translation hook/function (e.g.,
useTranslations or t) at the top of the component in error.tsx and replace the
literal strings in the <p className="text-sm text-red-400"> and the Retry button
text with the corresponding translation keys (e.g., sensors.crashed and
sensors.retry) so the text is rendered from the locale dictionary instead of
hard-coded English.
In `@src/components/Sensors/SensorsScreen.tsx`:
- Line 103: The JSX indentation around the div element with className "-mt-1
space-y-3 pb-4" in the SensorsScreen component is misaligned and triggers the
`@stylistic/indent` lint rule; locate the SensorsScreen function (or its return
JSX) and correct the indentation of that <div> and its child elements so they
follow the project's JSX/indent style (align opening/closing tags and nested
children consistently with surrounding JSX).
In `@src/scheduler/jobManager.ts`:
- Around line 171-173: The broadcast in schedulePowerOn is incomplete: update
the broadcastMutationStatus(sched.side, {...}) call to mirror the corrected
pattern used in device.ts:setPower by including the same additional state fields
(e.g., conditionally include targetLevel alongside targetTemperature) so the
power-on broadcast carries the full intended state; ensure you use the same
conditional-spread pattern (referencing sched.onTemperature and the
corresponding targetLevel source) so both places stay consistent once device.ts
is fixed.
In `@src/streaming/broadcastMutationStatus.ts`:
- Around line 18-35: The function broadcastMutationStatus currently accepts side
and sideOverlay independently which lets callers pass one without the other; add
an explicit runtime check at the start of broadcastMutationStatus to detect
mismatched arguments (e.g., side && !sideOverlay or !side && sideOverlay) and
handle it deterministically (either throw an Error or log a clear warning and
return) so overlays are not silently discarded; update callers only if you
choose to change the signature, otherwise keep the existing signature but
enforce the validation for side and sideOverlay together to prevent future
misuse.
- Around line 50-52: The empty catch block's formatting breaks the project's
brace-style rule; update the catch to accept an unused identifier and use a
single-line body with the intent comment to satisfy the linter. Specifically, in
broadcastMutationStatus (in src/streaming/broadcastMutationStatus.ts) change
"catch { /* comment on next line */ }" to "catch (_err) { // Fire-and-forget —
never block the mutation response }" so the catch is non-empty and conforms to
brace-style.
In `@src/streaming/normalizeFrame.ts`:
- Around line 89-104: Export the shared sentinel helpers (constant NO_SENSOR and
functions isSentinel, safeNum, cdToC) from this module and remove the duplicated
definitions from piezoStream.ts; update piezoStream.ts to import NO_SENSOR,
isSentinel, safeNum, and cdToC from this module so there is a single source of
truth for sentinel handling. Ensure the exported symbols have the same names and
behavior and adjust any local references in piezoStream.ts to use the imported
identifiers.
- Around line 91-94: The isSentinel function's multi-line boolean expression
should follow ESLint style by placing the logical OR operators at the start of
continuation lines; update the return expression in isSentinel to break after
the first operand and put each subsequent "||" at the start of its line
(referencing isSentinel and NO_SENSOR to locate the code).
In `@src/streaming/piezoStream.ts`:
- Line 751: The one-line try/catch around client.send(payload) violates ESLint
rules; change it to a multi-line block so the try and catch have their own lines
and the send and comment are on separate lines (e.g., replace "try {
client.send(payload) } catch { /* client gone between readyState check and send
*/ }" with a multi-line try { client.send(payload); } catch { /* client gone
between readyState check and send */ }), and make the same multi-line formatting
change for the other occurrence of the same try/catch around
client.send(payload) in this file.
In `@src/streaming/tests/normalizeFrame.test.ts`:
- Around line 125-180: The frzHealth tests miss assertions for temps.flowrate
and fan.bottom.rpm; update the normalizeFrame tests that use
FIRMWARE_FIXTURES.frzHealth to assert that result.left.temps.flowrate (or
result.left.flowrate if normalizer flattens it) equals the fixture value and
that result.fan.bottom.rpm (or result.fan.bottomRpm if normalized) equals the
fixture bottom fan rpm; add these two expectations near the other frzHealth
assertions so they fail until normalizeFrame properly maps temps.flowrate and
fan.bottom.rpm to the normalized output.
- Around line 11-25: Remove the stale comment block about extracting
normalizeFrame: delete or update the multi-line commentary that starts with
"normalizeFrame is not exported — we test it via a re-export helper..." and the
paragraph stating "Better approach: extract normalizeFrame into its own module."
and replace with a short note (or remove entirely) since normalizeFrame is
already exported and imported (see normalizeFrame import in the test). Ensure
references to useSensorStream.ts or duplication are removed so the test only
documents current import of normalizeFrame.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a166f5b9-4aa3-470c-a7b9-e217e5f9797a
📒 Files selected for processing (28)
.claude/docs/installation-scripts-audit.md.claude/docs/project-info.mdREADME.mdapp/[lang]/sensors/error.tsxdocs/DEPLOYMENT.mddocs/adr/0003-core-stack.mddocs/adr/0012-biometrics-module-system.mddocs/adr/0013-yocto-deployment-toolchain.mddocs/adr/0014-sensor-calibration.mddocs/adr/0015-event-bus-mutation-broadcast.mddocs/hardware/DAC-PROTOCOL.mddocs/piezo-processor.mddocs/trpc-api-architecture.mdscripts/README.mdsrc/components/Header/Header.module.csssrc/components/Sensors/DataPipeline.tsxsrc/components/Sensors/SensorsScreen.tsxsrc/components/TempScreen/TempScreen.tsxsrc/components/TemperatureDial/TemperatureDial.tsxsrc/hardware/tests/README.mdsrc/hooks/useSensorStream.tssrc/scheduler/jobManager.tssrc/server/routers/README.mdsrc/server/routers/device.tssrc/streaming/broadcastMutationStatus.tssrc/streaming/normalizeFrame.tssrc/streaming/piezoStream.tssrc/streaming/tests/normalizeFrame.test.ts
✅ Files skipped from review due to trivial changes (15)
- docs/adr/0014-sensor-calibration.md
- docs/adr/0003-core-stack.md
- docs/adr/0012-biometrics-module-system.md
- src/hardware/tests/README.md
- src/server/routers/README.md
- .claude/docs/installation-scripts-audit.md
- docs/hardware/DAC-PROTOCOL.md
- docs/adr/0013-yocto-deployment-toolchain.md
- src/components/Header/Header.module.css
- scripts/README.md
- docs/DEPLOYMENT.md
- src/hooks/useSensorStream.ts
- docs/piezo-processor.md
- .claude/docs/project-info.md
- README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/trpc-api-architecture.md
- docs/adr/0015-event-bus-mutation-broadcast.md
- setPower: only spread targetLevel:0 when powering off (was spreading undefined when powering on, corrupting the deviceStatus frame) - snoozeManager.test: mock @/src/streaming/piezoStream so Vitest's import analysis doesn't fail resolving it through dacMonitor.instance Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- setPower: only spread targetLevel when powering off (was injecting
undefined, corrupting the deviceStatus frame) — CodeRabbit feedback
- dacMonitor.instance: use relative import paths for dynamic
import('../streaming/piezoStream') so Vite resolves in CI test env
- Auto-fix lint style errors in broadcastMutationStatus, piezoStream,
useSensorStream (brace-style, member-delimiter, operator-linebreak)
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
pnpm lint runs eslint on the entire codebase which has 100+ pre-existing errors. Scope to only TS/TSX files changed in the PR so lint CI actually gates regressions instead of always failing. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Remove the duplicate normalizeFrame + helpers (136 lines) that were left behind after extraction to normalizeFrame.ts. Fixes the no-unused-vars lint error. Auto-fix remaining stylistic lint issues in changed files. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- setPower: include targetLevel + targetTemperature on power-on broadcast so UI power indicator updates instantly (was missing targetLevel) - schedulePowerOn: same fix for scheduled power-on jobs - snoozeManager: broadcast isAlarmVibrating:true when snooze re-fires - broadcastMutationStatus: add console.warn to catch block for observability, fix same-dir import to use ./piezoStream - useSensorStream: remove dead ClaimedMessage/ReleasedMessage types and no-op handler from removed claim_processing protocol Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
src/server/routers/device.ts (1)
281-284: Consider adding a typed overlay interface for compile-time safety.The
broadcastMutationStatusfunction acceptsRecord<string, unknown>for the overlay parameter (persrc/streaming/broadcastMutationStatus.ts:20), which means typos liketargetTempertureorisAlarmVibringwould compile without error but silently fail to update the intended field.A typed interface in
broadcastMutationStatus.tswould catch these at compile time:interface SideOverlay { targetTemperature?: number targetLevel?: number isAlarmVibrating?: boolean }This is optional since the dacMonitor 2s poll provides authoritative consistency, but it would add defense-in-depth.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server/routers/device.ts` around lines 281 - 284, The overlay parameter for broadcastMutationStatus is untyped (Record<string, unknown>) which allows silent typos; add a typed interface (e.g., interface SideOverlay { targetTemperature?: number; targetLevel?: number; isAlarmVibrating?: boolean }) in src/streaming/broadcastMutationStatus.ts, change the broadcastMutationStatus signature to accept SideOverlay or Partial<SideOverlay> instead of Record<string, unknown>, export the interface if needed, and update callers (e.g., the call in device router that passes { ...(input.temperature && { targetTemperature: input.temperature }), ...(!input.powered && { targetLevel: 0 }) }) so TypeScript enforces valid overlay keys at compile time.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/test.yml:
- Line 19: The lint step currently truncates the git diff to 200 lines and uses
plain xargs which can drop files and mishandle names with spaces or leading
dashes; update the CHANGED assignment and invocation so git emits null-delimited
paths (use git diff --name-only -z ...) and remove the head -200 truncation,
then pass those paths safely to eslint with xargs -0 (or use a null-aware loop)
and the -r/--no-run-if-empty behavior so the step prints "No TS/TSX files
changed" when there are none; adjust the existing CHANGED variable and the
subsequent call to pnpm eslint (the line containing CHANGED=$(git diff ... |
head -200); ... | xargs pnpm eslint) to use the null-delimited flow instead.
In `@src/components/Sensors/SensorsScreen.tsx`:
- Around line 137-243: Docstring says a "movement" panel is rendered but
SensorsScreen does not render MovementChart; either update the docstring or
re-add the component. To restore the intended UI, import MovementChart at the
top and add a SensorCard containing <MovementChart /> inside the streamEnabled
fragment (near the other cards like PiezoWaveform/Humidity or wherever
appropriate) so the rendered panels match the file-level docstring;
alternatively, if movement was intentionally removed, update the file-level
docstring to remove "movement" so it reflects the actual rendered components.
In `@src/hooks/useSensorStream.ts`:
- Around line 421-422: The inline single-line try/catch around the frame
callback (try { cb(frame) } catch { /* consumer error */ }) violates the
max-statements-per-line lint rule; change it to a multiline try/catch in the
useSensorStream hook (the place where cb and frame are used) so the invocation
is split across lines: put cb(frame) on its own line inside try { ... } and move
the catch block to its own lines (preserve the empty/ignored catch body or
comment). This targets the block that currently reads try { cb(frame) } catch {
/* consumer error */ } and makes it into a properly multiline try { ... } catch
{ ... } structure.
In `@src/streaming/normalizeFrame.ts`:
- Around line 101-104: The cdToC function currently converts any numeric
centidegree into Celsius, so sentinel freezer-no-sensor value -32768 becomes a
bogus -327.68; update cdToC to treat that sentinel (exact integer -32768) as
missing and return null instead of a numeric value, preserving the existing
null/undefined check and still dividing valid numbers by 100.0; this change will
ensure FrzTempFrame consumers (the code paths that call cdToC) receive null for
missing freezer-temp readings rather than spurious temperatures.
In `@src/streaming/piezoStream.ts`:
- Around line 477-479: The inline single-line try/catch usages (e.g., the one
wrapping fs.closeSync(fd) in src/streaming/piezoStream.ts and the similar
occurrences around the blocks referenced at lines 618 and 671) violate stylistic
rules; replace each single-line try { fs.closeSync(fd) } catch { /* ignore */ }
with expanded multi-line try/catch blocks (use try { fs.closeSync(fd); } catch
(err) { /* ignore */ } or a minimal multi-line catch) so the statements are on
separate lines and the `@stylistic/max-statements-per-line` lint rule is
satisfied; locate the occurrences by searching for fs.closeSync(fd) and update
the surrounding try/catch formatting accordingly.
---
Nitpick comments:
In `@src/server/routers/device.ts`:
- Around line 281-284: The overlay parameter for broadcastMutationStatus is
untyped (Record<string, unknown>) which allows silent typos; add a typed
interface (e.g., interface SideOverlay { targetTemperature?: number;
targetLevel?: number; isAlarmVibrating?: boolean }) in
src/streaming/broadcastMutationStatus.ts, change the broadcastMutationStatus
signature to accept SideOverlay or Partial<SideOverlay> instead of
Record<string, unknown>, export the interface if needed, and update callers
(e.g., the call in device router that passes { ...(input.temperature && {
targetTemperature: input.temperature }), ...(!input.powered && { targetLevel: 0
}) }) so TypeScript enforces valid overlay keys at compile time.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4ac7e4bd-167e-4206-9640-511dc8f5cbc7
📒 Files selected for processing (11)
.github/workflows/test.ymlsrc/components/Sensors/DataPipeline.tsxsrc/components/Sensors/SensorsScreen.tsxsrc/components/TemperatureDial/TemperatureDial.tsxsrc/hardware/dacMonitor.instance.tssrc/hooks/useSensorStream.tssrc/server/routers/biometrics.tssrc/server/routers/device.tssrc/streaming/broadcastMutationStatus.tssrc/streaming/normalizeFrame.tssrc/streaming/piezoStream.ts
✅ Files skipped from review due to trivial changes (1)
- src/streaming/broadcastMutationStatus.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/components/TemperatureDial/TemperatureDial.tsx
- src/server/routers/biometrics.ts
- src/components/Sensors/DataPipeline.tsx
| matrix: | ||
| task: | ||
| - { name: "Lint", cmd: "pnpm lint" } | ||
| - { name: "Lint", cmd: "CHANGED=$(git diff --name-only --diff-filter=d origin/${{ github.event.pull_request.base.ref }}...HEAD -- '*.ts' '*.tsx' | head -200); if [ -n \"$CHANGED\" ]; then echo \"$CHANGED\" | xargs pnpm eslint; else echo 'No TS/TSX files changed'; fi" } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
BASE_REF="origin/dev"
echo "Changed TS/TSX files (first 220):"
git diff --name-only --diff-filter=d "$BASE_REF...HEAD" -- '*.ts' '*.tsx' | nl -ba | sed -n '1,220p'
COUNT=$(git diff --name-only --diff-filter=d "$BASE_REF...HEAD" -- '*.ts' '*.tsx' | wc -l | tr -d ' ')
echo "Total changed TS/TSX files: $COUNT"
echo "Potentially unsafe filenames (whitespace or leading dash):"
git diff --name-only --diff-filter=d "$BASE_REF...HEAD" -- '*.ts' '*.tsx' | rg -n '(^-|[[:space:]])' || trueRepository: sleepypod/core
Length of output: 191
🏁 Script executed:
cat -n .github/workflows/test.yml | head -30Repository: sleepypod/core
Length of output: 1158
🏁 Script executed:
cat package.json | grep -A 20 '"scripts"'Repository: sleepypod/core
Length of output: 897
🏁 Script executed:
find . -type f \( -name '*.ts' -o -name '*.tsx' \) -print0 | xargs -0 basename -a | grep -E '(^-|[[:space:]])' || echo "No problematic filenames found"Repository: sleepypod/core
Length of output: 88
🏁 Script executed:
bash --version | head -3Repository: sleepypod/core
Length of output: 243
Fix silent truncation and unsafe file handling in lint command.
Line 19 truncates changed files to 200 via head -200 and uses plain xargs, which silently skips files in large PRs and risks mis-handling filenames with spaces or leading dashes. This weakens the CI lint gate.
Replace with null-delimited git output and array handling:
Proposed fix
- - { name: "Lint", cmd: "CHANGED=$(git diff --name-only --diff-filter=d origin/${{ github.event.pull_request.base.ref }}...HEAD -- '*.ts' '*.tsx' | head -200); if [ -n \"$CHANGED\" ]; then echo \"$CHANGED\" | xargs pnpm eslint; else echo 'No TS/TSX files changed'; fi" }
+ - { name: "Lint", cmd: "mapfile -d '' CHANGED < <(git diff --name-only --diff-filter=d -z origin/${{ github.event.pull_request.base.ref }}...HEAD -- '*.ts' '*.tsx'); COUNT=${`#CHANGED`[@]}; if [ \"$COUNT\" -eq 0 ]; then echo 'No TS/TSX files changed'; elif [ \"$COUNT\" -gt 200 ]; then echo \"Changed TS/TSX files: $COUNT (>200), running full lint\"; pnpm lint; else pnpm eslint -- \"${CHANGED[@]}\"; fi" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/test.yml at line 19, The lint step currently truncates the
git diff to 200 lines and uses plain xargs which can drop files and mishandle
names with spaces or leading dashes; update the CHANGED assignment and
invocation so git emits null-delimited paths (use git diff --name-only -z ...)
and remove the head -200 truncation, then pass those paths safely to eslint with
xargs -0 (or use a null-aware loop) and the -r/--no-run-if-empty behavior so the
step prints "No TS/TSX files changed" when there are none; adjust the existing
CHANGED variable and the subsequent call to pnpm eslint (the line containing
CHANGED=$(git diff ... | head -200); ... | xargs pnpm eslint) to use the
null-delimited flow instead.
Comment on lines
+137
to
+243
| {streamEnabled && ( | ||
| <> | ||
| {/* Data Pipeline — static DAG + live canvas timeline */} | ||
| <SensorCard> | ||
| <DataPipeline /> | ||
| </SensorCard> | ||
|
|
||
| {/* Piezo Waveform — real-time BCG signal */} | ||
| <SensorCard> | ||
| <PiezoWaveform /> | ||
| </SensorCard> | ||
| {/* Piezo Waveform — real-time BCG signal */} | ||
| <SensorCard> | ||
| <PiezoWaveform /> | ||
| </SensorCard> | ||
|
|
||
| {/* Bed Presence — capacitive sensing with zone activity */} | ||
| <SensorCard> | ||
| <PresenceCard /> | ||
| </SensorCard> | ||
| {/* Bed Presence — capacitive sensing with zone activity */} | ||
| <SensorCard> | ||
| <PresenceCard /> | ||
| </SensorCard> | ||
|
|
||
| {/* Sensor Matrix — Bed Temperature Grid */} | ||
| <SensorCard> | ||
| <BedTempMatrix /> | ||
| </SensorCard> | ||
| {/* Sensor Matrix — Bed Temperature Grid */} | ||
| <SensorCard> | ||
| <BedTempMatrix /> | ||
| </SensorCard> | ||
|
|
||
| {/* Bed Temperature Trend — recharts LineChart (from biometrics) */} | ||
| <SensorCard> | ||
| <div className="space-y-3"> | ||
| <div className="flex items-center justify-between"> | ||
| <div className="flex items-center gap-1.5"> | ||
| <TrendIcon trend={ambientTrend} /> | ||
| <h3 className="text-[10px] font-semibold uppercase tracking-wider text-zinc-500"> | ||
| Bed Temperature Trend | ||
| </h3> | ||
| </div> | ||
| <TimeRangeSelector value={timeRange} onChange={setTimeRange} /> | ||
| </div> | ||
|
|
||
| {bedTempQuery.isLoading ? ( | ||
| <div className="flex h-[200px] items-center justify-center"> | ||
| <div className="h-5 w-5 animate-spin rounded-full border-2 border-zinc-700 border-t-zinc-400" /> | ||
| </div> | ||
| ) : bedTempQuery.isError ? ( | ||
| <div className="flex h-[200px] items-center justify-center text-sm text-red-400"> | ||
| Failed to load temperature data | ||
| {/* Bed Temperature Trend — recharts LineChart (from biometrics) */} | ||
| <SensorCard> | ||
| <div className="space-y-3"> | ||
| <div className="flex items-center justify-between"> | ||
| <div className="flex items-center gap-1.5"> | ||
| <TrendIcon trend={ambientTrend} /> | ||
| <h3 className="text-[10px] font-semibold uppercase tracking-wider text-zinc-500"> | ||
| Bed Temperature Trend | ||
| </h3> | ||
| </div> | ||
| <TimeRangeSelector value={timeRange} onChange={setTimeRange} /> | ||
| </div> | ||
| ) : ( | ||
| <BedTempChart | ||
| data={bedTempQuery.data ?? []} | ||
| unit="F" | ||
| showAmbient | ||
| highlightSide="both" | ||
| /> | ||
| )} | ||
|
|
||
| {/* Summary stats */} | ||
| {summary && ( | ||
| <div className="flex flex-wrap items-center justify-center gap-x-4 gap-y-1 border-t border-zinc-800 pt-2"> | ||
| <SummaryItem | ||
| label="Avg Bed L" | ||
| value={summary.avgLeftCenterTemp != null ? `${Math.round(summary.avgLeftCenterTemp)}°` : '--'} | ||
| /> | ||
| <SummaryItem | ||
| label="Avg Bed R" | ||
| value={summary.avgRightCenterTemp != null ? `${Math.round(summary.avgRightCenterTemp)}°` : '--'} | ||
| /> | ||
| <SummaryItem | ||
| label="Avg Ambient" | ||
| value={summary.avgAmbientTemp != null ? `${Math.round(summary.avgAmbientTemp)}°` : '--'} | ||
| /> | ||
| <SummaryItem | ||
| label="Humidity" | ||
| value={summary.avgHumidity != null ? `${Math.round(summary.avgHumidity)}%` : '--'} | ||
| /> | ||
| </div> | ||
| )} | ||
| </div> | ||
| </SensorCard> | ||
| {bedTempQuery.isLoading | ||
| ? ( | ||
| <div className="flex h-[200px] items-center justify-center"> | ||
| <div className="h-5 w-5 animate-spin rounded-full border-2 border-zinc-700 border-t-zinc-400" /> | ||
| </div> | ||
| ) | ||
| : bedTempQuery.isError | ||
| ? ( | ||
| <div className="flex h-[200px] items-center justify-center text-sm text-red-400"> | ||
| Failed to load temperature data | ||
| </div> | ||
| ) | ||
| : ( | ||
| <BedTempChart | ||
| data={bedTempQuery.data ?? []} | ||
| unit="F" | ||
| showAmbient | ||
| highlightSide="both" | ||
| /> | ||
| )} | ||
|
|
||
| {/* Humidity Trend — recharts AreaChart (from biometrics) */} | ||
| <SensorCard> | ||
| <div className="space-y-2"> | ||
| <div className="flex items-center gap-1.5"> | ||
| <Droplets size={10} className="text-[#4a90d9]" /> | ||
| <h3 className="text-[10px] font-semibold uppercase tracking-wider text-zinc-500"> | ||
| Humidity | ||
| </h3> | ||
| {/* Summary stats */} | ||
| {summary && ( | ||
| <div className="flex flex-wrap items-center justify-center gap-x-4 gap-y-1 border-t border-zinc-800 pt-2"> | ||
| <SummaryItem | ||
| label="Avg Bed L" | ||
| value={summary.avgLeftCenterTemp != null ? `${Math.round(summary.avgLeftCenterTemp)}°` : '--'} | ||
| /> | ||
| <SummaryItem | ||
| label="Avg Bed R" | ||
| value={summary.avgRightCenterTemp != null ? `${Math.round(summary.avgRightCenterTemp)}°` : '--'} | ||
| /> | ||
| <SummaryItem | ||
| label="Avg Ambient" | ||
| value={summary.avgAmbientTemp != null ? `${Math.round(summary.avgAmbientTemp)}°` : '--'} | ||
| /> | ||
| <SummaryItem | ||
| label="Humidity" | ||
| value={summary.avgHumidity != null ? `${Math.round(summary.avgHumidity)}%` : '--'} | ||
| /> | ||
| </div> | ||
| )} | ||
| </div> | ||
| {bedTempQuery.isLoading ? ( | ||
| <div className="flex h-[140px] items-center justify-center"> | ||
| <div className="h-5 w-5 animate-spin rounded-full border-2 border-zinc-700 border-t-zinc-400" /> | ||
| </div> | ||
| ) : ( | ||
| <HumidityChart data={bedTempQuery.data ?? []} /> | ||
| )} | ||
| </div> | ||
| </SensorCard> | ||
| </SensorCard> | ||
|
|
||
| {/* Movement — keep chart, no duplicate date picker */} | ||
| <SensorCard> | ||
| <MovementChart hideNav /> | ||
| </SensorCard> | ||
| {/* Humidity Trend — recharts AreaChart (from biometrics) */} | ||
| <SensorCard> | ||
| <div className="space-y-2"> | ||
| <div className="flex items-center gap-1.5"> | ||
| <Droplets size={10} className="text-[#4a90d9]" /> | ||
| <h3 className="text-[10px] font-semibold uppercase tracking-wider text-zinc-500"> | ||
| Humidity | ||
| </h3> | ||
| </div> | ||
| {bedTempQuery.isLoading | ||
| ? ( | ||
| <div className="flex h-[140px] items-center justify-center"> | ||
| <div className="h-5 w-5 animate-spin rounded-full border-2 border-zinc-700 border-t-zinc-400" /> | ||
| </div> | ||
| ) | ||
| : ( | ||
| <HumidityChart data={bedTempQuery.data ?? []} /> | ||
| )} | ||
| </div> | ||
| </SensorCard> | ||
|
|
||
| {/* System — freezer thermal health */} | ||
| <SensorCard> | ||
| <FreezerHealthCard /> | ||
| </SensorCard> | ||
| </> | ||
| )} | ||
| </div> | ||
| {/* System — freezer thermal health */} | ||
| <SensorCard> | ||
| <FreezerHealthCard /> | ||
| </SensorCard> | ||
| </> | ||
| )} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether movement visualization is still rendered anywhere relevant.
fd MovementChart src
rg -n -C3 'MovementChart|movement' src/components/Sensors/SensorsScreen.tsx
rg -n -C3 'MovementChart' src/components/Sensors src/components/EnvironmentRepository: sleepypod/core
Length of output: 524
🏁 Script executed:
# Check imports and file-level docstring in SensorsScreen.tsx
head -35 src/components/Sensors/SensorsScreen.tsx
# Search for MovementChart imports and usage across the codebase
rg -n 'import.*MovementChart' src/
# Check if MovementChart is rendered anywhere in the entire app
rg -n 'MovementChart' src/ --type tsx --type tsRepository: sleepypod/core
Length of output: 1807
🏁 Script executed:
# Search for MovementChart usage anywhere in the codebase (without --type flag)
rg 'MovementChart' src/
# Also check if it's used in other screen files
rg -n 'movement' src/components/Sensors/ src/screens/Repository: sleepypod/core
Length of output: 419
🏁 Script executed:
# Check if MovementChart is imported or used anywhere else in the app
rg 'MovementChart' src/ | grep -v "export function MovementChart"
# Check the full scope of files changed/affected (get repo structure)
find src/components/Sensors -type f -name "*.tsx" | head -20Repository: sleepypod/core
Length of output: 726
Docstring-code mismatch: movement panel is not rendered but documented as present
The file-level docstring (line 28) claims the component "renders all live sensor data panels: connection bar, sensor matrix (bed temp), presence with zone activity, piezo waveform, bed temp trend (recharts), humidity (recharts), movement, and system health." However, the rendered card stack (lines 137-243) does not include a MovementChart component. The MovementChart component exists in the codebase but is not imported or used. Either the movement visualization was intentionally removed and the docstring needs updating, or the docstring was written before implementation and should be corrected to match the current render.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/Sensors/SensorsScreen.tsx` around lines 137 - 243, Docstring
says a "movement" panel is rendered but SensorsScreen does not render
MovementChart; either update the docstring or re-add the component. To restore
the intended UI, import MovementChart at the top and add a SensorCard containing
<MovementChart /> inside the streamEnabled fragment (near the other cards like
PiezoWaveform/Humidity or wherever appropriate) so the rendered panels match the
file-level docstring; alternatively, if movement was intentionally removed,
update the file-level docstring to remove "movement" so it reflects the actual
rendered components.
- Break circular dependency: snoozeManager → broadcastMutationStatus → snoozeManager by using dynamic import in async callback - Flatten vitest config: v4 projects don't inherit root plugins, so tsconfigPaths() wasn't resolving @/ aliases for test files - Exclude .claude/worktrees from vitest and eslint to prevent stale worktree files from interfering with checks - Fix lint errors in PR-changed files: non-null assertions, brace style, max-statements-per-line, require() imports Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR converts the WebSocket channel to a read-only pub/sub model and reduces multi-client latency by broadcasting updated deviceStatus frames immediately after successful hardware mutations (and scheduled jobs), while removing the unused claim_processing ownership/heartbeat protocol.
Changes:
- Remove dead “claim_processing”/heartbeat/processing ownership plumbing across WS server, client hook, and biometrics API.
- Add
broadcastMutationStatus()and invoke it after device mutations and scheduler jobs to push an immediatedeviceStatusframe to all WS clients. - Extract frame normalization into a dedicated module with new fixtures/tests, plus assorted UI/docs tweaks.
Reviewed changes
Copilot reviewed 34 out of 34 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| vitest.config.mts | Simplifies Vitest config and adds exclusions. |
| src/streaming/tests/normalizeFrame.test.ts | Adds fixtures/tests for the extracted frame normalizer. |
| src/streaming/processingState.ts | Deletes unused iOS processing ownership singleton. |
| src/streaming/piezoStream.ts | Removes claim/heartbeat protocol; hardens WS sends; minor refactors. |
| src/streaming/normalizeFrame.ts | New shared frame-normalization module (used by client). |
| src/streaming/broadcastMutationStatus.ts | New helper to broadcast deviceStatus after mutations/jobs. |
| src/server/routers/device.ts | Calls broadcastMutationStatus() after device mutations succeed. |
| src/server/routers/biometrics.ts | Removes getProcessingStatus endpoint and related imports. |
| src/server/routers/README.md | Updates router README wording/branding. |
| src/scheduler/jobManager.ts | Broadcasts mutation status after scheduled temperature/power/alarm actions. |
| src/hooks/useSensorStream.ts | Removes heartbeat + moves normalization import; subscription/refactor tweaks. |
| src/hardware/snoozeManager.ts | Broadcasts status when snoozed alarm restarts. |
| src/components/TemperatureDial/TemperatureDial.tsx | Tweaks SVG viewBox/cropping and small formatting changes. |
| src/components/TempScreen/TempScreen.tsx | Layout tweaks for temperature controls. |
| src/components/SideSelector/SideSelector.tsx | Minor spacing tweak. |
| src/components/Sensors/SensorsScreen.tsx | Removes Movement chart and adjusts layout/conditional rendering. |
| src/components/Sensors/DataPipeline.tsx | Memo-izes/static-izes the ReactFlow DAG to avoid render loop issues. |
| src/components/Header/Header.module.css | Adjusts header padding. |
| scripts/README.md | Branding/wording updates. |
| eslint.config.ts | Ignores .claude/**. |
| docs/trpc-api-architecture.md | Documents WS-first status + mutation broadcast path. |
| docs/piezo-processor.md | Wording/branding update. |
| docs/hardware/DAC-PROTOCOL.md | Wording/branding update. |
| docs/adr/0015-event-bus-mutation-broadcast.md | New ADR documenting the event-bus mutation broadcast decision. |
| docs/adr/0014-sensor-calibration.md | Wording/branding update. |
| docs/adr/0013-yocto-deployment-toolchain.md | Wording/formatting update. |
| docs/adr/0012-biometrics-module-system.md | Wording/branding update. |
| docs/adr/0003-core-stack.md | Wording/branding update. |
| docs/DEPLOYMENT.md | Wording/branding update. |
| app/[lang]/sensors/error.tsx | Adds an error boundary UI for the sensors route. |
| README.md | Updates architecture diagram and real-time WS description + ADR links. |
| .claude/docs/project-info.md | Updates internal project doc architecture diagram. |
| .claude/docs/installation-scripts-audit.md | Wording/branding updates. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| await client.setTemperature(input.side, input.temperature, input.duration) | ||
| return { success: true } | ||
| }, 'Failed to set temperature') | ||
| broadcastMutationStatus(input.side, { targetTemperature: input.temperature }) |
There was a problem hiding this comment.
After a temperature change succeeds, the broadcast overlay only sets targetTemperature. The UI uses targetLevel !== 0 to determine whether a side is “on” (e.g. TempScreen/PowerButton), so clients can remain “off” until the next 2s DacMonitor poll. Include targetLevel (e.g. via fahrenheitToLevel(input.temperature)) in the overlay so the immediate WS frame reflects powered state consistently.
Suggested change
| broadcastMutationStatus(input.side, { targetTemperature: input.temperature }) | |
| broadcastMutationStatus(input.side, { | |
| targetTemperature: input.temperature, | |
| targetLevel: fahrenheitToLevel(input.temperature), | |
| }) |
| await client.connect() | ||
| try { | ||
| await client.setTemperature(sched.side, sched.temperature) | ||
| broadcastMutationStatus(sched.side, { targetTemperature: sched.temperature }) |
There was a problem hiding this comment.
scheduleTemperature() broadcasts only targetTemperature. Because the UI derives “powered on” from targetLevel, scheduled temperature changes can still look off until the next poll. Overlay targetLevel derived from the scheduled temperature in this broadcast.
Suggested change
| broadcastMutationStatus(sched.side, { targetTemperature: sched.temperature }) | |
| broadcastMutationStatus(sched.side, { | |
| targetTemperature: sched.temperature, | |
| targetLevel: fahrenheitToLevel(sched.temperature), | |
| }) |
| duration: sched.duration, | ||
| }) | ||
| broadcastMutationStatus(sched.side, { | ||
| targetTemperature: sched.alarmTemperature, |
There was a problem hiding this comment.
In the alarm job you set temperature first, but the immediate broadcast overlay doesn’t include targetLevel. That can leave clients showing the side as off until the next DacMonitor poll. Include targetLevel (derived from sched.alarmTemperature) alongside targetTemperature in this overlay.
Suggested change
| targetTemperature: sched.alarmTemperature, | |
| targetTemperature: sched.alarmTemperature, | |
| targetLevel: fahrenheitToLevel(sched.alarmTemperature), |
Comment on lines
695
to
704
| export function useOnSensorFrame(callback: FrameCallback) { | ||
| const callbackRef = useRef(callback) | ||
| callbackRef.current = callback | ||
|
|
||
| useEffect(() => { | ||
| const handler: FrameCallback = (frame) => callbackRef.current(frame) | ||
| callbackRef.current = callback | ||
| }) | ||
|
|
||
| useEffect(() => { | ||
| const handler: FrameCallback = frame => callbackRef.current(frame) | ||
| frameCallbacks.add(handler) |
There was a problem hiding this comment.
useOnSensorFrame now updates callbackRef.current in a useEffect, which means frames arriving between render commit and the effect running can call a stale callback (especially in a high-frequency stream). Update the ref synchronously during render (common pattern) or use useLayoutEffect if you need to avoid mutating during render.
Comment on lines
+493
to
+507
| function updatePollRate(): void { | ||
| try { | ||
| // eslint-disable-next-line @typescript-eslint/no-require-imports -- lazy import to avoid circular deps | ||
| const { getDacMonitorIfRunning } = require('@/src/hardware/dacMonitor.instance') | ||
| const monitor = getDacMonitorIfRunning() | ||
| if (!monitor) return | ||
| const clientCount = wss?.clients.size ?? 0 | ||
| if (clientCount > 0) { | ||
| monitor.setActive() | ||
| } else { | ||
| } | ||
| else { | ||
| monitor.setIdle() | ||
| } | ||
| } catch { /* monitor not started yet */ } | ||
| } | ||
| catch { /* monitor not started yet */ } |
There was a problem hiding this comment.
updatePollRate() uses require() inside an ESM codebase (package.json has "type": "module") and needs an eslint disable. Elsewhere (e.g. dacMonitor.instance.ts) lazy loading uses import() to avoid circular deps. Consider switching this to a dynamic import() (or createRequire) so the module loading strategy is consistent and doesn’t depend on a CJS require being available at runtime.
Comment on lines
+11
to
+28
| // normalizeFrame is not exported — we test it via a re-export helper. | ||
| // We'll import the module and call the function directly. | ||
| // Since it's a client-side module ('use client'), we import the raw function. | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // Extract normalizeFrame for testing | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| // The normalizeFrame function lives in useSensorStream.ts. We can't import | ||
| // the whole module (it has browser deps). Instead, we duplicate the core | ||
| // logic in a testable helper and assert against the real types. | ||
| // | ||
| // Better approach: extract normalizeFrame into its own module. For now, | ||
| // we inline the test fixtures and expected outputs, and import the function | ||
| // from a small extraction. | ||
|
|
||
| // We'll test via the extracted module: | ||
| import { normalizeFrame } from '../normalizeFrame' |
There was a problem hiding this comment.
The header comments here describe normalizeFrame as “not exported” and say the test duplicates/extracts core logic because the original lived in useSensorStream.ts. In this PR normalizeFrame is exported from src/streaming/normalizeFrame.ts and is imported directly, so these comments are now misleading and should be updated/trimmed to reflect the current setup.
- Fix frzTemp sentinel: cdToC() now rejects -32768 as null - Fix frzHealth: add flowrate/bottomRpm to normalizer and FrzHealthFrame - Fix TemperatureDial: scaleY uses VIEW_HEIGHT instead of VIEW_SIZE - Fix error.tsx: show generic message instead of raw error.message - Fix broadcast overlays: add targetLevel to setTemperature and alarm jobs - Fix useOnSensorFrame: useLayoutEffect for callback ref (no stale frames) - Fix piezoStream: convert require() to dynamic import() for ESM - Fix normalizeFrame.test.ts: remove stale comments about non-exported fn - Update README and API docs: show DacTransport, read/write bus subgraphs
Switch from graph TD to graph LR so the data flow reads naturally left-to-right across five columns: Pod Hardware → Hardware Transport → Biometrics Sidecars → sleepypod-core (read/write buses) → Clients. broadcastFrame() is now an explicit convergence node — both the read bus (DacMonitor 2s poll) and write bus (broadcastMutationStatus) funnel through it before entering the WS server, making it clear that the WebSocket is a delivery pipe, not the event bus itself.
- error.tsx: break inline expression onto separate lines - FreezerHealthCard: remove unused imports and dead flowrate/bottomFan vars (UI doesn't render them), fix multiline-ternary - TemperatureDial: fix multiline-ternary formatting
- Remove unused UserSelector import - Break inline onSettled callback onto separate lines
|
🎉 This PR is included in version 1.1.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
7 tasks
5 tasks
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
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
Closes #244.
claim_processingprotocol — deleteprocessingState.ts, stripactiveClient/heartbeat/release machinery frompiezoStream.ts, removegetProcessingStatusfrom biometrics router, remove client-side heartbeat sending fromuseSensorStream.tsbroadcastMutationStatus()to device router — aftersetTemperature,setPower,setAlarm,clearAlarm, andsnoozeAlarmsucceed, overlay the mutation ontodacMonitor.getLastStatus()and callbroadcastFrame()so all WS clients see the change instantlyNet: −135 lines, +56 lines (79 lines removed)
Test plan
tsc --noEmitcleanUnknown message type: heartbeaterrors in WS server logs (client no longer sends heartbeats)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Refactor
UI Changes
Documentation