Merge pull request #2411 from tf/batch-fixes

Fix batch editing regressions

Author: tf

entry_types/scrolled/package/src/editor/models/ScrolledEntry/Batch.js

@@ -314,18 +314,28 @@ export function Batch(entry, section, {reviewSession} = {}) {
       },
 
       success(response) {
-        applyAdditions(response);
-        applyPositions();
-        applyDeletions();
+        // Each step's intermediate state is observable in the iframe
+        // (Backbone change events propagate as separate postMessages
+        // and re-render React without batching), so the order
+        // resolves these dependencies:
+        //
+        // - permaIds before thread updates: thread migrations
+        //   resolve `target.get('permaId')`.
+        // - Thread updates before configuration changes: the
+        //   value-flip render in `useCachedValue` clears rangeRefs
+        //   and falls back to `thread.subjectRange`, which must
+        //   already be migrated.
+        // - Positions before additions: Backbone's collection
+        //   comparator drops new elements into their sorted slot.
+        // - Configuration changes before deletions: a delete-merge
+        //   survivor grows before its sibling disappears (no
+        //   brief collapse).
+        applyPermaIds(response);
         applyThreadUpdates();
-        // Apply configuration changes last so that the value flip
-        // observed by `useCachedValue` happens after `reviewSession`
-        // already holds the migrated thread ranges. `onReset` then
-        // clears `useCommentRangeRefs`'s map and the post-render
-        // effect rebuilds it from the new thread subject ranges
-        // against the new editor content — no reliance on
-        // `isValidRange` filtering out stale OLD ranges.
         applyConfigurationChanges();
+        applyPositions();
+        applyAdditions();
+        applyDeletions();
 
         section.contentElements.sort();
 
@@ -384,20 +394,26 @@ export function Batch(entry, section, {reviewSession} = {}) {
   // Functionality to apply the recorded changes to the underlying
   // section once the request succeeded:
 
-  function applyAdditions(response) {
+  function applyPermaIds(response) {
     contentElements.forEach((contentElement, index) => {
       if (contentElement.isNew()) {
-        section.contentElements.add(contentElement);
-
         contentElement.set({
           id: response[index].id,
           permaId: response[index].permaId
         });
       }
-      else if (contentElement.section !== section) {
+    });
+  }
+
+  function applyAdditions() {
+    contentElements.forEach(contentElement => {
+      if (contentElement.section && contentElement.section !== section) {
         contentElement.section.contentElements.remove(contentElement);
         section.contentElements.add(contentElement);
       }
+      else if (!section.contentElements.contains(contentElement)) {
+        section.contentElements.add(contentElement);
+      }
     });
   }
 

entry_types/scrolled/package/src/frontend/inlineEditing/EditableText/BadgeColumn.js

@@ -1,7 +1,7 @@
 import React from 'react';
 
 import {FloatingPortal} from '@floating-ui/react';
-import {useSlate} from 'slate-react';
+import {useSlate, ReactEditor} from 'slate-react';
 
 import {Badge, useAnchoredFloating} from 'pageflow-scrolled/review';
 import {useFloatingPortalRoot} from '../../FloatingPortalRootProvider';
@@ -11,11 +11,17 @@ import {highlightOverlapsSelection} from './highlightOverlapsSelection';
 
 export function BadgeColumn({highlights, anchors}) {
   const editor = useSlate();
+  // Treat `editor.selection` as a live cursor only while the editor
+  // is focused. After the user clicks away, slate-react's throttled
+  // `selectionchange` listener can sync a clamped DOM cursor back
+  // into `editor.selection`, which would otherwise flip badges back
+  // to overlap mode without any actual selection.
+  const editorSelection = ReactEditor.isFocused(editor) ? editor.selection : null;
 
   return highlights.map(highlight => (
     <PositionedBadge key={highlight.key}
                      highlight={highlight}
-                     editorSelection={editor.selection}
+                     editorSelection={editorSelection}
                      anchors={anchors} />
   ));
 }

entry_types/scrolled/package/src/frontend/inlineEditing/EditableText/Selection.js

@@ -78,6 +78,17 @@ export function Selection(props) {
     }
 
     if (!isContentElementSelected && !boundsRef.current) {
+      // Only treat the selection as user-initiated when the editor is
+      // actually focused. After an external value replacement, the
+      // browser may clamp its DOM cursor to the start of the
+      // shrunken contenteditable; slate-react's throttled
+      // `selectionchange` listener then re-syncs that cursor into
+      // `editor.selection`. Without this guard, that synthetic
+      // selection would re-select the content element here.
+      if (!ReactEditor.isFocused(editor)) {
+        return;
+      }
+
       select();
     }
 

entry_types/scrolled/package/src/frontend/inlineEditing/EditableText/index.js

@@ -221,6 +221,19 @@ function resetSelectionIfOutsideNextValue(editor, nextValue) {
   if (editor.selection && (!hasTextAtPoint(nextEditor, editor.selection.anchor) ||
                            !hasTextAtPoint(nextEditor, editor.selection.focus))) {
     Transforms.deselect(editor);
+    // Also drop the browser's DOM cursor when it sits inside this
+    // editor's contenteditable. Otherwise slate-react's throttled
+    // `selectionchange` listener would later re-sync that stale
+    // cursor — clamped to the start of the now-shrunken content —
+    // back into `editor.selection`, and `Selection`'s auto-select
+    // would treat it as a user click and re-select this content
+    // element (overriding whatever the calling op selected, e.g. a
+    // freshly inserted image).
+    const domSelection = window.getSelection();
+    const editorEl = ReactEditor.toDOMNode(editor, editor);
+    if (domSelection && editorEl.contains(domSelection.anchorNode)) {
+      domSelection.removeAllRanges();
+    }
   }
 }
 

entry_types/scrolled/package/src/frontend/inlineEditing/useCachedValue.js

@@ -7,26 +7,13 @@ export function useCachedValue(value, {
   const [cachedValue, setCachedValue] = useState(value || defaultValue);
   const previousValue = useRef(value);
 
-  // Adjust state during render: when the upstream value changes,
-  // call `onReset` and flip `cachedValue` immediately so the first
-  // committed render after the change already observes the new
-  // value. React detects the in-render setState and re-runs the
-  // component before commit, so no extra render lands on the
-  // screen — and `onReset` side effects are visible to the same
-  // render that observes the new `cachedValue`.
-  if (previousValue.current !== value && value !== cachedValue) {
-    onReset && onReset(value);
-    setCachedValue(value);
-    previousValue.current = value;
-  }
+  useEffect(() => {
+    if (previousValue.current !== value && value !== cachedValue) {
+      onReset && onReset(value);
+      setCachedValue(value);
+    }
+  }, [onReset, value, cachedValue]);
 
-  // Track the upstream value across renders. The branch above only
-  // assigns when an external change is observed; without this effect,
-  // a render that sees `value` already in sync with `cachedValue` (a
-  // local edit echoed back via `onDebouncedChange`) would leave
-  // `previousValue.current` stale. The next local edit would then
-  // misread that stale value as an external change and overwrite the
-  // edit with `onReset`.
   useEffect(() => {
     previousValue.current = value;
   });