Repository navigation
[lexical-text] Bug Fix: preserve NodeState across ambiguous repeated-character insertions - #9730
wozhenbang2004 wants to merge 10 commits into
Conversation
…rtions ## Description Compare the common suffix independently of the prefix so repeated-character alignment ambiguity does not strip existing NodeState. Closes meta#9725. Regression tests and the package changeset are prepared locally and will follow on this branch. ## Test plan ### Before pnpm exec vitest run --project unit packages/lexical-text/src/__tests__/unit/registerLexicalTextEntityRepeatedText.test.ts AssertionError: expected '' to be 'c1' Tests 2 failed (2) ### After pnpm exec vitest run --project unit packages/lexical-text/src/__tests__/unit packages/lexical-hashtag/src/__tests__/unit Test Files 11 passed (11) Tests 110 passed (110) pnpm run tsc and pnpm run tsc-test exit 0. ESLint and Prettier pass. Full-unit runs terminated with exit 137; browser/E2E and Flow not run locally.
## Description Add two regression cases for meta#9725: inserting before either old A in $AA must preserve NodeState on the old final character when it becomes an entity. No workflow changes. ## Test plan ### Before pnpm exec vitest run --project unit packages/lexical-text/src/__tests__/unit/registerLexicalTextEntityRepeatedText.test.ts AssertionError: expected '' to be 'c1' Tests 2 failed (2) ### After pnpm exec vitest run --project unit packages/lexical-text/src/__tests__/unit packages/lexical-hashtag/src/__tests__/unit Test Files 11 passed (11) Tests 110 passed (110) pnpm run tsc and pnpm run tsc-test exit 0. Targeted lint/format pass. Full-unit runs exited137; browser/E2E and Flow not run.
## Description Add the required @lexical/text patch changeset for the NodeState fix in meta#9725, including the observable behavior and verification scenario. ## Test plan ### After pnpm run check-changesets Changeset check passed: 81 pending changeset(s) valid. The related lexical-text and hashtag unit tests pass: Test Files 11 passed (11), Tests 110 passed (110).
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
There was a problem hiding this comment.
Thanks for picking up #9725. The analysis of the ambiguous alignment is right, and the new test does fail without this change and pass with it. One behaviour regression needs fixing before this can land.
Requested change: the new rule also catches insertions the caret already settles
packages/lexical-text/src/registerLexicalTextEntity.ts, lines 280-286
Finding the suffix independently makes the inserted range the intersection of every alignment the text allows. When the inserted text ends with the same characters the node already ended with, that range shrinks even though the caret shows exactly where the insertion went. With the stock HashtagExtension and MarkStateExtension in a rich-text editor, a comment c1 on foo in see foo, and a plain-text paste of #foo at the comment's end:
main:"see " "foo"[c1] " " "#foo"- this PR:
"see " "foo"[c1] " " "#foo"[c1]
The pasted hashtag joins the comment (skipping the space before it), while main and the MarkNode flavor leave it out. That goes against the contract in the function's own doc comment (lines 247-251) and the existing tests "text inserted after a marked hashtag is not marked, like MarkNode" (packages/lexical-mark/src/__tests__/unit/MarkStateExtension.test.ts) and "a hashtag made only of text inserted into a node does not take its NodeState" (registerLexicalTextEntityNodeState.test.ts), which only insert non-repeating text and so stay green. Plain NodeState behaves the same: with note: 'c1' on tag, inserting #tag at its end now gives the new hashtag c1.
Suggested fix: resolve the alignment with the caret, as getInsertedRange in packages/lexical-mark/src/MarkStateExtension.ts already does, and keep your prefix/suffix intersection as the fallback when the collapsed caret is not in the node. Read the caret offset at the start of $textNodeTransform (before splitText moves it) and pass it to $isInsertedText; when text.slice(caret) is the end of prevText, the inserted range is [min(prefix, caret - (text.length - prevText.length)), caret). A quick sketch of this keeps your offset 1 and 2 cases carrying c1, leaves a typed A at the end of $AA without it, and passes the existing lexical-text, lexical-hashtag and lexical-mark unit suites.
Tests (suggestions)
- Add offset 3 to the
test.eachinregisterLexicalTextEntityRepeatedText.test.ts(typingAat the end of$AA, where the typed character is the one that becomes the entity) and assert the outcome you intend; with this PR it currently getsc1. - Add a repeated-tail paste case like the one above, modeled on the paste test in
MarkStateExtension.test.ts, so the edge contract is pinned with repeated text too.
The changeset follows CONTRIBUTING.md (patch, [fix], Verify:); if the rule changes, update its summary to match. The CI workflows have not run on this head yet and need a maintainer to approve them.
| // preserves the state of characters that may have existed before the edit. | ||
| while ( | ||
| suffix < max - prefix && | ||
| suffix < max && |
There was a problem hiding this comment.
This makes the inserted range the intersection of every alignment the text allows, which also catches insertions the caret already settles: pasting #foo right after a MarkStateExtension comment on foo now puts the pasted hashtag in the comment (on main, and with MarkNode, it stays out). Could you resolve the alignment with the caret first, as getInsertedRange in packages/lexical-mark/src/MarkStateExtension.ts does, and fall back to this intersection only when the collapsed caret is not in the node? The caret offset needs to be read at the start of $textNodeTransform, before splitText moves it.
| : null; | ||
| }; | ||
|
|
||
| test.each([1, 2])( |
There was a problem hiding this comment.
Suggestion: add offset 3 (typing A at the end of $AA, where the typed character is the one that becomes the entity) and a paste of text whose tail repeats the node's tail (for example #foo after a comment on foo), so the intended outcome of these edge cases is pinned.
Ensure caret resolves repeated-character entity insertions correctly without copying NodeState.
Description: Synchronize with upstream main a983160. The upstream lookbehind regression now succeeds with this PR, so replace test.fails with test without changing its assertions. Update the obsolete known-limit note to describe the caret-based insertion detection. Do not alter the MarkState assertions. Test plan: Before this compatibility change, the synchronized targeted suites report 472 passed and 2 failed, including an unexpected pass of test.fails. After: 473 passed and 1 failed across 17 files in the lexical-text, lexical-hashtag, and lexical-mark unit directories. The independent lookbehind file passes both tests; Prettier and targeted ESLint pass. Known blocker: the existing multi-hashtag paste test still fails because the space between the new hashtags inherits the preceding comment's mark IDs. Both hashtags correctly remain unmarked. This also reproduces without the caret fix; it is not fixed or hidden by this commit. Full monorepo and browser/E2E suites have not been run.
There was a problem hiding this comment.
Thanks for reworking this around the caret. The core change in registerLexicalTextEntity.ts now reads an insertion the way getInsertedRange in packages/lexical-mark/src/MarkStateExtension.ts already does: #9725's offsets 1 and 2 keep c1, a typed A at the end of $AA does not take it, and a hashtag pasted right after a comment stays out of the comment again. One thing needs fixing before this can merge.
Requested change: a newly added test case fails
packages/lexical-mark/src/__tests__/unit/MarkStateExtension.test.ts, lines 934-940 ('pasted repeated tail with multiple hashtags'), fails on this branch:
expected: paragraph("see"[c1] " " "#first" " " "#see" " more")
received: paragraph("see"[c1] " " "#first" " "[c1] "#see" " more")
Both hashtags correctly stay out of the comment, but the space between them keeps c1. main behaves the same, and so does any multi-hashtag paste after a comment ( #first #tag, #a #b #c), so your change does not cause it; as written, though, the unit job fails, and CONTRIBUTING.md asks that the test suite pass. Either:
- fix the space handling in
MarkStateExtension(theMarkNodeflavor leaves that space unmarked), or - move that case into its own
test.failsthat names a tracking issue, the way #9727 recorded #9725. With that case removed, the rest of the test passes.
Please also paste the lexical-mark run under the Test plan's After heading; AGENTS.md asks for the real output of the commands you ran.
Suggestions (non-blocking)
packages/lexical-text/src/registerLexicalTextEntity.ts, lines 279-285: the caret branch checks only the text after the caret, so an edit made by code earlier in the node, with the caret kept after the word, is read as running up to the caret. Withnote: 'c1'onsee foo bar, the caret afterfoo, thenspliceText(4, 0, '#')andselect(8, 8),maingives#foothec1and this branch drops it. Taking the branch only whenprefix >= caret - (text.length - prevText.length)(and using that value as the start) restoresmain's result there and keeps all of your new cases passing..changeset/lexical-text-entity-node-state.md, line 10, still says "One case still drops NodeState … (#9725)". Both changesets ship in the same release, so please remove that paragraph; otherwise the changelog calls the case both broken and fixed.packages/lexical-website/docs/concepts/transforms.md, lines 196-199: consider opening with what the rule decides (whether a new entity keeps the NodeState of the text it was made from) before how it decides it.
The CI workflows have not run on this head yet and need a maintainer to approve them.
| 'pasted repeated tail with multiple hashtags', | ||
| 3, | ||
| ' #first #see', | ||
| 'paragraph("see"[c1] " " "#first" " " "#see" " more")', |
There was a problem hiding this comment.
This case fails on this branch: the result is paragraph("see"[c1] " " "#first" " "[c1] "#see" " more"), because the space between the two pasted hashtags keeps c1. main does the same for any multi-hashtag paste after a comment, so either fix that in MarkStateExtension or move this case into a separate test.fails with a tracking issue. With it removed, the rest of the test passes.
| } | ||
| // The caret disambiguates repeated text when the unchanged suffix still | ||
| // follows it. Capture it before the transform splits or replaces any text. | ||
| if (caret >= 0 && prevText.endsWith(text.slice(caret))) { |
There was a problem hiding this comment.
Optional: this checks only the text after the caret. An edit made by code earlier in the node, with the caret kept after the word, is read as running up to the caret, so the entity made from the existing word drops its NodeState (spliceText(4, 0, '#') then select(8, 8) on see foo bar with c1: main keeps c1 on #foo, this drops it). Taking the branch only when prefix >= caret - (text.length - prevText.length), and using that value as the start, avoids it and keeps the new tests passing.
There was a problem hiding this comment.
Thanks for tracking down #9725. Reading the edit from the caret is a clean way to resolve the repeated-character case, and the new tests fail on main and pass on this branch.
Needs a fix before merge
packages/lexical-mark/src/__tests__/unit/MarkStateExtension.test.tslines 1101, 1123 and 1156 call$paragraph().getFirstChildOrThrow<TextNode>().ElementNode.getFirstChildOrThrow()is not generic, sopnpm run tscandpnpm run tsc-testfail with 8 errors at this head. Both are part ofpnpm run ci-check, which the core test workflow runs. A cast fixes it, as the same file already does elsewhere:$paragraph().getFirstChildOrThrow() as TextNode.
Optional suggestions (non-blocking)
packages/lexical-mark/src/MarkStateExtension.ts,fromComparedText(around line 2600): consider also requiring!(absorbedNew && normalized.has(source)), the check$isNewTextalready uses. Without it, text inserted with the comment's own id that merges into the commented node, and is then split by the hashtag transform, loses that id on the piece after the hashtag. For example, insertingx #y zthat carriesc1aftersee(c1) ends with" z", wheremainkeeps" z"[c1]. With the guard, the@lexical/markunit tests still pass. The path is narrow, but the changeset says explicitly copied marks keep their ids..changeset/repeated-text-entity-state.md: when the caret is not in the node, the independent suffix now gives a new entity whose tail repeats the node's text that node's NodeState. For example, settingtagtotag #tagby code gives#tagthe state. That matches the rule proposed in #9725, but the summary line says new entities never take it. CONTRIBUTING.md (Changesets) asks a fix that changes editing behavior to say what to verify, so a Verify sentence for the no-caret case would help.- The latest commit also changes
lexical(split-provenance recording and its Flow declarations) and@lexical/mark. It also fixes MarkStateExtension cases that fail onmainwithout any #9725 input. AGENTS.md (Commits and Pull Requests) asks the title to list every touched package directory ([lexical][lexical-mark][lexical-text][lexical-website]) and the description to say what changed, so please describe those changes there, or move them to their own PR. The Test plan should also show output for the current head: it reports results at74bd30b9and saystscpasses.
| ), | ||
| ); | ||
| update(editor, () => { | ||
| const node = $paragraph().getFirstChildOrThrow<TextNode>(); |
There was a problem hiding this comment.
ElementNode.getFirstChildOrThrow() is not generic, so this line (and 1123, 1156) fails pnpm run tsc / pnpm run tsc-test with TS2558 and follow-on property errors, which the core CI job runs through pnpm run ci-check. A cast fixes it, as the file does elsewhere:
const node = $paragraph().getFirstChildOrThrow() as TextNode;| source === undefined ? null : $getNodeByKey(source); | ||
| const fromComparedText = | ||
| source !== undefined && | ||
| compared.has(source) && |
There was a problem hiding this comment.
Optional: consider also requiring !(absorbedNew && normalized.has(source)) here, as $isNewText does. Without it, text inserted with the comment's own id that merges into the commented node, and is then split by the hashtag transform, loses that id on the piece after the hashtag. For example, inserting x #y z that carries c1 after see (c1) gives " z", where main keeps " z"[c1]. With the guard, the @lexical/mark unit tests still pass.
|
|
||
| [fix] Use the caret to resolve repeated-character entity insertions without copying NodeState onto newly inserted entities | ||
|
|
||
| Verify: Insert a repeated character before existing text matched by a lookbehind-based entity matcher and check that the existing character keeps its NodeState. Type the character at the end instead, or paste a hashtag whose tail repeats the marked text at a comment's edge, and check that the new entity does not inherit that state. Insert a hashtag prefix before existing text by code, with the caret left farther along the node, and check that the existing text keeps its NodeState. |
There was a problem hiding this comment.
Nit: when the caret is not in the node, a new entity whose tail repeats the node's text now does take its NodeState (setting tag to tag #tag by code gives #tag the state). That matches the rule proposed in #9725, but the summary line says new entities never take it. Consider a Verify sentence for that case (CONTRIBUTING.md, Changesets).
There was a problem hiding this comment.
This looks good. Thanks for tracking down #9725. Reading the caret before the transform splits the node, and trusting it only when it describes a pure insertion, fixes the lookbehind case without changing ordinary typing. The #9725, multi-hashtag paste and composition cases in registerLexicalTextEntityRepeatedText.test.ts, registerLexicalTextEntityLookbehind.test.ts and MarkStateExtension.test.ts fail without the change and pass with it. The suggestions below are optional and non-blocking.
-
The no-caret fallback is wider than the docs say (
packages/lexical-text/src/registerLexicalTextEntity.ts, lines 288-298). When there is no collapsed caret in the edited node, the suffix is now measured independently of the prefix, so one shared trailing character is enough for an appended entity to keep the node's NodeState. For example, changingsee(with NodeState) tosee #feeby code with the selection cleared gives the new#feethat NodeState, and withMarkStateExtensionthe hashtag joins the comment. This is the rule proposed in #9725, butpackages/lexical-website/docs/concepts/transforms.md(lines 196-199) and.changeset/repeated-text-entity-state.mdonly showtag→tag #tag. Consider saying that a single matching trailing character is enough, and adding asee→see #feetest so the trade-off is pinned. -
Describe the
lexicaland@lexical/markpart of the change. The diff also addsLexicalEditor._recordTextSplitsandEditorState._splitTextOrigins, and changesMarkStateExtensionso the text between pasted or composed hashtags stays out of a comment (.changeset/mark-appended-split-provenance.md). The pull request template and AGENTS.md ("Commits and Pull Requests") ask for two things:- a title that lists every package directory the diff touches, here
[lexical][lexical-mark][lexical-text][lexical-website]; - a Description and a Test plan, with Before/After output, that cover the whole diff. The current ones describe only the
@lexical/textfix.
Moving the core/mark fix into its own pull request would also work.
- a title that lists every package directory the diff touches, here
| // preserves the state of characters that may have existed before the edit. | ||
| while ( | ||
| suffix < max - prefix && | ||
| suffix < max && |
There was a problem hiding this comment.
Optional: without the max - prefix bound, an append now counts every trailing character it shares with the old text. So when there is no caret in the node, one matching character is enough for a new entity to keep the node's NodeState: see → see #fee by code, with the selection cleared, gives #fee the NodeState of see. This is the rule from #9725, but it's worth stating in transforms.md (and pinning with a test), since the changeset only shows tag → tag #tag.
Description
A repeated-character insertion can make the committed and current text match along more than one boundary. With
$AA→$AAA, the prefix-first comparison considers the lastAnewly inserted even when the user inserted before the two existing characters. A lookbehind-based entity matcher can then turn that old character into an entity without its NodeState.Capture the collapsed caret before the transform splits the text. When the text after that caret matches the previous suffix, use the caret to locate the insertion. Otherwise, compare the common prefix and suffix independently to preserve NodeState where the insertion boundary is ambiguous.
The regression cases cover insertion before the existing characters, insertion at the end, and selections that cannot identify the insertion. The paste cases check that a new hashtag with a repeated tail stays outside the preceding comment. The matcher uses captured context for compatibility with older browsers.
Related: #9727 documents this limitation; this PR addresses the production behavior without overlapping its documentation or Yjs changes.
Closes #9725
Changeset
Adds a patch changeset for
@lexical/textwith a verification scenario.Test plan
At
74bd30b9e34f2a988244c45c8a5f11cdcadc34c6, the isolated insertion-range check passes all 10 cases. The earlier suffix-only version fails 3 of those cases. The Lexical Tests workflow for the updated PR is awaiting maintainer approval. Workflow runBefore the initial fix
With the new regression and the original production condition:
After the initial fix
pnpm run tsc,pnpm run tsc-test, targeted ESLint, formatting, andpnpm run check-changesetspass.The full
pnpm run test-unitwas terminated with exit 137 in this environment, both at default concurrency and with--maxWorkers=2; it did not produce a completed suite result. Browser/E2E and Flow checks have not been run locally. No test, workflow, dependency, or compiler settings were changed to work around this.