-
Notifications
You must be signed in to change notification settings - Fork 13k
fix(federation): DM creation sending invite twice #37222
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Looks like this PR is ready to merge! 🎉 |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes a bug in DM creation where federation invite messages were being sent twice. The fix involves correcting the placement of a closing brace that was causing the member invitation loop to execute outside of its intended conditional block.
Key changes:
- Fixed brace placement in the DM creation logic to properly scope the member invitation loop
Comments suppressed due to low confidence (1)
ee/packages/federation-matrix/src/FederationMatrix.ts:1
- The closing brace was moved to line 337, but this creates a logical issue. The member invitation loop (lines 318-338) should only execute for group DMs, but now it will execute for all DM types. The loop should be properly scoped within the group DM conditional block that starts around line 314.
import { type IFederationMatrixService, ServiceClass } from '@rocket.chat/core-services';
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
WalkthroughRefactors group DM creation in ee/packages/federation-matrix/src/FederationMatrix.ts by inlining the per-member invitation loop under the group-room path and wrapping each invite in its own try-catch to log per-user errors without aborting the loop. One-on-one DM flow remains unchanged. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant FederationMatrix
participant MatrixServer as Matrix Server
participant Member as Federated Member(s)
Client->>FederationMatrix: createDirectMessageRoom(members)
alt 1-on-1 DM
FederationMatrix->>MatrixServer: createRoom(1:1)
MatrixServer-->>FederationMatrix: roomId
FederationMatrix->>MatrixServer: invite(otherUser)
MatrixServer-->>FederationMatrix: inviteResult
FederationMatrix-->>Client: roomId
else Group DM
FederationMatrix->>MatrixServer: createRoom(group)
MatrixServer-->>FederationMatrix: roomId
loop for each member
note right of FederationMatrix: try { invite(member) } catch { log(memberError) }
FederationMatrix->>MatrixServer: invite(member)
MatrixServer-->>FederationMatrix: inviteResult or error
FederationMatrix-->>FederationMatrix: log per-user error (non-blocking)
end
FederationMatrix-->>Client: roomId
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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 (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)ee/packages/federation-matrix/src/FederationMatrix.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 (1)
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 #37222 +/- ##
==================================================
- Coverage 66.39% 66.36% -0.03%
==================================================
Files 3386 3386
Lines 115618 115618
Branches 21352 21356 +4
==================================================
- Hits 76765 76733 -32
- Misses 36251 36279 +28
- Partials 2602 2606 +4
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit