chore: remove deprecated State method getServersInRoom#337
Merged
sampaiodiego merged 5 commits intomainfrom Mar 9, 2026
Merged
chore: remove deprecated State method getServersInRoom#337sampaiodiego merged 5 commits intomainfrom
sampaiodiego merged 5 commits intomainfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReplaced array-based server lookups with Set-based APIs: callers now use getServerSetInRoom(roomId, roomState?) which returns a Set; removed getServersInRoom. Presence EDU sending now concurrently aggregates unique non-origin servers via Promise.all before dispatching EDUs. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant EduSvc as EduService
participant StateSvc as StateService
participant Remote as RemoteServerPool
rect rgba(200,200,255,0.5)
Caller->>EduSvc: request presence update for roomIds[]
end
rect rgba(200,255,200,0.5)
EduSvc->>StateSvc: Promise.all(getServerSetInRoom(roomId) for each roomId)
StateSvc-->>EduSvc: Set<string> per room (concurrent)
end
rect rgba(255,200,200,0.5)
EduSvc->>Remote: send EDU to Array.from(aggregateNonOriginServers)
Remote-->>EduSvc: ACK / responses
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/federation-sdk/src/services/edu.service.ts (1)
50-59: Consider bounded concurrency for room fan-out.
Promise.all(roomIds.map(...))can overload state/backend calls whenroomIdsis large. A small batch limit keeps latency benefits with better stability.Suggested bounded-concurrency pattern
- await Promise.all( - roomIds.map(async (roomId) => { - const servers = await this.stateService.getServerSetInRoom(roomId); - for (const server of servers) { - if (server !== origin) { - uniqueServers.add(server); - } - } - }), - ); + const BATCH_SIZE = 50; + for (let i = 0; i < roomIds.length; i += BATCH_SIZE) { + const batch = roomIds.slice(i, i + BATCH_SIZE); + await Promise.all( + batch.map(async (roomId) => { + const servers = await this.stateService.getServerSetInRoom(roomId); + for (const server of servers) { + if (server !== origin) { + uniqueServers.add(server); + } + } + }), + ); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/federation-sdk/src/services/edu.service.ts` around lines 50 - 59, The current Promise.all over roomIds can overwhelm state/backend when roomIds is large; replace the unbounded parallelism in the block that calls this.stateService.getServerSetInRoom(roomId) (inside the map over roomIds) with a bounded-concurrency approach (e.g., use a small batch loop over roomIds or a concurrency limiter like p-limit) so only N calls run at once, still awaiting each batch before moving on, and continue adding results into uniqueServers while skipping origin; update the function containing that logic (referencing roomIds, stateService.getServerSetInRoom, uniqueServers, and origin) to implement this limiter/batching.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/federation-sdk/src/services/edu.service.ts`:
- Around line 50-59: The current Promise.all over roomIds can overwhelm
state/backend when roomIds is large; replace the unbounded parallelism in the
block that calls this.stateService.getServerSetInRoom(roomId) (inside the map
over roomIds) with a bounded-concurrency approach (e.g., use a small batch loop
over roomIds or a concurrency limiter like p-limit) so only N calls run at once,
still awaiting each batch before moving on, and continue adding results into
uniqueServers while skipping origin; update the function containing that logic
(referencing roomIds, stateService.getServerSetInRoom, uniqueServers, and
origin) to implement this limiter/batching.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/federation-sdk/src/services/edu.service.tspackages/federation-sdk/src/services/event-authorization.service.tspackages/federation-sdk/src/services/state.service.ts
💤 Files with no reviewable changes (1)
- packages/federation-sdk/src/services/state.service.ts
📜 Review details
⏰ 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). (2)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Code Quality Checks(lint, test, tsc)
🧰 Additional context used
🧠 Learnings (1)
📚 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/edu.service.ts
🔇 Additional comments (2)
packages/federation-sdk/src/services/edu.service.ts (1)
27-28: Good Set-based server retrieval and origin filtering.This change cleanly removes duplicate targets and keeps self-origin out of EDU dispatch.
packages/federation-sdk/src/services/event-authorization.service.ts (1)
252-253: Set membership check is a solid update.Switching to
getServerSetInRoom(...).has(...)is correct and more efficient than array membership checks.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/federation-sdk/src/services/state.service.spec.ts (1)
1439-1475: Consider consolidating overlapping join/leave server-list tests.These two cases validate nearly the same flow (remote user join → leave → rejoin). Keeping one as the canonical flow and narrowing the other to a distinct edge case would reduce test maintenance.
Also applies to: 2302-2332
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/federation-sdk/src/services/state.service.spec.ts` around lines 1439 - 1475, The two tests that exercise the same join→leave→rejoin flow should be consolidated: keep one full canonical test (the existing "should list correct servers for a room" that uses joinUser, leaveUser and stateService.getServerSetInRoom) and remove or narrow the duplicate elsewhere (the similar block at lines 2302-2332) so it asserts only a distinct edge-case; update the remaining test to cover the full join/leave/rejoin sequence with bob, diego and remoteUser, and simplify the duplicated test to validate a single behavior (e.g., only join or only leave) to avoid overlapping coverage and reduce maintenance. Ensure references to joinUser, leaveUser, and getServerSetInRoom remain in the canonical test so behavior is fully validated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/federation-sdk/src/services/state.service.spec.ts`:
- Around line 2292-2299: The test name "should return empty set for room with
only creator" contradicts its assertions; update the test description in the
it(...) call to reflect that the server set contains the creator's server (e.g.,
"should return set containing only the room creator's server") so it matches the
behavior exercised by createRoom and stateService.getServerSetInRoom and the
expect checks for 'example.com' and size === 1.
---
Nitpick comments:
In `@packages/federation-sdk/src/services/state.service.spec.ts`:
- Around line 1439-1475: The two tests that exercise the same join→leave→rejoin
flow should be consolidated: keep one full canonical test (the existing "should
list correct servers for a room" that uses joinUser, leaveUser and
stateService.getServerSetInRoom) and remove or narrow the duplicate elsewhere
(the similar block at lines 2302-2332) so it asserts only a distinct edge-case;
update the remaining test to cover the full join/leave/rejoin sequence with bob,
diego and remoteUser, and simplify the duplicated test to validate a single
behavior (e.g., only join or only leave) to avoid overlapping coverage and
reduce maintenance. Ensure references to joinUser, leaveUser, and
getServerSetInRoom remain in the canonical test so behavior is fully validated.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/federation-sdk/src/services/state.service.spec.ts
📜 Review details
⏰ 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: cubic · AI code reviewer
🧰 Additional context used
🧠 Learnings (1)
📚 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/state.service.spec.ts
🧬 Code graph analysis (1)
packages/federation-sdk/src/services/state.service.spec.ts (2)
packages/core/src/events/m.room.create.ts (1)
roomCreateEvent(26-39)packages/room/src/manager/room-state.ts (1)
creator(12-22)
🔇 Additional comments (1)
packages/federation-sdk/src/services/state.service.spec.ts (1)
1449-1451: Set-based migration assertions look correct.The switch to
getServerSetInRoom(...)andSet.has/Set.sizechecks is consistent with the new API and preserves the expected join/leave behavior checks.Also applies to: 1456-1459, 1464-1466, 1471-1474
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/federation-sdk/src/services/state.service.spec.ts">
<violation number="1" location="packages/federation-sdk/src/services/state.service.spec.ts:2292">
P3: Test name contradicts assertions: it claims an empty set but expects one server (`example.com`). Rename to match actual behavior.</violation>
<violation number="2" location="packages/federation-sdk/src/services/state.service.spec.ts:2302">
P3: New test case substantially duplicates existing join/leave/rejoin server-set coverage in the same spec, increasing maintenance cost.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
Pull request overview
Removes the deprecated array-based StateService.getServersInRoom API and migrates call sites/tests to the set-based getServerSetInRoom interface for server membership checks.
Changes:
- Removed deprecated
getServersInRoomfromStateService. - Updated services to use
Setsemantics (has,size) instead of array operations. - Expanded/updated tests to validate set semantics and membership filtering.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/federation-sdk/src/services/state.service.ts | Deletes deprecated convenience wrapper in favor of getServerSetInRoom. |
| packages/federation-sdk/src/services/state.service.spec.ts | Updates existing assertions to set semantics and adds a focused getServerSetInRoom test suite. |
| packages/federation-sdk/src/services/event-authorization.service.ts | Switches room membership check from includes to Set.has. |
| packages/federation-sdk/src/services/edu.service.ts | Uses getServerSetInRoom; presence updates now gather servers concurrently across rooms. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/federation-sdk/src/services/event-authorization.service.ts
Outdated
Show resolved
Hide resolved
Comment on lines
+50
to
+59
| await Promise.all( | ||
| roomIds.map(async (roomId) => { | ||
| const servers = await this.stateService.getServerSetInRoom(roomId); | ||
| for (const server of servers) { | ||
| if (server !== origin) { | ||
| uniqueServers.add(server); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| }), | ||
| ); |
There was a problem hiding this comment.
Promise.all(roomIds.map(...)) fans out one getServerSetInRoom call per room with no concurrency limit. If roomIds can be large, this can overwhelm the DB/state store and increase tail latency. Consider batching / limiting concurrency (e.g., chunking roomIds, a simple semaphore, or a library like p-limit).
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #337 +/- ##
==========================================
- Coverage 50.78% 50.77% -0.01%
==========================================
Files 97 97
Lines 10949 10946 -3
==========================================
- Hits 5560 5558 -2
+ Misses 5389 5388 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/federation-sdk/src/services/state.service.spec.ts (1)
2302-2332: Collapse this into the existing room-server transition test.This scenario is effectively the same flow already covered by
should list correct servers for a roomabove: initial creator-only set, remote join, remote leave, remote rejoin. Keeping both adds DB-heavy test time and duplicate maintenance without covering a new edge.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/federation-sdk/src/services/state.service.spec.ts` around lines 2302 - 2332, Remove the duplicate test "should update server list when users join and leave" and consolidate its assertions into the existing "should list correct servers for a room" test: reuse the same helpers (createRoom, joinUser, leaveUser) and stateService.getServerSetInRoom calls to add the remote join, remote leave, and remote rejoin assertions (checking has('remote.com') and size) at the appropriate points in that existing test so the full flow (creator-only → join → leave → rejoin) is covered in one test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/federation-sdk/src/services/state.service.spec.ts`:
- Around line 2228-2357: Tests only exercise StateService.getServerSetInRoom but
the PR also changed edu.service.ts and event-authorization.service.ts so add
regression tests that exercise the new callers: write unit/integration tests
that trigger edu.service.ts flows (e.g., call the function that processes EDUs
and ensure it uses Array.from(...).origin filtering) and tests for
event-authorization.service.ts that hit the authorization path that uses
Set.has(...) (simulate events from origins present/absent in the set and assert
allow/deny behavior); locate the entrypoints in edu.service (the EDU processing
method) and in event-authorization.service (the method that checks origins with
Set.has) and add test cases that validate the new filtering/authorization logic
across cross-server scenarios to ensure coverage.
---
Nitpick comments:
In `@packages/federation-sdk/src/services/state.service.spec.ts`:
- Around line 2302-2332: Remove the duplicate test "should update server list
when users join and leave" and consolidate its assertions into the existing
"should list correct servers for a room" test: reuse the same helpers
(createRoom, joinUser, leaveUser) and stateService.getServerSetInRoom calls to
add the remote join, remote leave, and remote rejoin assertions (checking
has('remote.com') and size) at the appropriate points in that existing test so
the full flow (creator-only → join → leave → rejoin) is covered in one test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 64e4fa74-1af0-4864-bf15-cef21f3ed4fc
📒 Files selected for processing (1)
packages/federation-sdk/src/services/state.service.spec.ts
📜 Review details
⏰ 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: cubic · AI code reviewer
🧰 Additional context used
🧠 Learnings (2)
📓 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/state.service.spec.ts
Comment on lines
+2228
to
+2357
| describe('getServerSetInRoom', () => { | ||
| it('should return servers with joined members', async () => { | ||
| const { roomCreateEvent } = await createRoom('public'); | ||
|
|
||
| const alice = '@alice:example.com'; | ||
| const bob = '@bob:remote.com'; | ||
| const charlie = '@charlie:another.com'; | ||
|
|
||
| await joinUser(roomCreateEvent.roomId, alice); | ||
| await joinUser(roomCreateEvent.roomId, bob); | ||
| await joinUser(roomCreateEvent.roomId, charlie); | ||
|
|
||
| const servers = await stateService.getServerSetInRoom(roomCreateEvent.roomId); | ||
|
|
||
| expect(servers.has('example.com')).toBe(true); | ||
| expect(servers.has('remote.com')).toBe(true); | ||
| expect(servers.has('another.com')).toBe(true); | ||
| expect(servers.size).toBe(3); | ||
| }); | ||
|
|
||
| it('should deduplicate servers with multiple users from same domain', async () => { | ||
| const { roomCreateEvent } = await createRoom('public'); | ||
|
|
||
| const alice = '@alice:example.com'; | ||
| const bob = '@bob:example.com'; | ||
| const charlie = '@charlie:example.com'; | ||
|
|
||
| await joinUser(roomCreateEvent.roomId, alice); | ||
| await joinUser(roomCreateEvent.roomId, bob); | ||
| await joinUser(roomCreateEvent.roomId, charlie); | ||
|
|
||
| const servers = await stateService.getServerSetInRoom(roomCreateEvent.roomId); | ||
|
|
||
| expect(servers.has('example.com')).toBe(true); | ||
| expect(servers.size).toBe(1); | ||
| }); | ||
|
|
||
| it('should exclude servers with non-joined members', async () => { | ||
| const { roomCreateEvent } = await createRoom('public'); | ||
|
|
||
| const creator = '@alice:example.com'; // Room creator with admin permissions | ||
| const joined = '@joined:joined.com'; | ||
| const left = '@left:left.com'; | ||
| const banned = '@banned:banned.com'; | ||
| const invited = '@invited:invited.com'; | ||
|
|
||
| await joinUser(roomCreateEvent.roomId, joined); | ||
| await joinUser(roomCreateEvent.roomId, left); | ||
| await joinUser(roomCreateEvent.roomId, banned); | ||
|
|
||
| // Change memberships - use creator for moderation actions | ||
| await leaveUser(roomCreateEvent.roomId, left); | ||
| await banUser(roomCreateEvent.roomId, banned, creator); | ||
| await inviteUser(roomCreateEvent.roomId, invited, creator); | ||
|
|
||
| const servers = await stateService.getServerSetInRoom(roomCreateEvent.roomId); | ||
|
|
||
| expect(servers.has('joined.com')).toBe(true); | ||
| expect(servers.has('left.com')).toBe(false); | ||
| expect(servers.has('banned.com')).toBe(false); | ||
| expect(servers.has('invited.com')).toBe(false); | ||
| expect(servers.size).toBe(2); // example.com (creator) + joined.com | ||
| }); | ||
|
|
||
| it('should return creator server for room with only creator', async () => { | ||
| const { roomCreateEvent } = await createRoom('public'); | ||
|
|
||
| const servers = await stateService.getServerSetInRoom(roomCreateEvent.roomId); | ||
|
|
||
| // Only the room creator from createRoom should be present | ||
| expect(servers.has('example.com')).toBe(true); | ||
| expect(servers.size).toBe(1); | ||
| }); | ||
|
|
||
| it('should update server list when users join and leave', async () => { | ||
| const { roomCreateEvent } = await createRoom('public'); | ||
|
|
||
| const remoteUser = '@alice:remote.com'; | ||
|
|
||
| // Initially only creator's server | ||
| const servers1 = await stateService.getServerSetInRoom(roomCreateEvent.roomId); | ||
| expect(servers1.has('example.com')).toBe(true); | ||
| expect(servers1.size).toBe(1); | ||
|
|
||
| // Add remote user | ||
| await joinUser(roomCreateEvent.roomId, remoteUser); | ||
| const servers2 = await stateService.getServerSetInRoom(roomCreateEvent.roomId); | ||
| expect(servers2.has('example.com')).toBe(true); | ||
| expect(servers2.has('remote.com')).toBe(true); | ||
| expect(servers2.size).toBe(2); | ||
|
|
||
| // Remove remote user | ||
| await leaveUser(roomCreateEvent.roomId, remoteUser); | ||
| const servers3 = await stateService.getServerSetInRoom(roomCreateEvent.roomId); | ||
| expect(servers3.has('example.com')).toBe(true); | ||
| expect(servers3.has('remote.com')).toBe(false); | ||
| expect(servers3.size).toBe(1); | ||
|
|
||
| // Add remote user back | ||
| await joinUser(roomCreateEvent.roomId, remoteUser); | ||
| const servers4 = await stateService.getServerSetInRoom(roomCreateEvent.roomId); | ||
| expect(servers4.has('example.com')).toBe(true); | ||
| expect(servers4.has('remote.com')).toBe(true); | ||
| expect(servers4.size).toBe(2); | ||
| }); | ||
|
|
||
| it('should handle multiple servers with different user counts', async () => { | ||
| const { roomCreateEvent } = await createRoom('public'); | ||
|
|
||
| // Server 1: 3 users | ||
| await joinUser(roomCreateEvent.roomId, '@user1:server1.com'); | ||
| await joinUser(roomCreateEvent.roomId, '@user2:server1.com'); | ||
| await joinUser(roomCreateEvent.roomId, '@user3:server1.com'); | ||
|
|
||
| // Server 2: 2 users | ||
| await joinUser(roomCreateEvent.roomId, '@alice:server2.com'); | ||
| await joinUser(roomCreateEvent.roomId, '@bob:server2.com'); | ||
|
|
||
| // Server 3: 1 user | ||
| await joinUser(roomCreateEvent.roomId, '@charlie:server3.com'); | ||
|
|
||
| const servers = await stateService.getServerSetInRoom(roomCreateEvent.roomId); | ||
|
|
||
| expect(servers.has('example.com')).toBe(true); | ||
| expect(servers.has('server1.com')).toBe(true); | ||
| expect(servers.has('server2.com')).toBe(true); | ||
| expect(servers.has('server3.com')).toBe(true); | ||
| expect(servers.size).toBe(4); | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
Add regression coverage for the changed callers, not just StateService.
This suite validates getServerSetInRoom itself, but the PR also changes edu.service.ts and event-authorization.service.ts, and those files are still showing 0% patch coverage in the PR report. A bug in the new Array.from(...)/origin filtering or the Set.has(...) authorization path would still slip through with this test set alone.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/federation-sdk/src/services/state.service.spec.ts` around lines 2228
- 2357, Tests only exercise StateService.getServerSetInRoom but the PR also
changed edu.service.ts and event-authorization.service.ts so add regression
tests that exercise the new callers: write unit/integration tests that trigger
edu.service.ts flows (e.g., call the function that processes EDUs and ensure it
uses Array.from(...).origin filtering) and tests for
event-authorization.service.ts that hit the authorization path that uses
Set.has(...) (simulate events from origins present/absent in the set and assert
allow/deny behavior); locate the entrypoints in edu.service (the EDU processing
method) and in event-authorization.service (the method that checks origins with
Set.has) and add test cases that validate the new filtering/authorization logic
across cross-server scenarios to ensure coverage.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/federation-sdk/src/services/event-authorization.service.ts (1)
239-253: Remove the unreachable!statebranch while touching this path.Reusing the fetched
stateingetServerSetInRoom(roomId, state)is the right optimization. The guard on Line 240 is dead, though:getLatestRoomState(roomId)throws when the room state is missing, so this method never receivesundefined.Based on learnings, `StateService.getLatestRoomState(roomId: string)` returns `Promise` and throws `No state found for room ${roomId}` when the state is missing.♻️ Suggested cleanup
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; - } const aclEvent = state.get('m.room.server_acl:'); const isServerAllowed = await this.checkServerAcl(aclEvent, serverName); if (!isServerAllowed) { this.logger.warn(`Server ${serverName} is denied by room ACL for room ${roomId}`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/federation-sdk/src/services/event-authorization.service.ts` around lines 239 - 253, The null-check branch for state is unreachable because stateService.getLatestRoomState(roomId) throws when missing; remove the conditional that tests if (!state) along with its logger.debug and return false, and keep using the fetched state variable when calling this.stateService.getServerSetInRoom(roomId, state). Ensure no other code assumes state can be undefined in the surrounding flow (methods referenced: stateService.getLatestRoomState, stateService.getServerSetInRoom, variable state, and this.logger.debug).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/federation-sdk/src/services/event-authorization.service.ts`:
- Around line 239-253: The null-check branch for state is unreachable because
stateService.getLatestRoomState(roomId) throws when missing; remove the
conditional that tests if (!state) along with its logger.debug and return false,
and keep using the fetched state variable when calling
this.stateService.getServerSetInRoom(roomId, state). Ensure no other code
assumes state can be undefined in the surrounding flow (methods referenced:
stateService.getLatestRoomState, stateService.getServerSetInRoom, variable
state, and this.logger.debug).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0de7fe31-6b33-49b9-a1d7-37f803204b9e
📒 Files selected for processing (2)
packages/federation-sdk/src/services/event-authorization.service.tspackages/federation-sdk/src/services/state.service.ts
📜 Review details
🧰 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/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
🔇 Additional comments (1)
packages/federation-sdk/src/services/state.service.ts (1)
695-720: Nice API upgrade for the deprecated lookup replacement.Accepting an optional
roomStatehere cleanly removes the redundant fetch and lets callers reuse the same snapshot for membership checks.
ggazzo
previously approved these changes
Mar 9, 2026
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/federation-sdk/src/services/state.service.spec.ts (1)
2228-2325: Consider adding tests for theroomStateparameter path.All tests call
getServerSetInRoom(roomId)without the optionalroomStateargument, leaving the externally-supplied-state code path untested. Per the implementation, whenroomStateis provided, it uses that snapshot directly instead of callinggetLatestRoomState().The
event-authorization.service.tscaller passes an explicit state snapshot (line 252 in the implementation), but this branch has no test coverage.💡 Example test for the `roomState` parameter
it('should use provided roomState instead of fetching latest', async () => { const { roomCreateEvent } = await createRoom('public'); // Join a user to create some state await joinUser(roomCreateEvent.roomId, '@alice:remote.com'); // Get the current state const currentState = await stateService.getLatestRoomState(roomCreateEvent.roomId); // Join another user (this won't be in our snapshot) await joinUser(roomCreateEvent.roomId, '@bob:another.com'); // Call with the earlier state snapshot - should NOT include 'another.com' const servers = await stateService.getServerSetInRoom(roomCreateEvent.roomId, currentState); expect(servers.has('example.com')).toBe(true); expect(servers.has('remote.com')).toBe(true); expect(servers.has('another.com')).toBe(false); // Not in the snapshot expect(servers.size).toBe(2); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/federation-sdk/src/services/state.service.spec.ts` around lines 2228 - 2325, Tests currently never exercise the getServerSetInRoom(roomId, roomState) code path that uses an externally supplied snapshot; add a unit test that takes a snapshot via stateService.getLatestRoomState(roomId), mutates the live room (e.g., join another user), then calls stateService.getServerSetInRoom(roomId, snapshot) and asserts the later join is NOT included. Locate test suite using getServerSetInRoom in state.service.spec.ts and add a case similar to the suggested example that references getLatestRoomState and verifies behavior when roomState is provided.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/federation-sdk/src/services/state.service.spec.ts`:
- Around line 2228-2325: Tests currently never exercise the
getServerSetInRoom(roomId, roomState) code path that uses an externally supplied
snapshot; add a unit test that takes a snapshot via
stateService.getLatestRoomState(roomId), mutates the live room (e.g., join
another user), then calls stateService.getServerSetInRoom(roomId, snapshot) and
asserts the later join is NOT included. Locate test suite using
getServerSetInRoom in state.service.spec.ts and add a case similar to the
suggested example that references getLatestRoomState and verifies behavior when
roomState is provided.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a8976964-d6f6-4b0b-83f7-9f49c52836cb
📒 Files selected for processing (1)
packages/federation-sdk/src/services/state.service.spec.ts
📜 Review details
⏰ 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: cubic · AI code reviewer
🧰 Additional context used
🧠 Learnings (2)
📓 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/state.service.spec.ts
🔇 Additional comments (1)
packages/federation-sdk/src/services/state.service.spec.ts (1)
1449-1474: LGTM!The migration from array-based checks to Set-based checks (
.has(),.size) is correct and properly exercises the join/leave/rejoin scenarios for server membership tracking.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit