Skip to content

Conversation

@lucas-a-pelegrino
Copy link
Contributor

@lucas-a-pelegrino lucas-a-pelegrino commented Sep 16, 2025

Proposed changes (including videos or screenshots)

This PR adds a deprecation warning for livechat:removeRoom meteor method, as well as it adds an endpoint (livechat/rooms.delete) to replace it.

Issue(s)

CTZ-60
CORE-1410

Steps to test or reproduce

Further comments

Tests will be implemented on this task

Summary by CodeRabbit

  • New Features
    • Added a REST endpoint to delete individual closed Livechat rooms.
    • Retained support for removing all closed rooms via the existing endpoint.
  • Bug Fixes
    • Prevented deletion of non-Livechat or open rooms with clearer validation and error feedback.
  • Documentation
    • Marked livechat:removeRoom as deprecated; use /v1/livechat/rooms.delete instead.
  • Chores
    • Updated dependencies with patch releases for core packages.

@dionisio-bot
Copy link
Contributor

dionisio-bot bot commented Sep 16, 2025

Looks like this PR is not ready to merge, because of the following issues:

  • This PR is targeting the wrong base branch. It should target 7.12.0, but it targets 7.11.0

Please fix the issues and try again

If you have any trouble, please check the PR guidelines

@changeset-bot
Copy link

changeset-bot bot commented Sep 16, 2025

🦋 Changeset detected

Latest commit: b6d570a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 42 packages
Name Type
@rocket.chat/meteor Patch
@rocket.chat/rest-typings Patch
@rocket.chat/api-client Patch
@rocket.chat/core-services Patch
@rocket.chat/ddp-client Patch
@rocket.chat/http-router Patch
@rocket.chat/models Patch
@rocket.chat/ui-contexts Patch
@rocket.chat/web-ui-registration Patch
@rocket.chat/account-service Patch
@rocket.chat/authorization-service Patch
@rocket.chat/ddp-streamer Patch
@rocket.chat/stream-hub-service Patch
@rocket.chat/federation-matrix Patch
@rocket.chat/omnichannel-services Patch
@rocket.chat/presence Patch
rocketchat-services Patch
@rocket.chat/federation-service Patch
@rocket.chat/omnichannel-transcript Patch
@rocket.chat/presence-service Patch
@rocket.chat/queue-worker Patch
@rocket.chat/network-broker Patch
@rocket.chat/omni-core-ee Patch
@rocket.chat/livechat Patch
@rocket.chat/mock-providers Patch
@rocket.chat/cron Patch
@rocket.chat/instance-status Patch
@rocket.chat/omni-core Patch
@rocket.chat/media-calls Patch
@rocket.chat/uikit-playground Patch
@rocket.chat/fuselage-ui-kit Patch
@rocket.chat/gazzodown Patch
@rocket.chat/ui-avatar Patch
@rocket.chat/ui-client Patch
@rocket.chat/ui-video-conf Patch
@rocket.chat/ui-voip Patch
@rocket.chat/core-typings Patch
@rocket.chat/apps Patch
@rocket.chat/freeswitch Patch
@rocket.chat/model-typings Patch
@rocket.chat/license Patch
@rocket.chat/pdf-worker Patch

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

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 16, 2025

Walkthrough

Adds REST endpoint POST livechat/rooms.delete for per-room deletion, deprecates Meteor method livechat:removeRoom with logging, updates client hook and e2e tests to use the new endpoint, tightens server-side room removal guards, and extends REST typings and route exposure. Includes changeset bump and deprecation note.

Changes

Cohort / File(s) Summary
REST typings: new remove room + custom fields adjustments
packages/rest-typings/src/v1/omnichannel.ts
Adds validators/types for POST livechat/rooms.delete (params and success). Reworks Remove Custom Fields schemas/exports (remove old, add new).
Livechat REST API: new per-room delete + keep bulk remove
apps/meteor/app/livechat/server/api/v1/room.ts
Introduces POST livechat/rooms.delete with auth/permission checks and error validators; retains livechat/rooms.removeAllClosedRooms. Exposes new route via endpoints typing.
Server lib: stricter room removal guards
apps/meteor/app/livechat/server/lib/rooms.ts
Adds isOmnichannelRoom check and prevents removal of open rooms in removeOmnichannelRoom.
Meteor method deprecation
apps/meteor/app/livechat/server/methods/removeRoom.ts
Logs deprecation for livechat:removeRoom pointing to /v1/livechat/rooms.delete; no signature/logic change otherwise.
Client hook migration to REST
apps/meteor/client/views/omnichannel/currentChats/hooks/useRemoveCurrentChatMutation.ts
Switches from useMethod('livechat:removeRoom') to useEndpoint('POST','/v1/livechat/rooms.delete'). Return type now null; options generics updated accordingly.
E2E tests: use REST delete
apps/meteor/tests/e2e/utils/omnichannel/rooms.ts
Replaces RPC-style method call with POST to /livechat/rooms.delete sending { roomId }.
Changeset metadata
.changeset/three-turkeys-dress.md
Bumps packages; documents deprecation of livechat:removeRoom with replacement endpoint.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor User
  participant Client
  participant API as REST API /v1/livechat/rooms.delete
  participant Auth as Auth & Permissions
  participant Rooms as RoomsService (removeOmnichannelRoom)
  participant DB as DB

  User->>Client: Remove room (rid)
  Client->>API: POST { roomId }
  API->>Auth: Validate token + permission remove-closed-livechat-room
  alt Unauthorized/Forbidden
    Auth-->>API: 401/403
    API-->>Client: Error (401/403)
  else Authorized
    API->>DB: Load room by id
    alt Not found / not livechat / open
      API-->>Client: 400 Bad Request
    else Closed livechat room
      API->>Rooms: removeOmnichannelRoom(roomId)
      Rooms->>DB: Delete room + artifacts
      Rooms-->>API: success
      API-->>Client: { success: true }
    end
  end
