fix(hub-client): honor serve-sim WebRTC settings - #22
krystofwoldrich-agent wants to merge 1 commit into
Conversation
Preserve advertised codec and ICE configuration during iOS preview resolution, keep codec fallback anchored to the requested codec, and expose viewer codec controls in the dashboard. Co-authored-by: Krystof Woldrich <31292499+krystofwoldrich@users.noreply.github.com> Co-authored-by: Codex <noreply@openai.com>
krystofwoldrich-agent
left a comment
krystofwoldrich-agent
left a comment
There was a problem hiding this comment.
🤖 This is an automated review. Addressing it doesn't guarantee a merge.
Verdict: REQUEST_CHANGES
Findings: 0 critical · 1 design · 2 suggestion · 1 nit
The PR reads streamSettings.codec/iceServers from serve-sim /api, adds webRtcCodec/setWebRtcCodec to DeviceClient, anchors the codec fallback to the requested codec, and adds a codec control to StreamSection. Tests and typechecks pass on the PR head (hub-client 55/55, expo-device-hub 68/68). The main concern: upstream/main is 54 commits ahead of the PR base and already contains this feature (#24, #29). git merge-tree upstream/main upstream/pr/22 reports conflicts in useIosDevice.ts (10 regions) and DeviceOptionsSection.tsx (1 region). Only ICE server validation and the FPS window reset are not on main.
Findings
- [design] Superseded by main and does not merge cleanly —
packages/@expo/hub-client/src/useIosDevice.ts:342. Main haswebRtcCodec/setWebRtcCodec(types.ts:534-535), readsstreamSettingscodec and ICE servers (useIosDevice.ts:689-691), anchors fallback to the requested codec (useIosDevice.ts:773), and renders codec controls inStreamOptionsSection.tsx:298-305. Main'sDeviceOptionsSectionno longer takes stream props. Recommend closing as superseded or rebasing down to the residual deltas. - [suggestion] Carry over the two deltas main lacks —
packages/@expo/hub-client/src/useIosDevice.ts:294(previewIceServersruntime validation; main's iOS path trusts the JSON cast while Android validates viaisIceServer) and:744(fpsCounterRefreset; main only callssetFps(0)). - [suggestion] Test imports a sibling package by relative path —
packages/expo-device-hub/src/dashboard/__tests__/streamSection.test.tsx:4. Use the@expo/hub-componentsworkspace package, or move the test topackages/@expo/hub-components/src/__tests__/which exists on main. - [nit] Duplicate state resets on codec change —
packages/@expo/hub-client/src/useIosDevice.ts:663.selectWebRtcCodecand the effect at lines 739-746 perform the same three writes.
| } | ||
|
|
||
| /** Convert serve-sim's middleware response into the URLs consumed by the iOS client. */ | ||
| export function resolveIosPreviewConfig( |
There was a problem hiding this comment.
[design] This PR no longer merges with upstream/main (git merge-tree reports 10 conflict regions in this file and 1 in DeviceOptionsSection.tsx), and main already implements this feature. On main, toMiddleware in this hook reads c.streamSettings.codec and c.streamSettings.iceServers into ResolvedConfig.webRtcCodec/webRtcIceServers (lines 689-691), the fallback effect calls webRtcFallbackDecision(webRtcCodec, activeWebRtcCodec, failure) (line 773), DeviceClient declares webRtcCodec: DeviceWebRtcCodec and setWebRtcCodec (types.ts:534-535, non-nullable, unlike this PR), and the codec UI lives in StreamOptionsSection.tsx:298-305. Main's DeviceOptionsSection no longer takes streamMode props, so the PR's UI wiring cannot apply. Recommend closing as superseded, or rebasing down to the two deltas main lacks (see the comment on previewIceServers).
|
|
||
| type BrowserLocation = Pick<Location, 'host' | 'protocol'>; | ||
|
|
||
| function previewIceServers(value: unknown): WebRtcIceServer[] | undefined { |
There was a problem hiding this comment.
[suggestion] This validator is one of two pieces of this PR that upstream/main does not have. Main's iOS path (useIosDevice.ts:690-691) passes c.streamSettings.iceServers through on the strength of the JSON cast alone, while the Android path validates each entry (useAndroidDevice.ts:179-180, isIceServer). The other missing piece is the fpsCounterRef reset at line 744; main's reset effect (useIosDevice.ts:848-851) only calls setFps(0). A small follow-up PR against current main with these two changes plus ios-preview-config.test.ts would be easy to review.
| }); | ||
| const handledWebRtcFailureRef = useRef<string | null>(null); | ||
|
|
||
| const selectWebRtcCodec = useCallback((codec: DeviceWebRtcCodec) => { |
There was a problem hiding this comment.
[nit] selectWebRtcCodec clears handledWebRtcFailureRef, sets webRtcCodec, and clears webRtcHttpFallback. The effect at lines 739-746 performs the same three writes whenever requestedWebRtcCodec changes, which this callback triggers. One of the two paths is enough. That effect also dispatches dispatchAvccFallback('reset') on a WebRTC codec change, which is unrelated to AVCC.
| import { expect, test } from 'bun:test'; | ||
| import { renderToStaticMarkup } from 'react-dom/server'; | ||
|
|
||
| import { StreamSection } from '../../../../@expo/hub-components/src/dashboard/StreamSection'; |
There was a problem hiding this comment.
[suggestion] This reaches into a sibling package by relative path. Every other file in expo-device-hub imports the @expo/hub-components workspace package, and packages/expo-device-hub/AGENTS.md says to import UI from @expo/hub-components. The test covers a hub-components component, so it belongs in packages/@expo/hub-components/src/__tests__/ (present on main). On main the component to test is StreamOptionsSection.
1 participant
Summary
Testing