fix: scrollback duplicate when message streamed artificially mid-turn - #50
Conversation
…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>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughIn ChangesFix duplicate scrollback entry in artificiallyStreamText
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. Comment |
Codecov Report❌ Patch coverage is
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
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
… 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>
Summary
#artificiallyStreamTextcalled#persistAndBuffertwice for the same message: once at start (empty placeholder) and once at end (full content).scrollback.pushhas no deduplication — both entries were stored in the buffer.scrollback.replay(e.g. Telegram mini app opening, or any client reattaching), both entries were delivered andreplaceScrollbackstored them both in thebySessionarray, rendering the message twice in the UI.Fix: The scrollback buffer holds
msgby reference, so its content is already updated when the streaming loop finishes. Replace the second#persistAndBuffercall with#scrollback.updateMessage(recounts bytes in place, no second push) + a single transcript append.Root cause
On
scrollback.replay, client receives[empty_msg, full_msg]with same messageId →replaceScrollbackstores both → message renders twice.Test plan
#artificiallyStreamText🤖 Generated with Claude Code
Summary by CodeRabbit