fix(ddp): populate reaction user names when UI_Use_Real_Name is enabled#40254
Open
divyanshu-patil wants to merge 11 commits intoRocketChat:developfrom
Open
fix(ddp): populate reaction user names when UI_Use_Real_Name is enabled#40254divyanshu-patil wants to merge 11 commits intoRocketChat:developfrom
divyanshu-patil wants to merge 11 commits intoRocketChat:developfrom
Conversation
Contributor
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughgetMessageToBroadcast now resolves real names for users in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested labels
🚥 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. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/app/lib/server/lib/notifyListener.ts`:
- Around line 479-489: The loop currently skips reactions that have
reaction.usernames but no reaction.names, leaving legacy/imported reactions
without populated display names; update the logic in the notify listener where
reactions are enriched (the block using "for (const [, reaction] of
Object.entries(message.reactions))" and calling getUserNameByUsernameCached) to
initialize reaction.names when missing (e.g., seed it from reaction.usernames or
create an array of the same length) before the inner index loop, then continue
to call getUserNameByUsernameCached(username) and overwrite each index with the
resolved real name when available.
In `@apps/meteor/app/reactions/server/setReaction.ts`:
- Around line 80-84: When initializing a missing names array in setReaction, pad
it to match the current usernames length so indices stay aligned: if
message.reactions[reaction].names is falsy, set it to
Array(message.reactions[reaction].usernames.length).fill('') before appending
the new username and its placeholder; then push the new username to
message.reactions[reaction].usernames and push an empty string to
message.reactions[reaction].names so both arrays remain the same length and
index-aligned.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cea5aa94-e568-4acb-bf87-494801cf9202
📒 Files selected for processing (2)
apps/meteor/app/lib/server/lib/notifyListener.tsapps/meteor/app/reactions/server/setReaction.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/app/lib/server/lib/notifyListener.tsapps/meteor/app/reactions/server/setReaction.ts
🧠 Learnings (8)
📚 Learning: 2026-03-11T22:04:20.529Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 39545
File: apps/meteor/client/views/room/body/hooks/useHasNewMessages.ts:59-61
Timestamp: 2026-03-11T22:04:20.529Z
Learning: In `apps/meteor/client/views/room/body/hooks/useHasNewMessages.ts`, the `msg.u._id === uid` early-return in the `streamNewMessage` handler is intentional: the "New messages" indicator is designed to notify about messages from other users only. Self-sent messages — including those sent from a different session/device — are always skipped, by design. Do not flag this as a multi-session regression.
Applied to files:
apps/meteor/app/lib/server/lib/notifyListener.ts
📚 Learning: 2026-03-09T18:39:21.178Z
Learnt from: Harxhit
Repo: RocketChat/Rocket.Chat PR: 39476
File: apps/meteor/server/methods/addAllUserToRoom.ts:0-0
Timestamp: 2026-03-09T18:39:21.178Z
Learning: In apps/meteor/server/methods/addAllUserToRoom.ts, the implementation uses a single cursor pass (Users.find(userFilter).batchSize(100)) that collects both the full user objects (collectedUsers: IUser[]) and their usernames (usernames: string[]) in one iteration. `beforeAddUserToRoom` is then called once with the full usernames batch (preserving batch-validation semantics), and the subsequent subscription/message processing loop iterates over the same stable `collectedUsers` array — no second DB query is made. This avoids any race condition between validation and processing while preserving the original batch-validation behavior.
Applied to files:
apps/meteor/app/lib/server/lib/notifyListener.tsapps/meteor/app/reactions/server/setReaction.ts
📚 Learning: 2026-01-17T01:51:47.764Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38219
File: packages/core-typings/src/cloud/Announcement.ts:5-6
Timestamp: 2026-01-17T01:51:47.764Z
Learning: In packages/core-typings/src/cloud/Announcement.ts, the AnnouncementSchema.createdBy field intentionally overrides IBannerSchema.createdBy (object with _id and optional username) with a string enum ['cloud', 'system'] to match existing runtime behavior. This is documented as technical debt with a FIXME comment at apps/meteor/app/cloud/server/functions/syncWorkspace/handleCommsSync.ts:53 and should not be flagged as an error until the runtime behavior is corrected.
Applied to files:
apps/meteor/app/lib/server/lib/notifyListener.ts
📚 Learning: 2026-01-19T18:17:46.433Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38262
File: apps/meteor/client/lib/utils/normalizeMessagePreview/getMessagePreview.ts:24-33
Timestamp: 2026-01-19T18:17:46.433Z
Learning: In the Rocket.Chat repository, usernames (lastMessage.u.username) and display names (lastMessage.u.name) are sanitized/escaped centrally elsewhere in the system, so individual display functions like getMessagePreview do not need to escape these values before interpolating them into strings.
Applied to files:
apps/meteor/app/lib/server/lib/notifyListener.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/app/lib/server/lib/notifyListener.tsapps/meteor/app/reactions/server/setReaction.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/app/lib/server/lib/notifyListener.tsapps/meteor/app/reactions/server/setReaction.ts
📚 Learning: 2026-03-20T13:51:23.302Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 39553
File: apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts:179-181
Timestamp: 2026-03-20T13:51:23.302Z
Learning: In `apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts`, the truthiness guards `...(integration.avatar && { avatar })`, `...(integration.emoji && { emoji })`, `...(integration.alias && { alias })`, and `...(integration.script && { script })` in the `$set` payload of `updateIncomingIntegration` are intentional. Empty-string values for these fields should NOT overwrite the stored value — only truthy values are persisted. Do not flag these as bugs preventing explicit clears.
Applied to files:
apps/meteor/app/reactions/server/setReaction.ts
📚 Learning: 2026-04-18T12:32:53.425Z
Learnt from: rodrigok
Repo: RocketChat/Rocket.Chat PR: 38623
File: apps/meteor/app/lib/server/functions/cleanRoomHistory.ts:146-149
Timestamp: 2026-04-18T12:32:53.425Z
Learning: In `apps/meteor/app/lib/server/functions/cleanRoomHistory.ts` (PR `#38623`), the read-receipt cleanup (both `ReadReceipts.removeByMessageIds` and `ReadReceiptsArchive.removeByMessageIds`) is intentionally only performed in the limited prune path (`limit && selectedMessageIds`). The unlimited/delete-all path (`limit === 0`) deliberately skips cleaning up orphaned read receipts in both hot and cold storage — this is by design. Do not flag this as a bug or missing cleanup in future reviews.
Applied to files:
apps/meteor/app/reactions/server/setReaction.ts
🔇 Additional comments (2)
apps/meteor/app/lib/server/lib/notifyListener.ts (1)
432-438: LGTM: cached username lookup is scoped and consistent.The helper mirrors the existing user-id lookup and only projects
name, which keeps repeated reaction enrichment cheap.apps/meteor/app/reactions/server/setReaction.ts (1)
27-28: LGTM: removal keeps normalized arrays aligned.Splicing
namesat the same index asusernamespreserves the parallel array contract for reactions that already havenames.
10 tasks
divyanshu-patil
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/meteor/app/reactions/server/setReaction.ts (1)
80-87: Drop the inline implementation comment.The padding logic is clear from the code, and Line 84 adds an implementation comment against the repository guideline. As per coding guidelines, "Avoid code comments in the implementation".
♻️ Proposed cleanup
- - // Pad to match current usernames length before appending while (message.reactions[reaction].names.length < message.reactions[reaction].usernames.length) { message.reactions[reaction].names.push(''); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/reactions/server/setReaction.ts` around lines 80 - 87, Remove the inline implementation comment "// Pad to match current usernames length before appending" and keep the existing padding logic intact that ensures message.reactions[reaction].names is initialized and while its length is less than message.reactions[reaction].usernames.length push(''); leave the variable checks and loop (the code inside setReaction handling message.reactions[reaction].names and usernames) as-is but delete that single-line comment to comply with the "avoid code comments in the implementation" guideline.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/app/reactions/server/setReaction.ts`:
- Around line 89-93: In setReaction.ts the code for appending a user's reaction
is duplicated: message.reactions[reaction].usernames.push(user.username as
string) and message.reactions[reaction].names.push('') are called twice, causing
duplicate entries; remove the second pair so you only push the username and
placeholder name once when adding a reaction (i.e., keep a single push to
message.reactions[reaction].usernames and a single push to
message.reactions[reaction].names in the add-reaction branch).
---
Nitpick comments:
In `@apps/meteor/app/reactions/server/setReaction.ts`:
- Around line 80-87: Remove the inline implementation comment "// Pad to match
current usernames length before appending" and keep the existing padding logic
intact that ensures message.reactions[reaction].names is initialized and while
its length is less than message.reactions[reaction].usernames.length push('');
leave the variable checks and loop (the code inside setReaction handling
message.reactions[reaction].names and usernames) as-is but delete that
single-line comment to comply with the "avoid code comments in the
implementation" guideline.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ceddb9b5-657d-4174-8c15-ea5978a637b6
📒 Files selected for processing (2)
apps/meteor/app/lib/server/lib/notifyListener.tsapps/meteor/app/reactions/server/setReaction.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/meteor/app/lib/server/lib/notifyListener.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/app/reactions/server/setReaction.ts
🧠 Learnings (6)
📓 Common learnings
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38262
File: apps/meteor/client/lib/utils/normalizeMessagePreview/getMessagePreview.ts:24-33
Timestamp: 2026-01-19T18:17:46.433Z
Learning: In the Rocket.Chat repository, usernames (lastMessage.u.username) and display names (lastMessage.u.name) are sanitized/escaped centrally elsewhere in the system, so individual display functions like getMessagePreview do not need to escape these values before interpolating them into strings.
📚 Learning: 2026-04-18T12:32:53.425Z
Learnt from: rodrigok
Repo: RocketChat/Rocket.Chat PR: 38623
File: apps/meteor/app/lib/server/functions/cleanRoomHistory.ts:146-149
Timestamp: 2026-04-18T12:32:53.425Z
Learning: In `apps/meteor/app/lib/server/functions/cleanRoomHistory.ts` (PR `#38623`), the read-receipt cleanup (both `ReadReceipts.removeByMessageIds` and `ReadReceiptsArchive.removeByMessageIds`) is intentionally only performed in the limited prune path (`limit && selectedMessageIds`). The unlimited/delete-all path (`limit === 0`) deliberately skips cleaning up orphaned read receipts in both hot and cold storage — this is by design. Do not flag this as a bug or missing cleanup in future reviews.
Applied to files:
apps/meteor/app/reactions/server/setReaction.ts
📚 Learning: 2026-03-20T13:51:23.302Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 39553
File: apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts:179-181
Timestamp: 2026-03-20T13:51:23.302Z
Learning: In `apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts`, the truthiness guards `...(integration.avatar && { avatar })`, `...(integration.emoji && { emoji })`, `...(integration.alias && { alias })`, and `...(integration.script && { script })` in the `$set` payload of `updateIncomingIntegration` are intentional. Empty-string values for these fields should NOT overwrite the stored value — only truthy values are persisted. Do not flag these as bugs preventing explicit clears.
Applied to files:
apps/meteor/app/reactions/server/setReaction.ts
📚 Learning: 2026-03-09T18:39:21.178Z
Learnt from: Harxhit
Repo: RocketChat/Rocket.Chat PR: 39476
File: apps/meteor/server/methods/addAllUserToRoom.ts:0-0
Timestamp: 2026-03-09T18:39:21.178Z
Learning: In apps/meteor/server/methods/addAllUserToRoom.ts, the implementation uses a single cursor pass (Users.find(userFilter).batchSize(100)) that collects both the full user objects (collectedUsers: IUser[]) and their usernames (usernames: string[]) in one iteration. `beforeAddUserToRoom` is then called once with the full usernames batch (preserving batch-validation semantics), and the subsequent subscription/message processing loop iterates over the same stable `collectedUsers` array — no second DB query is made. This avoids any race condition between validation and processing while preserving the original batch-validation behavior.
Applied to files:
apps/meteor/app/reactions/server/setReaction.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/app/reactions/server/setReaction.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/app/reactions/server/setReaction.ts
🔇 Additional comments (2)
apps/meteor/app/reactions/server/setReaction.ts (2)
27-28: LGTM — removal keeps reaction arrays aligned.Line 28 correctly removes the matching
namesentry at the same index asusernames.
74-78: LGTM — new reactions now initializenames.This keeps the stored reaction shape ready for the broadcast real-name population path.
Contributor
Author
|
@coderabbitai review |
Contributor
🧩 Analysis chainI'll pull the relevant code before writing up my review. 🏁 Script executed: Length of output: 21099 PR Description Images:
Review:
|
| # | Severity | Item |
|---|---|---|
| 1 | 🔶 Medium | Guard against findByUsernames([]) when all reactions are empty |
| 2 | 🔹 Minor | ` |
| 3 | 🔹 Minor | Two traversals of message.reactions can be one |
| 4 | 🔶 Medium | No unit tests for the new logic |
| 5 | 🔹 Minor | PR description references a helper that doesn't exist in the diff |
The core approach is sound. Addressing items 1 and 4 before merge would be ideal.
🧠 Learnings used
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38262
File: apps/meteor/client/lib/utils/normalizeMessagePreview/getMessagePreview.ts:24-33
Timestamp: 2026-01-19T18:17:46.433Z
Learning: In the Rocket.Chat repository, usernames (lastMessage.u.username) and display names (lastMessage.u.name) are sanitized/escaped centrally elsewhere in the system, so individual display functions like getMessagePreview do not need to escape these values before interpolating them into strings.
✅ Actions performed
Review triggered.
Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/meteor/app/lib/server/lib/notifyListener.ts (1)
478-488: Consider falling back tousernameinstead of empty string.When a reacting user has no
nameset (or the user record is missing — e.g., deleted account),nameByUsername.get(username) || ''yields'', which will render as a blank entry in the Reactions sheet (the very symptom this PR fixes). Falling back tousernamepreserves a meaningful label and matches the prior behavior beforeUI_Use_Real_Nameenrichment.Also worth noting: an empty
allUsernamesset (all reactions have emptyusernamesarrays — unlikely but possible) will still issue afindByUsernames([])query; a quickif (!allUsernames.size) { ... skip block }avoids it.♻️ Proposed fix
- const users = await Users.findByUsernames([...allUsernames], { - projection: { username: 1, name: 1 }, - }).toArray(); - - const nameByUsername = new Map(users.map((u) => [u.username, u.name])); - - for (const [, reaction] of Object.entries(message.reactions)) { - if (!reaction.usernames?.length) { - continue; - } - reaction.names = reaction.usernames.map((username) => nameByUsername.get(username) || ''); - } + if (allUsernames.size) { + const users = await Users.findByUsernames([...allUsernames], { + projection: { username: 1, name: 1 }, + }).toArray(); + + const nameByUsername = new Map(users.map((u) => [u.username, u.name])); + + for (const [, reaction] of Object.entries(message.reactions)) { + if (!reaction.usernames?.length) { + continue; + } + reaction.names = reaction.usernames.map((username) => nameByUsername.get(username) || username); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/lib/server/lib/notifyListener.ts` around lines 478 - 488, The reactions rendering code currently maps reaction.usernames to names using nameByUsername.get(username) || '' which yields empty strings when a real name is missing; change it to fall back to the username (e.g., nameByUsername.get(username) || username) inside the loop that assigns reaction.names, and guard the Users.findByUsernames call by skipping the DB query when the collected allUsernames set is empty (if (!allUsernames.size) return/continue) so you don't call findByUsernames([]); update references around Users.findByUsernames, the nameByUsername Map, and the for (const [, reaction] of Object.entries(message.reactions)) block accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/app/lib/server/lib/notifyListener.ts`:
- Line 472: Remove the implementation comment "// Collect all unique usernames
across all reactions" from apps/meteor/app/lib/server/lib/notifyListener.ts;
locate the comment inside the notification/reactions processing block (look for
variables like reactions or uniqueUsernames within the notify listener function)
and delete that line so the code remains self-explanatory and follows the
guideline to avoid implementation comments.
---
Nitpick comments:
In `@apps/meteor/app/lib/server/lib/notifyListener.ts`:
- Around line 478-488: The reactions rendering code currently maps
reaction.usernames to names using nameByUsername.get(username) || '' which
yields empty strings when a real name is missing; change it to fall back to the
username (e.g., nameByUsername.get(username) || username) inside the loop that
assigns reaction.names, and guard the Users.findByUsernames call by skipping the
DB query when the collected allUsernames set is empty (if (!allUsernames.size)
return/continue) so you don't call findByUsernames([]); update references around
Users.findByUsernames, the nameByUsername Map, and the for (const [, reaction]
of Object.entries(message.reactions)) block accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 314c3473-5580-4e06-bbf3-8c3a8e58f0bf
📒 Files selected for processing (1)
apps/meteor/app/lib/server/lib/notifyListener.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/app/lib/server/lib/notifyListener.ts
🧠 Learnings (9)
📓 Common learnings
Learnt from: smirk-dev
Repo: RocketChat/Rocket.Chat PR: 39625
File: apps/meteor/app/api/server/v1/push.ts:85-97
Timestamp: 2026-03-14T14:58:58.834Z
Learning: In RocketChat/Rocket.Chat, the `push.token` POST/DELETE endpoints in `apps/meteor/app/api/server/v1/push.ts` were already migrated to the chained router API pattern on `develop` prior to PR `#39625`. `cleanTokenResult` (which strips `authToken` and returns `PushTokenResult`) and `isPushTokenPOSTProps`/`isPushTokenDELETEProps` validators already exist on `develop`. PR `#39625` only migrates `push.get` and `push.info` to the chained pattern. Do not flag `cleanTokenResult` or `PushTokenResult` as newly introduced behavior-breaking changes when reviewing this PR.
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38262
File: apps/meteor/client/lib/utils/normalizeMessagePreview/getMessagePreview.ts:24-33
Timestamp: 2026-01-19T18:17:46.433Z
Learning: In the Rocket.Chat repository, usernames (lastMessage.u.username) and display names (lastMessage.u.name) are sanitized/escaped centrally elsewhere in the system, so individual display functions like getMessagePreview do not need to escape these values before interpolating them into strings.
📚 Learning: 2026-03-11T22:04:20.529Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 39545
File: apps/meteor/client/views/room/body/hooks/useHasNewMessages.ts:59-61
Timestamp: 2026-03-11T22:04:20.529Z
Learning: In `apps/meteor/client/views/room/body/hooks/useHasNewMessages.ts`, the `msg.u._id === uid` early-return in the `streamNewMessage` handler is intentional: the "New messages" indicator is designed to notify about messages from other users only. Self-sent messages — including those sent from a different session/device — are always skipped, by design. Do not flag this as a multi-session regression.
Applied to files:
apps/meteor/app/lib/server/lib/notifyListener.ts
📚 Learning: 2026-03-09T18:39:21.178Z
Learnt from: Harxhit
Repo: RocketChat/Rocket.Chat PR: 39476
File: apps/meteor/server/methods/addAllUserToRoom.ts:0-0
Timestamp: 2026-03-09T18:39:21.178Z
Learning: In apps/meteor/server/methods/addAllUserToRoom.ts, the implementation uses a single cursor pass (Users.find(userFilter).batchSize(100)) that collects both the full user objects (collectedUsers: IUser[]) and their usernames (usernames: string[]) in one iteration. `beforeAddUserToRoom` is then called once with the full usernames batch (preserving batch-validation semantics), and the subsequent subscription/message processing loop iterates over the same stable `collectedUsers` array — no second DB query is made. This avoids any race condition between validation and processing while preserving the original batch-validation behavior.
Applied to files:
apps/meteor/app/lib/server/lib/notifyListener.ts
📚 Learning: 2026-01-19T18:17:46.433Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38262
File: apps/meteor/client/lib/utils/normalizeMessagePreview/getMessagePreview.ts:24-33
Timestamp: 2026-01-19T18:17:46.433Z
Learning: In the Rocket.Chat repository, usernames (lastMessage.u.username) and display names (lastMessage.u.name) are sanitized/escaped centrally elsewhere in the system, so individual display functions like getMessagePreview do not need to escape these values before interpolating them into strings.
Applied to files:
apps/meteor/app/lib/server/lib/notifyListener.ts
📚 Learning: 2026-04-18T12:32:53.425Z
Learnt from: rodrigok
Repo: RocketChat/Rocket.Chat PR: 38623
File: apps/meteor/app/lib/server/functions/cleanRoomHistory.ts:146-149
Timestamp: 2026-04-18T12:32:53.425Z
Learning: In `apps/meteor/app/lib/server/functions/cleanRoomHistory.ts` (PR `#38623`), the read-receipt cleanup (both `ReadReceipts.removeByMessageIds` and `ReadReceiptsArchive.removeByMessageIds`) is intentionally only performed in the limited prune path (`limit && selectedMessageIds`). The unlimited/delete-all path (`limit === 0`) deliberately skips cleaning up orphaned read receipts in both hot and cold storage — this is by design. Do not flag this as a bug or missing cleanup in future reviews.
Applied to files:
apps/meteor/app/lib/server/lib/notifyListener.ts
📚 Learning: 2026-01-17T01:51:47.764Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38219
File: packages/core-typings/src/cloud/Announcement.ts:5-6
Timestamp: 2026-01-17T01:51:47.764Z
Learning: In packages/core-typings/src/cloud/Announcement.ts, the AnnouncementSchema.createdBy field intentionally overrides IBannerSchema.createdBy (object with _id and optional username) with a string enum ['cloud', 'system'] to match existing runtime behavior. This is documented as technical debt with a FIXME comment at apps/meteor/app/cloud/server/functions/syncWorkspace/handleCommsSync.ts:53 and should not be flagged as an error until the runtime behavior is corrected.
Applied to files:
apps/meteor/app/lib/server/lib/notifyListener.ts
📚 Learning: 2026-03-20T13:51:23.302Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 39553
File: apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts:179-181
Timestamp: 2026-03-20T13:51:23.302Z
Learning: In `apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts`, the truthiness guards `...(integration.avatar && { avatar })`, `...(integration.emoji && { emoji })`, `...(integration.alias && { alias })`, and `...(integration.script && { script })` in the `$set` payload of `updateIncomingIntegration` are intentional. Empty-string values for these fields should NOT overwrite the stored value — only truthy values are persisted. Do not flag these as bugs preventing explicit clears.
Applied to files:
apps/meteor/app/lib/server/lib/notifyListener.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/app/lib/server/lib/notifyListener.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/app/lib/server/lib/notifyListener.ts
🔇 Additional comments (1)
apps/meteor/app/lib/server/lib/notifyListener.ts (1)
471-490: Summary description is out of sync with the implementation.The PR summary describes a new memoized helper
getUserNameByUsernameCached(maxAge 10s) alongsidegetUserNameCached. The actual implementation takes a better approach: one batchedUsers.findByUsernamesper message rather than per-username cached lookups. The implementation is preferable — please update the PR description (and any mobile-PR linkage notes) so reviewers/backporters aren't misled.
divyanshu-patil
force-pushed
the
fix/reaction-names
branch
from
c251783 to
ed71553
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/app/lib/server/lib/notifyListener.ts`:
- Around line 471-490: Collecting usernames into allUsernames and then calling
Users.findByUsernames when the set is empty causes an unnecessary DB query;
before invoking Users.findByUsernames([...allUsernames], ...) inside the
message.reactions handling, guard on allUsernames.size (or length of the spread
array) and skip the DB call and mapping if it's zero, ensuring you still iterate
reactions to set reaction.names (to empty arrays) as needed; update the logic
around Users.findByUsernames, nameByUsername, and reaction.names to only perform
the DB round-trip when there are usernames to look up.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dd9db920-b815-49c0-a4bb-e7cdd8fa1199
📒 Files selected for processing (1)
apps/meteor/app/lib/server/lib/notifyListener.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/app/lib/server/lib/notifyListener.ts
🧠 Learnings (11)
📓 Common learnings
Learnt from: smirk-dev
Repo: RocketChat/Rocket.Chat PR: 39625
File: apps/meteor/app/api/server/v1/push.ts:85-97
Timestamp: 2026-03-14T14:58:58.834Z
Learning: In RocketChat/Rocket.Chat, the `push.token` POST/DELETE endpoints in `apps/meteor/app/api/server/v1/push.ts` were already migrated to the chained router API pattern on `develop` prior to PR `#39625`. `cleanTokenResult` (which strips `authToken` and returns `PushTokenResult`) and `isPushTokenPOSTProps`/`isPushTokenDELETEProps` validators already exist on `develop`. PR `#39625` only migrates `push.get` and `push.info` to the chained pattern. Do not flag `cleanTokenResult` or `PushTokenResult` as newly introduced behavior-breaking changes when reviewing this PR.
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38262
File: apps/meteor/client/lib/utils/normalizeMessagePreview/getMessagePreview.ts:24-33
Timestamp: 2026-01-19T18:17:46.433Z
Learning: In the Rocket.Chat repository, usernames (lastMessage.u.username) and display names (lastMessage.u.name) are sanitized/escaped centrally elsewhere in the system, so individual display functions like getMessagePreview do not need to escape these values before interpolating them into strings.
📚 Learning: 2026-03-11T22:04:20.529Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 39545
File: apps/meteor/client/views/room/body/hooks/useHasNewMessages.ts:59-61
Timestamp: 2026-03-11T22:04:20.529Z
Learning: In `apps/meteor/client/views/room/body/hooks/useHasNewMessages.ts`, the `msg.u._id === uid` early-return in the `streamNewMessage` handler is intentional: the "New messages" indicator is designed to notify about messages from other users only. Self-sent messages — including those sent from a different session/device — are always skipped, by design. Do not flag this as a multi-session regression.
Applied to files:
apps/meteor/app/lib/server/lib/notifyListener.ts
📚 Learning: 2026-01-17T01:51:47.764Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38219
File: packages/core-typings/src/cloud/Announcement.ts:5-6
Timestamp: 2026-01-17T01:51:47.764Z
Learning: In packages/core-typings/src/cloud/Announcement.ts, the AnnouncementSchema.createdBy field intentionally overrides IBannerSchema.createdBy (object with _id and optional username) with a string enum ['cloud', 'system'] to match existing runtime behavior. This is documented as technical debt with a FIXME comment at apps/meteor/app/cloud/server/functions/syncWorkspace/handleCommsSync.ts:53 and should not be flagged as an error until the runtime behavior is corrected.
Applied to files:
apps/meteor/app/lib/server/lib/notifyListener.ts
📚 Learning: 2026-01-19T18:17:46.433Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38262
File: apps/meteor/client/lib/utils/normalizeMessagePreview/getMessagePreview.ts:24-33
Timestamp: 2026-01-19T18:17:46.433Z
Learning: In the Rocket.Chat repository, usernames (lastMessage.u.username) and display names (lastMessage.u.name) are sanitized/escaped centrally elsewhere in the system, so individual display functions like getMessagePreview do not need to escape these values before interpolating them into strings.
Applied to files:
apps/meteor/app/lib/server/lib/notifyListener.ts
📚 Learning: 2026-04-18T12:32:53.425Z
Learnt from: rodrigok
Repo: RocketChat/Rocket.Chat PR: 38623
File: apps/meteor/app/lib/server/functions/cleanRoomHistory.ts:146-149
Timestamp: 2026-04-18T12:32:53.425Z
Learning: In `apps/meteor/app/lib/server/functions/cleanRoomHistory.ts` (PR `#38623`), the read-receipt cleanup (both `ReadReceipts.removeByMessageIds` and `ReadReceiptsArchive.removeByMessageIds`) is intentionally only performed in the limited prune path (`limit && selectedMessageIds`). The unlimited/delete-all path (`limit === 0`) deliberately skips cleaning up orphaned read receipts in both hot and cold storage — this is by design. Do not flag this as a bug or missing cleanup in future reviews.
Applied to files:
apps/meteor/app/lib/server/lib/notifyListener.ts
📚 Learning: 2026-03-20T13:51:23.302Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 39553
File: apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts:179-181
Timestamp: 2026-03-20T13:51:23.302Z
Learning: In `apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts`, the truthiness guards `...(integration.avatar && { avatar })`, `...(integration.emoji && { emoji })`, `...(integration.alias && { alias })`, and `...(integration.script && { script })` in the `$set` payload of `updateIncomingIntegration` are intentional. Empty-string values for these fields should NOT overwrite the stored value — only truthy values are persisted. Do not flag these as bugs preventing explicit clears.
Applied to files:
apps/meteor/app/lib/server/lib/notifyListener.ts
📚 Learning: 2026-03-09T18:39:21.178Z
Learnt from: Harxhit
Repo: RocketChat/Rocket.Chat PR: 39476
File: apps/meteor/server/methods/addAllUserToRoom.ts:0-0
Timestamp: 2026-03-09T18:39:21.178Z
Learning: In apps/meteor/server/methods/addAllUserToRoom.ts, the implementation uses a single cursor pass (Users.find(userFilter).batchSize(100)) that collects both the full user objects (collectedUsers: IUser[]) and their usernames (usernames: string[]) in one iteration. `beforeAddUserToRoom` is then called once with the full usernames batch (preserving batch-validation semantics), and the subsequent subscription/message processing loop iterates over the same stable `collectedUsers` array — no second DB query is made. This avoids any race condition between validation and processing while preserving the original batch-validation behavior.
Applied to files:
apps/meteor/app/lib/server/lib/notifyListener.ts
📚 Learning: 2026-03-04T14:16:49.202Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39304
File: packages/ui-contexts/src/ActionManagerContext.ts:26-26
Timestamp: 2026-03-04T14:16:49.202Z
Learning: In `packages/ui-contexts/src/ActionManagerContext.ts` (TypeScript, RocketChat/Rocket.Chat), the `disposeView` method in `IActionManager` uses an intentionally explicit union `UiKit.ModalView['id'] | UiKit.BannerView['viewId'] | UiKit.ContextualBarView['id']` to document which view types are accepted, even though all constituents resolve to the same primitive. The inline `// eslint-disable-next-line typescript-eslint/no-duplicate-type-constituents` comment is intentional and should not be flagged or removed.
Applied to files:
apps/meteor/app/lib/server/lib/notifyListener.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to **/*.{ts,tsx,js} : Avoid code comments in the implementation
Applied to files:
apps/meteor/app/lib/server/lib/notifyListener.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/app/lib/server/lib/notifyListener.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/app/lib/server/lib/notifyListener.ts
🔇 Additional comments (1)
apps/meteor/app/lib/server/lib/notifyListener.ts (1)
472-472: Remove implementation comment.As per coding guidelines (
**/*.{ts,tsx,js}: "Avoid code comments in the implementation"), drop this self-explanatory comment.♻️ Proposed fix
if (message.reactions) { - // Collect all unique usernames across all reactions const allUsernames = new Set<string>();
10 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed changes (including videos or screenshots)
When
UI_Use_Real_Nameis enabled, thegetMessageToBroadcastfunction was already enriching the message sender's name and mentioned users' names before broadcasting. However, it was not doing the same for reaction user names — onlyusernameswere present in the broadcasted payload, leavingnamesempty.This PR adds a loop inside
getMessageToBroadcastthat iterates over all reactions in a message and populatesreaction.names[i]by calling the memoizedgetUserNameByUsernameCachedlookup for each username inreaction.usernames. This mirrors the existing pattern used for mentions.Issue(s)
N/A (discovered during mobile testing)
Linked PR
RocketChat/Rocket.Chat.ReactNative#7204
Screenshots
Before
After (UI_Use_Real_Name is true)
Steps to test or reproduce
UI_Use_Real_Namein Admin → General settingsBefore: user names in each tab are blank
After: user real names are correctly displayed under each emoji tab
Further comments
The fix follows the same memoized lookup pattern already used for
message.uandmessage.mentionsingetMessageToBroadcast. A new cached helpergetUserNameByUsernameCached(keyed by username rather than user ID) is introduced alongside the existinggetUserNameCachedto avoid redundant DB calls when multiple users react with the same emoji. ThemaxAgeis kept consistent at 10 seconds.Summary by CodeRabbit