Fix piezo processor selecting SEQNO.RAW instead of sensor data#247
Conversation
|
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. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR implements an event-bus architectural shift: removing iOS "processing ownership" claim-based state management, introducing Changes
Sequence DiagramsequenceDiagram
actor User
participant tRPC API
participant DacMonitor
participant DAC Hardware
participant WebSocket Server
participant Client
User->>tRPC API: setTemperature(side, target)
tRPC API->>DAC Hardware: write command via dacTransport
DAC Hardware-->>tRPC API: success
tRPC API->>DacMonitor: broadcastMutationStatus(side, {targetTemperature})
DacMonitor->>DacMonitor: getLastStatus()
DacMonitor->>WebSocket Server: broadcastFrame({type: 'deviceStatus', ...})
WebSocket Server->>Client: emit frame (~200ms latency)
Client->>Client: normalizeFrame(payload)
Client->>User: update UI with new temperature
Note over DacMonitor: Authoritative 2s polling<br/>continues independently
sequenceDiagram
participant Scheduler
participant DacMonitor
participant DAC Hardware
participant WebSocket Server
Scheduler->>Scheduler: scheduled job tick
Scheduler->>DAC Hardware: executeJob (temperature/power/alarm)
DAC Hardware-->>Scheduler: success
Scheduler->>DacMonitor: broadcastMutationStatus(side, {targetTemperature, ...})
DacMonitor->>WebSocket Server: broadcastFrame({type: 'deviceStatus', ...})
WebSocket Server-->>Client: stream update (immediate)
par DacMonitor polling
DacMonitor->>DAC Hardware: query status
DAC Hardware-->>DacMonitor: current state
DacMonitor->>WebSocket Server: broadcastFrame (every 2s)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes This diff involves heterogeneous changes across multiple architectural layers: removal of a substantial control plane (processing state/claim-release), introduction of a new fire-and-forget broadcast pattern with overlaid mutation state, refactoring of client streaming to use shared normalization, UI component optimizations, and distributed sensor frame handling logic. The Possibly related issues
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 |
There was a problem hiding this comment.
Pull request overview
Fixes a production bug in the Python biometrics pipeline where RawFileFollower._find_latest() could select SEQNO.RAW (a metadata/index file) as the “newest” RAW file and then tail/parse it as CBOR, resulting in zero biometric processing.
Changes:
- Exclude
SEQNO.RAWfromRawFileFollower._find_latest()candidate selection so only actual sensor data RAW files are tailed.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ng
force-pushed
the
fix/246-exclude-seqno-raw
branch
2 times, most recently
from
62c4abe to
b422c60
Compare
Filter SEQNO.RAW from RawFileFollower._find_latest() so the metadata file is never mistaken for sensor data. Already done in prototype but never ported to production. Also: scope CI lint to changed files only, fix dacMonitor dynamic import path for test resolution. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ng
force-pushed
the
fix/246-exclude-seqno-raw
branch
from
b422c60 to
c3f014a
Compare
ng
left a comment
ng
left a comment
There was a problem hiding this comment.
Adversarial Code Review — PR #247
Depth: Standard (Sonnet Optimizer + Sonnet Skeptic)
Verdict: Approve with minor suggestions
The fix is correct
The SEQNO.RAW exclusion directly addresses the root cause. The Optimizer suggested also adding a file-size guard (matching the prototype's 100K threshold), but the Skeptic correctly identified that threshold is from a batch analysis script — the live follower is designed to tail partially-written files. Adding a size guard would introduce startup data gaps. Rejected.
Suggested improvements (non-blocking)
1. Add inline comment (raw_follower.py:49)
Without context, a future maintainer may remove the filter as dead code:
# SEQNO.RAW is a firmware metadata file, not sensor data (see #246)2. Add unit tests for _find_latest()
This is a regression fix for a production outage with zero test coverage. The piezo-processor tests stub out RawFileFollower entirely. Suggested test cases:
- Only
SEQNO.RAWpresent → returnsNone SEQNO.RAWhas newer mtime than data file → returns data file- Empty directory → returns
None
Pre-existing issues noted (not this PR's fault)
raw_follower.py:66:open(latest, "rb")outside try/except — file deletion between_find_latest()andopen()crashes the generatorcalibrator/main.py:89:sorted(..., key=lambda p: p.stat().st_mtime)with noFileNotFoundErrorprotection — crashes on file rotation
Full review artifacts: .claude/reviews/fix-246-exclude-seqno-raw/
## Summary - Add inline comment explaining why `SEQNO.RAW` is excluded from `_find_latest()` - Add 4 unit tests for the exclusion behavior to guard against regression Follow-up from adversarial review of #247. ## Test plan - [x] All 4 new tests pass - [x] Covers: empty dir, SEQNO-only, SEQNO with newer mtime, multiple data files 🤖 Generated with [Claude Code](https://claude.com/claude-code)
|
🎉 This PR is included in version 1.1.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Summary
SEQNO.RAWfromRawFileFollower._find_latest()candidates so the metadata file is never mistaken for sensor dataprototype_v2.py:511) but never ported to productionTest plan
SEQNO.RAWpresent that actual data file is selectedFixes #246
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
UI/UX Improvements
Documentation