feat: stateid for all events and state endpoints #171
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #171 +/- ##
=======================================
Coverage 80.42% 80.42%
=======================================
Files 58 58
Lines 4516 4516
=======================================
Hits 3632 3632
Misses 884 884 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ggazzo
force-pushed
the
chore/state-ids-for-all-events
branch
from
d351d1b to
bcbae69
Compare
ggazzo
force-pushed
the
chore/state-ids-for-all-events
branch
from
374df87 to
ba7cbd2
Compare
WalkthroughIntroduces federation state endpoints and underlying state resolution. Adds EventService methods to return state and state_ids at an event. Overhauls StateService to reconstruct room state at/before events, adds new builders and APIs, and uses stateId when persisting events. Registers a new homeserver controller plugin. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant HS as Federation Client
participant H as Homeserver (Elysia)
participant ES as EventService
participant SS as StateService
participant Repo as State/Room Store
HS->>H: GET /_matrix/federation/v1/state_ids/:roomId?event_id=E
H->>ES: getStateIds(roomId, E)
ES->>Repo: fetch event E
ES->>SS: findStateAtEvent(E)
SS->>Repo: load state snapshot / mappings / deltas
SS-->>ES: Map<StateKey, Event>
ES->>Repo: get room version and store
loop For each state event
ES->>Repo: resolve getAuthChain(event)
Note over ES: Collect pdu_ids and auth_chain_ids<br/>(skip on per-event errors)
end
ES-->>H: { pdu_ids, auth_chain_ids }
H-->>HS: 200 OK
HS->>H: GET /_matrix/federation/v1/state/:roomId?event_id=E
H->>ES: getState(roomId, E)
ES->>Repo: fetch event E
ES->>SS: findStateAtEvent(E)
SS-->>ES: Map<StateKey, Event>
ES->>Repo: resolve getAuthChain for all
ES->>Repo: fetch auth events by ids
ES-->>H: { pdus, auth_chain }
H-->>HS: 200 OK
sequenceDiagram
autonumber
participant ES as EventService
participant SS as StateService
participant Repo as StateRepository
participant RV as RoomVersion Resolver
ES->>SS: findStateAtEvent(eventId)
SS->>Repo: get raw event by id
SS->>SS: build PDU from raw
alt event is m.room.create
SS-->>ES: empty state map
else
SS->>Repo: get stateId / mappings / deltas
SS->>RV: get room version
opt No stored state mappings
SS->>Repo: reconstruct from stored deltas/events
end
SS->>SS: buildFullRoomStateFromStateMappings(...)
SS-->>ES: Map<StateKey, PersistentEventBase>
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
✨ Finishing touches
🧪 Generate unit tests
Comment Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
ggazzo
force-pushed
the
chore/state-ids-for-all-events
branch
from
a0a52cb to
27983d5
Compare
ggazzo
force-pushed
the
chore/state-ids-for-all-events
branch
from
27983d5 to
5b6f94b
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
ggazzo
force-pushed
the
chore/state-ids-for-all-events
branch
from
55e2213 to
517bd5e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/federation-sdk/src/services/state.service.ts (2)
221-307: Potential issue: Incorrect "before event" semantics and mixing stateId with eventId.
414-452: Fix inconsistent "latest"/"last" state retrieval (sort-order mismatch).
🧹 Nitpick comments (3)
packages/federation-sdk/src/services/state.service.ts (3)
322-333: Suggest using more descriptive method name.The method
buildLastRoomStateretrieves all state mappings for a room and reconstructs the complete room state. Consider renaming tobuildFullRoomStateFromAllMappingsor similar to better convey its comprehensive nature.-private async buildLastRoomState(roomId: string): Promise<State> { +private async buildFullRoomStateFromAllMappings(roomId: string): Promise<State> {Also update the call site at line 449:
-state: await this.buildLastRoomState(roomId), +state: await this.buildFullRoomStateFromAllMappings(roomId),
381-406: Suggest batching event retrieval to avoid N+1 queries.The current implementation fetches events one by one in a loop, causing N+1 database queries. Consider batching the event retrieval for better performance.
private async buildFullRoomStateStoredEvents( events: Map<StateMapKey, string>, roomVersion: RoomVersion, ): Promise<Map<StateMapKey, PersistentEventBase>> { const finalState = new Map<StateMapKey, PersistentEventBase>(); + // Batch fetch all events at once + const eventIds = Array.from(events.values()); + const eventsCursor = this.eventRepository.findByIds(eventIds); + const eventsArray = await eventsCursor.toArray(); + const eventsById = new Map(eventsArray.map(e => [e._id, e])); + - for (const [stateKey, eventId] of events) { - const event = await this.eventRepository.findById(eventId); + for (const [stateKey, eventId] of events) { + const event = eventsById.get(eventId); if (!event) { throw new Error('Event not found'); }
460-460: Fix method call after refactoring.The call should be updated to use the new
findStateBeforeEventmethod instead of the oldfindStateAroundEvent.-const state = await this.findStateAroundEvent(eventId); +const state = await this.findStateBeforeEvent(eventId);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/federation-sdk/src/repositories/state.repository.ts(1 hunks)packages/federation-sdk/src/services/state.service.ts(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/federation-sdk/src/repositories/state.repository.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/federation-sdk/src/services/state.service.ts (4)
packages/room/src/types/_common.ts (2)
State(9-9)StateMapKey(7-7)packages/room/src/manager/factory.ts (1)
PersistentEventFactory(37-968)packages/federation-sdk/src/repositories/state.repository.ts (1)
StateStore(14-25)packages/room/src/manager/room-state.ts (1)
RoomState(15-154)
🔇 Additional comments (8)
packages/federation-sdk/src/services/state.service.ts (8)
16-16: LGTM: Clean import of StateStore type.The StateStore type import enables proper typing for the new state reconstruction builders.
209-219: LGTM: Helper method for state chain traversal.The
findPreviousStateIdmethod correctly retrieves the last previous state ID from the chain, with proper error handling.
335-349: LGTM: Well-structured state reconstruction from single mapping.The method correctly combines the current state mapping with its previous states to reconstruct the full room state.
351-363: LGTM: Clean pipeline approach for state reconstruction.The method follows a clear pipeline: mappings → event IDs → persistent events, with proper separation of concerns.
365-379: LGTM: Efficient state mapping to event ID conversion.The method correctly builds a map of state keys to event IDs from state mappings, with proper validation for missing deltas.
408-412: LGTM: Clean delegation to new state retrieval method.The refactored
getFullRoomStateproperly delegates togetFullRoomStateAndStateIdand extracts only the state portion.
848-850: LGTM: Proper stateId propagation for timeline events.The change correctly retrieves both state and stateId, ensuring the stateId is properly set when persisting timeline events instead of using an empty string.
910-910: LGTM: StateId now properly set for timeline events.The timeline event persistence now uses the retrieved stateId instead of an empty string, which aligns with the PR objective to set stateId for all events.
Docstrings generation was requested by @ggazzo. * #171 (comment) The following files were modified: * `packages/homeserver/src/homeserver.module.ts` * `packages/room/src/state_resolution/definitions/definitions.ts`
|
Note Generated docstrings for this pull request at #194 |
ggazzo
force-pushed
the
chore/state-ids-for-all-events
branch
from
234ea6d to
da3ba13
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/federation-sdk/src/services/state.service.ts (1)
261-276: Incorrect “before event” semantics when there is no previous stateWhen
prevStateIds.length === 0, this path unconditionally adds the current event to the returned “before” snapshot. It should return an empty state unlessincludeEventis true.- // TODO: not sure which case is this - if (storedState.prevStateIds.length === 0) { - const previous = await this.eventRepository.findById( - storedState.delta.eventId, - ); - if (!previous) { - throw new Error(`Event ${eventId} not found`); - } - - state.set( - storedState.delta.identifier, - PersistentEventFactory.createFromRawEvent(previous.event, roomVersion), - ); - - return state; - } + // No previous state recorded -> "state before event" has no entry for this key, + // unless includeEvent is true. + if (storedState.prevStateIds.length === 0) { + if (includeEvent) { + const current = await this.eventRepository.findById( + storedState.delta.eventId, + ); + if (!current) { + throw new Error(`Event ${storedState.delta.eventId} not found`); + } + state.set( + storedState.delta.identifier as StateMapKey, + PersistentEventFactory.createFromRawEvent(current.event, roomVersion), + ); + } + return state; + }Follow-up: the same edge case exists in
findStateAroundEvent(...)at Lines 156-171 — please apply the same guard there.
🧹 Nitpick comments (5)
packages/federation-sdk/src/services/state.service.ts (5)
249-252: Fix logger call shapelogger.error should get a context object first, not a string. Use
{ eventId }for structured logs.- if (!storedState) { - this.logger.error(eventId, 'last state delta not found'); + if (!storedState) { + this.logger.error({ eventId }, 'last state delta not found');
278-301: Eliminate N+1 event lookups; clarify variable naming
- Batch-fetch events referenced by the mappings to avoid 1 query per state key.
eventsToFetchholds state IDs, not event IDs — rename tostateIdsToFetchfor clarity.- const eventsToFetch = [ - includeEvent && storedState._id.toString(), - ...storedState.prevStateIds, - ].filter(Boolean) as string[]; - - const stateMappings = await this.stateRepository - .getStateMappingsByStateIdsOrdered(eventsToFetch) - .toArray(); - - for await (const { delta } of stateMappings) { - const { identifier: stateKey, eventId } = delta; - const event = await this.eventRepository.findById(eventId); - if (!event) { - throw new Error(`Event ${eventId} not found`); - } - state.set( - stateKey as StateMapKey, - PersistentEventFactory.createFromRawEvent(event.event, roomVersion), - ); - } + const stateIdsToFetch = [ + includeEvent && storedState._id.toString(), + ...storedState.prevStateIds, + ].filter(Boolean) as string[]; + + const stateMappings = await this.stateRepository + .getStateMappingsByStateIdsOrdered(stateIdsToFetch) + .toArray(); + + const eventIds = stateMappings.map(({ delta }) => delta.eventId); + const eventsCursor = this.eventRepository.findByIds(eventIds); + const events = await eventsCursor.toArray(); + const eventsById = new Map(events.map((e) => [e._id, e])); + + for (const { delta } of stateMappings) { + const { identifier: stateKey, eventId } = delta; + const eventDoc = eventsById.get(eventId); + if (!eventDoc) { + throw new Error(`Event ${eventId} not found`); + } + state.set( + stateKey as StateMapKey, + PersistentEventFactory.createFromRawEvent(eventDoc.event, roomVersion), + ); + }
316-327: Fallback path is O(N) over room history — gate and measureThis rebuild scans all mappings ascending; fine as a fallback, but add a counter/metric to track usage and consider a feature flag to disable in prod.
375-399: Hydration does N+1 DB calls; batch by IDsBatch
findByIdsfor alleventIds and index them to avoid per-key lookups.- for (const [stateKey, eventId] of events) { - const event = await this.eventRepository.findById(eventId); - if (!event) { - throw new Error('Event not found'); - } - const pdu = PersistentEventFactory.createFromRawEvent( - event.event, - roomVersion, - ); - if (pdu.eventId !== eventId) { - throw new Error('Event id mismatch while building room state'); - } - finalState.set(stateKey, pdu); - } + const allEventIds = Array.from(events.values()); + const cursor = this.eventRepository.findByIds(allEventIds); + const docs = await cursor.toArray(); + const byId = new Map(docs.map((d) => [d._id, d])); + for (const [stateKey, eventId] of events) { + const doc = byId.get(eventId); + if (!doc) { + throw new Error(`Event ${eventId} not found`); + } + const pdu = PersistentEventFactory.createFromRawEvent(doc.event, roomVersion); + if (pdu.eventId !== eventId) { + throw new Error('Event id mismatch while building room state'); + } + finalState.set(stateKey, pdu); + }
454-455: Route to the stricter “before event” APITo avoid duplicating semantics and to respect the “before” contract (especially for rooms with no prior state), call
findStateBeforeEvent(eventId)here.- const state = await this.findStateAroundEvent(eventId); + const state = await this.findStateBeforeEvent(eventId);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
packages/federation-sdk/src/repositories/state.repository.ts(1 hunks)packages/federation-sdk/src/services/event.service.ts(2 hunks)packages/federation-sdk/src/services/state.service.ts(6 hunks)packages/homeserver/src/controllers/federation/event.controller.ts(1 hunks)packages/homeserver/src/controllers/federation/profiles.controller.ts(2 hunks)packages/homeserver/src/dtos/federation/profiles.dto.ts(1 hunks)packages/homeserver/src/dtos/federation/state-ids.dto.ts(1 hunks)packages/homeserver/src/dtos/federation/state.dto.ts(1 hunks)packages/homeserver/src/dtos/index.ts(1 hunks)packages/homeserver/src/homeserver.module.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/federation-sdk/src/repositories/state.repository.ts
- packages/homeserver/src/dtos/federation/profiles.dto.ts
- packages/homeserver/src/homeserver.module.ts
- packages/homeserver/src/controllers/federation/event.controller.ts
- packages/homeserver/src/dtos/federation/state.dto.ts
- packages/homeserver/src/dtos/federation/state-ids.dto.ts
- packages/homeserver/src/dtos/index.ts
- packages/federation-sdk/src/services/event.service.ts
- packages/homeserver/src/controllers/federation/profiles.controller.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/federation-sdk/src/services/state.service.ts (4)
packages/room/src/types/_common.ts (2)
State(9-9)StateMapKey(7-7)packages/room/src/manager/factory.ts (1)
PersistentEventFactory(37-144)packages/federation-sdk/src/repositories/state.repository.ts (1)
StateStore(14-25)packages/room/src/manager/room-state.ts (1)
RoomState(15-154)
🔇 Additional comments (6)
packages/federation-sdk/src/services/state.service.ts (6)
22-22: Import of StateStore type looks correctType is used below in the builders; no issues.
329-343: Good use of typed StateStore and single-call expansionFetching
[state._id, ...prevStateIds]then delegating to builders keeps concerns separated.
345-357: Builder composition LGTMThe split between event-id reconstruction and event hydration is clean.
402-406: Thin wrapper LGTMKeeping a single source of truth in
getFullRoomStateAndStateIdis good.
860-863: Good: fetch state and stateId together for timeline eventsUsing
getFullRoomStateAndStateIdensures the event is persisted against the correct stateId.
917-923: Correctly persist timeline event with derived stateIdPassing
stateIdties the message to the exact room state snapshot at write time; this avoids orphaned events.
| private async buildFullRoomStateFromEvents( | ||
| stateMappings: StateStore[], | ||
| ): Promise<Map<StateMapKey, string>> { | ||
| const state = new Map<StateMapKey, string>(); | ||
| // first reconstruct the final state | ||
| for await (const stateMapping of stateMappings) { | ||
| if (!stateMapping.delta) { | ||
| throw new Error('State mapping has no delta'); | ||
| } | ||
|
|
||
| if (!stateMapping.delta) { | ||
| throw new Error('State mapping delta is empty'); | ||
| } | ||
| const { identifier: stateKey, eventId } = stateMapping.delta; | ||
|
|
||
| state.set(stateKey as StateMapKey, eventId); | ||
| state.set(stateKey, eventId); | ||
| } | ||
|
|
||
| return state; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Minor: avoid for-await over arrays
stateMappings is an array; use for (const ...) to avoid implying async iteration.
- for await (const stateMapping of stateMappings) {
+ for (const stateMapping of stateMappings) {Also ensure getStateMappingsBy*Ordered returns mappings in ascending order so later entries override earlier ones deterministically.
🏁 Script executed:
#!/bin/bash
rg -nP -C2 'getStateMappingsByStateIdsOrdered|getStateMappingsByRoomIdOrderedAscending' packages/federation-sdk/src/repositories/state.repository.tsLength of output: 233
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="packages/federation-sdk/src/repositories/state.repository.ts"
nl -ba "$file" | sed -n '40,90p'Length of output: 40
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="packages/federation-sdk/src/repositories/state.repository.ts"
if [ -f "$file" ]; then
awk '{printf "%5d: %s\n", NR, $0}' "$file"
else
echo "MISSING: $file"
fiLength of output: 3220
Minor: replace async iteration with regular for-of
stateMappings is typed as StateStore[] — use a plain loop instead of implying async iteration.
- for await (const stateMapping of stateMappings) {
+ for (const stateMapping of stateMappings) {getStateMappingsByRoomIdOrdered and getStateMappingsByStateIdsOrdered already sort by createdAt ascending in packages/federation-sdk/src/repositories/state.repository.ts, so ordering is deterministic.
📝 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.
| private async buildFullRoomStateFromEvents( | |
| stateMappings: StateStore[], | |
| ): Promise<Map<StateMapKey, string>> { | |
| const state = new Map<StateMapKey, string>(); | |
| // first reconstruct the final state | |
| for await (const stateMapping of stateMappings) { | |
| if (!stateMapping.delta) { | |
| throw new Error('State mapping has no delta'); | |
| } | |
| if (!stateMapping.delta) { | |
| throw new Error('State mapping delta is empty'); | |
| } | |
| const { identifier: stateKey, eventId } = stateMapping.delta; | |
| state.set(stateKey as StateMapKey, eventId); | |
| state.set(stateKey, eventId); | |
| } | |
| return state; | |
| } | |
| private async buildFullRoomStateFromEvents( | |
| stateMappings: StateStore[], | |
| ): Promise<Map<StateMapKey, string>> { | |
| const state = new Map<StateMapKey, string>(); | |
| // first reconstruct the final state | |
| for (const stateMapping of stateMappings) { | |
| if (!stateMapping.delta) { | |
| throw new Error('State mapping has no delta'); | |
| } | |
| const { identifier: stateKey, eventId } = stateMapping.delta; | |
| state.set(stateKey, eventId); | |
| } | |
| return state; | |
| } |
🤖 Prompt for AI Agents
In packages/federation-sdk/src/services/state.service.ts around lines 359 to
373, the method uses an async iterator ("for await (const stateMapping of
stateMappings)") even though stateMappings is typed as a plain StateStore[];
replace the async iteration with a regular for-of loop ("for (const stateMapping
of stateMappings)") and remove the unnecessary async modifier from the function
signature (and any unused awaits) so the function is a normal synchronous
function that returns a Promise immediately.
ec85ee5 to
322bb64
Compare
ggazzo
force-pushed
the
chore/get-auth-chain
branch
from
ac21e3f to
4a41834
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/federation-sdk/src/services/state.service.ts (2)
359-373: Replace async iteration over an array with a plain loop.stateMappings is an array; for-await implies async sources.
- for await (const stateMapping of stateMappings) { + for (const stateMapping of stateMappings) {
215-276: Fix “before event” semantics when there is no previous state.If prevStateIds is empty, you currently always include the current event’s delta in the “before” snapshot. That’s incorrect unless includeEvent is true. Also the error references eventId rather than storedState.delta.eventId.
Apply:
- // TODO: not sure which case is this - if (storedState.prevStateIds.length === 0) { - const previous = await this.eventRepository.findById( - storedState.delta.eventId, - ); - if (!previous) { - throw new Error(`Event ${eventId} not found`); - } - - state.set( - storedState.delta.identifier, - PersistentEventFactory.createFromRawEvent(previous.event, roomVersion), - ); - - return state; - } + if (storedState.prevStateIds.length === 0) { + // No recorded previous state => state before this event has no entry for this key, + // unless the caller explicitly asks to include the event itself. + if (includeEvent) { + const current = await this.eventRepository.findById( + storedState.delta.eventId, + ); + if (!current) { + throw new Error(`Event ${storedState.delta.eventId} not found`); + } + state.set( + storedState.delta.identifier as StateMapKey, + PersistentEventFactory.createFromRawEvent(current.event, roomVersion), + ); + } + return state; + }
🧹 Nitpick comments (3)
packages/federation-sdk/src/services/state.service.ts (3)
278-301: Avoid N+1 lookups and keep includeEvent out of the stateIds query.Batch-fetch events for prevStateIds, then, if includeEvent is true, append the last delta explicitly. Also replace async iteration over arrays.
- const eventsToFetch = [ - includeEvent && storedState._id.toString(), - ...storedState.prevStateIds, - ].filter(Boolean) as string[]; - - const stateMappings = await this.stateRepository - .getStateMappingsByStateIdsOrdered(eventsToFetch) - .toArray(); - - for await (const { delta } of stateMappings) { - const { identifier: stateKey, eventId } = delta; - const event = await this.eventRepository.findById(eventId); - if (!event) { - throw new Error(`Event ${eventId} not found`); - } - - state.set( - stateKey as StateMapKey, - PersistentEventFactory.createFromRawEvent(event.event, roomVersion), - ); - } - - return state; + const stateMappings = await this.stateRepository + .getStateMappingsByStateIdsOrdered(storedState.prevStateIds) + .toArray(); + + // batch-fetch referenced events + const eventIds = stateMappings.map(({ delta }) => delta.eventId); + const eventsCursor = this.eventRepository.findByIds(eventIds); + const events = await eventsCursor.toArray(); + const eventsById = new Map(events.map((e) => [e._id, e])); + + for (const { delta } of stateMappings) { + const { identifier: stateKey, eventId: deltaEventId } = delta; + const eventDoc = eventsById.get(deltaEventId); + if (!eventDoc) { + throw new Error(`Event ${deltaEventId} not found`); + } + state.set( + stateKey as StateMapKey, + PersistentEventFactory.createFromRawEvent(eventDoc.event, roomVersion), + ); + } + + if (includeEvent) { + const { identifier: lastStateKey, eventId: lastStateEventId } = + storedState.delta; + const lastEvent = await this.eventRepository.findById(lastStateEventId); + if (!lastEvent) { + throw new Error(`Event ${lastStateEventId} not found`); + } + state.set( + lastStateKey as StateMapKey, + PersistentEventFactory.createFromRawEvent(lastEvent.event, roomVersion), + ); + } + + return state;
375-397: Batch-fetch events to avoid N+1 queries in a hot path.buildFullRoomStateStoredEvents fetches each event individually. Batch once and index by _id.
- for (const [stateKey, eventId] of events) { - const event = await this.eventRepository.findById(eventId); - if (!event) { - throw new Error('Event not found'); - } - - const pdu = PersistentEventFactory.createFromRawEvent( - event.event, - roomVersion, - ); - - if (pdu.eventId !== eventId) { - throw new Error('Event id mismatch while building room state'); - } - - finalState.set(stateKey, pdu); - } + const eventIds = Array.from(events.values()); + const cursor = this.eventRepository.findByIds(eventIds); + const docs = await cursor.toArray(); + const byId = new Map(docs.map((d) => [d._id, d])); + + for (const [stateKey, eventId] of events) { + const doc = byId.get(eventId); + if (!doc) { + throw new Error(`Event ${eventId} not found`); + } + const pdu = PersistentEventFactory.createFromRawEvent( + doc.event, + roomVersion, + ); + if (pdu.eventId !== eventId) { + throw new Error('Event id mismatch while building room state'); + } + finalState.set(stateKey, pdu); + }
454-454: Avoid divergence: reuse findStateBeforeEvent for the RoomState wrapper.This calls findStateAroundEvent (defaults to “before”) while a dedicated findStateBeforeEvent exists. Prefer routing both wrappers through a single helper to prevent drift.
- const state = await this.findStateAroundEvent(eventId); + const state = await this.findStateBeforeEvent(eventId);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/federation-sdk/src/services/event.service.ts(2 hunks)packages/federation-sdk/src/services/state.service.ts(6 hunks)packages/homeserver/src/controllers/federation/event.controller.ts(1 hunks)packages/homeserver/src/homeserver.module.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/homeserver/src/homeserver.module.ts
- packages/federation-sdk/src/services/event.service.ts
- packages/homeserver/src/controllers/federation/event.controller.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/federation-sdk/src/services/state.service.ts (4)
packages/room/src/types/_common.ts (2)
State(9-9)StateMapKey(7-7)packages/room/src/manager/factory.ts (1)
PersistentEventFactory(37-144)packages/federation-sdk/src/repositories/state.repository.ts (1)
StateStore(14-25)packages/room/src/manager/room-state.ts (1)
RoomState(15-154)
🔇 Additional comments (7)
packages/federation-sdk/src/services/state.service.ts (7)
22-22: LGTM: type import addition is correct.Brings StateStore into scope for the new helpers.
329-343: LGTM: single mapping → full state path is sound.Pulls the chain (self + prev) and reuses the shared builders.
345-357: LGTM: compose-from-multiple mappings reads well.Clear separation between collecting eventIds and materializing PDUs.
402-406: LGTM: wrapper preserves API and leverages cached stateId path.
860-863: LGTM: persisting timeline uses resolved stateId.Correctly attaches stateId from getFullRoomStateAndStateId before validation/persist.
922-922: LGTM: writes the computed stateId with the timeline event.
316-327: Confirmed: ascending order is correct.
getStateMappingsByRoomIdOrderedAscending uses this.collection.find({ roomId }).sort({ createdAt: 1 }), so it returns mappings oldest→newest and buildLastRoomState's assumption is valid.
…d processing logic
There was a problem hiding this comment.
Actionable comments posted: 0
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/state.service.ts (1)
194-208: Performance concern: N+1 database queries.The loop fetches events one by one, causing N+1 database queries. This should be optimized to batch fetch all required events.
Consider applying this optimization pattern from the later builder methods:
- for await (const { delta } of stateMappings) { - const { identifier: stateKey, eventId } = delta; - const event = await this.eventRepository.findById(eventId); - if (!event) { - throw new Error(`Event ${eventId} not found`); - } - state.set( - stateKey, - PersistentEventFactory.createFromRawEvent(event.event, roomVersion), - ); - } + // Batch fetch all events + const eventIds = stateMappings.map(({ delta }) => delta.eventId); + const eventsCursor = this.eventRepository.findByIds(eventIds); + const events = await eventsCursor.toArray(); + const eventsById = new Map(events.map((e) => [e._id, e])); + + for (const { delta } of stateMappings) { + const { identifier: stateKey, eventId } = delta; + const event = eventsById.get(eventId); + if (!event) { + throw new Error(`Event ${eventId} not found`); + } + state.set( + stateKey, + PersistentEventFactory.createFromRawEvent(event.event, roomVersion), + ); + }
♻️ Duplicate comments (3)
packages/federation-sdk/src/services/state.service.ts (3)
158-175: Potential issue: including current event in "before event" state when prevStateIds is empty.When
prevStateIds.length === 0, this code includes the current event (storedState.delta.eventId) in the returned state. However, since this method is documented to return state "prior to the event," including the current event may be semantically incorrect for the first state change in a room.
266-280: Use regular for-of loop instead of for-await.The
stateMappingsparameter is typed asStateStore[](a regular array), so usingfor awaitis unnecessary and potentially misleading.
177-188: Logic error: mixing eventId with stateIds in query.Line 186 pushes
storedState._id.toString()(a stateId) whenincludeEventis true, butgetStateMappingsByStateIdsOrderedexpects state IDs, not event IDs. This will cause incorrect query results.Apply this diff to fix the logic:
- const eventsToFetch = [ - includeEvent && storedState._id.toString(), - ...storedState.prevStateIds, - ].filter(Boolean) as string[]; + const eventsToFetch = storedState.prevStateIds;Then handle
includeEventseparately after processing the state mappings, similar to the pattern used in the later builder methods.
🧹 Nitpick comments (1)
packages/federation-sdk/src/repositories/state.repository.ts (1)
34-34: Remove debug logging from production code.This console.log statement will pollute production logs and should be removed before merging to main. Consider using the proper logger if debugging information is needed.
Apply this diff to remove the debug logging:
- console.log('stateId ->>>', stateId);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/federation-sdk/src/repositories/state.repository.ts(1 hunks)packages/federation-sdk/src/services/event.service.ts(2 hunks)packages/federation-sdk/src/services/state.service.ts(8 hunks)packages/homeserver/src/controllers/federation/event.controller.ts(1 hunks)packages/homeserver/src/homeserver.module.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/homeserver/src/homeserver.module.ts
- packages/federation-sdk/src/services/event.service.ts
- packages/homeserver/src/controllers/federation/event.controller.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/federation-sdk/src/services/state.service.ts (4)
packages/room/src/types/_common.ts (2)
State(9-9)StateMapKey(7-7)packages/room/src/manager/factory.ts (1)
PersistentEventFactory(37-144)packages/federation-sdk/src/repositories/state.repository.ts (1)
StateStore(14-25)packages/room/src/manager/room-state.ts (1)
RoomState(15-154)
🔇 Additional comments (13)
packages/federation-sdk/src/services/state.service.ts (13)
22-22: LGTM! Proper type import for state builder parameters.The import of
StateStoretype enables proper typing for the new private state-builder methods, which improves type safety across the state reconstruction pipeline.
113-116: LGTM! Clear documentation of state semantics.The JSDoc comment clarifies that this method returns state prior to the event, which is essential for understanding the validation context.
117-141: LGTM! Improved method naming and early return for create events.The rename from
findStateBeforeEventtofindStateAtEventbetter reflects the method's purpose, and the early return form.room.createevents correctly handles the edge case where no prior state exists.
143-157: LGTM! Proper state retrieval and error handling.The method correctly retrieves the stored state using the event's
stateIdand provides clear error messages when state is not found. The debug logging will help with troubleshooting state reconstruction issues.
223-234: LGTM! Well-structured private helper method.The
buildLastRoomStatemethod follows good practices by validating room version and delegating to the appropriate builder method.
236-250: LGTM! Modular state reconstruction from single mapping.This builder method properly reconstructs state from a single
StateStoremapping and its previous state IDs, following a clear pipeline from mappings → events → persistent events.
252-264: LGTM! Clean delegation pattern.The method follows good separation of concerns by delegating to specialized builder methods.
282-307: LGTM! Robust event validation and reconstruction.The method properly validates that fetched events match expected IDs and handles the reconstruction pipeline correctly with appropriate error handling.
309-313: LGTM! Clean wrapper method.The method provides a convenient wrapper around
getFullRoomStateAndStateIdfor callers who only need the state.
315-353: LGTM! Well-structured state retrieval with fallback.The method properly attempts to use stored state first and falls back to building the last room state when needed. The TODO comment indicates this fallback is intentionally kept for production stability during the transition period.
355-363: LGTM! Consistent wrapper methods.Both
getFullRoomState2andgetFullRoomStateBeforeEvent2provide clean wrappers that convert from the internalMap<StateMapKey, PersistentEventBase>format to theRoomStateclass.
767-770: LGTM! Proper state ID attachment for timeline events.The change to use
getFullRoomStateAndStateIdensures that timeline events are persisted with the correctstateId, which aligns with the PR objective of setting state IDs for all events.
829-829: LGTM! Consistent state ID usage.Using the
stateIdfrom the destructured result ensures timeline events are properly associated with their state context.
…vice for improved clarity and consistency
ggazzo
force-pushed
the
chore/state-ids-for-all-events
branch
from
8403766 to
2675b3a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/federation-sdk/src/services/state.service.ts (2)
194-205: Performance: N+1 database queries in state reconstructionThe loop fetches events one by one from the database, which will cause performance issues with large state sets. Consider batch fetching all events.
Replace the N+1 queries with batch fetching:
+const eventIds = stateMappings.map(({ delta }) => delta.eventId); +const eventsCursor = this.eventRepository.findByIds(eventIds); +const events = await eventsCursor.toArray(); +const eventsById = new Map(events.map(e => [e._id, e])); + -for await (const { delta } of stateMappings) { +for (const { delta } of stateMappings) { const { identifier: stateKey, eventId } = delta; - const event = await this.eventRepository.findById(eventId); + const event = eventsById.get(eventId); if (!event) { throw new Error(`Event ${eventId} not found`); } state.set( stateKey, PersistentEventFactory.createFromRawEvent(event.event, roomVersion), ); }
288-304: Performance: N+1 database queries in buildFullRoomStateStoredEventsSimilar to the earlier issue, this method fetches events one by one. Batch fetch for better performance.
Batch fetch the events:
private async buildFullRoomStateStoredEvents( events: Map<StateMapKey, string>, roomVersion: RoomVersion, ): Promise<Map<StateMapKey, PersistentEventBase>> { const finalState = new Map<StateMapKey, PersistentEventBase>(); + + const eventIds = Array.from(events.values()); + const eventsCursor = this.eventRepository.findByIds(eventIds); + const eventDocs = await eventsCursor.toArray(); + const eventsById = new Map(eventDocs.map(e => [e._id, e])); - for (const [stateKey, eventId] of events) { - const event = await this.eventRepository.findById(eventId); + for (const [stateKey, eventId] of events) { + const event = eventsById.get(eventId); if (!event) { throw new Error('Event not found'); } const pdu = PersistentEventFactory.createFromRawEvent( event.event, roomVersion, ); if (pdu.eventId !== eventId) { throw new Error('Event id mismatch while building room state'); } finalState.set(stateKey, pdu); } return finalState; }
♻️ Duplicate comments (1)
packages/federation-sdk/src/services/state.service.ts (1)
185-188: Potential type confusion: mixing state IDs with state mapping IDsLine 186 uses
storedState._id.toString()(the MongoDB ObjectId of the state mapping) whenincludeEventis true, butgetStateMappingsByStateIdsOrderedexpects state IDs. This appears to be a logic error where the state mapping's ObjectId is incorrectly used as a state ID.The state mapping's
_id(ObjectId) should not be mixed with state IDs. Based on the pattern inbuildFullRoomStateFromStateMapping(lines 240-245), you likely want:const eventsToFetch = [ - includeEvent && storedState._id.toString(), + includeEvent && stateId, ...storedState.prevStateIds, ].filter(Boolean) as string[];Note that
stateIdis already available from line 144 where it's destructured from the event.
🧹 Nitpick comments (3)
packages/federation-sdk/src/services/state.service.ts (3)
113-117: Documentation is slightly misleadingThe JSDoc comment says "This is the state prior to the event" but the implementation actually includes the event itself for timeline events (when
includeEvent = !pdu.isState()on line 177). Consider clarifying the documentation to reflect this behavior.Update the documentation to be more precise:
/** * Returns the state used to validate the event - * This is the state prior to the event + * For state events: Returns the state prior to the event + * For timeline events: Returns the state including the event itself */
271-271: Remove unnecessary async iterationThe
for awaitloop is unnecessary sincestateMappingsis already a resolved array (not an async iterable).-for await (const stateMapping of stateMappings) { +for (const stateMapping of stateMappings) {
338-342: Clean up commented production checkThere's commented-out code that appears to be a production environment check. Either remove it or uncomment it with proper configuration.
Either remove the commented code or make it configurable:
-// TODO: we should be ok to remove the fallback scenario for production, but since we are already running in some places I wanted to keep it for now - -// if (process.env.NODE_ENV === 'development') { -// throw new Error('No stored state found, and we are in production'); -// } +// TODO: Remove fallback scenario once all environments have migrated +if (this.configService.requireStoredState) { + throw new Error('No stored state found'); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/federation-sdk/src/services/event.service.ts(2 hunks)packages/federation-sdk/src/services/state.service.ts(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/federation-sdk/src/services/event.service.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/federation-sdk/src/services/state.service.ts (5)
packages/room/src/types/_common.ts (2)
State(9-9)StateMapKey(7-7)packages/room/src/manager/event-wrapper.ts (3)
event(102-112)stateKey(90-94)roomId(72-74)packages/room/src/manager/factory.ts (1)
PersistentEventFactory(37-144)packages/federation-sdk/src/repositories/state.repository.ts (1)
StateStore(14-25)packages/room/src/manager/room-state.ts (1)
RoomState(15-154)
🔇 Additional comments (3)
packages/federation-sdk/src/services/state.service.ts (3)
767-769: Good: State ID properly attached to timeline eventsThe change to use
getFullRoomStateAndStateIdand pass thestateIdwhen persisting timeline events is correct and aligns with the PR objective.
641-641: No change required — findStateAtEvent returns the pre-event state for state events.The method computes includeEvent = !pdu.isState(), so for the persistStateEvent path (a state event) the event is excluded; calling findStateAtEvent(lastEvent._id) therefore returns the state prior to the event and is correct.
160-175: Verify empty prevStateIds handling (packages/federation-sdk/src/services/state.service.ts:160-175) — createStateMapping accepts a default prevStateIds = [] and is invoked from this file, but I couldn't find where prevStateIds is populated; confirm the producer. If prevStateIds can legitimately be empty (initial state), keep this branch but add a clear comment and a unit test that reconstructs state via storedState.delta.eventId; if not, remove/simplify this special-case branch and ensure previous-state resolution is deterministic.
packages/homeserver/src/controllers/federation/event.controller.ts
Outdated
Show resolved
Hide resolved
ggazzo
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/federation-sdk/src/services/state.service.ts (2)
173-190: Eliminate N+1 lookups and async iteration over arrays in findStateAtEvent.Batch fetch events and use a plain for‑of loop. This path is hot.
- const stateMappings = await this.stateRepository - .getStateMappingsByStateIdsOrdered(eventsToFetch) - .toArray(); - - for await (const { delta } of stateMappings) { - const { identifier: stateKey, eventId } = delta; - const event = await this.eventRepository.findById(eventId); - if (!event) { - throw new Error(`Event ${eventId} not found`); - } - - state.set( - stateKey, - PersistentEventFactory.createFromRawEvent(event.event, roomVersion), - ); - } + const stateMappings = await this.stateRepository + .getStateMappingsByStateIdsOrdered(eventsToFetch) + .toArray(); + + const eventIds = stateMappings.map(({ delta }) => delta.eventId); + const eventsCursor = this.eventRepository.findByIds(eventIds); + const events = await eventsCursor.toArray(); + const eventsById = new Map(events.map((e) => [e._id, e])); + + for (const { delta } of stateMappings) { + const { identifier: stateKey, eventId: deltaEventId } = delta; + const eventDoc = eventsById.get(deltaEventId); + if (!eventDoc) { + throw new Error(`Event ${deltaEventId} not found`); + } + state.set( + stateKey as StateMapKey, + PersistentEventFactory.createFromRawEvent(eventDoc.event, roomVersion), + ); + }
254-260: Replace async iteration over plain arrays.- for await (const stateMapping of stateMappings) { + for (const stateMapping of stateMappings) {
🧹 Nitpick comments (6)
packages/homeserver/src/controllers/federation/state.controller.ts (2)
23-34: Expose 404/500 error schemas and map thrown errors to ErrorResponseDto.Only 200/400 are declared. Service-level “not found”/unexpected errors will surface as 500 without a documented shape. Add 404/500 to response to keep OpenAPI in sync and help clients.
response: { - 400: ErrorResponseDto, + 400: ErrorResponseDto, + 404: ErrorResponseDto, + 500: ErrorResponseDto, }, @@ response: { - 400: ErrorResponseDto, + 400: ErrorResponseDto, + 404: ErrorResponseDto, + 500: ErrorResponseDto, },Also applies to: 43-52
38-41: Drop the non‑null assertion; DTO already guarantees presence.GetStateQueryDto requires event_id, so the “!” isn’t needed.
- ({ params, query }) => - eventService.getState(params.roomId, query.event_id!), + ({ params, query }) => + eventService.getState(params.roomId, query.event_id),packages/federation-sdk/src/services/state.service.ts (4)
162-168: Clarify doc comment: “includeEvent” refers to last state mapping, not the timeline event.The state for a timeline event should be the room state at that point; the event itself isn’t part of the state. Reword to avoid confusion.
- * If the event is a state event, we don't include the event in the state, otherwise the state would be the new state - * computed because of the event - * - * If the event is a timeline event, we include the event in the state + * If the event is a state event, do not include its own mapping; we want the state prior to it. + * If the event is a timeline event, include the latest state mapping that existed at that time.
149-151: Structured logging object for error.Pass a context object to logger for consistency with other calls.
- this.logger.error(eventId, 'last state delta not found'); + this.logger.error({ eventId }, 'last state delta not found');
271-287: Batch event loads when materializing stored events.Avoid per-item findById; fetch all, index, then build.
- for (const [stateKey, eventId] of events) { - const event = await this.eventRepository.findById(eventId); - if (!event) { - throw new Error('Event not found'); - } - - const pdu = PersistentEventFactory.createFromRawEvent( - event.event, - roomVersion, - ); - - if (pdu.eventId !== eventId) { - throw new Error('Event id mismatch while building room state'); - } - - finalState.set(stateKey, pdu); - } + const ids = Array.from(events.values()); + const cursor = this.eventRepository.findByIds(ids); + const docs = await cursor.toArray(); + const byId = new Map(docs.map((e) => [e._id, e])); + + for (const [stateKey, eventId] of events) { + const doc = byId.get(eventId); + if (!doc) { + throw new Error('Event not found'); + } + const pdu = PersistentEventFactory.createFromRawEvent( + doc.event, + roomVersion, + ); + if (pdu.eventId !== eventId) { + throw new Error('Event id mismatch while building room state'); + } + finalState.set(stateKey, pdu); + }
847-849: Tighten TODOs/comments (public API surface).Clean up TODO phrasing to avoid ambiguous/internal notes in published code.
- // TODO: i know thisd is overcomplicated - //but writing this comment while not remembering what exactkly it does while not wanting to get my brain to do it either + // TODO: Simplify/clarify this flow; current implementation is more complex than necessary. @@ - // TODO: why it must to end whit `:` ? + // NOTE: Identifiers for state events include a trailing ":" by convention.Also applies to: 886-887
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/federation-sdk/src/services/state.service.ts(8 hunks)packages/homeserver/src/controllers/federation/state.controller.ts(1 hunks)packages/homeserver/src/homeserver.module.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/homeserver/src/homeserver.module.ts
🧰 Additional context used
🧬 Code graph analysis (2)
packages/homeserver/src/controllers/federation/state.controller.ts (3)
packages/homeserver/src/dtos/federation/state-ids.dto.ts (3)
GetStateIdsParamsDto(4-6)GetStateIdsQueryDto(8-10)GetStateIdsResponseDto(12-17)packages/homeserver/src/dtos/common/error.dto.ts (1)
ErrorResponseDto(3-6)packages/homeserver/src/dtos/federation/state.dto.ts (3)
GetStateParamsDto(4-6)GetStateQueryDto(8-10)GetStateResponseDto(12-19)
packages/federation-sdk/src/services/state.service.ts (4)
packages/room/src/types/_common.ts (2)
State(9-9)StateMapKey(7-7)packages/room/src/manager/factory.ts (1)
PersistentEventFactory(37-144)packages/federation-sdk/src/repositories/state.repository.ts (1)
StateStore(14-25)packages/room/src/manager/room-state.ts (1)
RoomState(15-154)
🔇 Additional comments (2)
packages/homeserver/src/controllers/federation/state.controller.ts (1)
14-21: LGTM: plugin wiring and DI are clean.Route paths, DTO usage, and tsyringe resolution look correct.
packages/federation-sdk/src/services/state.service.ts (1)
750-753: LGTM: timeline persistence now attaches the correct stateId.Persisting timeline events with the current room stateId fixes “empty stateId” issues downstream.
Also applies to: 812-813
| const storedState = await this.stateRepository.getStateById(stateId); | ||
|
|
||
| if (!lastStateDelta) { | ||
| if (!storedState) { | ||
| this.logger.error(eventId, 'last state delta not found'); | ||
| throw new Error(`State at event ${eventId} not found`); | ||
| } |
There was a problem hiding this comment.
Handle events persisted without a stateId (fallback by timestamp).
findStateAtEvent throws when stateId is empty/missing (e.g., conflicted state events stored without mapping). Use a fallback to the latest state mapping at/before the event timestamp.
- const storedState = await this.stateRepository.getStateById(stateId);
+ let storedState = await this.stateRepository.getStateById(stateId);
@@
- if (!storedState) {
- this.logger.error(eventId, 'last state delta not found');
- throw new Error(`State at event ${eventId} not found`);
- }
+ if (!storedState) {
+ // Fallback: event was stored without a state mapping (e.g., rejected/conflicted).
+ const lastWithState =
+ await this.eventRepository.findLatestEventByRoomIdBeforeTimestampWithAssociatedState(
+ event.event.room_id,
+ event.event.origin_server_ts,
+ );
+ const fallbackStateId = lastWithState?.stateId;
+ if (!fallbackStateId) {
+ this.logger.error({ eventId }, 'state mapping not found at/before event');
+ throw new Error(`State at event ${eventId} not found`);
+ }
+ const fallback = await this.stateRepository.getStateById(fallbackStateId);
+ if (!fallback) {
+ this.logger.error({ eventId, fallbackStateId }, 'fallback state missing');
+ throw new Error(`State mapping ${fallbackStateId} not found`);
+ }
+ storedState = fallback;
+ }Also applies to: 120-131
🤖 Prompt for AI Agents
In packages/federation-sdk/src/services/state.service.ts around lines 146-151
(and similarly 120-131), the current logic throws when a referenced stateId is
missing; change it to handle events persisted without a stateId by falling back
to the latest state mapping at or before the event timestamp: first check if
stateId is present and try stateRepository.getStateById; if that returns null or
if stateId is missing, call a repository method to fetch the latest state
mapping at or before the event's timestamp (e.g.,
getLatestStateByTimestamp/getStateMappingAtOrBefore) and use that state; only
log and throw if both direct lookup and the timestamp-based fallback return
nothing, and include eventId/timestamp in logs for context.
Summary by CodeRabbit
New Features
Improvements