[NO MERGE] fix(Android): Push notification on killed apps and deep link #6898
Conversation
- Refactored MainActivity to utilize NotificationIntentHandler for managing notification intents. - Added checkPendingNotification function to handle notifications when the app comes to the foreground. - Updated push notification configuration to retrieve pending notifications from the native module. - Enhanced notification handling logic in the notifications module to ensure proper processing of notifications.
- Refactored the E2E notification handling in CustomPushNotification to eliminate the asynchronous processor, streamlining the decryption process. - Enhanced error handling in the Encryption class for better robustness during key parsing and decryption. - Added additional fields to VideoConfBroadcast for improved notification data handling. - Updated notification parsing in the notifications module to include error handling for EJSON parsing failures.
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
WalkthroughCentralizes Android notification intent routing into NotificationIntentHandler, removes the async E2ENotificationProcessor, moves E2E decryption into synchronous paths in CustomPushNotification, and adds a TurboModule PushNotificationModule plus JS hooks to persist and surface pending notifications. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant MainActivity
participant NotificationIntentHandler
participant CustomPushNotification
participant PushNotificationModule
participant SharedPrefs as SharedPreferences
participant JS as JavaScript
MainActivity->>NotificationIntentHandler: handleIntent(context,intent)
alt video conference action
NotificationIntentHandler->>PushNotificationModule: storePendingNotification(videoConf JSON)
PushNotificationModule->>SharedPrefs: write KEY_PENDING_NOTIFICATION
else regular notification tap
NotificationIntentHandler->>CustomPushNotification: forward extras/ejson
alt encrypted
CustomPushNotification->>CustomPushNotification: decryptMessage (synchronous)
CustomPushNotification->>PushNotificationModule: storePendingNotification(decrypted JSON)
else not encrypted
CustomPushNotification->>PushNotificationModule: storePendingNotification(JSON)
end
PushNotificationModule->>SharedPrefs: write KEY_PENDING_NOTIFICATION
end
JS->>PushNotificationModule: getPendingNotification()
PushNotificationModule->>SharedPrefs: read & clear KEY_PENDING_NOTIFICATION
SharedPrefs-->>PushNotificationModule: notification JSON / null
PushNotificationModule-->>JS: Promise resolves with string|null
JS->>JS: checkPendingNotification() parses JSON and dispatches onNotification
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
android/app/src/main/java/chat/rocket/reactnative/notification/Encryption.java (1)
288-356: UseRoomKeyResultvalues directly instead of relying on instance field state.
encryptMessageContentobtainsRoomKeyResultcontaining thealgorithmvalue (line 301), but then ignores it and uses the instance fieldthis.algorithm(line 312) instead. Similarly,this.keyIdis set bydecryptRoomKeyand used byencryptMessageContentwithout being passed between methods. This creates implicit state coupling where the correctness depends on method call order within the same invocation.Refactor to pass
algorithmandkeyIdfromRoomKeyResultdirectly toEncryptionContentinstead of storing them as instance fields:// In encryptMessageContent: RoomKeyResult roomKeyResult = decryptRoomKey(room.e2eKey, ejson); // ... later ... return new EncryptionContent(roomKeyResult.algorithm, encryptedData, roomKeyResult.keyId, ...);
🤖 Fix all issues with AI agents
In
@android/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.java:
- Around line 236-247: handleE2ENotification currently drops the notification
when Encryption.shared.decryptMessage returns null; change it so that on null
you populate the bundle with a fallback message (e.g., "Encrypted message - open
app to decrypt") and any minimal metadata needed, set mBundle = bundle, create
an updated Ejson via safeFromJson(bundle.getString("ejson", "{}"), Ejson.class)
or reuse existing ejson, and call showNotification(bundle, updatedEjson, notId)
so a placeholder notification is shown instead of being dropped.
🧹 Nitpick comments (6)
app/lib/notifications/push.ts (2)
218-274: Consider extracting shared notification transformation logic.The notification transformation code (lines 228-243) duplicates the logic in
app/lib/notifications/index.ts(lines 107-121). Consider extracting a shared helper function to avoid maintaining two identical transformations.♻️ Suggested helper extraction
Create a shared transformation function:
// In a shared location, e.g., app/lib/notifications/utils.ts export const transformNativeNotificationData = (notificationData: Record<string, any>): INotification => ({ payload: { message: notificationData.message || '', style: notificationData.style || '', ejson: notificationData.ejson || '', collapse_key: notificationData.collapse_key || '', notId: notificationData.notId || '', msgcnt: notificationData.msgcnt || '', title: notificationData.title || '', from: notificationData.from || '', image: notificationData.image || '', soundname: notificationData.soundname || '', action: notificationData.action }, identifier: notificationData.notId || '' });Then use it in both
push.tsandindex.ts.
257-273: Simplify promise chain structure.The consecutive
.then()blocks can be combined for better readability. The current structure chains two.then()and two.catch()blocks which is harder to follow.♻️ Suggested simplification using async/await
- return NativePushNotificationModule.getPendingNotification() - .then(pendingNotification => { - if (pendingNotification) { - try { - // ... transformation logic - return transformed; - } catch (parseError) { - console.error('[push.ts] Error parsing notification data:', parseError); - return null; - } - } - return null; - }) - .catch(e => { - console.error('[push.ts] Error getting pending notification from native module:', e); - return null; - }) - .then(nativeNotification => { - if (nativeNotification) { - return nativeNotification; - } - const lastResponse = Notifications.getLastNotificationResponse(); - if (lastResponse) { - return transformNotificationResponse(lastResponse); - } - return null; - }) - .catch(e => { - console.error('[push.ts] Error in promise chain:', e); - return null; - }); + try { + const pendingNotification = await NativePushNotificationModule.getPendingNotification(); + if (pendingNotification) { + try { + const notificationData = JSON.parse(pendingNotification); + // ... transformation to INotification + return transformed; + } catch (parseError) { + console.error('[push.ts] Error parsing notification data:', parseError); + } + } + } catch (e) { + console.error('[push.ts] Error getting pending notification from native module:', e); + } + + // Fallback to expo-notifications + const lastResponse = Notifications.getLastNotificationResponse(); + if (lastResponse) { + return transformNotificationResponse(lastResponse); + } + return null;Note: This would require making
pushNotificationConfigurean async function.app/lib/notifications/index.ts (1)
98-133: MissingclearPendingNotificationcall after successful processing.After successfully retrieving and processing the pending notification, the native module's
clearPendingNotificationshould be called to prevent reprocessing ifcheckPendingNotificationis invoked again before the native side clears it.While
getPendingNotificationin the native module clears the notification after reading (line 54 inPushNotificationModule.kt), an explicit JS-side clear provides a safety net and clearer intent.♻️ Suggested improvement
const notificationData = JSON.parse(pendingNotification); const notification: INotification = { payload: { message: notificationData.message || '', style: notificationData.style || '', ejson: notificationData.ejson || '', collapse_key: notificationData.collapse_key || '', notId: notificationData.notId || '', msgcnt: notificationData.msgcnt || '', title: notificationData.title || '', from: notificationData.from || '', image: notificationData.image || '', soundname: notificationData.soundname || '', action: notificationData.action }, identifier: notificationData.notId || '' }; onNotification(notification); + // Explicitly clear after successful processing (belt-and-suspenders) + NativePushNotificationModule.clearPendingNotification?.(); } catch (e) {android/app/src/main/java/chat/rocket/reactnative/notification/PushNotificationModule.kt (2)
47-61: Minor race condition between read and clear operations.The read and clear are not atomic—if
getPendingNotificationis called concurrently (unlikely but possible), both calls could read the same notification before either clears it. Consider using an atomic read-and-remove pattern.♻️ Atomic read-and-clear
@ReactMethod override fun getPendingNotification(promise: Promise) { try { val prefs = reactApplicationContext.getSharedPreferences(PREFS_NAME, Context.MODE_PRIVATE) - val notification = prefs.getString(KEY_PENDING_NOTIFICATION, null) - - // Clear the notification after reading - notification?.let { - prefs.edit().remove(KEY_PENDING_NOTIFICATION).apply() - } + val notification: String? + synchronized(PushNotificationModule::class.java) { + notification = prefs.getString(KEY_PENDING_NOTIFICATION, null) + notification?.let { + prefs.edit().remove(KEY_PENDING_NOTIFICATION).commit() + } + } promise.resolve(notification) } catch (e: Exception) { promise.reject("ERROR", e.message) } }
67-76: Swallowed exception hides potential issues.Per the static analysis hint, the caught exception is discarded without logging. While errors in
clearPendingNotificationare non-critical, logging them aids debugging.♻️ Log the swallowed exception
@ReactMethod override fun clearPendingNotification() { try { reactApplicationContext.getSharedPreferences(PREFS_NAME, Context.MODE_PRIVATE) .edit() .remove(KEY_PENDING_NOTIFICATION) .apply() } catch (e: Exception) { - // Ignore errors + android.util.Log.w("RocketChat.PushNotificationModule", "Failed to clear pending notification", e) } }android/app/src/main/java/chat/rocket/reactnative/notification/NotificationIntentHandler.kt (1)
70-71: Consider caching the Gson instance.
GsonBuilder().create()is called each time an intent is processed. SinceGsonis thread-safe, a single instance can be reused.♻️ Cache Gson at companion object level
class NotificationIntentHandler { companion object { private const val TAG = "RocketChat.NotificationIntentHandler" + private val gson = GsonBuilder().create() // ... in handleVideoConfIntent: - val gson = GsonBuilder().create() - val jsonData = gson.toJson(data) + val jsonData = gson.toJson(data) // ... in handleNotificationIntent: - val gson = GsonBuilder().create() - val jsonData = gson.toJson(notificationData) + val jsonData = gson.toJson(notificationData)Also applies to: 139-140
📜 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 (14)
android/app/src/main/java/chat/rocket/reactnative/MainActivity.ktandroid/app/src/main/java/chat/rocket/reactnative/MainApplication.ktandroid/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.javaandroid/app/src/main/java/chat/rocket/reactnative/notification/E2ENotificationProcessor.javaandroid/app/src/main/java/chat/rocket/reactnative/notification/Encryption.javaandroid/app/src/main/java/chat/rocket/reactnative/notification/NativePushNotificationSpec.ktandroid/app/src/main/java/chat/rocket/reactnative/notification/NotificationIntentHandler.ktandroid/app/src/main/java/chat/rocket/reactnative/notification/PushNotificationModule.ktandroid/app/src/main/java/chat/rocket/reactnative/notification/PushNotificationTurboPackage.ktandroid/app/src/main/java/chat/rocket/reactnative/notification/VideoConfBroadcast.ktapp/lib/native/NativePushNotificationAndroid.tsapp/lib/notifications/index.tsapp/lib/notifications/push.tsapp/sagas/state.js
💤 Files with no reviewable changes (1)
- android/app/src/main/java/chat/rocket/reactnative/notification/E2ENotificationProcessor.java
🧰 Additional context used
🧬 Code graph analysis (5)
app/sagas/state.js (1)
app/lib/notifications/index.ts (1)
checkPendingNotification(98-133)
app/lib/notifications/index.ts (6)
app/lib/store/auxStore.ts (1)
store(6-6)app/actions/deepLinking.ts (1)
deepLinkingClickCallPush(26-31)app/actions/app.ts (1)
appInit(45-49)app/lib/methods/roomTypeToApiType.ts (1)
types(19-24)app/sagas/deepLinking.js (2)
name(62-62)type(61-61)app/definitions/INotification.ts (1)
INotification(1-16)
app/lib/notifications/push.ts (1)
app/definitions/INotification.ts (1)
INotification(1-16)
android/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.java (1)
android/app/src/main/java/chat/rocket/reactnative/notification/Encryption.java (1)
Encryption(105-511)
android/app/src/main/java/chat/rocket/reactnative/notification/Encryption.java (2)
app/lib/encryption/utils.ts (1)
decodePrefixedBase64(200-219)ios/Shared/RocketChat/Encryption.swift (1)
decodePrefixedBase64(75-92)
🪛 detekt (1.23.8)
android/app/src/main/java/chat/rocket/reactnative/notification/PushNotificationModule.kt
[warning] 73-73: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
🔇 Additional comments (15)
app/sagas/state.js (1)
32-35: LGTM - fire-and-forget pattern is appropriate here.The async call with
.catch()correctly prevents unhandled promise rejection. Note thatcheckPendingNotificationalready has internal try/catch handling (perapp/lib/notifications/index.ts), so the outer.catch()provides a safety net but may result in double-logging on errors.android/app/src/main/java/chat/rocket/reactnative/notification/VideoConfBroadcast.kt (1)
51-52: LGTM - consistent with existing field extraction pattern.The new
hostandcallIdfields follow the same safe extraction pattern with empty string defaults as the other fields in the data map.app/lib/native/NativePushNotificationAndroid.ts (1)
1-9: LGTM - clean TurboModule bridge definition.The interface correctly defines the contract matching the native
PushNotificationModule.ktimplementation. UsingTurboModuleRegistry.get()(which returnsSpec | null) is appropriate, and callers properly null-check before usage.android/app/src/main/java/chat/rocket/reactnative/MainApplication.kt (1)
22-22: LGTM - proper TurboPackage registration.The
PushNotificationTurboPackageis correctly imported and added to the packages list, following the established pattern for other TurboPackages in this application.Also applies to: 45-45
android/app/src/main/java/chat/rocket/reactnative/MainActivity.kt (1)
29-43: LGTM - clean centralized intent handling.The refactoring to use
NotificationIntentHandlerprovides good separation of concerns. The null-safe handling inonCreateviaintent?.letand thesetIntent(intent)call inonNewIntentare both appropriate patterns.android/app/src/main/java/chat/rocket/reactnative/notification/NativePushNotificationSpec.kt (1)
9-23: Well-structured TurboModule specification.The abstract class correctly defines the contract for the push notification native module with proper React Native annotations. The companion object pattern for the module name is idiomatic Kotlin.
android/app/src/main/java/chat/rocket/reactnative/notification/PushNotificationTurboPackage.kt (1)
12-37: Standard TurboModule package implementation.The package correctly registers the
PushNotificationModulewith appropriate module info flags. The lazy instantiation pattern ingetModuleis correct.app/lib/notifications/index.ts (2)
25-34: Good defensive error handling for video conf notifications.Wrapping the EJSON parse in try/catch prevents crashes from malformed video conference payloads while allowing the flow to continue.
48-51: Reasonable fallback for incomplete notification data.Dispatching
appInitwhen essential fields are missing ensures the app opens gracefully rather than failing silently or crashing.android/app/src/main/java/chat/rocket/reactnative/notification/PushNotificationModule.kt (1)
24-39: Good synchronous write with proper error handling.Using
commit()instead ofapply()ensures the notification is persisted before returning, which is important for the notification tap flow. The success check and exception handling are appropriate.android/app/src/main/java/chat/rocket/reactnative/notification/NotificationIntentHandler.kt (2)
22-31: Clean intent routing with clear separation of concerns.The dispatcher pattern effectively centralizes notification handling logic that was previously scattered in
MainActivity.
121-131: Heuristic for non-serializable detection is fragile but acceptable.The check
!stringValue.startsWith("android.") && !stringValue.contains("@")is a best-effort filter for object references. While imperfect, it's wrapped in try/catch and logs skipped extras, making it safe for production.android/app/src/main/java/chat/rocket/reactnative/notification/Encryption.java (3)
191-208: Good defensive column access with proper null handling.Checking column indices before accessing prevents crashes when the database schema differs from expectations. The early return with logging provides clear diagnostics.
248-272: Robust handling of double-encoded private keys.The fallback parsing for JSON-encoded strings handles edge cases where the private key may be stored as a serialized JSON string within JSON. The validation of required fields (n, e, d) ensures downstream operations won't fail with null pointer exceptions.
320-343: Improved error handling for RSA decryption and session key parsing.The wrapped operations with explicit error logging will significantly aid debugging E2E decryption failures in production. The null checks prevent silent failures.
diegolmello
force-pushed
the
fix.android-e2ee-killed-app-without-avatar-work
branch
from
a98d3d3 to
7f93c0e
Compare
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
@android/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.java:
- Around line 171-189: In handleE2ENotification, guard against a possible null
Encryption.shared before calling decryptMessage: check if Encryption.shared is
null, log a warning (e.g., using Log.w with TAG) and treat as decryption failure
by setting the fallback message ("Encrypted message"), assigning mBundle inside
the synchronized block, and calling showNotification(bundle, ejson, notId);
otherwise proceed to call Encryption.shared.decryptMessage(ejson, mContext) as
before and handle the decrypted result.
- Line 62: Multiple unsynchronized reads of the field mBundle in methods like
processNotification() and buildNotification() can see different Bundle instances
if another thread reassigns mBundle; to fix this either capture the reference
once at method entry (e.g., Bundle bundle = mBundle;) and use that local
variable for all reads in the method, or wrap any sequences that perform
multiple reads of mBundle in synchronized(this) blocks so they see a consistent
object; apply this change to all multi-read sequences that currently read
mBundle (including the locations where other threads perform synchronized writes
that reassign mBundle).
📜 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 (4)
android/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.javaandroid/app/src/main/java/chat/rocket/reactnative/notification/Ejson.javaapp/lib/native/NativePushNotificationAndroid.tsapp/sagas/state.js
💤 Files with no reviewable changes (1)
- android/app/src/main/java/chat/rocket/reactnative/notification/Ejson.java
🚧 Files skipped from review as they are similar to previous changes (2)
- app/sagas/state.js
- app/lib/native/NativePushNotificationAndroid.ts
🧰 Additional context used
🧬 Code graph analysis (1)
android/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.java (2)
android/app/src/main/java/chat/rocket/reactnative/notification/RCFirebaseMessagingService.kt (1)
TAG(14-51)android/app/src/main/java/chat/rocket/reactnative/notification/Encryption.java (1)
Encryption(105-511)
🔇 Additional comments (4)
android/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.java (4)
96-96: LGTM!The updated error message is clearer and more descriptive.
154-154: LGTM!The return statement correctly prevents fall-through to the regular notification path after E2E handling is complete.
180-188: Fallback notification properly addresses past review concern.The implementation now correctly shows a fallback notification with "Encrypted message" when decryption fails, rather than silently dropping it. This ensures users are notified even when E2E keys are unavailable.
215-217: Synchronization lock usage is consistent.The synchronized block correctly protects the
mBundleassignment. Note that within theloadNotificationAndProcesscallback (line 126), the code usessynchronized(CustomPushNotification.this)because it's within an anonymous inner class (Callback), whereas herethisis used directly. Both refer to the same lock object in their respective contexts, so the synchronization is correct.
diegolmello
had a problem deploying
to
experimental_ios_build
GitHub Actions
Error
diegolmello
had a problem deploying
to
official_ios_build
GitHub Actions
Error
diegolmello
temporarily deployed
to
experimental_android_build
GitHub Actions
Inactive
diegolmello
had a problem deploying
to
official_android_build
GitHub Actions
Error
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)
197-199: Race condition in notification message list initialization.The check-then-act pattern on
notificationMessagesis not atomic. Two threads could both seenullfor the samenotIdand both create ArrayLists, with one overwriting the other and losing messages. Additionally,ArrayListis not thread-safe for concurrentadd()calls.Use
computeIfAbsentwith a thread-safe list:Suggested fix
- // Initialize notification message list for this ID - if (notificationMessages.get(notId) == null) { - notificationMessages.put(notId, new ArrayList<>()); - } + // Initialize notification message list for this ID (atomic operation) + notificationMessages.computeIfAbsent(notId, k -> Collections.synchronizedList(new ArrayList<>()));You'll also need to add the import:
import java.util.Collections;
286-343: CapturemBundleat method entry to ensure consistent reads.This method reads
mBundlemultiple times (lines 288–291, 303, 328, 334–335) without synchronization. Apply the same fix as suggested forprocessNotification(): capture the reference once at the start.private Notification.Builder buildNotification(int notificationId) { + final Bundle bundle = mBundle; String notId = Integer.toString(notificationId); - String title = mBundle.getString("title"); - String message = mBundle.getString("message"); - Boolean notificationLoaded = mBundle.getBoolean("notificationLoaded", false); - Ejson ejson = safeFromJson(mBundle.getString("ejson", "{}"), Ejson.class); + String title = bundle.getString("title"); + String message = bundle.getString("message"); + Boolean notificationLoaded = bundle.getBoolean("notificationLoaded", false); + Ejson ejson = safeFromJson(bundle.getString("ejson", "{}"), Ejson.class); // ... and update remaining mBundle references in this method
🤖 Fix all issues with AI agents
In
@android/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.java:
- Around line 171-189: handleE2ENotification calls
Encryption.shared.decryptMessage(ejson, mContext) without guarding against
Encryption.shared being null; add a null check before calling decryptMessage
(e.g., if Encryption.shared == null) and in that branch treat as decryption
failure: log a warning, set bundle.putString("message", "Encrypted message"),
synchronize mBundle assignment, and call showNotification(bundle, ejson, notId)
so behavior matches the existing fallback path; ensure the non-null path
continues to call decryptMessage and behaves unchanged.
- Line 62: The methods processNotification and buildNotification perform
multiple unsynchronized reads of the volatile field mBundle; capture mBundle
once into a local Bundle variable at the start of each method (e.g., Bundle
bundle = mBundle), perform a single null check on that local reference, and then
use that local bundle throughout the method so all reads operate on the same
Bundle instance (update all uses in processNotification and all code paths in
buildNotification to reference the local variable instead of mBundle).
📜 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 (1)
android/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.java
🧰 Additional context used
🧬 Code graph analysis (1)
android/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.java (2)
android/app/src/main/java/chat/rocket/reactnative/notification/RCFirebaseMessagingService.kt (1)
TAG(14-51)android/app/src/main/java/chat/rocket/reactnative/notification/Encryption.java (1)
Encryption(105-511)
🔇 Additional comments (1)
android/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.java (1)
215-218: LGTM!The synchronized block correctly protects the
mBundleassignment, ensuring visibility to other threads.
android/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.java
Show resolved
Hide resolved
android/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.java
Show resolved
Hide resolved
diegolmello
had a problem deploying
to
upload_experimental_android
GitHub Actions
Error
|
Android Build Available Rocket.Chat Experimental 4.68.1.107999 Internal App Sharing: https://play.google.com/apps/test/RQVpXLytHNc/ahAO29uNRalD7VxOP4V_ybRJDJW4QKsEeZ6FQ7okTiCcS2SPASA2pr091mlKRS-1gjvGazda11GWP8hlkJ3sEGIZJg |
- Replaced direct usage of mBundle with a local bundle variable for improved readability and consistency. - Enhanced error handling in handleE2ENotification to manage null Encryption singleton cases. - Updated notification building methods to utilize the local bundle variable, ensuring clarity in data handling.
diegolmello
requested a deployment
to
approve_e2e_testing
GitHub Actions
Waiting
diegolmello
requested a deployment
to
experimental_ios_build
GitHub Actions
Waiting
diegolmello
temporarily deployed
to
experimental_android_build
GitHub Actions
Inactive
diegolmello
requested a deployment
to
official_ios_build
GitHub Actions
Waiting
diegolmello
requested a deployment
to
official_android_build
GitHub Actions
Waiting
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
android/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.java (1)
206-244: Potential race condition in notification message list initialization.The check-then-put pattern on lines 208-210 is not atomic. Two threads could both see
nulland create separateArrayListinstances, with one overwriting the other.🔧 Recommended fix using computeIfAbsent
- // Initialize notification message list for this ID - if (notificationMessages.get(notId) == null) { - notificationMessages.put(notId, new ArrayList<>()); - } + // Initialize notification message list for this ID (thread-safe) + notificationMessages.computeIfAbsent(notId, k -> new ArrayList<>());Note: After this change, line 239's
notificationMessages.get(notId).add(bundle)should also be synchronized or use a thread-safe list, sinceArrayListis not thread-safe for concurrent modifications.
🧹 Nitpick comments (1)
android/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.java (1)
172-200: Consider localizing the fallback message.The hardcoded string
"Encrypted message"on lines 175 and 194 should be moved to string resources for proper localization.♻️ Suggested improvement
- bundle.putString("message", "Encrypted message"); + bundle.putString("message", mContext.getString(R.string.encrypted_message_fallback));Add to
res/values/strings.xml:<string name="encrypted_message_fallback">Encrypted message</string>
📜 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 (1)
android/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.java
⏰ 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). (2)
- GitHub Check: ESLint and Test / run-eslint-and-test
- GitHub Check: format
🔇 Additional comments (7)
android/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.java (7)
62-62: Good use of volatile for thread visibility.The
volatilemodifier ensures visibility ofmBundlechanges across threads, which is appropriate given the multi-threaded nature of push notification handling.
78-98: LGTM!The simplified
onReceivedflow with proper validation and error handling is clean and appropriate.
138-160: Good thread-safety pattern with local bundle capture.Capturing
mBundleinto a localbundlevariable at the start of the method ensures consistent state throughout method execution, preventing potential race conditions.
226-229: LGTM!Consistent synchronized update of
mBundlemaintains thread safety.
297-355: LGTM!Consistent use of local
bundlevariable throughoutbuildNotificationensures thread-safe access and stable notification content during the build process.
455-555: LGTM!The
notificationStyleandnotificationReplymethods correctly use the passedbundleparameter, maintaining consistency with the thread-safe pattern established inbuildNotification.
113-136: The callback invoked byLoadNotification.load()executes on the same background thread that calledload()(the FCM message handler thread). The synchronized block inloadNotificationAndProcesscorrectly ensures thread-safe visibility of themBundleupdate, and sinceprocessNotification()immediately readsmBundleinto a local variable, there is no deadlock or race condition risk. The current implementation is thread-safe.
diegolmello
requested a deployment
to
upload_experimental_android
GitHub Actions
Waiting
|
Android Build Available Rocket.Chat Experimental 4.68.1.108000 Internal App Sharing: https://play.google.com/apps/test/RQVpXLytHNc/ahAO29uNR2VpEsu2R5bfS2jzzW9tL7uSx_jXrAhpiv5ION2OOXVro_0yAT3bkxu51KqE0Zp0egELa6ZkMixIr9uB_L |
Proposed changes
Issue(s)
Same work from #6888, but without #6853 dependency
https://rocketchat.atlassian.net/browse/CORE-1645
How to test or reproduce
Screenshots
Types of changes
Checklist
Further comments
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.