fix(mobile): start HealthKit observer sync immediately

[Asherlc]

Summary

• start each HealthKit observer sync immediately instead of waiting on a JavaScript debounce timer
• retain single-flight serialization so updates arriving during a sync run in the next batch
• document the production evidence and the successfully deployed Peloton incident fix

Root cause

The native 25-second observer deadline kept advancing while a background JavaScript setTimeout(500) did not execute for about 30 seconds. Removing that timer from the correctness path starts work in the observer callback while preserving the existing serialized queue.

Validation

• regression test fails on the old timer-gated implementation and passes on this change
• pnpm exec vitest run --project mobile packages/mobile/lib/background-health-kit-sync.test.ts (31/31)
• pnpm --filter dofek-mobile typecheck
• targeted Biome check
• git diff --check
• full mobile suite previously passed (1,019 tests) before the merge-forward; focused tests and typecheck were rerun afterward

Fixes DOFEK-MOBILE-1C

---

Summary by cubic

Start HealthKit observer sync immediately on native callbacks, track the active SyncContext, and reliably release the sync guard so the queue never stalls. Prevents background deliveries from missing the 25s deadline while keeping single-flight serialization; fixes DOFEK-MOBILE-1C.

• Bug Fixes
  • Removed the JS debounce and start sync right away in background-health-kit-sync.ts (log: “queueing sync”).
  • Added active SyncContext tracking; only acknowledge updates and run onSyncComplete for the current session, and clear context on teardown.
  • Wrapped sync in a try/finally to always release the guard and acknowledge failures; tests updated for immediate start, per-update completion, and context handoff; README and production incident baseline refreshed.

^{Written for commit 0e42a8d. Summary will update on new commits.}

[cursor]

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

[codereviewbot-ai]

Overall, the refactoring cleanly eliminates background timers and debouncing in favor of immediate queueing and serialized background syncs.

I left one inline recommendation in background-health-kit-sync.ts to wrap the syncing state resetting in a try...finally block to prevent the sync queue from becoming permanently stuck if an unhandled error occurs.

---

🤖 Reviewed by codereviewbot.ai - Catch bugs before your team does.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[coderabbitai]

Warning

Review limit reached

@Asherlc, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 9 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 00fd12e4-8e3b-49a0-bf0f-3e4b3210d981

📥 Commits

Reviewing files that changed from the base of the PR and between 7f28ff7 and 0e42a8d.

📒 Files selected for processing (5)

• docs/production-incident-baseline.md
• packages/mobile/lib/background-health-kit-sync.test.ts
• packages/mobile/lib/background-health-kit-sync.ts
• packages/mobile/modules/health-kit/README.md
• packages/mobile/modules/health-kit/index.ts

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[sourcery-ai]

Sorry @Asherlc, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

[qodo-code-review]

PR Summary by Qodo

Fix iOS background HealthKit observer sync to start immediately

🐞 Bug fix 🧪 Tests 📝 Documentation 🕐 40+ Minutes

AI Description

• Start HealthKit observer sync work immediately in the native callback path.
• Keep single-flight serialization so updates during a run execute in the next batch.
• Add regression coverage and document incident evidence/mitigation for DOFEK-MOBILE-1C.

Diagram

graph TD
  HK["HealthKit observer callback"] -->|"event (updateId)"| JS["JS listener"] -->|"add to Set"| Q["pendingUpdateIds"] -->|"trigger"| D["drainSyncQueue"] -->|"single-flight"| S["HealthKit sync"] -->|"tRPC"| API["Server"]
  S -->|"ack updateIds"| N["HealthKitModule (iOS)"] -->|"complete callbacks"| HK

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Keep debounce but move it to native (HKObserverQuery side)

• ➕ Avoids reliance on background JS timers while still allowing coalescing
• ➕ Can coalesce multiple observer updates before waking JS work
• ➖ More complex native implementation and testing surface
• ➖ Harder to iterate/deploy compared to JS-only fix (requires app release)

2. Replace setTimeout debounce with microtask/idle scheduling (Promise queue / InteractionManager)

• ➕ Still avoids long setTimeout delays in background
• ➕ Can preserve some batching behavior without explicit timers
• ➖ Scheduling guarantees still vary in background/locked-device states
• ➖ Adds indirection without addressing the core requirement: start work immediately when invoked

Recommendation: The chosen approach (remove the debounce timer from the correctness path and immediately drain an existing single-flight queue) is the most robust and lowest-risk fix for observer deadlines. It preserves serialization and ensures acknowledgements only occur after processing completes, while avoiding background timer starvation that caused DOFEK-MOBILE-1C. Native-side batching could be revisited later only if there is a proven need to reduce sync frequency.

