Skip to content

fix: add_comment finds anchors that span w:t run boundaries #21

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pablospe merged 5 commits into from
May 17, 2026
Merged

fix: add_comment finds anchors that span w:t run boundaries #21

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
57d11af
fix: add_comment finds anchors that span w:t run boundaries
pablospe May 16, 2026
bf182f2
fix(types): make _rebuild_run_with_markers narrowable by ty
pablospe May 16, 2026
ce912b4
test: regression guard for issue #14 CJK multi-run anchor
pablospe May 16, 2026
e84815b
test: cover defensive error paths in add_comment anchor lookup
pablospe May 16, 2026
3bebed2
test: cover document-wide occurrence search paths
pablospe May 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
301 changes: 273 additions & 28 deletions docx_editor/comments.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)
Expand All @@ -142,6 +160,199 @@ 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,
0,
end_node,
end_offset,
"",
end_marker,
)
self._rebuild_run_with_markers(
start_run,
start_node,
start_offset,
None,
0,
start_marker,
"",
)

def _rebuild_run_with_markers(
self,
run: Element,
start_node,
start_offset: int,
end_node,
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 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 ``<w:tab/>``, ``<w:br/>``, ``<w:drawing/>``, 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"<w:r>{rPr_xml}{child.toxml()}</w:r>")
continue

wt = child
wt_text = get_text_node_data(wt)
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:
xml_parts.append(f"<w:r>{rPr_xml}<w:t>{_escape_xml(wt_text)}</w:t></w:r>")
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"<w:r>{rPr_xml}<w:t>{_escape_xml(before)}</w:t></w:r>")
xml_parts.append(start_marker)
if matched:
xml_parts.append(f"<w:r>{rPr_xml}<w:t>{_escape_xml(matched)}</w:t></w:r>")
xml_parts.append(end_marker)
if after:
xml_parts.append(f"<w:r>{rPr_xml}<w:t>{_escape_xml(after)}</w:t></w:r>")
elif is_start_here:
before = wt_text[:start_offset]
after = wt_text[start_offset:]
if before:
xml_parts.append(f"<w:r>{rPr_xml}<w:t>{_escape_xml(before)}</w:t></w:r>")
xml_parts.append(start_marker)
if after:
xml_parts.append(f"<w:r>{rPr_xml}<w:t>{_escape_xml(after)}</w:t></w:r>")
else: # is_end_here only
before = wt_text[: end_offset + 1]
after = wt_text[end_offset + 1 :]
if before:
xml_parts.append(f"<w:r>{rPr_xml}<w:t>{_escape_xml(before)}</w:t></w:r>")
xml_parts.append(end_marker)
if after:
xml_parts.append(f"<w:r>{rPr_xml}<w:t>{_escape_xml(after)}</w:t></w:r>")

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.

Expand Down Expand Up @@ -497,3 +708,37 @@ def _comment_ref_run_xml(self, comment_id: int) -> str:
<w:rPr><w:rStyle w:val="CommentReference"/></w:rPr>
<w:commentReference w:id="{comment_id}"/>
</w:r>"""


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("&", "&amp;")
.replace("<", "&lt;")
.replace(">", "&gt;")
.replace('"', "&quot;")
.replace("'", "&apos;")
)
Comment on lines +735 to +744

@coderabbitai coderabbitai Bot May 16, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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 "&apos;" 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.

26 changes: 21 additions & 5 deletions docx_editor/document.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
Loading
Loading