Skip to content

[PERF] Parallelize initial database lookups in Activity Hub #39924

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
abhi02max wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

[PERF] Parallelize initial database lookups in Activity Hub #39924

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 46 additions & 38 deletions apps/meteor/app/api/server/lib/messages.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,29 +5,33 @@ import type { FindOptions } from 'mongodb';
import { canAccessRoomAsync } from '../../../authorization/server/functions/canAccessRoom';

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
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Mar 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

const [room, user] = await Promise.all([
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai bot Mar 28, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
Fix with Cubic

Rooms.findOneById(roomId),
Users.findOneById<Pick<IUser, 'username'>>(uid, { projection: { username: 1 } })
]);
Comment on lines 7 to +25
Copy link

Copilot AI Mar 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copilot uses AI. Check for mistakes.

if (!room || !(await canAccessRoomAsync(room, { _id: uid }))) {
throw new Error('error-not-allowed');
}
Comment on lines +22 to +29
Copy link

Copilot AI Mar 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
if (!user) {
throw new Error('invalid-user');
}

const { cursor, totalCount } = Messages.findPaginatedVisibleByMentionAndRoomId(user.username, roomId, {
const { cursor, totalCount } = Messages.findPaginatedVisibleByMentionAndRoomId(user.username, roomId, {
sort: sort || { ts: -1 },
skip: offset,
limit: count,
Expand All @@ -44,29 +48,33 @@ export async function findMentionedMessages({
}

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 } })
]);
Comment on lines 50 to +68
Copy link

Copilot AI Mar 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.

if (!room || !(await canAccessRoomAsync(room, { _id: uid }))) {
throw new Error('error-not-allowed');
}
Comment on lines +65 to +72
Copy link

Copilot AI Mar 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
if (!user) {
throw new Error('invalid-user');
}

const { cursor, totalCount } = Messages.findStarredByUserAtRoom(uid, roomId, {
const { cursor, totalCount } = Messages.findStarredByUserAtRoom(uid, roomId, {
sort: sort || { ts: -1 },
skip: offset,
limit: count,
Expand Down
Loading