fix(site/src/pages/AgentsPage): suppress duplicate streaming tool-result row for pending calls - #28063
fix(site/src/pages/AgentsPage): suppress duplicate streaming tool-result row for pending calls#28063DanielleMaywood wants to merge 5 commits into
Conversation
…ult row for pending calls When a local tool result streams before its tool step commits, the durable transcript already renders the pending call while the cleared stream state repopulates a result-only row. This produced a transient second row (for example "Read file") next to the durable pending row (for example "Reading AgentChatPage.tsx…") until the tool step commit merged them. Filter result-only streaming entries in the live tail whose tool_call_id matches a durable pending call, keyed on id so a parallel second call to the same tool is not hidden. Both the result entry and its tool block are dropped so the tail does not render a generic "Tool" placeholder for the dropped id, and when every block is dropped the tail behaves as if no output has accumulated yet while the durable pending row stays visible.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 536a0bf2f0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const visibleStreamState = filterPendingStreamState( | ||
| streamState, | ||
| pendingToolCallIDs, | ||
| ); |
There was a problem hiding this comment.
Add Storybook coverage for the filtered live tail
When a durable pending tool call and a result-only stream entry share an ID, this changes user-visible rendering through ChatPageTimeline and LiveStreamTail, but the added unit tests exercise only the pure filter helper. Add a Storybook story with a play assertion proving that only the durable row remains while a parallel call with a different ID stays visible; otherwise the prop wiring, live-status derivation, and rendered timeline behavior remain untested.
AGENTS.md reference: site/AGENTS.md:L9-L10
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 64e6939. Added the story StreamedResultForPendingToolDoesNotDuplicate in ChatPageContent.stories.tsx, which renders ChatPageTimeline with a real chat store: a durable pending read_file call (tc-1) plus its streamed result-only part, and a parallel in-stream call (tc-2). The play function asserts both sides of the invariant: tc-1 renders exactly once as the durable pending row with the duplicate suppressed, and the tc-2 row stays visible. Verified in chromium via the storybook test project.
| new Set(["tc-1"]), | ||
| ); | ||
| expect(filtered).not.toBeNull(); | ||
| expect(filtered!.toolResults).toEqual({}); |
There was a problem hiding this comment.
Replace the new non-null assertions
The new tests repeatedly access filtered! after a Vitest assertion, which does not narrow the variable and violates the frontend rule prohibiting non-null assertions. Use an explicit guard or another narrowing pattern before accessing the returned state so these tests retain type safety.
AGENTS.md reference: site/AGENTS.md:L166-L168
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 64e6939. Replaced all non-null assertions in the new filterPendingStreamState tests with optional-chaining assertions (filtered?.toolResults, filtered?.blocks, etc.), keeping the explicit not.toBeNull() guard only where it gives a readable failure. Pre-existing tests were left untouched.
| }, | ||
| sources: [], | ||
| }; | ||
| // Only tc-1 is pending in the durable transcript. |
There was a problem hiding this comment.
Remove comments that merely narrate the test inputs
This comment only restates the new Set(["tc-1"]) argument on the next line, and the similar comment before the no-op assertions does the same. Remove these comments because they add no behavioral context and violate the frontend requirement against comments that restate identifiers, assertions, or control flow.
AGENTS.md reference: site/AGENTS.md:L15-L16
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 64e6939. Removed the two comments that restated the test inputs. Kept the other two new comments because they document non-obvious behavior: why the tool block must be dropped alongside the result entry, and that an in-stream call wins over a durable pending entry.
…ltering Add a ChatPageTimeline story covering the streamed-result flicker window: a durable pending read_file call plus its streamed result-only row, which the live tail filters, and a parallel in-stream call that still renders. The play function asserts the durable row appears once and the parallel row stays visible. Replace non-null assertions in the filter tests with optional-chaining assertions and drop two comments that restated test inputs.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 64e6939692
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const toolResults: StreamState["toolResults"] = {}; | ||
| let dropped = false; | ||
| for (const [id, result] of Object.entries(streamState.toolResults)) { | ||
| if (pendingToolCallIDs.has(id) && !streamState.toolCalls[id]) { |
There was a problem hiding this comment.
Preserve streamed advisor result deltas
When an advisor call is durable and pending, chatd subsequently publishes its advice as tool-result parts containing result_delta (coderd/x/chatd/chatadvisor/tool.go:62-71). Those parts repopulate the cleared stream state without a matching in-stream call and have isStreaming: true, but this condition drops them exactly like final results. Consequently every advisor invocation loses its incremental advice UI until the final durable tool message commits. Exclude streaming results from this suppression or merge them into the durable pending row; the added story supplies only a final result, so it does not exercise this real streaming branch.
AGENTS.md reference: site/AGENTS.md:L9-L10
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d55db27. filterPendingStreamState now keeps result-only entries while they are still streaming: an entry is dropped only when it matches a durable pending call, has no in-stream call, and isStreaming is false. Advisor result_delta parts keep rendering the progressive advice UI, and once the final result part arrives (no delta, result set) the entry is suppressed again, preserving the flicker fix. Covered by two unit tests (keep-while-streaming, drop-once-final) and a new story (StreamedAdvisorAdviceForPendingToolStillRenders) asserting the accumulated advice renders for a pending advisor call.
…or pending calls filterPendingStreamState dropped result-only stream entries for durable pending calls, which also suppressed the advisor's streamed result_delta parts and hid the progressive advice UI until the final tool message committed. Keep entries while they are still streaming; the final result part is still suppressed once it arrives.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d55db27a97
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return { | ||
| ...streamState, | ||
| blocks: streamState.blocks.filter( |
There was a problem hiding this comment.
Treat a fully filtered stream as empty
When the pending result is the only live block, this returns a non-null StreamState whose blocks and tool maps are empty. LiveStreamTail passes that state to deriveLiveStatus, which treats every non-null state as streaming, and shouldShowGenericThinking consequently renders a generic “Thinking” row beneath the durable pending tool until the preview reset arrives. Normalize a completely filtered state to null, or otherwise derive the live status from whether visible output remains, so suppressing the duplicate does not replace it with an unrelated row.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d6e89c9. filterPendingStreamState now normalizes a fully filtered state to null: after dropping, if blocks are empty, there are no in-stream tool calls, and no sources, it returns null instead of a non-null empty state. The live tail then renders nothing beneath the durable pending row (no stray Thinking shimmer) until the preview reset arrives. Every surviving toolResult is guaranteed a tool block via ensureToolBlock, so empty blocks implies empty toolResults. Retry/reconnect/error phases and isAwaitingFirstStreamChunk are unaffected (they read retry/reconnect/stream-error state or the raw store state, not the filtered value). Covered by updated unit tests: fully filtered → null, partially filtered → non-null.
… to null When filtering drops the only live content for a durable pending call, the live tail rendered a generic Thinking shimmer beneath the pending row because a non-null empty state reads as streaming. Return null when no visible content remains so the tail renders nothing until the preview reset arrives.
…cs and guard Shorten the doc comment to the behavior contract and point at the tests for the full detail. Make the empty-state guard self-evident by checking the filtered toolResults directly instead of relying on the block/result invariant.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5e34550f4c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| pendingToolCallIDs.has(id) && | ||
| !streamState.toolCalls[id] && | ||
| !result.isStreaming |
There was a problem hiding this comment.
Keep final advisor advice visible until durable commit
When an advisor finishes, its final tool-result contains result, which makes applyMessagePartToStreamState clear isStreaming before the durable tool message commits; this predicate then removes the result and its block even though the durable pending row contains only the question and still says "Consulting advisor…". The advice therefore disappears between the final SSE event and the durable commit. Fresh evidence after the prior thread is the new drops the final result for a pending durable call once streaming completes test, which explicitly expects this state to become null, while the Storybook story covers only result_delta events. Preserve the advisor's final result until the durable result is available, and cover that final-event window in the story.
AGENTS.md reference: site/AGENTS.md:L9-L10
Useful? React with 👍 / 👎.
| !streamState.toolResults[block.id]?.isStreaming | ||
| ), | ||
| ); | ||
| // Nothing visible remains. |
There was a problem hiding this comment.
Remove the control-flow narration
The Nothing visible remains comment only paraphrases the zero-length and empty-map guard immediately below it, so it violates FE4's explicit prohibition on comments that restate control flow. Remove it rather than narrating what the condition already expresses.
AGENTS.md reference: site/AGENTS.md:L15-L15
Useful? React with 👍 / 👎.
When a local tool finishes, chatd streams its
tool-resultpart before the durable tool message is committed. During that window the transcript already renders the pending call (e.g. "Reading AgentChatPage.tsx…") while the live tail renders the result as a second, arg-less row (e.g. a generic "Read file"). Once the tool step commits, the two rows collapse into one ("Read AgentChatPage.tsx"), producing a jarring 1→2→1 flicker. read_file is the visible case because its label degrades without args; other local tools share the same transient duplicate.Filter result-only streaming entries in the live tail whose
tool_call_idmatches a durable pending call, keyed on id so a parallel second call to the same tool is not hidden. Both the result entry and itstoolblock are dropped so the tail does not render a generic "Tool" placeholder for the dropped id, and when every block is dropped the tail behaves as if no output has accumulated yet while the durable pending row stays visible.Root cause analysis
Sequence for one
read_fileprocessStepStream(coderd/x/chatd/chatloop/chatloop.go:908-914) publishestool-callparts withargs_delta. The frontend accumulates them inStreamStateand the live tail renders one running row: "Reading AgentChatPage.tsx…".taskStarter.generateAssistant(coderd/x/chatd/generation.go:768) commits the durable assistant message. The history bump emits a durablemessageevent pluspreview_reset(stream_loop.go:287-293); the client upserts the message and clearsstreamStateatomically (useChatStore.ts:659-683). The call is still pending (no result yet), sogetPendingToolCallIDs/mergeToolskeep it atstatus: "running": one row, "Reading AgentChatPage.tsx…".ExecuteLocalToolspublishes each tool result over SSE the instant the tool finishes (chatloop.go:606-612), while the durable tool-role message only lands at the latercommitGenerationStep(generation.go:889→transitions.go:1174). The streaming result repopulatesstreamState;buildStreamToolsyields a result-onlyMergedToolandBlockListrenders it as its own row. With noargs,getReadFileToolDatafalls back to path"file"(ReadFileTool.tsx:55), producing "Read file". Meanwhile the durable transcript still shows the pending call. Two rows.preview_resetclears the tail. One row again.Why the fix is render-side
ConversationTimeline) and the streaming tail (LiveStreamTail) are deliberately independent render paths with no shared correlation key, and the atomic upsert+clear in the store was built to avoid dual-render windows. Merging the streamed result into the durable row would re-couple them.result_deltastreaming (coderd/x/chatd/chatadvisor/tool.go:54-71) and compaction visibility.shouldRenderToolentirely (rendered viaReadFileTimelineBlock,ConversationTimeline.tsx:419-420), so a visibility-filter-only change would not reach them.What this change does
ChatPageContentpasses its already-computedpendingToolCallIDsintoLiveStreamTail.filterPendingStreamStatehelper drops a streaming result entry plus its{type:"tool", id}block only whenpendingToolCallIDs.has(id) && !streamState.toolCalls[id](i.e. durable pending call exists and there is no in-stream call to merge with). Returns the input reference unchanged when nothing is dropped so memoized consumers do not re-render.Generated by Coder Agents.