Repository navigation
fix(charts): stop drawing every metric as if it were maximally volatile - #2004
Merged
Merged
Conversation
ryanbr
force-pushed
the
fix/vital-chart-scale-and-gaps
branch
from
September 8, 2026 10:01
0fba7e3 to
cbca2f2
Compare
Two things made the Vital Signs trends read as noise, and both come from one place: pointsFor scales to the data's own min and max, so every chart fills its full height whatever the metric actually did. A Rest series moving 46..93 on a natural 0..100 scale is drawn exactly as violently as an Effort series moving 0..42. A reader cannot tell a calm metric from a wild one, and a single low day rewrites the whole shape. So LineChart takes an optional fixed y-domain, and the metrics whose natural range IS their interesting range use it. Opt-in, so every other chart in the app is byte-identical. Deliberately not "percentages get 0..100": blood oxygen is a percentage living in 90..100 and anchoring it would flatten the signal into a line at the top. Effort anchors to 0..100 whatever the display scale, because the readings store the raw composite and only format() converts to 0..21. The second is that the line slopes smoothly through days that have no reading. The reporter's Effort had nine readings across a fortnight, so a four-day gap was drawn exactly like a one-day step. The stroke now breaks on a missing day, which says "nothing measured here" without moving or dropping a single point. It rides the segment mechanism VO2max already uses for estimator changes. The break applies only where a missing day MEANS something. Fitness Age and Vitality come from metricSeries and are written only when the engine has the inputs, so they are sparse by design: breaking on every gap would isolate every point and leave scattered dots where a trend used to be. Breaking the line makes an isolated day possible, and a segment of one point strokes nothing: a Path with a moveTo and no lineTo draws no pixels. On the reported Effort series that day is 5 Sep, the fortnight's MAXIMUM, so the chart would have hidden the highest reading while the stats row still said 42.3. An isolated point is now drawn as a marker, so no reading can be invisible. Zeros are NOT dropped. An Effort 0.0 is a real computed value from thin wear time, not a no-data sentinel: days with no data are n/a and never become readings. Only the interpolation between real points changes.
ryanbr
force-pushed
the
fix/vital-chart-scale-and-gaps
branch
from
September 8, 2026 10:03
cbca2f2 to
c98f4a2
Compare
ryanbr
added a commit
that referenced
this pull request
Sep 8, 2026
#2004 anchored "recovery", "sleep_performance" and "strain". On Android the Rest detail key is "rest": sleep_performance is the SERIES this detail reads underneath and is never a detail key here. So Rest kept auto-scaling and looked exactly as it did before, which is what the reporter saw on the build. The test asserted vitalChartYDomain("sleep_performance") and passed, because the function happily answers for a key nothing ever passes. A green test for a key that does not exist is what made this feel verified. Rest is the metric where this was invisible in the worst way: the reporter's 14 readings cover 14 consecutive days, so the gap break draws nothing different and the domain was the ONLY thing that would have changed that chart. Getting the key wrong meant Rest was byte-identical to before. The allow-list is now pinned against the REAL key list the screen dispatches on, so an anchored key that no screen sends fails the test rather than passing it.
ryanbr
added a commit
that referenced
this pull request
Sep 8, 2026
#2005) #2004 anchored "recovery", "sleep_performance" and "strain". On Android the Rest detail key is "rest": sleep_performance is the SERIES this detail reads underneath and is never a detail key here. So Rest kept auto-scaling and looked exactly as it did before. Rest is where that was invisible in the worst way. The reporter's 14 readings cover 14 consecutive days, so the gap break draws nothing different and the domain was the ONLY thing that would have changed that chart. Getting the key wrong left it byte-identical. The test asserted vitalChartYDomain("sleep_performance") and passed, because the function answers for any key nothing ever sends. A green test for a key that does not exist is what made this feel verified. The allow-list is now pinned against the real key list the screen dispatches on, so an anchored key that no screen sends fails rather than passes. That mirror is hand-maintained, which the test says out loud: it catches a key nothing sends, not a newly added metric.
ryanbr
added a commit
that referenced
this pull request
Sep 8, 2026
#2004 broke the line across a missing day, but points are positioned by reading index, so a gap has ZERO width. The break then reads as a chopped line and an isolated day as an orphan dot floating in space, which is what the reporter saw on the month range. The break only means something once the gap has width. LineChart already accepted per-point timestamps for its scrub read-out. Those now position the points too, so a four-day gap occupies four days of width. Opt-in: without timestamps every existing chart keeps the even spacing it has always had. It refuses rather than guesses. A length mismatch, a zero span, or a non-ascending sequence all fall back to index spacing, and the day-key conversion is all-or-nothing, because positioning some points by time and the rest by a fallback is a worse lie than either rule applied consistently. The tap and drag hit-testing now takes the SAME fractions the geometry used. Deriving the nearest index from a uniform step, which is what it did, would have selected the wrong reading the moment spacing stopped being uniform: the chart would have highlighted one day and labelled another. That also retires the count-based helper, which nothing calls now. The positions are computed once and remembered rather than per gesture event: the drag handler fires every frame and this allocates a list the length of the series. The gesture handlers are keyed on that value too, because keying them on the values alone would let a timestamp change leave them hit-testing against stale positions. MultiLineChart keeps index spacing and says so: its series are index-aligned to each other rather than to a shared clock.
ryanbr
added a commit
that referenced
this pull request
Sep 8, 2026
…#2007) #2004 broke the line across a missing day, but points are positioned by reading index, so a gap has ZERO width. The break then reads as a chopped line and an isolated day as an orphan dot floating in space, which is what the reporter saw on the month range. The break only means something once the gap has width. LineChart already accepted per-point timestamps for its scrub read-out. Those now position the points too, so a three-day gap occupies three days of width and a run of consecutive days bunches together. Opt-in: without timestamps every existing chart keeps the even spacing it has always had. It refuses rather than guesses. A length mismatch, a zero span, or a non-ascending sequence all fall back to index spacing, and the day-key conversion is all-or-nothing, because positioning some points by time and the rest by a fallback is a worse lie than either rule applied consistently. The tap and drag hit-testing takes the SAME positions the geometry used. Deriving the nearest index from a uniform step, which is what it did, would have selected the wrong reading the moment spacing stopped being uniform: the chart would have highlighted one day and labelled another. A test pins that each point is the nearest index to its own position. That also retires the count-based helper, which nothing calls now. The positions are computed once and remembered rather than per gesture event, since the drag handler fires every frame, and the gesture handlers are keyed on that value so a timestamp change cannot leave them hit-testing stale positions. MultiLineChart keeps index spacing and says so: its series are index-aligned to each other rather than to a shared clock.
DX23876
added a commit
to DX23876/noop
that referenced
this pull request
Sep 16, 2026
Thirty real conflicts. The Android tree is dropped again per FORK_GUIDE; every other hunk was resolved by asking which mechanism this fork actually runs, not by preferring a side. Taken from upstream: • Protocol: ryanbr#2017/ryanbr#2018/ryanbr#2020 — the 5/MG historical optical and raw-IMU layouts, and the v26 window's ABSOLUTE base code beside its deltas. Its migration lands as `v57-ppg-waveform-base-code` (upstream's v44), since this fork's sequence is nine ahead of theirs. • BLE diagnostics ryanbr#1998-ryanbr#2003: a bond end that says WHY, named BLE integers, a session-budgeted hex dump, and "we hold the link" only when the OS says so. • Battery: ryanbr#1951 stop asking a 5/MG for a reading it refuses; ryanbr#1959 poll 0x2A19 for an unbonded one. • Scoring: ryanbr#1978/ryanbr#1982/ryanbr#1990 the RHR/HRV baseline gates, ryanbr#1977 the strain denominator's domain, RMSSD withheld on an over-counted night, ryanbr#1943 the resting-HR bin gate, the closed final HR/HRV window, HR-recovery requiring one CONTINUOUS effort, ryanbr#1984 nights bridged under minSleepMin, ryanbr#2015 motion re-folded only when that day's gravity moved, ryanbr#2016 a provisional Rest. • Charts ryanbr#2004/ryanbr#2005/ryanbr#2007/ryanbr#2009/ryanbr#2011 and ryanbr#1995's hero-ring detail route, ryanbr#1164's "Pending sync" caption under Rest. • Widgets: the heart-rate widget with its trace (ryanbr#1957/ryanbr#1964) and the widget cost counters. • Health: ryanbr#1503 — the Apple Health external UUID drops the strap id, so a re-pair no longer strands every prior record, plus the one-off sweep that reclaims the stranded ones; ryanbr#1205 GPS routes read back from Apple Health. • Storage: rolling retention for ppgWaveformSample, the skin-temp absolute backfill outside the 21-night window, per-table byte estimates. • Units split (body vs exercise distance), per-day wake overrides, the wind-down nudge pinned to its own day, and the German Sie→du conversion (29 entries, merged per LOCALE so the fork's own de edits stayed). Kept on the fork's side, with the reason: • ryanbr#1005's in-memory `dayScanCache` stays out, again: the persisted `DayScanFingerprint` survives relaunch and its key folds the per-day weight and max HR upstream's does not. • The steps-motion cache stays PERSISTED for the same reason. Upstream's failed-read-vs-empty-day distinction IS adopted into it — on a persisted cache a stale zero from a read error would outlive the process that made it. • Every stream read in the day loop keeps `limit: Int.max`. All five are uncapped together; taking StreamReadCap for three of them would leave the loop half-capped, which is worse than either choice. • The coach engine. Upstream's K1-K15 layer is not adopted: its single 40-row `coachMessage` table, its tool-less pre-baked context and its own brief are behind what this fork already ships. Its ADDITIVE half is kept — the on-device voice input, the Siri "Ask Coach" intent, the brief scheduler and brief widget, and streaming for the OpenAI-compatible providers this fork's Anthropic-only StreamingToolClient does not cover. They are wired up in the next commit; `CoachBriefWidget` is deliberately not registered until then. • Liquid's `DomainTheme` labels, value-sampled Charge tint, `LiquidScoreGauge` and `TodayCardSurface`; classic Today's ring order and `LiveEffort.today`. • RootTabView's four-tab shell and `MoreCatalog`: Coach hangs on content here, not on a tab. Two duplicate mechanisms collapsed rather than carried: • `ChargeBreakdownWiring` (upstream) and `ChargeBreakdownFormat` (fork) are the same extraction done twice. The fork's survives, Coupled now reads it, and upstream's TEST is kept — it is coverage this path never had. • Upstream's `CoachMessageStore` is dropped; `CoachConversationStore` already autosaves the whole multi-conversation history. One defect found while resolving, fixed here: `LiveEffort.today` read HR with no explicit limit, so `hrSamples`' 8000-row default truncated it — at ~18k rows a day that covered about ten hours and the live Effort silently stopped climbing. It failed safe (`effectiveEffort` takes the max), which is why it went unseen. `Live5RestFrozenTests` is removed by upstream, which replaces it with `Live5NoGravityRestChainTests`. `local_day_windows_oracle.json` moves out of upstream's android path into `Tests/StrandAnalyticsTests/Oracles/` — NOT `Fixtures/`, which this repo's .gitignore excludes everywhere. Verified: StrandAnalytics 2233, WhoopStore 627, WhoopProtocol 700, StrandImport 293, StrandDesign 120, NoopLocalAccess 14, SemanticMemory 19, OuraProtocol 209 and PolarProtocol 25 all green; macOS and iOS both build; doc_comment_lint clean; `i18n_audit --ci` byte-identical to its pre-merge output (it is red on main for reasons that predate this, and the seven new widget literals were given all nine locales so the gate did not get worse). Six StrandTests were ALREADY failing on main before this merge and still are — `CoachMyLogsAndZonesTests` (the tool census pin was not bumped when the Hevy tools landed) and five in `RawPhysiologyUnionTests`. Every input to them — CoachTools.swift, ToolConsent.swift, Repository.swift and both test files — is byte-identical to main, so this merge neither caused nor fixed them. They went unnoticed because app-build.yml last ran on 2026-08-25. Analysis migration required: YES. The scoring changes above make stored derived values stale; the recipe bump and its resumable migration are the next commit.
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.
Reported from the Effort and Rest detail screens: the trends look "a bit all over the place". They do, and
both causes sit in one function.
Every metric is drawn at maximum volatility
pointsForscales to the data's own min and max, so every chart fills its full height whatever themetric actually did. Rest moving 46..93 on a natural 0..100 scale is drawn exactly as violently as Effort
moving 0..42. A reader cannot tell a calm metric from a wild one, and one low day rewrites the whole shape.
LineChartnow takes an optional fixedyDomain, and the metrics whose natural range IS their interestingrange pass one. It is opt-in, so every other chart in the app stays byte-identical.
Deliberately not "percentages get 0..100":
would flatten the signal into a line at the top, strictly worse than auto-scaling.
composite and only
format()converts to the 0..21 reading. I had this backwards first and the codesaid so plainly: "Readings store the RAW 0-100 composite; only format() scales." Taking the domain from
the display scale would have squashed every Effort chart on a WHOOP-scale install into the bottom fifth
of its height.
A supplied domain can only ever change the scale, never hide a reading: it widens to contain any value
outside it rather than clipping.
The line slopes through days that were never measured
The reporter's Effort had nine readings across a fortnight, with 4 Sep and 29..31 Aug absent, and the
chart drew a four-day gap exactly like a one-day step. That is the chart asserting something it does not
know.
The stroke now breaks on a missing day. Nothing is moved and nothing is dropped: it rides the same segment
mechanism VO₂max already uses for estimator changes.
Zeros are not dropped. An Effort
0.0is a real computed value from thin wear time, not a no-datasentinel: a day with no data is
n/aand never becomes a reading at all. My first instinct was to filterthem and it was wrong. Only the interpolation between real points changes.
Not in this PR
Points are still spaced by index rather than by date (
stepX = width / (values.size - 1)), so gaps breakthe line but do not take up proportional width. That is a bigger change and belongs on its own.
The same two properties affect every other
LineChartin the app (Today HR, Stress, Apple Health, TrendsExplore, Health HR, Sleep, Trends). The mechanism is now central, so those can opt in individually once
each has been looked at on a real screen.
Tests
+9. The domain allow-list is pinned in both directions, including blood oxygen explicitly keeping
auto-scaling, and Effort anchoring to the stored scale rather than the displayed one. The gap break is
pinned on the reported shape, and on the invariant that matters most: no reading is dropped.
673 classes / 5701 tests, 0 failures. Android-only for now; the iOS charts are a separate surface.