chore: Merge 4.68.2 into single-server #6907
Conversation
diegolmello
requested a deployment
to
approve_e2e_testing
GitHub Actions
Waiting
WalkthroughVersion bump to 4.68.2 across platforms (Android, iOS, JavaScript). Refactored Android push notification handling to remove React context dependency, replacing async E2ENotificationProcessor with synchronous NotificationIntentHandler and new TurboModule infrastructure. Added markdown timestamp component and hardened encryption validation. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant System as Android System
participant MainActivity
participant IntentHandler as NotificationIntentHandler
participant PushModule as PushNotificationModule
participant SharedPref as SharedPreferences
participant ReactNative as React Native App
rect rgb(200, 220, 255)
Note over User,ReactNative: New Flow: Notification Received While App in Background
User->>System: Tap notification
System->>MainActivity: startActivity() with intent
MainActivity->>IntentHandler: handleIntent(context, intent)
alt Video Conf Intent
IntentHandler->>IntentHandler: handleVideoConfIntent()
IntentHandler->>PushModule: storePendingAction()
else Regular Notification
IntentHandler->>IntentHandler: handleNotificationIntent()
IntentHandler->>PushModule: storePendingNotification(jsonData)
PushModule->>SharedPref: write notification JSON
end
Note over User,ReactNative: App Comes to Foreground
System->>ReactNative: onResume / app foreground
ReactNative->>PushModule: getPendingNotification()
PushModule->>SharedPref: read notification JSON
SharedPref-->>PushModule: return JSON string
PushModule-->>ReactNative: return notification data
ReactNative->>ReactNative: process notification via onNotification()
ReactNative->>PushModule: clearPendingNotification()
PushModule->>SharedPref: clear notification
end
sequenceDiagram
participant PushHandler as CustomPushNotification
participant Encrypt as Encryption
participant Bundle as Bundle Data
rect rgb(220, 240, 220)
Note over PushHandler,Bundle: New Flow: Direct E2E Processing (No React Waiting)
PushHandler->>PushHandler: onReceived(bundle)
PushHandler->>PushHandler: check if E2E encrypted
alt E2E Encrypted
PushHandler->>Encrypt: decryptRoomKey(e2eKey)
Encrypt->>Encrypt: readRoom(), readUserKey()
Encrypt->>Encrypt: RSA decrypt sessionKey
Encrypt->>Encrypt: parse and validate key format
Encrypt-->>PushHandler: return decrypted key
PushHandler->>Encrypt: decryptMessage(encrypted, key)
Encrypt-->>PushHandler: return plaintext message
PushHandler->>Bundle: update with decrypted msg
else Not E2E
PushHandler->>Bundle: use plaintext
end
PushHandler->>PushHandler: buildNotification(bundle)
PushHandler->>PushHandler: showNotification()
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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 |
diegolmello
requested a deployment
to
official_android_build
GitHub Actions
Waiting
diegolmello
requested a deployment
to
experimental_ios_build
GitHub Actions
Waiting
diegolmello
requested a deployment
to
official_ios_build
GitHub Actions
Waiting
diegolmello
requested a deployment
to
experimental_android_build
GitHub Actions
Waiting
![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
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)
206-245:notificationMessagesis not thread-safe:ConcurrentHashMap+ArrayListcan still crash (CME/data races).
notificationMessagesis concurrent, but each value is a plainArrayList. You add to it (Line 239) and iterate it elsewhere (e.g., cancel + style building). Under concurrent notifications, this can lead toConcurrentModificationExceptionand/or lost updates.One straightforward approach (synchronized list + safe iteration)
+import java.util.Collections; @@ private void showNotification(Bundle bundle, Ejson ejson, String notId) { - // 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 list stored in concurrent map) + final List<Bundle> messages = + notificationMessages.computeIfAbsent(notId, k -> Collections.synchronizedList(new ArrayList<>())); @@ - notificationMessages.get(notId).add(bundle); + messages.add(new Bundle(bundle)); if (ENABLE_VERBOSE_LOGS) { - Log.d(TAG, "[After add] notificationMessages[" + notId + "].size=" + notificationMessages.get(notId).size()); + Log.d(TAG, "[After add] notificationMessages[" + notId + "].size=" + messages.size()); } postNotification(Integer.parseInt(notId)); } }And then, whenever iterating
List<Bundle> bundlesfetched from the map, wrap the loop insynchronized (bundles) { ... }(required forCollections.synchronizedList(...)).
297-348: If server load fails, you may post a blank fallback notification; also consider copying extras to avoid later mutation.
- In
loadNotificationAndProcess(), whenbundle == nullyou log “display placeholder”, butbuildNotification()will use whatever title/message were present in the originalmBundle(often empty for message-id-only), producing a low-quality or blank notification.intent.putExtras(bundle)passes a mutableBundleinstance; a defensive copy avoids surprising later changes.Low-impact hardening
private Notification.Builder buildNotification(int notificationId) { final Bundle bundle = mBundle; @@ - intent.putExtras(bundle); + intent.putExtras(new Bundle(bundle)); @@ - notification - .setContentTitle(title) - .setContentText(message) + notification + .setContentTitle(title != null ? title : mContext.getString(R.string.app_name)) + .setContentText(message != null ? message : "") .setContentIntent(pendingIntent) .setPriority(Notification.PRIORITY_HIGH) .setDefaults(Notification.DEFAULT_ALL) .setAutoCancel(true);
🤖 Fix all issues with AI agents
In @android/app/src/main/java/chat/rocket/reactnative/MainActivity.kt:
- Line 34: Notification intents are currently ignored for deep links because
NotificationIntentHandler.handleIntent calls handleNotificationIntent which
returns early when the "ejson" extra is empty, so ACTION_VIEW intents or intents
with a data URI (deep links) are never processed; update
NotificationIntentHandler (specifically handleIntent and
handleNotificationIntent) to detect Intent.ACTION_VIEW or a non-null intent.data
URI (in addition to videoConfAction and ejson) and extract/route the deep link
URI for processing, or if deep linking is not intended remove the ACTION_VIEW
deep link intent-filters from AndroidManifest.xml so deep link intents are not
delivered to this handler.
In
@android/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.java:
- Around line 169-200: handleE2ENotification currently calls
Encryption.shared.decryptMessage without guarding against exceptions and uses
Encryption.shared directly, which can cause a race and drop the notification;
cache Encryption.shared to a local variable, wrap the call to
decryptMessage(ejson, mContext) in a try/catch, and on any thrown exception or
null result set a fallback message ("Encrypted message"), update mBundle inside
the synchronized block, and always call showNotification(bundle, ejson, notId)
so the user sees a fallback notification.
In
@android/app/src/main/java/chat/rocket/reactnative/notification/NotificationIntentHandler.kt:
- Line 44: The current early return at "val event =
intent.getStringExtra(\"event\") ?: return true" in NotificationIntentHandler.kt
silently skips processing when videoConfAction is true but event is null; change
this to not return immediately—log a warning including intent details (use the
class's logger or Android Log) and allow execution to fall through to
handleNotificationIntent (or call the regular notification handling path) so the
notification is still processed when event is missing; update the branch around
the videoConfAction check to handle a null event explicitly instead of returning
true.
🧹 Nitpick comments (11)
app/containers/markdown/components/Inline.tsx (1)
25-79: Add key props to elements rendered in the map.The map function renders multiple JSX elements without key props, which can impact React's reconciliation performance. All elements returned from the switch cases should include a
key={index}prop.🔑 Add key prop to fix React reconciliation
Each case should include
key={index}on the returned element. Example for the new TIMESTAMP case:case 'TIMESTAMP': - return <Timestamp value={block.value} />; + return <Timestamp key={index} value={block.value} />;Apply similar changes to all other cases (IMAGE, PLAIN_TEXT, BOLD, STRIKE, ITALIC, LINK, MENTION_USER, EMOJI, MENTION_CHANNEL, INLINE_CODE, INLINE_KATEX).
app/containers/markdown/components/Timestamp.tsx (2)
17-49: Consider validating the parsed timestamp.While dayjs handles invalid dates gracefully, explicitly validating the
parseIntresult would make the error handling more predictable and catch malformed timestamps earlier.♻️ Optional improvements
Add timestamp validation:
const formatDate = React.useMemo(() => { - const timestamp = parseInt(value.timestamp) * 1000; + const parsed = parseInt(value.timestamp, 10); + if (isNaN(parsed)) { + return 'Invalid Date'; + } + const timestamp = parsed * 1000;Optional: Refactor format logic with a mapping object for better maintainability:
const formatDate = React.useMemo(() => { const parsed = parseInt(value.timestamp, 10); if (isNaN(parsed)) { return 'Invalid Date'; } const timestamp = parsed * 1000; const formatMap = { 't': 'hh:mm A', 'T': 'hh:mm:ss A', 'd': 'MM/DD/YYYY', 'D': 'dddd, MMM DD, YYYY', 'f': 'dddd, MMM DD, YYYY hh:mm A', 'F': 'dddd, MMM DD, YYYY hh:mm:ss A' }; if (value.format === 'R') { return dayjs(timestamp).fromNow(); } const format = formatMap[value.format]; return format ? dayjs(timestamp).format(format) : 'Invalid Date'; }, [value]);
51-54: Consider extracting timestamp parsing logic.The timestamp parsing logic is duplicated from the
formatDatememoization (line 18). Extracting it to a helper or memoizing the parsed value separately would improve maintainability.♻️ Reduce duplication
+const parsedTimestamp = React.useMemo(() => { + const parsed = parseInt(value.timestamp, 10); + return isNaN(parsed) ? null : parsed * 1000; +}, [value.timestamp]); + const formatDate = React.useMemo(() => { - const timestamp = parseInt(value.timestamp) * 1000; + if (parsedTimestamp === null) { + return 'Invalid Date'; + } if (value.format === 't') { - return dayjs(timestamp).format('hh:mm A'); + return dayjs(parsedTimestamp).format('hh:mm A'); } // ... rest of format logic -}, [value]); +}, [parsedTimestamp, value.format]); const handlePress = React.useCallback(() => { - const message = dayjs(parseInt(value.timestamp) * 1000).format('dddd, MMM DD, YYYY hh:mm A'); + if (parsedTimestamp === null) return; + const message = dayjs(parsedTimestamp).format('dddd, MMM DD, YYYY hh:mm A'); EventEmitter.emit(LISTENER, { message }); -}, [value.timestamp]); +}, [parsedTimestamp]);app/lib/notifications/push.ts (2)
228-243: Consider extracting a shared helper for notification transformation.The transformation logic here duplicates the pattern used in
transformNotificationResponse. Extract a helper function that maps raw notification data toINotificationto reduce duplication and ensure consistency.♻️ Suggested helper extraction
// Add this helper near the top of the file const createNotificationFromData = (data: Record<string, any>, identifier: string): 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 });Then use it in both places:
- const transformed: INotification = { - payload: { - message: notificationData.message || '', - // ... all fields - }, - identifier: notificationData.notId || '' - }; + const transformed = createNotificationFromData(notificationData, notificationData.notId || '');
262-262: Misleading comment: iOS fallback is unreachable in this branch.The comment mentions "for iOS" but this entire block is inside the
Platform.OS === 'android'guard. iOS uses the separate fallback at lines 276-280. Consider updating the comment.📝 Suggested fix
- // Fallback to expo-notifications (for iOS or if native module doesn't have data) + // Fallback to expo-notifications if native module doesn't have dataapp/lib/notifications/index.ts (2)
107-122: Duplicate transformation logic withpush.ts.This transformation code is duplicated from
push.tslines 228-243. Consider extracting a shared helper function in a common location to maintain consistency and reduce duplication.♻️ Suggested approach
Create a shared helper in a common location (e.g.,
app/lib/notifications/helpers.ts):export const createNotificationFromData = (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 import and use in both
push.tsandindex.ts.
101-101: Consider using static import instead of dynamic require.Since this function is already guarded by
Platform.OS === 'android', you could use a static import at the top of the file. Metro bundler handles platform-specific modules, and static imports are easier to analyze for type checking and tree-shaking.android/app/src/main/java/chat/rocket/reactnative/notification/PushNotificationModule.kt (1)
73-75: Consider logging the swallowed exception for debugging.While ignoring errors in a cleanup method is acceptable, logging at debug/warning level would help diagnose issues without affecting functionality.
📝 Suggested improvement
} catch (e: Exception) { - // Ignore errors + // Log but don't propagate - cleanup should not fail the caller + android.util.Log.w("RocketChat.PushNotificationModule", "Error clearing pending notification: ${e.message}") }android/app/src/main/java/chat/rocket/reactnative/notification/NotificationIntentHandler.kt (1)
70-71: Consider caching the Gson instance.
GsonBuilder().create()is called on each notification intent. Since Gson is thread-safe, consider creating a static instance for better performance.♻️ Suggested improvement
class NotificationIntentHandler { companion object { private const val TAG = "RocketChat.NotificationIntentHandler" + private val gson = GsonBuilder().create() // ... methods ... private fun handleVideoConfIntent(context: Context, intent: Intent): Boolean { // ... - val gson = GsonBuilder().create() val jsonData = gson.toJson(data) // ... } private fun handleNotificationIntent(context: Context, intent: Intent) { // ... - val gson = GsonBuilder().create() val jsonData = gson.toJson(notificationData) // ... } } }Also applies to: 139-140
android/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.java (2)
62-72: Consider defensively copyingBundleon assignment to avoid cross-thread/caller mutation surprises.
volatileimproves visibility, butBundleitself is mutable; a caller (or later code) can mutate the same instance after you store it inmBundleor innotificationMessages. Copying on write makes the “snapshot” intent explicit.Proposed hardening
public class CustomPushNotification { @@ - private volatile Bundle mBundle; + private volatile Bundle mBundle; @@ public CustomPushNotification(Context context, Bundle bundle) { this.mContext = context; - this.mBundle = bundle; + this.mBundle = bundle != null ? new Bundle(bundle) : new Bundle(); this.notificationManager = (NotificationManager) context.getSystemService(Context.NOTIFICATION_SERVICE);
78-97: Log message change is fine; consider guaranteeing a user-visible fallback on any exception.Right now an exception in
handleNotification()means no notification is shown at all (only logs). If the goal is resiliency for killed-app scenarios, it’s worth ensuring a minimal fallback notification still posts in the catch path.
📜 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 ignored due to path filters (1)
app/containers/markdown/__snapshots__/Markdown.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (23)
android/app/build.gradleandroid/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/Ejson.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/containers/markdown/Markdown.stories.tsxapp/containers/markdown/components/Inline.tsxapp/containers/markdown/components/Timestamp.tsxapp/lib/native/NativePushNotificationAndroid.tsapp/lib/notifications/index.tsapp/lib/notifications/push.tsapp/sagas/state.jsios/RocketChatRN.xcodeproj/project.pbxprojios/RocketChatRN/Info.plistios/ShareRocketChatRN/Info.plistpackage.json
💤 Files with no reviewable changes (2)
- android/app/src/main/java/chat/rocket/reactnative/notification/E2ENotificationProcessor.java
- android/app/src/main/java/chat/rocket/reactnative/notification/Ejson.java
🧰 Additional context used
🧬 Code graph analysis (4)
app/containers/markdown/Markdown.stories.tsx (1)
app/containers/MessageComposer/components/Toolbar/Markdown.tsx (1)
Markdown(9-46)
app/containers/markdown/components/Timestamp.tsx (5)
app/containers/markdown/Markdown.stories.tsx (1)
Timestamp(160-170)app/theme.tsx (1)
useTheme(29-29)app/lib/methods/helpers/room.ts (1)
formatDate(32-38)app/containers/Toast.tsx (1)
LISTENER(24-24)app/lib/constants/colors.ts (1)
colors(280-302)
app/sagas/state.js (1)
app/lib/notifications/index.ts (1)
checkPendingNotification(98-133)
app/lib/notifications/push.ts (1)
app/definitions/INotification.ts (1)
INotification(1-16)
🪛 Biome (2.1.2)
app/containers/markdown/components/Inline.tsx
[error] 75-75: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
🪛 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)
⏰ 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). (1)
- GitHub Check: ESLint and Test / run-eslint-and-test
🔇 Additional comments (26)
ios/RocketChatRN.xcodeproj/project.pbxproj (1)
3046-3046: ✓ Version bump for NotificationService target looks correct.The MARKETING_VERSION has been updated from 4.68.1 to 4.68.2 in both Debug and Release configurations of the NotificationService target, which aligns with the PR's release version bump. This is a straightforward configuration change with no functional impact.
Also applies to: 3098-3098
android/app/build.gradle (1)
93-93: LGTM! Version bump is consistent with the release.The version update to 4.68.2 aligns with the PR objective and matches the version updates across iOS and the root package.json.
ios/RocketChatRN/Info.plist (1)
31-31: LGTM! iOS version bump is consistent.The CFBundleShortVersionString update to 4.68.2 is correctly synchronized with the Android and package.json version updates.
ios/ShareRocketChatRN/Info.plist (1)
29-29: LGTM! Share extension version updated correctly.The CFBundleShortVersionString update to 4.68.2 maintains version consistency between the share extension and the main application.
package.json (2)
3-3: LGTM! Root package version updated correctly.The version bump to 4.68.2 is consistent with the Android and iOS platform-specific version updates.
8-9: Excellent addition of timezone normalization for tests!Prefixing test commands with
TZ=UTCensures deterministic test execution across different developer environments and CI systems. This is particularly important for any tests involving date/time operations or snapshots that include timestamps.app/containers/markdown/components/Inline.tsx (2)
13-13: LGTM!The Timestamp import is correctly placed and follows the existing import pattern.
74-75: LGTM!The TIMESTAMP case is correctly integrated and follows the established pattern of other block type handlers.
app/containers/markdown/Markdown.stories.tsx (1)
160-170: LGTM!The Timestamp story comprehensively demonstrates all supported format types ('t', 'T', 'd', 'D', 'f', 'F', 'R') and follows the established story structure.
app/containers/markdown/components/Timestamp.tsx (2)
10-12: LGTM!The interface is well-typed with explicit format literals that match Discord's timestamp formatting syntax.
56-62: LGTM!The render implementation correctly applies theming, handles user interaction, and adds intentional spacing around the formatted date for better visual presentation.
android/app/src/main/java/chat/rocket/reactnative/notification/VideoConfBroadcast.kt (1)
51-52: LGTM! Consistent field additions.The added "host" and "callId" fields follow the same pattern as existing fields, using safe null-coalescing to empty strings. This aligns with the centralized notification handling architecture mentioned in the PR.
app/sagas/state.js (1)
32-35: Verify non-blocking call is intentional.The call to
checkPendingNotification()is non-blocking (noyieldorawait), which means the saga continues immediately tosetUserPresenceOnline(). This might be intentional for performance, but consider whether notification handling should complete before updating user presence, especially if the notification needs to navigate or update state.If the current behavior is intentional, the error handling looks appropriate.
Consider these scenarios:
- Does the notification processing need to complete before presence is set online?
- Could race conditions occur if notification handling modifies state while other operations are in progress?
- Should this be
yield call(checkPendingNotification)instead?app/lib/native/NativePushNotificationAndroid.ts (1)
1-9: LGTM! Well-structured TurboModule binding.The TurboModule binding is properly implemented with:
- Correct TypeScript interface extending TurboModule
- Appropriate return types (Promise for async, void for sync)
- Module name 'PushNotificationModule' correctly matches the native implementation
- Clean module registry access
android/app/src/main/java/chat/rocket/reactnative/MainApplication.kt (1)
22-22: No action required—notification processing is independent of ReactContext.CustomPushNotification and RCFirebaseMessagingService do not depend on ReactContext. The notification flow (FCM reception → CustomPushNotification → notification display) uses only Android Context and system services. The TurboModule infrastructure (PushNotificationTurboPackage and PushNotificationModule) properly integrates ReactApplicationContext via the standard TurboReactPackage pattern and does not impact the core notification processing pipeline.
Likely an incorrect or invalid review comment.
app/lib/notifications/push.ts (1)
10-10: LGTM!The import of the native push notification module is correctly added for Android-specific notification handling.
android/app/src/main/java/chat/rocket/reactnative/notification/PushNotificationTurboPackage.kt (1)
1-37: LGTM!The TurboModule package is correctly implemented with proper module registration and metadata configuration.
android/app/src/main/java/chat/rocket/reactnative/notification/Encryption.java (3)
191-201: LGTM! Good defensive check for column existence.The column index validation prevents crashes when the database schema doesn't include expected columns, with appropriate error logging.
248-272: LGTM! Robust key parsing with format flexibility.The dual-format parsing (direct JSON vs double-encoded) with required field validation handles edge cases gracefully.
288-356: LGTM! Well-hardened decryption flow.The comprehensive null checks and try-catch blocks around each crypto operation provide robust error handling and prevent crashes from malformed data.
app/lib/notifications/index.ts (2)
25-34: LGTM! Good error handling for video conf notifications.Wrapping the EJSON parse in try-catch prevents crashes from malformed notification payloads.
48-51: LGTM!Graceful degradation to
appInitwhen required notification fields are missing ensures the app still launches.android/app/src/main/java/chat/rocket/reactnative/notification/PushNotificationModule.kt (2)
24-39: LGTM! Proper synchronous write with error handling.Using
commit()instead ofapply()ensures the write completes before returning, and the success check with explicit error handling is appropriate for critical notification data.
47-61: LGTM!The read-then-clear pattern correctly prevents duplicate notification processing, and using
apply()for the clear operation is appropriate since the data has already been retrieved.android/app/src/main/java/chat/rocket/reactnative/notification/NativePushNotificationSpec.kt (1)
1-24: LGTM!The TurboModule spec is correctly defined with the abstract methods that the implementation class must override.
android/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.java (1)
138-160: Good: snapshottingmBundleinto a localfinal bundlereduces stale reads.This makes the subsequent logic (E2E vs regular) more deterministic within a single processing pass.
| // Handle video conf action from notification | ||
| intent?.let { handleVideoConfIntent(it) } | ||
| // Handle notification intents | ||
| intent?.let { NotificationIntentHandler.handleIntent(this, it) } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "NotificationIntentHandler.kt" -type fRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 171
🏁 Script executed:
wc -l ./android/app/src/main/java/chat/rocket/reactnative/notification/NotificationIntentHandler.ktRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 175
🏁 Script executed:
cat -n ./android/app/src/main/java/chat/rocket/reactnative/notification/NotificationIntentHandler.ktRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 7595
🏁 Script executed:
sed -n '1,60p' android/app/src/main/java/chat/rocket/reactnative/MainActivity.ktRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 1713
🏁 Script executed:
rg -n "ACTION_VIEW|deep.*link|scheme|data.*intent|android:scheme" --type=kotlin android/app/src/main/Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 358
🏁 Script executed:
find . -name "AndroidManifest.xml" -path "*/android/app/src/main/*"Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 120
🏁 Script executed:
grep -A 20 "MainActivity" ./android/app/src/main/AndroidManifest.xmlRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 1407
🏁 Script executed:
rg -n "ACTION_VIEW|handleIntent" --type=kotlin android/app/src/main/java/chat/rocket/reactnative/Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 856
🏁 Script executed:
rg -n "intent.data|ACTION_VIEW|getAction|scheme" --type=kotlin android/app/src/main/java/chat/rocket/reactnative/ -A 3 -B 1Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 1987
Deep link intents are not being processed by NotificationIntentHandler.
The centralized notification handling correctly processes video conference notifications (via videoConfAction extra) and regular push notifications (via ejson extra). However, deep link intents registered in AndroidManifest.xml (ACTION_VIEW with https://go.rocket.chat paths) are silently dropped because they lack the ejson extra and are not checked for ACTION_VIEW or intent data. The handleNotificationIntent method returns early at line 87-92 when ejson is null/empty, which excludes all deep links from processing.
Either:
- Extend
NotificationIntentHandlerto handle ACTION_VIEW intents and extract/process the intent data URI - Or remove the deep link intent-filters from the manifest if deep linking is not supported
🤖 Prompt for AI Agents
In @android/app/src/main/java/chat/rocket/reactnative/MainActivity.kt at line
34, Notification intents are currently ignored for deep links because
NotificationIntentHandler.handleIntent calls handleNotificationIntent which
returns early when the "ejson" extra is empty, so ACTION_VIEW intents or intents
with a data URI (deep links) are never processed; update
NotificationIntentHandler (specifically handleIntent and
handleNotificationIntent) to detect Intent.ACTION_VIEW or a non-null intent.data
URI (in addition to videoConfAction and ejson) and extract/route the deep link
URI for processing, or if deep linking is not intended remove the ACTION_VIEW
deep link intent-filters from AndroidManifest.xml so deep link intents are not
delivered to this handler.
| /** | ||
| * Handles E2E encrypted notifications by delegating to the async processor. | ||
| * Handles E2E encrypted notifications | ||
| */ | ||
| private void handleE2ENotification(Bundle bundle, Ejson ejson, String notId) { | ||
| // 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); | ||
| if (Encryption.shared == null) { | ||
| Log.e(TAG, "Encryption singleton is null, cannot decrypt E2E notification"); | ||
| bundle.putString("message", "Encrypted message"); | ||
| synchronized(this) { | ||
| mBundle = bundle; | ||
| ejson = safeFromJson(bundle.getString("ejson", "{}"), Ejson.class); | ||
| showNotification(bundle, ejson, notId); | ||
| } else { | ||
| Log.w(TAG, "E2E decryption failed for notification"); | ||
| } | ||
| showNotification(bundle, ejson, notId); | ||
| return; | ||
| } | ||
|
|
||
| // Slow path: wait for React context asynchronously | ||
| Log.i(TAG, "Waiting for React context to decrypt E2E notification"); | ||
| String decrypted = Encryption.shared.decryptMessage(ejson, mContext); | ||
|
|
||
| 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"); | ||
| } | ||
|
|
||
| @Override | ||
| public void onTimeout(Bundle originalBundle, Ejson originalEjson, String notificationId) { | ||
| Log.w(TAG, "Timeout waiting for React context for E2E notification"); | ||
| } | ||
| if (decrypted != null) { | ||
| bundle.putString("message", decrypted); | ||
| synchronized(this) { | ||
| mBundle = bundle; | ||
| } | ||
| ); | ||
|
|
||
| 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 | ||
| bundle.putString("message", "Encrypted message"); | ||
| synchronized(this) { | ||
| mBundle = bundle; | ||
| } | ||
| showNotification(bundle, ejson, notId); | ||
| } | ||
| } |
There was a problem hiding this comment.
Wrap decrypt in try/catch and always show a fallback notification (decrypt can throw).
decryptMessage(...) is an external boundary; if it throws, you currently drop into onReceived()’s catch and do not notify the user. Also, caching Encryption.shared into a local reduces TOCTOU between the null-check and the call.
Proposed fix
private void handleE2ENotification(Bundle bundle, Ejson ejson, String notId) {
- if (Encryption.shared == null) {
+ final Encryption encryption = Encryption.shared;
+ if (encryption == null) {
Log.e(TAG, "Encryption singleton is null, cannot decrypt E2E notification");
bundle.putString("message", "Encrypted message");
synchronized(this) {
mBundle = bundle;
}
showNotification(bundle, ejson, notId);
return;
}
-
- String decrypted = Encryption.shared.decryptMessage(ejson, mContext);
+
+ final String decrypted;
+ try {
+ decrypted = encryption.decryptMessage(ejson, mContext);
+ } catch (Exception ex) {
+ Log.w(TAG, "E2E decryption threw, showing fallback notification", ex);
+ bundle.putString("message", "Encrypted message");
+ synchronized (this) { mBundle = bundle; }
+ showNotification(bundle, ejson, notId);
+ return;
+ }🤖 Prompt for AI Agents
In
@android/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.java
around lines 169 - 200, handleE2ENotification currently calls
Encryption.shared.decryptMessage without guarding against exceptions and uses
Encryption.shared directly, which can cause a race and drop the notification;
cache Encryption.shared to a local variable, wrap the call to
decryptMessage(ejson, mContext) in a try/catch, and on any thrown exception or
null result set a fallback message ("Encrypted message"), update mBundle inside
the synchronized block, and always call showNotification(bundle, ejson, notId)
so the user sees a fallback notification.
| } | ||
|
|
||
| val notificationId = intent.getIntExtra("notificationId", 0) | ||
| val event = intent.getStringExtra("event") ?: return true |
There was a problem hiding this comment.
Silent return when event is missing may leave notification unhandled.
When videoConfAction is true but event is null, the method returns true without processing, and handleNotificationIntent is skipped. Consider logging a warning or falling through to regular notification handling.
💡 Suggested improvement
- val event = intent.getStringExtra("event") ?: return true
+ val event = intent.getStringExtra("event")
+ if (event == null) {
+ Log.w(TAG, "Video conf intent missing 'event' extra, skipping")
+ return true
+ }📝 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.
| val event = intent.getStringExtra("event") ?: return true | |
| val event = intent.getStringExtra("event") | |
| if (event == null) { | |
| Log.w(TAG, "Video conf intent missing 'event' extra, skipping") | |
| return true | |
| } |
🤖 Prompt for AI Agents
In
@android/app/src/main/java/chat/rocket/reactnative/notification/NotificationIntentHandler.kt
at line 44, The current early return at "val event =
intent.getStringExtra(\"event\") ?: return true" in NotificationIntentHandler.kt
silently skips processing when videoConfAction is true but event is null; change
this to not return immediately—log a warning including intent details (use the
class's logger or Android Log) and allow execution to fall through to
handleNotificationIntent (or call the regular notification handling path) so the
notification is still processed when event is missing; update the branch around
the videoConfAction check to handle a null event explicitly instead of returning
true.
| val stringValue = value.toString() | ||
| // Only include if it's a reasonable string representation (not object reference) | ||
| if (!stringValue.startsWith("android.") && !stringValue.contains("@")) { | ||
| notificationData[key] = stringValue | ||
| } else { | ||
| Log.w(TAG, "Skipping non-serializable extra: $key (type: ${value.javaClass.simpleName})") | ||
| } |
There was a problem hiding this comment.
Fragile heuristic for detecting object references.
The check !stringValue.startsWith("android.") && !stringValue.contains("@") might produce false positives for legitimate string values containing "@" (like email addresses in extras). Consider using more specific type checks or documenting this limitation.
💡 Alternative approach
else -> {
// Only include known safe types that have meaningful toString() representations
when (value) {
is CharSequence -> notificationData[key] = value.toString()
is Number -> notificationData[key] = value
else -> Log.w(TAG, "Skipping non-serializable extra: $key (type: ${value.javaClass.simpleName})")
}
}
Proposed changes
Issue(s)
How to test or reproduce
Screenshots
Types of changes
Checklist
Further comments
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.