diff --git a/packages/super-editor/src/extensions/track-changes/trackChangesHelpers/addMarkStep.js b/packages/super-editor/src/extensions/track-changes/trackChangesHelpers/addMarkStep.js index 6fd16bebb1..57a5f16e5e 100644 --- a/packages/super-editor/src/extensions/track-changes/trackChangesHelpers/addMarkStep.js +++ b/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; diff --git a/packages/super-editor/src/extensions/track-changes/trackChangesHelpers/removeMarkStep.js b/packages/super-editor/src/extensions/track-changes/trackChangesHelpers/removeMarkStep.js index 0011e05edb..017c7070eb 100644 --- a/packages/super-editor/src/extensions/track-changes/trackChangesHelpers/removeMarkStep.js +++ b/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, diff --git a/packages/super-editor/src/extensions/track-changes/trackChangesHelpers/removeMarkStep.test.js b/packages/super-editor/src/extensions/track-changes/trackChangesHelpers/removeMarkStep.test.js new file mode 100644 index 0000000000..f66cb52a71 --- /dev/null +++ b/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: '

' })); + 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); + }); +}); diff --git a/tests/behavior/tests/comments/cancel-multinode-format-suggestion.spec.ts b/tests/behavior/tests/comments/cancel-multinode-format-suggestion.spec.ts new file mode 100644 index 0000000000..a1378321eb --- /dev/null +++ b/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']); +});