Activity

1. ryanbr commented on Jul 27, 2026

   

   ryanbr

   on Jul 27, 2026

   OwnerAuthor

   More actions

   Worked the open question through. My original framing — "which of the three rules should the chart follow?" — was wrong. None of them is right on its own. The correct read is source-class-dependent, and workoutHrDeviceId already encodes that distinction; it just returns one id where two cases need a list.

   What each source class actually needs:

   source	correct read	why
   detected	its own recording id (single)	the bout was recorded on a specific strap. A union pulls in a different strap's HR for that window.
   manual / imported	active ∪ canonical (#814)	no strap of its own, so it reconciles against "the worn strap" — which may sit in either namespace after a re-add.

   Measured against that, all three current behaviours are wrong somewhere:

   • Swift avg (workoutHrDeviceId, single) — right for detected, under-reads imported after a strap re-add.
   • Swift chart (hrBuckets, union) — right for imported, and reads the wrong strap entirely for a bout detected on a second WHOOP: it unions active + canonical, neither of which is the recording strap. So on that screen the average describes strap B while the chart describes strap A.
   • Kotlin chart (single active id) — wrong for a second-strap detected bout and under-reads imported. Both failure modes at once.

   That last point makes this worse than I first reported: it is not only a Kotlin-vs-Swift divergence, there is a live self-inconsistency on iOS where the Avg HR and the HR curve on one card can be sourced from different straps.

   Suggested fix, one rule for both platforms and both surfaces:

   workoutHrDeviceIds(source, activeStrapId) -> [String]
       detected          -> [recordingId]            // strip "-noop"
       manual / imported -> importedReadIds          // active ∪ canonical
   

   Then the chart, the zone minutes and hrWindowStats all read that list. Single-WHOOP installs collapse to one id in every branch, so the common case is byte-identical and the change is invisible to almost everyone.

   Not proposing to do this alongside anything else — it touches the Avg HR path #841 just changed, so it wants its own PR and its own before/after on a multi-strap fixture.
2. ryanbr commented on Jul 27, 2026

   

   ryanbr

   on Jul 27, 2026

   OwnerAuthor

   More actions

   #856 — sketch of the fix

   One rule, four surfaces, both platforms. Single-WHOOP installs collapse to one id in every
   branch, so the common case is byte-identical.

   1. The resolver returns a list

   Swift (Repository.swift) — workoutHrDeviceId → workoutHrDeviceIds:

   -    nonisolated static func workoutHrDeviceId(source: String, activeStrapId: String) -> String {
   -        guard WorkoutSource.classify(source) == .detected else { return activeStrapId }
   -        return source.hasSuffix("-noop") ? String(source.dropLast(5)) : source
   -    }
   +    /// The device ids whose HR backs a workout's chart, zones and Avg HR — in read precedence.
   +    ///
   +    /// DETECTED rows return exactly ONE id: the strap that recorded the bout. Unioning here would
   +    /// mix a different strap's HR into a session it never recorded (#510).
   +    ///
   +    /// MANUAL / IMPORTED rows carry no strap of their own, so they reconcile against "the worn
   +    /// strap" — which after a re-add may bank under the canonical id rather than the active one.
   +    /// Those get the #814 read-spine union, active first so it wins per timestamp.
   +    ///
   +    /// Single-WHOOP install ⇒ one id in both branches ⇒ byte-identical to the old single-id read.
   +    nonisolated static func workoutHrDeviceIds(source: String, activeStrapId: String,
   +                                               importedIds: [String]) -> [String] {
   +        guard WorkoutSource.classify(source) == .detected else { return importedIds }
   +        return [source.hasSuffix("-noop") ? String(source.dropLast(5)) : source]
   +    }

   Kotlin (WhoopRepository.kt) — same shape, keeping the extra rowDeviceId the Kotlin twin
   already threads:

   -        fun workoutHrDeviceId(source: String, rowDeviceId: String, activeStrapId: String): String =
   -            if (isStrapNativeWorkout(source)) rowDeviceId.removeSuffix("-noop") else activeStrapId
   +        fun workoutHrDeviceIds(source: String, rowDeviceId: String, activeStrapId: String): List<String> =
   +            if (isStrapNativeWorkout(source)) listOf(rowDeviceId.removeSuffix("-noop"))
   +            else importedSourceIdsFor(activeStrapId)

   2. The chart reads it

   Kotlin (AppViewModel.workoutHrBuckets) — currently the single active id, which is wrong for a
   second-strap bout and under-reads imported:

   -        return runCatching { repository.hrBuckets(deviceId, from, to, bucket) }.getOrDefault(emptyList())
   +        val ids = WhoopRepository.workoutHrDeviceIds(source, rowDeviceId, deviceId)
   +        return runCatching { repository.hrBucketsFor(ids, from, to, bucket) }.getOrDefault(emptyList())

   hrBucketsFor(ids, …) is mergeHrBucketsByStart(ids.map { dao.hrBuckets(it, …) }) — the existing
   dedup helper, just taking an explicit list instead of importedSourceIds(active).

   Swift (Repository.workoutHrBuckets) — stop delegating to the day-level union, which reads
   active ∪ canonical even for a bout recorded on a second strap:

   -        return await hrBuckets(from: from, to: to, bucketSeconds: bucket)
   +        return await hrBuckets(deviceIds: Self.workoutHrDeviceIds(source: source,
   +                                                                  activeStrapId: deviceId,
   +                                                                  importedIds: importedReadIds),
   +                               from: from, to: to, bucketSeconds: bucket)

   This needs workoutHrBuckets to take the row's source, which it currently doesn't — the call site
   has it.

   3. The Avg HR aggregate — the awkward part

   hrWindowStats is a single SQL aggregate. Feeding it N ids is not just an IN clause: if the
   same second exists under two ids, COUNT/AVG double-count it, where the chart's merge keeps one
   row per timestamp with the active id winning. So the aggregate has to dedup the same way:

   SELECT COUNT(*) AS n, AVG(bpm) AS avg, MAX(bpm) AS max FROM (
       SELECT ts, bpm FROM (
           SELECT ts, bpm, 0 AS pri FROM hrSample     WHERE deviceId = :primary   AND ts BETWEEN …
           UNION ALL
           SELECT ts, bpm, 1 AS pri FROM hrSample     WHERE deviceId = :secondary AND ts BETWEEN …
           -- …plus the ppgHrSample legs from #841, same anti-join
       ) GROUP BY ts HAVING pri = MIN(pri)
   )

   That is doable but it is a real query, and it has to stay byte-identical to whatever Swift does.
   This is the piece that makes #856 its own PR rather than a rename.

   A cheaper first cut, if you want the chart fixed without touching the metric path: land steps 1–2
   only, leave hrWindowStats on ids.first(), and note that imported rows still under-read after a
   re-add. That closes the self-inconsistency (chart and average would at least describe the same
   strap) without a new aggregate.

   4. Test shape

   The whole bug is invisible on a single-WHOOP install, so the fixture has to be multi-strap:

   • a detected bout under whoop-B-noop while the active strap is whoop-A — chart and average
     must both read whoop-B, and must not include whoop-A or canonical rows in the window
   • an imported row while history banks under canonical after a re-add — both must read the union
   • a single-strap control asserting byte-identical output to today, on both platforms

   That last one is the important one: it is what proves this is invisible to almost every user.

   ---

   The dedup concern above is real, not theoretical. Checked it against SQLite with the same second banked under two ids:

   naive IN('A','B')   n=20  avg=150.0   <- double-counts the 5 shared seconds
   dedup, A wins       n=15  avg=133.3
   expected            n=15  avg=133.3
   

   So a naive IN clause on hrWindowStats would inflate the sample count and skew the average on exactly the multi-namespace install this fix targets — and it would do so silently, since both numbers stay plausible. That is why step 3 needs the GROUP BY ts HAVING pri = MIN(pri) form rather than the obvious one.
3. ryanbr commented on Jul 27, 2026

   

   ryanbr

   on Jul 27, 2026

   OwnerAuthor

   More actions

   Correcting my own "cheaper first cut" suggestion — it does not do what I claimed.

   option	source	chart	avg	agree?
   today	detected	union(A,C)	B	NO
   today	imported	union(A,C)	A	NO
   cheap cut	detected	[B]	B	yes
   cheap cut	imported	union(A,C)	A	NO
   both .first()	detected	[B]	B	yes
   both .first()	imported	[A]	A	yes
   full fix	detected	[B]	B	yes
   full fix	imported	union(A,C)	union(A,C)	yes

   The cheap cut fixes the severe case (a detected bout reading a strap that never recorded it) but leaves imported rows with the chart and average still describing different data. So it does not close the self-inconsistency, only half of it.

   both .first() is consistent but regresses the chart — imported loses the canonical half it reads today. Not acceptable.

   So the only coherent option is the full fix, and the dedup aggregate is not optional.

   Recommended shape, mirroring how #841 was done this morning:

   1. Put the deduping window aggregate in WhoopStore, not Repository. swift-packages.yml runs Packages/** tests, so the query gets real CI coverage — whereas anything in Strand/ or android/ is compiled by nothing right now. That is exactly what made fix(workouts): count PPG-derived HR in the workout window stats #841's Swift half verifiable and its Kotlin half not.
   2. workoutHrDeviceIds on both platforms, returning a list.
   3. Chart, zone minutes and Avg HR all read that list.
   4. Multi-strap fixtures, plus a single-strap control asserting byte-identical output — that control is what proves this is invisible to almost every user.

   On priority, though: I would not do this next. It only bites multi-strap installs, it touches the aggregate #841 changed hours ago, and six merged changes are still sitting uncompiled because no staging build has been cut today. Getting those compiled — and giving @bartmuskala something to test for #835 / #836 — is worth more than a fix for a configuration most users do not have.

   Happy to implement whenever you want it; just flagging that it is not the highest-value thing on the board.
4. ryanbr commented on Jul 27, 2026

   

   ryanbr

   on Jul 27, 2026

   OwnerAuthor

   More actions

   #856 — the minimal coherent PR

   Four hunks. No schema, no migration, no new strings.

   The design turns on one observation: importedReadIds is at most two ids by construction
   (deviceId == canonical ? [deviceId] : [deviceId, canonical]). So this needs no dynamic IN
   list — which matters, because Room @Query is static SQL and cannot express per-id priority
   through a bound collection. Fixed 2-arity, secondary defaulting to primary.

   1. The resolver returns a list

   Swift (Repository.swift):

   -    nonisolated static func workoutHrDeviceId(source: String, activeStrapId: String) -> String {
   -        guard WorkoutSource.classify(source) == .detected else { return activeStrapId }
   -        return source.hasSuffix("-noop") ? String(source.dropLast(5)) : source
   -    }
   +    /// The device ids whose HR backs a workout's chart, zones and Avg HR, in read precedence.
   +    ///
   +    /// DETECTED rows return exactly ONE id — the strap that recorded the bout. Unioning here would
   +    /// mix in HR from a strap that never recorded the session (#510).
   +    ///
   +    /// MANUAL / IMPORTED rows carry no strap of their own, so they reconcile against "the worn
   +    /// strap", which after a re-add may bank under the canonical id. Those get the #814 union,
   +    /// active first so it wins per timestamp.
   +    ///
   +    /// At most two ids either way. Single-WHOOP install collapses to one in both branches.
   +    nonisolated static func workoutHrDeviceIds(source: String, activeStrapId: String,
   +                                               importedIds: [String]) -> [String] {
   +        guard WorkoutSource.classify(source) == .detected else { return importedIds }
   +        return [source.hasSuffix("-noop") ? String(source.dropLast(5)) : source]
   +    }

   Kotlin (WhoopRepository.kt), same shape:

   -        fun workoutHrDeviceId(source: String, rowDeviceId: String, activeStrapId: String): String =
   -            if (isStrapNativeWorkout(source)) rowDeviceId.removeSuffix("-noop") else activeStrapId
   +        fun workoutHrDeviceIds(source: String, rowDeviceId: String, activeStrapId: String): List<String> =
   +            if (isStrapNativeWorkout(source)) listOf(rowDeviceId.removeSuffix("-noop"))
   +            else importedSourceIdsFor(activeStrapId)

   2. The window aggregate takes two ids

   Goes in WhoopStore (Swift), not Repository — swift-packages.yml runs Packages/**
   tests, so this query gets real CI coverage. Anything under Strand/ or android/ is compiled
   by nothing today.

   -    public func hrWindowStats(deviceId: String, from: Int, to: Int) async throws -> HRWindowStats {
   +    /// Aggregate HR over a window across up to TWO device ids, `primary` winning per second.
   +    ///
   +    /// A naive `deviceId IN (…)` is wrong here: a second banked under both ids after a strap
   +    /// re-add would be counted twice, inflating `n` and skewing `avg` — silently, since both
   +    /// numbers stay plausible. `GROUP BY ts` with `MIN(pri)` keeps one row per second and picks
   +    /// the primary's, matching the dedup `hrBuckets` already does for the chart.
   +    ///
   +    /// Passing the same id for both is byte-identical to the old single-id read, so a
   +    /// single-WHOOP install needs no special case.
   +    public func hrWindowStats(primaryId: String, secondaryId: String,
   +                              from: Int, to: Int) async throws -> HRWindowStats {
            try syncRead { db in
                guard let row = try Row.fetchOne(db, sql: """
                    SELECT COUNT(*) AS n, AVG(bpm) AS avg, MAX(bpm) AS max FROM (
   -                    SELECT bpm FROM hrSample
   -                    WHERE deviceId = ? AND ts >= ? AND ts <= ?
   -                    UNION ALL
   -                    SELECT CAST(ROUND(p.bpm) AS INTEGER) AS bpm FROM ppgHrSample p
   -                    WHERE p.deviceId = ? AND p.ts >= ? AND p.ts <= ?
   -                      AND NOT EXISTS (
   -                        SELECT 1 FROM hrSample h
   -                        WHERE h.deviceId = p.deviceId AND h.ts = p.ts)
   +                    SELECT ts, MIN(pri), bpm FROM (
   +                        SELECT ts, bpm, 0 AS pri FROM hrSample
   +                        WHERE deviceId = ? AND ts >= ? AND ts <= ?
   +                        UNION ALL
   +                        SELECT p.ts, CAST(ROUND(p.bpm) AS INTEGER), 0 FROM ppgHrSample p
   +                        WHERE p.deviceId = ? AND p.ts >= ? AND p.ts <= ?
   +                          AND NOT EXISTS (SELECT 1 FROM hrSample h
   +                                          WHERE h.deviceId = p.deviceId AND h.ts = p.ts)
   +                        UNION ALL
   +                        SELECT ts, bpm, 1 FROM hrSample
   +                        WHERE deviceId = ? AND ts >= ? AND ts <= ?
   +                        UNION ALL
   +                        SELECT p.ts, CAST(ROUND(p.bpm) AS INTEGER), 1 FROM ppgHrSample p
   +                        WHERE p.deviceId = ? AND p.ts >= ? AND p.ts <= ?
   +                          AND NOT EXISTS (SELECT 1 FROM hrSample h
   +                                          WHERE h.deviceId = p.deviceId AND h.ts = p.ts)
   +                    ) GROUP BY ts
                    )

   MIN(pri) with a bare bpm is SQLite's documented bare-column rule: with a single min/max
   aggregate, the bare columns come from the matching row. Both Room and GRDB are SQLite, so the
   Kotlin @Query twin is the same text.

   3. Both surfaces read the list

   -        let hrDeviceId = Self.workoutHrDeviceId(source: rows[idx].source, activeStrapId: deviceId)
   +        let ids = Self.workoutHrDeviceIds(source: rows[idx].source, activeStrapId: deviceId,
   +                                          importedIds: importedReadIds)
            group.addTask { [ids] in
   -            guard let stats = try? await store.hrWindowStats(deviceId: hrDeviceId, from: startTs, to: endTs),
   +            guard let stats = try? await store.hrWindowStats(primaryId: ids[0],
   +                                                             secondaryId: ids.count > 1 ? ids[1] : ids[0],
   +                                                             from: startTs, to: endTs),

   and the chart, Kotlin side:

   -        return runCatching { repository.hrBuckets(deviceId, from, to, bucket) }.getOrDefault(emptyList())
   +        val ids = WhoopRepository.workoutHrDeviceIds(source, rowDeviceId, deviceId)
   +        return runCatching { repository.hrBucketsFor(ids, from, to, bucket) }.getOrDefault(emptyList())

   hrBucketsFor(ids, …) is mergeHrBucketsByStart(ids.map { dao.hrBuckets(it, …) }) — the dedup
   helper that already exists, taking an explicit list instead of importedSourceIds(active).

   Swift's workoutHrBuckets needs the row's source threaded in; the call site has it.

   4. Tests

   In WhoopStoreTests, so CI runs them:

   • two ids, overlapping seconds — n counts each second once, primary's value wins
   • same id twice — byte-identical to the pre-change single-id read (the control that proves
     this is invisible to single-WHOOP users)
   • detected bout on a second strap — reads only the recording strap; the active strap's rows
     in the same window do not appear
   • PPG legs still anti-joined per id — a measured second is never doubled by its own estimate

   Verified against SQLite before writing this:

   A(ts 0..9) + B(ts 5..19), A wins ties : n=20  avg=150.0  max=200
   expected                              : n=20  avg=150.0  max=200   correct
   naive IN('A','B')                     : n=25             <- double-counts ts 5..9
   A passed twice                        : n=10  avg=100.0  max=100
   today's single-id read                : n=10  avg=100.0  max=100   byte-identical
   

   Worth noting one of these bit me while writing it: my first fixture had the primary id's PPG
   leg covering the whole window, so the secondary contributed nothing and n matched the
   single-id case for the wrong reason. The fixture above is built so the second id genuinely
   adds seconds (ts 10..19) and overlaps on others (ts 5..9), which is what actually
   exercises the dedup.

   Scope note

   The Kotlin @Query twin is text-identical but nothing compiles or runs it, same as every Android
   change today. The Swift half carries the real verification, exactly as it did for #841.
5. ryanbr

   added 2 commits that reference this issue on Jul 27, 2026

   fix(workouts): zone minutes too — the third surface, which I had missed

   ...

   399c458

   fix(workouts): HRR too — the fourth surface on the same card

   ...

   43046d0
6. ryanbr

   closed this as completedin #857on Jul 27, 2026
7. ryanbr commented on Jul 27, 2026

   

   ryanbr

   on Jul 27, 2026

   OwnerAuthor

   More actions

   Merged as 7d2b0cdc. Closed automatically by the PR — recording what is and is not settled.

   Resolved: all four workout HR surfaces on both platforms now go through one workoutHrDeviceIds rule — chart, zone minutes, Avg HR / Effort, and heart-rate recovery. The issue as filed only named the chart; the other three turned up while checking whether the fix was complete.

   Not field-verified. The bug only manifests on a multi-strap install — a bout detected on a second WHOOP, or history banked under canonical after a re-add. Nobody here has that configuration. What exists is four WhoopStore tests exercising the two-id aggregate against real SQLite in CI, the Kotlin @Query extracted from source and executed to confirm it returns the same numbers, and a single-strap control proving the common case is byte-identical.

   That is good coverage for synthetic fixtures, and it is not the same as someone with two straps confirming their workout card agrees with itself. Not holding the issue open for it — the fix is not in doubt — but if a multi-strap report ever lands, this is the change to look at first.

   The Kotlin half is compiled by nothing, as with every Android change while android.yml is disabled. Its SQL was executed directly and matches the Swift twin.