fix: stop re-parsing the whole markdown message on every streaming delta - #98
Conversation
MarkdownBlock handed the entire accumulated content to SolidMarkdown per delta — a full unified() re-parse plus a DOM subtree rebuild per chunk, O(L²) over a stream. Long answers visibly stuttered the tab. While streaming: (1) deltas are coalesced to one renderer update per animation frame; (2) the text is split at safe block boundaries (blank lines and fence-opens outside code fences) so completed blocks parse once — <For> keys them by string identity — and only the small live tail re-parses, with renderingStrategy="reconcile" so its DOM updates in place. When streaming ends the message renders as one document again, so the final output is byte-identical to the non-streaming path. Simulated 20 KB answer streamed in 50 B deltas: total parse work drops 6972 ms → 279 ms (25x) — before counting the rAF coalescing (which divides per-event work by the delta:frame ratio) and the avoided per-delta DOM rebuilds. Fixes #87 Co-Authored-By: Claude Fable 5 <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 (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR adds streaming markdown helpers for frame-throttled updates and fence-aware block splitting, tests both utilities, and updates ChangesStreaming Markdown Rendering
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Msg as msg.content
participant MarkdownBlock
participant FrameThrottle as createFrameThrottled
participant Splitter as splitStreamingBlocks
participant Md
Msg->>MarkdownBlock: content delta
MarkdownBlock->>FrameThrottle: throttle text updates
FrameThrottle->>Splitter: throttled text
Splitter-->>MarkdownBlock: blocks and tail
MarkdownBlock->>Md: render completed blocks (memo)
MarkdownBlock->>Md: render tail (reconcile)
Related issues: Suggested labels: performance, frontend Suggested reviewers: saucam 🚥 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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #98 +/- ##
=======================================
Coverage 73.06% 73.06%
=======================================
Files 65 65
Lines 11032 11032
=======================================
Hits 8060 8060
Misses 2972 2972
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
web/src/components/transcript/MessageRow.tsx (1)
136-136: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPrefer
<Index>forsegments().blocks. The list is append-only, and repeated markdown blocks can still produce duplicate string values, which makes keyedForreconciliation ambiguous.Indexkeeps completed blocks stable and avoids that pitfall.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/components/transcript/MessageRow.tsx` at line 136, The rendering of segments().blocks in MessageRow should use an Index-based loop instead of For, since the blocks list is append-only and may contain duplicate markdown strings that make keyed reconciliation ambiguous. Update the JSX in MessageRow so each block is rendered through Index with Md, keeping completed blocks stable while preserving the existing display behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web/src/lib/streaming-markdown.ts`:
- Around line 69-99: The fence handling in splitStreamingBlocks is too
permissive because FENCE_LINE.test(line) toggles on any fence-like line, which
can incorrectly close an open code block. Update splitStreamingBlocks to record
the current opener’s fence character and length when entering a fence, and only
treat a later line as the matching closer if it uses the same character, has at
least the same length, and contains no non-whitespace after the fence. Keep the
conservative blank-run splitting behavior unchanged.
---
Nitpick comments:
In `@web/src/components/transcript/MessageRow.tsx`:
- Line 136: The rendering of segments().blocks in MessageRow should use an
Index-based loop instead of For, since the blocks list is append-only and may
contain duplicate markdown strings that make keyed reconciliation ambiguous.
Update the JSX in MessageRow so each block is rendered through Index with Md,
keeping completed blocks stable while preserving the existing display behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: aa214861-ce79-47fa-ad98-f1384f0fe8af
📒 Files selected for processing (3)
web/src/components/transcript/MessageRow.tsxweb/src/lib/streaming-markdown.test.tsweb/src/lib/streaming-markdown.ts
CodeRabbit review on #98: a closing fence must use the opener's char, be at least as long, and carry no trailing text (CommonMark) — the naive toggle let a literal fence-like line inside a block close it early and split mid-fence. Also swap <For> for <Index> on the completed-blocks list: it's append-only with possibly-duplicate strings, so positional keying is unambiguous and keeps blocks stable. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Problem (#87)
MarkdownBlockhanded the entire accumulatedmsg.contenttoSolidMarkdownon every streaming delta. Each chunk re-ran a fullunified()markdown parse and (with the defaultrenderingStrategy: "memo") rebuilt the message's rendered DOM subtree — O(L²) parse cost over a stream plus per-chunk DOM churn. A 10–20 KB answer streaming at 20–40 deltas/s visibly stuttered the tab, worse the longer it ran.Fix — three layers, streaming-only
New
web/src/lib/streaming-markdown.ts:createFrameThrottledmirrors the content signal but, while streaming, batches updates to at most one per animation frame (latest value wins). Inactive → synchronous passthrough.splitStreamingBlockssplits the accumulated text at safe boundaries: blank lines outside fenced code blocks, plus fence-opens (so a long streaming code block doesn't drag the finished paragraph before it into every re-parse). Completed blocks render as their ownSolidMarkdowndocuments inside<For>, which keys by string identity — a completed block never re-parses. Only the small live tail re-parses per frame.renderingStrategy="reconcile"so its per-frame re-render diffs the DOM instead of rebuilding it.When streaming ends, the message renders as ONE document again — the streaming segmentation is transient, so the final DOM is byte-identical to today's non-streaming path (plan-mode markdown and historical messages are untouched). That containment is what makes the conservative splitter safe: constructs spanning blank lines (reference-link definitions, loose lists) can render slightly differently across a boundary while text is still arriving, and self-heal on completion.
Micro-bench
20 KB assistant answer (paragraphs + GFM tables + code fences) streamed in 50 B deltas, same unified pipeline solid-markdown runs:
That 25× counts every delta as a parse event; in a real tab the rAF throttle further divides the tail parses by the delta:frame ratio, and the avoided full-subtree DOM rebuilds (the other half of the stutter) come on top.
Tests
streaming-markdown.test.ts(vitest): paragraph splitting + tail retention, reassembly (no line lost), never splitting inside a fence, unclosed streaming fence stays in tail,~~~/indented fences, no-boundary and empty inputs; throttle passthrough when inactive, single scheduled frame for a burst of updates, latest-value flush, and immediate flush on deactivation (manual rAF queue, assertions outsidecreateRootsince writes inside the root body are batched).tsc -b✓ · eslint: 20 warnings, identical set tomain(none in changed files) ✓ ·vitest run160/160 ✓Note: web coverage is not uploaded to codecov, so
codecov/patchis unaffected by this PR.Fixes #87
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests