From 57d11af3bab5cc62090d5d86342579f2235f71ab Mon Sep 17 00:00:00 2001 From: Pablo Speciale Date: Sat, 16 May 2026 18:24:32 +0200 Subject: [PATCH 1/5] fix: add_comment finds anchors that span w:t run boundaries add_comment() used get_node(tag='w:t', contains=anchor_text), which only matched text inside a single 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 nodes. --- docx_editor/comments.py | 300 ++++++++++++++++++++++++++++++---- docx_editor/document.py | 26 ++- tests/test_comments.py | 347 ++++++++++++++++++++++++++++++---------- 3 files changed, 553 insertions(+), 120 deletions(-) diff --git a/docx_editor/comments.py b/docx_editor/comments.py index 6d40b11..93ff1cb 100644 --- a/docx_editor/comments.py +++ b/docx_editor/comments.py @@ -8,9 +8,24 @@ from dataclasses import dataclass, field from datetime import datetime, timezone from pathlib import Path - -from .exceptions import CommentError, TextNotFoundError -from .xml_editor import DocxXMLEditor, _generate_hex_id, get_text_node_data +from xml.dom.minidom import Element + +from .exceptions import ( + CommentError, + HashMismatchError, + ParagraphIndexError, + TextNotFoundError, +) +from .xml_editor import ( + DocxXMLEditor, + ParagraphRef, + TextMapMatch, + _generate_hex_id, + build_text_map, + compute_paragraph_hash, + find_in_text_map, + get_text_node_data, +) # Path to template files TEMPLATE_DIR = Path(__file__).parent / "ooxml" / "templates" @@ -90,45 +105,48 @@ def _get_editor(self, xml_path: Path) -> DocxXMLEditor: ) return self._editors[path_str] - def add_comment(self, anchor_text: str, comment_text: str) -> int: + def add_comment( + self, + anchor_text: str, + comment_text: str, + *, + paragraph: str | None = None, + occurrence: int = 0, + ) -> int: """Add a comment anchored to specific text. + The anchor lookup uses the same text-map search as ``count_matches`` and + the tracked-change edit methods, so anchors that span ``w:t`` run + boundaries (formatting changes, smart-quote splits, ``w:ins`` wrappers) + are found. + Args: - anchor_text: Text to attach the comment to - comment_text: The comment content + anchor_text: Text to attach the comment to. + comment_text: The comment content. + paragraph: Optional paragraph reference (e.g., ``"P3#a7b2"``) to + scope the search. ``None`` searches the whole document. + occurrence: Which occurrence to anchor (0 = first). Returns: - The comment ID + The comment ID. Raises: - TextNotFoundError: If the anchor text is not found + TextNotFoundError: If the anchor text is not found. + HashMismatchError: If a paragraph reference's hash is stale. + ParagraphIndexError: If a paragraph reference's index is out of range. + CommentError: If ``anchor_text`` is empty. """ - # Find the anchor element - try: - elem = self.document_editor.get_node(tag="w:t", contains=anchor_text) - except Exception: - raise TextNotFoundError(anchor_text) from None - - # Get the parent run and paragraph - run = elem.parentNode - while run and run.nodeName != "w:r": - run = run.parentNode - - para = run - while para and para.nodeName != "w:p": - para = para.parentNode - - if not run or not para: - raise CommentError("Could not find parent run/paragraph") + if not anchor_text: + raise CommentError("anchor_text must not be empty") + _, match = self._find_anchor(anchor_text, paragraph, occurrence) comment_id = self.next_comment_id para_id = _generate_hex_id() durable_id = _generate_hex_id() timestamp = datetime.now(timezone.utc).strftime("%Y-%m-%dT%H:%M:%SZ") - # Add comment range markers to document.xml - self.document_editor.insert_before(run, self._comment_range_start_xml(comment_id)) - self.document_editor.append_to(para, self._comment_range_end_xml(comment_id)) + # Place range markers at character-precise positions in document.xml + self._place_markers(match, comment_id) # Add to all comment XML files self._add_to_comments_xml(comment_id, para_id, comment_text, timestamp) @@ -142,6 +160,198 @@ def add_comment(self, anchor_text: str, comment_text: str) -> int: return comment_id + def _find_anchor(self, anchor_text: str, paragraph: str | None, occurrence: int) -> tuple[Element, TextMapMatch]: + """Locate ``anchor_text`` and return its paragraph + text-map match. + + Mirrors ``RevisionManager._find_in_paragraph`` / + ``_find_across_boundaries`` so comment anchors and edit anchors find + the same text. + """ + if paragraph is not None: + ref = ParagraphRef.parse(paragraph) + paragraphs = self.document_editor.dom.getElementsByTagName("w:p") + if ref.index < 1 or ref.index > len(paragraphs): + raise ParagraphIndexError(ref.index, len(paragraphs)) + p = paragraphs[ref.index - 1] + actual = compute_paragraph_hash(p) + if actual != ref.hash: + tm = build_text_map(p) + preview = tm.text[:80] + if len(tm.text) > 80: + preview += "..." + raise HashMismatchError(ref.index, ref.hash, actual, preview) + text_map = build_text_map(p) + match = find_in_text_map(text_map, anchor_text, occurrence) + if match is not None: + return p, match + if occurrence > 0: + total = 0 + while find_in_text_map(text_map, anchor_text, total) is not None: + total += 1 + if total > 0: + raise TextNotFoundError( + anchor_text, + paragraph_ref=paragraph, + paragraph_preview=text_map.text, + occurrence=occurrence, + total_occurrences=total, + ) + raise TextNotFoundError( + anchor_text, + paragraph_ref=paragraph, + paragraph_preview=text_map.text, + ) + + # Document-wide search + current = 0 + for p in self.document_editor.dom.getElementsByTagName("w:p"): + text_map = build_text_map(p) + local = 0 + while True: + m = find_in_text_map(text_map, anchor_text, local) + if m is None: + break + if current == occurrence: + return p, m + current += 1 + local += 1 + if occurrence > 0 and current > 0: + raise TextNotFoundError( + anchor_text, + occurrence=occurrence, + total_occurrences=current, + ) + raise TextNotFoundError(anchor_text) + + def _place_markers(self, match: TextMapMatch, comment_id: int) -> None: + """Insert range start/end markers at the precise match boundaries.""" + start_pos = match.positions[0] + end_pos = match.positions[-1] + start_node = start_pos.node + start_offset = start_pos.offset_in_node + end_node = end_pos.node + end_offset = end_pos.offset_in_node + + start_run = _find_ancestor_run(start_node) + end_run = _find_ancestor_run(end_node) + if start_run is None or end_run is None: + raise CommentError("Could not find parent run for comment anchor") + + start_marker = self._comment_range_start_xml(comment_id) + end_marker = self._comment_range_end_xml(comment_id) + + if start_run is end_run: + self._rebuild_run_with_markers( + start_run, + start_node, + start_offset, + end_node, + end_offset, + start_marker, + end_marker, + ) + return + + # Independent runs — place END first (after start_run in document order), + # then START. Either order works since the two runs are siblings, but + # this matches the plan and reads naturally. + self._rebuild_run_with_markers( + end_run, + None, + None, + end_node, + end_offset, + None, + end_marker, + ) + self._rebuild_run_with_markers( + start_run, + start_node, + start_offset, + None, + None, + start_marker, + None, + ) + + def _rebuild_run_with_markers( + self, + run: Element, + start_node, + start_offset: int | None, + end_node, + end_offset: int | None, + start_marker: str | None, + end_marker: str | None, + ) -> None: + """Rebuild ``run``'s XML, splitting ``w:t`` children to insert markers. + + ``start_marker`` / ``end_marker`` are inserted right before the + character at ``start_offset`` of ``start_node`` and right after the + character at ``end_offset`` of ``end_node`` respectively. Pass ``None`` + for an end whose marker lives in a different run. + + Iterates *direct* children of ``run`` (not descendants) so non-``w:t`` + content like ````, ````, ````, and field + markers is preserved, and so ``w:t`` nodes nested inside a drawing's + text box are left untouched. + """ + rPr_xml = _get_rPr_xml(run) + xml_parts: list[str] = [] + + for child in run.childNodes: + if child.nodeType != child.ELEMENT_NODE: + continue + tag = getattr(child, "tagName", "") + if tag == "w:rPr": + continue # Already captured by rPr_xml + if tag != "w:t": + # Non-text content (w:tab, w:br, w:drawing, …) — preserve in + # its own run so document order is maintained. + xml_parts.append(f"{rPr_xml}{child.toxml()}") + continue + + wt = child + wt_text = get_text_node_data(wt) + is_start_here = start_marker is not None and wt is start_node + is_end_here = end_marker is not None and wt is end_node + + if not is_start_here and not is_end_here: + if wt_text: + xml_parts.append(f"{rPr_xml}{_escape_xml(wt_text)}") + continue + + if is_start_here and is_end_here: + before = wt_text[:start_offset] + matched = wt_text[start_offset : end_offset + 1] + after = wt_text[end_offset + 1 :] + if before: + xml_parts.append(f"{rPr_xml}{_escape_xml(before)}") + xml_parts.append(start_marker) + if matched: + xml_parts.append(f"{rPr_xml}{_escape_xml(matched)}") + xml_parts.append(end_marker) + if after: + xml_parts.append(f"{rPr_xml}{_escape_xml(after)}") + elif is_start_here: + before = wt_text[:start_offset] + after = wt_text[start_offset:] + if before: + xml_parts.append(f"{rPr_xml}{_escape_xml(before)}") + xml_parts.append(start_marker) + if after: + xml_parts.append(f"{rPr_xml}{_escape_xml(after)}") + else: # is_end_here only + before = wt_text[: end_offset + 1] + after = wt_text[end_offset + 1 :] + if before: + xml_parts.append(f"{rPr_xml}{_escape_xml(before)}") + xml_parts.append(end_marker) + if after: + xml_parts.append(f"{rPr_xml}{_escape_xml(after)}") + + self.document_editor.replace_node(run, "".join(xml_parts)) + def reply_to_comment(self, parent_comment_id: int, reply_text: str) -> int: """Add a reply to an existing comment. @@ -497,3 +707,37 @@ def _comment_ref_run_xml(self, comment_id: int) -> str: """ + + +def _find_ancestor_run(node) -> Element | None: + """Walk parents until reaching a ``w:r`` element, or ``None``.""" + parent = node.parentNode + while parent: + if parent.nodeType == parent.ELEMENT_NODE and parent.tagName == "w:r": + return parent + parent = parent.parentNode + return None + + +def _get_rPr_xml(run) -> str: + """Serialize ``run``'s direct ``w:rPr`` child, or empty string. + + Iterates direct children so a nested ``w:rPr`` deep inside a ``w:drawing`` + text-box descendant is not picked up by mistake. + """ + for child in run.childNodes: + if child.nodeType == child.ELEMENT_NODE and getattr(child, "tagName", "") == "w:rPr": + return child.toxml() + return "" + + +def _escape_xml(text: str) -> str: + """Escape text for safe XML inclusion.""" + return ( + text + .replace("&", "&") + .replace("<", "<") + .replace(">", ">") + .replace('"', """) + .replace("'", "'") + ) diff --git a/docx_editor/document.py b/docx_editor/document.py index ba36517..a9a362a 100644 --- a/docx_editor/document.py +++ b/docx_editor/document.py @@ -344,21 +344,37 @@ def batch_rewrite(self, rewrites: list[tuple[str, str]]) -> list[str]: # ==================== Comments API ==================== - def add_comment(self, anchor_text: str, comment: str) -> int: + def add_comment( + self, + anchor_text: str, + comment: str, + *, + paragraph: str | None = None, + occurrence: int = 0, + ) -> int: """Add a comment anchored to specific text. + Anchors are located with the same text-map search used by + :meth:`count_matches` and the tracked-change edit methods, so anchors + that span ``w:t`` run boundaries (formatting changes, smart-quote + splits, ``w:ins`` wrappers) are found. + Args: - anchor_text: Text to attach the comment to - comment: The comment content + anchor_text: Text to attach the comment to. + comment: The comment content. + paragraph: Optional paragraph reference (e.g., ``"P3#a7b2"``) to + scope the search. ``None`` searches the whole document. + occurrence: Which occurrence to anchor to (0 = first). Returns: - The comment ID + The comment ID. Example: doc.add_comment("Section 5", "Please review this section") + doc.add_comment("foo", "Note", paragraph="P3#a7b2", occurrence=1) """ self._ensure_open() - return self._comment_manager.add_comment(anchor_text, comment) + return self._comment_manager.add_comment(anchor_text, comment, paragraph=paragraph, occurrence=occurrence) def reply_to_comment(self, comment_id: int, reply: str) -> int: """Add a reply to an existing comment. diff --git a/tests/test_comments.py b/tests/test_comments.py index 0be8d11..5fc9672 100644 --- a/tests/test_comments.py +++ b/tests/test_comments.py @@ -2,7 +2,12 @@ import pytest -from docx_editor import CommentError, Document, TextNotFoundError +from docx_editor import ( + CommentError, + Document, + HashMismatchError, + TextNotFoundError, +) class TestAddComment: @@ -903,14 +908,9 @@ class TestAddCommentParentTraversal: """Tests for parent traversal in add_comment.""" def test_add_comment_traverses_to_find_run(self, clean_workspace): - """Test add_comment when w:t parent is not immediately w:r. - - This tests line 115 - traversing up to find w:r. - """ + """Test add_comment when w:t parent is not immediately w:r.""" doc = Document.open(clean_workspace) - # This is a normal case - the anchor should be found and - # the parent traversal logic should work try: comment_id = doc.add_comment("fox", "Test comment") assert isinstance(comment_id, int) @@ -919,86 +919,6 @@ def test_add_comment_traverses_to_find_run(self, clean_workspace): doc.close() - def test_add_comment_no_parent_run_raises_error(self, clean_workspace): - """Test add_comment raises CommentError when no parent run found. - - This tests line 122. - """ - from unittest.mock import MagicMock, patch - - doc = Document.open(clean_workspace) - - # Mock get_node to return an element without proper parent hierarchy - mock_elem = MagicMock() - mock_elem.parentNode = None # No parent - - with patch.object(doc._document_editor, "get_node", return_value=mock_elem): - with pytest.raises(CommentError, match="Could not find parent"): - doc.add_comment("fox", "Test comment") - - doc.close() - - def test_add_comment_no_parent_para_raises_error(self, clean_workspace): - """Test add_comment raises CommentError when no parent paragraph found. - - This tests line 122 - when run exists but para doesn't. - """ - from unittest.mock import MagicMock, patch - - doc = Document.open(clean_workspace) - - # Mock get_node to return an element with run but no paragraph parent - mock_elem = MagicMock() - mock_run = MagicMock() - mock_run.nodeName = "w:r" - mock_run.parentNode = None # No paragraph parent - mock_elem.parentNode = mock_run - - with patch.object(doc._document_editor, "get_node", return_value=mock_elem): - with pytest.raises(CommentError, match="Could not find parent"): - doc.add_comment("fox", "Test comment") - - doc.close() - - def test_add_comment_intermediate_parent_traversal(self, clean_workspace): - """Test add_comment traverses through intermediate elements to find w:r. - - This tests line 115 explicitly - the while loop that traverses up. - """ - from unittest.mock import MagicMock, patch - - doc = Document.open(clean_workspace) - - # Mock get_node to return an element with an intermediate node before w:r - mock_elem = MagicMock() - mock_intermediate = MagicMock() - mock_run = MagicMock() - mock_para = MagicMock() - - mock_intermediate.nodeName = "w:someOtherElement" # Not w:r - mock_intermediate.parentNode = mock_run - - mock_run.nodeName = "w:r" - mock_run.parentNode = mock_para - - mock_para.nodeName = "w:p" - mock_para.parentNode = None - - mock_elem.parentNode = mock_intermediate # Not directly w:r - - # Mock insert_before and append_to to do nothing - with patch.object(doc._document_editor, "get_node", return_value=mock_elem): - with patch.object(doc._document_editor, "insert_before"): - with patch.object(doc._document_editor, "append_to"): - with patch.object(doc._comment_manager, "_add_to_comments_xml"): - with patch.object(doc._comment_manager, "_add_to_comments_extended_xml"): - with patch.object(doc._comment_manager, "_add_to_comments_ids_xml"): - with patch.object(doc._comment_manager, "_add_to_comments_extensible_xml"): - comment_id = doc.add_comment("fox", "Test comment") - assert isinstance(comment_id, int) - - doc.close() - class TestSplitTextNodeComment: """Issue #9: comments with multi-TEXT_NODE w:t must round-trip full text.""" @@ -1030,3 +950,256 @@ def test_list_comments_full_text_with_split_w_t(self, clean_workspace): texts = [c.text for c in comments] assert "Hello World" in texts doc.close() + + +def _split_first_run_text(paragraph, split_at: int) -> None: + """Split the first ````'s single ```` text into two runs. + + Useful for synthesizing a cross-run anchor: callers locate a paragraph + whose run contains the anchor entirely in one ``w:t``, then call this to + rebuild it as ``headtail``. + """ + run = paragraph.getElementsByTagName("w:r")[0] + wt = run.getElementsByTagName("w:t")[0] + full = "".join(c.data for c in wt.childNodes if c.nodeType == c.TEXT_NODE) + head, tail = full[:split_at], full[split_at:] + owner = paragraph.ownerDocument + + new_run_2 = owner.createElement("w:r") + new_t_2 = owner.createElement("w:t") + new_t_2.setAttribute("xml:space", "preserve") + new_t_2.appendChild(owner.createTextNode(tail)) + new_run_2.appendChild(new_t_2) + + while wt.firstChild: + wt.removeChild(wt.firstChild) + wt.appendChild(owner.createTextNode(head)) + wt.setAttribute("xml:space", "preserve") + + parent = run.parentNode + next_sib = run.nextSibling + if next_sib: + parent.insertBefore(new_run_2, next_sib) + else: + parent.appendChild(new_run_2) + + +def _find_paragraph_with_text(doc, text: str): + """Return the ```` element whose visible text contains ``text``.""" + from docx_editor.xml_editor import build_text_map + + for p in doc._document_editor.dom.getElementsByTagName("w:p"): + if text in build_text_map(p).text: + return p + raise AssertionError(f"No paragraph contains {text!r}") + + +def _paragraph_ref(doc, text: str) -> str: + """Find ``P{i}#{hash}`` for the paragraph containing ``text``.""" + for entry in doc.list_paragraphs(): + if text in entry: + return entry.split("|", 1)[0] + raise AssertionError(f"No paragraph contains {text!r}") + + +class TestCommentCrossBoundaryAnchor: + """Issue #5: ``add_comment`` must locate anchors that span run boundaries. + + Mirrors the asymmetry fix between ``count_matches`` (text-map search) and + the old ``get_node(contains=...)`` lookup, which only saw text inside a + single ````. + """ + + def test_anchor_split_across_two_runs(self, clean_workspace): + """Regression: anchor spanning two ```` is found and commented.""" + doc = Document.open(clean_workspace) + try: + para = _find_paragraph_with_text(doc, "brown fox") + full = "The quick brown fox jumps over the lazy dog." + _split_first_run_text(para, full.index("brown") + 2) # split inside "brown" + + assert doc.count_matches("brown fox") >= 1 + comment_id = doc.add_comment("brown fox", "spans runs") + assert isinstance(comment_id, int) + + # Marker pair must be present in document.xml + starts = doc._document_editor.dom.getElementsByTagName("w:commentRangeStart") + ends = doc._document_editor.dom.getElementsByTagName("w:commentRangeEnd") + assert any(s.getAttribute("w:id") == str(comment_id) for s in starts) + assert any(e.getAttribute("w:id") == str(comment_id) for e in ends) + finally: + doc.close() + + def test_anchor_across_formatting_boundary(self, clean_workspace): + """Anchor splits across two runs with different ``rPr`` (bold + plain).""" + doc = Document.open(clean_workspace) + try: + para = _find_paragraph_with_text(doc, "quick brown") + # Build a paragraph with a bold "quick" run + plain " brown" run by + # replacing the existing single run. + run = para.getElementsByTagName("w:r")[0] + new_xml = ( + 'quick' + ' brown fox jumps over the lazy dog.' + ) + doc._document_editor.replace_node(run, new_xml) + + assert doc.count_matches("quick brown") == 1 + comment_id = doc.add_comment("quick brown", "fmt boundary") + assert isinstance(comment_id, int) + finally: + doc.close() + + def test_paragraph_scoped_lookup_finds_anchor(self, clean_workspace): + """``paragraph=`` scope restricts the search and accepts the anchor.""" + doc = Document.open(clean_workspace) + try: + ref = _paragraph_ref(doc, "brown fox") + comment_id = doc.add_comment("fox", "scoped", paragraph=ref) + assert isinstance(comment_id, int) + finally: + doc.close() + + def test_paragraph_scoped_lookup_anchor_not_in_paragraph(self, clean_workspace): + """Scoped search raises ``TextNotFoundError`` when anchor lives elsewhere.""" + doc = Document.open(clean_workspace) + try: + other_ref = _paragraph_ref(doc, "sample document") + # "fox" exists in a different paragraph + with pytest.raises(TextNotFoundError) as exc_info: + doc.add_comment("fox", "wrong paragraph", paragraph=other_ref) + assert exc_info.value.paragraph_ref == other_ref + finally: + doc.close() + + def test_stale_paragraph_hash_raises(self, clean_workspace): + """Passing a hash that no longer matches raises ``HashMismatchError``.""" + doc = Document.open(clean_workspace) + try: + ref = _paragraph_ref(doc, "brown fox") + # Construct a stale ref: keep the index, mangle the hash + idx_part = ref.split("#")[0] + stale = f"{idx_part}#0000" + with pytest.raises(HashMismatchError): + doc.add_comment("fox", "stale", paragraph=stale) + finally: + doc.close() + + def test_occurrence_index_selects_match(self, clean_workspace): + """``occurrence=N`` picks the Nth match within the scoped paragraph.""" + doc = Document.open(clean_workspace) + try: + # Synthesize a paragraph with two copies of "abc" + para = _find_paragraph_with_text(doc, "brown fox") + run = para.getElementsByTagName("w:r")[0] + doc._document_editor.replace_node( + run, + 'abc middle abc tail', + ) + # Recompute ref since hash changed + ref = _paragraph_ref(doc, "abc middle") + + id0 = doc.add_comment("abc", "first", paragraph=ref, occurrence=0) + # After adding a comment, the paragraph hash changes — recompute + ref2 = _paragraph_ref(doc, "abc middle") + id1 = doc.add_comment("abc", "second", paragraph=ref2, occurrence=1) + assert id0 != id1 + + # Third occurrence does not exist + ref3 = _paragraph_ref(doc, "abc middle") + with pytest.raises(TextNotFoundError) as exc_info: + doc.add_comment("abc", "missing", paragraph=ref3, occurrence=2) + assert exc_info.value.occurrence == 2 + assert exc_info.value.total_occurrences == 2 + finally: + doc.close() + + def test_marker_placement_is_character_precise(self, clean_workspace): + """Markers bracket exactly the anchor text (no oversized range).""" + doc = Document.open(clean_workspace) + try: + comment_id = doc.add_comment("brown", "tight range") + + para = _find_paragraph_with_text(doc, "quick") + # Collect children of w:p in document order; between + # commentRangeStart and commentRangeEnd, visible text must equal + # the anchor. + in_range = False + collected: list[str] = [] + for child in para.childNodes: + if child.nodeType != child.ELEMENT_NODE: + continue + if child.tagName == "w:commentRangeStart" and child.getAttribute("w:id") == str(comment_id): + in_range = True + continue + if child.tagName == "w:commentRangeEnd" and child.getAttribute("w:id") == str(comment_id): + in_range = False + continue + if in_range and child.tagName == "w:r": + for wt in child.getElementsByTagName("w:t"): + collected.append("".join(c.data for c in wt.childNodes if c.nodeType == c.TEXT_NODE)) + assert "".join(collected) == "brown" + finally: + doc.close() + + def test_anchor_inside_tracked_insertion(self, clean_workspace): + """Anchor text inside a ```` is found and bracketed.""" + doc = Document.open(clean_workspace) + try: + # Wrap the first run of a paragraph in + para = _find_paragraph_with_text(doc, "sample document") + run = para.getElementsByTagName("w:r")[0] + new_xml = ( + '' + 'This is a sample document for testing the editing features.' + "" + ) + doc._document_editor.replace_node(run, new_xml) + + comment_id = doc.add_comment("sample", "anchor inside w:ins") + # Verify markers exist + starts = doc._document_editor.dom.getElementsByTagName("w:commentRangeStart") + ends = doc._document_editor.dom.getElementsByTagName("w:commentRangeEnd") + assert any(s.getAttribute("w:id") == str(comment_id) for s in starts) + assert any(e.getAttribute("w:id") == str(comment_id) for e in ends) + finally: + doc.close() + + def test_non_text_children_preserved(self, clean_workspace): + """```` / ```` in the anchor's run survive the split. + + Regression guard: an earlier implementation iterated + ``getElementsByTagName("w:t")`` and silently dropped sibling tabs, + breaks, and drawings when rebuilding the run. + """ + doc = Document.open(clean_workspace) + try: + para = _find_paragraph_with_text(doc, "brown fox") + run = para.getElementsByTagName("w:r")[0] + # Run with interleaved tab + break alongside the matched text + doc._document_editor.replace_node( + run, + 'The ' + 'quick brown fox' + ' jumps', + ) + + comment_id = doc.add_comment("brown", "preserves siblings") + assert isinstance(comment_id, int) + + # The tab and break must still be in the paragraph after rebuild + tabs = para.getElementsByTagName("w:tab") + brs = para.getElementsByTagName("w:br") + assert len(tabs) == 1 + assert len(brs) == 1 + finally: + doc.close() + + def test_empty_anchor_text_raises(self, clean_workspace): + """``add_comment("")`` raises ``CommentError`` rather than IndexError.""" + doc = Document.open(clean_workspace) + try: + with pytest.raises(CommentError): + doc.add_comment("", "no anchor") + finally: + doc.close() From bf182f26be3ab7ebe276a548c51dc1b96cb711e2 Mon Sep 17 00:00:00 2001 From: Pablo Speciale Date: Sat, 16 May 2026 20:28:49 +0200 Subject: [PATCH 2/5] fix(types): make _rebuild_run_with_markers narrowable by ty 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. --- docx_editor/comments.py | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/docx_editor/comments.py b/docx_editor/comments.py index 93ff1cb..6a71799 100644 --- a/docx_editor/comments.py +++ b/docx_editor/comments.py @@ -258,10 +258,10 @@ def _place_markers(self, match: TextMapMatch, comment_id: int) -> None: self._rebuild_run_with_markers( end_run, None, - None, + 0, end_node, end_offset, - None, + "", end_marker, ) self._rebuild_run_with_markers( @@ -269,27 +269,28 @@ def _place_markers(self, match: TextMapMatch, comment_id: int) -> None: start_node, start_offset, None, - None, + 0, start_marker, - None, + "", ) def _rebuild_run_with_markers( self, run: Element, start_node, - start_offset: int | None, + start_offset: int, end_node, - end_offset: int | None, - start_marker: str | None, - end_marker: str | None, + end_offset: int, + start_marker: str, + end_marker: str, ) -> None: """Rebuild ``run``'s XML, splitting ``w:t`` children to insert markers. ``start_marker`` / ``end_marker`` are inserted right before the character at ``start_offset`` of ``start_node`` and right after the - character at ``end_offset`` of ``end_node`` respectively. Pass ``None`` - for an end whose marker lives in a different run. + character at ``end_offset`` of ``end_node`` respectively. Pass an + empty string (and any int offset, e.g. ``0``) when the corresponding + marker lives in a different run. Iterates *direct* children of ``run`` (not descendants) so non-``w:t`` content like ````, ````, ````, and field @@ -313,8 +314,8 @@ def _rebuild_run_with_markers( wt = child wt_text = get_text_node_data(wt) - is_start_here = start_marker is not None and wt is start_node - is_end_here = end_marker is not None and wt is end_node + is_start_here = bool(start_marker) and wt is start_node + is_end_here = bool(end_marker) and wt is end_node if not is_start_here and not is_end_here: if wt_text: From ce912b41c09eb362d0433d4be9e608171c3f229e Mon Sep 17 00:00:00 2001 From: Pablo Speciale Date: Sat, 16 May 2026 20:50:42 +0200 Subject: [PATCH 3/5] test: regression guard for issue #14 CJK multi-run anchor Pins the run-boundary fix to vincentfyt's reported failure mode: a 29-character Chinese anchor spanning nine elements. count_matches and find_text accepted it before this branch, add_comment rejected it. --- tests/test_comments.py | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/tests/test_comments.py b/tests/test_comments.py index 5fc9672..9af54e9 100644 --- a/tests/test_comments.py +++ b/tests/test_comments.py @@ -1203,3 +1203,33 @@ def test_empty_anchor_text_raises(self, clean_workspace): doc.add_comment("", "no anchor") finally: doc.close() + + def test_anchor_spans_many_cjk_runs(self, clean_workspace): + """Regression: GitHub #14. CJK anchor spanning nine ```` runs. + + Reporter's failing case: a 29-character Chinese anchor that + ``find_text``/``count_matches`` accepted but ``add_comment`` rejected + with ``TextNotFoundError`` because the old lookup only saw text inside + a single ```` element. + """ + doc = Document.open(clean_workspace) + try: + para = _find_paragraph_with_text(doc, "brown fox") + segments = [ + "息税前", "利润率", "，", "息税前利润率", "=", + "息税前利润", "/", "营业收入", "×100%", + ] + runs_xml = "".join( + f'{seg}' for seg in segments + ) + run = para.getElementsByTagName("w:r")[0] + doc._document_editor.replace_node(run, runs_xml) + + target = "".join(segments) + assert doc.count_matches(target) == 1 + assert doc.find_text(target) is not None + + comment_id = doc.add_comment(target, "issue-14 repro") + assert isinstance(comment_id, int) + finally: + doc.close() From e84815becf7dfafa0daa4e5b3b5a09c1b4ab5f1d Mon Sep 17 00:00:00 2001 From: Pablo Speciale Date: Sat, 16 May 2026 22:24:35 +0200 Subject: [PATCH 4/5] test: cover defensive error paths in add_comment anchor lookup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 with no ancestor, asserting the CommentError guard fires. Coverage on docx_editor/comments.py goes from ~92% to 95%. --- tests/test_comments.py | 60 ++++++++++++++++++++++++++++++++++++++---- 1 file changed, 55 insertions(+), 5 deletions(-) diff --git a/tests/test_comments.py b/tests/test_comments.py index 9af54e9..4c4ef74 100644 --- a/tests/test_comments.py +++ b/tests/test_comments.py @@ -6,6 +6,7 @@ CommentError, Document, HashMismatchError, + ParagraphIndexError, TextNotFoundError, ) @@ -1216,12 +1217,17 @@ def test_anchor_spans_many_cjk_runs(self, clean_workspace): try: para = _find_paragraph_with_text(doc, "brown fox") segments = [ - "息税前", "利润率", "，", "息税前利润率", "=", - "息税前利润", "/", "营业收入", "×100%", + "息税前", + "利润率", + "，", + "息税前利润率", + "=", + "息税前利润", + "/", + "营业收入", + "×100%", ] - runs_xml = "".join( - f'{seg}' for seg in segments - ) + runs_xml = "".join(f'{seg}' for seg in segments) run = para.getElementsByTagName("w:r")[0] doc._document_editor.replace_node(run, runs_xml) @@ -1233,3 +1239,47 @@ def test_anchor_spans_many_cjk_runs(self, clean_workspace): assert isinstance(comment_id, int) finally: doc.close() + + def test_paragraph_index_out_of_range_raises(self, clean_workspace): + """``paragraph=`` with an out-of-range index raises ``ParagraphIndexError``.""" + doc = Document.open(clean_workspace) + try: + with pytest.raises(ParagraphIndexError): + doc.add_comment("fox", "out of range", paragraph="P99#abcd") + finally: + doc.close() + + def test_stale_hash_long_paragraph_truncates_preview(self, clean_workspace): + """``HashMismatchError.paragraph_preview`` truncates at 80 chars + '...'.""" + doc = Document.open(clean_workspace) + try: + # Synthesize a >80-char paragraph so the truncation branch fires + para = _find_paragraph_with_text(doc, "brown fox") + run = para.getElementsByTagName("w:r")[0] + long_text = "a" * 100 + doc._document_editor.replace_node(run, f"{long_text}") + + ref = _paragraph_ref(doc, long_text[:20]) + idx_part = ref.split("#")[0] + stale = f"{idx_part}#0000" + + with pytest.raises(HashMismatchError) as exc_info: + doc.add_comment("aaa", "stale long", paragraph=stale) + assert exc_info.value.paragraph_preview.endswith("...") + # 80 chars of body + "..." + assert len(exc_info.value.paragraph_preview) == 83 + finally: + doc.close() + + def test_orphan_wt_raises_comment_error(self, clean_workspace): + """```` without a ```` ancestor → ``CommentError``, not IndexError.""" + doc = Document.open(clean_workspace) + try: + para = _find_paragraph_with_text(doc, "brown fox") + run = para.getElementsByTagName("w:r")[0] + # Bare w:t as direct child of w:p — _find_ancestor_run will return None + doc._document_editor.replace_node(run, 'orphan anchor text') + with pytest.raises(CommentError): + doc.add_comment("orphan", "no run wrapper") + finally: + doc.close() From 3bebed235feb970ae755ef49ce73515d21923811 Mon Sep 17 00:00:00 2001 From: Pablo Speciale Date: Sun, 17 May 2026 22:54:48 +0200 Subject: [PATCH 5/5] test: cover document-wide occurrence search paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- tests/test_comments.py | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/tests/test_comments.py b/tests/test_comments.py index 4c4ef74..9a16be7 100644 --- a/tests/test_comments.py +++ b/tests/test_comments.py @@ -1283,3 +1283,38 @@ def test_orphan_wt_raises_comment_error(self, clean_workspace): doc.add_comment("orphan", "no run wrapper") finally: doc.close() + + def test_document_wide_occurrence_skips_earlier_matches(self, clean_workspace): + """Unscoped search with ``occurrence>0`` walks past earlier hits.""" + doc = Document.open(clean_workspace) + try: + # "the" appears in multiple paragraphs of simple.docx + assert doc.count_matches("the") >= 2 + comment_id = doc.add_comment("the", "second occurrence", occurrence=1) + assert isinstance(comment_id, int) + finally: + doc.close() + + def test_document_wide_occurrence_out_of_range(self, clean_workspace): + """Unscoped search with ``occurrence`` past last match reports ``total_occurrences``.""" + doc = Document.open(clean_workspace) + try: + total = doc.count_matches("fox") + with pytest.raises(TextNotFoundError) as exc_info: + doc.add_comment("fox", "out of range", occurrence=99) + assert exc_info.value.occurrence == 99 + assert exc_info.value.total_occurrences == total + finally: + doc.close() + + def test_scoped_occurrence_when_anchor_absent(self, clean_workspace): + """Scoped search with ``occurrence>0`` and zero matches raises plain ``TextNotFoundError``.""" + doc = Document.open(clean_workspace) + try: + ref = _paragraph_ref(doc, "brown fox") + with pytest.raises(TextNotFoundError) as exc_info: + doc.add_comment("nonexistent", "x", paragraph=ref, occurrence=1) + # total_occurrences must NOT be set (the 'if total > 0' fall-through path) + assert exc_info.value.total_occurrences is None + finally: + doc.close()