feat: replace STUN servers with sovereign executor-based NAT traversal#557
Draft
HexaField wants to merge 2 commits into
Draft
feat: replace STUN servers with sovereign executor-based NAT traversal#557HexaField wants to merge 2 commits into
HexaField wants to merge 2 commits into
Conversation
Removes all hardcoded STUN server lists, ICE server management UI, and related configuration. ICE candidates will be provided by the AD4M executor via ad4mClient.runtime.iceCandidates() using addresses discovered by the Iroh transport layer. Part of: coasys/ad4m#719
- webrtcStore: fetch ICE candidates from executor via ad4mClient.runtime.iceCandidates()
- Format executor addresses as stun:{address}:{port} URLs for RTCPeerConnection
- Refresh STUN servers every 60s while in a call (start on join, stop on leave)
- Fallback to empty iceServers when executor unavailable (LAN-only mode)
- WebRTCManager: accept iceServers prop, add updateIceServers() for dynamic updates
❌ Deploy Preview for fluxsocial-dev failed. Why did it fail? →
|
📝 WalkthroughWalkthroughThis PR replaces static ICE/STUN server management with dynamic Iroh-based transport by removing hardcoded server defaults, eliminating manual server configuration UI, and introducing automatic STUN candidate fetching from an executor with periodic 60-second refresh cycles integrated into room join/leave lifecycle events. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/webrtc/src/WebRTCManager.ts (1)
524-527:⚠️ Potential issue | 🔴 CriticalSyntax error: extra closing brace.
Line 526 has an extraneous closing brace that will cause a compilation error. The
sendTestBroadcastmethod closes on line 524, and the class should close directly with the brace on line 527.🐛 Proposed fix
async sendTestBroadcast() { // console.log("⚙️ Sending TEST_BROADCAST to room"); this.neighbourhood.sendBroadcastU({ links: [ { source: this.source, predicate: TEST_BROADCAST, target: Literal.from('test broadcast').toUrl(), }, ], }); } - - } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/webrtc/src/WebRTCManager.ts` around lines 524 - 527, There’s an extra closing brace after the sendTestBroadcast method causing a syntax error; remove the redundant closing brace so the sendTestBroadcast method (in WebRTCManager) ends with its single closing brace and the class closes only with the final closing brace for the WebRTCManager class.
🧹 Nitpick comments (1)
app/src/stores/webrtcStore.ts (1)
74-91: Error handling is acceptable but could be more robust.The current approach of keeping existing servers on failure and falling back to empty on first failure is reasonable. However, consider logging the response status on non-JSON errors for debugging.
♻️ Optional enhancement for better debugging
const response = await fetch(httpUrl, { method: 'POST', headers: { 'Content-Type': 'application/json', ...(token ? { authorization: token } : {}) }, body: JSON.stringify({ query: '{ runtimeIceCandidates { address port } }' }), }); + if (!response.ok) { + console.warn(`iroh-ice: executor returned status ${response.status}`); + return; + } const json = await response.json();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/stores/webrtcStore.ts` around lines 74 - 91, The fetch in refreshIceServers should log HTTP response status/body when the server returns non-OK or non-JSON to improve debugging: after calling fetch(httpUrl, ...), check response.ok and if false call response.text() and log response.status and the returned text before returning/throwing; also wrap response.json() in its own try/catch so JSON parse failures are caught and logged (include response.status and any body text), then only proceed to call formatStunServers and assign executorIceServers.value when a valid JSON payload with data.runtimeIceCandidates exists.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/stores/webrtcStore.ts`:
- Around line 93-98: The initial ICE fetch is started but not awaited, causing
executorIceServers.value to be empty when peers are created; modify
startIceRefresh/refreshIceServers so the initial fetch is awaited (e.g., have
startIceRefresh return a Promise that awaits refreshIceServers for the first
call and then starts the interval) and update joinRoom to await startIceRefresh
(or directly await refreshIceServers) before calling createPeerConnection so
executorIceServers.value is populated when peer connections are constructed.
In `@packages/webrtc/src/WebRTCManager.ts`:
- Around line 110-114: The React package is not providing ICE servers to
WebRTCManager (iceServers defaults to [] in the WebRTCManager constructor) and
updateIceServers() is never invoked; fix by wiring ICE configuration into the
hook that instantiates WebRTCManager (useWebrtc.ts): fetch or accept STUN/TURN
server config in the hook, then call
webRTCManager.updateIceServers(fetchedIceServers) (or pass props.iceServers into
new WebRTCManager({...}) when constructing) so RTCPeerConnections get proper
RTCIceServer entries; alternatively, if you intend LAN-only, add clear docs and
a runtime warning in useWebrtc.ts referencing WebRTCManager and
updateIceServers.
---
Outside diff comments:
In `@packages/webrtc/src/WebRTCManager.ts`:
- Around line 524-527: There’s an extra closing brace after the
sendTestBroadcast method causing a syntax error; remove the redundant closing
brace so the sendTestBroadcast method (in WebRTCManager) ends with its single
closing brace and the class closes only with the final closing brace for the
WebRTCManager class.
---
Nitpick comments:
In `@app/src/stores/webrtcStore.ts`:
- Around line 74-91: The fetch in refreshIceServers should log HTTP response
status/body when the server returns non-OK or non-JSON to improve debugging:
after calling fetch(httpUrl, ...), check response.ok and if false call
response.text() and log response.status and the returned text before
returning/throwing; also wrap response.json() in its own try/catch so JSON parse
failures are caught and logged (include response.status and any body text), then
only proceed to call formatStunServers and assign executorIceServers.value when
a valid JSON payload with data.runtimeIceCandidates exists.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 85c440ec-1448-4796-bee5-26a7b763d653
📒 Files selected for processing (15)
app/src/components/call/window/CallWindow.vueapp/src/stores/webrtcStore.tsapp/src/views/main/modals/webrtc-settings/Connection.vueapp/src/views/main/modals/webrtc-settings/WebrtcSettingsModal.vuepackages/constants/src/videoSettings.tspackages/react-web/src/useWebrtc.tspackages/utils/src/getDefaultIceServers.tspackages/utils/src/index.tspackages/webrtc/src/WebRTCManager.tsviews/webrtc-debug/src/stun-servers.tsviews/webrtc-view/src/components/Settings/Connection/Connection.module.cssviews/webrtc-view/src/components/Settings/Connection/Connection.tsxviews/webrtc-view/src/components/Settings/Connection/index.tsviews/webrtc-view/src/components/Settings/Settings.tsxviews/webrtc-view/src/stun-servers.ts
💤 Files with no reviewable changes (11)
- views/webrtc-view/src/components/Settings/Settings.tsx
- views/webrtc-view/src/components/Settings/Connection/index.ts
- views/webrtc-view/src/stun-servers.ts
- packages/utils/src/getDefaultIceServers.ts
- app/src/views/main/modals/webrtc-settings/WebrtcSettingsModal.vue
- packages/utils/src/index.ts
- packages/constants/src/videoSettings.ts
- app/src/views/main/modals/webrtc-settings/Connection.vue
- views/webrtc-debug/src/stun-servers.ts
- views/webrtc-view/src/components/Settings/Connection/Connection.module.css
- views/webrtc-view/src/components/Settings/Connection/Connection.tsx
Comment on lines
+93
to
+98
| /** Start periodic ICE server refresh (every 60s). */ | ||
| function startIceRefresh() { | ||
| if (iceRefreshInterval) return; | ||
| refreshIceServers(); // immediate first fetch | ||
| iceRefreshInterval = setInterval(refreshIceServers, 60_000); | ||
| } |
There was a problem hiding this comment.
Race condition: ICE servers may not be available when peers connect.
startIceRefresh() calls refreshIceServers() without awaiting it. In joinRoom(), peer connections are created immediately after calling startIceRefresh() (lines 629-636). Since refreshIceServers() is async and involves a network fetch, executorIceServers.value will likely still be empty when createPeerConnection() runs.
Consider awaiting the initial fetch before creating peer connections:
🔧 Proposed fix
- /** Start periodic ICE server refresh (every 60s). */
- function startIceRefresh() {
+ /** Start periodic ICE server refresh (every 60s). Returns promise for initial fetch. */
+ async function startIceRefresh() {
if (iceRefreshInterval) return;
- refreshIceServers(); // immediate first fetch
+ await refreshIceServers(); // await initial fetch before connections
iceRefreshInterval = setInterval(refreshIceServers, 60_000);
}Then in joinRoom():
// Start fetching STUN servers from executor
- startIceRefresh();
+ await startIceRefresh();
try {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/stores/webrtcStore.ts` around lines 93 - 98, The initial ICE fetch is
started but not awaited, causing executorIceServers.value to be empty when peers
are created; modify startIceRefresh/refreshIceServers so the initial fetch is
awaited (e.g., have startIceRefresh return a Promise that awaits
refreshIceServers for the first call and then starts the interval) and update
joinRoom to await startIceRefresh (or directly await refreshIceServers) before
calling createPeerConnection so executorIceServers.value is populated when peer
connections are constructed.
Comment on lines
+110
to
114
| private iceServers: RTCIceServer[]; | ||
|
|
||
| constructor(props: Props) { | ||
| this.iceServers = props.iceServers || []; | ||
| this.init(props); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if updateIceServers is called anywhere in the React package
rg -n 'updateIceServers' packages/react-web/Repository: coasys/flux
Length of output: 37
🏁 Script executed:
# Check how WebRTCManager is instantiated in useWebrtc.ts
rg -B 5 -A 5 'new WebRTCManager' packages/react-web/src/useWebrtc.tsRepository: coasys/flux
Length of output: 300
🏁 Script executed:
# Check the Props interface and iceServers field
rg -B 3 -A 10 'interface Props' packages/webrtc/src/WebRTCManager.tsRepository: coasys/flux
Length of output: 37
🏁 Script executed:
# Check webrtcStore.ts for ICE fetching mechanism
head -50 packages/webrtc/src/webrtcStore.tsRepository: coasys/flux
Length of output: 148
🏁 Script executed:
# List all files in webrtc/src to understand structure
find packages/webrtc/src -type f -name "*.ts" | head -20Repository: coasys/flux
Length of output: 152
🏁 Script executed:
# Search for Props type definition more broadly in WebRTCManager.ts
rg -A 15 'Props' packages/webrtc/src/WebRTCManager.ts | head -40Repository: coasys/flux
Length of output: 1194
🏁 Script executed:
# Search for ICE server initialization or fetching anywhere in webrtc package
rg -n 'iceServers' packages/webrtc/src/ | head -20Repository: coasys/flux
Length of output: 557
🏁 Script executed:
# Search for updateIceServers calls across the entire codebase
rg -n 'updateIceServers' . --type ts --type jsRepository: coasys/flux
Length of output: 146
🏁 Script executed:
# Search for any ICE server initialization in other packages
rg -n 'iceServers' packages/ --type ts -B 2 -A 2 | head -80Repository: coasys/flux
Length of output: 1656
🏁 Script executed:
# Check if there are other instantiations of WebRTCManager outside react-web
rg -n 'new WebRTCManager' . --type tsRepository: coasys/flux
Length of output: 140
React package lacks ICE server integration.
The constructor defaults iceServers to an empty array, and useWebrtc.ts instantiates WebRTCManager without passing iceServers. This means React web package users will have no STUN/TURN servers configured, breaking NAT traversal for non-LAN connections.
While updateIceServers() is defined in WebRTCManager, it is never called anywhere in the codebase. Either:
- Implement ICE server configuration in the React hook (e.g., fetch STUN servers during initialization and call
updateIceServers()), or - Document that the React package only supports LAN-only operation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/webrtc/src/WebRTCManager.ts` around lines 110 - 114, The React
package is not providing ICE servers to WebRTCManager (iceServers defaults to []
in the WebRTCManager constructor) and updateIceServers() is never invoked; fix
by wiring ICE configuration into the hook that instantiates WebRTCManager
(useWebrtc.ts): fetch or accept STUN/TURN server config in the hook, then call
webRTCManager.updateIceServers(fetchedIceServers) (or pass props.iceServers into
new WebRTCManager({...}) when constructing) so RTCPeerConnections get proper
RTCIceServer entries; alternatively, if you intend LAN-only, add clear docs and
a runtime warning in useWebrtc.ts referencing WebRTCManager and
updateIceServers.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Removes all external STUN/TURN server dependencies and replaces them with dynamic ICE candidate fetching from the local AD4M executor.
Changes
Phase 4a — STUN removal:
getDefaultIceServers.ts,stun-servers.tsand all referencesPhase 4b — Executor STUN integration:
webrtcStore.ts: FetchesruntimeIceCandidatesfrom executor GraphQL, formats asstun:URLs, 60s refresh cycle tied to room join/leaveWebRTCManager.ts: Accepts dynamiciceServers, passes toRTCPeerConnectioniceServerswhen no addresses available (LAN-only mode)How it works
runtimeIceCandidatesGraphQL endpointRTCPeerConnectionDependencies
Part of: coasys/ad4m#719
Summary by CodeRabbit
Release Notes
New Features
UI Changes
depends on d9011e34-8219-4ba1-bdd1-7d104a25cd1f/91215135-9143-46eb-8f5d-b38504a40392#742
depends on d9011e34-8219-4ba1-bdd1-7d104a25cd1f/91215135-9143-46eb-8f5d-b38504a40392#719