Fix nested addMarkSteps inside insertions.

.yarn/versions/1b7b7658.yml

@@ -0,0 +1,2 @@
+releases:
+  "@handlewithcare/prosemirror-suggest-changes": patch

src/__tests__/addMarkStep.test.ts

@@ -95,4 +95,211 @@ describe("AddMarkStep", () => {
       `Expected ${trackedState.doc} to match ${expected}`,
     );
   });
+
+  it("should handle nested addMarkSteps with inner mark completely inside outer mark", () => {
+    // Starting document already has a suggestion change with strong mark applied
+    const doc = testBuilders.doc(
+      testBuilders.paragraph(
+        "This ",
+        testBuilders.deletion({ id: 1 }, "is a test paragraph with"),
+        testBuilders.insertion(
+          { id: 1 },
+          testBuilders.strong("is a <b>test paragraph<c> with"),
+        ),
+        " content",
+      ),
+    ) as TaggedNode;
+
+    const editorState = EditorState.create({
+      doc,
+    });
+
+    // Add emphasis mark to a smaller range completely inside the existing strong mark (from 'b' to 'c')
+    const originalTransaction = editorState.tr;
+    originalTransaction.addMark(
+      doc.tag["b"]!,
+      doc.tag["c"]!,
+      testBuilders.schema.marks.em.create(),
+    );
+    const step = originalTransaction.steps[0];
+    assert(step instanceof AddMarkStep, "Could not create AddMarkStep");
+
+    // Apply the step with tracking
+    const trackedTransaction = editorState.tr;
+    trackAddMarkStep(trackedTransaction, editorState, doc, step, [], 1);
+
+    const finalState = editorState.apply(trackedTransaction);
+
+    // Expected result should handle the nested mark properly within the existing suggestion changes
+    const expected = testBuilders.doc(
+      testBuilders.paragraph(
+        "This ",
+        testBuilders.deletion({ id: 1 }, "is a test paragraph with"),
+        testBuilders.insertion({ id: 1 }, testBuilders.strong("is a ")),
+        testBuilders.insertion(
+          { id: 1 },
+          testBuilders.strong(testBuilders.em("test paragraph")),
+        ),
+        testBuilders.insertion({ id: 1 }, testBuilders.strong(" with")),
+        " content",
+      ),
+    );
+
+    assert(
+      eq(finalState.doc, expected),
+      `Expected ${finalState.doc} to match ${expected}`,
+    );
+  });
+
+  it("should handle left-side partial overlap of mark on existing suggestion", () => {
+    // Existing suggestion spans "test paragraph"
+    const doc = testBuilders.doc(
+      testBuilders.paragraph(
+        "This is <a>a ",
+        testBuilders.deletion({ id: 1 }, "test paragraph"),
+        testBuilders.insertion(
+          { id: 1 },
+          testBuilders.strong("test para<b>graph"),
+        ),
+        " with content",
+      ),
+    ) as TaggedNode;
+
+    const editorState = EditorState.create({ doc });
+
+    // Add emphasis mark that partially overlaps from left: "a test para"
+    const originalTransaction = editorState.tr;
+    originalTransaction.addMark(
+      doc.tag["a"]!,
+      doc.tag["b"]!,
+      testBuilders.schema.marks.em.create(),
+    );
+    const step = originalTransaction.steps[0];
+    assert(step instanceof AddMarkStep, "Could not create AddMarkStep");
+
+    const trackedTransaction = editorState.tr;
+    trackAddMarkStep(trackedTransaction, editorState, doc, step, [], 2);
+
+    const finalState = editorState.apply(trackedTransaction);
+
+    // Expected: when there's overlap, marks are properly nested
+    const expected = testBuilders.doc(
+      testBuilders.paragraph(
+        "This is ",
+        testBuilders.deletion({ id: 1 }, "a test paragraph"),
+        testBuilders.insertion({ id: 1 }, testBuilders.em("a ")),
+        testBuilders.insertion(
+          { id: 1 },
+          testBuilders.em(testBuilders.strong("test para")),
+        ),
+        testBuilders.insertion({ id: 1 }, testBuilders.strong("graph")),
+        " with content",
+      ),
+    );
+
+    assert(
+      eq(finalState.doc, expected),
+      `Expected ${finalState.doc} to match ${expected}`,
+    );
+  });
+
+  it("should handle right-side partial overlap of mark on existing suggestion", () => {
+    // Existing suggestion spans "test paragraph"
+    const doc = testBuilders.doc(
+      testBuilders.paragraph(
+        "This is a ",
+        testBuilders.deletion({ id: 1 }, "test paragraph"),
+        testBuilders.insertion(
+          { id: 1 },
+          testBuilders.strong("test para<a>graph"),
+        ),
+        "<b> with content",
+      ),
+    ) as TaggedNode;
+
+    const editorState = EditorState.create({ doc });
+
+    // Add emphasis mark that partially overlaps from right: "graph with"
+    const originalTransaction = editorState.tr;
+    originalTransaction.addMark(
+      doc.tag["a"]!,
+      doc.tag["b"]!,
+      testBuilders.schema.marks.em.create(),
+    );
+    const step = originalTransaction.steps[0];
+    assert(step instanceof AddMarkStep, "Could not create AddMarkStep");
+
+    const trackedTransaction = editorState.tr;
+    trackAddMarkStep(trackedTransaction, editorState, doc, step, [], 2);
+
+    const finalState = editorState.apply(trackedTransaction);
+
+    // Current behavior: only marks adjacent get inherited ID
+    const expected = testBuilders.doc(
+      testBuilders.paragraph(
+        "This is a ",
+        testBuilders.deletion({ id: 1 }, "test paragraph"),
+        testBuilders.insertion({ id: 1 }, testBuilders.strong("test para")),
+        testBuilders.insertion(
+          { id: 1 },
+          testBuilders.em(testBuilders.strong("graph")),
+        ),
+        " with content",
+      ),
+    );
+
+    assert(
+      eq(finalState.doc, expected),
+      `Expected ${finalState.doc} to match ${expected}`,
+    );
+  });
+
+  it("should handle existing suggestion completely inside new mark", () => {
+    // Small existing suggestion "test"
+    const doc = testBuilders.doc(
+      testBuilders.paragraph(
+        "This is <a>a ",
+        testBuilders.deletion({ id: 1 }, "test"),
+        testBuilders.insertion({ id: 1 }, testBuilders.strong("test")),
+        " paragraph<b> with content",
+      ),
+    ) as TaggedNode;
+
+    const editorState = EditorState.create({ doc });
+
+    // Add emphasis mark that encompasses the suggestion: "a test paragraph"
+    const originalTransaction = editorState.tr;
+    originalTransaction.addMark(
+      doc.tag["a"]!,
+      doc.tag["b"]!,
+      testBuilders.schema.marks.em.create(),
+    );
+    const step = originalTransaction.steps[0];
+    assert(step instanceof AddMarkStep, "Could not create AddMarkStep");
+
+    const trackedTransaction = editorState.tr;
+    trackAddMarkStep(trackedTransaction, editorState, doc, step, [], 2);
+
+    const finalState = editorState.apply(trackedTransaction);
+
+    // Current behavior: uses new ID for entire range
+    const expected = testBuilders.doc(
+      testBuilders.paragraph(
+        "This is ",
+        testBuilders.deletion({ id: 2 }, "a test paragraph"),
+        testBuilders.insertion({ id: 2 }, testBuilders.em("a ")),
+        testBuilders.insertion(
+          { id: 2 },
+          testBuilders.em(testBuilders.strong("test")),
+        ),
+        testBuilders.insertion({ id: 2 }, testBuilders.em(" paragraph")),
+        " with content",
+      ),
+    );
+
+    assert(
+      eq(finalState.doc, expected),
+      `Expected ${finalState.doc} to match ${expected}`,
+    );
+  });
 });

