From 7fc8634b3140c9bc3c71b45a8cf38e16aaf1895b 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: Wed, 29 Apr 2026 12:51:42 +0700 Subject: [PATCH 1/2] refactor(coordinator): migrate RowOperations to selectedTabAndIndex + fix invalidateCachesForUndoRedo regression Two changes: 1. RowOperations migration. Six methods (addNewRow, deleteSelectedRows, duplicateSelectedRow, handleUndoResult, pasteRows, updateCellInTab) used the selectedTabIndex + bounds-check + tabs[index] pattern; now use selectedTabAndIndex which returns (tab, index) atomically. Three read-only methods (copySelectedRowsToClipboard, copySelectedRowsWithHeaders, copySelectedRowsAsJson) drop the index entirely and use selectedTab. undoInsertRow only needs the tabId so it uses selectedTab?.id. Behavior unchanged. 2. Bug fix discovered during RowOperations smoke test: pressing Delete on an existing row didn't show the red background until the user clicked another row. Phase D-b (PR #938) dropped the versionChanged branch in reloadAndSyncSelection, which used to reload visible rows on any changeManager.reloadVersion bump. invalidateCachesForUndoRedo cleared the displayCache and rebuilt the visualStateCache but never told NSTableView to re-query, so visible cell views kept rendering the pre-change state. Adding a reloadData(forRowIndexes: visibleRows, columnIndexes: allCols) call after the cache invalidation restores the lost behavior. Same root cause covers undo/redo of cell edits, deletes, and inserts. --- ...MainContentCoordinator+RowOperations.swift | 62 +++++++------------ .../Views/Results/DataGridCoordinator.swift | 7 +++ 2 files changed, 28 insertions(+), 41 deletions(-) diff --git a/TablePro/Views/Main/Extensions/MainContentCoordinator+RowOperations.swift b/TablePro/Views/Main/Extensions/MainContentCoordinator+RowOperations.swift index aa957e4db..a8115c4f6 100644 --- a/TablePro/Views/Main/Extensions/MainContentCoordinator+RowOperations.swift +++ b/TablePro/Views/Main/Extensions/MainContentCoordinator+RowOperations.swift @@ -3,11 +3,9 @@ import Foundation extension MainContentCoordinator { func addNewRow() { guard !safeModeLevel.blocksAllWrites, - let tabIndex = tabManager.selectedTabIndex, - tabIndex < tabManager.tabs.count else { return } - - let tab = tabManager.tabs[tabIndex] - guard tab.tableContext.isEditable, tab.tableContext.tableName != nil else { return } + let (tab, tabIndex) = tabManager.selectedTabAndIndex, + tab.tableContext.isEditable, + tab.tableContext.tableName != nil else { return } let tabId = tab.id let columnDefaults = tableRowsStore.tableRows(for: tabId).columnDefaults @@ -37,12 +35,11 @@ extension MainContentCoordinator { func deleteSelectedRows(indices: Set) { guard !safeModeLevel.blocksAllWrites, - let tabIndex = tabManager.selectedTabIndex, - tabIndex < tabManager.tabs.count, - tabManager.tabs[tabIndex].tableContext.isEditable, + let (tab, tabIndex) = tabManager.selectedTabAndIndex, + tab.tableContext.isEditable, !indices.isEmpty else { return } - let tabId = tabManager.tabs[tabIndex].id + let tabId = tab.id var deleteResult = RowOperationsManager.DeleteRowsResult( nextRowToSelect: -1, @@ -77,11 +74,9 @@ extension MainContentCoordinator { func duplicateSelectedRow(index: Int) { guard !safeModeLevel.blocksAllWrites, - let tabIndex = tabManager.selectedTabIndex, - tabIndex < tabManager.tabs.count else { return } - - let tab = tabManager.tabs[tabIndex] - guard tab.tableContext.isEditable, tab.tableContext.tableName != nil else { return } + let (tab, tabIndex) = tabManager.selectedTabAndIndex, + tab.tableContext.isEditable, + tab.tableContext.tableName != nil else { return } let tabId = tab.id let columns = tableRowsStore.tableRows(for: tabId).columns @@ -110,10 +105,7 @@ extension MainContentCoordinator { } func undoInsertRow(at rowIndex: Int) { - guard let tabIndex = tabManager.selectedTabIndex, - tabIndex < tabManager.tabs.count else { return } - - let tabId = tabManager.tabs[tabIndex].id + guard let tabId = tabManager.selectedTab?.id else { return } var undoResult = RowOperationsManager.UndoInsertRowResult( adjustedSelection: selectionState.indices, @@ -135,10 +127,8 @@ extension MainContentCoordinator { } func handleUndoResult(_ result: UndoResult) { - guard let tabIndex = tabManager.selectedTabIndex, - tabIndex < tabManager.tabs.count else { return } + guard let (tab, tabIndex) = tabManager.selectedTabAndIndex else { return } - let tab = tabManager.tabs[tabIndex] let tabId = tab.id var application = RowOperationsManager.UndoApplicationResult(adjustedSelection: nil, delta: .none) @@ -159,10 +149,7 @@ extension MainContentCoordinator { } func copySelectedRowsToClipboard(indices: Set) { - guard let index = tabManager.selectedTabIndex, - !indices.isEmpty else { return } - - let tab = tabManager.tabs[index] + guard let tab = tabManager.selectedTab, !indices.isEmpty else { return } let tableRows = tableRowsStore.tableRows(for: tab.id) rowOperationsManager.copySelectedRowsToClipboard( selectedIndices: indices, @@ -171,10 +158,7 @@ extension MainContentCoordinator { } func copySelectedRowsWithHeaders(indices: Set) { - guard let index = tabManager.selectedTabIndex, - !indices.isEmpty else { return } - - let tab = tabManager.tabs[index] + guard let tab = tabManager.selectedTab, !indices.isEmpty else { return } let tableRows = tableRowsStore.tableRows(for: tab.id) rowOperationsManager.copySelectedRowsToClipboard( selectedIndices: indices, @@ -184,9 +168,7 @@ extension MainContentCoordinator { } func copySelectedRowsAsJson(indices: Set) { - guard let index = tabManager.selectedTabIndex, - !indices.isEmpty else { return } - let tab = tabManager.tabs[index] + guard let tab = tabManager.selectedTab, !indices.isEmpty else { return } let tableRows = tableRowsStore.tableRows(for: tab.id) let rows = indices.sorted().compactMap { idx -> [String?]? in guard idx >= 0, idx < tableRows.count else { return nil } @@ -202,10 +184,8 @@ extension MainContentCoordinator { func pasteRows() { guard !safeModeLevel.blocksAllWrites, - let index = tabManager.selectedTabIndex else { return } - - let tab = tabManager.tabs[index] - guard tab.tabType == .table else { return } + let (tab, tabIndex) = tabManager.selectedTabAndIndex, + tab.tabType == .table else { return } let tabId = tab.id let columns = tableRowsStore.tableRows(for: tabId).columns @@ -226,19 +206,19 @@ extension MainContentCoordinator { let newIndices = Set(pasteResult.pastedRows.map { $0.rowIndex }) selectionState.indices = newIndices - tabManager.tabs[index].selectedRowIndices = newIndices - tabManager.tabs[index].hasUserInteraction = true + tabManager.tabs[tabIndex].selectedRowIndices = newIndices + tabManager.tabs[tabIndex].hasUserInteraction = true querySortCache.removeValue(forKey: tabId) dataTabDelegate?.tableViewCoordinator?.applyDelta(pasteResult.delta) } func updateCellInTab(rowIndex: Int, columnIndex: Int, value: String?) { - guard let index = tabManager.selectedTabIndex else { return } - let tabId = tabManager.tabs[index].id + guard let (tab, tabIndex) = tabManager.selectedTabAndIndex else { return } + let tabId = tab.id let delta = mutateActiveTableRows(for: tabId) { rows in rows.edit(row: rowIndex, column: columnIndex, value: value) } - tabManager.tabs[index].hasUserInteraction = true + tabManager.tabs[tabIndex].hasUserInteraction = true dataTabDelegate?.tableViewCoordinator?.applyDelta(delta) } } diff --git a/TablePro/Views/Results/DataGridCoordinator.swift b/TablePro/Views/Results/DataGridCoordinator.swift index 9e568927a..b2c73c212 100644 --- a/TablePro/Views/Results/DataGridCoordinator.swift +++ b/TablePro/Views/Results/DataGridCoordinator.swift @@ -394,6 +394,13 @@ final class TableViewCoordinator: NSObject, NSTableViewDelegate, NSTableViewData displayCache.removeAll() rebuildVisualStateCache() updateCache() + guard let tableView else { return } + let visibleRange = tableView.rows(in: tableView.visibleRect) + guard visibleRange.length > 0 else { return } + tableView.reloadData( + forRowIndexes: IndexSet(integersIn: visibleRange.location..<(visibleRange.location + visibleRange.length)), + columnIndexes: IndexSet(integersIn: 0.. Date: Wed, 29 Apr 2026 12:58:03 +0700 Subject: [PATCH 2/2] test+refactor: lock invalidateCachesForUndoRedo dispatch, tighten copy/undo paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two follow-ups to the review: 1. Read-only copy paths (copySelectedRowsToClipboard, copySelectedRowsWithHeaders, copySelectedRowsAsJson) and undoInsertRow now use selectedTabAndIndex instead of selectedTab. selectedTab has a fallback to tabs.first when selectedTabId is nil — the old selectedTabIndex pattern did not. Switching to selectedTabAndIndex (with _ for unused index) restores strict equivalence with the pre-refactor behavior. Closes the subtle init-transient divergence the review flagged. 2. New RowOperationsDispatchTests covers the dispatch wiring restored by this PR's bug fix: - softDeleteDispatchesInvalidate: deleteSelectedRows on existing rows fires invalidateCachesForUndoRedo (the path that triggers the visible-rows reload). Without this, a future refactor could silently re-drop the reload (as Phase D-b accidentally did). - physicalDeleteDispatchesDelta: deleting an inserted row fires applyDelta and bypasses invalidate — guards the branching in deleteSelectedRows so the two paths don't get tangled. --- ...MainContentCoordinator+RowOperations.swift | 9 +- .../Main/RowOperationsDispatchTests.swift | 113 ++++++++++++++++++ 2 files changed, 118 insertions(+), 4 deletions(-) create mode 100644 TableProTests/Views/Main/RowOperationsDispatchTests.swift diff --git a/TablePro/Views/Main/Extensions/MainContentCoordinator+RowOperations.swift b/TablePro/Views/Main/Extensions/MainContentCoordinator+RowOperations.swift index a8115c4f6..576a077da 100644 --- a/TablePro/Views/Main/Extensions/MainContentCoordinator+RowOperations.swift +++ b/TablePro/Views/Main/Extensions/MainContentCoordinator+RowOperations.swift @@ -105,7 +105,8 @@ extension MainContentCoordinator { } func undoInsertRow(at rowIndex: Int) { - guard let tabId = tabManager.selectedTab?.id else { return } + guard let (tab, _) = tabManager.selectedTabAndIndex else { return } + let tabId = tab.id var undoResult = RowOperationsManager.UndoInsertRowResult( adjustedSelection: selectionState.indices, @@ -149,7 +150,7 @@ extension MainContentCoordinator { } func copySelectedRowsToClipboard(indices: Set) { - guard let tab = tabManager.selectedTab, !indices.isEmpty else { return } + guard let (tab, _) = tabManager.selectedTabAndIndex, !indices.isEmpty else { return } let tableRows = tableRowsStore.tableRows(for: tab.id) rowOperationsManager.copySelectedRowsToClipboard( selectedIndices: indices, @@ -158,7 +159,7 @@ extension MainContentCoordinator { } func copySelectedRowsWithHeaders(indices: Set) { - guard let tab = tabManager.selectedTab, !indices.isEmpty else { return } + guard let (tab, _) = tabManager.selectedTabAndIndex, !indices.isEmpty else { return } let tableRows = tableRowsStore.tableRows(for: tab.id) rowOperationsManager.copySelectedRowsToClipboard( selectedIndices: indices, @@ -168,7 +169,7 @@ extension MainContentCoordinator { } func copySelectedRowsAsJson(indices: Set) { - guard let tab = tabManager.selectedTab, !indices.isEmpty else { return } + guard let (tab, _) = tabManager.selectedTabAndIndex, !indices.isEmpty else { return } let tableRows = tableRowsStore.tableRows(for: tab.id) let rows = indices.sorted().compactMap { idx -> [String?]? in guard idx >= 0, idx < tableRows.count else { return nil } diff --git a/TableProTests/Views/Main/RowOperationsDispatchTests.swift b/TableProTests/Views/Main/RowOperationsDispatchTests.swift new file mode 100644 index 000000000..f61fdcbf5 --- /dev/null +++ b/TableProTests/Views/Main/RowOperationsDispatchTests.swift @@ -0,0 +1,113 @@ +// +// RowOperationsDispatchTests.swift +// TableProTests +// +// Locks the dispatch wiring from RowOperations into TableViewCoordinating. +// These tests guard the path that PR #938 (Phase D-b) accidentally severed: +// invalidateCachesForUndoRedo must fire on soft-delete (existing rows) so the +// red row background and yellow modified marker propagate to NSTableView's +// visible cell views without requiring a tab switch or scroll-recycle. +// + +import Foundation +import Testing +@testable import TablePro + +@MainActor +private final class FakeTableViewCoordinator: TableViewCoordinating { + var fullReplaceCount = 0 + var insertedCount = 0 + var removedCount = 0 + var deltaCount = 0 + var invalidateCount = 0 + var commitEditCount = 0 + var refreshFKCount = 0 + var scrollToTopCount = 0 + var beginEditingCalls: [(row: Int, column: Int)] = [] + + func applyInsertedRows(_ indices: IndexSet) { insertedCount += 1 } + func applyRemovedRows(_ indices: IndexSet) { removedCount += 1 } + func applyFullReplace() { fullReplaceCount += 1 } + func applyDelta(_ delta: Delta) { deltaCount += 1 } + func invalidateCachesForUndoRedo() { invalidateCount += 1 } + func commitActiveCellEdit() { commitEditCount += 1 } + func beginEditing(displayRow: Int, column: Int) { + beginEditingCalls.append((row: displayRow, column: column)) + } + func refreshForeignKeyColumns() { refreshFKCount += 1 } + func scrollToTop() { scrollToTopCount += 1 } +} + +@Suite("RowOperations dispatch") +@MainActor +struct RowOperationsDispatchTests { + private struct Fixture { + let coordinator: MainContentCoordinator + let tabManager: QueryTabManager + let delegate: DataTabGridDelegate + let fake: FakeTableViewCoordinator + let tabId: UUID + } + + private func makeFixture(rowCount: Int = 5) -> Fixture { + let tabManager = QueryTabManager() + let coordinator = MainContentCoordinator( + connection: TestFixtures.makeConnection(), + tabManager: tabManager, + changeManager: DataChangeManager(), + filterStateManager: FilterStateManager(), + columnVisibilityManager: ColumnVisibilityManager(), + toolbarState: ConnectionToolbarState() + ) + let delegate = DataTabGridDelegate() + let fake = FakeTableViewCoordinator() + delegate.tableViewCoordinator = fake + coordinator.dataTabDelegate = delegate + + tabManager.addTableTab(tableName: "users") + let tabIndex = tabManager.selectedTabIndex ?? 0 + tabManager.tabs[tabIndex].tableContext.isEditable = true + let tabId = tabManager.tabs[tabIndex].id + + let columns = ["id", "name"] + let rows = (0..