From e2093a3cef3ecc74f613824c8d58eedf335ac2ef 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:36:36 +0700 Subject: [PATCH 1/2] refactor(coordinator): extract paginateIfPossible helper, add selectedTabAndIndex MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. else { return } paginateAfterConfirmation(tabIndex: tabIndex) { pagination in pagination.() } 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. --- TablePro/Models/Query/QueryTabManager.swift | 5 ++ .../MainContentCoordinator+Pagination.swift | 69 +++++-------------- 2 files changed, 22 insertions(+), 52 deletions(-) diff --git a/TablePro/Models/Query/QueryTabManager.swift b/TablePro/Models/Query/QueryTabManager.swift index 7fd593021..2276f5eef 100644 --- a/TablePro/Models/Query/QueryTabManager.swift +++ b/TablePro/Models/Query/QueryTabManager.swift @@ -45,6 +45,11 @@ final class QueryTabManager { return _tabIndexMap[id] } + var selectedTabAndIndex: (tab: QueryTab, index: Int)? { + guard let index = selectedTabIndex, index < tabs.count else { return nil } + return (tabs[index], index) + } + init() { tabs = [] selectedTabId = nil diff --git a/TablePro/Views/Main/Extensions/MainContentCoordinator+Pagination.swift b/TablePro/Views/Main/Extensions/MainContentCoordinator+Pagination.swift index 761f4e493..f5fda7022 100644 --- a/TablePro/Views/Main/Extensions/MainContentCoordinator+Pagination.swift +++ b/TablePro/Views/Main/Extensions/MainContentCoordinator+Pagination.swift @@ -10,82 +10,47 @@ import Foundation extension MainContentCoordinator { // MARK: - Pagination - /// Navigate to next page func goToNextPage() { - guard let tabIndex = tabManager.selectedTabIndex, - tabIndex < tabManager.tabs.count, - tabManager.tabs[tabIndex].pagination.hasNextPage else { return } - - paginateAfterConfirmation(tabIndex: tabIndex) { pagination in - pagination.goToNextPage() - } + paginateIfPossible(where: \.hasNextPage) { $0.goToNextPage() } } - /// Navigate to previous page func goToPreviousPage() { - guard let tabIndex = tabManager.selectedTabIndex, - tabIndex < tabManager.tabs.count, - tabManager.tabs[tabIndex].pagination.hasPreviousPage else { return } - - paginateAfterConfirmation(tabIndex: tabIndex) { pagination in - pagination.goToPreviousPage() - } + paginateIfPossible(where: \.hasPreviousPage) { $0.goToPreviousPage() } } - /// Navigate to first page func goToFirstPage() { - guard let tabIndex = tabManager.selectedTabIndex, - tabIndex < tabManager.tabs.count, - tabManager.tabs[tabIndex].pagination.hasPreviousPage else { return } - - paginateAfterConfirmation(tabIndex: tabIndex) { pagination in - pagination.goToFirstPage() - } + paginateIfPossible(where: \.hasPreviousPage) { $0.goToFirstPage() } } - /// Navigate to last page func goToLastPage() { - guard let tabIndex = tabManager.selectedTabIndex, - tabIndex < tabManager.tabs.count else { return } - - let tab = tabManager.tabs[tabIndex] - guard tab.pagination.currentPage != tab.pagination.totalPages else { return } - - paginateAfterConfirmation(tabIndex: tabIndex) { pagination in - pagination.goToLastPage() - } + paginateIfPossible(where: { $0.currentPage != $0.totalPages }) { $0.goToLastPage() } } - /// Update page size (limit) and reload func updatePageSize(_ newSize: Int) { - guard let tabIndex = tabManager.selectedTabIndex, - tabIndex < tabManager.tabs.count, - newSize > 0 else { return } - - paginateAfterConfirmation(tabIndex: tabIndex) { pagination in - pagination.updatePageSize(newSize) - } + guard newSize > 0 else { return } + paginateIfPossible { $0.updatePageSize(newSize) } } - /// Update offset and reload func updateOffset(_ newOffset: Int) { - guard let tabIndex = tabManager.selectedTabIndex, - tabIndex < tabManager.tabs.count, - newOffset >= 0 else { return } - - paginateAfterConfirmation(tabIndex: tabIndex) { pagination in - pagination.updateOffset(newOffset) - } + guard newOffset >= 0 else { return } + paginateIfPossible { $0.updateOffset(newOffset) } } - /// Apply both limit and offset changes and reload func applyPaginationSettings() { reloadCurrentPage() } // MARK: - Private - /// Confirm discard if needed, then mutate pagination state and reload. + private func paginateIfPossible( + where condition: (PaginationState) -> Bool = { _ in true }, + mutate: @escaping (inout PaginationState) -> Void + ) { + guard let (tab, tabIndex) = tabManager.selectedTabAndIndex, + condition(tab.pagination) else { return } + paginateAfterConfirmation(tabIndex: tabIndex, mutate: mutate) + } + private func paginateAfterConfirmation( tabIndex: Int, mutate: @escaping (inout PaginationState) -> Void From 4359f7d8b9b794cf3e0f10c82df2723701fb66e5 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:40:17 +0700 Subject: [PATCH 2/2] test(coordinator): cover QueryTabManager.selectedTabAndIndex contract MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../Models/Query/QueryTabManagerTests.swift | 60 +++++++++++++++++++ 1 file changed, 60 insertions(+) create mode 100644 TableProTests/Models/Query/QueryTabManagerTests.swift diff --git a/TableProTests/Models/Query/QueryTabManagerTests.swift b/TableProTests/Models/Query/QueryTabManagerTests.swift new file mode 100644 index 000000000..08421836f --- /dev/null +++ b/TableProTests/Models/Query/QueryTabManagerTests.swift @@ -0,0 +1,60 @@ +// +// QueryTabManagerTests.swift +// TableProTests +// +// Locks the contract for selectedTabAndIndex — the helper that +// MainContentCoordinator+Pagination (and future coordinator extensions) +// use in place of the selectedTabIndex + bounds-check + tabs[index] +// pattern. The tests guard against silent staleness if selectedTabId +// ever points to a removed tab. +// + +import Foundation +import Testing +@testable import TablePro + +@Suite("QueryTabManager.selectedTabAndIndex") +@MainActor +struct QueryTabManagerSelectedTabAndIndexTests { + @Test("returns nil when no tab is selected") + func nilWhenNoSelection() { + let manager = QueryTabManager() + #expect(manager.selectedTabAndIndex == nil) + } + + @Test("returns the selected tab and its index after addTableTab") + func returnsSelectedTabAfterAdd() { + let manager = QueryTabManager() + manager.addTableTab(tableName: "users") + + let result = manager.selectedTabAndIndex + #expect(result?.index == 0) + #expect(result?.tab.tableContext.tableName == "users") + } + + @Test("returns nil when selectedTabId points to a removed tab") + func nilWhenSelectionIsStale() { + let manager = QueryTabManager() + manager.addTableTab(tableName: "users") + let staleId = manager.tabs[0].id + + manager.tabs.removeAll() + manager.selectedTabId = staleId + + #expect(manager.selectedTabAndIndex == nil) + } + + @Test("returns the correct (tab, index) pair after switching tabs") + func returnsCorrectPairAfterSwitch() { + let manager = QueryTabManager() + manager.addTableTab(tableName: "users") + manager.addTableTab(tableName: "orders") + let firstId = manager.tabs[0].id + + manager.selectedTabId = firstId + + let result = manager.selectedTabAndIndex + #expect(result?.index == 0) + #expect(result?.tab.tableContext.tableName == "users") + } +}