refactor: migrate rooms.favorite endpoint to new OpenAPI pattern with AJV validation#38929
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: 528a5d6 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 |
WalkthroughThe rooms.favorite endpoint was moved from an inline route to an exported OpenAPI-style endpoint with AJV validation in the Meteor app; the previous public REST typing for the endpoint was removed and the new endpoint is exposed via module augmentation. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/meteor/app/api/server/v1/rooms.ts (1)
350-354: Consider consolidating the twodeclare module '@rocket.chat/rest-typings'augmentation blocks.Lines 352-354 add a second
interface Endpointsaugmentation in the same file; lines 1125-1127 already contain one forRoomEndpoints. Merging them reduces noise and follows the single-block pattern used elsewhere.♻️ Suggested consolidation
Remove the block at lines 352-354 and fold the new endpoint into the existing augmentation at lines 1125-1127:
-type RoomsFavoriteEndpoint = ExtractRoutesFromAPI<typeof roomsFavoriteEndpoint>; - -declare module '@rocket.chat/rest-typings' { - interface Endpoints extends RoomsFavoriteEndpoint {} -} // ...later in the file (lines ~1123-1127)... -type RoomEndpoints = ExtractRoutesFromAPI<typeof roomEndpoints> & ExtractRoutesFromAPI<typeof roomInviteEndpoints>; +type RoomEndpoints = ExtractRoutesFromAPI<typeof roomEndpoints> & ExtractRoutesFromAPI<typeof roomInviteEndpoints> & ExtractRoutesFromAPI<typeof roomsFavoriteEndpoint>; declare module '@rocket.chat/rest-typings' { // eslint-disable-next-line `@typescript-eslint/naming-convention`, `@typescript-eslint/no-empty-interface` interface Endpoints extends RoomEndpoints {} }🤖 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 350 - 354, There are two separate module augmentations for '@rocket.chat/rest-typings'—one declaring interface Endpoints with RoomEndpoints and a second declaring Endpoints with RoomsFavoriteEndpoint; remove the duplicate block containing RoomsFavoriteEndpoint and add RoomsFavoriteEndpoint into the existing augmentation that already contains RoomEndpoints so there is a single declare module '@rocket.chat/rest-typings' { interface Endpoints extends RoomEndpoints, RoomsFavoriteEndpoint {} } (update the existing augmentation to extend both symbols).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/app/api/server/v1/rooms.ts`:
- Around line 341-345: The handler now calls findRoomByIdOrName with its default
checkedArchived:true which causes archived rooms to throw error-room-archived
and prevents toggleFavoriteMethod from running; update the action function to
call findRoomByIdOrName with checkedArchived:false so archived rooms can still
be unfavorited (i.e., pass { params: this.bodyParams, checkedArchived: false }
to findRoomByIdOrName) while leaving toggleFavoriteMethod(this.userId, room._id,
favorite) unchanged.
- Line 67: Remove the invalid import "import { required } from 'zod/mini';" —
the symbol "required" is not exported by zod/mini and is unused; delete this
import statement and ensure no other code relies on a standalone required symbol
(keep using .required() on Zod schemas or string literals "required" where
appropriate).
---
Nitpick comments:
In `@apps/meteor/app/api/server/v1/rooms.ts`:
- Around line 350-354: There are two separate module augmentations for
'@rocket.chat/rest-typings'—one declaring interface Endpoints with RoomEndpoints
and a second declaring Endpoints with RoomsFavoriteEndpoint; remove the
duplicate block containing RoomsFavoriteEndpoint and add RoomsFavoriteEndpoint
into the existing augmentation that already contains RoomEndpoints so there is a
single declare module '@rocket.chat/rest-typings' { interface Endpoints extends
RoomEndpoints, RoomsFavoriteEndpoint {} } (update the existing augmentation to
extend both symbols).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.changeset/migrate-rooms-favorite-endpoint.mdapps/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 (2)
📚 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:
.changeset/migrate-rooms-favorite-endpoint.mdapps/meteor/app/api/server/v1/rooms.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/app/api/server/v1/rooms.ts
🧬 Code graph analysis (1)
apps/meteor/app/api/server/v1/rooms.ts (3)
apps/meteor/app/channel-settings/server/methods/saveRoomSettings.ts (1)
favorite(348-350)apps/meteor/app/api/server/ApiClass.ts (1)
ExtractRoutesFromAPI(74-78)packages/rest-typings/src/index.ts (1)
Endpoints(48-93)
🔇 Additional comments (1)
.changeset/migrate-rooms-favorite-endpoint.md (1)
1-5: LGTM!Patch-level bump is appropriate for an internal refactor with no consumer-visible runtime change.
| async function action() { | ||
| const { favorite } = this.bodyParams; | ||
| const room = await findRoomByIdOrName({ params: this.bodyParams }); | ||
|
|
||
| if (!this.bodyParams.hasOwnProperty('favorite')) { | ||
| return API.v1.failure("The 'favorite' param is required"); | ||
| } | ||
| await toggleFavoriteMethod(this.userId, room._id, favorite); |
There was a problem hiding this comment.
findRoomByIdOrName with the default checkedArchived: true may silently block favoriting archived rooms.
The old endpoint did not go through findRoomByIdOrName (per the AI summary, room resolution "now" happens here), so users who had archived rooms in their favorites could previously still toggle them. With checkedArchived: true the handler now throws error-room-archived before reaching toggleFavoriteMethod, making the behavior change inconsistent with the PR claim that "business logic is unchanged."
If archiving a room should not prevent it from being un-favorited, pass checkedArchived: false:
🐛 Proposed fix
- const room = await findRoomByIdOrName({ params: this.bodyParams });
+ const room = await findRoomByIdOrName({ params: this.bodyParams, checkedArchived: false });#!/bin/bash
# Check what the old rooms.favorite addRoute handler did before this PR (git blame / old code search)
rg -n 'rooms\.favorite' apps/meteor/app/api/server/v1/rooms.ts
rg -n 'toggleFavorite' apps/meteor/app/api/server/v1/rooms.ts🤖 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 341 - 345, The handler
now calls findRoomByIdOrName with its default checkedArchived:true which causes
archived rooms to throw error-room-archived and prevents toggleFavoriteMethod
from running; update the action function to call findRoomByIdOrName with
checkedArchived:false so archived rooms can still be unfavorited (i.e., pass {
params: this.bodyParams, checkedArchived: false } to findRoomByIdOrName) while
leaving toggleFavoriteMethod(this.userId, room._id, favorite) unchanged.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #38929 +/- ##
===========================================
+ Coverage 70.56% 70.66% +0.10%
===========================================
Files 3189 3189
Lines 112702 112716 +14
Branches 20429 20425 -4
===========================================
+ Hits 79526 79650 +124
+ Misses 31115 31016 -99
+ Partials 2061 2050 -11
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
I already have a PR open for this one. You should trace my existing PR here: RocketChat/Rocket.Chat-Open-API#150 before opening any new ones. so we don't have collisions between our PRs |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/meteor/app/api/server/v1/rooms.ts (1)
345-345:⚠️ Potential issue | 🟡 Minor
findRoomByIdOrNamestill usescheckedArchived: true(default), blocking archivied-room favorite toggles.This was raised in a previous review and remains unaddressed. The business logic claim in the PR ("unchanged") may not hold: the old
addRoutehandler did not callfindRoomByIdOrName, so archived rooms could previously be (un-)favorited. With the defaultcheckedArchived: true, the endpoint now throwserror-room-archivedbeforetoggleFavoriteMethodis reached.🐛 Proposed fix
- const room = await findRoomByIdOrName({ params: this.bodyParams }); + const room = await findRoomByIdOrName({ params: this.bodyParams, checkedArchived: false });🤖 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` at line 345, The handler calls findRoomByIdOrName({ params: this.bodyParams }) which uses the default checkedArchived: true and thus blocks archived-room favorite toggles before toggleFavoriteMethod runs; change the call to pass checkedArchived: false (e.g., findRoomByIdOrName({ params: this.bodyParams, checkedArchived: false })) so archived rooms are returned and toggleFavoriteMethod can handle (un)favoriting; update any related error handling or tests to reflect that the archived check is deferred to toggleFavoriteMethod.
🧹 Nitpick comments (1)
apps/meteor/app/api/server/v1/rooms.ts (1)
333-338: Add a TypeScript generic to the 200-responseajv.compilecall for consistency.Other endpoints in this file (e.g.,
roomInviteEndpointsat line 1105) useajv.compile<void>({...})for success-only responses. Omitting the generic is not a runtime problem, but it diverges from the established pattern.♻️ Proposed fix
- 200: ajv.compile({ + 200: ajv.compile<void>({🤖 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 333 - 338, The 200-response ajv.compile call lacks the TypeScript generic used elsewhere; update the ajv.compile invocation for the 200 response (the object schema with properties: { success: { type: 'boolean', enum: [true] } }) to include the <void> generic to match the established pattern (as used in roomInviteEndpoints) so the call becomes ajv.compile<void>(...) for consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/meteor/app/api/server/v1/rooms.ts`:
- Line 345: The handler calls findRoomByIdOrName({ params: this.bodyParams })
which uses the default checkedArchived: true and thus blocks archived-room
favorite toggles before toggleFavoriteMethod runs; change the call to pass
checkedArchived: false (e.g., findRoomByIdOrName({ params: this.bodyParams,
checkedArchived: false })) so archived rooms are returned and
toggleFavoriteMethod can handle (un)favoriting; update any related error
handling or tests to reflect that the archived check is deferred to
toggleFavoriteMethod.
---
Nitpick comments:
In `@apps/meteor/app/api/server/v1/rooms.ts`:
- Around line 333-338: The 200-response ajv.compile call lacks the TypeScript
generic used elsewhere; update the ajv.compile invocation for the 200 response
(the object schema with properties: { success: { type: 'boolean', enum: [true] }
}) to include the <void> generic to match the established pattern (as used in
roomInviteEndpoints) so the call becomes ajv.compile<void>(...) for consistency.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/meteor/app/api/server/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 (4)
📚 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-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-01-17T01:51:47.764Z
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.
Applied to files:
apps/meteor/app/api/server/v1/rooms.ts
📚 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/app/api/server/v1/rooms.ts
🧬 Code graph analysis (1)
apps/meteor/app/api/server/v1/rooms.ts (2)
apps/meteor/app/api/server/ApiClass.ts (1)
ExtractRoutesFromAPI(74-78)packages/rest-typings/src/index.ts (1)
Endpoints(48-93)
🔇 Additional comments (1)
apps/meteor/app/api/server/v1/rooms.ts (1)
353-358: LGTM!Type extraction via
ExtractRoutesFromAPIand the module augmentation are consistent with the existingRoomEndpointspattern at lines 1127–1131.
|
Ohh ok, my bad @ahmed-n-abdeltwab So do I close this or leave it as I have already migrated for the "rooms.favorite" to the new OpenAPI pattern As I am more particular about contributing immensely to this part of the project moving forward |
|
@ggazzo Thanks for the approval I've pulled changes from develop branch to my branch now |
|
I just noticed that the file is duplicated. If @ahmed-n-abdeltwab doesnt mind, I'll merge this one; otherwise, I'll wait for him to correct what I asked him to fix in the other one. |
|
Ok thank you, can I still be contributing to the api migration moving forward? |
sure, just look to see if there isn't already an open pr for the same thing |
4 participants
refactor: migrate rooms.favorite endpoint to new OpenAPI pattern with AJV validation
Proposed changes (including videos or screenshots)
Migrates the
rooms.favoriteendpoint from the legacyAPI.v1.addRoutepattern to the new OpenAPI-compliant format using AJV schema validation,
continuing the REST API migration effort across the codebase.
Key changes:
API.v1.addRoutewithAPI.v1.postforrooms.favoriteoneOfconstraint enforcing eitherroomIdorroomNamealongsidefavoritein the request bodypackages/rest-typings/src/v1/rooms.tssince it is now auto-generated via
ExtractRoutesFromAPISwagger UI — endpoint documented at
/api-docs/:200 OK response:
400 on invalid body:
** UI Cange:**
Issue(s)
Part of the ongoing REST API migration effort, following:
Steps to test or reproduce
yarn devhttp://localhost:3000/api-docs/X-Auth-TokenandX-User-IdPOST /api/v1/rooms.favorite{ "roomName": "GENERAL", "favorite": true }{ "success": true }{}to confirm 400 validation errorFurther comments
This follows the exact same migration pattern established.
The business logic is unchanged — only the validation and type inference
approach has been updated.
Summary by CodeRabbit