Skip to content

fix(desktop): preserve interim assistant text - #39576

Closed
sweetcornna wants to merge 2 commits into
NousResearch:mainfrom
sweetcornna:fix/39558-desktop-final-text
Closed

fix(desktop): preserve interim assistant text#39576
sweetcornna wants to merge 2 commits into
NousResearch:mainfrom
sweetcornna:fix/39558-desktop-final-text

Conversation

@sweetcornna

Copy link
Copy Markdown
Contributor

Fixes #39558.

Summary

  • preserve text parts that streamed before the last tool call when message.complete replaces the final assistant text
  • keep the existing behavior for normal no-tool completions by replacing all streamed text when no tool call was present
  • add a hook-level regression test for message.delta -> tool.start -> tool.complete -> message.complete

Red

  • npm run test:ui -- src/app/session/hooks/use-message-stream.test.tsx
    • failed with parts ['tool-call', 'text'] instead of ['text', 'tool-call', 'text']

Proof

  • npm run test:ui -- src/app/session/hooks/use-message-stream.test.tsx src/lib/chat-messages.test.ts
  • npx eslint src/app/session/hooks/use-message-stream.ts src/app/session/hooks/use-message-stream.test.tsx
  • npm run type-check
  • git diff --check (only Windows CRLF warning for apps/desktop/src/app/session/hooks/use-message-stream.ts)

@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have labels Jun 5, 2026
@sweetcornna
sweetcornna marked this pull request as ready for review June 5, 2026 05:31

Copy link
Copy Markdown
Contributor Author

Added follow-up ea40adcf1 after auditing a completion-shape edge case.

The previous fix preserved text before the last tool call, but if message.complete.text contains the full final assistant text rather than only the post-tool tail, Desktop could render duplicated pre-tool text, e.g. Planning. Planning. Done.. I added a RED regression for that shape and now trim already-rendered kept text from the completion before appending the final tail.

Fresh validation on this head:

npm run test:ui -- src/app/session/hooks/use-message-stream.test.tsx
# 2 passed

npm run test:ui -- src/app/session/hooks/use-message-stream.test.tsx src/lib/chat-messages.test.ts
# 2 files passed, 26 tests passed

npx eslint src/app/session/hooks/use-message-stream.ts src/app/session/hooks/use-message-stream.test.tsx
# passed

npm run type-check
# passed

git diff --check
# passed; Windows CRLF working-copy warnings only

The PR is still mergeable on GitHub after the push.

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the focused regression coverage. The reported behavior is still present on current main: apps/desktop/src/app/session/hooks/use-message-stream/index.ts:363-377 removes every streamed text part when a completion lands, while index.ts:294-328 preserves pre-tool text and tool ordering before that completion.

Problems

  • The PR edits the pre-refactor path apps/desktop/src/app/session/hooks/use-message-stream.ts; current main moved the stateful hook to use-message-stream/index.ts in 086343854 and then extracted event dispatch in 51a710e57.
  • The test harness is also stale: current MessageStreamOptions requires sessionStateByRuntimeIdRef at use-message-stream/index.ts:43-48.

Suggested changes

  • Port the retention/deduplication logic into current index.ts, operating on its existing visibleFinalText value at line 360 so generated-image filtering remains intact.
  • Update the regression test for the folder layout and current hook options; retain both completion shapes covered by the PR.

Automated hermes-sweeper review.

activeSessionIdRef,
hydrateFromStoredSession: async () => undefined,
queryClient: queryClientRef.current,
refreshHermesConfig: async () => undefined,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Current main's MessageStreamOptions requires sessionStateByRuntimeIdRef (apps/desktop/src/app/session/hooks/use-message-stream/index.ts:43-48), so this harness needs that ref when the test is ported to the current hook layout.

@teknium1 teknium1 added sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 14, 2026
@sweetcornna

Copy link
Copy Markdown
Contributor Author

Closing in favor of merged #65919, which supersedes this earlier attempt and preserves my contribution in the co-author credits. Thanks for consolidating the fix across the agent, gateway, Desktop, and TUI layers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Desktop: intermediate assistant text (emitted before a tool call) disappears from the rendered thread when the turn completes

3 participants