Repository navigation
perf(desktop): batch thinking deltas like text deltas while streaming - #160
iamcipherdev wants to merge 1 commit into
Conversation
Thinking deltas updated the store on every event, creating a new messages array and rebuilding the whole render model dozens of times per second. They now buffer in the same 50ms window as text deltas (pendingThinkingBySession + flush timer), so one window produces one store update. Ordering is preserved: pending thinking flushes synchronously before the first text delta, a tool call (content_start tool_use, tool_use_complete), turn end (message_complete), session switch/disconnect (disconnectSession), stopGeneration, errors and task summaries; clearSession drops it, and the session_patch reducer folds it into the message list the same way pending text is. Pending text is still applied before the thinking block lands, so thinking always appears before the text that follows it. Tests: 3 new tests in chatStore.test.ts (one window -> one store update; thinking lands before following text; later windows merge into the same thinking message); one existing test updated for the buffered timing. 83/83 pass, tsc clean. Render count: 10 thinking deltas in one window produced 10 store updates before, 1 after (measured in the test suite). Closes GoDiao#155
GoDiao
left a comment
There was a problem hiding this comment.
Thanks for picking up #155! The approach is right. Thinking now has its own buffer and timer next to text. The timer clears itself on flush. Thinking is flushed in the right order relative to text at tool calls, turn end, errors, stop and disconnect. The one-update-per-window test checks real behavior. A few things before merging:
1. Every reply that thinks before it writes text ends up in the wrong state (needs a fix).
The server sends content_start { blockType: 'text' } before the first content_delta. The PR flushes pending thinking at that first delta, not at content_start. So with thinking still buffered:
content_starttext setschatState: 'streaming'andactiveThinkingId: null.- The first
content_deltacallsflushPendingThinking, which setschatStateback to'thinking'andactiveThinkingIdto the thinking message. - Nothing resets them until the next tool call or turn end.
For the whole answer, MessageList renders the streaming text with isStreaming=false and keeps the ThinkingBlock active. We reproduced it with a test (thinking → content_start text → deltas): the state is streaming / null after content_start and thinking / <id> after the first delta.
Flushing at the top of content_start, for every block type, fixes it. That also makes the call in the tool_use branch redundant:
case 'content_start': {
const session = get().sessions[sessionId]
if (!session) break
flushPendingThinking(sessionId)
// ...
} else if (msg.blockType === 'tool_use') {
- flushPendingThinking(sessionId)
clearPendingToolInputDelta(sessionId)2. The ordering test should use the real event sequence.
"thinking appears before the text that follows it" sends thinking and then a text delta with no content_start in between, and it only compares message types. That's why #1 passes. Please send content_start text before the delta, then also assert the state:
send(TEST_SESSION_ID, { type: 'content_start', blockType: 'text' })
send(TEST_SESSION_ID, { type: 'content_delta', text: 'the answer' })
// ...
expect(session.chatState).toBe('streaming')
expect(session.activeThinkingId).toBeNull()3. user_message from another client doesn't flush thinking.
The user_message case still appends pending text before the incoming user message, but pending thinking isn't flushed, so it lands up to 50ms later, after the other client's message (we got ['user_text', 'thinking']). You already added the flush to the local sendMessage; the remote path needs it too. Add flushPendingThinking(sessionId) after the duplicate check and before the update.
4. Please drop the comments.
We keep new code free of comments, JSDoc included. The rule isn't in CONTRIBUTING yet; that's on us. There are three to remove:
- the three-line note above
pendingThinkingBySession - the JSDoc block, which describes
flushPendingThinkingbut sits onmergeThinkingIntoMessages - the
// the thinking landed ...line in the test
Optional while you're there: in the session_patch idle path, pending thinking is merged before pending text. flushPendingThinking does text first. It's practically unreachable during normal streaming, but swapping the two blocks would make both paths consistent.
No need to touch the CHANGELOG; we'll add the entry after merging. Thanks!
|
Hi @iamcipherdev, checking in on this one. The review above is still waiting on the |
Closes #155
Thinking deltas updated the store on every event, creating a new messages array and rebuilding the whole render model (buildRenderModel + tool groups) dozens of times per second. They now buffer in the same 50ms window as text deltas (pendingThinkingBySession + flush timer), so one window produces one store update.
Ordering is preserved exactly as the issue requires: pending thinking flushes synchronously before the first text delta, a tool call (content_start tool_use and tool_use_complete), turn end (message_complete), session switch/disconnect (disconnectSession), stopGeneration, errors and task summaries; clearSession drops it; the session_patch reducer folds pending thinking into the message list the same way pending text is folded. Pending text is still applied before the thinking block lands, so thinking always appears before the text that follows it.
Render count: 10 thinking deltas inside one window produced 10 store updates before this change and 1 after (measured with a counting probe in the vitest suite; before measured against the pre-change code, after on this branch).
Tests: desktop tests and typecheck pass (vitest: 83/83, tsc --noEmit clean). 3 new tests cover the Done-when criteria: several thinking deltas in one window produce one store update, and thinking appears before the text that follows it. One existing test ("flushes pending text before appending a thinking block") was updated for the buffered timing - its expected message order is unchanged.