Skip to content

charts: make every scrubbable graph read like the screen around it - #1664

Merged
ryanbr merged 1 commit into
mainfrom
fix/1662-chart-value-format
Aug 26, 2026
Merged

ryanbr merged 1 commit into
mainfrom
fix/1662-chart-value-format

Conversation

@ryanbr

@ryanbr ryanbr commented Aug 26, 2026 •

Copy link
Copy Markdown
Owner

Reported by @bartmuskala in #1662.

What was wrong

Tapping a graph printed a different number from the metric written beside it. Their screenshot shows a rounded headline over a one-decimal chart.

LineChart takes formatValue: ((Double) -> String)? = null. When it's null the scrub read-out falls back to formatLineValue, which rounds to an integer only if the value is within 0.05 of one and otherwise prints %.1f. The Vital Signs detail screen never passed it — even though detail.format is right there and drives the Min/Avg/Max row two lines below the chart. Same screen, two formatters, no shared source.

Their list is the accurate half of a clean split. Every metric formatted it.roundToInt().toString() disagreed with its own graph — recovery, rhr, hrv, rest, fitness_age, vitality, vo2max_est, steps_est, active_kcal. The three that already agreed (strain, resp, skin) were the ones never rounded.

This is the unfinished half of #463

formatValue exists because of that report: Trends' Effort chart tapped to the stored 0–100 figure beside a 0–21 axis. The parameter was added and only that call site was wired up. Three of twelve ever passed a formatter; the rest kept the default. So this isn't a new defect, it's the same one on the screens #463 never reached.

The change

Seven call sites, each given the formatter its own screen already uses beside the chart:

screen formatter
Vital Signs detail (reported) detail.format + unit
Explore MetricSpec.format
Stress one decimal, matching the Today/Avg/Peak footer
Sleep metric sheet spec.format + unit
Sleep hours ×2 hours with a unit
Sleep debt a duration (7h 20m)

BarChart had no formatValue parameter at all — it hardcoded null — so its three scrubbable call sites couldn't be fixed from outside. Added, with the same default and fallback as LineChart's.

The rule I applied, since the app was already split on it

Each chart's scrub label matches the read-out beside that chart — including the unit when that read-out carries one. That is why the vital detail scrubs 62 ms and Stress scrubs a bare 2.0.

Worth stating because the existing sites disagree: TodayScreen and SleepScreen include units (72 bpm), TrendsScreen does not (its formatY is an axis formatter reused for the label, and that chart carries several metrics). I did not touch TrendsScreen to force uniformity — a shared axis formatter has a good reason not to carry a unit, and the reporter's complaint was about a graph disagreeing with the screen it sits on, not about cross-screen consistency.

Explore was worse than a decimal

Its values are the raw stored numbers, and MetricSpec.format is what rescales Effort to WHOOP's 0–21 axis. The Y labels and the hero went through it; the scrub read-out did not. On that scale the axis said 12.4 and tapping the same point said 59 — a different value, not a different precision. That is the #463 failure verbatim, still live on that screen.

A consequence worth naming

A rounded metric now reads rounded on tap: HRV scrubs as 62 ms where it used to say 62.4. That is the alignment asked for, and the stored value is no more trustworthy than the headline implies — but it is a visible loss of detail, not a pure tidy-up. Someone will notice.

What I did not change

The shared default. A chart with genuinely fractional data still wants its decimal, and after this no scrubbable chart reaches the fallback — verified by walking all twelve call sites, not by assuming. Changing formatLineValue would also alter charts where the label is never drawn.

The reporter's second point — that Effort deserves a decimal on the 0–21 scale — is already true (LiquidTodayView does decimals: effortScale == .whoop ? 1 : 0, and #1660 extended the same rule to the trends report). The ask there was consistency, not new precision.

"stress" appears in their list but there's no stress key in the vital-detail catalogue, so that's a different surface. The Stress screen's own chart is fixed here; if they meant something else I'd want to see which screen.

iOS parity: the same parameter, wired everywhere

