Review 5515
#5515 — perf(markdown): bound incremental parsing to stream tail
#5515 perf(markdown): bound incremental parsing to stream tail by jiunshinn (bucket: collaborator)
Verdict: request changes (drafted; no public action authorized)
c2b2d12eb1094c50ad1def3b20a1b7ba34e91713 — every claim below was verified at this commit. Parent 41494fcd41310c819f0501df2e798235c4b41135.
LOOP VERSION: 1.4.1
AUDIT RUBRIC: 1.12
The task named loop 1.4.0; the authoritative Current file had advanced to 1.4.1 before gate 1, changing only the self-contained execution rule.
LANE: full WHY: this changes a published parser's state contract and result identity, the Markdown render path, and about 320 runtime lines. Correctness depends on stream history, document-global definitions, and tail boundaries, so one decisive check cannot settle it.
Bucket: collaborator. jiunshinn is a recurring collaborator, not an ENGOWNER or DESIGNOWNER. The same correctness bar applies; no outside-contributor Discord clause is needed.
Description → test plan → code. The body clearly names the four cumulative costs, the tail-bounded invariant, the global-definition exception, deterministic operation counts, and benchmark context. Its test plan is relevant and unusually strong for the append-only path. Code review followed that framing, then exercised omitted replacement and retained-result paths.
Prior review: none. The PR has no submitted reviews or inline review comments.
Main did not invalidate the mechanism. Current origin/main is 09d191cd9f0abc4f6923d62bfeceb6dc4d4e299e. Post-fork Markdown changes harden image URLs and forward BaseProps; they do not touch incremental state ownership. git merge-tree --write-tree origin/main HEAD exits 0.
Step-0 safety. The exact fork diff is one changeset plus source/tests under packages/core/src/Markdown; no package scripts, dependencies, lockfile, workflows, executable scripts, environment reads, or build-time network calls changed. The worktree was created before the exact gh pr diff --name-only check (an ordering miss); no PR code was installed or executed until the safety gate passed.
A long append-only Markdown stream repeatedly split, scanned, and assembled its already-settled prefix, making cumulative work quadratic. The linked #5406 measures 1.2 seconds of distributed parsing at 1,000 paragraphs and asks for deterministic counts rather than a clock budget.
VERDICT: clear
The parser keeps the settled character boundary, definitions, settled blocks, and one result array in state-owned cache data. Each append scans and parses only the mutable suffix, promotes newly settled definitions and blocks, and replaces only the result suffix; document-global definition changes intentionally reparse settled blocks. Markdown rekeys heading IDs and fade boundaries from streamed text rather than the now-stable array identity.
SOLUTION (4 decisions · ~320 runtime lines of 462 changed)
- Persist tail boundary and definition layers per incremental state.
- Append settled blocks while preserving the immutable object prefix.
- Mutate one returned result array's suffix instead of assembling a new full array.
- Adapt Markdown's heading-ID and fade-boundary derivations to that mutable result.
Every decision serves the stated performance problem, but decisions 2–3 narrow the published parser's prior behavior.
VERDICT: BLOCKS — append-only ownership leaks into existing replacement and result-retention behavior
OWNER: the incremental parser state; Markdown owns creating and resetting that state for its render stream
TIER 1: the existing parser and useStreamingText; no new subsystem or dependency
TIER 2: none
SEAMS: append-only chunks, mutable-tail rewrites, non-empty document replacement, empty reset, source-range/autolink flips, global definitions, public callers retaining prior AST results
BEHAVIOR UNIT: pure parser utility — focused state-history tests are the right boundary
| seam | driven result |
|---|---|
| seven adversarial documents × four option sets, every character boundary | 28/28 cases match a fresh-state oracle at every prefix |
| seven documents, chunk sizes 1–31 | 7/7 final ASTs equal a full parse, independent of chunk size |
| fences, loose lists, tables, blockquotes, duplicate headings, CRLF, unsafe/duplicate definitions | pass, including source ranges and autolink |
| empty reset; replacement shorter than settled prefix; mutable-tail rewrite | pass |
| same-length or longer replacement of settled text | fail: old settled heading/body survive and may be joined with a slice of the replacement |
| retain a prior public return value across the next call | fail: the earlier AST array changes in place |
| exact parent on the three failing probes | 3/3 pass; exact head 0/3 |
The fast owner is correct for an append-only stream. The missing seam is the public component/function boundary: neither the function signature nor <Markdown isStreaming> enforces that callers clear or replace state before a non-empty document replacement.
VERDICT: BLOCKS — documented public seams can produce stale content or mutable historical results
Append-only streams get the intended linear work and preserve syntax across every adversarial boundary tested. A caller that replaces a non-empty stream without first rendering an empty value instead sees the previous settled heading/body mixed into the new document; the real Markdown component reproduced this in Chromium. A parser caller retaining an earlier AST sees that value silently change after a later call.
VERDICT: BLOCKS — users can see content from the previous document; builders can observe prior parse results change
const state = createIncrementalState();
const first = parseMarkdownIncremental('# Alpha\n\nOld body\n\nTail', state);
parseMarkdownIncremental('# Bravo\n\nNew body\n\nTail', state);
// This head still returns Alpha/Old body, and `first` has also changed.| change | public? | class | doc'd? | verdict | |
|---|---|---|---|---|---|
~ |
an IncrementalParseState may no longer survive settled-prefix replacement unless the caller creates a new state |
yes — @astryxdesign/core/Markdown
|
existing state-reuse behavior | new source comment only | finding |
~ |
every returned BlockNode[] is now the same mutable array, updated by the next call |
yes — same public subpath | existing return-value behavior | new source comment only | finding |
OSSIFICATION: there is no new signature, but both behavior changes are observable on the released public parser. The parent explicitly reparsed when content before the boundary changed and returned a newly assembled array. If the mutable result remains public, removing it later changes identity again; retaining it requires a [breaking] changeset rather than the current [perf] patch note. Keeping a private mutable path for Markdown would avoid changing public parser semantics.
VERDICT: BLOCKS — published behavior changes without compatible handling or breaking classification
No theme target, token, CSS variable, StyleX declaration, painted element, or theme contract changes.
VERDICT: clear
BEHAVIOR: yes — settled-prefix replacement no longer reparses, and prior return values mutate on later calls API: yes, behaviorally — the published parser/state pair has a narrower reuse contract and different return identity; signatures are unchanged VISUAL: yes on the replacement path — the old document remains visibly rendered; ordinary append-only completed output is unchanged THEME: no — no theme surface changes
VERDICT: BLOCKS — the current patch changes released parser/Markdown behavior
EFFECTS: zero added or moved React Effects RENDER: the Markdown component still rerenders per smoothed-text tick; the parser preserves settled block identities and bounds result suffix replacement LISTENERS/OBSERVERS: zero LAYOUT: no new reads, writes, observers, or reflow mechanism BUNDLE: no dependency change; one WeakMap cache and work-counter function are added
Both exact implementations ran in one A/B command. Parent counts are the actual cumulative characters/lines handed to its whole-input split, boundary scan, definition scan, and returned-array assembly; head counts come from the new deterministic counters.
| sections | split chars parent → head | boundary lines parent → head | definition chars parent → head | result entries parent → head |
|---|---|---|---|---|
| 20 | 14,490 → 1,418 | 440 → 98 | 14,490 → 1,418 | 210 → 39 |
| 200 | 1,386,900 → 14,198 | 40,400 → 998 | 1,386,900 → 14,198 | 20,100 → 399 |
| 1,000 | 34,534,500 → 70,998 | 1,002,000 → 4,998 | 34,534,500 → 70,998 | 500,500 → 1,999 |
| 2,000 | 138,069,000 → 141,998 | 4,004,000 → 9,998 | 138,069,000 → 141,998 | 2,001,000 → 3,999 |
The 2,000-section row matches the repository perf fixture's XXL scale. The head grows linearly; the parent grows quadratically. The PR's 197 focused tests also report flat worst per-chunk work 71/5/71/2 at 20 and 200 sections and pass. No performance degradation was found; the block is semantic.
VERDICT: clear — the append-only performance goal is decisively met
VISUAL CHECK: manual frames required
WHY: Markdown.tsx changes render derivations and the review found a visible replacement endpoint absent from shipped stories; a green exact-head visual job alone cannot cover it.
| Sensor | Current main | Exact head | Pass? |
|---|---|---|---|
| Build | baseline main 7bd80db9929787c45e18c0f6aa27584bb7c9da78
|
c2b2d12eb1094c50ad1def3b20a1b7ba34e91713 |
yes — no reviewed path changed before latest main 09d191cd9f0abc4f6923d62bfeceb6dc4d4e299e
|
| Story/action |
core-markdown--default; stream Alpha/Old body, then replace args with same-length Bravo/New body
|
same | yes |
| Theme/mode/direction | neutral / light / LTR | same | yes |
| Viewport/media | 900×500 @1; forced-colors off; reduced-motion off; fine pointer; hover | same | yes |
| Semantic state | Storybook args are replacement text; isStreaming=true; one document |
same | yes |
| Geometry/readiness | one visible non-zero document; fonts loaded; zero running animations; no page/Storybook errors | same | yes |
| Rendered result | Bravo / New body / Tail |
Alpha / Old body / Tail |
regression |
| current main | exact head |
|---|---|
 |
 |
