fix(team): await default-room assignments when adding team members#38701
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
|
No actionable comments were generated in the recent review. 🎉 📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
WalkthroughReworked TeamService.addMembersToDefaultRooms to iterate default rooms sequentially and await per-room concurrent user additions via Promise.all, ensuring the method only resolves after all addUserToRoom operations complete; added unit tests verifying awaiting behavior and error propagation. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant TeamService
participant RoomsStore
participant UsersStore
participant RoomService
Caller->>TeamService: addMembersToDefaultRooms(teamId, users)
TeamService->>RoomsStore: getDefaultRooms(teamId)
RoomsStore-->>TeamService: defaultRooms[]
TeamService->>UsersStore: getActiveUsers(teamId)
UsersStore-->>TeamService: users[]
loop for each room (sequential)
TeamService->>RoomService: addUserToRoom(roomX, userA) (concurrent)
TeamService->>RoomService: addUserToRoom(roomX, userB) (concurrent)
RoomService-->>TeamService: Promise results (resolved/rejected)
end
TeamService-->>Caller: resolves when all rooms' user additions complete / rejects on error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This pull request fixes a critical bug in TeamService.addMembersToDefaultRooms where async operations were not properly awaited, causing the function to return before users were actually added to default team rooms and silently dropping errors.
Changes:
- Wrapped async room processing operations with
Promise.all()to ensure completion before function returns - Changed
for awaittoforfor iterating over regular arrays - Added comprehensive unit tests to verify completion semantics and error propagation
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| apps/meteor/server/services/team/service.ts | Fixed addMembersToDefaultRooms to properly await all async default-room membership operations using Promise.all() |
| apps/meteor/tests/unit/server/services/team/service.tests.ts | Added new test file with two regression tests: one verifying completion semantics and another verifying error propagation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #38701 +/- ##
===========================================
+ Coverage 70.55% 70.68% +0.12%
===========================================
Files 3189 3189
Lines 112703 112716 +13
Branches 20429 20411 -18
===========================================
+ Hits 79521 79676 +155
+ Misses 31119 30995 -124
+ Partials 2063 2045 -18
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
KevLehman
left a comment
KevLehman
left a comment
There was a problem hiding this comment.
Pls also fix the lint issues.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
apps/meteor/tests/unit/server/services/team/service.tests.ts (3)
56-57: Consider a more proportionate timeout.
this.timeout(15000)is ~750× the 20 ms delay in the test. If the awaiting logic regresses, the test suite will stall for 15 s before reporting failure. A value closer to 500–1000 ms provides the same CI safety margin with faster feedback.♻️ Suggested change
- this.timeout(15000); + this.timeout(500);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/tests/unit/server/services/team/service.tests.ts` around lines 56 - 57, The test "should wait for default room membership operations to finish" sets an excessively long timeout via this.timeout(15000); reduce it to a more proportionate value (e.g., 500–1000 ms) to keep CI fast while preserving margin for delays — locate the it(...) block in apps/meteor/tests/unit/server/services/team/service.tests.ts and replace the this.timeout(15000) call with a shorter timeout such as this.timeout(1000) (or 500) to avoid long stalls on failure.
6-6: Narrow theanytypes forRoomsandUsersto improve stub-contract safety.
Rooms: anyandUsers: anysuppress TypeScript type-checking on the stubs, making it easy to misname methods or return wrong shapes without a compile-time error.♻️ Proposed type narrowing
-const createTeamService = (deps: { Rooms: any; Users: any; addUserToRoom: sinon.SinonStub }) => { +const createTeamService = (deps: { + Rooms: { findDefaultRoomsForTeam: sinon.SinonStub }; + Users: { findActiveByIds: sinon.SinonStub }; + addUserToRoom: sinon.SinonStub; +}) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/tests/unit/server/services/team/service.tests.ts` at line 6, The test helper createTeamService currently types Rooms and Users as any; narrow them by introducing minimal stub interfaces (e.g., RoomCollectionStub and UserCollectionStub) that declare only the methods your tests call (for example findOne, update, insert, find, etc.) and use those interfaces in the createTeamService signature (createTeamService(deps: { Rooms: RoomCollectionStub; Users: UserCollectionStub; addUserToRoom: sinon.SinonStub })). Update existing sinon stubs in tests to be typed as SinonStub & Partial<RoomCollectionStub> / Partial<UserCollectionStub> as needed so TypeScript verifies method names and return shapes used by functions like createTeamService.
99-100: Same timeout concern applies here.♻️ Suggested change
- this.timeout(15000); + this.timeout(500);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/tests/unit/server/services/team/service.tests.ts` around lines 99 - 100, The test "should propagate errors from default room membership operations" currently calls this.timeout(15000); reduce or remove the explicit 15s timeout to avoid overly long/flaky tests—either delete the this.timeout call or set a much smaller, justified timeout (e.g., this.timeout(5000)), keeping the test declared as the existing async function so the this context is valid.apps/meteor/server/services/team/service.ts (1)
1001-1011: Remove inline comments per coding guidelines.The nested
Promise.allcorrectly fixes the awaiting and error-propagation semantics. The two inline comments on lines 1003 and 1006 violate the "Avoid code comments in the implementation" guideline.♻️ Proposed cleanup
- await Promise.all( - defaultRooms.map((room) => - // at this point, users are already part of the team so we won't check for membership - Promise.all( - users.map((user) => - // add each user to the default room - addUserToRoom(room._id, user, inviter, { skipSystemMessage: false }), - ), - ), - ), - ); + await Promise.all( + defaultRooms.map((room) => + Promise.all( + users.map((user) => addUserToRoom(room._id, user, inviter, { skipSystemMessage: false })), + ), + ), + );As per coding guidelines,
**/*.{ts,tsx,js}: "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/server/services/team/service.ts` around lines 1001 - 1011, Remove the two inline comments inside the Promise.all nesting by deleting the comment tokens and text that appear above the inner Promise.all and above addUserToRoom; leave the logic intact so defaultRooms.map(... Promise.all(users.map(... addUserToRoom(room._id, user, inviter, { skipSystemMessage: false }) ... ))) still runs and awaits properly. Ensure you only remove the comments (no other formatting or behavior changes) around defaultRooms, users and the addUserToRoom call.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/meteor/server/services/team/service.tsapps/meteor/tests/unit/server/services/team/service.tests.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/tests/unit/server/services/team/service.tests.tsapps/meteor/server/services/team/service.ts
🧠 Learnings (9)
📚 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/tests/unit/server/services/team/service.tests.tsapps/meteor/server/services/team/service.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 : Group related tests in the same file
Applied to files:
apps/meteor/tests/unit/server/services/team/service.tests.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 : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files
Applied to files:
apps/meteor/tests/unit/server/services/team/service.tests.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings by mapping subscription documents to room IDs, never undefined, even when user has no room subscriptions.
Applied to files:
apps/meteor/server/services/team/service.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings (mapping subscription documents to room IDs), never undefined, even when user has no room subscriptions.
Applied to files:
apps/meteor/server/services/team/service.ts
📚 Learning: 2025-10-28T16:53:42.761Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37205
File: ee/packages/federation-matrix/src/FederationMatrix.ts:296-301
Timestamp: 2025-10-28T16:53:42.761Z
Learning: In the Rocket.Chat federation-matrix integration (ee/packages/federation-matrix/), the createRoom method from rocket.chat/federation-sdk will support a 4-argument signature (userId, roomName, visibility, displayName) in newer versions. Code using this 4-argument call is forward-compatible with planned library updates and should not be flagged as an error.
Applied to files:
apps/meteor/server/services/team/service.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/server/services/team/service.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/services/team/service.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/services/team/service.ts
🧬 Code graph analysis (1)
apps/meteor/tests/unit/server/services/team/service.tests.ts (1)
apps/meteor/server/services/team/service.ts (1)
TeamService(39-1139)
⏰ 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: cubic · AI code reviewer
- GitHub Check: CodeQL-Build
🔇 Additional comments (2)
apps/meteor/tests/unit/server/services/team/service.tests.ts (1)
99-126: LGTM.Error propagation test correctly sets up a single user + single room so that
addUserToRoomis called once, rejects, and the rejection surfaces through the nestedPromise.all. Assertions oncallCount, error instance type, and message are all tight.apps/meteor/server/services/team/service.ts (1)
1001-1011: Verify error handling in callers ofaddMembersToDefaultRoomsto handle partial state — some users may be added to default rooms while others are not if any operation fails.The nested
Promise.all(rooms × users) rejects immediately on first failure, leaving in-flight operations to complete or fail silently. SinceTeamMember.createOneByTeamIdAndUserIdcompletes at line 885 before callingaddMembersToDefaultRoomsat line 887, an error here leaves users as team members but absent from default rooms. TheaddMembermethod has no error handling and propagates this state upward.Confirm that all callers of
addMember(especially line 125 inaddUserToRoom.tsand the API endpoint at line 485 inteams.ts) are prepared to handle and communicate partial-state scenarios to clients.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/meteor/server/services/team/service.ts`:
- Around line 1001-1011: Remove the two inline comments inside the Promise.all
nesting by deleting the comment tokens and text that appear above the inner
Promise.all and above addUserToRoom; leave the logic intact so
defaultRooms.map(... Promise.all(users.map(... addUserToRoom(room._id, user,
inviter, { skipSystemMessage: false }) ... ))) still runs and awaits properly.
Ensure you only remove the comments (no other formatting or behavior changes)
around defaultRooms, users and the addUserToRoom call.
In `@apps/meteor/tests/unit/server/services/team/service.tests.ts`:
- Around line 56-57: The test "should wait for default room membership
operations to finish" sets an excessively long timeout via this.timeout(15000);
reduce it to a more proportionate value (e.g., 500–1000 ms) to keep CI fast
while preserving margin for delays — locate the it(...) block in
apps/meteor/tests/unit/server/services/team/service.tests.ts and replace the
this.timeout(15000) call with a shorter timeout such as this.timeout(1000) (or
500) to avoid long stalls on failure.
- Line 6: The test helper createTeamService currently types Rooms and Users as
any; narrow them by introducing minimal stub interfaces (e.g.,
RoomCollectionStub and UserCollectionStub) that declare only the methods your
tests call (for example findOne, update, insert, find, etc.) and use those
interfaces in the createTeamService signature (createTeamService(deps: { Rooms:
RoomCollectionStub; Users: UserCollectionStub; addUserToRoom: sinon.SinonStub
})). Update existing sinon stubs in tests to be typed as SinonStub &
Partial<RoomCollectionStub> / Partial<UserCollectionStub> as needed so
TypeScript verifies method names and return shapes used by functions like
createTeamService.
- Around line 99-100: The test "should propagate errors from default room
membership operations" currently calls this.timeout(15000); reduce or remove the
explicit 15s timeout to avoid overly long/flaky tests—either delete the
this.timeout call or set a much smaller, justified timeout (e.g.,
this.timeout(5000)), keeping the test declared as the existing async function so
the this context is valid.
|
Updated per review feedback in 77f93d5 and all checks are now green. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/meteor/server/services/team/service.ts (2)
1001-1005: Core fix is correct — minorPromise.allfail-fast caveat worth notingThe replacement of the unawaited
defaultRooms.map(async …)withfor...of+await Promise.all(…)correctly fixes both the completion-before-assignment bug and the silent-rejection issue.One behavioural nuance to be aware of:
Promise.allrejects as soon as any singleaddUserToRoomcall throws. Because the rooms are processed sequentially, a rejection in room N means rooms N+1 … N+last are never attempted, potentially leaving a member present in theTeamMembercollection but absent from some default rooms. This is an inherent limitation of the architecture (no multi-document transaction) and was a pre-existing concern; the current change surfaces it rather than introducing it.If all rooms should always be attempted regardless of per-room errors — and the caller wants to handle accumulated failures —
Promise.allSettled+ a manual error-check would be the alternative:💡 Optional: resilient all-rooms variant using Promise.allSettled
- for (const room of defaultRooms) { - // at this point, users are already part of the team so we won't check for membership - // eslint-disable-next-line no-await-in-loop - await Promise.all(users.map((user) => addUserToRoom(room._id, user, inviter, { skipSystemMessage: false }))); - } + const results = await Promise.allSettled( + defaultRooms.flatMap((room) => + users.map((user) => addUserToRoom(room._id, user, inviter, { skipSystemMessage: false })), + ), + ); + const firstFailure = results.find((r): r is PromiseRejectedResult => r.status === 'rejected'); + if (firstFailure) { + throw firstFailure.reason; + }This also removes the
no-await-in-loopsuppression and the loop comment entirely, which aligns with the coding guideline to 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/server/services/team/service.ts` around lines 1001 - 1005, The current loop over defaultRooms uses await Promise.all(...) which will fail-fast and stop processing subsequent rooms if any addUserToRoom(room._id, user, inviter, ...) rejects; change the per-room concurrency from Promise.all to Promise.allSettled so every room is attempted even if some user additions fail, collect and surface any rejected results (e.g., aggregate errors or log them) after each Promise.allSettled for the given room, and remove the eslint-disable-next-line no-await-in-loop comment and the explanatory loop comment; key symbols: defaultRooms, addUserToRoom, users, inviter, and the Promise.all -> Promise.allSettled change.
1003-1003:eslint-disable-next-linecomment — minor guideline noteLine 1003 adds a
// eslint-disable-next-line no-await-in-loopdirective. Per the project's coding guidelines for**/*.{ts,tsx,js}, code comments in the implementation should be avoided. While ESLint suppression directives are practical necessities, the optionalPromise.allSettledrefactor above would eliminate theawait-in-loop pattern entirely and remove the need for this suppression.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/server/services/team/service.ts` at line 1003, You added a "// eslint-disable-next-line no-await-in-loop" to silence an await-in-loop; instead remove that suppression and refactor the surrounding loop into a concurrent pattern using Promise.allSettled (map the original loop body into an array of async tasks and await Promise.allSettled(tasks)), then handle results/errors as needed; delete the eslint-disable comment once the loop is converted so no inline ESLint suppression remains (make this change at the loop that currently contains the eslint-disable-next-line no-await-in-loop).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/meteor/server/services/team/service.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/server/services/team/service.ts
🧠 Learnings (4)
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings by mapping subscription documents to room IDs, never undefined, even when user has no room subscriptions.
Applied to files:
apps/meteor/server/services/team/service.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
Repo: RocketChat/Rocket.Chat PR: 37057
File: packages/apps-engine/src/definition/accessors/IUserRead.ts:23-27
Timestamp: 2025-09-25T09:59:26.461Z
Learning: AppUserBridge.getUserRoomIds in apps/meteor/app/apps/server/bridges/users.ts always returns an array of strings (mapping subscription documents to room IDs), never undefined, even when user has no room subscriptions.
Applied to files:
apps/meteor/server/services/team/service.ts
📚 Learning: 2025-10-28T16:53:42.761Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37205
File: ee/packages/federation-matrix/src/FederationMatrix.ts:296-301
Timestamp: 2025-10-28T16:53:42.761Z
Learning: In the Rocket.Chat federation-matrix integration (ee/packages/federation-matrix/), the createRoom method from rocket.chat/federation-sdk will support a 4-argument signature (userId, roomName, visibility, displayName) in newer versions. Code using this 4-argument call is forward-compatible with planned library updates and should not be flagged as an error.
Applied to files:
apps/meteor/server/services/team/service.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/server/services/team/service.ts
⏰ 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: cubic · AI code reviewer
- GitHub Check: CodeQL-Build
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/meteor/server/services/team/service.ts`:
- Around line 1001-1005: The current loop over defaultRooms uses await
Promise.all(...) which will fail-fast and stop processing subsequent rooms if
any addUserToRoom(room._id, user, inviter, ...) rejects; change the per-room
concurrency from Promise.all to Promise.allSettled so every room is attempted
even if some user additions fail, collect and surface any rejected results
(e.g., aggregate errors or log them) after each Promise.allSettled for the given
room, and remove the eslint-disable-next-line no-await-in-loop comment and the
explanatory loop comment; key symbols: defaultRooms, addUserToRoom, users,
inviter, and the Promise.all -> Promise.allSettled change.
- Line 1003: You added a "// eslint-disable-next-line no-await-in-loop" to
silence an await-in-loop; instead remove that suppression and refactor the
surrounding loop into a concurrent pattern using Promise.allSettled (map the
original loop body into an array of async tasks and await
Promise.allSettled(tasks)), then handle results/errors as needed; delete the
eslint-disable comment once the loop is converted so no inline ESLint
suppression remains (make this change at the loop that currently contains the
eslint-disable-next-line no-await-in-loop).
KevLehman
added
the
stat: QA assured
dionisio-bot
Bot
added
the
stat: ready to merge
|
Thanks @KevLehman for the review |
Summary
Testing
Fixes #38700
COMM-144
Summary by CodeRabbit
Bug Fixes
Tests