-
Notifications
You must be signed in to change notification settings - Fork 12.6k
fix(federation): previous states on initial state and remove emitter #37677
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
base: feat/invites
Are you sure you want to change the base?
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 |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. ✨ 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 |
ae0a9a2 to
88070a2
Compare
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 pull request refactors the federation-matrix package to use the SDK's built-in event emitter service instead of maintaining a local Emitter instance. The changes also improve error messages by adding contextual information.
Key Changes
- Removes local
Emitterinstantiation and passes the SDK'seventEmitterServiceto event handlers instead - Updates all event handler functions to access
federationSDK.eventEmitterServicedirectly - Improves error messages in room event handlers by including
roomIdanduserIdcontext - Changes room name event handler to use
senderfield instead ofstate_key
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| ee/packages/federation-matrix/src/setup.ts | Removes local Emitter instantiation and unused imports, simplifies initialization |
| ee/packages/federation-matrix/src/events/index.ts | Updates function signatures to remove emitter parameter |
| ee/packages/federation-matrix/src/events/room.ts | Changes event emitter usage, switches from state_key to sender field, improves error messages |
| ee/packages/federation-matrix/src/events/reaction.ts | Updates to use SDK's event emitter service, removes unused imports |
| ee/packages/federation-matrix/src/events/ping.ts | Updates to use SDK's event emitter service |
| ee/packages/federation-matrix/src/events/message.ts | Updates to use SDK's event emitter service, consolidates imports |
| ee/packages/federation-matrix/src/events/member.ts | Updates to use SDK's event emitter service, reorganizes imports |
| ee/packages/federation-matrix/src/events/edu.ts | Updates to use SDK's event emitter service |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| throw new Error(`mapped room not found: ${roomId}`); | ||
| } | ||
|
|
||
| const localUserId = await Users.findOneByUsername(userId, { projection: { _id: 1 } }); |
Copilot
AI
Dec 3, 2025
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.
The sender field in Matrix events contains the full Matrix user ID (format: @username:server.domain), but Users.findOneByUsername() expects only the username part. This will cause the user lookup to fail. Use the getUsernameServername utility function to extract the username, similar to how it's done in the room.role event handler (line 63).
| room_id: roomId, | ||
| content: { name }, | ||
| state_key: userId, | ||
| sender: userId, |
Copilot
AI
Dec 3, 2025
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.
The variable name userId is misleading as it contains the full Matrix user ID from sender (format: @username:server.domain), not just a local user ID. Consider renaming to senderId or senderMatrixId for clarity, and then use getUsernameServername() to extract the actual username before calling Users.findOneByUsername().
88070a2 to
32b5876
Compare
dba8733 to
85183c9
Compare
85183c9 to
aa7354e
Compare
ea084df to
b57caa1
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feat/invites #37677 +/- ##
===============================================
Coverage ? 54.17%
===============================================
Files ? 2638
Lines ? 50034
Branches ? 11198
===============================================
Hits ? 27107
Misses ? 20781
Partials ? 2146
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
82d2031 to
9525467
Compare
…rvice and remove Emitter dependency
…sure proper member and message reflection
9525467 to
7204642
Compare
https://rocketchat.atlassian.net/browse/FB-52
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments