fix: custom events #294
fix: custom events #294
Conversation
|
Warning Rate limit exceeded@ricardogarim has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 23 minutes and 31 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughfindAuthEvents now handles unknown/custom event types by querying create, power_levels, and membership as defaults; room PDU types updated (removed Changes
Sequence Diagram(s)%%{init: {"themeVariables":{"actorBackground":"#f7fbff","actorBorder":"#cfe3ff","noteBackground":"#fbfbfc"}} }%%
sequenceDiagram
participant Auth as Authorization
participant PL as PowerLevelEvent
participant Repo as EventRepository
participant DB as MongoDB
Auth->>PL: getRequiredPowerLevelForEvent(event)
note right of PL `#e8f5e9`: Inspect event.type and event.isState()\nUse content.events[event.type] or defaults
PL-->>Auth: requiredPower
Auth->>Repo: findAuthEvents(event.type, roomId, sender)
alt m.room.create / m.room.redaction
Repo->>DB: find($or: [create, power_levels, membership, ...specifics])
DB-->>Repo: FindCursor
Repo-->>Auth: FindCursor
else other/unknown event
Repo->>DB: find($or: [create, power_levels, membership])
DB-->>Repo: FindCursor
Repo-->>Auth: FindCursor
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
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 #294 +/- ##
==========================================
+ Coverage 60.46% 60.66% +0.20%
==========================================
Files 67 67
Lines 6672 6686 +14
==========================================
+ Hits 4034 4056 +22
+ Misses 2638 2630 -8 ☔ 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: 2
🧹 Nitpick comments (1)
packages/federation-sdk/src/repositories/event.repository.ts (1)
92-97: Graceful handling of unknown event types implemented correctly.The changes successfully prevent crashes when encountering unknown event types by returning an empty array instead of throwing an error. This aligns with the PR objective to "accept and persist unknown Matrix events... without triggering collateral behavior."
The implementation is sound:
- Type signature correctly reflects the new return possibility (
FindCursor<EventStore> | [])- Empty array is a safe, type-compatible return value
- Calling code can handle [] naturally (empty iteration)
Optional observability improvement:
Consider adding debug-level logging when unknown event types are encountered to aid troubleshooting:
default: - break; + // Log unknown event types for observability + // logger.debug(`Unknown event type in findAuthEvents: ${eventType}`); + break; } if (queries.length === 0) { + // Return empty array for unknown/unsupported event types return []; }This would help operators identify when new event types appear without breaking functionality.
📜 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 (2)
packages/federation-sdk/src/repositories/event.repository.ts(2 hunks)packages/room/src/types/v3-11.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ricardogarim
Repo: RocketChat/homeserver PR: 234
File: packages/federation-sdk/src/repositories/event.repository.ts:407-423
Timestamp: 2025-09-29T16:12:21.350Z
Learning: The Matrix Federation API backfill endpoint should include the anchor events provided in the 'v' parameter along with preceding events, as explicitly stated in the Matrix specification: "including the given event(s)".
📚 Learning: 2025-10-10T22:18:31.655Z
Learnt from: sampaiodiego
Repo: RocketChat/homeserver PR: 224
File: packages/federation-sdk/src/services/event-authorization.service.ts:261-268
Timestamp: 2025-10-10T22:18:31.655Z
Learning: In packages/federation-sdk/src/services/state.service.ts, the method `StateService.getLatestRoomState(roomId: string)` has return type `Promise<State>` and never returns undefined. If the state is not found, it throws an error with message "No state found for room ${roomId}" instead of returning undefined.
Applied to files:
packages/federation-sdk/src/repositories/event.repository.ts
🧬 Code graph analysis (2)
packages/room/src/types/v3-11.ts (1)
packages/room/src/manager/event-wrapper.ts (1)
type(97-99)
packages/federation-sdk/src/repositories/event.repository.ts (1)
packages/core/src/models/event.model.ts (1)
EventStore(26-38)
🔇 Additional comments (1)
packages/federation-sdk/src/repositories/event.repository.ts (1)
45-45: No issues found—call site handles the union return type correctly.The only call site of
findAuthEvents()at line 551 of event.service.ts already properly handles both return types throughfor awaititeration (line 558). This idiom works correctly whether the method returns an empty array or a FindCursor, eliminating the breaking change concern.
ricardogarim
force-pushed
the
fix/save-unknown-events
branch
from
046af4d to
b62c0f0
Compare
ricardogarim
force-pushed
the
fix/save-unknown-events
branch
2 times, most recently
from
32ff0b5 to
0375fc7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
packages/room/src/authorizartion-rules/rules.spec.ts(7 hunks)packages/room/src/manager/power-level-event-wrapper.ts(2 hunks)packages/room/src/types/v3-11.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ricardogarim
Repo: RocketChat/homeserver PR: 184
File: packages/federation-sdk/src/services/event-authorization.service.ts:213-223
Timestamp: 2025-09-14T13:34:12.448Z
Learning: ACL events (m.room.server_acl) are not yet implemented in the homeserver codebase, so ACL-related code currently handles the simpler case where no ACL events exist in room state.
📚 Learning: 2025-10-10T22:18:31.655Z
Learnt from: sampaiodiego
Repo: RocketChat/homeserver PR: 224
File: packages/federation-sdk/src/services/event-authorization.service.ts:261-268
Timestamp: 2025-10-10T22:18:31.655Z
Learning: In packages/federation-sdk/src/services/state.service.ts, the method `StateService.getLatestRoomState(roomId: string)` has return type `Promise<State>` and never returns undefined. If the state is not found, it throws an error with message "No state found for room ${roomId}" instead of returning undefined.
Applied to files:
packages/room/src/authorizartion-rules/rules.spec.tspackages/room/src/types/v3-11.ts
🧬 Code graph analysis (3)
packages/room/src/manager/power-level-event-wrapper.ts (2)
packages/room/src/types/v3-11.ts (1)
isStateEventType(796-814)packages/room/src/manager/event-wrapper.ts (1)
type(97-99)
packages/room/src/authorizartion-rules/rules.spec.ts (1)
packages/room/src/authorizartion-rules/rules.ts (1)
checkEventAuthWithState(776-888)
packages/room/src/types/v3-11.ts (1)
packages/room/src/manager/event-wrapper.ts (1)
type(97-99)
🔇 Additional comments (5)
packages/room/src/manager/power-level-event-wrapper.ts (2)
4-4: LGTM - Import added for new state event detection.The import is correctly added and used in the
getRequiredPowerLevelForEventmethod below.
112-117: Code change is correct and aligns with Matrix specification.Unknown non-state event types require
events_default; unknown state event types requirestate_defaultper the Matrix specification. Your code implements this correctly:
- Line 112-114: State events return
state_default ?? 50✓- Line 116-117: Unknown non-state events return
events_default ?? 0✓The fallback behavior is intentional per the spec, not a security weakness. No changes needed.
packages/room/src/authorizartion-rules/rules.spec.ts (2)
343-398: LGTM - Test correctly validates events_default for unknown events.The test has been properly updated to verify that unknown events (events with type 'test') are checked against
events_defaultrather than a hardcoded power level. The async/await patterns are correctly implemented.Test validates:
- Users below
events_default(alice with power 29 < 30) are rejected- Users at or above
events_default(bob with power 30 >= 30) are allowed
596-710: LGTM - Comprehensive test coverage for unknown event authorization.The test suite has been correctly updated to validate the new power level logic:
Coverage includes:
- Unknown events default to
events_defaultthreshold- Timeline events (messages) also use
events_default- Custom event-specific power levels in
eventsobject override defaults- Both rejection (insufficient power) and acceptance (sufficient power) paths
The async/await patterns are properly implemented throughout.
packages/room/src/types/v3-11.ts (1)
11-39: LGTM - Poll event type removed to treat as unknown.The removal of
'm.poll.start'fromPduTypeSchemais correct and aligns with the PR objective to gracefully handle unknown/unsupported event types. Poll events will now be treated as unknown events and processed through the generic unknown event handling path.This addresses the previous review feedback about architectural consistency.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/room/src/types/v3-11.ts (1)
841-859: LGTM! Helper correctly identifies all state event types.The
isStateEventType()function accurately lists all 15 state event types that correspond to thePduStateEventSchemadiscriminated union members, including the newly addedm.room.avatarandm.room.pinned_events.Optional maintainability improvement: The hardcoded list could drift if schemas are updated. Consider deriving the list from
PduStateEventSchema.optionsto keep it in sync automatically:const STATE_EVENT_TYPES = new Set( PduStateEventSchema.options.map(opt => opt.shape.type.value) ); export function isStateEventType(type: PduType): boolean { return STATE_EVENT_TYPES.has(type); }This is a nice-to-have refactor for maintainability, not a blocker for this PR.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/room/src/types/v3-11.ts(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ricardogarim
Repo: RocketChat/homeserver PR: 184
File: packages/federation-sdk/src/services/event-authorization.service.ts:213-223
Timestamp: 2025-09-14T13:34:12.448Z
Learning: ACL events (m.room.server_acl) are not yet implemented in the homeserver codebase, so ACL-related code currently handles the simpler case where no ACL events exist in room state.
📚 Learning: 2025-10-10T22:18:31.655Z
Learnt from: sampaiodiego
Repo: RocketChat/homeserver PR: 224
File: packages/federation-sdk/src/services/event-authorization.service.ts:261-268
Timestamp: 2025-10-10T22:18:31.655Z
Learning: In packages/federation-sdk/src/services/state.service.ts, the method `StateService.getLatestRoomState(roomId: string)` has return type `Promise<State>` and never returns undefined. If the state is not found, it throws an error with message "No state found for room ${roomId}" instead of returning undefined.
Applied to files:
packages/room/src/types/v3-11.ts
🧬 Code graph analysis (1)
packages/room/src/types/v3-11.ts (1)
packages/room/src/types/_common.ts (1)
eventIdSchema(6-6)
🔇 Additional comments (3)
packages/room/src/types/v3-11.ts (3)
368-395: LGTM! Avatar and pinned events content schemas correctly implement Matrix spec.The schemas are well-structured and match the Matrix specification:
PduRoomAvatarEventContentSchemaincludes the avatar URL, optional metadata (dimensions, MIME type, size), and thumbnail URL.PduRoomPinnedEventsEventContentSchemacorrectly useseventIdSchemafor type-safe event ID validation in the pinned array.This resolves the previous feedback requesting proper validation schemas for these state event types.
769-779: LGTM! Event schemas correctly structured as state events.Both
EventPduTypeRoomAvatarandEventPduTypeRoomPinnedEventsare properly implemented:
- Use
PduNoContentEmptyStateKeyStateEventSchema(correct base for state events withstate_key = "").- Type discriminators match the enum values.
- Content fields reference the appropriate content schemas.
The implementation follows the established pattern for other state event schemas in this file.
808-810: LGTM! Discriminated union updated to include new state event types.Adding
EventPduTypeRoomAvatarandEventPduTypeRoomPinnedEventsto thePduStateEventSchemadiscriminated union resolves the previous inconsistency where these types were classified as state events but lacked validation schemas.Runtime classification via
isStateEventType()and Zod validation are now consistent.
There was a problem hiding this comment.
Actionable comments posted: 1
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/repositories/event.repository.ts (1)
41-45: Incorrect return type annotation.The return type includes
| []but the implementation always returnsthis.collection.find(...)which is aFindCursor<EventStore>. The empty array literal is never returned in the code path.Apply this diff to fix the return type:
findAuthEvents( eventType: string, roomId: string, senderId: string, -): FindCursor<EventStore> | [] { +): FindCursor<EventStore> {
📜 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 (3)
packages/federation-sdk/src/repositories/event.repository.ts(2 hunks)packages/federation-sdk/src/services/state.service.spec.ts(3 hunks)packages/room/src/authorizartion-rules/rules.spec.ts(7 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: ricardogarim
Repo: RocketChat/homeserver PR: 184
File: packages/federation-sdk/src/services/event-authorization.service.ts:213-223
Timestamp: 2025-09-14T13:34:12.448Z
Learning: ACL events (m.room.server_acl) are not yet implemented in the homeserver codebase, so ACL-related code currently handles the simpler case where no ACL events exist in room state.
📚 Learning: 2025-10-10T22:18:31.655Z
Learnt from: sampaiodiego
Repo: RocketChat/homeserver PR: 224
File: packages/federation-sdk/src/services/event-authorization.service.ts:261-268
Timestamp: 2025-10-10T22:18:31.655Z
Learning: In packages/federation-sdk/src/services/state.service.ts, the method `StateService.getLatestRoomState(roomId: string)` has return type `Promise<State>` and never returns undefined. If the state is not found, it throws an error with message "No state found for room ${roomId}" instead of returning undefined.
Applied to files:
packages/federation-sdk/src/repositories/event.repository.tspackages/room/src/authorizartion-rules/rules.spec.ts
📚 Learning: 2025-09-29T16:12:21.350Z
Learnt from: ricardogarim
Repo: RocketChat/homeserver PR: 234
File: packages/federation-sdk/src/repositories/event.repository.ts:407-423
Timestamp: 2025-09-29T16:12:21.350Z
Learning: The Matrix Federation API backfill endpoint should include the anchor events provided in the 'v' parameter along with preceding events, as explicitly stated in the Matrix specification: "including the given event(s)".
Applied to files:
packages/federation-sdk/src/repositories/event.repository.ts
🧬 Code graph analysis (3)
packages/federation-sdk/src/repositories/event.repository.ts (1)
packages/core/src/models/event.model.ts (1)
EventStore(26-38)
packages/federation-sdk/src/services/state.service.spec.ts (4)
packages/federation-sdk/src/services/room.service.ts (2)
createRoom(223-350)joinUser(787-996)packages/core/src/events/m.room.create.ts (1)
roomCreateEvent(26-43)packages/room/src/types/v3-11.ts (1)
PduCreateEventContent(154-154)packages/room/src/types/_common.ts (1)
UserID(20-20)
packages/room/src/authorizartion-rules/rules.spec.ts (1)
packages/room/src/authorizartion-rules/rules.ts (1)
checkEventAuthWithState(776-888)
🔇 Additional comments (5)
packages/room/src/authorizartion-rules/rules.spec.ts (3)
343-398: LGTM! Test correctly updated to validate events_default for unknown events.The test properly validates that unknown/custom events without state_key are governed by
events_defaultrather thanstate_default. The shift to async assertions and variable renaming (randomEvent vs randomStateEvent) improves clarity and correctness.
596-711: LGTM! Comprehensive test coverage for power level enforcement on unknown events.The test thoroughly validates that:
- Users below events_default threshold cannot send unknown events
- Users at or above events_default can send unknown events
- Custom power levels for specific event types are correctly enforced
The async/await pattern is correctly applied throughout.
713-808: Excellent test coverage for custom and unknown Matrix events.This new test case validates the core PR objective by confirming that custom application events (io.rocketchat., com.example.) and unknown Matrix protocol events (m.poll.start) are properly authorized against
events_default. The test also verifies that insufficient power correctly rejects these events.packages/federation-sdk/src/services/state.service.spec.ts (2)
121-121: Good improvement for test configuration flexibility.Allowing MONGO_URI to be read from environment variables makes the test suite more configurable for different environments.
1281-1367: Excellent test validation for unknown event persistence.This test properly validates the core PR objective by confirming that unknown and custom events (io.rocketchat.custom, m.poll.start, com.example.test) can be:
- Built and processed without throwing errors
- Persisted to the database with correct event types
- Retrieved successfully from the repository
The test coverage aligns perfectly with the PR's goal to prevent event ingestion failures from unknown event types.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/room/src/manager/power-level-event-wrapper.ts(2 hunks)packages/room/src/types/v3-11.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-10-10T22:18:31.655Z
Learnt from: sampaiodiego
Repo: RocketChat/homeserver PR: 224
File: packages/federation-sdk/src/services/event-authorization.service.ts:261-268
Timestamp: 2025-10-10T22:18:31.655Z
Learning: In packages/federation-sdk/src/services/state.service.ts, the method `StateService.getLatestRoomState(roomId: string)` has return type `Promise<State>` and never returns undefined. If the state is not found, it throws an error with message "No state found for room ${roomId}" instead of returning undefined.
Applied to files:
packages/room/src/types/v3-11.ts
📚 Learning: 2025-09-14T13:30:07.786Z
Learnt from: ricardogarim
Repo: RocketChat/homeserver PR: 184
File: packages/core/src/utils/fetch.ts:12-47
Timestamp: 2025-09-14T13:30:07.786Z
Learning: In the RocketChat/homeserver codebase, the team prefers to keep implementations simple and functional rather than adding complex optimizations prematurely. Performance improvements are deferred to future work when there's clear evidence they're needed.
Applied to files:
packages/room/src/manager/power-level-event-wrapper.ts
📚 Learning: 2025-10-07T13:24:22.603Z
Learnt from: debdutdeb
Repo: RocketChat/homeserver PR: 252
File: packages/room/src/manager/event-wrapper.ts:468-476
Timestamp: 2025-10-07T13:24:22.603Z
Learning: In the RocketChat homeserver codebase, when calling `encodeCanonicalJson`, it's expected that the function handles all edge cases including undefined values properly. The responsibility for handling such cases lies with `encodeCanonicalJson` itself rather than requiring defensive code in callers.
Applied to files:
packages/room/src/manager/power-level-event-wrapper.ts
📚 Learning: 2025-09-14T13:15:46.588Z
Learnt from: ricardogarim
Repo: RocketChat/homeserver PR: 184
File: packages/federation-sdk/src/services/media.service.ts:21-31
Timestamp: 2025-09-14T13:15:46.588Z
Learning: In the RocketChat homeserver project, ricardogarim prefers to defer timeout and retry enhancements for media downloads when there's already a TODO to make timeouts configurable, indicating they manage technical debt systematically rather than implementing every suggested improvement immediately.
Applied to files:
packages/room/src/manager/power-level-event-wrapper.ts
🧬 Code graph analysis (2)
packages/room/src/types/v3-11.ts (1)
packages/room/src/types/_common.ts (1)
eventIdSchema(6-6)
packages/room/src/manager/power-level-event-wrapper.ts (2)
packages/room/src/manager/event-wrapper.ts (1)
event(129-138)packages/federation-sdk/src/index.ts (1)
PersistentEventBase(25-25)
ggazzo
force-pushed
the
fix/save-unknown-events
branch
from
beebf2a to
c10ed69
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/room/src/manager/power-level-event-wrapper.ts (1)
111-117: Remove unused helper methods.The private methods
isTimelineEventTypeandisStateEventTypeare not invoked anywhere in the code and provide no additional logic beyond wrappingevent.isState(). Since the code directly callsevent.isState()where needed, these helpers add no value.Apply this diff to remove the dead code:
- private isTimelineEventType(event: PersistentEventBase) { - return !event.isState(); - } - - private isStateEventType(event: PersistentEventBase) { - return event.isState(); - } -
📜 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 (2)
packages/room/src/manager/power-level-event-wrapper.ts(2 hunks)packages/room/src/types/v3-11.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-10-10T22:18:31.655Z
Learnt from: sampaiodiego
Repo: RocketChat/homeserver PR: 224
File: packages/federation-sdk/src/services/event-authorization.service.ts:261-268
Timestamp: 2025-10-10T22:18:31.655Z
Learning: In packages/federation-sdk/src/services/state.service.ts, the method `StateService.getLatestRoomState(roomId: string)` has return type `Promise<State>` and never returns undefined. If the state is not found, it throws an error with message "No state found for room ${roomId}" instead of returning undefined.
Applied to files:
packages/room/src/types/v3-11.tspackages/room/src/manager/power-level-event-wrapper.ts
📚 Learning: 2025-09-14T13:30:07.786Z
Learnt from: ricardogarim
Repo: RocketChat/homeserver PR: 184
File: packages/core/src/utils/fetch.ts:12-47
Timestamp: 2025-09-14T13:30:07.786Z
Learning: In the RocketChat/homeserver codebase, the team prefers to keep implementations simple and functional rather than adding complex optimizations prematurely. Performance improvements are deferred to future work when there's clear evidence they're needed.
Applied to files:
packages/room/src/manager/power-level-event-wrapper.ts
📚 Learning: 2025-10-07T13:24:22.603Z
Learnt from: debdutdeb
Repo: RocketChat/homeserver PR: 252
File: packages/room/src/manager/event-wrapper.ts:468-476
Timestamp: 2025-10-07T13:24:22.603Z
Learning: In the RocketChat homeserver codebase, when calling `encodeCanonicalJson`, it's expected that the function handles all edge cases including undefined values properly. The responsibility for handling such cases lies with `encodeCanonicalJson` itself rather than requiring defensive code in callers.
Applied to files:
packages/room/src/manager/power-level-event-wrapper.ts
📚 Learning: 2025-09-14T13:15:46.588Z
Learnt from: ricardogarim
Repo: RocketChat/homeserver PR: 184
File: packages/federation-sdk/src/services/media.service.ts:21-31
Timestamp: 2025-09-14T13:15:46.588Z
Learning: In the RocketChat homeserver project, ricardogarim prefers to defer timeout and retry enhancements for media downloads when there's already a TODO to make timeouts configurable, indicating they manage technical debt systematically rather than implementing every suggested improvement immediately.
Applied to files:
packages/room/src/manager/power-level-event-wrapper.ts
🧬 Code graph analysis (2)
packages/room/src/types/v3-11.ts (2)
packages/federation-sdk/src/index.ts (1)
eventIdSchema(339-339)packages/room/src/types/_common.ts (1)
eventIdSchema(6-6)
packages/room/src/manager/power-level-event-wrapper.ts (1)
packages/room/src/manager/event-wrapper.ts (1)
event(129-138)
🔇 Additional comments (4)
packages/room/src/manager/power-level-event-wrapper.ts (1)
90-109: Defaults are now correct; past issue resolved.The Matrix spec defaults are correctly implemented:
- When
_contentis absent: state events → 50, non-state events → 0- When
_contentexists: specific event type lookup →state_defaultfor state events →events_defaultfor timeline/unknown eventsThis aligns with the PR objective to handle unknown events gracefully by falling back to
events_default(0) for unrecognized timeline events.packages/room/src/types/v3-11.ts (3)
368-395: LGTM! Content schemas correctly implement Matrix spec.The content schemas for
m.room.avatarandm.room.pinned_eventsare well-structured:
- Avatar schema includes all required metadata fields per Matrix spec (url, info with dimensions/mimetype/size, thumbnail_url)
- Pinned events schema uses
eventIdSchemafor type safety- Optional fields provide graceful handling for edge cases, aligning with PR objectives to prevent validation failures
769-779: Excellent! Event schemas properly structured as state events.Both schemas correctly:
- Extend
PduNoContentEmptyStateKeyStateEventSchema(appropriate for state events withstate_key="")- Use literal type discriminators (
'm.room.avatar'and'm.room.pinned_events')- Reference the corresponding content schemas
- Are exported for external usage
This resolves the validation inconsistency flagged in previous reviews where these types were recognized by
isStateEventType()but lacked schemas inPduStateEventSchema.
808-810: Perfect! Discriminated union now complete.The integration of
EventPduTypeRoomAvatarandEventPduTypeRoomPinnedEventsintoPduStateEventSchemacompletes the fix:
- Runtime type classification (via
isStateEventType()) and Zod validation are now consistent- These standard Matrix state events will pass validation instead of causing ingestion failures
- Maintains architectural pattern where every recognized state event type has a corresponding schema
This directly addresses the PR objective of preventing event ingestion failures while properly handling standard Matrix events.
ggazzo
force-pushed
the
fix/save-unknown-events
branch
from
c10ed69 to
35f2839
Compare
ggazzo
force-pushed
the
fix/save-unknown-events
branch
from
35f2839 to
6a4a152
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 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 (6)
packages/federation-sdk/src/repositories/event.repository.ts(1 hunks)packages/federation-sdk/src/services/state.service.spec.ts(8 hunks)packages/room/src/authorizartion-rules/rules.spec.ts(9 hunks)packages/room/src/authorizartion-rules/rules.ts(1 hunks)packages/room/src/manager/power-level-event-wrapper.ts(2 hunks)packages/room/src/types/v3-11.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/room/src/authorizartion-rules/rules.ts
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-09-29T16:12:21.350Z
Learnt from: ricardogarim
Repo: RocketChat/homeserver PR: 234
File: packages/federation-sdk/src/repositories/event.repository.ts:407-423
Timestamp: 2025-09-29T16:12:21.350Z
Learning: The Matrix Federation API backfill endpoint should include the anchor events provided in the 'v' parameter along with preceding events, as explicitly stated in the Matrix specification: "including the given event(s)".
Applied to files:
packages/federation-sdk/src/repositories/event.repository.ts
📚 Learning: 2025-09-14T13:30:07.786Z
Learnt from: ricardogarim
Repo: RocketChat/homeserver PR: 184
File: packages/core/src/utils/fetch.ts:12-47
Timestamp: 2025-09-14T13:30:07.786Z
Learning: In the RocketChat/homeserver codebase, the team prefers to keep implementations simple and functional rather than adding complex optimizations prematurely. Performance improvements are deferred to future work when there's clear evidence they're needed.
Applied to files:
packages/federation-sdk/src/repositories/event.repository.tspackages/room/src/manager/power-level-event-wrapper.ts
📚 Learning: 2025-09-14T13:34:12.448Z
Learnt from: ricardogarim
Repo: RocketChat/homeserver PR: 184
File: packages/federation-sdk/src/services/event-authorization.service.ts:213-223
Timestamp: 2025-09-14T13:34:12.448Z
Learning: ACL events (m.room.server_acl) are not yet implemented in the homeserver codebase, so ACL-related code currently handles the simpler case where no ACL events exist in room state.
Applied to files:
packages/federation-sdk/src/repositories/event.repository.ts
📚 Learning: 2025-09-14T13:15:46.588Z
Learnt from: ricardogarim
Repo: RocketChat/homeserver PR: 184
File: packages/federation-sdk/src/services/media.service.ts:21-31
Timestamp: 2025-09-14T13:15:46.588Z
Learning: In the RocketChat homeserver project, ricardogarim prefers to defer timeout and retry enhancements for media downloads when there's already a TODO to make timeouts configurable, indicating they manage technical debt systematically rather than implementing every suggested improvement immediately.
Applied to files:
packages/federation-sdk/src/repositories/event.repository.tspackages/room/src/manager/power-level-event-wrapper.ts
📚 Learning: 2025-10-07T13:24:22.603Z
Learnt from: debdutdeb
Repo: RocketChat/homeserver PR: 252
File: packages/room/src/manager/event-wrapper.ts:468-476
Timestamp: 2025-10-07T13:24:22.603Z
Learning: In the RocketChat homeserver codebase, when calling `encodeCanonicalJson`, it's expected that the function handles all edge cases including undefined values properly. The responsibility for handling such cases lies with `encodeCanonicalJson` itself rather than requiring defensive code in callers.
Applied to files:
packages/room/src/manager/power-level-event-wrapper.ts
📚 Learning: 2025-10-10T22:18:31.655Z
Learnt from: sampaiodiego
Repo: RocketChat/homeserver PR: 224
File: packages/federation-sdk/src/services/event-authorization.service.ts:261-268
Timestamp: 2025-10-10T22:18:31.655Z
Learning: In packages/federation-sdk/src/services/state.service.ts, the method `StateService.getLatestRoomState(roomId: string)` has return type `Promise<State>` and never returns undefined. If the state is not found, it throws an error with message "No state found for room ${roomId}" instead of returning undefined.
Applied to files:
packages/room/src/authorizartion-rules/rules.spec.tspackages/room/src/types/v3-11.tspackages/federation-sdk/src/services/state.service.spec.ts
🧬 Code graph analysis (3)
packages/room/src/authorizartion-rules/rules.spec.ts (2)
packages/room/src/manager/event-wrapper.ts (1)
roomId(101-103)packages/room/src/authorizartion-rules/rules.ts (1)
checkEventAuthWithState(776-887)
packages/room/src/types/v3-11.ts (1)
packages/room/src/types/_common.ts (1)
eventIdSchema(6-6)
packages/federation-sdk/src/services/state.service.spec.ts (4)
packages/room/src/types/v3-11.ts (2)
PduPowerLevelsEventContent(337-339)PduCreateEventContent(154-154)packages/room/src/types/_common.ts (1)
UserID(20-20)packages/room/src/manager/factory.ts (1)
PersistentEventFactory(30-123)packages/room/src/authorizartion-rules/errors.ts (1)
RejectCodes(4-8)
| expect(() => stateService.handlePdu(messageEvent)).toThrow( | ||
| RejectCodes.AuthError, | ||
| ); |
There was a problem hiding this comment.
Fix async rejection assertion
stateService.handlePdu returns a promise. Wrapping it in a lambda and using toThrow never observes the rejection, so the assertion will fail even when the code rejects. Await the promise and use .rejects so the test exercises the async error path correctly. Please apply this pattern wherever you assert handlePdu rejects in this spec.
- expect(() => stateService.handlePdu(messageEvent)).toThrow(
- RejectCodes.AuthError,
- );
+ await expect(stateService.handlePdu(messageEvent)).rejects.toThrow(
+ RejectCodes.AuthError,
+ );📝 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.
| expect(() => stateService.handlePdu(messageEvent)).toThrow( | |
| RejectCodes.AuthError, | |
| ); | |
| await expect(stateService.handlePdu(messageEvent)).rejects.toThrow( | |
| RejectCodes.AuthError, | |
| ); |
🤖 Prompt for AI Agents
In packages/federation-sdk/src/services/state.service.spec.ts around lines 1304
to 1306, the test incorrectly uses expect(() =>
stateService.handlePdu(messageEvent)).toThrow(...) against an async function;
change the assertion to await the promise and use .rejects (e.g. await
expect(stateService.handlePdu(messageEvent)).rejects.toBe(RejectCodes.AuthError)
or equivalent matcher) so the rejection is observed. Apply this async .rejects
pattern to all other places in this spec where handlePdu rejection is asserted.
| expect( | ||
| checkEventAuthWithState(randomEvent, state, store), | ||
| ).rejects.toThrow(); | ||
|
|
There was a problem hiding this comment.
Await promise rejection assertions
checkEventAuthWithState is async; calling expect(promise).rejects without awaiting (or returning) the promise means the rejection is never asserted and the test can pass spuriously. Await the expectation here and on the other new rejects assertions added in this diff.
- expect(
- checkEventAuthWithState(randomEvent, state, store),
- ).rejects.toThrow();
+ await expect(
+ checkEventAuthWithState(randomEvent, state, store),
+ ).rejects.toThrow();📝 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.
| expect( | |
| checkEventAuthWithState(randomEvent, state, store), | |
| ).rejects.toThrow(); | |
| await expect( | |
| checkEventAuthWithState(randomEvent, state, store), | |
| ).rejects.toThrow(); |
🤖 Prompt for AI Agents
In packages/room/src/authorizartion-rules/rules.spec.ts around lines 398 to 401,
the test calls expect(checkEventAuthWithState(randomEvent, state,
store)).rejects.toThrow() without awaiting or returning the expectation; change
these to await expect(...).rejects.toThrow() (or return the expectation) so the
promise rejection is actually asserted. Update this line and any other new uses
of .rejects in the same diff to use await (or return) the expect call.
6a4a152 to
a958047
Compare
ricardogarim
force-pushed
the
fix/save-unknown-events
branch
2 times, most recently
from
f50e7df to
0aa543b
Compare
ricardogarim
force-pushed
the
fix/save-unknown-events
branch
from
0aa543b to
67db959
Compare
As per FDR-247, this fix ensures that unknown events are accepted and stored in our state without validation and without triggering any event emitters or collateral effects.
The goal is to gracefully handle custom or unsupported protocol events that may be received from federated homeservers, allowing them to be persisted without interrupting event ingestion or synchronization. This includes cases where custom Matrix events (like polls) are sent by remote servers.
Problem context
When an external user on Matrix.org sends a poll in a federated room, message synchronization from Matrix.org to Rocket.Chat breaks for that room. After the poll is sent, new messages from Matrix.org no longer appear in the corresponding Rocket.Chat room.
Steps to reproduce
Expected behavior
Rocket.Chat should handle the poll event gracefully - for example, by displaying an informative placeholder or message indicating unsupported content - and continue synchronizing messages in both directions.
Behavior before current fix
After the poll is sent from Element, subsequent messages from Matrix.org do not reach the Rocket.Chat room anymore, effectively breaking synchronization in that direction.
Additional Notes
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores