fix: transferToAgent doesn't stop on transfer failure#38061
fix: transferToAgent doesn't stop on transfer failure#38061kodiakhq[bot] merged 8 commits intodevelopfrom
transferToAgent doesn't stop on transfer failure#38061Conversation
|
Looks like this PR is ready to merge! 🎉 |
|
WalkthroughThe PR updates livechat inquiry routing and callback methods to use Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #38061 +/- ##
===========================================
+ Coverage 70.64% 70.69% +0.05%
===========================================
Files 3146 3146
Lines 108826 108829 +3
Branches 19579 19569 -10
===========================================
+ Hits 76876 76940 +64
+ Misses 29937 29890 -47
+ Partials 2013 1999 -14
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
📦 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/08 19:11 (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: Thu, 08 Jan 2026 19:11:09 GMT |
KevLehman
changed the title
transferToAgent doesn't stop on transfer failure
![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/tests/end-to-end/api/livechat/07-queue.ts:
- Around line 897-953: Two duplicate Mocha before hooks should be consolidated
into one: merge the second before (which forces agent B to take a room) into the
first before so setup runs once in order. Locate the first before block that
creates userA/userB, agents, logins, makes them available, creates
forwardingDept, updates Livechat settings, and starts room1 (symbols:
createUser, createAgent, login, makeAgentAvailable, createDepartment,
updateLivechatSettingsForUser, startANewLivechatRoomAndTakeIt,
getLivechatRoomInfo); append the logic from the second before (starting room2
for forwardUserB, assigning visitor2 and forwardingRoom and asserting servedBy)
into that same before, preserving the sequence and assertions, or if the second
setup must run before each test change its hook to beforeEach instead of before.
- Line 648: The before hook grants the 'transfer-livechat-guest' permission via
updatePermission('transfer-livechat-guest', ['livechat-agent']) but the after
hook never restores it; in the test file add a call in the after hook to
updatePermission('transfer-livechat-guest', []) to remove the granted role and
restore state, locating the after hook near the existing before hook and
ensuring the call runs even if tests fail (e.g., add it into the existing
teardown/after block).
📜 Review details
Configuration used: Organization 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 (5)
apps/meteor/app/livechat/server/lib/RoutingManager.tsapps/meteor/ee/app/livechat-enterprise/server/hooks/onAgentAssignmentFailed.tsapps/meteor/server/lib/callbacks.tsapps/meteor/server/services/omnichannel/queue.tsapps/meteor/tests/end-to-end/api/livechat/07-queue.ts
🧰 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/server/services/omnichannel/queue.tsapps/meteor/ee/app/livechat-enterprise/server/hooks/onAgentAssignmentFailed.tsapps/meteor/server/lib/callbacks.tsapps/meteor/app/livechat/server/lib/RoutingManager.tsapps/meteor/tests/end-to-end/api/livechat/07-queue.ts
🧠 Learnings (3)
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 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/ee/app/livechat-enterprise/server/hooks/onAgentAssignmentFailed.ts
📚 Learning: 2025-11-27T17:56:26.050Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37557
File: apps/meteor/client/views/admin/ABAC/AdminABACRooms.tsx:115-116
Timestamp: 2025-11-27T17:56:26.050Z
Learning: In Rocket.Chat, the GET /v1/abac/rooms endpoint (implemented in ee/packages/abac/src/index.ts) only returns rooms where abacAttributes exists and is not an empty array (query: { abacAttributes: { $exists: true, $ne: [] } }). Therefore, in components consuming this endpoint (like AdminABACRooms.tsx), room.abacAttributes is guaranteed to be defined for all returned rooms, and optional chaining before calling array methods like .join() is sufficient without additional null coalescing.
Applied to files:
apps/meteor/ee/app/livechat-enterprise/server/hooks/onAgentAssignmentFailed.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 apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
apps/meteor/tests/end-to-end/api/livechat/07-queue.ts
🧬 Code graph analysis (2)
apps/meteor/server/lib/callbacks.ts (1)
packages/core-typings/src/IRoom.ts (1)
IOmnichannelRoom(269-320)
apps/meteor/app/livechat/server/lib/RoutingManager.ts (1)
packages/core-typings/src/IRoom.ts (1)
IOmnichannelRoom(269-320)
🪛 Biome (2.1.2)
apps/meteor/tests/end-to-end/api/livechat/07-queue.ts
[error] 940-953: Disallow duplicate setup and teardown hooks.
Disallow before duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
⏰ 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). (10)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: cubic · AI code reviewer
- GitHub Check: cubic · AI code reviewer
- GitHub Check: cubic · AI code reviewer
- GitHub Check: cubic · AI code reviewer
- GitHub Check: cubic · AI code reviewer
- GitHub Check: cubic · AI code reviewer
- GitHub Check: cubic · AI code reviewer
- GitHub Check: cubic · AI code reviewer
- GitHub Check: cubic · AI code reviewer
🔇 Additional comments (5)
apps/meteor/server/services/omnichannel/queue.ts (1)
257-260: LGTM! Proper guard for routing failure.The guard correctly handles the new
falsesentinel value fromRoutingManager.delegateInquiry. The!roomcheck catchesnull,undefined, andfalse, preventing potential null-pointer access at line 262 while providing clear failure signaling.apps/meteor/ee/app/livechat-enterprise/server/hooks/onAgentAssignmentFailed.ts (1)
19-36: LGTM! Explicit failure signaling withfalsesentinel.The changes consistently replace implicit
undefinedreturns with explicitfalsereturns across all early-exit paths. This aligns with the updated callback signature inapps/meteor/server/lib/callbacks.ts(line 191) and makes failure states explicit and type-safe.apps/meteor/server/lib/callbacks.ts (1)
191-191: LGTM! Type signature updated for explicit failure handling.The return type change from
undefinedtofalsemakes failure states explicit in the type system. This aligns with the implementation changes inapps/meteor/ee/app/livechat-enterprise/server/hooks/onAgentAssignmentFailed.tsand provides type-safe failure signaling throughout the callback chain.apps/meteor/app/livechat/server/lib/RoutingManager.ts (1)
50-50: LGTM! Return types updated for explicit failure signaling.The return type updates from
voidtofalsefor bothdelegateInquiryandtakeInquiryalign with the PR's objective to make transfer failures explicit. This enables callers (like the queue processor) to distinguish between success and failure states, particularly at line 269 wherecbRoomcould now befalsefrom the callback chain.Also applies to: 65-65
apps/meteor/tests/end-to-end/api/livechat/07-queue.ts (1)
965-1003: Excellent test coverage for the transfer failure fix.The test suite properly validates the PR's fix by:
- Verifying that forwarding to an agent at their limit returns 400 and produces no transfer history messages when the waiting queue is enabled (lines 965-982)
- Confirming that forwarding succeeds when the waiting queue is disabled (lines 984-1003)
This comprehensive coverage ensures the new
falsesentinel behavior works correctly in real-world scenarios.
KevLehman
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
3 participants
Proposed changes (including videos or screenshots)
Issue(s)
https://rocketchat.atlassian.net/browse/SUP-930
Steps to test or reproduce
Further comments
For context: the process should have stopped by the callback returning
undefined... however, the callbacks doitem() ?? firstParamwhich causes that callbacks returning undefined (probably for the cases when the callback doesn't exist yet) are forced to return the first param.The
forwardToAgentfunction expected thetakeInquiryto returnundefinedto stop the process and avoid sending the system message but since it returned something, it just continued "as if it has succeeded" even when it did not.:confused_cat:
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.