diff --git a/packages/super-editor/src/editors/v1/core/Editor.ts b/packages/super-editor/src/editors/v1/core/Editor.ts index 60777862ae..df72153a30 100644 --- a/packages/super-editor/src/editors/v1/core/Editor.ts +++ b/packages/super-editor/src/editors/v1/core/Editor.ts @@ -3248,7 +3248,7 @@ export class Editor extends EventEmitter { }); const numberingData = this.converter.convertedXml['word/numbering.xml']; - const numbering = this.converter.schemaToXml(numberingData.elements[0]); + const numbering = numberingData?.elements?.[0] ? this.converter.schemaToXml(numberingData.elements[0]) : null; const appXmlData = this.converter.convertedXml['docProps/app.xml']; const appXml = appXmlData?.elements?.[0] ? this.converter.schemaToXml(appXmlData.elements[0]) : null; @@ -3262,7 +3262,7 @@ export class Editor extends EventEmitter { 'word/document.xml': String(documentXml), 'docProps/custom.xml': String(customXml), 'word/_rels/document.xml.rels': String(rels), - 'word/numbering.xml': String(numbering), + ...(numbering ? { 'word/numbering.xml': String(numbering) } : {}), 'word/styles.xml': String(styles), ...updatedHeadersFooters, ...(appXml ? { 'docProps/app.xml': String(appXml) } : {}), diff --git a/packages/super-editor/src/editors/v1/core/super-converter/SuperConverter.js b/packages/super-editor/src/editors/v1/core/super-converter/SuperConverter.js index 4e453eb2de..0b3e4f39bc 100644 --- a/packages/super-editor/src/editors/v1/core/super-converter/SuperConverter.js +++ b/packages/super-editor/src/editors/v1/core/super-converter/SuperConverter.js @@ -34,11 +34,6 @@ import { syncBibliographyPartToPackage, getBibliographyPartExportPaths, } from './citation-sources.js'; -import { - collectReferencedNumIds, - filterOrphanedNumberingDefinitions, -} from './export-helpers/strip-orphaned-numbering.js'; - const FONT_FAMILY_FALLBACKS = Object.freeze({ swiss: 'Arial, sans-serif', roman: 'Times New Roman, serif', @@ -1429,23 +1424,27 @@ class SuperConverter { #exportNumberingFile() { const numberingPath = 'word/numbering.xml'; + // SD-2911: source presence must be captured before the baseNumbering fallback — + // the importer fills `this.numbering` from baseNumbering when the source had no + // numbering part, so it can't be inferred from `this.numbering` at write time. + const sourceHadNumberingXml = Boolean(this.convertedXml[numberingPath]); let numberingXml = this.convertedXml[numberingPath]; if (!numberingXml) numberingXml = baseNumbering; const currentNumberingXml = numberingXml.elements[0]; - // D7: Strip orphaned numbering definitions (entries not referenced by any - // paragraph in the exported document parts). - const referencedNumIds = collectReferencedNumIds(this.convertedXml); + if (!sourceHadNumberingXml && !hasBodyNumberingReferences(this.convertedXml['word/document.xml'])) { + return; + } if (this.numbering?.definitions && this.numbering?.abstracts) { - const { liveAbstracts, liveDefinitions } = filterOrphanedNumberingDefinitions(this.numbering, referencedNumIds); - currentNumberingXml.elements = [...liveAbstracts, ...liveDefinitions]; + const abstracts = Object.values(this.numbering.abstracts); + const definitions = Object.values(this.numbering.definitions); + currentNumberingXml.elements = [...abstracts, ...definitions]; } else { currentNumberingXml.elements = []; } - // Update the numbering file this.convertedXml[numberingPath] = numberingXml; } @@ -1738,4 +1737,17 @@ function generateCustomXml() { return DEFAULT_CUSTOM_XML; } -export { SuperConverter }; +/** @returns {boolean} True if any descendant of `documentXml` is a `w:numPr` element. */ +function hasBodyNumberingReferences(documentXml) { + if (!documentXml || typeof documentXml !== 'object') return false; + const stack = Array.isArray(documentXml.elements) ? [...documentXml.elements] : []; + while (stack.length) { + const node = stack.pop(); + if (!node || typeof node !== 'object') continue; + if (node.name === 'w:numPr') return true; + if (Array.isArray(node.elements)) stack.push(...node.elements); + } + return false; +} + +export { SuperConverter, hasBodyNumberingReferences }; diff --git a/packages/super-editor/src/editors/v1/core/super-converter/export-helpers/strip-orphaned-numbering.js b/packages/super-editor/src/editors/v1/core/super-converter/export-helpers/strip-orphaned-numbering.js deleted file mode 100644 index c6ae4611f7..0000000000 --- a/packages/super-editor/src/editors/v1/core/super-converter/export-helpers/strip-orphaned-numbering.js +++ /dev/null @@ -1,77 +0,0 @@ -/** - * Collect all w:numId values referenced in exported document parts. - * Walks all word/* XML entries except word/numbering.xml itself. - * - * @param {Record} convertedXml - The full set of exported XML-JSON objects - * @returns {Set} Set of numId values referenced in the document - */ -export function collectReferencedNumIds(convertedXml) { - const numIds = new Set(); - - function walkElements(elements) { - if (!Array.isArray(elements)) return; - for (const el of elements) { - if (!el || typeof el !== 'object') continue; - if (el.name === 'w:numId' && el.attributes?.['w:val'] != null) { - numIds.add(Number(el.attributes['w:val'])); - } - if (el.elements) walkElements(el.elements); - } - } - - for (const [path, xml] of Object.entries(convertedXml)) { - if (path.startsWith('word/') && path !== 'word/numbering.xml' && xml?.elements) { - walkElements(xml.elements); - } - } - - return numIds; -} - -/** - * Extract the w:abstractNumId value from a w:num XML-JSON element. - * - * @param {object} numDef - A w:num XML-JSON element from numbering.definitions - * @returns {number | undefined} The abstractNumId, or undefined if not found - */ -function getAbstractNumIdFromDef(numDef) { - const abstractEl = numDef.elements?.find((el) => el.name === 'w:abstractNumId'); - if (abstractEl?.attributes?.['w:val'] != null) { - return Number(abstractEl.attributes['w:val']); - } - return undefined; -} - -/** - * Filter numbering definitions to remove orphaned entries not referenced by - * any paragraph in the exported document. Returns new arrays (does not mutate). - * - * @param {{ abstracts: Record, definitions: Record }} numbering - * The converter's numbering data (abstracts keyed by abstractNumId, definitions keyed by numId) - * @param {Set} referencedNumIds - * The set of numId values actually referenced in the exported document - * @returns {{ liveAbstracts: any[], liveDefinitions: any[] }} - * Filtered arrays ready to be written to word/numbering.xml - */ -export function filterOrphanedNumberingDefinitions(numbering, referencedNumIds) { - // Keep only w:num entries whose numId is still referenced - const liveDefinitions = Object.values(numbering.definitions).filter((def) => - referencedNumIds.has(Number(def.attributes?.['w:numId'])), - ); - - // Derive the set of abstractNumIds referenced by surviving w:num entries - const referencedAbstractIds = new Set(); - for (const def of liveDefinitions) { - const abstractId = getAbstractNumIdFromDef(def); - if (abstractId != null) { - referencedAbstractIds.add(abstractId); - } - } - - // Keep only w:abstractNum entries still referenced by a surviving w:num - const liveAbstracts = Object.values(numbering.abstracts).filter((abs) => - referencedAbstractIds.has(Number(abs.attributes?.['w:abstractNumId'])), - ); - - return { liveAbstracts, liveDefinitions }; -} diff --git a/packages/super-editor/src/editors/v1/core/super-converter/export-helpers/strip-orphaned-numbering.test.js b/packages/super-editor/src/editors/v1/core/super-converter/export-helpers/strip-orphaned-numbering.test.js deleted file mode 100644 index 2d2499ade5..0000000000 --- a/packages/super-editor/src/editors/v1/core/super-converter/export-helpers/strip-orphaned-numbering.test.js +++ /dev/null @@ -1,245 +0,0 @@ -import { describe, it, expect } from 'vitest'; -import { collectReferencedNumIds, filterOrphanedNumberingDefinitions } from './strip-orphaned-numbering.js'; - -// --------------------------------------------------------------------------- -// Helpers for building XML-JSON structures -// --------------------------------------------------------------------------- - -function makeNumIdElement(numId) { - return { - name: 'w:numId', - type: 'element', - attributes: { 'w:val': String(numId) }, - }; -} - -function makeParagraphWithNumId(numId) { - return { - name: 'w:p', - type: 'element', - elements: [ - { - name: 'w:pPr', - type: 'element', - elements: [ - { - name: 'w:numPr', - type: 'element', - elements: [makeNumIdElement(numId), { name: 'w:ilvl', type: 'element', attributes: { 'w:val': '0' } }], - }, - ], - }, - ], - }; -} - -function makeDocumentXml(paragraphs) { - return { - elements: [ - { - name: 'w:document', - type: 'element', - elements: [{ name: 'w:body', type: 'element', elements: paragraphs }], - }, - ], - }; -} - -function makeNumDef(numId, abstractNumId, extraElements = []) { - return { - name: 'w:num', - type: 'element', - attributes: { 'w:numId': String(numId) }, - elements: [ - { - name: 'w:abstractNumId', - type: 'element', - attributes: { 'w:val': String(abstractNumId) }, - }, - ...extraElements, - ], - }; -} - -function makeAbstractDef(abstractNumId) { - return { - name: 'w:abstractNum', - type: 'element', - attributes: { 'w:abstractNumId': String(abstractNumId) }, - elements: [], - }; -} - -// --------------------------------------------------------------------------- -// collectReferencedNumIds -// --------------------------------------------------------------------------- - -describe('collectReferencedNumIds', () => { - it('collects numIds from document body paragraphs', () => { - const convertedXml = { - 'word/document.xml': makeDocumentXml([makeParagraphWithNumId(1), makeParagraphWithNumId(3)]), - }; - const result = collectReferencedNumIds(convertedXml); - expect(result).toEqual(new Set([1, 3])); - }); - - it('collects numIds from headers and footers', () => { - const convertedXml = { - 'word/document.xml': makeDocumentXml([makeParagraphWithNumId(1)]), - 'word/header1.xml': { - elements: [{ name: 'w:hdr', type: 'element', elements: [makeParagraphWithNumId(5)] }], - }, - 'word/footer1.xml': { - elements: [{ name: 'w:ftr', type: 'element', elements: [makeParagraphWithNumId(7)] }], - }, - }; - const result = collectReferencedNumIds(convertedXml); - expect(result).toEqual(new Set([1, 5, 7])); - }); - - it('ignores word/numbering.xml to avoid self-referencing', () => { - const convertedXml = { - 'word/document.xml': makeDocumentXml([makeParagraphWithNumId(1)]), - 'word/numbering.xml': { - elements: [ - { - name: 'w:numbering', - type: 'element', - elements: [makeNumDef(1, 10), makeNumDef(99, 20)], - }, - ], - }, - }; - const result = collectReferencedNumIds(convertedXml); - // Only numId 1 from document body — numId 99 from numbering.xml should NOT appear - expect(result).toEqual(new Set([1])); - }); - - it('ignores non-word paths', () => { - const convertedXml = { - 'word/document.xml': makeDocumentXml([makeParagraphWithNumId(1)]), - 'docProps/custom.xml': { elements: [makeParagraphWithNumId(999)] }, - }; - const result = collectReferencedNumIds(convertedXml); - expect(result).toEqual(new Set([1])); - }); - - it('returns empty set when no paragraphs have numbering', () => { - const convertedXml = { - 'word/document.xml': makeDocumentXml([{ name: 'w:p', type: 'element', elements: [] }]), - }; - const result = collectReferencedNumIds(convertedXml); - expect(result).toEqual(new Set()); - }); - - it('deduplicates repeated numIds', () => { - const convertedXml = { - 'word/document.xml': makeDocumentXml([ - makeParagraphWithNumId(2), - makeParagraphWithNumId(2), - makeParagraphWithNumId(2), - ]), - }; - const result = collectReferencedNumIds(convertedXml); - expect(result).toEqual(new Set([2])); - }); -}); - -// --------------------------------------------------------------------------- -// filterOrphanedNumberingDefinitions -// --------------------------------------------------------------------------- - -describe('filterOrphanedNumberingDefinitions', () => { - it('keeps definitions referenced by document paragraphs', () => { - const numbering = { - abstracts: { 10: makeAbstractDef(10) }, - definitions: { 1: makeNumDef(1, 10) }, - }; - const referencedNumIds = new Set([1]); - - const { liveAbstracts, liveDefinitions } = filterOrphanedNumberingDefinitions(numbering, referencedNumIds); - - expect(liveDefinitions).toHaveLength(1); - expect(liveDefinitions[0].attributes['w:numId']).toBe('1'); - expect(liveAbstracts).toHaveLength(1); - expect(liveAbstracts[0].attributes['w:abstractNumId']).toBe('10'); - }); - - it('strips orphaned w:num not referenced by any paragraph', () => { - const numbering = { - abstracts: { 10: makeAbstractDef(10), 20: makeAbstractDef(20) }, - definitions: { 1: makeNumDef(1, 10), 99: makeNumDef(99, 20) }, - }; - // Only numId 1 is referenced — numId 99 is orphaned - const referencedNumIds = new Set([1]); - - const { liveAbstracts, liveDefinitions } = filterOrphanedNumberingDefinitions(numbering, referencedNumIds); - - expect(liveDefinitions).toHaveLength(1); - expect(liveDefinitions[0].attributes['w:numId']).toBe('1'); - // abstractNum 20 is also orphaned (only referenced by stripped numId 99) - expect(liveAbstracts).toHaveLength(1); - expect(liveAbstracts[0].attributes['w:abstractNumId']).toBe('10'); - }); - - it('keeps abstract shared by multiple w:num when at least one survives', () => { - const numbering = { - abstracts: { 10: makeAbstractDef(10) }, - definitions: { - 1: makeNumDef(1, 10), - 2: makeNumDef(2, 10), // same abstract as numId 1 - 3: makeNumDef(3, 10), // orphaned — not referenced - }, - }; - const referencedNumIds = new Set([1, 2]); - - const { liveAbstracts, liveDefinitions } = filterOrphanedNumberingDefinitions(numbering, referencedNumIds); - - expect(liveDefinitions).toHaveLength(2); - expect(liveAbstracts).toHaveLength(1); - expect(liveAbstracts[0].attributes['w:abstractNumId']).toBe('10'); - }); - - it('strips all definitions when no numIds are referenced', () => { - const numbering = { - abstracts: { 10: makeAbstractDef(10) }, - definitions: { 1: makeNumDef(1, 10) }, - }; - const referencedNumIds = new Set(); - - const { liveAbstracts, liveDefinitions } = filterOrphanedNumberingDefinitions(numbering, referencedNumIds); - - expect(liveDefinitions).toHaveLength(0); - expect(liveAbstracts).toHaveLength(0); - }); - - it('handles empty numbering gracefully', () => { - const numbering = { abstracts: {}, definitions: {} }; - const referencedNumIds = new Set([1]); - - const { liveAbstracts, liveDefinitions } = filterOrphanedNumberingDefinitions(numbering, referencedNumIds); - - expect(liveDefinitions).toHaveLength(0); - expect(liveAbstracts).toHaveLength(0); - }); - - it('preserves w:num entries with lvlOverride elements', () => { - const lvlOverride = { - name: 'w:lvlOverride', - type: 'element', - attributes: { 'w:ilvl': '0' }, - elements: [{ name: 'w:startOverride', type: 'element', attributes: { 'w:val': '5' } }], - }; - const numbering = { - abstracts: { 10: makeAbstractDef(10) }, - definitions: { 1: makeNumDef(1, 10, [lvlOverride]) }, - }; - const referencedNumIds = new Set([1]); - - const { liveDefinitions } = filterOrphanedNumberingDefinitions(numbering, referencedNumIds); - - expect(liveDefinitions).toHaveLength(1); - // Verify lvlOverride is preserved - expect(liveDefinitions[0].elements).toHaveLength(2); // abstractNumId + lvlOverride - }); -}); diff --git a/packages/super-editor/src/editors/v1/tests/data/sd-2911-active-numbering.docx b/packages/super-editor/src/editors/v1/tests/data/sd-2911-active-numbering.docx new file mode 100644 index 0000000000..cfe610c352 Binary files /dev/null and b/packages/super-editor/src/editors/v1/tests/data/sd-2911-active-numbering.docx differ diff --git a/packages/super-editor/src/editors/v1/tests/data/sd-2911-tentative-numbering.docx b/packages/super-editor/src/editors/v1/tests/data/sd-2911-tentative-numbering.docx new file mode 100644 index 0000000000..7b9b214fb1 Binary files /dev/null and b/packages/super-editor/src/editors/v1/tests/data/sd-2911-tentative-numbering.docx differ diff --git a/packages/super-editor/src/editors/v1/tests/import-export/sd-2911-numbering-roundtrip.test.js b/packages/super-editor/src/editors/v1/tests/import-export/sd-2911-numbering-roundtrip.test.js new file mode 100644 index 0000000000..d1cc8b04dd --- /dev/null +++ b/packages/super-editor/src/editors/v1/tests/import-export/sd-2911-numbering-roundtrip.test.js @@ -0,0 +1,124 @@ +import { describe, it, expect } from 'vitest'; +import { dirname, join } from 'path'; +import { fileURLToPath } from 'node:url'; +import { promises as fs } from 'fs'; +import { Editor } from '@core/Editor.js'; +import DocxZipper from '@core/DocxZipper.js'; +import { parseXmlToJson } from '@converter/v2/docxHelper.js'; +import { initTestEditor } from '../helpers/helpers.js'; + +const __dirname = dirname(fileURLToPath(import.meta.url)); + +const findNumberingRoot = (json) => { + if (!json?.elements?.length) return null; + if (json.elements[0]?.name === 'w:numbering') return json.elements[0]; + return json.elements.find((el) => el?.name === 'w:numbering') || null; +}; + +const countByName = (numberingRoot, elementName) => + (numberingRoot?.elements || []).filter((el) => el?.name === elementName).length; + +const collectIds = (numberingRoot, elementName, attrName) => + (numberingRoot?.elements || []) + .filter((el) => el?.name === elementName) + .map((el) => String(el.attributes?.[attrName])) + .filter(Boolean) + .sort(); + +async function roundTripNumberingCounts(fileName) { + const docxPath = join(__dirname, '../data', fileName); + const docxBuffer = await fs.readFile(docxPath); + + const originalZipper = new DocxZipper(); + const originalEntries = await originalZipper.getDocxData(docxBuffer, true); + const originalNumberingEntry = originalEntries.find((entry) => entry.name === 'word/numbering.xml'); + expect(originalNumberingEntry, 'fixture must contain word/numbering.xml').toBeDefined(); + const originalRoot = findNumberingRoot(parseXmlToJson(originalNumberingEntry.content)); + + const [docx, media, mediaFiles, fonts] = await Editor.loadXmlData(docxBuffer, true); + const { editor } = await initTestEditor({ content: docx, media, mediaFiles, fonts, isHeadless: true }); + + const exportedBuffer = await editor.exportDocx({ isFinalDoc: false }); + const exportedZipper = new DocxZipper(); + const exportedFiles = await exportedZipper.getDocxData(exportedBuffer, true); + const exportedNumberingEntry = exportedFiles.find((entry) => entry.name === 'word/numbering.xml'); + expect(exportedNumberingEntry, 'export must contain word/numbering.xml').toBeDefined(); + const exportedRoot = findNumberingRoot(parseXmlToJson(exportedNumberingEntry.content)); + + return { originalRoot, exportedRoot }; +} + +// SD-2911 P2 (Luccas review). Documents whose source package contained NO +// `word/numbering.xml` must not gain one on round-trip. The importer falls +// back to `baseNumbering` when the part is missing (docxImporter.js:644), so +// `this.numbering` is populated with SuperDoc's fallback definitions even for +// plain documents. The previous SD-2911 fix wrote those out unconditionally, +// silently injecting unused definitions into the exported package. The +// exporter must skip writing `word/numbering.xml` when the source had none +// AND no body paragraph references a numId. + +describe('SD-2911 P2 — plain documents do not gain numbering.xml on round-trip', () => { + async function exportAndCheckNumberingPart(fileName) { + const docxPath = join(__dirname, '../data', fileName); + const docxBuffer = await fs.readFile(docxPath); + + const originalZipper = new DocxZipper(); + const originalEntries = await originalZipper.getDocxData(docxBuffer, true); + const originalHasNumbering = originalEntries.some((entry) => entry.name === 'word/numbering.xml'); + + const [docx, media, mediaFiles, fonts] = await Editor.loadXmlData(docxBuffer, true); + const { editor } = await initTestEditor({ content: docx, media, mediaFiles, fonts, isHeadless: true }); + + const exportedBuffer = await editor.exportDocx({ isFinalDoc: false }); + const exportedZipper = new DocxZipper(); + const exportedFiles = await exportedZipper.getDocxData(exportedBuffer, true); + const exportedNumberingEntry = exportedFiles.find((entry) => entry.name === 'word/numbering.xml'); + + return { originalHasNumbering, exportedHasNumbering: Boolean(exportedNumberingEntry), exportedNumberingEntry }; + } + + it('source fixture has no word/numbering.xml (sanity for the assertion below)', async () => { + const { originalHasNumbering } = await exportAndCheckNumberingPart('blank-doc.docx'); + expect(originalHasNumbering).toBe(false); + }); + + it('does not emit word/numbering.xml on export when the source package had none (blank-doc.docx)', async () => { + const { originalHasNumbering, exportedHasNumbering } = await exportAndCheckNumberingPart('blank-doc.docx'); + expect(originalHasNumbering).toBe(false); + expect(exportedHasNumbering).toBe(false); + }); + + it('does not emit word/numbering.xml on export for a plain text document with no list usage (Hello docx world.docx)', async () => { + const { originalHasNumbering, exportedHasNumbering } = await exportAndCheckNumberingPart('Hello docx world.docx'); + expect(originalHasNumbering).toBe(false); + expect(exportedHasNumbering).toBe(false); + }); +}); + +describe('SD-2911 — numbering.xml definitions preserved on DOCX round-trip', () => { + it('preserves every abstractNum and num for the active-numbering fixture (numId 1 is used)', async () => { + const { originalRoot, exportedRoot } = await roundTripNumberingCounts('sd-2911-active-numbering.docx'); + + expect(countByName(originalRoot, 'w:abstractNum')).toBe(8); + expect(countByName(originalRoot, 'w:num')).toBe(8); + expect(countByName(exportedRoot, 'w:abstractNum')).toBe(countByName(originalRoot, 'w:abstractNum')); + expect(countByName(exportedRoot, 'w:num')).toBe(countByName(originalRoot, 'w:num')); + }); + + it('preserves every abstractNumId and numId verbatim for the active-numbering fixture', async () => { + const { originalRoot, exportedRoot } = await roundTripNumberingCounts('sd-2911-active-numbering.docx'); + expect(collectIds(exportedRoot, 'w:abstractNum', 'w:abstractNumId')).toEqual( + collectIds(originalRoot, 'w:abstractNum', 'w:abstractNumId'), + ); + expect(collectIds(exportedRoot, 'w:num', 'w:numId')).toEqual(collectIds(originalRoot, 'w:num', 'w:numId')); + }); + + it('preserves tentative numbering even when no numId is referenced in the document body', async () => { + const { originalRoot, exportedRoot } = await roundTripNumberingCounts('sd-2911-tentative-numbering.docx'); + + expect(countByName(originalRoot, 'w:abstractNum')).toBe(2); + expect(countByName(originalRoot, 'w:num')).toBe(1); + expect(countByName(exportedRoot, 'w:abstractNum')).toBe(countByName(originalRoot, 'w:abstractNum')); + expect(countByName(exportedRoot, 'w:num')).toBe(countByName(originalRoot, 'w:num')); + }); +});