fix: use Document[] for aggregation pipeline types#38897
fix: use Document[] for aggregation pipeline types#38897amitkumarashutosh wants to merge 2 commits into
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 |
WalkthroughReworked typing and return shapes in 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)
packages/models/src/models/Messages.ts (1)
237-241:sortstage runs unnecessarily in theonlyCountpipeline path.Because
sortis added toparamsbefore theonlyCountguard, the aggregation for a count request executes[..., group, project, sort, { $count: 'total' }], where the sort is a no-op and wastes resources. Movingsortinto the non-count branch eliminates the redundant stage.♻️ Proposed refactor
- const sort: Document = { $sort: options.sort || { name: 1 } }; - const params: Document[] = [...firstParams, group, project, sort]; + const params: Document[] = [...firstParams, group, project]; if (onlyCount) { params.push({ $count: 'total' }); return this.col.aggregate<{ total: number }>(params, { readPreference: readSecondaryPreferred() }); } + const sort: Document = { $sort: options.sort || { name: 1 } }; + params.push(sort);🤖 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 237 - 241, The pipeline currently pushes `sort` into the `params` array before checking `onlyCount`, causing the aggregation to include a redundant `$sort` when counting; update the logic in the method that builds `params` so `params` is constructed as [...firstParams, group, project] and only if `onlyCount` push `{ $count: 'total' }` and call `this.col.aggregate` for the count path, otherwise append `sort` then call `this.col.aggregate` for the non-count path—use the existing variable names `params`, `firstParams`, `group`, `project`, `sort`, `onlyCount`, and `this.col.aggregate` to locate and adjust the branches.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/models/src/models/Messages.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:
packages/models/src/models/Messages.ts
🧠 Learnings (1)
📓 Common learnings
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.
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.
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.
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37557
File: apps/meteor/client/views/admin/ABAC/AdminABACRooms.tsx:115-116
Timestamp: 2025-11-27T17:56:26.050Z
Learning: In Rocket.Chat, the GET /v1/abac/rooms endpoint (implemented in ee/packages/abac/src/index.ts) only returns rooms where abacAttributes exists and is not an empty array (query: { abacAttributes: { $exists: true, $ne: [] } }). Therefore, in components consuming this endpoint (like AdminABACRooms.tsx), room.abacAttributes is guaranteed to be defined for all returned rooms, and optional chaining before calling array methods like .join() is sufficient without additional null coalescing.
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.
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37419
File: apps/meteor/server/services/media-call/service.ts:141-141
Timestamp: 2025-11-19T18:20:37.116Z
Learning: In apps/meteor/server/services/media-call/service.ts, the sendHistoryMessage method should use call.caller.id or call.createdBy?.id as the message author, not call.transferredBy?.id. Even for transferred calls, the message should appear in the DM between the two users who are calling each other, not sent by the person who transferred the call.
🔇 Additional comments (1)
packages/models/src/models/Messages.ts (1)
242-247: AI summary inconsistency: pagination logic pre-exists this PR.The AI summary claims the
$skip/$limitstages (lines 242–247) were newly introduced by this change, but those lines carry no~marker, indicating they are unchanged pre-existing code. The PR description ("No runtime changes; types-only change") is consistent with the annotation, and the changes are purely type annotations as intended.
🤖 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 237-241: The pipeline currently pushes `sort` into the `params`
array before checking `onlyCount`, causing the aggregation to include a
redundant `$sort` when counting; update the logic in the method that builds
`params` so `params` is constructed as [...firstParams, group, project] and only
if `onlyCount` push `{ $count: 'total' }` and call `this.col.aggregate` for the
count path, otherwise append `sort` then call `this.col.aggregate` for the
non-count path—use the existing variable names `params`, `firstParams`, `group`,
`project`, `sort`, `onlyCount`, and `this.col.aggregate` to locate and adjust
the branches.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #38897 +/- ##
===========================================
+ Coverage 70.56% 70.63% +0.07%
===========================================
Files 3189 3189
Lines 112702 112716 +14
Branches 20429 20428 -1
===========================================
+ Hits 79526 79619 +93
+ Misses 31115 31054 -61
+ Partials 2061 2043 -18
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
amitkumarashutosh
force-pushed
the
fix/messages-aggregation-type-definitions
branch
from
f007f25 to
e619f5f
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/models/src/models/Messages.ts (1)
337-342:⚠️ Potential issue | 🟡 Minor
options.offsetis silently ignored — pagination is incomplete.
getTotalOfMessagesSentByDateacceptsPaginatedRequestwhich includesoffset, but no$skipstage is ever pushed. By contrast,findAllNumberOfTransferredRooms(also updated in this PR) correctly adds$skip:if (options.offset) { params.push({ $skip: options.offset }); }Without the
$skipstage, passingoptions.offsethas no effect and callers expecting offset-based pagination get incorrect results.🐛 Proposed fix
if (options.sort) { params.push({ $sort: options.sort }); } + if (options.offset) { + params.push({ $skip: options.offset }); + } if (options.count) { params.push({ $limit: options.count }); }🤖 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 337 - 342, getTotalOfMessagesSentByDate ignores PaginatedRequest.offset so offset-based pagination is broken; update the aggregation builder in getTotalOfMessagesSentByDate to push a { $skip: options.offset } stage into params when options.offset is present (same pattern used in findAllNumberOfTransferredRooms), ensuring you check options.offset and then push the $skip stage before $limit/$sort so params reflects offset-based pagination correctly.
🧹 Nitpick comments (2)
packages/models/src/models/Messages.ts (2)
601-604: Remove the explanatory code comment.Line 601 adds
// Dynamic nested key path cannot be statically typed via MatchKeysAndValues<IMessage>. Theas unknown as UpdateFilter<IMessage>['$set']cast is self-explanatory enough in context, and the comment is not needed. As per coding guidelines, code comments should be avoided in the implementation (**/*.{ts,tsx,js}).♻️ Proposed change
- // Dynamic nested key path cannot be statically typed via MatchKeysAndValues<IMessage> $set: { [`reactions.${reaction}.federationReactionEventIds.${federationEventId}`]: username, } as unknown as UpdateFilter<IMessage>['$set'],🤖 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 601 - 604, Remove the inline explanatory comment preceding the dynamic $set assignment; keep the code that sets [`reactions.${reaction}.federationReactionEventIds.${federationEventId}`] = username with the existing cast to as unknown as UpdateFilter<IMessage>['$set'] and do not add any replacement comment—just delete the line containing "// Dynamic nested key path cannot be statically typed via MatchKeysAndValues<IMessage>" so the block only contains the $set assignment and cast.
35-45: ExportTotalOfMessagesSentByDateResultand updateIMessagesModelaccordingly.The type is declared locally in
Messages.tsbutIMessagesModel.getTotalOfMessagesSentByDatereturnsPromise<any[]>. The implementation provides the correct return type (Promise<TotalOfMessagesSentByDateResult[]>), but callers using the interface receive no type benefit. Export the type (or move it to@rocket.chat/model-typings) and update the interface signature toPromise<TotalOfMessagesSentByDateResult[]>for consistency.🤖 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 35 - 45, Export the TotalOfMessagesSentByDateResult type from Messages.ts (or move it to `@rocket.chat/model-typings`) and update the IMessagesModel.getTotalOfMessagesSentByDate signature to return Promise<TotalOfMessagesSentByDateResult[]> so callers get the concrete type; ensure the export is named TotalOfMessagesSentByDateResult and update any imports/usages to reference the exported type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/models/src/models/Messages.ts`:
- Around line 337-342: getTotalOfMessagesSentByDate ignores
PaginatedRequest.offset so offset-based pagination is broken; update the
aggregation builder in getTotalOfMessagesSentByDate to push a { $skip:
options.offset } stage into params when options.offset is present (same pattern
used in findAllNumberOfTransferredRooms), ensuring you check options.offset and
then push the $skip stage before $limit/$sort so params reflects offset-based
pagination correctly.
---
Nitpick comments:
In `@packages/models/src/models/Messages.ts`:
- Around line 601-604: Remove the inline explanatory comment preceding the
dynamic $set assignment; keep the code that sets
[`reactions.${reaction}.federationReactionEventIds.${federationEventId}`] =
username with the existing cast to as unknown as UpdateFilter<IMessage>['$set']
and do not add any replacement comment—just delete the line containing "//
Dynamic nested key path cannot be statically typed via
MatchKeysAndValues<IMessage>" so the block only contains the $set assignment and
cast.
- Around line 35-45: Export the TotalOfMessagesSentByDateResult type from
Messages.ts (or move it to `@rocket.chat/model-typings`) and update the
IMessagesModel.getTotalOfMessagesSentByDate signature to return
Promise<TotalOfMessagesSentByDateResult[]> so callers get the concrete type;
ensure the export is named TotalOfMessagesSentByDateResult and update any
imports/usages to reference the exported type.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/models/src/models/Messages.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:
packages/models/src/models/Messages.ts
🧠 Learnings (1)
📓 Common learnings
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.
🔇 Additional comments (1)
packages/models/src/models/Messages.ts (1)
199-243: Core aggregation typing fix is correct.
Document/Document[]are the right types for MongoDB aggregation pipeline stages. The FIXME comment has been resolved and the workaround usingExclude<Parameters<Collection<IMessage>['aggregate']>[0], undefined>is cleanly replaced.
1 participant
Proposed changes
Replace the brittle aggregation pipeline type workaround in
MessagesRawwith the correctDocumenttype from the MongoDB driver.The following variables are now typed as
Document:match,lookup,unwind,group,project,sortThe following arrays are now typed as
Document[]:firstParams,paramsThe following objects are now typed as
Filter<IMessage>,UpdateFilter<IMessage>:query,updateBefore
const match = { $match: { ... } };const lookup = { $lookup: { ... } };const unwind = { $unwind: { ... } };const group = { $group: { ... } };const project = { $project: { ... } };const firstParams: Exclude<Parameters<Collection<IMessage>['aggregate']>[0], undefined> = [match, lookup,unwind];const sort = { $sort: options.sort || { name: 1 } };const params = [...firstParams, group, project, sort];const query = { 'u._id': userId, '$or': [{ 'file._id': { $exists: true } }, { 'files._id': { $exists: true } }], };const update = { $set: { _hidden: hidden, },};After
const match : Document = { $match: { ... } };const lookup : Document = { $lookup: { ... } };const unwind : Document= { $unwind: { ... } };const group : Document = { $group: { ... } };const project : Document = { $project: { ... } };const firstParams : Document[] = [match, lookup,unwind];const sort : Document= { $sort: options.sort || { name: 1 } };const params : Document[] = [...firstParams, group, project, sort];const query : Filter<IMessage>= { 'u._id': userId, '$or': [{ 'file._id': { $exists: true } }, { 'files._id': { $exists: true } }], };const update : UpdateFilter<IMessage> = { $set: { _hidden: hidden, },};Issue(s)
Closes #38896
Steps to reproduce
packages/models/src/models/Messages.ts.Testing
Summary by CodeRabbit
New Features
Refactor