fix: process joins from full state #37156
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
|
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 |
|
Contributor
WalkthroughAdds a new emitJoin method to FederationMatrix and exposes it via IFederationMatrixService. Updates invite handler to use latest room state retrieval and emit join membership events for existing members. Bumps @rocket.chat/federation-sdk dependency. Imports expanded for new typing. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant InviteAPI as Invite Handler
participant MatrixRoom as Room State
participant FedSvc as FederationMatrix
participant HS as Homeserver Services
User->>InviteAPI: Accepts/Processes Invite
InviteAPI->>MatrixRoom: Add user to room
Note right of InviteAPI: Retrieve latest room state
loop For each existing member join event
InviteAPI->>MatrixRoom: getMemberJoinEvents()
MatrixRoom-->>InviteAPI: membershipEvent
InviteAPI->>FedSvc: emitJoin(membershipEvent)
alt Homeserver services available
FedSvc->>HS: emit homeserver.matrix.membership (membership: join)
else Unavailable
FedSvc-->>InviteAPI: warn and return
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## chore/fed-branded #37156 +/- ##
=====================================================
- Coverage 67.46% 67.43% -0.03%
=====================================================
Files 3331 3331
Lines 113527 113527
Branches 20615 20611 -4
=====================================================
- Hits 76593 76562 -31
- Misses 34312 34343 +31
Partials 2622 2622
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
ggazzo
force-pushed
the
fix/process-joins-from-full-state
branch
from
e6e4893 to
6e96765
Compare
…0.1.17 in package.json and yarn.lock
ggazzo
force-pushed
the
fix/process-joins-from-full-state
branch
from
6e96765 to
7c07dcb
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (4)
ee/packages/federation-matrix/package.json(1 hunks)ee/packages/federation-matrix/src/FederationMatrix.ts(2 hunks)ee/packages/federation-matrix/src/api/_matrix/invite.ts(3 hunks)packages/core-services/src/types/IFederationMatrixService.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
ee/packages/federation-matrix/src/api/_matrix/invite.ts (1)
ee/packages/federation-matrix/src/FederationMatrix.ts (1)
FederationMatrix(145-999)
🔇 Additional comments (4)
ee/packages/federation-matrix/src/api/_matrix/invite.ts (2)
1-1: LGTM!The import of
FederationMatrixis necessary for the newemitJoinfunctionality added at lines 269-272.
175-175: Confirm use of getLatestRoomState2 vs getFullRoomState2
The call at line 175 was switched fromstate.getFullRoomState2tostate.getLatestRoomState2, altering which room state is fetched and conflicting with the PR title “process joins from full state.” Verify this change is intentional and doesn’t break invite processing or omit needed state.ee/packages/federation-matrix/src/FederationMatrix.ts (1)
12-20: LGTM!The expanded imports of
PersistentEventBaseandRoomVersionare necessary for the strongly-typedemitJoinmethod signature.ee/packages/federation-matrix/package.json (1)
41-41: Version 0.1.17 of @rocket.chat/federation-sdk is available on npm and has no known security advisories.
Comment on lines
+269
to
+272
| for await (const event of matrixRoom.getMemberJoinEvents()) { | ||
| console.log('emitting another join', event.toStrippedJson()); | ||
| await FederationMatrix.emitJoin(event); | ||
| } |
Contributor
There was a problem hiding this comment.
Replace console.log with proper logger and add error handling.
The loop has several issues:
- Line 270 uses
console.loginstead of a proper logger - No error handling for
FederationMatrix.emitJoin- if it fails, remaining events won't be processed - The log message "emitting another join" is informal
Apply this diff to use proper logging and add error handling:
await Room.addUserToRoom(internalRoomId, { _id: user._id }, { _id: senderUserId, username: inviteEvent.sender });
for await (const event of matrixRoom.getMemberJoinEvents()) {
- console.log('emitting another join', event.toStrippedJson());
- await FederationMatrix.emitJoin(event);
+ try {
+ await FederationMatrix.emitJoin(event);
+ } catch (error) {
+ console.error('Failed to emit join event', { eventId: event.eventId, error });
+ }
}
}Note: If a proper logger instance is available in this scope, replace console.error with the logger.
📝 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.
Suggested change
| for await (const event of matrixRoom.getMemberJoinEvents()) { | |
| console.log('emitting another join', event.toStrippedJson()); | |
| await FederationMatrix.emitJoin(event); | |
| } | |
| await Room.addUserToRoom(internalRoomId, { _id: user._id }, { _id: senderUserId, username: inviteEvent.sender }); | |
| for await (const event of matrixRoom.getMemberJoinEvents()) { | |
| - console.log('emitting another join', event.toStrippedJson()); | |
| try { | |
| await FederationMatrix.emitJoin(event); | |
| } catch (error) { | |
| console.error('Failed to emit join event', { eventId: event.eventId, error }); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In ee/packages/federation-matrix/src/api/_matrix/invite.ts around lines 269 to
272, replace the informal console.log with the module logger (or console.error
only if no logger in scope) and wrap the await FederationMatrix.emitJoin(event)
call in a try/catch so a failure for one event does not stop processing the
rest; on success use a clear info/debug log (include context like room id,
member and event id/stripped JSON) and on error log an error-level message
including the caught error and the same context, then continue to the next
event.
Comment on lines
+983
to
+998
| async emitJoin(membershipEvent: PersistentEventBase<RoomVersion, 'm.room.member'>) { | ||
| if (!this.homeserverServices) { | ||
| this.logger.warn('Homeserver services not available, skipping user role room scoped'); | ||
| return; | ||
| } | ||
|
|
||
| this.homeserverServices.emitter.emit('homeserver.matrix.membership', { | ||
| event_id: membershipEvent.eventId, | ||
| event: membershipEvent.event, | ||
| room_id: membershipEvent.roomId, | ||
| state_key: membershipEvent.stateKey, | ||
| content: { membership: 'join' }, | ||
| sender: membershipEvent.sender, | ||
| origin_server_ts: Date.now(), | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
Fix warning message and preserve original event data.
The method has several issues:
- Line 985: The warning message is copy-pasted from another method and says "skipping user role room scoped" instead of something relevant like "skipping join event emission"
- Line 994: The content hardcodes
membership: 'join', but this should preserve the original membership state from the event parameter - Line 996: Using
Date.now()fororigin_server_tsloses the original event timestamp
Apply this diff to fix these issues:
async emitJoin(membershipEvent: PersistentEventBase<RoomVersion, 'm.room.member'>) {
if (!this.homeserverServices) {
- this.logger.warn('Homeserver services not available, skipping user role room scoped');
+ this.logger.warn('Homeserver services not available, skipping join event emission');
return;
}
this.homeserverServices.emitter.emit('homeserver.matrix.membership', {
event_id: membershipEvent.eventId,
event: membershipEvent.event,
room_id: membershipEvent.roomId,
state_key: membershipEvent.stateKey,
- content: { membership: 'join' },
+ content: membershipEvent.event.content,
sender: membershipEvent.sender,
- origin_server_ts: Date.now(),
+ origin_server_ts: membershipEvent.event.origin_server_ts,
});
}📝 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.
Suggested change
| async emitJoin(membershipEvent: PersistentEventBase<RoomVersion, 'm.room.member'>) { | |
| if (!this.homeserverServices) { | |
| this.logger.warn('Homeserver services not available, skipping user role room scoped'); | |
| return; | |
| } | |
| this.homeserverServices.emitter.emit('homeserver.matrix.membership', { | |
| event_id: membershipEvent.eventId, | |
| event: membershipEvent.event, | |
| room_id: membershipEvent.roomId, | |
| state_key: membershipEvent.stateKey, | |
| content: { membership: 'join' }, | |
| sender: membershipEvent.sender, | |
| origin_server_ts: Date.now(), | |
| }); | |
| } | |
| async emitJoin(membershipEvent: PersistentEventBase<RoomVersion, 'm.room.member'>) { | |
| if (!this.homeserverServices) { | |
| this.logger.warn('Homeserver services not available, skipping join event emission'); | |
| return; | |
| } | |
| this.homeserverServices.emitter.emit('homeserver.matrix.membership', { | |
| event_id: membershipEvent.eventId, | |
| event: membershipEvent.event, | |
| room_id: membershipEvent.roomId, | |
| state_key: membershipEvent.stateKey, | |
| content: membershipEvent.event.content, | |
| sender: membershipEvent.sender, | |
| origin_server_ts: membershipEvent.event.origin_server_ts, | |
| }); | |
| } |
🤖 Prompt for AI Agents
In ee/packages/federation-matrix/src/FederationMatrix.ts around lines 983 to
998, fix the incorrect warning text and preserve original event data when
emitting joins: change the logger.warn text to something like "Homeserver
services not available, skipping join event emission"; set the emitted
content.membership to the membership value from the incoming event (e.g.
membershipEvent.event.content.membership) instead of hardcoding 'join'; and set
origin_server_ts to the original event timestamp (e.g.
membershipEvent.event.origin_server_ts or equivalent field on the event) rather
than using Date.now() so the original event time is preserved.
| inviteUsersToRoom(room: IRoomFederated, usersUserName: string[], inviter: IUser): Promise<void>; | ||
| notifyUserTyping(rid: string, user: string, isTyping: boolean): Promise<void>; | ||
| verifyMatrixIds(matrixIds: string[]): Promise<{ [key: string]: string }>; | ||
| emitJoin(membershipEvent: any): Promise<void>; |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Replace any with strongly-typed parameter.
The interface uses any for the membershipEvent parameter, but the implementation in FederationMatrix.ts (lines 983-998) uses the specific type PersistentEventBase<RoomVersion, 'm.room.member'>. This type mismatch defeats TypeScript's type safety.
Apply this diff to align the interface with the implementation:
+import type { PersistentEventBase, RoomVersion } from '@rocket.chat/federation-sdk';
+
export interface IFederationMatrixService {
createRoom(room: IRoomFederated, owner: IUser, members: string[]): Promise<{ room_id: string; event_id: string }>;
ensureFederatedUsersExistLocally(members: string[]): Promise<void>;
createDirectMessageRoom(room: IRoomFederated, members: IUser[], creatorId: IUser['_id']): Promise<void>;
sendMessage(message: IMessage, room: IRoomFederated, user: IUser): Promise<void>;
deleteMessage(matrixRoomId: string, message: IMessage): Promise<void>;
sendReaction(messageId: string, reaction: string, user: IUser): Promise<void>;
removeReaction(messageId: string, reaction: string, user: IUser, oldMessage: IMessage): Promise<void>;
getEventById(eventId: string): Promise<any | null>;
leaveRoom(rid: IRoomFederated['_id'], user: IUser): Promise<void>;
kickUser(room: IRoomNativeFederated, removedUser: IUser, userWhoRemoved: IUser): Promise<void>;
updateMessage(room: IRoomNativeFederated, message: IMessage): Promise<void>;
updateRoomName(rid: string, displayName: string, user: IUser): Promise<void>;
updateRoomTopic(room: IRoomNativeFederated, topic: string, user: IUser): Promise<void>;
addUserRoleRoomScoped(
room: IRoomNativeFederated,
senderId: string,
userId: string,
role: 'moderator' | 'owner' | 'leader' | 'user',
): Promise<void>;
inviteUsersToRoom(room: IRoomFederated, usersUserName: string[], inviter: IUser): Promise<void>;
notifyUserTyping(rid: string, user: string, isTyping: boolean): Promise<void>;
verifyMatrixIds(matrixIds: string[]): Promise<{ [key: string]: string }>;
- emitJoin(membershipEvent: any): Promise<void>;
+ emitJoin(membershipEvent: PersistentEventBase<RoomVersion, 'm.room.member'>): Promise<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.
Suggested change
| emitJoin(membershipEvent: any): Promise<void>; | |
| import type { PersistentEventBase, RoomVersion } from '@rocket.chat/federation-sdk'; | |
| export interface IFederationMatrixService { | |
| createRoom(room: IRoomFederated, owner: IUser, members: string[]): Promise<{ room_id: string; event_id: string }>; | |
| ensureFederatedUsersExistLocally(members: string[]): Promise<void>; | |
| createDirectMessageRoom(room: IRoomFederated, members: IUser[], creatorId: IUser['_id']): Promise<void>; | |
| sendMessage(message: IMessage, room: IRoomFederated, user: IUser): Promise<void>; | |
| deleteMessage(matrixRoomId: string, message: IMessage): Promise<void>; | |
| sendReaction(messageId: string, reaction: string, user: IUser): Promise<void>; | |
| removeReaction(messageId: string, reaction: string, user: IUser, oldMessage: IMessage): Promise<void>; | |
| getEventById(eventId: string): Promise<any | null>; | |
| leaveRoom(rid: IRoomFederated['_id'], user: IUser): Promise<void>; | |
| kickUser(room: IRoomNativeFederated, removedUser: IUser, userWhoRemoved: IUser): Promise<void>; | |
| updateMessage(room: IRoomNativeFederated, message: IMessage): Promise<void>; | |
| updateRoomName(rid: string, displayName: string, user: IUser): Promise<void>; | |
| updateRoomTopic(room: IRoomNativeFederated, topic: string, user: IUser): Promise<void>; | |
| addUserRoleRoomScoped( | |
| room: IRoomNativeFederated, | |
| senderId: string, | |
| userId: string, | |
| role: 'moderator' | 'owner' | 'leader' | 'user', | |
| ): Promise<void>; | |
| inviteUsersToRoom(room: IRoomFederated, usersUserName: string[], inviter: IUser): Promise<void>; | |
| notifyUserTyping(rid: string, user: string, isTyping: boolean): Promise<void>; | |
| verifyMatrixIds(matrixIds: string[]): Promise<{ [key: string]: string }>; | |
| emitJoin(membershipEvent: PersistentEventBase<RoomVersion, 'm.room.member'>): Promise<void>; | |
| } |
🤖 Prompt for AI Agents
In packages/core-services/src/types/IFederationMatrixService.ts around line 26,
the emitJoin(membershipEvent: any) signature uses any but the implementation
expects PersistentEventBase<RoomVersion, 'm.room.member'>; change the parameter
type to PersistentEventBase<RoomVersion, 'm.room.member'> and add the necessary
imports (PersistentEventBase and RoomVersion) from their source module so the
interface matches FederationMatrix.ts implementation and preserves TypeScript
type safety.
ggazzo
added a commit
that referenced
this pull request
Oct 8, 2025
Co-authored-by: Guilherme Gazzo <[email protected]>
ggazzo
added a commit
that referenced
this pull request
Oct 8, 2025
Co-authored-by: Guilherme Gazzo <[email protected]>
ggazzo
added a commit
that referenced
this pull request
Oct 8, 2025
Co-authored-by: Guilherme Gazzo <[email protected]>
ggazzo
added a commit
that referenced
this pull request
Oct 8, 2025
Co-authored-by: Guilherme Gazzo <[email protected]>
ggazzo
added a commit
that referenced
this pull request
Oct 8, 2025
Co-authored-by: Guilherme Gazzo <[email protected]>
ggazzo
added a commit
that referenced
this pull request
Oct 8, 2025
Co-authored-by: Guilherme Gazzo <[email protected]>
ricardogarim
pushed a commit
that referenced
this pull request
Oct 8, 2025
Co-authored-by: Guilherme Gazzo <[email protected]>
sampaiodiego
added a commit
that referenced
this pull request
Oct 9, 2025
This reverts commit e748342.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit