fix(comments): one anchor pair per comment id, regardless of paragraph crossings#3028
Conversation
…h crossings Resolving a comment whose mark spans more than one paragraph emitted N range pairs for an N-paragraph comment, producing a non-conformant DOCX with multiple `commentRangeStart` / `commentRangeEnd` / `commentReference` markers sharing the same `w:id`. Per ECMA-376 §17.13.4.3, §17.13.4.4, §17.13.4.5, `w:id` is the unique identifier for an annotation: each annotation produces exactly one start, one end, and one reference marker. Verified against Word output for a comment that spans a paragraph break: Word emits one pair, with the paragraph close sitting inside the range. `getCommentMarkRangesById` previously merged adjacent segments via `seg.from <= active.to`, which fails across paragraph boundaries because PM positions skip the close+open delta of the structural node. All segments returned by `getCommentMarkSegmentsById` belong to the same logical annotation by id (a single `commentMark` applied across the selection); collapse them into one envelope range covering the full extent. Tests: spec-derived suite for single-paragraph, multi-paragraph, three-paragraph, discontinuous, side-by-side, and overlapping nested cases (resolveCommentById emission), plus the canonical plugin-level regression test for the multi-paragraph case. Verified: 164/164 comment tests pass; 12087/12087 super-editor tests pass; runtime end-to-end against the BYO-UI demo (SuperDoc(11).docx → buggy: id=3 had 2/2/2 markers; SuperDoc(12).docx post-fix: id=3 has 1/1/1 markers).
![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: e404a9f6b3
ℹ️ 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".
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…view feedback) The first iteration of this fix collapsed every segment carrying the same `commentId` into one envelope range. That handles the multi-paragraph case (the SuperDoc(8) bug) but expands the range scope when PM legitimately stores two non-adjacent regions for the same id — most commonly when a user copy-pastes a commented region. The CommentMark has no `transformPasted` hook, so PM preserves the mark attrs verbatim across paste. Both copies share the same `commentId`, but the user's intent is two annotations on two regions, not one annotation covering everything between them. Distinguish the two cases by walking the doc between consecutive segments: `doc.textBetween(prev.to, seg.from, '', '')` returns the concatenated text of every text leaf in the gap, with empty block separators so paragraph close+open contributes nothing. Empty result → structural boundary only → merge (paragraph-crossing). Non-empty result → real uncommented gap → keep ranges separate. Two range pairs with the same id is still non-conformant per spec (`w:id` should be unique per annotation), and remapping to fresh ids on resolve is the proper long-term fix. Filed as a follow-up. For now, preserving per-region scope is strictly better than collapsing it. Adds the disjoint regression test and tightens the assertions so it pins the layout, not just marker counts.
Pins the full pipeline (resolveCommentById → prepareCommentsForExport → comment-range translator → word/document.xml) at the export boundary. Builds a multi-segment TextTarget anchored across two paragraph blocks, posts the comment, resolves it, exports to DOCX, and asserts every comment id appears exactly once for each marker type (commentRangeStart / commentRangeEnd / commentReference). Verified the test catches the regression: against the pre-fix getCommentMarkRangesById it fails with 2/2/2 markers for the multi-paragraph id; against the fix it returns 1/1/1. 3/3 browsers green (chromium / firefox / webkit).
|
🎉 This PR is included in @superdoc-dev/mcp v0.3.0-next.21 The release is available on GitHub release |
|
🎉 This PR is included in vscode-ext v2.3.0-next.66 |
|
🎉 This PR is included in @superdoc-dev/react v1.2.0-next.64 The release is available on GitHub release |
|
🎉 This PR is included in superdoc v1.30.0-next.23 The release is available on GitHub release |
|
🎉 This PR is included in superdoc-cli v0.8.0-next.39 The release is available on GitHub release |
|
🎉 This PR is included in superdoc-cli v0.8.0 The release is available on GitHub release |
|
🎉 This PR is included in superdoc-sdk v1.8.0 |
|
🎉 This PR is included in @superdoc-dev/mcp v0.3.0 The release is available on GitHub release |
|
🎉 This PR is included in superdoc v1.31.0 The release is available on GitHub release |
|
🎉 This PR is included in vscode-ext v2.3.0 |
|
🎉 This PR is included in @superdoc-dev/react v1.3.0 The release is available on GitHub release |
2 participants
Resolving a comment whose mark spans more than one paragraph was emitting N pairs of
commentRangeStart/commentRangeEnd/commentReferencemarkers all sharing the samew:id. The exported DOCX read fine in Word but is non-conformant per ECMA-376 §17.13.4 (theidattribute uniquely identifies an annotation; one start, one end, one reference per id), and Word would silently merge or render duplicates inconsistently.Verified against a Word-authored fixture (multi-paragraph comment): Word emits one pair, with the paragraph close sitting inside the range. The fix collapses the segment list returned by
getCommentMarkSegmentsByIdinto one envelope range; all segments share the samecommentIdby construction, so they all belong to the same logical annotation.Trigger: only multi-paragraph or discontinuous comments. Single-paragraph comments were always correct.
Verified:
SuperDoc(11).docxhad id=3 with 2/2/2 markers; post-fixSuperDoc(12).docxhas 1/1/1 for every id.