refactor: migrate rooms.delete endpoint to new API format#38549
refactor: migrate rooms.delete endpoint to new API format#38549ggazzo merged 5 commits intoRocketChat:developfrom
Conversation
|
Looks like this PR is ready to merge! 🎉 |
🦋 Changeset detectedLatest commit: 63493b7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 41 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 |
|
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughExtracts the rooms.delete route into a new exported Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
🧹 Nitpick comments (2)
apps/meteor/app/api/server/v1/rooms.ts (2)
156-176: Inconsistent error class:MeteorErrorvsMeteor.ErrorLine 162 throws
MeteorError(from@rocket.chat/core-services) while line 168 throwsMeteor.Error. Every other handler in this file usesMeteor.Error. Pick one for consistency within the handler—Meteor.Erroraligns with the rest of the file.Proposed fix
if (!room) { - throw new MeteorError('error-invalid-room', 'Invalid room', { + throw new Meteor.Error('error-invalid-room', 'Invalid room', { method: 'eraseRoom', }); }
159-165: Redundant room existence check —eraseRoomalready validates this.Per the
eraseRoomimplementation inapps/meteor/server/lib/eraseRoom.ts, it resolves the room from an ID, checks existence, and throws'error-invalid-room'if not found. The null check here duplicates that. The lookup itself is still needed for theteamMainguard on line 167, but the explicit "not found" throw (lines 161-165) could be removed without behavior change.Not blocking—just noting the duplication. The explicit check does give a slightly earlier/clearer error path.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/meteor/app/api/server/v1/rooms.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/app/api/server/v1/rooms.ts
🧠 Learnings (4)
📚 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/app/api/server/v1/rooms.ts
📚 Learning: 2025-10-28T16:53:42.761Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37205
File: ee/packages/federation-matrix/src/FederationMatrix.ts:296-301
Timestamp: 2025-10-28T16:53:42.761Z
Learning: In the Rocket.Chat federation-matrix integration (ee/packages/federation-matrix/), the createRoom method from rocket.chat/federation-sdk will support a 4-argument signature (userId, roomName, visibility, displayName) in newer versions. Code using this 4-argument call is forward-compatible with planned library updates and should not be flagged as an error.
Applied to files:
apps/meteor/app/api/server/v1/rooms.ts
📚 Learning: 2025-09-16T13:33:49.237Z
Learnt from: cardoso
Repo: RocketChat/Rocket.Chat PR: 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/app/api/server/v1/rooms.ts
📚 Learning: 2025-09-16T13:33:49.237Z
Learnt from: cardoso
Repo: RocketChat/Rocket.Chat PR: 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/app/api/server/v1/rooms.ts
🧬 Code graph analysis (1)
apps/meteor/app/api/server/v1/rooms.ts (2)
apps/meteor/server/lib/eraseRoom.ts (1)
eraseRoom(11-59)apps/meteor/app/api/server/ApiClass.ts (1)
ExtractRoutesFromAPI(74-78)
🔇 Additional comments (2)
apps/meteor/app/api/server/v1/rooms.ts (2)
124-177: Overall migration looks correct and follows the established pattern.The new endpoint mirrors the structure of
roomInviteEndpoints(AJV body/response schemas,actionhandler, type registration). Schema, auth, and response contracts are consistent.
1122-1124: Type union extension looks correct.The
RoomEndpointstype properly includes the newroomDeleteEndpointviaExtractRoutesFromAPI, matching the established pattern.
|
on running e2e test i am getting an error regarding the 'roomid' not provided. In the previous version of the api were expecting The 'roomId' param is required error but since we are migrating it should use new ajv validation error "must have required property 'roomId'". my question is do we need to modify the test first to migrate to new api format |
- Replace deprecated addRoute with chained .post() method - Add body validation using ajv.compile for roomId - Add response schemas for 200, 400, 401 status codes - Register types via ExtractRoutesFromAPI and declare module - Enable automatic OpenAPI spec generation for rooms.delete
Rohitgiri02
force-pushed
the
migrate/rooms-delete-to-new-api-format
branch
from
f4900ec to
cdd20fb
Compare
for now i have updated the test also to accept new format |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/meteor/app/api/server/v1/rooms.ts (1)
162-170: Inconsistent error constructors:MeteorErrorvsMeteor.Error.Line 162 throws
MeteorError(from@rocket.chat/core-services), while line 168 throwsMeteor.Error(frommeteor/meteor) — two different constructors, two consecutive guard blocks. Both should use the same class.♻️ Normalize to `MeteorError`
- throw new Meteor.Error('error-cannot-delete-team-channel', 'Cannot delete a team channel', { + throw new MeteorError('error-cannot-delete-team-channel', 'Cannot delete a team channel', { method: 'eraseRoom', });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/api/server/v1/rooms.ts` around lines 162 - 170, The two consecutive guards in eraseRoom use different error constructors (MeteorError vs Meteor.Error); change the second throw (the one that currently uses Meteor.Error('error-cannot-delete-team-channel', ...)) to use the same MeteorError class used earlier so both throws call MeteorError('error-cannot-delete-team-channel', 'Cannot delete a team channel', { method: 'eraseRoom' }); ensure the import/usage for MeteorError (the symbol MeteorError) is present and remove/replace any Meteor.Error usage in this function to keep them consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/meteor/app/api/server/v1/rooms.ts`:
- Around line 162-170: The two consecutive guards in eraseRoom use different
error constructors (MeteorError vs Meteor.Error); change the second throw (the
one that currently uses Meteor.Error('error-cannot-delete-team-channel', ...))
to use the same MeteorError class used earlier so both throws call
MeteorError('error-cannot-delete-team-channel', 'Cannot delete a team channel',
{ method: 'eraseRoom' }); ensure the import/usage for MeteorError (the symbol
MeteorError) is present and remove/replace any Meteor.Error usage in this
function to keep them consistent.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/meteor/app/api/server/v1/rooms.tspackages/rest-typings/src/v1/rooms.ts
💤 Files with no reviewable changes (1)
- packages/rest-typings/src/v1/rooms.ts
📜 Review details
🧰 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/app/api/server/v1/rooms.ts
🧠 Learnings (7)
📓 Common learnings
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:01.522Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:06.802Z
Learning: In Rocket.Chat PR reviews, maintain strict scope boundaries—when a PR is focused on a specific endpoint (e.g., rooms.favorite), avoid reviewing or suggesting changes to other endpoints that were incidentally refactored (e.g., rooms.invite) unless explicitly requested by maintainers.
📚 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/app/api/server/v1/rooms.ts
📚 Learning: 2026-02-24T19:09:01.522Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:01.522Z
Learning: In Rocket.Chat OpenAPI migration PRs for endpoints under apps/meteor/app/api/server/v1, avoid introducing logic changes. Only perform scope-tight changes that preserve behavior; style-only cleanups (e.g., removing inline comments) may be deferred to follow-ups to keep the migration PR focused.
Applied to files:
apps/meteor/app/api/server/v1/rooms.ts
📚 Learning: 2025-10-28T16:53:42.761Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37205
File: ee/packages/federation-matrix/src/FederationMatrix.ts:296-301
Timestamp: 2025-10-28T16:53:42.761Z
Learning: In the Rocket.Chat federation-matrix integration (ee/packages/federation-matrix/), the createRoom method from rocket.chat/federation-sdk will support a 4-argument signature (userId, roomName, visibility, displayName) in newer versions. Code using this 4-argument call is forward-compatible with planned library updates and should not be flagged as an error.
Applied to files:
apps/meteor/app/api/server/v1/rooms.ts
📚 Learning: 2025-09-16T13:33:49.237Z
Learnt from: cardoso
Repo: RocketChat/Rocket.Chat PR: 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/app/api/server/v1/rooms.ts
📚 Learning: 2025-09-16T13:33:49.237Z
Learnt from: cardoso
Repo: RocketChat/Rocket.Chat PR: 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/app/api/server/v1/rooms.ts
📚 Learning: 2026-02-23T17:53:06.802Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:06.802Z
Learning: During PR reviews that touch endpoint files under apps/meteor/app/api/server/v1, enforce strict scope: if a PR targets a specific endpoint (e.g., rooms.favorite), do not propose changes to unrelated endpoints (e.g., rooms.invite) unless maintainers explicitly request them. Focus feedback on the touched endpoint's behavior, API surface, and related tests; avoid broad cross-endpoint changes in the same PR unless requested.
Applied to files:
apps/meteor/app/api/server/v1/rooms.ts
🔇 Additional comments (2)
apps/meteor/app/api/server/v1/rooms.ts (2)
124-124: AI summary claimsroomDeleteEndpointis an exported constant, but it is declared withoutexport.Since it is only consumed within this file (
typeof roomDeleteEndpointin theRoomEndpointstype on line 1216), omittingexportis functionally correct. However, for consistency withroomEndpoints(export const) and the stated PR objective, clarify whether the export is intentional or an oversight.
1214-1216:⚠️ Potential issue | 🔴 Critical
roomInviteEndpointsis undefined and will cause a TypeScript compilation error.Line 1215 references
typeof roomInviteEndpointsin the type definition, but this identifier does not exist anywhere in the codebase. The similar identifiersroomEndpoints(line 1006) androomDeleteEndpoint(line 124) are both properly defined, butroomInviteEndpointsis missing entirely. This should be either defined in the file or imported from elsewhere.⛔ Skipped due to learnings
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).Learnt from: ggazzo Repo: RocketChat/Rocket.Chat PR: 35995 File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112 Timestamp: 2026-02-23T17:53:06.802Z Learning: In Rocket.Chat PR reviews, maintain strict scope boundaries—when a PR is focused on a specific endpoint (e.g., rooms.favorite), avoid reviewing or suggesting changes to other endpoints that were incidentally refactored (e.g., rooms.invite) unless explicitly requested by maintainers.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.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: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings (mapping subscription documents to room IDs), never undefined, even when user has no room subscriptions.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: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings by mapping subscription documents to room IDs, never undefined, even when user has no room subscriptions.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.
|
/jira ARCH-1935 |
ggazzo
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #38549 +/- ##
===========================================
- Coverage 70.67% 70.60% -0.07%
===========================================
Files 3190 3190
Lines 112740 112732 -8
Branches 20389 20411 +22
===========================================
- Hits 79678 79598 -80
- Misses 31014 31077 +63
- Partials 2048 2057 +9
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
github-project-automation
bot
moved this to Backlog
in rest api conversion to new endpoints
|
Would you mind running the lint and ts tests on your machine before pushing? It would make our efforts much more effective. |
|
there was some issues previously in my local repo build but now i checked it locally first |
github-project-automation
bot
moved this from Backlog
to Done
in rest api conversion to new endpoints
4 participants
Description
This PR migrates the
rooms.deleteendpoint from the deprecatedaddRoutepattern to the new OpenAPI + AJV validation pattern, following the guidelines inapps/meteor/app/api/README.md.Key Changes
API.v1.addRoute('rooms.delete', ...)with the chainedAPI.v1.post('rooms.delete', ...)method.bodyvalidation usingajv.compile<{ roomId: string }>()with JSON Schema — replaces the manualif (!roomId)runtime check.200— Success response ({ success: true })400—validateBadRequestErrorResponse401—validateUnauthorizedErrorResponse/api-docs/.ExtractRoutesFromAPI<typeof roomDeleteEndpoint>into theRoomEndpointsunion — removing the need for manual type definitions in@rocket.chat/rest-typings.Why
The
addRoutemethod andvalidateParamsproperty are deprecated per the API README. The new format enables:ExtractRoutesFromAPIbody/queryinstead ofvalidateParamsTesting & Verification
yarn turbo run typecheck --filter='@rocket.chat/meteor'— passes (0 errors)yarn eslint:fix && yarn stylelint:fix— no issuesyarn lint— passesScreenShot of Swagger ui testing
Summary by CodeRabbit
COMM-144
Task: ARCH-2001