fix(plugin-meetings): stop reachability when joining a meeting #4141
Conversation
📝 WalkthroughWalkthroughThe pull request introduces changes to manage reachability checks during media connection setup within meetings. A call to a new method, Possibly related PRs
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
yarn install v1.22.22 (For a CapTP with native promises, see @endo/eventual-send and @endo/captp) 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (4)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (18)
✨ Finishing Touches
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:
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
|
marcin-bazyl
force-pushed
the
spark-629490
branch
from
d1f50fb to
830b29b
Compare
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
There was a problem hiding this comment.
Changes LGTM.
Small doubt, since we are stopping the checks - let's assume some more resilient candidate pairs arrive slowly (edge case) - in that scenario, since we stop the connectivity checks, we don't get these candidates and essentially we will essentially be using a connection which is less optimal. Would this cause any major latency issues? Curious to know...
sreenara
left a comment
sreenara
left a comment
There was a problem hiding this comment.
Just one question about the way we're getting webex object.
| : undefined; | ||
|
|
||
| // ongoing reachability checks slow down new media connections especially on Firefox, so we stop them | ||
| this.getWebexObject().meetings.reachability.stopReachability(); |
There was a problem hiding this comment.
Quick question. Why do we have this.getWebexObject() here but I'm seeing instances of this.webex in other places, for example the line above (6719).
There was a problem hiding this comment.
it's really the same thing, I think getWebexObject() was added later, so some newer code calls it, older code uses this.webex. TBH, I'm not sure why the function was added in the first place
Once we do a join, we would have provide reachability results gathered up to the sending point of the media request. All pairs gathered later was just for information, and was not signaled to the Locus. |
4 participants
COMPLETES #SPARK-629490
This pull request addresses
If reachability is till being done in the background when we create a media connection, gathering of local candidates is very slow on Firefox, this delays meeting join and sometimes even causes Homer to timeout and join to fail.
by making the following changes
Stopping reachability when we create a media connection, this is after we've already sent the reachability results to the backend, so continuing reachability doesn't add value.
Change Type
The following scenarios where tested
unit tests
I certified that
I have read and followed contributing guidelines
I discussed changes with code owners prior to submitting this pull request
I have not skipped any automated checks
All existing and new tests passed
I have updated the documentation accordingly
Make sure to have followed the contributing guidelines before submitting.