feat(premeeting): pre-call connection test #15151
Conversation
config.js
Outdated
| @@ -1148,6 +1148,12 @@ var config = { | |||
| // This would also require to configure watchRTCConfigParams. | |||
| // Please remember to keep rtcstatsEnabled disabled for watchRTC to work. | |||
| // watchRTCEnabled: false, | |||
|
|
|||
| // Configuration for pre-call test | |||
There was a problem hiding this comment.
Since this happens in the pre-join screen, I think we should move it to the prejoinConfig section.
config.js
Outdated
| // By setting preCallTestEnabled, you enable the pre-call test in the prejoin page. | ||
| // ICE server credentials need to be provided over the preCallTestICEUrl | ||
| // preCallTestEnabled: false, | ||
| // preCallTestICEUrl: '' |
There was a problem hiding this comment.
This should likely not be whitelisted.
There was a problem hiding this comment.
Idk, the discussion was to not have this enabled by default, just some clients
lang/main.json
Outdated
| @@ -933,6 +935,7 @@ | |||
| "goodQuality": "Awesome! Your media quality is going to be great.", | |||
| "noMediaConnectivity": "We could not find a way to establish media connectivity for this test. This is typically caused by a firewall or NAT.", | |||
| "noVideo": "We expect that your video will be terrible.", | |||
| "testFailed": "The connection test encountered unexpected issues, but this might not impact your experience", | |||
There was a problem hiding this comment.
Should this sentence end in a period?
There was a problem hiding this comment.
yup, added
|
|
||
| dispatch(setPreCallTestResults({ status: PreCallTestStatus.RUNNING })); | ||
|
|
||
| const turnCredentialsUrl = getPreCallICEUrl(getState()); |
There was a problem hiding this comment.
Why not check this before even starting the check, and skip it entirely, with a warning log line?
There was a problem hiding this comment.
The reason is that I wanted to handle this state the same as an error state so it's only handled in a single place, that being the catch at the end here. A log line is still going to get printed just as an error.
If I were to move this somewhere else I'd have to add another state and complicate the logic a bit, this felt cleaner.
| throw new Error('No TURN credentials URL provided in config'); | ||
| } | ||
|
|
||
| const turnCredentials = await fetch(turnCredentialsUrl); |
There was a problem hiding this comment.
Let's give the whole operation a timeout, in case there is a chance for something to get stuck.
There was a problem hiding this comment.
The precall test lib already has a timeout mechanism, I think that's sufficient as it's pretty straight forward.
Plus the worse case scenario is that the precall test is just going to remain in a running state, the UX itself isn't going to be blocked, users can go on and join their meeting as normal.
| @@ -145,10 +158,15 @@ const CONNECTION_TYPE_MAP: { | |||
| * @param {IProps} props - The props of the component. | |||
| * @returns {ReactElement} | |||
| */ | |||
| function ConnectionStatus({ connectionDetails, t, connectionType }: IProps) { | |||
| const ConnectionStatus = React.memo(({ connectionDetails, t, connectionType, _runPreCallTest }: IProps) => { | |||
There was a problem hiding this comment.
The prejoin screen does a bunch of re-renders, this was just meant to prevent some on this component.
The component was refactored it to useSelector and useDispatch as requested on another comment, so this got removed.
| color = { 'green' } | ||
| size = 'medium' /> | ||
| </div> | ||
| <span> </span> |
There was a problem hiding this comment.
This was just meant to add a couple of spaces between the two, but it was just an artifact from testing and is not needed anymore, removed.
| * runPreCallTest: Function | ||
| * }} | ||
| */ | ||
| function mapDispatchToProps(dispatch: IStore['dispatch']) { |
There was a problem hiding this comment.
Not a fan of mDP it's a lot of boilerplate for a function that's 1 line... can we avoid this pl?
There was a problem hiding this comment.
avoided, switched to useSelector, and useDispatch
| @@ -1,4 +1,10 @@ | |||
| import { findIndex } from 'lodash'; | |||
| throughput.audioQuality === 2 | ||
| && throughput.videoQuality === 2 | ||
| && loss.audioQuality === 2 | ||
| && loss.videoQuality === 3 |
There was a problem hiding this comment.
Why is this 3 and not 2, or >= 2?
There was a problem hiding this comment.
I think the intent here was something like "If video packet loss is not bellow 0.05%, it's not considered good quality".
Adds the possibility of enabling a pre-call test to run on the pre-meeting screen. Depends on jitsi/lib-jitsi-meet#2578
Screenshots of how this looks: