fix(federation): leave a room is not propagating #37206
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 |
|
There was a problem hiding this comment.
Pull Request Overview
This PR reverts a previous change that was preventing federation room leave operations from propagating properly. The fix removes a subscription check that was incorrectly blocking legitimate leave room operations in the federation matrix service.
- Removes subscription validation that was preventing room leave operations from executing
- Allows federation room leave events to propagate correctly by removing the early return condition
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
WalkthroughAdded an optional Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant RC as Rocket.Chat
participant Callbacks as afterLeaveRoomCallback
participant FM as FederationMatrix
participant HS as Homeserver
User->>RC: leave room (local action / kick)
RC->>Callbacks: afterLeaveRoomCallback.run({ user, kicker? }, room)
Callbacks->>FM: leaveRoom(roomId, user, kicker?)
alt kicker is federated (remote)
Note right of FM #f6f8fa: debug log and ignore remote kicker
FM-->>RC: no-op (ignored)
else room is federated & kicker allowed
FM->>HS: matrix.leave(roomId, user)
HS-->>FM: result (success / error)
FM-->>RC: propagate result
end
RC-->>User: acknowledge leave (or no-op)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (2)
🧰 Additional context used🧬 Code graph analysis (2)apps/meteor/app/lib/server/functions/removeUserFromRoom.ts (1)
apps/meteor/lib/callbacks/afterLeaveRoomCallback.ts (1)
⏰ 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)
🔇 Additional comments (2)
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 @@
## release-7.11.0 #37206 +/- ##
===============================================
Coverage 66.37% 66.38%
===============================================
Files 3386 3386
Lines 115619 115619
Branches 21351 21359 +8
===============================================
+ Hits 76739 76748 +9
+ Misses 36275 36265 -10
- Partials 2605 2606 +1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ee/packages/federation-matrix/src/FederationMatrix.ts (1)
736-740: Consider logging kicker details for ignored removalsWhen we drop a remote-initiated removal, having the kicker’s identifiers in the debug line would make federation investigations easier without altering behavior.
- if (kicker && isUserNativeFederated(kicker)) { - this.logger.debug('Only local users can remove others, ignoring action'); + if (kicker && isUserNativeFederated(kicker)) { + this.logger.debug('Only local users can remove others, ignoring action', { + kicker: kicker.username ?? kicker._id, + target: user.username ?? user._id, + roomId, + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
apps/meteor/app/lib/server/functions/removeUserFromRoom.ts(1 hunks)apps/meteor/ee/server/hooks/federation/index.ts(1 hunks)apps/meteor/lib/callbacks/afterLeaveRoomCallback.ts(1 hunks)ee/packages/federation-matrix/src/FederationMatrix.ts(1 hunks)packages/core-services/src/types/IFederationMatrixService.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
apps/meteor/lib/callbacks/afterLeaveRoomCallback.ts (3)
apps/meteor/lib/callbacks/callbacksBase.ts (1)
Callbacks(24-234)packages/core-typings/src/IUser.ts (1)
IUser(186-255)packages/core-typings/src/IRoom.ts (1)
IRoom(21-95)
apps/meteor/app/lib/server/functions/removeUserFromRoom.ts (1)
apps/meteor/lib/callbacks/afterLeaveRoomCallback.ts (1)
afterLeaveRoomCallback(5-5)
ee/packages/federation-matrix/src/FederationMatrix.ts (1)
packages/core-typings/src/IUser.ts (2)
IUser(186-255)isUserNativeFederated(276-277)
apps/meteor/ee/server/hooks/federation/index.ts (2)
packages/core-typings/src/IRoom.ts (1)
IRoom(21-95)ee/packages/federation-matrix/src/FederationMatrix.ts (1)
FederationMatrix(137-1006)
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-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 (4)
packages/core-services/src/types/IFederationMatrixService.ts (1)
13-13: Interface signature aligned with new payloadExtending
leaveRoomto accept the optionalkickerkeeps the service contract in sync with the new callback shape, so implementing classes can act on the extra context.apps/meteor/lib/callbacks/afterLeaveRoomCallback.ts (1)
5-5: Callback payload matches federation flowTyping the first argument as
{ user, kicker? }mirrors how callers now package the context and keeps the callbacks API coherent.apps/meteor/app/lib/server/functions/removeUserFromRoom.ts (1)
75-77: Propagating kicker context is spot onPassing
{ user, kicker: options?.byUser }gives downstream hooks the info they need to differentiate self-leave from removals without touching the surrounding flow.apps/meteor/ee/server/hooks/federation/index.ts (1)
124-127: Handler update keeps federation bridge informedDestructuring the callback payload and forwarding
kickertoleaveRoommakes sure the bridge can differentiate self-leaves from removals without disturbing existing guard logic.
https://rocketchat.atlassian.net/browse/FDR-236
Reverts 3a85856
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit