Skip to content

Conversation

@diegolmello
Copy link
Member

@diegolmello diegolmello commented Jan 8, 2026

Proposed changes

Issue(s)

How to test or reproduce

Screenshots

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • Improvement (non-breaking change which improves a current function)
  • New feature (non-breaking change which adds functionality)
  • Documentation update (if none of the other choices apply)

Checklist

  • I have read the CONTRIBUTING doc
  • I have signed the CLA
  • Lint and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works (if applicable)
  • I have added necessary documentation (if applicable)
  • Any dependent changes have been merged and published in downstream modules

Further comments

Summary by CodeRabbit

  • Version Update

    • App version set to 4.68.2
  • New Features

    • Timestamp formatting support in messages with multiple date/time formats
  • Improvements

    • Android: native pending-notification pathway and more reliable notification tap handling
    • Hardened end-to-end encryption parsing and safer decryption/error handling
  • Tests

    • Test scripts updated to run with TZ=UTC

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 8, 2026

Walkthrough

Centralizes Android notification intent handling into a NotificationIntentHandler, replaces legacy async E2E processor with synchronous/deferred flows, adds a TurboModule-based pending-notification storage/read path, hardens encryption parsing, updates JS to check pending Android notifications on foreground, and adds markdown timestamp rendering. Version bumped to 4.68.2.

Changes

Cohort / File(s) Summary
Version Updates
android/app/build.gradle, ios/RocketChatRN.xcodeproj/project.pbxproj, ios/RocketChatRN/Info.plist, ios/ShareRocketChatRN/Info.plist, package.json
Updated app version strings to 4.68.2 and set Jest tests to run with TZ=UTC.
Activity / App wiring
android/app/src/main/java/chat/rocket/reactnative/MainActivity.kt, android/app/src/main/java/chat/rocket/reactnative/MainApplication.kt
Delegate notification intents to NotificationIntentHandler; remove prior video-conf-specific intent handling and ReactContext listener wiring; add PushNotificationTurboPackage.
Intent handler & TurboModule infra
android/app/src/main/java/chat/rocket/reactnative/notification/NotificationIntentHandler.kt, .../PushNotificationTurboPackage.kt, .../NativePushNotificationSpec.kt, .../PushNotificationModule.kt
New centralized intent handler that serializes intent extras, new TurboReactPackage and TurboModule spec, and a PushNotificationModule that stores/reads pending notification JSON in SharedPreferences.
Custom notification flow removed / simplified
android/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.java, android/app/src/main/java/chat/rocket/reactnative/notification/E2ENotificationProcessor.java, .../Ejson.java
Removed ReactApplicationContext waiting logic; simplified bundle handling; removed the async E2ENotificationProcessor; removed unused RN Callback shim.
Encryption robustness
android/app/src/main/java/chat/rocket/reactnative/notification/Encryption.java
Added defensive checks, improved parsing for double-encoded keys, guarded base64/RSA operations, explicit algorithm determination (v1/v2) and stronger null/error handling.
Video conference payload
android/app/src/main/java/chat/rocket/reactnative/notification/VideoConfBroadcast.kt
Added host and callId fields to video-conf payload sent to JS.
JS native binding & notification path
app/lib/native/NativePushNotificationAndroid.ts, app/lib/notifications/index.ts, app/lib/notifications/push.ts, app/sagas/state.js
Added TurboModule TS binding; JS now checks Android native pending notification on foreground (checkPendingNotification), parses native JSON into INotification, hardened ejson parsing, and invokes onNotification.
Markdown timestamp UI
app/containers/markdown/components/Timestamp.tsx, app/containers/markdown/components/Inline.tsx, app/containers/markdown/Markdown.stories.tsx
New Timestamp component with multiple dayjs formats, Inline renders TIMESTAMP blocks, added story demonstrating formats.

Sequence Diagram(s)

sequenceDiagram
    participant MainActivity
    participant NotificationIntentHandler
    participant VideoConfModule
    participant PushNotificationModule
    participant SharedPreferences
    participant JS

    MainActivity->>NotificationIntentHandler: handleIntent(context,intent)
    activate NotificationIntentHandler

    alt videoConfAction present
        NotificationIntentHandler->>VideoConfModule: build video-conf payload (rid,event,host,callId,caller)
        VideoConfModule->>SharedPreferences: store pending action (JSON)
    else regular notification
        NotificationIntentHandler->>PushNotificationModule: extract serializable extras -> JSON
        PushNotificationModule->>SharedPreferences: storePendingNotification(JSON)
    end

    deactivate NotificationIntentHandler

    Note over JS,MainActivity: App foreground / auth completes
    JS->>PushNotificationModule: checkPendingNotification()
    PushNotificationModule->>SharedPreferences: read pendingNotification
    SharedPreferences-->>JS: return JSON (or null)
    JS->>JS: parse JSON -> INotification
    JS->>JS: onNotification(INotification)
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Possibly related PRs

Suggested reviewers

  • OtavioStasiak

Poem

🐰 I hopped through intents, tidy and neat,

Stored JSON crumbs for the JS to meet.
Timestamps now tick in markdown delight,
Encryption guarded through tunnelled night.
A tiny rabbit cheers this tidy flight 🥕

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'chore: Merge 4.68.2 into master' clearly and specifically describes the main purpose of the pull request—merging a release branch into the master branch.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch 4.68.2-master

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@diegolmello diegolmello changed the base branch from master to develop January 8, 2026 19:48
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/Encryption.java (2)

127-133: Thread safety issue: mutable instance fields on singleton.

The keyId and algorithm fields are modified in decryptRoomKey() and later read in encryptMessageContent(). Since Encryption.shared is a singleton that can be accessed from multiple threads (e.g., concurrent notification handling), this creates a race condition where one thread's values could be overwritten by another.

RoomKeyResult already returns algorithm, but encryptMessageContent ignores it and uses the instance field instead. Consider:

  1. Adding keyId to RoomKeyResult
  2. Using the returned values directly instead of instance fields
