Skip to content

Fix review findings in the Grove FHIR migration - #220

Open
lukaskollmer wants to merge 6 commits into
lukas/ios27-frfrom
lukas/grove-fhir-review-fixes
Open

lukaskollmer wants to merge 6 commits into
lukas/ios27-frfrom
lukas/grove-fhir-review-fixes

Conversation

@lukaskollmer

@lukaskollmer lukaskollmer commented Sep 28, 2026 •

Copy link
Copy Markdown
Collaborator

♻️ Current situation & Problem

A review of the Grove FHIR migration stack (#202 → ...) found some problems, which this PR fixes on the app side.

  • A SensorKit sensor could stop uploading permanently. Each strategy published its records in one for … try loop, so a single record that Grove refuses aborted the batch. The batch was then never acknowledged and was delivered again on every run. Separately, when SensorKit did not replay a pending batch exactly (pendingBatchMismatch), neither the fetcher nor logout cleanup could recover, because resetAllQueryAnchors swallowed the error with try?.
  • The FHIR exchange ledger cost O(n²) per HealthKit batch. The ledger is one keychain-encrypted LocalStorage value (Grove's default setting). Every sample decrypted, rewrote and re-encrypted the whole ledger, so large deliveries and month-sized historical-export batches could not finish in a background window.
  • State of Mind samples, and any HeartRisk response with a skipped optional question, lost their HealthKit data. This is fixed in Grove, and this PR takes it in by bumping the pin.

⚙️ Release Notes

  • --studyBundle defaults to .firebase(.stanford) again, instead of .bundledWithApp(.stanford). The loader still substitutes the active study variant and still falls back to the bundled definition when the hosted one fails to load.
  • SensorKit: rework processing so a single failing record doesn't cancel the whole batch (we used to have a for ... try loop, which caused the batch to never get acknowledged/finalized, causing it to get re-run on every launch)
  • SensorKit: a record whose Grove preparation fails is refused and logged. Logs contain only the sensor, ordinal and diagnostic code, no PHI. Its reservation is released and the rest of the batch still gets acknowledged. Upload, account-fence and cancellation errors still abort without acknowledging.
  • SensorKit: on pendingBatchMismatch or retryContentChanged, the fetcher calls SensorKit.discardPendingBatches(for:) and drops that sensor's retry bookkeeping, and the next run fetches the range again under fresh identities. lastSensorKitFetch is only updated when the fetch succeeded. resetAllQueryAnchors now propagates failures, and clearPendingAccountData counts the anchor reset toward success.
  • FHIR exchange ledger: HealthKit deliveries, historical export and retractions reserve a whole batch in one transaction. An exact retry only reads the ledger. Single-key reservations such as SensorKit's use one transaction without an extra read.
  • Grove is pinned to 611d0cd8 and StudyDefinitions to d1cbe285, in the project, MyHeartCountsShared, Package.resolved and the submodule.

Behavior changes to be aware of:

  • Refused SensorKit records are dropped with a log line instead of wedging the sensor.
  • After an abandoned batch, up to one batch per incident can be uploaded twice under new IDs.
  • A sensor that keeps failing leaves lastSensorKitFetch stale, so foreground launches retry it.

Out of scope, per the review triage:

  • Dashboard read-compat for pre-migration LDL, score and Timed Walk documents: dropped. The dashboard has only shipped to TestFlight, so this is a one-time loss.
  • Backend items (MyHeartCounts-Firebase), still open:
    • the LDL writer still uses loinc.org as the quantity system;
    • nothing consumes healthDeletions/;
    • the status patch has no effect on Bundle documents.

📚 Documentation

Doc comments cover the study-bundle default, the refusal rule, and the batch reservation's durability and retry guarantees.

✅ Testing

  • Compiled locally: the app and the MyHeartCounts Unit Tests plan (build-for-testing, generic iOS Simulator, Xcode 27), with packages resolved against the new pins.
  • Not run locally: the app unit tests need a simulator, so CI runs them.
  • SwiftLint strict passes on every changed file.

New tests:

  • StudyBundleLaunchOptionsTests.defaultsToFirebase
  • DeviceUsageFHIRExchangeTests.refusedRecordsDoNotFailTheirBatch and accountFenceAbortsBatchInsteadOfRefusingRecord
  • FHIRExchangeStateTests: three batch-reservation tests (input order, existing keys consume no sequence, stale account generation), healthKitBatchReservesEverySampleUpFront, and abandoningSensorBatchesDropsOnlyThatSourcesRetryState
  • QuestionnaireGroveProjectionTests.bloodPressureRefusesAnIncompleteComponentSet now expects Grove's componentIncomplete.

Not covered by tests:

  • Fetcher-level recovery (discard on mismatch, the per-sensor success flag, the cleanup verdict). It needs a live SensorKit reader and a device.
  • The refusal loops for ECG, wrist temperature, CSV and PPG. Only the device-usage strategy has a test.

Code of Conduct & Contributing Guidelines

By creating and submitting this pull request, you agree to follow our Code of Conduct and Contributing Guidelines:

🤖 Generated with Claude Code

lukaskollmer and others added 5 commits September 28, 2026 17:06
#216 temporarily defaulted `--studyBundle` to the bundle shipped with the app. That default is not DEBUG-gated, so every TestFlight and App Store launch exported the compiled-in study definition and never fetched the hosted one: new study revisions, consent versions and sample-type lists could not reach installed apps.

The default is `.firebase(.stanford)` again. The variant is a placeholder that StudyBundleLoader replaces with the active study variant, and the Firebase path still falls back to the bundled definition when the hosted bundle is missing or can't be decoded, so the Imperial variant keeps working until its bundle is published.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Every HealthKit sample and retracted deletion reserved its event with its
own LocalStorage.modify of the whole encrypted ledger, plus another full
load for its identifier and two keychain reads, while the batch's earlier
reservations stayed in the ledger. A delivery or historical-export batch
therefore cost O(n^2) ECIES work and could not finish in the background.

FHIRExchangeStateStore.events(forKeys:) now reserves a whole batch in one
transaction: existing keys come back unchanged, new keys get consecutive
sequences in input order, and a batch whose keys all exist only reads the
ledger. HealthKitConversionBatch reserves live-delivery, direct-upload and
historical-export batches up front and builds each sample's context from
the returned producer instance and a once-read event scope; retraction
drain chunks do the same via healthKitRetractions(of:). Reservations are
still durable before any upload uses their identity, and stale account
generations are still rejected on both the read and write paths.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A record whose Grove preparation or conversion throws is now logged (without
content), its exchange reservation is released, and the batch is still
acknowledged. Account-fence, retry-ledger, staging and Firestore errors still
abort the batch unacknowledged. This applies to all five upload strategies,
including the single-record CSV and PPG batches.

When a fetch fails with pendingBatchMismatch or retryContentChanged, the
sensor's pending batches are discarded via Grove's discardPendingBatches(for:)
and their retry-only state is dropped, so the next run refetches the range
under fresh coordinates. lastSensorKitFetch is only bumped once every sensor
succeeded.

resetAllQueryAnchors no longer swallows errors: failures of authorized sensors
make clearPendingAccountData keep pendingAccountDataCleanupRequired set.

Requires the Grove revision that adds SensorKit.discardPendingBatches(for:).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
event(key:) went through the batch API, whose read-only probe adds a full
decrypt of the ledger before the write transaction whenever a key is new.
SensorKit reserves records one at a time and most of them are new, so that
made its per-record path slower. Single-key reservations now run one
transaction again; batches keep the probe, which spares an exact retry any
rewrite. Both share one reservation helper.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Grove moves to 611d0cd8 (SchmiedmayerLab/Grove#117), which adds
SensorKit.discardPendingBatches(for:) used by the SensorKit recovery above and
fixes the State of Mind, partial-extraction, breathing-disturbance, ECG timing
and DST issues. StudyDefinitions moves to d1cbe285
(SchmiedmayerLab/MyHeartCounts-StudyDefinitions#52), which only re-pins Grove,
because SwiftPM rejects two different revision requirements for Grove. Pinned
in the project, MyHeartCountsShared, Package.resolved and the submodule.

A half-answered blood-pressure panel still refuses, but Grove now reports it
as componentIncomplete instead of answerMissing.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Sep 28, 2026 •

Copy link
Copy Markdown

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 5ed40784-4aa3-4e12-b906-1f5ef74aaaf5


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@lukaskollmer
lukaskollmer added this pull request to stack #217 September 28, 2026 15:59
Xcode 26.6, which CI uses, could not type-check the `Date + Double * 60 + 30`
arithmetic inside the untyped `map` closure in reasonable time and failed the
test build. Explicit closure types and addingTimeInterval keep the same
samples.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

This branch has not been deployed

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

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants