chore: remove matrix bridged collections #37035
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 |
|
WalkthroughReplaces MatrixBridged models/helpers with native federation fields (mrid/mui/origin), updates federation APIs to accept full room/user objects and new return shapes, adjusts callback payloads to include full Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant RC as Rocket.Chat
participant FM as FederationMatrix
participant Rooms as Rooms Model
Note over RC,FM: Federated room creation (native)
User->>RC: createRoom(payload)
RC->>RC: build extraData (optionalExtra, federated?, federation.version)
RC->>FM: createRoom(room, owner, members)
FM-->>RC: { room_id, event_id }
RC->>Rooms: setAsFederated(roomId, { mrid: room_id, origin })
RC-->>User: success
sequenceDiagram
autonumber
actor Sender as Federated User
participant RC as Rocket.Chat
participant FM as FederationMatrix
participant Rooms as Rooms Model
participant Users as Users Model
Note over RC,FM: Sending a federated message
Sender->>RC: postMessage(roomId, message)
RC->>Rooms: findOne({ _id: roomId, 'federation.mrid': ... })
RC->>Users: findOne({ 'federation.mui': ... })
RC->>FM: sendMessage(message, room(native), user)
FM-->>RC: ack / error
RC-->>Sender: delivered locally
sequenceDiagram
autonumber
participant HS as Homeserver
participant RC as Rocket.Chat
participant Users as Users Model
participant Rooms as Rooms Model
Note over HS,RC: Incoming Matrix invite
HS->>RC: inviteEvent(sender, roomId, ...)
RC->>Users: findOne({ 'federation.mui': sender })
alt not found
RC->>Users: insertOne(federated user with mui/origin,...)
end
RC->>Rooms: findOne({ 'federation.mrid': roomId })
alt not found
RC->>RC: create internal room (DM/public)
end
RC-->>HS: 200 OK
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (37)
💤 Files with no reviewable changes (12)
🚧 Files skipped from review as they are similar to previous changes (14)
🧰 Additional context used🧠 Learnings (2)📓 Common learnings📚 Learning: 2025-09-19T15:15:04.642ZApplied to files:
🧬 Code graph analysis (8)apps/meteor/app/lib/server/functions/createRoom.ts (1)
apps/meteor/app/lib/server/functions/createDirectRoom.ts (1)
ee/packages/federation-matrix/src/events/message.ts (2)
apps/meteor/app/lib/server/functions/deleteMessage.ts (1)
packages/models/src/models/Rooms.ts (1)
ee/packages/federation-matrix/src/FederationMatrix.ts (9)
apps/meteor/ee/server/hooks/federation/index.ts (3)
packages/core-services/src/types/IFederationMatrixService.ts (3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
🔇 Additional comments (32)
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: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
apps/meteor/app/lib/server/functions/setUserActiveStatus.ts (1)
38-44: Group DMs can be reactivated even if some participants remain inactive.Only the first “other” uid is checked; multi‑user DMs could be re‑enabled while another participant is still inactive. Require all other members to be active before un‑readonly.
Apply:
- const roomsToReactivate = directConversations.reduce((acc: string[], room) => { - const otherUserId = isDirectMessageRoom(room) ? room.uids.find((u: string) => u !== userId) : undefined; - if (otherUserId && activeUserIds.includes(otherUserId)) { - acc.push(room._id); - } - return acc; - }, []); + const roomsToReactivate = directConversations.reduce((acc: string[], room) => { + if (!isDirectMessageRoom(room)) { + return acc; + } + const otherUserIds = room.uids.filter((u: string) => u !== userId); + if (otherUserIds.length > 0 && otherUserIds.every((id) => activeUserIds.includes(id))) { + acc.push(room._id); + } + return acc; + }, []);apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Statistics.ts (1)
48-53: Critical — fix return-type mismatch: countDistinctFederationRoomsExcluding must return string[] (not number)packages/model-typings/src/models/IRoomsModel.ts:316 and packages/models/src/models/Rooms.ts:2217 declare/implement countDistinctFederationRoomsExcluding as Promise (TODO returns 0) while apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Statistics.ts (lines 48–51) treats the result as string[] (uses .length and returns servers).
- Change IRoomsModel.countDistinctFederationRoomsExcluding signature to Promise<string[]> and implement Rooms.countDistinctFederationRoomsExcluding(...) to return the array of server names (packages/model-typings/src/models/IRoomsModel.ts, packages/models/src/models/Rooms.ts:2217).
- Normalize the settings arg at the call site and keep the UI shape. Apply this call-site change in Statistics.ts:
- const externalServers = await Rooms.countDistinctFederationRoomsExcluding(settings.get('Federation_Matrix_homeserver_domain')); + const selfDomain = settings.get('Federation_Matrix_homeserver_domain'); + const externalServers = await Rooms.countDistinctFederationRoomsExcluding(selfDomain ? [selfDomain] : []);Files to fix: packages/model-typings/src/models/IRoomsModel.ts, packages/models/src/models/Rooms.ts (implement and return string[]), and apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Statistics.ts (call-site normalization).
apps/meteor/ee/server/hooks/federation/index.ts (1)
83-83: Fix: deleteMessage call missing the required matrixRoomId parameter.The
deleteMessagemethod signature changed to requirematrixRoomIdas the first parameter, but this call only passes the message.if (FederationActions.shouldPerformFederationAction(room)) { - await FederationMatrix.deleteMessage(message); + await FederationMatrix.deleteMessage(room.federation.mrid, message); }ee/packages/federation-matrix/src/FederationMatrix.ts (2)
109-109: Bug: MEDIA_ENABLE_THUMBNAILS env cannot disable thumbnails (always true)
process.env.MEDIA_ENABLE_THUMBNAILS === 'true' || trueevaluates to true even when explicitly set to 'false'.Apply this fix so 'true' enables, 'false' disables, and default is true:
- enableThumbnails: process.env.MEDIA_ENABLE_THUMBNAILS === 'true' || true, + enableThumbnails: + process.env.MEDIA_ENABLE_THUMBNAILS === undefined + ? true + : process.env.MEDIA_ENABLE_THUMBNAILS === 'true',
646-667: Normalize invitee Matrix IDs and avoid re‑inviting existing membersCurrently forwards raw usernames; ensure '@' prefix and domain for remote users to prevent 400s from the homeserver.
Suggested change:
- await Promise.all( - usersUserName + await Promise.all( + usersUserName .filter((username) => { const isExternalUser = username.includes(':'); return isExternalUser; }) .map(async (username) => { const alreadyMember = await Subscriptions.findOneByRoomIdAndUsername(room._id, username, { projection: { _id: 1 } }); if (alreadyMember) { return; } - await this.homeserverServices.invite.inviteUserToRoom(username, room.federation.mrid, inviterUserId); + const invitee = username.startsWith('@') ? username : `@${username}`; + await this.homeserverServices.invite.inviteUserToRoom(invitee, room.federation.mrid, inviterUserId); }), );
🧹 Nitpick comments (15)
apps/meteor/app/lib/server/functions/setUserActiveStatus.ts (1)
29-37: Small optimization: avoid querying self when collecting active users.Exclude the current userId before fetching active users; it can’t affect the DM reactivation decision.
- const uniqueUserIds = [...new Set(userIds)]; + const uniqueUserIds = [...new Set(userIds)].filter((id) => id !== userId);apps/meteor/app/channel-settings/server/functions/saveRoomName.ts (1)
99-101: Add userId to payload for backward compatibility.Prevents breaking existing listeners while new user field rolls out.
Apply this diff:
- await callbacks.run('afterRoomNameChange', { room, name: displayName, oldName: room.name, user }); + await callbacks.run('afterRoomNameChange', { room, name: displayName, oldName: room.name, user, userId: user._id });packages/core-typings/src/IUser.ts (1)
229-235: Deprecation marker conflicts with active use of federation fieldsYou’ve added
muiandoriginunder the// @deprecated federationblock. If this object is the authoritative carrier for native federation during the transition, please clarify the deprecation note or point to the new canonical shape to avoid misleading consumers.- // @deprecated - federation?: { + // @deprecated (legacy shape kept during migration; prefer IUserNativeFederated['federation']) + federation?: {ee/packages/federation-matrix/src/events/message.ts (2)
134-144: Project only required fields to reduce payload and GC pressureYou only need
_idandusernameforuserand_id,name,tforroom. Fetching full docs on every message is expensive.-const user = await Users.findOne({ 'federation.mui': data.sender }); +const user = await Users.findOne({ 'federation.mui': data.sender }, { projection: { _id: 1, username: 1 } }); ... -const room = await Rooms.findOne({ 'federation.mrid': data.room_id }); +const room = await Rooms.findOne({ 'federation.mrid': data.room_id }, { projection: { _id: 1, name: 1, t: 1 } });Ensure there are DB indexes on
Users.federation.muiandRooms.federation.mrid.
145-151: Minor: unify relation parsing and guard thread fetchYou read
content['m.relates_to']twice. Simplify and guard.-const replyToRelation = content?.['m.relates_to']; -const threadRelation = content?.['m.relates_to']; -const isThreadMessage = threadRelation?.rel_type === 'm.thread'; +const relation = content?.['m.relates_to']; +const isThreadMessage = relation?.rel_type === 'm.thread'; const isQuoteMessage = replyToRelation?.['m.in_reply_to']?.event_id && !replyToRelation?.is_falling_back; -const threadRootEventId = isThreadMessage ? threadRelation.event_id : undefined; +const threadRootEventId = isThreadMessage ? relation.event_id : undefined; const thread = threadRootEventId ? await getThreadMessageId(threadRootEventId) : undefined;ee/packages/federation-matrix/src/events/room.ts (1)
10-21: LGTM: direct lookups with minimal projectionsUsing
Rooms.findOne({'federation.mrid'})andUsers.findOne({'federation.mui'})with_idprojection is correct and efficient for room.name.Confirm indexes exist on
Rooms.federation.mridandUsers.federation.mui.ee/packages/federation-matrix/src/events/invite.ts (2)
9-13: Project only what you need from roomYou only need
_idto add the user to the room.-const room = await Rooms.findOne({ 'federation.mrid': data.room_id }); +const room = await Rooms.findOne({ 'federation.mrid': data.room_id }, { projection: { _id: 1 } });
27-43: Upsert to avoid race on user creationTwo concurrent invites can race and cause a duplicate username error. Prefer atomic upsert.
-const { insertedId } = await Users.insertOne({ +const upsert = await Users.updateOne( + { username: internalUsername }, + { + $setOnInsert: { + username: internalUsername, + type: 'user', + status: UserStatus.ONLINE, + active: true, + roles: ['user'], + name: data.content.displayname || internalUsername, + requirePasswordChange: false, + createdAt: new Date(), + _updatedAt: new Date(), + federated: true, + 'federation.version': 1, + 'federation.mui': data.sender, + 'federation.origin': serverName, + }, + }, + { upsert: true }, +); -const user = await Users.findOneById(insertedId); +const user = await Users.findOne({ username: internalUsername }, { projection: { _id: 1, username: 1 } });ee/packages/federation-matrix/src/api/_matrix/invite.ts (2)
179-206: Consider error handling for user creation failure.The user creation logic correctly creates federated users with all required fields. However, there's no explicit error handling if the
insertOneoperation fails.Consider wrapping the insertion in a try-catch:
- const createdUser = await Users.insertOne({ + try { + const createdUser = await Users.insertOne({ // let the _id auto generate we deal with usernames username: inviteEvent.sender, type: 'user', status: 'online' as UserStatus, active: true, roles: ['user'], name: inviteEvent.sender, requirePasswordChange: false, federated: true, federation: { version: 1, mui: inviteEvent.sender, origin: matrixRoom.origin, }, createdAt: new Date(), _updatedAt: new Date(), - }); + }); - senderUserId = createdUser.insertedId; + senderUserId = createdUser.insertedId; + } catch (error) { + throw new Error(`Failed to create federated user ${inviteEvent.sender}: ${error}`); + }
284-288: TODO comment needs clarification.The TODO comment questions whether updating the MatrixBridgedRoom is needed. Since the PR removes MatrixBridged* models, this commented code should either be removed entirely or replaced with the appropriate federation field updates.
Consider removing this commented code block entirely since MatrixBridged* models are being removed:
- // TODO is this needed? - // if (isDM) { - // await MatrixBridgedRoom.createOrUpdateByLocalRoomId(internalRoomId, inviteEvent.roomId, matrixRoom.origin); - // }ee/packages/federation-matrix/src/FederationMatrix.ts (5)
351-355: Prefer plain for..of over for await..of on arraysThese loops iterate arrays, not async iterables; use
for..offor clarity.Example (also shown in the previous diff).
Also applies to: 219-228
732-736: Guard against missing user.username during reaction removalIf
user.usernameis undefined, the filter will never match and the reaction won't be removed.Apply a fallback:
- for await (const [eventId, username] of Object.entries(reactionData.federationReactionEventIds)) { - if (username !== user.username) { + const requester = user.username ?? user._id; + for await (const [eventId, username] of Object.entries(reactionData.federationReactionEventIds)) { + if (username !== requester) { continue; }
859-885: API surface consistency (nit): rid vs room object
updateRoomNametakesrid: string, whileupdateRoomTopicand other methods takeroom: IRoomNativeFederated. Consider aligning parameters for consistency.
931-939: Typing notifications ignore native MUI due to projection
findOneByUsernameprojects only_id, username;isUserNativeFederated(localUser)will be false and fallback will always use@username:${serverName}. Include federation fields.Apply:
- const localUser = await Users.findOneByUsername(user, { projection: { _id: 1, username: 1 } }); + const localUser = await Users.findOneByUsername(user, { projection: { _id: 1, username: 1, federated: 1, federation: 1 } });
399-399: Log message wording (nit)"bridged user" is legacy terminology; prefer "federated user" to reflect the new model.
📜 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 (32)
apps/meteor/app/channel-settings/server/functions/saveRoomName.ts(1 hunks)apps/meteor/app/channel-settings/server/functions/saveRoomTopic.ts(1 hunks)apps/meteor/app/lib/server/functions/createRoom.ts(1 hunks)apps/meteor/app/lib/server/functions/setUserActiveStatus.ts(1 hunks)apps/meteor/ee/server/hooks/federation/index.ts(8 hunks)apps/meteor/lib/callbacks.ts(2 hunks)apps/meteor/server/models.ts(0 hunks)apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Statistics.ts(2 hunks)ee/packages/federation-matrix/src/FederationMatrix.ts(22 hunks)ee/packages/federation-matrix/src/api/_matrix/invite.ts(4 hunks)ee/packages/federation-matrix/src/events/edu.ts(4 hunks)ee/packages/federation-matrix/src/events/invite.ts(3 hunks)ee/packages/federation-matrix/src/events/member.ts(2 hunks)ee/packages/federation-matrix/src/events/message.ts(2 hunks)ee/packages/federation-matrix/src/events/room.ts(1 hunks)ee/packages/federation-matrix/src/helpers/identifiers.ts(0 hunks)packages/core-services/src/types/IFederationMatrixService.ts(2 hunks)packages/core-typings/src/IRoom.ts(1 hunks)packages/core-typings/src/IUser.ts(2 hunks)packages/core-typings/src/federation/IMatrixBridgedRoom.ts(0 hunks)packages/core-typings/src/federation/IMatrixBridgedUser.ts(0 hunks)packages/core-typings/src/federation/index.ts(0 hunks)packages/model-typings/src/index.ts(0 hunks)packages/model-typings/src/models/IMatrixBridgedRoomModel.ts(0 hunks)packages/model-typings/src/models/IMatrixBridgedUserModel.ts(0 hunks)packages/model-typings/src/models/IRoomsModel.ts(2 hunks)packages/models/src/index.ts(0 hunks)packages/models/src/modelClasses.ts(0 hunks)packages/models/src/models/MatrixBridgedRoom.ts(0 hunks)packages/models/src/models/MatrixBridgedUser.ts(0 hunks)packages/models/src/models/Rooms.ts(2 hunks)packages/models/src/models/Subscriptions.ts(1 hunks)
💤 Files with no reviewable changes (12)
- packages/model-typings/src/index.ts
- packages/core-typings/src/federation/IMatrixBridgedRoom.ts
- packages/core-typings/src/federation/IMatrixBridgedUser.ts
- packages/core-typings/src/federation/index.ts
- packages/model-typings/src/models/IMatrixBridgedRoomModel.ts
- packages/models/src/models/MatrixBridgedUser.ts
- packages/models/src/modelClasses.ts
- ee/packages/federation-matrix/src/helpers/identifiers.ts
- packages/models/src/index.ts
- apps/meteor/server/models.ts
- packages/model-typings/src/models/IMatrixBridgedUserModel.ts
- packages/models/src/models/MatrixBridgedRoom.ts
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: rodrigok
PR: RocketChat/Rocket.Chat#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.
📚 Learning: 2025-09-19T15:15:04.642Z
Learnt from: rodrigok
PR: RocketChat/Rocket.Chat#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:
apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Statistics.tspackages/core-typings/src/IUser.tsapps/meteor/app/lib/server/functions/setUserActiveStatus.tspackages/core-services/src/types/IFederationMatrixService.tsee/packages/federation-matrix/src/FederationMatrix.tsapps/meteor/ee/server/hooks/federation/index.ts
🧬 Code graph analysis (14)
apps/meteor/lib/callbacks.ts (2)
packages/core-typings/src/IRoom.ts (1)
IRoom(21-95)packages/core-typings/src/IUser.ts (1)
IUser(186-255)
packages/model-typings/src/models/IRoomsModel.ts (1)
packages/core-typings/src/IRoom.ts (1)
IRoom(21-95)
apps/meteor/app/channel-settings/server/functions/saveRoomTopic.ts (2)
apps/meteor/lib/callbacks.ts (1)
callbacks(251-259)apps/meteor/app/channel-settings/server/methods/saveRoomSettings.ts (1)
roomTopic(226-233)
ee/packages/federation-matrix/src/events/room.ts (2)
apps/meteor/app/livechat/server/methods/closeRoom.ts (1)
roomId(46-96)packages/core-services/src/index.ts (1)
Room(167-167)
packages/models/src/models/Rooms.ts (1)
packages/core-typings/src/IRoom.ts (1)
IRoom(21-95)
ee/packages/federation-matrix/src/events/message.ts (2)
packages/models/src/index.ts (2)
Users(205-205)Rooms(196-196)ee/packages/federation-matrix/src/events/room.ts (1)
room(6-59)
apps/meteor/app/channel-settings/server/functions/saveRoomName.ts (2)
apps/meteor/lib/callbacks.ts (1)
callbacks(251-259)ee/packages/federation-matrix/src/events/room.ts (1)
room(6-59)
ee/packages/federation-matrix/src/api/_matrix/invite.ts (1)
packages/core-services/src/index.ts (1)
Room(167-167)
ee/packages/federation-matrix/src/events/member.ts (2)
ee/packages/federation-matrix/src/events/room.ts (1)
room(6-59)packages/core-services/src/index.ts (1)
Room(167-167)
apps/meteor/app/lib/server/functions/createRoom.ts (1)
packages/core-typings/src/IRoom.ts (1)
IRoom(21-95)
packages/core-services/src/types/IFederationMatrixService.ts (3)
packages/core-typings/src/IRoom.ts (2)
IRoomFederated(109-112)IRoomNativeFederated(114-120)packages/core-typings/src/IUser.ts (1)
IUser(186-255)packages/core-typings/src/IMessage/IMessage.ts (1)
IMessage(138-238)
ee/packages/federation-matrix/src/FederationMatrix.ts (7)
packages/models/src/index.ts (2)
Users(205-205)Rooms(196-196)packages/core-typings/src/IUser.ts (2)
isUserNativeFederated(275-276)IUser(186-255)packages/core-typings/src/IRoom.ts (3)
IRoom(21-95)IRoomNativeFederated(114-120)isRoomNativeFederated(124-125)ee/packages/federation-matrix/src/events/member.ts (1)
member(9-51)ee/packages/federation-matrix/src/events/message.ts (1)
message(121-290)ee/packages/federation-matrix/src/events/reaction.ts (1)
reaction(10-93)ee/packages/federation-matrix/src/helpers/message.parsers.ts (1)
toExternalMessageFormat(201-212)
apps/meteor/ee/server/hooks/federation/index.ts (3)
ee/packages/federation-matrix/src/FederationMatrix.ts (1)
FederationMatrix(48-940)apps/meteor/server/services/room/hooks/BeforeFederationActions.ts (1)
FederationActions(6-30)packages/core-typings/src/IUser.ts (1)
isUserNativeFederated(275-276)
ee/packages/federation-matrix/src/events/invite.ts (2)
ee/packages/federation-matrix/src/events/room.ts (1)
room(6-59)packages/core-services/src/index.ts (1)
Room(167-167)
⏰ 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 (45)
apps/meteor/app/lib/server/functions/setUserActiveStatus.ts (2)
67-72: Federated guard may be too coarse; confirm intent (local vs remote).Throwing for any “federated” user may block deactivating locally‑owned federated accounts. If the intent is to block only remote users, narrow the check (e.g., based on user.federation.origin).
Would you like a follow‑up patch that permits local federated users while still blocking remote placeholders?
3-3: Import consolidation OK — @rocket.chat/models exposes required APIs.All methods in the original checklist are present in the model typings and implementations (see packages/model-typings/src/models/* and packages/models/src/models/Rooms.ts, Users.ts, Subscriptions.ts).
packages/models/src/models/Subscriptions.ts (1)
2081-2084: Derive externalRoomId from room.federation.mrid — looks good; please confirm migration coverage.LGTM replacing bridged lookup. Verify all federated rooms have federation.mrid populated to avoid undefined externalRoomId; if not guaranteed, consider filtering for rooms where federation.mrid exists.
apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Statistics.ts (1)
2-2: Import swap to Rooms/Users is correct.Removal of bridged models at import site looks good.
packages/models/src/models/Rooms.ts (1)
670-672: Ensure typings include federation.origin.Method sets federation.origin, but IRoomNativeFederated originally lacked this field. After adding it in core-typings, this is consistent. No change needed here beyond that.
packages/model-typings/src/models/IRoomsModel.ts (1)
127-127: API surface aligns with setAsFederated changes.Object param with mrid/origin is consistent with usage.
packages/core-typings/src/IUser.ts (2)
263-263: No action neededWhitespace-only adjustment.
266-273: Good: required native federation shape on usersRequiring
muiandoriginalongsideversioninIUserNativeFederated['federation']is consistent with the rest of the PR.Confirm all writers of
IUserNativeFederatedpopulate non-empty strings formuiandorigin(invite flow, backfills).ee/packages/federation-matrix/src/events/message.ts (1)
6-6: Import changes OKModel imports consolidation looks fine.
ee/packages/federation-matrix/src/events/room.ts (2)
26-37: LGTM: topic updates use minimal fieldsPassing
{ _id, username }for the actor is sensible; ensure remote users’usernameequals their MUI (as inserted).
42-58: LGTM: role changes flowRole application via resolved local IDs is consistent with the refactor. Consider validating/whitelisting incoming
rolevalues elsewhere to avoid unexpected role names.ee/packages/federation-matrix/src/events/invite.ts (1)
50-51: LGTM: membership linkageAdding the (possibly newly created) user to the resolved room is correct.
apps/meteor/app/lib/server/functions/createRoom.ts (1)
134-145: Restore teamId in extraData & use Partial (avoid unsafe cast)Verified: the grep output shows the only prepareCreateRoomCallback consumer is apps/meteor/app/e2e/server/beforeCreateRoom.ts and it does not reference extraData.teamId — keep teamId in extraData and tighten the type.
-const { teamId, ...optionalExtraData } = roomExtraData || ({} as IRoom); +const { teamId, ...optionalExtraData } = (roomExtraData ?? {}) as Partial<IRoom>; -const extraData = { - ...optionalExtraData, - ...(optionalExtraData.federated && { - federated: true, - federation: { - version: 1, - // TODO we should be able to provide all values from here, currently we update on callback afterCreateRoom - }, - }), -}; +const extraData = { + ...optionalExtraData, + ...(teamId && { teamId }), + ...(optionalExtraData.federated && { + federated: true, + federation: { version: 1 }, // mrid/origin can be added by callbacks if not known here + }), +};ee/packages/federation-matrix/src/events/edu.ts (6)
13-17: LGTM! The migration to native federation fields is correct.The transition from
MatrixBridgedRoomto directly queryingRoomswithfederation.mridis implemented correctly, maintaining the appropriate projection and error handling.
19-23: LGTM! User lookup correctly migrated to native federation.The user lookup properly uses
federation.muiand correctly validates both user existence and username availability before proceeding.
25-29: LGTM! Broadcast properly uses migrated fields.The activity broadcast correctly uses
matrixUser.usernameandmatrixRoom._idfrom the migrated lookups.
37-41: LGTM! Presence user lookup properly migrated.The presence event handling correctly queries users via
federation.muiwith appropriate error handling for missing users.
50-58: LGTM! User status update uses correct ID.The status update correctly uses
matrixUser._idfor the update query and properly sets bothstatusandstatusDefaultfields.
60-65: LGTM! Presence broadcast uses correct user fields.The presence status broadcast correctly destructures and uses fields directly from
matrixUser, eliminating the need for a separate user lookup.ee/packages/federation-matrix/src/events/member.ts (4)
18-22: LGTM! Room lookup correctly migrated to native federation.The room lookup properly uses
federation.mridwith appropriate projection and maintains the existing error handling for missing rooms.
25-29: LGTM! User lookup properly migrated with correct logging.The affected user lookup correctly uses
federation.muiand the error log appropriately referencesdata.state_keyfor the Matrix user identifier.
34-35: LGTM! Voluntary leave correctly uses room._id.The voluntary leave logic properly uses
room._idand passes the completeaffectedUserobject to the removal service.
38-45: LGTM! Kick logic correctly migrated.The kick handling properly:
- Looks up the kicker via
federation.mui- Uses
room._idfor the removal operation- Maintains the fallback for missing kicker users
- Correctly logs with updated identifiers
ee/packages/federation-matrix/src/api/_matrix/invite.ts (2)
214-216: LGTM! Room lookup correctly uses native federation fields.The room lookup properly queries using
federation.mridto find existing mapped rooms.
259-262: Federation metadata is set on room creation — no change required.Rooms.setAsFederated(...) is called after room creation in ee/packages/federation-matrix/src/FederationMatrix.ts (lines 217, 402) and in apps/meteor/ee/server/hooks/federation/index.ts (lines 30–41); the implementation (packages/models/src/models/Rooms.ts:670–672) sets federated=true and writes federation.mrid and federation.origin.
packages/core-services/src/types/IFederationMatrixService.ts (4)
1-1: LGTM! Import correctly updated to include IRoomNativeFederated.The import statement properly includes the new
IRoomNativeFederatedtype needed for the migrated federation methods.
18-18: LGTM! createRoom now returns federation identifiers.The return type change to include
room_idandevent_idaligns with the federation requirements and allows proper tracking of federated rooms.
27-30: LGTM! Methods correctly updated to use full room and user objects.The signature changes for
kickUser,updateMessage,updateRoomName, andupdateRoomTopicproperly transition from passing IDs to passing full objects, which provides better type safety and access to federation fields.
31-36: LGTM! addUserRoleRoomScoped correctly uses IRoomNativeFederated.The method signature properly uses the native federated room type for role management operations.
apps/meteor/ee/server/hooks/federation/index.ts (8)
22-33: LGTM! Room federation setup correctly handles the createRoom result.The code properly:
- Handles the result from
FederationMatrix.createRoom- Extracts the
room_idand derives theoriginfrom it- Calls
setAsFederatedwith the correct federation data
38-41: LGTM! Federated room ID handling is consistent.The code properly handles pre-existing federated room IDs by extracting the origin and setting the federation data.
162-162: LGTM! kickUser correctly passes the room object.The method call properly passes the full room object instead of just the room ID, aligning with the new signature.
171-174: LGTM! Room name change correctly passes user object.The callback properly passes the full user object to
updateRoomName, aligning with the new signature requirements.
182-185: LGTM! Room topic change correctly passes room and user objects.The callback properly passes both the full room and user objects to
updateRoomTopic.
199-199: LGTM! Message update correctly passes room object.The
updateMessagecall properly passes the full room object along with the message.
209-209: LGTM! Role change correctly passes room object.The
addUserRoleRoomScopedcall properly passes the full room object for scoped role management.
49-51: Clarify conditional: allow federation when FederationActions.shouldPerformFederationAction(room) OR isUserNativeFederated(user).
The guardif (!FederationActions.shouldPerformFederationAction(room) && !isUserNativeFederated(user)) return;means execution proceeds only ifFederationActions.shouldPerformFederationAction(room) || isUserNativeFederated(user); confirm this is intended and add a test or doc. (apps/meteor/ee/server/hooks/federation/index.ts:49–51)Likely an incorrect or invalid review comment.
ee/packages/federation-matrix/src/FederationMatrix.ts (8)
8-16: Good move: shift to native federation type guards and enumsUsing isRoomNativeFederated/isUserNativeFederated and UserStatus aligns this service with the new native federation model and removes bridged-model coupling.
198-207: Room-type gate is fineRestricting creation to public/private channels ('c'/'p') is appropriate here.
217-218: Correctly marking rooms as native‑federatedPersisting MRID/origin via Rooms.setAsFederated is the right replacement for bridged room mapping.
402-405: DM: mark room as native‑federatedConsistent with channels, updating MRID/origin here is correct.
550-558: User MUI derivation is correctFalling back to
@username:serverfor non‑native users fits the native‑federation design.
771-789: Leave room flow looks correctNative federated room check + MUI derivation + homeserver call sequence is sound.
824-849: Update message: correct MUI derivation and formatted contentGood use of
toExternalMessageFormatand guarding for missing eventId.
141-158: Verified — findUserFederatedRoomIds projects externalRoomIdModel returns AggregationCursor<{ _id: IRoom['_id']; externalRoomId: string }> and projects externalRoomId from room.federation.mrid (packages/models/src/models/Subscriptions.ts:2060, 2083); no change required — fanout already filters falsy targets via .filter(Boolean).
| await Message.saveSystemMessage('room_changed_topic', rid, roomTopic || '', user); | ||
| } | ||
| await callbacks.run('afterRoomTopicChange', undefined, { room, topic: roomTopic, userId: user._id }); | ||
| await callbacks.run('afterRoomTopicChange', undefined, { room, topic: roomTopic, user }); |
There was a problem hiding this comment.
Pass userId for compatibility with existing handlers.
Current payload changed to user. Provide userId too to minimize breakage and align with callbacks type update.
Apply this diff:
- await callbacks.run('afterRoomTopicChange', undefined, { room, topic: roomTopic, user });
+ await callbacks.run('afterRoomTopicChange', undefined, { room, topic: roomTopic, user, userId: user._id });Additionally, consider typing the parameter as AtLeast<IUser, '_id' | 'username'> or loading the full IUser before invoking callbacks.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await callbacks.run('afterRoomTopicChange', undefined, { room, topic: roomTopic, user }); | |
| await callbacks.run('afterRoomTopicChange', undefined, { room, topic: roomTopic, user, userId: user._id }); |
🤖 Prompt for AI Agents
In apps/meteor/app/channel-settings/server/functions/saveRoomTopic.ts around
line 31, the callbacks.run('afterRoomTopicChange', ...) call currently passes
user but omits userId; update the payload to include userId (user._id) alongside
user to maintain compatibility with existing handlers, and consider typing the
parameter as AtLeast<IUser, '_id' | 'username'> or load/ensure a full IUser
before invoking callbacks so the userId and username are present.
| 'afterDeleteUser': (user: IUser) => void; | ||
| 'afterFileUpload': (params: { user: IUser; room: IRoom; message: IMessage }) => void; | ||
| 'afterRoomNameChange': (params: { room: IRoom; name: string; oldName: string; userId: IUser['_id'] }) => void; | ||
| 'afterRoomNameChange': (params: { room: IRoom; name: string; oldName: string; user: IUser }) => void; |
There was a problem hiding this comment.
Avoid breaking existing listeners: keep userId as a backward‑compat payload field.
Many callbacks likely read params.userId. Add an optional userId while introducing user to avoid ecosystem breakage.
Apply this diff:
- 'afterRoomNameChange': (params: { room: IRoom; name: string; oldName: string; user: IUser }) => void;
+ 'afterRoomNameChange': (params: { room: IRoom; name: string; oldName: string; user: IUser; userId?: IUser['_id'] }) => void;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 'afterRoomNameChange': (params: { room: IRoom; name: string; oldName: string; user: IUser }) => void; | |
| 'afterRoomNameChange': (params: { room: IRoom; name: string; oldName: string; user: IUser; userId?: IUser['_id'] }) => void; |
🤖 Prompt for AI Agents
In apps/meteor/lib/callbacks.ts around line 51, the afterRoomNameChange callback
type was changed to include user but removed the legacy userId; update the
params type to include both the new user?: IUser and the backward-compatible
userId?: string (optional) so existing listeners that read params.userId
continue to work while new code can use params.user.
| 'afterRoomTopicChange': (params: undefined, { room, topic, user }: { room: IRoom; topic: string; user: IUser }) => void; | ||
| }; |
There was a problem hiding this comment.
Match topic-change callback payload with name-change: add optional userId.
Keeps parity and eases migration for handlers still expecting userId.
Apply this diff:
- 'afterRoomTopicChange': (params: undefined, { room, topic, user }: { room: IRoom; topic: string; user: IUser }) => void;
+ 'afterRoomTopicChange': (params: undefined, { room, topic, user, userId }: { room: IRoom; topic: string; user: IUser; userId?: IUser['_id'] }) => void;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 'afterRoomTopicChange': (params: undefined, { room, topic, user }: { room: IRoom; topic: string; user: IUser }) => void; | |
| }; | |
| 'afterRoomTopicChange': (params: undefined, { room, topic, user, userId }: { room: IRoom; topic: string; user: IUser; userId?: IUser['_id'] }) => void; | |
| }; |
🤖 Prompt for AI Agents
In apps/meteor/lib/callbacks.ts around lines 208-209, the afterRoomTopicChange
callback payload currently types the second parameter as { room: IRoom; topic:
string; user: IUser }; update that type to include an optional userId property
to match name-change parity — e.g. { room: IRoom; topic: string; user: IUser;
userId?: string } — so handlers that still expect userId continue to work;
adjust any related type exports or usages if needed.
| const [, serverName] = data.sender.split(':'); | ||
| if (!serverName) { | ||
| throw new Error('Invalid sender format, missing server name'); | ||
| } | ||
|
|
There was a problem hiding this comment.
Bug: serverName parsing breaks when homeserver uses a port
Matrix user IDs are @localpart:serverName[:port]. split(':') drops the port. Use first-colon split or slice after the first colon.
-const [, serverName] = data.sender.split(':');
-if (!serverName) {
+const firstColon = data.sender.indexOf(':');
+const serverName = firstColon >= 0 ? data.sender.slice(firstColon + 1) : '';
+if (!serverName) {
throw new Error('Invalid sender format, missing server name');
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const [, serverName] = data.sender.split(':'); | |
| if (!serverName) { | |
| throw new Error('Invalid sender format, missing server name'); | |
| } | |
| const firstColon = data.sender.indexOf(':'); | |
| const serverName = firstColon >= 0 ? data.sender.slice(firstColon + 1) : ''; | |
| if (!serverName) { | |
| throw new Error('Invalid sender format, missing server name'); | |
| } |
🤖 Prompt for AI Agents
In ee/packages/federation-matrix/src/events/invite.ts around lines 22-26, the
current code uses data.sender.split(':') which drops a homeserver port; instead
find the first colon and slice after it so the serverName retains any additional
colons/port. Replace the split with: locate the index of the first ':' in
data.sender, throw if not found, then set serverName =
data.sender.slice(firstColon + 1) and validate it's non-empty.
| for await (const member of members) { | ||
| if (member === owner.username) { | ||
| continue; | ||
| } | ||
|
|
||
| try { | ||
| // TODO: Check if it is external user - split domain etc | ||
| const localUserId = await Users.findOneByUsername(member); | ||
| if (localUserId) { | ||
| await MatrixBridgedUser.createOrUpdateByLocalId(localUserId._id, member, false, this.serverName); | ||
| // continue; | ||
| } | ||
| } catch (error) { | ||
| this.logger.error('Error creating or updating bridged user:', error); | ||
| } | ||
| // We are not generating bridged users for members outside of the current workspace | ||
| // They will be created when the invite is accepted | ||
|
|
||
| await this.homeserverServices.invite.inviteUserToRoom(member, matrixRoomResult.room_id, matrixUserId); | ||
| } |
There was a problem hiding this comment.
Normalize invitee identifiers to valid Matrix user IDs
inviteUserToRoom likely expects Matrix user IDs. Passing raw Rocket.Chat usernames (no '@' or domain) will fail for locals and inconsistent for remotes.
Apply normalization:
- for await (const member of members) {
+ for (const member of members) {
if (member === owner.username) {
continue;
}
- await this.homeserverServices.invite.inviteUserToRoom(member, matrixRoomResult.room_id, matrixUserId);
+ const invitee = member.includes(':')
+ ? (member.startsWith('@') ? member : `@${member}`)
+ : `@${member}:${this.serverName}`;
+ await this.homeserverServices.invite.inviteUserToRoom(invitee, matrixRoomResult.room_id, matrixUserId);
}Also consider skipping local users entirely if they are auto-joined by your flow.
Committable suggestion skipped: line range outside the PR's diff.
| // Matrix's room ID. Example: !XqJXqZxXqJXq:matrix.org | ||
| mrid: string; | ||
| }; |
There was a problem hiding this comment.
Typing bug: federation.origin is used elsewhere but not declared here.
Rooms.setAsFederated sets federation.origin; Statistics depends on it. Add origin to the type.
Apply this diff:
export interface IRoomNativeFederated extends IRoomFederated {
federation: {
version: number;
- // Matrix's room ID. Example: !XqJXqZxXqJXq:matrix.org
- mrid: string;
+ // Matrix's room ID. Example: !XqJXqZxXqJXq:matrix.org
+ mrid: string;
+ // Origin homeserver of the room (server name)
+ origin: string;
};
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Matrix's room ID. Example: !XqJXqZxXqJXq:matrix.org | |
| mrid: string; | |
| }; | |
| export interface IRoomNativeFederated extends IRoomFederated { | |
| federation: { | |
| version: number; | |
| // Matrix's room ID. Example: !XqJXqZxXqJXq:matrix.org | |
| mrid: string; | |
| // Origin homeserver of the room (server name) | |
| origin: string; | |
| }; | |
| } |
🤖 Prompt for AI Agents
In packages/core-typings/src/IRoom.ts around lines 117-119, the
federation.origin field is referenced elsewhere but not declared in the type;
update the IRoom federation property to include origin: string (non-optional) so
Room.setAsFederated and Statistics consumers compile correctly, and
run/typecheck tests to ensure no other type errors.
sampaiodiego
force-pushed
the
remove-bridged-models
branch
from
70ceecd to
3445d55
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
ee/packages/federation-matrix/src/api/_matrix/invite.ts (1)
186-203: Code duplication - extract user creation logic.This user creation logic is duplicated from
ee/packages/federation-matrix/src/events/invite.ts. Extract it to a shared utility function.Would you like me to create a shared utility function for federated user creation to eliminate this duplication?
ee/packages/federation-matrix/src/FederationMatrix.ts (3)
107-107: Fix env flag parsing:enableThumbnailsis always true
=== 'true' || truemakes the value unconditionally true. Parse the env var correctly.- enableThumbnails: process.env.MEDIA_ENABLE_THUMBNAILS === 'true' || true, + enableThumbnails: (process.env.MEDIA_ENABLE_THUMBNAILS ?? 'true') === 'true',
250-256: Username normalization can produce invalid IDs (e.g.,@@foo:server)Building
externalUserIdfromusernamecan double-prefix@or miss the domain. Also computelocalNamefrom the normalized value.- if (!username.includes(':') && !username.includes('@')) { + if (!username.includes(':') && !username.includes('@')) { continue; } - - const externalUserId = username.includes(':') ? `@${username}` : `@${username}:${this.serverName}`; + const externalUserId = username.includes(':') + ? (username.startsWith('@') ? username : `@${username}`) + : (username.startsWith('@') ? `${username}:${this.serverName}` : `@${username}:${this.serverName}`); @@ - const localName = username.split(':')[0]?.replace('@', '') || username; + const localName = externalUserId.replace(/^@/, '').split(':')[0];Also applies to: 269-276
644-661: Normalize invitees to@user:domainininviteUsersToRoomUsernames without
@will likely be rejected by the homeserver.- .map(async (username) => { + .map(async (username) => { const alreadyMember = await Subscriptions.findOneByRoomIdAndUsername(room._id, username, { projection: { _id: 1 } }); if (alreadyMember) { return; } - await this.homeserverServices.invite.inviteUserToRoom(username, room.federation.mrid, inviterUserId); + const invitee = username.startsWith('@') ? username : `@${username}`; + await this.homeserverServices.invite.inviteUserToRoom(invitee, room.federation.mrid, inviterUserId); }),
🧹 Nitpick comments (7)
apps/meteor/ee/server/hooks/federation/index.ts (3)
60-63: Use project logger instead of console.error.Align with Rocket.Chat logging (structured logs, levels, context).
Example:
- console.error('[sendMessage] Failed to send message to Native Federation:', error); + SystemLogger.error({ msg: 'Native Federation sendMessage failed', rid: room?._id, mid: message?._id, err: error });(adapt to the repo’s logger utility in use)
75-77: External-user detection via “:”.This heuristic is brittle. Prefer a dedicated helper to detect Matrix/remote users (handles “@user:server”, ports, IPv6, future formats).
Happy to draft a small util like isExternalFederatedUsername(username: string): boolean and reuse across reactions/delete/update.
28-41: Optional: extract a helper to parse origin once.Avoid duplication and inconsistent parsing.
Add:
function originFromMrid(mrid: string): string { const i = mrid.indexOf(':'); return i >= 0 ? mrid.slice(i + 1) : ''; }Then:
- const fromServer = room_id.slice(room_id.indexOf(':') + 1); + const fromServer = originFromMrid(room_id); ... - const fromServer = federatedRoomId.slice(federatedRoomId.indexOf(':') + 1); + const fromServer = originFromMrid(federatedRoomId);ee/packages/federation-matrix/src/events/invite.ts (1)
27-43: Consider extracting user creation to a shared utility.The user creation logic appears in multiple places (invite.ts and api/_matrix/invite.ts). Consider extracting this to a shared utility function to ensure consistency and reduce duplication.
Create a shared utility:
// In a new file: ee/packages/federation-matrix/src/utils/userCreation.ts export async function createFederatedUser(mui: string, origin: string, displayName?: string) { return Users.insertOne({ username: mui, type: 'user', status: UserStatus.ONLINE, active: true, roles: ['user'], name: displayName || mui, requirePasswordChange: false, createdAt: new Date(), _updatedAt: new Date(), federated: true, federation: { version: 1, mui, origin, }, }); }ee/packages/federation-matrix/src/api/_matrix/invite.ts (2)
179-183: Add projection for performance optimization.When checking user existence, add a projection to only fetch the
_idfield for better performance.-const senderUser = await Users.findOne({ 'federation.mui': inviteEvent.sender }, { projection: { _id: 1 } }); +const senderUser = await Users.findOne({ 'federation.mui': inviteEvent.sender }, { projection: { _id: 1 } });
284-287: Remove commented-out code.This TODO comment with the commented-out code should be either addressed or removed entirely for cleaner code.
-// TODO is this needed? -// if (isDM) { -// await MatrixBridgedRoom.createOrUpdateByLocalRoomId(internalRoomId, inviteEvent.roomId, matrixRoom.origin); -// }ee/packages/federation-matrix/src/FederationMatrix.ts (1)
664-665: Microcopy: “a user”, not “an user”Minor log grammar nit.
- this.logger.error({ msg: 'Failed to invite an user to Matrix:', err: error }); + this.logger.error({ msg: 'Failed to invite a user to Matrix:', err: error });
📜 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 (32)
apps/meteor/app/channel-settings/server/functions/saveRoomName.ts(1 hunks)apps/meteor/app/channel-settings/server/functions/saveRoomTopic.ts(1 hunks)apps/meteor/app/lib/server/functions/createRoom.ts(1 hunks)apps/meteor/app/lib/server/functions/setUserActiveStatus.ts(1 hunks)apps/meteor/ee/server/hooks/federation/index.ts(8 hunks)apps/meteor/lib/callbacks.ts(2 hunks)apps/meteor/server/models.ts(0 hunks)apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Statistics.ts(2 hunks)ee/packages/federation-matrix/src/FederationMatrix.ts(22 hunks)ee/packages/federation-matrix/src/api/_matrix/invite.ts(4 hunks)ee/packages/federation-matrix/src/events/edu.ts(4 hunks)ee/packages/federation-matrix/src/events/invite.ts(3 hunks)ee/packages/federation-matrix/src/events/member.ts(2 hunks)ee/packages/federation-matrix/src/events/message.ts(2 hunks)ee/packages/federation-matrix/src/events/room.ts(1 hunks)ee/packages/federation-matrix/src/helpers/identifiers.ts(0 hunks)packages/core-services/src/types/IFederationMatrixService.ts(2 hunks)packages/core-typings/src/IRoom.ts(1 hunks)packages/core-typings/src/IUser.ts(2 hunks)packages/core-typings/src/federation/IMatrixBridgedRoom.ts(0 hunks)packages/core-typings/src/federation/IMatrixBridgedUser.ts(0 hunks)packages/core-typings/src/federation/index.ts(0 hunks)packages/model-typings/src/index.ts(0 hunks)packages/model-typings/src/models/IMatrixBridgedRoomModel.ts(0 hunks)packages/model-typings/src/models/IMatrixBridgedUserModel.ts(0 hunks)packages/model-typings/src/models/IRoomsModel.ts(2 hunks)packages/models/src/index.ts(0 hunks)packages/models/src/modelClasses.ts(0 hunks)packages/models/src/models/MatrixBridgedRoom.ts(0 hunks)packages/models/src/models/MatrixBridgedUser.ts(0 hunks)packages/models/src/models/Rooms.ts(2 hunks)packages/models/src/models/Subscriptions.ts(1 hunks)
💤 Files with no reviewable changes (12)
- packages/models/src/modelClasses.ts
- packages/models/src/index.ts
- packages/model-typings/src/models/IMatrixBridgedUserModel.ts
- packages/model-typings/src/index.ts
- packages/core-typings/src/federation/IMatrixBridgedRoom.ts
- packages/core-typings/src/federation/index.ts
- packages/core-typings/src/federation/IMatrixBridgedUser.ts
- ee/packages/federation-matrix/src/helpers/identifiers.ts
- apps/meteor/server/models.ts
- packages/models/src/models/MatrixBridgedRoom.ts
- packages/models/src/models/MatrixBridgedUser.ts
- packages/model-typings/src/models/IMatrixBridgedRoomModel.ts
🚧 Files skipped from review as they are similar to previous changes (13)
- apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Statistics.ts
- ee/packages/federation-matrix/src/events/room.ts
- apps/meteor/app/channel-settings/server/functions/saveRoomName.ts
- packages/core-typings/src/IRoom.ts
- ee/packages/federation-matrix/src/events/edu.ts
- apps/meteor/app/lib/server/functions/setUserActiveStatus.ts
- apps/meteor/app/lib/server/functions/createRoom.ts
- apps/meteor/lib/callbacks.ts
- packages/model-typings/src/models/IRoomsModel.ts
- packages/models/src/models/Subscriptions.ts
- apps/meteor/app/channel-settings/server/functions/saveRoomTopic.ts
- ee/packages/federation-matrix/src/events/message.ts
- packages/core-typings/src/IUser.ts
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: rodrigok
PR: RocketChat/Rocket.Chat#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.
📚 Learning: 2025-09-19T15:15:04.642Z
Learnt from: rodrigok
PR: RocketChat/Rocket.Chat#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:
apps/meteor/ee/server/hooks/federation/index.tspackages/core-services/src/types/IFederationMatrixService.tsee/packages/federation-matrix/src/FederationMatrix.ts
🧬 Code graph analysis (7)
ee/packages/federation-matrix/src/events/member.ts (2)
ee/packages/federation-matrix/src/events/room.ts (1)
room(6-59)packages/core-services/src/index.ts (1)
Room(167-167)
ee/packages/federation-matrix/src/api/_matrix/invite.ts (1)
packages/core-services/src/index.ts (1)
Room(167-167)
packages/models/src/models/Rooms.ts (1)
packages/core-typings/src/IRoom.ts (1)
IRoom(21-95)
ee/packages/federation-matrix/src/events/invite.ts (2)
ee/packages/federation-matrix/src/events/room.ts (1)
room(6-59)packages/core-services/src/index.ts (1)
Room(167-167)
apps/meteor/ee/server/hooks/federation/index.ts (2)
ee/packages/federation-matrix/src/FederationMatrix.ts (1)
FederationMatrix(46-938)packages/core-typings/src/IUser.ts (1)
isUserNativeFederated(275-276)
packages/core-services/src/types/IFederationMatrixService.ts (3)
packages/core-typings/src/IRoom.ts (2)
IRoomFederated(109-112)IRoomNativeFederated(114-120)ee/packages/federation-matrix/src/events/message.ts (1)
message(123-293)packages/core-typings/src/IMessage/IMessage.ts (1)
IMessage(138-238)
ee/packages/federation-matrix/src/FederationMatrix.ts (6)
packages/core-typings/src/IUser.ts (2)
isUserNativeFederated(275-276)IUser(186-255)ee/packages/federation-matrix/src/events/room.ts (1)
room(6-59)packages/core-typings/src/IRoom.ts (3)
IRoom(21-95)IRoomNativeFederated(114-120)isRoomNativeFederated(124-125)ee/packages/federation-matrix/src/events/member.ts (1)
member(9-51)ee/packages/federation-matrix/src/events/reaction.ts (1)
reaction(10-93)ee/packages/federation-matrix/src/helpers/message.parsers.ts (1)
toExternalMessageFormat(201-212)
⏰ 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 (28)
apps/meteor/ee/server/hooks/federation/index.ts (7)
2-3: Imports look good.Type-only imports are correct; adding isUserNativeFederated and Rooms aligns with downstream usage.
49-52: Re-evaluate OR condition for send path.As written, a native‑federated user in a non‑federated room will trigger sendMessage(room, …) with a non‑federated room, likely erroring and spamming logs.
Consider tightening:
- if (!FederationActions.shouldPerformFederationAction(room) && !isUserNativeFederated(user)) { + if (!FederationActions.shouldPerformFederationAction(room) || !room?.federation?.mrid) { return; }If the OR is intentional, add a guard for room.federation?.mrid before calling sendMessage.
53-59: Good loop-avoidance guard.Skipping when message already has a federation eventId avoids echo loops.
162-162: kickUser API usage matches new signature.Passing room and full users aligns with FederationMatrix.kickUser.
171-174: updateRoomName call signature is correct.rid + name + full user matches the new API.
182-185: updateRoomTopic call signature is correct.Passing the full room, topic, and user matches the API.
209-209: addUserRoleRoomScoped usage matches new signature.Room + fromUserId + userId + role is consistent with API.
ee/packages/federation-matrix/src/events/member.ts (4)
18-22: LGTM! Room lookup correctly migrated to federation model.The change from
MatrixBridgedRoomto directly queryingRoomswithfederation.mridaligns with the broader federation refactor, eliminating the bridging layer.
25-29: LGTM! User lookup correctly uses federation identifier.The migration to query users by
federation.muiis consistent with the new federated user model.
34-35: LGTM! API calls updated to use room._id consistently.The update to use
room._idinstead ofroom.ridproperly aligns with the new room lookup structure.
38-45: LGTM! Kick logic properly handles federated users.The kicker user lookup and subsequent removal with proper fallback to a default user when the kicker is not found is well-implemented.
packages/models/src/models/Rooms.ts (2)
670-672: LGTM! Federation metadata properly structured.The updated
setAsFederatedsignature correctly stores both the Matrix room ID (mrid) and origin server, supporting the federated room model.
2217-2220: Wrong return type - callers expect string array not number.ee/packages/federation-matrix/src/events/invite.ts (2)
9-13: LGTM! Room lookup properly uses federation identifier.The migration from
MatrixBridgedRoomto directRoomsmodel query withfederation.mridaligns with the federation refactor.
22-26: serverName parsing incorrectly drops port numbers.ee/packages/federation-matrix/src/api/_matrix/invite.ts (2)
208-210: Good defensive programming with user ID validation.The explicit check for
senderUserIdafter user creation/lookup prevents downstream issues.
214-216: LGTM! Room lookup correctly uses federation identifier.The migration to query rooms by
federation.mridis consistent with the new federated room model.packages/core-services/src/types/IFederationMatrixService.ts (4)
18-18: LGTM! Return type properly reflects Matrix API response.The updated return type
Promise<{ room_id: string; event_id: string } | undefined>properly captures the Matrix homeserver response, enabling callers to track created rooms.
22-22: Verify all callers handle the new matrixRoomId parameter.
27-30: API improvements align with federation-native model.The changes to accept
IRoomNativeFederatedand fullIUserobjects instead of simple IDs provide richer context and align with the federation-native architecture.
32-36: LGTM! Consistent use of IRoomNativeFederated.The update to use
IRoomNativeFederatedforaddUserRoleRoomScopedmaintains consistency with other method signatures.ee/packages/federation-matrix/src/FederationMatrix.ts (7)
689-691: LGTM: reaction sender resolutionUsing native MUI when available and falling back to
@username:serveris correct.Consider whether emojis outside shortname form are already normalized upstream.
829-847: LGTM: updateMessage uses federated room ID and sender MUICorrectly parses and updates the formatted message, scoped to room’s
mrid.
857-871: LGTM: updateRoomName resolves room by rid and uses sender MUIValidation and operation look consistent with the new native federated model.
880-883: LGTM: updateRoomTopic usessetRoomTopic(roomId, userMui, topic)Matches API expectations.
900-919: LGTM with a note: role-to-power mappingOwner=100, Moderator=50, User=0 is reasonable. Leader is explicitly unsupported.
Confirm that upstream Matrix power-level defaults for “user” meet your moderation needs.
217-226: Normalize invitees to valid Matrix IDs and skip locals when creating roomsInviting raw usernames will fail for externals and create inconsistent IDs. Normalize to
@user:domainand avoid inviting local users (handled by RC join flow).- for await (const member of members) { + for await (const member of members) { if (member === owner.username) { continue; } - - // We are not generating bridged users for members outside of the current workspace - // They will be created when the invite is accepted - - await this.homeserverServices.invite.inviteUserToRoom(member, matrixRoomResult.room_id, matrixUserId); + // Only invite external users; local users are joined internally + const invitee = member.includes(':') + ? (member.startsWith('@') ? member : `@${member}`) + : null; + if (!invitee) { + continue; + } + await this.homeserverServices.invite.inviteUserToRoom(invitee, matrixRoomResult.room_id, matrixUserId); }
368-391: Critical: DM bootstrap stores local Mongo_idintofederation.muiThe lookup uses
{'federation.mui': memberId}and inserts withmui: memberId.memberIdis the RC user_id, not a Matrix user ID. This corrupts federation data.- if (memberId) { - const existingMemberMatrixUserId = await Users.findOne({ 'federation.mui': memberId }); - if (!existingMemberMatrixUserId) { - const newUser = { - username: memberId, - name: memberId, + if (memberId) { + const originDomain = memberMatrixUserId.split(':').pop(); + const existingByMui = await Users.findOne( + { 'federation.mui': memberMatrixUserId }, + { projection: { _id: 1 } }, + ); + if (!existingByMui) { + const sanitizedUsername = memberMatrixUserId; + const newUser = { + username: sanitizedUsername, + name: sanitizedUsername.replace(/^@/, '').split(':')[0], type: 'user' as const, status: UserStatus.OFFLINE, active: true, roles: ['user'], requirePasswordChange: false, federated: true, federation: { version: 1, - mui: memberId, - origin: memberMatrixUserId.split(':').pop(), + mui: memberMatrixUserId, + origin: originDomain, }, createdAt: new Date(), _updatedAt: new Date(), }; await Users.insertOne(newUser); } }
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## chore/federation-backup #37035 +/- ##
========================================================
Coverage 67.33% 67.34%
========================================================
Files 3342 3342
Lines 113379 113374 -5
Branches 20657 20652 -5
========================================================
+ Hits 76346 76347 +1
- Misses 34427 34429 +2
+ Partials 2606 2598 -8
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/models/src/models/Rooms.ts (1)
670-672: Persisting mrid/origin is good; add supporting indexes to avoid scans.The write looks fine. For scale, add sparse indexes on federation.origin (and likely federation.mrid) to support distinct/lookup patterns introduced by the new API.
Add to modelIndexes():
// Add near other federation-related indexes { key: { 'federation.origin': 1 }, sparse: true }, { key: { 'federation.mrid': 1 }, sparse: true },apps/meteor/ee/server/hooks/federation/index.ts (2)
57-67: Prefer server logger over console.Route errors to the platform logger (e.g., SystemLogger) for consistency and observability.
106-107: Trim trailing space in callback id.Trailing space makes handler names brittle.
Apply:
- 'native-federation-on-add-users-to-room ', + 'native-federation-on-add-users-to-room',
📜 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 (3)
apps/meteor/ee/server/hooks/federation/index.ts(8 hunks)packages/model-typings/src/models/IRoomsModel.ts(2 hunks)packages/models/src/models/Rooms.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/model-typings/src/models/IRoomsModel.ts
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: rodrigok
PR: RocketChat/Rocket.Chat#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.
📚 Learning: 2025-09-19T15:15:04.642Z
Learnt from: rodrigok
PR: RocketChat/Rocket.Chat#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:
apps/meteor/ee/server/hooks/federation/index.ts
🧬 Code graph analysis (2)
packages/models/src/models/Rooms.ts (1)
packages/core-typings/src/IRoom.ts (1)
IRoom(21-95)
apps/meteor/ee/server/hooks/federation/index.ts (3)
ee/packages/federation-matrix/src/FederationMatrix.ts (1)
FederationMatrix(46-938)apps/meteor/server/services/room/hooks/BeforeFederationActions.ts (1)
FederationActions(6-30)packages/core-typings/src/IUser.ts (1)
isUserNativeFederated(275-276)
⏰ 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 (10)
packages/models/src/models/Rooms.ts (1)
2217-2220: Implement TODO: return distinct external origins; current stub breaks stats.Callers need the list of distinct federation origins; implement via distinct with optional exclusion.
Apply this diff:
- async countDistinctFederationRoomsExcluding(_serverNames: string[] = []): Promise<string[]> { - // TODO implement - return []; - } + async countDistinctFederationRoomsExcluding(_serverNames: string[] = []): Promise<string[]> { + const exclude = (_serverNames || []).filter((s) => !!s); + const query: Filter<IRoom> = { + federated: true, + 'federation.origin': { + $exists: true, + ...(exclude.length ? { $nin: exclude } : {}), + }, + }; + const origins = await this.col.distinct<string>('federation.origin', query); + return origins.filter((o): o is string => typeof o === 'string' && o.length > 0); + }apps/meteor/ee/server/hooks/federation/index.ts (9)
79-81: LGTM: external users’ deletions are skipped.Consistent with current policy.
166-166: LGTM: updated kickUser signature (room, removedUser, userWhoRemoved).Matches service API.
175-178: LGTM: updateRoomName uses rid and user.Signature aligns with service.
186-189: LGTM: updateRoomTopic uses room object.Matches current API.
196-204: Guard missing eventId and catch errors on edit path.Avoid throwing on non‑federated edits and don’t block local updates.
Apply:
async (message: IMessage, { room }) => { if (FederationActions.shouldPerformFederationAction(room)) { if (!isEditedMessage(message)) { return; } - - await FederationMatrix.updateMessage(room, message); + if (!message.federation?.eventId) { + return; + } + try { + await FederationMatrix.updateMessage(room, message); + } catch (error) { + console.error('[updateMessage] Failed to federate edit:', error); + } } },
213-213: LGTM: addUserRoleRoomScoped call shape is correct.(room, fromUserId, userId, role) matches service contract.
22-33: Remove redundant Rooms.setAsFederated and avoid brittle server parsing.
FederationMatrix.createRoomalready persists{ mrid, origin }; this second write is redundant and parses the server incorrectly withsplit(':')[1].Apply:
- const { room_id } = result; - - const fromServer = room_id.split(':')[1]; - - await Rooms.setAsFederated(room._id, { - mrid: room_id, - origin: fromServer, - }); + // FederationMatrix.createRoom already persists mrid/origin via Rooms.setAsFederated.#!/bin/bash # Verify duplication inside FederationMatrix rg -nC3 'createRoom\s*\(' ee/packages/federation-matrix/src/FederationMatrix.ts rg -nC3 'Rooms\.setAsFederated\(' ee/packages/federation-matrix/src/FederationMatrix.ts
36-41: Don’t parse Matrix server with split(':')[1]; preserve ports/IPv6.Use slice from the first colon onward.
Apply:
- const fromServer = federatedRoomId.split(':')[1]; + const colonIndex = federatedRoomId.indexOf(':'); + const fromServer = colonIndex === -1 ? federatedRoomId : federatedRoomId.slice(colonIndex + 1);
2-3: No action required — isUserNativeFederated is used (apps/meteor/ee/server/hooks/federation/index.ts:53).Keep the import; if you remove the isUserNativeFederated guard later, drop the import to avoid an unused symbol.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/meteor/ee/server/lib/engagementDashboard/messages.ts (1)
29-31: Use deletion timestamp (_deletedAt) when recording deleted messagesmessage.ts is the creation time; deleted records get _deletedAt (BaseRaw sets _deletedAt = new Date()). Record deletions by convertDateToInt(message._deletedAt ?? new Date()) so the event is attributed to the actual deletion day.
Location: apps/meteor/ee/server/lib/engagementDashboard/messages.ts:28-31
Suggested code fragment:
date: convertDateToInt(message._deletedAt ?? new Date()),ee/packages/federation-matrix/src/FederationMatrix.ts (1)
107-107: Bug: enableThumbnails is always true
=== 'true' || trueforces true and ignores the env flag.- enableThumbnails: process.env.MEDIA_ENABLE_THUMBNAILS === 'true' || true, + enableThumbnails: process.env.MEDIA_ENABLE_THUMBNAILS + ? process.env.MEDIA_ENABLE_THUMBNAILS === 'true' + : true,packages/core-services/src/types/IFederationMatrixService.ts (1)
18-38: Fix interface/impl mismatches (compile-time blockers)Several method signatures don’t match the implementation (and intended model). Update to native types and correct params.
- createRoom(room: IRoomFederated, owner: IUser, members: string[]): Promise<{ room_id: string; event_id: string }>; + createRoom(room: IRoom, owner: IUser, members: string[]): Promise<{ room_id: string; event_id: string }>; @@ - createDirectMessageRoom(room: IRoomFederated, members: IUser[], creatorId: IUser['_id']): Promise<void>; + createDirectMessageRoom(room: IRoom, members: (IUser | string)[], creatorId: IUser['_id']): Promise<void>; @@ - sendMessage(message: IMessage, room: IRoomFederated, user: IUser): Promise<void>; + sendMessage(message: IMessage, room: IRoomNativeFederated, user: IUser): Promise<void>; @@ - getEventById(eventId: string): Promise<any | null>; + getEventById(eventId: EventID): Promise<any | null>; @@ - leaveRoom(rid: IRoomFederated['_id'], user: IUser): Promise<void>; + leaveRoom(rid: string, user: IUser): Promise<void>; @@ - inviteUsersToRoom(room: IRoomFederated, usersUserName: string[], inviter: Pick<IUser, '_id' | 'username'>): Promise<void>; + inviteUsersToRoom(room: IRoomNativeFederated, usersUserName: string[], inviter: IUser): Promise<void>;
🧹 Nitpick comments (4)
apps/meteor/ee/server/lib/engagementDashboard/messages.ts (2)
24-26: Redundant null-check forroomwith non-optional type.Since
roomis typed as required,!roomis unnecessary. Either make it optional again or drop the check. Suggest removing the null-check for consistency.- if (!room || !roomTypesToShow.includes(room.t)) { + if (!roomTypesToShow.includes(room.t)) { return message; }
8-20: Optional: unify parameter shapes between sent/deleted handlers.
handleMessagesSentaccepts{ room?: IRoom }whilehandleMessagesDeletedrequires{ room: IRoom }. Consider aligning both to the same contract to reduce API friction.Also applies to: 22-34
ee/packages/federation-matrix/src/FederationMatrix.ts (2)
314-314: Use creator’s MUI when federatedEnsures correct sender when the creator is native federated.
- const actualMatrixUserId = `@${creator.username}:${this.serverName}`; + const actualMatrixUserId = isUserNativeFederated(creator) + ? creator.federation.mui + : `@${creator.username}:${this.serverName}`;
646-661: Use inviter’s MUI when federatedBuild inviter ID with federation awareness.
- const inviterUserId = `@${inviter.username}:${this.serverName}`; + const inviterUserId = isUserNativeFederated(inviter) + ? inviter.federation.mui + : `@${inviter.username}:${this.serverName}`; @@ - await this.homeserverServices.invite.inviteUserToRoom(username, room.federation.mrid, inviterUserId); + await this.homeserverServices.invite.inviteUserToRoom(username, room.federation.mrid, inviterUserId);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
apps/meteor/app/lib/server/functions/deleteMessage.ts(1 hunks)apps/meteor/ee/server/hooks/federation/index.ts(7 hunks)apps/meteor/ee/server/lib/engagementDashboard/messages.ts(1 hunks)apps/meteor/lib/callbacks.ts(2 hunks)ee/packages/federation-matrix/src/FederationMatrix.ts(22 hunks)packages/core-services/src/types/IFederationMatrixService.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/meteor/lib/callbacks.ts
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: rodrigok
PR: RocketChat/Rocket.Chat#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.
📚 Learning: 2025-09-19T15:15:04.642Z
Learnt from: rodrigok
PR: RocketChat/Rocket.Chat#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/src/types/IFederationMatrixService.tsapps/meteor/ee/server/hooks/federation/index.tsee/packages/federation-matrix/src/FederationMatrix.ts
🧬 Code graph analysis (5)
packages/core-services/src/types/IFederationMatrixService.ts (3)
packages/core-typings/src/IRoom.ts (2)
IRoomFederated(109-112)IRoomNativeFederated(114-120)packages/core-typings/src/IUser.ts (1)
IUser(186-255)packages/core-typings/src/IMessage/IMessage.ts (1)
IMessage(138-238)
apps/meteor/ee/server/hooks/federation/index.ts (3)
apps/meteor/server/services/room/hooks/BeforeFederationActions.ts (1)
FederationActions(6-30)packages/core-typings/src/IUser.ts (1)
isUserNativeFederated(275-276)ee/packages/federation-matrix/src/FederationMatrix.ts (1)
FederationMatrix(46-941)
apps/meteor/app/lib/server/functions/deleteMessage.ts (1)
apps/meteor/lib/callbacks.ts (1)
callbacks(251-259)
apps/meteor/ee/server/lib/engagementDashboard/messages.ts (2)
packages/core-typings/src/IMessage/IMessage.ts (1)
IMessage(138-238)packages/core-typings/src/IRoom.ts (1)
IRoom(21-95)
ee/packages/federation-matrix/src/FederationMatrix.ts (8)
packages/models/src/index.ts (2)
Users(205-205)Rooms(196-196)packages/core-typings/src/IUser.ts (2)
isUserNativeFederated(275-276)IUser(186-255)ee/packages/federation-matrix/src/events/room.ts (1)
room(6-59)packages/core-typings/src/IRoom.ts (3)
IRoom(21-95)IRoomNativeFederated(114-120)isRoomNativeFederated(124-125)ee/packages/federation-matrix/src/events/member.ts (1)
member(9-51)ee/packages/federation-matrix/src/events/message.ts (1)
message(123-293)ee/packages/federation-matrix/src/events/reaction.ts (1)
reaction(10-93)ee/packages/federation-matrix/src/helpers/message.parsers.ts (1)
toExternalMessageFormat(201-212)
⏰ 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 (10)
apps/meteor/ee/server/lib/engagementDashboard/messages.ts (1)
22-27: Signature requires{ room }— verification complete.callbacks.run('afterDeleteMessage', deletedMsg, { room, user }) (apps/meteor/app/lib/server/functions/deleteMessage.ts:96) supplies the object payload and handleMessagesDeleted is registered as the callback (apps/meteor/ee/server/lib/engagementDashboard/startup.ts:12). No direct call sites requiring migration were found.
apps/meteor/app/lib/server/functions/deleteMessage.ts (1)
96-96: Add null-safe message fallback when running afterDeleteMessage; verify listenersUse a message fallback so callbacks never receive a null first arg.
- await callbacks.run('afterDeleteMessage', deletedMsg, { room, user }); + await callbacks.run('afterDeleteMessage', deletedMsg ?? message, { room, user });Apply in: apps/meteor/app/lib/server/functions/deleteMessage.ts (around line 96)
Verify/inspect these listeners for compatibility:
- apps/meteor/app/search/server/events/index.ts
- apps/meteor/app/federation/server/hooks/afterDeleteMessage.js
- apps/meteor/app/slackbridge/server/RocketAdapter.js
- apps/meteor/app/discussion/server/hooks/propagateDiscussionMetadata.ts
- apps/meteor/ee/server/lib/engagementDashboard/startup.ts
- apps/meteor/ee/server/hooks/federation/index.ts
ee/packages/federation-matrix/src/FederationMatrix.ts (4)
254-286: Prevent duplicate user mappings and fix remote domain parsing
- Only checking by username risks duplicates; also set username to MUI for consistency.
split(':')[1]drops ports/IPv6. Use slice after the first colon.- const existingUser = await Users.findOneByUsername(username); - if (existingUser) { - // TODO review: DM - // const existingBridge = await MatrixBridgedUser.getExternalUserIdByLocalUserId(existingUser._id); // TODO review: DM - // if (!existingBridge) { - // const remoteDomain = externalUserId.split(':')[1] || this.serverName; - // await MatrixBridgedUser.createOrUpdateByLocalId(existingUser._id, externalUserId, true, remoteDomain); - // } - continue; - } - - // TODO: there is not need to check if the username includes ':' or '@', we should just use the username as is - const externalUserId = username.includes(':') ? `@${username}` : `@${username}:${this.serverName}`; + const externalUserId = username.includes(':') + ? (username.startsWith('@') ? username : `@${username}`) + : `@${username}:${this.serverName}`; + const [existingByMui, existingByUsername] = await Promise.all([ + Users.findOne({ 'federation.mui': externalUserId }, { projection: { _id: 1 } }), + Users.findOneByUsername(username, { projection: { _id: 1 } }), + ]); + if (existingByMui || existingByUsername) { + continue; + } this.logger.debug('Creating federated user locally', { externalUserId, username }); - const remoteDomain = externalUserId.split(':')[1]; + const remoteDomain = externalUserId.slice(externalUserId.indexOf(':') + 1); const localName = username.split(':')[0]?.replace('@', '') || username; const newUser = { - username, + username: externalUserId, name: localName, type: 'user' as const, status: UserStatus.OFFLINE, active: true, roles: ['user'], requirePasswordChange: false, federated: true, federation: { version: 1, - mui: externalUserId, - origin: remoteDomain, + mui: externalUserId, + origin: remoteDomain, }, createdAt: new Date(), _updatedAt: new Date(), };
633-636: Redaction uses actor UID — good fixPassing the deleter’s Matrix ID resolves the prior redactor/author confusion.
217-226: Normalize/skip local invitees when creating roomsInviting raw usernames (no '@' or domain) will fail; skip local users or normalize to Matrix IDs.
- for await (const member of members) { + for (const member of members) { if (member === owner.username) { continue; } - - // We are not generating bridged users for members outside of the current workspace - // They will be created when the invite is accepted - - await this.homeserverServices.invite.inviteUserToRoom(member, matrixRoomResult.room_id, matrixUserId); + // Invite only external users; local users are handled by RC + const invitee = member.includes(':') ? (member.startsWith('@') ? member : `@${member}`) : null; + if (!invitee) { + continue; + } + await this.homeserverServices.invite.inviteUserToRoom(invitee, matrixRoomResult.room_id, matrixUserId); }
368-391: Critical: DM bootstrap uses local _id as MUI
memberIdis a local Mongo _id; checks/inserts should be keyed by the Matrix ID (memberMatrixUserId).- if (memberId) { - const existingMemberMatrixUserId = await Users.findOne({ 'federation.mui': memberId }); - if (!existingMemberMatrixUserId) { - const newUser = { - username: memberId, - name: memberId, + if (memberId) { + const originDomain = memberMatrixUserId.slice(memberMatrixUserId.indexOf(':') + 1); + const existingByMui = await Users.findOne( + { 'federation.mui': memberMatrixUserId }, + { projection: { _id: 1 } }, + ); + if (!existingByMui) { + const sanitizedUsername = + (typeof otherMember !== 'string' && otherMember.username) + ? (otherMember.username.includes(':') ? (otherMember.username.startsWith('@') ? otherMember.username : `@${otherMember.username}`) : memberMatrixUserId) + : memberMatrixUserId; + const newUser = { + username: sanitizedUsername, + name: sanitizedUsername.replace(/^@/, '').split(':')[0], type: 'user' as const, status: UserStatus.OFFLINE, active: true, roles: ['user'], requirePasswordChange: false, federated: true, federation: { version: 1, - mui: memberId, - origin: memberMatrixUserId.split(':').pop(), + mui: memberMatrixUserId, + origin: originDomain, }, createdAt: new Date(), _updatedAt: new Date(), }; await Users.insertOne(newUser); } }apps/meteor/ee/server/hooks/federation/index.ts (4)
186-196: Guard missing eventId and catch errors on edit pathPrevents exceptions from breaking local edits.
- if (FederationActions.shouldPerformFederationAction(room)) { - if (!isEditedMessage(message)) { - return; - } - - await FederationMatrix.updateMessage(room, message); - } + if (FederationActions.shouldPerformFederationAction(room)) { + if (!isEditedMessage(message)) { + return; + } + if (!message.federation?.eventId) { + return; + } + try { + await FederationMatrix.updateMessage(room, message); + } catch (err) { + console.error('[updateMessage] Native Federation failed', { rid: room?._id, mid: message?._id, err }); + } + }
24-29: Preserve ports/IPv6 when parsing the server from a Matrix ID
split(':')[1]drops “:port” and breaks IPv6. Use slice from the first colon.- const fromServer = federatedRoomId.split(':')[1]; + const colonIndex = federatedRoomId.indexOf(':'); + const fromServer = colonIndex === -1 ? '' : federatedRoomId.slice(colonIndex + 1);
41-44: Don’t block local users from federating messages
sendMessagehandles non‑federated users; this guard drops valid sends.- if (!isUserNativeFederated(user)) { - return; - }
77-79: Don’t let federation delete failures block local deletesWrap in try/catch.
- await FederationMatrix.deleteMessage(room.federation.mrid, message, user.federation.mui); + try { + await FederationMatrix.deleteMessage(room.federation.mrid, message, user.federation.mui); + } catch (err) { + console.error('[deleteMessage] Native Federation failed', { rid: room?._id, mid: message?._id, err }); + }
| @@ -1,4 +1,4 @@ | |||
| import type { AtLeast, IMessage, IRoomFederated, IUser } from '@rocket.chat/core-typings'; | |||
| import type { IMessage, IRoomFederated, IRoomNativeFederated, IUser } from '@rocket.chat/core-typings'; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Align imports with actual usage (add IRoom, EventID)
Required by updated signatures below.
-import type { IMessage, IRoomFederated, IRoomNativeFederated, IUser } from '@rocket.chat/core-typings';
+import type { IMessage, IRoom, IRoomFederated, IRoomNativeFederated, IUser } from '@rocket.chat/core-typings';
+import type { EventID } from '@hs/room';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import type { IMessage, IRoomFederated, IRoomNativeFederated, IUser } from '@rocket.chat/core-typings'; | |
| import type { IMessage, IRoom, IRoomFederated, IRoomNativeFederated, IUser } from '@rocket.chat/core-typings'; | |
| import type { EventID } from '@hs/room'; |
🤖 Prompt for AI Agents
packages/core-services/src/types/IFederationMatrixService.ts around lines 1 to
1: the current import line only brings in IMessage, IRoomFederated,
IRoomNativeFederated, and IUser but the updated signatures require IRoom and
EventID; update the named import from '@rocket.chat/core-typings' to also
include IRoom and EventID so the file references resolve correctly.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/meteor/app/lib/server/methods/sendMessage.ts (1)
58-58: Avoid full user document load on hot pathFetch only needed fields to reduce I/O and GC pressure during message send.
- const user = await Users.findOneById(uid); + const user = await Users.findOneById(uid, { projection: { username: 1, type: 1, language: 1 } });apps/meteor/ee/server/hooks/federation/index.ts (1)
41-51: Prefer structured logging over console.errorUse SystemLogger for consistency and context.
- console.error('[sendMessage] Failed to send message to Native Federation:', error); + SystemLogger.error({ msg: '[sendMessage] Native Federation send failed', err: error });
📜 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 (4)
apps/meteor/app/lib/server/functions/createDirectRoom.ts(2 hunks)apps/meteor/app/lib/server/methods/sendMessage.ts(1 hunks)apps/meteor/ee/server/hooks/federation/index.ts(8 hunks)apps/meteor/lib/callbacks.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: rodrigok
PR: RocketChat/Rocket.Chat#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.
📚 Learning: 2025-09-19T15:15:04.642Z
Learnt from: rodrigok
PR: RocketChat/Rocket.Chat#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:
apps/meteor/ee/server/hooks/federation/index.ts
🧬 Code graph analysis (3)
apps/meteor/app/lib/server/functions/createDirectRoom.ts (1)
apps/meteor/lib/callbacks.ts (1)
callbacks(251-259)
apps/meteor/lib/callbacks.ts (4)
ee/packages/federation-matrix/src/events/message.ts (1)
message(123-293)packages/core-typings/src/IMessage/IMessage.ts (1)
IMessage(138-238)packages/core-typings/src/IRoom.ts (1)
IRoom(21-95)packages/core-typings/src/IUser.ts (1)
IUser(186-255)
apps/meteor/ee/server/hooks/federation/index.ts (3)
apps/meteor/server/services/room/hooks/BeforeFederationActions.ts (1)
FederationActions(6-30)ee/packages/federation-matrix/src/FederationMatrix.ts (1)
FederationMatrix(46-941)packages/core-typings/src/IUser.ts (2)
isUserNativeFederated(275-276)IUser(186-255)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (14)
apps/meteor/app/lib/server/functions/createDirectRoom.ts (2)
49-50: API surface: addition looks goodOptional federatedRoomId on options is fine and backward‑compatible.
183-187: Propagating mrid via callback is correct. callbacks type includes mrid?: string and the EE federation hook reads params.mrid (apps/meteor/lib/callbacks.ts:67; apps/meteor/ee/server/hooks/federation/index.ts:219–221).apps/meteor/ee/server/hooks/federation/index.ts (8)
60-75: Don’t let federation delete failures block local deletes; add try/catchMirror send path resilience; keep local delete successful even if remote fails.
- if (FederationActions.shouldPerformFederationAction(room)) { - await FederationMatrix.deleteMessage(room.federation.mrid, message, user.federation.mui); - } + if (FederationActions.shouldPerformFederationAction(room)) { + try { + await FederationMatrix.deleteMessage(room.federation.mrid, message, user.federation.mui); + } catch (err) { + SystemLogger.error({ msg: '[deleteMessage] Native Federation deletion failed', rid: room?._id, mid: message?._id, err }); + } + }
190-191: Guard edited-message federation and handle errorsSkip when no eventId; catch errors to avoid breaking local edits.
- await FederationMatrix.updateMessage(room, message); + if (message.federation?.eventId) { + try { + await FederationMatrix.updateMessage(room, message); + } catch (err) { + SystemLogger.error({ msg: '[updateMessage] Native Federation edit failed', rid: room?._id, mid: message?._id, err }); + } + }
153-154: API migration to object-based kickUser looks goodPassing full room and users matches new FederationMatrix signature.
162-165: Name change flow migration LGTMUsing user object and rid matches updated API.
173-176: Topic change flow migration LGTMObject-based params match new FederationMatrix.updateRoomTopic signature.
16-31: Incorrect — createRoom already persists mrid/origin; ignore duplicate-write suggestion.FederationMatrix.createRoom calls Rooms.setAsFederated(room._id, { mrid: matrixRoomResult.room_id, origin: this.serverName }) inside ee/packages/federation-matrix/src/FederationMatrix.ts (around lines ~211–216 and ~397–403), so the branch that calls createRoom does not need an additional setAsFederated; the else branch remains required when options.federatedRoomId is provided.
Likely an incorrect or invalid review comment.
24-30: Bug: origin parsing drops host/IPv6 and keeps only first segmentsplit(':')[1] breaks IPv6 and removes :port. Preserve everything after the first colon.
- const fromServer = federatedRoomId.split(':')[1]; + const colon = federatedRoomId.indexOf(':'); + const fromServer = colon >= 0 ? federatedRoomId.slice(colon + 1) : '';
221-226: Bug: origin computed as just port with split(':').pop()For mrid like "!abc:example.com:8448", .pop() yields "8448". Keep the full host[:port]/IPv6 by slicing after the first colon.
- await Rooms.setAsFederated(room._id, { - mrid: params.mrid, - origin: params.mrid.split(':').pop()!, - }); + const colon = params.mrid.indexOf(':'); + const origin = colon >= 0 ? params.mrid.slice(colon + 1) : ''; + await Rooms.setAsFederated(room._id, { mrid: params.mrid, origin });apps/meteor/lib/callbacks.ts (4)
51-51: Keep userId as optional for ecosystem parityAdd userId?: string to avoid breaking existing listeners that still read params.userId.
- 'afterRoomNameChange': (params: { room: IRoom; name: string; oldName: string; user: IUser }) => void; + 'afterRoomNameChange': (params: { room: IRoom; name: string; oldName: string; user: IUser; userId?: IUser['_id'] }) => void;
208-209: Match topic-change payload parity: add optional userIdMaintain optional userId for smooth migration like name-change.
- 'afterRoomTopicChange': (params: undefined, { room, topic, user }: { room: IRoom; topic: string; user: IUser }) => void; + 'afterRoomTopicChange': ( + params: undefined, + { room, topic, user, userId }: { room: IRoom; topic: string; user: IUser; userId?: IUser['_id'] } + ) => void;
67-67: afterCreateDirectRoom typing update — LGTMmrid?: string is present in apps/meteor/lib/callbacks.ts:67 and matches producers/consumers across the PR.
44-44: Ignore type-widening — keep the current afterDeleteMessage signaturecallbacks.run is invoked with callbacks.run('afterDeleteMessage', message, { room, user }) and registered handlers in the repo accept either a single message or the params object; I found registrations in apps/meteor/app/search/server/events/index.ts, apps/meteor/app/slackbridge/server/RocketAdapter.js, apps/meteor/ee/server/lib/engagementDashboard/startup.ts and the run call at apps/meteor/app/lib/server/functions/deleteMessage.ts:96. No handlers use (message, room), so widening the type is unnecessary.
Likely an incorrect or invalid review comment.
ggazzo
force-pushed
the
remove-bridged-models
branch
2 times, most recently
from
2db3419 to
805dd20
Compare
…indOneByUsername query
…ver services are unavailable
ggazzo
force-pushed
the
remove-bridged-models
branch
from
805dd20 to
ce2a254
Compare
Co-authored-by: Guilherme Gazzo <guilherme@gazzo.xyz>
Co-authored-by: Guilherme Gazzo <guilherme@gazzo.xyz>
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores