Repository navigation
[lexical-y] Bug Fix: quiesce() called again returns the first call's draft, so a retried handoff keeps unwritten text - #9792
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. quiesce() now keeps a copy of its first result, so retrying after a refused batched update still hands off the typed text with differs: true. The new QuiesceRepeat test fails against the previous binding and passes with this change, and the JSDoc, the y.md handoff bullets and the Flow comment match the code.
Optional, non-blocking suggestions:
.changeset/lexical-y-quiesce-repeat-draft.md:.changeset/README.md("Behavior changes") and AGENTS.md ask for aVerify:paragraph on a fix that changes observable behavior. For example:Verify: on a binding that is already read only, batch an edit, call binding.quiesce() twice; the second draft still holds the edit, with differs: true, and its state equals the first call's.packages/lexical-y/src/__tests__/unit/regressions/QuiesceRepeat.test.ts:155: the copy-isolation check only mutates a later result. MutatingfirstStatebefore the second call would also cover the copy taken atBinding.ts:2282-2288.
One intentional change to keep in mind, already documented in the JSDoc: repeat calls no longer re-encode the document, so nothing written after the first call shows up in later results. That fits the server guide's handoff, which retries the same bytes until the server acknowledges them.
| '@lexical/y': patch | ||
| --- | ||
|
|
||
| [fix] `YBinding.quiesce()` called again (before or after `dispose()`) returns a copy of the first call's result: the same state and the same draft, `differs` included. When the first call refused the editor's batched update (the binding was already read only, or the record stopped admitting writes during the call), that update was its draft with `differs: true`, but the refusal read the document back into the editor and a second call rebuilt the draft from it, without the unwritten text and with `differs: false`: a retried handoff lost what the user typed. Later calls now recompute nothing; what was written to the document after the first call is not in their state. |
There was a problem hiding this comment.
Optional: .changeset/README.md ("Behavior changes") and AGENTS.md ask for a Verify: paragraph on a fix that changes observable behavior. For example:
Verify: on a binding that is already read only, batch an edit, call binding.quiesce() twice; the second draft still holds the edit, with differs: true, and its state equals the first call's.
| const second = t.binding.quiesce(); | ||
| expectSame(second, firstState); | ||
| // A copy: changing what one call returned changes no later result. | ||
| second.state.fill(0); |
There was a problem hiding this comment.
Optional: this checks that mutating a later result leaves the cache intact. Mutating firstState before the second call would also cover the copy taken when the first result is cached (Binding.ts:2282-2288).
0abf21f to
89cccda
Compare
72c59cd to
be7dbe7
Compare
There was a problem hiding this comment.
The change looks good. Keeping the first quiesce() result and handing out copies fixes the retried handoff, and the new QuiesceRepeat / QuiesceEdges cells fail against the previous Binding.ts and pass with this change.
Before merging: this branch was cut before zurfyx/y-layout3-identity was force-pushed, so the diff here still carries older revisions of the five composition-hold commits now on the base. It conflicts with the base in packages/lexical-y/src/internal/Write.ts, three ComposeHold* tests and .changeset/lexical-y-held-composition.md. Please restack the quiesce commit onto the current base, where it applies cleanly, rather than resolving those conflicts toward this branch. The older Write.ts ($refusesRelocation / $removedWhole, line 4964) lets through the merge-into-unchanged-text cases that the base's ComposeHoldRefusesMergeIntoUnchangedText.test.ts refuses.
Optional, non-blocking suggestions:
copyDraftand class instances (packages/lexical-y/src/internal/Binding.ts:475).structuredClonecopies a class instance as a plain object of its own enumerable fields and drops its prototype and private fields. A NodeState value such as an object with a#centsfield and atoJSON()used to reach the first draft as itself ({"cents":250}once serialized); it now reaches it as{}. Cloning plain objects, arrays and the structured-clone built-ins (Date,Map,Set, typed arrays) one value at a time, and keeping any other prototype by reference, would keep both the value and the isolation.- Fallback sharing (
Binding.ts:499-502). When any value is refused (a function anywhere in the state),copyTreekeeps every non-plain object by reference. ADateelsewhere in the draft is then the editor's own NodeStateDate, and mutating it changes later results, although the JSDoc,y.mdand the changeset promise that every result is its own deep copy. The per-value copy above covers this too. - Changeset (
.changeset/lexical-y-quiesce-repeat-draft.md). CONTRIBUTING.md asks for a[category] one-line summaryfirst line, and the current first line is a 900-character paragraph; the PR title would work as the summary. Also,onStatus('readonly')is now delivered beforequiesce()returns, where it used to stay queued. Apps can observe that, so it deserves a line inVerify:(CONTRIBUTING.md: a fix that changes observable behavior says what to test). - Two more cells. The changeset's
Verify:promises that a later document update stays out of a repeat call's state, and the JSDoc promises that anonStatuslistener's retry gets the first result. A cell for each would pin those down.
| function copyDraft<T>(value: T): T { | ||
| if (typeof structuredClone === 'function') { | ||
| try { | ||
| return structuredClone(value); |
There was a problem hiding this comment.
Optional: structuredClone flattens a class instance into a plain object of its own enumerable fields, dropping the prototype and private fields. A NodeState value with a #cents field and toJSON() used to reach the first draft as itself ({"cents":250} serialized) and now arrives as {}. Cloning plain objects, arrays and Date/Map/Set/typed arrays per value, and keeping other prototypes by reference, would keep both the value and the isolation. It would also stop the fallback below from sharing every Date with the editor.
| '@lexical/y': patch | ||
| --- | ||
|
|
||
| [fix] `YBinding.quiesce()` called again (before or after `dispose()`) returns a copy of the first call's result: the same state and the same draft, `differs` included. When the first call refused the editor's batched update (the binding was already read only, or the record stopped admitting writes during the call), that update was its draft with `differs: true`, but the refusal read the document back into the editor and a second call rebuilt the draft from it, without the unwritten text and with `differs: false`: a retried handoff lost what the user typed. Later calls now recompute nothing; what was written to the document after the first call is not in their state. Every result, the first included, is its own deep copy, NodeState values such as a `Date` or a `BigInt` included, and a warning handler that calls `quiesce()` while the first call's warnings are delivered gets the first call's result. |
There was a problem hiding this comment.
Optional: CONTRIBUTING.md asks for a [category] one-line summary first line; this one is a ~900-character paragraph, so the PR title could be the summary and the rest a body paragraph. The Verify: paragraph could also mention that onStatus('readonly') is now delivered before quiesce() returns (it used to stay queued).
| } | ||
| for (const key of this.endangeredKeys(hidden)) { | ||
| if (this.$relocated(key)) { | ||
| return this.$refusesRelocation(hidden); |
There was a problem hiding this comment.
This is the older revision of the base's composition-hold fix. The current base refuses here unconditionally, and its ComposeHoldRefusesMergeIntoUnchangedText.test.ts fails against this version. Restacking the quiesce commit onto the current base drops this file from the diff; please don't resolve the conflict toward this branch.
be7dbe7 to
c61768f
Compare
…draft, so a retried handoff keeps unwritten text
## Description
The automated code review (CodeBunny) found a defect. `quiesce()`'s JSDoc,
y.md's "Handing a replica off" bullets and the `YQuiescedState` Flow comment
promised that a repeat call returns the same draft. That was false when the
first call refused the editor's batched update, which happens when the
binding is already read only or the record stops admitting writes during
the call:
1. The first call returned that update as the draft, with
`differs: true`.
2. The refusal read the document back into the editor.
3. A second call rebuilt the draft from the editor, without the unwritten
text and with `differs: false`.
A retried handoff lost what the user typed.
`quiesce()` now keeps its first result and returns copies of it from every
call, the first included, recomputing nothing after the first call. API
review rounds 9 to 11 are folded in:
- **One guard until the result is kept.** The first call runs inside one
`guard` (`quiesceFirst`) and keeps its result before the status changes.
So the refusal's warnings and the status notification go out only after
that, and an `onWarn` handler that retries gets the first call's draft.
The outside-an-update check and the kept fast path are unchanged.
- **The draft's domain is JSON.** That is what a quarantine record stores,
and `createYQuarantineRecord` refuses anything else.
- Before the drain, quiesce() serializes the held batched update and the
shown editor state with `JSON.stringify` (the preflight). A state JSON
cannot hold (a BigInt NodeState value, a cycle, a throwing getter)
throws a `TypeError` before anything has drained or changed.
- App code can make the state unserializable during the drain, for
example an update listener's own update. In that case the final
serialization throws a distinct `TypeError` (cause kept): `quiesce()
could not serialize the drained editor state. Local updates may already
have been written; no handoff result was cached. Store serializable
state and retry quiesce().` A private phase argument of `draftText`
picks the message.
- The final draft is serialized before the result is kept. The binding
keeps `{state bytes, draft text, differs}`.
- Every call returns a fresh byte copy of the state and a fresh
`JSON.parse` of the text. So a node serializer that reuses its output
objects cannot change the kept draft, a Date NodeState value is the
same ISO string in every result, and an own `__proto__` key survives.
- No structuredClone and no fallback copier.
- **Docs.** The JSDoc, y.md and the Flow comment say the draft is the
editor state's JSON (a Date is its ISO string, in every result). They
say a preflight failure throws a TypeError without draining or
quiescing, and that a state app code makes unserializable during the
drain can throw after local work was written, with nothing cached (fix
it, retry, and don't assume earlier edit callbacks didn't run). They also
say every result is its own copy. The `[fix]`
changeset's `Verify:` paragraph covers:
- refused batched text kept on retry, including from `onWarn`;
- mutation isolation;
- a retry after `dispose()`;
- a later document update staying out of the kept state;
- the Date and BigInt cases;
- the drain-time failure and its retry.
## Test plan
### Before
Two new files, gc off and on.
- `regressions/QuiesceRepeat.test.ts` covers three ways the first call
refuses the batched update: already read only, a record made newer in an
app transaction, or one made newer by a `beforeTransaction` listener.
Each cell retries normally, after mutating a result, and after
`dispose()`.
- `regressions/QuiesceEdges.test.ts` covers:
- the `onWarn` retry;
- a node serializer that reuses its output objects (the reviewer's
probe; the repeat equals the first);
- a Date NodeState (the same ISO string in first and repeat);
- an own `__proto__` key kept;
- a BigInt NodeState: a TypeError, with the status, editability and
document unchanged, then a JSON state hands off;
- preflight failures (held, shown, getter, cycle): the message says
nothing changed, and the document, pending update, composition,
status, editability, `quiesced` and the cache are untouched;
- an update listener that sets a BigInt during the drain (the reviewer's
probe): the drained-state message with its cause, the text stored, the
composition ended, `quiesced` false and no cache, then a retry with
serializable state hands off.
QuiesceEdges.test.ts with round 9's binding (bd508881f, its
structuredClone copies), gc off and on:
```
× a serializer that reuses its output objects: a later update does not change the kept draft
× a Date NodeState: its ISO string, the same in the first and a repeated draft
× an own __proto__ key in a NodeState value is kept, in every result
× a BigInt NodeState: a TypeError before anything changes (status, editability, the document), then a JSON state hands off
Tests 8 failed | 2 passed (10)
```
With round 10's binding (c61768f), the drain-time cell fails in both gc
modes (`Tests 2 failed | 18 passed (20)`): the error said `nothing was
changed`, `expected '@lexical/y: quiesce() hands off the e…' to be
'@lexical/y: quiesce() could not seria…'`.
On the parent (no fix), QuiesceRepeat.test.ts fails 6 of 6 (`expected
false to be true`, the draft's `differs`).
### After
`vitest --run --project unit packages/lexical-y/src/__tests__/unit/regressions/QuiesceEdges.test.ts packages/lexical-y/src/__tests__/unit/regressions/QuiesceRepeat.test.ts packages/lexical-y/src/__tests__/unit/regressions/MigrateQuiesceDraft.test.ts packages/lexical-y/src/__tests__/unit/MigrateCore.test.ts packages/lexical-y/src/__tests__/unit/regressions/BindingLifecycleEdges.test.ts`,
gc off and on:
```
Tests 66 passed (66)
```
`tsc`: 0 errors. `node scripts/check-flow-types.mjs`: `Found 0 errors`.
`node scripts/changesets/check.mjs`: 110 valid.
The whole lexical-y and lexical-yjs unit projects on this commit: gc off
22,311 passed, 315 expected fail; gc on 22,311 passed, 315 expected fail.
`pnpm run ci-check`: exit 0. `pnpm run build-release`: exit 0
(LexicalY.prod.js +804 / +327 bytes min/gzip against 89cccda).
c61768f to
6c03c00
Compare
There was a problem hiding this comment.
The change looks good. Keeping the first quiesce() result and handing out copies of it fixes the retried handoff: the new QuiesceRepeat and QuiesceEdges cells fail against the previous Binding.ts and pass with this change, and the JSDoc, y.md, the Flow comment and the changeset all describe the new contract.
Optional, non-blocking suggestions:
- A cell for the frozen state (
packages/lexical-y/src/__tests__/unit/regressions/QuiesceRepeat.test.ts). The changeset'sVerify:paragraph says a later update to the document stays out of a later call's state, but no test writes to the document between two calls. Applying another replica's update between two calls and asserting equal state bytes would pin that down, and it fails against the previous implementation. draftText'stry(packages/lexical-y/src/internal/Binding.ts:485).read()runs inside thetry, so an error thrown by a node's ownexportJSONis reported as a state that "holds a value JSON cannot (a BigInt NodeState value, or a cycle)". Callingread()before thetrykeeps the BigInt, cycle and getter cases (they all fail insideJSON.stringify, and the new tests still pass) and lets a node's own error through as itself.- Changeset summary (
.changeset/lexical-y-quiesce-repeat-draft.md:5). CONTRIBUTING.md and.changeset/README.mdask for a first line of[category] one-line summary, which the changelog shows as the entry; here the whole explanation is the first line. The PR title would work as the summary, with the rest as a body paragraph aboveVerify:.
| phase: 'preflight' | 'final', | ||
| ): string { | ||
| try { | ||
| return JSON.stringify(read()); |
There was a problem hiding this comment.
Optional: read() runs inside this try, so an error from a node's own exportJSON comes out as the "holds a value JSON cannot (a BigInt NodeState value, or a cycle)" TypeError. const json = read(); before the try keeps the BigInt, cycle and getter cases (all thrown by JSON.stringify) and lets a node's own error through unchanged.
| '@lexical/y': patch | ||
| --- | ||
|
|
||
| [fix] `YBinding.quiesce()` called again (before or after `dispose()`) returns a copy of the first call's result: the same state and the same draft, `differs` included. When the first call refused the editor's batched update (the binding was already read only, or the record stopped admitting writes during the call), that update was its draft with `differs: true`, but the refusal read the document back into the editor and a second call rebuilt the draft from it, without the unwritten text and with `differs: false`: a retried handoff lost what the user typed. Later calls now recompute nothing; what was written to the document after the first call is not in their state. The draft is the editor state's JSON (what a quarantine record stores), kept as text: every result, the first included, is its own copy, parsed again, and a NodeState value is as `JSON.stringify` writes it (a `Date` is its ISO string). Before draining, `quiesce()` checks that the pending and the shown editor states serialize as JSON: a failure at that preflight (a `BigInt` NodeState value, a cycle) throws a `TypeError` without draining or quiescing. If app code produces a non-serializable state during the drain, the final serialization may throw a `TypeError` after local work has been written and the composition has ended, with nothing cached: fix the state and retry `quiesce()`, and don't assume earlier edit callbacks didn't run. A warning handler that calls `quiesce()` while the first call's warnings are delivered gets the first call's result. |
There was a problem hiding this comment.
Optional: CONTRIBUTING.md asks for a first line of [category] one-line summary, which the changelog shows as the entry. Something like [fix] quiesce() called again returns the first call's draft, so a retried handoff keeps unwritten text, with the rest of this paragraph below it, would read as intended.
Description
The automated code review (CodeBunny) found a defect.
quiesce()'s JSDoc,y.md's "Handing a replica off" bullets and the
YQuiescedStateFlow commentpromised that a repeat call returns the same draft. That was false when the
first call refused the editor's batched update, which happens when the
binding is already read only or the record stops admitting writes during
the call:
differs: true.text and with
differs: false.A retried handoff lost what the user typed.
quiesce()now keeps its first result and returns copies of it from everycall, the first included, recomputing nothing after the first call. API
review rounds 9 to 11 are folded in:
guard(quiesceFirst) and keeps its result before the status changes.So the refusal's warnings and the status notification go out only after
that, and an
onWarnhandler that retries gets the first call's draft.The outside-an-update check and the kept fast path are unchanged.
and
createYQuarantineRecordrefuses anything else.shown editor state with
JSON.stringify(the preflight). A state JSONcannot hold (a BigInt NodeState value, a cycle, a throwing getter)
throws a
TypeErrorbefore anything has drained or changed.example an update listener's own update. In that case the final
serialization throws a distinct
TypeError(cause kept):quiesce() could not serialize the drained editor state. Local updates may already have been written; no handoff result was cached. Store serializable state and retry quiesce().A private phase argument ofdraftTextpicks the message.
keeps
{state bytes, draft text, differs}.JSON.parseof the text. So a node serializer that reuses its outputobjects cannot change the kept draft, a Date NodeState value is the
same ISO string in every result, and an own
__proto__key survives.editor state's JSON (a Date is its ISO string, in every result). They
say a preflight failure throws a TypeError without draining or
quiescing, and that a state app code makes unserializable during the
drain can throw after local work was written, with nothing cached (fix
it, retry, and don't assume earlier edit callbacks didn't run). They also
say every result is its own copy. The
[fix]changeset's
Verify:paragraph covers:onWarn;dispose();Test plan
Before
Two new files, gc off and on.
regressions/QuiesceRepeat.test.tscovers three ways the first callrefuses the batched update: already read only, a record made newer in an
app transaction, or one made newer by a
beforeTransactionlistener.Each cell retries normally, after mutating a result, and after
dispose().regressions/QuiesceEdges.test.tscovers:onWarnretry;probe; the repeat equals the first);
__proto__key kept;document unchanged, then a JSON state hands off;
nothing changed, and the document, pending update, composition,
status, editability,
quiescedand the cache are untouched;probe): the drained-state message with its cause, the text stored, the
composition ended,
quiescedfalse and no cache, then a retry withserializable state hands off.
QuiesceEdges.test.ts with round 9's binding (bd508881f, its
structuredClone copies), gc off and on:
With round 10's binding (c61768f), the drain-time cell fails in both gc
modes (
Tests 2 failed | 18 passed (20)): the error saidnothing was changed,expected '@lexical/y: quiesce() hands off the e…' to be '@lexical/y: quiesce() could not seria…'.On the parent (no fix), QuiesceRepeat.test.ts fails 6 of 6 (
expected false to be true, the draft'sdiffers).After
vitest --run --project unit packages/lexical-y/src/__tests__/unit/regressions/QuiesceEdges.test.ts packages/lexical-y/src/__tests__/unit/regressions/QuiesceRepeat.test.ts packages/lexical-y/src/__tests__/unit/regressions/MigrateQuiesceDraft.test.ts packages/lexical-y/src/__tests__/unit/MigrateCore.test.ts packages/lexical-y/src/__tests__/unit/regressions/BindingLifecycleEdges.test.ts,gc off and on:
tsc: 0 errors.node scripts/check-flow-types.mjs:Found 0 errors.node scripts/changesets/check.mjs: 110 valid.The whole lexical-y and lexical-yjs unit projects on this commit: gc off
22,311 passed, 315 expected fail; gc on 22,311 passed, 315 expected fail.
pnpm run ci-check: exit 0.pnpm run build-release: exit 0(LexicalY.prod.js +804 / +327 bytes min/gzip against 89cccda).
Stacked on #9777 (which is stacked on #9766); it merges right after them, before any release.