feat: Implement 'Mark as Read' action for push notifications … #6867
Conversation
…te Firebase configuration.
WalkthroughAdds a "Mark as read" notification action on Android and iOS: manifest and native handlers, Android broadcast receiver and PendingIntent wiring, iOS API request and client method, MARK_AS_READ action in push config, and an English localization key. The action posts to /api/v1/subscriptions.read and dismisses the notification on success. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant OS as Notification System
participant App
participant Server
User->>OS: Tap "Mark as read" action
OS->>App: Deliver intent / UNNotificationResponse
rect rgb(220,235,245)
Note over App: Extract payload (rid, serverURL, auth tokens)
App->>App: Validate payload & auth
end
rect rgb(245,235,220)
Note over App: Send API request
App->>Server: POST /api/v1/subscriptions.read { rid } (with auth headers)
end
alt success
Server-->>App: { success: true }
App->>App: Clear local unread state / cache
App->>OS: Cancel/dismiss notification
else failure
Server-->>App: error / non-200
App->>App: Log error (leave notification)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 4
🧹 Nitpick comments (2)
android/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.java (1)
659-689: Consider externalizing the action label for localization.The method correctly implements the Mark as Read action following the same pattern as
notificationReply. However, the label is hardcoded as "Mark as read" instead of loading from Android string resources, which prevents localization.🔎 Suggested improvement for localization
If you want to support localization similar to the iOS implementation, consider:
- Add to
android/app/src/main/res/values/strings.xml:<string name="mark_as_read">Mark as read</string>
- Update the method:
- String label = "Mark as read"; + String label = mContext.getString(R.string.mark_as_read);Alternatively, if localization isn't needed on Android (the label won't be translated), the current hardcoded approach is acceptable.
ios/ReplyNotification.swift (1)
125-145: Consider adding error handling for mark-as-read failures.The implementation correctly extracts the payload and performs the mark-as-read operation with proper background task management. However, unlike
handleReplyAction(lines 113-120), this method doesn't handle or notify the user about failures.🔎 Suggested improvement for consistency
To match the error handling pattern in
handleReplyAction:rocketchat.markAsRead(rid: rid) { response in DispatchQueue.main.async { + defer { + UIApplication.shared.endBackgroundTask(backgroundTask) + completionHandler() + } + + guard let response = response, response.success else { + // Optionally show failure notification + let content = UNMutableNotificationContent() + content.body = "Failed to mark as read." + let request = UNNotificationRequest(identifier: "markAsReadFailure", content: content, trigger: nil) + UNUserNotificationCenter.current().add(request, withCompletionHandler: nil) + return + } - UIApplication.shared.endBackgroundTask(backgroundTask) - completionHandler() } }This also addresses the SwiftLint warning about the unused
responseparameter.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
android/app/src/main/AndroidManifest.xmlandroid/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.javaandroid/app/src/main/java/chat/rocket/reactnative/notification/MarkAsReadBroadcast.javaapp/i18n/locales/en.jsonapp/lib/notifications/push.tsios/ReplyNotification.swiftios/Shared/RocketChat/API/Requests/MarkAsRead.swiftios/Shared/RocketChat/RocketChat.swift
🧰 Additional context used
🧬 Code graph analysis (4)
ios/ReplyNotification.swift (3)
ios/NotificationService/NotificationService.swift (1)
didReceive(9-73)ios/Shared/Extensions/String+Extensions.swift (1)
removeTrailingSlash(25-31)ios/Shared/RocketChat/RocketChat.swift (1)
markAsRead(84-95)
android/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.java (3)
android/app/src/main/java/chat/rocket/reactnative/notification/VideoConfBroadcast.kt (1)
TAG(14-73)android/app/src/main/java/chat/rocket/reactnative/notification/VideoConfNotification.kt (2)
context(21-210)createNotificationChannel(54-77)android/app/src/main/java/chat/rocket/reactnative/notification/MarkAsReadBroadcast.java (1)
MarkAsReadBroadcast(22-86)
android/app/src/main/java/chat/rocket/reactnative/notification/MarkAsReadBroadcast.java (1)
android/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.java (1)
CustomPushNotification(48-721)
ios/Shared/RocketChat/RocketChat.swift (4)
ios/Shared/RocketChat/API/API.swift (1)
fetch(54-91)ios/Shared/RocketChat/API/Request.swift (1)
request(45-72)ios/Shared/RocketChat/Database.swift (1)
readRoomEncrypted(117-125)ios/Shared/RocketChat/Encryption.swift (2)
encryptContent(212-269)decryptContent(175-210)
🪛 SwiftLint (0.57.0)
ios/ReplyNotification.swift
[Warning] 139-139: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
ios/Shared/RocketChat/RocketChat.swift
[Error] 25-25: @escaping must have a trailing space before the associated type
(attribute_name_spacing)
[Error] 39-39: @escaping must have a trailing space before the associated type
(attribute_name_spacing)
[Warning] 34-34: Avoid using unneeded break statements
(unneeded_break_in_switch)
[Error] 84-84: @escaping must have a trailing space before the associated type
(attribute_name_spacing)
[Warning] 71-71: Avoid using unneeded break statements
(unneeded_break_in_switch)
[Warning] 92-92: Avoid using unneeded break statements
(unneeded_break_in_switch)
🔇 Additional comments (7)
android/app/src/main/java/chat/rocket/reactnative/notification/MarkAsReadBroadcast.java (1)
22-43: LGTM!The
onReceiveimplementation correctly extracts notification data, validates the notification ID format, and delegates to themarkAsReadmethod. Error handling for invalid notification IDs is appropriate.app/i18n/locales/en.json (1)
502-502: LGTM!The localization entry follows the existing naming convention and is properly placed in alphabetical order.
app/lib/notifications/push.ts (1)
91-110: LGTM! Properly implements iOS notification action.The MARK_AS_READ_ACTION correctly mirrors the existing REPLY_ACTION pattern, with appropriate configuration:
- Uses localized button title via
I18n.t('Mark_as_read')- Sets
opensAppToForeground: falseto handle the action in the background- Properly registered in the MESSAGE category alongside existing actions
android/app/src/main/AndroidManifest.xml (1)
95-98: LGTM! Receiver declaration is properly configured.The receiver configuration is correct:
android:exported="false"properly restricts access to internal app components only (security best practice)- No intent-filter needed since it's triggered by direct PendingIntent from the notification action
- Logical placement between similar notification receivers
android/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.java (1)
446-446: LGTM! Action properly integrated into notification flow.The
notificationMarkAsReadcall is correctly placed alongsidenotificationReply, ensuring the Mark as Read action appears only on full notifications (not on fallback message-id-only placeholders).ios/ReplyNotification.swift (1)
44-48: LGTM! Action handler properly registered.The MARK_AS_READ_ACTION is correctly intercepted and handled natively before forwarding to the original delegate, following the same pattern as REPLY_ACTION.
ios/Shared/RocketChat/API/Requests/MarkAsRead.swift (1)
11-34: LGTM! API request implementation follows established patterns.The implementation correctly follows the Request/Response pattern used throughout the iOS codebase:
MarkAsReadBodyproperly encodes the requiredridparameterMarkAsReadResponseconforms to theResponseprotocolMarkAsReadRequestuses the correct HTTP POST method and endpoint path- Body encoding uses
JSONEncoderconsistently with other requests
android/app/src/main/java/chat/rocket/reactnative/notification/MarkAsReadBroadcast.java
Outdated
Show resolved
Hide resolved
android/app/src/main/java/chat/rocket/reactnative/notification/MarkAsReadBroadcast.java
Outdated
Show resolved
Hide resolved
android/app/src/main/java/chat/rocket/reactnative/notification/MarkAsReadBroadcast.java
Show resolved
Hide resolved
…edentials, and a memory monitor, while refactoring Android notification handling and updating Firebase configuration.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
android/app/src/main/java/chat/rocket/reactnative/notification/MarkAsReadBroadcast.java (2)
25-25: Configure timeouts for OkHttpClient.While the static instance addresses the reuse concern from previous reviews, the client lacks timeout configuration. Network operations without timeouts can hang indefinitely, particularly on poor connections or when the server is unresponsive.
🔎 Proposed timeout configuration
- private static final OkHttpClient client = new OkHttpClient(); + private static final OkHttpClient client = new OkHttpClient.Builder() + .connectTimeout(10, TimeUnit.SECONDS) + .readTimeout(10, TimeUnit.SECONDS) + .writeTimeout(10, TimeUnit.SECONDS) + .build();You'll also need to add the import:
import java.util.concurrent.TimeUnit;
56-56: Prefer Gson for JSON construction.Manual string formatting is fragile—if
ridcontains quotes or backslashes, the JSON will be malformed. Using Gson ensures proper escaping and is more maintainable.🔎 Proposed refactor
+ // Create a simple wrapper class or use a Map + java.util.Map<String, String> payload = new java.util.HashMap<>(); + payload.put("rid", rid); + String json = new Gson().toJson(payload); - String json = String.format("{\"rid\":\"%s\"}", rid); RequestBody body = RequestBody.create(JSON, json);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
android/app/src/main/java/chat/rocket/reactnative/notification/MarkAsReadBroadcast.javaios/Shared/RocketChat/RocketChat.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- ios/Shared/RocketChat/RocketChat.swift
🔇 Additional comments (2)
android/app/src/main/java/chat/rocket/reactnative/notification/MarkAsReadBroadcast.java (2)
60-61: No action needed. Theejson.token()andejson.userId()methods already validate their inputs and return non-null values (returning empty strings as fallback when credentials are missing), with appropriate error logging. The code at lines 60-61 is safe from null reference exceptions.Likely an incorrect or invalid review comment.
77-78: No action needed.CustomPushNotification.clearMessages(notId)is thread-safe because it performs an atomicremove()operation on aConcurrentHashMap, which is designed for concurrent access.Likely an incorrect or invalid review comment.
android/app/src/main/java/chat/rocket/reactnative/notification/MarkAsReadBroadcast.java
Show resolved
Hide resolved
…n, and improve Android notification async handling.
DSingh0304
requested a deployment
to
approve_e2e_testing
GitHub Actions
Waiting
|
@DSingh0304 Thanks for your contribution! |
DSingh0304
requested a deployment
to
experimental_android_build
GitHub Actions
Waiting
DSingh0304
requested a deployment
to
experimental_ios_build
GitHub Actions
Waiting
DSingh0304
requested a deployment
to
official_android_build
GitHub Actions
Waiting
DSingh0304
requested a deployment
to
official_ios_build
GitHub Actions
Waiting
Yes Sure !! |
2 participants
Proposed changes
This PR implements a "Mark as Read" quick action button for push notifications on both iOS and Android platforms. This feature allows users to mark messages/rooms as read directly from a push notification without opening the app, providing a convenient way to manage notifications.
Implementation Details:
MARK_AS_READ_ACTIONalongside the existing Reply actionPOST /api/v1/subscriptions.readAPI endpoint (available since Rocket.Chat server v0.61.0)Platforms:
Issue(s)
Closes #6842
How to test or reproduce
Prerequisites:
Testing Steps:
Note on Testing:
Due to Firebase configuration limitations in development builds, full end-to-end push notification testing requires production Firebase credentials. The implementation follows the existing Reply action pattern and uses the established
subscriptions.readAPI endpoint.Screenshots
Types of changes
Checklist
Further comments
Implementation Approach:
This feature was implemented by following the existing Reply notification action pattern:
Files Modified/Created (8 total):
Common (2 files):
MARK_AS_READ_ACTIONnotification actioniOS (3 files):
handleMarkAsReadAction()handlerAndroid (3 files):
android/app/src/main/java/.../MarkAsReadBroadcast.java- NEW: Broadcast receiverandroid/app/src/main/java/.../CustomPushNotification.java- Added notification action buttonAPI Endpoint:
Uses the existing
POST /api/v1/subscriptions.readendpoint which:rid(room ID) parameterTesting Limitations:
Full push notification testing requires production Firebase credentials which are not available in development builds. This is a known limitation discussed in the community (reference: community conversation). The code follows established patterns and compiles successfully on both platforms.
Why This Approach:
Summary by CodeRabbit
New Features
Localization
✏️ Tip: You can customize this high-level summary in your review settings.