android/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.java (1)

92-92: Consider adding more context to the error message.

The generic "Failed to process notification" message provides less debugging context than a more specific message would. Consider including details about the notification type or processing stage that failed.

💡 Example with more context

-            Log.e(TAG, "Failed to process notification", e);
+            Log.e(TAG, "Failed to process notification (notId: " + notId + ")", e);

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.

android/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.java (1)

321-332: Inconsistent debug log gating.

Lines 321-332 add extensive debug logging in buildNotification(), but these logs are not gated by ENABLE_VERBOSE_LOGS like similar debug statements elsewhere in the method (lines 308-314). This creates inconsistent logging behavior and potential log spam in production builds.

🔧 Proposed fix to gate debug logs consistently

-        // Log intent extras for debugging
-        Log.d(TAG, "[buildNotification] Creating intent with extras:");
-        Log.d(TAG, "[buildNotification]   - ejson: " + (mBundle.getString("ejson") != null ? "[present, length=" + mBundle.getString("ejson").length() + "]" : "[null]"));
-        Log.d(TAG, "[buildNotification]   - rid: " + (ejson != null && ejson.rid != null ? ejson.rid : "[null]"));
-        Log.d(TAG, "[buildNotification]   - type: " + (ejson != null && ejson.type != null ? ejson.type : "[null]"));
-        Log.d(TAG, "[buildNotification]   - host: " + (ejson != null && ejson.host != null ? "[present]" : "[null]"));
-        Log.d(TAG, "[buildNotification]   - messageId: " + (ejson != null && ejson.messageId != null ? ejson.messageId : "[null]"));
-        Log.d(TAG, "[buildNotification]   - notificationType: " + (ejson != null && ejson.notificationType != null ? ejson.notificationType : "[null]"));
-        Log.d(TAG, "[buildNotification]   - notId: " + mBundle.getString("notId"));
-        Log.d(TAG, "[buildNotification]   - title: " + (title != null ? "[present]" : "[null]"));
-        Log.d(TAG, "[buildNotification]   - message: " + (message != null ? "[present, length=" + message.length() + "]" : "[null]"));
+        if (ENABLE_VERBOSE_LOGS) {
+            // Log intent extras for debugging
+            Log.d(TAG, "[buildNotification] Creating intent with extras:");
+            Log.d(TAG, "[buildNotification]   - ejson: " + (mBundle.getString("ejson") != null ? "[present, length=" + mBundle.getString("ejson").length() + "]" : "[null]"));
+            Log.d(TAG, "[buildNotification]   - rid: " + (ejson != null && ejson.rid != null ? ejson.rid : "[null]"));
+            Log.d(TAG, "[buildNotification]   - type: " + (ejson != null && ejson.type != null ? ejson.type : "[null]"));
+            Log.d(TAG, "[buildNotification]   - host: " + (ejson != null && ejson.host != null ? "[present]" : "[null]"));
+            Log.d(TAG, "[buildNotification]   - messageId: " + (ejson != null && ejson.messageId != null ? ejson.messageId : "[null]"));
+            Log.d(TAG, "[buildNotification]   - notificationType: " + (ejson != null && ejson.notificationType != null ? ejson.notificationType : "[null]"));
+            Log.d(TAG, "[buildNotification]   - notId: " + mBundle.getString("notId"));
+            Log.d(TAG, "[buildNotification]   - title: " + (title != null ? "[present]" : "[null]"));
+            Log.d(TAG, "[buildNotification]   - message: " + (message != null ? "[present, length=" + message.length() + "]" : "[null]"));
+        }

app/sagas/state.js (1)

32-35: Consider yielding the pending notification check for better error handling.

The current implementation fires checkPendingNotification() asynchronously without yielding, which means:

• The saga continues immediately without waiting for completion
• Errors are only logged, not handled by the saga's error boundaries
• The operation races with setUserPresenceOnline()

While the fire-and-forget approach prevents blocking, it reduces visibility into failures and makes testing harder.

💡 Alternative: Yield the call for saga-managed execution

-		// Check for pending notification when app comes to foreground (Android - notification tap while in background)
-		checkPendingNotification().catch((e) => {
-			log('[state.js] Error checking pending notification:', e);
-		});
+		// Check for pending notification when app comes to foreground (Android - notification tap while in background)
+		try {
+			yield checkPendingNotification();
+		} catch (e) {
+			log('[state.js] Error checking pending notification:', e);
+		}

This approach:

• Maintains saga control flow and error boundaries
• Still doesn't block setUserPresenceOnline() (saga effects are sequential but the check should be fast)
• Improves testability

If the fire-and-forget behavior is intentional (to avoid any delay), consider documenting why.

app/lib/notifications/push.ts (2)

280-295: Extract shared notification transformation logic to reduce duplication.

This payload transformation to INotification format is duplicated in app/lib/notifications/index.ts (lines 109-124). Consider extracting a shared helper function to avoid maintaining the same mapping logic in two places.

♻️ Suggested helper function

Create a shared helper in a common location:

// e.g., in push.ts or a separate helper file
export const transformNativePayloadToNotification = (data: Record<string, any>): INotification => ({
  payload: {
    message: data.message || '',
    style: data.style || '',
    ejson: data.ejson || '',
    collapse_key: data.collapse_key || '',
    notId: data.notId || '',
    msgcnt: data.msgcnt || '',
    title: data.title || '',
    from: data.from || '',
    image: data.image || '',
    soundname: data.soundname || '',
    action: data.action
  },
  identifier: data.notId || ''
});

