feat: new method to create DMs #321
Conversation
|
Caution Review failedThe pull request is closed. Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a new federation-aware direct-message creation path (SDK + RoomService), changes RoomService.joinUser signature to derive residency from the roomId, emits homeserver.matrix.membership events during invite handling, and makes PersistentEventFactory.newCreateEvent's roomVersion optional. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SDK as FederationSDK
participant RoomSvc as RoomService
participant Factory as PersistentEventFactory
participant FedSvc as FederationService
participant Store as Storage/DB
Client->>SDK: createDirectMessage(creatorUserId, members)
SDK->>RoomSvc: createDirectMessage(creator, members)
RoomSvc->>Factory: newCreateEvent(creator[, roomVersion])
Factory-->>RoomSvc: signed m.room.create
RoomSvc->>Store: persist create & state events (power_levels, join_rules, etc.)
loop each member
alt member is remote
RoomSvc->>FedSvc: send invite (signed)
FedSvc-->>RoomSvc: ack
RoomSvc->>Store: persist outbound invite
else member is local
RoomSvc->>Store: persist local membership
end
end
RoomSvc-->>SDK: return roomId
SDK-->>Client: return roomId
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (2)
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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #321 +/- ##
==========================================
- Coverage 53.15% 52.04% -1.12%
==========================================
Files 97 97
Lines 12942 13153 +211
==========================================
- Hits 6879 6845 -34
- Misses 6063 6308 +245 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/federation-sdk/src/services/room.service.ts (1)
1636-1687: Consider error handling for partial invite failures.If one member invite fails (especially for remote federation), the room is left in an inconsistent state with some invites sent but the method throwing an error. Consider whether to:
- Continue with remaining members and collect failures, or
- Document this as expected behavior (fail-fast).
For group DMs, partial failure handling might provide a better UX.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
packages/federation-sdk/src/sdk.tspackages/federation-sdk/src/services/room.service.tspackages/room/src/manager/factory.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/sdk.ts
🧬 Code graph analysis (2)
packages/room/src/manager/factory.ts (2)
packages/room/src/types/_common.ts (1)
UserID(20-20)packages/room/src/manager/type.ts (1)
RoomVersion(14-14)
packages/federation-sdk/src/sdk.ts (1)
packages/federation-sdk/src/index.ts (1)
UserID(33-33)
🔇 Additional comments (5)
packages/room/src/manager/factory.ts (1)
85-88: LGTM!The optional
roomVersionparameter with a sensible default simplifies the API for callers while maintaining type safety. The default aligns with Synapse's room version.packages/federation-sdk/src/services/room.service.ts (2)
1681-1683: Clarify the commented-out broadcast.The commented
sendEventToAllServersInRoomcall may be needed for group DMs involving multiple remote servers. If server A creates a room and invites users from servers B and C, server B should receive the invite event for the user on server C (and vice versa) to maintain consistent room state.
1692-1694: LGTM!Proper deprecation annotation that preserves backward compatibility while guiding users to the new API.
packages/federation-sdk/src/sdk.ts (2)
2-2: LGTM!Import correctly added for the
UserIDtype used in the new method signature.
46-66: LGTM!Clean SDK facade implementation with proper deprecation annotation and typed delegation to the new
RoomService.createDirectMessagemethod.
| async createDirectMessage({ | ||
| creatorUserId, | ||
| members, | ||
| }: { | ||
| creatorUserId: UserID; | ||
| members: UserID[]; | ||
| }) { | ||
| const roomCreateEvent = | ||
| PersistentEventFactory.newCreateEvent(creatorUserId); |
There was a problem hiding this comment.
Missing check for existing DM room.
Unlike createDirectMessageRoom (lines 1703-1710), this method doesn't check for existing direct message rooms before creating a new one. This could lead to duplicate DM rooms between the same participants.
Consider adding a similar check using findExistingDirectMessageRoom or a variation that supports multiple members, or document that callers are responsible for this check.
🤖 Prompt for AI Agents
In packages/federation-sdk/src/services/room.service.ts around lines 1503 to
1511, this createDirectMessage function lacks the existing-DM check and may
create duplicate DM rooms; before constructing a new create event call the same
existing-room lookup used by createDirectMessageRoom (e.g.,
findExistingDirectMessageRoom) or implement a variant that accepts the creator
and members array to detect an existing room for the same participants, and if
found return that room (or its ID) instead of creating a new one; alternatively,
explicitly document that callers must ensure no existing DM exists if you choose
not to perform the check.
There was a problem hiding this comment.
this validation can be done on the application or UI side, but if the backend decides to do it differently, it shouldn't be prohibited.
There was a problem hiding this comment.
@ggazzo, understood! That's a reasonable architectural decision. Allowing the backend flexibility to create DMs without enforcing duplicate checks gives you more control at different layers of your application. The application or UI can implement the duplicate prevention logic as needed for specific use cases.
Thank you for clarifying! 👍
🧠 Learnings used
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.
|
@sampaiodiego Hi! I'm cubic. Here's how I can help with your PR: Ask questions
Request reviews
Give feedback
Request fixes
|
|
@cubic review this |
@sampaiodiego I have started the AI code review. It will take a few minutes to complete. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 3 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/federation-sdk/src/services/room.service.ts">
<violation number="1" location="packages/federation-sdk/src/services/room.service.ts:1510">
P1: Missing check for existing DM room before creation. The deprecated `createDirectMessageRoom` method checks for existing direct message rooms before creating a new one, but this new method creates a room immediately without verifying if a DM already exists between these participants. This could lead to duplicate DM rooms between the same users. Consider adding a lookup using `findExistingDirectMessageRoom` or a variant that supports multiple members before creating the room.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
| creatorUserId: UserID; | ||
| members: UserID[]; | ||
| }) { | ||
| const roomCreateEvent = |
There was a problem hiding this comment.
P1: Missing check for existing DM room before creation. The deprecated createDirectMessageRoom method checks for existing direct message rooms before creating a new one, but this new method creates a room immediately without verifying if a DM already exists between these participants. This could lead to duplicate DM rooms between the same users. Consider adding a lookup using findExistingDirectMessageRoom or a variant that supports multiple members before creating the room.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/federation-sdk/src/services/room.service.ts, line 1510:
<comment>Missing check for existing DM room before creation. The deprecated `createDirectMessageRoom` method checks for existing direct message rooms before creating a new one, but this new method creates a room immediately without verifying if a DM already exists between these participants. This could lead to duplicate DM rooms between the same users. Consider adding a lookup using `findExistingDirectMessageRoom` or a variant that supports multiple members before creating the room.</comment>
<file context>
@@ -1500,6 +1500,198 @@ export class RoomService {
+ creatorUserId: UserID;
+ members: UserID[];
+ }) {
+ const roomCreateEvent =
+ PersistentEventFactory.newCreateEvent(creatorUserId);
+
</file context>
There was a problem hiding this comment.
This validation can be done on the application or UI side, but if the backend decides to do it differently, it shouldn't be prohibited.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/federation-sdk/src/services/room.service.ts (1)
1510-1511: Pass explicitroomVersionparameter for consistency.While the default room version (
'10') is correct, other calls in this file explicitly passPersistentEventFactory.defaultRoomVersiontonewCreateEvent(). Align this call with the established pattern in the same file for consistency.const roomCreateEvent = PersistentEventFactory.newCreateEvent(creatorUserId, PersistentEventFactory.defaultRoomVersion);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
packages/federation-sdk/src/sdk.tspackages/federation-sdk/src/services/invite.service.tspackages/federation-sdk/src/services/room.service.ts
🧰 Additional context used
🧠 Learnings (2)
📚 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/sdk.tspackages/federation-sdk/src/services/room.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/room.service.ts
🧬 Code graph analysis (2)
packages/federation-sdk/src/sdk.ts (1)
packages/federation-sdk/src/index.ts (1)
UserID(33-33)
packages/federation-sdk/src/services/room.service.ts (2)
packages/room/src/manager/event-wrapper.ts (2)
residentServer(129-135)extractDomainFromId(23-29)packages/room/src/types/_common.ts (2)
RoomID(16-16)UserID(20-20)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cubic · AI code reviewer
🔇 Additional comments (11)
packages/federation-sdk/src/sdk.ts (3)
2-2: LGTM!The
UserIDtype import is correctly added to support the newcreateDirectMessagemethod signature.
46-53: LGTM!The deprecation notice is clear and provides a migration path to the new
createDirectMessagemethod while maintaining backward compatibility.
55-66: LGTM!The new
createDirectMessagemethod provides a clean API for creating direct messages with multiple members, which aligns with the PR objectives for improved group DM handling.packages/federation-sdk/src/services/room.service.ts (8)
913-919: LGTM! Correctness fix for resident server derivation.The change to derive the resident server from
roomId(line 919) instead of the sender is correct. The room's resident server is determined by the domain in the room ID, not the sender. MakingjoinUserprivate is also appropriate as it's an internal implementation detail.
1133-1133: LGTM!The call to
joinUseris correctly updated to match the refactored signature.
1545-1568: Power levels configuration addresses FB-165.The power levels are configured differently for 1:1 DMs vs group DMs:
- 1:1 DMs: both users get level 100 (line 1548)
- Group DMs: only creator gets 100
- Detailed event-specific permissions are set (lines 1552-1568)
This aligns with the PR objective FB-165 to match Element's DM power level structure. The configuration looks comprehensive and appropriate.
1681-1683: Commented-out federation broadcast.Lines 1681-1683 contain a commented-out call to
sendEventToAllServersInRoom. This suggests uncertainty about whether remote servers should be notified of member invites during DM creation.According to the Matrix federation specification, invite events are typically sent directly to the invited user's server via the
/inviteendpoint (as done on lines 1667-1670), not broadcast to all servers in the room. The commented code is likely correct to remain disabled, but please confirm this is intentional.
1692-1694: LGTM!The deprecation notice clearly indicates the migration path to
createDirectMessage.
1636-1687: LGTM! Proper handling of local vs remote member invitations.The loop correctly distinguishes between local and remote members, performing federation validation and using the appropriate invitation flow for each. The displayname extraction for invitees (line 1645) should help address PR objective FB-162 regarding invitee name display.
1644-1646: is_direct flag is correctly limited to 1:1 DMs.The implementation correctly sets
is_direct: trueonly for 1:1 DMs (members.length === 1). This matches Synapse's behavior per the Matrix specification, which treatsis_directas a signalling hint that should be set in invite events for rooms created as direct chats. Group DMs with more than one recipient should not include theis_directflag, and the code implements this correctly. The comment accurately reflects this behavior.
1521-1533: buildEvent automatically handles auth_events, prev_events, and depth calculation—the hardcoded initial values are correct.The
buildEventmethod (state.service.ts:289-290) auto-populatesauth_eventsandprev_eventswhen empty viaaddAuthEventsandaddPrevEvents. TheaddPrevEventsmethod (event-wrapper.ts:476-480) automatically recalculatesdepthbased on previous events:depth = max(prev_events.depth) + 1. The hardcoded sequential depths (2, 3, 4, 5, 6, 7+) in room.service.ts are intentional initial placeholders that are overwritten during event construction.
| await this.emitterService.emit('homeserver.matrix.membership', { | ||
| event_id: inviteEvent.eventId, | ||
| event: inviteEvent.event, | ||
| }); | ||
|
|
There was a problem hiding this comment.
Duplicate membership event emission for locally resident invites.
Lines 220-224 emit a homeserver.matrix.membership event after processing a locally resident invite. However, lines 263-266 unconditionally emit the same event after the if/else block completes. This causes locally resident invites to emit the membership event twice.
🔎 Proposed fix to remove the duplicate emission
Remove the emission at lines 220-224 since the emission at lines 263-266 already handles all cases:
if (residentServer === this.configService.serverName) {
await this.eventAuthorizationService.checkAclForInvite(
event.room_id,
residentServer,
);
await this.stateService.handlePdu(inviteEvent);
- await this.emitterService.emit('homeserver.matrix.membership', {
- event_id: inviteEvent.eventId,
- event: inviteEvent.event,
- });
-
return inviteEvent;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await this.emitterService.emit('homeserver.matrix.membership', { | |
| event_id: inviteEvent.eventId, | |
| event: inviteEvent.event, | |
| }); | |
| if (residentServer === this.configService.serverName) { | |
| await this.eventAuthorizationService.checkAclForInvite( | |
| event.room_id, | |
| residentServer, | |
| ); | |
| await this.stateService.handlePdu(inviteEvent); | |
| return inviteEvent; | |
| } |
🤖 Prompt for AI Agents
In packages/federation-sdk/src/services/invite.service.ts around lines 220 to
224 and 263 to 266, a homeserver.matrix.membership event is emitted twice for
locally resident invites (once inside the local-invite branch and again
unconditionally afterward); remove the first emission at lines 220-224 so the
single, unconditional emitter at lines 263-266 handles all cases, ensuring the
membership event is emitted only once.
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/federation-sdk/src/services/room.service.ts">
<violation number="1" location="packages/federation-sdk/src/services/room.service.ts:913">
P2: Changing `joinUser` from private to public allows external callers to bypass the invite validation in `acceptInvite`. For invite-only rooms (like DMs created by this PR), external code could attempt to join users without validating they have a proper invite first.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
| // if local room, add the user to the room if allowed. | ||
| // if remote room, run through the join process | ||
| async joinUser(roomId: RoomID, sender: UserID, userId: UserID) { | ||
| async joinUser(roomId: RoomID, userId: UserID) { |
There was a problem hiding this comment.
P2: Changing joinUser from private to public allows external callers to bypass the invite validation in acceptInvite. For invite-only rooms (like DMs created by this PR), external code could attempt to join users without validating they have a proper invite first.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/federation-sdk/src/services/room.service.ts, line 913:
<comment>Changing `joinUser` from private to public allows external callers to bypass the invite validation in `acceptInvite`. For invite-only rooms (like DMs created by this PR), external code could attempt to join users without validating they have a proper invite first.</comment>
<file context>
@@ -910,7 +910,7 @@ export class RoomService {
// if local room, add the user to the room if allowed.
// if remote room, run through the join process
- private async joinUser(roomId: RoomID, userId: UserID) {
+ async joinUser(roomId: RoomID, userId: UserID) {
const configService = this.configService;
const stateService = this.stateService;
</file context>
FB-162
FB-165
Summary by CodeRabbit
New Features
Deprecated
Bug Fixes / Reliability
✏️ Tip: You can customize this high-level summary in your review settings.