-
Notifications
You must be signed in to change notification settings - Fork 64
fix(react): End the assistant turn when a message is injected #249
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | |||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -31,15 +31,32 @@ export const ConversationContext = | ||||||||||
| export const PipecatConversationProvider: React.FC<React.PropsWithChildren> = ({ | |||||||||||
| children, | |||||||||||
| }) => { | |||||||||||
| useConversationEventWiring(); | |||||||||||
| const { finalizeLastAssistantMessageIfPending } = | |||||||||||
| useConversationEventWiring(); | |||||||||||
|
|
|||||||||||
| const injectMessage = useAtomCallback( | |||||||||||
| useCallback((get, set, message: { | |||||||||||
| role: "user" | "assistant" | "system"; | |||||||||||
| parts: ConversationMessagePart[]; | |||||||||||
| }) => { | |||||||||||
| // An injected message is a turn boundary the RTVI events never report: | |||||||||||
| // text input reaches the bot through `sendText`, so there is no | |||||||||||
| // UserStartedSpeaking to close the assistant's turn, and the bot was | |||||||||||
| // likely mid-utterance, so the BotStoppedSpeaking finalize timer is not | |||||||||||
| // armed either. Without this the next BotOutput reopens the still-open | |||||||||||
| // message and the following turn is appended to the previous one. | |||||||||||
| // | |||||||||||
| // System messages are excluded: `injectMessage` deliberately backdates | |||||||||||
| // them behind an in-flight assistant message so they don't split it. | |||||||||||
| // | |||||||||||
| // 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") { | |||||||||||
| finalizeLastAssistantMessageIfPending(); | |||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That is right when the bot is genuinely mid-utterance. It is wrong once Repro: bot speaks a sentence whose last
And it stays that way. Same outcome with a trailing Worth flagging: the 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. |
|||||||||||
| } | |||||||||||
| injectMessageAction(get, set, message); | |||||||||||
| }, []) | |||||||||||
| }, [finalizeLastAssistantMessageIfPending]) | |||||||||||
| ); | |||||||||||
|
|
|||||||||||
| const botOutputSupported = useAtomValue(botOutputSupportedAtom); | |||||||||||
|
|
|||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
role !== "system"also catches injected assistant messages, which changes their layout.mergeMessagesonly merges into a non-final predecessor, so finalizing first blocks the merge:Bot mid-turn, then
injectMessage({ role: "assistant", ... }):["Working on it.", "(tool result attached)"]injectMessageis public API viausePipecatConversation, 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", thenrole === "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
onMessageUpdatedfor the assistant message. Harmless for rendering, but consumers persisting on that callback will see one more event per typed message.