-
Notifications
You must be signed in to change notification settings - Fork 136
Unify LiveKit and Matrix connection states #3611
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
2e5e6b5 to
2e646bf
Compare
| ); | ||
|
|
||
| // Keep matrix rtc session in sync with localTransport$, connectRequested$ and muteStates.video.enabled$ | ||
| // inform the widget about the connect and disconnect intent from the user. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where does this come from.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was pulled to make the join rtc session functions sync.
BillCarsonFr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've done a live review of that together, so it is fine by me.
Just some minor comments, but no blockers
playwright/fixtures/widget-user.ts
Outdated
| try { | ||
| await expect(continueButton).toBeVisible({ timeout: 5000 }); | ||
| await expect(continueButton).toBeVisible({ timeout: 700 }); | ||
| // why do we need to put in the passwor if there is a continue button? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can be removed now? Looks like it was some old onboarding UI from web that is not needed anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool that makes sense. I was wondering in what cases it would be used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove all continueButton related code.
| page.getByRole("heading", { name: `Welcome ${username}` }), | ||
| ).toBeVisible(); | ||
|
|
||
| // Dismiss incompatible browser toast |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add more test to be sure that we really dismiss the popup we think we dismiss (check the title)?
This is fixed by updating playwright, right? is it really needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It helps us save time if we are slow with merging renovate and need to debug why our test fail.
| const mediaDevices = useMediaDevices(); | ||
| const trackProcessorState$ = useTrackProcessorObservable$(); | ||
| useEffect(() => { | ||
| logger.info("START CALL VIEW SCOPE"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add some prefix to the log until we have proper prefixed logger?
Something like [InCallView]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a proper logger.
From internal discussions about this PR: https://matrix.to/#/!tDLCaLXijNtYcJZEey:element.io/$jPuIV8UAaCVC8IPEe8LaJPDbaZ85o6nnCdxVeQoMcx0?via=element.io&via=matrix.org&via=robin.town