Skip to content

fix(mobile): defer background WHOOP init - #2026

Merged
Asherlc merged 13 commits into
mainfrom
Asherlc/use-subagents
Jul 26, 2026
Merged

Asherlc merged 13 commits into
mainfrom
Asherlc/use-subagents

Conversation

@Asherlc

@Asherlc Asherlc commented Jul 26, 2026

Copy link
Copy Markdown
Owner

Summary

  • defer WHOOP foreground BLE initialization when the app is already backgrounded
  • use the existing active-state transition to start the deferred sync
  • keep the explicit background-refresh sync path unchanged
  • document the DOFEK-MOBILE-1D incident and follow-up

Root cause

initBackgroundWhoopBleSync unconditionally started its foreground BLE path even when AppState.currentState was not active. The Sentry event came from whoop-ble-init-sync with app.in_foreground:false, producing a native DISCONNECTED error.

Validation

  • pnpm exec vitest run packages/mobile/lib/background-whoop-ble-sync.test.ts --project mobile (47 passed)
  • pnpm lint
  • pnpm typecheck
  • pnpm test (13,742 passed, 21 skipped)

Summary by cubic

Defers WHOOP BLE init when the app starts in the background, serializes the first sync, and cancels sync if the app backgrounds mid-flow; adds tests that await IMU stream startup. Prevents background BLE attempts, duplicate runs, and dangling connections; the explicit background-refresh path stays unchanged.

  • Bug Fixes
    • Run the initial BLE connect/IMU start only when the app is active; otherwise wait for the next active transition.
    • Serialize the initial sync so a foreground transition cannot start a second run.
    • Stop syncing if the app backgrounds while searching, connecting, or starting streaming; disconnect and resume on next foreground.

Written for commit 8177587. Summary will update on new commits.

Review in cubic

Summary by CodeRabbit

  • Bug Fixes

    • Improved WHOOP Bluetooth initialization when the app starts in the background.
    • Prevented premature Bluetooth connections and IMU streaming during background startup.
    • WHOOP synchronization now begins when the app returns to the foreground.
    • Preserved immediate synchronization when the app is already active.
  • Documentation

    • Added an incident-baseline entry documenting the behavior, mitigation, validation, and remaining rollout risk.

Copilot AI review requested due to automatic review settings July 26, 2026 08:03
@cursor

cursor Bot commented Jul 26, 2026

Copy link
Copy Markdown

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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Please try again later or upgrade to continue using Sourcery

@codereviewbot-ai

codereviewbot-ai Bot commented Jul 26, 2026

Copy link
Copy Markdown

The PR cleanly defers WHOOP BLE connection during initialization when the app is backgrounded, preventing BLE operations while suspended, and correctly connects once the app transitions to active. Test coverage for the background init case is included and verified. Left one minor nit regarding setting the syncing flag during initial sync.


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

@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Fix mobile: defer WHOOP BLE init until app is foregrounded

🐞 Bug fix 🧪 Tests 📝 Documentation 🕐 40+ Minutes

Grey Divider

AI Description

• Prevent WHOOP foreground BLE initialization from running while the app is backgrounded.
• Reuse the existing AppState active-transition hook to trigger the deferred initial sync.
• Add regression coverage and document the DOFEK-MOBILE-1D incident timeline and fix.
Diagram

graph TD
A["Auth/settings enable"] --> B["initBackgroundWhoopBleSync"] --> C{"AppState active?"}
C -->|"Yes"| D["syncOnForeground"] --> E["WHOOP BLE deps/device"]
C -->|"No"| F["Defer until AppState 'active'"] --> D
B --> G["Background refresh sync"]
subgraph Legend
  direction LR
  _fn["Function/module"] ~~~ _dec{"Decision"} ~~~ _ext["External device"]
end
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Enforce AppState gating inside syncOnForeground
  • ➕ Centralizes the safety check so all callers are protected
  • ➕ Reduces the risk of future call sites reintroducing the issue
  • ➖ May blur responsibilities if syncOnForeground is intended to be 'foreground-only' by contract
  • ➖ Could mask incorrect call ordering rather than making it explicit at the init boundary
2. Trigger an immediate synthetic AppState callback on subscription
  • ➕ Avoids a separate 'initial sync' branch by normalizing everything through the listener
  • ➕ Makes behavior uniform whether init runs active or background
  • ➖ Harder to reason about ordering with multiple listeners and teardown
  • ➖ Risk of subtle differences vs platform AppState semantics (especially across RN versions)

Recommendation: The PR’s approach is a good fit: it keeps the foreground BLE work explicitly gated at initialization time and reuses the existing active-transition pathway for deferred startup, while leaving the explicit background-refresh path untouched. If future callers start invoking syncOnForeground outside this init, consider adding a defensive AppState precondition inside syncOnForeground as a follow-up hardening step.

