Handles push notification clicks whenever app is in foreground, background and killed #3307
Conversation
Jignesh-dimagi
requested review from
OrangeAndGreen,
avazirna and
shubham1g5
and removed request for
OrangeAndGreen and
shubham1g5
📝 WalkthroughWalkthrough
Sequence Diagram(s)sequenceDiagram
autonumber
participant FCM as FCM
participant Svc as CommCareFirebaseMessagingService
participant Util as FirebaseMessagingUtil
participant Android as Android Notification Manager
participant Act as Target Activity
Note over Svc,Util: Changed: showNotification flag threading
FCM->>Svc: onMessageReceived(remoteMessage)
Svc->>Util: handleNotification(ctx, data, notification, true)
alt Valid PN data
Util->>Act: Create Intent for target activity
Util->>Android: showNotification(builder, true)
Android-->>Svc: System posts notification
else Empty or unrecognized
Util-->>Svc: null (no Intent)
Util->>Android: showNotification(..., true) or ignore
end
sequenceDiagram
autonumber
participant User as User/App Launch
participant DA as DispatchActivity
participant Util as FirebaseMessagingUtil
participant Act as PN Target Activity
Note over DA,Util: New PN deep-link flow without posting a notification
User->>DA: onResume()
DA->>DA: dispatch()
DA->>Util: getIntentForPNIfAny(ctx, getIntent())
Util->>Util: Extract extras -> dataPayload
Util->>Util: handleNotification(ctx, dataPayload, null, false)
alt Intent derivable
Util-->>DA: Intent (no notification shown)
DA->>Act: startActivity(intent)
else No PN intent
Util-->>DA: null
DA-->>User: Continue normal flow
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
app/build.gradle (1)
112-112: Unify Firebase dependency versions via BoM or verify compatibilityThe Firebase libraries in
app/build.gradleuse mixed version numbers, which can lead to untested combinations at runtime. Please consolidate these versions by adopting the Firebase BoM or, if you choose to keep explicit versions, verify compatibility across API levels 24 – 34 and push notification flows (foreground, background, killed).• Locations of mismatched Firebase SDK versions:
– Line 111:firebase-analytics:20.1.2
– Line 112:firebase-messaging:24.0.0
– Line 113:firebase-perf:21.0.1
– Line 114:firebase-crashlytics:18.3.7
– Line 162:firebase-auth:22.3.0• Suggested BoM-based refactor (in
app/build.gradle):dependencies { - implementation 'com.google.firebase:firebase-analytics:20.1.2' - implementation 'com.google.firebase:firebase-messaging:24.0.0' - implementation 'com.google.firebase:firebase-perf:21.0.1' - implementation 'com.google.firebase:firebase-crashlytics:18.3.7' - implementation "com.google.firebase:firebase-auth:22.3.0" + // Import the Firebase BoM + implementation platform('com.google.firebase:firebase-bom:32.1.0') + // Declare Firebase dependencies without versions + implementation 'com.google.firebase:firebase-analytics' + implementation 'com.google.firebase:firebase-messaging' + implementation 'com.google.firebase:firebase-perf' + implementation 'com.google.firebase:firebase-crashlytics' + implementation 'com.google.firebase:firebase-auth' }If you opt not to use BoM, ensure each explicitly versioned SDK is tested together on devices running Android 24 – 34, covering all notification delivery scenarios.
app/src/org/commcare/activities/connect/ConnectActivity.java (1)
13-13: Use a single emptiness utility; prefer existing Guava Strings import.We already import Guava’s Strings; avoid bringing in TextUtils just for an emptiness check and keep consistency.
-import android.text.TextUtils; +// (remove import) - String opportunityId = getIntent().getStringExtra(ConnectConstants.OPPORTUNITY_ID); - if(!TextUtils.isEmpty(opportunityId)){ - job = ConnectJobUtils.getCompositeJob(this, Integer.parseInt(opportunityId)); - } + String opportunityId = getIntent().getStringExtra(ConnectConstants.OPPORTUNITY_ID); + if (!Strings.isNullOrEmpty(opportunityId)) { + job = ConnectJobUtils.getCompositeJob(this, Integer.parseInt(opportunityId)); + }If GO_TO_JOB_STATUS can be set with an empty/“0”/non-numeric opportunity_id, Objects.requireNonNull(job) in handleInfoRedirect will crash. Please confirm upstream payloads always include a valid, positive integer ID when GO_TO_JOB_STATUS is true.
Also applies to: 83-86
app/src/org/commcare/utils/FirebaseMessagingUtil.java (5)
175-185: Skip notification construction when suppressed.When showNotification is false, we still construct FCMMessageData and call the general handler, which builds a NotificationCompat.Builder and may log “Empty push notification”. Early-return to avoid unnecessary work and logs.
-private static boolean showNotificationFromNotificationPayload(Context context, RemoteMessage.Notification payloadNotification,boolean showNotification) { +private static boolean showNotificationFromNotificationPayload(Context context, RemoteMessage.Notification payloadNotification, boolean showNotification) { if (payloadNotification != null && !Strings.isEmptyOrWhitespace(payloadNotification.getTitle()) && !Strings.isEmptyOrWhitespace(payloadNotification.getBody())) { - Map<String, String> notificationPayload = new HashMap<>(); - notificationPayload.put(NOTIFICATION_TITLE, payloadNotification.getTitle()); - notificationPayload.put(NOTIFICATION_BODY, payloadNotification.getBody()); - handleGeneralApplicationPushNotification(context, new FCMMessageData(notificationPayload),showNotification); + if (!showNotification) { + return true; + } + Map<String, String> notificationPayload = new HashMap<>(); + notificationPayload.put(NOTIFICATION_TITLE, payloadNotification.getTitle()); + notificationPayload.put(NOTIFICATION_BODY, payloadNotification.getBody()); + handleGeneralApplicationPushNotification(context, new FCMMessageData(notificationPayload), true); return true; } return false; }
220-249: Make PendingIntent requestCodes unique per payment to avoid action collisions.Using static request codes 1 and 2 can cause PendingIntent re-use across different notifications. Derive request codes from payment/opportunity IDs.
- PendingIntent yesPendingIntent = PendingIntent.getBroadcast(context, 1, + int base = (fcmMessageData.getPayloadData().get(PAYMENT_ID) != null + ? fcmMessageData.getPayloadData().get(PAYMENT_ID).hashCode() : 0); + PendingIntent yesPendingIntent = PendingIntent.getBroadcast(context, base, yesIntent, flags); ... - PendingIntent noPendingIntent = PendingIntent.getBroadcast(context, 2, + PendingIntent noPendingIntent = PendingIntent.getBroadcast(context, base + 1, noIntent, flags);
335-343: Defer large icon decode when notifications are suppressed.BitmapFactory.decodeResource runs regardless of showNotification. Guard expensive work behind showNotification and only when we’ll actually notify.
- fcmMessageData.setPriority(NotificationCompat.PRIORITY_MAX); - fcmMessageData.setLargeIcon(BitmapFactory.decodeResource(context.getResources(), R.drawable.ic_connect_message_large)); + fcmMessageData.setPriority(NotificationCompat.PRIORITY_MAX); ... - if (intent != null) { - showNotification(context, buildNotification(context, intent, fcmMessageData),showNotification); + if (intent != null) { + if (showNotification) { + fcmMessageData.setLargeIcon( + BitmapFactory.decodeResource(context.getResources(), R.drawable.ic_connect_message_large)); + } + showNotification(context, buildNotification(context, intent, fcmMessageData), showNotification); }Also applies to: 379-381
10-13: Avoid android.util.Log in production paths; use consistent logging.This file mixes Logger and Log. Prefer a single approach; either guard Log calls with BuildConfig.DEBUG or switch to Logger to avoid leaking info in release.
149-166: Add Javadoc for showNotification and provide a 3-arg overload for backward compatibilityWe’ve verified that all internal calls to handleNotification now use the new four-parameter API (no remaining three-arg invocations), so adding the overload will only benefit external callers. Please:
• In app/src/org/commcare/utils/FirebaseMessagingUtil.java around the existing method Javadoc (lines ~149–166), append:
/** * This is generic function used by both CommCareFirebaseMessagingService and launcher activity. * * @param context - application context * @param dataPayload - This will be data payload when calling from CommCareFirebaseMessagingService or will * be intent data payload when calling from launcher activity * @param notificationPayload - This will be notification payload when calling from CommCareFirebaseMessagingService and null when calling from launcher activity + * @param showNotification - When false, suppresses posting a system notification; only computes the target intent. * @return Intent - if need for launcher activity to start the activity */• Immediately after the existing four-arg method (e.g. after line 166), add:
public static Intent handleNotification(Context context, Map<String, String> dataPayload, RemoteMessage.Notification notificationPayload) { return handleNotification(context, dataPayload, notificationPayload, true); }These changes document the new flag and restore the original three-parameter signature for any callers that haven’t yet been updated.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
app/build.gradle(1 hunks)app/src/org/commcare/activities/DispatchActivity.java(3 hunks)app/src/org/commcare/activities/connect/ConnectActivity.java(2 hunks)app/src/org/commcare/fragments/connect/ConnectUnlockFragment.java(2 hunks)app/src/org/commcare/services/CommCareFirebaseMessagingService.java(1 hunks)app/src/org/commcare/utils/FirebaseMessagingUtil.java(14 hunks)
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: shubham1g5
PR: dimagi/commcare-android#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.
📚 Learning: 2025-07-29T14:10:55.131Z
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#3248
File: app/src/org/commcare/fragments/connect/ConnectUnlockFragment.java:0-0
Timestamp: 2025-07-29T14:10:55.131Z
Learning: In ConnectUnlockFragment.java, opportunityId values are expected to always contain valid integer strings. Integer.parseInt() should be allowed to throw NumberFormatException if invalid data is encountered, following the fail-fast philosophy used throughout the Connect feature for data contract violations.
Applied to files:
app/src/org/commcare/activities/connect/ConnectActivity.javaapp/src/org/commcare/fragments/connect/ConnectUnlockFragment.java
📚 Learning: 2025-05-08T11:08:18.530Z
Learnt from: shubham1g5
PR: dimagi/commcare-android#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.
Applied to files:
app/src/org/commcare/activities/connect/ConnectActivity.java
📚 Learning: 2025-06-04T19:17:21.213Z
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#3108
File: app/src/org/commcare/fragments/connect/ConnectUnlockFragment.java:62-64
Timestamp: 2025-06-04T19:17:21.213Z
Learning: In ConnectUnlockFragment.java, the user prefers to let getArguments() potentially throw NullPointerException rather than adding null checks, as the arguments are required for proper navigation flow and their absence indicates a programming error that should fail fast.
Applied to files:
app/src/org/commcare/fragments/connect/ConnectUnlockFragment.java
📚 Learning: 2025-02-04T21:29:29.594Z
Learnt from: pm-dimagi
PR: dimagi/commcare-android#2949
File: app/src/org/commcare/fragments/connectId/ConnectIdBiometricConfigFragment.java:235-236
Timestamp: 2025-02-04T21:29:29.594Z
Learning: The empty performPasswordUnlock method in ConnectIdBiometricConfigFragment is intentionally left empty and should not be flagged in reviews.
Applied to files:
app/src/org/commcare/fragments/connect/ConnectUnlockFragment.java
📚 Learning: 2025-05-22T14:28:35.959Z
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#3121
File: app/src/org/commcare/activities/CommCareSetupActivity.java:360-364
Timestamp: 2025-05-22T14:28:35.959Z
Learning: In CommCareSetupActivity.java, the call to installFragment.showConnectErrorMessage() after fragment transactions is intentionally unguarded with null checks. This follows the app's design pattern where critical error paths prefer immediate crashes over silent failures, making potential issues immediately visible during development rather than hiding them with defensive programming.
Applied to files:
app/src/org/commcare/fragments/connect/ConnectUnlockFragment.java
📚 Learning: 2025-05-22T14:26:41.341Z
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#3121
File: app/src/org/commcare/fragments/SelectInstallModeFragment.java:201-205
Timestamp: 2025-05-22T14:26:41.341Z
Learning: In SelectInstallModeFragment.java, the showConnectErrorMessage method intentionally omits null checks because it's called at a specific point in the startup flow where UI is guaranteed to be loaded. It's designed to crash if activity or view is null to make potential issues immediately visible rather than hiding them with defensive programming.
Applied to files:
app/src/org/commcare/fragments/connect/ConnectUnlockFragment.java
📚 Learning: 2025-03-10T08:16:59.436Z
Learnt from: shubham1g5
PR: dimagi/commcare-android#2949
File: app/src/org/commcare/fragments/connectId/ConnectIDSecondaryPhoneNumber.java:58-59
Timestamp: 2025-03-10T08:16:59.436Z
Learning: All fragments using view binding should implement proper cleanup in onDestroyView() by setting binding to null to prevent memory leaks.
Applied to files:
app/src/org/commcare/fragments/connect/ConnectUnlockFragment.java
🧬 Code graph analysis (5)
app/src/org/commcare/activities/DispatchActivity.java (1)
app/src/org/commcare/utils/FirebaseMessagingUtil.java (1)
FirebaseMessagingUtil(66-489)
app/src/org/commcare/activities/connect/ConnectActivity.java (1)
app/src/org/commcare/connect/ConnectConstants.java (1)
ConnectConstants(8-48)
app/src/org/commcare/services/CommCareFirebaseMessagingService.java (1)
app/src/org/commcare/utils/FirebaseMessagingUtil.java (1)
FirebaseMessagingUtil(66-489)
app/src/org/commcare/fragments/connect/ConnectUnlockFragment.java (1)
app/src/org/commcare/connect/PersonalIdManager.java (1)
PersonalIdManager(64-621)
app/src/org/commcare/utils/FirebaseMessagingUtil.java (4)
app/src/org/commcare/services/FCMMessageData.java (1)
FCMMessageData(26-188)app/src/org/commcare/google/services/analytics/FirebaseAnalyticsUtil.java (1)
FirebaseAnalyticsUtil(41-568)app/src/org/commcare/connect/database/ConnectUserDatabaseUtil.java (1)
ConnectUserDatabaseUtil(10-56)app/src/org/commcare/connect/ConnectConstants.java (1)
ConnectConstants(8-48)
🔇 Additional comments (4)
app/src/org/commcare/services/CommCareFirebaseMessagingService.java (1)
58-58: LGTM: updated call-site matches new handleNotification signature.Passing showNotification=true here is correct for service-driven notifications.
app/src/org/commcare/utils/FirebaseMessagingUtil.java (3)
448-452: LGTM: central notification gate.Early-return is simple and effective to suppress system notifications consistently.
194-210: Side-effect on UI thread: confirm turning on Connect access here.handleCCCActionPushNotification turns on Connect access even when showNotification is false (i.e., when we’re only computing a navigation intent). This writes to DB and runs on the main thread via DispatchActivity.onResume path.
Would you prefer moving ConnectUserDatabaseUtil.turnOnConnectAccess(context) to the actual screen entry point (e.g., ConnectActivity) or offloading to a background thread to avoid jank? I can propose a minimal refactor if desired.
318-324: Behavior check: null return for general app PN in “intent extraction” path.handleGeneralApplicationPushNotification always returns null. Consequently, getIntentForPNIfAny won’t navigate for general (non-CCC) PNs tapped from the tray. Confirm that this is intentional.
If not intended, we can return the DispatchActivity intent here when showNotification == false.
|
|
||
| private void checkIfAnyPNIntentPresent(){ | ||
| Intent pnIntent = FirebaseMessagingUtil.getIntentForPNIfAny(this,getIntent()); | ||
| if(pnIntent!=null){ | ||
| startActivity(pnIntent); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Avoid double-navigation: gate PN navigation when another activity launch is in-flight.
checkIfAnyPNIntentPresent() runs after dispatch(), which often startActivity/ForResult. Without guards, PN navigation can overlay login/home/other flows. Add simple checks before launching PN target.
private void checkIfAnyPNIntentPresent(){
- Intent pnIntent = FirebaseMessagingUtil.getIntentForPNIfAny(this,getIntent());
- if(pnIntent!=null){
- startActivity(pnIntent);
- }
+ if (waitingForActivityResultFromLogin || isFinishing() || isChangingConfigurations()) {
+ return;
+ }
+ Intent pnIntent = FirebaseMessagingUtil.getIntentForPNIfAny(this, getIntent());
+ if (pnIntent != null) {
+ startActivity(pnIntent);
+ // Optional: finish() to keep a clean back stack when entering via PN.
+ // finish();
+ }
} } else {
dispatch();
- checkIfAnyPNIntentPresent();
+ if (!waitingForActivityResultFromLogin && !isFinishing()) {
+ checkIfAnyPNIntentPresent();
+ }
}Confirm that FirebaseMessagingUtil.getIntentForPNIfAny(...) calling intent.replaceExtras(new Bundle()) won’t remove extras later relied upon by DispatchActivity on subsequent resumes.
Also applies to: 143-143
🏁 Script executed:
#!/bin/bash
set -e
# Locate the FirebaseMessagingUtil class and its getIntentForPNIfAny method
rg -nC3 'getIntentForPNIfAny' -g '*.java'
# Search for replaceExtras usages to see if the original Intent is mutated
rg -nC3 'replaceExtras' -g '*.java'Length of output: 2048
Prevent unintentional clearing of original Intent extras
Before gating PN navigation, note that FirebaseMessagingUtil.getIntentForPNIfAny(...) mutates the incoming Intent by calling
intent.replaceExtras(new Bundle()); // clear all intentThis clears every extra on the original Intent, which DispatchActivity and downstream code may rely on after resume. To avoid breaking other navigation flows:
• In getIntentForPNIfAny, operate on a copy rather than the passed-in Intent:
Intent safeIntent = new Intent(intent);
safeIntent.replaceExtras(new Bundle());
// …process safeIntent…
return handleNotification(context, dataPayload, safeIntent, false);• Or, extract and preserve needed extras from the original Intent before clearing.
• In DispatchActivity, continue passing only the copy to getIntentForPNIfAny, e.g.
checkPN(new Intent(getIntent()));Ensuring that the original Intent’s extras remain intact will prevent subtle navigation bugs when the activity resumes.
🤖 Prompt for AI Agents
In app/src/org/commcare/activities/DispatchActivity.java around lines 108-115,
the call to FirebaseMessagingUtil.getIntentForPNIfAny(this, getIntent()) can
cause the original Intent's extras to be cleared (since that method mutates the
passed Intent); change the call to operate on a copy of the incoming Intent
(e.g., pass new Intent(getIntent())) or otherwise preserve required extras
before calling, or update FirebaseMessagingUtil to make and mutate a copy
internally so the original Intent remains intact and downstream code continues
to receive its extras.
| public void onViewCreated(@NonNull View view, @Nullable Bundle savedInstanceState) { | ||
| super.onViewCreated(view, savedInstanceState); | ||
|
|
||
| new Handler(Looper.getMainLooper()).postDelayed(new Runnable() { | ||
| @Override | ||
| public void run() { | ||
| PersonalIdManager.getInstance().unlockConnect((CommCareActivity<?>)requireActivity(), success -> { | ||
| if (success) { | ||
| retrieveOpportunities(); | ||
| } else { | ||
| requireActivity().finish(); | ||
| } | ||
| }); | ||
| } | ||
| }); | ||
| }, 1000); // delayMillis is the delay in milliseconds | ||
|
|
||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Lifecycle leak/crash risk: delayed unlock Runnable isn’t lifecycle-aware.
Posting via a raw Handler without removing callbacks can run after the view is destroyed, causing IllegalStateException (requireActivity/requireContext) and leaking the Fragment. Tie the delay to the View lifecycle and remove callbacks; also null out binding in onDestroyView.
- new Handler(Looper.getMainLooper()).postDelayed(new Runnable() {
- @Override
- public void run() {
- PersonalIdManager.getInstance().unlockConnect((CommCareActivity<?>)requireActivity(), success -> {
- if (success) {
- retrieveOpportunities();
- } else {
- requireActivity().finish();
- }
- });
- }
- }, 1000); // delayMillis is the delay in milliseconds
+ // Post to the view's message queue so callbacks are scoped to the view lifecycle
+ binding.getRoot().postDelayed(unlockRunnable, 1000);Add these outside the changed hunk:
// Field
private final Runnable unlockRunnable = new Runnable() {
@Override
public void run() {
PersonalIdManager.getInstance().unlockConnect((CommCareActivity<?>) requireActivity(), success -> {
if (success) {
retrieveOpportunities();
} else {
requireActivity().finish();
}
});
}
};
@Override
public void onDestroyView() {
// Ensure pending callbacks are removed and binding is released
binding.getRoot().removeCallbacks(unlockRunnable);
binding = null;
super.onDestroyView();
}
There was a problem hiding this comment.
@shubham1g5 Thank you for the confirmation! The suggested lifecycle-aware approach will prevent potential crashes and memory leaks when the fragment is destroyed before the delayed unlock completes.
🧠 Learnings used
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#3108
File: app/src/org/commcare/fragments/connect/ConnectUnlockFragment.java:62-64
Timestamp: 2025-06-04T19:17:21.213Z
Learning: In ConnectUnlockFragment.java, the user prefers to let getArguments() potentially throw NullPointerException rather than adding null checks, as the arguments are required for proper navigation flow and their absence indicates a programming error that should fail fast.
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#3248
File: app/src/org/commcare/fragments/connect/ConnectUnlockFragment.java:0-0
Timestamp: 2025-07-29T14:10:55.131Z
Learning: In ConnectUnlockFragment.java, opportunityId values are expected to always contain valid integer strings. Integer.parseInt() should be allowed to throw NumberFormatException if invalid data is encountered, following the fail-fast philosophy used throughout the Connect feature for data contract violations.
Learnt from: pm-dimagi
PR: dimagi/commcare-android#2949
File: app/src/org/commcare/fragments/connectId/ConnectIdBiometricConfigFragment.java:235-236
Timestamp: 2025-02-04T21:29:29.594Z
Learning: The empty performPasswordUnlock method in ConnectIdBiometricConfigFragment is intentionally left empty and should not be flagged in reviews.
Learnt from: OrangeAndGreen
PR: dimagi/commcare-android#3121
File: app/src/org/commcare/activities/CommCareSetupActivity.java:360-364
Timestamp: 2025-05-22T14:28:35.959Z
Learning: In CommCareSetupActivity.java, the call to installFragment.showConnectErrorMessage() after fragment transactions is intentionally unguarded with null checks. This follows the app's design pattern where critical error paths prefer immediate crashes over silent failures, making potential issues immediately visible during development rather than hiding them with defensive programming.
Learnt from: shubham1g5
PR: dimagi/commcare-android#2949
File: app/src/org/commcare/fragments/connectId/ConnectIDSecondaryPhoneNumber.java:58-59
Timestamp: 2025-03-10T08:16:59.436Z
Learning: All fragments using view binding should implement proper cleanup in onDestroyView() by setting binding to null to prevent memory leaks.
Learnt from: shubham1g5
PR: dimagi/commcare-android#2949
File: app/src/org/commcare/fragments/connectId/ConnectIDSecondaryPhoneNumber.java:58-59
Timestamp: 2025-03-10T08:16:59.436Z
Learning: All fragments using view binding should implement proper cleanup in onDestroyView() by setting binding to null to prevent memory leaks.
Learnt from: shubham1g5
PR: dimagi/commcare-android#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.
| public static Intent getIntentForPNIfAny(Context context,Intent intent){ | ||
| Log.e("FBMsgUtil",intent.toString()); | ||
| if(intent!=null && intent.getExtras()!=null && intent.hasExtra("action")){ | ||
| Log.e("FBMsgUtil","intents are NOOOOOOOT null "); | ||
| Map<String, String> dataPayload = new HashMap<>(); | ||
| if (intent.getExtras() != null) { | ||
| for (String key : intent.getExtras().keySet()) { | ||
| String value = intent.getExtras().getString(key); | ||
| dataPayload.put(key, value); | ||
| } | ||
| } | ||
|
|
||
| Log.e("FBMsgUtil",dataPayload.toString()); | ||
|
|
||
| intent.replaceExtras(new Bundle()); // clear all intent | ||
|
|
||
| return handleNotification(context,dataPayload,null,false); | ||
|
|
||
| }else{ | ||
| Log.e("FBMsgUtil","intents are null "); | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Critical: PII/log leakage, destructive intent mutation, and non-String extras.
- Logging intent and extras at error level will leak user identifiers and server-provided data to logcat.
- intent.replaceExtras(new Bundle()) wipes all extras, potentially breaking other DispatchActivity flows.
- extras.getString(key) drops non-String extras (common in FCM), silently losing data.
Apply this fix:
-public static Intent getIntentForPNIfAny(Context context,Intent intent){
- Log.e("FBMsgUtil",intent.toString());
- if(intent!=null && intent.getExtras()!=null && intent.hasExtra("action")){
- Log.e("FBMsgUtil","intents are NOOOOOOOT null ");
- Map<String, String> dataPayload = new HashMap<>();
- if (intent.getExtras() != null) {
- for (String key : intent.getExtras().keySet()) {
- String value = intent.getExtras().getString(key);
- dataPayload.put(key, value);
- }
- }
-
- Log.e("FBMsgUtil",dataPayload.toString());
-
- intent.replaceExtras(new Bundle()); // clear all intent
-
- return handleNotification(context,dataPayload,null,false);
- }else{
- Log.e("FBMsgUtil","intents are null ");
- }
- return null;
-}
+public static Intent getIntentForPNIfAny(Context context, Intent intent) {
+ if (intent == null) {
+ return null;
+ }
+ Bundle extras = intent.getExtras();
+ if (extras == null || !intent.hasExtra("action")) {
+ return null;
+ }
+ // Avoid logging PII-rich payloads. Keep debug-only breadcrumb if needed.
+ if (BuildConfig.DEBUG) {
+ Log.d("FBMsgUtil", "PN intent keys: " + extras.keySet());
+ }
+ Map<String, String> dataPayload = new HashMap<>();
+ for (String key : extras.keySet()) {
+ Object v = extras.get(key);
+ if (v != null) {
+ dataPayload.put(key, String.valueOf(v));
+ }
+ }
+ // Remove only the PN routing key to prevent re-processing; keep other extras intact.
+ intent.removeExtra("action");
+ return handleNotification(context, dataPayload, null, false);
+}After patch, grep to ensure no remaining “NOOOOOOOT” debug strings and no Log.e with “FBMsgUtil”:
🏁 Script executed:
#!/bin/bash
rg -n 'NOOOOOOOT|Log\.e\("FBMsgUtil"' --type=javaLength of output: 498
Action required: Remove remaining Log.e calls and debug strings in FirebaseMessagingUtil
The grep results indicate that the original error-level logs and “NOOOOOOOT” string are still present in FirebaseMessagingUtil.java, so the patch needs correction:
• app/src/org/commcare/utils/FirebaseMessagingUtil.java:466
Log.e("FBMsgUtil", intent.toString());
• app/src/org/commcare/utils/FirebaseMessagingUtil.java:468
Log.e("FBMsgUtil","intents are NOOOOOOOT null ");
• app/src/org/commcare/utils/FirebaseMessagingUtil.java:477
Log.e("FBMsgUtil", dataPayload.toString());
• app/src/org/commcare/utils/FirebaseMessagingUtil.java:484
Log.e("FBMsgUtil","intents are null ");
Please update the method to remove or demote these logs (e.g. use BuildConfig.DEBUG and Log.d/Log.v), and eliminate the “NOOOOOOOT” string. For example:
- Log.e("FBMsgUtil", intent.toString());
+ if (BuildConfig.DEBUG) Log.d("FBMsgUtil", "PN intent keys: " + intent.getExtras().keySet());
...
- Log.e("FBMsgUtil","intents are NOOOOOOOT null ");
+ // debug-only breadcrumb removed
...
- Log.e("FBMsgUtil", dataPayload.toString());
+ if (BuildConfig.DEBUG) Log.d("FBMsgUtil", "PN payload: " + dataPayload);
...
- Log.e("FBMsgUtil","intents are null ");
+ // no-op: intent is nullEnsure no remaining Log.e("FBMsgUtil" or “NOOOOOOOT” occurrences before merging.
🤖 Prompt for AI Agents
In app/src/org/commcare/utils/FirebaseMessagingUtil.java around lines 465 to
487, remove all remaining Log.e calls and the “NOOOOOOOT” debug string; instead
either delete those logging lines or demote them to debug-level guarded by
BuildConfig.DEBUG (e.g. if (BuildConfig.DEBUG) Log.d(TAG, "...")), replace the
“NOOOOOOOT” text with a concise message if kept, and ensure no Log.e("FBMsgUtil"
...) or “NOOOOOOOT” occurrences remain before merging.
| } | ||
|
|
||
| public static Intent getIntentForPNIfAny(Context context,Intent intent){ | ||
| Log.e("FBMsgUtil",intent.toString()); |
There was a problem hiding this comment.
can remove the local logs
| public void onViewCreated(@NonNull View view, @Nullable Bundle savedInstanceState) { | ||
| super.onViewCreated(view, savedInstanceState); | ||
|
|
||
| new Handler(Looper.getMainLooper()).postDelayed(new Runnable() { | ||
| @Override | ||
| public void run() { | ||
| PersonalIdManager.getInstance().unlockConnect((CommCareActivity<?>)requireActivity(), success -> { | ||
| if (success) { | ||
| retrieveOpportunities(); | ||
| } else { | ||
| requireActivity().finish(); | ||
| } | ||
| }); | ||
| } | ||
| }); | ||
| }, 1000); // delayMillis is the delay in milliseconds | ||
|
|
||
|
|
| new Handler(Looper.getMainLooper()).postDelayed(new Runnable() { | ||
| @Override | ||
| public void run() { | ||
| PersonalIdManager.getInstance().unlockConnect((CommCareActivity<?>)requireActivity(), success -> { | ||
| if (success) { | ||
| retrieveOpportunities(); | ||
| } else { | ||
| requireActivity().finish(); | ||
| } | ||
| }); | ||
| } | ||
| }); | ||
| }, 1000); // delayMillis is the delay in milliseconds |
There was a problem hiding this comment.
what is the reasoning to move the unlock to another thread ?
There was a problem hiding this comment.
App was crashing with following error java.lang.IllegalStateException: FragmentManager is already executing transactions. It was requiring a separate thread to solve this issue.
There was a problem hiding this comment.
@Jignesh-dimagi Have you tested it without giving delay and handler?. I think there will be no need of delay and handler now as we are using onViewCreated()
There was a problem hiding this comment.
Putting without handler in onViewCreated() doesn't resolve the issue for me, but It did resolve on just using a handler without delay
There was a problem hiding this comment.
| finish(); | ||
| } else { | ||
| dispatch(); | ||
| checkIfAnyPNIntentPresent(); |
There was a problem hiding this comment.
- this should be part of
dispatch()itself as that's where we unify the logic to open an screen from here. - Also dispatch already open different activities based on app state, so this logic would not really get called say if dispath opened up Login Screen first. Think we need to check all those different app states and see what we want to do when user clicks on notification there.
There was a problem hiding this comment.
Here, the app is getting opened as user has clicked on the push notification. So whenever app opens, it will follow the logic in dispatch() depending upon the state of application and will open the activity. Now, upon that second activity will be opened for push notification logic e.g. connect activity.
I have intentionally not kept as a part of dispatch() so that whenever user clicks back on connect activity, it will get close and user lands on login activity.
There was a problem hiding this comment.
@Jignesh-dimagi I am lacking enough context on the changes here to be able to review this. Mostly I am not clear on how checkIfAnyPNIntentPresent has been tested, will you be able to outline a few workflows as Safety Story in PR description that you have tested in regard to this.
For eg. we should be playing out scenarios like below and specifying the product behaviour with this PR -
i) user clicks on a notification when user is logged out of personal ID
ii) user clicks on a notification when user is logged out of CommCare App
iii) An external app launch CommCare with an intent action (that doesn't correspond to a push notiifcation)
i) As we have talked, we need to take care in the coming new push notification changes but let me put the code to check it |
That'll be great to have in this PR.
Would you be able to confirm if you have tested that, I don't really seeing how we are distinguishing between the 2 kinds of intents here. |
--> added the check for personalId login check here. This will make sure that no push notification is shown if user is not logged in. --> App will confirm that it will not interfere with other intents of external app launch by having logic defined in this class |
|
@shubham1g5 Please review again. |
@Jignesh-dimagi Why would not app receive any intent if user is logged out ?
Can you link me to these parameters in code ? |
It starts from here and follows the code ahead |
|
@Jignesh-dimagi thanks, although still not clear on how this happens -
|
| launchLoginScreen(); | ||
| } | ||
| } | ||
| checkIfAnyPNIntentPresent(); |
There was a problem hiding this comment.
@Jignesh-dimagi Suppose as part of dispatch we end up calling launchLoginScreen above (i.e user was not logged into a CC app when they clicked on notificaion), it seems like we will first start the LoginActivity and then on top of it, will start the screen related to the notification intent. This is a problematic behaviour as we are calling startActivity back to back for 2 different activities and think will screw up the navigation stack.
My sense is that we should instead be calling checkIfAnyPNIntentPresent first thing in the dispatch (for personal ID notifications) and ignore the rest of dispatch handling if we redirect user to a screen as part of notification handling. Also think we need to be separating out the Personal ID notification handling here from the other CC notification handling. The idea being, if user is logged into Personal ID and user click on a personal ID notification, we don't need to depend on any other state being managed in dispatch and can safely direct user to the related personal ID screen.
Although, if user has clicked on a notification which is a CommCare notification, we do need to depend on other handling here (like making sure user is logged in first into CC app, before directing user to a screen that should only appear after a user login into the CC App.
Also open to other ideas if you have thoughts on how to manage this state more seamlessly.
There was a problem hiding this comment.
@shubham1g5
Regarding checkIfAnyPNIntentPresent first thing in the dispatch I was of the view that app should continue doing as per the logic while showing the notification activity on top of it. Now, removed it and put at the first place, it will continue logic after coming back from notification activity.
For your second concern, that notification should be handled separately for CCC and CC. It is like that only. This method is responsible for separating out the CCC, data sync, generic push notification (it has only notification but no data) and CC (if any in future). If push notification is CCC, currently it is checking whether user is PersonalId logged in or not.
There was a problem hiding this comment.
Yes, although what I meant was that we need to put in more thought on when it's safe to trigger each notification in conjunction with the checks that are happening in dispatch i.e we need to determine when each notification is safe to trigger in the dispatch codeflow. For example CCC notifications are fine to act on the first thing in dispatch without checking on other CommCareApp state code in dispatch but the other SYNC notification is not as that depends on CommCareApp and user being logged into the app.
As such I think the desired code flow here should be -
- Check for CCC notification and any general notificaions the first thing and exit from dispatch if we successfully redirect user as part of this handling, otherwise continue with rest of the dispatch logic.
- Just before this check, add a check for
Syncnotification and handle those (as we have made sure at this point that a login is active inside the app)
This only is true for CCC notifications though, if we call the |
|
@shubham1g5 Your action required again as merged master. Thanks. |
|
@damagatchi retest please |
1 similar comment
|
@damagatchi retest please |
|
@damagatchi retest this please |
|
@shubham1g5 Please review again, solved the test failing. |
Technical Summary
https://dimagi.atlassian.net/browse/CCCT-1365
QA Plan
QA can test the application by sending different types of push notification and clicking on these push notifications
Labels and Review