diff --git a/packages/super-editor/src/core/DocxZipper.js b/packages/super-editor/src/core/DocxZipper.js index 6340464537..9fc3d5a405 100644 --- a/packages/super-editor/src/core/DocxZipper.js +++ b/packages/super-editor/src/core/DocxZipper.js @@ -3,6 +3,7 @@ import JSZip from 'jszip'; import { getContentTypesFromXml, base64ToUint8Array } from './super-converter/helpers.js'; import { ensureXmlString, isXmlLike } from './encoding-helpers.js'; import { DOCX } from '@superdoc/common'; +import { COMMENT_FILE_BASENAMES } from './super-converter/constants.js'; /** * Class to handle unzipping and zipping of docx files @@ -143,9 +144,13 @@ class DocxZipper { (el) => el.name === 'Override' && el.attributes.PartName === '/word/commentsExtensible.xml', ); + /** + * Check if a file will exist in the final zip output. + * A null value in updatedDocs means the file is explicitly deleted. + */ const hasFile = (filename) => { if (updatedDocs && Object.prototype.hasOwnProperty.call(updatedDocs, filename)) { - return true; + return updatedDocs[filename] !== null; } if (!docx?.files) return false; if (!fromJson) return Boolean(docx.files[filename]); @@ -205,9 +210,23 @@ class DocxZipper { } }); + // Prune stale comment Override entries for parts that will not exist in the final zip. + const commentPartNames = COMMENT_FILE_BASENAMES.map((name) => `/word/${name}`); + const staleOverridePartNames = commentPartNames.filter((partName) => { + const filename = partName.slice(1); // strip leading / + return !hasFile(filename); + }); + const beginningString = ''; let updatedContentTypesXml = contentTypesXml.replace(beginningString, `${beginningString}${typesString}`); + // Remove Override elements for comment parts that no longer exist + for (const partName of staleOverridePartNames) { + const escapedPartName = partName.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); + const overrideRegex = new RegExp(`\\s*]*PartName="${escapedPartName}"[^>]*/>`, 'g'); + updatedContentTypesXml = updatedContentTypesXml.replace(overrideRegex, ''); + } + // Include any header/footer targets referenced from document relationships let relationshipsXml = updatedDocs['word/_rels/document.xml.rels']; if (!relationshipsXml) { @@ -298,10 +317,13 @@ class DocxZipper { zip.file(file.name, content); } - // Replace updated docs + // Replace updated docs (null = delete from zip) Object.keys(updatedDocs).forEach((key) => { - const content = updatedDocs[key]; - zip.file(key, content); + if (updatedDocs[key] === null) { + zip.remove(key); + } else { + zip.file(key, updatedDocs[key]); + } }); Object.keys(media).forEach((path) => { @@ -337,9 +359,13 @@ class DocxZipper { }); await Promise.all(filePromises); - // Make replacements of updated docs + // Make replacements of updated docs (null = delete from zip) Object.keys(updatedDocs).forEach((key) => { - unzippedOriginalDocx.file(key, updatedDocs[key]); + if (updatedDocs[key] === null) { + unzippedOriginalDocx.remove(key); + } else { + unzippedOriginalDocx.file(key, updatedDocs[key]); + } }); Object.keys(media).forEach((path) => { diff --git a/packages/super-editor/src/core/DocxZipper.test.js b/packages/super-editor/src/core/DocxZipper.test.js index b8295d1413..8af0a5a68e 100644 --- a/packages/super-editor/src/core/DocxZipper.test.js +++ b/packages/super-editor/src/core/DocxZipper.test.js @@ -256,6 +256,44 @@ describe('DocxZipper - updateContentTypes', () => { expect(updatedContentTypes).toContain('/word/header1.xml'); expect(updatedContentTypes).toContain('/word/footer1.xml'); }); + + it('removes stale comment overrides when updated docs mark comment files as deleted', async () => { + const zipper = new DocxZipper(); + const zip = new JSZip(); + + const contentTypes = ` + + + + + + + + + + `; + zip.file('[Content_Types].xml', contentTypes); + zip.file( + 'word/document.xml', + '', + ); + + const updatedDocs = { + 'word/comments.xml': null, + 'word/commentsExtended.xml': null, + 'word/commentsIds.xml': null, + 'word/commentsExtensible.xml': null, + }; + + await zipper.updateContentTypes(zip, {}, false, updatedDocs); + + const updatedContentTypes = await zip.file('[Content_Types].xml').async('string'); + expect(updatedContentTypes).toContain('PartName="/word/document.xml"'); + expect(updatedContentTypes).not.toContain('PartName="/word/comments.xml"'); + expect(updatedContentTypes).not.toContain('PartName="/word/commentsExtended.xml"'); + expect(updatedContentTypes).not.toContain('PartName="/word/commentsIds.xml"'); + expect(updatedContentTypes).not.toContain('PartName="/word/commentsExtensible.xml"'); + }); }); describe('DocxZipper - exportFromCollaborativeDocx media handling', () => { @@ -299,3 +337,119 @@ describe('DocxZipper - exportFromCollaborativeDocx media handling', () => { expect(Array.from(img2)).toEqual([87, 111, 114, 108, 100]); }); }); + +describe('DocxZipper - comment file deletion', () => { + const contentTypesWithComments = ` + + + + + + + + + `; + + const updatedDocsWithCommentDeletes = { + 'word/document.xml': '', + 'word/comments.xml': null, + 'word/commentsExtended.xml': null, + 'word/commentsIds.xml': null, + 'word/commentsExtensible.xml': null, + }; + + it('removes stale comment files in collaborative export path when null sentinels are provided', async () => { + const zipper = new DocxZipper(); + const docx = [ + { name: '[Content_Types].xml', content: contentTypesWithComments }, + { + name: 'word/document.xml', + content: '', + }, + { + name: 'word/comments.xml', + content: '', + }, + { + name: 'word/commentsExtended.xml', + content: '', + }, + { + name: 'word/commentsIds.xml', + content: '', + }, + { + name: 'word/commentsExtensible.xml', + content: '', + }, + ]; + + const result = await zipper.updateZip({ + docx, + updatedDocs: updatedDocsWithCommentDeletes, + media: {}, + fonts: {}, + isHeadless: true, + }); + + const readBack = await new JSZip().loadAsync(result); + expect(readBack.file('word/comments.xml')).toBeNull(); + expect(readBack.file('word/commentsExtended.xml')).toBeNull(); + expect(readBack.file('word/commentsIds.xml')).toBeNull(); + expect(readBack.file('word/commentsExtensible.xml')).toBeNull(); + + const updatedContentTypes = await readBack.file('[Content_Types].xml').async('string'); + expect(updatedContentTypes).not.toContain('PartName="/word/comments.xml"'); + expect(updatedContentTypes).not.toContain('PartName="/word/commentsExtended.xml"'); + expect(updatedContentTypes).not.toContain('PartName="/word/commentsIds.xml"'); + expect(updatedContentTypes).not.toContain('PartName="/word/commentsExtensible.xml"'); + }); + + it('removes stale comment files in original-file export path when null sentinels are provided', async () => { + const zipper = new DocxZipper(); + const originalZip = new JSZip(); + originalZip.file('[Content_Types].xml', contentTypesWithComments); + originalZip.file( + 'word/document.xml', + '', + ); + originalZip.file( + 'word/comments.xml', + '', + ); + originalZip.file( + 'word/commentsExtended.xml', + '', + ); + originalZip.file( + 'word/commentsIds.xml', + '', + ); + originalZip.file( + 'word/commentsExtensible.xml', + '', + ); + const originalDocxFile = await originalZip.generateAsync({ type: 'nodebuffer' }); + + const result = await zipper.updateZip({ + docx: [], + updatedDocs: updatedDocsWithCommentDeletes, + originalDocxFile, + media: {}, + fonts: {}, + isHeadless: true, + }); + + const readBack = await new JSZip().loadAsync(result); + expect(readBack.file('word/comments.xml')).toBeNull(); + expect(readBack.file('word/commentsExtended.xml')).toBeNull(); + expect(readBack.file('word/commentsIds.xml')).toBeNull(); + expect(readBack.file('word/commentsExtensible.xml')).toBeNull(); + + const updatedContentTypes = await readBack.file('[Content_Types].xml').async('string'); + expect(updatedContentTypes).not.toContain('PartName="/word/comments.xml"'); + expect(updatedContentTypes).not.toContain('PartName="/word/commentsExtended.xml"'); + expect(updatedContentTypes).not.toContain('PartName="/word/commentsIds.xml"'); + expect(updatedContentTypes).not.toContain('PartName="/word/commentsExtensible.xml"'); + }); +}); diff --git a/packages/super-editor/src/core/Editor.ts b/packages/super-editor/src/core/Editor.ts index 7f806c7369..ae4e4dcce1 100644 --- a/packages/super-editor/src/core/Editor.ts +++ b/packages/super-editor/src/core/Editor.ts @@ -47,6 +47,7 @@ import { createLinkedChildEditor } from '@core/child-editor/index.js'; import { unflattenListsInHtml } from './inputRules/html/html-helpers.js'; import { SuperValidator } from '@core/super-validator/index.js'; import { createDocFromMarkdown, createDocFromHTML } from '@core/helpers/index.js'; +import { COMMENT_FILE_BASENAMES } from '@core/super-converter/constants.js'; import { isHeadless } from '../utils/headless-helpers.js'; import { canUseDOM } from '../utils/canUseDOM.js'; import { buildSchemaSummary } from './schema-summary.js'; @@ -2586,7 +2587,7 @@ export class Editor extends EventEmitter { getUpdatedDocs?: boolean; fieldsHighlightColor?: string | null; compression?: 'DEFLATE' | 'STORE'; - } = {}): Promise | ProseMirrorJSON | string | undefined> { + } = {}): Promise | ProseMirrorJSON | string | undefined> { try { // Use provided comments, or fall back to imported comments from converter const effectiveComments = comments ?? this.converter.comments ?? []; @@ -2654,7 +2655,7 @@ export class Editor extends EventEmitter { const coreXmlData = this.converter.convertedXml['docProps/core.xml']; const coreXml = coreXmlData?.elements?.[0] ? this.converter.schemaToXml(coreXmlData.elements[0]) : null; - const updatedDocs: Record = { + const updatedDocs: Record = { ...this.options.customUpdatedFiles, 'word/document.xml': String(documentXml), 'docProps/custom.xml': String(customXml), @@ -2677,26 +2678,15 @@ export class Editor extends EventEmitter { updatedDocs['word/_rels/footnotes.xml.rels'] = String(footnotesRelsXml); } - if (preparedComments.length) { - const commentsXml = this.converter.schemaToXml(this.converter.convertedXml['word/comments.xml'].elements[0]); - updatedDocs['word/comments.xml'] = String(commentsXml); - - const commentsExtended = this.converter.convertedXml['word/commentsExtended.xml']; - if (commentsExtended?.elements?.[0]) { - const commentsExtendedXml = this.converter.schemaToXml(commentsExtended.elements[0]); - updatedDocs['word/commentsExtended.xml'] = String(commentsExtendedXml); - } - - const commentsExtensible = this.converter.convertedXml['word/commentsExtensible.xml']; - if (commentsExtensible?.elements?.[0]) { - const commentsExtensibleXml = this.converter.schemaToXml(commentsExtensible.elements[0]); - updatedDocs['word/commentsExtensible.xml'] = String(commentsExtensibleXml); - } - - const commentsIds = this.converter.convertedXml['word/commentsIds.xml']; - if (commentsIds?.elements?.[0]) { - const commentsIdsXml = this.converter.schemaToXml(commentsIds.elements[0]); - updatedDocs['word/commentsIds.xml'] = String(commentsIdsXml); + // Serialize each comment file if it exists in convertedXml, otherwise mark as null + // for deletion from the zip (removes stale originals). + const commentFiles = COMMENT_FILE_BASENAMES.map((name) => `word/${name}`); + for (const path of commentFiles) { + const data = this.converter.convertedXml[path]; + if (data?.elements?.[0]) { + updatedDocs[path] = String(this.converter.schemaToXml(data.elements[0])); + } else { + updatedDocs[path] = null; } } diff --git a/packages/super-editor/src/core/super-converter/SuperConverter.js b/packages/super-editor/src/core/super-converter/SuperConverter.js index 969616a1ac..ac95aef706 100644 --- a/packages/super-editor/src/core/super-converter/SuperConverter.js +++ b/packages/super-editor/src/core/super-converter/SuperConverter.js @@ -14,6 +14,7 @@ import { import { prepareFootnotesXmlForExport } from './v2/exporter/footnotesExporter.js'; import { DocxHelpers } from './docx-helpers/index.js'; import { mergeRelationshipElements } from './relationship-helpers.js'; +import { COMMENT_RELATIONSHIP_TYPES } from './constants.js'; const FONT_FAMILY_FALLBACKS = Object.freeze({ swiss: 'Arial, sans-serif', @@ -190,6 +191,9 @@ class SuperConverter { this.inlineDocumentFonts = []; this.commentThreadingProfile = null; + /** @type {string[]} Warnings emitted during export */ + this.exportWarnings = []; + // Store custom highlight colors this.docHiglightColors = new Set([]); @@ -1098,6 +1102,9 @@ class SuperConverter { exportJsonOnly = false, fieldsHighlightColor, ) { + // Reset export warnings for this export cycle + this.exportWarnings = []; + // Filter out synthetic tracked change comments - they shouldn't be exported to comments.xml const exportableComments = comments.filter((c) => !c.trackedChange); const commentsWithParaIds = exportableComments.map((c) => prepareCommentParaIds(c)); @@ -1144,26 +1151,42 @@ class SuperConverter { editor, ); - // Update content types and comments files as needed - let updatedXml = { ...this.convertedXml }; - let commentsRels = []; - if (comments.length) { - const { documentXml, relationships } = this.#prepareCommentsXmlFilesForExport({ - defs: params.exportedCommentDefs, - exportType: commentsExportType, - commentsWithParaIds, - }); - updatedXml = { ...documentXml }; - commentsRels = relationships; - } + // Update content types and comments files as needed — always run so cleanup + // happens even when all comments have been removed + const { + documentXml, + relationships: commentsRels, + removedTargets, + } = this.#prepareCommentsXmlFilesForExport({ + defs: params.exportedCommentDefs, + exportType: commentsExportType, + commentsWithParaIds, + }); + const updatedXml = { ...documentXml }; this.convertedXml = { ...this.convertedXml, ...updatedXml }; + // Physically remove comment parts that the exporter deleted from documentXml. + // The spread merge above only adds/overwrites keys — absent keys survive from + // the old this.convertedXml. Without this, Editor.ts sees stale data and + // serializes comment files that should have been null-sentinelled. + if (removedTargets?.length) { + for (const target of removedTargets) { + const key = target.startsWith('word/') ? target : `word/${target}`; + delete this.convertedXml[key]; + } + } + const headFootRels = this.#exportProcessHeadersFooters({ isFinalDoc }); // Update the rels table this.#exportProcessNewRelationships([...params.relationships, ...commentsRels, ...footnotesRels, ...headFootRels]); + // Prune relationships for comment parts that were removed + if (removedTargets?.length) { + this.#pruneCommentRelationships(removedTargets); + } + // Store SuperDoc version SuperConverter.setStoredSuperdocVersion(this.convertedXml); @@ -1238,7 +1261,12 @@ class SuperConverter { * Update comments files and relationships depending on export type */ #prepareCommentsXmlFilesForExport({ defs, exportType, commentsWithParaIds }) { - const { documentXml, relationships } = prepareCommentsXmlFilesForExport({ + const { + documentXml, + relationships, + removedTargets = [], + warnings = [], + } = prepareCommentsXmlFilesForExport({ exportType, convertedXml: this.convertedXml, defs, @@ -1246,7 +1274,11 @@ class SuperConverter { threadingProfile: this.commentThreadingProfile, }); - return { documentXml, relationships }; + if (warnings.length) { + this.exportWarnings.push(...warnings); + } + + return { documentXml, relationships, removedTargets }; } #exportProcessHeadersFooters({ isFinalDoc = false }) { @@ -1392,6 +1424,37 @@ class SuperConverter { relationships.elements = mergeRelationshipElements(relationships.elements, rels); } + /** + * Remove relationship entries for comment parts that are no longer being emitted. + * Matches by both normalized target AND comment relationship type to avoid + * accidentally pruning unrelated relationships. + * @param {string[]} removedTargets - bare filenames like 'commentsExtended.xml' + */ + #pruneCommentRelationships(removedTargets) { + const relsData = this.convertedXml['word/_rels/document.xml.rels']; + const relationships = relsData.elements.find((x) => x.name === 'Relationships'); + if (!relationships?.elements) return; + + const normalizeTarget = (target) => { + if (!target) return ''; + return target + .replace(/^\.\//, '') + .replace(/^\//, '') + .replace(/^word\//, ''); + }; + + const removedSet = new Set(removedTargets.map(normalizeTarget)); + + relationships.elements = relationships.elements.filter((rel) => { + const type = rel.attributes?.Type; + const target = normalizeTarget(rel.attributes?.Target); + if (COMMENT_RELATIONSHIP_TYPES.has(type) && removedSet.has(target)) { + return false; + } + return true; + }); + } + async #exportProcessMediaFiles(media = {}) { const processedData = { ...(this.convertedXml.media || {}), diff --git a/packages/super-editor/src/core/super-converter/SuperConverter.test.js b/packages/super-editor/src/core/super-converter/SuperConverter.test.js index c07c0fb760..4267f103c0 100644 --- a/packages/super-editor/src/core/super-converter/SuperConverter.test.js +++ b/packages/super-editor/src/core/super-converter/SuperConverter.test.js @@ -1324,3 +1324,102 @@ describe('SuperConverter styles fallback', () => { expect(styles?.elements?.[0]?.elements?.[0]?.attributes?.['w:styleId']).toBe('AltStyle2'); }); }); + +describe('SuperConverter comment cleanup on export', () => { + const makeCommentCleanupDocx = () => [ + { + name: 'word/document.xml', + content: ` + + Test + `, + }, + { + name: 'word/_rels/document.xml.rels', + content: ` + + + + + + + + `, + }, + { + name: 'docProps/custom.xml', + content: ` + + `, + }, + { + name: 'word/numbering.xml', + content: ` + `, + }, + { + name: 'word/comments.xml', + content: ` + `, + }, + { + name: 'word/commentsExtended.xml', + content: ` + `, + }, + { + name: 'word/commentsIds.xml', + content: ` + `, + }, + { + name: 'word/commentsExtensible.xml', + content: ` + `, + }, + ]; + + it('removes stale comment files and prunes only comment relationships when no comments remain', async () => { + const converter = new SuperConverter({ docx: makeCommentCleanupDocx() }); + converter.numbering = { abstracts: {}, definitions: {} }; + + const exportToXmlJsonSpy = vi.spyOn(converter, 'exportToXmlJson').mockReturnValue({ + result: converter.convertedXml['word/document.xml'].elements[0], + params: { + relationships: [], + media: {}, + exportedCommentDefs: [], + }, + }); + + await converter.exportToDocx({}, {}, {}, false, 'external', [], null, false, null); + + expect(converter.convertedXml['word/comments.xml']).toBeUndefined(); + expect(converter.convertedXml['word/commentsExtended.xml']).toBeUndefined(); + expect(converter.convertedXml['word/commentsIds.xml']).toBeUndefined(); + expect(converter.convertedXml['word/commentsExtensible.xml']).toBeUndefined(); + + const relationships = + converter.convertedXml['word/_rels/document.xml.rels'].elements.find((el) => el.name === 'Relationships') + .elements || []; + + expect( + relationships.some((rel) => + [ + 'http://schemas.openxmlformats.org/officeDocument/2006/relationships/comments', + 'http://schemas.microsoft.com/office/2011/relationships/commentsExtended', + 'http://schemas.microsoft.com/office/2016/09/relationships/commentsIds', + 'http://schemas.microsoft.com/office/2018/08/relationships/commentsExtensible', + ].includes(rel.attributes?.Type), + ), + ).toBe(false); + + // Non-comment relationships are retained even if they share a target name. + expect(relationships.some((rel) => rel.attributes?.Type?.includes('/hyperlink'))).toBe(true); + expect( + relationships.some((rel) => rel.attributes?.Type?.includes('/header') && rel.attributes?.Id === 'rId6'), + ).toBe(true); + + exportToXmlJsonSpy.mockRestore(); + }); +}); diff --git a/packages/super-editor/src/core/super-converter/constants.js b/packages/super-editor/src/core/super-converter/constants.js index 869fc81c37..4fe1a7ef5c 100644 --- a/packages/super-editor/src/core/super-converter/constants.js +++ b/packages/super-editor/src/core/super-converter/constants.js @@ -2,3 +2,19 @@ export const HYPERLINK_RELATIONSHIP_TYPE = 'http://schemas.openxmlformats.org/officeDocument/2006/relationships/hyperlink'; export const HEADER_RELATIONSHIP_TYPE = 'http://schemas.openxmlformats.org/officeDocument/2006/relationships/header'; export const FOOTER_RELATIONSHIP_TYPE = 'http://schemas.openxmlformats.org/officeDocument/2006/relationships/footer'; + +/** Bare filenames for all OOXML comment support parts */ +export const COMMENT_FILE_BASENAMES = [ + 'comments.xml', + 'commentsExtended.xml', + 'commentsIds.xml', + 'commentsExtensible.xml', +]; + +// Comment-related relationship types (used for pruning stale rels on export) +export const COMMENT_RELATIONSHIP_TYPES = new Set([ + 'http://schemas.openxmlformats.org/officeDocument/2006/relationships/comments', + 'http://schemas.microsoft.com/office/2011/relationships/commentsExtended', + 'http://schemas.microsoft.com/office/2016/09/relationships/commentsIds', + 'http://schemas.microsoft.com/office/2018/08/relationships/commentsExtensible', +]); diff --git a/packages/super-editor/src/core/super-converter/exporter-docx-defs.js b/packages/super-editor/src/core/super-converter/exporter-docx-defs.js index b420d96714..99bbaa9f08 100644 --- a/packages/super-editor/src/core/super-converter/exporter-docx-defs.js +++ b/packages/super-editor/src/core/super-converter/exporter-docx-defs.js @@ -6,7 +6,6 @@ * @property {Object} COMMENTS_IDS_XML_DEF - XML definition for comment identifiers. * @property {Object} DOCUMENT_RELS_XML_DEF - XML definition for document relationships. * @property {Object} PEOPLE_XML_DEF - XML definition for people-related information. - * @property {Object} CONTENT_TYPES - XML definition for custom settings. */ export const DEFAULT_DOCX_DEFS = { @@ -1258,155 +1257,6 @@ export const PEOPLE_XML_DEF = { ], }; -export const CONTENT_TYPES = { - declaration: { - attributes: { - version: '1.0', - encoding: 'UTF-8', - standalone: 'yes', - }, - }, - elements: [ - { - type: 'element', - name: 'Types', - attributes: { - xmlns: 'http://schemas.openxmlformats.org/package/2006/content-types', - }, - elements: [ - { - type: 'element', - name: 'Default', - attributes: { - Extension: 'rels', - ContentType: 'application/vnd.openxmlformats-package.relationships+xml', - }, - }, - { - type: 'element', - name: 'Default', - attributes: { - Extension: 'xml', - ContentType: 'application/xml', - }, - }, - { - type: 'element', - name: 'Override', - attributes: { - PartName: '/word/document.xml', - ContentType: 'application/vnd.openxmlformats-officedocument.wordprocessingml.document.main+xml', - }, - }, - { - type: 'element', - name: 'Override', - attributes: { - PartName: '/word/styles.xml', - ContentType: 'application/vnd.openxmlformats-officedocument.wordprocessingml.styles+xml', - }, - }, - { - type: 'element', - name: 'Override', - attributes: { - PartName: '/word/settings.xml', - ContentType: 'application/vnd.openxmlformats-officedocument.wordprocessingml.settings+xml', - }, - }, - { - type: 'element', - name: 'Override', - attributes: { - PartName: '/word/webSettings.xml', - ContentType: 'application/vnd.openxmlformats-officedocument.wordprocessingml.webSettings+xml', - }, - }, - { - type: 'element', - name: 'Override', - attributes: { - PartName: '/word/comments.xml', - ContentType: 'application/vnd.openxmlformats-officedocument.wordprocessingml.comments+xml', - }, - }, - { - type: 'element', - name: 'Override', - attributes: { - PartName: '/word/commentsExtended.xml', - ContentType: 'application/vnd.openxmlformats-officedocument.wordprocessingml.commentsExtended+xml', - }, - }, - { - type: 'element', - name: 'Override', - attributes: { - PartName: '/word/commentsIds.xml', - ContentType: 'application/vnd.openxmlformats-officedocument.wordprocessingml.commentsIds+xml', - }, - }, - { - type: 'element', - name: 'Override', - attributes: { - PartName: '/word/commentsExtensible.xml', - ContentType: 'application/vnd.openxmlformats-officedocument.wordprocessingml.commentsExtensible+xml', - }, - }, - { - type: 'element', - name: 'Override', - attributes: { - PartName: '/word/fontTable.xml', - ContentType: 'application/vnd.openxmlformats-officedocument.wordprocessingml.fontTable+xml', - }, - }, - { - type: 'element', - name: 'Override', - attributes: { - PartName: '/word/people.xml', - ContentType: 'application/vnd.openxmlformats-officedocument.wordprocessingml.people+xml', - }, - }, - { - type: 'element', - name: 'Override', - attributes: { - PartName: '/word/theme/theme1.xml', - ContentType: 'application/vnd.openxmlformats-officedocument.theme+xml', - }, - }, - { - type: 'element', - name: 'Override', - attributes: { - PartName: '/docProps/core.xml', - ContentType: 'application/vnd.openxmlformats-package.core-properties+xml', - }, - }, - { - type: 'element', - name: 'Override', - attributes: { - PartName: '/docProps/app.xml', - ContentType: 'application/vnd.openxmlformats-officedocument.extended-properties+xml', - }, - }, - { - type: 'element', - name: 'Override', - attributes: { - PartName: '/word/numbering.xml', - ContentType: 'application/vnd.openxmlformats-officedocument.wordprocessingml.numbering+xml', - }, - }, - ], - }, - ], -}; - /** * @type {CommentsXmlDefinitions} */ @@ -1417,5 +1267,4 @@ export const COMMENTS_XML_DEFINITIONS = { COMMENTS_IDS_XML_DEF, DOCUMENT_RELS_XML_DEF, PEOPLE_XML_DEF, - CONTENT_TYPES, }; diff --git a/packages/super-editor/src/core/super-converter/v2/exporter/commentsExporter.js b/packages/super-editor/src/core/super-converter/v2/exporter/commentsExporter.js index ada9dea835..0e66ac6271 100644 --- a/packages/super-editor/src/core/super-converter/v2/exporter/commentsExporter.js +++ b/packages/super-editor/src/core/super-converter/v2/exporter/commentsExporter.js @@ -2,6 +2,7 @@ import { translator as wPTranslator } from '@converter/v3/handlers/w/p'; import { carbonCopy } from '../../../utilities/carbonCopy.js'; import { COMMENT_REF, COMMENTS_XML_DEFINITIONS } from '../../exporter-docx-defs.js'; import { generateRandom32BitHex } from '../../../helpers/generateDocxRandomId.js'; +import { COMMENT_FILE_BASENAMES } from '../../constants.js'; /** * Insert w15:paraId into the comments @@ -148,7 +149,7 @@ export const updateCommentsXml = (commentDefs = [], commentsXml) => { /** * Determine export strategy based on comment origins - * @param {Array[Object]} comments The comments list + * @param {Object[]} comments The comments list * @returns {'word' | 'google-docs' | 'unknown'} The export strategy to use */ export const determineExportStrategy = (comments) => { @@ -175,7 +176,7 @@ const resolveThreadingStyle = (comment, threadingProfile) => { /** * This function updates the commentsExtended.xml structure with the comments list. * - * @param {Array[Object]} comments The comments list + * @param {Object[]} comments The comments list * @param {Object} commentsExtendedXml The commentsExtended.xml structure as JSON * @param {import('@superdoc/common').CommentThreadingProfile | 'word' | 'google-docs' | 'unknown'} threadingProfile * @returns {Object | null} The updated commentsExtended structure, or null if it shouldn't be generated @@ -240,41 +241,46 @@ export const updateCommentsExtendedXml = (comments = [], commentsExtendedXml, th }; /** - * Update commentsIds.xml and commentsExtensible.xml together since they have to - * share the same durableId for each comment. + * Update commentsIds.xml and/or commentsExtensible.xml. + * Either part may be null — only the provided parts are populated. + * Both share the same durable IDs when both are present. * - * @param {Array[Object]} comments The comments list - * @param {Object} commentsIds The commentsIds.xml structure as JSON - * @param {Object} extensible The commentsExtensible.xml structure as JSON - * @returns {Object} The updated commentsIds and commentsExtensible structures + * @param {Object[]} comments The comments list + * @param {Object | null} commentsIds The commentsIds.xml structure as JSON (null to skip) + * @param {Object | null} extensible The commentsExtensible.xml structure as JSON (null to skip) + * @returns {Object} The updated commentsIds and commentsExtensible structures (null for skipped parts) */ export const updateCommentsIdsAndExtensible = (comments = [], commentsIds, extensible) => { - const documentIdsUpdated = carbonCopy(commentsIds); - const extensibleUpdated = carbonCopy(extensible); + const documentIdsUpdated = commentsIds ? carbonCopy(commentsIds) : null; + const extensibleUpdated = extensible ? carbonCopy(extensible) : null; + + if (documentIdsUpdated) documentIdsUpdated.elements[0].elements = []; + if (extensibleUpdated) extensibleUpdated.elements[0].elements = []; - documentIdsUpdated.elements[0].elements = []; - extensibleUpdated.elements[0].elements = []; comments.forEach((comment) => { const newDurableId = generateRandom32BitHex(); - const newCommentIdDef = { - type: 'element', - name: 'w16cid:commentId', - attributes: { - 'w16cid:paraId': comment.commentParaId, - 'w16cid:durableId': newDurableId, - }, - }; - documentIdsUpdated.elements[0].elements.push(newCommentIdDef); - const newExtensible = { - type: 'element', - name: 'w16cex:commentExtensible', - attributes: { - 'w16cex:durableId': newDurableId, - 'w16cex:dateUtc': toIsoNoFractional(comment.createdTime), - }, - }; - extensibleUpdated.elements[0].elements.push(newExtensible); + if (documentIdsUpdated) { + documentIdsUpdated.elements[0].elements.push({ + type: 'element', + name: 'w16cid:commentId', + attributes: { + 'w16cid:paraId': comment.commentParaId, + 'w16cid:durableId': newDurableId, + }, + }); + } + + if (extensibleUpdated) { + extensibleUpdated.elements[0].elements.push({ + type: 'element', + name: 'w16cex:commentExtensible', + attributes: { + 'w16cex:durableId': newDurableId, + 'w16cex:dateUtc': toIsoNoFractional(comment.createdTime), + }, + }); + } }); return { @@ -283,15 +289,6 @@ export const updateCommentsIdsAndExtensible = (comments = [], commentsIds, exten }; }; -/** - * Generate the ocument.xml.rels definition - * - * @returns {Object} The updated document rels XML structure - */ -export const updateDocumentRels = () => { - return COMMENTS_XML_DEFINITIONS.DOCUMENT_RELS_XML_DEF; -}; - /** * Generate initial comments XML structure with no content * @@ -312,33 +309,12 @@ export const generateConvertedXmlWithCommentFiles = (convertedXml, fileSet = nul if (includeExtended) newXml['word/commentsExtended.xml'] = COMMENTS_XML_DEFINITIONS.COMMENTS_EXTENDED_XML_DEF; if (includeExtensible) newXml['word/commentsExtensible.xml'] = COMMENTS_XML_DEFINITIONS.COMMENTS_EXTENSIBLE_XML_DEF; if (includeIds) newXml['word/commentsIds.xml'] = COMMENTS_XML_DEFINITIONS.COMMENTS_IDS_XML_DEF; - newXml['[Content_Types].xml'] = COMMENTS_XML_DEFINITIONS.CONTENT_TYPES; + // Do NOT overwrite [Content_Types].xml here — DocxZipper.updateContentTypes() is the + // authoritative source that builds content types at zip-assembly time based on which + // files actually exist in updatedDocs. return newXml; }; -/** - * Get the comments files converted to XML - * - * @param {Object} converter The converter instance - * @returns {Object} The comments files converted to XML - */ -export const getCommentsFilesConverted = (converter, convertedXml) => { - const commentsXml = convertedXml['word/comments.xml']; - const commentsExtendedXml = convertedXml['word/commentsExtended.xml']; - const commentsIdsXml = convertedXml['word/commentsExtensible.xml']; - const commentsExtensibleXml = convertedXml['word/commentsIds.xml']; - const contentTypes = convertedXml['[Content_Types].xml']; - - return { - ...convertedXml, - 'word/comments.xml': converter.schemaToXml(commentsXml.elements[0]), - 'word/commentsExtended.xml': converter.schemaToXml(commentsExtendedXml.elements[0]), - 'word/commentsIds.xml': converter.schemaToXml(commentsIdsXml.elements[0]), - 'word/commentsExtensible.xml': converter.schemaToXml(commentsExtensibleXml.elements[0]), - '[Content_Types].xml': converter.schemaToXml(contentTypes.elements[0]), - }; -}; - /** * Remove comments files from the converted XML * @@ -368,11 +344,19 @@ export const generateRelationship = (target) => { return { ...rel }; }; +/** @type {readonly string[]} All possible comment support file targets */ +const ALL_COMMENT_TARGETS = COMMENT_FILE_BASENAMES; + /** * Generate comments files into convertedXml * - * @param {Object} param0 - * @returns + * @param {Object} params + * @param {Object} params.convertedXml Current converted XML map + * @param {Object[]} params.defs Export-ready `w:comment` definitions + * @param {Object[]} params.commentsWithParaIds Comments enriched with generated `commentParaId` + * @param {'clean' | string} params.exportType Export mode + * @param {import('@superdoc/common').CommentThreadingProfile | null} params.threadingProfile + * @returns {{ documentXml: Object, relationships: Object[], removedTargets: string[], warnings: string[] }} */ export const prepareCommentsXmlFilesForExport = ({ convertedXml, @@ -382,17 +366,34 @@ export const prepareCommentsXmlFilesForExport = ({ threadingProfile, }) => { const relationships = []; + const warnings = []; if (exportType === 'clean') { const documentXml = removeCommentsFilesFromConvertedXml(convertedXml); - return { documentXml, relationships }; + // Clean export: all comment parts are intentionally removed — no warnings + return { documentXml, relationships, removedTargets: ALL_COMMENT_TARGETS, warnings }; } + const hasComments = commentsWithParaIds && commentsWithParaIds.length > 0; + + // When all comments have been removed, clean up all comment parts + if (!hasComments) { + const documentXml = removeCommentsFilesFromConvertedXml(convertedXml); + const removedTargets = [...ALL_COMMENT_TARGETS]; + if (threadingProfile?.fileSet) { + warnings.push('All comments removed — cleaning up imported comment support files'); + } + return { documentXml, relationships, removedTargets, warnings }; + } + + const emittedTargets = new Set(); + const exportStrategy = determineExportStrategy(commentsWithParaIds); const updatedXml = generateConvertedXmlWithCommentFiles(convertedXml, threadingProfile?.fileSet); updatedXml['word/comments.xml'] = updateCommentsXml(defs, updatedXml['word/comments.xml']); relationships.push(generateRelationship('comments.xml')); + emittedTargets.add('comments.xml'); const commentsExtendedXml = updateCommentsExtendedXml( commentsWithParaIds, @@ -405,27 +406,54 @@ export const prepareCommentsXmlFilesForExport = ({ if (commentsExtendedXml !== null) { updatedXml['word/commentsExtended.xml'] = commentsExtendedXml; relationships.push(generateRelationship('commentsExtended.xml')); + emittedTargets.add('commentsExtended.xml'); } else { - // Remove the file from the XML structure so the importer uses range-based threading delete updatedXml['word/commentsExtended.xml']; + if (threadingProfile?.fileSet?.hasCommentsExtended) { + warnings.push('commentsExtended.xml removed — export strategy does not require it'); + } } - // Generate updates for documentIds.xml and commentsExtensible.xml here - // We do them at the same time as we need them to generate and share durable IDs between them - if (updatedXml['word/commentsIds.xml'] && updatedXml['word/commentsExtensible.xml']) { + // Generate updates for commentsIds.xml and/or commentsExtensible.xml independently. + // They share durable IDs when both are present, but either can exist without the other. + const hasIds = !!updatedXml['word/commentsIds.xml']; + const hasExtensible = !!updatedXml['word/commentsExtensible.xml']; + + if (hasIds !== hasExtensible) { + const present = hasIds ? 'commentsIds.xml' : 'commentsExtensible.xml'; + const absent = hasIds ? 'commentsExtensible.xml' : 'commentsIds.xml'; + warnings.push(`Partial comment file-set: ${present} present without ${absent}`); + } + + if (hasIds || hasExtensible) { const { documentIdsUpdated, extensibleUpdated } = updateCommentsIdsAndExtensible( commentsWithParaIds, - updatedXml['word/commentsIds.xml'], - updatedXml['word/commentsExtensible.xml'], + hasIds ? updatedXml['word/commentsIds.xml'] : null, + hasExtensible ? updatedXml['word/commentsExtensible.xml'] : null, ); - updatedXml['word/commentsIds.xml'] = documentIdsUpdated; - updatedXml['word/commentsExtensible.xml'] = extensibleUpdated; - relationships.push(generateRelationship('commentsIds.xml')); - relationships.push(generateRelationship('commentsExtensible.xml')); + if (documentIdsUpdated) { + updatedXml['word/commentsIds.xml'] = documentIdsUpdated; + relationships.push(generateRelationship('commentsIds.xml')); + emittedTargets.add('commentsIds.xml'); + } + if (extensibleUpdated) { + updatedXml['word/commentsExtensible.xml'] = extensibleUpdated; + relationships.push(generateRelationship('commentsExtensible.xml')); + emittedTargets.add('commentsExtensible.xml'); + } + } + + if (!threadingProfile && hasComments) { + warnings.push('Comments exist but no threading profile detected — using default export shape'); } + // Compute comment targets that are not emitted in this export cycle + const removedTargets = ALL_COMMENT_TARGETS.filter((target) => !emittedTargets.has(target)); + return { relationships, documentXml: updatedXml, + removedTargets, + warnings, }; }; diff --git a/packages/super-editor/src/core/super-converter/v2/exporter/commentsExporter.test.js b/packages/super-editor/src/core/super-converter/v2/exporter/commentsExporter.test.js index 272e9c868f..eeb1a90575 100644 --- a/packages/super-editor/src/core/super-converter/v2/exporter/commentsExporter.test.js +++ b/packages/super-editor/src/core/super-converter/v2/exporter/commentsExporter.test.js @@ -2,75 +2,419 @@ import { updateCommentsExtendedXml, updateCommentsIdsAndExtensible, updateCommentsXml, + prepareCommentsXmlFilesForExport, + removeCommentsFilesFromConvertedXml, toIsoNoFractional, } from './commentsExporter.js'; -describe('updateCommentsIdsAndExtensible', () => { - const comments = [ +// --- Shared fixtures --- + +const makeComment = (overrides = {}) => ({ + commentId: 'test-comment-1', + creatorName: 'Mary Jones', + createdTime: 1764111660000, + importedAuthor: { name: 'Mary Jones (imported)' }, + isInternal: false, + commentText: 'Here is a comment', + commentParaId: '126B0C7F', + ...overrides, +}); + +const makeCommentsIds = () => ({ + declaration: {}, + elements: [ { - commentId: '4cfaa5f7-252f-4e4a-be19-14dc6157e84d', - creatorName: 'Mary Jones', - createdTime: 1764111660000, - importedAuthor: { - name: 'Mary Jones (imported)', - }, - isInternal: false, - commentText: 'Here is a comment', - commentParaId: '126B0C7F', + type: 'element', + name: 'w16cid:commentsIds', + attributes: {}, + elements: [], }, - ]; + ], +}); - const commentsIds = { - declaration: {}, // Omitting for readability - elements: [ - { - type: 'element', - name: 'w16cid:commentsIds', - attributes: {}, // Omitting for readability - elements: [], - }, - ], - }; +const makeExtensible = () => ({ + declaration: {}, + elements: [ + { + type: 'element', + name: 'w16cex:commentsExtensible', + attributes: {}, + elements: [], + }, + ], +}); - const extensible = { - declaration: {}, // Omitting for readability +/** + * Build a minimal convertedXml structure for testing prepareCommentsXmlFilesForExport. + * The function generates fresh skeletons internally, so we only need document.xml + * and the comment-related entries that match the fileSet profile. + */ +const makeConvertedXml = () => ({ + 'word/document.xml': { elements: [{ elements: [] }] }, + 'word/_rels/document.xml.rels': { elements: [ { - type: 'element', - name: 'w16cex:commentsExtensible', - attributes: {}, // Omitting for readability + name: 'Relationships', + attributes: { xmlns: 'http://schemas.openxmlformats.org/package/2006/relationships' }, elements: [], }, ], - }; + }, +}); + +/** Minimal comment def that updateCommentsXml expects (post-getCommentDefinition) */ +const makeCommentDef = (id = '0', paraId = '126B0C7F') => ({ + type: 'element', + name: 'w:comment', + attributes: { + 'w:id': id, + 'w:author': 'Author', + 'w:date': '2025-01-01T00:00:00Z', + 'w:initials': 'A', + 'w15:paraId': paraId, + }, + elements: [{ type: 'element', name: 'w:p', attributes: {}, elements: [] }], +}); - it('should update the comments ids and extensible when created time is provided', () => { +// ============================================================================= +// updateCommentsIdsAndExtensible +// ============================================================================= + +describe('updateCommentsIdsAndExtensible', () => { + const comments = [makeComment()]; + const commentsIds = makeCommentsIds(); + const extensible = makeExtensible(); + + it('populates both parts when both are provided', () => { const result = updateCommentsIdsAndExtensible(comments, commentsIds, extensible); - const elements = result.extensibleUpdated.elements[0].elements; - expect(elements.length).toEqual(1); - expect(elements[0].type).toEqual('element'); - expect(elements[0].name).toEqual('w16cex:commentExtensible'); - expect(elements[0].attributes['w16cex:durableId']).toEqual(expect.any(String)); - expect(elements[0].attributes['w16cex:dateUtc']).toEqual(toIsoNoFractional(comments[0].createdTime)); + expect(result.documentIdsUpdated.elements[0].elements).toHaveLength(1); + expect(result.extensibleUpdated.elements[0].elements).toHaveLength(1); + + // Durable IDs must match between the two parts + const idsId = result.documentIdsUpdated.elements[0].elements[0].attributes['w16cid:durableId']; + const extId = result.extensibleUpdated.elements[0].elements[0].attributes['w16cex:durableId']; + expect(idsId).toBe(extId); + }); + + it('populates only commentsIds when extensible is null', () => { + const result = updateCommentsIdsAndExtensible(comments, commentsIds, null); + expect(result.documentIdsUpdated.elements[0].elements).toHaveLength(1); + expect(result.extensibleUpdated).toBeNull(); }); - it('should update the comments ids and extensible when created time is not provided', () => { - const commentsWithoutCreatedTime = comments.map((comment) => { - return { - ...comment, - createdTime: undefined, + it('populates only extensible when commentsIds is null', () => { + const result = updateCommentsIdsAndExtensible(comments, null, extensible); + expect(result.documentIdsUpdated).toBeNull(); + expect(result.extensibleUpdated.elements[0].elements).toHaveLength(1); + }); + + it('returns both null when both inputs are null', () => { + const result = updateCommentsIdsAndExtensible(comments, null, null); + expect(result.documentIdsUpdated).toBeNull(); + expect(result.extensibleUpdated).toBeNull(); + }); + + it('formats dateUtc correctly when createdTime is provided', () => { + const result = updateCommentsIdsAndExtensible(comments, commentsIds, extensible); + const el = result.extensibleUpdated.elements[0].elements[0]; + expect(el.attributes['w16cex:dateUtc']).toEqual(toIsoNoFractional(comments[0].createdTime)); + }); + + it('formats dateUtc with current time when createdTime is undefined', () => { + const before = Date.now(); + const commentsNoTime = comments.map((c) => ({ ...c, createdTime: undefined })); + const result = updateCommentsIdsAndExtensible(commentsNoTime, commentsIds, extensible); + const after = Date.now(); + const el = result.extensibleUpdated.elements[0].elements[0]; + const actual = el.attributes['w16cex:dateUtc']; + // Allow either second in case of boundary crossing + const valid = [toIsoNoFractional(before), toIsoNoFractional(after)]; + expect(valid).toContain(actual); + }); +}); + +// ============================================================================= +// prepareCommentsXmlFilesForExport +// ============================================================================= + +describe('prepareCommentsXmlFilesForExport', () => { + const commentsWithParaIds = [makeComment()]; + const defs = [makeCommentDef()]; + + describe('partial file-set handling', () => { + it('populates commentsIds.xml even when commentsExtensible.xml is absent', () => { + const threadingProfile = { + defaultStyle: 'commentsExtended', + mixed: false, + fileSet: { + hasCommentsExtended: true, + hasCommentsExtensible: false, + hasCommentsIds: true, + }, }; + + const result = prepareCommentsXmlFilesForExport({ + convertedXml: makeConvertedXml(), + defs, + commentsWithParaIds, + exportType: 'external', + threadingProfile, + }); + + // commentsIds.xml should be populated + const idsXml = result.documentXml['word/commentsIds.xml']; + expect(idsXml).toBeDefined(); + expect(idsXml.elements[0].elements).toHaveLength(1); + expect(idsXml.elements[0].elements[0].name).toBe('w16cid:commentId'); + + // commentsExtensible.xml should NOT exist + expect(result.documentXml['word/commentsExtensible.xml']).toBeUndefined(); + + // Relationship for commentsIds should be present + const idsRel = result.relationships.find((r) => r.attributes.Target === 'commentsIds.xml'); + expect(idsRel).toBeDefined(); + + // Relationship for commentsExtensible should NOT be present + const extRel = result.relationships.find((r) => r.attributes.Target === 'commentsExtensible.xml'); + expect(extRel).toBeUndefined(); + }); + + it('populates commentsExtensible.xml even when commentsIds.xml is absent', () => { + const threadingProfile = { + defaultStyle: 'commentsExtended', + mixed: false, + fileSet: { + hasCommentsExtended: true, + hasCommentsExtensible: true, + hasCommentsIds: false, + }, + }; + + const result = prepareCommentsXmlFilesForExport({ + convertedXml: makeConvertedXml(), + defs, + commentsWithParaIds, + exportType: 'external', + threadingProfile, + }); + + // commentsExtensible.xml should be populated + const extXml = result.documentXml['word/commentsExtensible.xml']; + expect(extXml).toBeDefined(); + expect(extXml.elements[0].elements).toHaveLength(1); + + // commentsIds.xml should NOT exist + expect(result.documentXml['word/commentsIds.xml']).toBeUndefined(); }); - const result = updateCommentsIdsAndExtensible(commentsWithoutCreatedTime, commentsIds, extensible); - const elements = result.extensibleUpdated.elements[0].elements; - expect(elements.length).toEqual(1); - expect(elements[0].type).toEqual('element'); - expect(elements[0].name).toEqual('w16cex:commentExtensible'); - expect(elements[0].attributes['w16cex:durableId']).toEqual(expect.any(String)); - expect(elements[0].attributes['w16cex:dateUtc']).toEqual(toIsoNoFractional(Date.now())); + }); + + describe('removedTargets tracking', () => { + it('returns removedTargets for parts not emitted', () => { + const threadingProfile = { + defaultStyle: 'commentsExtended', + mixed: false, + fileSet: { + hasCommentsExtended: true, + hasCommentsExtensible: false, + hasCommentsIds: true, + }, + }; + + const result = prepareCommentsXmlFilesForExport({ + convertedXml: makeConvertedXml(), + defs, + commentsWithParaIds, + exportType: 'external', + threadingProfile, + }); + + expect(result.removedTargets).toContain('commentsExtensible.xml'); + expect(result.removedTargets).not.toContain('comments.xml'); + expect(result.removedTargets).not.toContain('commentsIds.xml'); + expect(result.removedTargets).not.toContain('commentsExtended.xml'); + }); + + it('returns all targets as removed for clean export', () => { + const result = prepareCommentsXmlFilesForExport({ + convertedXml: makeConvertedXml(), + defs, + commentsWithParaIds, + exportType: 'clean', + threadingProfile: null, + }); + + expect(result.removedTargets).toHaveLength(4); + expect(result.removedTargets).toContain('comments.xml'); + expect(result.removedTargets).toContain('commentsExtended.xml'); + expect(result.removedTargets).toContain('commentsIds.xml'); + expect(result.removedTargets).toContain('commentsExtensible.xml'); + }); + }); + + describe('zero-comments cleanup', () => { + it('removes all comment files when there are no comments', () => { + const threadingProfile = { + defaultStyle: 'commentsExtended', + mixed: false, + fileSet: { + hasCommentsExtended: true, + hasCommentsExtensible: true, + hasCommentsIds: true, + }, + }; + + const result = prepareCommentsXmlFilesForExport({ + convertedXml: makeConvertedXml(), + defs: [], + commentsWithParaIds: [], + exportType: 'external', + threadingProfile, + }); + + expect(result.documentXml['word/comments.xml']).toBeUndefined(); + expect(result.documentXml['word/commentsExtended.xml']).toBeUndefined(); + expect(result.documentXml['word/commentsIds.xml']).toBeUndefined(); + expect(result.documentXml['word/commentsExtensible.xml']).toBeUndefined(); + expect(result.removedTargets).toHaveLength(4); + expect(result.relationships).toHaveLength(0); + }); + }); + + describe('warnings', () => { + it('warns about partial file-set', () => { + const threadingProfile = { + defaultStyle: 'commentsExtended', + mixed: false, + fileSet: { + hasCommentsExtended: true, + hasCommentsExtensible: false, + hasCommentsIds: true, + }, + }; + + const result = prepareCommentsXmlFilesForExport({ + convertedXml: makeConvertedXml(), + defs, + commentsWithParaIds, + exportType: 'external', + threadingProfile, + }); + + expect(result.warnings.some((w) => w.includes('Partial comment file-set'))).toBe(true); + }); + + it('does not warn on clean export', () => { + const result = prepareCommentsXmlFilesForExport({ + convertedXml: makeConvertedXml(), + defs, + commentsWithParaIds, + exportType: 'clean', + threadingProfile: null, + }); + + expect(result.warnings).toHaveLength(0); + }); + + it('warns when all comments are removed and profile had files', () => { + const threadingProfile = { + defaultStyle: 'commentsExtended', + mixed: false, + fileSet: { + hasCommentsExtended: true, + hasCommentsExtensible: true, + hasCommentsIds: true, + }, + }; + + const result = prepareCommentsXmlFilesForExport({ + convertedXml: makeConvertedXml(), + defs: [], + commentsWithParaIds: [], + exportType: 'external', + threadingProfile, + }); + + expect(result.warnings.some((w) => w.includes('All comments removed'))).toBe(true); + }); + + it('warns when comments exist but no threading profile', () => { + const result = prepareCommentsXmlFilesForExport({ + convertedXml: makeConvertedXml(), + defs, + commentsWithParaIds, + exportType: 'external', + threadingProfile: null, + }); + + expect(result.warnings.some((w) => w.includes('no threading profile'))).toBe(true); + }); + }); + + describe('full file-set', () => { + it('populates all four files when all are present', () => { + const threadingProfile = { + defaultStyle: 'commentsExtended', + mixed: false, + fileSet: { + hasCommentsExtended: true, + hasCommentsExtensible: true, + hasCommentsIds: true, + }, + }; + + const result = prepareCommentsXmlFilesForExport({ + convertedXml: makeConvertedXml(), + defs, + commentsWithParaIds, + exportType: 'external', + threadingProfile, + }); + + expect(result.documentXml['word/comments.xml']).toBeDefined(); + expect(result.documentXml['word/commentsExtended.xml']).toBeDefined(); + expect(result.documentXml['word/commentsIds.xml']).toBeDefined(); + expect(result.documentXml['word/commentsExtensible.xml']).toBeDefined(); + + // All four relationships should be present + expect(result.relationships).toHaveLength(4); + expect(result.removedTargets).toHaveLength(0); + }); + }); +}); + +// ============================================================================= +// removeCommentsFilesFromConvertedXml +// ============================================================================= + +describe('removeCommentsFilesFromConvertedXml', () => { + it('does not mutate the original object', () => { + const original = { + 'word/comments.xml': { elements: [] }, + 'word/commentsExtended.xml': { elements: [] }, + 'word/commentsExtensible.xml': { elements: [] }, + 'word/commentsIds.xml': { elements: [] }, + 'word/document.xml': { elements: [] }, + }; + const result = removeCommentsFilesFromConvertedXml(original); + + // Original still has the keys + expect(original['word/comments.xml']).toBeDefined(); + + // Result does not + expect(result['word/comments.xml']).toBeUndefined(); + expect(result['word/commentsExtended.xml']).toBeUndefined(); + expect(result['word/commentsExtensible.xml']).toBeUndefined(); + expect(result['word/commentsIds.xml']).toBeUndefined(); + + // Non-comment files preserved + expect(result['word/document.xml']).toBeDefined(); }); }); +// ============================================================================= +// updateCommentsExtendedXml (existing tests) +// ============================================================================= + describe('updateCommentsExtendedXml', () => { it('uses threadingParentCommentId for threaded replies when parent is tracked', () => { const comments = [ @@ -150,6 +494,10 @@ describe('updateCommentsExtendedXml', () => { }); }); +// ============================================================================= +// updateCommentsXml (existing tests) +// ============================================================================= + describe('updateCommentsXml', () => { it('stamps w14:paraId on the final paragraph for multi-paragraph comments', () => { const commentDef = { diff --git a/packages/super-editor/src/extensions/collaboration/collaboration-helpers.js b/packages/super-editor/src/extensions/collaboration/collaboration-helpers.js index fa79d65974..e145f9eb41 100644 --- a/packages/super-editor/src/extensions/collaboration/collaboration-helpers.js +++ b/packages/super-editor/src/extensions/collaboration/collaboration-helpers.js @@ -34,9 +34,10 @@ export const updateYdocDocxData = async (editor, ydoc) => { Object.keys(newXml).forEach((key) => { const fileIndex = docx.findIndex((item) => item.name === key); const existingContent = fileIndex > -1 ? docx[fileIndex].content : null; + const newContent = newXml[key]; // Skip if content hasn't changed - if (existingContent === newXml[key]) { + if (existingContent === newContent) { return; } @@ -44,10 +45,16 @@ export const updateYdocDocxData = async (editor, ydoc) => { if (fileIndex > -1) { docx.splice(fileIndex, 1); } - docx.push({ - name: key, - content: newXml[key], - }); + + // A null value means the file was deleted during export (e.g. comment + // parts removed). Only add entries with real content — pushing + // { content: null } would crash parseXmlToJson on next hydration. + if (newContent != null) { + docx.push({ + name: key, + content: newContent, + }); + } }); // Only transact if there were actual changes OR this is initial setup. diff --git a/packages/super-editor/src/extensions/collaboration/collaboration.test.js b/packages/super-editor/src/extensions/collaboration/collaboration.test.js index ece6035d24..5cd6c3ef14 100644 --- a/packages/super-editor/src/extensions/collaboration/collaboration.test.js +++ b/packages/super-editor/src/extensions/collaboration/collaboration.test.js @@ -256,6 +256,32 @@ describe('collaboration helpers', () => { ); }); + it('does not persist null comment xml payloads into meta.docx', async () => { + const existingDocx = [ + { name: 'word/document.xml', content: '' }, + { name: 'word/comments.xml', content: '' }, + { name: 'word/commentsExtended.xml', content: '' }, + ]; + const ydoc = createYDocStub({ docxValue: existingDocx }); + const metas = ydoc._maps.metas; + + const editor = { + options: { ydoc, user: { id: 'user-null-comments' } }, + exportDocx: vi.fn().mockResolvedValue({ + 'word/document.xml': '', + 'word/comments.xml': null, + 'word/commentsExtended.xml': null, + }), + }; + + await updateYdocDocxData(editor); + + const persistedDocx = metas.set.mock.calls.at(-1)?.[1] || []; + expect(persistedDocx.some((file) => file.name === 'word/comments.xml')).toBe(false); + expect(persistedDocx.some((file) => file.name === 'word/commentsExtended.xml')).toBe(false); + expect(persistedDocx.every((file) => typeof file.content === 'string')).toBe(true); + }); + it('triggers transaction when new file is added', async () => { const existingDocx = [{ name: 'word/document.xml', content: '' }]; const ydoc = createYDocStub({ docxValue: existingDocx }); diff --git a/packages/super-editor/src/tests/data/gdocs-single-comment.docx b/packages/super-editor/src/tests/data/gdocs-single-comment.docx new file mode 100644 index 0000000000..a00cde2792 Binary files /dev/null and b/packages/super-editor/src/tests/data/gdocs-single-comment.docx differ diff --git a/packages/super-editor/src/tests/data/nested-comments-gdocs.docx b/packages/super-editor/src/tests/data/nested-comments-gdocs.docx new file mode 100644 index 0000000000..93613138cd Binary files /dev/null and b/packages/super-editor/src/tests/data/nested-comments-gdocs.docx differ diff --git a/packages/super-editor/src/tests/export/commentThreadingProfile.test.js b/packages/super-editor/src/tests/export/commentThreadingProfile.test.js new file mode 100644 index 0000000000..d1886b1fc7 --- /dev/null +++ b/packages/super-editor/src/tests/export/commentThreadingProfile.test.js @@ -0,0 +1,283 @@ +import { beforeAll, describe, expect, it } from 'vitest'; +import { initTestEditor, loadTestDataForEditorTests } from '@tests/helpers/helpers.js'; +import DocxZipper from '@core/DocxZipper.js'; + +/** + * IT-554 – Comment threading profile mismatch can silently lose comments. + * + * These tests exercise the full export pipeline (Editor.exportDocx) and + * assert on the final updatedDocs map (what goes into the zip) as well as + * the actual zipped output. Three scenarios: + * + * 1. Partial profile (nested-comments.docx – has commentsIds but NOT + * commentsExtensible). The old &&-guard dropped commentsIds entirely. + * + * 2. Google-Docs profile without threading (gdocs-single-comment.docx – + * comments.xml only, single non-threaded comment). No auxiliary files + * should be fabricated. + * + * 3. Clean export – zero comment files regardless of input. + */ + +const COMMENT_FILES = [ + 'word/comments.xml', + 'word/commentsExtended.xml', + 'word/commentsIds.xml', + 'word/commentsExtensible.xml', +]; + +/** Helper: prepare comments array the same way SuperDoc.exportEditorsToDOCX does */ +const prepareCommentsForExport = (converter) => + (converter.comments ?? []).map((comment) => { + const elements = Array.isArray(comment.elements) && comment.elements.length ? comment.elements : undefined; + return { + ...comment, + commentJSON: comment.commentJSON ?? elements, + }; + }); + +// --------------------------------------------------------------------------- +// Scenario 1 – Partial profile (3 of 4 files) +// nested-comments.docx has: comments.xml, commentsExtended.xml, commentsIds.xml +// It does NOT have commentsExtensible.xml. +// --------------------------------------------------------------------------- +describe('Partial threading profile (nested-comments.docx)', () => { + let docx, media, mediaFiles, fonts; + + beforeAll(async () => { + ({ docx, media, mediaFiles, fonts } = await loadTestDataForEditorTests('nested-comments.docx')); + }); + + it('preserves commentsIds.xml and omits commentsExtensible.xml in updatedDocs', async () => { + const { editor } = initTestEditor({ content: docx, media, mediaFiles, fonts }); + + try { + const comments = prepareCommentsForExport(editor.converter); + expect(comments.length).toBeGreaterThan(0); + + const updatedDocs = await editor.exportDocx({ + comments, + commentsType: 'external', + getUpdatedDocs: true, + }); + + // comments.xml must be present (string) + expect(updatedDocs['word/comments.xml']).toEqual(expect.any(String)); + + // commentsExtended.xml must be present (string) + expect(updatedDocs['word/commentsExtended.xml']).toEqual(expect.any(String)); + + // commentsIds.xml must be present — this is the key Fix 1 assertion. + // Before the fix, the &&-guard dropped it because commentsExtensible was absent. + expect(updatedDocs['word/commentsIds.xml']).toEqual(expect.any(String)); + + // commentsExtensible.xml must be null (was never in the original) + expect(updatedDocs['word/commentsExtensible.xml']).toBeNull(); + } finally { + editor.destroy(); + } + }); + + it('produces a zip without commentsExtensible.xml', async () => { + const { editor } = initTestEditor({ content: docx, media, mediaFiles, fonts }); + + try { + const comments = prepareCommentsForExport(editor.converter); + const blob = await editor.exportDocx({ + comments, + commentsType: 'external', + }); + + const zipper = new DocxZipper(); + const zip = await zipper.unzip(blob); + + expect(zip.file('word/comments.xml')).not.toBeNull(); + expect(zip.file('word/commentsExtended.xml')).not.toBeNull(); + expect(zip.file('word/commentsIds.xml')).not.toBeNull(); + expect(zip.file('word/commentsExtensible.xml')).toBeNull(); + + // Content types must reference the three present files but NOT commentsExtensible + const contentTypes = await zip.file('[Content_Types].xml').async('string'); + expect(contentTypes).toContain('/word/comments.xml'); + expect(contentTypes).toContain('/word/commentsExtended.xml'); + expect(contentTypes).toContain('/word/commentsIds.xml'); + expect(contentTypes).not.toContain('/word/commentsExtensible.xml'); + } finally { + editor.destroy(); + } + }); +}); + +// --------------------------------------------------------------------------- +// Scenario 2 – Google Docs profile, no threading (comments.xml only) +// gdocs-single-comment.docx has: comments.xml with 1 non-threaded comment. +// No commentsExtended / commentsIds / commentsExtensible. +// Since there are no threaded comments, the exporter should NOT fabricate +// auxiliary files — the range-based threading model is preserved. +// --------------------------------------------------------------------------- +describe('Google Docs profile without threading (gdocs-single-comment.docx)', () => { + let docx, media, mediaFiles, fonts; + + beforeAll(async () => { + ({ docx, media, mediaFiles, fonts } = await loadTestDataForEditorTests('gdocs-single-comment.docx')); + }); + + it('emits only comments.xml — no auxiliary files fabricated', async () => { + const { editor } = initTestEditor({ content: docx, media, mediaFiles, fonts }); + + try { + const comments = prepareCommentsForExport(editor.converter); + expect(comments.length).toBeGreaterThan(0); + + const updatedDocs = await editor.exportDocx({ + comments, + commentsType: 'external', + getUpdatedDocs: true, + }); + + // comments.xml must be present + expect(updatedDocs['word/comments.xml']).toEqual(expect.any(String)); + + // The three auxiliary files must all be null (removed / never existed) + expect(updatedDocs['word/commentsExtended.xml']).toBeNull(); + expect(updatedDocs['word/commentsIds.xml']).toBeNull(); + expect(updatedDocs['word/commentsExtensible.xml']).toBeNull(); + } finally { + editor.destroy(); + } + }); + + it('produces a zip with only comments.xml', async () => { + const { editor } = initTestEditor({ content: docx, media, mediaFiles, fonts }); + + try { + const comments = prepareCommentsForExport(editor.converter); + const blob = await editor.exportDocx({ + comments, + commentsType: 'external', + }); + + const zipper = new DocxZipper(); + const zip = await zipper.unzip(blob); + + expect(zip.file('word/comments.xml')).not.toBeNull(); + expect(zip.file('word/commentsExtended.xml')).toBeNull(); + expect(zip.file('word/commentsIds.xml')).toBeNull(); + expect(zip.file('word/commentsExtensible.xml')).toBeNull(); + + const contentTypes = await zip.file('[Content_Types].xml').async('string'); + expect(contentTypes).toContain('/word/comments.xml'); + expect(contentTypes).not.toContain('/word/commentsExtended.xml'); + expect(contentTypes).not.toContain('/word/commentsIds.xml'); + expect(contentTypes).not.toContain('/word/commentsExtensible.xml'); + } finally { + editor.destroy(); + } + }); +}); + +// --------------------------------------------------------------------------- +// Scenario 2b – Google Docs profile WITH threading +// nested-comments-gdocs.docx has: comments.xml only, but comments include +// threaded replies. The exporter correctly fabricates commentsExtended.xml +// so Word can display the thread — this is intentional. +// --------------------------------------------------------------------------- +describe('Google Docs profile with threading (nested-comments-gdocs.docx)', () => { + let docx, media, mediaFiles, fonts; + + beforeAll(async () => { + ({ docx, media, mediaFiles, fonts } = await loadTestDataForEditorTests('nested-comments-gdocs.docx')); + }); + + it('fabricates commentsExtended.xml for threaded comments', async () => { + const { editor } = initTestEditor({ content: docx, media, mediaFiles, fonts }); + + try { + const comments = prepareCommentsForExport(editor.converter); + expect(comments.length).toBeGreaterThan(1); + + const updatedDocs = await editor.exportDocx({ + comments, + commentsType: 'external', + getUpdatedDocs: true, + }); + + // comments.xml must be present + expect(updatedDocs['word/comments.xml']).toEqual(expect.any(String)); + + // commentsExtended.xml IS fabricated because threaded replies exist — + // Word needs it to display the threading correctly. + expect(updatedDocs['word/commentsExtended.xml']).toEqual(expect.any(String)); + + // commentsIds and commentsExtensible remain null (not in original, not needed) + expect(updatedDocs['word/commentsIds.xml']).toBeNull(); + expect(updatedDocs['word/commentsExtensible.xml']).toBeNull(); + } finally { + editor.destroy(); + } + }); +}); + +// --------------------------------------------------------------------------- +// Scenario 3 – Clean export (zero comment files) +// Uses nested-comments.docx (3-file profile) to prove all parts are removed. +// --------------------------------------------------------------------------- +describe('Clean export strips all comment files', () => { + let docx, media, mediaFiles, fonts; + + beforeAll(async () => { + ({ docx, media, mediaFiles, fonts } = await loadTestDataForEditorTests('nested-comments.docx')); + }); + + it('sets all four comment files to null in updatedDocs', async () => { + const { editor } = initTestEditor({ content: docx, media, mediaFiles, fonts }); + + try { + const updatedDocs = await editor.exportDocx({ + commentsType: 'clean', + getUpdatedDocs: true, + }); + + for (const file of COMMENT_FILES) { + expect(updatedDocs[file]).toBeNull(); + } + + // Content types must not reference any comment file + const contentTypes = updatedDocs['[Content_Types].xml']; + expect(contentTypes).toBeDefined(); + for (const file of COMMENT_FILES) { + expect(contentTypes).not.toContain(`/${file}`); + } + } finally { + editor.destroy(); + } + }); + + it('produces a zip with zero comment files', async () => { + const { editor } = initTestEditor({ content: docx, media, mediaFiles, fonts }); + + try { + const blob = await editor.exportDocx({ commentsType: 'clean' }); + const zipper = new DocxZipper(); + const zip = await zipper.unzip(blob); + + for (const file of COMMENT_FILES) { + expect(zip.file(file)).toBeNull(); + } + + const contentTypes = await zip.file('[Content_Types].xml').async('string'); + for (const file of COMMENT_FILES) { + expect(contentTypes).not.toContain(`/${file}`); + } + + // Relationships should not reference any comment files + const rels = await zip.file('word/_rels/document.xml.rels').async('string'); + expect(rels).not.toContain('comments.xml'); + expect(rels).not.toContain('commentsExtended.xml'); + expect(rels).not.toContain('commentsIds.xml'); + expect(rels).not.toContain('commentsExtensible.xml'); + } finally { + editor.destroy(); + } + }); +});