feat: Implement 'Mark as Read' action for push notifications …#6867
feat: Implement 'Mark as Read' action for push notifications …#6867DSingh0304 wants to merge 14 commits into
Conversation
…te Firebase configuration.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a "Mark as read" quick action to push notifications across Android and iOS: manifest and broadcast receiver on Android, notification wiring and avatar/loading adjustments, native iOS action handling and API request, shared iOS API wrapper and RocketChat method, push config update, and English localization entry. Action POSTs to /api/v1/subscriptions.read and dismisses 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 rgba(220,235,245,0.5)
Note over App: Extract payload (rid, serverURL, auth tokens)
App->>App: Validate payload & auth
end
rect rgba(245,235,220,0.5)
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🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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
…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.
…n, and improve Android notification async handling.
DSingh0304
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Failure
|
@DSingh0304 Thanks for your contribution! |
DSingh0304
had a problem deploying
to
experimental_android_build
GitHub Actions
Failure
DSingh0304
had a problem deploying
to
experimental_ios_build
GitHub Actions
Failure
DSingh0304
had a problem deploying
to
official_android_build
GitHub Actions
Failure
DSingh0304
had a problem deploying
to
official_ios_build
GitHub Actions
Failure
Yes Sure !! |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
android/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.java (2)
160-167:⚠️ Potential issue | 🔴 CriticalRemove the duplicate
message-id-onlycheck (compile error).Two consecutive
ifstatements open the same block, leaving the first one unclosed.🧹 Proposed fix
- if (receivedEjson != null && receivedEjson.notificationType != null - && receivedEjson.notificationType.equals("message-id-only")) { - - if (receivedEjson != null && receivedEjson.notificationType != null && receivedEjson.notificationType.equals("message-id-only")) { + if (receivedEjson != null && receivedEjson.notificationType != null + && receivedEjson.notificationType.equals("message-id-only")) {
241-301:⚠️ Potential issue | 🔴 CriticalConflicting E2E implementations are interleaved (duplicate variable/braces).
The current block mixes the old and new flows, redeclares
decrypted, and leaves mismatched braces. Please consolidate to a single strategy and ensure success/fallback paths callshowNotification.🧹 Proposed consolidation (fast path + async fallback)
- // Check if React context is immediately available - if (reactApplicationContext != null) { - // Fast path: decrypt immediately - String decrypted = Encryption.shared.decryptMessage(ejson, reactApplicationContext); - - if (decrypted != null) { - bundle.putString("message", decrypted); - // Decrypt immediately using regular Android Context (mContext) - // This works without React Native initialization - String decrypted = Encryption.shared.decryptMessage(ejson, mContext); - - if (decrypted != null) { - bundle.putString("message", decrypted); - synchronized(this) { - mBundle = bundle; - } - return; - } + if (reactApplicationContext != null) { + String decrypted = Encryption.shared.decryptMessage(ejson, reactApplicationContext); + if (decrypted != null) { + bundle.putString("message", decrypted); + synchronized (this) { + mBundle = bundle; + } + showNotification(bundle, ejson, notId); + return; + } + } // Slow path: wait for React context asynchronously Log.i(TAG, "Waiting for React context to decrypt E2E notification"); E2ENotificationProcessor processor = new E2ENotificationProcessor( // Context provider () -> reactApplicationContext, // Callback new E2ENotificationProcessor.NotificationCallback() { `@Override` public void onDecryptionComplete(Bundle decryptedBundle, Ejson decryptedEjson, String notificationId) { mBundle = decryptedBundle; Ejson finalEjson = safeFromJson(decryptedBundle.getString("ejson", "{}"), Ejson.class); showNotification(decryptedBundle, finalEjson, notificationId); } `@Override` public void onDecryptionFailed(Bundle originalBundle, Ejson originalEjson, String notificationId) { Log.w(TAG, "E2E decryption failed for notification"); + originalBundle.putString("message", "Encrypted message"); + synchronized (CustomPushNotification.this) { + mBundle = originalBundle; + } + showNotification(originalBundle, originalEjson, notificationId); } `@Override` public void onTimeout(Bundle originalBundle, Ejson originalEjson, String notificationId) { Log.w(TAG, "Timeout waiting for React context for E2E notification"); + originalBundle.putString("message", "Encrypted message"); + synchronized (CustomPushNotification.this) { + mBundle = originalBundle; + } + showNotification(originalBundle, originalEjson, notificationId); } }); processor.processAsync(bundle, ejson, notId); - showNotification(bundle, ejson, notId); - } else { - Log.w(TAG, "E2E decryption failed for notification, showing fallback notification"); - // Show fallback notification so user knows a message arrived - // Use a placeholder message since we can't decrypt - bundle.putString("message", "Encrypted message"); - synchronized(this) { - mBundle = bundle; - } - showNotification(bundle, ejson, notId); - }
🤖 Fix all issues with AI agents
In
`@android/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.java`:
- Around line 495-528: The getAvatar method contains an unreachable final return
calling NotificationHelper.fetchAvatarBitmap because both the try block and its
catch already return; remove that dead code (the final "return
NotificationHelper.fetchAvatarBitmap(mContext, uri, largeIcon());") or refactor
so the method returns once (e.g., assign avatar in try/catch and have a single
final return), ensuring references to getAvatar, largeIcon, and
NotificationHelper.fetchAvatarBitmap are updated/removed accordingly so the code
compiles.
- Around line 73-78: The method isReactInitialized() references an undeclared
reactApplicationContext; declare a class-level field named
reactApplicationContext (type ReactApplicationContext) as volatile, import
com.facebook.react.bridge.ReactApplicationContext, and ensure it is populated
when the React instance is created (e.g., set it from your
ReactNativeHost/ReactInstanceManager or during initialization in the class
constructor or an init method). Make reads/writes thread-safe by keeping the
volatile modifier and update any places that previously accessed a local/react
context to use this new field so isReactInitialized() returns a valid check
against the populated context.
|
|
|
Deep seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
…centralize fallback notification logic.
…Bitmap` instead of direct Glide usage with timeout.
DSingh0304
force-pushed
the
feature/mark-as-read
branch
from
5634c1d to
3f598e8
Compare
3 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
Improvements
Localization