Repository navigation
[lexical-y][lexical-website] Chore: Follow-ups to the lone surrogate fix and D98 (review items) - #9718
Conversation
…fix and D98 (review items) ## Description Review items deferred from the lone surrogate fix and the D98 docs change, none of which changes stored content: - `hasLoneSurrogate` (the keystroke path's check) uses `String#isWellFormed` where the runtime has it, which builds no new string, with the current loop as the fallback (changeset `[perf]`). - y.md: `@lexical/yjs` also delivers U+FFFD to other clients, but there the author keeps the lone half until it reloads (the sentence read as if it also re-read the line). - D98 and y.md: an undo manager destroyed without a clear *can* keep a later-deleted table: it does when the delete's step is the first thing to keep the cell (Yjs's `keepItem` stops at an ancestor already kept); a cell the binding already kept for its own format (D88) is let go when it stops, so the table is collected. - Tests: `matrix/SurrogateShapes.test.ts` types lone high and low halves into every text node of every catalog shape that holds line text (links, list items, table cells, code lines, headings, quotes, token and segmented text, the text around inline shapes: 45 shapes), gc off and on, and expects every client and a fresh read of each to show exactly what an editor without collaboration shows when the app writes U+FFFD itself (text, and format and style per character); `SurrogateSelection.test.ts` freshly reads every client's document, the author's included; `UndoManagerStopWithoutClear.test.ts` covers both edit orders and checks in every case that the collaborator's delete arrived. ## Test plan ### Before `SurrogateShapes.test.ts` on main before the lone surrogate fix (2393e94): `Tests 90 failed (90)`. Its oracle rejects a binding that drops the U+FFFD of a trailing half in single-node paragraphs (10 of 90 cells fail) one that drops every U+FFFD it writes (90 of 90), and one that drops lone low halves inside a node (90 of 90). ### After `packages/lexical-y` unit tests: gc off 19513 passed | 351 expected fail | 0 failed; gc on 19510 passed | 354 expected fail | 0 failed. `ci-check` passes.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
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: |
2d231d8 to
d609428
Compare
d609428 to
403ee03
Compare
403ee03 to
aca4169
Compare
aca4169 to
80c749b
Compare
![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. Switching to String#isWellFormed gives the same answer as the old check for every input and avoids building a new string on the keystroke path. The D98 nuance matches how the binding records and releases its own keeps, and the new SurrogateShapes matrix is a solid oracle: it compares against an editor without collaboration that writes U+FFFD itself. The suggestions below are optional and non-blocking.
1. State the D98 condition per table, not per cell: packages/lexical-website/docs/collaboration/y.md:1196-1198 and packages/lexical-y/DECISIONS.md:130
The page says the table is kept "when nothing kept the cell before the delete", and D98 says "when the delete's step is the first thing to keep the cell". A bold anywhere else in the same table before the delete has the same effect as one in the same cell. Yjs's keepItem stops at the row or table the binding already kept, and the binding's release at stop lets the whole table go. Moving the bold in UndoManagerStopWithoutClear.test.ts to a second cell of the same row, or to a second row, leaves 0 deleted containers in B, just like the new bold, then delete case. A wording such as "it does when nothing in that table was kept before the delete; a table the binding already kept for a format of its own, in any of its cells, is let go when it stops" would say when it happens.
2. Reword or drop the changeset's Verify: line: .changeset/lexical-y-lone-surrogate-check.md:7
Once a surrogate is present, the check still scans the whole text, so typing in an emoji-holding node is much faster than before but not literally the same cost as typing in one without an emoji. Per .changeset/README.md (Behavior changes), any non-breaking entry with a Verify: paragraph is listed under Behavior changes, and a speedup with identical results gives upgraders nothing to check there. Dropping the paragraph, or saying "is no slower than before", would fit better.
3. Cover the fallback branch: packages/lexical-y/src/internal/Inline.ts:1238
CI's Node versions all have String#isWellFormed, and no test calls hasLoneSurrogate directly, so toWellFormed(s) !== s now only runs in older browsers. A short unit test that removes String.prototype.isWellFormed for one case, then restores it, would keep it covered.
4. Small test polish: packages/lexical-y/src/__tests__/unit/regressions/UndoManagerStopWithoutClear.test.ts
The header comment (lines 11-19) still says the delete's step keeps the cell, row and table unconditionally, which the new bold, then delete case contradicts. The kept column is only read as zero or non-zero; expect(left).toHaveLength(kept) would check the number it states.
AGENTS.md (Commits and Pull Requests) also asks the Test plan to paste the actual Before and After output rather than summarise it.
| keep the cell, its row and its table (it does when nothing kept the cell | ||
| before the delete; a cell the binding already kept for a format of its | ||
| own is let go when it stops). A table a collaborator deletes afterwards |
There was a problem hiding this comment.
Optional: the condition is per table rather than per cell. A bold in another cell of the same table (same row or another row) before the delete also lets the table go at stop, since Yjs's keepItem stops at the row or table the binding already kept. Something like "it does when nothing in that table was kept before the delete; a table the binding already kept for a format of its own, in any of its cells, is let go when it stops" would cover it.
There was a problem hiding this comment.
Agreed, the condition is per table (keepItem stops at the first kept ancestor, cell, row or table). Deferred to a later docs touch-up to keep this head stable; noted with your wording.
| | D91 | **Overlapping formats of one key from two writers can cancel:** two collaborators applying the same format (bold, the same comment id, a link) to overlapping ranges at once can leave part of the text without it. Since [#9601] this also reaches a link's edge and its first word (D82). | A Yjs text gives each place one value per key ([yjs#382]): a writer's end of its range ends the other writer's run there too. Avoiding it needs a marker per writer, a storage change. Formatting only, no text lost; [#9601] is kept. | `ACCEPTED LIMIT` | 10-31 | [#9626], [#9601], [limits] | | ||
| | D93 | **A comment on a word a collaborator rewrites at the same moment can land on the new word:** with `setTextContent` for one client id order, and with a block rewrite (the line's children replaced by one new text node, as an agent writes it) for one order when the comment ends at the rewritten word and for both when it continues past it; words of the range the rewrite kept keep the comment. | The same race as text typed at the start of a range a collaborator formats (y.md, Other races): the new word is inserted where the commented word starts, and Yjs orders the two writes by client id. Before [#9598] a block rewrite dropped the comment from the whole range instead. Formatting only, no text lost. | `ACCEPTED LIMIT` | 10-31 | [#9610], [limits] | | ||
| | D98 | **A Yjs `UndoManager` destroyed without a clear keeps what its steps protected.** The binding destroys its undo manager without clearing it when it stops (`binding.dispose()`, or the cleanup its start returns: an editor unmounted or mounted again), and Yjs destroys an `UndoManager` when its document is destroyed. Yjs then keeps, until the document is reloaded, the items the destroyed manager's steps kept: a character deleted in a table cell keeps that cell, its row and its table. A table a collaborator deletes afterwards stays in that client's document, and in any state it encodes: 108 to 175 bytes per table where one character was deleted and one word bolded in one cell of a 1x1 to a 4x4 table, more where more was deleted or more cells were edited (about 1.3 KB for one character deleted in every cell of a 4x4 table). Binding the same document again does not free it. Clearing the history (`binding.undoManager?.clear()`, while the binding is started) before stopping and before the table is deleted prevents it, where no other `UndoManager` on the document still needs what it kept (a clear releases that too); a table deleted while the history still held such a step stays after a later clear too, as releasing a keep collects nothing (D88). Reloading the document, or loading its encoded state into a new document with garbage collection on (a server or relay), compacts it. | Yjs's `UndoManager#destroy` does not clear, and its keep is one flag set on an item and its parents; any editor that destroys a Yjs `UndoManager` without clearing it first keeps the same items. Measured over 50 cycles per table size (an edit in a cell, the binding stopped, the table deleted by a collaborator): the same bytes on the release before D88's own-format keep, with that keep turned off and on the current release, and none when the history was cleared before stopping. Clearing at stop in the binding was weighed and not taken: Yjs's clear releases with that one flag, so it would also release what another `UndoManager` on the document (an app's, or a second binding's) still keeps for its own undo, and a later collection could drop what that undo restores (the hazard D88 records for a clear). | `ACCEPTED LIMIT` | 11-01 | [#9698], [limits] | | ||
| | D98 | **A Yjs `UndoManager` destroyed without a clear keeps what its steps protected.** The binding destroys its undo manager without clearing it when it stops (`binding.dispose()`, or the cleanup its start returns: an editor unmounted or mounted again), and Yjs destroys an `UndoManager` when its document is destroyed. Yjs then keeps, until the document is reloaded, the items the destroyed manager's steps kept: a character deleted in a table cell can keep that cell, its row and its table. It does when the delete's step is the first thing to keep the cell (Yjs's `keepItem` stops at an ancestor already kept); where the binding already kept the cell for one of its own format items (D88) before the delete, the step keeps only the character, and the binding's release at stop lets the cell, row and table go. When it does, a table a collaborator deletes afterwards stays in that client's document, and in any state it encodes: 108 to 175 bytes per table where one character was deleted and one word bolded in one cell of a 1x1 to a 4x4 table, more where more was deleted or more cells were edited (about 1.3 KB for one character deleted in every cell of a 4x4 table). Binding the same document again does not free it. Clearing the history (`binding.undoManager?.clear()`, while the binding is started) before stopping and before the table is deleted prevents it, where no other `UndoManager` on the document still needs what it kept (a clear releases that too); a table deleted while the history still held such a step stays after a later clear too, as releasing a keep collects nothing (D88). Reloading the document, or loading its encoded state into a new document with garbage collection on (a server or relay), compacts it. | Yjs's `UndoManager#destroy` does not clear, and its keep is one flag set on an item and its parents; any editor that destroys a Yjs `UndoManager` without clearing it first keeps the same items. Measured over 50 cycles per table size (an edit in a cell, the binding stopped, the table deleted by a collaborator): the same bytes on the release before D88's own-format keep, with that keep turned off and on the current release, and none when the history was cleared before stopping. Clearing at stop in the binding was weighed and not taken: Yjs's clear releases with that one flag, so it would also release what another `UndoManager` on the document (an app's, or a second binding's) still keeps for its own undo, and a later collection could drop what that undo restores (the hazard D88 records for a clear). | `ACCEPTED LIMIT` | 11-01 | [#9698], [limits] | |
There was a problem hiding this comment.
Optional: same as on the y.md caveat. "the first thing to keep the cell" is "the first thing to keep the table": an earlier own-format keep in any cell or row of the same table makes the step's walk stop below the table, and the release at stop lets it go.
There was a problem hiding this comment.
Same as on y.md: deferred, noted.
|
|
||
| [perf] Check typed text for a lone surrogate half in `@lexical/y` without building a new string where the runtime has `String.prototype.isWellFormed` | ||
|
|
||
| Verify: typing in a long text node that holds an emoji costs no more than in one without it. |
There was a problem hiding this comment.
Optional: with a Verify: paragraph this entry is listed under Behavior changes (.changeset/README.md), and the check still scans the whole text once a surrogate is present, so it is not literally the same cost as text without one. Dropping the paragraph, or "is no slower than before", fits a [perf] entry better.
There was a problem hiding this comment.
Agreed: the paragraph puts a perf entry under Behavior changes. Deferred to the next change in this package to keep this head stable.
| const str = s as string & {isWellFormed?: () => boolean}; | ||
| return typeof str.isWellFormed === 'function' | ||
| ? !str.isWellFormed() | ||
| : toWellFormed(s) !== s; |
There was a problem hiding this comment.
Optional: CI's Node versions all have String#isWellFormed, so no test reaches this fallback any more. A short unit test that removes String.prototype.isWellFormed for one case, then restores it, would keep it covered.
There was a problem hiding this comment.
Agreed; deferred: a unit test that hides String.prototype.isWellFormed for one case keeps the fallback covered.
Description
Review items deferred from the lone surrogate fix and the D98 docs change, none of which changes stored content:
hasLoneSurrogate(the keystroke path's check) usesString#isWellFormedwhere the runtime has it, which builds no new string, with the current loop as the fallback (changeset[perf]).@lexical/yjsalso delivers U+FFFD to other clients, but there the author keeps the lone half until it reloads (the sentence read as if it also re-read the line).keepItemstops at an ancestor already kept); a cell the binding already kept for its own format (D88) is let go when it stops, so the table is collected.matrix/SurrogateShapes.test.tstypes lone high and low halves into every text node of every catalog shape that holds line text (links, list items, table cells, code lines, headings, quotes, token and segmented text, the text around inline shapes: 45 shapes), gc off and on, and expects every client and a fresh read of each to show exactly what an editor without collaboration shows when the app writes U+FFFD itself (text, and format and style per character);SurrogateSelection.test.tsfreshly reads every client's document, the author's included;UndoManagerStopWithoutClear.test.tscovers both edit orders and checks in every case that the collaborator's delete arrived.Test plan
Before
SurrogateShapes.test.tson main before the lone surrogate fix (2393e94):Tests 90 failed (90). Its oracle rejects a binding that drops the U+FFFD of a trailing half in single-node paragraphs (10 of 90 cells fail) one that drops every U+FFFD it writes (90 of 90), and one that drops lone low halves inside a node (90 of 90).After
packages/lexical-yunit tests: gc off 19513 passed | 351 expected fail | 0 failed; gc on 19510 passed | 354 expected fail | 0 failed.ci-checkpasses.