Files changed (5) +89 / -49

Bug fix (1) +4 / -23

background-health-kit-sync.tsRemove observer debounce timer and drain sync queue immediately on updates +4/-23

Remove observer debounce timer and drain sync queue immediately on updates

• Eliminates the 500ms debounce gating and the observer-ready state, triggering drainSyncQueue directly from the observer callback and only running when pending update IDs (or catch-up) exist. Keeps single-flight sync behavior and acknowledges each update ID batch only after the sync settles.

packages/mobile/lib/background-health-kit-sync.ts

Tests (1) +29 / -15

background-health-kit-sync.test.tsAdd regression for immediate observer sync and adjust queue/ack expectations +29/-15

Add regression for immediate observer sync and adjust queue/ack expectations

• Introduces a test ensuring observer sync starts immediately without advancing timers, and updates existing tests to reflect serialized (not coalesced) acknowledgements and immediate queuing during an active sync.

packages/mobile/lib/background-health-kit-sync.test.ts

Documentation (3) +56 / -11

production-incident-baseline.mdDocument HealthKit observer timer starvation incident and resolution evidence +49/-6

Document HealthKit observer timer starvation incident and resolution evidence

• Updates the Peloton incident entry to reflect deployment and adds a new incident write-up for DOFEK-MOBILE-1C, including production evidence, root cause, mitigation, and validation steps.

docs/production-incident-baseline.md

README.mdUpdate observer completion docs to match serialized queue behavior +6/-4

Update observer completion docs to match serialized queue behavior

• Rewrites the Background Observer Completion section to describe immediate queueing into a single-flight sync and how updates received during a running sync are handled.

packages/mobile/modules/health-kit/README.md

index.tsClarify observer completion comment (serialized vs coalesced) +1/-1

Clarify observer completion comment (serialized vs coalesced)

• Adjusts the exported API comment to reflect that observer callbacks are completed after a serialized sync settles rather than a coalesced debounce batch.

packages/mobile/modules/health-kit/index.ts

[codereviewbot-ai]

LGTM!

The PR cleanly removes the background HealthKit sync debounce logic in favor of serialized execution (drainSyncQueue), addressing the previous review feedback by wrapping the sync execution in a try...finally block to ensure syncing state is safely reset. Tests have also been updated accordingly.

---

🤖 Reviewed by codereviewbot.ai - Catch bugs before your team does.

[github-actions]

Mobile Preview

Scan to open on device:

Channel	pr-2049
Deep Link	dofek://preview/pr-2049
Commit	fba603e

To test on device:

1. Build and install the preview client: PREVIEW_CHANNEL=pr-2049 pnpm expo prebuild --clean -p ios
2. Or tap deep link on an existing preview build: dofek://preview/pr-2049

Each PR gets its own channel. Build a preview client with PREVIEW_CHANNEL=pr-{N} to test.

[github-actions]

Storybook previews for fba603e9 are ready:

• Web Storybook
• Mobile Storybook

This comment updates automatically on each PR push.

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (1) 📜 Skill insights (0)

Context used

✅ Compliance rules (platform): 176 rules

✅ Skills: fix-provider, write-tests, cloudflare

1. Stale client after re-init ✓ Resolved 🐞 Bug ⛨ Security

Description

An in-flight drainSyncQueue continues recursion with the original trpcClient/onSyncComplete
arguments even after teardown/re-init, so observer updates queued during/after the new init can be
synced using a stale client context. This can cause HealthKit uploads or post-sync invalidations to
run under the wrong session/client (or fail) until another drain call using the new client context
runs.

Code

packages/mobile/lib/background-health-kit-sync.ts[R147-158]

  syncing = true;

Relevance

⭐⭐ Medium

No closely analogous precedent on stale client after re-init; could be real but more
architectural/racey.

PR-#1394
PR-#1526

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

drainSyncQueue() always recurses with its original arguments (so it can continue draining after a
re-init using stale parameters), while initBackgroundHealthKitSync()/app wiring explicitly
supports teardown + re-init with a new SyncTrpcClient (new wrapper object) on auth/client changes.

packages/mobile/lib/background-health-kit-sync.ts[127-158]
packages/mobile/lib/background-health-kit-sync.ts[167-206]
packages/mobile/app/_layout.tsx[197-224]
packages/mobile/app/_layout.tsx[297-304]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`drainSyncQueue()` keeps draining work by recursively calling itself with the *original* `trpcClient` / `onSyncComplete` arguments. When `initBackgroundHealthKitSync()` is called again (it calls `teardownBackgroundHealthKitSync()` and installs a new listener/client), any already-running drain from the prior init is not cancelled and can later process newly queued update IDs using stale client/callback state.

