Merge pull request #3 from aehrc/fix/replace-inactive-chained-regex-issue-2

fix: position-based Replace Inactive to avoid chained-regex corruption

Author: dionmcm

CHANGELOG.md

@@ -33,6 +33,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/).
 - **Anonymous Install Metrics**: Random UUID in User-Agent for install counting — no personal data collected.
 - **Welcome Screen**: First-launch onboarding with mailing list and GitHub links.
 
+### Fixed
+- **Replace Inactive**: Fixed chained-regex replacement that could corrupt output when one inactive concept's replacement target SCTID matched another inactive concept's ID in the same selection. Replacements are now located by position against the original text and applied in reverse order, mirroring Replace Selection (issue #2). The completion message now reports the number of concepts actually replaced (rather than the number attempted) and logs a warning if any concept with a replacement could not be located in the text.
+
 ### Security
 - XSS prevention in WebView rendering
 - Input size and depth limits on all parsers

Codeagogo/AppDelegate.swift

@@ -1122,19 +1122,21 @@ final class AppDelegate: NSObject, NSApplicationDelegate {
                     return
                 }
 
-                // 6. Replace in text — use regex to match concept ID + optional display term
-                var result = text
-                for (conceptId, replacement) in replacementsByCode {
-                    let pattern = "(?<![0-9])" + NSRegularExpression.escapedPattern(for: conceptId) + "(?![0-9])" + "(\\s*\\|[^|]*\\|)?"
-                    if let regex = try? NSRegularExpression(pattern: pattern) {
-                        let range = NSRange(result.startIndex..., in: result)
-                        result = regex.stringByReplacingMatches(
-                            in: result,
-                            options: [],
-                            range: range,
-                            withTemplate: NSRegularExpression.escapedTemplate(for: replacement)
-                        )
-                    }
+                // 6. Replace in text using position-based substitution against the
+                //    original text. Matching the concept ID (plus its optional display
+                //    term) by position and applying replacements in reverse order makes
+                //    each replacement immune to text inserted by other iterations —
+                //    avoiding corruption when one inactive concept's replacement target
+                //    equals another inactive concept's ID (see issue #2).
+                let replacement = model.replacingInactiveConcepts(in: text, with: replacementsByCode)
+                let result = replacement.text
+
+                // Reconcile intent against reality: a concept may have a replacement
+                // but not be located in the text by the extractor (e.g. input over the
+                // extraction size limit). Don't report those as replaced — log instead.
+                let missed = Set(replacementsByCode.keys).subtracting(replacement.replacedConceptIds)
+                if !missed.isEmpty {
+                    AppLog.warning(AppLog.ui, "Replace inactive: \(missed.count) concept(s) had a replacement but could not be located in the text: \(missed.sorted())")
                 }
 
                 // 7. Put result on clipboard and paste
@@ -1150,8 +1152,11 @@ final class AppDelegate: NSObject, NSApplicationDelegate {
 
                 progressHUD.hide()
 
-                let count = replacementsByCode.count
+                let count = replacement.replacedConceptIds.count
                 var message = "Replaced \(count) inactive concept\(count == 1 ? "" : "s")"
+                if !missed.isEmpty {
+                    message += " (\(missed.count) could not be located)"
+                }
                 if !noReplacementIds.isEmpty {
                     message += " (\(noReplacementIds.count) had no replacement)"
                 }

Codeagogo/LookupViewModel.swift

@@ -419,6 +419,54 @@ final class LookupViewModel: ObservableObject {
         return ConceptMatch(conceptId: conceptId, range: fullRange, existingTerm: existingTerm, isSCTID: isSCTID)
     }
 
+    /// The outcome of an inactive-concept replacement pass.
+    struct InactiveReplacementResult {
+        /// The text after substitution.
+        let text: String
+        /// The concept IDs that were actually located in the text and replaced.
+        ///
+        /// This may be a strict subset of `replacementsByCode.keys`: a code that
+        /// was supplied a replacement but is not surfaced in `text` by
+        /// ``extractAllConceptIds(from:)`` (e.g. input over the extraction size
+        /// limit, or a token the extractor's boundaries reject) is not replaced
+        /// and does not appear here. Callers should reconcile against the
+        /// intended keys rather than assume every replacement was applied.
+        let replacedConceptIds: Set<String>
+    }
+
+    /// Replaces inactive concept IDs in `text` with their target replacements.
+    ///
+    /// Every concept ID that ``extractAllConceptIds(from:)`` locates in `text`
+    /// and that is present as a key in `replacementsByCode` is replaced — along
+    /// with its optional pipe-delimited term — by the mapped replacement string.
+    /// A key with no corresponding match in `text` is left untouched and is
+    /// reported via `InactiveReplacementResult.replacedConceptIds` (by omission).
+    ///
+    /// Replacements are located against the **original** `text` and applied in
+    /// reverse positional order using `replaceSubrange`. Because positions are
+    /// resolved before any mutation and applied back-to-front, text inserted by
+    /// one replacement can never be re-matched by another. This avoids the
+    /// corruption that a chained regex over an accumulating result produces when
+    /// one inactive concept's replacement target equals another inactive
+    /// concept's ID (see issue #2).
+    ///
+    /// - Parameters:
+    ///   - text: The original selection text.
+    ///   - replacementsByCode: Map of inactive concept ID → replacement text
+    ///     (a code with optional term, or an `(A OR B)` group).
+    /// - Returns: The substituted text and the set of concept IDs actually replaced.
+    func replacingInactiveConcepts(in text: String, with replacementsByCode: [String: String]) -> InactiveReplacementResult {
+        let matches = extractAllConceptIds(from: text)
+        var result = text
+        var replaced: Set<String> = []
+        for match in matches.reversed() {
+            guard let replacement = replacementsByCode[match.conceptId] else { continue }
+            result.replaceSubrange(match.range, with: replacement)
+            replaced.insert(match.conceptId)
+        }
+        return InactiveReplacementResult(text: result, replacedConceptIds: replaced)
+    }
+
     /// Copies a string to the system pasteboard.
     ///
     /// - Parameter s: The string to copy. If `nil` or empty, no action is taken.

CodeagogoTests/LookupViewModelTests.swift

@@ -413,4 +413,105 @@ final class LookupViewModelTests: XCTestCase {
 
         XCTAssertEqual(result, "73211009")
     }
+
+    // MARK: - Replace Inactive Concepts Tests
+
+    /// Regression test for issue #2: two inactive concepts whose replacement
+    /// targets are each other's IDs (a 2-cycle swap). The old chained-regex
+    /// approach corrupted this in **both** dictionary iteration orders — whichever
+    /// code was processed first had its just-inserted target re-matched and
+    /// clobbered by the second iteration, so no ordering produced the correct
+    /// independent mapping. Position-based substitution resolves all positions
+    /// against the original text up front, so each original code maps to its own
+    /// target regardless of order. (Numbers are synthetic, not real SCTIDs.)
+    ///
+    /// The swap is used deliberately rather than a one-directional collision
+    /// (A→B, B→C): the latter's expected output coincidentally matches the buggy
+    /// output in one of the two iteration orders, so it would not reliably fail
+    /// against the old code. The swap fails the old code in every order.
+    func testReplacingInactiveConcepts_swapCycle_doesNotChainInEitherOrder() async {
+        let viewModel = LookupViewModel(selectionReader: MockSelectionReader(), client: MockLookupClient())
+        let text = "11111111 and 22222222"
+        let replacements = [
+            "11111111": "22222222 |Concept B|",
+            "22222222": "11111111 |Concept A|",
+        ]
+
+        let result = viewModel.replacingInactiveConcepts(in: text, with: replacements)
+
+        XCTAssertEqual(
+            result.text,
+            "22222222 |Concept B| and 11111111 |Concept A|",
+            "Each original code must map to its own target, not chain through inserted text"
+        )
+        XCTAssertEqual(result.replacedConceptIds, ["11111111", "22222222"])
+    }
+
+    /// Verifies an inactive concept's existing pipe-delimited term is consumed
+    /// and replaced along with the code.
+    func testReplacingInactiveConcepts_replacesExistingTerm() async {
+        let viewModel = LookupViewModel(selectionReader: MockSelectionReader(), client: MockLookupClient())
+        let text = "73211009 | Old display |"
+        let replacements = ["73211009": "385804009 |New display|"]
+
+        let result = viewModel.replacingInactiveConcepts(in: text, with: replacements)
+
+        XCTAssertEqual(result.text, "385804009 |New display|")
+    }
+
+    /// Verifies concepts not present in the replacement map are left untouched.
+    func testReplacingInactiveConcepts_leavesUnmappedCodesUntouched() async {
+        let viewModel = LookupViewModel(selectionReader: MockSelectionReader(), client: MockLookupClient())
+        let text = "73211009 and 22222222"
+        let replacements = ["22222222": "99999999 |Concept C|"]
+
+        let result = viewModel.replacingInactiveConcepts(in: text, with: replacements)
+
+        XCTAssertEqual(result.text, "73211009 and 99999999 |Concept C|")
+        XCTAssertEqual(result.replacedConceptIds, ["22222222"])
+    }
+
+    /// Verifies multiple occurrences of the same inactive code are all replaced.
+    func testReplacingInactiveConcepts_replacesAllOccurrences() async {
+        let viewModel = LookupViewModel(selectionReader: MockSelectionReader(), client: MockLookupClient())
+        let text = "11111111 OR 11111111"
+        let replacements = ["11111111": "22222222 |Concept B|"]
+
+        let result = viewModel.replacingInactiveConcepts(in: text, with: replacements)
+
+        XCTAssertEqual(result.text, "22222222 |Concept B| OR 22222222 |Concept B|")
+    }
+
+    /// Verifies empty text and text with no extractable codes are returned
+    /// unchanged with nothing reported as replaced.
+    func testReplacingInactiveConcepts_emptyOrNoMatch_returnsUnchanged() async {
+        let viewModel = LookupViewModel(selectionReader: MockSelectionReader(), client: MockLookupClient())
+        let replacements = ["11111111": "22222222 |Concept B|"]
+
+        let empty = viewModel.replacingInactiveConcepts(in: "", with: replacements)
+        XCTAssertEqual(empty.text, "")
+        XCTAssertTrue(empty.replacedConceptIds.isEmpty)
+
+        let noCodes = viewModel.replacingInactiveConcepts(in: "<< no codes here", with: replacements)
+        XCTAssertEqual(noCodes.text, "<< no codes here")
+        XCTAssertTrue(noCodes.replacedConceptIds.isEmpty)
+    }
+
+    /// Verifies that a code supplied a replacement but absent from the text is
+    /// NOT reported as replaced — so callers can detect the mismatch rather than
+    /// over-reporting success.
+    func testReplacingInactiveConcepts_reportsOnlyCodesActuallyReplaced() async {
+        let viewModel = LookupViewModel(selectionReader: MockSelectionReader(), client: MockLookupClient())
+        let text = "73211009 only"
+        let replacements = [
+            "73211009": "385804009 |Present|",
+            "22222222": "99999999 |Absent|",
+        ]
+
+        let result = viewModel.replacingInactiveConcepts(in: text, with: replacements)
+
+        XCTAssertEqual(result.text, "385804009 |Present| only")
+        XCTAssertEqual(result.replacedConceptIds, ["73211009"],
+                       "A code absent from the text must not be reported as replaced")
+    }
 }