feat(ui): render markdown reference links - #1168
Conversation
The simplified markdown parser only understood inline links `[text](url)`, so CommonMark reference links rendered as raw text: `[text][id]` and the `[id]: url` definition both showed literally (backnotprop#923). Add `resolveReferenceLinks`, a pure pass run at the top of `parseMarkdownToBlocks` that rewrites full (`[text][id]`), collapsed (`[text][]`), and shortcut (`[text]`) references, plus their image forms, into inline `[text](url)` links, so the existing inline renderer draws them. Link reference definitions are collected first (first definition wins, labels matched case-insensitively with collapsed whitespace, `<url>` and quoted-title forms supported) and then blanked in place, so a definition never renders and every block keeps its original source line number. Resolution is code-aware: references and definitions inside fenced code blocks and inline code spans are left verbatim, a shortcut is skipped when an inline `(...)` destination follows it or when it is a task-list checkbox marker at the start of a list item, and an unknown reference stays literal so bracketed prose like `[TODO]` or `[0]` never becomes a false link. A definition-shaped line is only collected when it can start a block (after a blank line, a code fence, another definition, or the document start), so a `[word]: token` line that continues a paragraph is left as text rather than deleted (CommonMark: a definition cannot interrupt a paragraph). A document with no definitions is returned unchanged.
|
In-depth review done. First: the core design call is exactly right. Blanking definition lines in place instead of deleting them preserves startLine, which the checkbox reporter and section grouping depend on, and that was verified downstream. Security also checks out end to end: resolved URLs go through sanitizeLinkUrl (javascript:/data:/vbscript:/file: blocked, verified), hrefs travel as React props so attribute smuggling is impossible, and a reference definition carries exactly the trust of an inline link. Idempotence confirmed. Full suite and typecheck clean, 142 new tests pass. That said, several content-corruption paths are reachable with ordinary plan markdown, so this needs a round of fixes:
Nits, take or leave: the resolver knows ~~~ fences but the block loop does not, so a stray ~~~~ silently disables the feature for the rest of the document; destinations are not escaped when re-inlined so a ) in a URL truncates the href and leaks stray text; nested brackets ([see [1]][id]) produce a spurious shortcut link; backslash-escaped brackets still resolve; definitions directly under a heading with no blank line are not recognized (degrades safely, but a ## References section written tightly will not work). One release-notes item rather than a code change: drafts and share URLs created before this PR whose anchor text spans raw [text][id] markup will fail text-search restore after upgrade (annotation survives unanchored with a console warning). Narrow and one-time. The new tests are good on everything the implementation considered; the fix round should add cases for indented fences, HTML blocks, footnotes, unreferenced definitions, CRLF, and an explicit idempotence assertion. Happy to re-review quickly on push. |
…link resolution Owner review round for reference-style link resolution (backnotprop#923): - Fence detection now mirrors the block parser's own naive rule exactly (full .trim() + startsWith('```'), any indentation, backtick-only — no ~~~ support) instead of a looser 0-3-space approximation, so indented and list-nested fences the block parser treats as code can never be rewritten. Aligns tilde-fence behavior the same way: since the block parser has no ~~~ support, the resolver no longer protects ~~~ blocks either. - Raw HTML blocks (<details>, <pre>, etc.) are now protected using the same HTML_BLOCK_TAGS/HTML_BLOCK_OPEN_RE/VOID_HTML_TAGS the block parser itself uses, with the same three termination rules (blank-line, void single-line, balanced-depth). - GFM footnote definitions ([^label]: ...) are excluded from collection entirely, so they and their [^label] references are never rewritten into inline links. - A definition-shaped line is now only blanked when its label was actually consumed by a resolved reference outside a protected region. Unused definitions, and definitions referenced only from inside code/HTML, stay visible. This also fixes a plan-diff bug: a URL-only edit to a definition line used to blank to nothing on both sides of the diff (a real change rendering as empty); now the isolated diff chunk keeps the definition visible and diffs normally. - CRLF lines are now recognized (definition regex tolerates a trailing \r) and preserved (a blanked line keeps its own \r). - Bound the label/text capture groups (999 chars, CommonMark's own label limit) and the code-span alternative (5000 chars) so a long run of unmatched brackets/backticks can no longer cause quadratic backtracking within the 2MB annotate cap; added a defense-in-depth cap on the number of definitions tracked per document. - Added coverage for nested brackets, backslash-escaped brackets, parenthesized destinations, idempotence, and confirmed dangerous destinations still flow through the existing sanitizeLinkUrl path unchanged. Added a migration-caveat note to the existing annotation-anchor section of packages/ui/HANDOFF.md: documents using reference-style links render differently now, which can shift position-based anchors captured before a host upgrades past this change.
…er case markProtectedLines and parseMarkdownToBlocks each independently scanned line-by-line from a multi-line HTML opener until its balanced open/close depth returned to zero, giving up only at end-of-document. That scan never advanced the outer index on failure, so a document with many consecutive unclosed openers (e.g. thousands of bare <div> lines with no </div> anywhere) made every one of them re-run the same O(N) tail scan — O(N^2) total, a real hazard well within the 2MB annotate cap. Extract the scan into one shared helper, findHtmlBlockEnd, used by both call sites so they can't drift apart: - closeExistsFromLine lazily builds (once per tag name, cached per document) a suffix array answering whether a closing tag exists at or after a given line, so an opener that can never close is rejected in O(1) instead of scanning to EOF. - MAX_HTML_BLOCK_SCAN_LINES bounds the residual case (a closing tag exists far away but depth never actually reaches zero before it) to a constant amount of work per start position — a documented, safe degradation: a block whose true close sits beyond the cap is treated as unclosed, identically to today's 'no close ever found' case. Added a failing-before-fix perf test (many unclosed <div> lines took ~2.3-3.4s and blew a 800ms bound; now ~12-15ms) for both parseMarkdownToBlocks and resolveReferenceLinks, plus a parity test proving a real <details>...</details> block stays intact and identically protected/parsed among thousands of decoy unclosed <div> lines.
|
Pushed the review fixes. Protected regions, footnotes, unused definitions, CRLF, bounded scanning, and clean diff visibility are covered. Checks are passing. |
|
Re-verified 9440be0 by re-running every original reproduction plus a 20,000-document differential fuzz (zero protected-region leaks, zero line-count changes, zero idempotence failures). All seven findings are genuinely fixed, and two of your choices are better than what the review suggested: sharing the block parser's exact conditions (single findHtmlBlockEnd used by both passes, byte-identical void-tag branches) and blanking only consumed labels, which also fixed the clean-diff invisibility with no diff-engine change needed. The quadratic scan is now clean linear: 128k chars went from 6.3s to 95ms, and the 2MB worst case from an extrapolated ~27 minutes to 1.4s. CRLF, footnotes, indented fences, HTML blocks, unused definitions: all confirmed against the original repro inputs. 25 new tests, full suite 2631 pass, typecheck clean. Removing ~~~ protection to agree with the block parser was also the right call. One new regression from the perf fix, and it is the only thing to hold for: MAX_HTML_BLOCK_SCAN_LINES = 2000 now applies to the pre-existing parseMarkdownToBlocks HTML scan. A Details(or raw ) whose closing tag sits more than ~2000 lines below the opener no longer renders as one block: measured, a 1999-line body splits into opener-alone, body-as-paragraphs, close-alone, so a plan wrapping a long log in loses its collapsible and leaks raw HTML as text. Mitigating facts, both measured: the same commit fixes a pre-existing O(N squared) hang (40k unclosed divs: 40.5s on main, 26ms here), and the closeExistsFromLine suffix array already handles the no-close-anywhere pathological shape without any cap. The cap only guards the rare close-exists-far-away case, which is still slow at 2000 anyway. Suggested: raise it substantially or scale with document length, e.g. Math.max(20000, lines.length).
Residual nits, all acceptable as-is: unbalanced ) in a destination still truncates the href (your new test covers only the balanced case); mixed backslash-escape [not a ref][id] still resolves; [see [1]][id] still yields a spurious link (explicitly documented and sanitization-guard-tested, so the invariant that matters holds); tight-heading definitions still degrade to old behavior; the clean-diff view is now consistently source-level, which disagrees with the rendered view but defensibly so. The HANDOFF note on pre-upgrade draft re-anchoring covers the migration caveat. Bump the cap and this is ready. |
MAX_HTML_BLOCK_SCAN_LINES (2000) fixed the O(N^2) unclosed-opener case but as a side effect also truncated genuinely valid, longer HTML blocks: a <details> or raw <table> block whose closing tag sits beyond 2000 lines got cut off mid-block, with its remaining content and the real closing tag falling through as separate, incorrect blocks. closeExistsFromLine already rejects an opener that can never close in O(1) (no closing tag anywhere in the document) without scanning a single line — that already eliminates the pathological 'many failing scans' case on its own. A cap on top of that only ever hurt the opposite case: a scan that DOES succeed, which happens once per document and costs O(L) for an L-line block exactly like reading any other block's content once. So the cap bought nothing further and could silently corrupt valid parsing for any block longer than it, however generous its value. Removed it; the scan now runs unbounded to its real end once closeExistsFromLine confirms a close exists at all. findHtmlBlockEnd is still the single shared implementation used by both markProtectedLines and parseMarkdownToBlocks, so both stay in parity. Added regression tests: a >2000-line <details> and a >2000-line raw <table> block each stay one whole html block (previously truncated); a link definition inside a >2000-line <details> block stays protected and never wins over a real definition outside it; a valid long <details> block survives even preceded by thousands of unclosed <div> decoys. Re-verified the 40k-unclosed-opener perf/parity case (already handled by closeExistsFromLine alone) stays fast and unaffected.
Removing the fixed line-count cap fixed truncation of valid long HTML blocks, but reopened a closely related O(N^2) case: N unclosed <div> openers followed by a single trailing </div> all still pass the 'does a close exist anywhere' pre-check, so every one of them independently scanned forward (mostly to end-of-document) before giving up. Measured before this fix: 5000 openers ~1.0s, 10000 ~4.1s, 40000 timed out past 69s. Replaced the scan entirely with a per-tag-name prefix-sum index (buildTagCloseIndex): the running open-minus-close count for a tag name, plus a classic 'next element at or below this one' index over that prefix sum (an O(N) monotonic-stack construction, each position pushed/popped at most once). Finding where (if anywhere) a block starting at a given line closes is exactly that classic query, so it is now an O(1) lookup with zero scanning per opener, whether the block never closes, closes after 3 lines, or closes 3000 lines away. Both markProtectedLines and parseMarkdownToBlocks still share the single findHtmlBlockEnd implementation, so they stay in parity. 40000 unclosed <div> openers + one trailing </div> now resolve in ~30-45ms (parser and resolver both), with block-boundary parity between them. Re-verified: no truncating cap reintroduced (>2000-line <details>/<table> blocks still stay whole), nested same-tag blocks still balance on true depth (not just tag presence), and independent tag types (e.g. a <table> nested inside a <details>) don't cross-talk between their separate per-tag indices.
|
Pushed the final HTML-block fix. Long valid blocks are preserved, while both unclosed-opener adversarial shapes now use a shared linear close index. Checks are passing. If you want a tiny palate cleanser after this parser one, #1154 is a small Vim HUD scroll fix. |
|
Verified fe017f1 empirically. The prefix-sum close index is a better answer than the raised cap the review suggested: long valid details blocks at 1999, 5000, and 30000 lines all render as a single HTML block (regression gone), 40k unclosed divs parse in 19ms, and the residual adversarial shape that previously cost 8.7s even under the cap (100k openers plus one trailing close) now parses in 51ms. Resolver idempotence holds on protected content, and all 176 parser tests pass. Nothing left from my side: merge-ready. |
2 participants
Summary
Closes #923.
The simplified markdown parser (
parseMarkdownToBlocks) only understood inline links[text](url). CommonMark reference links rendered as raw text: both the reference[text][id]and its[id]: urldefinition showed literally in the document. This adds reference-link support by resolving references into inline links before the block split, so the existing inline renderer draws them.Approach
A single pure function,
resolveReferenceLinks(markdown), runs at the top ofparseMarkdownToBlocks, right after frontmatter extraction. It does two passes:[label]: destination "optional title", up to three leading spaces,<url>or bare destination, first definition wins, labels normalized case-insensitively with collapsed internal whitespace,^-prefixed labels rejected so GFM footnotes are left alone). A line only counts as a definition when it can start a block (the previous line is blank, a code fence, another definition, or the document start) and its trailing content is empty or a valid quoted or parenthesized title, so ordinary prose is never mistaken for a definition and a definition can never interrupt a paragraph.[text][id], collapsed[text][], and shortcut[text], plus the image forms![alt][id]. Only definitions that are actually referenced somewhere are blanked; an unreferenced definition (or one whose only reference sits inside a protected region) is left as-is. Blanked lines are replaced with an empty line, not removed, so line count, and therefore every block'sstartLine, stays accurate.Fenced code blocks and HTML blocks share the exact block-boundary detection used by the rest of the parser, so a reference-looking pattern inside either is never rewritten and a
[id]: urlinside either is never collected as a definition. Because the output is ordinary[text](url)markdown, no renderer change is needed.Correctness and safety
<pre>,<details>, and similar) is never rewritten, and a reference-shaped line inside any of them is never collected as a definition.[^1]: ...) are left untouched, not consumed as reference definitions.[text]is left literal when an inline(...)destination follows it and when it does not name a definition, so bracketed prose like[TODO]or an array index[0]never becomes a false link. A checkbox marker (- [x]) at a list-item start is never treated as a shortcut reference.<details>section) still renders as a single block rather than being truncated for performance.Validation
226 targeted parser tests pass, including full/collapsed/shortcut/image resolution, case- and whitespace-insensitive labels, the angle-bracket destination, first-definition-wins, unknown references left literal, protected fenced code, inline code, HTML blocks and footnotes, CRLF handling, consumed-only definition blanking, and linear-time scanning on large documents including long well-formed HTML blocks. A full UI and typecheck pass (including the strict consumer) is also clean. No
node:imports were added, so the browser-safe boundary holds.Known limitations
)inside a destination can still truncate the resulting href; a nested reference like[see [1]][id]can still produce an extra link (its href is still sanitized); and a definition placed directly under a heading with no blank line in between is not recognized, matching the parser's existing prose-continuation handling.[text][id]reference markup, will not auto-restore its position after upgrading; the annotation is preserved but appears unanchored, with a console warning. This is a one-time transition case for pre-existing data, not an ongoing limitation.Compatibility
Additive. Documents without reference definitions are unchanged (identical block output). No public API, prop, or payload changes. The function is exported so it can be unit-tested and reused.
Related work
None. This is a self-contained parser feature.