fix: avoid full collection fetch in getAllUserMentionsByChannel#39772
Open
Khizarshah01 wants to merge 1 commit into
Open
fix: avoid full collection fetch in getAllUserMentionsByChannel#39772Khizarshah01 wants to merge 1 commit into
Khizarshah01 wants to merge 1 commit into
Conversation
Contributor
|
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 |
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe mentions endpoint now fetches paginated mentions and the total count in parallel: the handler concurrently calls Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant DB
Client->>Server: GET /mentions?roomId&offset&count
Server->>DB: getUserMentionsByChannel(userId, roomId, {sort, skip, limit})
Server->>DB: countVisibleByMentionAndRoomId(username, roomId)
DB-->>Server: mentions[] (paginated)
DB-->>Server: totalCount (number)
Server-->>Client: { mentions, count, offset, total }
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/models/src/models/Messages.ts (1)
94-102: Reduce future drift by centralizing the mention visibility query.This method is correct, but the same query now exists in three places. Extracting a small helper will keep count/find/paginated behavior locked together.
♻️ Proposed refactor
+ private getVisibleMentionAndRoomQuery(username: IUser['username'], rid: IRoom['_id']): Filter<IMessage> { + return { + _hidden: { $ne: true }, + 'mentions.username': username, + rid, + }; + } + findVisibleByMentionAndRoomId(username: IUser['username'], rid: IRoom['_id'], options?: FindOptions<IMessage>): FindCursor<IMessage> { - const query: Filter<IMessage> = { - '_hidden': { $ne: true }, - 'mentions.username': username, - rid, - }; + const query = this.getVisibleMentionAndRoomQuery(username, rid); return this.find(query, options); } countVisibleByMentionAndRoomId(username: IUser['username'], rid: IRoom['_id']): Promise<number> { - const query: Filter<IMessage> = { - '_hidden': { $ne: true }, - 'mentions.username': username, - rid, - }; + const query = this.getVisibleMentionAndRoomQuery(username, rid); return this.countDocuments(query); } findPaginatedVisibleByMentionAndRoomId( username: IUser['username'], rid: IRoom['_id'], options?: FindOptions<IMessage>, ): FindPaginated<FindCursor<IMessage>> { - const query: Filter<IMessage> = { - '_hidden': { $ne: true }, - 'mentions.username': username, - rid, - }; + const query = this.getVisibleMentionAndRoomQuery(username, rid); return this.findPaginated(query, options); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/models/src/models/Messages.ts` around lines 94 - 102, Extract the repeated mention-visibility filter into a shared helper (e.g., buildVisibleMentionQuery or getMentionVisibilityFilter) and replace the inline query in countVisibleByMentionAndRoomId with a call to that helper; update the other two places that construct the same Filter<IMessage> (the corresponding find/paginated message methods that currently duplicate '_hidden': { $ne: true }, 'mentions.username': username, rid) to use the same helper so all count/find/paginated behavior reuses a single source of truth.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/models/src/models/Messages.ts`:
- Around line 94-102: Extract the repeated mention-visibility filter into a
shared helper (e.g., buildVisibleMentionQuery or getMentionVisibilityFilter) and
replace the inline query in countVisibleByMentionAndRoomId with a call to that
helper; update the other two places that construct the same Filter<IMessage>
(the corresponding find/paginated message methods that currently duplicate
'_hidden': { $ne: true }, 'mentions.username': username, rid) to use the same
helper so all count/find/paginated behavior reuses a single source of truth.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e0f9c579-21da-4cc6-aaa8-b22503b290f1
📒 Files selected for processing (3)
apps/meteor/app/api/server/v1/channels.tspackages/model-typings/src/models/IMessagesModel.tspackages/models/src/models/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). (1)
- GitHub Check: cubic · AI code reviewer
🧰 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/model-typings/src/models/IMessagesModel.tspackages/models/src/models/Messages.tsapps/meteor/app/api/server/v1/channels.ts
🧠 Learnings (8)
📓 Common learnings
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.
📚 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:
packages/model-typings/src/models/IMessagesModel.tspackages/models/src/models/Messages.ts
📚 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:
packages/model-typings/src/models/IMessagesModel.tspackages/models/src/models/Messages.tsapps/meteor/app/api/server/v1/channels.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:
packages/model-typings/src/models/IMessagesModel.tspackages/models/src/models/Messages.tsapps/meteor/app/api/server/v1/channels.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:
packages/model-typings/src/models/IMessagesModel.tspackages/models/src/models/Messages.tsapps/meteor/app/api/server/v1/channels.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/models/src/models/Messages.ts
📚 Learning: 2026-02-23T17:53:06.802Z
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:06.802Z
Learning: During PR reviews that touch endpoint files under apps/meteor/app/api/server/v1, enforce strict scope: if a PR targets a specific endpoint (e.g., rooms.favorite), do not propose changes to unrelated endpoints (e.g., rooms.invite) unless maintainers explicitly request them. Focus feedback on the touched endpoint's behavior, API surface, and related tests; avoid broad cross-endpoint changes in the same PR unless requested.
Applied to files:
apps/meteor/app/api/server/v1/channels.ts
📚 Learning: 2026-02-24T19:09:01.522Z
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:01.522Z
Learning: In Rocket.Chat OpenAPI migration PRs for endpoints under apps/meteor/app/api/server/v1, avoid introducing logic changes. Only perform scope-tight changes that preserve behavior; style-only cleanups (e.g., removing inline comments) may be deferred to follow-ups to keep the migration PR focused.
Applied to files:
apps/meteor/app/api/server/v1/channels.ts
🔇 Additional comments (2)
packages/model-typings/src/models/IMessagesModel.ts (1)
42-42: Interface contract update looks correct.The new
countVisibleByMentionAndRoomIdsignature aligns with the model and endpoint usage and keeps typing consistent.apps/meteor/app/api/server/v1/channels.ts (1)
435-442: Performance improvement is correctly integrated at the endpoint.Using
Promise.allwithgetUserMentionsByChannel(...)andMessages.countVisibleByMentionAndRoomId(...)preserves response behavior while removing the full-fetch count path.Also applies to: 448-448
Khizarshah01
force-pushed
the
fix/mentions-count
branch
from
b58ae45 to
9e68c90
Compare
Khizarshah01
force-pushed
the
fix/mentions-count
branch
from
9e68c90 to
b79c5f0
Compare
Khizarshah01
force-pushed
the
fix/mentions-count
branch
from
b79c5f0 to
57d3827
Compare
Contributor
Author
|
PR is ready for review |
Khizarshah01
force-pushed
the
fix/mentions-count
branch
from
1b3bdf1 to
e1216e4
Compare
Contributor
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/v1/channels.ts`:
- Around line 435-448: The count query runs in parallel before access is
validated—ensure access is checked first by awaiting the access-owning call
(getUserMentionsByChannel or an explicit room/access check) before starting the
second promise; i.e., perform the room/access validation (call
getUserMentionsByChannel for validation or use the existing access check it
performs), then run Promise.all([getUserMentionsByChannel(...),
Messages.countVisibleByMentionAndRoomId(...)]) so both paginated fetch and count
execute in parallel only after validation succeeds.
🪄 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: 60897629-a0aa-4150-925e-343aa34b1c85
📒 Files selected for processing (1)
apps/meteor/app/api/server/v1/channels.ts
📜 Review details
🧰 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/v1/channels.ts
🧠 Learnings (6)
📓 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: smirk-dev
Repo: RocketChat/Rocket.Chat PR: 39625
File: apps/meteor/app/api/server/v1/push.ts:85-97
Timestamp: 2026-03-14T14:58:58.834Z
Learning: In RocketChat/Rocket.Chat, the `push.token` POST/DELETE endpoints in `apps/meteor/app/api/server/v1/push.ts` were already migrated to the chained router API pattern on `develop` prior to PR `#39625`. `cleanTokenResult` (which strips `authToken` and returns `PushTokenResult`) and `isPushTokenPOSTProps`/`isPushTokenDELETEProps` validators already exist on `develop`. PR `#39625` only migrates `push.get` and `push.info` to the chained pattern. Do not flag `cleanTokenResult` or `PushTokenResult` as newly introduced behavior-breaking changes when reviewing this PR.
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: 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.
📚 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/v1/channels.ts
📚 Learning: 2026-02-23T17:53:06.802Z
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:06.802Z
Learning: During PR reviews that touch endpoint files under apps/meteor/app/api/server/v1, enforce strict scope: if a PR targets a specific endpoint (e.g., rooms.favorite), do not propose changes to unrelated endpoints (e.g., rooms.invite) unless maintainers explicitly request them. Focus feedback on the touched endpoint's behavior, API surface, and related tests; avoid broad cross-endpoint changes in the same PR unless requested.
Applied to files:
apps/meteor/app/api/server/v1/channels.ts
📚 Learning: 2026-02-24T19:09:01.522Z
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:01.522Z
Learning: In Rocket.Chat OpenAPI migration PRs for endpoints under apps/meteor/app/api/server/v1, avoid introducing logic changes. Only perform scope-tight changes that preserve behavior; style-only cleanups (e.g., removing inline comments) may be deferred to follow-ups to keep the migration PR focused.
Applied to files:
apps/meteor/app/api/server/v1/channels.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/v1/channels.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/v1/channels.ts
Comment on lines
+435
to
+448
| const [mentions, total] = await Promise.all([ | ||
| getUserMentionsByChannel(this.userId, roomId, { | ||
| sort: sort || { ts: 1 }, | ||
| skip: offset, | ||
| limit: count, | ||
| }), | ||
| Messages.countVisibleByMentionAndRoomId(this.user.username, roomId), | ||
| ]); | ||
|
|
||
| return API.v1.success({ | ||
| mentions, | ||
| count: mentions.length, | ||
| offset, | ||
| total: allMentions.length, | ||
| total, |
Contributor
There was a problem hiding this comment.
Keep the count query behind the same access check.
getUserMentionsByChannel(...) currently owns the room/access validation, but Messages.countVisibleByMentionAndRoomId(...) now starts before that guard. With Promise.all, invalid or unauthorized requests still hit Messages because the count promise is already in flight. Please validate access first, then run the paginated fetch and count in parallel.
Possible fix while preserving the parallelism
+ const room = await Rooms.findOneById(roomId);
+ if (!room || !(await canAccessRoomAsync(room, this.user))) {
+ throw new Meteor.Error('error-invalid-room', 'Invalid room', {
+ method: 'getUserMentionsByChannel',
+ });
+ }
+
const [mentions, total] = await Promise.all([
- getUserMentionsByChannel(this.userId, roomId, {
+ Messages.findVisibleByMentionAndRoomId(this.user.username, roomId, {
sort: sort || { ts: 1 },
skip: offset,
limit: count,
- }),
+ }).toArray(),
Messages.countVisibleByMentionAndRoomId(this.user.username, roomId),
]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/app/api/server/v1/channels.ts` around lines 435 - 448, The count
query runs in parallel before access is validated—ensure access is checked first
by awaiting the access-owning call (getUserMentionsByChannel or an explicit
room/access check) before starting the second promise; i.e., perform the
room/access validation (call getUserMentionsByChannel for validation or use the
existing access check it performs), then run
Promise.all([getUserMentionsByChannel(...),
Messages.countVisibleByMentionAndRoomId(...)]) so both paginated fetch and count
execute in parallel only after validation succeeds.
Khizarshah01
force-pushed
the
fix/mentions-count
branch
from
e1216e4 to
d7579da
Compare
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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)
The
channels.getAllUserMentionsByChannelendpoint was callinggetUserMentionsByChanneltwice, once for paginated results and once with empty options{}to computetotalforlength. The second call fetches all matching documents into memory just to count them.This PR replaces the second call with a new
countVisibleByMentionAndRoomIdmethod on the Messages model. Both calls now run in parallel viaPromise.all.This follows the existing pattern already used throughout the codebase like
countVisibleByRoomIdBetweenTimestampsInclusive,countPinned,countStarredin Messages.tsNo behavior change
Further comments
Before: 2 sequential DB calls, second fetches all documents into memory
After: 2 parallel DB calls, second uses countDocuments (returns integer only)
Summary by CodeRabbit