From 8d1791e643da6762e11adc1710c0e4ffc62864ff Mon Sep 17 00:00:00 2001 From: Ngo Quoc Dat Date: Tue, 15 Sep 2026 16:39:34 +0700 Subject: [PATCH] fix(editor): close the query tab review findings --- ...inSplitViewController+MenuValidation.swift | 8 +++ .../MainSplitViewController.swift | 17 +++++-- .../Query/QueryCommandAvailability.swift | 5 ++ .../Query/QueryResultPresentation.swift | 20 ++++++-- .../Models/Query/ResultSetMenuModel.swift | 16 +++++- TablePro/Models/UI/TrailingPaneState.swift | 9 ++++ TablePro/Views/Editor/QueryEditorBar.swift | 50 ++++++++++++------- TablePro/Views/Editor/QueryEditorView.swift | 2 + .../Main/Child/MainEditorContentView.swift | 2 + .../MainContentCoordinator+Protection.swift | 14 ++++++ .../Main/MainContentCommandActions.swift | 5 ++ .../Query/QueryCommandAvailabilityTests.swift | 17 +++++++ .../Query/QueryResultPresentationTests.swift | 40 +++++++++++++++ 13 files changed, 180 insertions(+), 25 deletions(-) diff --git a/TablePro/Core/Services/Infrastructure/MainSplitViewController+MenuValidation.swift b/TablePro/Core/Services/Infrastructure/MainSplitViewController+MenuValidation.swift index 09c9a10e66..d19fe5f983 100644 --- a/TablePro/Core/Services/Infrastructure/MainSplitViewController+MenuValidation.swift +++ b/TablePro/Core/Services/Infrastructure/MainSplitViewController+MenuValidation.swift @@ -34,6 +34,8 @@ struct MenuValidationContext: Equatable { var canRestorePreviousValues = false var isQueryExecuting = false var hasQueryText = false + var canClearQuery = false + var canClearResults = false var hasPendingChanges = false var hasDataPendingChanges = false var hasRowSelection = false @@ -193,6 +195,10 @@ extension MainSplitViewController: NSMenuItemValidation { return context.isQueryTab && context.isConnected && context.hasQueryText && !context.isQueryExecuting case #selector(cancelQuery(_:)): return context.isQueryExecuting + case #selector(clearQuery(_:)): + return context.canClearQuery + case #selector(clearResults(_:)): + return context.canClearResults case #selector(previewSQL(_:)): return context.isConnected && context.hasDataPendingChanges case #selector(saveAsFavorite(_:)): @@ -353,6 +359,8 @@ extension MainSplitViewController: NSMenuItemValidation { canRestorePreviousValues: actions.canRestorePreviousValues, isQueryExecuting: actions.isQueryExecuting, hasQueryText: actions.hasQueryText, + canClearQuery: actions.canClearQuery, + canClearResults: actions.canClearResults, hasPendingChanges: actions.hasPendingChanges, hasDataPendingChanges: actions.hasDataPendingChanges, hasRowSelection: actions.hasRowSelection, diff --git a/TablePro/Core/Services/Infrastructure/MainSplitViewController.swift b/TablePro/Core/Services/Infrastructure/MainSplitViewController.swift index 73a5d54a45..a7241b9a4f 100644 --- a/TablePro/Core/Services/Infrastructure/MainSplitViewController.swift +++ b/TablePro/Core/Services/Infrastructure/MainSplitViewController.swift @@ -333,6 +333,10 @@ internal final class MainSplitViewController: NSSplitViewController, TrailingPan super.splitViewDidResizeSubviews(notification) recomputeWindowMinSize() toolbarOwner?.syncSidebarSelection() + /// A divider drag can collapse the trailing pane without going through `hideTrailingPane`, + /// and history's activation is keyed on the flag rather than on the pane, so it has to be + /// reconciled here as well or the panel keeps querying behind a collapsed divider. + syncHistoryPanelVisibility() } override func viewWillAppear() { @@ -935,6 +939,11 @@ internal final class MainSplitViewController: NSSplitViewController, TrailingPan isAIEnabled: AppSettingsManager.shared.ai.enabled ) inspectorPaneHost.show(selected.panes.trailingPane(for: surface)) + /// The single reconciliation point. Every route that changes which surface the pane shows, + /// or which workspace it belongs to, ends here: a reveal, a hide, a workspace switch and an + /// availability change all call this, so the flag cannot describe a different connection's + /// pane than the one on screen. + syncHistoryPanelVisibility() } // MARK: - Session Bindings @@ -1039,7 +1048,9 @@ internal final class MainSplitViewController: NSSplitViewController, TrailingPan /// the inspector unconditionally swapped the assistant out from under a half-typed question and /// persisted the inspector as that connection's surface, on every row the user clicked. func revealInspectorForSelection() { - guard !isAssistantVisible else { return } + /// History is deliberately opened, the same way the assistant is, so a row click must not + /// take it away and persist the inspector as this connection's surface behind it. + guard !isAssistantVisible, !isHistoryVisible else { return } showInspector() } @@ -1056,8 +1067,8 @@ internal final class MainSplitViewController: NSSplitViewController, TrailingPan /// on screen, and it is what the Find Past Queries tip reads to know it has been answered. /// Leaving it behind when history moved into the trailing pane would have left the panel /// mounted and inert, which is the shape of a pane that renders nothing forever. - private func syncHistoryPanelVisibility() { - guard let connectionId = workspaces.selected?.connectionId else { return } + internal func syncHistoryPanelVisibility() { + guard isViewLoaded, let connectionId = workspaces.selected?.connectionId else { return } let showing = isTrailingPaneOpen && resolvedTrailingSurface == .history let state = HistoryPanelState.forConnection(connectionId) guard state.isVisible != showing else { return } diff --git a/TablePro/Models/Query/QueryCommandAvailability.swift b/TablePro/Models/Query/QueryCommandAvailability.swift index 383d51377d..e3deb8d4ad 100644 --- a/TablePro/Models/Query/QueryCommandAvailability.swift +++ b/TablePro/Models/Query/QueryCommandAvailability.swift @@ -20,6 +20,10 @@ struct QueryCommandAvailability { let canSaveAsFavorite: Bool let canClearQuery: Bool let canClearResults: Bool + /// Whether the Run menu has anything live in it. Clear Query leaves the results standing and + /// takes `canRun` away with it, so gating the menu on Run alone hid Clear Results at the one + /// moment it was the command the reader wanted. + let canOpenRunMenu: Bool let explainVariants: [ExplainVariant] /// Every hint the bar shows, resolved here so a disabled control can say why rather than just @@ -47,6 +51,7 @@ struct QueryCommandAvailability { canSaveAsFavorite = hasQueryText canClearQuery = hasQueryText canClearResults = hasResults + canOpenRunMenu = canRun || hasQueryText || hasResults runHint = Self.hint( base: shortcutHint(String(localized: "Run"), .executeQuery), diff --git a/TablePro/Models/Query/QueryResultPresentation.swift b/TablePro/Models/Query/QueryResultPresentation.swift index 7bb87f2108..e22a19cd62 100644 --- a/TablePro/Models/Query/QueryResultPresentation.swift +++ b/TablePro/Models/Query/QueryResultPresentation.swift @@ -50,6 +50,9 @@ struct QueryResultInputs: Equatable { var activeResultRowsAffected = 0 var activeResultExecutionTime: TimeInterval? var activeResultStatusMessage: String? + /// A failed result carries its own message, which outlives the tab's. Pin a failure, run + /// something that works, and `executionErrorMessage` is cleared while this one is not. + var activeResultErrorMessage: String? var loadedColumnCount = 0 var loadedRowCount = 0 var executionErrorMessage: String? @@ -84,9 +87,18 @@ struct QueryResultPresentation: Equatable { content = Self.resolveContent(inputs) showsResultSetSelector = Self.resolvesResultSetSelector(inputs) showsFilterChrome = Self.resolvesFilterChrome(inputs) - showsFindBar = inputs.isFindBarVisible && inputs.tabType == .table + showsFindBar = inputs.isFindBarVisible + && inputs.tabType == .table + && inputs.viewMode.showsFindBar showsStatusBar = true - showsErrorBanner = inputs.executionErrorMessage != nil + showsErrorBanner = Self.resolvedError(inputs) != nil + } + + /// The error the pane is actually showing. The active result's own message wins, because it + /// describes the result on screen; the tab's is the fallback for a failure that produced no + /// result set at all. + static func resolvedError(_ inputs: QueryResultInputs) -> String? { + inputs.activeResultErrorMessage ?? inputs.executionErrorMessage } private static func resolveContent(_ inputs: QueryResultInputs) -> QueryResultContent { @@ -140,7 +152,7 @@ struct QueryResultPresentation: Equatable { private static func resolveSettledResult(_ inputs: QueryResultInputs) -> QueryResultContent? { guard inputs.hasExecuted, !inputs.isExecuting else { return nil } - if inputs.hasActiveResultSet, !inputs.activeResultHasColumns, inputs.executionErrorMessage == nil { + if inputs.hasActiveResultSet, !inputs.activeResultHasColumns, resolvedError(inputs) == nil { return .statementSucceeded( rowsAffected: inputs.activeResultRowsAffected, executionTime: inputs.activeResultExecutionTime, @@ -149,7 +161,7 @@ struct QueryResultPresentation: Equatable { } guard inputs.loadedColumnCount == 0 else { return resolveEmptyRows(inputs) } - guard inputs.executionErrorMessage == nil else { return nil } + guard resolvedError(inputs) == nil else { return nil } guard inputs.resultSetCount > 0 else { return .idle } return .statementSucceeded( diff --git a/TablePro/Models/Query/ResultSetMenuModel.swift b/TablePro/Models/Query/ResultSetMenuModel.swift index c7495a269e..9dba28c431 100644 --- a/TablePro/Models/Query/ResultSetMenuModel.swift +++ b/TablePro/Models/Query/ResultSetMenuModel.swift @@ -46,11 +46,25 @@ struct ResultSetMenuModel: Equatable { /// The same count in figures, for the tiers where the bar has no room for the sentence. The /// chooser never leaves the bar entirely, because Pin and Close have no other one-click route. + /// + /// A single result is named rather than counted, and a result is named after its table or its + /// leading comment, so the name is as long as the identifier. The chooser is `fixedSize`, so an + /// unbounded name here sets the bar's own floor and pushes the grid out of a narrow pane. var compactTitle: String { - guard total > 1 else { return entries.first?.label ?? "" } + guard total > 1 else { return Self.truncated(entries.first?.label ?? "") } return "\(activeOrdinal)/\(total)" } + /// Long enough to tell two results apart, short enough that no name decides the bar's width. + /// The full name stays on the menu entry and on the control's accessibility label. + private static func truncated(_ label: String) -> String { + let value = label as NSString + guard value.length > compactTitleCharacterLimit else { return label } + return value.substring(to: compactTitleCharacterLimit) + "…" + } + + private static let compactTitleCharacterLimit = 16 + var activeEntry: ResultSetMenuEntry? { entries.first { $0.isActive } } diff --git a/TablePro/Models/UI/TrailingPaneState.swift b/TablePro/Models/UI/TrailingPaneState.swift index 40854b2ead..0799b0b2e7 100644 --- a/TablePro/Models/UI/TrailingPaneState.swift +++ b/TablePro/Models/UI/TrailingPaneState.swift @@ -43,6 +43,15 @@ import os let raw = defaults.string(forKey: Self.surfaceKey(connectionId)), let stored = TrailingPaneSurface(rawValue: raw) { self.surface = stored + } else if let connectionId, HistoryPanelPreferencesStorage.load(for: connectionId).isVisible { + /// This connection last had the query history drawer open, and the drawer is now a + /// surface of this pane. Adopting it here is what carries that reader across the move + /// instead of silently closing their history on the first launch after upgrading. + /// + /// Deterministic without a migration flag: it is reachable only while no surface has + /// ever been stored for the connection, and storing one is what this initializer's + /// `didSet` does the first time anything changes it. + self.surface = .history } else { self.surface = .inspector } diff --git a/TablePro/Views/Editor/QueryEditorBar.swift b/TablePro/Views/Editor/QueryEditorBar.swift index 188715da66..6d1a9c48cb 100644 --- a/TablePro/Views/Editor/QueryEditorBar.swift +++ b/TablePro/Views/Editor/QueryEditorBar.swift @@ -28,6 +28,9 @@ struct QueryEditorBar: View { let commands: QueryCommandAvailability let isExecuting: Bool let vimMode: VimMode? + /// Whether the first-run tip pointing at query history is still owed. It anchors to the Run + /// menu, which is the control that produces the history it is telling the reader about. + let showsHistoryTip: Bool let onRun: () -> Void let onRunAllStatements: () -> Void @@ -121,6 +124,11 @@ struct QueryEditorBar: View { /// permanently dimmed button costs width the editor wants. TablePlus does the same: its Cancel /// appears in the query editor for a long query rather than standing there dimmed. The two are /// never both actionable, so nothing is reachable in one state and not the other. + /// + /// The two halves are separately enabled, which is the whole reason this is a `ControlGroup` + /// and not a `Menu(primaryAction:)`. Clear Query leaves the results standing and makes Run + /// unavailable, and disabling one control for both would have taken Clear Results down with it + /// at exactly the moment the reader wanted it. @ViewBuilder private var runControl: some View { if isExecuting { @@ -131,26 +139,34 @@ struct QueryEditorBar: View { .help(commands.stopHint) .accessibilityIdentifier("query-stop") } else { - Menu { - Button(String(localized: "Run All Statements"), action: onRunAllStatements) - Button(String(localized: "Run Without Limit"), action: onRunWithoutLimit) - Divider() - Button(String(localized: "Clear Query"), action: onClearQuery) - .disabled(!commands.canClearQuery) - Button(String(localized: "Clear Results"), action: onClearResults) - .disabled(!commands.canClearResults) - } label: { - Label(String(localized: "Run"), systemImage: "play.fill") - } primaryAction: { - onRun() + ControlGroup { + Button(String(localized: "Run"), systemImage: "play.fill", action: onRun) + .labelStyle(.titleAndIcon) + .disabled(!commands.canRun) + .help(commands.runHint) + .accessibilityIdentifier("query-run") + + Menu(String(localized: "Run Options"), systemImage: "chevron.down") { + Button(String(localized: "Run All Statements"), action: onRunAllStatements) + .disabled(!commands.canRun) + Button(String(localized: "Run Without Limit"), action: onRunWithoutLimit) + .disabled(!commands.canRun) + Divider() + Button(String(localized: "Clear Query"), action: onClearQuery) + .disabled(!commands.canClearQuery) + Button(String(localized: "Clear Results"), action: onClearResults) + .disabled(!commands.canClearResults) + } + .labelStyle(.iconOnly) + .disabled(!commands.canOpenRunMenu) + .accessibilityIdentifier("query-run-menu") + .modifier(FeatureTipPopoverAnchor( + tip: FindPastQueriesTip(shortcut: FeatureTipShortcut.display(for: .toggleHistory)), + isEnabled: showsHistoryTip + )) } - .menuStyle(.button) - .buttonStyle(.borderedProminent) .controlSize(.small) .fixedSize() - .disabled(!commands.canRun) - .help(commands.runHint) - .accessibilityIdentifier("query-run") } } } diff --git a/TablePro/Views/Editor/QueryEditorView.swift b/TablePro/Views/Editor/QueryEditorView.swift index b2945cebe4..ae294b8fcb 100644 --- a/TablePro/Views/Editor/QueryEditorView.swift +++ b/TablePro/Views/Editor/QueryEditorView.swift @@ -36,6 +36,7 @@ struct QueryEditorView: View { let scope: QueryScopeBarModel let commands: QueryCommandAvailability + var showsHistoryTip: Bool = false var onRun: () -> Void var onRunAllStatements: () -> Void var onRunWithoutLimit: () -> Void @@ -56,6 +57,7 @@ struct QueryEditorView: View { commands: commands, isExecuting: isExecuting, vimMode: AppSettingsManager.shared.editor.vimModeEnabled ? vimMode : nil, + showsHistoryTip: showsHistoryTip, onRun: onRun, onRunAllStatements: onRunAllStatements, onRunWithoutLimit: onRunWithoutLimit, diff --git a/TablePro/Views/Main/Child/MainEditorContentView.swift b/TablePro/Views/Main/Child/MainEditorContentView.swift index 752ab50f4c..8623688d1e 100644 --- a/TablePro/Views/Main/Child/MainEditorContentView.swift +++ b/TablePro/Views/Main/Child/MainEditorContentView.swift @@ -459,6 +459,7 @@ struct MainEditorContentView: View { }, scope: scopeBarModel(for: tab), commands: commandAvailability(for: tab), + showsHistoryTip: showsHistoryTip, onRun: { coordinator.runQuery(viewport: .firstRow) }, onRunAllStatements: { coordinator.runAllStatements() }, onRunWithoutLimit: { coordinator.runQuery(viewport: .firstRow, bypassRowLimit: true) }, @@ -825,6 +826,7 @@ struct MainEditorContentView: View { inputs.activeResultRowsAffected = activeResultSet?.rowsAffected ?? 0 inputs.activeResultExecutionTime = activeResultSet?.executionTime inputs.activeResultStatusMessage = activeResultSet?.statusMessage + inputs.activeResultErrorMessage = activeResultSet?.errorMessage inputs.loadedColumnCount = rows.columns.count inputs.loadedRowCount = rows.rows.count inputs.executionErrorMessage = tab.execution.errorMessage diff --git a/TablePro/Views/Main/Extensions/MainContentCoordinator+Protection.swift b/TablePro/Views/Main/Extensions/MainContentCoordinator+Protection.swift index 0ac3d62ccc..e94f6abfb1 100644 --- a/TablePro/Views/Main/Extensions/MainContentCoordinator+Protection.swift +++ b/TablePro/Views/Main/Extensions/MainContentCoordinator+Protection.swift @@ -1,3 +1,4 @@ +import AppKit import Foundation /// The single answer to "would closing or quitting destroy something the user cannot get back". @@ -127,6 +128,19 @@ extension MainContentCoordinator { tab.showsUnsavedIndicator || hasUnsavedWork(in: tab) } + /// Pushes the selected tab's answer onto the window's close button. + /// + /// The editor's text binding does this on every keystroke, which covers typing. A command that + /// changes the text without going through that binding has to say so itself, or the dot + /// describes the tab as it was before the command ran. + func refreshUnsavedIndicator() { + guard let tab = tabManager.selectedTab, let window = contentWindow else { return } + let showsIndicator = showsUnsavedIndicator(for: tab) + Task { @MainActor in + window.isDocumentEdited = showsIndicator + } + } + func hasAnyUnsavedWork() -> Bool { changeManager.hasChanges || hasPendingDestructiveTableOps diff --git a/TablePro/Views/Main/MainContentCommandActions.swift b/TablePro/Views/Main/MainContentCommandActions.swift index e256c5eda3..a1a49d713d 100644 --- a/TablePro/Views/Main/MainContentCommandActions.swift +++ b/TablePro/Views/Main/MainContentCommandActions.swift @@ -1233,6 +1233,11 @@ final class MainContentCommandActions { coordinator.tabManager.mutate(at: tabIndex) { $0.content.query = "" } coordinator.toolbarState.hasQueryText = false coordinator.scheduleDraftSave() + /// The editor's own text binding recomputes this on every keystroke, and emptying the tab + /// from a command does not go through that binding. Without it a scratch tab keeps the + /// dirty dot it no longer deserves, and a file-backed tab that this command just emptied + /// is not marked modified until some other window event happens to recompute it. + coordinator.refreshUnsavedIndicator() } var canClearQuery: Bool { diff --git a/TableProTests/Models/Query/QueryCommandAvailabilityTests.swift b/TableProTests/Models/Query/QueryCommandAvailabilityTests.swift index eee14b891e..f37af9d513 100644 --- a/TableProTests/Models/Query/QueryCommandAvailabilityTests.swift +++ b/TableProTests/Models/Query/QueryCommandAvailabilityTests.swift @@ -82,6 +82,23 @@ struct QueryCommandAvailabilityTests { #expect(Self.make(hasQueryText: false).formatHint.contains("nothing to format")) } + /// Clear Query leaves the results standing and takes `canRun` with it. Gating the whole Run + /// menu on `canRun` then hid Clear Results at exactly the moment it was the live command. + @Test("The Run menu stays reachable while a clear command is still valid") + func runMenuOutlivesRun() { + let clearedQueryWithResults = Self.make(hasQueryText: false, hasResults: true) + #expect(clearedQueryWithResults.canRun == false) + #expect(clearedQueryWithResults.canClearResults) + #expect(clearedQueryWithResults.canOpenRunMenu) + + let offlineWithText = Self.make(isConnected: false, hasQueryText: true) + #expect(offlineWithText.canRun == false) + #expect(offlineWithText.canOpenRunMenu) + + let nothingAtAll = Self.make(hasQueryText: false, hasResults: false) + #expect(nothingAtAll.canOpenRunMenu == false) + } + @Test("Clear Results follows the results, not the query text") func clearResultsFollowsResults() { #expect(Self.make(hasResults: true).canClearResults) diff --git a/TableProTests/Models/Query/QueryResultPresentationTests.swift b/TableProTests/Models/Query/QueryResultPresentationTests.swift index 001fb20baf..1a09dfe538 100644 --- a/TableProTests/Models/Query/QueryResultPresentationTests.swift +++ b/TableProTests/Models/Query/QueryResultPresentationTests.swift @@ -132,6 +132,46 @@ struct QueryResultPresentationTests { #expect(presentation.showsResultSetSelector) } + /// Pin a failure, run something that works, and the tab's own message is cleared while the + /// pinned result's is not. Switching back used to resolve the failure as a success, because the + /// error result reports no columns. + @Test("A pinned failure still reads as a failure after a later run succeeds") + func pinnedFailureStaysAFailure() { + var inputs = QueryResultInputs() + inputs.hasExecuted = true + inputs.hasActiveResultSet = true + inputs.activeResultHasColumns = false + inputs.activeResultErrorMessage = "syntax error" + inputs.executionErrorMessage = nil + + let presentation = QueryResultPresentation(inputs: inputs) + + #expect(presentation.showsErrorBanner) + if case .statementSucceeded = presentation.content { + Issue.record("A pinned failure must never resolve to the success view") + } + } + + /// The find bar searches the data grid's coordinator. Switching to a mode that unmounts the + /// grid left it on screen over nothing to search. + @Test("The find bar follows the mode, not just the tab type") + func findBarFollowsMode() { + var inputs = QueryResultInputs() + inputs.tabType = .table + inputs.isFindBarVisible = true + + inputs.viewMode = .data + #expect(QueryResultPresentation(inputs: inputs).showsFindBar) + + for mode in [ResultsViewMode.chart, .map, .structure] { + inputs.viewMode = mode + #expect( + QueryResultPresentation(inputs: inputs).showsFindBar == mode.showsFindBar, + "find bar must follow ResultsViewMode.showsFindBar for \(mode)" + ) + } + } + @Test("Structure mode needs a table to show the structure of") func structureNeedsATable() { var inputs = QueryResultInputs()