From b3cfa1456f39a79abf4ce331dd01628124718520 Mon Sep 17 00:00:00 2001 From: Ngo Quoc Dat Date: Sat, 29 Aug 2026 20:42:03 +0700 Subject: [PATCH 1/2] fix(tabs): close the review findings on moving a tab to its own window Claude-Session: https://claude.ai/code/session_01L7uaHbJBPV1LaWL5QXzxyp --- ...nSplitViewController+FileMenuActions.swift | 40 ++++++++++ ...plitViewController+TabStripAccessory.swift | 15 +++- .../MainSplitViewController.swift | 7 ++ .../Infrastructure/WindowHostSelection.swift | 12 ++- .../Infrastructure/WindowManager.swift | 24 +++++- .../Infrastructure/WorkspaceCloseAction.swift | 73 ++++++++++++++++--- .../WindowHostSelectionTests.swift | 46 ++++++++++++ 7 files changed, 199 insertions(+), 18 deletions(-) diff --git a/TablePro/Core/Services/Infrastructure/MainSplitViewController+FileMenuActions.swift b/TablePro/Core/Services/Infrastructure/MainSplitViewController+FileMenuActions.swift index 8e0c59e55..eb5f2de10 100644 --- a/TablePro/Core/Services/Infrastructure/MainSplitViewController+FileMenuActions.swift +++ b/TablePro/Core/Services/Infrastructure/MainSplitViewController+FileMenuActions.swift @@ -52,10 +52,50 @@ extension MainSplitViewController { WindowManager.shared.closeWindow(for: connectionId) return true } + /// A window that exists only to hold a tab moved out of another one closes with that tab. + /// `closeTab` deliberately leaves a window standing when its last tab goes, because the + /// connection is still open in it and its object browser is still useful; that is the right + /// answer for the window a connection lives in and the wrong one here, where Close would + /// empty the window and pressing Close again would take the connection down in both. + /// + /// It still goes through `closeTabAwaiting`, which is the primitive Cmd+W is required to + /// keep. Skipping to `super.performClose` closed the window over a save prompt that was + /// never shown and never reached Recently Closed Tabs. The window closes only once the tab + /// actually went, so Cancel at the prompt leaves both standing. + if isDetachedSingleTabWindow, let selected = workspaces.selected?.sessionState?.tabManager.selectedTab { + Task { @MainActor [weak self] in + await actions.closeTabAwaiting(id: selected.id) + guard let self, + self.workspaces.selected?.sessionState?.tabManager.tabs.isEmpty == true, + let workspace = self.workspaces.selected + else { return } + /// Asked again after the await. The save sheet can stand for as long as the user + /// likes, and the window this tab was moved out of can close underneath it: closing + /// then takes the connection's last window with it, which disconnects the session + /// and skips the whole-window confirmation that close would otherwise raise. + guard WindowManager.shared.workspaces(for: workspace.connectionId).count > 1 else { + return + } + self.view.window?.close() + } + return true + } actions.closeTab() return true } + /// The window built to hold a moved tab, now down to that one tab, with the connection still + /// hosted elsewhere. `hostsDetachedTab` is what separates it from the window it came from, + /// which can be in the identical state and must keep the ordinary last-tab behaviour. + private var isDetachedSingleTabWindow: Bool { + guard hostsDetachedTab, + workspaces.count == 1, + let workspace = workspaces.selected, + workspace.sessionState?.tabManager.tabs.count == 1 + else { return false } + return WindowManager.shared.workspaces(for: workspace.connectionId).count > 1 + } + /// The contextual menu on a rail row offers this too, and the HIG requires every context-menu /// command to be reachable from the menu bar. @objc func closeConnection(_ sender: Any?) { diff --git a/TablePro/Core/Services/Infrastructure/MainSplitViewController+TabStripAccessory.swift b/TablePro/Core/Services/Infrastructure/MainSplitViewController+TabStripAccessory.swift index 87e7961de..cd21db8b3 100644 --- a/TablePro/Core/Services/Infrastructure/MainSplitViewController+TabStripAccessory.swift +++ b/TablePro/Core/Services/Infrastructure/MainSplitViewController+TabStripAccessory.swift @@ -136,8 +136,19 @@ internal extension MainSplitViewController { return EditorTabDetachPolicy.canDetach( tabCount: sessionState.tabManager.tabs.count, hasUnsavedWork: sessionState.coordinator.hasUnsavedWork(forTab: id), - isBusy: sessionState.coordinator.tabExecution.isExecuting(id), - isConnected: DatabaseManager.shared.activeSessions[workspace.connectionId]?.driver != nil + /// `isBusy`, never `isExecuting`. Work that cannot claim the tab still runs on + /// it: Fetch All registers unclaimed work, and an exact row count lives in the + /// tab's own pagination state. Moving a tab in either of those leaves the + /// copied `isLoadingMore` or `isCountingExact` raised in the new window with + /// the completion still owned by the old one, so it never comes down. + isBusy: sessionState.coordinator.tabExecution.isBusy(id) + || sessionState.tabManager.tabs.first { $0.id == id }?.pagination.isBusy == true, + /// `reportedStatus`, never the driver handle. An installed driver is a handle, + /// not a live connection: it stays put through a reconnect and after the health + /// monitor gives up, so reading it moved tabs into windows that could not use + /// them. + isConnected: DatabaseManager.shared.activeSessions[workspace.connectionId]? + .reportedStatus.isConnected == true ) }, /// The resolver's description, not the drawn title: a table tab carries its database diff --git a/TablePro/Core/Services/Infrastructure/MainSplitViewController.swift b/TablePro/Core/Services/Infrastructure/MainSplitViewController.swift index c64a3df79..38aeb35e2 100644 --- a/TablePro/Core/Services/Infrastructure/MainSplitViewController.swift +++ b/TablePro/Core/Services/Infrastructure/MainSplitViewController.swift @@ -101,6 +101,13 @@ internal final class MainSplitViewController: NSSplitViewController, InspectorVi /// leaves one live arm per call behind until the next change wakes them all. These two say /// whether the live arm still watches the right tab manager, so a repeated call is free and /// only a workspace switch or a fired arm registers again. + /// Set on the window built to hold a tab moved out of another one, and never on the window it + /// came from. With exactly two tabs open, tearing one off leaves both windows holding one + /// workspace and one tab with the connection hosted twice, so a test on that state alone is + /// true of the source as well and Cmd+W there closed the window instead of leaving the + /// connection's own window standing. + var hostsDetachedTab = false + var tabStripObservationIsArmed = false var tabStripObservedManager: ObjectIdentifier? diff --git a/TablePro/Core/Services/Infrastructure/WindowHostSelection.swift b/TablePro/Core/Services/Infrastructure/WindowHostSelection.swift index 6f163c168..9a94f4ba8 100644 --- a/TablePro/Core/Services/Infrastructure/WindowHostSelection.swift +++ b/TablePro/Core/Services/Infrastructure/WindowHostSelection.swift @@ -17,8 +17,16 @@ internal enum WindowHostSelection { hostedConnections: [[UUID]], frontmostIndex: Int? ) -> Int? { - if let owning = hostedConnections.firstIndex(where: { $0.contains(connectionId) }) { - return owning + let owning = hostedConnections.indices.filter { hostedConnections[$0].contains(connectionId) } + if owning.count > 1, let frontmostIndex, owning.contains(frontmostIndex) { + /// A tab moved into its own window leaves the connection hosted twice, and the list + /// comes from a dictionary, so "the first one that has it" is arbitrary. A Create Table + /// or an object source opened from the detached window would then append to, and focus, + /// the window it was not invoked from. + return frontmostIndex + } + if let first = owning.first { + return first } guard let frontmostIndex, hostedConnections.indices.contains(frontmostIndex) else { return hostedConnections.isEmpty ? nil : hostedConnections.startIndex diff --git a/TablePro/Core/Services/Infrastructure/WindowManager.swift b/TablePro/Core/Services/Infrastructure/WindowManager.swift index d4dc235e7..12a8564b3 100644 --- a/TablePro/Core/Services/Infrastructure/WindowManager.swift +++ b/TablePro/Core/Services/Infrastructure/WindowManager.swift @@ -89,7 +89,11 @@ internal final class WindowManager { /// Refused for a window's last connection, where it would close the window and open an /// identical one. The rail hides the command in that case rather than dimming it. internal func canMoveToNewWindow(connectionId: UUID) -> Bool { - guard let host = hosts().first(where: { $0.workspaces.contains(connectionId) }) else { return false } + let owning = hosts().filter { $0.workspaces.contains(connectionId) } + /// Withheld while the connection is split across windows by a detached tab. Both this and + /// `moveToNewWindow` resolve the host by connection id alone, so with two of them the + /// command is offered in one rail and acts on the other's workspace. + guard owning.count == 1, let host = owning.first else { return false } return host.workspaces.count > 1 } @@ -143,9 +147,13 @@ internal final class WindowManager { tabTitle: tab.title, intent: .openContent ) + /// Enriched, not raw. A query tab's live caret and selection are held by the coordinator + /// that has it mounted and are written onto the tab only for persistence, so moving the raw + /// value drops the user's position in the editor. + let moved = sourceState.coordinator.enrichedForPersistence(tab) let state = SessionStateFactory.create(connection: connection, payload: nil) - state.tabManager.tabs = [tab] - state.tabManager.selectedTabId = tab.id + state.tabManager.tabs = [moved] + state.tabManager.selectedTabId = moved.id /// The rows the tab already loaded live in its `TabSession`, which belongs to the /// coordinator's registry rather than to the tab. Without handing it over the new window @@ -154,6 +162,14 @@ internal final class WindowManager { if let liveSession = sourceState.coordinator.tabSessionRegistry.session(for: tabId) { state.coordinator.tabSessionRegistry.register(liveSession) } + + /// The destination has to be told a tab arrived. Its coordinator was built with no payload + /// and `selectedTabId` is set before anything observes the manager, so the switch that + /// normally prepares an incoming tab never runs: `toolbarState.isTableTab` stayed false and + /// `changeManager` kept empty table, column and primary-key metadata, which leaves Find and + /// Filter disabled and a later edit unable to name the row it is saving. This is that same + /// preparation, with no outgoing tab to put away. + state.coordinator.handleTabChange(from: nil, to: moved.id, tabs: [moved]) SessionStateFactory.registerPending(state, for: payload.id) guard let window = buildWindow(payload: payload, sessionState: state, autoConnect: false) else { @@ -170,6 +186,8 @@ internal final class WindowManager { sourceState.coordinator.tabSessionRegistry.unregister(id: tabId) sourceState.tabManager.closeTab(id: tabId) + (window.contentViewController as? MainSplitViewController)?.hostsDetachedTab = true + /// A file's window mapping follows its tab, or reopening the file focuses the window the /// tab has left and does nothing there. if let sourceURL = tab.content.sourceFileURL, diff --git a/TablePro/Core/Services/Infrastructure/WorkspaceCloseAction.swift b/TablePro/Core/Services/Infrastructure/WorkspaceCloseAction.swift index 76310067a..195154744 100644 --- a/TablePro/Core/Services/Infrastructure/WorkspaceCloseAction.swift +++ b/TablePro/Core/Services/Infrastructure/WorkspaceCloseAction.swift @@ -83,7 +83,7 @@ internal enum WorkspaceCloseAction { /// nothing has to put all of that back: leaving the user on a connection they did not ask /// for, with the entry still listed, is a close that reads as a switch. let wasShowing = WindowManager.shared.shownConnection(besides: workspace.connectionId) - guard let closable = await confirm(victims, coordinator: coordinator, revealing: workspace) else { + guard let closable = await confirm(victims, across: coordinators, revealing: workspace) else { WindowManager.shared.show(wasShowing, inWindowHosting: workspace.connectionId) Self.logger.info("close cancelled at the save prompt container=\(workspace.container, privacy: .public)") return @@ -162,21 +162,51 @@ internal enum WorkspaceCloseAction { /// selection is a side effect no one asked for when the close has nothing to lose. /// nil when the user cancelled; otherwise the victims that may now be closed, which is every one /// of them unless Save could not reach some. + /// Asked of the window that owns each tab. + /// + /// A tab's live grid and structure edits exist only in its own coordinator: a background + /// snapshot of a tab in another window reads as clean whatever is staged in it. Once a + /// connection can be hosted twice, a container's tabs span both windows, and putting all of + /// them to the coordinator that answered first reported the foreign ones as safe and closed + /// over the edits without a prompt. + /// + /// Cancel anywhere cancels everything, because the entry either closes or it does not. private static func confirm( _ victims: [QueryTab], - coordinator: MainContentCoordinator?, + across coordinators: [MainContentCoordinator], revealing workspace: WorkspaceID ) async -> Set? { let everything = Set(victims.map(\.id)) - guard let actions = coordinator?.commandActions, !victims.isEmpty else { return everything } - guard actions.hasUnsavedWork(among: victims) else { return everything } - reveal(workspace, coordinator: coordinator) - switch await actions.resolveUnsavedWork(in: victims) { - case .cancel: - return nil - case .close(let closable): - return closable + guard !victims.isEmpty else { return everything } + + var closable: Set = [] + for coordinator in coordinators { + let owned = victims.filter { victim in + coordinator.tabManager.tabs.contains { $0.id == victim.id } + } + guard !owned.isEmpty else { continue } + guard let actions = coordinator.commandActions else { + closable.formUnion(owned.map(\.id)) + continue + } + guard actions.hasUnsavedWork(among: owned) else { + closable.formUnion(owned.map(\.id)) + continue + } + reveal(workspace, in: coordinator) + switch await actions.resolveUnsavedWork(in: owned) { + case .cancel: + return nil + case .close(let resolved): + closable.formUnion(resolved) + } } + /// A victim no window claims is already gone, so nothing is holding work for it. + let unclaimed = victims.map(\.id).filter { id in + !coordinators.contains { coordinator in coordinator.tabManager.tabs.contains { $0.id == id } } + } + closable.formUnion(unclaimed) + return closable } /// Leaves the container before it stops being listed, and only when it is the one being browsed. @@ -224,8 +254,29 @@ internal enum WorkspaceCloseAction { } } + /// Brings forward the window that owns the tabs about to be asked about. + /// + /// `resolveUnsavedWork` attaches its sheet to its own coordinator's window, so revealing the + /// connection's first host instead would select one window and then wait on a sheet hanging off + /// another, behind it or on a different native tab. + private static func reveal(_ workspace: WorkspaceID, in coordinator: MainContentCoordinator) { + reveal(workspace, window: coordinator.contentWindow, coordinator: coordinator) + } + private static func reveal(_ workspace: WorkspaceID, coordinator: MainContentCoordinator?) { - if let window = WindowManager.shared.window(for: workspace.connectionId) { + reveal( + workspace, + window: WindowManager.shared.window(for: workspace.connectionId), + coordinator: coordinator + ) + } + + private static func reveal( + _ workspace: WorkspaceID, + window: NSWindow?, + coordinator: MainContentCoordinator? + ) { + if let window { if let group = window.tabGroup, group.selectedWindow !== window { group.selectedWindow = window } diff --git a/TableProTests/Core/Services/Infrastructure/WindowHostSelectionTests.swift b/TableProTests/Core/Services/Infrastructure/WindowHostSelectionTests.swift index 4e347e930..6dd5dd886 100644 --- a/TableProTests/Core/Services/Infrastructure/WindowHostSelectionTests.swift +++ b/TableProTests/Core/Services/Infrastructure/WindowHostSelectionTests.swift @@ -74,4 +74,50 @@ struct WindowHostSelectionTests { #expect(first == second) #expect(first == 1) } + + /// A tab moved into its own window leaves the connection hosted twice, and the candidate list + /// comes from a dictionary, so "the first window that has it" is arbitrary. A payload opened + /// from the detached window would otherwise append to, and focus, the one it was not invoked + /// from. + @Test("The frontmost owner wins when a connection is hosted by more than one window") + func frontmostOwnerWinsWhenSplit() { + let hosted = [[Self.alpha], [Self.alpha, Self.beta]] + + #expect( + WindowHostSelection.hostIndex( + forConnection: Self.alpha, hostedConnections: hosted, frontmostIndex: 1 + ) == 1 + ) + #expect( + WindowHostSelection.hostIndex( + forConnection: Self.alpha, hostedConnections: hosted, frontmostIndex: 0 + ) == 0 + ) + } + + /// Only when the frontmost window is one of the owners. A connection split across two windows + /// while a third is in front still lands in an owner rather than opening somewhere new. + @Test("A frontmost window that does not host the connection does not take it") + func frontmostNonOwnerIsIgnored() { + let hosted = [[Self.alpha], [Self.alpha], [Self.beta]] + + #expect( + WindowHostSelection.hostIndex( + forConnection: Self.alpha, hostedConnections: hosted, frontmostIndex: 2 + ) == 0 + ) + } + + /// One owner keeps the old answer whatever is in front, which is what every existing case here + /// relies on. + @Test("A single owner still wins over the frontmost window") + func singleOwnerIsUnaffected() { + let hosted = [[Self.alpha], [Self.beta]] + + #expect( + WindowHostSelection.hostIndex( + forConnection: Self.alpha, hostedConnections: hosted, frontmostIndex: 1 + ) == 0 + ) + } } From b8983b55473764e5cc3c7f1a8bbe853dd928dff2 Mon Sep 17 00:00:00 2001 From: Ngo Quoc Dat Date: Sat, 29 Aug 2026 20:55:23 +0700 Subject: [PATCH 2/2] fix(tabs): raise the window save prompt when a detached window closes Claude-Session: https://claude.ai/code/session_01L7uaHbJBPV1LaWL5QXzxyp --- .../MainSplitViewController+FileMenuActions.swift | 7 ++++++- .../Infrastructure/WorkspaceCloseAction.swift | 14 ++++++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/TablePro/Core/Services/Infrastructure/MainSplitViewController+FileMenuActions.swift b/TablePro/Core/Services/Infrastructure/MainSplitViewController+FileMenuActions.swift index eb5f2de10..a2d5372e5 100644 --- a/TablePro/Core/Services/Infrastructure/MainSplitViewController+FileMenuActions.swift +++ b/TablePro/Core/Services/Infrastructure/MainSplitViewController+FileMenuActions.swift @@ -76,7 +76,12 @@ extension MainSplitViewController { guard WindowManager.shared.workspaces(for: workspace.connectionId).count > 1 else { return } - self.view.window?.close() + /// `closeWindowAwaiting`, not a raw `close()`. An inspector edit is connection + /// scoped, so `closeTabAwaiting` deliberately does not ask about it; closing the + /// window directly then tore down the per-window `RightPanelState` and dropped it + /// with no prompt. With the tab already gone this window has none left, which is + /// the branch that closes it. + await actions.closeWindowAwaiting() } return true } diff --git a/TablePro/Core/Services/Infrastructure/WorkspaceCloseAction.swift b/TablePro/Core/Services/Infrastructure/WorkspaceCloseAction.swift index 195154744..911006710 100644 --- a/TablePro/Core/Services/Infrastructure/WorkspaceCloseAction.swift +++ b/TablePro/Core/Services/Infrastructure/WorkspaceCloseAction.swift @@ -180,6 +180,16 @@ internal enum WorkspaceCloseAction { guard !victims.isEmpty else { return everything } var closable: Set = [] + /// What each window was showing before it was brought forward to ask. A dirty split + /// connection reveals more than one of them, and the caller can only put one back, so an + /// abandoned close left the others switched to a connection the user never chose. + var revealed: [(host: MainSplitViewController, shown: UUID?)] = [] + func restoreRevealed() { + for entry in revealed { + entry.host.workspaces.select(entry.shown) + } + } + for coordinator in coordinators { let owned = victims.filter { victim in coordinator.tabManager.tabs.contains { $0.id == victim.id } @@ -193,9 +203,13 @@ internal enum WorkspaceCloseAction { closable.formUnion(owned.map(\.id)) continue } + if let host = coordinator.splitViewController { + revealed.append((host, host.workspaces.selectedConnectionId)) + } reveal(workspace, in: coordinator) switch await actions.resolveUnsavedWork(in: owned) { case .cancel: + restoreRevealed() return nil case .close(let resolved): closable.formUnion(resolved)