Sensor receipts: current main · exact head.
I opened both 900×500 frames and inspected the rendered text and spacing. The head visibly retains the old heading and body. As a control, the completed core-markdown--streaming story was also captured at 1100×900 with full receipts; both 1100×1576 images are byte-identical (6dcf9c84597f0c71d659df51ca31c5cec2381b3c46714e740342a5f979d920b7), contain the final sentence, and show one document with loaded fonts, no animation, and no errors (main frame · head frame · main receipt · head receipt).
VERDICT: BLOCKS — the replacement path displays content from the previous document
No role, accessible name/state, focus, keyboard, announcement, user-facing literal, locale, direction, or styling contract changes. Exact-head pr-a11y and pr-rtl ran and passed. The stale-document defect reaches visual and assistive readers equally because the rendered DOM itself is wrong; it is owned by behavior/API above rather than double-counted here.
VERDICT: clear — no separate a11y/i18n defect beyond the blocking content error
| slot | verdict |
|---|---|
| PROBLEM | clear |
| SOLUTION | BLOCKS — append-only ownership changes prior behavior |
| ARCHITECTURE | BLOCKS — replacement/public-result seams fail |
| IMPACT | BLOCKS — stale content and mutable prior ASTs |
| API | BLOCKS — released behavior changed without classification |
| THEMING | clear |
| BREAKING | BLOCKS — behavior and semantic API |
| PERFORMANCE | clear — linear count evidence |
| VISUAL | BLOCKS — stale old document reproduced in Chromium |
| A11Y & I18N | clear |
GOAL: partly met — append-only work is linear through 2,000 sections and all 39 append/boundary checks pass; preserving existing parser/Markdown semantics is not met because three compatibility probes fail only on this head.
DISPOSITION:
- [BLOCKS] Reusing state after a same-length or longer settled-prefix replacement returns stale old blocks. → blocks now; exact-head parser probes and a real Chromium Markdown render reproduce it, while the parent passes.
- [BLOCKS] The public parser now mutates every earlier returned AST array. → blocks now unless the mutable path is private, or the behavior is deliberately documented and classified as breaking.
ADVICE: bounded direction — preserve correct non-empty replacement behavior at the public parser/Markdown boundary; keep prior returned values stable or treat their mutation as a deliberate breaking contract. The acceptance test is the three red-on-head/green-on-parent probes plus the Chromium replacement frame.
AUTHOR CAN PROCEED: yes — no API/design/ownership decision is withheld. The next head must render same-length and longer replacements exactly like a fresh state, and must either preserve historical result values or carry an explicit breaking contract for mutation.
WORST OUTCOME: “A caller that replaces a non-empty stream without first rendering an empty value sees the previous settled heading/body mixed into the new document” → request changes.
JUDGEMENT NEEDED: none — the linked issue already establishes the tail-cache direction; this review asks only to preserve or correctly classify existing public behavior.
request changes
- [BLOCKS] The reset condition only detects a shorter input.
→ a same-length/longer replacement renders stale blocks from the previous document ·
parser.ts:2114 - [BLOCKS] The exported parser mutates one result array across calls.
→ a builder retaining an earlier AST sees it silently change after later input ·
parser.ts:2224
Thanks—the append-only path is convincingly bounded, but this changes the public parser contract without a breaking changeset. Reusing state after a same-length or longer replacement keeps the old settled prefix (confirmed in Chromium: new Bravo/New body args still render Alpha/Old body), and a later call now mutates every earlier returned AST. Both pass on the parent and fail here. Could we restore replacement correctness and either preserve snapshot values or mark/document the mutation as breaking?
[Reviewed by Robohands]
-
packages/core/src/Markdown/parser.ts:2114— This only resets on shrink; same-length or longer replacements keep stale settled blocks. -
packages/core/src/Markdown/parser.ts:2224— This mutates earlier public results; could the mutable array stay private?
- Exact-head Markdown/parser suites pass 197/197; Core typecheck passes.
- Exact-head GitHub workflow jobs (test, build, lint, Storybook, sandbox, a11y, visual, RTL, smoke, dependencies, theme layers) passed;
review-requiredis correctly pending and the separate Vercel deployment status failed. - Current main merges cleanly with the head, and no newer main commit changed the reviewed parser or rubric bytes after gate 1.
TIME total 27m setup 5m safety read, exact-head worktree, fresh fork/upstream wiki clones, clone-install; warm main reused: yes reading 7m description, test plan, diff, parser history/callers, current main, rules/rulings measuring 10m 197 shipped tests, 42 adversarial tests, parent/head A/B, four-N operation counts, two Chrome frame pairs writing 5m presentation, two critic passes, and wiki record waste 2m first visual receipt targeted a transparent theme wrapper; first count assertion omitted retained separator characters
- The reason the separate Vercel deployment status failed; all GitHub workflow checks used by this review completed on the exact head.
Not posted. Public actions were explicitly forbidden. The proposed comment above is the exact text for a later authorized review.
Independent critic phase (same session, as loop 1.4.1 requires) — pass 1: FAIL on presentation, verdict challenged and upheld. The critic attacked the two blocks rather than accepting the red tests:
-
Is non-empty replacement unsupported? No. The parent explicitly reparses when settled text changes, the published signature has no append-only type boundary,
useStreamingTextonly resets on empty, and the real component renders stale text on this head. -
Is result mutation merely an implementation detail? No. The function and state type are exported from the released
@astryxdesign/core/Markdownsubpath, the parent returned a fresh array, and this head adds a caller warning to copy retained results. The behavior is deliberate, but therefore needs compatible isolation or breaking classification. - Could the performance evidence be a clock artifact? No. The verdict uses deterministic operation counts at four N values; the timing output is not part of the finding.
-
Could the browser frame be stale or the wrong state? No. Both receipts assert the same Storybook args, streaming state, theme/mode/direction, viewport/media, one visible document, settled fonts/animations, and no errors. The current-main frame renders
Bravo/New body; the exact head rendersAlpha/Old body.
The 79-word review passed the 150-word request-changes cap; both inlines pass the 20-word cap; all ten slot verdicts exist; the signature is exact; no private merge syntax or residue appears. Pass 1 still failed because the draft used prose labels instead of the current presentation's PR/HEAD REVIEWED/VERSIONS/LANE headings, called the 7bd baseline “current” after main advanced, and named scratch test paths after cleanup.
Rewrite. Adopted the exact presentation headings, pinned the visual baseline and latest-main relation, and archived the three probes beside this page.
Pass 2: PASS. The rewritten presentation preserves the request-changes verdict, one root contract finding with two user/builder consequences, two independent confirmations for each block, exact source anchors, actionable acceptance criteria, and no unsupported implementation prescription.
The focused harnesses are archived beside this page:
- adversarial boundaries and operation counts
- replacement and retained-result A/B
- parent operation counts
# Copy the archived harnesses into packages/core/src/Markdown, then:
npx --yes pnpm@11.10.0 vitest run packages/core/src/Markdown/review-5515-replacement-only.test.ts --reporter=verbose
npx --yes pnpm@11.10.0 vitest run packages/core/src/Markdown/review-5515-adversarial.test.ts --reporter=verbose
# Shipped focused suites and typecheck.
npx --yes pnpm@11.10.0 vitest run packages/core/src/Markdown/incremental.test.ts packages/core/src/Markdown/parser.test.ts packages/core/src/Markdown/parser.perf.test.ts --reporter=verbose
npx --yes pnpm@11.10.0 -F @astryxdesign/core typecheck