fix: Improve feedback messages when adding users already in room #37310
Conversation
|
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 |
|
WalkthroughThe Changes
Sequence DiagramsequenceDiagram
participant User
participant AddUsers as AddUsers.tsx
participant Server as addUsersToRoom
participant Room as Room Logic
User->>AddUsers: Click "Add Users"
AddUsers->>Server: saveAction({ rid, users })
Server->>Room: Process user additions
Room->>Room: Track added vs already present
Room-->>Server: { added, alreadyInRoom, total }
Server-->>AddUsers: Return counts object
AddUsers->>AddUsers: Compute message based on counts
alt All already present
AddUsers->>User: Show "users already in room" toast
else Some added, some present
AddUsers->>User: Show partial success toast
else All newly added
AddUsers->>User: Show success toast (federated aware)
end
AddUsers->>AddUsers: Call onClickBack & reload
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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 (2)
apps/meteor/app/lib/server/methods/addUsersToRoom.ts(5 hunks)apps/meteor/client/views/room/contextualBar/RoomMembers/AddUsers/AddUsers.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: Dnouv
PR: RocketChat/Rocket.Chat#37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings (mapping subscription documents to room IDs), never undefined, even when user has no room subscriptions.
Learnt from: ricardogarim
PR: RocketChat/Rocket.Chat#37205
File: ee/packages/federation-matrix/src/FederationMatrix.ts:296-301
Timestamp: 2025-10-28T16:53:42.761Z
Learning: In the Rocket.Chat federation-matrix integration (ee/packages/federation-matrix/), the createRoom method from rocket.chat/federation-sdk will support a 4-argument signature (userId, roomName, visibility, displayName) in newer versions. Code using this 4-argument call is forward-compatible with planned library updates and should not be flagged as an error.
Learnt from: ricardogarim
PR: RocketChat/Rocket.Chat#37205
File: ee/packages/federation-matrix/src/FederationMatrix.ts:296-301
Timestamp: 2025-10-28T16:53:42.761Z
Learning: In the Rocket.Chat federation-matrix integration (ee/packages/federation-matrix/), the createRoom method from rocket.chat/federation-sdk will support a 4-argument signature (userId, roomName, visibility, displayName) in newer versions. Code using this 4-argument call is forward-compatible with planned library updates and should not be flagged as an error.
Learnt from: Dnouv
PR: RocketChat/Rocket.Chat#37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings by mapping subscription documents to room IDs, never undefined, even when user has no room subscriptions.
📚 Learning: 2025-10-28T16:53:42.761Z
Learnt from: ricardogarim
PR: RocketChat/Rocket.Chat#37205
File: ee/packages/federation-matrix/src/FederationMatrix.ts:296-301
Timestamp: 2025-10-28T16:53:42.761Z
Learning: In the Rocket.Chat federation-matrix integration (ee/packages/federation-matrix/), the createRoom method from rocket.chat/federation-sdk will support a 4-argument signature (userId, roomName, visibility, displayName) in newer versions. Code using this 4-argument call is forward-compatible with planned library updates and should not be flagged as an error.
Applied to files:
apps/meteor/client/views/room/contextualBar/RoomMembers/AddUsers/AddUsers.tsxapps/meteor/app/lib/server/methods/addUsersToRoom.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
PR: RocketChat/Rocket.Chat#37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings (mapping subscription documents to room IDs), never undefined, even when user has no room subscriptions.
Applied to files:
apps/meteor/app/lib/server/methods/addUsersToRoom.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
PR: RocketChat/Rocket.Chat#37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings by mapping subscription documents to room IDs, never undefined, even when user has no room subscriptions.
Applied to files:
apps/meteor/app/lib/server/methods/addUsersToRoom.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
PR: RocketChat/Rocket.Chat#37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: UserBridge.doGetUserRoomIds in packages/apps-engine/src/server/bridges/UserBridge.ts has a bug where it implicitly returns undefined when the app lacks read permission (missing return statement in the else case of the permission check).
Applied to files:
apps/meteor/app/lib/server/methods/addUsersToRoom.ts
🧬 Code graph analysis (1)
apps/meteor/app/lib/server/methods/addUsersToRoom.ts (1)
packages/core-typings/src/IUser.ts (1)
IUser(186-255)
🔇 Additional comments (5)
apps/meteor/app/lib/server/methods/addUsersToRoom.ts (4)
17-17: LGTM! Interface updated to return detailed counts.The return type change from boolean to an object with
added,alreadyInRoom, andtotalcounts enables the client to display accurate, context-aware feedback messages.
30-30: Function signature correctly updated.The return type now matches the updated interface and supports the new feedback mechanism.
107-123: Consider whether ephemeral messages are still needed.The code still broadcasts individual ephemeral messages (lines 116-122) for each user already in the room. With the new client-side toast that displays aggregate counts (e.g., "Some users added, others already in room"), users will see both the ephemeral messages and the summary toast. This may be intentional for detailed feedback, but could also feel redundant.
Consider whether the ephemeral messages should be removed or if the dual-notification approach is the desired UX. You can test the user experience by:
- Attempting to add multiple users where some are already in the room
- Observing both the ephemeral messages and the final toast
- Deciding if this feels redundant or provides valuable detail
127-131: LGTM! Return statement provides accurate counts.The return object correctly reflects the results of the operation, enabling the client to display appropriate feedback.
apps/meteor/client/views/room/contextualBar/RoomMembers/AddUsers/AddUsers.tsx (1)
53-75: Excellent! Context-aware success messages implemented correctly.The message determination logic properly handles all scenarios:
- All users already in the room
- Partial success (some added, some already present)
- Full success (all added)
- Default fallback
The federated room distinction is preserved, and the implementation aligns perfectly with the PR objective to fix misleading success messages.
| message = t('All_users_are_already_in_this_room'); | ||
| } else if (result.added > 0 && result.alreadyInRoom > 0) { | ||
| message = t('Some_users_added_others_already_in_room', { added: result.added, total: result.total }); | ||
| } else if (result.added > 0) { | ||
| message = t(roomIsFederated && !isFederationBlocked ? 'Users_invited' : 'Users_added'); | ||
| } else { | ||
| message = t('Users_added'); |
There was a problem hiding this comment.
🧩 Analysis chain
Verify that translation keys are defined.
The implementation references several translation keys, including what appear to be new ones:
All_users_are_already_in_this_room(line 60)Some_users_added_others_already_in_room(line 62)
Ensure these keys are defined in the i18n translation files for all supported languages.
Run the following script to check if these translation keys exist:
🏁 Script executed:
#!/bin/bash
# Description: Check for translation key definitions
# Search for new translation keys in i18n files
echo "Checking for 'All_users_are_already_in_this_room':"
rg -n "All_users_are_already_in_this_room" --type=json
echo -e "\nChecking for 'Some_users_added_others_already_in_room':"
rg -n "Some_users_added_others_already_in_room" --type=jsonLength of output: 107
🏁 Script executed:
#!/bin/bash
# Description: Comprehensive translation key verification
echo "=== Finding i18n/translation directories ==="
fd -t d "i18n|lang|locale|translation" --type d
echo -e "\n=== Searching for existing key 'Users_added' in all files ==="
rg -n "Users_added" -C2 | head -30
echo -e "\n=== Searching for new key 'All_users_are_already_in_this_room' in all file types ==="
rg -n "All_users_are_already_in_this_room" | head -20
echo -e "\n=== Searching for new key 'Some_users_added_others_already_in_room' in all file types ==="
rg -n "Some_users_added_others_already_in_room" | head -20
echo -e "\n=== Checking file structure of public/locales or similar ==="
fd -t f "\.json$" -g "*locale*" -g "*lang*" -g "*i18n*" | head -20Length of output: 3399
🏁 Script executed:
#!/bin/bash
# Description: Check English locale files for translation keys
echo "=== Checking English locale files ==="
find packages/i18n/src/locales -name "*en*" -type f
echo -e "\n=== Searching for new keys in en.i18n.json ==="
rg -n "All_users_are_already_in_this_room|Some_users_added_others_already_in_room|Users_added" packages/i18n/src/locales/en.i18n.json
echo -e "\n=== Sample of en.i18n.json around 'Users' keys ==="
rg -n "\"Users" packages/i18n/src/locales/en.i18n.json -A1 -B1 | head -40Length of output: 2983
Add missing translation keys to i18n locale files.
The code references two translation keys that are not defined in the i18n files:
All_users_are_already_in_this_room(line 60)Some_users_added_others_already_in_room(line 62)
These keys must be added to all locale files in packages/i18n/src/locales/ (at minimum to en.i18n.json, then propagated to other language files).
🤖 Prompt for AI Agents
In apps/meteor/client/views/room/contextualBar/RoomMembers/AddUsers/AddUsers.tsx
around lines 60 to 66, the code uses two translation keys that are missing from
the i18n locale files; add "All_users_are_already_in_this_room" and
"Some_users_added_others_already_in_room" to
packages/i18n/src/locales/en.i18n.json (with appropriate English strings, e.g.
"All users are already in this room" and "Added {{added}} of {{total}} users;
others were already in the room"), then propagate these keys and translations to
all other locale files under packages/i18n/src/locales/, run the i18n
build/validation step if present, and ensure the interpolation placeholders
match the code (added, total).
1 participant
This PR fixes misleading success messages shown when attempting to add users who are already members of a room. Previously, the system would always display "The users have been added" even when no users were actually added, creating user confusion.
Proposed changes (including videos or screenshots)
This is how it looks like after changes.
Issue(s)
Closes #24701
Steps to test or reproduce
Further comments
Summary by CodeRabbit
New Features
Bug Fixes