Skip to content

Caffeine: import intakes from Apple Health (#949) - #975

Closed
ryanbr wants to merge 2 commits into
hydration-health-import-949from
caffeine-health-import-949
Closed

Caffeine: import intakes from Apple Health (#949)#975
ryanbr wants to merge 2 commits into
hydration-health-import-949from
caffeine-health-import-949

Conversation

@ryanbr

@ryanbr ryanbr commented Jul 30, 2026

Copy link
Copy Markdown
Owner

The caffeine half of #949. Stacked on #974 (both touch HealthKitBridge); base is hydration-health-import-949, so review that one first and this retargets to main automatically when it lands.

The caffeine window (#526) already stores exactly what a HealthKit caffeine sample carries — a timestamp and an optional mg — so a coffee logged elsewhere can feed the decay estimate instead of being typed in twice.

A sample query, not a day sum

Water uses HKStatisticsCollectionQuery day buckets. Caffeine cannot: the estimate decays each intake from its own time, so a day total would collapse a 7am coffee and a 9pm one into a single number that answers no question the card asks. This reads individual samples with their timestamps.

Correcting something I wrote on the issue: I said the mg figure "would not feed anything today". That was wrong — it feeds #526's half-life estimate. What is timing-only is DoseResponseEngine, which is a different feature. Both are true; I conflated them.

Dedup is harder here than for water

Water is one day total that can simply be replaced. Caffeine is a list of individual events. So an imported intake carries its HealthKit sample UUID in a new optional externalId, and a sync replaces the imported subset wholesale, leaving hand-logged intakes untouched.

That gives the same two properties the water PR gets from replacing a row:

  • re-importing is idempotent — it does not log a second copy of every coffee;
  • an intake deleted in the source app disappears here on the next sync rather than being stranded.

externalId is optional, so JSON written before this change still decodes — and every existing intake correctly reads back as hand-logged, not as an import a later sync would feel entitled to delete. There is a test for exactly that.

Imported intakes are not the user's to edit

remove and clearAll skip them and the UI offers no control, because the next sync re-reads the same window and would bring the entry straight back — offering the button would be offering something NOOP cannot honour. The row is labelled Apple Health instead. Remove it where it was logged.

One shared store

CaffeineLogStore gains a shared instance and the card observes it rather than owning its own. Two instances over one UserDefaults key would leave the card publishing its stale in-memory array while the imported intakes sat on disk, invisible. This follows LiquidPowerMonitor exactly — same @MainActor final class … ObservableObject + static let shared + @ObservedObject shape already used across the Liquid views.

Android is deliberately NOT matched

Health Connect has no caffeine-only scope. Caffeine is a field on NutritionRecord, gated by READ_NUTRITION — which grants the user's entire food log: every meal, every macro. Apple exposes .dietaryCaffeine as its own narrow type. Asking every Android user for their whole nutrition history to read one number is not a trade worth making quietly in a privacy-focused app, so the manifest is untouched and the gap is documented rather than papered over. Worth a separate discussion if you want it later; it should be an explicit decision, not a side effect of this PR.

(The water half of #949 is matched on both platforms — Health Connect has a narrow READ_HYDRATION.)

Verification — read this part

  • 15 tests in StrandTests/CaffeineImportTests.swift: idempotency, the vanished-sample case, hand-logged intakes surviving a sync, the backward-compatible decode, the not-editable rules, and a persistence round-trip.
  • No CI runs them. app-build.yml is disabled and StrandTests executes nowhere, so these tests are documentation until someone runs xcodebuild … test locally. I am not claiming they pass — I am claiming they are written and that I could not execute them.
  • All six changed Swift files pass swiftc -parse. That catches structural errors only; it is not a type-check, since SwiftUI/HealthKit are unavailable on this Linux host.
  • Tools/i18n_audit.py --ci exit 0, Tools/doc_comment_lint.py exit 0.
  • Not device-tested: no iPhone here, and the actual HealthKit read path can only be proven on hardware with real caffeine samples.

The honest summary: the logic is tested by construction and unexecuted, and the app target is uncompiled. Worth a local build before merging.

@ryanbr

ryanbr commented Jul 30, 2026

Copy link
Copy Markdown
Owner Author

Re-review found two real defects in this PR, fixed in 87cc98c.

1. Imported intakes got a new identity on every sync. CaffeineIntake.id defaults to UUID(), and the bridge rebuilds each intake from the HealthKit samples every pass — so the same coffee was a different row each time. That defeated replaceImported's "nothing changed" guard (a pointless JSON rewrite and republish every sync) and handed ForEach a wholly new list to rebuild. The sample's own uuid is stable, so it is now the id.

Worth noting: my own test could not have caught this. It reused a single CaffeineIntake value for both imports, so the ids matched trivially. It now builds a fresh intake per import the way the bridge does, plus an explicit equality test.

2. A failed caffeine query deleted every imported intake. collectCaffeine discarded the query error, so a failure returned an empty array and replaceImported cleared the set — the same bug as the hydration one in #974, and worse here because replacing the set is the function's whole job. It returns nil on failure now; only a genuine empty result clears anything.

Still no CI on this branch (it targets the water branch, and the workflows only trigger on PRs into main), and StrandTests runs nowhere regardless.

ryanbr added 2 commits July 30, 2026 12:48
The caffeine window (#526) already stores exactly what a HealthKit caffeine
sample carries — a timestamp and an optional mg — so a coffee logged in
another app can feed the decay estimate instead of being typed in twice.

A SAMPLE query, not the day-bucketed one water uses: the estimate decays each
intake from its own time, so a day total would collapse a 7am coffee and a 9pm
one into a number that answers no question the card asks.

Dedup is harder than water's. Water is one day total that can simply be
replaced; caffeine is a list of individual events. So an imported intake
carries its HealthKit sample UUID in a new optional `externalId`, and a sync
replaces the imported SUBSET wholesale, leaving hand-logged intakes alone.
That keeps a re-import idempotent instead of logging a second copy of every
coffee, and makes an intake deleted in the source app disappear here too.
The field is optional so existing stored JSON still decodes, with every
existing intake correctly reading back as hand-logged.

Imported intakes are not editable here: remove/clearAll skip them and the UI
offers no control, because the next sync would re-read the same window and
bring the entry straight back. Remove it where it was logged.

The store gains a `shared` instance — the card used to own its own, which was
fine while every write came from the card, but two instances over one
UserDefaults key would leave the card publishing a stale array while the
imported intakes sat invisible on disk. Follows the LiquidPowerMonitor
pattern already used for @mainactor ObservableObject singletons.

ANDROID IS DELIBERATELY NOT MATCHED. Health Connect has no caffeine-only
scope: caffeine is a field on NutritionRecord behind READ_NUTRITION, which
grants the user's entire food log. Apple exposes .dietaryCaffeine as its own
narrow type. Asking every Android user for their whole nutrition history to
read one number is not a trade this app should make quietly, so the manifest
is untouched and the gap is documented instead.

Tests: 15 in StrandTests covering idempotency, the vanished-sample case, the
backward-compatible decode, and the not-editable rules. No CI runs them —
app-build.yml is disabled and StrandTests executes nowhere.
…ead error

Two defects found re-reviewing the PR.

The intake id was freshly minted on every sync. CaffeineIntake.id defaults to
UUID(), and the bridge rebuilds every intake from the HealthKit samples each
time, so the same coffee got a new identity on every pass. That defeated
replaceImported's "nothing changed" guard (a pointless JSON rewrite and
republish on every sync) and gave ForEach an entirely new set of rows to
rebuild each time. The sample's own uuid is stable, so use it.

My own test could not have caught this: it reused one CaffeineIntake value for
both imports, so the ids matched trivially. It now builds a fresh intake per
import, the way the bridge does, plus an explicit test that two intakes built
from one sample are equal.

Second, collectCaffeine discarded the query error, so a failed read returned
an empty array and replaceImported deleted every imported intake — the same
class of bug as the hydration one in the previous commit, and worse here
because the caller's whole job is to replace the set. It now returns nil on
failure, and only a genuine empty result clears anything.
@ryanbr
ryanbr force-pushed the caffeine-health-import-949 branch from 87cc98c to 7e0ba0f Compare July 30, 2026 19:48
@ryanbr
ryanbr deleted the branch hydration-health-import-949 July 30, 2026 20:06
@ryanbr ryanbr closed this Jul 30, 2026
@ryanbr

ryanbr commented Jul 30, 2026

Copy link
Copy Markdown
Owner Author

Superseded by #985 — same branch and commits, retargeted to `main`. GitHub closed this automatically when #974 merged and its base branch was deleted; a closed PR cannot be retargeted or reopened, so it had to be re-raised.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant