Repository navigation
fix: run-level w:rPr must not leak from drawing text boxes (ISSUES.md #31) - #47
Conversation
…31) - Replace three descendant-scanning getElementsByTagName("w:rPr") sites in track_changes.py with new get_rPr_xml() (direct children only), so a w:rPr nested in a drawing's text box no longer leaks into rebuilt outer runs. - Consolidate duplicated helpers into xml_editor.py: _escape_xml (was defined twice) and new render_plain_wt() replacing five copy-pasted preserve-plain-w:t branches across the render_wt closures. - _build_style_outline_map: a w:style without w:type now defaults to paragraph per ECMA-376 instead of being skipped. - Tests: nested-rPr no-leak regression (replace + empty-paragraph insert paths), unit tests for the new helpers, typeless-style outline resolution.
|
Warning Review limit reached
Next review available in: 48 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughShared XML helpers now handle run properties, escaping, and plain text rendering. Comment and tracked-change reconstruction use them, typeless paragraph styles resolve correctly, and tests cover rendering and nested property behavior. ChangesXML rendering and document reconstruction
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/xml_editor.py`:
- Around line 665-674: Update render_plain_wt to preserve the original w:t
element attributes by reusing wt.toxml() instead of manually constructing the
element from escaped text. Keep the existing empty-text behavior and rPr_xml
wrapper unchanged.
- Around line 641-650: Replace the custom replacement chain in _escape_xml with
xml.sax.saxutils.escape, adding the standard-library import at the module level
and preserving the function’s text-to-escaped-XML behavior.
🪄 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: 52eec353-0de5-4976-af94-30e34ba3d906
📒 Files selected for processing (6)
docx_editor/comments.pydocx_editor/track_changes.pydocx_editor/xml_editor.pytests/test_paragraph_location.pytests/test_run_child_order.pytests/test_xml_editor.py
| 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.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Use xml.sax.saxutils.escape for XML escaping.
The custom string replacement chain works correctly, but Python's standard library provides this exact functionality. Prefer using standard libraries over rolling custom string replacement logic.
♻️ Proposed refactor
Add the import at the top of the file:
import xml.sax.saxutilsThen replace the function:
def _escape_xml(text: str) -> str:
"""Escape text for safe XML inclusion."""
- return (
- text
- .replace("&", "&")
- .replace("<", "<")
- .replace(">", ">")
- .replace('"', """)
- .replace("'", "'")
- )
+ return xml.sax.saxutils.escape(text, {'"': """, "'": "'"})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _escape_xml(text: str) -> str: | |
| """Escape text for safe XML inclusion.""" | |
| return ( | |
| text | |
| .replace("&", "&") | |
| .replace("<", "<") | |
| .replace(">", ">") | |
| .replace('"', """) | |
| .replace("'", "'") | |
| ) | |
| def _escape_xml(text: str) -> str: | |
| """Escape text for safe XML inclusion.""" | |
| return xml.sax.saxutils.escape(text, {'"': """, "'": "'"}) |
🤖 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/xml_editor.py` around lines 641 - 650, Replace the custom
replacement chain in _escape_xml with xml.sax.saxutils.escape, adding the
standard-library import at the module level and preserving the function’s
text-to-escaped-XML behavior.
| def render_plain_wt(wt, rPr_xml: str) -> list[str]: | ||
| """Render an unmatched ``w:t`` node as a plain run fragment carrying ``rPr_xml``. | ||
|
|
||
| The shared preserve-as-is step for ``render_wt`` callbacks passed to | ||
| :func:`rebuild_run_fragments` — nodes with no text yield no fragment. | ||
| """ | ||
| wt_text = get_text_node_data(wt) | ||
| if not wt_text: | ||
| return [] | ||
| return [f"<w:r>{rPr_xml}<w:t>{_escape_xml(wt_text)}</w:t></w:r>"] |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
Preserve <w:t> attributes by using toxml().
Manually rebuilding the <w:t> element drops any original attributes it carried, such as xml:space="preserve". Although DocxXMLEditor attempts to automatically restore xml:space during insertion, using wt.toxml() is simpler and more robust because it inherently preserves all original attributes while natively handling XML text escaping.
♻️ Proposed refactor
def render_plain_wt(wt, rPr_xml: str) -> list[str]:
"""Render an unmatched ``w:t`` node as a plain run fragment carrying ``rPr_xml``.
The shared preserve-as-is step for ``render_wt`` callbacks passed to
:func:`rebuild_run_fragments` — nodes with no text yield no fragment.
"""
- wt_text = get_text_node_data(wt)
- if not wt_text:
+ if not get_text_node_data(wt):
return []
- return [f"<w:r>{rPr_xml}<w:t>{_escape_xml(wt_text)}</w:t></w:r>"]
+ return [f"<w:r>{rPr_xml}{wt.toxml()}</w:r>"]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def render_plain_wt(wt, rPr_xml: str) -> list[str]: | |
| """Render an unmatched ``w:t`` node as a plain run fragment carrying ``rPr_xml``. | |
| The shared preserve-as-is step for ``render_wt`` callbacks passed to | |
| :func:`rebuild_run_fragments` — nodes with no text yield no fragment. | |
| """ | |
| wt_text = get_text_node_data(wt) | |
| if not wt_text: | |
| return [] | |
| return [f"<w:r>{rPr_xml}<w:t>{_escape_xml(wt_text)}</w:t></w:r>"] | |
| def render_plain_wt(wt, rPr_xml: str) -> list[str]: | |
| """Render an unmatched ``w:t`` node as a plain run fragment carrying ``rPr_xml``. | |
| The shared preserve-as-is step for ``render_wt`` callbacks passed to | |
| :func:`rebuild_run_fragments` — nodes with no text yield no fragment. | |
| """ | |
| if not get_text_node_data(wt): | |
| return [] | |
| return [f"<w:r>{rPr_xml}{wt.toxml()}</w:r>"] |
🤖 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/xml_editor.py` around lines 665 - 674, Update render_plain_wt to
preserve the original w:t element attributes by reusing wt.toxml() instead of
manually constructing the element from escaped text. Keep the existing
empty-text behavior and rPr_xml wrapper unchanged.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #47 +/- ##
=====================================
Coverage 95.2% 95.3%
=====================================
Files 11 11
Lines 3210 3192 -18
Branches 657 649 -8
=====================================
- Hits 3059 3045 -14
+ Misses 70 69 -1
+ Partials 81 78 -3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
- tests/test_xml_editor.py: _parse_run now reuses conftest.parse_paragraph instead of duplicating its minidom body (drops local defusedxml/NS imports). - tests/test_run_child_order.py: rename _has_direct_rpr -> _has_direct_rPr to match the codebase's rPr spelling (get_rPr_xml, run_rPr, TestNestedRPrNoLeak). - xml_editor.py: _build_style_outline_map docstring now reflects that a missing w:type defaults to paragraph per ECMA-376.
…S.md #47) Replace operations now trim words shared by find/replace_with at either end, so only the changed words become revisions. A replace that only adds or only removes words degenerates into a pure insertion or deletion; replacing text with itself is a no-op (EditResult keeps the input ref with group_id=None, revision_ids=()). The replacement insertion carries the rPr of the run that contributed the most characters to the trimmed span (ties break to the earliest run), replacing the old first-run rule at all four insertion sites: same-context, own-ins, foreign-ins, and mixed-state. TextMapMatch.narrowed() trims a match span and recomputes spans_boundary with the same rule as find_in_text_map, so trimmed matches dispatch to the same code path a direct search would.
…S.md #47) (#66) * feat: word-level trimming + majority-rPr formatting on replace (ISSUES.md #47) Replace operations now trim words shared by find/replace_with at either end, so only the changed words become revisions. A replace that only adds or only removes words degenerates into a pure insertion or deletion; replacing text with itself is a no-op (EditResult keeps the input ref with group_id=None, revision_ids=()). The replacement insertion carries the rPr of the run that contributed the most characters to the trimmed span (ties break to the earliest run), replacing the old first-run rule at all four insertion sites: same-context, own-ins, foreign-ins, and mixed-state. TextMapMatch.narrowed() trims a match span and recomputes spans_boundary with the same rule as find_in_text_map, so trimmed matches dispatch to the same code path a direct search would. * docs: majority-rPr wording — formatting covers most characters, not single run The tally groups by serialized rPr string, so runs sharing identical formatting count together; two same-rPr runs can jointly out-total a larger differently-formatted run. Say "the formatting that covers the most characters" instead of "the run that contributed the most characters" everywhere the rule is documented.
1 participant
Summary
Fixes the rPr correctness bug tracked as ISSUES.md #31 (repo-local issue list — not GitHub issue 31) plus the render_wt dedup that rode along with it.
Bug fix
run.getElementsByTagName("w:rPr")searches all descendants, so when an outer run had now:rPrbut contained aw:drawingwhose text-box run did, the inner run's formatting leaked into rebuilt fragments. Three sites intrack_changes.py(_get_run_info, the_rewrite_insert_atempty-paragraph branch, the foreign-w:insmiddle-split) now use the newget_rPr_xml(), which iterates direct children only.Consolidation
_escape_xmlwas defined verbatim in bothtrack_changes.pyandcomments.py— moved toxml_editor.py, re-imported (existing test imports keep working).render_plain_wt()replaces five copy-pasted preserve-plain-w:tbranches across therender_wtclosures in both modules.comments.py::_get_rPr_xmlpromoted toxml_editor.py::get_rPr_xml(it was already the correct reference implementation).Behavior change (intended, spec-conformant)
_build_style_outline_mapnow treats aw:stylewith now:typeattribute as a paragraph style (ECMA-376 default) instead of skipping it, so typeless styles carryingw:outlineLvlnow contribute outline levels.Net −41 lines in
docx_editor/, +148 lines of tests. Target release: 0.5.1 (no version bump here).Tests
TestNestedRPrNoLeak— regression coverage for both bug paths (replace via_get_run_info, insert into a visually-empty paragraph); all new behavior tests fail against the old code.TestRunFragmentHelpers— unit tests forget_rPr_xml/render_plain_wt(direct child / none / descendant-only, empty node, escaping).TestStyleMissingTypeDefaultsToParagraph— typeless style resolvesoutline_level.ruff check/ruff format --checkclean.ty checkexit 0, diagnostics byte-identical to baseline.Summary by CodeRabbit
Bug Fixes
Tests