refactor: standardize ForwardChatModal using GenericModal
#38424
+433
−147
Conversation
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
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
WalkthroughRefactored ForwardChatModal to handle chat forward flow internally via API request instead of a callback prop, replacing Modal with GenericModal and switching form control to react-hook-form. Updated QuickActions hook, added Storybook stories and Jest tests, and introduced E2E test coverage for error scenarios. Changes
Sequence DiagramsequenceDiagram
actor User
participant Modal as ForwardChatModal
participant Form as react-hook-form
participant API as POST /room.forward
participant Navigation as Router
participant Notification as Toast
User->>Modal: Select department/user & submit
Modal->>Form: Collect form values (departmentId/userId)
Form->>Modal: Validate & assemble payload
Modal->>API: Send forward request
alt Success
API-->>Modal: 200 OK
Modal->>Navigation: Navigate away from room
Modal->>Notification: Show success message
Note over Modal: Close modal
else Failure
API-->>Modal: Error response
Modal->>Notification: Display error toast
Note over Modal: Keep modal open, preserve state
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Contributor
|
Hi @dougfabris Great work on this PR! I noticed there's some overlap with my PR #38384 where I'm also standardizing Omnichannel modals to use My PR covers:
Since your implementation for Would you mind taking a quick look at #38384 when you have a moment? Your feedback would be really valuable! 🙏 Thanks! |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #38424 +/- ##
===========================================
+ Coverage 70.41% 70.45% +0.03%
===========================================
Files 3161 3162 +1
Lines 110151 110339 +188
Branches 19861 19853 -8
===========================================
+ Hits 77561 77735 +174
- Misses 30559 30575 +16
+ Partials 2031 2029 -2
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Contributor
📦 Docker Image Size Report➡️ Changes
📊 Historical Trend---
config:
theme: "dark"
xyChart:
width: 900
height: 400
---
xychart
title "Image Size Evolution by Service (Last 30 Days + This PR)"
x-axis ["11/18 22:53", "11/19 23:02", "11/21 16:49", "11/24 17:34", "11/27 22:32", "11/28 19:05", "12/01 23:01", "12/02 21:57", "12/03 21:00", "12/04 18:17", "12/05 21:56", "12/08 20:15", "12/09 22:17", "12/10 23:26", "12/11 21:56", "12/12 22:45", "12/13 01:34", "12/15 22:31", "12/16 22:18", "12/17 21:04", "12/18 23:12", "12/19 23:27", "12/20 21:03", "12/22 18:54", "12/23 16:16", "12/24 19:38", "12/25 17:51", "12/26 13:18", "12/29 19:01", "12/30 20:52", "01/30 20:31 (PR)"]
y-axis "Size (GB)" 0 --> 0.5
line "account-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.00]
line "authorization-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.00]
line "ddp-streamer-service" [0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.00]
line "omnichannel-transcript-service" [0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.00]
line "presence-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.00]
line "queue-worker-service" [0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.00]
line "rocketchat" [0.35, 0.35, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.00]
Statistics (last 30 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Fri, 30 Jan 2026 20:31:34 GMT |
dougfabris
force-pushed
the
refactor/forward-modal
branch
from
20f752d to
2fadb5b
Compare
MartinSchoeler
force-pushed
the
refactor/forward-modal
branch
from
e8c2d60 to
971618d
Compare
![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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/meteor/client/views/omnichannel/modals/ForwardChatModal.tsx`:
- Around line 53-95: The current handleForwardChat performs getUserData before
the try/catch, so failures escape the error handler and onCancel; move the
username lookup into the try block (or wrap the entire function body in
try/catch) so any error from getUserData is caught, call dispatchToastMessage({
type: 'error', message: ... }) with the error and always execute onCancel in
finally, preserving the current payload assembly logic (room._id, departmentId,
userId, comment, clientAction) and keeping forwardChat, router.navigate,
LegacyRoomManager.close and onCancel behavior intact.
🧹 Nitpick comments (2)
apps/meteor/tests/e2e/page-objects/omnichannel/omnichannel-contact-center/omnichannel-contact-center-chats.ts (1)
150-155: Wait for the filtered row before clicking to reduce flakiness.
fill()can debounce table filtering; clicking immediately risks a stale row. Waiting for the row makes the page-object more stable.Suggested update
async openChat(name: string) { await this.inputSearch.fill(name); - await this.table.findRowByName(name).click(); + const row = this.table.findRowByName(name); + await row.waitFor(); + await row.click(); await this.conversation.openChat(); await this.page.locator('#main-content').waitFor(); }apps/meteor/tests/e2e/omnichannel/omnichannel-chat-transfers.spec.ts (1)
129-153: Ensure the temporary department is always deleted (even on failure).If a step fails,
emptyDepartment.delete()won’t run, leaving test data behind.Suggested update
test(`OC - Chat transfers [Monitor role] - Transfer to department with no online agents should fail`, async ({ api }) => { const [roomA] = conversations.map(({ data }) => data.room); const [, agentB] = sessions; const emptyDepartment = await createDepartment(api); - - await test.step('expect to open forward chat modal', async () => { - await poOmnichannel.chats.openChat(roomA.fname); - await poOmnichannel.quickActionsRoomToolbar.forwardChat(); - }); - - await test.step('expect transfer to department with no online agents to fail', async () => { - await poOmnichannel.content.forwardChatModal.selectDepartment(emptyDepartment.data.name); - await poOmnichannel.content.forwardChatModal.inputComment.type('transfer_attempt'); - await expect(poOmnichannel.content.forwardChatModal.btnForward).toBeEnabled(); - await poOmnichannel.content.forwardChatModal.btnForward.click(); - await poOmnichannel.toastMessage.waitForDisplay({ type: 'error' }); - }); - - await test.step('expect conversation to remain with original agent', async () => { - await expect(agentB.poHomeOmnichannel.sidebar.getSidebarItemByName(roomA.fname)).not.toBeVisible(); - }); - - await emptyDepartment.delete(); + try { + await test.step('expect to open forward chat modal', async () => { + await poOmnichannel.chats.openChat(roomA.fname); + await poOmnichannel.quickActionsRoomToolbar.forwardChat(); + }); + + await test.step('expect transfer to department with no online agents to fail', async () => { + await poOmnichannel.content.forwardChatModal.selectDepartment(emptyDepartment.data.name); + await poOmnichannel.content.forwardChatModal.inputComment.type('transfer_attempt'); + await expect(poOmnichannel.content.forwardChatModal.btnForward).toBeEnabled(); + await poOmnichannel.content.forwardChatModal.btnForward.click(); + await poOmnichannel.toastMessage.waitForDisplay({ type: 'error' }); + }); + + await test.step('expect conversation to remain with original agent', async () => { + await expect(agentB.poHomeOmnichannel.sidebar.getSidebarItemByName(roomA.fname)).not.toBeVisible(); + }); + } finally { + await emptyDepartment.delete(); + } });As per coding guidelines: Ensure clean state for each test execution in Playwright tests.
Comment on lines
53
to
95
| const handleForwardChat = useCallback( | ||
| async ({ department: departmentId, username, comment }: ForwardChatModalFormData) => { | ||
| let uid; | ||
| let userId; | ||
|
|
||
| if (username) { | ||
| const { user } = await getUserData({ username }); | ||
| uid = user?._id; | ||
| userId = user?._id; | ||
| } | ||
|
|
||
| await onForward(departmentId, uid, comment); | ||
| if (departmentId && userId) { | ||
| return; | ||
| } | ||
|
|
||
| const payload: { | ||
| roomId: string; | ||
| departmentId?: string; | ||
| userId?: string; | ||
| comment?: string; | ||
| clientAction: boolean; | ||
| } = { | ||
| roomId: room._id, | ||
| comment, | ||
| clientAction: true, | ||
| }; | ||
|
|
||
| if (departmentId) { | ||
| payload.departmentId = departmentId; | ||
| } | ||
|
|
||
| if (userId) { | ||
| payload.userId = userId; | ||
| } | ||
|
|
||
| try { | ||
| await forwardChat(payload); | ||
| dispatchToastMessage({ type: 'success', message: t('Transferred') }); | ||
| router.navigate('/home'); | ||
| LegacyRoomManager.close(room.t + room._id); | ||
| } catch (error) { | ||
| dispatchToastMessage({ type: 'error', message: error }); | ||
| } finally { | ||
| onCancel(); | ||
| } |
Contributor
There was a problem hiding this comment.
Handle user-lookup failures inside the error boundary.
getUserData runs before the try/catch, so lookup failures can bypass the toast and onCancel, leaving the modal open with no clear feedback. Wrap the entire flow so any failure is surfaced consistently.
🔧 Suggested fix
const handleForwardChat = useCallback(
async ({ department: departmentId, username, comment }: ForwardChatModalFormData) => {
- let userId;
-
- if (username) {
- const { user } = await getUserData({ username });
- userId = user?._id;
- }
-
- if (departmentId && userId) {
- return;
- }
-
- const payload: {
- roomId: string;
- departmentId?: string;
- userId?: string;
- comment?: string;
- clientAction: boolean;
- } = {
- roomId: room._id,
- comment,
- clientAction: true,
- };
-
- if (departmentId) {
- payload.departmentId = departmentId;
- }
-
- if (userId) {
- payload.userId = userId;
- }
-
- try {
- await forwardChat(payload);
- dispatchToastMessage({ type: 'success', message: t('Transferred') });
- router.navigate('/home');
- LegacyRoomManager.close(room.t + room._id);
- } catch (error) {
- dispatchToastMessage({ type: 'error', message: error });
- } finally {
- onCancel();
- }
+ try {
+ let userId;
+
+ if (username) {
+ const { user } = await getUserData({ username });
+ userId = user?._id;
+ }
+
+ if (departmentId && userId) {
+ return;
+ }
+
+ const payload: {
+ roomId: string;
+ departmentId?: string;
+ userId?: string;
+ comment?: string;
+ clientAction: boolean;
+ } = {
+ roomId: room._id,
+ comment,
+ clientAction: true,
+ };
+
+ if (departmentId) {
+ payload.departmentId = departmentId;
+ }
+
+ if (userId) {
+ payload.userId = userId;
+ }
+
+ await forwardChat(payload);
+ dispatchToastMessage({ type: 'success', message: t('Transferred') });
+ router.navigate('/home');
+ LegacyRoomManager.close(room.t + room._id);
+ } catch (error) {
+ dispatchToastMessage({ type: 'error', message: error });
+ } finally {
+ onCancel();
+ }
},
[room._id, room.t, getUserData, forwardChat, dispatchToastMessage, t, router, onCancel],
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const handleForwardChat = useCallback( | |
| async ({ department: departmentId, username, comment }: ForwardChatModalFormData) => { | |
| let uid; | |
| let userId; | |
| if (username) { | |
| const { user } = await getUserData({ username }); | |
| uid = user?._id; | |
| userId = user?._id; | |
| } | |
| await onForward(departmentId, uid, comment); | |
| if (departmentId && userId) { | |
| return; | |
| } | |
| const payload: { | |
| roomId: string; | |
| departmentId?: string; | |
| userId?: string; | |
| comment?: string; | |
| clientAction: boolean; | |
| } = { | |
| roomId: room._id, | |
| comment, | |
| clientAction: true, | |
| }; | |
| if (departmentId) { | |
| payload.departmentId = departmentId; | |
| } | |
| if (userId) { | |
| payload.userId = userId; | |
| } | |
| try { | |
| await forwardChat(payload); | |
| dispatchToastMessage({ type: 'success', message: t('Transferred') }); | |
| router.navigate('/home'); | |
| LegacyRoomManager.close(room.t + room._id); | |
| } catch (error) { | |
| dispatchToastMessage({ type: 'error', message: error }); | |
| } finally { | |
| onCancel(); | |
| } | |
| const handleForwardChat = useCallback( | |
| async ({ department: departmentId, username, comment }: ForwardChatModalFormData) => { | |
| try { | |
| let userId; | |
| if (username) { | |
| const { user } = await getUserData({ username }); | |
| userId = user?._id; | |
| } | |
| if (departmentId && userId) { | |
| return; | |
| } | |
| const payload: { | |
| roomId: string; | |
| departmentId?: string; | |
| userId?: string; | |
| comment?: string; | |
| clientAction: boolean; | |
| } = { | |
| roomId: room._id, | |
| comment, | |
| clientAction: true, | |
| }; | |
| if (departmentId) { | |
| payload.departmentId = departmentId; | |
| } | |
| if (userId) { | |
| payload.userId = userId; | |
| } | |
| await forwardChat(payload); | |
| dispatchToastMessage({ type: 'success', message: t('Transferred') }); | |
| router.navigate('/home'); | |
| LegacyRoomManager.close(room.t + room._id); | |
| } catch (error) { | |
| dispatchToastMessage({ type: 'error', message: error }); | |
| } finally { | |
| onCancel(); | |
| } | |
| }, | |
| [room._id, room.t, getUserData, forwardChat, dispatchToastMessage, t, router, onCancel], | |
| ); |
🤖 Prompt for AI Agents
In `@apps/meteor/client/views/omnichannel/modals/ForwardChatModal.tsx` around
lines 53 - 95, The current handleForwardChat performs getUserData before the
try/catch, so failures escape the error handler and onCancel; move the username
lookup into the try block (or wrap the entire function body in try/catch) so any
error from getUserData is caught, call dispatchToastMessage({ type: 'error',
message: ... }) with the error and always execute onCancel in finally,
preserving the current payload assembly logic (room._id, departmentId, userId,
comment, clientAction) and keeping forwardChat, router.navigate,
LegacyRoomManager.close and onCancel behavior intact.
MartinSchoeler
approved these changes
Jan 30, 2026
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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)
Issue(s)
Steps to test or reproduce
Further comments
CORE-1545
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.