Skip to content

fix: scrollback duplicate when message streamed artificially mid-turn - #50

Merged
saucam merged 1 commit into
mainfrom
fix/scrollback-duplicate-artificial-stream
Jun 30, 2026
Merged

fix: scrollback duplicate when message streamed artificially mid-turn#50
saucam merged 1 commit into
mainfrom
fix/scrollback-duplicate-artificial-stream

Conversation

@saucam

@saucam saucam commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • #artificiallyStreamText called #persistAndBuffer twice for the same message: once at start (empty placeholder) and once at end (full content).
  • scrollback.push has no deduplication — both entries were stored in the buffer.
  • On scrollback.replay (e.g. Telegram mini app opening, or any client reattaching), both entries were delivered and replaceScrollback stored them both in the bySession array, rendering the message twice in the UI.

Fix: The scrollback buffer holds msg by reference, so its content is already updated when the streaming loop finishes. Replace the second #persistAndBuffer call with #scrollback.updateMessage (recounts bytes in place, no second push) + a single transcript append.

Root cause

artificiallyStreamText:
  #persistAndBuffer(msg)   // push empty msg → scrollback entry 1
  ... stream deltas ...
  msg.content = fullContent
  #persistAndBuffer(msg)   // push full msg → scrollback entry 2 (SAME messageId!)

On scrollback.replay, client receives [empty_msg, full_msg] with same messageId → replaceScrollback stores both → message renders twice.

Test plan

  • Send a message mid-turn to trigger #artificiallyStreamText
  • Detach and reattach (or open the Telegram mini app fresh) — message should appear exactly once
  • Normal streaming turns unaffected

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • Prevented duplicate scrollback entries when an assistant message finishes streaming.
    • Final messages now update the existing chat entry in place and are saved consistently, reducing repeated or duplicated content in the conversation history.
    • Improved reliability of final message handling with clearer error logging during persistence.

…ndBuffer replaced with updateMessage

The second #persistAndBuffer call at the end of #artificiallyStreamText
was pushing a second scrollback entry for the same messageId. On
scrollback.replay the client received both entries and replaceScrollback
stored them both in the array, rendering the message twice.

The scrollback buffer holds the message object by reference, so content
is already correct after the streaming loop. Replace the second push
with #scrollback.updateMessage (just recounts bytes) + transcript append.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jun 30, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 0e575d62-9c57-4ad6-a7f6-2e2bca50f057

📥 Commits

Reviewing files that changed from the base of the PR and between 8505395 and c91ca10.

📒 Files selected for processing (1)
  • src/daemon/session.ts

📝 Walkthrough

Walkthrough

In #artificiallyStreamText, the final persistence step no longer calls #persistAndBuffer(msg). Instead, it updates the existing scrollback entry in-place, appends the finalized message directly to the transcript store with error logging, and notifies the chunker.

Changes

Fix duplicate scrollback entry in artificiallyStreamText

Layer / File(s) Summary
In-place scrollback update replaces persistAndBuffer
src/daemon/session.ts
Removes the #persistAndBuffer(msg) call that created a duplicate scrollback entry for the same messageId; replaces it with an in-place scrollback update, a direct transcript append with explicit error logging, and a chunker notification for the finalized assistant message.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~5 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the duplicate scrollback fix for artificially streamed mid-turn messages.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/scrollback-duplicate-artificial-stream

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration.


Comment @coderabbitai help to get the list of available commands.

@codecov

codecov Bot commented Jun 30, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 80.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 81.07%. Comparing base (8505395) to head (c91ca10).
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
src/daemon/session.ts 80.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #50      +/-   ##
==========================================
- Coverage   81.07%   81.07%   -0.01%     
==========================================
  Files          55       55              
  Lines        7593     7597       +4     
==========================================
+ Hits         6156     6159       +3     
- Misses       1437     1438       +1     
Flag Coverage Δ
daemon 81.07% <80.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/daemon/session.ts 70.21% <80.00%> (+0.01%) ⬆️
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@saucam
saucam merged commit b3cd125 into main Jun 30, 2026
5 checks passed
saucam added a commit that referenced this pull request Jul 2, 2026
… via upsert (#74)

* fix: streamed messages pushed into scrollback twice — commit finalize via upsert, not a second push

The #50 fix covered only #artificiallyStreamText. The main streaming path
still called #persistAndBuffer at BOTH stream start (text_delta creates the
message) and finalize (text_done), and the same double-push lived in
#flushActiveAssistant (interrupt/turn-boundary flush) and
#finalizeActiveThinking (every thinking block). Consequences:

- scrollback.replay carried two entries per streamed messageId; clients
  rendered the message twice and the web virtualizer's messageId-keyed
  caches collided (the residual cause of the 'intermittent message
  overlap' that #73 partially fixed)
- the memory chunker received the stream-start push with empty content,
  emitting a prompt-only user_turn episode and then a promptless
  assistant_turn — every plain turn fragmented into two half-episodes
- byte accounting drifted negative (push #1 accounted the empty size,
  eviction subtracted the grown size twice), permanently disabling the
  20MB scrollback cap

Fixes:

- ScrollbackBuffer now records the accounted size per entry and upserts
  by messageId: re-pushing a buffered id re-accounts the existing entry
  in place (keeping its replay position) instead of appending a
  duplicate. Eviction subtracts exactly what was added — negative drift
  is structurally impossible. updateMessage is O(1) via the id index
  (was a front-to-back scan).
- Session stream-start sites push to scrollback only; the new
  #commitStreamed emits the durable transcript row and the chunker event
  exactly once, at finalize, with final content. #artificiallyStreamText
  drops its bespoke reset-and-updateMessage dance for the same helper.
- #seq now seeds past the persisted transcript tail on resume instead of
  restarting at 0, making seq usable as a monotonic replay cursor.

Tests: session-stream-commit.test.ts pins one-scrollback-entry-per-
messageId across all four finalize paths (text_done, thinking_done,
batch-reply artificial streaming, turn-boundary flush), buffer upsert +
byte-cap accounting under by-reference growth, chunker episode pairing
(including a test documenting the pre-fix fragmentation), and seq
continuation after resume.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* fix: account scrollback bytes as UTF-8, add live-session chunker regression test

CodeRabbit review follow-ups on #74:
- String.length counts UTF-16 code units; use Buffer.byteLength so the
  20MB cap holds for non-ASCII payloads
- end-to-end test that a real Session + MemoryEngine ingests exactly one
  combined user+assistant episode per streamed turn (a stray stream-start
  chunker feed would fail it)

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* fix: StubEmbedder missing close() from the Embedder interface

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant