ABCDE
', + user: { name: 'Track Tester', email: 'track@example.com' }, + }); + + try { + const fullTextRange = getFirstTextRange(interactionEditor.state.doc); + interactionEditor.commands.insertTrackedChange({ from: fullTextRange.from, to: fullTextRange.to, text: '' }); + + const selectionRange = getSubstringRange(interactionEditor.state.doc, 'BC'); + interactionEditor.view.dispatch( + interactionEditor.state.tr.setSelection( + TextSelection.create(interactionEditor.state.doc, selectionRange.from, selectionRange.to), + ), + ); + + interactionEditor.commands.rejectTrackedChangeOnSelection(); + expect(interactionEditor.state.doc.textContent).toBe('ABCDE'); + expect(getMarkedText(interactionEditor.state.doc, TrackDeleteMarkName)).toBe('ADE'); + + interactionEditor.commands.undo(); + expect(interactionEditor.state.doc.textContent).toBe('ABCDE'); + expect(getMarkedText(interactionEditor.state.doc, TrackDeleteMarkName)).toBe('ABCDE'); + + interactionEditor.commands.redo(); + expect(interactionEditor.state.doc.textContent).toBe('ABCDE'); + expect(getMarkedText(interactionEditor.state.doc, TrackDeleteMarkName)).toBe('ADE'); + } finally { + interactionEditor.destroy(); + } + }); + it('acceptTrackedChangeById links contiguous insertion segments sharing an id across formatting', () => { const italicMark = schema.marks.italic.create(); const insertionId = 'ins-multi'; diff --git a/packages/super-editor/src/extensions/track-changes/track-changes-toolbar-commands.test.js b/packages/super-editor/src/extensions/track-changes/track-changes-toolbar-commands.test.js index af2dc79e4f..7846b3833e 100644 --- a/packages/super-editor/src/extensions/track-changes/track-changes-toolbar-commands.test.js +++ b/packages/super-editor/src/extensions/track-changes/track-changes-toolbar-commands.test.js @@ -7,23 +7,39 @@ vi.mock('../comment/comments-plugin.js', () => ({ }, })); -describe('Track Changes Toolbar Commands', () => { +vi.mock('./permission-helpers.js', () => ({ + collectTrackedChanges: vi.fn(), +})); + +vi.mock('./trackChangesHelpers/getTrackChanges.js', () => ({ + getTrackChanges: vi.fn(), +})); + +describe('Track Changes Shared Resolution Commands', () => { let commands; let mockState; let mockCommands; let mockCommentsPluginGetState; + let mockCollectTrackedChanges; + let mockGetTrackChanges; beforeEach(async () => { vi.clearAllMocks(); const { CommentsPluginKey } = await import('../comment/comments-plugin.js'); + const { collectTrackedChanges } = await import('./permission-helpers.js'); + const { getTrackChanges } = await import('./trackChangesHelpers/getTrackChanges.js'); mockCommentsPluginGetState = CommentsPluginKey.getState; + mockCollectTrackedChanges = collectTrackedChanges; + mockGetTrackChanges = getTrackChanges; commands = TrackChanges.config.addCommands(); mockCommands = { + acceptTrackedChangesBetween: vi.fn().mockReturnValue(true), acceptTrackedChangeById: vi.fn().mockReturnValue(true), acceptTrackedChangeBySelection: vi.fn().mockReturnValue(true), + rejectTrackedChangesBetween: vi.fn().mockReturnValue(true), rejectTrackedChangeById: vi.fn().mockReturnValue(true), rejectTrackedChangeOnSelection: vi.fn().mockReturnValue(true), }; @@ -31,6 +47,9 @@ describe('Track Changes Toolbar Commands', () => { mockState = { selection: { from: 10, to: 10 }, }; + + mockCollectTrackedChanges.mockReturnValue([]); + mockGetTrackChanges.mockReturnValue([]); }); describe('acceptTrackedChangeFromToolbar', () => { @@ -46,15 +65,39 @@ describe('Track Changes Toolbar Commands', () => { }); const command = commands.acceptTrackedChangeFromToolbar; - const result = command()({ state: mockState, commands: mockCommands }); + const result = command()({ state: mockState, commands: mockCommands, editor: { options: {} } }); expect(result).toBe(true); expect(mockCommands.acceptTrackedChangeById).toHaveBeenCalledWith('tracked-change-123'); + expect(mockCommands.acceptTrackedChangesBetween).not.toHaveBeenCalled(); + expect(mockCommands.acceptTrackedChangeBySelection).not.toHaveBeenCalled(); + }); + + it('uses selection-based accept when text is selected, even with an active tracked change', () => { + mockState.selection = { from: 10, to: 15 }; + mockCollectTrackedChanges.mockReturnValue([{ id: 'tracked-change-456' }]); + + mockCommentsPluginGetState.mockReturnValue({ + activeThreadId: 'tracked-change-456', + trackedChanges: { + 'tracked-change-456': { + deletion: 'tracked-change-456', + }, + }, + }); + + const command = commands.acceptTrackedChangeFromToolbar; + const result = command()({ state: mockState, commands: mockCommands, editor: { options: {} } }); + + expect(result).toBe(true); + expect(mockCommands.acceptTrackedChangesBetween).toHaveBeenCalledWith(10, 15); + expect(mockCommands.acceptTrackedChangeById).not.toHaveBeenCalled(); expect(mockCommands.acceptTrackedChangeBySelection).not.toHaveBeenCalled(); }); - it('uses acceptTrackedChangeById when active tracked change exists (text selected)', () => { + it('uses acceptTrackedChangeById when a stale expanded selection does not touch the active tracked change', () => { mockState.selection = { from: 10, to: 15 }; + mockCollectTrackedChanges.mockReturnValue([{ id: 'different-change' }]); mockCommentsPluginGetState.mockReturnValue({ activeThreadId: 'tracked-change-456', @@ -66,10 +109,11 @@ describe('Track Changes Toolbar Commands', () => { }); const command = commands.acceptTrackedChangeFromToolbar; - const result = command()({ state: mockState, commands: mockCommands }); + const result = command()({ state: mockState, commands: mockCommands, editor: { options: {} } }); expect(result).toBe(true); expect(mockCommands.acceptTrackedChangeById).toHaveBeenCalledWith('tracked-change-456'); + expect(mockCommands.acceptTrackedChangesBetween).not.toHaveBeenCalled(); expect(mockCommands.acceptTrackedChangeBySelection).not.toHaveBeenCalled(); }); @@ -80,10 +124,11 @@ describe('Track Changes Toolbar Commands', () => { }); const command = commands.acceptTrackedChangeFromToolbar; - const result = command()({ state: mockState, commands: mockCommands }); + const result = command()({ state: mockState, commands: mockCommands, editor: { options: {} } }); expect(result).toBe(true); expect(mockCommands.acceptTrackedChangeBySelection).toHaveBeenCalled(); + expect(mockCommands.acceptTrackedChangesBetween).not.toHaveBeenCalled(); expect(mockCommands.acceptTrackedChangeById).not.toHaveBeenCalled(); }); @@ -95,10 +140,11 @@ describe('Track Changes Toolbar Commands', () => { }); const command = commands.acceptTrackedChangeFromToolbar; - const result = command()({ state: mockState, commands: mockCommands }); + const result = command()({ state: mockState, commands: mockCommands, editor: { options: {} } }); expect(result).toBe(true); expect(mockCommands.acceptTrackedChangeBySelection).toHaveBeenCalled(); + expect(mockCommands.acceptTrackedChangesBetween).not.toHaveBeenCalled(); expect(mockCommands.acceptTrackedChangeById).not.toHaveBeenCalled(); }); @@ -106,11 +152,62 @@ describe('Track Changes Toolbar Commands', () => { mockCommentsPluginGetState.mockReturnValue(undefined); const command = commands.acceptTrackedChangeFromToolbar; - const result = command()({ state: mockState, commands: mockCommands }); + const result = command()({ state: mockState, commands: mockCommands, editor: { options: {} } }); expect(result).toBe(true); expect(mockCommands.acceptTrackedChangeBySelection).toHaveBeenCalled(); + expect(mockCommands.acceptTrackedChangesBetween).not.toHaveBeenCalled(); + expect(mockCommands.acceptTrackedChangeById).not.toHaveBeenCalled(); + }); + + it('uses preserved toolbar selection when the live selection has collapsed', () => { + mockCollectTrackedChanges.mockReturnValue([{ id: 'tracked-change-456' }]); + + mockCommentsPluginGetState.mockReturnValue({ + activeThreadId: 'tracked-change-456', + trackedChanges: { + 'tracked-change-456': { + deletion: 'tracked-change-456', + }, + }, + }); + + const command = commands.acceptTrackedChangeFromToolbar; + const result = command()({ + state: mockState, + commands: mockCommands, + editor: { options: { lastSelection: { from: 12, to: 16 } } }, + }); + + expect(result).toBe(true); + expect(mockCollectTrackedChanges).toHaveBeenCalledWith({ state: mockState, from: 12, to: 16 }); + expect(mockCommands.acceptTrackedChangesBetween).toHaveBeenCalledWith(12, 16); expect(mockCommands.acceptTrackedChangeById).not.toHaveBeenCalled(); + expect(mockCommands.acceptTrackedChangeBySelection).not.toHaveBeenCalled(); + }); + + it('ignores malformed preserved selections and falls back to by-id resolution', () => { + mockCommentsPluginGetState.mockReturnValue({ + activeThreadId: 'tracked-change-456', + trackedChanges: { + 'tracked-change-456': { + deletion: 'tracked-change-456', + }, + }, + }); + + const command = commands.acceptTrackedChangeFromToolbar; + const result = command()({ + state: mockState, + commands: mockCommands, + editor: { options: { lastSelection: { from: undefined, to: 16 } } }, + }); + + expect(result).toBe(true); + expect(mockCollectTrackedChanges).not.toHaveBeenCalled(); + expect(mockCommands.acceptTrackedChangeById).toHaveBeenCalledWith('tracked-change-456'); + expect(mockCommands.acceptTrackedChangesBetween).not.toHaveBeenCalled(); + expect(mockCommands.acceptTrackedChangeBySelection).not.toHaveBeenCalled(); }); }); @@ -127,17 +224,18 @@ describe('Track Changes Toolbar Commands', () => { }); const command = commands.rejectTrackedChangeFromToolbar; - const result = command()({ state: mockState, commands: mockCommands }); + const result = command()({ state: mockState, commands: mockCommands, editor: { options: {} } }); expect(result).toBe(true); expect(mockCommands.rejectTrackedChangeById).toHaveBeenCalledWith('tracked-change-789'); + expect(mockCommands.rejectTrackedChangesBetween).not.toHaveBeenCalled(); expect(mockCommands.rejectTrackedChangeOnSelection).not.toHaveBeenCalled(); }); - it('uses rejectTrackedChangeById when active tracked change exists (text selected)', () => { + it('uses selection-based reject when text is selected, even with an active tracked change', () => { mockState.selection = { from: 20, to: 25 }; + mockCollectTrackedChanges.mockReturnValue([{ id: 'tracked-change-999' }]); - // Mock CommentsPlugin state with active tracked change mockCommentsPluginGetState.mockReturnValue({ activeThreadId: 'tracked-change-999', trackedChanges: { @@ -148,10 +246,11 @@ describe('Track Changes Toolbar Commands', () => { }); const command = commands.rejectTrackedChangeFromToolbar; - const result = command()({ state: mockState, commands: mockCommands }); + const result = command()({ state: mockState, commands: mockCommands, editor: { options: {} } }); expect(result).toBe(true); - expect(mockCommands.rejectTrackedChangeById).toHaveBeenCalledWith('tracked-change-999'); + expect(mockCommands.rejectTrackedChangesBetween).toHaveBeenCalledWith(20, 25); + expect(mockCommands.rejectTrackedChangeById).not.toHaveBeenCalled(); expect(mockCommands.rejectTrackedChangeOnSelection).not.toHaveBeenCalled(); }); @@ -162,10 +261,11 @@ describe('Track Changes Toolbar Commands', () => { }); const command = commands.rejectTrackedChangeFromToolbar; - const result = command()({ state: mockState, commands: mockCommands }); + const result = command()({ state: mockState, commands: mockCommands, editor: { options: {} } }); expect(result).toBe(true); expect(mockCommands.rejectTrackedChangeOnSelection).toHaveBeenCalled(); + expect(mockCommands.rejectTrackedChangesBetween).not.toHaveBeenCalled(); expect(mockCommands.rejectTrackedChangeById).not.toHaveBeenCalled(); }); @@ -176,11 +276,220 @@ describe('Track Changes Toolbar Commands', () => { }); const command = commands.rejectTrackedChangeFromToolbar; - const result = command()({ state: mockState, commands: mockCommands }); + const result = command()({ state: mockState, commands: mockCommands, editor: { options: {} } }); expect(result).toBe(true); expect(mockCommands.rejectTrackedChangeOnSelection).toHaveBeenCalled(); + expect(mockCommands.rejectTrackedChangesBetween).not.toHaveBeenCalled(); expect(mockCommands.rejectTrackedChangeById).not.toHaveBeenCalled(); }); + + it('uses rejectTrackedChangeById when a stale expanded selection does not touch the active tracked change', () => { + mockState.selection = { from: 20, to: 25 }; + mockCollectTrackedChanges.mockReturnValue([{ id: 'different-change' }]); + + mockCommentsPluginGetState.mockReturnValue({ + activeThreadId: 'tracked-change-999', + trackedChanges: { + 'tracked-change-999': { + insertion: 'tracked-change-999', + }, + }, + }); + + const command = commands.rejectTrackedChangeFromToolbar; + const result = command()({ state: mockState, commands: mockCommands, editor: { options: {} } }); + + expect(result).toBe(true); + expect(mockCommands.rejectTrackedChangeById).toHaveBeenCalledWith('tracked-change-999'); + expect(mockCommands.rejectTrackedChangesBetween).not.toHaveBeenCalled(); + expect(mockCommands.rejectTrackedChangeOnSelection).not.toHaveBeenCalled(); + }); + + it('uses preserved toolbar selection when the live selection has collapsed', () => { + mockCollectTrackedChanges.mockReturnValue([{ id: 'tracked-change-999' }]); + + mockCommentsPluginGetState.mockReturnValue({ + activeThreadId: 'tracked-change-999', + trackedChanges: { + 'tracked-change-999': { + insertion: 'tracked-change-999', + }, + }, + }); + + const command = commands.rejectTrackedChangeFromToolbar; + const result = command()({ + state: mockState, + commands: mockCommands, + editor: { options: { preservedSelection: { from: 21, to: 24 } } }, + }); + + expect(result).toBe(true); + expect(mockCollectTrackedChanges).toHaveBeenCalledWith({ state: mockState, from: 21, to: 24 }); + expect(mockCommands.rejectTrackedChangesBetween).toHaveBeenCalledWith(21, 24); + expect(mockCommands.rejectTrackedChangeById).not.toHaveBeenCalled(); + expect(mockCommands.rejectTrackedChangeOnSelection).not.toHaveBeenCalled(); + }); + }); + + describe('acceptTrackedChangeFromContextMenu', () => { + it('uses the explicit context-menu selection even when the live selection has collapsed', () => { + mockCollectTrackedChanges.mockReturnValue([{ id: 'tracked-change-456' }]); + + mockCommentsPluginGetState.mockReturnValue({ + activeThreadId: 'different-active-thread', + trackedChanges: { + 'tracked-change-456': { + insertion: 'tracked-change-456', + }, + }, + }); + + const command = commands.acceptTrackedChangeFromContextMenu; + const result = command({ from: 12, to: 16, trackedChangeId: 'tracked-change-456' })({ + state: mockState, + commands: mockCommands, + editor: { options: {} }, + }); + + expect(result).toBe(true); + expect(mockCollectTrackedChanges).toHaveBeenCalledWith({ state: mockState, from: 12, to: 16 }); + expect(mockCommands.acceptTrackedChangesBetween).toHaveBeenCalledWith(12, 16); + expect(mockCommands.acceptTrackedChangeById).not.toHaveBeenCalled(); + expect(mockCommands.acceptTrackedChangeBySelection).not.toHaveBeenCalled(); + }); + + it('falls back to by-id resolution when no explicit range is provided', () => { + mockCommentsPluginGetState.mockReturnValue({ + activeThreadId: null, + trackedChanges: { + 'tracked-change-456': { + insertion: 'tracked-change-456', + }, + }, + }); + + const command = commands.acceptTrackedChangeFromContextMenu; + const result = command({ trackedChangeId: 'tracked-change-456' })({ + state: mockState, + commands: mockCommands, + editor: { options: {} }, + }); + + expect(result).toBe(true); + expect(mockCommands.acceptTrackedChangeById).toHaveBeenCalledWith('tracked-change-456'); + expect(mockCommands.acceptTrackedChangesBetween).not.toHaveBeenCalled(); + expect(mockCommands.acceptTrackedChangeBySelection).not.toHaveBeenCalled(); + }); + + it('falls back to by-id resolution when the comment cache is empty but the document still has the tracked change', () => { + mockCommentsPluginGetState.mockReturnValue({ + activeThreadId: null, + trackedChanges: {}, + }); + mockGetTrackChanges.mockReturnValue([ + { + from: 12, + to: 16, + mark: { + type: { name: 'trackInsert' }, + attrs: { id: 'tracked-change-456' }, + }, + }, + ]); + + const command = commands.acceptTrackedChangeFromContextMenu; + const result = command({ trackedChangeId: 'tracked-change-456' })({ + state: mockState, + commands: mockCommands, + editor: { options: {} }, + }); + + expect(result).toBe(true); + expect(mockCommands.acceptTrackedChangeById).toHaveBeenCalledWith('tracked-change-456'); + expect(mockCommands.acceptTrackedChangesBetween).not.toHaveBeenCalled(); + expect(mockCommands.acceptTrackedChangeBySelection).not.toHaveBeenCalled(); + }); + }); + + describe('rejectTrackedChangeFromContextMenu', () => { + it('uses the explicit context-menu selection even when the live selection has collapsed', () => { + mockCollectTrackedChanges.mockReturnValue([{ id: 'tracked-change-999' }]); + + mockCommentsPluginGetState.mockReturnValue({ + activeThreadId: 'different-active-thread', + trackedChanges: { + 'tracked-change-999': { + insertion: 'tracked-change-999', + }, + }, + }); + + const command = commands.rejectTrackedChangeFromContextMenu; + const result = command({ from: 21, to: 24, trackedChangeId: 'tracked-change-999' })({ + state: mockState, + commands: mockCommands, + editor: { options: {} }, + }); + + expect(result).toBe(true); + expect(mockCollectTrackedChanges).toHaveBeenCalledWith({ state: mockState, from: 21, to: 24 }); + expect(mockCommands.rejectTrackedChangesBetween).toHaveBeenCalledWith(21, 24); + expect(mockCommands.rejectTrackedChangeById).not.toHaveBeenCalled(); + expect(mockCommands.rejectTrackedChangeOnSelection).not.toHaveBeenCalled(); + }); + + it('falls back to by-id resolution when no explicit range is provided', () => { + mockCommentsPluginGetState.mockReturnValue({ + activeThreadId: null, + trackedChanges: { + 'tracked-change-999': { + insertion: 'tracked-change-999', + }, + }, + }); + + const command = commands.rejectTrackedChangeFromContextMenu; + const result = command({ trackedChangeId: 'tracked-change-999' })({ + state: mockState, + commands: mockCommands, + editor: { options: {} }, + }); + + expect(result).toBe(true); + expect(mockCommands.rejectTrackedChangeById).toHaveBeenCalledWith('tracked-change-999'); + expect(mockCommands.rejectTrackedChangesBetween).not.toHaveBeenCalled(); + expect(mockCommands.rejectTrackedChangeOnSelection).not.toHaveBeenCalled(); + }); + + it('falls back to by-id resolution when the comment cache is empty but the document still has the tracked change', () => { + mockCommentsPluginGetState.mockReturnValue({ + activeThreadId: null, + trackedChanges: {}, + }); + mockGetTrackChanges.mockReturnValue([ + { + from: 21, + to: 24, + mark: { + type: { name: 'trackDelete' }, + attrs: { id: 'tracked-change-999' }, + }, + }, + ]); + + const command = commands.rejectTrackedChangeFromContextMenu; + const result = command({ trackedChangeId: 'tracked-change-999' })({ + state: mockState, + commands: mockCommands, + editor: { options: {} }, + }); + + expect(result).toBe(true); + expect(mockCommands.rejectTrackedChangeById).toHaveBeenCalledWith('tracked-change-999'); + expect(mockCommands.rejectTrackedChangesBetween).not.toHaveBeenCalled(); + expect(mockCommands.rejectTrackedChangeOnSelection).not.toHaveBeenCalled(); + }); }); }); diff --git a/packages/super-editor/src/extensions/track-changes/track-changes.js b/packages/super-editor/src/extensions/track-changes/track-changes.js index becdf83d24..a946f65a57 100644 --- a/packages/super-editor/src/extensions/track-changes/track-changes.js +++ b/packages/super-editor/src/extensions/track-changes/track-changes.js @@ -8,8 +8,9 @@ import { getTrackChanges } from './trackChangesHelpers/getTrackChanges.js'; import { markDeletion } from './trackChangesHelpers/markDeletion.js'; import { markInsertion } from './trackChangesHelpers/markInsertion.js'; import { collectTrackedChanges, isTrackedChangeActionAllowed } from './permission-helpers.js'; -import { CommentsPluginKey } from '../comment/comments-plugin.js'; +import { CommentsPluginKey, createOrUpdateTrackedChangeComment } from '../comment/comments-plugin.js'; import { findMarkInRangeBySnapshot } from './trackChangesHelpers/markSnapshotHelpers.js'; +import { hasExpandedSelection } from '@utils/selectionUtils.js'; export const TrackChanges = Extension.create({ name: 'trackChanges', @@ -30,157 +31,101 @@ export const TrackChanges = Extension.create({ // tr.setMeta('acceptReject', true); tr.setMeta('inputType', 'acceptReject'); - + const touchedChangeIds = new Set(); const map = new Mapping(); doc.nodesBetween(from, to, (node, pos) => { - if (node.marks && node.marks.find((mark) => mark.type.name === TrackDeleteMarkName)) { - const deletionStep = new ReplaceStep( - map.map(Math.max(pos, from)), - map.map(Math.min(pos + node.nodeSize, to)), - Slice.empty, - ); + const trackedMark = getTrackedMark(node); + if (!trackedMark) return; + + const mappedFrom = map.map(Math.max(pos, from)); + const mappedTo = map.map(Math.min(pos + node.nodeSize, to)); + if (mappedFrom >= mappedTo) return; + + if (trackedMark.attrs?.id) touchedChangeIds.add(trackedMark.attrs.id); + if (trackedMark.type.name === TrackDeleteMarkName) { + const deletionStep = new ReplaceStep(mappedFrom, mappedTo, Slice.empty); tr.step(deletionStep); map.appendMap(deletionStep.getMap()); - } else if (node.marks && node.marks.find((mark) => mark.type.name === TrackInsertMarkName)) { - const insertionMark = node.marks.find((mark) => mark.type.name === TrackInsertMarkName); - - tr.step( - new RemoveMarkStep( - map.map(Math.max(pos, from)), - map.map(Math.min(pos + node.nodeSize, to)), - insertionMark, - ), - ); - } else if (node.marks && node.marks.find((mark) => mark.type.name === TrackFormatMarkName)) { - const formatChangeMark = node.marks.find((mark) => mark.type.name === TrackFormatMarkName); - - tr.step( - new RemoveMarkStep( - map.map(Math.max(pos, from)), - map.map(Math.min(pos + node.nodeSize, to)), - formatChangeMark, - ), - ); + return; } - }); - if (tr.steps.length) { - dispatch(tr); - } + tr.step(new RemoveMarkStep(mappedFrom, mappedTo, trackedMark)); + }); - return true; + return dispatchTrackedChangeResolution({ + state, + tr, + dispatch, + editor, + touchedChangeIds, + }); }, rejectTrackedChangesBetween: (from, to) => ({ state, dispatch, editor }) => { - const trackedChangesInSelection = collectTrackedChanges({ state, from, to }); - const trackedChangesById = getTrackedChangesByTouchedIds(state, trackedChangesInSelection); - const trackedChangesWithoutId = trackedChangesInSelection.filter(({ mark }) => !mark?.attrs?.id); - const trackedChanges = dedupeTrackedChangeRanges([...trackedChangesById, ...trackedChangesWithoutId]); + const trackedChanges = collectTrackedChanges({ state, from, to }); if (!isTrackedChangeActionAllowed({ editor, action: 'reject', trackedChanges })) return false; const { tr, doc } = state; - // Keep the IDs rejected in this transaction so the comments layer can - // resolve/hide the corresponding tracked-change bubbles in one path. - const rejectedChangeIds = new Set(); - - // tr.setMeta('acceptReject', true); + const touchedChangeIds = new Set(); tr.setMeta('inputType', 'acceptReject'); const map = new Mapping(); - trackedChanges.forEach(({ from: rangeFrom, to: rangeTo }) => { - doc.nodesBetween(rangeFrom, rangeTo, (node, pos) => { - if (node.marks && node.marks.find((mark) => mark.type.name === TrackDeleteMarkName)) { - const deletionMark = node.marks.find((mark) => mark.type.name === TrackDeleteMarkName); - if (deletionMark?.attrs?.id) rejectedChangeIds.add(deletionMark.attrs.id); - - tr.step( - new RemoveMarkStep( - map.map(Math.max(pos, rangeFrom)), - map.map(Math.min(pos + node.nodeSize, rangeTo)), - deletionMark, - ), - ); - } else if (node.marks && node.marks.find((mark) => mark.type.name === TrackInsertMarkName)) { - const insertionMark = node.marks.find((mark) => mark.type.name === TrackInsertMarkName); - if (insertionMark?.attrs?.id) rejectedChangeIds.add(insertionMark.attrs.id); - - const deletionStep = new ReplaceStep( - map.map(Math.max(pos, rangeFrom)), - map.map(Math.min(pos + node.nodeSize, rangeTo)), - Slice.empty, - ); - - tr.step(deletionStep); - map.appendMap(deletionStep.getMap()); - } else if (node.marks && node.marks.find((mark) => mark.type.name === TrackFormatMarkName)) { - const formatChangeMark = node.marks.find((mark) => mark.type.name === TrackFormatMarkName); - if (formatChangeMark?.attrs?.id) rejectedChangeIds.add(formatChangeMark.attrs.id); - - formatChangeMark.attrs.before.forEach((oldMark) => { - tr.step( - new AddMarkStep( - map.map(Math.max(pos, rangeFrom)), - map.map(Math.min(pos + node.nodeSize, rangeTo)), - state.schema.marks[oldMark.type].create(oldMark.attrs), - ), - ); - }); - - formatChangeMark.attrs.after.forEach((newMark) => { - const mappedFrom = map.map(Math.max(pos, rangeFrom)); - const mappedTo = map.map(Math.min(pos + node.nodeSize, rangeTo)); - const liveMark = findMarkInRangeBySnapshot({ - doc: tr.doc, - from: mappedFrom, - to: mappedTo, - snapshot: newMark, - }); - - if (!liveMark) { - return; - } + doc.nodesBetween(from, to, (node, pos) => { + const trackedMark = getTrackedMark(node); + if (!trackedMark) return; - tr.step(new RemoveMarkStep(mappedFrom, mappedTo, liveMark)); - }); + const mappedFrom = map.map(Math.max(pos, from)); + const mappedTo = map.map(Math.min(pos + node.nodeSize, to)); + if (mappedFrom >= mappedTo) return; - tr.step( - new RemoveMarkStep( - map.map(Math.max(pos, rangeFrom)), - map.map(Math.min(pos + node.nodeSize, rangeTo)), - formatChangeMark, - ), - ); - } + if (trackedMark.attrs?.id) touchedChangeIds.add(trackedMark.attrs.id); + + if (trackedMark.type.name === TrackDeleteMarkName) { + tr.step(new RemoveMarkStep(mappedFrom, mappedTo, trackedMark)); + return; + } + + if (trackedMark.type.name === TrackInsertMarkName) { + const deletionStep = new ReplaceStep(mappedFrom, mappedTo, Slice.empty); + tr.step(deletionStep); + map.appendMap(deletionStep.getMap()); + return; + } + + trackedMark.attrs.before.forEach((oldMark) => { + tr.step(new AddMarkStep(mappedFrom, mappedTo, state.schema.marks[oldMark.type].create(oldMark.attrs))); }); - }); - if (tr.steps.length) { - dispatch(tr); - - if (editor?.emit && rejectedChangeIds.size) { - const resolvedByEmail = editor.options?.user?.email; - const resolvedByName = editor.options?.user?.name; - - // Bubble reject-by-selection/reject-all through the same - // tracked-change comment resolution channel used by bubble actions. - rejectedChangeIds.forEach((changeId) => { - editor.emit('commentsUpdate', { - type: 'trackedChange', - event: 'resolve', - changeId, - resolvedByEmail, - resolvedByName, - }); + trackedMark.attrs.after.forEach((newMark) => { + const liveMark = findMarkInRangeBySnapshot({ + doc: tr.doc, + from: mappedFrom, + to: mappedTo, + snapshot: newMark, }); - } - } - return true; + if (!liveMark) { + return; + } + + tr.step(new RemoveMarkStep(mappedFrom, mappedTo, liveMark)); + }); + + tr.step(new RemoveMarkStep(mappedFrom, mappedTo, trackedMark)); + }); + + return dispatchTrackedChangeResolution({ + state, + tr, + dispatch, + editor, + touchedChangeIds, + }); }, acceptTrackedChange: @@ -199,15 +144,36 @@ export const TrackChanges = Extension.create({ acceptTrackedChangeFromToolbar: () => - ({ state, commands }) => { - const commentsPluginState = CommentsPluginKey.getState(state); - const activeThreadId = commentsPluginState?.activeThreadId; + ({ state, commands, editor }) => { + return resolveTrackedChangeAction({ + action: 'accept', + state, + commands, + editor, + ...getTrackedChangeResolutionContext({ + state, + trackedChangeId: CommentsPluginKey.getState(state)?.activeThreadId, + }), + }); + }, - if (activeThreadId && commentsPluginState?.trackedChanges?.[activeThreadId]) { - return commands.acceptTrackedChangeById(activeThreadId); - } else { - return commands.acceptTrackedChangeBySelection(); - } + acceptTrackedChangeFromContextMenu: + ({ from, to, trackedChangeId = null } = {}) => + ({ state, commands, editor }) => { + return resolveTrackedChangeAction({ + action: 'accept', + state, + commands, + editor, + selection: + Number.isFinite(from) && Number.isFinite(to) + ? { + from, + to, + } + : null, + ...getTrackedChangeResolutionContext({ state, trackedChangeId }), + }); }, acceptTrackedChangeById: @@ -262,15 +228,36 @@ export const TrackChanges = Extension.create({ rejectTrackedChangeFromToolbar: () => - ({ state, commands }) => { - const commentsPluginState = CommentsPluginKey.getState(state); - const activeThreadId = commentsPluginState?.activeThreadId; + ({ state, commands, editor }) => { + return resolveTrackedChangeAction({ + action: 'reject', + state, + commands, + editor, + ...getTrackedChangeResolutionContext({ + state, + trackedChangeId: CommentsPluginKey.getState(state)?.activeThreadId, + }), + }); + }, - if (activeThreadId && commentsPluginState?.trackedChanges?.[activeThreadId]) { - return commands.rejectTrackedChangeById(activeThreadId); - } else { - return commands.rejectTrackedChangeOnSelection(); - } + rejectTrackedChangeFromContextMenu: + ({ from, to, trackedChangeId = null } = {}) => + ({ state, commands, editor }) => { + return resolveTrackedChangeAction({ + action: 'reject', + state, + commands, + editor, + selection: + Number.isFinite(from) && Number.isFinite(to) + ? { + from, + to, + } + : null, + ...getTrackedChangeResolutionContext({ state, trackedChangeId }), + }); }, rejectAllTrackedChanges: @@ -508,46 +495,168 @@ export const TrackChanges = Extension.create({ }, }); -// For reference. -// const trackChangesCallback = (action, acceptedChanges, revertedChanges, editor) => { -// const id = acceptedChanges.modifiers[0]?.id || revertedChanges.modifiers[0]?.id; -// if (action === 'accept') { -// editor.emit('trackedChangesUpdate', { action, id }); -// } else { -// editor.emit('trackedChangesUpdate', { action, id }); -// } -// }; - -const dedupeTrackedChangeRanges = (changes = []) => { - const byKey = new Map(); - changes.forEach((change) => { - if (!change || typeof change.from !== 'number' || typeof change.to !== 'number') { - return; - } +const TRACKED_CHANGE_MARKS = [TrackDeleteMarkName, TrackInsertMarkName, TrackFormatMarkName]; + +const getTrackedMark = (node) => node?.marks?.find((mark) => TRACKED_CHANGE_MARKS.includes(mark.type.name)) ?? null; + +const getTrackedChangeActionSelection = ({ state, editor }) => { + const currentSelection = state?.selection; + if (hasExpandedSelection(currentSelection)) { + return currentSelection; + } + + const preservedSelection = editor?.options?.preservedSelection ?? editor?.options?.lastSelection; + if (hasExpandedSelection(preservedSelection)) { + return preservedSelection; + } + + return currentSelection; +}; + +const getTrackedChangeResolutionContext = ({ state, trackedChangeId = null }) => { + const commentsPluginState = CommentsPluginKey.getState(state); + const resolvedTrackedChangeId = trackedChangeId ?? commentsPluginState?.activeThreadId ?? null; + const hasTrackedChangeInCache = Boolean( + resolvedTrackedChangeId && commentsPluginState?.trackedChanges?.[resolvedTrackedChangeId], + ); + const hasTrackedChangeInDocument = Boolean( + resolvedTrackedChangeId && getChangesByIdToResolve(state, resolvedTrackedChangeId)?.length, + ); + + return { + trackedChangeId: resolvedTrackedChangeId, + hasKnownTrackedChangeId: hasTrackedChangeInCache || hasTrackedChangeInDocument, + }; +}; + +const selectionTouchesTrackedChange = ({ state, trackedChangeId, selection = state?.selection }) => { + if (!selection) { + return false; + } + + if (!trackedChangeId) { + return ( + collectTrackedChanges({ + state, + from: selection.from, + to: selection.to, + }).length > 0 + ); + } - const type = change.mark?.type?.name || ''; - const id = change.mark?.attrs?.id || ''; - const key = `${change.from}:${change.to}:${type}:${id}`; - if (!byKey.has(key)) { - byKey.set(key, change); + return collectTrackedChanges({ + state, + from: selection.from, + to: selection.to, + }).some((change) => change.id === trackedChangeId); +}; + +const resolveTrackedChangeAction = ({ + action, + state, + commands, + editor, + trackedChangeId = null, + hasKnownTrackedChangeId = false, + selection = null, +}) => { + const targetSelection = selection ?? getTrackedChangeActionSelection({ state, editor }); + const betweenCommand = + action === 'accept' ? commands.acceptTrackedChangesBetween : commands.rejectTrackedChangesBetween; + const byIdCommand = action === 'accept' ? commands.acceptTrackedChangeById : commands.rejectTrackedChangeById; + const selectionCommand = + action === 'accept' ? commands.acceptTrackedChangeBySelection : commands.rejectTrackedChangeOnSelection; + const shouldUseSelection = + hasExpandedSelection(targetSelection) && + selectionTouchesTrackedChange({ + state, + trackedChangeId, + selection: targetSelection, + }); + + // An explicit text selection takes precedence over the active bubble/thread + // so partial accept/reject resolves exactly what the user highlighted. + if (shouldUseSelection) { + return betweenCommand(targetSelection.from, targetSelection.to); + } + + if (trackedChangeId && hasKnownTrackedChangeId) { + return byIdCommand(trackedChangeId); + } + + return hasExpandedSelection(targetSelection) + ? betweenCommand(targetSelection.from, targetSelection.to) + : selectionCommand(); +}; + +const collectRemainingMarksByType = (trackedChanges = []) => ({ + insertedMark: trackedChanges.find(({ mark }) => mark.type.name === TrackInsertMarkName)?.mark ?? null, + deletionMark: trackedChanges.find(({ mark }) => mark.type.name === TrackDeleteMarkName)?.mark ?? null, + formatMark: trackedChanges.find(({ mark }) => mark.type.name === TrackFormatMarkName)?.mark ?? null, +}); + +const emitTrackedChangeCommentLifecycle = ({ editor, nextState, touchedChangeIds }) => { + if (!editor?.emit || !touchedChangeIds?.size) { + return; + } + + const resolvedByEmail = editor.options?.user?.email; + const resolvedByName = editor.options?.user?.name; + + touchedChangeIds.forEach((changeId) => { + const remainingTrackedChanges = getTrackChanges(nextState, changeId); + + // Partial resolution keeps the tracked-change thread alive with updated text; + // full resolution emits the normal resolve event so the bubble can disappear. + if (!remainingTrackedChanges.length) { + editor.emit('commentsUpdate', { + type: 'trackedChange', + event: 'resolve', + changeId, + resolvedByEmail, + resolvedByName, + }); + return; } - }); - return Array.from(byKey.values()).sort((left, right) => { - if (left.from !== right.from) { - return left.from - right.from; + const marks = collectRemainingMarksByType(remainingTrackedChanges); + const updatePayload = createOrUpdateTrackedChangeComment({ + event: 'update', + marks, + deletionNodes: [], + nodes: [], + newEditorState: nextState, + documentId: editor.options?.documentId, + trackedChangesForId: remainingTrackedChanges, + }); + + if (updatePayload) { + editor.emit('commentsUpdate', updatePayload); } - return left.to - right.to; }); }; -const getTrackedChangesByTouchedIds = (state, trackedChanges = []) => { - const touchedIds = new Set(trackedChanges.map(({ mark }) => mark?.attrs?.id).filter(Boolean)); - if (!touchedIds.size) { - return trackedChanges; +const dispatchTrackedChangeResolution = ({ state, tr, dispatch, editor, touchedChangeIds }) => { + if (!tr.steps.length) { + return true; + } + + // Apply tr locally to get nextState for comment lifecycle; dispatch(tr) updates the editor afterward. + const nextState = state.apply(tr); + + if (dispatch) { + dispatch(tr); + } + + if (dispatch && touchedChangeIds?.size) { + emitTrackedChangeCommentLifecycle({ + editor, + nextState, + touchedChangeIds, + }); } - return Array.from(touchedIds).flatMap((id) => getChangesByIdToResolve(state, id) || []); + return true; }; const getChangesByIdToResolve = (state, id) => { diff --git a/packages/super-editor/src/extensions/types/track-changes-commands.ts b/packages/super-editor/src/extensions/types/track-changes-commands.ts index d28bb2be21..e42feb7603 100644 --- a/packages/super-editor/src/extensions/types/track-changes-commands.ts +++ b/packages/super-editor/src/extensions/types/track-changes-commands.ts @@ -24,6 +24,16 @@ export type TrackedChangeOptions = { trackedChange: TrackedChange; }; +/** Explicit context-menu resolution options */ +export type TrackedChangeResolutionOptions = { + /** Optional explicit range to resolve */ + from?: number; + /** Optional explicit range to resolve */ + to?: number; + /** Optional tracked change ID for by-id fallback */ + trackedChangeId?: string | null; +}; + /** Options for programmatic tracked change insertion */ export type InsertTrackedChangeOptions = { /** Start position (defaults to selection start) */ @@ -73,6 +83,11 @@ export interface TrackChangesCommands { */ acceptTrackedChangeFromToolbar: () => boolean; + /** + * Accept tracked change from context menu with optional explicit range + */ + acceptTrackedChangeFromContextMenu: (options?: TrackedChangeResolutionOptions) => boolean; + /** * Accept tracked change by its ID * @param id - The tracked change ID @@ -111,6 +126,11 @@ export interface TrackChangesCommands { */ rejectTrackedChangeFromToolbar: () => boolean; + /** + * Reject tracked change from context menu with optional explicit range + */ + rejectTrackedChangeFromContextMenu: (options?: TrackedChangeResolutionOptions) => boolean; + /** * Reject tracked change by its ID * @param id - The tracked change ID diff --git a/packages/super-editor/src/utils/selectionUtils.js b/packages/super-editor/src/utils/selectionUtils.js new file mode 100644 index 0000000000..c2d80d3817 --- /dev/null +++ b/packages/super-editor/src/utils/selectionUtils.js @@ -0,0 +1,9 @@ +/** + * Returns true when a selection-like object represents a non-collapsed range. + * + * @param {{ from?: number, to?: number } | null | undefined} selection + * @returns {boolean} + */ +export const hasExpandedSelection = (selection) => { + return Number.isFinite(selection?.from) && Number.isFinite(selection?.to) && selection.from !== selection.to; +}; diff --git a/packages/super-editor/src/utils/selectionUtils.test.js b/packages/super-editor/src/utils/selectionUtils.test.js new file mode 100644 index 0000000000..196fd0b5cb --- /dev/null +++ b/packages/super-editor/src/utils/selectionUtils.test.js @@ -0,0 +1,19 @@ +import { describe, expect, it } from 'vitest'; + +import { hasExpandedSelection } from './selectionUtils.js'; + +describe('selectionUtils', () => { + it('returns true for non-collapsed numeric ranges', () => { + expect(hasExpandedSelection({ from: 10, to: 15 })).toBe(true); + }); + + it('returns false for collapsed ranges', () => { + expect(hasExpandedSelection({ from: 10, to: 10 })).toBe(false); + }); + + it('returns false for missing or non-numeric boundaries', () => { + expect(hasExpandedSelection(null)).toBe(false); + expect(hasExpandedSelection({ from: undefined, to: 12 })).toBe(false); + expect(hasExpandedSelection({ from: 12, to: Number.NaN })).toBe(false); + }); +}); diff --git a/packages/superdoc/src/stores/comments-store.js b/packages/superdoc/src/stores/comments-store.js index 577a613494..dc12e32846 100644 --- a/packages/superdoc/src/stores/comments-store.js +++ b/packages/superdoc/src/stores/comments-store.js @@ -337,8 +337,11 @@ export const useCommentsStore = defineStore('comments', () => { const existing = commentsList.value.find((c) => c.commentId === changeId); if (existing) { // Already exists (e.g. created during batch import) — update instead of duplicating - existing.trackedChangeText = trackedChangeText; - if (deletedText) existing.deletedText = deletedText; + // Partial resolution can turn a replacement into insert-only/delete-only, so + // clear fields explicitly when the updated payload no longer includes them. + existing.trackedChangeText = trackedChangeText ?? null; + existing.trackedChangeType = trackedChangeType ?? null; + existing.deletedText = deletedText ?? null; const emitData = { type: COMMENT_EVENTS.UPDATE, @@ -355,11 +358,11 @@ export const useCommentsStore = defineStore('comments', () => { const existingTrackedChange = commentsList.value.find((comment) => comment.commentId === changeId); if (!existingTrackedChange) return; - existingTrackedChange.trackedChangeText = trackedChangeText; - - if (deletedText) { - existingTrackedChange.deletedText = deletedText; - } + // Partial resolution can turn a replacement into insert-only/delete-only, so + // clear fields explicitly when the updated payload no longer includes them. + existingTrackedChange.trackedChangeText = trackedChangeText ?? null; + existingTrackedChange.trackedChangeType = trackedChangeType ?? null; + existingTrackedChange.deletedText = deletedText ?? null; const emitData = { type: COMMENT_EVENTS.UPDATE, diff --git a/packages/superdoc/src/stores/comments-store.test.js b/packages/superdoc/src/stores/comments-store.test.js index c7e8b23425..c87d8399ad 100644 --- a/packages/superdoc/src/stores/comments-store.test.js +++ b/packages/superdoc/src/stores/comments-store.test.js @@ -193,6 +193,8 @@ describe('comments-store', () => { const existingComment = { commentId: 'change-1', trackedChangeText: 'old', + trackedChangeType: 'both', + deletedText: 'removed earlier', getValues: vi.fn(() => ({ commentId: 'change-1' })), }; @@ -216,6 +218,7 @@ describe('comments-store', () => { }); expect(existingComment.trackedChangeText).toBe('new text'); + expect(existingComment.trackedChangeType).toBe('insert'); expect(existingComment.deletedText).toBe('removed'); expect(syncCommentsToClientsMock).toHaveBeenCalledWith( superdoc, @@ -236,6 +239,43 @@ describe('comments-store', () => { ); }); + it('clears stale tracked-change metadata when an update removes one side of a replacement', () => { + const superdoc = { + emit: vi.fn(), + }; + + const existingComment = { + commentId: 'change-clear-1', + trackedChangeText: 'replacement', + trackedChangeType: 'both', + deletedText: 'original', + getValues: vi.fn(() => ({ commentId: 'change-clear-1' })), + }; + + store.commentsList = [existingComment]; + + store.handleTrackedChangeUpdate({ + superdoc, + params: { + event: 'update', + changeId: 'change-clear-1', + trackedChangeText: 'remaining insert', + trackedChangeType: 'insert', + deletedText: null, + authorEmail: 'user@example.com', + author: 'User', + date: 123, + importedAuthor: null, + documentId: 'doc-1', + coords: {}, + }, + }); + + expect(existingComment.trackedChangeText).toBe('remaining insert'); + expect(existingComment.trackedChangeType).toBe('insert'); + expect(existingComment.deletedText).toBeNull(); + }); + it('resolves tracked change comments on resolve events', () => { const superdoc = { emit: vi.fn(), diff --git a/tests/behavior/helpers/editor-interactions.ts b/tests/behavior/helpers/editor-interactions.ts new file mode 100644 index 0000000000..db9b586a9e --- /dev/null +++ b/tests/behavior/helpers/editor-interactions.ts @@ -0,0 +1,35 @@ +import type { Page } from '@playwright/test'; + +/** + * Right-clicks at the screen location corresponding to a document position in the SuperDoc editor. + * + * This helper queries the editor's coordinates for the given logical document position, calculates a suitable + * (x, y) point within the bounding rectangle, and dispatches a mouse right-click at that spot. + * + * Throws if coordinates cannot be resolved for the given position. + * + * @param {Page} page - The Playwright test page instance. + * @param {number} pos - The logical document position (character offset) at which to right-click. + * @returns {Promise