perf(ui): incrementally prepare streamed Markdown math - #4207
Conversation
Move math source preparation behind Astryx streaming and reuse only safe transformed prefixes while preserving the existing incremental parser and scheduler. Generated-by: Codex
860892c to
a217481
Compare
jackwener
left a comment
There was a problem hiding this comment.
Reviewed the latest head a2174817f. No P0 or P1 — approving. Two P3 notes are left inline.
The incremental design holds up. The safety of only re-scanning the tail rests on canMarkSafe latching off permanently at any unterminated construct — an unclosed fence, an unclosed backtick run, a pending delimiter, or a trailing \/$/` at end of input. Because that flag never resets within a scan, the safe boundary can never advance past a point whose interpretation a later append could change, so re-entering at that boundary with only "am I at a line start?" as carried context is sufficient. I checked each of those five branches individually.
Coverage is real rather than incidental. ${settled}growing → ${settled}growing live tail is a genuine prefix extension with math sitting in the settled portion, and it asserts the rendered math node keeps its identity across the two renders; the formula-rewrite test covers the cache-reset branch; and the unterminated fence, backtick, and delimiter cases each have behavioural tests.
One note on gating: test was still running when I reviewed, with label and windows_recovery green. The required checks still gate the merge, so this approval does not let anything through ahead of them.
Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.
| // Reset incremental cache when the autolink option toggles — cached | ||
| // settled blocks were parsed with the previous setting. | ||
| @@ -1139,17 +1142,25 @@ export function Markdown({ | ||
| - // Reset incremental cache when the autolink option toggles — cached |
There was a problem hiding this comment.
P3 — this silently drops a documented invariant guard.
The patch now removes Astryx's autolink-toggle cache reset. That reset existed for a stated reason: settled blocks already in the incremental cache were parsed under the previous autolink setting, so toggling it without invalidating them leaves stale parses on screen.
It is safe today only because of a single fact outside this file — markdown-body.tsx:161 passes autolink="gfm" as a hardcoded literal, and there is no other call site, so the flag cannot change at runtime and the reset was dead code here.
What I would ask for is a line in the PR body or the patch saying that, because the removal is currently unexplained. As it stands, the comment explaining why the guard existed is deleted along with the guard, so whoever later makes autolink dynamic gets a stale-cache bug with nothing left to warn them.
| text += run; | ||
| index = runEnd; | ||
| atLineStart = false; | ||
| canMarkSafe = false; |
There was a problem hiding this comment.
P3 — the one coverage combination that is missing.
Each half of this is tested: the unterminated cases (unclosed fence, unmatched backtick, pending delimiter) have behavioural tests, and the append path is exercised by the growing-tail streaming test.
What no test covers is the interaction — an append that closes a construct which was unterminated in the previous render. That is exactly the case this latch exists for: the boundary must have stayed behind the opener so the closer is re-scanned in context.
A cheap direct test would prepare a \b(unclosed backtick), then preparea `b` c, and assert the result equals a single one-shot preparation of the full string. The same shape for (` and for a fence would pin the invariant at the unit level instead of relying on it holding through the rendered output.
zhiiw
left a comment
There was a problem hiding this comment.
Reviewed at exact head a2174817 (verified unchanged at review time; test 25m, windows_recovery, and label all completed/success on this head).
Incremental math preparation, verified rather than assumed:
- The incremental cache boundary is conservative in the right places: the safe frontier only advances past closed constructs — closed fences, closed backtick runs, fully matched math tokens, and ordinary characters that cannot begin a delimiter (
\,$,`at the tail defer the mark). An unmatched backtick suppresses marking for the rest of that pass, so a later)can never retroactively change how a settled prefix tokenizes. Line-start anchoring for fence detection is carried across the cut viasource[sourceStart - 1] === '\n', and any non-prefix rewrite resets the whole cache. - The fixed token namespace is collision-safe by construction: literal
\uE000MAKA_MATH:…\uE001text in the source is re-encoded as a kind-2literal token that renders as itself (the old dynamic-namespace loop existed for exactly this; the new scheme answers it structurally instead of by probing). Partial transport sequences at the stream tail are treated as pending and copied verbatim — the new tests pin that transport syntax never becomes visible across the display cursor, including through a formula rewrite. - One path, honestly bounded: the transform now runs on Astryx's smoothed display text immediately before its incremental parser (
transformSourceseam in the patch), with rawsettledTexton both sides of the cursor check, so host and renderer compare the same strings. The PR claims the math-preparation CPU reduction (85%/84%, 7/7 paired wins) and explicitly does not claim a whole-renderer latency win — the tables back exactly that claim. - Noted, not blocking: the patch update drops the previous
autolink-toggle reset of the incremental parser cache. In this codebaseautolink="gfm"is a constant prop at the only call site, so the removed guard is unreachable here; worth remembering if the prop ever becomes dynamic.
Executed on a real Windows machine at this head: re-applied the updated Astryx patch (deleted the stale patched package and reinstalled — the build catches this correctly, since transformSource is a new prop), clean forced rebuild, then the markdown-body + streaming-text suites 40/40 — including the new adversarial cases (literal token as prose, malformed prefix recovery, unmatched backtick/delimiter recovery, display math across Markdown-looking block boundaries, transport syntax never exposed mid-stream, formula rewrite).
Automated review notice: This comment was posted by an automated review agent operated by zhiiw. It is not an independent human review and does not replace one.
简体中文
增量数学预处理,全部核实而非假设:安全前沿只在闭合构造后推进(闭合围栏/反引号/完整数学 token/不可能起始定界符的普通字符),未配对反引号会抑制本趟标记,尾部 \ $ ` 延迟标记——settled 前缀不会因后续增长改变切分;行首锚定跨切口正确传递;非前缀重写整体重置。固定命名空间用 kind-2 字面量重编码结构性解决碰撞(替代旧的动态探测);流尾半个传输序列按 pending 原样透传,新测试钉住传输语法永不越出显示光标。transformSource 缝让变换作用于 Astryx 平滑后的显示文本、紧贴其增量解析器前,settledText 两侧都是原始串。PR 只声称 math CPU 下降(85%/84%、7/7),不声称端到端延迟收益——表格与声明严格一致。注意项(不阻断):patch 删掉了 autolink 切换时重置增量缓存的守卫,本库唯一调用点 autolink 是常量字面量,不可达。本机真 Windows:重打 patch 后干净重建,markdown-body + streaming-text 40/40(含全部新对抗用例)。
M4n5ter
left a comment
There was a problem hiding this comment.
Approved on exact head a2174817fc45bff7bb88c8921bdf229da4fb3003: I found no P0 or P1. The incremental preparation matches one-shot preparation across ordinary growth, rewrites, shrink/restore, math, literal transport syntax, and complete backtick-fence paths. One non-blocking P2 remains inline for a tilde fence whose opener is split across displayed provider chunks.
The exact-head test, windows_recovery, and label checks are green. Locally, the complete UI suite passed 286/286, the affected builds and Biome check passed, and the conflict-free current-main merge tree does not alter any reviewed file.
Automated review notice: This comment was posted by an automated review agent operated by M4n5ter. It is not an independent human review and does not replace one.
| index < source.length || | ||
| (character !== '\\' && character !== '$' && character !== '`') | ||
| ) { | ||
| markSafe(); |
There was a problem hiding this comment.
[P2] Keep a partial line-start tilde fence behind the safe boundary
A normal streaming split can leave ~~ as the currently displayed source before the third ~ arrives. This branch marks both tildes safe because only trailing \, $, and backticks are held back. The next preparation therefore resumes after ~~ with startsAtLineStart=false, never recognizes the eventual ~~~ opener, and encodes math-looking text inside the code block as an internal transport token.
I reproduced this through the production MarkdownBody and Astryx path: render ~~ as the settled first target, then stream ~~~ts\n\\(not math\\)\n~~~, advance the real animation frames, and finally render the same text non-streaming. The code block still contains \uE000MAKA_MATH:0:6e6f74206d617468\uE001 instead of \(not math\), so the corruption survives the final flush. Existing fence tests provide the whole opener in one input and cannot exercise this boundary.
Please keep a one- or two-character line-start ~ run behind the safe boundary until a third tilde or a non-tilde decides whether it is a fence, and add this split-opener plus final-flush regression.
Automated review notice: This comment was posted by an automated review agent operated by M4n5ter. It is not an independent human review and does not replace one.
ARE404
left a comment
There was a problem hiding this comment.
Approving perf(ui): incrementally prepare streamed Markdown math at head a2174817.
I reviewed the incremental prepareMarkdownMath/protectMarkdownMath redesign and its tests, and verified the boundary-safety of the cache:
- Only fully closed constructs (closed fences, matched literal tokens, closed delimiters) advance
safeSourceEnd/safeTextEnd; any unterminated/pending construct leaves the tail unmarked, so the incremental re-parse always operates on the unsafe tail and reuses only provably-settled prefix. A math token still mid-stream is never cached as safe, so it is re-parsed fresh until it closes — the incremental slicing does not corrupt a token that straddles the boundary. - The
startsWithprefix check forces a full re-parse on any rewrite rather than reusing a stale prefix, so a mid-stream edit cannot reuse wrong cached state. testis green on this exact head (run33270540247, 25m01s). Two non-blocking [P3] review threads remain open (the Astryx autolink-toggle cache reset removal in the dependency patch, and a missing boundary-coverage combination); they do not block, but the author should still reconcile them.
No P0–P2.
简体中文
批准 perf(ui): incrementally prepare streamed Markdown math,head a2174817。
审查增量 prepareMarkdownMath/protectMarkdownMath 重设计与测试,核实缓存的边界安全:只有完全闭合的构造(闭合围栏、匹配的 literal token、闭合分隔符)才会推进 safeSourceEnd/safeTextEnd;任何未闭合/pending 构造会让尾段保持不安全,于是增量重解析始终只作用于不安全尾段、复用确定沉淀的前缀。还在流式中间态的 math token 永不标记为安全,直到闭合才重新解析——跨边界的 token 不会被增量切片破坏。startsWith 前缀检查保证任何改写都触发全量重解析、不复用过期前缀。test 在 exact head 绿(run 33270540247,25m01s)。两条非阻塞 [P3] 线程仍开着(依赖补丁里移除 Astryx autolink 缓存重置、以及一处边界覆盖缺失),不构成阻塞但建议作者另行统一。无 P0–P2。
Keep incomplete line-start fence markers behind the math cache boundary so later streaming chunks cannot turn code into math transport tokens. Generated-by: Codex
Summary
Streaming math previously transformed both the full Markdown source and
settledTexton every React render, before Astryx applied its own streaming cursor and incremental block parser. This change keeps Runtime Host text authoritative, lets Astryx smooth the raw text first, and incrementally prepares only the parser-facing math tail from the last safe boundary.The implementation remains one path:
MarkdownBody → useStreamingText → math source transform → Astryx incremental parser. It removes the dynamic token namespace, two per-render token maps, and the separate transformedsettledTextpath. It does not touch Desktop transcript range, virtualization, scroll ownership, or content visibility.Results
A/B method and limits
A is baseline
bb1256241eddb2384715f4131fb16c7599f92a8d; B is the product code infffe591a2203b60019a499be9912112543c01ba5. Both modes ran in the same minified production React/Markdown fixture bundle (sha256:d790dff180e4baa9054eb4873033e3bc28c27caf8c9f063eebcc71111bfd805a; recorded resultsha256:bdc6cab7cdd08ac0ca1ade74963f93b8320fcba88e573e41f018bb8a18908bea). The 64 KiB and 256 KiB fixtures each exercised ordinary Markdown, inline/display math, a long growing code fence, settled math plus a live tail, and final non-streaming flush. Seven interleaved paired runs alternated A/B and size order. CDP and the React hook were read-only; the temporary mode switch only selected A or B in the throwaway measurement bundle.The harness is intentionally not published with the product change: doing so would retain a benchmark-only product mode and a second fixture protocol. The recorded temporary command was:
./node_modules/.bin/esbuild /private/tmp/pr4207-perf-entry.tsx --bundle --minify --keep-names --platform=browser --format=iife --define:process.env.NODE_ENV='"production"' --outfile=/private/tmp/pr4207-perf-bundle.js PR4207_PERF_PAIRS=7 PR4207_PERF_OUTPUT=/private/tmp/pr4207-final-ab-v3.json node /private/tmp/pr4207-perf-runner.mjsThese paths are not present in a fresh checkout. This is a bare minified React/Markdown composition fixture, not a Desktop or Runtime Host E2E. The rendered-frame metric waits two
requestAnimationFramecallbacks and is not labeled literal next paint.math preparation CPUis direct transform instrumentation;Renderer busy CPU, GC, heap, long tasks, and LoAF come from CDP. The current full-string input contract still requiressource.startsWith(previousSource)to detect rewrites, so this removes repeated full math lexing and the secondsettledTextpass, but does not claim that every source identity check is tail-only.Raw 7 paired groups (14 size rows)
Verification
Coverage includes empty formula fallback; URL/email/Markdown markers inside math; display math across blank, heading, list, and table-looking lines; inline/fenced code isolation including split fence openers through final flush; literal and malformed transport syntax; rewritten/restored text; and real client rerenders that preserve settled KaTeX DOM identity through a growing tail and final flush. A fresh install reapplies the Astryx patch, and a source-condition bundle contains the same
transformSourceseam as the default dist build.Entropy ledger
MarkdownBodymountuseStreamingText; one incremental parser; existing observersThe required additions are the post-scheduler/pre-parser transform seam, the safe source/text boundary pair, and collision-safe literal transport. They replace the old double full-source transform and map registry in this diff; no second store, parser, scheduler, protocol, observer, or transcript authority is added. The cumulative diff is +624/-178; the added volume is primarily the incremental lexer, behavior tests, and duplicated source/dist dependency patch, while owner and representation counts decrease as shown above.
AI use
Tool(s) and scope: Codex diagnosed the repeated preprocessing path, implemented the owner-aligned transform and incremental lexer, added tests, ran the production A/B, and prepared this PR. The commits carry
Generated-by: Codextrailers.Checklist
Does this PR entail a change in behavior?