Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion Strand/App/RootView.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
34 changes: 34 additions & 0 deletions Strand/BLE/LiveState.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
5 changes: 5 additions & 0 deletions Strand/BLE/SourceCoordinator.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
66 changes: 48 additions & 18 deletions Strand/Liquid/LiquidTodayView.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -2648,14 +2664,17 @@ 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
)
}
#endif
return .resolve(
activeIsWhoop: live.activeIsWhoop,
connected: live.connected,
batteryPct: live.batteryPct,
charging: live.charging
Expand All @@ -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)
Expand All @@ -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
Expand Down Expand Up @@ -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):
Expand Down Expand Up @@ -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()
Expand Down
13 changes: 10 additions & 3 deletions Strand/MenuBar/MenuBarContent.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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(
Expand Down
3 changes: 2 additions & 1 deletion Strand/Screens/TodayView.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down
53 changes: 46 additions & 7 deletions StrandTests/LiquidBatteryDisplayTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}
Expand All @@ -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")
}
Expand All @@ -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))
}
}
Loading