SD-1525 - comment export with empty array#1790
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 84a978b339
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
caio-pizzol
left a comment
caio-pizzol
left a comment
There was a problem hiding this comment.
@mattConnHarbour looking good just a small comment + tests
…587) (#1806) * fix: remove dead code * refactor: table and paragraph converters to avoid redirection * test: adjust existing tests * fix: remove unused imports and dead code * fix: pass theme colors to node handlers * refactor: simplify type definitions for converters --------- Co-authored-by: Luccas Correa <luccas@superdoc.dev>
mattConnHarbour
force-pushed
the
SD-1525
branch
3 times, most recently
from
fbece02 to
7dfbad0
Compare
…false, with tests
mattConnHarbour
force-pushed
the
SD-1525
branch
from
7dfbad0 to
f28852b
Compare
caio-pizzol
left a comment
caio-pizzol
left a comment
There was a problem hiding this comment.
just 2 minor things - should be good after that
| ); | ||
|
|
||
| // Update content types and comments files as needed | ||
| // Empty array preserves existing comments unless preserveCommentsOnEmpty is true |
There was a problem hiding this comment.
Comment is backwards.
Says "unless preserveCommentsOnEmpty is true" but should be "false".
true = preserve
false = remove
|
|
||
| // Update content types and comments files as needed | ||
| // Empty array preserves existing comments unless preserveCommentsOnEmpty is true | ||
| let updatedXml = { ...this.convertedXml }; |
There was a problem hiding this comment.
let updatedXml = { ...this.convertedXml } is immediately overwritten on line 1031
There was a problem hiding this comment.
@mattConnHarbour the two items from last round are still open — backwards comment + dead init at SuperConverter.js:1023.
bigger issue: the remove-comments path cleans up the in-memory xml but doesn't touch the actual zip. DocxZipper keeps the original files unless you set updatedDocs[key] = null. so preserveCommentsOnEmpty: false doesn't actually remove comments from the exported file. should be a quick fix — DocxZipper already supports null sentinels (line 330).
tests are good for the flag logic but only check internal state, not the zip output. one test that exports + unzips and checks the file list would catch this (see sectionPropertiesExport.test.js for the pattern).
left inline comments.
|
|
||
| if (preparedComments.length) { | ||
| const commentsXml = this.converter.schemaToXml(this.converter.convertedXml['word/comments.xml'].elements[0]); | ||
| // Check if comment files exist in convertedXml (they're removed when cleaning or empty array) |
There was a problem hiding this comment.
skipping comment files in updatedDocs isn't enough — DocxZipper keeps the originals from the source file unless you explicitly set them to null. the old comments survive in the exported docx. fix: set updatedDocs['word/comments.xml'] = null (and the other 3) when they should be gone. DocxZipper already handles null as "remove from zip" (line 330).
| ); | ||
|
|
||
| // Update content types and comments files as needed | ||
| // Empty array preserves existing comments unless preserveCommentsOnEmpty is true |
There was a problem hiding this comment.
still open from last round — comment says "true" but means "false". and let updatedXml = { ...this.convertedXml } is overwritten two lines later, so the initial copy is wasted.
| // Empty array preserves existing comments unless preserveCommentsOnEmpty is true | |
| // Empty array preserves existing comments unless preserveCommentsOnEmpty is false | |
| let updatedXml; |
| updatedXml = { ...documentXml }; | ||
| commentsRels = relationships; | ||
|
|
||
| this.convertedXml = { ...this.convertedXml, ...updatedXml }; |
There was a problem hiding this comment.
the spread here puts the old comment keys back — so the deletion that happened inside prepareCommentsXmlFilesForExport gets undone. only the delete loop below (lines 1041-1048) actually removes them. same logic in two places, one of them doesn't work — worth picking one owner?
The previous implementation cleaned comment keys from in-memory XML but didn't signal DocxZipper to strip them from the actual zip output. The original files from the source document survived in the exported docx. - Add null sentinel support to DocxZipper: null values in updatedDocs now call zip.remove() instead of zip.file() - Editor.ts sets comment file entries to null when comments are removed - Consolidate duplicate removal logic in SuperConverter — the spread merge was undoing deletions from prepareCommentsXmlFilesForExport, requiring a second delete loop. Now assigns documentXml directly. - Fix backwards comment and dead variable init in SuperConverter - Add zip-level test that exports + unzips to verify comment files are actually absent from the output
|
hey @mattConnHarbour - this one's already been fixed on main in #2123. that PR made it so passing an empty comments array always removes comments, which is exactly what SD-1525 needed. |
3 participants
example: