Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Breaking Change][lexical][lexical-selection][lexical-list] Bug Fix: Fix infinite loop when splitting invalid ListItemNode #7037

Merged
merged 3 commits into from
Jan 10, 2025

Conversation

etrepum
Copy link
Collaborator

@etrepum etrepum commented Jan 10, 2025

Breaking Changes

  • insertList and removeList are now deprecated, use $insertList and $removeList instead.
  • INSERT_CHECK_LIST_COMMAND, INSERT_ORDERED_LIST_COMMAND, INSERT_UNORDERED_LIST_COMMAND and REMOVE_LIST_COMMAND now update synchronously when dispatched from inside an update, rather than deferring a nested update (this is the expected behavior for commands)
  • editor.focus() now happens synchronously when called from inside of an update, as if it was implemented with a command dispatch. Previously it would defer which is confusing behavior because the expected side-effect is to change the selection which you really do want to happen synchronously.

The breaking changes are the result of trying to unit test this functionality and finding out that editor.focus and list commands are asynchronous soup for no reason. These breaking changes are unlikely to affect any correctly written code, unless it is going very far out of the way to test or otherwise compensate for the previously maddening behavior.

Description

There's a while loop in $removeTextAndSplitBlock that can reach a fixed point and never terminate if it reaches the root (which is not a block). This ensures that if it gets to a fixed point, the loop terminates.

Additionally this fixes some incorrect code in $setBlocksType which mutated a potentially cached selection.getNodes() and did not always update the selection when it should (this was part of the preconditions for #7036) so you'd end up with a selection that is not normalized correctly that allowed you to insert TextNode adjacent to ListNode where it shouldn't be.

As a small related cleanup, the INTERNAL_$isBlock function is now exported from the lexical package so that it can be used in the implementation of @lexical/selection, previously this function was copied which added unnecessary bundle size and maintenance burden to maintain two copies of this function. It's marked @internal so the doc tools shouldn't pick it up.

Closes #7036

Test plan

Before

Creating a ListItemNode that has a ListItem first child and a TextNode second child causes an infinite loop when the return key is pressed, see #7036

After

A ParagraphNode is created and it is selected. A unit test covers this addition.

Copy link

vercel bot commented Jan 10, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
lexical ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 10, 2025 9:13pm
lexical-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 10, 2025 9:13pm

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jan 10, 2025
Copy link

github-actions bot commented Jan 10, 2025

size-limit report 📦

Path Size
lexical - cjs 31.41 KB (0%)
lexical - esm 31.2 KB (0%)
@lexical/rich-text - cjs 40.48 KB (0%)
@lexical/rich-text - esm 33.16 KB (0%)
@lexical/plain-text - cjs 39.05 KB (0%)
@lexical/plain-text - esm 30.33 KB (0%)
@lexical/react - cjs 42.32 KB (0%)
@lexical/react - esm 34.44 KB (0%)

@etrepum etrepum added the extended-tests Run extended e2e tests on a PR label Jan 10, 2025
@etrepum etrepum changed the title [lexical][lexical-selection] Bug Fix: Fix infinite loop when splitting invalid ListItemNode [Breaking Change][lexical][lexical-selection][lexical-list] Bug Fix: Fix infinite loop when splitting invalid ListItemNode Jan 10, 2025
@etrepum etrepum marked this pull request as ready for review January 10, 2025 20:56
@etrepum
Copy link
Collaborator Author

etrepum commented Jan 10, 2025

The test failure is a spurious flaky test failure, not worth running it any more times to try and get a clean run

CI

[1]   1 failed
[1]     [chromium] › packages/lexical-playground/__tests__/e2e/CopyAndPaste/lexical/ListsCopyAndPaste.spec.mjs:115:3 › Lists CopyAndPaste › Copy and paste of partial list items into the list 

Local

❯ npm run test-e2e-collab-chromium packages/lexical-playground/__tests__/e2e/CopyAndPaste/lexical/ListsCopyAndPaste.spec.mjs:115:3

> @lexical/[email protected] test-e2e-collab-chromium
> cross-env E2E_BROWSER=chromium E2E_EDITOR_MODE=rich-text-with-collab playwright test --project="chromium" packages/lexical-playground/__tests__/e2e/CopyAndPaste/lexical/ListsCopyAndPaste.spec.mjs:115:3


Running 1 test using 1 worker

  ✓  1 …dPaste.spec.mjs:115:3 › Lists CopyAndPaste › Copy and paste of partial list items into the list (2.5s)

  1 passed (3.5s)

@etrepum etrepum added this pull request to the merge queue Jan 10, 2025
Merged via the queue into facebook:main with commit bd874a3 Jan 10, 2025
39 of 40 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. extended-tests Run extended e2e tests on a PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: Weird list item state causes infinite loop on insertParagraph
3 participants