From 2f72caeb32affb84caeaccfce3b58cdb46298213 Mon Sep 17 00:00:00 2001 From: Clarence Palmer Date: Mon, 2 Mar 2026 21:45:28 -0800 Subject: [PATCH 1/7] fix(track-changes): allow for partial tracked-change accept and reject --- .../src/components/context-menu/menuItems.js | 8 +- .../context-menu/tests/menuItems.test.js | 76 ++++ .../track-changes-extension.test.js | 349 +++++++++++++++++- .../track-changes-toolbar-commands.test.js | 62 +++- .../extensions/track-changes/track-changes.js | 332 +++++++++-------- .../superdoc/src/stores/comments-store.js | 13 +- .../src/stores/comments-store.test.js | 40 ++ 7 files changed, 693 insertions(+), 187 deletions(-) diff --git a/packages/super-editor/src/components/context-menu/menuItems.js b/packages/super-editor/src/components/context-menu/menuItems.js index e935e9c0cf..138a0a2de5 100644 --- a/packages/super-editor/src/components/context-menu/menuItems.js +++ b/packages/super-editor/src/components/context-menu/menuItems.js @@ -150,7 +150,9 @@ export function getItems(context, customItems = [], includeDefaultItems = true) label: TEXTS.trackChangesAccept, isDefault: true, action: (editor, context) => { - if (context?.trackedChangeId) { + if (context?.hasSelection) { + editor.commands.acceptTrackedChangeBySelection(); + } else if (context?.trackedChangeId) { editor.commands.acceptTrackedChangeById(context.trackedChangeId); } else { editor.commands.acceptTrackedChangeBySelection(); @@ -167,7 +169,9 @@ export function getItems(context, customItems = [], includeDefaultItems = true) icon: ICONS.trackChangesReject, isDefault: true, action: (editor, context) => { - if (context?.trackedChangeId) { + if (context?.hasSelection) { + editor.commands.rejectTrackedChangeOnSelection(); + } else if (context?.trackedChangeId) { editor.commands.rejectTrackedChangeById(context.trackedChangeId); } else { editor.commands.rejectTrackedChangeOnSelection(); diff --git a/packages/super-editor/src/components/context-menu/tests/menuItems.test.js b/packages/super-editor/src/components/context-menu/tests/menuItems.test.js index b7dd341ad6..6e0b486a8a 100644 --- a/packages/super-editor/src/components/context-menu/tests/menuItems.test.js +++ b/packages/super-editor/src/components/context-menu/tests/menuItems.test.js @@ -126,6 +126,82 @@ describe('menuItems.js', () => { expect(itemIds).not.toContain('track-changes-reject'); }); + it('routes tracked-change context-menu actions through selection commands when text is selected', () => { + const acceptTrackedChangeBySelection = vi.fn(); + const acceptTrackedChangeById = vi.fn(); + const rejectTrackedChangeOnSelection = vi.fn(); + const rejectTrackedChangeById = vi.fn(); + + mockEditor.commands = { + acceptTrackedChangeBySelection, + acceptTrackedChangeById, + rejectTrackedChangeOnSelection, + rejectTrackedChangeById, + }; + + mockContext = createMockContext({ + editor: mockEditor, + trigger: TRIGGERS.click, + hasSelection: true, + isTrackedChange: true, + trackedChangeId: 'tracked-change-1', + }); + + const sections = getItems(mockContext); + const trackSection = sections.find((section) => section.id === 'track-changes'); + const acceptItem = trackSection?.items.find((item) => item.id === 'track-changes-accept'); + const rejectItem = trackSection?.items.find((item) => item.id === 'track-changes-reject'); + + expect(acceptItem).toBeDefined(); + expect(rejectItem).toBeDefined(); + + acceptItem.action(mockEditor, mockContext); + rejectItem.action(mockEditor, mockContext); + + expect(acceptTrackedChangeBySelection).toHaveBeenCalledTimes(1); + expect(acceptTrackedChangeById).not.toHaveBeenCalled(); + expect(rejectTrackedChangeOnSelection).toHaveBeenCalledTimes(1); + expect(rejectTrackedChangeById).not.toHaveBeenCalled(); + }); + + it('routes tracked-change context-menu actions through id commands for collapsed selections', () => { + const acceptTrackedChangeBySelection = vi.fn(); + const acceptTrackedChangeById = vi.fn(); + const rejectTrackedChangeOnSelection = vi.fn(); + const rejectTrackedChangeById = vi.fn(); + + mockEditor.commands = { + acceptTrackedChangeBySelection, + acceptTrackedChangeById, + rejectTrackedChangeOnSelection, + rejectTrackedChangeById, + }; + + mockContext = createMockContext({ + editor: mockEditor, + trigger: TRIGGERS.click, + hasSelection: false, + isTrackedChange: true, + trackedChangeId: 'tracked-change-2', + }); + + const sections = getItems(mockContext); + const trackSection = sections.find((section) => section.id === 'track-changes'); + const acceptItem = trackSection?.items.find((item) => item.id === 'track-changes-accept'); + const rejectItem = trackSection?.items.find((item) => item.id === 'track-changes-reject'); + + expect(acceptItem).toBeDefined(); + expect(rejectItem).toBeDefined(); + + acceptItem.action(mockEditor, mockContext); + rejectItem.action(mockEditor, mockContext); + + expect(acceptTrackedChangeBySelection).not.toHaveBeenCalled(); + expect(acceptTrackedChangeById).toHaveBeenCalledWith('tracked-change-2'); + expect(rejectTrackedChangeOnSelection).not.toHaveBeenCalled(); + expect(rejectTrackedChangeById).toHaveBeenCalledWith('tracked-change-2'); + }); + it('should filter AI items when AI module is not enabled', () => { const sections = getItems(mockContext); diff --git a/packages/super-editor/src/extensions/track-changes/track-changes-extension.test.js b/packages/super-editor/src/extensions/track-changes/track-changes-extension.test.js index a66395308c..4b61286d56 100644 --- a/packages/super-editor/src/extensions/track-changes/track-changes-extension.test.js +++ b/packages/super-editor/src/extensions/track-changes/track-changes-extension.test.js @@ -32,6 +32,35 @@ describe('TrackChanges extension commands', () => { }); return range; }; + const getSubstringRange = (doc, substring) => { + let range = null; + + doc.descendants((node, pos) => { + if (!node.isText || range || typeof node.text !== 'string') return; + + const startIndex = node.text.indexOf(substring); + if (startIndex === -1) return; + + range = { + from: pos + startIndex, + to: pos + startIndex + substring.length, + }; + }); + + return range; + }; + const getMarkedText = (doc, markName) => { + let text = ''; + + doc.descendants((node) => { + if (!node.isText) return; + if (node.marks.some((mark) => mark.type.name === markName)) { + text += node.text ?? ''; + } + }); + + return text; + }; beforeEach(() => { ({ editor } = initTestEditor({ mode: 'text', content: '

' })); @@ -146,6 +175,210 @@ describe('TrackChanges extension commands', () => { expect(markPresent(restoredState.doc, TrackDeleteMarkName)).toBe(false); }); + it('acceptTrackedChangesBetween accepts only the selected middle substring of an insertion', () => { + const changeId = 'ins-partial-accept'; + const insertMark = schema.marks[TrackInsertMarkName].create({ id: changeId }); + const doc = createDoc('ABCDE', [insertMark]); + const state = createState(doc); + const emit = vi.fn(); + const selectionRange = getSubstringRange(doc, 'BC'); + + let nextState; + commands.acceptTrackedChangesBetween( + selectionRange.from, + selectionRange.to, + )({ + state, + dispatch: (tr) => { + nextState = state.apply(tr); + }, + editor: { + emit, + options: { documentId: 'test-doc', user: { email: 'reviewer@example.com', name: 'Reviewer' } }, + }, + }); + + expect(nextState).toBeDefined(); + expect(nextState.doc.textContent).toBe('ABCDE'); + expect(getMarkedText(nextState.doc, TrackInsertMarkName)).toBe('ADE'); + + const updatePayload = emit.mock.calls.find( + ([eventName, payload]) => + eventName === 'commentsUpdate' && + payload?.type === 'trackedChange' && + payload?.event === 'update' && + payload?.changeId === changeId, + )?.[1]; + + expect(updatePayload).toEqual( + expect.objectContaining({ + trackedChangeText: 'ADE', + }), + ); + expect( + emit.mock.calls.some( + ([eventName, payload]) => + eventName === 'commentsUpdate' && + payload?.type === 'trackedChange' && + payload?.event === 'resolve' && + payload?.changeId === changeId, + ), + ).toBe(false); + }); + + it('rejectTrackedChangesBetween rejects only the selected middle substring of an insertion', () => { + const changeId = 'ins-partial-reject'; + const insertMark = schema.marks[TrackInsertMarkName].create({ id: changeId }); + const doc = createDoc('ABCDE', [insertMark]); + const state = createState(doc); + const emit = vi.fn(); + const selectionRange = getSubstringRange(doc, 'BC'); + + let nextState; + commands.rejectTrackedChangesBetween( + selectionRange.from, + selectionRange.to, + )({ + state, + dispatch: (tr) => { + nextState = state.apply(tr); + }, + editor: { + emit, + options: { documentId: 'test-doc', user: { email: 'reviewer@example.com', name: 'Reviewer' } }, + }, + }); + + expect(nextState).toBeDefined(); + expect(nextState.doc.textContent).toBe('ADE'); + expect(getMarkedText(nextState.doc, TrackInsertMarkName)).toBe('ADE'); + + const updatePayload = emit.mock.calls.find( + ([eventName, payload]) => + eventName === 'commentsUpdate' && + payload?.type === 'trackedChange' && + payload?.event === 'update' && + payload?.changeId === changeId, + )?.[1]; + + expect(updatePayload).toEqual( + expect.objectContaining({ + trackedChangeText: 'ADE', + }), + ); + expect( + emit.mock.calls.some( + ([eventName, payload]) => + eventName === 'commentsUpdate' && + payload?.type === 'trackedChange' && + payload?.event === 'resolve' && + payload?.changeId === changeId, + ), + ).toBe(false); + }); + + it('acceptTrackedChangesBetween accepts only the selected middle substring of a deletion', () => { + const changeId = 'del-partial-accept'; + const deleteMark = schema.marks[TrackDeleteMarkName].create({ id: changeId }); + const doc = createDoc('ABCDE', [deleteMark]); + const state = createState(doc); + const emit = vi.fn(); + const selectionRange = getSubstringRange(doc, 'BC'); + + let nextState; + commands.acceptTrackedChangesBetween( + selectionRange.from, + selectionRange.to, + )({ + state, + dispatch: (tr) => { + nextState = state.apply(tr); + }, + editor: { + emit, + options: { documentId: 'test-doc', user: { email: 'reviewer@example.com', name: 'Reviewer' } }, + }, + }); + + expect(nextState).toBeDefined(); + expect(nextState.doc.textContent).toBe('ADE'); + expect(getMarkedText(nextState.doc, TrackDeleteMarkName)).toBe('ADE'); + + const updatePayload = emit.mock.calls.find( + ([eventName, payload]) => + eventName === 'commentsUpdate' && + payload?.type === 'trackedChange' && + payload?.event === 'update' && + payload?.changeId === changeId, + )?.[1]; + + expect(updatePayload).toEqual( + expect.objectContaining({ + deletedText: 'ADE', + }), + ); + expect( + emit.mock.calls.some( + ([eventName, payload]) => + eventName === 'commentsUpdate' && + payload?.type === 'trackedChange' && + payload?.event === 'resolve' && + payload?.changeId === changeId, + ), + ).toBe(false); + }); + + it('rejectTrackedChangesBetween rejects only the selected middle substring of a deletion', () => { + const changeId = 'del-partial-reject'; + const deleteMark = schema.marks[TrackDeleteMarkName].create({ id: changeId }); + const doc = createDoc('ABCDE', [deleteMark]); + const state = createState(doc); + const emit = vi.fn(); + const selectionRange = getSubstringRange(doc, 'BC'); + + let nextState; + commands.rejectTrackedChangesBetween( + selectionRange.from, + selectionRange.to, + )({ + state, + dispatch: (tr) => { + nextState = state.apply(tr); + }, + editor: { + emit, + options: { documentId: 'test-doc', user: { email: 'reviewer@example.com', name: 'Reviewer' } }, + }, + }); + + expect(nextState).toBeDefined(); + expect(nextState.doc.textContent).toBe('ABCDE'); + expect(getMarkedText(nextState.doc, TrackDeleteMarkName)).toBe('ADE'); + + const updatePayload = emit.mock.calls.find( + ([eventName, payload]) => + eventName === 'commentsUpdate' && + payload?.type === 'trackedChange' && + payload?.event === 'update' && + payload?.changeId === changeId, + )?.[1]; + + expect(updatePayload).toEqual( + expect.objectContaining({ + deletedText: 'ADE', + }), + ); + expect( + emit.mock.calls.some( + ([eventName, payload]) => + eventName === 'commentsUpdate' && + payload?.type === 'trackedChange' && + payload?.event === 'resolve' && + payload?.changeId === changeId, + ), + ).toBe(false); + }); + it('rejectTrackedChangesBetween emits tracked-change resolve events for rejected IDs', () => { const insertMark = schema.marks[TrackInsertMarkName].create({ id: 'ins-resolve-1' }); const doc = createDoc('Pending', [insertMark]); @@ -176,17 +409,26 @@ describe('TrackChanges extension commands', () => { ); }); - it('rejectTrackedChangesBetween expands partial selection to reject the full tracked-change ID', () => { + it('rejectTrackedChangesBetween applies mixed selections per overlapped tracked segment', () => { const changeId = 'ins-partial-still-present'; const insertMark = schema.marks[TrackInsertMarkName].create({ id: changeId }); - const doc = createDoc('Pending', [insertMark]); + const deleteId = 'del-partial-still-present'; + const deleteMark = schema.marks[TrackDeleteMarkName].create({ id: deleteId }); + const paragraph = schema.nodes.paragraph.create(null, [ + schema.text('AB', [insertMark]), + schema.text('x'), + schema.text('CD', [deleteMark]), + ]); + const doc = schema.nodes.doc.create(null, paragraph); const state = createState(doc); const emit = vi.fn(); + const insertionSelection = getSubstringRange(doc, 'B'); + const deletionSelection = getSubstringRange(doc, 'C'); let nextState; commands.rejectTrackedChangesBetween( - 1, - 3, + insertionSelection.from, + deletionSelection.to, )({ state, dispatch: (tr) => { @@ -199,17 +441,27 @@ describe('TrackChanges extension commands', () => { }); expect(nextState).toBeDefined(); - expect(nextState.doc.textContent).toBe(''); - - const resolveEventsForId = emit.mock.calls.filter(([eventName, payload]) => { - return ( - eventName === 'commentsUpdate' && - payload?.type === 'trackedChange' && - payload?.event === 'resolve' && - payload?.changeId === changeId - ); - }); - expect(resolveEventsForId).toHaveLength(1); + expect(nextState.doc.textContent).toBe('AxCD'); + expect(getMarkedText(nextState.doc, TrackInsertMarkName)).toBe('A'); + expect(getMarkedText(nextState.doc, TrackDeleteMarkName)).toBe('D'); + + const updatedIds = emit.mock.calls + .filter( + ([eventName, payload]) => + eventName === 'commentsUpdate' && payload?.type === 'trackedChange' && payload?.event === 'update', + ) + .map(([, payload]) => payload.changeId); + + expect(updatedIds).toEqual(expect.arrayContaining([changeId, deleteId])); + expect( + emit.mock.calls.some( + ([eventName, payload]) => + eventName === 'commentsUpdate' && + payload?.type === 'trackedChange' && + payload?.event === 'resolve' && + [changeId, deleteId].includes(payload?.changeId), + ), + ).toBe(false); }); it('blocks rejecting tracked changes when permissionResolver denies access', () => { @@ -628,6 +880,73 @@ describe('TrackChanges extension commands', () => { } }); + it('undo/redo restores partially accepted insertion splits', () => { + const { editor: interactionEditor } = initTestEditor({ + mode: 'text', + content: '

', + user: { name: 'Track Tester', email: 'track@example.com' }, + }); + + try { + interactionEditor.commands.insertTrackedChange({ from: 1, to: 1, text: 'ABCDE' }); + + 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.acceptTrackedChangeBySelection(); + expect(interactionEditor.state.doc.textContent).toBe('ABCDE'); + expect(getMarkedText(interactionEditor.state.doc, TrackInsertMarkName)).toBe('ADE'); + + interactionEditor.commands.undo(); + expect(interactionEditor.state.doc.textContent).toBe('ABCDE'); + expect(getMarkedText(interactionEditor.state.doc, TrackInsertMarkName)).toBe('ABCDE'); + + interactionEditor.commands.redo(); + expect(interactionEditor.state.doc.textContent).toBe('ABCDE'); + expect(getMarkedText(interactionEditor.state.doc, TrackInsertMarkName)).toBe('ADE'); + } finally { + interactionEditor.destroy(); + } + }); + + it('undo/redo restores partially rejected deletion splits', () => { + const { editor: interactionEditor } = initTestEditor({ + mode: 'text', + content: '

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..c871f7773a 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,17 +7,24 @@ vi.mock('../comment/comments-plugin.js', () => ({ }, })); +vi.mock('./permission-helpers.js', () => ({ + collectTrackedChanges: vi.fn(), +})); + describe('Track Changes Toolbar Commands', () => { let commands; let mockState; let mockCommands; let mockCommentsPluginGetState; + let mockCollectTrackedChanges; beforeEach(async () => { vi.clearAllMocks(); const { CommentsPluginKey } = await import('../comment/comments-plugin.js'); + const { collectTrackedChanges } = await import('./permission-helpers.js'); mockCommentsPluginGetState = CommentsPluginKey.getState; + mockCollectTrackedChanges = collectTrackedChanges; commands = TrackChanges.config.addCommands(); @@ -31,6 +38,8 @@ describe('Track Changes Toolbar Commands', () => { mockState = { selection: { from: 10, to: 10 }, }; + + mockCollectTrackedChanges.mockReturnValue([]); }); describe('acceptTrackedChangeFromToolbar', () => { @@ -53,8 +62,30 @@ describe('Track Changes Toolbar Commands', () => { expect(mockCommands.acceptTrackedChangeBySelection).not.toHaveBeenCalled(); }); - it('uses acceptTrackedChangeById when active tracked change exists (text selected)', () => { + 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 }); + + expect(result).toBe(true); + expect(mockCommands.acceptTrackedChangeBySelection).toHaveBeenCalled(); + expect(mockCommands.acceptTrackedChangeById).not.toHaveBeenCalled(); + }); + + 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', @@ -134,10 +165,10 @@ describe('Track Changes Toolbar Commands', () => { 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: { @@ -151,8 +182,8 @@ describe('Track Changes Toolbar Commands', () => { const result = command()({ state: mockState, commands: mockCommands }); expect(result).toBe(true); - expect(mockCommands.rejectTrackedChangeById).toHaveBeenCalledWith('tracked-change-999'); - expect(mockCommands.rejectTrackedChangeOnSelection).not.toHaveBeenCalled(); + expect(mockCommands.rejectTrackedChangeOnSelection).toHaveBeenCalled(); + expect(mockCommands.rejectTrackedChangeById).not.toHaveBeenCalled(); }); it('falls back to rejectTrackedChangeOnSelection when no active tracked change', () => { @@ -182,5 +213,26 @@ describe('Track Changes Toolbar Commands', () => { expect(mockCommands.rejectTrackedChangeOnSelection).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 }); + + expect(result).toBe(true); + expect(mockCommands.rejectTrackedChangeById).toHaveBeenCalledWith('tracked-change-999'); + 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..cf0b4be73c 100644 --- a/packages/super-editor/src/extensions/track-changes/track-changes.js +++ b/packages/super-editor/src/extensions/track-changes/track-changes.js @@ -8,7 +8,7 @@ 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'; export const TrackChanges = Extension.create({ @@ -30,157 +30,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; - } - - tr.step(new RemoveMarkStep(mappedFrom, mappedTo, liveMark)); - }); - - tr.step( - new RemoveMarkStep( - map.map(Math.max(pos, rangeFrom)), - map.map(Math.min(pos + node.nodeSize, rangeTo)), - formatChangeMark, - ), - ); - } + doc.nodesBetween(from, to, (node, pos) => { + 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) { + 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: @@ -202,6 +146,16 @@ export const TrackChanges = Extension.create({ ({ state, commands }) => { const commentsPluginState = CommentsPluginKey.getState(state); const activeThreadId = commentsPluginState?.activeThreadId; + const shouldUseSelection = + hasExpandedSelection(state.selection) && + selectionTouchesTrackedChange({ + state, + trackedChangeId: activeThreadId, + }); + + if (shouldUseSelection) { + return commands.acceptTrackedChangeBySelection(); + } if (activeThreadId && commentsPluginState?.trackedChanges?.[activeThreadId]) { return commands.acceptTrackedChangeById(activeThreadId); @@ -265,6 +219,16 @@ export const TrackChanges = Extension.create({ ({ state, commands }) => { const commentsPluginState = CommentsPluginKey.getState(state); const activeThreadId = commentsPluginState?.activeThreadId; + const shouldUseSelection = + hasExpandedSelection(state.selection) && + selectionTouchesTrackedChange({ + state, + trackedChangeId: activeThreadId, + }); + + if (shouldUseSelection) { + return commands.rejectTrackedChangeOnSelection(); + } if (activeThreadId && commentsPluginState?.trackedChanges?.[activeThreadId]) { return commands.rejectTrackedChangeById(activeThreadId); @@ -508,46 +472,98 @@ 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 hasExpandedSelection = (selection) => { + if (!selection) return false; + return selection.from !== selection.to; +}; + +const selectionTouchesTrackedChange = ({ state, trackedChangeId }) => { + if (!trackedChangeId) { + return ( + collectTrackedChanges({ + state, + from: state.selection.from, + to: state.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: state.selection.from, + to: state.selection.to, + }).some((change) => change.id === trackedChangeId); +}; + +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); + + 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; + } + + 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/superdoc/src/stores/comments-store.js b/packages/superdoc/src/stores/comments-store.js index f05d048287..dc57064dca 100644 --- a/packages/superdoc/src/stores/comments-store.js +++ b/packages/superdoc/src/stores/comments-store.js @@ -337,8 +337,9 @@ 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; + existing.trackedChangeText = trackedChangeText ?? null; + existing.trackedChangeType = trackedChangeType ?? null; + existing.deletedText = deletedText ?? null; const emitData = { type: COMMENT_EVENTS.UPDATE, @@ -355,11 +356,9 @@ 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; - } + 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 d3de3f69e1..bb481efabd 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(), From 3294457551ca601e4c1f1eb901e6ca14edc0741f Mon Sep 17 00:00:00 2001 From: Clarence Palmer Date: Mon, 2 Mar 2026 23:20:41 -0800 Subject: [PATCH 2/7] refactor(track-changes): adjust context menu actions for accepting and rejecting tracked changes --- .../src/components/context-menu/menuItems.js | 24 +-- .../context-menu/tests/menuItems.test.js | 56 ++--- .../context-menu/tests/utils.test.js | 25 +++ .../src/components/context-menu/utils.js | 43 +++- .../track-changes-toolbar-commands.test.js | 195 ++++++++++++++++-- .../extensions/track-changes/track-changes.js | 166 +++++++++++---- .../types/track-changes-commands.ts | 20 ++ .../tracked-change-partial-resolution.spec.ts | 132 ++++++++++++ .../tracked-change-partial-resolution.spec.ts | 137 ++++++++++++ 9 files changed, 699 insertions(+), 99 deletions(-) create mode 100644 tests/behavior/tests/comments/tracked-change-partial-resolution.spec.ts create mode 100644 tests/visual/tests/behavior/tracked-change-partial-resolution.spec.ts diff --git a/packages/super-editor/src/components/context-menu/menuItems.js b/packages/super-editor/src/components/context-menu/menuItems.js index 138a0a2de5..62b06ee71e 100644 --- a/packages/super-editor/src/components/context-menu/menuItems.js +++ b/packages/super-editor/src/components/context-menu/menuItems.js @@ -150,13 +150,11 @@ export function getItems(context, customItems = [], includeDefaultItems = true) label: TEXTS.trackChangesAccept, isDefault: true, action: (editor, context) => { - if (context?.hasSelection) { - editor.commands.acceptTrackedChangeBySelection(); - } else if (context?.trackedChangeId) { - editor.commands.acceptTrackedChangeById(context.trackedChangeId); - } else { - editor.commands.acceptTrackedChangeBySelection(); - } + editor.commands.acceptTrackedChangeFromContextMenu({ + from: context?.selectionStart, + to: context?.selectionEnd, + trackedChangeId: context?.trackedChangeId, + }); }, showWhen: (context) => { const { trigger, isTrackedChange } = context; @@ -169,13 +167,11 @@ export function getItems(context, customItems = [], includeDefaultItems = true) icon: ICONS.trackChangesReject, isDefault: true, action: (editor, context) => { - if (context?.hasSelection) { - editor.commands.rejectTrackedChangeOnSelection(); - } else if (context?.trackedChangeId) { - editor.commands.rejectTrackedChangeById(context.trackedChangeId); - } else { - editor.commands.rejectTrackedChangeOnSelection(); - } + editor.commands.rejectTrackedChangeFromContextMenu({ + from: context?.selectionStart, + to: context?.selectionEnd, + trackedChangeId: context?.trackedChangeId, + }); }, showWhen: (context) => { const { trigger, isTrackedChange } = context; diff --git a/packages/super-editor/src/components/context-menu/tests/menuItems.test.js b/packages/super-editor/src/components/context-menu/tests/menuItems.test.js index 6e0b486a8a..05911e8d23 100644 --- a/packages/super-editor/src/components/context-menu/tests/menuItems.test.js +++ b/packages/super-editor/src/components/context-menu/tests/menuItems.test.js @@ -127,16 +127,12 @@ describe('menuItems.js', () => { }); it('routes tracked-change context-menu actions through selection commands when text is selected', () => { - const acceptTrackedChangeBySelection = vi.fn(); - const acceptTrackedChangeById = vi.fn(); - const rejectTrackedChangeOnSelection = vi.fn(); - const rejectTrackedChangeById = vi.fn(); + const acceptTrackedChangeFromContextMenu = vi.fn(); + const rejectTrackedChangeFromContextMenu = vi.fn(); mockEditor.commands = { - acceptTrackedChangeBySelection, - acceptTrackedChangeById, - rejectTrackedChangeOnSelection, - rejectTrackedChangeById, + acceptTrackedChangeFromContextMenu, + rejectTrackedChangeFromContextMenu, }; mockContext = createMockContext({ @@ -144,6 +140,8 @@ describe('menuItems.js', () => { trigger: TRIGGERS.click, hasSelection: true, isTrackedChange: true, + selectionStart: 10, + selectionEnd: 14, trackedChangeId: 'tracked-change-1', }); @@ -158,23 +156,25 @@ describe('menuItems.js', () => { acceptItem.action(mockEditor, mockContext); rejectItem.action(mockEditor, mockContext); - expect(acceptTrackedChangeBySelection).toHaveBeenCalledTimes(1); - expect(acceptTrackedChangeById).not.toHaveBeenCalled(); - expect(rejectTrackedChangeOnSelection).toHaveBeenCalledTimes(1); - expect(rejectTrackedChangeById).not.toHaveBeenCalled(); + expect(acceptTrackedChangeFromContextMenu).toHaveBeenCalledWith({ + from: 10, + to: 14, + trackedChangeId: 'tracked-change-1', + }); + expect(rejectTrackedChangeFromContextMenu).toHaveBeenCalledWith({ + from: 10, + to: 14, + trackedChangeId: 'tracked-change-1', + }); }); - it('routes tracked-change context-menu actions through id commands for collapsed selections', () => { - const acceptTrackedChangeBySelection = vi.fn(); - const acceptTrackedChangeById = vi.fn(); - const rejectTrackedChangeOnSelection = vi.fn(); - const rejectTrackedChangeById = vi.fn(); + it('routes tracked-change context-menu actions through toolbar commands for collapsed selections', () => { + const acceptTrackedChangeFromContextMenu = vi.fn(); + const rejectTrackedChangeFromContextMenu = vi.fn(); mockEditor.commands = { - acceptTrackedChangeBySelection, - acceptTrackedChangeById, - rejectTrackedChangeOnSelection, - rejectTrackedChangeById, + acceptTrackedChangeFromContextMenu, + rejectTrackedChangeFromContextMenu, }; mockContext = createMockContext({ @@ -196,10 +196,16 @@ describe('menuItems.js', () => { acceptItem.action(mockEditor, mockContext); rejectItem.action(mockEditor, mockContext); - expect(acceptTrackedChangeBySelection).not.toHaveBeenCalled(); - expect(acceptTrackedChangeById).toHaveBeenCalledWith('tracked-change-2'); - expect(rejectTrackedChangeOnSelection).not.toHaveBeenCalled(); - expect(rejectTrackedChangeById).toHaveBeenCalledWith('tracked-change-2'); + expect(acceptTrackedChangeFromContextMenu).toHaveBeenCalledWith({ + from: 10, + to: 10, + trackedChangeId: 'tracked-change-2', + }); + expect(rejectTrackedChangeFromContextMenu).toHaveBeenCalledWith({ + from: 10, + to: 10, + trackedChangeId: 'tracked-change-2', + }); }); it('should filter AI items when AI module is not enabled', () => { diff --git a/packages/super-editor/src/components/context-menu/tests/utils.test.js b/packages/super-editor/src/components/context-menu/tests/utils.test.js index 119b5d260e..86acee329b 100644 --- a/packages/super-editor/src/components/context-menu/tests/utils.test.js +++ b/packages/super-editor/src/components/context-menu/tests/utils.test.js @@ -255,6 +255,31 @@ describe('utils.js', () => { expect(Array.isArray(context.trackedChanges)).toBe(true); }); + it('prefers preserved selection for right-click context when the live selection collapsed inside it', async () => { + const mockEvent = { clientX: 300, clientY: 400 }; + + mockSelectionHasNodeOrMark.mockReturnValue(false); + mockEditor.options.preservedSelection = { from: 10, to: 15 }; + mockEditor.view.state.selection.empty = true; + mockEditor.view.state.selection.from = 12; + mockEditor.view.state.selection.to = 12; + mockEditor.view.posAtCoords.mockReturnValue({ pos: 12 }); + mockEditor.view.state.doc.nodeAt.mockReturnValue({ type: { name: 'text' } }); + mockEditor.view.state.doc.textBetween.mockReturnValue('selected text'); + mockEditor.view.state.doc.resolve.mockReturnValue({ + marks: vi.fn(() => []), + nodeBefore: null, + nodeAfter: null, + }); + + const context = await getEditorContext(mockEditor, mockEvent); + + expect(context.hasSelection).toBe(true); + expect(context.selectionStart).toBe(10); + expect(context.selectionEnd).toBe(15); + expect(context.selectedText).toBe('selected text'); + }); + it('should detect tracked change marks directly at the resolved cursor position', async () => { const mockEvent = { clientX: 150, clientY: 250 }; const trackFormatMark = { type: { name: 'trackFormat' }, attrs: { id: 'track-format-1' } }; diff --git a/packages/super-editor/src/components/context-menu/utils.js b/packages/super-editor/src/components/context-menu/utils.js index cc03dfb5b4..836a0f610c 100644 --- a/packages/super-editor/src/components/context-menu/utils.js +++ b/packages/super-editor/src/components/context-menu/utils.js @@ -85,9 +85,7 @@ export async function getEditorContext(editor, event) { const state = editor.state; if (!state) return null; - - const { from, to, empty } = state.selection; - const selectedText = !empty ? state.doc.textBetween(from, to) : ''; + const { from } = state.selection; let pos = null; let node = null; @@ -106,6 +104,10 @@ export async function getEditorContext(editor, event) { node = state.doc.nodeAt(pos); } + const selection = getContextSelection({ editor, state, pos, event }); + const hasSelection = hasExpandedSelection(selection); + const selectedText = hasSelection ? state.doc.textBetween(selection.from, selection.to) : ''; + // Don't read clipboard proactively to avoid permission prompts // Clipboard will be read only when user actually clicks "Paste" const clipboardContent = { @@ -169,7 +171,7 @@ export async function getEditorContext(editor, event) { const trackedChanges = event && pos !== null ? collectTrackedChangesForContext({ state, pos, trackedChangeId }) - : collectTrackedChanges({ state, from, to }); + : collectTrackedChanges({ state, from: selection.from, to: selection.to }); const cursorCoords = pos !== null ? editor.coordsAtPos?.(pos) : null; const cursorPosition = cursorCoords @@ -183,9 +185,9 @@ export async function getEditorContext(editor, event) { return { selectedText, - hasSelection: !empty, - selectionStart: from, - selectionEnd: to, + hasSelection, + selectionStart: selection.from, + selectionEnd: selection.to, isInTable, isInList, isInSectionNode, @@ -210,6 +212,33 @@ export async function getEditorContext(editor, event) { }; } +function hasExpandedSelection(selection) { + return Number.isFinite(selection?.from) && Number.isFinite(selection?.to) && selection.from !== selection.to; +} + +function selectionContainsPos(selection, pos) { + return hasExpandedSelection(selection) && Number.isFinite(pos) && pos >= selection.from && pos <= selection.to; +} + +function getContextSelection({ editor, state, pos, event }) { + const currentSelection = state.selection; + const preservedSelection = editor?.options?.preservedSelection ?? editor?.options?.lastSelection; + + if (hasExpandedSelection(currentSelection)) { + return currentSelection; + } + + if (!hasExpandedSelection(preservedSelection)) { + return currentSelection; + } + + if (event) { + return selectionContainsPos(preservedSelection, pos) ? preservedSelection : currentSelection; + } + + return preservedSelection; +} + function computeCanUndo(editor, state) { if (typeof editor?.can === 'function') { try { 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 c871f7773a..236c16f60b 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 @@ -11,7 +11,7 @@ vi.mock('./permission-helpers.js', () => ({ collectTrackedChanges: vi.fn(), })); -describe('Track Changes Toolbar Commands', () => { +describe('Track Changes Shared Resolution Commands', () => { let commands; let mockState; let mockCommands; @@ -29,8 +29,10 @@ describe('Track Changes Toolbar Commands', () => { 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), }; @@ -55,10 +57,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-123'); + expect(mockCommands.acceptTrackedChangesBetween).not.toHaveBeenCalled(); expect(mockCommands.acceptTrackedChangeBySelection).not.toHaveBeenCalled(); }); @@ -76,11 +79,12 @@ 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).toHaveBeenCalledWith(10, 15); expect(mockCommands.acceptTrackedChangeById).not.toHaveBeenCalled(); + expect(mockCommands.acceptTrackedChangeBySelection).not.toHaveBeenCalled(); }); it('uses acceptTrackedChangeById when a stale expanded selection does not touch the active tracked change', () => { @@ -97,10 +101,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(); }); @@ -111,10 +116,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(); }); @@ -126,10 +132,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(); }); @@ -137,12 +144,39 @@ 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(); + }); }); describe('rejectTrackedChangeFromToolbar', () => { @@ -158,10 +192,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-789'); + expect(mockCommands.rejectTrackedChangesBetween).not.toHaveBeenCalled(); expect(mockCommands.rejectTrackedChangeOnSelection).not.toHaveBeenCalled(); }); @@ -179,11 +214,12 @@ 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).toHaveBeenCalledWith(20, 25); expect(mockCommands.rejectTrackedChangeById).not.toHaveBeenCalled(); + expect(mockCommands.rejectTrackedChangeOnSelection).not.toHaveBeenCalled(); }); it('falls back to rejectTrackedChangeOnSelection when no active tracked change', () => { @@ -193,10 +229,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(); }); @@ -207,10 +244,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(); }); @@ -228,10 +266,139 @@ 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).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(); + }); + }); + + 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(); }); }); 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 cf0b4be73c..c84520a989 100644 --- a/packages/super-editor/src/extensions/track-changes/track-changes.js +++ b/packages/super-editor/src/extensions/track-changes/track-changes.js @@ -143,25 +143,36 @@ export const TrackChanges = Extension.create({ acceptTrackedChangeFromToolbar: () => - ({ state, commands }) => { - const commentsPluginState = CommentsPluginKey.getState(state); - const activeThreadId = commentsPluginState?.activeThreadId; - const shouldUseSelection = - hasExpandedSelection(state.selection) && - selectionTouchesTrackedChange({ + ({ state, commands, editor }) => { + return resolveTrackedChangeAction({ + action: 'accept', + state, + commands, + editor, + ...getTrackedChangeResolutionContext({ state, - trackedChangeId: activeThreadId, - }); - - if (shouldUseSelection) { - return commands.acceptTrackedChangeBySelection(); - } + 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: @@ -216,25 +227,36 @@ export const TrackChanges = Extension.create({ rejectTrackedChangeFromToolbar: () => - ({ state, commands }) => { - const commentsPluginState = CommentsPluginKey.getState(state); - const activeThreadId = commentsPluginState?.activeThreadId; - const shouldUseSelection = - hasExpandedSelection(state.selection) && - selectionTouchesTrackedChange({ + ({ state, commands, editor }) => { + return resolveTrackedChangeAction({ + action: 'reject', + state, + commands, + editor, + ...getTrackedChangeResolutionContext({ state, - trackedChangeId: activeThreadId, - }); - - if (shouldUseSelection) { - return commands.rejectTrackedChangeOnSelection(); - } + 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: @@ -481,24 +503,90 @@ const hasExpandedSelection = (selection) => { return selection.from !== selection.to; }; -const selectionTouchesTrackedChange = ({ state, trackedChangeId }) => { +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; + + return { + trackedChangeId: resolvedTrackedChangeId, + hasKnownTrackedChangeId: Boolean( + resolvedTrackedChangeId && commentsPluginState?.trackedChanges?.[resolvedTrackedChangeId], + ), + }; +}; + +const selectionTouchesTrackedChange = ({ state, trackedChangeId, selection = state?.selection }) => { + if (!selection) { + return false; + } + if (!trackedChangeId) { return ( collectTrackedChanges({ state, - from: state.selection.from, - to: state.selection.to, + from: selection.from, + to: selection.to, }).length > 0 ); } return collectTrackedChanges({ state, - from: state.selection.from, - to: state.selection.to, + 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, + }); + + 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, 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/tests/behavior/tests/comments/tracked-change-partial-resolution.spec.ts b/tests/behavior/tests/comments/tracked-change-partial-resolution.spec.ts new file mode 100644 index 0000000000..217038d3e2 --- /dev/null +++ b/tests/behavior/tests/comments/tracked-change-partial-resolution.spec.ts @@ -0,0 +1,132 @@ +import { test, expect } from '../../fixtures/superdoc.js'; +import { getDocumentText } from '../../helpers/document-api.js'; + +test.use({ config: { toolbar: 'full', comments: 'panel', trackChanges: true, showSelection: true } }); + +const TRACK_TEXT = 'ABCDE'; +const PARTIAL_TEXT = 'BC'; +const ACCEPT_TRACKED_CHANGES_BUTTON = 'Accept tracked changes'; + +async function insertTrackedChange( + page: import('@playwright/test').Page, + options: { + from: number; + to: number; + text: string; + }, +) { + await page.evaluate((payload) => { + (window as any).editor.commands.insertTrackedChange({ + ...payload, + user: { name: 'Track Tester', email: 'track@example.com' }, + }); + }, options); +} + +async function getMarkedText(page: import('@playwright/test').Page, markName: string): Promise { + return page.evaluate((name) => { + let text = ''; + const doc = (window as any).editor.state.doc; + + doc.descendants((node: any) => { + if (!node.isText) return; + if (node.marks.some((mark: any) => mark.type.name === name)) { + text += node.text ?? ''; + } + }); + + return text; + }, markName); +} + +async function getSelectedText(page: import('@playwright/test').Page): Promise { + return page.evaluate(() => { + const { from, to, empty } = (window as any).editor.state.selection; + if (empty) return ''; + return (window as any).editor.state.doc.textBetween(from, to); + }); +} + +async function rightClickAtDocPos(page: import('@playwright/test').Page, pos: number) { + const coords = await page.evaluate((p) => { + const editor = (window as any).editor; + const rect = editor?.coordsAtPos?.(p); + if (!rect) return null; + return { + left: Number(rect.left), + right: Number(rect.right), + top: Number(rect.top), + bottom: Number(rect.bottom), + }; + }, pos); + + if (!coords) { + throw new Error(`Could not resolve coordinates for document position ${pos}`); + } + + const x = Math.min(Math.max(coords.left + 1, coords.left), Math.max(coords.right - 1, coords.left + 1)); + const y = (coords.top + coords.bottom) / 2; + await page.mouse.click(x, y, { button: 'right' }); +} + +test('toolbar accept partially resolves a tracked insertion and updates the bubble text', async ({ superdoc }) => { + await insertTrackedChange(superdoc.page, { from: 1, to: 1, text: TRACK_TEXT }); + await superdoc.waitForStable(); + + const selectionStart = await superdoc.findTextPos(PARTIAL_TEXT); + await superdoc.setTextSelection(selectionStart, selectionStart + PARTIAL_TEXT.length); + await superdoc.waitForStable(); + + const trackedDialog = superdoc.page.locator('.comment-placeholder .comments-dialog', { + has: superdoc.page.locator('.tracked-change-text.is-inserted', { hasText: TRACK_TEXT }), + }); + await expect(trackedDialog).toBeVisible(); + + await superdoc.snapshot('tracked-change-partial-insert-before-accept'); + + await superdoc.page.getByRole('button', { name: ACCEPT_TRACKED_CHANGES_BUTTON }).click(); + await superdoc.waitForStable(); + + await expect.poll(() => getDocumentText(superdoc.page)).toBe(TRACK_TEXT); + await expect.poll(() => getMarkedText(superdoc.page, 'trackInsert')).toBe('ADE'); + await expect(superdoc.page.locator('.comment-placeholder .comments-dialog .tracked-change-text')).toBeVisible(); + + await superdoc.snapshot('tracked-change-partial-insert-after-accept'); +}); + +test('context menu reject partially resolves a tracked insertion and updates the bubble text', async ({ + superdoc, + browserName, +}) => { + test.skip(browserName === 'firefox', 'Firefox collapses selection on right-click natively'); + + await insertTrackedChange(superdoc.page, { from: 1, to: 1, text: TRACK_TEXT }); + await superdoc.waitForStable(); + + const selectionStart = await superdoc.findTextPos(PARTIAL_TEXT); + await superdoc.setTextSelection(selectionStart, selectionStart + PARTIAL_TEXT.length); + await superdoc.waitForStable(); + + await expect.poll(() => getSelectedText(superdoc.page)).toBe(PARTIAL_TEXT); + + const trackedDialog = superdoc.page.locator('.comment-placeholder .comments-dialog', { + has: superdoc.page.locator('.tracked-change-text.is-inserted', { hasText: TRACK_TEXT }), + }); + await expect(trackedDialog).toBeVisible(); + + await superdoc.snapshot('tracked-change-partial-insert-before-context-reject'); + + await rightClickAtDocPos(superdoc.page, selectionStart + 1); + await superdoc.waitForStable(); + + const menu = superdoc.page.locator('.context-menu'); + await expect(menu).toBeVisible(); + await menu.locator('.context-menu-item').filter({ hasText: 'Reject change' }).click(); + await superdoc.waitForStable(); + + await expect.poll(() => getDocumentText(superdoc.page)).toBe('ADE'); + await expect.poll(() => getMarkedText(superdoc.page, 'trackInsert')).toBe('ADE'); + await expect(superdoc.page.locator('.comment-placeholder .comments-dialog .tracked-change-text')).toBeVisible(); + + await superdoc.snapshot('tracked-change-partial-insert-after-context-reject'); +}); diff --git a/tests/visual/tests/behavior/tracked-change-partial-resolution.spec.ts b/tests/visual/tests/behavior/tracked-change-partial-resolution.spec.ts new file mode 100644 index 0000000000..ab359ecf28 --- /dev/null +++ b/tests/visual/tests/behavior/tracked-change-partial-resolution.spec.ts @@ -0,0 +1,137 @@ +import { test, expect } from '../fixtures/superdoc.ts'; + +test.use({ + config: { + toolbar: 'full', + comments: 'panel', + trackChanges: true, + hideSelection: false, + }, +}); + +const TRACK_TEXT = 'ABCDE'; +const PARTIAL_TEXT = 'BC'; +async function insertTrackedChange( + page: import('@playwright/test').Page, + options: { + from: number; + to: number; + text: string; + }, +) { + await page.evaluate((payload) => { + (window as any).editor.commands.insertTrackedChange({ + ...payload, + user: { name: 'Track Tester', email: 'track@example.com' }, + }); + }, options); +} + +async function getMarkedText(page: import('@playwright/test').Page, markName: string): Promise { + return page.evaluate((name) => { + let text = ''; + const doc = (window as any).editor.state.doc; + + doc.descendants((node: any) => { + if (!node.isText) return; + if (node.marks.some((mark: any) => mark.type.name === name)) { + text += node.text ?? ''; + } + }); + + return text; + }, markName); +} + +async function getDocumentText(page: import('@playwright/test').Page): Promise { + return page.evaluate(() => (window as any).editor.doc.getText({})); +} + +async function getSelectedText(page: import('@playwright/test').Page): Promise { + return page.evaluate(() => { + const { from, to, empty } = (window as any).editor.state.selection; + if (empty) return ''; + return (window as any).editor.state.doc.textBetween(from, to); + }); +} + +async function rightClickAtDocPos(page: import('@playwright/test').Page, pos: number) { + const coords = await page.evaluate((p) => { + const editor = (window as any).editor; + const rect = editor?.coordsAtPos?.(p); + if (!rect) return null; + return { + left: Number(rect.left), + right: Number(rect.right), + top: Number(rect.top), + bottom: Number(rect.bottom), + }; + }, pos); + + if (!coords) { + throw new Error(`Could not resolve coordinates for document position ${pos}`); + } + + const x = Math.min(Math.max(coords.left + 1, coords.left), Math.max(coords.right - 1, coords.left + 1)); + const y = (coords.top + coords.bottom) / 2; + await page.mouse.click(x, y, { button: 'right' }); +} + +test('@behavior toolbar accept partially resolves a tracked insertion', async ({ superdoc }) => { + await insertTrackedChange(superdoc.page, { from: 1, to: 1, text: TRACK_TEXT }); + await superdoc.waitForStable(); + + const selection = await superdoc.findTextRange(PARTIAL_TEXT); + await superdoc.setTextSelection(selection.from, selection.to); + await superdoc.waitForStable(); + + const trackedDialog = superdoc.page.locator('.comment-placeholder .comments-dialog', { + has: superdoc.page.locator('.tracked-change-text.is-inserted', { hasText: TRACK_TEXT }), + }); + await expect(trackedDialog).toBeVisible(); + + await superdoc.screenshot('tracked-change-partial-insert-before-accept'); + + await superdoc.executeCommand('acceptTrackedChangeFromToolbar'); + await superdoc.waitForStable(); + + await expect.poll(() => getDocumentText(superdoc.page)).toBe(TRACK_TEXT); + await expect.poll(() => getMarkedText(superdoc.page, 'trackInsert')).toBe('ADE'); + await expect(superdoc.page.locator('.comment-placeholder .comments-dialog .tracked-change-text')).toBeVisible(); + + await superdoc.screenshot('tracked-change-partial-insert-after-accept'); +}); + +test('@behavior context menu reject partially resolves a tracked insertion', async ({ superdoc, browserName }) => { + test.skip(browserName === 'firefox', 'Firefox collapses selection on right-click natively'); + + await insertTrackedChange(superdoc.page, { from: 1, to: 1, text: TRACK_TEXT }); + await superdoc.waitForStable(); + + const selection = await superdoc.findTextRange(PARTIAL_TEXT); + await superdoc.setTextSelection(selection.from, selection.to); + await superdoc.waitForStable(); + + await expect.poll(() => getSelectedText(superdoc.page)).toBe(PARTIAL_TEXT); + + const trackedDialog = superdoc.page.locator('.comment-placeholder .comments-dialog', { + has: superdoc.page.locator('.tracked-change-text.is-inserted', { hasText: TRACK_TEXT }), + }); + await expect(trackedDialog).toBeVisible(); + + await superdoc.screenshot('tracked-change-partial-insert-before-context-reject'); + + await rightClickAtDocPos(superdoc.page, selection.from + 1); + await superdoc.waitForStable(); + + const menu = superdoc.page.locator('.context-menu'); + await expect(menu).toBeVisible(); + await menu.locator('.context-menu-item').filter({ hasText: 'Reject change' }).click(); + await superdoc.waitForStable(); + + await expect.poll(() => getDocumentText(superdoc.page)).toBe('ADE'); + await expect.poll(() => getMarkedText(superdoc.page, 'trackInsert')).toBe('ADE'); + await expect(superdoc.page.locator('.comment-placeholder .comments-dialog .tracked-change-text')).toBeVisible(); + + await superdoc.screenshot('tracked-change-partial-insert-after-context-reject'); +}); From f1ef5977505914de44bdeb6bb85bdcb0447fca71 Mon Sep 17 00:00:00 2001 From: Clarence Palmer Date: Tue, 3 Mar 2026 08:36:15 -0800 Subject: [PATCH 3/7] fix(context-menu): enhance tracked changes handling for right-click actions within expanded selections --- .../context-menu/tests/utils.test.js | 36 +++++++++++++++++++ .../src/components/context-menu/utils.js | 9 +++-- 2 files changed, 43 insertions(+), 2 deletions(-) diff --git a/packages/super-editor/src/components/context-menu/tests/utils.test.js b/packages/super-editor/src/components/context-menu/tests/utils.test.js index 86acee329b..92ccf3e3d0 100644 --- a/packages/super-editor/src/components/context-menu/tests/utils.test.js +++ b/packages/super-editor/src/components/context-menu/tests/utils.test.js @@ -61,6 +61,10 @@ import { undoDepth, redoDepth } from 'prosemirror-history'; import { yUndoPluginKey } from 'y-prosemirror'; import { isCellSelection as isCellSelectionMock } from '@extensions/table/tableHelpers/isCellSelection.js'; import { selectedRect as selectedRectMock } from 'prosemirror-tables'; +import { + collectTrackedChanges, + collectTrackedChangesForContext, +} from '@extensions/track-changes/permission-helpers.js'; // Get the mocked functions const mockReadFromClipboard = vi.mocked(readFromClipboard); @@ -68,6 +72,8 @@ const mockSelectionHasNodeOrMark = vi.mocked(selectionHasNodeOrMark); const mockUndoDepth = vi.mocked(undoDepth); const mockRedoDepth = vi.mocked(redoDepth); const mockYUndoPluginKeyGetState = vi.mocked(yUndoPluginKey.getState); +const mockCollectTrackedChanges = vi.mocked(collectTrackedChanges); +const mockCollectTrackedChangesForContext = vi.mocked(collectTrackedChangesForContext); describe('utils.js', () => { let mockEditor; @@ -84,6 +90,10 @@ describe('utils.js', () => { mockUndoDepth.mockReturnValue(1); mockRedoDepth.mockReturnValue(1); mockYUndoPluginKeyGetState.mockReturnValue({ undoManager: { undoStack: [1], redoStack: [1] } }); + mockCollectTrackedChanges.mockReset(); + mockCollectTrackedChangesForContext.mockReset(); + mockCollectTrackedChanges.mockReturnValue([]); + mockCollectTrackedChangesForContext.mockReturnValue([]); // Create editor with default configuration mockEditor = createMockEditor({ @@ -280,6 +290,32 @@ describe('utils.js', () => { expect(context.selectedText).toBe('selected text'); }); + it('uses selection-scoped tracked changes for right-click actions inside an expanded selection', async () => { + const mockEvent = { clientX: 300, clientY: 400 }; + + mockSelectionHasNodeOrMark.mockReturnValue(false); + mockEditor.view.state.selection.empty = false; + mockEditor.view.state.selection.from = 10; + mockEditor.view.state.selection.to = 15; + mockEditor.view.posAtCoords.mockReturnValue({ pos: 12 }); + mockEditor.view.state.doc.nodeAt.mockReturnValue({ type: { name: 'text' } }); + mockEditor.view.state.doc.textBetween.mockReturnValue('selected text'); + mockEditor.view.state.doc.resolve.mockReturnValue({ + marks: vi.fn(() => []), + nodeBefore: null, + nodeAfter: null, + }); + + await getEditorContext(mockEditor, mockEvent); + + expect(mockCollectTrackedChanges).toHaveBeenCalledWith({ + state: mockEditor.view.state, + from: 10, + to: 15, + }); + expect(mockCollectTrackedChangesForContext).not.toHaveBeenCalled(); + }); + it('should detect tracked change marks directly at the resolved cursor position', async () => { const mockEvent = { clientX: 150, clientY: 250 }; const trackFormatMark = { type: { name: 'trackFormat' }, attrs: { id: 'track-format-1' } }; diff --git a/packages/super-editor/src/components/context-menu/utils.js b/packages/super-editor/src/components/context-menu/utils.js index 836a0f610c..09025dcc8c 100644 --- a/packages/super-editor/src/components/context-menu/utils.js +++ b/packages/super-editor/src/components/context-menu/utils.js @@ -168,8 +168,13 @@ export async function getEditorContext(editor, event) { const isTrackedChange = activeMarks.includes('trackInsert') || activeMarks.includes('trackDelete') || activeMarks.includes('trackFormat'); - const trackedChanges = - event && pos !== null + // If there is an expanded selection and the right-click happened inside + // that selection, use collectTrackedChanges for the full selection range + const shouldUseSelectionTrackedChanges = + event && pos !== null ? hasExpandedSelection(selection) && selectionContainsPos(selection, pos) : hasSelection; + const trackedChanges = shouldUseSelectionTrackedChanges + ? collectTrackedChanges({ state, from: selection.from, to: selection.to }) + : event && pos !== null ? collectTrackedChangesForContext({ state, pos, trackedChangeId }) : collectTrackedChanges({ state, from: selection.from, to: selection.to }); From e9011434b89a12c44d7e558622bd32f02df4f98f Mon Sep 17 00:00:00 2001 From: Clarence Palmer Date: Tue, 3 Mar 2026 09:36:59 -0800 Subject: [PATCH 4/7] fix: resolve id for doc or cache --- .../track-changes-toolbar-commands.test.js | 66 +++++++++++++++++++ .../extensions/track-changes/track-changes.js | 14 +++- .../superdoc/src/stores/comments-store.js | 4 ++ 3 files changed, 81 insertions(+), 3 deletions(-) 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 236c16f60b..26b2136caf 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 @@ -11,20 +11,27 @@ 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(); @@ -42,6 +49,7 @@ describe('Track Changes Shared Resolution Commands', () => { }; mockCollectTrackedChanges.mockReturnValue([]); + mockGetTrackChanges.mockReturnValue([]); }); describe('acceptTrackedChangeFromToolbar', () => { @@ -350,6 +358,35 @@ describe('Track Changes Shared Resolution Commands', () => { 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', () => { @@ -401,5 +438,34 @@ describe('Track Changes Shared Resolution Commands', () => { 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 c84520a989..df4cf6b1db 100644 --- a/packages/super-editor/src/extensions/track-changes/track-changes.js +++ b/packages/super-editor/src/extensions/track-changes/track-changes.js @@ -520,12 +520,16 @@ const getTrackedChangeActionSelection = ({ state, editor }) => { 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: Boolean( - resolvedTrackedChangeId && commentsPluginState?.trackedChanges?.[resolvedTrackedChangeId], - ), + hasKnownTrackedChangeId: hasTrackedChangeInCache || hasTrackedChangeInDocument, }; }; @@ -574,6 +578,8 @@ const resolveTrackedChangeAction = ({ 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); } @@ -604,6 +610,8 @@ const emitTrackedChangeCommentLifecycle = ({ editor, nextState, touchedChangeIds 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', diff --git a/packages/superdoc/src/stores/comments-store.js b/packages/superdoc/src/stores/comments-store.js index 4a2cb7104c..dc12e32846 100644 --- a/packages/superdoc/src/stores/comments-store.js +++ b/packages/superdoc/src/stores/comments-store.js @@ -337,6 +337,8 @@ 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 + // 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; @@ -356,6 +358,8 @@ export const useCommentsStore = defineStore('comments', () => { const existingTrackedChange = commentsList.value.find((comment) => comment.commentId === changeId); if (!existingTrackedChange) return; + // 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; From ade952e01d28355f715e47b0f4e5f58ef5ee3eaa Mon Sep 17 00:00:00 2001 From: Clarence Palmer Date: Tue, 3 Mar 2026 15:23:52 -0800 Subject: [PATCH 5/7] refactor: extract shared helper --- .../src/components/context-menu/utils.js | 5 +--- .../track-changes-toolbar-commands.test.js | 24 +++++++++++++++++++ .../extensions/track-changes/track-changes.js | 6 +---- .../super-editor/src/utils/selectionUtils.js | 9 +++++++ .../src/utils/selectionUtils.test.js | 19 +++++++++++++++ 5 files changed, 54 insertions(+), 9 deletions(-) create mode 100644 packages/super-editor/src/utils/selectionUtils.js create mode 100644 packages/super-editor/src/utils/selectionUtils.test.js diff --git a/packages/super-editor/src/components/context-menu/utils.js b/packages/super-editor/src/components/context-menu/utils.js index 09025dcc8c..07580e5d0a 100644 --- a/packages/super-editor/src/components/context-menu/utils.js +++ b/packages/super-editor/src/components/context-menu/utils.js @@ -9,6 +9,7 @@ import { } from '@extensions/track-changes/permission-helpers.js'; import { isList } from '@core/commands/list-helpers'; import { isCellSelection } from '@extensions/table/tableHelpers/isCellSelection.js'; +import { hasExpandedSelection } from '@utils/selectionUtils.js'; import { selectedRect } from 'prosemirror-tables'; /** * Get props by item id @@ -217,10 +218,6 @@ export async function getEditorContext(editor, event) { }; } -function hasExpandedSelection(selection) { - return Number.isFinite(selection?.from) && Number.isFinite(selection?.to) && selection.from !== selection.to; -} - function selectionContainsPos(selection, pos) { return hasExpandedSelection(selection) && Number.isFinite(pos) && pos >= selection.from && pos <= selection.to; } 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 26b2136caf..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 @@ -185,6 +185,30 @@ describe('Track Changes Shared Resolution Commands', () => { 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(); + }); }); describe('rejectTrackedChangeFromToolbar', () => { 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 df4cf6b1db..d32bedf845 100644 --- a/packages/super-editor/src/extensions/track-changes/track-changes.js +++ b/packages/super-editor/src/extensions/track-changes/track-changes.js @@ -10,6 +10,7 @@ import { markInsertion } from './trackChangesHelpers/markInsertion.js'; import { collectTrackedChanges, isTrackedChangeActionAllowed } from './permission-helpers.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', @@ -498,11 +499,6 @@ const TRACKED_CHANGE_MARKS = [TrackDeleteMarkName, TrackInsertMarkName, TrackFor const getTrackedMark = (node) => node?.marks?.find((mark) => TRACKED_CHANGE_MARKS.includes(mark.type.name)) ?? null; -const hasExpandedSelection = (selection) => { - if (!selection) return false; - return selection.from !== selection.to; -}; - const getTrackedChangeActionSelection = ({ state, editor }) => { const currentSelection = state?.selection; if (hasExpandedSelection(currentSelection)) { 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); + }); +}); From 2013876bb5c7a6401c6c291280a402def0b8d167 Mon Sep 17 00:00:00 2001 From: Clarence Palmer Date: Tue, 3 Mar 2026 15:42:33 -0800 Subject: [PATCH 6/7] test: more extractions for helpers in tests --- tests/behavior/helpers/editor-interactions.ts | 35 ++++++++++ tests/behavior/helpers/tracked-changes.ts | 39 +++++++++++ .../tracked-change-partial-resolution.spec.ts | 64 +------------------ tests/behavior/tests/slash-menu/paste.spec.ts | 23 +------ .../tracked-change-partial-resolution.spec.ts | 63 +----------------- 5 files changed, 79 insertions(+), 145 deletions(-) create mode 100644 tests/behavior/helpers/editor-interactions.ts 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} Resolves when the click has been dispatched. + */ +export async function rightClickAtDocPos(page: Page, pos: number): Promise { + const coords = await page.evaluate((targetPos) => { + const editor = (window as any).editor; + const rect = editor?.coordsAtPos?.(targetPos); + if (!rect) return null; + return { + left: Number(rect.left), + right: Number(rect.right), + top: Number(rect.top), + bottom: Number(rect.bottom), + }; + }, pos); + + if (!coords) { + throw new Error(`Could not resolve coordinates for document position ${pos}`); + } + + const x = Math.min(Math.max(coords.left + 1, coords.left), Math.max(coords.right - 1, coords.left + 1)); + const y = (coords.top + coords.bottom) / 2; + await page.mouse.click(x, y, { button: 'right' }); +} diff --git a/tests/behavior/helpers/tracked-changes.ts b/tests/behavior/helpers/tracked-changes.ts index 48f6047621..1736a59b7c 100644 --- a/tests/behavior/helpers/tracked-changes.ts +++ b/tests/behavior/helpers/tracked-changes.ts @@ -1,5 +1,11 @@ import type { Page } from '@playwright/test'; +type InsertTrackedChangeOptions = { + from: number; + to: number; + text: string; +}; + /** * Reject all tracked changes in the document via document-api. */ @@ -12,3 +18,36 @@ export async function rejectAllTrackedChanges(page: Page): Promise { decide({ decision: 'reject', target: { scope: 'all' } }); }); } + +export async function insertTrackedChange(page: Page, options: InsertTrackedChangeOptions): Promise { + await page.evaluate((payload) => { + (window as any).editor.commands.insertTrackedChange({ + ...payload, + user: { name: 'Track Tester', email: 'track@example.com' }, + }); + }, options); +} + +export async function getMarkedText(page: Page, markName: string): Promise { + return page.evaluate((name) => { + let text = ''; + const doc = (window as any).editor.state.doc; + + doc.descendants((node: any) => { + if (!node.isText) return; + if (node.marks.some((mark: any) => mark.type.name === name)) { + text += node.text ?? ''; + } + }); + + return text; + }, markName); +} + +export async function getSelectedText(page: Page): Promise { + return page.evaluate(() => { + const { from, to, empty } = (window as any).editor.state.selection; + if (empty) return ''; + return (window as any).editor.state.doc.textBetween(from, to); + }); +} diff --git a/tests/behavior/tests/comments/tracked-change-partial-resolution.spec.ts b/tests/behavior/tests/comments/tracked-change-partial-resolution.spec.ts index 217038d3e2..fc7c64d928 100644 --- a/tests/behavior/tests/comments/tracked-change-partial-resolution.spec.ts +++ b/tests/behavior/tests/comments/tracked-change-partial-resolution.spec.ts @@ -1,5 +1,7 @@ import { test, expect } from '../../fixtures/superdoc.js'; import { getDocumentText } from '../../helpers/document-api.js'; +import { rightClickAtDocPos } from '../../helpers/editor-interactions.js'; +import { getMarkedText, getSelectedText, insertTrackedChange } from '../../helpers/tracked-changes.js'; test.use({ config: { toolbar: 'full', comments: 'panel', trackChanges: true, showSelection: true } }); @@ -7,68 +9,6 @@ const TRACK_TEXT = 'ABCDE'; const PARTIAL_TEXT = 'BC'; const ACCEPT_TRACKED_CHANGES_BUTTON = 'Accept tracked changes'; -async function insertTrackedChange( - page: import('@playwright/test').Page, - options: { - from: number; - to: number; - text: string; - }, -) { - await page.evaluate((payload) => { - (window as any).editor.commands.insertTrackedChange({ - ...payload, - user: { name: 'Track Tester', email: 'track@example.com' }, - }); - }, options); -} - -async function getMarkedText(page: import('@playwright/test').Page, markName: string): Promise { - return page.evaluate((name) => { - let text = ''; - const doc = (window as any).editor.state.doc; - - doc.descendants((node: any) => { - if (!node.isText) return; - if (node.marks.some((mark: any) => mark.type.name === name)) { - text += node.text ?? ''; - } - }); - - return text; - }, markName); -} - -async function getSelectedText(page: import('@playwright/test').Page): Promise { - return page.evaluate(() => { - const { from, to, empty } = (window as any).editor.state.selection; - if (empty) return ''; - return (window as any).editor.state.doc.textBetween(from, to); - }); -} - -async function rightClickAtDocPos(page: import('@playwright/test').Page, pos: number) { - const coords = await page.evaluate((p) => { - const editor = (window as any).editor; - const rect = editor?.coordsAtPos?.(p); - if (!rect) return null; - return { - left: Number(rect.left), - right: Number(rect.right), - top: Number(rect.top), - bottom: Number(rect.bottom), - }; - }, pos); - - if (!coords) { - throw new Error(`Could not resolve coordinates for document position ${pos}`); - } - - const x = Math.min(Math.max(coords.left + 1, coords.left), Math.max(coords.right - 1, coords.left + 1)); - const y = (coords.top + coords.bottom) / 2; - await page.mouse.click(x, y, { button: 'right' }); -} - test('toolbar accept partially resolves a tracked insertion and updates the bubble text', async ({ superdoc }) => { await insertTrackedChange(superdoc.page, { from: 1, to: 1, text: TRACK_TEXT }); await superdoc.waitForStable(); diff --git a/tests/behavior/tests/slash-menu/paste.spec.ts b/tests/behavior/tests/slash-menu/paste.spec.ts index 1fcf4d9487..d977653e36 100644 --- a/tests/behavior/tests/slash-menu/paste.spec.ts +++ b/tests/behavior/tests/slash-menu/paste.spec.ts @@ -1,4 +1,5 @@ import { test, expect } from '../../fixtures/superdoc.js'; +import { rightClickAtDocPos } from '../../helpers/editor-interactions.js'; test.use({ config: { toolbar: 'full' } }); @@ -16,28 +17,6 @@ async function writeToClipboard(page: import('@playwright/test').Page, text: str await page.evaluate((t) => navigator.clipboard.writeText(t), text); } -async function rightClickAtDocPos(page: import('@playwright/test').Page, pos: number) { - const coords = await page.evaluate((p) => { - const editor = (window as any).editor; - const rect = editor?.coordsAtPos?.(p); - if (!rect) return null; - return { - left: Number(rect.left), - right: Number(rect.right), - top: Number(rect.top), - bottom: Number(rect.bottom), - }; - }, pos); - - if (!coords) { - throw new Error(`Could not resolve coordinates for document position ${pos}`); - } - - const x = Math.min(Math.max(coords.left + 1, coords.left), Math.max(coords.right - 1, coords.left + 1)); - const y = (coords.top + coords.bottom) / 2; - await page.mouse.click(x, y, { button: 'right' }); -} - test('right-click opens context menu and paste inserts clipboard text', async ({ superdoc }) => { await superdoc.type('Hello world'); await superdoc.newLine(); diff --git a/tests/visual/tests/behavior/tracked-change-partial-resolution.spec.ts b/tests/visual/tests/behavior/tracked-change-partial-resolution.spec.ts index ab359ecf28..89dd197b5e 100644 --- a/tests/visual/tests/behavior/tracked-change-partial-resolution.spec.ts +++ b/tests/visual/tests/behavior/tracked-change-partial-resolution.spec.ts @@ -1,4 +1,6 @@ import { test, expect } from '../fixtures/superdoc.ts'; +import { rightClickAtDocPos } from '../../../behavior/helpers/editor-interactions.js'; +import { getMarkedText, getSelectedText, insertTrackedChange } from '../../../behavior/helpers/tracked-changes.js'; test.use({ config: { @@ -11,72 +13,11 @@ test.use({ const TRACK_TEXT = 'ABCDE'; const PARTIAL_TEXT = 'BC'; -async function insertTrackedChange( - page: import('@playwright/test').Page, - options: { - from: number; - to: number; - text: string; - }, -) { - await page.evaluate((payload) => { - (window as any).editor.commands.insertTrackedChange({ - ...payload, - user: { name: 'Track Tester', email: 'track@example.com' }, - }); - }, options); -} - -async function getMarkedText(page: import('@playwright/test').Page, markName: string): Promise { - return page.evaluate((name) => { - let text = ''; - const doc = (window as any).editor.state.doc; - - doc.descendants((node: any) => { - if (!node.isText) return; - if (node.marks.some((mark: any) => mark.type.name === name)) { - text += node.text ?? ''; - } - }); - - return text; - }, markName); -} async function getDocumentText(page: import('@playwright/test').Page): Promise { return page.evaluate(() => (window as any).editor.doc.getText({})); } -async function getSelectedText(page: import('@playwright/test').Page): Promise { - return page.evaluate(() => { - const { from, to, empty } = (window as any).editor.state.selection; - if (empty) return ''; - return (window as any).editor.state.doc.textBetween(from, to); - }); -} - -async function rightClickAtDocPos(page: import('@playwright/test').Page, pos: number) { - const coords = await page.evaluate((p) => { - const editor = (window as any).editor; - const rect = editor?.coordsAtPos?.(p); - if (!rect) return null; - return { - left: Number(rect.left), - right: Number(rect.right), - top: Number(rect.top), - bottom: Number(rect.bottom), - }; - }, pos); - - if (!coords) { - throw new Error(`Could not resolve coordinates for document position ${pos}`); - } - - const x = Math.min(Math.max(coords.left + 1, coords.left), Math.max(coords.right - 1, coords.left + 1)); - const y = (coords.top + coords.bottom) / 2; - await page.mouse.click(x, y, { button: 'right' }); -} - test('@behavior toolbar accept partially resolves a tracked insertion', async ({ superdoc }) => { await insertTrackedChange(superdoc.page, { from: 1, to: 1, text: TRACK_TEXT }); await superdoc.waitForStable(); From 04499e4983f71b3f1eb5400984f650d779c43a7b Mon Sep 17 00:00:00 2001 From: Clarence Palmer Date: Tue, 3 Mar 2026 15:48:33 -0800 Subject: [PATCH 7/7] chore: add clarifying comment --- .../super-editor/src/extensions/track-changes/track-changes.js | 1 + 1 file changed, 1 insertion(+) 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 d32bedf845..a946f65a57 100644 --- a/packages/super-editor/src/extensions/track-changes/track-changes.js +++ b/packages/super-editor/src/extensions/track-changes/track-changes.js @@ -641,6 +641,7 @@ const dispatchTrackedChangeResolution = ({ state, tr, dispatch, editor, touchedC return true; } + // Apply tr locally to get nextState for comment lifecycle; dispatch(tr) updates the editor afterward. const nextState = state.apply(tr); if (dispatch) {