Markdown in messaging #2948
Markdown in messaging #2948
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
📝 WalkthroughWalkthroughThis pull request introduces changes to multiple files in the CommCare messaging system, focusing on enhancing message handling and notification mechanisms. The modifications span across three key files: The changes primarily involve implementing markdown support for chat messages, adding broadcast receivers to manage messaging updates, and refining the logic for displaying Firebase Cloud Messaging (FCM) notifications. New static variables and methods have been introduced to track active messaging states and handle real-time updates. The implementation allows for more dynamic message rendering and improves the way notifications are managed based on the current user context within the messaging interface. Sequence DiagramsequenceDiagram
participant User
participant Fragment
participant BroadcastReceiver
participant FirebaseMessagingService
User->>Fragment: Enters Messaging Screen
Fragment->>BroadcastReceiver: Register Receiver
FirebaseMessagingService->>BroadcastReceiver: Send Update Broadcast
BroadcastReceiver->>Fragment: Trigger UI Refresh
Fragment->>Fragment: Update Message Display
Possibly related PRs
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: 3
🧹 Nitpick comments (6)
app/src/org/commcare/fragments/connectMessaging/ConnectMessageChannelListFragment.java (2)
30-30: Consider alternatives to static fragment state.Using a static boolean to track fragment state can lead to memory leaks and issues during configuration changes (e.g., screen rotation). Consider using ViewModel or saved instance state to manage fragment lifecycle state.
90-95: Add error handling to the BroadcastReceiver.The current implementation could be more robust with proper error handling:
- Validate the incoming intent
- Handle potential exceptions in
refreshUi()private final BroadcastReceiver updateReceiver = new BroadcastReceiver() { @Override public void onReceive(Context context, Intent intent) { - refreshUi(); + if (intent != null && + CommCareFirebaseMessagingService.MESSAGING_UPDATE_BROADCAST.equals(intent.getAction())) { + try { + refreshUi(); + } catch (Exception e) { + // Log the error and possibly show a user-friendly message + e.printStackTrace(); + } + } } };app/src/org/commcare/fragments/connectMessaging/ConnectMessageFragment.java (1)
90-95: Add error handling to BroadcastReceiver.The current implementation lacks error handling. Consider adding try-catch block and intent validation.
private final BroadcastReceiver updateReceiver = new BroadcastReceiver() { @Override public void onReceive(Context context, Intent intent) { + if (intent == null || !CommCareFirebaseMessagingService.MESSAGING_UPDATE_BROADCAST.equals(intent.getAction())) { + return; + } + try { refreshUi(); + } catch (Exception e) { + Logger.log(LogTypes.TYPE_ERROR_SERVER_COMMS, "Error refreshing messaging UI: " + e.getMessage()); + } } };app/src/org/commcare/services/CommCareFirebaseMessagingService.java (1)
178-196: Review notification priority setting.Setting all notifications to HIGH priority (PRIORITY_HIGH) might lead to notification fatigue. Consider implementing priority levels based on message type or importance.
- .setPriority(NotificationCompat.PRIORITY_HIGH) + int priority = isUrgentMessage(payloadData) + ? NotificationCompat.PRIORITY_HIGH + : NotificationCompat.PRIORITY_DEFAULT; + .setPriority(priority)Add this helper method:
private boolean isUrgentMessage(Map<String, String> payloadData) { // Determine urgency based on message type, content, or metadata return payloadData.containsKey("urgent") || "payment".equals(payloadData.get("type")); }app/src/org/commcare/adapters/ConnectMessageAdapter.java (2)
42-44: Add defensive programming measures for message handling.While the markdown implementation looks good, consider these improvements for robustness:
- Add null check for
chat.getMessage()- Add error handling for markdown parsing failures
- Consider reusing SpannableStringBuilder instances for better performance
public void bind(ConnectMessageChatData chat) { + String message = chat.getMessage(); + if (message == null) { + message = ""; + } + try { SpannableStringBuilder builder = new SpannableStringBuilder(); - builder.append(chat.getMessage()); + builder.append(message); MarkupUtil.setMarkdown(binding.tvChatMessage, builder, new SpannableStringBuilder()); + } catch (Exception e) { + // Fallback to plain text if markdown parsing fails + binding.tvChatMessage.setText(message); }
59-61: Consider extracting common message formatting logic.The message formatting code is duplicated between
LeftViewHolderandRightViewHolder. Consider extracting this into a shared utility method to improve maintainability.+ private static void formatMessage(TextView messageView, String message) { + if (message == null) { + message = ""; + } + try { + SpannableStringBuilder builder = new SpannableStringBuilder(); + builder.append(message); + MarkupUtil.setMarkdown(messageView, builder, new SpannableStringBuilder()); + } catch (Exception e) { + messageView.setText(message); + } + } public static class LeftViewHolder extends RecyclerView.ViewHolder { // ... public void bind(ConnectMessageChatData chat) { - SpannableStringBuilder builder = new SpannableStringBuilder(); - builder.append(chat.getMessage()); - MarkupUtil.setMarkdown(binding.tvChatMessage, builder, new SpannableStringBuilder()); + formatMessage(binding.tvChatMessage, chat.getMessage()); binding.tvUserName.setText(DateUtils.formatDateTime(chat.getTimestamp(), DateUtils.FORMAT_HUMAN_READABLE_SHORT)); } } public static class RightViewHolder extends RecyclerView.ViewHolder { // ... public void bind(ConnectMessageChatData chat) { - SpannableStringBuilder builder = new SpannableStringBuilder(); - builder.append(chat.getMessage()); - MarkupUtil.setMarkdown(binding.tvChatMessage, builder, new SpannableStringBuilder()); + formatMessage(binding.tvChatMessage, chat.getMessage()); binding.tvUserName.setText(DateUtils.formatDateTime(chat.getTimestamp(), DateUtils.FORMAT_HUMAN_READABLE_SHORT)); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/src/org/commcare/adapters/ConnectMessageAdapter.java(4 hunks)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 (2)
83-88: LGTM! Proper cleanup in onPause.The broadcast receiver is correctly unregistered and the fragment state is cleaned up.
Line range hint
1-120: Consider adding tests for the messaging update mechanism.While the PR objectives mention that automated tests aren't available for Connect code, it would be valuable to add tests for this new messaging update mechanism to ensure reliable delivery of markdown-formatted messages.
Key test scenarios to consider:
- Broadcast receiver registration/unregistration
- UI updates in response to broadcasts
- Error handling scenarios
app/src/org/commcare/fragments/connectMessaging/ConnectMessageFragment.java (1)
70-74: LGTM! Proper lifecycle management.The registration and unregistration of the BroadcastReceiver in onResume/onPause is correctly implemented, following Android best practices.
Also applies to: 82-85
app/src/org/commcare/services/CommCareFirebaseMessagingService.java (1)
10-10: LGTM! Well-defined constant and appropriate import.The MESSAGING_UPDATE_BROADCAST constant follows naming conventions and LocalBroadcastManager is the appropriate choice for internal app communication.
Also applies to: 43-43
app/src/org/commcare/adapters/ConnectMessageAdapter.java (2)
3-3: LGTM! Required imports added for markdown support.The new imports for
SpannableStringBuilderandMarkupUtilare appropriate for implementing markdown rendering functionality.Also applies to: 13-13
42-44: Verify markdown parsing security and edge cases.Let's verify the implementation of
MarkupUtil.setMarkdownto ensure it properly handles:
- Security concerns (e.g., XSS prevention)
- Edge cases in markdown syntax
- Performance with large messages
Also applies to: 59-61
✅ Verification successful
Implementation verified - markdown parsing is secure and robust
The markdown implementation uses the Markwon library with proper input sanitization and security measures:
- Input validation and character encoding handling
- Secure file handling with proper permissions
- HTML content sanitization
- Null-safety and edge case handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for MarkupUtil implementation and tests echo "Searching for MarkupUtil implementation..." rg -A 10 "class MarkupUtil" echo "Searching for MarkupUtil tests..." rg -l "class.*MarkupUtil.*Test" echo "Checking for security-related comments or annotations..." rg -i "security|xss|sanitize|escape" "$(fd -t f "MarkupUtil.java")"Length of output: 1291
Script:
#!/bin/bash # Get full implementation of MarkupUtil echo "Full MarkupUtil implementation:" cat app/src/org/commcare/utils/MarkupUtil.java echo -e "\nChecking for markdown library dependencies:" rg -i "markdown" build.gradle echo -e "\nChecking for input validation patterns:" ast-grep --pattern 'setMarkdown($_,$_,$_) { $$$ }'Length of output: 9016
| isActive = true; | ||
|
|
||
| LocalBroadcastManager.getInstance(requireContext()).registerReceiver(updateReceiver, | ||
| new IntentFilter(CommCareFirebaseMessagingService.MESSAGING_UPDATE_BROADCAST)); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid duplicate message retrieval calls.
MessageManager.retrieveMessages() is called in both onCreateView and onResume. This could lead to unnecessary network calls and potential race conditions. Consider consolidating these calls or implementing a debounce mechanism.
@Override
public void onResume() {
super.onResume();
isActive = true;
LocalBroadcastManager.getInstance(requireContext()).registerReceiver(updateReceiver,
new IntentFilter(CommCareFirebaseMessagingService.MESSAGING_UPDATE_BROADCAST));
- MessageManager.retrieveMessages(requireActivity(), success -> {
- refreshUi();
- });
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| isActive = true; | |
| LocalBroadcastManager.getInstance(requireContext()).registerReceiver(updateReceiver, | |
| new IntentFilter(CommCareFirebaseMessagingService.MESSAGING_UPDATE_BROADCAST)); | |
| isActive = true; | |
| LocalBroadcastManager.getInstance(requireContext()).registerReceiver(updateReceiver, | |
| new IntentFilter(CommCareFirebaseMessagingService.MESSAGING_UPDATE_BROADCAST)); | |
|
|
||
| public class ConnectMessageFragment extends Fragment { | ||
|
|
||
| public static String activeChannel; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider alternative to static variable for tracking active channel.
Using a static variable to track the active channel state could lead to memory leaks and issues during configuration changes. Consider using a ViewModel or other lifecycle-aware component to manage this state.
Here's a suggested implementation using ViewModel:
class ConnectMessageViewModel : ViewModel() {
private val _activeChannel = MutableLiveData<String?>()
val activeChannel: LiveData<String?> = _activeChannel
fun setActiveChannel(channelId: String?) {
_activeChannel.value = channelId
}
}Usage in fragment:
-public static String activeChannel;
+private ConnectMessageViewModel viewModel;
@Override
public void onCreate(@Nullable Bundle savedInstanceState) {
super.onCreate(savedInstanceState);
+ viewModel = new ViewModelProvider(this).get(ConnectMessageViewModel.class);
}| 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 { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Include channel information in the broadcast intent.
The broadcast intent should include the channel ID to allow receivers to verify if they need to update their UI.
if(ConnectMessageChannelListFragment.isActive ||
channelId.equals(ConnectMessageFragment.activeChannel)) {
//Notify active page to update instead of showing notification
Intent broadcastIntent = new Intent(MESSAGING_UPDATE_BROADCAST);
+ broadcastIntent.putExtra("channel_id", channelId);
+ broadcastIntent.putExtra("update_type", isMessage ? "message" : "channel");
LocalBroadcastManager.getInstance(this).sendBroadcast(broadcastIntent);
}Then update the receiver in ConnectMessageFragment:
private final BroadcastReceiver updateReceiver = new BroadcastReceiver() {
@Override
public void onReceive(Context context, Intent intent) {
+ String updatedChannelId = intent.getStringExtra("channel_id");
+ if (updatedChannelId != null && updatedChannelId.equals(channelId)) {
refreshUi();
+ }
}
};Committable suggestion skipped: line range outside the PR's diff.
Summary
Simple change to support markdown in the messaging page
Jira ticket
cross-request: dimagi/commcare-core#1455
Feature Flag
ConnectID Messaging
Product Description
When messages contain markdown, it will be rendered appropriately in the messaging page.
PR Checklist
Automated test coverage
No automated tests for Connect code yet.
Safety story
Very small change, simply changes how the text is displayed and relies on existing code to render the markdown.