Repository navigation
[lexical-y] Chore: Test that internal writes of node attributes go through Seen - #9550
Merged
Merged
Conversation
|
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 29, 2026 23:20
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
force-pushed
the
zurfyx/y-seen-writes-guard
branch
from
October 29, 2026 23:36
c4967f0 to
59785e1
Compare
zurfyx
force-pushed
the
zurfyx/y-seen-writes-guard
branch
from
October 29, 2026 23:51
59785e1 to
ad94a30
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. The guard reports exactly the five root-record writes at this head and fails when an internal module writes an attribute raw (for example y.setAttr(attr, slotY) in place of setAttr(y, attr, slotY, writer.tr) from Seen in Slots.ts). The suggestions below are optional and non-blocking.
- Report the three-argument
insert(packages/lexical-y/src/__tests__/unit/Source.test.ts, lines 116–120). In@y/y14.0.0-rc.29,Y.Node#insert(index, content, formats)writes formats at an index counted in the document as Yjs has it now, just like the coveredformat(index, length, formats)(both apply a delta that starts withretain(index)), but it isn't reported and isn't in the description's not-covered list. The binding's ownCursor#insert(content, formats)takes two arguments (packages/lexical-y/src/internal/Cursor.ts, line 686), so adding(method === 'insert' && args !== null && args.length === 3)reports nothing today. The two-argument form andpush/unshiftcan't be told apart syntactically and could join the not-covered list. - Name the limits in the comment (lines 45–55). The not-covered list lives only in the commit message, and a few forms are in neither: a destructuring assignment
({setAttr: f} = y)(the declarationconst {setAttr} = yis reported), adeleteorformatcallee wrapped in!oras(the unwrapping at lines 107–109 steps through parentheses only), and a rawnew Item(…, y, key, content).integrate(tr, 0), which is howSeen.setAttritself writes. One sentence there keeps a green run from reading as proof that no raw write remains. - Keep entries whitespace-stable (line 129).
args[0].getText(file)keeps a wrapped argument's line break and indentation, so a reformat alone could change an expected entry;args[0].getText(file).replace(/\s+/g, ' ')avoids that. - Scan every non-test module (lines 63–66). Only top-level
src/internal/*.tsis parsed; walkingsrcrecursively, as theprocess.envtest above does, would also cover a future subdirectory and the two entry modules, and reports nothing more today.
No changeset is needed for a test-only change (CONTRIBUTING.md, Changesets section).
Comment on lines
+45
to
+55
| // A binding writes node attributes through Seen (setAttr, deleteAttr): while | ||
| // a remote change waits to be read, those write where the binding's editor | ||
| // sees the document. Written raw, an attribute is set where Yjs has it now, | ||
| // and the write brings back what that change deleted. The root's records | ||
| // (the layout and schema versions written, and the pinned storage of a | ||
| // type) are written raw: they are not content the editor shows. The scan is | ||
| // syntactic (the TypeScript AST of each internal module): it lists every | ||
| // use of a Y.Node method that writes attributes (`setAttr`, `deleteAttr`, | ||
| // `clearAttrs`, `applyDelta`), called or not, and every call shaped like | ||
| // Y.Node's `delete(index, length)` or `format(index, length, attrs)`, by | ||
| // the function it is in and its first argument. |
There was a problem hiding this comment.
Optional: this says what the scan reports but not what it can't see; the not-covered list is only in the commit message. A sentence here (plus forms that list omits: a destructuring assignment ({setAttr: f} = y), a delete/format callee wrapped in ! or as, and a raw new Item(…).integrate(tr, 0), which is how Seen.setAttr writes) keeps a green run from reading as proof that no raw write remains.
Comment on lines
+63
to
+66
| const dir = path.resolve(__dirname, '../../internal'); | ||
| const found: string[] = []; | ||
| for (const name of fs.readdirSync(dir).sort()) { | ||
| if (!name.endsWith('.ts') || name === 'Seen.ts') { |
There was a problem hiding this comment.
Optional: only top-level src/internal/*.ts is parsed, so a module in a future subdirectory of internal/ (or a write in src/headless.ts / src/index.ts) is never scanned. Walking every non-test module under src, as the process.env test above does, reports nothing more today.
Comment on lines
+118
to
+119
| (method === 'delete' && args !== null && args.length === 2) || | ||
| (method === 'format' && args !== null && args.length === 3) |
There was a problem hiding this comment.
Optional: Y.Node#insert(index, content, formats) writes formats at a Yjs index just like format(index, length, formats) (in @y/y 14.0.0-rc.29 both apply a delta that starts with retain(index)), but it isn't reported. The binding's Cursor#insert(content, formats) takes two arguments, so a three-argument rule reports nothing today:
(method === 'insert' && args !== null && args.length === 3) ||The two-argument form and push/unshift can't be told apart syntactically; they could join the not-covered list.
| ? '(not called)' | ||
| : args.length === 0 | ||
| ? '' | ||
| : args[0].getText(file); |
There was a problem hiding this comment.
Optional: getText keeps a wrapped first argument's line break and indentation, so an allowed write whose key Prettier wraps would need them in its expected entry, and a reformat alone would fail the test. args[0].getText(file).replace(/\s+/g, ' ') keeps entries stable.
…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.
zurfyx
force-pushed
the
zurfyx/y-seen-writes-guard
branch
from
October 30, 2026 20:23
51be725 to
e1963c5
Compare
There was a problem hiding this comment.
Thanks, this looks good. The guard does what the description says: it passes on this branch, reports each of the listed shapes once on a scratch module (optional calls, bracket and template-literal access, bound aliases, parentheses, destructuring, clearAttrs, applyDelta, two-argument delete, three-argument format), ignores comments and strings, and, run on the parent of #9523, reports the nine raw setAttr/deleteAttr calls that change moved behind the view-aware helpers.
One optional, non-blocking suggestion:
- Two attribute-writing forms pass the scan and are not in the "Not covered" list (
packages/lexical-y/src/__tests__/unit/Source.test.ts, lines 45-55 and 116-120). In@y/y14.0.0-rc.29,Y.Node#insert(index, content, format)writes formatting attributes through the sameapplyDeltapath asformat(index, length, formats), which the scan already reports; andnew Item(id, left, origin, right, rightOrigin, parent, parentSub, content).integrate(tr, 0)with a stringparentSubwrites an attribute directly, the waySeen.setAttrdoes (packages/lexical-y/src/internal/Seen.ts, lines 194-203). Adding(method === 'insert' && args !== null && args.length === 3)next to theformatrule, and reporting anew Item(...)whose seventh argument is not the literalnull, finds nothing new on this branch (Cursor#inserttakes two arguments and the othernew Itemcalls passnull), so the list stays at five entries. If you would rather keep the scan as it is, naming these forms, along with positionalinsert(index, content),pushandunshift, in the comment keeps its coverage claim exact.
No changeset is needed: CONTRIBUTING.md says changes with no effect on people who use Lexical, such as tests, need none.
| if ( | ||
| ATTR_WRITES.has(method) || | ||
| (method === 'delete' && args !== null && args.length === 2) || | ||
| (method === 'format' && args !== null && args.length === 3) |
There was a problem hiding this comment.
Optional, non-blocking: in @y/y 14.0.0-rc.29, Y.Node#insert(index, content, format) writes formatting attributes through the same applyDelta path as format(index, length, formats), so a three-argument insert could sit beside this rule:
(method === 'insert' && args !== null && args.length === 3)It reports nothing new on this branch, since Cursor#insert(content, formats) takes two arguments. The other attribute write the scan does not see is a new Item(...) whose seventh argument (parentSub) is a key rather than null, which is how Seen.setAttr writes; if both stay out of scope, the comment above the case could name them as not covered.
zurfyx
added a commit
that referenced
this pull request
Oct 30, 2026
## Description Merges `main` at be5afea88816 (including #9550 and #9559). One conflict, in the link definitions of `packages/lexical-y/DECISIONS.md`, resolved by keeping both sides; the guard's accepted-limit row is renumbered D79, as `main` now uses D74 to D78. ## Test plan ### After See the next commit, which fixes the two tests the merge breaks, and the PR's test plan.
zurfyx
added a commit
that referenced
this pull request
Oct 30, 2026
…n; say plainly that the peer dependency enforces the exact version ## Description After merging `main` (#9550, #9559): - **#9550's Seen guard** (`Source.test.ts`, "internal writes of node attributes go through Seen") flagged `YjsContract.ts`: its probe writes to scratch documents of its own, never a document a binding reads, so there is no Seen record to keep. The file is excluded there, with that reason. - **#9559 uses `encodeStateVector`**, which the contract test listed as used but not in the contract: added to `YJS_EXPORTS` (checked as a function). And, on how tight the coupling is: `@y/y` exposes no version at run time (no export or constant; its duplicate-import flag `globalThis['__ $YJS14$ __']` is only `true`; its `package.json` subpath needs a JSON import that not every bundler or runtime supports, and none synchronously), so the exact version is the peer dependency's to enforce and the run-time check is a shape check. Measured: `14.0.0-rc.24` and earlier fail at build or load (no `Node` export: esbuild `No matching export ... for import "Node"`, native ES modules `SyntaxError`), and `rc.25`, `rc.26` and `rc.28` pass the probe. The changeset summary (it read as an exact-version check), `y.md` Install and Limits, the README and D79 now say so. ## Test plan ### Before On the merged tree, before this commit: ``` × internal writes of node attributes go through Seen + "YjsContract.ts probeMembers: root.setAttr('probe')", ... (6 entries) × lists exactly what the sources use of @y/y + "export encodeStateVector (internal/Binding.ts)", ``` ### After ``` $ pnpm exec vitest run --project unit packages/lexical-y/src/__tests__/unit/YjsContract.test.ts packages/lexical-y/src/__tests__/unit/Source.test.ts Tests 45 passed (45) ``` Per release candidate (the contract bundled with esbuild, `@y/y` external, run in Node against each install): ``` rc.24 load error: The requested module '@y/y' does not provide an export named 'Node' rc.25 [] rc.26 [] rc.28 [] rc.29 [] ```
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
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'ssetAttranddeleteAttr: while a remote change waits to be read, they write where the binding's editor sees the document. A rawy.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.envone. It parses each internal module exceptSeen.tswith the TypeScript AST and lists every use of aY.Nodemethod that writes attributes (setAttr,deleteAttr,clearAttrs,applyDelta; property or string-literal element access, called or only referenced as in abind, or destructured), and every call shaped likeY.Node'sdelete(index, length)orformat(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 throughdeleteItem) and a one-argumentY.Node#delete(index), which a syntactic scan cannot tell from aMaporSetdelete; aliases ofdeleteandformat; 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.tsof 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):After
On this branch as committed:
pnpm run ci-check:Found 0 errors.