Files changed (3) +65 / -12

Bug fix (1) +21 / -12
background-whoop-ble-sync.tsGate initial foreground WHOOP BLE sync on active AppState +21/-12

Gate initial foreground WHOOP BLE sync on active AppState

• Updates the initializer to run the initial sync immediately only when the app is active; otherwise logs and defers until the next active transition, preventing BLE connection attempts while suspended. Updates the function docstring to reflect the foreground-only behavior.

packages/mobile/lib/background-whoop-ble-sync.ts

Tests (1) +18 / -0
background-whoop-ble-sync.test.tsAdd regression test for deferred BLE connect when backgrounded +18/-0

Add regression test for deferred BLE connect when backgrounded

• Introduces a test that initializes the sync while AppState is backgrounded and asserts no BLE connection occurs until the app transitions back to active, then verifies connect/streaming starts.

packages/mobile/lib/background-whoop-ble-sync.test.ts

Documentation (1) +26 / -0
production-incident-baseline.mdDocument DOFEK-MOBILE-1D WHOOP BLE init-in-background incident +26/-0

Document DOFEK-MOBILE-1D WHOOP BLE init-in-background incident

• Adds an incident entry describing the Sentry signal, root cause (foreground BLE init running while backgrounded), mitigation, validation, and rollout follow-ups.

docs/production-incident-baseline.md

Comment thread packages/mobile/lib/background-whoop-ble-sync.ts
@github-actions

github-actions Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Mobile Preview

Scan to open on device:

QR code for dofek://preview/pr-2026

Channel pr-2026
Deep Link dofek://preview/pr-2026
Commit 44e33b9

To test on device:

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

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

@coderabbitai

coderabbitai Bot commented Jul 26, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

WHOOP BLE initialization now checks AppState.currentState, immediately syncing only while active and deferring connection and IMU streaming until foregrounding otherwise. Tests cover the background-to-active transition, and the incident baseline documents the behavior and mitigation.

Changes

WHOOP BLE lifecycle

Layer / File(s) Summary
Foreground-gated BLE initialization
packages/mobile/lib/background-whoop-ble-sync.ts
Initialization runs syncOnForeground immediately only when the app is active; otherwise it logs deferral and waits for the next foreground event.
Lifecycle validation and incident record
packages/mobile/lib/background-whoop-ble-sync.test.ts, docs/production-incident-baseline.md
Tests verify that background initialization avoids connection and IMU streaming until activation, while the incident baseline records the root cause, fix, and validation status.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested labels: area/mobile, type/bug

Suggested reviewers: copilot

