fix(track-changes): remove ghost TrackFormat on multi-node format cancel (#2233)

* fix: remove persisting yellow underline after canceling track-format suggestion

Two bugs caused the gold/yellow TrackFormat underline to persist after
reversing a tracked format change:

1. addMarkStep applied TrackFormat using the full step range instead of
   clamped per-node boundaries, leaking wrong before/after arrays onto
   adjacent text nodes.

2. removeMarkStep didn't clean up fully-reversed format changes — when
   a tracked addition was toggled back (after=[] but before had items),
   the TrackFormat mark persisted instead of being removed.

* fix: only remove TrackFormat when both before and after are empty

Narrowed the early-exit condition so TrackFormat is only removed when
both arrays are empty (true no-op). Keeps the mark when before has
legitimate tracked removals (e.g. before=[italic], after=[]).

* fix: share TrackFormat ID across nodes in the same addMarkStep

Hoist a sharedWid variable so all inline nodes touched by a single
addMarkStep reuse the same TrackFormat ID. With per-node clamped ranges,
getLiveInlineMarksInRange no longer sees the previous node's TrackFormat,
causing each node to generate a separate UUID. This broke ID-based
reject/accept flows for multi-node selections.

* fix: share TrackFormat ID across nodes in removeMarkStep

Mirror the sharedWid pattern from addMarkStep. When an existing
formatChangeMark is present, reuse its ID to preserve change grouping.
When creating new TrackFormat marks across multiple nodes, share a
single generated ID so reject/accept treats them as one change.

* fix(track-changes): clean up TrackFormat when cancel is a no-op on nodes with pre-existing marks

When bold is toggled on then off across multi-node selections where some
nodes have pre-existing marks (e.g., italic), the removeMarkStep cancel
path only cleaned up when both `before` and `after` arrays were empty.
This missed the case where `before` had pre-existing marks that were
never actually removed — leaving a ghost TrackFormat mark showing
"Format: removed italic" even though italic was untouched.

The fix checks whether all marks in `before` still exist on the node.
If they do, the tracked change is a no-op and the TrackFormat is removed.

* fix(track-changes): use attribute-aware comparison in isNoop check

The isNoop check compared mark snapshots by type name only, ignoring
attributes. For textStyle/highlight marks with differing attrs, this
could incorrectly treat a real change as a no-op. Use the existing
markSnapshotMatchesStepMark helper for full type+attrs comparison.

Also simplifies multi-node test to use the addThenRemoveMark helper.

* test(track-changes): add multi-node format cancel tests

- Unit tests for sharedWid across runs and range clamping
- Behavior test for toggling bold on/off across mixed-mark nodes

---------

Co-authored-by: G Pardhiv Varma <gpardhivvarma@gmail.com>

Author: caio-pizzol

packages/super-editor/src/extensions/track-changes/trackChangesHelpers/addMarkStep.js

@@ -18,6 +18,7 @@ import { getLiveInlineMarksInRange } from './getLiveInlineMarksInRange.js';
  */
 export const addMarkStep = ({ state, step, newTr, doc, user, date }) => {
   const meta = {};
+  let sharedWid = null;
 
   doc.nodesBetween(step.from, step.to, (node, pos) => {
     if (!node.isInline || node.type.name === 'run') {
@@ -30,6 +31,7 @@ export const addMarkStep = ({ state, step, newTr, doc, user, date }) => {
 
     const rangeFrom = Math.max(step.from, pos);
     const rangeTo = Math.min(step.to, pos + node.nodeSize);
+
     const liveMarks = getLiveInlineMarksInRange({
       doc: newTr.doc,
       from: rangeFrom,
@@ -38,7 +40,7 @@ export const addMarkStep = ({ state, step, newTr, doc, user, date }) => {
     const existingChangeMark = liveMarks.find((mark) =>
       [TrackDeleteMarkName, TrackFormatMarkName].includes(mark.type.name),
     );
-    const wid = existingChangeMark ? existingChangeMark.attrs.id : uuidv4();
+    const wid = existingChangeMark ? existingChangeMark.attrs.id : (sharedWid ?? (sharedWid = uuidv4()));
     newTr.addMark(Math.max(step.from, pos), Math.min(step.to, pos + node.nodeSize), step.mark);
 
     const allowedMarks = ['bold', 'italic', 'strike', 'underline', 'textStyle', 'highlight'];
@@ -93,11 +95,7 @@ export const addMarkStep = ({ state, step, newTr, doc, user, date }) => {
           before,
           after,
         });
-        newTr.addMark(
-          step.from, // Math.max(step.from, pos)
-          step.to, // Math.min(step.to, pos + node.nodeSize),
-          newFormatMark,
-        );
+        newTr.addMark(Math.max(step.from, pos), Math.min(step.to, pos + node.nodeSize), newFormatMark);
 
         meta.formatMark = newFormatMark;
         meta.step = step;

packages/super-editor/src/extensions/track-changes/trackChangesHelpers/removeMarkStep.js

@@ -16,6 +16,7 @@ import { getLiveInlineMarksInRange } from './getLiveInlineMarksInRange.js';
  */
 export const removeMarkStep = ({ state, step, newTr, doc, user, date }) => {
   const meta = {};
+  let sharedWid = null;
 
   doc.nodesBetween(step.from, step.to, (node, pos) => {
     if (!node.isInline || node.type.name === 'run') {
@@ -52,6 +53,23 @@ export const removeMarkStep = ({ state, step, newTr, doc, user, date }) => {
           after = [
             ...formatChangeMark.attrs.after.filter((mark) => !markSnapshotMatchesStepMark(mark, step.mark, true)),
           ];
+          if (after.length === 0) {
+            // All additions were canceled. Check if any marks in `before` were
+            // actually removed from the node. If they all still exist, the
+            // tracked change is a no-op — clean it up.
+            const remainingFormatMarks = liveMarksBeforeRemove.filter(
+              (m) =>
+                ![TrackDeleteMarkName, TrackFormatMarkName].includes(m.type.name) &&
+                m.type.name !== step.mark.type.name,
+            );
+            const isNoop = formatChangeMark.attrs.before.every((snapshot) =>
+              remainingFormatMarks.some((m) => markSnapshotMatchesStepMark(snapshot, m, true)),
+            );
+            if (isNoop) {
+              newTr.removeMark(Math.max(step.from, pos), Math.min(step.to, pos + node.nodeSize), formatChangeMark);
+              return;
+            }
+          }
           before = [...formatChangeMark.attrs.before];
         } else {
           after = [...formatChangeMark.attrs.after];
@@ -77,7 +95,7 @@ export const removeMarkStep = ({ state, step, newTr, doc, user, date }) => {
 
       if (after.length || before.length) {
         const newFormatMark = state.schema.marks[TrackFormatMarkName].create({
-          id: uuidv4(),
+          id: formatChangeMark ? formatChangeMark.attrs.id : (sharedWid ?? (sharedWid = uuidv4())),
           author: user.name,
           authorEmail: user.email,
           authorImage: user.image,

packages/super-editor/src/extensions/track-changes/trackChangesHelpers/removeMarkStep.test.js

@@ -0,0 +1,229 @@
+import { beforeEach, afterEach, describe, expect, it, vi } from 'vitest';
+import { EditorState } from 'prosemirror-state';
+import { AddMarkStep, RemoveMarkStep } from 'prosemirror-transform';
+import { TrackFormatMarkName } from '../constants.js';
+import { TrackChangesBasePluginKey } from '../plugins/trackChangesBasePlugin.js';
+import { addMarkStep } from './addMarkStep.js';
+import { removeMarkStep } from './removeMarkStep.js';
+import { initTestEditor } from '@tests/helpers/helpers.js';
+
+describe('removeMarkStep cancel logic', () => {
+  let editor;
+  let schema;
+  let basePlugins;
+
+  const user = { name: 'Track Tester', email: 'track@example.com' };
+  const date = '2024-01-01T00:00:00.000Z';
+
+  beforeEach(() => {
+    ({ editor } = initTestEditor({ mode: 'text', content: '<p></p>' }));
+    schema = editor.schema;
+    basePlugins = editor.state.plugins;
+  });
+
+  afterEach(() => {
+    vi.restoreAllMocks();
+    editor?.destroy();
+    editor = null;
+  });
+
+  const createDocWithRuns = (runs) => {
+    const runNodes = runs.map(({ text, marks = [] }) => schema.nodes.run.create({}, schema.text(text, marks)));
+    return schema.nodes.doc.create({}, schema.nodes.paragraph.create({}, runNodes));
+  };
+
+  const createState = (doc) =>
+    EditorState.create({
+      schema,
+      doc,
+      plugins: basePlugins,
+    });
+
+  /**
+   * Simulate add-then-remove of a mark and return the resulting doc.
+   * Applies addMarkStep to get the TrackFormat mark, then removeMarkStep.
+   */
+  const addThenRemoveMark = ({ state, mark, from, to }) => {
+    const addStep = new AddMarkStep(from, to, mark);
+    const addTr = state.tr;
+    addMarkStep({ state, step: addStep, newTr: addTr, doc: state.doc, user, date });
+    const stateAfterAdd = state.apply(addTr);
+
+    const removeStep = new RemoveMarkStep(from, to, mark);
+    const removeTr = stateAfterAdd.tr;
+    removeMarkStep({ state: stateAfterAdd, step: removeStep, newTr: removeTr, doc: stateAfterAdd.doc, user, date });
+    return { stateAfterRemove: stateAfterAdd.apply(removeTr), removeTr };
+  };
+
+  const hasTrackFormatMark = (doc) => {
+    let found = false;
+    doc.descendants((node) => {
+      if (node.marks?.some((m) => m.type.name === TrackFormatMarkName)) {
+        found = true;
+      }
+    });
+    return found;
+  };
+
+  it('removes TrackFormat when bold is toggled on then off (no pre-existing marks)', () => {
+    const doc = createDocWithRuns([{ text: 'Hello' }]);
+    const state = createState(doc);
+
+    const { stateAfterRemove } = addThenRemoveMark({
+      state,
+      mark: schema.marks.bold.create(),
+      from: 2,
+      to: 7,
+    });
+
+    expect(hasTrackFormatMark(stateAfterRemove.doc)).toBe(false);
+  });
+
+  it('removes TrackFormat when bold is toggled on then off (node has pre-existing italic)', () => {
+    const italic = schema.marks.italic.create();
+    const doc = createDocWithRuns([{ text: 'world', marks: [italic] }]);
+    const state = createState(doc);
+
+    const { stateAfterRemove } = addThenRemoveMark({
+      state,
+      mark: schema.marks.bold.create(),
+      from: 2,
+      to: 7,
+    });
+
+    // The bug: before this fix, a ghost TrackFormat with before=[italic], after=[]
+    // would persist, showing "Format: removed italic" even though italic was never touched.
+    expect(hasTrackFormatMark(stateAfterRemove.doc)).toBe(false);
+  });
+
+  it('keeps TrackFormat when before marks were actually removed from the node', () => {
+    // Setup: "world" has italic + bold
+    const italic = schema.marks.italic.create();
+    const bold = schema.marks.bold.create();
+    const doc = createDocWithRuns([{ text: 'world', marks: [italic, bold] }]);
+    const state = createState(doc);
+
+    // Step 1: Remove italic (creates TrackFormat with before=[italic], after=[])
+    const removeItalicStep = new RemoveMarkStep(2, 7, italic);
+    const removeItalicTr = state.tr;
+    removeMarkStep({ state, step: removeItalicStep, newTr: removeItalicTr, doc: state.doc, user, date });
+    const stateAfterRemoveItalic = state.apply(removeItalicTr);
+
+    // Step 2: Add bold (bold already exists, so this is a no-op for addMarkStep's
+    // hasMatchingMark check — bold is already live). Instead, simulate adding underline
+    // then removing it to test the cancel logic with a pre-existing removal.
+    const addUnderlineStep = new AddMarkStep(2, 7, schema.marks.underline.create());
+    const addUnderlineTr = stateAfterRemoveItalic.tr;
+    addMarkStep({
+      state: stateAfterRemoveItalic,
+      step: addUnderlineStep,
+      newTr: addUnderlineTr,
+      doc: stateAfterRemoveItalic.doc,
+      user,
+      date,
+    });
+    const stateAfterAddUnderline = stateAfterRemoveItalic.apply(addUnderlineTr);
+
+    // Step 3: Remove underline (cancel the addition). The TrackFormat should remain
+    // because italic was genuinely removed and is no longer on the node.
+    const removeUnderlineStep = new RemoveMarkStep(2, 7, schema.marks.underline.create());
+    const removeUnderlineTr = stateAfterAddUnderline.tr;
+    removeMarkStep({
+      state: stateAfterAddUnderline,
+      step: removeUnderlineStep,
+      newTr: removeUnderlineTr,
+      doc: stateAfterAddUnderline.doc,
+      user,
+      date,
+    });
+    const stateAfterRemoveUnderline = stateAfterAddUnderline.apply(removeUnderlineTr);
+
+    // TrackFormat should persist because italic was actually removed
+    expect(hasTrackFormatMark(stateAfterRemoveUnderline.doc)).toBe(true);
+
+    // Verify the remaining TrackFormat records the italic removal
+    let trackFormatAttrs = null;
+    stateAfterRemoveUnderline.doc.descendants((node) => {
+      const mark = node.marks?.find((m) => m.type.name === TrackFormatMarkName);
+      if (mark) trackFormatAttrs = mark.attrs;
+    });
+    expect(trackFormatAttrs.before).toEqual(expect.arrayContaining([expect.objectContaining({ type: 'italic' })]));
+  });
+
+  it('shares TrackFormat ID across nodes when addMarkStep spans multiple runs', () => {
+    const italic = schema.marks.italic.create();
+    const doc = createDocWithRuns([{ text: 'Hello ' }, { text: 'world', marks: [italic] }]);
+    const state = createState(doc);
+
+    const bold = schema.marks.bold.create();
+    const step = new AddMarkStep(2, 13, bold);
+    const tr = state.tr;
+    addMarkStep({ state, step, newTr: tr, doc: state.doc, user, date });
+    const result = state.apply(tr);
+
+    // Collect TrackFormat mark IDs from all inline nodes
+    const ids = [];
+    result.doc.descendants((node) => {
+      const mark = node.marks?.find((m) => m.type.name === TrackFormatMarkName);
+      if (mark) ids.push(mark.attrs.id);
+    });
+
+    expect(ids.length).toBe(2);
+    expect(ids[0]).toBe(ids[1]);
+  });
+
+  it('clamps TrackFormat mark to node boundaries, not step range', () => {
+    // doc > paragraph > run_1("Hello " at pos 2-7) + run_2("world" at pos 10-14)
+    // Run open/close tags add +1 offset each, so run_2 content starts at pos 10.
+    const doc = createDocWithRuns([{ text: 'Hello ' }, { text: 'world' }]);
+    const state = createState(doc);
+
+    // Apply bold from pos 5-11. Clamped per-node:
+    //   run_1 text [2,8): [max(5,2), min(11,8)] = [5,8] → "lo " (3 chars)
+    //   run_2 text [10,15): [max(5,10), min(11,15)] = [10,11] → "w" (1 char)
+    const bold = schema.marks.bold.create();
+    const step = new AddMarkStep(5, 11, bold);
+    const tr = state.tr;
+    addMarkStep({ state, step, newTr: tr, doc: state.doc, user, date });
+    const result = state.apply(tr);
+
+    // Collect TrackFormat marks — each should be scoped to its text portion
+    const trackMarks = [];
+    result.doc.descendants((node, pos) => {
+      const mark = node.marks?.find((m) => m.type.name === TrackFormatMarkName);
+      if (mark) trackMarks.push({ pos, size: node.nodeSize, text: node.text, id: mark.attrs.id });
+    });
+
+    // Two TrackFormat marks: one on "lo " (from first run) and one on "w" (from second run)
+    expect(trackMarks.length).toBe(2);
+    expect(trackMarks[0].text).toBe('lo ');
+    expect(trackMarks[1].text).toBe('w');
+    // Both should share the same ID (sharedWid)
+    expect(trackMarks[0].id).toBe(trackMarks[1].id);
+    // "Hel" and "orld" should NOT have TrackFormat (they were outside the step range)
+    const nonTrackedTexts = [];
+    result.doc.descendants((node) => {
+      if (node.isText && !node.marks?.some((m) => m.type.name === TrackFormatMarkName)) {
+        nonTrackedTexts.push(node.text);
+      }
+    });
+    expect(nonTrackedTexts).toContain('Hel');
+    expect(nonTrackedTexts).toContain('orld');
+  });
+
+  it('removes TrackFormat for multi-node bold toggle (Hello plain + world italic)', () => {
+    const italic = schema.marks.italic.create();
+    const doc = createDocWithRuns([{ text: 'Hello ' }, { text: 'world', marks: [italic] }]);
+    const state = createState(doc);
+
+    const { stateAfterRemove } = addThenRemoveMark({
+      state,
+      mark: schema.marks.bold.create(),
+      from: 2,
+      to: 13,
+    });
+
+    // Neither node should have a TrackFormat mark
+    expect(hasTrackFormatMark(stateAfterRemove.doc)).toBe(false);
+  });
+});

tests/behavior/tests/comments/cancel-multinode-format-suggestion.spec.ts

@@ -0,0 +1,63 @@
+import { test, expect } from '../../fixtures/superdoc.js';
+
+test.use({ config: { toolbar: 'full', comments: 'panel', trackChanges: true } });
+
+/**
+ * Regression test for the multi-node cancel scenario:
+ * When bold is toggled on then off across nodes with different pre-existing marks,
+ * no ghost TrackFormat marks should remain.
+ */
+test('no ghost TrackFormat after toggling bold on then off across mixed-mark nodes', async ({ superdoc }) => {
+  // Type two words — we'll make only the second one italic to create mixed marks
+  await superdoc.type('Hello world');
+  await superdoc.waitForStable();
+
+  // Select "world" and make it italic in editing mode
+  await superdoc.page.evaluate(() => {
+    const editor = (window as any).editor;
+    const { doc } = editor.state;
+    // Find "world" position (after "Hello ")
+    let worldFrom = 0;
+    let worldTo = 0;
+    doc.descendants((node: any, pos: number) => {
+      if (node.isText && node.text?.includes('world')) {
+        const offset = node.text.indexOf('world');
+        worldFrom = pos + offset;
+        worldTo = worldFrom + 'world'.length;
+      }
+    });
+    editor.commands.setTextSelection({ from: worldFrom, to: worldTo });
+    editor.commands.toggleItalic();
+  });
+  await superdoc.waitForStable();
+
+  // Switch to suggesting mode
+  await superdoc.setDocumentMode('suggesting');
+  await superdoc.waitForStable();
+
+  // Select all and toggle bold ON
+  await superdoc.selectAll();
+  await superdoc.page.evaluate(() => (window as any).editor.commands.toggleBold());
+  await superdoc.waitForStable();
+
+  // Verify a format tracked change was created
+  await superdoc.assertTrackedChangeExists('format');
+
+  // Toggle bold OFF (cancel the suggestion)
+  await superdoc.selectAll();
+  await superdoc.page.evaluate(() => (window as any).editor.commands.toggleBold());
+  await superdoc.waitForStable();
+
+  // No track-format decorations should remain — the cancel was a no-op
+  await expect(superdoc.page.locator('.track-format-dec')).toHaveCount(0);
+
+  // Text should be unchanged
+  await superdoc.assertTextContent('Hello world');
+
+  // "world" should still have italic (it was never touched)
+  await superdoc.assertTextHasMarks('world', ['italic']);
+
+  // Neither word should have bold
+  await superdoc.assertTextLacksMarks('Hello', ['bold']);
+  await superdoc.assertTextLacksMarks('world', ['bold']);
+});