regression(native-federation): fix typing loop between instances #37115
Conversation
|
Looks like this PR is ready to merge! 🎉 |
|
|
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. WalkthroughRestricts user-activity notifications to the local instance and simplifies federation typing handling by using the incoming Matrix Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant EB as EventBus
participant L as UserActivityListener
participant N as Notifier
participant C as RoomClients
EB->>L: "user.activity"(roomId, user, isTyping)
rect rgb(230,245,255)
note right of L: Changed — notify only current instance
L->>N: notifyRoomInThisInstance(roomId, 'user-activity', user, tags)
end
N-->>C: deliver event locally
sequenceDiagram
autonumber
participant MX as MatrixEDU
participant H as EduHandler
participant B as TypingBroadcaster
participant RC as RCClients
MX->>H: typing { user_id, room_id, typing }
rect rgb(240,255,240)
note right of H: Changed — use incoming data.user_id directly
H->>B: broadcastTyping(room_id, user_id, typing)
end
B-->>RC: typing indicator update
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
There was a problem hiding this comment.
Pull Request Overview
This PR addresses a typing loop issue in the native federation system by modifying the user activity notification mechanism. The fix prevents infinite loops by changing how typing notifications are broadcasted between federated instances.
Key changes:
- Modified user activity event to use Matrix user ID directly instead of looking up local username
- Changed notification method to restrict broadcasting to current instance only
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| ee/packages/federation-matrix/src/events/edu.ts | Removed Matrix user lookup and uses raw user_id for activity events |
| apps/meteor/server/modules/listeners/listeners.module.ts | Changed notification method to instance-specific broadcasting |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| return; | ||
| } | ||
|
|
||
| const matrixUser = await Users.findOne({ 'federation.mui': data.user_id }); |
There was a problem hiding this comment.
this find was useless since data.user_id is the same as matrixUser.username .. we also don't need to check if it exists since this comes from homeserver which already has done validations
sampaiodiego
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release-7.11.0 #37115 +/- ##
==================================================
- Coverage 67.39% 67.37% -0.02%
==================================================
Files 3328 3328
Lines 113353 113353
Branches 20565 20564 -1
==================================================
- Hits 76389 76368 -21
- Misses 34359 34383 +24
+ Partials 2605 2602 -3
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
sampaiodiego
force-pushed
the
fix-federation-typing
branch
from
03cf03a to
04eb044
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: 1
📜 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 (2)
apps/meteor/server/modules/listeners/listeners.module.ts(1 hunks)ee/packages/federation-matrix/src/events/edu.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/meteor/server/modules/listeners/listeners.module.ts
⏰ 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). (4)
- GitHub Check: 📦 Build Packages
- GitHub Check: Builds matrix rust bindings against alpine
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (1)
ee/packages/federation-matrix/src/events/edu.ts (1)
33-82: Presence handler looks correct.The presence event handler properly validates that the user exists and is federated before processing, maps Matrix presence states to Rocket.Chat statuses, avoids redundant updates, and includes appropriate error handling.
| void api.broadcast('user.activity', { | ||
| user: matrixUser.username, | ||
| user: data.user_id, | ||
| isTyping: data.typing, | ||
| roomId: matrixRoom._id, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
Verify downstream consumers and local user filtering.
The typing handler now uses data.user_id directly without validating that the user exists or is federated, unlike the presence handler (lines 39-48) which performs these checks. This raises two concerns:
-
Inconsistent validation pattern: The presence handler validates the user is federated before processing, but typing does not. If Matrix sends typing events for local users back to the local instance, this could cause the self-typing issue described in the PR objectives.
-
Downstream consumer compatibility: The previous code used
matrixUser.username(a local username), now it usesdata.user_id(a Matrix user ID). Verify that consumers of theuser.activitybroadcast can handle Matrix user ID format.
Run the following script to check how user.activity events are consumed:
Additionally, clarify:
- Does the Matrix homeserver send typing events for local users back to the local instance, or only for remote users?
- Is the filtering of local users handled by the
notifyRoomInThisInstancechange mentioned in the AI summary?
🏁 Script executed:
#!/bin/bash
# Find all consumers of 'user.activity' broadcast events
rg -n -C5 "on\('user\.activity'" --type=ts --type=js
# Also search for any user.activity string references
rg -n -C3 "'user\.activity'|\"user\.activity\"" --type=ts --type=jsLength of output: 1732
🏁 Script executed:
#!/bin/bash
set -e
# Show context around the onEvent handler in listeners.module.ts
sed -n '150,230p' apps/meteor/server/modules/listeners/listeners.module.ts
# Find import of notifications
grep -R "import .*notifications" -n apps/meteor/server/modules/listeners/listeners.module.ts
# Search for definition of notifyRoomInThisInstance
rg -n "notifyRoomInThisInstance" -C5 apps/meteor/server/modulesLength of output: 9073
Restore user validation and map data.user_id to a local user ID before broadcasting (ee/packages/federation-matrix/src/events/edu.ts L23-27)
The presence handler still looks up and validates the federated user before emitting, but the typing handler now sends the raw Matrix user ID—downstream consumers rely on internal user IDs to filter out self-typing and drive UI logic correctly.
🤖 Prompt for AI Agents
In ee/packages/federation-matrix/src/events/edu.ts around lines 23–27, the
typing handler currently broadcasts the raw Matrix user ID; restore the
federated-user lookup/validation used in the presence handler and map
data.user_id to the local user record before broadcasting. Specifically,
fetch/validate the local user (using the same helper used by presence, e.g. the
federated user lookup), bail out if the federated user is not found/invalid, and
pass user: localUser._id (not data.user_id) in the api.broadcast payload; keep
existing error handling/logging when lookup fails.
Proposed changes (including videos or screenshots)
Issue(s)
FDR-181
Steps to test or reproduce
Further comments
Summary by CodeRabbit