Skip to content

Commit 429807e

Browse files
appflowyclaude
andauthored
fix: clamp stale selection offsets in withYjs to prevent toDOMPoint crash (#349)
When a peer shortens (or replaces) the text node under the local cursor via a null-origin yjs transaction (Y.applyUpdate without origin — used by row prefetch, version reset/revert paths), withYjs#applyRemoteEvents restores the cached selection without re-validating its offsets against the new tree. The path stays reachable, the offset overshoots the new text length, and slate-react's render-time selection sync crashes in toDOMPoint with "Cannot resolve a DOM point from Slate point". Validate via the existing offset-aware `isValidSelection`, clamp via `findNearestValidSelection`, and deselect as a last resort. Apply the same offset check to `initializeDocumentContent` so the reconnect path doesn't re-introduce the same precondition. Add a regression test that fails on the pre-fix code (3 scenarios: shrink past cursor, shrink under cursor at the reported offset, sanity-check that appends remain valid). Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 783cc34 commit 429807e

2 files changed

Lines changed: 249 additions & 6 deletions

File tree

Lines changed: 217 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,217 @@
1+
/**
2+
* Regression tests for the "Cannot resolve a DOM point from Slate point" crash.
3+
*
4+
* Background
5+
* ----------
6+
* withYjs#applyRemoteEvents caches editor.selection BEFORE applying remote
7+
* events to editor.children, then restores it after the events are applied.
8+
* For transactions with `origin === null` (e.g. bare `Y.applyUpdate(doc, state)`
9+
* calls in row prefetch, version reset, version revert, etc.) this overwrite
10+
* is unconditional — the cached selection is written back without re-validating
11+
* its offsets against the new text-node lengths. When a remote update shortens
12+
* the text under the local cursor, the restored selection keeps a path that
13+
* still exists but an offset past the end of the new text. Slate-react's
14+
* render-time selection sync then calls ReactEditor.toDOMPoint, which finds
15+
* the DOM text shorter than the requested offset and throws.
16+
*
17+
* These tests assert the DESIRED post-fix behavior:
18+
* • After a remote shrink, the selection is either cleared OR clamped so
19+
* that `isValidSelection` returns true (offset within new text length).
20+
* • A remote append leaves a valid selection unchanged.
21+
*
22+
* Until withYjs#applyRemoteEvents and #initializeDocumentContent are updated
23+
* to validate offsets (e.g. via `isValidSelection` + clamp/deselect), the two
24+
* shrink tests are expected to FAIL — that's how they protect against the
25+
* regression.
26+
*/
27+
28+
import { expect } from '@jest/globals';
29+
import { createEditor, Editor, Range, Transforms } from 'slate';
30+
import * as Y from 'yjs';
31+
32+
import { yDocToSlateContent } from '@/application/slate-yjs/utils/convert';
33+
import { isValidSelection } from '@/application/slate-yjs/utils/transformSelection';
34+
import { YjsEditorKey, YTextMap } from '@/application/types';
35+
36+
import {
37+
generateId,
38+
getTestingDocData,
39+
insertBlock,
40+
withTestingYDoc,
41+
withTestingYjsEditor,
42+
} from './withTestingYjsEditor';
43+
44+
jest.mock('nanoid');
45+
// __mocks__/lodash-es.ts and __mocks__/lodash-es/isEqual.ts are auto-applied
46+
// for npm modules. They provide a partial surface that breaks translateYEvents
47+
// here (auto-mocked `omit` is undefined; auto-mocked `isEqual` always returns
48+
// true so text events get routed into the block-update branch). Use the real
49+
// implementations so this test exercises the real applyRemoteEvents flow.
50+
jest.mock('lodash-es', () => jest.requireActual('lodash'));
51+
jest.mock('lodash-es/isEqual', () => jest.requireActual('lodash/isEqual'));
52+
53+
/** Find the slate path of the first text leaf inside the first paragraph. */
54+
function findFirstLeafPath(editor: Editor): number[] {
55+
for (const [node, path] of Editor.nodes(editor, { at: [], match: (n) => 'text' in n })) {
56+
if (typeof (node as { text: string }).text === 'string') {
57+
return path;
58+
}
59+
}
60+
61+
throw new Error('No text leaf found in editor.children');
62+
}
63+
64+
/**
65+
* Mutate `remoteDoc` (which mirrors `localDoc`) and ship the diff to
66+
* `localDoc` with origin=Remote, so withYjs treats it as a remote event.
67+
*/
68+
function shipRemoteUpdate(
69+
remoteDoc: Y.Doc,
70+
localDoc: Y.Doc,
71+
mutate: (textMap: YTextMap) => void
72+
) {
73+
const before = Y.encodeStateVector(localDoc);
74+
const { meta } = getTestingDocData(remoteDoc);
75+
const textMap = meta.get(YjsEditorKey.text_map) as YTextMap;
76+
77+
mutate(textMap);
78+
79+
const diff = Y.encodeStateAsUpdateV2(remoteDoc, before);
80+
81+
// Use origin=null to exercise withYjs#applyRemoteEvents's "restore cached
82+
// selection" branch (the buggy one). Other origins skip that branch and
83+
// rely on Slate's per-operation selection transforms, which clamp offsets
84+
// automatically and hide the bug.
85+
Y.applyUpdateV2(localDoc, diff);
86+
}
87+
88+
function buildSyncedPair(blockId: string, initialText: string) {
89+
const pageId = generateId();
90+
const remoteDoc = withTestingYDoc(pageId);
91+
const remoteBlock = insertBlock({
92+
doc: remoteDoc,
93+
blockObject: {
94+
id: blockId,
95+
ty: 'paragraph',
96+
relation_id: blockId,
97+
text_id: blockId,
98+
data: '{}',
99+
},
100+
});
101+
102+
remoteBlock.applyDelta([{ insert: initialText }]);
103+
104+
const localDoc = new Y.Doc();
105+
106+
Y.applyUpdateV2(localDoc, Y.encodeStateAsUpdateV2(remoteDoc));
107+
108+
return { remoteDoc, localDoc };
109+
}
110+
111+
describe('withYjs selection restoration after remote events', () => {
112+
it('keeps selection valid when remote shrink occurs under cursor', () => {
113+
const blockId = generateId();
114+
const { remoteDoc, localDoc } = buildSyncedPair(blockId, 'Hello World');
115+
116+
const editor = withTestingYjsEditor(createEditor(), localDoc);
117+
118+
editor.connect();
119+
120+
// Sanity-check the slate tree mirrors the yjs tree.
121+
expect(editor.children).toEqual(yDocToSlateContent(localDoc)?.children ?? []);
122+
123+
// Place the local "cursor" at the end of "Hello World" (offset 11).
124+
const leafPath = findFirstLeafPath(editor);
125+
126+
expect(Editor.string(editor, leafPath)).toBe('Hello World');
127+
128+
Transforms.select(editor, { path: leafPath, offset: 11 });
129+
130+
expect(editor.selection).not.toBeNull();
131+
expect(Range.isCollapsed(editor.selection!)).toBe(true);
132+
expect(editor.selection!.anchor.offset).toBe(11);
133+
expect(isValidSelection(editor, editor.selection!)).toBe(true);
134+
135+
// Collaborator shrinks the same text to "Hi".
136+
shipRemoteUpdate(remoteDoc, localDoc, (textMap) => {
137+
const text = textMap.get(blockId);
138+
139+
expect(text).toBeDefined();
140+
expect(text!.toString()).toBe('Hello World');
141+
142+
text!.delete(0, text!.length);
143+
text!.insert(0, 'Hi');
144+
});
145+
146+
// Editor tree reflects "Hi" (length 2).
147+
expect(Editor.string(editor, leafPath)).toBe('Hi');
148+
149+
// Post-fix expectation: the selection must NOT be left pointing past the
150+
// end of the new text. Either it is cleared, or its offset is clamped to
151+
// within the new text length. Anything else is the crash precondition
152+
// that fires "Cannot resolve a DOM point from Slate point" on next render.
153+
if (editor.selection !== null) {
154+
expect(isValidSelection(editor, editor.selection)).toBe(true);
155+
expect(editor.selection.anchor.offset).toBeLessThanOrEqual(2);
156+
expect(editor.selection.focus.offset).toBeLessThanOrEqual(2);
157+
}
158+
});
159+
160+
it('keeps selection valid when remote trims to before the cursor offset', () => {
161+
const blockId = generateId();
162+
const { remoteDoc, localDoc } = buildSyncedPair(blockId, 'The quick brown fox');
163+
164+
const editor = withTestingYjsEditor(createEditor(), localDoc);
165+
166+
editor.connect();
167+
const leafPath = findFirstLeafPath(editor);
168+
169+
// Cursor at offset 16 — the offending offset from the original error message.
170+
Transforms.select(editor, { path: leafPath, offset: 16 });
171+
expect(isValidSelection(editor, editor.selection!)).toBe(true);
172+
173+
shipRemoteUpdate(remoteDoc, localDoc, (textMap) => {
174+
const text = textMap.get(blockId)!;
175+
176+
// Trim to "The quick" (length 9), well under offset 16.
177+
text.delete(9, text.length - 9);
178+
});
179+
180+
expect(Editor.string(editor, leafPath)).toBe('The quick');
181+
182+
// Post-fix: offset 16 is past the new length 9, so the selection must
183+
// be cleared or clamped (offset <= 9) — never left at 16.
184+
if (editor.selection !== null) {
185+
expect(isValidSelection(editor, editor.selection)).toBe(true);
186+
expect(editor.selection.anchor.offset).toBeLessThanOrEqual(9);
187+
expect(editor.selection.focus.offset).toBeLessThanOrEqual(9);
188+
}
189+
});
190+
191+
it('sanity: remote append leaves a valid selection unchanged', () => {
192+
// If a remote update only LENGTHENS text under the cursor, the cached
193+
// offset stays in-bounds and selection remains valid. This proves the bug
194+
// is specifically offset > new text length, not "any remote event
195+
// invalidates selection".
196+
const blockId = generateId();
197+
const { remoteDoc, localDoc } = buildSyncedPair(blockId, 'Hello');
198+
199+
const editor = withTestingYjsEditor(createEditor(), localDoc);
200+
201+
editor.connect();
202+
const leafPath = findFirstLeafPath(editor);
203+
204+
Transforms.select(editor, { path: leafPath, offset: 5 });
205+
expect(isValidSelection(editor, editor.selection!)).toBe(true);
206+
207+
shipRemoteUpdate(remoteDoc, localDoc, (textMap) => {
208+
const text = textMap.get(blockId)!;
209+
210+
text.insert(text.length, ' World');
211+
});
212+
213+
expect(Editor.string(editor, leafPath)).toBe('Hello World');
214+
expect(editor.selection!.anchor.offset).toBe(5);
215+
expect(isValidSelection(editor, editor.selection!)).toBe(true);
216+
});
217+
});

src/application/slate-yjs/plugins/withYjs.ts

Lines changed: 32 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,10 @@
11
import { BaseRange, Descendant, Editor, Operation, Transforms } from 'slate';
2-
import { ReactEditor } from 'slate-react';
32
import Y, { Transaction, YEvent } from 'yjs';
43

54
import { translateYEvents } from '@/application/slate-yjs/utils/applyToSlate';
65
import { applyToYjs } from '@/application/slate-yjs/utils/applyToYjs';
76
import { yDocToSlateContent } from '@/application/slate-yjs/utils/convert';
7+
import { findNearestValidSelection, isValidSelection } from '@/application/slate-yjs/utils/transformSelection';
88
import { CollabOrigin, YjsEditorKey, YSharedRoot } from '@/application/types';
99

1010
type LocalChange = {
@@ -116,11 +116,16 @@ export function withYjs<T extends Editor>(
116116
e.children = content.children;
117117
}
118118

119-
if (selection && !ReactEditor.hasRange(editor, selection)) {
119+
// `ReactEditor.hasRange` only checks path existence, not whether each
120+
// offset is within its node's text length. After a content swap the path
121+
// can still resolve while the offset overshoots the new text — that's the
122+
// "Cannot resolve a DOM point from Slate point" precondition. Use the
123+
// offset-aware validator instead.
124+
if (selection && !isValidSelection(e, selection)) {
120125
try {
121126
Transforms.select(e, Editor.start(editor, [0]));
122-
} catch (e) {
123-
console.error(e);
127+
} catch (err) {
128+
console.error(err);
124129
editor.deselect();
125130
}
126131
}
@@ -155,13 +160,34 @@ export function withYjs<T extends Editor>(
155160
translateYEvents(e, events);
156161
}
157162

158-
// Update my cursor position
163+
// Restore the cached cursor. The cached selection's paths may still be
164+
// reachable in the new tree while its offsets overshoot the new text
165+
// length (e.g. a peer shortened the text under the cursor). Applying it
166+
// unchanged would leave editor.selection at offset > text length, which
167+
// crashes slate-react's render-time selection sync (`toDOMPoint`). Clamp
168+
// via `findNearestValidSelection`, falling back to deselect if no point
169+
// can be recovered.
159170
try {
160171
if (newSelection) {
161-
Transforms.select(editor, newSelection);
172+
if (isValidSelection(editor, newSelection)) {
173+
Transforms.select(editor, newSelection);
174+
} else {
175+
const clamped = findNearestValidSelection(editor, newSelection);
176+
177+
if (clamped) {
178+
Transforms.select(editor, clamped);
179+
} else {
180+
editor.deselect();
181+
}
182+
}
162183
}
163184
} catch (error) {
164185
console.error(error);
186+
try {
187+
editor.deselect();
188+
} catch {
189+
// Selection may already be null.
190+
}
165191
}
166192

167193
// Restore the apply function to store local changes after applying remote changes

0 commit comments

Comments
 (0)