Skip to content

perf(web): completed Markdown blocks no longer render twice - #33794

Open
voltcrash wants to merge 1 commit into
pingdotgg:mainfrom
voltcrash:t3/markdown-cache-rerender
Open

voltcrash wants to merge 1 commit into
pingdotgg:mainfrom
voltcrash:t3/markdown-cache-rerender

Conversation

@voltcrash

@voltcrash voltcrash commented Nov 10, 2026 •

Copy link
Copy Markdown
Contributor

When a new paragraph arrives during streaming, the previous Markdown block renders again just because its cache prop changes from false to true. The existing app streaming scenario appends 30 paragraphs but renders MarkdownParagraph 59 times on main.

Parse independently of cache promotion so React Compiler can reuse the same element across cache-only updates, then put that committed element into the existing bounded shared cache. Finished blocks and completed replies still populate the cache for remounts. Existing cache tests now cover a growing block becoming finished, stream completion with stable extra plugins, and plugin changes.

This is a small, focused fix for an obvious redundant-render defect under CONTRIBUTING's focused exception; no maintainer approval is claimed. Web and desktop share this renderer. Mobile has a separate renderer. No visible markup, styling, contracts, or provider behavior changes.

Overlap rechecked before implementation: #27866 changes parsing/islands but retains this cache prop and default memo comparison; #28991 adds imperative rendering for some live rows but retains the affected React fallback. Neither current diff fixes this cache-only transition. This PR changes only cache promotion, with no renderer rewrite.

Proof on Linux/headless Chromium, reproduced against b48b8ea2a0019b3d38de09406fe8be820dd8df15. Rebased cleanly onto bcee0f5d2872ba193c66ed9ba979bbe424f89ff9 before submission; the intervening commits only change server Claude startup and usage pricing, and git range-diff confirms this patch is unchanged:

  • From apps/web: T3CODE_RENDER_REPORT_DIR=/tmp/markdown-renders vp test run --project renders test/appScenarios.renders.tsx -t 'warm up|app-stream-assistant-turn' (create the report directory first). Before: fails with expected 59 to be 30. After: 30 paragraph renders for the same 30 updates. The assertion was not changed.
  • T3CODE_RENDER_REPORT_DIR=/tmp/markdown-renders vp test run --project renders test/appScenarios.renders.tsx: all 11 app scenarios pass after the fix.
  • Both extended cache browser tests fail with the original implementation: unchanged blocks parse twice instead of once. They pass with the fix; the full ChatMarkdown.browser.test.tsx suite passes (20 tests).
  • From repo root: vp run check:changed passes formatting, lint, both knip checks, affected typechecks (2 packages), 40 adjacent tests, and compiler coverage (2 files).

These are render-count measurements, not a wall-clock speedup claim. The primary integrated the three independent performance patches locally and ran a real Replay turn through the server, adapter and web client against isolated synthetic state. Paragraphs, list, code and table rendered; switching to another thread and back retained code and table content with no alert. Plugin changes are covered by the extended browser tests; line-break/math UI toggles and packaged Electron were not separately exercised.

Before appearance · After appearance · Replay streaming recording · Runtime method and limits. The before capture uses the baseline built client; after uses the integrated dev client, so these images do not establish latency or pixel-identical layout.

Macroscope correctness passed without findings. Approvability requires maintainer review because the new reasoned t3code/no-unreasoned-state-sync suppression falls under .macroscope/approvability.md; no automatic eligibility approval is claimed.

Reviewed with review-loop: 1 round with Claude Opus 5.5. Reviewer verdict: approved.

Created with GPT-6.1 Sol (extra-high reasoning) in the Codex harness through T3 Code.

@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:S 10-29 changed lines (additions + deletions). labels Nov 10, 2026
@macroscopeapp

macroscopeapp Bot commented Nov 10, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Needs Human Approval

Macroscope's review found this PR requires human approval — The change avoids redundant Markdown rendering by caching committed elements after rendering. It also adds a line-level lint suppression for the new effect, which requires human review.

You can add or adjust custom eligibility rules. Learn more.

@coderabbitai

coderabbitai Bot commented Nov 10, 2026 •

Copy link
Copy Markdown
Contributor

Explain Change →

No actionable comments were generated in the recent review.

Walkthrough

Markdown rendering now separates cache lookup from cache insertion. The renderer exposes a remember method, and MarkdownSource calls it after rendering when shared caching is enabled. Browser tests cover streaming updates and plugin replacement.

Changes: Markdown cache behavior

Layer / File(s) Summary
Renderer cache insertion method
apps/web/src/components/chat/markdownRenderer.ts
The renderer returns a render function with a remember method. It inserts the supplied element only when the renderer-specific cache key is not already present.
Post-render cache insertion and coverage
apps/web/src/components/ChatMarkdown.tsx, apps/web/src/components/ChatMarkdown.browser.test.tsx
MarkdownSource renders with cache lookup disabled, then remembers the element when shared and cache are true. Browser tests start with a partial message before rerendering the complete message, and check that a replacement plugin runs.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related issues

  • pingdotgg/t3code#9661: The issue tracks repeated work during streaming Markdown rendering. This PR changes cache insertion for completed renders, which relates to that performance objective.

Suggested reviewers: juliusmarminge, macroscopeapp

Merge Risk: Minimal · up to 684c3

No merge-blocking issue is established. The pending integrated replay remains a validation step, not evidence of a failure.

Pre-merge checks: Passed 4

Check name Status Explanation
Title check Passed The title clearly identifies the Markdown rendering change and is relevant to the primary objective.
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.
Description check Passed The description covers the problem, change, focused scope and approval rationale, and verification results. It includes before-and-after screenshots and a recording, and identifies the model and harne…
Full details: Description check

Explanation

The description covers the problem, change, focused scope and approval rationale, and verification results. It includes before-and-after screenshots and a recording, and identifies the model and harness used. The required information is present even though the template headings are omitted.

Finishing Touches

Generate unit tests (beta)

  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Recent review info

Run configuration

Commits

Reviewing files that changed from the base of the PR and between bcee0f5 and 684c3ab.

Files selected for processing: 3

  • apps/web/src/components/ChatMarkdown.browser.test.tsx
  • apps/web/src/components/ChatMarkdown.tsx
  • apps/web/src/components/chat/markdownRenderer.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:S 10-29 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant