feat(plugin-meetings): reconnection manager refactoring #4304
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
| .then((iceServers) => this.waitForMediaReconnection(iceServers)) | ||
| .then(() => this.maybeResendMediaRequest()) | ||
| .catch((error) => { | ||
| this.iceServersDefer.reject(); |
There was a problem hiding this comment.
I kinda don't like that I need to remember to duplicate this line here and in the meeting reconnection pipeline
There was a problem hiding this comment.
yeah I agree, maybe at least we can have a common method in ReconnectionPipeline that would do this, something like cleanupOnError() and call that in the catch from both pipelines?
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
k-wasniowski
changed the title
There was a problem hiding this comment.
Looks good but it's going to be difficult to make sure we don't regress anything (like the missing CA events) not sure how much we can rely on the existing unit tests - they might be a bit too basic. Maybe we'll need to add some more unit tests before this refactor?
| * @returns {Promise<void>} | ||
| */ | ||
| protected async initializeReconnection(): Promise<void> { | ||
| this.reconnectionPromise = this.meeting.mediaProperties.webrtcMediaConnection.reconnect( |
There was a problem hiding this comment.
this.reconnectionPromise is never read
| /** | ||
| * MediaReconnectionPipeline class extends ReconnectionPipeline | ||
| */ | ||
| export default class MeetingReconnectionPipeline extends ReconnectionPipeline { |
There was a problem hiding this comment.
maybe a better name for this class would be MeetingRejoinPipeline? then the methods executeMeetingReconnectionPipeline and startMeetingReconnection could also be renamed to "*MeetingRejoin" accordingly
| protected async waitForMediaReconnection(iceServers: RTCIceServer[]): Promise<void> { | ||
| this.iceServersDefer.resolve(iceServers); | ||
|
|
||
| await this.meeting.waitForRemoteSDPAnswer(); |
There was a problem hiding this comment.
in existing code these 2 calls are done through a callback - when I was adding them I just didn't want to add more dependencies on this.meeting, now we have these deps here again, but I guess that's ok as it won't make much difference if we don't remove all the other dependencies on this.meeting and probably to do that we should instead of defining callbacks take out all media related methods out of Meeting and put them in a separate module that would sit below Meeting and ReconnectionManager.
|
|
||
| throw error; | ||
| // This is a network retry, so we should not log START metrics again | ||
| await this.executeMediaReconnectionPipeline(networkDisconnect); |
There was a problem hiding this comment.
the existing code is setting networkDisconnect=true in this case, don't we have to do it here too then? BTW, I've never liked the name of networkDisconnect, maybe we could rename it to something more meaningful, once we fully understand what is its purpose as it's been always confusing me....
Also the existing code is setting networkRetry=true and that is then used to avoid sending "client.media.reconnecting" CA event the second time, but in this new refactored code I don't see us sending "client.media.reconnecting" at all. Where is it?
| // This is a network retry, so we should not log START metrics again | ||
| await this.executeMediaReconnectionPipeline(networkDisconnect); | ||
| } else { | ||
| throw reconnectError; |
There was a problem hiding this comment.
We also seem to be missing the code for sending 'client.call.aborted' CA event
COMPLETES #< INSERT LINK TO ISSUE >
This pull request addresses
< DESCRIBE THE CONTEXT OF THE ISSUE >
by making the following changes
< DESCRIBE YOUR CHANGES >
Change Type
The following scenarios were tested
< ENUMERATE TESTS PERFORMED, WHETHER MANUAL OR AUTOMATED >
The GAI Coding Policy And Copyright Annotation Best Practices
I certified that
Make sure to have followed the contributing guidelines before submitting.