feat: add support to update room membership to sdk#381
Merged
Conversation
Contributor
WalkthroughTwo new methods were added to enable room membership updates. A public method in the FederationSDK class delegates to a new RoomService method that constructs, signs, and distributes m.room.member events to change user membership status in rooms. Changes
Sequence DiagramsequenceDiagram
participant Client
participant SDK as FederationSDK
participant RoomSvc as RoomService
participant StateSvc as StateService
participant FedSvc as FederationService
Client->>SDK: updateRoomMembership(roomId, userId, membership)
SDK->>RoomSvc: updateRoomMembership(...)
RoomSvc->>StateSvc: getRoomVersion(roomId)
StateSvc-->>RoomSvc: version
RoomSvc->>RoomSvc: Construct m.room.member event
RoomSvc->>StateSvc: buildEvent(event, version)
StateSvc-->>RoomSvc: signed event
RoomSvc->>StateSvc: handlePdu(event)
StateSvc-->>RoomSvc: membershipEvent
alt Event Rejected
RoomSvc->>RoomSvc: Throw error (rejectReason)
else Event Accepted
RoomSvc->>FedSvc: sendEventToAllServersInRoom(roomId, event)
RoomSvc-->>Client: eventId
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
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.
Actionable comments posted: 2
🤖 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/room.service.ts`:
- Around line 1537-1546: The updateRoomMembership API currently hardcodes the
event sender to the target user, preventing moderator/admin-driven membership
changes; modify the updateRoomMembership function signature to accept an
explicit senderId (e.g., add senderId: UserID param) and use senderId when
building the membership PDU/event instead of setting sender = userId; update the
PDU content/type construction and any related helper calls inside
updateRoomMembership (and the similar membership helper at the other location
referenced) to pass and honor senderId, and adjust callers to provide the
correct sender (defaulting callers to userId only where appropriate).
- Around line 1553-1556: The object merge currently spreads caller-provided
content before adding membership, which allows content.membership to override
the explicit membership argument; update the merge order so the explicit
membership wins by placing membership after the spread (i.e., produce content: {
...content, membership }) and ensure any tests or callers relying on the
previous behavior are adjusted; look for the merge expression in room.service.ts
where content: { membership, ...content } is used and change it so membership is
applied last.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bb840cf9-5be4-4065-ba87-cfe015b50725
📒 Files selected for processing (2)
packages/federation-sdk/src/sdk.tspackages/federation-sdk/src/services/room.service.ts
📜 Review details
🧰 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/sdk.tspackages/federation-sdk/src/services/room.service.ts
🔇 Additional comments (1)
packages/federation-sdk/src/sdk.ts (1)
278-280: LGTM — clean SDK surface delegation.This keeps the public SDK API consistent with existing passthrough methods.
Comment on lines
+1537
to
+1546
| async updateRoomMembership({ | ||
| roomId, | ||
| userId, | ||
| membership, | ||
| content, | ||
| }: { | ||
| roomId: RoomID; | ||
| userId: UserID; | ||
| membership: 'join' | 'leave' | 'ban'; | ||
| content?: PduForType<'m.room.member'>['content']; |
Contributor
There was a problem hiding this comment.
Add senderId to this API; hardcoding sender = userId blocks valid moderation membership updates.
This method exposes 'ban' | 'leave', but with sender always equal to target user it cannot represent admin/moderator-driven transitions.
Proposed fix
async updateRoomMembership({
roomId,
userId,
+ senderId,
membership,
content,
}: {
roomId: RoomID;
userId: UserID;
+ senderId: UserID;
membership: 'join' | 'leave' | 'ban';
content?: PduForType<'m.room.member'>['content'];
}) {
@@
- sender: userId,
+ sender: senderId,
},
roomVersion,
);Also applies to: 1563-1564
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/federation-sdk/src/services/room.service.ts` around lines 1537 -
1546, The updateRoomMembership API currently hardcodes the event sender to the
target user, preventing moderator/admin-driven membership changes; modify the
updateRoomMembership function signature to accept an explicit senderId (e.g.,
add senderId: UserID param) and use senderId when building the membership
PDU/event instead of setting sender = userId; update the PDU content/type
construction and any related helper calls inside updateRoomMembership (and the
similar membership helper at the other location referenced) to pass and honor
senderId, and adjust callers to provide the correct sender (defaulting callers
to userId only where appropriate).
Comment on lines
+1553
to
+1556
| content: { | ||
| membership, | ||
| ...content, | ||
| }, |
Contributor
There was a problem hiding this comment.
Prevent content.membership from overriding the explicit membership argument.
Current merge order lets caller-provided content.membership silently override membership, which breaks the method contract.
Proposed fix
content: {
- membership,
- ...content,
+ ...content,
+ membership,
},📝 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.
Suggested change
| content: { | |
| membership, | |
| ...content, | |
| }, | |
| content: { | |
| ...content, | |
| membership, | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/federation-sdk/src/services/room.service.ts` around lines 1553 -
1556, The object merge currently spreads caller-provided content before adding
membership, which allows content.membership to override the explicit membership
argument; update the merge order so the explicit membership wins by placing
membership after the spread (i.e., produce content: { ...content, membership })
and ensure any tests or callers relying on the previous behavior are adjusted;
look for the merge expression in room.service.ts where content: { membership,
...content } is used and change it so membership is applied last.
![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 2 files
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/room.service.ts">
<violation number="1" location="packages/federation-sdk/src/services/room.service.ts:1540">
P2: `content.membership` can override the `membership` argument due to spread order, causing incorrect membership state to be sent.</violation>
<violation number="2" location="packages/federation-sdk/src/services/room.service.ts:1563">
P1: The new API cannot represent actor/target separately (`sender` is forced to `userId`), so moderation membership updates like banning another user are non-functional.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| depth: 0, | ||
| prev_events: [], | ||
| origin_server_ts: Date.now(), | ||
| sender: userId, |
There was a problem hiding this comment.
P1: The new API cannot represent actor/target separately (sender is forced to userId), so moderation membership updates like banning another user are non-functional.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/federation-sdk/src/services/room.service.ts, line 1563:
<comment>The new API cannot represent actor/target separately (`sender` is forced to `userId`), so moderation membership updates like banning another user are non-functional.</comment>
<file context>
@@ -1533,4 +1533,46 @@ export class RoomService {
+ depth: 0,
+ prev_events: [],
+ origin_server_ts: Date.now(),
+ sender: userId,
+ },
+ roomVersion,
</file context>
Comment on lines
+1540
to
+1541
| membership, | ||
| content, |
There was a problem hiding this comment.
P2: content.membership can override the membership argument due to spread order, causing incorrect membership state to be sent.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/federation-sdk/src/services/room.service.ts, line 1540:
<comment>`content.membership` can override the `membership` argument due to spread order, causing incorrect membership state to be sent.</comment>
<file context>
@@ -1533,4 +1533,46 @@ export class RoomService {
+ async updateRoomMembership({
+ roomId,
+ userId,
+ membership,
+ content,
+ }: {
</file context>
Suggested change
| membership, | |
| content, | |
| ...content, | |
| membership, |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #381 +/- ##
==========================================
- Coverage 50.56% 50.37% -0.20%
==========================================
Files 99 99
Lines 11291 11336 +45
==========================================
+ Hits 5709 5710 +1
- Misses 5582 5626 +44 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ggazzo
approved these changes
Mar 20, 2026
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
3 participants
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.
FGA-13
Summary by CodeRabbit