SD-3386 - fix: deletion handling across paragraph boundaries

packages/super-editor/src/editors/v1/extensions/track-changes/track-changes-overlap.test.js

@@ -19,6 +19,7 @@ import { beforeEach, afterEach, describe, expect, it } from 'vitest';
 import { TextSelection } from 'prosemirror-state';
 import { TrackInsertMarkName, TrackDeleteMarkName } from './constants.js';
 import { initTestEditor } from '@tests/helpers/helpers.js';
+import { handleBackspace, handleDelete } from '@core/extensions/keymap.js';
 import { buildReviewGraph, CanonicalChangeType } from './review-model/review-graph.js';
 
 const ALICE = { name: 'Alice', email: 'alice@example.com' };
@@ -215,3 +216,149 @@ describe('overlap wired: insertTrackedChange delegates to compiler', () => {
     expect(ok).toBe(false);
   });
 });
+
+describe('overlap wired: deletion spanning a paragraph boundary (SD-3386)', () => {
+  let editor;
+  afterEach(() => {
+    editor?.destroy();
+    editor = null;
+  });
+
+  const TWO_PARAGRAPHS = '<p>First line of text</p><p>Second line of text</p>';
+
+  // Selection from the start of paragraph 1 (pos 1) through the middle of
+  // paragraph 2 ("Second|" → 21 + 6 = 27). deleteSelection routes through
+  // deleteRange, which emits a ReplaceStep whose slice is a structural
+  // paragraph re-join shell with no inline content.
+  const deleteAcrossBoundary = () => {
+    editor.commands.command(({ tr, dispatch }) => {
+      tr.setSelection(TextSelection.create(tr.doc, 1, 27));
+      tr.deleteSelection();
+      if (dispatch) dispatch(tr);
+      return true;
+    });
+  };
+
+  const paragraphTexts = (doc) => {
+    const texts = [];
+    doc.forEach((node) => texts.push(node.textContent));
+    return texts;
+  };
+
+  it('marks the range deleted without splitting the trailing paragraph', () => {
+    editor = setup(ALICE, TWO_PARAGRAPHS);
+    deleteAcrossBoundary();
+
+    // No spurious paragraph split: still exactly two paragraphs with the
+    // original text (the deletion is only marked, not applied).
+    expect(editor.state.doc.childCount).toBe(2);
+    expect(paragraphTexts(editor.state.doc)).toEqual(['First line of text', 'Second line of text']);
+
+    // One logical deletion — not a replacement wrapping an empty block shell.
+    const graph = graphFor(editor);
+    expect(graph.changes.size).toBe(1);
+    const change = Array.from(graph.changes.values())[0];
+    expect(change.type).toBe(CanonicalChangeType.Deletion);
+  });
+
+  it('accept removes the marked content and all tracked marks', () => {
+    editor = setup(ALICE, TWO_PARAGRAPHS);
+    deleteAcrossBoundary();
+
+    const ok = editor.commands.acceptTrackedChangesBetween(0, editor.state.doc.content.size);
+    expect(ok).toBe(true);
+    expect(editor.storage.trackChanges?.lastDecisionFailure ?? null).toBeNull();
+
+    // Deleted content is gone, remainder of paragraph 2 survives.
+    expect(paragraphTexts(editor.state.doc)).toEqual(['', 'ond line of text']);
+
+    // No tracked-change state (marks/decorations) survives the decision.
+    const graph = graphFor(editor);
+    expect(graph.changes.size).toBe(0);
+  });
+
+  it('reject restores the content and removes all tracked marks', () => {
+    editor = setup(ALICE, TWO_PARAGRAPHS);
+    deleteAcrossBoundary();
+
+    const graph = graphFor(editor);
+    const changeId = Array.from(graph.changes.keys())[0];
+    const ok = editor.commands.rejectTrackedChangeById(changeId);
+    expect(ok).toBe(true);
+    expect(editor.storage.trackChanges?.lastDecisionFailure ?? null).toBeNull();
+
+    // Original two-paragraph content is intact and unmarked.
+    expect(editor.state.doc.childCount).toBe(2);
+    expect(paragraphTexts(editor.state.doc)).toEqual(['First line of text', 'Second line of text']);
+    expect(graphFor(editor).changes.size).toBe(0);
+
+    let hasTrackedMarks = false;
+    editor.state.doc.descendants((node) => {
+      if (node.marks.some((m) => m.type.name === TrackInsertMarkName || m.type.name === TrackDeleteMarkName)) {
+        hasTrackedMarks = true;
+      }
+    });
+    expect(hasTrackedMarks).toBe(false);
+  });
+});
+
+describe('overlap wired: cross-paragraph deletion through the real keymap path (SD-3386)', () => {
+  let editor;
+  afterEach(() => {
+    editor?.destroy();
+    editor = null;
+  });
+
+  // The keymap path differs from a raw command dispatch: handleBackspace tags
+  // the transaction with inputType 'deleteContentBackward' and deleteSelection
+  // sets the post-step selection, which can land inside the structural shell
+  // slice that the tracked compile never inserts. Mapping that position back
+  // through a falsely-mirrored invert map produced `Position NaN` and the
+  // dispatch fallback then applied the deletion untracked.
+  const setupTwoLines = () => {
+    ({ editor } = initTestEditor({
+      mode: 'text',
+      content: '<p>Line 1</p><p>Line 2</p>',
+      user: ALICE,
+      trackedChanges: {},
+    }));
+    editor.commands.enableTrackChanges();
+
+    let p2TextStart = null;
+    editor.state.doc.descendants((node, pos) => {
+      if (node.isText && node.text === 'Line 2') p2TextStart = pos;
+    });
+    // User selection from the start of "Line 1" through "Lin" of line 2,
+    // dispatched on its own like a mouse selection.
+    editor.commands.command(({ tr, dispatch }) => {
+      tr.setSelection(TextSelection.create(tr.doc, 1, p2TextStart + 3));
+      if (dispatch) dispatch(tr);
+      return true;
+    });
+  };
+
+  const paragraphTexts = (doc) => {
+    const texts = [];
+    doc.forEach((node) => texts.push(node.textContent));
+    return texts;
+  };
+
+  it.each([
+    ['Backspace', handleBackspace],
+    ['Delete', handleDelete],
+  ])('%s tracks the deletion instead of hard-deleting', (_label, handler) => {
+    setupTwoLines();
+    const handled = handler(editor);
+    expect(handled).toBe(true);
+
+    // Content survives as a tracked deletion — never hard-deleted.
+    expect(paragraphTexts(editor.state.doc)).toEqual(['Line 1', 'Line 2']);
+    const graph = graphFor(editor);
+    expect(graph.changes.size).toBe(1);
+    expect(Array.from(graph.changes.values())[0].type).toBe(CanonicalChangeType.Deletion);
+
+    // Caret lands at the left edge of the tracked deletion, inside paragraph 1.
+    expect(editor.state.selection.empty).toBe(true);
+    expect(editor.state.selection.from).toBeLessThan(editor.state.doc.firstChild.nodeSize);
+  });
+});