Then use it in both files:

- const transformed: INotification = {
-   payload: {
-     message: notificationData.message || '',
-     // ... all the fields
-   },
-   identifier: notificationData.notId || ''
- };
+ const transformed = transformNativePayloadToNotification(notificationData);

---

309-325: Consider simplifying the promise chain for clarity.

The double .then().catch() chain works correctly, but could be clearer. The second .catch() at line 322 only handles errors from the fallback logic (lines 314-320), while the first .catch() (line 305) handles native module errors. This structure is correct but slightly confusing to read.

♻️ Optional: Use async/await for cleaner flow

// Alternative using async/await (would need to wrap in async IIFE)
try {
  const pendingNotification = await NativePushNotificationModule.getPendingNotification();
  if (pendingNotification) {
    const notificationData = JSON.parse(pendingNotification);
    return transformNativePayloadToNotification(notificationData);
  }
} catch (e) {
  console.error('[push.ts] Error getting pending notification:', e);
}

// Fallback to expo-notifications
const lastResponse = Notifications.getLastNotificationResponse();
return lastResponse ? transformNotificationResponse(lastResponse) : null;

app/lib/notifications/index.ts (1)

103-104: Consider using static import for consistency with push.ts.

The dynamic require() here contrasts with the static import in push.ts (line 10). Since the function is already guarded by Platform.OS === 'android', a static import at the top of the file with the same guard pattern would be more consistent and bundler-friendly.

♻️ Use static import pattern

+ import NativePushNotificationModule from '../native/NativePushNotificationAndroid';

  export const checkPendingNotification = async (): Promise<void> => {
    if (Platform.OS === 'android') {
      try {
-       const NativePushNotificationModule = require('../native/NativePushNotificationAndroid').default;
        if (NativePushNotificationModule) {

android/app/src/main/java/chat/rocket/reactnative/notification/PushNotificationModule.kt (1)

55-65: Consider adding debug logging for error visibility.

The empty catch block silently swallows exceptions. While acceptable for cleanup operations, adding a log statement would aid debugging if issues arise.

♻️ Add error logging

  @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("PushNotificationModule", "Failed to clear pending notification", e)
      }
  }

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.

app/lib/notifications/push.ts (1)

218-274: Consider simplifying the promise chain structure.

The promise chain has a nested .then().catch().then().catch() pattern that's harder to follow. The second .catch() on line 270 is likely unreachable since getLastNotificationResponse() is synchronous and line 257's .then() doesn't throw.

The transformation logic correctly maps all INotification fields per the interface definition.

♻️ Suggested simplification using async/await

-	// First check native module for stored notification data (Android - when notification was created natively)
-	if (Platform.OS === 'android' && NativePushNotificationModule) {
-		return NativePushNotificationModule.getPendingNotification()
-			.then(pendingNotification => {
-				if (pendingNotification) {
-					try {
-						// Parse the stored notification data
-						const notificationData = JSON.parse(pendingNotification);
-
-						// Transform to INotification format
-						const transformed: 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 || ''
-						};
-
-						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;
-				}
-
-				// Fallback to expo-notifications (for iOS or if native module doesn't have data)
-				const lastResponse = Notifications.getLastNotificationResponse();
-				if (lastResponse) {
-					return transformNotificationResponse(lastResponse);
-				}
-
-				return null;
-			})
-			.catch(e => {
-				console.error('[push.ts] Error in promise chain:', e);
-				return null;
-			});
-	}
+	// First check native module for stored notification data (Android - when notification was created natively)
+	if (Platform.OS === 'android' && NativePushNotificationModule) {
+		return (async () => {
+			try {
+				const pendingNotification = await NativePushNotificationModule.getPendingNotification();
+				if (pendingNotification) {
+					try {
+						const notificationData = JSON.parse(pendingNotification);
+						return {
+							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 || ''
+						} as INotification;
+					} 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();
+			return lastResponse ? transformNotificationResponse(lastResponse) : null;
+		})();
+	}

android/app/src/main/java/chat/rocket/reactnative/notification/PushNotificationModule.kt (1)

46-61: Potential race condition: clearing before resolving the promise.

The notification is cleared (line 54) before the promise is resolved (line 57). If the JS side encounters an error after receiving the notification but before processing it, the data is lost. Consider clearing only after successful JS-side processing, or document this as intentional "at-most-once" delivery.

Additionally, using apply() for the remove (async) while using commit() for store (sync) creates an inconsistency. If the clear fails silently, duplicate processing could occur on next app launch.

♻️ Consider returning the value and letting JS trigger clear explicitly

If at-most-once semantics are acceptable, the current approach is fine. Otherwise, keep clearPendingNotification() as the explicit cleanup call from JS after successful processing:

     @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()
-            }
+            // Note: Caller should invoke clearPendingNotification() after successful processing
 
             promise.resolve(notification)
         } catch (e: Exception) {
             promise.reject("ERROR", e.message)
         }
     }

android/app/src/main/java/chat/rocket/reactnative/notification/NotificationIntentHandler.kt (1)

70-71: Consider reusing Gson instance.

Two GsonBuilder().create() calls create separate Gson instances. While this is fine for infrequent notification handling, a single shared instance in the companion object would be slightly more efficient.

♻️ Optional: Share Gson instance

 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

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.