perf(tui): incremental syntect for streaming fenced code blocks - #575
Conversation
Layout re-renders the streaming assistant block on every flush, which re-highlighted every previous code line through a fresh HighlightLines. Keep highlighter state across pure appends of complete lines so only new lines pay syntect; incomplete last-line tails stay O(1) per flush.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 44e079c126
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let can_extend = cache.lang == lang | ||
| && cache.num_w == num_w | ||
| && complete.starts_with(&cache.content) | ||
| && (cache.hl.is_some() || cache.content.is_empty()); |
There was a problem hiding this comment.
When /theme previews or /color switches themes, theme_picker::apply_theme invalidates the layout specifically to repaint styled transcript lines, but this cache key considers only language, gutter width, and content. Re-rendering an unchanged fenced block therefore clones cache.body with the previous palette's gutter/background and keeps the previous dark/light syntect highlighter; switching from one-dark to solarized-light can leave unreadable dark-theme code colors and makes the existing highlighting_stays_legible_against_the_code_background test fail on its second iteration. Include the active theme/palette generation in the reset condition.
AGENTS.md reference: AGENTS.md:L31-L39
Useful? React with 👍 / 👎.
| thread_local! { | ||
| static CODE_HL_STREAM: RefCell<CodeHlStream> = const { | ||
| RefCell::new(CodeHlStream { | ||
| lang: String::new(), | ||
| content: String::new(), |
There was a problem hiding this comment.
When the last rendered response contains a large fenced block, this thread-local retains both the full source and a cloned styled body until another incompatible fenced block happens to render or the process exits. clear_transcript_view deliberately replaces the transcript and invalidates its layout so /clear releases large conversation allocations, but it cannot reach this static cache, so clearing or leaving such a session no longer frees all of that code-block memory. Add a lifecycle reset/eviction path rather than retaining the most recent block indefinitely.
Useful? React with 👍 / 👎.
Cached fenced-code lines bake in the syntect theme and code_bg of the paint that produced them. Switching light/dark left those spans in the thread-local cache, so the legibility test (and real theme swaps) saw no matching highlighted runs against the new code_bg.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce2cab8b7b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let mut cache = cell.borrow_mut(); | ||
| let can_extend = cache.lang == lang | ||
| && cache.num_w == num_w | ||
| && cache.dark == dark |
There was a problem hiding this comment.
When /theme previews or selects another theme with the same is_dark value (for example, One Dark to Dracula), the layout is invalidated and rerendered, but this predicate still accepts the cached body because it compares only polarity. Those cached spans contain the previous theme's accent, muted, and code_bg colors, so the code body retains the old palette while its header and the rest of the UI use the new one until the code changes; key the cache on the relevant palette values or a theme identity instead.
Useful? React with 👍 / 👎.
| self.spans_emitted = self | ||
| .spans_emitted | ||
| .saturating_add(body.len().saturating_mul(4)); |
There was a problem hiding this comment.
When markdown contains multiple syntax-dense fenced blocks whose lines produce more than four syntect runs, this estimate undercounts spans_emitted, so render_markdown continues accepting later blocks and events after the intended 20,000-span guard has effectively been exceeded. The previous implementation counted each emitted highlight range; calculate the actual number of spans in body rather than assuming four per line.
Useful? React with 👍 / 👎.
| content: String::new(), | ||
| body: Vec::new(), |
There was a problem hiding this comment.
After a large fenced block has been rendered, /clear and session replacement release the transcript and layout buffers, but this process-long thread-local remains an owner of both the complete source and the cloned styled body. No production path resets it unless another incompatible code block is later rendered, so memory that clear_transcript_view deliberately tries to release can remain allocated for the rest of the process; provide a production invalidation path or bound/evict this cache.
Useful? React with 👍 / 👎.
Export reset_code_hl_stream and call it from clear_transcript_view so fenced-block highlight memory is released with the transcript. Add a polarity-flip regression test for one-dark → solarized-light.
1 participant
Summary
HighlightLinesevery time (~10 Hz) — quadratic in block length.highlight_line.Part of #561 (D3-26).
Test plan
streaming_code_block_reuses_highlight_on_append