fix: Prevent adding duplicate omnichannel agents with client-side validation#38687
fix: Prevent adding duplicate omnichannel agents with client-side validation#38687Dnyanesh-29 wants to merge 1 commit intoRocketChat:developfrom
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: b4e118e The changes in this PR will be included in the next version bump. This PR includes changesets to release 40 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 |
|
No actionable comments were generated in the recent review. 🎉 📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
🧰 Additional context used📓 Path-based instructions (1)**/*.{ts,tsx,js}📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
Files:
🔇 Additional comments (1)
WalkthroughClient-side duplicate-check added: AddAgent now receives the current agents list, validates the username before calling the save mutation, shows an error toast and aborts if duplicate; otherwise proceeds with the existing save and success handling. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant AgentsTable as AgentsTable
participant AddAgent as AddAgent
participant Validation as Validation
participant API as API
participant QueryCache as QueryCache
participant Toast as Toast
User->>AgentsTable: open Add Agent UI
AgentsTable->>AddAgent: render (agents list -> prop)
User->>AddAgent: enter username & click Add
AddAgent->>Validation: check username in agents list
alt username exists
Validation-->>AddAgent: duplicate found
AddAgent->>Toast: show "User_is_already_an_agent" (error)
else username new
Validation-->>AddAgent: not found
AddAgent->>API: saveAction (POST /v1/livechat/users/agent)
API-->>AddAgent: success
AddAgent->>QueryCache: invalidate agents query
AddAgent->>Toast: show success toast
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (2 warnings)
✅ 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 |
![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 28-37: The duplicate-agent check in handleSave (which reads agents
from the paginated agents variable) only compares against the current page;
change the flow to call a server-side existence check (e.g., a new
/v1/livechat/users/agent/:username endpoint) or fetch an unpaginated list of
agent usernames before validating to ensure full-scope detection, and update
handleSave to use that result instead of agents?.some(...); also add an error
handler to the saveAction mutation invocation to surface API failures via
dispatchToastMessage (type: 'error') and add the missing translation key
'User_is_already_an_agent' to i18n so the error message resolves correctly.
In `@apps/meteor/client/views/omnichannel/agents/AgentsTable/AgentsTable.tsx`:
- Line 75: The AddAgent child receives only the paginated subset data?.users
which misses agents on other pages and is undefined while loading, so modify
AgentsTable to provide AddAgent with a complete, reliable source for duplicate
checks: either fetch all agents (e.g., call a new fetchAllUsers or use the
existing non-paginated users query) and pass that array (e.g., allUsers) to the
AddAgent component, or at minimum pass a stable empty array plus the isLoading
flag (agents={allUsers ?? []} isLoading={isLoading}) so AddAgent can defer
checking until data is ready; update the call site (<AddAgent ...>) and wiring
so AddAgent uses the fullUsers/allUsers prop (or the isLoading prop) for
duplicate detection.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.changeset/chilly-wasps-love.mdapps/meteor/client/views/omnichannel/agents/AgentsTable/AddAgent.tsxapps/meteor/client/views/omnichannel/agents/AgentsTable/AgentsTable.tsx
🧰 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/AddAgent.tsxapps/meteor/client/views/omnichannel/agents/AgentsTable/AgentsTable.tsx
⏰ 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/chilly-wasps-love.md (1)
1-5: LGTM!Changeset is properly formatted with the correct package name and patch level.
apps/meteor/client/views/omnichannel/agents/AgentsTable/AddAgent.tsx (1)
34-34:⚠️ Potential issue | 🟠 MajorAdd
User_is_already_an_agenttranslation key to i18n files.The key is used in the code but is missing from all translation files in
packages/i18n/src/locales/. Add the translation to at leasten.i18n.jsonand other locale files, otherwise users will see the raw key string.⛔ Skipped due to learnings
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.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
KevLehman
added
the
valid
Dnyanesh-29
force-pushed
the
fix/omnichannel-duplicate-agents-v2
branch
from
f5b8cc0 to
e64a7c4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/meteor/client/views/omnichannel/agents/AgentsTable/AddAgent.tsx`:
- Around line 28-39: The handleSave async event in AddAgent.tsx redundantly
calls setUsername('') after await saveAction({ username }) while the
saveAction's onSuccess callback already clears the username; remove the
duplicate by deleting the setUsername('') inside handleSave (the function
created with useEffectEvent) so only the onSuccess handler performs success-side
effects, or alternatively move all success effects into handleSave and remove
the onSuccess callback—ensure you update references to handleSave, saveAction,
and the onSuccess callback accordingly.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.changeset/chilly-wasps-love.mdapps/meteor/client/views/omnichannel/agents/AgentsTable/AddAgent.tsxapps/meteor/client/views/omnichannel/agents/AgentsTable/AgentsTable.tsxpackages/i18n/src/locales/en.i18n.json
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/meteor/client/views/omnichannel/agents/AgentsTable/AgentsTable.tsx
🧰 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/AddAgent.tsx
🧠 Learnings (1)
📚 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
🔇 Additional comments (3)
.changeset/chilly-wasps-love.md (1)
1-5: LGTM! Changeset is properly formatted.The changeset follows the correct format, specifies an appropriate patch version bump for a bug fix, and provides a clear description of the change.
packages/i18n/src/locales/en.i18n.json (1)
40-40: LGTM — clear, user-facing string for duplicate-agent error.apps/meteor/client/views/omnichannel/agents/AgentsTable/AddAgent.tsx (1)
37-39: The code already has protection against raw error objects rendering as[object Object]throughToastMessagesProvider. WhendispatchToastMessage({ type: 'error', message: error })is called, the provider'ssubscribeToToastMessagescallback automatically extracts the error message usinggetErrorMessage()for error objects and handlesErrorinstances explicitly before falling back toString(error).However, removing the
anytype annotation and extractingerror.messagedefensively is still reasonable practice for clarity:Suggested fix
- } catch (error: any) { - dispatchToastMessage({ type: 'error', message: error }); + } catch (error) { + dispatchToastMessage({ type: 'error', message: error instanceof Error ? error.message : String(error) }); }
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
Hi, I'm the original reporter of issue #38684. I've addressed this along with the correct i18n key placement in my PR #38702. |
Dnyanesh-29
force-pushed
the
fix/omnichannel-duplicate-agents-v2
branch
from
e64a7c4 to
450db2f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/meteor/client/views/omnichannel/agents/AgentsTable/AddAgent.tsx`:
- Around line 28-33: The duplicate-agent check in the handleSave (created via
useEffectEvent) uses strict equality and is case-sensitive, so normalize both
sides for a case-insensitive comparison: when checking agents?.some((agent) =>
agent.username && agent.username === username) convert agent.username and
username to the same case (e.g., .toLowerCase()) and guard null/undefined
usernames to avoid errors; update the isAlreadyAgent computation accordingly so
mixed-case display names from UserAutoComplete match stored lowercase usernames.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.changeset/chilly-wasps-love.mdapps/meteor/client/views/omnichannel/agents/AgentsTable/AddAgent.tsxapps/meteor/client/views/omnichannel/agents/AgentsTable/AgentsTable.tsxpackages/i18n/src/locales/en.i18n.json
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/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 (1)
📚 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/client/views/omnichannel/agents/AgentsTable/AddAgent.tsx
🧬 Code graph analysis (1)
apps/meteor/client/views/omnichannel/agents/AgentsTable/AddAgent.tsx (1)
packages/models/src/models/Users.ts (1)
setUsername(2835-2839)
🔇 Additional comments (2)
.changeset/chilly-wasps-love.md (1)
1-5: LGTM!Changeset is correctly configured as a patch bump for
@rocket.chat/meteor.apps/meteor/client/views/omnichannel/agents/AgentsTable/AddAgent.tsx (1)
37-38: The code is correct as-is. TheToastMessagesProviderimplementation explicitly handles error objects passed todispatchToastMessageby callinggetErrorMessage()(line 51-52 in ToastMessagesProvider.tsx), which gracefully extracts the message string from error properties likereason,error, ormessage. The error object will not render as[object Object]because the framework's error handling is designed to accept error objects for the error type (as shown in ToastMessagePayload's type definition where error messages acceptunknown). No changes needed.Likely an incorrect or invalid review comment.
Dnyanesh-29
force-pushed
the
fix/omnichannel-duplicate-agents-v2
branch
from
450db2f to
b4e118e
Compare
3 participants
Summary
Currently, attempting to add a user as an Omnichannel agent when they are already an agent results in a misleading "Agent added" success toast, even though no change occurred on the server. This PR introduces client-side validation to check the existing agents list before the API call is made.
Changes
AgentsTable.tsx: Now passes the agents data list to the AddAgent component.
AddAgent.tsx:
Added a check using .some() to verify if the selected username already exists in the current agents list.
Prevents the API request if a duplicate is found.
Displays an error toast: "User is already an agent."
Changeset: Added a patch changeset for @rocket.chat/meteor.
Fixes
Closes #38684 ---
How to test
Navigate to Omnichannel > Agents.
Add a user as an agent.
Immediately try to add that same user again.
Expected Result: An error toast should appear, and no network request should be sent to the server.
Summary by CodeRabbit
Bug Fixes
Localization