fix(tabs): keep the tab in front after a save prompt and dim tab navigation it cannot use

CHANGELOG.md

@@ -328,6 +328,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Show Previous Tab and Show Next Tab listed twice in the Window menu.
 - Control-Tab and Control-Shift-Tab indenting a multi-line selection in the SQL editor.
 - Shift-Tab and Control-Tab accepting an inline AI suggestion instead of outdenting or reaching the menu.
+- Closing a background tab with unsaved work landing on its neighbour instead of the tab you were on.
+- Show Previous Tab, Show Next Tab and Select Tab 1 to 9 enabled in Agent mode and with no tab to go to.
+- Row data of a window's first connection kept in memory after switching to another connection.
 
 ### Security
 

TablePro/Core/Services/Infrastructure/MainSplitViewController+MenuValidation.swift

@@ -84,6 +84,11 @@ struct MenuValidationContext: Equatable {
     var canCloseOtherTabs = false
     var canCloseTabsForOtherDatabases = false
     var canCloseAllTabs = false
+    /// How many editor tabs the connection on screen has open.
+    var editorTabCount = 0
+    /// The tab a Select Tab item names, read off the item being validated. Nil when the item is not
+    /// one of them.
+    var requestedTabNumber: Int?
     /// Whether the connections this window can show hold two tabs between them, which is the least
     /// Control-Tab needs to switch anywhere.
     var hasRecentTabToSwitchTo = false
@@ -170,10 +175,18 @@ extension MainSplitViewController: NSMenuItemValidation {
              #selector(closeResultTab(_:)),
              #selector(focusSidebarFilter(_:)),
              #selector(showERDiagram(_:)),
-             #selector(previewFKReference(_:)),
-             #selector(selectNumberedTab(_:)):
+             #selector(previewFKReference(_:)):
             return context.isConnected
 
+        /// Each of these moves the selection of a strip, so each needs one on screen, which Agent mode
+        /// does not show, with a tab for the command to reach. Left on `isConnected` alone they
+        /// changed the selected tab behind the conversation, and did nothing at all with one tab or
+        /// for a number past the last tab.
+        case #selector(selectNumberedTab(_:)):
+            guard context.isConnected, !context.isAgentMode, context.editorTabCount > 1 else { return false }
+            guard let number = context.requestedTabNumber else { return true }
+            return number >= 1 && number <= context.editorTabCount
+
         case #selector(goToFirstPage(_:)),
              #selector(goToPreviousPage(_:)),
              #selector(goToNextPage(_:)),
@@ -201,7 +214,7 @@ extension MainSplitViewController: NSMenuItemValidation {
         case #selector(switchConnection(_:)):
             return context.hasSelectedWorkspace
         case #selector(selectNextEditorTab(_:)), #selector(selectPreviousEditorTab(_:)):
-            return context.isConnected
+            return context.isConnected && !context.isAgentMode && context.editorTabCount > 1
         case #selector(switchToRecentTab(_:)), #selector(switchToLeastRecentTab(_:)):
             return (context.isConnected && !context.isAgentMode && context.hasRecentTabToSwitchTo)
                 || context.hasOtherWindowTabs
@@ -593,6 +606,7 @@ extension MainSplitViewController: NSMenuItemValidation {
             canCloseOtherTabs: actions.canCloseOtherTabs,
             canCloseTabsForOtherDatabases: actions.canCloseTabsForOtherDatabases,
             canCloseAllTabs: actions.canCloseAllTabs,
+            editorTabCount: actions.openTabCount,
             hasRecentTabToSwitchTo: hasRecentTabToSwitchTo,
             hasOtherWindowTabs: hasOtherWindowTabs,
             canPinResultTab: actions.canPinResultTab,
@@ -660,7 +674,11 @@ extension MainSplitViewController: NSMenuItemValidation {
     /// what the session commands beside it reported.
     private func menuValidationContext(naming menuItem: NSMenuItem) -> MenuValidationContext {
         var context = menuValidationContext
-        guard let action = menuItem.action, Self.agentSessionSelectors.contains(action) else { return context }
+        guard let action = menuItem.action else { return context }
+        if action == #selector(selectNumberedTab(_:)) {
+            context.requestedTabNumber = menuItem.tag
+        }
+        guard Self.agentSessionSelectors.contains(action) else { return context }
         context.agentSessionTarget = agentSessionTarget(for: menuItem)?.status
         return context
     }

TablePro/Core/Services/Infrastructure/MainSplitViewController.swift

@@ -641,9 +641,19 @@ internal final class MainSplitViewController: NSSplitViewController {
         /// concerned. Only the selected one receives the real `windowDidBecomeKey`, so without
         /// this the outgoing connection keeps `isKeyWindow` true and never schedules the eviction
         /// that frees its row buffers.
+        ///
+        /// The outgoing side is read from each coordinator rather than from `lastActiveCoordinator`,
+        /// which only a switch sets. The window's first connection is made key by the window itself,
+        /// so the first switch away from it found no cached coordinator to resign: it kept
+        /// `isKeyWindow` and never scheduled the eviction.
         let incoming = workspaces.selected?.sessionState?.coordinator
+        for workspace in workspaces.workspaces {
+            guard let coordinator = workspace.sessionState?.coordinator,
+                  coordinator !== incoming,
+                  coordinator.isKeyWindow else { continue }
+            coordinator.handleWindowDidResignKey()
+        }
         if lastActiveCoordinator !== incoming {
-            lastActiveCoordinator?.handleWindowDidResignKey()
             incoming?.handleWindowDidBecomeKey()
             lastActiveCoordinator = incoming
         }

TablePro/Views/Main/MainContentCommandActions.swift

@@ -58,6 +58,11 @@ final class MainContentCommandActions: ObservableObject {
 
     var isTextInputFocusCheckScheduled = false
 
+    /// Asks whether to save a tab being closed. The alert by default, a scripted answer in a test.
+    var confirmSaveChanges: (String, NSWindow?) async -> AlertHelper.SaveConfirmationResult = { message, window in
+        await AlertHelper.confirmSaveChanges(message: message, window: window)
+    }
+
     /// Task handles for async notification observers; cancelled on deinit.
     private var notificationTasks: [Task<Void, Never>] = []
 
@@ -655,20 +660,32 @@ final class MainContentCommandActions: ObservableObject {
         let previousSelection = coordinator.tabManager.selectedTabId
         revealTab(id)
 
-        switch await AlertHelper.confirmSaveChanges(
-            message: String(localized: "Your changes will be lost if you don't save them."),
-            window: closeAnchorWindow
+        switch await confirmSaveChanges(
+            String(localized: "Your changes will be lost if you don't save them."),
+            closeAnchorWindow
         ) {
         case .save:
             guard await saveSelectedTabWork() else { return }
-            coordinator.closeTabsByUser(ids: [id])
+            closeRevealedTab(id, returningTo: previousSelection)
         case .dontSave:
-            coordinator.closeTabsByUser(ids: [id])
+            closeRevealedTab(id, returningTo: previousSelection)
         case .cancel:
+            guard coordinator.tabManager.selectedTabId == id else { return }
             restoreSelection(previousSelection)
         }
     }
 
+    /// The selection goes back only while the closing tab still holds it. A save can wait on the
+    /// server with the strip still live, and a tab the user picked in the meantime is a newer choice
+    /// than the one this close set aside.
+    private func closeRevealedTab(_ id: UUID, returningTo previousSelection: UUID?) {
+        guard let coordinator else { return }
+        let stillShowsClosingTab = coordinator.tabManager.selectedTabId == id
+        coordinator.closeTabsByUser(ids: [id])
+        guard stillShowsClosingTab else { return }
+        restoreSelection(previousSelection)
+    }
+
     /// Shown, then asked. The save and discard machinery reads the selected tab, so the tab being
     /// closed has to be the selected one before the question is put; naming work the user cannot
     /// see would also ask them to decide about something they have no way to look at first.
@@ -677,8 +694,11 @@ final class MainContentCommandActions: ObservableObject {
         coordinator.tabManager.selectedTabId = id
     }
 
-    /// Cancel puts everything back, including a selection that only moved so the sheet had
-    /// somewhere honest to point.
+    /// Every answer puts the selection back where the user had it, unless they have since picked
+    /// another tab, because it only moved so the alert had somewhere honest to point. After a close that is the tab they were working in, not
+    /// the neighbour of the one that went: closing a tab in the background leaves the one in front
+    /// alone whether or not it had anything to save. Closing the tab in front lands on its
+    /// neighbour as before, since the tab it would restore is gone.
     private func restoreSelection(_ id: UUID?) {
         guard let coordinator,
               let id,

TableProTests/Core/Menu/WindowMenuTabCommandsTests.swift

@@ -195,3 +195,54 @@ struct RecentTabMenuValidationTests {
         ) == nil)
     }
 }
+
+/// Show Previous and Next Tab and Select Tab 1 to 9 move a strip's selection. Validated on
+/// `isConnected` alone they stayed lit in Agent mode, where no strip is drawn, and with a tab count
+/// that left them nothing to do.
+@Suite("Tab navigation menu validation")
+struct TabNavigationMenuValidationTests {
+    private func context(tabs: Int, agent: Bool = false, number: Int? = nil) -> MenuValidationContext {
+        var context = MenuValidationContext()
+        context.hasSelectedWorkspace = true
+        context.isConnected = true
+        context.isAgentMode = agent
+        context.editorTabCount = tabs
+        context.requestedTabNumber = number
+        return context
+    }
+
+    private let stepping = [
+        #selector(MainSplitViewController.selectNextEditorTab(_:)),
+        #selector(MainSplitViewController.selectPreviousEditorTab(_:))
+    ]
+    private let numbered = #selector(MainSplitViewController.selectNumberedTab(_:))
+
+    @Test("Stepping needs a second tab and a strip on screen")
+    @MainActor
+    func steppingNeedsTwoTabsInBrowse() {
+        for selector in stepping {
+            #expect(MainSplitViewController.isEnabled(selector, context: context(tabs: 2)))
+            #expect(MainSplitViewController.isEnabled(selector, context: context(tabs: 1)) == false)
+            #expect(MainSplitViewController.isEnabled(selector, context: context(tabs: 3, agent: true)) == false)
+        }
+    }
+
+    @Test("Select Tab N needs an Nth tab and a strip on screen")
+    @MainActor
+    func numberedNeedsThatTab() {
+        #expect(MainSplitViewController.isEnabled(numbered, context: context(tabs: 3, number: 3)))
+        #expect(MainSplitViewController.isEnabled(numbered, context: context(tabs: 3, number: 4)) == false)
+        #expect(MainSplitViewController.isEnabled(numbered, context: context(tabs: 3, agent: true, number: 1)) == false)
+    }
+
+    /// One tab shows no strip, and Select Tab 1 would reselect the tab already in front.
+    @Test("Select Tab 1 dims with a single tab")
+    @MainActor
+    func numberedDimsWithOneTab() throws {
+        let menu = try #require(WindowMenuBuilder.build(keyboard: KeyboardSettings()).submenu)
+        let selectFirst = try #require(menu.items.first { $0.action == numbered && $0.keyEquivalent == "1" })
+
+        #expect(MainSplitViewController.isEnabled(numbered, context: context(tabs: 1, number: selectFirst.tag)) == false)
+        #expect(MainSplitViewController.isEnabled(numbered, context: context(tabs: 2, number: selectFirst.tag)))
+    }
+}

TableProTests/Core/Services/Infrastructure/WorkspaceSwitchKeyStateTests.swift

@@ -0,0 +1,97 @@
+//
+//  WorkspaceSwitchKeyStateTests.swift
+//  TableProTests
+//
+//  Switching connection inside a window is that window's key change as far as each connection's
+//  coordinator is concerned: the one leaving the screen resigns, which is what schedules the
+//  eviction of its row buffers.
+//
+
+import AppKit
+import Foundation
+@testable import TablePro
+import Testing
+
+@Suite("Workspace switch key state", .serialized)
+@MainActor
+struct WorkspaceSwitchKeyStateTests {
+    @MainActor
+    private struct Harness {
+        let controller: MainSplitViewController
+        let first: ConnectionWorkspace
+        let second: ConnectionWorkspace
+        let window: NSWindow
+
+        init() {
+            first = Self.makeWorkspace(name: "First")
+            second = Self.makeWorkspace(name: "Second")
+            controller = MainSplitViewController(payload: nil, sessionState: nil, adopting: first)
+            controller.workspaces.insert(second, select: false)
+            window = NSWindow(
+                contentRect: NSRect(x: 0, y: 0, width: 900, height: 600),
+                styleMask: [.titled],
+                backing: .buffered,
+                defer: false
+            )
+            window.isReleasedWhenClosed = false
+            window.contentViewController = controller
+        }
+
+        func coordinator(_ workspace: ConnectionWorkspace) throws -> MainContentCoordinator {
+            try #require(workspace.sessionState?.coordinator)
+        }
+
+        func tearDown() {
+            window.orderOut(nil)
+            window.contentViewController = nil
+            first.teardown()
+            second.teardown()
+        }
+
+        private static func makeWorkspace(name: String) -> ConnectionWorkspace {
+            let connection = TestFixtures.makeConnection(name: name)
+            return ConnectionWorkspace(
+                connectionId: connection.id,
+                payload: nil,
+                autoConnect: false,
+                payloadConnection: connection,
+                session: nil,
+                sessionState: SessionStateFactory.create(connection: connection, payload: nil),
+                trailingPaneState: nil,
+                phase: .idle
+            )
+        }
+    }
+
+    /// The window makes its first connection key itself, so the controller's cached coordinator was
+    /// still nil at the first switch and the connection leaving the screen was never told.
+    @Test("The first switch away from a window's first connection resigns it")
+    func firstSwitchResignsTheFirstConnection() throws {
+        let harness = Harness()
+        defer { harness.tearDown() }
+        let first = try harness.coordinator(harness.first)
+        let second = try harness.coordinator(harness.second)
+        first.handleWindowDidBecomeKey()
+
+        harness.controller.workspaces.select(harness.second.connectionId)
+
+        #expect(first.isKeyWindow == false)
+        #expect(first.evictionTask != nil)
+        #expect(second.isKeyWindow)
+    }
+
+    @Test("Switching back hands key status back")
+    func switchingBackHandsItBack() throws {
+        let harness = Harness()
+        defer { harness.tearDown() }
+        let first = try harness.coordinator(harness.first)
+        let second = try harness.coordinator(harness.second)
+        first.handleWindowDidBecomeKey()
+
+        harness.controller.workspaces.select(harness.second.connectionId)
+        harness.controller.workspaces.select(harness.first.connectionId)
+
+        #expect(first.isKeyWindow)
+        #expect(second.isKeyWindow == false)
+    }
+}