-
Couldn't load subscription status.
- Fork 442
fix: #613 Listen to peerConnection state in OpenAIRealtimeWebRTC to detect disconnects
#620
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
🦋 Changeset detectedLatest commit: 83c6f5f The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
sdk.v0.1.11.webrtc.bug.movwebrtc.bug.fixed.mov |
|
Thanks for sending this patch! I actually also did something similar, so I think this should be fine. I will look into details and do testing on my end too. |
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@IM594 I asked my local codex to review the changes again, and it says: And this makes sense to me. The suggested changes are: diff --git a/packages/agents-realtime/src/openaiRealtimeWebRtc.ts b/packages/agents-realtime/src/openaiRealtimeWebRtc.ts
index 2c5c661..19d5279 100644
--- a/packages/agents-realtime/src/openaiRealtimeWebRtc.ts
+++ b/packages/agents-realtime/src/openaiRealtimeWebRtc.ts
@@ -181,17 +181,22 @@ export class OpenAIRealtimeWebRTC
const dataChannel = peerConnection.createDataChannel('oai-events');
let callId: string | undefined = undefined;
- peerConnection.onconnectionstatechange = () => {
- switch (peerConnection.connectionState) {
- case 'disconnected':
- case 'failed':
- case 'closed':
- this.close();
- break;
- // 'connected' state is handled by dataChannel.onopen. So we don't need to handle it here.
- // 'new' and 'connecting' are intermediate states and don't require action here
- }
+ const attachConnectionStateHandler = (
+ connection: RTCPeerConnection,
+ ) => {
+ connection.onconnectionstatechange = () => {
+ switch (connection.connectionState) {
+ case 'disconnected':
+ case 'failed':
+ case 'closed':
+ this.close();
+ break;
+ // 'connected' state is handled by dataChannel.onopen. So we don't need to handle it here.
+ // 'new' and 'connecting' are intermediate states and do not require action here.
+ }
+ };
};
+ attachConnectionStateHandler(peerConnection);
this.#state = {
status: 'connecting',
@@ -261,8 +266,13 @@ export class OpenAIRealtimeWebRTC
peerConnection.addTrack(stream.getAudioTracks()[0]);
if (this.options.changePeerConnection) {
+ const originalPeerConnection = peerConnection;
peerConnection =
await this.options.changePeerConnection(peerConnection);
+ if (originalPeerConnection !== peerConnection) {
+ originalPeerConnection.onconnectionstatechange = null;
+ }
+ attachConnectionStateHandler(peerConnection);
this.#state = { ...this.#state, peerConnection };
}
What do you think? |
…eerConnection hook
…r on peer connection replacement
|
@seratch Great catch, and thanks for pointing this out! You're right, the handler needed to be moved. I've added a test for this. My apologies for the oversight and any extra work it caused. I really appreciate the thorough review—it's what makes the SDK more robust |
This pull request resolves #613
To resolve this issue, I added an onconnectionstatechange listener to the RTCPeerConnection instance — this is the minimum fix. If the underlying connection state transitions to 'disconnected', 'failed', or 'closed', the close() method is now invoked.