fix(converter): preserve numbering.xml definitions on round-trip (SD-2911)#3231
Conversation
…2911) #exportNumberingFile filtered out every w:num whose numId wasn't referenced from the exported document parts (including headers, footers, and footnotes). Word's tentative numbering, where a document carries definitions for lists the user hasn't applied yet, was therefore silently wiped: the active-numbering fixture lost 7 of 8 definitions and the tentative fixture lost them all. The filter was introduced for the lists.delete document-api operation, but it couldn't distinguish "user just deleted a list in this session" from "definition was always unused in the source file" — both arrived at the export with no referencing paragraph and both were dropped. Word tolerates unused definitions, so the safe default on export is to emit every abstractNum and num the importer captured. The strip-orphaned helper is removed entirely; the inline write in #exportNumberingFile is now four lines. Verified: both fixtures round-trip byte-identical (after pretty-print) at the numbering.xml level. New integration test covers both the active and tentative variants.
superdoc-bot
Bot
added
review: thorough
review: careful
and removed
review: thorough
labels
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
luccas-harbour
left a comment
luccas-harbour
left a comment
There was a problem hiding this comment.
hey! can you check this one?
- [P2] Avoid exporting fallback numbering for docs without numbering — packages/super-editor/src/editors/v1/core/super-converter/SuperConverter.js:1440-1442
When the input package has no `word/numbering.xml` (or an empty one), the importer still fills `this.numbering` from `baseNumbering` via `ensureElementsArray`; writing all `Object.values` here therefore exports SuperDoc's fallback numbering definitions even though the source document had none. Main previously emitted no definitions because no paragraphs referenced those fallback numIds, so plain documents now gain unused numbering definitions on round-trip. Track whether the numbering part came from the source, or filter only the fallback, before preserving everything.
…no refs (SD-2911) The previous SD-2911 fix wrote `this.numbering`'s contents on every export. The importer falls back to `baseNumbering` when the source had no `word/numbering.xml`, so plain documents gained unused fallback definitions on round-trip — the regression Luccas flagged. Capture whether the source package shipped the numbering part before the baseNumbering fallback runs, and short-circuit the write when both: source had none AND no `w:numPr` reference exists in the exported body. Sourced numbering (including tentative variants) still round-trips fully, and a list created during the session still ships because the body now carries `w:numPr`. Editor.ts: the numbering part is now optional in the export bundle — same shape as appXml / coreXml. TDD - 3 new failing integration tests on blank-doc.docx / Hello docx world.docx (no source numbering, no body refs). - All 3 green after the fix. - The 3 existing SD-2911 tests (active + tentative variants) still pass. Verification: `pnpm --filter super-editor test --run`: 12 698 / 12 698 pass.
|
@luccas-harbour pushed `9e9e8b318`. The regression you flagged is fixed: plain documents no longer gain a `word/numbering.xml` on round-trip. Fix shape`SuperConverter.js::#exportNumberingFile` now captures source presence before `baseNumbering` is substituted, and returns early when both:
If the source had numbering (active or tentative), every definition still round-trips (SD-2911's original intent is preserved). If a user creates a list in the session, the body carries `w:numPr` and the numbering part is written normally. `Editor.ts` was the only other place that assumed numbering.xml was always set on `convertedXml`; treating it as optional now mirrors the appXml / coreXml handling already in that block. TDD3 new integration tests against `blank-doc.docx` and `Hello docx world.docx` (both have no source numbering): `exportedNumberingEntry` was being emitted before the fix and is now absent. The 3 existing tests for the active + tentative SD-2911 fixtures still pass. Verification
|
|
🎉 This PR is included in @superdoc-dev/mcp v0.3.0-next.99 The release is available on GitHub release |
|
🎉 This PR is included in vscode-ext v2.3.0-next.145 |
|
🎉 This PR is included in @superdoc-dev/react v1.2.0-next.143 The release is available on GitHub release |
|
🎉 This PR is included in superdoc-cli v0.8.0-next.114 The release is available on GitHub release |
|
🎉 This PR is included in superdoc v1.30.0-next.95 The release is available on GitHub release |
4 participants
Summary
#exportNumberingFileno longer filtersnumbering.xmlby whichnumIds are referenced from the document body. Every<w:abstractNum>and<w:num>the importer captured is emitted on export.strip-orphaned-numbering.js+ its unit tests; the inline write is now four lines.Why
Customer fixture in SD-2911:
numIdused in the body):<w:abstractNum>dropped from 8 → 1,<w:num>from 8 → 1. Live numbering definitions Word would otherwise expose in the Multilevel List dropdown were silently lost.numIdreferenced):numbering.xmlwas effectively wiped (2 → 0 abstractNum, 1 → 0 num).The filter was added by PR #2223 for the
lists.deletedocument-api operation — when a user deletes a list, the dead definition should be cleaned up. But the filter ran on every export and had no way to tell "user just deleted in this session" from "definition was always unused in the source file" (Word's tentative numbering). Both reached it with no referencing paragraph and were treated identically. Word tolerates unused definitions, so the safe default on round-trip is to preserve everything.Test plan
sd-2911-numbering-roundtrip.test.js) round-trips both customer fixtures and asserts<w:abstractNum>/<w:num>counts and ids match the input exactly.pnpm testforsuper-editor(12 695 / 12 708 pass — the 13 deletions are the removedstrip-orphaned-numbering.test.jsunit tests).numbering.xmlbyte-identical to input after pretty-print (8 / 8 preserved).numbering.xmlbyte-identical (2 / 1 preserved).Verified diffs on the customer fixtures
<w:abstractNum><w:num><w:abstractNum><w:num>What about the lists.delete flow that originally motivated the filter?
If
lists.deleteneeds to remove the definition for the list it deletes, that should happen at the operation site — where the deletion intent is known — not as an opportunistic GC pass at every export. There's no existing test in the repo that gates this behavior end-to-end against the document-api; adding one is out of scope for this fix.