Skip to content

Commit b2f088b

Browse files
authored
fix(track-changes): remove logic that combines adjacent TCs with different IDs (#2326)
* fix(track-changes): update tests and logic for handling adjacent tracked changes with different IDs * fix: pair imported Word replacements without comments and preserve source ids * chore: fix cli test
1 parent a79fcaa commit b2f088b

19 files changed

Lines changed: 1043 additions & 130 deletions

apps/cli/src/lib/bootstrap.ts

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -231,6 +231,10 @@ function sleep(ms: number): Promise<void> {
231231
return new Promise<void>((resolve) => setTimeout(resolve, ms));
232232
}
233233

234+
function nextTimerTurn(): Promise<void> {
235+
return new Promise<void>((resolve) => setTimeout(resolve, 0));
236+
}
237+
234238
// ---------------------------------------------------------------------------
235239
// Bootstrap claim
236240
// ---------------------------------------------------------------------------
@@ -273,7 +277,16 @@ export async function claimBootstrap(
273277

274278
const observer = observeCompetitor(ydoc);
275279
try {
276-
if (settlingMs > 0) await sleep(settlingMs);
280+
if (settlingMs > 0) {
281+
await sleep(settlingMs);
282+
283+
// Give already-due timer callbacks one more turn to run before the
284+
// final ownership check. This makes bootstrap claiming more
285+
// conservative under event-loop jitter, where a competing marker can
286+
// be queued before the settling window ends but execute immediately
287+
// after our sleep resolves.
288+
await nextTimerTurn();
289+
}
277290

278291
const competitor = observer.getCompetitor();
279292
if (competitor) return { granted: false, competitor };

packages/super-editor/src/core/super-converter/v2/importer/documentCommentsImporter.js

Lines changed: 9 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -265,8 +265,6 @@ const extractCommentRangesFromDocument = (docx, converter) => {
265265
let positionIndex = 0;
266266
let lastElementWasCommentMarker = false;
267267
const recentlyClosedComments = new Set();
268-
let lastTrackedChange = null;
269-
270268
const walkElements = (elements, currentTrackedChangeId = null) => {
271269
if (!elements || !Array.isArray(elements)) return;
272270

@@ -311,61 +309,25 @@ const extractCommentRangesFromDocument = (docx, converter) => {
311309
}
312310
lastElementWasCommentMarker = true;
313311
} else if (isTrackedChange) {
314-
const trackedChangeId = element.attributes?.['w:id'];
315-
const author = element.attributes?.['w:author'];
316-
const date = element.attributes?.['w:date'];
317-
const elementType = element.name;
318-
let mappedId = trackedChangeId;
319-
let isReplacement = false;
320-
321-
if (trackedChangeId !== undefined && converter) {
322-
if (!converter.trackedChangeIdMap) {
323-
converter.trackedChangeIdMap = new Map();
324-
}
325-
326-
// Word uses different IDs for deletion and insertion in replacements, link them by same author/date
327-
if (
328-
lastTrackedChange &&
329-
lastTrackedChange.type !== elementType &&
330-
lastTrackedChange.author === author &&
331-
lastTrackedChange.date === date
332-
) {
333-
mappedId = lastTrackedChange.mappedId;
334-
converter.trackedChangeIdMap.set(String(trackedChangeId), mappedId);
335-
isReplacement = true;
336-
} else {
337-
if (!converter.trackedChangeIdMap.has(String(trackedChangeId))) {
338-
converter.trackedChangeIdMap.set(String(trackedChangeId), uuidv4());
339-
}
340-
mappedId = converter.trackedChangeIdMap.get(String(trackedChangeId));
341-
}
342-
}
343-
344-
if (currentTrackedChangeId === null) {
345-
if (isReplacement) {
346-
lastTrackedChange = null;
347-
} else {
348-
lastTrackedChange = {
349-
type: elementType,
350-
author,
351-
date,
352-
mappedId,
353-
wordId: String(trackedChangeId),
354-
};
355-
}
356-
}
312+
// ID mapping and replacement pairing are handled by trackedChangeIdMapper.
313+
// Here we only associate recently-closed comments with the tracked change.
314+
const wordId = element.attributes?.['w:id'];
315+
const mappedId =
316+
wordId != null
317+
? (converter?.trackedChangeIdMap?.get(String(wordId)) ?? String(wordId))
318+
: currentTrackedChangeId;
357319

358320
if (mappedId && recentlyClosedComments.size > 0) {
359321
recentlyClosedComments.forEach((commentId) => {
360322
if (!commentsInTrackedChanges.has(commentId)) {
361-
commentsInTrackedChanges.set(commentId, String(mappedId));
323+
commentsInTrackedChanges.set(commentId, mappedId);
362324
}
363325
});
364326
}
365327
recentlyClosedComments.clear();
366328

367329
if (element.elements && Array.isArray(element.elements)) {
368-
walkElements(element.elements, mappedId !== undefined ? String(mappedId) : currentTrackedChangeId);
330+
walkElements(element.elements, mappedId);
369331
}
370332
} else {
371333
if (lastElementWasCommentMarker) {
@@ -375,7 +337,6 @@ const extractCommentRangesFromDocument = (docx, converter) => {
375337

376338
if (element.name === 'w:p') {
377339
recentlyClosedComments.clear();
378-
lastTrackedChange = null;
379340
}
380341

381342
if (element.elements && Array.isArray(element.elements)) {

packages/super-editor/src/core/super-converter/v2/importer/docxImporter.js

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import { autoPageHandlerEntity, autoTotalPageCountEntity } from './autoPageNumbe
1818
import { pageReferenceEntity } from './pageReferenceImporter.js';
1919
import { pictNodeHandlerEntity } from './pictNodeImporter.js';
2020
import { importCommentData } from './documentCommentsImporter.js';
21+
import { buildTrackedChangeIdMap } from './trackedChangeIdMapper.js';
2122
import { importFootnoteData, importEndnoteData } from './documentFootnotesImporter.js';
2223
import { getDefaultStyleDefinition } from '@converter/docx-helpers/index.js';
2324
import { pruneIgnoredNodes } from './ignoredNodes.js';
@@ -148,6 +149,7 @@ export const createDocumentJson = (docx, converter, editor) => {
148149

149150
patchNumberingDefinitions(docx);
150151
const numbering = getNumberingDefinitions(docx);
152+
converter.trackedChangeIdMap = buildTrackedChangeIdMap(docx);
151153
const comments = importCommentData({ docx, nodeListHandler, converter, editor });
152154
const footnotes = importFootnoteData({ docx, nodeListHandler, converter, editor, numbering });
153155
const endnotes = importEndnoteData({ docx, nodeListHandler, converter, editor, numbering });
Lines changed: 150 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,150 @@
1+
// @ts-check
2+
import { v4 as uuidv4 } from 'uuid';
3+
4+
/**
5+
* @typedef {{ type: string, author: string, date: string, internalId: string }} TrackedChangeEntry
6+
* @typedef {{ lastTrackedChange: TrackedChangeEntry | null }} WalkContext
7+
*/
8+
9+
const TRACKED_CHANGE_NAMES = new Set(['w:ins', 'w:del']);
10+
11+
/**
12+
* Non-content marker elements that can appear between the two halves of a Word
13+
* replacement without breaking the pairing. These are range/annotation markers
14+
* that carry no document content.
15+
*
16+
* Any element NOT in this set (e.g. w:r, w:hyperlink, w:sdt) is treated as
17+
* content and resets the pairing state so unrelated revisions in the same
18+
* paragraph are never falsely linked.
19+
*/
20+
const PAIRING_TRANSPARENT_NAMES = new Set([
21+
'w:commentRangeStart',
22+
'w:commentRangeEnd',
23+
'w:bookmarkStart',
24+
'w:bookmarkEnd',
25+
'w:proofErr',
26+
'w:permStart',
27+
'w:permEnd',
28+
'w:moveFromRangeStart',
29+
'w:moveFromRangeEnd',
30+
'w:moveToRangeStart',
31+
'w:moveToRangeEnd',
32+
]);
33+
34+
/**
35+
* Two adjacent tracked changes form a Word replacement pair when they are
36+
* opposite types (delete vs insert) from the same author at the same timestamp.
37+
*
38+
* @param {TrackedChangeEntry} previous
39+
* @param {{ type: string, author: string, date: string }} current
40+
* @returns {boolean}
41+
*/
42+
function isReplacementPair(previous, current) {
43+
return previous.type !== current.type && previous.author === current.author && previous.date === current.date;
44+
}
45+
46+
/**
47+
* Assigns an internal UUID to a tracked change element. Adjacent replacement
48+
* halves (w:del + w:ins with matching author/date) share the same UUID.
49+
*
50+
* @param {object} element XML element (w:ins or w:del)
51+
* @param {Map<string, string>} idMap Accumulates Word ID → internal UUID
52+
* @param {WalkContext} context Mutable walk state for replacement pairing
53+
* @param {boolean} insideTrackedChange Whether this element is nested in another tracked change
54+
*/
55+
function assignInternalId(element, idMap, context, insideTrackedChange) {
56+
const wordId = String(element.attributes?.['w:id'] ?? '');
57+
if (!wordId) return;
58+
59+
// Nested tracked changes get their own UUID but are never paired.
60+
if (insideTrackedChange) {
61+
if (!idMap.has(wordId)) {
62+
idMap.set(wordId, uuidv4());
63+
}
64+
return;
65+
}
66+
67+
const current = {
68+
type: element.name,
69+
author: element.attributes?.['w:author'] ?? '',
70+
date: element.attributes?.['w:date'] ?? '',
71+
};
72+
73+
if (context.lastTrackedChange && isReplacementPair(context.lastTrackedChange, current)) {
74+
// Second half of a replacement — share the first half's UUID, but only
75+
// if this w:id hasn't already been mapped. A reused id that was already
76+
// part of an earlier pair must keep its original mapping.
77+
if (!idMap.has(wordId)) {
78+
idMap.set(wordId, context.lastTrackedChange.internalId);
79+
}
80+
context.lastTrackedChange = null;
81+
} else {
82+
// Reuse an existing mapping when the same w:id appears more than once
83+
// (Word reuses tracked-change ids across the document). Minting a fresh
84+
// UUID here would overwrite the earlier entry and break any replacement
85+
// pair that was already recorded for this id.
86+
const internalId = idMap.get(wordId) ?? uuidv4();
87+
idMap.set(wordId, internalId);
88+
context.lastTrackedChange = { ...current, internalId };
89+
}
90+
}
91+
92+
/**
93+
* Recursively walks XML elements, assigning internal UUIDs to every tracked
94+
* change and pairing adjacent replacements.
95+
*
96+
* @param {Array} elements
97+
* @param {Map<string, string>} idMap
98+
* @param {WalkContext} context
99+
* @param {boolean} [insideTrackedChange]
100+
*/
101+
function walkElements(elements, idMap, context, insideTrackedChange = false) {
102+
if (!Array.isArray(elements)) return;
103+
104+
for (const element of elements) {
105+
if (TRACKED_CHANGE_NAMES.has(element.name)) {
106+
assignInternalId(element, idMap, context, insideTrackedChange);
107+
108+
if (element.elements) {
109+
// Descend with an isolated context so content inside a tracked change
110+
// cannot clear the outer replacement candidate.
111+
walkElements(element.elements, idMap, { lastTrackedChange: null }, /* insideTrackedChange */ true);
112+
}
113+
} else {
114+
// Content-bearing elements break replacement pairing. Only non-content
115+
// markers (comment/bookmark/permission ranges) are transparent.
116+
if (!PAIRING_TRANSPARENT_NAMES.has(element.name)) {
117+
context.lastTrackedChange = null;
118+
}
119+
120+
if (element.elements) {
121+
walkElements(element.elements, idMap, context, insideTrackedChange);
122+
}
123+
}
124+
}
125+
}
126+
127+
/**
128+
* Builds a map from OOXML `w:id` values to stable internal UUIDs by scanning
129+
* `word/document.xml`.
130+
*
131+
* Word tracked replacements use separate `w:id` values for the delete and
132+
* insert halves. This function detects adjacent opposite-type changes with
133+
* matching author and date and maps both halves to the same internal UUID so
134+
* the editor can resolve them as a single logical change.
135+
*
136+
* Must run before comment import so all consumers — translators, comment
137+
* helpers, and the tracked-change resolver — see a fully populated map.
138+
*
139+
* @param {object} docx Parsed DOCX package
140+
* @returns {Map<string, string>} Word `w:id` → internal UUID
141+
*/
142+
export function buildTrackedChangeIdMap(docx) {
143+
const body = docx?.['word/document.xml']?.elements?.[0];
144+
if (!body?.elements) return new Map();
145+
146+
const idMap = new Map();
147+
walkElements(body.elements, idMap, { lastTrackedChange: null });
148+
149+
return idMap;
150+
}

0 commit comments

Comments
 (0)