Skip to content

Refactor markdown rendering for separate compilation #5804

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
pranavjoshi001 wants to merge 3 commits into from
Closed

Refactor markdown rendering for separate compilation #5804

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0d338fd
Refactor markdown rendering for separate compilation
pranavjoshi001 Apr 14, 2026
e9fea5b
trying to fix streaming
pranavjoshi001 Apr 14, 2026
736c4e2
Merge branch 'streaming-trying-to-fix' of https://github.com/pranavjo…
pranavjoshi001 Apr 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 20 additions & 12 deletions packages/bundle/src/markdown/createStreamingRenderer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,9 +197,18 @@ export default function createStreamingRenderer(
wrapper.append(applyTransform(activeFragment, options.transformFragment));
} else {
// New block boundary in tail: commit newly-finished blocks, replace active.
// We must compile the committed and active portions as separate substrings
// (not filtered events) because reference links like [1] require their
// definitions to be present in the same compilation unit.
const newActiveOffsetInTail = tailBlocks[tailBlocks.length - 1].startOffset;
const committedTailEvents = tailEvents.filter(([, token]) => token.start.offset < newActiveOffsetInTail);
const committedTailHTML = compile(micromarkOptions)(committedTailEvents);
const committedTailMarkdown = processedMarkdown.slice(
activeBlockStartOffset,
activeBlockStartOffset + newActiveOffsetInTail
);
const activeTailMarkdown = processedMarkdown.slice(activeBlockStartOffset + newActiveOffsetInTail);
Comment on lines +204 to +208
Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code now duplicates the same conceptual operation (split markdown at an offset → compile two parts) in multiple branches (tail split vs. last-block split), which increases the risk of subtle inconsistencies (especially once reference-definition handling is corrected). Consider factoring this into a small helper that (a) computes the two substrings and (b) enforces a single consistent strategy for reference definitions/whitespace preservation, so future fixes don’t need to be applied in two places.

Copilot uses AI. Check for mistakes.

const committedTailHTML = compile(micromarkOptions)(parseEvents(committedTailMarkdown));
const activeTailHTML = compile(micromarkOptions)(parseEvents(activeTailMarkdown));
Comment on lines +208 to +211
Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change compiles activeTailMarkdown / activeMarkdown in isolation, which means reference-style links in the active portion (e.g. [label]) will not resolve if their reference definitions (e.g. [label]: …) are in the committed portion. That appears to contradict the new comment about definitions needing to be in the same compilation unit, and likely regresses link rendering across the committed/active boundary. A concrete fix is to ensure the active compilation unit includes the committed reference definitions (e.g., extract reference-definition lines from committedMarkdown and prepend them to the active markdown before parseEvents, or otherwise supply definitions to the micromark pipeline) so active links can resolve against already-committed definitions.

Copilot uses AI. Check for mistakes.
Comment on lines +210 to +211
Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Within the hot streaming path, this now instantiates the compiler and does a full parseEvents() + compile() pass twice per split (committed + active). At minimum, consider creating a single const compiler = compile(micromarkOptions) once (outside the loop / outside the split branches) and reuse it here to avoid repeated compiler creation. If streaming updates are frequent, also consider caching the committed side’s HTML/events and only recompiling the delta (plus whatever is needed for reference-definition correctness) to avoid repeatedly recompiling large committed prefixes.

Copilot uses AI. Check for mistakes.

activeBlockStartOffset += newActiveOffsetInTail;

Expand All @@ -209,8 +218,7 @@ export default function createStreamingRenderer(
committedFragment.append(...Array.from(committedDoc.body.childNodes));
betterLinkDocumentMod(committedFragment, createDecorate(emptyDefinitions, externalLinkAlt));

const remainingHTML = tailHTML.slice(committedTailHTML.length);
const activeDoc = domParser.parseFromString(remainingHTML.trim(), 'text/html');
const activeDoc = domParser.parseFromString(activeTailHTML.trim(), 'text/html');
const activeFragment = activeDoc.createDocumentFragment();

activeFragment.append(...Array.from(activeDoc.body.childNodes));
Expand Down Expand Up @@ -251,14 +259,14 @@ export default function createStreamingRenderer(

activeBlockStartOffset = lastBlock.startOffset;

// Compile the active (last block) portion first — it is always a single
// block and therefore cheap. Then derive the committed HTML by slicing
// the already-compiled full output so that inter-block whitespace
// (produced by lineEnding events the compiler only flushes mid-stream)
// is preserved in the committed fragment.
const activeEvents = fullEvents.filter(([, token]) => token.start.offset >= lastBlock.startOffset);
const activeHTML = compile(micromarkOptions)(activeEvents);
const committedHTML = rawHTML.slice(0, rawHTML.length - activeHTML.length);
// Compile the committed and active portions as separate substrings
// (not filtered events) because reference links like [1] require their
// definitions to be present in the same compilation unit.
const committedMarkdown = processedMarkdown.slice(0, lastBlock.startOffset);
const activeMarkdown = processedMarkdown.slice(lastBlock.startOffset);
Comment on lines +262 to +266
Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code now duplicates the same conceptual operation (split markdown at an offset → compile two parts) in multiple branches (tail split vs. last-block split), which increases the risk of subtle inconsistencies (especially once reference-definition handling is corrected). Consider factoring this into a small helper that (a) computes the two substrings and (b) enforces a single consistent strategy for reference definitions/whitespace preservation, so future fixes don’t need to be applied in two places.

Copilot uses AI. Check for mistakes.

const committedHTML = compile(micromarkOptions)(parseEvents(committedMarkdown));
const activeHTML = compile(micromarkOptions)(parseEvents(activeMarkdown));
Comment on lines +265 to +269
Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change compiles activeTailMarkdown / activeMarkdown in isolation, which means reference-style links in the active portion (e.g. [label]) will not resolve if their reference definitions (e.g. [label]: …) are in the committed portion. That appears to contradict the new comment about definitions needing to be in the same compilation unit, and likely regresses link rendering across the committed/active boundary. A concrete fix is to ensure the active compilation unit includes the committed reference definitions (e.g., extract reference-definition lines from committedMarkdown and prepend them to the active markdown before parseEvents, or otherwise supply definitions to the micromark pipeline) so active links can resolve against already-committed definitions.

Copilot uses AI. Check for mistakes.
Comment on lines +267 to +269
Copy link

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Within the hot streaming path, this now instantiates the compiler and does a full parseEvents() + compile() pass twice per split (committed + active). At minimum, consider creating a single const compiler = compile(micromarkOptions) once (outside the loop / outside the split branches) and reuse it here to avoid repeated compiler creation. If streaming updates are frequent, also consider caching the committed side’s HTML/events and only recompiling the delta (plus whatever is needed for reference-definition correctness) to avoid repeatedly recompiling large committed prefixes.

Suggested change
const committedHTML = compile(micromarkOptions)(parseEvents(committedMarkdown));
const activeHTML = compile(micromarkOptions)(parseEvents(activeMarkdown));
const compiler = compile(micromarkOptions);
const committedHTML = compiler(parseEvents(committedMarkdown));
const activeHTML = compiler(parseEvents(activeMarkdown));

Copilot uses AI. Check for mistakes.

const committedDoc = domParser.parseFromString(committedHTML, 'text/html');
const committedFragment = committedDoc.createDocumentFragment();
Expand Down
Loading