Improvements #65
Improvements #65
Conversation
kamil-stasiak
commented
Jan 18, 2024
•
kamil-stasiak
commented
- Remove started tracks after disconnecting.
- Display at most one error of the same type.
- Allow setting a custom room ID.
- Custom room IDs can be auto-incremented.
- Show server state now displays HTTP response.
- Rooms are sorted.
- Peers are sorted.
- The jellyfish instance is automatically selected.
src/utils/url.ts
Outdated
| @@ -0,0 +1,20 @@ | |||
| export const isValidHttpUrl = (url: string) => { | |||
There was a problem hiding this comment.
I don't think that function is actually used in any import. I would consider inlining it inside isValidJellyfishWebhookUrl. It would save us one call to URL constructor.
|
|
||
| try { | ||
| const { pathname, origin } = new URL(url); | ||
| return url !== origin || pathname !== "/"; |
There was a problem hiding this comment.
I don't think that url !== origin is needed here, if they are equal, the pathname will also equal "/"
There was a problem hiding this comment.
without it http://localhost:3000/this URL is considered invalid
src/containers/RoomsContext.tsx
Outdated
| | { type: "SET_TRACK_STREAMED"; roomId: string; peerId: string; trackId: string; serverId?: string }; | ||
|
|
||
| export type AppStore = { | ||
| rooms: Record<string, RoomState>; | ||
| selectedRoom: string | null; | ||
| }; | ||
|
|
||
| const deepCopyStateUpToPeer = (state: AppStore, roomId: string, peerId: string): AppStore => ({ |
There was a problem hiding this comment.
I don't think the name of the function accurately describes it. The are copying the state of the peer, it's just the tracks that are reset .
src/containers/Dashboard.tsx
Outdated
| setIsWss(false); | ||
| setHost(value.replace("http://", "")); | ||
| } else { | ||
| setHost(event.target.value.trim()); |
There was a problem hiding this comment.
| setHost(event.target.value.trim()); | |
| setHost(value); |
src/components/CreateRoom.tsx
Outdated
|
|
||
| const [roomId, setRoomId] = useAtom(roomIdAtom(host)); | ||
| const [roomIdInput, setRoomIdInput] = useAtom(roomIdInputAtom); | ||
| const [roomIdAutoIncrementCheckbox, setRoomIdAutoIncrement] = useAtom(roomIdAutoIncrementCheckboxAtom); |
There was a problem hiding this comment.
setter and getter names not consistent
src/components/CreateRoom.tsx
Outdated
| }; | ||
|
|
||
| const onChangeWebhookUrl = (event: ChangeEvent<HTMLInputElement>) => { | ||
| setWebhookUrl(event.target.value === "" ? null : event.target.value); |
There was a problem hiding this comment.
maybe we should trim the value?
src/components/CreateRoom.tsx
Outdated
| </label> | ||
|
|
||
| <div className="flex flex-row gap-2 items-center"> | ||
| <span className="text">Webhook URL</span> |
There was a problem hiding this comment.
Should probably use <label> for accessibility instead
Co-authored-by: Konrad Bochnia <kbochnia@gmail.com>
…hboard into improvements
…nto improvements # Conflicts: # src/containers/Room.tsx
