fix: push notification titles and avatars for discussions and threads #6900
Conversation
diegolmello
requested a deployment
to
approve_e2e_testing
GitHub Actions
Waiting
|
Caution Review failedThe pull request is closed. WalkthroughStandardizes sender-name selection and avatar-URI construction; simplifies Android notification title/conversation-title logic; adds early fetch flow for message-id-only pushes; and restructures iOS NotificationService payload handling and defensive title/body assignment. Changes
Sequence Diagram(s)sequenceDiagram
participant Push as Push Receiver
participant Parser as EJSON Parser
participant Fetch as Message Fetcher
participant Builder as Notification Builder
participant OS as Notification Manager
Push->>Parser: receive push -> parse ejson
Parser->>Parser: compute displaySenderName, check type
alt payload is message-id-only
Parser->>Fetch: fetch full message content
Fetch-->>Parser: return full payload (update ejson, title, body)
end
Parser->>Builder: build notification (title/body/avatar using displaySenderName & base title)
Builder->>OS: display notification
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (4)
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
had a problem deploying
to
official_android_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
temporarily deployed
to
experimental_ios_build
GitHub Actions
Inactive
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
android/app/src/main/java/chat/rocket/reactnative/notification/Ejson.java (1)
77-82: Consider URL-encoding credentials for consistency, though not strictly required.The code correctly generates avatar URIs with optional credentials. While
userTokenanduidare appended without encoding, Rocket.Chat tokens use only URL-safe characters (A–Z, a–z, 0–9,-,_) and user IDs are alphanumeric, so the URL remains well-formed. However, the method already usesURLEncoderelsewhere foravatarPathcomponents (lines 94, 105, 127), creating an inconsistency. For consistency and defense-in-depth, consider encoding these query parameters:if (!userToken.isEmpty() && !uid.isEmpty()) { try { finalUri += "&rc_token=" + URLEncoder.encode(userToken, "UTF-8") + "&rc_uid=" + URLEncoder.encode(uid, "UTF-8"); } catch (UnsupportedEncodingException e) { Log.e(TAG, "Failed to encode credentials", e); } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
android/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.javaandroid/app/src/main/java/chat/rocket/reactnative/notification/Ejson.javaios/NotificationService/NotificationService.swift
🧰 Additional context used
🧬 Code graph analysis (1)
ios/NotificationService/NotificationService.swift (1)
ios/Shared/Extensions/String+Extensions.swift (1)
removeTrailingSlash(25-31)
⏰ 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 (6)
ios/NotificationService/NotificationService.swift (4)
17-32: Clean refactoring of the entry point.The if-let binding chain is cleaner than a guard-let cascade here since it allows a natural fallback in the else branch. Correctly returns the original
request.contentwhen payload parsing fails.
82-84: Correctly preserves server-provided titles.This conditional assignment ensures human-readable room names from the payload aren't overwritten by the sender name, addressing the core issue for discussions and threads.
117-124: Consistent use of computed title for group name.Using
bestAttemptContent.titleforgroupNameensures the communication intent displays the same human-readable name as the notification title, maintaining consistency across iOS notification surfaces.
213-225: Title from server-provided notification is preserved correctly.The code sets
bestAttemptContent.titlefrom the notification returned bygetPushWithIdbefore callingprocessPayload. SinceprocessPayloadonly assignssenderNamewhentitle.isEmpty, the server-provided title (which includes the proper discussion/thread name) is preserved and won't be overwritten.android/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.java (2)
162-166: LGTM! Early return prevents double processing.The early return after
loadNotificationAndProcessis correct—the notification will be processed asynchronously in the callback after fetching full content from the server, avoiding duplicate calls toprocessNotification().
373-374: Title field handling is safe and confirmed by server fetch logic.The simplified logic at lines 374 and 535 that directly uses the bundle title is valid. LoadNotification.java confirms that the server response includes a
titlefield (line 200:bundle.putString("title", json.data.notification.title)), and message-id-only notifications properly fetch this from the server before processing. The code consistently relies on the server providing the title field, which aligns with removing the previous type-based fallback logic.No evidence of notification types that exclude the title field. The change is correct and removes unnecessary type-checking while maintaining UX by depending on server-provided titles.
diegolmello
had a problem deploying
to
upload_experimental_android
GitHub Actions
Error
|
Android Build Available Rocket.Chat Experimental 4.69.0.108004 Internal App Sharing: https://play.google.com/apps/test/RQVpXLytHNc/ahAO29uNTprCWSTau0A58sU5qhKHvuOKCmuflidkzIziZDqL4go7TFahzkHHgcz7Tvff4NoyMM0Cr26GZ_VkJCWbex |
|
iOS Build Available Rocket.Chat Experimental 4.69.0.108005 |
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
diegolmello
had a problem deploying
to
official_ios_build
GitHub Actions
Error
diegolmello
temporarily deployed
to
experimental_ios_build
GitHub Actions
Inactive
diegolmello
had a problem deploying
to
official_android_build
GitHub Actions
Error
diegolmello
temporarily deployed
to
experimental_android_build
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
@android/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.java:
- Around line 551-561: The fallback for the sender display name is inconsistent
with showNotification: update the logic that computes displaySenderName (used in
CustomPushNotification within the message-style branch) to use
ejson.sender.username as the fallback instead of ejson.sender.name, and ensure
the final fallback remains the intended value (e.g., "Unknown" or the same title
used by showNotification if desired); locate the variable displaySenderName and
the branches where ejson.sender is accessed and replace the reference to
sender.name with sender.username to keep display behavior consistent with
showNotification.
- Around line 292-297: The sender name fallback is inconsistent: in
CustomPushNotification you build displaySenderName using ejson.sender.username
but elsewhere (notificationStyle and VideoConfNotification) use
ejson.sender.name; update the construction of displaySenderName (and the
username bundle entry) to use ejson.sender.name with the same null/empty checks
(i.e., replace references to ejson.sender.username with ejson.sender.name) so
notificationStyle, VideoConfNotification, and this bundle all display the same
sender value.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
android/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.javaandroid/app/src/main/java/chat/rocket/reactnative/notification/VideoConfNotification.ktios/NotificationService/NotificationService.swift
🧰 Additional context used
🧬 Code graph analysis (1)
ios/NotificationService/NotificationService.swift (1)
ios/Shared/Extensions/String+Extensions.swift (1)
removeTrailingSlash(25-31)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: ESLint and Test / run-eslint-and-test
- GitHub Check: format
🔇 Additional comments (7)
android/app/src/main/java/chat/rocket/reactnative/notification/VideoConfNotification.kt (1)
94-94: LGTM! Sender name fallback prioritization is correct.The changes correctly prioritize
ejson.senderNameas the primary source for caller name display, withcaller.name/sender.nameas fallbacks. This aligns with the PR's objective to standardize sender name handling across notification types.Also applies to: 98-98
android/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.java (1)
378-378: LGTM! Title simplification aligns with PR objectives.Removing the dynamic title selection logic and consistently using the bundle
titlefield directly addresses the PR's goal of prioritizing the human-readable room name provided in the notification payload. This ensures discussions and threads display their proper names rather than falling back to technical IDs or sender names.Also applies to: 539-539
ios/NotificationService/NotificationService.swift (5)
17-32: LGTM! Well-structured routing logic.The restructuring from nested if-let to guard-let with dedicated handler routing improves code clarity and maintainability. The addition of the general
processPayloadhandler (line 28) and the graceful fallback to original content on parsing failure (line 31) enhance robustness.
51-51: LGTM! Caller name priority aligns with PR objectives.Prioritizing
payload.senderNameensures consistency with the Android changes and aligns with the PR objective to standardize sender name handling across platforms.
79-84: LGTM! Defensive title preservation.The
isEmptycheck ensures that existing titles (e.g., human-readable room/discussion names from the payload) are not overwritten with sender names. This directly addresses the PR objective to preserve correct notification titles for discussions and threads.
123-123: LGTM! Correct group name propagation.Using
bestAttemptContent.titleas thegroupNameensures that the preserved human-readable room name is correctly propagated to the iOS communication intent, aligning with the PR objective to display correct titles for discussions and threads.
305-310: Verify alignment with PR objectives for avatar resolution.The PR objectives mention "allowing avatar URI generation even when local credentials are not immediately available, improving reliability for public rooms and discussions." However,
fetchAvatarDatastill requires credentials and returnsnilif unavailable (lines 307-310).While the Android changes (Ejson.java) may handle URI generation differently, please confirm whether the iOS implementation should also support avatar fetching for public rooms/discussions without local credentials, or if the current iOS behavior is intentional due to platform differences in avatar data fetching (direct binary fetch vs. URI construction).
android/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.java
Show resolved
Hide resolved
android/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.java
Show resolved
Hide resolved
|
iOS Build Available Rocket.Chat Experimental 4.69.0.108008 |
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
diegolmello
temporarily deployed
to
experimental_ios_build
GitHub Actions
Inactive
diegolmello
had a problem deploying
to
official_ios_build
GitHub Actions
Error
diegolmello
had a problem deploying
to
experimental_android_build
GitHub Actions
Error
diegolmello
temporarily deployed
to
official_android_build
GitHub Actions
Inactive
diegolmello
had a problem deploying
to
upload_official_android
GitHub Actions
Error
|
Android Build Available Rocket.Chat 4.69.0.108009 Internal App Sharing: https://play.google.com/apps/test/RQQ8k09hlnQ/ahAO29uNT-eG-7Q_sLFSn9AtxOVMyeq9C5XtpkKRaJcmVoZnYmpEvJaoAQCuoTJ93i-Dz_YARjBcFoCHTgBzCcLdJq |
|
iOS Build Available Rocket.Chat Experimental 4.69.0.108010 |
OtavioStasiak
left a comment
OtavioStasiak
left a comment
There was a problem hiding this comment.
Looks good to me!
Tested on both platforms.
…de in CustomPushNotification and Ejson classes
…r improved clarity
diegolmello
force-pushed
the
fix.push-thread-discussion
branch
from
2dbe2fb to
11a6691
Compare
diegolmello
requested a deployment
to
approve_e2e_testing
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
diegolmello
requested a deployment
to
official_android_build
GitHub Actions
Waiting
Proposed changes
This PR fixes several inconsistencies in push notification display for discussions and threads on both Android and iOS.
It ensures that the human-readable room name provided in the notification payload is correctly displayed as the title, instead of falling back to technical room IDs or sender names. It also aligns the avatar resolution logic with the main application, improving reliability for public rooms and discussions by allowing URI generation even when local credentials are not immediately available.
Issue(s)
https://rocketchat.atlassian.net/browse/CORE-1649
How to test or reproduce
Screenshots
N/A (Verified with logs and visual inspection on physical devices)
Types of changes
Checklist
Further comments
The fix involves:
titlefield inCustomPushNotification.javaand updatingEjson.javato generate avatar URIs without requiring strict local credential presence.NotificationService.swiftto use the original title as thegroupNamein communication intents and ensuring the title is updated when fetching content for "message-id-only" notifications.Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.