Suggested approach
 class RoomKeyResult {
     String decryptedKey;
     String algorithm;
+    String keyId;
     
-    RoomKeyResult(String decryptedKey, String algorithm) {
+    RoomKeyResult(String decryptedKey, String algorithm, String keyId) {
         this.decryptedKey = decryptedKey;
         this.algorithm = algorithm;
+        this.keyId = keyId;
     }
 }

Then in encryptMessageContent, use roomKeyResult.algorithm and roomKeyResult.keyId instead of the instance fields.


457-472: Use returned values instead of instance fields here.

This is where the thread safety issue manifests. Lines 457 and 470 use the instance fields algorithm and keyId instead of the values from roomKeyResult. After adding keyId to RoomKeyResult, update this code:

Suggested fix
-            if ("rc.v2.aes-sha2".equals(algorithm)) {
+            if ("rc.v2.aes-sha2".equals(roomKeyResult.algorithm)) {
                 // V2 format: Use AES-GCM with 12-byte IV
                 bytes = new byte[12];
                 random.nextBytes(bytes);
                 encryptedData = AESCrypto.INSTANCE.encryptGcmBase64(
                     Base64.encodeToString(cypher.getBytes("UTF-8"), Base64.NO_WRAP), 
                     e2eKey, 
                     CryptoUtils.INSTANCE.bytesToHex(bytes)
                 );
                 
                 return new EncryptionContent(
-                    algorithm,
+                    roomKeyResult.algorithm,
                     encryptedData,
-                    keyId,
+                    roomKeyResult.keyId,
                     Base64.encodeToString(bytes, Base64.NO_WRAP)
                 );
🤖 Fix all issues with AI agents
In @app/containers/markdown/components/Timestamp.tsx:
- Line 18: The code uses parseInt(value.timestamp) without a radix and
duplicates the seconds-to-milliseconds conversion; update both occurrences to
parseInt(value.timestamp, 10) and extract the logic into a single memoized value
(e.g., const timestampMs = useMemo(() => parseInt(value.timestamp, 10) * 1000,
[value.timestamp])) inside the Timestamp component so both references use
timestampMs, eliminating duplication and ensuring base‑10 parsing.

In @package.json:
- Line 3: The package.json "version" field was lowered from 4.69.0 to 4.68.2;
confirm whether this downgrade is intentional (e.g., a backport/hotfix from a
maintenance branch) and either restore the correct semver (set "version" back to
4.69.0 or the appropriate next release) or add documentation in the PR/repo
describing the release branching strategy and the reason for the backport;
reference the "version" key in package.json and update the PR description or
RELEASE_NOTES/README to make the decision explicit.
🧹 Nitpick comments (11)
android/app/src/main/java/chat/rocket/reactnative/notification/Encryption.java (1)

489-489: Use TAG constant for consistent logging.

This line uses a hardcoded string "[ROCKETCHAT][E2E]" while the rest of the class uses the TAG constant ("RocketChat.E2E"). This inconsistency can make log filtering more difficult.

Suggested fix
-            Log.e("[ROCKETCHAT][E2E]", Log.getStackTraceString(e));
+            Log.e(TAG, "Encryption failed", e);
app/containers/markdown/components/Inline.tsx (1)

74-75: Implementation looks good; note pre-existing key issue.

The TIMESTAMP case correctly delegates to the Timestamp component and is consistent with the existing switch cases.

The static analysis correctly identifies that rendered components lack a key prop. However, this is a pre-existing pattern issue affecting all cases in this map (lines 25-79), not specific to this addition. Consider adding unique keys to all rendered elements in a future refactor to satisfy React's reconciliation requirements.

app/containers/markdown/components/Timestamp.tsx (2)

20-48: Refactor to if-else if chain for efficiency.

The sequential if statements evaluate all conditions even after finding a match. Use an if-else if chain or switch statement to short-circuit after the first match.

♻️ Proposed refactor
-		if (value.format === 't') {
+		if (value.format === 't') {
 			return dayjs(timestamp).format('hh:mm A');
-		}
-
-		if (value.format === 'T') {
+		} else if (value.format === 'T') {
 			return dayjs(timestamp).format('hh:mm:ss A');
-		}
-
-		if (value.format === 'd') {
+		} else if (value.format === 'd') {
 			return dayjs(timestamp).format('MM/DD/YYYY');
-		}
-
-		if (value.format === 'D') {
+		} else if (value.format === 'D') {
 			return dayjs(timestamp).format('dddd, MMM DD, YYYY');
-		}
-
-		if (value.format === 'f') {
+		} else if (value.format === 'f') {
 			return dayjs(timestamp).format('dddd, MMM DD, YYYY hh:mm A');
-		}
-
-		if (value.format === 'F') {
+		} else if (value.format === 'F') {
 			return dayjs(timestamp).format('dddd, MMM DD, YYYY hh:mm:ss A');
-		}
-
-		if (value.format === 'R') {
+		} else if (value.format === 'R') {
 			return dayjs(timestamp).fromNow();
+		} else {
+			return 'Invalid Date';
 		}
-
-		return 'Invalid Date';

56-62: Consider adding accessibility attributes.

The pressable Text element lacks accessibility attributes. Consider adding accessibilityRole="button" and accessibilityLabel to improve the experience for users with screen readers.

♻️ Accessibility enhancement
 	return (
 		<Text
 			style={[sharedStyles.textMedium, { backgroundColor: colors.surfaceSelected, color: colors.fontDefault }]}
+			accessibilityRole="button"
+			accessibilityLabel={`Timestamp: ${formatDate}. Tap to view full date.`}
 			onPress={handlePress}>
 			{` ${formatDate} `}
 		</Text>
 	);
app/sagas/state.js (1)

32-35: Consider using saga-style error handling for consistency.

The current implementation uses promise .catch() while the rest of the saga uses yield with try-catch. This mixing of patterns could miss synchronous errors thrown during the call.

♻️ Suggested refactor for consistency with saga patterns
-		// 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 {
+			checkPendingNotification().catch((e) => {
+				log('[state.js] Error checking pending notification:', e);
+			});
+		} catch (e) {
+			log('[state.js] Error invoking checkPendingNotification:', e);
+		}

Alternatively, if you want to wait for completion within the saga flow:

-		// 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);
+		}
app/lib/native/NativePushNotificationAndroid.ts (1)

1-7: LGTM with optional type safety consideration.

The TurboModule interface is clean and follows standard patterns. The getPendingNotification method returns a JSON string that requires parsing on the JS side.

For future consideration: Using React Native Codegen with a structured return type would provide compile-time type safety and eliminate JSON parsing. However, for this use case, the current string-based approach is acceptable and pragmatic.

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

218-274: Code duplication: Extract INotification transformation helper.

The notification transformation logic at lines 228-243 duplicates the structure from transformNotificationResponse. Consider extracting a shared helper to avoid drift between the two paths.

Additionally, the second .catch() at lines 270-273 is unreachable since the prior .then() at lines 257-269 doesn't throw—it only returns values. This catch block can be removed.

♻️ Suggested refactor
+/**
+ * Transform raw notification data to INotification format
+ */
+const transformRawNotificationData = (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 || ''
+});

 // In the Android native module path:
-						const transformed: INotification = {
-							payload: {
-								message: notificationData.message || '',
-								// ... all fields
-							},
-							identifier: notificationData.notId || ''
-						};
-						return transformed;
+						return transformRawNotificationData(notificationData);

Remove the unreachable catch:

 			.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;
 			});
