Skip to content

Commit 45d03e1

Browse files
chittolinagchittolinacaio-pizzol
authored
SD-2443 - fix: comments being merged by range when they shouldn't (#2735)
* fix: comments being merged * fix: ooxml compliance * test: added more tests around paraId and paraIdParent * fix(comments-export): key commentsExtended.xml guard off file-set, not origin (SD-2443) detectDocumentOrigin stamps every file missing commentsExtended.xml as origin='google-docs' on import, including legacy Word range-based files, so the previous exportStrategy !== 'google-docs' guard never fired for the exact class of files SD-2443 targets. Testing 123.docx only round-tripped because the pre-existing hasThreadedComments safeguard caught one imported reply link; strip parent links and the regression returns. Flips the two commentThreadingProfile integration tests and the commentsExporter "still honors Google Docs export strategy" unit test that codified the bug (they asserted commentsExtended.xml should be dropped for comments.xml-only imports). Adds the range-based + origin=google-docs + no parent links regression case, plus a commentsExtended profile test so the override does not over-fire. --------- Co-authored-by: Gabriel Chittolina <gabrielchittolina1@gmail.com> Co-authored-by: Caio Pizzol <caio@superdoc.dev>
1 parent 4809517 commit 45d03e1

4 files changed

Lines changed: 193 additions & 17 deletions

File tree

‎packages/super-editor/src/editors/v1/core/super-converter/v2/exporter/commentsExporter.js‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,6 @@ export const getCommentDefinition = (comment, commentId, allComments, editor) =>
3737
const attributes = {
3838
'w:id': String(commentId),
3939
'w:author': comment.creatorName || comment.importedAuthor?.name,
40-
'w:email': comment.creatorEmail || comment.importedAuthor?.email,
4140
'w:date': toIsoNoFractional(comment.createdTime),
4241
'w:initials': getInitials(comment.creatorName),
4342
'w:done': comment.resolvedTime ? '1' : '0',
@@ -48,6 +47,7 @@ export const getCommentDefinition = (comment, commentId, allComments, editor) =>
4847
'custom:trackedChangeType': comment.trackedChangeType,
4948
'custom:trackedChangeDisplayType': comment.trackedChangeDisplayType || null,
5049
'custom:trackedDeletedText': comment.deletedText || null,
50+
'custom:email': comment.creatorEmail || comment.importedAuthor?.email,
5151
};
5252

