Repository navigation
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughConsolidates text handling for minidom-split ChangesSplit Text Node Handling Fix
Sequence DiagramsequenceDiagram
participant Client
participant replace_text
participant _get_node_text
participant w_t_element
participant _set_node_text
Client->>replace_text: replace_text(search, replacement)
replace_text->>_get_node_text: request full text
_get_node_text->>w_t_element: read all TEXT_NODE children
w_t_element-->>_get_node_text: concatenated text
_get_node_text-->>replace_text: merged text
replace_text->>_set_node_text: provide updated text
_set_node_text->>w_t_element: remove TEXT_NODE children, append single TEXT_NODE
_set_node_text-->>replace_text: done
replace_text-->>Client: operation complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #16 +/- ##
=======================================
+ Coverage 93.8% 94.0% +0.1%
=======================================
Files 10 10
Lines 2175 2173 -2
Branches 492 485 -7
=======================================
+ Hits 2042 2044 +2
Misses 34 34
+ Partials 99 95 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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/track_changes.py`:
- Around line 1070-1076: The branches that update text on a single node (the
elif not before_text, elif not after_text, and the "Middle split" branch that
call _set_node_text on first_node) do not call _set_xml_space_preserve, so
leading/trailing whitespace may be collapsed; after each _set_node_text call in
those branches (the calls that target first_node in track_changes.py) add a call
to _set_xml_space_preserve(first_node, <the_text_you_set>) to match the
multi-node case behavior and preserve whitespace.
🪄 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: 7114b830-1989-42d3-8beb-ec484b73b87a
📒 Files selected for processing (8)
docx_editor/comments.pydocx_editor/ooxml/pack.pydocx_editor/track_changes.pydocx_editor/xml_editor.pytests/test_comments.pytests/test_ooxml.pytests/test_track_changes.pytests/test_xml_editor.py
minidom can split a single <w:t> element's text across multiple TEXT_NODE children — Word documents with smart quotes (U+2018/U+2019) have been reported to land in this state. The read paths in replace_text, suggest_deletion, _insert_text, and _parse_revision only consulted firstChild.data, so any text adjacent to a smart quote raised TextNotFoundError. The corresponding write paths assigned to firstChild.data, which silently corrupted the remaining sibling text nodes. Add a _set_node_text helper that consolidates children to a single TEXT_NODE, document _get_node_text, and route all firstChild.data text reads/writes through these helpers. Element-level firstChild traversal (unwrap, restore deletion, ins-insertion point lookup) is unchanged. Tests cover the buggy code paths directly (paragraph=None branches and the cross-boundary in-ins path) and round-trip a real .docx built with smart-quote content through open → edit → save → reopen.
Code review (gemini reviewer) flagged that the same firstChild.data bug class exists outside track_changes.py: - xml_editor.add_xml_space_to_t: inspected only firstChild.data when deciding whether to add xml:space="preserve". For a <w:t> split into multiple TEXT_NODE children, trailing/leading whitespace sitting in a later fragment was missed, so Word would collapse those spaces. - comments._parse_comment: appended only firstChild.data to text_parts when reading comment text. Comment bodies containing characters that trigger node splitting (e.g. smart quotes) were returned truncated. Both sites now concatenate all TEXT_NODE children of each <w:t>, matching the approach already used by RevisionManager._get_node_text. Tests construct the split-text-node state directly and verify the read paths return the full content.
Adversarial review iteration 2 surfaced two more second-order consequences of issue #9 (minidom can split a single <w:t> across multiple TEXT_NODE children). - ooxml.pack.condense_xml only skipped elements with tag suffix ":t". Tracked deletions (w:delText) and field instructions (w:instrText) were unprotected, so a whitespace-only TEXT_NODE created by a split would be stripped on save, corrupting tracked changes that contain spaces near smart quotes. Extend the suffix check to cover both. - XMLEditor._get_element_text unconditionally dropped any whitespace- only TEXT_NODE. That's fine for inter-element pretty-print whitespace, but it also dropped the lone-space fragment of a split <w:t> like ["Library", " ", "Bookshelves"], silently breaking the `contains=` search used by get_node/find_all_nodes. Only skip a whitespace TEXT_NODE when it has no meaningful-text TEXT_NODE sibling.
The disjunct in test_condense_preserves_whitespace_in_delText was a near-tautology — the right-hand side evaluated True whenever the space had been stripped, so the test would have silently passed against a regression. The stricter form fails fast if the space disappears from the repacked archive. Skipped on this iteration: - xml_editor._get_element_text edge case where a lone whitespace-only TEXT_NODE child is dropped (pre-existing behavior, not introduced by this PR; xml:space="preserve" guard would be the proper fix) - pytest.skip pattern in TestSplitTextNodeComment (matches the rest of test_comments.py — consistent with project style)
…#9) CodeRabbit flagged an asymmetry: the multi-node branch of _remove_from_insertion pairs every _set_node_text() write with _set_xml_space_preserve(), but the single-node branch did not. If the surviving text after a deletion has leading or trailing whitespace (common when only part of an insertion is removed), Word would silently collapse it on render. Aligning the single-node branch with the multi-node pattern.
Addresses the three LOW items the previous reviewer pass flagged but that had been deferred: * Extract the "concat all TEXT_NODE children" pattern into a single module-level helper `xml_editor.get_text_node_data`. Replace the four inline copies (build_text_map, add_xml_space_to_t, _parse_comment, RevisionManager._get_node_text) with calls into it. The method on RevisionManager stays as a thin delegate so call sites and any external subclasses remain unchanged. * `_get_element_text` now treats a whitespace-only TEXT_NODE child as content when its parent carries `xml:space="preserve"`, not only when it sits alongside a meaningful-text sibling. Closes the edge case where a lone-space `<w:t xml:space="preserve"> </w:t>` was incorrectly returned as the empty string by `contains=` search. * New focused unit test for `_set_node_text` asserts the contract directly: starting from a multi-TEXT_NODE state, ends with exactly one TEXT_NODE carrying the new content. Catches future "simplifications" that re-introduce the `firstChild.data =` pattern without an integration regression. * New test covering the `xml:space="preserve"` branch of `_get_element_text`.
Owner
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
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.
Fixes #9.
Summary
replace_text/suggest_deletion/_insert_text/_parse_revisionintrack_changes.pyno longer rely onfirstChild.datafor w:t reads or writes — a new_set_node_texthelper consolidates split TEXT_NODE children and_get_node_textalready concatenates them.comments._parse_comment(truncated comment text),xml_editor.add_xml_space_to_t(missed leading/trailing whitespace in non-first fragments), andooxml.pack.condense_xml(stripped lone-space TEXT_NODEs from w:delText/w:instrText).XMLEditor._get_element_textnow only skips whitespace TEXT_NODEs when they're pretty-print indentation, so thecontains=search returns hits for documents where a w:t is split such that a space sits alone in its own TEXT_NODE.Root cause
xml.dom.minidomcan split the text of a single<w:t>element across multiple TEXT_NODE children — reportedly when smart quotes (U+2018, U+2019) are present. Code reading onlyfirstChild.datareturned just the first fragment (theTextNotFoundError), and code writing tofirstChild.dataleft the remaining sibling text nodes in place (silently corrupting the document).Test plan
TestMultiTextNodeWtElementscovers the previously-brokenparagraph=Nonepaths in replace/delete/insert plus the in-w:inscross-boundary write path.TestSmartQuoteEndToEndbuilds a real.docxcontaining U+2018/U+2019, exercises the full open → edit → save → reopen cycle, and asserts the smart quotes survive.TestSplitTextNodeCommentverifieslist_commentsreturns full text when a comment w:t is split.test_inject_xml_space_for_split_text_nodesverifiesxml:space="preserve"is added when whitespace sits in a later TEXT_NODE.TestCondenseXmlPreservesTextContentverifies tracked-deletion and field-instruction whitespace survives pack.test_get_element_text_preserves_split_whitespaceverifiescontains=search works against a split w:t.uv run pytest→ 507 passed, 6 skipped.uv run ruff checkandruff formatclean.Review process
Local multi-reviewer pass (claude opus + gemini + codex) ran twice:
comments.pyandxml_editor.py:add_xml_space_to_tas HIGH issues outside the originally-reported file.ooxml/pack.py:condense_xml(CRITICAL: w:delText/w:instrText unprotected) and_get_element_textwhitespace stripping (HIGH: broke contains= search).Summary by CodeRabbit
Bug Fixes
Tests