Repository navigation
fix: preserve run-internal order of tabs/breaks/drawings in tracked edits (ISSUES.md #20) - #46
Merged
Conversation
Non-text run children (w:tab, w:br, w:drawing, field chars) were front-hoisted into a single run emitted before all text parts whenever a tracked replace/delete/insert rebuilt a run, reordering document content. - Add rebuild_run_fragments() to xml_editor: walks a run's direct children in document order, preserves non-text children in place, and delegates each w:t to a caller-supplied renderer. - Route all rebuild sites through it: _replace_same_context, _delete_regular_segment, _delete_same_context, _insert_near_match, and the comment-marker rebuild in comments.py. - _split_foreign_ins_at keeps its own ordered walk (children must land on the correct side of the split) instead of front-hoisting. - Drop the now-unused _get_non_text_children_xml helper. - Side effect: w:t nested inside a drawing's text box is no longer duplicated as a top-level run. Adds tests/test_run_child_order.py (19 ordered-token tests) plus two split-side tests in test_foreign_ins_edits.py.
…me, fixture docstring Multi-reviewer pass verdicts: correctness/dead-code/codex CLEAN; three cosmetic findings confirmed by red-team meta-review and applied: - annotate rebuild_run_fragments' callback as Callable[[Element], list[str]] - rename the _delete_regular_segment closure param wt_node -> wt to match the four sibling render_wt closures - add the conventional temp_xml fixture docstring in test_run_child_order.py
📝 WalkthroughWalkthroughThis change adds a shared helper for rebuilding WordprocessingML runs, updates comment and tracked-change transformations to use it, and adds regression tests covering ordering of text, tabs, breaks, fields, drawings, and tracked-change markers. ChangesRun Fragment Ordering
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #46 +/- ##
=======================================
+ Coverage 94.8% 95.2% +0.4%
=======================================
Files 11 11
Lines 3217 3210 -7
Branches 664 657 -7
=======================================
+ Hits 3051 3059 +8
+ Misses 77 70 -7
+ Partials 89 81 -8 ☔ View full report in Codecov by Harness. 🚀 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docx_editor/track_changes.py (1)
822-833: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winLook up
w:rPrfrom direct children only
run.getElementsByTagName("w:rPr")scans descendants, so a nested drawing/text box can supply the wrong formatting for the outer run and leak it into rebuilt fragments. Iterate the run’s direct children here, ascomments.py::_get_rPr_xmlalready does.🤖 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/track_changes.py` around lines 822 - 833, Update _get_run_info to find w:rPr only among run’s direct children, matching comments.py::_get_rPr_xml, instead of using run.getElementsByTagName("w:rPr"). Preserve the existing empty-string fallback and return values while preventing nested content from supplying the outer run’s formatting.
🤖 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 615-639: The plain w:t preservation logic is duplicated across
five render_wt closures. In docx_editor/xml_editor.py lines 615-639, add
render_plain_wt beside rebuild_run_fragments to return no fragments for empty
text and otherwise produce one escaped, rPr-wrapped run fragment; update the
callers in docx_editor/track_changes.py lines 1006-1040, 1479-1523, 1571-1590,
and 1720-1742, plus docx_editor/comments.py lines 289-354, to use this helper in
their unmatched/plain preservation branches.
In `@tests/test_foreign_ins_edits.py`:
- Around line 391-440: Strengthen the assertions in
test_insert_mid_run_tab_between_texts_stays_in_order and
test_insert_mid_run_split_skips_rpr_and_empty_wt so they verify the w:tab child
appears before the w:t containing “world” within a_ins[1]. Inspect child-node
order or compare serialized-node positions while preserving the existing text,
count, and formatting checks.
---
Outside diff comments:
In `@docx_editor/track_changes.py`:
- Around line 822-833: Update _get_run_info to find w:rPr only among run’s
direct children, matching comments.py::_get_rPr_xml, instead of using
run.getElementsByTagName("w:rPr"). Preserve the existing empty-string fallback
and return values while preventing nested content from supplying the outer run’s
formatting.
🪄 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: b82a6911-c50f-4b5b-b9a9-41259ebf0c88
📒 Files selected for processing (5)
docx_editor/comments.pydocx_editor/track_changes.pydocx_editor/xml_editor.pytests/test_foreign_ins_edits.pytests/test_run_child_order.py
Comment on lines
+615
to
+639
| def rebuild_run_fragments(run, rPr_xml: str, render_wt: Callable[[Element], list[str]]) -> list[str]: | ||
| """Rebuild ``run``'s children as XML fragments, preserving document order. | ||
|
|
||
| Iterates *direct* children of ``run`` (not descendants, so ``w:t`` nodes | ||
| nested inside a drawing's text box stay inside the drawing): | ||
|
|
||
| - ``w:rPr`` is skipped — callers bake ``rPr_xml`` into every fragment. | ||
| - Non-text children (``w:tab``, ``w:br``, ``w:drawing``, field chars, …) | ||
| are preserved in place, each wrapped in its own run carrying ``rPr_xml``. | ||
| - Each direct ``w:t`` child is replaced by ``render_wt(wt)``'s fragments. | ||
| """ | ||
| fragments: list[str] = [] | ||
| for child in run.childNodes: | ||
| if child.nodeType != child.ELEMENT_NODE: | ||
| continue | ||
| tag = getattr(child, "tagName", "") | ||
| if tag == "w:rPr": | ||
| continue | ||
| if tag != "w:t": | ||
| fragments.append(f"<w:r>{rPr_xml}{child.toxml()}</w:r>") | ||
| continue | ||
| fragments.extend(render_wt(child)) | ||
| return fragments | ||
|
|
||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Extract the repeated "preserve plain w:t fragment" snippet into one helper next to rebuild_run_fragments. The same 3-line pattern (get text, skip if empty, wrap as <w:r>{rPr_xml}<w:t>{escaped}</w:t></w:r>) is copy-pasted into five different render_wt closures across two files, all built specifically to feed rebuild_run_fragments.
docx_editor/xml_editor.py#L615-L639: add a helper, e.g.def render_plain_wt(wt_text: str, rPr_xml: str) -> list[str], alongsiderebuild_run_fragmentsfor all callers to share.docx_editor/track_changes.py#L1006-L1040: replace the "Unmatched sibling — preserve" branch in_replace_same_context'srender_wtwith a call to the new helper.docx_editor/track_changes.py#L1479-L1523: replace the "Unmatched sibling — preserve as-is" branch in_delete_regular_segment'srender_wt.docx_editor/track_changes.py#L1571-L1590: replace the "Unmatched sibling — preserve" branch in_delete_same_context'srender_wt.docx_editor/track_changes.py#L1720-L1742: replace the "Preserve unmatched sibling w:t" branch in_insert_near_match'srender_wt.docx_editor/comments.py#L289-L354: replace thenot is_start_here and not is_end_herebranch in_rebuild_run_with_markers'srender_wt.
♻️ Proposed shared helper
# docx_editor/xml_editor.py
def render_plain_wt(wt_text: str, rPr_xml: str) -> list[str]:
"""Wrap unmatched/plain w:t text as a single run fragment, or [] if empty."""
if not wt_text:
return []
return [f"<w:r>{rPr_xml}<w:t>{_escape_xml(wt_text)}</w:t></w:r>"]📍 Affects 3 files
docx_editor/xml_editor.py#L615-L639(this comment)docx_editor/track_changes.py#L1006-L1040docx_editor/track_changes.py#L1479-L1523docx_editor/track_changes.py#L1571-L1590docx_editor/track_changes.py#L1720-L1742docx_editor/comments.py#L289-L354
🤖 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 615 - 639, The plain w:t preservation
logic is duplicated across five render_wt closures. In docx_editor/xml_editor.py
lines 615-639, add render_plain_wt beside rebuild_run_fragments to return no
fragments for empty text and otherwise produce one escaped, rPr-wrapped run
fragment; update the callers in docx_editor/track_changes.py lines 1006-1040,
1479-1523, 1571-1590, and 1720-1742, plus docx_editor/comments.py lines 289-354,
to use this helper in their unmatched/plain preservation branches.
Comment on lines
+391
to
+440
| def test_insert_mid_run_tab_between_texts_stays_in_order(self, temp_xml): | ||
| """A w:tab between two w:t nodes lands on the correct side of the split. | ||
|
|
||
| Splitting A's ins after "Hello" must leave the tab (which follows the | ||
| split point) in A's *second* ins, before "world" — not front-hoisted | ||
| into the first (issue #20). | ||
| """ | ||
| xml_path = temp_xml(f"<w:p>{_foreign_ins('<w:r><w:t>Hello</w:t><w:tab/><w:t>world</w:t></w:r>')}</w:p>") | ||
| manager = _make_manager(xml_path) | ||
|
|
||
| manager.insert_text_after("Hello", "X") | ||
|
|
||
| _assert_no_nested_ins(manager) | ||
| assert _visible_text(manager) == "HelloXworld" | ||
| ins_elems = _ins_elems(manager) | ||
| a_ins = [e for e in ins_elems if e.getAttribute("w:author") == AUTHOR_A] | ||
| b_ins = [e for e in ins_elems if e.getAttribute("w:author") == AUTHOR_B] | ||
| assert len(a_ins) == 2 | ||
| assert len(b_ins) == 1 | ||
| # B's ins sits between the two A halves in document order | ||
| assert [e.getAttribute("w:author") for e in ins_elems] == [AUTHOR_A, AUTHOR_B, AUTHOR_A] | ||
| # First A half: just "Hello", no tab | ||
| assert _ins_visible_text(a_ins[0]) == "Hello" | ||
| assert len(a_ins[0].getElementsByTagName("w:tab")) == 0 | ||
| # Second A half: the tab followed by "world" | ||
| assert _ins_visible_text(a_ins[1]) == "world" | ||
| assert len(a_ins[1].getElementsByTagName("w:tab")) == 1 | ||
| assert _ins_visible_text(b_ins[0]) == "X" | ||
|
|
||
| def test_insert_mid_run_split_skips_rpr_and_empty_wt(self, temp_xml): | ||
| """The split walk skips w:rPr, whitespace text nodes, and empty w:t | ||
| children while keeping formatting on both rebuilt halves.""" | ||
| run_xml = "<w:r><w:rPr><w:b/></w:rPr><w:t>Hello</w:t> <w:t></w:t><w:tab/><w:t>world</w:t></w:r>" | ||
| xml_path = temp_xml(f"<w:p>{_foreign_ins(run_xml)}</w:p>") | ||
| manager = _make_manager(xml_path) | ||
|
|
||
| manager.insert_text_after("Hello", "X") | ||
|
|
||
| _assert_no_nested_ins(manager) | ||
| assert _visible_text(manager) == "HelloXworld" | ||
| a_ins = [e for e in _ins_elems(manager) if e.getAttribute("w:author") == AUTHOR_A] | ||
| assert len(a_ins) == 2 | ||
| assert _ins_visible_text(a_ins[0]) == "Hello" | ||
| assert len(a_ins[0].getElementsByTagName("w:tab")) == 0 | ||
| assert _ins_visible_text(a_ins[1]) == "world" | ||
| assert len(a_ins[1].getElementsByTagName("w:tab")) == 1 | ||
| # Formatting survives the rebuild on both sides of the split | ||
| for ins in a_ins: | ||
| assert ins.getElementsByTagName("w:b") | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
New tests don't actually assert the tab lands before "world" inside a_ins[1].
Both new tests check _ins_visible_text(a_ins[1]) == "world" plus len(a_ins[1].getElementsByTagName("w:tab")) == 1, but neither checks the tab's position relative to the w:t node. Since _ins_visible_text ignores tabs and getElementsByTagName is order-agnostic, a regression that reorders "world" ahead of the tab inside the same <w:ins> half would still pass these assertions — even though the docstring on lines 392-397 explicitly claims to guard against exactly that ("must leave the tab … in A's second ins, before 'world'").
Consider asserting on child order directly, e.g. via each run's childNodes tag sequence or a toxml() index comparison, to make the regression guard match the stated intent.
✅ Example strengthened assertion for `a_ins[1]`
assert _ins_visible_text(a_ins[1]) == "world"
assert len(a_ins[1].getElementsByTagName("w:tab")) == 1
+ # Verify the tab precedes "world" in document order, not just that both exist
+ run_children = [c for c in a_ins[1].getElementsByTagName("w:r")[0].childNodes if c.nodeType == c.ELEMENT_NODE]
+ tags = [c.tagName for c in run_children if c.tagName != "w:rPr"]
+ assert tags.index("w:tab") < tags.index("w:t")📝 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.
Suggested change
| def test_insert_mid_run_tab_between_texts_stays_in_order(self, temp_xml): | |
| """A w:tab between two w:t nodes lands on the correct side of the split. | |
| Splitting A's ins after "Hello" must leave the tab (which follows the | |
| split point) in A's *second* ins, before "world" — not front-hoisted | |
| into the first (issue #20). | |
| """ | |
| xml_path = temp_xml(f"<w:p>{_foreign_ins('<w:r><w:t>Hello</w:t><w:tab/><w:t>world</w:t></w:r>')}</w:p>") | |
| manager = _make_manager(xml_path) | |
| manager.insert_text_after("Hello", "X") | |
| _assert_no_nested_ins(manager) | |
| assert _visible_text(manager) == "HelloXworld" | |
| ins_elems = _ins_elems(manager) | |
| a_ins = [e for e in ins_elems if e.getAttribute("w:author") == AUTHOR_A] | |
| b_ins = [e for e in ins_elems if e.getAttribute("w:author") == AUTHOR_B] | |
| assert len(a_ins) == 2 | |
| assert len(b_ins) == 1 | |
| # B's ins sits between the two A halves in document order | |
| assert [e.getAttribute("w:author") for e in ins_elems] == [AUTHOR_A, AUTHOR_B, AUTHOR_A] | |
| # First A half: just "Hello", no tab | |
| assert _ins_visible_text(a_ins[0]) == "Hello" | |
| assert len(a_ins[0].getElementsByTagName("w:tab")) == 0 | |
| # Second A half: the tab followed by "world" | |
| assert _ins_visible_text(a_ins[1]) == "world" | |
| assert len(a_ins[1].getElementsByTagName("w:tab")) == 1 | |
| assert _ins_visible_text(b_ins[0]) == "X" | |
| def test_insert_mid_run_split_skips_rpr_and_empty_wt(self, temp_xml): | |
| """The split walk skips w:rPr, whitespace text nodes, and empty w:t | |
| children while keeping formatting on both rebuilt halves.""" | |
| run_xml = "<w:r><w:rPr><w:b/></w:rPr><w:t>Hello</w:t> <w:t></w:t><w:tab/><w:t>world</w:t></w:r>" | |
| xml_path = temp_xml(f"<w:p>{_foreign_ins(run_xml)}</w:p>") | |
| manager = _make_manager(xml_path) | |
| manager.insert_text_after("Hello", "X") | |
| _assert_no_nested_ins(manager) | |
| assert _visible_text(manager) == "HelloXworld" | |
| a_ins = [e for e in _ins_elems(manager) if e.getAttribute("w:author") == AUTHOR_A] | |
| assert len(a_ins) == 2 | |
| assert _ins_visible_text(a_ins[0]) == "Hello" | |
| assert len(a_ins[0].getElementsByTagName("w:tab")) == 0 | |
| assert _ins_visible_text(a_ins[1]) == "world" | |
| assert len(a_ins[1].getElementsByTagName("w:tab")) == 1 | |
| # Formatting survives the rebuild on both sides of the split | |
| for ins in a_ins: | |
| assert ins.getElementsByTagName("w:b") | |
| def test_insert_mid_run_tab_between_texts_stays_in_order(self, temp_xml): | |
| """A w:tab between two w:t nodes lands on the correct side of the split. | |
| Splitting A's ins after "Hello" must leave the tab (which follows the | |
| split point) in A's *second* ins, before "world" — not front-hoisted | |
| into the first (issue `#20`). | |
| """ | |
| xml_path = temp_xml(f"<w:p>{_foreign_ins('<w:r><w:t>Hello</w:t><w:tab/><w:t>world</w:t></w:r>')}</w:p>") | |
| manager = _make_manager(xml_path) | |
| manager.insert_text_after("Hello", "X") | |
| _assert_no_nested_ins(manager) | |
| assert _visible_text(manager) == "HelloXworld" | |
| ins_elems = _ins_elems(manager) | |
| a_ins = [e for e in ins_elems if e.getAttribute("w:author") == AUTHOR_A] | |
| b_ins = [e for e in ins_elems if e.getAttribute("w:author") == AUTHOR_B] | |
| assert len(a_ins) == 2 | |
| assert len(b_ins) == 1 | |
| # B's ins sits between the two A halves in document order | |
| assert [e.getAttribute("w:author") for e in ins_elems] == [AUTHOR_A, AUTHOR_B, AUTHOR_A] | |
| # First A half: just "Hello", no tab | |
| assert _ins_visible_text(a_ins[0]) == "Hello" | |
| assert len(a_ins[0].getElementsByTagName("w:tab")) == 0 | |
| # Second A half: the tab followed by "world" | |
| assert _ins_visible_text(a_ins[1]) == "world" | |
| assert len(a_ins[1].getElementsByTagName("w:tab")) == 1 | |
| # Verify the tab precedes "world" in document order, not just that both exist | |
| run_children = [c for c in a_ins[1].getElementsByTagName("w:r")[0].childNodes if c.nodeType == c.ELEMENT_NODE] | |
| tags = [c.tagName for c in run_children if c.tagName != "w:rPr"] | |
| assert tags.index("w:tab") < tags.index("w:t") | |
| assert _ins_visible_text(b_ins[0]) == "X" | |
| def test_insert_mid_run_split_skips_rpr_and_empty_wt(self, temp_xml): | |
| """The split walk skips w:rPr, whitespace text nodes, and empty w:t | |
| children while keeping formatting on both rebuilt halves.""" | |
| run_xml = "<w:r><w:rPr><w:b/></w:rPr><w:t>Hello</w:t> <w:t></w:t><w:tab/><w:t>world</w:t></w:r>" | |
| xml_path = temp_xml(f"<w:p>{_foreign_ins(run_xml)}</w:p>") | |
| manager = _make_manager(xml_path) | |
| manager.insert_text_after("Hello", "X") | |
| _assert_no_nested_ins(manager) | |
| assert _visible_text(manager) == "HelloXworld" | |
| a_ins = [e for e in _ins_elems(manager) if e.getAttribute("w:author") == AUTHOR_A] | |
| assert len(a_ins) == 2 | |
| assert _ins_visible_text(a_ins[0]) == "Hello" | |
| assert len(a_ins[0].getElementsByTagName("w:tab")) == 0 | |
| assert _ins_visible_text(a_ins[1]) == "world" | |
| assert len(a_ins[1].getElementsByTagName("w:tab")) == 1 | |
| # Formatting survives the rebuild on both sides of the split | |
| for ins in a_ins: | |
| assert ins.getElementsByTagName("w:b") |
🤖 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 `@tests/test_foreign_ins_edits.py` around lines 391 - 440, Strengthen the
assertions in test_insert_mid_run_tab_between_texts_stays_in_order and
test_insert_mid_run_split_skips_rpr_and_empty_wt so they verify the w:tab child
appears before the w:t containing “world” within a_ins[1]. Inspect child-node
order or compare serialized-node positions while preserving the existing text,
count, and formatting checks.
pablospe
added a commit
that referenced
this pull request
Jul 22, 2026
* feat: reconstruct revision groups at parse time — inferred groups for pre-existing revisions (ISSUES.md #46) Revisions already in the file (previous sessions, foreign reviewers, Word round-trips) now get inferred groups at RevisionManager construction: maximal runs of contiguous same-paragraph revisions sharing identical raw w:author + w:date become one group, so whole logical edits resolve as a unit after reopen too. New Revision.group_source ("recorded" | "inferred") reports provenance; split-off tails of own pre-session insertions are adopted into their origin's inferred group; ungroupable revisions (missing author/date, outside any paragraph, nonconforming ids) stay group_id=None and break contiguity runs. Known imprecisions (same-paragraph same-second over-merge, nested-paragraph over-split, rump groups) are documented and pinned by tests. * fix: review round 1 — freeze one w:date per edit, safe nonconforming-id listing, snapshot dataclass - frozen_timestamp(): one logical operation now stamps all its revisions with a single w:date (previously each injection call read the clock, so a multi-hunk edit crossing a second boundary would reconstruct as several inferred groups after reopen); adversarial ticking-clock test. - list_revisions() no longer crashes on a non-numeric w:id (omitted: unrepresentable by int-keyed operations); a duplicated w:id is now wholly ungrouped — every occurrence — since id-keyed lookup cannot tell occurrences apart. Docs aligned. - _RegistrySnapshot is a named dataclass (CLAUDE.md: named objects over 3+-tuples); Revision.group_source reuses the _GroupSource alias. * fix: review round 2 — duplicate-id pre-scan, doc parity for foreign split halves, public GroupSource alias - _reconstruct_groups now pre-scans for duplicated w:ids independent of groupability: an ungroupable occurrence (e.g. missing date) no longer lets its groupable twin win a group that id-keyed lookup would report for both elements; replaces the round-1 demotion surgery with a simpler never-group-duplicates rule. - docs/api.md and SKILL.md now list the fourth group_id=None case (mid-session split half of a foreign insertion), matching the Revision docstring. - _GroupSource renamed to GroupSource: it types the public Revision.group_source field (same convention as session.ExecStatus).
pablospe
added a commit
that referenced
this pull request
Jul 22, 2026
…ES.md #53) One changeset (a single edit call, or one whole batch_edit/batch_rewrite call) stamps one whole-second w:date, bumped past the previous stamp when the same author's changesets collide in one real second. Parse-time group reconstruction (#46) therefore never merges two of our own changesets; same-paragraph ops of one batch call share their date and merge by design. - xml_editor: _next_changeset_timestamp() allocates collision-bumped stamps (monotonic per author, also across clock regressions); frozen_timestamp() becomes reentrant-by-reuse so nested per-op freezes join the enclosing batch changeset. - track_changes: batch_edit/batch_rewrite wrap the whole call in one frozen_timestamp() scope — one date per batch call, one group per op. - tests: TestChangesetDateStamping pins the contract (shared batch date under a ticking clock, no bump when seconds differ, bounded drift, distinct dates for two batches in one second, nested freeze reuse, w16du:dateUtc parity); the former same-second over-merge relaxation flips into test_same_paragraph_same_second_two_groups.
pablospe
added a commit
that referenced
this pull request
Jul 22, 2026
…ES.md #53) (#65) * feat: collision-bumped w:date stamping at changeset granularity (ISSUES.md #53) One changeset (a single edit call, or one whole batch_edit/batch_rewrite call) stamps one whole-second w:date, bumped past the previous stamp when the same author's changesets collide in one real second. Parse-time group reconstruction (#46) therefore never merges two of our own changesets; same-paragraph ops of one batch call share their date and merge by design. - xml_editor: _next_changeset_timestamp() allocates collision-bumped stamps (monotonic per author, also across clock regressions); frozen_timestamp() becomes reentrant-by-reuse so nested per-op freezes join the enclosing batch changeset. - track_changes: batch_edit/batch_rewrite wrap the whole call in one frozen_timestamp() scope — one date per batch call, one group per op. - tests: TestChangesetDateStamping pins the contract (shared batch date under a ticking clock, no bump when seconds differ, bounded drift, distinct dates for two batches in one second, nested freeze reuse, w16du:dateUtc parity); the former same-second over-merge relaxation flips into test_same_paragraph_same_second_two_groups. * docs+style: scope the never-share-a-second guarantee to one open session Multi-review round 1: the collision counter is per-editor-instance and not seeded from persisted dates, so the docs (api.md, SKILL.md, Revision.group_source and RevisionManager docstrings) now state the guarantee as per-session — own writes from a previous session merge like foreign revisions on identical author + date. Also rename _next_changeset_timestamp to _get_next_changeset_timestamp to match the sibling allocators (_get_next_change_id, _get_next_comment_id) and rewrap the api.md paragraph left with an orphan line. Skipped per red-team triage: stamp-slot consumption by empty/failed changesets (accepted bounded drift, in plan), per-author stamp dict (author is immutable per editor — false positive), test docstring issue-reference form (subjective). * docs+test: round-2 review polish — session qualifier, helper reuse, regression test - frozen_timestamp docstring and TestChangesetDateStamping class docstring now carry the same per-session qualifier as every other statement of the never-share-a-second guarantee (round 1 missed these two spots). - test_nested_frozen_timestamp_reuses_outer_stamp builds its editor via the file's _make_manager helper instead of a raw DocxXMLEditor construction. - New test_clock_regression_keeps_dates_monotonic drives the settable clock strictly backwards, pinning the documented NTP-step-back behavior (dates never regress; previous+1 until the clock catches up) — previously only the equality branch was exercised. * fix: dedupe SKILL.md lines duplicated in rebase conflict resolution
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.
Summary
Tracked replace/delete/insert operations rebuilt runs by front-hoisting every non-text child (
w:tab,w:br,w:drawing, field chars) into a single run emitted before all text parts, reordering document content. A run like<w:r><w:t>foo</w:t><w:tab/><w:t>bar</w:t></w:r>lost its tab position on any tracked edit. This is ISSUES.md item #20 (repo-local issue list, not a GitHub issue).Changes
rebuild_run_fragments()toxml_editor.py: walks a run's direct children in document order, preserves non-text children in place (each wrapped in its own run carrying the originalrPr), and delegates eachw:tto a caller-supplied renderer._replace_same_context,_delete_regular_segment,_delete_same_context,_insert_near_match, and the comment-marker rebuild incomments.py._split_foreign_ins_atkeeps its own ordered walk (children must land on the correct side of the split point) instead of front-hoisting._get_non_text_children_xmlhelper.w:tnested inside a drawing's text box is no longer duplicated as a top-level run (the old descendant walk re-emitted it).Review
Multi-reviewer loop (3 independent reviewers + codex + red-team meta-review): correctness, dead-code, and codex verdicts CLEAN; three cosmetic findings (callback type hint, closure param naming, fixture docstring) confirmed and applied in the follow-up commit.
Known pre-existing limitation (unchanged by this PR, candidate follow-up issue): a single match spanning from regular text into a drawing's textbox content still mis-rebuilds — the paragraph text map indexes textbox
w:tnodes.Testing
tests/test_run_child_order.py(19 tests): replace/delete/insert around mid-run tabs/breaks in pending, accepted, and rejected states; cross-run replace; mixed-state delete; multi-w:tordering; empty-w:tsiblings; field-char begin/instr/end sequences; textbox non-duplication. Assertions compare exact ordered token lists.tests/test_foreign_ins_edits.py.ruff check,ruff format --check, andty checkall clean.Summary by CodeRabbit
Bug Fixes
Tests