Skip to content

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

Closed
superposition wants to merge 1 commit into
NousResearch:mainfrom
superposition:fix/desktop-preserve-interim-assistant
Closed

fix(desktop): preserve interim assistant text#41049
superposition wants to merge 1 commit into
NousResearch:mainfrom
superposition:fix/desktop-preserve-interim-assistant

Conversation

@superposition

Copy link
Copy Markdown
Contributor

Fixes #40903

Summary

  • preserve earlier assistant text parts when a streamed turn completes after tool calls
  • replace only the current assistant text segment instead of dropping every text part in the live bubble
  • add regression coverage for final-segment and full-stream completion payloads

Test plan

  • npm run test:ui -- src/lib/chat-messages.test.ts
  • npm run type-check
  • npx eslint src/lib/chat-messages.ts src/lib/chat-messages.test.ts src/app/session/hooks/use-message-stream.ts
  • git diff --check

Note: full npm run lint currently reports unrelated existing lint errors outside the touched files.

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review Summary

Verdict: Approved

Refactors assistant text finalization into a shared helper and removes an inline dedupe branch.

What changed

  • Extracted finalizeAssistantTextParts from inline logic in use-message-stream.ts into chat-messages.ts
  • Tests cover: partial text before tool calls, duplication when completion text equals streamed text

Looks Good

  • Shared helper is a clean DRY improvement
  • Dedupe logic is now unit-testable in isolation
  • No behavior change beyond cleanup — low risk

Minor Note

  • The normalizeTextForDedupe whitespace collapse (/\s+/g → ' ') is intentionally aggressive. If future tool-parts include meaningful whitespace formatting, this may need revisiting. Not a blocker here.

Reviewed by Hermes Agent

@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have labels Jun 7, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for isolating the text-part finalization behavior. The underlying Desktop defect still exists on current main: apps/desktop/src/app/session/hooks/use-message-stream/index.ts:363-377 removes every live text part and replaces them with only the terminal completion payload.

Problems

  • The PR edits the pre-refactor use-message-stream.ts; current main moved this code to apps/desktop/src/app/session/hooks/use-message-stream/index.ts in 086343854, and GitHub reports this PR as conflicting.
  • A port must preserve current generated-image handling at index.ts:360 (stripGeneratedImageEchoes(...)), which was added after this PR's base.
  • The new tests cover the helper only, not the live message.delta → tool boundary → message.complete path dispatched at gateway-event.ts:313-335.

Suggested changes

  • Port the segment-preserving merge into current index.ts while retaining generated-image echo stripping.
  • Add an event-path regression test with interleaved text/tool parts and a final-segment completion payload.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 14, 2026
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-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages 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.

Hermes Desktop drops intermediate assistant messages in multi-message turns (v0.16.0)

4 participants