Added network logs whenever FCM token fetch is failed #3481
Conversation
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
Jignesh-dimagi
requested review from
OrangeAndGreen,
conroy-ricketts and
shubham1g5
📝 WalkthroughWalkthroughThe PR introduces a new public API Sequence DiagramsequenceDiagram
participant Caller
participant ConnectivityStatus
participant ActivityCompat
participant ConnectivityManager
participant NetworkCapabilities
participant TelephonyManager
Caller->>ConnectivityStatus: getNetworkType(context)
Note over ConnectivityStatus: Check Android Version
alt Android N (API 24+)
ConnectivityStatus->>ConnectivityManager: getActiveNetwork()
ConnectivityManager-->>ConnectivityStatus: network
alt Network exists
ConnectivityStatus->>ConnectivityManager: getNetworkCapabilities(network)
ConnectivityManager-->>ConnectivityStatus: capabilities
Note over ConnectivityStatus: Check Transport Type
alt TRANSPORT_WIFI
ConnectivityStatus-->>Caller: "wifi"
else TRANSPORT_ETHERNET
ConnectivityStatus-->>Caller: "ethernet"
else TRANSPORT_CELLULAR
rect rgb(200, 220, 255)
Note over ConnectivityStatus: Check READ_PHONE_STATE Permission
ConnectivityStatus->>ActivityCompat: checkSelfPermission()
ActivityCompat-->>ConnectivityStatus: permission result
alt Permission Granted
ConnectivityStatus->>TelephonyManager: getNetworkType()
TelephonyManager-->>ConnectivityStatus: subtype constant
ConnectivityStatus->>ConnectivityStatus: mapSubtypeToString(subtype)
ConnectivityStatus-->>Caller: "2g"/"3g"/"4g"/"5g"
else Permission Denied
ConnectivityStatus-->>Caller: "unknown_network"
end
end
else Other
ConnectivityStatus-->>Caller: "unknown_network"
end
else No network
ConnectivityStatus-->>Caller: "no_network"
end
else Below Android N
rect rgb(240, 220, 200)
Note over ConnectivityStatus: Legacy NetworkInfo API
ConnectivityStatus->>ConnectivityManager: getActiveNetworkInfo()
ConnectivityManager-->>ConnectivityStatus: networkInfo
alt NetworkInfo type = TYPE_WIFI
ConnectivityStatus-->>Caller: "wifi"
else TYPE_ETHERNET
ConnectivityStatus-->>Caller: "ethernet"
else TYPE_MOBILE
ConnectivityStatus->>TelephonyManager: getNetworkType()
TelephonyManager-->>ConnectivityStatus: subtype
ConnectivityStatus->>ConnectivityStatus: mapSubtypeToString(subtype)
ConnectivityStatus-->>Caller: "2g"/"3g"/"4g"/"5g"
else Other or None
ConnectivityStatus-->>Caller: "unknown_network"/"no_network"
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @app/src/org/commcare/utils/ConnectivityStatus.java:
- Around line 63-86: getNetworkTypeFromAndroid24 may NPE because
connectivityManager from context.getSystemService(ConnectivityManager.class) can
be null; add a null check right after obtaining connectivityManager in that
method (ConnectivityManager connectivityManager = ...), and if it's null return
NO_NETWORK (same convention used elsewhere), so the method only proceeds to call
getActiveNetwork() and getNetworkCapabilities() when connectivityManager is
non-null.
- Around line 88-104: In getNetworkTypeFromBelowAndroid24, add a null check for
the ConnectivityManager (cm) immediately after obtaining it and return
NO_NETWORK (or the same sentinel used in getNetworkTypeFromAndroid24) if cm is
null to avoid an NPE; i.e., after "ConnectivityManager cm =
(ConnectivityManager) context.getSystemService(...)" check "if (cm == null)
return NO_NETWORK;" before calling cm.getActiveNetworkInfo(), keeping existing
logic for activeNetwork, WIFI, TYPE_MOBILE, and READ_PHONE_STATE_UNKNOWN_NETWORK
intact.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/src/org/commcare/utils/ConnectivityStatus.javaapp/src/org/commcare/utils/FirebaseMessagingUtil.java
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: shubham1g5
Repo: dimagi/commcare-android PR: 0
File: :0-0
Timestamp: 2025-05-08T11:08:18.530Z
Learning: PR #3048 "Phase 4 Connect PR" introduces a substantial feature called "Connect" to the CommCare Android app, which includes messaging, job management, delivery tracking, payment processing, authentication flows, and learning modules. It follows a modern architecture using Navigation Components with three navigation graphs, segregated business logic in Manager classes, and proper database persistence.
🧬 Code graph analysis (1)
app/src/org/commcare/utils/FirebaseMessagingUtil.java (1)
app/src/org/commcare/utils/ConnectivityStatus.java (1)
ConnectivityStatus(24-122)
🔇 Additional comments (4)
app/src/org/commcare/utils/FirebaseMessagingUtil.java (1)
103-119: LGTM! Network status logging for FCM diagnostics looks good.The addition of network type information to the error logs will help diagnose intermittent FCM token fetch failures. The implementation correctly covers both the empty-token and task-failure cases.
app/src/org/commcare/utils/ConnectivityStatus.java (3)
28-36: LGTM! Well-defined constants for network types.The constants provide clear, descriptive labels for network state logging. The
READ_PHONE_STATE_UNKNOWN_NETWORKdistinction is useful for diagnosing permission-related issues separately from actual network detection failures.
44-53: Good defensive null check added.The null check for
conManagerbefore callinggetActiveNetworkInfo()prevents a potential NPE and maintains consistent logging behavior.
106-121: LGTM! Comprehensive network type mapping.The switch expression correctly maps all common TelephonyManager network type constants to their respective generations. The default case appropriately handles any unknown or future network types.
Comment on lines
63
to
86
| @RequiresApi(Build.VERSION_CODES.N) | ||
| private static String getNetworkTypeFromAndroid24(Context context){ | ||
| ConnectivityManager connectivityManager = context.getSystemService(ConnectivityManager.class); | ||
| Network currentNetwork = connectivityManager.getActiveNetwork(); | ||
| if(currentNetwork!=null) { | ||
| NetworkCapabilities networkCapabilities = connectivityManager.getNetworkCapabilities(currentNetwork); | ||
| if (networkCapabilities != null) { | ||
| if (networkCapabilities.hasTransport(NetworkCapabilities.TRANSPORT_WIFI)) { | ||
| return WIFI; | ||
| }else if(networkCapabilities.hasTransport(NetworkCapabilities.TRANSPORT_ETHERNET)){ | ||
| return ETHERNET; | ||
| }else if (networkCapabilities.hasTransport(NetworkCapabilities.TRANSPORT_CELLULAR)) { | ||
| TelephonyManager tm = (TelephonyManager)context.getSystemService(Context.TELEPHONY_SERVICE); | ||
| if (ActivityCompat.checkSelfPermission(context, Manifest.permission.READ_PHONE_STATE) == PackageManager.PERMISSION_GRANTED) { | ||
| return getNetworkType(tm.getDataNetworkType()); | ||
| }else{ | ||
| return READ_PHONE_STATE_UNKNOWN_NETWORK; | ||
| } | ||
| } | ||
| return UNKNOWN_NETWORK; | ||
| } | ||
| } | ||
| return NO_NETWORK; | ||
| } |
There was a problem hiding this comment.
Add null check for ConnectivityManager to prevent potential NPE.
getSystemService() can return null in edge cases (e.g., restricted contexts). The method isNetworkAvailable already handles this scenario with a null check, but this method does not.
🐛 Proposed fix
@RequiresApi(Build.VERSION_CODES.N)
private static String getNetworkTypeFromAndroid24(Context context){
ConnectivityManager connectivityManager = context.getSystemService(ConnectivityManager.class);
+ if (connectivityManager == null) {
+ return UNKNOWN_NETWORK;
+ }
Network currentNetwork = connectivityManager.getActiveNetwork();
if(currentNetwork!=null) {🤖 Prompt for AI Agents
In @app/src/org/commcare/utils/ConnectivityStatus.java around lines 63 - 86,
getNetworkTypeFromAndroid24 may NPE because connectivityManager from
context.getSystemService(ConnectivityManager.class) can be null; add a null
check right after obtaining connectivityManager in that method
(ConnectivityManager connectivityManager = ...), and if it's null return
NO_NETWORK (same convention used elsewhere), so the method only proceeds to call
getActiveNetwork() and getNetworkCapabilities() when connectivityManager is
non-null.
Comment on lines
88
to
104
| private static String getNetworkTypeFromBelowAndroid24(Context context) { | ||
| ConnectivityManager cm = (ConnectivityManager) context.getSystemService(Context.CONNECTIVITY_SERVICE); | ||
| NetworkInfo activeNetwork = cm.getActiveNetworkInfo(); | ||
| if (activeNetwork == null || !activeNetwork.isConnected()) { | ||
| return NO_NETWORK; | ||
| }else if (activeNetwork.getType() == ConnectivityManager.TYPE_WIFI) { | ||
| return WIFI; | ||
| } else if (activeNetwork.getType() == ConnectivityManager.TYPE_MOBILE) { | ||
| TelephonyManager teleMan = (TelephonyManager) context.getSystemService(Context.TELEPHONY_SERVICE); | ||
| if (ActivityCompat.checkSelfPermission(context, Manifest.permission.READ_PHONE_STATE) == PackageManager.PERMISSION_GRANTED) { | ||
| return getNetworkType(teleMan.getNetworkType()); | ||
| }else{ | ||
| return READ_PHONE_STATE_UNKNOWN_NETWORK; | ||
| } | ||
| } | ||
| return UNKNOWN_NETWORK; | ||
| } |
There was a problem hiding this comment.
Add null check for ConnectivityManager for consistency.
Same issue as getNetworkTypeFromAndroid24 - add a null check for the ConnectivityManager to prevent potential NPE.
🐛 Proposed fix
private static String getNetworkTypeFromBelowAndroid24(Context context) {
ConnectivityManager cm = (ConnectivityManager) context.getSystemService(Context.CONNECTIVITY_SERVICE);
+ if (cm == null) {
+ return UNKNOWN_NETWORK;
+ }
NetworkInfo activeNetwork = cm.getActiveNetworkInfo();
if (activeNetwork == null || !activeNetwork.isConnected()) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private static String getNetworkTypeFromBelowAndroid24(Context context) { | |
| ConnectivityManager cm = (ConnectivityManager) context.getSystemService(Context.CONNECTIVITY_SERVICE); | |
| NetworkInfo activeNetwork = cm.getActiveNetworkInfo(); | |
| if (activeNetwork == null || !activeNetwork.isConnected()) { | |
| return NO_NETWORK; | |
| }else if (activeNetwork.getType() == ConnectivityManager.TYPE_WIFI) { | |
| return WIFI; | |
| } else if (activeNetwork.getType() == ConnectivityManager.TYPE_MOBILE) { | |
| TelephonyManager teleMan = (TelephonyManager) context.getSystemService(Context.TELEPHONY_SERVICE); | |
| if (ActivityCompat.checkSelfPermission(context, Manifest.permission.READ_PHONE_STATE) == PackageManager.PERMISSION_GRANTED) { | |
| return getNetworkType(teleMan.getNetworkType()); | |
| }else{ | |
| return READ_PHONE_STATE_UNKNOWN_NETWORK; | |
| } | |
| } | |
| return UNKNOWN_NETWORK; | |
| } | |
| private static String getNetworkTypeFromBelowAndroid24(Context context) { | |
| ConnectivityManager cm = (ConnectivityManager) context.getSystemService(Context.CONNECTIVITY_SERVICE); | |
| if (cm == null) { | |
| return UNKNOWN_NETWORK; | |
| } | |
| NetworkInfo activeNetwork = cm.getActiveNetworkInfo(); | |
| if (activeNetwork == null || !activeNetwork.isConnected()) { | |
| return NO_NETWORK; | |
| }else if (activeNetwork.getType() == ConnectivityManager.TYPE_WIFI) { | |
| return WIFI; | |
| } else if (activeNetwork.getType() == ConnectivityManager.TYPE_MOBILE) { | |
| TelephonyManager teleMan = (TelephonyManager) context.getSystemService(Context.TELEPHONY_SERVICE); | |
| if (ActivityCompat.checkSelfPermission(context, Manifest.permission.READ_PHONE_STATE) == PackageManager.PERMISSION_GRANTED) { | |
| return getNetworkType(teleMan.getNetworkType()); | |
| }else{ | |
| return READ_PHONE_STATE_UNKNOWN_NETWORK; | |
| } | |
| } | |
| return UNKNOWN_NETWORK; | |
| } |
🤖 Prompt for AI Agents
In @app/src/org/commcare/utils/ConnectivityStatus.java around lines 88 - 104, In
getNetworkTypeFromBelowAndroid24, add a null check for the ConnectivityManager
(cm) immediately after obtaining it and return NO_NETWORK (or the same sentinel
used in getNetworkTypeFromAndroid24) if cm is null to avoid an NPE; i.e., after
"ConnectivityManager cm = (ConnectivityManager) context.getSystemService(...)"
check "if (cm == null) return NO_NETWORK;" before calling
cm.getActiveNetworkInfo(), keeping existing logic for activeNetwork, WIFI,
TYPE_MOBILE, and READ_PHONE_STATE_UNKNOWN_NETWORK intact.
conroy-ricketts
requested changes
Jan 7, 2026
Contributor
conroy-ricketts
left a comment
conroy-ricketts
left a comment
There was a problem hiding this comment.
This will be great to have for additional context from these errors!
shubham1g5
previously approved these changes
Jan 8, 2026
conroy-ricketts
previously approved these changes
Jan 9, 2026
Jignesh-dimagi
force-pushed
the
jignesh/ccct-1991
branch
from
9d17088 to
f5fb0a9
Compare
conroy-ricketts
previously approved these changes
Jan 12, 2026
| return UNKNOWN_NETWORK; | ||
| } | ||
|
|
||
| private static @networkType String getNetworkType(int telephonyManagerNetworkType){ |
Contributor
There was a problem hiding this comment.
@networkType was a nice addition here 👍
shubham1g5
requested changes
Jan 12, 2026
|
|
||
| @StringDef({WIFI,ETHERNET,TWO_G,THREE_G,FOUR_G,FIVE_G,UNKNOWN_NETWORK,READ_PHONE_STATE_UNKNOWN_NETWORK,NO_NETWORK}) | ||
| @Retention(RetentionPolicy.SOURCE) | ||
| public @interface networkType {} |
Contributor
There was a problem hiding this comment.
annotations should start with capital letter - NetworkType
| return (netInfo != null && netInfo.isConnected()); | ||
| } | ||
|
|
||
| public static String getNetworkType(Context context){ |
Contributor
There was a problem hiding this comment.
all the getNetworkType methods should now use the @NetworkType
OrangeAndGreen
previously approved these changes
Jan 12, 2026
Contributor
OrangeAndGreen
left a comment
OrangeAndGreen
left a comment
There was a problem hiding this comment.
Looks good to me pending Shubham's comments.
Jignesh-dimagi
dismissed stale reviews from OrangeAndGreen and conroy-ricketts
via
c4b69a0
shubham1g5
approved these changes
Jan 13, 2026
OrangeAndGreen
approved these changes
Jan 13, 2026
Contributor
Author
|
@damagatchi reset this please |
Contributor
Author
|
@damagatchi retest this please |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Product Description
There are no visible changes
Technical Summary
https://dimagi.atlassian.net/browse/CCCT-1991
As it's not clear what is causing FCM token fetch fail, it has been decided to add more network logs to understand this issue. This PR will not solve the said ticket but will give more info about the network state, which will be used further to take the action accordingly.
QA Plan
As no specific QA required for testing this ticket.
Labels and Review