Loading
sequenceDiagram
  autonumber
  participant Admin as Admin Client
  participant API as REST API /v1/livechat/rooms.removeAllClosedRooms
  participant Rooms as RoomsService
  participant DB as DB
  Admin->>API: POST
  API->>DB: Find closed livechat rooms
  loop For each closed room
    API->>Rooms: removeOmnichannelRoom(roomId)
    Rooms->>DB: Delete room data
  end
  API-->>Admin: { removedRooms: N, success: true }
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Suggested reviewers

Poem

A nibble of code, a hop to delete,
Old burrow’s marked “deprecated” neat.
New trail: rooms.delete—swift and bright,
Guards keep open warrens in sight.
Tests thump approval, hooks realign—
Patchy clouds parted; endpoints shine. 🐇✨

Pre-merge checks and finishing touches

❌ Failed checks (3 warnings)
Check name Status Explanation Resolution
Linked Issues Check ⚠️ Warning The PR fulfills CORE-1410 by deprecating the Meteor method and adding the new REST endpoint as specified, but it does not include any tests for the livechat/rooms.delete endpoint, which CORE-1362 requires for compliance. Include automated tests for the livechat/rooms.delete endpoint in this PR or link to a companion PR implementing CORE-1362 to meet test coverage requirements.
Out of Scope Changes Check ⚠️ Warning The changes around introducing and removing custom fields schemas in the REST typings are unrelated to the deprecation of livechat:removeRoom and the new delete endpoint, indicating modifications beyond the linked issues’ objectives. Revert or relocate the custom fields schema changes into a separate PR to keep this changeset focused on the deprecation warning and endpoint replacement.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The pull request title clearly describes the main changes by noting the addition of a deprecation warning on the existing livechat:removeRoom method and the introduction of its replacement endpoint in a concise, single sentence that is fully related to the changeset.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch chore/v7/CTZ-60

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov
Copy link

codecov bot commented Sep 16, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 66.26%. Comparing base (a4f615f) to head (b6d570a).
⚠️ Report is 1 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop   #36958      +/-   ##
===========================================
- Coverage    67.30%   66.26%   -1.05%     
===========================================
  Files         3278     3330      +52     
  Lines       112000   114112    +2112     
  Branches     20317    20933     +616     
===========================================
+ Hits         75386    75620     +234     
- Misses       34017    35886    +1869     
- Partials      2597     2606       +9     
Flag Coverage Δ
e2e 57.29% <100.00%> (-0.01%) ⬇️
unit 71.14% <ø> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@lucas-a-pelegrino lucas-a-pelegrino marked this pull request as ready for review September 17, 2025 17:39
@lucas-a-pelegrino lucas-a-pelegrino requested review from a team as code owners September 17, 2025 17:39
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 OmnichannelEndpoints

Client 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 path

Clarify 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 conventions

Aligns 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 signal

Match 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 name

It 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 result

Endpoint 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 27cc5b7 and a68b10c.

📒 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 export

Pattern matches existing success validators.

apps/meteor/app/livechat/server/api/v1/room.ts (3)

22-25: LGTM: typings imports for new route

Imports are correct and minimal.


444-476: LGTM: new livechat/rooms.delete route

Auth, permission, validation, and failure paths look good; aligns with existing API style.


477-502: LGTM: removeAllClosedRooms stays intact

Good logging and permission split from single-delete.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 signature

Surface 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.

📥 Commits

Reviewing files that changed from the base of the PR and between a68b10c and fa8c42a.

📒 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 correct

Repository 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.

KevLehman
KevLehman previously approved these changes Sep 17, 2025
@dougfabris dougfabris added this to the 7.12.0 milestone Sep 24, 2025
@kodiakhq kodiakhq bot removed the stat: ready to merge PR tested and approved waiting for merge label Sep 24, 2025
@kodiakhq
Copy link
Contributor

kodiakhq bot commented Sep 24, 2025

This PR currently has a merge conflict. Please resolve this and then re-add the ['stat: ready to merge', 'automerge'] label.

…e/v7/CTZ-60

# Conflicts:
#	packages/rest-typings/src/v1/omnichannel.ts
@lucas-a-pelegrino lucas-a-pelegrino added the stat: ready to merge PR tested and approved waiting for merge label Sep 25, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

📜 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 447c6cc and 00bf64d.

📒 Files selected for processing (2)
  • apps/meteor/app/livechat/server/lib/rooms.ts (2 hunks)
  • packages/rest-typings/src/v1/omnichannel.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • apps/meteor/app/livechat/server/lib/rooms.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/rest-typings/src/v1/omnichannel.ts (1)
packages/rest-typings/src/v1/Ajv.ts (1)
  • ajv (23-23)
⏰ 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)
  • GitHub Check: 📦 Build Packages
  • GitHub Check: CodeQL-Build
  • GitHub Check: CodeQL-Build

@kodiakhq kodiakhq bot merged commit fd4f9b2 into develop Oct 7, 2025
48 of 49 checks passed
@kodiakhq kodiakhq bot deleted the chore/v7/CTZ-60 branch October 7, 2025 16:39
debdutdeb pushed a commit that referenced this pull request Oct 7, 2025
juliajforesti pushed a commit that referenced this pull request Oct 8, 2025
juliajforesti pushed a commit that referenced this pull request Oct 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stat: QA assured Means it has been tested and approved by a company insider stat: ready to merge PR tested and approved waiting for merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants