-
Notifications
You must be signed in to change notification settings - Fork 161
Reset overwrite url if it is invalid (does fail to reach sfu) #3595
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
base: livekit
Are you sure you want to change the base?
Changes from 4 commits
bd3e917
ed45177
5ceb140
c20b206
df2f503
c35b960
eb60473
582d1d8
93c9225
636f737
dbdcde2
f09e2c8
c376d9e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -28,6 +28,7 @@ import { | |||||
| InlineField, | ||||||
| Label, | ||||||
| RadioControl, | ||||||
| Text, | ||||||
| } from "@vector-im/compound-web"; | ||||||
|
|
||||||
| import { FieldRow, InputField } from "../input/Input"; | ||||||
|
|
@@ -45,6 +46,7 @@ import { | |||||
| import type { Room as LivekitRoom } from "livekit-client"; | ||||||
| import styles from "./DeveloperSettingsTab.module.css"; | ||||||
| import { useUrlParams } from "../UrlParams"; | ||||||
| import { getSFUConfigWithOpenID } from "../livekit/openIDSFU"; | ||||||
|
|
||||||
| interface Props { | ||||||
| client: MatrixClient; | ||||||
|
|
@@ -92,6 +94,8 @@ export const DeveloperSettingsTab: FC<Props> = ({ | |||||
| alwaysShowIphoneEarpieceSetting, | ||||||
| ); | ||||||
|
|
||||||
| const [customLivekitUrlUpdateError, setCustomLivekitUrlUpdateError] = | ||||||
| useState<string | null>(null); | ||||||
| const [customLivekitUrl, setCustomLivekitUrl] = useSetting( | ||||||
| customLivekitUrlSetting, | ||||||
| ); | ||||||
|
|
@@ -217,6 +221,7 @@ export const DeveloperSettingsTab: FC<Props> = ({ | |||||
| />{" "} | ||||||
| </FieldRow> | ||||||
| <EditInPlace | ||||||
| serverInvalid={customLivekitUrlUpdateError !== null} | ||||||
| onSubmit={(e) => e.preventDefault()} | ||||||
| helpLabel={ | ||||||
| customLivekitUrl === null | ||||||
|
|
@@ -229,14 +234,36 @@ export const DeveloperSettingsTab: FC<Props> = ({ | |||||
| savingLabel={t("developer_mode.custom_livekit_url.saving")} | ||||||
| cancelButtonLabel={t("developer_mode.custom_livekit_url.reset")} | ||||||
| onSave={useCallback( | ||||||
| (e: React.FormEvent<HTMLFormElement>) => { | ||||||
| setCustomLivekitUrl( | ||||||
| customLivekitUrlTextBuffer === "" | ||||||
| ? null | ||||||
| : customLivekitUrlTextBuffer, | ||||||
| ); | ||||||
| async (e: React.FormEvent<HTMLFormElement>): Promise<void> => { | ||||||
| if ( | ||||||
| customLivekitUrlTextBuffer === "" || | ||||||
| customLivekitUrlTextBuffer === null | ||||||
| ) { | ||||||
| setCustomLivekitUrl(null); | ||||||
| return Promise.resolve(); | ||||||
| } | ||||||
|
|
||||||
| try { | ||||||
| logger.debug("try setting"); | ||||||
|
||||||
| await getSFUConfigWithOpenID( | ||||||
| client, | ||||||
| customLivekitUrlTextBuffer, | ||||||
| "Test-room-alias-" + Date.now().toString() + client.getUserId(), | ||||||
|
||||||
| "Test-room-alias-" + Date.now().toString() + client.getUserId(), | |
| `Test-room-alias-${Date.now()}${client.getUserId()}`, |
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.
What's the room alias thing about, btw?
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 can be any id lk should use. But I am not sure the jwt service will use it for checking if we are part of the room. its saver to set this to the actual roomId.
Outdated
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 seems like an unusual thing to do? Usually inputs change the error state when the value changes, 4 seconds isn't even enough time to understand the error.
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.
Also, since you're not cancelling the timeout, you'll have these stack up if you try to save within that 2 second window.
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.
think it should be fine to just show the error until the user has actually entered sth reasonable.
I was worried it might be confusing what is actually set right now if the control shows an error. But probably users will change it until they have set it to sth that works.
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.
Do we need to return an explicit resolve in an async function?
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.
At the time I did not have the async call below this was needed i suspect ;) thanks for catching it.