Repository navigation
Conversation
Found while tracing #2199, and separate from the anchor question that issue is actually about. The nav header's date line read: clock = night?.clockLabel ?: model?.clockLabel `night` is null when the browsed night has no stage data. `model` is built with selectedDay = null, so it resolves to the NEWEST night rather than the one being browsed. The header then printed the latest night's date underneath a relative label that was still counting correctly for the browsed night, with nothing on screen to say the two lines were about different nights. A confidently wrong date is worse than a missing one. Browsing to a stage-less night silently relabelled it as the most recent night, which is indistinguishable from an off-by-one in the label itself, and that is the shape of what was reported. The fallback now reads the browsed night's OWN window out of navDays, using SleepGroupEdit.groupWindow, the same (min effectiveStartTs, max endTs) pair the hero and the time editor use, so a stage-less night dates itself the way a decoded one does. When even that is unavailable it returns null and the header renders an em dash, which is honest. The expression moved out of the composable into navHeaderClockLabel in SleepHeroLogic.kt, next to calendarNightsAgo, so the rule is unit-testable rather than buried in a call site. Four cases pin it: the hero's own label wins; a stage-less night dates itself rather than borrowing the newest night's date; an out-of-range offset yields nothing; a degenerate group yields nothing. Negative control: pointing the fallback back at navDays[0], which is what the old code effectively did, fails 2 of the 4. SleepHeroLogic.kt was restored byte-for-byte before the final run. Android only. The iOS nav header does not share this structure and has no equivalent fallback. This does NOT address the anchor mismatch in #2199, where the relative label counts from the wake timestamp while the date formats the onset. That one needs a decision about which end of the night names it, and it is still open. com.noop.ui: 1046 tests, 0 failures.
Re-review of my own #2201. The date assertions were timezone-dependent and would have failed for anyone west of UTC. clockLabelFor formats in the DEFAULT zone, since it takes no zone parameter, while the fixtures build their timestamps in UTC. A 07:00 UTC wake means a 01:00 UTC onset, which formats as the PREVIOUS day once the reader's zone is far enough west. That is why it passed everywhere I looked: my machine is NZST and the runners are UTC, which are exactly the two cases where it happens to hold. Under TZ=America/New_York the assertion gets "Tue 11 Aug" instead of "Wed 12 Aug". The default zone is now pinned to UTC for the class and restored afterwards, so the literal dates mean the same thing everywhere. The pre-existing tests in this file are unaffected, since they pass their zone to calendarNightsAgo explicitly and build in UTC already. Verified by running the suite under five zones spanning UTC-8 to UTC+12: America/Los_Angeles, America/New_York, Europe/Berlin, Pacific/Auckland and UTC. 13 tests, 0 failures in each. Control: with the pin removed, TZ=America/New_York fails navHeaderClockLabel_datesTheBrowsedNight_notTheNewestOne with "got Tue 11 Aug · 21:00 - 03:00", so the pin is doing the work rather than the runner's zone hiding the problem. Tests only.
Owner
Author
|
Re-reviewed my own change before asking for it to go in, and found a defect in the tests I had just written. The date assertions were timezone-dependent. It passed everywhere I happened to look because my machine is NZST and the runners are UTC, which are exactly the two cases where it holds. Under The default zone is now pinned to UTC for the class and restored afterwards. The locale was already pinned ( Verified under five zones spanning UTC-8 to UTC+12: Control: with the pin removed, The pre-existing tests in this file are unaffected: they pass their zone to Nothing else changed. The production fix is as it was. |
Second re-review pass on #2201. No behaviour change; the comment was wrong about its own justification, which is the kind of thing a later reader acts on. I wrote that the fallback uses "the same (min effectiveStartTs, max endTs) pair the hero and the time editor use". That is true of the time editor and false of the hero. The hero dates from heroGroup.first().effectiveStartTs, which is the group AFTER dropping a spurious pre-onset awake stub, so the bedtime label matches where the hypnogram starts (#736). groupWindow takes the minimum across the whole group, stub included. On a night with such a stub the two rules give different dates. The implementation stays as it is, because the difference is forced rather than accidental. The stub test, isPreOnsetAwakeStub, weighs a fragment's DECODED asleep minutes against the group's largest span. A night reaching this fallback has no hero, so those minutes are exactly what is missing, and there is no hypnogram to align a bedtime to either. The full window is the best available answer and, unlike the old fallback, is unambiguously about the night being browsed. The comment now says that, including the consequence a reader should expect: this can read a stub's onset where the hero would have skipped one. Tests unchanged and still green: 13, 0 failures.
This was referenced Sep 14, 2026
ryanbr
added a commit
that referenced
this pull request
Sep 15, 2026
…date @bartmuskala reported the caption dates three times, and the first two explanations were wrong. This is the third, with his own table as the test. The carousel groups nights by `localDayString(endTs)`, so every row IS one wake date and the rows are unique by construction. The date beside them came from the ONSET, and a night that begins after midnight has its onset on its own wake date. Two adjacent rows then printed the same date while counting correctly: Last night Mon 14 Sep 1 night ago Sun 13 Sep 2 nights ago Sun 13 Sep <- the after-midnight row, repeating the one above 3 nights ago Sat 12 Sep Dating a night by its wake day minus one uses the same key the row is selected by, so a repeat becomes impossible rather than unlikely. That is what "keep the logic from start to end" asks for. The times stay the true onset and wake, so such a night reads "Sat 12 Sep · 00:30 - 07:00": the night of Saturday into Sunday in the ordinary sense, reported exactly when it ran. `minusDays(1)` on the zoned date rather than `wakeTs - 86_400`, because across a DST boundary the day is 23 or 25 hours and the arithmetic form lands on the wrong calendar date. ## What I got wrong twice First I said the whole list was uniformly one day out, label from the wake end and date from the onset end. His table ruled that out: a uniform offset cannot print the same date twice. Then I said the repeat was the borrowed-date defect and that #2201 had settled it, adding that there was no design decision left. #2201 was real and separate, and it landed. This survived it, which his 11.7.0 table showed by keeping the duplicate and moving it down a row while naming the same night. I also wrote that an after-midnight night looking out of sequence was "one case that looks wrong but is not". That was the bug, dismissed in advance. ## Tests `afterMidnightNightDoesNotRepeatTheDateAboveIt_issue2199` is his Tuesday table, and it asserts no two rows may share a date rather than only checking the four expected strings. Confirmed failing before the fix with the exact reported symptom. The earlier `nightCaptionPairsEachLabelWithItsOwnOnsetDate_issue2199` could not have caught this: every night in it crosses midnight, so onset and wake agree on ordering. `navHeaderClockLabel_datesTheBrowsedNight_notTheNewestOne` pinned the old anchor in its own comment, on a 01:00 to 07:00 fixture, which is exactly this shape. Repinned to the same shape rather than deleted: the invariant it exists for, a stage-less night not borrowing the newest night's date, still holds. Android suite: 6121 tests, 0 failures. ## Not included iOS renders this differently and is untouched. `Night.spanLabel` shows a span for a cross-midnight night and a bare date when onset and wake share a day, so it has a milder form of the same ambiguity: two rows can still both lead with Sun 13. Nobody has reported it. Converging the two is a design decision worth its own issue, and his table cannot settle it, since both anchors agree on an ordinary cross-midnight night.
ryanbr
added a commit
that referenced
this pull request
Sep 15, 2026
Re-review of my own change. `clockLabel` fills ONE slot from two branches: the session when the night decoded, the daily metric's day key when it did not. The previous commit moved only the session branch. Sleep rows are keyed by `dayString(endTs)`, "the day its night ENDS on", so the fallback prints the WAKE day. The two branches therefore behaved like this: | night | before #2199 | after the first commit | now | |---|---|---|---| | crosses midnight | disagreed already | still disagreed | agree | | begins after midnight | agreed | DISAGREED, my regression | agree | So a night would have read a day apart depending on whether its stages happened to decode. That is the two-paths-one-night shape of #2201, which I had just told the reporter was settled. Moving both ends together also repairs the cross-midnight disagreement, which predates all of this and nobody reported. A latent bug goes with it: the fallback parsed the day key at UTC midnight with one formatter and rendered it in the DEVICE zone with another, so west of UTC it already printed the previous day. `LocalDate` carries no instant to misplace. `clockLabelNamesTheSameNightFromEitherBranch_issue2199` asserts the two branches AGREE rather than pinning two strings, so it keeps holding if the anchor ever moves again. Mutation-checked: reverting the fallback turns that test red and nothing else. Android suite: 6122 tests, 0 failures, 0 errors.
ryanbr
added a commit
that referenced
this pull request
Sep 15, 2026
…date (#2235) Thanks @bartmuskala, and sorry it took three rounds. The typed rows are what made the diagnosis possible, both times. The carousel groups nights by `localDayString(endTs)`, so every row IS one wake date and the rows are unique by construction. The date beside them came from the ONSET, and a night that begins after midnight has its onset on its own wake date. Two adjacent rows then printed the same date while counting correctly: Last night Mon 14 Sep 1 night ago Sun 13 Sep 2 nights ago Sun 13 Sep <- the after-midnight row, repeating the one above 3 nights ago Sat 12 Sep A night is now dated by its wake day minus one, the same key the row is selected by, so a repeat is impossible rather than unlikely. That is "keep the logic from start to end". The times stay the true onset and wake, so such a night reads "Sat 12 Sep · 00:30 - 07:00". `minusDays(1)` on the zoned date rather than subtracting 86_400 seconds: a DST day is 23 or 25 hours long and the arithmetic form lands on the wrong date. ## What I got wrong I said the list was uniformly one day out. His table ruled that out: a uniform offset cannot print the same date twice. I then said the repeat was the borrowed-date defect, that #2201 had settled it, and that no design decision remained. #2201 was real and separate and it landed; this survived it. I also described an after-midnight night looking out of sequence as "one case that looks wrong but is not". That was this bug, dismissed in advance. ## Both ends, and both platforms `clockLabel` fills ONE slot from two branches, the session when the night decoded and the day key when it did not, and moving only the first would have made the same night read a day apart depending on whether its stages decoded. Both move. That also repairs a cross-midnight disagreement older than this report, and a latent bug where the fallback parsed the day key at UTC midnight and rendered it in the device zone, printing the previous day west of UTC. Apple carried the same defect more quietly: its cross-midnight span already led with the wake day minus one, but the single-date branch named the night by its onset, so two rows shared a LEADING date rather than an identical string. Converged, so both platforms print the same date for the same night. The cross-midnight span is untouched. ## Tests The reported table, asserting that no two rows share a date rather than checking four strings. Confirmed failing first with the exact symptom. The earlier #2199 fixture could not have caught this: every night in it crosses midnight, so onset and wake agree on ordering. `navHeaderClockLabel_datesTheBrowsedNight_notTheNewestOne` pinned the old anchor in its own comment and is repinned to the same shape, not deleted: the invariant it exists for still holds. Android 6122 tests, 0 failures. The Apple tests derive expected dates through the system locale rather than hardcoding English, after a re-review caught them spelling out "Sat 12 Sep" against a formatter that pins no locale by design.
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
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.
Found while tracing #2199, and separate from the anchor question that issue is actually about. Reported by @bartmuskala.
The bug
The nav header's date line read:
nightis null when the browsed night has no stage data.modelis built withselectedDay = null, so it resolves to the newest night rather than the one being browsed. The header then printed the latest night's date underneath a relative label still counting correctly for the browsed night, with nothing on screen to say the two lines were about different nights.A confidently wrong date is worse than a missing one. Browsing to a stage-less night silently relabelled it as the most recent night, which from the outside is indistinguishable from an off-by-one in the label, and that is the shape of what was reported.
The change
The fallback reads the browsed night's own window out of
navDaysviaSleepGroupEdit.groupWindow, the same (mineffectiveStartTs, maxendTs) pair the hero and the time editor use, so a stage-less night dates itself the way a decoded one does. When even that is unavailable it returns null and the header renders an em dash, which is honest.The expression moved out of the composable into
navHeaderClockLabelinSleepHeroLogic.kt, besidecalendarNightsAgo, so the rule is unit-testable rather than buried in a call site.Tests
Four cases pin it:
Negative control: pointing the fallback back at
navDays[0], which is what the old code effectively did, fails 2 of the 4, including the regression case.SleepHeroLogic.ktwas restored byte-for-byte before the final run.com.noop.ui: 1046 tests, 0 failures.compileFullDebugKotlinclean,doc_comment_lintclean.Scope
Android only. The iOS nav header is structured differently and has no equivalent fallback, so there is no twin to keep in step; UI parity here is feature-level.
This does not address the substance of #2199. That report is about the relative label counting from the wake timestamp while the date formats the onset, so the two lines name different ends of the same night. That needs a decision about which end names a night, and it stays open.