Migrate to Firebase BOM 33.16.0#3691
Open
avazirna wants to merge 5 commits into
Open
Conversation
- com.google.gms:google-services 4.3.14 -> 4.4.2 - com.google.firebase:firebase-crashlytics-gradle 2.9.2 -> 3.0.2 Crashlytics SDK 19.x (which the upcoming firebase-bom:33.16.0 brings in) requires the 3.x line of the gradle plugin for mapping-file uploads to function correctly. Bumping these in a separate commit so the change shows up cleanly in blame and is easy to revert independently. The firebase:perf-plugin stays at 2.0.2 — confirmed compatible with firebase-perf SDK 21.0.5. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Replace the five hand-pinned Firebase artifact versions with a single firebase-bom:33.16.0 platform() dependency. The BOM resolves the artifacts to a coherent set: firebase-analytics: 20.1.2 -> 22.5.0 firebase-messaging: 24.0.0 -> 24.1.2 firebase-perf: 21.0.1 -> 21.0.5 firebase-crashlytics: 18.3.7 -> 19.4.4 firebase-auth: 23.2.0 -> 23.2.1 Eliminates per-artifact version drift; future Firebase bumps only need to change the BOM line. The gradle-side plugin bump that pairs with this (Crashlytics 3.0.2, google-services 4.4.2) lands in the previous commit. Audit notes: - No usage of removed/deprecated Analytics APIs (no setCurrentScreen). - No usage of Crashlytics KeyValueBuilder (only fully removed at 20.x). - No firebase-*-ktx modules in use, so 34.x's KTX removal stays out of scope for this bump. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
ITEM_LIST was deprecated and removed from firebase-analytics around
21.0.0 in favor of ITEM_LIST_ID and ITEM_LIST_NAME. The single call
site (reportViewArchivedFormsList) was passing a human-readable list
name ("incomplete" / "saved"), so ITEM_LIST_NAME is the semantic
match. Required to compile against the firebase-bom:33.16.0 versions
(analytics 22.5.0).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/unit-tests/src/org/commcare/android/tests/DemoUserRestoreTest.java (1)
90-94: ⚡ Quick winAdd a tracked issue ID to the
@Ignoreso this test doesn’t get stranded.
Please include a ticket reference (and ideally an expiry/re-enable condition) directly in the ignore reason to keep follow-up enforceable.As per coding guidelines, "Use unit tests for logic verification".
🤖 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 `@app/unit-tests/src/org/commcare/android/tests/DemoUserRestoreTest.java` around lines 90 - 94, The `@Ignore` annotation on DemoUserRestoreTest.java currently has a long reason but no tracked issue or re-enable condition; update the ignore reason on the `@Ignore` above the DemoUserRestoreTest class/test to include a ticket/issue ID (e.g., JIRA-1234 or GitHub issue number) and an explicit re-enable condition or expiry (for example "re-enable when migrated to LooperMode.PAUSED and Robolectric bumped" or a date), so the test won't be orphaned—edit the `@Ignore` string literal to append the issue reference and the re-enable condition.
🤖 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.
Nitpick comments:
In `@app/unit-tests/src/org/commcare/android/tests/DemoUserRestoreTest.java`:
- Around line 90-94: The `@Ignore` annotation on DemoUserRestoreTest.java
currently has a long reason but no tracked issue or re-enable condition; update
the ignore reason on the `@Ignore` above the DemoUserRestoreTest class/test to
include a ticket/issue ID (e.g., JIRA-1234 or GitHub issue number) and an
explicit re-enable condition or expiry (for example "re-enable when migrated to
LooperMode.PAUSED and Robolectric bumped" or a date), so the test won't be
orphaned—edit the `@Ignore` string literal to append the issue reference and the
re-enable condition.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3fb7631e-0b44-4342-9006-3dd6763620c2
📒 Files selected for processing (6)
app/AndroidManifest.xmlapp/build.gradleapp/src/org/commcare/google/services/analytics/FirebaseAnalyticsUtil.javaapp/unit-tests/src/org/commcare/CommCareTestApplication.javaapp/unit-tests/src/org/commcare/android/tests/DemoUserRestoreTest.javabuild.gradle
4 tasks
avazirna
force-pushed
the
deps/firebase-bom-33.16.0
branch
from
July 14, 2026 08:42
41fed58 to
2838794
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3691 +/- ##
============================================
+ Coverage 26.81% 26.84% +0.02%
- Complexity 4674 4684 +10
============================================
Files 984 984
Lines 58522 58587 +65
Branches 6969 6975 +6
============================================
+ Hits 15692 15726 +34
- Misses 40929 40956 +27
- Partials 1901 1905 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
avazirna
changed the title
shubham1g5
requested changes
Jul 14, 2026
shubham1g5
left a comment
shubham1g5
left a comment
Contributor
There was a problem hiding this comment.
Think the QA notes are not well written and needs to be revised here.
Comment on lines
+237
to
+238
| - **Analytics:** spot-check that Firebase events still arrive (e.g. in the Firebase console DebugView), including the event logged when opening the Saved/Incomplete forms list, whose parameter name changed with the upgrade. | ||
| - **Crashlytics:** force a test crash on a debug/QA build and confirm the report shows up in the Crashlytics console. |
Contributor
There was a problem hiding this comment.
think both of these are in scope of dev-testing and not QA.
Comment on lines
+235
to
+236
| - **Push notifications (Firebase Messaging):** verify Connect push notifications are still received and open the right screens — with the app in foreground, backgrounded, and fully killed. A fresh install should register for notifications correctly (new FCM token). | ||
| - **Phone OTP (Firebase Auth):** run a PersonalID signup and a recovery, verifying the SMS OTP is sent, auto-read/entry works, and wrong-code and resend flows behave as before. |
Contributor
There was a problem hiding this comment.
lets be conscise here and say verify Personal ID notifications and phone otp verification still works , the more verbose these notes are, the more likely they are to get lost amongst noise.
| - **Phone OTP (Firebase Auth):** run a PersonalID signup and a recovery, verifying the SMS OTP is sent, auto-read/entry works, and wrong-code and resend flows behave as before. | ||
| - **Analytics:** spot-check that Firebase events still arrive (e.g. in the Firebase console DebugView), including the event logged when opening the Saved/Incomplete forms list, whose parameter name changed with the upgrade. | ||
| - **Crashlytics:** force a test crash on a debug/QA build and confirm the report shows up in the Crashlytics console. | ||
| - **UI smoke test (AppCompat backs all toolbar, options-menu, dialog, and dropdown widgets):** |
Contributor
There was a problem hiding this comment.
QA would not know what is AppCompat
Comment on lines
+240
to
+242
| - App Home screen: grid tiles render and size correctly (portrait and landscape), toolbar title shows, options menu opens and shows the right items — including as a demo user, where only "Change Language" should be visible. | ||
| - Navigation drawer opens/closes correctly from the home screen. | ||
| - Login screen, form entry, and app settings/preferences screens render correctly, including dialogs and spinners/combobox dropdowns. |
Contributor
There was a problem hiding this comment.
can we remove anything here that's already verified by our integration tests.
Contributor
Author
I agree. I plan to run the dev-focused testing and will revise the notes accordingly |
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
2 participants
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.
Technical Summary
Replaces five hand-pinned Firebase artifact versions with a single
firebase-bom:33.16.0platform()dependency. Brings the Firebase SDK family from substantial drift (analytics 20.1.2 / crashlytics 18.3.7) to a coherent, current baseline.What this PR really is — read this carefully
Adopting BOM 33.16.0 transitively forces a wide set of AndroidX upgrades. Hidden dependencies bumped:
So this PR is implicitly also an fragment-1.6 / activity-1.8 upgrade. QA accordingly.
Resolved Firebase versions (managed by BOM)
Plugin classpath bumps (paired with the BOM)
Required code change
FirebaseAnalytics.Param.ITEM_LISTwas removed in analytics 21+. Single call site (FirebaseAnalyticsUtil.reportViewArchivedFormsList) migrated toITEM_LIST_NAME, which matches the human-readable list name actually being passed.Safety Assurance
Safety story
ITEM_LIST→ITEM_LIST_NAMEmigration.Labels and Review