HRV: default Overnight only ON for fresh installs (#1008, minimum half) - #1019
Merged
Conversation
WHOOP publishes no daytime HRV figure - its reading is an overnight one - so a 24/7 stream has no official-app analogue, and the setting's own copy says overnight-only roughly halves the battery cost. Turning on "Continuous HRV capture" currently gives you the expensive, non-WHOOP-like behaviour unless you separately find "Overnight only", which is the wrong way round. EXISTING USERS ARE UNCHANGED. The unset case resolves from whether the base Continuous HRV key exists: present means the user has been through this screen and experienced always-on, so they keep it; absent means a fresh install, which gets overnight-only. That preserves the deliberate choice the #927 comment records ("Default OFF, so existing Continuous HRV users keep the always-on behaviour with no migration") while changing what new users get. Resolved at read time rather than by writing a migration, because the only thing that must not happen is silently narrowing capture for someone relying on it for daytime Stress - and a migration that runs at the wrong moment does exactly that. An explicit choice always wins in both directions, including an explicit OFF on a fresh install. This is the SAFE half of #1008. The other half - defaulting the HRV window to DEEP_SLEEP to match WHOOP's reading - is deliberately not here: it moves every existing HRV number and its baselines, and it depends on deep-sleep staging quality that is still unsettled on 5/MG. That needs SleepPSG evidence first. Rule lifted into continuousHrvOvernightDefault on both platforms so it is testable without a Context or UserDefaults. Four twin tests each; the Kotlin ones run in CI, the Swift ones live in StrandTests which app-build.yml would run if it were enabled.
The first version of this resolved the default at READ time, keyed on whether Continuous HRV had ever been enabled. That fact is created by the user's own opt-in, so a fresh install read overnight-only ON and then flipped to OFF the moment they enabled Continuous HRV - the exact opposite of the intent. State-by-state tests could not see it: every individual state was correct, and only the sequence was wrong. Replaced with a one-time launch migration on both platforms: an install that has used Continuous HRV and never chose an overnight setting is pinned to the old OFF; everything else is left alone and takes the new ON default. Taken before the user can reach either toggle, so the inputs cannot move under it. A @AppStorage onChange hook cannot substitute on iOS - @AppStorage writes the value BEFORE the handler runs, so a first-ever toggle is indistinguishable from any other by then. That asymmetry is why this is a launch migration rather than something wired to the toggle. The extracted rule now describes the MIGRATION decision rather than the read, because the read is simply getBoolean(key, true) - the previous extraction had become dead code describing the broken design, and its tests were exercising that dead code rather than production. Five twin tests each, including the sequence that broke it and idempotence.
The replacement added shouldPinLegacyOvernightDefault but left continuousHrvOvernightDefault behind on the Swift side - dead code whose doc describes the design this PR abandoned, and points at a Kotlin twin that no longer exists. Worse than ordinary dead code: it documents the read-time approach as though it were current, which is the specific mistake the migration exists to prevent someone repeating. Found by grepping for stale references after the rewrite; nothing referenced it, so removal is inert.
Changing the read default left SettingsView's @AppStorage on false, so a fresh install would show "Overnight only" OFF while capture was actually overnight-only. They read the same key by different routes. The failure mode is worse than a wrong label: a user "correcting" the toggle by flipping it on and off would write an explicit false and end up with the 24/7 behaviour they were trying to avoid. Android was never affected - its toggle reads through NoopPrefs.continuousHrvOvernight, so it cannot disagree with what the BLE client acts on. Its stale "Default OFF" comment is corrected too. Checked the rest of SettingsView for the same class: journalReminderEnabled and experimentalSleepV2Enabled are the only other true-default toggles, and both already pair with an accessor that handles the unset case - the first with the exact `object(forKey:) as? Bool ?? true` spelling used here. So this now matches how the codebase already solves it, and no other instance exists.
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.
The safe half of #1008. No existing user's behaviour changes.
What
WHOOP publishes no daytime HRV figure — its reading is an overnight one. So a 24/7 stream has no official-app analogue, and the setting's own copy says overnight-only "roughly halv[es] the battery cost".
Today, turning on Continuous HRV capture gives you the expensive, non-WHOOP-like behaviour unless you separately find and enable Overnight only. That is the wrong way round: the cheaper, comparable option should be what you get, and the expensive one should be the deliberate choice.
Existing users are unchanged, and that is the whole design
A one-time launch migration pins the old default for anyone already running it:
It runs before the user can reach either toggle, and it is idempotent, so it is a no-op on every launch after the first and on every fresh install.
Why a migration and not a read-time default
I tried the read-time version first and it was wrong. It keyed the default on "has ever enabled Continuous HRV" — a fact the user's own opt-in creates. So a fresh install read overnight-only ON, and then flipped to OFF the moment they enabled Continuous HRV: the exact opposite of the intent.
State-by-state tests could not see it. Every individual state was correct; only the sequence was wrong. There is now a test for that sequence specifically.
A
@AppStorageonChangehook cannot substitute on iOS either —@AppStoragewrites the value before the handler runs, so by then a first-ever toggle is indistinguishable from any other. That asymmetry is why this is a launch migration rather than something wired to the toggle.What is deliberately NOT here
The other half of #1008 — defaulting
HrvWindowtoDEEP_SLEEPto match WHOOP's reading. Two reasons, both in the issue:setHrvWindow's own comment says it "re-scores + re-baselines (the value itself moves)". Flipping it moves every existing HRV number and everything built on it.DEEP_SLEEPpools RMSSD over deep-sleep windows, so it is only as trustworthy as deep-sleep detection — which changed in Sleep: forbid wake → deep/REM transitions — the one part of #348 that survives a de-contaminated benchmark #987 and is still unsettled on 5/MG. It needsTools/SleepPSGevidence before it can be a default, and possibly needs to be family-aware rather than global.Parity
Both platforms, same rule, same five test cases in the same order. Android's UI
NoopPrefsand iOS'sPuffinExperimenteach keep their own storage; only the decision is shared in shape.UserDefaults.bool(forKey:)cannot express the new default on its own — it returnsfalsefor a missing key, which is indistinguishable from an explicit off — so the read isobject(forKey:) as? Bool ?? true, and the migration checksobject(forKey:)for presence, mirroring Android'scontains().The migration is wired into
StrandiOSApp.init()andStrandApp.init()(macOS). Those are app-target Swift, which nothing compiles here — parse-checked only, same limitation as the rest of the app target whileapp-build.ymlis disabled.Verification
continuousHrvOvernightDefaulton both platforms so it is testable without aContextorUserDefaults.testFullDebugUnitTest.mainworktree: 2517 errors both sides, 41 inMainActivity.ktboth sides, error sets byte-identical. No new errors.swiftc -parseclean onPuffinExperiment.swiftand the new test.Tools/doc_comment_lint.pyclean.StrandTests, which only runs underxcodebuildon macOS — andapp-build.ymlis disabled, so they are not executed by CI today. Stated in the test file itself so nobody reads them as covered.Note for the release
No stored value changes and no score moves, so this does not belong under "scores that change". It is worth one line in the notes for new users: enabling Continuous HRV now captures overnight by default, and the all-day behaviour is one toggle away.
Re-review: the iOS toggle and the behaviour disagreed
Changing the read default left
SettingsView's@AppStorageonfalse. Same key, two routes — so a fresh install would show "Overnight only" OFF while capture was actually overnight-only.The failure mode is worse than a wrong label: a user "correcting" the toggle by flipping it on and off would write an explicit
falseand end up with exactly the 24/7 behaviour they were trying to avoid.Android was never affected — its toggle reads through
NoopPrefs.continuousHrvOvernight, so it cannot disagree with what the BLE client acts on. That is the better shape, and it is why the bug was iOS-only.I then checked the rest of
SettingsViewfor the same class.journalReminderEnabledandexperimentalSleepV2Enabledare the only othertrue-default toggles, and both already pair with an accessor that handles the unset case — the first with the exactobject(forKey:) as? Bool ?? truespelling used here. So the fix matches how the codebase already solves this twice, and there is no other instance.