Repository navigation
[lexical-y] Bug Fix: Keep list numbering right after several follow-ups in a row that are read again - #9634
[lexical-y] Bug Fix: Keep list numbering right after several follow-ups in a row that are read again#9634zurfyx wants to merge 2 commits into
Conversation
…ps in a row that are read again ## Description A follow-up update (BlockIdExtension's naming, the update that runs a newly registered transform) may only set attributes or delete. Anything else it changes is not written and is read again, and the re-read refreshes the derived fields of the groups it rebuilds (list numbering), as every read does. To stop a group's own transform that changes the same content on every refresh from looping (refresh, re-read, refresh, ...), the binding counts re-reads in a row and lets the fourth one run no refresh. It counted every re-read until a user's edit. So four separate follow-ups in a row, each read again, made the last re-read skip its refresh. The author then kept a stale derived value: a number list's item holding a nested list showed `value` 1, where its own fresh read, its peers and Lexical's numbering showed 2. It stayed wrong after later syncs and edits. Now only re-reads of the binding's own refresh are counted. A re-read of any other update's change starts the count again, as a user's edit already did, so its refresh always runs. The loop the count guards against is still cut off after the same number of refreshes. Closes #9632 ## Test plan `packages/lexical-y/src/__tests__/unit/regressions/RefusedFollowUpRefresh.test.ts` (new): - 0, 1, 2, 3, 4 or 6 follow-ups in a row that each insert a paragraph (read again), then one that empties the list item holding a nested list. Tagged as BlockIdExtension's naming and untagged, both client id orders. After each, the author shows what a fresh reader shows, list numbering included, and its peer matches. - A list whose own transform adds an item every time it runs. The refresh is still cut off after 4 runs with one warning, and the editor shows the document. ### Before ``` $ pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/regressions/RefusedFollowUpRefresh.test.ts × 3 before, tag block-id-repair, ids 1,2 × 3 before, tag block-id-repair, ids 2,1 × 4 before, tag block-id-repair, ids 1,2 × 4 before, tag block-id-repair, ids 2,1 × 6 before, tag block-id-repair, ids 1,2 × 6 before, tag block-id-repair, ids 2,1 AssertionError: empty the item holding the nested list: expected [ 1, 1 ] to deeply equal [ 1, 2 ] Tests 6 failed | 19 passed (25) ``` `FollowUpFuzz.test.ts` with seeds 1-36,000 (`FOLLOWUP_FUZZ_SEEDS`, run in ranges): 52 seeds fail on main, every one a derived list `value` on the author after a refused follow-up. ### After ``` $ pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/regressions/RefusedFollowUpRefresh.test.ts Tests 25 passed (25) $ pnpm exec vitest run --project unit packages/lexical-y Tests 14593 passed | 190 expected fail | 130 skipped (14913) ``` `FollowUpFuzz.test.ts` with seeds 1-36,000: 0 fail.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
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.
|
… of counting only refresh re-reads ## Description The previous commit counted only re-reads of the binding's own refresh toward `MAX_REREADS`; a re-read of any other update's change started the count again. An app's update listener that answers each refresh (or each collaboration-tagged commit) with a follow-up the binding does not write then looped without end: that reply is read again, the re-read refreshes, and the refresh sets the listener off again. Main stops it after four re-reads (review: 24 cells, with active and read-only clients, discrete and batched updates, one or both peers, both client id orders). Telling such replies from an app's own follow-ups by timing cannot work either: a listener can answer in a microtask. So the count is main's again: re-reads in a row until a user's edit. What changes is the last re-read. It ran no refresh at all, which left derived fields stale (#9632). It now runs the refresh in its own update, with no refresh commit after it. The editor shows the numbering a fresh reader shows, and nothing that answers a refresh (a listener keyed on the refresh, a group's own transform) runs again. A group whose transforms change more than its derived fields (a transform that rewrites the same content every time) is read again at once, in the same update, so the editor shows the document. ## Test plan New, from the review: - `regressions/RefusedFollowUpLoops.test.ts`: 16 cells, listener on one or both peers, triggered by the refresh or by collaboration-tagged commits, discrete and batched, both client id orders. - `regressions/RefusedFollowUpReadOnly.test.ts`: 8 cells on a read-only client, `gc` on and off, discrete and batched, both triggers. - `regressions/RefusedFollowUpFreshness.test.ts`: 32 cells of follow-ups in a row (0, 3, 6, 12), the numbering checked synchronously after each update, discrete and batched, with and without BlockIdExtension, lists starting at 1 and 7. `RefusedFollowUpRefresh.test.ts`: the transform that adds an item on every run now runs 5 times (the last re-read's refresh, read again in the same update), with one warning, and the editor equals a fresh reader. ### Before On the previous commit (be4f766): ``` $ pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/regressions/RefusedFollowUp × listener refusal cycle ... (16 of 16) × read-only refused listener ... (8 of 8) AssertionError: listener-generated refusal chain must terminate before fuse: expected 20 to be less than 20 AssertionError: refusal chain terminates before harness fuse: expected 20 to be less than 20 ``` On main (1853707), the 24 loop cells pass and 24 of the 32 freshness cells fail (the #9632 numbering). ### After ``` $ pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/regressions/RefusedFollowUp packages/lexical-y/src/__tests__/unit/DisposeMemory.test.ts packages/lexical-y/src/__tests__/unit/Headless.test.ts ``` Test Files 6 passed (6) Tests 114 passed (114) $ pnpm exec vitest run --project unit --project scripts-unit Tests 25699 passed | 190 expected fail | 135 skipped (26024) $ pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/matrix Tests 6083 passed | 29 expected fail | 128 skipped (6240) $ pnpm run ci-check Found 0 errors ``` `FollowUpFuzz.test.ts` with seeds 1-36,000: 0 fail (main: 52).
![meta-code-review[bot]](https://avatars.githubusercontent.com/in/5334748?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks, this looks good. The last re-read now refreshes list numbering inside its own update instead of dropping the refresh, which fixes #9632 without reopening the listener loop the earlier head ran into. The new numbering and freshness tests fail on main and pass on this branch, as do the 11 FollowUpFuzz seeds listed in #9632, and the loop and read-only tests keep the re-read cap honest. A few optional, non-blocking suggestions:
-
Changeset
Verify:paragraph (.changeset/lexical-y-reread-refresh.md). CONTRIBUTING.md ("Changesets") and.changeset/README.md("Behavior changes") ask a fix that changes observable behavior to carry aVerify:paragraph. Here the list itemvaluethe author's editor shows changes, so something like:Verify: list numbering (ListItemNode.getValue()) on the editor that made several BlockIdExtension naming updates in a row that the binding does not write, compared with a fresh read of the same document. -
MAX_REREADSJSDoc (packages/lexical-y/src/internal/Binding.ts:276-278). "every re-read leaves the derived fields a fresh reader shows" does not hold for a group whose own transform changes structure.$refreshInLastReadreads that group again and then clearsstate.refresh, so it keeps numbering computed on the structure that was just reverted. For example, a number list whose$transformprepends an item on every run ends at[2, 3, 4]on the author, against[1, 2, 3]on the peer and in a fresh reader.maingives the same result, so this is not a regression; limiting the sentence to groups whose transforms only set derived fields would keep it accurate. -
Test plan Before block and the run-count assertion (
packages/lexical-y/src/__tests__/unit/regressions/RefusedFollowUpRefresh.test.ts:168). At this head the file fails 7 of 25 onmain, not 6: theGrowingListNodecell also stops atexpect(runs).toBe(5)(4 onmain). AGENTS.md asks the Test plan to carry the real Before output, so it is worth refreshing that block. The run count also ties the test to how often the refresh runs the transform; the JSON comparisons after it already show the editor matching the document, so you could drop or loosen the count.
|
|
||
| [fix] Keep list numbering right on an editor after several follow-up updates in a row that the binding reads again | ||
|
|
||
| A follow-up update (BlockIdExtension's naming, the update that runs a newly registered transform) may only set attributes or delete; anything else it changes is read again, and the read refreshes the derived fields of the lists it rebuilds. After the fourth such re-read in a row without a user's edit, the binding stopped refreshing, to cut off a loop: the editor then showed a list item's value differently from a fresh reader of its document. The last re-read now refreshes in its own update, with no refresh after it, so the loop is still cut off and the numbering is right. |
There was a problem hiding this comment.
Optional: CONTRIBUTING.md ("Changesets") and .changeset/README.md ("Behavior changes") ask a fix that changes observable behavior to add a Verify: paragraph. Here the list item value the author's editor shows changes, so something like:
Verify: list numbering (ListItemNode.getValue()) on the editor that made several BlockIdExtension naming updates in a row that the binding does not write, compared with a fresh read of the same document.
| * again at most this many times in a row (until a user's edit), and every | ||
| * re-read leaves the derived fields a fresh reader shows. Why: a transform |
There was a problem hiding this comment.
Optional nit: "every re-read leaves the derived fields a fresh reader shows" does not hold for a group whose own transform changes structure. $refreshInLastRead reads that group again and then clears state.refresh, so it keeps numbering computed on the reverted structure. A number list whose $transform prepends an item on every run ends at [2, 3, 4] here against [1, 2, 3] on the peer and in a fresh reader. main behaves the same, so this is only about the comment: limiting it to groups whose transforms only set derived fields would keep it accurate.
| // The first refresh, the one after each of the re-reads before the last | ||
| // (MAX_REREADS = 3), and the last re-read's own, whose change is read | ||
| // again in the same update. | ||
| expect(runs).toBe(5); |
There was a problem hiding this comment.
Optional: this count is how often the transform runs, which ties the test to the refresh's internals (it is 4 on main, so this cell also fails there; the Test plan's Before block lists 6 failures, not 7). The JSON comparisons below already show the editor matching the document, so you could drop or loosen the count.
Parked (10/30)
Draft until a maintainer picks one of the options below. Three requirements turned out to conflict:
valueis 2, it clears that item with a follow-up the binding does not write.On main, (c) stops only because the fourth re-read in a row leaves the value stale. Main gets (b) by violating (a). As long as each round leaves the value fresh, an app that answers that value with a refused follow-up keeps being answered, whatever the binding counts. The only ways to end that chain are to stop re-reading (the editor then shows content that is not stored) or to show a stale value again (#9632). This head (2c4a89b) satisfies (a) and (b) for replies that do not read the value. It does not stop (c): nested-update, microtask and
setTimeoutreplies each run until the test's fuse, while main and the nightly stop after 4 or 5 rounds.Options:
test.failsnaming Bug: [lexical-y] After refused naming follow-ups, the author shows a number list's wrapper item with value 1 where its own document reads 2 #9632.Description
A follow-up update (BlockIdExtension's naming, the update that runs a newly registered transform) may only set attributes or delete. Anything else it changes is not written and is read again, and the re-read refreshes the derived fields of the groups it rebuilds (list numbering), as every read does.
To stop a loop (a group's own transform that rewrites the same content on every refresh, or an app's listener that answers every refresh with such a follow-up), the binding counts re-reads in a row until a user's edit, and the fourth one ran no refresh at all. So four follow-ups in a row, each read again, left the author with a stale derived value: a number list's item holding a nested list showed
value1, where its own fresh read, its peers and Lexical's numbering showed 2. It stayed wrong after later syncs and edits.The count is unchanged. The last re-read now runs the refresh in its own update, with no refresh commit after it:
(An earlier head of this pull request counted only re-reads of the binding's own refresh. Review showed that an app's listener answering each refresh with a refused follow-up then looped without end, so that was replaced.)
No decision row: this keeps the existing rules that every read refreshes derived fields and that the loop is cut off after the same number of re-reads.
Closes #9632
Changeset
lexical-y-reread-refresh.md(@lexical/y, fix): Keep list numbering right on an editor after several follow-up updates in a row that the binding reads again.Test plan
New regression tests in
packages/lexical-y/src/__tests__/unit/regressions/:RefusedFollowUpRefresh.test.ts(25 cells):RefusedFollowUpFreshness.test.ts(32 cells): follow-ups in a row (0, 3, 6, 12), with the numbering checked synchronously after each update. Discrete and batched, with and without BlockIdExtension, lists starting at 1 and 7.RefusedFollowUpLoops.test.ts(16 cells): an app listener that answers the refresh, or collaboration-tagged commits, with a refused follow-up. On one or both peers, discrete and batched, both client id orders. The loop stops.RefusedFollowUpReadOnly.test.ts(8 cells): the same on a read-only client,gcon and off. Nothing is written and the editor shows the document.Before
On main (1853707):
RefusedFollowUpFreshness.test.ts: 24 of 32 cells fail on main.expected 20 to be less than 20, the test's fuse).FollowUpFuzz.test.tswith seeds 1-36,000 (FOLLOWUP_FUZZ_SEEDS, run in ranges): 52 seeds fail on main, every one a derived listvalueon the author after a refused follow-up. Seeds 1-6,000 fail 11 (1793, 1856, 1982, 2750, 3127, 3316, 4471, 4527, 5014, 5023, 5217). The same seeds fail on the 0.53.1 nightly.After
On 2c4a89b:
FollowUpFuzz.test.tswith seeds 1-36,000: 0 fail.