feat: add room invites support #300
Conversation
WalkthroughRefactors invite and leave flows: adds leave-related methods (makeLeave, sendLeave), adds accept/reject invite paths, injects EventRepository into InviteService and adds EventRepository.insertOutlierEvent, updates RoomService join/leave signatures and controller call sites to derive server info from events. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant RoomService
participant InviteService
participant EventRepository
participant StateService
participant FederationService
rect rgb(220,240,255)
Note over Client,InviteService: Invite processing (non-host)
Client->>InviteService: processInvite(event, eventId, room_version, strippedState)
InviteService->>InviteService: derive residentServer from event
InviteService->>StateService: signEvent(inviteEvent)
StateService-->>InviteService: signed event
InviteService->>EventRepository: find existing m.room.create for room
alt m.room.create exists
EventRepository-->>InviteService: found create
InviteService->>StateService: handlePdu(inviteEvent)
StateService-->>InviteService: processed
else no create (outlier)
InviteService->>EventRepository: insertOutlierEvent(eventId, inviteEvent, origin)
EventRepository-->>InviteService: insert result
end
InviteService->>Client: emit membership event
end
rect rgb(240,255,240)
Note over Client,FederationService: Reject invite for remote room (high-level)
Client->>RoomService: rejectInvite(roomId, userId)
RoomService->>FederationService: makeLeave(domain, roomId, userId)
FederationService-->>RoomService: { event: leaveEvent, room_version }
RoomService->>StateService: signEvent(leaveEvent)
StateService-->>RoomService: signed
RoomService->>FederationService: sendLeave(leaveEvent)
FederationService-->>RoomService: response
RoomService->>Client: reject result
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ 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 |
ricardogarim
force-pushed
the
feat/invites
branch
from
57c7fd6 to
aa1a23c
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: 0
🧹 Nitpick comments (3)
packages/federation-sdk/src/services/federation.service.ts (1)
118-143: LGTM! Consider extracting resident server logic.The
sendLeaveimplementation is correct and follows the same patterns assendJoin. The error handling, validation, and request structure are all appropriate.The resident server extraction logic (
leaveEvent.roomId.split(':').pop()) is duplicated insendJoin(line 80) and here (line 125). Consider extracting this into a helper method to reduce duplication:private extractResidentServer(roomId: string): string { const residentServer = roomId.split(':').pop(); if (!residentServer) { throw new Error(`invalid room_id ${roomId}, no server_name part`); } return residentServer; }Then use it in both methods:
- const residentServer = leaveEvent.roomId.split(':').pop(); - - if (!residentServer) { - this.logger.debug({ msg: 'invalid room_id', event: leaveEvent.event }); - throw new Error( - `invalid room_id ${leaveEvent.roomId}, no server_name part`, - ); - } + const residentServer = this.extractResidentServer(leaveEvent.roomId);packages/federation-sdk/src/services/invite.service.ts (2)
236-243: Make “no state yet” for partial invites explicit instead of'' as StateIDUsing
'' as StateIDas thestateIdwhen callingforceInsertOrUpdateEventWithStateIdrelies on a type cast to bypass theStateIDschema and may store an invalid value in persistence or mislead downstream code that assumesstateIdis always a real identifier.If partial invites are expected to have “no state yet”, consider modeling that explicitly instead of abusing an empty string, for example by:
- Allowing
stateIdto be nullable/optional inforceInsertOrUpdateEventWithStateIdand passingnull/undefined, or- Introducing a well‑named sentinel (e.g.,
NO_STATE_ID) that is documented and shared across the codebase.That would improve type safety and make the intent of partial invite persistence clearer.
245-254: Membership emission payload and placement look correctEmitting
homeserver.matrix.membershipafter the invite is persisted (either viastateService.handlePdufor host rooms orEventRepositoryfor remote rooms) ensures consumers see a consistent view. The payload fields (event_id,event,room_id,sender,state_key,origin_server_ts,content,stripped_state) line up with the described membership signature and carry both the invite content (includingis_directwhen present) and the stripped state used for authorization/UI.Only minor note:
state_keyon a membership event should always be defined; the?? ''fallback may hide upstream issues. If you want to fail fast on malformed events, you could assert/log instead of silently substituting an empty string, but this is optional.
📜 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 (10)
packages/federation-sdk/src/index.ts(1 hunks)packages/federation-sdk/src/repositories/event-staging.repository.ts(1 hunks)packages/federation-sdk/src/repositories/event.repository.ts(1 hunks)packages/federation-sdk/src/sdk.ts(1 hunks)packages/federation-sdk/src/services/federation.service.ts(1 hunks)packages/federation-sdk/src/services/invite.service.ts(5 hunks)packages/federation-sdk/src/services/room.service.ts(2 hunks)packages/federation-sdk/src/services/state.service.ts(2 hunks)packages/homeserver/src/controllers/federation/rooms.controller.ts(5 hunks)packages/room/src/manager/room-state.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-10T22:18:31.655Z
Learnt from: sampaiodiego
Repo: RocketChat/homeserver PR: 224
File: packages/federation-sdk/src/services/event-authorization.service.ts:261-268
Timestamp: 2025-10-10T22:18:31.655Z
Learning: In packages/federation-sdk/src/services/state.service.ts, the method `StateService.getLatestRoomState(roomId: string)` has return type `Promise<State>` and never returns undefined. If the state is not found, it throws an error with message "No state found for room ${roomId}" instead of returning undefined.
Applied to files:
packages/homeserver/src/controllers/federation/rooms.controller.tspackages/room/src/manager/room-state.tspackages/federation-sdk/src/services/room.service.tspackages/federation-sdk/src/services/state.service.tspackages/federation-sdk/src/services/invite.service.ts
🧬 Code graph analysis (6)
packages/federation-sdk/src/services/federation.service.ts (3)
packages/room/src/manager/event-wrapper.ts (1)
roomId(101-103)packages/room/src/types/v3-11.ts (1)
Pdu(828-828)packages/federation-sdk/src/specs/federation-api.ts (1)
FederationEndpoints(24-71)
packages/federation-sdk/src/services/room.service.ts (3)
packages/room/src/types/_common.ts (3)
EventID(8-8)UserID(20-20)StateID(12-12)packages/room/src/manager/factory.ts (1)
PersistentEventFactory(30-123)packages/room/src/manager/event-wrapper.ts (2)
roomId(101-103)extractDomainFromId(23-29)
packages/federation-sdk/src/repositories/event.repository.ts (2)
packages/room/src/types/_common.ts (2)
EventID(8-8)StateID(12-12)packages/room/src/types/v3-11.ts (1)
Pdu(828-828)
packages/federation-sdk/src/services/state.service.ts (2)
packages/room/src/types/_common.ts (1)
RoomID(16-16)packages/room/src/manager/event-wrapper.ts (1)
roomId(101-103)
packages/federation-sdk/src/index.ts (1)
packages/room/src/types/_common.ts (1)
PduForType(26-26)
packages/federation-sdk/src/services/invite.service.ts (1)
packages/room/src/types/_common.ts (1)
StateID(12-12)
🔇 Additional comments (13)
packages/federation-sdk/src/services/federation.service.ts (1)
101-116: The review comment is based on a misunderstanding of the Matrix Federation API spec.The GET /_matrix/federation/v1/make_leave/{roomId}/{userId} endpoint does not take a room-version query parameter; it simply returns an event template and a room_version field in the response. This is a different design pattern than makeJoin, which supports version negotiation via a
verquery parameter. The current implementation ofmakeLeavecorrectly omits the version parameter and instead receives theroom_versionfrom the response—this is the intended behavior per the spec.Likely an incorrect or invalid review comment.
packages/federation-sdk/src/sdk.ts (1)
148-154: LGTM!The new
acceptInviteandrejectInvitemethods follow the established delegation pattern consistently with other SDK methods. The type-safe parameter forwarding is appropriate.packages/federation-sdk/src/services/state.service.ts (1)
1131-1148: LGTM! Safe handling of optional room names.The changes correctly handle the case where room names may be undefined (e.g., for DM rooms). The optional type annotation and
?? ''fallback ensure type safety and provide a sensible default.Based on learnings
packages/federation-sdk/src/repositories/event.repository.ts (1)
408-429: LGTM! Clear separation of concerns.The new
forceInsertOrUpdateEventWithStateIdmethod provides a variant that allows overwriting the event on updates (by placingeventin$setinstead of$setOnInsert). The method name clearly conveys this behavior, and it's appropriately used for remote processing flows where event data may need to be updated.packages/homeserver/src/controllers/federation/rooms.controller.ts (2)
8-30: LGTM! Safe handling of optional room names.The changes ensure that the
namefield is always present in public room objects by providing an empty string fallback when the name is undefined. This maintains API contract consistency.
54-89: LGTM! Safe null handling in filter logic.The optional chaining on Line 74 (
?.toLowerCase()) prevents runtime errors whennameis undefined during filtering. Combined with the fallback on Line 87, this ensures robust handling of missing room names.packages/room/src/manager/room-state.ts (1)
65-76: LGTM! Correct handling of DM rooms without name events.The change to return
string | undefinedis appropriate for DM rooms, which don't have name events. The clarifying comment is helpful, and downstream consumers correctly handle the undefined case with fallback values.packages/federation-sdk/src/index.ts (1)
213-238: LGTM! Appropriate extension of membership event type.The addition of
is_directfor DM identification andstripped_statefor invite context follows standard Matrix patterns. Both fields are optional, maintaining backward compatibility while enabling richer membership state representation.packages/federation-sdk/src/services/room.service.ts (2)
1054-1068: Verify room version handling in acceptInvite.Line 1061 uses
PersistentEventFactory.defaultRoomVersionto reconstruct the invite event. However, the invite event should already have room version information in the stored event. Using a hardcoded default version could cause issues if the room uses a different version.Consider extracting the room version from the stored event or fetching it from the room:
- const roomVersion = PersistentEventFactory.defaultRoomVersion; + const createEvent = await this.eventRepository.findByRoomIdAndType( + inviteEventStore.event.room_id, + 'm.room.create', + ); + const roomVersion = createEvent?.event.content?.room_version ?? PersistentEventFactory.defaultRoomVersion; const inviteEvent = PersistentEventFactory.createFromRawEvent( inviteEventStore.event, roomVersion, );
1070-1115: LGTM! Correct invite rejection flow.The
rejectInvitemethod correctly implements the leave flow for rejected invites:
- Validates the invite exists
- Gets a leave template from the inviting server via
makeLeave- Signs and sends the leave event
- Stores locally with empty
StateIDandpartial=true(appropriate since we don't have full room state)- Emits a membership event for local tracking
The use of
forceInsertOrUpdateEventWithStateIdwith an emptyStateIDis appropriate for this remote rejection scenario where full state resolution isn't needed.packages/federation-sdk/src/repositories/event-staging.repository.ts (1)
29-48: Based on my investigation into the codebase and Matrix federation protocol, the original review comment's concern is valid and requires clarification from the developer.Verify the
originfield overwrite behavior.In Matrix federation, it is the responsibility of the originating server of a PDU to deliver that event to its recipient servers. Moving
originfrom the filter to the$setblock means that if the sameeventIdis received from different federation sources, the origin will be overwritten rather than preserved.Based on the two call sites found:
event.service.ts:213passes an explicitoriginvalueroom.service.ts:991passesresidentServeras originThese varying origin values across calls suggest events with the same ID could arrive from different sources. The current implementation will overwrite the origin field on subsequent upserts, which may be unintended if your use case requires tracking the original source.
Consider clarifying the intent:
- If you want to preserve the first origin: use
$setOnInsertfor the origin field instead of$set- If you intentionally want to track the latest origin: add a comment explaining this behavior for maintainability
packages/federation-sdk/src/services/invite.service.ts (2)
9-19: Constructor DI wiring for EventEmitterService and EventRepository looks soundAdding
EventEmitterServiceas a plain constructor dependency andEventRepositoryvia@inject(delay(() => EventRepository))is consistent with tsyringe patterns for avoiding circular deps. The new imports and constructor parameters are coherent and keepInviteServiceresponsibilities clear.Also applies to: 38-46
152-163: Includingm.room.memberinstrippedStateEventstypes is appropriateExtending the
PduForType<...>union with'm.room.member'forstrippedStateEventsin bothshouldProcessInviteandprocessInvitematches the new usage where stripped membership data is forwarded to listeners. The existing logic still only inspectsjoin_rulesandencryption, so behavior is unchanged while the type becomes more accurate.Also applies to: 193-203
ricardogarim
force-pushed
the
feat/invites
branch
from
aa1a23c to
57c7fd6
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
packages/federation-sdk/src/services/invite.service.ts (1)
51-74: Code duplication: Avatar URL construction is duplicated across multiple services.This
getAvatarUrlForUsermethod duplicates the same logic found inroom.service.tsand the avatar URL construction inprofiles.service.ts. Please see the refactoring suggestion in the review ofprofiles.service.tsto extract this into a shared method.
🧹 Nitpick comments (4)
packages/federation-sdk/src/services/profiles.service.ts (1)
28-57: Input validation is handled at DTO layer; the edge case is already mitigated.The
UsernameDtopattern (^@[A-Za-z0-9_=\\/.+-]+:(.+)$) enforces that userId must contain:when present, so the HTTP endpoint prevents malformed input beforequeryProfileis called. The current code already handles the edge case defensively—ifserverNamebecomesundefined, the check on line 36 (serverName !== this.configService.serverName) returnsnull.The suggested refactoring improves explicitness by validating the split result, which would be a code clarity improvement but is not required to prevent the edge case.
packages/federation-sdk/src/services/room.service.ts (3)
69-89: Improve username extraction robustness and clarify avatarETag fallback logic.Several concerns with this implementation:
Line 79: The username extraction
userId.split(':')[0]?.slice(1)assumes userId format@username:domainbut doesn't validate. If the format is unexpected or the result is empty, this could lead to incorrect avatar URLs.Line 86: The fallback
user?.avatarETag || usernamewill useusernamewhenavatarETagis an empty string (which is falsy). If an emptyavatarETaghas a specific meaning (e.g., no avatar set), this fallback behavior might be unintended.No error handling: If
userRepository.findByUsernamethrows, the entire flow will fail without graceful degradation.Consider these improvements:
private async getAvatarUrlForUser( userId: UserID, ): Promise<string | undefined> { const userDomain = extractDomainFromId(userId); const localDomain = this.configService.serverName; if (userDomain !== localDomain) { return undefined; } - const username = userId.split(':')[0]?.slice(1); + // Extract username from userId format: @username:domain + const match = userId.match(/^@([^:]+):/); + const username = match?.[1]; if (!username) { return undefined; } - // Fetch user to get avatarETag - const user = await this.userRepository.findByUsername(username); - const avatarIdentifier = user?.avatarETag || username; + try { + const user = await this.userRepository.findByUsername(username); + // Use avatarETag if explicitly set, otherwise fallback to username + const avatarIdentifier = user?.avatarETag ?? username; - - return `mxc://${localDomain}/avatar${avatarIdentifier}`; + return `mxc://${localDomain}/avatar${avatarIdentifier}`; + } catch (error) { + this.logger.warn(`Failed to fetch user ${username} for avatar URL`, error); + return undefined; + } }
281-292: Extract username parsing to reduce duplication.The username extraction logic on line 282 (
username.split(':')[0]?.slice(1)) is duplicated in multiple places throughout this file (lines 282, 902, 1570, 1689). This creates maintenance burden and inconsistency risk.Consider extracting this to a helper method:
private extractUsername(userId: UserID): string | undefined { const match = userId.match(/^@([^:]+):/); return match?.[1]; }Then use it consistently:
- const displayname = username.split(':')[0]?.slice(1); + const displayname = this.extractUsername(username);
901-934: Reduce duplication in event content construction.The membership content is constructed twice - once for the event (lines 907-911) and again for the emitter (lines 930-934). This duplication is error-prone and makes maintenance harder.
Consider reusing the event content:
const membershipEvent = await stateService.buildEvent<'m.room.member'>( { type: 'm.room.member', content: { membership: 'join', ...(displayname && { displayname }), ...(avatarUrl && { avatar_url: avatarUrl }), }, // ... rest of config }, PersistentEventFactory.defaultRoomVersion, ); await stateService.handlePdu(membershipEvent); this.eventEmitterService.emit('homeserver.matrix.membership', { event_id: membershipEvent.eventId, event: membershipEvent.event, room_id: roomId, state_key: userId, - content: { - membership: 'join', - ...(displayname && { displayname }), - ...(avatarUrl && { avatar_url: avatarUrl }), - }, + content: membershipEvent.event.content, sender: userId, origin_server_ts: Date.now(), });
📜 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 (16)
packages/federation-sdk/src/index.ts(3 hunks)packages/federation-sdk/src/repositories/event-staging.repository.ts(1 hunks)packages/federation-sdk/src/repositories/event.repository.ts(1 hunks)packages/federation-sdk/src/repositories/user.repository.ts(1 hunks)packages/federation-sdk/src/sdk.ts(5 hunks)packages/federation-sdk/src/services/event-authorization.service.ts(1 hunks)packages/federation-sdk/src/services/federation.service.ts(1 hunks)packages/federation-sdk/src/services/invite.service.ts(7 hunks)packages/federation-sdk/src/services/profiles.service.ts(3 hunks)packages/federation-sdk/src/services/room.service.ts(11 hunks)packages/federation-sdk/src/services/state.service.ts(2 hunks)packages/homeserver/src/controllers/federation/profiles.controller.ts(2 hunks)packages/homeserver/src/controllers/federation/rooms.controller.ts(5 hunks)packages/homeserver/src/dtos/federation/error.dto.ts(1 hunks)packages/homeserver/src/dtos/federation/profiles.dto.ts(1 hunks)packages/room/src/manager/room-state.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/federation-sdk/src/services/state.service.ts
- packages/federation-sdk/src/index.ts
- packages/homeserver/src/controllers/federation/rooms.controller.ts
- packages/room/src/manager/room-state.ts
- packages/federation-sdk/src/repositories/event-staging.repository.ts
- packages/federation-sdk/src/services/federation.service.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-10T22:18:31.655Z
Learnt from: sampaiodiego
Repo: RocketChat/homeserver PR: 224
File: packages/federation-sdk/src/services/event-authorization.service.ts:261-268
Timestamp: 2025-10-10T22:18:31.655Z
Learning: In packages/federation-sdk/src/services/state.service.ts, the method `StateService.getLatestRoomState(roomId: string)` has return type `Promise<State>` and never returns undefined. If the state is not found, it throws an error with message "No state found for room ${roomId}" instead of returning undefined.
Applied to files:
packages/federation-sdk/src/services/room.service.tspackages/federation-sdk/src/services/profiles.service.tspackages/federation-sdk/src/services/invite.service.ts
🧬 Code graph analysis (5)
packages/federation-sdk/src/services/room.service.ts (3)
packages/room/src/types/_common.ts (4)
UserID(20-20)EventID(8-8)StateID(12-12)RoomID(16-16)packages/room/src/manager/event-wrapper.ts (2)
extractDomainFromId(23-29)roomId(101-103)packages/room/src/manager/factory.ts (1)
PersistentEventFactory(30-123)
packages/homeserver/src/controllers/federation/profiles.controller.ts (3)
packages/federation-sdk/src/index.ts (1)
federationSDK(348-348)packages/homeserver/src/dtos/federation/profiles.dto.ts (2)
QueryProfileQueryDto(9-14)QueryProfileResponseDto(16-19)packages/homeserver/src/dtos/federation/error.dto.ts (2)
FederationErrorResponseDto(3-12)FederationErrorResponseDto(14-16)
packages/federation-sdk/src/services/profiles.service.ts (1)
packages/federation-sdk/src/services/config.service.ts (1)
serverName(108-110)
packages/federation-sdk/src/services/invite.service.ts (2)
packages/room/src/types/_common.ts (2)
UserID(20-20)StateID(12-12)packages/room/src/manager/event-wrapper.ts (1)
extractDomainFromId(23-29)
packages/federation-sdk/src/repositories/event.repository.ts (2)
packages/room/src/types/_common.ts (2)
EventID(8-8)StateID(12-12)packages/room/src/types/v3-11.ts (1)
Pdu(828-828)
🔇 Additional comments (13)
packages/homeserver/src/dtos/federation/profiles.dto.ts (1)
17-17: Breaking change verified:displaynameno longer acceptsnull.The DTO change is confirmed at line 17 (profiles.dto.ts), removing the nullable union from the response schema. However, the implementation already handles this correctly: the profiles service (line 124) uses conditional spreading to omit the displayname field when it's falsy, preventing null values from being returned. This aligns the runtime behavior with the new schema.
While this is a breaking change for API consumers expecting
{ displayname: null }, no code modifications are needed—the implementation already conforms to the new contract.packages/homeserver/src/dtos/federation/error.dto.ts (1)
9-9: LGTM!The addition of
M_NOT_FOUNDerror code properly supports 404 responses in the federation API, aligning with Matrix specification error handling.packages/homeserver/src/controllers/federation/profiles.controller.ts (1)
31-51: LGTM!The updated profile query handler properly handles the not-found case by returning a 404 response with
M_NOT_FOUND, and correctly filters the response to include only the relevant fields (displaynameandavatar_url). This aligns with Matrix federation specification for profile queries.packages/federation-sdk/src/repositories/user.repository.ts (2)
27-47: LGTM! The query correctly filters for local users only.The
findByUsernamemethod appropriately filters for non-federated users using the$orcondition, ensuring only local users are returned. The explicit projection is also good practice for performance.
4-19: Field naming inconsistency is intentional and correct—no changes needed.The
Usertype usesavatarUrl(camelCase) as part of the internal TypeScript domain model. When federation services communicate with the Matrix protocol, they explicitly convert toavatar_url(snake_case)—which is mandated by the Matrix specification. This boundary mapping is visible throughout the codebase (e.g.,...(avatarUrl && { avatar_url: avatarUrl })). The design aligns internal model naming with the external protocol requirements, which is the correct architectural approach.packages/federation-sdk/src/sdk.ts (1)
41-41: LGTM! The new delegation methods follow the established pattern.The additions properly expose the new room service methods (
updateMemberProfile,acceptInvite,rejectInvite,updateUserProfile) and event emitter functionality (emit) through the SDK's public API, following the existing delegation pattern consistently.Also applies to: 104-108, 156-168, 319-321
packages/federation-sdk/src/services/invite.service.ts (2)
276-285: LGTM! Event emission provides good extensibility.The event emission after invite processing enables event-driven reactions to membership changes, which is a good architectural pattern for extensibility.
267-274: Confirm: Non-host invite persistence pattern is correct.The empty
StateIDandpartial=truepattern is intentional and properly handles federation scenarios:
Empty StateID is correct — it's a sentinel value indicating events without full state context, used consistently across invite, leave, and room creation flows.
partial=trueimplications — marks events for state completion; the_needsProcessingmethod propagates this flag through the state graph, and events remain tracked viafindPartialsByRoomId.Events are resolved later — confirmed by the room join workflow:
isRoomStatePartial()detects incomplete state,getPartialEvents()retrieves them,_fetchFullBranch()fetches missing branches, andupdateStateId()transitions events from empty to actual StateID during state resolution (state.service.ts:979).This is a well-designed pattern for non-host servers receiving invites with incomplete state.
packages/federation-sdk/src/repositories/event.repository.ts (1)
408-429: No actionable issues found. Current usages are appropriate for federation event handling.The verification shows both usages of
forceInsertOrUpdateEventWithStateIdare in federation contexts where events lack local state context (stateId='', partial=true). Ininvite.service.ts(line 268) androom.service.ts(line 1146), the method is called only when handling remote server events—a scenario where the overwrite behavior is intentional. Events marked aspartial=trueare explicitly designed to be re-processed once state becomes available, as evidenced by the state service logic andfindPartialsByRoomIdmethod. The method naming with "force" prefix and consistent parameter patterns across both usages demonstrate this distinction is well-understood at call sites.packages/federation-sdk/src/services/room.service.ts (4)
30-30: LGTM: New dependencies properly integrated.The StateID import and UserRepository injection are appropriately added to support the new avatar and profile functionality.
Also applies to: 37-37, 62-63
1167-1215: LGTM: Profile update relies on authorization checks in handlePdu.The method doesn't pre-validate that the user is a member of the room (unlike
updateMemberProfileat lines 757-762), but thehandlePducall at line 1195 will perform proper authorization checks. If the user lacks permission or isn't a member, the event will be rejected (checked at lines 1210-1212).
1571-1571: LGTM: Avatar URL integration is consistent.The avatar URL fetching and conditional inclusion in membership events follows the same pattern established in
createRoomandjoinUser.Also applies to: 1581-1581, 1690-1690, 1700-1700
1146-1151: The empty StateID usage is correct and follows established codebase patterns.Verification confirms that using
'' as StateIDfor leave events during invite rejection is intentional and consistent throughout the codebase. The StateID type is a branded string, and empty StateID serves as a sentinel value for events that don't participate in state resolution. This is explicitly documented in the codebase with the comment: "for a state that doesn't NEED to have state. latest events filter out rejected events anyway, therefore no need to be afraid of having no state id associated with an event here" (state.service.ts:633-634). Database queries reinforce this pattern by explicitly filtering events withstateId: { $ne: '' as StateID }. For rejected invites where the user was never actually in the room, bypassing state resolution is the correct behavior.
packages/federation-sdk/src/services/event-authorization.service.ts
Outdated
Show resolved
Hide resolved
ricardogarim
force-pushed
the
feat/invites
branch
from
57c7fd6 to
96dbcaf
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #300 +/- ##
==========================================
+ Coverage 52.81% 53.22% +0.40%
==========================================
Files 96 96
Lines 12597 12794 +197
==========================================
+ Hits 6653 6809 +156
- Misses 5944 5985 +41 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ricardogarim
force-pushed
the
feat/invites
branch
from
96dbcaf to
ab1a5e6
Compare
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)
packages/federation-sdk/src/services/invite.service.ts (1)
34-259: Add comprehensive test coverage for InviteService security-critical authorization and invite flows.The InviteService has zero test coverage. This is problematic because:
- Authorization logic is untested: The service injects and uses
eventAuthorizationService.checkAclForInvite()(line 226) to enforce room access controls—critical for preventing unauthorized invites.- No validation tests: The
shouldProcessInvite()method (line 152) validates room encryption and privacy settings based on configuration, but this logic is unverified.- Multiple untested code paths: The service branches on whether the invited user is local or remote (lines 105–118), whether the server is the room host (line 224–232), and handles several error conditions—all without test coverage.
- Event signing and federation operations: Cryptographic signing and federation messages have no verification.
Please add unit and integration tests covering:
- Local and remote user invite flows with different room configurations
- ACL rejection scenarios (encrypted rooms, public rooms based on config)
- Invalid input validation (malformed roomId/userId)
- Authorization failure paths
- Event signing and persistence
- Event emission for membership changes
♻️ Duplicate comments (1)
packages/federation-sdk/src/services/room.service.ts (1)
1054-1068: Fetch and use the actual room version instead of the default.This issue was flagged in a previous review and remains unresolved. The method uses
PersistentEventFactory.defaultRoomVersion(line 1061) when reconstructing the invite event, but the actual room version should be fetched from the invite event itself or viastateService.getRoomVersion(roomId).The entire codebase follows a consistent pattern of fetching the actual room version before calling
createFromRawEvent(see event.service.ts:193, state.service.ts:203, room.service.ts:405, 434, and rejectInvite at line 1092 which correctly uses the version from makeLeave). Room versions have different schemas, canonicalization rules, and validation requirements—using the wrong version can cause event validation or signature verification failures.Apply this diff:
async acceptInvite(inviteEventId: EventID, userId: UserID) { const inviteEventStore = await this.eventService.getEventById(inviteEventId); if (!inviteEventStore) { throw new Error(`Invite event not found: ${inviteEventId}`); } - const roomVersion = PersistentEventFactory.defaultRoomVersion; + const roomId = inviteEventStore.event.room_id; + const roomVersion = await this.stateService.getRoomVersion(roomId); + const inviteEvent = PersistentEventFactory.createFromRawEvent( inviteEventStore.event, roomVersion, ); return this.joinUser(inviteEvent, userId); }
📜 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 (12)
packages/federation-sdk/src/index.ts(1 hunks)packages/federation-sdk/src/repositories/event-staging.repository.ts(1 hunks)packages/federation-sdk/src/repositories/event.repository.ts(1 hunks)packages/federation-sdk/src/sdk.ts(1 hunks)packages/federation-sdk/src/services/federation.service.ts(1 hunks)packages/federation-sdk/src/services/invite.service.ts(5 hunks)packages/federation-sdk/src/services/room.service.ts(3 hunks)packages/federation-sdk/src/services/send-join.service.ts(1 hunks)packages/federation-sdk/src/services/staging-area.service.ts(1 hunks)packages/federation-sdk/src/services/state.service.ts(2 hunks)packages/homeserver/src/controllers/federation/rooms.controller.ts(5 hunks)packages/room/src/manager/room-state.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/homeserver/src/controllers/federation/rooms.controller.ts
- packages/federation-sdk/src/services/federation.service.ts
- packages/federation-sdk/src/repositories/event-staging.repository.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-10T22:18:31.655Z
Learnt from: sampaiodiego
Repo: RocketChat/homeserver PR: 224
File: packages/federation-sdk/src/services/event-authorization.service.ts:261-268
Timestamp: 2025-10-10T22:18:31.655Z
Learning: In packages/federation-sdk/src/services/state.service.ts, the method `StateService.getLatestRoomState(roomId: string)` has return type `Promise<State>` and never returns undefined. If the state is not found, it throws an error with message "No state found for room ${roomId}" instead of returning undefined.
Applied to files:
packages/federation-sdk/src/services/state.service.tspackages/room/src/manager/room-state.tspackages/federation-sdk/src/services/room.service.tspackages/federation-sdk/src/services/staging-area.service.tspackages/federation-sdk/src/services/invite.service.ts
🧬 Code graph analysis (7)
packages/federation-sdk/src/services/state.service.ts (2)
packages/room/src/types/_common.ts (1)
RoomID(16-16)packages/room/src/manager/event-wrapper.ts (1)
roomId(101-103)
packages/federation-sdk/src/services/send-join.service.ts (1)
packages/room/src/manager/event-wrapper.ts (1)
roomId(101-103)
packages/federation-sdk/src/services/room.service.ts (3)
packages/room/src/manager/event-wrapper.ts (2)
roomId(101-103)extractDomainFromId(23-29)packages/room/src/types/_common.ts (3)
EventID(8-8)UserID(20-20)StateID(12-12)packages/room/src/manager/factory.ts (1)
PersistentEventFactory(30-123)
packages/federation-sdk/src/services/staging-area.service.ts (1)
packages/room/src/manager/event-wrapper.ts (2)
roomId(101-103)event(129-138)
packages/federation-sdk/src/index.ts (2)
packages/core/src/events/isRoomMemberEvent.ts (1)
Membership(11-11)packages/room/src/types/_common.ts (1)
PduForType(26-26)
packages/federation-sdk/src/services/invite.service.ts (1)
packages/room/src/types/_common.ts (1)
StateID(12-12)
packages/federation-sdk/src/repositories/event.repository.ts (3)
packages/room/src/types/_common.ts (2)
EventID(8-8)StateID(12-12)packages/room/src/manager/event-wrapper.ts (1)
event(129-138)packages/room/src/types/v3-11.ts (1)
Pdu(828-828)
🔇 Additional comments (12)
packages/federation-sdk/src/services/state.service.ts (1)
1134-1144: LGTM! Properly handles optional room names.The changes correctly align with
RoomState.namenow returningstring | undefined. The nullish coalescing operator provides a sensible fallback to empty string for public room listings.packages/federation-sdk/src/services/room.service.ts (2)
883-891: LGTM! Extended membership event payload.The emission now includes additional contextual fields (room_id, sender, state_key, origin_server_ts, content) that align with the updated
HomeserverEventSignaturestype definition.
1070-1115: I need to verify a few more implementation details before finalizing the assessment.Based on my verification of the codebase through script execution and web searches, I can now provide an assessment:
The pattern of using empty StateID for leave events rejecting invites is correct and consistent with the codebase design.
Evidence from verification:
- Empty StateID is used consistently across the codebase in 6+ locations for events that don't modify room state: room creation events (state.service.ts line 431), initial state creation (state.service.ts line 582), rejected events (state.service.ts line 636), invite events (invite.service.ts line 240), and leave events from rejectInvite (room.service.ts line 1102)
- The codebase explicitly documents this pattern in state.service.ts (line 633-634): "latest events filter out rejected events anyway, therefore no need to be afraid of having no state id associated with an event here"
- Downstream queries explicitly filter OUT events with empty StateID using
{ $ne: '' as StateID }in two locations (event.repository.ts lines 146 and 483), ensuring events with empty StateID don't get included in state resolution or timeline queries- The Matrix Federation API specification permits this pattern: when rejecting invites, the server makes a /make_leave request, signs the event, and sends it via /send_leave
The design correctly handles the concern raised:
- Empty StateID serves as a marker for non-state-modifying events
- Downstream consumers are aware and explicitly filter these out where appropriate
- The event emission still works correctly for logging/tracking purposes
- This matches the Matrix specification for handling invite rejection
packages/federation-sdk/src/services/staging-area.service.ts (1)
309-320: LGTM! Membership event emission enriched with contextual data.The emission now includes all required fields from the updated
HomeserverEventSignaturestype, includingstripped_statesourced from the invite'sunsigned.invite_room_state, which provides invited users with minimal room context.packages/federation-sdk/src/services/send-join.service.ts (1)
73-81: LGTM! Join event emission properly enriched.The emission correctly extracts and includes all required contextual fields from the signed join event, with a sensible fallback for
state_key.packages/federation-sdk/src/sdk.ts (1)
148-154: LGTM! Invite handling methods properly exposed.The new
acceptInviteandrejectInvitemethods follow the established delegation pattern and correctly expose the room service functionality through the SDK's public API.packages/federation-sdk/src/index.ts (1)
106-130: LGTM! Type signature correctly reflects enriched membership events.The extended
homeserver.matrix.membershipsignature properly captures all the contextual fields now emitted across the codebase (staging-area.service.ts, send-join.service.ts, room.service.ts). Thestripped_statearray includes appropriate room state event types that provide invited users with necessary room context.packages/federation-sdk/src/repositories/event.repository.ts (1)
408-429: Behavior confirmed as intentional; force overwrite is by design, not a bug.The technical observation in the review is accurate:
forceInsertOrUpdateEventWithStateIdplaceseventin$set(applies on both insert and update), whileinsertOrUpdateEventWithStateIdplaces it in$setOnInsert(applies only on insert). However, this difference is intentional.Both call sites—invite.service.ts:237 and room.service.ts:1099—pass
partial = true, indicating these are partial/incomplete event states that may be updated. The "force" prefix and$setplacement signal that subsequent calls should overwrite the event data. This pattern is appropriate for handling federated events where partial state is initially stored and later updated.No issues found. The code functions as designed.
packages/room/src/manager/room-state.ts (1)
65-76: LGTM! Breaking change properly handles rooms without names.The getter now correctly returns
undefinedfor rooms without name events (e.g., DMs), which is more accurate than potentially throwing or returning a default value. The related changes instate.service.ts(line 1144) properly handle this by defaulting to an empty string for public room listings.Verification confirms all consumers of
RoomState.namehandle theundefinedcase:
- Production code at
state.service.ts:1144usesstate.name ?? ''- No other production code chains
.namedirectly aftergetLatestRoomState2()- Test expectations align with actual behavior
packages/federation-sdk/src/services/invite.service.ts (3)
245-254: I need to search for downstream consumers of this event to complete the verification:Let me search for event listeners in the codebase directly:
The review comment's concerns about
stripped_stateare based on incorrect assumptions. The type definition at packages/federation-sdk/src/index.ts explicitly includesstripped_state?: PduForType<...>as an optional field within the 'homeserver.matrix.membership' event schema, confirming that inclusion of this field is intentional design, not an oversight. This is consistent with how other services like staging-area.service.ts handle the same event type.Likely an incorrect or invalid review comment.
236-243: Revert review comment—schema validation concern is incorrect.The original review's primary concern about type safety is unfounded.
StateIDis defined asz.string().brand('StateID'), which validates that a value is a valid string; empty string''passes this validation. The use of'' as StateIDdoes not bypass validation.Additionally, the empty StateID pattern with
partial = trueis intentional throughout the codebase (used in 8 locations across repositories and services) to represent events that haven't been fully processed yet. The MongoDBupdateOneoperation doesn't throw on empty string values—it successfully upserts the document.While systematic error handling is reasonable as defensive programming, the reviewed code is consistent with patterns in this file (e.g., the host path at line 231 also lacks error handling). No changes are required.
44-45: Based on my verification, I cannot find an actual import-time circular dependency between InviteService and EventRepository. Here's what I discovered:Dependency analysis:
EventRepositoryonly depends on MongoDB (Collection<EventStore>) with no service importsStateServiceimportsEventRepositorydirectly (one-way)InviteServiceimportsStateServiceandEventRepositorydirectly (one-way)- No import-level cycle detected
Key findings:
StateServicealso uses@inject(delay(() => EventRepository)), suggesting the pattern is broader than just this one serviceEventRepositorydoes not importInviteServiceorStateService, so there's no back-reference creating a cycle- Both services import
EventRepositorydirectly without cyclesThe
delay()pattern from tsyringe is designed to break circular dependencies, but my analysis shows no actual circular imports. The usage might be:
- Defensive/anticipatory coding
- Addressing a runtime initialization order issue
- Unnecessary and safe to reconsider
Please verify:
- Whether the
delay()pattern is actually needed or if it's redundant defensive coding- If removing
@inject(delay(() => EventRepository))from bothInviteServiceandStateServicewould cause any runtime issues- The original architectural reason this pattern was introduced
ricardogarim
force-pushed
the
feat/invites
branch
5 times, most recently
from
4cdd35c to
b420e79
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/federation-sdk/src/services/room.service.ts (1)
1049-1067: Fetch the actual room version instead of using the default.Line 1060 uses
PersistentEventFactory.defaultRoomVersionwhen reconstructing the invite event. This pattern is inconsistent with the rest of the codebase, wherestateService.getRoomVersion(roomId)is called beforecreateFromRawEvent(see lines 370, 399, 434, and 621 in this file). Using an incorrect room version can cause event validation, signing, or hashing failures since different versions have different schemas and canonicalization rules.Based on learnings,
stateService.getRoomVersion()throws an error if the room is not found, so you should handle that case appropriately.Apply this diff to fetch the actual room version:
async acceptInvite(roomId: RoomID, userId: UserID) { const inviteEventStore = await this.eventService.findInviteEvent( roomId, userId, ); if (!inviteEventStore) { throw new Error( `Invite event not found for user ${userId} in room ${roomId}`, ); } - const roomVersion = PersistentEventFactory.defaultRoomVersion; + const roomVersion = await this.stateService.getRoomVersion(roomId); const inviteEvent = PersistentEventFactory.createFromRawEvent( inviteEventStore.event, roomVersion, ); return this.joinUser(inviteEvent, userId); }
🧹 Nitpick comments (1)
packages/federation-sdk/src/services/invite.service.ts (1)
198-208: Consider documenting why an empty StateID is used for invite persistence.The code persists the invite event with an empty
StateIDandpartial=true, then emits a membership event. This deviates from the typical persistence flow (usingstateService.handlePdu). While this aligns with the PR's design (mentioned in the AI summary), adding a comment would clarify why invites bypass normal state resolution.+ // Invites are persisted with empty state as they don't affect room state until accepted/rejected await this.eventRepository.forceInsertOrUpdateEventWithStateId( inviteEvent.eventId, inviteEvent.event, '' as StateID, true, // partial = true );
📜 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)
packages/federation-sdk/src/repositories/event.repository.ts(1 hunks)packages/federation-sdk/src/sdk.ts(1 hunks)packages/federation-sdk/src/services/federation.service.ts(1 hunks)packages/federation-sdk/src/services/invite.service.ts(4 hunks)packages/federation-sdk/src/services/room.service.ts(2 hunks)packages/federation-sdk/src/services/state.service.ts(1 hunks)packages/homeserver/src/controllers/federation/invite.controller.ts(0 hunks)
💤 Files with no reviewable changes (1)
- packages/homeserver/src/controllers/federation/invite.controller.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/federation-sdk/src/services/federation.service.ts
- packages/federation-sdk/src/sdk.ts
- packages/federation-sdk/src/repositories/event.repository.ts
- packages/federation-sdk/src/services/state.service.ts
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-10-10T22:18:31.655Z
Learnt from: sampaiodiego
Repo: RocketChat/homeserver PR: 224
File: packages/federation-sdk/src/services/event-authorization.service.ts:261-268
Timestamp: 2025-10-10T22:18:31.655Z
Learning: In packages/federation-sdk/src/services/state.service.ts, the method `StateService.getLatestRoomState(roomId: string)` has return type `Promise<State>` and never returns undefined. If the state is not found, it throws an error with message "No state found for room ${roomId}" instead of returning undefined.
Applied to files:
packages/federation-sdk/src/services/room.service.tspackages/federation-sdk/src/services/invite.service.ts
📚 Learning: 2025-09-14T13:30:07.786Z
Learnt from: ricardogarim
Repo: RocketChat/homeserver PR: 184
File: packages/core/src/utils/fetch.ts:12-47
Timestamp: 2025-09-14T13:30:07.786Z
Learning: In the RocketChat/homeserver codebase, the team prefers to keep implementations simple and functional rather than adding complex optimizations prematurely. Performance improvements are deferred to future work when there's clear evidence they're needed.
Applied to files:
packages/federation-sdk/src/services/invite.service.ts
📚 Learning: 2025-09-14T13:15:46.588Z
Learnt from: ricardogarim
Repo: RocketChat/homeserver PR: 184
File: packages/federation-sdk/src/services/media.service.ts:21-31
Timestamp: 2025-09-14T13:15:46.588Z
Learning: In the RocketChat homeserver project, ricardogarim prefers to defer timeout and retry enhancements for media downloads when there's already a TODO to make timeouts configurable, indicating they manage technical debt systematically rather than implementing every suggested improvement immediately.
Applied to files:
packages/federation-sdk/src/services/invite.service.ts
📚 Learning: 2025-09-14T13:34:12.448Z
Learnt from: ricardogarim
Repo: RocketChat/homeserver PR: 184
File: packages/federation-sdk/src/services/event-authorization.service.ts:213-223
Timestamp: 2025-09-14T13:34:12.448Z
Learning: ACL events (m.room.server_acl) are not yet implemented in the homeserver codebase, so ACL-related code currently handles the simpler case where no ACL events exist in room state.
Applied to files:
packages/federation-sdk/src/services/invite.service.ts
🧬 Code graph analysis (2)
packages/federation-sdk/src/services/room.service.ts (4)
packages/room/src/manager/event-wrapper.ts (2)
roomId(101-103)extractDomainFromId(23-29)packages/federation-sdk/src/index.ts (2)
RoomID(37-37)UserID(36-36)packages/room/src/types/_common.ts (3)
RoomID(16-16)UserID(20-20)StateID(12-12)packages/room/src/manager/factory.ts (1)
PersistentEventFactory(30-123)
packages/federation-sdk/src/services/invite.service.ts (3)
packages/federation-sdk/src/index.ts (2)
PersistentEventBase(33-33)RoomVersion(34-34)packages/room/src/manager/event-wrapper.ts (1)
event(129-138)packages/room/src/types/_common.ts (1)
StateID(12-12)
🔇 Additional comments (4)
packages/federation-sdk/src/services/invite.service.ts (1)
37-40: LGTM! Dependency injection properly configured.The constructor correctly injects
EventEmitterServiceand uses delayed injection forEventRepositoryto handle circular dependencies.packages/federation-sdk/src/services/room.service.ts (3)
1069-1086: LGTM! Error handling is thorough.The method correctly validates that the invite exists and extracts the inviting server from the sender. The error messages are clear and actionable.
1088-1099: LGTM! Leave event construction follows the correct pattern.The code obtains a leave template from the inviting server (which includes the correct
room_version), constructs the event with that version, and signs it locally before sending. This is the proper federation flow for rejecting an invite.
1101-1111: Persistence and emission align with invite processing.The leave event is persisted with an empty
StateIDandpartial=true, then emitted via the membership event—consistent with howinvite.service.tshandles invites (lines 198-208). This maintains consistency across invite-related flows.
ricardogarim
force-pushed
the
feat/invites
branch
from
4f5e732 to
f315e96
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/federation-sdk/src/services/invite.service.ts (2)
145-161: Stricter invite_room_state validation is correct; consider using a domain error typeThe new
strippedStateEventsparameter type and the guard:
- Ensure
shouldProcessInviteis only run wheninvite_room_stateis actually present and non-empty.- Match the new call site
event.unsigned?.invite_room_stateinprocessInvite, closing the nullability hole highlighted in earlier reviews.The behavior (throwing when stripped state is missing) is reasonable given that policy decisions depend on it. If you want upstream handling to treat “missing invite_room_state” similarly to other policy rejections (e.g. mapping to a 4xx instead of a generic 5xx), consider throwing
NotAllowedError(or another explicit domain error) here as well rather than a bareError. That’s optional but would keep error classification more uniform.
186-221: Empty string sentinel forstateIdis intentional but could be cleanerVerification confirms the
'' as StateIDpattern is deliberate throughout the codebase. Empty string serves as a documented sentinel indicating events without state graph associations yet. Queries that depend on real stateIds explicitly filter empty strings:findLatestEventByRoomIdBeforeTimestampWithAssociatedStateandfindStateIdsByEventIdsboth usestateId: { $ne: '' as StateID }. No invariants are broken—the pattern is consistent in bothinvite.service.tsandroom.service.ts.However, the type escape hatch remains suboptimal. Consider:
- Making
stateIdparameterStateID | nullwith null as the sentinel, or- Introducing a named constant (e.g.,
UNASSOCIATED_STATE_ID = '' as StateID) with clear documentationThis improves clarity without changing behavior.
📜 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 (1)
packages/federation-sdk/src/services/invite.service.ts(5 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-10-10T22:18:31.655Z
Learnt from: sampaiodiego
Repo: RocketChat/homeserver PR: 224
File: packages/federation-sdk/src/services/event-authorization.service.ts:261-268
Timestamp: 2025-10-10T22:18:31.655Z
Learning: In packages/federation-sdk/src/services/state.service.ts, the method `StateService.getLatestRoomState(roomId: string)` has return type `Promise<State>` and never returns undefined. If the state is not found, it throws an error with message "No state found for room ${roomId}" instead of returning undefined.
Applied to files:
packages/federation-sdk/src/services/invite.service.ts
📚 Learning: 2025-09-14T13:30:07.786Z
Learnt from: ricardogarim
Repo: RocketChat/homeserver PR: 184
File: packages/core/src/utils/fetch.ts:12-47
Timestamp: 2025-09-14T13:30:07.786Z
Learning: In the RocketChat/homeserver codebase, the team prefers to keep implementations simple and functional rather than adding complex optimizations prematurely. Performance improvements are deferred to future work when there's clear evidence they're needed.
Applied to files:
packages/federation-sdk/src/services/invite.service.ts
📚 Learning: 2025-09-14T13:15:46.588Z
Learnt from: ricardogarim
Repo: RocketChat/homeserver PR: 184
File: packages/federation-sdk/src/services/media.service.ts:21-31
Timestamp: 2025-09-14T13:15:46.588Z
Learning: In the RocketChat homeserver project, ricardogarim prefers to defer timeout and retry enhancements for media downloads when there's already a TODO to make timeouts configurable, indicating they manage technical debt systematically rather than implementing every suggested improvement immediately.
Applied to files:
packages/federation-sdk/src/services/invite.service.ts
📚 Learning: 2025-10-07T13:17:41.878Z
Learnt from: debdutdeb
Repo: RocketChat/homeserver PR: 252
File: packages/federation-sdk/src/repositories/state-graph.repository.ts:60-63
Timestamp: 2025-10-07T13:17:41.878Z
Learning: In the codebase, when types explicitly declare a field as a non-nullable, non-optional string (e.g., `stateKey: string` in `StateGraphStore`), trust the type system and avoid adding defensive coalescing or null-checks in data pipelines. If violations occur, they should be fixed at the source where the contract is broken, not masked with defensive code.
Applied to files:
packages/federation-sdk/src/services/invite.service.ts
📚 Learning: 2025-09-14T13:34:12.448Z
Learnt from: ricardogarim
Repo: RocketChat/homeserver PR: 184
File: packages/federation-sdk/src/services/event-authorization.service.ts:213-223
Timestamp: 2025-09-14T13:34:12.448Z
Learning: ACL events (m.room.server_acl) are not yet implemented in the homeserver codebase, so ACL-related code currently handles the simpler case where no ACL events exist in room state.
Applied to files:
packages/federation-sdk/src/services/invite.service.ts
🧬 Code graph analysis (1)
packages/federation-sdk/src/services/invite.service.ts (2)
packages/federation-sdk/src/index.ts (3)
PduForType(30-30)PersistentEventBase(33-33)RoomVersion(34-34)packages/room/src/types/_common.ts (2)
PduForType(26-26)StateID(12-12)
🔇 Additional comments (3)
packages/federation-sdk/src/services/invite.service.ts (3)
32-40: Constructor wiring for new dependencies looks coherentInjecting
EventEmitterServiceandEventRepositorydirectly intoInviteServicekeeps invite processing cohesive and avoids additional service indirections. The constructor surface still looks manageable and consistent with the rest of the federation SDK.
169-181: Invite policy logic for encrypted/public rooms is clear and minimalThe computation of
isRoomNonPrivateandisRoomEncrypted, combined withallowedEncryptedRooms/allowedNonPrivateRooms, keeps the policy check straightforward and easy to reason about. Even for rooms that are both encrypted and public, the behavior is deterministic, and the singleNotAllowedErrorpath is appropriate for a first iteration. This aligns with the preference for simple, functional implementations over premature complexity.
213-216: Membership event emission on invite persistence is a good hookEmitting
homeserver.matrix.membershipafter persisting the signed invite gives downstream consumers a clear, centralized signal for membership-related changes initiated via invites. The payload shape{ event_id, event }is simple and future-proof if more metadata is needed later.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/federation-sdk/src/services/room.service.ts (1)
1061-1079: Fetch and use the actual room version instead of the default.The
acceptInvitemethod usesPersistentEventFactory.defaultRoomVersion(line 1072) when reconstructing the invite event, but should use the room's actual version. The codebase consistently fetches the actual room version viastateService.getRoomVersion(roomId)before callingcreateFromRawEvent(as seen in other parts of the codebase).Room version is critical for event validation, signing, and hashing—different versions have different schemas and canonicalization rules. Using the default version could lead to event validation or signature verification failures.
Extract the
room_idfrominviteEventStore.event.room_idand callstateService.getRoomVersion(roomId)to get the actual room version before passing it tocreateFromRawEvent.
🧹 Nitpick comments (1)
packages/federation-sdk/src/services/state.service.ts (1)
788-818: Error handling ingetServerSetInRoomnow masks state/room errors as “empty server set”Wrapping
getLatestRoomStatein a broad try/catch and returning the (possibly empty)serversset means callers can no longer distinguish between:
- room with no joined members, and
- failures like “no state found for room” or other internal errors (including
UnknownRoomError).Given
getLatestRoomState(roomId)is guaranteed to throw when there is no state instead of returning undefined, this is a behavioral change forgetServerSetInRoom/getServersInRoomthat might hide genuine problems. Based on learnings, this may be surprising to existing callers.Consider tightening the error handling, for example:
- Only swallow specific, expected errors (e.g., missing membership events) and rethrow or propagate room/state-not-found conditions, or
- Return a richer result (or throw) when room/state is missing, so the caller can tell “room doesn’t exist / state not ready” from “room has no servers”.
The per-event try/catch around
extractDomainFromIdis a nice robustness improvement; you might optionally downgrade the log level or includeroomIdfor better correlation.
📜 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 (12)
packages/federation-sdk/src/repositories/event.repository.ts(1 hunks)packages/federation-sdk/src/sdk.ts(1 hunks)packages/federation-sdk/src/services/federation.service.ts(1 hunks)packages/federation-sdk/src/services/invite.service.ts(7 hunks)packages/federation-sdk/src/services/profiles.service.ts(2 hunks)packages/federation-sdk/src/services/room.service.ts(12 hunks)packages/federation-sdk/src/services/state.service.ts(1 hunks)packages/homeserver/src/controllers/federation/invite.controller.ts(0 hunks)packages/homeserver/src/controllers/internal/invite.controller.ts(2 hunks)packages/homeserver/src/controllers/internal/room.controller.ts(2 hunks)packages/room/src/types/_common.ts(1 hunks)packages/room/src/types/v3-11.ts(2 hunks)
💤 Files with no reviewable changes (1)
- packages/homeserver/src/controllers/federation/invite.controller.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/federation-sdk/src/sdk.ts
- packages/federation-sdk/src/services/federation.service.ts
- packages/federation-sdk/src/repositories/event.repository.ts
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-10-10T22:18:31.655Z
Learnt from: sampaiodiego
Repo: RocketChat/homeserver PR: 224
File: packages/federation-sdk/src/services/event-authorization.service.ts:261-268
Timestamp: 2025-10-10T22:18:31.655Z
Learning: In packages/federation-sdk/src/services/state.service.ts, the method `StateService.getLatestRoomState(roomId: string)` has return type `Promise<State>` and never returns undefined. If the state is not found, it throws an error with message "No state found for room ${roomId}" instead of returning undefined.
Applied to files:
packages/homeserver/src/controllers/internal/room.controller.tspackages/room/src/types/v3-11.tspackages/federation-sdk/src/services/profiles.service.tspackages/federation-sdk/src/services/room.service.tspackages/homeserver/src/controllers/internal/invite.controller.tspackages/room/src/types/_common.tspackages/federation-sdk/src/services/state.service.tspackages/federation-sdk/src/services/invite.service.ts
📚 Learning: 2025-09-14T13:30:07.786Z
Learnt from: ricardogarim
Repo: RocketChat/homeserver PR: 184
File: packages/core/src/utils/fetch.ts:12-47
Timestamp: 2025-09-14T13:30:07.786Z
Learning: In the RocketChat/homeserver codebase, the team prefers to keep implementations simple and functional rather than adding complex optimizations prematurely. Performance improvements are deferred to future work when there's clear evidence they're needed.
Applied to files:
packages/federation-sdk/src/services/invite.service.ts
📚 Learning: 2025-09-14T13:15:46.588Z
Learnt from: ricardogarim
Repo: RocketChat/homeserver PR: 184
File: packages/federation-sdk/src/services/media.service.ts:21-31
Timestamp: 2025-09-14T13:15:46.588Z
Learning: In the RocketChat homeserver project, ricardogarim prefers to defer timeout and retry enhancements for media downloads when there's already a TODO to make timeouts configurable, indicating they manage technical debt systematically rather than implementing every suggested improvement immediately.
Applied to files:
packages/federation-sdk/src/services/invite.service.ts
📚 Learning: 2025-10-07T13:17:41.878Z
Learnt from: debdutdeb
Repo: RocketChat/homeserver PR: 252
File: packages/federation-sdk/src/repositories/state-graph.repository.ts:60-63
Timestamp: 2025-10-07T13:17:41.878Z
Learning: In the codebase, when types explicitly declare a field as a non-nullable, non-optional string (e.g., `stateKey: string` in `StateGraphStore`), trust the type system and avoid adding defensive coalescing or null-checks in data pipelines. If violations occur, they should be fixed at the source where the contract is broken, not masked with defensive code.
Applied to files:
packages/federation-sdk/src/services/invite.service.ts
📚 Learning: 2025-09-14T13:34:12.448Z
Learnt from: ricardogarim
Repo: RocketChat/homeserver PR: 184
File: packages/federation-sdk/src/services/event-authorization.service.ts:213-223
Timestamp: 2025-09-14T13:34:12.448Z
Learning: ACL events (m.room.server_acl) are not yet implemented in the homeserver codebase, so ACL-related code currently handles the simpler case where no ACL events exist in room state.
Applied to files:
packages/federation-sdk/src/services/invite.service.ts
🧬 Code graph analysis (6)
packages/homeserver/src/controllers/internal/room.controller.ts (1)
packages/room/src/types/_common.ts (1)
stateIdSchema(18-18)
packages/room/src/types/v3-11.ts (1)
packages/room/src/types/_common.ts (1)
stateIdSchema(18-18)
packages/federation-sdk/src/services/profiles.service.ts (1)
packages/room/src/types/_common.ts (1)
stateIdSchema(18-18)
packages/federation-sdk/src/services/room.service.ts (2)
packages/room/src/types/_common.ts (2)
stateIdSchema(18-18)UserID(16-16)packages/room/src/manager/event-wrapper.ts (1)
extractDomainFromId(23-29)
packages/homeserver/src/controllers/internal/invite.controller.ts (1)
packages/room/src/types/_common.ts (1)
stateIdSchema(18-18)
packages/federation-sdk/src/services/state.service.ts (1)
packages/room/src/manager/event-wrapper.ts (2)
event(129-138)extractDomainFromId(23-29)
🔇 Additional comments (10)
packages/room/src/types/v3-11.ts (1)
6-6: LGTM! Improved type accuracy for state_key.The change from
userIdSchematostateIdSchemafor thestate_keydescriptor is correct. Thestate_keyfield can hold various identifiers (empty string for many state events, user IDs for membership events, etc.), so using a dedicatedStateIDtype provides more accurate typing than constraining it toUserID.Also applies to: 644-644
packages/room/src/types/_common.ts (1)
18-20: LGTM! Proper export of StateID types.Exporting
stateIdSchemaand theStateIDtype enables consistent state_key handling across the codebase. The branded type provides compile-time type safety while accepting string values at runtime.packages/federation-sdk/src/services/profiles.service.ts (1)
10-10: LGTM! Consistent state_key normalization.The use of
stateIdSchema.parse(userId)aligns with the broader pattern of normalizing state_key values across the codebase. SinceuserIdis typed asUserID, the parse should succeed. This change maintains consistency with other membership event constructions in the PR.Also applies to: 94-94
packages/federation-sdk/src/services/room.service.ts (2)
266-266: LGTM! Consistent state_key normalization across membership events.The use of
stateIdSchema.parse()for all state_key assignments in membership events (join, leave, kick, ban, DM creation) provides consistent validation and type safety throughout the room service.Also applies to: 491-491, 621-621, 702-702, 745-745, 828-828, 878-878, 1492-1492, 1609-1609, 1667-1668
1081-1124: LGTM! rejectInvite correctly handles room version and federation flow.The
rejectInvitemethod properly:
- Validates the invite exists
- Extracts and validates the inviting server domain
- Obtains the actual room version from the federation
makeLeaveresponse (line 1100)- Signs and sends the leave event through federation
- Persists the event with proper state_key handling
- Emits the membership event for downstream processing
The implementation follows the correct pattern for federated leave handling.
packages/homeserver/src/controllers/internal/invite.controller.ts (1)
5-5: LGTM! Consistent state_key handling in invite controller.The use of
stateIdSchema.parse(username)for the invite event's state_key aligns with the standardized state_key normalization pattern across the federation and homeserver modules.Also applies to: 44-44
packages/homeserver/src/controllers/internal/room.controller.ts (1)
7-7: LGTM! Consistent state_key handling in ban endpoint.The use of
stateIdSchema.parse(userIdToBan)for the ban membership event's state_key maintains consistency with the standardized state_key normalization applied across the federation-related codebase.Also applies to: 358-358
packages/federation-sdk/src/services/invite.service.ts (3)
31-38: LGTM! Proper dependency injection for event persistence.The constructor correctly wires in
EventEmitterServiceandEventRepositoryusing delayed injection for the repository to handle circular dependencies. This enables the new invite event persistence flow.
76-76: LGTM! Consistent state_key normalization in invites.Using
stateIdSchema.parse(userId)for the invite event's state_key maintains consistency with the standardized identifier handling across the federation SDK.
188-227: LGTM! Improved invite processing with proper validation and persistence.The
processInvitemethod now:
- Validates
invite_room_stateis present before processing (lines 193-197), addressing the previous review feedback- Uses repository-based persistence via
insertInviteEvent(lines 212-217) with proper state_key handling- Emits membership events through
eventEmitterService(lines 219-222)The validation ensures policy checks can proceed safely, and the repository-based persistence provides proper event storage with stateId support.
ricardogarim
force-pushed
the
feat/invites
branch
from
dd6ace8 to
814a7e2
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/federation-sdk/src/services/invite.service.ts (1)
153-185: Invite policy validation and error handling are sound; consider richer contextThe
shouldProcessInvitelogic correctly derivesisRoomNonPrivateandisRoomEncryptedfrom stripped state and enforcesallowedEncryptedRooms/allowedNonPrivateRooms, usingNotAllowedErrorfor policy rejections. The new guard inprocessInvitethat throws whenevent.unsigned?.invite_room_stateis missing ensures policy is always evaluated on well-formed data, which is important.If you want easier debugging later, consider including
eventIdand/orevent.room_idin the'Missing invite_room_state required for policy validation'error message (or logging it beforehand), but this is optional.Also applies to: 192-197
📜 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 (1)
packages/federation-sdk/src/services/invite.service.ts(6 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-10-10T22:18:31.655Z
Learnt from: sampaiodiego
Repo: RocketChat/homeserver PR: 224
File: packages/federation-sdk/src/services/event-authorization.service.ts:261-268
Timestamp: 2025-10-10T22:18:31.655Z
Learning: In packages/federation-sdk/src/services/state.service.ts, the method `StateService.getLatestRoomState(roomId: string)` has return type `Promise<State>` and never returns undefined. If the state is not found, it throws an error with message "No state found for room ${roomId}" instead of returning undefined.
Applied to files:
packages/federation-sdk/src/services/invite.service.ts
📚 Learning: 2025-09-14T13:30:07.786Z
Learnt from: ricardogarim
Repo: RocketChat/homeserver PR: 184
File: packages/core/src/utils/fetch.ts:12-47
Timestamp: 2025-09-14T13:30:07.786Z
Learning: In the RocketChat/homeserver codebase, the team prefers to keep implementations simple and functional rather than adding complex optimizations prematurely. Performance improvements are deferred to future work when there's clear evidence they're needed.
Applied to files:
packages/federation-sdk/src/services/invite.service.ts
📚 Learning: 2025-09-14T13:15:46.588Z
Learnt from: ricardogarim
Repo: RocketChat/homeserver PR: 184
File: packages/federation-sdk/src/services/media.service.ts:21-31
Timestamp: 2025-09-14T13:15:46.588Z
Learning: In the RocketChat homeserver project, ricardogarim prefers to defer timeout and retry enhancements for media downloads when there's already a TODO to make timeouts configurable, indicating they manage technical debt systematically rather than implementing every suggested improvement immediately.
Applied to files:
packages/federation-sdk/src/services/invite.service.ts
📚 Learning: 2025-10-07T13:17:41.878Z
Learnt from: debdutdeb
Repo: RocketChat/homeserver PR: 252
File: packages/federation-sdk/src/repositories/state-graph.repository.ts:60-63
Timestamp: 2025-10-07T13:17:41.878Z
Learning: In the codebase, when types explicitly declare a field as a non-nullable, non-optional string (e.g., `stateKey: string` in `StateGraphStore`), trust the type system and avoid adding defensive coalescing or null-checks in data pipelines. If violations occur, they should be fixed at the source where the contract is broken, not masked with defensive code.
Applied to files:
packages/federation-sdk/src/services/invite.service.ts
📚 Learning: 2025-09-14T13:34:12.448Z
Learnt from: ricardogarim
Repo: RocketChat/homeserver PR: 184
File: packages/federation-sdk/src/services/event-authorization.service.ts:213-223
Timestamp: 2025-09-14T13:34:12.448Z
Learning: ACL events (m.room.server_acl) are not yet implemented in the homeserver codebase, so ACL-related code currently handles the simpler case where no ACL events exist in room state.
Applied to files:
packages/federation-sdk/src/services/invite.service.ts
🧬 Code graph analysis (1)
packages/federation-sdk/src/services/invite.service.ts (2)
packages/federation-sdk/src/index.ts (2)
PersistentEventBase(29-29)RoomVersion(30-30)packages/room/src/types/_common.ts (1)
StateID(12-12)
🔇 Additional comments (3)
packages/federation-sdk/src/services/invite.service.ts (3)
1-14: Constructor wiring and DI look consistentThe additional imports and constructor parameters for
StateService,EventEmitterService, and theEventRepositorywith@inject(delay(...))are consistent with the rest of the file and look correct for avoiding circular dependencies. No changes needed here.Also applies to: 18-18, 30-37
103-106: Consistent membership event emission across invite flowsEmitting
'homeserver.matrix.membership'for:
- local invites (lines 103–106),
- remote invites you send (lines 138–141), and
- inbound invites processed via
processInvite(lines 218–221),gives a unified signal for downstream consumers regardless of the invite source. This looks coherent and should simplify listeners. No action required.
Also applies to: 138-141, 218-221
211-216: Pattern is well-established in the codebase and intentionally designed for partial events.The use of
'' as StateIDis not a type safety issue but an established pattern throughout the federation SDK for handling partial events (invites, rejected events, etc.).Evidence:
- Type definition: StateID is a Zod branded type (
z.string().brand('StateID')), which explicitly supports empty strings- Consistent usage: The same pattern appears in
state.service.ts(lines 440, 591, 645),state-graph.repository.ts(line 137), andinvite.service.ts(line 214)- Repository layer support:
EventRepositoryexplicitly handles this pattern:
- Query filters:
stateId: { $ne: '' as StateID }distinguish partial from state-bearing eventsfindPartialsByRoomId()method retrieves partial events- Intentional design: The
partialparameter documents when StateID is emptyThis is not a type contract violation—it's intentional use of branded types for event categorization. No cleaner API exists, and the current implementation aligns with established codebase conventions.
ricardogarim
force-pushed
the
feat/invites
branch
2 times, most recently
from
e120b16 to
865e1d1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/federation-sdk/src/services/room.service.ts (1)
1060-1078: Prefer using the room’s actual version instead ofdefaultRoomVersionwhen reconstructing the invite.
acceptInvitecurrently reconstructs the stored invite withPersistentEventFactory.defaultRoomVersionbefore passing it tojoinUser. Elsewhere in this file and in the SDK, events are consistently reconstructed with the real room version (viastateService.getRoomVersion(roomId)), which avoids subtle mismatches if non‑default versions are used.Even though
joinUsertoday mostly needsroomIdandsender, using the correctroomVersionhere would:
- Keep this path consistent with the rest of the codebase.
- Reduce the risk of future breakage if
joinUseror downstream logic starts usingevent.version.Consider replacing the hard‑coded default with:
const roomVersion = await this.stateService.getRoomVersion(roomId); const inviteEvent = PersistentEventFactory.createFromRawEvent( inviteEventStore.event, roomVersion, );
📜 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)
packages/federation-sdk/src/repositories/event.repository.ts(1 hunks)packages/federation-sdk/src/sdk.ts(1 hunks)packages/federation-sdk/src/services/federation.service.ts(1 hunks)packages/federation-sdk/src/services/invite.service.ts(6 hunks)packages/federation-sdk/src/services/room.service.ts(2 hunks)packages/federation-sdk/src/services/state.service.ts(1 hunks)packages/homeserver/src/controllers/federation/invite.controller.ts(0 hunks)
💤 Files with no reviewable changes (1)
- packages/homeserver/src/controllers/federation/invite.controller.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/federation-sdk/src/repositories/event.repository.ts
- packages/federation-sdk/src/sdk.ts
- packages/federation-sdk/src/services/federation.service.ts
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-10-10T22:18:31.655Z
Learnt from: sampaiodiego
Repo: RocketChat/homeserver PR: 224
File: packages/federation-sdk/src/services/event-authorization.service.ts:261-268
Timestamp: 2025-10-10T22:18:31.655Z
Learning: In packages/federation-sdk/src/services/state.service.ts, the method `StateService.getLatestRoomState(roomId: string)` has return type `Promise<State>` and never returns undefined. If the state is not found, it throws an error with message "No state found for room ${roomId}" instead of returning undefined.
Applied to files:
packages/federation-sdk/src/services/state.service.tspackages/federation-sdk/src/services/room.service.tspackages/federation-sdk/src/services/invite.service.ts
📚 Learning: 2025-09-14T13:30:07.786Z
Learnt from: ricardogarim
Repo: RocketChat/homeserver PR: 184
File: packages/core/src/utils/fetch.ts:12-47
Timestamp: 2025-09-14T13:30:07.786Z
Learning: In the RocketChat/homeserver codebase, the team prefers to keep implementations simple and functional rather than adding complex optimizations prematurely. Performance improvements are deferred to future work when there's clear evidence they're needed.
Applied to files:
packages/federation-sdk/src/services/invite.service.ts
📚 Learning: 2025-09-14T13:15:46.588Z
Learnt from: ricardogarim
Repo: RocketChat/homeserver PR: 184
File: packages/federation-sdk/src/services/media.service.ts:21-31
Timestamp: 2025-09-14T13:15:46.588Z
Learning: In the RocketChat homeserver project, ricardogarim prefers to defer timeout and retry enhancements for media downloads when there's already a TODO to make timeouts configurable, indicating they manage technical debt systematically rather than implementing every suggested improvement immediately.
Applied to files:
packages/federation-sdk/src/services/invite.service.ts
📚 Learning: 2025-10-07T13:17:41.878Z
Learnt from: debdutdeb
Repo: RocketChat/homeserver PR: 252
File: packages/federation-sdk/src/repositories/state-graph.repository.ts:60-63
Timestamp: 2025-10-07T13:17:41.878Z
Learning: In the codebase, when types explicitly declare a field as a non-nullable, non-optional string (e.g., `stateKey: string` in `StateGraphStore`), trust the type system and avoid adding defensive coalescing or null-checks in data pipelines. If violations occur, they should be fixed at the source where the contract is broken, not masked with defensive code.
Applied to files:
packages/federation-sdk/src/services/invite.service.ts
🧬 Code graph analysis (2)
packages/federation-sdk/src/services/state.service.ts (1)
packages/room/src/manager/event-wrapper.ts (3)
roomId(101-103)event(129-138)extractDomainFromId(23-29)
packages/federation-sdk/src/services/room.service.ts (2)
packages/room/src/manager/event-wrapper.ts (2)
roomId(101-103)extractDomainFromId(23-29)packages/room/src/manager/factory.ts (1)
PersistentEventFactory(30-123)
🔇 Additional comments (4)
packages/federation-sdk/src/services/room.service.ts (1)
1080-1122: Reject‑invite flow looks consistent and side‑effect complete.The
rejectInviteimplementation validates the stored invite, derives the inviting server, callsmakeLeave/sendLeave, persists the resulting leave event viainsertInviteEvent, and emitshomeserver.matrix.membership. This aligns well with Matrix invite rejection semantics for the invited homeserver and keeps invite/leave persistence consolidated in the repository.packages/federation-sdk/src/services/invite.service.ts (3)
30-37: DI wiring and singleton scope look good.Injecting
StateService,EventEmitterService, andEventRepositorywith@singleton()and@inject(delay(...))is consistent with the rest of the SDK and keeps invite handling focused on this service without adding extra global state.
88-142: Consistent membership event emission viaeventEmitterService.Using
this.eventEmitterService.emit('homeserver.matrix.membership', …)for both local and federated invite paths centralizes membership notifications and matches how other services publish these events, which should simplify consumers.
153-225: Invite policy check + repository‑backed persistence form a clean inbound‑invite pipeline.
processInvitenow:
- Validates that
event.unsigned.invite_room_stateis present and runs it throughshouldProcessInvite(encryption/publicity policy).- Rebuilds the invite as a
PersistentEventBase, checks theeventId, signs it, persists viaeventRepository.insertInviteEvent, and emits a membership event.This keeps policy enforcement, persistence, and notification concerns cleanly separated on the invited server side and avoids incorrectly mutating the main room state graph for invites.
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)
packages/federation-sdk/src/services/invite.service.ts(5 hunks)packages/federation-sdk/src/services/room.service.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/federation-sdk/src/services/room.service.ts
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-10-10T22:18:31.655Z
Learnt from: sampaiodiego
Repo: RocketChat/homeserver PR: 224
File: packages/federation-sdk/src/services/event-authorization.service.ts:261-268
Timestamp: 2025-10-10T22:18:31.655Z
Learning: In packages/federation-sdk/src/services/state.service.ts, the method `StateService.getLatestRoomState(roomId: string)` has return type `Promise<State>` and never returns undefined. If the state is not found, it throws an error with message "No state found for room ${roomId}" instead of returning undefined.
Applied to files:
packages/federation-sdk/src/services/invite.service.ts
📚 Learning: 2025-09-14T13:30:07.786Z
Learnt from: ricardogarim
Repo: RocketChat/homeserver PR: 184
File: packages/core/src/utils/fetch.ts:12-47
Timestamp: 2025-09-14T13:30:07.786Z
Learning: In the RocketChat/homeserver codebase, the team prefers to keep implementations simple and functional rather than adding complex optimizations prematurely. Performance improvements are deferred to future work when there's clear evidence they're needed.
Applied to files:
packages/federation-sdk/src/services/invite.service.ts
📚 Learning: 2025-09-14T13:15:46.588Z
Learnt from: ricardogarim
Repo: RocketChat/homeserver PR: 184
File: packages/federation-sdk/src/services/media.service.ts:21-31
Timestamp: 2025-09-14T13:15:46.588Z
Learning: In the RocketChat homeserver project, ricardogarim prefers to defer timeout and retry enhancements for media downloads when there's already a TODO to make timeouts configurable, indicating they manage technical debt systematically rather than implementing every suggested improvement immediately.
Applied to files:
packages/federation-sdk/src/services/invite.service.ts
📚 Learning: 2025-10-07T13:17:41.878Z
Learnt from: debdutdeb
Repo: RocketChat/homeserver PR: 252
File: packages/federation-sdk/src/repositories/state-graph.repository.ts:60-63
Timestamp: 2025-10-07T13:17:41.878Z
Learning: In the codebase, when types explicitly declare a field as a non-nullable, non-optional string (e.g., `stateKey: string` in `StateGraphStore`), trust the type system and avoid adding defensive coalescing or null-checks in data pipelines. If violations occur, they should be fixed at the source where the contract is broken, not masked with defensive code.
Applied to files:
packages/federation-sdk/src/services/invite.service.ts
🧬 Code graph analysis (1)
packages/federation-sdk/src/services/invite.service.ts (2)
packages/federation-sdk/src/index.ts (2)
PersistentEventBase(29-29)RoomVersion(30-30)packages/room/src/manager/event-wrapper.ts (1)
event(129-138)
🔇 Additional comments (1)
packages/federation-sdk/src/services/invite.service.ts (1)
210-214: Based on my investigation, I can now provide a rewritten review comment with the verification results:
Verify authorization checks for received invite events are appropriate for the use case.
The change from
stateService.handlePdu()toeventRepository.insertInviteEvent()bypasses the authorization validation thathandlePduprovides. Investigation confirms:
handlePduperforms comprehensive authorization checks viacheckEventAuthWithState(), validating the event against its auth events and room state (lines 629, 704 in state.service.ts)insertInviteEvent()is a simple database upsert with no validation (lines 408-427 in event.repository.ts)processInvite()only validates:
- The
invite_room_statefield exists- Policy checks (encrypted/public room restrictions via
shouldProcessInvite())- Event ID matches the request parameter
However,
processInvite()is specifically for receiving invitation events from remote Matrix servers. The design appears intentional:
- The code comment states "let them handle the transactions" (line 222), indicating the originating server is responsible for authorization
- No code path currently retrieves or processes these stored invite events through room state validation
- Invite events are stored for federation protocol compliance and relaying, not immediately processed into room state
Recommendation: Clarify whether this design is intentional by:
- Adding a comment explaining why authorization checks are not performed for received invites, or
- If checks ARE needed, add basic event structure validation and auth event chain verification before storing
ricardogarim
force-pushed
the
feat/invites
branch
from
bdede9b to
6be144f
Compare
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 (7)
packages/federation-sdk/src/repositories/event.repository.ts(2 hunks)packages/federation-sdk/src/sdk.ts(2 hunks)packages/federation-sdk/src/services/federation.service.ts(1 hunks)packages/federation-sdk/src/services/invite.service.ts(5 hunks)packages/federation-sdk/src/services/room.service.spec.ts(5 hunks)packages/federation-sdk/src/services/room.service.ts(4 hunks)packages/homeserver/src/controllers/federation/invite.controller.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/federation-sdk/src/services/federation.service.ts
- packages/federation-sdk/src/sdk.ts
- packages/federation-sdk/src/services/room.service.spec.ts
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-10-10T22:18:31.655Z
Learnt from: sampaiodiego
Repo: RocketChat/homeserver PR: 224
File: packages/federation-sdk/src/services/event-authorization.service.ts:261-268
Timestamp: 2025-10-10T22:18:31.655Z
Learning: In packages/federation-sdk/src/services/state.service.ts, the method `StateService.getLatestRoomState(roomId: string)` has return type `Promise<State>` and never returns undefined. If the state is not found, it throws an error with message "No state found for room ${roomId}" instead of returning undefined.
Applied to files:
packages/federation-sdk/src/services/room.service.tspackages/federation-sdk/src/services/invite.service.ts
📚 Learning: 2025-09-14T13:30:07.786Z
Learnt from: ricardogarim
Repo: RocketChat/homeserver PR: 184
File: packages/core/src/utils/fetch.ts:12-47
Timestamp: 2025-09-14T13:30:07.786Z
Learning: In the RocketChat/homeserver codebase, the team prefers to keep implementations simple and functional rather than adding complex optimizations prematurely. Performance improvements are deferred to future work when there's clear evidence they're needed.
Applied to files:
packages/federation-sdk/src/services/invite.service.ts
📚 Learning: 2025-09-14T13:15:46.588Z
Learnt from: ricardogarim
Repo: RocketChat/homeserver PR: 184
File: packages/federation-sdk/src/services/media.service.ts:21-31
Timestamp: 2025-09-14T13:15:46.588Z
Learning: In the RocketChat homeserver project, ricardogarim prefers to defer timeout and retry enhancements for media downloads when there's already a TODO to make timeouts configurable, indicating they manage technical debt systematically rather than implementing every suggested improvement immediately.
Applied to files:
packages/federation-sdk/src/services/invite.service.ts
📚 Learning: 2025-10-07T13:17:41.878Z
Learnt from: debdutdeb
Repo: RocketChat/homeserver PR: 252
File: packages/federation-sdk/src/repositories/state-graph.repository.ts:60-63
Timestamp: 2025-10-07T13:17:41.878Z
Learning: In the codebase, when types explicitly declare a field as a non-nullable, non-optional string (e.g., `stateKey: string` in `StateGraphStore`), trust the type system and avoid adding defensive coalescing or null-checks in data pipelines. If violations occur, they should be fixed at the source where the contract is broken, not masked with defensive code.
Applied to files:
packages/federation-sdk/src/services/invite.service.ts
📚 Learning: 2025-09-14T13:34:12.448Z
Learnt from: ricardogarim
Repo: RocketChat/homeserver PR: 184
File: packages/federation-sdk/src/services/event-authorization.service.ts:213-223
Timestamp: 2025-09-14T13:34:12.448Z
Learning: ACL events (m.room.server_acl) are not yet implemented in the homeserver codebase, so ACL-related code currently handles the simpler case where no ACL events exist in room state.
Applied to files:
packages/federation-sdk/src/services/invite.service.ts
🧬 Code graph analysis (3)
packages/federation-sdk/src/services/room.service.ts (3)
packages/room/src/manager/event-wrapper.ts (4)
roomId(111-113)event(147-156)extractDomainFromId(23-29)sender(115-117)packages/room/src/types/_common.ts (4)
RoomID(16-16)UserID(20-20)PduForType(26-26)EventID(8-8)packages/room/src/manager/factory.ts (1)
PersistentEventFactory(30-123)
packages/federation-sdk/src/repositories/event.repository.ts (4)
packages/room/src/types/_common.ts (2)
EventID(8-8)StateID(12-12)packages/room/src/types/v3-11.ts (1)
Pdu(828-828)packages/room/src/state_resolution/definitions/definitions.ts (1)
EventStore(101-103)packages/core/src/models/event.model.ts (1)
EventStore(26-38)
packages/federation-sdk/src/services/invite.service.ts (2)
packages/federation-sdk/src/index.ts (2)
PersistentEventBase(29-29)RoomVersion(30-30)packages/room/src/manager/event-wrapper.ts (3)
residentServer(129-135)event(147-156)extractDomainFromId(23-29)
🔇 Additional comments (13)
packages/homeserver/src/controllers/federation/invite.controller.ts (1)
15-26: LGTM! Cleaner API while preserving security.The refactored
processInvitecall no longer requires explicitroomIdandauthenticatedServerparameters, as the SDK now derives this information from the event itself (viaevent.room_idand domain extraction). The authentication validation at the controller level (lines 16-18) is correctly preserved before the SDK call.packages/federation-sdk/src/repositories/event.repository.ts (1)
409-424: LGTM! Well-structured method for outlier event persistence.The
insertOutlierEventmethod correctly supports the invite workflow for rooms where state is not yet available. The method:
- Properly initializes all
EventStorefields with appropriate defaults- Sets
outlier: trueto mark the event as received out-of-band- Uses
partial: falsesince the event itself is complete- Returns the MongoDB
InsertOneResultfor caller verificationThis enables the non-host invite path in
InviteServiceto persist invite events before room state is established.packages/federation-sdk/src/services/room.service.ts (6)
571-601: LGTM! Proper implementation of leave template generation.The
makeLeavemethod correctly implements the Matrix federation make_leave flow:
- Retrieves the actual room version from room information
- Builds a membership event template with
membership: 'leave'- Adds the
originfield to match federation protocol expectations- Returns both the event template and room version for the caller
This method is used by both
leaveRoom(line 683) andrejectInvite(line 1160) to obtain properly formatted leave event templates.
603-640: LGTM! Secure leave event processing with proper validation.The
sendLeavemethod correctly implements the Matrix federation send_leave flow with important security checks:
- Validates room version exists before processing (lines 608-611)
- Security: Verifies this server is the resident server for the room (lines 622-627), preventing malicious leave requests for rooms hosted elsewhere
- Validates the event ID matches the computed hash (lines 618-620)
- Checks for event rejection and propagates errors (lines 630-632)
- Properly persists via
handlePduand broadcasts to all servers
642-698: LGTM! Cleaner refactor using makeLeave.The
leaveRoommethod now delegates tomakeLeave(line 683) instead of constructing the leave event inline. This refactor:
- Eliminates code duplication with the new leave flow
- Ensures consistent room version handling via
makeLeave- Maintains the same permission checks and processing logic
- Follows the same pattern used in
rejectInvite(remote leave case)
1118-1130: LGTM! Consider adding unit tests.The
acceptInvitemethod correctly:
- Finds the invite event via
findInviteEvent- Extracts the inviter from
inviteEventStore.event.sender- Delegates to
joinUserwith the inviter as the sender parameter, ensuring the join goes through the inviting server's federation endpointAs noted in previous review comments, this method has clear test coverage opportunities (mocking invite lookup, testing missing invite error, etc.), but deferring test implementation is acceptable for this PR.
1132-1176: LGTM! Thoughtful handling of local vs. remote invite rejection.The
rejectInvitemethod correctly handles both scenarios:Local rejection (lines 1152-1155): When the inviter is from our server, delegates to
leaveRoomwhich handles full state updates.Remote rejection (lines 1157-1170): When the inviter is from another server:
- The inline comment (lines 1157-1158) correctly notes we may not have room state yet
- Calls
federationService.makeLeaveto get the leave template from the inviting server- Signs and sends the leave event via
federationService.sendLeave- Manually handles the flow since we can't rely on local state
The membership event emission (lines 1172-1175) ensures both paths notify other components.
909-1116: All callers ofjoinUserhave been updated with the newsenderparameter. The method is used in two test cases (line 218, 318 in room.service.spec.ts) and inacceptInvite(line 1129 in room.service.ts where it correctly passesinviteEventStore.event.sender). The signature change does not break existing code within this codebase.packages/federation-sdk/src/services/invite.service.ts (5)
1-18: LGTM! Proper dependency injection setup.The import and constructor changes correctly introduce
EventRepositoryvia dependency injection withdelay()to handle potential circular dependencies (line 36). This supports the new outlier event persistence in the non-host invite path.Also applies to: 36-37
178-191: LGTM! Cleaner API with explicit stripped state parameter.The
processInvitesignature now requiresstrippedStateEventsas an explicit parameter (line 182-190) instead of accessingevent.unsigned.invite_room_state. This:
- Makes the API contract clearer
- Addresses the previous null safety concern
- Allows the controller to validate and pass the data explicitly
- Maintains type safety with the specific union of allowed event types
204-204: LGTM! Using the residentServer getter as suggested.Line 204 correctly uses the
residentServergetter frominviteEvent(which extracts the domain fromroom_id) instead of manual extraction. This follows the pattern suggested in previous review feedback and is cleaner than callingextractDomainFromIddirectly.Based on past review comments, as per ggazzo's suggestion.
217-227: LGTM! Security validation addresses past critical issue.Lines 217-227 correctly validate that the invited user (from
event.state_key) belongs to this server before signing the invite. This addresses the critical security issue raised in previous reviews where a malicious remote server could request signatures for invites to users on other servers.The validation:
- Extracts the domain from
state_key(line 217)- Validates the domain exists (lines 218-222)
- Verifies it matches
this.configService.serverName(lines 223-227)- Occurs before signing (line 229), preventing unauthorized signatures
Based on past review comments, as per previous security feedback.
229-255: LGTM! Thoughtful refactor handling state availability.The non-host path correctly handles two scenarios:
When we have room state (lines 241-242): If a
m.room.createevent exists, we're already aware of the room and can process the invite normally viahandlePdu.When we don't have state (lines 244-249): For out-of-band invites (first contact with the room), the invite is stored as an outlier via
insertOutlierEvent. This allows the invite to be processed later when accepting (which triggers a join flow that fetches room state).Additional details:
- Line 233: Stores stripped state in
unsigned.invite_room_statefor later policy validation (as explained in the comment)- Line 229: Signs before persisting (security requirement)
- Lines 252-255: Emits membership event after successful persistence
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 (1)
packages/federation-sdk/src/sdk.ts(2 hunks)
🔇 Additional comments (2)
packages/federation-sdk/src/sdk.ts (2)
86-92: LGTM! Well-structured delegation methods.The
makeLeaveandsendLeavemethods follow the established delegation pattern and are positioned logically beforeleaveRoom. The type-safe parameter forwarding using TypeScript'sParametersutility is consistent with the rest of the class. Both methods are properly exposed byRoomService.
159-165: SDK delegation methods follow established patterns correctly.The
acceptInviteandrejectInvitemethods properly delegate tothis.roomServicewith type-safe parameter forwarding, consistent with other methods in the class. However, RoomService's implementation is not visible in the repository, so the underlying method availability cannot be verified from the codebase alone.
Task: FB-44
Related PR: RocketChat/Rocket.Chat#37532
Summary by CodeRabbit
New Features
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.