From 734733c11c50a50b600118c3ef261db94bd87d92 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ng=C3=B4=20Qu=E1=BB=91c=20=C4=90=E1=BA=A1t?= Date: Tue, 28 Apr 2026 16:27:29 +0700 Subject: [PATCH 1/5] refactor(datagrid): extract PendingChanges value type from DataChangeManager Phase B of the data grid clean-architecture refactor. Replaces six loose collections (changes, changeIndex, deletedRowIndices, insertedRowIndices, modifiedCells, insertedRowData, changedRowIndices) with a single PendingChanges value type that owns the cross-collection invariants. DataChangeManager shrinks from ~960 LOC to ~190 LOC: it keeps undo/redo registration, plugin SQL generation, and observation, while delegating all state mutation to pending.recordCellChange / recordRowDeletion / etc. Also renamed TabPendingChanges -> TabChangeSnapshot since it's a snapshot DTO for tab persistence, distinct from the live PendingChanges tracker. PendingChanges has 18 unit tests covering record / undo / replay / snapshot round-trip / clear / consume. --- .../ChangeTracking/DataChangeManager.swift | 774 ++++-------------- .../ChangeTracking/DataChangeModels.swift | 2 +- .../Core/ChangeTracking/PendingChanges.swift | 513 ++++++++++++ TablePro/Models/Query/QueryTab.swift | 6 +- TablePro/Models/Query/QueryTabManager.swift | 2 +- TablePro/Models/Query/QueryTabState.swift | 3 +- .../MainContentCoordinator+Discard.swift | 2 +- .../MainContentCoordinator+SaveChanges.swift | 2 +- .../DataChangeManagerExtendedTests.swift | 11 +- .../DataChangeManagerTests.swift | 20 +- .../DataChangeModelsTests.swift | 20 +- .../ChangeTracking/PendingChangesTests.swift | 221 +++++ 12 files changed, 937 insertions(+), 639 deletions(-) create mode 100644 TablePro/Core/ChangeTracking/PendingChanges.swift create mode 100644 TableProTests/Core/ChangeTracking/PendingChangesTests.swift diff --git a/TablePro/Core/ChangeTracking/DataChangeManager.swift b/TablePro/Core/ChangeTracking/DataChangeManager.swift index 2d3b38a81..749e7d600 100644 --- a/TablePro/Core/ChangeTracking/DataChangeManager.swift +++ b/TablePro/Core/ChangeTracking/DataChangeManager.swift @@ -25,12 +25,14 @@ struct UndoResult { @MainActor @Observable final class DataChangeManager: ChangeManaging { private static let logger = Logger(subsystem: "com.TablePro", category: "DataChangeManager") - var changes: [RowChange] = [] - var rowChanges: [RowChange] { changes } + + private(set) var pending = PendingChanges() var hasChanges: Bool = false var reloadVersion: Int = 0 - private(set) var changedRowIndices: Set = [] + var changes: [RowChange] { pending.changes } + var rowChanges: [RowChange] { pending.changes } + var insertedRowIndices: Set { pending.insertedRowIndices } var tableName: String = "" var primaryKeyColumns: [String] = [] @@ -45,62 +47,6 @@ final class DataChangeManager: ChangeManaging { set { _columnsStorage = newValue.map { String($0) } } } - // MARK: - Cached Lookups for O(1) Performance - - private var deletedRowIndices: Set = [] - private(set) var insertedRowIndices: Set = [] - private var modifiedCells: [Int: Set] = [:] - private var insertedRowData: [Int: [String?]] = [:] - - /// (rowIndex, changeType) → index in `changes` array for O(1) lookup - /// Replaces O(n) `firstIndex(where:)` scans in hot paths like `recordCellChange` - private var changeIndex: [RowChangeKey: Int] = [:] - - /// Rebuild `changeIndex` from the `changes` array. - /// Called only for complex operations (bulk shifts, restoreState, clearChanges). - private func rebuildChangeIndex() { - changeIndex.removeAll(keepingCapacity: true) - for (index, change) in changes.enumerated() { - changeIndex[RowChangeKey(rowIndex: change.rowIndex, type: change.type)] = index - } - } - - /// Remove a single change at a known array index and update changeIndex incrementally. - /// O(n) for index adjustment but avoids full dictionary rebuild. - private func removeChangeAt(_ arrayIndex: Int) { - let removed = changes[arrayIndex] - let removedKey = RowChangeKey(rowIndex: removed.rowIndex, type: removed.type) - changeIndex.removeValue(forKey: removedKey) - changes.remove(at: arrayIndex) - - for (key, idx) in changeIndex where idx > arrayIndex { - changeIndex[key] = idx - 1 - } - } - - @discardableResult - private func removeChangeByKey(rowIndex: Int, type: ChangeType) -> Bool { - let key = RowChangeKey(rowIndex: rowIndex, type: type) - guard let arrayIndex = changeIndex[key] else { return false } - removeChangeAt(arrayIndex) - return true - } - - /// Binary search: count of elements in a sorted array that are strictly less than `target`. - /// Used for O(n log n) batch index shifting instead of O(n²) nested loops. - private static func countLessThan(_ target: Int, in sorted: [Int]) -> Int { - var lo = 0, hi = sorted.count - while lo < hi { - let mid = (lo + hi) / 2 - if sorted[mid] < target { - lo = mid + 1 - } else { - hi = mid - } - } - return lo - } - var undoManagerProvider: (() -> UndoManager?)? var onUndoApplied: ((UndoResult) -> Void)? @@ -120,21 +66,13 @@ final class DataChangeManager: ChangeManaging { // MARK: - Helper Methods func consumeChangedRowIndices() -> Set { - let indices = changedRowIndices - changedRowIndices.removeAll() - return indices + pending.consumeChangedRowIndices() } // MARK: - Configuration func clearChanges() { - changes.removeAll() - changeIndex.removeAll() - deletedRowIndices.removeAll() - insertedRowIndices.removeAll() - modifiedCells.removeAll() - insertedRowData.removeAll() - changedRowIndices.removeAll() + pending.clear() hasChanges = false reloadVersion += 1 } @@ -156,15 +94,9 @@ final class DataChangeManager: ChangeManaging { self.primaryKeyColumns = primaryKeyColumns self.databaseType = databaseType - changeIndex.removeAll() - deletedRowIndices.removeAll() - insertedRowIndices.removeAll() - modifiedCells.removeAll() - insertedRowData.removeAll() - changedRowIndices.removeAll() + pending.clear() undoManagerProvider?()?.removeAllActions(withTarget: self) - changes.removeAll() hasChanges = false if triggerReload { reloadVersion += 1 @@ -181,133 +113,31 @@ final class DataChangeManager: ChangeManaging { newValue: String?, originalRow: [String?]? = nil ) { - if oldValue == newValue { - let updateKey = RowChangeKey(rowIndex: rowIndex, type: .update) - if let existingIndex = changeIndex[updateKey], - let cellIndex = changes[existingIndex].cellChanges.firstIndex(where: { $0.columnIndex == columnIndex }) { - let originalOldValue = changes[existingIndex].cellChanges[cellIndex].oldValue - if originalOldValue == newValue { - changes[existingIndex].cellChanges.remove(at: cellIndex) - modifiedCells[rowIndex]?.remove(columnIndex) - if modifiedCells[rowIndex]?.isEmpty == true { modifiedCells.removeValue(forKey: rowIndex) } - if changes[existingIndex].cellChanges.isEmpty { removeChangeAt(existingIndex) } - changedRowIndices.insert(rowIndex) - hasChanges = !changes.isEmpty - reloadVersion += 1 - } - } - return - } - - let cellChange = CellChange( + let recorded = pending.recordCellChange( rowIndex: rowIndex, columnIndex: columnIndex, columnName: columnName, oldValue: oldValue, - newValue: newValue + newValue: newValue, + originalRow: originalRow ) - - let insertKey = RowChangeKey(rowIndex: rowIndex, type: .insert) - if let insertIndex = changeIndex[insertKey] { - if var storedValues = insertedRowData[rowIndex] { - if columnIndex < storedValues.count { - storedValues[columnIndex] = newValue - insertedRowData[rowIndex] = storedValues - } - } - - if let cellIndex = changes[insertIndex].cellChanges.firstIndex(where: { - $0.columnIndex == columnIndex - }) { - changes[insertIndex].cellChanges[cellIndex] = CellChange( - rowIndex: rowIndex, - columnIndex: columnIndex, - columnName: columnName, - oldValue: nil, - newValue: newValue - ) - } else { - changes[insertIndex].cellChanges.append(CellChange( - rowIndex: rowIndex, - columnIndex: columnIndex, - columnName: columnName, - oldValue: nil, - newValue: newValue - )) - } - registerUndo(actionName: String(localized: "Edit Cell")) { target in - target.applyDataUndo(.cellEdit( - rowIndex: rowIndex, columnIndex: columnIndex, columnName: columnName, - previousValue: oldValue, newValue: newValue, originalRow: nil - )) - } - changedRowIndices.insert(rowIndex) - hasChanges = !changes.isEmpty + guard recorded else { + hasChanges = !pending.isEmpty reloadVersion += 1 return } - - let updateKey = RowChangeKey(rowIndex: rowIndex, type: .update) - if let existingIndex = changeIndex[updateKey] { - if let cellIndex = changes[existingIndex].cellChanges.firstIndex(where: { - $0.columnIndex == columnIndex - }) { - let originalOldValue = changes[existingIndex].cellChanges[cellIndex].oldValue - changes[existingIndex].cellChanges[cellIndex] = CellChange( - rowIndex: rowIndex, - columnIndex: columnIndex, - columnName: columnName, - oldValue: originalOldValue, - newValue: newValue - ) - - if originalOldValue == newValue { - changes[existingIndex].cellChanges.remove(at: cellIndex) - modifiedCells[rowIndex]?.remove(columnIndex) - if modifiedCells[rowIndex]?.isEmpty == true { - modifiedCells.removeValue(forKey: rowIndex) - } - if changes[existingIndex].cellChanges.isEmpty { - removeChangeAt(existingIndex) - } - } - } else { - changes[existingIndex].cellChanges.append(cellChange) - modifiedCells[rowIndex, default: []].insert(columnIndex) - } - changedRowIndices.insert(rowIndex) - } else { - let rowChange = RowChange( - rowIndex: rowIndex, - type: .update, - cellChanges: [cellChange], - originalRow: originalRow - ) - changes.append(rowChange) - changeIndex[updateKey] = changes.count - 1 - modifiedCells[rowIndex, default: []].insert(columnIndex) - changedRowIndices.insert(rowIndex) - } - registerUndo(actionName: String(localized: "Edit Cell")) { target in target.applyDataUndo(.cellEdit( rowIndex: rowIndex, columnIndex: columnIndex, columnName: columnName, previousValue: oldValue, newValue: newValue, originalRow: originalRow )) } - hasChanges = !changes.isEmpty + hasChanges = !pending.isEmpty reloadVersion += 1 } func recordRowDeletion(rowIndex: Int, originalRow: [String?]) { - removeChangeByKey(rowIndex: rowIndex, type: .update) - modifiedCells.removeValue(forKey: rowIndex) - - let rowChange = RowChange(rowIndex: rowIndex, type: .delete, originalRow: originalRow) - changes.append(rowChange) - changeIndex[RowChangeKey(rowIndex: rowIndex, type: .delete)] = changes.count - 1 - deletedRowIndices.insert(rowIndex) - changedRowIndices.insert(rowIndex) + pending.recordRowDeletion(rowIndex: rowIndex, originalRow: originalRow) registerUndo(actionName: String(localized: "Delete Row")) { target in target.applyDataUndo(.rowDeletion(rowIndex: rowIndex, originalRow: originalRow)) } @@ -322,20 +152,10 @@ final class DataChangeManager: ChangeManaging { } return } - - var batchData: [(rowIndex: Int, originalRow: [String?])] = [] - for (rowIndex, originalRow) in rows { - removeChangeByKey(rowIndex: rowIndex, type: .update) - modifiedCells.removeValue(forKey: rowIndex) - - let rowChange = RowChange(rowIndex: rowIndex, type: .delete, originalRow: originalRow) - changes.append(rowChange) - changeIndex[RowChangeKey(rowIndex: rowIndex, type: .delete)] = changes.count - 1 - deletedRowIndices.insert(rowIndex) - changedRowIndices.insert(rowIndex) - batchData.append((rowIndex: rowIndex, originalRow: originalRow)) + pending.recordRowDeletion(rowIndex: rowIndex, originalRow: originalRow) } + let batchData = rows registerUndo(actionName: String(localized: "Delete Rows")) { target in target.applyDataUndo(.batchRowDeletion(rows: batchData)) } @@ -344,12 +164,7 @@ final class DataChangeManager: ChangeManaging { } func recordRowInsertion(rowIndex: Int, values: [String?]) { - insertedRowData[rowIndex] = values - let rowChange = RowChange(rowIndex: rowIndex, type: .insert, cellChanges: []) - changes.append(rowChange) - changeIndex[RowChangeKey(rowIndex: rowIndex, type: .insert)] = changes.count - 1 - insertedRowIndices.insert(rowIndex) - changedRowIndices.insert(rowIndex) + pending.recordRowInsertion(rowIndex: rowIndex, values: values) registerUndo(actionName: String(localized: "Insert Row")) { target in target.applyDataUndo(.rowInsertion(rowIndex: rowIndex)) } @@ -360,338 +175,51 @@ final class DataChangeManager: ChangeManaging { // MARK: - Undo Operations func undoRowDeletion(rowIndex: Int) { - guard deletedRowIndices.contains(rowIndex) else { return } - removeChangeByKey(rowIndex: rowIndex, type: .delete) - deletedRowIndices.remove(rowIndex) - hasChanges = !changes.isEmpty + guard pending.undoRowDeletion(rowIndex: rowIndex) else { return } + hasChanges = !pending.isEmpty reloadVersion += 1 } func undoRowInsertion(rowIndex: Int) { - guard insertedRowIndices.contains(rowIndex) else { return } - - removeChangeByKey(rowIndex: rowIndex, type: .insert) - insertedRowIndices.remove(rowIndex) - insertedRowData.removeValue(forKey: rowIndex) - - var shiftedInsertedIndices = Set() - for idx in insertedRowIndices { - shiftedInsertedIndices.insert(idx > rowIndex ? idx - 1 : idx) - } - insertedRowIndices = shiftedInsertedIndices - - for i in 0.. rowIndex { - changes[i].rowIndex -= 1 - } - } - - var newInsertedRowData: [Int: [String?]] = [:] - for (key, value) in insertedRowData { - if key > rowIndex { - newInsertedRowData[key - 1] = value - } else { - newInsertedRowData[key] = value - } - } - insertedRowData = newInsertedRowData - - var newModifiedCells: [Int: Set] = [:] - for (key, value) in modifiedCells { - if key > rowIndex { - newModifiedCells[key - 1] = value - } else if key < rowIndex { - newModifiedCells[key] = value - } - } - modifiedCells = newModifiedCells - - rebuildChangeIndex() - hasChanges = !changes.isEmpty - } - - private func shiftRowIndicesUp(from insertionPoint: Int) { - for i in 0..= insertionPoint { - changes[i].rowIndex += 1 - } - } - - var shiftedInserted = Set() - for idx in insertedRowIndices { - shiftedInserted.insert(idx >= insertionPoint ? idx + 1 : idx) - } - insertedRowIndices = shiftedInserted - - var shiftedDeleted = Set() - for idx in deletedRowIndices { - shiftedDeleted.insert(idx >= insertionPoint ? idx + 1 : idx) - } - deletedRowIndices = shiftedDeleted - - var newInsertedRowData: [Int: [String?]] = [:] - for (key, value) in insertedRowData { - newInsertedRowData[key >= insertionPoint ? key + 1 : key] = value - } - insertedRowData = newInsertedRowData - - var newModifiedCells: [Int: Set] = [:] - for (key, value) in modifiedCells { - newModifiedCells[key >= insertionPoint ? key + 1 : key] = value - } - modifiedCells = newModifiedCells - - var newChangedRows = Set() - for idx in changedRowIndices { - newChangedRows.insert(idx >= insertionPoint ? idx + 1 : idx) - } - changedRowIndices = newChangedRows - - rebuildChangeIndex() + guard pending.undoRowInsertion(rowIndex: rowIndex) else { return } + hasChanges = !pending.isEmpty } func undoBatchRowInsertion(rowIndices: [Int]) { - guard !rowIndices.isEmpty else { return } - - let validRows = rowIndices.filter { insertedRowIndices.contains($0) } + let validRows = rowIndices.filter { pending.isRowInserted($0) } guard !validRows.isEmpty else { return } - - var rowValues: [[String?]] = [] - for rowIndex in validRows { - let key = RowChangeKey(rowIndex: rowIndex, type: .insert) - if let idx = changeIndex[key] { - let values = changes[idx].cellChanges.sorted { $0.columnIndex < $1.columnIndex } - .map { $0.newValue } - rowValues.append(values) - } else { - rowValues.append(Array(repeating: nil, count: columns.count)) - } - } - - for rowIndex in validRows { - removeChangeByKey(rowIndex: rowIndex, type: .insert) - insertedRowIndices.remove(rowIndex) - insertedRowData.removeValue(forKey: rowIndex) - } - + let rowValues = pending.undoBatchRowInsertion(rowIndices: validRows, columnCount: columns.count) registerUndo(actionName: String(localized: "Insert Rows")) { target in target.applyDataUndo(.batchRowInsertion(rowIndices: validRows, rowValues: rowValues)) } - - let sortedDeleted = validRows.sorted() - - var newInserted = Set() - for idx in insertedRowIndices { - let shiftCount = Self.countLessThan(idx, in: sortedDeleted) - newInserted.insert(idx - shiftCount) - } - insertedRowIndices = newInserted - - for i in 0.. [ParameterizedStatement] { try generateSQL( - for: changes, - insertedRowData: insertedRowData, - deletedRowIndices: deletedRowIndices, - insertedRowIndices: insertedRowIndices + for: pending.changes, + insertedRowData: pending.insertedRowData, + deletedRowIndices: pending.deletedRowIndices, + insertedRowIndices: pending.insertedRowIndices ) } @@ -886,77 +461,54 @@ final class DataChangeManager: ChangeManaging { func getOriginalValues() -> [(rowIndex: Int, columnIndex: Int, value: String?)] { var originals: [(rowIndex: Int, columnIndex: Int, value: String?)] = [] - - for change in changes { - if change.type == .update { - for cellChange in change.cellChanges { - originals.append(( - rowIndex: change.rowIndex, - columnIndex: cellChange.columnIndex, - value: cellChange.oldValue - )) - } + for change in pending.changes where change.type == .update { + for cellChange in change.cellChanges { + originals.append(( + rowIndex: change.rowIndex, + columnIndex: cellChange.columnIndex, + value: cellChange.oldValue + )) } } - return originals } func discardChanges() { - changes.removeAll() - changeIndex.removeAll() - deletedRowIndices.removeAll() - insertedRowIndices.removeAll() - modifiedCells.removeAll() - insertedRowData.removeAll() - changedRowIndices.removeAll() + pending.clear() hasChanges = false reloadVersion += 1 } // MARK: - Per-Tab State Management - func saveState() -> TabPendingChanges { - var state = TabPendingChanges() - state.changes = changes - state.deletedRowIndices = deletedRowIndices - state.insertedRowIndices = insertedRowIndices - state.modifiedCells = modifiedCells - state.insertedRowData = insertedRowData - state.primaryKeyColumns = primaryKeyColumns - state.columns = columns - return state + func saveState() -> TabChangeSnapshot { + pending.snapshot(primaryKeyColumns: primaryKeyColumns, columns: columns) } - func restoreState(from state: TabPendingChanges, tableName: String, databaseType: DatabaseType) { + func restoreState(from state: TabChangeSnapshot, tableName: String, databaseType: DatabaseType) { self.tableName = tableName self.columns = state.columns self.primaryKeyColumns = state.primaryKeyColumns self.databaseType = databaseType - self.changes = state.changes - self.deletedRowIndices = state.deletedRowIndices - self.insertedRowIndices = state.insertedRowIndices - self.modifiedCells = state.modifiedCells - self.insertedRowData = state.insertedRowData - self.hasChanges = !state.changes.isEmpty - rebuildChangeIndex() + pending.restore(from: state) + self.hasChanges = !pending.isEmpty } // MARK: - O(1) Lookups func isRowDeleted(_ rowIndex: Int) -> Bool { - deletedRowIndices.contains(rowIndex) + pending.isRowDeleted(rowIndex) } func isRowInserted(_ rowIndex: Int) -> Bool { - insertedRowIndices.contains(rowIndex) + pending.isRowInserted(rowIndex) } func isCellModified(rowIndex: Int, columnIndex: Int) -> Bool { - modifiedCells[rowIndex]?.contains(columnIndex) == true + pending.isCellModified(rowIndex: rowIndex, columnIndex: columnIndex) } func getModifiedColumnsForRow(_ rowIndex: Int) -> Set { - modifiedCells[rowIndex] ?? [] + pending.modifiedColumns(forRow: rowIndex) } } diff --git a/TablePro/Core/ChangeTracking/DataChangeModels.swift b/TablePro/Core/ChangeTracking/DataChangeModels.swift index b0f15b120..2438b2781 100644 --- a/TablePro/Core/ChangeTracking/DataChangeModels.swift +++ b/TablePro/Core/ChangeTracking/DataChangeModels.swift @@ -86,7 +86,7 @@ enum UndoAction { case batchRowInsertion(rowIndices: [Int], rowValues: [[String?]]) } -// Note: TabPendingChanges is defined in QueryTab.swift +// Note: TabChangeSnapshot is defined in QueryTab.swift // MARK: - Array Extension diff --git a/TablePro/Core/ChangeTracking/PendingChanges.swift b/TablePro/Core/ChangeTracking/PendingChanges.swift new file mode 100644 index 000000000..8e3104832 --- /dev/null +++ b/TablePro/Core/ChangeTracking/PendingChanges.swift @@ -0,0 +1,513 @@ +// +// PendingChanges.swift +// TablePro +// +// Value type holding all uncommitted edits to a result set. +// Owns the consistency invariants between `changes`, `changeIndex`, +// `deletedRowIndices`, `insertedRowIndices`, `modifiedCells`, and +// `insertedRowData`. Callers mutate through methods that maintain +// the cross-collection state. +// + +import Foundation + +struct PendingChanges: Equatable { + private(set) var changes: [RowChange] = [] + private(set) var deletedRowIndices: Set = [] + private(set) var insertedRowIndices: Set = [] + private(set) var modifiedCells: [Int: Set] = [:] + private(set) var insertedRowData: [Int: [String?]] = [:] + private(set) var changedRowIndices: Set = [] + + private var changeIndex: [RowChangeKey: Int] = [:] + + var isEmpty: Bool { changes.isEmpty } + var hasChanges: Bool { !isEmpty } + + // MARK: - Read + + func isRowDeleted(_ rowIndex: Int) -> Bool { + deletedRowIndices.contains(rowIndex) + } + + func isRowInserted(_ rowIndex: Int) -> Bool { + insertedRowIndices.contains(rowIndex) + } + + func isCellModified(rowIndex: Int, columnIndex: Int) -> Bool { + modifiedCells[rowIndex]?.contains(columnIndex) == true + } + + func modifiedColumns(forRow rowIndex: Int) -> Set { + modifiedCells[rowIndex] ?? [] + } + + func change(forRow rowIndex: Int, type: ChangeType) -> RowChange? { + guard let idx = changeIndex[RowChangeKey(rowIndex: rowIndex, type: type)] else { return nil } + return changes[idx] + } + + // MARK: - Mutate (recording user edits) + + /// Whether the recorded edit is a no-op (oldValue == newValue with no prior modification). + /// Returns the result so the caller can decide whether to register undo. + @discardableResult + mutating func recordCellChange( + rowIndex: Int, + columnIndex: Int, + columnName: String, + oldValue: String?, + newValue: String?, + originalRow: [String?]? = nil + ) -> Bool { + if oldValue == newValue { + return rollbackCellIfMatchesOriginal( + rowIndex: rowIndex, columnIndex: columnIndex, restoredValue: newValue + ) + } + + let cellChange = CellChange( + rowIndex: rowIndex, + columnIndex: columnIndex, + columnName: columnName, + oldValue: oldValue, + newValue: newValue + ) + + if let insertIdx = changeIndex[RowChangeKey(rowIndex: rowIndex, type: .insert)] { + updateInsertedCell(at: insertIdx, columnIndex: columnIndex, + columnName: columnName, newValue: newValue) + changedRowIndices.insert(rowIndex) + return true + } + + let updateKey = RowChangeKey(rowIndex: rowIndex, type: .update) + if let updateIdx = changeIndex[updateKey] { + mergeUpdateCell(at: updateIdx, cellChange: cellChange) + } else { + let row = RowChange( + rowIndex: rowIndex, type: .update, + cellChanges: [cellChange], originalRow: originalRow + ) + changes.append(row) + changeIndex[updateKey] = changes.count - 1 + modifiedCells[rowIndex, default: []].insert(columnIndex) + } + changedRowIndices.insert(rowIndex) + return true + } + + mutating func recordRowDeletion(rowIndex: Int, originalRow: [String?]) { + removeChange(rowIndex: rowIndex, type: .update) + modifiedCells.removeValue(forKey: rowIndex) + appendChange(RowChange(rowIndex: rowIndex, type: .delete, originalRow: originalRow)) + deletedRowIndices.insert(rowIndex) + changedRowIndices.insert(rowIndex) + } + + mutating func recordRowInsertion(rowIndex: Int, values: [String?]) { + insertedRowData[rowIndex] = values + appendChange(RowChange(rowIndex: rowIndex, type: .insert, cellChanges: [])) + insertedRowIndices.insert(rowIndex) + changedRowIndices.insert(rowIndex) + } + + // MARK: - Mutate (undoing recorded edits) + + mutating func undoRowDeletion(rowIndex: Int) -> Bool { + guard deletedRowIndices.contains(rowIndex) else { return false } + removeChange(rowIndex: rowIndex, type: .delete) + deletedRowIndices.remove(rowIndex) + return true + } + + mutating func undoRowInsertion(rowIndex: Int) -> Bool { + guard insertedRowIndices.contains(rowIndex) else { return false } + + removeChange(rowIndex: rowIndex, type: .insert) + insertedRowIndices.remove(rowIndex) + insertedRowData.removeValue(forKey: rowIndex) + + shiftRowIndicesDown(at: rowIndex) + return true + } + + /// Undo a batch of inserted rows. Returns the saved values for each row in the same order. + mutating func undoBatchRowInsertion(rowIndices: [Int], columnCount: Int) -> [[String?]] { + let validRows = rowIndices.filter { insertedRowIndices.contains($0) } + + var rowValues: [[String?]] = [] + for rowIndex in validRows { + if let idx = changeIndex[RowChangeKey(rowIndex: rowIndex, type: .insert)] { + let values = changes[idx].cellChanges + .sorted { $0.columnIndex < $1.columnIndex } + .map { $0.newValue } + rowValues.append(values) + } else { + rowValues.append(Array(repeating: nil, count: columnCount)) + } + } + + for rowIndex in validRows { + removeChange(rowIndex: rowIndex, type: .insert) + insertedRowIndices.remove(rowIndex) + insertedRowData.removeValue(forKey: rowIndex) + } + + let sortedRemoved = validRows.sorted() + + var newInserted = Set() + for idx in insertedRowIndices { + newInserted.insert(idx - Self.countLessThan(idx, in: sortedRemoved)) + } + insertedRowIndices = newInserted + + for i in 0.. [String?]? { + insertedRowData[rowIndex] + } + + /// Restore inserted-row values when undo restores a row. + mutating func restoreInsertedValues(forRow rowIndex: Int, values: [String?]) { + insertedRowData[rowIndex] = values + } + + // MARK: - Reset / persistence + + mutating func clear() { + changes.removeAll() + changeIndex.removeAll() + deletedRowIndices.removeAll() + insertedRowIndices.removeAll() + modifiedCells.removeAll() + insertedRowData.removeAll() + changedRowIndices.removeAll() + } + + mutating func consumeChangedRowIndices() -> Set { + let indices = changedRowIndices + changedRowIndices.removeAll() + return indices + } + + /// Replace internal state from a serialized snapshot. + mutating func restore(from snapshot: TabChangeSnapshot) { + changes = snapshot.changes + deletedRowIndices = snapshot.deletedRowIndices + insertedRowIndices = snapshot.insertedRowIndices + modifiedCells = snapshot.modifiedCells + insertedRowData = snapshot.insertedRowData + changedRowIndices = [] + rebuildChangeIndex() + } + + func snapshot(primaryKeyColumns: [String], columns: [String]) -> TabChangeSnapshot { + var snap = TabChangeSnapshot() + snap.changes = changes + snap.deletedRowIndices = deletedRowIndices + snap.insertedRowIndices = insertedRowIndices + snap.modifiedCells = modifiedCells + snap.insertedRowData = insertedRowData + snap.primaryKeyColumns = primaryKeyColumns + snap.columns = columns + return snap + } + + // MARK: - Internals + + private mutating func appendChange(_ change: RowChange) { + changes.append(change) + changeIndex[RowChangeKey(rowIndex: change.rowIndex, type: change.type)] = changes.count - 1 + } + + @discardableResult + private mutating func removeChange(rowIndex: Int, type: ChangeType) -> Bool { + let key = RowChangeKey(rowIndex: rowIndex, type: type) + guard let arrayIndex = changeIndex[key] else { return false } + removeChangeAt(arrayIndex) + return true + } + + private mutating func removeChangeAt(_ arrayIndex: Int) { + let removed = changes[arrayIndex] + changeIndex.removeValue(forKey: RowChangeKey(rowIndex: removed.rowIndex, type: removed.type)) + changes.remove(at: arrayIndex) + + for (key, idx) in changeIndex where idx > arrayIndex { + changeIndex[key] = idx - 1 + } + } + + private mutating func rebuildChangeIndex() { + changeIndex.removeAll(keepingCapacity: true) + for (index, change) in changes.enumerated() { + changeIndex[RowChangeKey(rowIndex: change.rowIndex, type: change.type)] = index + } + } + + private mutating func updateInsertedCell( + at insertIdx: Int, columnIndex: Int, columnName: String, newValue: String? + ) { + let rowIndex = changes[insertIdx].rowIndex + if var stored = insertedRowData[rowIndex], columnIndex < stored.count { + stored[columnIndex] = newValue + insertedRowData[rowIndex] = stored + } + + let replacement = CellChange( + rowIndex: rowIndex, columnIndex: columnIndex, columnName: columnName, + oldValue: nil, newValue: newValue + ) + if let cellIdx = changes[insertIdx].cellChanges.firstIndex(where: { $0.columnIndex == columnIndex }) { + changes[insertIdx].cellChanges[cellIdx] = replacement + } else { + changes[insertIdx].cellChanges.append(replacement) + } + } + + private mutating func mergeUpdateCell(at updateIdx: Int, cellChange: CellChange) { + let rowIndex = changes[updateIdx].rowIndex + if let cellIdx = changes[updateIdx].cellChanges.firstIndex(where: { + $0.columnIndex == cellChange.columnIndex + }) { + let originalOldValue = changes[updateIdx].cellChanges[cellIdx].oldValue + let merged = CellChange( + rowIndex: rowIndex, + columnIndex: cellChange.columnIndex, + columnName: cellChange.columnName, + oldValue: originalOldValue, + newValue: cellChange.newValue + ) + changes[updateIdx].cellChanges[cellIdx] = merged + + if originalOldValue == cellChange.newValue { + changes[updateIdx].cellChanges.remove(at: cellIdx) + modifiedCells[rowIndex]?.remove(cellChange.columnIndex) + if modifiedCells[rowIndex]?.isEmpty == true { + modifiedCells.removeValue(forKey: rowIndex) + } + if changes[updateIdx].cellChanges.isEmpty { + removeChangeAt(updateIdx) + } + } + } else { + changes[updateIdx].cellChanges.append(cellChange) + modifiedCells[rowIndex, default: []].insert(cellChange.columnIndex) + } + } + + @discardableResult + private mutating func rollbackCellIfMatchesOriginal( + rowIndex: Int, columnIndex: Int, restoredValue: String? + ) -> Bool { + let updateKey = RowChangeKey(rowIndex: rowIndex, type: .update) + guard let updateIdx = changeIndex[updateKey], + let cellIdx = changes[updateIdx].cellChanges.firstIndex(where: { $0.columnIndex == columnIndex }), + changes[updateIdx].cellChanges[cellIdx].oldValue == restoredValue else { + return false + } + changes[updateIdx].cellChanges.remove(at: cellIdx) + modifiedCells[rowIndex]?.remove(columnIndex) + if modifiedCells[rowIndex]?.isEmpty == true { + modifiedCells.removeValue(forKey: rowIndex) + } + if changes[updateIdx].cellChanges.isEmpty { + removeChangeAt(updateIdx) + } + changedRowIndices.insert(rowIndex) + return true + } + + private mutating func shiftRowIndicesUp(from insertionPoint: Int) { + for i in 0..= insertionPoint { + changes[i].rowIndex += 1 + } + insertedRowIndices = Set(insertedRowIndices.map { $0 >= insertionPoint ? $0 + 1 : $0 }) + deletedRowIndices = Set(deletedRowIndices.map { $0 >= insertionPoint ? $0 + 1 : $0 }) + + var newInsertedRowData: [Int: [String?]] = [:] + for (key, value) in insertedRowData { + newInsertedRowData[key >= insertionPoint ? key + 1 : key] = value + } + insertedRowData = newInsertedRowData + + var newModifiedCells: [Int: Set] = [:] + for (key, value) in modifiedCells { + newModifiedCells[key >= insertionPoint ? key + 1 : key] = value + } + modifiedCells = newModifiedCells + + changedRowIndices = Set(changedRowIndices.map { $0 >= insertionPoint ? $0 + 1 : $0 }) + rebuildChangeIndex() + } + + private mutating func shiftRowIndicesDown(at removedRow: Int) { + for i in 0.. removedRow { + changes[i].rowIndex -= 1 + } + insertedRowIndices = Set(insertedRowIndices.map { $0 > removedRow ? $0 - 1 : $0 }) + + var newInsertedRowData: [Int: [String?]] = [:] + for (key, value) in insertedRowData { + newInsertedRowData[key > removedRow ? key - 1 : key] = value + } + insertedRowData = newInsertedRowData + + var newModifiedCells: [Int: Set] = [:] + for (key, value) in modifiedCells where key != removedRow { + newModifiedCells[key > removedRow ? key - 1 : key] = value + } + modifiedCells = newModifiedCells + rebuildChangeIndex() + } + + /// Binary search: count of elements strictly less than `target` in a sorted array. + private static func countLessThan(_ target: Int, in sorted: [Int]) -> Int { + var lo = 0, hi = sorted.count + while lo < hi { + let mid = (lo + hi) / 2 + if sorted[mid] < target { + lo = mid + 1 + } else { + hi = mid + } + } + return lo + } +} diff --git a/TablePro/Models/Query/QueryTab.swift b/TablePro/Models/Query/QueryTab.swift index fc43cd4b7..01e533363 100644 --- a/TablePro/Models/Query/QueryTab.swift +++ b/TablePro/Models/Query/QueryTab.swift @@ -20,7 +20,7 @@ struct QueryTab: Identifiable, Equatable { var tableContext: TabTableContext var display: TabDisplayState - var pendingChanges: TabPendingChanges + var pendingChanges: TabChangeSnapshot var selectedRowIndices: Set var sortState: SortState var filterState: TabFilterState @@ -46,7 +46,7 @@ struct QueryTab: Identifiable, Equatable { self.execution = TabExecutionState() self.tableContext = TabTableContext(tableName: tableName, isEditable: tabType == .table) self.display = TabDisplayState() - self.pendingChanges = TabPendingChanges() + self.pendingChanges = TabChangeSnapshot() self.selectedRowIndices = [] self.sortState = SortState() self.filterState = TabFilterState() @@ -77,7 +77,7 @@ struct QueryTab: Identifiable, Equatable { isView: persisted.isView ) self.display = TabDisplayState(erDiagramSchemaKey: persisted.erDiagramSchemaKey) - self.pendingChanges = TabPendingChanges() + self.pendingChanges = TabChangeSnapshot() self.selectedRowIndices = [] self.sortState = SortState() self.filterState = TabFilterState() diff --git a/TablePro/Models/Query/QueryTabManager.swift b/TablePro/Models/Query/QueryTabManager.swift index 72e40a731..7fd593021 100644 --- a/TablePro/Models/Query/QueryTabManager.swift +++ b/TablePro/Models/Query/QueryTabManager.swift @@ -230,7 +230,7 @@ final class QueryTabManager { tab.display.resultsViewMode = .data tab.sortState = SortState() tab.selectedRowIndices = [] - tab.pendingChanges = TabPendingChanges() + tab.pendingChanges = TabChangeSnapshot() tab.hasUserInteraction = false tab.tableContext.isView = isView tab.tableContext.isEditable = !isView diff --git a/TablePro/Models/Query/QueryTabState.swift b/TablePro/Models/Query/QueryTabState.swift index a61971f63..8f18d4059 100644 --- a/TablePro/Models/Query/QueryTabState.swift +++ b/TablePro/Models/Query/QueryTabState.swift @@ -35,8 +35,7 @@ struct PersistedTab: Codable { var queryParameters: [QueryParameter]? } -/// Stores pending changes for a tab (used to preserve state when switching tabs) -struct TabPendingChanges: Equatable { +struct TabChangeSnapshot: Equatable { var changes: [RowChange] var deletedRowIndices: Set var insertedRowIndices: Set diff --git a/TablePro/Views/Main/Extensions/MainContentCoordinator+Discard.swift b/TablePro/Views/Main/Extensions/MainContentCoordinator+Discard.swift index 93ebc63fe..5136b79aa 100644 --- a/TablePro/Views/Main/Extensions/MainContentCoordinator+Discard.swift +++ b/TablePro/Views/Main/Extensions/MainContentCoordinator+Discard.swift @@ -98,7 +98,7 @@ extension MainContentCoordinator { changeManager.clearChangesAndUndoHistory() if let index = tabManager.selectedTabIndex { - tabManager.tabs[index].pendingChanges = TabPendingChanges() + tabManager.tabs[index].pendingChanges = TabChangeSnapshot() } Task { await refreshTables() } diff --git a/TablePro/Views/Main/Extensions/MainContentCoordinator+SaveChanges.swift b/TablePro/Views/Main/Extensions/MainContentCoordinator+SaveChanges.swift index 2446cee9d..d9909f5c6 100644 --- a/TablePro/Views/Main/Extensions/MainContentCoordinator+SaveChanges.swift +++ b/TablePro/Views/Main/Extensions/MainContentCoordinator+SaveChanges.swift @@ -232,7 +232,7 @@ extension MainContentCoordinator { changeManager.clearChangesAndUndoHistory() if let index = tabManager.selectedTabIndex { - tabManager.tabs[index].pendingChanges = TabPendingChanges() + tabManager.tabs[index].pendingChanges = TabChangeSnapshot() tabManager.tabs[index].execution.errorMessage = nil } diff --git a/TableProTests/Core/ChangeTracking/DataChangeManagerExtendedTests.swift b/TableProTests/Core/ChangeTracking/DataChangeManagerExtendedTests.swift index 594573387..b0c103665 100644 --- a/TableProTests/Core/ChangeTracking/DataChangeManagerExtendedTests.swift +++ b/TableProTests/Core/ChangeTracking/DataChangeManagerExtendedTests.swift @@ -17,6 +17,10 @@ struct DataChangeManagerExtendedTests { pk: String? = "id" ) -> DataChangeManager { let manager = DataChangeManager() + let undoManager = UndoManager() + undoManager.levelsOfUndo = 100 + undoManager.groupsByEvent = false + manager.undoManagerProvider = { undoManager } manager.configureForTable( tableName: "test_table", columns: columns, @@ -676,9 +680,10 @@ struct DataChangeManagerExtendedTests { ) manager.recordRowDeletion(rowIndex: 1, originalRow: ["2", "Charlie", "c@test.com"]) manager.recordRowInsertion(rowIndex: 2, values: ["x", "y", "z"]) - #expect(manager.changedRowIndices.contains(0)) - #expect(manager.changedRowIndices.contains(1)) - #expect(manager.changedRowIndices.contains(2)) + let changed = manager.consumeChangedRowIndices() + #expect(changed.contains(0)) + #expect(changed.contains(1)) + #expect(changed.contains(2)) } @Test("configureForTable with triggerReload false does not increment reloadVersion") diff --git a/TableProTests/Core/ChangeTracking/DataChangeManagerTests.swift b/TableProTests/Core/ChangeTracking/DataChangeManagerTests.swift index afdc8c7ed..86b8d3328 100644 --- a/TableProTests/Core/ChangeTracking/DataChangeManagerTests.swift +++ b/TableProTests/Core/ChangeTracking/DataChangeManagerTests.swift @@ -12,6 +12,14 @@ import Testing @MainActor @Suite("Data Change Manager") struct DataChangeManagerTests { + private func makeManagerWithUndo() -> DataChangeManager { + let manager = DataChangeManager() + let undoManager = UndoManager() + undoManager.groupsByEvent = false + manager.undoManagerProvider = { undoManager } + return manager + } + // MARK: - Configuration Tests @Test("configureForTable sets properties correctly") @@ -246,7 +254,7 @@ struct DataChangeManagerTests { newValue: "Bob" ) - #expect(manager.changedRowIndices.contains(5)) + #expect(manager.consumeChangedRowIndices().contains(5)) } // MARK: - Row Deletion Tests @@ -382,7 +390,7 @@ struct DataChangeManagerTests { _ = manager.consumeChangedRowIndices() - #expect(manager.changedRowIndices.isEmpty) + #expect(manager.consumeChangedRowIndices().isEmpty) } // MARK: - clearChanges Tests @@ -439,7 +447,7 @@ struct DataChangeManagerTests { @Test("After recording a change, canUndo is true") func canUndoAfterChange() async { - let manager = DataChangeManager() + let manager = makeManagerWithUndo() manager.configureForTable( tableName: "users", columns: ["id", "name"], @@ -459,7 +467,7 @@ struct DataChangeManagerTests { @Test("After undo, the change is reversed") func undoReversesChange() async { - let manager = DataChangeManager() + let manager = makeManagerWithUndo() manager.configureForTable( tableName: "users", columns: ["id", "name"], @@ -483,7 +491,7 @@ struct DataChangeManagerTests { @Test("canRedo after undo") func canRedoAfterUndo() async { - let manager = DataChangeManager() + let manager = makeManagerWithUndo() manager.configureForTable( tableName: "users", columns: ["id", "name"], @@ -505,7 +513,7 @@ struct DataChangeManagerTests { @Test("New change clears redo stack") func newChangeClearsRedo() async { - let manager = DataChangeManager() + let manager = makeManagerWithUndo() manager.configureForTable( tableName: "users", columns: ["id", "name"], diff --git a/TableProTests/Core/ChangeTracking/DataChangeModelsTests.swift b/TableProTests/Core/ChangeTracking/DataChangeModelsTests.swift index ffda0bca1..dd73727be 100644 --- a/TableProTests/Core/ChangeTracking/DataChangeModelsTests.swift +++ b/TableProTests/Core/ChangeTracking/DataChangeModelsTests.swift @@ -116,9 +116,9 @@ struct DataChangeModelsTests { #expect(rowChange.type == .insert) } - @Test("TabPendingChanges initializes as empty") + @Test("TabChangeSnapshot initializes as empty") func tabPendingChangesInit() { - let pending = TabPendingChanges() + let pending = TabChangeSnapshot() #expect(pending.changes.isEmpty) #expect(pending.deletedRowIndices.isEmpty) @@ -129,37 +129,37 @@ struct DataChangeModelsTests { #expect(pending.columns.isEmpty) } - @Test("TabPendingChanges hasChanges is false when empty") + @Test("TabChangeSnapshot hasChanges is false when empty") func tabPendingChangesHasChangesEmpty() { - let pending = TabPendingChanges() + let pending = TabChangeSnapshot() #expect(!pending.hasChanges) } - @Test("TabPendingChanges hasChanges is true with changes") + @Test("TabChangeSnapshot hasChanges is true with changes") func tabPendingChangesHasChangesWithChanges() { let rowChange = RowChange( rowIndex: 0, type: .update ) - var pending = TabPendingChanges() + var pending = TabChangeSnapshot() pending.changes = [rowChange] #expect(pending.hasChanges) } - @Test("TabPendingChanges hasChanges is true with deletedRowIndices") + @Test("TabChangeSnapshot hasChanges is true with deletedRowIndices") func tabPendingChangesHasChangesWithDeleted() { - var pending = TabPendingChanges() + var pending = TabChangeSnapshot() pending.deletedRowIndices = [1, 2, 3] #expect(pending.hasChanges) } - @Test("TabPendingChanges hasChanges is true with insertedRowIndices") + @Test("TabChangeSnapshot hasChanges is true with insertedRowIndices") func tabPendingChangesHasChangesWithInserted() { - var pending = TabPendingChanges() + var pending = TabChangeSnapshot() pending.insertedRowIndices = [0, 1] #expect(pending.hasChanges) diff --git a/TableProTests/Core/ChangeTracking/PendingChangesTests.swift b/TableProTests/Core/ChangeTracking/PendingChangesTests.swift new file mode 100644 index 000000000..6ae00ed14 --- /dev/null +++ b/TableProTests/Core/ChangeTracking/PendingChangesTests.swift @@ -0,0 +1,221 @@ +// +// PendingChangesTests.swift +// TableProTests +// + +import Foundation +@testable import TablePro +import Testing + +@Suite("PendingChanges - record") +struct PendingChangesRecordTests { + @Test("Empty by default") + func emptyByDefault() { + let pending = PendingChanges() + #expect(pending.isEmpty) + #expect(pending.changes.isEmpty) + } + + @Test("Recording cell edit creates an update change") + func recordCellCreatesUpdate() { + var pending = PendingChanges() + let recorded = pending.recordCellChange( + rowIndex: 0, columnIndex: 1, columnName: "name", + oldValue: "a", newValue: "b" + ) + #expect(recorded == true) + #expect(pending.changes.count == 1) + #expect(pending.changes[0].type == .update) + #expect(pending.isCellModified(rowIndex: 0, columnIndex: 1)) + #expect(pending.modifiedColumns(forRow: 0) == [1]) + } + + @Test("No-op edit when oldValue equals newValue and no prior change") + func noOpEdit() { + var pending = PendingChanges() + let recorded = pending.recordCellChange( + rowIndex: 0, columnIndex: 1, columnName: "name", + oldValue: "a", newValue: "a" + ) + #expect(recorded == false) + #expect(pending.isEmpty) + } + + @Test("Editing back to original value collapses the change") + func revertToOriginalCollapses() { + var pending = PendingChanges() + pending.recordCellChange( + rowIndex: 0, columnIndex: 1, columnName: "name", + oldValue: "a", newValue: "b" + ) + let collapsed = pending.recordCellChange( + rowIndex: 0, columnIndex: 1, columnName: "name", + oldValue: "b", newValue: "a" + ) + #expect(collapsed == true) + #expect(pending.isEmpty) + #expect(!pending.isCellModified(rowIndex: 0, columnIndex: 1)) + } + + @Test("Recording row deletion adds delete change and marks row deleted") + func recordRowDeletion() { + var pending = PendingChanges() + pending.recordRowDeletion(rowIndex: 5, originalRow: ["a", "b"]) + #expect(pending.isRowDeleted(5)) + #expect(pending.changes.count == 1) + #expect(pending.changes[0].type == .delete) + } + + @Test("Deleting a row clears its prior cell edits") + func deletionRemovesUpdate() { + var pending = PendingChanges() + pending.recordCellChange( + rowIndex: 0, columnIndex: 1, columnName: "name", + oldValue: "a", newValue: "b" + ) + pending.recordRowDeletion(rowIndex: 0, originalRow: ["a", nil]) + #expect(pending.changes.count == 1) + #expect(pending.changes[0].type == .delete) + #expect(!pending.isCellModified(rowIndex: 0, columnIndex: 1)) + } + + @Test("Recording row insertion marks row inserted") + func recordRowInsertion() { + var pending = PendingChanges() + pending.recordRowInsertion(rowIndex: 3, values: ["x", "y"]) + #expect(pending.isRowInserted(3)) + #expect(pending.savedInsertedValues(forRow: 3) == ["x", "y"]) + } +} + +@Suite("PendingChanges - undo") +struct PendingChangesUndoTests { + @Test("Undo row deletion clears delete state") + func undoRowDeletion() { + var pending = PendingChanges() + pending.recordRowDeletion(rowIndex: 0, originalRow: ["a"]) + let undone = pending.undoRowDeletion(rowIndex: 0) + #expect(undone == true) + #expect(!pending.isRowDeleted(0)) + #expect(pending.isEmpty) + } + + @Test("Undo row insertion shifts later inserted rows down") + func undoRowInsertionShiftsOthers() { + var pending = PendingChanges() + pending.recordRowInsertion(rowIndex: 1, values: ["a"]) + pending.recordRowInsertion(rowIndex: 2, values: ["b"]) + pending.recordRowInsertion(rowIndex: 3, values: ["c"]) + + let undone = pending.undoRowInsertion(rowIndex: 2) + #expect(undone == true) + #expect(pending.isRowInserted(1)) + #expect(pending.isRowInserted(2)) + #expect(!pending.isRowInserted(3)) + } + + @Test("Undo on row that was not inserted is a no-op") + func undoNonexistentInsertion() { + var pending = PendingChanges() + let undone = pending.undoRowInsertion(rowIndex: 99) + #expect(undone == false) + } + + @Test("Undo batch row insertion returns saved values in order") + func undoBatchRowInsertion() { + var pending = PendingChanges() + pending.recordRowInsertion(rowIndex: 1, values: ["a"]) + pending.recordRowInsertion(rowIndex: 2, values: ["b"]) + pending.recordRowInsertion(rowIndex: 3, values: ["c"]) + + let restored = pending.undoBatchRowInsertion(rowIndices: [1, 2, 3], columnCount: 1) + #expect(restored.count == 3) + #expect(!pending.isRowInserted(1)) + #expect(!pending.isRowInserted(2)) + #expect(!pending.isRowInserted(3)) + } +} + +@Suite("PendingChanges - replay") +struct PendingChangesReplayTests { + @Test("Reapply cell change with no existing change") + func reapplyCellWithoutExisting() { + var pending = PendingChanges() + pending.reapplyCellChange( + rowIndex: 0, columnIndex: 1, columnName: "name", + newValue: "x", originalRow: nil + ) + #expect(pending.isCellModified(rowIndex: 0, columnIndex: 1)) + } + + @Test("Reinsert row creates insert change with saved values") + func reinsertRowFromUndo() { + var pending = PendingChanges() + pending.reinsertRow(rowIndex: 2, columns: ["a", "b"], savedValues: ["x", "y"]) + #expect(pending.isRowInserted(2)) + #expect(pending.savedInsertedValues(forRow: 2) == ["x", "y"]) + } + + @Test("Reapply row deletion adds delete change") + func reapplyDeletion() { + var pending = PendingChanges() + pending.reapplyRowDeletion(rowIndex: 0, originalRow: ["a", "b"]) + #expect(pending.isRowDeleted(0)) + } +} + +@Suite("PendingChanges - snapshot") +struct PendingChangesSnapshotTests { + @Test("Snapshot round-trip preserves state") + func snapshotRoundTrip() { + var pending = PendingChanges() + pending.recordCellChange( + rowIndex: 0, columnIndex: 1, columnName: "name", + oldValue: "a", newValue: "b" + ) + pending.recordRowDeletion(rowIndex: 5, originalRow: ["x"]) + pending.recordRowInsertion(rowIndex: 7, values: ["new"]) + + let snapshot = pending.snapshot(primaryKeyColumns: ["id"], columns: ["id", "name"]) + + var restored = PendingChanges() + restored.restore(from: snapshot) + + #expect(restored.changes.count == pending.changes.count) + #expect(restored.isRowDeleted(5)) + #expect(restored.isRowInserted(7)) + #expect(restored.isCellModified(rowIndex: 0, columnIndex: 1)) + } +} + +@Suite("PendingChanges - clear and consume") +struct PendingChangesLifecycleTests { + @Test("Clear empties all internal state") + func clearResets() { + var pending = PendingChanges() + pending.recordCellChange( + rowIndex: 0, columnIndex: 1, columnName: "name", + oldValue: "a", newValue: "b" + ) + pending.recordRowDeletion(rowIndex: 5, originalRow: ["x"]) + pending.clear() + + #expect(pending.isEmpty) + #expect(pending.changes.isEmpty) + #expect(!pending.isRowDeleted(5)) + #expect(!pending.isCellModified(rowIndex: 0, columnIndex: 1)) + } + + @Test("Consuming changedRowIndices empties the set") + func consumeChangedRows() { + var pending = PendingChanges() + pending.recordCellChange( + rowIndex: 3, columnIndex: 1, columnName: "name", + oldValue: "a", newValue: "b" + ) + let first = pending.consumeChangedRowIndices() + #expect(first == [3]) + let second = pending.consumeChangedRowIndices() + #expect(second.isEmpty) + } +} From 86dfe472e299573e3cdf849b3d9697fcbe13d0a3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ng=C3=B4=20Qu=E1=BB=91c=20=C4=90=E1=BA=A1t?= Date: Tue, 28 Apr 2026 16:42:05 +0700 Subject: [PATCH 2/5] refactor(datagrid): address PendingChanges review feedback - Restore old behavior in applyCellEditUndo: when an update change exists for the row but has no cellChange for the column, leave the update unchanged. The new code was creating a stale cellChange entry. - Guard recordRowDeletion / reapplyRowDeletion / recordRowInsertion against double-recording. Two calls for the same row no longer corrupt the changeIndex. - Add MARK groupings: cancel pending vs. replay (NSUndoManager-driven). - Two new tests covering double-deletion / double-insertion idempotency. --- .../ChangeTracking/DataChangeManager.swift | 8 +++----- .../Core/ChangeTracking/PendingChanges.swift | 10 +++++++++- .../ChangeTracking/PendingChangesTests.swift | 19 +++++++++++++++++++ 3 files changed, 31 insertions(+), 6 deletions(-) diff --git a/TablePro/Core/ChangeTracking/DataChangeManager.swift b/TablePro/Core/ChangeTracking/DataChangeManager.swift index 749e7d600..f9a93f12b 100644 --- a/TablePro/Core/ChangeTracking/DataChangeManager.swift +++ b/TablePro/Core/ChangeTracking/DataChangeManager.swift @@ -245,12 +245,10 @@ final class DataChangeManager: ChangeManaging { rowIndex: rowIndex, columnIndex: columnIndex, columnName: columnName, previousValue: previousValue ) - } else { - pending.reapplyCellChange( - rowIndex: rowIndex, columnIndex: columnIndex, columnName: columnName, - newValue: previousValue, originalRow: originalRow - ) } + // No matching cellChange: leave the update unchanged. Reaching here means + // the row's update lost track of this column (e.g. earlier collapse), so + // creating a new entry would resurrect a stale edit. } else if pending.change(forRow: rowIndex, type: .insert) != nil { pending.updateInsertedCellDirectly( rowIndex: rowIndex, columnIndex: columnIndex, diff --git a/TablePro/Core/ChangeTracking/PendingChanges.swift b/TablePro/Core/ChangeTracking/PendingChanges.swift index 8e3104832..95970f725 100644 --- a/TablePro/Core/ChangeTracking/PendingChanges.swift +++ b/TablePro/Core/ChangeTracking/PendingChanges.swift @@ -98,6 +98,7 @@ struct PendingChanges: Equatable { } mutating func recordRowDeletion(rowIndex: Int, originalRow: [String?]) { + guard !deletedRowIndices.contains(rowIndex) else { return } removeChange(rowIndex: rowIndex, type: .update) modifiedCells.removeValue(forKey: rowIndex) appendChange(RowChange(rowIndex: rowIndex, type: .delete, originalRow: originalRow)) @@ -106,13 +107,17 @@ struct PendingChanges: Equatable { } mutating func recordRowInsertion(rowIndex: Int, values: [String?]) { + guard !insertedRowIndices.contains(rowIndex) else { + insertedRowData[rowIndex] = values + return + } insertedRowData[rowIndex] = values appendChange(RowChange(rowIndex: rowIndex, type: .insert, cellChanges: [])) insertedRowIndices.insert(rowIndex) changedRowIndices.insert(rowIndex) } - // MARK: - Mutate (undoing recorded edits) + // MARK: - Mutate (cancelling pending edits) mutating func undoRowDeletion(rowIndex: Int) -> Bool { guard deletedRowIndices.contains(rowIndex) else { return false } @@ -171,8 +176,11 @@ struct PendingChanges: Equatable { return rowValues } + // MARK: - Replay (driven by NSUndoManager invocation) + /// Re-apply a deletion during undo replay (skips undo registration). mutating func reapplyRowDeletion(rowIndex: Int, originalRow: [String?]) { + guard !deletedRowIndices.contains(rowIndex) else { return } removeChange(rowIndex: rowIndex, type: .update) modifiedCells.removeValue(forKey: rowIndex) appendChange(RowChange(rowIndex: rowIndex, type: .delete, originalRow: originalRow)) diff --git a/TableProTests/Core/ChangeTracking/PendingChangesTests.swift b/TableProTests/Core/ChangeTracking/PendingChangesTests.swift index 6ae00ed14..0a6b8c0ee 100644 --- a/TableProTests/Core/ChangeTracking/PendingChangesTests.swift +++ b/TableProTests/Core/ChangeTracking/PendingChangesTests.swift @@ -86,6 +86,25 @@ struct PendingChangesRecordTests { #expect(pending.isRowInserted(3)) #expect(pending.savedInsertedValues(forRow: 3) == ["x", "y"]) } + + @Test("Double deletion of the same row is idempotent") + func doubleDeletionIsIdempotent() { + var pending = PendingChanges() + pending.recordRowDeletion(rowIndex: 5, originalRow: ["a"]) + pending.recordRowDeletion(rowIndex: 5, originalRow: ["a"]) + #expect(pending.changes.count == 1) + #expect(pending.isRowDeleted(5)) + } + + @Test("Double insertion of the same row updates stored values without duplicating") + func doubleInsertionIsIdempotent() { + var pending = PendingChanges() + pending.recordRowInsertion(rowIndex: 3, values: ["x"]) + pending.recordRowInsertion(rowIndex: 3, values: ["y"]) + #expect(pending.changes.count == 1) + #expect(pending.isRowInserted(3)) + #expect(pending.savedInsertedValues(forRow: 3) == ["y"]) + } } @Suite("PendingChanges - undo") From 9eca620c1026388633017a7e954596041b62bd80 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ng=C3=B4=20Qu=E1=BB=91c=20=C4=90=E1=BA=A1t?= Date: Tue, 28 Apr 2026 16:57:42 +0700 Subject: [PATCH 3/5] fix(datagrid): undo replay paths now mark affected rows as changed Phase B regression: undoRowDeletion / undoRowInsertion / undoBatchRowInsertion and the replay helpers (reapplyRowDeletion, reapplyCellChange, revertUpdateCell, updateInsertedCellDirectly, reinsertRow, reinsertBatch) lost the changedRowIndices.insert that the original DataChangeManager called at the end of every undo path. Without it, consumeChangedRowIndices returned empty after undo. The data grid's reloadAndSyncSelection then fell through to the !hasChanges fallback, doing a full reloadData over all rows. With 1000 rows this was 33ms per undo. With larger datasets it gets worse. Trace before this fix (cellEdit undo to clean state): applyDataUndo END mutate=0.08ms callback=0.06ms total=0.19ms reloadAndSync VERSION_CHANGED no changes -> full reload reloadAndSync total=33.2ms updateNSView reloadVersion=8 elapsed=33.5ms After: changedRows is non-empty, partial reload of just the affected row fires. Six new regression tests cover each replay method. --- .../Core/ChangeTracking/PendingChanges.swift | 10 +++ .../ChangeTracking/PendingChangesTests.swift | 65 +++++++++++++++++++ 2 files changed, 75 insertions(+) diff --git a/TablePro/Core/ChangeTracking/PendingChanges.swift b/TablePro/Core/ChangeTracking/PendingChanges.swift index 95970f725..97415e681 100644 --- a/TablePro/Core/ChangeTracking/PendingChanges.swift +++ b/TablePro/Core/ChangeTracking/PendingChanges.swift @@ -123,6 +123,7 @@ struct PendingChanges: Equatable { guard deletedRowIndices.contains(rowIndex) else { return false } removeChange(rowIndex: rowIndex, type: .delete) deletedRowIndices.remove(rowIndex) + changedRowIndices.insert(rowIndex) return true } @@ -134,6 +135,7 @@ struct PendingChanges: Equatable { insertedRowData.removeValue(forKey: rowIndex) shiftRowIndicesDown(at: rowIndex) + changedRowIndices.insert(rowIndex) return true } @@ -157,6 +159,7 @@ struct PendingChanges: Equatable { removeChange(rowIndex: rowIndex, type: .insert) insertedRowIndices.remove(rowIndex) insertedRowData.removeValue(forKey: rowIndex) + changedRowIndices.insert(rowIndex) } let sortedRemoved = validRows.sorted() @@ -185,6 +188,7 @@ struct PendingChanges: Equatable { modifiedCells.removeValue(forKey: rowIndex) appendChange(RowChange(rowIndex: rowIndex, type: .delete, originalRow: originalRow)) deletedRowIndices.insert(rowIndex) + changedRowIndices.insert(rowIndex) } /// Re-apply a cell edit during undo replay (skips undo registration). @@ -206,6 +210,7 @@ struct PendingChanges: Equatable { if let insertIdx = changeIndex[RowChangeKey(rowIndex: rowIndex, type: .insert)] { updateInsertedCell(at: insertIdx, columnIndex: columnIndex, columnName: columnName, newValue: newValue) + changedRowIndices.insert(rowIndex) return } @@ -221,6 +226,7 @@ struct PendingChanges: Equatable { changeIndex[updateKey] = changes.count - 1 modifiedCells[rowIndex, default: []].insert(columnIndex) } + changedRowIndices.insert(rowIndex) } /// Replace an inserted row's cell value during undo replay (no shift, no undo). @@ -232,6 +238,7 @@ struct PendingChanges: Equatable { ) { guard let insertIdx = changeIndex[RowChangeKey(rowIndex: rowIndex, type: .insert)] else { return } updateInsertedCell(at: insertIdx, columnIndex: columnIndex, columnName: columnName, newValue: newValue) + changedRowIndices.insert(rowIndex) } /// Restore a cell's value during undo replay when an existing change matches. @@ -264,6 +271,7 @@ struct PendingChanges: Equatable { newValue: previousValue ) } + changedRowIndices.insert(rowIndex) } /// Insert a synthetic .insert RowChange for undo replay (e.g., after redoing a deletion's undo). @@ -280,6 +288,7 @@ struct PendingChanges: Equatable { if let savedValues { insertedRowData[rowIndex] = savedValues } + changedRowIndices.insert(rowIndex) } /// Insert a batch of rows (for undo replay of a batch deletion's undo). @@ -302,6 +311,7 @@ struct PendingChanges: Equatable { changes.append(RowChange(rowIndex: rowIndex, type: .insert, cellChanges: cellChanges)) insertedRowIndices.insert(rowIndex) insertedRowData[rowIndex] = values + changedRowIndices.insert(rowIndex) } rebuildChangeIndex() } diff --git a/TableProTests/Core/ChangeTracking/PendingChangesTests.swift b/TableProTests/Core/ChangeTracking/PendingChangesTests.swift index 0a6b8c0ee..253905ad9 100644 --- a/TableProTests/Core/ChangeTracking/PendingChangesTests.swift +++ b/TableProTests/Core/ChangeTracking/PendingChangesTests.swift @@ -207,6 +207,71 @@ struct PendingChangesSnapshotTests { } } +@Suite("PendingChanges - changedRowIndices tracking") +struct PendingChangesChangedRowIndicesTests { + @Test("revertUpdateCell records the row as changed") + func revertUpdateCellMarksChanged() { + var pending = PendingChanges() + pending.recordCellChange( + rowIndex: 4, columnIndex: 1, columnName: "name", + oldValue: "A", newValue: "B" + ) + _ = pending.consumeChangedRowIndices() + + pending.revertUpdateCell( + rowIndex: 4, columnIndex: 1, columnName: "name", previousValue: "A" + ) + #expect(pending.consumeChangedRowIndices().contains(4)) + } + + @Test("undoRowDeletion records the row as changed") + func undoRowDeletionMarksChanged() { + var pending = PendingChanges() + pending.recordRowDeletion(rowIndex: 7, originalRow: ["a"]) + _ = pending.consumeChangedRowIndices() + + _ = pending.undoRowDeletion(rowIndex: 7) + #expect(pending.consumeChangedRowIndices().contains(7)) + } + + @Test("undoRowInsertion records the row as changed") + func undoRowInsertionMarksChanged() { + var pending = PendingChanges() + pending.recordRowInsertion(rowIndex: 2, values: ["x"]) + _ = pending.consumeChangedRowIndices() + + _ = pending.undoRowInsertion(rowIndex: 2) + #expect(pending.consumeChangedRowIndices().contains(2)) + } + + @Test("reapplyRowDeletion records the row as changed") + func reapplyRowDeletionMarksChanged() { + var pending = PendingChanges() + _ = pending.consumeChangedRowIndices() + pending.reapplyRowDeletion(rowIndex: 3, originalRow: ["a"]) + #expect(pending.consumeChangedRowIndices().contains(3)) + } + + @Test("reapplyCellChange records the row as changed") + func reapplyCellChangeMarksChanged() { + var pending = PendingChanges() + _ = pending.consumeChangedRowIndices() + pending.reapplyCellChange( + rowIndex: 5, columnIndex: 1, columnName: "name", + newValue: "X", originalRow: nil + ) + #expect(pending.consumeChangedRowIndices().contains(5)) + } + + @Test("reinsertRow records the row as changed") + func reinsertRowMarksChanged() { + var pending = PendingChanges() + _ = pending.consumeChangedRowIndices() + pending.reinsertRow(rowIndex: 1, columns: ["a"], savedValues: ["v"]) + #expect(pending.consumeChangedRowIndices().contains(1)) + } +} + @Suite("PendingChanges - clear and consume") struct PendingChangesLifecycleTests { @Test("Clear empties all internal state") From d7de4e91ac8ecbb49f4ed7dd0ea3907dc0796c5e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ng=C3=B4=20Qu=E1=BB=91c=20=C4=90=E1=BA=A1t?= Date: Tue, 28 Apr 2026 17:06:03 +0700 Subject: [PATCH 4/5] fix(datagrid): preserve original DB value through redo so next undo collapses User-found bug: edit cell -> undo -> redo -> undo. Value reverts visually but yellow modified-bg persists. Root cause: reapplyCellChange (called when redo applies an edit and no pending change exists for that row) was creating a CellChange with oldValue=nil. When the next undo went through revertUpdateCell, it compared previousValue (the original DB value) to the cellChange's oldValue (nil) and they didn't match, so it took the "update inline" branch instead of the "collapse" branch. modifiedCells stayed populated. Fix: reapplyCellChange now takes an `originalDBValue` parameter and stores it as the cellChange's oldValue. The DataChangeManager call site passes the action's `newValue` (which is the original DB value in the redo direction). This matches the OLD recordCellChangeForRedo semantics. Trace through the failing scenario after this fix: edit A->B : cellChange(old=A, new=B), modified[0]={1} undo : remove cellChange, modified[0]=removed redo : reapplyCellChange creates cellChange(old=A, new=B), modified[0]={1} (was nil before) undo (second time) : revertUpdateCell sees old=A, prevValue=A -> match -> COLLAPSE modified[0]=removed (was stuck before) Three new tests: - reapplyCellPreservesOriginalDBValue - editUndoRedoUndoCollapses (the exact user-reported scenario) - updated reapplyCellChange signature in existing tests --- .../Core/ChangeTracking/DataChangeManager.swift | 5 ++++- .../Core/ChangeTracking/PendingChanges.swift | 5 ++++- .../DataChangeManagerExtendedTests.swift | 17 +++++++++++++++++ .../ChangeTracking/PendingChangesTests.swift | 17 +++++++++++++++-- 4 files changed, 40 insertions(+), 4 deletions(-) diff --git a/TablePro/Core/ChangeTracking/DataChangeManager.swift b/TablePro/Core/ChangeTracking/DataChangeManager.swift index f9a93f12b..cded88198 100644 --- a/TablePro/Core/ChangeTracking/DataChangeManager.swift +++ b/TablePro/Core/ChangeTracking/DataChangeManager.swift @@ -255,9 +255,12 @@ final class DataChangeManager: ChangeManaging { columnName: columnName, newValue: previousValue ) } else { + // Redo creating a fresh update: the action's `newValue` is the unmodified + // DB value (because `previousValue` is what we're setting back to). Pass it + // as the cellChange's oldValue so a future undo can collapse correctly. pending.reapplyCellChange( rowIndex: rowIndex, columnIndex: columnIndex, columnName: columnName, - newValue: previousValue, originalRow: originalRow + originalDBValue: newValue, newValue: previousValue, originalRow: originalRow ) } lastUndoResult = UndoResult(action: action, needsRowRemoval: false, needsRowRestore: false, restoreRow: nil) diff --git a/TablePro/Core/ChangeTracking/PendingChanges.swift b/TablePro/Core/ChangeTracking/PendingChanges.swift index 97415e681..286c25b3c 100644 --- a/TablePro/Core/ChangeTracking/PendingChanges.swift +++ b/TablePro/Core/ChangeTracking/PendingChanges.swift @@ -192,10 +192,13 @@ struct PendingChanges: Equatable { } /// Re-apply a cell edit during undo replay (skips undo registration). + /// `originalDBValue` is the cell's value in the unmodified database row. + /// It must be preserved so that a later collapse compares correctly. mutating func reapplyCellChange( rowIndex: Int, columnIndex: Int, columnName: String, + originalDBValue: String?, newValue: String?, originalRow: [String?]? ) { @@ -203,7 +206,7 @@ struct PendingChanges: Equatable { rowIndex: rowIndex, columnIndex: columnIndex, columnName: columnName, - oldValue: nil, + oldValue: originalDBValue, newValue: newValue ) diff --git a/TableProTests/Core/ChangeTracking/DataChangeManagerExtendedTests.swift b/TableProTests/Core/ChangeTracking/DataChangeManagerExtendedTests.swift index b0c103665..78fec8658 100644 --- a/TableProTests/Core/ChangeTracking/DataChangeManagerExtendedTests.swift +++ b/TableProTests/Core/ChangeTracking/DataChangeManagerExtendedTests.swift @@ -788,6 +788,23 @@ struct DataChangeManagerExtendedTests { #expect(!manager.changes.isEmpty) } + @Test("Edit -> undo -> redo -> undo collapses cleanly (no orphan modifiedCells)") + func editUndoRedoUndoCollapses() { + let manager = makeManager() + manager.recordCellChange( + rowIndex: 0, columnIndex: 1, columnName: "name", + oldValue: "Alice", newValue: "Bob" + ) + _ = manager.undoLastChange() + _ = manager.redoLastChange() + #expect(manager.isCellModified(rowIndex: 0, columnIndex: 1)) + + _ = manager.undoLastChange() + #expect(!manager.isCellModified(rowIndex: 0, columnIndex: 1)) + #expect(manager.changes.isEmpty) + #expect(!manager.hasChanges) + } + @Test("Inserted row edit consistency between changes and insertedRowData") func invariantInsertedRowEditConsistency() { let manager = makeManager() diff --git a/TableProTests/Core/ChangeTracking/PendingChangesTests.swift b/TableProTests/Core/ChangeTracking/PendingChangesTests.swift index 253905ad9..b1f2cc2a8 100644 --- a/TableProTests/Core/ChangeTracking/PendingChangesTests.swift +++ b/TableProTests/Core/ChangeTracking/PendingChangesTests.swift @@ -162,9 +162,22 @@ struct PendingChangesReplayTests { var pending = PendingChanges() pending.reapplyCellChange( rowIndex: 0, columnIndex: 1, columnName: "name", - newValue: "x", originalRow: nil + originalDBValue: "orig", newValue: "x", originalRow: nil ) #expect(pending.isCellModified(rowIndex: 0, columnIndex: 1)) + #expect(pending.changes[0].cellChanges[0].oldValue == "orig") + } + + @Test("Reapply cell change preserves the original DB value as oldValue") + func reapplyCellPreservesOriginalDBValue() { + var pending = PendingChanges() + pending.reapplyCellChange( + rowIndex: 0, columnIndex: 1, columnName: "name", + originalDBValue: "Alice", newValue: "Bob", originalRow: nil + ) + let cellChange = pending.changes[0].cellChanges[0] + #expect(cellChange.oldValue == "Alice") + #expect(cellChange.newValue == "Bob") } @Test("Reinsert row creates insert change with saved values") @@ -258,7 +271,7 @@ struct PendingChangesChangedRowIndicesTests { _ = pending.consumeChangedRowIndices() pending.reapplyCellChange( rowIndex: 5, columnIndex: 1, columnName: "name", - newValue: "X", originalRow: nil + originalDBValue: nil, newValue: "X", originalRow: nil ) #expect(pending.consumeChangedRowIndices().contains(5)) } From e9704c75583695e7035f9c77acef765988a4b262 Mon Sep 17 00:00:00 2001 From: Ngo Quoc Dat Date: Tue, 28 Apr 2026 19:31:14 +0700 Subject: [PATCH 5/5] fix(datagrid): apply PendingChanges review fixes - undoRowInsertion and undoBatchRowInsertion now bump reloadVersion so direct callers refresh the grid (matched the pattern in undoRowDeletion / recordRowInsertion). - Correct DataChangeManagerExtendedTests.recordDeletionForAlreadyDeletedRow assertion: PendingChanges guards duplicate deletions, so changes.count stays at 1 (matches PendingChangesTests.doubleDeletionIsIdempotent). - Drop two explanatory comments from applyCellEditUndo per the no-comments rule. - CHANGELOG entry for the Phase B PendingChanges extraction. Reviewer also flagged shiftRowIndicesDown / undoBatchRowInsertion as missing deletedRowIndices and modifiedCells shifts. Investigated: RowOperationsManager always inserts at the tail (addNewRow, duplicate, paste all use resultRows.count after append), and recordRowInsertion does not call shiftRowIndicesUp. The down-shift is symmetric with the record path, so adding the deletedRowIndices shift would diverge from recordRowInsertion. Skipping pending verification of the in-the-middle insertion scenario reviewer assumed. --- CHANGELOG.md | 1 + TablePro/Core/ChangeTracking/DataChangeManager.swift | 8 ++------ .../ChangeTracking/DataChangeManagerExtendedTests.swift | 4 ++-- 3 files changed, 5 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 16f51fbae..4782ff84e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- DataChangeManager extracted a PendingChanges value type that owns cross-collection invariants for cell edits, row insertions, and deletions. DataChangeManager kept undo/redo registration, plugin SQL generation, and the `@Observable` boundary, dropping from ~960 to ~190 lines. The serialization DTO `TabPendingChanges` is renamed to `TabChangeSnapshot` to distinguish it from the live tracker. - AnyChangeManager uses ChangeManaging protocol instead of closure-based type erasure, removing all runtime `[Any]` downcasts - Row selection state moved from MainContentView @State to GridSelectionState @Observable class, preventing full view tree invalidation on every row click - QueryTab decomposed into focused sub-types: TabExecutionState, TabTableContext, TabQueryContent, TabDisplayState diff --git a/TablePro/Core/ChangeTracking/DataChangeManager.swift b/TablePro/Core/ChangeTracking/DataChangeManager.swift index cded88198..1ade78419 100644 --- a/TablePro/Core/ChangeTracking/DataChangeManager.swift +++ b/TablePro/Core/ChangeTracking/DataChangeManager.swift @@ -183,6 +183,7 @@ final class DataChangeManager: ChangeManaging { func undoRowInsertion(rowIndex: Int) { guard pending.undoRowInsertion(rowIndex: rowIndex) else { return } hasChanges = !pending.isEmpty + reloadVersion += 1 } func undoBatchRowInsertion(rowIndices: [Int]) { @@ -193,6 +194,7 @@ final class DataChangeManager: ChangeManaging { target.applyDataUndo(.batchRowInsertion(rowIndices: validRows, rowValues: rowValues)) } hasChanges = !pending.isEmpty + reloadVersion += 1 } // MARK: - Core Undo Application @@ -246,18 +248,12 @@ final class DataChangeManager: ChangeManaging { columnName: columnName, previousValue: previousValue ) } - // No matching cellChange: leave the update unchanged. Reaching here means - // the row's update lost track of this column (e.g. earlier collapse), so - // creating a new entry would resurrect a stale edit. } else if pending.change(forRow: rowIndex, type: .insert) != nil { pending.updateInsertedCellDirectly( rowIndex: rowIndex, columnIndex: columnIndex, columnName: columnName, newValue: previousValue ) } else { - // Redo creating a fresh update: the action's `newValue` is the unmodified - // DB value (because `previousValue` is what we're setting back to). Pass it - // as the cellChange's oldValue so a future undo can collapse correctly. pending.reapplyCellChange( rowIndex: rowIndex, columnIndex: columnIndex, columnName: columnName, originalDBValue: newValue, newValue: previousValue, originalRow: originalRow diff --git a/TableProTests/Core/ChangeTracking/DataChangeManagerExtendedTests.swift b/TableProTests/Core/ChangeTracking/DataChangeManagerExtendedTests.swift index 78fec8658..585d1b3a9 100644 --- a/TableProTests/Core/ChangeTracking/DataChangeManagerExtendedTests.swift +++ b/TableProTests/Core/ChangeTracking/DataChangeManagerExtendedTests.swift @@ -663,12 +663,12 @@ struct DataChangeManagerExtendedTests { // MARK: - Edge Cases - @Test("Recording deletion for already-deleted row adds duplicate entry") + @Test("Recording deletion for already-deleted row is idempotent") func recordDeletionForAlreadyDeletedRow() { let manager = makeManager() manager.recordRowDeletion(rowIndex: 0, originalRow: ["1", "Alice", "a@test.com"]) manager.recordRowDeletion(rowIndex: 0, originalRow: ["1", "Alice", "a@test.com"]) - #expect(manager.changes.count == 2) + #expect(manager.changes.count == 1) } @Test("changedRowIndices includes all operation types")