[No QA] Remove Firebase Crashlytics, use Sentry instead#82370
[No QA] Remove Firebase Crashlytics, use Sentry instead#82370roryabraham merged 25 commits intomainfrom
Conversation
- Rename isFirebaseChecked to isSentryChecked throughout the checklist generation and parsing code - Replace Firebase Crashlytics links with release-specific Sentry URLs (current release filtered to staging environment, previous release without environment filter) - Add previousTag parameter to generateStagingDeployCashBodyAndAssignees so the checklist can link to the specific previous Sentry release - Add explanation that the previous release check matters because mobile deploys use a phased rollout and completing the checklist deploys the previous version to 100% of users - Update tests and rebuild GH Actions bundles Co-authored-by: Cursor <cursoragent@cursor.com>
- Replace Crashlytics error recording with Sentry.captureException in ErrorBoundary - Replace Crashlytics logging with no-op in Firebase/index.native.ts - Remove setCrashlyticsCollectionEnabled from platformSetup - Replace Crashlytics crash() with Sentry.nativeCrash() in testCrash - Remove setCrashlyticsUserId (Sentry.setUser already handles this) - Update TestCrash component to not depend on firebase.json config - Remove @react-native-firebase/crashlytics from package.json Co-authored-by: Cursor <cursoragent@cursor.com>
- Delete firebase.json (only contained crashlytics config) - Remove firebase-crashlytics-gradle classpath from android/build.gradle - Remove firebase-crashlytics dependency and plugin from android/app/build.gradle - Remove firebase.json 1Password download steps from deploy.yml - Remove firebase.json from Pods cache keys in deploy.yml and verifyHybridApp.yml Co-authored-by: Cursor <cursoragent@cursor.com>
|
npm has a |
|
@Julesssss 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] |
- Regenerate package-lock.json after removing @react-native-firebase/crashlytics - Remove patches/@react-native-firebase/crashlytics/ (patch-package patch no longer needed) - Note: ios/Podfile.lock and Mobile-Expensify/iOS/Podfile.lock will be updated by CI when pod install runs during the build. Local CocoaPods version (1.12.1) doesn't support visionos syntax in dependencies. Co-authored-by: Cursor <cursoragent@cursor.com>
pod install removes RNFBCrashlytics, FirebaseCrashlytics, and their transitive dependencies from the lockfile. Co-authored-by: Cursor <cursoragent@cursor.com>
Remove the import and setCrashlyticsCollectionEnabled call that was missed in the initial Crashlytics removal. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@codex review |
- Refactor generateStagingDeployCashBodyAndAssignees to use an options object for isSentryChecked, isGHStatusChecked, and previousTag (fixes @typescript-eslint/max-params violation) - Use early return in platformSetup/index.native.ts (fixes rulesdir/prefer-early-return) - Rename remaining isFirebaseChecked to isSentryChecked in test mock objects (fixes typecheck) - Rebuild GH Actions bundles Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f1455e220b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
- Replace Firebase.log() no-op with Sentry.addBreadcrumb() in Firebase/index.native.ts (addresses review comment about losing navigation breadcrumbs) - Fix test: use version 1.0.2-2 in the version-bump test scenario (the Sentry URL must match the bumped version) - Rebuild GH Actions bundles from clean install Co-authored-by: Cursor <cursoragent@cursor.com>
- Firebase/index.native.ts: Replace crashlytics with Sentry.addBreadcrumb (this file was missed in the original commit) - testCrash/index.native.ts: Replace with Sentry.nativeCrash() - TestCrash/index.native.tsx: Remove firebase.json dependency - GithubUtilsTest.ts: Update Firebase verification constants to Sentry (these were missed in the earlier test update) Co-authored-by: Cursor <cursoragent@cursor.com>
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
This comment was marked as resolved.
This comment was marked as resolved.
Delete the upload_dsyms and upload_dsyms_hybrid fastlane lanes that uploaded dSYM files to Firebase Crashlytics. Sentry handles debug symbol uploads via its own Xcode build phase and Gradle plugin. Also remove the "Upload DSYMs to Firebase for HybridApp" step from deploy.yml that called the now-deleted lane. Co-authored-by: Cursor <cursoragent@cursor.com>
Add environment=production to the previous release Sentry URL so deployers see only production crashes for the version being rolled out to 100% of users. Co-authored-by: Cursor <cursoragent@cursor.com>
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
The Firebase lib was a thin wrapper that only called Sentry.addBreadcrumb on native and was a no-op on web. Replace the single usage in NavigationRoot with a direct Sentry.addBreadcrumb call that works on all platforms, delete the now-unused Firebase lib, its dead web config, and the @firebase/app dependency. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@Julesssss ready for review |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
🚧 @Julesssss has triggered a test Expensify/App build. You can view the workflow run here. |
This comment was marked as off-topic.
This comment was marked as off-topic.
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
|
Seems like something still expects Firebase to exist in the iOS build. Mayyybe will be resolved and is an outdated run? I'll try a test build |
|
🚧 @Julesssss 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! 🧪🧪
|
|
🚧 @roryabraham has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/roryabraham in version: 9.3.25-0 🚀
|
|
🚀 Deployed to production by https://github.com/puneetlath in version: 9.3.25-13 🚀
|
Explanation of Change
Remove Firebase Crashlytics from the App, replacing it with Sentry (which is already integrated). This also updates the deploy checklist to link to Sentry instead of Firebase Crashlytics.
Firebase Analytics and the web SDK are kept for Google Tag Manager.
Deploy checklist:
isFirebaseCheckedtoisSentryCheckedpreviousTagparameter so the checklist links to the exact previous Sentry releaseApp source:
setCrashlyticsUserId(redundant withSentry.setUser())@react-native-firebase/crashlyticsdependencyNative build config:
firebase.json(only contained crashlytics config)firebase.json1Password download steps and cache key references from CI workflowsDepends on: https://github.com/Expensify/Mobile-Expensify/pull/13849 (Mobile-Expensify crashlytics removal -- submodule bump needed after merge)
Fixed Issues
$
Tests
Offline tests
N/A
QA Steps
N/A - [No QA]
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))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.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
N/A - no UI changes