fix: enhance drawing and inline node handling in translation [SD-824]#1271
fix: enhance drawing and inline node handling in translation [SD-824]#1271
Conversation
- Updated `drawing-translator` to only wrap valid `wp:inline` and `wp:anchor` children in `w:drawing`. - Added tests to ensure correct behavior when child nodes are not drawing elements. - Improved `translateInlineNode` to guard against invalid drawing structures and ensure proper handling of image nodes. - Introduced tests for image and signature placeholder exports, ensuring non-zero IDs are generated when missing.
palmer-cl
changed the title
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
harbournick
force-pushed
the
main
branch
2 times, most recently
from
84f8ea5 to
2a443d8
Compare
caio-pizzol
left a comment
caio-pizzol
left a comment
There was a problem hiding this comment.
@palmer-cl good stuff — these fixes are still needed on main and the approach makes sense.
no blockers. one small edge case on the ID generation, and worth checking if the anchor path needs the same guard as inline.
code is clean — a couple cleanup suggestions inline.
tests cover the main paths well. adding a case for attrs.id = 0 would lock down the exact bug this fixes.
left inline comments.
| const drawingXmlns = 'http://schemas.openxmlformats.org/drawingml/2006/main'; | ||
| const pictureXmlns = 'http://schemas.openxmlformats.org/drawingml/2006/picture'; | ||
|
|
||
| // Ensure valid positive docPr/cNvPr IDs |
There was a problem hiding this comment.
this can still produce id: 0 in rare cases (~1 in 2 billion). easy fix:
| // Ensure valid positive docPr/cNvPr IDs | |
| const docPrId = attrs.id && Number(attrs.id) > 0 ? attrs.id : Math.max(1, parseInt(generateDocxRandomId(), 16)); |
| // Guard: only wrap when we have wp:inline/wp:anchor content. | ||
| const validDrawingChildren = new Set(['wp:inline', 'wp:anchor']); | ||
| if (!resultNode || !validDrawingChildren.has(resultNode.name)) { |
There was a problem hiding this comment.
no need for a Set with just two values — a plain check is clearer and avoids creating an object every call.
| // Guard: only wrap when we have wp:inline/wp:anchor content. | |
| const validDrawingChildren = new Set(['wp:inline', 'wp:anchor']); | |
| if (!resultNode || !validDrawingChildren.has(resultNode.name)) { | |
| if (!resultNode || (resultNode.name !== 'wp:inline' && resultNode.name !== 'wp:anchor')) { | |
| return resultNode; | |
| } |
| if (!nodeElements || nodeElements.name === 'w:r') { | ||
| return nodeElements; | ||
| } | ||
| const hasExtent = | ||
| Array.isArray(nodeElements.elements) && nodeElements.elements.some((el) => el?.name === 'wp:extent'); | ||
| if (!hasExtent) { | ||
| return nodeElements; | ||
| } |
There was a problem hiding this comment.
these two guards do the same thing — both return nodeElements. one check covers all cases:
| if (!nodeElements || nodeElements.name === 'w:r') { | |
| return nodeElements; | |
| } | |
| const hasExtent = | |
| Array.isArray(nodeElements.elements) && nodeElements.elements.some((el) => el?.name === 'wp:extent'); | |
| if (!hasExtent) { | |
| return nodeElements; | |
| } | |
| if (!nodeElements?.elements?.some((el) => el?.name === 'wp:extent')) { | |
| return nodeElements; | |
| } |
| * | ||
| * Guard: ensure we only emit a valid inline drawing structure. | ||
| * wp:inline must contain expected drawing children (like wp:extent). | ||
|
|
There was a problem hiding this comment.
missing * on the empty line breaks the JSDoc block.
| const graphic = inline.elements.find((el) => el.name === 'a:graphic'); | ||
| const graphicData = graphic.elements.find((el) => el.name === 'a:graphicData'); | ||
| const pic = graphicData.elements.find((el) => el.name === 'pic:pic'); | ||
| const nvPicPr = pic.elements.find((el) => el.name === 'pic:nvPicPr'); | ||
| const cNvPr = nvPicPr.elements.find((el) => el.name === 'pic:cNvPr'); |
There was a problem hiding this comment.
this 5-line walk to pic:cNvPr is copy-pasted at lines 57-61. a small findCNvPr(inline) helper at the top would clean it up.
|
one more thing outside this diff: |
|
closing this one in favor of #2363 |
3 participants
drawing-translatorto only wrap validwp:inlineandwp:anchorchildren inw:drawing.translateInlineNodeto guard against invalid drawing structures and ensure proper handling of image nodes.