android/app/src/main/java/chat/rocket/reactnative/notification/PushNotificationModule.kt (1)

46-61: Inconsistent write approach and missing error cause in rejection.

  1. storePendingNotification uses commit() for reliability, but line 54 uses apply() for clearing. If the clear fails silently, the same notification could be re-processed on the next call. Consider using commit() here as well, or at minimum logging failures.

  2. Line 59 rejects the promise without the exception cause, which loses the stack trace on the JS side.

♻️ Suggested improvements
     @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 cleared = prefs.edit().remove(KEY_PENDING_NOTIFICATION).commit()
+                if (!cleared) {
+                    android.util.Log.w("RocketChat.PushNotificationModule", "Failed to clear pending notification")
+                }
             }

             promise.resolve(notification)
         } catch (e: Exception) {
-            promise.reject("ERROR", e.message)
+            promise.reject("ERROR", e.message, e)
         }
     }
app/lib/notifications/index.ts (1)

98-133: Code duplication: INotification transformation repeated across files.

The notification transformation logic at lines 107-122 is identical to the transformation in push.ts (lines 228-243). This creates maintenance burden and risk of drift. Consider extracting a shared utility function.

The dynamic require() at line 101 is unconventional but acceptable here to avoid bundling issues on iOS where the native module doesn't exist.

♻️ Suggested approach

Create a shared transformation utility:

// In a new file like app/lib/notifications/transformNotification.ts
import { INotification } from '../../definitions';

export const transformRawNotificationData = (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.ts and index.ts.

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

70-71: Avoid creating Gson instances on every call.

GsonBuilder().create() is called on every notification (lines 70-71 and 139-140). Gson instances are thread-safe and reusable—creating them repeatedly adds unnecessary allocation overhead.

♻️ Move Gson to companion object
 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)

         // ... in handleNotificationIntent:
-            val gson = GsonBuilder().create()
             val jsonData = gson.toJson(notificationData)

Also applies to: 139-140


121-131: Fragile heuristic for filtering object references.

The check !stringValue.contains("@") at line 126 will filter out any string containing @, which could unintentionally exclude legitimate values like email addresses if they ever appear in extras. Consider a more specific check for object reference patterns:

