fix(workout): suppress PPG spikes in the workout max HR (#127)#133
Merged
Conversation
The workout summary took a raw `hr.reduce(math.max)` over the 1 Hz samples with no artefact rejection, so a single 1-2 s PPG motion spike defined the session max (160 bpm) while the HR page's minute-mean peak showed the true 143. Add a shared smoother (compute/hr_max.dart): a physiological reject (age-derived ceiling, hard-capped at 220) plus a short rolling median (~5 s) that steps over 1-2 s transients while preserving a genuine brief effort peak. Apply it consistently to all three producers of the max: - getWorkout: RECOMPUTE from the smoothed raw instead of flooring against the stored column (which may already hold a spiked live value); the time-to-peak index follows the smoothed peak. - getWorkouts list: fill max_hr from the batched raw via the shared smoother so the list agrees with the detail screen; the SQL MAX(d.hr) is now physiologically ceiling-bounded and used only as a last fallback when raw is pruned. - live tick: maxHrSeen now accrues through a rolling-median accumulator so a spike can't set the session max or fire a spurious "new max!". Keep the HR-page minute-mean path unchanged. Add unit + repo-seam tests: a 1-2 s spike is excluded, a sustained peak is preserved, the live accumulator matches the batch recompute, and getWorkout/getWorkouts report the same smoothed peak.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughAdds age-aware physiological filtering and rolling-median heart-rate peak computation across repository enrichment and live workout tracking, including peak-index reporting, updated milestone gating, database sample retrieval, and spike-focused tests. ChangesHeart-rate peak pipeline
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AppState
participant LiveWorkoutState
participant RollingMaxHr
participant LiveSessionScreen
AppState->>LiveWorkoutState: accrueHr(currentHr)
LiveWorkoutState->>RollingMaxHr: add(hr)
RollingMaxHr-->>LiveWorkoutState: update maxHrSeen
LiveSessionScreen->>LiveWorkoutState: evaluate maxHrSeen
LiveSessionScreen-->>LiveSessionScreen: emit NEW MAX when gated
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/workout_max_hr_spike_test.dart`:
- Around line 188-192: Make the getWorkouts list test self-contained by
extracting the w-spike session and sample setup from the preceding test into a
shared fixture helper, then invoke that helper from both tests (or seed it in
setUp). Ensure the test still verifies the same smoothed max_hr value without
relying on execution order.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1ba75a92-4a1a-4351-a9a0-d34135627c62
📒 Files selected for processing (6)
lib/compute/hr_max.dartlib/data/db.dartlib/data/local_repository_impl.dartlib/state/app_state.dartlib/ui/activity/live_session_screen.darttest/workout_max_hr_spike_test.dart
Seed the w-spike session + samples once in setUpAll via a shared helper rather than inside the first test, so the getWorkouts-list test no longer depends on the preceding getWorkout test having created the fixture. Each test is now self-contained and passes when run alone.
) Symmetric to the max fix: a single 1-2 s low PPG dropout could define the workout min via the raw `reduce(math.min)`. Apply the same rolling median + physiological reject to the trough. - hr_max.dart: factor the max helper into a shared core and add smoothedMinHr / smoothedMinHrAt (rolling median over the same window so a 1-2 s low can't be the median; the floor/ceiling reject drops garbage such as a stray sub-30 bpm reading before the min). - getWorkout: min_hr now uses smoothedMinHr instead of reduce(math.min). - getWorkouts list: fill min_hr from the batched raw via smoothedMinHr so the list agrees with the detail recompute; the SQL MIN(d.hr) is now floor-bounded (minHrFloor) and used only when raw is pruned. The live path tracks no running min, so no accumulator was added there. Keep the HR-page path untouched. Extend the tests: a 1-2 s low dropout is excluded, a genuine sustained low is preserved, the physiological floor drops sub-30 garbage, and the repo-seam fixture now carries a 40 bpm dropout + 20 bpm garbage that neither getWorkout nor getWorkouts lets define the min.
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
2 participants
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.
Problem
Closes #127. The workout summary shows max HR 160 bpm while the Heart-rate page peak is 143 (correct). The HR page peak is the max of per-minute-averaged HR (transient spikes averaged away), but the workout max was a raw
hr.reduce(math.max)over the 1 Hz samples with no artefact rejection — so a single 1–2 s PPG motion spike defines the whole session's max.Fix
New shared smoother
lib/compute/hr_max.dart:220 − age+ headroom, hard-capped at 220).Applied consistently to all three producers that write the workout max, so the detail screen, the workout list, and the live session agree:
getWorkout(local_repository_impl.dart) — RECOMPUTE from the smoothed raw instead ofmath.max(stored, peak). Flooring against the stored column would preserve a spike the old live path had already written. Time-to-peak follows the smoothed peak's index.getWorkoutslist (local_repository_impl.dart+db.dart) — fillmax_hrfrom the batched raw 1 Hz via the shared smoother (one indexed join), so the list matches the detail recompute. The SQLMAX(d.hr)is now physiologically ceiling-bounded and used only as a coarse last fallback when raw has been pruned (14-day retention).app_state.dart) —maxHrSeennow accrues through a rolling-median accumulator (RollingMaxHr) rather than> maxHrSeen, so a spike can't set the session max or fire a spurious "new max!" callout. The live-session "NEW MAX" moment keys off the smoothed value.The HR-page minute-mean path is unchanged.
Tests
test/workout_max_hr_spike_test.dart:RollingMaxHr/LiveWorkoutState.accrueHr) matches the batch recompute.LocalDb(sqflite_ffi):getWorkoutandgetWorkoutsboth report the smoothed peak (a persisted spikedmax_hrof 160 is overridden), and the two agree.Not verified without a device
No Flutter/Dart SDK in this environment, so
flutter analyze/flutter testwere not run — the change is verified by inspection and the new tests are written to the existingworkout_enrichment_test.dartharness. Please run the suite on a machine with the SDK.On-device confirmation still wanted: inspect the raw
decoded_onehzHR over the reported session to confirm the 160 was in fact a 1–2 s outlier (not a genuine short burst), and eyeball that the summary now matches the HR-page 143.Summary by CodeRabbit