fix: correct mention total count calculation (#38908)#38912
fix: correct mention total count calculation (#38908)#38912Amanjha112113 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 |
|
|
|
WalkthroughThe change refactors the channels.getAllUserMentionsByChannel endpoint to resolve a performance regression. Previously, the endpoint called getUserMentionsByChannel twice per request—once for paginated results and again without pagination to compute total count. The new implementation uses a single paginated query method that returns both data and total count efficiently. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/meteor/app/api/server/v1/channels.ts (1)
439-446: Avoid the extraUsers.findOneByIdround-trip — usethis.userdirectly.Since the route is
authRequired: true,this.useris already populated by the auth middleware and carries theusernamefield. Fetching the full document again just to readusernameadds an unnecessary DB query on every mention request.♻️ Proposed refactor
- const user = await Users.findOneById(this.userId); - if (!user?.username) { + if (!this.user?.username) { return API.v1.failure('Invalid user'); } if (!(await canAccessRoomAsync(findResult, { _id: this.userId }))) { return API.v1.forbidden(); } const { cursor, totalCount } = Messages.findPaginatedVisibleByMentionAndRoomId( - user.username, + this.user.username,🤖 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 439 - 446, Replace the extra DB lookup Users.findOneById with the request-authenticated user object (this.user): check this.user?.username instead of awaiting Users.findOneById(this.userId) and keep the existing authorization call canAccessRoomAsync(findResult, { _id: this.userId }) as-is; this removes the unnecessary round-trip while preserving the username check and room access logic.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/meteor/app/api/server/v1/channels.ts
🧰 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 (4)
📚 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/v1/channels.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:
apps/meteor/app/api/server/v1/channels.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:
apps/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:
apps/meteor/app/api/server/v1/channels.ts
🧬 Code graph analysis (1)
apps/meteor/app/api/server/v1/channels.ts (2)
packages/models/src/index.ts (1)
Users(192-192)apps/meteor/app/authorization/server/index.ts (1)
canAccessRoomAsync(10-10)
🔇 Additional comments (2)
apps/meteor/app/api/server/v1/channels.ts (2)
66-71: LGTM — formatting-only change, no behavioral impact.
448-465: Normalization concern is not applicable — old implementation also returns raw documents.The old
getUserMentionsByChannelmethod (apps/meteor/app/mentions/server/methods/getUserMentionsByChannel.ts:36) returnsMessages.findVisibleByMentionAndRoomId(user.username, roomId, options).toArray()without callingnormalizeMessagesForUser. The new endpoint maintains identical behavior. While other message-returning endpoints likechannels.messagesandchannels.anonymousreaddo normalize, the mentions endpoint never did, so this is not a regression.Likely an incorrect or invalid review comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/meteor/app/api/server/v1/channels.ts`:
- Around line 439-446: Replace the extra DB lookup Users.findOneById with the
request-authenticated user object (this.user): check this.user?.username instead
of awaiting Users.findOneById(this.userId) and keep the existing authorization
call canAccessRoomAsync(findResult, { _id: this.userId }) as-is; this removes
the unnecessary round-trip while preserving the username check and room access
logic.
2 participants
Proposed changes
Fixes #38908
This PR corrects a critical performance regression in the
channels.getAllUserMentionsByChannelendpoint.Previously, the endpoint used getUserMentionsByChannel twice - once for the requested paginated chunk and once without skip/limit to compute the
.lengthof the entire document set.This has been refactored to use
Messages.findPaginatedVisibleByMentionAndRoomId. This allows us to concurrently resolve the paginated cursor and optimally fetch the total document count strictly from the database viaPromise.all([cursor.toArray(), totalCount]).This eliminates massive O(n) memory allocation and server latency when querying channels with thousands of mentions.
Issue(s)
Closes #38908
Steps to test or reproduce
GET /api/v1/channels.getAllUserMentionsByChannel?roomId=<id>&count=25&offset=0.Summary by CodeRabbit
Bug Fixes
Refactor