Repository navigation
Upgrade Sentry React Native SDK to 8.32.0 and revert the TurboModuleContext workaround - #105398
Conversation
…ontext workaround
|
|
|
@MelvinBot review |
Review: no blocking issuesThe upgrade looks correct. CI Verified against the published 8.27.0 and 8.32.0 sources:
Suggestions (non-blocking):
Not verified
|
|
Answering to this Thanks, both wording fixes applied |
|
@MelvinBot review |
|
@shubham1206agra Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Re-review: no blocking issuesThe only change since my last review is commit Re-verified against the upstream 8.27.0 and 8.32.0 sources:
Optional nitThe root |
|
@shubham1206agra its ready for review |
|
@arekm213 Please add video for atleast one platform |
|
@shubham1206agra i added video from iOS native - there is no UI changes in this PR so what i recorded is simply running app and switching between main tabs to make sure nothing breaks |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
|
🚧 mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
Explanation of Change
Upgrades
@sentry/react-nativefrom 8.27.0 to 8.32.0 and reverts the workaround from #103900, which disabled theTurboModuleContextintegration.Why the workaround can go: in 8.27.0,
TurboModuleContextalways wrappedRNSentryitself, in addition to any modules listed in itsmodulesoption. 8.32.0 fixes this upstream (getsentry/sentry-react-native#6921):RNSentryis no longer wrapped unless it is explicitly listed in the integration'smodules. So the integration goes back into our default list.Dependency changes:
@sentry/react-native8.27.0 → 8.32.0@sentry/coreand@sentry/browser-utils(devDep) 10.75.0 → 10.76.2. 8.32.0 pins the JS SDK to exactly 10.76.2, so we bump these to keep a single copy of@sentry/core.@sentry/*packages (plus@sentry/cli3.8.1 and adotenvrelocation).ios/Podfile.lock: RNSentry 8.32.0, prebuilt checksum.Removed
_experiments.profilingOptions: 8.32.0 (getsentry/sentry-react-native#6916) auto-addsuiProfilingIntegrationwhenever_experiments.profilingOptionsis set. That integration calls the nativestartProfilerfor each root span. We start the native SDK ourselves inSentryNativeSDKManagerwithout any profiling options (autoInitializeNativeSdk: false). On iOS, sentry-cocoa then seesoptions.profiling == nil, skips session sampling and starts the continuous profiler unconditionally, which means on every session regardless ofprofileSessionSampleRate. This only starts with 8.32.0; on main (8.27.0) nothing callsstartProfiler, so the option has no effect there. Reproduced on an iOS simulator with 8.32.0: withprofilingOptionskept, anio.sentry.SamplingProfilerthread runs; with it removed, as in this PR, it doesn't. Android is unaffected because sentry-java gates onprofileSessionSampleRate.The option did nothing before this upgrade. It only reached the native SDK through the JS-side native init, which we skip, and Sentry shows 0 iOS UI profiles over the last 14 days. Removing it therefore changes no data we collect today. A comment in
setupSentry.tsexplains why it must not be re-added on the JS side alone. Real UI profiling would need the options in the native init (App + Mobile-Expensify) and can be done as a follow-up if we want it.Other upstream changes between 8.27 and 8.32 were reviewed and don't affect us:
maxTransactionDurationExceededwas removed. We don't use it.sideEffects: falseonly affects web tree-shaking.Fixed Issues
$ #103906
PROPOSAL: N/A
MOBILE-EXPENSIFY: https://github.com/Expensify/Mobile-Expensify/pull/14212
Tests
frames.*measurements and navigation transactions include TTID.turbo_modulecontext or tag forRNSentry.data-sentry-labeland verify theui.clickbreadcrumb contains the label.Offline tests
N/A. Telemetry library upgrade, no change in offline behaviour.
QA Steps
http.clientspan for an/apirequest from this build and verify it still has therequest-idattribute.PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
screen-recording-4EB9FEDE-39A1-4AA2-AAB9-7E7D188D5B3F-1793611118128.mp4
iOS: mWeb Safari
MacOS: Chrome / Safari