Make a detected nap visible on the Sleep screen - #205
Open
egoran2 wants to merge 3 commits into
Open
Conversation
📝 WalkthroughWalkthroughSleep periods are normalized into screen data, with main-sleep metrics preserved and nap metadata reduced. Sleep details now expose nap navigation and summaries. Missing confidence values no longer render as zero-confidence indicators. ChangesSleep period display
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant SleepRepository
participant sleepPeriodsForScreen
participant SleepNightContent
participant SleepPeriodsScreen
SleepRepository->>sleepPeriodsForScreen: raw periods and night metrics
sleepPeriodsForScreen-->>SleepRepository: normalized periods
SleepRepository->>SleepNightContent: completed night payload
SleepNightContent->>SleepPeriodsScreen: open periods for the current date
SleepPeriodsScreen-->>SleepNightContent: render periods without absent confidence dots
Possibly related PRs
Suggested labels: 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 |
A daytime nap is detected, stored, and drawn as a band on the day timeline, but the Sleep screen only ever showed the main night. The screen that does list every sleep of the day was unreachable: its only entry point is an AppScaffold action, and the Sleep tab embeds SleepNightContent (embedded: true), so that scaffold never builds. The periods payload was also keyed wrong. The engine writes is_main / start / end / asleep_min; SleepPeriodsScreen reads onset_ts / wake_ts / duration_min, so every card would have rendered as "0m" with no time range under it. - map sleep_periods onto the keys the screen reads, and give the main period the night's TST, efficiency, stage minutes and hypnogram so the two sleep screens can't print different numbers for the same night - the day total is now the sum of what the cards show - add a naps row under the night summary; tapping it opens the breakdown, and it says the nap is not part of the numbers above - a nap carries no confidence instead of a literal 0, and the confidence dot is hidden when there is none - test/sleep_naps_visible_test.dart pins the mapping and the row No stored analytics output changed, so kAlgoVersion stays put.
egoran2
force-pushed
the
fix/naps-visible-on-sleep-screen
branch
from
August 6, 2026 17:01
147bf3b to
7c38e28
Compare
egoran2
marked this pull request as ready for review
August 6, 2026 17:02
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/data/local_repository_impl.dart`:
- Around line 2695-2701: Validate the parsed asleep_min in the duration
calculation around asleepMin, accepting it only when it falls within
0..windowMinutes derived from end and start; otherwise use the period span as
the fallback. Ensure both main and nap duration paths use this bounded value,
and add regression cases in sleep_naps_visible_test.dart for negative and
oversized asleep_min 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: 93bc9857-0e6b-4c58-913b-760bfbbade28
📒 Files selected for processing (4)
lib/data/local_repository_impl.dartlib/ui/sleep/sleep_detail_screen.dartlib/ui/sleep/sleep_periods_screen.darttest/sleep_naps_visible_test.dart
CodeRabbit review: sleepPeriodsForScreen took asleep_min at face value, so a negative or larger-than-window value would print a negative duration on the card and skew the day total. The engine never writes one today (a nap's asleep_min is exactly its window), but this function parses defensively everywhere else, so it should here too. Falls back to the window, which the period's own start/end already vouch for.
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)
test/sleep_naps_visible_test.dart (1)
625-640: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd regression coverage for the recomputed day total.
The new path derives
total_asleep_minfrom mapped period durations, but the supplied tests assert individual period fields and nap-row behavior only. Add an assertion for the existing fixture total:400minutes of main sleep plus101minutes of nap sleep equals501minutes. Also cover the no-period fallback if that fallback remains part of the contract.As per coding guidelines: “Behavior changes, especially regressions involving readiness, abstention, idempotence, synchronization, migrations, and lifecycle safety, must include regression tests.”
🤖 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/sleep_naps_visible_test.dart` around lines 625 - 640, Add regression coverage in the existing sleep/naps test near the mapped period assertions, verifying the fixture’s recomputed total_asleep_min is 501 minutes from 400 main-sleep plus 101 nap-sleep minutes. If the no-period fallback remains supported, add a case asserting its expected total as well.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/data/local_repository_impl.dart`:
- Around line 2698-2702: Validate the main period’s night['duration_min']
against windowMin using the same non-negative, upper-bound checks applied to
p['asleep_min']; when invalid, fall back to the already validated asleepMin
value before assigning duration_min. Add regression coverage for negative and
oversized main duration values.
In `@test/sleep_naps_visible_test.dart`:
- Around line 93-100: Update the test “a length outside its own window falls
back to the window” to assert that the result collection p contains exactly
three elements before checking mapped duration_min values, ensuring
everyElement(101) cannot pass for an empty iterable.
---
Outside diff comments:
In `@test/sleep_naps_visible_test.dart`:
- Around line 625-640: Add regression coverage in the existing sleep/naps test
near the mapped period assertions, verifying the fixture’s recomputed
total_asleep_min is 501 minutes from 400 main-sleep plus 101 nap-sleep minutes.
If the no-period fallback remains supported, add a case asserting its expected
total as well.
🪄 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: 73855007-bddc-45cb-a8ba-da6e4ca0f51c
📒 Files selected for processing (2)
lib/data/local_repository_impl.darttest/sleep_naps_visible_test.dart
CodeRabbit review, second pass: - the window check covered a nap's asleep_min but not the main period, which took night['duration_min'] straight through. Same bound now: nobody sleeps longer than the window they slept in. - the fallback test asserted everyElement on a list it never sized, so it would have passed if the mapper dropped all three inputs. It checks the length first now. Adds a TST regression case for the negative and oversized values.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/data/local_repository_impl.dart`:
- Around line 2706-2710: Update the duration parsing around tstMin and
durationMin to range-check the raw numeric duration before calling toInt(),
rejecting fractional values such as -0.5 and 0.5 so both main-duration and nap
asleepMin fallbacks are preserved. Add regression coverage for fractional inputs
in both paths.
🪄 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: 09176f3b-fa41-48a6-8291-d7508549290c
📒 Files selected for processing (2)
lib/data/local_repository_impl.darttest/sleep_naps_visible_test.dart
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I napped for ~1h40m this afternoon, synced, and the nap was nowhere in the app. It turned out the detection is fine (the nap shows as a band under Your day), but two things kept it off the Sleep screen.
1. The periods screen is unreachable.
SleepPeriodsScreenis only pushed from anAppScaffoldaction inSleepDetailScreen, and the Sleep tab always builds that screen withembedded: true(screens.dart:50,55), which returns the bareColumnwithout the scaffold.SleepDetailScreen.today()is never called either, so nothing in the shipped app can open it.2. The periods payload is keyed wrong. The engine writes each period as
is_main/start/end/asleep_min(derivation_engine.dart:3626), while the screen readsonset_ts/wake_ts/duration_min. Even if you reached the screen, every card would say "0m" with no time range.Before: Sleep tab shows the night only, 6h40m, no sign of the nap anywhere.
After: a row under the night summary, "Daytime nap, 1h 41m, not included in the night above", which opens the per-period breakdown with real times and durations.
Also in here:
0and the confidence dot is hidden insteadtest/sleep_naps_visible_test.dartcovers the mapping and the row (visible with a nap, absent without, tap fires)Nothing about how a nap is computed changed, so
kAlgoVersionstays put.Left alone deliberately, happy to do either in a follow-up: naps still don't enter TST or readiness (they only credit tonight's need via
nap_min), and_daySleepreturnshas_sleep: falsewhen there's no night, so a nap-only day still shows nothing.Verified on Flutter 3.41.6 (the pinned version):
flutter analyzeclean,flutter test --concurrency=1gives 1205 passed / 2 skipped, the two skips being thewhoop_hist.jsonlreplays that need the capture file.Summary by CodeRabbit