Skip to content

refactor(sleep): one stage-key canonicalisation, not two kept in parity by comment - #829

Merged
ryanbr merged 4 commits into
mainfrom
refactor/dedupe-stage-canonicalisation
Jul 26, 2026
Merged

ryanbr merged 4 commits into
mainfrom
refactor/dedupe-stage-canonicalisation

Conversation

@ryanbr

@ryanbr ryanbr commented Jul 26, 2026

Copy link
Copy Markdown
Owner

Follow-up to #733, found reviewing it.

Two implementations of one rule, and the code already knew. stageColorFor did its own trim().lowercase() and folded "awake", "wake" in a when branch, while canonicalStage() in SleepStageTimelineLogic.kt does exactly the same — carrying the comment "(stageColorFor parity)". A drift risk documented instead of removed.

// before
private fun stageColorFor(name: String): Color = when (name.trim().lowercase()) {
    …
    "awake", "wake" -> Palette.sleepAwake

// after
private fun stageColorFor(name: String): Color = when (canonicalStage(name)) {
    …
    "awake" -> Palette.sleepAwake

Both files are package com.noop.ui and canonicalStage is internal, so no plumbing. Adding a stage alias is now a one-place edit — before, a new alias in canonicalStage would silently fail to pick up a colour, which is the quiet-wrong-answer failure mode rather than a visible one.

Behaviour-identical, verified across every input the old branch accepted: "deep", " REM ", "Light", "awake", "WAKE", "wake", whitespace variants, empty string and unknown all resolve to the same tint before and after, fallback included.

#733 didn't create this — it made it visible by moving the two halves into adjacent files, which is a good argument for that refactor rather than against it.

Android only, no schema, no scores, no strings; i18n clean. Not compiled — android.yml is disabled — though this is a two-line change to a when subject within one package.

ryanbr added 3 commits July 25, 2026 23:58
…ty by comment

stageColorFor did its own trim().lowercase() and folded "wake"/"awake" in a when
branch, duplicating canonicalStage() in SleepStageTimelineLogic. The duplication was
known — canonicalStage carried the comment "(stageColorFor parity)" — which is a
drift risk documented rather than removed.

#733 did not create this; it made it visible by moving the two halves into adjacent
files.

stageColorFor now keys off canonicalStage. Both files are package com.noop.ui and
canonicalStage is internal, so no plumbing was needed. Adding a stage alias is now a
one-place edit and the tint follows; before, a new alias in canonicalStage would
silently not colour.

Behaviour-identical, checked across every input the old branch accepted — "deep",
" REM ", "Light", "awake", "WAKE", "wake", whitespace variants, empty and unknown
all resolve to the same tint before and after, fallback included.

Android only. No schema, no scores, no strings — the i18n audit is clean and the
moved code uses uiString(R.string.…) lookups throughout.
[stageColorFor] would not resolve from SleepStageTimelineLogic: the target is
private in SleepStageBreakdownUi.kt, so it is not in scope there and Dokka would
warn on it. The reverse link is fine — [canonicalStage] is internal in the same
package, which does resolve.

Replaced with a code span plus the filename. A pointer that cannot be followed is
worse than one that reads as prose, and the asymmetry is easy to miss because the
two links look identical.
The phrase described a hand-maintained invariant. It isn't one any more — the tint
derives from the same canonicalStage call the spans do, so this test now covers the
colour path as a side effect. Say that instead, and note why there's no direct
assertion: stageColorFor is private.
@ryanbr

ryanbr commented Jul 26, 2026

Copy link
Copy Markdown
Owner Author

Re-review found two things in my own change, both now pushed.

1. A KDoc link that could not resolve. [stageColorFor] from SleepStageTimelineLogic.kt points at a private function in another file — not in scope there, so Dokka would warn. The reverse link is fine: [canonicalStage] is internal in the same package. The asymmetry is easy to miss because the two links look identical. Now a code span plus the filename.

2. The last (stageColorFor parity) note, in StageRowSpansTest.kt:33. That phrase described a hand-maintained invariant, which is exactly what this PR removes — leaving it means grepping it turns up a stale hint. The test now covers the colour path as a side effect of covering the spans, so it says that instead.

No behaviour change in either. CI green, three commits.

I added a doc block above the existing one instead of editing it, leaving two
consecutive /** */ before the same declaration. Kotlin takes only the last as KDoc;
the first silently becomes a dangling comment, so the "case-insensitive" note was no
longer attached to anything.

Merged, keeping the original wording and the fallback behaviour, which the old block
did not state. Duplicate documentation in a PR about removing duplicate logic.
@ryanbr

ryanbr commented Jul 26, 2026

Copy link
Copy Markdown
Owner Author

Third pass, one more of my own: I had added a doc block above the existing one on stageColorFor instead of editing it, leaving two consecutive /** */ on the same declaration. Kotlin takes only the last as KDoc, so the original case-insensitive note silently became a dangling comment. Duplicate documentation, in a PR about removing duplicate logic.

Merged into one block, keeping the original wording and adding the unknown-stage fallback, which neither block stated.

CI green, MERGEABLE/CLEAN, +13/-5.

Out of scope here, but the same defect exists in two other places and I put both there: PuffinExperiment.kt:118 (harmless — near-identical text) and HrvAnalyzer.kt:342, where the detached block is the one reading "Byte-parity twin of Swift classifyCoverage". That is a parity-contract note that is no longer attached to the function it documents. Happy to fix both in a separate one-liner PR.

@ryanbr
ryanbr merged commit 10f8399 into main Jul 26, 2026
1 check passed
@ryanbr
ryanbr deleted the refactor/dedupe-stage-canonicalisation branch July 26, 2026 07:12
ryanbr added a commit that referenced this pull request Jul 27, 2026
Three things found re-reviewing my own change.

The app_name exemption was defined twice — once inside android_strings_xml_gaps and
again inline in the new discovered-locale check. That is the same two-definitions-of-
one-rule pattern #829 removed this morning, and it would have let the hard-gated path
and the discovered path disagree the first time anyone added a second exempt key.
Hoisted to ANDROID_EXEMPT_KEYS, used by both.

The new check re-loaded every catalog the loop above had just parsed — a second full
parse of a 3255-string file for no reason. The extra-locale counts are now collected
inside that existing loop and gated afterwards.

--update-baseline rewrites the JSON baseline only, so its help now says it does not
touch the locale allowance. That file is lowered by hand on purpose: shrinking it
should be a deliberate act, not a side effect of regenerating something else.

No behaviour change — all four gate cases re-verified after the refactor.
DX23876 added a commit to DX23876/noop that referenced this pull request Aug 1, 2026
Classic Today kept an older, larger OverviewHRChart with its own pinch-zoom
(ryanbr#829), a hint row and sleep / workout / Charge / Effort markers, plus a live
bpm badge that was not tappable. Liquid draws a compact live-HR card and hangs
the whole-day reading on a "Full day" link into the Deep Timeline. Classic now
hosts the SAME card.

LiquidLiveHR moves out of LiquidTodayView into its own file — it was already
styled purely in StrandPalette / StrandFont, so this is a move, not a restyle.
Two optional parameters carry the classic call site, both defaulted so Liquid
is unchanged: `headerRoute` makes the read-out row (not the scrubbable thread,
which would swallow the drag as a tap) a link, so a tap on the live bpm opens
the Deep Timeline; `showsLive` is false on a past day, where a live number says
nothing about the day on screen.

Removed with the chart: the ryanbr#829 zoom window, its hint and reset, the pure
reclampHrZoom rule and its tests, the HRChartFrameKey mask, the cached hrAxis,
and the four marker properties — all of that reading lives in the Deep Timeline,
and OverviewHRChart itself is untouched and still drawn there. The day-swipe now
yields to the thread via hrScrubbing / hrScrubEndedAt, as Liquid already does.
The ryanbr#863 calibrating / empty-day branch is deliberately unchanged.

Android's HeartRateTrendCard is not changed: Android has no Liquid Today, so
there is no divergence to close. No analytics and no stored-data change.

Verified: xcodegen generate, then Strand (macOS) and NOOPiOS (iOS) both build
(no default CI compiles app-target Swift); StrandTests green; StrandDesign green
apart from a palette test that fails identically on a clean tree. Not exercised
on a strap or in the simulator.
simoncad7 pushed a commit to simoncad7/noop that referenced this pull request Aug 17, 2026
…ty by comment (ryanbr#829)

stageColorFor did its own trim().lowercase() and folded "wake"/"awake" in a when branch,
duplicating canonicalStage in SleepStageTimelineLogic — which carried the comment
"(stageColorFor parity)", a drift risk documented rather than removed. ryanbr#733 did not
create this; it made it visible by moving the two halves into adjacent files.

stageColorFor now keys off canonicalStage. Both files are package com.noop.ui and
canonicalStage is internal, so no plumbing was needed. Adding a stage alias is a
one-place edit and the tint follows; before, a new alias would silently not colour.

Behaviour-identical by inspection, not just by sampling: canonicalStage is
trim().lowercase() plus "wake" -> "awake", so the only change to the when subject is
that "wake" now arrives as "awake", which the same arm still catches. No input can
reach a different branch. Checked across "deep", " REM ", "Light", "awake", "WAKE",
"wake", whitespace variants, empty and unknown; all resolve to the same tint,
fallback included.

Also fixed three documentation defects introduced while writing this change:
a KDoc link to a private symbol in another file (would not resolve), the last stale
"(stageColorFor parity)" note in StageRowSpansTest, and a stacked pair of /** */
blocks on stageColorFor that silently detached the original case-insensitive note.

Android only. No schema, no scores, no strings; i18n clean. Not compiled —
android.yml is disabled.
ryanbr added a commit that referenced this pull request Sep 27, 2026
chartTimeTicks is shared. The Deep Timeline passes its pinch-zoom window, but
Today's HR card passes the RENDERED extent of its banked buckets rather than a
nominal window, so adding tiers below an hour reached Today as well: a morning
holding ten minutes of HR drew ten 1-minute dotted gridlines on a small card.
Labels are thinned by an overlap skip; the gridlines are not.

The sub-hour tiers now sit behind deepZoom, off by default and set only by the
surface that actually zooms. That also makes the retained comment true again,
the one about thresholds sitting below the nominal Today windows so a window
covering slightly less than nominal still lands on its intended interval.

Swift keys that on zoomBounds rather than zoomDomain, for the reason
OverviewHRChart already records twice: zoomBounds is set once by the zooming
call sites, while zoomDomain stays nil until the first pinch, so keying on it
would hand a not-yet-zoomed Deep Timeline the static tiers and then switch them
under the user (#829).

The Swift function now returns dates and no longer builds a DateFormatter per
call. Every label it formatted was discarded by the caller's map, once per body
evaluation, which during a pinch is once per frame. Its doc no longer claims the
two platforms read identically: positions are shared, labels are not, because
Swift draws them with AxisValueLabel() in the viewer's locale where the Kotlin
side formats HH:mm itself.

Tests: the three deep-zoom tiers now pass the flag, and both platforms pin the
Today guarantee. ChartTimeTicksTests is new, because eight thresholds written
twice in two languages drift, and a drift between them is invisible in a build.
llmassisted pushed a commit to llmassisted/noop that referenced this pull request Oct 1, 2026
chartTimeTicks is shared. The Deep Timeline passes its pinch-zoom window, but
Today's HR card passes the RENDERED extent of its banked buckets rather than a
nominal window, so adding tiers below an hour reached Today as well: a morning
holding ten minutes of HR drew ten 1-minute dotted gridlines on a small card.
Labels are thinned by an overlap skip; the gridlines are not.

The sub-hour tiers now sit behind deepZoom, off by default and set only by the
surface that actually zooms. That also makes the retained comment true again,
the one about thresholds sitting below the nominal Today windows so a window
covering slightly less than nominal still lands on its intended interval.

Swift keys that on zoomBounds rather than zoomDomain, for the reason
OverviewHRChart already records twice: zoomBounds is set once by the zooming
call sites, while zoomDomain stays nil until the first pinch, so keying on it
would hand a not-yet-zoomed Deep Timeline the static tiers and then switch them
under the user (ryanbr#829).

The Swift function now returns dates and no longer builds a DateFormatter per
call. Every label it formatted was discarded by the caller's map, once per body
evaluation, which during a pinch is once per frame. Its doc no longer claims the
two platforms read identically: positions are shared, labels are not, because
Swift draws them with AxisValueLabel() in the viewer's locale where the Kotlin
side formats HH:mm itself.

Tests: the three deep-zoom tiers now pass the flag, and both platforms pin the
Today guarantee. ChartTimeTicksTests is new, because eight thresholds written
twice in two languages drift, and a drift between them is invisible in a build.

(cherry picked from commit 1e8ef1e)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant