From d9751a05647cc645d1fd4d0eb1c4497dcba06506 Mon Sep 17 00:00:00 2001 From: Ngo Quoc Dat Date: Fri, 21 Aug 2026 23:12:00 +0700 Subject: [PATCH] fix(toolbar): derive the window's busy state from the execution registry --- CHANGELOG.md | 2 + .../Coordinators/PaginationCoordinator.swift | 9 +- ...QueryExecutionCoordinator+Parameters.swift | 2 - .../Core/Execution/TabExecutionRegistry.swift | 41 +++++- .../MainWindowToolbar+Buttons.swift | 5 + .../MainWindowToolbar+Validation.swift | 8 +- .../Connection/ConnectionToolbarState.swift | 120 ++++-------------- .../MainContentCoordinator+Explain.swift | 1 - .../MainContentCoordinator+Registry.swift | 2 +- .../Extensions/MainContentView+Helpers.swift | 14 +- .../Main/MainContentCommandActions.swift | 2 +- .../Views/Main/MainContentCoordinator.swift | 35 ++--- .../Toolbar/ExecutionIndicatorView.swift | 2 + .../Views/Toolbar/TableProToolbarView.swift | 2 +- .../Execution/TabExecutionRegistryTests.swift | 80 ++++++++++++ .../Execution/WindowBusyStateGuardTests.swift | 95 ++++++++++++++ .../MainWindowToolbarValidationTests.swift | 46 ++++--- .../MainContentCoordinatorLazyLoadTests.swift | 22 +++- .../Main/QueryFailureReportingTests.swift | 39 ++++-- .../WindowExecutionIndicatorUITests.swift | 79 ++++++++++++ 20 files changed, 436 insertions(+), 170 deletions(-) create mode 100644 TableProTests/Core/Execution/WindowBusyStateGuardTests.swift create mode 100644 TableProUITests/WindowExecutionIndicatorUITests.swift diff --git a/CHANGELOG.md b/CHANGELOG.md index 4952f80cc..055a6927a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - Fixed a crash on macOS 26 and later when the editor redrew a diagnostic underline or search highlight whose text had been edited away. +- The toolbar can no longer sit on "Executing…" after a query has ended, so Stop is not the only way back. (#2342) +- The session context buttons no longer empty out for the length of every query. - The XLSX, MQL and SQL Import plugins linked to a documentation page that did not exist. They now point at Import & Export. ## [0.67.0] - 2026-08-21 diff --git a/TablePro/Core/Coordinators/PaginationCoordinator.swift b/TablePro/Core/Coordinators/PaginationCoordinator.swift index d26586d0c..95b22d0d3 100644 --- a/TablePro/Core/Coordinators/PaginationCoordinator.swift +++ b/TablePro/Core/Coordinators/PaginationCoordinator.swift @@ -140,7 +140,6 @@ final class PaginationCoordinator { parent.cancelAllRowCountTasks() parent.releaseAllExactCounts() parent.reportEndedExecutions(parent.tabExecution.invalidateAll(reason: .cancelledByUser)) - parent.toolbarState.setExecuting(false) for idx in parent.tabManager.tabs.indices where parent.tabManager.tabs[idx].pagination.isBusy { parent.tabManager.mutate(at: idx) { tab in tab.pagination.isLoadingMore = false @@ -288,12 +287,18 @@ final class PaginationCoordinator { let storedParamValues = parent.tabManager.tabs[idx].pagination.baseQueryParameterValues parent.tabManager.mutate(at: idx) { $0.pagination.isLoadingMore = true } - parent.toolbarState.setExecuting(true) + + /// Fetch All extends the result already on screen instead of replacing it, so it validates + /// against the tab's content epoch and cannot claim the tab: claiming mints a new epoch and + /// would discard its own rows. It registers as unclaimed work instead, which is what keeps + /// the titlebar reporting it, and releases that on every exit including cancellation. + let workToken = parent.tabExecution.beginUnclaimedWork(for: tabId) let route = DatabaseManager.shared.executionRoute(for: scope) let startedAt = ContinuousClock.Instant.now let fetchAllTask = Task { [weak self, parent] in + defer { parent.tabExecution.endUnclaimedWork(workToken, for: tabId) } guard let self, !parent.isTearingDown else { return } do { diff --git a/TablePro/Core/Coordinators/QueryExecutionCoordinator+Parameters.swift b/TablePro/Core/Coordinators/QueryExecutionCoordinator+Parameters.swift index 9b02c484c..60eaba570 100644 --- a/TablePro/Core/Coordinators/QueryExecutionCoordinator+Parameters.swift +++ b/TablePro/Core/Coordinators/QueryExecutionCoordinator+Parameters.swift @@ -115,7 +115,6 @@ extension QueryExecutionCoordinator { tab.execution.errorMessage = nil } let tab = parent.tabManager.tabs[index] - parent.toolbarState.setExecuting(true) if PluginManager.shared.supportsQueryProgress(for: parent.connection.type) { parent.installClickHouseProgressHandler() @@ -269,7 +268,6 @@ extension QueryExecutionCoordinator { tab.execution.executionTime = nil tab.execution.errorMessage = nil } - parent.toolbarState.setExecuting(true) let conn = parent.connection let tabId = parent.tabManager.tabs[index].id diff --git a/TablePro/Core/Execution/TabExecutionRegistry.swift b/TablePro/Core/Execution/TabExecutionRegistry.swift index 1ce9392b2..bc578b5bb 100644 --- a/TablePro/Core/Execution/TabExecutionRegistry.swift +++ b/TablePro/Core/Execution/TabExecutionRegistry.swift @@ -50,6 +50,11 @@ internal struct EndedExecution: Equatable, Sendable { /// handles that a tab retarget participated in none of. Busy state is derived from membership here, /// never stored on the tab, because a stored flag is exactly what let a retargeted tab stay busy /// forever and silently swallow every later navigation. +/// +/// The window's chrome derives from it too. A second copy of the same fact lived on +/// `ConnectionToolbarState`, raised and lowered by hand beside each execution and released only +/// behind two ownership checks, so any path that ended an execution without satisfying both left +/// the titlebar, Stop, `Cmd+.` and the disconnect warning describing work that was over (#2342). internal struct TabExecutionRegistry { private struct Entry { let epoch: Int @@ -58,6 +63,7 @@ internal struct TabExecutionRegistry { private var entries: [UUID: Entry] = [:] private var contentEpochs: [UUID: Int] = [:] + private var unclaimedWork: [UUID: Set] = [:] private var lastEpoch: Int = 0 internal init() {} @@ -102,6 +108,7 @@ internal struct TabExecutionRegistry { let ended = entries.removeValue(forKey: tabId).map { EndedExecution(tabId: tabId, startedAt: $0.startedAt, reason: reason) } + unclaimedWork.removeValue(forKey: tabId) lastEpoch += 1 contentEpochs[tabId] = lastEpoch return ended @@ -113,6 +120,7 @@ internal struct TabExecutionRegistry { EndedExecution(tabId: $0.key, startedAt: $0.value.startedAt, reason: reason) } entries.removeAll() + unclaimedWork.removeAll() for tabId in tabIds { lastEpoch += 1 contentEpochs[tabId] = lastEpoch @@ -120,6 +128,32 @@ internal struct TabExecutionRegistry { return ended } + /// Work that runs against a tab without owning its result. + /// + /// Fetch All is why this exists. `claim` mints a new content epoch, which is the very value the + /// fetch validates against before writing its rows back, so claiming would discard the result it + /// runs to extend. It still has to count as busy, or the window reports idle while it works. + /// + /// Keyed by tab like everything else here, so `invalidate` and `invalidateAll` release it on the + /// same terms as a claim. A token tied to nothing could only ever be released by the one + /// function that minted it, which is the shape this file exists to get rid of. + internal mutating func beginUnclaimedWork(for tabId: UUID) -> UUID { + let token = UUID() + unclaimedWork[tabId, default: []].insert(token) + return token + } + + /// Takes no `ExecutionEndReason` because there is nothing to report: this is the completion + /// path, the counterpart of `settle`, not one of the ways an execution is ended from outside. + /// Ending a token the registry no longer holds is a no-op, so work unwinding after Stop has + /// already cleared everything cannot put the window back to busy. + internal mutating func endUnclaimedWork(_ token: UUID, for tabId: UUID) { + unclaimedWork[tabId]?.remove(token) + if unclaimedWork[tabId]?.isEmpty == true { + unclaimedWork.removeValue(forKey: tabId) + } + } + /// Ends a claim that ran to completion and reports whether it still owned the tab. A claim that /// is no longer current settles nothing, so a late result cannot clear the busy state of the /// navigation that superseded it. @@ -142,7 +176,12 @@ internal struct TabExecutionRegistry { entries[tabId] != nil } + /// The window's whole answer to "is anything running here", and the only one. + /// + /// The toolbar's indicator, Stop, `Cmd+.` and the disconnect warning all read this rather than + /// a flag raised and lowered by hand beside each execution. A stored copy is what let the + /// titlebar report a query that had already ended, recoverable only by pressing Stop (#2342). internal var isAnyExecuting: Bool { - !entries.isEmpty + !entries.isEmpty || !unclaimedWork.isEmpty } } diff --git a/TablePro/Core/Services/Infrastructure/MainWindowToolbar+Buttons.swift b/TablePro/Core/Services/Infrastructure/MainWindowToolbar+Buttons.swift index 91123dea8..6c24438b0 100644 --- a/TablePro/Core/Services/Infrastructure/MainWindowToolbar+Buttons.swift +++ b/TablePro/Core/Services/Infrastructure/MainWindowToolbar+Buttons.swift @@ -78,6 +78,11 @@ struct SessionContextToolbarButton: View { .help(context.label) } } + /// Keyed on the connection alone. It used to reload on every query as well, because the + /// toolbar's `executing` case made one look like a connection change, and the load then + /// refused to run and emptied the row of buttons for the query's duration. The only driver + /// that answers `fetchSessionContexts` is Snowflake, which pays two round trips for it, so + /// per-query reloading was not free either. A context the reader switches reloads itself. .task(id: coordinator.toolbarState.connectionState) { await coordinator.loadSessionContexts() } diff --git a/TablePro/Core/Services/Infrastructure/MainWindowToolbar+Validation.swift b/TablePro/Core/Services/Infrastructure/MainWindowToolbar+Validation.swift index 62d8986a9..9a94b73b5 100644 --- a/TablePro/Core/Services/Infrastructure/MainWindowToolbar+Validation.swift +++ b/TablePro/Core/Services/Infrastructure/MainWindowToolbar+Validation.swift @@ -26,11 +26,15 @@ extension MainWindowToolbar: NSToolbarItemValidation { } /// Listed exhaustively so a new state has to choose a side instead of inheriting "alive". + /// + /// `.connecting` counts because the health monitor writes it on every reconnect attempt, and + /// the window keeps showing the session's tabs and rows throughout. Graying the whole toolbar + /// out for the length of a backoff would take Sidebar Toggle with it. static func hasLiveSession(_ state: ToolbarConnectionState) -> Bool { switch state { - case .connected, .executing: + case .connected, .connecting: return true - case .disconnected, .connecting, .error: + case .disconnected, .error: return false } } diff --git a/TablePro/Models/Connection/ConnectionToolbarState.swift b/TablePro/Models/Connection/ConnectionToolbarState.swift index df2eef34d..79ef6c759 100644 --- a/TablePro/Models/Connection/ConnectionToolbarState.swift +++ b/TablePro/Models/Connection/ConnectionToolbarState.swift @@ -3,7 +3,6 @@ // TablePro // // Observable state container for toolbar connection information. -// Centralizes all toolbar-related state in a single, composable object. // import AppKit @@ -13,60 +12,39 @@ import TableProPluginKit // MARK: - Connection State -/// Represents the current state of the database connection +/// The state of the database connection, and only that. +/// +/// Whether a query is running is a separate axis owned by `TabExecutionRegistry`. The two used to +/// share one `executing` case, which let a connection that was merely dialing paint the query +/// indicator, and made every consumer that asked "is this connection up" answer no for the whole +/// duration of any query (#2342). enum ToolbarConnectionState: Equatable { case disconnected case connecting case connected - case executing case error(String) - /// Status indicator color - var indicatorColor: Color { - switch self { - case .disconnected: return .gray - case .connecting: return .orange - case .connected: return .green - case .executing: return .blue - case .error: return .red - } - } - - /// Human-readable description - var description: String { - switch self { - case .disconnected: return String(localized: "Disconnected") - case .connecting: return String(localized: "Connecting…") - case .connected: return String(localized: "Connected") - case .executing: return String(localized: "Executing…") - case .error(let message): return String(format: String(localized: "Error: %@"), message) - } - } - - /// Short label for toolbar display - var label: String { - switch self { - case .disconnected: return String(localized: "Disconnected") - case .connecting: return String(localized: "Connecting") - case .connected: return String(localized: "Connected") - case .executing: return String(localized: "Executing") - case .error: return String(localized: "Error") - } - } - - /// Whether to show activity indicator - var isAnimating: Bool { - switch self { - case .connecting, .executing: return true - default: return false + /// The one mapping from a session's status to what the titlebar shows. Three copies of it used + /// to exist, and two of them dropped the failure's message, so the same failed connection + /// compared unequal to itself and the state was rewritten on every notification. + init(status: ConnectionStatus) { + switch status { + case .disconnected: self = .disconnected + case .connecting: self = .connecting + case .connected: self = .connected + case .error(let message): self = .error(message) } } } // MARK: - Toolbar State -/// Observable state container for the connection toolbar. -/// This is the single source of truth for all toolbar UI state. +/// Observable state container for the connection toolbar's connection and session state: which +/// database, which schema, which safe mode, what the tab holds. +/// +/// Whether anything is running is NOT here. That is derived from `TabExecutionRegistry`, which is +/// the only thing that knows, and a stored copy of it on this object is what let the titlebar +/// report a query that had already ended (#2342). Do not reintroduce one. @Observable @MainActor final class ConnectionToolbarState { @@ -105,26 +83,6 @@ final class ConnectionToolbarState { // MARK: - Query Execution - /// Whether a query is currently executing. - private(set) var isExecuting: Bool = false - - /// Set execution state and update connectionState atomically. - func setExecuting(_ executing: Bool) { - let newState: ToolbarConnectionState - if executing && connectionState == .connected { - newState = .executing - } else if !executing && connectionState == .executing { - newState = .connected - } else { - newState = connectionState - } - - guard executing != isExecuting || newState != connectionState else { return } - - isExecuting = executing - connectionState = newState - } - /// Duration of the last completed query var lastQueryDuration: TimeInterval? @@ -199,23 +157,6 @@ final class ConnectionToolbarState { ) } - /// Tooltip text for the status indicator - var statusTooltip: String { - var parts: [String] = [connectionState.description] - - if let latency = latencyMs { - parts.append(String(format: String(localized: "Latency: %dms"), latency)) - } - - if let lag = replicationLagSeconds { - parts.append(String(format: String(localized: "Replication lag: %ds"), lag)) - } - - parts.append(safeModeLevel.displayName) - - return parts.joined(separator: " • ") - } - // MARK: - Initialization init() {} @@ -266,18 +207,14 @@ final class ConnectionToolbarState { } } - /// Update connection state from ConnectionStatus + /// Update connection state from ConnectionStatus. + /// + /// Guarded like every other write on this object: a redundant write still notifies observers, + /// and one of them keys a `task(id:)` on this value. func updateConnectionState(from status: ConnectionStatus) { - switch status { - case .disconnected: - connectionState = .disconnected - case .connecting: - connectionState = .connecting - case .connected: - connectionState = isExecuting ? .executing : .connected - case .error(let message): - connectionState = .error(message) - } + let resolved = ToolbarConnectionState(status: status) + guard connectionState != resolved else { return } + connectionState = resolved } /// Reset to default disconnected state @@ -291,7 +228,6 @@ final class ConnectionToolbarState { databaseGroupingStrategy = .byDatabase displayColor = databaseType.themeColor connectionState = .disconnected - isExecuting = false lastQueryDuration = nil clickHouseProgress = nil lastClickHouseProgress = nil diff --git a/TablePro/Views/Main/Extensions/MainContentCoordinator+Explain.swift b/TablePro/Views/Main/Extensions/MainContentCoordinator+Explain.swift index 13db3d0e5..b2b5bf1fb 100644 --- a/TablePro/Views/Main/Extensions/MainContentCoordinator+Explain.swift +++ b/TablePro/Views/Main/Extensions/MainContentCoordinator+Explain.swift @@ -132,7 +132,6 @@ extension MainContentCoordinator { let conn = connection tabManager.mutate(at: index) { $0.execution.errorMessage = nil } - toolbarState.setExecuting(true) let explainTask = Task { [weak self] in guard let self else { return } diff --git a/TablePro/Views/Main/Extensions/MainContentCoordinator+Registry.swift b/TablePro/Views/Main/Extensions/MainContentCoordinator+Registry.swift index fb5f16d4b..6d6dba458 100644 --- a/TablePro/Views/Main/Extensions/MainContentCoordinator+Registry.swift +++ b/TablePro/Views/Main/Extensions/MainContentCoordinator+Registry.swift @@ -42,7 +42,7 @@ extension MainContentCoordinator { static func hasRunningQuery(forConnection connectionId: UUID) -> Bool { activeCoordinators.values .filter { $0.connectionId == connectionId } - .contains { $0.toolbarState.isExecuting } + .contains { $0.tabExecution.isAnyExecuting } } static func allTabs(for connectionId: UUID) -> [QueryTab] { diff --git a/TablePro/Views/Main/Extensions/MainContentView+Helpers.swift b/TablePro/Views/Main/Extensions/MainContentView+Helpers.swift index 711426b43..e92561b3d 100644 --- a/TablePro/Views/Main/Extensions/MainContentView+Helpers.swift +++ b/TablePro/Views/Main/Extensions/MainContentView+Helpers.swift @@ -35,10 +35,7 @@ extension MainContentView { coordinator.lazyLoadCurrentTabIfNeeded() } } - let mappedState = mapSessionStatus(session.status) - if mappedState != toolbarState.connectionState { - toolbarState.connectionState = mappedState - } + toolbarState.updateConnectionState(from: session.status) toolbarState.syncFromSession(for: connection) } @@ -53,15 +50,6 @@ extension MainContentView { } } - private func mapSessionStatus(_ status: ConnectionStatus) -> ToolbarConnectionState { - switch status { - case .connected: return .connected - case .connecting: return .executing - case .disconnected: return .disconnected - case .error: return .error("") - } - } - // MARK: - Inspector Context func scheduleInspectorUpdate() { diff --git a/TablePro/Views/Main/MainContentCommandActions.swift b/TablePro/Views/Main/MainContentCommandActions.swift index db23022b3..285b88582 100644 --- a/TablePro/Views/Main/MainContentCommandActions.swift +++ b/TablePro/Views/Main/MainContentCommandActions.swift @@ -293,7 +293,7 @@ final class MainContentCommandActions { /// existing proves nothing: it is kept alive across a lost session so a reconnect can restore /// the user's tabs. var isConnected: Bool { coordinator?.splitViewController?.isConnected ?? false } - var isQueryExecuting: Bool { coordinator?.toolbarState.isExecuting ?? false } + var isQueryExecuting: Bool { coordinator?.tabExecution.isAnyExecuting ?? false } var safeModeLevel: SafeModeLevel { coordinator?.toolbarState.safeModeLevel ?? connection.safeModeLevel } diff --git a/TablePro/Views/Main/MainContentCoordinator.swift b/TablePro/Views/Main/MainContentCoordinator.swift index 248132cb9..e3c674d98 100644 --- a/TablePro/Views/Main/MainContentCoordinator.swift +++ b/TablePro/Views/Main/MainContentCoordinator.swift @@ -948,7 +948,7 @@ final class MainContentCoordinator { toolbarState.update(from: connection) if let session = services.databaseManager.session(for: connectionId) { - toolbarState.connectionState = mapSessionStatus(session.status) + toolbarState.updateConnectionState(from: session.status) if let driver = session.driver { toolbarState.databaseVersion = driver.serverVersion } @@ -969,16 +969,6 @@ final class MainContentCoordinator { await loadSchemaIfNeeded() } - /// Map ConnectionStatus to ToolbarConnectionState - private func mapSessionStatus(_ status: ConnectionStatus) -> ToolbarConnectionState { - switch status { - case .connected: return .connected - case .connecting: return .executing - case .disconnected: return .disconnected - case .error: return .error("") - } - } - // MARK: - Query Execution func runQuery(trigger: TableLoadTrigger = .userInitiated, bypassRowLimit: Bool = false) { @@ -1231,7 +1221,6 @@ final class MainContentCoordinator { tab.execution.errorMessage = nil } let tab = tabManager.tabs[index] - toolbarState.setExecuting(true) if services.pluginManager.supportsQueryProgress(for: connection.type) { installClickHouseProgressHandler() @@ -1262,7 +1251,6 @@ final class MainContentCoordinator { tabManager.mutate(at: index) { tab in tab.execution.errorMessage = String(localized: "Not connected to database") } - toolbarState.setExecuting(false) return } @@ -1417,14 +1405,16 @@ final class MainContentCoordinator { currentQueryTaskOwner = claim } - /// Retires the window's task handle and the spinner it drives, but only for the execution that - /// installed them. A completion that owns its own tab can still be a stranger to the query the - /// window is running, and clearing that one's spinner reports on work still in flight. + /// Retires the window's Stop handle, but only for the execution that installed it. A completion + /// that owns its own tab can still be a stranger to the query the window is running, and taking + /// that one's handle down would leave a live query with nothing to cancel it. + /// + /// It no longer reports anything: what the titlebar shows is derived from `tabExecution`, so a + /// completion that cannot retire the handle can no longer leave the window claiming to be busy. internal func retireQueryTask(for claim: TabExecutionClaim?) { guard currentQueryTaskOwner == claim else { return } currentQueryTask = nil currentQueryTaskOwner = nil - toolbarState.setExecuting(false) } internal func cancelInFlightQueryTask(reach: DriverCancellationReach = .userStop) { @@ -1443,19 +1433,14 @@ final class MainContentCoordinator { /// new claim is minted is what makes "the user navigated away and no successor ever ran" still /// discard the old result, which a counter that only moved on a successful start could not do. /// - /// Clearing the spinner here is what makes the cancelled execution's own completion free to stay - /// silent. A retarget need not be followed by a successor, so nothing else would put the - /// titlebar back to idle, and a stuck spinner keeps Stop enabled and makes Disconnect warn about - /// a query that is not running. + /// Removing the entry is also what puts the titlebar back to idle, because the indicator reads + /// the registry. A retarget need not be followed by a successor, and nothing else would have + /// lowered a stored flag. internal func supersedeExecution(for tabId: UUID) { reportEndedExecutions(tabExecution.invalidate(tabId, reason: .supersededNavigation).map { [$0] } ?? []) cancelTableLoad(for: tabId) cancelRowCountTask(for: tabId) - let hadInFlightQuery = currentQueryTask != nil cancelInFlightQueryTask(reach: .supersededNavigation) - if hadInFlightQuery { - toolbarState.setExecuting(false) - } } /// Reset execution state when a query is cancelled. The task handle is retired through the same diff --git a/TablePro/Views/Toolbar/ExecutionIndicatorView.swift b/TablePro/Views/Toolbar/ExecutionIndicatorView.swift index 5051edcda..5841b9028 100644 --- a/TablePro/Views/Toolbar/ExecutionIndicatorView.swift +++ b/TablePro/Views/Toolbar/ExecutionIndicatorView.swift @@ -22,6 +22,7 @@ struct ExecutionIndicatorView: View { ProgressView() .controlSize(.small) .accessibilityLabel(String(localized: "Query executing")) + .accessibilityIdentifier("execution-indicator") if let progress = clickHouseProgress { Text(progress.formattedLive) .font(.system(.subheadline, design: .monospaced).weight(.regular)) @@ -39,6 +40,7 @@ struct ExecutionIndicatorView: View { } .buttonStyle(.plain) .controlSize(.small) + .accessibilityIdentifier("execution-stop") .help(String(localized: "Cancel Query (⌘.)")) } else if let chProgress = lastClickHouseProgress { Text(chProgress.formattedSummary) diff --git a/TablePro/Views/Toolbar/TableProToolbarView.swift b/TablePro/Views/Toolbar/TableProToolbarView.swift index f7bc05829..367ac73ba 100644 --- a/TablePro/Views/Toolbar/TableProToolbarView.swift +++ b/TablePro/Views/Toolbar/TableProToolbarView.swift @@ -56,7 +56,7 @@ struct ToolbarPrincipalContent: View { )) ExecutionIndicatorView( - isExecuting: state.isExecuting, + isExecuting: coordinator?.tabExecution.isAnyExecuting ?? false, lastDuration: state.lastQueryDuration, clickHouseProgress: state.clickHouseProgress, lastClickHouseProgress: state.lastClickHouseProgress, diff --git a/TableProTests/Core/Execution/TabExecutionRegistryTests.swift b/TableProTests/Core/Execution/TabExecutionRegistryTests.swift index 55e25817e..7ddaf8c73 100644 --- a/TableProTests/Core/Execution/TabExecutionRegistryTests.swift +++ b/TableProTests/Core/Execution/TabExecutionRegistryTests.swift @@ -153,6 +153,86 @@ struct TabExecutionRegistryTests { } + /// Fetch All extends the result already on screen, so it validates against the content epoch and + /// cannot claim the tab without discarding its own rows. The window still has to call it busy. + @Test("Unclaimed work makes the window busy without claiming the tab") + func unclaimedWorkCountsAsBusy() { + var registry = TabExecutionRegistry() + let tabId = UUID() + let epochBefore = registry.contentEpoch(for: tabId) + + let token = registry.beginUnclaimedWork(for: tabId) + + #expect(registry.isAnyExecuting) + #expect(registry.isExecuting(tabId) == false) + #expect(registry.contentEpoch(for: tabId) == epochBefore) + + registry.endUnclaimedWork(token, for: tabId) + #expect(registry.isAnyExecuting == false) + } + + @Test("Two pieces of unclaimed work on one tab end independently") + func unclaimedWorkTokensAreIndependent() { + var registry = TabExecutionRegistry() + let tabId = UUID() + let first = registry.beginUnclaimedWork(for: tabId) + let second = registry.beginUnclaimedWork(for: tabId) + + registry.endUnclaimedWork(first, for: tabId) + #expect(registry.isAnyExecuting) + + registry.endUnclaimedWork(second, for: tabId) + #expect(registry.isAnyExecuting == false) + } + + /// Work unwinding after Stop must not put the window back to busy. + @Test("Ending a token the registry has already released is a no-op") + func endingAReleasedTokenIsANoOp() { + var registry = TabExecutionRegistry() + let tabId = UUID() + let token = registry.beginUnclaimedWork(for: tabId) + + _ = registry.invalidateAll(reason: .cancelledByUser) + #expect(registry.isAnyExecuting == false) + + registry.endUnclaimedWork(token, for: tabId) + #expect(registry.isAnyExecuting == false) + } + + @Test("Retargeting a tab releases its unclaimed work with its claim") + func invalidateReleasesUnclaimedWork() { + var registry = TabExecutionRegistry() + let tabId = UUID() + _ = registry.claim(tabId) + _ = registry.beginUnclaimedWork(for: tabId) + + _ = registry.invalidate(tabId, reason: .supersededNavigation) + + #expect(registry.isAnyExecuting == false) + } + + /// The window's chrome reads `isAnyExecuting`, so every way an execution can end has to leave it + /// false. A stored second copy of this answer is what kept the titlebar busy after the work was + /// over, recoverable only by pressing Stop (#2342). + @Test("Every way an execution ends leaves the window idle") + func everyEndingLeavesTheWindowIdle() { + for reason: ExecutionEndReason in [.cancelledByUser, .supersededNavigation, .sessionEnded, .abandoned] { + var registry = TabExecutionRegistry() + let tabId = UUID() + _ = registry.claim(tabId) + #expect(registry.isAnyExecuting) + + _ = registry.invalidate(tabId, reason: reason) + #expect(registry.isAnyExecuting == false) + } + + var settling = TabExecutionRegistry() + let claim = settling.claim(UUID()) + let settled = settling.settle(claim) + #expect(settled) + #expect(settling.isAnyExecuting == false) + } + /// Epochs are window-global so two tabs never share one, which keeps a claim comparable on its /// own without also carrying the tab's mutable identity fields. @Test("Epochs are unique across tabs") diff --git a/TableProTests/Core/Execution/WindowBusyStateGuardTests.swift b/TableProTests/Core/Execution/WindowBusyStateGuardTests.swift new file mode 100644 index 000000000..67ff6067b --- /dev/null +++ b/TableProTests/Core/Execution/WindowBusyStateGuardTests.swift @@ -0,0 +1,95 @@ +// +// WindowBusyStateGuardTests.swift +// TableProTests +// +// "Executing…" and the Stop control beside it are a function of `TabExecutionRegistry`, and of +// nothing else. They used to be a stored bool on `ConnectionToolbarState`, raised and lowered by +// hand beside each execution and released only behind two ownership checks, so any path that ended +// an execution without satisfying both left the titlebar, Stop, `Cmd+.` and the disconnect warning +// describing work that was over. The only way back was pressing Stop (#2342, and #548 before it). +// +// A second copy of that answer cannot come back quietly, so the build fails on one. +// + +import Foundation +@testable import TablePro +import Testing + +@Suite("Window busy state guard") +struct WindowBusyStateGuardTests { + @Test("Nothing stores or writes a second copy of whether the window is busy") + func noStoredWindowExecutionFlag() throws { + let offenders = try Self.sourceLines { line in + line.contains("setExecuting(") || line.contains("toolbarState.isExecuting") + } + #expect( + offenders.isEmpty, + """ + Whether anything is running is derived from `tabExecution.isAnyExecuting`. A stored copy \ + desynchronizes the moment one path ends an execution without lowering it: \ + \(offenders.map(\.description).sorted()) + """ + ) + } + + @Test("The toolbar state holds no execution flag of its own") + func toolbarStateHoldsNoExecutionFlag() throws { + let offenders = try Self.sourceLines(in: "ConnectionToolbarState.swift") { line in + line.contains("isExecuting") && !line.trimmingCharacters(in: .whitespaces).hasPrefix("///") + } + #expect( + offenders.isEmpty, + """ + `ConnectionToolbarState` owns the connection's state. Whether a query is running belongs \ + to `TabExecutionRegistry`: \(offenders.map(\.description).sorted()) + """ + ) + } + + /// The scan above only proves the old flag is gone. This proves the indicator reads the registry, + /// so a future edit cannot satisfy both scans by wiring the toolbar to some third value. + @Test("The execution indicator is fed from the execution registry") + func executionIndicatorReadsTheRegistry() throws { + let callSites = try Self.sourceLines { $0.contains("isExecuting: coordinator?.tabExecution.isAnyExecuting") } + #expect(callSites.count == 1) + } + + private struct SourceLine { + let file: String + let line: Int + + var description: String { "\(file):\(line)" } + } + + private static func sourceLines( + in fileName: String? = nil, + matching predicate: (String) -> Bool + ) throws -> [SourceLine] { + let sourceRoot = try repoRoot().appendingPathComponent("TablePro") + guard let enumerator = FileManager.default.enumerator( + at: sourceRoot, + includingPropertiesForKeys: [.isRegularFileKey] + ) else { return [] } + + var matches: [SourceLine] = [] + for case let url as URL in enumerator where url.pathExtension == "swift" { + if let fileName, url.lastPathComponent != fileName { continue } + let text = try String(contentsOf: url, encoding: .utf8) + for (offset, line) in text.components(separatedBy: .newlines).enumerated() where predicate(line) { + matches.append(SourceLine(file: url.lastPathComponent, line: offset + 1)) + } + } + return matches + } + + private static func repoRoot() throws -> URL { + var directory = URL(fileURLWithPath: #filePath).deletingLastPathComponent() + for _ in 0 ..< 12 { + if FileManager.default.fileExists(atPath: directory.appendingPathComponent("TablePro.xcodeproj").path) { + return directory + } + directory = directory.deletingLastPathComponent() + } + throw CocoaError(.fileNoSuchFile) + } +} diff --git a/TableProTests/Services/MainWindowToolbarValidationTests.swift b/TableProTests/Services/MainWindowToolbarValidationTests.swift index a5533bba4..1b16d9866 100644 --- a/TableProTests/Services/MainWindowToolbarValidationTests.swift +++ b/TableProTests/Services/MainWindowToolbarValidationTests.swift @@ -198,32 +198,44 @@ struct MainWindowToolbarValidationTests { #expect(MainWindowToolbar.isEnabled(itemIdentifier: unknown, context: context) == true) } - @Test("A running query still counts as a live session") - func executingCountsAsLiveSession() { + /// The health monitor writes `.connecting` on every reconnect attempt while the window keeps + /// showing the session's tabs and rows, so a backoff must not gray the toolbar out. + @Test("A session that is up or reconnecting counts as live") + func connectedAndReconnectingCountAsLiveSession() { #expect(MainWindowToolbar.hasLiveSession(.connected) == true) - #expect(MainWindowToolbar.hasLiveSession(.executing) == true) + #expect(MainWindowToolbar.hasLiveSession(.connecting) == true) #expect(MainWindowToolbar.hasLiveSession(.disconnected) == false) - #expect(MainWindowToolbar.hasLiveSession(.connecting) == false) #expect(MainWindowToolbar.hasLiveSession(.error("boom")) == false) } - @Test("Toolbar state entering and leaving execution keeps a live session") - func toolbarStateStaysLiveWhileExecuting() { + /// The connection's state and whether a query is running are two axes. They shared one case + /// until #2342, which is how a connection that was merely dialing painted the query indicator. + @Test("A running query does not change what the connection state says") + func runningQueryDoesNotChangeConnectionState() { let state = ConnectionToolbarState() - state.connectionState = .connected - state.setExecuting(true) - #expect(state.connectionState == .executing) - #expect(MainWindowToolbar.hasLiveSession(state.connectionState) == true) - - state.setExecuting(false) + state.updateConnectionState(from: .connected) #expect(state.connectionState == .connected) - #expect(MainWindowToolbar.hasLiveSession(state.connectionState) == true) + + state.updateConnectionState(from: .connecting) + #expect(state.connectionState == .connecting) + + state.updateConnectionState(from: .disconnected) + #expect(MainWindowToolbar.hasLiveSession(state.connectionState) == false) + } + + /// A failure's message is part of the state, so the same failure has to compare equal to itself. + @Test("A connection error keeps its message through the mapping") + func connectionErrorKeepsItsMessage() { + #expect(ToolbarConnectionState(status: .error("boom")) == .error("boom")) + #expect(ToolbarConnectionState(status: .connecting) == .connecting) + #expect(ToolbarConnectionState(status: .connected) == .connected) + #expect(ToolbarConnectionState(status: .disconnected) == .disconnected) } @Test("Session-scoped items stay enabled while a query runs") func sessionScopedItemsStayEnabledWhileExecuting() { let context = makeContext( - connected: MainWindowToolbar.hasLiveSession(.executing), + connected: MainWindowToolbar.hasLiveSession(.connected), hasPendingChanges: true, hasDataPendingChanges: true ) @@ -234,7 +246,7 @@ struct MainWindowToolbarValidationTests { @Test("Session-scoped items stay disabled when the session is gone") func sessionScopedItemsDisabledWhenNotLive() { - for state: ToolbarConnectionState in [.disconnected, .connecting, .error("boom")] { + for state: ToolbarConnectionState in [.disconnected, .error("boom")] { let context = makeContext( connected: MainWindowToolbar.hasLiveSession(state), hasPendingChanges: true, @@ -257,8 +269,8 @@ struct MainWindowToolbarValidationTests { coordinator.toolbarState.connectionState = .connected #expect(owner.validateToolbarItem(refresh) == true) - coordinator.toolbarState.setExecuting(true) - #expect(coordinator.toolbarState.connectionState == .executing) + _ = coordinator.tabExecution.claim(UUID()) + #expect(coordinator.toolbarState.connectionState == .connected) #expect(owner.validateToolbarItem(refresh) == true) coordinator.toolbarState.connectionState = .disconnected diff --git a/TableProTests/Views/Main/MainContentCoordinatorLazyLoadTests.swift b/TableProTests/Views/Main/MainContentCoordinatorLazyLoadTests.swift index 638d19407..02034f7b7 100644 --- a/TableProTests/Views/Main/MainContentCoordinatorLazyLoadTests.swift +++ b/TableProTests/Views/Main/MainContentCoordinatorLazyLoadTests.swift @@ -285,6 +285,24 @@ struct MainContentCoordinatorLazyLoadTests { #expect(coordinator.tabSessionRegistry.isEvicted(foreground) == false) } + /// A claim with no task behind it is healed on the next lazy load. That heal never lowered the + /// window's stored busy flag, so the titlebar kept reporting a query that had no task and no way + /// to finish, and only Stop could clear it (#2342). The window's state is derived now, so the + /// heal is the whole fix. + @Test("Healing an abandoned claim leaves the window reporting idle") + func abandonedClaimLeavesTheWindowIdle() { + let (coordinator, tabManager) = makeCoordinator() + let tabId = addTableTab(to: tabManager) + let claim = coordinator.tabExecution.claim(tabId) + #expect(coordinator.currentQueryTask == nil) + #expect(coordinator.tabExecution.isAnyExecuting) + + coordinator.lazyLoadCurrentTabIfNeeded() + + #expect(coordinator.tabExecution.isCurrent(claim) == false) + #expect(coordinator.tabExecution.isAnyExecuting == false) + } + // MARK: - Regression: handleWindowDidBecomeKey does NOT trigger query work @Test("handleWindowDidBecomeKey does not change tab execution state") @@ -297,13 +315,13 @@ struct MainContentCoordinatorLazyLoadTests { } let executingBefore = coordinator.tabExecution.isExecuting(tabId) let executedAtBefore = tabManager.tabs[idx].execution.lastExecutedAt - let toolbarBefore = coordinator.toolbarState.isExecuting + let toolbarBefore = coordinator.tabExecution.isAnyExecuting coordinator.handleWindowDidBecomeKey() let executingAfter = coordinator.tabExecution.isExecuting(tabId) let executedAtAfter = tabManager.tabs[idx].execution.lastExecutedAt - let toolbarAfter = coordinator.toolbarState.isExecuting + let toolbarAfter = coordinator.tabExecution.isAnyExecuting #expect(executingAfter == executingBefore) #expect(executedAtAfter == executedAtBefore) diff --git a/TableProTests/Views/Main/QueryFailureReportingTests.swift b/TableProTests/Views/Main/QueryFailureReportingTests.swift index 12c1f6ef4..d96e69dc2 100644 --- a/TableProTests/Views/Main/QueryFailureReportingTests.swift +++ b/TableProTests/Views/Main/QueryFailureReportingTests.swift @@ -87,24 +87,24 @@ struct QueryFailureReportingTests { } /// The other half of the gate. A result that lost the tab must not report into it, and must not - /// clear the spinner belonging to the navigation that replaced it. - @Test("A superseded failure writes nothing and leaves the successor's spinner alone") + /// take down the busy state belonging to the navigation that replaced it. + @Test("A superseded failure writes nothing and leaves the successor running") func supersededFailureIsSilent() { let (coordinator, tabManager) = Self.makeCoordinator() let tabId = Self.addQueryTab(to: tabManager) let stale = coordinator.tabExecution.claim(tabId) _ = coordinator.tabExecution.claim(tabId) - coordinator.toolbarState.setExecuting(true) Self.finishFailure(on: coordinator, tabId: tabId, claim: stale) #expect(tabManager.tabs.first?.execution.errorMessage == nil) - #expect(coordinator.toolbarState.isExecuting) + #expect(coordinator.tabExecution.isAnyExecuting) #expect(coordinator.tabExecution.isExecuting(tabId)) } /// The window's task handle is one per window while claims are one per tab, so owning your own - /// tab is not owning the query the window is running. + /// tab is not owning the query the window is running. Retiring the handle says nothing about + /// whether the window is still busy: the executions do. @Test("Retiring the task handle only works for the execution that installed it") func onlyTheInstallerRetiresTheTaskHandle() { let (coordinator, tabManager) = Self.makeCoordinator() @@ -115,15 +115,19 @@ struct QueryFailureReportingTests { let task = Task {} coordinator.installQueryTask(task, for: running) - coordinator.toolbarState.setExecuting(true) coordinator.retireQueryTask(for: stranger) #expect(coordinator.currentQueryTask != nil) - #expect(coordinator.toolbarState.isExecuting) coordinator.retireQueryTask(for: running) #expect(coordinator.currentQueryTask == nil) - #expect(coordinator.toolbarState.isExecuting == false) + #expect(coordinator.tabExecution.isAnyExecuting) + + let runningSettled = coordinator.tabExecution.settle(running) + let strangerSettled = coordinator.tabExecution.settle(stranger) + #expect(runningSettled) + #expect(strangerSettled) + #expect(coordinator.tabExecution.isAnyExecuting == false) task.cancel() } @@ -140,12 +144,11 @@ struct QueryFailureReportingTests { let task = Task {} coordinator.installQueryTask(task, for: successor) - coordinator.toolbarState.setExecuting(true) coordinator.resetExecutionState(claim: cancelled, executionTime: 0.5) #expect(coordinator.currentQueryTask != nil) - #expect(coordinator.toolbarState.isExecuting) + #expect(coordinator.tabExecution.isAnyExecuting) #expect(coordinator.tabExecution.isExecuting(tabId) == false) #expect(coordinator.tabExecution.isCurrent(successor)) task.cancel() @@ -170,6 +173,22 @@ struct QueryFailureReportingTests { #expect(coordinator.toolbarState.isResultsCollapsed) } + /// A retarget need not be followed by a successor, so nothing else puts the window back to idle. + /// The stored flag this replaced was only lowered when a query task happened to be in flight, + /// which is not the case for a navigation that superseded work that had already finished. + @Test("Superseding a tab with no successor leaves the window reporting idle") + func supersedeWithoutSuccessorLeavesTheWindowIdle() { + let (coordinator, tabManager) = Self.makeCoordinator() + let tabId = Self.addQueryTab(to: tabManager) + _ = coordinator.tabExecution.claim(tabId) + #expect(coordinator.tabExecution.isAnyExecuting) + + coordinator.supersedeExecution(for: tabId) + + #expect(coordinator.tabExecution.isAnyExecuting == false) + #expect(coordinator.currentQueryTask == nil) + } + /// The change manager is one per window and holds whichever tab is selected. Clearing it from a /// completion on a different tab throws away edits and undo history the user can never get back. @Test("Clearing pending changes never reaches a tab the user has switched to") diff --git a/TableProUITests/WindowExecutionIndicatorUITests.swift b/TableProUITests/WindowExecutionIndicatorUITests.swift new file mode 100644 index 000000000..0ecaa81c2 --- /dev/null +++ b/TableProUITests/WindowExecutionIndicatorUITests.swift @@ -0,0 +1,79 @@ +// +// WindowExecutionIndicatorUITests.swift +// TableProUITests +// + +import XCTest + +/// #2342: opening a SQLite database and clicking a table left the toolbar on "Executing…" with a +/// live Stop control and no rows, and pressing Stop was the only way out. +/// +/// The moment the indicator is raised is not observable from here, because a local SQLite query +/// finishes in well under XCUITest's polling interval. The moment it is meant to be lowered is, +/// and that is the half the report is about: once the result has landed, nothing in the toolbar may +/// still claim a query is running. +final class WindowExecutionIndicatorUITests: UITestCase { + /// Scoped to the toolbar rather than the window. An identifier lookup that has to walk a window + /// holding a loaded data grid is the expensive query shape in this suite, and this one runs + /// inside a poll. + private func executionIndicator(in window: XCUIElement) -> XCUIElement { + window.toolbars.descendants(matching: .any)["execution-indicator"].firstMatch + } + + private func executionStop(in window: XCUIElement) -> XCUIElement { + window.toolbars.descendants(matching: .any)["execution-stop"].firstMatch + } + + func testTheExecutingIndicatorClearsOnceEachTableHasLoaded() throws { + let app = try launchWithSampleDatabase() + let window = app.windows.firstMatch + + for table in ["Album", "Artist", "Genre"] { + let row = objectBrowserRow(table, in: window) + XCTAssertTrue(row.waitToExist(timeout: 15), "The object browser must list \(table)") + XCTAssertTrue(waitUntilHittable(row, timeout: 15), "\(table)'s row must settle before it is clicked") + clickAtCenter(row) + + let readout = window.staticTexts["result-status-readout"].firstMatch + XCTAssertTrue(readout.waitToExist(timeout: 20), "\(table): the result must land") + + let settled = waitForPredicate(timeout: 15) { + !executionIndicator(in: window).exists + && !executionStop(in: window).exists + } + XCTAssertTrue(settled, "\(table): the toolbar still reports a query that has already finished") + } + } + + /// The other half, and the reason the test above is not vacuous: an indicator wired to something + /// that never becomes true would pass it. A query slow enough to observe proves the toolbar + /// follows the execution registry in both directions. + func testTheExecutingIndicatorAppearsWhileAQueryRunsAndClearsAfterIt() throws { + let app = try launchWithSampleDatabase() + let window = app.windows.firstMatch + + app.typeKey("t", modifierFlags: .command) + let editor = editorTextView(in: app) + XCTAssertTrue(editor.waitToExist(timeout: 10)) + editor.click() + app.typeText( + "WITH RECURSIVE c(x) AS (SELECT 1 UNION ALL SELECT x + 1 FROM c WHERE x < 20000000) SELECT count(*) FROM c;" + ) + app.typeKey(.return, modifierFlags: .command) + + let indicator = executionIndicator(in: window) + XCTAssertTrue( + indicator.waitToExist(timeout: 15), + "The toolbar must report a query that is running" + ) + XCTAssertTrue( + executionStop(in: window).exists, + "A running query must offer Stop" + ) + + let settled = waitForPredicate(timeout: 90) { + !executionIndicator(in: window).exists + } + XCTAssertTrue(settled, "The toolbar must go back to idle once the query has finished") + } +}