Skip to content

Commit cf97b96

Browse files
datlechindatlechin
authored
refactor(coordinator): RowOperations migrates to selectedTabAndIndex + fix delete-row visual regression (#940)
* 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. * test+refactor: lock invalidateCachesForUndoRedo dispatch, tighten copy/undo paths 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.
1 parent ce570a9 commit cf97b96

3 files changed

Lines changed: 142 additions & 41 deletions

File tree

‎TablePro/Views/Main/Extensions/MainContentCoordinator+RowOperations.swift‎

Lines changed: 22 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -3,11 +3,9 @@ import Foundation
33
extension MainContentCoordinator {
44
func addNewRow() {
55
guard !safeModeLevel.blocksAllWrites,
6-
let tabIndex = tabManager.selectedTabIndex,
7-
tabIndex < tabManager.tabs.count else { return }
8-
9-
let tab = tabManager.tabs[tabIndex]
10-
guard tab.tableContext.isEditable, tab.tableContext.tableName != nil else { return }
6+
let (tab, tabIndex) = tabManager.selectedTabAndIndex,
7+
tab.tableContext.isEditable,
8+
tab.tableContext.tableName != nil else { return }
119

1210
let tabId = tab.id
1311
let columnDefaults = tableRowsStore.tableRows(for: tabId).columnDefaults
@@ -37,12 +35,11 @@ extension MainContentCoordinator {
3735

3836
func deleteSelectedRows(indices: Set<Int>) {
3937
guard !safeModeLevel.blocksAllWrites,
40-
let tabIndex = tabManager.selectedTabIndex,
41-
tabIndex < tabManager.tabs.count,
42-
tabManager.tabs[tabIndex].tableContext.isEditable,
38+
let (tab, tabIndex) = tabManager.selectedTabAndIndex,
39+
tab.tableContext.isEditable,
4340
!indices.isEmpty else { return }
4441

45-
let tabId = tabManager.tabs[tabIndex].id
42+
let tabId = tab.id
4643

4744
var deleteResult = RowOperationsManager.DeleteRowsResult(
4845
nextRowToSelect: -1,
@@ -77,11 +74,9 @@ extension MainContentCoordinator {
7774

7875
func duplicateSelectedRow(index: Int) {
7976
guard !safeModeLevel.blocksAllWrites,
80-
let tabIndex = tabManager.selectedTabIndex,
81-
tabIndex < tabManager.tabs.count else { return }
82-
83-
let tab = tabManager.tabs[tabIndex]
84-
guard tab.tableContext.isEditable, tab.tableContext.tableName != nil else { return }
77+
let (tab, tabIndex) = tabManager.selectedTabAndIndex,
78+
tab.tableContext.isEditable,
79+
tab.tableContext.tableName != nil else { return }
8580

8681
let tabId = tab.id
8782
let columns = tableRowsStore.tableRows(for: tabId).columns
@@ -110,10 +105,8 @@ extension MainContentCoordinator {
110105
}
111106

112107
func undoInsertRow(at rowIndex: Int) {
113-
guard let tabIndex = tabManager.selectedTabIndex,
114-
tabIndex < tabManager.tabs.count else { return }
115-
116-
let tabId = tabManager.tabs[tabIndex].id
108+
guard let (tab, _) = tabManager.selectedTabAndIndex else { return }
109+
let tabId = tab.id
117110

118111
var undoResult = RowOperationsManager.UndoInsertRowResult(
119112
adjustedSelection: selectionState.indices,
@@ -135,10 +128,8 @@ extension MainContentCoordinator {
135128
}
136129

137130
func handleUndoResult(_ result: UndoResult) {
138-
guard let tabIndex = tabManager.selectedTabIndex,
139-
tabIndex < tabManager.tabs.count else { return }
131+
guard let (tab, tabIndex) = tabManager.selectedTabAndIndex else { return }
140132

141-
let tab = tabManager.tabs[tabIndex]
142133
let tabId = tab.id
143134

144135
var application = RowOperationsManager.UndoApplicationResult(adjustedSelection: nil, delta: .none)
@@ -159,10 +150,7 @@ extension MainContentCoordinator {
159150
}
160151

161152
func copySelectedRowsToClipboard(indices: Set<Int>) {
162-
guard let index = tabManager.selectedTabIndex,
163-
!indices.isEmpty else { return }
164-
165-
let tab = tabManager.tabs[index]
153+
guard let (tab, _) = tabManager.selectedTabAndIndex, !indices.isEmpty else { return }
166154
let tableRows = tableRowsStore.tableRows(for: tab.id)
167155
rowOperationsManager.copySelectedRowsToClipboard(
168156
selectedIndices: indices,
@@ -171,10 +159,7 @@ extension MainContentCoordinator {
171159
}
172160

173161
func copySelectedRowsWithHeaders(indices: Set<Int>) {
174-
guard let index = tabManager.selectedTabIndex,
175-
!indices.isEmpty else { return }
176-
177-
let tab = tabManager.tabs[index]
162+
guard let (tab, _) = tabManager.selectedTabAndIndex, !indices.isEmpty else { return }
178163
let tableRows = tableRowsStore.tableRows(for: tab.id)
179164
rowOperationsManager.copySelectedRowsToClipboard(
180165
selectedIndices: indices,
@@ -184,9 +169,7 @@ extension MainContentCoordinator {
184169
}
185170

186171
func copySelectedRowsAsJson(indices: Set<Int>) {
187-
guard let index = tabManager.selectedTabIndex,
188-
!indices.isEmpty else { return }
189-
let tab = tabManager.tabs[index]
172+
guard let (tab, _) = tabManager.selectedTabAndIndex, !indices.isEmpty else { return }
190173
let tableRows = tableRowsStore.tableRows(for: tab.id)
191174
let rows = indices.sorted().compactMap { idx -> [String?]? in
192175
guard idx >= 0, idx < tableRows.count else { return nil }
@@ -202,10 +185,8 @@ extension MainContentCoordinator {
202185

203186
func pasteRows() {
204187
guard !safeModeLevel.blocksAllWrites,
205-
let index = tabManager.selectedTabIndex else { return }
206-
207-
let tab = tabManager.tabs[index]
208-
guard tab.tabType == .table else { return }
188+
let (tab, tabIndex) = tabManager.selectedTabAndIndex,
189+
tab.tabType == .table else { return }
209190

210191
let tabId = tab.id
211192
let columns = tableRowsStore.tableRows(for: tabId).columns
@@ -226,19 +207,19 @@ extension MainContentCoordinator {
226207
let newIndices = Set(pasteResult.pastedRows.map { $0.rowIndex })
227208
selectionState.indices = newIndices
228209

229-
tabManager.tabs[index].selectedRowIndices = newIndices
230-
tabManager.tabs[index].hasUserInteraction = true
210+
tabManager.tabs[tabIndex].selectedRowIndices = newIndices
211+
tabManager.tabs[tabIndex].hasUserInteraction = true
231212
querySortCache.removeValue(forKey: tabId)
232213
dataTabDelegate?.tableViewCoordinator?.applyDelta(pasteResult.delta)
233214
}
234215

235216
func updateCellInTab(rowIndex: Int, columnIndex: Int, value: String?) {
236-
guard let index = tabManager.selectedTabIndex else { return }
237-
let tabId = tabManager.tabs[index].id
217+
guard let (tab, tabIndex) = tabManager.selectedTabAndIndex else { return }
218+
let tabId = tab.id
238219
let delta = mutateActiveTableRows(for: tabId) { rows in
239220
rows.edit(row: rowIndex, column: columnIndex, value: value)
240221
}
241-
tabManager.tabs[index].hasUserInteraction = true
222+
tabManager.tabs[tabIndex].hasUserInteraction = true
242223
dataTabDelegate?.tableViewCoordinator?.applyDelta(delta)
243224
}
244225
}

‎TablePro/Views/Results/DataGridCoordinator.swift‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -394,6 +394,13 @@ final class TableViewCoordinator: NSObject, NSTableViewDelegate, NSTableViewData
394394
displayCache.removeAll()
395395
rebuildVisualStateCache()
396396
updateCache()
397+
guard let tableView else { return }
398+
let visibleRange = tableView.rows(in: tableView.visibleRect)
399+
guard visibleRange.length > 0 else { return }
400+
tableView.reloadData(
401+
forRowIndexes: IndexSet(integersIn: visibleRange.location..<(visibleRange.location + visibleRange.length)),
402+
columnIndexes: IndexSet(integersIn: 0..<tableView.numberOfColumns)
403+
)
397404
}
398405

399406
func commitActiveCellEdit() {

‎TableProTests/Views/Main/RowOperationsDispatchTests.swift‎

Lines changed: 113 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,113 @@
1+
//
2+
// RowOperationsDispatchTests.swift
3+
// TableProTests
4+
//
5+
// Locks the dispatch wiring from RowOperations into TableViewCoordinating.
6+
// These tests guard the path that PR #938 (Phase D-b) accidentally severed:
7+
// invalidateCachesForUndoRedo must fire on soft-delete (existing rows) so the
8+
// red row background and yellow modified marker propagate to NSTableView's
9+
// visible cell views without requiring a tab switch or scroll-recycle.
10+
//
11+
12+
import Foundation
13+
import Testing
14+
@testable import TablePro
15+
16+
@MainActor
17+
private final class FakeTableViewCoordinator: TableViewCoordinating {
18+
var fullReplaceCount = 0
19+
var insertedCount = 0
20+
var removedCount = 0
21+
var deltaCount = 0
22+
var invalidateCount = 0
23+
var commitEditCount = 0
24+
var refreshFKCount = 0
25+
var scrollToTopCount = 0
26+
var beginEditingCalls: [(row: Int, column: Int)] = []
27+
28+
func applyInsertedRows(_ indices: IndexSet) { insertedCount += 1 }
29+
func applyRemovedRows(_ indices: IndexSet) { removedCount += 1 }
30+
func applyFullReplace() { fullReplaceCount += 1 }
31+
func applyDelta(_ delta: Delta) { deltaCount += 1 }
32+
func invalidateCachesForUndoRedo() { invalidateCount += 1 }
33+
func commitActiveCellEdit() { commitEditCount += 1 }
34+
func beginEditing(displayRow: Int, column: Int) {
35+
beginEditingCalls.append((row: displayRow, column: column))
36+
}
37+
func refreshForeignKeyColumns() { refreshFKCount += 1 }
38+
func scrollToTop() { scrollToTopCount += 1 }
39+
}
40+
41+
@Suite("RowOperations dispatch")
42+
@MainActor
43+
struct RowOperationsDispatchTests {
44+
private struct Fixture {
45+
let coordinator: MainContentCoordinator
46+
let tabManager: QueryTabManager
47+
let delegate: DataTabGridDelegate
48+
let fake: FakeTableViewCoordinator
49+
let tabId: UUID
50+
}
51+
52+
private func makeFixture(rowCount: Int = 5) -> Fixture {
53+
let tabManager = QueryTabManager()
54+
let coordinator = MainContentCoordinator(
55+
connection: TestFixtures.makeConnection(),
56+
tabManager: tabManager,
57+
changeManager: DataChangeManager(),
58+
filterStateManager: FilterStateManager(),
59+
columnVisibilityManager: ColumnVisibilityManager(),
60+
toolbarState: ConnectionToolbarState()
61+
)
62+
let delegate = DataTabGridDelegate()
63+
let fake = FakeTableViewCoordinator()
64+
delegate.tableViewCoordinator = fake
65+
coordinator.dataTabDelegate = delegate
66+
67+
tabManager.addTableTab(tableName: "users")
68+
let tabIndex = tabManager.selectedTabIndex ?? 0
69+
tabManager.tabs[tabIndex].tableContext.isEditable = true
70+
let tabId = tabManager.tabs[tabIndex].id
71+
72+
let columns = ["id", "name"]
73+
let rows = (0..<rowCount).map { i in ["\(i)", "name\(i)"] }
74+
let columnTypes: [ColumnType] = Array(repeating: .text(rawType: nil), count: columns.count)
75+
coordinator.setActiveTableRows(
76+
TableRows.from(queryRows: rows, columns: columns, columnTypes: columnTypes),
77+
for: tabId
78+
)
79+
80+
return Fixture(
81+
coordinator: coordinator,
82+
tabManager: tabManager,
83+
delegate: delegate,
84+
fake: fake,
85+
tabId: tabId
86+
)
87+
}
88+
89+
@Test("Soft-delete of existing rows dispatches invalidateCachesForUndoRedo")
90+
func softDeleteDispatchesInvalidate() {
91+
let f = makeFixture(rowCount: 5)
92+
let beforeInvalidate = f.fake.invalidateCount
93+
94+
f.coordinator.deleteSelectedRows(indices: [0, 1])
95+
96+
#expect(f.fake.invalidateCount == beforeInvalidate + 1)
97+
#expect(f.fake.deltaCount == 0)
98+
}
99+
100+
@Test("Physical delete of inserted rows dispatches applyDelta, not invalidate")
101+
func physicalDeleteDispatchesDelta() {
102+
let f = makeFixture(rowCount: 3)
103+
f.coordinator.addNewRow()
104+
let insertedIndex = f.coordinator.tableRowsStore.tableRows(for: f.tabId).count - 1
105+
let beforeInvalidate = f.fake.invalidateCount
106+
let beforeDelta = f.fake.deltaCount
107+
108+
f.coordinator.deleteSelectedRows(indices: [insertedIndex])
109+
110+
#expect(f.fake.invalidateCount == beforeInvalidate)
111+
#expect(f.fake.deltaCount == beforeDelta + 1)
112+
}
113+
}

0 commit comments

Comments
 (0)