feat: Room List Bridge #37719
feat: Room List Bridge #37719
Conversation
|
Looks like this PR is ready to merge! 🎉 |
🦋 Changeset detectedLatest commit: b6bac7f The changes in this PR will be included in the next version bump. This PR includes changesets to release 43 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 Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a paginated, filterable Apps-Engine API to list rooms as IRoomRaw: new raw room type and exports, bridge API with permission guard and abstract/impl method, accessor with validation, model query and DB implementation, converters for raw output, tests, permission entry and i18n label. Changes
sequenceDiagram
participant App as App
participant RoomRead as RoomRead (accessor)
participant RoomBridge as RoomBridge (API)
participant AppRoomBridge as AppRoomBridge (impl)
participant DB as Rooms DB
participant Converter as Rooms Converter
App->>RoomRead: getAllRooms(options)
activate RoomRead
RoomRead->>RoomRead: validate options & permissions
RoomRead->>RoomBridge: doGetAllRooms(filters, options, appId)
deactivate RoomRead
activate RoomBridge
RoomBridge->>AppRoomBridge: getAllRooms(filters, options, appId)
deactivate RoomBridge
activate AppRoomBridge
AppRoomBridge->>DB: findAllByTypesAndDiscussionAndTeam(query, findOptions)
DB-->>AppRoomBridge: room documents[]
AppRoomBridge->>Converter: convertRoomRaw(each)
Converter-->>AppRoomBridge: IRoomRaw[]
AppRoomBridge-->>RoomBridge: IRoomRaw[]
RoomBridge-->>RoomRead: IRoomRaw[]
RoomRead-->>App: IRoomRaw[]
deactivate AppRoomBridge
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.{ts,tsx,js}📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
Files:
🧠 Learnings (6)📓 Common learnings📚 Learning: 2025-09-25T09:59:26.461ZApplied to files:
📚 Learning: 2025-09-25T09:59:26.461ZApplied to files:
📚 Learning: 2025-09-25T09:59:26.461ZApplied to files:
📚 Learning: 2025-11-27T17:56:26.050ZApplied to files:
📚 Learning: 2025-10-28T16:53:42.761ZApplied to files:
⏰ 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)
🔇 Additional comments (6)
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 |
📦 Docker Image Size Report📈 Changes
📊 Historical Trend---
config:
theme: "dark"
xyChart:
width: 900
height: 400
---
xychart
title "Image Size Evolution by Service (Last 30 Days + This PR)"
x-axis ["11/15 22:28", "11/16 01:28", "11/17 23:50", "11/18 22:53", "11/19 23:02", "11/21 16:49", "11/24 17:34", "11/27 22:32", "11/28 19:05", "12/01 23:01", "12/02 21:57", "12/03 21:00", "12/04 18:17", "12/05 21:56", "12/08 20:15", "12/09 22:17", "12/10 23:26", "12/11 21:56", "12/12 22:45", "12/13 01:34", "12/15 22:31", "12/16 22:18", "12/17 21:04", "12/18 23:12", "12/19 04:43", "12/19 09:58 (PR)"]
y-axis "Size (GB)" 0 --> 0.5
line "account-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "authorization-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "ddp-streamer-service" [0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12]
line "omnichannel-transcript-service" [0.14, 0.14, 0.14, 0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13]
line "presence-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "queue-worker-service" [0.14, 0.14, 0.14, 0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13]
line "rocketchat" [0.36, 0.36, 0.35, 0.35, 0.35, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.35]
line "stream-hub-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
Statistics (last 25 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Fri, 19 Dec 2025 09:58:56 GMT |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #37719 +/- ##
===========================================
+ Coverage 67.66% 67.71% +0.05%
===========================================
Files 3469 3474 +5
Lines 113796 113862 +66
Branches 20926 20942 +16
===========================================
+ Hits 76999 77102 +103
+ Misses 34619 34581 -38
- Partials 2178 2179 +1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![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
🧹 Nitpick comments (2)
apps/meteor/app/apps/server/bridges/rooms.ts (1)
6-6:getAllRoomsquery construction and lightweight conversion are sound; consider minimal extra defense on raw limitsThe new bridge implementation correctly:
- Maps
GetRoomsFilterto a typed MongoDB query (types,onlyDiscussionsvsincludeDiscussions,teamMainOnly).- Applies a hard cap of
Math.min(limit, 100)and deterministic sort (ts: -1) for pagination stability.- Uses the lightweight room converter to avoid extra lookups while still honoring the Apps Engine room shape, and fails fast if the converter is missing.
Given that external apps should normally go through
RoomRead.getAllRooms, which already validates limit and skip, it’s acceptable thatgetAllRoomsassumes a sanefilter. If you expect other internal callers to hit this bridge directly, you might also want to clampskiptoMath.max(skip, 0)(or validate it) to guard against negative values sneaking into the Mongo query.Also applies to: 10-10, 154-196
packages/apps-engine/src/server/bridges/RoomBridge.ts (1)
3-3: Avoid returningundefinedfromdoGetAllRoomson permission failure
doGetAllRoomscurrently follows the existing pattern of otherdo*methods:public async doGetAllRooms(filter: GetRoomsFilter = {}, appId: string): Promise<Array<IRoom>> { if (this.hasReadPermission(appId)) { return this.getAllRooms(filter, appId); } }This means that when
hasReadPermission(appId)is false, the async function resolves toundefinedeven though the signature promisesPromise<Array<IRoom>>. We already know fromUserBridge.doGetUserRoomIdsthat this "implicit undefined on permission failure" behavior is error‑prone. Based on learnings, it would be safer to either:
- Return an empty array for the no‑permission case, or
- Throw a permission‑related error, letting callers handle it explicitly.
For example:
- public async doGetAllRooms(filter: GetRoomsFilter = {}, appId: string): Promise<Array<IRoom>> { - if (this.hasReadPermission(appId)) { - return this.getAllRooms(filter, appId); - } - } + public async doGetAllRooms(filter: GetRoomsFilter = {}, appId: string): Promise<Array<IRoom>> { + if (!this.hasReadPermission(appId)) { + return []; + } + + return this.getAllRooms(filter, appId); + }This keeps the happy path unchanged while ensuring the method never resolves to
undefinedfor callers likeRoomRead.getAllRooms.Also applies to: 18-25, 70-75, 156-157
📜 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 (9)
.changeset/chatty-dingos-bathe.md(1 hunks)apps/meteor/app/apps/server/bridges/rooms.ts(2 hunks)apps/meteor/app/apps/server/converters/rooms.js(3 hunks)packages/apps-engine/src/definition/accessors/IRoomRead.ts(2 hunks)packages/apps-engine/src/server/accessors/RoomRead.ts(2 hunks)packages/apps-engine/src/server/bridges/RoomBridge.ts(4 hunks)packages/apps-engine/tests/server/accessors/RoomRead.spec.ts(4 hunks)packages/apps-engine/tests/test-data/bridges/roomBridge.ts(2 hunks)packages/apps/src/converters/IAppRoomsConverter.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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/apps/server/bridges/rooms.tspackages/apps-engine/tests/test-data/bridges/roomBridge.tspackages/apps-engine/src/server/accessors/RoomRead.tspackages/apps-engine/src/server/bridges/RoomBridge.tspackages/apps-engine/src/definition/accessors/IRoomRead.tspackages/apps-engine/tests/server/accessors/RoomRead.spec.tspackages/apps/src/converters/IAppRoomsConverter.tsapps/meteor/app/apps/server/converters/rooms.js
**/*.spec.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.spec.ts: Use descriptive test names that clearly communicate expected behavior in Playwright tests
Use.spec.tsextension for test files (e.g.,login.spec.ts)
Files:
packages/apps-engine/tests/server/accessors/RoomRead.spec.ts
🧠 Learnings (7)
📓 Common learnings
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: 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).
📚 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: 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.
Applied to files:
apps/meteor/app/apps/server/bridges/rooms.tspackages/apps-engine/tests/test-data/bridges/roomBridge.tspackages/apps-engine/src/server/accessors/RoomRead.tspackages/apps-engine/src/server/bridges/RoomBridge.ts.changeset/chatty-dingos-bathe.mdpackages/apps-engine/src/definition/accessors/IRoomRead.tspackages/apps-engine/tests/server/accessors/RoomRead.spec.tspackages/apps/src/converters/IAppRoomsConverter.tsapps/meteor/app/apps/server/converters/rooms.js
📚 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: 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.
Applied to files:
apps/meteor/app/apps/server/bridges/rooms.tspackages/apps-engine/tests/test-data/bridges/roomBridge.tspackages/apps-engine/src/server/accessors/RoomRead.tspackages/apps-engine/src/server/bridges/RoomBridge.ts.changeset/chatty-dingos-bathe.mdpackages/apps-engine/src/definition/accessors/IRoomRead.tspackages/apps-engine/tests/server/accessors/RoomRead.spec.tspackages/apps/src/converters/IAppRoomsConverter.tsapps/meteor/app/apps/server/converters/rooms.js
📚 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/apps/server/bridges/rooms.tspackages/apps-engine/tests/test-data/bridges/roomBridge.tspackages/apps-engine/src/server/accessors/RoomRead.tspackages/apps-engine/src/server/bridges/RoomBridge.tspackages/apps-engine/src/definition/accessors/IRoomRead.tspackages/apps-engine/tests/server/accessors/RoomRead.spec.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/apps/server/bridges/rooms.tspackages/apps-engine/tests/test-data/bridges/roomBridge.tspackages/apps-engine/src/server/accessors/RoomRead.tspackages/apps-engine/src/server/bridges/RoomBridge.tspackages/apps-engine/src/definition/accessors/IRoomRead.tspackages/apps-engine/tests/server/accessors/RoomRead.spec.tspackages/apps/src/converters/IAppRoomsConverter.tsapps/meteor/app/apps/server/converters/rooms.js
📚 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/apps/server/bridges/rooms.tspackages/apps-engine/tests/test-data/bridges/roomBridge.tspackages/apps-engine/src/server/accessors/RoomRead.tspackages/apps-engine/src/server/bridges/RoomBridge.ts.changeset/chatty-dingos-bathe.mdpackages/apps-engine/tests/server/accessors/RoomRead.spec.tsapps/meteor/app/apps/server/converters/rooms.js
📚 Learning: 2025-10-06T20:30:45.540Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37152
File: packages/apps-engine/tests/test-data/storage/storage.ts:101-122
Timestamp: 2025-10-06T20:30:45.540Z
Learning: In `packages/apps-engine/tests/test-data/storage/storage.ts`, the stub methods (updatePartialAndReturnDocument, updateStatus, updateSetting, updateAppInfo, updateMarketplaceInfo) intentionally throw "Method not implemented." Tests using these methods must stub them using `SpyOn` from the test library rather than relying on actual implementations.
Applied to files:
packages/apps-engine/tests/test-data/bridges/roomBridge.ts
🧬 Code graph analysis (5)
apps/meteor/app/apps/server/bridges/rooms.ts (2)
packages/apps-engine/src/server/bridges/RoomBridge.ts (1)
GetRoomsFilter(18-25)packages/core-typings/src/IRoom.ts (1)
IRoom(21-95)
packages/apps-engine/tests/test-data/bridges/roomBridge.ts (2)
packages/apps-engine/src/server/bridges/RoomBridge.ts (1)
GetRoomsFilter(18-25)packages/core-typings/src/IRoom.ts (1)
IRoom(21-95)
packages/apps-engine/src/definition/accessors/IRoomRead.ts (2)
packages/apps-engine/src/server/bridges/RoomBridge.ts (1)
GetRoomsFilter(18-25)packages/core-typings/src/IRoom.ts (1)
IRoom(21-95)
packages/apps-engine/tests/server/accessors/RoomRead.spec.ts (1)
packages/core-typings/src/IRoom.ts (1)
IRoom(21-95)
packages/apps/src/converters/IAppRoomsConverter.ts (1)
packages/core-typings/src/IRoom.ts (1)
IRoom(21-95)
🪛 Biome (2.1.2)
apps/meteor/app/apps/server/converters/rooms.js
[error] 1-2: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 2-3: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
🔇 Additional comments (8)
.changeset/chatty-dingos-bathe.md (1)
1-7: Changeset entry matches the new Apps Engine room listing featureThe summary is accurate and scoped to the minor bumps and new room listing bridge; no changes needed.
packages/apps-engine/src/definition/accessors/IRoomRead.ts (1)
1-1: NewgetAllRoomsaccessor is well-shaped and consistent with bridge filter typeThe import of
GetRoomsFilterand thegetAllRooms(filter?: Partial<GetRoomsFilter>): Promise<Array<IRoom>>signature cleanly expose the new workspace-wide room listing API and match the server accessor/bridge contract.Also applies to: 64-71
packages/apps-engine/src/server/accessors/RoomRead.ts (1)
6-6: Accessor‑side validation and normalization forgetAllRoomslook correct
- Limit and skip validation enforce sane bounds (1–100, skip ≥ 0) before hitting the bridge.
- Conflict check for
onlyDiscussionswithincludeDiscussions === falseavoids ambiguous filter semantics.- Passing
{ ...filter, limit, skip: filter.skip ?? 0 }todoGetAllRoomskeeps the effective filter explicit while matching theGetRoomsFiltershape.No changes needed here.
Also applies to: 49-72
packages/apps-engine/tests/test-data/bridges/roomBridge.ts (1)
5-5: Test bridgegetAllRoomsstub correctly matches the abstract contractThe added
getAllRooms(filter: GetRoomsFilter, appId: string)mirrorsRoomBridge’s abstract signature and follows the existing pattern of throwing"Method not implemented."in test data, which is consistent with other bridges.Also applies to: 36-38
packages/apps-engine/tests/server/accessors/RoomRead.spec.ts (1)
38-39: Tests exercise both failure and success paths ofgetAllRooms
doGetAllRoomsmock returningtheRoomsverifies the accessor delegates correctly.- The
caughtflag aroundrr.getAllRooms({ limit: 200 })confirms the limit validation triggers.- Final assertions on
await rr.getAllRooms()ensure the default call returns the expected room array.Looks good; no changes required.
Also applies to: 58-60, 79-87, 99-100
apps/meteor/app/apps/server/converters/rooms.js (1)
2-2: Lightweight room conversion path is consistent and avoids extra lookups
- Extending
convertRoomwith{ lightweight = false }and dispatching toconvertRoomWithoutLookupskeeps the existing behavior untouched while enabling an optimized listing path.convertRoomWithoutLookupsconstructs a reasonablecreatorobject fromroom.uwhen available, with sensible defaults and a fallback user when it is not, and maps core fields (id, names, type, usernames/userIds, flags, counts, timestamps, description, custom/custom livechat data) without any DB access.- Using
_convertTypeToApp(room.t)ensures the same room type semantics as the full converter.Overall this looks correct for the intended “many rooms, low per‑room cost” use case, and it aligns with the new
getAllRoomsbridge behavior.Also applies to: 180-188, 345-404
packages/apps/src/converters/IAppRoomsConverter.ts (2)
5-7: RoomConversionOptions shape is clear and extensibleUsing a dedicated
RoomConversionOptionstype with alightweight?: booleanflag is a clean way to introduce lightweight conversion without overloading the method signature, and it leaves room for future options. No issues from a typing or design perspective here.
12-14: convertRoom overloads with options are consistent; verify implementations and callersThe overload set correctly distinguishes
undefined | nullfrom concreteIRoomand uses a broader union for the implementation signature; addingoptions?: RoomConversionOptionsacross them is type-safe and backward compatible at the call-site level. Please double-check that all concreteIAppRoomsConverterimplementations and any indirect wrappers (e.g., room bridge / server converters) have updated signatures and that they actually honoroptions.lightweightrather than silently ignoring it.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/apps-engine/src/definition/rooms/RoomType.ts (1)
1-8: Remove or correct theDISCUSSIONandTEAMenum values — they do not map to actual Rocket.Chat room types.The values
DISCUSSION = 'discussion'andTEAM = 'team'are fundamentally incompatible with Rocket.Chat's room type system. In Rocket.Chat, discussions and teams are not room types themselves; they are identified by metadata fields (discussions usepridfor parent room ID, teams useteamId/teamMain) while retaining a base room type of'c'(public) or'p'(private). These enum values have no internal representation in Rocket.Chat and are unused in the apps-engine codebase. Apps attempting to useRoomType.DISCUSSIONorRoomType.TEAMfor room creation or filtering will fail. Either remove these enum members or provide conversion logic and clear documentation.
🧹 Nitpick comments (2)
packages/apps-engine/tests/server/accessors/RoomRead.spec.ts (1)
92-99: Consider using Alsatian's built-in throw assertion.The try/catch pattern works, but Alsatian provides
Expect(() => ...).toThrowAsync()for cleaner error assertions that also verify the error message.- let caught = false; - try { - await rr.getAllRooms({ limit: 200 }); - } catch (e) { - caught = true; - } - - Expect(caught).toBeTruthy(); + await Expect(async () => rr.getAllRooms({ limit: 200 })).toThrowAsync();apps/meteor/app/apps/server/bridges/rooms.ts (1)
348-405: Clarify intended behavior for room type filtering overlap.When filtering by
RoomType.CHANNEL, the query returns all rooms witht: 'c', which includes discussion rooms created from channels (since discussions inherit the parent room's type). Requesting bothRoomType.CHANNELandRoomType.DISCUSSIONcreates overlapping conditions via$or: discussions of channels match both{ t: { $in: ['c'] } }and{ prid: { $exists: true } }.If filtering for "pure channels" (excluding discussions) is a supported use case, the current API provides no way to achieve this.
📜 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 (12)
apps/meteor/app/apps/server/bridges/rooms.ts(3 hunks)apps/meteor/app/apps/server/converters/rooms.js(5 hunks)packages/apps-engine/src/definition/accessors/IRoomRead.ts(2 hunks)packages/apps-engine/src/definition/rooms/IRoomRaw.ts(1 hunks)packages/apps-engine/src/definition/rooms/RoomType.ts(1 hunks)packages/apps-engine/src/definition/rooms/index.ts(1 hunks)packages/apps-engine/src/server/accessors/RoomRead.ts(2 hunks)packages/apps-engine/src/server/bridges/RoomBridge.ts(4 hunks)packages/apps-engine/tests/server/accessors/RoomRead.spec.ts(6 hunks)packages/apps-engine/tests/test-data/bridges/roomBridge.ts(2 hunks)packages/apps/src/AppsEngine.ts(1 hunks)packages/apps/src/converters/IAppRoomsConverter.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/apps-engine/src/definition/accessors/IRoomRead.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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:
packages/apps-engine/src/definition/rooms/RoomType.tspackages/apps-engine/src/definition/rooms/index.tspackages/apps-engine/src/server/bridges/RoomBridge.tspackages/apps/src/AppsEngine.tspackages/apps-engine/tests/test-data/bridges/roomBridge.tsapps/meteor/app/apps/server/bridges/rooms.tspackages/apps-engine/src/definition/rooms/IRoomRaw.tspackages/apps-engine/src/server/accessors/RoomRead.tsapps/meteor/app/apps/server/converters/rooms.jspackages/apps/src/converters/IAppRoomsConverter.tspackages/apps-engine/tests/server/accessors/RoomRead.spec.ts
**/*.spec.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.spec.ts: Use descriptive test names that clearly communicate expected behavior in Playwright tests
Use.spec.tsextension for test files (e.g.,login.spec.ts)
Files:
packages/apps-engine/tests/server/accessors/RoomRead.spec.ts
🧠 Learnings (11)
📓 Common learnings
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 by mapping subscription documents to room IDs, never undefined, even when user has no room subscriptions.
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.
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: 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.
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).
📚 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:
packages/apps-engine/src/definition/rooms/RoomType.tspackages/apps-engine/src/server/bridges/RoomBridge.tspackages/apps/src/AppsEngine.tspackages/apps-engine/tests/test-data/bridges/roomBridge.tsapps/meteor/app/apps/server/bridges/rooms.tspackages/apps-engine/src/definition/rooms/IRoomRaw.tspackages/apps-engine/src/server/accessors/RoomRead.tsapps/meteor/app/apps/server/converters/rooms.jspackages/apps/src/converters/IAppRoomsConverter.tspackages/apps-engine/tests/server/accessors/RoomRead.spec.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:
packages/apps-engine/src/definition/rooms/index.tspackages/apps-engine/src/server/bridges/RoomBridge.tspackages/apps-engine/tests/test-data/bridges/roomBridge.tsapps/meteor/app/apps/server/bridges/rooms.tspackages/apps-engine/src/server/accessors/RoomRead.tspackages/apps-engine/tests/server/accessors/RoomRead.spec.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: 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.
Applied to files:
packages/apps-engine/src/server/bridges/RoomBridge.tspackages/apps/src/AppsEngine.tspackages/apps-engine/tests/test-data/bridges/roomBridge.tsapps/meteor/app/apps/server/bridges/rooms.tspackages/apps-engine/src/definition/rooms/IRoomRaw.tspackages/apps-engine/src/server/accessors/RoomRead.tsapps/meteor/app/apps/server/converters/rooms.jspackages/apps/src/converters/IAppRoomsConverter.tspackages/apps-engine/tests/server/accessors/RoomRead.spec.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: 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.
Applied to files:
packages/apps-engine/src/server/bridges/RoomBridge.tspackages/apps/src/AppsEngine.tspackages/apps-engine/tests/test-data/bridges/roomBridge.tsapps/meteor/app/apps/server/bridges/rooms.tspackages/apps-engine/src/definition/rooms/IRoomRaw.tspackages/apps-engine/src/server/accessors/RoomRead.tsapps/meteor/app/apps/server/converters/rooms.jspackages/apps/src/converters/IAppRoomsConverter.tspackages/apps-engine/tests/server/accessors/RoomRead.spec.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:
packages/apps-engine/src/server/bridges/RoomBridge.tspackages/apps-engine/tests/test-data/bridges/roomBridge.tsapps/meteor/app/apps/server/bridges/rooms.tspackages/apps-engine/src/definition/rooms/IRoomRaw.tspackages/apps-engine/src/server/accessors/RoomRead.tsapps/meteor/app/apps/server/converters/rooms.jspackages/apps/src/converters/IAppRoomsConverter.tspackages/apps-engine/tests/server/accessors/RoomRead.spec.ts
📚 Learning: 2025-10-06T20:30:45.540Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37152
File: packages/apps-engine/tests/test-data/storage/storage.ts:101-122
Timestamp: 2025-10-06T20:30:45.540Z
Learning: In `packages/apps-engine/tests/test-data/storage/storage.ts`, the stub methods (updatePartialAndReturnDocument, updateStatus, updateSetting, updateAppInfo, updateMarketplaceInfo) intentionally throw "Method not implemented." Tests using these methods must stub them using `SpyOn` from the test library rather than relying on actual implementations.
Applied to files:
packages/apps-engine/tests/test-data/bridges/roomBridge.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files
Applied to files:
packages/apps-engine/tests/server/accessors/RoomRead.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
packages/apps-engine/tests/server/accessors/RoomRead.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Maintain test isolation between test cases in Playwright tests
Applied to files:
packages/apps-engine/tests/server/accessors/RoomRead.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `expect` matchers for assertions (`toEqual`, `toContain`, `toBeTruthy`, `toHaveLength`, etc.) instead of `assert` statements in Playwright tests
Applied to files:
packages/apps-engine/tests/server/accessors/RoomRead.spec.ts
🧬 Code graph analysis (7)
packages/apps-engine/tests/test-data/bridges/roomBridge.ts (2)
packages/apps-engine/src/server/bridges/RoomBridge.ts (1)
GetRoomsOptions(18-22)packages/apps-engine/src/definition/rooms/IRoomRaw.ts (1)
IRoomRaw(8-28)
apps/meteor/app/apps/server/bridges/rooms.ts (2)
packages/apps-engine/src/server/bridges/RoomBridge.ts (1)
GetRoomsOptions(18-22)packages/apps-engine/src/definition/rooms/IRoomRaw.ts (1)
IRoomRaw(8-28)
packages/apps-engine/src/definition/rooms/IRoomRaw.ts (2)
packages/apps-engine/src/definition/rooms/index.ts (2)
IRoomRaw(13-13)RoomType(14-14)packages/apps/src/AppsEngine.ts (1)
IRoomRaw(16-16)
packages/apps-engine/src/server/accessors/RoomRead.ts (2)
packages/apps-engine/src/server/bridges/RoomBridge.ts (1)
GetRoomsOptions(18-22)packages/apps-engine/src/definition/rooms/IRoomRaw.ts (1)
IRoomRaw(8-28)
apps/meteor/app/apps/server/converters/rooms.js (1)
packages/apps-engine/src/definition/rooms/index.ts (1)
RoomType(14-14)
packages/apps/src/converters/IAppRoomsConverter.ts (2)
packages/apps-engine/src/definition/rooms/index.ts (1)
IRoom(12-12)packages/apps/src/AppsEngine.ts (1)
IRoom(16-16)
packages/apps-engine/tests/server/accessors/RoomRead.spec.ts (3)
packages/apps-engine/src/definition/rooms/IRoomRaw.ts (1)
IRoomRaw(8-28)packages/apps-engine/src/definition/rooms/index.ts (1)
IRoomRaw(13-13)packages/apps/src/AppsEngine.ts (1)
IRoomRaw(16-16)
🪛 Biome (2.1.2)
packages/apps-engine/tests/test-data/bridges/roomBridge.ts
[error] 4-4: Shouldn't redeclare 'IRoomRaw'. Consider to delete it or rename it.
'IRoomRaw' is defined here:
(lint/suspicious/noRedeclare)
apps/meteor/app/apps/server/converters/rooms.js
[error] 1-2: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 2-3: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
⏰ 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
🔇 Additional comments (14)
packages/apps-engine/src/definition/rooms/index.ts (1)
8-13: LGTM!The
IRoomRawimport and export follow the established pattern for other room-related types in this barrel file.packages/apps/src/AppsEngine.ts (1)
16-16: LGTM!The
IRoomRaw as IAppsRoomRawexport follows the established naming convention used for other type aliases in this file (e.g.,IRoom as IAppsRoom).packages/apps-engine/tests/server/accessors/RoomRead.spec.ts (2)
39-73: LGTM!Test fixtures correctly define
IRoomRaw[]structure withIUserLookupcreator format, and the mock bridge implementation properly returns the expected data.
112-125: LGTM!Good test coverage for the
getAllRoomsmethod verifying both that it returns defined values and that the structure matches the expectedIRoomRawformat.packages/apps-engine/src/server/accessors/RoomRead.ts (1)
49-68: LGTM on the overall implementation!The
getAllRoomsmethod correctly validates pagination parameters, applies sensible defaults, and delegates to the bridge with normalized options. The validation logic is consistent with other methods in the class.packages/apps-engine/src/server/bridges/RoomBridge.ts (3)
67-71: Follows existing permission-check pattern, but note the implicit undefined return.The implementation correctly follows the established pattern in this class. However, like other
do*methods (e.g.,doGetById,doGetMembers), whenhasReadPermissionreturnsfalse, this method implicitly returnsundefineddespite the return type beingPromise<Array<IRoomRaw>>. This is a pre-existing pattern throughout the file.Based on learnings, there's a known similar issue in
UserBridge.doGetUserRoomIds. Consider addressing this pattern in a follow-up to make the permission denial explicit.
18-22: LGTM!The
GetRoomsOptionstype is well-defined with appropriate optional fields for filtering by room types and pagination.
153-154: LGTM!The abstract method signature correctly defines the contract for concrete bridge implementations to provide room listing functionality.
packages/apps-engine/tests/test-data/bridges/roomBridge.ts (1)
38-40: LGTM!The
getAllRoomsstub follows the established pattern for test bridge methods in this file.apps/meteor/app/apps/server/bridges/rooms.ts (1)
154-182: LGTM - well-structured pagination with sensible defaults.The implementation correctly handles pagination, validates the converter, and iterates asynchronously. The hardcoded max limit of 100 is a reasonable safeguard against excessive queries.
packages/apps-engine/src/definition/rooms/IRoomRaw.ts (1)
1-28: LGTM - well-designed lightweight interface.The interface serves its documented purpose of providing a non-relational view for listing operations. The use of
IUserLookupfor the creator field is appropriate to avoid additional lookups.apps/meteor/app/apps/server/converters/rooms.js (2)
24-60: LGTM - clean raw conversion.The
convertRoomRawmethod correctly builds a lightweight representation using the room's embedded user data without additional lookups.
444-454: LGTM - clean type resolution logic.The
_resolveRoomTypehelper correctly prioritizesteamMainandpridchecks before falling back to the standard type conversion, ensuring TEAM and DISCUSSION types are properly detected.packages/apps/src/converters/IAppRoomsConverter.ts (1)
3-21: LGTM - well-typed interface extension.The new
RoomConversionOptionstype andconvertRoomRawoverloads cleanly extend the converter interface. The overload signatures properly handle undefined/null inputs with appropriate return types.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/meteor/app/apps/server/bridges/rooms.ts (1)
348-415:buildRoomQuerymapping looks correct; consider future‑proofing for unknown RoomType valuesThe mapping from
RoomType→{ t, prid, teamMain }filters and the way discussions/teams are included or excluded across combinations (base types +DISCUSSION+TEAM) is coherent and matches the intended sets of rooms.One small robustness concern: if
typesever contains only enum values that fall into thedefaultbranch (e.g., futureRoomTypeadditions not yet handled here),conditionsstays empty and the helper returns{ query: {} }, effectively returning all rooms instead of “no match”. Consider treating the “no recognized types” case as an empty result (e.g., using an impossible filter like{ _id: null }) or at least logging, so misuse or future enum extensions don’t silently broaden the query.
📜 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/apps/server/bridges/rooms.ts(3 hunks)
🧰 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/apps/server/bridges/rooms.ts
🧠 Learnings (6)
📓 Common learnings
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: 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.
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.
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: 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: 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).
📚 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: 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.
Applied to files:
apps/meteor/app/apps/server/bridges/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: 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.
Applied to files:
apps/meteor/app/apps/server/bridges/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/apps/server/bridges/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/apps/server/bridges/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/apps/server/bridges/rooms.ts
🧬 Code graph analysis (1)
apps/meteor/app/apps/server/bridges/rooms.ts (4)
packages/apps-engine/src/server/bridges/RoomBridge.ts (1)
GetRoomsOptions(18-22)packages/apps-engine/src/definition/rooms/index.ts (2)
IRoomRaw(13-13)RoomType(14-14)packages/apps-engine/src/definition/rooms/IRoomRaw.ts (1)
IRoomRaw(8-28)packages/apps/src/AppsEngine.ts (1)
IRoomRaw(16-16)
⏰ 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
🔇 Additional comments (2)
apps/meteor/app/apps/server/bridges/rooms.ts (2)
3-10: New imports for raw rooms, options, and Mongo filters are consistent
IRoomRaw,GetRoomsOptions, andFilterare all used in the new methods (getAllRoomsandbuildRoomQuery) with correct typing againstICoreRoomand Mongo'sFindOptions. No issues here.
154-182:getAllRoomsimplementation is sound and boundedThe method correctly logs the call, applies sensible defaults, caps
limitat 100 to bound query cost, delegates type handling tobuildRoomQuery, and usesconvertRoomRawto keep conversion lightweight while skipping falsy results. Overall behavior looks correct and matches the newIRoomRaw-based contract.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/meteor/app/apps/server/bridges/rooms.ts (1)
348-421: Complex query construction handles room type filtering correctly.The method correctly translates RoomType enums to MongoDB queries, accounting for the orthogonal nature of discussions and teams relative to base room types. The logic properly combines conditions using
$orwhen needed and handles edge cases like empty type arrays.The implementation is sound but consider extracting the condition-building logic into smaller helper methods if this grows more complex in the future.
📜 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/apps/server/bridges/rooms.ts(3 hunks)
🧰 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/apps/server/bridges/rooms.ts
🧠 Learnings (6)
📓 Common learnings
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: 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.
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.
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: 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: 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).
📚 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: 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.
Applied to files:
apps/meteor/app/apps/server/bridges/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: 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.
Applied to files:
apps/meteor/app/apps/server/bridges/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/apps/server/bridges/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/apps/server/bridges/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/apps/server/bridges/rooms.ts
🧬 Code graph analysis (1)
apps/meteor/app/apps/server/bridges/rooms.ts (3)
packages/apps-engine/src/server/bridges/RoomBridge.ts (1)
GetRoomsOptions(18-22)packages/apps-engine/src/definition/rooms/IRoomRaw.ts (1)
IRoomRaw(8-28)packages/apps/src/AppsEngine.ts (1)
IRoomRaw(16-16)
⏰ 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
🔇 Additional comments (2)
apps/meteor/app/apps/server/bridges/rooms.ts (2)
3-3: LGTM!The import additions correctly support the new room listing functionality.
Also applies to: 6-6, 10-10
154-182: No changes needed—sorting bytsis correct.The
tsfield is the appropriate timestamp field used consistently throughout the codebase for database queries. The mapping betweents(internal database) andcreatedAt(app-facing interface) is confirmed by the sorting patterns at lines 121 and 300. The implementation correctly sorts rooms in descending order by timestamp, handles pagination limits appropriately, and includes proper error handling for missing converters.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/meteor/app/apps/server/converters/rooms.js (1)
382-416: Useidproperty instead of_idto conform to Apps Engine IUser interface.Both
creatorFromRoom(line 385) andfallbackUser(line 392) use_id, but the Apps EngineIUserinterface expectsid. This creates inconsistency with the fullconvertRoommethod which properly converts user objects via the user converter.Apply this diff:
const creatorFromRoom = room.u ? { - _id: room.u._id, + id: room.u._id, username: room.u.username, name: room.u.name || room.u.username || 'Unknown', } : undefined; const fallbackUser = { - _id: 'unknown', + id: 'unknown', username: 'unknown', name: 'Unknown', };
📜 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/apps/server/converters/rooms.js(4 hunks)
🧰 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/apps/server/converters/rooms.js
🧠 Learnings (8)
📓 Common learnings
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: 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.
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.
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).
📚 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/apps/server/converters/rooms.js
📚 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/apps/server/converters/rooms.js
📚 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: 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.
Applied to files:
apps/meteor/app/apps/server/converters/rooms.js
📚 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: 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.
Applied to files:
apps/meteor/app/apps/server/converters/rooms.js
📚 Learning: 2025-11-19T18:20:37.116Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37419
File: apps/meteor/server/services/media-call/service.ts:141-141
Timestamp: 2025-11-19T18:20:37.116Z
Learning: In apps/meteor/server/services/media-call/service.ts, the sendHistoryMessage method should use call.caller.id or call.createdBy?.id as the message author, not call.transferredBy?.id. Even for transferred calls, the message should appear in the DM between the two users who are calling each other, not sent by the person who transferred the call.
Applied to files:
apps/meteor/app/apps/server/converters/rooms.js
📚 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/apps/server/converters/rooms.js
📚 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/apps/server/converters/rooms.js
🧬 Code graph analysis (1)
apps/meteor/app/apps/server/converters/rooms.js (1)
packages/apps-engine/src/definition/rooms/index.ts (1)
RoomType(14-14)
⏰ 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
🔇 Additional comments (3)
apps/meteor/app/apps/server/converters/rooms.js (3)
217-224: LGTM! Well-structured performance optimization.The lightweight option provides an efficient path for bulk room listings by bypassing expensive lookups, which aligns well with the PR's objective to support workspace-wide room retrieval.
265-268: LGTM! Consistent type derivation.Using
_resolveRoomTypeinstead of direct_convertTypeToAppensures consistent type resolution logic across all conversion methods (handles team rooms and discussions correctly).
418-428: LGTM! Clean type resolution helper.The helper correctly prioritizes team rooms and discussions before falling back to standard type conversion. This centralization ensures consistent type derivation across all conversion methods.
There was a problem hiding this comment.
Pull request overview
This pull request adds room listing capabilities to the Apps Engine, enabling apps to retrieve and filter rooms across the workspace with pagination support. The implementation introduces a new lightweight conversion mode for improved performance when listing multiple rooms.
Key Changes:
- Added
getAllRoomsmethod to the Apps Engine bridge andIRoomReadaccessor with type-based filtering and pagination (limit/skip) - Introduced
IRoomRawinterface andconvertRoomRawmethod for lightweight room representations without expensive database lookups - Extended
RoomTypeenum withDISCUSSIONandTEAMtypes to differentiate derived room types from base types
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
packages/apps-engine/src/definition/rooms/IRoomRaw.ts |
New interface defining lightweight room representation without relational data lookups |
packages/apps-engine/src/definition/rooms/RoomType.ts |
Added DISCUSSION and TEAM enum values |
packages/apps-engine/src/definition/rooms/index.ts |
Exported IRoomRaw interface |
packages/apps-engine/src/definition/accessors/IRoomRead.ts |
Added getAllRooms method signature to the read accessor interface |
packages/apps-engine/src/server/bridges/RoomBridge.ts |
Added GetRoomsOptions type and abstract getAllRooms method to bridge |
packages/apps-engine/src/server/accessors/RoomRead.ts |
Implemented getAllRooms with validation for limit and skip parameters |
apps/meteor/app/apps/server/bridges/rooms.ts |
Implemented getAllRooms with filtering logic via buildRoomQuery method |
apps/meteor/app/apps/server/converters/rooms.js |
Added convertRoomRaw and convertRoomWithoutLookups methods, plus _resolveRoomType helper |
packages/apps/src/converters/IAppRoomsConverter.ts |
Added RoomConversionOptions type and convertRoomRaw method signatures |
packages/apps/src/AppsEngine.ts |
Exported IRoomRaw type from apps-engine |
packages/apps-engine/tests/test-data/bridges/roomBridge.ts |
Added getAllRooms stub to test bridge |
packages/apps-engine/tests/server/accessors/RoomRead.spec.ts |
Added tests for getAllRooms method with validation checks |
.changeset/chatty-dingos-bathe.md |
Changeset documenting the room listing feature |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/apps-engine/tests/server/accessors/RoomRead.spec.ts (1)
136-171: Comprehensive edge case testing addresses previous feedback!This test method comprehensively covers all validation scenarios requested in the previous review (negative limits, zero limits, non-finite values, and invalid skip values), plus validates that normal cases still work correctly.
Minor formatting issue: Line 165 has a leading space before
await.Apply this diff to fix the formatting:
// Test valid calls to ensure validation doesn't break normal behavior - await Expect(async () => rr.getAllRooms({}, { limit: 1 })).not.toThrowAsync(); + await Expect(async () => rr.getAllRooms({}, { limit: 1 })).not.toThrowAsync();
📜 Review details
Configuration used: Organization 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)
packages/apps-engine/tests/server/accessors/RoomRead.spec.ts(7 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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:
packages/apps-engine/tests/server/accessors/RoomRead.spec.ts
**/*.spec.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.spec.ts: Use descriptive test names that clearly communicate expected behavior in Playwright tests
Use.spec.tsextension for test files (e.g.,login.spec.ts)
Files:
packages/apps-engine/tests/server/accessors/RoomRead.spec.ts
🧠 Learnings (13)
📓 Common learnings
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: 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.
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.
📚 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:
packages/apps-engine/tests/server/accessors/RoomRead.spec.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: 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.
Applied to files:
packages/apps-engine/tests/server/accessors/RoomRead.spec.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: 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.
Applied to files:
packages/apps-engine/tests/server/accessors/RoomRead.spec.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:
packages/apps-engine/tests/server/accessors/RoomRead.spec.ts
📚 Learning: 2025-11-19T12:32:29.696Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37547
File: packages/i18n/src/locales/en.i18n.json:634-634
Timestamp: 2025-11-19T12:32:29.696Z
Learning: Repo: RocketChat/Rocket.Chat
Context: i18n workflow
Learning: In this repository, new translation keys should be added to packages/i18n/src/locales/en.i18n.json only; other locale files are populated via the external translation pipeline and/or fall back to English. Do not request adding the same key to all locale files in future reviews.
Applied to files:
packages/apps-engine/tests/server/accessors/RoomRead.spec.ts
📚 Learning: 2025-11-10T19:06:20.146Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37408
File: apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.tsx:53-69
Timestamp: 2025-11-10T19:06:20.146Z
Learning: In the Rocket.Chat repository, do not provide suggestions or recommendations about code sections marked with TODO comments. The maintainers have already identified these as future work and external reviewers lack the full context about implementation plans and timing.
Applied to files:
packages/apps-engine/tests/server/accessors/RoomRead.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files
Applied to files:
packages/apps-engine/tests/server/accessors/RoomRead.spec.ts
📚 Learning: 2025-12-16T17:29:45.163Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37834
File: apps/meteor/tests/e2e/page-objects/fragments/admin-flextab-emoji.ts:12-22
Timestamp: 2025-12-16T17:29:45.163Z
Learning: In page object files under `apps/meteor/tests/e2e/page-objects/`, always import `expect` from `../../utils/test` (Playwright's async expect), not from Jest. Jest's `expect` has a synchronous signature and will cause TypeScript errors when used with web-first assertions like `toBeVisible()`.
Applied to files:
packages/apps-engine/tests/server/accessors/RoomRead.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
packages/apps-engine/tests/server/accessors/RoomRead.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Maintain test isolation between test cases in Playwright tests
Applied to files:
packages/apps-engine/tests/server/accessors/RoomRead.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `expect` matchers for assertions (`toEqual`, `toContain`, `toBeTruthy`, `toHaveLength`, etc.) instead of `assert` statements in Playwright tests
Applied to files:
packages/apps-engine/tests/server/accessors/RoomRead.spec.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:
packages/apps-engine/tests/server/accessors/RoomRead.spec.ts
🧬 Code graph analysis (1)
packages/apps-engine/tests/server/accessors/RoomRead.spec.ts (2)
packages/apps-engine/src/definition/rooms/IRoomRaw.ts (1)
IRoomRaw(11-46)packages/apps-engine/src/server/bridges/index.ts (1)
RoomBridge(43-43)
⏰ 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
🔇 Additional comments (4)
packages/apps-engine/tests/server/accessors/RoomRead.spec.ts (4)
4-4: LGTM!The
IRoomRawimport is correctly added to support the new test data structure.
26-26: LGTM!The defensive fallback for
room.idensures test stability, and thetheRoomsmock data correctly matches theIRoomRawinterface structure with appropriate field mappings.Also applies to: 39-51
71-73: LGTM!The mock bridge implementation for
doGetAllRoomsis appropriate for testing, and the double type assertion at line 83 is a standard pattern for partial mocks in tests.Also applies to: 83-83
104-117: LGTM!The test assertions properly verify that
getAllRooms()returns defined data matching the expectedIRoomRawstructure.
Co-authored-by: Guilherme Gazzo <guilherme@gazzo.xyz>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/meteor/app/apps/server/bridges/rooms.ts (1)
189-198: Add defensive validation forlimitandskipparameters.While the accessor layer validates these inputs, adding defensive validation at the bridge boundary would prevent potential issues from malformed input. The current code doesn't validate that
limitandskipare non-negative finite numbers.🔎 Suggested defensive validation
- const { limit = 100, skip = 0 } = options; + const rawLimit = Number(options?.limit ?? 100); + const rawSkip = Number(options?.skip ?? 0); + + const limit = Math.max(0, Math.min(Number.isFinite(rawLimit) ? rawLimit : 100, 100)); + const skip = Math.max(0, Number.isFinite(rawSkip) ? rawSkip : 0);
📜 Review details
Configuration used: Organization 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 (7)
apps/meteor/app/apps/server/bridges/rooms.ts(3 hunks)apps/meteor/app/apps/server/converters/rooms.js(3 hunks)apps/meteor/app/apps/server/converters/visitors.js(1 hunks)packages/apps-engine/src/definition/livechat/IVisitor.ts(1 hunks)packages/apps-engine/src/definition/rooms/IRoomRaw.ts(1 hunks)packages/model-typings/src/models/IRoomsModel.ts(1 hunks)packages/models/src/models/Rooms.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/model-typings/src/models/IRoomsModel.ts
- packages/apps-engine/src/definition/rooms/IRoomRaw.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:
packages/apps-engine/src/definition/livechat/IVisitor.tsapps/meteor/app/apps/server/converters/visitors.jsapps/meteor/app/apps/server/converters/rooms.jsapps/meteor/app/apps/server/bridges/rooms.tspackages/models/src/models/Rooms.ts
🧠 Learnings (11)
📓 Common learnings
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: 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).
📚 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/apps/server/converters/rooms.jsapps/meteor/app/apps/server/bridges/rooms.tspackages/models/src/models/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/apps/server/converters/rooms.jsapps/meteor/app/apps/server/bridges/rooms.tspackages/models/src/models/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: 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.
Applied to files:
apps/meteor/app/apps/server/converters/rooms.jsapps/meteor/app/apps/server/bridges/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/apps/server/converters/rooms.jsapps/meteor/app/apps/server/bridges/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: 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.
Applied to files:
apps/meteor/app/apps/server/converters/rooms.jsapps/meteor/app/apps/server/bridges/rooms.ts
📚 Learning: 2025-11-19T18:20:37.116Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37419
File: apps/meteor/server/services/media-call/service.ts:141-141
Timestamp: 2025-11-19T18:20:37.116Z
Learning: In apps/meteor/server/services/media-call/service.ts, the sendHistoryMessage method should use call.caller.id or call.createdBy?.id as the message author, not call.transferredBy?.id. Even for transferred calls, the message should appear in the DM between the two users who are calling each other, not sent by the person who transferred the call.
Applied to files:
apps/meteor/app/apps/server/converters/rooms.js
📚 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/apps/server/converters/rooms.js
📚 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/apps/server/converters/rooms.js
📚 Learning: 2025-11-10T19:06:20.146Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37408
File: apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.tsx:53-69
Timestamp: 2025-11-10T19:06:20.146Z
Learning: In the Rocket.Chat repository, do not provide suggestions or recommendations about code sections marked with TODO comments. The maintainers have already identified these as future work and external reviewers lack the full context about implementation plans and timing.
Applied to files:
apps/meteor/app/apps/server/converters/rooms.js
📚 Learning: 2025-10-24T17:32:05.348Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37299
File: apps/meteor/ee/server/lib/ldap/Manager.ts:438-454
Timestamp: 2025-10-24T17:32:05.348Z
Learning: In Rocket.Chat, ABAC attributes can only be set on private rooms and teams (type 'p'), not on public rooms (type 'c'). Therefore, when checking for ABAC-protected rooms/teams during LDAP sync or similar operations, it's sufficient to query only private rooms using methods like `findPrivateRoomsByIdsWithAbacAttributes`.
Applied to files:
apps/meteor/app/apps/server/bridges/rooms.ts
🧬 Code graph analysis (2)
apps/meteor/app/apps/server/bridges/rooms.ts (2)
packages/apps-engine/src/server/bridges/RoomBridge.ts (2)
GetRoomsFilters(21-40)GetRoomsOptions(42-45)packages/apps-engine/src/definition/rooms/IRoomRaw.ts (1)
IRoomRaw(10-43)
packages/models/src/models/Rooms.ts (1)
packages/core-typings/src/IRoom.ts (1)
IRoom(22-98)
⏰ 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
🔇 Additional comments (10)
packages/apps-engine/src/definition/livechat/IVisitor.ts (1)
14-14: LGTM! Clean addition of visitor activity field.The optional
activityfield appropriately extends the visitor entity to support richer room data in the new listing API.apps/meteor/app/apps/server/converters/visitors.js (1)
39-39: Activity field is correctly mapped as read-only in visitor converters.The
activityfield is mapped inconvertVisitor(DB → App, line 39) but intentionally excluded fromconvertAppVisitor(App → DB, lines 45-63), making it read-only from the app's perspective. No write operations to visitor activity exist in the codebase; activity is only read in contexts like contact mapping and logging.packages/models/src/models/Rooms.ts (2)
117-120: LGTM!The sparse index on
teamMainis appropriate and will improve query performance for the newfindAllByTypesAndDiscussionAndTeammethod that filters by team main rooms.
2324-2349: LGTM!The query construction logic is clean and follows existing patterns in the codebase. The method correctly builds MongoDB filters based on the provided options.
apps/meteor/app/apps/server/converters/rooms.js (4)
23-33: LGTM!The input guard and
mapUserLookuphelper are well-implemented. The helper correctly handles bothuser._idanduser.idwith appropriate fallback logic, conforming to theIUserLookupinterface.
67-103: LGTM!The field transformers for
creator,closedBy,servedBy, andresponseByfollow a consistent pattern: check for existence, transform usingmapUserLookup, and delete the source field to prevent internal structure leakage. This addresses previous review feedback.
143-143: LGTM!The addition of the
activityfield to the visitor payload is straightforward and aligns with the enhanced visitor entity parity mentioned in the PR objectives.
290-291: LGTM!The addition of
teamIdandisTeamMainmappings ensures parity between the raw and non-raw conversion paths, supporting the new team-related room retrieval functionality.apps/meteor/app/apps/server/bridges/rooms.ts (2)
20-53: LGTM!The
rawRoomProjectionconstant is well-defined and includes all necessary fields for the raw room representation, improving query performance by reducing data transfer.
194-217: LGTM!The main logic of
getAllRoomsis well-implemented. It correctly constructs the query options, calls the model method with appropriate filters, converts each room using the raw converter, and filters out any undefined results. The sequential conversion approach is clear and appropriate for this use case.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/apps-engine/src/server/bridges/RoomBridge.ts (1)
94-98: Consider consistent parameter naming.The first parameter is named
filters(plural) indoGetAllRoomsbutfilter(singular) in the abstractgetAllRooms. Consider using the same name in both methods for consistency.🔎 Proposed fix
- protected abstract getAllRooms(filter: GetRoomsFilters, options: GetRoomsOptions, appId: string): Promise<Array<IRoomRaw>>; + protected abstract getAllRooms(filters: GetRoomsFilters, options: GetRoomsOptions, appId: string): Promise<Array<IRoomRaw>>;Also applies to: 180-180
📜 Review details
Configuration used: Organization 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)
packages/apps-engine/src/server/bridges/RoomBridge.ts(5 hunks)
🧰 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:
packages/apps-engine/src/server/bridges/RoomBridge.ts
🧠 Learnings (6)
📓 Common learnings
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 by 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 (mapping subscription documents to room IDs), never undefined, even when user has no room subscriptions.
📚 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:
packages/apps-engine/src/server/bridges/RoomBridge.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: 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.
Applied to files:
packages/apps-engine/src/server/bridges/RoomBridge.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: 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.
Applied to files:
packages/apps-engine/src/server/bridges/RoomBridge.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:
packages/apps-engine/src/server/bridges/RoomBridge.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:
packages/apps-engine/src/server/bridges/RoomBridge.ts
🧬 Code graph analysis (1)
packages/apps-engine/src/server/bridges/RoomBridge.ts (3)
packages/apps-engine/src/definition/rooms/index.ts (2)
RoomType(14-14)IRoomRaw(13-13)packages/apps-engine/src/definition/rooms/IRoomRaw.ts (1)
IRoomRaw(10-43)packages/apps-engine/src/server/permissions/AppPermissions.ts (1)
AppPermissions(24-129)
⏰ 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
🔇 Additional comments (2)
packages/apps-engine/src/server/bridges/RoomBridge.ts (2)
18-49: LGTM!The type definitions are well-documented and clearly explain the filter behavior, especially the tri-state logic for
discussionsandteamsfilters.
238-251: LGTM!The permission check method follows the established pattern in this class and correctly references the new
system-view-allpermission.
Dnouv
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
Co-authored-by: Douglas Gubert <douglas.gubert@gmail.com> Co-authored-by: Guilherme Gazzo <guilherme@gazzo.xyz>
Proposed changes (including videos or screenshots)
This pull request introduces a new capability for apps to list and retrieve rooms across the workspace, with flexible filtering options, and adds a lightweight conversion mode for room data to improve performance. The changes affect the Apps Engine bridge, room conversion logic, and the accessors/interfaces that expose these features to apps, along with corresponding tests and type definitions.
Room Listing API and Filtering Enhancements:
getAllRoomsmethod to the Apps Engine bridge and theIRoomReadaccessor, allowing apps to retrieve rooms workspace-wide with optional filters such as type, pagination, discussion-only, and team-main flags. [1] [2] [3] [4]GetRoomsFiltertype to specify filtering options for room listing, and enforced validation for filter parameters (e.g., limit and skip) in the accessor implementation. [1] [2]Room Conversion Optimization:
AppRoomsConverter) with a newlightweightoption, enabling conversion of room data without expensive lookups for improved performance when listing many rooms. [1] [2] [3]Testing and Type Updates:
IRoomRead,IAppRoomsConverter) to support new parameters and conversion options. [1] [2]These changes collectively enable apps to efficiently list and filter rooms across the workspace, with improved performance and robust API contracts.
Issue(s)
Steps to test or reproduce
Room filter logging (click to expand) 🍎
This function applies several room filters (channels, groups, DMs, discussions, pagination, etc.) and logs the matched rooms.
Further comments
RCAI6-58
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.