fix(ui): prevent chat messages from disappearing after send when tools are used#67699
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1bf1971e67
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| void loadChatHistory(host as unknown as OpenClawApp).then(() => { | ||
| resetToolStream(toolHost); | ||
| }); |
There was a problem hiding this comment.
Scope deferred tool-stream reset to the originating run
The deferred resetToolStream runs unconditionally after loadChatHistory resolves, but flushChatQueueForEvent is triggered just before this block and can immediately start the next queued run. In that case, the .then(...) callback can fire while the new run is already streaming tool output and clear its chatToolMessages/chatStreamSegments, causing tool cards to disappear mid-run. This race is introduced by the new async reset timing and is reproducible when a queued follow-up message starts quickly after a tool-using final event.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR fixes a bug where user messages and assistant replies disappear briefly after tool-assisted exchanges in the dashboard chat UI. The root cause was
Confidence Score: 4/5Safe to merge after resolving the tab-nav session switch regression; all other fixes are correct. The four fixes correctly address the disappearing-message bug and are well-tested. One P1 regression was identified: ui/src/ui/app-render.helpers.ts —
|
| // would cause the streamed text to vanish and reappear on the final event. | ||
| if (!state.chatRunId) { | ||
| maybeResetToolStream(state); | ||
| state.chatStream = null; | ||
| state.chatStreamStartedAt = null; | ||
| } |
There was a problem hiding this comment.
maybeResetToolStream inside loadChatHistory neutralises the defer in Fix 3
handleTerminalChatEvent defers resetToolStream until after loadChatHistory resolves so that tool cards stay visible during the round-trip. However, by the time the deferred loadChatHistory is called, handleChatEvent has already set chatRunId = null (final-event path, line 344). So !state.chatRunId is true here and maybeResetToolStream fires inside loadChatHistory—before the .then() callback—making the second resetToolStream in handleTerminalChatEvent a no-op.
The guard still achieves the right UX (tool cards cleared only after history arrives), but the logic is misleading: the behaviour leans entirely on the maybeResetToolStream call here, not on the deferred .then(). Consider documenting this interaction, or skip maybeResetToolStream in loadChatHistory when called from the tool-event path (e.g. by accepting an option like { keepToolStream?: boolean }) so Fix 3's intent is explicit rather than incidental.
Prompt To Fix With AI
This is a comment left during a code review.
Path: ui/src/ui/controllers/chat.ts
Line: 113-118
Comment:
**`maybeResetToolStream` inside `loadChatHistory` neutralises the defer in Fix 3**
`handleTerminalChatEvent` defers `resetToolStream` until after `loadChatHistory` resolves so that tool cards stay visible during the round-trip. However, by the time the deferred `loadChatHistory` is called, `handleChatEvent` has already set `chatRunId = null` (final-event path, line 344). So `!state.chatRunId` is true here and `maybeResetToolStream` fires *inside* `loadChatHistory`—before the `.then()` callback—making the second `resetToolStream` in `handleTerminalChatEvent` a no-op.
The guard still achieves the right UX (tool cards cleared only after history arrives), but the logic is misleading: the behaviour leans entirely on the `maybeResetToolStream` call here, not on the deferred `.then()`. Consider documenting this interaction, or skip `maybeResetToolStream` in `loadChatHistory` when called from the tool-event path (e.g. by accepting an option like `{ keepToolStream?: boolean }`) so Fix 3's intent is explicit rather than incidental.
How can I resolve this? If you propose a fix, please make it concise.|
This issue has a similar root cause to what I fixed in PR #68014. The problem is that |
|
Closing this as implemented after Codex automated review. Current main already implements the #67699 behavior: stale chat.history snapshots preserve optimistic user/assistant tail messages, tool-assisted final events reload history before clearing tool streams with run scoping, and session switches clear old chat messages. The related #68014 and #70391 discussions were useful, but this PR is now superseded by shipped/current main behavior. Best possible solution: Close #67699 as implemented on main. Keep the current signature-based optimistic-tail preservation, run-scoped deferred tool reset, and session-switch clearing behavior; any remaining edge case should be filed as a narrower reproduction against current main rather than merging this stale duplicate PR. What I checked:
So I’m closing this as already implemented rather than keeping a duplicate issue open. Codex Review notes: model gpt-5.5, reasoning high; reviewed against d54d2d6b9b8a; fix evidence: release v2026.4.24, commit cbcfdf62c729. |
Summary
handleTerminalChatEventcallsloadChatHistoryafter the run completes (hadToolEvents && state === "final"). The server returns a history snapshot that is 1–2 messages behind the UI (disk write hasn't caught up), and without a guardstate.chatMessages = filteredoverwrites the in-memory state — wiping the optimistic user message and the assistant reply that were already displayed.ui/src/ui/controllers/chat.ts): guardloadChatHistoryso it only replaceschatMessageswhen the server returned at least as many non-system messages as the client currently holds. Covers both the tool-reload scenario and post-compaction reloads.ui/src/ui/controllers/chat.ts): don't clearchatStream/ reset tool stream insideloadChatHistorywhile a run is still active (chatRunIdis set), so streaming text isn't wiped before the final event arrives.ui/src/ui/app-gateway.ts): deferresetToolStreamuntil afterloadChatHistoryresolves when tool events were seen, so tool cards stay visible during the network round-trip.ui/src/ui/app-render.helpers.ts,ui/src/ui/app-render.ts): clearchatMessageson session switch so the guard'slength === 0fast path fires correctly on initial load after switching.Test plan
pnpm test -- ui/src/ui/controllers/chat.test.ts— 31 tests pass (3 new tests added)pnpm test -- ui/src/ui/app-gateway.sessions.node.test.ts— passespnpm check— cleanwillReplace: false— messages no longer disappear after send🤖 Generated with Claude Code