I first assumed iOS was safe because TrendChart.valueFormat defaults to a rounded int, so a forgotten formatter agrees with a rounded headline rather than contradicting it. That's true about the default and wrong about the reason — iOS never reaches it. Auditing every call site:

  • TrendChart: 7 of 7 production sites pass valueFormat
  • OverviewHRChart: 2 of 2 (the third is a #Preview with synthetic sin() data)

So Apple has no instance of this in either direction, not because the fallback is kinder but because the wiring was finished. Android was at 3 of 12. This PR brings every scrubbable chart across.

A useful confirmation fell out of it: iOS's Stress chart already passes valueFormat: { String(format: "%.1f", $0) } — the exact formatter this PR gives Android's Stress chart. The Android choice converges on shipped iOS behaviour rather than inventing one.

Verification

6 new tests pinning the divergence beside each fix, so a future caller that forgets one has a written statement of what goes wrong:

lineChartSelectionLabel(62.4, null)         == "62.4"     // what the graph said
lineChartSelectionLabel(62.4, roundedMetric) == "62 ms"   // what the screen says
lineChartSelectionLabel(2.0, null)          == "2"        // near-integer collapse
lineChartSelectionLabel(2.0, stressScore)   == "2.0"      // the footer's reading
lineChartSelectionLabel(59.0, null)         == "59"       // Explore, raw
lineChartSelectionLabel(59.0, toWhoopScale) == "12.4"     // Explore, the axis

Android 4516 tests green, compileFullDebugKotlin clean, doc_comment_lint and i18n_audit --ci clean. Android-only, so no app-build run is needed. Not hardware-dependent; no BLE path touched.

Tapping a graph printed a different number from the metric written beside it:
a rounded headline over a one-decimal chart. Reported against #1662.

This is the unfinished half of #463. `formatValue` exists BECAUSE of that
report -- Trends' Effort chart tapped to the stored 0-100 figure beside a 0-21
axis -- but only that call site was wired up. Every other scrubbable chart
kept lineChartSelectionLabel's fallback, which collapses near-integers and
otherwise prints one decimal. Three of twelve call sites passed a formatter.

Fixed at seven call sites, each given the formatter its own screen already
uses for the read-out beside the chart:

  - Vital Signs detail: detail.format + unit (the reported screen)
  - Explore: MetricSpec.format
  - Stress: one decimal, matching the Today/Avg/Peak footer
  - Sleep metric sheet: spec.format + unit
  - three Sleep cards: hours with a unit, and debt as a duration

BarChart had no formatValue parameter at all -- it hardcoded null -- so its
three scrubbable call sites could not be fixed from the outside. Added, with
the same default and the same fallback as LineChart's.

Explore was worse than a decimal. Its `values` are the RAW stored numbers and
MetricSpec.format is what rescales Effort to the WHOOP 0-21 axis. The Y labels
and the hero went through it; the scrub read-out did not. On that scale the
axis said 12.4 and tapping the same point said 59 -- the #463 failure exactly,
unfixed on that screen.

Consequence worth naming: a rounded metric now READS rounded on tap. HRV
scrubs as "62 ms" where it said "62.4". That is the alignment asked for, and
the stored value is not more trustworthy than the headline implies, but it is
a visible loss of detail rather than a pure tidy-up.

Left alone: the shared default. A chart with genuinely fractional data still
wants its decimal, and after this no scrubbable chart reaches the fallback --
verified by walking all twelve call sites, not by assumption.

iOS is unaffected: TrendChart.valueFormat defaults to a ROUNDED int, so a
forgotten formatter there agrees with a rounded headline instead of
contradicting it. Same class of omission, opposite default, no visible bug.

Verification: 6 new tests pinning the divergence beside each fix. Android 4516
tests green, compileFullDebugKotlin clean, doc_comment_lint and i18n_audit
clean. Not hardware-dependent.
@ryanbr ryanbr closed this Aug 26, 2026
@ryanbr ryanbr reopened this Aug 26, 2026
@ryanbr
ryanbr merged commit 541a4c3 into main Aug 26, 2026
4 checks passed
@ryanbr
ryanbr deleted the fix/1662-chart-value-format branch August 26, 2026 22:47
ryanbr added a commit that referenced this pull request Aug 26, 2026
The first version changed Android's DETAIL and iOS's TILE. Those are not the
same surface, and both platforms have both, so it left each one's screen
disagreeing with its own drill-in -- tapping an Android tile went from
"+0.2 dC" to "34.6 C", and iOS went the other way. That is the #1664 defect I
merged an hour earlier, reintroduced.

Now the TILE on both. It is the surface both platforms share, both can read
from skinTempC, and it has no range picker -- which matters: the detail's range
chips key off the displayed series, and a 21-day absolute history would have
locked M/3M/6M/1Y for anyone with real history, permanently, because only the
trailing 21 days are ever re-scored.

Reverts the Android detail (HealthScreen.kt is byte-identical to main again)
and moves the pure seams beside the tile logic they now serve.

The rule is the same on both platforms: resolve the displayed night, then lead
with its absolute if it has one. Asking about the row already being shown is
what stops the tile stepping back to an older night -- an import-only night
carries a deviation and no absolute.

A CALIBRATING night is the reverse, and is a real win: recomputeSkinTempDev
returns null until the baseline is usable (~4 nights) while the absolute is
already measured, so those wearers read "needs ~4 worn nights" today with a
real temperature sitting unshown behind it.

Also fixes a bug in the first version's iOS secondary: it reached for the
freshest deviation anywhere, so on a calibrating night it would have printed a
PREVIOUS night's number under tonight's temperature. Bound to the displayed
night.

Banding, the sparkline, the range caption and the previous-value delta all now
follow whichever series is displayed, on both platforms.

Android 4526 tests green (10 Kotlin + 5 Swift for this), compile clean,
doc_comment_lint and i18n_audit clean.

Not addressed: the DETAIL screens. Android's reads DailyMetric and iOS's reads
the skin_temp metric series, which carries no absolute -- a data-path change
and its own concern.
ryanbr added a commit that referenced this pull request Aug 26, 2026
…1665)

