chore: Adds deprecation warning on livechat:removeRoom with new endpoint replacing it
#36958
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: 5b9097c 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 |
WalkthroughAdds a REST endpoint POST /v1/livechat/rooms.delete to delete a single closed livechat room, logs deprecation for the Meteor method Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as Client Hook\n(useRemoveCurrentChatMutation)
participant API as REST API\n(/v1/livechat/rooms.delete)
participant Auth as Auth & Permissions
participant Svc as removeOmnichannelRoom
participant DB as DB
User->>UI: Trigger remove chat (rid)
UI->>API: POST /livechat/rooms.delete { roomId }
API->>Auth: Verify auth + permission (remove-closed-livechat-room)
Auth-->>API: OK / Error
API->>API: Validate params and room state\n(type 'l', closed)
alt Valid
API->>Svc: removeOmnichannelRoom(roomId)
Svc->>DB: Delete room and related data
DB-->>Svc: Done
Svc-->>API: Success
API-->>UI: { success: true }
UI-->>User: Confirm removal
else Invalid / Forbidden
API-->>UI: 400/401/403 error
UI-->>User: Show error
end
sequenceDiagram
autonumber
participant Client
participant Method as Meteor Method\n(livechat:removeRoom)
note right of Method #f9f2e7: Deprecated — emits deprecation log\n(use /v1/livechat/rooms.delete)
Client->>Method: Call method (rid)
Method->>Method: Emit deprecation log
Method-->>Client: Proceed with removal (unchanged)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (3)
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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #36958 +/- ##
===========================================
- Coverage 67.37% 66.31% -1.07%
===========================================
Files 3325 3380 +55
Lines 113211 115462 +2251
Branches 20553 21202 +649
===========================================
+ Hits 76274 76565 +291
- Misses 34331 36289 +1958
- Partials 2606 2608 +2
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
…TZ-60 # Conflicts: # apps/meteor/app/livechat/server/api/v1/room.ts
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/rest-typings/src/v1/omnichannel.ts (1)
4056-4687: Add POST '/v1/livechat/rooms.delete' to OmnichannelEndpointsClient code calls useEndpoint('POST', '/v1/livechat/rooms.delete') (apps/meteor/client/views/omnichannel/currentChats/hooks/useRemoveCurrentChatMutation.ts) and the server references it (apps/meteor/app/livechat/server/methods/removeRoom.ts); add a POST entry for '/v1/livechat/rooms.delete' in packages/rest-typings/src/v1/omnichannel.ts with the correct params/response types.
🧹 Nitpick comments (5)
.changeset/three-turkeys-dress.md (1)
6-7: Clean up text and include full endpoint pathClarify the endpoint with /v1 prefix and remove the stray “7” line.
-Adds deprecation warning on `livechat:removeRoom` with new endpoint replacing it; `livechat/rooms.delete` -7 +Deprecate `livechat:removeRoom`; use `/v1/livechat/rooms.delete`.packages/rest-typings/src/v1/omnichannel.ts (1)
3992-4005: Use IRoom['_id'] for roomId to match conventionsAligns with nearby types (e.g., LivechatRoomOnHold). No runtime effect; improves type accuracy.
-type POSTLivechatRemoveRoomParams = { - roomId: string; -}; +type POSTLivechatRemoveRoomParams = { + roomId: IRoom['_id']; +};apps/meteor/tests/e2e/utils/omnichannel/rooms.ts (1)
55-58: Assert delete status for earlier failure signalMatch the pattern used in updateRoom() for clearer test failures.
async delete() { await closeRoom(api, { roomId: room._id, visitorToken }); - return api.post('/livechat/rooms.delete', { roomId: room._id }); + const response = await api.post('/livechat/rooms.delete', { roomId: room._id }); + if (response.status() !== 200) { + throw Error(`Unable to delete room [http status: ${response.status()}]`); + } + return response; },apps/meteor/app/livechat/server/methods/removeRoom.ts (1)
19-19: Fix deprecation log: wrong method nameIt should reference livechat:removeRoom.
- methodDeprecationLogger.method('livechat:removeCustomField', '8.0.0', '/v1/livechat/rooms.delete'); + methodDeprecationLogger.method('livechat:removeRoom', '8.0.0', '/v1/livechat/rooms.delete');apps/meteor/client/views/omnichannel/currentChats/hooks/useRemoveCurrentChatMutation.ts (1)
7-9: Prefer void over null for mutation resultEndpoint returns success with no payload; keep types as void for consistency.
-export const useRemoveCurrentChatMutation = ( - options?: Omit<UseMutationOptions<null, Error, IRoom['_id']>, 'mutationFn'>, -): UseMutationResult<null, Error, IRoom['_id']> => { +export const useRemoveCurrentChatMutation = ( + options?: Omit<UseMutationOptions<void, Error, IRoom['_id']>, 'mutationFn'>, +): UseMutationResult<void, Error, IRoom['_id']> => { const removeRoom = useEndpoint('POST', '/v1/livechat/rooms.delete');Also applies to: 13-13
📜 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 (6)
.changeset/three-turkeys-dress.md(1 hunks)apps/meteor/app/livechat/server/api/v1/room.ts(2 hunks)apps/meteor/app/livechat/server/methods/removeRoom.ts(2 hunks)apps/meteor/client/views/omnichannel/currentChats/hooks/useRemoveCurrentChatMutation.ts(1 hunks)apps/meteor/tests/e2e/utils/omnichannel/rooms.ts(1 hunks)packages/rest-typings/src/v1/omnichannel.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
apps/meteor/tests/e2e/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
apps/meteor/tests/e2e/**/*.{ts,tsx,js,jsx}: Write concise, technical TypeScript/JavaScript with accurate typing
Follow DRY by extracting reusable logic into helper functions or page objects
Avoid code comments in the implementation
Files:
apps/meteor/tests/e2e/utils/omnichannel/rooms.ts
apps/meteor/tests/e2e/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
apps/meteor/tests/e2e/**/*.{ts,tsx}: Avoid using page.locator(); prefer semantic locators like page.getByRole, page.getByLabel, page.getByText, and page.getByTitle
Store commonly used locators in variables/constants for reuse
Use page.waitFor() with specific conditions and avoid hardcoded timeouts
Implement proper wait strategies for dynamic content
Follow the Page Object Model pattern consistently
Files:
apps/meteor/tests/e2e/utils/omnichannel/rooms.ts
🧠 Learnings (3)
📓 Common learnings
Learnt from: cardoso
PR: RocketChat/Rocket.Chat#36890
File: apps/meteor/tests/e2e/e2e-encryption/e2ee-otr.spec.ts:21-26
Timestamp: 2025-09-16T13:33:49.237Z
Learning: In Rocket.Chat test files, the im.delete API endpoint accepts either a `roomId` parameter (requiring the actual DM room _id) or a `username` parameter (for the DM partner's username). It does not accept slug-like constructions such as concatenating usernames together.
📚 Learning: 2025-09-16T13:33:49.237Z
Learnt from: cardoso
PR: RocketChat/Rocket.Chat#36890
File: apps/meteor/tests/e2e/e2e-encryption/e2ee-otr.spec.ts:21-26
Timestamp: 2025-09-16T13:33:49.237Z
Learning: In Rocket.Chat test files, the im.delete API endpoint accepts either a `roomId` parameter (requiring the actual DM room _id) or a `username` parameter (for the DM partner's username). It does not accept slug-like constructions such as concatenating usernames together.
Applied to files:
apps/meteor/tests/e2e/utils/omnichannel/rooms.ts
📚 Learning: 2025-09-16T13:33:49.237Z
Learnt from: cardoso
PR: RocketChat/Rocket.Chat#36890
File: apps/meteor/tests/e2e/e2e-encryption/e2ee-otr.spec.ts:21-26
Timestamp: 2025-09-16T13:33:49.237Z
Learning: The im.delete API endpoint accepts either a `roomId` parameter (requiring the actual DM room _id) or a `username` parameter (for the DM partner's username). Constructing slug-like identifiers like `user2${Users.userE2EE.data.username}` doesn't work for this endpoint.
Applied to files:
apps/meteor/tests/e2e/utils/omnichannel/rooms.ts
🧬 Code graph analysis (2)
apps/meteor/client/views/omnichannel/currentChats/hooks/useRemoveCurrentChatMutation.ts (2)
packages/core-typings/src/IRoom.ts (1)
IRoom(21-95)packages/ui-contexts/src/index.ts (1)
useEndpoint(32-32)
apps/meteor/app/livechat/server/api/v1/room.ts (8)
apps/meteor/app/api/server/api.ts (1)
API(42-77)packages/rest-typings/src/v1/omnichannel.ts (4)
POSTLivechatRemoveRoomSuccess(4020-4020)isPOSTLivechatRemoveRoomParams(4007-4007)isPOSTLivechatRoomsCloseAllSuccessResponse(3988-3990)isPOSTLivechatRoomsCloseAll(3971-3971)packages/rest-typings/src/v1/Ajv.ts (1)
validateBadRequestErrorResponse(46-46)packages/models/src/index.ts (1)
LivechatRooms(175-175)apps/meteor/app/livechat/server/lib/rooms.ts (1)
removeOmnichannelRoom(257-292)apps/meteor/app/livechat/server/lib/logger.ts (1)
livechatLogger(5-5)apps/meteor/lib/callbacks.ts (1)
callbacks(244-252)packages/core-typings/src/IRoom.ts (1)
IOmnichannelRoom(252-303)
🔇 Additional comments (4)
packages/rest-typings/src/v1/omnichannel.ts (1)
4009-4021: LGTM on success schema/validator exportPattern matches existing success validators.
apps/meteor/app/livechat/server/api/v1/room.ts (3)
22-25: LGTM: typings imports for new routeImports are correct and minimal.
444-476: LGTM: new livechat/rooms.delete routeAuth, permission, validation, and failure paths look good; aligns with existing API style.
477-502: LGTM: removeAllClosedRooms stays intactGood logging and permission split from single-delete.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/meteor/app/livechat/server/methods/removeRoom.ts (1)
7-7: Import LGTM; add JSDoc deprecation on the DDP method signatureSurface the deprecation to TS consumers as well.
declare module '@rocket.chat/ddp-client' { // eslint-disable-next-line @typescript-eslint/naming-convention interface ServerMethods { - 'livechat:removeRoom'(rid: IRoom['_id']): void; + /** + * @deprecated Use POST /v1/livechat/rooms.delete instead. Scheduled for removal in 8.0.0. + */ + 'livechat:removeRoom'(rid: IRoom['_id']): void; } }
📜 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 (1)
apps/meteor/app/livechat/server/methods/removeRoom.ts(2 hunks)
⏰ 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). (7)
- GitHub Check: 🔨 Test Storybook / Test Storybook
- GitHub Check: 🔨 Test Unit / Unit Tests
- GitHub Check: 🔎 Code Check / TypeScript
- GitHub Check: 🔎 Code Check / Code Lint
- GitHub Check: 📦 Meteor Build - coverage
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (1)
apps/meteor/app/livechat/server/methods/removeRoom.ts (1)
19-19: No change required — deprecation style and permissions are correctRepository deprecation messages predominantly use endpoint paths without HTTP verbs; changing this one to "POST /v1/..." would be inconsistent. The API route (apps/meteor/app/livechat/server/api/v1/room.ts) registers POST 'livechat/rooms.delete' with permission 'remove-closed-livechat-room', and the Meteor method checks the same permission — permission parity verified.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/meteor/app/livechat/server/api/v1/room.ts (2)
485-499: Avoid unbounded parallel deletions; count actual successesCurrent approach spawns one promise per room and reports attempts, not successes. This can overload DB and misreport results.
Apply this diff to iterate with backpressure and accurate counting:
- const promises: Promise<void>[] = []; - await LivechatRooms.findClosedRooms(params?.departmentIds, {}, extraQuery).forEach(({ _id }: IOmnichannelRoom) => { - promises.push(removeOmnichannelRoom(_id)); - }); - await Promise.all(promises); - - livechatLogger.info(`User ${this.userId} removed ${promises.length} closed rooms`); - return API.v1.success({ removedRooms: promises.length }); + let removed = 0; + const cursor = LivechatRooms.findClosedRooms(params?.departmentIds, {}, extraQuery); + for await (const { _id } of cursor) { + await removeOmnichannelRoom(_id); // sequential to protect DB; switch to batched if needed + removed++; + } + + livechatLogger.info(`User ${this.userId} removed ${removed} closed rooms`); + return API.v1.success({ removedRooms: removed });If volumes are high, I can provide a batched/concurrency-limited variant (e.g., N=10 in-flight) instead of fully sequential.
446-459: Optional: add audit log for single-room deletionsConsider logging userId and roomId for traceability similar to the bulk endpoint.
Example:
async function action() { const { roomId } = this.bodyParams; + livechatLogger.info(`User ${this.userId} is removing closed room ${roomId}`);
📜 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/livechat/server/api/v1/room.ts(2 hunks)apps/meteor/app/livechat/server/lib/rooms.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
apps/meteor/app/livechat/server/api/v1/room.ts (7)
packages/rest-typings/src/v1/omnichannel.ts (4)
POSTLivechatRemoveRoomSuccess(4020-4020)isPOSTLivechatRemoveRoomParams(4007-4007)isPOSTLivechatRoomsCloseAllSuccessResponse(3988-3990)isPOSTLivechatRoomsCloseAll(3971-3971)packages/rest-typings/src/v1/Ajv.ts (3)
validateBadRequestErrorResponse(46-46)validateUnauthorizedErrorResponse(69-69)validateForbiddenErrorResponse(92-92)apps/meteor/app/livechat/server/lib/rooms.ts (1)
removeOmnichannelRoom(258-301)apps/meteor/app/livechat/server/lib/logger.ts (1)
livechatLogger(5-5)apps/meteor/lib/callbacks.ts (1)
callbacks(244-252)packages/models/src/index.ts (1)
LivechatRooms(175-175)packages/core-typings/src/IRoom.ts (1)
IOmnichannelRoom(252-303)
apps/meteor/app/livechat/server/lib/rooms.ts (1)
packages/core-typings/src/IRoom.ts (1)
isOmnichannelRoom(344-344)
🔇 Additional comments (4)
apps/meteor/app/livechat/server/lib/rooms.ts (1)
2-11: Type guard import LGTMBrings runtime/type safety into the deletion path; good call.
apps/meteor/app/livechat/server/api/v1/room.ts (3)
22-27: New typings/validators import LGTMMatches the new endpoint’s schema contracts.
461-466: Past feedback addressed: open-room check moved into removerOpen/room-type checks now live in removeOmnichannelRoom, avoiding redundant fetch/validation here.
463-472: Extract a robust error message before callingAPI.v1.failure
In thecatchblock, computeconst msg = error instanceof Meteor.Error ? error.reason || (typeof error.error === 'string' ? error.error : undefined) || error.message : error instanceof Error ? error.message : 'error-removing-room'; return API.v1.failure(msg);This always returns
{ success: false; error: string; [stack?]; [errorType?]; [details?] }, satisfyingvalidateBadRequestErrorResponse.
lucas-a-pelegrino
requested review from
sampaiodiego,
dougfabris,
MartinSchoeler and
KevLehman
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/app/livechat/server/lib/rooms.ts (1)
1-302: Add descriptive reasons to all error throws
- Update every
throw new Meteor.Error('error-code')inapps/meteor/app/livechat/server/lib/rooms.tsto include a human-readable reason as the second argument (e.g.
new Meteor.Error('error-omnichannel-is-disabled', 'Livechat is disabled')).- Replace
throw new Error('error-contact-channel-blocked')withthrow new Meteor.Error('error-contact-channel-blocked', 'Contact channel is blocked').
🧹 Nitpick comments (3)
apps/meteor/app/livechat/server/lib/rooms.ts (3)
59-61: Use Meteor.Error (not Error) for API-facing error codesPlain
Error('error-…')won’t populate REST error fields consistently. PreferMeteor.Error(code, reason).- throw new Error('error-contact-channel-blocked'); + throw new Meteor.Error('error-contact-channel-blocked', 'Contact channel is blocked'); - throw new Error('error-contact-channel-blocked'); + throw new Meteor.Error('error-contact-channel-blocked', 'Contact channel is blocked');Also applies to: 101-103
259-261: Log after validation to avoid misleading “Deleting room …” entriesMinor: move the debug log below the existence check to avoid logging deletions for invalid rids.
- livechatLogger.debug(`Deleting room ${rid}`); - check(rid, String); - const room = await LivechatRooms.findOneById(rid); + check(rid, String); + const room = await LivechatRooms.findOneById(rid); + livechatLogger.debug(`Deleting room ${rid}`);
266-273: (Optional) Redundant type guard given LivechatRooms sourceSince
findOneByIdis fromLivechatRooms,t === 'l'should already be guaranteed. Keeping the guard is harmless; if you prefer, document the intent as a defensive check.
📜 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 (1)
apps/meteor/app/livechat/server/lib/rooms.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/meteor/app/livechat/server/lib/rooms.ts (1)
packages/core-typings/src/IRoom.ts (1)
isOmnichannelRoom(355-355)
🔇 Additional comments (3)
apps/meteor/app/livechat/server/lib/rooms.ts (3)
11-11: Import looks goodSeparating value import from the existing
import typeis idiomatic and keeps type‑only elided from runtime.
54-54: Propagate explicit reasons across this flow for consistent REST errorsFor consistency with the endpoint behavior, add reasons to all listed throws in this file.
- throw new Meteor.Error('error-omnichannel-is-disabled'); + throw new Meteor.Error('error-omnichannel-is-disabled', 'Omnichannel is disabled'); - throw new Meteor.Error('cannot-access-room'); + throw new Meteor.Error('cannot-access-room', 'You do not have permission to access this room'); - throw new Meteor.Error('error-omnichannel-is-disabled'); + throw new Meteor.Error('error-omnichannel-is-disabled', 'Omnichannel is disabled'); - throw new Meteor.Error('room-closed'); + throw new Meteor.Error('room-closed', 'Room is already closed'); - throw new Meteor.Error('error-room-onHold'); + throw new Meteor.Error('error-room-onHold', 'Room is on hold'); - throw new Meteor.Error('error-invalid-user'); + throw new Meteor.Error('error-invalid-user', 'Invalid user'); - throw new Meteor.Error('error-returning-inquiry'); + throw new Meteor.Error('error-returning-inquiry', 'Error returning inquiry');Also applies to: 76-76, 98-98, 215-215, 219-219, 228-228, 250-250
266-273: Add human‑readable reasons to Meteor.Error throws (REST returns reason; currently blank)Clients calling the new REST deletion endpoint will see empty
error.reasonfor these throws. Add concise reasons.if (!isOmnichannelRoom(room)) { - throw new Meteor.Error('error-this-is-not-a-livechat-room'); + throw new Meteor.Error('error-this-is-not-a-livechat-room', 'This is not a livechat room'); } if (room.open) { - throw new Meteor.Error('error-room-is-not-closed'); + throw new Meteor.Error('error-room-is-not-closed', 'Room is not closed'); }
lucas-a-pelegrino
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
kodiakhq
bot
removed
the
stat: ready to merge
|
This PR currently has a merge conflict. Please resolve this and then re-add the |
…e/v7/CTZ-60 # Conflicts: # packages/rest-typings/src/v1/omnichannel.ts
lucas-a-pelegrino
dismissed stale reviews from KevLehman and dougfabris
via
4c226f2
lucas-a-pelegrino
added
the
stat: ready to merge
apps/meteor/client/omnichannel/cannedResponses/CannedResponseEditWithDepartmentData.tsx
Outdated
Show resolved
Hide resolved
Proposed changes (including videos or screenshots)
This PR adds a deprecation warning for
livechat:removeRoommeteor method, as well as it adds an endpoint (livechat/rooms.delete) to replace it.Issue(s)
CTZ-60
Steps to test or reproduce
Further comments
Tests will be implemented on this task
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores