Fix edge cases in inline comment range ref tracking

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
+      // `<Slate>` 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}}
+      });
+    });
   });
 });

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 `<Slate>` 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 => {