refactor: propagate room version through PersistentEventBase #230
Conversation
WalkthroughThe changes thread explicit room version from events into event wrappers and factory constructors, replace per-event async fetches with ID-based store lookups for auth/prev events, tighten several type signatures (notably power-level/create events), and update state resolution and authorization code to use the new helpers and generics. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor HS as Homeserver
participant SS as StateService
participant PF as PersistentEventFactory
participant EV as PersistentEvent*
participant ES as EventStore
HS->>SS: persistTimelineEvent(rawEvent)
SS->>SS: derive roomVersion = rawEvent.version
SS->>PF: createFromRawEvent(rawEvent, roomVersion)
PF-->>HS: EV (constructed with version)
SS->>EV: getAuthEventIds()
SS->>ES: getEvents(authEventIds)
ES-->>SS: authEvents
SS->>EV: getPreviousEventIds()
SS->>ES: getEvents(prevEventIds)
ES-->>SS: prevEvents
SS->>SS: validate & persist with provided version
SS-->>HS: result
sequenceDiagram
autonumber
participant SR as StateResolution
participant ES as EventStore
participant EV as PersistentEvent*
SR->>EV: getAuthEventIds()
SR->>ES: getEvents(authEventIds)
ES-->>SR: authEvents
SR->>SR: mainlineOrdering(..., powerLevelEvent: PersistentEventBase<RoomVersion,'m.room.power_levels'>)
SR-->>SR: compute ordering
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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 #230 +/- ##
==========================================
+ Coverage 81.67% 81.74% +0.07%
==========================================
Files 63 63
Lines 4682 4695 +13
==========================================
+ Hits 3824 3838 +14
+ Misses 858 857 -1 ☔ 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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/room/src/state_resolution/definitions/definitions.ts (2)
120-142: Auth-chain memoization is incomplete; cache result for non-leaf events.You only cache when there are zero auth events. This defeats the intended memoization for typical events and can cause redundant store calls.
Apply this diff to cache the computed chain for all events:
- const authEvents = await store.getEvents(event.getAuthEventIds()); + const authEvents = await store.getEvents(event.getAuthEventIds()); if (authEvents.length === 0) { eventIdToAuthChainMap.set(eventId, existingAuthChainPart); return existingAuthChainPart; } const authEventIdsSet = new Set(authEvents.map((e) => e.eventId)); let newAuthChainPart = existingAuthChainPart.union(authEventIdsSet); for (const authEvent of authEvents) { const nextAuthChainPart = await _getAuthChain( authEvent, newAuthChainPart, ); if (!nextAuthChainPart) { continue; } newAuthChainPart = newAuthChainPart.union(nextAuthChainPart); } - return newAuthChainPart; + eventIdToAuthChainMap.set(eventId, newAuthChainPart); + return newAuthChainPart;
350-371: Missing await in recursive graph build causes race conditions.buildIndegreeGraph is async but recursive calls aren’t awaited. Graph and power-level maps may be incomplete when used.
Apply this diff:
- if (conflictedSet.has(authEvent.eventId)) { - graph.get(event.eventId)?.add(authEvent.eventId); // add this as an edge - - buildIndegreeGraph(graph, authEvent); - } + if (conflictedSet.has(authEvent.eventId)) { + graph.get(event.eventId)?.add(authEvent.eventId); // add this as an edge + await buildIndegreeGraph(graph, authEvent); + }
🧹 Nitpick comments (8)
packages/room/src/manager/room-state.ts (1)
146-155: Prefer using the wrapper’s version over content castUse the strongly-typed event wrapper field instead of reading and casting from content.
Apply this diff:
- return createEvent.getContent().room_version as RoomVersion; + return createEvent.version;packages/federation-sdk/src/services/state.service.ts (3)
85-95: Narrow the return type to RoomVersionAlign with types by casting the create event’s room_version to RoomVersion.
Apply this diff:
- return createEvent.event.content.room_version; + return createEvent.event.content.room_version as RoomVersion;
441-445: Redundant undefined check on event.versionevent.version is non-optional on PersistentEventBase; the guard can be dropped.
Apply this diff:
- const roomVersion = event.version; - if (!roomVersion) { - throw new Error('Room version not found while filling prev events'); - } + const roomVersion = event.version;
591-596: Redundant undefined check on event.versionSame note as above; simplify.
Apply this diff:
- const roomVersion = event.version; - - if (!roomVersion) { - throw new Error('Room version not found'); - } + const roomVersion = event.version;packages/room/src/manager/event-wrapper.ts (1)
423-434: Harden addPrevEvents/authedBy: empty arrays, dedupe, and depth from max
- addPrevEvents assumes non-empty events; calling with [] will crash.
- prev_events/auth_events may be undefined on raw input; initialize before push.
- Depth should use max depth of provided events (order not guaranteed).
Apply this diff:
- addPrevEvents(events: PersistentEventBase<Version>[]) { - this.rawEvent.prev_events.push(...events.map((e) => e.eventId)); - if (this.rawEvent.depth <= events[events.length - 1].depth) { - this.rawEvent.depth = events[events.length - 1].depth + 1; - } - return this; - } + addPrevEvents(events: PersistentEventBase<Version>[]) { + if (!events?.length) { + return this; + } + this.rawEvent.prev_events ??= []; + this.rawEvent.prev_events.push(...events.map((e) => e.eventId)); + const maxDepth = Math.max(...events.map((e) => e.depth)); + if (this.rawEvent.depth <= maxDepth) { + this.rawEvent.depth = maxDepth + 1; + } + return this; + } - authedBy(event: PersistentEventBase<Version>) { - this.rawEvent.auth_events.push(event.eventId); - return this; - } + authedBy(event: PersistentEventBase<Version>) { + this.rawEvent.auth_events ??= []; + if (!this.rawEvent.auth_events.includes(event.eventId)) { + this.rawEvent.auth_events.push(event.eventId); + } + return this; + }packages/room/src/manager/v9.ts (1)
1-6: Prefer a type-only import forPduType.
PduTypeis exported purely as a type alias, so we can lean onimport typehere to keep the emitted JS tidy.-import { PduType } from '../types/v3-11'; +import type { PduType } from '../types/v3-11';packages/room/src/manager/v6.ts (1)
1-6: ImportPduTypeas a type-only dependency.Same rationale as in
v9.ts: usingimport typeavoids pulling in a non-existent runtime export.-import { PduType } from '../types/v3-11'; +import type { PduType } from '../types/v3-11';packages/room/src/authorizartion-rules/rules.ts (1)
6-6: ImportRoomVersionas a type.
RoomVersionis also a type alias; switching to a type-only import keeps the emitted module surface clean.-import { RoomVersion } from '../manager/type'; +import type { RoomVersion } from '../manager/type';
📜 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 (13)
packages/federation-sdk/src/services/state.service.ts(5 hunks)packages/room/src/authorizartion-rules/rules.ts(3 hunks)packages/room/src/manager/event-wrapper.ts(5 hunks)packages/room/src/manager/factory.ts(3 hunks)packages/room/src/manager/power-level-event-wrapper.ts(1 hunks)packages/room/src/manager/room-state.ts(1 hunks)packages/room/src/manager/v11.ts(1 hunks)packages/room/src/manager/v3.ts(2 hunks)packages/room/src/manager/v6.ts(1 hunks)packages/room/src/manager/v8.ts(1 hunks)packages/room/src/manager/v9.spec.ts(1 hunks)packages/room/src/manager/v9.ts(1 hunks)packages/room/src/state_resolution/definitions/definitions.ts(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (11)
packages/room/src/manager/v8.ts (2)
packages/room/src/types/v3-11.ts (1)
PduType(47-47)packages/room/src/manager/v6.ts (1)
PersistentEventV6(4-15)
packages/room/src/manager/power-level-event-wrapper.ts (2)
packages/federation-sdk/src/index.ts (1)
PersistentEventBase(21-21)packages/room/src/manager/type.ts (1)
RoomVersion(14-14)
packages/room/src/authorizartion-rules/rules.ts (2)
packages/core/src/events/m.room.create.ts (1)
roomCreateEvent(26-43)packages/room/src/manager/type.ts (1)
RoomVersion(14-14)
packages/room/src/manager/v11.ts (2)
packages/room/src/types/v3-11.ts (1)
PduType(47-47)packages/room/src/manager/v9.ts (1)
PersistentEventV9(4-16)
packages/room/src/manager/factory.ts (5)
packages/room/src/manager/v3.ts (1)
PersistentEventV3(12-72)packages/room/src/manager/v6.ts (1)
PersistentEventV6(4-15)packages/room/src/manager/v8.ts (1)
PersistentEventV8(4-14)packages/room/src/manager/v9.ts (1)
PersistentEventV9(4-16)packages/room/src/manager/v11.ts (1)
PersistentEventV11(5-46)
packages/room/src/manager/event-wrapper.ts (2)
packages/room/src/manager/type.ts (1)
RoomVersion(14-14)packages/room/src/manager/room-state.ts (1)
version(146-155)
packages/room/src/state_resolution/definitions/definitions.ts (1)
packages/room/src/manager/type.ts (1)
RoomVersion(14-14)
packages/room/src/manager/v9.ts (2)
packages/room/src/types/v3-11.ts (1)
PduType(47-47)packages/room/src/manager/v8.ts (1)
PersistentEventV8(4-14)
packages/room/src/manager/v6.ts (2)
packages/room/src/types/v3-11.ts (1)
PduType(47-47)packages/room/src/manager/v3.ts (1)
PersistentEventV3(12-72)
packages/room/src/manager/v3.ts (3)
packages/room/src/types/v3-11.ts (1)
PduType(47-47)packages/federation-sdk/src/index.ts (1)
PersistentEventBase(21-21)packages/room/src/manager/type.ts (1)
RoomVersion3To11(3-12)
packages/room/src/manager/v9.spec.ts (3)
packages/room/src/manager/event-wrapper.ts (1)
event(108-117)packages/room/src/manager/v9.ts (1)
PersistentEventV9(4-16)packages/room/src/types/_common.ts (1)
EventID(8-8)
🔇 Additional comments (20)
packages/federation-sdk/src/services/state.service.ts (2)
481-482: LGTM: using event.version for store/state opsConsistent with the PR’s direction to thread version via the wrapper.
743-744: LGTM: timeline path now uses event.version for storeMatches the new store/getEvents flow.
packages/room/src/manager/event-wrapper.ts (4)
44-46: Generic parameterization looks goodSwitching to Version extends RoomVersion with a sensible default improves type safety across wrappers.
147-153: New helpers for IDs are clear and align with store-based retrievalNaming reflects return values; no concerns.
167-216: Type guards updated to carry Version — goodNarrowing to PersistentEventBase<Version, ...> keeps downstream code precise.
53-56: Update direct instantiations to include the new version parameter
- packages/room/src/manager/v9.spec.ts:7 – add the
versionargument tonew PersistentEventV9(...)- Scan for any other
new PersistentEvent*(calls (especially in tests) and pass the appropriateversion- Verify each
PersistentEventV*subclass constructor forwards(event, version)tosuperpackages/room/src/manager/v8.ts (1)
4-6: Generic Type parameterization: LGTMMatches v3/v6/v9/v11 and keeps the event type precise.
packages/room/src/manager/v3.ts (2)
12-15: v3 wrapper now generic over PduType — LGTMExtending PersistentEventBase<RoomVersion3To11, Type> aligns with the new API surface.
3-4: Importing PduType for the generic constraint is correctNo additional changes needed here.
packages/room/src/manager/factory.ts (1)
53-76: Explicit room version propagation in the factory looks solid.Thanks for threading the checked
roomVersionthrough to every concretePersistentEventV*ctor—this keeps the wrappers’ hashing/ID paths consistent with the room schema. The dispatch table still covers the supported versions cleanly.packages/room/src/manager/v9.spec.ts (1)
6-41: Test update covers the new constructor contract.Nice to see the spec re-based on the two-arg ctor and the stricter ID typing—it keeps confidence that the hashed event ID remains stable across versions.
packages/room/src/authorizartion-rules/rules.ts (2)
172-198: Batching the membership prev-event lookup is a welcome improvement.Fetching the previous PDUs in one
getEventscall should trim latency on high-latency stores without altering the downstream logic—looks good.
352-356: TypedroomCreateEventrequirement is spot on.Constraining the power-level validator to a concrete
'm.room.create'wrapper gives the helper the invariants it expects when reading defaults.packages/room/src/manager/v11.ts (2)
1-1: Type-only import is correct.Keeps runtime lean and aligns with TS best practices.
5-7: Generic parameterization looks good.Extends the v9 generic correctly and matches the broader version‑aware refactor.
packages/room/src/manager/power-level-event-wrapper.ts (1)
71-74: Tightened type for createEvent is appropriate.Constraining to PersistentEventBase<RoomVersion, 'm.room.create'> improves safety without changing behavior.
packages/room/src/state_resolution/definitions/definitions.ts (4)
24-27: Returning possibly undefined from getStateByMapKey is correct.Matches real map semantics and prevents unsafe assumptions at call sites.
Please confirm all call sites now handle undefined (e.g., guards or non-null asserts removed).
445-445: Store-backed fetch in mainline traversal is fine.Matches the refactor to getEvents().
500-502: Store-backed fetch for mainline position lookup is fine.
579-579: Store-backed fetch for iterative auth checks is fine.Keeps a consistent data access path.
Consider confirming EventStore.getEvents is idempotent and efficiently batches duplicates to avoid N+1 patterns during state resolution.
Summary by CodeRabbit