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 }; diff --git a/packages/super-editor/src/core/Editor.ts b/packages/super-editor/src/core/Editor.ts index 31897ca222..406e55c70f 100644 --- a/packages/super-editor/src/core/Editor.ts +++ b/packages/super-editor/src/core/Editor.ts @@ -3318,7 +3318,6 @@ export class Editor extends EventEmitter { }, SYNC_TIMEOUT_MS); } }); - doReplaceFileSync(); } else { this.#insertNewFileData(); } 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 5aacf46818..e2e8a32421 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 @@ -26,9 +26,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, @@ -85,7 +89,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 76744afa62..12cbcc97df 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 @@ -28,40 +28,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([ @@ -74,6 +71,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', () => { @@ -82,6 +97,7 @@ describe('w:del translator', () => { type: 'trackDelete', attrs: { id: '123', + sourceId: '', author: 'Test', authorEmail: 'test@example.com', date: '2025-10-09T12:00:00Z', @@ -114,6 +130,26 @@ 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' }] }); + + 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 64490ff2c7..0ed46c4834 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 @@ -26,9 +26,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, @@ -82,7 +86,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 071be55e27..113d0680b6 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 @@ -3,7 +3,6 @@ import { config, translator } from './ins-translator.js'; import { NodeTranslator } from '@translator'; import { exportSchemaToJson } from '@converter/exporter.js'; -// Mock external modules vi.mock('@converter/exporter.js', () => ({ exportSchemaToJson: vi.fn(), })); @@ -28,19 +27,16 @@ describe('w:ins 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', }; @@ -48,12 +44,22 @@ describe('w:ins translator', () => { { nodeListHandler: mockNodeListHandler, extraParams: { node: mockNode }, + converter, path: [], }, { ...encodedAttrs }, ); - // Ensure handler is called properly + return { result, mockNodeListHandler }; + } + + function getMarkAttrs(result) { + return result[0].content[0].marks[0].attrs; + } + + it('wraps subnodes with trackInsert mark and sets importedAuthor', () => { + const { result, mockNodeListHandler } = encodeWith(); + expect(mockNodeListHandler.handler).toHaveBeenCalledWith( expect.objectContaining({ insideTrackChange: true, @@ -61,7 +67,6 @@ describe('w:ins translator', () => { }), ); - // Ensure results are annotated correctly expect(result).toHaveLength(1); expect(result[0].marks).toEqual([]); expect(result[0].content[0].marks).toEqual([ @@ -74,6 +79,24 @@ describe('w:ins 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', () => { @@ -82,6 +105,7 @@ describe('w:ins translator', () => { type: 'trackInsert', attrs: { id: '123', + sourceId: '', author: 'Test', authorEmail: 'test@example.com', date: '2025-10-09T12:00:00Z', @@ -103,7 +127,6 @@ describe('w:ins translator', () => { const result = config.decode({ node }); expect(exportSchemaToJson).toHaveBeenCalled(); - expect(result.name).toBe('w:ins'); expect(result.attributes).toEqual({ 'w:id': '123', @@ -113,6 +136,26 @@ describe('w:ins 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' }] }); + + 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-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/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 152833a8f5..e055b1476b 100644 --- a/packages/super-editor/src/tests/import-export/trackChangesRoundtrip.test.js +++ b/packages/super-editor/src/tests/import-export/trackChangesRoundtrip.test.js @@ -231,14 +231,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 f04cb0c119..a6dc447ce3 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)', @@ -300,6 +303,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)', @@ -314,6 +318,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 0000000000..2c2a6cf4f1 Binary files /dev/null and b/tests/behavior/tests/comments/fixtures/sd-1960-word-replacement-no-comments.docx differ 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 new file mode 100644 index 0000000000..514cf0a720 --- /dev/null +++ b/tests/behavior/tests/comments/tracked-change-adjacent-different-ids-resolution.spec.ts @@ -0,0 +1,174 @@ +import type { Page } from '@playwright/test'; +import { test, expect } from '../../fixtures/superdoc.js'; +import { + assertDocumentApiReady, + deleteText, + findFirstSelectionTarget, + findFirstTextRange, + insertText, + listTrackChanges, +} 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 } }); + +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 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, +): 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 = requireSelectionTarget(await findFirstSelectionTarget(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' }]); +});