♻️ More specific object reference check
-                                if (!stringValue.startsWith("android.") && !stringValue.contains("@")) {
+                                // Filter out object references like "android.os.Bundle@abc123"
+                                val isObjectReference = stringValue.matches(Regex(".*@[0-9a-f]+$"))
+                                if (!stringValue.startsWith("android.") && !isObjectReference) {
                                     notificationData[key] = stringValue
📜 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 9a472ea and 4a72064.

⛔ Files ignored due to path filters (1)
  • app/containers/markdown/__snapshots__/Markdown.test.tsx.snap is excluded by !**/*.snap
📒 Files selected for processing (23)
  • android/app/build.gradle
  • android/app/src/main/java/chat/rocket/reactnative/MainActivity.kt
  • android/app/src/main/java/chat/rocket/reactnative/MainApplication.kt
  • android/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.java
  • android/app/src/main/java/chat/rocket/reactnative/notification/E2ENotificationProcessor.java
  • android/app/src/main/java/chat/rocket/reactnative/notification/Ejson.java
  • android/app/src/main/java/chat/rocket/reactnative/notification/Encryption.java
  • android/app/src/main/java/chat/rocket/reactnative/notification/NativePushNotificationSpec.kt
  • android/app/src/main/java/chat/rocket/reactnative/notification/NotificationIntentHandler.kt
  • android/app/src/main/java/chat/rocket/reactnative/notification/PushNotificationModule.kt
  • android/app/src/main/java/chat/rocket/reactnative/notification/PushNotificationTurboPackage.kt
  • android/app/src/main/java/chat/rocket/reactnative/notification/VideoConfBroadcast.kt
  • app/containers/markdown/Markdown.stories.tsx
  • app/containers/markdown/components/Inline.tsx
  • app/containers/markdown/components/Timestamp.tsx
  • app/lib/native/NativePushNotificationAndroid.ts
  • app/lib/notifications/index.ts
  • app/lib/notifications/push.ts
  • app/sagas/state.js
  • ios/RocketChatRN.xcodeproj/project.pbxproj
  • ios/RocketChatRN/Info.plist
  • ios/ShareRocketChatRN/Info.plist
  • package.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 (8)
app/containers/markdown/Markdown.stories.tsx (1)
app/containers/MessageComposer/components/Toolbar/Markdown.tsx (1)
  • Markdown (9-46)
app/containers/markdown/components/Inline.tsx (1)
app/containers/markdown/Markdown.stories.tsx (1)
  • Timestamp (160-170)
app/containers/markdown/components/Timestamp.tsx (2)
app/theme.tsx (1)
  • useTheme (29-29)
app/containers/Toast.tsx (1)
  • LISTENER (24-24)
app/lib/notifications/index.ts (4)
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/definitions/INotification.ts (1)
  • INotification (1-16)
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)
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)
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)
🪛 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)

⏰ 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: format
🔇 Additional comments (24)
android/app/src/main/java/chat/rocket/reactnative/notification/Encryption.java (3)

191-207: Good defensive column lookup.

The column index checks protect against schema mismatches and provide clear error logging when the expected e2e_key column is missing. The graceful fallback for a missing encrypted column is also appropriate.


248-272: Robust handling of both direct and double-encoded private keys.

The fallback parsing correctly handles cases where the private key might be JSON-encoded as a string (double-encoded). The validation of required fields (n, e, d) at lines 269-272 prevents downstream NPEs when building the JWK map.


287-357: Improved null safety and error handling in decryption flow.

The defensive null/empty checks and try-catch blocks around base64 decoding, RSA decryption, and JSON parsing significantly improve robustness. The algorithm derivation based on the alg field correctly distinguishes between v1 (rc.v1.aes-sha2) and v2 (rc.v2.aes-sha2) formats.

Note: Line 354 stores algorithm as an instance field, which relates to the thread safety concern flagged earlier.

app/containers/markdown/components/Inline.tsx (1)

13-13: LGTM!

The import is correctly placed and follows the existing pattern of component imports in this file.

app/containers/markdown/Markdown.stories.tsx (1)

159-170: LGTM!

The Timestamp story effectively demonstrates all supported format options and follows the established pattern of other stories in this file. The examples provide clear visual documentation of the timestamp rendering feature.

app/containers/markdown/components/Timestamp.tsx (1)

10-15: LGTM!

The interface is well-typed with specific format options, and the component setup properly leverages the theme for consistent styling.

android/app/build.gradle (1)

93-93: Version alignment looks good.

The version downgrade to 4.68.2 aligns with the PR objective to merge the 4.68.2 release branch. This change is consistent across all platforms (Android, iOS).

ios/ShareRocketChatRN/Info.plist (1)

29-29: iOS share extension version properly aligned.

ios/RocketChatRN/Info.plist (1)

31-31: iOS main app version properly aligned.

ios/RocketChatRN.xcodeproj/project.pbxproj (1)

3009-3009: NotificationService extension versions properly aligned.

Also applies to: 3062-3062

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

51-52: Video conference payload extension is correctly implemented.

The host and callId fields use proper null-safe extraction with empty string defaults in VideoConfBroadcast.kt, and integration verification confirms:

  • VideoConfNotification populates both fields from the ejson object before passing to the intent
  • NotificationIntentHandler extracts them consistently
  • The JS layer receives and handles these fields: IncomingCallNotification component uses callId to fetch video conference info, while host is available in the notification payload for routing purposes
package.json (1)

8-9: LGTM! Good practice to enforce UTC for tests.

Setting TZ=UTC ensures test consistency across different environments and prevents timezone-related test failures. This is a solid improvement to test reliability.

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

22-22: LGTM! TurboModule integration follows React Native best practices.

The addition of PushNotificationTurboPackage provides a clean, modern bridge for push notification functionality. This aligns with React Native's recommended TurboModule architecture and replaces the legacy ReactContext listener pattern.

Also applies to: 45-45

android/app/src/main/java/chat/rocket/reactnative/MainActivity.kt (3)

33-34: LGTM! Clean delegation with proper null safety.

The centralized notification intent handling via NotificationIntentHandler is a good architectural improvement. The null-safe call ensures robustness.


39-39: LGTM! Essential Android lifecycle handling.

Adding setIntent(intent) ensures the activity's intent reference stays current when receiving new intents. This prevents stale intent data from being used by subsequent getIntent() calls, which is critical for proper notification handling.


37-42: LGTM! Proper handling of new intents.

The implementation correctly updates the intent reference and delegates to the centralized handler. Since onNewIntent always receives a non-null Intent, no additional null checking is needed.

app/lib/native/NativePushNotificationAndroid.ts (1)

9-9: The module name is correctly configured. The TypeScript code requests 'PushNotificationModule', which matches the NAME constant defined in NativePushNotificationSpec.kt (line 13) and inherited by PushNotificationModule.kt through its getName() method.

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

63-76: LGTM with minor observation.

The clearPendingNotification method intentionally ignores errors, which is acceptable for a cleanup utility. Consider adding a debug log for troubleshooting if issues arise in production.

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

12-37: LGTM!

Standard TurboReactPackage implementation with correct module info configuration. The flags are appropriately set for a TurboModule.

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

25-34: Good defensive error handling for video conf notifications.

The try/catch around video conf EJSON parsing with a warning log is appropriate. Note that on parse failure, execution continues past this block rather than returning early—this seems intentional to allow fallback processing.


48-51: Good guard rails for missing required fields.

Validating rid, type, and host before proceeding and dispatching appInit() as fallback is sensible defensive coding. This prevents downstream errors from malformed notification data.

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

9-23: LGTM!

Clean TurboModule specification following React Native conventions. The abstract class properly defines the contract for getPendingNotification and clearPendingNotification.

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

172-200: Improved E2E notification handling with proper null checks.

The refactored handleE2ENotification method correctly handles the case when Encryption.shared is null (line 173) by showing a placeholder "Encrypted message". The synchronized blocks properly protect mBundle updates. The fallback on decryption failure (lines 191-199) ensures users still see notification arrivals even when decryption fails.


138-160: Good thread-safety pattern with local bundle capture.

Capturing mBundle to a local final variable at line 139 ensures consistent reads within the method, while the volatile modifier on mBundle (line 62) ensures visibility across threads. The synchronized blocks for updates maintain proper thread safety.

const { colors } = useTheme();

const formatDate = React.useMemo(() => {
const timestamp = parseInt(value.timestamp) * 1000;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Add radix parameter to parseInt and eliminate duplication.

Both calls to parseInt(value.timestamp) omit the radix parameter. Always specify parseInt(value.timestamp, 10) to ensure base-10 parsing and avoid potential edge-case bugs.

Additionally, the timestamp parsing and millisecond conversion logic is duplicated between line 18 and line 52. Consider extracting this into a memoized value to improve maintainability.

🔧 Proposed fix
 const Timestamp = ({ value }: ITimestampProps): React.ReactElement => {
 	const { colors } = useTheme();
 
+	const timestampMs = React.useMemo(() => parseInt(value.timestamp, 10) * 1000, [value.timestamp]);
+
 	const formatDate = React.useMemo(() => {
-		const timestamp = parseInt(value.timestamp) * 1000;
-
 		if (value.format === 't') {
-			return dayjs(timestamp).format('hh:mm A');
+			return dayjs(timestampMs).format('hh:mm A');
 		}
 
 		if (value.format === 'T') {
-			return dayjs(timestamp).format('hh:mm:ss A');
+			return dayjs(timestampMs).format('hh:mm:ss A');
 		}
 
 		if (value.format === 'd') {
-			return dayjs(timestamp).format('MM/DD/YYYY');
+			return dayjs(timestampMs).format('MM/DD/YYYY');
 		}
 
 		if (value.format === 'D') {
-			return dayjs(timestamp).format('dddd, MMM DD, YYYY');
+			return dayjs(timestampMs).format('dddd, MMM DD, YYYY');
 		}
 
 		if (value.format === 'f') {
-			return dayjs(timestamp).format('dddd, MMM DD, YYYY hh:mm A');
+			return dayjs(timestampMs).format('dddd, MMM DD, YYYY hh:mm A');
 		}
 
 		if (value.format === 'F') {
-			return dayjs(timestamp).format('dddd, MMM DD, YYYY hh:mm:ss A');
+			return dayjs(timestampMs).format('dddd, MMM DD, YYYY hh:mm:ss A');
 		}
 
 		if (value.format === 'R') {
-			return dayjs(timestamp).fromNow();
+			return dayjs(timestampMs).fromNow();
 		}
 
 		return 'Invalid Date';
-	}, [value]);
+	}, [timestampMs, value.format]);
 
 	const handlePress = React.useCallback(() => {
-		const message = dayjs(parseInt(value.timestamp) * 1000).format('dddd, MMM DD, YYYY hh:mm A');
+		const message = dayjs(timestampMs).format('dddd, MMM DD, YYYY hh:mm A');
 		EventEmitter.emit(LISTENER, { message });
-	}, [value.timestamp]);
+	}, [timestampMs]);

Also applies to: 52-52

🤖 Prompt for AI Agents
In @app/containers/markdown/components/Timestamp.tsx at line 18, The code uses
parseInt(value.timestamp) without a radix and duplicates the
seconds-to-milliseconds conversion; update both occurrences to
parseInt(value.timestamp, 10) and extract the logic into a single memoized value
(e.g., const timestampMs = useMemo(() => parseInt(value.timestamp, 10) * 1000,
[value.timestamp])) inside the Timestamp component so both references use
timestampMs, eliminating duplication and ensuring base‑10 parsing.

@diegolmello diegolmello changed the base branch from develop to master January 8, 2026 19:57
@diegolmello diegolmello temporarily deployed to approve_e2e_testing January 8, 2026 19:58 — with GitHub Actions Inactive
@diegolmello diegolmello temporarily deployed to experimental_ios_build January 8, 2026 20:01 — with GitHub Actions Inactive
@diegolmello diegolmello deployed to experimental_android_build January 8, 2026 20:01 — with GitHub Actions Active
@diegolmello diegolmello deployed to official_android_build January 8, 2026 20:01 — with GitHub Actions Active
@diegolmello diegolmello temporarily deployed to official_ios_build January 8, 2026 20:01 — with GitHub Actions Inactive
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

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/MainApplication.kt (1)

27-35: Update the notification architecture documentation.

The comment still references CustomPushNotification, which was replaced by PushNotificationTurboPackage in this refactoring. The documentation should be updated to reflect the new TurboModule-based notification architecture.

📝 Suggested documentation update
 /**
  * Main Application class.
  * 
  * NOTIFICATION ARCHITECTURE:
  * - JS layer uses expo-notifications for token registration and event handling
- * - Native layer uses RCFirebaseMessagingService + CustomPushNotification for:
+ * - Native layer uses RCFirebaseMessagingService + PushNotificationModule (TurboModule) for:
  *   - FCM message handling
  *   - Notification display with MessagingStyle
  *   - E2E encrypted message decryption
  *   - Direct reply functionality
  *   - Message-id-only notification loading
  */
android/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.java (1)

113-136: Address thread-safety race condition in Encryption.shared singleton.

The Encryption.shared singleton has unprotected access to instance fields keyId (line 128) and algorithm (line 129). These fields are written in decryptRoomKey() (lines 296, 354) without synchronization and read in encryptMessageContent() (lines 457, 470). Since decryptMessage() and encryptMessageContent() can be called concurrently from multiple threads (including the background thread callback in CustomPushNotification at line 189), this creates a data race condition.

Make these fields thread-safe by either:

  • Making keyId and algorithm local variables passed as parameters instead of instance state, or
  • Protecting reads/writes with synchronization, or
  • Using thread-local storage if per-thread state is required.
🤖 Fix all issues with AI agents
In @android/app/src/main/java/chat/rocket/reactnative/MainActivity.kt:
- Around line 33-34: The handleVideoConfIntent path should mirror the error
handling used for notification intents: wrap the call to
VideoConfModule.storePendingAction (invoked from MainActivity.onCreate or the
helper handling video-conf intents) in a try-catch that catches Exception and
logs it via the same logger used for notifications, ensuring any thrown error
does not propagate and block onCreate; locate the call to
VideoConfModule.storePendingAction and add the protective try-catch around it
with an informative error log message.

In @app/containers/markdown/components/Timestamp.tsx:
- Around line 17-49: The useMemo for formatDate currently depends on the whole
value object which breaks memoization; change the dependency array to use the
primitive fields [value.timestamp, value.format] so formatDate only recomputes
when those change, and inside the useMemo validate the timestamp (use
parseInt(..., 10), check for NaN and non-finite values) before multiplying by
1000 and creating the dayjs instance, returning 'Invalid Date' or a safe
fallback when the timestamp is invalid; update references to value.timestamp and
value.format accordingly.

In @package.json:
- Around line 8-9: The test scripts in package.json use the Unix-only prefix
"TZ=UTC" which breaks on Windows; install cross-env as a dev dependency (e.g.,
yarn add --dev cross-env) and update the "test" and "test-update" scripts to
prefix with "cross-env TZ=UTC" (referencing the "test" and "test-update" script
entries) so the TZ variable is set cross-platform, or if your environments are
strictly Unix-only, document that requirement in the README instead of changing
the scripts.
🧹 Nitpick comments (6)
android/app/src/main/java/chat/rocket/reactnative/notification/Encryption.java (1)

488-490: Inconsistent logging tag.

This uses a hardcoded "[ROCKETCHAT][E2E]" string while the rest of the file uses the TAG constant ("RocketChat.E2E"). Consider using TAG for consistency and easier log filtering.

Suggested fix
-            Log.e("[ROCKETCHAT][E2E]", Log.getStackTraceString(e));
+            Log.e(TAG, "Encryption failed", e);
app/lib/notifications/push.ts (2)

218-274: Consider extracting the notification transformation to a shared helper.

The notification data transformation logic (lines 228-243) is duplicated in app/lib/notifications/index.ts (lines 107-122 in checkPendingNotification). Both create an INotification object from parsed JSON with identical field mappings.

Suggested approach

Create a shared helper function:

// In push.ts or a shared utils file
export const transformNativeNotificationData = (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 push.ts and index.ts.


253-273: The second .catch() is unreachable.

The first .catch() (lines 253-256) handles errors and returns null. The second .then() (lines 257-269) only checks nativeNotification and accesses synchronous getLastNotificationResponse(). The second .catch() (lines 270-273) is effectively dead code since no errors can propagate from the second .then().

Simplified chain
             .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;
             });
android/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.java (1)

172-200: Minor duplication in fallback handling.

Both the Encryption.shared == null path (line 175) and the decryption failure path (line 194) set the same fallback message. Consider consolidating:

♻️ Suggested consolidation
 private void handleE2ENotification(Bundle bundle, Ejson ejson, String notId) {
+    String decrypted = null;
+    
-    if (Encryption.shared == 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;
+    if (Encryption.shared != null) {
+        decrypted = Encryption.shared.decryptMessage(ejson, mContext);
     }
     
-    String decrypted = Encryption.shared.decryptMessage(ejson, mContext);
-    
     if (decrypted != null) {
         bundle.putString("message", decrypted);
-        synchronized(this) {
-            mBundle = bundle;
-        }
-        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
+        Log.w(TAG, Encryption.shared == null 
+            ? "Encryption singleton is null, cannot decrypt E2E notification"
+            : "E2E decryption failed for notification, showing fallback notification");
         bundle.putString("message", "Encrypted message");
-        synchronized(this) {
-            mBundle = bundle;
-        }
-        showNotification(bundle, ejson, notId);
     }
+    
+    synchronized(this) {
+        mBundle = bundle;
+    }
+    showNotification(bundle, ejson, notId);
 }
app/containers/markdown/components/Timestamp.tsx (1)

51-54: Extract duplicate timestamp parsing logic.

The timestamp parsing logic is duplicated from the formatDate memoization. Consider extracting this into a shared constant or utility function within the component.

♻️ Proposed refactor
 const Timestamp = ({ value }: ITimestampProps): React.ReactElement => {
   const { colors } = useTheme();
 
+  const timestampMs = 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 (timestampMs === null) {
+      return 'Invalid Date';
+    }
 
     if (value.format === 't') {
-      return dayjs(timestamp).format('hh:mm A');
+      return dayjs(timestampMs).format('hh:mm A');
     }
 
     // ... update all other format checks to use timestampMs ...
 
     return 'Invalid Date';
-  }, [value.timestamp, value.format]);
+  }, [timestampMs, value.format]);
 
   const handlePress = React.useCallback(() => {
-    const message = dayjs(parseInt(value.timestamp) * 1000).format('dddd, MMM DD, YYYY hh:mm A');
+    if (timestampMs === null) return;
+    const message = dayjs(timestampMs).format('dddd, MMM DD, YYYY hh:mm A');
     EventEmitter.emit(LISTENER, { message });
-  }, [value.timestamp]);
+  }, [timestampMs]);
app/containers/markdown/components/Inline.tsx (1)

