Repository navigation
fix: add_comment finds anchors that span w:t run boundaries - #21
Merged
Merged
Conversation
add_comment() used get_node(tag='w:t', contains=anchor_text), which only matched text inside a single <w:t> run. count_matches() and the hash-ref edit API used build_text_map() and matched across runs, so add_comment() raised TextNotFoundError on anchors that count_matches reported as present (real-world repro on a 9,641-paragraph docx). add_comment now uses build_text_map + find_in_text_map to locate the anchor at exact character positions across run boundaries. New paragraph= and occurrence= keyword-only parameters bring the API in line with the hash-anchored edit methods. Range markers are placed character-precisely; runs are split when start and end positions sit inside different <w:t> nodes.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds paragraph-scoped and occurrence-based anchoring to Document.add_comment/CommentManager.add_comment, resolves anchors using the document text map to compute exact character offsets, and inserts character-precise comment range markers by splitting w:t nodes and rebuilding runs. Tests cover split text nodes and cross-run anchors. ChangesExtended comment anchoring with paragraph scoping and text-map precision
Sequence DiagramsequenceDiagram
participant Caller
participant Document
participant CommentManager
participant TextMap
participant WordXML
Caller->>Document: add_comment(anchor_text, comment, paragraph?, occurrence?)
Document->>CommentManager: add_comment(anchor_text, comment, paragraph?, occurrence?)
CommentManager->>CommentManager: Validate anchor_text (not empty)
alt paragraph scoped search
CommentManager->>CommentManager: Load paragraph by reference, validate hash
CommentManager->>TextMap: Search anchor within paragraph with occurrence
else document-wide search
CommentManager->>TextMap: Search anchor across document with occurrence
end
TextMap-->>CommentManager: TextMapMatch(node, start_offset, end_offset)
CommentManager->>CommentManager: Generate comment ID, paragraph ID, durable ID, timestamp
CommentManager->>WordXML: _place_markers(match) — compute positions
CommentManager->>WordXML: _rebuild_run_with_markers — split w:t nodes, insert markers
WordXML->>WordXML: Split affected runs, preserve non-w:t children, insert w:commentRangeStart/End
WordXML-->>CommentManager: Updated run structure
CommentManager-->>Document: comment_id
Document-->>Caller: comment_id
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ty could not narrow start_offset/end_offset (int | None) or start_marker/end_marker (str | None) through the is_start_here / is_end_here boolean flags, producing 8 unsupported-operator / invalid-argument-type diagnostics on the int+1 and list.append calls. Make the offsets plain int (callers pass 0 as placeholder) and the markers plain str (empty string = no marker). bool(start_marker) replaces start_marker is not None. Semantics and tests unchanged.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #21 +/- ##
======================================
Coverage 94.0% 94.1%
======================================
Files 10 10
Lines 2180 2291 +111
Branches 484 515 +31
======================================
+ Hits 2051 2156 +105
- Misses 34 36 +2
- Partials 95 99 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Pins the run-boundary fix to vincentfyt's reported failure mode: a 29-character Chinese anchor spanning nine <w:t> elements. count_matches and find_text accepted it before this branch, add_comment rejected it.
Three small regression tests fill the codecov/patch gaps in the new add_comment cross-run anchor code: - test_paragraph_index_out_of_range_raises — exercises the ParagraphIndexError raise in _find_anchor when paragraph= refers to an index past the end of the document. - test_stale_hash_long_paragraph_truncates_preview — exercises the "preview += '...'" truncation branch of HashMismatchError when the paragraph text exceeds 80 chars. - test_orphan_wt_raises_comment_error — exercises the _find_ancestor_run -> None path by synthesizing a bare <w:t> with no <w:r> ancestor, asserting the CommentError guard fires. Coverage on docx_editor/comments.py goes from ~92% to 95%.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docx_editor/comments.py`:
- Around line 735-744: Replace the manual chain of .replace calls in _escape_xml
with the standard library helper: call html.escape(text, quote=True) and then
(if you still need apostrophe escaped) replace "'" with "'" on the result;
update the _escape_xml function to use html.escape for &, <, > and " and only
handle the apostrophe specially to keep behavior identical while reducing
maintenance.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3ecdd6e9-3ec4-4a1d-95a4-a1e901b96817
📒 Files selected for processing (3)
docx_editor/comments.pydocx_editor/document.pytests/test_comments.py
Comment on lines
+735
to
+744
| def _escape_xml(text: str) -> str: | ||
| """Escape text for safe XML inclusion.""" | ||
| return ( | ||
| text | ||
| .replace("&", "&") | ||
| .replace("<", "<") | ||
| .replace(">", ">") | ||
| .replace('"', """) | ||
| .replace("'", "'") | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider using html.escape instead of manual XML escaping.
html.escape (already imported at line 6) handles &, <, >, and " when quote=True. For the apostrophe, you could extend it or keep the current implementation. The current code is correct but introduces minor maintenance overhead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docx_editor/comments.py` around lines 735 - 744, Replace the manual chain of
.replace calls in _escape_xml with the standard library helper: call
html.escape(text, quote=True) and then (if you still need apostrophe escaped)
replace "'" with "'" on the result; update the _escape_xml function to use
html.escape for &, <, > and " and only handle the apostrophe specially to keep
behavior identical while reducing maintenance.
Three additional tests for the unscoped (paragraph=None) anchor lookup in CommentManager._find_anchor, where codecov/patch flagged 3 missing statements (lines 216-217, 219-223): - test_document_wide_occurrence_skips_earlier_matches — exercises the current/local counter increments inside the while loop when occurrence > 0. - test_document_wide_occurrence_out_of_range — exercises the TextNotFoundError raise with total_occurrences set on the occurrence > 0 / current > 0 fall-through. - test_scoped_occurrence_when_anchor_absent — exercises the symmetric scoped path where occurrence > 0 but total == 0, raising plain TextNotFoundError without total_occurrences. Statement coverage on docx_editor/comments.py reaches 100% (Miss=0). Remaining partials are branch-only on defensive guards.
Contributor
|
Thanks for shipping this! We hit the same issue (~30% anchor failure on Chinese commercial contracts with whitespace placeholders) and were working around it with a wrapper that finds a unique substring within the target paragraph. The new keyword-only Any rough ETA for a PyPI release that includes this fix? We're currently pinned to 0.2.3 in production. Happy to provide additional test cases (Chinese commercial contracts with table/form-field placeholders) if useful for regression coverage. |
Owner
Author
|
I will do soon a new release. Let you know. I was working on something else, and waiting to put it together, but it doesn't hurt to release a new version.
That would be awesome! Could you please propose a Pull Request? I will merge it. |
Owner
Author
|
@snowmays you can install the new version now 0.2.4 Or, if you prefer a specific version: |
pablospe
added a commit
that referenced
this pull request
Jul 14, 2026
* fix: unify occurrence search through text maps (ISSUES.md #21) Document-wide replace/delete/insert used an element-level w:t search (_get_nth_match) with a text-map fallback, and the two counted occurrences over different sequences: a match split across fragmented runs was invisible to the element-level path, so occurrence=N could target different matches depending on which path won. Route all three methods through a single _find_document_wide text-map lookup (the same sequence count_matches uses), delete the duplicate single-node edit bodies and the drift shim (~250 lines), and rewrite _insert_near_match to split the edge w:t at the match boundary, fixing mid-node anchor placement for the paragraph-scoped and batch_edit insert paths as well ("HelloXX world", not "Hello worldXX"). TextNotFoundError.total_occurrences now matches count_matches, and runs with non-text children (w:tab, w:br) keep them during inserts. * review: add occurrence default to _find_document_wide Multi-reviewer pass on PR #39. Fixed: missing `occurrence: int = 0` default, matching the sibling finders (_find_in_paragraph, _find_across_boundaries). Verified as not applicable here and deferred with notes in ISSUES.md: straddle-match insertion into a foreign w:ins (needs #19's ins-splitting machinery; in-place rebuild would nest w:ins) and front-hoisted non-text run children in the insert rebuild (#20, same pattern as the existing replace/delete paths). xml:space on generated w:t was a false positive — the editor's attribute injection adds it.
This was referenced Jul 14, 2026
Merged
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
2 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.
Closes #14.
Summary
add_comment()used a single-w:tlookup and raisedTextNotFoundErroron anchors thatcount_matches()reported as present (formatting changes, smart-quote splits,w:inswrappers, multi-run CJK text).w:truns.Changes
CommentManager.add_commentnow usesbuild_text_map+find_in_text_mapfor character-precise anchor lookup that spans run boundaries.paragraph=andoccurrence=parameters bring the API in line with the hash-anchored edit methods.w:tnodes.Test plan
uv run pytest tests/test_comments.pyparagraph=+occurrence=behave the same as the edit-method counterpartsSummary by CodeRabbit
New Features
Tests