diff --git a/Strand/App/RootView.swift b/Strand/App/RootView.swift index b5bb0222f8..c2a5ca9d5a 100644 --- a/Strand/App/RootView.swift +++ b/Strand/App/RootView.swift @@ -545,7 +545,13 @@ private struct SidebarStatus: View { Text(statusText) .font(StrandFont.rounded(12, weight: .medium)) .foregroundStyle(StrandPalette.textPrimary) - Text(live.batteryPct.map { String(localized: "Battery \(Int($0))%") } ?? String(localized: "Strap not connected")) + // #2208: gated on BOTH the link and whose device it is. This read had NO gate at all, so + // it showed the strap's last charge with nothing connected: `batteryPct` is never cleared, + // which made the honest `nil` branch below unreachable on any install that had paired a + // strap once. "Strap not connected" was dead text. + Text(live.connected && live.activeIsWhoop + ? live.batteryPct.map { String(localized: "Battery \(Int($0))%") } ?? String(localized: "Strap not connected") + : String(localized: "Strap not connected")) .font(StrandFont.rounded(11)) .foregroundStyle(StrandPalette.textTertiary) } diff --git a/Strand/BLE/LiveState.swift b/Strand/BLE/LiveState.swift index 92cfd508a0..4130cbed8c 100644 --- a/Strand/BLE/LiveState.swift +++ b/Strand/BLE/LiveState.swift @@ -72,6 +72,22 @@ public final class LiveState: ObservableObject { /// separate short history to render an actually-moving R-R strip / rolling RMSSD. Appended (never /// replaced) by `setRRIntervals(_:)`; emptied by `clearBiometrics()`. @Published public private(set) var rrRecent: [Int] = [] + /// The WHOOP strap's last reported charge. NOT the active device's. + /// + /// Two properties make this dangerous to read on its own, and both are deliberate. It is the + /// strap's alone, with no other source writing it. And it is never cleared: [clearBiometrics] + /// blanks the ring's charge beside it and leaves this, so a strap's last percentage outlives its + /// link on purpose, which is what lets a reconnect show a number before the first fresh reading. + /// + /// So `connected` does not qualify it. That flag goes true the moment ANY source streams, including + /// a ring's first live heart rate, at which point a stale strap percentage satisfies both halves of + /// the obvious gate. That is #2076 and #2208, the same bug found twice, across ten surfaces. + /// + /// Any readout naming the ACTIVE device must go through `LiveConsoleReadout.batteryPercent`, which + /// substitutes the ring's own charge. Any readout labelled as the strap's must pair this with + /// [activeIsWhoop] and show nothing when it is false: substituting there would put a ring's number + /// under a WHOOP heading. Which of the two a surface needs depends on what it claims to be showing, + /// and that is a question about the label rather than about this field. @Published public var batteryPct: Double? = nil /// Strap battery pack VOLTAGE (mV), decoded from the ~8-min BATTERY_LEVEL event (mv@21/@25) and the /// GET_EXTENDED_BATTERY_INFO response (#592). Shown on the Devices card as a "x.xx V" readout beside the @@ -124,6 +140,24 @@ public final class LiveState: ObservableObject { /// That is #2075, where a ring on 93% displayed the strap's 72%. Nil when no ring has reported. @Published public var ouraBatteryPct: Int? = nil + /// Whether the ACTIVE device is a WHOOP, published so a readout can answer "whose charge is this" + /// without observing `AppModel`. + /// + /// It sits here because [batteryPct] does, and the two are only meaningful together. `batteryPct` is + /// the strap's and is never cleared, deliberately, so on its own it cannot say whether it describes + /// the device the wearer is currently looking at. Every surface that reads it needs this alongside, + /// and `Today` in particular cannot reach the device registry: it observes `BLEManager` rather than + /// `AppModel` on purpose, because `AppModel` publishes on the 1 Hz heart-rate tick and observing it + /// would re-render the whole screen every second. + /// + /// Written in ONE place, `SourceCoordinator.activeDeviceChanged`, from the same `activeDeviceId` + /// transition that decides which live source runs. Defaults to true, matching + /// `LiveConsoleReadout.activeIsWhoop`'s WHOOP-first default for an unresolvable row. + /// + /// Not cleared by [clearBiometrics]: which device is active is not a biometric and does not stop + /// being true when a link drops. (#2208) + @Published public var activeIsWhoop: Bool = true + // MARK: - Battery runtime estimate (#713) /// Rolling buffer of `(unix-seconds, SoC%)` battery readings banked from the live link, the twin of diff --git a/Strand/BLE/SourceCoordinator.swift b/Strand/BLE/SourceCoordinator.swift index a8f2fb1870..99ac55a5c7 100644 --- a/Strand/BLE/SourceCoordinator.swift +++ b/Strand/BLE/SourceCoordinator.swift @@ -158,6 +158,11 @@ final class SourceCoordinator: ObservableObject { /// • WHOOP active after a strap → stop the strap source + resume WHOOP. /// • A generic strap → pause WHOOP + (re)start `StandardHRSource` for that strap's id. func activeDeviceChanged(to id: String) { + // #2208: publish whose device this is BEFORE any branch returns. Readouts that show the strap's + // charge need it to know the number is not the active device's, and the Apple Watch path below + // short-circuits, so setting it inside the WHOOP/strap split would leave a watch reading `true`. + live.activeIsWhoop = isWhoop(id) + // The Apple Watch is a HealthKit source with `peripheralId: nil` (see `AppleWatchDevice`): there is // no BLE peripheral to connect, and the M1 live read happens entirely in `HealthKitBridge`'s // observers + sync, off this BLE coordinator. Short-circuit BEFORE the WHOOP branch so we never diff --git a/Strand/Liquid/LiquidTodayView.swift b/Strand/Liquid/LiquidTodayView.swift index ad137e383e..839caab4f2 100644 --- a/Strand/Liquid/LiquidTodayView.swift +++ b/Strand/Liquid/LiquidTodayView.swift @@ -2526,8 +2526,24 @@ extension LiquidTodayView { case pending(charging: Bool) /// A reading from the current link. case charge(pct: Double, charging: Bool) - - static func resolve(connected: Bool, batteryPct: Double?, charging: Bool?) -> StrapBatteryDisplay { + /// The strap is not the active device, so this control has nothing to say and is not drawn. + /// + /// Distinct from [offline], which asserts a strap that IS active is not connected. Collapsing the + /// two put a crossed-out bolt and "strap not connected" on the header of a wearer whose ring was + /// streaming, which is a different false claim from the one #2208 is about rather than a fix for + /// it, and the only one a ring-only wearer would see every day. (@pipiche38 on #2216) + case notActiveDevice + + /// #2208: `activeIsWhoop` is required, not defaulted. `connected` alone was never enough: it is + /// true the moment ANY source streams, `batteryPct` is the strap's and is never cleared, so under + /// an active ring both halves of the old gate passed and this drew the strap's charge. Charging + /// is strap-only for the same reason, so a non-WHOOP active device reports neither. + /// + /// No default value on purpose. A defaulted flag is one a future call site can forget, and + /// forgetting it reinstates exactly this bug in a form that still compiles. + static func resolve(activeIsWhoop: Bool, connected: Bool, + batteryPct: Double?, charging: Bool?) -> StrapBatteryDisplay { + guard activeIsWhoop else { return .notActiveDevice } guard connected else { return .offline } guard let pct = batteryPct else { return .pending(charging: charging == true) } return .charge(pct: pct, charging: charging == true) @@ -2648,7 +2664,9 @@ private struct LiquidBatteryButton: View { private var batteryDisplay: LiquidTodayView.StrapBatteryDisplay { #if DEBUG if DemoSyncHarness.active { + // The harness stands in for a connected WHOOP, so it answers this the way one would. return .resolve( + activeIsWhoop: true, connected: true, batteryPct: DemoSyncHarness.batteryPercent, charging: DemoSyncHarness.charging @@ -2656,6 +2674,7 @@ private struct LiquidBatteryButton: View { } #endif return .resolve( + activeIsWhoop: live.activeIsWhoop, connected: live.connected, batteryPct: live.batteryPct, charging: live.charging @@ -2664,7 +2683,7 @@ private struct LiquidBatteryButton: View { private var indicatorState: ChargeSyncIndicator.BatteryState { switch batteryDisplay { - case .offline: + case .offline, .notActiveDevice: return .offline case .pending(let charging): return .pending(charging: charging) @@ -2674,21 +2693,29 @@ private struct LiquidBatteryButton: View { } var body: some View { - Button { router.openDevices() } label: { - ChargeSyncIndicator( - batteryState: indicatorState, - syncing: syncing, - chunks: syncChunks - ) + // Not drawn at all when the strap is not the active device. The alternative is a glyph that + // has to say SOMETHING about a strap nobody is wearing, and every option is a claim: a charge + // that is not the active device's, or a crossed-out bolt asserting a disconnection that is not + // the interesting fact. The two Today rows already resolve it this way. (#2208) + if case .notActiveDevice = batteryDisplay { + EmptyView() + } else { + Button { router.openDevices() } label: { + ChargeSyncIndicator( + batteryState: indicatorState, + syncing: syncing, + chunks: syncChunks + ) + } + .nativeLiquidGlassSyncButton() + .accessibilityLabel(batteryAccessibility) + .debouncedSyncSignal(syncingRaw, into: $syncing) + // DEBUG-gated at the CALL SITE too, not just in the body: in Release the harness must cost + // literally nothing, rather than an async task created and immediately returned per appearance. + #if DEBUG + .task { await runDemoSyncCycleIfNeeded() } + #endif } - .nativeLiquidGlassSyncButton() - .accessibilityLabel(batteryAccessibility) - .debouncedSyncSignal(syncingRaw, into: $syncing) - // DEBUG-gated at the CALL SITE too, not just in the body: in Release the harness must cost - // literally nothing, rather than an async task created and immediately returned per appearance. - #if DEBUG - .task { await runDemoSyncCycleIfNeeded() } - #endif } /// DEBUG `--demo-sync` only: loop the syncing signal so the charge→sync morph plays in both @@ -2755,6 +2782,8 @@ private struct LiquidBatteryButton: View { } switch batteryDisplay { + case .notActiveDevice: + return "" // not drawn; the label is unreachable and must not claim anything case .offline: return String(localized: "Strap battery, strap not connected") case .pending(let charging): @@ -2871,7 +2900,8 @@ private struct LiquidSyncStatusRow: View { private struct LiquidStrapBatteryRow: View { @EnvironmentObject var live: LiveState var body: some View { - if live.connected, let pct = live.batteryPct { + // #2208: the strap's charge only when the strap is the active device. + if live.connected, live.activeIsWhoop, let pct = live.batteryPct { HStack { Text("Strap battery").font(StrandFont.subhead).foregroundStyle(StrandPalette.textSecondary) Spacer() diff --git a/Strand/MenuBar/MenuBarContent.swift b/Strand/MenuBar/MenuBarContent.swift index 21d589740d..f9eca49445 100644 --- a/Strand/MenuBar/MenuBarContent.swift +++ b/Strand/MenuBar/MenuBarContent.swift @@ -105,8 +105,15 @@ public struct MenuBarContent: View { : live.connected ? String(localized: "CONNECTED") : String(localized: "OFFLINE") } + /// #2208: the strap's charge, or nil when it is not the active device's to report. This surface read + /// `live.batteryPct` with NO gate, so it showed the strap's last percentage with nothing connected and + /// under an active ring alike. One accessor so the number and its tint cannot disagree. + private var strapBatteryPct: Double? { + (live.connected && live.activeIsWhoop) ? live.batteryPct : nil + } + private var batteryTone: StrandTone { - guard let pct = live.batteryPct else { return .neutral } + guard let pct = strapBatteryPct else { return .neutral } switch pct { case ..<15: return .critical case ..<35: return .warning @@ -212,8 +219,8 @@ public struct MenuBarContent: View { HStack(spacing: 0) { statCell( "BATTERY", - live.batteryPct.map { "\(Int($0.rounded()))%" } ?? "—", - tint: live.batteryPct == nil ? StrandPalette.textPrimary : toneColor(batteryTone) + strapBatteryPct.map { "\(Int($0.rounded()))%" } ?? "—", + tint: strapBatteryPct == nil ? StrandPalette.textPrimary : toneColor(batteryTone) ) cellDivider statCell( diff --git a/Strand/Screens/TodayView.swift b/Strand/Screens/TodayView.swift index f227ff24cb..d92d314947 100644 --- a/Strand/Screens/TodayView.swift +++ b/Strand/Screens/TodayView.swift @@ -5787,7 +5787,8 @@ private struct StrapBatteryRow: View { } var body: some View { - if live.connected, let pct = live.batteryPct { + // #2208: the strap's charge only when the strap is the active device. + if live.connected, live.activeIsWhoop, let pct = live.batteryPct { Divider().overlay(StrandPalette.hairline) HStack(spacing: 10) { SourceBadge("Strap battery", tint: tint(pct)) diff --git a/StrandTests/LiquidBatteryDisplayTests.swift b/StrandTests/LiquidBatteryDisplayTests.swift index bd13f7a1d8..352ea98174 100644 --- a/StrandTests/LiquidBatteryDisplayTests.swift +++ b/StrandTests/LiquidBatteryDisplayTests.swift @@ -19,7 +19,7 @@ final class LiquidBatteryDisplayTests: XCTestCase { /// ever) telling us a number. The old view nested the bolt inside `if let pct`, making this state /// unrenderable — it drew `bolt.slash` at a wearer who was sitting on the charger. func testChargingIsReportedEvenWithNoChargeReadingYet() { - let d = Display.resolve(connected: true, batteryPct: nil, charging: true) + let d = Display.resolve(activeIsWhoop: true, connected: true, batteryPct: nil, charging: true) XCTAssertEqual(d, .pending(charging: true), "a known charging state must survive a missing % — it is the wearer's live question") } @@ -28,7 +28,7 @@ final class LiquidBatteryDisplayTests: XCTestCase { /// `.pending(charging: false)` and `.offline` must stay distinguishable so the view can render one as /// a pending ellipsis and the other as a crossed-out bolt. func testConnectedWithNoReadingIsPendingNotOffline() { - let d = Display.resolve(connected: true, batteryPct: nil, charging: nil) + let d = Display.resolve(activeIsWhoop: true, connected: true, batteryPct: nil, charging: nil) XCTAssertEqual(d, .pending(charging: false)) XCTAssertNotEqual(d, .offline, "connected-but-silent is not the same claim as no link") } @@ -40,28 +40,67 @@ final class LiquidBatteryDisplayTests: XCTestCase { /// forever, and a view keying off `batteryPct` alone shows a dead strap's stale charge as if live. /// During the incident that rendered a 21 h old 11% identically to a fresh one. func testStaleChargeIsNotShownOnceTheLinkIsGone() { - let d = Display.resolve(connected: false, batteryPct: 11, charging: false) + let d = Display.resolve(activeIsWhoop: true, connected: false, batteryPct: 11, charging: false) XCTAssertEqual(d, .offline, "a % with no link behind it must not render as a live reading") } /// Disconnect must also drop a charging bit — nothing about the old link is still true. func testStaleChargingFlagIsNotShownOnceTheLinkIsGone() { - XCTAssertEqual(Display.resolve(connected: false, batteryPct: nil, charging: true), .offline) + XCTAssertEqual(Display.resolve(activeIsWhoop: true, connected: false, batteryPct: nil, charging: true), .offline) } // MARK: - The normal path still reads normally func testConnectedReadingCarriesPctAndChargingThrough() { - XCTAssertEqual(Display.resolve(connected: true, batteryPct: 87.4, charging: true), + XCTAssertEqual(Display.resolve(activeIsWhoop: true, connected: true, batteryPct: 87.4, charging: true), .charge(pct: 87.4, charging: true)) - XCTAssertEqual(Display.resolve(connected: true, batteryPct: 87.4, charging: false), + XCTAssertEqual(Display.resolve(activeIsWhoop: true, connected: true, batteryPct: 87.4, charging: false), .charge(pct: 87.4, charging: false)) } /// `charging` is `Bool?` — nil means "the strap hasn't said" (no BATTERY_LEVEL event this session), /// which must read as not-charging, never as charging. Same `== true` posture as the rest of the app. func testUnknownChargingReadsAsNotCharging() { - XCTAssertEqual(Display.resolve(connected: true, batteryPct: 50, charging: nil), + XCTAssertEqual(Display.resolve(activeIsWhoop: true, connected: true, batteryPct: 50, charging: nil), .charge(pct: 50, charging: false)) } + + // MARK: - #2208 whose charge is this + + /// The reported bug. `connected` goes true the moment ANY source streams, and the strap's percentage + /// is never cleared, so under an active ring both halves of the old gate passed and Today drew the + /// strap's charge. A non-WHOOP active device must show nothing rather than someone else's number. + func testRingActiveShowsNothingEvenWithAStalestrapCharge() { + XCTAssertEqual(Display.resolve(activeIsWhoop: false, connected: true, batteryPct: 72.4, charging: false), + .notActiveDevice) + } + + /// Charging is the strap's BATTERY_LEVEL event, so it is strap-only for the same reason. A ring must + /// not inherit the strap's charger state either. + func testRingActiveShowsNothingWhileTheStrapCharges() { + XCTAssertEqual(Display.resolve(activeIsWhoop: false, connected: true, batteryPct: 88, charging: true), + .notActiveDevice) + } + + /// A ring active with no strap charge ever recorded is the same answer, reached the other way. + func testRingActiveWithNoStrapChargeIsAlsoNotDrawn() { + XCTAssertEqual(Display.resolve(activeIsWhoop: false, connected: true, batteryPct: nil, charging: nil), + .notActiveDevice) + } + + /// The distinction @pipiche38 asked for on #2216. A strap that IS active and disconnected is offline, + /// a real claim worth making. A strap that is not the active device is a different answer entirely, + /// and collapsing the two told a wearer with a streaming ring that their strap was not connected. + func testNotActiveIsNotTheSameAnswerAsOffline() { + XCTAssertNotEqual(Display.resolve(activeIsWhoop: false, connected: true, batteryPct: 72, charging: false), + Display.resolve(activeIsWhoop: true, connected: false, batteryPct: 72, charging: false)) + XCTAssertEqual(Display.resolve(activeIsWhoop: true, connected: false, batteryPct: 72, charging: false), + .offline) + } + + /// The WHOOP path is unchanged: this fix must not blank a strap that IS the active device. + func testWhoopActiveStillReportsItsCharge() { + XCTAssertEqual(Display.resolve(activeIsWhoop: true, connected: true, batteryPct: 61, charging: false), + .charge(pct: 61, charging: false)) + } } diff --git a/android/app/src/main/java/com/noop/ui/TodayScreen.kt b/android/app/src/main/java/com/noop/ui/TodayScreen.kt index 480f6b1cca..bcba5b975d 100644 --- a/android/app/src/main/java/com/noop/ui/TodayScreen.kt +++ b/android/app/src/main/java/com/noop/ui/TodayScreen.kt @@ -333,6 +333,10 @@ fun TodayScreen( val days by viewModel.recentDays.collectAsStateWithLifecycle() val activeDayCycle by viewModel.activeDayCycle.collectAsStateWithLifecycle() val spo2CandidateByDay by viewModel.spo2CandidateByDay.collectAsStateWithLifecycle() + // #2208: `connected` alone never said WHOSE charge liveSnap.batteryPct is. It goes true the moment any + // source streams, and the strap's percentage is never cleared, so under an active ring both halves of + // the old gate passed and Today drew the strap's charge. Same seam the Devices list already uses. + val activeIsWhoop by viewModel.activeIsWhoop.collectAsStateWithLifecycle() val v5Signals by viewModel.v5Signals.collectAsStateWithLifecycle() val cycleEnabled by viewModel.cycleTrackingEnabled.collectAsStateWithLifecycle() val cycleHidden by viewModel.cycleAwarenessHidden.collectAsStateWithLifecycle() @@ -1392,6 +1396,7 @@ fun TodayScreen( humanDate = humanDate, selectedDay = selectedDay, batteryPct = if (liveSnap.connected) liveSnap.batteryPct else null, + strapIsActiveDevice = activeIsWhoop, backfilling = liveSnap.backfilling, syncChunksThisSession = liveSnap.syncChunksThisSession, lastSyncAt = liveSnap.lastSyncAt, @@ -1901,8 +1906,16 @@ fun TodayScreen( item { TodaySourcesSection( footer, - strapBatteryPct = if (liveSnap.connected) liveSnap.batteryPct?.roundToInt() else null, - strapBatteryEstimate = if (liveSnap.connected) batteryEstimateText else null, + // NOT routed through LiveConsoleReadout.batteryPercent, which substitutes the RING's charge + // for a non-WHOOP active device. That is right for a readout that names the active device, + // and wrong here: this value lands in the SourceRow badged `today_source_whoop`, and it + // also feeds that row's `present` flag. Substituting would put the ring's number under a + // WHOOP label and assert a WHOOP source that is not there, which is worse than the stale + // reading being fixed. Nothing is the honest answer for a strap that is not active. + strapBatteryPct = if (liveSnap.connected && activeIsWhoop) + liveSnap.batteryPct?.roundToInt() else null, + // The runtime estimate is banked from strap SoC samples, so it is the strap's alone. + strapBatteryEstimate = if (liveSnap.connected && activeIsWhoop) batteryEstimateText else null, expanded = sourcesExpanded, onToggle = { sourcesExpanded = !sourcesExpanded }, ) @@ -2499,6 +2512,11 @@ private fun LiquidTodayHeader( humanDate: String, selectedDay: LocalDate, batteryPct: Double?, + /** Whether the STRAP is the active device. Separate from [batteryPct] on purpose: that says what + * the control reads, this says whether the control should exist. A null percentage while the strap + * IS active means "connected, no reading yet" and is worth drawing; a strap that is not the active + * device has nothing to say and is not drawn at all. (#2208) */ + strapIsActiveDevice: Boolean, // #245: sync state for the compact header chip (twin of iOS SyncStatusChip). backfilling: Boolean = false, syncChunksThisSession: Int = 0, @@ -2628,7 +2646,11 @@ private fun LiquidTodayHeader( // disc → the quick-actions menu). Sized to match the rest of the liquid cluster (shared HeaderClusterControl). QuickActionDisc(onClick = onQuickActions) // (c) Strap battery ring showing the % (iOS LiquidBatteryButton). Tap → Devices. - LiquidBatteryRing(batteryPct = batteryPct, onClick = onOpenDevices) + // Not drawn when the strap is not the active device: an empty "Strap battery" ring under a + // streaming ring is a control asserting something about a strap nobody is wearing. + if (strapIsActiveDevice) { + LiquidBatteryRing(batteryPct = batteryPct, onClick = onOpenDevices) + } } } } diff --git a/android/app/src/test/java/com/noop/ui/LiveConsoleReadoutTest.kt b/android/app/src/test/java/com/noop/ui/LiveConsoleReadoutTest.kt index be578af183..c9bc34f41e 100644 --- a/android/app/src/test/java/com/noop/ui/LiveConsoleReadoutTest.kt +++ b/android/app/src/test/java/com/noop/ui/LiveConsoleReadoutTest.kt @@ -109,4 +109,22 @@ class LiveConsoleReadoutTest { // Symmetry: a ring that reported earlier must not leak into a strap's readout either. assertNull(LiveConsoleReadout.batteryPercent(true, null, 93)) } + + /** + * #2208: Today used to gate on `connected` alone, which goes true the moment ANY source streams, + * while the strap's percentage is never cleared. Both halves passed under an active ring and the + * screen drew the strap's charge. The seam's answer for a non-WHOOP active device is the ring's own + * number, or nothing, never the strap's. + */ + @Test + fun ringActiveNeverFallsBackToTheStrapCharge() { + assertEquals(93, LiveConsoleReadout.batteryPercent(false, 72.4, 93)) + assertNull(LiveConsoleReadout.batteryPercent(false, 72.4, null)) + } + + /** And the strap keeps reporting when it IS the active device. */ + @Test + fun whoopActiveStillReportsItsOwnCharge() { + assertEquals(72, LiveConsoleReadout.batteryPercent(true, 72.4, 93)) + } }