diff --git a/entry_types/scrolled/package/spec/frontend/inlineEditing/EditableText/useCommentRangeRefs-spec.js b/entry_types/scrolled/package/spec/frontend/inlineEditing/EditableText/useCommentRangeRefs-spec.js index a645dc5dc4..cb71ba0e87 100644 --- a/entry_types/scrolled/package/spec/frontend/inlineEditing/EditableText/useCommentRangeRefs-spec.js +++ b/entry_types/scrolled/package/spec/frontend/inlineEditing/EditableText/useCommentRangeRefs-spec.js @@ -57,6 +57,33 @@ describe('useCommentRangeRefs', () => { }); }); + it('preserves the live rangeRef across threads array updates', () => { + const range = { + anchor: {path: [0, 0], offset: 6}, + focus: {path: [0, 0], offset: 11} + }; + + const {editor, result, rerender} = setup([{id: 1, subjectRange: range}]); + + // Live edit ahead of any debounced save: rangeRef shifts forward. + act(() => { + Transforms.insertText(editor, 'X', {at: {path: [0, 0], offset: 0}}); + }); + + // The reviewSession state provider produces a new threads array + // each time anything in the session updates — including the + // round-trip echo of a debounced save, which arrives with the + // *saved* `subjectRange` (i.e., behind any keystrokes that + // landed during the network round-trip). Recreating the rangeRef + // here would snap the highlight back to that stale range; the + // live tracking must survive. + rerender({threads: [{id: 1, subjectRange: range}]}); + + expect(result.current.getTrackedSubjectRanges()).toEqual({ + 1: {anchor: {path: [0, 0], offset: 7}, focus: {path: [0, 0], offset: 12}} + }); + }); + it('includes all tracked threads regardless of whether their range moved', () => { const unchangedRange = {anchor: {path: [0, 0], offset: 0}, focus: {path: [0, 0], offset: 2}}; @@ -129,5 +156,65 @@ describe('useCommentRangeRefs', () => { focus: {path: [0, 0], offset: 6} }); }); + + it('tracks edits for a thread whose migrated range targets a path not in the previous value', () => { + const editor = withReact(createEditor()); + editor.children = [ + {type: 'paragraph', children: [{text: 'hello'}]} + ]; + + const initialThreads = []; + + const {result, rerender} = renderHook( + ({threads}) => useCommentRangeRefs(editor, threads), + {initialProps: {threads: initialThreads}} + ); + + // A delete-merge brings in a thread that previously lived on the + // removed neighbour: the migrated range targets path [1, 0], + // which doesn't exist in the previous value — mirroring the + // production order where `applyThreadUpdates` lands before the + // value flip, so threads update against the still-old content. + const migratedThreads = [{ + id: 1, + subjectRange: {anchor: {path: [1, 0], offset: 0}, + focus: {path: [1, 0], offset: 5}} + }]; + + rerender({threads: migratedThreads}); + + // Order matters: in production, `onReset` (and thus + // `resetRangeRefs`) runs from `useCachedValue`'s effect *before* + // `setCachedValue` triggers the next render in which Slate's + // `` useMemo replaces `editor.children` with V_NEW. Reset + // first, *then* flip the content — otherwise this test would + // also pass for an implementation that synced immediately from + // inside `resetRangeRefs`, because the sync would run against + // the already-flipped content and find the migrated thread + // valid. + act(() => { + result.current.resetRangeRefs(); + editor.children = [ + {type: 'paragraph', children: [{text: 'hello'}]}, + {type: 'paragraph', children: [{text: 'world'}]} + ]; + }); + + // Stand in for the `setCachedValue`-driven re-render that would + // fire in production right after `onReset` returns; the + // deferred-sync effect consumes its armed ref here. + rerender({threads: migratedThreads}); + + act(() => { + Transforms.insertText(editor, 'x', { + at: {path: [1, 0], offset: 0} + }); + }); + + expect(result.current.getTrackedSubjectRanges()).toEqual({ + 1: {anchor: {path: [1, 0], offset: 1}, + focus: {path: [1, 0], offset: 6}} + }); + }); }); }); diff --git a/entry_types/scrolled/package/src/frontend/inlineEditing/EditableText/useCommentRangeRefs.js b/entry_types/scrolled/package/src/frontend/inlineEditing/EditableText/useCommentRangeRefs.js index a59f059f84..03434307da 100644 --- a/entry_types/scrolled/package/src/frontend/inlineEditing/EditableText/useCommentRangeRefs.js +++ b/entry_types/scrolled/package/src/frontend/inlineEditing/EditableText/useCommentRangeRefs.js @@ -5,11 +5,19 @@ import {Editor, Node} from 'slate'; // text edits stay reflected in the thread's effective subject range // without round-tripping through the server. When the upstream value // is replaced (e.g. after a structural shift applied by the editor), -// `useCachedValue` invokes `resetRangeRefs` from `onReset` so the -// next render falls back to the upstream `subjectRange` and the -// post-render effect repopulates the map against the new content. +// `EditableText` calls `resetRangeRefs` from `useCachedValue`'s +// `onReset`, which fires from an effect *before* `setCachedValue` +// triggers the re-render in which Slate's `` useMemo replaces +// `editor.children` with the new value. So `resetRangeRefs` only +// drops the stale refs and arms `pendingResyncRef`; the second effect +// below consumes that flag on the next render — by which point +// `editor.children` has flipped — and rebuilds against the new +// content. Rebuilding inside `resetRangeRefs` would sync against the +// still-old content and skip migrated threads whose new path doesn't +// yet exist there. export function useCommentRangeRefs(editor, threads) { const rangeRefsMap = useRef(new Map()); + const pendingResyncRef = useRef(false); const syncRangeRefs = useCallback((threads) => { const map = rangeRefsMap.current; @@ -30,22 +38,32 @@ export function useCommentRangeRefs(editor, threads) { }, [editor]); useEffect(() => { - const map = rangeRefsMap.current; syncRangeRefs(threads); + }, [threads, syncRangeRefs]); + useEffect(() => { + const map = rangeRefsMap.current; return () => { for (const rangeRef of map.values()) { rangeRef.unref(); } map.clear(); }; - }, [threads, syncRangeRefs]); + }, [editor]); + + useEffect(() => { + if (pendingResyncRef.current) { + pendingResyncRef.current = false; + syncRangeRefs(threads); + } + }); const resetRangeRefs = useCallback(() => { for (const rangeRef of rangeRefsMap.current.values()) { rangeRef.unref(); } rangeRefsMap.current.clear(); + pendingResyncRef.current = true; }, []); const trackedThreads = threads.map(t => {