🚥 Pre-merge checks | ✅ 1 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Title check ⚠️ Warning Title is relevant, but it violates the required format by using fix(mobile): instead of a bracketed area prefix. Change it to an imperative title with a bracketed area prefix, e.g. [mobile] defer background WHOOP init.
✅ Passed checks (1 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

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.

@github-actions

github-actions Bot commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Storybook previews for 44e33b9a are ready:

This comment updates automatically on each PR push.

@qodo-code-review

qodo-code-review Bot commented Jul 26, 2026

Copy link
Copy Markdown

Code Review by Qodo

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

Context used
✅ Compliance rules (platform): 167 rules

Grey Divider


Remediation recommended

1. Connect not foreground-guarded ✓ Resolved 🐞 Bug ☼ Reliability
Description
initBackgroundWhoopBleSync now defers the initial sync when not active, but syncOnForeground can
still run findWhoop()/connect()/startImuStreaming() after the app backgrounds mid-sync because
shouldContinueUploading() is only checked inside the upload loops. This leaves a race where a
foreground-triggered sync can still initiate BLE work while suspended, leading to intermittent
native disconnect/init failures and unnecessary power use.
Code

packages/mobile/lib/background-whoop-ble-sync.ts[R163-183]

+  // The AppState listener only fires on state transitions, so sync immediately
+  // when init runs in the foreground. Defer a backgrounded initialization until
+  // the next active transition so this foreground sync does not try to start a
+  // BLE connection while the app is suspended.
+  if (shouldRunForegroundPeriodicDrain()) {
+    logger.info(LOG_CATEGORY, "initializing background sync");
+    try {
+      await syncOnForeground(
+        trpcClient,
+        whoopDeps,
+        realtimeClient,
+        shouldRunForegroundPeriodicDrain,
+      );
+      logger.info(LOG_CATEGORY, "initial sync complete");
+    } catch (error: unknown) {
+      logger.error(LOG_CATEGORY, `initial sync error: ${error}`);
+      captureException(error, { source: "whoop-ble-init-sync" });
+    }
+  } else {
+    logger.info(LOG_CATEGORY, "initial sync deferred until app foregrounds");
  }
Relevance

⭐⭐⭐ High

Team has accepted race/foreground-guard fixes around AppState-triggered sync; likely to add
additional guards/checks.

PR-#1526
PR-#1401
PR-#1404

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The new init logic defers the initial sync when not active, with explicit intent to avoid starting
BLE while suspended, but the foreground predicate is only enforced during buffer upload. The BLE
connection/streaming setup path in syncOnForeground has no shouldContinueUploading() checks, so
a background transition during an in-flight sync can still execute BLE setup steps while no longer
active.

packages/mobile/lib/background-whoop-ble-sync.ts[163-183]
packages/mobile/lib/background-whoop-ble-sync.ts[254-327]
packages/mobile/lib/background-whoop-ble-sync.ts[381-387]
packages/mobile/lib/background-whoop-ble-sync.ts[434-438]

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 PR defers the *start* of the initial foreground sync based on `AppState.currentState`, but once `syncOnForeground(...)` begins, it does not re-check foreground state before/after awaited BLE setup steps (`findWhoop`, `connect`, `startImuStreaming`). As a result, a background transition during an in-flight sync can still cause BLE work to run while the app is suspended.

### Issue Context
`shouldContinueUploading` is passed to `syncOnForeground` specifically to stop work when the app stops being active, but it is currently only enforced during uploads (inside `drainBuffer`). The intent in the new comment is to avoid starting BLE connection while suspended; to fully uphold that invariant, the BLE setup phase must also honor `shouldContinueUploading`.

### Fix Focus Areas
- packages/mobile/lib/background-whoop-ble-sync.ts[163-188]
- packages/mobile/lib/background-whoop-ble-sync.ts[254-327]
- packages/mobile/lib/background-whoop-ble-sync.ts[359-446]

### Suggested change
- In `syncOnForeground(...)`, add `if (!shouldContinueUploading()) return;` checks:
 - before calling `findWhoop()`
 - immediately before `connect(...)`
 - immediately before `startImuStreaming()`
 - optionally after each awaited step (to handle backgrounding during the await)
- If backgrounding occurs after a successful `connect` but before `startImuStreaming`, consider best-effort cleanup (e.g., `disconnect()` and leave `connected = false`) so subsequent active transitions can retry cleanly.
- (Optional) Add a regression test that simulates the app backgrounding during the connect path (e.g., flip `AppState.currentState` to `background` between `findWhoop` and `connect`) and assert that `connect`/`startImuStreaming` are not invoked (or are cleaned up).

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


2. BLE init logic in TS ✗ Dismissed 📘 Rule violation ⌂ Architecture
Description
The change adds additional app-state orchestration for WHOOP BLE syncing in TypeScript, which is
domain logic rather than a thin Expo bridge. This conflicts with the requirement that BLE/HealthKit
domain logic live in Swift native modules.
Code

packages/mobile/lib/background-whoop-ble-sync.ts[R163-183]

+  // The AppState listener only fires on state transitions, so sync immediately
+  // when init runs in the foreground. Defer a backgrounded initialization until
+  // the next active transition so this foreground sync does not try to start a
+  // BLE connection while the app is suspended.
+  if (shouldRunForegroundPeriodicDrain()) {
+    logger.info(LOG_CATEGORY, "initializing background sync");
+    try {
+      await syncOnForeground(
+        trpcClient,
+        whoopDeps,
+        realtimeClient,
+        shouldRunForegroundPeriodicDrain,
+      );
+      logger.info(LOG_CATEGORY, "initial sync complete");
+    } catch (error: unknown) {
+      logger.error(LOG_CATEGORY, `initial sync error: ${error}`);
+      captureException(error, { source: "whoop-ble-init-sync" });
+    }
+  } else {
+    logger.info(LOG_CATEGORY, "initial sync deferred until app foregrounds");
  }
Relevance

⭐⭐ Medium

Repo often keeps background sync/orchestration in TS; no clear precedent enforcing “Swift-only” for
this orchestration.

PR-#1405
PR-#1404
PR-#1519

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
PR Compliance ID 721993 requires BLE domain logic to live in Swift with TypeScript used only for
bridging. The added TypeScript code gates and triggers BLE sync behavior based on app state
(foreground/background), which is BLE lifecycle/domain orchestration in TS.

Rule 721993: BLE and HealthKit domain logic must reside in Swift (TypeScript is bridge-only)
packages/mobile/lib/background-whoop-ble-sync.ts[163-183]

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 WHOOP BLE initialization/sync orchestration logic is implemented in TypeScript (`initBackgroundWhoopBleSync`), but compliance requires BLE domain logic to reside in Swift with TS acting as a bridge only.

## Issue Context
This PR adds/changes foreground/background gating for the initial sync in TypeScript. Per the compliance rule, decisions about when to start BLE connections/sync should be owned by the Swift module and exposed via a higher-level native API to JS.

## Fix Focus Areas
- packages/mobile/lib/background-whoop-ble-sync.ts[163-183]

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


3. WHOOP BLE change lacks device test ✓ Resolved 📘 Rule violation ≡ Correctness
Description
This PR changes WHOOP BLE/background-delivery initialization behavior but only adds a unit test,
without any repo-tracked device-only test case/checklist for validating BLE behavior on physical
hardware. This risks shipping BLE regressions that cannot be exercised on the iOS Simulator.
Code

packages/mobile/lib/background-whoop-ble-sync.ts[R163-182]

+  // The AppState listener only fires on state transitions, so sync immediately
+  // when init runs in the foreground. Defer a backgrounded initialization until
+  // the next active transition so this foreground sync does not try to start a
+  // BLE connection while the app is suspended.
+  if (shouldRunForegroundPeriodicDrain()) {
+    logger.info(LOG_CATEGORY, "initializing background sync");
+    try {
+      await syncOnForeground(
+        trpcClient,
+        whoopDeps,
+        realtimeClient,
+        shouldRunForegroundPeriodicDrain,
+      );
+      logger.info(LOG_CATEGORY, "initial sync complete");
+    } catch (error: unknown) {
+      logger.error(LOG_CATEGORY, `initial sync error: ${error}`);
+      captureException(error, { source: "whoop-ble-init-sync" });
+    }
+  } else {
+    logger.info(LOG_CATEGORY, "initial sync deferred until app foregrounds");
Relevance

⭐⭐ Medium

No clear precedent requiring repo-tracked physical-device test artifacts; changes typically
validated via unit tests/CI.

PR-#1401
PR-#1404
PR-#1519

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The compliance rule requires physical-device test coverage artifacts for BLE/background-delivery
changes. The PR modifies the BLE init/deferral logic and adds only a Vitest unit test for the
scenario; existing mobile docs acknowledge BLE requires physical-device validation, but no
device-only test case/checklist is added or referenced for this specific path.

Rule 2237092: Require physical-device testing for BLE, motion sensor, and background delivery code paths
packages/mobile/lib/background-whoop-ble-sync.ts[163-182]
packages/mobile/lib/background-whoop-ble-sync.test.ts[109-125]
packages/mobile/README.md[69-73]

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 WHOOP BLE init behavior changed, but there is no explicit, repo-tracked device-only validation case for this BLE/background path.

## Issue Context
BLE behavior cannot be validated in iOS Simulator; the compliance rule requires at least one clearly identified physical-device test case/checklist for affected BLE/background-delivery paths.

## Fix Focus Areas
- docs/production-incident-baseline.md[17904-17928]
- packages/mobile/lib/background-whoop-ble-sync.ts[163-182]
- packages/mobile/lib/background-whoop-ble-sync.test.ts[109-125]
- packages/mobile/README.md[69-73]

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


Grey Divider

Qodo Logo

@codereviewbot-ai

codereviewbot-ai Bot commented Jul 26, 2026

Copy link
Copy Markdown

LGTM


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

Comment thread packages/mobile/lib/background-whoop-ble-sync.ts
Comment thread packages/mobile/lib/background-whoop-ble-sync.ts
Comment thread packages/mobile/lib/background-whoop-ble-sync.ts

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packages/mobile/lib/background-whoop-ble-sync.test.ts`:
- Around line 121-124: Update the test’s vi.waitFor block to include the
assertion that whoopDeps.startImuStreaming was called, keeping the connect and
streaming assertions together while preserving the existing expected call
arguments.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 1cafb9c5-b66c-490f-b758-b7ab4715e98d

📥 Commits

Reviewing files that changed from the base of the PR and between a853a79 and ea8efc6.

📒 Files selected for processing (3)
  • docs/production-incident-baseline.md
  • packages/mobile/lib/background-whoop-ble-sync.test.ts
  • packages/mobile/lib/background-whoop-ble-sync.ts

Comment thread packages/mobile/lib/background-whoop-ble-sync.test.ts Outdated
@codereviewbot-ai

Copy link
Copy Markdown

🤖 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.

@codereviewbot-ai

Copy link
Copy Markdown

🤖 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.

@Asherlc
Asherlc merged commit ec6f4ac into main Jul 26, 2026
181 of 184 checks passed
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.

2 participants