fix(ui): keep escaped brackets in link labels out of display math - #5311
dedsec-terminal wants to merge 3 commits into
Conversation
Generated-by: OpenCode (Muse Spark)
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
This change adds a hand-written Markdown link scanner, re-scans recognized labels with display math disabled, and adds focused rendering regressions. The intended inline-link case is fixed, but this head is not ready to merge because the new scanner regresses standard reference links and introduces pathological synchronous work on malformed input.
Validation on this exact head: build:test; focused Markdown 39/39; full UI 469/469; full typecheck, lint, format, and ASF checks; renderer architecture 112/112; E2E budget 38; production renderer build; git diff --check; and a clean merge tree with current main (c22768c3b0dc47518f6f8584e864f86f0b1e5379). GitHub currently exposes only the successful label check, not a hosted test check. I did not exercise a packaged Desktop on native Windows or macOS.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
|
|
||
| const tail = source[labelEnd + 1] ?? ''; | ||
| if (tail === '(') { | ||
| const tailEnd = findInlineTailEnd(source, labelEnd + 1); |
There was a problem hiding this comment.
[P2] Bound or linearize the pending link-tail scan. For every [x]( opener without a closing ), findInlineTailEnd() walks the entire remaining suffix and this branch resumes from only openerEnd, so repeated openers are quadratic; the pending state also leaves safeSourceEnd=0, forcing every streaming append to repeat the whole scan. On this exact head, a 128,000-character input took 6.68 s in prepareMarkdownMath versus 24.7 ms on the exact base; streaming 16 chunks to 64,000 characters took 18.6 s cumulatively versus 17 ms. This runs synchronously before Markdown rendering, so one malformed model or user message can freeze the Desktop. Please use a single-pass or bounded parser, or advance/fence pending tails, and add an adversarial linear-time regression.
There was a problem hiding this comment.
Addressed in e65c467. Link-tail parsing is now bounded and pending tails consume the suffix once; valid link tails remain opaque. Added full/streaming adversarial regressions for repeated malformed [x]( input; focused runs stayed under 1 ms locally.
|
|
||
| /** | ||
| * Recognize a Markdown inline link or image starting at `index` so its label | ||
| * can be re-scanned without display math. Bare `[text]` without a `(...)` or |
There was a problem hiding this comment.
[P2] Cover the standard collapsed and shortcut reference-link forms before rewriting label identity. Astryx already resolves both. On the exact base, [\\[DISCUSS\\] label][] plus its matching definition renders an anchor (with the old unwanted display math); on this head the use-site label becomes literal transport tokens while the definition becomes a display-math token, so the reference IDs no longer match and the UI shows literal [[DISCUSS] label][]. A shortcut reference still forms an anchor but keeps the original block KaTeX inside it because this scanner deliberately ignores a label without an explicit tail. The escaped-bracket fix needs to preserve identity across full, collapsed, and shortcut reference definitions, with regressions for both forms.
There was a problem hiding this comment.
Addressed in e65c467. Closed labels now use escaped-bracket transport consistently across full, collapsed, and shortcut reference uses and definitions; valid link tails stay opaque so reference IDs and hrefs remain unchanged. Added regressions for all three forms.
Bound link-label and destination scans, preserve opaque link tails, and keep escaped reference labels stable across full, collapsed, and shortcut forms. Generated-by: OpenAI Codex
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
This follow-up bounds the malformed-link scan and restores the collapsed and shortcut reference cases reported on the previous head. Those two issues are fixed, but this head is still not ready to merge because two production parsing paths remain chunk-dependent or inconsistent.
Validation on this exact head: clean install; build:test; focused Markdown 41/41; full UI 471/471; full typecheck, lint, format, and ASF checks; renderer architecture 112/112; E2E budget 38; production renderer build; git diff --check; and a clean merge tree with current main (c22768c3b0dc47518f6f8584e864f86f0b1e5379). GitHub currently exposes no hosted checks. I did not exercise a packaged Desktop on native Windows or macOS.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
| const refEnd = findLabelEnd(source, labelEnd + 2); | ||
| if (refEnd === 'pending') return { kind: 'pending', end: source.length }; | ||
| if (typeof refEnd !== 'number') return match(refEnd.end); | ||
| return match(refEnd + 1); |
There was a problem hiding this comment.
[P2] Normalize the explicit reference identifier as well as the visible label. A valid full reference such as [visible][\\[topic\\]] with definition [\\[topic\\]]: https://example.com/ref renders an anchor on the exact base, but on this head the second label is copied verbatim here while the definition label is rewritten to literal transport tokens. The IDs no longer match, so the UI displays literal [visible][[topic]]. The new full-reference test uses a plain topic identifier and misses this path. Re-scan the reference label with the same identity-preserving transform and add an escaped-identifier regression.
There was a problem hiding this comment.
You are right, sorry about that - I rewrote the visible label but left the identifier verbatim, so of course the IDs diverged. Fixed now: the explicit reference label goes through the same transform, with an escaped-identifier regression using your exact case. Is this looking better?
| if (typeof refEnd !== 'number') return match(refEnd.end); | ||
| return match(refEnd + 1); | ||
| } | ||
| return match(labelEnd + 1); |
There was a problem hiding this comment.
[P2] Do not mark a bare closed label safe while an appended byte can still turn it into a link. The incremental cache resumes after this label, so a later (...) tail bypasses the opaque-tail scan. In a production MarkdownBody rerender, rendering [label] and then appending (https://example.com/$$value$$) produced an href containing a raw private-use MAKA_MATH token; a fresh mount of the identical final text preserved the correct https://example.com/$$value$$ href. Thus link targets depend on stream chunking and can change after remount. Keep enough lookbehind or unsafe suffix to reclassify the label when ( or [ arrives, and add a one-shot-versus-incremental equivalence regression.
There was a problem hiding this comment.
Good catch, that was my mistake - I was settling a bare closed label even though a later ( or [ can still turn it into a link. Bare labels at end of input now stay unsettled so the tail gets scanned with the label context, and I added a one-shot-vs-incremental equivalence test with your href case. Does that cover it?
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
The previous explicit-reference and chunk-stable link-tail findings are fixed on this head, but the generalized scanner still has three production regressions. The review therefore remains a technical NO-GO at 3 x P2.
Validated on the exact head with a clean install, build:test, Markdown 43/43, UI 473/473, full typecheck/lint/format/ASF checks, renderer architecture 112/112, E2E budget 38, production renderer build, diff check, and a clean merge tree against current main c22768c3b0dc47518f6f8584e864f86f0b1e5379. GitHub currently reports no hosted checks. I did not run native Windows/macOS packaged Desktop validation.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
| // Image alt text is rendered as a raw attribute, not as Markdown | ||
| // inline content. Leave its source untouched and continue scanning | ||
| // the destination normally. | ||
| text += source.slice(index, link.end); |
There was a problem hiding this comment.
[P2] Apply the escaped-bracket/reference-ID normalization to images too. This branch copies the entire image use unchanged, while the matching definition label is normalized later. On this head, valid ![visible][\\[topic\\]] with [\\[topic\\]]: https://example.com/image.png renders as literal Markdown; the exact base renders <img src="https://example.com/image.png" alt="visible">. Inline, collapsed, and shortcut image forms with escaped brackets fail similarly, and the new tests cover only links. Preserve image identity while keeping alt text non-math, and add image regressions.
| // or reference tail, so it must not settle: resuming after it would | ||
| // scan that tail without the label context. Once any byte follows the | ||
| // label the link question is decided and settling is safe again. | ||
| const mayGrowTail = link.end === link.labelEnd + 1 && link.end >= source.length; |
There was a problem hiding this comment.
[P2] This cache hold starts only after the scanner has already seen ![, so a stream whose first chunk ends at the lone ! still caches that byte as safe. On the next update scanning resumes at [ and treats the image label as an ordinary link label. In a production MarkdownBody rerender, ! followed by ![alt \\[x\\]](https://example.com/a.png) produced an image whose alt contains private-use MAKA_MATH tokens, while a fresh mount of the identical final source rendered literal Markdown. Keep an unescaped trailing ! unsafe (or preserve enough lookbehind) and add an image split-boundary regression.
| + source.slice(refEnd, link.end); | ||
| index = link.end; | ||
| atLineStart = source[index - 1] === '\n'; | ||
| const labelSafe = protectedLabel.safeSourceEnd >= link.labelEnd - link.labelStart; |
There was a problem hiding this comment.
[P2] A closed label should not inherit end-of-input uncertainty from the recursive scanner. For the valid link [price$](https://example.com), the nested label scan treats the terminal $ as a possible future $$, so labelSafe is false and permanently disables every later cache checkpoint. A 512 KiB response appended in 64 updates took 1.58 s here with safeSourceEnd=0, versus 66 ms on the exact base (and 93 ms for a normal label on this head); 1 MiB/128 updates took 7.28 s. Treat an already bounded label/reference substring as final, or use a non-streaming nested mode, and add a label-ending-$ streaming performance regression.
Summary
prepareMarkdownMath()tokenizes\[...\]as display math before Markdown parsing, so escaped brackets inside link labels became block-level KaTeX inside the<a>instead of literal text. The math scanner now recognizes inline links/images, re-scans labels with display math disabled (inline math still renders inside labels), and rewrites escaped brackets in real link labels as literal transport tokens so bracket matching still forms the link.Fixes #5310
Verification
packages/ui/src/__tests__/markdown-body.test.ts(escaped-bracket label, inline math in label,$$in label stays literal, display math after a link). Checked the two display-math tests fail on unpatchedmarkdown-math.tsxand all pass with the fix:markdown-body39/39, han/rhythm suites 4/4.biome checkandtscbuild clean on the changed files.@maka/uidist suite: only failures are 6 pre-existingmermaid-render-cacheenvironment failures, identical on the pristine tree.AI use
Select exactly one:
Tool(s) and scope: OpenCode (Muse Spark) - root-cause analysis, the link-aware preprocessing fix and regression tests, plus the local verification above.
Checklist
Does this PR entail a change in behavior?