test(federation): validate empty room state #37824
Conversation
|
Looks like this PR is ready to merge! 🎉 |
|
WalkthroughThis PR refactors direct message room naming by introducing a new Changes
Sequence DiagramsequenceDiagram
actor User
participant RoomService
participant UserDB as User Database
participant getNameForDMs
participant SubscriptionDB as Subscription Store
participant NotificationService
User->>RoomService: updateDirectMessageRoomName(room)
RoomService->>SubscriptionDB: fetch subscriptions by room ID
SubscriptionDB-->>RoomService: subscriptions[]
RoomService->>UserDB: load user details by user IDs
UserDB-->>RoomService: users[]
RoomService->>getNameForDMs: compute display names from members
getNameForDMs-->>RoomService: Map<userId, {fname, name}>
loop For each subscription
RoomService->>SubscriptionDB: update subscription name/fname
SubscriptionDB-->>RoomService: update confirmed
RoomService->>NotificationService: notify on subscription changed
end
NotificationService-->>User: subscription name updated
RoomService-->>User: returns true
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)
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 |
sampaiodiego
force-pushed
the
test-federation-empty-dm-state
branch
from
57904f6 to
f4cd676
Compare
📦 Docker Image Size Report📈 Changes
📊 Historical Trend---
config:
theme: "dark"
xyChart:
width: 900
height: 400
---
xychart
title "Image Size Evolution by Service (Last 30 Days + This PR)"
x-axis ["11/15 22:28", "11/16 01:28", "11/17 23:50", "11/18 22:53", "11/19 23:02", "11/21 16:49", "11/24 17:34", "11/27 22:32", "11/28 19:05", "12/01 23:01", "12/02 21:57", "12/03 21:00", "12/04 18:17", "12/05 21:56", "12/08 20:15", "12/09 22:17", "12/10 23:26", "12/11 21:56", "12/12 22:45", "12/13 01:34", "12/15 22:31", "12/16 22:18", "12/17 21:04", "12/18 19:04", "12/18 19:24 (PR)"]
y-axis "Size (GB)" 0 --> 0.5
line "account-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "authorization-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "ddp-streamer-service" [0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12]
line "omnichannel-transcript-service" [0.14, 0.14, 0.14, 0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13]
line "presence-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "queue-worker-service" [0.14, 0.14, 0.14, 0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13]
line "rocketchat" [0.36, 0.36, 0.35, 0.35, 0.35, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.35]
line "stream-hub-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
Statistics (last 24 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Thu, 18 Dec 2025 19:24:56 GMT |
sampaiodiego
force-pushed
the
test-federation-empty-dm-state
branch
from
f4cd676 to
7030de4
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #37824 +/- ##
===========================================
+ Coverage 67.75% 67.81% +0.06%
===========================================
Files 3463 3464 +1
Lines 113702 113726 +24
Branches 20901 20905 +4
===========================================
+ Hits 77034 77128 +94
+ Misses 34501 34415 -86
- Partials 2167 2183 +16
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
sampaiodiego
force-pushed
the
test-federation-empty-dm-state
branch
5 times, most recently
from
75fc3d0 to
6674a96
Compare
ggazzo
force-pushed
the
test-federation-empty-dm-state
branch
2 times, most recently
from
83299a8 to
9b8c873
Compare
sampaiodiego
force-pushed
the
test-federation-empty-dm-state
branch
from
56a63e2 to
baefb64
Compare
sampaiodiego
force-pushed
the
test-federation-empty-dm-state
branch
from
baefb64 to
3692b7a
Compare
![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
🧹 Nitpick comments (2)
apps/meteor/server/services/room/service.ts (1)
39-55: Consider edge-case handling for empty subscriptions and missing name mappings.The method works well for the common case, but consider these edge cases:
If
subsis empty (no subscriptions),uidswill be empty,roomMemberswill be empty,roomNameswill be{}, and the loop does nothing. Returningtrueis misleading—consider returningfalseor the count of updated subscriptions.If a subscription's user ID is not in the
roomNamesmap (e.g., due to a deleted user not found byfindUsersByIds),roomNames[sub.u._id]will beundefined, and the$setwill setundefinedfields.🔎 Suggested defensive check:
for await (const sub of subs) { + const names = roomNames[sub.u._id]; + if (!names) { + continue; + } - await Subscriptions.updateOne({ _id: sub._id }, { $set: roomNames[sub.u._id] }); + await Subscriptions.updateOne({ _id: sub._id }, { $set: names }); void notifyOnSubscriptionChangedByRoomIdAndUserId(room._id, sub.u._id, 'updated'); }ee/packages/federation-matrix/tests/end-to-end/dms.spec.ts (1)
205-284: Consider extracting shared test setup logic to reduce duplication.The "Room list name validations" and "Permission validations" blocks share nearly identical setup code and initial tests (DM creation, invitation acceptance, fname display). While separate
describeblocks with independent setup is correct for test isolation, the repetitive test cases (lines 235-284 mirror 124-173) could be:
- Extracted into shared helper functions, or
- Consolidated if they don't need to run in separate contexts
The unique test in "Permission validations" is
should leave the DM from Rocket.Chat(lines 286-303), which justifies a separate block, but the preceding tests could be shared.
📜 Review details
Configuration used: Organization 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 (11)
apps/meteor/app/lib/server/functions/createDirectRoom.ts(4 hunks)apps/meteor/app/lib/server/functions/removeUserFromRoom.ts(3 hunks)apps/meteor/server/services/room/getNameForDMs.ts(1 hunks)apps/meteor/server/services/room/service.ts(2 hunks)apps/meteor/tests/data/rooms.helper.ts(1 hunks)apps/meteor/tests/unit/server/services/room/getNameForDMs.spec.ts(1 hunks)ee/packages/federation-matrix/src/events/member.ts(2 hunks)ee/packages/federation-matrix/tests/end-to-end/dms.spec.ts(2 hunks)packages/core-services/src/types/IRoomService.ts(1 hunks)packages/model-typings/src/models/IRoomsModel.ts(1 hunks)packages/models/src/models/Rooms.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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/core-services/src/types/IRoomService.tsapps/meteor/app/lib/server/functions/createDirectRoom.tsapps/meteor/server/services/room/service.tsapps/meteor/app/lib/server/functions/removeUserFromRoom.tsapps/meteor/server/services/room/getNameForDMs.tspackages/model-typings/src/models/IRoomsModel.tsapps/meteor/tests/unit/server/services/room/getNameForDMs.spec.tsapps/meteor/tests/data/rooms.helper.tsee/packages/federation-matrix/src/events/member.tspackages/models/src/models/Rooms.tsee/packages/federation-matrix/tests/end-to-end/dms.spec.ts
**/*.spec.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.spec.ts: Use descriptive test names that clearly communicate expected behavior in Playwright tests
Use.spec.tsextension for test files (e.g.,login.spec.ts)
Files:
apps/meteor/tests/unit/server/services/room/getNameForDMs.spec.tsee/packages/federation-matrix/tests/end-to-end/dms.spec.ts
🧠 Learnings (22)
📓 Common learnings
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.
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.
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37377
File: apps/meteor/ee/server/hooks/federation/index.ts:86-88
Timestamp: 2025-11-04T16:49:19.107Z
Learning: In Rocket.Chat's federation system (apps/meteor/ee/server/hooks/federation/), permission checks follow two distinct patterns: (1) User-initiated federation actions (creating rooms, adding users to federated rooms, joining from invites) should throw MeteorError to inform users they lack 'access-federation' permission. (2) Remote server-initiated federation events should silently skip/ignore when users lack permission. The beforeAddUserToRoom hook only executes for local user-initiated actions, so throwing an error there is correct. Remote federation events are handled separately by the federation Matrix package with silent skipping logic.
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37532
File: ee/packages/federation-matrix/src/FederationMatrix.ts:920-927
Timestamp: 2025-12-09T20:01:07.355Z
Learning: In Rocket.Chat's federation invite handling (ee/packages/federation-matrix/src/FederationMatrix.ts), when a user rejects an invite via federationSDK.rejectInvite(), the subscription cleanup happens automatically through an event-driven flow: Matrix emits a leave event back, which is processed by handleLeave() in ee/packages/federation-matrix/src/events/member.ts, and that function calls Room.performUserRemoval() to clean up the subscription. No explicit cleanup is needed in the reject branch of handleInvite() because the leave event handler takes care of it.
<!-- </add_learning>
📚 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:
packages/core-services/src/types/IRoomService.tsapps/meteor/app/lib/server/functions/createDirectRoom.tsapps/meteor/server/services/room/service.tsapps/meteor/app/lib/server/functions/removeUserFromRoom.tsapps/meteor/server/services/room/getNameForDMs.tspackages/model-typings/src/models/IRoomsModel.tsapps/meteor/tests/data/rooms.helper.tsee/packages/federation-matrix/src/events/member.tspackages/models/src/models/Rooms.tsee/packages/federation-matrix/tests/end-to-end/dms.spec.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/app/lib/server/functions/createDirectRoom.tsapps/meteor/server/services/room/service.tsapps/meteor/app/lib/server/functions/removeUserFromRoom.tsapps/meteor/server/services/room/getNameForDMs.tspackages/model-typings/src/models/IRoomsModel.tsapps/meteor/tests/data/rooms.helper.tspackages/models/src/models/Rooms.tsee/packages/federation-matrix/tests/end-to-end/dms.spec.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/createDirectRoom.tsapps/meteor/server/services/room/service.tsapps/meteor/app/lib/server/functions/removeUserFromRoom.tsapps/meteor/server/services/room/getNameForDMs.tspackages/model-typings/src/models/IRoomsModel.tsapps/meteor/tests/data/rooms.helper.tspackages/models/src/models/Rooms.tsee/packages/federation-matrix/tests/end-to-end/dms.spec.ts
📚 Learning: 2025-11-04T16:49:19.107Z
Learnt from: ricardogarim
Repo: RocketChat/Rocket.Chat PR: 37377
File: apps/meteor/ee/server/hooks/federation/index.ts:86-88
Timestamp: 2025-11-04T16:49:19.107Z
Learning: In Rocket.Chat's federation system (apps/meteor/ee/server/hooks/federation/), permission checks follow two distinct patterns: (1) User-initiated federation actions (creating rooms, adding users to federated rooms, joining from invites) should throw MeteorError to inform users they lack 'access-federation' permission. (2) Remote server-initiated federation events should silently skip/ignore when users lack permission. The beforeAddUserToRoom hook only executes for local user-initiated actions, so throwing an error there is correct. Remote federation events are handled separately by the federation Matrix package with silent skipping logic.
Applied to files:
apps/meteor/app/lib/server/functions/createDirectRoom.tsapps/meteor/server/services/room/service.tsapps/meteor/app/lib/server/functions/removeUserFromRoom.tsee/packages/federation-matrix/tests/end-to-end/dms.spec.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: 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). Constructing slug-like identifiers like `user2${Users.userE2EE.data.username}` doesn't work for this endpoint.
Applied to files:
apps/meteor/app/lib/server/functions/removeUserFromRoom.tspackages/model-typings/src/models/IRoomsModel.tsee/packages/federation-matrix/src/events/member.tspackages/models/src/models/Rooms.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/removeUserFromRoom.tspackages/model-typings/src/models/IRoomsModel.tsee/packages/federation-matrix/src/events/member.tspackages/models/src/models/Rooms.tsee/packages/federation-matrix/tests/end-to-end/dms.spec.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/removeUserFromRoom.tsapps/meteor/tests/data/rooms.helper.tspackages/models/src/models/Rooms.ts
📚 Learning: 2025-11-19T18:20:37.116Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37419
File: apps/meteor/server/services/media-call/service.ts:141-141
Timestamp: 2025-11-19T18:20:37.116Z
Learning: In apps/meteor/server/services/media-call/service.ts, the sendHistoryMessage method should use call.caller.id or call.createdBy?.id as the message author, not call.transferredBy?.id. Even for transferred calls, the message should appear in the DM between the two users who are calling each other, not sent by the person who transferred the call.
Applied to files:
apps/meteor/app/lib/server/functions/removeUserFromRoom.ts
📚 Learning: 2025-12-09T20:01:07.355Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37532
File: ee/packages/federation-matrix/src/FederationMatrix.ts:920-927
Timestamp: 2025-12-09T20:01:07.355Z
Learning: In Rocket.Chat's federation invite handling (ee/packages/federation-matrix/src/FederationMatrix.ts), when a user rejects an invite via federationSDK.rejectInvite(), the subscription cleanup happens automatically through an event-driven flow: Matrix emits a leave event back, which is processed by handleLeave() in ee/packages/federation-matrix/src/events/member.ts, and that function calls Room.performUserRemoval() to clean up the subscription. No explicit cleanup is needed in the reject branch of handleInvite() because the leave event handler takes care of it.
<!-- </add_learning>
Applied to files:
apps/meteor/app/lib/server/functions/removeUserFromRoom.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:
packages/model-typings/src/models/IRoomsModel.tspackages/models/src/models/Rooms.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:
packages/model-typings/src/models/IRoomsModel.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/tests/unit/server/services/room/getNameForDMs.spec.tsee/packages/federation-matrix/tests/end-to-end/dms.spec.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/room/getNameForDMs.spec.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 **/*.spec.ts : Use descriptive test names that clearly communicate expected behavior in Playwright tests
Applied to files:
apps/meteor/tests/unit/server/services/room/getNameForDMs.spec.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 : All test files must be created in `apps/meteor/tests/e2e/` directory
Applied to files:
apps/meteor/tests/unit/server/services/room/getNameForDMs.spec.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/room/getNameForDMs.spec.ts
📚 Learning: 2025-10-06T20:32:23.658Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37152
File: packages/apps-engine/tests/test-data/utilities.ts:557-573
Timestamp: 2025-10-06T20:32:23.658Z
Learning: In packages/apps-engine/tests/test-data/utilities.ts, the field name `isSubscripbedViaBundle` in the `IMarketplaceSubscriptionInfo` type should not be flagged as a typo, as it may match the upstream API's field name.
Applied to files:
apps/meteor/tests/data/rooms.helper.ts
📚 Learning: 2025-12-09T20:01:00.324Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37532
File: ee/packages/federation-matrix/src/FederationMatrix.ts:920-927
Timestamp: 2025-12-09T20:01:00.324Z
Learning: When reviewing federation invite handling in Rocket.Chat (specifically under ee/packages/federation-matrix), understand that rejecting an invite via federationSDK.rejectInvite() triggers an event-driven cleanup: a leave event is emitted and handled by handleLeave() in ee/packages/federation-matrix/src/events/member.ts, which calls Room.performUserRemoval() to remove the subscription. Do not add explicit cleanup in the reject branch of handleInvite(); rely on the existing leave-event flow for cleanup. If making changes, ensure this invariant remains and that any related paths still funnel cleanup through the leave event to avoid duplicate or missing removals.
Applied to files:
ee/packages/federation-matrix/src/events/member.tsee/packages/federation-matrix/tests/end-to-end/dms.spec.ts
📚 Learning: 2025-11-05T21:04:35.787Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37357
File: ee/packages/federation-matrix/src/setup.ts:103-120
Timestamp: 2025-11-05T21:04:35.787Z
Learning: In Rocket.Chat's federation-matrix setup (ee/packages/federation-matrix/src/setup.ts and apps/meteor/ee/server/startup/federation.ts), configureFederationMatrixSettings does not need to be called before setupFederationMatrix. The SDK's init() establishes infrastructure (database, event handlers, APIs) first, and the configuration can be applied later via settings watchers before actual federation events are processed. The config only matters when events actually occur, at which point all infrastructure is already configured.
Applied to files:
ee/packages/federation-matrix/tests/end-to-end/dms.spec.ts
📚 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:
ee/packages/federation-matrix/tests/end-to-end/dms.spec.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:
ee/packages/federation-matrix/tests/end-to-end/dms.spec.ts
🧬 Code graph analysis (7)
packages/core-services/src/types/IRoomService.ts (1)
packages/core-typings/src/IRoom.ts (1)
IRoom(22-98)
apps/meteor/app/lib/server/functions/createDirectRoom.ts (1)
apps/meteor/server/services/room/getNameForDMs.ts (1)
getNameForDMs(18-33)
apps/meteor/server/services/room/service.ts (2)
packages/models/src/index.ts (2)
Subscriptions(213-213)Users(216-216)apps/meteor/server/services/room/getNameForDMs.ts (1)
getNameForDMs(18-33)
apps/meteor/server/services/room/getNameForDMs.ts (1)
packages/core-typings/src/IUser.ts (1)
IUser(187-259)
apps/meteor/tests/unit/server/services/room/getNameForDMs.spec.ts (1)
apps/meteor/server/services/room/getNameForDMs.ts (1)
getNameForDMs(18-33)
ee/packages/federation-matrix/src/events/member.ts (2)
ee/packages/federation-matrix/src/events/room.ts (1)
room(7-87)packages/core-services/src/index.ts (1)
Room(171-171)
packages/models/src/models/Rooms.ts (1)
packages/core-typings/src/IRoom.ts (1)
IRoom(22-98)
⏰ 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 (14)
apps/meteor/app/lib/server/functions/createDirectRoom.ts (1)
161-185: Centralized DM naming viagetNameForDMs.The refactoring correctly replaces local helper functions with the shared
getNameForDMsutility. The destructuringconst { fname, name } = roomNames[member._id]assumes the member exists in the map, which is valid givengetNameForDMsiterates over the sameroomMembersarray.packages/model-typings/src/models/IRoomsModel.ts (1)
332-332: Interface addition for DM user reference cleanup.The new method signature is well-typed and consistent with other update methods in the interface.
packages/core-services/src/types/IRoomService.ts (1)
70-70: New service method for DM room name updates.The method signature is appropriate. The
IRoomparameter provides full context for computing updated names viagetNameForDMs.ee/packages/federation-matrix/src/events/member.ts (2)
80-80: Correctly excludesfnamefor direct message rooms.DMs compute display names per-user via subscriptions, so omitting
fnamefrom room extraData is correct.
235-238: DM room name refresh after member leaves.This correctly triggers subscription name updates for remaining DM members after a user leaves. Per retrieved learnings, the leave event flow handles cleanup through
performUserRemoval, and this addition ensures DM display names stay consistent.apps/meteor/app/lib/server/functions/removeUserFromRoom.ts (2)
30-33: Early validation ensures username exists.Throwing early if
usernameis missing prevents silent failures downstream and makes the requirement explicit.
77-80: DM reference cleanup on user removal.This correctly removes the departing user from the room's
usernamesanduidsarrays for direct messages, keeping the room document consistent with the subscription changes.apps/meteor/tests/unit/server/services/room/getNameForDMs.spec.ts (1)
5-128: Comprehensive unit tests forgetNameForDMs.Good coverage of edge cases including empty arrays, single members, pairs, group DMs with alphabetical sorting, and fallback to username when name is empty. Test names clearly communicate expected behavior.
packages/models/src/models/Rooms.ts (1)
2348-2363: Implementation ofremoveUserReferenceFromDMsById.The method correctly targets DM rooms by ID and type, using
$pullto atomically remove the user from bothusernamesanduidsarrays. The implementation aligns with the interface definition.apps/meteor/tests/data/rooms.helper.ts (1)
115-131: Improved error handling with proper Promise rejection.The updated signature enables dependency injection, and the function now correctly rejects when the request fails or subscription is not found. This is a breaking change for callers that relied on the promise always resolving. All current test callers properly handle rejected promises through mocha's async test framework.
apps/meteor/server/services/room/getNameForDMs.ts (1)
18-33: LGTM on the overall design.The function logic for computing per-member display names is sound. The fallback chains on lines 27-28 handle edge cases where other members may not have names. The O(n²) complexity from the filter inside the loop is acceptable given DMs typically have a small number of members.
ee/packages/federation-matrix/tests/end-to-end/dms.spec.ts (3)
124-147: Well-structured async test with proper retry handling.The test correctly uses the
retryhelper to handle async federation operations and includes comprehensive assertions verifying room properties (_id,t,uids, absence offname) and subscription state (status: 'INVITED'). This aligns with the PR objective of validating empty room state.
175-202: Core test for empty room state validation.This test effectively validates the PR's main objective—ensuring correct room name display when a user is alone in a federated DM. The assertions properly verify:
- Subscription
nameandfnamereflect the remaining user's identity- Room
usersCountis 1uidsandusernamesarrays contain only the remaining user
286-303: LGTM on the leave DM test.The test correctly verifies that leaving a federated DM from Rocket.Chat side emits the expected Matrix leave event. Good use of the event promise pattern to verify cross-server synchronization.
| export function getNameForDMs(members: AtLeast<IUser, '_id' | 'name' | 'username'>[]): NameMap { | ||
| const nameMap: NameMap = {}; | ||
|
|
||
| const sortedMembers = members.sort((u1, u2) => (u1.name! || u1.username!).localeCompare(u2.name! || u2.username!)); |
There was a problem hiding this comment.
Mutates input array and uses unsafe non-null assertions.
Two issues on this line:
-
members.sort()mutates the input array in place. If callers don't expect mutation, this could cause subtle bugs. Consider using[...members].sort()to sort a copy. -
The non-null assertions
u1.name!andu1.username!are unsafe—if bothnameandusernameareundefined(which is possible perIUsertype), the expression evaluates toundefinedandlocaleComparethrows.
🔎 Suggested fix:
- const sortedMembers = members.sort((u1, u2) => (u1.name! || u1.username!).localeCompare(u2.name! || u2.username!));
+ const sortedMembers = [...members].sort((u1, u2) => (u1.name || u1.username || '').localeCompare(u2.name || u2.username || ''));🤖 Prompt for AI Agents
In apps/meteor/server/services/room/getNameForDMs.ts around line 21, the current
sort mutates the input array and uses unsafe non-null assertions; change to sort
a shallow copy (e.g., [...members]) and replace u1.name! || u1.username! with a
safe fallback such as (u1.name ?? u1.username ?? '') and same for u2 so
localeCompare never receives undefined; ensure you call localeCompare on a
string (or use String(...).localeCompare(...)) to avoid runtime errors.
| { | ||
| username: userDm, | ||
| password: 'random', | ||
| email: `${userDm}}@rocket.chat`, |
There was a problem hiding this comment.
Fix typo: double closing brace in email template.
The email has an extra } character: ${userDm}}@rocket.chat should be ${userDm}@rocket.chat. This same typo appears in multiple places (lines 110, 221, 362, 373).
🔎 Apply this fix in all occurrences:
- email: `${userDm}}@rocket.chat`,
+ email: `${userDm}@rocket.chat`,📝 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.
| email: `${userDm}}@rocket.chat`, | |
| email: `${userDm}@rocket.chat`, |
🤖 Prompt for AI Agents
In ee/packages/federation-matrix/tests/end-to-end/dms.spec.ts around lines 110,
221, 362 and 373, there's a typo in the email template using a double closing
brace (`${userDm}}@rocket.chat`); remove the extra `}` so the template reads
`${userDm}@rocket.chat` at each occurrence to correct the generated email
addresses.
dionisio-bot
bot
added
the
stat: ready to merge
Co-authored-by: Guilherme Gazzo <guilherme@gazzo.xyz>
Proposed changes (including videos or screenshots)
FB-151
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.