chore: bring improvements from federation branch #36968
Conversation
|
Looks like this PR is ready to merge! 🎉 |
|
|
Caution Review failedThe pull request is closed. WalkthroughIntroduces new and revised callbacks across messaging, room creation, user addition, and role changes; updates callback payloads (e.g., afterSaveMessage now receives user, afterRoomNameChange carries room and userId); replaces federation hooks with local callbacks; adds a federation-enabled hook for clients; tightens types; and removes numerous federation-related unit tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor UI as Client/UI
participant S as Server
participant CB1 as prepareCreateRoomCallback
participant CB2 as beforeCreateRoomCallback
participant Fed as Federation Router
UI->>S: createRoom(type, owner, extraData, options)
S->>CB1: run({ type, extraData })
Note right of CB1: Preparation step
S->>CB2: run({ owner, room })
CB2->>Fed: beforeCreateRoom(room) [conditional]
Note over Fed: Chooses EE or CE path
CB2-->>S: done
S-->>UI: room created
sequenceDiagram
autonumber
participant S as Server
participant CB as afterSaveMessage
participant Hooks as callbacks.run/Async
S->>CB: afterSaveMessage(message, room, user)
CB->>Hooks: run/Async(message, { room, user, roomUpdater })
alt roomUpdater has changes
CB->>S: Rooms.updateFromUpdater(roomUpdater)
end
CB-->>S: IMessage
sequenceDiagram
autonumber
participant API as addUserToRoom API
participant Hook as beforeAddUserToRoom
participant Fed as Federation Hooks
participant Core as Room Ops
API->>Hook: run({ user, inviter? }, room)
Note right of Hook: Validation/guards
Hook-->>API: continue or throw
API->>Fed: onAddUsersToRoom({ invitees/inviter }, room) [when applicable]
API->>Core: add user, subscriptions, msgs
Core-->>API: result
sequenceDiagram
autonumber
participant M as Method
participant Pre as beforeChangeRoomRole
participant Sub as Subscriptions
M->>Pre: run({ fromUserId, userId, roomId, role })
Pre-->>M: ok or error
M->>Sub: add/remove role
Sub-->>M: done
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (5)
Comment |
ggazzo
force-pushed
the
chore/federation-findings
branch
from
4285071 to
b1bd338
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #36968 +/- ##
===========================================
- Coverage 67.11% 66.20% -0.92%
===========================================
Files 3404 3384 -20
Lines 117789 115025 -2764
Branches 21478 21062 -416
===========================================
- Hits 79053 76149 -2904
- Misses 36035 36271 +236
+ Partials 2701 2605 -96
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
ggazzo
force-pushed
the
chore/federation-findings
branch
4 times, most recently
from
3785945 to
1d2816d
Compare
…on in federated rooms
ggazzo
force-pushed
the
chore/federation-findings
branch
2 times, most recently
from
3b0dd23 to
853d75f
Compare
ggazzo
force-pushed
the
chore/federation-findings
branch
from
853d75f to
b6be26c
Compare
ggazzo
force-pushed
the
chore/federation-findings
branch
from
380b360 to
d3b4357
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 (3)
apps/meteor/server/methods/removeRoomOwner.ts (1)
21-37: Parity with addRoomOwner: gate federated role changes on readiness/enabled.Unlike addRoomOwner, this path doesn’t guard federated rooms behind isFederationEnabled/isFederationReady, which can lead to divergent state when federation is misconfigured.
Apply:
import { settings } from '../../app/settings/server'; +import { isFederationReady, isFederationEnabled, FederationMatrixInvalidConfigurationError } from '../services/federation/utils'; export const removeRoomOwner = async (fromUserId: string, rid: string, userId: string): Promise<boolean> => { @@ - if (!(await hasPermissionAsync(fromUserId, 'set-owner', rid)) && !isRoomFederated(room)) { + const isFederated = isRoomFederated(room); + if (!(await hasPermissionAsync(fromUserId, 'set-owner', rid)) && !isFederated) { throw new Meteor.Error('error-not-allowed', 'Not allowed', { method: 'removeRoomOwner', }); } + + if (isFederated && (!isFederationEnabled() || !isFederationReady())) { + throw new FederationMatrixInvalidConfigurationError('unable to change room owners'); + }apps/meteor/server/services/room/service.ts (1)
63-71: Match theaddUserToRoomparameter type to the underlying functionaddUserToRoom (apps/meteor/app/lib/server/functions/addUserToRoom.ts) accepts Pick<IUser, '_id' | 'username'> | string; the service narrows it to Pick<IUser, '_id'> which restricts callers.
File: apps/meteor/server/services/room/service.ts (addUserToRoom)
- user: Pick<IUser, '_id'>, + user: Pick<IUser, '_id' | 'username'> | string,apps/meteor/app/lib/server/functions/createRoom.ts (1)
136-144: Guard against missing extraData in prepareCreateRoomCallback consumerapps/meteor/app/e2e/server/beforeCreateRoom.ts (lines 4–12) — the handler destructures ({ type, extraData }) and does
extraData.encrypted = extraData.encrypted ?? true;, which will throw ifextraDatais undefined. Fix by either ensuring callers always pass an object forextraData(e.g.,{}) or initializing/guardingextraDatain the handler before accessing/modifying it.
♻️ Duplicate comments (1)
apps/meteor/app/lib/server/functions/addUserToRoom.ts (1)
59-63: Preserve error codes from hooks.Wrapping with
new Meteor.Error(message)drops error codes. The diff above switches toerror?.error || 'error-pre-add-user'.
🧹 Nitpick comments (20)
apps/meteor/app/discussion/server/hooks/propagateDiscussionMetadata.ts (1)
90-96: Type the payload and handle empty oldName explicitlyAdd a concrete type for the new callback payload and avoid relying on truthiness for oldName (empty string would currently skip the filter).
-callbacks.add( - 'afterRoomNameChange', - async (roomConfig) => { - const { - room: { _id: rid }, - name, - oldName, - } = roomConfig; +callbacks.add( + 'afterRoomNameChange', + async ({ room, name, oldName }: { room: IRoom; name: string; oldName?: string }) => { + const rid = room._id; - await Rooms.updateMany({ prid: rid, ...(oldName && { topic: oldName }) }, { $set: { topic: name } }); + await Rooms.updateMany({ prid: rid, ...(oldName ? { topic: oldName } : {}) }, { $set: { topic: name } });apps/meteor/app/lib/client/methods/sendMessage.ts (1)
44-47: Narrow user type before passing to callbacks.runEnsure the argument matches the updated signature and avoid nullable type bleed-through.
- return callbacks.run('afterSaveMessage', message, { room, user }); + return callbacks.run('afterSaveMessage', message, { room, user: user as IUser });apps/meteor/server/methods/removeRoomModerator.ts (1)
61-62: Clarify beforeChangeRoomRole payload semantics (add vs remove)remove flows pass role: 'user' (apps/meteor/server/methods/removeRoomModerator.ts:61, removeRoomOwner.ts:67) while add flows pass the added role (addRoomModerator.ts:68, addRoomOwner.ts:68). If listeners need to know which role was removed or to distinguish add vs remove, extend the payload to include an action and the target role (e.g. { action: 'remove', role: 'moderator' }) or document that role represents the post-change role.
packages/core-services/src/types/IRoomService.ts (1)
10-13: Exporting ICreateRoomOptions is fine; consider narrowing the index signature.A top-level Partial<Record<string, string | ISubscriptionExtraData>> makes the API very permissive and weakly typed. If feasible, prefer explicit fields and move extensibility behind a dedicated bag to avoid accidental key collisions with future well-known options.
Example shape:
-export interface ICreateRoomOptions extends Partial<Record<string, string | ISubscriptionExtraData>> { - creator: string; - subscriptionExtra?: ISubscriptionExtraData; -} +export interface ICreateRoomOptions { + creator: string; + subscriptionExtra?: ISubscriptionExtraData; + additionalOptions?: Record<string, string | ISubscriptionExtraData>; +}If keeping the index signature is intentional for federation use-cases, please confirm consumers won’t rely on arbitrary keys that we may later formalize.
apps/meteor/client/hooks/useIsFederationEnabled.ts (1)
3-6: Tiny type/style nit: add explicit return type and inline return.Keeps the hook self-documenting and avoids widening.
-export const useIsFederationEnabled = () => { - const federationMatrixEnabled = useSetting('Federation_Matrix_enabled', false) === true; - return federationMatrixEnabled; -}; +export const useIsFederationEnabled = (): boolean => useSetting('Federation_Matrix_enabled', false) === true;apps/meteor/server/methods/addRoomOwner.ts (1)
79-81: Fix mislabeled method name in error payload.The thrown error still labels method as addRoomLeader.
- method: 'addRoomLeader', + method: 'addRoomOwner',apps/meteor/server/methods/addRoomModerator.ts (3)
42-44: Fix copy: error message says “owners” instead of “moderators”.This path is for moderators; the message is misleading.
Apply:
- throw new FederationMatrixInvalidConfigurationError('unable to change room owners'); + throw new FederationMatrixInvalidConfigurationError('unable to change room moderators');
79-81: Incorrect method name in error context.Should be
addRoomModerator.Apply:
- method: 'addRoomLeader', + method: 'addRoomModerator',
12-12: Allow async callbacks — relax beforeChangeRoomRole typingConfirmed: Callbacks.run is async (returns Promise) and Callbacks.create supports Promise-returning callbacks. Update apps/meteor/lib/callbacks/beforeChangeRoomRole.ts to allow async callbacks, e.g. change the generic from (args: {...}) => void to (args: {...}) => void | Promise.
apps/meteor/lib/callbacks/beforeChangeRoomRole.ts (1)
3-6: Allow async callbacks and export payload type.Typings imply sync-only; many hooks benefit from async prechecks. Widen the signature.
Apply:
+export type BeforeChangeRoomRoleArgs = { + fromUserId: string; + userId: string; + roomId: string; + role: 'moderator' | 'owner' | 'leader' | 'user'; +}; + -export const beforeChangeRoomRole = - Callbacks.create<(args: { fromUserId: string; userId: string; roomId: string; role: 'moderator' | 'owner' | 'leader' | 'user' }) => void>( - 'beforeChangeRoomRole', - ); +export const beforeChangeRoomRole = + Callbacks.create<(args: BeforeChangeRoomRoleArgs) => void | Promise<void>>('beforeChangeRoomRole');apps/meteor/client/sidebar/header/actions/hooks/useCreateRoomMenu.tsx (1)
9-9: Avoid duplicated permission lists across menus.
CREATE_ROOM_PERMISSIONSappears identical to the one inuseCreateNewMenu.tsx. Consider extracting to a shared module to prevent drift.apps/meteor/lib/callbacks/beforeCreateRoomCallback.ts (2)
8-9: Allow async for prepareCreateRoom callback — verified it's awaited in create flowType looks fine; change the callback to accept Promise as well. createRoom awaits the callback at apps/meteor/app/lib/server/functions/createRoom.ts:136.
Apply:
-export const prepareCreateRoomCallback = - Callbacks.create<(data: { type: IRoom['t']; extraData: { encrypted?: boolean } }) => void>('prepareCreateRoom'); +export const prepareCreateRoomCallback = + Callbacks.create<(data: { type: IRoom['t']; extraData: { encrypted?: boolean } }) => void | Promise<void>>( + 'prepareCreateRoom', + );
5-7: Allow async callbacks and restrict owner fields inbeforeCreateRoom
- Change the callback signature in
apps/meteor/lib/callbacks/beforeCreateRoomCallback.tstoCallbacks.create< (data: { owner: Pick<IUser, '_id' | 'username' | 'name' | 'federated'>; room: Omit<IRoom, '_id' | '_updatedAt'>; }) => void | Promise<void> >('beforeCreateRoom');- In
apps/meteor/app/lib/server/functions/createRoom.ts(around line 241), pass only the picked owner fields:- await beforeCreateRoomCallback.run({ owner, room: roomProps }); + await beforeCreateRoomCallback.run({ + owner: { + _id: owner._id, + username: owner.username, + name: owner.name, + federated: owner.federated, + }, + room: roomProps, + });- All existing listeners already use the destructured
{ owner, room }pattern and require no further updates.apps/meteor/app/e2e/server/beforeCreateRoom.ts (1)
4-12: Set a stable callback id (and priority) to avoid ordering surprises.Even though you only set
encryptedwhen undefined, giving this hook an explicit id/priority improves traceability and prevents accidental duplicate registrations.Apply this diff:
-import { prepareCreateRoomCallback } from '../../../lib/callbacks/beforeCreateRoomCallback'; +import { prepareCreateRoomCallback } from '../../../lib/callbacks/beforeCreateRoomCallback'; +import { callbacks } from '../../../lib/callbacks'; @@ -prepareCreateRoomCallback.add(({ type, extraData }) => { +prepareCreateRoomCallback.add(({ type, extraData }) => { @@ -}); +}, callbacks.priority.LOW, 'e2e-default-encryption');apps/meteor/server/services/federation/utils.ts (2)
49-56: Gate on service readiness and add id/priority to the hook.
- Ensure we throw early if federation is disabled or not ready.
- Provide an explicit id/priority for the callback registration.
Apply this diff:
-import { beforeCreateRoomCallback } from '../../../lib/callbacks/beforeCreateRoomCallback'; +import { beforeCreateRoomCallback } from '../../../lib/callbacks/beforeCreateRoomCallback'; +import { callbacks } from '../../../lib/callbacks'; @@ -beforeCreateRoomCallback.add(async ({ owner, room }) => { +beforeCreateRoomCallback.add(async ({ owner, room }) => { const shouldBeHandledByFederation = room.federated === true || owner.username?.includes(':'); if (shouldBeHandledByFederation) { + // Fail fast if the service is not ready + throwIfFederationNotEnabledOrNotReady(); const federation = (await License.hasValidLicense()) ? FederationEE : Federation; await federation.beforeCreateRoom(room); } -}); +}, callbacks.priority.HIGH, 'federation-beforeCreateRoom-router');
50-50: Using deprecated room.federated flag.
IRoom['federated']is marked deprecated. If possible, prefer a non-deprecated indicator (e.g., a create option/owner realm test) and keep this as a fallback with a TODO.apps/meteor/ee/server/local-services/federation/infrastructure/rocket-chat/hooks/index.ts (1)
90-103: Ensure robust inviter resolutionThe inviter resolution fetches the full user object but falls back to
undefinedsilently. Consider adding error logging when the inviter lookup fails unexpectedly.beforeAddUserToARoom.add( async (params, room: IRoom) => { if (!room || !isRoomFederated(room) || !params || !params.user) { return; } throwIfFederationNotEnabledOrNotReady(); - const inviter = (params.inviter && (await Users.findOneById(params.inviter._id))) || undefined; + const inviter = params.inviter ? await Users.findOneById(params.inviter._id) : undefined; + if (params.inviter && !inviter) { + // Log warning when inviter is expected but not found + console.warn(`[Federation] Inviter with ID ${params.inviter._id} not found`); + } await callback(params.user, room, inviter); }, callbacks.priority.HIGH, 'federation-v2-before-add-user-to-the-room', );apps/meteor/server/services/federation/infrastructure/rocket-chat/hooks/index.ts (1)
67-72: Add logging for missing inviter scenariosWhen the inviter is not found in the database, the callback silently returns. Consider logging this scenario for debugging purposes.
const inviter = (params.inviter && (await Users.findOneById(params.inviter._id))) || undefined; if (!inviter) { + console.warn(`[Federation] Inviter with ID ${params.inviter._id} not found for federated room ${room._id}`); return; }apps/meteor/app/lib/server/lib/afterSaveMessage.ts (1)
15-16: Address the TODO comment about type fixingThe TODO comment indicates a known type issue with the callback configuration. This should be tracked and resolved to ensure type safety.
Would you like me to create an issue to track this type configuration fix?
apps/meteor/lib/callbacks.ts (1)
88-88: Event naming consistency issueThe new event
federation.onAddUsersToRoomreplaces the removedfederation.onAddUsersToARoom. Document this naming change for API consumers.
📜 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 (64)
apps/meteor/app/channel-settings/server/functions/saveRoomName.ts(1 hunks)apps/meteor/app/discussion/server/hooks/propagateDiscussionMetadata.ts(1 hunks)apps/meteor/app/discussion/server/methods/createDiscussion.ts(1 hunks)apps/meteor/app/e2e/server/beforeCreateRoom.ts(1 hunks)apps/meteor/app/lib/client/methods/sendMessage.ts(1 hunks)apps/meteor/app/lib/server/functions/addUserToRoom.ts(4 hunks)apps/meteor/app/lib/server/functions/createRoom.ts(6 hunks)apps/meteor/app/lib/server/functions/sendMessage.ts(1 hunks)apps/meteor/app/lib/server/functions/updateMessage.ts(1 hunks)apps/meteor/app/lib/server/lib/afterSaveMessage.ts(2 hunks)apps/meteor/app/lib/server/methods/addUsersToRoom.ts(1 hunks)apps/meteor/client/NavBarV2/NavBarPagesGroup/actions/CreateChannelModal.tsx(2 hunks)apps/meteor/client/NavBarV2/NavBarPagesGroup/hooks/useCreateNewMenu.tsx(2 hunks)apps/meteor/client/hooks/useIsFederationEnabled.ts(1 hunks)apps/meteor/client/sidebar/header/CreateChannel/CreateChannelModal.tsx(2 hunks)apps/meteor/client/sidebar/header/actions/hooks/useCreateRoomMenu.tsx(2 hunks)apps/meteor/client/views/room/composer/ComposerFederation/ComposerFederation.tsx(1 hunks)apps/meteor/client/views/room/composer/messageBox/MessageBox.tsx(2 hunks)apps/meteor/ee/server/local-services/federation/infrastructure/rocket-chat/hooks/index.ts(4 hunks)apps/meteor/ee/tests/unit/server/federation/server/application/UserService.spec.ts(0 hunks)apps/meteor/ee/tests/unit/server/federation/server/application/room/sender/DirectMessageRoomServiceSender.spec.ts(0 hunks)apps/meteor/ee/tests/unit/server/federation/server/application/room/sender/RoomServiceSender.spec.ts(0 hunks)apps/meteor/ee/tests/unit/server/federation/server/application/room/sender/input/RoomInputDto.spec.ts(0 hunks)apps/meteor/ee/tests/unit/server/federation/server/infrastructure/rocket-chat/converters/RoomSender.spec.ts(0 hunks)apps/meteor/ee/tests/unit/server/federation/server/infrastructure/rocket-chat/hooks/hooks.spec.ts(0 hunks)apps/meteor/ee/tests/unit/server/federation/server/infrastructure/rocket-chat/slash-commands/actions.spec.ts(0 hunks)apps/meteor/lib/callbacks.ts(3 hunks)apps/meteor/lib/callbacks/beforeAddUserToARoom.ts(1 hunks)apps/meteor/lib/callbacks/beforeChangeRoomRole.ts(1 hunks)apps/meteor/lib/callbacks/beforeCreateRoomCallback.ts(1 hunks)apps/meteor/server/methods/addRoomModerator.ts(2 hunks)apps/meteor/server/methods/addRoomOwner.ts(2 hunks)apps/meteor/server/methods/removeRoomModerator.ts(2 hunks)apps/meteor/server/methods/removeRoomOwner.ts(2 hunks)apps/meteor/server/services/federation/infrastructure/rocket-chat/hooks/index.ts(5 hunks)apps/meteor/server/services/federation/utils.ts(2 hunks)apps/meteor/server/services/room/service.ts(1 hunks)apps/meteor/tests/unit/server/federation/Federation.spec.ts(0 hunks)apps/meteor/tests/unit/server/federation/application/room/message/receiver/MessageServiceReceiver.spec.ts(0 hunks)apps/meteor/tests/unit/server/federation/application/room/message/sender/MessageServiceSender.spec.ts(0 hunks)apps/meteor/tests/unit/server/federation/application/room/message/sender/message-sender-helper.spec.ts(0 hunks)apps/meteor/tests/unit/server/federation/application/room/receiver/RoomServiceReceiver.spec.ts(0 hunks)apps/meteor/tests/unit/server/federation/application/room/sender/RoomInternalValidator.spec.ts(0 hunks)apps/meteor/tests/unit/server/federation/application/room/sender/RoomServiceSender.spec.ts(0 hunks)apps/meteor/tests/unit/server/federation/application/user/receiver/UserServiceReceiver.spec.ts(0 hunks)apps/meteor/tests/unit/server/federation/application/user/sender/UserServiceSender.spec.ts(0 hunks)apps/meteor/tests/unit/server/federation/domain/FederatedRoom.spec.ts(0 hunks)apps/meteor/tests/unit/server/federation/domain/FederatedUser.spec.ts(0 hunks)apps/meteor/tests/unit/server/federation/infrastructure/matrix/Bridge.spec.ts(0 hunks)apps/meteor/tests/unit/server/federation/infrastructure/matrix/converters/room/RoomReceiver.spec.ts(0 hunks)apps/meteor/tests/unit/server/federation/infrastructure/matrix/converters/room/to-internal-parser-formatter.spec.ts(0 hunks)apps/meteor/tests/unit/server/federation/infrastructure/matrix/converters/user/UserReceiver.spec.ts(0 hunks)apps/meteor/tests/unit/server/federation/infrastructure/matrix/handlers/BaseEvent.spec.ts(0 hunks)apps/meteor/tests/unit/server/federation/infrastructure/matrix/handlers/MatrixEventsHandler.spec.ts(0 hunks)apps/meteor/tests/unit/server/federation/infrastructure/matrix/handlers/Room.spec.ts(0 hunks)apps/meteor/tests/unit/server/federation/infrastructure/queue/InMemoryQueue.spec.ts(0 hunks)apps/meteor/tests/unit/server/federation/infrastructure/rocket-chat/adapters/Statistics.spec.ts(0 hunks)apps/meteor/tests/unit/server/federation/infrastructure/rocket-chat/converters/RocketTextParser.spec.ts(0 hunks)apps/meteor/tests/unit/server/federation/infrastructure/rocket-chat/converters/RoomSender.spec.ts(0 hunks)apps/meteor/tests/unit/server/federation/infrastructure/rocket-chat/hooks/hooks.spec.ts(0 hunks)apps/meteor/tests/unit/server/federation/infrastructure/rocket-chat/slash-commands/actions.spec.ts(0 hunks)apps/meteor/tests/unit/server/federation/utils.spec.ts(0 hunks)packages/core-services/src/index.ts(1 hunks)packages/core-services/src/types/IRoomService.ts(1 hunks)
💤 Files with no reviewable changes (32)
- apps/meteor/tests/unit/server/federation/infrastructure/queue/InMemoryQueue.spec.ts
- apps/meteor/tests/unit/server/federation/infrastructure/matrix/converters/user/UserReceiver.spec.ts
- apps/meteor/tests/unit/server/federation/application/room/message/receiver/MessageServiceReceiver.spec.ts
- apps/meteor/ee/tests/unit/server/federation/server/application/UserService.spec.ts
- apps/meteor/tests/unit/server/federation/infrastructure/matrix/converters/room/to-internal-parser-formatter.spec.ts
- apps/meteor/tests/unit/server/federation/infrastructure/matrix/handlers/Room.spec.ts
- apps/meteor/tests/unit/server/federation/infrastructure/matrix/handlers/BaseEvent.spec.ts
- apps/meteor/ee/tests/unit/server/federation/server/infrastructure/rocket-chat/hooks/hooks.spec.ts
- apps/meteor/ee/tests/unit/server/federation/server/infrastructure/rocket-chat/slash-commands/actions.spec.ts
- apps/meteor/tests/unit/server/federation/infrastructure/matrix/converters/room/RoomReceiver.spec.ts
- apps/meteor/tests/unit/server/federation/infrastructure/matrix/Bridge.spec.ts
- apps/meteor/tests/unit/server/federation/application/room/message/sender/message-sender-helper.spec.ts
- apps/meteor/tests/unit/server/federation/infrastructure/rocket-chat/adapters/Statistics.spec.ts
- apps/meteor/tests/unit/server/federation/application/room/sender/RoomServiceSender.spec.ts
- apps/meteor/ee/tests/unit/server/federation/server/infrastructure/rocket-chat/converters/RoomSender.spec.ts
- apps/meteor/tests/unit/server/federation/infrastructure/matrix/handlers/MatrixEventsHandler.spec.ts
- apps/meteor/tests/unit/server/federation/infrastructure/rocket-chat/hooks/hooks.spec.ts
- apps/meteor/tests/unit/server/federation/utils.spec.ts
- apps/meteor/ee/tests/unit/server/federation/server/application/room/sender/input/RoomInputDto.spec.ts
- apps/meteor/tests/unit/server/federation/Federation.spec.ts
- apps/meteor/tests/unit/server/federation/domain/FederatedUser.spec.ts
- apps/meteor/tests/unit/server/federation/infrastructure/rocket-chat/converters/RoomSender.spec.ts
- apps/meteor/tests/unit/server/federation/application/user/receiver/UserServiceReceiver.spec.ts
- apps/meteor/tests/unit/server/federation/application/user/sender/UserServiceSender.spec.ts
- apps/meteor/tests/unit/server/federation/application/room/message/sender/MessageServiceSender.spec.ts
- apps/meteor/ee/tests/unit/server/federation/server/application/room/sender/DirectMessageRoomServiceSender.spec.ts
- apps/meteor/tests/unit/server/federation/infrastructure/rocket-chat/slash-commands/actions.spec.ts
- apps/meteor/tests/unit/server/federation/application/room/sender/RoomInternalValidator.spec.ts
- apps/meteor/tests/unit/server/federation/application/room/receiver/RoomServiceReceiver.spec.ts
- apps/meteor/tests/unit/server/federation/infrastructure/rocket-chat/converters/RocketTextParser.spec.ts
- apps/meteor/tests/unit/server/federation/domain/FederatedRoom.spec.ts
- apps/meteor/ee/tests/unit/server/federation/server/application/room/sender/RoomServiceSender.spec.ts
🧰 Additional context used
🧬 Code graph analysis (29)
apps/meteor/lib/callbacks/beforeAddUserToARoom.ts (3)
apps/meteor/ee/server/local-services/federation/infrastructure/rocket-chat/hooks/index.ts (1)
beforeAddUserToARoom(89-103)packages/core-typings/src/IUser.ts (1)
IUser(186-252)packages/core-typings/src/IRoom.ts (1)
IRoom(21-95)
apps/meteor/app/lib/server/methods/addUsersToRoom.ts (1)
apps/meteor/lib/callbacks.ts (1)
callbacks(251-259)
packages/core-services/src/types/IRoomService.ts (1)
packages/core-services/src/index.ts (2)
ICreateRoomOptions(82-82)ISubscriptionExtraData(112-112)
apps/meteor/client/sidebar/header/CreateChannel/CreateChannelModal.tsx (1)
apps/meteor/client/hooks/useIsFederationEnabled.ts (1)
useIsFederationEnabled(3-6)
apps/meteor/client/views/room/composer/messageBox/MessageBox.tsx (1)
apps/meteor/client/hooks/useIsFederationEnabled.ts (1)
useIsFederationEnabled(3-6)
apps/meteor/server/methods/addRoomModerator.ts (1)
apps/meteor/lib/callbacks/beforeChangeRoomRole.ts (1)
beforeChangeRoomRole(3-6)
apps/meteor/client/views/room/composer/ComposerFederation/ComposerFederation.tsx (2)
apps/meteor/client/views/room/composer/ComposerMessage.tsx (1)
ComposerMessageProps(12-25)apps/meteor/client/hooks/useIsFederationEnabled.ts (1)
useIsFederationEnabled(3-6)
apps/meteor/app/lib/client/methods/sendMessage.ts (1)
apps/meteor/lib/callbacks.ts (1)
callbacks(251-259)
apps/meteor/server/methods/removeRoomModerator.ts (1)
apps/meteor/lib/callbacks/beforeChangeRoomRole.ts (1)
beforeChangeRoomRole(3-6)
apps/meteor/client/hooks/useIsFederationEnabled.ts (1)
packages/ui-contexts/src/index.ts (1)
useSetting(69-69)
apps/meteor/client/NavBarV2/NavBarPagesGroup/actions/CreateChannelModal.tsx (1)
apps/meteor/client/hooks/useIsFederationEnabled.ts (1)
useIsFederationEnabled(3-6)
apps/meteor/server/methods/addRoomOwner.ts (1)
apps/meteor/lib/callbacks/beforeChangeRoomRole.ts (1)
beforeChangeRoomRole(3-6)
apps/meteor/server/services/federation/utils.ts (2)
apps/meteor/lib/callbacks/beforeCreateRoomCallback.ts (1)
beforeCreateRoomCallback(5-6)packages/core-services/src/index.ts (3)
License(157-157)FederationEE(182-182)Federation(181-181)
apps/meteor/lib/callbacks/beforeCreateRoomCallback.ts (2)
packages/core-typings/src/IUser.ts (1)
IUser(186-252)packages/core-typings/src/IRoom.ts (1)
IRoom(21-95)
apps/meteor/app/discussion/server/methods/createDiscussion.ts (1)
apps/meteor/app/lib/server/lib/afterSaveMessage.ts (1)
afterSaveMessageAsync(19-25)
apps/meteor/app/e2e/server/beforeCreateRoom.ts (1)
apps/meteor/lib/callbacks/beforeCreateRoomCallback.ts (1)
prepareCreateRoomCallback(8-9)
apps/meteor/client/NavBarV2/NavBarPagesGroup/hooks/useCreateNewMenu.tsx (1)
apps/meteor/client/hooks/useIsFederationEnabled.ts (1)
useIsFederationEnabled(3-6)
apps/meteor/app/channel-settings/server/functions/saveRoomName.ts (1)
apps/meteor/lib/callbacks.ts (1)
callbacks(251-259)
apps/meteor/app/lib/server/functions/sendMessage.ts (1)
apps/meteor/app/lib/server/lib/afterSaveMessage.ts (1)
afterSaveMessage(7-17)
apps/meteor/server/methods/removeRoomOwner.ts (1)
apps/meteor/lib/callbacks/beforeChangeRoomRole.ts (1)
beforeChangeRoomRole(3-6)
apps/meteor/client/sidebar/header/actions/hooks/useCreateRoomMenu.tsx (1)
apps/meteor/client/hooks/useIsFederationEnabled.ts (1)
useIsFederationEnabled(3-6)
apps/meteor/server/services/room/service.ts (1)
packages/core-typings/src/IUser.ts (1)
IUser(186-252)
apps/meteor/app/lib/server/functions/updateMessage.ts (1)
apps/meteor/app/lib/server/lib/afterSaveMessage.ts (1)
afterSaveMessage(7-17)
apps/meteor/app/lib/server/lib/afterSaveMessage.ts (4)
packages/core-typings/src/IMessage/IMessage.ts (1)
IMessage(138-238)packages/core-typings/src/IRoom.ts (1)
IRoom(21-95)packages/core-typings/src/IUser.ts (1)
IUser(186-252)apps/meteor/lib/callbacks.ts (1)
callbacks(251-259)
apps/meteor/lib/callbacks.ts (4)
packages/core-typings/src/IRoom.ts (1)
IRoom(21-95)packages/core-typings/src/IUser.ts (2)
IUser(186-252)Username(40-40)packages/core-typings/src/IMessage/IMessage.ts (1)
IMessage(138-238)packages/core-services/src/types/IRoomService.ts (1)
ICreateRoomOptions(10-13)
apps/meteor/ee/server/local-services/federation/infrastructure/rocket-chat/hooks/index.ts (4)
apps/meteor/lib/callbacks/beforeAddUserToARoom.ts (1)
beforeAddUserToARoom(5-6)packages/core-typings/src/IRoom.ts (2)
IRoom(21-95)isRoomFederated(109-109)apps/meteor/server/services/federation/utils.ts (1)
throwIfFederationNotEnabledOrNotReady(14-22)apps/meteor/lib/callbacks.ts (1)
callbacks(251-259)
apps/meteor/app/lib/server/functions/addUserToRoom.ts (4)
apps/meteor/server/services/room/service.ts (1)
addUserToRoom(61-71)packages/core-typings/src/IUser.ts (1)
IUser(186-252)apps/meteor/ee/server/local-services/federation/infrastructure/rocket-chat/hooks/index.ts (1)
beforeAddUserToARoom(89-103)apps/meteor/lib/callbacks/beforeAddUserToARoom.ts (1)
beforeAddUserToARoom(5-6)
apps/meteor/app/lib/server/functions/createRoom.ts (3)
apps/meteor/ee/server/local-services/federation/infrastructure/rocket-chat/hooks/index.ts (1)
beforeAddUserToARoom(89-103)apps/meteor/lib/callbacks/beforeAddUserToARoom.ts (1)
beforeAddUserToARoom(5-6)apps/meteor/lib/callbacks/beforeCreateRoomCallback.ts (2)
prepareCreateRoomCallback(8-9)beforeCreateRoomCallback(5-6)
apps/meteor/server/services/federation/infrastructure/rocket-chat/hooks/index.ts (3)
apps/meteor/ee/server/local-services/federation/infrastructure/rocket-chat/hooks/index.ts (1)
beforeAddUserToARoom(89-103)apps/meteor/lib/callbacks/beforeAddUserToARoom.ts (1)
beforeAddUserToARoom(5-6)apps/meteor/server/services/federation/utils.ts (1)
throwIfFederationNotEnabledOrNotReady(14-22)
🔇 Additional comments (31)
apps/meteor/app/lib/server/functions/sendMessage.ts (1)
294-297: LGTMPassing the full user object to afterSaveMessage aligns with the new callback payload.
apps/meteor/client/views/room/composer/ComposerFederation/ComposerFederation.tsx (1)
8-12: LGTMHook-based federation toggle is cleaner and consistent with the rest of the UI.
apps/meteor/app/discussion/server/methods/createDiscussion.ts (1)
192-194: LGTMafterSaveMessageAsync now receives user; matches the updated signature across the codebase.
packages/core-services/src/index.ts (1)
82-83: LGTMRe-exporting ICreateRoomOptions from core-services is appropriate for external consumers.
apps/meteor/app/lib/server/methods/addUsersToRoom.ts (1)
79-83: Guarantee inviter is defined before calling federation callbackuser is optional; ensure a concrete IUser is passed to the federation callback — if user is undefined, load via Users.findOneById(userId) and throw a Meteor.Error when not found.
File: apps/meteor/app/lib/server/methods/addUsersToRoom.ts (lines 79-83)
- if (isRoomFederated(room)) { - await callbacks.run('federation.onAddUsersToRoom', { invitees: data.users, inviter: user }, room); + if (isRoomFederated(room)) { + const inviter = user ?? (await Users.findOneById(userId)); + if (!inviter) { + throw new Meteor.Error('error-invalid-user', 'Invalid user', { method: 'addUsersToRoom' }); + } + await callbacks.run('federation.onAddUsersToRoom', { invitees: data.users, inviter }, room); return true; }Ripgrep returned no matches for the event-rename check — verify event name(s) in the repo with:
rg -nP "onAddUsersToARoom|federation.onAddUsersToARoom" -C2apps/meteor/app/channel-settings/server/functions/saveRoomName.ts (1)
99-101: Confirm afterRoomNameChange payload semantics for discussions/federated rooms.When only fname changes (discussions/federated), oldName is room.name (slug) while name is displayName. If subscribers expect the previous display name, consider passing oldFname or switching oldName accordingly. Please confirm downstream expectations.
apps/meteor/server/methods/addRoomOwner.ts (1)
68-69: Pre-change role hook: LGTM.Awaiting beforeChangeRoomRole prior to mutation is the right ordering.
apps/meteor/client/sidebar/header/CreateChannel/CreateChannelModal.tsx (1)
72-73: Migrating to useIsFederationEnabled: LGTM.Centralizing the setting read via a hook reduces duplication and helps future changes.
apps/meteor/server/methods/removeRoomOwner.ts (1)
67-68: Pre-change role hook: LGTM.Hook placement before mutation is correct.
apps/meteor/client/NavBarV2/NavBarPagesGroup/actions/CreateChannelModal.tsx (1)
69-69: Hook adoption: LGTM.Consistent federation gating via useIsFederationEnabled keeps behavior uniform with other UIs.
apps/meteor/app/lib/server/functions/updateMessage.ts (1)
100-103: Switching afterSaveMessage to pass the full user — LGTM.Verified: no call sites pass user._id; usages at apps/meteor/app/lib/server/functions/sendMessage.ts (≈295) and apps/meteor/app/lib/server/functions/updateMessage.ts (≈102) pass the full user; implementation at apps/meteor/app/lib/server/lib/afterSaveMessage.ts (≈7). Note: a separate local afterSaveMessage exists in apps/meteor/app/search/server/events/index.ts that only takes IMessage (unrelated).
apps/meteor/server/methods/addRoomModerator.ts (2)
91-101: Verify event payload:u.namelikely should be the target user’s name.Currently uses
fromUser.name. If the event consumer expects the updated user’s identity, setname: user.name(or omit if unused).Apply:
- u: { - _id: user._id, - username: user.username, - name: fromUser.name, - }, + u: { + _id: user._id, + username: user.username, + name: user.name, + },
36-40: Confirm federated-room permission policyAll four methods skip set-owner/set-moderator permission checks for federated rooms (addRoomModerator.ts:36, addRoomOwner.ts:36, removeRoomModerator.ts:33, removeRoomOwner.ts:32). Confirm this is intentional; if not, enforce the permission checks for federated flows.
apps/meteor/client/NavBarV2/NavBarPagesGroup/hooks/useCreateNewMenu.tsx (1)
7-7: Centralized federation flag usage LGTM.Switching to
useIsFederationEnabled()improves consistency; enterprise gate preserved.Also applies to: 16-16
apps/meteor/client/sidebar/header/actions/hooks/useCreateRoomMenu.tsx (1)
7-7: Centralized federation flag usage LGTM.Matches the pattern used elsewhere; enterprise gate preserved.
Also applies to: 16-16
apps/meteor/client/views/room/composer/messageBox/MessageBox.tsx (1)
47-47: LGTM — confirm federation message sending should be license-gated.MessageBox gates canSend using useIsFederationEnabled() (setting-only) while other code (ComposerFederation, CreateChannelModal) also checks useHasLicenseModule('federation') / isEnterprise; decide whether MessageBox should also require the 'federation' license and update accordingly.
Locations: apps/meteor/client/views/room/composer/messageBox/MessageBox.tsx (≈283–296); see apps/meteor/client/views/room/composer/ComposerFederation/ComposerFederation.tsx and apps/meteor/client/sidebar/header/CreateChannel/CreateChannelModal.tsx for examples.apps/meteor/app/lib/server/functions/addUserToRoom.ts (2)
35-36: Explicit return type is good.Adding
Promise<boolean | undefined>clarifies intent.
24-25: Approve — broader user input accepted.Accepting Pick<IUser, '_id' | 'username'> | string is compatible with the lookups; I scanned the repo for addUserToRoom call sites that pass an object literal with username but no _id and found none.
apps/meteor/lib/callbacks/beforeAddUserToARoom.ts (1)
1-6: Approve pre-add hook migration
No references to the oldfederation.beforeAddUserToARoomremain—all registrations and invocations use the new typed hook.apps/meteor/server/services/federation/infrastructure/rocket-chat/hooks/index.ts (3)
3-3: LGTM! Good migration to typed callbacksThe migration from generic
callbacks.addto the typedbeforeAddUserToARoomcallback improves type safety and code clarity.Also applies to: 8-8
261-261: LGTM! Consistent callback removal patternThe migration to
beforeAddUserToARoom.removefor cleanup is consistent with the add operations and improves code maintainability.Also applies to: 268-269
229-236: Incorrect — not a breaking change: callbacks use params.room._idThe codebase already uses params.room (not params.rid): the callbacks type declares afterRoomNameChange as { room } (apps/meteor/lib/callbacks.ts:51), saveRoomName invokes callbacks.run('afterRoomNameChange', { room, ... }) (apps/meteor/app/channel-settings/server/functions/saveRoomName.ts:99), and a consumer destructures room._id (apps/meteor/app/discussion/server/hooks/propagateDiscussionMetadata.ts:88). The federation wrapper using params.room._id is correct.
Likely an incorrect or invalid review comment.
apps/meteor/app/lib/server/lib/afterSaveMessage.ts (2)
7-9: LGTM! Improved type safety with IUser parameterThe change from
uid?: IUser['_id']touser: IUserprovides better type safety and richer context to callback consumers.
19-20: No action required — callers pass full IUser objectsVerified callers: apps/meteor/app/discussion/server/methods/createDiscussion.ts (afterSaveMessageAsync(..., user)), apps/meteor/app/lib/server/functions/sendMessage.ts (afterSaveMessage(..., user)), apps/meteor/app/lib/server/functions/updateMessage.ts (afterSaveMessage(..., user)). No occurrences of callers passing a userId/_id.
apps/meteor/app/lib/server/functions/createRoom.ts (3)
11-12: LGTM! Good modularization of callback logicThe introduction of dedicated callback modules
beforeAddUserToARoomandbeforeCreateRoomCallbackimproves code organization and type safety.
68-72: Simplified user lookup is cleanerThe removal of the projection parameter from
Users.findUsersByUsernamessimplifies the API while maintaining functionality.
241-244: LGTM! Improved callback structure with owner contextThe
beforeCreateRoomCallbacknow receives both owner and room data, providing better context for pre-creation validations.apps/meteor/ee/server/local-services/federation/infrastructure/rocket-chat/hooks/index.ts (1)
108-108: Verify all references to the renamed event — sandbox search returned no matches for "federation.onAddUsersToARoom"; re-run a repo-wide search locally (e.g., rg -n "federation.onAddUsersToARoom" -S --hidden -g '!node_modules') and confirm all producers/consumers, tests, docs, adapters, and migration scripts were updated. Location: apps/meteor/ee/server/local-services/federation/infrastructure/rocket-chat/hooks/index.ts (≈line 108).apps/meteor/lib/callbacks.ts (3)
1-1: ICreateRoomOptions is properly exported
Verified export exists in packages/core-services/src/index.ts.
52-52: Breaking change: afterSaveMessage now requires user objectapps/meteor/lib/callbacks.ts — signature changed from
uid?: IUser['_id']touser: IUser; update all consumers (callbacks.add('afterSaveMessage', ...) handlers and any callers expectinguid) to accept the newIUserparameter.
51-51: Resolved — internal consumers updated to new payloadRepository consumers were updated to the new payload shape ({ room, name, oldName, userId }) — e.g. apps/meteor/app/channel-settings/server/functions/saveRoomName.ts; apps/meteor/app/discussion/server/hooks/propagateDiscussionMetadata.ts; apps/meteor/server/services/federation/infrastructure/rocket-chat/hooks/index.ts. No changes required in this PR.
apps/meteor/ee/server/local-services/federation/infrastructure/rocket-chat/hooks/index.ts
Show resolved
Hide resolved
apps/meteor/server/services/federation/infrastructure/rocket-chat/hooks/index.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Diego Sampaio <[email protected]>
ggazzo
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
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/server/services/federation/infrastructure/rocket-chat/hooks/index.ts (1)
46-56: Missing non‑federated room guard; risk of double invocation.Listener will also run for federated rooms. Add a guard so this path only handles non‑federated rooms.
Apply this diff:
- async (params, room: IRoom): Promise<void> => { - if (!params?.user || !room || !isFederationEnabled()) { + async (params, room: IRoom): Promise<void> => { + if (!params?.user || !room || isRoomFederated(room) || !isFederationEnabled()) { return; }
🧹 Nitpick comments (2)
apps/meteor/server/services/federation/infrastructure/rocket-chat/hooks/index.ts (2)
267-268: LGTM, but there’s a duplicate removal below.afterSaveMessage/federation‑v2‑after‑room‑message‑sent is removed twice (Lines 274–275). Drop the duplicate.
Apply this diff:
callbacks.remove('afterSaveMessage', 'federation-v2-after-room-message-updated'); callbacks.remove('afterSaveMessage', 'federation-v2-after-room-message-sent'); - callbacks.remove('afterSaveMessage', 'federation-v2-after-room-message-sent');
110-124: Nit: method name casing.Consider renaming afterMessageunReacted -> afterMessageUnreacted for consistency with camelCase.
📜 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/app/lib/server/functions/createRoom.ts(5 hunks)apps/meteor/lib/callbacks/beforeAddUserToARoom.ts(1 hunks)apps/meteor/server/services/federation/infrastructure/rocket-chat/hooks/index.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/meteor/lib/callbacks/beforeAddUserToARoom.ts
- apps/meteor/app/lib/server/functions/createRoom.ts
🧰 Additional context used
🧬 Code graph analysis (1)
apps/meteor/server/services/federation/infrastructure/rocket-chat/hooks/index.ts (2)
apps/meteor/ee/server/local-services/federation/infrastructure/rocket-chat/hooks/index.ts (1)
beforeAddUserToARoom(89-103)apps/meteor/server/services/federation/utils.ts (1)
throwIfFederationNotEnabledOrNotReady(14-22)
⏰ 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 (3)
apps/meteor/server/services/federation/infrastructure/rocket-chat/hooks/index.ts (3)
3-3: LGTM: import is used.
8-8: LGTM: migrating to beforeAddUserToARoom API.
260-262: Confirm CE removal scope.Only the “federated‑room” listener is removed here, while the “non‑federated‑room” one remains until removeAllListeners(). Is this intentional for CE behavior?
| beforeAddUserToARoom.add( | ||
| async (params, room: IRoom): Promise<void> => { | ||
| if (!params?.user || !params.inviter || !room || !isFederationEnabled()) { | ||
| return; | ||
| } | ||
|
|
||
| await callback(params.user, params.inviter, room); | ||
| const inviter = (params.inviter && (await Users.findOneById(params.inviter._id))) || undefined; | ||
| if (!inviter) { | ||
| return; | ||
| } | ||
|
|
||
| await callback(params.user, inviter, room); | ||
| }, |
There was a problem hiding this comment.
Missing federated room guard; possible wrong-path execution.
Ensure this listener only runs for federated rooms; also simplify inviter lookup.
Apply this diff:
- async (params, room: IRoom): Promise<void> => {
- if (!params?.user || !params.inviter || !room || !isFederationEnabled()) {
+ async (params, room: IRoom): Promise<void> => {
+ if (!params?.user || !params.inviter || !room || !isRoomFederated(room) || !isFederationEnabled()) {
return;
}
-
- const inviter = (params.inviter && (await Users.findOneById(params.inviter._id))) || undefined;
- if (!inviter) {
- return;
- }
+ const inviter = await Users.findOneById(params.inviter._id);
+ if (!inviter) {
+ return;
+ }📝 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.
| beforeAddUserToARoom.add( | |
| async (params, room: IRoom): Promise<void> => { | |
| if (!params?.user || !params.inviter || !room || !isFederationEnabled()) { | |
| return; | |
| } | |
| await callback(params.user, params.inviter, room); | |
| const inviter = (params.inviter && (await Users.findOneById(params.inviter._id))) || undefined; | |
| if (!inviter) { | |
| return; | |
| } | |
| await callback(params.user, inviter, room); | |
| }, | |
| beforeAddUserToARoom.add( | |
| async (params, room: IRoom): Promise<void> => { | |
| if (!params?.user || !params.inviter || !room || !isRoomFederated(room) || !isFederationEnabled()) { | |
| return; | |
| } | |
| const inviter = await Users.findOneById(params.inviter._id); | |
| if (!inviter) { | |
| return; | |
| } | |
| await callback(params.user, inviter, room); | |
| }, |
🤖 Prompt for AI Agents
In
apps/meteor/server/services/federation/infrastructure/rocket-chat/hooks/index.ts
around lines 60 to 72, the listener currently runs for all rooms and performs
the inviter DB lookup even for non-federated rooms; add an explicit
federated-room guard and move/simplify the inviter lookup after that guard.
Specifically, first check that the room is federated (e.g., if
(!room?.federated) return; or use the existing isFederatedRoom helper) and
return early if not; then simplify the inviter resolution to const inviter =
params.inviter ? await Users.findOneById(params.inviter._id) : undefined and
return if no inviter before calling await callback(params.user, inviter, room).
| if (!params.room._id || !params.name) { | ||
| return; | ||
| } | ||
|
|
||
| throwIfFederationNotEnabledOrNotReady(); | ||
|
|
||
| await callback(params.rid, params.name); | ||
| await callback(params.room._id, params.name); | ||
| }, |
There was a problem hiding this comment.
Null‑safety and federation check missing; can throw at runtime.
Accessing params.room without guarding can crash; also align with other handlers by checking room is federated.
Apply this diff:
- async (params): Promise<void> => {
- if (!params.room._id || !params.name) {
+ async (params): Promise<void> => {
+ if (!params?.room || !params.room._id || !params.name || !isRoomFederated(params.room)) {
return;
}📝 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.
| if (!params.room._id || !params.name) { | |
| return; | |
| } | |
| throwIfFederationNotEnabledOrNotReady(); | |
| await callback(params.rid, params.name); | |
| await callback(params.room._id, params.name); | |
| }, | |
| async (params): Promise<void> => { | |
| if (!params?.room || !params.room._id || !params.name || !isRoomFederated(params.room)) { | |
| return; | |
| } | |
| throwIfFederationNotEnabledOrNotReady(); | |
| await callback(params.room._id, params.name); | |
| }, |
🤖 Prompt for AI Agents
In
apps/meteor/server/services/federation/infrastructure/rocket-chat/hooks/index.ts
around lines 229-236, the handler accesses params.room._id without guarding and
calls throwIfFederationNotEnabledOrNotReady after that; change it to first call
throwIfFederationNotEnabledOrNotReady, then null-check params and params.room
using optional chaining (e.g. params?.room?. _id) and params.name, and return
early if any are missing; also check that the room is federated (room.federated
or equivalent flag) and return early if it's not, before invoking the callback.
https://rocketchat.atlassian.net/browse/FDR-142
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests