Steps from a real pedometer only; movement minutes on measured evidence - #182
Steps from a real pedometer only; movement minutes on measured evidence#182abdulsaheel wants to merge 6 commits into
Conversation
A user reported 2,645 steps on a day they took under 400. On their real DB,
81.5% of every step figure the app has ever shown (41,753 of 51,225 across 26
days) was manufactured by a 1 Hz estimator that cannot see gait. This makes
steps real-measured-only and rebuilds what remains on measured evidence.
STEPS ARE NOW REAL-MEASURED ONLY (kAlgoVersion 54 -> 56)
`scalars.steps` is ABSENT, not 0, unless something that can actually resolve
gait measured the day. The 1 Hz substrate contributes nothing to it. The old
hybrid also persisted a hard 0.0 whenever the estimator abstained, which
poisoned the `dyn_p90` median and dragged every average and record down.
PHONE PEDOMETER
The band is on the wrist and its 24/7 stream is 1 Hz, where walking is
physically unresolvable. The phone rides in a pocket, sees trunk motion, and
already counts steps into the on-device health store. `PhonePedometer` reads
them via the health package already in pubspec (no new dependency; HealthKit
entitlements and READ_STEPS were already declared). Uses
`getTotalStepsInInterval`, which on iOS is an HKStatisticsQuery cumulative sum,
so HealthKit de-duplicates iPhone/Watch overlap itself.
`live_coverage` gains a `source` column (db v26 -> v27, via the existing
guarded `_addColumnIfMissing`). Phone and band counts are NEVER summed -- they
are the same walk seen from pocket and wrist, so adding them roughly doubles a
day. Phone wins outright when present; band is the fallback. Phone sync is
delete-then-insert scoped to `source='phone'`, so it is idempotent by
construction. Disabling the toggle drops the phone rows, since a stale row
would otherwise keep overriding the band from a source no longer being read.
Steps are no longer written to Apple Health / Health Connect: the old value was
fabricated, and we now READ that store, so writing our copy back would
double-count into it and feed our own number to ourselves. STEPS stays in the
export type list so the per-day delete pass actively PURGES the samples earlier
versions wrote.
MOVEMENT MINUTES -- every change proven on 4 days of real substrate first
* HR GATE DELETED. `restingHr + 8 bpm` changed active minutes by exactly ZERO
on every day tested; at RHR ~62 it sits at ~6% of heart-rate reserve, below
every ACSM band, and 73-100% of covered minutes already cleared it. It also
failed in the wrong direction -- PPG HR is least reliable during the motion
being gated, so a dropout deleted minutes the accelerometer measured fine.
* x3 CEILING DELETED. Rejected ZERO minutes with 0.42-0.55 g of headroom, and
cannot fire on artifacts (a 3 s knock averages ~0.23 g, below the FLOOR).
* FLOOR NOW FROZEN after a 14-day enrollment, persisted in `baselines`. A
floor derived from the signal it thresholds cancels the trend it exists to
report: scaling a real day's dynAmp gave 37 active minutes at 1x, 1.5x, 2x
AND 3x when recomputed, versus 23 -> 254 frozen. Re-freezes only on
device/wrist change, a 30-day wear gap, or 365 days.
* Coverage exclusion dropped from the movement estimate -- it existed only to
stop step double-counting, and there is no longer a step total to double
count into.
REFUTED, deliberately not built: a sleep-anchored floor (CV 138.6% across days
vs 9.3%, and on one night it landed above the entire day's range, which would
report zero); accel autocalibration (+5% gain moves the gate decision by
0.0000); gravity orientation (solved the ambulation problem this deletes).
The 1 Hz accel field is a fused GRAVITY vector, not acceleration -- p50 1.027 g
across 269,486 samples, 1.033 g +- 0.006 during the most vigorous minute of a
day. That is the true root cause of the original gRef collapse.
1118 tests pass. CI reproduced locally against the PINNED analytics (overrides
moved aside) before committing.
📝 WalkthroughWalkthroughAlgorithm version 57 removes estimated steps from 1 Hz movement data. Measured steps come only from band, phone, or imported pedometers. Movement minutes remain a separate wrist-movement metric. Phone coverage, frozen movement floors, health export cleanup, and related UI flows are updated. ChangesMeasured steps and phone coverage
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant ProfileScreen
participant AppState
participant PhonePedometer
participant LocalDb
participant DerivationEngine
ProfileScreen->>AppState: requestPhoneSteps()
AppState->>PhonePedometer: requestPermission()
AppState->>PhonePedometer: syncRecent()
PhonePedometer->>LocalDb: replacePhoneCoverageForDay()
LocalDb-->>DerivationEngine: phone-preferred daily steps
DerivationEngine-->>AppState: measured steps and movement minutes
AppState-->>ProfileScreen: sync status
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
PR Reviewer Guide 🔍(Review updated until commit f97e278)Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Latest suggestions up to f97e278 Previous suggestionsSuggestions up to commit abe614f
Suggestions up to commit f804659
Suggestions up to commit 07ade7f
Suggestions up to commit 87a8a5d
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/compute/derivation_engine.dart (2)
3110-3113: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winA thin band substrate silently drops measured phone steps.
_stepsAndEnergyreturns early whendaySub.length < 60or whenmotion.isEmpty. Both guards protect the 1 Hz movement computation. They now also gate the step assignment at lines 3157-3162, which depends on nothing from the band substrate —liveStepsRealcomes fromlive_coverage.Effect: a day with real phone-pedometer coverage but only a few seconds of band 1 Hz data derives with no step count at all.
_buildWakeDayFeaturesleavesstepsnull on purpose, and the copy-back at lines 4104-4107 only copies non-null values, so nothing recovers it.Assign measured steps before the movement guards.
🛠️ Proposed fix: separate the measured-step write from the movement guards
) { try { + // STEPS FIRST, and independent of the 1 Hz substrate. `liveStepsReal` is + // a real pedometer count from `live_coverage`; it does not need a single + // band sample to be valid. Gating it on the movement guards below made a + // phone-measured day with a thin band capture report no steps at all. + _applyMeasuredSteps(bundle, scMap, liveStepsReal); if (daySub.length < 60) return; final motion = _motionMinutes(daySub); if (motion.isEmpty) return;Then move the
haveRealStepsblock (lines 3157-3176) into that new_applyMeasuredStepshelper and delete it from its current position.Also applies to: 3157-3162
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/compute/derivation_engine.dart` around lines 3110 - 3113, Update _stepsAndEnergy so measured steps from live_coverage are assigned before the daySub.length and motion.isEmpty early returns. Extract the haveRealSteps block into a dedicated _applyMeasuredSteps helper, invoke it before the movement guards, and remove the original step-assignment block from the guarded path.
2370-2375: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale comments describe the removed hybrid estimator.
Line 2370 states steps are "real 100 Hz count + 1 Hz estimate over uncovered minutes". Line 2374 states "Steps are derived FROM this" about
active_min. Both statements are now false, and both contradict the v55/v56 changelog at lines 429-438. The same stale description appears at lines 4097-4103, which still calls the copy-back "the hybrid real-100Hz + 1Hz-estimate count".📝 Proposed comment fix
- // Steps = real 100 Hz count + 1 Hz estimate over uncovered minutes - // (computed in _stepsAndEnergy; never double-counted). + // Steps = REAL measured pedometer count only (band 100 Hz or phone), + // summed from `live_coverage` in _stepsAndEnergy. Null when no + // gait-capable source measured the day — see kAlgoVersion v55. 'steps': sc('steps'), - // Ambulatory minutes — the quantity 1 Hz can actually resolve, and the - // unit public activity guidance uses. Steps are derived FROM this. + // Movement minutes — an explicitly NON-locomotion activity-volume + // index. Never converted to steps (v55/v56). 'active_min': sc('active_min'),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/compute/derivation_engine.dart` around lines 2370 - 2375, Update the stale comments near the activity metric mappings and the copy-back logic to reflect the current v55/v56 behavior: remove references to hybrid 100 Hz plus 1 Hz step estimation and the claim that steps are derived from active_min. Apply the same wording correction to the comments around the relevant copy-back logic, without changing implementation behavior.
🤖 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 `@lib/compute/derivation_engine.dart`:
- Around line 3054-3067: Update the frozen-floor logic around the stored value
and enrollment checks so a persisted floor remains usable when re-freezing is
requested but insufficient history prevents computing a replacement. In the
insufficient-history and null-derived-floor paths, return the stored floor when
available; only return null when no stored floor exists, while preserving
replacement computation when enrollment completes.
- Around line 3049-3058: Update _frozenMovementFloor to obtain and pass the
current deviceChanged, wristChanged, and wearGapDays values to
shouldRefreezeFloor alongside age, preserving the existing stored-floor return
behavior when no refreeze signal is present.
In `@lib/data/db.dart`:
- Around line 885-912: Update or remove the coverage-window contract associated
with coverageWindowsOverlapping to match the current behavior: phone-backed
imported runs must still cover the no-double-count path when existing windows
are read, or delete the method if that path no longer exists. Ensure any
remaining overlap query preserves the required source handling instead of
claiming the obsolete 1 Hz exclusion behavior.
In `@lib/health/health_export.dart`:
- Around line 65-72: Bound the legacy STEPS deletion in exportAll using a
persistent one-shot cursor in the existing sync_cursor mechanism, such as
health_steps_purged_through. Update the per-day delete loop driven by _types so
STEPS is deleted only through the recorded migration boundary, then advance the
cursor after successful purging and skip future STEPS deletes while retaining
HealthDataType.STEPS in _types for request() write scope.
In `@lib/health/phone_pedometer.dart`:
- Around line 59-71: Remove the hasPermission gate from syncRecent so phone-step
reads are attempted even when _health.hasPermissions returns null or false on
Android. Preserve hasPermission for UI display if needed, while allowing the
read operation to enforce actual access and return no data for genuinely
ungranted scopes, matching the convention in health_export.dart.
- Around line 94-99: Make the day and hour iteration in the relevant pedometer
sync methods DST-safe by constructing each local day boundary from calendar
fields rather than adding or subtracting fixed Durations. In the hourly loop,
advance the cursor with the calculated calendar-hour boundary (`to`) and retain
the existing “overshoot” termination behavior so fall-back days do not cross
into the next day and spring-forward days are not skipped. Update `syncRecent`
to derive prior midnights via calendar fields instead of
`midnight.subtract(Duration(days: d))`, remove the now-unused `midnight` value,
and preserve correct `dayLabelOf` results.
- Around line 94-111: Reduce the startup work in the phone pedometer sync path
around syncRecent, syncDay, and syncPhoneSteps so launch-triggered
synchronization only reads today and yesterday instead of issuing hourly
platform reads for the full seven-day window. Preserve the existing seven-day
behavior behind an explicit heavier sync path, or replace the per-hour
getTotalStepsInInterval calls with the supported batched
getHealthIntervalDataFromTypes API using 3600-second intervals.
- Around line 101-118: Update the hourly-read validation in the day coverage
flow containing anyRead and replacePhoneCoverageForDay so elapsed days are
accepted only when every polled hour returns a non-null result. Track
missing/null hour reads separately from successful reads, return null before
replacePhoneCoverageForDay when any expected hour is missing, and preserve the
existing behavior for fully successful reads and all-null days.
In `@lib/state/app_state.dart`:
- Around line 233-236: Update the comment near syncPhoneSteps() to describe the
unawaited call as best-effort initialization rather than claiming it completes
before the first derive sweep. Keep the existing unawaited(syncPhoneSteps())
behavior and do not add awaiting or other ordering changes.
- Around line 427-437: Update disablePhoneSteps so disabling phone steps also
re-derives persisted step values for the recent analysis window after
clearPhoneCoverage(), using the existing _reanalyzeForOverride() mechanism or
equivalent. Keep preference persistence and listener notification intact, and
ensure the behavior is explicit for days outside that window if they are not
re-derived.
- Around line 393-397: Gate the unawaited syncPhoneSteps() call in
healthSyncNow() on the user's phone-steps preference, and add the same
preference guard inside syncPhoneSteps() so disabled phone steps never resync
regardless of caller. Preserve health export and return behavior.
In `@pubspec.yaml`:
- Around line 71-86: Update the openstrap_analytics dependency ref in
pubspec.yaml from the temporary PR-branch commit to the reachable analytics main
merge commit containing dailyActiveMinutes, shouldRefreezeFloor,
enrollmentDaysForFrozenFloor, and personalDynFloorFromDailySummaries. Remove or
revise the temporary pin comments so they no longer instruct a pre-merge re-pin.
In `@test/movement_floor_frozen_test.dart`:
- Around line 69-87: Add regression coverage for
DerivationEngine._frozenMovementFloor using a debug test seam consistent with
debugTargetDayWindow and debugMarkDaySkipped. Test that missing stored floor
with insufficient dyn_p90 history returns null, and that a stale stored floor
with insufficient history preserves and returns the existing floor instead of
discarding it.
In `@test/phone_step_source_test.dart`:
- Around line 16-20: Add a regression test in the existing database test setup
that creates a version-26 database with live_coverage lacking source, inserts a
row, then reopens it at LocalDb.schemaVersion and verifies liveStepsForDay
reports the row as a band count. Reopen the upgraded database a second time and
repeat the assertion to confirm the v27 migration is idempotent, exercising the
oldV < 27 path and _ensureLiveCoverageSource rather than onCreate.
---
Outside diff comments:
In `@lib/compute/derivation_engine.dart`:
- Around line 3110-3113: Update _stepsAndEnergy so measured steps from
live_coverage are assigned before the daySub.length and motion.isEmpty early
returns. Extract the haveRealSteps block into a dedicated _applyMeasuredSteps
helper, invoke it before the movement guards, and remove the original
step-assignment block from the guarded path.
- Around line 2370-2375: Update the stale comments near the activity metric
mappings and the copy-back logic to reflect the current v55/v56 behavior: remove
references to hybrid 100 Hz plus 1 Hz step estimation and the claim that steps
are derived from active_min. Apply the same wording correction to the comments
around the relevant copy-back logic, without changing implementation behavior.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 34ffe1a6-ae0f-43f9-aa7b-3e6ea6f3bf7c
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
lib/compute/derivation_engine.dartlib/data/db.dartlib/health/health_export.dartlib/health/phone_pedometer.dartlib/state/app_state.dartlib/ui/profile/profile_screen.dartpubspec.yamltest/derive_day_window_test.darttest/movement_floor_frozen_test.darttest/phone_step_source_test.darttest/step_personal_floor_test.dart
… bugs CodeRabbit findings on #182. Each verified against the code before fixing; the pin is also advanced to analytics 38a8636 (that PR's review fixes). * A THIN BAND SUBSTRATE SILENTLY DROPPED MEASURED PHONE STEPS. `_stepsAndEnergy` returns early on `daySub.length < 60` or `motion.isEmpty`. Both guards protect the 1 Hz MOVEMENT computation, but the step assignment sat after them — and steps now depend on nothing from the band substrate (`liveStepsReal` comes from `live_coverage`). A day with real phone-pedometer steps but little band data reported no steps at all, discarding a real measurement because an unrelated signal was missing. Extracted to `_writeSteps` and hoisted above both guards. * A RE-FREEZE WITH THIN HISTORY DESTROYED A USABLE FLOOR. When `shouldRefreezeFloor` fired, the method fell through to enrollment and returned null if history was short — so `active_min` vanished for the day even though a perfectly good floor was on disk. Reachable exactly when re-freezing matters most: an old floor on a user whose `dyn_p90` history was pruned or is sparse. Now keeps serving the stored floor until a replacement can actually be computed, including when the recompute itself returns null. * `shouldRefreezeFloor`'s WEAR-GAP TRIGGER WAS DEAD. Only `daysSinceFrozen` was ever passed, so a 30-day wear gap could never thaw the floor and the 365-day ceiling was the sole trigger. Wear gap IS derivable — a run of days with no `dyn_p90` row means the band was not worn — so it is now computed (`_wearGapDays` + `_BaselineHistoryCache.datesFor`) and passed. `deviceChanged`/`wristChanged` remain deliberately unpassed rather than fabricated as `false`, with the reason stated inline. * ANDROID COULD SILENTLY NEVER SYNC. `hasPermission()` treated a null `hasPermissions` result as NO, and `syncRecent` then returned without attempting a read. `health_export.dart` documents the opposite finding from this same codebase — Health Connect "frequently returns null/false even after the user grants everything", which is why the exporter attempts every write and lets the platform enforce. Null is now MAYBE: only an explicit `false` blocks, and a failed probe attempts the read anyway. An ungranted read returns no data, which `syncDay` already treats as unknown, not zero. * DST-UNSAFE DAY AND HOUR ARITHMETIC. `Duration` maths on a local DateTime is absolute, so a fixed 24-iteration `add(Duration(hours: h))` walk covered 25 wall-clock hours on a fall-back day (the last bucket crossed into the next local day and double-counted) and 23 on spring-forward (one hour never queried); and `midnight.subtract(Duration(days: d))` landed on 23:00/01:00 across a transition, mislabelling the day. Both are now calendar-constructed and bounded by the next local midnight. * `healthSyncNow` SYNCED PHONE STEPS AFTER THE USER DISABLED THEM. `disablePhoneSteps` deliberately does not revoke the platform permission, so "Sync now" wrote phone rows straight back — and since `liveStepsForDay` prefers phone rows, it re-suppressed the band count, the exact outcome that method exists to prevent. Now gated on the user's own preference. * Corrected an `_init` comment claiming an ordering guarantee `unawaited` does not provide, and documented the real limit that disabling phone steps leaves already-derived days showing phone-sourced values (screens read persisted scalars, and days past finalization never re-derive). * Added the MISSING v27 MIGRATION TEST. Every existing test opened a fresh DB, so `onCreate` emitted `source` directly and the `oldV < 27` path — the one every real install takes — was never executed. It carries a load-bearing assumption: pre-v27 rows must default to 'band', or existing band counts would read as phone counts and suppress the band fallback. Verified by MUTATION: flipping the default to 'phone' makes the new test fail 137 -> 0. Not changed: the STEPS purge in the health export runs on every pass rather than behind a one-shot cursor (a real but trivial cost, and the purge is worth more than the round trip), and the temporary analytics pin, which is intentional and re-pinned before merge as the pubspec comment states. 1120 tests pass. CI reproduced locally against the pinned analytics.
|
Thanks — this was a strong review. Every finding I checked was real; fixed in The most serious was the one outside the diff: Also fixed: the re-freeze path destroying a usable stored floor; the dead wear-gap trigger (now derived from missing On the missing v27 migration test — you were right that the path every real install takes was untested. Added it, and confirmed it isn't decorative by mutation: flipping the migration default from Two deliberately not changed, with reasons in the commit: the STEPS purge running every pass rather than behind a one-shot cursor (real but trivial cost, and the purge is worth more than the round trip), and the temporary analytics pin — that one is intentional and flagged in both the PR body and a comment directly above the ref; it gets re-pinned to the analytics main SHA before merge. 1120 tests pass; CI reproduced locally against the pinned analytics. |
|
Persistent review updated to latest commit 87a8a5d |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/compute/derivation_engine.dart`:
- Around line 3115-3131: Update _wearGapDays so each candidate label is derived
by calendar-date arithmetic rather than target.subtract(Duration(days: back));
construct the candidate DateTime from normalized year, month, and day fields
(following _adjacentDayIds) before formatting the label. Preserve the existing
60-day search, have.contains check, and gap behavior while avoiding
DST-dependent absolute-duration subtraction.
In `@lib/health/phone_pedometer.dart`:
- Around line 119-128: In the hourly walk around the `capped` guard, skip
zero-length buckets caused by spring-forward normalization instead of breaking
the loop; only break when the current time has been reached. Preserve the
existing `replacePhoneCoverageForDay` behavior while allowing later valid hours
to be queried, without assuming a 24-hour day.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 87b85226-870e-4236-8a25-88edc13ee605
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
lib/compute/derivation_engine.dartlib/health/phone_pedometer.dartlib/state/app_state.dartpubspec.yamltest/phone_step_source_test.dart
Review follow-up. Every change below was checked against the actual code or the actual platform behaviour before being made; two review findings were disproved and are not acted on (see the PR thread). phone_pedometer * A spring-forward day truncated and then PERMANENTLY corrupted its own step count. `DateTime(y,m,d,2)` and `DateTime(y,m,d,3)` resolve to the same instant across the missing local hour, so h=2 is zero-width and the `!capped.isAfter(from)` guard ended the whole walk — hours 3-23 were never queried while `anyRead` was already true, so the day was REPLACED with ~3 hours of windows. Reproduced against the Dart runtime under TZ=America/New_York. Skip the bucket instead of breaking; the "reached now" case is already handled by the `from.isAfter(now)` guard above. * A partial read now abandons the day. `null` from this plugin means the query FAILED, not that the hour was empty — verified in both natives at health 11.1.1 (iOS returns 0 via `steps = 0.0` when `sumQuantity()` is nil; Android returns 0 via `?: 0L`, and null only from the catch). Since `replacePhoneCoverageForDay` is delete-then-insert and phone rows win over band rows, banking a short read lowered a good total and kept the band suppressed. * Routine syncs pull 2 days, not 7. Each hourly bucket is one platform round trip, so the old default was up to 168 sequential calls on every launch and again after every export. The full backfill window still runs on the explicit gestures. * A `stepReader` seam makes the walk testable — `Health` is a private-constructor singleton and cannot be faked otherwise, which is why the two bugs above had no coverage. db / import * `coverageWindowsOverlapping` now filters by source. Its only remaining caller is the NOOP importer's double-bank guard, and phone rows share the table over the same hours — so a user with phone steps enabled importing a NOOP backup had their BAND step runs clipped against the PHONE's windows and silently dropped, the import reporting success while banking nothing. derivation_engine * The frozen floor is one shared scalar resolved by a read-modify-write that a NEWEST-FIRST concurrent sweep reaches from many days at once — and this version bump forces exactly that sweep. Serialized it, clamped `daysSinceFrozen` to >= 0 so a backfill day is not read as stale, and stopped an older day overwriting a newer freeze. Without these, sweep order decided every day's active_min, which is what `_BaselineHistoryCache` already forbids for baselines. * `_wearGapDays` walked back with `Duration`, which skips the spring-forward day entirely (from 2026-03-10 it yields 03-09, 03-07, ... — 03-08 never appears). Calendar fields now, via the shared day_label helper. * Stopped REMOVING `active_min` on abstention. `_applyWakeDayFeatures` had already written it from `_activeMinutes`, a separate quantity never part of the fabricated step conversion; deleting it wiped a number the user had for the whole enrollment window and nulled its trend series. Abstaining from the new index is right, destroying the old measurement to do it is not. * `inputs_used` no longer claims hr_1hz — the HR gate was deleted. health_export * The legacy STEPS purge is a one-shot migration with its own cursor, out of the per-day rewrite loop and out of the day's success accounting. It was running a delete for a type nothing writes on every re-export of the unfinalized tail, forever, and could fail a day's export. honesty (the point of the PR) * The Steps screen still explained the deleted estimator: an "est" tag, an info panel describing hours "ESTIMATED from your walking minutes and cadence", and "Walk with the app open to sharpen the estimate". * "Calibrate steps — Walk ~250 steps with the app open" was still routed and still wrote a `step_calibration` baseline, but its only reader was `dailyStepEstimate`. The user could finish the walk and be told they had calibrated something nothing read. Removed the screen, the route, the app_state methods, the post-session write and the dead db accessors; the Tier-A AN-2554 pedometer never used it. * "no steps yet" became "not measured" — absent means nothing could resolve gait, which is not the same claim as zero. * NSHealthShareUsageDescription said we read samples "to avoid writing duplicates". We now read steps to display them, which is a different purpose than the one the user consented to. app_state * Turning phone steps off now re-derives. Clearing `live_coverage` only changed what future derives compute, so the user kept seeing phone-sourced counts; `setSleepOverride` already handles the equivalent case this way. Scope is bounded by raw retention, not the whole history. * Surfaced the sync result in Profile. On iOS `requestAuthorization` reports success even when READ is denied, so the toggle sat on while nothing ever arrived, with nothing for the user to act on. tests: 1118 -> 1140, analyze clean, run against the pinned analytics with no overrides file present (pubspec.lock untouched).
|
Persistent review updated to latest commit 07ade7f |
Two categories were missed on the first pass: CodeRabbit's "outside diff range" findings live in the review BODY, not the inline-comments API I queried, and I never opened PR Agent's suggestions at all. Both checked against real code now. * The copy-back comment still described "the hybrid real-100Hz + 1Hz-estimate count". I fixed the identical stale wording in the `series:` map last commit and missed this second site, which CodeRabbit had explicitly listed. Rewritten to say what the code does: real pedometer counts only, copied into the early-read `wake` artifact so Today does not show a blank on a day that WAS measured. * `syncDay` captured `DateTime.now()` once before the hour walk. Each bucket is an async platform query, so a full day's walk can straddle an hour boundary and the stale `now` capped the current hour short, under-reporting today's most recent steps until a later sync re-read the day. Re-read per iteration. Already covered, verified rather than assumed: - PR Agent's #1 (backfill regresses the frozen floor, importance 8) is the same defect as CodeRabbit's and my own; `mayCommitFloorOn` is literally the guard it proposed. - Its future-`frozenOn` clock-skew case is closed by the same clamp — a negative difference now returns 0 instead of a large `abs()`. - Its wear-gap DST case is closed by `dayLabelBefore`. - CodeRabbit's outside-diff "thin band substrate drops measured phone steps" was already fixed by the author at 87a8a5d: `_writeSteps` runs before the `daySub.length < 60` guard. NOT fixed, deliberately: both bots flag the analytics pin pointing at an unmerged branch head. That is the documented merge-order dependency — it gets re-pinned to the analytics main SHA after analytics#35 merges, not now. analyze clean, 1140 tests green.
|
Persistent review updated to latest commit f804659 |
No behaviour change. PR Agent read the `mayCommitFloorOn` guard as dead code (claiming the `stored != null` branch always returns early), which is wrong — the branch falls through when a re-freeze IS required and history IS long enough, which is exactly the backfill-clobber case. But two readers misreading it is signal, so the guard now sits inside that branch against a non-nullable `stored.frozenOn`, with the reachable path named in the comment. The other three suggestions on f804659 are refuted, each empirically: * "Async lock swallows action errors" — it does not. `whenComplete` preserves the error for the caller; verified by running both shapes. The proposed `completeError` fix is an ACTIVE regression: the gate future then carries the error, so the next waiter's `prev.then((_) => action())` skips its action entirely and fails with the PREVIOUS day's error, plus an unhandled exception. One day's floor failure would cascade through the whole sweep. Releasing the gate normally while propagating the error to its own caller is the correct mutex contract. * "Fall-back day loses its final hour, raise the bound to 26" — a fall-back day has 25 real hours but only 24 local hour LABELS. The repeat is absorbed by the h=1 bucket, which spans 2 real hours. Measured under TZ=America/New_York: the walk covers 25h of a 25h day and h=24 breaks because the day is already complete. A bound of 26 changes nothing. * "Absent metric key incorrectly asserted present" — the assertion is load-bearing and must stay. `putDayResult` always includes 'steps' in its series map, so a null value writes a NULL row under REPLACE and OVERWRITES a previously fabricated value. Drop the key and the stale v54 number survives. The null does not read as zero: `metricValueOn` returns null (asserted on the next line), trend buckets gate on `has`, and no query coalesces it to 0. analyze clean, 1140 tests green.
|
Persistent review updated to latest commit abe614f |
|
@coderabbitai review |
|
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@lib/compute/derivation_engine.dart`:
- Around line 3097-3105: Replace the isolate-local
_floorLock/_frozenMovementFloor read-modify-write flow with an exclusive
LocalDb.updateBaseline transaction for the movement_floor record. Perform the
get-and-update logic inside that transaction so concurrent derives serialize at
the database level and preserve the newest frozen floor, removing reliance on
_AsyncLock.
In `@lib/compute/movement_floor_policy.dart`:
- Around line 66-71: Update daysSinceFrozen in
lib/compute/movement_floor_policy.dart: construct the parsed dates from their
UTC calendar fields, subtract those UTC calendar dates, then clamp negative
results to zero. Update the related coverage in
test/movement_floor_policy_test.dart at lines 11-18 and 70-86 to verify
calendar-day behavior across daylight-saving boundaries and preserve the
non-negative result contract.
In `@lib/health/health_export.dart`:
- Around line 391-393: Move _purgeLegacyStepsIfNeeded out of _exportDay and
invoke it from exportAll through an independent walk of the retained day window
before applying the export-cursor skip. Ensure the purge runs for
already-exported days during normal syncs without requiring reset: true, while
leaving export success accounting unchanged.
In `@test/app_state_regressions_test.dart`:
- Around line 81-85: Replace the removed step-calibration coverage comment with
a regression test that drives a remaining live-consumer path, such as workout,
spot-check, or breathing, and verifies the `_hasLiveConsumer` latch becomes
asserted through the `debugHasLiveConsumer` observable. Keep the test focused on
preserving live-mode behavior after the consumer activates.
In `@test/phone_pedometer_hour_walk_test.dart`:
- Around line 145-148: Remove the local _label helper and reuse dayLabelOf from
lib/data/day_label.dart wherever the test derives the live_coverage.day key,
updating the relevant assertions to call the shared formatter directly.
🪄 Autofix
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c6c0be4f-ede5-4116-b688-c43680854678
📒 Files selected for processing (16)
ios/Runner/Info.plistlib/compute/derivation_engine.dartlib/compute/movement_floor_policy.dartlib/data/db.dartlib/health/health_export.dartlib/health/phone_pedometer.dartlib/import/noop_import.dartlib/state/app_state.dartlib/ui/profile/profile_screen.dartlib/ui/screens/metric_row.dartlib/ui/screens/screens.dartlib/ui/today/step_calibration_screen.darttest/app_state_regressions_test.darttest/metric_trend_redesign_test.darttest/movement_floor_policy_test.darttest/phone_pedometer_hour_walk_test.dart
💤 Files with no reviewable changes (1)
- lib/ui/today/step_calibration_screen.dart
| static Future<double?> _frozenMovementFloor( | ||
| _BaselineHistoryCache history, | ||
| String dayId, | ||
| ) => | ||
| _floorLock.run(() => _resolveMovementFloor(history, dayId)); | ||
|
|
||
| /// Serializes the shared-floor read-modify-write across concurrent day | ||
| /// workers. See [_frozenMovementFloor]. | ||
| static final _AsyncLock _floorLock = _AsyncLock(); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 8 '_floorLock|_resolveMovementFloor|getMovementFloor|putMovementFloor' \
lib/compute/derivation_engine.dart lib/data/db.dart
rg -n -C 5 'DerivationEngine\(|background:\s*true|derivationDispatcher|Workmanager' \
lib --glob '*.dart'Repository: OpenStrap/edge
Length of output: 24374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== DerivationEngine movement floor implementation =="
sed -n '3080,3175p' lib/compute/derivation_engine.dart
echo
echo "== LocalDb movement floor implementation =="
sed -n '3928,3968p' lib/data/db.dart
echo
echo "== baseline write helpers around transaction patterns =="
sed -n '4010,4065p' lib/data/db.dart
rg -n -C 4 'putBaseline\(|baseline\(|transaction\(|execute|write' lib/data/db.dart | sed -n '1,240p'Repository: OpenStrap/edge
Length of output: 20981
Move movement-floor updates into an exclusive baseline transaction.
_floorLock is isolate-local, while LocalDb.getMovementFloor() reads movement_floor and LocalDb.putMovementFloor() writes it with ConflictAlgorithm.replace outside updateBaseline. Two concurrent derives can complete reads before either commit and an older day can overwrite a newer freeze. Rebuild the movement-floor update as updateBaseline('movement_floor', ...) with an exclusive transaction.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/compute/derivation_engine.dart` around lines 3097 - 3105, Replace the
isolate-local _floorLock/_frozenMovementFloor read-modify-write flow with an
exclusive LocalDb.updateBaseline transaction for the movement_floor record.
Perform the get-and-update logic inside that transaction so concurrent derives
serialize at the database level and preserve the newest frozen floor, removing
reliance on _AsyncLock.
| int daysSinceFrozen({required String frozenOn, required String dayId}) { | ||
| final from = DateTime.tryParse(frozenOn); | ||
| final to = DateTime.tryParse(dayId); | ||
| if (from == null || to == null) return 0; | ||
| final diff = to.difference(from).inDays; | ||
| return diff > 0 ? diff : 0; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 5 'daysSinceFrozen|difference\(from\)\.inDays' \
lib/compute/movement_floor_policy.dart
rg -n -C 5 'spring-forward|daysSinceFrozen|Duration\(days' \
test/movement_floor_policy_test.dart
fd -HI -t f -e yml -e yaml -e sh . |
while IFS= read -r file; do
rg -n 'TZ=|flutter test|movement_floor_policy_test' "$file" || true
doneRepository: OpenStrap/edge
Length of output: 2927
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## movement_floor_policy.dart outline"
ast-grep outline lib/compute/movement_floor_policy.dart || true
echo "## movement_floor_policy.dart relevant sections"
sed -n '1,140p' lib/compute/movement_floor_policy.dart | cat -n
echo "## movement_floor_policy_test.dart relevant sections"
sed -n '1,115p' test/movement_floor_policy_test.dart | cat -n
echo "## timezone config search"
rg -n --hidden --glob '!**/.git/**' 'TZ=|dart test|flutter test|timezone|America/|America/New_York|UTC' . || true
echo "## dart availability"
command -v dart || true
dart --version || trueRepository: OpenStrap/edge
Length of output: 20576
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## search for daysSinceFrozen callers"
rg -n --hidden --glob '!**/.git/**' 'daysSinceFrozen\(' . || true
echo "## test config around movement_floor_policy_test"
sed -n '1,110p' .github/workflows/test.yml | cat -n
sed -n '80,108p' test/day_window_dst_test.dart | cat -n
echo "## inspect day_label implementation"
sed -n '1,120p' lib/data/day_label.dart | cat -n
echo "## deterministic Dart DateTime duration probe from Dart source behavior"
# Probe only if Dart is available so this does not execute repository code.
if command -v dart >/dev/null 2>&1; then
TZ=America/New_York dart - <<'DART' || true
void main() {
final from = DateTime.tryParse('2026-03-01');
final to = DateTime.tryParse('2026-03-08');
final toDsr = DateTime.tryParse('2026-03-09');
print('from=$from zone=${from.timeZoneOffset}');
print('to=$to');
print('difference(${to.difference(from)}) inDays=${to.difference(from).inDays}');
print('toDst=$toDsr');
print('difference=${toDsr.difference(to).inDays}');
print('utc diff=${DateTime.utc(toDsr.year,toDsr.month,toDsr.day).difference(DateTime.utc(to.year,to.month,to.day)).inDays}');
}
DART
else
echo "dart not available; reading Dart docs in next step"
fiRepository: OpenStrap/edge
Length of output: 9822
🌐 Web query:
Dart documentation DateTime inDays Duration days property rounds toward zero example local DST
💡 Result:
In Dart, the Duration.inDays property returns the number of entire days spanned by a Duration object [1][2]. The value is calculated using integer division (~/) of the duration's total microseconds by the number of microseconds in a day [1][3]. This effectively truncates the result toward zero, meaning it returns the number of whole days and discards any remaining time (hours, minutes, etc.) [4][5][6]. For example: - A duration of 4 days and 3 hours will return 4 for inDays [1][2]. - A duration of 88 hours (3 days and 16 hours) will return 3 for inDays [4][6]. Regarding Daylight Saving Time (DST) and DateTime calculations: The Duration class represents a fixed span of time independent of calendar or time zone context [5]. When using DateTime.difference to calculate a duration between two points in local time, the result is the absolute difference in microseconds [7][8]. Because DST changes can cause a day to have 23 or 25 hours rather than 24, calculating the difference between two calendar dates in local time may not yield a whole multiple of 24 hours [9][7]. If a DST transition causes a period to be shorter than a full 24-hour day (e.g., 23 hours), Duration.inDays will return 0 for that period because the time span is less than one complete day [9]. If you need to count calendar days regardless of DST shifts, it is recommended to perform calculations using UTC time instead of local time [9].
Citations:
- 1: https://api.dart.dev/dart-core/Duration/inDays.html
- 2: https://api.flutter.dev/flutter/dart-core/Duration/inDays.html
- 3: https://github.com/dart-lang/sdk/blob/b6f9dbc570c4e9065e922228c9747b4ea8104904/sdk/lib/core/duration.dart
- 4: https://api.dartlang.org/stable/latest/dart-core/Duration-class.html
- 5: https://api.dart.dev/dart-core/Duration-class.html
- 6: https://api.flutter.dev/flutter/dart-core/Duration-class.html
- 7: https://api.dart.dev/dart-core/DateTime/difference.html
- 8: https://api.dart.dev/stable/2.19.0/dart-core/DateTime-class.html
- 9: Duration.InDays() works incorrect (?) dart-lang/sdk#32431
🌐 Web query:
Dart DateTime inDays Duration property rounds toward zero example local DST
💡 Result:
In Dart, the Duration.inDays property retrieves the number of entire days in a duration by performing integer division of the total microseconds by the number of microseconds in a day [1][2]. Consequently, it effectively truncates toward zero—it always rounds down to the nearest whole number (e.g., a duration of 4 days and 23 hours returns 4) [1][2]. Because Duration is a context-independent measurement of elapsed time—where 1 day is strictly defined as 24 hours—it does not account for calendar-based changes like Daylight Saving Time (DST) [2][3][4]. When you use DateTime.difference on local times that cross a DST transition, the resulting Duration may not contain a multiple of 24 hours, leading to behavior that can appear "incorrect" if you are expecting calendar days [5][6][4]. Example of DST impact on inDays: If you calculate the difference between two midnights in a local time zone that transitions to DST, the elapsed time might be 23 hours instead of 24. Even though it is "one calendar day" apart, Duration.inDays will return 0 because it has not yet reached the 24-hour threshold [5][4][7]. Example of rounding: const duration = Duration(hours: 47); print(duration.inDays); // Output: 1 (47 hours / 24 = 1.958, rounded down to 1) [1][2] To perform calendar-aware arithmetic that respects DST or standard calendar days, it is recommended to use UTC time to avoid local zone shifts or to manually handle day increments using the DateTime constructor [5][6][3][8].
Citations:
- 1: https://api.dart.dev/dart-core/Duration/inDays.html
- 2: https://api.dartlang.org/stable/latest/dart-core/Duration-class.html
- 3: DateTime add(Duration(days: 1)) does not consider DST, but difference() does dart-lang/sdk#47666
- 4: Incorrect difference.inDays value after time change dart-lang/language#3692
- 5: https://api.dart.dev/dart-core/DateTime/difference.html
- 6: Duration.InDays() works incorrect (?) dart-lang/sdk#32431
- 7: https://api.dart.dev/dart-core/DateTime-class.html
- 8: DateTime .add(Duration(days: 1)) fails to give the correct response when the specific day has 23/25 hours like when transitioning to or from DST dart-lang/sdk#48744
Use UTC calendar fields for daysSinceFrozen.
daysSinceFrozen uses to.difference(from).inDays on local-midnight instants. Near spring-forward, one calendar day can be 23 hours, so inDays reports 0 and delays the re-freeze age check. Subtract the constructed UTC calendar days before clamping at zero. The caller from DerivationEngine.run() reads this age during newest-first sweeps.
Proposed fix
- final diff = to.difference(from).inDays;
+ final fromDay = DateTime.utc(from.year, from.month, from.day);
+ final toDay = DateTime.utc(to.year, to.month, to.day);
+ final diff = toDay.difference(fromDay).inDays;
return diff > 0 ? diff : 0;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| int daysSinceFrozen({required String frozenOn, required String dayId}) { | |
| final from = DateTime.tryParse(frozenOn); | |
| final to = DateTime.tryParse(dayId); | |
| if (from == null || to == null) return 0; | |
| final diff = to.difference(from).inDays; | |
| return diff > 0 ? diff : 0; | |
| int daysSinceFrozen({required String frozenOn, required String dayId}) { | |
| final from = DateTime.tryParse(frozenOn); | |
| final to = DateTime.tryParse(dayId); | |
| if (from == null || to == null) return 0; | |
| final fromDay = DateTime.utc(from.year, from.month, from.day); | |
| final toDay = DateTime.utc(to.year, to.month, to.day); | |
| final diff = toDay.difference(fromDay).inDays; | |
| return diff > 0 ? diff : 0; |
📍 Affects 2 files
lib/compute/movement_floor_policy.dart#L66-L71(this comment)test/movement_floor_policy_test.dart#L11-L18test/movement_floor_policy_test.dart#L70-L86
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/compute/movement_floor_policy.dart` around lines 66 - 71, Update
daysSinceFrozen in lib/compute/movement_floor_policy.dart: construct the parsed
dates from their UTC calendar fields, subtract those UTC calendar dates, then
clamp negative results to zero. Update the related coverage in
test/movement_floor_policy_test.dart at lines 11-18 and 70-86 to verify
calendar-day behavior across daylight-saving boundaries and preserve the
non-negative result contract.
Source: Coding guidelines
| // One-shot cleanup of the fabricated step samples earlier versions wrote. | ||
| // Outside the success accounting on purpose — see the method doc. | ||
| await _purgeLegacyStepsIfNeeded(date, dayStart, dayEnd); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the purge is unreachable for days inside the export cursor prefix.
set -euo pipefail
echo "== exportAll: cursor skip vs _exportDay call order =="
rg -n -C 4 'health_export_through|compareTo\(cursor\)|_exportDay\(' lib/health/health_export.dart
echo
echo "== every call site of exportAll (does anything pass reset: true?) =="
rg -nP --type=dart -C 3 '\bexportAll\s*\(' lib
echo
echo "== _purgeLegacyStepsIfNeeded call sites =="
rg -nP --type=dart -C 3 '_purgeLegacyStepsIfNeeded' libRepository: OpenStrap/edge
Length of output: 9176
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== relevant health_export.dart sections =="
sed -n '1,130p' lib/health/health_export.dart
sed -n '260,410p' lib/health/health_export.dart
echo
echo "== localMidnight implementation =="
rg -nP --type=dart -C 4 '"_localMidnight"|localMidnight|function _localMidnight|_localMidnight\s*\(' lib/health/health_export.dart
echo
echo "== rewrite types header/context =="
sed -n '45,85p' lib/health/health_export.dartRepository: OpenStrap/edge
Length of output: 15513
Drive the legacy STEPS purge over the retained days, not through _exportDay.
exportAll() skips each row with date.compareTo(cursor) <= 0 before _exportDay() can run, and normal sync calls do not pass reset: true. Because the fabricated STEPS samples were written on already-exported days, the purge inside _exportDay() never visits those rows. Move the purge to its own independent walk over the retained day window before the export-cursor skip. [low_effort_and_HIGH_reward]
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/health/health_export.dart` around lines 391 - 393, Move
_purgeLegacyStepsIfNeeded out of _exportDay and invoke it from exportAll through
an independent walk of the retained day window before applying the export-cursor
skip. Ensure the purge runs for already-exported days during normal syncs
without requiring reset: true, while leaving export success accounting
unchanged.
| // ── 5. (removed) the step-calibration live-consumer latch ───────────────── | ||
| // The guided calibration walk was deleted in v56 along with the 1 Hz step | ||
| // estimator that was its only consumer, so there is no longer an arming path | ||
| // that can latch `_hasLiveConsumer`. The spot-check and workout consumers | ||
| // keep their own latch coverage. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find remaining coverage for the live-consumer latch.
set -euo pipefail
rg -nP --type=dart -C 5 'debugHasLiveConsumer|_hasLiveConsumer|_maybeDowngradeLiveForBackground' test libRepository: OpenStrap/edge
Length of output: 6404
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Dart test files =="
git ls-files 'test/*.dart' 'test/**/*.dart' | sed -n '1,120p'
echo
echo "== debugHasLiveConsumer occurrences =="
rg -n 'debugHasLiveConsumer|hasLiveConsumer|_hasLiveConsumer|_maybeDowngradeLiveForBackground|enableHrOnlyLive|enableLiveStreams' test lib --glob '*.dart' || true
echo
echo "== spot/activity/workspace tests likely exercising latches =="
rg -n "spotActive|breathingActive|activeWorkout|openSession|backgroundActive|debug" test --glob '*.dart' | sed -n '1,220p'Repository: OpenStrap/edge
Length of output: 13884
Add regression coverage to keep _hasLiveConsumer asserted.
debugHasLiveConsumer is only referenced in this comment, while _hasLiveConsumer still controls background live-mode downgrade. If the removed live-consumer path should not disappear from coverage, add a regression for the remaining latch (workout, spot-check, or breathing).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/app_state_regressions_test.dart` around lines 81 - 85, Replace the
removed step-calibration coverage comment with a regression test that drives a
remaining live-consumer path, such as workout, spot-check, or breathing, and
verifies the `_hasLiveConsumer` latch becomes asserted through the
`debugHasLiveConsumer` observable. Keep the test focused on preserving live-mode
behavior after the consumer activates.
| String _label(DateTime d) => | ||
| '${d.year.toString().padLeft(4, '0')}-' | ||
| '${d.month.toString().padLeft(2, '0')}-' | ||
| '${d.day.toString().padLeft(2, '0')}'; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Reuse dayLabelOf instead of reimplementing the day label.
syncDay derives its live_coverage.day key with dayLabelOf from lib/data/day_label.dart. This local _label duplicates that formatting. If dayLabelOf changes, these assertions read a different key than the code under test writes, and the tests pass or fail for the wrong reason.
♻️ Proposed refactor
+import 'package:openstrap_edge/data/day_label.dart';
import 'package:openstrap_edge/data/db.dart';-String _label(DateTime d) =>
- '${d.year.toString().padLeft(4, '0')}-'
- '${d.month.toString().padLeft(2, '0')}-'
- '${d.day.toString().padLeft(2, '0')}';
+String _label(DateTime d) => dayLabelOf(d);Or call dayLabelOf directly at each assertion and drop the helper.
As per coding guidelines: "Use todayLabel() or dayLabelOf() from data/day_label.dart for local day labels".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| String _label(DateTime d) => | |
| '${d.year.toString().padLeft(4, '0')}-' | |
| '${d.month.toString().padLeft(2, '0')}-' | |
| '${d.day.toString().padLeft(2, '0')}'; | |
| import 'package:openstrap_edge/data/day_label.dart'; | |
| import 'package:openstrap_edge/data/db.dart'; | |
| String _label(DateTime d) => dayLabelOf(d); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/phone_pedometer_hour_walk_test.dart` around lines 145 - 148, Remove the
local _label helper and reuse dayLabelOf from lib/data/day_label.dart wherever
the test derives the live_coverage.day key, updating the relevant assertions to
call the shared formatter directly.
Source: Coding guidelines
Four findings from the latest bot round, each checked against the code
before touching it.
The one with teeth: an all-zero phone read wiped a day that already held
real counts. `replacePhoneCoverageForDay` is delete-then-insert, and this
file already documents the failure that produces exactly that read — on
iOS `requestAuthorization` reports success even when the user denied
READ, so queries return EMPTY rather than null, forever. One sync after
that and a multi-thousand-step day was gone, without even the band
fallback, since phone rows win outright. Now an all-zero read over a day
that already holds phone steps keeps the banked day and reports the day
as unread, which is what it is. Mutation-verified: the new test fails
137 -> 0 with the guard stubbed out.
The absent `steps` block carried `tier: 'ESTIMATE'`, and `Metric.parse`
turns that tier into `beta: true` — so a day nothing measured rendered
the estimate badge, on a card with no number on it. `tier: null` and an
empty `inputs_used` instead. `ABSENT` was deliberately not invented as a
fifth tier: `Tier.all` in analytics is a closed set of four published
grades and the edge must not widen it. kAlgoVersion 56 -> 57, since the
persisted bundle changes even though no value does.
`requestPhoneSteps`/`disablePhoneSteps` set the in-memory flag before
awaiting the pref write, so a failed write left the toggle ON for the
run and OFF on the next launch — banking phone rows the restored state
says were never enabled, which nothing then clears. Persist first.
The `containsKey('steps')` assertion in derive_day_window_test was
vacuous: `got` is built by the test's own helper, which seeds every key
unconditionally, so it could not fail whatever the derivation did.
Replaced with real assertions on the persisted bundle.
Not changed, deliberately:
* `_AsyncLock` "swallows the error". `whenComplete` DOES release the
lock on the error path and the error still propagates to the caller.
The suggested `completeError` would be a real bug: `_tail` would then
carry an error, and the NEXT waiter's `previous.then(...)` would skip
its action entirely and inherit an unrelated failure.
* Hour walk `h < 25` -> `h < 26`. `DateTime(y, m, d, 24)` normalises to
`nextMidnight` in every timezone, so h=24 and h=25 both break; 26
changes nothing. The extra fall-back hour is already covered — bucket
[DateTime(y,m,d,1), DateTime(y,m,d,2)) spans BOTH occurrences of 1am,
and the walk runs to nextMidnight.
* SnackBar -> `NotificationCenter.emit`. The one-emitter rule is about
NotificationService system alerts; this is an in-app SnackBar in
direct response to a tap, the pattern used in 13 other UI files.
* Floor commit on the wrong day. `mayCommitFloorOn` is
`dayId >= frozenOn`, and `stored` is re-read inside the lock, so a
backfill day older than a freeze a newer day just wrote is rejected.
* Concurrent `syncPhoneSteps`. Per day it is ONE transaction, so two
overlapping syncs cannot interleave a delete past an insert; a
re-entrancy latch would only silently drop the 7-day backfill.
* The analytics pin, which stays on analytics#35 until that merges.
|
Checked all of these against the code before touching anything — 4 real, 6 refuted. Fixed in The one with teeth was the all-zero phone read. Absent steps no longer label themselves. Also fixed: Refuted, with reasons:
|
|
Persistent review updated to latest commit f97e278 |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
lib/compute/derivation_engine.dart (1)
3106-3181: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftThe movement-floor read-modify-write is still not serialized across isolates.
_frozenMovementFloorserializes concurrent access with_floorLock, astatic final _AsyncLock. Dart gives each isolate its own independent copy of every static field, so this lock only serializes callers inside ONE isolate.derivationDispatcherbuilds a separateDerivationEnginein its own background WorkManager isolate, so a background heavy pass and a foreground sweep each hold a different_floorLockinstance. Both can readLocalDb.getMovementFloor()before either writes, and both can then callLocalDb.putMovementFloor()— the second commit silently overwrites the first underConflictAlgorithm.replace.This file already fixes the identical bug class for
sleep_user_profilein_foldObservationIntoProfile, usingLocalDb.updateBaseline()(an exclusive SQLite transaction, which is cross-connection and therefore cross-isolate) instead of a Dart lock, with a comment explaining exactly why astaticlock cannot do this job. The same fix has not been applied to the movement floor.Move the read-decide-write in
_resolveMovementFloorintoLocalDb.updateBaseline('movement_floor', ...)so the decision is made and committed inside one exclusive transaction, the same way the sleep profile fold works. NoteupdateBaseline's transform returnsvoid/no-value-back-to-caller today, so this needs either a follow-up read of the committed value, or a small extension toupdateBaselinethat lets the transform return the value to use downstream.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/compute/derivation_engine.dart` around lines 3106 - 3181, Replace the isolate-local _floorLock protection around _resolveMovementFloor with an atomic LocalDb.updateBaseline('movement_floor', ...) transaction, keeping the read, refreeze decision, fallback handling, and conditional commit inside the transaction so concurrent isolates cannot overwrite each other. Extend updateBaseline or perform a committed follow-up read as needed to return the effective floor to callers, and retain the existing behavior for enrollment, insufficient history, and backfill days.
🤖 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.
Duplicate comments:
In `@lib/compute/derivation_engine.dart`:
- Around line 3106-3181: Replace the isolate-local _floorLock protection around
_resolveMovementFloor with an atomic LocalDb.updateBaseline('movement_floor',
...) transaction, keeping the read, refreeze decision, fallback handling, and
conditional commit inside the transaction so concurrent isolates cannot
overwrite each other. Extend updateBaseline or perform a committed follow-up
read as needed to return the effective floor to callers, and retain the existing
behavior for enrollment, insufficient history, and backfill days.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3dff926c-b36c-4fcd-98ce-dc3f7ce5da3b
📒 Files selected for processing (6)
lib/compute/derivation_engine.dartlib/data/db.dartlib/health/phone_pedometer.dartlib/state/app_state.darttest/derive_day_window_test.darttest/phone_pedometer_hour_walk_test.dart
1 participant
User description
Why
A user reported 2,645 steps on a day they took under 400. Diagnosed on their real DB: 81.5% of every step figure this app has ever shown was manufactured — 41,753 of 51,225 steps across 26 days — by a 1 Hz estimator that physically cannot see gait.
7 of 26 days now correctly show nothing. They fill in with real counts once phone steps are enabled.
Steps are real-measured only (
kAlgoVersion54 → 56)scalars.stepsis absent, not 0, unless a gait-capable source measured the day. The old hybrid persisted a hard0.0whenever the estimator abstained, which poisoned thedyn_p90median and dragged every average and "most steps" record down.Phone pedometer
The band is on the wrist and its 24/7 stream is 1 Hz, where walking is unresolvable. The phone rides in a pocket, sees trunk motion, and already counts steps into the on-device health store.
healthwas already in pubspec, with HealthKit entitlements,NSHealthShareUsageDescriptionandREAD_STEPSalready declared.getTotalStepsInInterval, which on iOS is anHKStatisticsQuerycumulative sum — HealthKit de-duplicates iPhone/Watch overlap itself, which a raw sample read would not.live_coveragegains asourcecolumn (db v26 → v27, via the existing guarded_addColumnIfMissing). Phone and band counts are never summed — they're the same walk seen from pocket and wrist, so adding them roughly doubles a day. Phone wins outright when present; band is the fallback. Sync is delete-then-insert scoped tosource='phone', idempotent by construction. Disabling the toggle drops the phone rows, since a stale row would otherwise keep overriding the band from a source no longer being read.Steps are no longer written to Apple Health / Health Connect. The old value was fabricated, and we now read that store — writing our copy back would double-count into it and feed our own number back to us.
STEPSstays in the export type list so the per-day delete pass actively purges the samples earlier versions wrote.Movement minutes — every change proven on real substrate first
baselinesA recomputed floor reports the same number whether the user tripled their activity or did nothing. Re-freezes only on device/wrist change, a 30-day wear gap, or 365 days.
Refuted — deliberately not built
Root cause, finally
The R24 1 Hz accel field is a fused gravity vector, not acceleration — p50 1.027 g across 269,486 samples; 1.033 g ± 0.006 during the most vigorous minute of a day, 0 of 420 samples above 1.2 g. So ENMO over this substrate reduces to
~(1.03 − gRef): a pure calibration artifact with zero signal. That is why the old estimator gave 42,155 steps at gRef 0.97 and 0 at 1.02.Caveat
All measurements rest on 4 days of per-minute substrate (the export's retention limit). The direction of each result is unambiguous; the CV figures are thin.
Test plan
flutter analyzecleanflutter test --concurrency=1→ 1118 passingpubspec_overrides.yamlmoved aside,resolved-refverified) before committing — analyze + full suite both greenpubspec.lockdiff verified to be only the pin change, nopath:pollution🤖 Generated with Claude Code
PR Type
Bug fix, Enhancement, Tests
Description
Steps are now real-measured only: the fabricated 1 Hz wrist estimate is deleted;
scalars.stepsis absent (not zero) unless a gait-capable source (band 100 Hz or phone pedometer) measured the dayNew
PhonePedometerreads hourly step counts from HealthKit/Health Connect intolive_coveragewithsource='phone'; phone wins over band when present, the two are never summedkAlgoVersionbumped 54 → 56; schema bumped 25 → 27 (live_coverage.sourcecolumn added, frozen movement floor stored inbaselinetable)Movement minutes rebuilt as a non-locomotion activity index with a frozen personal floor; fabricated steps purged from Apple Health/Health Connect export
Diagram Walkthrough
File Walkthrough
3 files
Delete 1 Hz step estimate; bump kAlgoVersion 54→56; freeze movementfloorSchema v26→v27: add live_coverage.source, phone CRUD, frozen floorstorageStop writing fabricated steps; purge old STEPS samples from healthstore3 files
New PhonePedometer reads hourly steps from on-device health storeWire PhonePedometer: enable/disable toggle, sync on launch and afterexportAdd phone step count toggle to profile health section4 files
Update test: no-gait-source day must have null steps, not a fabricatedvalueNew tests: frozen movement floor round-trip, thaw policy, degeneraterejectionNew tests: phone/band source priority, idempotency, clear-and-fallbackUpdate calls from dailyStepEstimate to dailyActiveMinutes; fixassertions1 files
Temporary PR pin to analytics#35 HEAD (00a2efe); must re-pin beforemergeSummary by CodeRabbit
New Features
Improvements
Bug Fixes