feat(apps): graduate getUserRoomIds experiment to the user bridge #37547
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
|
Looks like this PR is ready to merge! 🎉 |
🦋 Changeset detectedLatest commit: da87b79 The changes in this PR will be included in the next version bump. This PR includes changesets to release 42 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
WalkthroughThe PR graduates getUserRoomIds from the experimental API into the stable user bridge: it removes the experimental implementation and permission, adds the method to IUserRead/UserBridge/UserRead, updates Meteor bridge implementations, tests, i18n, and a changeset. (≤50 words) Changes
Sequence Diagram(s)sequenceDiagram
participant App as App
participant UserRead as UserRead (Accessor)
participant UserBridge as UserBridge
participant DB as Database
App->>UserRead: getUserRoomIds(userId)
activate UserRead
UserRead->>UserBridge: doGetUserRoomIds(userId, appId)
activate UserBridge
Note right of UserBridge `#e6f7ff`: permission check (hasReadPermission)
alt has read permission
UserBridge->>UserBridge: getUserRoomIds(userId, appId)
UserBridge->>DB: query Subscriptions by userId (project rid)
DB-->>UserBridge: list of rids
UserBridge-->>UserRead: Promise<string[]>
else no permission
UserBridge-->>UserRead: undefined
end
deactivate UserBridge
deactivate UserRead
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
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 |
Contributor
📦 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 21:05", "11/19 21:39 (PR)"]
y-axis "Size (GB)" 0 --> 0.5
line "account-service" [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]
line "ddp-streamer-service" [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.14]
line "presence-service" [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.14]
line "rocketchat" [0.36, 0.36, 0.35, 0.35, 0.35, 0.36]
line "stream-hub-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
Statistics (last 5 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Wed, 19 Nov 2025 21:39:58 GMT |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/apps-engine/src/definition/accessors/IExperimentalRead.ts (1)
8-9: Consider removing the empty interface or converting to a type alias.The
IExperimentalReadinterface is now empty after thegetUserRoomIdsmigration. If this interface serves no purpose, consider removing it entirely in a future major version. Alternatively, if it must be retained for backward compatibility, follow the static analysis suggestion to convert it to a type alias:export type IExperimentalRead = {}.packages/apps-engine/src/server/accessors/ExperimentalRead.ts (1)
6-7: Constructor visibility changed to protected.The constructor parameters have been changed from
privatetoprotected readonly, allowing subclasses to access these fields. While this expands the subclass API surface slightly, it's a safe change that may support future extensibility.apps/meteor/app/apps/server/bridges/experimental.ts (1)
5-5: Constructor visibility changed to protected.Similar to ExperimentalRead, the constructor parameter has been changed from
private readonlytoprotected readonly. This is a consistent pattern across the experimental bridge implementations and allows for future extensibility.
📜 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 (13)
.changeset/calm-falcons-gather.md(1 hunks)apps/meteor/app/apps/server/bridges/experimental.ts(1 hunks)apps/meteor/app/apps/server/bridges/users.ts(1 hunks)packages/apps-engine/src/definition/accessors/IExperimentalRead.ts(1 hunks)packages/apps-engine/src/definition/accessors/IUserRead.ts(1 hunks)packages/apps-engine/src/server/accessors/ExperimentalRead.ts(1 hunks)packages/apps-engine/src/server/accessors/UserRead.ts(1 hunks)packages/apps-engine/src/server/bridges/ExperimentalBridge.ts(1 hunks)packages/apps-engine/src/server/bridges/UserBridge.ts(2 hunks)packages/apps-engine/src/server/permissions/AppPermissions.ts(0 hunks)packages/apps-engine/tests/server/accessors/UserRead.spec.ts(3 hunks)packages/apps-engine/tests/test-data/bridges/experimentalBridge.ts(1 hunks)packages/apps-engine/tests/test-data/bridges/userBridge.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/apps-engine/src/server/permissions/AppPermissions.ts
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: UserBridge.doGetUserRoomIds in packages/apps-engine/src/server/bridges/UserBridge.ts has a bug where it implicitly returns undefined when the app lacks read permission (missing return statement in the else case of the permission check).
Learnt from: 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.
📚 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/definition/accessors/IUserRead.tsapps/meteor/app/apps/server/bridges/users.tspackages/apps-engine/tests/test-data/bridges/experimentalBridge.tspackages/apps-engine/tests/test-data/bridges/userBridge.tspackages/apps-engine/src/server/accessors/ExperimentalRead.tspackages/apps-engine/tests/server/accessors/UserRead.spec.tspackages/apps-engine/src/server/accessors/UserRead.ts.changeset/calm-falcons-gather.mdapps/meteor/app/apps/server/bridges/experimental.tspackages/apps-engine/src/server/bridges/UserBridge.tspackages/apps-engine/src/server/bridges/ExperimentalBridge.tspackages/apps-engine/src/definition/accessors/IExperimentalRead.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/definition/accessors/IUserRead.tsapps/meteor/app/apps/server/bridges/users.tspackages/apps-engine/tests/test-data/bridges/experimentalBridge.tspackages/apps-engine/tests/test-data/bridges/userBridge.tspackages/apps-engine/src/server/accessors/ExperimentalRead.tspackages/apps-engine/tests/server/accessors/UserRead.spec.tspackages/apps-engine/src/server/accessors/UserRead.ts.changeset/calm-falcons-gather.mdapps/meteor/app/apps/server/bridges/experimental.tspackages/apps-engine/src/server/bridges/UserBridge.tspackages/apps-engine/src/server/bridges/ExperimentalBridge.tspackages/apps-engine/src/definition/accessors/IExperimentalRead.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/accessors/IUserRead.tsapps/meteor/app/apps/server/bridges/users.tspackages/apps-engine/tests/test-data/bridges/experimentalBridge.tspackages/apps-engine/tests/test-data/bridges/userBridge.tspackages/apps-engine/src/server/accessors/ExperimentalRead.tspackages/apps-engine/tests/server/accessors/UserRead.spec.tspackages/apps-engine/src/server/accessors/UserRead.ts.changeset/calm-falcons-gather.mdapps/meteor/app/apps/server/bridges/experimental.tspackages/apps-engine/src/server/bridges/UserBridge.tspackages/apps-engine/src/server/bridges/ExperimentalBridge.tspackages/apps-engine/src/definition/accessors/IExperimentalRead.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/definition/accessors/IUserRead.tsapps/meteor/app/apps/server/bridges/users.tspackages/apps-engine/tests/test-data/bridges/userBridge.tspackages/apps-engine/tests/server/accessors/UserRead.spec.tspackages/apps-engine/src/server/accessors/UserRead.ts.changeset/calm-falcons-gather.mdapps/meteor/app/apps/server/bridges/experimental.tspackages/apps-engine/src/server/bridges/UserBridge.ts
📚 Learning: 2025-11-04T16:49:19.107Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37377
File: apps/meteor/ee/server/hooks/federation/index.ts:86-88
Timestamp: 2025-11-04T16:49:19.107Z
Learning: In Rocket.Chat's federation system (apps/meteor/ee/server/hooks/federation/), permission checks follow two distinct patterns: (1) User-initiated federation actions (creating rooms, adding users to federated rooms, joining from invites) should throw MeteorError to inform users they lack 'access-federation' permission. (2) Remote server-initiated federation events should silently skip/ignore when users lack permission. The beforeAddUserToRoom hook only executes for local user-initiated actions, so throwing an error there is correct. Remote federation events are handled separately by the federation Matrix package with silent skipping logic.
Applied to files:
apps/meteor/app/apps/server/bridges/users.ts.changeset/calm-falcons-gather.md
📚 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/experimentalBridge.tspackages/apps-engine/tests/test-data/bridges/userBridge.ts
📚 Learning: 2025-09-19T15:15:04.642Z
Learnt from: rodrigok
Repo: RocketChat/Rocket.Chat PR: 36991
File: apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts:219-221
Timestamp: 2025-09-19T15:15:04.642Z
Learning: The Federation_Matrix_homeserver_domain setting in apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts is part of the old federation system and is being deprecated/removed, so configuration issues with this setting should not be flagged for improvement.
Applied to files:
.changeset/calm-falcons-gather.mdapps/meteor/app/apps/server/bridges/experimental.ts
📚 Learning: 2025-09-15T06:21:00.139Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 36868
File: apps/meteor/app/apps/server/bridges/serverEndpoints.ts:35-48
Timestamp: 2025-09-15T06:21:00.139Z
Learning: In ServerEndpointsBridge.ts, the permission model distinguishes between token pass-through and true impersonation: `server-endpoints.call` is required for all endpoint access, while `server-endpoints.impersonate` is only required when `info.user.id` is provided without `info.user.token` (lines 48-53), meaning the bridge needs to mint a token. When both user ID and token are provided, it's considered legitimate credential usage, not impersonation.
Applied to files:
packages/apps-engine/src/server/bridges/UserBridge.tspackages/apps-engine/src/server/bridges/ExperimentalBridge.ts
🧬 Code graph analysis (2)
packages/apps-engine/tests/test-data/bridges/experimentalBridge.ts (1)
packages/apps-engine/src/server/bridges/index.ts (1)
ExperimentalBridge(49-49)
packages/apps-engine/src/server/bridges/ExperimentalBridge.ts (1)
packages/apps-engine/src/server/bridges/index.ts (1)
ExperimentalBridge(49-49)
🪛 Biome (2.1.2)
packages/apps-engine/src/definition/accessors/IExperimentalRead.ts
[error] 9-9: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
🔇 Additional comments (11)
.changeset/calm-falcons-gather.md (1)
1-6: LGTM! Clear documentation of the API graduation.The changeset appropriately documents the graduation of
getUserRoomIdsfrom experimental to stable with correct minor version bumps for both affected packages.packages/apps-engine/src/definition/accessors/IUserRead.ts (1)
23-28: LGTM! Clean interface addition with proper documentation.The
getUserRoomIdsmethod is correctly added to theIUserReadinterface with appropriate JSDoc and a return type that accounts for permission-denied scenarios.packages/apps-engine/src/server/accessors/UserRead.ts (1)
27-29: LGTM! Consistent delegation pattern.The implementation follows the established pattern of delegating to the bridge layer, maintaining consistency with other methods in the
UserReadclass.packages/apps-engine/tests/test-data/bridges/userBridge.ts (1)
41-43: LGTM! Consistent test scaffolding.The stub method follows the established pattern for test bridges, where methods throw "Method not implemented." and are stubbed using
SpyOnin actual tests.packages/apps-engine/tests/test-data/bridges/experimentalBridge.ts (1)
3-3: LGTM! Correct removal of experimental method override.The removal of the
getUserRoomIdsoverride aligns with the broader migration away from the experimental bridge, simplifying the test bridge to rely solely on the baseExperimentalBridgeimplementation.packages/apps-engine/src/server/bridges/UserBridge.ts (1)
76-76: Clarify intent: does the abstract method signature represent the implementation contract or the wrapper contract?The abstract method signature declares
Promise<string[] | undefined>, but the concrete implementationAppUserBridge.getUserRoomIds(apps/meteor/app/apps/server/bridges/users.ts:172-178) always returns astring[]array—neverundefined. Theundefinedreturn actually comes from the public wrapperdoGetUserRoomIds(line 48-52), which implicitly returnsundefinedwhen permission checks fail.Two possible directions:
- Narrow the abstract signature to
Promise<string[]>if implementations should never returnundefined(wrapper handles permission denial)- Keep the current signature if subclasses are intended to return
undefinedin error cases or other scenarios beyond the current implementationpackages/apps-engine/src/server/bridges/ExperimentalBridge.ts (1)
10-10: LGTM! Clean graduation of experimental feature.The
getUserRoomIdsmethod has been successfully removed from the experimental bridge, leaving a clean placeholder for future experimental features.packages/apps-engine/tests/server/accessors/UserRead.spec.ts (3)
15-15: LGTM! Proper test fixture setup.The
roomIdsfixture is correctly initialized with test data for the graduatedgetUserRoomIdsfunctionality.Also applies to: 20-20
33-35: LGTM! Mock implementation added for graduated method.The
doGetUserRoomIdsmock properly returns the test fixture data, enabling verification of the graduated API.
54-54: LGTM! Test coverage added for getUserRoomIds.The test properly verifies that
getUserRoomIdsreturns the expected room IDs through the UserRead accessor.apps/meteor/app/apps/server/bridges/experimental.ts (1)
1-8: Fix missing explicit return statement in UserBridge.doGetUserRoomIds permission-denied case.The bug from previous learnings persists in the stable implementation. In
packages/apps-engine/src/server/bridges/UserBridge.ts, thedoGetUserRoomIdsmethod (lines showing permission check only) implicitly returnsundefinedwhenhasReadPermission(appId)is false—there's no explicit return or else clause. Add an explicitreturn undefined;orreturn [];in the else case to make the permission-denied behavior clear.⛔ Skipped due to learnings
Learnt from: Dnouv Repo: RocketChat/Rocket.Chat PR: 37057 File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27 Timestamp: 2025-09-25T09:59:26.461Z Learning: UserBridge.doGetUserRoomIds in packages/apps-engine/src/server/bridges/UserBridge.ts has a bug where it implicitly returns undefined when the app lacks read permission (missing return statement in the else case of the permission check).Learnt from: 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: 36868 File: apps/meteor/app/apps/server/bridges/serverEndpoints.ts:35-48 Timestamp: 2025-09-15T06:21:00.139Z Learning: In ServerEndpointsBridge.ts, the impersonation permission (`server-endpoints.impersonate`) is enforced at lines 48-49 specifically when `info.user` is provided without `info.user.token`, which represents the impersonation scenario where a token needs to be minted. Token pass-through (when both user ID and token are provided) doesn't require the impersonate permission since the app already possesses a valid token.
d-gubert
requested changes
Nov 19, 2025
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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 (7)
apps/meteor/app/apps/server/bridges/users.ts(1 hunks)packages/apps-engine/src/definition/accessors/IUserRead.ts(1 hunks)packages/apps-engine/src/server/accessors/UserRead.ts(1 hunks)packages/apps-engine/src/server/bridges/UserBridge.ts(2 hunks)packages/apps-engine/src/server/permissions/AppPermissions.ts(1 hunks)packages/apps-engine/tests/test-data/bridges/userBridge.ts(1 hunks)packages/i18n/src/locales/en.i18n.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/apps-engine/src/server/accessors/UserRead.ts
- packages/apps-engine/tests/test-data/bridges/userBridge.ts
- packages/apps-engine/src/server/bridges/UserBridge.ts
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: UserBridge.doGetUserRoomIds in packages/apps-engine/src/server/bridges/UserBridge.ts has a bug where it implicitly returns undefined when the app lacks read permission (missing return statement in the else case of the permission check).
Learnt from: 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.
📚 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/permissions/AppPermissions.tspackages/apps-engine/src/definition/accessors/IUserRead.tsapps/meteor/app/apps/server/bridges/users.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/definition/accessors/IUserRead.tsapps/meteor/app/apps/server/bridges/users.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/definition/accessors/IUserRead.tsapps/meteor/app/apps/server/bridges/users.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/definition/accessors/IUserRead.tsapps/meteor/app/apps/server/bridges/users.ts
📚 Learning: 2025-10-06T20:32:23.658Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37152
File: packages/apps-engine/tests/test-data/utilities.ts:557-573
Timestamp: 2025-10-06T20:32:23.658Z
Learning: In packages/apps-engine/tests/test-data/utilities.ts, the field name `isSubscripbedViaBundle` in the `IMarketplaceSubscriptionInfo` type should not be flagged as a typo, as it may match the upstream API's field name.
Applied to files:
apps/meteor/app/apps/server/bridges/users.ts
⏰ 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)
packages/apps-engine/src/definition/accessors/IUserRead.ts (1)
23-28: LGTM! Interface definition is consistent with implementation.The return type
Promise<string[]>correctly reflects that this method always returns an array (empty when the user has no subscriptions), which is consistent with the implementation inapps/meteor/app/apps/server/bridges/users.ts.Based on learnings
apps/meteor/app/apps/server/bridges/users.ts (1)
172-178: LGTM! Implementation is correct and return type consistency issue resolved.The implementation correctly:
- Queries subscriptions with minimal projection for efficiency
- Returns an array of room IDs (empty array when user has no subscriptions)
- Uses the non-optional return type
Promise<string[]>, which is now consistent with the interface definition inIUserRead.ts(line 28)The return type mismatch flagged in previous reviews has been addressed.
Based on learnings
packages/apps-engine/src/server/permissions/AppPermissions.ts (1)
125-127: Good consolidation following past feedback.The change from a specific
getUserRoomIdspermission to a genericexperimental.defaultpermission aligns with d-gubert's previous suggestion to keep the experimental scope for future APIs. This approach allows the experimental permission to be reused as new experimental features are added. The corresponding i18n keyApps_Permissions_experimental_defaultis confirmed present in the locale files.
d-gubert
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #37547 +/- ##
===========================================
- Coverage 68.97% 68.95% -0.02%
===========================================
Files 3359 3359
Lines 114214 114177 -37
Branches 20535 20533 -2
===========================================
- Hits 78784 78736 -48
- Misses 33335 33351 +16
+ Partials 2095 2090 -5
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
d-gubert
approved these changes
Nov 19, 2025
d-gubert
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Proposed changes (including videos or screenshots)
Graduate the
getUserRoomIdsaccessor from the experimental bridge into the stable user bridge so apps can rely on a non-experimental API, guarded by the existinguser.readpermission. The experimental rollout proved stable in testing, so we want to remove the experimental flag, expose the method through the standard accessor, and make the implementation authoritative without the metrics wrapper.getUserRoomIdsto IUserRead/UserRead and routed it through the user bridge with the regular read permission.Issue(s)
Steps to test or reproduce
Further comments
RCAI6-45
Summary by CodeRabbit
New Features
Chores
Localization
Tests
✏️ Tip: You can customize this high-level summary in your review settings.