naps + sleep need: one detector, today-scoped inputs, disclosed adjustments (kAlgoVersion 56) - #204
naps + sleep need: one detector, today-scoped inputs, disclosed adjustments (kAlgoVersion 56)#204svssathvik7 wants to merge 6 commits into
Conversation
PIN NOT YET UPDATED. pubspec.yaml still points analytics at f0d1153, which does NOT contain the new nap detector; local builds resolve it via the gitignored pubspec_overrides.yaml. Re-pin to the analytics nap commit (locally 54ba3c6) before this ships — AGENTS §3.5, and the v43 changelog that described an analytics fix its pin never contained, leaving the bug live three releases. Requires analytics `sleep/nap.dart`: naps were "detected" by the NOCTURNAL detector, which rejects them by design, so the 20–45 min nap was structurally undetectable. Edge side: ONE NAP SOURCE (§3.8). `_sleepPeriods` ran its own second detector — 20-min runs of still, on-wrist minutes — beside `detectNaps`. They disagreed on real days: the committed payload.json has a 21-minute period from one and `naps.count: 0` from the other, feeding the Sleep-periods screen and the Timeline respectively. Naps are now passed in, not re-derived. THE SCREEN NEVER WORKED. sleep_periods_screen read onset_ts/wake_ts/ duration_min/efficiency/confidence/stages/hypnogram; the producer wrote start/end/asleep_min. Every nap card rendered "0m" with a red low-confidence dot no matter what was detected. Periods now speak that contract, and duration_min is minutes ASLEEP for the main sleep and naps alike — they were different units under one label, and then summed. nap_min IS TST, NOT TIME IN BED, and is subtracted 1:1 from sleep need, so crediting in-bed minutes over-credited every nap by its awake time and always erred toward recommending LESS sleep than the user needs. WRIST_OFF/WRIST_ON and CHARGING_ON/OFF now reach the detector, from band_events. A band on a table or a charger is motionless and reads as deep rest — the dominant nap false positive. These events have been decoded and persisted all along, and detectSleep has always taken a `wristOff` argument; nothing ever supplied one (§4.7). ABSENT IS NOT ZERO (§3.3/§4.1). When nap detection cannot judge a day, nap_min is left UNWRITTEN and the naps block carries a null value. The sleep-need credit reads TODAY only, via an explicit `is_today` stamp — it previously fell through `_lastNum` to YESTERDAY's nap minutes, and taking the last record positionally is also yesterday on any day not yet derived. total_asleep_min is null when any component is unknown, instead of a confident total short by the unmeasured part. A period with no asleep minutes renders "—", and an unknown confidence draws no dot rather than a red one. THE CREDIT IS DISCLOSED. sleep_coach.nap_credit_min carries the reduction that was actually APPLIED — sleepNeed clamps to [6h, 11h] after subtracting, so the raw nap minutes are not always what came off — and the coach card shows it instead of silently shrinking both the need and the "% of need" ring. Main-sleep TST/efficiency are carried into the day-blocks isolate. It builds its own scMap seeded with rhr alone, so reading scMap['tst_min'] there is null forever, which would have made every main-sleep card read "—". Efficiency is normalized from the stored percent to the 0..1 the card wants. The hypnogram is attached at the read seam instead, the first point holding the whole bundle. Tests: test/nap_credit_test.dart pins the today-scoping and the applied-credit disclosure, each verified to FAIL against the pre-fix code (need dropped to 22628s from 28028s — exactly 5400s, yesterday's nap). Note test/workout_reliability_test.dart has one PRE-EXISTING failure, confirmed identical on clean main via a worktree; it is unrelated to this change.
v55 fixed `nap_min`'s cross-day read and left the other today-scoped input to
`sleepNeed` two lines above it still going through `_lastNum`, which walks
BACKWARD through the oldest-first day records and returns the last non-null.
On any day whose strain compute abstained, tonight's strain bonus was therefore
built from an EARLIER day's workout — the identical §3.3 imputation, in the
identical function. Measured on a 7-day fixture: a carried strain of 18
inflated `need_sec` by 2314 s (38.6 min).
The direction is the OPPOSITE of the nap bug, and that difference is the whole
reason this needs more than a one-word substitution. Naps are SUBTRACTED and
strain is ADDED, so while both inputs floor at 0, that floor is an upper bound
on need for naps and a LOWER bound for strain. Abstaining to 0 strain
recommends up to 45 min LESS sleep — it is NOT the cautious direction, and the
"no credit is the safe direction" reasoning that justified the nap fix does not
transfer. It is still correct, on two other grounds:
- Carrying yesterday forward is not a safety margin either. It inflates need
only when yesterday happened to be harder than today and deflates it when
yesterday was a rest day — noise around the true value, not a conservative
bound, and forbidden regardless.
- Strain is a same-day ACCUMULATING quantity that starts at 0 and only rises.
Before today logs anything, 0 is where it genuinely sits, not a substituted
default.
Making `need` abstain entirely was rejected: there is no `is_today` row until
today derives, so the whole Sleep Coach card — need, performance, bedtime,
wake — would blank every morning, and a false empty state is itself a §4.1 bug
pattern.
Because 0 is not the cautious direction, it is not allowed to be silent:
- `sleep_coach.strain_bonus_min` reports the minutes the bonus ACTUALLY
added, measured like `nap_credit_min` (re-run with strain zeroed, then
diff), so the [6 h, 11 h] clamp cannot make the card claim an increase
`need_sec` never took.
- It is NULL, never 0, when today produced no strain reading. A confident 0
says "you rested"; null says "we could not measure today's strain, so
tonight's need is short by up to 45 min". Collapsing those would re-hide
exactly what the today-scoping fix exposed.
- The Sleep Coach card renders the applied bonus via `strainBonusCaption`,
mirroring the nap credit. It stays SILENT on null, matching that
precedent — surfacing "today's strain was not measured" is a product
decision, and the bundle now carries the distinction for whoever takes it.
12 tests, each watched fail first. The absent-reading test asserts the KEY is
present before asserting the value is null, so it cannot pass against a bundle
that never emitted the field — that guard fired during the red run. One test
pins that the strain-18 bonus is fully realized for the default fixture, so the
regression above cannot pass because the clamp flattened everything.
nap_credit_test's fixture stamped `strain` on every row including today's.
Under `_lastNum` that cancelled between the two series it compares; once strain
is today-scoped it no longer does, so the fixture was coupling an unrelated
input into the nap assertions. Removed — the strain path has its own file now.
kAlgoVersion 55 -> 56 so affected days re-derive; `need_sec` changes and
`day_result` rows are immutable per version.
|
Warning Review limit reached
Next review available in: 21 seconds You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe update unifies analytics nap detection, excludes off-wrist and charging intervals, scopes sleep-coach inputs to today, exposes applied adjustments, enriches main sleep data, and preserves unknown sleep values in the UI. Regression tests cover nap attribution, freshness, credits, bonuses, and legacy payloads. ChangesSleep insights pipeline
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant DayBlocks
participant LocalDb
participant detectNaps
participant SleepPeriods
participant CrossdayPipeline
participant CoachCards
DayBlocks->>LocalDb: Load wrist-off and charging spans
DayBlocks->>detectNaps: Detect naps with exclusions
detectNaps-->>DayBlocks: Return detected nap periods
DayBlocks->>SleepPeriods: Build shared sleep-period records
CrossdayPipeline->>CoachCards: Provide applied nap and strain adjustments
CoachCards-->>CrossdayPipeline: Render applicable captions
🚥 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 |
svssathvik7
changed the title
kAlgoVersion 55 and 56 both cite the new nap detector as their sibling change, and until this commit neither was backed by the pin. pubspec.yaml still pointed at OpenStrap#34 (f0d1153), which has no `sleep/nap.dart`, no `wristOff:`/`exclude:` on `detectNaps`, and no `tstSec`/`tibSec`/`efficiency` on `NapWindow`. This is the §3.5 failure mode the v43 changelog is remembered for, except worse in kind: v43 shipped a changelog describing a fix its pin merely lacked, while this branch did not COMPILE against its own pin — 10 analyzer errors in derivation_engine.dart. It went unnoticed because pubspec_overrides.yaml is gitignored and resolves both siblings to local working copies, so every local build and test run silently used analytics HEAD rather than the pinned SHA. Confirmed by moving the override aside and running `flutter pub get` against the real pin. Verified present at c3a30be, per §3.5: git show c3a30be:lib/src/onehz/sleep/nap.dart | grep -cE 'wristOff|exclude|tibSec' -> 16 git show c3a30be:lib/src/onehz/sleep/van_hees.dart | grep -c immobilityMask -> 2 pubspec.lock regenerated with the override moved aside, so it locks the git SHA rather than `path: ../analytics`. A path-source lock fails CI `flutter pub get` (exit 66) and is the reason that file must never be regenerated with the override in place. No kAlgoVersion bump: 56 is already the version describing this analytics behaviour, and it has not shipped. The pin and the version now land together, which is the whole point. Against the real pin: flutter analyze lib/ clean, 1150 tests pass. The single failure is workout_reliability_test.dart's queued-job case, pre-existing and identical on clean main.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/crossday_pipeline.dart`:
- Around line 463-468: Update _todayNum in
lib/compute/crossday_pipeline.dart:463-468 to require the last record’s date to
match todayLabel() or dayLabelOf() for the local day, using date as
authoritative while retaining the is_today stamp check if desired; do not derive
the label from UTC strings. Keep is_today persisted in
lib/compute/derivation_engine.dart:2759-2763 for readability, with no direct
change required there. Add a regression test covering a stale is_today stamp and
verify it is not used for today’s strain or nap values.
In `@lib/data/local_repository_impl.dart`:
- Around line 608-618: Update the main-period data assembled by _sleepPeriods to
include the existing sleepConf value as its confidence field before passing it
to _periodsWithMainStages. Preserve the current stage-minute and hypnogram
enrichment, and leave nap confidence behavior unchanged.
In `@lib/ui/insights/coach_cards.dart`:
- Around line 190-197: Extract the inline nap caption logic into a documented
public napCreditCaption function beside strainBonusCaption, accepting num?,
rounding the applied credit, and returning null for null or non-positive values.
Reuse this function at the napLine call site so wording, rounding, and guarding
are centralized and testable.
🪄 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: 1c9c3b8a-e687-4c25-a227-f4e0f630d3c5
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
lib/compute/crossday_pipeline.dartlib/compute/derivation_engine.dartlib/data/db.dartlib/data/local_repository_impl.dartlib/ui/insights/coach_cards.dartlib/ui/sleep/sleep_periods_screen.dartpubspec.yamltest/nap_credit_test.darttest/strain_bonus_test.dart
Addresses the review on OpenStrap#204. `is_today` OUTLIVING ITS DAY — the real find, and it reopened this PR's own bug through a different door. `_refreshCrossDayInputArtifact` stamps `is_today: true` on the most recent record so `_todayNum` can tell "today abstained" from "today has no row yet". That is a fact ABOUT A DAY stored as a bare boolean, and it goes into the DURABLE `crossday_input` baseline row. `_crossDayInputDays()` then preferred that cache whenever it merely PARSED — no day check, no algo_version check. A cache written yesterday hands back a series whose last record still claims to be today, so `_todayNum` reports yesterday's strain and nap minutes as today's and they land inside `need_sec`: exactly the imputation this PR removes, arriving through the cache instead of `_lastNum`. NOT reachable today, and I checked before deciding how to fix it: all four `_runCrossDay` call sites refresh the artifact immediately beforehand, and the two conditional ones (`if (done > 0)`) skip both together. But that is an unenforced ordering coincidence, not a guarantee — one new caller, or one early return inside `_refreshBaselines`, makes it live and silent. A day-relative fact should not depend on call ordering to stay true. The envelope now carries `built_for_day`, and `crossDayArtifactUsableToday` is the only thing allowed to declare a cached artifact reusable. Pure and static so it is unit-testable without a database, which the seam that consumes it is not. An artifact with no `built_for_day` — anything written before this field — cannot be SHOWN to be fresh, so it is rebuilt rather than assumed fresh. Costs nothing in the normal path, since the refresh already runs first. MAIN SLEEP HAD NO CONFIDENCE DOT. `_periodsWithMainStages` enriches the main period with the hypnogram and stage minutes but emitted no `confidence`, while every nap carries one, and sleep_periods_screen draws a ConfDot for any period that has one. So the best-evidenced period on the screen was the only one rendering as unknown. `sleep.accounting.confidence` was already in hand two lines above. Null stays null and correctly draws nothing. NAP CAPTION EXTRACTED to `napCreditCaption`, mirroring `strainBonusCaption`. The strain line was extracted and unit-tested when it was added; leaving its twin inline meant the two disclosures on the same card were built and covered differently for no reason. Same silence rule, now pinned by tests. flutter analyze lib/ test/ clean; 1158 tests pass (was 1150). The single failure is workout_reliability_test.dart's queued-job case, pre-existing and identical on clean main.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/compute/derivation_engine.dart (1)
3889-3895: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftAdd direct nap-detector regression tests.
test/nap_credit_test.dartcovers cross-day credit calculations. It does not exercise_attachNapsorana.detectNaps. Add fixtures for wrist-off exclusion, charging exclusion, and detector abstention. Assert excluded spans produce no credited nap and abstention leavesnap_minunwritten.🤖 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 3889 - 3895, Add direct regression tests for ana.detectNaps and the _attachNaps flow, covering wrist-off exclusion, charging exclusion, and detector abstention. Verify excluded spans produce no credited nap, while abstention leaves nap_min unwritten; use fixtures consistent with the existing tests in test/nap_credit_test.dart.Source: Coding guidelines
🤖 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`:
- Line 2741: Replace both LocalDb.localDayLabelNow() calls in the derivation
flow, including the crossDayArtifactUsableToday check and artifact stamp at the
referenced locations, with the shared todayLabel() helper. Keep the existing
cache-gate and artifact-stamping behavior unchanged.
In `@lib/data/local_repository_impl.dart`:
- Around line 613-627: The sleep summary flow around _sleepPeriods must not
return early with has_sleep: false when TST is null if a valid main period or
sleep window exists. Update the relevant guard to treat that period/window as
sleep evidence, preserve nullable duration and debt fields, and still return the
enriched periods including mainConfidence. Add a regression test covering an
unknown-TST main period with present confidence.
---
Outside diff comments:
In `@lib/compute/derivation_engine.dart`:
- Around line 3889-3895: Add direct regression tests for ana.detectNaps and the
_attachNaps flow, covering wrist-off exclusion, charging exclusion, and detector
abstention. Verify excluded spans produce no credited nap, while abstention
leaves nap_min unwritten; use fixtures consistent with the existing tests in
test/nap_credit_test.dart.
🪄 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: 93dc1d62-138b-4fb5-a963-1ebd15871c87
📒 Files selected for processing (5)
lib/compute/derivation_engine.dartlib/data/local_repository_impl.dartlib/ui/insights/coach_cards.darttest/crossday_artifact_freshness_test.darttest/nap_credit_test.dart
| // fact about the day the artifact was BUILT on; serving them on a later | ||
| // day makes `_todayNum` read yesterday's strain and nap minutes as | ||
| // today's (§3.3). See [crossDayArtifactUsableToday]. | ||
| if (crossDayArtifactUsableToday(decoded, LocalDb.localDayLabelNow())) { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use the shared local-day helper.
Lines 2741 and 2764 add LocalDb.localDayLabelNow() as another local-day-label source. Replace both reads with todayLabel() so the cache gate and artifact stamp use the required shared day-label contract.
Also applies to: 2764-2764
🤖 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` at line 2741, Replace both
LocalDb.localDayLabelNow() calls in the derivation flow, including the
crossDayArtifactUsableToday check and artifact stamp at the referenced
locations, with the shared todayLabel() helper. Keep the existing cache-gate and
artifact-stamping behavior unchanged.
Source: Coding guidelines
There was a problem hiding this comment.
Declining this one — the correctness premise does not hold. LocalDb.localDayLabelNow() is not a second day-label source:
static String localDayLabelNow() => todayLabel();A one-line delegation to the canonical helper, and db.dart imports day_label.dart. The value is provably identical, so §3.7 is not in play — there is no second "today" to diverge from.
What is fair is consistency: this file uses both spellings (todayLabel() at 2130/2477, LocalDb.localDayLabelNow() at 1747/2747/2770), and the codebase leans 23-to-6 toward the direct call. My line 2747 matched the pre-existing 2764/2770 two lines away, so switching only mine would make the same function internally inconsistent.
Happy to convert all three in this file if @abdulsaheel wants it uniform, but it is cosmetic and touches pre-existing lines, so not doing it unprompted.
| 'periods': _periodsWithMainStages( | ||
| b, | ||
| { | ||
| 'light_min': min('light_sec'), | ||
| 'deep_min': min('deep_sec'), | ||
| 'rem_min': min('rem_sec'), | ||
| 'nrem_min': min('nrem_sec'), | ||
| }, | ||
| // Naps carry their own confidence and the screen draws a ConfDot for | ||
| // any period that has one, so omitting the main period's left the main | ||
| // card as the ONLY one with no dot — reading as "unknown" for the | ||
| // best-evidenced period on the screen. Stays null when accounting had | ||
| // no confidence, which correctly draws nothing. | ||
| mainConfidence: sleepConf, | ||
| ), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Preserve main periods when TST is unknown.
_sleepPeriods can now emit a main period with duration_min: null. However, line 558 returns has_sleep: false before this changed forwarding code runs when tst is null. The periods screen then cannot render the main period or its confidence.
Treat a valid main period or sleep window as sleep evidence. Return nullable duration and debt fields, but still return enriched periods. Add a regression test for a main period with unknown TST and present confidence.
🤖 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/data/local_repository_impl.dart` around lines 613 - 627, The sleep
summary flow around _sleepPeriods must not return early with has_sleep: false
when TST is null if a valid main period or sleep window exists. Update the
relevant guard to treat that period/window as sleep evidence, preserve nullable
duration and debt fields, and still return the enriched periods including
mainConfidence. Add a regression test covering an unknown-TST main period with
present confidence.
Source: Coding guidelines
There was a problem hiding this comment.
Real and reachable — I confirmed _sleepPeriods adds the main period whenever a WINDOW exists (offsetSec > onsetSec) with duration_min: mainTstMin, which is nullable. So a day where segmentation found the window but staging produced no TST genuinely has a main period that _daySleep then hides behind has_sleep: false.
But it is not a regression, and that changes what to do about it. Tracing both halves:
| Origin | |
|---|---|
nullable duration_min |
e68f830 — this PR |
has_sleep: false gate |
14829be — predates this PR |
That day rendered as "no sleep" before this PR too, because tst was null then as well. The PR added richer data that a pre-existing gate happens to suppress in exactly that case. Net user-visible behaviour is unchanged.
So this is a genuine enhancement — surfacing partial sleep evidence when windowing succeeds but staging fails — rather than something this PR broke. Combined with your own 🏗️ Heavy lift tag (it means reworking the has_sleep contract that drives the Sleep screen empty states, plus nullable duration/debt downstream), restructuring the day contract at commit 6 of an already-large PR is how a regression lands that nobody attributes to this change.
Recommending a follow-up issue instead. @abdulsaheel to call it.
…idnight Three review findings on the nap -> sleep-periods seam. All three verified against the code first; the pin (a) was already fixed by 5e57df5. 1. ABSENT IS NOT ZERO, AND IT IS NOT A CONFIDENT TOTAL EITHER. `_attachNaps` returned `const []` from FOUR places: the day was judged and held no nap, the substrate was too short, the detector abstained, and an exception. `_sleepPeriods` could not tell them apart, so it left `totalKnown = true` and published `total_asleep_min = mainTstMin` as a complete day total on days whose naps were never assessed -- in the very same bundle where `naps.value` is null and `nap_min` is (correctly) left unwritten. The PR already applied "an unknown component makes the SUM unknown" to unknown DURATIONS; this extends it to unknown EXISTENCE. `_attachNaps` now returns null for the three unjudged cases. 2. THE MIDNIGHT DOUBLE-COUNT. Attribution was guarded on the trailing edge only (`start < attributionEndSec`, plus the analytics-side backward `unfinished` walk). Nothing guarded the leading edge: `napSub` opens AT local midnight, and analytics' `stillAt(0)` short-circuits its discontinuity check at `k == 0` (nap.dart), so the post-midnight remainder of a nap yesterday already emitted whole is re-detected today as a fresh bout at index 0 and credited a second time -- phantom nap card, phantom Timeline band, and its minutes subtracted from today's sleep need. Couch 23:40-00:40 reproduces it. This was unreachable while the old nocturnal detector needed 60+ min and an HR dip; `minNapSec` at 15 min makes it reachable. Gated on CONTIGUITY, not on index alone. If the record only starts hours into the day, yesterday's detector broke on that same recording discontinuity and dropped the bout too -- dropping it here as well would trade a double-count for silent data loss. `napLeadingEdgeContiguitySec`. 3. PRE-RENAME DAYS RENDER BLANK CARDS FOREVER. The producer moved to `onset_ts`/`wake_ts`/`duration_min`, but days derived before that keep `start`/`end`/`asleep_min` and are never rewritten: a day finalizes ~48 h behind the data edge and raw is pruned after `rawRetentionDays`, so once its substrate is gone a kAlgoVersion bump cannot re-derive it and `dayResult()` serves that payload forever. Every such day showed "--" for onset, wake AND duration under a still-confident hero total, which reads as data loss rather than an old schema. Translated on READ, which also makes this independent of the merge order of the two PRs touching this seam. Also corrected `napBoundaryBufferSec`'s doc comment, which asserted the buffer "can't double-count" -- finding 2 is exactly the case where it does. 9 tests added, each mutation-verified (reverting the guard fails the test and only that test). Full suite: 1156 tests, the 6 failures in notification_dedupe_test are pre-existing and reproduce on origin/main unmodified. kAlgoVersion deliberately NOT touched here -- findings 1 and 2 do change derived output, so whatever number this lands on must be new.
|
Picking up the review of this PR. Pushed one commit ( (a) The analytics pin — already fixed by you in
|
| mutation | result |
|---|---|
| leading-edge guard removed | only the double-count test fails |
| unjudged-total guard no-op'd | only the "unjudged publishes no total" test fails |
| legacy-key translation removed | only the legacy-keys test fails |
Full suite 1164 passing / 6 failing. The 6 are all in notification_dedupe_test and are pre-existing — they reproduce on origin/main unmodified, so they're not from this PR. Worth a separate issue.
Refuted / not acted on
- "stale
nap_minleaks intometric_serieswhen a re-derive abstains" — not real. The series argument is a fixed literal map that lists every key withsc()returning null, so an abstaining re-derive overwrites the row with NULL rather than leaving the old value. No fix needed. - kAlgoVersion deliberately left untouched, since the 56/56 collision with Steps from a real pedometer only; movement minutes on measured evidence #182 is being renumbered separately. Flagging clearly: findings 1 and 2 do change derived output, so whatever number this lands on has to be a new one.
One thing I did not fix, for #205's benefit
_daySleep returns early on tst == null before the periods mapping, so a nap-only / night-shift day still shows "No sleep recorded" with no nap row. I hit this writing the fixtures for finding 3. It's squarely #205's goal rather than this PR's, so I left it there — but it means #205 can't meet its own goal without touching that early return.
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)
lib/compute/derivation_engine.dart (1)
3825-3840: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAdd
confidenceto the main sleep period.The main-period map omits
confidence. Nap-period maps include it. This violates the documented shared period contract and prevents consumers from distinguishing absent confidence from an omitted field.Pass the main-sleep confidence through
_DayBlocksInputand write it into the main period. Add coverage for both known and absent confidence.#!/bin/bash set -euo pipefail ast-grep outline lib/data/local_repository_impl.dart --match _canonicalPeriod --view expanded rg -n -C 6 "'confidence'|confidence" \ lib/data/local_repository_impl.dart \ lib/ui/sleep/sleep_periods_screen.dart \ test/sleep_periods_legacy_keys_test.dart🤖 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 3825 - 3840, Add a nullable main-sleep confidence field to _DayBlocksInput, propagate it through the derivation flow, and include it as 'confidence' in the main period map alongside the existing fields. Preserve null when confidence is absent, and add coverage verifying both populated and absent confidence values.
🤖 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 3896-3898: Update _computeDayBlocks so every abstention path
publishes the explicit unknown naps envelope: write the same naps value used by
the !m.present path before returning null for short input at
lib/compute/derivation_engine.dart lines 3896-3898 and in the error path at
lines 4006-4008. Update test/nap_attribution_test.dart lines 252-260 to assert
the returned bundle contains naps.value == null for short input.
In `@lib/data/local_repository_impl.dart`:
- Around line 699-702: Update the legacy-field fallback logic in the map
transformation around onset_ts, wake_ts, and duration_min to check
!m.containsKey(...) rather than whether each current-schema value is null,
preserving explicit current-schema nulls while still backfilling absent fields.
Add a regression test covering current-schema null values alongside non-null
legacy fields and verify the nulls remain unchanged.
In `@test/nap_attribution_test.dart`:
- Around line 128-263: The _attachNaps day-boundary attribution tests need
regression coverage for exclusion spans. Add separate nap-shaped cases
overlapping a wristOff span and a charging span, pass each exclusion input
through the test substrate/detectNaps setup, and assert the result is judged,
contains no periods, and records nap_min as 0.0.
---
Outside diff comments:
In `@lib/compute/derivation_engine.dart`:
- Around line 3825-3840: Add a nullable main-sleep confidence field to
_DayBlocksInput, propagate it through the derivation flow, and include it as
'confidence' in the main period map alongside the existing fields. Preserve null
when confidence is absent, and add coverage verifying both populated and absent
confidence values.
🪄 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: 90af5255-8a22-4fd9-823a-d756337543cd
📒 Files selected for processing (5)
lib/compute/derivation_engine.dartlib/compute/derive_prepare.dartlib/data/local_repository_impl.darttest/nap_attribution_test.darttest/sleep_periods_legacy_keys_test.dart
Both verified against the code (and one against a repro) before fixing.
1. EXPLICIT NULL WAS BEING PROMOTED INTO A MEASUREMENT.
`_canonicalPeriod`'s legacy-key fallback tested `m['onset_ts'] == null`,
which cannot tell an ABSENT key from a key present with an explicit null.
On a mixed payload -- `duration_min: null` (the new producer's honest "not
measured") sitting beside a stale `asleep_min: 40` -- the null test
back-filled 40 and the card rendered a confident duration for a period
nobody measured. Reproduced directly:
current guard -> duration_min = 40
containsKey -> duration_min = null
That is exactly the dishonesty this seam was added to remove, reintroduced
one layer up. Now `containsKey` on all three fields.
2. "UNKNOWN" HAD TWO DIFFERENT ENCODINGS.
`_computeDayBlocks` starts from an empty bundlePatch and `_attachNaps` is
the only writer of `naps`. The `!m.present` path wrote
`naps.value: null`, but the short-input and error paths returned without
writing anything, so the key was missing entirely. Which encoding a day got
depended on HOW the abstention happened, and a reader checking
`bundle['naps']?['value'] == null` and one checking
`bundle.containsKey('naps')` would disagree about the same day.
`_writeUnknownNaps` now publishes one explicit envelope on every path.
Also added the wrist-off / charging coverage CodeRabbit asked for. Fair hit:
the PR threads both spans into `detectNaps` and nothing exercised either, yet
a band on a charger is perfectly still and is the dominant nap false positive
-- so that guard is doing real work and was untested.
5 tests added, each mutation-verified (restoring the null test fails exactly
the mixed-payload test; removing the envelope fails exactly the envelope test).
Suite 1169 passing; the 6 failures in notification_dedupe_test are pre-existing
and reproduce on origin/main unmodified.
|
@coderabbitai reviewed my commit and found two real defects in it. Both fixed in 1. Explicit null was being promoted into a measurement — valid, and the sharpest of the three
So the card would render a confident duration for a period nobody measured. That's exactly the dishonesty this seam was added to remove, reintroduced one layer up by my own fix. Now My existing test only covered legacy key also absent, which is why this slipped through — good catch. 2. "Unknown" had two different encodings — valid
3. wrist-off / charging coverage — fair hitThe PR threads both spans into Both fixes are mutation-verified: restoring the null test fails exactly the mixed-payload test, and removing the envelope fails exactly the envelope test. Suite 1169 passing (the 6 @svssathvik7 — also confirmed your |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- commit summary ---'
git show --stat --oneline --decorate 87d3258
printf '%s\n' '--- changed Dart files ---'
git diff-tree --no-commit-id --name-only -r 87d3258 -- '*.dart'
printf '%s\n' '--- method outlines ---'
for f in lib/data/local_repository_impl.dart lib/compute/derivation_engine.dart; do
if [ -f "$f" ]; then
ast-grep outline "$f" --items all --type function,method --match '_canonicalPeriod|_attachNaps|_writeUnknownNaps|_computeDayBlocks' || true
fi
done
printf '%s\n' '--- relevant implementation and test references ---'
rg -n -C 5 \
-e '_canonicalPeriod\b' \
-e '_writeUnknownNaps\b' \
-e '_attachNaps\b' \
-e 'wristOffSpans|chargingSpans|wrist.off|charging' \
-e 'containsKey\(' \
lib testLength of output: 130176
These findings are addressed. The added regression tests protect the required behavior. 🐇 ✅ ✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
|
Correction to my earlier note on this PR, and it's good news. I described the 6 They are a time bomb, not a standing breakage. The suite builds date-prefixed dedupe keys from a hardcoded
Proved it by substituting today's date into the unmodified file on Practical impact here: this PR's CI cannot go green until #207 merges, regardless of its own content. Sorry for the noise — "pre-existing and unrelated" was accurate but undersold that it was actively blocking you. |
|
@abdulsaheel — reviewed I re-verified your pre-existing-failure claim rather than taking it on trust, and it holds: exactly 6 failures in Two things before merge. 1.
|
2 participants
Note
Unblocked. OpenStrap/analytics#38 merged as
c3a30be, and5e57df5repins to it. This branch previously did not compile against its own pin (f0d1153— 10 analyzer errors, masked locally by the gitignoredpubspec_overrides.yaml). Against the real pin now:flutter analyze lib/clean, 1150 tests pass.Three commits.
e68f830wires up the new nap detector,60fcc5afixes the same class of bug for strain,5e57df5repins analytics.e68f830— naps: one detector, honest minutes, no phantom nap (kAlgoVersion 55)Consumes analytics' new
sleep/nap.dartas the only nap source, passing the strap's own WRIST_OFF/WRIST_ON and CHARGING_ON/OFF spans (decoded and persisted toband_eventsall along, never used — a band on a table or charger is motionless and is the dominant nap false positive).nap_minnow carries minutes ASLEEP, not the in-bed span. A 2 h lie-down at 70% efficiency credited 120 min against sleep need instead of 84.nap_minis left unwritten.is_todaystamp on the cross-day record. Taking the last record positionally is yesterday on any day whose row has not been derived yet.sleep_coach.nap_credit_minexposes the credit that was actually applied, so the coach card can show it instead of silently shrinking the ring —sleepNeedclamps to [6 h, 11 h] after subtracting, so a large credit is only partly realized.60fcc5a— sleep need: read TODAY's strain, and disclose the bonus it addsv55 fixed
nap_min's cross-day read and left the other today-scoped input tosleepNeed, two lines above it, still going through_lastNum— which walks backward through the oldest-first records and returns the last non-null. On any day whose strain compute abstained, tonight's strain bonus was built from an earlier day's workout: the identical §3.3 imputation, in the identical function.Measured on a 7-day fixture: a carried strain of 18 inflated
need_secby 2314 s (38.6 min).The direction is the opposite of the nap bug
This is why it needed more than a one-word substitution. Naps are SUBTRACTED, strain is ADDED, so while both inputs floor at 0, that floor is an upper bound on need for naps and a lower bound for strain:
sleepNeednap_minstrainSo
?? 0.0recommends up to 45 min less sleep. It is not the cautious direction, and the "no credit is the safe direction" reasoning that justified the nap fix does not transfer. It is still correct, on two other grounds:Making
needabstain entirely was rejected: there is nois_todayrow until today derives, so the whole Sleep Coach card — need, performance, bedtime, wake — would blank every morning, and a false empty state is itself a §4.1 bug pattern.Because 0 is not cautious, it is not silent
sleep_coach.strain_bonus_minreports the minutes the bonus actually added, measured likenap_credit_min(re-run with strain zeroed, then diff), so the clamp cannot make the card claim an increaseneed_secnever took.0says "you rested";nullsays "we could not measure today's strain, so tonight's need is short by up to 45 min". Collapsing those would re-hide exactly what the today-scoping fix exposed.strainBonusCaption, mirroring the nap credit. It stays silent on null, matching that precedent — surfacing "today's strain was not measured" to the user is a product decision, and the bundle now carries the distinction for whoever takes it.Tests
12 new tests in
test/strain_bonus_test.dart, each watched fail first.nap_credit_test's fixture stampedstrainon every row including today's. Under_lastNumthat cancelled between the two series it compares; once strain is today-scoped it no longer does, so the fixture was coupling an unrelated input into the nap assertions. Removed — the strain path has its own file now.Version
kAlgoVersion55 → 56, with a changelog entry.need_secchanges andday_resultrows are immutable per version, so without the bump nothing re-derives and the fix is inert in the field.Verification
Against the real pin (
c3a30be), withpubspec_overrides.yamlmoved aside so nothing resolves to a local working copy.flutter analyze lib/test/strain_bonus_test.dart+test/nap_credit_test.dartThe one failure is
workout_reliability_test.dart— "a queued job stays parked for the session, then runs on release". It is pre-existing and fails on clean main; unrelated to this branch.Summary by CodeRabbit
New Features
Bug Fixes