Fix timing issue: start WebRTC connection when clicking peer icon#24
Fix timing issue: start WebRTC connection when clicking peer icon#24Abmarne wants to merge 1 commit intoDhanushNehru:mainfrom
Conversation
- Start connection immediately when user clicks on peer, not when file is selected - This gives the connection time to establish before file transfer begins - Added exponential backoff retry logic instead of fixed timeout - Added connection state change handler for better debugging - Improved error handling with retry mechanism
There was a problem hiding this comment.
Pull request overview
Updates the client-side WebRTC file transfer flow to initiate peer connections earlier (on peer click), add connection-state notifications, and replace a fixed delay with retry-based waiting before sending file metadata.
Changes:
- Start WebRTC connection immediately when a peer is clicked (before file selection).
- Add
RTCPeerConnection.onconnectionstatechangelogging and user toasts for connected/lost states. - Replace the previous fixed 1s wait with retry loops (including exponential backoff) before sending file headers / waiting for the data channel to open.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| const peer = peers.get(currentTransferTarget); | ||
| if (!peer.connection || peer.connection.connectionState !== 'connected') { | ||
| await startConnection(currentTransferTarget); | ||
| // Wait briefly for connection (in reality, should listen for connection state change) | ||
| setTimeout(() => sendFileHeader(currentTransferTarget, file), 1000); | ||
| } else { | ||
|
|
||
| // Check if connection exists and is connected | ||
| if (peer.connection && peer.connection.connectionState === 'connected') { | ||
| sendFileHeader(currentTransferTarget, file); |
There was a problem hiding this comment.
fileInput change handler assumes the selected currentTransferTarget still exists. If the peer leaves between click and file selection, peers.get(currentTransferTarget) will be undefined and peer.connection will throw. Add a guard for missing peer (and ideally reset currentTransferTarget / show a toast) before accessing peer.connection.
| const peer = peers.get(peerId); | ||
| if (!peer || !peer.dataChannel) { | ||
| showToast('Connection not ready. Trying again...', 'info'); | ||
|
|
||
| // Try to start connection and wait for it | ||
| startConnection(peerId); | ||
| let attempts = 0; |
There was a problem hiding this comment.
In sendFileHeader, the !peer and !peer.dataChannel cases are handled together, but the function still calls startConnection(peerId) even when peer is null/undefined. That will cause getOrCreateConnection to return null and then pc.createDataChannel(...) will throw. Split the condition so that a missing peer returns early without trying to connect.
| const checkConnection = () => { | ||
| attempts++; | ||
| if (peer.connection && peer.connection.connectionState === 'connected') { | ||
| sendFileHeader(currentTransferTarget, file); | ||
| } else if (attempts < maxAttempts) { | ||
| // Exponential backoff: 100ms, 200ms, 400ms, etc. | ||
| setTimeout(checkConnection, 100 * Math.pow(2, attempts)); |
There was a problem hiding this comment.
The exponential-backoff timeout calculation doesn’t match the comment and grows very large: with attempts++ before scheduling, the first delay is 200ms (not 100ms) and by attempt 10 it waits ~102s, so the total wait can exceed 3 minutes before failing. Consider using 2 ** (attempts - 1) (or increment after scheduling) and capping the delay to a reasonable max.
| const checkConnection = () => { | |
| attempts++; | |
| if (peer.connection && peer.connection.connectionState === 'connected') { | |
| sendFileHeader(currentTransferTarget, file); | |
| } else if (attempts < maxAttempts) { | |
| // Exponential backoff: 100ms, 200ms, 400ms, etc. | |
| setTimeout(checkConnection, 100 * Math.pow(2, attempts)); | |
| const maxDelay = 5000; // cap per-attempt delay to 5 seconds | |
| const checkConnection = () => { | |
| attempts++; | |
| if (peer.connection && peer.connection.connectionState === 'connected') { | |
| sendFileHeader(currentTransferTarget, file); | |
| } else if (attempts < maxAttempts) { | |
| // Exponential backoff: 100ms, 200ms, 400ms, etc., capped at maxDelay | |
| const delay = Math.min(100 * Math.pow(2, attempts - 1), maxDelay); | |
| setTimeout(checkConnection, delay); |
| // Start WebRTC connection immediately when user clicks on peer | ||
| // This gives time for connection to establish before file is selected | ||
| const peer = peers.get(id); | ||
| if (!peer.connection || peer.connection.connectionState !== 'connected') { |
There was a problem hiding this comment.
Clicking a peer can trigger startConnection(id) repeatedly (e.g., multiple clicks while connectionState is still new/connecting). startConnection always creates a new DataChannel and sends a new offer, which can lead to multiple channels/offers in flight and negotiation errors. Consider tracking a per-peer “connecting/negotiating” flag (or checking pc.signalingState / existing peer.dataChannel) so startConnection is only invoked once until it either connects or fails.
| if (!peer.connection || peer.connection.connectionState !== 'connected') { | |
| const state = peer && peer.connection ? peer.connection.connectionState : null; | |
| if (!peer.connection || state === 'failed' || state === 'disconnected' || state === 'closed') { |
| showToast('Connection not ready. Trying again...', 'info'); | ||
|
|
||
| // Try to start connection and wait for it | ||
| startConnection(peerId); |
There was a problem hiding this comment.
startConnection is async but is invoked here without await/.catch(...). If createOffer/setLocalDescription fails, this can surface as an unhandled promise rejection and you won’t be able to notify the user. Consider handling the returned promise (even if you don’t block UI) and showing an error toast on failure.
| startConnection(peerId); | |
| startConnection(peerId).catch((err) => { | |
| console.error('Failed to start connection for peer', peerId, err); | |
| showToast('Connection failed. Try again.', 'error'); | |
| }); |
|
@Abmarne please check the review changes and justify the changes that you have did |
Fix:#12