Repository navigation
[lexical-mdast] Bug Fix: A block shortcut no longer strips the lines after its marker when the text holds a newline - #9776
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
|
AI review status for this pull request.
|
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: |
![meta-code-review[bot]](https://avatars.githubusercontent.com/in/5334748?s=60&v=4){kind=small}
There was a problem hiding this comment.
This looks good to me. The guard in MarkdownStreamScanner.scanBlock stops the block shortcuts from deleting the lines after a marker when a text node holds a newline, and the new tests reproduce the lost text on main and pass here. A few optional, non-blocking suggestions:
- A line break before the marker (
packages/lexical-mdast/src/MdastStream.ts:124). The marker is measured from the start of the text, so a marker typed after a leading line break in the same text node no longer converts, wheremainconverted it. Examples are\nfollowed by a typed#, or#\nfollowed by Enter; CommonMark 4.9 ignores leading blank lines, so\n#is a heading. The test atMdastShortcutsMultilineText.test.ts:159asserts the new behavior, but the changeset (.changeset/mdast-shortcut-multiline-text.md:7) says typing a marker on its own line is unchanged. Either measure the line break from the construct's own start (first.position.start.offset), which keepsmain's behavior there, or add a sentence to the changeset that describes it;.changeset/README.mdasks each entry to describe the change from the user's side. - Space for the other markers (
MdastShortcutsMultilineText.test.ts:114). Onmain, a Space typed after#\nvaluable(or-,>,1.) also deletedvaluable, as the changeset says, but only Enter is tested for those markers. Adding Space to thatit.eachwould cover the Space branch inMdastShortcuts.ts. - The
- [ ]case passes onmaintoo:- [ ]\nvaluableparses as a task item whose paragraph continues lazily intovaluable. It guards against a regression but doesn't show the fix. - Test environment header. AGENTS.md (Testing Strategy) asks for
// @vitest-environment nodeafter the copyright header in tests that only exercise editor state, as the sibling tests inpackages/lexical-mdast/src/__tests__/unit/have it.
| // first line (a rule, a fence, a heading or list item with no content) | ||
| // span the whole text as its marker, and the lines after it would be | ||
| // stripped with it. | ||
| return match !== null && /[\n\r]/.test(line.slice(0, match.markerLength)) |
There was a problem hiding this comment.
Optional: markerLength is measured from offset 0, so a line break before the construct counts too. A marker typed after a leading \n in the same text node (\n then # , or #\n + Enter) no longer converts, where main converted it. Checking only from first.position.start.offset would keep main's behavior there. Otherwise, the changeset's line 7 ("Typing a marker on its own line … unchanged") could mention it.
There was a problem hiding this comment.
Done in 289aa20: the line break is now looked for only from the construct's own start (position.start.offset) to the end of its marker, so a marker after leading blank lines in the same text node converts as on main. The changeset says so, and a test covers it.
| }); | ||
| }); | ||
|
|
||
| it.each(['#', '-', '>', '```\n```', '1.', '- [ ]'])( |
There was a problem hiding this comment.
Optional: on main, a Space typed after #\nvaluable (and -, >, 1.) also deleted valuable, but these markers are tested with Enter only. A Space case here would cover the Space branch in MdastShortcuts.ts. The - [ ] entry passes on main too: it parses as a task item whose paragraph continues lazily into valuable.
There was a problem hiding this comment.
Done in 289aa20: Space is now tested after #, -, > and 1. followed by a line, and the - [ ] case, which passes on main, is removed.
| * LICENSE file in the root directory of this source tree. | ||
| * | ||
| */ | ||
|
|
There was a problem hiding this comment.
Nit: AGENTS.md (Testing Strategy) asks for // @vitest-environment node after the copyright header in tests that only exercise editor state, as the other shortcut tests in this directory have it.
There was a problem hiding this comment.
Done in 289aa20: the file now has // @vitest-environment node.
…after its marker when the text holds a newline
## Description
A text node can hold a newline when app code inserts text that contains one (for example `selection.insertText('---\nnotes')`; typing and pasting split lines into line breaks or paragraphs instead). A space or Enter typed at the end of such a node is scanned by `MarkdownStreamScanner.scanBlock` as markdown, and an empty construct on its first line (a thematic break `---`, `***`, `___`, an empty fence, or a heading, list item or quote with no content) reported the whole text as its marker. The shortcut then turned the paragraph into a horizontal rule or an empty heading, list item, quote or code block and deleted the lines after the marker. The thematic-break case on Space is new with #9484; the Enter cases and the other markers on Space are older.
Now `scanBlock` does not report a construct whose marker, from the construct's own start, would span a line break (`\n` or `\r`), so such text stays as it is. A marker typed on its own line (also after blank lines in the same text node, which are not part of the construct), and Enter with content after the marker in a later text node, are unchanged.
## Changeset
`.changeset/mdast-shortcut-multiline-text.md` (`@lexical/mdast` patch, `[fix]`).
## Test plan
- `packages/lexical-mdast/src/__tests__/unit/MdastShortcutsMultilineText.test.ts` (new, 25 tests): Space and Enter after `---`, `***` and `___` followed by a line in the same text node; Enter and Space after an empty `#`, `-`, `>` and `1.` (Enter also after an empty fence) followed by a line; `\r` and `\r\n`; a quote marker spanning two lines with content in the next text node; markers alone on their line still convert, also after a blank line; Enter with a newline only in a later text node still converts as before.
### Before
```
$ ./node_modules/.bin/vitest --run --project unit packages/lexical-mdast/src/__tests__/unit/MdastShortcutsMultilineText.test.ts
AssertionError: expected '\n' to be '---\nvaluable ' // Object.is equality
AssertionError: expected '\n' to contain '---\nvaluable'
AssertionError: expected '' to contain '>\nvaluable'
Tests 18 failed | 7 passed (25)
```
### After
```
$ ./node_modules/.bin/vitest --run --project unit packages/lexical-mdast/src/__tests__/unit/MdastShortcutsMultilineText.test.ts
Tests 25 passed (25)
```
The `@lexical/mdast` and `@lexical/markdown` unit tests pass with no change from main. A differential of 1,386 cases (33 markers, including headings, quotes, every list marker, task items, fences and thematic breaks, in 14 layouts: one line, a newline inside the text node after or before the marker, one or two blank lines or `\r\n` before it, a line break node after or before it, text after or before it on the same line, typed character by character; then Space, Enter or nothing) gives the same editor state as main in every case except the 136 whose text node holds a newline after the marker, where main deleted the following line in 88. The playground does not use `@lexical/mdast`, so no e2e test covers it; no browser was run.
## Review
Independent review with gpt-6.1-sol over three rounds (the last clean); the first found that a newline after the caret, in a later text node, changed Enter, which this keeps as before.
dab3e5c to
289aa20
Compare
There was a problem hiding this comment.
This looks good to me. Looking for a line break only between the construct's own start and the end of its marker in MarkdownStreamScanner.scanBlock stops the Space and Enter shortcuts from deleting the lines after a marker, and markers after leading blank lines still convert as they do on main. The new tests reproduce the lost text on main (18 of 25 fail there) and pass here, and the changeset follows .changeset/README.md, Verify: paragraph included.
One optional, non-blocking suggestion:
- Test name (
packages/lexical-mdast/src/__tests__/unit/MdastShortcutsMultilineText.test.ts:199). Onmain,>\n>followed byvaluablein the next text node became a quote holdingvaluable, so nothing after the marker was lost there; this test pins the new decision not to convert a marker that spans two lines. A name such as "a quote marker spanning two lines no longer converts on Enter" would say what it checks.
| ); | ||
| }); | ||
|
|
||
| it('a marker spanning a line break keeps the text after it on Enter', () => { |
There was a problem hiding this comment.
Optional: on main this case already kept valuable (it became a quote holding valuable, losing only the >\n> marker), so the test pins the new decision not to convert a marker that spans two lines rather than a lost line. A name such as a quote marker spanning two lines no longer converts on Enter would say what it checks.
There was a problem hiding this comment.
Right: on main this case kept valuable and dropped only the two-line quote marker and its line break; the test pins the decision that a marker spanning a line break does not convert. Not renamed here, to keep the head that was verified; the name will say that if the file changes again.
Description
A text node can hold a newline when app code inserts text that contains one (for example
selection.insertText('---\nnotes'); typing and pasting split lines into line breaks or paragraphs instead). A space or Enter typed at the end of such a node is scanned byMarkdownStreamScanner.scanBlockas markdown, and an empty construct on its first line (a thematic break---,***,___, an empty fence, or a heading, list item or quote with no content) reported the whole text as its marker. The shortcut then turned the paragraph into a horizontal rule or an empty heading, list item, quote or code block and deleted the lines after the marker. The thematic-break case on Space is new with #9484; the Enter cases and the other markers on Space are older.Now
scanBlockdoes not report a construct whose marker, from the construct's own start, would span a line break (\nor\r), so such text stays as it is. A marker typed on its own line (also after blank lines in the same text node, which are not part of the construct), and Enter with content after the marker in a later text node, are unchanged.Changeset
.changeset/mdast-shortcut-multiline-text.md(@lexical/mdastpatch,[fix]).Test plan
packages/lexical-mdast/src/__tests__/unit/MdastShortcutsMultilineText.test.ts(new, 25 tests): Space and Enter after---,***and___followed by a line in the same text node; Enter and Space after an empty#,-,>and1.(Enter also after an empty fence) followed by a line;\rand\r\n; a quote marker spanning two lines with content in the next text node; markers alone on their line still convert, also after a blank line; Enter with a newline only in a later text node still converts as before.Before
After
The
@lexical/mdastand@lexical/markdownunit tests pass with no change from main. A differential of 1,386 cases (33 markers, including headings, quotes, every list marker, task items, fences and thematic breaks, in 14 layouts: one line, a newline inside the text node after or before the marker, one or two blank lines or\r\nbefore it, a line break node after or before it, text after or before it on the same line, typed character by character; then Space, Enter or nothing) gives the same editor state as main in every case except the 136 whose text node holds a newline after the marker, where main deleted the following line in 88. The playground does not use@lexical/mdast, so no e2e test covers it; no browser was run.Review
Independent review with gpt-6.1-sol over three rounds (the last clean); the first found that a newline after the caret, in a later text node, changed Enter, which this keeps as before.