MatrixRTC: Support the optional leave_reason in rtc membership content - #5437
MatrixRTC: Support the optional leave_reason in rtc membership content#5437BillCarsonFr wants to merge 13 commits into
Conversation
2456592 to
92707b2
Compare
Johennes
left a comment
There was a problem hiding this comment.
Overall looks good. Just some details.
There was a problem hiding this comment.
Pull request overview
This PR adds MSC4143 leave_reason support to MatrixRTC membership leave flows, propagating an optional structured leave reason through the public session API and membership manager, and updating event typings and unit tests accordingly.
Changes:
- Introduces
LeaveReason/leave_reasontypes and constants, and includesleave_reasonin leave and delayed-leave event content. - Updates the membership action scheduler/manager to send an explicit leave event (optionally with reason) and cancel any scheduled delayed leave.
- Extends event type mappings and updates unit tests to validate the new leave behavior.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/matrixrtc/types.ts | Adds LeaveReason types/constants and leave event content shape. |
| src/matrixrtc/MembershipManagerActionScheduler.ts | Threads leave-reason action data through the scheduler and triggers cancel+leave actions. |
| src/matrixrtc/MembershipManager.ts | Implements sending { leave_reason }, cancels scheduled delayed leave, and updates action/data plumbing. |
| src/matrixrtc/MatrixRTCSession.ts | Exposes leaveReason through leaveRoomSession and filters leave-only events. |
| src/matrixrtc/index.ts | Re-exports selected leave-reason constants from the public entrypoint. |
| src/matrixrtc/IMembershipManager.ts | Extends the public leave() API with an optional leave reason. |
| src/@types/event.ts | Updates event content typings to permit leave_reason. |
| spec/unit/matrixrtc/MembershipManager.spec.ts | Updates/adds tests for cancel + explicit leave event behavior and content. |
Comments suppressed due to low confidence (1)
src/matrixrtc/types.ts:248
- The LEAVE_REASON_DELAYED constant uses code "delayed_leaved", but MSC4143 specifies the generic code as "delayed_leave". This should match the LeaveCode union and the spec value for wire compatibility.
export const LEAVE_REASON_DELAYED: LeaveReason = {
code: "delayed_leaved",
reason: "The user was removed due to inactivity (no heartbeat received).",
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
dbkr
left a comment
There was a problem hiding this comment.
Mostly looks fine but you've said 'feat' in the PR title but 'task' in the label. It sounds like an enhancement (we use the labels instead of conventional commits because our PR titles go in the changelog).
| /** The member left intentionally (e.g. by hanging up a call) */ | ||
| export const LEAVE_REASON_HANGUP: LeaveReason = { | ||
| code: "leave", | ||
| reason: "The member left intentionally (e.g. by hanging up a call).", |
There was a problem hiding this comment.
It feels a little funny to have a user-facing string that's nonspecific about the type of application. I would rather omit the reasons in this file and (ideally) instead have them passed to the membership manager by the application's code.
| [M_POLL_START.name]: PollStartEventContent; | ||
| [M_POLL_END.name]: PollEndEventContent; | ||
| [EventType.RTCMembership]: RtcMembershipData | { msc4354_sticky_key: string }; // An object containing just the sticky key is empty. | ||
| [EventType.RTCMembership]: RtcMembershipData | { msc4354_sticky_key: string; leave_reason?: LeaveReason }; |
There was a problem hiding this comment.
Shouldn't there be more properties in this variant, like slot_id and member?
| | SessionMembershipData | ||
| | LeaveMembershipEventContent | ||
| | EmptyObject; | ||
| [EventType.RTCMembership]: RtcMembershipData | EmptyObject; |
There was a problem hiding this comment.
By the way, should this state event type still exist? (If so, should it also get a LeaveMembershipEventContent variant?)
| | LeaveMembershipEventContent | ||
| | EmptyObject; |
There was a problem hiding this comment.
The EmptyObject variant is redundant to the LeaveMembershipEventContent variant, since that interface also admits an empty object
| const eventKeysCount = Object.keys(content).filter((k) => k !== "msc4354_sticky_key").length; | ||
| // Don't even bother about empty events (saves us from costly type/"key in" checks in bigger rooms) | ||
| if (eventKeysCount === 0) return false; | ||
| if (eventKeysCount === 0 || (eventKeysCount === 1 && "leave_reason" in content)) return false; |
There was a problem hiding this comment.
| if (eventKeysCount === 0 || (eventKeysCount === 1 && "leave_reason" in content)) return false; | |
| if (eventKeysCount === 0 || "leave_reason" in content) return false; |
To account for the other keys that can be part of a leave membership (slot_id etc.)
| /** | ||
| * Additional parameters of the action | ||
| */ | ||
| data?: MembershipActionData[MembershipActionType]; |
There was a problem hiding this comment.
| data?: MembershipActionData[MembershipActionType]; | |
| data: MembershipActionData[MembershipActionType]; |
Seems reasonable to require the data when it's specified for an action type
| replace: [ | ||
| { | ||
| ts: Date.now(), | ||
| type: MembershipActionType.SendLeaveEvent, | ||
| data: leaveReason ? { leaveReason } : undefined, | ||
| }, | ||
| { | ||
| ts: Date.now(), | ||
| type: MembershipActionType.CancelledScheduledDelayedLeaveEvent, | ||
| }, | ||
| ], |
There was a problem hiding this comment.
I am glad to see that this is sending the leave event before canceling, since that's presumably a better way of dealing with the lack of atomicity in this operation. But ideally, the membership manager would only cancel the delayed event in step 2 if it actually succeeds at sending the leave event in step 1. Do you see any way to make the machine do that?
| this.clientWithSticky._unstable_sendStickyDelayedEvent( | ||
| protected clientSendDelayedDisconnectMembership: () => Promise<SendDelayedEventResponse> = () => { | ||
| this.logger.debug( | ||
| `StickyMembershipManager send delayed disconnect membership event memberId: ${this.memberId}`, |
There was a problem hiding this comment.
| `StickyMembershipManager send delayed disconnect membership event memberId: ${this.memberId}`, | |
| `send delayed disconnect membership event memberId: ${this.memberId}`, |
(It's already prefixed)
Adds support for
leave_reasonas per MSC4143Breaking change:
leave_reasonoptional key instead of enptyChecklist
public/exportedsymbols have accurate TSDoc documentation.