Skip to content

MatrixRTC: New membership manager#4726

Merged
toger5 merged 136 commits intodevelopfrom
toger5/new-MembershipManager
Mar 11, 2025
Merged

MatrixRTC: New membership manager#4726
toger5 merged 136 commits intodevelopfrom
toger5/new-MembershipManager

Conversation

@toger5
Copy link
Copy Markdown
Contributor

@toger5 toger5 commented Feb 19, 2025

Fixes: https://github.com/element-hq/voip-internal/issues/309
Part of element-hq/element-call#2972.

This does not yet use the new manager by default. It only runs the test suite with it.

Notes on error cases encountered when making HTTP requests to the Homeserver:

Type MatrixClient stack RoomWidgetClient stack
"Local" request timeout enforced by the Matrix client throws Error with name === AbortError ?
HTTP 5xx response from homeserver throws HttpError throws MatrixError
Network/socket error/timeout throws ConnectionError ?
Request timeout between widget and host Matrix client n/a now mapped to ConnectionError as part of this PR

Checklist

  • Tests written for new code (and old code if feasible).
  • New or updated public/exported symbols have accurate TSDoc documentation.
  • Linter and other CI checks pass.
  • Sign-off given on the changes (see CONTRIBUTING.md).

Copy link
Copy Markdown
Member

@dbkr dbkr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for all the fixes, looking much better. I think you may have had a merge gone wrong though as it looks like there's some reverts crept in here.

@toger5
Copy link
Copy Markdown
Contributor Author

toger5 commented Mar 7, 2025

Thanks for all the fixes, looking much better. I think you may have had a merge gone wrong though as it looks like there's some reverts crept in here.

This is probably: 0cdb075 which we needed to do to make CI happy. currently EC rc-1 depends on this branch. So we will merge this with the develop branch before we merge the PR.
This should fix the unexpected changes.

@toger5
Copy link
Copy Markdown
Contributor Author

toger5 commented Mar 8, 2025

I created the branch in this PR #4744 to keep the the commit 0cdb075 alive which we need for the EC v0.8.0-rc.1. For the actual release we will merge this PR first

Comment thread src/errors.ts Outdated
Copy link
Copy Markdown
Member

@andybalaam andybalaam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is way too huge for me to have an opinion on generally, but nothing concerning from a crypto point of view.

Comment thread spec/unit/matrixrtc/MembershipManager.spec.ts Outdated
Comment thread spec/unit/matrixrtc/MembershipManager.spec.ts Outdated
Comment thread spec/unit/matrixrtc/MembershipManager.spec.ts Outdated
Comment thread src/matrixrtc/NewMembershipManager.ts
Copy link
Copy Markdown
Member

@hughns hughns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the direction this is heading 👍

Comment thread src/matrixrtc/NewMembershipManager.ts Outdated
Comment thread src/matrixrtc/NewMembershipManager.ts Outdated
Comment thread src/matrixrtc/NewMembershipManager.ts Outdated
Comment thread src/matrixrtc/NewMembershipManager.ts Outdated
Comment thread src/matrixrtc/NewMembershipManager.ts Outdated
Comment thread src/matrixrtc/NewMembershipManager.ts Outdated
Comment thread src/matrixrtc/NewMembershipManager.ts Outdated
Comment thread src/matrixrtc/NewMembershipManager.ts Outdated
Comment thread src/matrixrtc/NewMembershipManager.ts Outdated
toger5 and others added 2 commits March 11, 2025 10:54
Co-authored-by: Hugh Nimmo-Smith <hughns@users.noreply.github.com>
Copy link
Copy Markdown
Member

@hughns hughns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Latest changes look good

Copy link
Copy Markdown
Member

@dbkr dbkr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the changes: I think splitting up the handler function has made a big difference in particular. I think I'm happy to give this a go.

Copy link
Copy Markdown
Member

@hughns hughns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome. It's great to get this over the line. 👍

@hughns
Copy link
Copy Markdown
Member

hughns commented Mar 11, 2025

@toger5 one more thing that could go in this PR, but could be a separate one, is adding some comments to say what is a workaround due to element-hq/synapse#17810 having not landed.

This would make it easier to come back and remove the workaround later.

@toger5
Copy link
Copy Markdown
Contributor Author

toger5 commented Mar 11, 2025

@toger5 one more thing that could go in this PR, but could be a separate one, is adding some comments to say what is a workaround due to element-hq/synapse#17810 having not landed.

I will do this as a seperate PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants