fix(thinking): drop stale thinking chunks that outlive their turn's answer - #1347
fix(thinking): drop stale thinking chunks that outlive their turn's answer#1347chr1syy wants to merge 2 commits into
Conversation
The thinking listener writes on a ~16ms rAF while the final answer arrives via the 200ms batched flush, which owns the only mid-turn clear point. A chunk buffered just before that flush was applied just after it, appending a stale thinking log below the completed answer. Guard the fast writer's non-continuation branch: when the tab's last log is already the turn's stdout answer, drop the chunk. Applies in sticky mode too (sticky preserves logs already on screen, it does not entitle a stale buffer to create a new one). Closes finding T1. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe thinking listener now timestamps buffered thinking batches and drops batches that are not newer than a stdout answer. Regression tests cover normal and sticky modes, prompts, intermediate stdout messages, existing thinking logs, and tool activity. ChangesThinking listener behavior
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThe PR adds a renderer-side guard intended to prevent rAF-buffered thinking chunks from appearing below an answer, together with regression coverage for the targeted race.
Confidence Score: 4/5This needs to distinguish turn-final answers from intermediate stdout before merging, otherwise valid later reasoning can disappear during active turns. The parser can emit intermediate text through both thinking and stdout paths, while the new unconditional stdout check discards any reasoning chunk that arrives after such an intermediate stdout flush. Files Needing Attention: src/renderer/hooks/agent/internal/useAgentThinkingListener.ts and its focused test file Important Files Changed
Sequence DiagramsequenceDiagram
participant Agent
participant StdoutHandler
participant Batch as 200ms stdout batch
participant RAF as Thinking rAF
participant Logs as Tab logs
Agent->>StdoutHandler: Non-reasoning partial
StdoutHandler->>RAF: thinking-chunk
StdoutHandler->>Batch: streamed text
Batch->>Logs: Append intermediate stdout
Agent->>StdoutHandler: Later reasoning partial
StdoutHandler->>RAF: thinking-chunk
RAF->>Logs: Inspect last log
Logs-->>RAF: source is stdout
RAF--xLogs: Drop legitimate reasoning
Reviews (1): Last reviewed commit: "MAESTRO: drop stale thinking chunks that..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // now would resurrect a stale prefix of that same answer underneath | ||
| // it. Tested explicitly (not as !isContinuation) because a | ||
| // self-contained thinking card is a non-continuation too. | ||
| if (lastLog?.source === 'stdout') continue; |
There was a problem hiding this comment.
When Claude Code or Factory Droid emits more reasoning after an intermediate non-reasoning partial has flushed as stdout, this source-only guard treats that partial as the completed answer and drops the later thinking chunk, causing the thinking display to stop prematurely during an active turn.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
…(T1 review) Greptile P1 on PR RunMaestro#1347: dropping a thinking chunk whenever the tab's last log is `stdout` was too broad. Claude Code and Factory Droid stream at MESSAGE granularity - one `type:"assistant"` event per completed message - so an intermediate assistant message flushes as stdout mid-turn and more reasoning can legitimately follow it. The source-only guard treated that intermediate message as the completed answer and silenced the thinking display for the rest of the turn. Only a chunk that was ALREADY buffered when the stdout landed is stale; one that arrived afterwards is new content. The buffer now records when buffering started (first chunk of the coalesced batch wins), and the guard drops only when `lastLog.timestamp >= bufferedAt`. Tests: the two existing stale-straggler cases stamped the answer at `timestamp: 1`, which predates the buffer and therefore modelled the intermediate case rather than the race - they now place the answer AFTER buffering via a `landAnswerAfterBuffering` helper, which is the real interleaving. Adds a case for reasoning arriving after an intermediate stdout message; verified it fails against the old source-only guard.
|
Thanks - the P1 is real and is now fixed in You're right that a source-only guard cannot distinguish the turn's final answer from an intermediate assistant message. Since claude-code and factory-droid stream at message granularity, an intermediate message flushes as The distinguishing signal is when the chunk was buffered relative to the stdout log:
The rAF buffer now records Test changes worth calling out: the two original stale-straggler cases stamped the answer at |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/renderer/hooks/agent/internal/useAgentThinkingListener.ts`:
- Around line 64-69: Update the buffering logic using thinkingChunkBufferRef so
thinking content received after an intermediate stdout event is kept separate
from the stale pre-stdout batch. Track timestamped segments or split the batch
at the stdout boundary, and ensure the rAF flush does not drop valid post-stdout
reasoning. Add a regression test covering a thinking chunk, stdout log, later
thinking chunk, and rAF flush.
- Around line 142-144: Update the stale-buffer check in the rAF flush logic
around outlivedItsAnswer to locate the most recent stdout entry in
targetTab.logs, rather than relying on lastLog, and compare buffered.bufferedAt
against that stdout timestamp so newer buffered chunks are retained. Add a
regression test covering buffered thinking followed by stdout, tool activity,
and the rAF flush.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: acd3ad56-39b8-46e2-bff6-7ff2ee87dc7f
📒 Files selected for processing (2)
src/__tests__/renderer/hooks/agent/internal/useAgentThinkingListener.test.tsxsrc/renderer/hooks/agent/internal/useAgentThinkingListener.ts
| const existing = thinkingChunkBufferRef.current.get(bufferKey); | ||
| thinkingChunkBufferRef.current.set(bufferKey, { | ||
| text: (existing?.text ?? '') + content, | ||
| // Keep the FIRST chunk's timestamp for the whole coalesced batch. | ||
| bufferedAt: existing?.bufferedAt ?? Date.now(), | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Keep post-stdout chunks separate from stale chunks.
This buffer retains the first chunk timestamp for all later content. If a chunk buffers before an intermediate stdout log and another chunk arrives after it before the rAF flush, line 143 drops both chunks. This suppresses valid later reasoning.
Store timestamped chunk segments, or split the buffered batch at the stdout boundary. Add a regression test for: thinking chunk, stdout log, later thinking chunk, then rAF flush.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/renderer/hooks/agent/internal/useAgentThinkingListener.ts` around lines
64 - 69, Update the buffering logic using thinkingChunkBufferRef so thinking
content received after an intermediate stdout event is kept separate from the
stale pre-stdout batch. Track timestamped segments or split the batch at the
stdout boundary, and ensure the rAF flush does not drop valid post-stdout
reasoning. Add a regression test covering a thinking chunk, stdout log, later
thinking chunk, and rAF flush.
| const outlivedItsAnswer = | ||
| lastLog?.source === 'stdout' && lastLog.timestamp >= buffered.bufferedAt; | ||
| if (outlivedItsAnswer) continue; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Check the latest stdout log instead of only lastLog.
A stale chunk can buffer before stdout, then a tool entry can append before the rAF flush. In that case lastLog is not stdout, so this code appends the stale chunk after the tool entry.
Find the most recent stdout log in targetTab.logs before comparing timestamps. Keep chunks whose bufferedAt is later than that stdout timestamp. Add a regression test for: buffered thinking, stdout, tool activity, then rAF flush.
Proposed fix
- const outlivedItsAnswer =
- lastLog?.source === 'stdout' && lastLog.timestamp >= buffered.bufferedAt;
+ const latestStdoutLog = [...targetTab.logs]
+ .reverse()
+ .find((log) => log.source === 'stdout');
+ const outlivedItsAnswer =
+ latestStdoutLog?.timestamp >= buffered.bufferedAt;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const outlivedItsAnswer = | |
| lastLog?.source === 'stdout' && lastLog.timestamp >= buffered.bufferedAt; | |
| if (outlivedItsAnswer) continue; | |
| const latestStdoutLog = [...targetTab.logs] | |
| .reverse() | |
| .find((log) => log.source === 'stdout'); | |
| const outlivedItsAnswer = | |
| latestStdoutLog?.timestamp >= buffered.bufferedAt; | |
| if (outlivedItsAnswer) continue; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/renderer/hooks/agent/internal/useAgentThinkingListener.ts` around lines
142 - 144, Update the stale-buffer check in the rAF flush logic around
outlivedItsAnswer to locate the most recent stdout entry in targetTab.logs,
rather than relying on lastLog, and compare buffered.bufferedAt against that
stdout timestamp so newer buffered chunks are retained. Add a regression test
covering buffered thinking followed by stdout, tool activity, and the rAF flush.
1 participant
Closes finding T1.
Problem
A thinking log could survive below the final answer, duplicating it. Two writers hit the same
tab.logsarray on different schedules:useAgentThinkingListenerrequestAnimationFrame, ~16msuseBatchedSessionUpdatesThe mid-turn clear point lives only in the slow writer. The fast writer's non-continuation branch appended unconditionally, so a chunk buffered just before a flush was applied after it, landing below the answer it duplicated.
Fix
Refuse to append a thinking log when the tab's last log is already the turn's
stdoutanswer - a late chunk for an answered turn is stale by definition.Deliberately NOT changed: the 200ms flush interval (documented as load-bearing for input latency), and claude-code's forward-all-partials behavior in
StdoutHandler(its comment records that gating onisReasoningwas already tried and silenced the thinking display for ordinary turns).Testing
Scoped tests only, per repo convention. Adds 100 lines to
useAgentThinkingListener.test.tsxcovering the race directly: buffered chunk, stdout flush first, then the rAF fires - asserts no thinking log is appended after the answer. Also asserts the normal path and continuation coalescing still work.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests