Skip to content

fix(workouts): one HR device-id rule for the chart, zones and Avg HR#857

Merged
ryanbr merged 5 commits into
mainfrom
fix/856-workout-hr-device-ids
Jul 27, 2026
Merged

fix(workouts): one HR device-id rule for the chart, zones and Avg HR#857
ryanbr merged 5 commits into
mainfrom
fix/856-workout-hr-device-ids

Conversation

@ryanbr

@ryanbr ryanbr commented Jul 27, 2026

Copy link
Copy Markdown
Owner

Closes #856.

Four surfaces on one card, no two agreeing

surface before wrong when
Swift Avg HR workoutHrDeviceId (single) under-reads imported after a strap re-add
Swift chart day union reads the wrong strap for a bout detected on a second WHOOP
Swift zones day union same
Swift HRR day union same
Kotlin chart single active id both failure modes
Kotlin zones single active id both
Kotlin HRR day union wrong strap for detected

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 mixes in a strap that never recorded it (#510)
manual / imported -> active ∪ canonical  no strap of its own; the worn strap may bank
                                         under either after a re-add (#814)

workoutHrDeviceIds returns that list on both platforms, and all four surfaces consume it:

surface Swift Kotlin
HR chart
zone minutes
Avg HR / Effort
heart-rate recovery

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 — inflating n, 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, not 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, and it is what makes this invisible to almost everyone.

The aggregate lives in WhoopStore, where swift-packages.yml actually 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 @Query 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             (precedence follows argument order)
PPG anti-join per id           -> n=15  max=130               (#841's anti-join still holds)
same id twice                  -> identical to the pre-change read

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, workoutZoneMinutes and workoutHeartRateRecovery take the row's source, defaulted so any caller without a row keeps today's behaviour.
  • Swift's WorkoutRow carries no deviceId, so its resolver takes only source where Kotlin's also takes rowDeviceId — pre-existing and documented in the original missing Effort Score and no HR+Calories for Workouts #510 note.
  • The day-level hrSamplesUnion / hrBucketsUnion are untouched and still used by Today, the full-day chart and the auto-workout nudge, where the union is the correct read.
  • The Kotlin half is not compiled or run by CI. Its SQL was executed directly and matches the Swift twin's output.

ryanbr added 2 commits July 26, 2026 19:37
…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.
@ryanbr

ryanbr commented Jul 27, 2026

Copy link
Copy Markdown
Owner Author

Re-reviewed this against what I actually promised on #856"the chart, the ZONE MINUTES and hrWindowStats all read that list" — and had only done two of the three.

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:

Swift zones    hrSamples(from:to:)   the day union  -> wrong strap for a detected bout
Kotlin zones   single active id      wrong for detected AND imported

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 — 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 any caller without a row keeps today's behaviour.

All three surfaces now go through workoutHrDeviceIds:

surface Swift Kotlin
HR chart
zone minutes
Avg HR / Effort

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.

ryanbr added 3 commits July 26, 2026 20:07
…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.
@ryanbr
ryanbr merged commit 7d2b0cd into main Jul 27, 2026
10 checks passed
@ryanbr
ryanbr deleted the fix/856-workout-hr-device-ids branch July 27, 2026 03:45
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.

[parity] Kotlin workoutHrBuckets reads one device id where Swift reads the #814 union

1 participant