refactor: standardize event creation in RoomService and PersistentEve… #188
Conversation
WalkthroughRefactors event construction across services to use a unified builder (StateService.buildEvent / PersistentEventFactory.newEvent), replacing numerous specialized factory methods. Updates schemas and types (message content unions, empty state_key constraint; tombstone auth fields required). Adjusts controllers and services to the new build-and-persist flow, removing manual auth/prev wiring and signing calls. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller as Service/Controller
participant State as StateService
participant Factory as PersistentEventFactory
participant Store as Persistence
Caller->>State: buildEvent(rawEvent, roomVersion)
State->>Factory: createFromRawEvent(rawEvent, roomVersion)
Factory-->>State: PersistentEventBase
State->>State: addAuthEvents(event)
State->>State: addPrevEvents(event)
State->>State: signEvent(event)
State-->>Caller: event
Caller->>Store: persistStateEvent/persistTimelineEvent(event)
Store-->>Caller: event_id
sequenceDiagram
autonumber
participant Client as MessageService
participant State as StateService
participant Store as EventStore
participant Fed as Federation
Client->>State: buildEvent<'m.room.message'>(content+relations)
State-->>Client: event
Client->>Store: persistTimelineEvent(event)
Store-->>Client: event_id
Client->>Fed: notify/broadcast(event)
Fed-->>Client: ack
sequenceDiagram
autonumber
participant InvSvc as InviteService
participant State as StateService
participant Fed as FederationService
participant Store as EventStore
alt Local invite
InvSvc->>State: buildEvent<'m.room.member'>(membership: invite)
State-->>InvSvc: event
InvSvc->>Store: persistStateEvent(event)
InvSvc->>Fed: notifyAllServers(event)
else Remote invite
InvSvc->>Fed: inviteUser(rawInvite)
Fed-->>InvSvc: signedInviteEvent
InvSvc->>Store: persistStateEvent(signedInviteEvent)
InvSvc->>Fed: notifyAllServers(signedInviteEvent)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## chore/message-type #188 +/- ##
=======================================================
+ Coverage 68.94% 80.41% +11.46%
=======================================================
Files 58 58
Lines 5288 4514 -774
=======================================================
- Hits 3646 3630 -16
+ Misses 1642 884 -758 ☔ 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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/room/src/types/v3-11.ts (1)
553-557: Bug: m.room.tombstone must be an empty-state-key state eventModeled as a timeline event here; switch to the empty-state-key state schema to match core and avoid validation/build-id mismatches.
File: packages/room/src/types/v3-11.ts:553-557
Apply:
-const EventPduTypeRoomTombstone = z.object({ - ...PduNoContentTimelineEventSchema, +const EventPduTypeRoomTombstone = z.object({ + ...PduNoContentEmptyStateKeyStateEventSchema, type: z.literal('m.room.tombstone'), content: PduRoomTombstoneEventContentSchema, });
🧹 Nitpick comments (6)
packages/federation-sdk/src/services/send-join.service.ts (1)
35-38: Unified builder: OK — stateService signs events before persisting; remove redundant explicit signbuildEvent calls this.signEvent (state.service.ts:353) and persistStateEvent also signs before storing (state.service.ts:453), so persisting joinEvent at send-join.service.ts:49 is safe; remove the redundant stateService.signEvent(joinEvent) at send-join.service.ts:71.
packages/federation-sdk/src/services/invite.service.ts (1)
44-69: Robust localpart extraction for DM invitesUse a safe regex to extract the localpart and only include displayname when present.
-const displayname = isDirectMessage - ? userId.split(':').shift()?.slice(1) - : undefined; +const displayname = isDirectMessage + ? (() => { + const m = userId.match(/^@([^:]+):/); + return m ? m[1] : undefined; + })() + : undefined;buildEvent (StateService.buildEvent) populates auth_events and prev_events via addAuthEvents/addPrevEvents, but it does NOT compute/set depth — callers must provide the correct depth when constructing the raw event.
packages/federation-sdk/src/services/state.service.ts (2)
371-384: Cap prev_events and avoid overriding provided prev_events.
Spec and interop benefit from a small prev set. Also, if caller set prev_events, skip recomputing.Apply:
- private async addPrevEvents(event: PersistentEventBase) { + private async addPrevEvents(event: PersistentEventBase) { + // If caller supplied prev_events, don't override. + if ((event.event.prev_events?.length ?? 0) > 0) { + return; + } const roomVersion = await this.getRoomVersion(event.roomId); if (!roomVersion) { throw new Error('Room version not found while filling prev events'); } const prevEvents = await this.eventRepository.findPrevEvents(event.roomId); - event.addPrevEvents( - prevEvents.map((e) => - PersistentEventFactory.createFromRawEvent(e.event, roomVersion), - ), - ); + const MAX_PREV = 10; + event.addPrevEvents( + prevEvents.slice(0, MAX_PREV).map((e) => + PersistentEventFactory.createFromRawEvent(e.event, roomVersion), + ), + ); }
386-406: Harden signing: validate serverName and key; check returned signature.
Prevents silent failures on misconfig.Apply:
- async signEvent(event: PersistentEventBase) { - const signingKey = await this.configService.getSigningKey(); - - const origin = this.configService.serverName; + async signEvent(event: PersistentEventBase) { + const signingKey = await this.configService.getSigningKey(); + const origin = this.configService.serverName; + if (!origin) { + throw new Error('Server name is not configured; cannot sign event'); + } + if (!Array.isArray(signingKey) || !signingKey[0]) { + throw new Error('Signing key not available; cannot sign event'); + } @@ - const keyId = `${signingKey[0].algorithm}:${signingKey[0].version}`; - - event.addSignature(origin, keyId, result.signatures[origin][keyId]); + const keyId = `${signingKey[0].algorithm}:${signingKey[0].version}`; + const sig = result.signatures?.[origin]?.[keyId]; + if (!sig) { + throw new Error('Signing failed: missing signature in result'); + } + event.addSignature(origin, keyId, sig); return event; }packages/federation-sdk/src/services/message.service.ts (1)
376-403: Rename variable: redactionEvent → messageUpdateEvent.
This block creates a message replacement, not a redaction.Apply:
- const redactionEvent = await this.stateService.buildEvent<'m.room.message'>( + const messageUpdateEvent = await this.stateService.buildEvent<'m.room.message'>( @@ - await this.stateService.persistTimelineEvent(redactionEvent); + await this.stateService.persistTimelineEvent(messageUpdateEvent); @@ - void this.federationService.sendEventToAllServersInRoom(redactionEvent); + void this.federationService.sendEventToAllServersInRoom(messageUpdateEvent); @@ - return redactionEvent.eventId; + return messageUpdateEvent.eventId;packages/federation-sdk/src/services/room.service.ts (1)
1226-1227: Displayname extraction: avoid slice(1) pitfalls.
User IDs may not start with '@' in all cases.Apply:
- const creatorDisplayname = creatorUserId.split(':').shift()?.slice(1); + const creatorDisplayname = creatorUserId.split(':',1)[0]?.replace(/^@/, ''); @@ - const displayname = targetUserId.split(':').shift()?.slice(1); + const displayname = targetUserId.split(':',1)[0]?.replace(/^@/, '');Also applies to: 1343-1344
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
packages/core/src/events/m.room.tombstone.ts(1 hunks)packages/federation-sdk/src/services/invite.service.ts(1 hunks)packages/federation-sdk/src/services/message.service.ts(9 hunks)packages/federation-sdk/src/services/profiles.service.ts(1 hunks)packages/federation-sdk/src/services/room.service.ts(13 hunks)packages/federation-sdk/src/services/send-join.service.ts(1 hunks)packages/federation-sdk/src/services/state.service.ts(4 hunks)packages/homeserver/src/controllers/internal/invite.controller.ts(1 hunks)packages/homeserver/src/controllers/internal/room.controller.ts(1 hunks)packages/room/src/manager/factory.ts(1 hunks)packages/room/src/types/v3-11.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
packages/federation-sdk/src/services/profiles.service.ts (1)
packages/room/src/manager/event-wrapper.ts (1)
roomId(72-74)
packages/homeserver/src/controllers/internal/invite.controller.ts (2)
packages/room/src/manager/factory.ts (1)
PersistentEventFactory(37-144)packages/room/src/types/v3-11.ts (1)
PduCreateEventContent(148-148)
packages/homeserver/src/controllers/internal/room.controller.ts (3)
packages/room/src/manager/factory.ts (1)
PersistentEventFactory(37-144)packages/room/src/manager/event-wrapper.ts (1)
roomId(72-74)packages/room/src/types/v3-11.ts (1)
PduCreateEventContent(148-148)
packages/federation-sdk/src/services/state.service.ts (5)
packages/room/src/types/v3-11.ts (1)
PduType(45-45)packages/room/src/manager/event-wrapper.ts (2)
event(102-112)PduWithHashesAndSignaturesOptional(33-35)packages/room/src/types/_common.ts (1)
PduForType(11-11)packages/room/src/manager/type.ts (1)
RoomVersion(14-14)packages/room/src/manager/factory.ts (1)
PersistentEventFactory(37-144)
packages/federation-sdk/src/services/message.service.ts (4)
packages/room/src/manager/event-wrapper.ts (2)
event(102-112)roomId(72-74)packages/core/src/events/m.reaction.ts (1)
reactionEvent(51-94)packages/room/src/manager/v3.ts (1)
eventId(21-33)packages/core/src/events/m.room.redaction.ts (1)
redactionEvent(36-87)
packages/federation-sdk/src/services/room.service.ts (3)
packages/core/src/events/m.room.create.ts (1)
roomCreateEvent(26-43)packages/room/src/manager/factory.ts (1)
PersistentEventFactory(37-144)packages/room/src/manager/event-wrapper.ts (3)
roomId(72-74)sender(76-78)event(102-112)
packages/room/src/manager/factory.ts (4)
packages/room/src/types/v3-11.ts (1)
PduType(45-45)packages/room/src/manager/event-wrapper.ts (2)
event(102-112)PduWithHashesAndSignaturesOptional(33-35)packages/room/src/types/_common.ts (1)
PduForType(11-11)packages/room/src/manager/type.ts (1)
RoomVersion(14-14)
🔇 Additional comments (11)
packages/room/src/types/v3-11.ts (3)
476-480: Good addition: empty state_key state schemaIntroducing PduNoContentEmptyStateKeyStateEventSchema is correct and aligns with spec for state events that must use an empty state_key.
482-485: m.room.create: enforce empty state_keySwitching to the empty-state-key schema is correct; create events must have state_key ''.
494-497: join_rules and power_levels: enforce empty state_keyBoth event types must use state_key ''. This narrows types safely and prevents accidental non-empty keys.
Also applies to: 500-503
packages/core/src/events/m.room.tombstone.ts (1)
23-26: Stronger typing for auth_eventsRequiring strings here is good; it ensures compile-time presence. Keeping the isTruthy filter still guards against empty strings.
packages/homeserver/src/controllers/internal/room.controller.ts (1)
372-386: Ban membership event: align with builder pathSame as invites — replace the manual PersistentEventFactory.newEvent with stateService.buildEvent so prev/auth events and canonical fields are handled consistently.
-const membershipEvent = - PersistentEventFactory.newEvent<'m.room.member'>(/* payload */, roomVersion); +const membershipEvent = + await stateService.buildEvent<'m.room.member'>(/* payload */, roomVersion);Verify event_id stability after the change (compare generated event_id or run unit/integration tests).
packages/homeserver/src/controllers/internal/invite.controller.ts (1)
39-52: Prefer stateService.buildEvent for consistencyUsing PersistentEventFactory.newEvent bypasses StateService's auth/prev enrichment and canonicalization — switch to stateService.buildEvent (async) to match other invite paths.
File: packages/homeserver/src/controllers/internal/invite.controller.ts (lines 39-52)
-const membershipEvent = PersistentEventFactory.newEvent<'m.room.member'>( - { /* payload */ }, - createEvent.getContent<PduCreateEventContent>().room_version, -); +const membershipEvent = await stateService.buildEvent<'m.room.member'>( + { /* same payload */ }, + createEvent.getContent<PduCreateEventContent>().room_version, +);Verify that the resulting event_id for invites from this route matches pre-change behavior; if it differs, standardize all invite paths to use buildEvent.
packages/federation-sdk/src/services/state.service.ts (2)
8-14: Types import LGTM.
The new generics enable strongly typed build flow.
358-369: Auth wiring centralization LGTM.
This keeps timeline/state validation happy.packages/federation-sdk/src/services/room.service.ts (1)
1064-1082: Avoid overriding provided prev/auth in buildEvent for tombstone.
This payload already supplies prev_events, depth, and auth_events. Ensure StateService.addAuthEvents/addPrevEvents are no-ops when fields are prefilled to avoid duplication/mismatch.Would you like me to adjust buildEvent to skip add* steps when arrays are non-empty?
packages/room/src/manager/factory.ts (1)
138-143: Unified newEvent wrapper LGTM.
Thin delegate to createFromRawEvent keeps API consistent and typed.packages/federation-sdk/src/services/message.service.ts (1)
57-74: Resolved — buildEvent already populates prev_events and updates depth before signing.
StateService.buildEvent calls addPrevEvents (which uses eventRepository.findPrevEvents) and PersistentEventBase.addPrevEvents bumps rawEvent.depth based on prev events, so the depth: 0 placeholders in message.service are overridden and no change is required.
| const membershipEvent = await stateService.buildEvent<'m.room.member'>( | ||
| { | ||
| type: 'm.room.member', | ||
| content: { membership: 'join' }, | ||
| room_id: roomId, | ||
| state_key: userId, | ||
| auth_events: [], | ||
| depth: 0, | ||
| prev_events: [], | ||
| origin_server_ts: Date.now(), | ||
| sender: userId, | ||
| }, | ||
| roomInformation.room_version, | ||
| ); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm make_join template vs fully-built event
Using stateService.buildEvent centralizes logic—good. For federation “make_join”, confirm we should populate origin_server_ts/depth and compute IDs/signatures here vs returning a minimal template to the joiner.
🌐 Web query:
Matrix federation make_join: What fields must the returned event include? Should auth_events, prev_events, depth, origin_server_ts be populated by the resident server, or left for the joining server?
💡 Result:
Short answer: the resident must return a proto-event that includes auth_events, prev_events, depth and the other common PDU fields (type, sender, state_key, content, room_id, origin, signatures/ hashes may be present in some room-versions). The joining server is expected to fill in (or replace) the event_id, origin and origin_server_ts on that template and then sign the resulting event before calling send_join. Room-version details can change exact format, so follow the room-version rules when building the final PDU.
Sources: Matrix Server‑Server API (make_join / send_join) and legacy make_join description. [1][2]
References
[1] Matrix Spec — Server‑Server API (make_join / send_join) — describes the template, and states the joining server should add/replace origin, origin_server_ts and event_id.
[2] Legacy make_join description — lists required proto-event fields (type, auth_events, content, depth, origin, origin_server_ts, prev_events, room_id, sender, state_key) and shows the joining server adds event_id/origin/origin_server_ts.
Do not populate origin/origin_server_ts/event_id in the make_join proto-event — return a template only.
Matrix spec: the resident should return a proto-event containing type, sender, state_key, content, room_id, auth_events, prev_events and depth; the joining server must add/replace origin, origin_server_ts and event_id and sign before send_join.
File: packages/federation-sdk/src/services/profiles.service.ts (lines 82–95) — remove origin_server_ts: Date.now() (and avoid setting event_id/origin/signatures) while keeping auth_events/prev_events/depth per room-version rules.
🤖 Prompt for AI Agents
In packages/federation-sdk/src/services/profiles.service.ts around lines 82–95,
the make_join proto-event is being populated with origin_server_ts (and must not
set event_id/origin/signatures); change the function to return only the
proto-event template containing type, sender, state_key, content, room_id,
auth_events, prev_events and depth (per room version rules) and remove
origin_server_ts: Date.now() and any assignment of event_id/origin/signatures so
the joining server can add/replace origin, origin_server_ts and event_id and
sign before send_join.
| await stateService.buildEvent<'m.room.member'>( | ||
| { | ||
| type: 'm.room.member', | ||
| content: { | ||
| membership: 'join', | ||
| is_direct: true, | ||
| displayname: displayname, | ||
| }, | ||
| room_id: roomCreateEvent.roomId, | ||
| state_key: targetUserId, | ||
| auth_events: [], | ||
| depth: 0, | ||
| prev_events: [], | ||
| origin_server_ts: Date.now(), | ||
| sender: creatorUserId, | ||
| }, | ||
| PersistentEventFactory.defaultRoomVersion, | ||
| ); |
There was a problem hiding this comment.
Join event sender/state_key mismatch for local DM target.
A join should be authored by the joining user; current code sets sender to creatorUserId. Either:
- change to sender: targetUserId for membership: 'join', or
- switch to an invite flow (membership: 'invite', sender: creatorUserId) and let the target join.
Apply minimal fix:
- sender: creatorUserId,
+ sender: targetUserId,📝 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 stateService.buildEvent<'m.room.member'>( | |
| { | |
| type: 'm.room.member', | |
| content: { | |
| membership: 'join', | |
| is_direct: true, | |
| displayname: displayname, | |
| }, | |
| room_id: roomCreateEvent.roomId, | |
| state_key: targetUserId, | |
| auth_events: [], | |
| depth: 0, | |
| prev_events: [], | |
| origin_server_ts: Date.now(), | |
| sender: creatorUserId, | |
| }, | |
| PersistentEventFactory.defaultRoomVersion, | |
| ); | |
| await stateService.buildEvent<'m.room.member'>( | |
| { | |
| type: 'm.room.member', | |
| content: { | |
| membership: 'join', | |
| is_direct: true, | |
| displayname: displayname, | |
| }, | |
| room_id: roomCreateEvent.roomId, | |
| state_key: targetUserId, | |
| auth_events: [], | |
| depth: 0, | |
| prev_events: [], | |
| origin_server_ts: Date.now(), | |
| sender: targetUserId, | |
| }, | |
| PersistentEventFactory.defaultRoomVersion, | |
| ); |
🤖 Prompt for AI Agents
In packages/federation-sdk/src/services/room.service.ts around lines 1346 to
1363, the generated 'm.room.member' join event incorrectly uses sender:
creatorUserId while state_key is targetUserId; for a direct join the sender must
be the joining user. Fix by changing the event sender to targetUserId when
membership is 'join' (leave the rest of the payload unchanged), or alternatively
convert this to an 'invite' event (membership: 'invite') with sender:
creatorUserId if you intend an invite flow—apply the minimal change to set
sender: targetUserId for the current join path.
Updated message content types in the core and federation SDK to better categorize message types into text, file, and location. Introduced new schemas for message content, including detailed structures for file information and new content for edits. This refactor improves type safety and clarity in message handling across the application.
…ntFactory Updated the RoomService and PersistentEventFactory to use a new unified method for creating events, enhancing code consistency and readability. This change replaces multiple event creation methods with a single `newEvent` method, streamlining the process of generating various room-related events.
Refactored the event creation process in InviteService, MessageService, ProfilesService, RoomService, and StateService to utilize the new `buildEvent` method. This change enhances code consistency and readability by standardizing how events are constructed and signed across different services.
…ntFactory Eliminated multiple outdated methods for creating various room events in PersistentEventFactory. This refactor simplifies the codebase by relying on the unified `newEvent` method, enhancing maintainability and consistency in event generation.
Updated ProfilesService, RoomService, and SendJoinService to utilize the new `buildEvent` method for creating room events. This refactor enhances code consistency and readability by standardizing the event construction process across multiple services, while also removing deprecated methods for adding authentication and previous events.
Updated message content types in the core and federation SDK to better categorize message types into text, file, and location. Introduced new schemas for message content, including detailed structures for file information and new content for edits. This refactor improves type safety and clarity in message handling across the application.
ggazzo
force-pushed
the
chore/simplfy-factory
branch
from
a562524 to
f7848ef
Compare
3 participants
…ntFactory
Updated the RoomService and PersistentEventFactory to use a new unified method for creating events, enhancing code consistency and readability. This change replaces multiple event creation methods with a single
newEventmethod, streamlining the process of generating various room-related events.Summary by CodeRabbit
New Features
Refactor