Repository navigation
[lexical-code-core][lexical-code-prism][lexical-code-shiki] Bug Fix: The code highlighter keeps the NodeState of text inside a code block when it tokenizes the block again - #9832
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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.
|
…The code highlighter keeps the NodeState of text inside a code block when it tokenizes the block again
## Description
Text inside a code block can carry NodeState of its own: comment ids, a
suggestion's members, which collaboration stores on the text. When text with
NodeState enters a code block (a quote or paragraph merged into it, or a
collaborator's change read with `skipTransforms`), the highlighter keeps it
as it is until its next pass over the block, which any change that makes the
CodeNode dirty runs (an edit inside the block, or a node inserted next to
it). That pass tokenizes the block's text again and splices fresh
CodeHighlightNodes in, so the NodeState of the text was dropped: a comment or
suggestion was lost on text nobody edited. The highlighters already keep
NodeState the other way, when code highlight nodes leave a code block.
The new tokens now keep it (`$keepCodeNodeState` in @lexical/code-core, used
by the Prism and Shiki highlighters): a token is split where the NodeState of
the old children changes, and each part gets the NodeState of the node its
text came from. A state the old node knows by its StateConfig stays known on
the copy, here and where highlight nodes leave a code block: copied as JSON
only, a StateConfig not registered for the new node's class is read back as
an unknown value, which collaboration stores whole (a set-valued state as one
value instead of one entry per member). What the copy has of its own stays
as the copy of the JSON leaves it: a state the old node has at its default
value (which its JSON leaves out) is not copied over the value the copy's
class sets when it is made (a node replacement's own state), and a key the
copy's class has a StateConfig of its own for is read with that one, not
given the old node's StateConfig (which would put that StateConfig in the
place of the class's own for every node of the class, so that a later read
with the class's own throws a State key collision). The highlighters'
comparison of old and new children also compares NodeState
(`$isSameCodeNodeState`), so a token that gained or lost state is replaced.
A block with no NodeState (the common case, on every keystroke in code) is
tokenized as before: the helper returns the new tokens untouched after one
pass over the children.
## Changeset
`.changeset/code-retokenize-keeps-state.md`: a patch for `@lexical/code-core`,
`@lexical/code-prism` and `@lexical/code-shiki`.
## Test plan
`CodePrismRetokenizeKeepsState.test.ts` and
`CodeShikiRetokenizeKeepsState.test.ts` (one per highlighter):
- a text node with NodeState (a string and a set-valued state) inside a code
block, as a collaborator's change is read (no transforms), keeps both when
a paragraph inserted right after the block makes the highlighter tokenize
it again, in a highlight token of its own, known by its StateConfig;
- a quote merged into a code block keeps the NodeState of its text;
- a block with no NodeState is tokenized exactly as before;
- with a TextNode replacement, and with a code highlight node replacement,
that sets a state when it is made and has a StateConfig of its own for a
key: the text that takes a token's place when it leaves a code block, and
a new token, keep the state their class sets where the old node has that
state at its default value, and read a value the old node has under
another StateConfig for that key with their class's own (no State key
collision). On main, the two cells for text leaving a code block pass
(the copy there is JSON only) and the two for new tokens fail (the state
is dropped).
### Before
```
$ vitest --run --project unit CodePrismRetokenizeKeepsState CodeShikiRetokenizeKeepsState
× a text node with NodeState inside a code block, tokenized again by an insert next to the block (Prism, Shiki)
× a quote merged into a code block keeps the NodeState of its text (Prism, Shiki)
✓ without NodeState, the block is tokenized as before (Prism, Shiki)
✓ text leaving a code block keeps the state its class sets, where the token has that state at its default (Prism, Shiki)
✓ text leaving a code block is read with the StateConfig its class has for a key (Prism, Shiki)
× a new token keeps the state its class sets, where the text it comes from has that state at its default (Prism, Shiki)
× a new token is read with the StateConfig its class has for a key (Prism, Shiki)
AssertionError: expected [] to deeply equal [ true ]
AssertionError: expected 'const a = 1;t50z' to be 'const a = 1;t50z[members=s1]' // Object.is equality
AssertionError: expected [] to deeply equal [ [ true, 'kept', 'initialized' ] ]
AssertionError: expected [] to deeply equal [ [ true, 'kept' ] ]
Tests 8 failed | 6 passed (14)
```
### After
```
$ vitest --run --project unit CodePrismRetokenizeKeepsState CodeShikiRetokenizeKeepsState
Tests 14 passed (14)
```
The whole suite, the types and Flow:
```
$ LEXICAL_Y_GC=0 vitest run --project unit --project scripts-unit
Test Files 920 passed | 6 skipped (926)
Tests 37176 passed | 371 expected fail | 160 skipped (37707)
$ pnpm run tsc
$ pnpm run flow
Found 0 errors
```
Files that went over a time or memory bound while the machine was loaded
were run again with four workers, and are counted from that run.
Highlighting cost on every keystroke in a large block (Prism, a JavaScript
block of `const valueN = fetch(N);` lines, an edit of the number in the
middle line, median of five runs of 150 updates each, before -> after):
```
1,000 lines, no NodeState: 49.7 ms -> 50.1 ms
2,000 lines, no NodeState: 111.9 ms -> 110.7 ms
1,000 lines, 20 tokens with state: 49.3 ms -> 52.5 ms
2,000 lines, 40 tokens with state: 110.4 ms -> 118.2 ms
```
The tokens with state are the numbers of every 50th line. Before, they keep
their state too (main's comparison leaves a token whose text and type did
not change in place); after, each pass over a block with NodeState also
splits the new tokens where the state changes and compares the state of old
and new children, which is what the last two pay for.
1c5afe1 to
eaade84
Compare
There was a problem hiding this comment.
Thanks, this looks good. The fix is well scoped: the new path only runs when a code block's text carries NodeState, a block without state is tokenized exactly as before, and the new tests on both highlighters fail on main and pass here. I also checked the collaboration case with two peers on @lexical/yjs (v1 and v2) and @lexical/y: with this change both peers keep the state after one of them highlights the block, and their documents converge.
A few optional, non-blocking suggestions:
- Share
$copyNodeState(packages/lexical-code-core/src/FlatStructureUtils.ts:260). The same function now lives in@lexical/code-core,CodeHighlighterPrism.tsandCodeHighlighterShiki.ts, and this PR adds the same block to all three. Exporting the code-core one as@internal(asregisterCodeGutteralready is) and calling it from both highlighters would keep the copy into a code block and the copy out of one on a single rule. - Use a real set-valued state in the tests (
CodePrismRetokenizeKeepsState.test.ts:45,CodeShikiRetokenizeKeepsState.test.ts:49).membersStateis described as set-valued but has a hand-writtenparse;parse: arraySetValue(stringValue())makes it the kind of state@lexical/ystores one entry per member, which is what the description says the copy preserves. - Consider a two-peer test. The tests stand in for a collaborator's change with
skipTransforms: true. A test in the@lexical/yharness, whichRemoteReadAppEdits.test.tsalready runs with Prism highlighting, would pin the behavior users actually rely on. - Per-update cost with a highlight-node replacement that sets NodeState. When a
CodeHighlightNodereplacement sets a state on every token (likeCreatedHighlightNodein the tests), the early return in$keepCodeNodeStatenever fires, so every pass copies and compares state for every token even though it already matches. In a quick measurement (Prism, a 1,000-line block) that took an update from about 103-109 ms to 138-143 ms; without such a replacement the cost was unchanged, matching your numbers. Skipping the copy where the old node's state already equals the new token's is one way to cut it, if that configuration matters to you.
| * Give `to` the NodeState of `from`, flat states included, keeping the state | ||
| * that `to` has for the keys `from` does not set. | ||
| */ | ||
| function $copyNodeState(from: LexicalNode, to: LexicalNode): void { |
There was a problem hiding this comment.
Optional: this is now the third copy of $copyNodeState (the others are in CodeHighlighterPrism.ts and CodeHighlighterShiki.ts), and this PR adds the same known-state block to each. Exporting this one as @internal (as registerCodeGutter is) and calling it from both highlighters would keep the copy into a code block and the copy out of one on a single rule.
| if (!prevNodes.some(hasNodeState)) { | ||
| return nextNodes; | ||
| } |
There was a problem hiding this comment.
Optional: with a CodeHighlightNode replacement that sets a NodeState on every new token (like CreatedHighlightNode in the tests), this early return never fires, so every pass copies and compares state for every token. In a quick Prism measurement on a 1,000-line block that took an update from about 103-109 ms to 138-143 ms; without such a replacement the cost was unchanged. Skipping the copy where the old node's state already equals the new token's is one way to cut it, if that configuration matters.
| const membersState = createState('members', { | ||
| parse: (value: unknown) => | ||
| Array.isArray(value) ? (value as string[]) : ([] as string[]), | ||
| }); |
There was a problem hiding this comment.
Nit: this is described as a set-valued state, but with a hand-written parse it is not one to @lexical/y (which stores a state one entry per member only when its schema is a set). parse: arraySetValue(stringValue()) here and in the Shiki test would exercise the member-wise case the PR description talks about.
Description
Text inside a code block can carry NodeState of its own: comment ids, a
suggestion's members, which collaboration stores on the text. When text with
NodeState enters a code block (a quote or paragraph merged into it, or a
collaborator's change read with
skipTransforms), the highlighter keeps itas it is until its next pass over the block, which any change that makes the
CodeNode dirty runs (an edit inside the block, or a node inserted next to
it). That pass tokenizes the block's text again and splices fresh
CodeHighlightNodes in, so the NodeState of the text was dropped: a comment or
suggestion was lost on text nobody edited. The highlighters already keep
NodeState the other way, when code highlight nodes leave a code block.
The new tokens now keep it (
$keepCodeNodeStatein @lexical/code-core, usedby the Prism and Shiki highlighters): a token is split where the NodeState of
the old children changes, and each part gets the NodeState of the node its
text came from. A state the old node knows by its StateConfig stays known on
the copy, here and where highlight nodes leave a code block: copied as JSON
only, a StateConfig not registered for the new node's class is read back as
an unknown value, which collaboration stores whole (a set-valued state as one
value instead of one entry per member). What the copy has of its own stays
as the copy of the JSON leaves it: a state the old node has at its default
value (which its JSON leaves out) is not copied over the value the copy's
class sets when it is made (a node replacement's own state), and a key the
copy's class has a StateConfig of its own for is read with that one, not
given the old node's StateConfig (which would put that StateConfig in the
place of the class's own for every node of the class, so that a later read
with the class's own throws a State key collision). The highlighters'
comparison of old and new children also compares NodeState
(
$isSameCodeNodeState), so a token that gained or lost state is replaced.A block with no NodeState (the common case, on every keystroke in code) is
tokenized as before: the helper returns the new tokens untouched after one
pass over the children.
Changeset
.changeset/code-retokenize-keeps-state.md: a patch for@lexical/code-core,@lexical/code-prismand@lexical/code-shiki.Test plan
CodePrismRetokenizeKeepsState.test.tsandCodeShikiRetokenizeKeepsState.test.ts(one per highlighter):block, as a collaborator's change is read (no transforms), keeps both when
a paragraph inserted right after the block makes the highlighter tokenize
it again, in a highlight token of its own, known by its StateConfig;
that sets a state when it is made and has a StateConfig of its own for a
key: the text that takes a token's place when it leaves a code block, and
a new token, keep the state their class sets where the old node has that
state at its default value, and read a value the old node has under
another StateConfig for that key with their class's own (no State key
collision). On main, the two cells for text leaving a code block pass
(the copy there is JSON only) and the two for new tokens fail (the state
is dropped).
Before
After
The whole suite, the types and Flow:
Files that went over a time or memory bound while the machine was loaded
were run again with four workers, and are counted from that run.
Highlighting cost on every keystroke in a large block (Prism, a JavaScript
block of
const valueN = fetch(N);lines, an edit of the number in themiddle line, median of five runs of 150 updates each, before -> after):
The tokens with state are the numbers of every 50th line. Before, they keep
their state too (main's comparison leaves a token whose text and type did
not change in place); after, each pass over a block with NodeState also
splits the new tokens where the state changes and compares the state of old
and new children, which is what the last two pay for.