fix(omnichannel): prevent duplicate agent addition with client-side validation#38702
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 |
🦋 Changeset detectedLatest commit: f44e45c The changes in this PR will be included in the next version bump. This PR includes changesets to release 41 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThis pull request implements client-side validation to prevent duplicate agent addition in the Omnichannel agents interface. The AddAgent component now accepts an agents list prop and validates whether a username already exists before attempting to save, displaying an error toast if detected. Changes
Poem
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![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
🤖 Fix all issues with AI agents
In `@apps/meteor/client/views/omnichannel/agents/AgentsTable/AddAgent.tsx`:
- Around line 30-32: The client-side duplicate check in AddAgent.tsx
(isAlreadyAgent comparing against agents/data?.users) only inspects the current
paginated page and can miss existing agents; update AddAgent to perform a
lightweight server-side existence check before calling the addAgent POST: call a
new or existing GET endpoint (or RPC) that verifies whether the username already
has the livechat-agent role (the same logic used by the server addAgent that
uses $addToSet) and block the POST + show an appropriate error/toast when that
check returns true; ensure you reference the username variable and replace/guard
the isAlreadyAgent logic with the API call result so the client never reports a
misleading success for duplicates.
In `@apps/meteor/client/views/omnichannel/agents/AgentsTable/AgentsTable.tsx`:
- Line 75: AgentsTable currently passes only the paginated set (data?.users)
into AddAgent, so AddAgent's duplicate check is incomplete; change AgentsTable
to provide the full user set (not just the current page) to AddAgent — e.g., run
or reuse a query that fetches all users (call it usersAll or similar) and pass
that array as the agents prop, or alternatively change AddAgent to perform a
server-side duplicate check (expose a validation API and call it from AddAgent).
Update the prop passed to AddAgent (agents) and ensure AddAgent.tsx uses that
full list (or the validation API) for deduplication.
🧹 Nitpick comments (2)
apps/meteor/client/views/omnichannel/agents/AgentsTable/AddAgent.tsx (2)
12-12: Props type could be narrowed withreadonly.Minor: consider marking the
agentsprop asreadonlyto signal immutability intent.
28-40: The early return skips save but doesn't clear the input field.When a duplicate is detected, the username input retains the selected value, so the user would need to manually clear it. This is arguably fine (the user sees the error toast and can pick a different user), but consider whether clearing the field or keeping the button disabled for a known duplicate would be a better UX.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.changeset/seven-pandas-explain.mdapps/meteor/client/views/omnichannel/agents/AgentsTable/AddAgent.tsxapps/meteor/client/views/omnichannel/agents/AgentsTable/AgentsTable.tsxpackages/i18n/src/locales/en.i18n.json
🧰 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/client/views/omnichannel/agents/AgentsTable/AgentsTable.tsxapps/meteor/client/views/omnichannel/agents/AgentsTable/AddAgent.tsx
🧠 Learnings (2)
📚 Learning: 2025-11-19T12:32:29.696Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37547
File: packages/i18n/src/locales/en.i18n.json:634-634
Timestamp: 2025-11-19T12:32:29.696Z
Learning: Repo: RocketChat/Rocket.Chat
Context: i18n workflow
Learning: In this repository, new translation keys should be added to packages/i18n/src/locales/en.i18n.json only; other locale files are populated via the external translation pipeline and/or fall back to English. Do not request adding the same key to all locale files in future reviews.
Applied to files:
packages/i18n/src/locales/en.i18n.json.changeset/seven-pandas-explain.md
📚 Learning: 2025-11-04T16:49:19.107Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37377
File: apps/meteor/ee/server/hooks/federation/index.ts:86-88
Timestamp: 2025-11-04T16:49:19.107Z
Learning: In Rocket.Chat's federation system (apps/meteor/ee/server/hooks/federation/), permission checks follow two distinct patterns: (1) User-initiated federation actions (creating rooms, adding users to federated rooms, joining from invites) should throw MeteorError to inform users they lack 'access-federation' permission. (2) Remote server-initiated federation events should silently skip/ignore when users lack permission. The beforeAddUserToRoom hook only executes for local user-initiated actions, so throwing an error there is correct. Remote federation events are handled separately by the federation Matrix package with silent skipping logic.
Applied to files:
.changeset/seven-pandas-explain.md
⏰ 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: cubic · AI code reviewer
🔇 Additional comments (2)
.changeset/seven-pandas-explain.md (1)
1-6: Changeset looks good.Patch bumps and summary align with the stated fix.
packages/i18n/src/locales/en.i18n.json (1)
473-473: LGTM: i18n key addition is clear and consistent.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| const isAlreadyAgent = agents.some( | ||
| (agent) => agent.username?.toLowerCase() === username.toLowerCase() | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "AddAgent.tsx" "AgentsTable.tsx" -x cat -n {}Repository: RocketChat/Rocket.Chat
Length of output: 168
🏁 Script executed:
rg -n "useAgentsQuery" --type=ts --type=tsx -B 3 -A 10 | head -100Repository: RocketChat/Rocket.Chat
Length of output: 93
🏁 Script executed:
fd -t f -path "*/omnichannel/agents/*" | head -20Repository: RocketChat/Rocket.Chat
Length of output: 234
🏁 Script executed:
fd "AddAgent\.tsx|AgentsTable\.tsx"Repository: RocketChat/Rocket.Chat
Length of output: 459
🏁 Script executed:
rg "useAgentsQuery" -g "*.ts" -g "*.tsx" --max-count=20Repository: RocketChat/Rocket.Chat
Length of output: 464
🏁 Script executed:
cat -n apps/meteor/client/views/omnichannel/agents/AgentsTable/AddAgent.tsxRepository: RocketChat/Rocket.Chat
Length of output: 2515
🏁 Script executed:
cat -n apps/meteor/client/views/omnichannel/agents/AgentsTable/AgentsTable.tsxRepository: RocketChat/Rocket.Chat
Length of output: 5164
🏁 Script executed:
cat -n apps/meteor/client/views/omnichannel/agents/hooks/useAgentsQuery.tsRepository: RocketChat/Rocket.Chat
Length of output: 648
🏁 Script executed:
rg "livechat/users/agent" -g "*.ts" -A 20 | grep -A 20 "POST.*agent" | head -60Repository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
fd "useAgentsQuery" -x dirname {} | head -1 | xargs -I {} find {} -name "*.ts" -o -name "*.tsx" | xargs rg "getAgents\|POST.*agent" -lRepository: RocketChat/Rocket.Chat
Length of output: 50
🏁 Script executed:
rg "POST.*livechat/users/agent" -g "*.ts" -A 30Repository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
rg "POST.*v1/livechat/users/agent" -g "*.ts" -g "*.tsx"Repository: RocketChat/Rocket.Chat
Length of output: 229
🏁 Script executed:
rg "/v1/livechat/users/agent" -g "*.ts" -A 5 -B 5 | head -80Repository: RocketChat/Rocket.Chat
Length of output: 6904
🏁 Script executed:
find . -path "*/api/*" -name "*.ts" | xargs rg "livechat/users/agent" 2>/dev/null | head -20Repository: RocketChat/Rocket.Chat
Length of output: 1830
🏁 Script executed:
rg "LivechatUsersManagerPOSTProps" -g "*.ts" -A 10Repository: RocketChat/Rocket.Chat
Length of output: 2390
🏁 Script executed:
find . -path "*/api/*livechat*" -name "*.ts" | xargs rg "users/agent" -l 2>/dev/null | head -5Repository: RocketChat/Rocket.Chat
Length of output: 122
🏁 Script executed:
rg "POST.*users.*agent" -g "*.ts" -B 5 -A 20 | head -100Repository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
fd "01-agents.ts" -x cat -n {} | head -150Repository: RocketChat/Rocket.Chat
Length of output: 6711
🏁 Script executed:
rg "addAgent\|createAgent\|POST.*agent" --type=ts -g "*/api/*" -A 20 | head -100Repository: RocketChat/Rocket.Chat
Length of output: 214
🏁 Script executed:
find . -path "*/livechat*" -name "*.ts" | xargs rg "users/agent.*POST\|addAgent" 2>/dev/null | head -20Repository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
cat -n apps/meteor/tests/end-to-end/api/livechat/01-agents.ts | sed -n '150,300p'Repository: RocketChat/Rocket.Chat
Length of output: 5913
🏁 Script executed:
rg "users/agent" -g "*.ts" --path-separator=/ -B 3 -A 15 | grep -A 15 "POST" | head -80Repository: RocketChat/Rocket.Chat
Length of output: 2131
🏁 Script executed:
find . -type f -name "*.ts" -path "*/api/*livechat*" | xargs grep -l "POST.*agent\|addAgent" 2>/dev/null | head -3Repository: RocketChat/Rocket.Chat
Length of output: 127
🏁 Script executed:
find . -path "*/api/*" -name "*.ts" | xargs grep -l "livechat/users/agent.*POST\|addAgent" 2>/dev/null | head -5Repository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
rg "livechat/users/agent" -g "*.ts" --path-separator=/ | grep -E "(controller|handler|endpoint)" | head -10Repository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
find . -type d -name "livechat" | head -10Repository: RocketChat/Rocket.Chat
Length of output: 404
🏁 Script executed:
find ./apps/meteor/app/livechat -type f -name "*.ts" | xargs grep -l "users/agent" 2>/dev/nullRepository: RocketChat/Rocket.Chat
Length of output: 50
🏁 Script executed:
find ./apps/meteor/app/livechat -type f -name "*.ts" -o -name "*.js" | head -20Repository: RocketChat/Rocket.Chat
Length of output: 1291
🏁 Script executed:
rg "POST.*users.*agent|users.*agent.*POST" ./apps/meteor/app/livechat -A 30 | head -150Repository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
cat -n ./apps/meteor/app/livechat/imports/server/rest/users.tsRepository: RocketChat/Rocket.Chat
Length of output: 4387
🏁 Script executed:
cat -n ./apps/meteor/app/livechat/imports/server/rest/agent.tsRepository: RocketChat/Rocket.Chat
Length of output: 878
🏁 Script executed:
rg "addAgent" -g "*.ts" --path-separator=/ -B 3 -A 20 | grep -A 25 "export.*addAgent\|function addAgent" | head -100Repository: RocketChat/Rocket.Chat
Length of output: 4285
🏁 Script executed:
find ./apps/meteor/app/livechat -type f -name "*.ts" | xargs grep -l "addAgent\|removeAgent" | head -10Repository: RocketChat/Rocket.Chat
Length of output: 470
🏁 Script executed:
cat -n ./apps/meteor/app/livechat/server/lib/omni-users.ts | head -150Repository: RocketChat/Rocket.Chat
Length of output: 4228
🏁 Script executed:
rg "addUserRolesAsync" -g "*.ts" -B 3 -A 15 | head -100Repository: RocketChat/Rocket.Chat
Length of output: 7279
🏁 Script executed:
rg "livechat-agent" -g "*.ts" -B 2 -A 5 | grep -A 5 "addUserRoles\|hasRole" | head -80Repository: RocketChat/Rocket.Chat
Length of output: 1543
🏁 Script executed:
cat -n ./apps/meteor/server/lib/roles/addUserRoles.tsRepository: RocketChat/Rocket.Chat
Length of output: 2194
🏁 Script executed:
rg "livechat-agent" -g "*.ts" | grep -E "(hasRole|includes.*livechat-agent)" | head -20Repository: RocketChat/Rocket.Chat
Length of output: 1582
🏁 Script executed:
rg "findOneByUsername.*livechat-agent\|already.*agent\|duplicate.*agent" -g "*.ts" -iRepository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
rg "afterAgentAdded" -g "*.ts" -B 2 -A 15 | head -80Repository: RocketChat/Rocket.Chat
Length of output: 6531
🏁 Script executed:
rg "Users.addRolesByUserId" -g "*.ts" -B 3 -A 10 | head -100Repository: RocketChat/Rocket.Chat
Length of output: 1481
🏁 Script executed:
rg "already.*agent\|agent.*exists\|duplicate.*agent" -g "*.ts" -iRepository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
cat -n ./apps/meteor/app/livechat/server/lib/hooks.ts | head -100Repository: RocketChat/Rocket.Chat
Length of output: 3962
🏁 Script executed:
cat -n packages/models/src/models/Users.ts | grep -A 15 "addRolesByUserId"Repository: RocketChat/Rocket.Chat
Length of output: 592
Duplicate check only validates against the current page of agents.
agents is sourced from data?.users in AgentsTable.tsx, which is a paginated query result. If the user to be added exists as an agent but isn't on the currently displayed page (or is filtered out by the search text), this check will miss the duplicate and the API call will proceed. Additionally, the server-side addAgent function uses MongoDB's $addToSet operator when assigning the livechat-agent role, which silently succeeds even if the user already has that role, resulting in a misleading success toast for duplicate additions.
Consider either:
- Making a lightweight API call (e.g., a GET to check if the user is already an agent) before the POST, or
- Fetching the full agent list for this check (though this may not scale).
Option 1 is more robust and aligns with the PR's goal of preventing the false-positive success toast.
🤖 Prompt for AI Agents
In `@apps/meteor/client/views/omnichannel/agents/AgentsTable/AddAgent.tsx` around
lines 30 - 32, The client-side duplicate check in AddAgent.tsx (isAlreadyAgent
comparing against agents/data?.users) only inspects the current paginated page
and can miss existing agents; update AddAgent to perform a lightweight
server-side existence check before calling the addAgent POST: call a new or
existing GET endpoint (or RPC) that verifies whether the username already has
the livechat-agent role (the same logic used by the server addAgent that uses
$addToSet) and block the POST + show an appropriate error/toast when that check
returns true; ensure you reference the username variable and replace/guard the
isAlreadyAgent logic with the API call result so the client never reports a
misleading success for duplicates.
| return ( | ||
| <> | ||
| <AddAgent /> | ||
| <AddAgent agents={data?.users ?? []}/> |
There was a problem hiding this comment.
data?.users is paginated — this feeds into the incomplete duplicate check in AddAgent.
As noted in AddAgent.tsx, this passes only the current page of agents. When the query changes (filter text, page, sort), the agents prop updates, but it never represents the full set of agents. This is the root of the incomplete validation discussed in the other file.
🤖 Prompt for AI Agents
In `@apps/meteor/client/views/omnichannel/agents/AgentsTable/AgentsTable.tsx` at
line 75, AgentsTable currently passes only the paginated set (data?.users) into
AddAgent, so AddAgent's duplicate check is incomplete; change AgentsTable to
provide the full user set (not just the current page) to AddAgent — e.g., run or
reuse a query that fetches all users (call it usersAll or similar) and pass that
array as the agents prop, or alternatively change AddAgent to perform a
server-side duplicate check (expose a validation API and call it from AddAgent).
Update the prop passed to AddAgent (agents) and ensure AddAgent.tsx uses that
full list (or the validation API) for deduplication.
There was a problem hiding this comment.
Thank you for the detailed analysis. You're right that data?.users is paginated and only represents the current
page — this is a valid concern.
I will address this by making a lightweight GET API call to check if the user is already an agent before making
the POST call, as suggested in Option 1, once the maintainers provide their feedback on the current approach.
There was a problem hiding this comment.
@Naetiksoni08, that sounds like a solid plan! Using a GET API call to validate whether the user is already an agent before attempting the POST is the right approach to ensure complete duplicate detection across all agents (not just the current page).
Waiting for maintainer feedback before implementing the additional change is also a good call — it ensures alignment on the approach and avoids unnecessary rework.
If you need any assistance when you're ready to implement the API-based validation (e.g., identifying the right endpoint, structuring the call, or handling edge cases), feel free to reach out. Good luck with the PR! 🚀
KevLehman
added
the
valid
|
There's already an open pr for fixing this, pls check before creating new prs. thx |
2 participants
Proposed changes (including videos or screenshots)
When adding an agent in Omnichannel > Agents, selecting a user who is already an agent and clicking "Add agent" would incorrectly make an API call and show a success toast "Agent added".
This fix adds client-side validation in
AddAgent.tsxto check if the selected username already exists in the current agents list before making the API call.Key fix — Case-insensitive comparison:
The
UserAutoCompletecomponent returns display names with mixed case (e.g. "Rocket.Cat") while the database stores lowercase usernames (e.g. "rocket.cat"). A simple strict equality check (===) would fail in this case and allow duplicate additions.This fix uses
toLowerCase()on both sides to handle this:If the user is already an agent, the API call is blocked and an error toast is shown using the i18n key
Agent_already_exists,added in alphabetical order alongside existing Agent keys.
Before Fix:
After Fix:
Issue(s)
Closes #38684
Steps to test or reproduce
Expected: Error toast "User is already an agent" appears, no API call is made.
Actual (before fix): Success toast "Agent added" appears and API call is made even though user is already an agent.
Further comments
Note: I am the original reporter of issue #38684.
This fix specifically addresses the case-sensitivity edge case where
UserAutoCompletereturns mixed case display names(e.g "Rocket.Cat") while the database stores lowercase usernames (e.g "rocket.cat"). Without
toLowerCase(), the duplicate check silently fails and the bug persists.The i18n key
Agent_already_existshas been added in correct alphabetical order inpackages/i18n/src/locales/en.i18n.json, consistent with existing Agent keys convention.Summary by CodeRabbit