Add expoUpdatesListenerIntegration that records breadcrumbs for Expo Updates lifecycle events#5795
Add expoUpdatesListenerIntegration that records breadcrumbs for Expo Updates lifecycle events#5795
expoUpdatesListenerIntegration that records breadcrumbs for Expo Updates lifecycle events#5795Conversation
Semver Impact of This PR⚪ None (no version bump detected) 📋 Changelog PreviewThis is how your changes will appear in the changelog.
🤖 This preview updates automatically when you update the PR. |
|
alwx
changed the title
expoUpdatesListenerIntegration that records breadcrumbs for Expo Updates lifecycle events
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| const ctx = event.context; | ||
| handleStateChange(previousContext, ctx); | ||
| previousContext = ctx; | ||
| }); |
There was a problem hiding this comment.
Listener callback lacks try/catch, risking app crash
Medium Severity
The addListener callback from expo-updates is not wrapped in a try/catch. If event.context is unexpectedly null/undefined, or if any getData function throws (e.g., the non-null assertion ctx.rollback!.commitTime at line 98, or the as Error cast at lines 82/90), the exception propagates unhandled into expo-updates internals and could crash the host app. Per project rules, SDK instrumentation errors must never crash the host application — dangerous paths need try/catch with graceful fallback. This violates the rule from the rules file requiring error paths to be handled explicitly.
Triggered by project rule: PR Review Guidelines for Cursor Bot
|
|
||
| integrations.push(expoContextIntegration()); | ||
| integrations.push(expoConstantsIntegration()); | ||
| integrations.push(expoUpdatesListenerIntegration()); |
There was a problem hiding this comment.
m: I was wondering if we should add an option for this and not always add the integration unconditionally. Since this adds a listener it might add overhead and users might not always want to enable.
Wdyt of making it opt-in?
There was a problem hiding this comment.
We could add it by default on the next major, what do you think?
| - Add `expoUpdatesListenerIntegration` that records breadcrumbs for Expo Updates lifecycle events ([#5795](https://github.com/getsentry/sentry-react-native/pull/5795)) | ||
| - Tracks update checks, downloads, errors, rollbacks, and restarts as `expo.updates` breadcrumbs | ||
| - Enabled by default in Expo apps (requires `expo-updates` to be installed) | ||
|
|
There was a problem hiding this comment.
We should omit the blank line for consistency
lucas-zimerman
left a comment
lucas-zimerman
left a comment
There was a problem hiding this comment.
looking good! My comments are the same ones as the ones made by Antonis
3 participants
📢 Type of change
📜 Description
Add
expoUpdatesListenerIntegrationthat records breadcrumbs for Expo Updates lifecycle eventsexpo.updatesbreadcrumbsexpo-updatesto be installed)Fixes #5425
📝 Checklist
sendDefaultPIIis enabled🔮 Next steps