[PERF] Parallelize initial database lookups in Activity Hub#39924
[PERF] Parallelize initial database lookups in Activity Hub#39924abhi02max wants to merge 1 commit intoRocketChat:developfrom
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 |
|
|
|
WalkthroughTwo functions in the messages library API— Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR aims to reduce latency in Activity Hub message endpoints by parallelizing independent initial database lookups (Room + User) using Promise.all, instead of performing them sequentially before running the access check.
Changes:
- Parallelize
Rooms.findOneById(roomId)andUsers.findOneById(uid)infindMentionedMessages. - Parallelize the same lookups in
findStarredMessages. - Minor refactor/formatting changes around these functions (including newly added inline comments).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const [room, user] = await Promise.all([ | ||
| Rooms.findOneById(roomId), | ||
| Users.findOneById<Pick<IUser, 'username'>>(uid, { projection: { username: 1 } }) | ||
| ]); | ||
|
|
||
| if (!room || !(await canAccessRoomAsync(room, { _id: uid }))) { | ||
| throw new Error('error-not-allowed'); | ||
| } |
There was a problem hiding this comment.
Same Promise.all tradeoff here: the user document is fetched even when the room is missing/inaccessible, increasing DB work on invalid/unauthorized requests and potentially altering error behavior under transient DB failures. Consider gating the user lookup behind a successful room fetch (and optionally parallelizing it with the room access check) to retain fast-fail behavior.
| export async function findMentionedMessages({ | ||
| uid, | ||
| roomId, | ||
| pagination: { offset, count, sort }, | ||
| uid, | ||
| roomId, | ||
| pagination: { offset, count, sort }, | ||
| }: { | ||
| uid: string; | ||
| roomId: string; | ||
| pagination: { offset: number; count: number; sort: FindOptions<IMessage>['sort'] }; | ||
| uid: string; | ||
| roomId: string; | ||
| pagination: { offset: number; count: number; sort: FindOptions<IMessage>['sort'] }; | ||
| }): Promise<{ | ||
| messages: IMessage[]; | ||
| count: number; | ||
| offset: number; | ||
| total: number; | ||
| messages: IMessage[]; | ||
| count: number; | ||
| offset: number; | ||
| total: number; | ||
| }> { | ||
| const room = await Rooms.findOneById(roomId); | ||
| if (!room || !(await canAccessRoomAsync(room, { _id: uid }))) { | ||
| throw new Error('error-not-allowed'); | ||
| } | ||
| const user = await Users.findOneById<Pick<IUser, 'username'>>(uid, { projection: { username: 1 } }); | ||
| if (!user) { | ||
| throw new Error('invalid-user'); | ||
| } | ||
| // THE STRIKE: Fetch Room and User concurrently | ||
| const [room, user] = await Promise.all([ | ||
| Rooms.findOneById(roomId), | ||
| Users.findOneById<Pick<IUser, 'username'>>(uid, { projection: { username: 1 } }) | ||
| ]); |
There was a problem hiding this comment.
Indentation in this function was changed to spaces, but the rest of this file uses tabs (e.g., the query options and return block below). This mixed indentation is likely to fail formatting/linting and makes diffs noisier; please reformat the new/changed block to match the existing tab-based indentation in this file (or run the repo formatter).
| const [room, user] = await Promise.all([ | ||
| Rooms.findOneById(roomId), | ||
| Users.findOneById<Pick<IUser, 'username'>>(uid, { projection: { username: 1 } }) | ||
| ]); | ||
|
|
||
| if (!room || !(await canAccessRoomAsync(room, { _id: uid }))) { | ||
| throw new Error('error-not-allowed'); | ||
| } |
There was a problem hiding this comment.
Using Promise.all here forces a user lookup even when the room doesn’t exist or the caller can’t access it. That increases DB load for unauthorized/invalid room requests and can also change failure behavior (e.g., a transient Users query error would now fail the request even if the room is missing). Consider fetching the room first, then parallelizing the access check with the user lookup (or otherwise gating the user query) to keep the fast-fail behavior while still getting concurrency benefits.
| export async function findStarredMessages({ | ||
| uid, | ||
| roomId, | ||
| pagination: { offset, count, sort }, | ||
| uid, | ||
| roomId, | ||
| pagination: { offset, count, sort }, | ||
| }: { | ||
| uid: string; | ||
| roomId: string; | ||
| pagination: { offset: number; count: number; sort: FindOptions<IMessage>['sort'] }; | ||
| uid: string; | ||
| roomId: string; | ||
| pagination: { offset: number; count: number; sort: FindOptions<IMessage>['sort'] }; | ||
| }): Promise<{ | ||
| messages: IMessage[]; | ||
| count: number; | ||
| offset: number; | ||
| total: number; | ||
| messages: IMessage[]; | ||
| count: number; | ||
| offset: number; | ||
| total: number; | ||
| }> { | ||
| const room = await Rooms.findOneById(roomId); | ||
| if (!room || !(await canAccessRoomAsync(room, { _id: uid }))) { | ||
| throw new Error('error-not-allowed'); | ||
| } | ||
| const user = await Users.findOneById<Pick<IUser, 'username'>>(uid, { projection: { username: 1 } }); | ||
| if (!user) { | ||
| throw new Error('invalid-user'); | ||
| } | ||
| // THE STRIKE: Fetch Room and User concurrently | ||
| const [room, user] = await Promise.all([ | ||
| Rooms.findOneById(roomId), | ||
| Users.findOneById<Pick<IUser, 'username'>>(uid, { projection: { username: 1 } }) | ||
| ]); |
There was a problem hiding this comment.
Same as above: this function’s parameters/body were re-indented with spaces while the rest of the file uses tabs. Please reformat this block to match the surrounding style to avoid mixed indentation and noisy diffs.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/meteor/app/api/server/lib/messages.ts">
<violation number="1" location="apps/meteor/app/api/server/lib/messages.ts:22">
P2: Parallelizing room and user lookups removes the prior auth gate, causing unnecessary user reads on denied paths and allowing user-query failures to preempt the intended authorization error flow.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| throw new Error('invalid-user'); | ||
| } | ||
| // THE STRIKE: Fetch Room and User concurrently | ||
| const [room, user] = await Promise.all([ |
There was a problem hiding this comment.
P2: Parallelizing room and user lookups removes the prior auth gate, causing unnecessary user reads on denied paths and allowing user-query failures to preempt the intended authorization error flow.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/app/api/server/lib/messages.ts, line 22:
<comment>Parallelizing room and user lookups removes the prior auth gate, causing unnecessary user reads on denied paths and allowing user-query failures to preempt the intended authorization error flow.</comment>
<file context>
@@ -5,29 +5,33 @@ import type { FindOptions } from 'mongodb';
- throw new Error('invalid-user');
- }
+ // THE STRIKE: Fetch Room and User concurrently
+ const [room, user] = await Promise.all([
+ Rooms.findOneById(roomId),
+ Users.findOneById<Pick<IUser, 'username'>>(uid, { projection: { username: 1 } })
</file context>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/app/api/server/lib/messages.ts`:
- Line 21: Remove the inline implementation comments "THE STRIKE: Fetch Room and
User concurrently" (and the similar comment at the other location) from the
production code in apps/meteor/app/api/server/lib/messages.ts; locate the
comment text to find where it's inserted (around the code that fetches Room and
User) and delete those lines, leaving only the actual implementation and any
necessary function/class identifiers intact (e.g., the surrounding function
handling message fetch/processing). Ensure no other inline implementation
comments remain in that file.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 76581c47-114f-4b35-a0f5-8a378652fb5d
📒 Files selected for processing (1)
apps/meteor/app/api/server/lib/messages.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Agent
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/app/api/server/lib/messages.ts
🧠 Learnings (9)
📓 Common learnings
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:18.785Z
Learning: In Rocket.Chat PR reviews, maintain strict scope boundaries—when a PR is focused on a specific endpoint (e.g., rooms.favorite), avoid reviewing or suggesting changes to other endpoints that were incidentally refactored (e.g., rooms.invite) unless explicitly requested by maintainers.
Learnt from: Harxhit
Repo: RocketChat/Rocket.Chat PR: 39476
File: apps/meteor/server/methods/addAllUserToRoom.ts:0-0
Timestamp: 2026-03-09T18:39:21.178Z
Learning: In apps/meteor/server/methods/addAllUserToRoom.ts, the implementation uses a single cursor pass (Users.find(userFilter).batchSize(100)) that collects both the full user objects (collectedUsers: IUser[]) and their usernames (usernames: string[]) in one iteration. `beforeAddUserToRoom` is then called once with the full usernames batch (preserving batch-validation semantics), and the subsequent subscription/message processing loop iterates over the same stable `collectedUsers` array — no second DB query is made. This avoids any race condition between validation and processing while preserving the original batch-validation behavior.
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 39545
File: apps/meteor/client/views/room/body/hooks/useHasNewMessages.ts:59-61
Timestamp: 2026-03-11T22:04:20.529Z
Learning: In `apps/meteor/client/views/room/body/hooks/useHasNewMessages.ts`, the `msg.u._id === uid` early-return in the `streamNewMessage` handler is intentional: the "New messages" indicator is designed to notify about messages from other users only. Self-sent messages — including those sent from a different session/device — are always skipped, by design. Do not flag this as a multi-session regression.
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: 2026-03-09T18:39:21.178Z
Learnt from: Harxhit
Repo: RocketChat/Rocket.Chat PR: 39476
File: apps/meteor/server/methods/addAllUserToRoom.ts:0-0
Timestamp: 2026-03-09T18:39:21.178Z
Learning: In apps/meteor/server/methods/addAllUserToRoom.ts, the implementation uses a single cursor pass (Users.find(userFilter).batchSize(100)) that collects both the full user objects (collectedUsers: IUser[]) and their usernames (usernames: string[]) in one iteration. `beforeAddUserToRoom` is then called once with the full usernames batch (preserving batch-validation semantics), and the subsequent subscription/message processing loop iterates over the same stable `collectedUsers` array — no second DB query is made. This avoids any race condition between validation and processing while preserving the original batch-validation behavior.
Applied to files:
apps/meteor/app/api/server/lib/messages.ts
📚 Learning: 2026-03-11T22:04:20.529Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 39545
File: apps/meteor/client/views/room/body/hooks/useHasNewMessages.ts:59-61
Timestamp: 2026-03-11T22:04:20.529Z
Learning: In `apps/meteor/client/views/room/body/hooks/useHasNewMessages.ts`, the `msg.u._id === uid` early-return in the `streamNewMessage` handler is intentional: the "New messages" indicator is designed to notify about messages from other users only. Self-sent messages — including those sent from a different session/device — are always skipped, by design. Do not flag this as a multi-session regression.
Applied to files:
apps/meteor/app/api/server/lib/messages.ts
📚 Learning: 2026-03-12T10:26:26.697Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 39340
File: apps/meteor/app/api/server/v1/im.ts:1349-1398
Timestamp: 2026-03-12T10:26:26.697Z
Learning: In `apps/meteor/app/api/server/v1/im.ts` (PR `#39340`), the `DmEndpoints` type intentionally includes temporary stub entries for `/v1/im.kick`, `/v1/dm.kick`, `/v1/im.leave`, and `/v1/dm.leave` (using `DmKickProps` and `DmLeaveProps`) even though no route handlers exist for them yet. These stubs were added to preserve type compatibility after removing the original `DmLeaveProps` and related files. They are planned for cleanup in a follow-up PR. Do not flag these as missing implementations when reviewing this file until the follow-up is merged.
Applied to files:
apps/meteor/app/api/server/lib/messages.ts
📚 Learning: 2026-01-17T01:51:47.764Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38219
File: packages/core-typings/src/cloud/Announcement.ts:5-6
Timestamp: 2026-01-17T01:51:47.764Z
Learning: In packages/core-typings/src/cloud/Announcement.ts, the AnnouncementSchema.createdBy field intentionally overrides IBannerSchema.createdBy (object with _id and optional username) with a string enum ['cloud', 'system'] to match existing runtime behavior. This is documented as technical debt with a FIXME comment at apps/meteor/app/cloud/server/functions/syncWorkspace/handleCommsSync.ts:53 and should not be flagged as an error until the runtime behavior is corrected.
Applied to files:
apps/meteor/app/api/server/lib/messages.ts
📚 Learning: 2026-02-24T19:09:09.561Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
Applied to files:
apps/meteor/app/api/server/lib/messages.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/api/server/lib/messages.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/app/api/server/lib/messages.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/app/api/server/lib/messages.ts
🔇 Additional comments (1)
apps/meteor/app/api/server/lib/messages.ts (1)
22-32: Good perf refactor with preserved auth/error behavior.Nice change: the parallel prefetch keeps the same authorization and invalid-user checks while removing unnecessary sequential wait time.
Also applies to: 65-75
| if (!user) { | ||
| throw new Error('invalid-user'); | ||
| } | ||
| // THE STRIKE: Fetch Room and User concurrently |
There was a problem hiding this comment.
Remove inline implementation comments added in production code.
Line 21 and Line 64 add implementation comments in code paths where the repo guideline asks to avoid them.
✂️ Minimal cleanup
- // THE STRIKE: Fetch Room and User concurrently
const [room, user] = await Promise.all([
Rooms.findOneById(roomId),
Users.findOneById<Pick<IUser, 'username'>>(uid, { projection: { username: 1 } })
]);
...
- // THE STRIKE: Fetch Room and User concurrently
const [room, user] = await Promise.all([
Rooms.findOneById(roomId),
Users.findOneById<Pick<IUser, 'username'>>(uid, { projection: { username: 1 } })
]);As per coding guidelines "Avoid code comments in the implementation".
Also applies to: 64-64
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/app/api/server/lib/messages.ts` at line 21, Remove the inline
implementation comments "THE STRIKE: Fetch Room and User concurrently" (and the
similar comment at the other location) from the production code in
apps/meteor/app/api/server/lib/messages.ts; locate the comment text to find
where it's inserted (around the code that fetches Room and User) and delete
those lines, leaving only the actual implementation and any necessary
function/class identifiers intact (e.g., the surrounding function handling
message fetch/processing). Ensure no other inline implementation comments remain
in that file.
While auditing the Activity Hub endpoints (findMentionedMessages and findStarredMessages), I identified a sequential database lookup bottleneck.
Previously, the system would fetch the Room profile, wait for the network/database response, run the access check, and then fire a completely independent query to fetch the User profile.
This PR refactors these initial authorization lookups to execute concurrently using Promise.all. Since the User lookup does not depend on the Room lookup, parallelizing them eliminates unnecessary idle time on the Node.js event loop and directly reduces the overall API latency, especially under heavy database loads.
Type of change
[x] Improvement (non-breaking change that improves a feature, code style, or performance)
Summary by CodeRabbit