feat(apple): hourly step import (v26) — per-hour iPhone steps with 90-day backfill - #369
feat(apple): hourly step import (v26) — per-hour iPhone steps with 90-day backfill#369vishk23 wants to merge 5 commits into
Conversation
…-day backfill The Apple Health importer flattened steps to one daily total, so a phone that died mid-hike just read as a low/zero day instead of showing which hours had no data. HealthKit retains hourly statistics historically, so this adds a per-hour store (appleStepHour, PK deviceId+ts) and wires an hourly HKStatisticsCollectionQuery alongside the existing daily collector, with a one-time 90-day backfill on first run so past days answer retroactively too. A transient HealthKit query error now throws instead of silently succeeding, so the backfill flag is only set after a real collection completes — an HK error retries on the next sync instead of permanently losing the 90-day window. Migration registers as v26-apple-step-hour (the next open slot on this repo's migrator) with options: [.ifNotExists] on the table create — forks/sideloads that already carry this table under a different migration identifier converge cleanly instead of failing the migrator.
|
Really clean instrument-first PR — additive One robustness nit on the backfill flag — it closes the error path but not the arguably-more-common empty path: This bridge treats a successful auth request as if !hourlySteps.isEmpty { try await store.upsertAppleStepHours(hourlySteps, …) }
if !hourlyStepsBackfilled { UserDefaults.standard.set(true, forKey: …hourlyStepsBackfilledKey) } // sets even when emptySo: first sync with step-read declined → empty → flag set → user later grants step access → the 90-day hourly backfill is permanently skipped (forward data only). Users grant Health scopes incrementally, so this deny-then-grant path is easy to hit — and it silently loses exactly the historical hours the backfill exists to capture. Suggested 1-line guard — only mark backfilled once real hourly data actually lands: if !hourlySteps.isEmpty {
try await store.upsertAppleStepHours(hourlySteps, deviceId: appleDeviceId)
if !hourlyStepsBackfilled { UserDefaults.standard.set(true, forKey: HealthKitBridge.hourlyStepsBackfilledKey) }
}Trade-off: a genuinely step-less 90-day HealthKit (strap-only user who never carries a phone) would re-scan each sync until some step exists — cheap, bounded, and self-healing, and far better than losing the backfill for deny-then-grant. Since the flag lives in app-target HealthKit code (not unit-testable, rests on the on-device run), erring conservative is worth it. Not blocking given it's instrumentation with no consumer yet — but worth folding in now or as a fast follow, since the whole value of the one-time widen is the historical hours. |
HealthKit returns empty (not an error) when step read-access is denied, so setting the flag unconditionally after a no-throw collection burned the one-time 90-day backfill in a deny-then-grant sequence. Gate the flag-set inside the non-empty branch: a later grant still gets the historical widen, and a genuinely step-less user just re-scans each sync — cheap, bounded, self-healing.
|
Folded in (ff42c13). The flag-set now lives inside the Verified compile-only (this is app-target code CI doesn't build): |
Adds the Room migration (v18 -> v19) + @entity for the appleStepHour table, the Android twin of the Swift WhoopStore v26-apple-step-hour GRDB migration. Column order (deviceId, ts, steps) and PK (deviceId, ts) match the GRDB schema so the .noopbak schema stays byte-identical, pinned by AppleStepHourMigrationTest. SCHEMA-ONLY: the hourly-step import is Apple-Health-sourced (iOS-only; HealthKit has no Android analogue), so no import code is ported — Android carries the table for backup parity but no importer writes to it.
…ream Upstream took every slot this branch had claimed, on both platforms. Four conflicts, all schema-numbering, resolved by moving this PR's migration to the end of each chain — the table itself, its SQL, and its tests are unchanged. - GRDB: `v26-apple-step-hour` → **`v31-apple-step-hour`**. Upstream now holds v26 (efficiency-heal), v27 (ppg-waveform), v28 (raw-imu), v29 (score-input-provenance), v30 (rr-ord). - Room: `MIGRATION_18_19` → **`MIGRATION_24_25`**, database `version` 19 → 25. Upstream is at version 24 / `MIGRATION_23_24`; `MIGRATION_24_25` is appended to `addMigrations(...)` and `AppleStepHour::class` re-added to the entity list alongside upstream's `PpgWaveformSampleEntity` / `RawImuSampleEntity`. - `DeviceRegistryStore.deviceScopedTables` and `Entities.kt` were additive collisions — upstream's new tables and `appleStepHour` all kept. - `AppleStepHourMigrationTest` version-pair assertions updated to 24→25. NOTE: PR #475 currently also claims GRDB v31 (`v31-daily-avg-sdnn`). GRDB keys migrations by name so both would apply correctly in either order, but whichever of the two lands second should bump to v32 to keep the numbering honest. Verified: WhoopStore 299/299 (incl. the 3 AppleStepHourStore tests); Android `./gradlew testFullDebugUnitTest` 3064 tests, only the pre-existing `SyncChipStateTest.lastSyncedAt_takesPriorityOverHistorySync` failure (it fails on clean main too).
|
Rebuilt on current
Heads-up for whoever lands these: #475 currently also claims GRDB v31 ( The review nit from July — only setting the backfill flag once hourly rows actually land, so a deny-then-grant HealthKit sequence keeps the 90-day widen — is in the branch already ( Verified: |
…m 24->25 -> 25->26 Upstream took both slots this branch was sitting in: `v31-deep-capture-channels` (GRDB) and MIGRATION_24_25 (Room), landed by #950 after this branch last rebased. Both had to move regardless of the #475 collision. GRDB `v31-apple-step-hour` -> `v32-apple-step-hour`, registered after upstream's v31. Room `MIGRATION_24_25` -> `MIGRATION_25_26`, database version 25 -> 26, with `AppleStepHour::class` re-added alongside upstream's `V18AuxSampleEntity` and `MIGRATION_25_26` appended to `addMigrations(...)`. `exportSchema` stays `true` (upstream's #775 change); this branch's old `false` is dropped. On the collision with #475: #475 has moved to `v32-daily-avg-sdnn`, so both PRs now sit at v32. That is not an oversight — the schema-parity check requires the BASELINE lineage to read exactly v1..vN with no gaps, so "next" is a single slot and two PRs cannot be green at v32 and v33 simultaneously without one stacking on the other. Each is green standalone; whichever lands first, THIS PR renumbers to v33 / Room 26 -> 27, as offered in its thread. A Room version is a bare integer with no slug, so unlike a GRDB identifier it cannot be disambiguated by declaring a lineage; taking the next free slot is the only resolution. `DeviceRegistryStore.deviceScopedTables` keeps both new tables (upstream's `v18AuxSample` and this branch's `appleStepHour`), so a device-data wipe still clears both. schema_oracle.json (both byte-identical copies) records roomVersion 26, the `v32-apple-step-hour` identifier, and the `appleStepHour` table shape. Verified: WhoopStore 339/339 (including the 3 AppleStepHourStore tests and the renumbered migration), android `./gradlew testFullDebugUnitTest` 3235 tests / 0 failures (5 skipped), up 4 from the 3231 on this tree before the merge.
|
Rebased onto
On the v31 collision with #475 — and being the one that movesI said in this thread I was happy to be the one that moves, and that still holds. What changed is that upstream took v31 in the meantime, so "one of us bumps to v32" became "we both bump, and one of us bumps twice." Both PRs now sit at v32, and that is deliberate rather than an oversight worth flagging. The #889 baseline check requires the registration order to read exactly v1..vN with no gaps — so "next" is a single slot, and #369 at v33 while #475 is at v32 would fail its own CI (v31, then v33) unless it also carried #475's migration. So each PR is green standalone at v32, and whichever you merge first, this one renumbers to v33 / Room 26 → 27. Two reasons it should be this PR rather than #475: I offered, and it is also the cheaper direction. #475 is one additive nullable column in one namespace. This PR must claim a Room integer version too, and a Room version is a bare integer with no slug — unlike a GRDB identifier it cannot be disambiguated by declaring a lineage in (Worth noting the durable fix for this whole class is already open as #905 — it teaches the oracle to accept a declared lineage for identifiers that cannot be renumbered. It does not apply here: neither of these identifiers has shipped under its upstream spelling, so both can honestly be renumbered, and using the escape hatch for convenience would be exactly the accident the check is meant to catch.) Verified: WhoopStore 339/339 including the 3 |
|
Thanks @vishk23 — the implementation here is solid (the deny-then-grant reasoning on the backfill flag especially), but the branch had gone stale in the one way that can't be rebased around: it was 385 commits behind and both migration slots it claimed had been taken since — GRDB Two things I adjusted while porting: the doc claim about missing hours (only hours with a step sample produce a row, so a gap can't distinguish a dead phone from a still hour — it's evidence, not proof), and a new test pinning |
…ackfill Re-cut of #369 (by @vishk23) against current main. That branch was 385 commits behind and BOTH of its migration slots had been taken since: GRDB v32-apple-step-hour collided with v32-rr-src-channel (main is at v37) and Room 26 collided with MIGRATION_25_26 (main is at 30). Renumbered to the current tail rather than mutating shipped migrations. The daily Apple Health import flattens iPhone steps to one appleDaily.steps total, so an hour the phone spent dead or on a desk is invisible - the day just reads low. This adds an hourly table beside it: - GRDB v38-apple-step-hour + Room MIGRATION_30_31 (@database version 31, SCHEMA_VERSION 31): additive CREATE TABLE only, appleStepHour(deviceId, ts, steps), PK (deviceId, ts) like every other per-sample table. - Store API upsertAppleStepHours/appleStepHours mirroring OuraRawStore, with an idempotent ON CONFLICT upsert keyed on the natural key. - HKStatisticsCollectionQuery at hour interval over the same sync window, same anchor and error handling as the existing daily collect(), plus a one-time 90-day backfill flagged only after rows actually land (HealthKit returns EMPTY rather than an error when step access is denied, so gating on no-throw alone would burn the one-time widen on a deny-then-grant). - appleStepHour registered in deviceScopedTables so Forget-device wipes it. - Kotlin schema twin + AppleStepHourMigrationTest, and both byte-identical schema_oracle.json copies updated (roomVersion 31, v38 migration, table entry). Fixes on top of the original: - Corrected the documented claim about what a missing hour means. Only hours with a step sample produce a row, so an absent hour is "no steps recorded" - a dead phone AND a still hour both look identical. Step data carries no separate "was recording" signal, so the doc now says a gap is evidence, not proof, and a consumer must not label it "phone off". - Refreshed the stale renumbering commentary and the "v27 store" header left over from the earlier renumber. - Added a test pinning SCHEMA_VERSION to the migration target so the two cannot drift apart (the #1410 failure mode). SCOPE: plumbing only, no UI reads the table yet - same precedent as #346/#895. Android carries the schema but no importer writes it (HealthKit has no Android analogue), exactly as the older appleDaily table already does, so .noopbak stays byte-identical. Re-review also found the Android Forget-device path incomplete: appleStepHour was registered in the Swift deviceScopedTables (whose test enumerates every deviceId-keyed table automatically) but NOT in the hand-maintained Android twin in DeviceRegistryDao. Android never writes the table, but a .noopbak restored FROM iOS carries its rows, so "delete this device's data" — and in particular "Remove Apple Health imported data", which is exactly this data — left the hourly step history behind. Added the delete + re-key queries, wired both call sites, and extended the fan-out guard test to pin appleStepHour. Validation: full Android suite 4037 tests / 489 classes green (the fan-out guard caught the gap above before this landed), including SchemaOracleTest (compares Room KSP-generated schema against the oracle) and the new migration test. test (WhoopStore) green in CI — that covers the v38 migration, the store API, the GRDB-side oracle and the automatic deviceScopedTables coverage assertion. HealthKitBridge is app-target Swift, validated by app-build. Co-authored-by: vishk23 <119831996+vishk23@users.noreply.github.com>
…ackfill (#1429) Re-cut of #369 (by @vishk23) against current main. That branch was 385 commits behind and BOTH of its migration slots had been taken since: GRDB v32-apple-step-hour collided with v32-rr-src-channel (main is at v37) and Room 26 collided with MIGRATION_25_26 (main is at 30). Renumbered to the current tail rather than mutating shipped migrations. The daily Apple Health import flattens iPhone steps to one appleDaily.steps total, so an hour the phone spent dead or on a desk is invisible - the day just reads low. This adds an hourly table beside it: - GRDB v38-apple-step-hour + Room MIGRATION_30_31 (@database version 31, SCHEMA_VERSION 31): additive CREATE TABLE only, appleStepHour(deviceId, ts, steps), PK (deviceId, ts) like every other per-sample table. - Store API upsertAppleStepHours/appleStepHours mirroring OuraRawStore, with an idempotent ON CONFLICT upsert keyed on the natural key. - HKStatisticsCollectionQuery at hour interval over the same sync window, same anchor and error handling as the existing daily collect(), plus a one-time 90-day backfill flagged only after rows actually land (HealthKit returns EMPTY rather than an error when step access is denied, so gating on no-throw alone would burn the one-time widen on a deny-then-grant). - appleStepHour registered in deviceScopedTables so Forget-device wipes it. - Kotlin schema twin + AppleStepHourMigrationTest, and both byte-identical schema_oracle.json copies updated (roomVersion 31, v38 migration, table entry). Fixes on top of the original: - Corrected the documented claim about what a missing hour means. Only hours with a step sample produce a row, so an absent hour is "no steps recorded" - a dead phone AND a still hour both look identical. Step data carries no separate "was recording" signal, so the doc now says a gap is evidence, not proof, and a consumer must not label it "phone off". - Refreshed the stale renumbering commentary and the "v27 store" header left over from the earlier renumber. - Added a test pinning SCHEMA_VERSION to the migration target so the two cannot drift apart (the #1410 failure mode). SCOPE: plumbing only, no UI reads the table yet - same precedent as #346/#895. Android carries the schema but no importer writes it (HealthKit has no Android analogue), exactly as the older appleDaily table already does, so .noopbak stays byte-identical. Re-review also found the Android Forget-device path incomplete: appleStepHour was registered in the Swift deviceScopedTables (whose test enumerates every deviceId-keyed table automatically) but NOT in the hand-maintained Android twin in DeviceRegistryDao. Android never writes the table, but a .noopbak restored FROM iOS carries its rows, so "delete this device's data" — and in particular "Remove Apple Health imported data", which is exactly this data — left the hourly step history behind. Added the delete + re-key queries, wired both call sites, and extended the fan-out guard test to pin appleStepHour. Validation: full Android suite 4037 tests / 489 classes green (the fan-out guard caught the gap above before this landed), including SchemaOracleTest (compares Room KSP-generated schema against the oracle) and the new migration test. test (WhoopStore) green in CI — that covers the v38 migration, the store API, the GRDB-side oracle and the automatic deviceScopedTables coverage assertion. HealthKitBridge is app-target Swift, validated by app-build. Co-authored-by: vishk23 <119831996+vishk23@users.noreply.github.com>
2 participants
What. The Apple Health import currently flattens iPhone steps to one daily total. This adds an hourly step import alongside it: migration
v26-apple-step-hour(additive tableappleStepHour(deviceId, ts hour-start, steps)), anHKStatisticsCollectionQueryat hour interval over the same sync window (same anchor, auth checks, and error tolerance as the existing dailycollect()), and a one-time 90-day historical backfill (UserDefaults-gated, set only after a successful collection — a HealthKit query error throws into the existing catch and retries next sync, so a transient failure can't permanently skip the backfill).Why. Intra-day phone-vs-strap comparison answers questions daily totals can't: when was the phone actually recording (dead battery windows show as missing hours), how much of a wrist-count gap is arm-motion inflation vs. the phone simply not being carried, and hour-level activity timelines generally. Verified on a real device: a 90-day backfill landed, and an hour-by-hour overlay against strap step ticks cleanly isolated a mid-hike dead-phone window and quantified wrist-vs-pocket divergence per hour.
Scope. Plumbing + tests only — no UI reads the table yet (same precedent as the WHOOP 5 event-log capture, #346). Store API (
upsertAppleStepHours/appleStepHours) mirrors theOuraRawStoreextension style;appleStepHouris listed indeviceScopedTables(wiped with the device's data). Android: Kotlin schema twin included + gradle-tested. The Room migration (v18→v19) +AppleStepHour@entity mirror theappleStepHourGRDB table (columnsdeviceId, ts, steps; PK(deviceId, ts); column order == entity field order) so the.noopbakschema stays byte-identical, pinned byAppleStepHourMigrationTest. No import code is ported: the hourly-step collection is the iOS-only live-HealthKit path (HealthKit has no Android analogue) and the file-based importer both platforms share is untouched../gradlew :app:testFullDebugUnitTestgreen.Tests/verification. WhoopStore package suite green including 3 new store tests (idempotent upsert by natural key, range read) and the device-scoped-tables invariant; both app targets build. The backfill-flag error path is the second commit — a review caught that a swallowed HealthKit error could set the done-flag with zero rows persisted;
collectHourlyStepsnow throws on a real query error so the flag only sets after success.Note: the migration uses ifNotExists so existing sideload databases that already carry this table converge cleanly.