Repository navigation
A 5/MG never sees the steps-calibration prompt, coefficient or not - #1582
Merged
Merged
Conversation
Follow-up to #1579, which stopped a partial sample-day count activating the 4.0-only calibration affordance on a WHOOP 5.0. The coefficient paths could still activate it, and those are profile-global — so a user who calibrated a 4.0 and later moved to a 5.0 kept getting the 4.0 prompt on any day the 5.0 logged no steps and no estimate existed. Same complaint #1523 opened, on a narrower trigger. @kavemang justified keeping those paths as preserving estimate behaviour across that migration. That reasoning does not apply to THIS gate: `estSteps` comes from `stepsEstByDay` and is computed independently — the gate is only consulted for `realSteps == nil && estSteps == nil`, so all it decides is whether a BLANK tile offers to calibrate. A strap that reports steps natively has nothing to calibrate. So a positively-identified 5/MG now returns false before anything else is considered. Android has been immune this way all along: `stepsCalibrationPrompt` returns early on `model != WhoopModel.WHOOP4.name` before reading any calibration state. The coefficient paths are NOT deleted, which is the tempting reading. A legacy 4.0 owner whose `selectedWhoopModel` key was never written has no family to match on, and only the coefficient says they are mid-estimate; dropping them would silently take the calibration gear away from exactly the users #1491 restored it for. A new case pins that. Two of the six cases from #1579 flip and are renamed to say what they now assert, with the reason recorded on them rather than in a commit nobody will read again. Replayed the whole matrix before changing anything: only those two move. Builds on @kavemang's diagnosis and their pure-function extraction, which is what made this testable at all.
Re-review. `family == .whoop5` is behaviourally correct today — WhoopModel has exactly two cases, so it IS the set of positively-identified non-4.0 straps — but it encodes the wrong thing. The rule is "a strap we have positively identified as something other than a 4.0 has nothing to calibrate", and naming a specific family only happens to express that while the enum has two members. Add a third model and the version naming `.whoop5` silently stops excluding it: the new strap falls through to the coefficient paths and gets the 4.0 prompt — the exact bug this PR exists to close, reintroduced by an enum case. `if let family, family != .whoop4` says the rule directly, is the same set today (replayed all eight cases, zero differ), and is the same way round as the Android twin, which returns early on `model != WhoopModel.WHOOP4.name`. The `family == .whoop4 && hasDayData` clause below is NOT now redundant: past that guard family is either nil or .whoop4, and the two must still be told apart — nil is the legacy owner with no recorded model, who reaches the affordance only via a coefficient.
ryanbr
pushed a commit
that referenced
this pull request
Aug 24, 2026
Gives Android's blank, uncalibrated WHOOP 4.0 Steps tile a direct route to Steps calibration. The tile always opened `vital_detail/steps_est`, even when its own caption was telling the user to connect phone steps before an estimate could exist — a dead end pointing at the trend for a metric that has no trend yet. The actionable empty state now opens calibration; measured, imported, estimated, calibrated-quiet and WHOOP 5.0 / MG states keep the trend destination. Steps calibration also becomes one canonical nested NavHost destination: Today pushes it from the blank tile, Settings pushes the same destination from its Steps estimate row, and the Settings-local full-screen dialog instance is gone. The routing predicate inherits its 4.0-only gating from `stepsCalibrationPrompt` rather than re-deriving it — which is why a 5/MG keeps the trend destination without this needing its own model check, and why it composes with the gate changes in #1579 and #1582 instead of drifting from them. This brings Android to what iOS already does: the iOS Steps tile shows a calibration button on `isEstimated || needsCalibration` that opens "the SAME one Settings hosts". Android had no route at all from a blank tile. Verified on the PR head rather than from the description: full Android suite 4281 tests 0 failures, the new StepsTileNavigationTest 4 of 4, StepsCalibrationScreen's signature matches the new call site exactly, the Settings-local dialog left zero orphaned references, and `drawerGroups` is an explicit list so the new Destination case cannot leak into the More drawer. doc-comments and i18n clean. Not verified interactively — the contributor had no device or emulator, and neither did I. Back behaviour from the two push sites is reasoned, not observed. Thanks to @kavemang.
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.
Follow-up to #1579, which I flagged when reviewing it and recorded in that merge commit.
#1579 stopped a partial sample-day count activating the 4.0-only calibration affordance on a WHOOP
5.0. The coefficient paths could still activate it, and those are profile-global — so a user who
calibrated a 4.0 and later moved to a 5.0 kept getting the 4.0 prompt on any day the 5.0 logged no
steps and no estimate existed. That is the complaint issue #1523 opened, on a narrower trigger.
Why the original justification doesn't hold
@kavemang kept those paths to preserve estimate behaviour across that migration. Reasonable on its
face — but this gate doesn't control the estimate:
estStepscomes fromstepsEstByDay[selectedDayKey](TodayView.swift:3737), computedindependently;
needsCalibration = realSteps == nil && estSteps == nil && stepsPipelineActive(…).So all it decides is whether a blank tile offers to calibrate. A strap that reports steps natively
has nothing to calibrate.
The change
A positively-identified 5/MG returns
falsebefore anything else is considered.Android has been immune this way all along —
stepsCalibrationPromptreturns early onmodel != WhoopModel.WHOOP4.namebefore reading any calibration state. This brings iOS to the sameguarantee while keeping its more permissive fallback for unknown straps.
What is deliberately NOT changed
The coefficient paths are not deleted, which is the tempting reading of the above. A legacy 4.0
owner whose
selectedWhoopModelkey was never written has no family to match on, and only thecoefficient says they are mid-estimate. Dropping them would silently take the calibration gear away
from exactly the users #1491 restored it for. A new case pins that:
Matrix, replayed before changing anything
Only the two intended cases move. Those two tests from #1579 are renamed to say what they now assert,
with the reasoning recorded on them rather than buried in a commit message.
Verification
doc_comment_lint.pyOK. No strings, no Android change, no schema.TodayView.swiftis app-target Swift and nothing else compiles it; that ishow Fix WHOOP 5 steps calibration prompt (#1523) #1579's own tests went unrun until merge. Not dispatched.
Credit to @kavemang: the diagnosis and the pure-function extraction in #1579 are what made this
testable at all — the gate was previously unreachable from any test.