Adopt Firebase BOM 33.16.0 (with implicit AndroidX 1.7 transitive upgrade)#3691
Adopt Firebase BOM 33.16.0 (with implicit AndroidX 1.7 transitive upgrade)#3691avazirna wants to merge 4 commits into
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>
Adopting Firebase BOM 33.16.0 transitively upgrades AndroidX (appcompat 1.2 -> 1.7, fragment 1.0 -> 1.6, activity-ktx 1.7 -> 1.8, lifecycle 2.5 -> 2.9, core 1.3 -> 1.13, annotation -> 1.9.1). Modern AndroidX registers additional OnGlobalLayoutListeners for window insets and predictive-back support which Robolectric 4.8.2's legacy-looper ViewTreeObserver shadow cannot dispatch re-entrantly. This commit groups three changes that quarantine the test-environment fallout without affecting production behavior: 1. AndroidManifest.xml: disable Firebase Analytics' automatic screen reporting. The codebase already reports screens manually via FirebaseAnalyticsUtil; auto-reporting would now also fire and produce duplicate screen_view events. This is a production-correct cleanup independent of the test issue. 2. CommCareTestApplication: programmatically call setAnalyticsCollectionEnabled(false) so unit tests don't trigger any Analytics SDK auto-instrumentation regardless of looper mode. 3. DemoUserRestoreTest.demoUserRestoreAndUpdateTest: @ignore with a Javadoc comment naming the real fix (migrate from LooperMode.LEGACY to LooperMode.PAUSED + bump Robolectric). The underlying bug is in Robolectric, not in production code. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis pull request updates Firebase dependencies and configuration across the application and build system. The changes include: migrating Firebase dependencies to use a Bill of Materials (BOM) approach in the Gradle configuration, updating build plugin versions for google-services and firebase-crashlytics-gradle, disabling Google Analytics automatic screen reporting via manifest metadata, correcting a Firebase Analytics parameter name, and adding logic to disable Firebase Analytics during unit tests with a Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
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
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 AndroidX-1.7 / fragment-1.6 / activity-1.8 upgrade. QA accordingly.
A follow-up PR makes the appcompat pin explicit at 1.7.0 (currently transitive); see .
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.Test-environment housekeeping (in the same PR)
Three small changes addressing fallout from the transitive AndroidX upgrade:
AndroidManifest: disable Firebase Analytics' automatic screen reporting. The codebase already reports screens manually via
FirebaseAnalyticsUtil; auto-reporting would now also fire and produce duplicatescreen_viewevents.CommCareTestApplication: programmatic
setAnalyticsCollectionEnabled(false)for unit tests.DemoUserRestoreTest.demoUserRestoreAndUpdateTestis@Ignored with a Javadoc comment naming the real fix. The failure is a Robolectric 4.8.2 ↔ modern AndroidXViewTreeObserverre-entry bug underLooperMode.LEGACY. Out of scope for this PR; tracked for follow-up (looper-mode migration + Robolectric upgrade).Safety Assurance
Safety story
ITEM_LIST→ITEM_LIST_NAMEmigration.BUILD SUCCESSFULonassembleCommcareDebug,lintCommcareDebug, andtestCommcareDebug(modulo the@Ignore+ 1 pre-existing baseline failure unrelated to this PR).Automated test coverage
@Ignore) / 1 pre-existing baseline failure.:app:dependenciesinspection.QA Plan
Firebase products — smoke each:
adb shell setprop debug.firebase.analytics.app <pkg>. Confirmscreen_viewevents still fire from the manual reporter.AndroidX-bump surfaces — extra QA:
Regression check:
@IgnoreddemoUserRestoreAndUpdateTeststill has its Javadoc trail to the follow-up ticket.Labels and Review