fix(super-editor): keep comment range markers around unpaired tracked changes (SD-2528)#3239
Merged
Merged
Conversation
… tracked changes (SD-2528) mergeConsecutiveTrackedChanges greedily absorbed trailing w:commentRangeEnd and w:r->w:commentReference elements into a w:del/w:ins wrapper even when no same-id wrapper followed to actually merge into. This produced a lopsided structure where w:commentRangeStart sat outside the wrapper but w:commentRangeEnd ended up inside it, breaking comment round-trip on redlined text. Buffer comment markers during the forward scan and only commit them inside the wrapper when a same-id merge actually happens. Otherwise emit them as siblings, matching Word's expected OOXML. SD-1519 merge behavior is unchanged and covered by the new tests.
superdoc-bot
Bot
added
review: thorough
review: careful
and removed
review: thorough
labels
Contributor
|
Note: The MCP Status: PASS The PR doesn't change the OOXML vocabulary being emitted — it only adjusts where comment range markers land relative to a tracked-change wrapper. Quick verification against the spec:
The |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
luccas-harbour
requested changes
May 13, 2026
Contributor
luccas-harbour
left a comment
luccas-harbour
left a comment
There was a problem hiding this comment.
hey @tupizz!
I performed the same steps that were shown in the loom video that was in the ticket and I can see that the problems persist:
- After recreating the same document as in the video, exporting it and loading it into SD again, the comments are displayed separately instead of threaded
- If I accept the added track change, the comment reply does not go away
- If I accept the replacement track change, the comment reply doesn't go away either
Can you check?
…rip (SD-2528) The previous fix left commentRangeStart as a sibling of w:ins/w:del. documentCommentsImporter.js' extractCommentRangesFromDocument only associates a comment with a tracked change when commentRangeStart sits inside the wrapper, so the sibling shape silently dropped the comment to TC link on re-import. Fold any leading commentRangeStart sibling into the immediately following w:ins/w:del as its first child, matching the shape Word produces. The existing SD-1519 same-id merge for trailing commentRangeEnd and w:r/w:commentReference stays unchanged. Adds an end-to-end test that loads the Google-Docs TC+comment fixture, exports it, re-imports the exported XML, and asserts every comment that was originally inside a tracked change still carries trackedChangeParentId after the round-trip.
…ts (SD-2528) A user comment anchored to a tracked change carries trackedChangeParentId pointing at the TC. Two bugs broke the link end-to-end: 1. docxImporter built two tracked-change id maps independently (trackedChangeIdMap and trackedChangeIdMapsByPart), each minting fresh UUIDs for the same Word w:id. The comments importer used the global map; ins/del translators used the per-part map. The two never matched, so trackedChangeParentId on a comment never pointed at the actual TC mark id in the PM doc. Fix: build the per-part maps first and reuse the document.xml entry as the global map. 2. The comments-store resolve handler only resolved the TC's own redline-display entry. User comments with trackedChangeParentId === the resolved TC stayed open. Fix: after resolving the TC entity, iterate commentsList and resolve every comment whose trackedChangeParentId matches. Defer via Promise.resolve so the cascading resolveComment doesn't dispatch into a still-running accept/rejectTrackedChangeById loop and collide with the loop's mutable tr. E2E browser repro on the real Google-Docs TC+comment fixture: accept TC by id, both the TC and its anchored user comments resolve in one user action. Same for reject. No mismatched-tr errors. The export-side round-trip test also asserts the two id maps are aligned and every comment trackedChangeParentId matches a real tracked-change mark id in the PM doc.
… on re-import (SD-2528) A reply that the user typed under a tracked-change bubble has parentCommentId pointing at the synthetic TC entity in the comments store. On export the TC parent is filtered out of comments.xml (TC entries are not real comments), so the reply lands in the file without any paraIdParent. On re-import the reply gets trackedChangeParentId via the document.xml walker (the commentRange wraps the TC text) but parentCommentId was left undefined — the sidebar then renders the reply as a separate top-level bubble next to the TC instead of nested under it, matching the user-reported regression in image 1 of SD-2528. Promote trackedChangeParentId to parentCommentId when no explicit parent is set. CommentDialog already threads via direct parentCommentId === trackedChangeId (line 321), so this is the cheapest path to restore the live pre-export state. Round-trip stable: re-export still filters TC parents but re-emits the commentRange inside the wrapper, which gets re-detected on the next import via extractCommentRangesFromDocument and re-establishes the linkage.
Contributor
Author
|
thanks @luccas-harbour I missunderstood the requirements |
…n (SD-2528) The UI guarded TC reply threading with isRangeThreadedComment, which is true only when the source DOCX has no commentsExtended.xml (Google Docs style). SuperDoc-exported DOCX files always write commentsExtended.xml, so on re-import the guard short-circuited and the reply rendered as a top-level bubble next to its TC instead of nested under it. Drop the file-origin guard from the two sites that threaded TC replies: collectTrackedChangeThread in CommentDialog.vue and shouldThreadWithTrackedChange in comments-store.js. trackedChangeParentId pointing at a tracked-change entity is sufficient to thread; file origin should not change whether a comment threads under its TC. Reverts the earlier importer-side patch that promoted trackedChangeParentId into parentCommentId. That patch violated the comment-diffing contract (parentCommentId is diffed; trackedChangeParentId is intentionally ignored because it is regenerated across imports) and broke six existing tests. The UI-side change is surgical and breaks no tests.
…RTED/resolve gates (SD-2528) Three visual round-trip regressions after the SD-2528 fix made TC replies thread again: 1. CommentHighlightDecorator painted its pink (external) / green (internal) inline background on every element with the superdoc-comment-highlight class — including text that already carries a track-insert-dec / track-delete-dec decoration. The inline style won over the TC's own CSS class background, so a green trackInsert came back pink after re-import. Skip the BG override when the element is also a tracked-change decoration: the TC color (green for insert, red for delete) is the right signal for the user, and the comment range is still visually identified by its dashed border + sidebar bubble. 2. CommentHeader's IMPORTED tag fired whenever comment.origin or importedAuthor was set — including comments authored by the current user in a previous session. Round-tripping a file you exported then re-opened should not relabel your own comments as imported. Suppress the tag when the comment's creatorEmail matches the current user's email. 3. CommentHeader's allowResolve guard treated parentCommentId as the only marker of a child comment. A TC-anchored reply on re-import keeps the linkage through trackedChangeParentId only (parentCommentId is left undefined to preserve the comment-diffing contract). The resolve check affordance therefore appeared on re-imported replies even though the pre-export state had no parentCommentId either. Treat trackedChangeParentId as an equivalent child signal. All three are surgical render-side gates — no converter / data-model changes. 1369 super-editor presentation tests pass.
luccas-harbour
requested changes
May 14, 2026
Contributor
luccas-harbour
left a comment
luccas-harbour
left a comment
There was a problem hiding this comment.
hey @tupizz!
do you mind checking these?
- [P2] Restrict cascading resolve to the active document — packages/superdoc/src/stores/comments-store.js:690-695
When a tracked-change resolve event includes `documentId`, `findTrackedChangeById()` filters to that document, but this new cascade scans every item in `commentsList`. In multi-document sessions where imported/live tracked-change ids are reused across documents, accepting or rejecting a change in one document will also resolve comments attached to the same `trackedChangeParentId` in another document.
- [P2] Only suppress comment fill for highlighted redlines — packages/super-editor/src/editors/v1/core/presentation-editor/dom/CommentHighlightDecorator.ts:184-186
This treats any base tracked-change class as having its own background, but final/original modes still render visible spans with `track-insert-dec`/`track-delete-dec` and no `.highlighted` background, and format changes only have a border. In those modes a comment anchored on the text has its comment background cleared with no TC background replacing it, making the comment highlight disappear.
- [P2] Preserve regular parent threads for TC-anchored replies — packages/superdoc/src/components/CommentsLayer/CommentDialog.vue:327-329
This now pulls every comment with `trackedChangeParentId` into the tracked-change dialog, even when the comment also has a `parentCommentId` pointing to a regular comment thread. The importer supports that shape for replies whose range is inside a TC but whose parent spans outside it, so those replies will appear both under their real parent and inside the TC dialog without the parent context.
…s (SD-2528) Addresses Codex's 3 P2 review findings from PR #3239: P2 #1 — comments-store.js cascade scope The new cascade-resolve scan introduced in aa88a58 didn't honour the resolve event's documentId. findTrackedChangeById (line 591) correctly scopes its match by belongsToTrackedChangeSyncDocument; the cascade six lines lower did not. In multi-document sessions where imported tracked-change ids collide across files (each w:id space is local), accepting a change in document A would also resolve comments anchored on the same id in document B. Mirror the same per-document filter when a documentId is provided; single-document callers (no documentId on the event) keep the legacy global behaviour. P2 #2 — CommentHighlightDecorator.ts visual gate The earlier suppression triggered on any of `track-insert-dec`, `track-delete-dec`, or `track-format-dec`. Per layout-engine styles.ts only `.track-insert-dec.highlighted` and `.track-delete-dec.highlighted` paint a background; `.track-format-dec` only paints a border-bottom, and the `.highlighted` modifier is only applied in "review" / All Markup mode (renderer.ts:909-928). In Original / Final modes, and on format-only changes, the suppression cleared the comment fill with nothing to replace it, making the bubble invisible. Tighten the gate to require both `.highlighted` and one of the bg-painting base classes. P2 #3 — collectTrackedChangeThread parent shadowing documentCommentsImporter can produce a comment with BOTH a non-TC `parentCommentId` and a `trackedChangeParentId`: the comment's range lives inside a TC, but its conversational thread starts at a regular comment outside the TC. The previous unconditional pull on trackedChangeParentId placed such replies in both threads. Restrict the direct seed to roots (no parentCommentId) and let the BFS step pick up same-TC-anchored chains via parent links. Extract the helper to a sibling module so the BFS logic can be unit-tested in isolation — previously trapped inside CommentDialog.vue's <script setup>. Verification - 8 new unit tests covering each P2 case (3 in collect-tracked-change-thread.test.js, 4+ in CommentHighlightDecorator.test.ts, 1 cross-doc + 1 single-doc regression in comments-store.test.js). - SD-2528 integration round-trip test still passes (1/1). - super-editor: 12 850 / 12 850 unit tests pass. - superdoc: 966 unit tests pass (1 pre-existing collab-server import failure, unrelated, present on main and other branches). - Browser repro on the corpus fixture: accepting an imported TC still cascade-resolves both anchored user comments end-to-end.
Contributor
Author
|
Pushed Fixes
TDD evidence (Red → Green)
Regression guard
Why extract
|
Contributor
Author
|
@luccas-harbour all three of Codex's P2 concerns are addressed in
TDD: 8 new failing tests before the fixes, all green after. SD-2528 round-trip test still passes. Full Full verification matrix and per-fix line counts in the previous comment. |
Contributor
|
🎉 This PR is included in superdoc v1.30.0-next.93 The release is available on GitHub release |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Demo
After export-import
Summary
When a user comments on redlined text, the exported DOCX put
w:commentRangeStartoutside thew:delwrapper butw:commentRangeEndand thew:commentReferencerun inside it. Re-importing that file separates the comment from the redline (and from itself) instead of restoring a single unified bubble. Customer-reported (Bonterms).Root cause is in
mergeConsecutiveTrackedChanges(translate-paragraph-node.js). The forward scan greedily absorbed every comment marker / comment-reference run it found after aw:ins/w:del, even when no same-id wrapper actually followed to merge with. So a lone tracked change always swallowed its trailing comment markers.Fix: buffer comment markers during the scan and only commit them inside the wrapper when a same-id merge actually happens. Otherwise emit them as siblings.
w:commentRangeStart...w:del...w:commentRangeEnd...w:r/w:commentReferencenow stays as siblings (SD-2528).Test plan
translate-paragraph-node.test.jscover three cases: SD-2528 (single wrapper + trailing comments), SD-1519 (same-id merge absorbs comments), different-id (no merge, comments stay siblings).pnpm --filter super-editor testpasses (1008 files, 12711 tests).