From 821c463165357c08eb05ee58f8081978b5d8fee8 Mon Sep 17 00:00:00 2001 From: Ngo Quoc Dat Date: Thu, 28 May 2026 17:23:17 +0700 Subject: [PATCH] fix(coordinator): load a table tab's initial query exactly once --- CHANGELOG.md | 1 + ...inContentCoordinator+WindowLifecycle.swift | 63 ++++++++++--------- .../Extensions/MainContentView+Setup.swift | 4 +- .../Views/Main/MainContentCoordinator.swift | 3 + .../MainContentCoordinatorLazyLoadTests.swift | 35 ++++++++--- 5 files changed, 64 insertions(+), 42 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e74fe55e1..2bb1cafd4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,6 +29,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Opening a saved connection that fails now shows the detailed troubleshooting dialog with suggested fixes, the same one Test Connection shows, instead of a generic error alert. (#1425, #483) - Oracle connection errors no longer surface the driver's raw internal message; failures now explain the cause in plain language. (#483) - AWS IAM authentication with a named profile now reads `~/.aws/config` (not just `~/.aws/credentials`) and supports `credential_process`, so profiles backed by SSO, IAM Identity Center, or assume-role work through `aws configure export-credentials`. (#1291) +- Opening a table no longer runs the initial query multiple times before the data arrives. The same query could fire up to four times on a single tab open; it now runs once. - iOS: a connection's Safe Mode setting now survives relaunch. iCloud sync no longer drops the value, so a connection set to Confirm Writes or Read-Only no longer reverts to Off after reopening the app. - iOS: running a query that returns a very large result no longer crashes the app. The query editor keeps the first rows it loads, stops before memory runs low, and tells you to add LIMIT to fetch more. - iOS: Safe Mode "Confirm Writes" now prompts before saving a row edit or inserting a row, matching the query editor. Previously grid edits and inserts saved with no confirmation. diff --git a/TablePro/Views/Main/Extensions/MainContentCoordinator+WindowLifecycle.swift b/TablePro/Views/Main/Extensions/MainContentCoordinator+WindowLifecycle.swift index a8ddd5076..ec2fef2a4 100644 --- a/TablePro/Views/Main/Extensions/MainContentCoordinator+WindowLifecycle.swift +++ b/TablePro/Views/Main/Extensions/MainContentCoordinator+WindowLifecycle.swift @@ -105,37 +105,12 @@ extension MainContentCoordinator { // MARK: - Lazy Load - /// Execute the current tab's query if it is a table tab whose row data is - /// missing or evicted. Apple-pattern guards in cheap-content-first order: - /// trivial content checks reject before the expensive connection probe. - /// Idempotent — repeated calls with the same state are no-ops. func lazyLoadCurrentTabIfNeeded() { guard let tab = tabManager.selectedTab else { return } - guard tab.tabType == .table else { return } - guard tab.execution.errorMessage == nil else { return } - guard !tab.content.query.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty else { return } + guard canAutoLoadTableTab(tab) else { return } + guard tableLoadTasks[tab.id] == nil else { return } - let rows = tabSessionRegistry.tableRows(for: tab.id) - let isEvicted = tabSessionRegistry.isEvicted(tab.id) - let hasFreshRows = !rows.rows.isEmpty && !isEvicted - let hasExecuted = tab.execution.lastExecutedAt != nil && !isEvicted - guard !hasFreshRows, !hasExecuted else { return } - - let hasPendingEdits = - changeManager.hasChanges - || tab.pendingChanges.hasChanges - guard !hasPendingEdits else { return } - - // A previous load that was cancelled mid-flight (e.g. user rapidly - // switched away) leaves `isExecuting = true` with no rows and no - // `lastExecutedAt`. Clear the stale flag inline so the executor's - // own `!tab.execution.isExecuting` guard inside - // `executeTableTabQueryDirectly` doesn't suppress this re-fire. - if tab.execution.isExecuting && rows.rows.isEmpty && tab.execution.lastExecutedAt == nil { - tabManager.mutate(tabId: tab.id) { $0.execution.isExecuting = false } - } else if tab.execution.isExecuting { - return - } + clearAbandonedExecutingFlagIfNeeded(for: tab) guard let session = DatabaseManager.shared.session(for: connectionId), session.isConnected else { @@ -143,9 +118,37 @@ extension MainContentCoordinator { return } + let tabId = tab.id Self.lifecycleLogger.debug( - "[switch] coordinator.lazyLoadCurrentTabIfNeeded executing tabId=\(tab.id, privacy: .public) evicted=\(isEvicted)" + "[switch] coordinator.lazyLoadCurrentTabIfNeeded executing tabId=\(tabId, privacy: .public)" ) - executeTableTabQueryDirectly() + tableLoadTasks[tabId] = Task { @MainActor [weak self] in + guard let self else { return } + defer { self.tableLoadTasks[tabId] = nil } + self.executeTableTabQueryDirectly() + if let task = self.currentQueryTask { + await task.value + } + } + } + + private func canAutoLoadTableTab(_ tab: QueryTab) -> Bool { + guard tab.tabType == .table else { return false } + guard tab.execution.errorMessage == nil else { return false } + guard !tab.content.query.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty else { return false } + + let rows = tabSessionRegistry.tableRows(for: tab.id) + let isEvicted = tabSessionRegistry.isEvicted(tab.id) + let hasFreshRows = !rows.rows.isEmpty && !isEvicted + let hasExecuted = tab.execution.lastExecutedAt != nil && !isEvicted + guard !hasFreshRows, !hasExecuted else { return false } + + let hasPendingEdits = changeManager.hasChanges || tab.pendingChanges.hasChanges + return !hasPendingEdits + } + + private func clearAbandonedExecutingFlagIfNeeded(for tab: QueryTab) { + guard tab.execution.isExecuting, currentQueryTask == nil else { return } + tabManager.mutate(tabId: tab.id) { $0.execution.isExecuting = false } } } diff --git a/TablePro/Views/Main/Extensions/MainContentView+Setup.swift b/TablePro/Views/Main/Extensions/MainContentView+Setup.swift index de36cca30..8882b7510 100644 --- a/TablePro/Views/Main/Extensions/MainContentView+Setup.swift +++ b/TablePro/Views/Main/Extensions/MainContentView+Setup.swift @@ -67,7 +67,7 @@ extension MainContentView { { await coordinator.switchDatabase(to: selectedTab.tableContext.databaseName) } else { - coordinator.executeTableTabQueryDirectly() + coordinator.lazyLoadCurrentTabIfNeeded() } } else { coordinator.needsLazyLoad = true @@ -157,7 +157,7 @@ extension MainContentView { { Task { await coordinator.switchDatabase(to: firstTab.tableContext.databaseName) } } else { - coordinator.executeTableTabQueryDirectly() + coordinator.lazyLoadCurrentTabIfNeeded() } } else { coordinator.needsLazyLoad = true diff --git a/TablePro/Views/Main/MainContentCoordinator.swift b/TablePro/Views/Main/MainContentCoordinator.swift index bc430815e..3c44d5646 100644 --- a/TablePro/Views/Main/MainContentCoordinator.swift +++ b/TablePro/Views/Main/MainContentCoordinator.swift @@ -175,6 +175,7 @@ final class MainContentCoordinator { @ObservationIgnored internal var queryGeneration: Int = 0 @ObservationIgnored internal var currentQueryTask: Task? + @ObservationIgnored internal var tableLoadTasks: [UUID: Task] = [:] @ObservationIgnored internal var redisDatabaseSwitchTask: Task? @ObservationIgnored private var changeManagerUpdateTask: Task? @ObservationIgnored private var activeSortTasks: [UUID: Task] = [:] @@ -612,6 +613,8 @@ final class MainContentCoordinator { fileWatcher = nil currentQueryTask?.cancel() currentQueryTask = nil + for task in tableLoadTasks.values { task.cancel() } + tableLoadTasks.removeAll() changeManagerUpdateTask?.cancel() changeManagerUpdateTask = nil redisDatabaseSwitchTask?.cancel() diff --git a/TableProTests/Views/Main/MainContentCoordinatorLazyLoadTests.swift b/TableProTests/Views/Main/MainContentCoordinatorLazyLoadTests.swift index 7b6fc8aec..84ad90612 100644 --- a/TableProTests/Views/Main/MainContentCoordinatorLazyLoadTests.swift +++ b/TableProTests/Views/Main/MainContentCoordinatorLazyLoadTests.swift @@ -137,20 +137,18 @@ struct MainContentCoordinatorLazyLoadTests { #expect(coordinator.needsLazyLoad == false) } - @Test("Returns early when tab is already executing") - func skipsWhenAlreadyExecuting() { + @Test("Returns early when a load Task is already registered for this tab") + func skipsWhenLoadTaskRegistered() { let (coordinator, tabManager) = makeCoordinator() let tabId = addTableTab(to: tabManager) - seedRows(coordinator, for: tabId, rowCount: 1) - guard let idx = tabManager.tabs.firstIndex(where: { $0.id == tabId }) else { - Issue.record("expected tab to exist") - return - } - tabManager.tabs[idx].execution.lastExecutedAt = Date() - tabManager.tabs[idx].execution.isExecuting = true - coordinator.tabSessionRegistry.evict(for: tabId) + let inFlight = Task { _ = try? await Task.sleep(for: .seconds(60)) } + defer { inFlight.cancel() } + coordinator.tableLoadTasks[tabId] = inFlight coordinator.lazyLoadCurrentTabIfNeeded() + + #expect(coordinator.tableLoadTasks.count == 1) + #expect(coordinator.tableLoadTasks[tabId] != nil) #expect(coordinator.needsLazyLoad == false) } @@ -187,6 +185,23 @@ struct MainContentCoordinatorLazyLoadTests { #expect(coordinator.needsLazyLoad == false) } + @Test("Clears an abandoned executing flag when no in-flight task remains") + func recoversAbandonedExecutingFlag() { + let (coordinator, tabManager) = makeCoordinator() + let tabId = addTableTab(to: tabManager) + guard let idx = tabManager.tabs.firstIndex(where: { $0.id == tabId }) else { + Issue.record("expected tab to exist") + return + } + tabManager.tabs[idx].execution.isExecuting = true + coordinator.currentQueryTask = nil + + coordinator.lazyLoadCurrentTabIfNeeded() + + #expect(tabManager.tabs[idx].execution.isExecuting == false) + #expect(coordinator.needsLazyLoad == true) + } + // MARK: - loadEpoch bump triggers reload after eviction @Test("Eviction bumps the tab's loadEpoch so .task(id:) re-fires")