diff --git a/Packages/WhoopStore/Sources/WhoopStore/Reads.swift b/Packages/WhoopStore/Sources/WhoopStore/Reads.swift index 4d56e18105..6625c4b8d9 100644 --- a/Packages/WhoopStore/Sources/WhoopStore/Reads.swift +++ b/Packages/WhoopStore/Sources/WhoopStore/Reads.swift @@ -91,20 +91,41 @@ extension WhoopStore { /// /// Same anti-join as [hrSamples]: a measured second is never double-counted by its PPG estimate. /// `avg`/`max` are nil when `n == 0`. Kotlin twin: `WhoopDao.hrWindowStats`. - public func hrWindowStats(deviceId: String, from: Int, to: Int) async throws -> HRWindowStats { + /// + /// #856: aggregates across up to TWO device ids, `primaryId` 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`, and both numbers would stay plausible so nothing + /// would look wrong. `GROUP BY ts` with `MIN(pri)` keeps one row per second and takes the primary's, + /// matching the dedup `hrBuckets` already does for the chart; SQLite's bare-column rule makes `bpm` + /// come from the row that supplied the `MIN`. + /// + /// Passing the same id for both is byte-identical to the old single-id read, so a single-WHOOP + /// install needs no special case and every existing number is unchanged. + 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 ) - """, arguments: [deviceId, from, to, deviceId, from, to]) + """, arguments: [primaryId, from, to, primaryId, from, to, + secondaryId, from, to, secondaryId, from, to]) else { return HRWindowStats(n: 0, avg: nil, max: nil) } return HRWindowStats(n: row["n"], avg: row["avg"], max: row["max"]) } diff --git a/Packages/WhoopStore/Tests/WhoopStoreTests/ReadTests.swift b/Packages/WhoopStore/Tests/WhoopStoreTests/ReadTests.swift index 1710dd981d..cc7f4f783b 100644 --- a/Packages/WhoopStore/Tests/WhoopStoreTests/ReadTests.swift +++ b/Packages/WhoopStore/Tests/WhoopStoreTests/ReadTests.swift @@ -88,7 +88,7 @@ final class ReadTests: XCTestCase { for t in 15..<415 { ppg.append(PpgHrSample(ts: t, bpm: 140, conf: 0.8)) } _ = try await store.insert(Streams(hr: hr, ppgHr: ppg), deviceId: "dev1") - let stats = try await store.hrWindowStats(deviceId: "dev1", from: 0, to: 1_000) + let stats = try await store.hrWindowStats(primaryId: "dev1", secondaryId: "dev1", from: 0, to: 1_000) XCTAssertEqual(stats.n, 415, "measured ∪ PPG, with the 5 overlapping seconds counted once") XCTAssertEqual(stats.max, 140) @@ -107,7 +107,7 @@ final class ReadTests: XCTestCase { Streams(hr: (0..<10).map { HRSample(ts: $0, bpm: 100) }, ppgHr: (0..<10).map { PpgHrSample(ts: $0, bpm: 180, conf: 0.9) }), deviceId: "dev1") - let stats = try await store.hrWindowStats(deviceId: "dev1", from: 0, to: 1_000) + let stats = try await store.hrWindowStats(primaryId: "dev1", secondaryId: "dev1", from: 0, to: 1_000) XCTAssertEqual(stats.n, 10, "every second is measured; no PPG row may be added") XCTAssertEqual(try XCTUnwrap(stats.avg), 100, accuracy: 1e-9) XCTAssertEqual(stats.max, 100, "the PPG 180s must not become the peak") @@ -123,7 +123,7 @@ final class ReadTests: XCTestCase { let hr = (0.. [HRSample] { + guard let store = await ensureStore(), !deviceIds.isEmpty else { return [] } + guard deviceIds.count > 1 else { + return (try? await store.hrSamples(deviceId: deviceIds[0], from: from, to: to, + limit: limit)) ?? [] + } + var byTs: [Int: HRSample] = [:] + for id in deviceIds { + for s in (try? await store.hrSamples(deviceId: id, from: from, to: to, + limit: limit)) ?? [] where byTs[s.ts] == nil { + byTs[s.ts] = s + } + } + return byTs.values.sorted { $0.ts < $1.ts } + } + func hrSamples(from: Int, to: Int, limit: Int = 8000) async -> [HRSample] { guard let store = await ensureStore() else { return [] } // UNION the active strap + canonical so the HR trend renders whether the landed day's raw sits under @@ -939,6 +957,24 @@ final class Repository: ObservableObject { /// Downsampled HR (mean bpm per `bucketSeconds`) for the strap, for a Today/24h trend chart. /// Aggregated in SQL so a full day never loads the raw ~1 Hz rows. + /// #856: the same union, over an EXPLICIT id list. Lets a workout read its own recording strap + /// instead of the day-level active ∪ canonical. Order is precedence: earlier ids win per bucket. + func hrBuckets(deviceIds: [String], from: Int, to: Int, bucketSeconds: Int = 300) async -> [HRBucket] { + guard let store = await ensureStore(), !deviceIds.isEmpty else { return [] } + guard deviceIds.count > 1 else { + return (try? await store.hrBuckets(deviceId: deviceIds[0], from: from, to: to, + bucketSeconds: bucketSeconds)) ?? [] + } + var byStart: [Int: HRBucket] = [:] + for id in deviceIds { + for b in (try? await store.hrBuckets(deviceId: id, from: from, to: to, + bucketSeconds: bucketSeconds)) ?? [] where byStart[b.ts] == nil { + byStart[b.ts] = b + } + } + return byStart.values.sorted { $0.ts < $1.ts } + } + func hrBuckets(from: Int, to: Int, bucketSeconds: Int = 300) async -> [HRBucket] { guard let store = await ensureStore() else { return [] } // UNION the active strap + canonical for the trend chart. Per bucket-start the active strap wins; a @@ -2223,16 +2259,19 @@ final class Repository: ObservableObject { // row's `source` IS its computed strap id ("-noop"), so a bout auto-detected on a 2nd // WHOOP reads "" instead of the active strap's empty window. Resolved on the main actor; // only the resulting Sendable String crosses into the task. - let hrDeviceId = Self.workoutHrDeviceId(source: rows[idx].source, activeStrapId: deviceId) - group.addTask { [hrDeviceId] in + let hrIds = Self.workoutHrDeviceIds(source: rows[idx].source, activeStrapId: deviceId, + importedIds: importedReadIds) + group.addTask { [hrIds] in // #836: aggregate in SQLite over the WHOLE window instead of reducing up to 8000 // materialised rows. The old read capped at 8000, so a workout longer than ~2h13m // at 1 Hz reported the mean of its FIRST 8000 samples as the session average — wrong // on its own terms, and divergent from Kotlin, which aggregates the lot. This also // makes the common path (no strain fill) read no rows at all, which is strictly // better for the #833 freeze this function exists to avoid. - guard let stats = try? await store.hrWindowStats(deviceId: hrDeviceId, - from: startTs, to: endTs), + guard let stats = try? await store.hrWindowStats( + primaryId: hrIds[0], + secondaryId: hrIds.count > 1 ? hrIds[1] : hrIds[0], + from: startTs, to: endTs), stats.n >= minSamples, let mean = stats.avg, let peak = stats.max else { return nil } let avg = Int(mean.rounded()) @@ -2243,7 +2282,7 @@ final class Repository: ObservableObject { // cap stays here: it bounds the strain window, not the average. (Kotlin does the same.) let strain: Double? if wantStrain, let p = strainProfile { - let samples = (try? await store.hrSamples(deviceId: hrDeviceId, + let samples = (try? await store.hrSamples(deviceId: hrIds[0], from: startTs, to: endTs, limit: 8000)) ?? [] strain = StrainScorer.strain(samples, maxHR: p.hrMax, sex: p.sex) @@ -2278,7 +2317,7 @@ final class Repository: ObservableObject { } } - /// #510 (Kotlin twin: `WhoopRepository.workoutHrDeviceId`). The device id whose `hrSample` rows back a + /// #510 (Kotlin twin: `WhoopRepository.workoutHrDeviceIds`). The device ids whose `hrSample` rows back a /// workout's Avg HR / Effort reconcile. A DETECTED row's own `source` IS its computed strap id /// ("-noop"), so strip the suffix to read HR under the raw "" — a bout auto-detected on a /// SECOND WHOOP no longer reads the active strap's empty window (which blanked its reconcile). MANUAL and @@ -2287,9 +2326,21 @@ final class Repository: ObservableObject { /// byte-identical. (The Kotlin twin also keys MANUAL rows on their stored deviceId; the Swift read-model /// `WorkoutRow` carries no deviceId, so a manual session created on a NON-active strap reconciles here /// against the active strap instead — a minor, display-only divergence, never persisted.) - 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 + /// #856: the 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, which is what #510 fixed. + /// + /// 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. + /// + /// At most two ids either way (`importedReadIds` is 1 or 2 by construction), and a single-WHOOP + /// install collapses to one in both branches — so every existing number is unchanged. + 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] } // MARK: - Workout editing (manual add/edit · relabel · dismiss · delete) @@ -2568,11 +2619,19 @@ final class Repository: ObservableObject { /// Downsampled HR over a workout window for the detail HR-curve. A short session wants a finer /// bucket than the Today 24h chart (300 s would flatten a 30-min run to ~6 points), so the bucket /// scales with duration: ~120 buckets across the window, floored at 15 s and capped at 300 s. - func workoutHrBuckets(from: Int, to: Int) async -> [HRBucket] { + /// #856: reads the ids `workoutHrDeviceIds` resolves for this row, not the day-level union. A bout + /// detected on a SECOND WHOOP is charted from the strap that recorded it; previously this delegated + /// to `hrBuckets(from:to:)`, which unions active + canonical — neither of which is that strap — so + /// the curve and the Avg HR on the same card could describe different straps entirely. + /// `source` defaults to "" (⇒ the imported/manual branch, the union) so callers without a row keep + /// today's behaviour. + func workoutHrBuckets(from: Int, to: Int, source: String = "") async -> [HRBucket] { guard to > from else { return [] } let span = to - from let bucket = max(15, min(300, span / 120)) - return await hrBuckets(from: from, to: to, bucketSeconds: bucket) + let ids = Self.workoutHrDeviceIds(source: source, activeStrapId: deviceId, + importedIds: importedReadIds) + return await hrBuckets(deviceIds: ids, from: from, to: to, bucketSeconds: bucket) } /// Raw HR samples binned into per-zone MINUTES for a workout window, using the age-derived @@ -2581,9 +2640,15 @@ final class Repository: ObservableObject { /// `zonesJSON` still gets a real time-in-zone split. Returns nil when the window carries no HR (so /// the view shows nothing rather than five empty bars). `age <= 0` falls back to a 30 y default , /// the zones are approximate either way and clearly labelled as such in the UI. - func workoutZoneMinutes(from: Int, to: Int, age: Int) async -> [Double]? { + /// #856: bins the same rows the chart plots and the Avg HR aggregates. Previously this read the + /// day-level union, so a bout detected on a second WHOOP had its zones computed from a strap that + /// never recorded it. `source` defaults to "" (⇒ the imported branch, the union) so a caller + /// without a row keeps today's behaviour. + func workoutZoneMinutes(from: Int, to: Int, age: Int, source: String = "") async -> [Double]? { guard to > from else { return nil } - let samples = await hrSamples(from: from, to: to) + let ids = Self.workoutHrDeviceIds(source: source, activeStrapId: deviceId, + importedIds: importedReadIds) + let samples = await hrSamples(deviceIds: ids, from: from, to: to) guard !samples.isEmpty else { return nil } let zoneSet = HRZones.zones(age: age > 0 ? Double(age) : 30) let tiz = HRZones.timeInZone(samples, zoneSet: zoneSet) @@ -2595,11 +2660,17 @@ final class Repository: ObservableObject { /// post-workout minutes. This is a narrow read (at most ~10 minutes), not a whole-workout scan, and the /// pure engine owns every eligibility/coverage guard. Missing post-workout HR therefore returns nil /// instead of fabricating a recovery value. Kotlin twin: `AppViewModel.workoutHeartRateRecovery`. - func workoutHeartRateRecovery(from: Int, to: Int, maxHR: Double) async -> HeartRateRecovery.Result? { + /// #856: reads the ids this row resolves to, like the chart, zones and Avg HR. The recovery window + /// extends PAST the bout, but the strap that recorded it is still the one on the wrist a few + /// minutes later — so a detected bout reads its own strap here too rather than the day union. + func workoutHeartRateRecovery(from: Int, to: Int, maxHR: Double, + source: String = "") async -> HeartRateRecovery.Result? { guard to > from, maxHR > 0 else { return nil } let readFrom = max(from, to - HeartRateRecovery.eligibilityLookbackSeconds) let readTo = to + 5 * 60 + HeartRateRecovery.measurementToleranceSeconds - let samples = await hrSamples(from: readFrom, to: readTo, limit: 2_000) + let ids = Self.workoutHrDeviceIds(source: source, activeStrapId: deviceId, + importedIds: importedReadIds) + let samples = await hrSamples(deviceIds: ids, from: readFrom, to: readTo, limit: 2_000) return HeartRateRecovery.calculate(samples: samples, workoutStart: from, workoutEnd: to, maxHR: maxHR) } diff --git a/Strand/Screens/WorkoutDetailView.swift b/Strand/Screens/WorkoutDetailView.swift index ba46818fac..18ddfdb4f6 100644 --- a/Strand/Screens/WorkoutDetailView.swift +++ b/Strand/Screens/WorkoutDetailView.swift @@ -107,7 +107,7 @@ struct WorkoutDetailView: View { // HR curve over the exact session window — a finer bucket than the 24h chart so a short run // still reads as a curve, not a handful of points. - let buckets = await repo.workoutHrBuckets(from: row.startTs, to: row.endTs) + let buckets = await repo.workoutHrBuckets(from: row.startTs, to: row.endTs, source: row.source) let points = buckets.map { TrendPoint(date: Date(timeIntervalSince1970: TimeInterval($0.ts)), value: $0.bpm) } // Zones: prefer the imported per-workout percentages (a WHOOP-computed split), and only fall @@ -123,11 +123,12 @@ struct WorkoutDetailView: View { } } if minutes == nil { - minutes = await repo.workoutZoneMinutes(from: row.startTs, to: row.endTs, age: profile.age) + minutes = await repo.workoutZoneMinutes(from: row.startTs, to: row.endTs, age: profile.age, + source: row.source) } let hrr = await repo.workoutHeartRateRecovery( - from: row.startTs, to: row.endTs, maxHR: Double(profile.hrMax)) + from: row.startTs, to: row.endTs, maxHR: Double(profile.hrMax), source: row.source) // Steps for an on-foot session (#398), computed at display time over the exact window so it // "fills in after sync": prefer the strap's own counter (MG/5.0) once it has offloaded the window, diff --git a/Strand/Screens/WorkoutsView.swift b/Strand/Screens/WorkoutsView.swift index 9b5a96239c..6fcddb0176 100644 --- a/Strand/Screens/WorkoutsView.swift +++ b/Strand/Screens/WorkoutsView.swift @@ -304,7 +304,8 @@ struct WorkoutsView: View { for row in recoveryTrendRows { if Task.isCancelled { return } if let result = await repo.workoutHeartRateRecovery( - from: row.startTs, to: row.endTs, maxHR: Double(model.profile.hrMax)) { + from: row.startTs, to: row.endTs, maxHR: Double(model.profile.hrMax), + source: row.source) { built.append(WorkoutRecoveryTrendPoint(startTs: row.startTs, result: result)) } } diff --git a/StrandTests/WorkoutHrDeviceKeyTests.swift b/StrandTests/WorkoutHrDeviceKeyTests.swift index f34083595d..4b492485fb 100644 --- a/StrandTests/WorkoutHrDeviceKeyTests.swift +++ b/StrandTests/WorkoutHrDeviceKeyTests.swift @@ -3,7 +3,7 @@ import XCTest /// #510: `reconcileWorkoutHrWithTrace` used to read every workout's HR window under the Repository's single /// active `deviceId`, so a bout auto-detected on a SECOND WHOOP (its `hrSample` rows banked under that strap) -/// read the active strap's empty window and its Avg HR / Effort went un-reconciled. `Repository.workoutHrDeviceId` +/// read the active strap's empty window and its Avg HR / Effort went un-reconciled. `Repository.workoutHrDeviceIds` /// now resolves the read key per row from the workout's own `source`. Kotlin twin: `WorkoutHrDeviceKeyTest`. /// /// NB the Swift read-model `WorkoutRow` carries no `deviceId`, so only DETECTED rows (whose `source` IS their @@ -15,24 +15,28 @@ final class WorkoutHrDeviceKeyTests: XCTestCase { /// The active strap being something else must NOT redirect the read to an empty window. func testDetectedReadsItsOwnBaseStrap_notActive() { XCTAssertEqual( - Repository.workoutHrDeviceId(source: "whoop-aabbcc-noop", activeStrapId: "my-whoop"), - "whoop-aabbcc") + Repository.workoutHrDeviceIds(source: "whoop-aabbcc-noop", activeStrapId: "my-whoop", + importedIds: ["my-whoop"]), + ["whoop-aabbcc"], + "#856: exactly ONE id — unioning would mix in a strap that never recorded this bout") } /// Single-WHOOP: the canonical detected id strips to the active id, so the read is byte-identical to the /// old `self.deviceId` behaviour. func testDetectedCanonicalIsByteIdenticalToActive() { XCTAssertEqual( - Repository.workoutHrDeviceId(source: "my-whoop-noop", activeStrapId: "my-whoop"), - "my-whoop") + Repository.workoutHrDeviceIds(source: "my-whoop-noop", activeStrapId: "my-whoop", + importedIds: ["my-whoop"]), + ["my-whoop"]) } /// Manual rows carry no strap id in `source`, so they reconcile against the active strap (the documented /// display-only divergence from the Kotlin twin, which keys manual rows on their stored deviceId). func testManualReadsActiveStrap() { XCTAssertEqual( - Repository.workoutHrDeviceId(source: "manual", activeStrapId: "whoop-aabbcc"), - "whoop-aabbcc") + Repository.workoutHrDeviceIds(source: "manual", activeStrapId: "whoop-aabbcc", + importedIds: ["whoop-aabbcc"]), + ["whoop-aabbcc"]) } /// Imported rows (Apple / activity file / lifting / WHOOP CSV) reconcile against the active strap — the @@ -41,9 +45,29 @@ final class WorkoutHrDeviceKeyTests: XCTestCase { func testImportedReadsActiveStrap() { for src in ["apple-health", "apple_health", "activity-file", "lifting", "my-whoop", "Health Connect"] { XCTAssertEqual( - Repository.workoutHrDeviceId(source: src, activeStrapId: "whoop-aabbcc"), - "whoop-aabbcc", + Repository.workoutHrDeviceIds(source: src, activeStrapId: "whoop-aabbcc", + importedIds: ["whoop-aabbcc"]), + ["whoop-aabbcc"], "imported source \(src) should reconcile against the active strap") } } + + /// #856: an imported row on a MULTI-namespace install gets the whole #814 union, not just the + /// active strap — it has no strap of its own, and after a re-add the worn strap's history may sit + /// under the canonical id. This is the case the single-id resolver silently under-read. + func testImportedGetsTheFullUnionWhenTheyDiffer() { + XCTAssertEqual( + Repository.workoutHrDeviceIds(source: "apple-health", activeStrapId: "whoop-aabbcc", + importedIds: ["whoop-aabbcc", "my-whoop"]), + ["whoop-aabbcc", "my-whoop"]) + } + + /// …while a DETECTED row on that same install still resolves to one id. The union must not leak + /// into the branch where it would be wrong. + func testDetectedIgnoresTheUnionEvenWhenOneIsAvailable() { + XCTAssertEqual( + Repository.workoutHrDeviceIds(source: "whoop-aabbcc-noop", activeStrapId: "my-whoop", + importedIds: ["my-whoop", "whoop-zzz"]), + ["whoop-aabbcc"]) + } } diff --git a/android/app/src/main/java/com/noop/data/WhoopDao.kt b/android/app/src/main/java/com/noop/data/WhoopDao.kt index baf756955a..8c8d9bb08c 100644 --- a/android/app/src/main/java/com/noop/data/WhoopDao.kt +++ b/android/app/src/main/java/com/noop/data/WhoopDao.kt @@ -299,16 +299,30 @@ interface WhoopDao : DeviceRegistryDao { * it: its twin reduces `store.hrSamples`, the coalescing read. Same anti-join as [hrSamples], so a * measured second is never double-counted by its PPG estimate. */ @Query( + // #856: up to TWO ids, :primaryId winning per second. A naive `deviceId IN (…)` would count a + // second banked under BOTH ids twice after a strap re-add, inflating n and skewing avg — and + // both numbers would stay plausible, so nothing would look wrong. GROUP BY ts with MIN(pri) + // keeps one row per second and takes the primary's, matching the dedup the chart already does. + // Passing the same id for both is byte-identical to the old single-id read. "SELECT COUNT(*) AS n, AVG(bpm) AS avg, MAX(bpm) AS max FROM (" + - "SELECT bpm FROM hrSample " + - "WHERE deviceId = :deviceId AND ts >= :from AND ts <= :to " + + "SELECT ts, MIN(pri), bpm FROM (" + + "SELECT ts, bpm, 0 AS pri FROM hrSample " + + "WHERE deviceId = :primaryId AND ts >= :from AND ts <= :to " + "UNION ALL " + - "SELECT p.bpm AS bpm FROM ppgHrSample p " + - "WHERE p.deviceId = :deviceId AND p.ts >= :from AND p.ts <= :to " + - "AND NOT EXISTS (SELECT 1 FROM hrSample h WHERE h.deviceId = p.deviceId AND h.ts = p.ts)" + + "SELECT p.ts AS ts, p.bpm AS bpm, 0 AS pri FROM ppgHrSample p " + + "WHERE p.deviceId = :primaryId AND p.ts >= :from AND p.ts <= :to " + + "AND NOT EXISTS (SELECT 1 FROM hrSample h WHERE h.deviceId = p.deviceId AND h.ts = p.ts) " + + "UNION ALL " + + "SELECT ts, bpm, 1 AS pri FROM hrSample " + + "WHERE deviceId = :secondaryId AND ts >= :from AND ts <= :to " + + "UNION ALL " + + "SELECT p.ts AS ts, p.bpm AS bpm, 1 AS pri FROM ppgHrSample p " + + "WHERE p.deviceId = :secondaryId AND p.ts >= :from AND p.ts <= :to " + + "AND NOT EXISTS (SELECT 1 FROM hrSample h WHERE h.deviceId = p.deviceId AND h.ts = p.ts) " + + ") GROUP BY ts" + ")" ) - suspend fun hrWindowStats(deviceId: String, from: Long, to: Long): HrWindowStats + suspend fun hrWindowStats(primaryId: String, secondaryId: String, from: Long, to: Long): HrWindowStats @Query( // #823: `ord` FIRST, so same-second beats come back in EMISSION order. Ordering by rrMs made diff --git a/android/app/src/main/java/com/noop/data/WhoopRepository.kt b/android/app/src/main/java/com/noop/data/WhoopRepository.kt index 780bfaeb9e..89aae70c14 100644 --- a/android/app/src/main/java/com/noop/data/WhoopRepository.kt +++ b/android/app/src/main/java/com/noop/data/WhoopRepository.kt @@ -689,6 +689,14 @@ class WhoopRepository(private val dao: WhoopDao) { suspend fun hrSamples(deviceId: String, from: Long, to: Long, limit: Int = DEFAULT_LIMIT) = dao.hrSamples(deviceId, from, to, limit) + /** #856: HR samples over an EXPLICIT id list, deduped by ts with earlier ids winning — the sample + * twin of [hrBucketsFor], so a workout's ZONE MINUTES bin the same rows its chart plots and its + * Avg HR aggregates. One id ⇒ a plain single-id read. */ + suspend fun hrSamplesFor(deviceIds: List, from: Long, to: Long, limit: Int = DEFAULT_LIMIT): + List = + if (deviceIds.isEmpty()) emptyList() + else mergeHrByTs(deviceIds.map { dao.hrSamples(it, from, to, limit) }) + /** * HR samples over the read-side UNION of the active strap id AND the canonical "my-whoop" (SPINE / * #814 + HIGH-2), deduped by ts with the active strap winning. This is the Kotlin twin of the Swift @@ -742,6 +750,14 @@ class WhoopRepository(private val dao: WhoopDao) { suspend fun hrBuckets(deviceId: String, from: Long, to: Long, bucketSeconds: Long = 300L) = dao.hrBuckets(deviceId, from, to, bucketSeconds) + /** #856: the same dedup over an EXPLICIT id list, so a workout can read its OWN recording strap + * rather than the day-level active ∪ canonical. Order is precedence — earlier ids win per bucket + * start, exactly as [hrBucketsUnion] does. One id ⇒ a plain single-id read. */ + suspend fun hrBucketsFor(deviceIds: List, from: Long, to: Long, bucketSeconds: Long = 300L): + List = + if (deviceIds.isEmpty()) emptyList() + else mergeHrBucketsByStart(deviceIds.map { dao.hrBuckets(it, from, to, bucketSeconds) }) + /** * Downsampled HR buckets over the read-side UNION of the active strap id AND the canonical "my-whoop" * (SPINE / #814 + HIGH-2), deduped by bucket start with the active strap winning. Kotlin twin of the @@ -778,7 +794,7 @@ class WhoopRepository(private val dao: WhoopDao) { rows: List, // HR read key for IMPORTED rows ONLY (Apple/HC/CSV/activity file): they carry no strap HR of their // own, so #77 derives it from the worn strap. STRAP-NATIVE rows ignore this and key on their OWN - // recording strap (see [workoutHrDeviceId]). The canonical "my-whoop" default is the worn strap on a + // recording strap (see [workoutHrDeviceIds]). The canonical "my-whoop" default is the worn strap on a // single-WHOOP install (and every current caller uses it); which strap was worn during an imported // session on a MULTI-strap install is undetermined, so that case is left as-is (not the active strap). strapDeviceId: String = "my-whoop", @@ -807,14 +823,19 @@ class WhoopRepository(private val dao: WhoopDao) { // for a strap-native row (never a hardcoded id), the [strapDeviceId] worn-strap default for an // imported one. A 2nd WHOOP (id "whoop-") used to read the empty "my-whoop" window, so its // strap-native workouts' Avg HR wasn't reconciled from the trace and a null Effort wasn't recomputed. - val hrDeviceId = workoutHrDeviceId(row.source, row.deviceId, strapDeviceId) - val stats = dao.hrWindowStats(hrDeviceId, row.startTs, row.endTs) + // #856: up to two ids, the first winning per second. A detected bout reads only the strap + // that recorded it; an imported row reads the active ∪ canonical union, because it has no + // strap of its own and the worn strap may bank under either after a re-add. + val hrIds = workoutHrDeviceIds(row.source, row.deviceId, strapDeviceId) + val stats = dao.hrWindowStats( + hrIds[0], hrIds.getOrElse(1) { hrIds[0] }, row.startTs, row.endTs, + ) if (stats.n < minSamples || stats.avg == null || stats.max == null) return@map row // #961: recompute Effort from the SAME samples the graph/zones use. Read the raw window ONLY when // this row actually needs a strain (keeps the common no-fill path a single aggregate query), and // let StrainScorer return null on a still-too-thin window (never a fabricated number). val filledStrain = if (needsStrainFill && strainMaxHR != null) { - val samples = dao.hrSamples(hrDeviceId, row.startTs, row.endTs, 8000) + val samples = dao.hrSamples(hrIds[0], row.startTs, row.endTs, 8000) com.noop.analytics.StrainScorer.strain(samples, maxHR = strainMaxHR, sex = strainSex) } else null if (strapNative) { @@ -1575,7 +1596,7 @@ class WhoopRepository(private val dao: WhoopDao) { /** A workout row is STRAP-NATIVE when NOOP recorded/scored it from a strap trace: a "manual" * session or a detected bout (source "-noop"). Everything else (Apple Health / Health Connect / * WHOOP CSV / activity file) is IMPORTED and carries its own avg/max. Single source of truth for the - * classification shared by [fillWorkoutHrFromStrap] and [workoutHrDeviceId]. */ + * classification shared by [fillWorkoutHrFromStrap] and [workoutHrDeviceIds]. */ fun isStrapNativeWorkout(source: String): Boolean { val s = source.lowercase() return s == "manual" || s.endsWith("-noop") @@ -1588,8 +1609,9 @@ class WhoopRepository(private val dao: WhoopDao) { * the active strap [activeStrapId]. Before this both keyed on a hardcoded "my-whoop", so a strap-native * workout on a SECOND WHOOP ("whoop-") read an empty window — its Avg HR went un-reconciled and a * null Effort un-recomputed. (This fill only sets avgHr/maxHr/strain; calories come from the detector.) */ - 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 = + if (isStrapNativeWorkout(source)) listOf(rowDeviceId.removeSuffix("-noop")) + else importedSourceIdsFor(activeStrapId) /** Default row cap on range reads. Matches the Swift call sites' bounded scans. */ const val DEFAULT_LIMIT = 100_000 diff --git a/android/app/src/main/java/com/noop/ui/AppViewModel.kt b/android/app/src/main/java/com/noop/ui/AppViewModel.kt index 057e80d7c7..82c6234b40 100644 --- a/android/app/src/main/java/com/noop/ui/AppViewModel.kt +++ b/android/app/src/main/java/com/noop/ui/AppViewModel.kt @@ -1651,11 +1651,22 @@ class AppViewModel(app: Application) : AndroidViewModel(app) { * HR-curve. A short session wants a finer bucket than the Today 24h chart (300 s would flatten a * 30-min run to ~6 points), so the bucket scales with duration: ~120 buckets across the window, * floored at 15 s and capped at 300 s. Mirrors macOS Repository.workoutHrBuckets. */ - suspend fun workoutHrBuckets(from: Long, to: Long): List { + suspend fun workoutHrBuckets( + from: Long, + to: Long, + source: String = "", + rowDeviceId: String = deviceId, + ): List { if (to <= from) return emptyList() val span = to - from val bucket = (span / 120).coerceIn(15L, 300L) - return runCatching { repository.hrBuckets(deviceId, from, to, bucket) }.getOrDefault(emptyList()) + // #856: read the ids this row actually belongs to. A bout detected on a SECOND WHOOP charts + // from the strap that recorded it; an imported row gets the active ∪ canonical union, since it + // has no strap of its own and the worn strap may bank under either after a re-add. Previously + // this read the single active id, so both cases could chart the wrong data — and disagree with + // the Avg HR on the same card. Defaults keep any caller without a row on today's behaviour. + val ids = WhoopRepository.workoutHrDeviceIds(source, rowDeviceId, deviceId) + return runCatching { repository.hrBucketsFor(ids, from, to, bucket) }.getOrDefault(emptyList()) } /** Per-zone MINUTES for a workout window, binning the strap's raw HR samples into the age-derived @@ -1663,9 +1674,17 @@ class AppViewModel(app: Application) : AndroidViewModel(app) { * percentages, but from the strap's own samples so a session WITHOUT imported zones still gets a * real time-in-zone split. null when the window carries no HR. age <= 0 falls back to 30 y. * Mirrors macOS Repository.workoutZoneMinutes. */ - suspend fun workoutZoneMinutes(from: Long, to: Long): List? { + suspend fun workoutZoneMinutes( + from: Long, + to: Long, + source: String = "", + rowDeviceId: String = deviceId, + ): List? { if (to <= from) return null - val samples = runCatching { repository.hrSamples(deviceId, from, to) }.getOrDefault(emptyList()) + // #856: the same resolved ids the chart and Avg HR use. Binning a different strap's samples + // than the curve plots would put three different answers on one card. + val ids = WhoopRepository.workoutHrDeviceIds(source, rowDeviceId, deviceId) + val samples = runCatching { repository.hrSamplesFor(ids, from, to) }.getOrDefault(emptyList()) if (samples.isEmpty()) return null val age = profileStore.age.toDouble().takeIf { it > 0 } ?: 30.0 val zoneSet = com.noop.analytics.HrZones.zones(age = age) @@ -1677,12 +1696,21 @@ class AppViewModel(app: Application) : AndroidViewModel(app) { /** HRR for one workout (#516), derived from a narrow workout-end + post-workout HR read. The pure * engine owns the intensity and coverage gates, so a disconnect after exercise returns null rather * than an interpolated value. Mirrors macOS Repository.workoutHeartRateRecovery. */ - suspend fun workoutHeartRateRecovery(from: Long, to: Long): com.noop.analytics.HeartRateRecovery.Result? { + suspend fun workoutHeartRateRecovery( + from: Long, + to: Long, + source: String = "", + rowDeviceId: String = deviceId, + ): com.noop.analytics.HeartRateRecovery.Result? { if (to <= from) return null val readFrom = maxOf(from, to - com.noop.analytics.HeartRateRecovery.eligibilityLookbackSeconds) val readTo = to + 5 * 60 + com.noop.analytics.HeartRateRecovery.measurementToleranceSeconds + // #856: the same resolved ids as the chart, zones and Avg HR — the fourth surface on this card. + // The recovery window extends PAST the bout, but the strap that recorded it is still the one on + // the wrist a few minutes later, so a detected bout reads its own strap here too. + val ids = WhoopRepository.workoutHrDeviceIds(source, rowDeviceId, deviceId) val samples = runCatching { - repository.hrSamplesUnion(activeStrapId, readFrom, readTo, limit = 2_000) + repository.hrSamplesFor(ids, readFrom, readTo, limit = 2_000) }.getOrDefault(emptyList()) return com.noop.analytics.HeartRateRecovery.calculate( samples = samples, diff --git a/android/app/src/main/java/com/noop/ui/WorkoutsScreen.kt b/android/app/src/main/java/com/noop/ui/WorkoutsScreen.kt index 2919a1e5fd..b7bd055699 100644 --- a/android/app/src/main/java/com/noop/ui/WorkoutsScreen.kt +++ b/android/app/src/main/java/com/noop/ui/WorkoutsScreen.kt @@ -201,7 +201,7 @@ fun WorkoutsScreen(vm: AppViewModel) { LaunchedEffect(recoveryInputKey, vm.activeStrapId, lastHistorySyncAt) { val built = ArrayList() for (row in recoveryRows) { - val result = vm.workoutHeartRateRecovery(row.startTs, row.endTs) ?: continue + val result = vm.workoutHeartRateRecovery(row.startTs, row.endTs, row.source, row.deviceId) ?: continue built += WorkoutRecoveryTrendPoint(row.startTs, result) } recoveryTrend = built @@ -1295,7 +1295,7 @@ private fun WorkoutDetailSheet(vm: AppViewModel, row: WorkoutRow, onDismiss: () // so it "fills in after sync". null for non-foot sports or when no strap counter covers the window. var steps by remember(row.startTs) { mutableStateOf(null) } LaunchedEffect(row.startTs, row.endTs) { - hrCurve = vm.workoutHrBuckets(row.startTs, row.endTs).map { it.avgBpm } + hrCurve = vm.workoutHrBuckets(row.startTs, row.endTs, row.source, row.deviceId).map { it.avgBpm } steps = if (WorkoutSport.isOnFoot(row.sport)) vm.workoutSteps(row.startTs, row.endTs) else null val imported = parseZonePercents(row.zonesJSON) if (imported != null) { @@ -1306,10 +1306,10 @@ private fun WorkoutDetailSheet(vm: AppViewModel, row: WorkoutRow, onDismiss: () } } if (zoneMinutes == null) { - zoneMinutes = vm.workoutZoneMinutes(row.startTs, row.endTs) + zoneMinutes = vm.workoutZoneMinutes(row.startTs, row.endTs, row.source, row.deviceId) zonesFromImport = false } - heartRateRecovery = vm.workoutHeartRateRecovery(row.startTs, row.endTs) + heartRateRecovery = vm.workoutHeartRateRecovery(row.startTs, row.endTs, row.source, row.deviceId) } ModalBottomSheet( diff --git a/android/app/src/test/java/com/noop/data/WorkoutHrDeviceKeyTest.kt b/android/app/src/test/java/com/noop/data/WorkoutHrDeviceKeyTest.kt index 169c128050..9902cd60da 100644 --- a/android/app/src/test/java/com/noop/data/WorkoutHrDeviceKeyTest.kt +++ b/android/app/src/test/java/com/noop/data/WorkoutHrDeviceKeyTest.kt @@ -13,6 +13,11 @@ import org.junit.Test */ class WorkoutHrDeviceKeyTest { + // #856: the resolver returns a LIST now — one id for a detected bout (the strap that recorded it), + // the active ∪ canonical union for manual/imported. These cases all assert the id that wins, which + // is `.first()` in both branches, so the original expectations are unchanged. + + @Test fun `strap-native sources are classified as strap-native`() { assertTrue(WhoopRepository.isStrapNativeWorkout("manual")) assertTrue(WhoopRepository.isStrapNativeWorkout("MANUAL")) // case-insensitive @@ -34,19 +39,19 @@ class WorkoutHrDeviceKeyTest { // "whoop-aabbcc". The active strap being something else must NOT redirect the read. assertEquals( "whoop-aabbcc", - WhoopRepository.workoutHrDeviceId("whoop-aabbcc-noop", "whoop-aabbcc-noop", activeStrapId = "my-whoop"), + WhoopRepository.workoutHrDeviceIds("whoop-aabbcc-noop", "whoop-aabbcc-noop", activeStrapId = "my-whoop").first(), ) // Canonical single-WHOOP detected row is unchanged from the old "my-whoop" behaviour. assertEquals( "my-whoop", - WhoopRepository.workoutHrDeviceId("my-whoop-noop", "my-whoop-noop", activeStrapId = "my-whoop"), + WhoopRepository.workoutHrDeviceIds("my-whoop-noop", "my-whoop-noop", activeStrapId = "my-whoop").first(), ) } @Test fun `manual row reads HR under its own strap id`() { assertEquals( "whoop-aabbcc", - WhoopRepository.workoutHrDeviceId("manual", "whoop-aabbcc", activeStrapId = "my-whoop"), + WhoopRepository.workoutHrDeviceIds("manual", "whoop-aabbcc", activeStrapId = "my-whoop").first(), ) } @@ -54,11 +59,11 @@ class WorkoutHrDeviceKeyTest { // An Apple/HC/activity-file row carries no strap HR; #77 fills it from the worn strap = active strap. assertEquals( "whoop-aabbcc", - WhoopRepository.workoutHrDeviceId("apple-health", "apple-health", activeStrapId = "whoop-aabbcc"), + WhoopRepository.workoutHrDeviceIds("apple-health", "apple-health", activeStrapId = "whoop-aabbcc").first(), ) assertEquals( "my-whoop", - WhoopRepository.workoutHrDeviceId("activity-file", "activity-file", activeStrapId = "my-whoop"), + WhoopRepository.workoutHrDeviceIds("activity-file", "activity-file", activeStrapId = "my-whoop").first(), ) } @@ -66,7 +71,7 @@ class WorkoutHrDeviceKeyTest { // removeSuffix is a no-op when the suffix is absent — a manual/base id must not be truncated. assertEquals( "whoop-aabbcc", - WhoopRepository.workoutHrDeviceId("manual", "whoop-aabbcc", activeStrapId = "ignored"), + WhoopRepository.workoutHrDeviceIds("manual", "whoop-aabbcc", activeStrapId = "ignored").first(), ) } }