Repository navigation
fix: unify occurrence search through text maps (ISSUES.md #21) - #39
Conversation
Document-wide replace/delete/insert used an element-level w:t search
(_get_nth_match) with a text-map fallback, and the two counted
occurrences over different sequences: a match split across fragmented
runs was invisible to the element-level path, so occurrence=N could
target different matches depending on which path won.
Route all three methods through a single _find_document_wide text-map
lookup (the same sequence count_matches uses), delete the duplicate
single-node edit bodies and the drift shim (~250 lines), and rewrite
_insert_near_match to split the edge w:t at the match boundary,
fixing mid-node anchor placement for the paragraph-scoped and
batch_edit insert paths as well ("HelloXX world", not "Hello worldXX").
TextNotFoundError.total_occurrences now matches count_matches, and
runs with non-text children (w:tab, w:br) keep them during inserts.
|
Warning Review limit reached
Next review available in: 28 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)
✨ 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 #39 +/- ##
=======================================
+ Coverage 93.5% 94.0% +0.4%
=======================================
Files 11 11
Lines 2950 2812 -138
Branches 634 592 -42
=======================================
- Hits 2761 2644 -117
+ Misses 84 74 -10
+ Partials 105 94 -11 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Multi-reviewer pass on PR #39. Fixed: missing `occurrence: int = 0` default, matching the sibling finders (_find_in_paragraph, _find_across_boundaries). Verified as not applicable here and deferred with notes in ISSUES.md: straddle-match insertion into a foreign w:ins (needs #19's ins-splitting machinery; in-place rebuild would nest w:ins) and front-hoisted non-text run children in the insert rebuild (#20, same pattern as the existing replace/delete paths). xml:space on generated w:t was a false positive — the editor's attribute injection adds it.
… (ISSUES.md #39) (#74) Replacing text inside our own pending insertion anchored the replacement at ins_elem.firstChild — the match position only when the match starts at the insertion's first character. Every other match came out reordered. _replace_within_own_ins() rebuilds each affected run where it stands via rebuild_run_fragments: text before the match, the matched text dropped, the replacement emitted at the match's last node, text after, unmatched w:t siblings preserved. Runs are swapped one at a time and rebuilt deepest-first, so content never migrates between adjacent own insertions and a run nested in another matched run's w:drawing is mutated while still attached. An insertion left with no element children is removed; one holding another author's nested w:del is kept. Behaviour change: a match covering a whole own insertion now amends it in place instead of minting a fresh w:ins, returning -1 / group_id=None like every partial splice. The insertion keeps its id, date and group; undoing an amendment means rejecting that group. group_id_of() resolves a negative revision id to None so the sentinel cannot collide with a document carrying a negative w:id. Docs (api.md, SKILL.md, EditResult, replace(), rewrite_paragraph()) state the amend-in-place contract. Tests cover the repro across pending/accepted/rejected views, single-run, mid-run, sibling w:t, two adjacent own insertions, a consumed leading insertion, a consumed insertion holding a foreign w:del, both text-box shapes, and the start-anchored case.
1 participant
Problem
replace_text,suggest_deletion, andinsert_text_after/before(document-wide path) tried an element-levelw:tsearch first and fell back to the text-map search only on a miss. The two searches count occurrences over different sequences — the element-level one only sees wholew:tnodes containing the text, so a match split across fragmented runs is invisible to it. Result:occurrence=Ncould target different matches depending on which path won. Concretely, with occurrence 0 of "target" split"tar"+"get"in P1 and occurrence 1 whole in P2,replace_text("target", "X", occurrence=0)edited P2.Same bug class as issue #5 (comments), which already routes exclusively through the text map.
Fix
_find_document_wide(text, occurrence): single text-map lookup shared by all three methods, counting over the same sequence ascount_matches. Each method'sparagraph=Nonebranch is now two lines._get_nth_match,_find_match_containing_node, and the three duplicate single-node edit bodies (~250 lines) — the cross-boundary helpers were already the mainline for paragraph-scoped calls andbatch_edit._insert_near_matchto split the edgew:tat the match boundary instead of inserting relative to whole runs. This also fixes a pre-existing mid-node placement bug in the paragraph-scoped andbatch_editinsert paths (insert_text_after("Hello", "XX")on<w:t>Hello world</w:t>produced"Hello worldXX"; now"HelloXX world"). Anchors insidew:insare spliced at the exact offset with no nestedw:ins; non-text run children (w:tab,w:br) are preserved.TextNotFoundError.total_occurrencesnow reports the text-map count, consistent withcount_matches(message wording unchanged).Tests
TestDocumentWideOccurrenceStability(9): pins occurrence indexing on a split-match fixture for replace/delete/insert plus the error contract. Failed before the fix.TestInsertPlacementMidNode(9): exact-output placement assertions across document-wide, paragraph-scoped, andbatch_editpaths, sibling/w:insedge cases. Six failed before the fix.TestDocumentWideEditsRealXml(7): real-XML replacements for the deleted mock-driven edge-case tests (rPr preservation, in-placew:inssemantics, prefix/suffix splits)._get_nth_matchunit tests against the public API.Full suite: 760 passed, 2 skipped (baseline 746/8 — six formerly-skipped tests skipped on
TextNotFoundErrorand now run and pass because the unified search finds their targets).Out of scope
Author-aware edits inside foreign
w:ins(ISSUES.md #19) and non-text run-children ordering (#20) are follow-up tasks that build on this unification.