Skip to content

refactor(coordinator): extract paginateIfPossible helper, add selectedTabAndIndex#939

Merged
datlechin merged 2 commits intomainfrom
refactor/coordinator-pagination-helpers
Apr 29, 2026
Merged

refactor(coordinator): extract paginateIfPossible helper, add selectedTabAndIndex#939
datlechin merged 2 commits intomainfrom
refactor/coordinator-pagination-helpers

Conversation

@datlechin
Copy link
Copy Markdown
Member

Summary

First focused cleanup pass on MainContentCoordinator. Pagination had seven methods, six of which followed an identical six-line shape:

guard let tabIndex = tabManager.selectedTabIndex,
      tabIndex < tabManager.tabs.count,
      tabManager.tabs[tabIndex].pagination.<condition> else { return }
paginateAfterConfirmation(tabIndex: tabIndex) { pagination in
    pagination.<action>()
}

Each becomes a one-liner via two small extractions.

What's new

  • QueryTabManager.selectedTabAndIndex — returns (tab: QueryTab, index: Int)? atomically with the bounds check. Replaces the two-step selectedTabIndex + < tabs.count + tabs[index] pattern. Five lines, single purpose.
  • paginateIfPossible(where:mutate:) (private to Pagination) — captures the "active tab + precondition + confirm-discard + mutate" chain. Each public pagination method passes a KeyPath<PaginationState, Bool> (or short closure) for the precondition and a single-statement closure for the mutation.

Result

Pagination methods that used to be 5–6 lines each:

func goToNextPage() {
    paginateIfPossible(where: \.hasNextPage) { $0.goToNextPage() }
}
func goToPreviousPage() {
    paginateIfPossible(where: \.hasPreviousPage) { $0.goToPreviousPage() }
}
func goToLastPage() {
    paginateIfPossible(where: { $0.currentPage != $0.totalPages }) { $0.goToLastPage() }
}

Net −30 LOC in Pagination, +5 LOC in QueryTabManager.

What's not in this PR

There are ~14 other selectedTabIndex + bounds-check sites across MainContentCoordinator+RowOperations, +Filtering, +Navigation, +FKNavigation, etc. Each has slightly different surrounding logic — some use the index for write-back, some use the tab for read-only, some interact with confirmDiscardChangesIfNeeded. Mechanical migration risks subtle behavior changes; each warrants its own focused PR with smoke testing.

selectedTabAndIndex is the foothold for those future PRs.

Test plan

  • Build clean
  • Manual: Next, Previous, First, Last buttons across multiple pages
  • Manual: change page size — list reloads
  • Manual: type offset + Go — jumps correctly
  • Manual: each action scrolls back to row 0

…dTabAndIndex

Pagination had seven methods (goToNextPage, goToPreviousPage, goToFirstPage, goToLastPage, updatePageSize, updateOffset, applyPaginationSettings) where six followed the same shape:

  guard let tabIndex = tabManager.selectedTabIndex,
        tabIndex < tabManager.tabs.count,
        tabManager.tabs[tabIndex].pagination.<condition> else { return }
  paginateAfterConfirmation(tabIndex: tabIndex) { pagination in
      pagination.<action>()
  }

Six guard prologues and six tail calls collapse into one-line bodies via:

- New QueryTabManager.selectedTabAndIndex helper that returns (tab, index) atomically with the bounds check, replacing the two-step (selectedTabIndex + bounds-check + tabs[index]) pattern.
- New private paginateIfPossible(where:mutate:) that captures the selected-tab + condition + paginateAfterConfirmation chain. Each public pagination method becomes a one-liner using a KeyPath or short closure for the precondition.

Behavior unchanged. -30 LOC in Pagination, +5 in QueryTabManager. Smoke-tested all six pagination actions plus offset Go.

selectedTabAndIndex is also a foothold for further coordinator dedup — there are ~14 other selectedTabIndex + bounds-check sites in MainContentCoordinator extensions that could migrate, but each has slightly different surrounding logic so they're left for separate focused PRs.
Four cases:
- nilWhenNoSelection: empty manager returns nil
- returnsSelectedTabAfterAdd: addTableTab autoselects the new tab; helper returns it with index 0
- nilWhenSelectionIsStale: if selectedTabId points to a removed tab, the bounds check kicks in and returns nil rather than crashing or returning stale data
- returnsCorrectPairAfterSwitch: explicit selectedTabId assignment resolves to the matching (tab, index) pair

The staleness test is the load-bearing one — it locks the contract that future migrations of the other ~14 selectedTabIndex + bounds-check sites can rely on. Without this, a refactor that loosens the bounds check could silently regress.
@datlechin datlechin merged commit ce570a9 into main Apr 29, 2026
2 checks passed
@datlechin datlechin deleted the refactor/coordinator-pagination-helpers branch April 29, 2026 05:41
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)
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