feat(federation): add room invites support #37532
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 |
|
WalkthroughAdds federated room-invite handling: new POST /v1/rooms.invite endpoint, FederationMatrix.handleInvite, INVITED subscription status and inviter fields, accept/reject flows and APIs, subscription/model/service changes, new message types/localization, and related federation hooks and route handlers. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as REST API (/v1/rooms.invite)
participant FedMatrix as FederationMatrix
participant SubModel as Subscriptions Model
participant FedSDK as Federation SDK
participant RoomSvc as RoomService
Client->>API: POST /v1/rooms.invite { roomId, action }
API->>FedMatrix: handleInvite(roomId, userId, action)
FedMatrix->>SubModel: findInvitedSubscription(roomId, userId)
SubModel-->>FedMatrix: subscription (status: INVITED)
alt action == "accept"
FedMatrix->>FedSDK: acceptInvite/processInvite (matrix identifiers)
FedSDK-->>FedMatrix: event/result
FedMatrix->>RoomSvc: performAcceptRoomInvite(room, subscription, user)
RoomSvc->>SubModel: acceptInvitationById(subscriptionId) / update subscription
RoomSvc->>RoomSvc: run app hooks & post-join callbacks
else action == "reject"
FedMatrix->>FedSDK: rejectInvite (matrix identifiers)
FedSDK-->>FedMatrix: result
FedMatrix->>SubModel: optional cleanup/notify
end
FedMatrix-->>API: success/failure
API-->>Client: 200 / error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ee/packages/federation-matrix/src/api/_matrix/profiles.ts (1)
368-386: Inconsistent null handling between the two response paths needs fixing.The optional chaining added on line 376 (
response?.[field]) indicatesresponsemay be undefined. However, line 383 returnsresponsedirectly without the same safety check, creating an inconsistency.Evidence from the codebase shows similar SDK methods can return null/undefined:
downloadFromRemoteServerexplicitly checks for null (MatrixMediaService.ts:107)getEventByIdchecks if undefined before returning (transactions.ts:443)If
queryProfilecan return undefined:
- Both code paths (line 376 and 383) need consistent null handling
- Returning HTTP 200 with undefined body may violate the schema contract
Recommendation: Add explicit error handling if response is null/undefined, or document that
queryProfileguarantees a valid object to justify removing the optional chaining.
🧹 Nitpick comments (8)
packages/i18n/src/locales/en.i18n.json (1)
5565-5566: Copy tweak for article consistency.Use “the” as in the previous line (“to the room”).
- "User_rejected_invitation_to_room": "rejected invitation to room", + "User_rejected_invitation_to_room": "rejected the invitation to the room",apps/meteor/app/api/server/v1/rooms.ts (1)
1-1: rooms.invite route wiring looks good; minor nits on validation/error handlingThe new
rooms.inviteroute correctly restricts actions to the authenticated user (viathis.userIdinFederationMatrix.handleInvite) and validates the payload viaisRoomsInviteProps, which is great for safety.Two small suggestions:
- Since
isRoomsInvitePropsalready constrainssubscriptionIdandaction, the explicit!subscriptionIdandaction !== 'accept' && action !== 'reject'checks are redundant; you can rely purely on ajv to avoid duplicate validation logic.- In the
catchblock, you currently only surface the error message to the client; consider also logging the raw error server‑side (if not already logged inhandleInvite) to aid debugging of Matrix/federation issues.Also applies to: 18-19, 1079-1104
packages/core-services/src/types/IRoomService.ts (1)
34-48: addUserToRoom options extension matches subscription invite metadataAdding
invited?: booleanandfederation?: { inviteEventId?: string; inviterUsername?: string }to theaddUserToRoomoptions cleanly exposes the invite metadata used by the new Matrix flows and matches theISubscriptionshape.If you expect this metadata to evolve, you might consider typing
federationas aPick<ISubscription, 'federation'>['federation']to avoid duplicating the structure, but the current explicit interface is also fine.ee/packages/federation-matrix/src/FederationMatrix.ts (1)
1-1: handleInvite implementation is consistent; consider centralizing Matrix user ID derivationThe new
handleInvitemethod correctly:
- Ensures the subscription belongs to the current user and is flagged
invited: true.- Requires
federation.inviteEventIdto be present.- Looks up the user and emits appropriate system messages for accept (
uj) and reject (uir).A couple of small improvements:
- You compute
matrixUserIdbefore checkinguser.username; for readability, consider moving the!user.usernameguard before constructing the local MXID branch.- The logic that turns a
IUserinto a Matrix user ID now exists in several places (here and in helpers). Extracting a shared helper for "user → Matrix ID" would reduce drift if the federation rules change.Also applies to: 10-10, 902-937
ee/packages/federation-matrix/src/events/helpers.ts (1)
12-84: Federated user/room helpers look correct; minor cleanups possibleThe helpers nicely encapsulate the recurring patterns:
getOrCreateFederatedUsercorrectly distinguishes local vs remote Matrix IDs viagetUsernameServername, avoids auto‑creating local users, and delegates remote creation tocreateOrUpdateFederatedUser.getOrCreateFederatedRoomuses the Matrix room ID as the lookup key (federation.mrid) and passesfederatedRoomId/fnameintoRoom.create, which is consistent with the newICreateRoomOptionsfields.Small nits:
- The
_inviterMatrixIdparameter is currently unused; if there’s no near‑term use planned, you can drop it to keep the signature minimal.- For clarity, it might be worth renaming the first parameter of
getOrCreateFederatedRoomto something likematrixRoomIdto distinguish it from the human display name.apps/meteor/app/lib/server/functions/addUserToRoom.ts (1)
22-41: Invite/federation plumbing in addUserToRoom is coherent; tiny message nitThe changes here line up well with the new invite flow:
invitedandfederationoptions are threaded through and conditionally added to the subscription document, matching the newISubscriptionfields.- The system-message logic now:
- Requires
inviter.usernamebefore using inviter‑specific branches.- Emits
uiwheninvitedis true, carrying the inviter asuin the payload, which matches the new invite message type.Minor nit: the
uibranch doesn’t reuse theextraDataobject or set an explicittslike theau/team messages. If you want identical timestamp semantics, you could pass{ ts: now, u: { ... } }instead of constructing a separate object.Also applies to: 102-113, 123-147
ee/packages/federation-matrix/src/events/member.ts (2)
55-111: handleInvite flow is solid; tighten room name fallback and type usageThe invite handler correctly:
- Resolves/creates inviter and invitee via
getOrCreateFederatedUser.- Derives room type (
'd'for direct,'c'otherwise) and a human‑readableroomFName.- Ensures a federated room exists via
getOrCreateFederatedRoom, then callsRoom.addUserToRoomwithinvitedandfederationmetadata so downstream logic can treat it as a pending invite.Two small points to consider:
- The fallback
roomName = \${room_id}:${roomOriginDomain}`will produce strings like!abc:server:undefinedwhenroomOriginDomainis missing; you might want to default to justroom_idin that case to avoid oddfname` values._inviterMatrixIdpassed intogetOrCreateFederatedRoomis unused; either remove the parameter or start using it (e.g., if you later need the Matrix ID inside room creation).
113-158: Join handler correctly distinguishes direct joins vs invite acceptanceThe join handler behavior looks good:
- If no local room exists yet, it falls back to
membershipJoinAction, preserving the legacy behavior.- If there’s no subscription for the joining user, it also delegates to
membershipJoinAction, treating it as a regular join.- When a subscription exists, it interprets the event as “accepting an invite” and unsets
invitedand the invite metadata while reopening the subscription.One minor improvement: you might want to guard the “accept invite” path with
if (subscription.invited)and simply return wheninvitedis falsy, to avoid unnecessary$unset/$setupdates on repeated join events for already‑joined users.
📜 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 (20)
apps/meteor/app/api/server/v1/rooms.ts(3 hunks)apps/meteor/app/lib/lib/MessageTypes.ts(1 hunks)apps/meteor/app/lib/server/functions/addUserToRoom.ts(4 hunks)apps/meteor/lib/publishFields.ts(1 hunks)apps/meteor/server/lib/dataExport/exportRoomMessagesToFile.ts(1 hunks)apps/meteor/server/modules/watchers/watchers.module.ts(1 hunks)ee/packages/federation-matrix/src/FederationMatrix.ts(3 hunks)ee/packages/federation-matrix/src/api/_matrix/invite.ts(1 hunks)ee/packages/federation-matrix/src/api/_matrix/profiles.ts(1 hunks)ee/packages/federation-matrix/src/api/_matrix/rooms.ts(1 hunks)ee/packages/federation-matrix/src/events/helpers.ts(1 hunks)ee/packages/federation-matrix/src/events/member.ts(2 hunks)packages/apps-engine/src/definition/messages/MessageType.ts(1 hunks)packages/core-services/src/types/IFederationMatrixService.ts(2 hunks)packages/core-services/src/types/IRoomService.ts(1 hunks)packages/core-typings/src/IMessage/IMessage.ts(1 hunks)packages/core-typings/src/ISubscription.ts(2 hunks)packages/i18n/src/locales/en.i18n.json(2 hunks)packages/message-types/src/registrations/common.ts(1 hunks)packages/rest-typings/src/v1/rooms.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (9)
📓 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.
📚 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:
ee/packages/federation-matrix/src/FederationMatrix.tsee/packages/federation-matrix/src/events/member.tsee/packages/federation-matrix/src/events/helpers.tsapps/meteor/app/lib/server/functions/addUserToRoom.tsee/packages/federation-matrix/src/api/_matrix/invite.tspackages/core-services/src/types/IFederationMatrixService.tsapps/meteor/app/api/server/v1/rooms.tsapps/meteor/server/lib/dataExport/exportRoomMessagesToFile.tspackages/core-services/src/types/IRoomService.tsee/packages/federation-matrix/src/api/_matrix/rooms.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/src/FederationMatrix.tsee/packages/federation-matrix/src/events/member.tsee/packages/federation-matrix/src/events/helpers.tsee/packages/federation-matrix/src/api/_matrix/invite.tspackages/core-services/src/types/IFederationMatrixService.tsapps/meteor/app/api/server/v1/rooms.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:
ee/packages/federation-matrix/src/FederationMatrix.tsee/packages/federation-matrix/src/events/member.tsee/packages/federation-matrix/src/events/helpers.tsapps/meteor/app/lib/server/functions/addUserToRoom.tsee/packages/federation-matrix/src/api/_matrix/invite.tspackages/core-services/src/types/IFederationMatrixService.tsapps/meteor/app/api/server/v1/rooms.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/src/FederationMatrix.tsapps/meteor/lib/publishFields.tsee/packages/federation-matrix/src/events/member.tsee/packages/federation-matrix/src/events/helpers.tsapps/meteor/app/lib/server/functions/addUserToRoom.tsee/packages/federation-matrix/src/api/_matrix/invite.tspackages/core-services/src/types/IFederationMatrixService.tsapps/meteor/app/api/server/v1/rooms.tsee/packages/federation-matrix/src/api/_matrix/rooms.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:
ee/packages/federation-matrix/src/FederationMatrix.tsee/packages/federation-matrix/src/events/member.tsee/packages/federation-matrix/src/events/helpers.tsapps/meteor/app/lib/server/functions/addUserToRoom.tspackages/core-services/src/types/IFederationMatrixService.tsapps/meteor/app/api/server/v1/rooms.tspackages/core-services/src/types/IRoomService.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:
ee/packages/federation-matrix/src/FederationMatrix.tsee/packages/federation-matrix/src/events/member.tsee/packages/federation-matrix/src/events/helpers.tsapps/meteor/app/lib/server/functions/addUserToRoom.tspackages/core-services/src/types/IFederationMatrixService.tsapps/meteor/app/api/server/v1/rooms.tspackages/core-services/src/types/IRoomService.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/lib/publishFields.tspackages/core-typings/src/ISubscription.tsapps/meteor/server/modules/watchers/watchers.module.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/addUserToRoom.ts
🧬 Code graph analysis (6)
ee/packages/federation-matrix/src/FederationMatrix.ts (2)
packages/core-typings/src/ISubscription.ts (1)
ISubscription(10-81)packages/core-typings/src/IUser.ts (2)
IUser(186-255)isUserNativeFederated(276-277)
ee/packages/federation-matrix/src/events/member.ts (3)
ee/packages/federation-matrix/src/FederationMatrix.ts (1)
handleInvite(902-937)ee/packages/federation-matrix/src/events/helpers.ts (2)
getOrCreateFederatedUser(12-48)getOrCreateFederatedRoom(50-84)packages/core-typings/src/IRoom.ts (1)
RoomID(12-12)
ee/packages/federation-matrix/src/events/helpers.ts (4)
packages/core-typings/src/IUser.ts (1)
IUser(186-255)ee/packages/federation-matrix/src/FederationMatrix.ts (3)
getUsernameServername(73-85)createOrUpdateFederatedUser(92-133)extractDomainFromMatrixUserId(60-66)packages/core-typings/src/IRoom.ts (2)
RoomID(12-12)IRoom(21-95)packages/core-services/src/index.ts (1)
Room(169-169)
apps/meteor/app/lib/server/functions/addUserToRoom.ts (1)
packages/core-services/src/index.ts (1)
Message(184-184)
packages/core-services/src/types/IFederationMatrixService.ts (2)
packages/core-typings/src/ISubscription.ts (1)
ISubscription(10-81)packages/core-typings/src/IUser.ts (1)
IUser(186-255)
apps/meteor/app/api/server/v1/rooms.ts (2)
packages/rest-typings/src/v1/rooms.ts (1)
isRoomsInviteProps(712-712)ee/packages/federation-matrix/src/FederationMatrix.ts (1)
FederationMatrix(137-938)
⏰ 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 (17)
ee/packages/federation-matrix/src/api/_matrix/rooms.ts (1)
189-189: LGTM! Good defensive programming.The optional chaining prevents potential runtime errors when room names are undefined during filtering.
apps/meteor/server/lib/dataExport/exportRoomMessagesToFile.ts (1)
82-89: LGTM! Consistent implementation.The new message type handlers follow the established pattern and properly apply username obfuscation for the 'ui' case, consistent with similar user action types like 'au'.
apps/meteor/lib/publishFields.ts (1)
45-46: LGTM! Subscription field additions.The new
invitedandfederationfields extend subscription projections to support the federation invite workflow.packages/core-services/src/types/IFederationMatrixService.ts (2)
1-1: LGTM! Required import for new method.Adding
ISubscriptionimport supports the newhandleInvitemethod signature.
31-31: LGTM! Well-typed method signature.The
handleInvitemethod signature is clear and properly typed with union type for the action parameter.packages/core-typings/src/IMessage/IMessage.ts (1)
71-72: LGTM! Type definitions for new message types.The new 'ui' and 'uir' message type literals are properly added to the MessageTypes array, enabling type-safe usage across the codebase.
packages/apps-engine/src/definition/messages/MessageType.ts (1)
78-81: LGTM! Clear type definitions with descriptive comments.The new message types are well-documented and properly integrated into the MessageType union.
packages/message-types/src/registrations/common.ts (1)
28-38: LGTM! Consistent system message registrations.The new 'ui' and 'uir' message type registrations follow established patterns and properly integrate with the i18n system.
ee/packages/federation-matrix/src/api/_matrix/invite.ts (3)
1-10: LGTM!The import changes are clean and all imported symbols are properly utilized in the code.
174-271: LGTM!The route handler correctly validates parameters, checks permissions, and returns appropriate HTTP status codes. The permission check at line 220 appropriately returns a 403 response to inform the remote federation server that the user cannot access federation, which is the correct behavior for an HTTP endpoint.
136-172: TheacceptInvitefunction is actively used in the codebase and is not dead code.Verification confirms that
acceptInviteis imported and called inee/packages/federation-matrix/src/FederationMatrix.tsat line 570 within an invite handling promise chain. The original review comment's concern was based on incomplete information.Likely an incorrect or invalid review comment.
packages/rest-typings/src/v1/rooms.ts (2)
691-712: LGTM!The type definition and schema are well-structured with proper validation. The schema correctly enforces required fields, constrains the action to valid values, and prevents additional properties.
895-897: LGTM!The endpoint definition is consistent with the rest of the API and correctly references the new
RoomsInvitePropstype.packages/core-typings/src/ISubscription.ts (1)
76-80: LGTM!The new fields are well-structured to support federation invite tracking. The
invited?: truepattern effectively creates a boolean flag that defaults to undefined, and thefederationobject provides appropriate metadata for federated invites.packages/i18n/src/locales/en.i18n.json (1)
3366-3367: Message type labels properly wired; no action needed.The new types
uianduirare correctly defined in MessageTypes.ts (lines 33–39) with matching i18n labels in the JSON file. The Hide_System_Messages setting usesMessageTypesValuesdynamically, automatically including all registered message types. Verification confirms proper wiring across the system.apps/meteor/server/modules/watchers/watchers.module.ts (1)
140-147: Including invited/federation in watched subscription fields is appropriateExtending the
Pick<ISubscription, ...>to includeinvitedandfederationkeeps the watcher’s projection in sync withsubscriptionFieldsand ensures invite state is broadcast to other instances/clients.ee/packages/federation-matrix/src/events/helpers.ts (1)
86-103: extractDomainFromId / extractUsernameFromMatrixId are safe, defensive wrappersBoth extraction helpers handle malformed IDs by logging and returning a fallback, which is appropriate for event‑driven federation code where robustness beats hard failures.
No issues here; just ensure call sites are prepared for the empty‑string / original‑ID fallbacks when verification is important.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/app/lib/server/functions/removeUserFromRoom.ts (1)
35-37: Critical:invitedfield missing from projection.The subscription is fetched with only
{ _id: 1 }in the projection, but line 48 checkssubscription.invited. Sinceinvitedis not included in the projection, it will always beundefined, causing the new conditional branch (lines 48-49) to never execute.Apply this diff to include the
invitedfield:const subscription = await Subscriptions.findOneByRoomIdAndUserId(rid, user._id, { - projection: { _id: 1 }, + projection: { _id: 1, invited: 1 }, });
🧹 Nitpick comments (1)
ee/packages/federation-matrix/src/events/member.ts (1)
71-71: Remove redundant membership check.Line 71 checks
content.membership === 'invite', but this function is only called when membership is already 'invite' (line 184 in the dispatcher). This condition is always true here.Apply this diff:
-const roomType = content.membership === 'invite' && content?.is_direct ? 'd' : 'c'; +const roomType = content?.is_direct ? 'd' : 'c';
📜 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 (5)
apps/meteor/app/lib/lib/MessageTypes.ts(1 hunks)apps/meteor/app/lib/server/functions/removeUserFromRoom.ts(1 hunks)ee/packages/federation-matrix/src/FederationMatrix.ts(2 hunks)ee/packages/federation-matrix/src/events/member.ts(2 hunks)packages/core-typings/src/ISubscription.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/core-typings/src/ISubscription.ts
- apps/meteor/app/lib/lib/MessageTypes.ts
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
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: 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.
📚 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:
ee/packages/federation-matrix/src/FederationMatrix.tsapps/meteor/app/lib/server/functions/removeUserFromRoom.tsee/packages/federation-matrix/src/events/member.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:
ee/packages/federation-matrix/src/FederationMatrix.tsapps/meteor/app/lib/server/functions/removeUserFromRoom.tsee/packages/federation-matrix/src/events/member.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/src/FederationMatrix.tsee/packages/federation-matrix/src/events/member.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/src/FederationMatrix.tsee/packages/federation-matrix/src/events/member.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:
ee/packages/federation-matrix/src/FederationMatrix.tsapps/meteor/app/lib/server/functions/removeUserFromRoom.tsee/packages/federation-matrix/src/events/member.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:
ee/packages/federation-matrix/src/FederationMatrix.tsapps/meteor/app/lib/server/functions/removeUserFromRoom.tsee/packages/federation-matrix/src/events/member.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.ts
📚 Learning: 2025-11-10T19:06:20.146Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37408
File: apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.tsx:53-69
Timestamp: 2025-11-10T19:06:20.146Z
Learning: In the Rocket.Chat repository, do not provide suggestions or recommendations about code sections marked with TODO comments. The maintainers have already identified these as future work and external reviewers lack the full context about implementation plans and timing.
Applied to files:
ee/packages/federation-matrix/src/events/member.ts
🧬 Code graph analysis (2)
ee/packages/federation-matrix/src/FederationMatrix.ts (2)
packages/core-typings/src/ISubscription.ts (1)
ISubscription(10-81)packages/core-typings/src/IUser.ts (2)
IUser(186-255)isUserNativeFederated(276-277)
ee/packages/federation-matrix/src/events/member.ts (2)
ee/packages/federation-matrix/src/FederationMatrix.ts (1)
handleInvite(902-935)ee/packages/federation-matrix/src/events/helpers.ts (2)
getOrCreateFederatedUser(12-48)getOrCreateFederatedRoom(50-84)
⏰ 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 (5)
apps/meteor/app/lib/server/functions/removeUserFromRoom.ts (1)
48-49: Verify the semantic correctness of message type 'uir'—this is a confirmed issue.The 'uir' message type is defined as "user rejected invitation to room" (per the comment in
apps/meteor/app/lib/lib/MessageTypes.ts). However, the code at lines 48-49 sends this message when an admin/moderator removes a user in an invited state (indicated byoptions.byUserbeing present), not when the user themselves rejects the invitation.The commit history shows that 'uir' was previously used in federation code where a user actively rejected an invite. Moving this to the common
removeUserFromRoomfunction conflates two different scenarios:
- User-initiated rejection (federation workflow)
- Admin-initiated removal of invited user (your new code path)
The translation displays "User rejected invitation to room," which suggests the user took the action, creating confusion in user-facing messages when an admin revokes an invite.
Recommended actions:
- Define a distinct message type for admin invite revocation (e.g., 'uir-revoked', 'air', or 'invite-revoked'), or
- If intentionally reusing 'uir' for both scenarios, update the message type definition and translation to reflect both user rejection and admin revocation.
ee/packages/federation-matrix/src/events/member.ts (3)
113-158: LGTM! Proper invite acceptance flow.The join handler correctly distinguishes between direct joins (no prior invite) and invite acceptances, with appropriate subscription state updates to clear invite metadata and activate the subscription.
160-177: LGTM! Clean leave implementation.The leave handler appropriately delegates to the
Room.removeUserFromRoomservice method, which handles all necessary cleanup including subscription removal and user count adjustments.
182-197: LGTM! Clear event dispatching.The switch-based dispatcher cleanly routes membership events to their respective handlers with appropriate logging for unknown membership types.
ee/packages/federation-matrix/src/FederationMatrix.ts (1)
929-934: Add explicit handling for invalid action values.The method uses separate
ifstatements for 'accept' and 'reject' actions. If an invalid action value is passed, the method silently does nothing, which could mask bugs.Apply this diff to use if/else and explicitly handle invalid actions:
if (action === 'accept') { await federationSDK.acceptInvite(subscription.federation?.inviteEventId, matrixUserId); -} -if (action === 'reject') { +} else if (action === 'reject') { await federationSDK.rejectInvite(subscription.federation?.inviteEventId, matrixUserId); +} else { + throw new Error(`Invalid action: ${action}`); }⛔ Skipped due to learnings
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.
ricardogarim
force-pushed
the
feat/invites
branch
4 times, most recently
from
4776e08 to
3e950a0
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
ee/packages/federation-matrix/src/events/member.ts (1)
17-21: Validate inviter username before usage.The code uses
inviterUser.usernameon lines 62 and 71 without validation. SinceIUser.usernameis optional, this could lead to:
- Line 62: Unsafe cast to
UserIDif username is undefined- Line 71: Storing
undefinedasinviterUsernamein federation metadataAdd validation after line 21:
const inviterUser = await getOrCreateFederatedUser(senderId as UserID); if (!inviterUser) { logger.error(`Failed to get or create inviter user: ${senderId}`); return; } +if (!inviterUser.username) { + logger.error(`Inviter user ${inviterUser._id} has no username`); + return; +}Note: This issue was flagged in a previous review but appears unresolved.
Also applies to: 57-63, 69-72
🧹 Nitpick comments (4)
apps/meteor/app/lib/server/functions/acceptRoomInvite.ts (1)
9-20: Guard against mismatched room/subscription/user tripletsThe function assumes
room,subscription, anduserare consistent (same room and user). To make this helper safer against accidental misuse by future callers, consider asserting:
subscription.rid === room._idsubscription.u._id === user._idbefore proceeding, and throwing a
Meteor.Errorif they do not match. This keeps the trust boundary clear while reusing the helper in other call sites.apps/meteor/app/lib/server/methods/addUsersToRoom.ts (1)
110-121: MakeinviteResult[0]handling robust against empty resultsThe new federation invite path is wired correctly, but
inviteResult[0]assumes thatinviteUsersToRoomalways returns at least one element:const inviteResult = await FederationMatrix.inviteUsersToRoom(...); inviteOptions = { invited: true, federation: { inviteEventId: inviteResult[0].event_id, inviterUsername: user.username, }, };Today this holds when called from local user flows, but
inviteUsersToRoomcan return an empty array if the caller ever passes a combination that leads tousersToInvite.length === 0. To future‑proof this helper, consider guarding the assignment:- const inviteResult = await FederationMatrix.inviteUsersToRoom(room, [newUser.username], user); - inviteOptions = { - invited: true, - federation: { - inviteEventId: inviteResult[0].event_id, - inviterUsername: user.username, - }, - }; + const inviteResult = await FederationMatrix.inviteUsersToRoom(room, [newUser.username], user); + if (inviteResult.length > 0) { + inviteOptions = { + invited: true, + federation: { + inviteEventId: inviteResult[0].event_id, + inviterUsername: user.username, + }, + }; + }This keeps the invite metadata optional but avoids potential
undefinedaccess if the federation layer changes behavior later.Since this depends on
FederationMatrix.inviteUsersToRoom’s contract, please confirm its “no‑op” behavior and whether it can ever legitimately return[]for this call pattern.Also applies to: 123-124
apps/meteor/app/lib/server/functions/addUserToRoom.ts (1)
134-137: Consider tightening inviter type to require username.The new UI message path on line 134 uses
inviter.usernameon line 136 without validation. While this follows the existing pattern (line 129), a past reviewer (ggazzo) suggested "it should be impossible to have an inviter without username, what about fix the type?"Consider updating the
inviterparameter type fromPick<IUser, '_id' | 'username'>toPick<IUser, '_id'> & Required<Pick<IUser, 'username'>>to enforce username at the type level.Note: This applies to the entire function, not just the new code.
ee/packages/federation-matrix/src/events/helpers.ts (1)
86-93: Consider returning null instead of empty string on error.The
extractDomainFromIdfunction returns an empty string on error (line 91), which could mask failures. Callers might not distinguish between "no domain" and "error extracting domain."Consider returning
nullor the originalmatrixIdon error for clearer error signaling:-export function extractDomainFromId(matrixId: string): string { +export function extractDomainFromId(matrixId: string): string | null { try { return extractDomainFromMatrixUserId(matrixId); } catch (error) { logger.error(`Error extracting domain from Matrix ID ${matrixId}:`, error); - return ''; + return null; } }
📜 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 (18)
apps/meteor/app/api/server/v1/rooms.ts(3 hunks)apps/meteor/app/lib/lib/MessageTypes.ts(1 hunks)apps/meteor/app/lib/server/functions/acceptRoomInvite.ts(1 hunks)apps/meteor/app/lib/server/functions/addUserToRoom.ts(3 hunks)apps/meteor/app/lib/server/functions/removeUserFromRoom.ts(2 hunks)apps/meteor/app/lib/server/methods/addUsersToRoom.ts(2 hunks)apps/meteor/ee/server/hooks/federation/index.ts(1 hunks)apps/meteor/lib/publishFields.ts(1 hunks)apps/meteor/server/lib/dataExport/exportRoomMessagesToFile.ts(1 hunks)apps/meteor/server/lib/findUsersOfRoomOrderedByRole.ts(1 hunks)apps/meteor/server/modules/watchers/watchers.module.ts(1 hunks)apps/meteor/server/services/room/service.ts(2 hunks)ee/packages/federation-matrix/src/FederationMatrix.ts(4 hunks)ee/packages/federation-matrix/src/api/_matrix/invite.ts(3 hunks)ee/packages/federation-matrix/src/events/helpers.ts(1 hunks)ee/packages/federation-matrix/src/events/member.ts(1 hunks)ee/packages/federation-matrix/src/events/room.ts(1 hunks)ee/packages/federation-matrix/src/helpers/message.parsers.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/meteor/server/lib/dataExport/exportRoomMessagesToFile.ts
- apps/meteor/lib/publishFields.ts
- apps/meteor/app/lib/lib/MessageTypes.ts
- apps/meteor/server/modules/watchers/watchers.module.ts
- apps/meteor/app/api/server/v1/rooms.ts
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
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: 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: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37408
File: apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.tsx:53-69
Timestamp: 2025-11-10T19:06:20.146Z
Learning: In the Rocket.Chat repository, do not provide suggestions or recommendations about code sections marked with TODO comments. The maintainers have already identified these as future work and external reviewers lack the full context about implementation plans and timing.
📚 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/acceptRoomInvite.tsapps/meteor/server/lib/findUsersOfRoomOrderedByRole.tsee/packages/federation-matrix/src/events/room.tsee/packages/federation-matrix/src/events/member.tsapps/meteor/ee/server/hooks/federation/index.tsapps/meteor/app/lib/server/functions/removeUserFromRoom.tsapps/meteor/app/lib/server/methods/addUsersToRoom.tsee/packages/federation-matrix/src/api/_matrix/invite.tsapps/meteor/server/services/room/service.tsee/packages/federation-matrix/src/FederationMatrix.tsee/packages/federation-matrix/src/events/helpers.tsapps/meteor/app/lib/server/functions/addUserToRoom.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/acceptRoomInvite.tsapps/meteor/server/lib/findUsersOfRoomOrderedByRole.tsee/packages/federation-matrix/src/events/room.tsee/packages/federation-matrix/src/events/member.tsapps/meteor/app/lib/server/functions/removeUserFromRoom.tsapps/meteor/app/lib/server/methods/addUsersToRoom.tsapps/meteor/server/services/room/service.tsee/packages/federation-matrix/src/FederationMatrix.tsee/packages/federation-matrix/src/events/helpers.tsapps/meteor/app/lib/server/functions/addUserToRoom.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/acceptRoomInvite.tsapps/meteor/server/lib/findUsersOfRoomOrderedByRole.tsee/packages/federation-matrix/src/events/room.tsee/packages/federation-matrix/src/events/member.tsapps/meteor/app/lib/server/functions/removeUserFromRoom.tsapps/meteor/app/lib/server/methods/addUsersToRoom.tsapps/meteor/server/services/room/service.tsee/packages/federation-matrix/src/FederationMatrix.tsee/packages/federation-matrix/src/events/helpers.tsapps/meteor/app/lib/server/functions/addUserToRoom.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/acceptRoomInvite.tsee/packages/federation-matrix/src/events/room.tsee/packages/federation-matrix/src/events/member.tsapps/meteor/ee/server/hooks/federation/index.tsapps/meteor/app/lib/server/functions/removeUserFromRoom.tsapps/meteor/app/lib/server/methods/addUsersToRoom.tsee/packages/federation-matrix/src/api/_matrix/invite.tsapps/meteor/server/services/room/service.tsee/packages/federation-matrix/src/FederationMatrix.tsee/packages/federation-matrix/src/events/helpers.tsapps/meteor/app/lib/server/functions/addUserToRoom.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/src/events/room.tsee/packages/federation-matrix/src/events/member.tsapps/meteor/ee/server/hooks/federation/index.tsapps/meteor/app/lib/server/methods/addUsersToRoom.tsee/packages/federation-matrix/src/api/_matrix/invite.tsee/packages/federation-matrix/src/FederationMatrix.tsee/packages/federation-matrix/src/events/helpers.tsapps/meteor/app/lib/server/functions/addUserToRoom.ts
📚 Learning: 2025-11-10T19:06:20.146Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37408
File: apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.tsx:53-69
Timestamp: 2025-11-10T19:06:20.146Z
Learning: In the Rocket.Chat repository, do not provide suggestions or recommendations about code sections marked with TODO comments. The maintainers have already identified these as future work and external reviewers lack the full context about implementation plans and timing.
Applied to files:
ee/packages/federation-matrix/src/events/member.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:
ee/packages/federation-matrix/src/events/member.tsapps/meteor/app/lib/server/functions/removeUserFromRoom.tsapps/meteor/app/lib/server/functions/addUserToRoom.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:
ee/packages/federation-matrix/src/events/member.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/src/events/member.tsapps/meteor/ee/server/hooks/federation/index.tsapps/meteor/app/lib/server/methods/addUsersToRoom.tsee/packages/federation-matrix/src/api/_matrix/invite.tsee/packages/federation-matrix/src/FederationMatrix.tsee/packages/federation-matrix/src/events/helpers.ts
📚 Learning: 2025-10-01T12:20:13.147Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37110
File: apps/meteor/ee/server/startup/federation.ts:17-21
Timestamp: 2025-10-01T12:20:13.147Z
Learning: In Rocket.Chat's StreamerCentral broadcast for 'notify-room' events with 'user-activity', the `activity` argument is a string (e.g., 'user-typing') when a user starts typing and an empty array when the user stops typing.
Applied to files:
ee/packages/federation-matrix/src/FederationMatrix.ts
🧬 Code graph analysis (7)
apps/meteor/app/lib/server/functions/acceptRoomInvite.ts (6)
apps/meteor/server/services/room/service.ts (1)
acceptRoomInvite(86-88)packages/core-typings/src/IRoom.ts (1)
IRoom(21-95)packages/core-typings/src/ISubscription.ts (1)
ISubscription(10-81)packages/core-typings/src/IUser.ts (1)
IUser(186-255)apps/meteor/lib/callbacks.ts (1)
callbacks(252-260)apps/meteor/app/lib/server/lib/notifyListener.ts (1)
notifyOnSubscriptionChangedById(535-544)
ee/packages/federation-matrix/src/events/member.ts (2)
ee/packages/federation-matrix/src/FederationMatrix.ts (1)
handleInvite(916-950)ee/packages/federation-matrix/src/events/helpers.ts (2)
getOrCreateFederatedUser(12-48)getOrCreateFederatedRoom(50-84)
apps/meteor/app/lib/server/methods/addUsersToRoom.ts (4)
packages/core-typings/src/IRoom.ts (1)
isRoomNativeFederated(124-125)ee/packages/federation-matrix/src/FederationMatrix.ts (1)
FederationMatrix(144-951)packages/core-services/src/index.ts (1)
FederationMatrix(199-199)apps/meteor/app/lib/server/functions/addUserToRoom.ts (1)
addUserToRoom(22-173)
apps/meteor/server/services/room/service.ts (2)
packages/core-typings/src/ISubscription.ts (1)
ISubscription(10-81)apps/meteor/app/lib/server/functions/acceptRoomInvite.ts (1)
acceptRoomInvite(9-31)
ee/packages/federation-matrix/src/FederationMatrix.ts (3)
packages/core-typings/src/IRoom.ts (3)
IRoomNativeFederated(114-120)RoomID(12-12)isRoomNativeFederated(124-125)packages/core-typings/src/IUser.ts (2)
IUser(186-255)isUserNativeFederated(276-277)packages/core-typings/src/ISubscription.ts (1)
ISubscription(10-81)
ee/packages/federation-matrix/src/events/helpers.ts (4)
packages/core-typings/src/IUser.ts (1)
IUser(186-255)ee/packages/federation-matrix/src/FederationMatrix.ts (3)
getUsernameServername(80-92)createOrUpdateFederatedUser(99-140)extractDomainFromMatrixUserId(67-73)packages/core-typings/src/IRoom.ts (2)
RoomID(12-12)IRoom(21-95)packages/core-services/src/index.ts (1)
Room(169-169)
apps/meteor/app/lib/server/functions/addUserToRoom.ts (1)
packages/core-services/src/index.ts (1)
Message(184-184)
⏰ 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 (20)
ee/packages/federation-matrix/src/helpers/message.parsers.ts (1)
5-5: MatrixMessageContent now correctly mirrors the federation SDK event content typeSwitching
MatrixMessageContenttoHomeserverEventSignatures['homeserver.matrix.message']['event']['content'] & { format?: string }aligns the local type with the SDK’s Matrix event shape and keepscreateReplyContentstrongly typed against the actualevent.contentpayload. The extra& { format?: string }is safe (and covers cases whereformatis not present on the SDK type) and doesn’t alter runtime behavior.apps/meteor/app/lib/server/functions/acceptRoomInvite.ts (1)
22-30: Join flow wiring looks consistent with existing patternsUsing
beforeJoinRoom/afterJoinRoomcallbacks, updating the subscription state, broadcasting vianotifyOnSubscriptionChangedById, and emitting a'uj'system message matches the existing room-join semantics, which should keep invite acceptance aligned with normal joins.apps/meteor/ee/server/hooks/federation/index.ts (1)
1-1: Imports are consistent with downstream usageThe
FederationMatrixandbeforeAddUsersToRoomimports line up with the usages later in this file (create room, message propagation, and ensuring federated users exist locally). The wiring here looks correct and keeps the federation hooks centralized.Also applies to: 8-8
apps/meteor/app/lib/server/functions/removeUserFromRoom.ts (1)
36-37: Nice separation of “reject invite” from normal leave/kick flowsIncluding
invitedin the projection and adding thesubscription.invitedbranch with the'uir'system message clearly distinguishes:
- Kick / removal by someone else (
options.byUserpresent),- Explicit invite rejection (
subscription.invited === true),- Normal user leave (
ul/ult).This should make invite rejection UX much clearer, as long as the reject paths call
removeUserFromRoomwithoutoptions.byUser. The implementation itself looks solid.Please double‑check that all “reject invite” call sites invoke
removeUserFromRoomwithoutoptions.byUser, so they hit the new'uir'branch instead of the generic leave/kick messages.Also applies to: 51-57
apps/meteor/app/lib/server/methods/addUsersToRoom.ts (1)
1-1: Federated invite integration intoaddUsersToRoomlooks correct; ensureaccess-federationpermission is still enforced elsewhereThe method now:
- Validates federated usernames upfront and rejects them for non‑federated rooms.
- Runs
beforeAddUsersToRoomonce for all usernames, passinginviter: user.- For users not yet subscribed:
- Optionally calls
FederationMatrix.inviteUsersToRoomwhen the room is native‑federated and the user has a username.- Threads
invitedandfederationmetadata through toaddUserToRoom, so subscription/system messages can reflect the invite.- For already‑subscribed users with
username, emits the ephemeral “already in here” message viaapi.broadcast.This is a good consolidation of the invite path into the existing add‑user flow.
Given the prior pattern in federation hooks (user‑initiated federation actions should enforce
access-federationand fail loudly, while remote events should be silent when lacking permission – based on learnings), just make sure that this method (or abeforeAddUsersToRoomsubscriber) still enforcesaccess-federationfor federated rooms when appropriate.If
access-federationenforcement has been moved to a shared callback (e.g., inbeforeAddUserToRoom.ts), please double‑check that it still runs for this method’s code path, and not for remote homeserver‑initiated events.Also applies to: 88-99, 101-109, 126-136
apps/meteor/server/services/room/service.ts (1)
86-88: LGTM! Clean delegation pattern for invite acceptance.The new method follows the established service pattern by delegating to the imported
acceptRoomInvitefunction. The signature is type-safe and consistent with other methods in the class.ee/packages/federation-matrix/src/api/_matrix/invite.ts (3)
136-172: LGTM! Validate local-only invite acceptance.The
acceptInvitefunction correctly validates that both inviter and invitee are local users (not native federated) when processing invites to federated rooms, which aligns with the comment stating this is for "inside rocket chat we invite users inside rockechat."
220-230: LGTM! Proper federation permission check added.The permission check correctly validates
access-federationbefore processing the invite, and logs appropriately when access is denied. This aligns with the federation permission patterns.Based on learnings
232-261: LGTM! Comprehensive error handling for invite processing.The error handling correctly distinguishes between
NotAllowedError(403) and general errors (500), providing appropriate error codes and messages for each case.ee/packages/federation-matrix/src/events/member.ts (3)
75-98: LGTM! Clean join handling with proper validation.The
handleJoinfunction correctly validates all prerequisites (user, room, subscription) before callingRoom.acceptRoomInvite. Error handling distinguishes between critical errors (missing room) and expected conditions (missing subscription for logging).
100-116: LGTM! Straightforward leave handling.The
handleLeavefunction correctly validates user and room existence before removing the user. The implementation is clean and follows consistent error logging patterns.
118-140: LGTM! Clean event dispatcher with proper error handling.The membership event dispatcher correctly routes to specialized handlers based on membership type, with appropriate error handling and logging. The
eventIdis passed only tohandleInvitewhere it's needed for federation metadata.apps/meteor/app/lib/server/functions/addUserToRoom.ts (2)
30-40: LGTM! Clean parameter extension for federation support.The optional
invitedandfederationparameters are well-typed and don't break existing callers. The federation object structure clearly captures the invite metadata needed for federation tracking.
109-110: LGTM! Clean conditional inclusion of federation fields.The subscription creation correctly uses conditional spread operators to include
invitedandfederationfields only when present, keeping the subscription document clean.ee/packages/federation-matrix/src/FederationMatrix.ts (3)
550-589: LGTM! Enhanced invite handling with proper user filtering.The updated
inviteUsersToRoommethod correctly filters users based on whether the inviter is native federated, preventing unnecessary processing. The new return type provides useful invite event context to callers.
859-862: LGTM! Important subscription verification added.The subscription check on lines 859-862 correctly verifies the user has joined the room before sending typing notifications. This prevents inappropriate typing notifications for users who haven't actually joined the room yet.
916-950: LGTM! Proper validation order for invite handling.The
handleInvitemethod correctly validates the subscription, user, and username (line 937-939) before constructing thematrixUserId(line 942). All validations are in the proper order to prevent errors.Note: A previous review comment suggested moving username validation before matrixUserId construction, but the current code already has the correct order.
ee/packages/federation-matrix/src/events/helpers.ts (3)
12-48: LGTM! Robust federated user resolution.The
getOrCreateFederatedUserhelper correctly distinguishes between local and remote users, only auto-creating remote federated users. Local users that don't exist trigger a warning and return null, which is the correct behavior.
50-84: LGTM! Clean federated room creation helper.The
getOrCreateFederatedRoomhelper follows a clean lookup-or-create pattern and correctly uses the Room service for creation. Federation metadata and room properties are properly configured.
95-103: LGTM! Safe username extraction with fallback.The
extractUsernameFromMatrixIdfunction safely extracts the username and returns the originalmatrixIdas a fallback on error, which is a reasonable approach.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
apps/meteor/app/lib/server/functions/addUserToRoom.ts (1)
131-134: Validate inviter username before using it in system message.The
uisystem message path usesinviter.usernamewithout checking if it's defined. Sinceusernameis optional onIUser, this could result in an undefined value being stored in the message.Based on past review comments.
Apply this diff to add validation:
} else if (status === 'INVITED') { + if (!inviter.username) { + throw new Meteor.Error('error-invalid-inviter', 'Cannot create invite message without inviter username'); + } await Message.saveSystemMessage('ui', rid, userToBeAdded.username, userToBeAdded, { u: { _id: inviter._id, username: inviter.username }, });apps/meteor/app/lib/server/methods/addUsersToRoom.ts (1)
110-120: Validate inviter username before using it in invite options.Line 116 uses
user.usernamewithout checking if it's defined. Sinceusernameis optional onIUser, this could result in undefinedinviterUsernamebeing passed to the invite flow.Based on past review comments.
Apply this diff to add validation:
let inviteOptions: { status?: SubscriptionStatus; inviterUsername?: string } = {}; if (isRoomNativeFederated(room) && user && newUser.username) { + if (!user.username) { + throw new Meteor.Error('error-invalid-inviter', 'Cannot invite to federated room without inviter username'); + } await FederationMatrix.inviteUsersToRoom(room, [newUser.username], user); inviteOptions = { status: 'INVITED', inviterUsername: user.username, }; }ee/packages/federation-matrix/src/events/member.ts (1)
54-69: Validate inviter username before using it.Lines 59 and 68 use
inviterUser.usernamewithout checking if it's defined. Sinceusernameis optional onIUser, the cast toUserIDon line 59 is unsafe, and line 68 could store undefined asinviterUsername.Based on past review comments.
Apply this diff to add validation after line 18:
const inviterUser = await getOrCreateFederatedUser(senderId as UserID); if (!inviterUser) { logger.error(`Failed to get or create inviter user: ${senderId}`); return; } + if (!inviterUser.username) { + logger.error(`Inviter user ${inviterUser._id} has no username`); + return; + }
🧹 Nitpick comments (3)
ee/packages/federation-matrix/src/FederationMatrix.ts (3)
842-867: LGTM with minor optional refinement.The enhanced validation logic properly ensures the user has joined the room before sending typing notifications, preventing potential errors.
Optional refinement: Line 859 uses
localUser?._idwith optional chaining, but this is unnecessary since lines 855-857 already ensurelocalUserexists. You can simplify tolocalUser._id.- const hasUserJoinedRoom = await Subscriptions.findOneByRoomIdAndUserId(room._id, localUser?._id, { projection: { _id: 1 } }); + const hasUserJoinedRoom = await Subscriptions.findOneByRoomIdAndUserId(room._id, localUser._id, { projection: { _id: 1 } });
939-944: Conditional username validation needed for native federated users.The username validation at lines 939-941 occurs before constructing
matrixUserId(good!), but it should only be required when the user is NOT a native federated user. Native federated users useuser.federation.muidirectly and may not have a username set.Apply this diff to make username validation conditional:
const user = await Users.findOneById(userId); if (!user) { throw new Error('User not found'); } -if (!user.username) { - throw new Error('User username not found'); -} - // TODO: should use common function to get matrix user ID const matrixUserId = isUserNativeFederated(user) ? user.federation.mui : `@${user.username}:${this.serverName}`; + +if (!isUserNativeFederated(user) && !user.username) { + throw new Error('User username not found'); +}Based on learnings, this aligns with federation patterns where native federated users have different identifier requirements.
943-943: Address or track the TODO comment.The TODO suggests creating a common function to get the matrix user ID, which would improve code reusability since this pattern appears in multiple places (lines 466, 557, 610, 650, 699, 714, 747, 780, 795, 820, 864, 944).
Would you like me to open a new issue to track the creation of a common helper function for retrieving matrix user IDs?
📜 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 (13)
apps/meteor/app/lib/server/functions/acceptRoomInvite.ts(1 hunks)apps/meteor/app/lib/server/functions/addUserToRoom.ts(4 hunks)apps/meteor/app/lib/server/functions/removeUserFromRoom.ts(2 hunks)apps/meteor/app/lib/server/methods/addUsersToRoom.ts(2 hunks)apps/meteor/lib/publishFields.ts(1 hunks)apps/meteor/server/lib/findUsersOfRoomOrderedByRole.ts(1 hunks)apps/meteor/server/modules/watchers/watchers.module.ts(1 hunks)ee/packages/federation-matrix/src/FederationMatrix.ts(4 hunks)ee/packages/federation-matrix/src/events/member.ts(1 hunks)packages/core-services/src/types/IRoomService.ts(2 hunks)packages/core-typings/src/IInquiry.ts(1 hunks)packages/core-typings/src/ISubscription.ts(2 hunks)packages/models/src/models/Subscriptions.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/meteor/app/lib/server/functions/removeUserFromRoom.ts
- packages/core-typings/src/ISubscription.ts
- apps/meteor/server/lib/findUsersOfRoomOrderedByRole.ts
- apps/meteor/app/lib/server/functions/acceptRoomInvite.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:
packages/models/src/models/Subscriptions.tspackages/core-typings/src/IInquiry.tsapps/meteor/app/lib/server/functions/addUserToRoom.tsee/packages/federation-matrix/src/FederationMatrix.tspackages/core-services/src/types/IRoomService.tsee/packages/federation-matrix/src/events/member.tsapps/meteor/lib/publishFields.tsapps/meteor/server/modules/watchers/watchers.module.tsapps/meteor/app/lib/server/methods/addUsersToRoom.ts
🧠 Learnings (12)
📓 Common learnings
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: 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.
📚 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/addUserToRoom.tsee/packages/federation-matrix/src/FederationMatrix.tspackages/core-services/src/types/IRoomService.tsee/packages/federation-matrix/src/events/member.tsapps/meteor/app/lib/server/methods/addUsersToRoom.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/addUserToRoom.tsee/packages/federation-matrix/src/FederationMatrix.tspackages/core-services/src/types/IRoomService.tsee/packages/federation-matrix/src/events/member.tsapps/meteor/app/lib/server/methods/addUsersToRoom.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/addUserToRoom.tsee/packages/federation-matrix/src/FederationMatrix.tspackages/core-services/src/types/IRoomService.tsee/packages/federation-matrix/src/events/member.tsapps/meteor/app/lib/server/methods/addUsersToRoom.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/addUserToRoom.tsee/packages/federation-matrix/src/FederationMatrix.tsee/packages/federation-matrix/src/events/member.tsapps/meteor/app/lib/server/methods/addUsersToRoom.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/addUserToRoom.tsee/packages/federation-matrix/src/events/member.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/src/FederationMatrix.tsee/packages/federation-matrix/src/events/member.tsapps/meteor/app/lib/server/methods/addUsersToRoom.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/src/FederationMatrix.tsee/packages/federation-matrix/src/events/member.tsapps/meteor/app/lib/server/methods/addUsersToRoom.ts
📚 Learning: 2025-10-01T12:20:13.147Z
Learnt from: sampaiodiego
Repo: RocketChat/Rocket.Chat PR: 37110
File: apps/meteor/ee/server/startup/federation.ts:17-21
Timestamp: 2025-10-01T12:20:13.147Z
Learning: In Rocket.Chat's StreamerCentral broadcast for 'notify-room' events with 'user-activity', the `activity` argument is a string (e.g., 'user-typing') when a user starts typing and an empty array when the user stops typing.
Applied to files:
ee/packages/federation-matrix/src/FederationMatrix.ts
📚 Learning: 2025-11-10T19:06:20.146Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37408
File: apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.tsx:53-69
Timestamp: 2025-11-10T19:06:20.146Z
Learning: In the Rocket.Chat repository, do not provide suggestions or recommendations about code sections marked with TODO comments. The maintainers have already identified these as future work and external reviewers lack the full context about implementation plans and timing.
Applied to files:
ee/packages/federation-matrix/src/events/member.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:
ee/packages/federation-matrix/src/events/member.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/lib/publishFields.tsapps/meteor/server/modules/watchers/watchers.module.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). (1)
- GitHub Check: CodeQL-Build
🔇 Additional comments (21)
apps/meteor/lib/publishFields.ts (1)
45-46: LGTM!The addition of
statusandinviterUsernamefields to the subscription broadcast projection is correct and aligns with the new invite flow support.packages/models/src/models/Subscriptions.ts (2)
164-164: LGTM!Filtering out subscriptions with a
statusfield ensures that invited users are not counted as active room members, which is the correct behavior for the invite flow.
2090-2104: LGTM!The
markInviteAsAcceptedmethod correctly transitions an invited subscription to an accepted state by unsetting invite-specific fields and opening the room for the user.packages/core-typings/src/IInquiry.ts (1)
64-69: LGTM!The enhanced type guard correctly distinguishes livechat inquiry records from regular subscriptions by validating that the
statusfield contains aLivechatInquiryStatusvalue, preventing false positives from subscriptions withSubscriptionStatusvalues.apps/meteor/server/modules/watchers/watchers.module.ts (1)
140-141: LGTM!Adding
statusandinviterUsernameto the subscription projection ensures these invite-related fields are properly broadcast to clients when subscriptions change.packages/core-services/src/types/IRoomService.ts (3)
1-1: LGTM!Import additions support the extended
addUserToRoomoptions and newacceptRoomInvitemethod signature.
42-43: LGTM!The optional
statusandinviterUsernamefields properly extend the options to support the federated invite flow while maintaining backward compatibility.
47-54: LGTM!The new
acceptRoomInvitemethod signature is well-defined and appropriate for handling the invite acceptance flow with optional system message suppression.apps/meteor/app/lib/server/functions/addUserToRoom.ts (3)
4-4: LGTM!Import addition supports the new
statusparameter for invite handling.
30-31: LGTM!The optional
statusandinviterUsernameparameters are properly typed and integrated into the function signature.Also applies to: 36-37
106-107: LGTM!The conditional spreading ensures
statusandinviterUsernameare only included in the subscription when defined, maintaining data integrity.apps/meteor/app/lib/server/methods/addUsersToRoom.ts (2)
1-2: LGTM!Import additions support the new federated room invite integration.
123-133: LGTM!The early return when
newUser.usernameis undefined prevents issues downstream, and the ephemeral message correctly uses the validated username.ee/packages/federation-matrix/src/events/member.ts (5)
11-24: LGTM!The inviter and invitee user resolution with early returns for missing users is a robust approach to error handling.
28-49: LGTM!The room type determination and naming logic correctly handles direct messages vs. group rooms, with the added check for
roomOriginDomainaddressing the previous review concern.
72-95: LGTM!The join handler correctly resolves the joining user, validates the room and subscription exist, and delegates to the room service for invite acceptance.
97-113: LGTM!The leave handler properly resolves the leaving user and room, with appropriate error logging for missing entities.
115-138: LGTM!The switch-based dispatcher provides clean routing to the appropriate membership handler with good error handling and logging for unknown membership types.
ee/packages/federation-matrix/src/FederationMatrix.ts (3)
10-20: LGTM!Type imports properly added to support the new invite handling functionality.
550-589: Verify the subscription status update after invite acceptance/rejection.The
inviteUsersToRoommethod now properly distinguishes between native federated and local inviters. However, ensure that when invites are sent, the corresponding subscription records are created or updated with the correct status (e.g., 'INVITED') and that acceptance/rejection flows properly update these records.
916-952: Verify subscription status updates after invite handling.The
handleInvitemethod correctly validates the subscription is in 'INVITED' status before processing, but ensure that accepting or rejecting the invite properly updates the subscription status (e.g., to 'JOINED' or removes the subscription). This might be handled by Matrix event handlers, but should be confirmed.
…ws to avoid propagation loops
…nse when user is INVITED
sampaiodiego
force-pushed
the
feat/invites
branch
from
fddb5b3 to
cb7d33a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
apps/meteor/ee/server/hooks/federation/index.ts (1)
96-142: Fix federation permission check to validate the inviter, not the invited userIn the
beforeAddUserToRoomhook, the federation permission is currently checked against the invited user:if (!(await Authorization.hasPermission(user._id, 'access-federation'))) { throw new MeteorError('error-not-authorized-federation', 'Not authorized to access federation'); }Per the federation permission pattern (user‑initiated actions must be checked on the actor, while remote events skip), this should be validating the inviter. As written, a user without
access-federationcan invite someone who has it, and external/placeholder users will incorrectly fail the check.A minimal fix:
- // TODO should we really check for "user" here? it is potentially an external user - if (!(await Authorization.hasPermission(user._id, 'access-federation'))) { + const inviterId = inviter._id; + if (!inviterId || !(await Authorization.hasPermission(inviterId, 'access-federation'))) { throw new MeteorError('error-not-authorized-federation', 'Not authorized to access federation'); }This restores the intended contract: only local users with
access-federationcan initiate federated invites, regardless of who is being invited. Remote‑origin invites still bypass this hook via the Matrix event handlers, matching the documented pattern. Based on learnings, this should be enforced here.ee/packages/federation-matrix/src/events/member.ts (1)
121-172: HardenhandleInviteagainst missing invite state and optional usernamesThere are a few robustness issues in the
handleInviteflow that are worth tightening up:
unsigned.invite_room_stateis accessed without guardingunsigned, which can throw if it is absent.roomOriginDomainis derived fromsenderIdinstead ofroomId, which will mislabel rooms in third‑server relay scenarios.roomFNameis built as${matrixRoomName}:${roomOriginDomain}even whenmatrixRoomNameis undefined, leading to values likeundefined:server.inviterUser.usernameandinviteeUser.usernameare optional inIUser, but are used as if always present (including being cast tostring), which can propagateundefinedinto room creation and federation metadata.You can address all of these with a small set of changes:
async function handleInvite({ sender: senderId, state_key: userId, room_id: roomId, content, unsigned, }: HomeserverEventSignatures['homeserver.matrix.membership']['event']): Promise<void> { const inviterUser = await getOrCreateFederatedUser(senderId); if (!inviterUser) { throw new Error(`Failed to get or create inviter user: ${senderId}`); } const inviteeUser = await getOrCreateFederatedUser(userId); if (!inviteeUser) { throw new Error(`Failed to get or create invitee user: ${userId}`); } - const strippedState = unsigned.invite_room_state; + if (!inviterUser.username) { + logger.error(`Inviter user ${inviterUser._id} has no username for room ${roomId}`); + return; + } + + if (content?.is_direct && !inviteeUser.username) { + logger.error(`Invitee user ${inviteeUser._id} has no username for direct room ${roomId}`); + return; + } + + const strippedState = unsigned?.invite_room_state ?? []; @@ - const roomOriginDomain = senderId.split(':')?.pop(); + const roomOriginDomain = roomId.split(':')?.pop(); if (!roomOriginDomain) { throw new Error(`Room origin domain not found: ${roomId}`); } @@ let roomName: string; let roomFName: string; if (content?.is_direct) { roomName = senderId; roomFName = senderId; } else { - roomName = roomId.replace('!', '').replace(':', '_'); - roomFName = `${matrixRoomName}:${roomOriginDomain}`; + roomName = roomId.replace('!', '').replace(':', '_'); + roomFName = matrixRoomName ? `${matrixRoomName}:${roomOriginDomain}` : roomId; } @@ const room = await getOrCreateFederatedRoom({ matrixRoomId: roomId, roomName, roomFName, roomType, inviterUserId: inviterUser._id, - inviterUsername: inviterUser.username as string, // TODO: Remove force cast - inviteeUsername: content?.is_direct ? inviteeUser.username : undefined, + inviterUsername: inviterUser.username, + inviteeUsername: content?.is_direct ? inviteeUser.username : undefined, });This keeps the happy path unchanged, but avoids crashes on malformed events, correctly uses the room’s origin domain, generates valid
fnames when nom.room.nameexists, and removes unsafe assumptions around optional usernames.apps/meteor/app/lib/server/functions/removeUserFromRoom.ts (1)
13-17: Documentation vs implementation mismatch for callbacks.The JSDoc states "no callbacks" but
beforeLeaveRoomCallback.runis called on line 27.apps/meteor/app/api/server/v1/rooms.ts (1)
1100-1105: Error handling doesn't utilize the defined response schemas.The endpoint declares
400and401response validators but the catch block returns a genericAPI.v1.failure()that doesn't match these schemas.ee/packages/federation-matrix/src/FederationMatrix.ts (1)
535-567: Useinviter.federation.muifor native-federated inviters to avoid malformed Matrix IDs.Line 537 always builds
inviterUserIdas@${inviter.username}:${this.serverName}, but for native-federated users,usernameis already a Matrix ID (e.g.,@alice:remote.tld), producing invalid IDs like@@alice:remote.tld:your-server.
🧹 Nitpick comments (8)
ee/packages/federation-matrix/src/api/_matrix/send-leave.ts (1)
13-46: Body schema missing nested required properties.The
contentobject schema definesmembershipbut doesn't mark it as required within the nested object. This could allow events withoutcontent.membershipto pass validation.Consider adding
requiredarrays to nested objects:const isSendLeaveBodyProps = ajv.compile({ type: 'object', properties: { content: { type: 'object', properties: { membership: { type: 'string', const: 'leave', }, }, + required: ['membership'], }, // ... rest of properties }, required: ['content', 'depth', 'origin', 'origin_server_ts', 'sender', 'state_key', 'type'], });ee/packages/federation-matrix/src/api/_matrix/make-leave.ts (2)
76-78: Track the TODO for spec compliance.The TODO indicates the SDK returns out-of-spec attributes. Consider tracking this in an issue to ensure cleanup before the feature is considered complete.
Would you like me to open an issue to track this TODO for removing out-of-spec attributes from the make_leave response?
13-48: Success response schema missing nested required properties.Similar to
send-leave.ts, the success response schema defines properties for theeventobject but doesn't includerequiredarrays for nested objects. This may allow responses with missing fields to pass validation.Consider adding
requiredconstraints to nested objects for stricter validation:const isMakeLeaveSuccessResponseProps = ajv.compile({ type: 'object', properties: { event: { type: 'object', properties: { content: { type: 'object', properties: { membership: { type: 'string', const: 'leave', }, }, + required: ['membership'], }, // ... other properties }, + required: ['content', 'origin', 'origin_server_ts', 'sender', 'state_key', 'type'], }, room_version: { type: 'string' }, }, + required: ['event', 'room_version'], });ee/packages/federation-matrix/src/api/_matrix/invite.ts (1)
167-175: Minor: Unused variable and unclear error message.The destructured
domainvariable is unused (only a TODO comment mentions it), and the error message on line 174 has unclear grammar.- const [username /* domain */] = userToCheck.split(':'); + const [username] = userToCheck.split(':'); + // TODO: validate domain matches expected server - // TODO: check domain - const ourUser = await Users.findOneByUsername(username.slice(1)); if (!ourUser) { - throw new Error('user not found not processing invite'); + throw new Error('User not found, cannot process invite'); }apps/meteor/app/lib/server/functions/createRoom.ts (1)
62-81: Synchronous invite loop is appropriate for federation but consider error resilience.The synchronous approach is acceptable for small member lists as noted in the comment. However, if
ensureFederatedUsersExistLocallysucceeds but a user lookup fails, the room creation will fail mid-way, potentially leaving the room in an inconsistent state with some invites sent.Consider whether partial failure should be handled gracefully, or if the current fail-fast behavior is intentional:
for await (const memberUsername of membersToInvite) { const member = await Users.findOneByUsername(memberUsername); if (!member) { - throw new Error('Federated user not found locally'); + throw new Error(`Federated user ${memberUsername} not found locally after sync`); }ee/packages/federation-matrix/tests/end-to-end/room.spec.ts (1)
1537-1576: Consider adding tests for leave/rejection scenarios.Per reviewer comment, the
make_leaveandsend_leaveendpoints are missing from this PR. When those are added, corresponding test coverage for rejection flows from the remote server perspective would strengthen the test suite.Would you like me to help outline test cases for the leave/rejection flows once the endpoints are implemented?
apps/meteor/app/lib/server/functions/removeUserFromRoom.ts (1)
29-47: Remove redundant conditional check.The
if (subscription)check on line 29 is always true because the function returns early on lines 22-24 whensubscriptionis falsy.- if (subscription) { - const removedUser = user; - if (options?.byUser) { + const removedUser = user; + if (options?.byUser) { const extraData = { u: options.byUser, }; if (room.teamMain) { await Message.saveSystemMessage('removed-user-from-team', room._id, user.username || '', user, extraData); } else { await Message.saveSystemMessage('ru', room._id, user.username || '', user, extraData); } - } else if (subscription.status === 'INVITED') { - await Message.saveSystemMessage('uir', room._id, removedUser.username || '', removedUser); - } else if (room.teamMain) { - await Message.saveSystemMessage('ult', room._id, removedUser.username || '', removedUser); - } else { - await Message.saveSystemMessage('ul', room._id, removedUser.username || '', removedUser); - } + } else if (subscription.status === 'INVITED') { + await Message.saveSystemMessage('uir', room._id, removedUser.username || '', removedUser); + } else if (room.teamMain) { + await Message.saveSystemMessage('ult', room._id, removedUser.username || '', removedUser); + } else { + await Message.saveSystemMessage('ul', room._id, removedUser.username || '', removedUser); }packages/core-services/src/types/IRoomService.ts (1)
56-65: Consider aligning inviter type with existing patterns.The
inviterparameter includesname(line 60), butaddUserToRoom(line 31) usesPick<IUser, '_id' | 'username'>withoutname. This inconsistency could cause confusion. Consider whether both should use the same type, or document why they differ.
📜 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 ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (39)
apps/meteor/app/api/server/v1/rooms.ts(3 hunks)apps/meteor/app/lib/lib/MessageTypes.ts(1 hunks)apps/meteor/app/lib/server/functions/acceptRoomInvite.ts(1 hunks)apps/meteor/app/lib/server/functions/addUserToRoom.ts(4 hunks)apps/meteor/app/lib/server/functions/createDirectRoom.ts(2 hunks)apps/meteor/app/lib/server/functions/createRoom.ts(6 hunks)apps/meteor/app/lib/server/functions/removeUserFromRoom.ts(4 hunks)apps/meteor/app/lib/server/methods/addUsersToRoom.ts(1 hunks)apps/meteor/ee/server/hooks/federation/index.ts(5 hunks)apps/meteor/lib/publishFields.ts(1 hunks)apps/meteor/package.json(1 hunks)apps/meteor/server/lib/dataExport/exportRoomMessagesToFile.ts(1 hunks)apps/meteor/server/lib/findUsersOfRoomOrderedByRole.ts(6 hunks)apps/meteor/server/methods/createDirectMessage.ts(1 hunks)apps/meteor/server/modules/watchers/watchers.module.ts(1 hunks)apps/meteor/server/services/room/service.ts(3 hunks)apps/meteor/tests/data/rooms.helper.ts(1 hunks)ee/packages/federation-matrix/package.json(1 hunks)ee/packages/federation-matrix/src/FederationMatrix.ts(9 hunks)ee/packages/federation-matrix/src/api/_matrix/invite.ts(3 hunks)ee/packages/federation-matrix/src/api/_matrix/make-leave.ts(1 hunks)ee/packages/federation-matrix/src/api/_matrix/send-leave.ts(1 hunks)ee/packages/federation-matrix/src/api/routes.ts(2 hunks)ee/packages/federation-matrix/src/events/member.ts(1 hunks)ee/packages/federation-matrix/tests/end-to-end/room.spec.ts(13 hunks)packages/apps-engine/src/definition/messages/MessageType.ts(1 hunks)packages/core-services/package.json(1 hunks)packages/core-services/src/types/IFederationMatrixService.ts(2 hunks)packages/core-services/src/types/IRoomService.ts(4 hunks)packages/core-typings/src/IMessage/IMessage.ts(1 hunks)packages/core-typings/src/IRoom.ts(1 hunks)packages/core-typings/src/ISubscription.ts(2 hunks)packages/i18n/src/locales/en.i18n.json(2 hunks)packages/message-types/src/registrations/common.ts(1 hunks)packages/model-typings/src/models/IRoomsModel.ts(1 hunks)packages/model-typings/src/models/ISubscriptionsModel.ts(1 hunks)packages/models/src/models/Rooms.ts(1 hunks)packages/models/src/models/Subscriptions.ts(2 hunks)packages/rest-typings/src/v1/rooms.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (15)
- apps/meteor/server/lib/dataExport/exportRoomMessagesToFile.ts
- packages/apps-engine/src/definition/messages/MessageType.ts
- apps/meteor/lib/publishFields.ts
- apps/meteor/app/lib/server/functions/acceptRoomInvite.ts
- apps/meteor/server/lib/findUsersOfRoomOrderedByRole.ts
- packages/core-typings/src/IRoom.ts
- packages/models/src/models/Rooms.ts
- apps/meteor/server/modules/watchers/watchers.module.ts
- packages/i18n/src/locales/en.i18n.json
- apps/meteor/app/lib/lib/MessageTypes.ts
- packages/message-types/src/registrations/common.ts
- apps/meteor/server/methods/createDirectMessage.ts
- packages/models/src/models/Subscriptions.ts
- packages/core-services/src/types/IFederationMatrixService.ts
- packages/core-typings/src/IMessage/IMessage.ts
🧰 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/model-typings/src/models/ISubscriptionsModel.tsee/packages/federation-matrix/src/api/_matrix/send-leave.tsee/packages/federation-matrix/src/api/_matrix/invite.tsee/packages/federation-matrix/src/api/_matrix/make-leave.tsapps/meteor/tests/data/rooms.helper.tsapps/meteor/app/lib/server/functions/addUserToRoom.tsapps/meteor/app/lib/server/functions/createDirectRoom.tsee/packages/federation-matrix/src/events/member.tsapps/meteor/ee/server/hooks/federation/index.tsapps/meteor/app/lib/server/functions/removeUserFromRoom.tsapps/meteor/app/lib/server/methods/addUsersToRoom.tsapps/meteor/app/lib/server/functions/createRoom.tsapps/meteor/app/api/server/v1/rooms.tsee/packages/federation-matrix/src/FederationMatrix.tsapps/meteor/server/services/room/service.tspackages/rest-typings/src/v1/rooms.tspackages/core-services/src/types/IRoomService.tsee/packages/federation-matrix/src/api/routes.tsee/packages/federation-matrix/tests/end-to-end/room.spec.tspackages/model-typings/src/models/IRoomsModel.tspackages/core-typings/src/ISubscription.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:
ee/packages/federation-matrix/tests/end-to-end/room.spec.ts
🧠 Learnings (20)
📓 Common learnings
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: 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.
📚 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:
packages/core-services/package.jsonee/packages/federation-matrix/src/api/_matrix/invite.tsapps/meteor/package.jsonapps/meteor/app/lib/server/functions/addUserToRoom.tsapps/meteor/app/lib/server/functions/createDirectRoom.tsee/packages/federation-matrix/src/events/member.tsapps/meteor/ee/server/hooks/federation/index.tsapps/meteor/app/lib/server/functions/removeUserFromRoom.tsapps/meteor/app/lib/server/methods/addUsersToRoom.tsapps/meteor/app/lib/server/functions/createRoom.tsapps/meteor/app/api/server/v1/rooms.tsee/packages/federation-matrix/src/FederationMatrix.tsee/packages/federation-matrix/package.json
📚 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/package.jsonee/packages/federation-matrix/src/api/_matrix/send-leave.tsee/packages/federation-matrix/src/api/_matrix/invite.tsee/packages/federation-matrix/src/api/_matrix/make-leave.tsapps/meteor/tests/data/rooms.helper.tsapps/meteor/package.jsonapps/meteor/app/lib/server/functions/addUserToRoom.tsapps/meteor/app/lib/server/functions/createDirectRoom.tsee/packages/federation-matrix/src/events/member.tsapps/meteor/ee/server/hooks/federation/index.tsapps/meteor/app/lib/server/functions/removeUserFromRoom.tsapps/meteor/app/lib/server/methods/addUsersToRoom.tsapps/meteor/app/lib/server/functions/createRoom.tsapps/meteor/app/api/server/v1/rooms.tsee/packages/federation-matrix/src/FederationMatrix.tsapps/meteor/server/services/room/service.tspackages/rest-typings/src/v1/rooms.tspackages/core-services/src/types/IRoomService.tsee/packages/federation-matrix/tests/end-to-end/room.spec.tsee/packages/federation-matrix/package.jsonpackages/model-typings/src/models/IRoomsModel.tspackages/core-typings/src/ISubscription.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:
packages/model-typings/src/models/ISubscriptionsModel.tsapps/meteor/server/services/room/service.tspackages/core-typings/src/ISubscription.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:
packages/model-typings/src/models/ISubscriptionsModel.tsapps/meteor/app/lib/server/functions/addUserToRoom.tsapps/meteor/app/lib/server/functions/createDirectRoom.tsee/packages/federation-matrix/src/events/member.tsapps/meteor/ee/server/hooks/federation/index.tsapps/meteor/app/lib/server/functions/removeUserFromRoom.tsapps/meteor/app/lib/server/methods/addUsersToRoom.tsapps/meteor/app/lib/server/functions/createRoom.tsee/packages/federation-matrix/src/FederationMatrix.tsapps/meteor/server/services/room/service.tspackages/rest-typings/src/v1/rooms.tspackages/core-services/src/types/IRoomService.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:
packages/model-typings/src/models/ISubscriptionsModel.tsapps/meteor/app/lib/server/functions/addUserToRoom.tsapps/meteor/app/lib/server/functions/createDirectRoom.tsee/packages/federation-matrix/src/events/member.tsapps/meteor/ee/server/hooks/federation/index.tsapps/meteor/app/lib/server/functions/removeUserFromRoom.tsapps/meteor/app/lib/server/methods/addUsersToRoom.tsapps/meteor/app/lib/server/functions/createRoom.tsee/packages/federation-matrix/src/FederationMatrix.tsapps/meteor/server/services/room/service.tspackages/rest-typings/src/v1/rooms.tspackages/core-services/src/types/IRoomService.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:
ee/packages/federation-matrix/src/api/_matrix/send-leave.tsee/packages/federation-matrix/src/api/_matrix/invite.tsee/packages/federation-matrix/src/api/_matrix/make-leave.tsapps/meteor/package.jsonapps/meteor/app/lib/server/functions/addUserToRoom.tsapps/meteor/app/lib/server/functions/createDirectRoom.tsee/packages/federation-matrix/src/events/member.tsapps/meteor/ee/server/hooks/federation/index.tsapps/meteor/app/lib/server/functions/removeUserFromRoom.tsapps/meteor/app/lib/server/methods/addUsersToRoom.tsapps/meteor/app/lib/server/functions/createRoom.tsapps/meteor/app/api/server/v1/rooms.tsee/packages/federation-matrix/src/FederationMatrix.tsapps/meteor/server/services/room/service.tsee/packages/federation-matrix/tests/end-to-end/room.spec.tspackages/core-typings/src/ISubscription.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/src/api/_matrix/invite.tsee/packages/federation-matrix/src/events/member.tsapps/meteor/ee/server/hooks/federation/index.tsapps/meteor/app/lib/server/methods/addUsersToRoom.tsapps/meteor/app/lib/server/functions/createRoom.tsapps/meteor/app/api/server/v1/rooms.tsee/packages/federation-matrix/src/FederationMatrix.tsee/packages/federation-matrix/src/api/routes.tsee/packages/federation-matrix/package.json
📚 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/addUserToRoom.tsee/packages/federation-matrix/src/events/member.tsapps/meteor/ee/server/hooks/federation/index.tsapps/meteor/app/lib/server/functions/removeUserFromRoom.tsapps/meteor/app/lib/server/methods/addUsersToRoom.tsapps/meteor/app/api/server/v1/rooms.tsapps/meteor/server/services/room/service.ts
📚 Learning: 2025-11-10T19:06:20.146Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37408
File: apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.tsx:53-69
Timestamp: 2025-11-10T19:06:20.146Z
Learning: In the Rocket.Chat repository, do not provide suggestions or recommendations about code sections marked with TODO comments. The maintainers have already identified these as future work and external reviewers lack the full context about implementation plans and timing.
Applied to files:
ee/packages/federation-matrix/src/events/member.tspackages/core-typings/src/ISubscription.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:
ee/packages/federation-matrix/src/events/member.tsapps/meteor/ee/server/hooks/federation/index.tsapps/meteor/app/api/server/v1/rooms.tsapps/meteor/server/services/room/service.tspackages/rest-typings/src/v1/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:
ee/packages/federation-matrix/src/events/member.tsapps/meteor/ee/server/hooks/federation/index.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:
ee/packages/federation-matrix/src/events/member.tsapps/meteor/app/lib/server/functions/removeUserFromRoom.tsapps/meteor/app/lib/server/functions/createRoom.ts
📚 Learning: 2025-09-30T13:00:05.465Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 36990
File: apps/meteor/ee/server/apps/storage/AppRealStorage.ts:55-58
Timestamp: 2025-09-30T13:00:05.465Z
Learning: In AppRealStorage (apps/meteor/ee/server/apps/storage/AppRealStorage.ts), the `remove` method is designed to be idempotent and returns `{ success: true }` unconditionally because the goal is to ensure the app is removed, not to distinguish whether this specific call performed the deletion. Database errors will throw exceptions.
Applied to files:
apps/meteor/app/lib/server/functions/removeUserFromRoom.ts
📚 Learning: 2025-11-19T18:20:07.720Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 37419
File: packages/i18n/src/locales/en.i18n.json:918-921
Timestamp: 2025-11-19T18:20:07.720Z
Learning: Repo: RocketChat/Rocket.Chat — i18n/formatting
Learning: This repository uses a custom message formatting parser in UI blocks/messages; do not assume standard Markdown rules. For keys like Call_ended_bold, Call_not_answered_bold, Call_failed_bold, and Call_transferred_bold in packages/i18n/src/locales/en.i18n.json, retain the existing single-asterisk emphasis unless maintainers request otherwise.
Applied to files:
ee/packages/federation-matrix/src/FederationMatrix.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:
ee/packages/federation-matrix/tests/end-to-end/room.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 : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
ee/packages/federation-matrix/tests/end-to-end/room.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 : Use `expect` matchers for assertions (`toEqual`, `toContain`, `toBeTruthy`, `toHaveLength`, etc.) instead of `assert` statements in Playwright tests
Applied to files:
ee/packages/federation-matrix/tests/end-to-end/room.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/**/*.{ts,spec.ts} : Store commonly used locators in variables/constants for reuse
Applied to files:
ee/packages/federation-matrix/tests/end-to-end/room.spec.ts
📚 Learning: 2025-11-19T12:32:29.696Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37547
File: packages/i18n/src/locales/en.i18n.json:634-634
Timestamp: 2025-11-19T12:32:29.696Z
Learning: Repo: RocketChat/Rocket.Chat
Context: i18n workflow
Learning: In this repository, new translation keys should be added to packages/i18n/src/locales/en.i18n.json only; other locale files are populated via the external translation pipeline and/or fall back to English. Do not request adding the same key to all locale files in future reviews.
Applied to files:
packages/core-typings/src/ISubscription.ts
🧬 Code graph analysis (13)
packages/model-typings/src/models/ISubscriptionsModel.ts (1)
packages/core-typings/src/ISubscription.ts (1)
ISubscription(11-79)
ee/packages/federation-matrix/src/api/_matrix/make-leave.ts (2)
packages/rest-typings/src/v1/Ajv.ts (1)
ajv(24-24)ee/packages/federation-matrix/src/api/middlewares/isAuthenticated.ts (1)
isAuthenticatedMiddleware(5-39)
apps/meteor/app/lib/server/functions/createDirectRoom.ts (2)
packages/core-typings/src/IUser.ts (1)
IUser(186-256)packages/core-typings/src/ISubscription.ts (1)
ISubscription(11-79)
ee/packages/federation-matrix/src/events/member.ts (2)
ee/packages/federation-matrix/src/FederationMatrix.ts (2)
getUsernameServername(72-84)createOrUpdateFederatedUser(91-132)packages/core-typings/src/IRoom.ts (2)
IRoom(21-95)IRoomNativeFederated(114-121)
apps/meteor/ee/server/hooks/federation/index.ts (2)
packages/core-typings/src/IRoom.ts (2)
isRoomNativeFederated(125-126)IRoom(21-95)packages/core-typings/src/IUser.ts (2)
isUserNativeFederated(277-278)IUser(186-256)
apps/meteor/app/lib/server/methods/addUsersToRoom.ts (2)
apps/meteor/app/lib/server/functions/addUserToRoom.ts (1)
addUserToRoom(20-130)apps/meteor/server/services/room/service.ts (1)
addUserToRoom(73-83)
apps/meteor/app/api/server/v1/rooms.ts (4)
apps/meteor/app/api/server/index.ts (1)
API(55-55)packages/rest-typings/src/v1/rooms.ts (1)
isRoomsInviteProps(712-712)packages/rest-typings/src/v1/Ajv.ts (3)
validateBadRequestErrorResponse(47-47)validateUnauthorizedErrorResponse(70-70)ajv(24-24)apps/meteor/app/api/server/ApiClass.ts (1)
ExtractRoutesFromAPI(73-77)
packages/rest-typings/src/v1/rooms.ts (2)
packages/core-typings/src/IUser.ts (1)
IUser(186-256)packages/core-typings/src/ISubscription.ts (1)
ISubscription(11-79)
packages/core-services/src/types/IRoomService.ts (7)
packages/core-services/src/index.ts (3)
ICreateRoomParams(99-99)ICreateRoomOptions(86-86)IRoomService(115-115)packages/core-typings/src/IRoom.ts (1)
IRoom(21-95)apps/meteor/app/livechat/server/methods/getFirstRoomMessage.ts (1)
uid(17-35)apps/meteor/client/startup/iframeCommands.ts (1)
user(80-89)packages/core-typings/src/IUser.ts (1)
IUser(186-256)apps/meteor/app/livechat/server/methods/getAnalyticsChartData.ts (1)
options(18-38)packages/core-typings/src/ISubscription.ts (1)
ISubscription(11-79)
ee/packages/federation-matrix/src/api/routes.ts (3)
ee/packages/federation-matrix/src/api/_matrix/media.ts (1)
getMatrixMediaRoutes(76-149)ee/packages/federation-matrix/src/api/_matrix/send-leave.ts (1)
getMatrixSendLeaveRoutes(60-109)ee/packages/federation-matrix/src/api/_matrix/make-leave.ts (1)
getMatrixMakeLeaveRoutes(58-106)
ee/packages/federation-matrix/tests/end-to-end/room.spec.ts (4)
apps/meteor/tests/data/rooms.helper.ts (4)
getGroupHistory(343-358)acceptRoomInvite(439-455)createRoom(21-72)rejectRoomInvite(468-484)packages/core-typings/src/IMessage/IMessage.ts (1)
IMessage(164-262)apps/meteor/app/lib/server/functions/createRoom.ts (1)
createRoom(145-327)ee/packages/federation-matrix/src/FederationMatrix.ts (1)
createRoom(209-234)
packages/model-typings/src/models/IRoomsModel.ts (1)
packages/core-typings/src/IRoom.ts (1)
IRoomFederated(109-112)
packages/core-typings/src/ISubscription.ts (1)
packages/core-typings/src/IUser.ts (1)
IUser(186-256)
⏰ 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 (34)
packages/model-typings/src/models/IRoomsModel.ts (1)
168-169: LGTM!The new method signature is well-typed and follows existing naming conventions. Using
FindOptions<IRoomFederated>provides stronger type safety for federated room queries, and the placement alongside other federated room methods maintains good interface organization.apps/meteor/package.json (1)
100-102: Federation SDK version bump is consistent across workspaceThe
@rocket.chat/federation-sdkupgrade to0.3.3here matches the other packages in this PR and is expected for the new invite/leave APIs. Please just ensure the lockfile and federation bundle are regenerated with this version.ee/packages/federation-matrix/package.json (1)
22-26: Aligning federation-matrix with SDK 0.3.3Updating
@rocket.chat/federation-sdkto0.3.3here keeps the Matrix integration in sync with apps/meteor and core-services and is required for the new membership and leave endpoints. Looks good; just confirm tests against this SDK version pass (especially invite/join/leave flows).packages/core-services/package.json (1)
18-22: Core-services now on federation-sdk 0.3.3The bump of
@rocket.chat/federation-sdkto0.3.3here is consistent with the rest of the workspace and needed for the updated federation room/subscribe APIs. No issues from a typing/structure standpoint; please just verify any core-services federation tests (room invite/accept/reject) against this version.apps/meteor/app/lib/server/functions/createDirectRoom.ts (1)
47-49: Federated DM invite metadata and creator validation look consistentTyping
options.creatorasIUser['_id'], enforcing that the creator is part of the room for federated DMs, and injectingstatus: 'INVITED'plusinviterinto the non-creator subscriptions align well with the newISubscriptionshape and the invite semantics described in the PR. Non-federated flows remain unchanged as theroomExtraData.federatedguard keeps this path isolated.Also applies to: 157-188
apps/meteor/ee/server/hooks/federation/index.ts (1)
255-269: Federated DM detection and Matrix DM creation flow look coherentThe new
federation.beforeCreateDirectMessagecallback that infers DM federation by Matrix‑style usernames, combined with theafterCreateDirectRoomhook that creates the Matrix DM (and backfillsroom.federation.mridwhen missing), provides a clear split between marking DMs as federated and actually provisioning them on Matrix. The guards onFederationActions.shouldPerformFederationAction(room)ensure non‑federated DMs are unaffected.Also applies to: 271-283
ee/packages/federation-matrix/src/api/routes.ts (1)
6-7: Matrix make_leave/send_leave routes correctly integratedAdding
getMatrixMakeLeaveRoutes()andgetMatrixSendLeaveRoutes()to the federation router, after the existing Matrix routes, closes the gap around leave handling and aligns the HTTP surface with the SDK’s leave APIs. Paths are distinct, so the chaining order here is fine.Also applies to: 11-12, 32-35
packages/core-typings/src/ISubscription.ts (1)
10-10: New subscription status/inviter fields match invite flow usageDefining
SubscriptionStatus = 'INVITED'and extendingISubscriptionwith optionalstatusandinvitercleanly model the new invite state used byRoom.createUserSubscription,createDirectRoom, and the federation hooks. The narrowed union is a good starting point and can be extended if more statuses are added later.Also applies to: 77-79
apps/meteor/tests/data/rooms.helper.ts (1)
439-455: LGTM! Test helpers for federation invite accept/reject.The new
acceptRoomInviteandrejectRoomInvitehelpers follow the established patterns in this file. The implementation is consistent with other helpers likeloadHistoryandgetRoomInfo.One minor note: the
_errparameter is ignored in the.end()callback, which is consistent with other helpers in this file, but consider whether test failures should surface network errors for better debugging.Also applies to: 468-484
packages/model-typings/src/models/ISubscriptionsModel.ts (1)
339-340: LGTM! New subscription model methods for invite handling.The new
findInvitedSubscriptionandacceptInvitationByIdmethod signatures are well-typed and align with the federation invite workflow. The return types appropriately useISubscription | nullfor queries andUpdateResultfor mutations.ee/packages/federation-matrix/src/api/_matrix/send-leave.ts (1)
60-108: LGTM! Matrix federation send_leave endpoint implementation.The route handler correctly implements the Matrix federation send_leave protocol:
- Proper authentication via
isAuthenticatedMiddleware- AJV validation for request params and body
- Appropriate error handling with Matrix-standard error codes (M_FORBIDDEN, M_UNKNOWN)
- Logging for server errors
The implementation is consistent with the parallel
make-leave.tsroute.apps/meteor/app/lib/server/methods/addUsersToRoom.ts (1)
99-113: Simplified user addition by delegating toaddUserToRoom.The refactored code now delegates to
addUserToRoomfor the actual user addition logic, which handles federation checks internally (per the relevant code snippet showingisRoomNativeFederatedcheck at line 82 ofaddUserToRoom.ts). This aligns with the past review feedback to keep federation code centralized.The control flow is correct:
- If user is not in room → delegate to
addUserToRoom- If user is already in room → broadcast ephemeral notification
ee/packages/federation-matrix/src/api/_matrix/make-leave.ts (1)
58-105: LGTM! Matrix federation make_leave endpoint implementation.The route handler correctly implements the Matrix federation make_leave protocol:
- GET method for fetching leave event template
- Proper authentication and AJV validation
- Consistent error handling pattern with
send-leave.tsee/packages/federation-matrix/src/api/_matrix/invite.ts (1)
177-188: LGTM! Proper permission check for remote federation invites.The authorization check correctly verifies
access-federationpermission before processing the invite, returning a clear 403 response with appropriate error code when denied. This aligns with the federation permission pattern documented in learnings.apps/meteor/app/lib/server/functions/createRoom.ts (1)
299-304: Callback ordering change is intentional and well-documented.The federation callback is now invoked before subscription creation to ensure the Matrix room exists before invites are sent. The comment appropriately notes the naming inconsistency and suggests future renaming.
apps/meteor/app/lib/server/functions/addUserToRoom.ts (2)
97-105: LGTM! Centralized subscription creation.The switch to
Room.createUserSubscriptionproperly delegates subscription creation to the service layer, passing all relevant options includingskipSystemMessageandskipAlertSound.
82-86: No action required. The early return for federated rooms is correct and does not skip required post-join processing. Federation room subscriptions are handled through dedicated federation flows (FederationMatrix.handleInvitefor user-initiated accepts and remote event handlers) that properly manage callbacks and subscription creation.Likely an incorrect or invalid review comment.
packages/rest-typings/src/v1/rooms.ts (1)
691-712: LGTM! Well-defined invite action types.The
RoomsInvitePropstype and schema are properly defined with appropriate validation constraints. Theactionenum cleanly restricts to valid operations.ee/packages/federation-matrix/tests/end-to-end/room.spec.ts (2)
385-400: LGTM! Test correctly validates invite system message.The test properly verifies that federated users receive 'ui' (user invited) system messages instead of the previous 'au' (added user) messages, aligning with the new invitation-based flow.
1537-1576: LGTM! Good permission boundary testing.The new test suite properly validates that users cannot accept or reject invitations on behalf of others, verifying the expected error message. This is critical for security in federated environments.
apps/meteor/app/lib/server/functions/removeUserFromRoom.ts (1)
70-98: Well-structured separation of concerns.The refactoring cleanly separates the core removal logic (
performUserRemoval) from the full removal flow with callbacks and app events (removeUserFromRoom). This enables federation to use the minimal path while local actions use the complete flow.apps/meteor/app/api/server/v1/rooms.ts (3)
1-1: Import addition looks correct.The
FederationMatriximport is properly added alongside existing core-services imports.
18-20: Validator imports are appropriate.The added validators align with the response schema declarations in the new endpoint.
1079-1109: Endpoint structure and type augmentation look correct.The
rooms.inviteendpoint is properly configured with authentication, body validation, and response schemas. The type augmentation correctly extendsRoomEndpointsto include bothroomEndpointsandroomInviteEndpoints.ee/packages/federation-matrix/src/FederationMatrix.ts (3)
91-132: Function return type improvement.Returning the full
IUserobject instead of just the ID is more useful for callers.
209-234: Room creation flow refactored correctly.The comment on line 225 clarifies that member invites are now handled via the
beforeAddUserToRoomcallback rather than during room creation. This is consistent with the PR's invite flow design.
840-843: Good defensive check for room membership before sending typing notifications.This prevents typing notifications from being sent for users who haven't joined the room yet.
packages/core-services/src/types/IRoomService.ts (3)
1-1: Import additions are correct.Added
ISubscriptionimport to support the new method signatures.
15-25: Generic type parameter improves flexibility.Making
ICreateRoomParamsgeneric allows better type inference for room-specific extra data.
39-40: New federation-related methods are well-defined.The
performUserRemovalandperformAcceptRoomInvitemethods provide clean interfaces for federation flows that need direct access to room/subscription state.apps/meteor/server/services/room/service.ts (4)
1-18: Import updates are well-organized.All new imports are necessary for the added functionality:
Messagefor system messages,ISubscriptionfor type safety, and the new helper functions.
89-95: Wrapper methods correctly delegate to implementation functions.These thin wrappers expose the federation-specific removal and accept functions through the service interface.
222-289: Comprehensive subscription creation with proper system message handling.The
createUserSubscriptionmethod correctly:
- Applies auto-translate configuration
- Sets the INVITED status when provided
- Stores inviter information
- Notifies on subscription changes
- Emits appropriate system messages ('ui' for invited, 'au' for added, etc.)
The branching logic for system messages (lines 261-286) handles all the cases defined in the PR objectives.
273-275: No action required. The 'ui' system message type is properly registered inpackages/message-types/src/registrations/common.tsand is an established system message type used throughout the codebase for user invitation notifications.
| if (action === 'accept') { | ||
| await federationSDK.acceptInvite(room.federation.mrid, matrixUserId); | ||
|
|
||
| await Room.performAcceptRoomInvite(room, subscription, user); | ||
| } | ||
| if (action === 'reject') { | ||
| await federationSDK.rejectInvite(room.federation.mrid, matrixUserId); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's understand the context around lines 920-927
cd /tmp && find . -name "FederationMatrix.ts" -type f 2>/dev/null | head -5Repository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
# Search for performUserRemoval and related cleanup methods
rg "performUserRemoval|removeUserFromRoom|performRejectRoomInvite" --type=ts -B 2 -A 5 | head -100Repository: RocketChat/Rocket.Chat
Length of output: 7978
🏁 Script executed:
# Look at the broader context of the reject action and what happens before/after
rg "action === 'reject'" --type=ts -B 5 -A 10Repository: RocketChat/Rocket.Chat
Length of output: 888
🏁 Script executed:
# Check if subscription is cleaned up elsewhere for invite rejections
rg "rejectInvite|reject.*[Ii]nvite" --type=ts -B 3 -A 5 | head -120Repository: RocketChat/Rocket.Chat
Length of output: 6051
🏁 Script executed:
# Find performAcceptRoomInvite implementation to understand the pattern
rg "performAcceptRoomInvite" --type=ts -B 2 -A 15 | head -80Repository: RocketChat/Rocket.Chat
Length of output: 7615
🏁 Script executed:
# Check if there's a performRejectRoomInvite or similar
rg "performReject|rejectInvite.*Room" --type=ts -B 2 -A 10Repository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
# Check subscription schema to see if it tracks invitation state
rg "interface ISubscription|type ISubscription" --type=ts -A 30 | head -100Repository: RocketChat/Rocket.Chat
Length of output: 7948
🏁 Script executed:
# Find performAcceptRoomInvite implementation
rg -n "export.*performAcceptRoomInvite|function performAcceptRoomInvite" --type=ts -A 30 | head -100Repository: RocketChat/Rocket.Chat
Length of output: 2940
🏁 Script executed:
# Find acceptRoomInvite file and read it
fd "acceptRoomInvite" --type fRepository: RocketChat/Rocket.Chat
Length of output: 122
🏁 Script executed:
# Find findInvitedSubscription to understand how invites are tracked
rg -n "findInvitedSubscription" --type=ts -B 2 -A 15Repository: RocketChat/Rocket.Chat
Length of output: 3752
🏁 Script executed:
# Check if there's a rejectInvitationById method or similar
rg "rejectInvitation" --type=tsRepository: RocketChat/Rocket.Chat
Length of output: 48
🏁 Script executed:
# Check if subscriptions are deleted elsewhere for rejections or invitations
rg "remove.*[Ii]nvit|delete.*[Ii]nvit" --type=ts | head -30Repository: RocketChat/Rocket.Chat
Length of output: 2795
🏁 Script executed:
# Look at the full acceptRoomInvite file to understand the complete pattern
cat apps/meteor/app/lib/server/functions/acceptRoomInvite.tsRepository: RocketChat/Rocket.Chat
Length of output: 2303
Clean up subscription on reject to match accept behavior.
When action === 'reject', the code calls federationSDK.rejectInvite but doesn't remove the subscription. The accept path calls Room.performAcceptRoomInvite to update subscription state, but the reject path has no corresponding cleanup. This leaves the subscription in INVITED state indefinitely.
Add:
if (action === 'reject') {
await federationSDK.rejectInvite(room.federation.mrid, matrixUserId);
+ await Room.performUserRemoval(room, user);
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In ee/packages/federation-matrix/src/FederationMatrix.ts around lines 920-927,
the reject branch calls federationSDK.rejectInvite but does not update or remove
the subscription like the accept branch does; add a call to the
room/subscription cleanup routine (matching the accept path) — e.g. after
federationSDK.rejectInvite(...), call Room.performRejectRoomInvite(room,
subscription, user) (or the equivalent method that clears the INVITED
subscription) so the subscription state is cleaned up and the invite doesn't
remain indefinitely.
| '/v1/rooms.membersOrderedByRole': { | ||
| GET: (params: RoomsMembersOrderedByRoleProps) => PaginatedResult<{ | ||
| members: (IUser & { roles?: IRole['_id'][] })[]; | ||
| members: (IUser & { subscription: Pick<ISubscription, '_id' | 'status' | 'ts' | 'roles'> })[]; | ||
| }>; | ||
| }; |
There was a problem hiding this comment.
Breaking API change: member response structure modified.
The RoomsMembersOrderedByRoleProps response type changed from { roles?: IRole['_id'][] } to { subscription: Pick<ISubscription, '_id' | 'status' | 'ts' | 'roles'> }. This moves roles inside a subscription object and is a breaking change for API consumers.
Ensure this change is documented in release notes or migration guides, as clients using /v1/rooms.membersOrderedByRole will need to update their code to access member.subscription.roles instead of member.roles.
Task: FB-44
Related PR: RocketChat/homeserver#300
Installing local homeserver version in Rocket.Chat
Run homeserver build and bundle:
To test this locally, from the Rocket.Chat project root run:
Make sure the homeserver repository is cloned at the same directory level as the Rocket.Chat repository.
How to test without the UI
Run this in the browser console while your local Rocket.Chat instance is running.
Test cases
INVITEDstatusINVITED; send system message thatrcUser2was invitedINVITEDINVITEDstatusINVITEDINVITED; RC sends system message “remote user invited”INVITED; RC sends system message “remote user invited”INVITED; send system message “user joined”INVITED; send system message “user joined”INVITED; send system message “user joined”INVITED; send system message “user joined”INVITED; send system message “user joined”INVITED; sends system messages when invited / accepted / rejected*while a subscription is in
INVITEDstatus, the RC user cannot perform any room actions except accept or reject the invite.*to all RC started invites, we should test in three different scenarios: using new channel creation modal, room members add user sidepanel and slash command.
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.