The tile only ever showed a deviation, and a deviation with no anchor cannot
be read: "+0.9" is a fever or a warm bedroom and nothing says which. The
reporter's flu night read "+0.94 dF", which looks like nothing, against 96.4 F
on a 24-night mean of 94.4 F. Requested by @justinjor-bit in #1636.

The display half; #1663 persisted the value.

Scoped to the TILE on both platforms. An earlier draft changed Android's
DETAIL and iOS's TILE -- not the same surface -- which left each platform's
screen disagreeing with its own drill-in, the #1664 defect reintroduced. The
tile is what both platforms share, both can read from skinTempC, and it has no
range picker: the detail's range chips key off the displayed series, and a
21-day absolute history would have locked M/3M/6M/1Y permanently, since only
the trailing 21 days are ever re-scored.

Same rule on both: resolve the displayed night, then lead with its absolute if
it has one. Asking about the row already shown is what stops the tile stepping
back to an older night when the freshest is import-only.

A calibrating night is the reverse and is a real win: recomputeSkinTempDev
returns null until the baseline is usable (~4 nights) while the absolute is
already measured, so those wearers read "needs ~4 worn nights" today with a
real temperature sitting unshown behind it.

iOS gains a `secondary` slot kept distinct from `caveat` -- caveat says a
reading is unreliable, secondary says what it means. Kotlin gains the twin.
Banding, sparkline, range caption and the previous-value delta all follow
whichever series is displayed.

Verified: 4526 Android tests, 10 Kotlin + 5 Swift for this change, app-build
green on both legs with StrandTests executed.
ryanbr added a commit that referenced this pull request Aug 30, 2026
…#1740)

Two residuals from #1662, reported by @bartmuskala after #1664 landed.

PRECISION. Android formatted vo2max_est with roundToInt while the line was plotted at full precision, so the labels could not move with it: VO2max shifts well under 1 ml/kg between weekly points, so the trend visibly sloped while the scrub read-out, the Min/Avg/Max row and the readings table all printed the same integer. #1664 made those text surfaces agree with EACH OTHER; it could not make them agree with the line, because the precision was too coarse to express what the line draws.

A parity bug rather than a new policy: the iOS MetricCatalog already declares `decimals: 1` for both VO2max keys. Checked and deliberately left rounded are the Today card and the Health StatePill, because iOS's unitText defaults to `decimals: 0` and its card does not override it - summary-integer / detail-decimal is the split iOS already ships. Locale.US matches the chart's own formatLineValue fallback, so no separator changes for anyone.

THE BREAK. The VO2max line is split on purpose wherever the attribution source changes, so two non-adjacent Nes runs are never joined across an incompatible Uth stretch. Nothing said so, and a silent gap in a trend is indistinguishable from a rendering fault - it was reported as "something weird with a broken line". A footnote now appears only when a break exists, derived from vo2MaxTrendSegmentIds rather than recomputed so the caption and the segmentation cannot disagree.

Three review passes, and the last two found things no gate could reach.

The iOS caption was first placed beside the chart, inside a closure ChartCard applies `chart().frame(height:)` to - so the line and the text would have shared one fixed height, the chart shrinking to make room for the sentence explaining it. Valid Swift, and app-build was green on it; only reading the container shows it. It moved to the footer, the one full-height slot under the chart. Android keeps its caption under the plot because its card has no such frame, and the reason is recorded at the site so the asymmetry does not read as an oversight.

The caption then claimed more than it could know. The line splits on attribution SOURCE, and an untagged legacy reading resolves to "estimator:unknown" - vo2MaxAttributionSource says a missing tag must never inherit today's method - so an unknown -> Nes transition breaks the line exactly like a real method change while the method may never have changed. "the estimation method changed" asserted the one thing that data cannot establish; it now reads "changed or was not recorded". vo2MaxTrendHasMethodChange became vo2MaxTrendHasBreak on both platforms: a predicate whose name over-claims is how the caption came to over-claim.

Strings land in all six Android locales including pl and zh, and all ten iOS. Polish is currently 100% complete, so omitting it would have regressed a finished locale. The xcstrings entry was written and later replaced as TEXT, keeping the diff to two lines rather than a re-serialised file.

6 tests on the break predicate, including the untagged case that justifies the wording. 4845 Android tests green, i18n audit and doc-comment lint clean, app-build green on macOS and iOS.
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