fix: change create DM room generic call to newCreateEvent #205
Conversation
WalkthroughRefactors two call sites in room.service to create m.room.create events via PersistentEventFactory.newCreateEvent(creatorUserId, version) instead of manually constructing the event with newEvent<'m.room.create'>(...). The returned roomCreateEvent (with roomId and eventId) is then used as before; surrounding logic remains unchanged. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant RoomService
participant PersistentEventFactory
participant EventStore
Caller->>RoomService: createRoom(...)
RoomService->>PersistentEventFactory: newCreateEvent(creatorUserId, version)
PersistentEventFactory-->>RoomService: roomCreateEvent (roomId, eventId, content)
RoomService->>EventStore: persist(roomCreateEvent)
EventStore-->>RoomService: ack
RoomService-->>Caller: result (includes roomId/eventId)
Note over RoomService,PersistentEventFactory: Changed: uses newCreateEvent instead of manual newEvent payload
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #205 +/- ##
=======================================
Coverage 81.01% 81.01%
=======================================
Files 63 63
Lines 4692 4692
=======================================
Hits 3801 3801
Misses 891 891 ☔ 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
📜 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/room.service.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/federation-sdk/src/services/room.service.ts (2)
packages/core/src/events/m.room.create.ts (1)
roomCreateEvent(26-43)packages/room/src/manager/factory.ts (1)
PersistentEventFactory(37-144)
| const roomCreateEvent = PersistentEventFactory.newCreateEvent( | ||
| creatorUserId, | ||
| PersistentEventFactory.defaultRoomVersion, | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
Swap to PersistentEventFactory.newCreateEvent looks correct; add a guard to ensure local creator and avoid bad roomId domains
newCreateEvent derives the roomId domain from creatorUserId. If creatorUserId is not local or contains a port/IPv6 (e.g., @U:example.org:8448), we may create a room hosted on the wrong “resident” server, which breaks subsequent logic that assumes a locally hosted room. Add a defensive check before creating the room.
Apply near this call:
@@
- const stateService = this.stateService;
+ const stateService = this.stateService;
+ // Ensure the DM is created on this server; prevents room_id domain mismatch
+ const creatorServerName = creatorUserId.split(':').slice(1).join(':');
+ if (creatorServerName !== localServerName) {
+ throw new HttpException('Creator must be a local user to create a DM room on this server', HttpStatus.BAD_REQUEST);
+ }
- const roomCreateEvent = PersistentEventFactory.newCreateEvent(
+ const roomCreateEvent = PersistentEventFactory.newCreateEvent(
creatorUserId,
PersistentEventFactory.defaultRoomVersion,
);Optional follow‑up: consider extending newCreateEvent to accept an explicit domain/serverName to avoid inferring it from a userId string.
Run to verify no remaining call sites use newEvent<'m.room.create'> and to inspect DM room creation call sites:
🏁 Script executed:
#!/bin/bash
rg -nP --type=ts "PersistentEventFactory\.newEvent<\s*'m\.room\.create'"
rg -n --type=ts -C3 "createDirectMessageRoom\("Length of output: 1350
🏁 Script executed:
#!/bin/bash
# Show the implementation of createDirectMessageRoom to confirm where newCreateEvent is called and context variables
awk 'NR>=1170 && NR<=1250' packages/federation-sdk/src/services/room.service.ts || echo "File not found"Length of output: 2083
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Searching for PersistentEventFactory..."
rg -n --hidden "PersistentEventFactory" || true
echo
echo "Locating file that defines the factory..."
file=$(rg -n --hidden "export class PersistentEventFactory|class PersistentEventFactory" --hidden --files-with-matches | head -n1 || true)
echo "Found: $file"
if [ -n "$file" ]; then
echo
echo "Showing file contents around newCreateEvent..."
rg -n "newCreateEvent" "$file" -n -N -S || true
sed -n '1,240p' "$file"
fiLength of output: 11441
Require creator to be local before calling PersistentEventFactory.newCreateEvent
newCreateEvent derives the room_id domain from the creator string (packages/room/src/manager/factory.ts uses creator.split(':').pop()). createDirectMessageRoom calls it without verifying the creator is local (packages/federation-sdk/src/services/room.service.ts ≈ line 1197) — this can produce a room hosted on the wrong server. Add a defensive guard.
@@
- const stateService = this.stateService;
+ const stateService = this.stateService;
+ // Ensure the DM is created on this server; prevents room_id domain mismatch
+ const creatorServerName = creatorUserId.split(':').slice(1).join(':');
+ if (creatorServerName !== localServerName) {
+ throw new HttpException('Creator must be a local user to create a DM room on this server', HttpStatus.BAD_REQUEST);
+ }
- const roomCreateEvent = PersistentEventFactory.newCreateEvent(
+ const roomCreateEvent = PersistentEventFactory.newCreateEvent(
creatorUserId,
PersistentEventFactory.defaultRoomVersion,
);Optional: extend newCreateEvent to accept an explicit serverName to avoid inferring the domain.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/federation-sdk/src/services/room.service.ts around lines 1197-1200,
the code calls PersistentEventFactory.newCreateEvent(creatorUserId, ...) without
ensuring the creator is a local user, which lets newCreateEvent infer the
room_id domain from a non-local creator and can produce a room hosted on the
wrong server; add a defensive guard that verifies the creatorUserId is local
(use the existing isLocalUser/isLocalMxid utility or compare the MXID domain to
the local server name) and throw or return an error if not local before calling
newCreateEvent, or alternatively refactor to call newCreateEvent with an
explicit serverName parameter (update factory signature and call sites) so the
room domain is not inferred from the creator.
Summary by CodeRabbit