Surface pages-behind on the sync chip while backfilling (#689/#815 follow-up)#833
Open
digitalerdude wants to merge 2 commits into
Open
Surface pages-behind on the sync chip while backfilling (#689/#815 follow-up)#833digitalerdude wants to merge 2 commits into
digitalerdude wants to merge 2 commits into
Conversation
…anbr#815 follow-up) PR ryanbr#806 declined a percentage-progress sync indicator because the WHOOP protocol never reveals a total pending-record count. But ryanbr#818/ryanbr#821 confirmed GET_DATA_RANGE's pagesBehind field (write pointer minus read pointer against the ring capacity) decodes correctly and validated it against real captures on both WHOOP 4.0 and 5.0/MG (ryanbr#815) — a real, bounded backlog measure, just not a percentage. It was already logged as a diagnostic but never stored. This threads the single GET_DATA_RANGE sample taken at connect into LiveState (pagesBehindAtConnect) and appends it as a "N pages behind" detail to the sync chip's existing chunk count while backfilling is true — no new BLE traffic, no re-polling mid-offload (see ryanbr#377 on firmware pacing), no percentage math. nil renders identically to before this field existed. Swift: LiveState.pagesBehindAtConnect, banked in BLEManager's handleDataRangeResponse, surfaced via SyncChipState.syncing(chunks:pagesBehind:) in both SyncStatusChip and LiquidSyncChip, localized into all 8 shipped locales. Kotlin: LiveState.pagesBehindAtConnect (WhoopBleClient), SyncChipState.Syncing extended (TodayScoring), SyncStatusChip/ChipCapsule render the detail (TodayScreen), strings added across the 6 shipped locale files. Tests: SyncChipStateTests (iOS) + SyncChipStateTest (Kotlin) cover the new pagesBehind-present case; GattCrashSafetyTest asserts disconnect clears the sample so it can't outlive the link.
DataRange.pagesBehind() returns Long on Kotlin (Int on Swift, a pre-existing platform asymmetry) — convert at the LiveState write so pagesBehindAtConnect stays Int, matching the Swift type. TodayScreen reads the header's LiveState through a derivedStateOf snapshot (TodayLiveSnapshot) that scopes recomposition to a minimal stable field set, not the raw LiveState — the new field needed adding there too, not just on LiveState itself.
ryanbr
added a commit
that referenced
this pull request
Jul 26, 2026
…836) The Android half of this PR made the Kotlin aggregate count PPG-derived seconds. That left the twins disagreeing a second way: Swift reduced store.hrSamples(limit: 8000), so any workout longer than ~2h13m at 1 Hz reported the mean of its FIRST 8000 samples as the session average. On a 3h session with drifting HR that is 131 bpm against a true 135 — wrong on its own terms, not merely divergent. Adds WhoopStore.hrWindowStats, the twin of WhoopDao.hrWindowStats: same measured-∪-PPG union, same anti-join, aggregated in SQLite over the whole window with no row limit. Swift keeps CAST(ROUND(p.bpm) AS INTEGER) because its ppgHrSample.bpm column is REAL where Kotlin's is INTEGER; both decoders emit whole bpm (#219), so the values agree and the two queries return identical results on their own column types — verified by running each against its platform's schema: (415, 141.8867469879518, 146) both. Deliberately put in WhoopStore rather than Repository: swift-packages.yml runs Packages/** tests, while Strand/ is the app target that nothing compiles. So the query and its five tests get real CI coverage, which the old in-Repository reduce never had. Also faster on the path this function exists to protect. #833 moved an 8000-row materialise-and-reduce off the main actor to stop a first-paint freeze; now the common case (no strain fill) materialises no rows at all. Rows are read only when a strain backfill needs the series, where the 8000 cap correctly stays — it bounds the strain window, not the average. That mirrors Kotlin exactly. Retires Repository.reduceWorkoutHr, which had no callers left, and its test file with it. What it guarded (mean rounding) is now covered by the WhoopStore tests, which CI actually executes — StrandTests does not run anywhere today.
ryanbr
added a commit
that referenced
this pull request
Jul 26, 2026
The chart and the average read different tables. The chart uses hrBuckets and the zone/Effort paths use hrSamples — all three UNION measured hrSample with PPG-derived ppgHrSample (#156). hrWindowStats, which backs Avg HR, read hrSample alone. Three of four consumers of the same workout window counted the union; the fourth did not. fillWorkoutHrFromStrap needs 60 samples before it shows anything. On a WHOOP 5 the firmware banks v26 PPG instead of v18 HR per second, so a PPG-heavy workout draws a full chart and still counts under 60 measured rows — Avg HR renders blank. That is the reporter (#836) exactly: "as the HR data is shown on the chart it can be calculated", only SOME workouts affected (PPG density varies per window), and manual AND Health Connect both hit because both pass the same gate. Effort is blocked by the same gate, which is why the report said "all details", plural. Both platforms now aggregate the same measured-∪-PPG rows, with the identical anti-join, so a measured second is never double-counted by its PPG estimate. maxHr can now be a PPG-derived peak — correct, and the point: this function exists to keep the shown number agreeing with the graph and zones, which already included those rows. Stored PPG rows already cleared a 0.3 confidence floor at estimate time on both platforms, so nothing unvetted enters. iOS had a second, independent bug. It reduced store.hrSamples(limit: 8000), so any workout over ~2h13m at 1 Hz reported the mean of its FIRST 8000 samples as the session average — 131 bpm against a true 135 on a 3h session with drifting HR. Adds WhoopStore.hrWindowStats, the Kotlin query's twin, uncapped. Swift keeps CAST(ROUND(p.bpm) AS INTEGER) because its ppgHrSample.bpm column is REAL where Kotlin's is INTEGER; both decoders emit whole bpm (#219), and each query returns (415, 141.8867469879518, 146) against its own platform's schema. Put in WhoopStore rather than Repository on purpose: swift-packages.yml runs Packages/** tests while Strand/ is the app target nothing compiles, so the query and its five tests get real CI coverage the old in-Repository reduce never had. Also improves #833 rather than fighting it. That freeze fix moved an 8000-row materialise-and-reduce off the main actor; the common path now materialises no rows at all. Rows are read only for a strain backfill, where the 8000 cap correctly stays — it bounds the strain window, not the average. Same shape as Kotlin. No schema change, no migration, read-only. Empty windows still return n=0 / null avg so the caller guard is unchanged, and a device with no PPG at all (WHOOP 4, or a 5 that banked v18 throughout) reads byte-identically to before. Retires Repository.reduceWorkoutHr, which had no callers left, and its test file. Not verified by CI: the Kotlin query has not been through Room KSP (android.yml is disabled), though hrBuckets ten lines above is the same aggregate-over-union-subquery shape and ships today; and the Repository.swift wiring is in the uncompiled app target. A staging build exercises both. Unconfirmed: whether the reporter's specific workouts are PPG-heavy. The mechanism is established from the code; their export would confirm it.
ryanbr
reviewed
Jul 26, 2026
ryanbr
left a comment
ryanbr
left a comment
Owner
There was a problem hiding this comment.
Nicely scoped — reusing the sample already taken at connect instead of re-polling respects #377, and pagesBehind is a real bounded measure rather than the fake percentage that was rightly declined on #806. Localization is complete this time (catalog + all six Android locale files), shared pagesBehindDetail keeps the two chips consistent, and the accessibility label carrying the same figure is a good touch.
Two things:
-
No pluralisation —
pagesBehind == 1renders "1 pages behind" on both platforms. Android uses a plain<string name="…pages_behind">%1$d pages behind</string>rather than<plurals>, and Swift usesString(localized: "\($0) pages behind")with no variations. The repo already has the machinery on both sides — 7<plurals>blocks in the basestrings.xml, and 4 catalog entries with plural variations (%lld days,%lld markers,%lld sessions). Same for the accessibility string, which has two counts to vary. Worth also deciding what0should do: it currently renders "0 pages behind" while the chip says syncing. -
The figure is frozen at connect, so during a long offload the chip shows a number that never moves. That is the right protocol call, but "N pages behind" reads as live and a static count can look stuck. Would "N pages behind at connect" (or similar) be more honest, given you deliberately are not re-polling?
Also: your unticked testFullDebugUnitTest box is no longer blocked — #828 landed the missing fake overrides, so the Android suite compiles again. Worth re-running before this merges.
This was referenced Jul 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #806, addressing this comment: a percentage sync-progress indicator was correctly declined there (the WHOOP protocol never reveals a total pending-record count), but
pagesBehind— decoded fromGET_DATA_RANGE's ring-buffer write/read pointers — is a real, bounded backlog measure, and #818/#821 confirmed the decode against real captures on both WHOOP 4.0 and 5.0/MG. This surfaces it as a static "N pages behind" detail on the sync chip, using the single sample already taken at connect — no new BLE traffic, no re-polling mid-offload (per #377's firmware-pacing concern), no percentage math.LiveState.pagesBehindAtConnect, banked inBLEManager.handleDataRangeResponse(previously log-only).SyncChipState.syncinggained an optionalpagesBehind, with sharedpagesBehindDetail/syncingAccessibilityLabelhelpers used by bothSyncStatusChip(classic Today) andLiquidSyncChip(Liquid Today header).LiveState.pagesBehindAtConnect(WhoopBleClient),SyncChipState.Syncingextended (TodayScoring),SyncStatusChip/ChipCapsulerender the detail (TodayScreen) — including theTodayLiveSnapshotrecomposition-scoping DTO, which needed the field too.8d49c380/Sync indicator missing (or I am dumb?) #245's review.nil/null(no sample yet, or a family/frame that doesn't decode) renders identically to the chip's existing chunk-count-only behavior — purely additive.This is scoped deliberately narrow per your "worth revisiting" framing rather than a firm spec — happy to adjust wording or visual treatment (the chip is still marked DRAFT in the code) if you'd rather it look different, or if you'd prefer the detail live somewhere else (e.g. the Data Sources card's
LiquidSyncStatusRow, which I left untouched to keep this PR to one surface).Test plan
swift build/swift testforPackages/WhoopProtocol(untouched by this PR, sanity-only)xcodegen generate && xcodebuild -scheme Strand -destination 'platform=macOS' build— succeedsStrandTests.SyncChipStateTests— 7/7 pass (includes the new pages-behind case)Tools/i18n_audit.py --ci origin/main— clean on both platforms./gradlew compileFullDebugKotlin— succeeds (found and fixed two real Kotlin issues along the way: aLong→Intmismatch sinceDataRange.pagesBehind()returnsLongon Kotlin vsInton Swift, and theTodayLiveSnapshotDTO needing the new field)./gradlew testFullDebugUnitTest— blocked module-wide by a pre-existing, unrelated gap: threeFakeRegistryDao/FakeDaotest fakes are missing methodsDeviceRegistryDaonow declares. Not caused by this change (none of the touched files), and it was already flagged in Add a sync-status chip to the Liquid Today header (#245) #806's review thread.