Repository navigation
feat: revision grouping — EditResult with group_id, accept_group/reject_group (ISSUES.md #37) - #54
Conversation
…ct_group (ISSUES.md #37)
|
Warning Review limit reached
Next review available in: 30 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)
📝 WalkthroughWalkthroughTracked-edit operations now return ChangesRevision Group Management
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant Document
participant RevisionManager
participant DocxXMLEditor
Caller->>Document: replace, delete, insert, or rewrite
Document->>RevisionManager: execute grouped operation
RevisionManager->>DocxXMLEditor: create and collect tracked changes
DocxXMLEditor-->>RevisionManager: revision elements and IDs
RevisionManager-->>Document: group ID and revision IDs
Document-->>Caller: EditResult
Caller->>Document: accept_group or reject_group
Document->>RevisionManager: resolve group members
RevisionManager-->>Caller: resolved revision count
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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #54 +/- ##
=======================================
- Coverage 95.3% 95.3% -0.1%
=======================================
Files 11 11
Lines 3451 3576 +125
Branches 701 719 +18
=======================================
+ Hits 3291 3410 +119
- Misses 76 79 +3
- Partials 84 87 +3 ☔ 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: 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 539-546: Replace the heterogeneous tuple used by
_registry_snapshot and _restore_registry with a dedicated dataclass, following
the existing _GroupCapture pattern. Define fields for the group counter, groups
mapping, and revision-groups mapping, return that dataclass from
_registry_snapshot, and update _restore_registry to access its named fields
while preserving the existing rollback 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: 352c750d-cc7b-49fb-b68c-c43116dd9f23
📒 Files selected for processing (7)
docs/api.mddocx_editor/__init__.pydocx_editor/document.pydocx_editor/track_changes.pydocx_editor/xml_editor.pyskills/docx/SKILL.mdtests/test_revision_groups.py
| def _registry_snapshot(self) -> tuple[int, dict[int, tuple[int, ...]], dict[int, int]]: | ||
| """Snapshot the group registry for rollback alongside a DOM snapshot.""" | ||
| return (self._group_counter, dict(self._groups), dict(self._revision_groups)) | ||
|
|
||
| def _restore_registry(self, snapshot: tuple[int, dict[int, tuple[int, ...]], dict[int, int]]) -> None: | ||
| """Restore the group registry captured by ``_registry_snapshot``.""" | ||
| self._group_counter, self._groups, self._revision_groups = snapshot | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Return a dataclass instead of a bare 3-tuple.
_registry_snapshot/_restore_registry pass around tuple[int, dict[int, tuple[int, ...]], dict[int, int]] — three heterogeneous values via positional tuple. _GroupCapture right above already shows the preferred pattern in this same file.
♻️ Proposed fix using a dataclass
+@dataclass
+class _RegistrySnapshot:
+ """Point-in-time copy of the group registry, for rollback."""
+
+ group_counter: int
+ groups: dict[int, tuple[int, ...]]
+ revision_groups: dict[int, int]
+
- def _registry_snapshot(self) -> tuple[int, dict[int, tuple[int, ...]], dict[int, int]]:
+ def _registry_snapshot(self) -> "_RegistrySnapshot":
"""Snapshot the group registry for rollback alongside a DOM snapshot."""
- return (self._group_counter, dict(self._groups), dict(self._revision_groups))
+ return _RegistrySnapshot(self._group_counter, dict(self._groups), dict(self._revision_groups))
- def _restore_registry(self, snapshot: tuple[int, dict[int, tuple[int, ...]], dict[int, int]]) -> None:
+ def _restore_registry(self, snapshot: "_RegistrySnapshot") -> None:
"""Restore the group registry captured by ``_registry_snapshot``."""
- self._group_counter, self._groups, self._revision_groups = snapshot
+ self._group_counter = snapshot.group_counter
+ self._groups = snapshot.groups
+ self._revision_groups = snapshot.revision_groupsAs per coding guidelines: "Use named objects (dataclasses) instead of tuples for functions returning 3 or more values."
🤖 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 539 - 546, Replace the
heterogeneous tuple used by _registry_snapshot and _restore_registry with a
dedicated dataclass, following the existing _GroupCapture pattern. Define fields
for the group counter, groups mapping, and revision-groups mapping, return that
dataclass from _registry_snapshot, and update _restore_registry to access its
named fields while preserving the existing rollback behavior.
Source: Coding guidelines
Multi-reviewer findings (iteration 1): - Split-off tail of an own insertion with no origin group (pre-session) was claimed by the splitting operation's _grouped capture — via the rewrite path the poisoned group even reached EditResult.group_id, so the documented reject_group undo idiom would remove pre-existing text. _adopt_split_tail now always registers tails (origin group or None) and the registry value type is widened to int | None. - Drop dead _GroupCapture.revision_ids (never read). - Single int(rev_id) parse in _parse_revision. - SKILL.md: fix stale 'plain string' return-value line; api.md: batch_edit group_id=None caveat. Skipped (false positive): claimed _grouped nesting in batch_edit — _apply_single_edit dispatches to low-level helpers, not wrapped methods.
Multi-reviewer findings (iteration 2): - HIGH: splitting a foreign insertion re-serializes its trailing children through attribute injection; a pre-session own revision nested in there (e.g. our w:del inside their w:ins) was collected and claimed into the splitting operation's group, so reject_group resurrected already-deleted text. The collector now only reports elements whose w:id it freshly assigned — pre-existing revisions passing back through injection keep their ids and are never collected. Regression test covers the repro. - Docs: rewrite group_id=None also covers changes merged into own pending insertions (EditResult/rewrite_paragraph docstrings, api.md); drop ALL-CAPS emphasis in SKILL.md. - Coverage: pragma on unreachable return after always-returning loop. Skipped (false positive): claimed missing 'return results' in batch_edit — present on the success path, verified by source and passing batch tests.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
docx_editor/track_changes.py (1)
1448-1457: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNew instance of a repeated "find the fresh w:ins/w:del id in new_nodes" pattern.
This fix correctly surfaces the wrapper's real id instead of
-1, but the same node-scan-and-return-id snippet is now repeated ~6-7 times across this file (e.g. lines 1474-1477, 1549-1551, 1619-1621, 1811-1813, 2093-2095, 2236-2238). Worth extracting a small shared helper (e.g._first_new_element_id(nodes, tag)), especially since this diff adds another copy of it.🤖 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 1448 - 1457, Extract the repeated fresh revision-ID scan into a shared helper such as _first_new_element_id(nodes, tag), returning the first matching element’s w:id. Replace the duplicated w:ins/w:del node loops throughout the affected methods, including the insertion flow around new_nodes, while preserving the existing fallback behavior when no matching element is found.docx_editor/document.py (2)
742-770: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
delete()'s "Returns" doc omits the documentedgroup_id=Nonecase.
_grouped()'s own docstring (track_changes.py) and theTestOwnInsertionSplits.test_middle_delete_adopts_split_tail_into_origin_grouptest confirmdelete()can returngroup_id=Nonewhen the delete only trims text inside the caller's own pending insertion (the split tail is adopted into the origin insertion's group, not a new one for the delete).rewrite_paragraph()andbatch_edit()docstrings both explicitly call out theirgroup_id=Nonecase, butdelete()'s "Returns" section here doesn't mention it at all — a caller relying onresult.group_idforaccept_group()/reject_group()could be surprised.📝 Proposed doc fix
Returns: EditResult — the new paragraph reference with updated hash (e.g., "P2#c3d4"), carrying ``group_id``/``revision_ids`` of the - revisions this edit created. + revisions this edit created (``group_id`` is None when the + deletion only trimmed text inside your own pending insertion, + with no new revision of its own — e.g. the split tail is + adopted into the origin insertion's group instead).🤖 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/document.py` around lines 742 - 770, Update the delete() docstring’s Returns section to state that EditResult.group_id may be None when the deletion only trims text within the caller’s own pending insertion and adopts the split tail into the origin group; retain the existing description for edits that create revisions.
1137-1195: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winGuard
Nonebefore callingaccept_group/reject_groupdocx_editor/document.py:1137-1195
EditResult.group_idisint | None, but these helpers only acceptint, so the documenteddoc.accept_group(result.group_id)pattern will fail on no-op edits withRevisionError("Unknown revision group: None"). Either acceptint | Nonehere or show aNoneguard at the call site.🤖 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/document.py` around lines 1137 - 1195, Update Document.accept_group and reject_group to handle EditResult.group_id values of None without raising an unknown-group error, while preserving existing behavior for valid integer group IDs. Adjust the type annotations and return handling consistently with the chosen no-op behavior, and update the docstrings’ examples or parameter documentation if needed.docs/api.md (1)
300-316: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
delete()doc gap — shares root cause with the docstring gap indocx_editor/document.py.See the consolidated comment for this shared fix.
🤖 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 `@docs/api.md` around lines 300 - 316, Update the `delete()` API documentation and its corresponding `Document.delete` docstring together so they describe the same behavior and parameters, including how authorship and pending insertions are handled. Keep the documented signature, occurrence-selection rules, return type, and `AmbiguousTextError` behavior consistent across both locations.
🤖 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 `@tests/test_revision_groups.py`:
- Around line 471-497: Split the combined content assertion in
test_presession_rewrite_does_not_claim_split_tail into separate assert
statements, one validating that “BETA” is present and another that “beta” is
absent; likewise split the assertion for “alpha” and “gamma” into individual
checks so each failure identifies the specific missing or unexpected text.
---
Outside diff comments:
In `@docs/api.md`:
- Around line 300-316: Update the `delete()` API documentation and its
corresponding `Document.delete` docstring together so they describe the same
behavior and parameters, including how authorship and pending insertions are
handled. Keep the documented signature, occurrence-selection rules, return type,
and `AmbiguousTextError` behavior consistent across both locations.
In `@docx_editor/document.py`:
- Around line 742-770: Update the delete() docstring’s Returns section to state
that EditResult.group_id may be None when the deletion only trims text within
the caller’s own pending insertion and adopts the split tail into the origin
group; retain the existing description for edits that create revisions.
- Around line 1137-1195: Update Document.accept_group and reject_group to handle
EditResult.group_id values of None without raising an unknown-group error, while
preserving existing behavior for valid integer group IDs. Adjust the type
annotations and return handling consistently with the chosen no-op behavior, and
update the docstrings’ examples or parameter documentation if needed.
In `@docx_editor/track_changes.py`:
- Around line 1448-1457: Extract the repeated fresh revision-ID scan into a
shared helper such as _first_new_element_id(nodes, tag), returning the first
matching element’s w:id. Replace the duplicated w:ins/w:del node loops
throughout the affected methods, including the insertion flow around new_nodes,
while preserving the existing fallback behavior when no matching element is
found.
🪄 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: bd439b61-e163-4e03-b95f-43c31a5def12
📒 Files selected for processing (6)
docs/api.mddocx_editor/document.pydocx_editor/track_changes.pydocx_editor/xml_editor.pyskills/docx/SKILL.mdtests/test_revision_groups.py
| def test_presession_rewrite_does_not_claim_split_tail(self, temp_docx): | ||
| # Same defect through the public rewrite path: without tail | ||
| # registration, the rewrite's EditResult.group_id would hold ONLY | ||
| # the pre-session insertion's tail, and reject_group (the documented | ||
| # undo idiom) would rip out pre-existing text instead of undoing | ||
| # the rewrite. | ||
| with Document.open(temp_docx) as doc: | ||
| ref = find_ref(doc, "quick brown fox") | ||
| doc.insert_after("fox", " alpha beta gamma", paragraph=ref) | ||
| doc.save() | ||
|
|
||
| with Document.open(temp_docx) as doc: | ||
| ref = find_ref(doc, "quick brown fox") | ||
| current = _paragraph_text(doc, 1) | ||
| result = doc.rewrite_paragraph(ref, current.replace("beta", "BETA")) | ||
|
|
||
| # The only physical change happens inside our own pre-session | ||
| # insertion — no trackable revisions, so no group at all. | ||
| assert result.group_id is None | ||
| assert doc._revision_manager._groups == {} | ||
| assert all(rev.group_id is None for rev in doc.list_revisions()) | ||
| # Placement-agnostic content check: replacement position inside | ||
| # an own insertion is the pre-existing ISSUES.md #31 ordering | ||
| # bug, out of scope here. | ||
| text = _paragraph_text(doc, 1) | ||
| assert "BETA" in text and "beta" not in text | ||
| assert "alpha" in text and "gamma" in text |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Split combined checks per Ruff PT018.
Lines 496-497 pack two conditions each into one assert; splitting them gives a clearer failure message pinpointing which half failed.
🧪 Proposed split
text = _paragraph_text(doc, 1)
- assert "BETA" in text and "beta" not in text
- assert "alpha" in text and "gamma" in text
+ assert "BETA" in text
+ assert "beta" not in text
+ assert "alpha" in text
+ assert "gamma" in 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 test_presession_rewrite_does_not_claim_split_tail(self, temp_docx): | |
| # Same defect through the public rewrite path: without tail | |
| # registration, the rewrite's EditResult.group_id would hold ONLY | |
| # the pre-session insertion's tail, and reject_group (the documented | |
| # undo idiom) would rip out pre-existing text instead of undoing | |
| # the rewrite. | |
| with Document.open(temp_docx) as doc: | |
| ref = find_ref(doc, "quick brown fox") | |
| doc.insert_after("fox", " alpha beta gamma", paragraph=ref) | |
| doc.save() | |
| with Document.open(temp_docx) as doc: | |
| ref = find_ref(doc, "quick brown fox") | |
| current = _paragraph_text(doc, 1) | |
| result = doc.rewrite_paragraph(ref, current.replace("beta", "BETA")) | |
| # The only physical change happens inside our own pre-session | |
| # insertion — no trackable revisions, so no group at all. | |
| assert result.group_id is None | |
| assert doc._revision_manager._groups == {} | |
| assert all(rev.group_id is None for rev in doc.list_revisions()) | |
| # Placement-agnostic content check: replacement position inside | |
| # an own insertion is the pre-existing ISSUES.md #31 ordering | |
| # bug, out of scope here. | |
| text = _paragraph_text(doc, 1) | |
| assert "BETA" in text and "beta" not in text | |
| assert "alpha" in text and "gamma" in text | |
| def test_presession_rewrite_does_not_claim_split_tail(self, temp_docx): | |
| # Same defect through the public rewrite path: without tail | |
| # registration, the rewrite's EditResult.group_id would hold ONLY | |
| # the pre-session insertion's tail, and reject_group (the documented | |
| # undo idiom) would rip out pre-existing text instead of undoing | |
| # the rewrite. | |
| with Document.open(temp_docx) as doc: | |
| ref = find_ref(doc, "quick brown fox") | |
| doc.insert_after("fox", " alpha beta gamma", paragraph=ref) | |
| doc.save() | |
| with Document.open(temp_docx) as doc: | |
| ref = find_ref(doc, "quick brown fox") | |
| current = _paragraph_text(doc, 1) | |
| result = doc.rewrite_paragraph(ref, current.replace("beta", "BETA")) | |
| # The only physical change happens inside our own pre-session | |
| # insertion — no trackable revisions, so no group at all. | |
| assert result.group_id is None | |
| assert doc._revision_manager._groups == {} | |
| assert all(rev.group_id is None for rev in doc.list_revisions()) | |
| # Placement-agnostic content check: replacement position inside | |
| # an own insertion is the pre-existing ISSUES.md `#31` ordering | |
| # bug, out of scope here. | |
| text = _paragraph_text(doc, 1) | |
| assert "BETA" in text | |
| assert "beta" not in text | |
| assert "alpha" in text | |
| assert "gamma" in text |
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 496-496: Assertion should be broken down into multiple parts
Break down assertion into multiple parts
(PT018)
[warning] 497-497: Assertion should be broken down into multiple parts
Break down assertion into multiple parts
(PT018)
🤖 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_revision_groups.py` around lines 471 - 497, Split the combined
content assertion in test_presession_rewrite_does_not_claim_split_tail into
separate assert statements, one validating that “BETA” is present and another
that “beta” is absent; likewise split the assertion for “alpha” and “gamma” into
individual checks so each failure identifies the specific missing or unexpected
text.
Source: Linters/SAST tools
…turn Iteration-3 review: batch_rewrite docs (api.md and docstring) now carry the same group_id=None caveat as rewrite_paragraph; the 'no cover' pragma added in iteration 2 sat on a line the splice-in-place branch falls through to — removed so coverage stays honest. No code-behavior changes.
…ES.md #54) (#68) Third revision-tracking tier: one whole call (single edit, or an entire batch_edit/batch_rewrite) = one changeset ⊇ ≥1 group ⊇ revisions. A changeset is the (author, date) equivalence class over groups (global, not contiguous). Recorded live via a reentrant _changeset() boundary; reconstructed as inferred changesets at parse time; registry snapshot/restore extended so a failed batch leaves no ghost changeset. Adds Document.accept_changeset/reject_changeset, changeset_id on EditResult, changeset_id/changeset_source on Revision, changeset_id on RevisionError.
1 participant
Summary
Implements revision grouping (ISSUES.md item #37 — not a GitHub issue): every logical edit operation registers the revisions it creates as one revision group, so a multi-revision edit — especially
rewrite_paragraph, where each revision is a diff hunk — can be accepted or rejected as a unit instead of hunk-by-hunk, which garbles the paragraph when only partially applied.API
EditResult(new, exported): return type ofreplace/delete/insert_after/insert_before/rewrite_paragraphand the element type ofbatch_edit/batch_rewriteresults. Astrsubclass whose value is the new paragraph ref, so it is drop-in compatible everywhere a ref string is expected; carriesgroup_idandrevision_ids.Document.accept_group(group_id)/Document.reject_group(group_id): resolve every revision of one edit as a unit (reverse-id, loop-until-no-progress — same pattern asaccept_all). Members already resolved individually are skipped.Revision.group_id: stamped on session-created revisions bylist_revisions();Nonefor foreign/pre-session revisions.save()keeps them;close()/reopen drops them (unknown ids raiseRevisionError). Nothing is written into the.docx.Implementation
RevisionManager._grouped()context manager captures the<w:ins>/<w:del>elements processed by attribute injection (via a new single-slotDocxXMLEditor.collect_tracked_changes()collector), then filters to elements that are ours, still DOM-attached, and not already registered — foreign split halves and adopted tails stay out._max_change_id) so ids of removed revisions are never reissued into id-keyed bookkeeping.<w:ins>(its group is reachable from the result), and a split-off tail of an own insertion is adopted into its origin insertion's group (_adopt_split_tail), so rejecting the original edit removes both halves.Testing
tests/test_revision_groups.py: EditResult contract, group stamping, accept/reject as unit, per-id interplay, rewrite grouping (headline case), per-op batch groups, batch rollback registry restore, session scoping across save/reopen, foreign-insertion split-half exclusion, monotonic id floor, own-insertion split adoption.ruff check/ruff format --checkclean;ty checkexit 0 (6 pre-existing warnings in untouched test files).docs/api.md(EditResult, accept_group/reject_group sections) andskills/docx/SKILL.md.Summary by CodeRabbit
New Features
EditResult(a paragraph-ref string subclass) that includes revision group details.accept_group(group_id)andreject_group(group_id)to apply or undo all revisions from one logical edit atomically.EditResultper operation/rewrite.EditResultis re-exported from the top-leveldocx_editorpackage.Documentation
EditResult,group_id/revision_ids, and group lifecycle/behavior.