feat(apps): experimental member room IDs read bridge #37057
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
🦋 Changeset detectedLatest commit: afd4374 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 |
WalkthroughAdds an AppExperimentalBridge that returns room IDs for a user, wires it into RealAppBridges, updates reader tests to include the experimental accessor, and adds a changeset bumping Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as App (Marketplace/In-house)
participant AE as Apps Engine
participant XB as AppExperimentalBridge
participant Orch as Orchestrator
participant Subs as Subscriptions
App->>AE: Request user room IDs (userId)
AE->>XB: getUserRoomIds(userId, appId)
XB->>Orch: debugLog("getUserRoomIds")
XB->>Subs: findByUserId(userId, { projection: { rid: 1 } })
Subs-->>XB: Cursor
XB->>XB: toArray() → map to rid[]
XB-->>AE: roomId[]
AE-->>App: roomId[]
note right of XB #D3E4CD: New experimental bridge path to read memberships
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #37057 +/- ##
===========================================
- Coverage 67.60% 67.60% -0.01%
===========================================
Files 3341 3341
Lines 114016 114016
Branches 20669 20673 +4
===========================================
- Hits 77086 77075 -11
- Misses 34258 34260 +2
- Partials 2672 2681 +9
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: 4
🧹 Nitpick comments (4)
.changeset/chatty-foxes-attend.md (1)
6-6: Call out permission gating in the changeset note.Mention that this API requires AppPermissions.user.read so app authors understand permission needs.
-Adds a user room ID bridge to the apps engine so apps can retrieve members’ room identifiers. +Adds a user room ID bridge to the apps engine so apps can retrieve members’ room identifiers (requires AppPermissions.user.read).packages/apps-engine/tests/test-data/bridges/userBridge.ts (1)
41-43: Prefer a harmless default over throwing in test double.Returning an empty array keeps unrelated tests resilient if this stub gets exercised.
- protected getUserRoomIds(uid: string, appId: string): Promise<Array<string>> { - throw new Error('Method not implemented.'); - } + protected getUserRoomIds(uid: string, appId: string): Promise<Array<string>> { + return Promise.resolve([]); + }apps/meteor/app/apps/server/bridges/users.ts (1)
172-178: Harden result shaping (strip _id; filter falsy rids).Small improvements to projection and mapping safety.
- const subscriptions = await Subscriptions.findByUserId(uid, { projection: { rid: 1 } }).toArray(); - - return subscriptions.map((subscription) => subscription.rid); + const subscriptions = await Subscriptions.findByUserId(uid, { projection: { rid: 1, _id: 0 } }).toArray(); + return subscriptions.map((s) => s.rid).filter(Boolean) as Array<string>;packages/apps-engine/tests/server/accessors/UserRead.spec.ts (1)
34-36: Mock looks good. Consider adding a permission-denied case.Add a test where doGetUserRoomIds resolves to undefined to assert accessor behavior when permission is missing.
📜 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)
.changeset/chatty-foxes-attend.md(1 hunks)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/tests/server/accessors/UserRead.spec.ts(3 hunks)packages/apps-engine/tests/test-data/bridges/userBridge.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/apps-engine/tests/server/accessors/UserRead.spec.ts (1)
packages/apps-engine/tests/test-data/utilities.ts (1)
TestData(178-547)
🔇 Additional comments (1)
packages/apps-engine/src/server/bridges/UserBridge.ts (1)
76-77: Abstract remains non-optional (only called when permitted).No change needed; signature is correct.
Dnouv
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
d-gubert
removed
stat: QA assured
d-gubert
left a comment
d-gubert
left a comment
There was a problem hiding this comment.
I'm wondering about the API. When the app calls the getUserRoomIds it will get the room ids, but is this info enough for the use case? I'm thinking that if the app will always have to getUserRoomIds and then fetch the full room info, maybe we should fetch the whole room objects by default? What do you think?
|
For the target use case, only room IDs are required; no use of the whole room object. |
That's ok for now, then. I'd say if we ever encounter a use case where the room objects are necessary, it is more advantageous to provide a batch reader to avoid N network roundtrips to fetch room objects. But we can cross that bridge when we get there |
Co-authored-by: Debdut Chakraborty <debdut.chakraborty@rocket.chat>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/meteor/app/apps/server/bridges/bridges.js (1)
11-11: Import wiring looks correct; watch Biome’s parse warningImport path is consistent with local module resolution used elsewhere in this file. The Biome “Illegal use of an import declaration outside of a module” hint appears to be a config false positive since this file already uses ESM imports throughout. Consider ensuring Biome treats these files as modules (e.g., set sourceType=module or add a per-path override) to avoid CI noise.
apps/meteor/app/apps/server/bridges/experimental.ts (1)
10-16: Tighten the return type and trim projection for efficiency
- This implementation always returns an array (empty when no subs), so the return type can drop undefined to reflect behavior and the accessor’s contract.
- Exclude _id from the projection to reduce payload for large result sets.
Based on learnings
Apply:
- protected async getUserRoomIds(userId: string, appId: string): Promise<string[] | undefined> { + protected async getUserRoomIds(userId: string, appId: string): Promise<string[]> { this.orch.debugLog(`The App ${appId} is getting the room ids for the user: "${userId}"`); - const subscriptions = await Subscriptions.findByUserId(userId, { projection: { rid: 1 } }).toArray(); + const subscriptions = await Subscriptions.findByUserId(userId, { projection: { rid: 1, _id: 0 } }).toArray(); return subscriptions.map((subscription) => subscription.rid); }
📜 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 (2)
apps/meteor/app/apps/server/bridges/bridges.js(3 hunks)apps/meteor/app/apps/server/bridges/experimental.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: Dnouv
PR: RocketChat/Rocket.Chat#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
PR: RocketChat/Rocket.Chat#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
PR: RocketChat/Rocket.Chat#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/experimental.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
PR: RocketChat/Rocket.Chat#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/experimental.ts
🧬 Code graph analysis (2)
apps/meteor/app/apps/server/bridges/bridges.js (1)
apps/meteor/app/apps/server/bridges/experimental.ts (1)
AppExperimentalBridge(5-17)
apps/meteor/app/apps/server/bridges/experimental.ts (1)
packages/apps-engine/src/server/bridges/index.ts (1)
ExperimentalBridge(49-49)
🪛 Biome (2.1.2)
apps/meteor/app/apps/server/bridges/bridges.js
[error] 10-11: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 11-12: 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 (4)
apps/meteor/app/apps/server/bridges/bridges.js (2)
174-176: Accessor addition LGTMGetter aligns with existing pattern (one getter per private bridge field). No concerns.
63-63: Instantiation LGTM; AppBridges interface already defines getExperimentalBridgeapps/meteor/app/apps/server/bridges/experimental.ts (2)
1-4: Imports are appropriateTypes and models are correctly sourced; aligns with existing server bridge files.
5-9: Constructor DI pattern LGTMExtends ExperimentalBridge and captures orchestrator for debug logging, matching other bridges’ style.
d-gubert
changed the title
|
LGTM! |
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 (1)
.changeset/chatty-foxes-attend.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
.changeset/chatty-foxes-attend.md
[grammar] ~6-~6: Ensure spelling is correct
Context: ...etrieves the ids of rooms the user is a memeber of
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~6-~6: Ensure spelling is correct
Context: ...he ids of rooms the user is a memeber of
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_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)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
d-gubert
force-pushed
the
new/ae/roomIdreadbridge
branch
from
7a2957a to
863fa0a
Compare
d-gubert
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
d-gubert
changed the title
Proposed changes (including videos or screenshots)
getUserRoomIdson the apps-engine user accessor and bridgelogging
Issue(s)
Steps to test or reproduce
Further comments
RCAI6-8
Benchmark:
getUserRoomIds(5 runs per sample)Looks like the first run on larger datasets pays a warm-up/cache cost, then stabilizes in the ~80–90 ms
range for ~24k rooms and ~200 ms for ~70k–100k rooms.
Summary by CodeRabbit
New Features
Tests
Chores