Repository navigation
Conversation
Regaddi
left a comment
There was a problem hiding this comment.
Went through this against 1.8.2 (90f8f42) by running the real wiring through usePipecatConversation and diffing the rendered output. The core fix does what it says on the tin, and I confirmed the legacy 1.4.x path, genuine mid-utterance interrupts, system-message backdating, and type-then-speak all come out identical before and after.
Two things I think are worth a look before merge, both inline below.
| // leaves the speech cursor where it stopped — the turn was interrupted, | ||
| // so unspoken text must stay unspoken. | ||
| if (message.role !== "system") { | ||
| finalizeLastAssistantMessageIfPending(); |
There was a problem hiding this comment.
finalizeLastAssistantMessageIfPending opens with an unconditional cancelFinalizeTimer(), and that timer is the only caller of snapSpeechCursorToEnd. Once it is cancelled the karaoke cursor freezes where it stopped, permanently, because the message is finalized in the same breath.
That is right when the bot is genuinely mid-utterance. It is wrong once BotStoppedSpeaking has already fired: the bot finished its turn, the text really was spoken, and the snap is what marks it so.
Repro: bot speaks a sentence whose last spoken_progress leaves the cursor mid-sentence (the mismatch case snapSpeechCursorToEnd's own docstring exists for), BotStoppedSpeaking fires, user types 500ms later.
| 1.8.2 | this branch | |
|---|---|---|
| spoken | "Let me know if you need anything else." |
"Let me know if you" |
| unspoken | "" |
" need anything else." |
And it stays that way. Same outcome with a trailing will_be_spoken: false segment, which never receives a progress event at all, so only the snap can ever mark it spoken.
Worth flagging: the UserStartedSpeaking path this mirrors already has the same bug on 1.8.2. Speaking inside the 2500ms window produces the identical stuck cursor today. So this is inheriting the flaw rather than inventing it, but it does turn a currently-correct text path into an incorrect one.
The discriminator is already to hand, namely whether the timer was armed:
const botFinishedSpeaking = botStoppedSpeakingTimeoutRef.current !== undefined;
cancelFinalizeTimer();
// ...
if (lastAssistant && !lastAssistant.final) {
if (botFinishedSpeaking) snapSpeechCursorToEnd(get, set);
finalizeLastMessage(get, set, "assistant");
}I tried that locally: it fixes the text path and the pre-existing voice path, still leaves a real mid-utterance interrupt unspoken (your cursor test passes unchanged), and all 269 tests stay green.
| // Finalizing here mirrors the UserStartedSpeaking path, which likewise | ||
| // leaves the speech cursor where it stopped — the turn was interrupted, | ||
| // so unspoken text must stay unspoken. | ||
| if (message.role !== "system") { |
There was a problem hiding this comment.
role !== "system" also catches injected assistant messages, which changes their layout. mergeMessages only merges into a non-final predecessor, so finalizing first blocks the merge:
Bot mid-turn, then injectMessage({ role: "assistant", ... }):
- 1.8.2: one bubble, parts
["Working on it.", "(tool result attached)"] - this branch: two bubbles
injectMessage is public API via usePipecatConversation, so anyone using assistant injection to append to the bot's live message (a citation, a tool-result note) gets a different layout. The reasoning in the comment above is all about text input, and the assistant case is not covered by the new tests. If the intent is really "the user typed something", then role === "user" says that directly. If the split is deliberate, probably worth a line in the comment and a test pinning it.
Minor and not blocking, while you are in here: every non-system inject now fires an extra onMessageUpdated for the assistant message. Harmless for rendering, but consumers persisting on that callback will see one more event per typed message.
Text input reaches the bot through `sendText`, which produces no UserStartedSpeaking, and the bot is usually mid-utterance, so the BotStoppedSpeaking finalize timer is not armed either. On the RTVI 2.x path those are the only two things that end an assistant turn, so the open message stayed open and every following turn was appended to it. `injectMessage` now closes an in-flight assistant turn first, mirroring UserStartedSpeaking — including leaving the speech cursor where it stopped. System messages are exempt: they are deliberately backdated behind an in-flight message so they don't split it.
The conversation's `sendText()` adds its user message itself rather than through `injectMessage`, so it missed the turn boundary: sent while the bot was speaking, the text left the bot's message open and its reply was appended to it. Once the text is sent, `sendText()` now ends the bot's turn the way an injected user message does. Text that can't be sent leaves it open.
da9620f to
1fc957a
Compare
mattieruth
left a comment
There was a problem hiding this comment.
I think this approach has aged out and we should use bot-interrupted. See #277
|
This was solved via #277. Closing |
Fix: a user message added with
injectMessage, or text sent with the conversation'ssendText(), now ends an in-flight assistant turn, so the bot's reply opens a new bubble instead of appending to the previous one.On the RTVI 2.x path a turn is ended only by
UserStartedSpeakingor the 2500 msBotStoppedSpeakingtimer. Text input produces neither: there's no VAD event, and the timer is cancelled while the bot is speaking. The nextBotOutputreopens the still-open message, and every following turn accumulates in one bubble.Regressed in 1.8.2 (#246), which dropped the per-sentence
finalflag on the v2 path. That flag was incidentally the only thing closing the bubble on the text path.Only user messages end the turn: an injected assistant message still merges into the active bubble, and a system message is still backdated behind it. If the bot was cut off mid-utterance, the speech cursor stays where it stopped, matching the
UserStartedSpeakingpath; if the bot had already stopped, it snaps to the end.sendText()ends the turn only once the text is sent, so text that can't be sent leaves it open.Rebased onto main (RTVI 2.3.0). Main's
sendText()adds its user message itself rather than throughinjectMessage, so 1fc957a applies the same turn boundary there.Checked: 6 tests in
eventWiring.test.tsxand 3 insendText.test.tsx; the twosendTexttests that expect the turn to end fail without 1fc957a. Both packages' suites pass and build. Live against a Pipecat 1.14.1.dev bot over SmallWebRTC, text sent while the bot was mid-answer used to append the reply to the interrupted bubble, and now opens a new one.Release Notes
client-react
Bug Fixes
sendText()fromusePipecatConversationor as a user message added withinjectMessage, now ends the bot's turn, so its reply starts a new message instead of being appended to the one it interrupted.