5353
// Add the w15:paraIdParent attribute if the comment has a parent
@@ -132,7 +132,6 @@ export const updateCommentsXml = (commentDefs = [], commentsXml) => {
132132
commentDef.attributes = {
133133
'w:id': commentDef.attributes['w:id'],
134134
'w:author': commentDef.attributes['w:author'],
135-
'w:email': commentDef.attributes['w:email'],
136135
'w:date': commentDef.attributes['w:date'],
137136
'w:initials': commentDef.attributes['w:initials'],
138137
'custom:internalId': commentDef.attributes['custom:internalId'],
@@ -141,6 +140,7 @@ export const updateCommentsXml = (commentDefs = [], commentsXml) => {
141140
'custom:trackedChangeType': commentDef.attributes['custom:trackedChangeType'],
142141
'custom:trackedChangeDisplayType': commentDef.attributes['custom:trackedChangeDisplayType'],
143142
'custom:trackedDeletedText': commentDef.attributes['custom:trackedDeletedText'],
143+
'custom:email': commentDef.attributes['custom:email'],
144144
'xmlns:custom': 'http://schemas.openxmlformats.org/wordprocessingml/2006/main',
145145
};
146146
});
@@ -400,10 +400,17 @@ export const prepareCommentsXmlFilesForExport = ({
400400
relationships.push(generateRelationship('comments.xml'));
401401
emittedTargets.add('comments.xml');
402402

403+
// Key off the file-set capability, not exportStrategy: the importer tags
404+
// every file missing commentsExtended.xml as origin='google-docs', including
405+
// legacy Word range-based files, so exportStrategy can't distinguish them.
406+
const forceWordThreadingProfile =
407+
threadingProfile?.defaultStyle === 'range-based' && threadingProfile?.fileSet?.hasCommentsExtended === false;
408+
const effectiveThreadingProfile = forceWordThreadingProfile ? 'word' : threadingProfile || exportStrategy;
409+
403410
const commentsExtendedXml = updateCommentsExtendedXml(
404411
commentsWithParaIds,
405412
updatedXml['word/commentsExtended.xml'],
406-
threadingProfile || exportStrategy,
413+
effectiveThreadingProfile,
407414
);
408415

409416
// Only add the file and relationship if we're actually generating commentsExtended.xml

‎packages/super-editor/src/editors/v1/core/super-converter/v2/exporter/commentsExporter.test.js‎

Lines changed: 149 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -381,12 +381,133 @@ describe('prepareCommentsXmlFilesForExport', () => {
381381
expect(result.removedTargets).toHaveLength(0);
382382
});
383383
});
384+
385+
describe('threading profile overrides', () => {
386+
it('forces Word-style threading when profile is range-based and the import lacks commentsExtended.xml', () => {
387+
const threadingProfile = {
388+
defaultStyle: 'range-based',
389+
mixed: false,
390+
fileSet: {
391+
hasCommentsExtended: false,
392+
hasCommentsExtensible: false,
393+
hasCommentsIds: false,
394+
},
395+
};
396+
397+
// Multiple unthreaded comments — exercises the scenario where the
398+
// importer would otherwise guess thread parents from overlapping ranges.
399+
const unthreadedComments = [
400+
makeComment({ commentId: 'c1', commentParaId: 'AAAAAAA1' }),
401+
makeComment({ commentId: 'c2', commentParaId: 'AAAAAAA2' }),
402+
makeComment({ commentId: 'c3', commentParaId: 'AAAAAAA3' }),
403+
];
404+
const unthreadedDefs = unthreadedComments.map((c, i) => makeCommentDef(String(i), c.commentParaId));
405+
406+
const result = prepareCommentsXmlFilesForExport({
407+
convertedXml: makeConvertedXml(),
408+
defs: unthreadedDefs,
409+
commentsWithParaIds: unthreadedComments,
410+
exportType: 'external',
411+
threadingProfile,
412+
});
413+
414+
const extXml = result.documentXml['word/commentsExtended.xml'];
415+
expect(extXml).toBeDefined();
416+
const rel = result.relationships.find((r) => r.attributes.Target === 'commentsExtended.xml');
417+
expect(rel).toBeDefined();
418+
419+
// One w15:commentEx entry per comment, each with w15:paraId and NO
420+
// w15:paraIdParent — the missing parent ids are what prevents the
421+
// importer from reconstructing threads from overlapping ranges.
422+
const entries = extXml.elements[0].elements;
423+
expect(entries).toHaveLength(unthreadedComments.length);
424+
const paraIds = new Set();
425+
for (const entry of entries) {
426+
expect(entry.name).toBe('w15:commentEx');
427+
expect(entry.attributes['w15:paraId']).toBeDefined();
428+
expect(entry.attributes['w15:paraIdParent']).toBeUndefined();
429+
paraIds.add(entry.attributes['w15:paraId']);
430+
}
431+
expect(paraIds.size).toBe(unthreadedComments.length);
432+
});
433+
434+
it('emits commentsExtended.xml for range-based files with no original extended part, even when every comment is tagged origin=google-docs', () => {
435+
// Regression case: detectDocumentOrigin stamps every comment in a
436+
// comments.xml-only file as origin='google-docs', including legacy Word
437+
// range-based files. Without the fileSet-based guard, the exporter
438+
// silently dropped commentsExtended.xml here and re-import rebuilt
439+
// threads from range overlaps.
440+
const threadingProfile = {
441+
defaultStyle: 'range-based',
442+
mixed: false,
443+
fileSet: {
444+
hasCommentsExtended: false,
445+
hasCommentsExtensible: false,
446+
hasCommentsIds: false,
447+
},
448+
};
449+
450+
const importedAsGoogleDocs = [
451+
makeComment({ commentId: 'c1', commentParaId: '126B0C7F', origin: 'google-docs' }),
452+
makeComment({ commentId: 'c2', commentParaId: '126B0C80', origin: 'google-docs' }),
453+
];
454+
const importedDefs = [makeCommentDef('0', '126B0C7F'), makeCommentDef('1', '126B0C80')];
455+
456+
const result = prepareCommentsXmlFilesForExport({
457+
convertedXml: makeConvertedXml(),
458+
defs: importedDefs,
459+
commentsWithParaIds: importedAsGoogleDocs,
460+
exportType: 'external',
461+
threadingProfile,
462+
});
463+
464+
const extendedXml = result.documentXml['word/commentsExtended.xml'];
465+
expect(extendedXml).toBeDefined();
466+
467+
const entries = extendedXml.elements[0].elements;
468+
expect(entries).toHaveLength(2);
469+
for (const entry of entries) {
470+
expect(entry.attributes['w15:paraId']).toBeDefined();
471+
expect(entry.attributes['w15:paraIdParent']).toBeUndefined();
472+
}
473+
474+
const rel = result.relationships.find((r) => r.attributes.Target === 'commentsExtended.xml');
475+
expect(rel).toBeDefined();
476+
});
477+
478+
it('leaves existing commentsExtended profile untouched when the import already ships commentsExtended.xml', () => {
479+
// The override keys off fileSet.hasCommentsExtended === false. When the
480+
// import already carries commentsExtended.xml the importer classifies
481+
// the profile as 'commentsExtended' and the existing export path owns
482+
// it; the override must not re-enter.
483+
const threadingProfile = {
484+
defaultStyle: 'commentsExtended',
485+
mixed: false,
486+
fileSet: {
487+
hasCommentsExtended: true,
488+
hasCommentsExtensible: false,
489+
hasCommentsIds: false,
490+
},
491+
};
492+
493+
const result = prepareCommentsXmlFilesForExport({
494+
convertedXml: makeConvertedXml(),
495+
defs,
496+
commentsWithParaIds,
497+
exportType: 'external',
498+
threadingProfile,
499+
});
500+
501+
expect(result.documentXml['word/commentsExtended.xml']).toBeDefined();
502+
});
503+
});
384504
});
385505

