Repository navigation
Conversation
…wrapping a nested list ## Description Closes #9661. An item that only wraps a nested list does not count in its list's numbering (`updateChildrenListItemValue`). When the nested list's last item is removed, the nested list goes and the wrapper item stays, empty, and now counts; when a list is appended to an empty item, the item stops counting. In both cases only the item is written, not its list, so the list's transform, which renumbers, did not run: the items after the item kept stale values (`1, 1` instead of `1, 2`), and a split there (paragraphs inserted at the empty item) started the next list from the stale value. `ListItemNode`'s transform now compares the item with its previous version: where it has started or stopped wrapping a nested list since the last update, its list renumbers its items. Nothing else changes: a value an app sets on an item directly is left as before (until the list is written), and an item that keeps wrapping its nested list leaves the list unwritten. ## Test plan `RenumberOnNestedListRemoval.test.ts`: - the issue's repro: removing the only item of a nested list renumbers the items after it, and a split there starts the next list at 2; - several items after it, in a list that starts at 5; - the last item of a nested list that has two (the first removal changes nothing, the second renumbers); - an empty item that comes to hold only a nested list stops counting; - bullet and check lists; - controls: the same split with up-to-date values; a removal that leaves the item wrapping its nested list leaves the outer list unwritten; a value an app sets on an item is not renumbered by this. ### Before main (c0c7e42): ``` Tests 6 failed | 2 passed (8) × removing the only item of a nested list renumbers the items after it expected [ { start: 1, values: [ 1, 1 ] } ] to deeply equal [ { start: 1, values: [ 1, 2 ] } ] ``` ### After ``` $ vitest --run --project unit packages/lexical-list/src/__tests__/unit/RenumberOnNestedListRemoval.test.ts Tests 9 passed (9) ``` All unit tests: 24,726 passed, 421 expected fail, 0 failed. `pnpm run ci-check` passes.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
zurfyx
requested review from
acywatson,
etrepum,
ivailop7 and
potatowagon
as code owners
October 31, 2026 05:34
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: |
…ed item, keeping values an app set ## Description Review of the first commit found two flaws in the renumbering: - It compared the item with the committed state and renumbered the whole list, so it also overwrote a value an app set on the item in the same update, and an app transform that keeps a value (`setValue(7)`) fought it until Lexical threw and rolled the update back. - Each changed item renumbered the whole list: emptying N wrappers in one update cost N renumberings (512 wrappers: 523,776 value reads). Now an item that started or stopped wrapping a nested list renumbers only where the item after it is out of step, and the renumbering (`$renumberAfterWrapChange`) starts at the first changed item of the list: the items before it and that item keep their values, an item whose value changed in this update keeps it (the items after it count on from it), and one pass leaves the list consistent, so the other changed items find nothing to do. ## Test plan `RenumberOnNestedListRemoval.test.ts` adds: a value an app sets in the same update as the removal stays (7, and the next item 8); an app transform that keeps a value does not conflict (no error, 7 and 8); emptying 64, 128 and 256 wrappers in one update, first to last and last to first, reads values a linear number of times. The four fail on the previous head; all 13 pass here. All unit tests: 24,730 passed, 421 expected fail, 0 failed. `pnpm run ci-check` passes.
zurfyx
marked this pull request as draft
October 31, 2026 05:59
![meta-code-review[bot]](https://avatars.githubusercontent.com/in/5334748?s=60&v=4){kind=small}
There was a problem hiding this comment.
The change looks good: it fixes the stale numbering from #9661, and the new tests fail on main and pass with the fix. Everything below is optional and non-blocking.
- A second wrap change made by a transform in a later round (
packages/lexical-list/src/LexicalListItemNode.ts:923-925).$renumberAfterWrapChangekeeps any item whose value differs from the committed state, treating it as app-set. It cannot tell those values from ones its own earlier pass wrote in the same update. Repro: a list[w0⟨x⟩, a, w1⟨y⟩, b]with aListItemNodetransform that removes empty items inside nested lists. One update removesxand emptiesy. The result is[1, 2, 3, 3], while the list's own transform would give[1, 2, 3, 4]. The same two removals made directly in one update come out right. One way to fix it: remember the keys this function writes during the pending update (for example aWeakMapkeyed by the pendingEditorState) and don't treat those as app-set. A test for it would sit well beside the 64/128/256-wrapper cases. $isListItemNodeloses its doc comment (LexicalListItemNode.ts:879-884). The helpers were inserted between its JSDoc and its declaration, so the emitted.d.tsand the API docs no longer describe the exported guard, and$wrapChangedends up with two doc blocks. Moving both helpers above the/** Checks to see if the node is a ListItemNode.block fixes it.- Test environment.
RenumberOnNestedListRemoval.test.tsexercises editor state only. AGENTS.md (Testing Strategy) asks such tests to add// @vitest-environment nodeafter the copyright header, as several sibling list tests do. - Test plan. The
### Beforeoutput (6 failed | 2 passed (8)) comes from an earlier version of the test file; at this head it fails 10 of 13 onmain. AGENTS.md (Commits and Pull Requests) asks for the real Before and After output of the command that was run. - Changeset wording. The summary mentions only the removal case. The fix also stops an empty item counting once it comes to hold only a nested list, which could be one more clause.
| * @param node - The node to be checked. | ||
| * @returns true if the node is a ListItemNode, false otherwise. | ||
| */ | ||
| /** Whether `item` started or stopped wrapping a nested list since `prevMap`. */ |
There was a problem hiding this comment.
Optional: these helpers sit between $isListItemNode's JSDoc (lines 879-883) and its declaration, so the exported guard's emitted .d.ts loses its doc comment and $wrapChanged gets two doc blocks. Moving both helpers above the /** Checks to see if the node is a ListItemNode. block keeps the docs where they were.
Comment on lines
+923
to
+925
| const prev = prevMap.get(child.getKey()); | ||
| if ($isListItemNode(prev) && prev.__value !== current) { | ||
| value = current; |
There was a problem hiding this comment.
Optional: this also keeps values that an earlier $renumberAfterWrapChange pass wrote in the same update, so a wrap change that a transform makes in a later round stops being corrected after the changed item. Repro: [w0⟨x⟩, a, w1⟨y⟩, b] with a ListItemNode transform that removes empty items inside nested lists; one update removes x and empties y. The result is [1, 2, 3, 3] instead of [1, 2, 3, 4]. Tracking the keys this function writes during the pending update, and not treating those as app-set, would close it.
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Parked (10/30)
Parked after the second review round. Two findings on bf7d979:
[1, 2, 2]where[1, 2, 3]is right (f527252 gives[1, 2, 3]).(N + 2)(N - 1)whole-list scans (main: none).Both come from the same gap: inside one update, the item transform cannot tell a value an app set from a value this renumbering wrote earlier in the update, as both differ from the committed value. Telling them apart needs a small state addition: a marker on the item (written by the renumbering), or a list-level "renumber pending" flag that registerList keeps for the update. That is an API or state decision for the maintainer. #9661 stays open; the tests here (the issue's repro and controls) are the starting point.
Description
Closes #9661.
An item that only wraps a nested list does not count in its list's
numbering (
updateChildrenListItemValue). When the nested list's last itemis removed, the nested list goes and the wrapper item stays, empty, and now
counts; when a list is appended to an empty item, the item stops counting.
In both cases only the item is written, not its list, so the list's
transform, which renumbers, did not run: the items after the item kept
stale values (
1, 1instead of1, 2), and a split there (paragraphsinserted at the empty item) started the next list from the stale value.
ListItemNode's transform now compares the item with its previous version:where it has started or stopped wrapping a nested list since the last
update and the item after it is out of step, the list's items are
renumbered once (
$renumberAfterWrapChange): from the first changed itemof the list, counting on from its value. The items before it and that item
keep their values, and so does an item whose value an app changed in this
update (the items after it count on from it), so an app transform that keeps
a value never conflicts with it. One pass leaves the list consistent, so the
cost is linear in the list however many items changed. An item that keeps
wrapping its nested list leaves the list unwritten.
Changeset
.changeset/list-renumber-nested-removal.md(@lexical/listpatch,[fix]with aVerify:paragraph).Test plan
RenumberOnNestedListRemoval.test.ts:items after it, and a split there starts the next list at 2;
nothing, the second renumbers);
the item wrapping its nested list leaves the outer list unwritten; a
value an app sets on an item is not renumbered by this;
transform that keeps a value does not conflict; emptying 64, 128 and 256
wrappers in one update (first to last and last to first) reads values a
linear number of times.
Before
main (c0c7e42):
After
All unit tests: 24,730 passed, 421 expected fail, 0 failed.
pnpm run ci-checkpasses.