Disconnect the client when the bot disconnects, with option to disable#185
Disconnect the client when the bot disconnects, with option to disable#185mattieruth wants to merge 5 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the JS client packages and adjusts bot-disconnect handling in @pipecat-ai/client-js, including an option to automatically disconnect the client when the bot disconnects.
Changes:
- Bump workspace/package-lock versions and update transitive deps (e.g., minimatch, ajv, brace-expansion).
- Change
botDisconnected/onBotDisconnectedcallback typings to allow an undefinedParticipant. - Add
disconnectOnBotDisconnectoption and triggerPipecatClient.disconnect()on bot disconnect by default. - Relax commitlint rules by disabling header/body case checks.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| package-lock.json | Updates lockfile versions for workspace packages and transitive dependencies. |
| client-js/rtvi/events.ts | Changes botDisconnected event handler type to accept an optional Participant. |
| client-js/client/client.ts | Adds disconnectOnBotDisconnect option and auto-disconnect behavior; updates callback signature accordingly. |
| .commitlintrc.json | Disables commitlint header-case and body-case rules. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| botConnected: (participant: Participant) => void; | ||
| botReady: (botData: BotReadyData) => void; | ||
| botDisconnected: (participant: Participant) => void; | ||
| botDisconnected: (participant?: Participant) => void; |
There was a problem hiding this comment.
Changing botDisconnected handler to accept an optional Participant is a breaking TypeScript API change under strictFunctionTypes (existing handlers typed as (participant: Participant) => void will no longer be assignable). If the participant can be missing, consider keeping the public handler type unchanged and guaranteeing a Participant is passed (e.g., cache the bot participant from botConnected / botReady and reuse it on disconnect), or introduce a separate event for disconnect-without-participant rather than weakening this one.
| botDisconnected: (participant?: Participant) => void; | |
| botDisconnected: (participant: Participant) => void; |
There was a problem hiding this comment.
copilot has a valid point here, but i don't know how to NOT break this. There IS no participant when it comes to the smallWebRTC Transport.
| onBotConnected: (participant: Participant) => void; | ||
| onBotReady: (botReadyData: BotReadyData) => void; | ||
| onBotDisconnected: (participant: Participant) => void; | ||
| onBotDisconnected: (participant?: Participant) => void; |
There was a problem hiding this comment.
onBotDisconnected now takes an optional Participant, which is a breaking change for TS consumers using (participant: Participant) => void handlers (not assignable to an optional-param callback under strict). Prefer keeping the callback signature stable and ensuring a Participant is always provided, or add a new separate callback/event for the no-participant case instead of changing this one.
| onBotDisconnected: (participant?: Participant) => void; | |
| onBotDisconnected: (participant: Participant) => void; |
| /** | ||
| * Disconnect when the bot disconnects. | ||
| * | ||
| * Default to true |
There was a problem hiding this comment.
disconnectOnBotDisconnect defaults to true, which changes default client behavior (auto-disconnect on bot disconnect) without the caller opting in. For a library patch/minor release this is likely an unexpected breaking behavior change; consider defaulting to false (opt-in), or clearly treating this as a breaking change (major bump / explicit migration note) if the new default is required.
| * Default to true | |
| * Default to true. | |
| * | |
| * NOTE: This default of `true` is a breaking change compared to earlier | |
| * versions, where the client would remain connected when the bot | |
| * disconnected unless this behavior was explicitly requested. | |
| * If you rely on the previous behavior, explicitly set | |
| * `disconnectOnBotDisconnect: false` when creating the client. |
There was a problem hiding this comment.
another good point by copilot except... that technically the current behavior is breaking. we used to disconnect the client when the bot disconnected but somewhere along the way that stopped and it's pretty annoying.
client-js/client/client.ts
Outdated
| this.emit(RTVIEvent.BotDisconnected, p); | ||
| if (this._disconnectOnBotDisconnect) { | ||
| logger.info("Bot disconnected. Disconnecting client..."); | ||
| this.disconnect(); |
There was a problem hiding this comment.
disconnect() is async and can reject, but it's being called without await/error handling inside the onBotDisconnected callback. This can lead to unhandled promise rejections and makes disconnect failures silent. Consider explicitly handling the promise (e.g., await in an async wrapper or attach a .catch(...) that reports via onError/logger).
| this.disconnect(); | |
| this.disconnect().catch((error) => { | |
| logger.error("Failed to disconnect client after bot disconnected", error); | |
| }); |
| onBotDisconnected: (p) => { | ||
| options?.callbacks?.onBotDisconnected?.(p); | ||
| this.emit(RTVIEvent.BotDisconnected, p); | ||
| if (this._disconnectOnBotDisconnect) { | ||
| logger.info("Bot disconnected. Disconnecting client..."); | ||
| this.disconnect(); | ||
| } |
There was a problem hiding this comment.
New behavior (auto-disconnect when the bot disconnects, and the disconnectOnBotDisconnect opt-out) isn’t covered by existing Jest tests in client-js/tests/client.spec.ts. Please add tests that simulate a bot-disconnect callback from the transport and assert (1) disconnect is triggered by default, and (2) disconnect is not triggered when disconnectOnBotDisconnect: false.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
|
@mattieruth I've opened a new pull request, #186, to work on those changes. Once the pull request is ready, I'll request review from you. |
f8bd058 to
bf6c51b
Compare
…e option to stay connected
Co-authored-by: mattieruth <4050826+mattieruth@users.noreply.github.com>
32be552 to
ad1c19a
Compare
Right now, when the bot disconnects, transports handle the situation differently and don't necessarily even inform the client in any way. This change (along with this transport PR) changes the standard bot disconnection path so that the idea is when the bot disconnects, the transport triggers the 'botDisconnected' callback/event and the client then decides whether to disconnect based on the new flag(
disconnectOnBotDisconnect). This flag defaults totruewhich matches behavior of old that got broken somewhere along the way.