Update messaging pages when active instead of showing push notification #2947
Conversation
FirebaseMessagingService now checks for active messaging fragments before notifying about messages. When active, service issues a broadcast instead of showing a push notification.
… into dv/notification_suppression
📝 WalkthroughWalkthroughThe pull request introduces changes to three files related to messaging functionality in the CommCare application. The modifications primarily focus on implementing a In The Sequence DiagramsequenceDiagram
participant FMS as FirebaseMessagingService
participant Fragment as ConnectMessageFragment
participant Receiver as BroadcastReceiver
FMS->>FMS: Receive new message
FMS->>Fragment: Broadcast messaging update
Fragment->>Receiver: onReceive() triggered
Receiver->>Fragment: Call refreshUI()
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
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: 0
🧹 Nitpick comments (4)
app/src/org/commcare/fragments/connectMessaging/ConnectMessageChannelListFragment.java (1)
90-95: Add error handling in the BroadcastReceiver.The current implementation might crash if refreshUi() fails. Consider adding try-catch block:
private final BroadcastReceiver updateReceiver = new BroadcastReceiver() { @Override public void onReceive(Context context, Intent intent) { - refreshUi(); + try { + refreshUi(); + } catch (Exception e) { + Logger.log(LogTypes.TYPE_ERROR_SERVER_COMMS, + "Error refreshing UI: " + e.getMessage()); + } } };app/src/org/commcare/fragments/connectMessaging/ConnectMessageFragment.java (1)
90-95: Add error handling in the BroadcastReceiver.Similar to the channel list fragment, add error handling to prevent crashes:
private final BroadcastReceiver updateReceiver = new BroadcastReceiver() { @Override public void onReceive(Context context, Intent intent) { - refreshUi(); + try { + refreshUi(); + } catch (Exception e) { + Logger.log(LogTypes.TYPE_ERROR_SERVER_COMMS, + "Error refreshing message UI: " + e.getMessage()); + } } };app/src/org/commcare/services/CommCareFirebaseMessagingService.java (2)
153-166: Consider extracting notification logic into separate methods.The notification handling logic is becoming complex. Consider refactoring for better maintainability:
-if(ConnectMessageChannelListFragment.isActive || - channelId.equals(ConnectMessageFragment.activeChannel)) { - //Notify active page to update instead of showing notification - Intent broadcastIntent = new Intent(MESSAGING_UPDATE_BROADCAST); - LocalBroadcastManager.getInstance(this).sendBroadcast(broadcastIntent); -} else { - //Show push notification - notificationTitle = getString(notificationTitleId); - notificationText = notificationMessage; - - intent = new Intent(getApplicationContext(), ConnectMessagingActivity.class); - intent.putExtra("action", action); - intent.putExtra(ConnectMessagingMessageRecord.META_MESSAGE_CHANNEL_ID, channelId); -} +if (shouldUpdateUIDirectly(channelId)) { + sendUIUpdateBroadcast(); +} else { + prepareNotificationIntent(channelId, action, notificationTitleId, notificationMessage); +} +private boolean shouldUpdateUIDirectly(String channelId) { + return ConnectMessageChannelListFragment.isActive || + channelId.equals(ConnectMessageFragment.activeChannel); +} +private void sendUIUpdateBroadcast() { + Intent broadcastIntent = new Intent(MESSAGING_UPDATE_BROADCAST); + LocalBroadcastManager.getInstance(this).sendBroadcast(broadcastIntent); +} +private void prepareNotificationIntent(String channelId, String action, + int titleId, String message) { + notificationTitle = getString(titleId); + notificationText = message; + intent = new Intent(getApplicationContext(), ConnectMessagingActivity.class); + intent.putExtra("action", action); + intent.putExtra(ConnectMessagingMessageRecord.META_MESSAGE_CHANNEL_ID, channelId); +}
188-219: Consider implementing a NotificationBuilder pattern.The notification building logic is becoming complex. Consider extracting it into a dedicated builder class:
public class CommCareNotificationBuilder { private final Context context; private final NotificationCompat.Builder builder; public CommCareNotificationBuilder(Context context) { this.context = context; this.builder = new NotificationCompat.Builder(context, CommCareNoficationManager.NOTIFICATION_CHANNEL_PUSH_NOTIFICATIONS_ID); } public CommCareNotificationBuilder setBasicInfo(String title, String text, PendingIntent intent) { builder.setContentTitle(title) .setContentText(text) .setContentIntent(intent) // ... other basic settings return this; } public CommCareNotificationBuilder addPaymentActions(Map<String, String> payloadData) { // Add payment specific actions return this; } public NotificationCompat.Builder build() { return builder; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/src/org/commcare/fragments/connectMessaging/ConnectMessageChannelListFragment.java(2 hunks)app/src/org/commcare/fragments/connectMessaging/ConnectMessageFragment.java(3 hunks)app/src/org/commcare/services/CommCareFirebaseMessagingService.java(8 hunks)
🔇 Additional comments (6)
app/src/org/commcare/fragments/connectMessaging/ConnectMessageChannelListFragment.java (3)
73-77: LGTM! Proper broadcast receiver registration.The broadcast receiver registration is correctly implemented using LocalBroadcastManager, and the API call sequence is appropriate.
83-88: LGTM! Proper cleanup in onPause.The implementation correctly handles cleanup by unregistering the receiver and updating the state flag.
30-30: Consider thread safety and memory management implications of static flag.The static
isActiveflag could lead to race conditions in multi-threaded scenarios and potential memory leaks. Consider using a non-static approach or implementing proper synchronization.Alternative approaches:
- Use ViewModel to manage UI state
- Use LiveData for lifecycle-aware state management
- If static flag is necessary, ensure thread safety:
-public static boolean isActive; +public static volatile boolean isActive;app/src/org/commcare/fragments/connectMessaging/ConnectMessageFragment.java (2)
70-74: LGTM! Proper lifecycle management.The implementation correctly handles the lifecycle events, managing both the channel reference and broadcast receiver registration.
Also applies to: 82-85
34-34: Ensure thread safety for static channel reference.The static
activeChannelvariable could lead to race conditions. Consider using thread-safe alternatives:-public static String activeChannel; +private static final AtomicReference<String> activeChannel = new AtomicReference<>();This would require updating the access patterns:
// Setting: activeChannel.set(channelId) // Getting: activeChannel.get() // Clearing: activeChannel.set(null)✅ Verification successful
Current implementation of
activeChannelis thread-safe for its use caseThe static variable is safely managed:
- Modified only in UI thread through lifecycle methods
- Used as a simple flag for UI state
- No concurrent modification patterns observed
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Find all references to activeChannel echo "=== Searching for activeChannel references ===" rg "activeChannel" -t java -A 3 -B 3 echo -e "\n=== Checking for threading patterns ===" # Look for threading-related code rg "Thread|Runnable|AsyncTask|Handler|ExecutorService" -t java app/src/org/commcare/fragments/connectMessaging/ConnectMessageFragment.java echo -e "\n=== Checking class structure ===" # Get the full class context ast-grep --pattern 'class ConnectMessageFragment { $$$ }'Length of output: 4063
app/src/org/commcare/services/CommCareFirebaseMessagingService.java (1)
43-43: LGTM! Secure broadcast implementation.Good use of LocalBroadcastManager with a well-defined broadcast action constant.
Summary
This change allows the CommCareFirebaseMessagingService to send a broadcast instead of issuing a push notification when the received FCM message is related to messaging and one of the messaging pages is active either the channel list or the message page with the corresponding channel).
Jira ticket
The two related fragments use static variables to indicate their active state easily to the service, and register/unregister for the broadcasts when the fragment resumes/pauses.
cross-request: dimagi/commcare-core#1455
Feature Flag
ConnectID Messaging
Product Description
When the user is on the channel list page or messaging page (for the channel indicated in the FCM message), they will no longer see a push notification for the received message. Instead, they will see the page update immediately with the new channel or message.
PR Checklist
Automated test coverage
No automated tests for Connect code yet.
Safety story
This change is only implemented in the special case of FCM messages where the action is "ccc_message", so push notifications will never be skipped for other FCM messages.