Jignesh/feat/ccct 1816 #3391
Jignesh/feat/ccct 1816 #3391
Conversation
Jignesh-dimagi
requested review from
OrangeAndGreen,
conroy-ricketts and
shubham1g5
📝 WalkthroughWalkthroughThis pull request implements end-to-end notification read-state tracking across the CommCare messaging system. Changes propagate a Sequence DiagramsequenceDiagram
participant FCM as Firebase Cloud<br/>Messaging
participant FMU as FirebaseMessaging<br/>Util
participant DB as Notification<br/>Database
participant CA as Connect<br/>Activity
participant NSW as NotificationSync<br/>Worker
FCM->>FMU: Receive message with payload
FMU->>FMU: Extract/generate NOTIFICATION_ID
FMU->>FMU: Create notification with ID in extras
FMU->>CA: User taps notification<br/>(Intent with NOTIFICATION_ID)
CA->>DB: updateReadStatus(NOTIFICATION_ID, true)
DB->>DB: Mark notification as read
rect rgb(200, 220, 255)
Note over NSW,DB: Next sync cycle
NSW->>NSW: Check showNotification flag
NSW->>NSW: userAlreadyReadThisNotificationPreviously?
NSW->>DB: Query: getNotificationById(NOTIFICATION_ID)
DB-->>NSW: Return read status
alt Notification already read
NSW->>NSW: Skip raiseFCMPushNotification
else Notification not read
NSW->>FCM: Trigger push notification
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✨ 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
🧹 Nitpick comments (1)
app/src/org/commcare/pn/workers/NotificationsSyncWorker.kt (1)
204-214: Simplify redundant null checks.The implementation has redundant null checks and unnecessary non-null assertions:
- Line 205:
containsKeyreturns a non-nullable Boolean, so!!is unnecessary- Lines 205-206: Checking both
containsKeyand!= nullis redundant- Line 209: Uses
!!after already verifying non-null on line 206private fun userAlreadyReadThisNotificationPreviously(): Boolean { - return if (notificationPayload?.containsKey(NOTIFICATION_ID)!! && - notificationPayload?.get(NOTIFICATION_ID) != null - ) { - val notification = - getNotificationById(appContext, notificationPayload?.get(NOTIFICATION_ID)!!) - return notification != null && notification.readStatus - } else { - false - } + val notificationId = notificationPayload?.get(NOTIFICATION_ID) + if (notificationId.isNullOrEmpty()) { + return false + } + val notification = getNotificationById(appContext, notificationId) + return notification?.readStatus == true }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/src/org/commcare/activities/connect/ConnectActivity.java(3 hunks)app/src/org/commcare/activities/connect/ConnectMessagingActivity.java(2 hunks)app/src/org/commcare/pn/workers/NotificationsSyncWorker.kt(2 hunks)app/src/org/commcare/utils/FirebaseMessagingUtil.java(4 hunks)app/src/org/commcare/utils/StringUtils.java(1 hunks)
🧰 Additional context used
🧠 Learnings (17)
📓 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.
Learnt from: Jignesh-dimagi
Repo: dimagi/commcare-android PR: 0
File: :0-0
Timestamp: 2025-05-08T13:40:19.645Z
Learning: PR #3048 introduces a comprehensive messaging system in the Connect feature, implementing secure encryption using AES-GCM for message content, proper channel management with consent flows, and a well-designed UI separation between sent and received messages with real-time notification integration.
📚 Learning: 2025-05-08T11:08:18.530Z
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.
Applied to files:
app/src/org/commcare/activities/connect/ConnectMessagingActivity.javaapp/src/org/commcare/activities/connect/ConnectActivity.javaapp/src/org/commcare/pn/workers/NotificationsSyncWorker.kt
📚 Learning: 2025-05-09T10:57:41.073Z
Learnt from: Jignesh-dimagi
Repo: dimagi/commcare-android PR: 3093
File: app/res/navigation/nav_graph_connect_messaging.xml:41-45
Timestamp: 2025-05-09T10:57:41.073Z
Learning: In the CommCare Android codebase, the navigation graph for Connect messaging (`nav_graph_connect_messaging.xml`) intentionally uses `channel_id` as the argument name in the connectMessageFragment, despite using `channelId` in other parts of the same navigation graph. This naming difference is by design in the refactored code.
Applied to files:
app/src/org/commcare/activities/connect/ConnectMessagingActivity.java
📚 Learning: 2025-05-08T13:40:19.645Z
Learnt from: Jignesh-dimagi
Repo: dimagi/commcare-android PR: 0
File: :0-0
Timestamp: 2025-05-08T13:40:19.645Z
Learning: PR #3048 introduces a comprehensive messaging system in the Connect feature, implementing secure encryption using AES-GCM for message content, proper channel management with consent flows, and a well-designed UI separation between sent and received messages with real-time notification integration.
Applied to files:
app/src/org/commcare/activities/connect/ConnectMessagingActivity.java
📚 Learning: 2025-07-29T14:14:07.954Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3248
File: app/src/org/commcare/adapters/ConnectMessageAdapter.java:0-0
Timestamp: 2025-07-29T14:14:07.954Z
Learning: In the CommCare Android Connect messaging system (ConnectMessageAdapter.java), the team prefers to let getMessage() potentially return null and crash rather than adding defensive null checks. This follows the fail-fast philosophy used throughout the Connect feature to catch programming errors immediately during development.
Applied to files:
app/src/org/commcare/activities/connect/ConnectMessagingActivity.java
📚 Learning: 2025-04-18T20:13:29.655Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3040
File: app/src/org/commcare/android/database/connect/models/ConnectJobDeliveryFlagRecord.java:39-55
Timestamp: 2025-04-18T20:13:29.655Z
Learning: In the CommCare Android Connect feature, the JSON object passed to `ConnectJobDeliveryFlagRecord.fromJson()` method should never be null based on the implementation design.
Applied to files:
app/src/org/commcare/activities/connect/ConnectMessagingActivity.javaapp/src/org/commcare/activities/connect/ConnectActivity.javaapp/src/org/commcare/pn/workers/NotificationsSyncWorker.kt
📚 Learning: 2025-02-19T15:15:01.935Z
Learnt from: pm-dimagi
Repo: dimagi/commcare-android PR: 2956
File: app/src/org/commcare/fragments/connectMessaging/ConnectMessageChannelListFragment.java:58-60
Timestamp: 2025-02-19T15:15:01.935Z
Learning: Error handling for message retrieval in ConnectMessageChannelListFragment's retrieveMessages callback is not required as per user preference.
Applied to files:
app/src/org/commcare/activities/connect/ConnectMessagingActivity.javaapp/src/org/commcare/activities/connect/ConnectActivity.java
📚 Learning: 2025-07-29T14:09:49.805Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3248
File: app/src/org/commcare/fragments/connectMessaging/ConnectMessageFragment.java:0-0
Timestamp: 2025-07-29T14:09:49.805Z
Learning: In the CommCare Android Connect messaging system, message retry logic is implemented at the MessageManager level via MessageManager.sendUnsentMessages(). This method is called automatically when the channel list loads (ConnectMessageChannelListFragment), and it attempts to resend any outgoing messages that are not yet confirmed. Individual message fragments do not need to implement their own retry logic.
Applied to files:
app/src/org/commcare/activities/connect/ConnectMessagingActivity.java
📚 Learning: 2025-05-22T14:26:41.341Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3121
File: app/src/org/commcare/fragments/SelectInstallModeFragment.java:201-205
Timestamp: 2025-05-22T14:26:41.341Z
Learning: In the Connect error handling flow of CommCare Android, error messages are shown once and then automatically cleared because the underlying error record gets deleted after being displayed. This is why there's no need for explicit methods to hide these messages.
Applied to files:
app/src/org/commcare/activities/connect/ConnectMessagingActivity.java
📚 Learning: 2025-05-22T14:28:35.959Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 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/activities/connect/ConnectMessagingActivity.java
📚 Learning: 2025-07-29T14:45:13.470Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3248
File: app/src/org/commcare/connect/MessageManager.java:0-0
Timestamp: 2025-07-29T14:45:13.470Z
Learning: In the CommCare Android Connect messaging system (MessageManager.java), the error handling logic in updateChannelConsent's processFailure callback intentionally attempts to retrieve the channel encryption key even after consent update failure. This behavior was recommended by the server team and the worst case scenario is silently failing to retrieve the encryption key, which is considered acceptable.
Applied to files:
app/src/org/commcare/activities/connect/ConnectMessagingActivity.java
📚 Learning: 2025-06-06T19:54:26.428Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3108
File: app/src/org/commcare/fragments/connect/ConnectDownloadingFragment.java:74-78
Timestamp: 2025-06-06T19:54:26.428Z
Learning: In ConnectDownloadingFragment.java and similar Connect-related code, the team prefers to let "should never happen" scenarios like null app records crash rather than add defensive null checks, following a fail-fast philosophy to catch programming errors during development.
Applied to files:
app/src/org/commcare/activities/connect/ConnectActivity.java
📚 Learning: 2025-07-29T14:10:55.131Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 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.java
📚 Learning: 2025-06-06T19:52:53.173Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3108
File: app/src/org/commcare/fragments/connect/ConnectDeliveryDetailsFragment.java:163-180
Timestamp: 2025-06-06T19:52:53.173Z
Learning: In the CommCare Android Connect feature, database operations like ConnectJobUtils.upsertJob should be allowed to crash rather than being wrapped in try-catch blocks. The team prefers fail-fast behavior for database errors instead of graceful error handling.
Applied to files:
app/src/org/commcare/activities/connect/ConnectActivity.javaapp/src/org/commcare/pn/workers/NotificationsSyncWorker.kt
📚 Learning: 2025-01-21T17:28:09.007Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 2912
File: app/src/org/commcare/android/database/connect/models/ConnectUserRecord.java:66-71
Timestamp: 2025-01-21T17:28:09.007Z
Learning: The ConnectUserRecord class in CommCare Android uses Persisting annotations with sequential indices for field persistence, and MetaField annotations for fields that have corresponding META constants. Fields should be documented with Javadoc comments explaining their purpose, format requirements, and relationships with other fields.
Applied to files:
app/src/org/commcare/activities/connect/ConnectActivity.java
📚 Learning: 2025-01-21T18:19:05.799Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 2912
File: app/src/org/commcare/fragments/connectId/ConnectIdPasswordVerificationFragment.java:214-215
Timestamp: 2025-01-21T18:19:05.799Z
Learning: In ConnectIdPasswordVerificationFragment, when creating a ConnectUserRecord, it's acceptable for payment information (paymentName and paymentPhone) to be empty strings if the server response doesn't include payment info in the CONNECT_PAYMENT_INFO field.
Applied to files:
app/src/org/commcare/activities/connect/ConnectActivity.java
📚 Learning: 2025-01-27T09:08:32.722Z
Learnt from: pm-dimagi
Repo: dimagi/commcare-android PR: 2847
File: app/src/org/commcare/android/database/connect/models/ConnectJobDeliveryRecord.java:86-93
Timestamp: 2025-01-27T09:08:32.722Z
Learning: The JSON fields (status, unitName, slug, entityId, entityName, reason) in ConnectJobDeliveryRecord's fromJson method are guaranteed to be non-null through the application's data contract, except for the 'id' field which is explicitly checked.
Applied to files:
app/src/org/commcare/pn/workers/NotificationsSyncWorker.kt
🧬 Code graph analysis (4)
app/src/org/commcare/activities/connect/ConnectMessagingActivity.java (2)
app/src/org/commcare/connect/ConnectConstants.java (1)
ConnectConstants(8-56)app/src/org/commcare/connect/database/NotificationRecordDatabaseHelper.kt (1)
updateReadStatus(34-38)
app/src/org/commcare/activities/connect/ConnectActivity.java (2)
app/src/org/commcare/connect/ConnectConstants.java (1)
ConnectConstants(8-56)app/src/org/commcare/connect/database/NotificationRecordDatabaseHelper.kt (1)
updateReadStatus(34-38)
app/src/org/commcare/pn/workers/NotificationsSyncWorker.kt (1)
app/src/org/commcare/connect/database/NotificationRecordDatabaseHelper.kt (1)
getNotificationById(23-29)
app/src/org/commcare/utils/FirebaseMessagingUtil.java (2)
app/src/org/commcare/connect/ConnectConstants.java (1)
ConnectConstants(8-56)app/src/org/commcare/utils/StringUtils.java (1)
StringUtils(15-103)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Lint Code Base
🔇 Additional comments (4)
app/src/org/commcare/activities/connect/ConnectMessagingActivity.java (1)
17-17: LGTM: Notification read-state tracking implemented correctly.The implementation properly reads the NOTIFICATION_ID from the intent and marks the notification as read. The null/empty check is appropriate, and the pattern is consistent with ConnectActivity.
Also applies to: 21-21, 79-82
app/src/org/commcare/activities/connect/ConnectActivity.java (1)
4-4: LGTM: Consistent notification read-state handling.The read-state update logic in handleSecureRedirect properly mirrors the implementation in ConnectMessagingActivity, ensuring notifications are marked as read when users interact with redirects.
Also applies to: 41-41, 141-144
app/src/org/commcare/utils/FirebaseMessagingUtil.java (1)
442-444: LGTM: NOTIFICATION_ID properly propagated through intent and notification extras.The Bundle correctly carries NOTIFICATION_ID to both the intent and notification extras, enabling downstream read-state tracking.
Also applies to: 461-462
app/src/org/commcare/pn/workers/NotificationsSyncWorker.kt (1)
199-199: LGTM: Prevents duplicate notification dispatch for already-read notifications.The guard condition correctly prevents re-triggering push notifications that users have already interacted with, completing the end-to-end read-state tracking flow.
| private static void showNotification(Context context, NotificationCompat.Builder notificationBuilder) { | ||
| NotificationManager mNM = (NotificationManager)context.getSystemService(NOTIFICATION_SERVICE); | ||
| mNM.notify(FCM_NOTIFICATION, notificationBuilder.build()); | ||
| mNM.notify(convertToIntegers(notificationBuilder.getExtras().getString(NOTIFICATION_ID)), notificationBuilder.build()); |
There was a problem hiding this comment.
🧩 Analysis chain
Verify null-safety and collision handling for notification IDs.
The convertToIntegers method could return 0 if NOTIFICATION_ID is null or missing from the payload, causing all such notifications to share the same notification ID (0). This would lead to notifications replacing each other in the system tray.
Additionally, the simple summation algorithm in convertToIntegers has a high collision risk—different notification IDs could map to the same integer.
Consider adding a fallback to ensure unique IDs:
-mNM.notify(convertToIntegers(notificationBuilder.getExtras().getString(NOTIFICATION_ID)), notificationBuilder.build());
+String notificationId = notificationBuilder.getExtras().getString(NOTIFICATION_ID);
+int notifId = !TextUtils.isEmpty(notificationId)
+ ? convertToIntegers(notificationId)
+ : (int) System.currentTimeMillis();
+mNM.notify(notifId, notificationBuilder.build());Run the following script to verify that NOTIFICATION_ID is always present in FCM payloads:
🏁 Script executed:
#!/bin/bash
# Search for places where FCM notification payloads are constructed
# to verify NOTIFICATION_ID is always included
rg -nP --type=java -C3 'new\s+FCMMessageData\('
rg -nP --type=java -C3 'putExtra.*NOTIFICATION_ID|put.*NOTIFICATION_ID'Length of output: 2453
I need to examine the convertToIntegers method and the broader context around line 479 to verify the null-safety concerns.
---
🏁 Script executed:
```shell
#!/bin/bash
# Find convertToIntegers method definition
ast-grep --pattern 'int convertToIntegers($_) { $$$ }'
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Get full context around line 479 in FirebaseMessagingUtil.java
sed -n '460,500p' app/src/org/commcare/utils/FirebaseMessagingUtil.javaLength of output: 1451
🏁 Script executed:
#!/bin/bash
# Search for FCMMessageData class definition and getPayloadData method
rg -nP --type=java -A15 'class\s+FCMMessageData'
rg -nP --type=java -A5 'getPayloadData\(\)\s*\{' --type=javaLength of output: 1667
🏁 Script executed:
#!/bin/bash
# Check if NOTIFICATION_ID constant is set anywhere or if it's sourced from remote FCM
rg -nP --type=java 'private.*NOTIFICATION_ID|public.*NOTIFICATION_ID|static.*NOTIFICATION_ID' -A2Length of output: 1822
🏁 Script executed:
#!/bin/bash
# Search for where NOTIFICATION_ID might be missing or optional in data
rg -nP --type=java 'NOTIFICATION_ID' | head -30Length of output: 2391
I need to find the convertToIntegers method and verify the null-safety chain more carefully.
---
🏁 Script executed:
```shell
#!/bin/bash
# Find convertToIntegers method with simpler pattern
rg -nP --type=java 'convertToIntegers' -B2 -A8
Length of output: 3034
🏁 Script executed:
#!/bin/bash
# Check the entire flow in handleGeneralApplicationPushNotification around lines 430-480
sed -n '420,485p' app/src/org/commcare/utils/FirebaseMessagingUtil.javaLength of output: 2788
🏁 Script executed:
#!/bin/bash
# Verify if NOTIFICATION_ID is always included in notificationPayload at lines 193-196
sed -n '190,200p' app/src/org/commcare/utils/FirebaseMessagingUtil.javaLength of output: 625
🏁 Script executed:
#!/bin/bash
# Check if there are any null checks for getPayloadData() results
rg -nP --type=java 'getPayloadData\(\)\.get' -B3 -A3Length of output: 7586
I need to see the complete convertToIntegers algorithm and verify the context around line 196 to fully confirm the issue.
---
🏁 Script executed:
```shell
#!/bin/bash
# Get full convertToIntegers implementation
sed -n '80,100p' app/src/org/commcare/utils/StringUtils.java
Length of output: 743
🏁 Script executed:
#!/bin/bash
# Check context around line 196 to see if NOTIFICATION_ID should be added
sed -n '155,210p' app/src/org/commcare/utils/FirebaseMessagingUtil.javaLength of output: 2823
Confirm notification ID null-safety and collision issues are valid—fix is required.
The concerns raised are substantiated:
-
Null-safety confirmed: Line 196 creates a
notificationPayloadHashMap with onlyNOTIFICATION_TITLEandNOTIFICATION_BODY, omittingNOTIFICATION_ID. When this flows throughbuildNotification()(line 443),fcmMessageData.getPayloadData().get(NOTIFICATION_ID)returnsnull. Thisnullis passed toconvertToIntegers()at line 479, which returns0for null/empty input. All such notifications will share ID0and replace each other in the system tray. -
Collision risk confirmed: The
convertToIntegers()algorithm sums character values (letters: a=1–z=26, digits: 0–9, others: 1). This produces high collision risk—e.g.,"ab"and"c"both equal3.
The suggested fix using System.currentTimeMillis() as a fallback for missing IDs is sound. Additionally, line 196 should ideally include a generated or placeholder NOTIFICATION_ID to prevent the null case entirely.
🤖 Prompt for AI Agents
In app/src/org/commcare/utils/FirebaseMessagingUtil.java around line 479, the
current call to mNM.notify(convertToIntegers(...)) can receive null or colliding
IDs causing all notifications to use ID 0 or collide; fix by ensuring a stable,
unique integer ID is used: when building the notification payload (around line
196) always include a generated NOTIFICATION_ID (e.g., a timestamp-based
placeholder) so null is not propagated, and update convertToIntegers to treat
null/empty input by returning a fallback of (int)(System.currentTimeMillis() &
Integer.MAX_VALUE) or otherwise derive a stable int from the provided string
(e.g., a robust hash or parsing with fallback) so notifications no longer share
ID 0 or collide frequently.
There was a problem hiding this comment.
Null-safety confirmed: Line 196 is for showing the general notification with only notification payload and we are here only considering notification with data payload so we can ignore it.
Collision risk confirmed: Its required that code should generate same integer value for given same notification_id so adding system time will not help here.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for where data payloads are constructed in FCM context
# to verify NOTIFICATION_ID is always included
echo "=== Searching for FCMMessageData construction with data payloads ==="
rg -nP --type=java -C5 'new FCMMessageData\(' | head -80
echo -e "\n=== Searching for where NOTIFICATION_ID is put into data payloads ==="
rg -nP --type=java -C3 '\.put.*NOTIFICATION_ID' | head -50
echo -e "\n=== Check if NOTIFICATION_ID is validated/required anywhere ==="
rg -nP --type=java 'containsKey.*NOTIFICATION_ID|isEmpty.*NOTIFICATION_ID' -C2
Length of output: 3566
@Jignesh-dimagi, thank you for the clarification!
You're absolutely right about:
- Line 196 - It's only for notification-only payloads without data, so that path doesn't need NOTIFICATION_ID
- Deterministic mapping requirement - The same
notification_idmust always produce the same integer so updated notifications replace previous ones in the system tray
However, there's still a potential null-safety issue in the data payload path:
At line 443, fcmMessageData.getPayloadData().get(NOTIFICATION_ID) could return null if the backend doesn't send NOTIFICATION_ID in the data payload. This null would propagate to line 479 where convertToIntegers(null) returns 0, causing all such notifications to collide.
Recommendations:
-
Better deterministic hash: Consider using
String.hashCode()instead ofconvertToIntegers()for better collision resistance while maintaining determinism:private static void showNotification(Context context, NotificationCompat.Builder notificationBuilder) { NotificationManager mNM = (NotificationManager)context.getSystemService(NOTIFICATION_SERVICE); String notificationId = notificationBuilder.getExtras().getString(NOTIFICATION_ID); int notifId = !TextUtils.isEmpty(notificationId) ? notificationId.hashCode() : FCM_NOTIFICATION; // fallback to constant mNM.notify(notifId, notificationBuilder.build()); }
-
Add defensive null check: Even if the backend is expected to always send
NOTIFICATION_ID, adding a fallback would prevent edge cases:- Use a constant ID for missing cases
- Or log an error and skip the notification
Question: Does the backend/server guarantee that all data payloads include NOTIFICATION_ID? If not, should we add validation or a fallback strategy?
🧬 Code Graph Analysis Results
[FirebaseMessagingUtil.java: showNotificationFromNotificationPayload]
/**
* Show notification from notification payload
*
* @param context
* @param payloadNotification
* @return
*/
private static boolean showNotificationFromNotificationPayload(Context context, RemoteMessage.Notification payloadNotification,boolean showNotification) {
if (payloadNotification != null &&
!Strings.isEmptyOrWhitespace(payloadNotification.getTitle()) &&
!Strings.isEmptyOrWhitespace(payloadNotification.getBody()) &&
showNotification) {
Map<String, String> notificationPayload = new HashMap<>();
notificationPayload.put(NOTIFICATION_TITLE, payloadNotification.getTitle());
notificationPayload.put(NOTIFICATION_BODY, payloadNotification.getBody());
handleGeneralApplicationPushNotification(context, new FCMMessageData(notificationPayload),showNotification);
return true;
}
return false;
}
[FirebaseMessagingUtil.java: buildNotification]
/**
* Build notification
*
* @param context
* @param intent
* @param fcmMessageData
* @return NotificationCompat.Builder
*/
private static NotificationCompat.Builder buildNotification(Context context, Intent intent, FCMMessageData fcmMessageData) {
intent.addFlags(Intent.FLAG_ACTIVITY_CLEAR_TOP | Intent.FLAG_ACTIVITY_SINGLE_TOP |
Intent.FLAG_ACTIVITY_NEW_TASK);
int flags = Build.VERSION.SDK_INT >= Build.VERSION_CODES.S
? PendingIntent.FLAG_UPDATE_CURRENT | PendingIntent.FLAG_IMMUTABLE
: PendingIntent.FLAG_UPDATE_CURRENT;
Bundle bundleExtras = new Bundle();
bundleExtras.putString(NOTIFICATION_ID, fcmMessageData.getPayloadData().get(NOTIFICATION_ID));
intent.putExtra(NOTIFICATION_ID, fcmMessageData.getPayloadData().get(NOTIFICATION_ID));
PendingIntent contentIntent = PendingIntent.getActivity(context, 0, intent, flags);
if (Strings.isEmptyOrWhitespace(fcmMessageData.getNotificationTitle()) && Strings.isEmptyOrWhitespace(fcmMessageData.getNotificationText())) {
Logger.exception("Empty push notification",
new Throwable(String.format("Empty notification for action '%s'", fcmMessageData.getAction())));
}
NotificationCompat.Builder fcmNotification = new NotificationCompat.Builder(context,
fcmMessageData.getNotificationChannel())
.setContentTitle(fcmMessageData.getNotificationTitle())
.setContentText(fcmMessageData.getNotificationText())
.setContentIntent(contentIntent)
.setAutoCancel(true)
.setSmallIcon(R.drawable.commcare_actionbar_logo)
.setPriority(fcmMessageData.getPriority())
.setWhen(System.currentTimeMillis())
.setExtras(bundleExtras);
if (fcmMessageData.getLargeIcon() != null) {
fcmNotification.setLargeIcon(fcmMessageData.getLargeIcon());
}
return fcmNotification;
}
[FirebaseMessagingUtil.java: showNotification]
/**
* Show notification
*
* @param context
* @param notificationBuilder
*/
private static void showNotification(Context context, NotificationCompat.Builder notificationBuilder) {
NotificationManager mNM = (NotificationManager)context.getSystemService(NOTIFICATION_SERVICE);
mNM.notify(convertToIntegers(notificationBuilder.getExtras().getString(NOTIFICATION_ID)), notificationBuilder.build());
}
🧠 Learnings used
Learnt from: shubham1g5
Repo: dimagi/commcare-android PR: 2955
File: app/src/org/commcare/models/database/user/models/CommCareEntityStorageCache.java:135-143
Timestamp: 2025-02-18T17:42:31.516Z
Learning: In CommCare Android, record IDs are expected to be numeric and should fail fast (crash) if a non-numeric ID is encountered, rather than handling the NumberFormatException.
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 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: 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.
There was a problem hiding this comment.
+1 for suggesting hashcode
There was a problem hiding this comment.
@Jignesh-dimagi, great! I'm glad the hashCode() approach works for you.
While String.hashCode() provides better collision resistance than simple character summation and maintains deterministic behavior (same string → same integer), collisions can still occur—for example, "Aa" and "BB" both produce hashCode 2112. However, with an ideal hash function, you'd need about 77,163 objects to reach a 50% collision probability, which is much better than the current convertToIntegers() approach.
Here's the recommended implementation for line 479:
private static void showNotification(Context context, NotificationCompat.Builder notificationBuilder) {
NotificationManager mNM = (NotificationManager)context.getSystemService(NOTIFICATION_SERVICE);
String notificationId = notificationBuilder.getExtras().getString(NOTIFICATION_ID);
int notifId = !TextUtils.isEmpty(notificationId)
? notificationId.hashCode()
: FCM_NOTIFICATION; // fallback to constant for missing IDs
mNM.notify(notifId, notificationBuilder.build());
}Key benefits:
- ✅ Deterministic: same
notification_idalways produces same integer - ✅ Better collision resistance than
convertToIntegers() - ✅ Null-safe with fallback to constant
Question: Should I also update the convertToIntegers() usages in other files (ConnectActivity.java, ConnectMessagingActivity.java as shown in the AI summary), or are those being handled separately?
🧠 Learnings used
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 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: shubham1g5
Repo: dimagi/commcare-android PR: 2955
File: app/src/org/commcare/models/database/user/models/CommCareEntityStorageCache.java:135-143
Timestamp: 2025-02-18T17:42:31.516Z
Learning: In CommCare Android, record IDs are expected to be numeric and should fail fast (crash) if a non-numeric ID is encountered, rather than handling the NumberFormatException.
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.
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3248
File: app/src/org/commcare/adapters/ConnectMessageAdapter.java:0-0
Timestamp: 2025-07-29T14:14:07.954Z
Learning: In the CommCare Android Connect messaging system (ConnectMessageAdapter.java), the team prefers to let getMessage() potentially return null and crash rather than adding defensive null checks. This follows the fail-fast philosophy used throughout the Connect feature to catch programming errors immediately during development.
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3108
File: app/src/org/commcare/fragments/connect/ConnectDownloadingFragment.java:74-78
Timestamp: 2025-06-06T19:54:26.428Z
Learning: In ConnectDownloadingFragment.java and similar Connect-related code, the team prefers to let "should never happen" scenarios like null app records crash rather than add defensive null checks, following a fail-fast philosophy to catch programming errors during development.
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 2912
File: app/src/org/commcare/fragments/connect/ConnectPaymentSetupFragment.java:61-66
Timestamp: 2025-01-21T17:29:58.014Z
Learning: In the CommCare Android app, for non-critical convenience features like phone number auto-population, exceptions should be logged but fail silently when there's a manual fallback available. This approach prevents app crashes while maintaining the ability to debug issues through logs.
Learnt from: shubham1g5
Repo: dimagi/commcare-android PR: 3116
File: app/src/org/commcare/fragments/connectId/PersonalIdPhotoCaptureFragment.java:0-0
Timestamp: 2025-05-19T15:36:34.511Z
Learning: In the CommCare Android codebase, a "fail fast" approach is preferred for internal encoding/decoding operations where both sides are controlled by the application. This helps surface bugs immediately rather than masking them with graceful error handling.
| public static int convertToIntegers(String input) { | ||
| int returnInt = 0; | ||
| if (input == null || input.isEmpty()) { | ||
| return returnInt; | ||
| } | ||
|
|
||
| try { | ||
| for (int i = 0; i < input.length(); i++) { | ||
| char ch = input.charAt(i); | ||
| if (Character.isAlphabetic(ch)) { | ||
| char lowerCaseCh = Character.toLowerCase(ch); | ||
| returnInt += lowerCaseCh - 'a' + 1; // 'a' is 1, 'b' is 2, etc. | ||
| } else if (Character.isDigit(ch)) { | ||
| returnInt += ch - '0'; | ||
| } else { | ||
| returnInt += 1; // any random number | ||
| } | ||
| } | ||
| } catch (Exception ignore) { | ||
|
|
||
| } | ||
| return returnInt; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
High collision risk with simple summation algorithm.
The convertToIntegers method uses character-value summation, which produces identical results for many different inputs (e.g., "ab" = "ba" = "aaa" = 3). This creates a high risk of notification ID collisions, causing notifications to replace each other.
Additionally:
- Line 95: The comment "any random number" is misleading—it's actually the constant value 1
- Line 98: Silently catching all exceptions could hide bugs
- The method lacks documentation explaining its purpose, collision properties, and acceptable use cases
Consider using a hash-based approach for better distribution:
public static int convertToIntegers(String input) {
- int returnInt = 0;
if (input == null || input.isEmpty()) {
- return returnInt;
+ return 0;
}
- try {
- for (int i = 0; i < input.length(); i++) {
- char ch = input.charAt(i);
- if (Character.isAlphabetic(ch)) {
- char lowerCaseCh = Character.toLowerCase(ch);
- returnInt += lowerCaseCh - 'a' + 1; // 'a' is 1, 'b' is 2, etc.
- } else if (Character.isDigit(ch)) {
- returnInt += ch - '0';
- } else {
- returnInt += 1; // any random number
- }
- }
- } catch (Exception ignore) {
-
- }
- return returnInt;
+ // Use hashCode() for better distribution and lower collision probability
+ return Math.abs(input.hashCode());
}Run this script to check for potential collisions in existing notification IDs:
🏁 Script executed:
#!/bin/bash
# Find sample notification IDs in the codebase to test for collisions
rg -nP --type=java --type=kotlin 'NOTIFICATION_ID|notification_id' -A2 -B2 | grep -E '(String|"[^"]+")' Length of output: 3511
---
🏁 Script executed:
```shell
#!/bin/bash
# Find all usages of convertToIntegers method
rg -nP 'convertToIntegers' --type=java --type=kotlin -B2 -A2
Length of output: 1500
🏁 Script executed:
#!/bin/bash
# Search for FCM message data handling to understand notification ID format
rg -nP 'getPayloadData|fcmMessageData' --type=java --type=kotlin -B3 -A3Length of output: 36507
🏁 Script executed:
#!/bin/bash
# Check for any documentation or tests related to convertToIntegers
rg -nP 'convertToIntegers' --type=java --type=kotlin -B5 -A10Length of output: 4466
---
🏁 Script executed:
```shell
#!/bin/bash
# Find where NOTIFICATION_ID is set or created in payloads
rg -nP 'NOTIFICATION_ID.*=' --type=java --type=kotlin -B2 -A2 | head -100
Length of output: 2306
🏁 Script executed:
#!/bin/bash
# Check FCMMessageData class to understand payload handling
cat app/src/org/commcare/services/FCMMessageData.java | head -100Length of output: 3163
🏁 Script executed:
#!/bin/bash
# Search for tests related to notification ID conversion or FCM message handling
fd -e java -e kt | xargs grep -l "convertToIntegers\|NOTIFICATION_ID" | grep -i testLength of output: 0
---
🏁 Script executed:
```shell
#!/bin/bash
# Look for any integration tests, mock FCM payloads, or fixtures that show NOTIFICATION_ID format
fd -e java -e kt -e json | xargs grep -l "notification_id" | grep -E '(test|fixture|mock|sample)'
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for backend or server-side code that generates FCM payloads
rg -nP '"notification_id"|notification_id.*:' --type=java --type=kt --type=json -B3 -A3 | head -150Length of output: 26
🏁 Script executed:
#!/bin/bash
# Check if there are any docs or comments explaining the purpose of convertToIntegers
rg -nP 'notification.*id|convert.*integer' --type=java --type=kt -i -B5 -A5 | grep -i 'purpose\|use\|doc' | head -50Length of output: 26
Fix silent exception handling and use hash-based notification ID conversion to prevent collisions.
The convertToIntegers method has two critical issues per the codebase patterns:
-
Silent exception catching violates fail-fast philosophy: Per learnings from this codebase, numeric payloads (like
NOTIFICATION_ID) should fail-fast on invalid data. Line 98'scatch (Exception ignore)contradicts this pattern. -
High collision probability with summation algorithm: Character summation produces identical results for different inputs ("ab" = "ba" = "aaa" = 3), causing Android notifications to replace each other when IDs collide.
Additionally:
- Line 95 comment "any random number" is misleading—it's actually the constant value 1
- Method lacks documentation explaining its purpose and collision properties
Replace with proper hash-based approach:
public static int convertToIntegers(String input) {
if (input == null || input.isEmpty()) {
return 0;
}
return Math.abs(input.hashCode());
}🤖 Prompt for AI Agents
In app/src/org/commcare/utils/StringUtils.java around lines 80 to 102, replace
the current summation-based implementation and silent catch with a fail-fast,
hash-based conversion to avoid collisions and hidden errors: remove the
try/catch that swallows exceptions, return 0 for null/empty input, and compute a
non-negative ID using Math.abs(input.hashCode()); update the method Javadoc to
explain the purpose (producing a stable int for notification IDs), note that
hash collisions are still possible but far less likely than character-sum, and
delete/replace the misleading "any random number" comment; keep the method
signature the same and do not suppress exceptions silently.
conroy-ricketts
left a comment
conroy-ricketts
left a comment
There was a problem hiding this comment.
Code Rabbit has some interesting comments that are worth looking at
| if (showNotification && !userAlreadyReadThisNotificationPreviously()) { | ||
| FirebaseMessagingUtil.handleNotification(appContext, notificationPayload, null, true) | ||
| } | ||
| } | ||
|
|
||
| private fun userAlreadyReadThisNotificationPreviously(): Boolean { | ||
| return if (notificationPayload?.containsKey(NOTIFICATION_ID)!! && | ||
| notificationPayload?.get(NOTIFICATION_ID) != null | ||
| ) { | ||
| val notification = | ||
| getNotificationById(appContext, notificationPayload?.get(NOTIFICATION_ID)!!) | ||
| return notification != null && notification.readStatus | ||
| } else { | ||
| false | ||
| } | ||
| } |
There was a problem hiding this comment.
Optional: We could shorten this function name
| if (showNotification && !userAlreadyReadThisNotificationPreviously()) { | |
| FirebaseMessagingUtil.handleNotification(appContext, notificationPayload, null, true) | |
| } | |
| } | |
| private fun userAlreadyReadThisNotificationPreviously(): Boolean { | |
| return if (notificationPayload?.containsKey(NOTIFICATION_ID)!! && | |
| notificationPayload?.get(NOTIFICATION_ID) != null | |
| ) { | |
| val notification = | |
| getNotificationById(appContext, notificationPayload?.get(NOTIFICATION_ID)!!) | |
| return notification != null && notification.readStatus | |
| } else { | |
| false | |
| } | |
| } | |
| if (showNotification && !notificationRead()) { | |
| FirebaseMessagingUtil.handleNotification(appContext, notificationPayload, null, true) | |
| } | |
| } | |
| private fun notificationRead(): Boolean { | |
| return if (notificationPayload?.containsKey(NOTIFICATION_ID)!! && | |
| notificationPayload?.get(NOTIFICATION_ID) != null | |
| ) { | |
| val notification = | |
| getNotificationById(appContext, notificationPayload?.get(NOTIFICATION_ID)!!) | |
| return notification != null && notification.readStatus | |
| } else { | |
| false | |
| } | |
| } |
There was a problem hiding this comment.
+1 for a shortened name here, given it's boolean, I would start with is like isNotificationRead
| public static int convertToIntegers(String input) { | ||
| int returnInt = 0; | ||
| if (input == null || input.isEmpty()) { | ||
| return returnInt; | ||
| } | ||
|
|
||
| try { | ||
| for (int i = 0; i < input.length(); i++) { | ||
| char ch = input.charAt(i); | ||
| if (Character.isAlphabetic(ch)) { | ||
| char lowerCaseCh = Character.toLowerCase(ch); | ||
| returnInt += lowerCaseCh - 'a' + 1; // 'a' is 1, 'b' is 2, etc. | ||
| } else if (Character.isDigit(ch)) { | ||
| returnInt += ch - '0'; | ||
| } else { | ||
| returnInt += 1; // any random number | ||
| } | ||
| } | ||
| } catch (Exception ignore) { | ||
|
|
||
| } | ||
| return returnInt; | ||
| } |
There was a problem hiding this comment.
Have we explored other options for generating unique IDs?
For example, here is a quick example I grabbed from ChatGPT using AtomicIntegers:
import java.util.concurrent.atomic.AtomicInteger;
public class UniqueIdGenerator {
private static final AtomicInteger counter = new AtomicInteger(0);
public static int getUniqueId() {
return counter.incrementAndGet();
}
public static void main(String[] args) {
System.out.println(getUniqueId()); // 1
System.out.println(getUniqueId()); // 2
System.out.println(getUniqueId()); // 3
}
}
There was a problem hiding this comment.
@conroy-ricketts Here aim is not to generate the random unique number but to convert notification_id to integer and it should be same every time for same notification_id so that system doesn't show the same notification twice on tray. The adapted method follows particular calculations making sure that integer generated is unique (not true theoretically but given long notification_id compared to number of notification generated, chances are very very high).
| private static void showNotification(Context context, NotificationCompat.Builder notificationBuilder) { | ||
| NotificationManager mNM = (NotificationManager)context.getSystemService(NOTIFICATION_SERVICE); | ||
| mNM.notify(FCM_NOTIFICATION, notificationBuilder.build()); | ||
| mNM.notify(convertToIntegers(notificationBuilder.getExtras().getString(NOTIFICATION_ID)), notificationBuilder.build()); |
There was a problem hiding this comment.
@Jignesh-dimagi Is notification id alphanumeric ? can't we simply use Integer.parseInt()
There was a problem hiding this comment.
@shubham1g5 Yes its alphanumeric with - symbol so cannot use directly Integer.parseInt().
| .setPriority(fcmMessageData.getPriority()) | ||
| .setWhen(System.currentTimeMillis()); | ||
| .setWhen(System.currentTimeMillis()) | ||
| .setExtras(bundleExtras); |
There was a problem hiding this comment.
when is this extras used ?
There was a problem hiding this comment.
To generate the unique notification id.
There was a problem hiding this comment.
we can't directly pass that as an argument to showNotification ?
There was a problem hiding this comment.
Its being used across many functions so better to have extras as can be utilized directly without need to pass as function parameters everytime.
There was a problem hiding this comment.
I do not think we want to use exttas to pass arguments between methods as a design pattern. Bundles or extras are only really meant be used for cross-process comms and data passing as it has a extra overhead of serializing and unserializing data from a byte stream
There was a problem hiding this comment.
@shubham1g5 Agree, it's mainly used for media type notification for maintaining the interaction for sessions in notification. But thought that single string will not cause overhead and it was giving nit code at that expense.
Product Description
https://dimagi.atlassian.net/browse/CCCT-1816
Labels and Review