refactor: move index creation to onServerVersionChange#39253
Open
refactor: move index creation to onServerVersionChange#39253
Conversation
Contributor
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRefactors permission upsertion to batched bulk operations with retry logic, defers index creation via an Emitter-based registry, replaces a server-version-change callback with a predicate, adds concurrent chunked app loading, adjusts settings initialization loop, registers the Trash model, and adds an ESLint VS Code setting and an emitter dependency. Changes
Sequence Diagram(s)sequenceDiagram
participant SettingsSvc as Settings Service
participant SettingsColl as Settings Collection
participant PermSvc as Permissions Service
participant DB as Database
SettingsSvc->>SettingsColl: find().toArray()
SettingsColl-->>SettingsSvc: visible settings list
loop construct permission docs
SettingsSvc->>PermSvc: buildSettingPermissionDoc(setting, previousPermissions)
PermSvc->>DB: queue updateOne upsert ops
end
PermSvc->>DB: bulkWrite(batch of upsert ops, ordered:false)
alt E11000 duplicate key
PermSvc->>DB: retry batch with updateOne (upsert:false)
DB-->>PermSvc: retry result
end
PermSvc->>DB: deleteMany({ _id: { $in: obsoleteIds } })
DB-->>PermSvc: deletion result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 #39253 +/- ##
===========================================
+ Coverage 70.52% 70.53% +0.01%
===========================================
Files 3263 3263
Lines 116664 116666 +2
Branches 21040 21062 +22
===========================================
+ Hits 82278 82295 +17
+ Misses 32328 32315 -13
+ Partials 2058 2056 -2
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/models/src/proxify.ts (1)
61-65: Remove the new implementation comment block.Please keep this behavior self-evident via naming/call-site usage rather than inline comments.
As per coding guidelines, "Avoid code comments in the implementation".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/models/src/proxify.ts` around lines 61 - 65, Remove the top-of-file comment block that describes "Runs createIndexes() on all registered models..." and any similar implementation-level comments above the function that invokes createIndexes on registered models (the routine used from onServerVersionChange); leave the function and its name/usage intact so the behavior is conveyed by the function name and call sites rather than inline comments.apps/meteor/app/authorization/server/functions/upsertPermissions.ts (1)
108-117: Drop the newly added inline implementation comments in this batch loop.Prefer self-descriptive extraction/naming over these new comments.
As per coding guidelines, "Avoid code comments in the implementation".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/authorization/server/functions/upsertPermissions.ts` around lines 108 - 117, Remove the inline implementation comments inside the batch loop and replace them with clearer, self-descriptive names or extracted helper functions; specifically, in the for-loop that iterates over updateOps with step BULK_WRITE_BATCH_SIZE and calls Permissions.col.bulkWrite(batch, { ordered: false }), eliminate comments like "E11000 duplicate key: retry without upsert for this batch (doc already exists)" and instead extract the retry logic into a well-named helper (e.g., retryWithoutUpsertForDuplicateKey) or add a descriptive variable name for the Promise.all call that invokes Permissions.updateOne(op.updateOne.filter, op.updateOne.update) so the code is self-documenting without comments. Ensure the behavior (catch E11000 and retry updates) remains unchanged.apps/meteor/ee/server/apps/orchestrator.js (1)
203-203: Avoid inline implementation comments for lint suppression.Please refactor this loop to avoid the added inline comment at Line 203.
As per coding guidelines, "Avoid code comments in the implementation".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/ee/server/apps/orchestrator.js` at line 203, The inline eslint-disable-next-line no-await-in-loop is suppressing a lint rule; instead refactor the loop that follows it to avoid awaiting inside a loop by mapping the iterable to async tasks and awaiting Promise.all (e.g. replace the loop after the comment with: const tasks = items.map(async item => { /* original loop body using item */ }); await Promise.all(tasks)); if true sequential behavior is required, extract the body into a named async helper (e.g. async function processItem(item) { ... }) and use a controlled-sequence implementation (for example await items.reduce((p, it) => p.then(() => processItem(it)), Promise.resolve())) or use a concurrency helper like p-map—refer to the loop that had the eslint-disable-next-line no-await-in-loop to locate where to apply this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/app/authorization/server/functions/upsertPermissions.ts`:
- Around line 49-55: In createSettingPermission, the duplicate-key retry logic
is inverted: the catch block currently retries only when the error is NOT
E11000; instead, detect E11000 (duplicate key) and perform the retry of
Permissions.updateOne({_id: permissionId}, {$set: doc}, {upsert: true}) only
when the caught error message includes 'E11000'; leave other errors to
propagate. Update the catch to check (e as Error).message.includes('E11000') and
call Permissions.updateOne in that branch, otherwise rethrow the error.
In `@packages/models/src/proxify.ts`:
- Around line 75-77: The catch in ensureAllIndexes currently only logs failures
(console.warn(`ensureAllIndexes: failed for model ${name}`, err)) which hides
errors and makes callers think indexing succeeded; change ensureAllIndexes to
propagate failures instead of swallowing them by either rethrowing the caught
error (including model name) or accumulating errors and rejecting with an
AggregateError after processing all models so callers observe failure; update
the catch block that references name and err (and remove or replace the
console.warn) to throw or record a contextual Error/AggregateError with the
model name and original err.
---
Nitpick comments:
In `@apps/meteor/app/authorization/server/functions/upsertPermissions.ts`:
- Around line 108-117: Remove the inline implementation comments inside the
batch loop and replace them with clearer, self-descriptive names or extracted
helper functions; specifically, in the for-loop that iterates over updateOps
with step BULK_WRITE_BATCH_SIZE and calls Permissions.col.bulkWrite(batch, {
ordered: false }), eliminate comments like "E11000 duplicate key: retry without
upsert for this batch (doc already exists)" and instead extract the retry logic
into a well-named helper (e.g., retryWithoutUpsertForDuplicateKey) or add a
descriptive variable name for the Promise.all call that invokes
Permissions.updateOne(op.updateOne.filter, op.updateOne.update) so the code is
self-documenting without comments. Ensure the behavior (catch E11000 and retry
updates) remains unchanged.
In `@apps/meteor/ee/server/apps/orchestrator.js`:
- Line 203: The inline eslint-disable-next-line no-await-in-loop is suppressing
a lint rule; instead refactor the loop that follows it to avoid awaiting inside
a loop by mapping the iterable to async tasks and awaiting Promise.all (e.g.
replace the loop after the comment with: const tasks = items.map(async item => {
/* original loop body using item */ }); await Promise.all(tasks)); if true
sequential behavior is required, extract the body into a named async helper
(e.g. async function processItem(item) { ... }) and use a controlled-sequence
implementation (for example await items.reduce((p, it) => p.then(() =>
processItem(it)), Promise.resolve())) or use a concurrency helper like
p-map—refer to the loop that had the eslint-disable-next-line no-await-in-loop
to locate where to apply this change.
In `@packages/models/src/proxify.ts`:
- Around line 61-65: Remove the top-of-file comment block that describes "Runs
createIndexes() on all registered models..." and any similar
implementation-level comments above the function that invokes createIndexes on
registered models (the routine used from onServerVersionChange); leave the
function and its name/usage intact so the behavior is conveyed by the function
name and call sites rather than inline comments.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.vscode/settings.jsonapps/meteor/app/authorization/server/functions/upsertPermissions.tsapps/meteor/app/livechat/server/lib/closeRoom.tsapps/meteor/app/settings/server/startup.tsapps/meteor/ee/server/apps/orchestrator.jsapps/meteor/server/startup/migrations/xrun.tspackages/models/src/index.tspackages/models/src/models/BaseRaw.tspackages/models/src/proxify.ts
💤 Files with no reviewable changes (1)
- packages/models/src/models/BaseRaw.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cubic · AI code reviewer
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
packages/models/src/index.tsapps/meteor/app/authorization/server/functions/upsertPermissions.tsapps/meteor/server/startup/migrations/xrun.tsapps/meteor/app/settings/server/startup.tsapps/meteor/app/livechat/server/lib/closeRoom.tsapps/meteor/ee/server/apps/orchestrator.jspackages/models/src/proxify.ts
🧠 Learnings (13)
📓 Common learnings
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.
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: In Rocket.Chat PRs, keep feature PRs free of unrelated lockfile-only dependency bumps; prefer reverting lockfile drift or isolating such bumps into a separate "chore" commit/PR, and always use yarn install --immutable with the Yarn version pinned in package.json via Corepack.
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
packages/models/src/index.tsapps/meteor/app/authorization/server/functions/upsertPermissions.tsapps/meteor/server/startup/migrations/xrun.tsapps/meteor/app/settings/server/startup.tsapps/meteor/app/livechat/server/lib/closeRoom.tspackages/models/src/proxify.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
packages/models/src/index.tsapps/meteor/app/authorization/server/functions/upsertPermissions.tsapps/meteor/server/startup/migrations/xrun.tsapps/meteor/app/settings/server/startup.tsapps/meteor/app/livechat/server/lib/closeRoom.tspackages/models/src/proxify.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/server/startup/migrations/xrun.tsapps/meteor/app/livechat/server/lib/closeRoom.ts
📚 Learning: 2026-02-24T19:05:56.710Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: Rocket.Chat repo context: When a workspace manifest on develop already pins a dependency version (e.g., packages/web-ui-registration → "rocket.chat/ui-contexts": "27.0.1"), a lockfile change in a feature PR that upgrades only that dependency’s resolution is considered a manifest-driven sync and can be kept, preferably as a small "chore: sync yarn.lock with manifests" commit.
Applied to files:
apps/meteor/server/startup/migrations/xrun.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
apps/meteor/server/startup/migrations/xrun.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/server/startup/migrations/xrun.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/server/startup/migrations/xrun.tsapps/meteor/app/settings/server/startup.ts
📚 Learning: 2025-11-05T20:53:57.761Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37357
File: apps/meteor/ee/server/startup/federation.ts:39-74
Timestamp: 2025-11-05T20:53:57.761Z
Learning: In Rocket.Chat (apps/meteor/app/settings/server/CachedSettings.ts), the settings.watchMultiple() method immediately invokes its callback with current values if all requested settings exist in the store, then continues watching for subsequent changes. It does not wait for a setting to change before the first invocation.
Applied to files:
apps/meteor/server/startup/migrations/xrun.tsapps/meteor/app/settings/server/startup.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/app/livechat/server/lib/closeRoom.ts
📚 Learning: 2025-10-28T16:53:42.761Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37205
File: ee/packages/federation-matrix/src/FederationMatrix.ts:296-301
Timestamp: 2025-10-28T16:53:42.761Z
Learning: In the Rocket.Chat federation-matrix integration (ee/packages/federation-matrix/), the createRoom method from rocket.chat/federation-sdk will support a 4-argument signature (userId, roomName, visibility, displayName) in newer versions. Code using this 4-argument call is forward-compatible with planned library updates and should not be flagged as an error.
Applied to files:
apps/meteor/app/livechat/server/lib/closeRoom.ts
📚 Learning: 2026-03-03T11:11:45.505Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 39230
File: apps/meteor/app/api/server/v1/chat.ts:214-222
Timestamp: 2026-03-03T11:11:45.505Z
Learning: In apps/meteor/server/lib/moderation/reportMessage.ts, the reportMessage function validates that description is not empty or whitespace-only with `if (!description.trim())`. When migrating the chat.reportMessage endpoint to OpenAPI, adding minLength validation to the schema preserves this existing behavior.
Applied to files:
apps/meteor/app/livechat/server/lib/closeRoom.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: UserBridge.doGetUserRoomIds in packages/apps-engine/src/server/bridges/UserBridge.ts has a bug where it implicitly returns undefined when the app lacks read permission (missing return statement in the else case of the permission check).
Applied to files:
apps/meteor/app/livechat/server/lib/closeRoom.ts
🧬 Code graph analysis (2)
apps/meteor/app/authorization/server/functions/upsertPermissions.ts (1)
packages/models/src/index.ts (1)
Permissions(176-176)
apps/meteor/server/startup/migrations/xrun.ts (2)
packages/models/src/index.ts (1)
ensureAllIndexes(121-121)packages/models/src/proxify.ts (1)
ensureAllIndexes(66-80)
🔇 Additional comments (6)
.vscode/settings.json (1)
2-2: LGTM.Setting
eslint.workingDirectoriesto the repo root at Line 2 is a sensible workspace default.apps/meteor/app/livechat/server/lib/closeRoom.ts (1)
45-47: Good defensive hardening in transaction and update checks.The
session.inTransaction()guard at Line 45 and the saferupdatedRoom?.modifiedCountcheck at Line 191 improve failure-path robustness.Also applies to: 191-192
apps/meteor/app/settings/server/startup.ts (1)
6-9: LGTM.The explicit
toArray()+for...offlow is straightforward and keeps initialization ordering explicit.apps/meteor/server/startup/migrations/xrun.ts (1)
62-62: LGTM — good startup sequencing.Running
ensureAllIndexes()at Line 62 before migration-side writes is the right ordering.packages/models/src/index.ts (1)
121-121: LGTM.Re-exporting
ensureAllIndexesat Line 121 cleanly exposes the new index bootstrap hook.apps/meteor/ee/server/apps/orchestrator.js (1)
205-209: No TOCTOU risk exists; concurrent loading is safe.The
canEnableApp()function performs only read-only checks on the license and app initialization state—it does not depend on mutable enabled-app counts, slots, or limits. EachloadOne()call in the concurrent map operates independently on a distinct app, with no shared mutable resources. The concurrent parallelization is safe.The ESLint pragma at Line 203 (
// eslint-disable-next-line no-await-in-loop) is a legitimate compiler directive, not an implementation code comment.Likely an incorrect or invalid review comment.
Comment on lines
+49
to
+55
| try { | ||
| await Permissions.updateOne({ _id: permissionId }, { $set: doc }, { upsert: true }); | ||
| } catch (e) { | ||
| if (!(e as Error).message.includes('E11000')) { | ||
| await Permissions.updateOne({ _id: permissionId }, { $set: doc }, { upsert: true }); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Fix inverted duplicate-key retry logic in createSettingPermission.
At Line 52, the condition is reversed. E11000 is currently ignored, while non-E11000 is retried. This can leave the permission doc stale after duplicate-key races.
🐛 Proposed fix
try {
await Permissions.updateOne({ _id: permissionId }, { $set: doc }, { upsert: true });
} catch (e) {
- if (!(e as Error).message.includes('E11000')) {
- await Permissions.updateOne({ _id: permissionId }, { $set: doc }, { upsert: true });
- }
+ if ((e as Error).message.includes('E11000')) {
+ await Permissions.updateOne({ _id: permissionId }, { $set: doc });
+ } else {
+ throw e;
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/app/authorization/server/functions/upsertPermissions.ts` around
lines 49 - 55, In createSettingPermission, the duplicate-key retry logic is
inverted: the catch block currently retries only when the error is NOT E11000;
instead, detect E11000 (duplicate key) and perform the retry of
Permissions.updateOne({_id: permissionId}, {$set: doc}, {upsert: true}) only
when the caught error message includes 'E11000'; leave other errors to
propagate. Update the catch to check (e as Error).message.includes('E11000') and
call Permissions.updateOne in that branch, otherwise rethrow the error.
packages/models/src/proxify.ts
Outdated
Comment on lines
+75
to
+77
| } catch (err) { | ||
| console.warn(`ensureAllIndexes: failed for model ${name}`, err); | ||
| } |
Contributor
There was a problem hiding this comment.
Do not swallow index-creation failures in ensureAllIndexes.
At Lines 75-77, failures are only logged. The caller sees success even if some models failed to build indexes, which can leave startup in a partially indexed state.
🛠️ Proposed fix
export async function ensureAllIndexes(): Promise<void> {
const names = new Set([...lazyModels.keys(), ...models.keys()]);
+ const failures: Array<{ name: string; error: unknown }> = [];
await Promise.all(
Array.from(names).map(async (name) => {
try {
const model = proxify(name) as IBaseModel<any, any, any> & { createIndexes?: () => Promise<void> };
if (typeof model.createIndexes === 'function') {
await model.createIndexes();
}
} catch (err) {
- console.warn(`ensureAllIndexes: failed for model ${name}`, err);
+ failures.push({ name, error: err });
}
}),
);
+ if (failures.length > 0) {
+ throw new Error(`ensureAllIndexes failed for ${failures.length} model(s): ${failures.map(({ name }) => name).join(', ')}`);
+ }
}📝 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.
Suggested change
| } catch (err) { | |
| console.warn(`ensureAllIndexes: failed for model ${name}`, err); | |
| } | |
| export async function ensureAllIndexes(): Promise<void> { | |
| const names = new Set([...lazyModels.keys(), ...models.keys()]); | |
| const failures: Array<{ name: string; error: unknown }> = []; | |
| await Promise.all( | |
| Array.from(names).map(async (name) => { | |
| try { | |
| const model = proxify(name) as IBaseModel<any, any, any> & { createIndexes?: () => Promise<void> }; | |
| if (typeof model.createIndexes === 'function') { | |
| await model.createIndexes(); | |
| } | |
| } catch (err) { | |
| failures.push({ name, error: err }); | |
| } | |
| }), | |
| ); | |
| if (failures.length > 0) { | |
| throw new Error(`ensureAllIndexes failed for ${failures.length} model(s): ${failures.map(({ name }) => name).join(', ')}`); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/models/src/proxify.ts` around lines 75 - 77, The catch in
ensureAllIndexes currently only logs failures (console.warn(`ensureAllIndexes:
failed for model ${name}`, err)) which hides errors and makes callers think
indexing succeeded; change ensureAllIndexes to propagate failures instead of
swallowing them by either rethrowing the caught error (including model name) or
accumulating errors and rejecting with an AggregateError after processing all
models so callers observe failure; update the catch block that references name
and err (and remove or replace the console.warn) to throw or record a contextual
Error/AggregateError with the model name and original err.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 9 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="apps/meteor/ee/server/apps/orchestrator.js">
<violation number="1" location="apps/meteor/ee/server/apps/orchestrator.js:200">
P1: Race condition: parallelizing app enablement can exceed license app limits. The removed comment explicitly stated "This needs to happen sequentially to keep track of app limits." `canEnableApp` checks `shouldPreventAction` against a count that won't reflect the other apps being concurrently enabled in the same chunk, so multiple apps can pass the limit check simultaneously and exceed the allowed count.
(Based on your team's feedback about questioning behavioral changes that affect concurrency.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| err: error, | ||
| }); | ||
| } | ||
| const CONCURRENCY_LIMIT = 4; |
Contributor
There was a problem hiding this comment.
P1: Race condition: parallelizing app enablement can exceed license app limits. The removed comment explicitly stated "This needs to happen sequentially to keep track of app limits." canEnableApp checks shouldPreventAction against a count that won't reflect the other apps being concurrently enabled in the same chunk, so multiple apps can pass the limit check simultaneously and exceed the allowed count.
(Based on your team's feedback about questioning 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 apps/meteor/ee/server/apps/orchestrator.js, line 200:
<comment>Race condition: parallelizing app enablement can exceed license app limits. The removed comment explicitly stated "This needs to happen sequentially to keep track of app limits." `canEnableApp` checks `shouldPreventAction` against a count that won't reflect the other apps being concurrently enabled in the same chunk, so multiple apps can pass the limit check simultaneously and exceed the allowed count.
(Based on your team's feedback about questioning behavioral changes that affect concurrency.) </comment>
<file context>
@@ -191,24 +191,30 @@ export class AppServerOrchestrator {
- err: error,
- });
- }
+ const CONCURRENCY_LIMIT = 4;
+ for (let i = 0; i < apps.length; i += CONCURRENCY_LIMIT) {
+ const chunk = apps.slice(i, i + CONCURRENCY_LIMIT);
</file context>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
apps/meteor/app/authorization/server/functions/upsertPermissions.ts (1)
49-55:⚠️ Potential issue | 🟠 MajorFix the still-inverted duplicate-key retry branch in
createSettingPermission.The duplicate-key path is still skipped, while non-duplicate errors are retried. This can leave the permission stale after a race and hide the real failure mode.
Proposed fix
try { await Permissions.updateOne({ _id: permissionId }, { $set: doc }, { upsert: true }); } catch (e) { - if (!(e as Error).message.includes('E11000')) { - await Permissions.updateOne({ _id: permissionId }, { $set: doc }, { upsert: true }); - } + if ((e as Error).message.includes('E11000')) { + await Permissions.updateOne({ _id: permissionId }, { $set: doc }); + } else { + throw e; + } }For the MongoDB Node.js driver version used by Rocket.Chat, what is the recommended way to detect duplicate-key errors for update operations (error code 11000 vs parsing error.message)?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/authorization/server/functions/upsertPermissions.ts` around lines 49 - 55, The retry branch in createSettingPermission is inverted: currently it retries on non-duplicate errors and skips duplicate-key errors; change the catch to detect duplicate-key properly (use (e as MongoServerError).code === 11000 or e.code === 11000) and only perform the second Permissions.updateOne({ _id: permissionId }, { $set: doc }, { upsert: true }) when the error is a duplicate-key (11000); for non-duplicate errors rethrow or surface the error instead of retrying. Ensure you reference Permissions.updateOne, permissionId, doc and the createSettingPermission function when making the change.
🧹 Nitpick comments (1)
apps/meteor/app/authorization/server/functions/upsertPermissions.ts (1)
93-106: Consider bounded-memory batching instead of materializing all settings and ops.
toArray()+updateOps[]holds the full dataset twice before writing. Streaming and flushing per batch would keep memory stable on large workspaces.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/authorization/server/functions/upsertPermissions.ts` around lines 93 - 106, The code currently materializes all settings via Settings.findNotHidden().toArray() and builds updateOps[], using lots of memory; change to streaming/batched processing by iterating the cursor (e.g., Settings.findNotHidden() cursor or forEach) and accumulating a small batchOps array (eg batchSize = 500–2000), calling collection.bulkWrite(batchOps) when batchOps.length reaches batchSize, then clearing batchOps and continuing; keep using buildSettingPermissionDoc(setting, previousSettingPermissions) and delete previousSettingPermissions[permissionId] as you do now, and after the loop bulkWrite any remaining batchOps and handle an empty batch safely.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/app/authorization/server/functions/upsertPermissions.ts`:
- Around line 108-117: The batching loop contains inline implementation comments
that violate the repo guideline to avoid implementation comments; remove the
comments inside the for-loop (the lines commenting batching behavior and the
E11000 retry explanation) so the code around updateOps, BULK_WRITE_BATCH_SIZE,
Permissions.col.bulkWrite(...) and the E11000 catch path that calls
Permissions.updateOne(...) remains functionally identical but without those
inline explanatory comments.
- Around line 112-120: The bulkWrite error handling in upsertPermissions
currently checks the error message string for 'E11000' which is unsafe; change
the catch to inspect the thrown BulkWriteError's writeErrors array and ensure
every item has code === 11000 before retrying with non-upsert updates. Locate
the Permissions.col.bulkWrite call and in its catch, cast the error to the
BulkWriteError shape, check Array.isArray(err.writeErrors) and verify
err.writeErrors.every(e => e.code === 11000); only then run the Promise.all
fallback using Permissions.updateOne for each op, otherwise rethrow the original
error.
---
Duplicate comments:
In `@apps/meteor/app/authorization/server/functions/upsertPermissions.ts`:
- Around line 49-55: The retry branch in createSettingPermission is inverted:
currently it retries on non-duplicate errors and skips duplicate-key errors;
change the catch to detect duplicate-key properly (use (e as
MongoServerError).code === 11000 or e.code === 11000) and only perform the
second Permissions.updateOne({ _id: permissionId }, { $set: doc }, { upsert:
true }) when the error is a duplicate-key (11000); for non-duplicate errors
rethrow or surface the error instead of retrying. Ensure you reference
Permissions.updateOne, permissionId, doc and the createSettingPermission
function when making the change.
---
Nitpick comments:
In `@apps/meteor/app/authorization/server/functions/upsertPermissions.ts`:
- Around line 93-106: The code currently materializes all settings via
Settings.findNotHidden().toArray() and builds updateOps[], using lots of memory;
change to streaming/batched processing by iterating the cursor (e.g.,
Settings.findNotHidden() cursor or forEach) and accumulating a small batchOps
array (eg batchSize = 500–2000), calling collection.bulkWrite(batchOps) when
batchOps.length reaches batchSize, then clearing batchOps and continuing; keep
using buildSettingPermissionDoc(setting, previousSettingPermissions) and delete
previousSettingPermissions[permissionId] as you do now, and after the loop
bulkWrite any remaining batchOps and handle an empty batch safely.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.vscode/settings.jsonapps/meteor/app/authorization/server/functions/upsertPermissions.tsapps/meteor/app/settings/server/startup.tsapps/meteor/ee/server/apps/orchestrator.jsapps/meteor/server/startup/migrations/xrun.tspackages/models/src/index.tspackages/models/src/models/BaseRaw.tspackages/models/src/proxify.ts
💤 Files with no reviewable changes (1)
- packages/models/src/models/BaseRaw.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- .vscode/settings.json
- apps/meteor/server/startup/migrations/xrun.ts
- apps/meteor/ee/server/apps/orchestrator.js
- packages/models/src/proxify.ts
- apps/meteor/app/settings/server/startup.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). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🧰 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/index.tsapps/meteor/app/authorization/server/functions/upsertPermissions.ts
🧠 Learnings (5)
📓 Common learnings
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.
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: In Rocket.Chat PRs, keep feature PRs free of unrelated lockfile-only dependency bumps; prefer reverting lockfile drift or isolating such bumps into a separate "chore" commit/PR, and always use yarn install --immutable with the Yarn version pinned in package.json via Corepack.
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: Rocket.Chat repo context: When a workspace manifest on develop already pins a dependency version (e.g., packages/web-ui-registration → "rocket.chat/ui-contexts": "27.0.1"), a lockfile change in a feature PR that upgrades only that dependency’s resolution is considered a manifest-driven sync and can be kept, preferably as a small "chore: sync yarn.lock with manifests" commit.
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
packages/models/src/index.tsapps/meteor/app/authorization/server/functions/upsertPermissions.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
packages/models/src/index.tsapps/meteor/app/authorization/server/functions/upsertPermissions.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/app/authorization/server/functions/upsertPermissions.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/app/authorization/server/functions/upsertPermissions.ts
🔇 Additional comments (2)
packages/models/src/index.ts (1)
121-121: Public export update looks correct.Line 121 correctly exposes
ensureAllIndexesfrom the package entrypoint, which aligns with the startup/index-migration refactor and keeps imports centralized.apps/meteor/app/authorization/server/functions/upsertPermissions.ts (1)
59-79: Nice extraction ofbuildSettingPermissionDoc.Centralizing ID/doc creation improves cohesion and keeps both single-update and bulk paths consistent.
Comment on lines
+108
to
+117
| // Batches run sequentially so E11000 retry applies per batch | ||
| /* eslint-disable no-await-in-loop */ | ||
| for (let i = 0; i < updateOps.length; i += BULK_WRITE_BATCH_SIZE) { | ||
| const batch = updateOps.slice(i, i + BULK_WRITE_BATCH_SIZE); | ||
| try { | ||
| await Permissions.col.bulkWrite(batch, { ordered: false }); | ||
| } catch (e) { | ||
| if ((e as Error).message.includes('E11000')) { | ||
| // E11000 duplicate key: retry without upsert for this batch (doc already exists) | ||
| await Promise.all(batch.map((op) => Permissions.updateOne(op.updateOne.filter, op.updateOne.update))); |
Contributor
There was a problem hiding this comment.
Remove new inline implementation comments in the batching path.
Line 108 and Line 116 add implementation comments; this repo guideline asks to keep TS/JS implementation comment-free.
As per coding guidelines, "Avoid code comments in the implementation".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/app/authorization/server/functions/upsertPermissions.ts` around
lines 108 - 117, The batching loop contains inline implementation comments that
violate the repo guideline to avoid implementation comments; remove the comments
inside the for-loop (the lines commenting batching behavior and the E11000 retry
explanation) so the code around updateOps, BULK_WRITE_BATCH_SIZE,
Permissions.col.bulkWrite(...) and the E11000 catch path that calls
Permissions.updateOne(...) remains functionally identical but without those
inline explanatory comments.
Comment on lines
+112
to
+120
| try { | ||
| await Permissions.col.bulkWrite(batch, { ordered: false }); | ||
| } catch (e) { | ||
| if ((e as Error).message.includes('E11000')) { | ||
| // E11000 duplicate key: retry without upsert for this batch (doc already exists) | ||
| await Promise.all(batch.map((op) => Permissions.updateOne(op.updateOne.filter, op.updateOne.update))); | ||
| } else { | ||
| throw e; | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
MongoDB Node.js driver bulkWrite writeErrors code 11000 duplicate key error detection
💡 Result:
In the MongoDB Node.js driver, collection.bulkWrite() throws a MongoBulkWriteError when one or more operations fail (especially with ordered: true). That error includes a writeErrors array you can inspect. [1]
To detect duplicate key failures from the bulk, check for error code 11000 in the writeErrors entries (each entry corresponds to an operation index in your operations array). MongoDB reports duplicate key as code: 11000 (E11000). [2]
import { MongoBulkWriteError } from "mongodb";
try {
const res = await collection.bulkWrite(ops, { ordered: false });
} catch (err) {
if (err instanceof MongoBulkWriteError) {
const dupKeyErrors = (err.writeErrors ?? []).filter(e => e.code === 11000);
// each error typically includes: e.index (op index), e.code, e.errmsg, e.op
for (const e of dupKeyErrors) {
console.log("dup key at op index:", e.index, "msg:", e.errmsg);
}
} else {
throw err;
}
}Note: If you’re using the driver’s newer “client bulk write” API, writeErrors is a Map (operation index → error) rather than an array, but the same code === 11000 check applies. [1]
Sources:
[1] MongoDB Node.js Driver Docs — Bulk write exceptions / MongoBulkWriteError.writeErrors (array) and client bulk write writeErrors (Map) (mongodb.com)
[2] MongoDB Server Manual — bulkWrite() duplicate key example showing code: 11000 in writeErrors (mongodb.com)
Citations:
- 1: https://www.mongodb.com/docs/drivers/node/current/crud/bulk-write/
- 2: https://www.mongodb.com/docs/v8.0/reference/method/db.collection.bulkwrite/
🏁 Script executed:
# First, let's examine the actual code in the file
find . -type f -name "upsertPermissions.ts" | head -5Repository: RocketChat/Rocket.Chat
Length of output: 135
🏁 Script executed:
# Check the actual code around lines 112-120
sed -n '105,125p' ./apps/meteor/app/authorization/server/functions/upsertPermissions.tsRepository: RocketChat/Rocket.Chat
Length of output: 841
Avoid relying on error message string matching to detect duplicate-key-only failures in unordered bulkWrite.
On unordered bulks, multiple distinct errors can occur. If any error message happens to contain 'E11000', the current code will retry the entire batch, potentially masking non-duplicate errors. Use the writeErrors array instead, where each error has a code field (11000 for duplicate key). Only retry if all errors in the batch are code 11000.
Correct pattern
try {
await Permissions.col.bulkWrite(batch, { ordered: false });
} catch (e) {
- if ((e as Error).message.includes('E11000')) {
- // E11000 duplicate key: retry without upsert for this batch (doc already exists)
- await Promise.all(batch.map((op) => Permissions.updateOne(op.updateOne.filter, op.updateOne.update)));
- } else {
- throw e;
- }
+ const writeErrors = (e as { writeErrors?: Array<{ code?: number }> }).writeErrors ?? [];
+ const duplicateOnly = writeErrors.length > 0 && writeErrors.every((we) => we.code === 11000);
+ if (!duplicateOnly) {
+ throw e;
+ }
+ await Promise.all(batch.map((op) => Permissions.updateOne(op.updateOne.filter, op.updateOne.update)));
}📝 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.
Suggested change
| try { | |
| await Permissions.col.bulkWrite(batch, { ordered: false }); | |
| } catch (e) { | |
| if ((e as Error).message.includes('E11000')) { | |
| // E11000 duplicate key: retry without upsert for this batch (doc already exists) | |
| await Promise.all(batch.map((op) => Permissions.updateOne(op.updateOne.filter, op.updateOne.update))); | |
| } else { | |
| throw e; | |
| } | |
| try { | |
| await Permissions.col.bulkWrite(batch, { ordered: false }); | |
| } catch (e) { | |
| const writeErrors = (e as { writeErrors?: Array<{ code?: number }> }).writeErrors ?? []; | |
| const duplicateOnly = writeErrors.length > 0 && writeErrors.every((we) => we.code === 11000); | |
| if (!duplicateOnly) { | |
| throw e; | |
| } | |
| await Promise.all(batch.map((op) => Permissions.updateOne(op.updateOne.filter, op.updateOne.update))); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/app/authorization/server/functions/upsertPermissions.ts` around
lines 112 - 120, The bulkWrite error handling in upsertPermissions currently
checks the error message string for 'E11000' which is unsafe; change the catch
to inspect the thrown BulkWriteError's writeErrors array and ensure every item
has code === 11000 before retrying with non-upsert updates. Locate the
Permissions.col.bulkWrite call and in its catch, cast the error to the
BulkWriteError shape, check Array.isArray(err.writeErrors) and verify
err.writeErrors.every(e => e.code === 11000); only then run the Promise.all
fallback using Permissions.updateOne for each op, otherwise rethrow the original
error.
ggazzo
changed the title
ggazzo
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/server/lib/migrations.ts (1)
316-336:⚠️ Potential issue | 🟠 Major
getHashVersion()misses a return, so the predicate can always run on first call.At Line 335, the fetched value is assigned but never returned in the non-cached path. That makes
shouldRunServerVersionChange()treat the first call as changed (undefined !== Info.commit.hash), which can re-run version-change work on normal restarts.Proposed fix
-let hashVersion: string | undefined; +let hashVersion: string | undefined; -const getHashVersion = async () => { +const getHashVersion = async (): Promise<string | undefined> => { if (hashVersion) { return hashVersion; } const result = await Migrations.findOneAndUpdate( { _id: 'upgrade', }, { $set: { hash: Info.commit.hash, }, }, { upsert: true, }, ); hashVersion = result?.hash; + return hashVersion; };Also applies to: 338-341
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/server/lib/migrations.ts` around lines 316 - 336, getHashVersion currently sets hashVersion from the Migrations.findOneAndUpdate result but doesn't return it on the non-cached path, causing shouldRunServerVersionChange to see undefined; update getHashVersion to return the resolved hash (e.g., set hashVersion from result and then return hashVersion or return result.hash) after the upsert, and ensure you reference the correct property from the Migrations.findOneAndUpdate response so Info.commit.hash comparison works as intended.
🧹 Nitpick comments (1)
packages/models/src/models/BaseRaw.ts (1)
54-56: Remove inline implementation comments in this block.Prefer expressive naming over explanatory comments for this logic block.
As per coding guidelines, "Avoid code comments in the implementation".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/models/src/models/BaseRaw.ts` around lines 54 - 56, Remove the inline implementation comments in the BaseRaw block and instead make the code self-documenting by renaming identifiers and extracting intent-revealing small helpers; specifically, in the BaseRaw class replace ambiguous variable/method names involved in "accumulating indexes" and "lazy model instantiation" (e.g., the accumulator variable and the function that currently creates indexes) with expressive names like collectIndexes, pendingIndexSet, and ensureIndexesOnInstantiate, or extract a helper method createPendingIndexes that encapsulates the logic so the comment is unnecessary—then delete the two explanatory comment lines shown in this block.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/models/src/models/BaseRaw.ts`:
- Around line 57-71: The Set indexesThatShouldBeCreated and the listener
lifecycle are causing duplicate callbacks and repeated onAddedExecute
attachments; change registration to dedupe by a unique key (e.g., collectionKey
or id) instead of storing raw functions in indexesThatShouldBeCreated, have
onAdded push a keyed entry (or store fn keyed) and ensure ensureIndexes() calls
are idempotent by (a) only attaching onAddedExecute once (check a boolean flag
or remove before attach) and (b) removing onAddedExecute in cancel(); update
functions referenced (indexesThatShouldBeCreated, onAdded, onAddedExecute,
indexes.ensureIndexes, cancel, ee.on/off) so ensureIndexes() iterates unique
keyed entries and clears both the Set/map and any attached onAddedExecute
listener to prevent duplicated createIndexes executions and lingering listeners.
---
Outside diff comments:
In `@apps/meteor/server/lib/migrations.ts`:
- Around line 316-336: getHashVersion currently sets hashVersion from the
Migrations.findOneAndUpdate result but doesn't return it on the non-cached path,
causing shouldRunServerVersionChange to see undefined; update getHashVersion to
return the resolved hash (e.g., set hashVersion from result and then return
hashVersion or return result.hash) after the upsert, and ensure you reference
the correct property from the Migrations.findOneAndUpdate response so
Info.commit.hash comparison works as intended.
---
Nitpick comments:
In `@packages/models/src/models/BaseRaw.ts`:
- Around line 54-56: Remove the inline implementation comments in the BaseRaw
block and instead make the code self-documenting by renaming identifiers and
extracting intent-revealing small helpers; specifically, in the BaseRaw class
replace ambiguous variable/method names involved in "accumulating indexes" and
"lazy model instantiation" (e.g., the accumulator variable and the function that
currently creates indexes) with expressive names like collectIndexes,
pendingIndexSet, and ensureIndexesOnInstantiate, or extract a helper method
createPendingIndexes that encapsulates the logic so the comment is
unnecessary—then delete the two explanatory comment lines shown in this block.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (5)
apps/meteor/server/database/trash.tsapps/meteor/server/lib/migrations.tsapps/meteor/server/startup/migrations/xrun.tspackages/models/package.jsonpackages/models/src/models/BaseRaw.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/meteor/server/startup/migrations/xrun.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). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🧰 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/server/lib/migrations.tspackages/models/src/models/BaseRaw.tsapps/meteor/server/database/trash.ts
🧠 Learnings (9)
📓 Common learnings
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
📚 Learning: 2026-02-24T19:05:56.710Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: Rocket.Chat repo context: When a workspace manifest on develop already pins a dependency version (e.g., packages/web-ui-registration → "rocket.chat/ui-contexts": "27.0.1"), a lockfile change in a feature PR that upgrades only that dependency’s resolution is considered a manifest-driven sync and can be kept, preferably as a small "chore: sync yarn.lock with manifests" commit.
Applied to files:
packages/models/package.json
📚 Learning: 2026-02-24T19:05:56.710Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: In Rocket.Chat PRs, keep feature PRs free of unrelated lockfile-only dependency bumps; prefer reverting lockfile drift or isolating such bumps into a separate "chore" commit/PR, and always use yarn install --immutable with the Yarn version pinned in package.json via Corepack.
Applied to files:
packages/models/package.json
📚 Learning: 2025-12-10T21:00:54.909Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:54.909Z
Learning: Rocket.Chat monorepo: Jest testMatch pattern '<rootDir>/src/**/*.spec.(ts|js|mjs)' is valid in this repo and used across multiple packages (e.g., packages/tools, ee/packages/omnichannel-services). Do not flag it as invalid in future reviews.
Applied to files:
packages/models/package.json
📚 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:
packages/models/package.jsonpackages/models/src/models/BaseRaw.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/server/lib/migrations.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/server/lib/migrations.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/server/lib/migrations.tspackages/models/src/models/BaseRaw.tsapps/meteor/server/database/trash.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/server/lib/migrations.tspackages/models/src/models/BaseRaw.tsapps/meteor/server/database/trash.ts
🧬 Code graph analysis (1)
apps/meteor/server/database/trash.ts (2)
packages/models/src/proxify.ts (1)
registerModel(41-55)packages/models/src/index.ts (1)
registerModel(121-121)
🪛 Biome (2.4.4)
packages/models/src/models/BaseRaw.ts
[error] 63-63: This callback passed to forEach() iterable method should not return a value.
(lint/suspicious/useIterableCallbackReturn)
🔇 Additional comments (4)
apps/meteor/server/database/trash.ts (2)
1-1: Import change is appropriate for the new model registration flow.Bringing in
registerModelalongsideTrashRawis the right integration point for this refactor.
8-8: Model registration wiring looks correct.Registering
ITrashModelhere ensures Trash participates in centralized model/index orchestration.packages/models/package.json (1)
21-21: Dependency addition is consistent with the deferred-index design.
@rocket.chat/emitterat Line 21 matches the new model index orchestration introduced in this PR.packages/models/src/models/BaseRaw.ts (1)
391-391: Return value update is correct.Returning
dochere preserves thefindOneAndDeletebehavior after the manual delete path.
Comment on lines
+57
to
+71
| const indexesThatShouldBeCreated = new Set<IndexRegisterFn>(); | ||
| const onAdded = (fn: IndexRegisterFn) => indexesThatShouldBeCreated.add(fn); | ||
| const onAddedExecute = (fn: IndexRegisterFn) => fn(); | ||
| ee.on('added', onAdded); | ||
| export const indexes = { | ||
| ensureIndexes: () => { | ||
| indexesThatShouldBeCreated.forEach((fn) => fn()); | ||
| indexesThatShouldBeCreated.clear(); | ||
| ee.off('added', onAdded); | ||
| ee.on('added', onAddedExecute); | ||
| }, | ||
| cancel: () => { | ||
| ee.off('added', onAdded); | ||
| indexesThatShouldBeCreated.clear(); | ||
| }, |
Contributor
There was a problem hiding this comment.
Index registration currently accumulates duplicates and can multiply executions.
At Line 109, every instantiation emits a new closure, so the Set at Line 57 keeps growing with non-deduplicated callbacks. Also, Line 66 re-attaches onAddedExecute on repeated ensureIndexes() calls, and cancel() (Lines 68-70) does not detach it. This can cause duplicate createIndexes() runs and lingering execution mode.
💡 Proposed fix (dedupe by collection key + idempotent listener lifecycle)
export type IndexRegisterFn = () => Promise<void>;
+type IndexRegistration = { key: string; fn: IndexRegisterFn };
const ee = new Emitter<{
- added: IndexRegisterFn;
+ added: IndexRegistration;
}>();
-const indexesThatShouldBeCreated = new Set<IndexRegisterFn>();
-const onAdded = (fn: IndexRegisterFn) => indexesThatShouldBeCreated.add(fn);
-const onAddedExecute = (fn: IndexRegisterFn) => fn();
+const indexesThatShouldBeCreated = new Map<string, IndexRegisterFn>();
+const onAdded = ({ key, fn }: IndexRegistration) => indexesThatShouldBeCreated.set(key, fn);
+const onAddedExecute = ({ fn }: IndexRegistration) => void fn();
+let isExecutingMode = false;
ee.on('added', onAdded);
export const indexes = {
ensureIndexes: () => {
- indexesThatShouldBeCreated.forEach((fn) => fn());
+ if (isExecutingMode) {
+ return;
+ }
+ for (const fn of indexesThatShouldBeCreated.values()) {
+ void fn();
+ }
indexesThatShouldBeCreated.clear();
ee.off('added', onAdded);
+ ee.off('added', onAddedExecute);
ee.on('added', onAddedExecute);
+ isExecutingMode = true;
},
cancel: () => {
ee.off('added', onAdded);
+ ee.off('added', onAddedExecute);
indexesThatShouldBeCreated.clear();
+ isExecutingMode = false;
},
} as const;- void ee.emit('added', () => this.createIndexes());
+ void ee.emit('added', { key: this.collectionName, fn: () => this.createIndexes() });Also applies to: 109-109
🧰 Tools
🪛 Biome (2.4.4)
[error] 63-63: This callback passed to forEach() iterable method should not return a value.
(lint/suspicious/useIterableCallbackReturn)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/models/src/models/BaseRaw.ts` around lines 57 - 71, The Set
indexesThatShouldBeCreated and the listener lifecycle are causing duplicate
callbacks and repeated onAddedExecute attachments; change registration to dedupe
by a unique key (e.g., collectionKey or id) instead of storing raw functions in
indexesThatShouldBeCreated, have onAdded push a keyed entry (or store fn keyed)
and ensure ensureIndexes() calls are idempotent by (a) only attaching
onAddedExecute once (check a boolean flag or remove before attach) and (b)
removing onAddedExecute in cancel(); update functions referenced
(indexesThatShouldBeCreated, onAdded, onAddedExecute, indexes.ensureIndexes,
cancel, ee.on/off) so ensureIndexes() iterates unique keyed entries and clears
both the Set/map and any attached onAddedExecute listener to prevent duplicated
createIndexes executions and lingering listeners.
Contributor
There was a problem hiding this comment.
1 issue found across 8 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="apps/meteor/server/lib/migrations.ts">
<violation number="1" location="apps/meteor/server/lib/migrations.ts:335">
P1: `getHashVersion()` misses a return in the non-cached path, so the first call always yields `undefined` and `shouldRunServerVersionChange()` incorrectly returns `true`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
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)
apps/meteor/server/lib/migrations.ts (1)
316-337:⚠️ Potential issue | 🟠 MajorRemove stale hash cache;
findOneAndUpdatereturns pre-update document by defaultLine 335 caches
result?.hash(the pre-update hash). On subsequent calls within the same process,shouldRunServerVersionChange()returns the cached stale hash, making the check non-idempotent. Without explicitreturnDocument: 'before'in the options, the MongoDB driver defaults to returning the pre-update document—this implicit dependency should be made explicit.Proposed fix
let hashVersion: string | undefined; -const getHashVersion = async () => { - if (hashVersion) { - return hashVersion; - } +const getPreviousHashAndUpdate = async (): Promise<string | undefined> => { const result = await Migrations.findOneAndUpdate( { _id: 'upgrade', }, { $set: { hash: Info.commit.hash, }, }, { upsert: true, + returnDocument: 'before', }, ); - hashVersion = result?.hash; - return hashVersion; + return result?.hash; }; export async function shouldRunServerVersionChange(): Promise<boolean> { - const hash = await getHashVersion(); - - return hash !== Info.commit.hash; + if (hashVersion === Info.commit.hash) { + return false; + } + + const previousHash = await getPreviousHashAndUpdate(); + const shouldRun = previousHash !== Info.commit.hash; + hashVersion = Info.commit.hash; + return shouldRun; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/server/lib/migrations.ts` around lines 316 - 337, getHashVersion currently caches result?.hash from Migrations.findOneAndUpdate, but findOneAndUpdate returns the pre-update document by default so hashVersion can be stale; change the call to Migrations.findOneAndUpdate inside getHashVersion to request the post-update document (set the options to include returnDocument: 'after' or returnOriginal: false depending on driver) and then set hashVersion from that returned document (or directly from Info.commit.hash) so the cached hash is the updated value and shouldRunServerVersionChange() becomes idempotent; ensure you still keep upsert: true.
🤖 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 `@apps/meteor/server/lib/migrations.ts`:
- Around line 316-337: getHashVersion currently caches result?.hash from
Migrations.findOneAndUpdate, but findOneAndUpdate returns the pre-update
document by default so hashVersion can be stale; change the call to
Migrations.findOneAndUpdate inside getHashVersion to request the post-update
document (set the options to include returnDocument: 'after' or returnOriginal:
false depending on driver) and then set hashVersion from that returned document
(or directly from Info.commit.hash) so the cached hash is the updated value and
shouldRunServerVersionChange() becomes idempotent; ensure you still keep upsert:
true.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/meteor/server/lib/migrations.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). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🧰 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/server/lib/migrations.ts
🧠 Learnings (6)
📓 Common learnings
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
📚 Learning: 2026-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/server/lib/migrations.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/server/lib/migrations.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: UserBridge.doGetUserRoomIds in packages/apps-engine/src/server/bridges/UserBridge.ts has a bug where it implicitly returns undefined when the app lacks read permission (missing return statement in the else case of the permission check).
Applied to files:
apps/meteor/server/lib/migrations.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/server/lib/migrations.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/server/lib/migrations.ts
- Remove createIndexes() call from BaseRaw constructor to avoid thundering herd of index creation on every model instantiation - Add ensureAllIndexes() in proxify to run createIndexes on all registered models - Call ensureAllIndexes() inside onServerVersionChange callback in xrun.ts so indexes are created only on first start or when server version (commit hash) changes - Reduces MongoDB load during normal restarts Made-with: Cursor
Fetch all settings in one MongoDB round-trip instead of streaming with forEach to reduce startup time when there is network latency. Made-with: Cursor
- Fetch all settings in one MongoDB round-trip with toArray() - Add setBulk() to CachedSettings to fill store without N ready listeners - Reduces startup time and memory churn during settings init Made-with: Cursor
…psertPermissions
- Build updateOne ops for all settings, run in batches of 500 via bulkWrite
- Use deleteMany for obsolete permissions instead of N deleteOne calls
- Keeps createSettingPermission for the settings.on('*') callback (single setting at runtime)
Made-with: Cursor
- Process apps in chunks of CONCURRENCY_LIMIT (4) with Promise.all - Each chunk runs canEnableApp + loadOne in parallel, then next chunk - Preserves app limit semantics by waiting for chunk before starting next Made-with: Cursor
- Added eslint-disable-next-line directive to permit await usage within a loop for better readability and maintainability of the code.
…ogic - Removed the setBulk method from CachedSettings to streamline the settings loading process. - Updated the initializeSettings function to set records individually instead of using bulk loading, enhancing clarity and maintainability.
- Registered the Trash model with the new registerModel function. - Refactored migration logic to use a hash version check for server version changes. - Updated migration procedures to ensure indexes are created only when necessary, improving performance and reducing unnecessary operations.
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/app/authorization/server/functions/upsertPermissions.ts`:
- Around line 95-107: The second pass in upsertPermissions is rebuilding every
permission via createSettingPermission() with roles: [] and thus clears existing
role assignments; change the second pass to only operate on the remaining keys
in previousSettingPermissions (i.e., those not deleted during the first loop) or
guard createSettingPermission() by checking that the permission id still exists
in previousSettingPermissions before calling it. Locate the upsertPermissions
function and adjust the loop that currently iterates/createSettingPermission at
lines around where createSettingPermission, previousSettingPermissions,
buildSettingPermissionDoc, and settingsList are used so it only processes
missing permissions rather than replaying the full snapshot.
🪄 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: 7070d7e7-114d-467e-9000-d6dc6665f7f2
📒 Files selected for processing (3)
.vscode/settings.jsonapps/meteor/app/authorization/server/functions/upsertPermissions.tsapps/meteor/app/settings/server/startup.ts
✅ Files skipped from review due to trivial changes (1)
- .vscode/settings.json
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/meteor/app/settings/server/startup.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). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/app/authorization/server/functions/upsertPermissions.ts
🧠 Learnings (18)
📓 Common learnings
Learnt from: smirk-dev
Repo: RocketChat/Rocket.Chat PR: 39625
File: apps/meteor/app/api/server/v1/push.ts:85-97
Timestamp: 2026-03-14T14:58:58.834Z
Learning: In RocketChat/Rocket.Chat, the `push.token` POST/DELETE endpoints in `apps/meteor/app/api/server/v1/push.ts` were already migrated to the chained router API pattern on `develop` prior to PR `#39625`. `cleanTokenResult` (which strips `authToken` and returns `PushTokenResult`) and `isPushTokenPOSTProps`/`isPushTokenDELETEProps` validators already exist on `develop`. PR `#39625` only migrates `push.get` and `push.info` to the chained pattern. Do not flag `cleanTokenResult` or `PushTokenResult` as newly introduced behavior-breaking changes when reviewing this PR.
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
📚 Learning: 2026-03-20T13:51:23.302Z
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 39553
File: apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts:179-181
Timestamp: 2026-03-20T13:51:23.302Z
Learning: In `apps/meteor/app/integrations/server/methods/incoming/updateIncomingIntegration.ts`, the truthiness guards `...(integration.avatar && { avatar })`, `...(integration.emoji && { emoji })`, `...(integration.alias && { alias })`, and `...(integration.script && { script })` in the `$set` payload of `updateIncomingIntegration` are intentional. Empty-string values for these fields should NOT overwrite the stored value — only truthy values are persisted. Do not flag these as bugs preventing explicit clears.
Applied to files:
apps/meteor/app/authorization/server/functions/upsertPermissions.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/app/authorization/server/functions/upsertPermissions.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/app/authorization/server/functions/upsertPermissions.ts
📚 Learning: 2026-03-18T16:47:56.781Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 39701
File: packages/apps-engine/src/server/runtime/deno/AppsEngineDenoRuntime.ts:137-151
Timestamp: 2026-03-18T16:47:56.781Z
Learning: In `packages/apps-engine/src/server/runtime/deno/AppsEngineDenoRuntime.ts`, the `EEXIST` catch block in the `DenoRuntimeSubprocessController` constructor (around lines 144–151) intentionally skips validating the target of a pre-existing `deno-runtime` symlink in the temp directory. The rationale is that spoofing the symlink requires the attacker to already have system-level write access to the temp directory, which grants far greater control than the deno-runtime sandbox could provide. Do not flag this as a security issue in future reviews.
Applied to files:
apps/meteor/app/authorization/server/functions/upsertPermissions.ts
📚 Learning: 2026-03-09T21:20:12.687Z
Learnt from: pierre-lehnen-rc
Repo: RocketChat/Rocket.Chat PR: 39386
File: apps/meteor/server/services/push/tokenManagement/findDocumentToUpdate.ts:12-15
Timestamp: 2026-03-09T21:20:12.687Z
Learning: In `apps/meteor/server/services/push/tokenManagement/findDocumentToUpdate.ts`, the early return `if (data.voipToken) return null` (Lines 13-15) is intentionally correct. VoIP token updates always include an `_id`, so they are handled by the `_id` lookup block above (Lines 5-9) and never reach this guard. The guard is only a safety net for edge cases where `_id` is absent or no document was found, preventing an incorrect `token + appName` fallback match for VoIP-only payloads.
Applied to files:
apps/meteor/app/authorization/server/functions/upsertPermissions.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/app/authorization/server/functions/upsertPermissions.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to **/*.{ts,tsx,js} : Avoid code comments in the implementation
Applied to files:
apps/meteor/app/authorization/server/functions/upsertPermissions.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/app/authorization/server/functions/upsertPermissions.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:
apps/meteor/app/authorization/server/functions/upsertPermissions.ts
📚 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/app/authorization/server/functions/upsertPermissions.ts
📚 Learning: 2026-03-12T10:26:26.697Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 39340
File: apps/meteor/app/api/server/v1/im.ts:1349-1398
Timestamp: 2026-03-12T10:26:26.697Z
Learning: In `apps/meteor/app/api/server/v1/im.ts` (PR `#39340`), the `DmEndpoints` type intentionally includes temporary stub entries for `/v1/im.kick`, `/v1/dm.kick`, `/v1/im.leave`, and `/v1/dm.leave` (using `DmKickProps` and `DmLeaveProps`) even though no route handlers exist for them yet. These stubs were added to preserve type compatibility after removing the original `DmLeaveProps` and related files. They are planned for cleanup in a follow-up PR. Do not flag these as missing implementations when reviewing this file until the follow-up is merged.
Applied to files:
apps/meteor/app/authorization/server/functions/upsertPermissions.ts
📚 Learning: 2026-03-04T14:16:49.202Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 39304
File: packages/ui-contexts/src/ActionManagerContext.ts:26-26
Timestamp: 2026-03-04T14:16:49.202Z
Learning: In `packages/ui-contexts/src/ActionManagerContext.ts` (TypeScript, RocketChat/Rocket.Chat), the `disposeView` method in `IActionManager` uses an intentionally explicit union `UiKit.ModalView['id'] | UiKit.BannerView['viewId'] | UiKit.ContextualBarView['id']` to document which view types are accepted, even though all constituents resolve to the same primitive. The inline `// eslint-disable-next-line typescript-eslint/no-duplicate-type-constituents` comment is intentional and should not be flagged or removed.
Applied to files:
apps/meteor/app/authorization/server/functions/upsertPermissions.ts
📚 Learning: 2026-03-19T13:59:40.678Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38357
File: apps/meteor/app/apps/server/converters/uploads.ts:45-49
Timestamp: 2026-03-19T13:59:40.678Z
Learning: In `apps/meteor/app/apps/server/converters/uploads.ts`, the `room` async handler in `convertToApp` uses non-null assertions (`upload.rid!` and `result!`) intentionally. The data flow guarantees that any upload reaching this point must have a `rid`; if it does not, throwing an error is the desired behavior (fail-fast / data integrity guard). Do not flag these non-null assertions as unsafe during code review.
Applied to files:
apps/meteor/app/authorization/server/functions/upsertPermissions.ts
📚 Learning: 2026-02-12T15:39:28.416Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 32703
File: apps/meteor/client/lib/chats/flows/uploadFiles.ts:52-58
Timestamp: 2026-02-12T15:39:28.416Z
Learning: In `apps/meteor/client/lib/chats/flows/uploadFiles.ts`, when E2E encryption is required but not allowed (e.g., `E2E_Enable_Encrypt_Files` setting is disabled), the function intentionally abandons the entire upload queue and displays a toast error. This fail-fast behavior prevents partial uploads when encryption requirements cannot be met and is the expected behavior, not a bug.
Applied to files:
apps/meteor/app/authorization/server/functions/upsertPermissions.ts
📚 Learning: 2026-03-09T18:39:21.178Z
Learnt from: Harxhit
Repo: RocketChat/Rocket.Chat PR: 39476
File: apps/meteor/server/methods/addAllUserToRoom.ts:0-0
Timestamp: 2026-03-09T18:39:21.178Z
Learning: In apps/meteor/server/methods/addAllUserToRoom.ts, the implementation uses a single cursor pass (Users.find(userFilter).batchSize(100)) that collects both the full user objects (collectedUsers: IUser[]) and their usernames (usernames: string[]) in one iteration. `beforeAddUserToRoom` is then called once with the full usernames batch (preserving batch-validation semantics), and the subsequent subscription/message processing loop iterates over the same stable `collectedUsers` array — no second DB query is made. This avoids any race condition between validation and processing while preserving the original batch-validation behavior.
Applied to files:
apps/meteor/app/authorization/server/functions/upsertPermissions.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/app/authorization/server/functions/upsertPermissions.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/app/authorization/server/functions/upsertPermissions.ts
🔇 Additional comments (1)
apps/meteor/app/authorization/server/functions/upsertPermissions.ts (1)
60-80: Nice:_updatedAtstays populated on the bulk path.
Permissions.col.bulkWrite()bypassesBaseRaw.updateOne(), so stamping_updatedAtinbuildSettingPermissionDoc()keeps these permission updates aligned with the model-layer behavior.
Comment on lines
+95
to
+107
| const settingsList = await Settings.findNotHidden().toArray(); | ||
|
|
||
| const updateOps: SettingPermissionUpdateOp[] = []; | ||
| for (const setting of settingsList) { | ||
| const { _id: permissionId, doc } = buildSettingPermissionDoc(setting, previousSettingPermissions); | ||
| updateOps.push({ | ||
| updateOne: { | ||
| filter: { _id: permissionId }, | ||
| update: { $set: doc }, | ||
| upsert: true, | ||
| }, | ||
| }); | ||
| delete previousSettingPermissions[permissionId]; |
Contributor
There was a problem hiding this comment.
Don't replay the full second settings snapshot through createSettingPermission().
After Line 107 removes each processed ID from previousSettingPermissions, the second pass at Lines 127-130 rebuilds those same permissions with roles: []. That clears existing role assignments whenever upsertPermissions() runs. If this pass is meant to close the race before settings.on('*') is registered, it needs to handle only settings that were missing from the first snapshot.
🐛 Proposed fix
const settingsList = await Settings.findNotHidden().toArray();
+ const initialSettingIds = new Set(settingsList.map(({ _id }) => _id));
const updateOps: SettingPermissionUpdateOp[] = [];
for (const setting of settingsList) {
const { _id: permissionId, doc } = buildSettingPermissionDoc(setting, previousSettingPermissions);
updateOps.push({
@@
- const settings = await Settings.findNotHidden().toArray();
- for (const setting of settings) {
- await createSettingPermission(setting, previousSettingPermissions);
+ const latestSettings = await Settings.findNotHidden().toArray();
+ for (const setting of latestSettings) {
+ if (initialSettingIds.has(setting._id)) {
+ continue;
+ }
+
+ await createSettingPermission(setting, await getPreviousPermissions(setting._id));
}Also applies to: 127-130
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/app/authorization/server/functions/upsertPermissions.ts` around
lines 95 - 107, The second pass in upsertPermissions is rebuilding every
permission via createSettingPermission() with roles: [] and thus clears existing
role assignments; change the second pass to only operate on the remaining keys
in previousSettingPermissions (i.e., those not deleted during the first loop) or
guard createSettingPermission() by checking that the permission id still exists
in previousSettingPermissions before calling it. Locate the upsertPermissions
function and adjust the loop that currently iterates/createSettingPermission at
lines around where createSettingPermission, previousSettingPermissions,
buildSettingPermissionDoc, and settingsList are used so it only processes
missing permissions rather than replaying the full snapshot.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Made-with: Cursor
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
Performance Improvements
Chores