fix: normalize bookmarks in tables #1892
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: 1f6ac31816
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (isBookmarkNode(child)) { | ||
| if (seenRow) { | ||
| trailing.push(child); | ||
| } else { | ||
| leading.push(child); |
There was a problem hiding this comment.
When a bookmark node appears between table rows (for example, a bookmarkStart before row 1 and a bookmarkEnd immediately after row 1), this logic marks every bookmark seen after the first row as trailing. Those trailing bookmarks are later inserted into the last row, which expands the bookmark to cover the rest of the table. This changes bookmark boundaries for DOCX files that place start/end nodes between rows. Consider anchoring each bookmark to the adjacent row/cell or preserving its relative position instead of collapsing all post-first-row bookmarks into the last row.
Useful? React with 👍 / 👎.
caio-pizzol
left a comment
caio-pizzol
left a comment
There was a problem hiding this comment.
looks good overall! just a minor comment
| function insertInlineIntoRow(rowNode, inlineNodes, editor, position) { | ||
| if (!rowNode || !inlineNodes?.length || !Array.isArray(rowNode.content)) return rowNode; | ||
|
|
||
| const targetIndex = position === 'end' ? rowNode.content.length - 1 : 0; |
There was a problem hiding this comment.
if rowNode.content is empty, targetIndex becomes -1 and bookmarks get silently dropped - should we add a guard for rowNode.content.length === 0?
| @@ -1,5 +1,9 @@ | |||
| import { describe, it, expect } from 'vitest'; | |||
| import { collapseWhitespaceNextToInlinePassthrough, filterOutRootInlineNodes } from './docxImporter.js'; | |||
| import { | |||
There was a problem hiding this comment.
should we add a test for empty rows (no cells)? that case would expose the -1 index issue mentioned above
No description provided.