fix: position-based Replace Inactive to avoid chained-regex corruption - #3
Merged
Merged
Conversation
replaceInactiveConceptsInSelection applied replacements sequentially against an accumulating result string. When one inactive concept's replacement target SCTID matched another inactive concept's ID in the same selection, the second iteration's regex matched inside text just inserted by the first, corrupting the output (issue #2). Mirror the existing replaceSelection approach: locate each concept (code plus optional pipe-delimited term) against the original text via extractAllConceptIds, then apply replacements in reverse positional order with replaceSubrange. This makes each replacement immune to text inserted by other iterations. Adds the new pure LookupViewModel.replacingInactiveConcepts(in:with:) helper and 4 regression tests, including the exact synthetic reproducer from issue #2. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ment count Two issues found in multi-agent review of this PR: 1. (Critical) The collision regression test was order-dependent: the old chained-regex code only corrupted the one-directional case (A→B, B→C) in one of two Dictionary iteration orders, so the test could pass against the buggy code. Replaced it with a 2-cycle swap (A→B, B→A) that corrupts in BOTH orders — verified the swap fails the old implementation regardless of order. 2. (Important) The "Replaced N inactive concepts" message used replacementsByCode.count (intent), but substitution is driven by a different extractor (extractAllConceptIds, which has a size cap) than the one that found the concepts (eclBridge.extractConceptIds). A concept that couldn't be located was silently not replaced yet still counted. replacingInactiveConcepts now returns the set of codes actually replaced; the count reflects reality and a warning is logged for any concept that could not be located. Also tightened the method docstring and added empty/no-match and actual-replaced-reporting tests. Full unit suite: 553 passing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
Closes #2
Problem
replaceInactiveConceptsInSelectionapplied replacements sequentially against an accumulatingresultstring. When one inactive concept's replacement target SCTID matched another inactive concept's ID in the same selection, the second iteration's regex matched inside text just inserted by the first — corrupting the output.Fix
Mirrors the existing
replaceSelectionapproach: locate each concept (code + optional pipe-delimited term) against the original text viaextractAllConceptIds, then apply replacements in reverse positional order withreplaceSubrange. Each replacement is now immune to text inserted by other iterations.LookupViewModel.replacingInactiveConcepts(in:with:)Verification
xcodebuild build— succeededCodeagogo-Unittest plan — all passNote: the Windows port (
codeagogo-winTrayAppContext.cs) is tracked separately per the issue and is not touched here.🤖 Generated with Claude Code