chore: move event notification handling in EventService #314
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughReplaces plain Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 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 (beta)
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 |
ggazzo
force-pushed
the
chore/emit-events
branch
2 times, most recently
from
db03e4d to
a0761b7
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #314 +/- ##
=======================================
Coverage 52.80% 52.81%
=======================================
Files 96 96
Lines 12589 12597 +8
=======================================
+ Hits 6648 6653 +5
- Misses 5941 5944 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ggazzo
force-pushed
the
chore/emit-events
branch
from
a0761b7 to
80de435
Compare
ggazzo
changed the title
![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 (2)
packages/federation-sdk/src/queues/staging-area.queue.ts (1)
31-32: Redundant type assertion.Since
queueis typed asRoomID[],shift()already returnsRoomID | undefined. The explicit castas RoomIDis unnecessary and could mask issues if the queue type changes in the future.- const roomId = this.queue.shift() as RoomID; + const roomId = this.queue.shift();packages/federation-sdk/src/services/event.service.ts (1)
980-989: Consider documenting the power level to role mapping.The
getRolehelper only maps100 → 'owner'and50 → 'moderator'; all other levels default to'user'. If this is intentional (e.g., mapping to a specific downstream system), a brief comment would clarify the design choice.
📜 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 (12)
packages/federation-sdk/src/listeners/staging-area.listener.ts(2 hunks)packages/federation-sdk/src/queues/staging-area.queue.ts(2 hunks)packages/federation-sdk/src/repositories/event-staging.repository.ts(2 hunks)packages/federation-sdk/src/repositories/upload.repository.ts(1 hunks)packages/federation-sdk/src/services/edu.service.ts(3 hunks)packages/federation-sdk/src/services/event-authorization.service.ts(4 hunks)packages/federation-sdk/src/services/event.service.ts(6 hunks)packages/federation-sdk/src/services/room.service.ts(1 hunks)packages/federation-sdk/src/services/send-join.service.ts(2 hunks)packages/federation-sdk/src/services/staging-area.service.ts(3 hunks)packages/federation-sdk/src/services/state.service.spec.ts(2 hunks)packages/federation-sdk/src/services/state.service.ts(10 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
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.
📚 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/services/room.service.tspackages/federation-sdk/src/services/send-join.service.tspackages/federation-sdk/src/services/edu.service.tspackages/federation-sdk/src/services/event-authorization.service.tspackages/federation-sdk/src/repositories/event-staging.repository.tspackages/federation-sdk/src/services/event.service.tspackages/federation-sdk/src/services/state.service.tspackages/federation-sdk/src/services/staging-area.service.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/services/event-authorization.service.ts
🧬 Code graph analysis (6)
packages/federation-sdk/src/services/send-join.service.ts (3)
packages/room/src/manager/event-wrapper.ts (1)
roomId(111-113)packages/room/src/types/_common.ts (1)
RoomID(16-16)packages/federation-sdk/src/index.ts (1)
RoomID(33-33)
packages/federation-sdk/src/listeners/staging-area.listener.ts (1)
packages/federation-sdk/src/index.ts (1)
RoomID(33-33)
packages/federation-sdk/src/queues/staging-area.queue.ts (2)
packages/federation-sdk/src/queues/base.queue.ts (1)
QueueHandler(3-3)packages/federation-sdk/src/index.ts (1)
RoomID(33-33)
packages/federation-sdk/src/services/event-authorization.service.ts (2)
packages/room/src/types/_common.ts (2)
RoomID(16-16)EventID(8-8)packages/federation-sdk/src/index.ts (2)
RoomID(33-33)EventID(31-31)
packages/federation-sdk/src/services/event.service.ts (5)
packages/room/src/types/_common.ts (2)
RoomID(16-16)EventID(8-8)packages/federation-sdk/src/index.ts (4)
RoomID(33-33)Pdu(25-25)State(49-49)EventID(31-31)packages/room/src/types/v3-11.ts (1)
Pdu(828-828)packages/room/src/manager/v3.ts (1)
eventId(17-29)packages/room/src/manager/room-state.ts (1)
RoomState(11-179)
packages/federation-sdk/src/services/state.service.ts (5)
packages/room/src/types/_common.ts (1)
RoomID(16-16)packages/federation-sdk/src/index.ts (4)
RoomID(33-33)RoomVersion(30-30)State(49-49)Pdu(25-25)packages/room/src/manager/type.ts (1)
RoomVersion(14-14)packages/federation-sdk/src/specs/federation-api.ts (1)
State(171-171)packages/room/src/types/v3-11.ts (1)
Pdu(828-828)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Agent
🔇 Additional comments (18)
packages/federation-sdk/src/listeners/staging-area.listener.ts (1)
2-2: LGTM!The type change from
stringtoRoomIDis consistent with the broader PR migration and aligns with theStagingAreaQueuechanges.Also applies to: 18-18
packages/federation-sdk/src/services/room.service.ts (1)
946-950: LGTM!Good addition of informational logging before processing the initial state. This improves observability when a join targets a room not yet in the local state.
packages/federation-sdk/src/repositories/upload.repository.ts (1)
1-1: LGTM!Type update for
federation.mridtoRoomIDis consistent with the PR-wide migration to branded types.Also applies to: 9-9
packages/federation-sdk/src/services/send-join.service.ts (1)
4-4: LGTM!The
sendJoinmethod signature update to acceptRoomIDis consistent with the broader type-safety improvements across the federation-sdk.Also applies to: 24-25
packages/federation-sdk/src/repositories/event-staging.repository.ts (1)
71-72: Type assertion is reasonable but relies on data integrity.The cast to
RoomID[]assumes all storedroomIdvalues are valid Matrix room IDs. Since the data originates fromevent.room_id(Line 36), this should hold true. Consider adding runtime validation if this assumption could be violated by corrupted data.packages/federation-sdk/src/services/edu.service.ts (1)
7-7: LGTM!Type updates for
roomId: RoomIDandroomIds: RoomID[]parameters are consistent with the PR-wide migration.Also applies to: 23-24, 53-55
packages/federation-sdk/src/services/event.service.ts (3)
23-26: LGTM!The
RoomStateandStateimports support the newnotify()method, and theMap<RoomID, Pdu[]>type change aligns with the PR-wide RoomID migration.Also applies to: 157-157
767-784: LGTM!The
getStatemethod signature update toRoomIDis consistent with the PR migration. The explicitStatetype annotation at Line 781 is redundant given the immediate assignment but doesn't cause issues.
898-974: LGTM!The event notification switch statement correctly routes events to the appropriate emitter channels. The
switch(true)pattern with case expressions is a valid approach for type-based dispatch.packages/federation-sdk/src/services/staging-area.service.ts (2)
126-130: Consider error handling fornotify()call.If
eventService.notify()throws, the event will not be marked as unstaged and will be reprocessed. Verify this is the intended behavior. If notification failures should not block event processing, consider wrapping in try-catch or handling errors separately.
76-84: LGTM!The RoomID type usage is consistent. The helper function correctly caches room versions using the RoomID as key, which works since branded types are still strings at runtime.
packages/federation-sdk/src/services/event-authorization.service.ts (2)
369-387: LGTM!The generic
canAccessResourcemethod with conditional types provides good type safety while routing to the appropriate typed methods. The casts at lines 375 and 379 are necessary given TypeScript's limitations with narrowing generic types.
355-363: No action required —rcUpload.federation.mridis already correctly typed asRoomIDin theUploadRepository. The type safety is maintained andserverHasAccessToResourcereceives the correct type.packages/federation-sdk/src/services/state.service.spec.ts (1)
152-157: LGTM!The minimal EventService stub is appropriate for tests focused on StateService behavior. The
notifystub returns a resolved promise, preventing tests from failing due to the new dependency.packages/federation-sdk/src/services/state.service.ts (4)
79-80: Dynamic require pattern for circular dependency.Using
require('./event.service').EventServiceinside@inject(delay(...))is a valid approach to handle circular dependencies with tsyringe. However, this pattern can be fragile with bundlers or when the file is renamed.Consider documenting this circular dependency or exploring alternative architectures to break the cycle if this pattern becomes common across multiple services.
1077-1091: Good defensive guard added.The explicit check for empty
stateIdArrayat lines 1080-1082 prevents a potential undefined access that could occur ifstateIdswas unexpectedly empty despite the size check at line 1077.
344-346: LGTM!Casting to
Pduinstead ofanyimproves type safety while maintaining the same runtime behavior.
106-116: LGTM!The method signature correctly uses
RoomIDand throwsUnknownRoomErrordirectly with the typed parameter. Theroom_versionaccess at line 115 is safe because if no create event exists, the method throws at line 112.Based on learnings,
StateService.getLatestRoomStatethrows rather than returning undefined, which aligns with the error-throwing pattern here.
There was a problem hiding this comment.
Pull request overview
This PR refactors event notification handling by moving the processNotificationStage logic from StagingAreaService to a new notify method in EventService. Additionally, it improves type safety across the codebase by replacing generic string types with more specific RoomID types.
- Moved 200+ lines of event notification logic from StagingAreaService to EventService
- Enhanced type safety by using
RoomIDbranded type instead of genericstringthroughout the codebase - Added StateService dependency to EventService and EventService dependency to StateService (creating managed circular dependencies)
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/federation-sdk/src/services/event.service.ts | Added new notify() method containing moved notification logic; added StateService dependency; improved types in processIncomingPDUs and getState |
| packages/federation-sdk/src/services/staging-area.service.ts | Removed processNotificationStage() method (200+ lines); replaced with call to eventService.notify(); improved getRoomVersion logic |
| packages/federation-sdk/src/services/state.service.ts | Added EventService dependency; changed parameter types from string to RoomID; improved type casting; added defensive error check |
| packages/federation-sdk/src/services/state.service.spec.ts | Added mock EventService to test setup |
| packages/federation-sdk/src/services/send-join.service.ts | Changed sendJoin roomId parameter type from string to RoomID |
| packages/federation-sdk/src/services/room.service.ts | Added logging for unknown room scenarios |
| packages/federation-sdk/src/services/event-authorization.service.ts | Improved canAccessResource with generic types; updated method signatures to use RoomID |
| packages/federation-sdk/src/services/edu.service.ts | Changed method signatures to use RoomID type |
| packages/federation-sdk/src/repositories/upload.repository.ts | Changed mrid field type from string to RoomID |
| packages/federation-sdk/src/repositories/event-staging.repository.ts | Changed return type of getDistinctStagedRooms to RoomID[] |
| packages/federation-sdk/src/queues/staging-area.queue.ts | Changed queue and handler types from string to RoomID |
| packages/federation-sdk/src/listeners/staging-area.listener.ts | Updated handleQueueItem parameter type to RoomID |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @inject(delay(() => require('./event.service').EventService)) | ||
| private readonly eventService: EventService, |
There was a problem hiding this comment.
The eventService dependency is injected but never used in StateService. Consider removing this unused dependency to avoid unnecessary circular dependency complexity and reduce coupling.
…tagingAreaService to utilize it
sampaiodiego
force-pushed
the
chore/emit-events
branch
from
80de435 to
319b3e0
Compare
…ositories for improved type safety
sampaiodiego
force-pushed
the
chore/emit-events
branch
from
319b3e0 to
863af4b
Compare
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/federation-sdk/src/services/event.service.ts (1)
975-1098: Users removed from power levels are not notified of role resetIn the
m.room.power_levelscase, the logic only considers:
- Users present in the new
usersmap but not in the old (new \ old) viasetOrUnsetPowerLevels.- Users present in both with changed values.
Users that were present in
oldPowerLevelsbut removed fromchangedUserPowers(i.e.,old \ new) never get ahomeserver.matrix.room.roleevent resetting them back to the default'user'role. Their effective power level changes, but no notification is emitted.You can fix this by computing and iterating the removed set as well, emitting a reset role event:
const usersInOldPowerLevelEvent = Object.keys(oldPowerLevels); const usersInNewPowerLevelEvent = Object.keys(changedUserPowers); const setOrUnsetPowerLevels = new Set( usersInNewPowerLevelEvent, ).difference(new Set(usersInOldPowerLevelEvent)); + + // Users removed from explicit power levels (in old but not in new) + const removedFromPowerLevels = new Set( + usersInOldPowerLevelEvent, + ).difference(new Set(usersInNewPowerLevelEvent)); + + for (const userId of removedFromPowerLevels) { + this.logger.debug( + `Emitting role reset for ${userId} removed from power levels`, + ); + this.eventEmitterService.emit( + 'homeserver.matrix.room.role', + { + sender_id: event.event.sender, + user_id: userId, + room_id: roomId, + role: 'user', + }, + ); + }This ensures all users whose effective role changes (including removals) get a corresponding role event.
🧹 Nitpick comments (5)
packages/federation-sdk/src/repositories/event-staging.repository.ts (3)
3-3: ImportRoomIDas a type-only symbol
RoomIDis only used in type positions here; you can mark it as a type-only import to avoid unnecessary runtime import from@rocket.chat/federation-room.-import { type EventID, Pdu, RoomID } from '@rocket.chat/federation-room'; +import { type EventID, Pdu, type RoomID } from '@rocket.chat/federation-room';
51-54: AlignroomIdparameter type withRoomID
getLeastDepthEventForRoomstill acceptsroomId: string, while the rest of the PR is moving toRoomID. For consistency and stronger typing, consider updating the signature:- getLeastDepthEventForRoom(roomId: string): Promise<EventStagingStore | null> { + getLeastDepthEventForRoom(roomId: RoomID): Promise<EventStagingStore | null> {This helps keep the repository API aligned with the domain-specific
RoomIDtype.
71-72: Avoid unchecked cast fromdistinct()result toRoomID[]
collection.distinct('roomId')is being asserted directly toPromise<RoomID[]>, which is an unchecked cast and can hide mismatches between stored values and theRoomIDcontract.Consider either:
- Keeping the DB result as strings and letting callers wrap/validate, or
- Explicitly mapping and asserting/validating per element:
async getDistinctStagedRooms(): Promise<RoomID[]> { const roomIds = await this.collection.distinct<string>('roomId'); return roomIds.map((id) => id as RoomID); // or, if available: // return roomIds.map((id) => roomIdSchema.parse(id)); }This makes the assumption about stored data more explicit and avoids a blanket cast on the whole promise type.
packages/federation-sdk/src/services/event-authorization.service.ts (1)
285-289: Dead code:getLatestRoomStatethrows on missing state, never returns undefined.Based on learnings,
StateService.getLatestRoomState()throws an error if state is not found rather than returning undefined. Thisif (!state)check is unreachable.Consider either:
- Removing the dead check, or
- Wrapping the call in try-catch if you want to return
falseon missing state instead of propagating the errorpackages/federation-sdk/src/services/state.service.ts (1)
29-82: InjectedEventServiceis unused and introduces extra circular coupling
StateServicenow injectsEventServicevia:import type { EventService } from './event.service'; @inject(delay(() => require('./event.service').EventService)) private readonly eventService: EventService,but
this.eventServiceisn’t referenced anywhere in this class. This adds an unnecessary circular dependency betweenStateServiceandEventService.Unless you’re about to use
eventServicehere, consider removing this dependency:-import type { EventService } from './event.service'; @@ - @inject(delay(() => require('./event.service').EventService)) - private readonly eventService: EventService,This keeps the dependency graph simpler and avoids potential DI/container surprises.
📜 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 (12)
packages/federation-sdk/src/listeners/staging-area.listener.ts(2 hunks)packages/federation-sdk/src/queues/staging-area.queue.ts(2 hunks)packages/federation-sdk/src/repositories/event-staging.repository.ts(2 hunks)packages/federation-sdk/src/repositories/upload.repository.ts(1 hunks)packages/federation-sdk/src/services/edu.service.ts(3 hunks)packages/federation-sdk/src/services/event-authorization.service.ts(4 hunks)packages/federation-sdk/src/services/event.service.ts(6 hunks)packages/federation-sdk/src/services/room.service.ts(1 hunks)packages/federation-sdk/src/services/send-join.service.ts(2 hunks)packages/federation-sdk/src/services/staging-area.service.ts(3 hunks)packages/federation-sdk/src/services/state.service.spec.ts(2 hunks)packages/federation-sdk/src/services/state.service.ts(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/federation-sdk/src/services/state.service.spec.ts
- packages/federation-sdk/src/services/room.service.ts
- packages/federation-sdk/src/queues/staging-area.queue.ts
- packages/federation-sdk/src/listeners/staging-area.listener.ts
- packages/federation-sdk/src/services/edu.service.ts
- packages/federation-sdk/src/services/send-join.service.ts
- packages/federation-sdk/src/services/staging-area.service.ts
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
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.
📚 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/services/event.service.tspackages/federation-sdk/src/repositories/event-staging.repository.tspackages/federation-sdk/src/services/state.service.tspackages/federation-sdk/src/services/event-authorization.service.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/services/event-authorization.service.ts
🧬 Code graph analysis (3)
packages/federation-sdk/src/repositories/event-staging.repository.ts (2)
packages/room/src/types/_common.ts (1)
RoomID(16-16)packages/federation-sdk/src/index.ts (1)
RoomID(33-33)
packages/federation-sdk/src/services/state.service.ts (6)
packages/room/src/manager/event-wrapper.ts (3)
roomId(111-113)State(47-55)event(147-156)packages/room/src/types/_common.ts (1)
RoomID(16-16)packages/federation-sdk/src/index.ts (4)
RoomID(33-33)RoomVersion(30-30)State(49-49)Pdu(25-25)packages/room/src/manager/type.ts (1)
RoomVersion(14-14)packages/federation-sdk/src/specs/federation-api.ts (1)
State(171-171)packages/room/src/types/v3-11.ts (1)
Pdu(828-828)
packages/federation-sdk/src/services/event-authorization.service.ts (2)
packages/room/src/types/_common.ts (2)
RoomID(16-16)EventID(8-8)packages/federation-sdk/src/index.ts (2)
RoomID(33-33)EventID(31-31)
🔇 Additional comments (14)
packages/federation-sdk/src/services/event-authorization.service.ts (5)
7-12: LGTM!The
RoomIDimport is correctly added to support the branded type usage in the updated method signatures.
266-283: LGTM!The migration from
stringtoRoomIDbranded type forcheckAclForInviteandserverHasAccessToResourceparameters improves type safety and aligns with the codebase-wideRoomIDadoption.
369-379: LGTM!The generic conditional type design for
canAccessResourceprovides excellent type safety, ensuring the correct ID type is enforced based on the entity type at compile time.
355-363:rcUpload.federation.mridis correctly typed asRoomID.The Upload type definition confirms that
federation.mridis typed asRoomID(packages/federation-sdk/src/repositories/upload.repository.ts, line 9), matching the parameter type expected byserverHasAccessToResource. The upload repository/entity has been properly updated.
345-353: Theroom_idproperty is already correctly typed asRoomID.The
event.event.room_idreturned byeventService.getEventById()is properly typed asRoomIDthrough theEventStorestructure from@rocket.chat/federation-core. The code at line 352 passes it directly toserverHasAccessToResource(), which expects aRoomIDparameter, and TypeScript validates this type safety automatically. No changes are required.packages/federation-sdk/src/repositories/upload.repository.ts (1)
1-13: RoomID typing forUpload.federation.mridlooks correctUsing
RoomIDforfederation.mridkeeps the persisted shape as a string while tightening types across the federation SDK. No issues spotted.packages/federation-sdk/src/services/event.service.ts (3)
51-64: Lazy injection ofStateServiceis appropriateUsing
@inject(delay(() => StateService))forstateServiceis consistent with the other delayed injections and is a good way to avoid DI timing issues. No changes needed here.
155-167: Room-scoped PDU grouping keyed byRoomIDis consistentSwitching
eventsByRoomIdtoMap<RoomID, Pdu[]>aligns this flow with the brandedRoomIDtype used elsewhere, without changing runtime behavior.
767-843:getState(roomId: RoomID, eventId: EventID)implementation looks soundThe method now:
- Verifies the event belongs to the requested
roomId.- Uses
StateService.getStateBeforeEventand the strongly-typedStateto derive PDUs and auth chain.- Logs and rethrows on failure.
This matches the intended federation
/statesemantics and the new RoomID-based typing.packages/federation-sdk/src/services/state.service.ts (5)
106-116:getRoomVersion(roomId: RoomID)refactor is consistentUsing
RoomIDfor the parameter and throwingUnknownRoomErrorwhen the create event is missing matches the intended contract and uses theroom_versionfrom the stored create event directly. Behavior is clear and type-safe.
171-197: RoomID-aware state accessors preserve previous semantics
getLatestRoomState,getLatestRoomState2, andgetStrippedRoomStatenow takeRoomIDbut still:
- Resolve state via
_mergeDivergentBranches.- Throw if no state is found (
No state found for room ${roomId}), rather than returning undefined.This matches the documented behavior for
getLatestRoomStatefrom earlier work and simply tightens the types. Based on learnings, this looks correct.
334-356: Type cast insignEventimproves correctness of typingsPassing
event.redactedEvent as PduintosignEventmatches the expected shape and keeps the runtime behavior identical (signing the redacted form). This is a safe typing improvement.
787-817: RoomID-typed server discovery helpers are aligned with the rest of the APIUpdating
getServerSetInRoomandgetServersInRoomto acceptRoomIDis a type-only change; the internal logic (iterating membership events and extracting domains) is unchanged and consistent with existing usage.
1063-1103: Defensive guard in_mergeDivergentBranchesis acceptableThe added check:
const stateIdArray = stateIds.values().toArray(); if (stateIdArray.length === 0) { throw new Error('StateService: expected state ID but found none'); }inside the
stateIds.size === 1branch is a defensive assertion against an unexpected empty set. It doesn’t alter the normal path (single stateId) and yields a clearer error in edge cases. No change needed.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/federation-sdk/src/services/event.service.ts (1)
1047-1073: Missing handling for users removed from power levels.The diff logic only handles users newly added to
power_levels.users(users in NEW but not in OLD). Users who were in the old power levels but removed in the new event are not notified of their role change to'user'.Apply this diff to handle removed users:
const setOrUnsetPowerLevels = new Set( usersInNewPowerLevelEvent, ).difference(new Set(usersInOldPowerLevelEvent)); + // Users removed from power levels (in old but not in new) + const removedFromPowerLevels = new Set( + usersInOldPowerLevelEvent, + ).difference(new Set(usersInNewPowerLevelEvent)); + + for (const userId of removedFromPowerLevels) { + this.logger.debug( + `User ${userId} removed from power levels, resetting to user`, + ); + this.eventEmitterService.emit('homeserver.matrix.room.role', { + sender_id: event.event.sender, + user_id: userId, + room_id: roomId, + role: 'user', + }); + } + this.logger.debug(
🧹 Nitpick comments (5)
packages/federation-sdk/src/repositories/upload.repository.ts (1)
5-13: BrandedRoomIDforfederation.mridimproves type safety; consider aligningridSwitching
federation.mridtoRoomIDtightens typing without affecting persistence, which is good. Ifridis also a Matrix/federation room identifier of the same shape, consider migrating it toRoomIDas well for consistency; otherwise, maybe clarify the distinction via naming or a separate branded type.packages/federation-sdk/src/services/event.service.ts (3)
939-944: Consider usingisRedactedEventfor consistency with original logic.The original
processNotificationStageinStagingAreaServiceusedisRedactedEvent(event.event)which checks bothtype === 'm.room.redaction'and'redacts' in event. This implementation only checks the type.+import { isRedactedEvent } from '@rocket.chat/federation-core'; ... - case event.event.type === 'm.room.redaction': { + case isRedactedEvent(event.event): {
1002-1002: Add type cast for power levels content.The
event.event.content.usersaccess lacks a type cast. Sinceevent.eventis typed asPduwith generic content, TypeScript won't properly typeusers. Consider usingPduPowerLevelsEventContentwhich is already imported elsewhere in this file context.- const changedUserPowers = event.event.content.users; + const changedUserPowers = (event.event.content as PduPowerLevelsEventContent).users;You'll need to add the import:
import { PduPowerLevelsEventContent } from '@rocket.chat/federation-room';
897-1107: Large method with high cyclomatic complexity.The
notify()method is ~210 lines with a large switch statement and nested conditionals for power levels handling. Consider extracting the power levels logic into a separate private method to improve readability and testability.private async handlePowerLevelsNotification(event: { eventId: EventID; event: Pdu }, roomId: RoomID): Promise<void> { // Move lines 974-1099 here }packages/federation-sdk/src/services/event-authorization.service.ts (1)
285-289: Unreachable code:getLatestRoomStatethrows on missing state.Based on learnings,
StateService.getLatestRoomState(roomId)throws an error with message "No state found for room ${roomId}" if the state is not found - it never returnsundefined. This null check is dead code.async serverHasAccessToResource( roomId: RoomID, serverName: string, ): Promise<boolean> { - const state = await this.stateService.getLatestRoomState(roomId); - if (!state) { - this.logger.debug(`Room ${roomId} not found`); - return false; - } + let state: State; + try { + state = await this.stateService.getLatestRoomState(roomId); + } catch (error) { + this.logger.debug(`Room ${roomId} not found`); + return false; + }Or if the intent is to propagate the error, simply remove the null check:
async serverHasAccessToResource( roomId: RoomID, serverName: string, ): Promise<boolean> { const state = await this.stateService.getLatestRoomState(roomId); - if (!state) { - this.logger.debug(`Room ${roomId} not found`); - return false; - }
📜 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 (11)
packages/federation-sdk/src/listeners/staging-area.listener.ts(2 hunks)packages/federation-sdk/src/queues/staging-area.queue.ts(2 hunks)packages/federation-sdk/src/repositories/event-staging.repository.ts(2 hunks)packages/federation-sdk/src/repositories/upload.repository.ts(1 hunks)packages/federation-sdk/src/services/edu.service.ts(3 hunks)packages/federation-sdk/src/services/event-authorization.service.ts(4 hunks)packages/federation-sdk/src/services/event.service.ts(5 hunks)packages/federation-sdk/src/services/room.service.ts(1 hunks)packages/federation-sdk/src/services/send-join.service.ts(2 hunks)packages/federation-sdk/src/services/staging-area.service.ts(3 hunks)packages/federation-sdk/src/services/state.service.ts(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/federation-sdk/src/services/room.service.ts
- packages/federation-sdk/src/listeners/staging-area.listener.ts
- packages/federation-sdk/src/queues/staging-area.queue.ts
- packages/federation-sdk/src/services/edu.service.ts
- packages/federation-sdk/src/repositories/event-staging.repository.ts
- packages/federation-sdk/src/services/send-join.service.ts
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
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.
📚 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/services/event-authorization.service.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/federation-sdk/src/services/event-authorization.service.tspackages/federation-sdk/src/services/event.service.tspackages/federation-sdk/src/services/staging-area.service.tspackages/federation-sdk/src/services/state.service.ts
🧬 Code graph analysis (2)
packages/federation-sdk/src/services/staging-area.service.ts (6)
packages/room/src/manager/event-wrapper.ts (2)
roomId(111-113)event(147-156)packages/room/src/types/_common.ts (1)
RoomID(16-16)packages/federation-sdk/src/index.ts (2)
RoomID(33-33)RoomVersion(30-30)packages/federation-sdk/src/services/event.service.ts (1)
getRoomVersion(466-471)packages/federation-sdk/src/repositories/room.repository.ts (1)
getRoomVersion(54-60)packages/room/src/manager/room-state.ts (1)
version(142-151)
packages/federation-sdk/src/services/state.service.ts (5)
packages/room/src/types/_common.ts (1)
RoomID(16-16)packages/federation-sdk/src/index.ts (4)
RoomID(33-33)RoomVersion(30-30)State(49-49)Pdu(25-25)packages/room/src/manager/type.ts (1)
RoomVersion(14-14)packages/federation-sdk/src/specs/federation-api.ts (1)
State(171-171)packages/room/src/types/v3-11.ts (1)
Pdu(828-828)
🔇 Additional comments (15)
packages/federation-sdk/src/repositories/upload.repository.ts (1)
1-1: Import ofRoomIDlooks correctThe new
RoomIDimport integrates cleanly with this file and matches the usage in theUploadtype below.packages/federation-sdk/src/services/staging-area.service.ts (3)
18-18: LGTM - Import aligned with RoomID migration.The addition of
RoomIDtype import is consistent with the PR-wide migration from plainstringto the brandedRoomIDtype.
76-84: Type signature updated correctly for RoomID.The
processEventForRoommethod now correctly acceptsRoomID, and the internalgetRoomVersionhelper properly propagates this type. The cache map keyed bystringis acceptable sinceRoomIDis a branded string type.
126-129: Notification logic consolidated into EventService.The call to
eventService.notify()replaces the previously removedprocessNotificationStagemethod, centralizing event notification handling. This aligns with the PR objective of moving event notification handling intoEventService.packages/federation-sdk/src/services/event.service.ts (3)
23-25: LGTM - Added required type imports.The
RoomStateandStateimports are necessary for the newnotify()method's power levels handling logic.
156-156: Map key type updated to RoomID.Aligns with the PR-wide migration to use branded
RoomIDtype for room identifiers.
767-767: Type signature and variable updated for RoomID consistency.The
getStatemethod now correctly usesRoomIDfor theroomIdparameter andStatefor the internal state variable, improving type safety.Also applies to: 780-780
packages/federation-sdk/src/services/event-authorization.service.ts (3)
11-11: LGTM - Import added for RoomID type.Consistent with the PR-wide RoomID migration.
266-266: Method signatures updated to use RoomID.The
checkAclForInvite,serverHasAccessToResource, andcanAccessRoommethods now correctly use the brandedRoomIDtype instead of plainstring.Also applies to: 282-282, 365-365
369-379: Well-designed generic type constraint for entity IDs.The conditional type
T extends 'event' ? EventID : T extends 'room' ? RoomID : stringprovides compile-time type safety for theentityIdparameter based on theentityType.packages/federation-sdk/src/services/state.service.ts (5)
104-113: LGTM - Type signature and return updated for RoomID.The method now properly uses
RoomIDand the return statement accessesroom_versiondirectly. This is safe because ifcreateEventis found (line 109 check passed), the event content will haveroom_versionas per them.room.createevent schema.
169-169: Method signatures consistently updated to use RoomID.All public methods that accept room identifiers now use the branded
RoomIDtype, providing better type safety across the codebase.Also applies to: 180-180, 185-185, 785-785, 813-813
342-343: Type cast improved fromanytoPdu.Casting
event.redactedEventtoPduinstead ofanyimproves type safety for thesignEventfunction call.
1061-1061: LGTM - Method signature updated for RoomID.Consistent with other method updates in this file.
1077-1079: Simplified array access is correct.The explicit array conversion and indexed access at
stateIdArray[0]is clearer than using.pop()from the iterator. This correctly handles the single-state case.
| import { EventRepository } from '../repositories/event.repository'; | ||
| import { StateGraphRepository } from '../repositories/state-graph.repository'; | ||
| import { ConfigService } from './config.service'; | ||
| import type { EventService } from './event.service'; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n 'EventService|eventService' packages/federation-sdk/src/services/state.service.tsRepository: RocketChat/homeserver
Length of output: 120
Remove unused EventService import.
The EventService type is imported at line 29 but has no usages in the file. It is not injected in the constructor nor referenced anywhere else. Remove the dead import.
🤖 Prompt for AI Agents
In packages/federation-sdk/src/services/state.service.ts around line 29, the
type import "EventService" is unused; remove the dead import statement to clean
up the file and avoid lint/type warnings by deleting the line "import type {
EventService } from './event.service';".
Summary by CodeRabbit
New Features
Refactor
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.