chore: migrate rooms.hide and rooms.open to OpenAPI-compliant pattern#39072
Open
Verifieddanny wants to merge 14 commits intoRocketChat:developfrom
Open
chore: migrate rooms.hide and rooms.open to OpenAPI-compliant pattern#39072Verifieddanny wants to merge 14 commits intoRocketChat:developfrom
Verifieddanny wants to merge 14 commits intoRocketChat:developfrom
Conversation
…rifieddanny/Rocket.Chat into refactor/migrate-rooms-leave-endpoint
…rifieddanny/Rocket.Chat into refactor/migrate-rooms-leave-endpoint
…efactor/migrate-rooms-leave-endpoint
…efactor/migrate-rooms-leave-endpoint
Contributor
|
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: 7591ff7 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 |
Contributor
WalkthroughMigrates the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.changeset/chore-mirgrate-room-hide-room-open.md (1)
1-8: Minor typo in changeset filename.The filename contains a typo:
chore-mirgrate-room-hide-room-open.mdshould bechore-migrate-room-hide-room-open.md("mirgrate" → "migrate").🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.changeset/chore-mirgrate-room-hide-room-open.md around lines 1 - 8, The changeset filename has a typo ("mirgrate" → "migrate"); rename the file .changeset/chore-mirgrate-room-hide-room-open.md to .changeset/chore-migrate-room-hide-room-open.md, update any references to that filename in CI or release tooling if present, commit the rename (git mv ...) so history is preserved, and ensure the changeset content (the header and body) remains unchanged other than the filename correction.
🤖 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`:
- Line 1175: Fix the typo in the query options for Users.findOneById: change the
option key from "projections" to the correct Mongo/Mongoose key "projection" so
the call Users.findOneById(this.userId, { projection: { _id: 1 } }) actually
returns only the _id; update the invocation in the surrounding function to use
"projection" wherever "projections" appears.
---
Nitpick comments:
In @.changeset/chore-mirgrate-room-hide-room-open.md:
- Around line 1-8: The changeset filename has a typo ("mirgrate" → "migrate");
rename the file .changeset/chore-mirgrate-room-hide-room-open.md to
.changeset/chore-migrate-room-hide-room-open.md, update any references to that
filename in CI or release tooling if present, commit the rename (git mv ...) so
history is preserved, and ensure the changeset content (the header and body)
remains unchanged other than the filename correction.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.changeset/chore-mirgrate-room-hide-room-open.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
⏰ 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: cubic · AI code reviewer
- GitHub Check: CodeQL-Build
🧰 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 (9)
📓 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:09.561Z
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:18.785Z
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: 2026-02-24T19:09:09.561Z
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:09.561Z
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.
Applied to files:
.changeset/chore-mirgrate-room-hide-room-open.md
📚 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/chore-mirgrate-room-hide-room-open.mdapps/meteor/app/api/server/v1/rooms.ts
📚 Learning: 2026-02-24T19:05:56.710Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: Rocket.Chat repo context: When a workspace manifest on develop already pins a dependency version (e.g., packages/web-ui-registration → "rocket.chat/ui-contexts": "27.0.1"), a lockfile change in a feature PR that upgrades only that dependency’s resolution is considered a manifest-driven sync and can be kept, preferably as a small "chore: sync yarn.lock with manifests" commit.
Applied to files:
.changeset/chore-mirgrate-room-hide-room-open.md
📚 Learning: 2026-02-23T17:53:18.785Z
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:18.785Z
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.
Applied to files:
.changeset/chore-mirgrate-room-hide-room-open.md
📚 Learning: 2026-02-25T20:10:07.657Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38913
File: packages/ddp-client/src/legacy/types/SDKLegacy.ts:34-34
Timestamp: 2026-02-25T20:10:07.657Z
Learning: In the RocketChat/Rocket.Chat monorepo, packages/ddp-client and apps/meteor do not use TypeScript project references. Module augmentations in apps/meteor (e.g., declare module 'rocket.chat/rest-typings') are not visible when compiling packages/ddp-client in isolation, which is why legacy SDK methods that depend on OperationResult types for OpenAPI-migrated endpoints must remain commented out.
Applied to files:
.changeset/chore-mirgrate-room-hide-room-open.md
📚 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-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-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
🧬 Code graph analysis (1)
apps/meteor/app/api/server/v1/rooms.ts (1)
packages/rest-typings/src/v1/rooms.ts (2)
isRoomsHideProps(688-688)isRoomsOpenProps(628-628)
🔇 Additional comments (3)
apps/meteor/app/api/server/v1/rooms.ts (3)
1150-1188: LGTM! Therooms.hideendpoint migration follows the OpenAPI-compliant pattern correctly.The endpoint properly:
- Uses
isRoomsHidePropsfor AJV body validation- Defines response schemas for 200/400/401 status codes
- Preserves business logic (authorization check, user validation, hide method with mod count check)
1190-1214: LGTM! Therooms.openendpoint migration is clean and follows the pattern.The endpoint correctly uses
isRoomsOpenPropsfor validation and maintains the original simple logic of callingopenRoom(userId, roomId).
1147-1148: Continuation ofrooms.leaveendpoint looks correct.The success response follows the established pattern for the endpoint chain.
| return API.v1.unauthorized('Unauthorized'); | ||
| } | ||
|
|
||
| const user = await Users.findOneById(this.userId, { projections: { _id: 1 } }); |
Contributor
There was a problem hiding this comment.
Typo in projection option: projections should be projection.
MongoDB/Mongoose uses projection (singular) as the option name. This typo will cause the projection to be ignored, returning the full user document instead of just { _id: 1 }.
Proposed fix
- const user = await Users.findOneById(this.userId, { projections: { _id: 1 } });
+ const user = await Users.findOneById(this.userId, { projection: { _id: 1 } });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const user = await Users.findOneById(this.userId, { projections: { _id: 1 } }); | |
| const user = await Users.findOneById(this.userId, { projection: { _id: 1 } }); |
🤖 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 1175, Fix the typo in the
query options for Users.findOneById: change the option key from "projections" to
the correct Mongo/Mongoose key "projection" so the call
Users.findOneById(this.userId, { projection: { _id: 1 } }) actually returns only
the _id; update the invocation in the surrounding function to use "projection"
wherever "projections" appears.
Contributor
There was a problem hiding this comment.
isnt this outoff the scoop for this PR?
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for the old implementation of rooms.hide endpoint to check if the typo existed before
# Look for the legacy API.v1.addRoute pattern for rooms.hide
git log -p --all -S "rooms.hide" -- "apps/meteor/app/api/server/v1/rooms.ts" | grep -A 20 -B 5 "projections\|projection" | head -100
Length of output: 59
🏁 Script executed:
#!/bin/bash
# Also search the current state of develop/main branch to see the original implementation
git show origin/develop:apps/meteor/app/api/server/v1/rooms.ts | grep -A 30 "rooms.hide" | grep -A 20 "Users.findOneById"
Length of output: 408
🏁 Script executed:
#!/bin/bash
# Search for other usages of Users.findOneById in the file to see the correct pattern
rg -A 2 "Users\.findOneById" apps/meteor/app/api/server/v1/rooms.ts
Length of output: 354
@ahmed-n-abdeltwab You're absolutely right—this is out of scope for this migration PR.
I checked the original implementation on the develop branch, and the projections typo already existed in the old code. Since this PR is focused on migrating to the OpenAPI pattern while preserving existing behavior, fixing the typo would constitute a logic change (the current code returns the full user document, fixing it would return only _id).
Per the migration guidelines, this PR should maintain behavioral parity with the original implementation, even if that includes pre-existing bugs. The typo should be addressed in a separate bug-fix PR.
My apologies for flagging this during a migration-focused review. Would you like me to open a follow-up issue to track fixing this typo across the codebase?
🧠 Learnings used
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.
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: 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.
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.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #39072 +/- ##
===========================================
+ Coverage 70.66% 70.69% +0.03%
===========================================
Files 3191 3191
Lines 112965 112965
Branches 20451 20467 +16
===========================================
+ Hits 79829 79866 +37
+ Misses 31088 31055 -33
+ Partials 2048 2044 -4
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Migrate
rooms.hideandrooms.opento OpenAPI-compliant patternOverview
Migrated
rooms.hideandrooms.openREST API endpoints from the legacyAPI.v1.addRoute()pattern to the new chainable.post()pattern with full OpenAPI compliance, AJV validation, and typed response schemas.This continues the ongoing effort to migrate all REST endpoints to the OpenAPI-compliant format tracked in RocketChat/Rocket.Chat-Open-API#150.
Changes
rooms.hideendpoint (POST /api/v1/rooms.hide)API.v1.addRoute('rooms.hide', ...)to chainable.post('rooms.hide', ...)patternisRoomsHidePropsbody validator for request validation200:{ success: true }— room successfully hidden400: Bad request error response (invalid roomId, room already hidden, invalid user)401: Unauthorized error responsecanAccessRoomIdAsync, user existence validation, andhideRoomMethodcall with mod count checkrooms.openendpoint (POST /api/v1/rooms.open)API.v1.addRoute('rooms.open', ...)to chainable.post('rooms.open', ...)patternisRoomsOpenPropsbody validator for request validation200:{ success: true }— room successfully opened400: Bad request error response401: Unauthorized error responseopenRoom(userId, roomId)rest-typingscleanuprest-typingspackageExtractRoutesFromAPIfrom the endpoint definitions themselvesTesting
rooms.hidehttp://localhost:3000/api-docs/POST /api/v1/rooms.hide{ "roomId": "<valid-room-id>" }200with{ "success": true }400witherror-room-already-hidden400401rooms.openPOST /api/v1/rooms.open{ "roomId": "<valid-room-id>" }200with{ "success": true }401Screenshots
rooms.hide request and response
rooms..open request and response
Related
Summary by CodeRabbit