Conversation
… 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.
…y/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.
datlechin
added a commit
that referenced
this pull request
Apr 29, 2026
…across all extensions (#941) * refactor(coordinator): sweep selectedTabIndex + bounds-check pattern across all extensions Migrates 12 files / 30+ call sites of the duplicated 'guard let tabIndex = tabManager.selectedTabIndex, tabIndex < tabManager.tabs.count' pattern to selectedTabAndIndex (write-back paths) or selectedTab (read-only paths). Both helpers were introduced in PR #939 and #940; this PR completes the migration outside Pagination and RowOperations. Files touched: - MainContentCoordinator.swift: runQuery, executeTableTabQueryDirectly, loadQueryIntoEditor, insertQueryFromAI, runExplainQuery, executeQueryInternal, handleSort, EXPLAIN error fallback - MainContentCoordinator+ClickHouse.swift: runVariantExplain - MainContentCoordinator+ColumnVisibility.swift: saveColumnVisibilityToTab - MainContentCoordinator+Discard.swift: handleDiscard (two sites) - MainContentCoordinator+ExecuteAll.swift: runAllStatements - MainContentCoordinator+Favorites.swift: insertFavorite, runFavoriteInNewTab - MainContentCoordinator+Filtering.swift: applyFilters, clearFiltersAndReload, restoreFiltersForTable - MainContentCoordinator+FKNavigation.swift: navigateToFKReference (four sites) - MainContentCoordinator+LoadMore.swift: loadMoreRows, fetchAllRows - MainContentCoordinator+Navigation.swift: openTableTab (four sites), promotePreviewTab - MainContentCoordinator+QueryParameters.swift: executeQueryWithParameters, executeQueryInternalParameterized, executeMultipleStatementsWithParameters - MainContentCoordinator+Refresh.swift: handleRefresh (three sites) - MainContentCommandActions.swift: formatQuery, toggleResults, previousResultTab, nextResultTab Behavior unchanged. Each migration preserves the original tab-not-selected and out-of-bounds short-circuits via selectedTabAndIndex's atomic check. Read-only paths use selectedTab (no fallback divergence — these don't use the index, so the strict-bounds requirement is moot). The dispatchParameterizedStatements / dispatchStatements helpers (called via tabIndex parameter, not selectedTabIndex) were left alone — they take the index from the caller's already-validated lookup. ExecuteAll's tabIndex parameter pattern preserved. Smoke-tested across query execution, EXPLAIN, table open from sidebar, FK navigation, filter apply/clear, refresh, discard dialog, pagination, format query, toggle results, insert favorite. Targeted tests pass. * refactor(coordinator): standardize selectedTabAndIndex destructuring as (tab, tabIndex)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two related changes in the same area:
1. RowOperations migration (continuation of PR #939)
Migrates
MainContentCoordinator+RowOperationsto useselectedTabAndIndex(added in #939) andselectedTab:tabManager.tabs[idx]):addNewRow,deleteSelectedRows,duplicateSelectedRow,handleUndoResult,pasteRows,updateCellInTabuseselectedTabAndIndex.copySelectedRowsToClipboard,copySelectedRowsWithHeaders,copySelectedRowsAsJsonuseselectedTab(drops the unused index entirely — also tightens safety since they previously did uncheckedtabs[index]after just an optional check).undoInsertRowonly needs thetabId→ usesselectedTab?.id.Behavior unchanged. The migration also adds a missing
tab.tableContext.tableName != nilguard toaddNewRowandduplicateSelectedRow(was previously a separate guard line; now folded into the main guard).2. Bug fix: delete row didn't show red background until user clicked elsewhere
Discovered while smoke-testing the migration. Pressing Delete on an existing (database-backed) row updated the change tracker correctly but the row's red background didn't appear until the user clicked a different row.
Root cause: PR #938 (Phase D-b) removed the
versionChangedbranch inreloadAndSyncSelection. That branch used to reload visible rows on anychangeManager.reloadVersionbump — that's how the visual state cache changes were propagated to NSTableView's cell views. After D-b,invalidateCachesForUndoRedo()cleared thedisplayCacheand rebuilt therowVisualStateCachecorrectly, but never told NSTableView to re-query, so visible cell views kept rendering the pre-delete state.Fix:
invalidateCachesForUndoRedonow also callstableView.reloadData(forRowIndexes: visibleRows, columnIndexes: allCols)— the call the droppedversionChangedbranch used to do implicitly. Single-place change inDataGridCoordinator. Same fix covers undo/redo of cell edits, deletes, and inserts (all go through this path).Test plan
TableRowsMutationTests,SortCacheInvalidationTests,EvictionTests,QueryTabManagerSelectedTabAndIndexTests)Stat
+28 / −41LOC across two files.