Repository navigation
[lexical-y] Refactor: One module answers what a binding's editor sees of Yjs - #9533
Merged
Merged
Conversation
… of Yjs
## Description
What a binding's editor shows of the document is Yjs minus two things: the remote changes it has not read yet (the `Unread` view, used while it writes a local update made before them) and the items held back while the local editor composes (`Hidden`). The questions about it were spread over three modules: `isGone`, `attrOf`, `attrsOf`, `attrsBefore` and the write helpers lived in `Unread.ts`, `isShown`, `shownLength` and `shownText` in `Hidden.ts`, and `isShownEnd`, `shownAttrs` and `prevLiveEnd` in `State.ts`. Several call sites also combined the hold by hand (`hidden === null || !hidden.hides(it)`, `hidden === null ? it.length : hidden.shownLength(it)`).
This moves all of them, unchanged, into a new internal module, `Seen.ts`, whose header states what the editor sees and which helper answers what. `Unread.ts` keeps the view itself (`seenBy`, `emptyView`, and `viewOf`, `inserted`, `deleted` for `Seen`), and `Hidden.ts` keeps the hold. Two small helpers name the hand-written forms: `isHeld(item, hidden)` and `unheldLength(item, hidden)`. Every reader and writer now imports these from one place.
No behavior changes: the moved functions are the same code (compared token by token with comments stripped), and each substituted expression is the same expression. The raw `item.deleted` reads in `Cursor`'s run-opener scans (`liveItemBefore`, `runOpener`, `restatedEnd`) are left as they are: reading them through the unread view writes different bytes when a local format change is written while a collaborator's format change is still unread, so that switch is a behavior change for its own PR.
## Changeset
None: internal refactor, no public API or behavior change.
## Test plan
Every gate compares this branch with `main` (`0823b74e`) on the same machine.
### Before
`main`: golden corpus recorded (below), full unit suite as listed under After, and the same extended fuzz runs.
### After
Stored bytes over fuzz seeds: a scratch recorder (not committed) runs fuzz scripts and hashes, per seed and peer, the Yjs state bytes, the editor JSON, the warnings, the selection and the node map.
```
bid: 1000 seeds, 0 differ
bidmoves: 600 seeds, 0 differ
gc: 1000 seeds, 0 differ
keys: 600 seeds, 0 differ
long: 400 seeds, 0 differ
moves: 1500 seeds, 0 differ
nomove: 3000 seeds, 0 differ
GOLD IDENTICAL
```
Stored bytes over the test corpus: every `@lexical/y` unit test (the saved fuzz scripts in `__tests__/fuzz/` included) run with a setup file that gives each `Y.Doc` a deterministic client id and hashes its state bytes after the test.
```
tests: 7286; stable across base runs: 7047 (7024 with docs, 67564 docs); unstable: 239
new run: 7286 tests; missing from new: 0; stable tests whose bytes differ: 0
CAP IDENTICAL
```
`pnpm run ci-check`:
```
Changeset check passed: 30 pending changeset(s) valid.
Found 0 errors
```
`vitest --project unit --project scripts-unit` (36 workers), per test against `main`:
```
base {'passed': 18179, 'skipped': 6, 'failed': 1}
new {'passed': 18179, 'skipped': 7, 'failed': 2}
missing 0, added 2, status changed 1
CHANGED passed -> failed FollowUpCheckCost.test.ts (timing threshold)
ADDED passed treeShakingSource.test.ts :: ... @lexical/y (packages/lexical-y/src/internal/Seen.ts)
```
The one failure on each side is a timing or heap threshold under 36 parallel workers (`DisposeMemory` on `main`, `FollowUpCheckCost` here); run alone, both pass:
```
npx vitest run --project unit DisposeMemory.test.ts FollowUpCheckCost.test.ts Cursor.test.ts CompositionHoldBack.test.ts
Test Files 4 passed (4)
Tests 1827 passed | 2 skipped (1829)
```
Extended fuzz, same results as `main`: `Fuzz.test.ts` seeds 1 to 3000 (gc 1000) and 1 to 1000 with block ids, `ConcurrentShapesFuzz` 600 seeds, `FollowUpFuzz` 1000 seeds, `NestedFuzz`, `SlotsFuzz`, `BlockIdFuzzSeeds` and `FuzzReplay` pass. Seed 2277 fails with `dup` on `main` and on this branch alike (outside the default 300 seeds; not caused by this change).
Browser and e2e tests were not run: no code path changes.
zurfyx
requested review from
acywatson,
etrepum,
ivailop7 and
potatowagon
as code owners
October 29, 2026 22:28
|
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: |
zurfyx
added a commit
that referenced
this pull request
Oct 29, 2026
…rough Seen ## Description Stacked on #9533 (base branch `zurfyx/y-seen-view`; one module for what a binding's editor sees of Yjs). A binding must write node attributes through `Seen`'s `setAttr` and `deleteAttr`: while a remote change waits to be read, they write where the binding's editor sees the document. A raw `y.setAttr(...)` writes where Yjs has it now and brings back what that change deleted. Misses of this kind were found by review twice before (the N2 and G-4 items of the integration review); nothing enforced the rule. This adds a source test next to the existing `process.env` one. It parses each internal module except `Seen.ts` with the TypeScript AST and lists every use of a `Y.Node` method that writes attributes (`setAttr`, `deleteAttr`, `clearAttrs`, `applyDelta`; property or string-literal element access, called or only referenced, as in a `bind`), and every call shaped like `Y.Node`'s `delete(index, length)` or `format(index, length, attrs)`, each by the function it is in and its first argument. The only ones allowed are the five writes of the root's records (the layout and schema versions written, a type's pinned storage), listed by call site and key: they are not content the editor shows. Comments and strings do not hide or invent a match. Not covered: `Item.delete(tr)` (a binding deletes items through `deleteItem`), which a syntactic scan cannot tell from a `Map` or `Set` delete; reads, which the reader of remote changes and scans of what each client wrote do raw on purpose; and writes reached through computed names or other modules' helpers. ## Changeset None: test only. ## Test plan ### Before With a scratch module `internal/ZZProbe.ts` of raw writes (optional calls, bracket access, a bound alias, comment and string tricks, `clearAttrs`, `applyDelta`, `delete(0, 1)`, `format(0, 1, …)`; not committed): ``` npx vitest run --project unit packages/lexical-y/src/__tests__/unit/Source.test.ts + "ZZProbe.ts probe: node.setAttr('x')", + "ZZProbe.ts probe: node.setAttr('x')", + "ZZProbe.ts probe: node.setAttr('x')", + "ZZProbe.ts probe: node.setAttr((not called))", + "ZZProbe.ts probe: node.setAttr('x')", + "ZZProbe.ts probe: node.deleteAttr('x')", + "ZZProbe.ts probe: node.setAttr('y')", + "ZZProbe.ts probe: node.clearAttrs()", + "ZZProbe.ts probe: node.applyDelta(node.change['setAttr']('x', 'new'))", + "ZZProbe.ts probe: node.change.setAttr('x')", + "ZZProbe.ts probe: node.delete(0)", + "ZZProbe.ts probe: node.format(0)", Tests 1 failed | 3 passed (4) ``` ### After On this branch as committed: ``` npx vitest run --project unit packages/lexical-y/src/__tests__/unit/Source.test.ts Tests 4 passed (4) ``` `pnpm run ci-check`: `Found 0 errors`.
zurfyx
added a commit
that referenced
this pull request
Oct 29, 2026
…rough Seen ## Description Stacked on #9533 (base branch `zurfyx/y-seen-view`; one module for what a binding's editor sees of Yjs). A binding must write node attributes through `Seen`'s `setAttr` and `deleteAttr`: while a remote change waits to be read, they write where the binding's editor sees the document. A raw `y.setAttr(...)` writes where Yjs has it now and brings back what that change deleted. Misses of this kind were found by review twice before (the N2 and G-4 items of the integration review); nothing enforced the rule. This adds a syntactic guard, a source test next to the existing `process.env` one. It parses each internal module except `Seen.ts` with the TypeScript AST and lists every use of a `Y.Node` method that writes attributes (`setAttr`, `deleteAttr`, `clearAttrs`, `applyDelta`; property or string-literal element access, called or only referenced as in a `bind`, or destructured), and every call shaped like `Y.Node`'s `delete(index, length)` or `format(index, length, attrs)`, each by the function it is in and its first argument. The only ones allowed are the five writes of the root's records (the layout and schema versions written, a type's pinned storage), listed by call site and key: they are not content the editor shows. Comments and strings do not hide or invent a match. It is not exhaustive. Not covered: `Item.delete(tr)` (a binding deletes items through `deleteItem`) and a one-argument `Y.Node#delete(index)`, which a syntactic scan cannot tell from a `Map` or `Set` delete; aliases of `delete` and `format`; computed method names; writes reached through other modules' helpers; and reads, which the reader of remote changes and scans of what each client wrote do raw on purpose. ## Changeset None: test only. ## Test plan ### Before With a scratch module `internal/ZZProbe.ts` of raw writes (optional calls, bracket access, a bound alias, comment and string tricks, `clearAttrs`, `applyDelta`, `delete(0, 1)`, `format(0, 1, …)`; not committed; a second probe with destructured and parenthesized forms is caught the same way): ``` npx vitest run --project unit packages/lexical-y/src/__tests__/unit/Source.test.ts + "ZZProbe.ts probe: node.setAttr('x')", + "ZZProbe.ts probe: node.setAttr('x')", + "ZZProbe.ts probe: node.setAttr('x')", + "ZZProbe.ts probe: node.setAttr((not called))", + "ZZProbe.ts probe: node.setAttr('x')", + "ZZProbe.ts probe: node.deleteAttr('x')", + "ZZProbe.ts probe: node.setAttr('y')", + "ZZProbe.ts probe: node.clearAttrs()", + "ZZProbe.ts probe: node.applyDelta(node.change['setAttr']('x', 'new'))", + "ZZProbe.ts probe: node.change.setAttr('x')", + "ZZProbe.ts probe: node.delete(0)", + "ZZProbe.ts probe: node.format(0)", Tests 1 failed | 3 passed (4) ``` ### After On this branch as committed: ``` npx vitest run --project unit packages/lexical-y/src/__tests__/unit/Source.test.ts Tests 4 passed (4) ``` `pnpm run ci-check`: `Found 0 errors`.
## Description This commit changes no file. The repository's review bot reviews a pull request when its branch is pushed; this branch was last pushed before the bot started reviewing the repository, so it had no review yet.
![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. The helpers move into packages/lexical-y/src/internal/Seen.ts unchanged apart from three bodies, and there the new isHeld and unheldLength helpers compute exactly the expressions they replace, in the same order, as they do at the six call sites in Read.ts, Write.ts and Inline.ts. Two optional, non-blocking suggestions:
- Changeset label. The Changesets check warns that
@lexical/ychanged without a changeset. CONTRIBUTING.md ("Changesets") says changes with no effect on people who use Lexical need none, and that a maintainer can add theskip-changesetlabel when CI asks anyway; that label fits this internal refactor better than a[chore]entry in the changelog. - Header wording in
Seen.ts(lines 35-38). "Where no view applies to an item's type (see viewOf), they are Yjs's owndeleted, getAttr and getAttrs" comes right after a list that includesattrsBefore, which with no view still skips whattrinserted (line 152) and keeps whattrdeleted (line 163), and it leaves the writers' fallbacks unnamed. Something like: "Where no view applies to an item's type (see viewOf), isGone, attrOf and attrsOf are Yjs's owndeleted, getAttr and getAttrs, and the writers are Yjs's own setAttr, deleteAttr and delete."
Comment on lines
+35
to
+38
| * before. isGone, attrOf, attrsOf and attrsBefore read through them; | ||
| * setAttr, deleteAttr and deleteItem write through them. Where no view | ||
| * applies to an item's type (see viewOf), they are Yjs's own `deleted`, | ||
| * getAttr and getAttrs. |
There was a problem hiding this comment.
Nit (optional): with no view, attrsBefore (listed just above) is not getAttrs: it still skips what tr inserted (line 152) and keeps what tr deleted (line 163), and the writers' fallbacks (setAttr, deleteAttr, delete) are not named. Suggested: "Where no view applies to an item's type (see viewOf), isGone, attrOf and attrsOf are Yjs's own deleted, getAttr and getAttrs, and the writers are Yjs's own setAttr, deleteAttr and delete."
zurfyx
added a commit
that referenced
this pull request
Oct 30, 2026
…rough Seen ## Description Stacked on #9533 (base branch `zurfyx/y-seen-view`; one module for what a binding's editor sees of Yjs). A binding must write node attributes through `Seen`'s `setAttr` and `deleteAttr`: while a remote change waits to be read, they write where the binding's editor sees the document. A raw `y.setAttr(...)` writes where Yjs has it now and brings back what that change deleted. Misses of this kind were found by review twice before (the N2 and G-4 items of the integration review); nothing enforced the rule. This adds a syntactic guard, a source test next to the existing `process.env` one. It parses each internal module except `Seen.ts` with the TypeScript AST and lists every use of a `Y.Node` method that writes attributes (`setAttr`, `deleteAttr`, `clearAttrs`, `applyDelta`; property or string-literal element access, called or only referenced as in a `bind`, or destructured), and every call shaped like `Y.Node`'s `delete(index, length)` or `format(index, length, attrs)`, each by the function it is in and its first argument. The only ones allowed are the five writes of the root's records (the layout and schema versions written, a type's pinned storage), listed by call site and key: they are not content the editor shows. Comments and strings do not hide or invent a match. It is not exhaustive. Not covered: `Item.delete(tr)` (a binding deletes items through `deleteItem`) and a one-argument `Y.Node#delete(index)`, which a syntactic scan cannot tell from a `Map` or `Set` delete; aliases of `delete` and `format`; computed method names; writes reached through other modules' helpers; and reads, which the reader of remote changes and scans of what each client wrote do raw on purpose. ## Changeset None: test only. ## Test plan ### Before With a scratch module `internal/ZZProbe.ts` of raw writes (optional calls, bracket access, a bound alias, comment and string tricks, `clearAttrs`, `applyDelta`, `delete(0, 1)`, `format(0, 1, …)`; not committed; a second probe with destructured and parenthesized forms is caught the same way): ``` npx vitest run --project unit packages/lexical-y/src/__tests__/unit/Source.test.ts + "ZZProbe.ts probe: node.setAttr('x')", + "ZZProbe.ts probe: node.setAttr('x')", + "ZZProbe.ts probe: node.setAttr('x')", + "ZZProbe.ts probe: node.setAttr((not called))", + "ZZProbe.ts probe: node.setAttr('x')", + "ZZProbe.ts probe: node.deleteAttr('x')", + "ZZProbe.ts probe: node.setAttr('y')", + "ZZProbe.ts probe: node.clearAttrs()", + "ZZProbe.ts probe: node.applyDelta(node.change['setAttr']('x', 'new'))", + "ZZProbe.ts probe: node.change.setAttr('x')", + "ZZProbe.ts probe: node.delete(0)", + "ZZProbe.ts probe: node.format(0)", Tests 1 failed | 3 passed (4) ``` ### After On this branch as committed: ``` npx vitest run --project unit packages/lexical-y/src/__tests__/unit/Source.test.ts Tests 4 passed (4) ``` `pnpm run ci-check`: `Found 0 errors`.
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.
Description
What a binding's editor shows of the document is Yjs minus two things: the remote changes it has not read yet (the
Unreadview, used while it writes a local update made before them) and the items held back while the local editor composes (Hidden). The questions about it were spread over three modules:isGone,attrOf,attrsOf,attrsBeforeand the write helpers lived inUnread.ts,isShown,shownLengthandshownTextinHidden.ts, andisShownEnd,shownAttrsandprevLiveEndinState.ts. Several call sites also combined the hold by hand (hidden === null || !hidden.hides(it),hidden === null ? it.length : hidden.shownLength(it)).This moves all of them, unchanged, into a new internal module,
Seen.ts, whose header states what the editor sees and which helper answers what.Unread.tskeeps the view itself (seenBy,emptyView, andviewOf,inserted,deletedforSeen), andHidden.tskeeps the hold. Two small helpers name the hand-written forms:isHeld(item, hidden)andunheldLength(item, hidden). Every reader and writer now imports these from one place.No behavior changes: the moved functions are the same code (compared token by token with comments stripped), and each substituted expression is the same expression. The raw
item.deletedreads inCursor's run-opener scans (liveItemBefore,runOpener,restatedEnd) are left as they are: reading them through the unread view writes different bytes when a local format change is written while a collaborator's format change is still unread, so that switch is a behavior change for its own PR.Changeset
None: internal refactor, no public API or behavior change.
Test plan
Every gate compares this branch with
main(0823b74e) on the same machine.Before
main: golden corpus recorded (below), full unit suite as listed under After, and the same extended fuzz runs.After
Stored bytes over fuzz seeds: a scratch recorder (not committed) runs fuzz scripts and hashes, per seed and peer, the Yjs state bytes, the editor JSON, the warnings, the selection and the node map.
Stored bytes over the test corpus: every
@lexical/yunit test (the saved fuzz scripts in__tests__/fuzz/included) run with a setup file that gives eachY.Doca deterministic client id and hashes its state bytes after the test.pnpm run ci-check:vitest --project unit --project scripts-unit(36 workers), per test againstmain:The one failure on each side is a timing or heap threshold under 36 parallel workers (
DisposeMemoryonmain,FollowUpCheckCosthere); run alone, both pass:Extended fuzz, same results as
main:Fuzz.test.tsseeds 1 to 3000 (gc 1000) and 1 to 1000 with block ids,ConcurrentShapesFuzz600 seeds,FollowUpFuzz1000 seeds,NestedFuzz,SlotsFuzz,BlockIdFuzzSeedsandFuzzReplaypass. Seed 2277 fails withduponmainand on this branch alike (outside the default 300 seeds; not caused by this change).Browser and e2e tests were not run: no code path changes.