Repository navigation
fix(sleep): name a night by the evening it belongs to, not its onset date - #2235
Merged
Merged
Conversation
…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.
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.
Android and Apple would otherwise name the same night differently after the two
commits above, where before they agreed.
`Night.spanLabel` renders a span when a night crosses midnight and a single date
when onset and wake share a day. The span already leads with the wake day minus
one, so it was right by construction. The single-date branch named the night by
its onset, which for a night BEGINNING after midnight is its own wake date, so
it led with the date the row above already leads with:
1 night ago Sun 13 -> Mon 14 Sep
2 nights ago Sun 13 Sep <- leading with Sun 13 a second time
That is the reported defect presented more quietly. Because the strings are not
identical, it reads as a coincidence rather than a repeat, which is likely why
Apple was never reported while Android was, three times.
The single-date branch now names the night by its wake day minus one, so every
row leads with the same anchor and the two platforms print the same date for the
same night. `byAdding:` rather than subtracting 86_400 seconds: a DST day is 23
or 25 hours long and the arithmetic form lands on the wrong calendar date.
The cross-midnight span is untouched. It states both ends, which is more than
the anchor needs, and it was never part of the report.
## Verification
Compiled the label logic standalone with `swiftc -O` against the reported rows,
since the app target does not build on Linux:
BEFORE AFTER
Last night Mon 14 -> Tue 15 Sep Mon 14 -> Tue 15 Sep
1 night ago Sun 13 -> Mon 14 Sep Sun 13 -> Mon 14 Sep
2 nights ago Sun 13 Sep Sat 12 Sep
3 nights ago Sat 12 Sep Fri 11 Sep
`SleepNightSpanLabelTests` pins the reported shape, that consecutive rows never
share a leading date, and that the cross-midnight span survives. The second of
those is the invariant rather than the strings, matching its Android twin.
NOT run locally: StrandTests needs the app target and a Mac. CI runs it through
the `build (Strand, macOS)` leg. No new user-facing strings, so no catalogue
entries are needed.
Re-review of the commit above. The Apple test hardcoded "Sat 12 Sep", and
`Night.dateFmt` pins no locale by design, so it renders in the reader's. The
test would have passed here and failed on a runner configured for anywhere else.
That is the same fragility as the `Locale.ROOT` repair in the Kotlin oracle
earlier: what is under test is the DATE, not its spelling.
Expected values now come from a formatter carrying the same format and the
system locale, applied to the wake day minus one, so the assertion follows the
anchor into any locale.
Two smaller repairs from the same pass:
* the leading-date comparison split on " ->" instead of taking a fixed six
characters, which assumed English-width day and month abbreviations.
* `hasPrefix` became exact equality against whichever form the row takes, the
span's "EEE d" or a single date's "EEE d MMM". `hasPrefix` would let a row
leading "Sun 1" pass against an expected "Sun 13 Sep".
Verified by compiling the shipped label and the test's helpers standalone with
`swiftc -O` over the reported rows: every row's leading date equals its own wake
day minus one, and no two rows share one. StrandTests itself still needs a Mac
and runs on the `build (Strand, macOS)` leg.
CI's doc-comments gate caught it: inserting `afterMidnightNight` above the `crossMidnightNight` DECLARATION put it below that declaration's KDoc, leaving the KDoc stacked against my new block and documenting nothing. Dokka and IDE hover would have shown neither. This is the exact hazard CLAUDE.md names, insert above the neighbour's DOC BLOCK rather than above its declaration, and I inserted between the two. The one-line KDoc now sits on `crossMidnightNight` again. `python3 Tools/doc_comment_lint.py` locally: no new detached doc comments, 2196 files, 24 baselined sites remaining. The baseline is untouched, which the gate explicitly asks for.
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.
@bartmuskala reported the caption dates on the Sleep page three times. The first
two explanations I gave were wrong. This is the third, with his own table as the
test.
The cause
The carousel groups nights by
localDayString(endTs), so every row IS one wakedate and the rows are unique by construction. The date printed 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 print the same date while counting correctly:
The change
A night is dated by its wake day minus one, the same key the row is selected by,
so a repeat becomes impossible rather than unlikely. The times stay the true
onset and wake, so such a night reads
Sat 12 Sep · 00:30 - 07:00: the night ofSaturday into Sunday in the ordinary sense, reported exactly when it ran.
minusDays(1)on the zoned date rather thanwakeTs - 86_400, because across aDST boundary the day is 23 or 25 hours long and the arithmetic form lands on the
wrong calendar date.
What I got wrong twice, since it shaped the fix
from the onset end. His table ruled that out: a uniform offset cannot print
the same date twice, and his did.
adding that no design decision remained. sleep: date the browsed night, not the newest one, in the nav header #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 called an after-midnight night looking out of sequence "one case that
looks wrong but is not". That was this bug, dismissed in advance. The reporter
was right all three times.
Tests
afterMidnightNightDoesNotRepeatTheDateAboveIt_issue2199is his Tuesday table.It asserts no two rows may share a date rather than only checking four expected
strings, because uniqueness is the property he actually asked for. Confirmed
failing before the change with the exact reported symptom:
The earlier
nightCaptionPairsEachLabelWithItsOwnOnsetDate_issue2199could nothave caught this: every night in it crosses midnight, so onset and wake agree on
ordering.
navHeaderClockLabel_datesTheBrowsedNight_notTheNewestOnepinned the old anchorin its own comment, on a 01:00 to 07:00 fixture, which is exactly this shape.
Repinned to the same shape rather than deleted, because the invariant it exists
for still holds: a stage-less night must not borrow the newest night's date.
Android suite: 6121 tests, 0 failures, 0 errors.
A regression found by re-reviewing this, and fixed in the second commit
clockLabelfills ONE slot from two branches: the session when the nightdecoded, the daily metric's day key when it did not. The first commit moved only
the session branch.
Sleep rows are keyed by
dayString(endTs), "the day its night ENDS on", so thefallback prints the WAKE day:
A night would have read a day apart depending on whether its stages happened to
decode, which is the two-paths-one-night shape of #2201 that I had just told the
reporter was settled. Moving both ends together also repairs the cross-midnight
disagreement, which predates this PR 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.
clockLabelNamesTheSameNightFromEitherBranch_issue2199asserts the two branchesAGREE rather than pinning two strings, so it keeps holding if the anchor moves
again. Mutation-checked: reverting the fallback turns that test red and nothing
else. Suite now 6122 tests, 0 failures.
Apple, converged in the third commit
Android and Apple would otherwise name the same night differently where before
they agreed, so this now moves both.
Night.spanLabelrenders a span when a night crosses midnight and a single datewhen onset and wake share a day. The span already leads with the wake day minus
one, so it was right by construction. The single-date branch named the night by
its onset, which for a night BEGINNING after midnight is its own wake date:
The same defect, presented more quietly. Because the strings are not identical
it reads as coincidence rather than a repeat, which is likely why Apple was
never reported while Android was, three times. The cross-midnight span is
untouched: it states both ends, and it was never part of the report.
Verified by compiling the label standalone with
swiftc -O, since the apptarget does not build on Linux:
Mon 14 → Tue 15 SepSun 13 → Mon 14 SepSun 13 SepSat 12 SepSat 12 SepFri 11 SepStrandTests were NOT run locally. They need a Mac and the app target; CI
covers them on the
build (Strand, macOS)leg. No new user-facing strings, sono catalogue entries.
A test that would have passed here and failed elsewhere
The fourth commit. The Apple test first hardcoded
"Sat 12 Sep", andNight.dateFmtpins no locale by design, so it renders in the reader's. Expectedvalues now derive from a formatter carrying the same format and the system
locale applied to the wake day minus one, so the assertion follows the anchor
into any locale. The leading-date comparison also split on
" →"rather thantaking six characters, which assumed English-width abbreviations, and
hasPrefixbecame exact equality, since
hasPrefixwould let a row leadingSun 1passagainst an expected
Sun 13 Sep.Not included, deliberately
The parity ledger is unaffected: none of these files are scanned.
KOTLIN_GLOBScovers analytics, ingest, data, protocol and oura, not
ui, andSWIFT_GLOBScovers
Packages/**, notStrand/Screens. Checked rather than assumed.Nap and daytime-sleep rows do not flow through either label, so nothing that is
not a night is renamed.