74-75: Missing key prop is consistent with existing pattern.

Static analysis correctly identifies that the TIMESTAMP case (like all other cases in this switch) is missing a key prop for the mapped elements. However, this is consistent with the existing code pattern in this file—all cases from line 44-73 share the same issue.

Consider addressing this systematically by adding keys to all cases in a follow-up refactor, using the index parameter that's already available in the map function.

💡 Example fix for all cases
 {value.map((block, index) => {
   // ... existing logic ...
   
   switch (block.type) {
     case 'IMAGE':
-      return <Image value={block.value} />;
+      return <Image key={index} value={block.value} />;
     case 'PLAIN_TEXT':
-      return <Plain value={block.value} />;
+      return <Plain key={index} value={block.value} />;
     // ... apply to all other cases including TIMESTAMP ...
     case 'TIMESTAMP':
-      return <Timestamp value={block.value} />;
+      return <Timestamp key={index} value={block.value} />;
     default:
       return null;
   }
 })}

Note: Since blocks are parsed sequentially and don't reorder, using index as the key is acceptable here.

📜 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 4a72064 and 86fab84.

⛔ Files ignored due to path filters (1)
  • app/containers/markdown/__snapshots__/Markdown.test.tsx.snap is excluded by !**/*.snap
📒 Files selected for processing (19)
  • android/app/src/main/java/chat/rocket/reactnative/MainActivity.kt
  • android/app/src/main/java/chat/rocket/reactnative/MainApplication.kt
  • android/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.java
  • android/app/src/main/java/chat/rocket/reactnative/notification/E2ENotificationProcessor.java
  • android/app/src/main/java/chat/rocket/reactnative/notification/Ejson.java
  • android/app/src/main/java/chat/rocket/reactnative/notification/Encryption.java
  • android/app/src/main/java/chat/rocket/reactnative/notification/NativePushNotificationSpec.kt
  • android/app/src/main/java/chat/rocket/reactnative/notification/NotificationIntentHandler.kt
  • android/app/src/main/java/chat/rocket/reactnative/notification/PushNotificationModule.kt
  • android/app/src/main/java/chat/rocket/reactnative/notification/PushNotificationTurboPackage.kt
  • android/app/src/main/java/chat/rocket/reactnative/notification/VideoConfBroadcast.kt
  • app/containers/markdown/Markdown.stories.tsx
  • app/containers/markdown/components/Inline.tsx
  • app/containers/markdown/components/Timestamp.tsx
  • app/lib/native/NativePushNotificationAndroid.ts
  • app/lib/notifications/index.ts
  • app/lib/notifications/push.ts
  • app/sagas/state.js
  • package.json
💤 Files with no reviewable changes (2)
  • android/app/src/main/java/chat/rocket/reactnative/notification/Ejson.java
  • android/app/src/main/java/chat/rocket/reactnative/notification/E2ENotificationProcessor.java
🚧 Files skipped from review as they are similar to previous changes (5)
  • app/lib/native/NativePushNotificationAndroid.ts
  • android/app/src/main/java/chat/rocket/reactnative/notification/PushNotificationModule.kt
  • android/app/src/main/java/chat/rocket/reactnative/notification/NotificationIntentHandler.kt
  • android/app/src/main/java/chat/rocket/reactnative/notification/VideoConfBroadcast.kt
  • android/app/src/main/java/chat/rocket/reactnative/notification/PushNotificationTurboPackage.kt
🧰 Additional context used
🧬 Code graph analysis (5)
app/containers/markdown/Markdown.stories.tsx (1)
app/containers/MessageComposer/components/Toolbar/Markdown.tsx (1)
  • Markdown (9-46)
app/sagas/state.js (1)
app/lib/notifications/index.ts (1)
  • checkPendingNotification (98-133)
app/containers/markdown/components/Inline.tsx (1)
app/containers/markdown/Markdown.stories.tsx (1)
  • Timestamp (160-170)
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)
🪛 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)

🔇 Additional comments (21)
android/app/src/main/java/chat/rocket/reactnative/MainApplication.kt (1)

22-22: LGTM! Clean TurboModule integration.

The import and package registration for PushNotificationTurboPackage are correctly implemented, following the same pattern as other TurboModule packages in the application.

Also applies to: 45-45

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

9-23: LGTM!

The TurboModule spec follows the standard React Native pattern correctly. The abstract class properly extends ReactContextBaseJavaModule, implements TurboModule, and declares the expected abstract methods with @ReactMethod annotations.

android/app/src/main/java/chat/rocket/reactnative/notification/Encryption.java (3)

191-208: Good defensive coding for database column access.

The column index checks before accessing values prevent potential crashes when the schema doesn't match expectations. This is a solid improvement over direct column access.


248-272: Robust private key parsing with format flexibility.

The double-decode fallback pattern handles edge cases where the private key might be JSON-encoded twice. The required field validation (n, e, d) prevents downstream failures.


287-357: Comprehensive guards and error handling in room key decryption.

The null checks, try-catch blocks around cryptographic operations, and explicit field validation make this much more resilient. The algorithm determination based on the alg field correctly distinguishes v1 vs v2 formats.

app/sagas/state.js (1)

32-35: LGTM!

The fire-and-forget pattern with .catch() is appropriate here since pending notification handling is supplementary and shouldn't block the main foreground flow. The error logging ensures failures are observable without disrupting user presence updates.

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

25-34: Good defensive parsing for video conference notifications.

Wrapping EJSON.parse in try-catch prevents crashes from malformed payloads. The warning log aids debugging without disrupting the user experience.


48-51: LGTM!

Early validation for essential fields (rid, type, host) with fallback to appInit() prevents errors from incomplete notification payloads while still bringing the app to a usable state.


59-64: LGTM!

The room name resolution correctly prioritizes sender-specific fields based on subscription type while falling back to name when unavailable.


98-132: Well-structured Android pending notification handler.

The dynamic require() pattern avoids iOS bundling issues, and the nested try-catch properly distinguishes module loading errors from JSON parsing errors.

Note: The notification transformation logic (lines 107-122) is duplicated from push.ts (lines 228-243). See the refactoring suggestion in the push.ts review.

android/app/src/main/java/chat/rocket/reactnative/MainActivity.kt (2)

12-12: LGTM: Import added for centralized notification handling.

The import of NotificationIntentHandler is necessary for the refactored notification handling approach.


37-43: Good addition of setIntent(intent) call; verify error handling.

The addition of setIntent(intent) at line 39 is essential and correct. Without it, subsequent calls to getIntent() would still return the original intent rather than the new one.

However, similar to the onCreate concern, verify that NotificationIntentHandler.handleIntent has proper error handling to prevent crashes when processing new intents while the activity is running.

The verification script in the previous comment will also help confirm the error handling for this code path.

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

62-62: LGTM on the volatile declaration.

The volatile modifier ensures visibility of mBundle updates across threads, which is appropriate given that notifications may be processed on background threads. Combined with the synchronized write blocks and local copy read pattern used throughout, this provides proper thread-safety.


138-160: LGTM on local bundle capture pattern.

Capturing mBundle into a local final variable at the start of processNotification ensures a consistent snapshot is used throughout the method. This correctly prevents issues where mBundle could be modified by another thread mid-execution.


226-228: LGTM on synchronized mBundle update.

The synchronized block correctly protects the mBundle assignment after enriching the bundle with metadata. This maintains consistency with the thread-safety pattern used throughout the class.


297-355: LGTM on buildNotification refactoring.

The method correctly captures mBundle into a local final variable and uses it consistently throughout. Passing this local reference to helper methods (notificationIcons, notificationStyle, notificationReply) and to intent.putExtras() ensures the notification is built with a coherent snapshot of the bundle state.

app/containers/markdown/components/Timestamp.tsx (2)

1-12: LGTM!

The imports and interface definition are well-structured. The format union type provides good type safety for the supported timestamp formats.


56-62: LGTM!

The component render is well-structured with appropriate theming and interactive functionality.

app/containers/markdown/components/Inline.tsx (1)

13-13: LGTM!

The import is correctly added to support the new TIMESTAMP rendering capability.

app/containers/markdown/Markdown.stories.tsx (1)

159-170: LGTM!

The Timestamp story comprehensively demonstrates all seven supported format types. The fixed epoch (January 1, 2025, 12:00:00 UTC) provides a consistent reference point for visual verification and regression testing.

package.json (1)

3-3: LGTM! Version bump aligns with the release merge.

The version increment from 4.68.1 to 4.68.2 is consistent with the PR objective and cross-platform version updates mentioned in the AI summary.

Comment on lines +33 to +34
// Handle notification intents
intent?.let { NotificationIntentHandler.handleIntent(this, it) }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Find NotificationIntentHandler implementation
fd -e kt NotificationIntentHandler --exec cat {}

Repository: RocketChat/Rocket.Chat.ReactNative

Length of output: 6524


Add error handling to handleVideoConfIntent method.

The handleNotificationIntent has good error handling, but handleVideoConfIntent lacks protection around the VideoConfModule.storePendingAction call. If this throws an exception, it will bubble up and prevent onCreate from completing. Wrap the storage operation in a try-catch block similar to the handleNotificationIntent implementation to ensure robustness during activity initialization.

🤖 Prompt for AI Agents
In @android/app/src/main/java/chat/rocket/reactnative/MainActivity.kt around
lines 33 - 34, The handleVideoConfIntent path should mirror the error handling
used for notification intents: wrap the call to
VideoConfModule.storePendingAction (invoked from MainActivity.onCreate or the
helper handling video-conf intents) in a try-catch that catches Exception and
logs it via the same logger used for notifications, ensuring any thrown error
does not propagate and block onCreate; locate the call to
VideoConfModule.storePendingAction and add the protective try-catch around it
with an informative error log message.

Comment on lines +17 to +49
const formatDate = React.useMemo(() => {
const timestamp = parseInt(value.timestamp) * 1000;

if (value.format === 't') {
return dayjs(timestamp).format('hh:mm A');
}

if (value.format === 'T') {
return dayjs(timestamp).format('hh:mm:ss A');
}

if (value.format === 'd') {
return dayjs(timestamp).format('MM/DD/YYYY');
}

if (value.format === 'D') {
return dayjs(timestamp).format('dddd, MMM DD, YYYY');
}

if (value.format === 'f') {
return dayjs(timestamp).format('dddd, MMM DD, YYYY hh:mm A');
}

if (value.format === 'F') {
return dayjs(timestamp).format('dddd, MMM DD, YYYY hh:mm:ss A');
}

if (value.format === 'R') {
return dayjs(timestamp).fromNow();
}

return 'Invalid Date';
}, [value]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Fix the useMemo dependency array to use primitive values.

The memoization depends on the value object reference, which will change on every parent re-render even if timestamp and format remain the same. This defeats the purpose of memoization.

Additionally, consider adding validation for the timestamp value to handle edge cases gracefully.

🔧 Proposed fix
 const formatDate = React.useMemo(() => {
-  const timestamp = parseInt(value.timestamp) * 1000;
+  const parsedTimestamp = parseInt(value.timestamp, 10);
+  if (isNaN(parsedTimestamp)) {
+    return 'Invalid Date';
+  }
+  const timestamp = parsedTimestamp * 1000;

   if (value.format === 't') {
     return dayjs(timestamp).format('hh:mm A');
   }

   // ... rest of format checks ...

   return 'Invalid Date';
- }, [value]);
+ }, [value.timestamp, value.format]);
🤖 Prompt for AI Agents
In @app/containers/markdown/components/Timestamp.tsx around lines 17 - 49, The
useMemo for formatDate currently depends on the whole value object which breaks
memoization; change the dependency array to use the primitive fields
[value.timestamp, value.format] so formatDate only recomputes when those change,
and inside the useMemo validate the timestamp (use parseInt(..., 10), check for
NaN and non-finite values) before multiplying by 1000 and creating the dayjs
instance, returning 'Invalid Date' or a safe fallback when the timestamp is
invalid; update references to value.timestamp and value.format accordingly.

@diegolmello diegolmello deployed to upload_official_android January 8, 2026 20:33 — with GitHub Actions Active
@diegolmello diegolmello deployed to upload_experimental_android January 8, 2026 20:34 — with GitHub Actions Active
@diegolmello diegolmello temporarily deployed to upload_official_ios January 8, 2026 20:34 — with GitHub Actions Inactive
@github-actions
Copy link

github-actions bot commented Jan 8, 2026

@github-actions
Copy link

github-actions bot commented Jan 8, 2026

Android Build Available

Rocket.Chat Experimental 4.68.2.108011

Internal App Sharing: https://play.google.com/apps/test/RQVpXLytHNc/ahAO29uNTCBMCgrbR3Fj0G1HuqaPhWMfI0e0Vl1-MciMlb4JKUNqZfRBD1xP7vEK7RuWjMBS703Voi1SzPLQmKmgJF

@github-actions
Copy link

github-actions bot commented Jan 8, 2026

Android Build Available

Rocket.Chat 4.68.2.108012

@github-actions
Copy link

github-actions bot commented Jan 8, 2026

Android Build Available

Rocket.Chat Experimental 4.68.2.108011

@github-actions
Copy link

github-actions bot commented Jan 8, 2026

iOS Build Available

Rocket.Chat 4.68.2.108013

@github-actions
Copy link

github-actions bot commented Jan 8, 2026

iOS Build Available

Rocket.Chat Experimental 4.68.2.108014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants