Skip to content

fix(react): Keep an assistant turn in one message on RTVI 2.0.0 #246

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.

Sign up for GitHub

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

Merged
markbackman merged 3 commits into from
Aug 25, 2026
Merged

fix(react): Keep an assistant turn in one message on RTVI 2.0.0 #246

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d42e831
fix(react): Keep an assistant turn in one message on RTVI 2.0.0
markbackman Aug 24, 2026
d696de2
refactor(react): Address review feedback on turn-splitting fix
markbackman Aug 25, 2026
6d6dbe5
chore: Add shared Claude Code settings
markbackman Aug 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
143 changes: 86 additions & 57 deletions client-react/src/conversation/useConversationEventWiring.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,10 +83,15 @@ export function useConversationEventWiring() {

// -- helpers ---------------------------------------------------------------

/** Cancel any pending delayed finalize, leaving no timer armed. */
const cancelFinalizeTimer = useCallback(() => {
clearTimeout(botStoppedSpeakingTimeoutRef.current);
botStoppedSpeakingTimeoutRef.current = undefined;
}, []);

const finalizeLastAssistantMessageIfPending = useAtomCallback(
useCallback((get, set) => {
clearTimeout(botStoppedSpeakingTimeoutRef.current);
botStoppedSpeakingTimeoutRef.current = undefined;
cancelFinalizeTimer();
const messages = get(messagesAtom);
const lastAssistant = findLast(messages,
(m: ConversationMessage) => m.role === "assistant"
Expand Down Expand Up @@ -139,6 +144,58 @@ export function useConversationEventWiring() {
}, [])
);

/**
* Arms (or re-arms) the delayed finalize for the in-flight assistant turn.
*
* Finalizing is deferred rather than immediate because the bot may just be
* pausing mid-turn; BotStartedSpeaking cancels the timer when that happens.
* On RTVI 2.0.0+ this timer and UserStartedSpeaking are the *only* things
* that end an assistant turn, so any path that cancels the timer has to
* re-arm it here rather than dropping it on the floor.
*/
const armBotStoppedFinalizeTimer = useAtomCallback(
useCallback((get, set) => {
cancelFinalizeTimer();

const messages = get(messagesAtom);
const lastAssistant = findLast(messages,
(m: ConversationMessage) => m.role === "assistant"
);
if (!lastAssistant || lastAssistant.final) return;

botStoppedSpeakingTimeoutRef.current = setTimeout(() => {
botStoppedSpeakingTimeoutRef.current = undefined;

// Snap the speech-progress cursor to the end of all parts.
// The bot finished speaking normally (not interrupted), so all
// text should render as "spoken". Without this, text from the
// last sentence can remain grey if the spoken BotOutput event
// didn't match the unspoken text exactly.
const msgs = get(messagesAtom);
const cursorMap = new Map(get(botOutputMessageStateAtom));
const last = findLast(msgs,
(m: ConversationMessage) => m.role === "assistant"
);
if (last) {
const cursor = cursorMap.get(last.createdAt);
if (cursor && last.parts && last.parts.length > 0) {
const lastPartIdx = last.parts.length - 1;
const lastPartText = last.parts[lastPartIdx]?.text;
cursor.currentPartIndex = lastPartIdx;
cursor.currentCharIndex =
typeof lastPartText === "string" ? lastPartText.length : 0;
for (let i = 0; i <= lastPartIdx; i++) {
cursor.partFinalFlags[i] = true;
}
set(botOutputMessageStateAtom, cursorMap);
}
}

finalizeLastMessage(get, set, "assistant");
}, BOT_STOPPED_FINALIZE_DELAY_MS);
}, [])

@Regaddi Regaddi Aug 25, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

react-hooks/exhaustive-deps flags this array for a missing cancelFinalizeTimer, and the same warning fires at line 102 on finalizeLastAssistantMessageIfPending. main is clean on this file, so both are new here.

cancelFinalizeTimer is useCallback(..., []) and therefore stable, so adding it to both arrays is behaviorally a no-op. It would also match how you already updated Connected and BotStartedSpeaking in this same diff, which is what made the inconsistency visible.

Worth noting nothing will catch this for you: CI does not run eslint on this repo, and npm run lint already fails on main with 828 no-undef errors on jest globals in the tests, so the two real warnings are buried.

@markbackman markbackman Aug 25, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 27607e2 — cancelFinalizeTimer added to both arrays. It is useCallback(..., []) so this is a no-op behaviorally, and it matches what I had already done to Connected and BotStartedSpeaking; the inconsistency was mine.

npx eslint on the two source files is now silent. Your point about nothing catching this stands and is worth its own issue: CI does not run eslint here, and npm run lint fails on main with 828 no-undef errors on jest globals, so any real warning is buried. Not fixing that in this PR.

);

// -- event handlers --------------------------------------------------------

useRTVIClientEvent(
Expand All @@ -148,10 +205,9 @@ export function useConversationEventWiring() {
clearMessages(get, set);
set(botOutputSupportedAtom, null);
set(botOutputProtocolAtom, null);
clearTimeout(botStoppedSpeakingTimeoutRef.current);
botStoppedSpeakingTimeoutRef.current = undefined;
cancelFinalizeTimer();
botOutputLastChunkRef.current = { spoken: "", unspoken: "" };
}, [])
}, [cancelFinalizeTimer])
)
);

Expand Down Expand Up @@ -180,11 +236,6 @@ export function useConversationEventWiring() {
useAtomCallback(
useCallback(
(get, set, data: BotOutputData) => {
// A BotOutput event means the response is still active; cancel any
// pending finalize timer from BotStoppedSpeaking.
clearTimeout(botStoppedSpeakingTimeoutRef.current);
botStoppedSpeakingTimeoutRef.current = undefined;

const protocol = get(botOutputProtocolAtom) ?? "legacy";

if (protocol === "v2") {
Expand All @@ -207,12 +258,17 @@ export function useConversationEventWiring() {
segment_id: data.segment_id,
};

const isFinal = data.aggregated_by === "sentence";
// `aggregated_by` describes how the text was chunked, not whether
// the turn is over: a turn contains many sentences. Marking each
// sentence final ends the message after the first one, and the
// next segment then opens a new bubble instead of continuing the
// turn. On 2.0.0 the turn is finalized by BotStoppedSpeaking (or
// by the user starting a new turn), so leave it open here.
updateAssistantBotOutput(
get,
set,
data.text,
isFinal,
false,

@Regaddi Regaddi Aug 25, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the right call, and the comment above it earns its length.

Confirming the third step of the chain in your description: I read updateAssistantBotOutput, and final: final ? true : lastMessage.final means passing false can never un-finalize a message. So a late progress event landing on an already-finalized turn just advances the cursor and does nothing else. No side effect from the looser flag.

One small thing: this comment explains why v2 passes false, but the legacy branch below has nothing saying why it deliberately keeps isFinal. Your reasoning for the asymmetry (on 1.4.x an unspoken event precedes the spoken one for each sentence, so hasUnspokenContent is true, ensureAssistantMessage reopens rather than splits, and the per-sentence final is effectively inert) is convincing, but right now it lives only in the PR description. A line of it on the legacy branch would stop someone from "fixing" the inconsistency later and quietly reintroducing this bug on 1.4.x.

@markbackman markbackman Aug 25, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 27607e2 — the reasoning is now a comment on the legacy branch itself, right above const isFinal = data.aggregated_by === "sentence". You are right that leaving it only in the PR description was an invitation to "fix" the inconsistency later and quietly reintroduce this on 1.4.x.

Thanks for independently confirming step 3. final: final ? true : lastMessage.final being monotonic is what makes the looser flag safe, and it is worth having that verified by someone other than me.

payload,
data.aggregated_by
);
Expand Down Expand Up @@ -253,64 +309,37 @@ export function useConversationEventWiring() {
data.aggregated_by
);
}

// A BotOutput event means the response is still active, so push a
// pending finalize deadline back rather than letting it fire
// mid-turn. It is postponed, not dropped: on 2.0.0 nothing else ends
// the turn, so cancelling outright for a trailing event would leave
// the message non-final until the user speaks again. This handler is
// synchronous, so a pending timer cannot have fired before here.
// Re-arming is a no-op once the message is final (legacy path).
if (botStoppedSpeakingTimeoutRef.current) {

@Regaddi Regaddi Aug 25, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This guard is the crux of the change and the comment is worth having.

One scoping note for the follow-up. Because the timer is only re-armed when one is already pending, a v2 turn that is never spoken has no finalize path at all except UserStartedSpeaking. On a text-input-only client with no VAD, that event never fires.

Per probe A in my review comment, main has the same merging behavior, so this is not a regression and I am not asking for it here. But it does mean the follow-up should be scoped to the merging, not only to the final flag. The flag is the smaller half of that problem.

@markbackman markbackman Aug 25, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch on the scoping, and I have rewritten the limitation section to match. It now separates the two symptoms explicitly: the final flag is new here and cosmetic, while the cross-turn merging via hasUnspokenContent is pre-existing and identical on main — which your probe A is what established. The follow-up is scoped to the merging, with a note that fixing the flag alone would not address it.

The text-input-only-client-with-no-VAD case is called out by name, since that is the configuration where there is genuinely no finalize path at all.

armBotStoppedFinalizeTimer();
}
},
[ensureAssistantMessage]
[armBotStoppedFinalizeTimer, ensureAssistantMessage]
)
)
);

useRTVIClientEvent(
RTVIEvent.BotStoppedSpeaking,
useAtomCallback(
useCallback((get, set) => {
// Don't finalize immediately; start a timer. Bot may start speaking again (pause).
clearTimeout(botStoppedSpeakingTimeoutRef.current);
const messages = get(messagesAtom);
const lastAssistant = findLast(messages,
(m: ConversationMessage) => m.role === "assistant"
);
if (!lastAssistant || lastAssistant.final) return;
botStoppedSpeakingTimeoutRef.current = setTimeout(() => {
botStoppedSpeakingTimeoutRef.current = undefined;

// Snap the speech-progress cursor to the end of all parts.
// The bot finished speaking normally (not interrupted), so all
// text should render as "spoken". Without this, text from the
// last sentence can remain grey if the spoken BotOutput event
// didn't match the unspoken text exactly.
const msgs = get(messagesAtom);
const cursorMap = new Map(get(botOutputMessageStateAtom));
const last = findLast(msgs,
(m: ConversationMessage) => m.role === "assistant"
);
if (last) {
const cursor = cursorMap.get(last.createdAt);
if (cursor && last.parts && last.parts.length > 0) {
const lastPartIdx = last.parts.length - 1;
const lastPartText = last.parts[lastPartIdx]?.text;
cursor.currentPartIndex = lastPartIdx;
cursor.currentCharIndex =
typeof lastPartText === "string" ? lastPartText.length : 0;
for (let i = 0; i <= lastPartIdx; i++) {
cursor.partFinalFlags[i] = true;
}
set(botOutputMessageStateAtom, cursorMap);
}
}

finalizeLastMessage(get, set, "assistant");
}, BOT_STOPPED_FINALIZE_DELAY_MS);
}, [])
)
useCallback(() => {
// Don't finalize immediately; start a timer. Bot may start speaking again (pause).
armBotStoppedFinalizeTimer();
}, [armBotStoppedFinalizeTimer])
);

useRTVIClientEvent(
RTVIEvent.BotStartedSpeaking,
useCallback(() => {
// Bot is speaking again; reset the finalize timer (bot was just pausing).
clearTimeout(botStoppedSpeakingTimeoutRef.current);
botStoppedSpeakingTimeoutRef.current = undefined;
}, [])
cancelFinalizeTimer();
}, [cancelFinalizeTimer])
);

useRTVIClientEvent(
Expand Down
38 changes: 36 additions & 2 deletions client-react/tests/conversation/helpers/storeHarness.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,12 +135,13 @@ export function createStoreHarness() {
ensureAssistantMessage();
}

const isFinal = aggregated_by === "sentence";
// v2 never finalizes per segment; the turn is finalized by
// BotStoppedSpeaking / UserStartedSpeaking. Mirrors useConversationEventWiring.
actions.updateAssistantBotOutput(
store.get,
store.set,
text,
isFinal,
false,
{ protocol: "v2", will_be_spoken, spoken_status, spoken_progress, segment_id },
aggregated_by
);
Expand All @@ -161,6 +162,38 @@ export function createStoreHarness() {
actions.finalizeLastMessage(store.get, store.set, "assistant");
}

/**
* Finalize the assistant turn the way the BotStoppedSpeaking timer does:
* snap the speech-progress cursor to the end of all parts, then finalize.
* Mirrors armBotStoppedFinalizeTimer in useConversationEventWiring.
*
* Use this for a normal, uninterrupted turn boundary. `finalizeAssistant`
* models the raw UserStartedSpeaking / interruption path, which deliberately
* does not snap the cursor (unspoken text stays unspoken).
*/
function finalizeAssistantAfterBotStoppedSpeaking() {
const messages = store.get(messagesAtom);
const cursorMap = new Map(store.get(botOutputMessageStateAtom));
const last = [...messages]
.reverse()
.find((m: ConversationMessage) => m.role === "assistant");
if (last) {
const cursor = cursorMap.get(last.createdAt);
if (cursor && last.parts && last.parts.length > 0) {
const lastPartIdx = last.parts.length - 1;
const lastPartText = last.parts[lastPartIdx]?.text;
cursor.currentPartIndex = lastPartIdx;
cursor.currentCharIndex =
typeof lastPartText === "string" ? lastPartText.length : 0;
for (let i = 0; i <= lastPartIdx; i++) {
cursor.partFinalFlags[i] = true;
}
store.set(botOutputMessageStateAtom, cursorMap);
}
}
finalizeAssistant();
}
Comment on lines +175 to +178

@Regaddi Regaddi Aug 25, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is now the second copy of the cursor-snap block, the first being inside armBotStoppedFinalizeTimer.

A test helper that mirrors production logic drifts silently, and the drift shows up as tests that keep passing while the behavior changes, which is the exact failure this harness exists to catch. Extracting the snap into an exported helper in conversationActions.ts and calling it from both sides would make the harness exercise the real thing instead of a lookalike.

Worth doing now, while the two copies are still identical and the diff is trivial.

More broadly: eventWiring.test.tsx driving the real hook through a fake event bus is the right direction for this file, and it is what let me verify the behavior above with confidence. The harness-based v2 tests would be better off migrating there over time.

@markbackman markbackman Aug 25, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 27607e2. Extracted as snapSpeechCursorToEnd(get, set) in conversationActions.ts; both armBotStoppedFinalizeTimer and the harness now call it, so the harness exercises the real function. Net -33 lines.

You were right that it was worth doing now — the copies were still byte-identical, so the diff was mechanical.

Verified behavior-neutral: 265 passing both before and after the extraction (stashed and re-ran to be sure), against 253 on main. That cross-check also caught that the PR description understated its own numbers — 12 tests added, not 5, which matches your 212 vs 200. Description corrected.

Agreed on the direction for eventWiring.test.tsx. Driving the real hook is what made the v2 tests worth trusting, and migrating the harness-based ones there over time is the right call.


/**
* Finalize the last user message.
*/
Expand Down Expand Up @@ -342,6 +375,7 @@ export function createStoreHarness() {
emitBotOutputV2,
emitUserTranscript,
finalizeAssistant,
finalizeAssistantAfterBotStoppedSpeaking,
finalizeUser,
finalizeAssistantIfPending,
finalizeUserIfPending,
Expand Down
17 changes: 17 additions & 0 deletions client-react/tests/conversation/integration/botOutputAssembly.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -653,4 +653,21 @@ describe("BotOutput assembly", () => {
expect(cursorAfter.currentCharIndex).toBe(absorbedOffset);
});
});

// -----------------------------------------------------------------------
// Backwards compatibility (protocol 1.4.x)
// -----------------------------------------------------------------------
describe("legacy per-sentence finalization", () => {
it("still marks a sentence-aggregated message final", () => {
harness.emitBotOutput("Hello there.", false, "sentence");

expect(harness.getMessages()[0].final).toBe(true);
});

it("does not mark non-sentence aggregations final", () => {
harness.emitBotOutput("Hello", false, "word");

expect(harness.getMessages()[0].final).toBeFalsy();
});
});
});
Loading
Loading