fix(health): feed the Android body clock, closing a live cross-platform gap - #855
Merged
Conversation
…rm gap (#852) Body Clock has shipped on iOS/macOS all along: AppModel.computeCircadianPhase pools HR by local hour-of-day as the activity proxy, fits the cosinor, publishes circadianPhase, and SkinTempCardsView renders it. Android has a byte-for-byte twin of the engine, its own full test suite and a finished BodyClockCard — and never fed any of it. V5HealthSignals hardcoded `bodyClock = null`, so HealthScreen's `?.let` skipped the card on every device. The comment that guarded it was wrong: it said we lack "per-hour rest-activity bins we don't bank here". Swift does not bank them either — it derives them from HR buckets Android also has. The pipe was never built on this side, not missing. Mirrors the Swift maths exactly: 3600 s buckets over 14 days, the >= 24-bucket floor, pooling by LOCAL hour, mean bpm per populated hour, distinct local days as daysObserved, and the >= 6-bin floor before a fit is attempted. The binning lives in AppViewModel rather than CircadianEngine precisely because Swift keeps it in AppModel — the engine stays an untouched byte-for-byte mirror on both platforms. Its pure half is a top-level `circadianBinsFrom` so it is unit-testable with no Context, no ViewModel and no database; six tests pin the floors, local-hour pooling, mean-not-sum, the negative-offset wrap and sparse coverage. Every expectation was cross-checked against an independent implementation before commit. Both new parameters default, so every existing evaluate() caller is byte-identical. The synchronous settings-toggle re-evaluate reuses the collector pass's cached bins rather than blanking the estimate — it has no coroutine to read the store from. KNOWN DIVERGENCE, deliberately not closed here: Swift derives habitualWakeHour from the most recent banked wake and falls back to 07:00; Kotlin has no such helper, so this uses the existing 7.0 default. That shifts offsetVsScheduleMinutes for anyone who does not wake at 07:00. Worth a follow-up; adding a wake-time derivation is a separate change. Does NOT settle whether this should ship at all. Without an observedTempMinHour the estimate derives the CBT minimum as `activityAcrophase - 12.0h`, a population constant, and the confidence bands key on data sufficiency rather than on whether that constant holds for this person. That question is about the ALREADY-SHIPPED Apple card; leaving Android silently inert was not an answer to it.
Re-reviewing my own PR: the Swift twin's repo.hrBuckets(from:to:bucketSeconds:) UNIONs the active strap with the canonical "my-whoop" and dedupes by bucket start. I called Kotlin's single-id hrBuckets, so after a strap re-add Android would read strictly less data than Apple — plausibly missing the >= 24-bucket floor and rendering no estimate where Swift renders one. hrBucketsUnion is documented as the twin of exactly that overload. Single-WHOOP install resolves to one id either way, so nothing changes for most users; the divergence only appears on the multi-namespace case the read spine exists for. A parity bug in the parity fix.
A plain var written from a coroutine and read from a UI callback looks like a race, and a reviewer would reasonably stop on it. It is not one: the write resumes on viewModelScope (Dispatchers.Main.immediate) and the read is a main-thread settings callback, so both touch it on the main thread. The Swift twin gets the same guarantee from @mainactor on AppModel. Recording the reasoning rather than making the next reader re-derive it.
java.util.TimeZone sorted before kotlin.math.roundToInt, matching the java-then-kotlin order used elsewhere in the tree (HealthConnectImporter, DataBackup). Stopped round-tripping the clock: it divided currentTimeMillis to seconds and multiplied back to look up the timezone offset. Keeps nowMs and uses it directly. No behaviour change — the offset is identical anywhere inside the same second.
Owner
Author
|
Cleared the two nits from the last pass, both cosmetic:
Also documented, from the pass before: Both gates green. Nothing else outstanding from my side — the two limitations in the description ( |
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.
Closes #852. Supersedes #853, which I closed — that PR proposed deleting this, on the mistaken belief there was no Swift twin.
The gap
Body Clock has shipped on iOS/macOS all along.
AppModel.computeCircadianPhasepools HR by local hour-of-day as the activity proxy, fits the cosinor, publishescircadianPhase, andSkinTempCardsViewrenders it.Android has a byte-for-byte twin of the engine, its own full test suite, and a finished
BodyClockCard— and never fed any of it.V5HealthSignalshardcodedbodyClock = null, soHealthScreen's?.letskipped the card on every device.The comment guarding it was wrong. It said we lack "per-hour rest-activity bins we don't bank here". Swift doesn't bank them either — it derives them from HR buckets Android also has. The pipe was never built on this side, not missing.
The change
Two hunks, ~30 lines. Mirrors the Swift maths exactly: 3600 s buckets over 14 days, the
>= 24-bucket floor, pooling by local hour, mean bpm per populated hour, distinct local days asdaysObserved, and the>= 6-bin floor before a fit is attempted.The binning lives in
AppViewModel, notCircadianEngine, because Swift keeps it inAppModel— the engine stays an untouched byte-for-byte mirror on both platforms, which is the point.Its pure half is a top-level
circadianBinsFromso it is testable with no Context, no ViewModel and no database.Verification
Six tests pin the floors, local-hour pooling, mean-not-sum, the negative-offset wrap and sparse coverage. Every expectation was cross-checked against an independent implementation of the algorithm before commit:
Both new parameters default, so every existing
evaluate()caller is byte-identical. The synchronous settings-toggle re-evaluate reuses the collector pass's cached bins rather than blanking the estimate — it has no coroutine to read the store from.Known divergence, deliberately not closed here
Swift derives
habitualWakeHourfrom the most recent banked wake, falling back to 07:00. Kotlin has no such helper, so this uses the existing7.0default. That shiftsoffsetVsScheduleMinutesfor anyone who doesn't wake at 07:00 — real and bounded, worth a follow-up, but a wake-time derivation is a separate change.What this does NOT settle
Whether this should ship at all. Without an
observedTempMinHour,estimatePhasederives the CBT minimum asactivityAcrophase − 12.0h— a population constant — and the confidence bands key on data sufficiency, not on whether that constant holds for this person.That question is about the already-shipped Apple card. Leaving Android silently inert was never an answer to it, which is why this lands first: it makes the platforms consistent so the real decision can be made once, for both.
Not compiled —
android.ymlis disabled.