From 019d00a3aa8bc8b08052b3add7a285fc2291b73a Mon Sep 17 00:00:00 2001 From: Gabriel Chittolina Date: Fri, 5 Jun 2026 18:10:02 -0300 Subject: [PATCH] fix: deletion handling across paragraph boundaries --- .../track-changes-overlap.test.js | 147 ++++++++++++++++++ .../extensions/track-changes/track-changes.js | 4 +- .../trackChangesHelpers/replaceStep.js | 49 +++++- .../CommentsLayer/CommentDialog.test.js | 45 ++++++ .../CommentsLayer/CommentDialog.vue | 41 +++-- .../components/CommentsLayer/use-comment.js | 11 +- .../superdoc/src/stores/comments-store.js | 5 + .../src/stores/comments-store.test.js | 1 + 8 files changed, 284 insertions(+), 19 deletions(-) diff --git a/packages/super-editor/src/editors/v1/extensions/track-changes/track-changes-overlap.test.js b/packages/super-editor/src/editors/v1/extensions/track-changes/track-changes-overlap.test.js index a680c5dccf..1215d3c463 100644 --- a/packages/super-editor/src/editors/v1/extensions/track-changes/track-changes-overlap.test.js +++ b/packages/super-editor/src/editors/v1/extensions/track-changes/track-changes-overlap.test.js @@ -19,6 +19,7 @@ import { beforeEach, afterEach, describe, expect, it } from 'vitest'; import { TextSelection } from 'prosemirror-state'; import { TrackInsertMarkName, TrackDeleteMarkName } from './constants.js'; import { initTestEditor } from '@tests/helpers/helpers.js'; +import { handleBackspace, handleDelete } from '@core/extensions/keymap.js'; import { buildReviewGraph, CanonicalChangeType } from './review-model/review-graph.js'; const ALICE = { name: 'Alice', email: 'alice@example.com' }; @@ -215,3 +216,149 @@ describe('overlap wired: insertTrackedChange delegates to compiler', () => { expect(ok).toBe(false); }); }); + +describe('overlap wired: deletion spanning a paragraph boundary (SD-3386)', () => { + let editor; + afterEach(() => { + editor?.destroy(); + editor = null; + }); + + const TWO_PARAGRAPHS = '

First line of text

Second line of text

'; + + // Selection from the start of paragraph 1 (pos 1) through the middle of + // paragraph 2 ("Second|" → 21 + 6 = 27). deleteSelection routes through + // deleteRange, which emits a ReplaceStep whose slice is a structural + // paragraph re-join shell with no inline content. + const deleteAcrossBoundary = () => { + editor.commands.command(({ tr, dispatch }) => { + tr.setSelection(TextSelection.create(tr.doc, 1, 27)); + tr.deleteSelection(); + if (dispatch) dispatch(tr); + return true; + }); + }; + + const paragraphTexts = (doc) => { + const texts = []; + doc.forEach((node) => texts.push(node.textContent)); + return texts; + }; + + it('marks the range deleted without splitting the trailing paragraph', () => { + editor = setup(ALICE, TWO_PARAGRAPHS); + deleteAcrossBoundary(); + + // No spurious paragraph split: still exactly two paragraphs with the + // original text (the deletion is only marked, not applied). + expect(editor.state.doc.childCount).toBe(2); + expect(paragraphTexts(editor.state.doc)).toEqual(['First line of text', 'Second line of text']); + + // One logical deletion — not a replacement wrapping an empty block shell. + const graph = graphFor(editor); + expect(graph.changes.size).toBe(1); + const change = Array.from(graph.changes.values())[0]; + expect(change.type).toBe(CanonicalChangeType.Deletion); + }); + + it('accept removes the marked content and all tracked marks', () => { + editor = setup(ALICE, TWO_PARAGRAPHS); + deleteAcrossBoundary(); + + const ok = editor.commands.acceptTrackedChangesBetween(0, editor.state.doc.content.size); + expect(ok).toBe(true); + expect(editor.storage.trackChanges?.lastDecisionFailure ?? null).toBeNull(); + + // Deleted content is gone, remainder of paragraph 2 survives. + expect(paragraphTexts(editor.state.doc)).toEqual(['', 'ond line of text']); + + // No tracked-change state (marks/decorations) survives the decision. + const graph = graphFor(editor); + expect(graph.changes.size).toBe(0); + }); + + it('reject restores the content and removes all tracked marks', () => { + editor = setup(ALICE, TWO_PARAGRAPHS); + deleteAcrossBoundary(); + + const graph = graphFor(editor); + const changeId = Array.from(graph.changes.keys())[0]; + const ok = editor.commands.rejectTrackedChangeById(changeId); + expect(ok).toBe(true); + expect(editor.storage.trackChanges?.lastDecisionFailure ?? null).toBeNull(); + + // Original two-paragraph content is intact and unmarked. + expect(editor.state.doc.childCount).toBe(2); + expect(paragraphTexts(editor.state.doc)).toEqual(['First line of text', 'Second line of text']); + expect(graphFor(editor).changes.size).toBe(0); + + let hasTrackedMarks = false; + editor.state.doc.descendants((node) => { + if (node.marks.some((m) => m.type.name === TrackInsertMarkName || m.type.name === TrackDeleteMarkName)) { + hasTrackedMarks = true; + } + }); + expect(hasTrackedMarks).toBe(false); + }); +}); + +describe('overlap wired: cross-paragraph deletion through the real keymap path (SD-3386)', () => { + let editor; + afterEach(() => { + editor?.destroy(); + editor = null; + }); + + // The keymap path differs from a raw command dispatch: handleBackspace tags + // the transaction with inputType 'deleteContentBackward' and deleteSelection + // sets the post-step selection, which can land inside the structural shell + // slice that the tracked compile never inserts. Mapping that position back + // through a falsely-mirrored invert map produced `Position NaN` and the + // dispatch fallback then applied the deletion untracked. + const setupTwoLines = () => { + ({ editor } = initTestEditor({ + mode: 'text', + content: '

Line 1

Line 2

', + user: ALICE, + trackedChanges: {}, + })); + editor.commands.enableTrackChanges(); + + let p2TextStart = null; + editor.state.doc.descendants((node, pos) => { + if (node.isText && node.text === 'Line 2') p2TextStart = pos; + }); + // User selection from the start of "Line 1" through "Lin" of line 2, + // dispatched on its own like a mouse selection. + editor.commands.command(({ tr, dispatch }) => { + tr.setSelection(TextSelection.create(tr.doc, 1, p2TextStart + 3)); + if (dispatch) dispatch(tr); + return true; + }); + }; + + const paragraphTexts = (doc) => { + const texts = []; + doc.forEach((node) => texts.push(node.textContent)); + return texts; + }; + + it.each([ + ['Backspace', handleBackspace], + ['Delete', handleDelete], + ])('%s tracks the deletion instead of hard-deleting', (_label, handler) => { + setupTwoLines(); + const handled = handler(editor); + expect(handled).toBe(true); + + // Content survives as a tracked deletion — never hard-deleted. + expect(paragraphTexts(editor.state.doc)).toEqual(['Line 1', 'Line 2']); + const graph = graphFor(editor); + expect(graph.changes.size).toBe(1); + expect(Array.from(graph.changes.values())[0].type).toBe(CanonicalChangeType.Deletion); + + // Caret lands at the left edge of the tracked deletion, inside paragraph 1. + expect(editor.state.selection.empty).toBe(true); + expect(editor.state.selection.from).toBeLessThan(editor.state.doc.firstChild.nodeSize); + }); +}); diff --git a/packages/super-editor/src/editors/v1/extensions/track-changes/track-changes.js b/packages/super-editor/src/editors/v1/extensions/track-changes/track-changes.js index e839e0b236..10738b5adb 100644 --- a/packages/super-editor/src/editors/v1/extensions/track-changes/track-changes.js +++ b/packages/super-editor/src/editors/v1/extensions/track-changes/track-changes.js @@ -96,7 +96,9 @@ const dispatchReviewDecision = ({ editor, state, dispatch, decision, target }) = originalId: event.changeId, }).some(({ mark }) => mark.attrs?.splitFromId === event.changeId); if (successorsPresent) continue; - editor.emit('commentsUpdate', event); + // Carry the decision so resolved bubbles can report accepted vs + // rejected instead of assuming every resolution was an accept. + editor.emit('commentsUpdate', { ...event, decision }); } const touched = diff --git a/packages/super-editor/src/editors/v1/extensions/track-changes/trackChangesHelpers/replaceStep.js b/packages/super-editor/src/editors/v1/extensions/track-changes/trackChangesHelpers/replaceStep.js index 1e0e241fb5..c4199ad94f 100644 --- a/packages/super-editor/src/editors/v1/extensions/track-changes/trackChangesHelpers/replaceStep.js +++ b/packages/super-editor/src/editors/v1/extensions/track-changes/trackChangesHelpers/replaceStep.js @@ -136,6 +136,27 @@ const normalizeReplaceStepSingleCharDelete = ({ step, doc }) => { } }; +/** + * Whether a slice carries any inline leaf content (text, hard breaks, inline + * widgets). A slice without inline leaves is a purely structural shell — e.g. + * the paragraph re-join slice `deleteRange` produces for a deletion that spans + * a paragraph boundary (`` with open ends). + * + * @param {import('prosemirror-model').Slice} slice + * @returns {boolean} + */ +const sliceHasInlineLeafContent = (slice) => { + let found = false; + slice.content.descendants((node) => { + if (found) return false; + if (node.isInline && (node.isText || node.isLeaf)) { + found = true; + return false; + } + }); + return found; +}; + /** * Replace step. * @param {object} options Replace step options. @@ -259,6 +280,14 @@ const tryCompileStep = ({ if (!hasInlineContent) return { handled: false }; } + // A non-empty slice without inline leaf content is a structural re-join + // shell, not user-visible replacement content. `deleteRange` emits one for + // a deletion that spans a paragraph boundary; compiling it as a replacement + // would insert the empty block shell at the range end and split the + // trailing paragraph (SD-3386). Track it as a plain deletion instead. + const isStructuralShellDelete = + step.from !== step.to && step.slice.content.size > 0 && !sliceHasInlineLeafContent(step.slice); + // Build the intent. Pure inserts and pure deletes use the matching intent // type; mixed (text-replace) carries the original slice. let intent; @@ -274,7 +303,7 @@ const tryCompileStep = ({ source, preserveExistingReviewState, }); - } else if (step.from !== step.to && step.slice.content.size === 0) { + } else if (step.from !== step.to && (step.slice.content.size === 0 || isStructuralShellDelete)) { intent = makeTextDeleteIntent({ from: step.from, to: step.to, @@ -345,7 +374,17 @@ const tryCompileStep = ({ map.appendMap(invertStep.getMap()); const mirrorIndex = map.maps.length - 1; for (let i = beforeSteps; i < newTr.steps.length; i += 1) { - map.appendMap(newTr.steps[i].getMap(), mirrorIndex); + // Mirror pairing assumes the compiled steps re-apply the original + // step's position effect (the condensed insert in the replace path). + // A structural-shell delete compiles to mark steps only — pairing + // their empty maps as mirrors of the invert map corrupts position + // recovery (Position NaN) for positions inside the never-inserted + // shell, so append those without mirrors (SD-3386). + if (isStructuralShellDelete) { + map.appendMap(newTr.steps[i].getMap()); + } else { + map.appendMap(newTr.steps[i].getMap(), mirrorIndex); + } } } else { for (let i = beforeSteps; i < newTr.steps.length; i += 1) { @@ -380,7 +419,11 @@ const tryCompileStep = ({ // fall back to a shaped step the comments plugin already understands. meta.step = { slice: { content: { content: result.insertedNodes } } }; } - if (result.selection?.kind === 'near' && stepWasNormalized && !result.insertedMark) { + // Structural-shell deletes also need the explicit override: the original + // step's post-step selection can sit inside the shell slice that was never + // inserted, so mapping it back is meaningless — honor the compiler's caret + // hint (left edge of the tracked deletion) instead. + if (result.selection?.kind === 'near' && (stepWasNormalized || isStructuralShellDelete) && !result.insertedMark) { meta.selectionPos = result.selection.pos; } newTr.setMeta(TrackChangesBasePluginKey, meta); diff --git a/packages/superdoc/src/components/CommentsLayer/CommentDialog.test.js b/packages/superdoc/src/components/CommentsLayer/CommentDialog.test.js index 5574d352f1..ba5f71651e 100644 --- a/packages/superdoc/src/components/CommentsLayer/CommentDialog.test.js +++ b/packages/superdoc/src/components/CommentsLayer/CommentDialog.test.js @@ -767,6 +767,7 @@ describe('CommentDialog.vue', () => { email: superdocStoreStub.user.email, name: superdocStoreStub.user.name, superdoc: expect.any(Object), + decision: 'accept', }); expect(superdocStub.focus).toHaveBeenCalledTimes(1); @@ -778,6 +779,50 @@ describe('CommentDialog.vue', () => { expect(superdocStub.focus).toHaveBeenCalledTimes(2); }); + it('does not resolve the tracked-change thread when the decision fails (SD-3386)', async () => { + const { wrapper, baseComment } = await mountDialog({ + baseCommentOverrides: { + trackedChange: true, + trackedChangeType: 'trackDelete', + trackedChangeText: 'Removed', + }, + }); + commentsStoreStub.decideTrackedChangeFromSidebar.mockReturnValueOnce({ ok: true, success: false }); + + const header = wrapper.findComponent(CommentHeaderStub); + header.vm.$emit('reject'); + await nextTick(); + expect(baseComment.resolveComment).not.toHaveBeenCalled(); + }); + + it('labels a rejected tracked change as Rejected, not Accepted (SD-3386)', async () => { + const { wrapper } = await mountDialog({ + baseCommentOverrides: { + trackedChange: true, + trackedChangeType: 'trackDelete', + trackedChangeText: 'Removed', + resolvedTime: Date.now(), + trackedChangeDecision: 'reject', + }, + }); + + expect(wrapper.find('.resolved-badge').text()).toContain('Rejected'); + }); + + it('labels an accepted tracked change as Accepted', async () => { + const { wrapper } = await mountDialog({ + baseCommentOverrides: { + trackedChange: true, + trackedChangeType: 'trackInsert', + trackedChangeText: 'Added', + resolvedTime: Date.now(), + trackedChangeDecision: 'accept', + }, + }); + + expect(wrapper.find('.resolved-badge').text()).toContain('Accepted'); + }); + it('renders hyperlink additions without a format label', async () => { const { wrapper } = await mountDialog({ baseCommentOverrides: { diff --git a/packages/superdoc/src/components/CommentsLayer/CommentDialog.vue b/packages/superdoc/src/components/CommentsLayer/CommentDialog.vue index 6f04259edb..1fc9ea89a7 100644 --- a/packages/superdoc/src/components/CommentsLayer/CommentDialog.vue +++ b/packages/superdoc/src/components/CommentsLayer/CommentDialog.vue @@ -279,7 +279,8 @@ const isDialogActive = computed(() => { /* ── Step 1: Resolved badge ── */ const resolvedBadgeLabel = computed(() => { if (!props.comment.resolvedTime) return null; - return props.comment.trackedChange ? 'Accepted' : 'Resolved'; + if (!props.comment.trackedChange) return 'Resolved'; + return props.comment.trackedChangeDecision === 'reject' ? 'Rejected' : 'Accepted'; }); /* ── Pending new comment (brand-new, not a reply) ── */ @@ -597,26 +598,31 @@ const handleAddComment = () => { const handleReject = () => { const customHandler = proxy.$superdoc.config.onTrackedChangeBubbleReject; + // Custom handlers always resolve so the bubble disappears from + // getFloatingComments (SD-2049). The internal decision path only resolves + // when the decision actually applied — otherwise the tracked marks are + // still in the document and the thread must stay open (SD-3386). + let decisionApplied = true; if (props.comment.trackedChange && typeof customHandler === 'function') { customHandler(props.comment, proxy.$superdoc.activeEditor); } else if (props.comment.trackedChange) { - commentsStore.decideTrackedChangeFromSidebar({ + const result = commentsStore.decideTrackedChangeFromSidebar({ superdoc: proxy.$superdoc, comment: props.comment, decision: 'reject', }); + decisionApplied = Boolean(result?.ok) && result?.success !== false; } else { commentsStore.deleteComment({ superdoc: proxy.$superdoc, commentId: props.comment.commentId }); } - // Always resolve tracked changes so resolvedTime is set and the bubble - // disappears from getFloatingComments — even when a custom handler is used (SD-2049). - if (props.comment.trackedChange) { + if (props.comment.trackedChange && decisionApplied) { props.comment.resolveComment({ id: superdocStore.user.id, email: superdocStore.user.email, name: superdocStore.user.name, superdoc: proxy.$superdoc, + decision: 'reject', }); } @@ -632,26 +638,33 @@ const handleReject = () => { const handleResolve = () => { const customHandler = proxy.$superdoc.config.onTrackedChangeBubbleAccept; + // Custom handlers always resolve so the bubble disappears from + // getFloatingComments (SD-2049). The internal decision path only resolves + // when the decision actually applied — otherwise the tracked marks are + // still in the document and the thread must stay open (SD-3386). + let decisionApplied = true; if (props.comment.trackedChange && typeof customHandler === 'function') { customHandler(props.comment, proxy.$superdoc.activeEditor); } else { if (props.comment.trackedChange) { - commentsStore.decideTrackedChangeFromSidebar({ + const result = commentsStore.decideTrackedChangeFromSidebar({ superdoc: proxy.$superdoc, comment: props.comment, decision: 'accept', }); + decisionApplied = Boolean(result?.ok) && result?.success !== false; } } - // Always resolve so resolvedTime is set and the bubble disappears - // from getFloatingComments — even when a custom handler is used (SD-2049). - props.comment.resolveComment({ - id: superdocStore.user.id, - email: superdocStore.user.email, - name: superdocStore.user.name, - superdoc: proxy.$superdoc, - }); + if (decisionApplied) { + props.comment.resolveComment({ + id: superdocStore.user.id, + email: superdocStore.user.email, + name: superdocStore.user.name, + superdoc: proxy.$superdoc, + ...(props.comment.trackedChange ? { decision: 'accept' } : {}), + }); + } // Always cleanup the dialog state nextTick(() => { diff --git a/packages/superdoc/src/components/CommentsLayer/use-comment.js b/packages/superdoc/src/components/CommentsLayer/use-comment.js index 8f87f316b2..d420e954a3 100644 --- a/packages/superdoc/src/components/CommentsLayer/use-comment.js +++ b/packages/superdoc/src/components/CommentsLayer/use-comment.js @@ -102,6 +102,9 @@ export default function useComment(params) { const resolvedById = ref(params.resolvedById || null); const resolvedByEmail = ref(params.resolvedByEmail || null); const resolvedByName = ref(params.resolvedByName || null); + // For tracked-change threads: which decision resolved the thread + // ('accept' | 'reject'). Null for plain comments and legacy payloads. + const trackedChangeDecision = ref(params.trackedChangeDecision || null); /** * Mark this conversation as resolved with UTC date @@ -109,14 +112,18 @@ export default function useComment(params) { * @param {String} id The actor id of the user marking this conversation as done * @param {String} email The email of the user marking this conversation as done * @param {String} name The name of the user marking this conversation as done + * @param {'accept' | 'reject'} [decision] The tracked-change decision that resolved this thread * @returns {void} */ - const resolveComment = ({ id, email, name, superdoc }) => { + const resolveComment = ({ id, email, name, superdoc, decision }) => { if (resolvedTime.value) return; resolvedTime.value = Date.now(); resolvedById.value = id ?? null; resolvedByEmail.value = email; resolvedByName.value = name; + if (decision === 'accept' || decision === 'reject') { + trackedChangeDecision.value = decision; + } const emitData = { type: comments_module_events.RESOLVED, comment: getValues() }; propagateUpdate(superdoc, emitData); @@ -318,6 +325,7 @@ export default function useComment(params) { trackedChangeStoryKind: trackedChangeStoryKind.value, trackedChangeStoryLabel: trackedChangeStoryLabel.value, trackedChangeAnchorKey: trackedChangeAnchorKey.value, + trackedChangeDecision: trackedChangeDecision.value, deletedText: deletedText.value, resolvedTime: resolvedTime.value, resolvedById: resolvedById.value, @@ -360,6 +368,7 @@ export default function useComment(params) { trackedChangeStoryKind, trackedChangeStoryLabel, trackedChangeAnchorKey, + trackedChangeDecision, resolvedTime, resolvedById, resolvedByEmail, diff --git a/packages/superdoc/src/stores/comments-store.js b/packages/superdoc/src/stores/comments-store.js index 1bd11e2e74..2d4119a493 100644 --- a/packages/superdoc/src/stores/comments-store.js +++ b/packages/superdoc/src/stores/comments-store.js @@ -297,6 +297,7 @@ export const useCommentsStore = defineStore('comments', () => { resolvedById: comment.resolvedById ?? null, resolvedByEmail: comment.resolvedByEmail ?? null, resolvedByName: comment.resolvedByName ?? null, + trackedChangeDecision: comment.trackedChangeDecision ?? null, }); } // Sets the resolved state to null so it can be restored in the comments sidebar @@ -304,6 +305,7 @@ export const useCommentsStore = defineStore('comments', () => { comment.resolvedById = null; comment.resolvedByEmail = null; comment.resolvedByName = null; + comment.trackedChangeDecision = null; }; const restoreResolvedMetadata = (comment) => { @@ -315,6 +317,7 @@ export const useCommentsStore = defineStore('comments', () => { comment.resolvedById = snapshot.resolvedById ?? null; comment.resolvedByEmail = snapshot.resolvedByEmail ?? null; comment.resolvedByName = snapshot.resolvedByName ?? null; + comment.trackedChangeDecision = snapshot.trackedChangeDecision ?? null; return true; }; @@ -848,6 +851,7 @@ export const useCommentsStore = defineStore('comments', () => { id: params.resolvedById ?? superdoc?.user?.id ?? null, email: params.resolvedByEmail ?? superdoc?.user?.email ?? null, name: params.resolvedByName ?? superdoc?.user?.name ?? null, + decision: params.decision ?? null, superdoc, }; @@ -1588,6 +1592,7 @@ export const useCommentsStore = defineStore('comments', () => { comment.resolvedById = resolutionSnapshot.resolvedById ?? null; comment.resolvedByEmail = resolutionSnapshot.resolvedByEmail ?? null; comment.resolvedByName = resolutionSnapshot.resolvedByName ?? null; + comment.trackedChangeDecision = resolutionSnapshot.trackedChangeDecision ?? null; restoredComments.push(comment); return true; } diff --git a/packages/superdoc/src/stores/comments-store.test.js b/packages/superdoc/src/stores/comments-store.test.js index d1e66c2472..ee2731f79b 100644 --- a/packages/superdoc/src/stores/comments-store.test.js +++ b/packages/superdoc/src/stores/comments-store.test.js @@ -676,6 +676,7 @@ describe('comments-store', () => { id: 'reviewer-id', email: 'reviewer@example.com', name: 'Reviewer', + decision: null, superdoc, }); expect(existingComment.resolvedTime).not.toBeNull();