fix: typing and missing pl handling #213
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughRefactors power-level handling to tolerate missing m.room.power_levels across auth and state-resolution paths, adds tests for those cases, tightens typings in event/power-level wrappers, and makes mainline ordering accept an optional power-level event. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Auth as Auth Rules
participant Store as State Store
participant PLE as PowerLevelEvent
Client->>Auth: checkEventAuthWithState(event)
Auth->>Store: get `m.room.power_levels`
alt power_levels exists and is valid
Store-->>Auth: power_levels event
Auth->>PLE: PowerLevelEvent.fromEvent(existing)
else missing/invalid
Auth->>PLE: PowerLevelEvent.fromDefault()
end
Auth->>Auth: evaluate membership / power permissions
Auth-->>Client: allow / deny (no crash on missing PL)
sequenceDiagram
autonumber
participant SR as StateResolution v2
participant Store as EventStore
participant Main as mainlineOrdering
SR->>Store: get `m.room.power_levels` (may be undefined)
SR->>Main: mainlineOrdering(events, optionalPowerLevel, store)
alt optional PL present and valid
Main->>Store: build mainline from PL ancestry
Main->>Main: reverseTopologicalPowerSort with mainline
else no PL
Main->>Main: reverseTopologicalPowerSort without mainline
end
Main-->>SR: ordered events
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (7)
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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #213 +/- ##
==========================================
+ Coverage 80.98% 81.07% +0.09%
==========================================
Files 63 63
Lines 4706 4719 +13
==========================================
+ Hits 3811 3826 +15
+ Misses 895 893 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/room/src/state_resolution/definitions/algorithm/v2.ts (1)
269-271: Pass a copy intomainlineOrderingto avoid in-place mutation
mainlineOrderingsorts in-place. If any code later relies onsanitizedRemainingEventsorder, pass a copy.Apply this diff:
- const orderedRemainingEvents = await mainlineOrdering( - sanitizedRemainingEvents, - wrappedStore, - powerLevelEvent, - ); + const orderedRemainingEvents = await mainlineOrdering( + [...sanitizedRemainingEvents], + wrappedStore, + powerLevelEvent, + );Also applies to: 286-289
packages/room/src/state_resolution/definitions/algorithm/v2.spec.ts (1)
530-591: Avoid mutatingeventsbefore callingmainlineOrderingYou derive the expected order by sorting
eventsin-place, then pass the same array tomainlineOrdering, which also sorts in-place. Use copies to keep expectations independent of the function under test.Apply this diff:
- const sortedEvents = events - .sort((e1, e2) => { + const sortedEvents = [...events] + .sort((e1, e2) => { if (e1.originServerTs !== e2.originServerTs) { return e1.originServerTs - e2.originServerTs; } return e1.eventId.localeCompare(e2.eventId); }) .map((e) => { return e.eventId; }); - const mainlineSorted = (await mainlineOrdering(events, eventStore)).map((e) => { + const mainlineSorted = (await mainlineOrdering([...events], eventStore)).map((e) => { return e.eventId; });packages/room/src/manager/power-level-event-wrapper.ts (2)
26-31: Type thefromDefaultreturn explicitly and add a presence type‑guardReturning
PowerLevelEvent<undefined>keeps inference precise at call sites. AhasEvent()type‑guard makes it easier to narrow when needed.Apply this diff:
- static fromDefault() { - return new PowerLevelEvent(undefined); - } + static fromDefault(): PowerLevelEvent<undefined> { + return new PowerLevelEvent(undefined); + }Additionally, consider adding:
// helper for narrowing hasEvent(): this is PowerLevelEvent<PersistentEventBase<RoomVersion, 'm.room.power_levels'>> { return !!this.event; } // optional non-throwing accessor getSenderOrUndefined(): string | undefined { return this.event?.sender; }
187-193: Throwingsendergetter is OK; offer a non-throwing alternativeFor ergonomics, prefer
getSenderOrUndefined()where absence is valid, reserving the throwing getter for strict paths.
📜 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 (7)
packages/room/src/authorizartion-rules/rules.spec.ts(1 hunks)packages/room/src/authorizartion-rules/rules.ts(4 hunks)packages/room/src/manager/event-wrapper.ts(2 hunks)packages/room/src/manager/power-level-event-wrapper.ts(1 hunks)packages/room/src/state_resolution/definitions/algorithm/v2.spec.ts(2 hunks)packages/room/src/state_resolution/definitions/algorithm/v2.ts(2 hunks)packages/room/src/state_resolution/definitions/definitions.ts(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
packages/room/src/state_resolution/definitions/algorithm/v2.ts (1)
packages/room/src/state_resolution/definitions/definitions.ts (1)
getStateMapKey(11-16)
packages/room/src/manager/power-level-event-wrapper.ts (4)
packages/federation-sdk/src/index.ts (1)
PersistentEventBase(21-21)packages/room/src/manager/type.ts (1)
RoomVersion(14-14)packages/room/src/types/v3-11.ts (1)
PduPowerLevelsEventContent(334-336)packages/room/src/manager/event-wrapper.ts (1)
event(105-114)
packages/room/src/authorizartion-rules/rules.ts (1)
packages/room/src/state_resolution/definitions/definitions.ts (1)
getStateMapKey(11-16)
packages/room/src/manager/event-wrapper.ts (1)
packages/room/src/manager/power-level-event-wrapper.ts (1)
PowerLevelEvent(196-196)
packages/room/src/state_resolution/definitions/definitions.ts (2)
packages/federation-sdk/src/index.ts (2)
PersistentEventBase(21-21)RoomVersion(22-22)packages/room/src/manager/type.ts (1)
RoomVersion(14-14)
packages/room/src/authorizartion-rules/rules.spec.ts (1)
packages/room/src/authorizartion-rules/rules.ts (1)
checkEventAuthWithState(702-824)
packages/room/src/state_resolution/definitions/algorithm/v2.spec.ts (1)
packages/room/src/state_resolution/definitions/definitions.ts (1)
mainlineOrdering(415-549)
🔇 Additional comments (13)
packages/room/src/manager/event-wrapper.ts (2)
71-73: Explicit return type cast fortypelooks goodThis makes the getter’s intent clear and aligns with the branded/narrowed usages below.
136-142: Use ofPowerLevelEvent.fromEvent(this)is correctMatches the new constructor pattern and preserves the type guard from
isPowerLevelEvent().packages/room/src/authorizartion-rules/rules.spec.ts (1)
1148-1218: Good coverage for missing power level event pathsThese tests exercise the defaulting behavior for membership and power level updates when PL is absent and align with the new wrapper semantics.
packages/room/src/manager/power-level-event-wrapper.ts (1)
34-36: toEventBase() usages are guarded — no unsafe property access foundSearch shows calls in packages/room/src/authorizartion-rules/rules.ts: lines 527–528 use optional chaining (existingPowerLevel.toEventBase()?.getContent(), newPowerLevel.toEventBase()?.getContent()); line 796 passes powerLevelEvent.toEventBase() into an error object (reason: powerLevelEvent.toEventBase()) and is not dereferenced. No changes required; use hasEvent() only when you need to assert presence before accessing properties.
packages/room/src/authorizartion-rules/rules.ts (3)
158-165: LGTM! Robust null-safe handling for missing power levels.The implementation correctly creates a default PowerLevelEvent when none exists in the auth state, aligning with the Matrix specification requirement that rooms without m.room.power_levels should use default power levels.
748-751: Good defensive check for membership validation.The strengthened membership check ensures both that the event is a valid membership event and that the actual membership value is being compared, preventing potential issues with invalid or missing membership data.
773-780: Consistent null-safe pattern applied.Good consistency with the null-safe pattern established earlier in the file for handling missing power level events.
packages/room/src/state_resolution/definitions/definitions.ts (6)
365-365: LGTM! Correct usage of PowerLevelEvent.fromDefault().Using
PowerLevelEvent.fromDefault()instead ofnew PowerLevelEvent()is the proper way to create default power levels when none exist.
418-420: Good architectural change for optional power levels.Making the
powerLevelEventparameter optional allows for proper handling of rooms without power level events, which can occur during room initialization or in specific edge cases.
457-461: Proper conditional mainline construction.The mainline is now correctly constructed only when a valid power level event exists, preventing potential runtime errors from attempting to process undefined events.
532-537: Good explicit undefined checks.The explicit
!== undefinedchecks are more robust than relying on truthy/falsy evaluation, especially when dealing with numeric values that could be 0.
540-545: Consistent comparison improvements.The explicit inequality check and fallback to locale-based comparison ensure deterministic ordering even when timestamps are equal.
421-431: Incorrect — RoomVersion is used correctly as the generic parameter. PersistentEventBase is declared as PersistentEventBase<T extends RoomVersion = '11', ...>, so PersistentEventBase<RoomVersion, 'm.room.power_levels'> is valid and RoomVersion is imported at the top of the file; if you want a type-only import, change toimport type { RoomVersion } from '../../manager/type';Likely an incorrect or invalid review comment.
ggazzo
force-pushed
the
fix-default-pl
branch
from
20a4f5e to
513d14e
Compare
primarily handles missing pl, but also found other minor bugs in the same area so added to same pr.
Summary by CodeRabbit
Bug Fixes
Refactor
Tests