chore(federation): refactor edu handling #37013
chore(federation): refactor edu handling #37013sampaiodiego merged 9 commits intochore/federation-backupfrom
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 |
|
WalkthroughConsolidates typing/presence signaling into a single Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Streamer as StreamerCentral
participant EEStart as EE Startup
participant FedSvc as FederationMatrix
participant EDU as Federation EDU
Client->>Streamer: broadcast('notify-room', '.../user-activity/<rid>', [user, activity])
Streamer-->>EEStart: notify-room event (eventName endsWith 'user-activity')
EEStart->>EEStart: parse rid, user, isTyping = activity.includes('user-typing')
EEStart->>FedSvc: notifyUserTyping(rid, user, isTyping)
FedSvc->>FedSvc: resolve local/external room & user mappings
FedSvc->>EDU: send typing EDU to remote homeservers
EDU-->>FedSvc: ack / response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
ggazzo
force-pushed
the
chore/feat-typing-fed
branch
from
a19db18 to
d8bd435
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/ee/server/hooks/federation/index.ts (1)
231-244: Missing ‘user-activity’ emission on local streamer
The listener at apps/meteor/ee/server/hooks/federation/index.ts:231 never fires because NotificationsModule (→ apps/meteor/server/modules/notifications/notifications.module.ts:471–474) is never emitting a'user-activity'event on itsstreamLocal. Add anotifications.streamLocal.emit('user-activity', { rid, username, activities })(e.g. whennotifyRoomhandles a typing event) or switch the hook to subscribe tostreamRoomfor${rid}/user-activity. After making this change, verify the end-to-end flow.
🧹 Nitpick comments (1)
ee/packages/federation-matrix/src/events/edu.ts (1)
75-77: Optionally include previousStatus if availableIf you can read the prior status cheaply, pass it to reduce downstream lookups; otherwise fine to keep undefined.
📜 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 (7)
apps/meteor/ee/server/hooks/federation/index.ts(2 hunks)apps/meteor/ee/server/index.ts(0 hunks)apps/meteor/server/modules/listeners/listeners.module.ts(1 hunks)apps/meteor/server/modules/notifications/notifications.module.ts(3 hunks)ee/packages/federation-matrix/src/events/edu.ts(2 hunks)packages/core-services/src/events/Events.ts(1 hunks)packages/ddp-client/src/types/streams.ts(2 hunks)
💤 Files with no reviewable changes (1)
- apps/meteor/ee/server/index.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-19T15:15:04.613Z
Learnt from: rodrigok
PR: RocketChat/Rocket.Chat#36991
File: apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts:219-221
Timestamp: 2025-09-19T15:15:04.613Z
Learning: The Federation_Matrix_homeserver_domain setting in apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts is part of the old federation system and is being deprecated/removed, so configuration issues with this setting should not be flagged for improvement.
Applied to files:
apps/meteor/ee/server/hooks/federation/index.ts
🧬 Code graph analysis (2)
apps/meteor/ee/server/hooks/federation/index.ts (1)
packages/core-services/src/index.ts (1)
License(160-160)
apps/meteor/server/modules/notifications/notifications.module.ts (1)
apps/meteor/client/lib/cachedStores/CachedStore.ts (1)
eventName(75-80)
⏰ 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 (6)
packages/ddp-client/src/types/streams.ts (2)
47-49: notify-room user-activity typing is consistentSignature
[username, activities]matches server emit usage.
574-583: Local user-activity event is well-defined but depends on server emitTypes align with intended payload. Ensure NotificationsModule emits to streamLocal (see suggested fix there).
Consider a literal union for activities to prevent typos:
type UserActivity = 'user-typing'; activities: UserActivity[];ee/packages/federation-matrix/src/events/edu.ts (1)
31-35: Typing → user-activity broadcast LGTMMinimal, correct mapping.
apps/meteor/server/modules/listeners/listeners.module.ts (2)
154-175: Presence handler inlined correctlyEmits userData, user-status, and presence with proper STATUS_MAP.
183-185: user-activity → notifyRoom wiring looks correctMatches ddp-client signature; pairs with server-side forwarder fix in NotificationsModule.
packages/core-services/src/events/Events.ts (1)
153-153: Approve — new user-activity event signature is correctVerified: repo search shows only expected usages. service.onEvent('user-activity') in apps/meteor/server/modules/listeners/listeners.module.ts already adapts the new { username, isTyping, roomId } payload to the existing notify-room stream (username, activities[]). Remaining occurrences are the stream/client shapes (packages/ddp-client, packages/livechat), the federation bridge (ee/packages/federation-matrix/src/events/edu.ts) and notifications handlers — all expected; no legacy producers require changes.
| public readonly inner = new Emitter<InnerEvents>(); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Undefined type and un-wired bus: InnerEvents is missing and inner isn’t bridged to streamLocal
TypeScript will error (InnerEvents not in scope). Also, without forwarding to streamLocal, listeners on notifications.streamLocal('user-activity') will never fire.
Apply this diff to type payloads and forward internally:
- public readonly inner = new Emitter<InnerEvents>();
+ public readonly inner = new Emitter<InnerEvents>();And update notifyRoom to emit properly and bridge to streamLocal:
- this.inner.emit(eventName, {
- rid: room,
- eventName,
- args,
- });
+ if (eventName === 'user-activity') {
+ const [username, activities] = args as [string, string[]];
+ this.inner.emit('user-activity', { rid: room, username, activities });
+ this.streamLocal.emitWithoutBroadcast('user-activity', { rid: room, username, activities });
+ }Add this type (outside changed ranges, e.g., after the class):
type InnerEvents = {
'user-activity': [{ rid: string; username: string; activities: string[] }];
};Run to confirm no remaining gaps:
🏁 Script executed:
#!/bin/bash
rg -nP '(\btype|\binterface)\s+InnerEvents\b'
rg -n "inner\\.on\\('user-activity'"
rg -n "streamLocal\\.emit.*user-activity" -n -C2Length of output: 0
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "PWD: $(pwd)"
echo
echo "1) Locate notifications.module.ts files"
find . -type f -iname 'notifications.module.ts' -print || true
echo
file=$(find . -type f -iname 'notifications.module.ts' -print -quit || true)
if [ -n "$file" ]; then
echo "2) Showing $file (first 300 lines)"
nl -ba "$file" | sed -n '1,300p' || true
else
echo "2) No notifications.module.ts file found"
fi
echo
echo "3) Search for 'InnerEvents' type/interface declarations"
rg -nP '(\btype|\binterface)\s+InnerEvents\b' --hidden || true
echo
echo "4) Search for usages: inner.on('user-activity')"
rg -n "inner\.on\('user-activity'" --hidden || true
echo
echo "5) Search for streamLocal emit of user-activity"
rg -n "streamLocal\.emit.*user-activity" --hidden -n -C2 || true
echo
echo "6) Search for 'inner = new Emitter' occurrences"
rg -n "inner\s*=\s*new\s+Emitter" --hidden -S || true
echo
echo "7) Search for function/method 'notifyRoom'"
rg -n "notifyRoom" --hidden -n -C5 || trueLength of output: 21275
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file=apps/meteor/server/modules/notifications/notifications.module.ts
echo "File: $file"
echo "---- head (1-140) ----"
cat -n "$file" | sed -n '1,140p' || true
echo
echo "---- around inner (40-80) ----"
cat -n "$file" | sed -n '40,80p' || true
echo
echo "---- notifyRoom area (470-520) ----"
cat -n "$file" | sed -n '470,520p' || true
echo
echo "---- search for streamLocal, streamRoom, streamLogged in repo ----"
rg -n "streamLocal|streamRoom|streamLogged" --hidden --no-ignore || true
echo
echo "---- search for InnerEvents type/interface ----"
rg -nP '(\btype|\binterface)\s+InnerEvents\b' --hidden --no-ignore || true
echo
echo "---- search for usages of inner.on / inner.emit ----"
rg -n "inner\.on|inner\.emit" --hidden --no-ignore || trueLength of output: 16948
Missing InnerEvents type + bridge 'user-activity' to streamLocal (TypeScript error & listeners won't fire)
InnerEvents is not declared (TypeScript will error) and notifyRoom only emits to inner/streamRoom — it never forwards the typed 'user-activity' payload to streamLocal where code listens (e.g. apps/meteor/ee/server/hooks/federation/index.ts). Fix both: declare the type and forward the event.
- Add this type (e.g. after the class):
type InnerEvents = Record<string, any[]> & {
'user-activity': [{ rid: string; username: string; activities: string[] }];
};- Replace the current inner.emit(...) in notifyRoom with a special-case forward to streamLocal:
if (eventName === 'user-activity') {
const [username, activities] = args as [string, string[]];
const payload = { rid: room, username, activities };
this.inner.emit('user-activity', payload);
this.streamLocal.emitWithoutBroadcast('user-activity', payload);
} else {
this.inner.emit(eventName, {
rid: room,
eventName,
args,
});
}Locations: apps/meteor/server/modules/notifications/notifications.module.ts — inner declaration (line ~51) and notifyRoom (around lines 488–499).
🤖 Prompt for AI Agents
In apps/meteor/server/modules/notifications/notifications.module.ts around lines
51 and 488–499, declare the missing InnerEvents type (e.g. a Record with a
specific 'user-activity' signature: { rid: string; username: string; activities:
string[] }) immediately after the class, and modify notifyRoom so that when
eventName === 'user-activity' you build a payload { rid: room, username,
activities } from the args, emit it to this.inner with the 'user-activity' key
and also forward it to this.streamLocal via emitWithoutBroadcast; for all other
events keep emitting to this.inner with the generic { rid, eventName, args }
shape.
| this.inner.emit(eventName, { | ||
| rid: room, | ||
| eventName, | ||
| args, | ||
| }); |
There was a problem hiding this comment.
Emit only for user-activity and use the correct shape
Guard by event name and pass { rid, username, activities } instead of a generic envelope.
Apply the diff above in the previous comment.
🤖 Prompt for AI Agents
In apps/meteor/server/modules/notifications/notifications.module.ts around lines
493-497, limit emission to only the "user-activity" event and change the emitted
payload shape: check if eventName === 'user-activity' before calling
this.inner.emit, extract username and activities from args (or default them) and
call this.inner.emit(eventName, { rid: room, username, activities }) instead of
the generic envelope containing eventName/args; ensure types/null-safety for
username/activities when extracting.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## chore/federation-backup #37013 +/- ##
===========================================================
+ Coverage 67.33% 67.35% +0.01%
===========================================================
Files 3342 3342
Lines 113397 113398 +1
Branches 20704 20663 -41
===========================================================
+ Hits 76361 76380 +19
+ Misses 34435 34421 -14
+ Partials 2601 2597 -4
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
chore/feat-typing-fed
branch
from
d8bd435 to
db08115
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/core-services/src/events/Events.ts (1)
153-153: Consider dot-separated naming for consistencyMost events use dot notation (e.g., user.typing, presence.status). Consider 'user.activity' unless there's a strong reason to keep a hyphen.
Apply this diff (if you opt to align):
- 'user-activity'(data: { userId: IUser['_id']; username?: IUser['username']; isTyping: boolean; roomId: IRoom['_id'] }): void; + 'user.activity'(data: { userId: IUser['_id']; username?: IUser['username']; isTyping: boolean; roomId: IRoom['_id'] }): void;
📜 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 (7)
apps/meteor/ee/server/hooks/federation/index.ts(2 hunks)apps/meteor/ee/server/index.ts(0 hunks)apps/meteor/server/modules/listeners/listeners.module.ts(1 hunks)apps/meteor/server/modules/notifications/notifications.module.ts(1 hunks)ee/packages/federation-matrix/src/events/edu.ts(2 hunks)packages/core-services/src/events/Events.ts(1 hunks)packages/ddp-client/src/types/streams.ts(2 hunks)
💤 Files with no reviewable changes (1)
- apps/meteor/ee/server/index.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- ee/packages/federation-matrix/src/events/edu.ts
- apps/meteor/ee/server/hooks/federation/index.ts
- packages/ddp-client/src/types/streams.ts
- apps/meteor/server/modules/notifications/notifications.module.ts
- 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). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
| user: Pick<IUser, '_id' | 'username' | 'status' | 'statusText' | 'name' | 'roles'>; | ||
| previousStatus?: UserStatus; | ||
| }): void; | ||
| 'user-activity'(data: { username: string; isTyping: boolean; roomId: string }): void; |
There was a problem hiding this comment.
Use stable identifiers: prefer userId over username in the event payload
Usernames can change and are not globally unique across federated spaces. Emit a stable ID and keep username optional for UI.
Apply this diff:
- 'user-activity'(data: { username: string; isTyping: boolean; roomId: string }): void;
+ 'user-activity'(data: { userId: IUser['_id']; username?: IUser['username']; isTyping: boolean; roomId: IRoom['_id'] }): 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.
| 'user-activity'(data: { username: string; isTyping: boolean; roomId: string }): void; | |
| 'user-activity'(data: { userId: IUser['_id']; username?: IUser['username']; isTyping: boolean; roomId: IRoom['_id'] }): void; |
🤖 Prompt for AI Agents
In packages/core-services/src/events/Events.ts around line 153, the
'user-activity' event currently requires username but should use a stable userId
instead; update the event signature to accept data: { userId: string; username?:
string; isTyping: boolean; roomId: string } (userId required, username
optional), and then update all emitters and listeners across the codebase to
pass and consume userId rather than username while keeping username as an
optional UI-only field for backward compatibility.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/ee/server/startup/federation.ts (1)
47-60: Unregister StreamerCentral 'broadcast' listener on stop (apps/meteor/ee/server/startup/federation.ts)Capture the broadcast handler in a const when calling StreamerCentral.on(...) and call StreamerCentral.off('broadcast', handler) inside stopService before destroying the service to avoid callbacks running with federationMatrixService === undefined. (app.unpkg.com)
🧹 Nitpick comments (2)
ee/packages/federation-matrix/src/FederationMatrix.ts (2)
118-151: Presence EDU dispatch looks correct; small guard to avoid no‑op calls.Skip the EDU call when there are no federated rooms for the user. Keeps hot path a bit cleaner.
- const roomsUserIsMemberOf = await Subscriptions.findUserFederatedRoomIds(localUser._id).toArray(); + const roomsUserIsMemberOf = await Subscriptions.findUserFederatedRoomIds(localUser._id).toArray(); + const roomIds = roomsUserIsMemberOf.map(({ externalRoomId }) => externalRoomId).filter(Boolean); + if (roomIds.length === 0) { + return; + } @@ - void instance.homeserverServices.edu.sendPresenceUpdateToRooms( + void instance.homeserverServices.edu.sendPresenceUpdateToRooms( [ { user_id: externalUserId, presence: statusMap[user.status] || 'offline', }, ], - roomsUserIsMemberOf.map(({ externalRoomId }) => externalRoomId).filter(Boolean), + roomIds, );
989-1007: Harden notifyUserTyping: add service guard + explicit return type (hot path).Prevents NPE if service isn’t ready and clarifies intent.
- async notifyUserTyping(rid: string, user: string, isTyping: boolean) { + async notifyUserTyping(rid: string, user: string, isTyping: boolean): Promise<void> { + if (!this.homeserverServices) { + this.logger.warn('Homeserver services not available, skipping typing notification'); + return; + } if (!rid || !user) { return; }Optional next step: cache recent username→external user id and rid→externalRoomId lookups (small TTL, e.g., 5–10s) to reduce DB hits on bursty typing events.
📜 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 (3)
apps/meteor/ee/server/hooks/federation/index.ts(1 hunks)apps/meteor/ee/server/startup/federation.ts(2 hunks)ee/packages/federation-matrix/src/FederationMatrix.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/meteor/ee/server/hooks/federation/index.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-19T15:15:04.642Z
Learnt from: rodrigok
PR: RocketChat/Rocket.Chat#36991
File: apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts:219-221
Timestamp: 2025-09-19T15:15:04.642Z
Learning: The Federation_Matrix_homeserver_domain setting in apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts is part of the old federation system and is being deprecated/removed, so configuration issues with this setting should not be flagged for improvement.
Applied to files:
apps/meteor/ee/server/startup/federation.ts
🧬 Code graph analysis (2)
ee/packages/federation-matrix/src/FederationMatrix.ts (1)
packages/models/src/index.ts (3)
MatrixBridgedRoom(216-216)Users(209-209)MatrixBridgedUser(217-217)
apps/meteor/ee/server/startup/federation.ts (1)
apps/meteor/client/lib/cachedStores/CachedStore.ts (1)
eventName(75-80)
sampaiodiego
force-pushed
the
chore/feat-typing-fed
branch
from
ccb067f to
567a4e0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/ee/server/hooks/federation/index.ts (1)
183-197: Prevent edit-loop: skip Matrix‑origin edits and remove no-op callEdits coming from Matrix can be echoed back because this handler doesn’t guard against
message.federation?.eventIdor external users. Also, Line 190 is a dead call.Apply:
callbacks.add( 'afterSaveMessage', async (message: IMessage, { room }) => { if (FederationActions.shouldPerformFederationAction(room)) { - if (!isEditedMessage(message)) { - return; - } - FederationActions.shouldPerformFederationAction(room); - - await FederationMatrix.updateMessage(message._id, message.msg, message.u); + if (!isEditedMessage(message)) { + return; + } + // Skip events that originated from federation to avoid echo/loops + if (message.federation?.eventId || message.u?.username?.includes(':')) { + return; + } + await FederationMatrix.updateMessage(message._id, message.msg, message.u); } }, callbacks.priority.HIGH, 'federation-matrix-after-room-message-updated', );
🧹 Nitpick comments (3)
apps/meteor/ee/server/hooks/federation/index.ts (3)
93-95: Fix trailing space in callback idTrailing space can create inconsistent IDs and hinder deduplication.
- 'native-federation-on-add-users-to-room ', + 'native-federation-on-add-users-to-room',
1-1: Clean up unused imports
api,License,settings, andnotificationsappear unused here. Remove to keep TS noUnusedLocals happy and reduce bundle noise.-import { api, FederationMatrix } from '@rocket.chat/core-services'; +import { FederationMatrix } from '@rocket.chat/core-services'; -import { License } from '@rocket.chat/license'; -import notifications from '../../../../app/notifications/server/lib/Notifications'; -import { settings } from '../../../../app/settings/server';Also applies to: 3-3, 6-6, 7-7
82-95: Prevent duplicate federation invites — make invites idempotent or guard the hooksVerified: 'federation.onAddUsersToRoom' is invoked from apps/meteor/app/lib/server/methods/addUsersToRoom.ts (bulk), while 'beforeAddUserToRoom' is invoked from apps/meteor/app/lib/server/functions/addUserToRoom.ts and apps/meteor/app/lib/server/functions/createRoom.ts (single/create-room). Either dedupe inside FederationMatrix.inviteUsersToRoom (recommended) or add guards so the same user+room invite isn't sent twice.
📜 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 (6)
apps/meteor/ee/server/hooks/federation/index.ts(1 hunks)apps/meteor/ee/server/startup/federation.ts(2 hunks)apps/meteor/server/modules/listeners/listeners.module.ts(1 hunks)ee/packages/federation-matrix/src/FederationMatrix.ts(2 hunks)ee/packages/federation-matrix/src/events/edu.ts(2 hunks)packages/core-services/src/events/Events.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/meteor/server/modules/listeners/listeners.module.ts
- apps/meteor/ee/server/startup/federation.ts
- ee/packages/federation-matrix/src/events/edu.ts
- ee/packages/federation-matrix/src/FederationMatrix.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-19T15:15:04.642Z
Learnt from: rodrigok
PR: RocketChat/Rocket.Chat#36991
File: apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts:219-221
Timestamp: 2025-09-19T15:15:04.642Z
Learning: The Federation_Matrix_homeserver_domain setting in apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts is part of the old federation system and is being deprecated/removed, so configuration issues with this setting should not be flagged for improvement.
Applied to files:
apps/meteor/ee/server/hooks/federation/index.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). (2)
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (2)
packages/core-services/src/events/Events.ts (2)
152-152: Use explicit identifiers and types; add optional origin to prevent loopsRename
usertouserId, type room/user ids precisely, and include an optionaloriginto help listeners avoid re-broadcast loops.- 'user.activity'(data: { user: string; isTyping: boolean; roomId: string }): void; + 'user.activity'(data: { + userId: IUser['_id']; + roomId: IRoom['_id']; + isTyping: boolean; + origin?: 'local' | 'federation'; + }): void;This also aligns naming with nearby events (e.g.,
user.video-conferenceusesuserId).
152-152: Gate producers/consumers to the new signature — verifiedFound and verified payload uses { user, isTyping, roomId } in:
- ee/packages/federation-matrix/src/events/edu.ts (producer)
- apps/meteor/server/modules/listeners/listeners.module.ts (consumer)
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/meteor/ee/server/hooks/federation/index.ts (2)
38-38: Potential crash: user may be undefined.Guard
userwhen checking for external usernames.Apply:
- const shouldBeHandledByFederation = room.federated === true || user.username?.includes(':'); + const shouldBeHandledByFederation = room.federated === true || user?.username?.includes(':');
182-191: Missing loop-prevention and redundant call in edit path.
- Edits originating from Matrix can loop back; skip external users and require
message.federation.eventId.- Remove the no-op duplicate
shouldPerformFederationActioncall.- Add error handling (align with send path).
Apply:
callbacks.add( 'afterSaveMessage', async (message: IMessage, { room }) => { if (FederationActions.shouldPerformFederationAction(room)) { if (!isEditedMessage(message)) { return; } - FederationActions.shouldPerformFederationAction(room); - - await FederationMatrix.updateMessage(message._id, message.msg, message.u); + // Skip updates coming from Matrix (external) to avoid loops + if (message.u?.username?.includes(':')) { + return; + } + // Only federate edits for messages that were federated before + if (!message.federation?.eventId) { + return; + } + try { + await FederationMatrix.updateMessage(message._id, message.msg, message.u); + } catch (error) { + console.error('[updateMessage] Failed to federate message edit:', { mid: message._id, rid: message.rid, error }); + } } }, callbacks.priority.HIGH, 'federation-matrix-after-room-message-updated', );
🧹 Nitpick comments (3)
apps/meteor/ee/server/hooks/federation/index.ts (3)
49-52: Use structured logger instead of console.error.Prefer the project logger to keep logs consistent and searchable.
Example:
- console.error('[sendMessage] Failed to send message to Native Federation:', error); + logger.error({ msg: '[sendMessage] Failed to send message to Native Federation', err: error, mid: message._id, rid: room._id });If
loggerisn’t available here, follow the repo’s standard logging pattern for server hooks.
26-28: Fragile Matrix roomId parsing.
split(':')[1]can be undefined if the format is unexpected. Add a safe parse + guard.Apply:
- const fromServer = federatedRoomId.split(':')[1]; - await MatrixBridgedRoom.createOrUpdateByLocalRoomId(room._id, federatedRoomId, fromServer); + const parts = federatedRoomId.split(':', 2); + const fromServer = parts.length === 2 ? parts[1] : undefined; + if (!fromServer) { + console.warn('[federation] Invalid federatedRoomId, expected "!id:server"', { federatedRoomId, rid: room._id }); + return; + } + await MatrixBridgedRoom.createOrUpdateByLocalRoomId(room._id, federatedRoomId, fromServer);
91-91: Trailing space in callback id.The id string has a trailing space, which can break deduplication/removal of the handler.
Apply:
- 'native-federation-on-add-users-to-room ', + 'native-federation-on-add-users-to-room',
📜 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/ee/server/hooks/federation/index.ts(1 hunks)apps/meteor/ee/server/startup/federation.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/meteor/ee/server/startup/federation.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-19T15:15:04.642Z
Learnt from: rodrigok
PR: RocketChat/Rocket.Chat#36991
File: apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts:219-221
Timestamp: 2025-09-19T15:15:04.642Z
Learning: The Federation_Matrix_homeserver_domain setting in apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts is part of the old federation system and is being deprecated/removed, so configuration issues with this setting should not be flagged for improvement.
Applied to files:
apps/meteor/ee/server/hooks/federation/index.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). (2)
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (1)
apps/meteor/ee/server/hooks/federation/index.ts (1)
1-1: FederationMatrix import validated — OK.Verified packages/core-services/index.ts exports:
export const FederationMatrix = proxify<IFederationMatrixService>('federation-matrix');— importing from@rocket.chat/core-servicesis correct.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/core-services/src/types/IFederationMatrixService.ts (1)
38-38: Disambiguateuser(id vs username); avoid plainstring.If this expects a user id, type it as
IUser['_id'](and consider naminguserId). If it’s a username, rename tousername. If both are acceptable, either document clearly or type asPick<IUser, '_id' | 'username'>.Example if using id:
- notifyUserTyping(rid: IRoomFederated['_id'], user: string, isTyping: boolean): Promise<void>; + notifyUserTyping(rid: IRoomFederated['_id'], userId: IUser['_id'], isTyping: boolean): Promise<void>;
📜 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/ee/server/startup/federation.ts(2 hunks)packages/core-services/src/types/IFederationMatrixService.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/meteor/ee/server/startup/federation.ts
🔇 Additional comments (1)
packages/core-services/src/types/IFederationMatrixService.ts (1)
38-38: Verified all implementers have the new method - no changes needed.The verification confirms that:
- Only one class implements
IFederationMatrixService: theFederationMatrixclass inee/packages/federation-matrix/src/FederationMatrix.ts- This implementation already includes the
notifyUserTypingmethod (line 989)- The method is properly implemented and called from the federation startup code (line 38 in
apps/meteor/ee/server/startup/federation.ts)- No test mocks or other implementations were found that would break due to the interface change
The interface addition is complete and all implementers are properly updated.
| role: 'moderator' | 'owner' | 'leader' | 'user', | ||
| ): Promise<void>; | ||
| inviteUsersToRoom(room: IRoomFederated, usersUserName: string[], inviter: Pick<IUser, '_id' | 'username'>): Promise<void>; | ||
| notifyUserTyping(rid: string, user: string, isTyping: boolean): Promise<void>; |
There was a problem hiding this comment.
Type rid as IRoomFederated['_id'] for consistency and stronger type safety.
Other methods (e.g., leaveRoom, updateRoomName/Topic) use IRoomFederated['_id']. Keep this consistent.
- notifyUserTyping(rid: string, user: string, isTyping: boolean): Promise<void>;
+ notifyUserTyping(rid: IRoomFederated['_id'], user: string, isTyping: boolean): 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.
| notifyUserTyping(rid: string, user: string, isTyping: boolean): Promise<void>; | |
| notifyUserTyping(rid: IRoomFederated['_id'], user: string, isTyping: boolean): Promise<void>; |
🤖 Prompt for AI Agents
In packages/core-services/src/types/IFederationMatrixService.ts around line 38,
the notifyUserTyping method currently types its rid parameter as string; change
it to use IRoomFederated['_id'] for consistency and stronger type safety
(aligning with leaveRoom and updateRoomName/Topic). Update the method signature
to accept rid: IRoomFederated['_id'], ensure IRoomFederated is imported in this
file (or add the import), and adjust any call sites/types if necessary to
satisfy the new type.
Co-authored-by: Diego Sampaio <[email protected]>
Co-authored-by: Diego Sampaio <[email protected]>
2 participants
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
Bug Fixes
Improvements
Refactor