From 48e4f64137fea0790c9075253c7eef179059c030 Mon Sep 17 00:00:00 2001 From: Clarence Palmer Date: Sat, 7 Mar 2026 22:06:52 -0800 Subject: [PATCH 1/3] fix(track-changes): update tests and logic for handling adjacent tracked changes with different IDs --- .../track-changes-extension.test.js | 56 ++++-- .../extensions/track-changes/track-changes.js | 33 +--- ...-adjacent-different-ids-resolution.spec.ts | 165 ++++++++++++++++++ 3 files changed, 209 insertions(+), 45 deletions(-) create mode 100644 tests/behavior/tests/comments/tracked-change-adjacent-different-ids-resolution.spec.ts 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 4b61286d56..457569b28c 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 @@ -989,7 +989,7 @@ describe('TrackChanges extension commands', () => { expect(rejectSpy).toHaveBeenNthCalledWith(3, 3, 4); }); - it('acceptTrackedChangeById chains contiguous same-id insertions before linking complementary deletions', () => { + it('acceptTrackedChangeById resolves contiguous same-id insertions without pulling in adjacent different-id deletions', () => { const italicMark = schema.marks.italic.create(); const deletionMark = schema.marks[TrackDeleteMarkName].create({ id: 'del-id' }); const insertionId = 'shared-id'; @@ -1012,10 +1012,9 @@ describe('TrackChanges extension commands', () => { }); expect(result).toBe(true); - expect(acceptSpy).toHaveBeenCalledTimes(3); + expect(acceptSpy).toHaveBeenCalledTimes(2); expect(acceptSpy).toHaveBeenNthCalledWith(1, 4, 5); expect(acceptSpy).toHaveBeenNthCalledWith(2, 5, 6); - expect(acceptSpy).toHaveBeenNthCalledWith(3, 1, 4); const rejectSpy = vi.fn().mockReturnValue(true); const rejectResult = commands.rejectTrackedChangeById(insertionId)({ @@ -1025,13 +1024,12 @@ describe('TrackChanges extension commands', () => { }); expect(rejectResult).toBe(true); - expect(rejectSpy).toHaveBeenCalledTimes(3); + expect(rejectSpy).toHaveBeenCalledTimes(2); expect(rejectSpy).toHaveBeenNthCalledWith(1, 4, 5); expect(rejectSpy).toHaveBeenNthCalledWith(2, 5, 6); - expect(rejectSpy).toHaveBeenNthCalledWith(3, 1, 4); }); - it('acceptTrackedChangeById and rejectTrackedChangeById SHOULD link deletion-insertion pairs', () => { + it('acceptTrackedChangeById and rejectTrackedChangeById should NOT link adjacent deletion-insertion pairs with different ids', () => { const deletionMark = schema.marks[TrackDeleteMarkName].create({ id: 'del-id' }); const insertionMark = schema.marks[TrackInsertMarkName].create({ id: 'ins-id' }); const paragraph = schema.nodes.paragraph.create(null, [ @@ -1050,10 +1048,8 @@ describe('TrackChanges extension commands', () => { }); expect(result).toBe(true); - // Should resolve both the insertion and the linked deletion - expect(acceptSpy).toHaveBeenCalledTimes(2); - expect(acceptSpy).toHaveBeenNthCalledWith(1, 4, 7); - expect(acceptSpy).toHaveBeenNthCalledWith(2, 1, 4); + expect(acceptSpy).toHaveBeenCalledTimes(1); + expect(acceptSpy).toHaveBeenCalledWith(4, 7); const rejectSpy = vi.fn().mockReturnValue(true); const rejectResult = commands.rejectTrackedChangeById('ins-id')({ @@ -1062,10 +1058,44 @@ describe('TrackChanges extension commands', () => { commands: { rejectTrackedChangesBetween: rejectSpy }, }); expect(rejectResult).toBe(true); - // Should resolve both the insertion and the linked deletion + expect(rejectSpy).toHaveBeenCalledTimes(1); + expect(rejectSpy).toHaveBeenCalledWith(4, 7); + }); + + it('acceptTrackedChangeById and rejectTrackedChangeById should still link adjacent deletion-insertion pairs with the same id', () => { + const sharedId = 'replace-id'; + const deletionMark = schema.marks[TrackDeleteMarkName].create({ id: sharedId }); + const insertionMark = schema.marks[TrackInsertMarkName].create({ id: sharedId }); + const paragraph = schema.nodes.paragraph.create(null, [ + schema.text('old', [deletionMark]), + schema.text('new', [insertionMark]), + ]); + const doc = schema.nodes.doc.create(null, paragraph); + const state = createState(doc); + + const acceptSpy = vi.fn().mockReturnValue(true); + const tr = state.tr; + const result = commands.acceptTrackedChangeById(sharedId)({ + state, + tr, + commands: { acceptTrackedChangesBetween: acceptSpy }, + }); + + expect(result).toBe(true); + expect(acceptSpy).toHaveBeenCalledTimes(2); + expect(acceptSpy).toHaveBeenNthCalledWith(1, 1, 4); + expect(acceptSpy).toHaveBeenNthCalledWith(2, 4, 7); + + const rejectSpy = vi.fn().mockReturnValue(true); + const rejectResult = commands.rejectTrackedChangeById(sharedId)({ + state, + tr, + commands: { rejectTrackedChangesBetween: rejectSpy }, + }); + expect(rejectResult).toBe(true); expect(rejectSpy).toHaveBeenCalledTimes(2); - expect(rejectSpy).toHaveBeenNthCalledWith(1, 4, 7); // insertion "new" - expect(rejectSpy).toHaveBeenNthCalledWith(2, 1, 4); // deletion "old" + expect(rejectSpy).toHaveBeenNthCalledWith(1, 1, 4); + expect(rejectSpy).toHaveBeenNthCalledWith(2, 4, 7); }); it('should NOT link changes separated by untracked content', () => { 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 a946f65a57..c8b899e1d4 100644 --- a/packages/super-editor/src/extensions/track-changes/track-changes.js +++ b/packages/super-editor/src/extensions/track-changes/track-changes.js @@ -667,26 +667,6 @@ const getChangesByIdToResolve = (state, id) => { const matchingChange = trackedChanges[changeIndex]; const matchingId = matchingChange.mark.attrs.id; - const getSegmentSize = ({ from, to }) => to - from; - const areDirectlyConnected = (left, right) => { - if (!left || !right) { - return false; - } - - if (left.to !== right.from) { - return false; - } - - const hasContentBetween = - state.doc.textBetween(left.from, right.to, '\n').length > getSegmentSize(left) + getSegmentSize(right); - - return !hasContentBetween; - }; - - const isComplementaryPair = (firstType, secondType) => - (firstType === TrackDeleteMarkName && secondType === TrackInsertMarkName) || - (firstType === TrackInsertMarkName && secondType === TrackDeleteMarkName); - const linkedBefore = []; const linkedAfter = []; @@ -702,15 +682,8 @@ const getChangesByIdToResolve = (state, id) => { break; } - const [left, right] = direction < 0 ? [neighbor, currentChange] : [currentChange, neighbor]; const sharesId = neighbor.mark.attrs.id === matchingId; - const complementary = isComplementaryPair(currentChange.mark.type.name, neighbor.mark.type.name); - - if (!sharesId && !areDirectlyConnected(left, right)) { - break; - } - - if (!sharesId && !complementary) { + if (!sharesId) { break; } @@ -718,10 +691,6 @@ const getChangesByIdToResolve = (state, id) => { currentIndex = neighborIndex; currentChange = neighbor; - - if (!sharesId) { - break; - } } }; diff --git a/tests/behavior/tests/comments/tracked-change-adjacent-different-ids-resolution.spec.ts b/tests/behavior/tests/comments/tracked-change-adjacent-different-ids-resolution.spec.ts new file mode 100644 index 0000000000..58f18a4c29 --- /dev/null +++ b/tests/behavior/tests/comments/tracked-change-adjacent-different-ids-resolution.spec.ts @@ -0,0 +1,165 @@ +import type { Page } from '@playwright/test'; +import { test, expect } from '../../fixtures/superdoc.js'; +import { + assertDocumentApiReady, + deleteText, + findFirstTextRange, + insertText, + listTrackChanges, +} from '../../helpers/document-api.js'; +import type { TextAddress, TextMutationReceipt, TrackChangeType } from '../../helpers/document-api.js'; + +test.use({ config: { toolbar: 'full', comments: 'panel', trackChanges: true } }); + +type SuperDocHarness = { + page: Page; + type: (text: string) => Promise; + waitForStable: () => Promise; +}; + +type TrackedSegment = { + from: number; + id: string; + text: string; + to: number; + type: TrackChangeType; +}; + +function requireTextTarget(target: TextAddress | null, pattern: string): TextAddress { + if (target != null) { + return target; + } + + throw new Error(`Could not find a text target for pattern "${pattern}".`); +} + +function assertMutationSucceeded( + operationName: string, + receipt: TextMutationReceipt, +): asserts receipt is Extract { + if (receipt.success) { + return; + } + + throw new Error(`${operationName} failed (${receipt.failure.code}): ${receipt.failure.message}`); +} + +async function createAdjacentTrackedDeleteAndInsert(superdoc: SuperDocHarness) { + await superdoc.type('AB'); + await superdoc.waitForStable(); + + const deleteTarget = requireTextTarget(await findFirstTextRange(superdoc.page, 'A'), 'A'); + const deleteReceipt = await deleteText(superdoc.page, { target: deleteTarget }, { changeMode: 'tracked' }); + assertMutationSucceeded('deleteText', deleteReceipt); + + const beforeB = requireTextTarget(await findFirstTextRange(superdoc.page, 'B'), 'B'); + const insertTarget: TextAddress = { + ...beforeB, + range: { + start: beforeB.range.start, + end: beforeB.range.start, + }, + }; + const insertReceipt = await insertText( + superdoc.page, + { value: 'X', target: insertTarget, type: 'text' }, + { changeMode: 'tracked' }, + ); + assertMutationSucceeded('insertText', insertReceipt); + + await expect.poll(async () => (await listTrackChanges(superdoc.page)).changes.length).toBe(2); + + const listed = await listTrackChanges(superdoc.page); + const deleteChange = listed.changes.find((change) => change.type === 'delete'); + const insertChange = listed.changes.find((change) => change.type === 'insert'); + const trackedSegments = await listTrackedSegments(superdoc.page); + const deleteSegment = trackedSegments.find((segment) => segment.type === 'delete'); + const insertSegment = trackedSegments.find((segment) => segment.type === 'insert'); + + expect(deleteChange).toBeTruthy(); + expect(insertChange).toBeTruthy(); + expect(deleteChange?.id).not.toBe(insertChange?.id); + expect(deleteSegment).toBeTruthy(); + expect(insertSegment).toBeTruthy(); + expect(deleteSegment?.id).not.toBe(insertSegment?.id); + expect(deleteSegment?.to).toBe(insertSegment?.from); + expect(trackedSegments).toEqual([ + { from: expect.any(Number), id: expect.any(String), to: expect.any(Number), type: 'delete', text: 'A' }, + { from: expect.any(Number), id: expect.any(String), to: expect.any(Number), type: 'insert', text: 'X' }, + ]); + + return { + deleteChange, + deleteSegment: deleteSegment!, + insertChange, + insertSegment: insertSegment!, + }; +} + +async function listTrackedSegments(page: Page): Promise { + return page.evaluate(() => { + const segments: Array<{ from: number; id: string; text: string; to: number; type: TrackChangeType }> = []; + const editor = (window as any).editor; + + editor.state.doc.descendants((node: any, pos: number) => { + if (!node?.isText || !node.text) { + return; + } + + const trackedMark = (node.marks ?? []).find((mark: any) => { + const name = mark.type?.name; + return name === 'trackInsert' || name === 'trackDelete'; + }); + + if (!trackedMark) { + return; + } + + segments.push({ + from: Number(pos), + id: String(trackedMark.attrs?.id ?? ''), + text: String(node.text), + to: Number(pos + node.nodeSize), + type: trackedMark.type.name === 'trackDelete' ? 'delete' : 'insert', + }); + }); + + return segments; + }); +} + +async function resolveTrackedChangeByRawId( + page: Page, + input: { action: 'accept' | 'reject'; id: string }, +): Promise { + await page.evaluate((payload) => { + const editor = (window as any).editor; + const command = + payload.action === 'accept' ? editor.commands.acceptTrackedChangeById : editor.commands.rejectTrackedChangeById; + command(payload.id); + }, input); +} + +test('accepting an adjacent tracked insertion by id keeps the separate tracked deletion', async ({ superdoc }) => { + await assertDocumentApiReady(superdoc.page); + + const { deleteSegment, insertSegment } = await createAdjacentTrackedDeleteAndInsert(superdoc); + + await resolveTrackedChangeByRawId(superdoc.page, { action: 'accept', id: insertSegment.id }); + + await expect + .poll(() => listTrackedSegments(superdoc.page)) + .toEqual([{ from: deleteSegment.from, id: deleteSegment.id, to: deleteSegment.to, type: 'delete', text: 'A' }]); +}); + +test('rejecting an adjacent tracked deletion by id keeps the separate tracked insertion', async ({ superdoc }) => { + await assertDocumentApiReady(superdoc.page); + + const { deleteSegment, insertSegment } = await createAdjacentTrackedDeleteAndInsert(superdoc); + + await resolveTrackedChangeByRawId(superdoc.page, { action: 'reject', id: deleteSegment.id }); + + await expect + .poll(() => listTrackedSegments(superdoc.page)) + .toEqual([{ from: insertSegment.from, id: insertSegment.id, to: insertSegment.to, type: 'insert', text: 'X' }]); +}); From ac20a42f5baddf5d398abbf53c29850b0d16f56e Mon Sep 17 00:00:00 2001 From: Nick Bernal Date: Sun, 15 Mar 2026 13:20:06 -0700 Subject: [PATCH 2/3] fix: pair imported Word replacements without comments and preserve source ids --- .../v2/importer/documentCommentsImporter.js | 57 +--- .../v2/importer/docxImporter.js | 2 + .../v2/importer/trackedChangeIdMapper.js | 150 ++++++++++ .../v2/importer/trackedChangeIdMapper.test.js | 258 ++++++++++++++++++ .../v3/handlers/w/del/del-translator.js | 10 +- .../v3/handlers/w/del/del-translator.test.js | 69 +++-- .../v3/handlers/w/ins/ins-translator.js | 10 +- .../v3/handlers/w/ins/ins-translator.test.js | 71 +++-- .../extensions/track-changes/track-delete.js | 5 + .../extensions/track-changes/track-insert.js | 5 + ...-1960-word-replacement-no-comments.test.js | 102 +++++++ .../trackChangesRoundtrip.test.js | 4 +- .../tests/import/trackChangesImporter.test.js | 7 +- .../sd-1960-word-replacement-no-comments.docx | Bin 0 -> 14198 bytes ...-1960-word-replacement-no-comments.spec.ts | 152 +++++++++++ ...-adjacent-different-ids-resolution.spec.ts | 13 +- 16 files changed, 823 insertions(+), 92 deletions(-) create mode 100644 packages/super-editor/src/core/super-converter/v2/importer/trackedChangeIdMapper.js create mode 100644 packages/super-editor/src/core/super-converter/v2/importer/trackedChangeIdMapper.test.js create mode 100644 packages/super-editor/src/tests/import-export/sd-1960-word-replacement-no-comments.test.js create mode 100644 tests/behavior/tests/comments/fixtures/sd-1960-word-replacement-no-comments.docx create mode 100644 tests/behavior/tests/comments/sd-1960-word-replacement-no-comments.spec.ts diff --git a/packages/super-editor/src/core/super-converter/v2/importer/documentCommentsImporter.js b/packages/super-editor/src/core/super-converter/v2/importer/documentCommentsImporter.js index 5254721cb1..3f131cfc96 100644 --- a/packages/super-editor/src/core/super-converter/v2/importer/documentCommentsImporter.js +++ b/packages/super-editor/src/core/super-converter/v2/importer/documentCommentsImporter.js @@ -265,8 +265,6 @@ const extractCommentRangesFromDocument = (docx, converter) => { let positionIndex = 0; let lastElementWasCommentMarker = false; const recentlyClosedComments = new Set(); - let lastTrackedChange = null; - const walkElements = (elements, currentTrackedChangeId = null) => { if (!elements || !Array.isArray(elements)) return; @@ -311,61 +309,25 @@ const extractCommentRangesFromDocument = (docx, converter) => { } lastElementWasCommentMarker = true; } else if (isTrackedChange) { - const trackedChangeId = element.attributes?.['w:id']; - const author = element.attributes?.['w:author']; - const date = element.attributes?.['w:date']; - const elementType = element.name; - let mappedId = trackedChangeId; - let isReplacement = false; - - if (trackedChangeId !== undefined && converter) { - if (!converter.trackedChangeIdMap) { - converter.trackedChangeIdMap = new Map(); - } - - // Word uses different IDs for deletion and insertion in replacements, link them by same author/date - if ( - lastTrackedChange && - lastTrackedChange.type !== elementType && - lastTrackedChange.author === author && - lastTrackedChange.date === date - ) { - mappedId = lastTrackedChange.mappedId; - converter.trackedChangeIdMap.set(String(trackedChangeId), mappedId); - isReplacement = true; - } else { - if (!converter.trackedChangeIdMap.has(String(trackedChangeId))) { - converter.trackedChangeIdMap.set(String(trackedChangeId), uuidv4()); - } - mappedId = converter.trackedChangeIdMap.get(String(trackedChangeId)); - } - } - - if (currentTrackedChangeId === null) { - if (isReplacement) { - lastTrackedChange = null; - } else { - lastTrackedChange = { - type: elementType, - author, - date, - mappedId, - wordId: String(trackedChangeId), - }; - } - } + // ID mapping and replacement pairing are handled by trackedChangeIdMapper. + // Here we only associate recently-closed comments with the tracked change. + const wordId = element.attributes?.['w:id']; + const mappedId = + wordId != null + ? (converter?.trackedChangeIdMap?.get(String(wordId)) ?? String(wordId)) + : currentTrackedChangeId; if (mappedId && recentlyClosedComments.size > 0) { recentlyClosedComments.forEach((commentId) => { if (!commentsInTrackedChanges.has(commentId)) { - commentsInTrackedChanges.set(commentId, String(mappedId)); + commentsInTrackedChanges.set(commentId, mappedId); } }); } recentlyClosedComments.clear(); if (element.elements && Array.isArray(element.elements)) { - walkElements(element.elements, mappedId !== undefined ? String(mappedId) : currentTrackedChangeId); + walkElements(element.elements, mappedId); } } else { if (lastElementWasCommentMarker) { @@ -375,7 +337,6 @@ const extractCommentRangesFromDocument = (docx, converter) => { if (element.name === 'w:p') { recentlyClosedComments.clear(); - lastTrackedChange = null; } if (element.elements && Array.isArray(element.elements)) { diff --git a/packages/super-editor/src/core/super-converter/v2/importer/docxImporter.js b/packages/super-editor/src/core/super-converter/v2/importer/docxImporter.js index 7ec8ad698d..734a2a8dc8 100644 --- a/packages/super-editor/src/core/super-converter/v2/importer/docxImporter.js +++ b/packages/super-editor/src/core/super-converter/v2/importer/docxImporter.js @@ -18,6 +18,7 @@ import { autoPageHandlerEntity, autoTotalPageCountEntity } from './autoPageNumbe import { pageReferenceEntity } from './pageReferenceImporter.js'; import { pictNodeHandlerEntity } from './pictNodeImporter.js'; import { importCommentData } from './documentCommentsImporter.js'; +import { buildTrackedChangeIdMap } from './trackedChangeIdMapper.js'; import { importFootnoteData, importEndnoteData } from './documentFootnotesImporter.js'; import { getDefaultStyleDefinition } from '@converter/docx-helpers/index.js'; import { pruneIgnoredNodes } from './ignoredNodes.js'; @@ -148,6 +149,7 @@ export const createDocumentJson = (docx, converter, editor) => { patchNumberingDefinitions(docx); const numbering = getNumberingDefinitions(docx); + converter.trackedChangeIdMap = buildTrackedChangeIdMap(docx); const comments = importCommentData({ docx, nodeListHandler, converter, editor }); const footnotes = importFootnoteData({ docx, nodeListHandler, converter, editor, numbering }); const endnotes = importEndnoteData({ docx, nodeListHandler, converter, editor, numbering }); diff --git a/packages/super-editor/src/core/super-converter/v2/importer/trackedChangeIdMapper.js b/packages/super-editor/src/core/super-converter/v2/importer/trackedChangeIdMapper.js new file mode 100644 index 0000000000..ab7187b469 --- /dev/null +++ b/packages/super-editor/src/core/super-converter/v2/importer/trackedChangeIdMapper.js @@ -0,0 +1,150 @@ +// @ts-check +import { v4 as uuidv4 } from 'uuid'; + +/** + * @typedef {{ type: string, author: string, date: string, internalId: string }} TrackedChangeEntry + * @typedef {{ lastTrackedChange: TrackedChangeEntry | null }} WalkContext + */ + +const TRACKED_CHANGE_NAMES = new Set(['w:ins', 'w:del']); + +/** + * Non-content marker elements that can appear between the two halves of a Word + * replacement without breaking the pairing. These are range/annotation markers + * that carry no document content. + * + * Any element NOT in this set (e.g. w:r, w:hyperlink, w:sdt) is treated as + * content and resets the pairing state so unrelated revisions in the same + * paragraph are never falsely linked. + */ +const PAIRING_TRANSPARENT_NAMES = new Set([ + 'w:commentRangeStart', + 'w:commentRangeEnd', + 'w:bookmarkStart', + 'w:bookmarkEnd', + 'w:proofErr', + 'w:permStart', + 'w:permEnd', + 'w:moveFromRangeStart', + 'w:moveFromRangeEnd', + 'w:moveToRangeStart', + 'w:moveToRangeEnd', +]); + +/** + * Two adjacent tracked changes form a Word replacement pair when they are + * opposite types (delete vs insert) from the same author at the same timestamp. + * + * @param {TrackedChangeEntry} previous + * @param {{ type: string, author: string, date: string }} current + * @returns {boolean} + */ +function isReplacementPair(previous, current) { + return previous.type !== current.type && previous.author === current.author && previous.date === current.date; +} + +/** + * Assigns an internal UUID to a tracked change element. Adjacent replacement + * halves (w:del + w:ins with matching author/date) share the same UUID. + * + * @param {object} element XML element (w:ins or w:del) + * @param {Map} idMap Accumulates Word ID → internal UUID + * @param {WalkContext} context Mutable walk state for replacement pairing + * @param {boolean} insideTrackedChange Whether this element is nested in another tracked change + */ +function assignInternalId(element, idMap, context, insideTrackedChange) { + const wordId = String(element.attributes?.['w:id'] ?? ''); + if (!wordId) return; + + // Nested tracked changes get their own UUID but are never paired. + if (insideTrackedChange) { + if (!idMap.has(wordId)) { + idMap.set(wordId, uuidv4()); + } + return; + } + + const current = { + type: element.name, + author: element.attributes?.['w:author'] ?? '', + date: element.attributes?.['w:date'] ?? '', + }; + + if (context.lastTrackedChange && isReplacementPair(context.lastTrackedChange, current)) { + // Second half of a replacement — share the first half's UUID, but only + // if this w:id hasn't already been mapped. A reused id that was already + // part of an earlier pair must keep its original mapping. + if (!idMap.has(wordId)) { + idMap.set(wordId, context.lastTrackedChange.internalId); + } + context.lastTrackedChange = null; + } else { + // Reuse an existing mapping when the same w:id appears more than once + // (Word reuses tracked-change ids across the document). Minting a fresh + // UUID here would overwrite the earlier entry and break any replacement + // pair that was already recorded for this id. + const internalId = idMap.get(wordId) ?? uuidv4(); + idMap.set(wordId, internalId); + context.lastTrackedChange = { ...current, internalId }; + } +} + +/** + * Recursively walks XML elements, assigning internal UUIDs to every tracked + * change and pairing adjacent replacements. + * + * @param {Array} elements + * @param {Map} idMap + * @param {WalkContext} context + * @param {boolean} [insideTrackedChange] + */ +function walkElements(elements, idMap, context, insideTrackedChange = false) { + if (!Array.isArray(elements)) return; + + for (const element of elements) { + if (TRACKED_CHANGE_NAMES.has(element.name)) { + assignInternalId(element, idMap, context, insideTrackedChange); + + if (element.elements) { + // Descend with an isolated context so content inside a tracked change + // cannot clear the outer replacement candidate. + walkElements(element.elements, idMap, { lastTrackedChange: null }, /* insideTrackedChange */ true); + } + } else { + // Content-bearing elements break replacement pairing. Only non-content + // markers (comment/bookmark/permission ranges) are transparent. + if (!PAIRING_TRANSPARENT_NAMES.has(element.name)) { + context.lastTrackedChange = null; + } + + if (element.elements) { + walkElements(element.elements, idMap, context, insideTrackedChange); + } + } + } +} + +/** + * Builds a map from OOXML `w:id` values to stable internal UUIDs by scanning + * `word/document.xml`. + * + * Word tracked replacements use separate `w:id` values for the delete and + * insert halves. This function detects adjacent opposite-type changes with + * matching author and date and maps both halves to the same internal UUID so + * the editor can resolve them as a single logical change. + * + * Must run before comment import so all consumers — translators, comment + * helpers, and the tracked-change resolver — see a fully populated map. + * + * @param {object} docx Parsed DOCX package + * @returns {Map} Word `w:id` → internal UUID + */ +export function buildTrackedChangeIdMap(docx) { + const body = docx?.['word/document.xml']?.elements?.[0]; + if (!body?.elements) return new Map(); + + const idMap = new Map(); + walkElements(body.elements, idMap, { lastTrackedChange: null }); + + return idMap; +} diff --git a/packages/super-editor/src/core/super-converter/v2/importer/trackedChangeIdMapper.test.js b/packages/super-editor/src/core/super-converter/v2/importer/trackedChangeIdMapper.test.js new file mode 100644 index 0000000000..8bc6ce61d9 --- /dev/null +++ b/packages/super-editor/src/core/super-converter/v2/importer/trackedChangeIdMapper.test.js @@ -0,0 +1,258 @@ +import { describe, expect, it } from 'vitest'; +import { buildTrackedChangeIdMap } from './trackedChangeIdMapper.js'; + +// --------------------------------------------------------------------------- +// Test helpers +// --------------------------------------------------------------------------- + +function trackedChange(name, id, author = 'Alice', date = '2024-01-01T00:00:00Z', children = []) { + return { + name, + attributes: { 'w:id': id, 'w:author': author, 'w:date': date }, + elements: children, + }; +} + +function wordDelete(id, text, author = 'Alice', date = '2024-01-01T00:00:00Z') { + return trackedChange('w:del', id, author, date, [ + { + name: 'w:r', + elements: [{ name: 'w:delText', elements: [{ text }] }], + }, + ]); +} + +function wordInsert(id, text, author = 'Alice', date = '2024-01-01T00:00:00Z') { + return trackedChange('w:ins', id, author, date, [ + { + name: 'w:r', + elements: [{ name: 'w:t', elements: [{ text }] }], + }, + ]); +} + +function paragraph(...children) { + return { name: 'w:p', elements: children }; +} + +function createDocx(...bodyChildren) { + return { + 'word/document.xml': { + elements: [{ name: 'w:document', elements: bodyChildren }], + }, + }; +} + +// --------------------------------------------------------------------------- +// Tests +// --------------------------------------------------------------------------- + +describe('buildTrackedChangeIdMap', () => { + it('returns an empty map when document.xml is missing', () => { + expect(buildTrackedChangeIdMap({})).toEqual(new Map()); + }); + + it('returns an empty map when the body has no elements', () => { + const docx = { 'word/document.xml': { elements: [{ name: 'w:document' }] } }; + expect(buildTrackedChangeIdMap(docx)).toEqual(new Map()); + }); + + it('assigns a unique UUID to each standalone tracked change', () => { + const docx = createDocx(paragraph(trackedChange('w:del', '1')), paragraph(trackedChange('w:ins', '2'))); + + const idMap = buildTrackedChangeIdMap(docx); + + expect(idMap.size).toBe(2); + expect(idMap.get('1')).toBeTruthy(); + expect(idMap.get('2')).toBeTruthy(); + expect(idMap.get('1')).not.toBe(idMap.get('2')); + }); + + describe('replacement pairing', () => { + it('maps adjacent w:del + w:ins with same author/date to the same UUID', () => { + const docx = createDocx( + paragraph( + trackedChange('w:del', '10', 'Alice', '2024-01-01T00:00:00Z'), + trackedChange('w:ins', '11', 'Alice', '2024-01-01T00:00:00Z'), + ), + ); + + const idMap = buildTrackedChangeIdMap(docx); + + expect(idMap.size).toBe(2); + expect(idMap.get('10')).toBe(idMap.get('11')); + }); + + it('maps adjacent w:ins + w:del with same author/date to the same UUID', () => { + const docx = createDocx( + paragraph( + trackedChange('w:ins', '20', 'Bob', '2024-06-15T12:00:00Z'), + trackedChange('w:del', '21', 'Bob', '2024-06-15T12:00:00Z'), + ), + ); + + const idMap = buildTrackedChangeIdMap(docx); + + expect(idMap.get('20')).toBe(idMap.get('21')); + }); + + it('does NOT pair adjacent changes of the same type', () => { + const docx = createDocx( + paragraph( + trackedChange('w:del', '30', 'Alice', '2024-01-01T00:00:00Z'), + trackedChange('w:del', '31', 'Alice', '2024-01-01T00:00:00Z'), + ), + ); + + const idMap = buildTrackedChangeIdMap(docx); + + expect(idMap.get('30')).not.toBe(idMap.get('31')); + }); + + it('does NOT pair changes with different authors', () => { + const docx = createDocx( + paragraph( + trackedChange('w:del', '40', 'Alice', '2024-01-01T00:00:00Z'), + trackedChange('w:ins', '41', 'Bob', '2024-01-01T00:00:00Z'), + ), + ); + + const idMap = buildTrackedChangeIdMap(docx); + + expect(idMap.get('40')).not.toBe(idMap.get('41')); + }); + + it('does NOT pair changes with different dates', () => { + const docx = createDocx( + paragraph( + trackedChange('w:del', '50', 'Alice', '2024-01-01T00:00:00Z'), + trackedChange('w:ins', '51', 'Alice', '2024-06-15T12:00:00Z'), + ), + ); + + const idMap = buildTrackedChangeIdMap(docx); + + expect(idMap.get('50')).not.toBe(idMap.get('51')); + }); + }); + + it('resets pairing at paragraph boundaries', () => { + const docx = createDocx( + paragraph(trackedChange('w:del', '60', 'Alice', '2024-01-01T00:00:00Z')), + paragraph(trackedChange('w:ins', '61', 'Alice', '2024-01-01T00:00:00Z')), + ); + + const idMap = buildTrackedChangeIdMap(docx); + + expect(idMap.get('60')).not.toBe(idMap.get('61')); + }); + + it('preserves pairing across non-content markers (comment/bookmark ranges)', () => { + const docx = createDocx( + paragraph( + trackedChange('w:del', '70', 'Alice', '2024-01-01T00:00:00Z'), + { name: 'w:commentRangeEnd', attributes: { 'w:id': '99' } }, + { name: 'w:bookmarkEnd', attributes: { 'w:id': '100' } }, + trackedChange('w:ins', '71', 'Alice', '2024-01-01T00:00:00Z'), + ), + ); + + const idMap = buildTrackedChangeIdMap(docx); + + // Range markers carry no content and don't break replacement pairing. + expect(idMap.get('70')).toBe(idMap.get('71')); + }); + + it('does NOT pair changes separated by a content run', () => { + const docx = createDocx( + paragraph( + trackedChange('w:del', '72', 'Alice', '2024-01-01T00:00:00Z'), + { name: 'w:r', elements: [{ name: 'w:t', elements: [{ text: 'live text' }] }] }, + trackedChange('w:ins', '73', 'Alice', '2024-01-01T00:00:00Z'), + ), + ); + + const idMap = buildTrackedChangeIdMap(docx); + + // A content run between tracked changes means they are separate revisions. + expect(idMap.get('72')).not.toBe(idMap.get('73')); + }); + + it('assigns UUIDs to nested tracked changes independently', () => { + const inner = trackedChange('w:ins', '81', 'Alice', '2024-01-01T00:00:00Z'); + const outer = trackedChange('w:del', '80', 'Alice', '2024-01-01T00:00:00Z', [inner]); + + const docx = createDocx(paragraph(outer)); + const idMap = buildTrackedChangeIdMap(docx); + + expect(idMap.size).toBe(2); + expect(idMap.get('80')).toBeTruthy(); + expect(idMap.get('81')).toBeTruthy(); + expect(idMap.get('80')).not.toBe(idMap.get('81')); + }); + + it('consumes only one pair per replacement', () => { + // del(A) + ins(B) pair together; del(C) stands alone. + const docx = createDocx( + paragraph( + trackedChange('w:del', '90', 'Alice', '2024-01-01T00:00:00Z'), + trackedChange('w:ins', '91', 'Alice', '2024-01-01T00:00:00Z'), + trackedChange('w:del', '92', 'Alice', '2024-01-01T00:00:00Z'), + ), + ); + + const idMap = buildTrackedChangeIdMap(docx); + + expect(idMap.get('90')).toBe(idMap.get('91')); + expect(idMap.get('92')).not.toBe(idMap.get('90')); + }); + + it('preserves earlier mapping when a w:id is reused later in the document', () => { + // del(1) + ins(2) pair, then del(1) appears again in a later paragraph. + // The second occurrence of id "1" must keep the UUID from the first + // occurrence, not overwrite it with a fresh one. + const docx = createDocx( + paragraph( + trackedChange('w:del', '1', 'Alice', '2024-01-01T00:00:00Z'), + trackedChange('w:ins', '2', 'Alice', '2024-01-01T00:00:00Z'), + ), + paragraph(trackedChange('w:del', '1', 'Alice', '2024-01-01T00:00:00Z')), + ); + + const idMap = buildTrackedChangeIdMap(docx); + + // The pair is intact: both map to the same internal id. + expect(idMap.get('1')).toBe(idMap.get('2')); + }); + + it('preserves earlier mapping when a reused w:id appears as the second half of a later pair', () => { + // del(1) + ins(2) pair first. Then del(3) + ins(2) would try to pair, + // but id "2" is already mapped — it must keep its original UUID so the + // first replacement stays intact. + const docx = createDocx( + paragraph( + trackedChange('w:del', '1', 'Alice', '2024-01-01T00:00:00Z'), + trackedChange('w:ins', '2', 'Alice', '2024-01-01T00:00:00Z'), + ), + paragraph( + trackedChange('w:del', '3', 'Alice', '2024-01-01T00:00:00Z'), + trackedChange('w:ins', '2', 'Alice', '2024-01-01T00:00:00Z'), + ), + ); + + const idMap = buildTrackedChangeIdMap(docx); + + // Original pair is preserved. + expect(idMap.get('1')).toBe(idMap.get('2')); + // id "3" must NOT have overwritten id "2" onto a different UUID. + expect(idMap.get('3')).not.toBe(idMap.get('1')); + }); + + it('pairs real Word-shaped replacement siblings with run children', () => { + const docx = createDocx(paragraph(wordDelete('0', 'test '), wordInsert('1', 'abc '))); + + const idMap = buildTrackedChangeIdMap(docx); + + expect(idMap.get('0')).toBe(idMap.get('1')); + }); +}); diff --git a/packages/super-editor/src/core/super-converter/v3/handlers/w/del/del-translator.js b/packages/super-editor/src/core/super-converter/v3/handlers/w/del/del-translator.js index 11460b4ad8..55255de9f5 100644 --- a/packages/super-editor/src/core/super-converter/v3/handlers/w/del/del-translator.js +++ b/packages/super-editor/src/core/super-converter/v3/handlers/w/del/del-translator.js @@ -27,9 +27,13 @@ const encode = (params, encodedAttrs = {}) => { const { nodeListHandler, extraParams = {}, converter } = params; const { node } = extraParams; - if (encodedAttrs.id && converter?.trackedChangeIdMap?.has(encodedAttrs.id)) { - encodedAttrs.id = converter.trackedChangeIdMap.get(encodedAttrs.id); + // Preserve the original OOXML w:id for round-trip export fidelity. + // The internal id is remapped to a shared UUID for replacement pairing. + const originalWordId = encodedAttrs.id; + if (originalWordId && converter?.trackedChangeIdMap?.has(originalWordId)) { + encodedAttrs.id = converter.trackedChangeIdMap.get(originalWordId); } + encodedAttrs.sourceId = originalWordId || ''; const subs = nodeListHandler.handler({ ...params, @@ -86,7 +90,7 @@ function decode(params) { return { name: 'w:del', attributes: { - 'w:id': trackedMark.attrs.id, + 'w:id': trackedMark.attrs.sourceId || trackedMark.attrs.id, 'w:author': trackedMark.attrs.author, 'w:authorEmail': trackedMark.attrs.authorEmail, 'w:date': trackedMark.attrs.date, diff --git a/packages/super-editor/src/core/super-converter/v3/handlers/w/del/del-translator.test.js b/packages/super-editor/src/core/super-converter/v3/handlers/w/del/del-translator.test.js index 028b4eafd0..7843ae0651 100644 --- a/packages/super-editor/src/core/super-converter/v3/handlers/w/del/del-translator.test.js +++ b/packages/super-editor/src/core/super-converter/v3/handlers/w/del/del-translator.test.js @@ -33,40 +33,37 @@ describe('w:del translator', () => { }); describe('encode', () => { - it('wraps subnodes with trackDelete mark and sets importedAuthor', () => { - const mockNode = { elements: [{ text: 'deleted text' }] }; + const mockNode = { elements: [{ text: 'deleted text' }] }; + function encodeWith({ converter, id = '123' } = {}) { const mockSubNodes = [{ content: [{ type: 'text', text: 'deleted text' }] }]; - - const mockNodeListHandler = { - handler: vi.fn().mockReturnValue(mockSubNodes), - }; + const mockNodeListHandler = { handler: vi.fn().mockReturnValue(mockSubNodes) }; const encodedAttrs = { author: 'Test', authorEmail: 'test@example.com', - id: '123', + id, date: '2025-10-09T12:00:00Z', }; - const result = config.encode( + return config.encode( { nodeListHandler: mockNodeListHandler, extraParams: { node: mockNode }, + converter, path: [], }, { ...encodedAttrs }, ); + } - // Ensure handler is called properly - expect(mockNodeListHandler.handler).toHaveBeenCalledWith( - expect.objectContaining({ - insideTrackChange: true, - nodes: mockNode.elements, - }), - ); + function getMarkAttrs(result) { + return result[0].content[0].marks[0].attrs; + } + + it('wraps subnodes with trackDelete mark and sets importedAuthor', () => { + const result = encodeWith(); - // Ensure results are annotated correctly expect(result).toHaveLength(1); expect(result[0].marks).toEqual([]); expect(result[0].content[0].marks).toEqual([ @@ -79,6 +76,24 @@ describe('w:del translator', () => { }, ]); }); + + it('preserves the original Word ID as sourceId when no map exists', () => { + const result = encodeWith(); + + expect(getMarkAttrs(result)).toEqual(expect.objectContaining({ id: '123', sourceId: '123' })); + }); + + it('remaps id via trackedChangeIdMap and preserves sourceId', () => { + const converter = { + trackedChangeIdMap: new Map([['123', 'shared-uuid-abc']]), + }; + + const result = encodeWith({ converter }); + const attrs = getMarkAttrs(result); + + expect(attrs.id).toBe('shared-uuid-abc'); + expect(attrs.sourceId).toBe('123'); + }); }); describe('decode', () => { @@ -87,6 +102,7 @@ describe('w:del translator', () => { type: 'trackDelete', attrs: { id: '123', + sourceId: '', author: 'Test', authorEmail: 'test@example.com', date: '2025-10-09T12:00:00Z', @@ -120,6 +136,27 @@ describe('w:del translator', () => { expect(result.elements[0].elements[0].name).toBe('w:delText'); }); + it('writes sourceId to w:id for round-trip fidelity', () => { + const mockTrackedMark = { + type: 'trackDelete', + attrs: { + id: 'shared-uuid-abc', + sourceId: '456', + author: 'Test', + authorEmail: 'test@example.com', + date: '2025-10-09T12:00:00Z', + }, + }; + + exportSchemaToJson.mockReturnValue({ elements: [{ name: 'w:t' }] }); + createTrackStyleMark.mockReturnValue(null); + + const node = { type: 'text', marks: [mockTrackedMark] }; + const result = config.decode({ node }); + + expect(result.attributes['w:id']).toBe('456'); + }); + it('returns null if node is missing or invalid', () => { expect(config.decode({ node: null })).toBeNull(); expect(config.decode({ node: {} })).toBeNull(); diff --git a/packages/super-editor/src/core/super-converter/v3/handlers/w/ins/ins-translator.js b/packages/super-editor/src/core/super-converter/v3/handlers/w/ins/ins-translator.js index 8805bb2825..a20313b4d0 100644 --- a/packages/super-editor/src/core/super-converter/v3/handlers/w/ins/ins-translator.js +++ b/packages/super-editor/src/core/super-converter/v3/handlers/w/ins/ins-translator.js @@ -27,9 +27,13 @@ const encode = (params, encodedAttrs = {}) => { const { nodeListHandler, extraParams = {}, converter } = params; const { node } = extraParams; - if (encodedAttrs.id && converter?.trackedChangeIdMap?.has(encodedAttrs.id)) { - encodedAttrs.id = converter.trackedChangeIdMap.get(encodedAttrs.id); + // Preserve the original OOXML w:id for round-trip export fidelity. + // The internal id is remapped to a shared UUID for replacement pairing. + const originalWordId = encodedAttrs.id; + if (originalWordId && converter?.trackedChangeIdMap?.has(originalWordId)) { + encodedAttrs.id = converter.trackedChangeIdMap.get(originalWordId); } + encodedAttrs.sourceId = originalWordId || ''; const subs = nodeListHandler.handler({ ...params, @@ -83,7 +87,7 @@ function decode(params) { return { name: 'w:ins', attributes: { - 'w:id': trackedMark.attrs.id, + 'w:id': trackedMark.attrs.sourceId || trackedMark.attrs.id, 'w:author': trackedMark.attrs.author, 'w:authorEmail': trackedMark.attrs.authorEmail, 'w:date': trackedMark.attrs.date, diff --git a/packages/super-editor/src/core/super-converter/v3/handlers/w/ins/ins-translator.test.js b/packages/super-editor/src/core/super-converter/v3/handlers/w/ins/ins-translator.test.js index b433da367a..522396906c 100644 --- a/packages/super-editor/src/core/super-converter/v3/handlers/w/ins/ins-translator.test.js +++ b/packages/super-editor/src/core/super-converter/v3/handlers/w/ins/ins-translator.test.js @@ -13,7 +13,7 @@ vi.mock('@converter/v3/handlers/helpers.js', () => ({ createTrackStyleMark: vi.fn(), })); -describe('w:del translator', () => { +describe('w:ins translator', () => { beforeEach(() => { vi.clearAllMocks(); }); @@ -33,40 +33,37 @@ describe('w:del translator', () => { }); describe('encode', () => { - it('wraps subnodes with trackInsert mark and sets importedAuthor', () => { - const mockNode = { elements: [{ text: 'added text' }] }; + const mockNode = { elements: [{ text: 'added text' }] }; + function encodeWith({ converter, id = '123' } = {}) { const mockSubNodes = [{ content: [{ type: 'text', text: 'added text' }] }]; - - const mockNodeListHandler = { - handler: vi.fn().mockReturnValue(mockSubNodes), - }; + const mockNodeListHandler = { handler: vi.fn().mockReturnValue(mockSubNodes) }; const encodedAttrs = { author: 'Test', authorEmail: 'test@example.com', - id: '123', + id, date: '2025-10-09T12:00:00Z', }; - const result = config.encode( + return config.encode( { nodeListHandler: mockNodeListHandler, extraParams: { node: mockNode }, + converter, path: [], }, { ...encodedAttrs }, ); + } - // Ensure handler is called properly - expect(mockNodeListHandler.handler).toHaveBeenCalledWith( - expect.objectContaining({ - insideTrackChange: true, - nodes: mockNode.elements, - }), - ); + function getMarkAttrs(result) { + return result[0].content[0].marks[0].attrs; + } + + it('wraps subnodes with trackInsert mark and sets importedAuthor', () => { + const result = encodeWith(); - // Ensure results are annotated correctly expect(result).toHaveLength(1); expect(result[0].marks).toEqual([]); expect(result[0].content[0].marks).toEqual([ @@ -79,6 +76,24 @@ describe('w:del translator', () => { }, ]); }); + + it('preserves the original Word ID as sourceId when no map exists', () => { + const result = encodeWith(); + + expect(getMarkAttrs(result)).toEqual(expect.objectContaining({ id: '123', sourceId: '123' })); + }); + + it('remaps id via trackedChangeIdMap and preserves sourceId', () => { + const converter = { + trackedChangeIdMap: new Map([['123', 'shared-uuid-abc']]), + }; + + const result = encodeWith({ converter }); + const attrs = getMarkAttrs(result); + + expect(attrs.id).toBe('shared-uuid-abc'); + expect(attrs.sourceId).toBe('123'); + }); }); describe('decode', () => { @@ -87,6 +102,7 @@ describe('w:del translator', () => { type: 'trackInsert', attrs: { id: '123', + sourceId: '', author: 'Test', authorEmail: 'test@example.com', date: '2025-10-09T12:00:00Z', @@ -119,6 +135,27 @@ describe('w:del translator', () => { }); }); + it('writes sourceId to w:id for round-trip fidelity', () => { + const mockTrackedMark = { + type: 'trackInsert', + attrs: { + id: 'shared-uuid-abc', + sourceId: '456', + author: 'Test', + authorEmail: 'test@example.com', + date: '2025-10-09T12:00:00Z', + }, + }; + + exportSchemaToJson.mockReturnValue({ elements: [{ name: 'w:t' }] }); + createTrackStyleMark.mockReturnValue(null); + + const node = { type: 'text', marks: [mockTrackedMark] }; + const result = config.decode({ node }); + + expect(result.attributes['w:id']).toBe('456'); + }); + it('returns null if node is missing or invalid', () => { expect(config.decode({ node: null })).toBeNull(); expect(config.decode({ node: {} })).toBeNull(); diff --git a/packages/super-editor/src/extensions/track-changes/track-delete.js b/packages/super-editor/src/extensions/track-changes/track-delete.js index 765c1e582b..885f4d9368 100644 --- a/packages/super-editor/src/extensions/track-changes/track-delete.js +++ b/packages/super-editor/src/extensions/track-changes/track-delete.js @@ -75,6 +75,11 @@ export const TrackDelete = Mark.create({ }, }, + sourceId: { + default: '', + rendered: false, + }, + importedAuthor: { default: '', rendered: false, diff --git a/packages/super-editor/src/extensions/track-changes/track-insert.js b/packages/super-editor/src/extensions/track-changes/track-insert.js index 746ed528b2..4c7a1d17fe 100644 --- a/packages/super-editor/src/extensions/track-changes/track-insert.js +++ b/packages/super-editor/src/extensions/track-changes/track-insert.js @@ -75,6 +75,11 @@ export const TrackInsert = Mark.create({ }, }, + sourceId: { + default: '', + rendered: false, + }, + importedAuthor: { default: '', rendered: false, diff --git a/packages/super-editor/src/tests/import-export/sd-1960-word-replacement-no-comments.test.js b/packages/super-editor/src/tests/import-export/sd-1960-word-replacement-no-comments.test.js new file mode 100644 index 0000000000..81f0428b93 --- /dev/null +++ b/packages/super-editor/src/tests/import-export/sd-1960-word-replacement-no-comments.test.js @@ -0,0 +1,102 @@ +import path from 'node:path'; +import { readFile } from 'node:fs/promises'; +import { fileURLToPath } from 'node:url'; +import { describe, expect, it } from 'vitest'; +import { Editor } from '@core/Editor.js'; +import { PresentationEditor } from '@core/presentation-editor/PresentationEditor.ts'; +import { getStarterExtensions } from '@extensions/index.js'; +import { initTestEditor } from '../helpers/helpers.js'; + +const __dirname = path.dirname(fileURLToPath(import.meta.url)); +const FIXTURE_PATH = path.resolve( + __dirname, + '../../../../../tests/behavior/tests/comments/fixtures/sd-1960-word-replacement-no-comments.docx', +); + +const collectTrackedSegments = (doc) => { + const segments = []; + + doc.descendants((node, pos) => { + if (!node?.isText || !node.text) return; + + const trackedMark = node.marks?.find( + (mark) => mark.type.name === 'trackInsert' || mark.type.name === 'trackDelete', + ); + if (!trackedMark) return; + + segments.push({ + from: pos, + id: String(trackedMark.attrs.id ?? ''), + sourceId: String(trackedMark.attrs.sourceId ?? ''), + text: String(node.text), + to: pos + node.nodeSize, + type: trackedMark.type.name === 'trackDelete' ? 'delete' : 'insert', + }); + }); + + return segments; +}; + +describe('SD-1960 Word replacement import without comments.xml', () => { + it('loads the imported replacement under one internal tracked-change id', async () => { + const buffer = await readFile(FIXTURE_PATH); + const [docx, media, mediaFiles, fonts] = await Editor.loadXmlData(buffer, true); + const { editor } = initTestEditor({ content: docx, media, mediaFiles, fonts, isHeadless: true }); + + try { + const segments = collectTrackedSegments(editor.state.doc); + const inserts = segments.filter((segment) => segment.type === 'insert'); + const deletes = segments.filter((segment) => segment.type === 'delete'); + + expect(inserts.length).toBeGreaterThan(0); + expect(deletes.length).toBeGreaterThan(0); + expect(new Set(inserts.map((segment) => segment.id)).size).toBe(1); + expect(new Set(deletes.map((segment) => segment.id)).size).toBe(1); + expect(inserts[0].id).toBe(deletes[0].id); + expect(inserts[0].sourceId).not.toBe(deletes[0].sourceId); + expect(inserts.map((segment) => segment.text).join('')).toBe('abc '); + expect(deletes.map((segment) => segment.text).join('')).toBe('test '); + } finally { + editor.destroy(); + } + }); + + it('keeps the shared internal id when booted through PresentationEditor with a Blob fileSource', async () => { + const buffer = await readFile(FIXTURE_PATH); + const blob = new Blob([buffer], { + type: 'application/vnd.openxmlformats-officedocument.wordprocessingml.document', + }); + const [docx, media, mediaFiles, fonts] = await Editor.loadXmlData(blob); + const host = document.createElement('div'); + document.body.appendChild(host); + + const editor = new PresentationEditor({ + mode: 'docx', + element: host, + fileSource: blob, + extensions: getStarterExtensions(), + documentId: 'sd-1960-browser-path', + content: docx, + media, + mediaFiles, + fonts, + isCommentsEnabled: true, + documentMode: 'editing', + suppressDefaultDocxStyles: true, + }); + + try { + const segments = collectTrackedSegments(editor.editor.state.doc); + const inserts = segments.filter((segment) => segment.type === 'insert'); + const deletes = segments.filter((segment) => segment.type === 'delete'); + + expect(inserts.length).toBeGreaterThan(0); + expect(deletes.length).toBeGreaterThan(0); + expect(inserts[0].id).toBe(deletes[0].id); + expect(inserts[0].sourceId).not.toBe(deletes[0].sourceId); + } finally { + editor.destroy(); + host.remove(); + } + }); +}); diff --git a/packages/super-editor/src/tests/import-export/trackChangesRoundtrip.test.js b/packages/super-editor/src/tests/import-export/trackChangesRoundtrip.test.js index 6178f68b4a..d465c748d8 100644 --- a/packages/super-editor/src/tests/import-export/trackChangesRoundtrip.test.js +++ b/packages/super-editor/src/tests/import-export/trackChangesRoundtrip.test.js @@ -177,14 +177,14 @@ describe('msword tracked changes import/export round trip', () => { ({ docx, media, mediaFiles, fonts } = await loadTestDataForEditorTests(filename)); }); - it('preserves separate add and delete revisions through export', async () => { + it('combines Word replacements internally while preserving separate OOXML ids on export', async () => { const { editor } = await initTestEditor({ content: docx, media, mediaFiles, fonts, isHeadless: true }); try { const initialMarks = collectTrackMarkIds(editor.state.doc); expect(initialMarks.insert.length).toBeGreaterThan(0); expect(initialMarks.delete.length).toBeGreaterThan(0); - expect(getIntersection(initialMarks.insert, initialMarks.delete)).toHaveLength(0); + expect(getIntersection(initialMarks.insert, initialMarks.delete).length).toBeGreaterThan(0); const exportedBuffer = await editor.exportDocx({ isFinalDoc: false }); const exportedBody = await loadExportedDocumentBody(exportedBuffer); diff --git a/packages/super-editor/src/tests/import/trackChangesImporter.test.js b/packages/super-editor/src/tests/import/trackChangesImporter.test.js index 0035c22bc5..7191beffa3 100644 --- a/packages/super-editor/src/tests/import/trackChangesImporter.test.js +++ b/packages/super-editor/src/tests/import/trackChangesImporter.test.js @@ -87,7 +87,6 @@ describe('TrackChangesImporter', () => { ]; const result = handleTrackChangeNode({ docx: {}, nodes, nodeListHandler: defaultNodeListHandler() }); - console.log('result:', result.nodes[0].content[0]); expect(result.nodes.length).toBe(1); expect(result.consumed).toBe(1); // Find the trackDelete mark (may not be first mark due to fontSize fallback adding textStyle) @@ -95,6 +94,7 @@ describe('TrackChangesImporter', () => { expect(trackDeleteMark).toBeDefined(); expect(trackDeleteMark.attrs).toEqual({ id: '1', + sourceId: '1', date: '2023-10-01', author: 'Author', importedAuthor: 'Author (imported)', @@ -123,6 +123,7 @@ describe('TrackChangesImporter', () => { expect(trackInsertMark).toBeDefined(); expect(trackInsertMark.attrs).toEqual({ id: '1', + sourceId: '1', date: '2023-10-01', author: 'Author', importedAuthor: 'Author (imported)', @@ -147,6 +148,7 @@ describe('TrackChangesImporter', () => { expect(mark).toBeDefined(); expect(mark.attrs).toEqual({ id: '3', + sourceId: '3', date: '2024-09-05T10:44:00Z', author: 'Nested Author', importedAuthor: 'Nested Author (imported)', @@ -173,6 +175,7 @@ describe('TrackChangesImporter', () => { // Check the trackDelete mark attrs (mark may not be at index 0 due to fontSize fallback) expect(mark.attrs).toEqual({ id: '4', + sourceId: '4', date: '2024-09-05T11:12:00Z', author: 'Nested Author', importedAuthor: 'Nested Author (imported)', @@ -222,6 +225,7 @@ describe('trackChanges live xml test', () => { expect(insertionMark).toBeDefined(); expect(insertionMark.attrs).toEqual({ id: '0', + sourceId: '0', date: '2024-09-02T15:56:00Z', author: 'torcsi@harbourcollaborators.com', importedAuthor: 'torcsi@harbourcollaborators.com (imported)', @@ -236,6 +240,7 @@ describe('trackChanges live xml test', () => { expect(deletionMark).toBeDefined(); expect(deletionMark.attrs).toEqual({ id: '1', + sourceId: '1', date: '2024-09-02T15:56:00Z', author: 'torcsi@harbourcollaborators.com', importedAuthor: 'torcsi@harbourcollaborators.com (imported)', diff --git a/tests/behavior/tests/comments/fixtures/sd-1960-word-replacement-no-comments.docx b/tests/behavior/tests/comments/fixtures/sd-1960-word-replacement-no-comments.docx new file mode 100644 index 0000000000000000000000000000000000000000..2c2a6cf4f1dece5f568c1799675a0d6a8b1c1007 GIT binary patch literal 14198 zcmeHuWpEtHvhIkPnb{gKGg{2d%*+fHGc%JVS93JF_w?`zr-$FmQALBmf!!0FVGGqioHLK>&a}2mk;L01c`wY;Wgc zYUiS_;^|=OtV{1KsqPeC&U&3wzWfkA0h%>2pzt@}Buv0%}gVV=$gx!Cr=f)|Qk(TUoMNe0kn7pAd*HLxxHrHtKKM<7MlFGjkfjHTlYLn>%|1v zG?+69rZ4bC4}2PqbAq5duiM_ubtS^?otQ@IqoE5ipmr>zNZw%Z3UYA}xy$;g%E5CNMz%_~D2V!x`D>}cKd z^Qh^nwa#8Y2V$Z`7H+Of`0A5WS!GrFR>$B|u`KPNwZt_3)39_#UHKT|fDW#U8K#rR z2uK)d%XVyv0Mpl()Ngi~wm$=ucUrvFcVaHmBDy&YIoJvs(n&Y>v(+R)C|mP9Qmr8; zuPNUV!9k1pW8|YTz>R6Nz>7TmT)<~%s-!Jh#+WedGJcNWI+JGt6I99zfflL|?rFUD zr$9?F+1orKB(aUcn5;btEG8*NLkpk~12b~|>?GIzp*Z7;w}g&o!z3M`MbAW~?3DAH*ro{xDB@z-PM z`y-8qmvfl2I#`L{W~gS?zKCzcQpP>#z86PV*Vxbw&)?wIsqULEvew#!Rc&{@r)t#` z9gCaB?SnGE@Y(kIej$v0?`1IOBCM>lSH2G;Al98B`ZMf)I@&Y@2D%13MFqdKqpMJf zOC9>5<-s!a3m=pq!9?!6Z7oxL`j0iyyqrX(K*#E=O$#s>xjn1R5IBL(%jc!bja%iz zP}5A5+elu5#^#{tdbPWlZrnTzNCfxVvprE7nsx%yA`ZH5*(vq9fv-Jj2H710JQrbD z`WFw%ab$(J`s`~3VQwkx@yVt33n_iZhOrATLkvFy;l_{xhi!m6EPidF%vO*edcSEn zwyHmu;*c)!EBm+(rEH1nPg!#RoB+xJJ#Lv9=aZe;N>8j!%a2`x_q?pX?d~%bDTAwo(#%C2p~}`p;B?I&myFzSyt(9mI_-UFN)FPSI~Tq=E;4htK;AOXx8DD|kIJMmx!*G{iKieo@2yXQAqqKm$Vx59{GVOlAtiXwdVB1g z)@i{~C5MZqI#;#rjen`Q_Fj)rzsjemAeL4_wm?X!s>0yKxW(ci2DR2ch^3{*X8g=8 zAfU}>P{KtYQtbM^9}^Fqb^Jta)ByL$Rbn_lh5_SV&l%r`E{A=pU93%FuIOqTaX5~9 zpjc&tW0xY_$l)%}uDpC6N-(b(i>faebQCe>5m_|EI=DNC04h7y!U{>+!omxmcLmnlk*}GW`|_ zjx;Bt@Ho(WupR`$+jt1$deS_C1R7-Bk|+*8?EqIUq8Bn}h(|h@NE8y48A61#yr`}g zgsBi%r>CV~(?f#UpUJ2#ZKj47TrTVK^N(;B$o6k@eZ5Zi_cfibzXiqnqxEwZD~?g= zIuiI7Fy(9v+jF^x1qtnmwzP^c601k;xRS{eqQUS4(h7snNT3)O(4Y0WIh_Dm)2)H+pnV zP$QH=Dtu`j9?aw*!XHcla8Uh>1v+jWdS&EbHCtYXam}2o?-|K*Tw?Ck(zd|v(l<34 zv+YXT!$^n_tb5B zV!j^iG3YQZp5Y@I@RX3(f}QJG>B>halCy zf|>4|ESvfQ?B#Ha{NxYKRRU;v8)tWp&egmj=94;EQw~AM31L!QL?3h#kgwP|Bqk^w zadn1^V7z9@b7g>Ark{c~Y$M2V(H2r6Hx9t(`dXCa7j_$G2P79@UD;_q8XDCtBsqIZ zIJQ|Q;vQ(I+8`sj<_ijX32}-8K zyR-4o)v#o<1U~LVNXF=@Y)5pHy1erYctvQ$vf9LtKM)md_o1+N+aC$VLOZ6`SKLi; z>!HGNGgwwq1;4Jw!7+yn7k5qwkqtECG>eIwpWx+p#UTfjI0PcMK4u`u8~6R7+Q$nR zr}iE;%fg$ER-KPUPHRe}wxf+5ScP6D+nK}Xhl+~nZ}3-y*3A8(&B9{xK=zKKkWO#J z!|apN_}Aj5q}B;xW1luHBruEM6f0-%PWtrC9*$*Wr5|?$Cezwu!~HLV%{W3vGVze( z$VRpH(k)8;4t~@H%}JP9+PlR+ie=XnR)JnZ_&LfT&m1XnqFY8*ggj?HictMAtr**qWb>oeOb&FCv|I)*Z-1Ca!f>I&1>UsL`?Rh;CIuXQMWAc!wul6~Z_S?kd=GzM{<-uHKWYQ$Uzc6{ z$^7hIscGuHOdD;UON(j?F7f4=zg+6ANBQKedzKO)vT?nt`q8xPUE+5wzAk9pibXno z{pM`^}TYAH5j?3V1Zr)6@YK8=HIYUTG^;&kS;siD4U>ef#ZBW!DNQ%(;whbF8!1$D#wl37l)ibe!J^0ijeZR9@a zGIG7)NtxuVt1~L_{a?Q?*i;-M!S3Yguj8ZKJN9CH;>? z&^T86CXj3cOG0$VY-PFz@8dJ<2r@m*`$h90vlJ^3!&P<5w6z&p_J3-K`O2$RIFq&( zQ~NbBF6nlq$}k6<=$fWMfsW(YaUrc)bf|xU+;7|c4xfEmshet|Ej2#1Bhp(LKEHJD?Dq6PX=wd0oWNrp zn~u>k$s73Uwfx7@ipr|^$X<`L>^O!b|R`q@+qU5qHQ7teA;U5d$}c`nsLIa3D@2k zC^+eOJn`t{89n_jz{2hIMG2*MPtSqrTr54Ufb;{)Z8vY_p!#UxlU-E{vImdlt45(;Lo67fYB3W@5Ne#ZS;&c2%yeKEh-G1g zBECOvnk0Jg#iV;z?O4h)nSDwYiqOJKQu)e8<@oA=dBNt(lN18f4sB~D^ogMACvN?g zD8|KpQ)mOQBN&;xWk2sPom+uOQnzuZG;yuq?X z{)|b7kq(8{!5z!XA4mM*eSGrWJ#TD9ZXeW`F`-h`F&%LuyUo`HX@CcE}eVr~9&);14I92rG~tz2_x=4jVW%qW%=1PNZD~!}x9H0S)B? z17JL6PhNox=@GJqahC4WeoXdql7`}JWkj;j_j6dQ?TPG!_CZv9MCfGi%E=*f_$D1dc6Rxx#AFCW$8I2&EdbcGTIt7`oH4 z>4W=8wJ_5$8|4fj7=@5qG_tS5ch3NKjKx~};NXLhh{!y(IURUU00YQHG@P_hOCk-8 zq~)3$->Ua6pD3-glte&gJt&WL970NFj@MQEWcsGE%DBCh*W8 z7Ct~*d0y{Je&a>hUoOIx8i|efHj$n^FyennyrqyfwHrmKQJiy8)wT(26CW|F08_TW z{Z4(?*vK&O?x=3s$S`afOC>WWz^w<{BtMB@UpB{vdzh3m3R3gP!s9ibWt65g2Q?Av7bzeZ&z3p{NycF{u-v zNEwGE*&6xfQ;@MYisi~x{I{vr$@nj-aa?n)EYIE<{hEm)4UV(`@A6d1ti>Hn0?^DW=?uyfj z7^=^D`7;!Ho*NOqu7qMewUkt1Rg<-?IMeRLB{FtoxS&6re`-q2dK(CSJrl9lv=RDZ zY6UH-K_1Td{#?49&X1wI9=gO0nyDyGX$DAf0enQz+)z#Ql3T1G26v8{Nyzt#^{hsc zsTWbbXbg}7uUEG{&vzbr$?`A^pdj&>dr@abj0|JD_r32?Chr3=1WcrJBXb&P^=p+r zOq?QN()wx8>4z*Ejf{HWQaawdIDGKnB*aedgp*>=8X)mY3aNmQmp1i_zZ4-hvt)`( z6Aigpf;eUxD>Tgw=>+z_XF!cuuC!;b+>lOkSL4x8;FZ5hbKp(is6aI55Dj^J*9 zLpi_$21#B+`#4#!p}x!xMv&o8w!XD+Lp3azf{jA&@sTQghXrO=zfc!2XKrs$$0B~B zqgl?zxRmYJ3>3!K4FA@fNc*9diOX(U&gyeX$c_U_ZcmSVL$)kh0C^`fax4bW;J#-= z{6&%Io})<@G3uUCdIyThN&seGO!F>uYKfW(?w+k@VnY2=_PK3jwjP>rN)u)9Jm+VP zg$qj5&AM)}C@-=%C(Y;>saXuq-i7dXZY{8CLfsJ^3DG19iC2E$q(%n*sqVYVlcGJg zs&=($341dIZC6e6`PF<{#2T0NoC>Qelfiw(ND(tHZVu%+AB}ojxba1n(DWvKT=l^D zrTuOs#l;YmnZcF5b!+O%x(Y)YZM161QsoY5IYA7PDN>>aRox=a^`67j~zwExX(usV|l| zihLDO@g?V0&xGeg(Z-C|V-3e>!bB*ecG1@5nM-9SkgA|%U8p_;{l^Qf3q7G_qZZI$%l>g zySi}6qm_iDHxU9=`koC?j?9rzu_tT-@Ku)+k*&ZH-$;0yvoUzkQ34`nTLBbzX!EB9 zei%`qBfeeaXR3zj-H_A=%4H^Vv}lQ-TP__u~<<8kvO_SCnvqpywVkkmJPVbr^7HT=)om1*%E3d1x;UduT1tyk zi;IjAD=qd&H4<0U<6sIJzWn1FcD0+^+9rVARwr@NdluRND8IxxO-V}89qrqCzo7+< zXVWHv6p0RVVi+$fhSu5OO?P?e+^EK*e2!V!RPtrAxSz?FG)02Ri!&T?ld^>|zW3bEu^M}d z1i587`v^#lL?Bbz)O}4t1htK5y#sC`_1Z{mK8NkrRl+Go9K?c;Q{!P)?4R-;5gqWG zf=HVFOj9j8h_foMsNCOpkVj*GYf#X$wyubu(>4q^HXewMAo1sW^(ZTWMtc-0_PphN zRcEDfEq)i~XZ7S`<`uzq02;x1luE!zD5JDmRO3fpqY!bA&#-Y19x%FMn<&XU=-+!T zI7!Kb0~DF^KbKl47^vU_8I?dE*7J=6u6tyMKo9+DVj;;gK{X6ie8s#TDBDMk0~GHw zKnpKeAjEdfVWUTqKV5LaC7y^RrGBUzcS;pNdEYI2$43T?w`yGB z%^IOJtXEm@mjzH+e}&mJ(e6`Bc*4s|bd@zyyovfqI+{p67H(yIh!$JF+Uahamf?l^ z$sOzMfn0c|itxJQC#|UMjG;Yi47%N_a&V{c3H0AyGN+FpTJ<2X@UL2GQXJu#b>P|H zUefgTC2))}Ssjj; zXN`5c`ScxO3!j*%9eQ;3Dlo@&H`D@c$^3;a)!a>HTIG1&UEaU7=G{yV>0V8R%;GTz zJ*TT*>0wtoXAuMvu4CvLgO05T&Ml`KB8Of0q~j363>Tn{gjS7T4Tc@>3Wimj z779VO$QOccRM-TEr&O_sL!)lIMWGfsfBU|N!w~uxE~7HQe+4^73cs=>R>@QRROD?4F5kkUE2ST@hmRoog-#lvz}49 z5B58admf8u=E|mS3Rl^`*7B&#({QNWnxjX zLn(#J>(OZu*fcJ@{ji_ti7BMZ0hkX^oouD8GXuzf*dPsPz~=qN8?nGyEoWi>TQcGNOYvZ!^3+C{A`ul{|Os zTx)j^I&ouLVTi+ln1!?SuqsAuK4W|053!%4xp?_BF?QhSd2-mib#_43K0Ba5dH#5< z$|Kx-jDHa>sfQwh!BbhN!gm$nH;JJ^zY+q zH>=RPmM&x;?_9oYM7Rp)AH{E2tAh4Dm}Qu4`QOfqaMS48e$ylSrl;s#x#O}D+MBWw z$v=cmzm2r|H{tIi?RN+woPS?{j>{6};}dRqT>UVNKLwEQ9Th$rt!E$@-!^oeBA9*I2WsoT8(VF@A;1$}pXr zjJ9HVUMNP=Pim(S&ao_XP2jrm9wT*j+S6{3Jz3WVnI zn_^!|Q~~Y4WFYl=IB`*o4&vfkh_Wy?tBRF%Y`TBNZr#gCw8!Pf9S8HCb<%mY*K?u% zYtIVuE=xz6CWn2Aw`#&Bk8@2v$%-38S5=8OLe7b)-R(!mnoajDz4>^aRqHgg<382c ziE&%w0GXz$l8Wf;YNM0We8-d-IQJBm73K0;BtFrRu9nbXP=gGsP-4wUoVtP|Mm6s@cbcABj**?_i<)>= z2b|c}r3cjI?Kxe&g32VOJ^J?DHxcIj1R- zYv5k(fj`1)a-Q(iEbjcCv37fKxAs(c`L^@1Z7?&j8h8#Y%mRuY5Z+sP7TM}MRDRjn zn2CO+CtQ1`H)U%nnkxUcq6PR0VOUb=dei8XX;D59c3bAsD85hODqm!S7k8B5@F$SZjHf~)M*l-t_d>(XYt{N%2-9_cT?1WC7Ww1Lek|~SAHyQGSX%l z%ZuIGrc+SDL=0VP=Q<~${zxh$Ujbawz4wb9I=s;l9*h+2J;B^RX>cT@TxLc@N2pLJ zisf#Eiq1=tKGF!#qfUlPFq-%F1k>kot*^^Q&=0l(J!M5AaPG0nqkK}9ui%)QhqUZ% zagA(zWJ#)EtbwEH!3e56U=Ykc12ULH{nPtKZBG}T6gkmVqm{T@Du|nr6hA_qy>|3I zcM%+tDTzOq$jjo&Zi>K@v<6zH%l{iN`jw`j{;;nFix>+qY*WYejc z?~+yM3KAN@(5Og9paC+KRD`@BQ=T=`G+bs#uP$jqZ@dhX6FA$R zjY7GgMd5h~as3pH>~_s1s)VjsorbZ|ig$;mT+KsLd|zw7ca4Q@D!|J=!|%q|wSS?g zksnb%!cn)$8gq3?d-yovy?2ndX-mZJp-nich|3G=TCjRd;N2-Z7A@**v7#emn&#~S zn(?KQq-X$0v@j|s2hfpXu8CT2~|M}B8%CCDGc zZhYz_J(ex?tZwXTU5Z#P*BsYL-Mz2!bU~m3R<3K^B|!)YM`ue|C65c(ZN6+J)_}#H zpLi4%(mtg?uKB9W8EO#)$?D}qv!l=C_3f{-G>HpiX0~xAUBYZ!Ehq9ny~x12180l; zLFH@aOSVd0|BtC zX=64`%(OLH>1IB_mof^_5)d6%d4baTc@JZEWyI;r-7^mTlP0O|Uhar@l!b;;sEqg_ ztXbI~e5xVi$Q^TWdeL!*M?mm3=}%67xT^I%fOR7IwUYw}O*8jCU3HHFT|2=iR*`8K z4eJFfGLUpT@-vr@t8pE^Vq7}WrCdRA%gy0f;&?{|ijr{glC4`Zc8`=`svys_Dyf}( zns2dS(^Ayj+0fC5QxlhGv78*YaDuQLq2s1~Vx%puD$N&q0NRpPz4%)S7<_;j=RA7E ziK}`qY_^zCkjKKqqvVqPiZKV$Uya4&%Q=sdmHxdh`>fz-C}%Q!Q0Ir{h>j-}W_cdx ze)tevalC!UyQ`JPeH`AO9-*1$HF!a(RjTn{s${+lNa1q&_TBdv!x*kd57}A<2@Mb* zeIxDEuk8GK0i|Y zSXimtqRG%6iyZsKkl)icj))LRi^6+TP^*D_IXqDW+YZjMS^DCoxjQEboRSUaT1tGO5Tl)APw?MTOlWM(S>|(3(MS~EJ>67Ya0-8X>qie|rr!DTe=2tkP@1HQ^pU`D5 z#;nX%FLW@qclej-d`ca0%3{PGI?dVVVO*_S zGG?ipqnL|j==$UX5JeS3ql*S_zPw4%SerHT*P(^EdcN9QseBBhF`vu;$`&nk^bWcc zkjD3`WZ#_B>xZYeEi1`{j{ zbSUM>CZ%W^uRut|ALcCy_m5J+4))#PXi$d~422&AK zO*kBe#uP@a1zVv4>VPooV?!#dG-9WKR#<^Xss}`u!%%68KMV~O9ZGB_yR^ij%Sx`e zev;%qw+tLhguY}>i|iYvoriHK)jODF*+5mh%3#*&!Y$6b!Il^pgB2aLB0hs=c()2d z&t!;lLgjS2AZ3!%#j&KluW7xG5Cma(8Ot_w3}{o6QSYkC-0oO-QY^?sQ)JTvi)7pu zr*e5emd|t6#Clo(lVq<2-{D8)x;s@+M$t{r!p%~sA^2p%`ao=h+WkuZ=R6eIC~Vpv z5_gy5dN3R~yo8;jY#fsaX6$){$Z`NGQkGX*ufb+x0IOQd?sSIfyz{qG4MYM0yz!#| zzo<5l4KEbZXfFb{+8-RWF*G`~eb-E&3z%xP{$9Y$)buGEa6YDPl4h7ea@>au(?nA_ zX~+$lo(MxvFa3O9^?G`7Tpyxi&gP@2E?znVR|vnA;H$@eUW{7Sob19Q4^*XIb2Ide z3s)HkZdvf78c7o#R%IvK+?A*OcU%o$P~nf z98?7pU5WUj^jTG6iy1}*vBg{b0__tGS}lhqv}mDcl8d1+6yFS-ChIxnwt?r)_G$KL z459luP|5;lXY^M|AH)@~QLyk&dVy9NOV$UQ|4dUls*KXa4flqCm) zvKh>DcJ_>wO_nmB86`t*b1r?Rk<3!m{g~s--M2JL0BuAjFR{J~Bn!VRTQ-vYnQH?g zzvU4L!TBYetQ7@OtoJfj-U8$z7+K}!IX|rw{zcjDF zfQf(aj$+;qCz+UfcUu7h*U6=6zgu*f%gp${?O z8Q@2w2ksR#h}8J>j~P`{G4zY$n`G$CM3Mh$qJ|C*zlH1nTIid3{$`;{zq98$$8n_&`c^< zW?QD*HXSL#_;p-0CAHP@V!A-qMh_h>v?N$9KlAQA<0-~z?0!$d=*Xo02ZpKKE6(I4 zFEZ4NG`m@u6luEJfRI@VL#Z`VhHyfFjc825l>arvk*-3*RKPVvB_2w!OrrpQ2MBwh z!)e!y61f(w-vV>%&+_Chy;ZX{&f0T~kKjDXVISw(SeQc3KH1tAG7PIByCQerU$7G| zNg6uHE_+hQh!_ZfSLz;yP;jxUe7Z$h|FF-#hTARXn^YL>os*e^Re$NkBgbR)IaWi( z#Ls~d`AlL8MTSzop;_$OXV1QHW!?Lg;ogRc?xzms&4l97X5X3o+s*!0W&i?8_m<@U z^B$w$pUGdEf7o%PApLg*f8Pl47c3zAjYayGZ4ti$e`SdO3GIAi4E_gS{8#wjxvGDH z0RYFhH0S@uT>VwkuO!7kb$xkbZTur;@mCeU(meiD(TDbL6@Mjt{0jf|h3=nlZ0tYa z|MPnHSMaZkw?Dy_z<+~(S<3yY;n&s0pBiAP|E=L~YmHy=zvh>J;w5PRjsG*-{1yE7 yVE!i*0Kk4*3;joE{}ulCiT1DX4yM1r|2^$0NJG5Y761VM_IY`mnvGb0yZS%5?@Q_c literal 0 HcmV?d00001 diff --git a/tests/behavior/tests/comments/sd-1960-word-replacement-no-comments.spec.ts b/tests/behavior/tests/comments/sd-1960-word-replacement-no-comments.spec.ts new file mode 100644 index 0000000000..3347cee1c3 --- /dev/null +++ b/tests/behavior/tests/comments/sd-1960-word-replacement-no-comments.spec.ts @@ -0,0 +1,152 @@ +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; +import type { Page } from '@playwright/test'; +import { test, expect } from '../../fixtures/superdoc.js'; +import { assertDocumentApiReady, getDocumentText, listTrackChanges } from '../../helpers/document-api.js'; + +const __dirname = path.dirname(fileURLToPath(import.meta.url)); +const DOC_PATH = path.resolve(__dirname, 'fixtures/sd-1960-word-replacement-no-comments.docx'); + +test.use({ config: { toolbar: 'full', comments: 'panel', trackChanges: true } }); + +type TrackChangeSegment = { + from: number; + id: string; + sourceId: string; + text: string; + to: number; + type: 'delete' | 'insert'; +}; + +function normalizeText(value: string): string { + return value.replace(/\s+/g, ' ').trim(); +} + +async function listTrackedSegments(page: Page): Promise { + return page.evaluate(() => { + const rawSegments: TrackChangeSegment[] = []; + const editor = (window as any).superdoc?.activeEditor ?? (window as any).editor; + + editor.state.doc.descendants((node: any, pos: number) => { + if (!node?.isText || !node.text) { + return; + } + + const trackedMark = (node.marks ?? []).find((mark: any) => { + const name = mark.type?.name; + return name === 'trackInsert' || name === 'trackDelete'; + }); + + if (!trackedMark) { + return; + } + + rawSegments.push({ + from: Number(pos), + id: String(trackedMark.attrs?.id ?? ''), + sourceId: String(trackedMark.attrs?.sourceId ?? ''), + text: String(node.text), + to: Number(pos + node.nodeSize), + type: trackedMark.type.name === 'trackDelete' ? 'delete' : 'insert', + }); + }); + + return rawSegments.reduce((segments, segment) => { + const previous = segments.at(-1); + if (previous && previous.id === segment.id && previous.type === segment.type && previous.to === segment.from) { + previous.text += segment.text; + previous.to = segment.to; + return segments; + } + + segments.push(segment); + return segments; + }, []); + }); +} + +async function resolveTrackedChangeById(page: Page, input: { action: 'accept' | 'reject'; id: string }): Promise { + await page.evaluate((payload) => { + const editor = (window as any).superdoc?.activeEditor ?? (window as any).editor; + const command = + payload.action === 'accept' ? editor.commands.acceptTrackedChangeById : editor.commands.rejectTrackedChangeById; + + command(payload.id); + }, input); +} + +function combineSegments(segments: TrackChangeSegment[]): TrackChangeSegment { + const [first, ...rest] = [...segments].sort((left, right) => left.from - right.from); + if (!first) { + throw new Error('Expected at least one tracked segment to combine.'); + } + + return rest.reduce( + (combined, segment) => ({ + ...combined, + text: `${combined.text}${segment.text}`, + to: Math.max(combined.to, segment.to), + }), + { ...first }, + ); +} + +async function loadImportedReplacement(page: Page, loadDocument: (filePath: string) => Promise) { + await loadDocument(DOC_PATH); + await assertDocumentApiReady(page); + + const trackedSegments = await listTrackedSegments(page); + const deleteSegments = trackedSegments.filter((segment) => segment.type === 'delete'); + const insertSegments = trackedSegments.filter((segment) => segment.type === 'insert'); + const deleteSegment = deleteSegments.length > 0 ? combineSegments(deleteSegments) : null; + const insertSegment = insertSegments.length > 0 ? combineSegments(insertSegments) : null; + + const replacementDialog = page.locator('.comment-placeholder .comments-dialog', { + has: page.locator('.change-type', { hasText: 'Replaced' }), + }); + + expect(deleteSegment).toBeTruthy(); + expect(insertSegment).toBeTruthy(); + expect(deleteSegment?.id).toBe(insertSegment?.id); + expect(deleteSegment?.sourceId).not.toBe(insertSegment?.sourceId); + expect(normalizeText(deleteSegment?.text ?? '')).toBe('test'); + expect(normalizeText(insertSegment?.text ?? '')).toBe('abc'); + expect(deleteSegment?.to).toBeLessThanOrEqual(insertSegment?.from ?? Number.POSITIVE_INFINITY); + + await expect(replacementDialog).toHaveCount(1); + await expect(replacementDialog.locator('.tracked-change-text.is-deleted')).toContainText('test'); + await expect(replacementDialog.locator('.tracked-change-text.is-inserted')).toContainText('abc'); + + return { + deleteSegment: deleteSegment!, + insertSegment: insertSegment!, + }; +} + +test('SD-1960 accepting imported Word replacement with no comments resolves both tracked halves', async ({ + superdoc, +}) => { + const { insertSegment } = await loadImportedReplacement(superdoc.page, superdoc.loadDocument); + + await expect.poll(async () => (await listTrackChanges(superdoc.page)).total).toBeGreaterThanOrEqual(1); + + await resolveTrackedChangeById(superdoc.page, { action: 'accept', id: insertSegment.id }); + + await expect.poll(() => listTrackedSegments(superdoc.page)).toEqual([]); + await expect.poll(async () => (await listTrackChanges(superdoc.page)).total).toBe(0); + await expect.poll(async () => normalizeText(await getDocumentText(superdoc.page))).toBe('Test abc test'); +}); + +test('SD-1960 rejecting imported Word replacement with no comments resolves both tracked halves', async ({ + superdoc, +}) => { + const { deleteSegment } = await loadImportedReplacement(superdoc.page, superdoc.loadDocument); + + await expect.poll(async () => (await listTrackChanges(superdoc.page)).total).toBeGreaterThanOrEqual(1); + + await resolveTrackedChangeById(superdoc.page, { action: 'reject', id: deleteSegment.id }); + + await expect.poll(() => listTrackedSegments(superdoc.page)).toEqual([]); + await expect.poll(async () => (await listTrackChanges(superdoc.page)).total).toBe(0); + await expect.poll(async () => normalizeText(await getDocumentText(superdoc.page))).toBe('Test test test'); +}); diff --git a/tests/behavior/tests/comments/tracked-change-adjacent-different-ids-resolution.spec.ts b/tests/behavior/tests/comments/tracked-change-adjacent-different-ids-resolution.spec.ts index 58f18a4c29..514cf0a720 100644 --- a/tests/behavior/tests/comments/tracked-change-adjacent-different-ids-resolution.spec.ts +++ b/tests/behavior/tests/comments/tracked-change-adjacent-different-ids-resolution.spec.ts @@ -3,11 +3,12 @@ import { test, expect } from '../../fixtures/superdoc.js'; import { assertDocumentApiReady, deleteText, + findFirstSelectionTarget, findFirstTextRange, insertText, listTrackChanges, } from '../../helpers/document-api.js'; -import type { TextAddress, TextMutationReceipt, TrackChangeType } from '../../helpers/document-api.js'; +import type { SelectionTarget, TextAddress, TextMutationReceipt, TrackChangeType } from '../../helpers/document-api.js'; test.use({ config: { toolbar: 'full', comments: 'panel', trackChanges: true } }); @@ -33,6 +34,14 @@ function requireTextTarget(target: TextAddress | null, pattern: string): TextAdd throw new Error(`Could not find a text target for pattern "${pattern}".`); } +function requireSelectionTarget(target: SelectionTarget | null, pattern: string): SelectionTarget { + if (target != null) { + return target; + } + + throw new Error(`Could not find a selection target for pattern "${pattern}".`); +} + function assertMutationSucceeded( operationName: string, receipt: TextMutationReceipt, @@ -48,7 +57,7 @@ async function createAdjacentTrackedDeleteAndInsert(superdoc: SuperDocHarness) { await superdoc.type('AB'); await superdoc.waitForStable(); - const deleteTarget = requireTextTarget(await findFirstTextRange(superdoc.page, 'A'), 'A'); + const deleteTarget = requireSelectionTarget(await findFirstSelectionTarget(superdoc.page, 'A'), 'A'); const deleteReceipt = await deleteText(superdoc.page, { target: deleteTarget }, { changeMode: 'tracked' }); assertMutationSucceeded('deleteText', deleteReceipt); From 0c7389b639ca15483d65027daff315ccb101c921 Mon Sep 17 00:00:00 2001 From: Nick Bernal Date: Sun, 15 Mar 2026 13:31:13 -0700 Subject: [PATCH 3/3] chore: fix cli test --- apps/cli/src/lib/bootstrap.ts | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/apps/cli/src/lib/bootstrap.ts b/apps/cli/src/lib/bootstrap.ts index cd4ec66049..74aa439f3d 100644 --- a/apps/cli/src/lib/bootstrap.ts +++ b/apps/cli/src/lib/bootstrap.ts @@ -231,6 +231,10 @@ function sleep(ms: number): Promise { return new Promise((resolve) => setTimeout(resolve, ms)); } +function nextTimerTurn(): Promise { + return new Promise((resolve) => setTimeout(resolve, 0)); +} + // --------------------------------------------------------------------------- // Bootstrap claim // --------------------------------------------------------------------------- @@ -273,7 +277,16 @@ export async function claimBootstrap( const observer = observeCompetitor(ydoc); try { - if (settlingMs > 0) await sleep(settlingMs); + if (settlingMs > 0) { + await sleep(settlingMs); + + // Give already-due timer callbacks one more turn to run before the + // final ownership check. This makes bootstrap claiming more + // conservative under event-loop jitter, where a competing marker can + // be queued before the settling window ends but execute immediately + // after our sleep resolves. + await nextTimerTurn(); + } const competitor = observer.getCompetitor(); if (competitor) return { granted: false, competitor };