fix(workouts): one HR device-id rule for the chart, zones and Avg HR#857
Conversation
…856) Three id resolutions were in play on one screen, and all three were wrong somewhere: Swift avg workoutHrDeviceId (single) right for detected, UNDER-READS imported Swift chart hrBuckets (day union) right for imported, reads the WRONG STRAP for a bout detected on a second WHOOP Kotlin chart single active id both failure modes at once So on iOS the Avg HR and the HR curve on the same card could describe different straps. That is not a Kotlin-vs-Swift divergence, it is a live self-inconsistency. Neither existing rule is right on its own. The correct read is source-class-dependent, which workoutHrDeviceId already encoded — it just returned one id where one case needs two: detected -> [recording strap] a union would mix in a strap that never recorded the session (#510) manual / imported -> active u canonical no strap of its own, and the worn strap may bank under either after a re-add (#814) workoutHrDeviceIds now returns that list on both platforms, and the chart, the zone read and the window aggregate all consume it. The aggregate needed real work. A naive `deviceId IN (...)` double-counts a second banked under both ids, inflating n and skewing avg — silently, because both numbers stay plausible. GROUP BY ts with MIN(pri) keeps one row per second and takes the primary's, matching the dedup the chart already did. Fixed 2-arity rather than a bound collection: importedReadIds is 1 or 2 by construction, and Room @query is static SQL that cannot express per-id priority through an IN list. Passing the same id twice is byte-identical to the old single-id read, so a single-WHOOP install is unchanged and needs no special case. That is the control test. The aggregate lives in WhoopStore, where swift-packages.yml actually runs its tests — the same placement that made #841's Swift half verifiable and its Kotlin half not. Verified against SQLite before commit, and the Kotlin @query text was extracted from source and executed to confirm it returns the same numbers as the Swift twin: A(0..9) + B(5..19), A primary -> n=20 avg=150.0 max=200 (naive IN would say 25) B primary -> n=20 avg=175.0 PPG anti-join per id -> n=15 max=130 same id twice -> identical to the pre-change read Nine tests: five existing moved to the two-id signature (same id twice, so their assertions are untouched), four added for overlap, precedence, the per-id PPG anti-join and the single-id control.
Re-reviewing this PR against what I promised on #856: "the chart, the ZONE MINUTES and hrWindowStats all read that list". Only two of the three were done. Left as it was, the card would have been NEWLY inconsistent rather than fixed — chart and Avg HR reading the recording strap while the zone split still binned someone else's samples: Swift zones hrSamples(from:to:) the day union -> wrong strap for a detected bout Kotlin zones single active id wrong for detected AND imported Both now resolve the same ids. Adds the sample-level twin of the bucket helper — Repository.hrSamples(deviceIds:) and WhoopRepository.hrSamplesFor — deduped by ts with earlier ids winning, so the zone split bins exactly the rows the curve plots. workoutZoneMinutes takes the row's source on both platforms, defaulted so a caller without a row keeps today's behaviour.
|
Re-reviewed this against what I actually promised on #856 — "the chart, the ZONE MINUTES and Zone minutes was missed, and leaving it would have been worse than not touching the screen at all. Chart and Avg HR would read the recording strap while the zone split still binned someone else's samples: That is a new three-way inconsistency on the card this PR exists to make consistent. Both now resolve the same ids, via the sample-level twin of the bucket helper — All three surfaces now go through
10/10 green, +271/-55. Worth recording how this was caught: not by re-reading the diff, but by re-reading the issue comment listing what the fix should contain and checking each item off against the code. The diff looked complete on its own terms. |
…the rename is for I renamed workoutHrDeviceId -> workoutHrDeviceIds and updated the KOTLIN resolver test while missing its SWIFT twin, which still called the old name — a compile break in StrandTests. Nothing caught it: StrandTests runs only under xcodebuild on macOS, and app-build.yml is disabled. Anyone running tests locally on a Mac would have hit it, which #854 has just made possible again. The four existing cases keep their expectations, now as single-element lists. Added the two cases none of them covered, which are the whole point of the change: an IMPORTED row on a multi-namespace install resolves to the full #814 union (the case the single-id resolver silently under-read), while a DETECTED row on that same install still resolves to exactly one id — the union must not leak into the branch where it would pull in a strap that never recorded the bout.
Asked whether this actually resolves #856 and went looking for surfaces I had not touched. Heart-rate recovery (#516) was reading the day-level union on BOTH platforms: Kotlin via hrSamplesUnion, Swift via hrSamples(from:to:). No parity break there — both were wrong the same way — but it is the same wrong-strap error as the chart, and leaving it would have recreated on a fourth surface exactly the inconsistency this PR removes from the other three. Its window extends PAST the bout, which is the one reason it might have wanted different treatment. It does not: the strap that recorded the session is still the one on the wrist a few minutes later, so a detected bout reads its own strap here too. All four workout HR surfaces now resolve through workoutHrDeviceIds on both platforms: chart, zone minutes, Avg HR / Effort, and HRR.
The previous commit added the parameter and updated Kotlin, but a failed edit meant the Swift call sites never got it. Because `source` is defaulted, that compiled and stayed green while silently keeping HRR on the old union read — a change that looks applied and is not, which is the worst shape for a defect to take. Caught by grepping the call sites rather than trusting the commit.
Closes #856.
Four surfaces on one card, no two agreeing
workoutHrDeviceId(single)So on iOS the Avg HR and the HR curve on the same card could describe different straps. That is not a Kotlin-vs-Swift divergence — it is a live self-inconsistency.
Neither existing rule is right on its own
The correct read is source-class-dependent, which
workoutHrDeviceIdalready encoded. It just returned one id where one case needs two:workoutHrDeviceIdsreturns that list on both platforms, and all four surfaces consume it:HRR's window extends past the bout, which is the one reason it might have wanted different treatment. It doesn't — the strap that recorded the session is still on the wrist a few minutes later.
The aggregate needed real work
A naive
deviceId IN (…)double-counts a second banked under both ids — inflatingn, skewingavg, silently, because both numbers stay plausible.GROUP BY tswithMIN(pri)keeps one row per second and takes the primary's, matching the dedup the chart already did.Fixed 2-arity, not a bound collection:
importedReadIdsis 1 or 2 by construction, and Room@Queryis static SQL that cannot express per-id priority through anINlist.Passing the same id twice is byte-identical to the old single-id read, so a single-WHOOP install is unchanged and needs no special case. That is the control test, and it is what makes this invisible to almost everyone.
The aggregate lives in
WhoopStore, whereswift-packages.ymlactually runs its tests — the placement that made #841's Swift half verifiable and its Kotlin half not.Verification
Checked against SQLite before commit, and the Kotlin
@Querywas extracted from source and executed to confirm it returns the same numbers as the Swift twin:Tests: four added in
WhoopStoreTests(overlap, precedence, per-id PPG anti-join, single-id control); five existing moved to the two-id signature with assertions untouched; both resolver tests (Swift + Kotlin) moved to the list API, plus two new Swift cases covering the behaviour the rename exists for — an imported row getting the full union on a multi-namespace install, and a detected row still resolving to one id on that same install.Notes
workoutHrBuckets,workoutZoneMinutesandworkoutHeartRateRecoverytake the row'ssource, defaulted so any caller without a row keeps today's behaviour.WorkoutRowcarries nodeviceId, so its resolver takes onlysourcewhere Kotlin's also takesrowDeviceId— pre-existing and documented in the original missing Effort Score and no HR+Calories for Workouts #510 note.hrSamplesUnion/hrBucketsUnionare untouched and still used by Today, the full-day chart and the auto-workout nudge, where the union is the correct read.