fix(docx-core): preserve reply comments in rebuild mode (#108)#112
Merged
stevenobiajulu merged 1 commit intomainfrom Apr 28, 2026
Merged
fix(docx-core): preserve reply comments in rebuild mode (#108)#112stevenobiajulu merged 1 commit intomainfrom
stevenobiajulu merged 1 commit intomainfrom
Conversation
Reply comments — and their `commentsExtended.xml` / `people.xml` entries —
were silently dropped from rebuild output. Reply `<w:comment>` entries do
not have their own `<w:commentReference>` in document.xml; they thread
through `commentsExtended.xml` via `w15:paraIdParent`. The previous
implementation only walked the directly-merged root comment IDs, so
reply entries never entered the merge set.
Two compounding bugs:
1. The comment-merge post-pass walked only `commentMergeResult.mergedIds`
— root IDs that the generic auxiliary-part merge appended. Replies
are siblings, not nested, so they were never even inspected.
2. The post-pass call site was gated on `mergeResults.has('comment')`,
meaning it only ran when at least one root comment was newly merged.
When the original already contained the root and the revised side
only added replies under it, the pass never ran at all and replies
were dropped even with the BFS fix from (1) in place. (Surfaced by
peer review of the v1 plan.)
Fix:
- Drive the post-pass off `<w:commentReference>` IDs in the result
document, so it runs whenever any comment is anchored — regardless of
what the generic merge appended.
- Build full source-comment maps (id ↔ paraId ↔ author) once.
- BFS the `w15:paraIdParent` graph from each root paraId, only
including children that resolve to a real source `<w:comment>` so
orphaned `commentEx` entries can't drag in dangling ancillary XML.
- Append any included `<w:comment>` definitions still missing from the
result `comments.xml` (replies — and defensively, any roots).
- Then call the existing `mergeCommentsExtended` / `mergePeople` with
the expanded paraId / author sets.
Regression coverage adds three rebuild-mode tests: reply-to-existing-root
(catches the gating bug), bootstrap with reply (root and reply both new),
and a 3-level chain that justifies the BFS by behavior rather than by
implementation detail.
Fixes: #108
Ref: #94, #101
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
3 tasks
stevenobiajulu
added a commit
that referenced
this pull request
Apr 28, 2026
* chore(release): bump workspace versions to 0.9.1 Patch release covering bug fixes accumulated since v0.9.0: - fix(docx-core): merge auxiliary parts in rebuild mode (#94, #101) - fix(docx-core): emit paragraph-level markers outside <w:r> on rebuild (#109) - fix(docx-core): preserve reply comments in rebuild mode (#108, #112) - test(docx-mcp): harden vitest path allowlist against /tmp symlink topology (#105) - ci: SHA-pin all GitHub Actions, add dependency review and Dependabot - ci: fix PR title check for acronyms and fork PRs (#88) - chore(skills): sync docx-editing to 0.3.0 (#95) Generated via scripts/bump_version.mjs. * fix(docx-mcp): declare @xmldom/xmldom dependency explicitly The bumped package-lock.json regenerated cleanly and exposed a latent bug: docx-mcp directly imports @xmldom/xmldom in tag_parser.ts but did not declare it as a dependency. It was previously masked by npm hoisting from docx-core's transitive dep — the workspace happened to work because the lockfile carried a stale hoisted xmldom@0.8.11. Pinning to ^0.8.11 (matching what was already running in production via the silent hoist) preserves observable behavior. tag_parser depends on xmldom 0.8's lenient parsing of cross-nested tags (<b><i>text</b></i>) — xmldom 0.9 throws on such input. Migrating docx-mcp's tag_parser to xmldom 0.9 is its own concern; this commit keeps the 0.9.1 release a pure version bump. docx-core continues to use xmldom ^0.9.9 (PR #75); the two versions coexist under their respective package's node_modules. No DOM nodes cross the docx-core/docx-mcp boundary, so the version split is safe.
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
1 participant
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.
Summary
Fix issue #108: in rebuild mode, threaded reply comments — and their
commentsExtended.xml/people.xmlentries — were silently dropped from output.Reply
<w:comment>entries do not have their own<w:commentReference>indocument.xml; they thread throughcommentsExtended.xmlviaw15:paraIdParent. Two compounding bugs caused the drop:mergeResults.has('comment'), so it didn't even run when the original already contained the root and the revised side only added replies under it. Surfaced by peer review of the initial plan.Fix shape
<w:commentReference>IDs in the resultdocument.xml, so it runs whenever any comment is anchored — regardless of what the generic auxiliary merge appended.id ↔ paraId ↔ author) once.w15:paraIdParentgraph from each root paraId, only including children that resolve to a real source<w:comment>(so orphanedcommentExentries can't drag in dangling ancillary XML).<w:comment>definitions still missing from the resultcomments.xml(replies — and defensively, any roots).mergeCommentsExtended/mergePeoplewith the expanded paraId / author sets.The generic
mergeAuxiliaryPartDefinitionshelper stays generic —paraIdParentis a comment-only concern.Regression coverage (3 new tests in
rebuild-auxiliary-merge.test.ts)Fixes: #108
Ref: #94, #101
Test plan
@usejunior/docx-coresuite — 1097 passed | 1 skippednpm run build— cleannpm run lint:workspaces— clean (1 pre-existing warning in unrelated file)npm run test:run— full suite greennpm run check:spec-coverage— all PASS