Draft
Conversation
✅ Deploy Preview for fluxsocial-dev ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
❌ Deploy Preview for fluxdocs failed. Why did it fail? →
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughIntroduces SFU (Selective Forwarding Unit) support to WebRTC calls with a new SfuManager module for topology resolution and participant tracking, UI components for SFU indicators and quality selection, and video layout enhancements to automatically switch to Focused layout when participants exceed 8. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller/Local
participant SfuMgr as SfuManager
participant Nbhood as NeighbourhoodProxy
participant GraphQL as GraphQL API
participant RemoteGW as Remote Gateway/SFU
Caller->>SfuMgr: join(localStream)
SfuMgr->>SfuMgr: Create RTCPeerConnection
SfuMgr->>SfuMgr: Add local tracks (simulcast)
SfuMgr->>GraphQL: callJoin(roomId, offer)
GraphQL->>RemoteGW: Forward offer
RemoteGW->>RemoteGW: Process offer, generate answer
RemoteGW->>GraphQL: Return answer + participants
GraphQL->>SfuMgr: Receive answer + remote participants
SfuMgr->>SfuMgr: Set remote description
SfuMgr->>SfuMgr: Track incoming remote streams
SfuMgr->>Caller: Emit participant-joined, stream-added
Caller->>SfuMgr: leave()
SfuMgr->>GraphQL: callLeave(roomId)
SfuMgr->>SfuMgr: Close peer connection
SfuMgr->>Caller: Emit participant-left
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
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 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: 6
🧹 Nitpick comments (4)
app/src/components/call/controls/QualitySelector.vue (2)
26-26:QualityPreferencetype duplicated - consider sharing with SfuManager.The
QualityPreferencetype is defined here locally, butSfuManager.setQualityPreferencealso expects the same union type. Consider exporting this type from the webrtc package to ensure consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/call/controls/QualitySelector.vue` at line 26, The QualityPreference union is duplicated here; instead export the shared type from the webrtc package and import it where needed so SfuManager.setQualityPreference and this component use the same type. Update the declaration of QualityPreference to remove the local definition, import the exported type (e.g., QualityPreference) from the webrtc package, and adjust any references in QualitySelector.vue and SfuManager to use the single exported symbol to keep types consistent.
7-19: Dropdown lacks click-outside-to-close behavior.The dropdown opens on button click but doesn't close when clicking outside. This is a common UX pattern that users expect.
♻️ Suggested approach using a click-outside directive or composable
+import { onMounted, onUnmounted } from 'vue'; + +const dropdownRef = ref<HTMLElement | null>(null); + +function handleClickOutside(event: MouseEvent) { + if (dropdownRef.value && !dropdownRef.value.contains(event.target as Node)) { + isOpen.value = false; + } +} + +onMounted(() => document.addEventListener('click', handleClickOutside)); +onUnmounted(() => document.removeEventListener('click', handleClickOutside));And in template:
- <div class="quality-selector" v-if="showSelector"> + <div class="quality-selector" v-if="showSelector" ref="dropdownRef">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/call/controls/QualitySelector.vue` around lines 7 - 19, The dropdown in QualitySelector.vue uses isOpen and selectQuality but never closes when clicking outside; add a click-outside handler (either a v-click-outside directive or a small composable) that listens for document click events and sets isOpen = false when the click target is outside the component root; register the listener on mount and remove it on unmount (or use the directive lifecycle) and ensure the root element or the element wrapping the template (the element that currently contains the v-if="isOpen") is used to detect "outside" so selectQuality and options behavior remains unchanged.packages/webrtc/src/SfuManager.ts (1)
102-107: Missingoff()method to unsubscribe event listeners.The event system provides
on()but no way to remove listeners. This can cause memory leaks when consumers need to clean up subscriptions.♻️ Proposed addition
on(event: SfuEvent, callback: SfuEventCallback): void { if (!this.callbacks.has(event)) { this.callbacks.set(event, []); } this.callbacks.get(event)!.push(callback); } + + off(event: SfuEvent, callback: SfuEventCallback): void { + const cbs = this.callbacks.get(event); + if (cbs) { + const idx = cbs.indexOf(callback); + if (idx !== -1) cbs.splice(idx, 1); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/webrtc/src/SfuManager.ts` around lines 102 - 107, The SfuManager currently exposes on(event: SfuEvent, callback: SfuEventCallback) but lacks a corresponding off to remove listeners, which can leak memory; add an off(event: SfuEvent, callback?: SfuEventCallback) method on the SfuManager that locates the callbacks array in this.callbacks for the given event, and if a callback is provided removes only that function (filtering or splicing) and if no callback is provided clears the entire array (or deletes the map entry); ensure you handle the case where the event key is missing and after removing the last listener delete the map entry to keep this.callbacks clean.app/src/components/call/widgets/SfuSettingsPanel.vue (1)
37-43: Designated peer may go offline before save - no validation.The dropdown shows online agents at mount time, but there's no refresh mechanism or validation that the selected peer is still online when saving. Consider either refreshing the list periodically or validating at save time.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/components/call/widgets/SfuSettingsPanel.vue` around lines 37 - 43, The designatedPeer dropdown uses the initial members list but lacks validation on save; update the SfuSettingsPanel.vue to (1) refresh or re-fetch the members list before persisting and/or periodically (e.g., on mount and before save) so the options reflect current online agents, and (2) validate in the save handler (the component's save/submit method) that config.designatedPeer still exists in members and is online—if not, clear config.designatedPeer or surface a validation error and prevent save. Reference the select binding config.designatedPeer and the members array to implement these checks and the re-fetch call.
🤖 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/components/call/composables/useVideoLayout.ts`:
- Around line 88-96: The watcher that auto-switches layout ignores user choice:
modify the logic around the watch of peers.value.length so it checks a "user
selected" flag before calling uiStore.setVideoLayout; e.g., add or use a boolean
like userSelectedVideoLayout (or a method on uiStore such as
uiStore.isUserSelectedLayout()) and only call
uiStore.setVideoLayout(videoLayoutOptions[2]) when that flag is false, and
ensure any UI action that sets a layout (the existing setter used by users /
uiStore.setVideoLayout) toggles that flag to true so subsequent peer-count
changes do not override manual selections.
In `@app/src/components/call/widgets/SfuSettingsPanel.vue`:
- Around line 81-84: The onMounted handler currently swallows errors in two
empty catch blocks; update the catches for the calls to
props.neighbourhood.sfuConfig() and props.neighbourhood.onlineAgents() to log
the thrown errors (including context) instead of ignoring them — e.g., catch
(err) { console.error("Failed to load SFU config", err) } and catch (err) {
console.error("Failed to fetch online agents", err) } while leaving the existing
assignments to config and members unchanged; locate these in the onMounted block
and replace the empty catch bodies accordingly.
In `@packages/webrtc/src/SfuManager.ts`:
- Around line 124-130: Hardcoded TURN/STUN credentials are present in the
RTCPeerConnection iceServers config in SfuManager.ts (the const pc creation);
replace the embedded usernames/credentials and server URLs by reading them from
configuration/environment variables (e.g., process.env.TURN_URL,
process.env.TURN_USERNAME, process.env.TURN_CREDENTIAL and fallbacks for STUN),
and construct the iceServers array dynamically so SfuManager (the
RTCPeerConnection instantiation) uses the provided config at runtime rather than
hardcoded values; ensure sensible fallback behavior or skip TURN entry if env
vars are not present.
- Around line 181-186: The ice-gathering await in SfuManager (inside the join()
flow) can hang indefinitely because the Promise waiting on pc.iceGatheringState
=== "complete" has no timeout; update that Promise to include a configurable
timeout (e.g., default ~5–10s) so it resolves or rejects after the timeout, and
ensure you clear the timeout and remove/clear pc.onicegatheringstatechange when
finished to avoid leaks; reference the existing Promise block that checks
pc.iceGatheringState and pc.onicegatheringstatechange and add the timeout,
cleanup, and a clear resolution path on timeout so join() cannot hang forever.
- Around line 216-220: The destroy() implementation only closes the local
PeerConnection and clears callbacks/participants but never notifies the SFU
server; update destroy() to invoke the existing leave() (or callLeave) flow
before closing this.state.peerConnection so the server session is cleaned up —
call and await this.leave() (or call this.callLeave() if leave is internal) and
handle/rethrow/log errors, then proceed to close this.state.peerConnection,
clear this.callbacks and this.state.participants to avoid orphaned sessions.
- Around line 161-170: The participant DID is incorrectly set to the browser
MediaStream id in the pc.ontrack handler—update the SFU signaling and SfuManager
to use a real participant-to-stream mapping: extend the CallSession response (or
SDP/track metadata) to include a mapping of streamId (or trackId) →
participantId, store that mapping on the SfuManager (e.g., this.session or
this.state as streamIdToParticipantId), and change the pc.ontrack flow that
constructs SfuParticipantState to lookup the real DID via that mapping (use the
new mapping key when creating the SfuParticipantState in SfuManager’s pc.ontrack
handler instead of stream.id); ensure fallback logging if a mapping is missing
so you can detect unmapped streams.
---
Nitpick comments:
In `@app/src/components/call/controls/QualitySelector.vue`:
- Line 26: The QualityPreference union is duplicated here; instead export the
shared type from the webrtc package and import it where needed so
SfuManager.setQualityPreference and this component use the same type. Update the
declaration of QualityPreference to remove the local definition, import the
exported type (e.g., QualityPreference) from the webrtc package, and adjust any
references in QualitySelector.vue and SfuManager to use the single exported
symbol to keep types consistent.
- Around line 7-19: The dropdown in QualitySelector.vue uses isOpen and
selectQuality but never closes when clicking outside; add a click-outside
handler (either a v-click-outside directive or a small composable) that listens
for document click events and sets isOpen = false when the click target is
outside the component root; register the listener on mount and remove it on
unmount (or use the directive lifecycle) and ensure the root element or the
element wrapping the template (the element that currently contains the
v-if="isOpen") is used to detect "outside" so selectQuality and options behavior
remains unchanged.
In `@app/src/components/call/widgets/SfuSettingsPanel.vue`:
- Around line 37-43: The designatedPeer dropdown uses the initial members list
but lacks validation on save; update the SfuSettingsPanel.vue to (1) refresh or
re-fetch the members list before persisting and/or periodically (e.g., on mount
and before save) so the options reflect current online agents, and (2) validate
in the save handler (the component's save/submit method) that
config.designatedPeer still exists in members and is online—if not, clear
config.designatedPeer or surface a validation error and prevent save. Reference
the select binding config.designatedPeer and the members array to implement
these checks and the re-fetch call.
In `@packages/webrtc/src/SfuManager.ts`:
- Around line 102-107: The SfuManager currently exposes on(event: SfuEvent,
callback: SfuEventCallback) but lacks a corresponding off to remove listeners,
which can leak memory; add an off(event: SfuEvent, callback?: SfuEventCallback)
method on the SfuManager that locates the callbacks array in this.callbacks for
the given event, and if a callback is provided removes only that function
(filtering or splicing) and if no callback is provided clears the entire array
(or deletes the map entry); ensure you handle the case where the event key is
missing and after removing the last listener delete the map entry to keep
this.callbacks clean.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a118d53c-76ae-4915-a6bf-1f6b82055648
📒 Files selected for processing (6)
app/src/components/call/composables/useVideoLayout.tsapp/src/components/call/controls/QualitySelector.vueapp/src/components/call/widgets/SfuIndicator.vueapp/src/components/call/widgets/SfuSettingsPanel.vuepackages/webrtc/src/SfuManager.tspackages/webrtc/src/index.ts
HexaField
force-pushed
the
feat/sfu-integration
branch
2 times, most recently
from
d78828f to
9aaf7c6
Compare
- SfuManager: WebRTC SFU client with simulcast (3 encoding layers), topology resolution, and call join/leave via executor GraphQL API - SfuIndicator.vue: topology mode indicator (SFU/Mesh) with peer info - QualitySelector.vue: video quality dropdown (auto/high/medium/low) - SfuSettingsPanel.vue: neighbourhood admin panel for SFU config (mode, designated peer picker, max mesh participants) - useVideoLayout: auto-switch to speaker view for >8 participants, extended column grid for up to 25+ participants Refs: coasys/ad4m#700
- SfuManager: add off() method for event listener cleanup, configurable ICE servers via SfuIceConfig (removes hardcoded TURN credentials), ICE gathering timeout (8s), stream-to-participant DID mapping, destroy() now calls leave() to notify SFU server - Export shared QualityPreference type from webrtc package - QualitySelector: import shared type, add click-outside-to-close - SfuSettingsPanel: log errors in catch blocks, refresh members and validate designated peer is still online before save - useVideoLayout: respect user's manual layout selection (don't auto-switch to speaker view if user chose a layout)
HexaField
force-pushed
the
feat/sfu-integration
branch
from
8411a7e to
cac44af
Compare
…U state Without this, leaving an SFU call didn't call sfuManager.leave() which meant: 1. The server never received the callLeave mutation (peer not removed) 2. Rejoining would create ICE restart SDP mismatch errors because the old server-side peer still existed with stale ICE credentials
- Add agentDid parameter to SfuManager constructor - After joining, subscribe to callRenegotiationOffer for the user's DID - When a server offer arrives: setRemoteDescription, createAnswer, send back via callAnswerServerOffer - New tracks from renegotiation are handled by existing ontrack handler
neighbourhoodUrl was declared inside a try block but referenced in a sibling if block, causing ReferenceError when joining SFU calls.
The `config` variable from sfuConfig() was declared inside a try block, making it inaccessible in the outer scope where it was referenced for topology mode selection. This caused a ReferenceError that silently fell back to mesh mode. Fix: extract sfuMode before the try block, set it inside, and use the extracted variable in the outer scope. Also fix topology mapping to use 'sfu' instead of 'gateway' as the default mode.
…g bugs Bug 1: resolveTopology with maxMeshParticipants=0 must force SFU Bug 2: config mode must map to valid SfuTopology (gateway->sfu, cascaded->cascaded) Also: update existing tests for agentDid constructor parameter
… check The SFU code path was gated on communityService.value?.neighbourhood being non-null, but this proxy isn't always populated (admin token bypass, direct URL navigation, race conditions during community load). Since we already have neighbourhoodUrl from route params and use appStore.ad4mClient.neighbourhood (NeighbourhoodClient) for all SFU queries, gate on neighbourhoodUrl instead.
The joinRoom function was being invoked multiple times per button click (likely due to component re-renders or duplicate event dispatching). Added early return guard on joiningCall/inCall state.
Remove hardcoded turn:relay.ad4m.dev credentials. TURN server URL and credentials are now fetched from the sfuConfig query and applied to iceServers dynamically. Falls back to STUN-only if no TURN config.
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.
SFU Integration for Flux
Client-side integration for AD4M's embedded SFU (coasys/ad4m#712). Adds the ability to make calls through the SFU instead of direct peer-to-peer mesh, with automatic topology selection and seamless fallback.
Tracking issue: coasys/ad4m#700
Architecture
graph TD subgraph Flux["Flux App"] WS["webrtcStore<br/>(call orchestration)"] SM["SfuManager<br/>(@coasys/flux-webrtc)"] MD["mediaDevicesStore<br/>(local stream)"] UI["Call UI Components"] end subgraph Executor["AD4M Executor"] GQL["GraphQL API"] SFU["Embedded SFU"] end WS -- "topology decision" --> SM WS -- "mesh fallback" --> SimplePeer SM -- "callJoin / callLeave / callRenegotiate" --> GQL GQL --> SFU MD -- "local MediaStream" --> WS UI -- "mode toggle" --> WS SM -- "remote streams + events" --> WSKey Changes
SfuManager.ts(new,@coasys/flux-webrtcpackage)RTCPeerConnectionper SFU call (vs N-1 for mesh)ontrack→ participant DID correlation via stream mapping +CallStreamEventsubscriptions"auto"/"high"/"medium"/"low"topology-changed,participant-joined/left,active-speaker,stream-added/removed,errorwebrtcStore.ts(modified)resolveTopology()determines SFU vs mesh based on config + participant countinitSfuManager()/destroySfuManager()lifecycle managementsyncCallModeFromConfig()queriessfuConfigon mountsfuTopology,sfuPeerDid,cascadeNodeCountBug fixes during integration
recvonlytransceivers for receive-only participantsenabled=false)Topology Selection
Verified Working
Dependencies
Requires AD4M executor with
--features sfu(coasys/ad4m#712). When the SFU feature is not available, Flux falls back to mesh automatically — no breaking changes.