fix(team): await default-room assignments when adding team members#38701
fix(team): await default-room assignments when adding team members#38701Shreyas2004wagh wants to merge 2 commits intoRocketChat:developfrom
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
WalkthroughModified TeamService's addMembersToDefaultRooms to properly await completion of all async operations using Promise.all instead of non-awaited map calls. Users are added sequentially per room. Comprehensive unit tests were added to verify proper awaiting and error propagation. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (1)**/*.{ts,tsx,js}📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
Files:
🧠 Learnings (12)📚 Learning: 2025-09-25T09:59:26.461ZApplied to files:
📚 Learning: 2025-09-25T09:59:26.461ZApplied to files:
📚 Learning: 2025-10-28T16:53:42.761ZApplied to files:
📚 Learning: 2025-11-24T17:08:17.065ZApplied to files:
📚 Learning: 2025-11-27T17:56:26.050ZApplied to files:
📚 Learning: 2026-01-17T01:51:47.764ZApplied to files:
📚 Learning: 2025-09-25T09:59:26.461ZApplied to files:
📚 Learning: 2025-11-24T17:08:17.065ZApplied to files:
📚 Learning: 2025-11-24T17:08:17.065ZApplied to files:
📚 Learning: 2025-11-24T17:08:17.065ZApplied to files:
📚 Learning: 2025-11-24T17:08:17.065ZApplied to files:
📚 Learning: 2025-11-24T17:08:17.065ZApplied to files:
⏰ 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). (4)
🔇 Additional comments (4)
✏️ Tip: You can disable this entire section by setting 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.
| resolved = true; | ||
| }); | ||
|
|
||
| await flushMicrotasks(); |
There was a problem hiding this comment.
I’m using flushMicrotasks() here because resolved is set inside operation.then(...), and that callback only runs once the microtask queue is processed ,Without yeilding to microtasks first, resolved could still be false simply because the .then() hasn’t executed yet — not because addMembersToDefaultRooms() is actually still pending
Flushing microtasks makes the assertion meaningful, and after that point, if resolved is still false, it confirms the function is waiting for the second addUserToRoom promise to resolve, which is the behavior the test is validating.
![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.49% 70.59% +0.10%
===========================================
Files 3175 3176 +1
Lines 111094 111153 +59
Branches 20045 20039 -6
===========================================
+ Hits 78311 78464 +153
+ Misses 30738 30652 -86
+ Partials 2045 2037 -8
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
2 participants
Summary
Testing
Fixes #38700
Summary by CodeRabbit
Bug Fixes
Tests