Repository navigation
[lexical-markdown] Bug Fix: Escape a literal bracket only where it would start a link - #9600
Conversation
…uld start a link ## Description Since #9481 the Markdown export writes every literal `[` and `]` as `[` and `]`, so ordinary text such as `cleared review [1][2]`, `[x] done` or `array[0]` changes on every save to `cleared review [1][2]`. CommonMark renders both forms the same, but the stored text changes, which breaks callers that read the exported string (citation regexes such as `/\[(\d+)\]/`, exact round-trip checks, checklist detection). CommonMark only needs a bracket escaped where it would otherwise form a link: - Outside a link, a link, image or link reference definition needs a `]` followed by `(` (https://spec.commonmark.org/0.31.2/#inline-link), or by `:` after a label that starts a line (https://spec.commonmark.org/0.31.2/#link-reference-definitions). Escaping that `]` is enough, whatever precedes it, so only that `]` is escaped. A `]` at the end of a text node is checked against the next text sibling, so constructs split across differently styled nodes are still caught. - Inside link text, brackets are allowed as matched pairs (https://spec.commonmark.org/0.31.2/#link-text). Unmatched brackets, and a pair followed by `(` (which would read as a nested link), are escaped. Other matched pairs stay as typed, so `[[1]](url)` exports unchanged. The `<` and `&` escaping from #9481, and import of the old `[`/`]` output, are unchanged. ## Changeset `.changeset/markdown-narrow-bracket-escapes.md` (`[fix]`, `@lexical/markdown` patch, with a `Verify:` paragraph). ## Test plan ### Before `pnpm exec vitest run --project unit packages/lexical-markdown/src/__tests__/unit/MarkdownEscaping.test.ts -t 'brackets that cannot become Markdown syntax'` on `main`: ``` × leaves "cleared review [1][2]" as typed × leaves "[x] done and [ ] todo" as typed × writes link text "[1]" as "[[1]](uri)" ... Expected: "cleared review [1][2]" Received: "cleared review [1][2]" Expected: "[x] done and [ ] todo" Received: "[x] done and [ ] todo" ``` ### After - `pnpm exec vitest run --project unit packages/lexical-markdown`: 858 tests pass, including 28 new cases. Each plain-text case is exported split at every offset across two styled text nodes, checked against the expected Markdown, and imported back to the same text with no link. - CommonMark check (a local script, not committed): every export was rendered with micromark 4.0.2 and compared with the source. 50,000 random plain-text strings over `[]()!: a1\<&#;*`, split at a random offset across two styled nodes, render as the same text with no link or image. 30,000 random link labels render as exactly one link with the same text. `main` passes the same check: both versions are spec-correct, and this one escapes less. - `pnpm run test-unit`: 587 files, 18,207 tests pass. - `pnpm run ci-check` (tsc, flow, prettier, lint, changesets) passes. - Meta-internal www: with this build, three www test suites that #9481 broke (citation round trips and checklist rendering) pass again. Browser and E2E suites were not run; the change is limited to Markdown export string generation.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
lexical
@lexical/a11y
@lexical/clipboard
@lexical/code
@lexical/code-core
@lexical/code-prism
@lexical/code-shiki
@lexical/compiler
@lexical/devtools-core
@lexical/dragon
@lexical/eslint-plugin
@lexical/extension
@lexical/fast-check
@lexical/file
@lexical/hashtag
@lexical/headless
@lexical/history
@lexical/html
@lexical/internal
@lexical/link
@lexical/list
@lexical/mark
@lexical/markdown
@lexical/mdast
@lexical/offset
@lexical/overflow
@lexical/plain-text
@lexical/react
@lexical/rich-text
@lexical/selection
@lexical/table
@lexical/tailwind
@lexical/text
@lexical/utils
@lexical/y
@lexical/yjs
commit: |
|
AI review status for this pull request.
|
etrepum
left a comment
etrepum
left a comment
There was a problem hiding this comment.
Claude review:
Review of #9600 (head ab23dd0)
Requesting changes, but the approach is right. Escaping only the ] that would close a link label is the minimal rule CommonMark needs, and it fixes the real breakage from #9481 ([1], [x] and array[0] were rewritten on every save). I checked the remaining gaps by rendering each export with micromark 4.0.2. Most of them come from the line-start check and the next-character check reading something other than what is actually exported. Fixes and tests are in lexical-9600-label-and-neighbours.patch, which applies on ab23dd0 (@lexical/markdown unit tests and tsc -p tsconfig.test.json pass).
1. $endsDefinitionLabel misses labels, so the text turns into a definition and is lost. It reads earlier siblings' raw getTextContent(), but it reads this node's text after escaping, and its regex treats \x as an escape. Each case below exports a valid link reference definition, which micromark renders as nothing:
[a\+]: /urlin two styled nodes exports[a\\]: /url. The raw\looks like it escapes the], but the export doubles it, so the]really closes the label.[a](x]: /urlin one node exports[a](x]: /url. The first]is escaped to], so it no longer closes anything, but the check still sees it as a bracket.$convertSelectionToMarkdownStringwith only[a]: /urlselected after an unselectedintroexports[a]: /url. The walk reads unselected siblings, so it seesintro [aas the line.
The patch replaces the regex with a small state machine (up to three spaces, then [, then no further [). It keeps ] inside the label, which only over-escapes, and it stops the walk at the start of the selection, using $exportedText for siblings.
2. The export is now quadratic. For every ]:, $endsDefinitionLabel slices the node's text from 0 and walks back through every earlier sibling until it finds a newline. A paragraph of 8,000 styled xa]: nodes takes 53 ms on main and 39.8 s on this PR, and an 80,000-character single node takes 60 ms on main and 4.4 s here. Text from paste, import and collab can be this large. The patch computes the line state once per node, and only for nodes with a ] that : might follow. It bounds the walk at 1,003 characters, since a label holds at most 999 characters. With the patch those inputs take 534 ms and 12 ms.
3. Inline elements hide the neighbouring text. getTextSibling returns null at the edge of a mark or another inline element that exports only its children, and the line-start walk stops there too. Comment marks are the common case. [a] inside a mark followed by (b) exports [a](b), which is a link to b. [a followed by a mark around ]: /url exports as a definition. The patch adds $adjacentInline, which looks through inline elements other than links, for both checks.
4. A leading [ ] or [x] becomes a checklist again (please decide). On main, #9481 also kept these from re-importing as task items. With this PR, a bullet item whose text is [ ] todo exports as - [ ] todo, and a paragraph [x] done exports as [x] done. With CHECK_LIST in the transformers, both import as check list items, because CHECK_LIST_REGEX matches even without a list marker. For the bullet item, GFM renders a task list item too. Escaping only the ] of a [ ]/[x]/[X] followed by whitespace at the start of a block would keep the rest of the PR's plain-text output unchanged. If the www checklist suites depend on [x] done coming out raw, please say so in the description, so the trade-off is explicit.
![meta-code-review[bot]](https://avatars.githubusercontent.com/in/5334748?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for narrowing this. Exporting [1], [x] and array[0] as typed again is a real improvement, and keeping the old [/] output importable is the right call.
I agree with the earlier review on this head: the cases below confirm its points and add one, and the first needs a fix before this lands.
1. A ] next to an inline element boundary is not escaped (blocking): packages/lexical-markdown/src/MarkdownExport.ts:671-675
The character after a node's last ] comes only from a sibling text node (getTextSibling(node, false)), and $endsDefinitionLabel only walks siblings in the same parent (:738-748). When the ] and the following ( or : sit on either side of an inline element such as a comment MarkNode:
- a comment on
[a]followed by(b)exports as[a](b), which$convertFromMarkdownStringreads back as a linka→b; - a link to
urlwhose text has a comment on[x]followed by(y)exports as[[x](y)](url), which reads back as a link toy; - a comment on
[a]followed by: bat the start of a paragraph exports as[a]: b, a link reference definition, so CommonMark renderers show nothing.
The package README promises that default export "protects ... link syntax in text, including constructs split across text nodes". Finding the neighbouring exported character in document order within the block (out of an inline parent, into the next inline element's first text, a line break read as a newline, and a conservative escape next to a decorator) would cover these. Please add them beside the existing split cases in MarkdownEscaping.test.ts.
2. The definition-label check reads text that differs from the output (suggestion): MarkdownExport.ts:737-751
Besides the escaped ] ([a](b]: c exports as [a](b]: c), the doubled backslash of an earlier sibling (a node ending in \ before a ]: b node exports as [a\\]: b) and the unselected text read under $convertSelectionToMarkdownString, a link label may span lines: [foo, a line break, then bar]: /url exports unchanged and is a valid definition, so the paragraph renders as nothing. Checking the label against the escaped output from the start of the block, not the line, covers all four. The same loop rebuilds and rescans the line for every ]:, which is the super-linear export the earlier review measured; scanning back from the ] and stopping at the first bracket or the line start keeps it linear.
3. Task markers at the start of list items (please decide)
A bullet item whose text is [x] done now exports as - [x] done and a numbered item [ ] todo as 1. [ ] todo; GFM renders both as task list items, and with CHECK_LIST among the transformers the bullet reads back as a check-list item. Because CHECK_LIST_REGEX (MarkdownTransformers.ts:223) makes the list marker optional, a paragraph starting with [ ] reads back as a check list too. Escaping a leading task marker in list items, or stating the trade-off in the changeset, would make this deliberate.
4. Changeset and README wording (nit)
Once the above is settled, the changeset's sentence that escaping ]( and a line-start ]: "is enough to keep literal text from parsing as a link, image or link reference definition", and the README line saying literal brackets use numeric references, should describe the final rule; .changeset/README.md notes the entry becomes the CHANGELOG text people upgrading read.
| const nextSibling = getTextSibling(node, false); | ||
| const nextChar = | ||
| nextSibling && (!selection || nextSibling.isSelected(selection)) | ||
| ? $exportedText(nextSibling, selection).charAt(0) | ||
| : ''; |
There was a problem hiding this comment.
Blocking: the character after a node's last ] only comes from a sibling text node, so a ] that ends the text inside an inline element (a comment MarkNode, say) gets '' here. A comment on [a] followed by (b) then exports as [a](b), which $convertFromMarkdownString reads back as a link a → b, and a comment on [a] followed by : b at the start of a paragraph exports as [a]: b, a CommonMark link reference definition that renders as nothing. Walking to the next exported character in document order within the block (out of an inline parent, into the next inline element's first text, a line break as a newline) and escaping conservatively next to a decorator would cover it; $endsDefinitionLabel needs the same walk backwards.
| let line = text.slice(0, index); | ||
| for ( | ||
| let sibling = node.getPreviousSibling(); | ||
| sibling !== null && !line.includes('\n'); | ||
| sibling = sibling.getPreviousSibling() | ||
| ) { | ||
| if ($isLineBreakNode(sibling)) { | ||
| line = '\n' + line; | ||
| } else { | ||
| line = sibling.getTextContent() + line; | ||
| } | ||
| } | ||
| return /^ {0,3}\[(?:[^[\]\\]|\\.)*$/.test( | ||
| line.slice(line.lastIndexOf('\n') + 1), | ||
| ); |
There was a problem hiding this comment.
The label is tested against text that differs from the output: a ] this pass escapes still ends it ([a](b]: c exports as [a](b]: c), earlier siblings contribute raw text (a node ending in \ before a ]: b node exports as [a\\]: b), only the current line is read although a CommonMark label may span lines ([foo, a line break, bar]: /url), and selection export reads unselected text. Each is a valid link reference definition, so the paragraph renders as nothing. Rebuilding and rescanning the line for every ]: is also super-linear on a long line with many styled runs; scanning back from the ] and stopping at the first bracket or the line start would address both.
…ine elements, check definition labels and task markers on exported text, in linear time Addresses review on #9600.
|
Thanks for both reviews. All points are addressed in 29954b9:
I also ran a randomized check against micromark: 246,925 paragraphs mixing text, inline elements, line breaks and links, plus 15,239 selections, all render as the original text with one anchor per link and read back to the same document. Details are in the updated description. |
There was a problem hiding this comment.
Thanks for narrowing this. Escaping only the ] that would close a link, a definition label or a task marker fixes the v0.53.0 regression for text like [1][2], array[0] and done [x], and the new tests cover the rule thoroughly.
Requested change: links exported without brackets (packages/lexical-markdown/src/MarkdownExport.ts: $isInsideLink L829, $isTransparentInline L840, $nextExportedChar L859 and L881, $leafText L944)
These helpers assume every LinkNode is exported as [text](url). The LINK transformer returns null for an AutoLinkNode (MarkdownTransformers.ts:1247), so an autolink is exported as its bare text, and when LINK is not in the transformer list every link is. Text that main escapes then becomes Markdown syntax:
- With
AutoLinkExtensionand the stockautoLinkUrlMatcher, the plain paragraph[see https://example.com today]: https://example.comgets two autolinks and exports unescaped. That is a link reference definition, so CommonMark and GFM renderers show nothing for the paragraph;mainexports[see … today]: …. - An autolink whose text ends in
[a], followed by(b), exports as…[a](b), which$convertFromMarkdownStringreads back as a link. - With a transformer list that lacks
LINK, a link whose text is[a]followed by(b)exports as[a](b)and re-imports as a link, and[+ link +]: /urlexports as a definition.
The README paragraph this PR rewrites says a bracket is escaped wherever it would start a link or a link reference definition, so these cases contradict it. Treating an AutoLinkNode like a transparent inline element in those four places ($isLinkNode(node) && !$isAutoLinkNode(node), matching LINK) fixes the autolink cases without changing any existing expectation. For lists without LINK, pass whether links are written in brackets, or apply the literal rules to link text when they are not. A few $createAutoLinkNode cases next to the across inline elements tests would cover both.
Optional, non-blocking: the task-marker rule only fires when [ starts the line, but CHECK_LIST_REGEX (MarkdownTransformers.ts:223) also accepts a leading - or + marker. A paragraph whose text is - [x] done now exports raw and re-imports with CHECK_LIST as a checked item done, where main kept [x] done as bullet text. Counting an optional -/+ and one space as part of the line start for that rule would keep the earlier result.
|
|
||
| /** Whether `node` is an inline element whose exported text is its children's. */ | ||
| function $isTransparentInline(node: LexicalNode | null): node is ElementNode { | ||
| return $isElementNode(node) && node.isInline() && !$isLinkNode(node); |
There was a problem hiding this comment.
LINK returns null for an AutoLinkNode (MarkdownTransformers.ts:1247), so an autolink is exported as its bare children, exactly like a transparent inline element, but this check (and $isInsideLink, $nextExportedChar and $leafText) treats it as [text](url). With AutoLinkExtension and the stock autoLinkUrlMatcher, the plain paragraph [see https://example.com today]: https://example.com exports unescaped, and CommonMark reads it as a link reference definition that renders nothing. Suggest $isLinkNode(node) && !$isAutoLinkNode(node) in all four places, plus $createAutoLinkNode tests. A transformer list without LINK has the same problem for every LinkNode.
| const atLineStart = | ||
| distance !== -1 && | ||
| state.openerChar !== BARRIER && | ||
| state.openerIndent !== -1; |
There was a problem hiding this comment.
Optional: CHECK_LIST_REGEX (MarkdownTransformers.ts:223) also accepts a leading - or + marker, so a paragraph whose text is - [x] done now re-imports as a checked check-list item done, where main kept [x] done as bullet text. Counting an optional -/+ and one space as part of the line start for the task-marker rule would keep that.
…ts as text, and count block markers as a line start Addresses the second review on #9600.
|
Thanks, both addressed in the latest push:
I extended the randomized micromark check to cover these: 92,000+ paragraphs each with and without |
There was a problem hiding this comment.
Thanks for narrowing the bracket escaping. Exporting [1][2], done [x] and array[0] as typed is the right fix, and computing the line state once per export keeps it linear. One case still needs a change before this lands.
Requested change
A ] after an inline decorator inside a line-start label is not escaped (packages/lexical-markdown/src/MarkdownExport.ts:784-789). When unknown text (a decorator, or a node a text match transformer writes in its own syntax) follows the [, advanceLineState makes it the new opener and recomputes openerAtLineStart from the current phase, which is already text, so the [ that opened the line is forgotten. A paragraph [ + inline decorator (text foo) + ]: done exports [foo]: done, which CommonMark reads as a link reference definition (spec): the paragraph renders as nothing, and a later paragraph see [foo] here renders as a link to the URL typed in the first one. With a decorator whose text is x, [ + decorator + ] todo exports [x] todo, which reads back as a check list item with CHECK_LIST. The PR description and the changeset say a bracket next to a decorator is escaped. Keeping the line start through unknown text fixes all of these:
if (character === '[' || character === BARRIER || character === UNKNOWN) {
const keepsLineStart =
character === UNKNOWN &&
state.openerDistance !== -1 &&
state.openerChar !== BARRIER &&
state.openerAtLineStart;
state.openerDistance = 0;
state.openerChar = character;
state.openerNext = '';
state.openerAtLineStart = state.phase === 'start' || keepsLineStart;
}Please add cases with an inline DecoratorNode inside a bracket pair to MarkdownEscaping.test.ts, next to the TestInlineNode cases.
Optional, non-blocking
- Inline code holding
]:after a line-start[(MarkdownExport.ts:1045):[a+ inline code]:bexports[a`]:b`, which is also a definition, because a link label ends at the first]that is not backslash-escaped and a code span's]cannot be escaped. Only escaping the opening[avoids it, so this needs a look-ahead from a line-start[to a following code span that holds]. - Extra transformer calls (
MarkdownExport.ts:904-918):$exportKindcalls every text match transformer'sexportwith stub callbacks on the parent block and on decorators, whose answer is then ignored. Checking the node type first would keep block nodes away from custom transformers whoseexportrecords state. - Sentinels in typed text (
MarkdownExport.ts:698,:703): U+0001 and U+0002 in a text node are read as a link or unknown text, so[a+ U+0001 +]: /urlexports unescaped. - Changeset wording:
.changeset/markdown-narrow-bracket-escapes.mdsays a bracket next to a decorator is escaped; that holds once the fix above lands. The entry otherwise follows CONTRIBUTING.md. - Test plan: AGENTS.md asks the After section to carry the pasted output of the commands that were run rather than a description.
| if (character === '[' || character === BARRIER || character === UNKNOWN) { | ||
| state.openerDistance = 0; | ||
| state.openerChar = character; | ||
| state.openerNext = ''; | ||
| state.openerAtLineStart = state.phase === 'start'; | ||
| } |
There was a problem hiding this comment.
Unknown text (a decorator, or a node a transformer writes in its own syntax) replaces the opener here and takes openerAtLineStart from the current phase, which is already text after a [. So [ + inline decorator (text foo) + ]: done exports [foo]: done, a link reference definition: the paragraph renders as nothing and a later [foo] renders as a link. With decorator text x, [x] todo reads back as a check list item with CHECK_LIST. Keeping the line start through unknown text fixes it:
| if (character === '[' || character === BARRIER || character === UNKNOWN) { | |
| state.openerDistance = 0; | |
| state.openerChar = character; | |
| state.openerNext = ''; | |
| state.openerAtLineStart = state.phase === 'start'; | |
| } | |
| if (character === '[' || character === BARRIER || character === UNKNOWN) { | |
| // Unknown text inside a line-start label keeps the label's line start. | |
| const keepsLineStart = | |
| character === UNKNOWN && | |
| state.openerDistance !== -1 && | |
| state.openerChar !== BARRIER && | |
| state.openerAtLineStart; | |
| state.openerDistance = 0; | |
| state.openerChar = character; | |
| state.openerNext = ''; | |
| state.openerAtLineStart = state.phase === 'start' || keepsLineStart; | |
| } |
| return UNKNOWN; | ||
| } else if ($isTextNode(leaf)) { | ||
| const text = $exportedText(leaf, selection); | ||
| return leaf.hasFormat('code') ? text.replace(/[[\] \t]/g, '`') : text; |
There was a problem hiding this comment.
Optional: masking a code span's ] hides a label end that cannot be escaped. [a + inline code ]:b exports [a`]:b` ; a link label ends at the first ] that is not backslash-escaped and definitions are read before code spans, so this is a definition and the paragraph renders as nothing. Only escaping the opening [ (a look-ahead to a following code span holding ]) avoids it.
| const isWritten = context | ||
| ? context.textMatchTransformers.some(transformer => { | ||
| try { | ||
| return ( | ||
| transformer.export !== undefined && | ||
| transformer.export( | ||
| node, | ||
| () => '', | ||
| () => '', | ||
| ) != null | ||
| ); | ||
| } catch { | ||
| return true; | ||
| } | ||
| }) |
There was a problem hiding this comment.
Optional: this runs every text match transformer's export with stub callbacks on whatever node is asked about, including the parent block (from $isInsideLink and $nextExportedChar) and decorators, where the answer is ignored below. Checking $isTextNode / inline element first would keep block nodes and extra calls away from custom transformers whose export records state.
…ator, escape a [ that inline code or a later link would close, and read typed control characters as text Addresses the third review on #9600.
|
Addressed in d1fd7cf:
My randomized check also found one more round trip issue: a Tests: 901 |
There was a problem hiding this comment.
Thanks, this looks good. Escaping a bracket only where it would become syntax fixes the [1] to [1] churn introduced in v0.53.0, the new MarkdownEscaping.test.ts cases fail on main and pass here, and starting LINK at the real opener is a worthwhile import fix on its own. Everything below is optional and non-blocking; the first two are small and would be nice to close before merge.
1. Inline code holding ]( after a raw [ (packages/lexical-markdown/src/MarkdownExport.ts:868-876). A paragraph of see [ + inline code ]( + x) now now exports see [`](`x) now. A CommonMark reader keeps the text, because code spans bind tighter than link brackets (https://spec.commonmark.org/0.31.2/#code-spans), but $convertFromMarkdownString reads [`](`x) as a link to `x, so the round trip becomes see ` now plus a link. The export only looks into inline code for a line-start ]:, and matchLink (importTextMatchTransformer.ts:32) cannot see the code span because only its opening backtick sits inside the lazy label. Either escape a raw [ whose next bracket on the line is a ] inside code followed by (, or have matchLink reject a match whose ]( falls inside a code span of the whole line.
2. An escaped ] does not end the inline-code label check (MarkdownExport.ts:894-896). [a](b) + inline code ]:x exports [a](b) `]:x` , which a CommonMark reader takes as a link reference definition (definitions are read before inline content, https://spec.commonmark.org/0.31.2/#link-reference-definitions), so the paragraph renders as nothing. The same happens with [] or [x] see + code ]:url, and with [a]: b + code ]:c. $labelClosesInCode stops at the first raw ] even when this pass escapes it; deciding the ] escapes first and skipping escaped positions would match how the ] check already treats an escaped ] as still inside the label. A test for each of these two cases would pin the fix.
3. Probe calls to TextMatchTransformer.export (MarkdownExport.ts:992-1006). Export now calls each text match transformer with placeholder callbacks to classify nodes, on top of the real call, so a transformer that keeps state in export changes its output (for example, one that numbers reference-style links counts each link twice). The JSDoc now asks for a side-effect-free export; since AGENTS.md's Backwards Compatibility section asks not to change existing public API behavior in ways that can break callers, please also name this in the changeset's Verify: paragraph so people upgrading see it.
4. Bracket syntax written by a custom transformer (MarkdownExport.ts:1012-1015). Only a LinkNode counts as [text](url). Text inside a non-link inline element that a custom transformer writes as [children](url) gets the outside-a-link rules, so child text a] exports [a]](url) and the link is lost (it was [a]](url) before). In the other direction, a LinkNode that a custom transformer writes in another syntax, such as an HTML anchor, still counts as a barrier, so [ + that link + ]: /x becomes a definition. Classifying by the probe's returned string (a leading [) and applying the link-text rules inside such an element would cover both.
Nits
nextLinePhaseassumes unknown text holds no line break (MarkdownExport.ts:751-754), so an inline decorator whose text ends in a newline, followed by[x] done, re-imports as a check list withCHECK_LIST.- The linear-time cases in
MarkdownEscaping.test.tsassert a 2,000 ms wall-clock bound, which can flake on a busy CI runner.
| } else if ( | ||
| character === '[' && | ||
| state.phase === 'start' && | ||
| $labelClosesInCode(text, index, $labelClosesAfter) | ||
| ) { | ||
| // The `]` that would close this label is in inline code, where it | ||
| // cannot be escaped, so the `[` is. | ||
| escaped.add(index); | ||
| } |
There was a problem hiding this comment.
Optional: this only looks into inline code for a line-start ]:. A paragraph of see [ + inline code ]( + x) now exports see [`](`x) now, which a CommonMark reader keeps as text (code spans bind tighter than link brackets, https://spec.commonmark.org/0.31.2/#code-spans), but $convertFromMarkdownString reads [`](`x) as a link to `x, so the round trip becomes see ` now plus a link. Escaping a raw [ whose next bracket on the line is a ] inside code followed by ( (or rejecting such a match in matchLink) would keep the round trip.
| for (let next = index + 1; next < text.length; next++) { | ||
| if (text[next] === '[' || text[next] === ']') { | ||
| return false; |
There was a problem hiding this comment.
Optional: this stops at the first raw ] even when this pass escapes it. [a](b) + inline code ]:x exports [a](b) `]:x` , which a CommonMark reader takes as a link reference definition, so the paragraph renders as nothing; [] or [x] see + code ]:url behave the same. Deciding the ] escapes first and skipping escaped positions here would match how the ] check treats an escaped ] as still inside the label.
| : isWritten | ||
| ? $isLinkNode(node) | ||
| ? 'link' | ||
| : 'unknown' |
There was a problem hiding this comment.
Optional: only a LinkNode counts as written in [text](url). A non-link inline element that a custom transformer writes as [children](url) now leaves child text such as a] unescaped ([a]](url), the link is lost), and a LinkNode written in another syntax still counts as a barrier. The probe already returns the written string, so classifying by its shape (a leading [) would cover both.
…re links inside code spans on import, and classify transformer output by its syntax Addresses the optional points of the fifth review on #9600.
|
Thanks for the approval. I took the optional points as well, in 18e9a95:
All eight new cases fail on 97a7d0e and pass here. 918 |
There was a problem hiding this comment.
This looks good. Escaping only the brackets that would otherwise become syntax brings [1], array[0] and done [x] back as typed. The new MarkdownEscaping.test.ts cases fail on main and pass here, and starting LINK import at the real opener fixes formatting before a link. The two suggestions below are optional and non-blocking.
1. Text inside an inline element that a custom transformer writes (optional). When $nextExportedChar leaves a parent that is neither transparent nor a link, it returns '' (packages/lexical-markdown/src/MarkdownExport.ts:1080-1081). $previousLeaf returns null at the start of such a parent (:1147). Either way, the text there is read as if the block ended or began. Take a text match transformer that writes an inline element as ${exportChildren(node)}(note): see [ followed by that element holding a] exports as see [a](note), and $convertFromMarkdownString reads that back as a link (on main it was see [a](note)). With a : note suffix, an element holding [a] at the start of a paragraph exports as [a]: note, which CommonMark reads as a link reference definition (https://spec.commonmark.org/0.31.2/#link-reference-definitions) and renders as nothing. If crossing such a parent reported UNKNOWN, the PR's own rule for brackets next to a node written in its own syntax would apply from the inside too.
2. The code-span check in matchLink (optional). isInCode scans every code span for each candidate match (packages/lexical-markdown/src/importTextMatchTransformer.ts:31-48), and the matcher runs again on the rest of the line after each code span is applied. A single line with thousands of code spans that hold link syntax therefore imports much more slowly than on main: [a`](x)` repeated 4,000 times takes about 3 s on main and about 40 s here. scanCodeSpans returns spans in order, so a binary search or an advancing pointer would keep this at main's cost. A timing case next to the export ones would keep it there.
Thanks for documenting the extra export calls with placeholder callbacks in the TextMatchTransformer.export JSDoc and in the changeset's Verify: paragraph, as CONTRIBUTING.md asks for a fix that changes observable behavior. I'm treating that as an intentional, disclosed trade-off.
| return ']'; | ||
| } | ||
| if (!$isTransparentInline(parent)) { | ||
| return ''; |
There was a problem hiding this comment.
Optional: leaving a parent that a text match transformer writes in its own syntax (neither transparent nor a link) returns '' here, as if the block ended, and $previousLeaf does the same at its start (line 1147). Take a transformer that writes an inline element as ${exportChildren(node)}(note): see [ followed by that element holding a] exports as see [a](note) and imports back as a link. Returning UNKNOWN when the walk crosses such a parent would escape that edge the way a decorator's is.
| // falls inside one is not a link. | ||
| const codeSpans = text.includes('`') ? scanCodeSpans(text) : []; | ||
| const isInCode = (index: number) => | ||
| codeSpans.some( |
There was a problem hiding this comment.
Optional: codeSpans.some is a linear scan for each check, and matchLink runs again on the rest of the line after each code span is applied. A line with thousands of code spans holding link syntax therefore imports much more slowly than on main ([a`](x)` repeated 4,000 times: about 3 s on main, about 40 s here). scanCodeSpans returns spans in order, so a binary search would make each check logarithmic.
…sformer writes as unknown text, and find code spans by binary search on import Addresses the optional points of the sixth review on #9600.
… keeps the parent's type Fixes the type check of dec73bd.
|
Took both optional points in dec73bd (plus db8da5b, which fixes a type narrowing error in dec73bd):
918 → 921 |
There was a problem hiding this comment.
This looks good to me. Escaping only the ] that would actually close a link, a line-start definition label or a task marker fixes the v0.53.0 rewrite of [1], [x] and array[0], and starting LINK import at the real opener fixes formatting before a link. The new tests pin both directions. The points below are optional and non-blocking.
- Lines that start with Unicode whitespace (
packages/lexical-markdown/src/MarkdownExport.ts:755).nextLinePhasetreats only a space or a tab as indentation, butCHECK_LIST_REGEX(MarkdownTransformers.ts:228) matches any\s. A paragraph\u00A0[x] doneor\u3000[ ] todo(a full-width indent, common in CJK text), or a bullet item whose text is\u00A0[x] done, now exports with the marker raw and re-imports withCHECK_LISTas a check list item whose text is justdoneortodo; onmainall three round-trip. Changing that line toconst isSpace = /\s/.test(character);escapes them (\u00A0[x] done) and keeps the existing tests passing. Adding the three cases to the task-marker tests would pin it. - Code spans full of
[on import (packages/lexical-markdown/src/importTextMatchTransformer.ts:60). When a match's[is inside a code span, the loop retries frommatch.index + 1, and each retry re-scans to the same](. A single line holding a code span of 80,000[followed by](u)takes about 3 s to import, against about 1 ms onmain. MovinglastIndexto the end of the code span that holdsmatch.index(the binary search already finds it) makes this linear. - Extra calls into custom transformers. Export now calls each
TextMatchTransformer.exportwith placeholder callbacks to classify a bracket's neighbours. The JSDoc and the changeset'sVerify:paragraph already say so, as AGENTS.md's backwards-compatibility section asks; worth keeping visible in the release notes, since a transformer that numbers links or footnotes insideexportwill see extra calls.
| // Unknown text may end with a line break, so what follows may start a line. | ||
| return 'start'; | ||
| } | ||
| const isSpace = character === ' ' || character === '\t'; |
There was a problem hiding this comment.
Optional: CHECK_LIST_REGEX (MarkdownTransformers.ts:228) accepts any \s as indentation, so a paragraph \u00A0[x] done or \u3000[ ] todo, or a bullet item \u00A0[x] done, now exports with the marker raw and re-imports with CHECK_LIST as a check list item (done/todo); main round-trips them. const isSpace = /\s/.test(character); escapes them and keeps the existing tests green.
| isInCode(labelEnd) || | ||
| isInCode(match.index + match[0].length - 1) | ||
| ) { | ||
| search.lastIndex = match.index + 1; |
There was a problem hiding this comment.
Optional: when match.index is inside a code span, each retry from match.index + 1 re-scans to the same ](, so a code span of 80,000 [ before ](u) takes about 3 s to import (about 1 ms on main). Jumping lastIndex to that span's endIndex makes it linear.
… and skip a whole code span when a link candidate opens inside it Addresses the optional points of the seventh review on #9600.
|
Both taken in 8189fcc:
924 |
…ts in the test Avoids a CodeQL incomplete-replacement finding on the test helper.
There was a problem hiding this comment.
This looks good. Narrowing the bracket escapes to the places where CommonMark or GFM would read a bracket as syntax fixes the [1] / [x] / array[0] churn on every save, and starting an imported link at the [ that really opens it fixes the formatting lost before such links. The new tests cover both directions. The suggestion below is optional and non-blocking.
Optional: treat a lone CR as a line ending (packages/lexical-markdown/src/MarkdownExport.ts:789)
advanceLineState returns to line start only on \n, and nextLinePhase reads \r as ordinary whitespace, but CommonMark counts a bare carriage return as a line ending (https://spec.commonmark.org/0.31.2/#line-ending). A lone \r can sit inside a TextNode (plain-text paste splits only on \n and \r\n), and for such text the export now leaves brackets raw where other CommonMark readers see syntax:
foo\r\r[a]: /urlis read as a link reference definition, so the line disappears;see [a]\r> [a]: https://example.comturns[a]into a link;notes\r- [x] donebecomes a GFM task list item.
Lexical's own import is unaffected, since it splits lines on \n. Treating \r like \n in advanceLineState, with a test for each case above, would close the gap.
Thanks also for documenting the new placeholder export calls in the TextMatchTransformer.export JSDoc and in the changeset's Verify: paragraph; that is what authors of custom transformers need to know.
| state.openerDistance = -1; | ||
| } | ||
| } | ||
| if (character === '\n') { |
There was a problem hiding this comment.
Optional, non-blocking: CommonMark treats a bare \r as a line ending (https://spec.commonmark.org/0.31.2/#line-ending), and a lone \r can sit inside a TextNode (plain-text paste splits only on \n and \r\n). Only \n resets the line here, so notes\r- [x] done exports unescaped and renders as a task list item, and foo\r\r[a]: /url as a link reference definition. Treating \r like \n (if (character === '\n' || character === '\r')) would close it.
## Description Merges main at c0c7e42 (#9640, #9600, #9652, #9659, #9648 and #9657). The one conflict is in the link list of `packages/lexical-y/DECISIONS.md`, where both sides added a reference (#9597 here, #9606 on main); both are kept. ## Test plan ### Before The branch was on main f59f74d. ### After `pnpm run ci-check` and `pnpm exec vitest run --project unit --project scripts-unit` pass on the merged tree (outputs in the pull request).
Description
Since #9481 the Markdown export writes every literal
[and]as[and], so ordinary text such ascleared review [1][2],done [x]orarray[0]changes on every save tocleared review [1][2]. CommonMark renders both forms the same, but the stored text changes, which breaks callers that read the exported string (citation regexes such as/\[(\d+)\]/, exact round-trip checks).A literal bracket only needs escaping where it would otherwise become syntax, and escaping its
]is enough in each case:]followed by(closes the text of an inline link or image (https://spec.commonmark.org/0.31.2/#inline-link);]followed by:closes the label of a link reference definition when its[starts a line, possibly inside a block container (https://spec.commonmark.org/0.31.2/#link-reference-definitions). The label may span lines, and a]this pass escapes, or one written by a doubled backslash, does not end the search, so the check errs towards escaping;[ ],[x]or[]at the start of a line (or after a block marker such as-,1.or>), followed by whitespace, reads back as a check list item withCHECK_LIST(whose list marker is optional) and as a task list item in GFM. This keeps a bullet item or paragraph whose text starts with[ ]from turning into a check list, which [lexical-markdown] Bug Fix: Preserve literal text and escaped link brackets in Markdown round trips #9481 also prevented.A
[is escaped only when the label it starts at a line start would close at a]inside inline code followed by:, which cannot be escaped.On import,
LINKnow starts a link at the[that really opens its text. Its lazy label pattern runs from the first[on the line, and the text before the real opener used to be kept as raw source, so[1] **bold** [link](url)lost its bold, whether written by export or by hand. That text now goes through the other transformers.Inside link text, brackets are allowed as matched pairs (https://spec.commonmark.org/0.31.2/#link-text); unmatched brackets, and a pair followed by
((which would read as a nested link), are escaped, and other matched pairs stay as typed, so[[1]](url)exports unchanged.The text on either side of a bracket is read the way it is exported: across text nodes and through inline elements whose children are exported in place, such as comment marks and autolinks (or every link when
LINKis not among the transformers); a line break as a newline; a link written as[text](url)as text that cannot hold a label; inline code with its brackets masked; and the start of an exported selection as a line start. Whether a node is written in its own syntax is decided by asking the export's text match transformers. A bracket next to a decorator, or next to a node a transformer writes in its own syntax, is escaped. The state of the text before each node is computed once per export and reused, so export stays linear: 8,000 styledxa]:nodes export in tens of milliseconds (53 ms onmain, 39.8 s on the first version of this PR).The
<and&escaping from #9481, and import of the[/]output written since v0.53.0, are unchanged.Changeset
.changeset/markdown-narrow-bracket-escapes.md([fix],@lexical/markdownpatch, with aVerify:paragraph).Test plan
Before
pnpm exec vitest run --project unit packages/lexical-markdown/src/__tests__/unit/MarkdownEscaping.test.ts -t 'brackets that cannot become Markdown syntax'onmain:After
The new
MarkdownEscaping.test.tscases cover each review point:], across a line break and at the start of an exported selection;CHECK_LISTas text, while a real check list still exports as- [ ] todo) and after block markers (- [x] done,> [a]: b);LINK;Claim [1] is *important*, see [source](url)), checking the imported nodes' formats and links;Randomized checks against micromark 4.0.2 (local scripts, not committed):
Each paragraph must render as its original text, with no definition swallowing text, no task list input (GFM task list extension), and one anchor per link written in brackets. It must also read back through
$convertFromMarkdownStringwithCHECK_LISTwith no check list, the same links and the same text.mainpasses the same checks; the first revision of this PR fails the token check within a few thousand cases.In Meta's internal www, with this build in the next Lexical sync, 2,898 Lexical-dependent Jest suites run. Two fail, and they also fail on the base revision. MetaCRM's and Rhythm's citation round trips pass again; they fail with v0.53.0 alone. Rhythm's own renderer is being updated separately to read a task marker written as
[ ].Browser and E2E suites were not run; the change is limited to Markdown export string generation.