Skip to content

refactor(coordinator): sweep selectedTabIndex + bounds-check pattern across all extensions#941

Merged
datlechin merged 2 commits intomainfrom
refactor/coordinator-selectedtabandindex-sweep
Apr 29, 2026
Merged

refactor(coordinator): sweep selectedTabIndex + bounds-check pattern across all extensions#941
datlechin merged 2 commits intomainfrom
refactor/coordinator-selectedtabandindex-sweep

Conversation

@datlechin
Copy link
Copy Markdown
Member

Summary

Completes the selectedTabAndIndex migration across 13 files / 30+ call sites. Closes the parking lot for this pattern. −104 / +81 LOC (net −23, but the more meaningful win is consistency: every active-tab lookup now uses one of two named helpers instead of the inline three-line pattern).

What was migrated

Every site that used guard let tabIndex = tabManager.selectedTabIndex, tabIndex < tabManager.tabs.count else { return } followed by let tab = tabManager.tabs[tabIndex]:

File Methods
MainContentCoordinator.swift runQuery, executeTableTabQueryDirectly, loadQueryIntoEditor, insertQueryFromAI, runExplainQuery, executeQueryInternal, handleSort, EXPLAIN error fallback
+ClickHouse.swift runVariantExplain
+ColumnVisibility.swift saveColumnVisibilityToTab
+Discard.swift handleDiscard (two sites)
+ExecuteAll.swift runAllStatements
+Favorites.swift insertFavorite, runFavoriteInNewTab
+Filtering.swift applyFilters, clearFiltersAndReload, restoreFiltersForTable
+FKNavigation.swift navigateToFKReference (four sites)
+LoadMore.swift loadMoreRows, fetchAllRows
+Navigation.swift openTableTab (four sites), promotePreviewTab
+QueryParameters.swift executeQueryWithParameters, executeQueryInternalParameterized, executeMultipleStatementsWithParameters
+Refresh.swift handleRefresh (three sites)
MainContentCommandActions.swift formatQuery, toggleResults, previousResultTab, nextResultTab

Pattern

  • Write-back paths (need the index to mutate tabManager.tabs[i]): selectedTabAndIndex returning (tab, index).
  • Read-only paths (only need the tab): selectedTab, plus an explicit (tab, _) from selectedTabAndIndex where strict bounds-check equivalence is preferred.
  • Mixed paths (read tab properties, write back via index): (tab, tabIndex) destructured.

Behavior

Unchanged. Each migration preserves the original short-circuits (no tab selected, out-of-bounds, etc.) via selectedTabAndIndex's atomic bounds check. The minor exception was that read-only methods previously did tabs[index] after just an optional check — those now strictly bounds-check via selectedTabAndIndex, a tiny safety improvement.

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.

Test plan

  • Build clean
  • Targeted tests pass (TableRowsMutationTests, SortCacheInvalidationTests, EvictionTests, QueryTabManagerSelectedTabAndIndexTests, RowOperationsDispatchTests)
  • Manual: Cmd+Enter run query, Cmd+Shift+Enter run all, EXPLAIN
  • Manual: open table from sidebar, FK navigation
  • Manual: apply / clear filters, refresh (Cmd+R), discard dialog
  • Manual: pagination Next/Prev
  • Manual: format query (right-click), toggle results panel, insert favorite

What's next (parking lot, future PRs)

The selectedTabAndIndex pattern is now the canonical lookup. With it in place, the next coordinator dedup target would be:

  • The confirmDiscardChangesIfNeeded callback pattern (used in Pagination, Filtering, Navigation — each captures its own copy of tabIndex/tabId via [weak self] then re-validates inside the callback). Could move to a per-action helper.
  • The tabManager.tabs[tabIndex].execution.errorMessage = ... pattern for safe-mode error reporting (used 5+ times). Could become a setExecutionError(_:on:) helper on the coordinator.

Both warrant separate focused PRs.

…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.
@datlechin datlechin merged commit 709c027 into main Apr 29, 2026
2 checks passed
@datlechin datlechin deleted the refactor/coordinator-selectedtabandindex-sweep branch April 29, 2026 06:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant