fix: fixes reviews of chore/federation-backup #37016
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 |
|
|
Warning Rate limit exceeded@ggazzo has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 1 minutes and 2 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (13)
WalkthroughRenames a federation callback, adjusts callback parameter names, adds federation.version to messages, updates invite-to-room username handling, refactors federated user creation to a helper, revises media/file message handling to attachments with optional file metadata, tightens types (AtLeast sender, IUser in interfaces), replaces AJV validators with precompiled schemas, and simplifies federation-service config. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Matrix as Matrix Homeserver
participant FM as FederationMatrix
participant IDS as Identifiers Helper
participant MS as Message Service
participant Media as MatrixMediaService
Note over Matrix,FM: Incoming event (message or media)
Matrix->>FM: send event (content, sender, room)
FM->>IDS: saveLocalUserForExternalUserId(senderId, domain)
IDS-->>FM: { userId }
alt Media message
FM->>Media: downloadAndStoreRemoteFile(mxc://..., { name, size?, type?, ... })
Media-->>FM: fileURL
FM-->>MS: saveMessageFromFederation({ attachments[], federation:{eventId, version:1} })
else Text/edit/quote
FM-->>MS: saveMessageFromFederation({ msg, federation:{eventId, version:1} })
end
MS-->>FM: ack
FM-->>Matrix: 200 OK
sequenceDiagram
autonumber
actor RC as Rocket.Chat
participant Hook as Federation Hook
participant FM as FederationMatrix
participant IDS as Identifiers Helper
Note over Hook,FM: Add users to room
RC->>Hook: on-add-users-to-room(room, users[])
Hook->>Hook: map users -> usernames, filter nulls
Hook-->>FM: inviteUsersToRoom(inviter: IUser, room, usernames[])
FM->>IDS: ensure/save local users for external IDs (as needed)
FM-->>RC: invitations sent
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
Comment |
rodrigok
force-pushed
the
chore/federation-backup-fixes
branch
5 times, most recently
from
5468908 to
b0c8d92
Compare
rodrigok
force-pushed
the
chore/federation-backup-fixes
branch
2 times, most recently
from
543ebfc to
c1c5624
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## chore/federation-backup #37016 +/- ##
==========================================================
Coverage ? 67.33%
==========================================================
Files ? 3342
Lines ? 113374
Branches ? 20651
==========================================================
Hits ? 76343
Misses ? 34434
Partials ? 2597
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
rodrigok
force-pushed
the
chore/federation-backup-fixes
branch
from
c1c5624 to
f8c9d2e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
ee/packages/federation-matrix/src/api/_matrix/rooms.ts (3)
186-197: Don't drop filter.room_types; implement minimal support and combine with generic_search_term.Silently ignoring room_types changes client-visible behavior. Support the spec’s common cases (null => regular rooms; 'm.space' => spaces) and apply both filters conjunctively.
Apply this diff:
- chunk: publicRooms - .filter((r) => { - if (filter.generic_search_term) { - return r.name.toLowerCase().includes(filter.generic_search_term.toLowerCase()); - } - - // Today only one room type is supported (https://spec.matrix.org/v1.15/client-server-api/#types) - // TODO: https://rocketchat.atlassian.net/browse/FDR-152 -> Implement logic to handle custom room types - // if (filter.room_types) { - // } - - return true; - }) + chunk: publicRooms + .filter((r) => { + const term = filter.generic_search_term?.trim().toLowerCase(); + const matchesSearch = term ? (r.name?.toLowerCase() ?? '').includes(term) : true; + + const types = Array.isArray(filter.room_types) ? filter.room_types : []; + if (types.length === 0) { + return matchesSearch; + } + + // Minimal support: null => regular rooms (no room_type), 'm.space' => spaces. + const wantsRegular = types.some((t) => t == null); + const wantsSpace = types.some((t) => t === 'm.space'); + const roomType = r.room_type ?? null; + const matchesType = + (wantsRegular && roomType == null) || + (wantsSpace && roomType === 'm.space'); + + // If only unsupported types were requested, do not return the room. + return matchesSearch && (wantsRegular || wantsSpace ? matchesType : false); + })
90-99: POST schema: include_all_networks must be boolean (string here will 400 valid clients).Clients typically send a boolean for include_all_networks on POST; declaring it as string causes validation failures when present.
- include_all_networks: { - type: 'string', + include_all_networks: { + type: 'boolean', description: 'Include all networks (ignored)', nullable: true, },
5-18: Align GET publicRooms schema with the Matrix federation spec
- Remove include_all_networks from PublicRoomsQuerySchema.required (keep it as type: "boolean"); GET accepts include_all_networks but it is not mandatory. File: ee/packages/federation-matrix/src/api/_matrix/rooms.ts.
- Ensure the handler honours limit (treat as integer if supported), validate filter.room_types as an array of room-type strings (allow null to request default-type rooms), and return pagination fields (next_batch, prev_batch) and total_room_count_estimate when available.
ee/packages/federation-matrix/src/helpers/identifiers.ts (1)
16-24: Bug: wrong mapping direction — use a reverse lookup to resolve local user by external IDMatrixBridgedUser.getExternalUserIdByLocalUserId expects a local user ID; here an externalUserId is passed, so the lookup will fail. In ee/packages/federation-matrix/src/helpers/identifiers.ts (getLocalUserForExternalUserId) replace the call with a reverse lookup that returns the local user id for the given external ID — e.g. MatrixBridgedUser.getLocalUserIdByExternalUserId(externalUserId) if available, or add/implement a helper to query the mapping collection and then call Users.findOneById(localUserId).
ee/packages/federation-matrix/src/FederationMatrix.ts (1)
102-102: Fix always-true thumbnail flag (env var ignored).
process.env.MEDIA_ENABLE_THUMBNAILS === 'true' || truealways evaluates totrue. Honor the env var.Apply:
- enableThumbnails: process.env.MEDIA_ENABLE_THUMBNAILS === 'true' || true, + enableThumbnails: + typeof process.env.MEDIA_ENABLE_THUMBNAILS === 'string' + ? ['1', 'true', 'yes', 'on'].includes(process.env.MEDIA_ENABLE_THUMBNAILS.toLowerCase()) + : true,ee/packages/federation-matrix/src/events/message.ts (1)
117-127: File message attachments are built but never used; message is saved with emptymsgand no file linkage.This likely drops the file in the UI. Include attachments (and file references) in the saved message.
Apply:
- let attachment: FileAttachmentProps = { + let attachment: FileAttachmentProps = { title: fileName, type: 'file', title_link: fileUrl, title_link_download: true, description: '', }; @@ - return { + return { fromId: user._id, rid: room._id, - msg: '', + msg: '', + attachments: [attachment], + files: [{ _id: fileRefId }], federation_event_id: eventId, tmid, };If
Message.saveMessageFromFederationexpects a different shape, adapt accordingly (happy to adjust once you confirm the exact contract).Also applies to: 140-175, 177-183
🧹 Nitpick comments (13)
packages/core-services/src/types/IFederationMatrixService.ts (1)
28-28: Narrow inviter to AtLeast<IUser, '_id' | 'username'> and keep implementation in syncChange the API to accept only the minimal user fields; update the implementing method to match and align room types if needed.
Apply:
- inviteUsersToRoom(room: IRoomFederated, usersUserName: string[], inviter: IUser): Promise<void>; + inviteUsersToRoom( + room: IRoomFederated, + usersUserName: string[], + inviter: AtLeast<IUser, '_id' | 'username'>, + ): Promise<void>;
- Update implementation: ee/packages/federation-matrix/src/FederationMatrix.ts (method at ~line 652) — change the inviter param to AtLeast<IUser, '_id' | 'username'> and consider changing room: IRoom → IRoomFederated to match the interface.
- Nit: rename usersUserName → usernames for clarity.
ee/packages/federation-matrix/src/api/_matrix/rooms.ts (2)
95-99: Honor the limit parameter to bound payloads.limit is defined but unused. Enforce it server-side to avoid large responses.
- chunk: publicRooms + chunk: publicRooms .filter((r) => { // ... }) .map((room) => ({ ...defaultObj, ...room, - })), + })) + .slice( + 0, + typeof body.limit === 'number' && body.limit > 0 ? Math.min(body.limit, 100) : undefined, + ),Also applies to: 185-202
139-146: DRY the defaultObj.defaultObj is duplicated in GET and POST. Extract a shared const to avoid drift.
+const DEFAULT_PUBLIC_ROOM_PROPS = { + join_rule: 'public', + guest_can_join: false, + world_readable: false, + avatar_url: '', +}; // ... -const defaultObj = { - join_rule: 'public', - guest_can_join: false, // trying to reduce required endpoint hits - world_readable: false, // ^^^ - avatar_url: '', // ?? don't have any yet -}; +const defaultObj = DEFAULT_PUBLIC_ROOM_PROPS;Also applies to: 172-177
ee/packages/federation-matrix/src/api/middlewares/isFederationEnabled.ts (1)
5-9: LGTM; consider reusing the shared util for consistency.Check is correct and returns 403 early. Optionally delegate to a central helper (e.g., throwIfFederationNotEnabled) to keep error text/status uniform across the stack.
If that helper exists in this branch, confirm we want middleware and service layers to share the same error semantics.
ee/packages/federation-matrix/src/helpers/identifiers.ts (1)
5-10: Username decisions TODO impacts UX.Storing full Matrix IDs (with @/:) as username can violate local username constraints and UI assumptions. Consider normalizing consistently (e.g., reuse convertExternalUserIdToInternalUsername) or store the MXID elsewhere and keep username compliant.
ee/packages/federation-matrix/src/services/MatrixMediaService.ts (1)
91-97: Making size/type optional is reasonable; guard required routing data.Upload.uploadFile typically needs a rid for proper association. Since metadata.roomId is optional, add a guard or defaulting strategy to avoid orphan uploads.
static async downloadAndStoreRemoteFile( mxcUri: string, metadata: { name: string; size?: number; type?: string; messageId?: string; roomId?: string; userId?: string; }, ): Promise<string> { try { + if (!metadata.roomId) { + throw new Error('roomId is required to associate the uploaded file to a room'); + }Confirm Upload.uploadFile’s current contract for details.rid; if optional, document the behavior here.
ee/packages/federation-matrix/src/api/_matrix/profiles.ts (1)
152-155: Remove no-longer-needed @ts-ignore/no-unused-vars guards.These validators are now referenced below; the suppressions can be dropped.
-// @ts-ignore -// eslint-disable-next-line @typescript-eslint/no-unused-vars const isMakeJoinParamsProps = ajv.compile(MakeJoinParamsSchema); ... -// @ts-ignore -// eslint-disable-next-line @typescript-eslint/no-unused-vars const isMakeJoinQueryProps = ajv.compile(MakeJoinQuerySchema); ... -// @ts-ignore -// eslint-disable-next-line @typescript-eslint/no-unused-vars const isMakeJoinResponseProps = ajv.compile(MakeJoinResponseSchema);Also applies to: 170-173, 256-259
apps/meteor/server/methods/addRoomModerator.ts (1)
42-44: Wrong error message target (“owners” vs “moderator”).This method sets moderator; the error mentions owners.
- throw new FederationMatrixInvalidConfigurationError('unable to change room owners'); + throw new FederationMatrixInvalidConfigurationError('unable to change room moderators');apps/meteor/server/services/federation/utils.ts (1)
13-22: Consider more flexible error message formatting.The error message construction could be more robust to handle edge cases where the cause message might already start with a lowercase letter or be empty.
Consider this more robust implementation:
export class FederationMatrixInvalidConfigurationError extends Error { constructor(cause?: string) { - // eslint-disable-next-line prefer-template - const message = 'Federation configuration is invalid' + (cause ? ',' + cause[0].toLowerCase() + cause.slice(1) : ''); + const message = cause + ? `Federation configuration is invalid, ${cause[0].toLowerCase()}${cause.slice(1)}` + : 'Federation configuration is invalid'; super(message);ee/packages/federation-matrix/src/FederationMatrix.ts (3)
236-251: Invite only external members; fix local member Matrix ID mapping.Inviting
memberas-is will fail for local users and can mis-invite. Build a proper Matrix ID and invite only external users.Apply:
- 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) { + try { + // Local users: ensure bridge mapping; do not invite on Matrix (bridge handles membership). + const localUser = await Users.findOneByUsername(member); + if (localUser) { + const localMatrixId = `@${member}:${this.serverName}`; + await MatrixBridgedUser.createOrUpdateByLocalId(localUser._id, localMatrixId, 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); + // External users: invite by their Matrix ID (must start with '@' and include domain). + const externalMatrixId = member.startsWith('@') ? member : `@${member}`; + await this.homeserverServices.invite.inviteUserToRoom(externalMatrixId, matrixRoomResult.room_id, matrixUserId);
421-421: Avoidfor awaitover arrays.
message.filesis a plain array. Usefor ... ofto avoid async-iterable semantics confusion.Apply:
- for await (const file of message.files) { + for (const file of message.files) {
254-254: Replaceconsole.logwith structured logger.Use
this.logger.errorconsistently.Apply:
- console.log(error); + this.logger.error(error);ee/packages/federation-matrix/src/events/message.ts (1)
191-191: Defensive cast forcontent.body.
content.body.toString()will throw ifbodyis undefined/null. Use a safe cast.Apply:
- const messageBody = content.body.toString(); + const messageBody = typeof content.body === 'string' ? content.body : String(content.body ?? '');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (20)
apps/meteor/ee/server/hooks/federation/index.ts(4 hunks)apps/meteor/lib/callbacks.ts(1 hunks)apps/meteor/server/methods/addRoomModerator.ts(2 hunks)apps/meteor/server/methods/addRoomOwner.ts(2 hunks)apps/meteor/server/services/federation/utils.ts(1 hunks)apps/meteor/server/services/messages/service.ts(1 hunks)apps/meteor/server/services/room/hooks/BeforeFederationActions.ts(2 hunks)ee/apps/federation-service/src/config.ts(0 hunks)ee/packages/federation-matrix/src/FederationMatrix.ts(8 hunks)ee/packages/federation-matrix/src/api/_matrix/invite.ts(4 hunks)ee/packages/federation-matrix/src/api/_matrix/profiles.ts(1 hunks)ee/packages/federation-matrix/src/api/_matrix/rooms.ts(1 hunks)ee/packages/federation-matrix/src/api/_matrix/send-join.ts(1 hunks)ee/packages/federation-matrix/src/api/middlewares/isFederationEnabled.ts(1 hunks)ee/packages/federation-matrix/src/events/invite.ts(2 hunks)ee/packages/federation-matrix/src/events/message.ts(7 hunks)ee/packages/federation-matrix/src/helpers/identifiers.ts(2 hunks)ee/packages/federation-matrix/src/services/MatrixMediaService.ts(1 hunks)packages/core-services/src/events/Events.ts(0 hunks)packages/core-services/src/types/IFederationMatrixService.ts(1 hunks)
💤 Files with no reviewable changes (2)
- packages/core-services/src/events/Events.ts
- ee/apps/federation-service/src/config.ts
🧰 Additional context used
🧠 Learnings (2)
📚 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/methods/addRoomOwner.tsapps/meteor/server/methods/addRoomModerator.tsee/packages/federation-matrix/src/api/middlewares/isFederationEnabled.tsapps/meteor/ee/server/hooks/federation/index.tsee/packages/federation-matrix/src/events/message.tsapps/meteor/server/services/room/hooks/BeforeFederationActions.tsapps/meteor/server/services/federation/utils.ts
📚 Learning: 2025-09-16T13:33:49.237Z
Learnt from: cardoso
PR: RocketChat/Rocket.Chat#36890
File: apps/meteor/tests/e2e/e2e-encryption/e2ee-otr.spec.ts:21-26
Timestamp: 2025-09-16T13:33:49.237Z
Learning: The im.delete API endpoint accepts either a `roomId` parameter (requiring the actual DM room _id) or a `username` parameter (for the DM partner's username). Constructing slug-like identifiers like `user2${Users.userE2EE.data.username}` doesn't work for this endpoint.
Applied to files:
ee/packages/federation-matrix/src/api/_matrix/invite.ts
🧬 Code graph analysis (9)
packages/core-services/src/types/IFederationMatrixService.ts (2)
packages/core-typings/src/IRoom.ts (1)
IRoomFederated(109-112)packages/core-typings/src/IUser.ts (1)
IUser(186-253)
apps/meteor/server/methods/addRoomOwner.ts (1)
apps/meteor/server/services/federation/utils.ts (1)
isFederationEnabled(3-5)
apps/meteor/lib/callbacks.ts (3)
packages/core-typings/src/IMessage/IMessage.ts (1)
IMessage(138-238)packages/core-typings/src/IUser.ts (1)
IUser(186-253)packages/core-typings/src/IRoom.ts (1)
IRoom(21-95)
apps/meteor/server/methods/addRoomModerator.ts (1)
apps/meteor/server/services/federation/utils.ts (1)
isFederationEnabled(3-5)
ee/packages/federation-matrix/src/events/invite.ts (1)
ee/packages/federation-matrix/src/helpers/identifiers.ts (1)
saveLocalUserForExternalUserId(40-82)
ee/packages/federation-matrix/src/events/message.ts (4)
ee/packages/federation-matrix/src/helpers/identifiers.ts (1)
saveLocalUserForExternalUserId(40-82)ee/packages/federation-matrix/src/services/MatrixMediaService.ts (1)
MatrixMediaService(18-148)ee/packages/federation-matrix/src/helpers/message.parsers.ts (2)
toInternalQuoteMessageFormat(125-151)toInternalMessageFormat(59-73)ee/packages/federation-matrix/src/FederationMatrix.ts (1)
fileTypes(34-39)
apps/meteor/server/services/room/hooks/BeforeFederationActions.ts (1)
apps/meteor/server/services/federation/utils.ts (1)
throwIfFederationNotEnabled(7-11)
ee/packages/federation-matrix/src/FederationMatrix.ts (4)
ee/packages/federation-matrix/src/helpers/identifiers.ts (1)
saveLocalUserForExternalUserId(40-82)ee/packages/federation-matrix/src/events/message.ts (1)
message(186-356)ee/packages/federation-matrix/src/services/MatrixMediaService.ts (1)
MatrixMediaService(18-148)packages/core-typings/src/IUser.ts (1)
IUser(186-253)
ee/packages/federation-matrix/src/api/_matrix/invite.ts (1)
ee/packages/federation-matrix/src/helpers/identifiers.ts (1)
saveLocalUserForExternalUserId(40-82)
🔇 Additional comments (28)
ee/packages/federation-matrix/src/helpers/identifiers.ts (2)
1-2: Type import split is fine.Importing UserStatus as a value is required after switching status default below.
64-65: Sane default: set new bridged users to OFFLINE.Good change; avoids showing newly created remote users as online.
Confirm no downstream logic assumed 'online' on creation for remote users (e.g., presence broadcasting).
apps/meteor/lib/callbacks.ts (1)
83-87: Typo fix to “params” looks good.Signature rename is source-compatible for TS implementers.
ee/packages/federation-matrix/src/api/_matrix/profiles.ts (1)
424-428: Good switch to precompiled validators for make_join.This aligns with the validator pattern used elsewhere and improves reuse/readability.
ee/packages/federation-matrix/src/api/_matrix/send-join.ts (1)
231-235: Validator swap looks correct.Params/body/response schemas match the route contract and improve clarity.
apps/meteor/server/methods/addRoomModerator.ts (1)
14-15: Import cleanup aligns with enabled-only check.Removing isFederationReady and keeping isFederationEnabled is consistent with the new gating model.
apps/meteor/server/services/messages/service.ts (1)
111-114: Type mismatch: IMessage.federation lacksversion.You're assigning federation: { eventId, version: 1 } but IMessage.federation only declares eventId — update the typing or narrow-cast to avoid TS excess-property errors.
Suggested typing change:
--- a/packages/core-typings/src/IMessage/IMessage.ts +++ b/packages/core-typings/src/IMessage/IMessage.ts @@ - federation?: { - eventId: string; - }; + federation?: { + eventId: string; + version?: number; + };If updating core-typings isn't possible now, narrow-cast the object and add a TODO to remove the cast once typings are updated.
Verify other producers/consumers tolerate the extra field (run locally):
git grep -n 'federation' || rg -uu -n '\bfederation\b' -Sapps/meteor/server/services/room/hooks/BeforeFederationActions.ts (2)
4-4: LGTM! Import change aligns with federation simplification.The change from
throwIfFederationNotEnabledOrNotReadytothrowIfFederationNotEnabledcorrectly reflects the removal of readiness checks across the federation codebase.
28-28: LGTM! Simplified federation check is consistent.The function call update maintains the same error-handling behavior while aligning with the broader simplification of federation state management.
apps/meteor/server/methods/addRoomOwner.ts (2)
14-14: LGTM! Import update aligns with federation utils refactor.Removing
isFederationReadyfrom the imports is consistent with the federation simplification effort across the codebase.
42-44: LGTM! Simplified federation check is appropriate.The removal of the readiness check (
isFederationReady()) simplifies the federation validation while maintaining the core requirement that federation must be enabled for federated rooms.ee/packages/federation-matrix/src/api/_matrix/invite.ts (4)
9-10: LGTM! Good refactor using the centralized helper.Using
saveLocalUserForExternalUserIdeliminates code duplication and centralizes the local user creation logic.
187-187: LGTM! Consistent use of the helper function.The replacement of inline user creation with
saveLocalUserForExternalUserIdmaintains consistency with the federation user management pattern.
225-232: Simplified user resolution logic is correct.The change from fetching both users in parallel to directly using the already-resolved
inviteeUserreduces unnecessary database queries. The error message update "Invitee user not found" is more grammatically correct.
319-319: LGTM! Removed unnecessary type cast.Passing
inviteEventdirectly without casting toPersistentEventBaseis cleaner since the type is already correct.ee/packages/federation-matrix/src/events/invite.ts (2)
4-6: LGTM! Consistent import updates.The import changes align with the centralized user creation approach using
saveLocalUserForExternalUserId.
23-24: LGTM! Good refactor using the helper function.Using
saveLocalUserForExternalUserIdcentralizes the federated user creation logic and ensures consistency across the codebase. The server name extraction with a fallback to 'unknown' provides reasonable default behavior.apps/meteor/server/services/federation/utils.ts (2)
3-5: LGTM! Simplified federation enabled check.The direct return of the
Federation_Service_Enabledsetting value simplifies the logic and removes unnecessary version-based complexity.
7-11: LGTM! Function rename reflects simplified logic.The rename from
throwIfFederationNotEnabledOrNotReadytothrowIfFederationNotEnabledaccurately reflects the removal of readiness checks.apps/meteor/ee/server/hooks/federation/index.ts (4)
23-23: LGTM! Fixed comment typo.The comment correction from "if room if exists" to "if room exists" improves clarity.
42-42: LGTM! Simplified federation check logic.Removing the
getFederationVersioncheck simplifies the flow while maintaining the same functionality throughshouldBeHandledByFederation.
58-58: LGTM! Updated callback identifier is more descriptive.The rename from 'federation-v2-after-room-message-sent' to 'native-federation-after-room-message-sent' better reflects the current federation implementation.
86-86: Good improvement for type safety!The updated mapping logic properly handles both string and object invitees while filtering out null values, making the code more robust.
ee/packages/federation-matrix/src/FederationMatrix.ts (3)
399-406: LGTM: MIME-to-Matrix msgtype mapping is correct and typed.Good fallback to
filewhen unknown.
83-83: VerifysigningKeyPathexpects a file, not a folder.You switched to
CONFIG_FOLDERor a default filename. Confirm the homeserver SDK reads a file path here.Would you check the SDK contract and ensure we pass a file path (and not a directory) in all deployments?
874-874: LGTM: narrowedupdateMessagesender type.Using
AtLeast<IUser, '_id' | 'username'>is appropriate for this call path.ee/packages/federation-matrix/src/events/message.ts (2)
33-33: LGTM: centralized federated user creation.Switch to
saveLocalUserForExternalUserIdaligns flows across events.
90-99: LGTM: thread root lookup by EventID.Typed
EventIDandfindOneByFederationIdusage is correct.
| return; | ||
| } | ||
|
|
||
| const formatted = toInternalMessageFormat({ | ||
| rawMessage: data.content['m.new_content'].body, | ||
| formattedMessage: data.content.formatted_body || '', |
There was a problem hiding this comment.
Edited-plain path should use m.new_content.formatted_body.
Formatting currently reads content.formatted_body. Use the edited payload.
Apply:
- const formatted = toInternalMessageFormat({
- rawMessage: content['m.new_content'].body,
- formattedMessage: content.formatted_body || '',
+ const formatted = toInternalMessageFormat({
+ rawMessage: content['m.new_content']?.body ?? '',
+ formattedMessage: content['m.new_content']?.formatted_body || content.formatted_body || '',
homeServerDomain: serverName,
senderExternalId: data.sender,
});📝 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 formatted = toInternalMessageFormat({ | |
| rawMessage: content['m.new_content'].body, | |
| formattedMessage: content.formatted_body || '', | |
| homeServerDomain: serverName, | |
| senderExternalId: data.sender, | |
| }); | |
| const formatted = toInternalMessageFormat({ | |
| rawMessage: content['m.new_content']?.body ?? '', | |
| formattedMessage: content['m.new_content']?.formatted_body || content.formatted_body || '', | |
| homeServerDomain: serverName, | |
| senderExternalId: data.sender, | |
| }); |
🤖 Prompt for AI Agents
In ee/packages/federation-matrix/src/events/message.ts around lines 256 to 261,
the formatted message is incorrectly read from content.formatted_body; update
that to read the edited payload field content['m.new_content'].formatted_body
(with the same fallback to an empty string) so the toInternalMessageFormat call
uses rawMessage: content['m.new_content'].body and formattedMessage:
content['m.new_content'].formatted_body || ''.
| let lastEventId: { eventId: string } | null = null; | ||
|
|
||
| for await (const file of message.files) { | ||
| const mxcUri = await MatrixMediaService.prepareLocalFileForMatrix(file._id, matrixDomain); | ||
|
|
||
| const msgtype = this.getMatrixMessageType(file.type); | ||
| const fileContent = { | ||
| body: file.name, | ||
| msgtype, | ||
| url: mxcUri, | ||
| info: { | ||
| mimetype: file.type, | ||
| size: file.size, | ||
| }, | ||
| }; | ||
|
|
||
| lastEventId = await this.homeserverServices.message.sendFileMessage(matrixRoomId, fileContent, matrixUserId); | ||
| } | ||
|
|
||
| return this.homeserverServices.message.sendFileMessage(matrixRoomId, fileContent, matrixUserId); | ||
| return lastEventId; | ||
| } catch (error) { |
There was a problem hiding this comment.
Multi-file messages lose all but last Matrix event ID.
handleFileMessage returns only the last eventId, and we persist a single federation.eventId. Deletions/edits will miss earlier file events.
- Return all event IDs from
handleFileMessage(e.g.,eventIds: string[]) and persist them (new field or mapping table). - Update deletion/redaction paths to iterate all event IDs.
If schema changes aren’t feasible now, gate multi-file sending or send a single aggregated file until persistence supports multiples.
Also applies to: 571-571
rodrigok
force-pushed
the
chore/federation-backup-fixes
branch
2 times, most recently
from
d81142c to
25b9ff0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ee/packages/federation-matrix/src/FederationMatrix.ts (1)
570-571: Potential data loss with multi-file messages.Line 571 shows
Messages.setFederationEventIdById(message._id, result.eventId)which only stores the last event ID. For multi-file messages wherehandleFileMessageprocesses multiple files and returns the last event ID, this means earlier file events won't be tracked for deletion/redaction operations.Consider implementing one of these solutions:
- Return and persist all event IDs from multi-file handling
- Gate multi-file sending until proper persistence is implemented
- Create a separate mapping table for file-to-event relationships
This is critical for maintaining data integrity in federation scenarios where messages may need to be deleted or edited.
🧹 Nitpick comments (1)
ee/packages/federation-matrix/src/FederationMatrix.ts (1)
82-82: Minor adjustment to signing key path default.The change from
./rocketchat.signing.keyto'./rocketchat.signing.key'improves consistency, though the functional impact is minimal.
📜 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/server/services/messages/service.ts(1 hunks)ee/packages/federation-matrix/src/FederationMatrix.ts(8 hunks)ee/packages/federation-matrix/src/api/_matrix/invite.ts(4 hunks)ee/packages/federation-matrix/src/api/_matrix/send-join.ts(1 hunks)ee/packages/federation-matrix/src/events/invite.ts(2 hunks)ee/packages/federation-matrix/src/events/message.ts(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- ee/packages/federation-matrix/src/api/_matrix/send-join.ts
- apps/meteor/server/services/messages/service.ts
- ee/packages/federation-matrix/src/events/invite.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-16T13:33:49.237Z
Learnt from: cardoso
PR: RocketChat/Rocket.Chat#36890
File: apps/meteor/tests/e2e/e2e-encryption/e2ee-otr.spec.ts:21-26
Timestamp: 2025-09-16T13:33:49.237Z
Learning: The im.delete API endpoint accepts either a `roomId` parameter (requiring the actual DM room _id) or a `username` parameter (for the DM partner's username). Constructing slug-like identifiers like `user2${Users.userE2EE.data.username}` doesn't work for this endpoint.
Applied to files:
ee/packages/federation-matrix/src/api/_matrix/invite.ts
🧬 Code graph analysis (3)
ee/packages/federation-matrix/src/FederationMatrix.ts (3)
ee/packages/federation-matrix/src/helpers/identifiers.ts (1)
saveLocalUserForExternalUserId(40-82)ee/packages/federation-matrix/src/events/message.ts (1)
message(189-364)ee/packages/federation-matrix/src/services/MatrixMediaService.ts (1)
MatrixMediaService(18-148)
ee/packages/federation-matrix/src/api/_matrix/invite.ts (1)
ee/packages/federation-matrix/src/helpers/identifiers.ts (1)
saveLocalUserForExternalUserId(40-82)
ee/packages/federation-matrix/src/events/message.ts (4)
ee/packages/federation-matrix/src/helpers/identifiers.ts (1)
saveLocalUserForExternalUserId(40-82)ee/packages/federation-matrix/src/services/MatrixMediaService.ts (1)
MatrixMediaService(18-148)ee/packages/federation-matrix/src/helpers/message.parsers.ts (2)
toInternalQuoteMessageFormat(125-151)toInternalMessageFormat(59-73)ee/packages/federation-matrix/src/FederationMatrix.ts (1)
fileTypes(33-38)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: 🔨 Test Unit / Unit Tests
- GitHub Check: 🔎 Code Check / Code Lint
- GitHub Check: 🔎 Code Check / TypeScript
- GitHub Check: 📦 Meteor Build - coverage
- GitHub Check: CodeQL-Build
🔇 Additional comments (18)
ee/packages/federation-matrix/src/api/_matrix/invite.ts (4)
4-4: LGTM! Improved imports for centralized user creation.The addition of specific type imports and the
saveLocalUserForExternalUserIdimport align well with the centralized user creation pattern being implemented across the federation module.Also applies to: 9-9
185-187: **LGTM! Consolidated user creation logic.**The replacement of inline user creation withsaveLocalUserForExternalUserIdis well-implemented and follows the centralized approach established in the referenced helper function. The function properly creates the user with appropriate federation metadata and returns the inserted ID.
225-233: LGTM! Improved user lookup logic for DM creation.The change to sequential processing using the already-resolved
inviteeUser(the mapped user) directly is more efficient than parallel fetching. The error message improvement from "inviteeUser user not found" to "Invitee user not found" enhances readability.
318-319: LGTM! Clean inviteEvent parameter passing.The removal of casting and direct passing of
inviteEventtostartJoiningRoommaintains type safety and follows the function signature improvements in the codebase.ee/packages/federation-matrix/src/FederationMatrix.ts (7)
3-3: LGTM! Enhanced type imports for better type safety.The addition of
FileMessageTypeandAtLeasttypes, along with the update to includeAtLeastin the user type imports, improves type safety across the federation module.Also applies to: 9-9
29-29: LGTM! Centralized user creation helper exported.The addition of
saveLocalUserForExternalUserIdto the exports aligns with the centralized user creation pattern being implemented across the federation module.
33-38: LGTM! Improved type mapping for file messages.The updated
fileTypesmapping withFileMessageTypeas the value type provides better type safety and includes the new 'file' to 'm.file' mapping.
268-294: Review Matrix user ID normalization logic.There are potential issues with the Matrix user ID construction and normalization in this section that align with the previous review comments about '@@' prefixes and lookup mismatches. The current logic can produce malformed IDs.
398-405: LGTM! Improved file type resolution.The updated
getMatrixMessageTypemethod signature now returnsFileMessageTypefor better type safety and uses the newfileTypesmapping correctly.
418-438: **Multi-file message persistence issue remains.**The issue raised about multi-file messages losing all but the last Matrix event ID persists. When multiple files are sent in a single message, only the lasteventIdis returned and persisted. This means that for deletions/edits, earlier file events will be missed.
873-873: LGTM! Improved type safety for message updates.The
updateMessagesignature now usesAtLeast<IUser, '_id' | 'username'>which provides better type safety by ensuring only the required user fields are available while maintaining flexibility.ee/packages/federation-matrix/src/events/message.ts (7)
1-1: LGTM! Enhanced imports for typed message handling.The addition of
FileMessageContent,MessageType,EventID, andFileAttachmentPropsimports improves type safety throughout the message processing pipeline.Also applies to: 3-3, 5-5
11-11: LGTM! Centralized user creation pattern.The import of
saveLocalUserForExternalUserIdaligns with the centralized user creation approach being implemented across the federation module.
33-33: LGTM! Consolidated user creation logic.The replacement of inline user creation with the centralized
saveLocalUserForExternalUserIdhelper improves consistency and maintainability across the federation module.
90-90: LGTM! Improved type safety for event handling.The updates to use
EventIDtype instead ofstringforthreadRootEventIdparameter and return type annotations improve type safety in the thread and media message handling flow.Also applies to: 108-109, 116-116
101-117: LGTM! Enhanced media message handling with proper typing.The updated
handleMediaMessagesignature and implementation properly uses the new typed parameters:
url: string,fileInfo: FileMessageContent['info'],msgtype: MessageType- Returns
attachments: [FileAttachmentProps]with proper type-specific fields for image, video, and audioThe per-media-type attachment construction with optional dimensions and size fields derived from
fileInfois well-implemented.Also applies to: 141-177
234-274: Edited message handling needs content field fixes.The edited message handling in both quote and plain text paths is still using incorrect content fields for the new content.
The issues mentioned in the past review comments about using
content['m.new_content']?.formatted_bodyinstead ofcontent.formatted_bodyfor edited messages need to be addressed.
307-312: LGTM! Improved media message detection and handling.The use of
Object.values(fileTypes)to getMessageType[]and the updatedhandleMediaMessagecall with the new signature parameters (content.url,content.info,msgtype, etc.) properly leverages the enhanced typing system.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
apps/meteor/app/lib/server/methods/sendMessage.ts (3)
2-2: Drop redundant union import (INativeFederatedMessage extends IMessage).If we simplify the param type (see Line 22), this import becomes unnecessary.
-import type { AtLeast, IMessage, IUser, INativeFederatedMessage } from '@rocket.chat/core-typings'; +import type { AtLeast, IMessage, IUser } from '@rocket.chat/core-typings';
22-22: Simplify the parameter type: AtLeast<IMessage, 'rid'> is sufficient.INativeFederatedMessage extends IMessage, so callers with federated messages still type‑check. This avoids AtLeast distributing over a union and keeps inference cleaner.
-export async function executeSendMessage(uid: IUser['_id'], message: AtLeast<IMessage | INativeFederatedMessage, 'rid'>, previewUrls?: string[]) { +export async function executeSendMessage(uid: IUser['_id'], message: AtLeast<IMessage, 'rid'>, previewUrls?: string[]) {
103-108: Ephemeral error should target the normalized rid (handles nested threads).You compute a corrected rid above for nested threads, but still broadcast to message.rid. Use the resolved rid to avoid misrouting the ephemeral error.
- void api.broadcast('notify.ephemeralMessage', uid, message.rid, { + void api.broadcast('notify.ephemeralMessage', uid, rid, { msg: i18n.t(errorMessage, { ...errorContext, lng: user.language }), });
📜 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 (8)
apps/meteor/app/lib/server/functions/createRoom.ts(1 hunks)apps/meteor/app/lib/server/methods/sendMessage.ts(2 hunks)apps/meteor/server/lib/ldap/Manager.ts(1 hunks)apps/meteor/server/services/messages/service.ts(1 hunks)ee/packages/federation-matrix/src/helpers/identifiers.ts(2 hunks)packages/core-typings/src/IUser.ts(2 hunks)packages/models/src/models/Rooms.ts(1 hunks)packages/models/src/models/Users.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/models/src/models/Users.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- ee/packages/federation-matrix/src/helpers/identifiers.ts
- apps/meteor/server/services/messages/service.ts
🧰 Additional context used
🧠 Learnings (1)
📚 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-typings/src/IUser.ts
🧬 Code graph analysis (1)
apps/meteor/app/lib/server/methods/sendMessage.ts (1)
packages/core-typings/src/IMessage/IMessage.ts (2)
IMessage(138-238)INativeFederatedMessage(283-288)
⏰ 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). (2)
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (7)
apps/meteor/app/lib/server/methods/sendMessage.ts (1)
117-122: Confirm intent: keep DDP method limited to IMessage?ServerMethods still requires AtLeast<IMessage, '_id' | 'rid' | 'msg'> while executeSendMessage documents acceptance of INativeFederatedMessage. If the goal is to prevent RPC clients from forging federation fields, keeping IMessage here is correct. If not, consider widening the DDP type and tightening runtime validation of federation shape.
apps/meteor/app/lib/server/functions/createRoom.ts (1)
196-196: LGTM! Federation version standardized to string format.The change from numeric
1to string'1'aligns with the broader federation version type standardization mentioned in the AI summary. This ensures consistency across the federation system.packages/core-typings/src/IUser.ts (3)
266-266: LGTM! Template literal type ensures type safety.The required
version: ${number}field inIUserNativeFederatedproperly enforces that federation version must be a string representation of a number, which aligns with the federation version standardization.
230-230: Verify template literal type usage for federation.versionThe change
version?: \${number}`` makes version a stringified number; confirm all runtime validators/serializers, JSON schemas, API contracts, and consumers accept string numeric versions (and update tests/consumers that expect numeric types).
Location: packages/core-typings/src/IUser.ts:230
270-271: Approve: isUserNativeFederated correctly validates string-based federation.versiontypeof user.federation?.version === 'string' properly covers the
${number}template-literal (runtime values like '1'); no changes required.packages/models/src/models/Rooms.ts (1)
1975-1975: LGTM! Federation version consistency maintained.The change from numeric
1to string'1'in thecreateWithFullRoomDatamethod maintains consistency with the federation version standardization. The logic correctly sets the federation version when a room is marked as federated.apps/meteor/server/lib/ldap/Manager.ts (1)
199-205: String literal change looks correct; replace magic literal with a constant to avoid drift.Repo search did not locate a central typing for federation.version and there are unrelated numeric
version: 1literals elsewhere — add the constant, switch the literal, and verify federation.version is consistently treated as a string.Apply this diff locally:
- federation: { - version: '1', - }, + federation: { + version: LDAPManager.FEDERATION_VERSION, + },Add near the top of LDAPManager (apps/meteor/server/lib/ldap/Manager.ts):
private static readonly FEDERATION_VERSION = '1' as const;
rodrigok
force-pushed
the
chore/federation-backup-fixes
branch
4 times, most recently
from
7fb930d to
f6a91f5
Compare
ggazzo
force-pushed
the
chore/federation-backup-fixes
branch
3 times, most recently
from
19d54ca to
48af953
Compare
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 (2)
ee/packages/federation-matrix/src/FederationMatrix.ts (1)
252-277: Normalize Matrix IDs and check bridge mapping first to avoid duplicates.Current logic can build
@@user:domainand misses existing bridges. Normalize and short‑circuit if a mapping exists.- const externalUserId = username.includes(':') ? `@${username}` : `@${username}:${this.serverName}`; - - const existingUser = await Users.findOneByUsername(username); + const externalUserId = username.startsWith('@') + ? (username.includes(':') ? username : `${username}:${this.serverName}`) + : (username.includes(':') ? `@${username}` : `@${username}:${this.serverName}`); + + // If already bridged, skip. + const bridgedLocalId = await MatrixBridgedUser.getLocalUserIdByExternalId(externalUserId); + if (bridgedLocalId) { + continue; + } + + const existingUser = await Users.findOneByUsername(username);ee/packages/federation-matrix/src/events/message.ts (1)
17-35: Wrong lookup for existing federated user — will recreate users.Users.findOneByUsername(matrixUserId) won't match local sanitized usernames; map external Matrix ID to a local user id first with MatrixBridgedUser.getLocalUserIdByExternalId and then load by local id.
File: ee/packages/federation-matrix/src/events/message.ts (lines 17-35)
-const user = await Users.findOneByUsername(matrixUserId); -if (user) { - await MatrixBridgedUser.createOrUpdateByLocalId(user._id, matrixUserId, false, domain); - return user; -} +const localId = await MatrixBridgedUser.getLocalUserIdByExternalId(matrixUserId); +if (localId) { + const user = await Users.findOneById(localId); + if (user) return user; +}
🧹 Nitpick comments (2)
ee/packages/federation-matrix/src/api/_matrix/invite.ts (1)
225-233: Avoid “user” shadowing; clarify sender vs. invitee.Minor readability: earlier a block‑scoped
const user = ...exists; laterinviteeUser = userrefers to the param. Rename the earlier local to prevent confusion.- const user = await Users.findOneById(internalSenderUserId); - if (!user) { + const mappedSenderUser = await Users.findOneById(internalSenderUserId); + if (!mappedSenderUser) { throw new Error('user not found although should have as it is in mapping not processing invite'); } - senderUserId = user._id; + senderUserId = mappedSenderUser._id;ee/packages/federation-matrix/src/events/message.ts (1)
101-117: Change attachments typing from single‑element tuple to arrayReplace the return-type property
attachments: [FileAttachmentProps]withattachments: FileAttachmentProps[]so multiple attachments are supported. (No runtime change required — the valueattachments: [attachment]is already a valid array.)File: ee/packages/federation-matrix/src/events/message.ts (line 116)
Diff:
-): Promise<{ …; attachments: [FileAttachmentProps]; }> {
+): Promise<{ …; attachments: FileAttachmentProps[]; }> {
📜 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 (16)
apps/meteor/ee/server/hooks/federation/index.ts(3 hunks)apps/meteor/lib/callbacks.ts(1 hunks)apps/meteor/server/services/messages/service.ts(1 hunks)ee/apps/federation-service/src/config.ts(0 hunks)ee/packages/federation-matrix/src/FederationMatrix.ts(7 hunks)ee/packages/federation-matrix/src/api/_matrix/invite.ts(4 hunks)ee/packages/federation-matrix/src/api/_matrix/profiles.ts(1 hunks)ee/packages/federation-matrix/src/api/_matrix/rooms.ts(1 hunks)ee/packages/federation-matrix/src/api/_matrix/send-join.ts(1 hunks)ee/packages/federation-matrix/src/api/middlewares/isFederationEnabled.ts(1 hunks)ee/packages/federation-matrix/src/events/invite.ts(2 hunks)ee/packages/federation-matrix/src/events/message.ts(6 hunks)ee/packages/federation-matrix/src/helpers/identifiers.ts(2 hunks)ee/packages/federation-matrix/src/services/MatrixMediaService.ts(1 hunks)packages/core-services/src/types/IFederationMatrixService.ts(1 hunks)packages/core-typings/src/IMessage/IMessage.ts(1 hunks)
💤 Files with no reviewable changes (1)
- ee/apps/federation-service/src/config.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- ee/packages/federation-matrix/src/api/middlewares/isFederationEnabled.ts
- ee/packages/federation-matrix/src/events/invite.ts
- ee/packages/federation-matrix/src/api/_matrix/rooms.ts
- apps/meteor/server/services/messages/service.ts
- apps/meteor/ee/server/hooks/federation/index.ts
- ee/packages/federation-matrix/src/services/MatrixMediaService.ts
- ee/packages/federation-matrix/src/api/_matrix/profiles.ts
- ee/packages/federation-matrix/src/api/_matrix/send-join.ts
- ee/packages/federation-matrix/src/helpers/identifiers.ts
- apps/meteor/lib/callbacks.ts
🧰 Additional context used
🧠 Learnings (3)
📓 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-16T13:33:49.237Z
Learnt from: cardoso
PR: RocketChat/Rocket.Chat#36890
File: apps/meteor/tests/e2e/e2e-encryption/e2ee-otr.spec.ts:21-26
Timestamp: 2025-09-16T13:33:49.237Z
Learning: The im.delete API endpoint accepts either a `roomId` parameter (requiring the actual DM room _id) or a `username` parameter (for the DM partner's username). Constructing slug-like identifiers like `user2${Users.userE2EE.data.username}` doesn't work for this endpoint.
Applied to files:
ee/packages/federation-matrix/src/api/_matrix/invite.ts
📚 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:
ee/packages/federation-matrix/src/FederationMatrix.ts
🧬 Code graph analysis (4)
packages/core-services/src/types/IFederationMatrixService.ts (3)
ee/packages/federation-matrix/src/events/room.ts (1)
room(6-64)packages/core-typings/src/IRoom.ts (1)
IRoomFederated(109-112)packages/core-typings/src/IUser.ts (1)
IUser(186-253)
ee/packages/federation-matrix/src/api/_matrix/invite.ts (1)
ee/packages/federation-matrix/src/helpers/identifiers.ts (1)
saveLocalUserForExternalUserId(40-82)
ee/packages/federation-matrix/src/events/message.ts (3)
ee/packages/federation-matrix/src/helpers/identifiers.ts (1)
saveLocalUserForExternalUserId(40-82)ee/packages/federation-matrix/src/services/MatrixMediaService.ts (1)
MatrixMediaService(18-148)ee/packages/federation-matrix/src/FederationMatrix.ts (1)
fileTypes(33-38)
ee/packages/federation-matrix/src/FederationMatrix.ts (2)
ee/packages/federation-matrix/src/helpers/identifiers.ts (1)
saveLocalUserForExternalUserId(40-82)ee/packages/federation-matrix/src/services/MatrixMediaService.ts (1)
MatrixMediaService(18-148)
⏰ 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 (9)
ee/packages/federation-matrix/src/api/_matrix/invite.ts (2)
9-10: Use saveLocalUserForExternalUserId instead of ad‑hoc creation — good change.Reduces duplication and centralizes mapping. No issues spotted.
Also applies to: 187-188
318-323: Passing inviteEvent directly to startJoiningRoom — LGTM.ee/packages/federation-matrix/src/FederationMatrix.ts (2)
856-897: updateMessage sender typing tightened to AtLeast — LGTM.
401-421: Multi‑file send returns only the last eventId — earlier files become orphaned.Return and persist all Matrix event IDs (or gate to single‑file until persistence supports arrays).
- private async handleFileMessage(…): Promise<{ eventId: string } | null> { + private async handleFileMessage(…): Promise<{ eventIds: string[] } | null> { … - let lastEventId: { eventId: string } | null = null; + const eventIds: string[] = []; … - lastEventId = await this.homeserverServices.message.sendFileMessage(matrixRoomId, fileContent, matrixUserId); + const { eventId } = await this.homeserverServices.message.sendFileMessage(matrixRoomId, fileContent, matrixUserId); + eventIds.push(eventId); … - return lastEventId; + return { eventIds };Then adapt callers/persistence, or temporarily block multi‑file send to one file.
#!/bin/bash rg -nP -C2 'handleFileMessage\(|setFederationEventIdById\(|federation\.eventId\b'ee/packages/federation-matrix/src/events/message.ts (3)
244-249: Edited‑quote should use m.new_content fields.Use updated content for replacements.
-const formatted = await toInternalQuoteMessageFormat({ +const formatted = await toInternalQuoteMessageFormat({ messageToReplyToUrl, - formattedMessage: data.content.formatted_body || '', - rawMessage: messageBody, + formattedMessage: data.content['m.new_content']?.formatted_body || data.content.formatted_body || '', + rawMessage: data.content['m.new_content']?.body ?? messageBody, homeServerDomain: serverName, senderExternalId: data.sender, });
261-266: Edited‑plain should use m.new_content.formatted_body.-const formatted = toInternalMessageFormat({ - rawMessage: data.content['m.new_content'].body, - formattedMessage: data.content.formatted_body || '', +const formatted = toInternalMessageFormat({ + rawMessage: data.content['m.new_content']?.body ?? '', + formattedMessage: data.content['m.new_content']?.formatted_body || data.content.formatted_body || '', homeServerDomain: serverName, senderExternalId: data.sender, });
302-305: Media dispatch via MatrixMediaService — LGTM.packages/core-typings/src/IMessage/IMessage.ts (1)
285-288: federation.version switched to number — add runtime guard and plan migration.Typing change is fine; assert numeric type at runtime and migrate any persisted string versions. I ran the suggested rg and got no matches — manually verify persistence/serialization/read/write paths that may store federation.version.
Apply:
-export const isMessageFromNativeFederation = (message: IMessage): message is INativeFederatedMessage => - isMessageFromMatrixFederation(message) && 'version' in message.federation; +export const isMessageFromNativeFederation = (message: IMessage): message is INativeFederatedMessage => + isMessageFromMatrixFederation(message) && + typeof (message.federation as any).version === 'number';packages/core-services/src/types/IFederationMatrixService.ts (1)
28-28: inviteUsersToRoom requires IUser — verified callers pass full IUser; no action required.Checked call sites: apps/meteor/app/lib/server/methods/addUsersToRoom.ts (passes Meteor.userAsync user), apps/meteor/ee/server/hooks/federation/index.ts (both invocations pass inviter), apps/meteor/app/lib/server/functions/addUserToRoom.ts (inviter resolved via Users.findOneById).
| const { msgtype } = content; | ||
| const messageBody = content.body.toString(); | ||
|
|
||
| if (!messageBody && !msgtype) { | ||
| logger.debug('No message content found in event'); |
There was a problem hiding this comment.
Possible TypeError: content.body may be undefined.
Guard before .toString().
-const messageBody = content.body.toString();
+const messageBody = content.body != null ? String(content.body) : '';📝 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 { msgtype } = content; | |
| const messageBody = content.body.toString(); | |
| if (!messageBody && !msgtype) { | |
| logger.debug('No message content found in event'); | |
| const { msgtype } = content; | |
| const messageBody = content.body != null ? String(content.body) : ''; | |
| if (!messageBody && !msgtype) { | |
| logger.debug('No message content found in event'); |
🤖 Prompt for AI Agents
In ee/packages/federation-matrix/src/events/message.ts around lines 193 to 197,
the code calls content.body.toString() but content.body can be undefined which
will throw a TypeError; guard access before calling toString by first checking
whether content.body is defined (and optionally whether it's a string/Buffer),
e.g. compute messageBody only if content.body != null (or use optional
chaining/ternary to default to an empty string), move the !messageBody &&
!msgtype check after this safe extraction, and ensure any logging or downstream
logic handles the default empty string case.
ggazzo
force-pushed
the
chore/federation-backup-fixes
branch
from
a3ff8bb to
89f1d59
Compare
Co-authored-by: Guilherme Gazzo <[email protected]>
Co-authored-by: Guilherme Gazzo <[email protected]>
Depends on RocketChat/homeserver#207
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
New Features
Improvements
Bug Fixes