fix(android): Push notification crashing on some edge cases#6667
fix(android): Push notification crashing on some edge cases#6667diegolmello merged 2 commits intodevelopfrom
Conversation
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
|
Caution Review failedThe pull request is closed. WalkthroughCentralizes JSON parsing in CustomPushNotification by adding a shared Gson and a safeFromJson helper; replaces ad‑hoc Gson.fromJson calls with null-safe parsing and guards, adds sender/avatar fallbacks and null checks across notification build, decryption, style, icon selection, and dismissal/deduplication flows. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant OS as Android OS
participant RN as CustomPushNotification
participant JSON as safeFromJson()
participant UI as NotificationBuilder
OS->>RN: onReceived(push payload)
RN->>JSON: safeFromJson(payload.ejson)
alt parsed
RN->>RN: extract sender, avatar, type, message
else null/invalid
RN->>RN: use defaults (sender="Unknown", skip avatar)
end
RN->>RN: decrypt if loadedEjson present
RN->>UI: build notification (icons, MessagingStyle) with guarded fields
UI-->>OS: post notification
OS->>RN: dismissal/update
RN->>JSON: safeFromJson(notif.ejson)
RN->>RN: dedupe/dismiss with null-safe comparisons
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
Comment |
diegolmello
changed the title
diegolmello
had a problem deploying
to
official_ios_build
GitHub Actions
Error
diegolmello
temporarily deployed
to
experimental_android_build
GitHub Actions
Inactive
diegolmello
had a problem deploying
to
official_android_build
GitHub Actions
Error
diegolmello
had a problem deploying
to
experimental_ios_build
GitHub Actions
Error
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
android/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.java (3)
243-249: Fix substring bounds; avoid crash on short messages like "A:"substring(start, length) can throw StringIndexOutOfBounds when pos + 2 > length, and message may be null.
Apply:
- if (ejson != null && ejson.type != null && !ejson.type.equals("d")) { - int pos = message.indexOf(":"); - int start = pos == -1 ? 0 : pos + 2; - return message.substring(start, message.length()); - } - return message; + if (message == null) { + return null; // or "" if preferred + } + if (ejson != null && ejson.type != null && !"d".equals(ejson.type)) { + final int pos = message.indexOf(':'); + final int start = pos == -1 ? 0 : Math.min(message.length(), pos + 2); + return message.substring(start); + } + return message;
165-171: Null-safe server URL comparison to prevent NPEserverURL() may be null; calling equals() on it risks NPE.
Apply:
- if (ejson != null && notEjson != null && ejson.serverURL().equals(notEjson.serverURL())) { + if (ejson != null && notEjson != null + && android.text.TextUtils.equals(ejson.serverURL(), notEjson.serverURL())) {If you prefer, add
import android.text.TextUtils;and drop the fully-qualified name.
58-58: Harden notificationMessages against concurrent access (possible CME/crashes)onReceived and builder paths can interleave; iterating while mutating risks ConcurrentModificationException.
Apply:
- private static Map<String, List<Bundle>> notificationMessages = new HashMap<String, List<Bundle>>(); + private static Map<String, List<Bundle>> notificationMessages = new java.util.concurrent.ConcurrentHashMap<>();- if (notificationMessages.get(notId) == null) { - notificationMessages.put(notId, new ArrayList<Bundle>()); - } + notificationMessages.computeIfAbsent(notId, k -> new java.util.concurrent.CopyOnWriteArrayList<>());CopyOnWriteArrayList makes the iteration below safe even when other threads add items.
Also applies to: 87-89, 159-173
🧹 Nitpick comments (5)
android/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.java (5)
157-157: Remove unused local Gson instancesThese locals are dead code after introducing safeFromJson.
Apply:
- Gson gson = new Gson();- Gson gson = new Gson();Also applies to: 276-276
96-101: Wrap decryption in try/catch to prevent rare crashesdecryptMessage could throw; today it’s unchecked.
Apply:
- if (loadedEjson != null && loadedEjson.msg != null) { - // Override message with the decrypted content - String decrypted = Encryption.shared.decryptMessage(loadedEjson, reactApplicationContext); - if (decrypted != null) { - bundle.putString("message", decrypted); - } - } + if (loadedEjson != null && loadedEjson.msg != null) { + try { + final String decrypted = Encryption.shared.decryptMessage(loadedEjson, reactApplicationContext); + if (decrypted != null) { + bundle.putString("message", decrypted); + } + } catch (Exception ex) { + android.util.Log.e("CustomPushNotification", "Message decryption failed", ex); + } + }
108-109: Simplify type checkIf notificationType is declared as String, instanceof is redundant; prefer constant-first equals.
Apply:
- if (loadedEjson != null && loadedEjson.notificationType instanceof String && loadedEjson.notificationType.equals("videoconf")) { + if (loadedEjson != null && "videoconf".equals(loadedEjson.notificationType)) {
103-103: Micro: avoid allocationSystem.currentTimeMillis() is cheaper than new Date().getTime().
Apply:
- bundle.putLong("time", new Date().getTime()); + bundle.putLong("time", System.currentTimeMillis());
330-333: Gate inline reply on valid Ejson, not just "{}" stringInvalid JSON still enables reply today; prefer parsing result.
Apply:
- String ejson = bundle.getString("ejson", "{}"); - if (Build.VERSION.SDK_INT < Build.VERSION_CODES.N || notId.equals("1") || ejson.equals("{}")) { + Ejson e = safeFromJson(bundle.getString("ejson", "{}"), Ejson.class); + if (Build.VERSION.SDK_INT < Build.VERSION_CODES.N || "1".equals(notId) || e == null) { return; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
android/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.java(11 hunks)
⏰ 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 (1)
android/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.java (1)
46-47: Good: single, shared Gson instance (thread-safe).Centralizing and reusing a single Gson is correct and avoids per-call allocations.
| /** | ||
| * Safely parse JSON string to object with error handling. | ||
| * | ||
| * @param json JSON string to parse | ||
| * @param classOfT Target class type | ||
| * @param <T> Type parameter | ||
| * @return Parsed object, or null if parsing fails | ||
| */ | ||
| private static <T> T safeFromJson(String json, Class<T> classOfT) { | ||
| if (json == null || json.trim().isEmpty() || json.equals("{}")) { | ||
| return null; // no need to create a new instance | ||
| } | ||
|
|
||
| try { | ||
| return gson.fromJson(json, classOfT); | ||
| } catch (Exception e) { | ||
| android.util.Log.e("CustomPushNotification", | ||
| "Failed to parse JSON into " + classOfT.getSimpleName() + ": " + json, e); | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
🧩 Analysis chain
Nice helper; add unit tests and scan for remaining direct fromJson/new Gson
The helper is good; ensure no other call sites bypass it.
Run:
🏁 Script executed:
#!/bin/bash
# Find direct Gson usages that might bypass safeFromJson
rg -nP --type=java '\bnew\s+Gson\s*\(' -S
rg -nP --type=java '\bfromJson\s*\(' -S
# Find remaining raw "message-id-only" checks that may need null-safety parity
rg -nP --type=java '"message-id-only"\)|\.equals\("message-id-only"\)' -n
# Smoke-check for other extractMessage implementations
rg -nP --type=java '\bextractMessage\s*\(' -n -C2Length of output: 3794
Centralize JSON parsing: replace direct Gson/fromJson with safeFromJson; add unit tests
Helper is fine; several call sites still instantiate or call Gson.fromJson — convert them to safeFromJson (or justify local Gson) and add unit tests for null/empty JSON and the "message-id-only" flows.
- android/app/src/main/java/chat/rocket/reactnative/notification/LoadNotification.java — new Gson (line 80); gson.fromJson (line 81).
- android/app/src/main/java/chat/rocket/reactnative/notification/Encryption.java — new Gson (line 68); gson.fromJson at lines 138, 164, 198, 203.
- android/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.java — private static Gson (line 47); local new Gson at 157 and 276; safeFromJson/gson.fromJson inside helper at 394.
- android/app/src/main/java/chat/rocket/reactnative/notification/ReplyBroadcast.java — new Gson (line 51); gson.fromJson (line 52).
🤖 Prompt for AI Agents
In
android/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.java
around lines 380-400, centralize JSON parsing by replacing all direct new
Gson()/gson.fromJson usages across the listed files with the safeFromJson
helper: update LoadNotification (lines ~80-81), Encryption (lines ~68, 138, 164,
198, 203), CustomPushNotification (remove local new Gson at ~157 and ~276 and
use the private static gson + safeFromJson), and ReplyBroadcast (lines ~51-52)
to call safeFromJson(jsonString, TargetClass.class) instead of constructing
local Gson or calling fromJson directly; remove redundant local Gson fields,
ensure callers handle null returns from safeFromJson (fall back to
message-id-only logic where required), and add unit tests covering
null/empty/"{}" JSON and message-id-only flows to verify behavior.
android/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.java
Outdated
Show resolved
Hide resolved
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
android/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.java (2)
151-174: Fix potential NPE in serverURL comparison; remove unused Gson.
new Gson()is unused here.ejson.serverURL().equals(notEjson.serverURL())can NPE if either serverURL is null. UseObjects.equals.- Gson gson = new Gson(); @@ - if (ejson != null && notEjson != null && ejson.serverURL().equals(notEjson.serverURL())) { + if (notEjson != null && java.util.Objects.equals(ejson.serverURL(), notEjson.serverURL())) {
243-250: extractMessage can crash on null message; add null-safe handling.
message.indexOf(":")will throw ifmessageis null. Also flip"d".equals(...)to avoid NPE.- if (ejson != null && ejson.type != null && !ejson.type.equals("d")) { - int pos = message.indexOf(":"); - int start = pos == -1 ? 0 : pos + 2; - return message.substring(start, message.length()); - } - return message; + final String m = message == null ? "" : message; + if (ejson != null && ejson.type != null && !"d".equals(ejson.type)) { + final int pos = m.indexOf(':'); + final int start = pos == -1 ? 0 : Math.min(m.length(), pos + 2); + return m.substring(start); + } + return m;
♻️ Duplicate comments (2)
android/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.java (2)
389-403: Make safeFromJson robust to whitespace and reusable across classes.
- Treat
" {} "as empty too.- Consider moving this to a small shared util (e.g.,
JsonSafe) or make it package-visible so other classes can use it. Currentprivatescope blocks reuse, which contradicts the “replace across the codebase” goal.- private static <T> T safeFromJson(String json, Class<T> classOfT) { - if (json == null || json.trim().isEmpty() || json.equals("{}")) { + private static <T> T safeFromJson(String json, Class<T> classOfT) { + final String s = json == null ? null : json.trim(); + if (s == null || s.isEmpty() || "{}".equals(s)) { return null; // no need to create a new instance } try { - return gson.fromJson(json, classOfT); + return gson.fromJson(s, classOfT); } catch (Exception e) { android.util.Log.e( "CustomPushNotification", "Failed to parse JSON into " + classOfT.getSimpleName() + " (payload redacted).", e ); return null; } }
45-47: Sweep remaining direct Gson usages across the module; ensure all parsing goes through safeFromJson.There are still local
new Gson()instantiations here and (likely) in adjacent classes. Consolidate for consistency and testability.#!/bin/bash # Repo-wide scan for direct Gson usage that should be routed through safeFromJson rg -nP --type=java '\bnew\s+Gson\s*\(' -S rg -nP --type=java '\bfromJson\s*\(' -S | rg -v 'safeFromJson'I can raise a follow-up PR to extract
safeFromJsoninto a smallnotificationpackage util and update all call sites. Want me to draft it?Also applies to: 151-174, 276-276, 389-403
🧹 Nitpick comments (4)
android/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.java (4)
95-101: Guard decrypt against null React context.
reactApplicationContextcan be null early in process lifetime; add a null-check to avoid crashes.- String decrypted = Encryption.shared.decryptMessage(loadedEjson, reactApplicationContext); + String decrypted = reactApplicationContext != null + ? Encryption.shared.decryptMessage(loadedEjson, reactApplicationContext) + : null;Can you confirm
reactApplicationContextis always set viasetReactContextbefore notifications can arrive on cold start?
261-270: InboxStyle path should also guard null message.Prevent passing null to
addLine.- String message = data.getString("message"); - messageStyle.addLine(message); + String message = data.getString("message"); + messageStyle.addLine(message != null ? message : "");
276-286: Remove unused local Gson in notificationStyle.This instance isn’t used.
- Gson gson = new Gson();
300-312: Harden senderName fallback.If
ejsonexists butsenderNameis null/empty, fall back to a safe default to avoid blank names in UI.- String senderName = ejson != null ? ejson.senderName : "Unknown"; + String senderName = (ejson != null && ejson.senderName != null && !ejson.senderName.isEmpty()) + ? ejson.senderName + : "Unknown"; @@ - String senderName = ejson != null ? ejson.senderName : "Unknown"; + String senderName = (ejson != null && ejson.senderName != null && !ejson.senderName.isEmpty()) + ? ejson.senderName + : "Unknown";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
android/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.java(11 hunks)
🔇 Additional comments (3)
android/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.java (3)
45-47: Good move: single, reusable Gson instance.Centralizing the Gson instance reduces object churn and keeps parsing behavior consistent.
69-72: Null-safe gate for "message-id-only" is correct.This prevents NPEs on malformed payloads before attempting server load.
211-223: Icon path: good null-guards on avatarUri.Large icon is only set when available; Glide fallback in getAvatar() covers failures.
diegolmello
had a problem deploying
to
experimental_ios_build
GitHub Actions
Error
diegolmello
temporarily deployed
to
experimental_android_build
GitHub Actions
Inactive
diegolmello
had a problem deploying
to
official_ios_build
GitHub Actions
Error
diegolmello
had a problem deploying
to
official_android_build
GitHub Actions
Error
diegolmello
had a problem deploying
to
upload_experimental_android
GitHub Actions
Error
|
Android Build Available Rocket.Chat Experimental 4.65.0.107352 Internal App Sharing: https://play.google.com/apps/test/RQVpXLytHNc/ahAO29uNStlxkiH412JATX3b3QdQ_b72KhKzGNOgE72f9mhFTEYa7TMRyaOFdpipnyAlIAQPcl3T-AgPrHyZvIAm4a |
…rors gracefully - Introduced a static method `safeFromJson` to safely parse JSON strings into Ejson objects, returning null for invalid inputs. - Updated all instances of Gson parsing to use `safeFromJson`, ensuring null checks are in place to prevent potential NullPointerExceptions. - Enhanced error logging for JSON parsing failures.
…/CustomPushNotification.java Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
diegolmello
force-pushed
the
fix.push-android
branch
from
3641925 to
850b18b
Compare
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Failure
diegolmello
had a problem deploying
to
official_android_build
GitHub Actions
Failure
diegolmello
had a problem deploying
to
experimental_android_build
GitHub Actions
Failure
diegolmello
had a problem deploying
to
experimental_ios_build
GitHub Actions
Failure
diegolmello
had a problem deploying
to
official_ios_build
GitHub Actions
Failure
2 participants
Proposed changes
safeFromJsonto safely parse JSON strings into Ejson objects, returning null for invalid inputs.safeFromJson, ensuring null checks are in place to prevent potential NullPointerExceptions.Issue(s)
#6661
https://rocketchat.atlassian.net/browse/NATIVE-1026
How to test or reproduce
Screenshots
Types of changes
Checklist
Further comments
Summary by CodeRabbit