Change useSandbox hook api#525
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the useSandbox hook API to require an explicit Room Manager URL, and adjusts the livestream e2e client to use the new API as part of the changed Room Manager communication approach.
Changes:
- Change
useSandboxfrom an optional-props hook (deriving URL fromFishjamProvider) to a required{ sandboxApiUrl }API. - Update the livestream e2e app to pass
ROOM_MANAGER_URLintouseSandbox. - Introduce
ROOM_MANAGER_URLconfig sourced fromVITE_ROOM_MANAGER_URL.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| packages/react-client/src/hooks/useSandbox.ts | Makes sandboxApiUrl mandatory and removes URL derivation from FishjamProvider/fishjamId. |
| e2e-tests/livestream-client/src/App.tsx | Updates e2e app to call useSandbox({ sandboxApiUrl: ROOM_MANAGER_URL }). |
| e2e-tests/livestream-client/config.ts | Adds ROOM_MANAGER_URL env-based config for the new required hook parameter. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export const useSandbox = ({ sandboxApiUrl }: UseSandboxProps) => { | ||
| const getSandboxPeerToken = useCallback( | ||
| async (roomName: string, peerName: string, roomType: RoomType = "conference") => { | ||
| const url = new URL(sandboxApiUrl); |
There was a problem hiding this comment.
Now that sandboxApiUrl is fully caller-provided, the later string concatenations like ${sandboxApiUrl}/${roomName}/... and ${sandboxApiUrl}/livestream become sensitive to trailing slashes (double //) and unescaped roomName path segments. It’d be safer to normalize sandboxApiUrl (trim a trailing /) and build paths via new URL() + encodeURIComponent(roomName) to avoid malformed requests.
There was a problem hiding this comment.
not our problem i think
There was a problem hiding this comment.
eventually each customer problem is our problem - worth checking if it breaks once the trailing slash is added
czerwiukk
left a comment
czerwiukk
left a comment
There was a problem hiding this comment.
update e2e test utils to use sandbox url instead of building on fishjam_url, line 86 scenarios/utils.ts
Description
Changed the api of useSandbox hook as well as updated e2e livestream tests to support this new api
Motivation and Context
we've changed how users communicate with room manager
Documentation impact
Types of changes
not work as expected)