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 |
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughCentralizes allowDiskUse behavior, converts cursor/facet aggregation patterns to parallel aggregations and simpler lookups (localField/foreignField + post-filtering), updates model typings/returns from cursors to promises, and adds CI/Docker support to optionally use an external MongoDB via new workflow secrets and compose flags. Changes
Sequence Diagram(s)sequenceDiagram
participant GH as GitHub Actions
participant WF as ci-test-e2e.yml
participant Env as Job Env
participant NetBird as NetBird
participant Compose as Docker Compose CI
participant RC as Rocket.Chat services
participant DB as External MongoDB / DocumentDB
GH->>WF: workflow_call (with optional `CI_MONGO_URL`, `NETBIRD_SETUP_KEY`, `NETBIRD_PSK`)
WF->>Env: populate inputs & secrets
alt `CI_MONGO_URL` provided
WF->>NetBird: run NetBird connect with `NETBIRD_SETUP_KEY`/`NETBIRD_PSK`
Env->>Env: derive run-specific DB name from `CI_MONGO_URL`
Env-->>WF: export `MONGO_URL`, `DOCKER_MONGO_URL`, `EXTERNAL_MONGO=true`
else
Env-->>WF: internal DB (no external flags)
end
WF->>Compose: docker-compose up (passes `DOCKER_MONGO_URL`, `EXTERNAL_MONGO`)
alt `EXTERNAL_MONGO == "true"`
Compose->>DB: services connect to external `DOCKER_MONGO_URL`
Compose->>Compose: skip starting internal mongo (--scale mongo=0)
else
Compose->>Compose: start internal mongo
Compose->>RC: start services using internal mongo
end
RC->>DB: Rocket.Chat services connect using provided DB URL
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labelstype: chore 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #39415 +/- ##
===========================================
+ Coverage 70.57% 70.60% +0.02%
===========================================
Files 3263 3278 +15
Lines 116660 117430 +770
Branches 21067 21229 +162
===========================================
+ Hits 82336 82908 +572
- Misses 32271 32465 +194
- Partials 2053 2057 +4
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
ggazzo
force-pushed
the
chore/document-db
branch
3 times, most recently
from
2b6a948 to
e5722e7
Compare
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
6 issues found across 28 files
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="packages/models/src/models/Sessions.ts">
<violation number="1" location="packages/models/src/models/Sessions.ts:837">
P2: Splitting paginated docs and total count into two separate aggregates can return inconsistent pagination metadata under concurrent writes.
(Based on your team's feedback about behavioral changes that affect concurrency.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/meteor/app/api/server/v1/users.ts">
<violation number="1" location="apps/meteor/app/api/server/v1/users.ts:562">
P2: `users` and `total` now come from two independent aggregations, so writes between them can make `total` inconsistent with the returned page. The previous $facet kept them in one snapshot; consider keeping it single-pipeline to preserve consistent pagination counts.
(Based on your team's feedback about concurrency changes affecting behavior.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/meteor/server/lib/findUsersOfRoomOrderedByRole.ts">
<violation number="1" location="apps/meteor/server/lib/findUsersOfRoomOrderedByRole.ts:95">
P2: The new $lookup joins on user id only and filters by `rid` afterwards, so it loads all of a user’s subscriptions into the pipeline before filtering. This is much more work than matching `rid` inside the lookup and can cause a noticeable performance regression for users with many rooms.</violation>
</file>
<file name="packages/models/src/models/Rooms.ts">
<violation number="1" location="packages/models/src/models/Rooms.ts:2276">
P2: Running count and page data as separate aggregations can return inconsistent totals vs. results under concurrent writes. Compute both in one aggregation (e.g., `$facet`) so they come from the same snapshot.
(Based on your team's feedback about behavioral changes that affect concurrency.) [FEEDBACK_USED]</violation>
</file>
<file name="packages/models/src/models/Users.ts">
<violation number="1" location="packages/models/src/models/Users.ts:291">
P2: Paginated results and total count are now computed in separate aggregations, which can return mismatched metadata under concurrent updates.
(Based on your team's feedback about behavioral changes that affect concurrency.) [FEEDBACK_USED]</violation>
</file>
<file name="packages/models/src/models/LivechatRooms.ts">
<violation number="1" location="packages/models/src/models/LivechatRooms.ts:2174">
P2: extraQuery conditions are now evaluated against the room document, not the message element, so message-level filters (like `$token` in analytics) stop working and counts can include visitor/system messages. Adjust the filter to apply extraQuery to `$$msg` or update call sites to pass `$$msg`-scoped expressions.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| }, | ||
| ]; | ||
|
|
||
| const [users, countResult] = await Promise.all([ |
There was a problem hiding this comment.
P2: users and total now come from two independent aggregations, so writes between them can make total inconsistent with the returned page. The previous $facet kept them in one snapshot; consider keeping it single-pipeline to preserve consistent pagination counts.
(Based on your team's feedback about concurrency changes affecting behavior.)
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/v1/users.ts, line 562:
<comment>`users` and `total` now come from two independent aggregations, so writes between them can make `total` inconsistent with the returned page. The previous $facet kept them in one snapshot; consider keeping it single-pipeline to preserve consistent pagination counts.
(Based on your team's feedback about concurrency changes affecting behavior.) </comment>
<file context>
@@ -543,42 +543,28 @@ API.v1.addRoute(
+ },
+ ];
+
+ const [users, countResult] = await Promise.all([
+ Users.col.aggregate<IUser>([...baseQuery, { $sort: actualSort }, { $skip: offset }, ...limit]).toArray(),
+ Users.col.aggregate<{ total: number }>([...baseQuery, { $count: 'total' }]).toArray(),
</file context>
| }, | ||
| ]; | ||
|
|
||
| const [paginatedResults, countResult] = await Promise.all([ |
There was a problem hiding this comment.
P2: Running count and page data as separate aggregations can return inconsistent totals vs. results under concurrent writes. Compute both in one aggregation (e.g., $facet) so they come from the same snapshot.
(Based on your team's feedback about behavioral changes that affect concurrency.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/models/src/models/Rooms.ts, line 2276:
<comment>Running count and page data as separate aggregations can return inconsistent totals vs. results under concurrent writes. Compute both in one aggregation (e.g., `$facet`) so they come from the same snapshot.
(Based on your team's feedback about behavioral changes that affect concurrency.) </comment>
<file context>
@@ -2284,13 +2271,14 @@ export class RoomsRaw extends BaseRaw<IRoom> implements IRoomsModel {
- },
+ ];
+
+ const [paginatedResults, countResult] = await Promise.all([
+ this.col.aggregate<IRoom>([...baseQuery, { $skip: options?.skip || 0 }, { $limit: options?.limit || 50 }]).toArray(),
+ this.col.aggregate<{ count: number }>([...baseQuery, { $count: 'count' }]).toArray(),
</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: 4
🧹 Nitpick comments (4)
packages/models/src/models/Subscriptions.ts (1)
467-467: Drop the new stage narration comments.These lines just restate the adjacent aggregation stages and add noise to a pipeline that already reads clearly.
As per coding guidelines, "Avoid code comments in the implementation".
Also applies to: 482-482, 491-491, 497-497
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/models/src/models/Subscriptions.ts` at line 467, Remove the redundant stage-narration comments inside the aggregation pipeline in packages/models/src/models/Subscriptions.ts (e.g., the comment "Filter out the requesting user's own subscriptions" and the similar narration comments at the other aggregation stages referenced). Locate the aggregation pipeline used in the Subscriptions model (the array or variable that builds the Mongo aggregation stages—look for the pipeline in the function that constructs/returns subscriptions) and delete those comments that merely restate the adjacent stage behavior, leaving only any necessary inline notes if truly needed.packages/models/src/models/LivechatBusinessHours.ts (1)
82-118: Avoidas anytype assertion.The
as anycast on line 117 suggests a type mismatch between the aggregation results andIWorkHoursCronJobsWrapper[]. The aggregation returns{ day: string; times: string[] }[]for bothstartandfinish, which should matchIWorkHoursCronJobsItem[]as defined in the interface.♻️ Suggested fix to remove type assertion
- return [{ start, finish }] as any; + return [{ start, finish }] as IWorkHoursCronJobsWrapper[];Or better, ensure the aggregation result type is explicitly typed:
const [start, finish] = await Promise.all([ this.col - .aggregate([ + .aggregate<IWorkHoursCronJobsItem>([ ...basePipeline,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/models/src/models/LivechatBusinessHours.ts` around lines 82 - 118, The return uses an unsafe `as any` in findHoursToScheduleJobs; instead type the aggregation results and return value properly: cast the two aggregate .toArray() calls to IWorkHoursCronJobsItem[] (or use generics on this.col.aggregate<...> to yield { day: string; times: string[] }[] / IWorkHoursCronJobsItem[]) for both start and finish, then return [{ start, finish }] as IWorkHoursCronJobsWrapper[] (or construct the wrapper object typed to IWorkHoursCronJobsWrapper[]) so the types line up without `as any`; update the signatures on the this.col.aggregate calls or local variables `start` and `finish` accordingly.packages/models/src/models/Users.ts (1)
569-590: Extract the repeated department-join block.These two routing methods now carry the same department lookup/filter sequence. A small helper returning the shared stages would make future routing fixes land in one place.
Also applies to: 656-677
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/models/src/models/Users.ts` around lines 569 - 590, Extract the repeated department-join aggregation stages into a small helper (e.g., buildDepartmentJoinStages or getDepartmentLookupStages) that returns the three stages: the $lookup from 'rocketchat_livechat_department_agents', the $addFields that filters department entries by the provided department variable, and the $match that ensures department has $size: 1; then replace both inline occurrences in Users.ts with a call to that helper, passing the same department identifier used in the original code.packages/models/src/models/LivechatRooms.ts (1)
2160-2179: Consider factoring the shared message-join block.These two analytics helpers now repeat the same message lookup/filter stages. A small pipeline helper would reduce drift if the message predicate changes again.
Also applies to: 2231-2248
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/models/src/models/LivechatRooms.ts` around lines 2160 - 2179, The message lookup/filter aggregation stages are duplicated; extract them into a single helper (e.g., getMessagesLookupStages(extraQuery) or buildMessageJoinStages) that returns the two pipeline stages shown (the $lookup and the $addFields with the $filter using extraQuery), then replace the duplicated inline stages in both places (the blocks currently around the message join) with a spread of that helper result so both analytics helpers call the shared function and preserve the existing extraQuery behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci-test-e2e.yml:
- Around line 160-168: Add a teardown step that drops the dynamically created
external MongoDB database when EXTERNAL_MONGO is true: after tests complete, run
a job or a post-step that reads CI_MONGO_URL and DB_NAME (the env vars set in
the existing step: CI_MONGO_URL, DB_NAME, DOCKER_MONGO_URL, EXTERNAL_MONGO) and
invokes the Mongo client (mongosh/mongo) to connect to CI_MONGO_URL and run a
dropDatabase() against the DB_NAME; ensure this cleanup is conditional on
EXTERNAL_MONGO=true, uses the same secret (CI_MONGO_URL) for auth, and treats
failures non-blocking/logged so cleanup does not mask test results.
In `@packages/models/src/models/Subscriptions.ts`:
- Around line 499-509: The current $match remapping uses
Object.entries(extraConditions) which flattens nested/compound operators (e.g.,
$or) into user.$or and breaks filters; fix by building a prefixed filter that
only prefixes plain field paths (keys that do not start with $) in
extraConditions to `user.<field>` while leaving operator keys (keys starting
with $) intact, produce a single prefixedExtraConditions object, and then
combine prefixedExtraConditions with the search clause via an $and (instead of
merging them directly) in the $match; update the remapping logic around
extraConditions, orStatement, searchTerm and exceptions in the $match block so
compound Filters (like { $or: [...] }) are preserved and applied correctly to
`user.*` fields.
In `@packages/models/src/models/Users.ts`:
- Around line 606-617: The subs $filter inside the $addFields stage is currently
including all open subscriptions (channels, DMs) whereas
getAgentAndAmountOngoingChats() expects only livechat items (it checks $$sub.t
=== 'l'); update the $and conditions in the subs filter (the $filter under
$addFields -> subs) to include { $eq: ['$$sub.t', 'l'] } alongside the existing
{ $eq: ['$$sub.open', true] } and { $ne: ['$$sub.onHold', true] } (keeping the
department conditional untouched) so routing counts only livechat subscriptions.
- Around line 289-293: The aggregation pipeline currently builds
paginationStages with unconditional { $sort: options?.sort } and { $skip:
options?.skip }, which may insert undefined values and break MongoDB; change the
construction in the method that uses paginationStages (the code that calls
this.col.aggregate([...aggregate, ...paginationStages]) and
this.col.aggregate([...aggregate, { $count: 'total' }])) to only include each
stage when its value is present (e.g., push or conditionally spread { $sort:
options.sort } only if options?.sort !== undefined, { $skip: options.skip } only
if options?.skip !== undefined, and { $limit: options.limit } only if
options?.limit !== undefined) so the pipeline contains no stages with undefined
values.
---
Nitpick comments:
In `@packages/models/src/models/LivechatBusinessHours.ts`:
- Around line 82-118: The return uses an unsafe `as any` in
findHoursToScheduleJobs; instead type the aggregation results and return value
properly: cast the two aggregate .toArray() calls to IWorkHoursCronJobsItem[]
(or use generics on this.col.aggregate<...> to yield { day: string; times:
string[] }[] / IWorkHoursCronJobsItem[]) for both start and finish, then return
[{ start, finish }] as IWorkHoursCronJobsWrapper[] (or construct the wrapper
object typed to IWorkHoursCronJobsWrapper[]) so the types line up without `as
any`; update the signatures on the this.col.aggregate calls or local variables
`start` and `finish` accordingly.
In `@packages/models/src/models/LivechatRooms.ts`:
- Around line 2160-2179: The message lookup/filter aggregation stages are
duplicated; extract them into a single helper (e.g.,
getMessagesLookupStages(extraQuery) or buildMessageJoinStages) that returns the
two pipeline stages shown (the $lookup and the $addFields with the $filter using
extraQuery), then replace the duplicated inline stages in both places (the
blocks currently around the message join) with a spread of that helper result so
both analytics helpers call the shared function and preserve the existing
extraQuery behavior.
In `@packages/models/src/models/Subscriptions.ts`:
- Line 467: Remove the redundant stage-narration comments inside the aggregation
pipeline in packages/models/src/models/Subscriptions.ts (e.g., the comment
"Filter out the requesting user's own subscriptions" and the similar narration
comments at the other aggregation stages referenced). Locate the aggregation
pipeline used in the Subscriptions model (the array or variable that builds the
Mongo aggregation stages—look for the pipeline in the function that
constructs/returns subscriptions) and delete those comments that merely restate
the adjacent stage behavior, leaving only any necessary inline notes if truly
needed.
In `@packages/models/src/models/Users.ts`:
- Around line 569-590: Extract the repeated department-join aggregation stages
into a small helper (e.g., buildDepartmentJoinStages or
getDepartmentLookupStages) that returns the three stages: the $lookup from
'rocketchat_livechat_department_agents', the $addFields that filters department
entries by the provided department variable, and the $match that ensures
department has $size: 1; then replace both inline occurrences in Users.ts with a
call to that helper, passing the same department identifier used in the original
code.
🪄 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: d61e3f68-dbcf-4811-af9a-1e3e970097ea
📒 Files selected for processing (28)
.github/workflows/ci-test-e2e.yml.github/workflows/ci.ymlapps/meteor/app/api/server/lib/users.tsapps/meteor/app/api/server/v1/users.tsapps/meteor/ee/server/api/abac/index.tsapps/meteor/ee/server/api/audit.tsapps/meteor/ee/server/lib/engagementDashboard/channels.tsapps/meteor/ee/server/models/raw/Users.tsapps/meteor/server/lib/findUsersOfRoomOrderedByRole.tsapps/meteor/server/services/team/service.tsdocker-compose-ci.ymldocs/documentdb-compatibility.mdpackages/model-typings/src/models/IAnalyticsModel.tspackages/model-typings/src/models/IRoomsModel.tspackages/models/src/allowDiskUse.tspackages/models/src/index.tspackages/models/src/models/Analytics.tspackages/models/src/models/LivechatAgentActivity.tspackages/models/src/models/LivechatBusinessHours.tspackages/models/src/models/LivechatContacts.tspackages/models/src/models/LivechatDepartment.tspackages/models/src/models/LivechatRooms.tspackages/models/src/models/Messages.tspackages/models/src/models/ModerationReports.tspackages/models/src/models/Rooms.tspackages/models/src/models/Sessions.tspackages/models/src/models/Subscriptions.tspackages/models/src/models/Users.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). (10)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.0 coverage (4/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.0 coverage (1/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.0 coverage (2/5)
- GitHub Check: 🔨 Test API Livechat (EE) / MongoDB 8.0 coverage (1/1)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.0 coverage (3/5)
- GitHub Check: 🔨 Test UI (EE) / MongoDB 8.0 coverage (5/5)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 8.0 (3/4)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 8.0 (1/4)
- GitHub Check: 🔨 Test UI (CE) / MongoDB 8.0 (4/4)
🧰 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/ee/server/lib/engagementDashboard/channels.tsapps/meteor/ee/server/api/abac/index.tspackages/models/src/models/LivechatAgentActivity.tsapps/meteor/app/api/server/lib/users.tsapps/meteor/server/lib/findUsersOfRoomOrderedByRole.tsapps/meteor/server/services/team/service.tspackages/models/src/index.tspackages/models/src/models/LivechatContacts.tsapps/meteor/ee/server/api/audit.tspackages/models/src/allowDiskUse.tspackages/models/src/models/Messages.tspackages/models/src/models/ModerationReports.tspackages/models/src/models/LivechatDepartment.tspackages/model-typings/src/models/IAnalyticsModel.tspackages/models/src/models/LivechatBusinessHours.tsapps/meteor/ee/server/models/raw/Users.tspackages/models/src/models/Sessions.tsapps/meteor/app/api/server/v1/users.tspackages/models/src/models/Subscriptions.tspackages/models/src/models/Users.tspackages/model-typings/src/models/IRoomsModel.tspackages/models/src/models/LivechatRooms.tspackages/models/src/models/Rooms.tspackages/models/src/models/Analytics.ts
🧠 Learnings (28)
📚 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/ee/server/lib/engagementDashboard/channels.tsapps/meteor/app/api/server/lib/users.tsapps/meteor/server/lib/findUsersOfRoomOrderedByRole.tsapps/meteor/server/services/team/service.tsapps/meteor/ee/server/api/audit.tsapps/meteor/ee/server/models/raw/Users.tspackages/models/src/models/Sessions.tsapps/meteor/app/api/server/v1/users.tspackages/models/src/models/Subscriptions.tspackages/models/src/models/Users.tspackages/model-typings/src/models/IRoomsModel.tspackages/models/src/models/LivechatRooms.tspackages/models/src/models/Rooms.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/ee/server/lib/engagementDashboard/channels.tsapps/meteor/ee/server/api/abac/index.tsapps/meteor/app/api/server/lib/users.tsapps/meteor/server/lib/findUsersOfRoomOrderedByRole.tspackages/models/src/models/Messages.tsapps/meteor/ee/server/models/raw/Users.tsdocs/documentdb-compatibility.md
📚 Learning: 2026-03-16T22:56:54.500Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 39677
File: packages/models/src/helpers/omnichannel/agentStatus.ts:10-29
Timestamp: 2026-03-16T22:56:54.500Z
Learning: In `packages/models/src/helpers/omnichannel/agentStatus.ts` (PR `#39677`), the `queryStatusAgentOnline` function intentionally omits the `$or` offline-status guard for non-bot agents when `isLivechatEnabledWhenAgentIdle === true`. This is by design: the setting `Livechat_enabled_when_agent_idle` (`accept_chats_when_agent_idle`) means agents should receive chats even when idle/offline, so the offline filter must be removed in that path. Bots are always status-agnostic and are always included regardless of their online/offline status. Do not flag this as a bug.
Applied to files:
apps/meteor/ee/server/lib/engagementDashboard/channels.tspackages/models/src/models/LivechatAgentActivity.tspackages/models/src/models/LivechatContacts.tspackages/models/src/models/LivechatDepartment.tspackages/models/src/models/LivechatBusinessHours.tsapps/meteor/ee/server/models/raw/Users.tspackages/models/src/models/Users.tspackages/models/src/models/LivechatRooms.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/ee/server/lib/engagementDashboard/channels.tsapps/meteor/ee/server/api/abac/index.tspackages/models/src/models/LivechatAgentActivity.tsapps/meteor/app/api/server/lib/users.tsapps/meteor/server/lib/findUsersOfRoomOrderedByRole.tsapps/meteor/server/services/team/service.tspackages/models/src/index.tspackages/models/src/models/LivechatContacts.tsapps/meteor/ee/server/api/audit.tspackages/models/src/allowDiskUse.tspackages/models/src/models/Messages.tspackages/models/src/models/ModerationReports.tspackages/models/src/models/LivechatDepartment.tspackages/model-typings/src/models/IAnalyticsModel.tspackages/models/src/models/LivechatBusinessHours.tsapps/meteor/ee/server/models/raw/Users.tspackages/models/src/models/Sessions.tsapps/meteor/app/api/server/v1/users.tspackages/models/src/models/Subscriptions.tspackages/models/src/models/Users.tspackages/model-typings/src/models/IRoomsModel.tspackages/models/src/models/LivechatRooms.tspackages/models/src/models/Rooms.tspackages/models/src/models/Analytics.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/ee/server/lib/engagementDashboard/channels.tsapps/meteor/ee/server/api/abac/index.tspackages/models/src/models/LivechatAgentActivity.tsapps/meteor/app/api/server/lib/users.tsapps/meteor/server/lib/findUsersOfRoomOrderedByRole.tsapps/meteor/server/services/team/service.tspackages/models/src/index.tspackages/models/src/models/LivechatContacts.tsapps/meteor/ee/server/api/audit.tspackages/models/src/allowDiskUse.tspackages/models/src/models/Messages.tspackages/models/src/models/ModerationReports.tspackages/models/src/models/LivechatDepartment.tspackages/model-typings/src/models/IAnalyticsModel.tspackages/models/src/models/LivechatBusinessHours.tsapps/meteor/ee/server/models/raw/Users.tspackages/models/src/models/Sessions.tsapps/meteor/app/api/server/v1/users.tspackages/models/src/models/Subscriptions.tspackages/models/src/models/Users.tspackages/model-typings/src/models/IRoomsModel.tspackages/models/src/models/LivechatRooms.tspackages/models/src/models/Rooms.tspackages/models/src/models/Analytics.ts
📚 Learning: 2025-11-07T14:50:33.544Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37423
File: packages/i18n/src/locales/en.i18n.json:18-18
Timestamp: 2025-11-07T14:50:33.544Z
Learning: Rocket.Chat settings: in apps/meteor/ee/server/settings/abac.ts, the Abac_Cache_Decision_Time_Seconds setting uses invalidValue: 0 as the fallback when ABAC is unlicensed. With a valid license, admins can still set the value to 0 to intentionally disable the ABAC decision cache.
Applied to files:
apps/meteor/ee/server/api/abac/index.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/ee/server/api/abac/index.tsapps/meteor/app/api/server/lib/users.tsapps/meteor/ee/server/api/audit.tsapps/meteor/ee/server/models/raw/Users.ts
📚 Learning: 2026-03-15T14:31:28.969Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39647
File: apps/meteor/app/api/server/v1/users.ts:710-757
Timestamp: 2026-03-15T14:31:28.969Z
Learning: In RocketChat/Rocket.Chat, the `UserCreateParamsPOST` type in `apps/meteor/app/api/server/v1/users.ts` (migrated from `packages/rest-typings/src/v1/users/UserCreateParamsPOST.ts`) intentionally has `fields: string` (non-optional) and `settings?: IUserSettings` without a corresponding AJV schema entry. This is a pre-existing divergence carried over verbatim from the original rest-typings source (PR `#39647`). Do not flag this type/schema misalignment during the OpenAPI migration review — it is tracked as a separate follow-up fix.
Applied to files:
apps/meteor/ee/server/api/abac/index.tsapps/meteor/app/api/server/lib/users.tsapps/meteor/server/lib/findUsersOfRoomOrderedByRole.tsapps/meteor/ee/server/api/audit.tsapps/meteor/ee/server/models/raw/Users.ts
📚 Learning: 2026-03-16T21:50:42.118Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39676
File: .changeset/migrate-users-register-openapi.md:3-3
Timestamp: 2026-03-16T21:50:42.118Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs, removing endpoint types and validators from `rocket.chat/rest-typings` (e.g., `UserRegisterParamsPOST`, `/v1/users.register` entry) is the *required* migration pattern per RocketChat/Rocket.Chat-Open-API#150 Rule 7 ("No More rest-typings or Manual Typings"). The endpoint type is re-exposed via a module augmentation `.d.ts` file in the consuming package (e.g., `packages/web-ui-registration/src/users-register.d.ts`). This is NOT a breaking change — the correct changeset bump for `rocket.chat/rest-typings` in this scenario is `minor`, not `major`. Do not flag this as a breaking change during OpenAPI migration reviews.
Applied to files:
apps/meteor/ee/server/api/abac/index.tsapps/meteor/app/api/server/lib/users.tsapps/meteor/ee/server/api/audit.ts
📚 Learning: 2025-11-27T17:56:26.050Z
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.
Applied to files:
apps/meteor/ee/server/api/abac/index.tsapps/meteor/server/services/team/service.tsapps/meteor/ee/server/api/audit.tspackages/models/src/models/LivechatRooms.tspackages/models/src/models/Rooms.ts
📚 Learning: 2025-10-27T14:38:46.994Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37303
File: apps/meteor/tests/end-to-end/api/abac.ts:1125-1137
Timestamp: 2025-10-27T14:38:46.994Z
Learning: In Rocket.Chat ABAC feature, when ABAC is disabled globally (ABAC_Enabled setting is false), room-level ABAC attributes are not evaluated when changing room types. This means converting a private room to public will succeed even if the room has ABAC attributes, as long as the global ABAC setting is disabled.
Applied to files:
apps/meteor/ee/server/api/abac/index.ts
📚 Learning: 2026-02-25T20:10:16.987Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38913
File: packages/ddp-client/src/legacy/types/SDKLegacy.ts:34-34
Timestamp: 2026-02-25T20:10:16.987Z
Learning: In the RocketChat/Rocket.Chat monorepo, packages/ddp-client and apps/meteor do not use TypeScript project references. Module augmentations in apps/meteor (e.g., declare module 'rocket.chat/rest-typings') are not visible when compiling packages/ddp-client in isolation, which is why legacy SDK methods that depend on OperationResult types for OpenAPI-migrated endpoints must remain commented out.
Applied to files:
apps/meteor/app/api/server/lib/users.tsapps/meteor/server/lib/findUsersOfRoomOrderedByRole.tsapps/meteor/ee/server/api/audit.tsapps/meteor/ee/server/models/raw/Users.tspackages/models/src/models/Sessions.ts
📚 Learning: 2026-03-16T23:33:15.721Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39676
File: apps/meteor/app/api/server/v1/users.ts:862-869
Timestamp: 2026-03-16T23:33:15.721Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs (e.g., PR `#39676` for users.register in apps/meteor/app/api/server/v1/users.ts), calls to `this.parseJsonQuery()` inside migrated handlers are intentionally preserved without adding a corresponding `query` AJV schema to the route options. Adding query-param schemas for the `fields`/`sort`/`query` parameters consumed by `parseJsonQuery()` is a separate cross-cutting concern shared by many endpoints (e.g., users.create, users.update, users.list) and is explicitly out of scope for individual endpoint migration PRs. Do not flag the absence of a `query` schema for `parseJsonQuery()` usage as a violation of OpenAPI/AJV contract during migration reviews.
Applied to files:
apps/meteor/app/api/server/lib/users.tsapps/meteor/ee/server/api/audit.ts
📚 Learning: 2026-03-14T14:58:58.834Z
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.
Applied to files:
apps/meteor/app/api/server/lib/users.ts
📚 Learning: 2026-03-15T14:31:23.493Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39647
File: apps/meteor/app/api/server/v1/users.ts:891-899
Timestamp: 2026-03-15T14:31:23.493Z
Learning: In RocketChat/Rocket.Chat, `IUser.inactiveReason` in `packages/core-typings/src/IUser.ts` is typed as `'deactivated' | 'pending_approval' | 'idle_too_long'` (optional, no `null`), but the database stores `null` for newly created users. The Typia-generated `$ref: '#/components/schemas/IUser'` schema therefore correctly rejects `null` for `inactiveReason`. This causes the test "should create a new user with default roles" to fail when response validation is active (TEST_MODE). The fix is to add `| null` to `inactiveReason` in core-typings and rebuild Typia schemas in a separate PR. Do not flag this test failure as a bug introduced by the users.create OpenAPI migration (PR `#39647`). Do not suggest inlining a custom schema to work around it, as migration rules require using `$ref` when a Typia schema exists.
Applied to files:
apps/meteor/app/api/server/lib/users.ts
📚 Learning: 2025-09-19T15:15:04.642Z
Learnt from: rodrigok
Repo: RocketChat/Rocket.Chat PR: 36991
File: apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts:219-221
Timestamp: 2025-09-19T15:15:04.642Z
Learning: The Federation_Matrix_homeserver_domain setting in apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts is part of the old federation system and is being deprecated/removed, so configuration issues with this setting should not be flagged for improvement.
Applied to files:
apps/meteor/app/api/server/lib/users.tsapps/meteor/ee/server/models/raw/Users.ts
📚 Learning: 2025-11-04T16:49:19.107Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37377
File: apps/meteor/ee/server/hooks/federation/index.ts:86-88
Timestamp: 2025-11-04T16:49:19.107Z
Learning: In Rocket.Chat's federation system (apps/meteor/ee/server/hooks/federation/), permission checks follow two distinct patterns: (1) User-initiated federation actions (creating rooms, adding users to federated rooms, joining from invites) should throw MeteorError to inform users they lack 'access-federation' permission. (2) Remote server-initiated federation events should silently skip/ignore when users lack permission. The beforeAddUserToRoom hook only executes for local user-initiated actions, so throwing an error there is correct. Remote federation events are handled separately by the federation Matrix package with silent skipping logic.
Applied to files:
apps/meteor/app/api/server/lib/users.tsapps/meteor/server/lib/findUsersOfRoomOrderedByRole.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/server/lib/findUsersOfRoomOrderedByRole.tsapps/meteor/server/services/team/service.tsapps/meteor/ee/server/api/audit.tspackages/models/src/models/Subscriptions.tspackages/models/src/models/Rooms.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/server/lib/findUsersOfRoomOrderedByRole.tsapps/meteor/server/services/team/service.tsapps/meteor/ee/server/api/audit.tspackages/models/src/models/Subscriptions.tspackages/models/src/models/Rooms.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/server/lib/findUsersOfRoomOrderedByRole.tspackages/models/src/models/Messages.tsapps/meteor/ee/server/models/raw/Users.tspackages/models/src/models/LivechatRooms.tspackages/models/src/models/Rooms.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/server/lib/findUsersOfRoomOrderedByRole.tsapps/meteor/ee/server/api/audit.tspackages/models/src/models/Rooms.ts
📚 Learning: 2026-03-10T08:13:52.153Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 39414
File: apps/meteor/app/api/server/v1/rooms.ts:1241-1297
Timestamp: 2026-03-10T08:13:52.153Z
Learning: In the RocketChat/Rocket.Chat OpenAPI migration PRs for endpoints under apps/meteor/app/api/server/v1/rooms.ts, the pattern `ajv.compile<void>({...})` is intentionally used for the 200 response schema even when the endpoint returns `{ success: true }`. This is an established convention across all migrated endpoints (rooms.leave, rooms.favorite, rooms.delete, rooms.muteUser, rooms.unmuteUser). Do not flag this as a type mismatch during reviews of these migration PRs.
Applied to files:
apps/meteor/ee/server/api/audit.ts
📚 Learning: 2026-03-18T22:07:19.687Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38357
File: apps/meteor/app/apps/server/converters/departments.ts:59-70
Timestamp: 2026-03-18T22:07:19.687Z
Learning: In `apps/meteor/app/apps/server/converters/departments.ts`, the `convertAppDepartment` method uses non-null assertions (`!`) on `department.name`, `department.email`, and `department.offlineMessageChannelName` when constructing `newDepartment: ILivechatDepartment`. These fields are optional on `IAppsDepartment`, but the App framework flow guarantees their presence at the call site. Do not flag these non-null assertions as unsafe during code review.
Applied to files:
packages/models/src/models/LivechatDepartment.tsapps/meteor/ee/server/models/raw/Users.tspackages/models/src/models/Users.tspackages/models/src/models/LivechatRooms.ts
📚 Learning: 2026-03-11T16:46:55.955Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 39535
File: apps/meteor/app/apps/server/bridges/livechat.ts:249-249
Timestamp: 2026-03-11T16:46:55.955Z
Learning: In `apps/meteor/app/apps/server/bridges/livechat.ts`, `createVisitor()` intentionally does not propagate `externalIds` to `registerData`. This is by design: the method is deprecated (JSDoc: `deprecated Use createAndReturnVisitor instead. Note: This method does not support externalIds.`) to push callers toward `createAndReturnVisitor()`, which does support `externalIds`. Do not flag the missing `externalIds` field in `createVisitor()` as a bug.
Applied to files:
packages/models/src/models/LivechatDepartment.tsapps/meteor/ee/server/models/raw/Users.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/users.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/users.ts
📚 Learning: 2026-03-15T14:31:25.380Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39647
File: apps/meteor/app/api/server/v1/users.ts:710-757
Timestamp: 2026-03-15T14:31:25.380Z
Learning: Do not flag this type/schema misalignment in the OpenAPI/migration review for apps/meteor/app/api/server/v1/users.ts. The UserCreateParamsPOST type intentionally uses non-optional fields: fields: string and settings?: IUserSettings without an AJV schema entry, carried over from the original rest-typings (PR `#39647`). Treat this as a known pre-existing divergence and document it as a separate follow-up fix; do not block or mark it as a review issue during the migration.
Applied to files:
apps/meteor/app/api/server/v1/users.ts
📚 Learning: 2026-03-16T23:33:11.443Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39676
File: apps/meteor/app/api/server/v1/users.ts:862-869
Timestamp: 2026-03-16T23:33:11.443Z
Learning: In rockets OpenAPI/AJV migration reviews for RocketChat/Rocket.Chat, when reviewing migrations that involve apps/meteor/app/api/server/v1/users.ts, do not require or flag a missing query AJV schema for the fields consumed by parseJsonQuery() (i.e., fields, sort, query) as part of this endpoint's migration PR. The addition of global query-param schemas for parseJsonQuery() usage is a cross-cutting concern and out of scope for individual endpoint migrations. Only flag violations related to the specific scope of the migration, not the absence of a query schema for parseJsonQuery() in this file.
Applied to files:
apps/meteor/app/api/server/v1/users.ts
🔇 Additional comments (36)
packages/models/src/models/Subscriptions.ts (1)
454-459: The lookup translation keeps the original join shape.Using
localField/foreignFieldhere, then excluding the requester after unwind, preserves the room→subscription and userId→user joins without relying on the correlated$lookupform.Also applies to: 468-472, 483-489
docker-compose-ci.yml (1)
19-19: LGTM! Proper parameterization for external MongoDB support.The
DOCKER_MONGO_URLenvironment variable with sensible defaults and therequired: falsecondition on the mongo dependency allows the stack to connect to an external MongoDB/DocumentDB while maintaining backward compatibility with the local mongo service.Also applies to: 32-36
packages/models/src/index.ts (1)
125-125: LGTM! Clean re-export of the new utility.The
getAllowDiskUsehelper is properly exported alongside other utilities, making it available to downstream code importing from@rocket.chat/models.apps/meteor/ee/server/api/abac/index.ts (1)
4-4: LGTM! Consistent application of the DocumentDB compatibility pattern.The import and usage of
getAllowDiskUse()follows the same pattern applied across other pagination endpoints, ensuring consistent behavior when running against DocumentDB.Also applies to: 396-396
.github/workflows/ci.yml (1)
503-503: LGTM! Consistent secret injection for external MongoDB support.The
CI_MONGO_URLsecret is properly passed to all E2E test jobs, enabling optional external MongoDB/DocumentDB connectivity. Using secrets for database connection strings is the correct security practice.Also applies to: 520-520, 546-546, 567-567, 588-588, 617-617
apps/meteor/ee/server/lib/engagementDashboard/channels.ts (1)
40-47: LGTM! Aligned with updated upstream method signature.The removal of
.toArray()is consistent with the refactoredfindRoomsByTypesWithNumberOfMessagesBetweenDatemethod, which now returns aPromiseresolving to an array directly rather than anAggregationCursor.apps/meteor/app/api/server/lib/users.ts (1)
2-2: LGTM! Consistent DocumentDB compatibility pattern applied.The import and usage of
getAllowDiskUse()follows the established pattern for conditionally enabling disk use in paginated queries.Also applies to: 232-232
apps/meteor/ee/server/api/audit.ts (1)
2-2: LGTM! Audit endpoint updated with DocumentDB compatibility.The
getAllowDiskUse()helper is correctly imported and applied to the paginated server events query, consistent with other audit-related endpoints.Also applies to: 183-183
packages/models/src/models/LivechatContacts.ts (2)
29-29: LGTM! Proper option merging in findPaginatedContacts.The
getAllowDiskUse()is spread before...options, allowing callers to override the disk use setting if needed while providing sensible defaults.Also applies to: 182-188
440-456: LGTM! Aggregation options properly combined.The
getStatisticsmethod correctly merges the dynamic disk use configuration with the secondary read preference for the aggregation pipeline.packages/models/src/models/LivechatAgentActivity.ts (1)
7-7: LGTM!The import and usage of
getAllowDiskUse()is correct and consistent with the centralized approach for DocumentDB compatibility. The spread operator properly merges the conditionalallowDiskUsewith thereadPreferenceoption.Also applies to: 237-237
packages/models/src/models/Messages.ts (1)
33-33: LGTM!The
getAllowDiskUse()helper is correctly imported and applied to both aggregation call sites, maintaining the existingreadPreferenceconfiguration while centralizing theallowDiskUsebehavior.Also applies to: 251-253, 331-331
apps/meteor/server/lib/findUsersOfRoomOrderedByRole.ts (1)
93-128: LGTM!The
$lookuprefactor correctly replaces thelet/pipelinecorrelation pattern (unsupported by DocumentDB) with alocalField/foreignFieldjoin followed by an$addFieldsstage that filters thesubscriptionarray to entries matching the roomrid. The subsequent$unwindstage ensures the finalsubscriptionfield matches the expected scalar type. ThegetAllowDiskUse()helper is correctly applied.packages/models/src/allowDiskUse.ts (1)
1-17: LGTM!The
getAllowDiskUse()helper is well-implemented with clear documentation. The conditional logic correctly handles the DocumentDB compatibility case by returning an empty object (which spreads to nothing) whenDOCUMENTDB=true, allowing the option to be omitted from aggregation calls.packages/models/src/models/LivechatDepartment.ts (2)
387-413: LGTM!The replacement of
$$REMOVEwithnullfollowed by a$filterstage to strip null values is the correct DocumentDB-compatible pattern. This maintains the same semantic behavior while avoiding the unsupported$$REMOVEoperator.
427-443: LGTM!The
$lookuprefactor correctly replaces thelet/pipelinepattern withlocalField/foreignFieldjoin followed by an$addFieldsstage that filters themonitorsarray to entries matching the providedmonitorId. The subsequent$matchstage correctly validates that the filtered array is non-empty.packages/models/src/models/ModerationReports.ts (1)
13-13: LGTM!The
getAllowDiskUse()helper is correctly imported and applied to both aggregation methods. ThefindUserReportsmethod properly preserves thereadSecondaryPreferred()configuration alongside the conditionalallowDiskUseoption.Also applies to: 140-140, 197-197
apps/meteor/app/api/server/v1/users.ts (1)
546-567: LGTM!The
$facetreplacement with parallelPromise.allqueries is correctly implemented. The sharedbaseQuerystages are properly factored out, and both aggregations execute concurrently to minimize latency impact. The total extraction withcountResult[0]?.total || 0correctly handles edge cases where the count aggregation returns an empty array.apps/meteor/server/services/team/service.ts (1)
1124-1125: The destructuring pattern is safe and does not require modification.
findChildrenOfTeamalways returns an array with exactly one element:[{ totalCount, paginatedResults }]. Even when no matching rooms exist, it returns[{ totalCount: [], paginatedResults: [] }]. Since the returned array is never empty, the destructuring pattern[{ totalCount: [...] = [], paginatedResults: data = [] }]safely handles all cases, andtotalbecomesundefinedwhentotalCountis empty, whiledatadefaults to[].The
|| []fallback is a defensive measure but unnecessary given the type signature; however, it causes no harm..github/workflows/ci-test-e2e.yml (1)
190-194: LGTM!The conditional
--scale mongo=0correctly disables the local mongo container when an external MongoDB is configured, preserving the existing behavior for local setups.packages/model-typings/src/models/IAnalyticsModel.ts (1)
42-49: LGTM!The return type change from
AggregationCursortoPromise<...[]>correctly reflects the refactoring from cursor-based$facetaggregation to parallelPromise.allqueries for DocumentDB compatibility.packages/model-typings/src/models/IRoomsModel.ts (1)
321-328: LGTM!The return type update aligns with the implementation in
Rooms.tsand the caller inteam/service.tscorrectly handles the new Promise-based return structure.apps/meteor/ee/server/models/raw/Users.ts (3)
28-52: LGTM!The
$lookuprefactoring correctly replaces the pipeline-based correlation withlocalField/foreignFieldjoin, followed by$addFields/$filterto filter department entries matching the provideddepartmentId. The subsequent$match: { department: { $size: 1 } }correctly ensures exactly one matching department.
70-82: LGTM!The subscription filtering is correctly refactored to use post-join
$filterwith the appropriate livechat conditions (t: 'l',open: true,onHold != true).
111-111: LGTM!Correct usage of
getAllowDiskUse()spread withreadPreferencefor DocumentDB compatibility.packages/models/src/models/Sessions.ts (4)
126-163: LGTM!The
$$REMOVEreplacement withnull+$filteris correctly implemented. ThefilterNullsstage properly removes null values fromusersListbefore the count operation.
837-846: LGTM!The
$facetreplacement withPromise.allcorrectly separates data retrieval and count aggregation. ThecountResult[0]?.total || 0pattern safely handles empty result sets.
951-970: LGTM!Consistent implementation with
aggregateSessionsByUserId. The parallel queries correctly split the paginated data and count operations.
1140-1140: LGTM!The
groups.filterNullsstage is correctly inserted betweenlistGroupandcountGroupto filter out null values before counting.docs/documentdb-compatibility.md (1)
1-92: LGTM!Comprehensive documentation that clearly explains the DocumentDB compatibility patterns, affected files, and solutions implemented. The structure with problem/solution format makes it easy to understand each change.
packages/models/src/models/Rooms.ts (3)
608-611: LGTM!Correct usage of
getAllowDiskUse()spread withreadPreferencefor DocumentDB compatibility.
2276-2281: LGTM!The
$facetreplacement withPromise.allcorrectly separates paginated results and count queries. The return structure[{ totalCount: countResult, paginatedResults }]matches the interface definition and is correctly handled by the caller inteam/service.ts.
2239-2259: The subscription type filter condition is correct and preserves original semantics.The change from
let/pipelinetolocalField/foreignField+$addFieldsis functionally equivalent. Both implementations:
- Filter subscriptions to the current user:
$eq: ['$$sub.u._id', userId]- Exclude 'c' type subscriptions:
$ne: ['$$sub.t', 'c']- Allow channels (
t: 'c') to be visible to everyone- Require subscriptions for private rooms and discussions
The refactoring simplifies the query from a complex let/pipeline approach to a straightforward lookup with post-processing. Since the final result only checks subscription array size (not contents), the behavior is identical. The filter logic is intentional and correct.
packages/models/src/models/LivechatRooms.ts (2)
108-216: The helper-based aggregate option pattern is applied consistently.
getQueueMetrics()still returns[{ sortedResults, totalCount }], which matches the queue caller, and the same option pattern is now reused infindAllNumberOfTransferredRooms().Also applies to: 704-704
1531-1565: This cleanup keepsfullTypesfree of sentinels.The intermediate arrays are normalized before the final union, so callers do not see placeholder elements leaking through.
packages/models/src/models/Analytics.ts (1)
298-318: The split count/page refactor preserves the public contract.Returning
[{ channels, total }]keeps the downstream caller simple while reusing the same base pipeline and option bundle for both queries.
| - name: Configure external MongoDB URL | ||
| env: | ||
| CI_MONGO_URL: ${{ secrets.CI_MONGO_URL }} | ||
| if: env.CI_MONGO_URL != '' | ||
| run: | | ||
| DB_NAME="rocketchat_${{ inputs.type }}_${{ matrix.shard }}_${{ github.run_id }}_${{ github.run_attempt }}" | ||
| echo "MONGO_URL=${CI_MONGO_URL}/${DB_NAME}" >> $GITHUB_ENV | ||
| echo "DOCKER_MONGO_URL=${CI_MONGO_URL}/${DB_NAME}" >> $GITHUB_ENV | ||
| echo "EXTERNAL_MONGO=true" >> $GITHUB_ENV |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there's any cleanup mechanism for external MongoDB databases in the workflow or elsewhere
rg -n "drop.*database|dropDatabase|cleanup.*mongo|mongo.*cleanup" .github/Repository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
#!/bin/bash
# Get the full workflow file to check for any cleanup steps or jobs
cat .github/workflows/ci-test-e2e.yml | tail -100Repository: RocketChat/Rocket.Chat
Length of output: 4310
🏁 Script executed:
#!/bin/bash
# Search entire repository (not just .github/) for MongoDB cleanup patterns
rg -i "mongodump|mongodb.*cleanup|cleanup.*mongodb|drop.*db|dropdb|truncate.*collection" --type yaml --type sh --type jsRepository: RocketChat/Rocket.Chat
Length of output: 200
🏁 Script executed:
#!/bin/bash
# Check if there's a separate cleanup workflow or script
fd -e yml -e yaml -e sh | xargs grep -l "cleanup\|teardown\|mongo" | head -20Repository: RocketChat/Rocket.Chat
Length of output: 268
🏁 Script executed:
#!/bin/bash
# Check the docker-compose-ci.yml for any cleanup or teardown configurations
head -50 docker-compose-ci.ymlRepository: RocketChat/Rocket.Chat
Length of output: 1840
🏁 Script executed:
#!/bin/bash
# Check if there are any cleanup hooks or scripts referenced
grep -r "external.*mongo\|CI_MONGO_URL" . --include="*.yml" --include="*.yaml" -B 2 -A 2 | head -50Repository: RocketChat/Rocket.Chat
Length of output: 2956
🏁 Script executed:
#!/bin/bash
# Search for any comments or documentation about external MongoDB cleanup expectations
rg -i "external.*mongo.*cleanup|cleanup.*external.*mongo|external.*mongo.*manual" --type md --type txt --type ymlRepository: RocketChat/Rocket.Chat
Length of output: 93
🏁 Script executed:
#!/antml/bin/bash
# Check if there are any GitHub issues or discussion references
grep -r "cleanup\|orphan" .github/workflows/ | grep -i "mongo\|database"Repository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
#!/bin/bash
# Look at the full "Configure external MongoDB URL" step and surrounding context
sed -n '155,175p' .github/workflows/ci-test-e2e.ymlRepository: RocketChat/Rocket.Chat
Length of output: 880
Add database cleanup for external MongoDB test runs.
When using an external MongoDB (via CI_MONGO_URL), each run creates a new database with a unique name (rocketchat_${{ inputs.type }}_${{ matrix.shard }}_${{ github.run_id }}_${{ github.run_attempt }}), but there is no cleanup step to drop these databases after tests complete. The unique naming prevents collisions across concurrent runs, but without cleanup, databases will accumulate on the external MongoDB instance over time, consuming storage and potentially impacting performance.
Consider adding a cleanup step (or job) that drops the test database after tests complete, especially for external MongoDB configurations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci-test-e2e.yml around lines 160 - 168, Add a teardown
step that drops the dynamically created external MongoDB database when
EXTERNAL_MONGO is true: after tests complete, run a job or a post-step that
reads CI_MONGO_URL and DB_NAME (the env vars set in the existing step:
CI_MONGO_URL, DB_NAME, DOCKER_MONGO_URL, EXTERNAL_MONGO) and invokes the Mongo
client (mongosh/mongo) to connect to CI_MONGO_URL and run a dropDatabase()
against the DB_NAME; ensure this cleanup is conditional on EXTERNAL_MONGO=true,
uses the same secret (CI_MONGO_URL) for auth, and treats failures
non-blocking/logged so cleanup does not mask test results.
| $match: { | ||
| ...Object.fromEntries(Object.entries(extraConditions).map(([k, v]) => [`user.${k}`, v])), | ||
| 'user.active': true, | ||
| 'user.username': { | ||
| $exists: true, | ||
| ...(exceptions.length > 0 && { $nin: exceptions }), | ||
| }, | ||
| ...(searchTerm && | ||
| orStatement.length > 0 && { | ||
| $or: orStatement.map((cond) => Object.fromEntries(Object.entries(cond).map(([k, v]) => [`user.${k}`, v]))), | ||
| }), |
There was a problem hiding this comment.
Preserve logical operators when remapping extraConditions.
Object.entries(extraConditions) only works for flat field predicates. A valid Filter<IUser> like { $or: [...] } becomes user.$or, so compound caller filters stop matching here. Prefix only field paths and compose the prefixed filter with the search clause under $and.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/models/src/models/Subscriptions.ts` around lines 499 - 509, The
current $match remapping uses Object.entries(extraConditions) which flattens
nested/compound operators (e.g., $or) into user.$or and breaks filters; fix by
building a prefixed filter that only prefixes plain field paths (keys that do
not start with $) in extraConditions to `user.<field>` while leaving operator
keys (keys starting with $) intact, produce a single prefixedExtraConditions
object, and then combine prefixedExtraConditions with the search clause via an
$and (instead of merging them directly) in the $match; update the remapping
logic around extraConditions, orStatement, searchTerm and exceptions in the
$match block so compound Filters (like { $or: [...] }) are preserved and applied
correctly to `user.*` fields.
| const paginationStages = [{ $sort: options?.sort }, { $skip: options?.skip }, ...(options?.limit ? [{ $limit: options.limit }] : [])]; | ||
|
|
||
| const [sortedResults, countResult] = await Promise.all([ | ||
| this.col.aggregate<T & { departments: string[] }>([...aggregate, ...paginationStages]).toArray(), | ||
| this.col.aggregate<{ total: number }>([...aggregate, { $count: 'total' }]).toArray(), |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does MongoDB aggregation accept $sort: undefinedor$skip: undefined in a pipeline stage?
💡 Result:
No, MongoDB aggregation does not accept $sort: undefined or $skip: undefined in a pipeline stage. These stages have strict type requirements that undefined violates, resulting in errors.
Citations:
- 1: https://www.mongodb.com/docs/v5.3/reference/operator/aggregation/sort/
- 2: https://www.mongodb.com/docs/manual/reference/operator/aggregation/sort/
- 3: https://stackoverflow.com/questions/62874918/mongo-error-sort-stage-must-have-at-least-one-sort-key
- 4: https://www.mongodb.com/docs/manual/reference/operator/aggregation/skip/
- 5: https://www.mongodb.com/docs/v8.0/reference/operator/aggregation/skip/
- 6: https://stackoverflow.com/questions/28071517/the-sort-key-specification-must-be-an-object
🏁 Script executed:
# Check the file and examine lines around 289-293
sed -n '280,300p' packages/models/src/models/Users.ts | cat -nRepository: RocketChat/Rocket.Chat
Length of output: 1081
🏁 Script executed:
# Find the function signature to understand the options parameter
sed -n '248,295p' packages/models/src/models/Users.ts | cat -nRepository: RocketChat/Rocket.Chat
Length of output: 1828
🏁 Script executed:
# Search for callers of findAgentsWithDepartments to see if sort/skip are optional
rg "findAgentsWithDepartments" --type ts -B 2 -A 2Repository: RocketChat/Rocket.Chat
Length of output: 1324
🏁 Script executed:
# Check all callers of findAgentsWithDepartments to see if any omit the options parameter
rg "findAgentsWithDepartments\(" --type ts -A 3 | head -50Repository: RocketChat/Rocket.Chat
Length of output: 380
🏁 Script executed:
# Check the FindOptions type to confirm sort and skip are optional
rg "type FindOptions|interface FindOptions" --type ts -A 10Repository: RocketChat/Rocket.Chat
Length of output: 1908
🏁 Script executed:
# Search more broadly for all calls to findAgentsWithDepartments across the codebase
rg "findAgentsWithDepartments" --type tsRepository: RocketChat/Rocket.Chat
Length of output: 393
🏁 Script executed:
# Verify the exact MongoDB behavior by checking error scenarios
# Let's also see if there are any other places in the codebase with similar pagination stage construction
rg "const paginationStages|paginationStages\s*=" --type ts -B 2 -A 2Repository: RocketChat/Rocket.Chat
Length of output: 448
Guard optional pagination stages before appending them.
Line 289 unconditionally includes $sort and $skip stages with potentially undefined values. MongoDB aggregation rejects undefined values in these stages. Since options is optional and all its properties (sort, skip, limit) are optional, callers can omit them entirely or provide only some fields, resulting in invalid pipeline stages at runtime. Build each stage only when its value is present.
🛠️ Suggested fix
- const paginationStages = [{ $sort: options?.sort }, { $skip: options?.skip }, ...(options?.limit ? [{ $limit: options.limit }] : [])];
+ const paginationStages: Document[] = [];
+
+ if (options?.sort) {
+ paginationStages.push({ $sort: options.sort as Record<string, SortDirection> });
+ }
+
+ if (typeof options?.skip === 'number') {
+ paginationStages.push({ $skip: options.skip });
+ }
+
+ if (typeof options?.limit === 'number') {
+ paginationStages.push({ $limit: options.limit });
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/models/src/models/Users.ts` around lines 289 - 293, The aggregation
pipeline currently builds paginationStages with unconditional { $sort:
options?.sort } and { $skip: options?.skip }, which may insert undefined values
and break MongoDB; change the construction in the method that uses
paginationStages (the code that calls this.col.aggregate([...aggregate,
...paginationStages]) and this.col.aggregate([...aggregate, { $count: 'total'
}])) to only include each stage when its value is present (e.g., push or
conditionally spread { $sort: options.sort } only if options?.sort !==
undefined, { $skip: options.skip } only if options?.skip !== undefined, and {
$limit: options.limit } only if options?.limit !== undefined) so the pipeline
contains no stages with undefined values.
| $addFields: { | ||
| subs: { | ||
| $filter: { | ||
| input: '$subs', | ||
| as: 'sub', | ||
| cond: { | ||
| $and: [ | ||
| { $eq: ['$$sub.open', true] }, | ||
| { $ne: ['$$sub.onHold', true] }, | ||
| ...(department ? [{ $eq: ['$$sub.department', department] }] : []), | ||
| ], | ||
| }, |
There was a problem hiding this comment.
Only count livechat subscriptions in least-busy routing.
This filter now counts every open subscription the agent has, including channels and DMs. getAgentAndAmountOngoingChats() later in this file still guards on $$sub.t === 'l'; without the same check here, routing is no longer based on active livechat load.
🛠️ Suggested fix
$and: [
+ { $eq: ['$$sub.t', 'l'] },
{ $eq: ['$$sub.open', true] },
{ $ne: ['$$sub.onHold', true] },
...(department ? [{ $eq: ['$$sub.department', department] }] : []),
],📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $addFields: { | |
| subs: { | |
| $filter: { | |
| input: '$subs', | |
| as: 'sub', | |
| cond: { | |
| $and: [ | |
| { $eq: ['$$sub.open', true] }, | |
| { $ne: ['$$sub.onHold', true] }, | |
| ...(department ? [{ $eq: ['$$sub.department', department] }] : []), | |
| ], | |
| }, | |
| $addFields: { | |
| subs: { | |
| $filter: { | |
| input: '$subs', | |
| as: 'sub', | |
| cond: { | |
| $and: [ | |
| { $eq: ['$$sub.t', 'l'] }, | |
| { $eq: ['$$sub.open', true] }, | |
| { $ne: ['$$sub.onHold', true] }, | |
| ...(department ? [{ $eq: ['$$sub.department', department] }] : []), | |
| ], |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/models/src/models/Users.ts` around lines 606 - 617, The subs $filter
inside the $addFields stage is currently including all open subscriptions
(channels, DMs) whereas getAgentAndAmountOngoingChats() expects only livechat
items (it checks $$sub.t === 'l'); update the $and conditions in the subs filter
(the $filter under $addFields -> subs) to include { $eq: ['$$sub.t', 'l'] }
alongside the existing { $eq: ['$$sub.open', true] } and { $ne: ['$$sub.onHold',
true] } (keeping the department conditional untouched) so routing counts only
livechat subscriptions.
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
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=".github/workflows/ci-test-e2e.yml">
<violation number="1" location=".github/workflows/ci-test-e2e.yml:168">
P2: `QUERY` extraction can prepend `/?` after trimming `BASE`, producing malformed Mongo URLs like `.../<db>/?...`. Strip the leading `/` from `QUERY` before composing `FULL_URL`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
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 @.github/workflows/ci-test-e2e.yml:
- Around line 164-170: The step-level if uses secrets.CI_MONGO_URL (invalid);
change the workflow to expose the secret as an environment variable and refer to
it via env in the step-level if. Specifically, set an env entry (e.g.,
CI_MONGO_URL: ${{ secrets.CI_MONGO_URL }}) at the job or workflow level and then
update the "Connect to NetBird" step's if to check env.CI_MONGO_URL; keep the
existing with: inputs (setup-key, preshared-key, test-ip) but ensure those
secret references remain in env if they must be used in conditions.
🪄 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: c92dda1d-639a-41bf-bd5c-9f88c50bc474
📒 Files selected for processing (2)
.github/workflows/ci-test-e2e.yml.github/workflows/ci.yml
📜 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). (3)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🧰 Additional context used
🪛 actionlint (1.7.11)
.github/workflows/ci-test-e2e.yml
[error] 165-165: context "secrets" is not allowed here. available contexts are "env", "github", "inputs", "job", "matrix", "needs", "runner", "steps", "strategy", "vars". see https://docs.github.com/en/actions/learn-github-actions/contexts#context-availability for more details
(expression)
🔇 Additional comments (4)
.github/workflows/ci-test-e2e.yml (3)
172-185: External Mongo DB lifecycle still lacks teardown.This still creates per-run DB names but does not show a cleanup/drop path after tests. That can accumulate databases on the external cluster over time.
60-65: Optional secret contract is correctly defined.Declaring these new workflow-call secrets as optional is the right choice for fork/CE compatibility.
206-210: Good container startup adjustment for external Mongo mode.Conditionally applying
--scale mongo=0whenEXTERNAL_MONGO=trueis a clean way to avoid starting the local Mongo service.Also applies to: 218-222
.github/workflows/ci.yml (1)
503-505: Consistent secret forwarding across all E2E callers looks good.Good update: these additions are uniformly applied across CE/EE API/UI workflow-call jobs, matching the reusable workflow interface.
Also applies to: 522-524, 550-552, 573-575, 596-598, 627-629
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
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=".github/workflows/ci-test-e2e.yml">
<violation number="1" location=".github/workflows/ci-test-e2e.yml:166">
P1: Pin the NetBird GitHub Action to an immutable commit SHA instead of `@main` to avoid supply-chain risk and non-reproducible CI runs.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| - name: Connect to NetBird | ||
| if: secrets.CI_MONGO_URL != '' | ||
| uses: RocketChat/netbird-connect@main |
There was a problem hiding this comment.
P1: Pin the NetBird GitHub Action to an immutable commit SHA instead of @main to avoid supply-chain risk and non-reproducible CI runs.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/ci-test-e2e.yml, line 166:
<comment>Pin the NetBird GitHub Action to an immutable commit SHA instead of `@main` to avoid supply-chain risk and non-reproducible CI runs.</comment>
<file context>
@@ -157,6 +161,14 @@ jobs:
+ - name: Connect to NetBird
+ if: secrets.CI_MONGO_URL != ''
+ uses: RocketChat/netbird-connect@main
+ with:
+ setup-key: ${{ secrets.NETBIRD_SETUP_KEY }}
</file context>
a7dc7bd to
85b5097
Compare
…xcept (Subscriptions.ts) Replace two $lookup stages using `let` parameter with DocumentDB 8.0 compatible `localField/foreignField` joins followed by $filter/$match stages for post-lookup filtering. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…nt and getLastAvailableAgentRouted (Users.ts) Replace $lookup stages using `let` parameter with DocumentDB 8.0 compatible `localField/foreignField` joins followed by $addFields/$filter stages for post-lookup conditional filtering. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace $lookup stages using `let` parameter in getQueueMetrics, getAnalyticsMetricsBetweenDateWithMessages, and getAnalyticsBetweenDate with DocumentDB 8.0 compatible localField/foreignField joins and $filter/$addFields for post-lookup filtering. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…(Rooms.ts) Replace $lookup with let/pipeline with localField/foreignField join and $addFields/$filter for post-lookup filtering. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…s (ee/Users.ts) Replace $lookup with let/pipeline and pipeline-only $lookup with DocumentDB 8.0 compatible localField/foreignField joins and $addFields/$filter for post-lookup filtering. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…redByRole Replace $lookup with let/pipeline with localField/foreignField join and $addFields/$filter for post-lookup room filtering. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Split $facet into two parallel queries using Promise.all() for DocumentDB 8.0 compatibility. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Split $facet into parallel queries with Promise.all(). Updated method signature from AggregationCursor to Promise and updated caller. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…chatBusinessHours.ts) Split $facet into parallel queries with Promise.all() for DocumentDB 8.0 compatibility. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Split $facet into parallel queries with Promise.all(). Updated method signature from AggregationCursor to Promise and updated caller. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Split $facet in aggregateSessionsByUserId and aggregateSessionsAndPopulate into parallel queries with Promise.all(). Removed unused WithItemCount import. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ers.ts) Split $facet into parallel queries with Promise.all() for DocumentDB 8.0 compatibility. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…tment.ts Replace $lookup with pipeline (without let) with localField/foreignField join and $addFields/$filter for DocumentDB 8.0 compatibility. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…entStatuses (LivechatDepartment.ts) Replace $$REMOVE with null + $filter to strip null values for DocumentDB 8.0 compatibility. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…hatRooms.ts) Replace $$REMOVE with null + $filter to strip null values for DocumentDB 8.0 compatibility. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
….ts) Replace $$REMOVE with null + $filter to strip null values. Added filterNulls stage between listGroup and countGroup in aggregation pipelines. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…DB env var
DocumentDB does not support allowDiskUse for find commands and uses sort
merge by default for aggregations. The new getAllowDiskUse() utility returns
{ allowDiskUse: true } on MongoDB or {} when DOCUMENTDB=true, omitting the
option entirely. Also adds docs/documentdb-compatibility.md documenting all
changes made in this PR.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add optional CI_MONGO_URL secret to ci-test-e2e workflow - Generate unique DB name per job/run to avoid stale data on re-runs - Skip local mongo container (--scale mongo=0) when external URL is set - Use DOCKER_MONGO_URL in docker-compose to avoid conflict with workflow-level MONGO_URL (localhost vs docker network hostname) - Make mongo dependency optional (required: false) in docker-compose Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… name Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ird connection step
ggazzo
force-pushed
the
chore/document-db
branch
from
79c6f9f to
e01dc85
Compare
…d clean up coverage directory syntax
ggazzo
force-pushed
the
chore/document-db
branch
from
e01dc85 to
5a06c52
Compare
…tBird connection configuration
…rrors DocumentDB only supports one index build at a time per collection. Monkey-patches Collection.prototype.createIndex/createIndexes to queue calls per collection when DOCUMENTDB=true, covering both Meteor packages and Rocket.Chat BaseRaw models. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…y loading The previous patch in @rocket.chat/models loaded too late — Meteor packages (accounts-base, accounts-password) create indexes before the app's main module runs. Moving the native driver monkey-patch into rocketchat:mongo-config ensures it runs at position 53 in the Meteor load order, before accounts-base (56) and accounts-password (69). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Meteor bundles its own copy of the mongodb driver in npm-mongo/node_modules/mongodb, separate from the app's node_modules/mongodb. Patching Collection.prototype from the app's copy does not reach Meteor's internal Collection instances used by accounts-base / accounts-password. Patch MongoConnection.createIndexAsync in rocketchat:mongo-config to cover all Meteor package index calls, and share a queue via globalThis with the native driver patch in @rocket.chat/models so BaseRaw and Meteor calls serialize together. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
setControl() is fire-and-forget (was prefixed with void) and is called from
multiple paths in quick succession (getControl fallback + migrateDatabase
initial setup). Two parallel updateMany upserts on { _id: 'control' } can
race; the loser gets E11000. The rejected promise becomes an unhandled
rejection and crashes the process under EXIT_UNHANDLEDPROMISEREJECTION (CI).
Catch the rejection and ignore E11000 specifically — losing the upsert race
is safe because the winning caller already wrote the document. Log other
errors instead of swallowing them.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
MongoDB and DocumentDB require read preference 'primary' inside transactions. When the client is configured with 'secondaryPreferred' (as in the CI connection to DocumentDB), transactions fail with "Read preference in a transaction must be primary, not: secondaryPreferred". Export transactionOptions from database/utils with readPreference: primary and pass it to startTransaction() in all four callsites: - wrapInSessionTransaction (database/utils.ts) - verifyContactChannel (ee/server/patches) - QueueManager (app/livechat/server/lib) - closeRoom (app/livechat/server/lib) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…tartup Two CI-only adjustments to make tests deterministic against DocumentDB: 1. Override readPreference=primary in the test MONGO_URL. The CI secret has readPreference=secondaryPreferred which causes read-after-write inconsistency in tests (e.g. user authenticates, immediately reads via findOneById, secondary returns null). Last value in the connection string wins per MongoDB spec, so appending overrides without touching the secret. secondaryPreferred behavior is an application concern and should not be asserted via tests that need read-your-writes semantics. 2. Bump healthcheck start_period from 60s to 180s. Startup against an external DocumentDB cluster is much slower than the local Mongo container; the rocketchat service was being marked unhealthy before it finished initial setup (indexes, migrations, settings load). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The MongoDB Node.js driver rejects connection strings with duplicate URI options (MongoInvalidArgumentError: URI option "readPreference" cannot appear more than once). Last-value-wins is per the connection string spec but not enforced by node-mongodb-native. Strip any existing readPreference= entry with sed before appending our own, preserving other options like replicaSet and retryWrites. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Forcing readPreference=primary on the test MongoDB URL caused all reads (including the many startup queries for indexes, migrations, settings, permissions, roles, etc.) to hit the primary node only, overloading DocumentDB and slowing rocketchat startup from ~36s to >190s — past the healthcheck timeout. Trade-off accepted: keep secondaryPreferred (production-realistic, fast startup) and live with read-after-write test failures in suites that create-then-immediately-read via findOneById. Transactions still get readPreference: primary via startTransaction(transactionOptions) which was added in c4b2dcb. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
SettingsRegistry.add does a non-atomic check (this.store.getSetting) then insertOne. When the main rocketchat process and the EE microservices boot in parallel against the same database, both can race past the in-memory check and call insertOne for the same setting _id; the loser gets E11000 and crashes the process under EXIT_UNHANDLEDPROMISEREJECTION. Catch the rejection and ignore E11000 specifically — the setting now exists, which is the desired outcome. Re-throw any other error. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
DocumentDB rejects two index categories that Rocket.Chat models declare:
1. Text indexes (e.g. { msg: 'text' } in Messages) — DocumentDB has no
native text search.
2. Partial indexes whose partialFilterExpression uses operators outside
the supported subset ($eq, $gt, $gte, $lt, $lte). The codebase relies
on $exists in 16 indexes across 8 models (Messages, LivechatRooms,
LivechatVisitors, LivechatContacts, Sessions, Rooms, Users), all of
which currently fail with "Bad query specified" at boot.
Add a centralized filter in packages/models/src/filterIndexes.ts that
strips these indexes when DOCUMENTDB=true and logs what was skipped.
Wire it into BaseRaw.createIndexes() so every model benefits without
touching individual model files.
Trade-off: queries that depended on the skipped indexes fall back to
collection scans. Functionality is preserved; performance degrades on
DocumentDB. Future work could replace some $exists partial filters with
sparse: true equivalents (close in semantics) or rewrite them into the
$gt/$lt subset DocumentDB allows.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… insert Same race as in `add` (fixed in 3d30c8c) but in `addGroup`: the in-memory `this.store.has(_id)` check is non-atomic, so parallel boot of main app + EE microservices can race on insertOne and crash with E11000. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 participant
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
New Features
Improvements