fix: Orphan team when last owner user deleted #36807
Conversation
|
Looks like this PR is ready to merge! 🎉 |
🦋 Changeset detectedLatest commit: 937a0df The changes in this PR will be included in the next version bump. This PR includes changesets to release 41 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #36807 +/- ##
===========================================
- Coverage 68.94% 68.91% -0.03%
===========================================
Files 3360 3360
Lines 114282 114325 +43
Branches 20562 20684 +122
===========================================
- Hits 78790 78787 -3
- Misses 33406 33449 +43
- Partials 2086 2089 +3
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
apps/meteor/app/lib/server/functions/relinquishRoomOwnerships.ts
Outdated
Show resolved
Hide resolved
apps/meteor/app/lib/server/functions/relinquishRoomOwnerships.ts
Outdated
Show resolved
Hide resolved
apps/meteor/app/lib/server/functions/relinquishRoomOwnerships.ts
Outdated
Show resolved
Hide resolved
apps/meteor/app/lib/server/functions/relinquishRoomOwnerships.ts
Outdated
Show resolved
Hide resolved
abhinavkrin
force-pushed
the
fix/users.delete-last-owner-team
branch
from
f09fc07 to
3a6c005
Compare
WalkthroughAdds a reusable team/room erasure module and tests; integrates it into team deletion and relinquish-ownership flows; surfaces deleted room IDs from user-deletion and relinquish paths; changes Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant API as REST API
participant Erase as eraseTeam module
participant TeamSvc as Team Service
participant RoomSvc as Room Service
participant Apps as Apps Bridge
Client->>API: DELETE /v1/teams/:id (roomsToRemove)
API->>Erase: eraseTeam(user, team, roomsToRemove)
Erase->>TeamSvc: getMatchingTeamRooms(team)
TeamSvc-->>Erase: matchingRooms
loop each non-main room
Erase->>RoomSvc: eraseRoomFn(rid)
RoomSvc->>Apps: onPreDelete(room)
Apps-->>RoomSvc: allow/deny
alt allowed
RoomSvc->>RoomSvc: deleteRoom(rid)
RoomSvc->>Apps: onPostDelete(room)
RoomSvc-->>Erase: success (rid)
else denied or failed
RoomSvc-->>Erase: failure (skip)
end
end
Erase->>TeamSvc: unsetTeamIdOfRooms(user, remainingRooms)
Erase->>TeamSvc: removeAllMembersFromTeam(team)
Erase->>TeamSvc: deleteTeam(team)
Erase-->>API: { deletedRooms }
API-->>Client: 200 { deletedRooms }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 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)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
.changeset/stale-sloths-smoke.md(1 hunks)apps/meteor/app/api/server/lib/eraseTeam.ts(1 hunks)apps/meteor/app/api/server/v1/teams.ts(3 hunks)apps/meteor/app/lib/server/functions/deleteUser.ts(1 hunks)apps/meteor/app/lib/server/functions/relinquishRoomOwnerships.ts(2 hunks)apps/meteor/server/services/team/service.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
apps/meteor/app/lib/server/functions/deleteUser.ts (1)
apps/meteor/app/lib/server/functions/relinquishRoomOwnerships.ts (1)
relinquishRoomOwnerships(58-82)
apps/meteor/app/api/server/lib/eraseTeam.ts (2)
packages/core-typings/src/IUser.ts (1)
IUser(186-255)packages/core-typings/src/IRoom.ts (1)
IRoom(21-95)
apps/meteor/app/lib/server/functions/relinquishRoomOwnerships.ts (5)
packages/core-typings/src/IRoom.ts (1)
IRoom(21-95)apps/meteor/app/api/server/lib/eraseTeam.ts (1)
eraseTeam(6-27)apps/meteor/app/file-upload/server/lib/FileUpload.ts (1)
FileUpload(110-681)apps/meteor/app/lib/server/lib/notifyListener.ts (1)
notifyOnSubscriptionChanged(515-519)packages/core-typings/src/IUser.ts (1)
IUser(186-255)
apps/meteor/app/api/server/v1/teams.ts (1)
apps/meteor/app/api/server/lib/eraseTeam.ts (1)
eraseTeam(6-27)
🪛 Biome (2.1.2)
apps/meteor/app/lib/server/functions/relinquishRoomOwnerships.ts
[error] 18-32: Promise executor functions should not be async.
(lint/suspicious/noAsyncPromiseExecutor)
⏰ 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). (7)
- GitHub Check: 🔎 Code Check / Code Lint
- GitHub Check: 🔨 Test Unit / Unit Tests
- GitHub Check: 🔨 Test Storybook / Test Storybook
- GitHub Check: 🔎 Code Check / TypeScript
- GitHub Check: 📦 Meteor Build - coverage
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
apps/meteor/app/lib/server/functions/relinquishRoomOwnerships.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
apps/meteor/app/api/server/lib/eraseTeam.ts(1 hunks)apps/meteor/tests/end-to-end/api/teams.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/meteor/app/api/server/lib/eraseTeam.ts
🧰 Additional context used
🧬 Code graph analysis (1)
apps/meteor/tests/end-to-end/api/teams.ts (3)
packages/core-typings/src/IRoom.ts (1)
IRoom(21-95)apps/meteor/tests/data/api-data.ts (2)
request(10-10)credentials(39-42)apps/meteor/tests/e2e/utils/create-target-channel.ts (1)
deleteRoom(48-50)
🪛 Biome (2.1.2)
apps/meteor/tests/end-to-end/api/teams.ts
[error] 1509-1526: Disallow duplicate setup and teardown hooks.
Disallow before duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
[error] 1528-1544: Disallow duplicate setup and teardown hooks.
Disallow before duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
[error] 1546-1563: Disallow duplicate setup and teardown hooks.
Disallow before duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
[error] 1565-1581: Disallow duplicate setup and teardown hooks.
Disallow before duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
⏰ 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)
- GitHub Check: 🔨 Test Storybook / Test Storybook
- GitHub Check: 🔎 Code Check / Code Lint
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/meteor/app/lib/server/functions/relinquishRoomOwnerships.ts (1)
10-31: Critical: Team main rooms deleted twice, causing potential errors.Team main rooms undergo double deletion:
eraseTeam(userId, team, [])at Line 28 callseraseRoom(team.roomId, userId)(seeeraseTeam.tsLine 17), which deletes the team's main room.Rooms.removeByIds(rids)at Line 48 attempts to delete the same room again, since team main rooms are included inrids.Depending on how
eraseRoomorRooms.removeByIdshandle non-existent rooms, this could throw errors or cause inconsistent state.Solution: Exclude team main rooms from the final
Rooms.removeByIds(rids)call, since they are already handled byeraseTeam.Apply this diff:
const bulkRoomCleanUp = async (rids: string[], userId?: string) => { // no bulk deletion for files await Promise.all(rids.map((rid) => FileUpload.removeFilesByRoomId(rid))); + const rooms = userId ? await Rooms.findByIds(rids, { projection: { _id: 1, teamMain: 1 } }).toArray() : []; + const teamMainRoomIds = new Set(rooms.filter((room) => room.teamMain).map((room) => room._id)); + await Promise.all([ Subscriptions.removeByRoomIds(rids, { async onTrash(doc) { void notifyOnSubscriptionChanged(doc, 'removed'); }, }), Messages.removeByRoomIds(rids), ReadReceipts.removeByRoomIds(rids), ...[userId && bulkTeamCleanup(rids, userId)], ]); - await Rooms.removeByIds(rids); + // Exclude team main rooms as they are already deleted by eraseTeam + const roomsToDelete = rids.filter((rid) => !teamMainRoomIds.has(rid)); + if (roomsToDelete.length > 0) { + await Rooms.removeByIds(roomsToDelete); + } };
🧹 Nitpick comments (2)
apps/meteor/app/lib/server/functions/relinquishRoomOwnerships.ts (2)
19-21: Move userId validation beforePromise.allfor efficiency.The userId validation is executed inside the map callback for every team ID. Since userId is constant across all iterations, validate it once before entering
Promise.allto avoid redundant checks.Apply this diff:
const bulkTeamCleanup = async (rids: IRoom['_id'][], userId: string) => { const rooms = await Rooms.findByIds(rids).toArray(); const teamsToRemove = rooms.filter((room) => room.teamMain); const teamIds = teamsToRemove.map((room) => room.teamId).filter((teamId) => teamId !== undefined); const uniqueTeamIds = [...new Set(teamIds)]; + + if (!userId) { + throw new Error('error-user-not-found'); + } await Promise.all( uniqueTeamIds.map(async (teamId) => { - if (!userId) { - throw new Error('error-user-not-found'); - } - const team = await Team.findOneById(teamId); if (!team) { throw new Error('error-team-not-found'); } await eraseTeam(userId, team, []); }), ); };
45-45: Prefer cleaner conditional promise pattern.The spread pattern
...[userId && bulkTeamCleanup(rids, userId)]works (Promise.all handles thefalsevalue when userId is falsy), but is not idiomatic.Apply this diff for clearer intent:
await Promise.all([ Subscriptions.removeByRoomIds(rids, { async onTrash(doc) { void notifyOnSubscriptionChanged(doc, 'removed'); }, }), Messages.removeByRoomIds(rids), ReadReceipts.removeByRoomIds(rids), - ...[userId && bulkTeamCleanup(rids, userId)], + ...(userId ? [bulkTeamCleanup(rids, userId)] : []), ]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/meteor/app/lib/server/functions/relinquishRoomOwnerships.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/meteor/app/lib/server/functions/relinquishRoomOwnerships.ts (5)
packages/core-typings/src/IRoom.ts (1)
IRoom(21-95)apps/meteor/app/api/server/lib/eraseTeam.ts (1)
eraseTeam(6-29)apps/meteor/app/file-upload/server/lib/FileUpload.ts (1)
FileUpload(110-681)apps/meteor/app/lib/server/lib/notifyListener.ts (1)
notifyOnSubscriptionChanged(515-519)packages/core-typings/src/IUser.ts (1)
IUser(186-255)
⏰ 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 comments (1)
apps/meteor/app/lib/server/functions/relinquishRoomOwnerships.ts (1)
51-56: LGTM: Clean signature extension with proper propagation.The optional
deletedByparameter is correctly added and propagated tobulkRoomCleanUp, enabling context-aware team cleanup during user deletion. The parameter naming clearly conveys intent.Also applies to: 72-72
The current behavior first deletes the rooms listed in If the team’s main room ID is included in To fix this, I’ll filter out the team’s main room ID from The fix related to this should in the pr #36807 |
apps/meteor/app/lib/server/functions/relinquishRoomOwnerships.ts
Outdated
Show resolved
Hide resolved
apps/meteor/app/lib/server/functions/relinquishRoomOwnerships.ts
Outdated
Show resolved
Hide resolved
apps/meteor/app/lib/server/functions/relinquishRoomOwnerships.ts
Outdated
Show resolved
Hide resolved
apps/meteor/app/lib/server/functions/relinquishRoomOwnerships.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/meteor/app/lib/server/functions/relinquishRoomOwnerships.ts (1)
44-44: Verify the double-deletion flow is safe.Based on past review discussions, there's a known issue where the team's main room may be deleted twice:
- First by
eraseTeam(called withinbulkTeamCleanupat line 24, which internally callseraseRoomfor the main room)- Then by
Rooms.removeByIds(rids)at line 44 if the main room ID is inridsPer previous comments, this is being tracked separately in CORE-1432 and
Rooms.removeByIdsis expected to be idempotent (won't fail on non-existent IDs). However, sinceeraseRoom"does a lot more than simply delete from mongo" (past comment), please confirm this flow is safe and won't cause cascading issues.
🧹 Nitpick comments (1)
apps/meteor/app/lib/server/functions/relinquishRoomOwnerships.ts (1)
41-41: Prefer explicit ternary for conditional Promise inclusion.The pattern
...[userId && bulkTeamCleanup(rids, userId)]works but relies on spreading a falsy value into the array whenuserIdis undefined. A more explicit and readable approach:- ...[userId && bulkTeamCleanup(rids, userId)], + ...(userId ? [bulkTeamCleanup(rids, userId)] : []),This makes the intent clearer: conditionally add the Promise to the array only when
userIdis defined.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/meteor/app/lib/server/functions/relinquishRoomOwnerships.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
PR: RocketChat/Rocket.Chat#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/app/lib/server/functions/relinquishRoomOwnerships.ts
📚 Learning: 2025-09-25T09:59:26.461Z
Learnt from: Dnouv
PR: RocketChat/Rocket.Chat#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/app/lib/server/functions/relinquishRoomOwnerships.ts
🧬 Code graph analysis (1)
apps/meteor/app/lib/server/functions/relinquishRoomOwnerships.ts (3)
packages/core-typings/src/IRoom.ts (1)
IRoom(21-95)apps/meteor/app/api/server/lib/eraseTeam.ts (1)
eraseTeam(6-25)apps/meteor/app/file-upload/server/lib/FileUpload.ts (1)
FileUpload(110-681)
⏰ 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 comments (2)
apps/meteor/app/lib/server/functions/relinquishRoomOwnerships.ts (2)
10-27: LGTM! Good defensive programming with early return.The implementation correctly:
- Deduplicates team IDs to avoid redundant eraseTeam calls
- Returns early if a team is not found (line 21) rather than throwing, which is appropriate since teams may have been deleted already or could be stale references
- Uses the proper
Promise.allwithmap(async ...)patternThe empty array passed to
eraseTeamat line 24 is intentional: it triggers deletion of the team's main room and team entity while preserving (un-setting teamId from) other team rooms, which is the correct behavior when the last team owner is being removed.
47-52: LGTM! Deletion context now propagates correctly.The new
deletedByparameter enables the user deletion context to flow through tobulkRoomCleanUpand ultimately trigger team cleanup viabulkTeamCleanup. This is the key fix for preventing orphaned teams when the last owner is deleted.The type
IUser['_id']is consistent with theuserId: stringparameter inbulkRoomCleanUp, and the optional nature ensures backward compatibility.Also applies to: 68-68
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/meteor/app/lib/server/functions/relinquishRoomOwnerships.ts (2)
10-33: Tighten typing and aggregation inbulkTeamCleanup.The overall flow (identify
teamMainrooms, dedupeteamIds, calleraseTeamOnRelinquishRoomOwnerships, aggregate deleted IDs) looks correct and aligns with the PR goals. Two small improvements:
teamIdsstill has a potentialundefinedin its type even afterfilter((teamId) => teamId !== undefined). To keep TypeScript happy and avoid accidentally querying_id: undefined, consider a type‑guard filter:const teamIds = teamsToRemove .map((room) => room.teamId) .filter((teamId): teamId is string => typeof teamId === 'string');
- Since
deletedRoomIdsis populated from multiple async branches and may also contain rooms beyond the originalrids, using aSet<string>here (mirroringeraseTeamOnRelinquishRoomOwnerships) would avoid any accidental duplication:const deletedRoomIds = new Set<string>(); await Promise.all( uniqueTeamIds.map(async (teamId) => { const team = await Team.findOneById(teamId); if (!team) { return; } const ids = await eraseTeamOnRelinquishRoomOwnerships(team, []); ids.forEach((id) => deletedRoomIds.add(id)); }), ); return [...deletedRoomIds];
35-61: Verify cleanup for rooms deleted only via team erasure and consider minor optimization.The new
bulkRoomCleanUpcorrectly delegates team‑owned rooms tobulkTeamCleanupfirst and then runseraseRoomLooseValidationon the remaining room IDs, returning all deleted IDs. Two follow‑ups:
Rooms deleted indirectly via
eraseTeamOnRelinquishRoomOwnerships(i.e., team rooms not present in the originalrids) will not go through the top‑levelFileUpload.removeFilesByRoomId(rid)call on Line 37. This is fine ifdeleteRoom(invoked byeraseRoomLooseValidation) already takes care of uploads for those rooms; otherwise, uploads for those extra team rooms could be left behind.Please confirm whether
deleteRoomfully handles file cleanup for any room it deletes. If it does not, you likely need to either:
- call
FileUpload.removeFilesByRoomIdinside theeraseTeamOnRelinquishRoomOwnershipscallback for eachridit deletes, or- after
bulkTeamCleanup, invokeremoveFilesByRoomIdfor anydeletedRoomIdsthat are not already in the originalrids.Minor:
restRidsToRemoveis computed viadeletedRoomIds.includes(rid), which is O(n²) in the worst case. Since membership is all you need there, you can snapshot the team‑deleted IDs into aSetfor that filter:const deletedByTeam = new Set(deletedRoomIds); const restRidsToRemove = rids.filter((rid) => !deletedByTeam.has(rid));The rest of the logic, including returning
deletedRoomIdsso callers can surfacedeletedRooms, looks good.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/meteor/app/lib/server/functions/relinquishRoomOwnerships.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (7)
📚 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/app/lib/server/functions/relinquishRoomOwnerships.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/app/lib/server/functions/relinquishRoomOwnerships.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/lib/server/functions/relinquishRoomOwnerships.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/lib/server/functions/relinquishRoomOwnerships.ts
📚 Learning: 2025-10-24T17:32:05.348Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37299
File: apps/meteor/ee/server/lib/ldap/Manager.ts:438-454
Timestamp: 2025-10-24T17:32:05.348Z
Learning: In Rocket.Chat, ABAC attributes can only be set on private rooms and teams (type 'p'), not on public rooms (type 'c'). Therefore, when checking for ABAC-protected rooms/teams during LDAP sync or similar operations, it's sufficient to query only private rooms using methods like `findPrivateRoomsByIdsWithAbacAttributes`.
Applied to files:
apps/meteor/app/lib/server/functions/relinquishRoomOwnerships.ts
📚 Learning: 2025-09-23T00:27:05.438Z
Learnt from: pierre-lehnen-rc
Repo: RocketChat/Rocket.Chat PR: 36718
File: packages/media-signaling/src/lib/Call.ts:633-642
Timestamp: 2025-09-23T00:27:05.438Z
Learning: In PR #36718, pierre-lehnen-rc prefers to maintain consistency with the old architecture patterns for DTMF handling rather than implementing immediate validation improvements, deferring enhancements to future work.
Applied to files:
apps/meteor/app/lib/server/functions/relinquishRoomOwnerships.ts
📚 Learning: 2025-09-16T13:33:49.237Z
Learnt from: cardoso
Repo: RocketChat/Rocket.Chat PR: 36890
File: apps/meteor/tests/e2e/e2e-encryption/e2ee-otr.spec.ts:21-26
Timestamp: 2025-09-16T13:33:49.237Z
Learning: In Rocket.Chat test files, the im.delete API endpoint accepts either a `roomId` parameter (requiring the actual DM room _id) or a `username` parameter (for the DM partner's username). It does not accept slug-like constructions such as concatenating usernames together.
Applied to files:
apps/meteor/app/lib/server/functions/relinquishRoomOwnerships.ts
🧬 Code graph analysis (1)
apps/meteor/app/lib/server/functions/relinquishRoomOwnerships.ts (4)
packages/core-typings/src/IRoom.ts (1)
IRoom(21-95)apps/meteor/app/api/server/lib/eraseTeam.ts (2)
eraseTeamOnRelinquishRoomOwnerships(55-64)eraseRoomLooseValidation(66-96)apps/meteor/app/file-upload/server/lib/FileUpload.ts (1)
FileUpload(110-681)apps/meteor/app/lib/server/lib/notifyListener.ts (1)
notifyOnSubscriptionChanged(515-519)
⏰ 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). (5)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: ⚙️ Variables Setup
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (1)
apps/meteor/app/lib/server/functions/relinquishRoomOwnerships.ts (1)
63-84: All callsites properly handle the newstring[]return.Verification confirms both callsites handle the return value correctly:
- deleteUser.ts (line 68): Captures the return value into the
deletedRoomsvariable and includes it in the function's return object ({ deletedRooms }), fully consistent with the new API.- setUserActiveStatus.ts (line 97): Return value is intentionally not captured; the call is within
Promise.allSettled()where the caller only cares that the operation completes.No other callsites exist.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/meteor/app/api/server/lib/eraseTeam.spec.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-06T20:30:45.540Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37152
File: packages/apps-engine/tests/test-data/storage/storage.ts:101-122
Timestamp: 2025-10-06T20:30:45.540Z
Learning: In `packages/apps-engine/tests/test-data/storage/storage.ts`, the stub methods (updatePartialAndReturnDocument, updateStatus, updateSetting, updateAppInfo, updateMarketplaceInfo) intentionally throw "Method not implemented." Tests using these methods must stub them using `SpyOn` from the test library rather than relying on actual implementations.
Applied to files:
apps/meteor/app/api/server/lib/eraseTeam.spec.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: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (3)
apps/meteor/app/api/server/lib/eraseTeam.spec.ts (3)
1-92: LGTM! Comprehensive test scaffolding.The test setup properly uses proxyquire with noCallThru() to isolate dependencies, creates comprehensive stubs for all external dependencies (Team, Users, Rooms, loggers, Apps bridges), and includes proper cleanup in afterEach.
121-133: LGTM! Test correctly verifies team main room erasure.The test properly validates that
eraseTeamdelegates the team's main room deletion toeraseRoomviaeraseTeamShared, and verifies the correct arguments are passed.
163-271: LGTM! Comprehensive coverage of eraseRoomLooseValidation scenarios.The tests thoroughly cover all edge cases:
- Missing rooms return false
- Federated rooms return false
- App pre-delete prevention returns false with proper event handling
- Deletion failures log errors and return false
- Successful deletions trigger post-delete events and return true
The multiple
proxyquireinvocations in this test suite (lines 202, 225, 253) are justified because each test requires different Apps bridge configurations (varyingisLoadedstates and listener behaviors).The test at lines 242-270 correctly verifies that
roomEventStubis called twice—once for the pre-delete event (returning false to allow deletion) and once for the post-delete event.
abhinavkrin
dismissed
pierre-lehnen-rc’s stale review
Got reviewed from Lucas, Pierre on leave. I have added additional tests for the refactor.
abhinavkrin
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
dionisio-bot
bot
removed
the
stat: ready to merge
abhinavkrin
added
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
Proposed changes (including videos or screenshots)
deletedRoomsfield to theusers.deleteendpoint response, indicating which rooms were deleted as part of the user deletion process.Issue(s)
Steps to test or reproduce
Further comments
SUP-836
CORE-1432
Summary by CodeRabbit
Bug Fixes
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.