fix(healthkit): stop reporting observer expirations to Sentry - #2352
Merged
Merged
Conversation
Mark observer sync in progress at native delivery and in the JavaScript listener so iOS background timing boundaries no longer produce false DOFEK-MOBILE-1C alerts. Co-authored-by: Cursor <cursoragent@cursor.com>
|
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. |
|
🤖 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. |
Contributor
Reviewer's GuideThis PR adjusts HealthKit observer sync state handling and Sentry reporting so that observer expirations are tracked as non-error breadcrumbs, and the sync-in-progress flag is aligned with both native and JavaScript pending updates to avoid false-positive Sentry issues under background suspension. Sequence diagram for updated HealthKit observer sync and expiration handlingsequenceDiagram
participant HealthKit as HealthKit
participant HealthKitModule as HealthKitModule
participant Coordinator as HealthKitObserverUpdateCoordinator
participant JS as background_health_kit_sync
participant Sentry as SentrySDK
HealthKit->>HealthKitModule: observer callback(typeIdentifier, completionHandler)
HealthKitModule->>Coordinator: register(typeIdentifier, completion)
HealthKitModule->>HealthKitModule: observerSyncInProgress = true
HealthKitModule->>JS: MainThreadEventEmitter.emit(event)
JS->>JS: pendingUpdates.set(updateId, typeIdentifier)
JS->>HealthKitModule: setObserverSyncInProgress(true)
JS->>JS: drainSyncQueue()
JS->>HealthKitModule: setObserverSyncInProgress(pendingUpdates.size > 0)
Coordinator-->>HealthKitModule: reportExpiration(expiration)
HealthKitModule->>Sentry: addBreadcrumb(breadcrumb)
alt !observerUpdateCoordinator.hasPendingUpdates
HealthKitModule->>HealthKitModule: observerSyncInProgress = false
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughThe PR fixes HealthKit observer synchronization and expiration telemetry, centralizes weekly and monthly report empty-state fixtures, updates report data types and tests, and changes the Withings timeout test to use a typed error assertion. ChangesHealthKit observer synchronization
Report empty-state data flow
Provider timeout assertion
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant HealthKitObserver
participant HealthKitModule
participant HealthKitObserverUpdateCoordinator
participant BackgroundHealthKitSync
participant Sentry
HealthKitObserver->>HealthKitModule: register observer update
HealthKitModule->>HealthKitObserverUpdateCoordinator: store pending update
HealthKitModule->>BackgroundHealthKitSync: set sync in progress
HealthKitObserver->>HealthKitModule: report expiration
HealthKitModule->>Sentry: record expiration breadcrumb
BackgroundHealthKitSync->>HealthKitObserverUpdateCoordinator: process queued updates
HealthKitObserverUpdateCoordinator-->>BackgroundHealthKitSync: pending updates remain or queue is empty
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
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. Comment |
PR Summary by QodoHealthKit: treat observer expirations as breadcrumbs; tighten sync-in-progress tracking
AI Description
Diagram
High-Level Assessment
Files changed (7)
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The
setObserverSyncInProgresssemantics are now more nuanced (false is ignored when native has pending updates); consider renaming or documenting this function to reflect its coordination role rather than a simple boolean setter to avoid future misuse. - For the Sentry breadcrumbs on observer expirations, you might include additional fields like
observerSyncInProgressandhasPendingUpdatesinbreadcrumb.datato make it easier to distinguish background-suspension scenarios during incident analysis.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `setObserverSyncInProgress` semantics are now more nuanced (false is ignored when native has pending updates); consider renaming or documenting this function to reflect its coordination role rather than a simple boolean setter to avoid future misuse.
- For the Sentry breadcrumbs on observer expirations, you might include additional fields like `observerSyncInProgress` and `hasPendingUpdates` in `breadcrumb.data` to make it easier to distinguish background-suspension scenarios during incident analysis.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
Mobile PreviewScan to open on device:
To test on device:
|
Contributor
|
Storybook previews for This comment updates automatically on each PR push. |
Code Review by Qodo
Context used✅ Compliance rules (platform):
179 rules✅ Skills:
fix-provider, write-tests, cloudflare 1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Replace mise.toml locked settings and mise trust with MISE_LOCKED=1 in setup scripts and docs so Conductor, devcontainer, and local workflows install only pinned versions. Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 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
enabled auto-merge (squash)
July 31, 2026 01:52
Fix undefined fullSync in provider routine sync, add report emptyState fixtures, use WeeklyReportData for display components, update Withings timeout expectations, and apply Biome formatting. Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 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
disabled auto-merge
July 31, 2026 02:20
Asherlc
enabled auto-merge (squash)
July 31, 2026 02:22
Resolve conflicts in report empty-state fixtures/tests and mobile provider sync tests. Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 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. |
Remove recovery fields from component fixtures that use WeeklyReportData/MonthlyReportData, strip recovery when rendering shared reports, and avoid referencing observerUpdateCoordinator during its lazy initialization. Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/web/src/components/WeeklyReportCard.stories.tsx (1)
78-95: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the imported fixture in the
Emptystory.The default story uses
weeklyReportEmptyStateFixture, but theEmptystory keeps a second copy of the same payload. Replace the inline object with the imported fixture so the story cannot drift from the shared contract.🤖 Prompt for 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. In `@packages/web/src/components/WeeklyReportCard.stories.tsx` around lines 78 - 95, Update the Empty story in WeeklyReportCard stories to use the imported weeklyReportEmptyStateFixture instead of duplicating the inline emptyState payload. Preserve the story’s existing behavior and remove only the redundant object definition.Source: Coding guidelines
🤖 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-health-kit-sync.test.ts`:
- Around line 665-673: Add a regression test around the observer listener setup
that queues an update while pendingCatchUp remains unresolved, then complete the
queued catch-up and assert mockSetObserverSyncInProgress is never called with
false before catch-up finalization. Preserve the existing assertion for the
immediate true transition and use the existing listener, pendingCatchUp, and
catch-up completion symbols.
In `@packages/mobile/lib/background-health-kit-sync.ts`:
- Line 207: Update the sync-state assignment in drainSyncQueue to remain true
whenever either pendingUpdates has entries or pendingCatchUp is set, preventing
the native HealthKit sync from appearing idle before catch-up begins. Add a
regression test covering an immediate observer delivery during setup and verify
the state stays active until catch-up completes.
In `@packages/mobile/modules/health-kit/ios/HealthKitModule.swift`:
- Around line 54-67: Accesses and transitions of observerSyncInProgress are not
synchronized across concurrent HealthKit paths. In
packages/mobile/modules/health-kit/ios/HealthKitModule.swift lines 54-67,
850-854, and 906-911, introduce or reuse one shared observer-state lock and
guard the flag reads, writes, and hasPendingUpdates check together; update
handleObserverUpdateExpiration and the corresponding registration,
expiration-completion, and bridge-call paths so all observerSyncInProgress state
changes are serialized without changing their existing behavior.
In `@packages/mobile/modules/health-kit/README.md`:
- Around line 39-44: Update the expiration description in the HealthKit README
to cover both cases: JavaScript never responding and synchronization starting
without native completion being received by the 25-second deadline. Replace the
phrase “when JavaScript never responds” with wording centered on native
completion not arriving before the deadline.
In
`@packages/mobile/modules/health-kit/Tests/HealthKitObserverUpdateCoordinatorTests.swift`:
- Around line 57-66: Extend
testHasPendingUpdatesReflectsRegistrationAndCompletion to register two updates,
assert pending state after both registrations, complete only one and assert
hasPendingUpdates remains true, then complete the second and assert it becomes
false. Keep the existing XCTest coordinator-state coverage and use the returned
update IDs to verify each completion transition.
In `@packages/web/src/routes/health-report.tsx`:
- Around line 167-174: Update the route-boundary schemas in the weekly and
monthly report handling so their current fields accept null by making
weeklyReportSchema.current use weekSummarySchema.nullable() and
monthlyReportSchema.current use monthSummarySchema.nullable(). Add route tests
covering empty weekly and monthly reports, ensuring validation succeeds and the
existing empty-state rendering is reached.
- Around line 167-174: Use the canonical server payload in the health report
route: in both the WeeklyReportCard and MonthlyReportCard data constructions,
pass reportData.emptyState instead of the corresponding fixture. Remove the
now-unused fixture import and delete the production duplicate in
packages/web/src/components/report-empty-state-fixtures.ts, or restrict it to a
typed test/story projection of the canonical contract; apply these changes at
packages/web/src/routes/health-report.tsx lines 167-174 and 185-192, and
packages/web/src/components/report-empty-state-fixtures.ts lines 1-38.
---
Outside diff comments:
In `@packages/web/src/components/WeeklyReportCard.stories.tsx`:
- Around line 78-95: Update the Empty story in WeeklyReportCard stories to use
the imported weeklyReportEmptyStateFixture instead of duplicating the inline
emptyState payload. Preserve the story’s existing behavior and remove only the
redundant object definition.
🪄 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: a1a3dbaa-6ceb-4541-882a-4e4b317def20
📒 Files selected for processing (17)
docs/production-incident-baseline.mdpackages/mobile/lib/background-health-kit-sync.test.tspackages/mobile/lib/background-health-kit-sync.tspackages/mobile/modules/health-kit/README.mdpackages/mobile/modules/health-kit/Tests/HealthKitObserverUpdateCoordinatorTests.swiftpackages/mobile/modules/health-kit/ios/HealthKitModule.swiftpackages/mobile/modules/health-kit/ios/HealthKitObserverUpdateCoordinator.swiftpackages/server/src/routers/health-report.test.tspackages/web/src/components/MonthlyReportContent.stories.tsxpackages/web/src/components/MonthlyReportContent.test.tsxpackages/web/src/components/MonthlyReportContent.tsxpackages/web/src/components/WeeklyReportCard.stories.tsxpackages/web/src/components/WeeklyReportCard.test.tsxpackages/web/src/components/WeeklyReportCard.tsxpackages/web/src/components/report-empty-state-fixtures.tspackages/web/src/routes/health-report.tsxsrc/providers/withings.test.ts
Avoid Docker Hub timeouts that were failing the image vulnerability scan and cascading security gates. Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 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. |
Keep observer sync active while catch-up is pending, serialize native observer state under a lock, accept nullable empty report snapshots, and add regression tests plus Apple doc citations. Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 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. |
2 tasks
Protects the breadcrumb snapshot and clear path from racing with bridge and delivery writes, and enriches expiration telemetry with pending state. Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
com.dofek.healthkit-observer: Code: 1as an error.Resolves DOFEK-MOBILE-1C.
Test plan
pnpm vitest run --project mobile packages/mobile/lib/background-health-kit-sync.test.tsswift testinpackages/mobile/modules/health-kitMade with Cursor
Summary by cubic
Stops reporting HealthKit observer expirations to Sentry as errors and hardens sync-in-progress tracking across iOS background suspension and JS catch-up. Also pins tool installs via
MISE_LOCKED, aligns report data types and empty states with main, and mirrorsanchore/grypepulls in CI to avoid timeouts.Bug Fixes
hasPendingUpdates.com.dofek.healthkit-observer: Code: 1), and includeobserverSyncInProgressandhasPendingUpdatesin breadcrumb data.currentsnapshots; align web toWeeklyReportData/MonthlyReportData, droprecovery, and render server-providedemptyState.sinceDaysfor provider sync; remove the undefined full-sync path. Withings tests now expectProviderRequestTimeoutErrorfrom@dofek/provider-http.Dependencies
MISE_LOCKED=1for pinned installs across Conductor, devcontainer, and docs; removed locked settings frommise.tomland droppedmise trust.anchore/grypeviamirror.gcr.ioin CI to avoid Docker Hub timeouts.Written for commit 46a4722. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Report Improvements
Documentation