This is especially risky across auth transitions (logout/login) or when the underlying tRPC client instance changes.

## Issue Context
- `initBackgroundHealthKitSync()` can be called multiple times and explicitly tears down/re-inits when an existing subscription exists.
- App code constructs a new `SyncTrpcClient` in a React effect, and calls teardown on effect cleanup.
- There is no cancellation / generation check for the currently-running drain, and recursion preserves the original args.

## Fix Focus Areas
- packages/mobile/lib/background-health-kit-sync.ts[127-158]
- packages/mobile/lib/background-health-kit-sync.ts[167-206]
- packages/mobile/lib/background-health-kit-sync.ts[208-225]
- packages/mobile/app/_layout.tsx[197-224]
- packages/mobile/app/_layout.tsx[297-304]

## Suggested fix approach
- Introduce a module-level `syncGeneration` (number) that increments on each init and teardown.
- Capture the generation at the start of `drainSyncQueue()` and check it:
 - Before starting a sync
 - After `await performHealthKitSync(...)`
 - Before recursing / continuing
 If the generation no longer matches, stop draining (do not process `pendingUpdateIds` with stale context).
- Store the “current” `trpcClient` and `onSyncComplete` in module-level variables updated on init, and have draining always read from those variables (rather than passing them through recursion). This prevents stale argument capture.
- Ensure any `void drainSyncQueue(...)` calls are made against the current generation/context.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Observer-start test not isolated ✓ Resolved 🐞 Bug ☼ Reliability

Description

The new regression test asserts "Starting sync" immediately after invoking the observer listener,
but it does not first deterministically wait for the init catch-up sync to finish; the same
"Starting sync" log can be emitted by that catch-up sync and satisfy the assertion. This makes the
test vulnerable to false positives/negatives depending on when the catch-up promise chain completes
relative to mock clearing and the listener invocation.

Code

packages/mobile/lib/background-health-kit-sync.test.ts[R577-592]

+  it("starts observer sync without waiting for a background timer (DOFEK-MOBILE-1C)", async () => {
+    vi.useFakeTimers();
+    const client = createMockClient();
+    await initBackgroundHealthKitSync(client);
+    await vi.runAllTimersAsync();
+    mockLoggerInfo.mockClear();
+
+    const listener = mockAddSampleUpdateListener.mock.calls[0][0];
+    listener({
+      typeIdentifier: "HKQuantityTypeIdentifierStepCount",
+      updateId: "update-1",
+    });
+
+    expect(mockLoggerInfo).toHaveBeenCalledWith("bg-healthkit-sync", "Starting sync");
+    vi.useRealTimers();
+  });

Relevance

⭐⭐⭐ High

Matching flakiness warning (async catch-up + mockClear/log asserts) was accepted previously.

PR-#1971

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The implementation always kicks off an async catch-up sync that logs "Starting sync" and is not
awaited by init; the new test clears and then asserts against the shared log message without first
awaiting catch-up completion, so it isn’t guaranteed to be observing the listener-triggered path.

packages/mobile/lib/background-health-kit-sync.test.ts[577-592]
packages/mobile/lib/background-health-kit-sync.ts[69-75]
packages/mobile/lib/background-health-kit-sync.ts[201-205]
PR-#1971

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The test `starts observer sync without waiting for a background timer (DOFEK-MOBILE-1C)` clears `mockLoggerInfo` after init, but does not synchronously/explicitly await completion of the init-triggered catch-up sync. Because both catch-up and observer-triggered syncs log the same message (`"Starting sync"`), the assertion can be satisfied by the catch-up sync rather than the observer callback.

## Issue Context
`initBackgroundHealthKitSync()` always schedules a catch-up drain (`pendingCatchUp = ...; void drainSyncQueue(...)`) and `performHealthKitSync()` always logs `"Starting sync"`. The test currently uses timer flushing (`runAllTimersAsync`) but does not tie its assertion to an observer-specific effect.

## Fix Focus Areas
- packages/mobile/lib/background-health-kit-sync.test.ts[577-592]
- packages/mobile/lib/background-health-kit-sync.ts[63-113]
- packages/mobile/lib/background-health-kit-sync.ts[201-205]