src/addMarkStep.ts

@@ -7,7 +7,7 @@ import {
   replaceStep,
 } from "prosemirror-transform";
 
-import { applySuggestionsToSlice } from "./commands.js";
+import { applySuggestionsToRange } from "./commands.js";
 import { suggestReplaceStep } from "./replaceStep.js";
 
 /**
@@ -27,13 +27,8 @@ export function trackAddMarkStep(
 ) {
   const applied = step.apply(doc).doc;
   if (!applied) return false;
-  const slice = applied.slice(step.from, step.to);
-  const replace = replaceStep(
-    doc,
-    step.from,
-    step.to,
-    applySuggestionsToSlice(slice),
-  );
+  const slice = applySuggestionsToRange(applied, step.from, step.to);
+  const replace = replaceStep(doc, step.from, step.to, slice);
   if (!replace) return false;
 
   return suggestReplaceStep(

src/commands.ts

@@ -1,10 +1,4 @@
-import {
-  Fragment,
-  type Mark,
-  type MarkType,
-  type Node,
-  Slice,
-} from "prosemirror-model";
+import { type Mark, type MarkType, type Node } from "prosemirror-model";
 import {
   type Command,
   type EditorState,
@@ -31,6 +25,8 @@ function applySuggestionsToTransform(
   markTypeToApply: MarkType,
   markTypeToRevert: MarkType,
   suggestionId?: number,
+  from?: number,
+  to?: number,
 ) {
   const toApplyIsInSet =
     suggestionId === undefined
@@ -55,6 +51,12 @@ function applySuggestionsToTransform(
   }
 
   node.descendants((child, pos) => {
+    if (from !== undefined && pos < from) {
+      return true;
+    }
+    if (to !== undefined && pos > to) {
+      return false;
+    }
     const isToRevert = toRevertIsInSet(child.marks);
     const isToApply = toApplyIsInSet(child.marks);
     if (!isToRevert && !isToApply) {
@@ -140,21 +142,34 @@ function revertModifications(node: Node, pos: number, tr: Transform) {
   }
 }
 
+function modificationIsInSet(
+  modification: MarkType,
+  id: number | undefined,
+  marks: readonly Mark[],
+) {
+  const mark = modification.isInSet(marks);
+  if (id === undefined) return mark;
+
+  if (mark?.attrs["id"] === id) return mark;
+
+  return undefined;
+}
+
 function applyModificationsToTransform(
   node: Node,
   tr: Transform,
   dir: number,
   suggestionId?: number,
+  from?: number,
+  to?: number,
 ) {
   const { modification } = getSuggestionMarks(node.type.schema);
 
-  const modificationIsInSet =
-    suggestionId === undefined
-      ? (marks: readonly Mark[]) => modification.isInSet(marks)
-      : (marks: readonly Mark[]) =>
-          modification.create({ id: suggestionId }).isInSet(marks);
-
-  const isModification = modificationIsInSet(node.marks);
+  const isModification = modificationIsInSet(
+    modification,
+    suggestionId,
+    node.marks,
+  );
 
   if (isModification) {
     let prevLength: number;
@@ -169,7 +184,17 @@ function applyModificationsToTransform(
   }
 
   node.descendants((child, pos) => {
-    const isModification = modificationIsInSet(child.marks);
+    if (from !== undefined && pos < from) {
+      return true;
+    }
+    if (to !== undefined && pos > to) {
+      return false;
+    }
+    const isModification = modificationIsInSet(
+      modification,
+      suggestionId,
+      child.marks,
+    );
     if (!isModification) {
       return true;
     }
@@ -188,26 +213,44 @@ function applyModificationsToTransform(
   });
 }
 
-function applySuggestionsToNode(node: Node) {
+export function applySuggestionsToNode(node: Node) {
   const { deletion, insertion } = getSuggestionMarks(node.type.schema);
 
-  if (deletion.isInSet(node.marks)) {
-    return null;
-  }
-
   const transform = new Transform(node);
   applySuggestionsToTransform(node, transform, insertion, deletion);
   applyModificationsToTransform(node, transform, 1);
   return transform.doc;
 }
 
-export function applySuggestionsToSlice(slice: Slice) {
-  const nodes: Node[] = [];
-  slice.content.forEach((node) => {
-    const applied = applySuggestionsToNode(node);
-    if (applied) nodes.push(applied);
-  });
-  return new Slice(Fragment.from(nodes), slice.openStart, slice.openEnd);
+export function applySuggestionsToRange(doc: Node, from: number, to: number) {
+  // blockRange can only return null if a predicate is provided
+  // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
+  const nodeRange = doc.resolve(from).blockRange(doc.resolve(to))!;
+
+  const { deletion, insertion } = getSuggestionMarks(doc.type.schema);
+
+  const transform = new Transform(doc);
+  applySuggestionsToTransform(
+    doc,
+    transform,
+    insertion,
+    deletion,
+    undefined,
+    nodeRange.start,
+    nodeRange.end,
+  );
+  applyModificationsToTransform(
+    doc,
+    transform,
+    1,
+    nodeRange.start,
+    nodeRange.end,
+  );
+
+  return transform.doc.slice(
+    transform.mapping.map(from),
+    transform.mapping.map(to),
+  );
 }
 
 /**