386506
describe('getCommentDefinition', () => {
387507
it('preserves tracked change display metadata for exported tracked-change comments', () => {
388508
const definition = getCommentDefinition(
389509
makeComment({
510+
creatorEmail: 'author@example.com',
390511
trackedChange: true,
391512
trackedChangeType: 'trackFormat',
392513
trackedChangeText: 'https://example.com',
@@ -400,6 +521,8 @@ describe('getCommentDefinition', () => {
400521
expect(definition.attributes['custom:trackedChangeType']).toBe('trackFormat');
401522
expect(definition.attributes['custom:trackedChangeText']).toBe('https://example.com');
402523
expect(definition.attributes['custom:trackedChangeDisplayType']).toBe('hyperlinkAdded');
524+
expect(definition.attributes['custom:email']).toBe('author@example.com');
525+
expect(definition.attributes['w:email']).toBeUndefined();
403526
});
404527
});
405528

@@ -609,5 +732,31 @@ describe('updateCommentsXml', () => {
609732
const lastParagraph = updatedComment.elements[updatedComment.elements.length - 1];
610733

611734
expect(lastParagraph.attributes['w14:paraId']).toBe('ABC12345');
735+
expect(updatedComment.attributes['w:email']).toBeUndefined();
736+
expect(updatedComment.attributes['custom:email']).toBeUndefined();
737+
});
738+
739+
it('preserves custom author email attribute and omits w:email', () => {
740+
const commentDef = {
741+
type: 'element',
742+
name: 'w:comment',
743+
attributes: {
744+
'w:id': '1',
745+
'w:author': 'Author',
746+
'w:initials': 'A',
747+
'w15:paraId': 'EMAIL123',
748+
'custom:email': 'author@example.com',
749+
},
750+
elements: [{ type: 'element', name: 'w:p', attributes: {}, elements: [] }],
751+
};
752+
const commentsXml = {
753+
elements: [{ elements: [] }],
754+
};
755+
756+
const result = updateCommentsXml([commentDef], commentsXml);
757+
const updatedComment = result.elements[0].elements[0];
758+
759+
expect(updatedComment.attributes['w:email']).toBeUndefined();
760+
expect(updatedComment.attributes['custom:email']).toBe('author@example.com');
612761
});
613762
});

‎packages/super-editor/src/editors/v1/tests/export/commentThreadingProfile.test.js‎

Lines changed: 17 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -109,20 +109,21 @@ describe('Partial threading profile (nested-comments.docx)', () => {
109109
});
110110

111111
// ---------------------------------------------------------------------------
112-
// Scenario 2 – Google Docs profile, no threading (comments.xml only)
113-
// gdocs-single-comment.docx has: comments.xml with 1 non-threaded comment.
114-
// No commentsExtended / commentsIds / commentsExtensible.
115-
// Since there are no threaded comments, the exporter should NOT fabricate
116-
// auxiliary files — the range-based threading model is preserved.
112+
// Scenario 2 – Range-based profile without a shipped commentsExtended.xml
113+
// (gdocs-single-comment.docx: comments.xml + 1 non-threaded comment, no
114+
// commentsExtended / commentsIds / commentsExtensible).
115+
// The exporter must synthesize commentsExtended.xml so re-import does not
116+
// reconstruct threads from range overlaps. commentsIds / commentsExtensible
117+
// stay absent: they were not in the import file-set.
117118
// ---------------------------------------------------------------------------
118-
describe('Google Docs profile without threading (gdocs-single-comment.docx)', () => {
119+
describe('Range-based profile without commentsExtended (gdocs-single-comment.docx)', () => {
119120
let docx, media, mediaFiles, fonts;
120121

121122
beforeAll(async () => {
122123
({ docx, media, mediaFiles, fonts } = await loadTestDataForEditorTests('gdocs-single-comment.docx'));
123124
});
124125

125-
it('emits only comments.xml — no auxiliary files fabricated', async () => {
126+
it('synthesizes commentsExtended.xml and leaves commentsIds/Extensible absent', async () => {
126127
const { editor } = initTestEditor({ content: docx, media, mediaFiles, fonts });
127128

128129
try {
@@ -135,19 +136,16 @@ describe('Google Docs profile without threading (gdocs-single-comment.docx)', ()
135136
getUpdatedDocs: true,
136137
});
137138

138-
// comments.xml must be present
139139
expect(updatedDocs['word/comments.xml']).toEqual(expect.any(String));
140-
141-
// The three auxiliary files must all be null (removed / never existed)
142-
expect(updatedDocs['word/commentsExtended.xml']).toBeNull();
140+
expect(updatedDocs['word/commentsExtended.xml']).toEqual(expect.any(String));
143141
expect(updatedDocs['word/commentsIds.xml']).toBeNull();
144142
expect(updatedDocs['word/commentsExtensible.xml']).toBeNull();
145143
} finally {
146144
editor.destroy();
147145
}
148146
});
149147

150-
it('produces a zip with only comments.xml', async () => {
148+
it('produces a zip with comments.xml and the synthesized commentsExtended.xml', async () => {
151149
const { editor } = initTestEditor({ content: docx, media, mediaFiles, fonts });
152150

153151
try {
@@ -161,13 +159,18 @@ describe('Google Docs profile without threading (gdocs-single-comment.docx)', ()
161159
const zip = await zipper.unzip(blob);
162160

163161
expect(zip.file('word/comments.xml')).not.toBeNull();
164-
expect(zip.file('word/commentsExtended.xml')).toBeNull();
162+
expect(zip.file('word/commentsExtended.xml')).not.toBeNull();
165163
expect(zip.file('word/commentsIds.xml')).toBeNull();
166164
expect(zip.file('word/commentsExtensible.xml')).toBeNull();
167165

166+
const extendedXml = await zip.file('word/commentsExtended.xml').async('string');
167+
const paraIdMatches = extendedXml.match(/w15:paraId="/g) ?? [];
168+
expect(paraIdMatches.length).toBe(comments.length);
169+
expect(extendedXml).not.toContain('w15:paraIdParent');
170+
168171
const contentTypes = await zip.file('[Content_Types].xml').async('string');
169172
expect(contentTypes).toContain('/word/comments.xml');
170-
expect(contentTypes).not.toContain('/word/commentsExtended.xml');
173+
expect(contentTypes).toContain('/word/commentsExtended.xml');
171174
expect(contentTypes).not.toContain('/word/commentsIds.xml');
172175
expect(contentTypes).not.toContain('/word/commentsExtensible.xml');
173176
} finally {

‎packages/super-editor/src/editors/v1/tests/import/documentCommentsImporter.unit.test.js‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,7 @@ const buildDocx = ({ comments = [], extended = [], documentRanges = [] } = {}) =
4545
'custom:trackedChangeType': comment.trackedChangeType,
4646
'custom:trackedChangeDisplayType': comment.trackedChangeDisplayType,
4747
'custom:trackedDeletedText': comment.trackedDeletedText,
48+
...(comment.customEmail ? { 'custom:email': comment.customEmail } : {}),
4849
},
4950
elements: comment.elements ?? [{ fakeParaId: comment.paraId ?? `para-${comment.id}` }],
5051
}));
@@ -280,6 +281,22 @@ describe('importCommentData metadata parsing', () => {
280281
const [comment] = importCommentData({ docx });
281282
expect(comment.elements).toHaveLength(2);
282283
});
284+
285+
it('reads custom:email when w:email is absent', () => {
286+
const docx = buildDocx({
287+
comments: [
288+
{
289+
id: 6,
290+
author: 'Custom Email',
291+
customEmail: 'custom@example.com',
292+
},
293+
],
294+
});
295+
delete docx['word/comments.xml'].elements[0].elements[0].attributes['w:email'];
296+
297+
const [comment] = importCommentData({ docx });
298+
expect(comment.creatorEmail).toBe('custom@example.com');
299+
});
283300
});
284301

285302
describe('importCommentData extended metadata', () => {

0 commit comments

Comments
 (0)