Reset overwrite url if it is invalid (does fail to reach sfu)#3595
Reset overwrite url if it is invalid (does fail to reach sfu)#3595
Conversation
toger5
added
the
PR-Developer-Experience
toger5
force-pushed
the
toger5/dont-trap-in-invalid-config
branch
from
8348e94 to
4148a05
Compare
toger5
force-pushed
the
toger5/dont-trap-in-invalid-config
branch
from
7db6d1c to
ed45177
Compare
toger5
force-pushed
the
toger5/dont-trap-in-invalid-config
branch
from
08219fe to
5ceb140
Compare
|
@Half-Shot I updated this PR so it now checks before setting the value. |
| } | ||
|
|
||
| try { | ||
| logger.debug("try setting"); |
There was a problem hiding this comment.
Assume all these debug logs will go before merge?
| customLivekitUrlTextBuffer === null | ||
| ) { | ||
| setCustomLivekitUrl(null); | ||
| return Promise.resolve(); |
There was a problem hiding this comment.
Do we need to return an explicit resolve in an async function?
There was a problem hiding this comment.
At the time I did not have the async call below this was needed i suspect ;) thanks for catching it.
| await getSFUConfigWithOpenID( | ||
| client, | ||
| customLivekitUrlTextBuffer, | ||
| "Test-room-alias-" + Date.now().toString() + client.getUserId(), |
There was a problem hiding this comment.
| "Test-room-alias-" + Date.now().toString() + client.getUserId(), | |
| `Test-room-alias-${Date.now()}${client.getUserId()}`, |
There was a problem hiding this comment.
What's the room alias thing about, btw?
There was a problem hiding this comment.
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.
| } catch (e) { | ||
| logger.error("failed setting", e); | ||
| setCustomLivekitUrlUpdateError("invalid URL (did not update)"); | ||
| // automatically unset the error after 4 seconds (2 seconds will be for the save label) |
There was a problem hiding this comment.
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.
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.
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.
toger5
force-pushed
the
toger5/dont-trap-in-invalid-config
branch
from
6d85353 to
eb60473
Compare
Half-Shot
left a comment
Half-Shot
left a comment
There was a problem hiding this comment.
Code looks good, I've added a few ideas but looks otherwise okay.
| // import { DeveloperSettingsTab } from "./DeveloperSettingsTab"; | ||
| import { DeveloperSettingsTab } from "./DeveloperSettingsTab"; |
There was a problem hiding this comment.
You seem to have a commented out import :)
| vi.mock("../livekit/openIDSFU", () => ({ | ||
| getSFUConfigWithOpenID: vi.fn().mockResolvedValue({ | ||
| url: "mock-url", | ||
| jwt: "mock-jwt", |
There was a problem hiding this comment.
I think this is okay, but just beware there are bits of code now that expect a valid JWT so in the future this might fail. There is a fixture that exports a valid test JWT.
| "local-metadata", | ||
| ), | ||
| { | ||
| isLocal: false, |
There was a problem hiding this comment.
could this also use createMockLivekitRoom (and thus avoid the as unknown stuff) if we expose isLocal as a param?
2 participants
The overwrite option currently can brick your element call and you need to update the local store manually. This PR automatically resets the overwrite url if it errored once while trying to get the livekit focus. (auto fallback to nen dev overwrite)