## Suggested fix approach
- Before clearing mocks / triggering the observer listener, *deterministically* wait for the init catch-up sync to finish (e.g., `await vi.waitFor(() => expect(client.healthKitSync.pushWorkouts.mutate).toHaveBeenCalledTimes(1))` or wait for an `"Observer processing complete"` log).
- Then capture the current `mockLoggerInfo.mock.calls.length` (or count of `"Starting sync"` calls), invoke the listener, and `await vi.waitFor(...)` that the count increases by 1.
- Alternatively, assert that the `"Sample update event received, queueing sync"` log is emitted and then that the subsequent `"Starting sync"` call happens *after* it (by comparing call order / indices).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. README still says coalesced ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

The HealthKit README now documents immediate enqueueing into a serialized single-flight queue, but
the physical-device validation checklist still instructs validating completion after a “coalesced
sync,” which no longer matches the described behavior. This can mislead on-call/validation
expectations for observer completion timing and batching.

Code

packages/mobile/modules/health-kit/README.md[R31-36]

+retains HealthKit's completion callback while JavaScript immediately places the
+delivery in a single-flight sync queue and reports the batch result through
+`completeObserverUpdates`. Updates delivered during a running sync remain
+pending for the next serialized sync. Re-registration, JavaScript teardown, and
+Expo module destruction stop the queries and complete every callback still
+pending.

Relevance

⭐⭐⭐ High

Doc wording alignment is typically accepted; low-risk maintainability change.

PR-#1124
PR-#1881

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The README’s Background Observer Completion section now describes immediate queueing and serialized
syncs, but the later checklist still tells readers to expect update IDs to complete after a
coalesced sync, which conflicts with the updated description.

packages/mobile/modules/health-kit/README.md[28-36]
packages/mobile/modules/health-kit/README.md[76-89]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Documentation was updated to describe a serialized sync queue, but the physical-device validation checklist still references a "coalesced sync" when describing when update IDs should be completed.

## Issue Context
Engineers may use this checklist during incident response; mismatched terminology can lead to incorrect conclusions about expected observer batching/ack timing.

## Fix Focus Areas
- packages/mobile/modules/health-kit/README.md[28-36]
- packages/mobile/modules/health-kit/README.md[76-89]

## Suggested fix approach
- Replace "coalesced sync" in step 4 with wording consistent with the new model, e.g.:
 - "...completed exactly once after the serialized sync queue drains" or
 - "...after the queued serialized sync(s) settle" (to cover updates delivered during an active sync).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

4. HealthKit sync logic in TS 📘 Rule violation ⌂ Architecture

Description

background-health-kit-sync.ts implements HealthKit observer state/queueing and sync orchestration
in TypeScript rather than keeping TS as a thin bridge to Swift. This violates the requirement that
BLE/HealthKit domain logic reside in Swift, and risks correctness in background execution (the exact
class of failure this PR is addressing).

Code

packages/mobile/lib/background-health-kit-sync.ts[R186-188]

+    logger.info(TAG, "Sample update event received, queueing sync");
    pendingUpdateIds.add(event.updateId);
-    scheduleObserverSync(trpcClient, onSyncComplete);
+    void drainSyncQueue(trpcClient, onSyncComplete);

Relevance

⭐ Low

Analogous “move TS orchestration to Swift” compliance suggestion was rejected in WHOOP BLE context.

PR-#2026

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 721993 requires BLE/HealthKit domain logic (state machines/long-lived
state/retry/behavior decisions) to live in Swift and not in TypeScript. The modified observer
callback now queues updateIds and immediately drains a serialized sync queue from TS
(pendingUpdateIds.add(...) + drainSyncQueue(...)), and the queue/state machine itself is
implemented in TS (pendingUpdateIds, syncing, drainSyncQueue).

Rule 721993: BLE and HealthKit domain logic must reside in Swift (TypeScript is bridge-only)
packages/mobile/lib/background-health-kit-sync.ts[15-24]
packages/mobile/lib/background-health-kit-sync.ts[127-158]
packages/mobile/lib/background-health-kit-sync.ts[183-189]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The HealthKit observer sync orchestration (queueing `updateId`s, single-flight serialization, and triggering sync runs) is implemented in TypeScript, but the compliance checklist requires HealthKit domain logic to live in Swift with TS acting as a bridge-only layer.

## Issue Context
The PR changes the observer flow to immediately queue and drain sync work from the JS callback. While it fixes a debounce/timer issue, it further entrenches HealthKit domain state management in JS.

## Fix Focus Areas
- packages/mobile/lib/background-health-kit-sync.ts[127-158]
- packages/mobile/lib/background-health-kit-sync.ts[167-206]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[codereviewbot-ai]

🤖 Review skipped: Repository rate limit exceeded. Free accounts are limited to 2 reviews per 4 hours per repository. Upgrade to a paid plan for unlimited reviews.