Hydration: import water from Apple Health and Health Connect (#949) - #974
Conversation
Drinks logged in a dedicated hydration app or by a smart bottle had to be entered into NOOP a second time. Both platforms already read their health store; this adds water to what they read. Imported water is kept in its OWN metricSeries row (`hydrationImported`) rather than folded into the hand-logged total, because the two behave differently on write: the hand-logged row ACCUMULATES (each tap adds to it) while the imported row is REPLACED with the health store's recomputed day sum. That replacement is what makes re-importing idempotent without tracking individual sample ids, and it means a drink deleted in the source app makes the figure go down here instead of being stranded forever. Every day in the window is written, zeros included, so a deletion actually propagates. Keeping them apart is also what keeps the write paths correct. A quick-add stores `current + amount`, so reading the COMBINED figure there would copy every imported millilitre into the hand-logged row, and the next import would add the imported water on top of that copy — one tap after an import would silently double the day and compound from there. `log`, `remove` and `set` all read the manual row; only display reads the sum. Both platforms sum water ACROSS sources rather than taking the max the way steps and calories do (#589): a phone and a watch measure the same walk, but a smart bottle and a manual entry are two different real glasses. - iOS: `.dietaryWater` as a cumulative day sum, excluded from NOOP's own samples by the existing `notNoopAuthored` predicate. - Android: `HydrationRecord` over its own 30-day window (the aggregate window is 10 years, which would mean ~3,650 zero-fill rows) plus the READ_HYDRATION manifest declaration, without which Health Connect silently drops the runtime request. - Both gate the write on the hydration toggle, which is opt-in and default OFF, so an import never populates a feature the user turned off. - The detail screen names the imported share instead of letting the day total move on its own, and it is not editable there — it belongs to the app that logged it. Android's "Clear today" now clears only what NOOP owns. Tests: 10 JVM tests over the two pure helpers (the same-day merge that a plain `associate {}` would silently halve, and the zero-fill/idempotency contract). i18n audit and doc-comment lint pass.
Found re-reviewing the PR. Both platforms swallow a per-type read failure — Android's readAll catches and logs, iOS's collect discards the error — which is correct for the aggregate types, because a failed type simply contributes nothing and no row is written for it. Its own comment says so: "a partial type is simply absent, never corrupt." That reasoning does not survive a write that REPLACES rather than adds. For imported water, "read nothing" and "there is nothing" arrive as the same empty result, and the zero-fill then writes 0 for every day in the window — so a transient Health Connect hiccup or a HealthKit query error would erase up to 30 days of imported water and report a clean import. Both readers now say whether they actually completed, and the write is gated on it. readAll returns Boolean and collect returns Bool (@discardableResult, so the accumulating callers are unchanged). Nothing is lost when the read does fail: the health store is the source of truth and the next import restores it. collect also stopped ignoring the error argument — a nil result with an error was previously indistinguishable from an empty day range.
|
Re-review found one real defect in this PR, fixed in 0fd2def. Both platforms swallow a per-type read failure — Android's That reasoning does not survive a write that replaces rather than adds. For imported water, "read nothing" and "there is nothing" arrive as the same empty result, and the zero-fill then writes 0 for every day — so a transient Health Connect hiccup or a HealthKit query error would erase up to 30 days of imported water and report a clean import. Both readers now report whether they actually completed, and the write is gated on it. Nothing is lost when a read does fail: the health store is the source of truth and the next import restores it. Checks still green, including the APK build and the JVM tests. |
…949) Without this the whole feature is dead for everyone who already connected — which is most users. It reads as "you have no water logged", silently, forever. iOS: HealthKit never reports read authorization, and requestAuthorization is only called from the connect button, so a read type added in an update stays notDetermined for anyone who granted access before it existed and its queries just return empty. Store a fingerprint of the read set and re-request when it changes. Quiet by construction: HealthKit presents the sheet only for types still undetermined, so an unchanged set shows no UI and a returning user is asked about exactly the new one. Stored only on success, so a failed request is retried rather than swallowed. Android: the same gap for the same reason. The import gate is `granted.any { ... }` by design (#150 partial grants), so an existing user goes straight to importing and READ_HYDRATION is never requested. Same fix — compare a stored fingerprint of PERMISSIONS and route through the request once when the set grows. Marked before launching so declining is remembered and this never becomes a nag. Found by asking what an existing user would actually see, rather than what a fresh install does.
sync() is also driven by background observer wakes, where no permission sheet can be presented. If such a call reported success without showing anything the signature would be stored and the user would never be asked at all — the exact silent failure the re-request exists to prevent. Gate on the app being active; a background wake simply leaves it for the next foreground sync.
|
Final sweep found one more, and it was the biggest: the feature would have done nothing for existing users. Fixed in 9b00966 + 1882604. HealthKit never reports read authorization, and Both now compare a stored fingerprint of the requested set and ask once when it grows. Quiet by construction — HealthKit only presents the sheet for still-undetermined types, and the Android side marks before launching so declining is remembered rather than nagged. Follow-up on that fix: the iOS re-request is gated on the app being active. Also checked and clear: no widget reads hydration, Unrelated observation while tracing this: |
1 participant
Closes #949 (water half — caffeine is a separate follow-up, see the end).
Drinks logged in a hydration app or by a smart bottle had to be entered into NOOP a second time. Both platforms already read their health store, so this adds water to what they read. Stays on-device: a plain HealthKit / Health Connect read, no network.
The design decision that matters
Imported water lives in its own
metricSeriesrow (hydrationImported) rather than being folded into the hand-logged total, because the two behave differently on write:hydration)hydrationImported)The replacement is what makes re-importing idempotent without tracking individual sample ids — HealthKit's
HKStatisticsCollectionQueryand Health Connect's day bucket both hand back the whole day, so writing it wholesale means a second import of the same day stores the same number. Every day in the window is written, zeros included, so a drink deleted in the source app makes the figure go down here rather than being stranded forever.Why that separation is load-bearing
A quick-add stores
current + amount. Reading the combined figure there would copy every imported millilitre into the hand-logged row, and the next import would then add the imported water on top of that copy — one tap after an import would silently double the day, compounding on every tap after it. Android'sremovewas the sharper version of the same trap: with 200 ml logged and 500 ml imported, removing 100 would have computed 700−100=600 and stored that as the hand-logged total, inflating the day to 1100 instead of reducing it to 600.So
log/remove/setall read the manual row; only display reads the sum.hydrationTotal(display) andhydrationManualTotal(write) are now separate calls on both platforms.Sources are SUMMED, not maxed
Steps and calories take the max across sources (#589) because a phone and a watch measure the same walk. Water is the opposite: a smart bottle and a manual entry are two different real glasses. Taking the max would silently drop whichever app logged less.
Per platform
.dietaryWateras a cumulative day sum. NOOP's own samples are already excluded by the existingnotNoopAuthoredpredicate, so a tap in NOOP can never return as an import. Adding a read type is safe for returning users:refreshAuthIfPreviouslyGrantedresumes offlegacyCoreWriteTypes(write types only), and HealthKit never exposes read status anyway.HydrationRecordover its own 30-day window; the aggregate window is 10 years, which would mean ~3,650 zero-fill rows per import to back a screen showing today plus 7 bars. 30 days matches the iOS sync window exactly. Also declaresREAD_HYDRATION, without which Health Connect silently drops the runtime request (the Fix carried Charge source label #412 trap). Respects partial permissions (Home screen Icons #150) — skipped entirely when the water scope is not granted, rather than zeroing the window.UI
The detail screen names the imported share instead of letting the day total move on its own with nothing on screen accounting for it. It is deliberately not editable there — "deleting" it would be a lie, since the next sync re-reads the same day and it would come straight back. Android's "Clear today" now clears only what NOOP owns, and is hidden when there is nothing of ours to clear.
Verification
HydrationImportTest) over the two pure helpers: the same-day merge (the oldassociate { it.day to it.value }keeps only the last row — with a manual row and an imported row on the same day that silently halves every history bar), and the zero-fill / idempotency / clamping contract. All 10 also executed locally against the shipped function bodies.Rrefs, which are the two new string resources my ad-hoc classpath has no generatedRfor. Both files confirmed analysed by deliberately injecting an error and watching it get reported.Tools/i18n_audit.py --ciexit 0 — new string added to all 6 Android locales and both new Apple literals added to the catalog with 8 translations each.Tools/doc_comment_lint.pyexit 0.Tools/suite 53/53.xcodebuildbefore merging.Noticed while in here, NOT fixed (out of scope)
DistanceRecordis inREAD_RECORDS, soPERMISSIONSrequestsREAD_DISTANCE— butREAD_DISTANCEis never declared in the manifest, which by the same #412 rule means Health Connect silently drops it and distance never imports. Pre-existing and unrelated to hydration; happy to send a one-line PR separately.Follow-up: caffeine
Deliberately not here. NOOP already models caffeine, but
DoseResponseEngineis explicit that the dose is a timing proxy, never mg. So the useful part of a HealthKit caffeine sample is its timestamp mapped onto the existing 0–3 time-of-day bucket — a different change against a different engine, and it should not ride along with a storage change.