packages/super-editor/src/editors/v1/extensions/track-changes/track-changes.js

@@ -96,7 +96,9 @@ const dispatchReviewDecision = ({ editor, state, dispatch, decision, target }) =
           originalId: event.changeId,
         }).some(({ mark }) => mark.attrs?.splitFromId === event.changeId);
         if (successorsPresent) continue;
-        editor.emit('commentsUpdate', event);
+        // Carry the decision so resolved bubbles can report accepted vs
+        // rejected instead of assuming every resolution was an accept.
+        editor.emit('commentsUpdate', { ...event, decision });
       }
 
       const touched =

packages/super-editor/src/editors/v1/extensions/track-changes/trackChangesHelpers/replaceStep.js

@@ -171,6 +171,27 @@ const normalizeReplaceStepSingleCharDelete = ({ step, doc }) => {
   }
 };
 
+/**
+ * Whether a slice carries any inline leaf content (text, hard breaks, inline
+ * widgets). A slice without inline leaves is a purely structural shell — e.g.
+ * the paragraph re-join slice `deleteRange` produces for a deletion that spans
+ * a paragraph boundary (`<paragraph><run/></paragraph>` with open ends).
+ *
+ * @param {import('prosemirror-model').Slice} slice
+ * @returns {boolean}
+ */
+const sliceHasInlineLeafContent = (slice) => {
+  let found = false;
+  slice.content.descendants((node) => {
+    if (found) return false;
+    if (node.isInline && (node.isText || node.isLeaf)) {
+      found = true;
+      return false;
+    }
+  });
+  return found;
+};
+
 /**
  * Replace step.
  * @param {object} options Replace step options.
@@ -555,6 +576,14 @@ const tryCompileStep = ({
     if (!hasInlineContent) return { handled: false };
   }
 
+  // A non-empty slice without inline leaf content is a structural re-join
+  // shell, not user-visible replacement content. `deleteRange` emits one for
+  // a deletion that spans a paragraph boundary; compiling it as a replacement
+  // would insert the empty block shell at the range end and split the
+  // trailing paragraph (SD-3386). Track it as a plain deletion instead.
+  const isStructuralShellDelete =
+    step.from !== step.to && step.slice.content.size > 0 && !sliceHasInlineLeafContent(step.slice);
+
   // Build the intent. Pure inserts and pure deletes use the matching intent
   // type; mixed (text-replace) carries the original slice.
   let intent;
@@ -570,7 +599,7 @@ const tryCompileStep = ({
         source,
         preserveExistingReviewState,
       });
-    } else if (step.from !== step.to && step.slice.content.size === 0) {
+    } else if (step.from !== step.to && (step.slice.content.size === 0 || isStructuralShellDelete)) {
       intent = makeTextDeleteIntent({
         from: step.from,
         to: step.to,
@@ -641,7 +670,17 @@ const tryCompileStep = ({
     map.appendMap(invertStep.getMap());
     const mirrorIndex = map.maps.length - 1;
     for (let i = beforeSteps; i < newTr.steps.length; i += 1) {
-      map.appendMap(newTr.steps[i].getMap(), mirrorIndex);
+      // Mirror pairing assumes the compiled steps re-apply the original
+      // step's position effect (the condensed insert in the replace path).
+      // A structural-shell delete compiles to mark steps only — pairing
+      // their empty maps as mirrors of the invert map corrupts position
+      // recovery (Position NaN) for positions inside the never-inserted
+      // shell, so append those without mirrors (SD-3386).
+      if (isStructuralShellDelete) {
+        map.appendMap(newTr.steps[i].getMap());
+      } else {
+        map.appendMap(newTr.steps[i].getMap(), mirrorIndex);
+      }
     }
   } else {
     for (let i = beforeSteps; i < newTr.steps.length; i += 1) {
@@ -676,7 +715,11 @@ const tryCompileStep = ({
     // fall back to a shaped step the comments plugin already understands.
     meta.step = { slice: { content: { content: result.insertedNodes } } };
   }
-  if (result.selection?.kind === 'near' && stepWasNormalized && !result.insertedMark) {
+  // Structural-shell deletes also need the explicit override: the original
+  // step's post-step selection can sit inside the shell slice that was never
+  // inserted, so mapping it back is meaningless — honor the compiler's caret
+  // hint (left edge of the tracked deletion) instead.
+  if (result.selection?.kind === 'near' && (stepWasNormalized || isStructuralShellDelete) && !result.insertedMark) {
     meta.selectionPos = result.selection.pos;
   }
   newTr.setMeta(TrackChangesBasePluginKey, meta);

packages/superdoc/src/components/CommentsLayer/CommentDialog.test.js

@@ -767,6 +767,7 @@ describe('CommentDialog.vue', () => {
       email: superdocStoreStub.user.email,
       name: superdocStoreStub.user.name,
       superdoc: expect.any(Object),
+      decision: 'accept',
     });
     expect(superdocStub.focus).toHaveBeenCalledTimes(1);
 
@@ -778,6 +779,50 @@ describe('CommentDialog.vue', () => {
     expect(superdocStub.focus).toHaveBeenCalledTimes(2);
   });
 
+  it('does not resolve the tracked-change thread when the decision fails (SD-3386)', async () => {
+    const { wrapper, baseComment } = await mountDialog({
+      baseCommentOverrides: {
+        trackedChange: true,
+        trackedChangeType: 'trackDelete',
+        trackedChangeText: 'Removed',
+      },
+    });
+    commentsStoreStub.decideTrackedChangeFromSidebar.mockReturnValueOnce({ ok: true, success: false });
+
+    const header = wrapper.findComponent(CommentHeaderStub);
+    header.vm.$emit('reject');
+    await nextTick();
+    expect(baseComment.resolveComment).not.toHaveBeenCalled();
+  });
+
+  it('labels a rejected tracked change as Rejected, not Accepted (SD-3386)', async () => {
+    const { wrapper } = await mountDialog({
+      baseCommentOverrides: {
+        trackedChange: true,
+        trackedChangeType: 'trackDelete',
+        trackedChangeText: 'Removed',
+        resolvedTime: Date.now(),
+        trackedChangeDecision: 'reject',
+      },
+    });
+
+    expect(wrapper.find('.resolved-badge').text()).toContain('Rejected');
+  });
+
+  it('labels an accepted tracked change as Accepted', async () => {
+    const { wrapper } = await mountDialog({
+      baseCommentOverrides: {
+        trackedChange: true,
+        trackedChangeType: 'trackInsert',
+        trackedChangeText: 'Added',
+        resolvedTime: Date.now(),
+        trackedChangeDecision: 'accept',
+      },
+    });
+
+    expect(wrapper.find('.resolved-badge').text()).toContain('Accepted');
+  });
+
   it('renders hyperlink additions without a format label', async () => {
     const { wrapper } = await mountDialog({
       baseCommentOverrides: {