diff --git a/CHANGELOG.md b/CHANGELOG.md index a066b166d..457f68837 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -69,6 +69,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Editor tab strip tests reporting four appearances while running plain Aqua and Dark Aqua twice. - Icon cut off the entry at the top of a scrolled connections strip. (#2452) - Connections strip not scrolling to the entry you switch to. +- Background connections stuck on the session preparation screen after connecting. (#2545) +- Blank sidebar, grid and inspector on a connection opened into a window that was already on screen. - Grid cells left at the old column positions until the next click, after a resize, an auto-fit, a reorder, hiding a column, or a row-number width change. (#2449, #2446) - The row-number column draggable out of first place, which walked it to the far right on the next refresh. - A time entered into a date cell discarded when the stored value carried no time. diff --git a/CLAUDE.md b/CLAUDE.md index f0fb9ca2d..36c553d97 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -195,6 +195,8 @@ To ship one: add the record type or field in CloudKit Console (or `xcrun cktool **A workspace's content is a function of its own `ConnectionWindowPhase`, never of `activeSessions` membership**: the global session dictionary can only say *present* or *absent*, and that vocabulary cannot tell "never started" from "connecting" from "failed" from "the user cancelled" from "the window is closing". Deriving the pane from it shipped a window that painted a live spinner forever after a failed launch restore, could not be repainted by a later successful connect, and left no route back to the connection list except the Dock icon's context menu. `ConnectionWorkspace` owns the `phase`, one per connection the window hosts; `ConnectionWindowPhaseMachine` owns the transitions (pure, exhaustive, `.closing` absorbing), and `ConnectionWindowPaneResolver` owns the pane choice (pure). `MainSplitViewController` renders the selected workspace and routes a transition by `connectionId` through `transition(to:for:)`; it is only an adapter, and its `phase` property is a pass-through to `workspaces.selected`. Three rules follow. First, every phase must have an exit: the old `closingSessionId` latch was set once and never cleared, so the controller went permanently deaf to `connectionStatusChanged`. Second, a cancel updates the UI synchronously with the button press and never waits on the driver, because `Task.cancel()` is cooperative and may have no observable effect; the attempt is fenced by a per-workspace `attemptToken` (`ConnectionWorkspace.attemptToken`) plus `DatabaseManager.invalidateConnectionAttempt`, so a late failure cannot write into a workspace that moved on. The token cannot live on the window, because the window did not move on: one of the connections it hosts did. Closing a window therefore cancels the in-flight attempt of every workspace it hosts, not just the one its original payload named, and a completion that finds its workspace gone discards itself rather than resurrecting it. Third, a failure is presented inline through `ConnectionUnavailableView`, never as an alert, per the HIG's rule against alerts at startup and its one-alert-at-a-time rule (N restored connections would mean N modals). Only one presenter per failure: `LaunchIntentRouter.presentError` stays silent when a window for that connection exists. +**A workspace's panes are rendered from its final phase, and every workspace's, not just the one on screen**: each connection owns four `NSHostingController`s that outlive every switch, so a pane built from a phase that is about to change stays wrong until something builds it again, and nothing did. `reconcileStatus` used to adopt the session, paint, and only then write the phase, while `transition(to:for:)` repainted the selected workspace alone. So a connection that finished connecting while the user was looking at another one kept its "Preparing the session" screen and its Cancel button for good, over a session the rail and the toolbar both reported as connected (#2545); a background connect that failed, a background session that dropped and a background reconnect were each wrong the same way. Two rules follow. Nothing paints before the phase is final: `transition(to:for:)` is the only phase writer outside `markWindowClosing`'s batch, and it ends in `syncPanes(of:)` for the workspace it names, selected or not, and on an unchanged phase too, because a session can be replaced under one (a database switch does). And the panes carry a `WorkspacePaneRenderKey` of what they were last built from, which `refreshPanes` alone writes: that is what makes a repaint free to ask for from anywhere, including on every workspace switch, where an unconditional rebuild would re-evaluate the whole content tree and give back the work `WorkspacePanes` exists to avoid. The key is why selection can repair a workspace nothing else reached, which is what `adoptWorkspace` needs: it hands over a connection with a live session and no phase change to follow, so its panes still held the empty view they were constructed with and the connection opened blank. + **The app runs the AppKit lifecycle, and AppKit owns the menu bar**: `main.swift` assigns the delegate before `NSApplicationMain`, and `MainMenuBuilder.install` runs in `applicationWillFinishLaunching`. Do not reintroduce a SwiftUI `App`. SwiftUI reconciles `NSApp.mainMenu` once shortly after launch and removes every item it did not build itself, and no hook can undo it: `NSApp.mainMenu` is not KVO-compliant, `didUpdateNotification`, `didBecomeKeyNotification` and the `applicationDidUpdate(_:)` delegate method never fire under `@NSApplicationDelegateAdaptor`, and `applicationDidBecomeActive` fires before the reconciliation. Only a wall-clock delay worked, which is why #2057 shipped a menu bar that vanished half a second after launch and had to be reverted (#2071). Every window is an `NSWindowController`; the Welcome window is one too, so closing it is an ordinary `close()` and the old "closed, never ordered out" rule no longer applies. **An emptied tab manager is not the same as "the user closed every tab"**: a coordinator torn down by a lost session has already emptied `tabManager.tabs`, so any persistence path that reads "no tabs" as "clear the saved tabs" wipes tabs the user never closed. The fix is that the teardown path cannot clear at all: `TabPersistenceCoordinator.saveAggregatedSync()`, which disconnect and window-close call, opens with `guard !tabs.isEmpty else { return }`. Clearing requires explicit consent and happens on the `closeTabsByUser` path instead. Keep those two paths separate; the moment a teardown path can write an empty aggregate, the bug is back. @@ -205,7 +207,7 @@ To ship one: add the record type or field in CloudKit Console (or `xcrun cktool **A SwiftUI-hosted split view needs an explicit divider cursor**: `NSSplitView` shows the resize cursor over its dividers through AppKit's cursor-rects system, which does not fire once the split view is mounted inside an `NSHostingController` (every tab-content split is, several SwiftUI layers deep under `MainSplitViewController.detailHosting`). The divider still drags because drag hit-testing is independent of cursor rects, but the pointer never changes. Every SwiftUI-hosted split-view controller must subclass `ResizeCursorSplitViewController`, which adds a key-window tracking area to its own split view and sets `NSCursor.columnResize`/`rowResize` (falling back to `resizeLeftRight`/`resizeUpDown` before macOS 15) in `mouseMoved`, the same hand-rolled approach `SortableHeaderView` uses for column resize. It attaches the tracking area to the framework's split view in `viewDidLoad` rather than replacing the split view, so `NSSplitViewController`'s own layout and divider orientation stay intact; replacing the split view through a `loadView` override that skips `super` leaves the controller half-initialized and its panes stack instead of laying out side by side. Do not swap the controller back to a plain `NSSplitViewController` expecting the stock cursor to work; the window's own sidebar and inspector dividers only get the cursor for free because `MainSplitViewController` is the window's `contentViewController` directly, with no SwiftUI host in between. This shipped as Users & Roles, Structure, Server Dashboard, and query editor dividers that dragged but never showed the resize cursor (#1905). -**Appearance is not lifetime, so `onDisappear` is never a destructor**: switching connection unparents the outgoing connection's panes (`WorkspacePaneHost.show`) while `WorkspacePanes` keeps the hosting controllers alive, and SwiftUI reports that as `onDisappear` followed by `onAppear` again on the same view identity with the same `@State`. Measured: `removeFromSuperview` fires the pair, `isHidden` and `window.orderOut` fire neither, and nothing fires on window close. Everything released under a pane's `onDisappear` must therefore be rebuilt by its `onAppear`, and anything that cannot be rebuilt must not be released there. `SQLEditorView` released the editor's text storage, highlighter, tree-sitter client, text coordinators and local key monitor from `onDisappear`, and `TextViewController` installs the last three in `loadView` alone, so one connection switch left a blank editor with no highlighting, no undo, no Cmd+/ or Cmd+[ and no Vim mode, permanently (#2236). Terminal teardown belongs to `NSViewControllerRepresentable.dismantleNSViewController`, which fires only on identity destruction, or to the explicit `ConnectionWorkspace.teardown()` chain. One catch there: SwiftUI reconciles a hosting controller on a layout pass and nothing lays out a detached view, so a `rootView` write on a pane that may be unparented needs `layoutSubtreeIfNeeded()` to take effect at all. `WorkspacePanes.teardown()` is the reference shape; `MainSplitViewController.refreshPanes` still writes `rootView` without one. +**Appearance is not lifetime, so `onDisappear` is never a destructor**: switching connection unparents the outgoing connection's panes (`WorkspacePaneHost.show`) while `WorkspacePanes` keeps the hosting controllers alive, and SwiftUI reports that as `onDisappear` followed by `onAppear` again on the same view identity with the same `@State`. Measured: `removeFromSuperview` fires the pair, `isHidden` and `window.orderOut` fire neither, and nothing fires on window close. Everything released under a pane's `onDisappear` must therefore be rebuilt by its `onAppear`, and anything that cannot be rebuilt must not be released there. `SQLEditorView` released the editor's text storage, highlighter, tree-sitter client, text coordinators and local key monitor from `onDisappear`, and `TextViewController` installs the last three in `loadView` alone, so one connection switch left a blank editor with no highlighting, no undo, no Cmd+/ or Cmd+[ and no Vim mode, permanently (#2236). Terminal teardown belongs to `NSViewControllerRepresentable.dismantleNSViewController`, which fires only on identity destruction, or to the explicit `ConnectionWorkspace.teardown()` chain. One catch there: SwiftUI reconciles a hosting controller on a layout pass and nothing lays out a detached view, so a teardown that has to happen *now* needs `layoutSubtreeIfNeeded()` to force one. That is `WorkspacePanes.teardown()`, whose panes are never parented again. It is not a general rule about writing `rootView` while detached: measured, such a write is deferred rather than lost, the last value written is what mounts when the pane is put back, and `@State`, grid scroll, selection and undo all survive it. `MainSplitViewController.refreshPanes` therefore writes `rootView` without one deliberately. Adding one there would mount every intermediate value instead of letting one run-loop turn settle on the final one, which is exactly the coalescing a phase change and a session adoption in the same turn rely on. **The data grid header owns all of its own chrome, so nothing may ask AppKit to paint any of it**: `NSTableHeaderCell` and `NSTableHeaderView` both paint a fixed 28pt band that they centre vertically in whatever frame they are given, a 16pt column divider on `midY` and a 1pt rule at `midY + 13`. The data grid grows its header to 42pt for a column comment, so that band lands mid-cell: the rule crosses the comment's descenders and sits 8pt above the real bottom edge. `SortableHeaderChrome` is therefore the single owner of header geometry and colours, `SortableHeaderCell.draw(withFrame:in:)` never calls `super`, and `SortableHeaderView.draw(_:)` fills the background and rules the bottom edge itself. The trap is that the header view paints a second copy of that same band for `NSTableView.highlightedTableColumn`, driven by *state* rather than by a drawing call, so no cell override can reach it: setting it gives the sorted column a stray divider and a rule no other column has. TablePro already draws the sorted-column affordance itself (bold title, chevron, priority number, with `drawSortIndicator` overridden to nothing), so `highlightedTableColumn` is a redundant second channel and must stay unset. All sorted-column presentation goes through `SortableHeaderView.applySortState(_:schema:)`, which publishes the order natively through `tableView.sortDescriptors` (for accessibility; it paints nothing) and updates the cells. `SortableHeaderRenderingTests` rasterises the header and guards this. This shipped as a rule through the comment line and a stray divider on the sorted column (#2017). diff --git a/TablePro/Core/Services/Infrastructure/ConnectionWorkspace.swift b/TablePro/Core/Services/Infrastructure/ConnectionWorkspace.swift index 5100e16bf..1d9de66b7 100644 --- a/TablePro/Core/Services/Infrastructure/ConnectionWorkspace.swift +++ b/TablePro/Core/Services/Infrastructure/ConnectionWorkspace.swift @@ -22,8 +22,18 @@ internal final class ConnectionWorkspace { /// lands on has to be recorded like any other: it is the entry the user leaves behind when they /// switch, and an entry nobody recorded is one that disappears the moment they do. internal var session: ConnectionSession? { - didSet { recordBrowsedContainer() } + didSet { + sessionRevision &+= 1 + recordBrowsedContainer() + } } + + /// Bumped whenever the adopted session is replaced. `ConnectionSession` is a value, so a + /// database switch or a status change produces a different one under an unchanged phase, and + /// the panes have to be rebuilt for it. This is how the render key says so without holding the + /// session, and through it the driver, itself. + internal private(set) var sessionRevision = 0 + internal var sessionState: SessionStateFactory.SessionState? internal var rightPanelState: RightPanelState? internal var attemptToken: UUID? @@ -189,6 +199,26 @@ internal final class ConnectionWorkspace { payloadConnection ?? session?.connection } + /// The pane this workspace resolves to right now. `ConnectionWindowPaneResolver` decides it and + /// `MainSplitViewController` renders it; the workspace only supplies the state both read. + internal var resolvedPane: ConnectionWindowPane { + ConnectionWindowPaneResolver.pane( + phase: phase, + hasConnection: connection != nil, + hasRenderableSession: session != nil && rightPanelState != nil && sessionState != nil + ) + } + + /// Everything the panes are built from, compared against `panes.renderedKey` to decide whether + /// they have to be built at all. + internal var paneRenderKey: WorkspacePaneRenderKey { + WorkspacePaneRenderKey( + pane: resolvedPane, + connection: connection, + sessionRevision: sessionRevision + ) + } + /// Opens what the payload names. Held until the session exists if the connection is still /// being established. internal func open(_ payload: EditorTabPayload) { diff --git a/TablePro/Core/Services/Infrastructure/MainSplitViewController.swift b/TablePro/Core/Services/Infrastructure/MainSplitViewController.swift index 2aa542a8f..3627cc5b5 100644 --- a/TablePro/Core/Services/Infrastructure/MainSplitViewController.swift +++ b/TablePro/Core/Services/Infrastructure/MainSplitViewController.swift @@ -57,9 +57,8 @@ internal final class MainSplitViewController: NSSplitViewController, InspectorVi var phase: ConnectionWindowPhase { get { workspaces.selected?.phase ?? .idle } set { - guard let selected = workspaces.selected, selected.phase != newValue else { return } - selected.phase = newValue - applyPhase() + guard let connectionId = workspaces.selectedConnectionId else { return } + transition(to: newValue, for: connectionId) } } @@ -153,6 +152,10 @@ internal final class MainSplitViewController: NSSplitViewController, InspectorVi super.init(nibName: nil, bundle: nil) if let workspace { + /// It arrives with its panes already built, by the controller it is leaving. Every + /// closure in them calls back into that one, so they have to be produced again here + /// even though nothing about the connection has changed. + workspace.panes.invalidate() workspaces.insert(workspace) } else { adoptWorkspace(payload: payload, autoConnect: autoConnect) @@ -294,7 +297,7 @@ internal final class MainSplitViewController: NSSplitViewController, InspectorVi } restoreUserPaneLayout() - refreshSelectedPanes() + syncSelectedPanes() showSelectedPanes() applyPaneChrome() } @@ -382,6 +385,11 @@ internal final class MainSplitViewController: NSSplitViewController, InspectorVi ?? DatabaseManager.shared.activeSessions[connectionId]?.connection else { continue } workspace.payloadConnection = record workspace.sessionState?.toolbarState.update(from: record) + /// The one repaint the render key cannot decide, so the only one that skips it. Deleting + /// a connection whose session is still open purges the per-connection registries the + /// panes hold without changing the record they were built from, and a pane left holding + /// the purged `SharedSidebarState` stops seeing what the rest of the window does to the + /// newly registered one. refreshPanes(of: workspace) if workspaces.selectedConnectionId == connectionId { repaint = true } } @@ -441,17 +449,22 @@ internal final class MainSplitViewController: NSSplitViewController, InspectorVi session: snapshot, ownsAttempt: workspace.attemptToken != nil ) + /// Nothing here paints. The panes are built from the phase, so they can only be built once + /// the phase is final, and `transition(to:for:)` is where that happens. Adopting a session + /// bumps the workspace's session revision, which is what tells the sync that a session + /// whose phase did not move still has to be redrawn. + /// + /// `isContentViewEquivalent` answers whether the views would draw the same, not whether it + /// is the same session: it excludes the driver, the effective connection and the cached + /// password on purpose. A connection arriving back at `.connected` therefore adopts whatever + /// the manager now holds even when the two would draw alike, or a tunnel recovery would + /// leave the workspace holding the driver it just disconnected, and that driver's cached + /// credentials, until something else replaced the session. if nextPhase == .connected, let session { - let alreadyRendered = workspace.session?.isContentViewEquivalent(to: session) ?? false - if alreadyRendered, workspace.phase == nextPhase { return } - adoptSession(session, into: workspace) - /// Repainted whether or not this workspace is the one on screen, and whether or not it - /// already had this phase. The equality guard can never hold on the first `.connecting` - /// to `.connected` step, and `transition(to:for:)` repaints only the selected workspace, - /// so a connection that finished connecting while the user was looking at another one - /// kept whatever placeholder it last rendered: its `MainContentView` never mounted, its - /// `commandActions` stayed nil, and every command aimed at it did nothing. - refreshPanes(of: workspace) + let drawsTheSame = workspace.session?.isContentViewEquivalent(to: session) ?? false + if !drawsTheSame || workspace.phase != nextPhase { + adoptSession(session, into: workspace) + } } else if workspace.phase == .connected, nextPhase != .connected, !snapshot.exists { releaseSession(workspace) } @@ -502,8 +515,9 @@ internal final class MainSplitViewController: NSSplitViewController, InspectorVi } /// The panes are rebuilt rather than dropped: the workspace stays in the registry so it can /// render its own phase, and its content view is now the not-connected pane. Leaving the - /// old tree mounted would keep the torn-down coordinator alive through it. - refreshPanes(of: workspace) + /// old tree mounted would keep the torn-down coordinator alive through it. Clearing the + /// session above bumped the revision, so the sync at the end of the transition this is part + /// of is what does it, after the phase has stopped saying `.connected`. if isShowing(workspace) { navigationSidebar.objectBrowser.updateSidebarState(nil) } @@ -539,6 +553,11 @@ internal final class MainSplitViewController: NSSplitViewController, InspectorVi toolbarOwner?.repoint(to: nil) } + /// A workspace can reach the window already built and never repainted: `adoptWorkspace` + /// hands one over with a live session and no phase change to follow, so its panes still + /// held the empty view they were constructed with and the connection opened blank. This + /// costs a key comparison when nothing has moved, which is what the record is for. + syncSelectedPanes() showSelectedPanes() applyDetailMinimumThicknessForSelection() applyPaneChrome() @@ -550,7 +569,7 @@ internal final class MainSplitViewController: NSSplitViewController, InspectorVi } private func applyPhase() { - refreshSelectedPanes() + syncSelectedPanes() applyPaneChrome() applyWindowTitle() SessionRecoveryTracker.sync() @@ -575,18 +594,24 @@ internal final class MainSplitViewController: NSSplitViewController, InspectorVi phase = next } - /// A window now hosts several connections, so a phase change has to name the one it belongs - /// to. Repainting is skipped for a workspace the user is not looking at: its state is still - /// correct, it simply is not the thing on screen. + /// A window now hosts several connections, so a phase change has to name the one it belongs to, + /// and this is the only place a phase is written outside a window close. + /// + /// The sync runs whether or not this workspace is the one on screen, because a background one + /// owns persistent panes that would otherwise keep the phase they last rendered (#2545), and + /// whether or not the phase moved, because a session can be replaced under an unchanged one as + /// a database switch does. Only the window's own chrome is gated on the phase moving. internal func transition(to next: ConnectionWindowPhase, for connectionId: UUID) { guard let workspace = workspaces.workspace(for: connectionId) else { return } - guard workspace.phase != next else { return } + let phaseChanged = workspace.phase != next workspace.phase = next + syncPanes(of: workspace) + guard phaseChanged else { return } if workspaces.selectedConnectionId == connectionId { - applyPhase() - } else { - SessionRecoveryTracker.sync() + applyPaneChrome() + applyWindowTitle() } + SessionRecoveryTracker.sync() } internal func refreshFromActiveSessions() { @@ -614,19 +639,37 @@ internal final class MainSplitViewController: NSSplitViewController, InspectorVi // MARK: - Pane Construction /// Rebuilds one connection's panes into its own hosting controllers, whether or not it is the - /// one on screen. This is the only place a pane's content is produced. + /// one on screen, and records what they were built from. This is the only place all four panes + /// are produced, and the only writer of the record; `rebuildInspectorPane()` refines the + /// inspector alone once `commandActions` exists, which is a redraw of the same key rather than + /// a different one. + /// + /// Reaching a pane that is not on screen is safe and deliberate: a `rootView` write on an + /// unparented hosting controller is deferred rather than lost, and the last value written is + /// what mounts when the pane is put back. Do not force it with `layoutSubtreeIfNeeded()`, which + /// `teardown()` needs only because its panes are never parented again: here it would mount every + /// intermediate value instead of letting one run-loop turn settle on the final one. private func refreshPanes(of workspace: ConnectionWorkspace) { workspace.panes.sidebar.rootView = AnyView(buildSidebarView(for: workspace)) workspace.panes.detail.rootView = AnyView(buildDetailView(for: workspace)) workspace.panes.inspector.rootView = AnyView(buildInspectorView(for: workspace)) refreshTabStripPane(of: workspace) + workspace.panes.markRendered(workspace.paneRenderKey) guard isShowing(workspace) else { return } bindSidebarChrome(to: workspace) } - private func refreshSelectedPanes() { + /// The single entry point for a repaint, so a caller never has to know whether one is due. The + /// record is what makes it free when nothing has moved, which is what lets a workspace switch + /// ask for one without paying for the rebuild the panes exist to avoid. + private func syncPanes(of workspace: ConnectionWorkspace) { + guard workspace.paneRenderKey != workspace.panes.renderedKey else { return } + refreshPanes(of: workspace) + } + + private func syncSelectedPanes() { guard let selected = workspaces.selected else { return } - refreshPanes(of: selected) + syncPanes(of: selected) } /// Puts the selected connection's already-built panes on screen. This is the whole cost of a @@ -658,20 +701,7 @@ internal final class MainSplitViewController: NSSplitViewController, InspectorVi } var currentPane: ConnectionWindowPane { - workspaces.selected.map(Self.pane) ?? .unavailable(.notConnected) - } - - /// The pane a given workspace shows, which is not always the one on screen. Each workspace owns - /// its panes now, so a background connection that fails or reconnects repaints itself instead of - /// waiting for the user to switch to it and the switch to rebuild everything. - private static func pane(of workspace: ConnectionWorkspace) -> ConnectionWindowPane { - ConnectionWindowPaneResolver.pane( - phase: workspace.phase, - hasConnection: workspace.connection != nil, - hasRenderableSession: workspace.session != nil - && workspace.rightPanelState != nil - && workspace.sessionState != nil - ) + workspaces.selected?.resolvedPane ?? .unavailable(.notConnected) } /// The one answer to "does this window have a database to talk to". `releaseSession` keeps the @@ -702,7 +732,7 @@ internal final class MainSplitViewController: NSSplitViewController, InspectorVi /// so the tree is per-connection by construction and an identity would only throw it away. @ViewBuilder private func buildSidebarView(for workspace: ConnectionWorkspace) -> some View { - if Self.pane(of: workspace) == .content, + if workspace.resolvedPane == .content, let session = workspace.session, let sessionState = workspace.sessionState { SidebarView( @@ -723,7 +753,7 @@ internal final class MainSplitViewController: NSSplitViewController, InspectorVi @ViewBuilder private func buildDetailView(for workspace: ConnectionWorkspace) -> some View { - let pane = Self.pane(of: workspace) + let pane = workspace.resolvedPane if pane == .connecting, let pendingConnection = workspace.connection { ConnectingStateView(connection: pendingConnection) { [weak self] in self?.cancelConnectionAttempt(for: workspace.connectionId) diff --git a/TablePro/Core/Services/Infrastructure/WorkspacePanes.swift b/TablePro/Core/Services/Infrastructure/WorkspacePanes.swift index 2475cd6f2..442c75762 100644 --- a/TablePro/Core/Services/Infrastructure/WorkspacePanes.swift +++ b/TablePro/Core/Services/Infrastructure/WorkspacePanes.swift @@ -6,13 +6,32 @@ import AppKit import SwiftUI +/// What a workspace's cached panes were last built from. +/// +/// The panes are a rendering of the workspace's resolved pane, and a rendering is only correct for +/// the state it was made from. Recording that state is what lets a repaint be asked for from +/// anywhere and cost nothing when nothing has moved, which is what makes it safe to ask on every +/// workspace switch. Repainting unconditionally there would re-evaluate the whole content tree on +/// each switch and give back exactly the work `WorkspacePanes` exists to avoid. +/// +/// It carries the connection record because a rename changes what the panes draw without changing +/// which pane they draw, and a session revision rather than the session itself because a session +/// holds the driver: keeping one here would hold a released driver alive for as long as the record. +internal struct WorkspacePaneRenderKey: Equatable { + internal let pane: ConnectionWindowPane + internal let connection: DatabaseConnection? + internal let sessionRevision: Int +} + /// One connection's three panes, kept alive for as long as the window hosts that connection. /// /// The window used to own one hosting controller per pane and reassign its `rootView` on every -/// workspace switch. `AnyView` erases identity, so SwiftUI could not diff: it tore down and rebuilt -/// the whole data grid, editor and object tree each time. Measured at 120ms to 190ms of layout per -/// click, none of it visible to a timer around the switch itself because assigning `rootView` -/// returns immediately and books the work for the next layout pass. +/// workspace switch, which tore down and rebuilt the whole data grid, editor and object tree each +/// time. Measured at 120ms to 190ms of layout per click, none of it visible to a timer around the +/// switch itself because assigning `rootView` returns immediately and books the work for the next +/// layout pass. The cost was the pane changing under one controller, not `AnyView`: every builder +/// here erases a single `_ConditionalContent`, so the wrapped type is stable and a rewrite that +/// keeps the same branch is measurably free. /// /// Rebuilding also destroyed everything those views own that no model holds: grid scroll position, /// rectangular cell selection, the editor's find panel and undo stack, and an unsaved Create Table @@ -32,6 +51,10 @@ internal final class WorkspacePanes { /// same teardown as everything else the connection owns. internal let tabStrip: NSHostingController + /// Written by the one function that produces pane content, and read by the one that decides + /// whether it has to run. `nil` means the panes hold nothing anybody has vouched for. + internal private(set) var renderedKey: WorkspacePaneRenderKey? + internal init() { detail = NSHostingController(rootView: AnyView(Color.clear)) inspector = NSHostingController(rootView: AnyView(Color.clear)) @@ -46,6 +69,17 @@ internal final class WorkspacePanes { [detail, inspector, sidebar, tabStrip] } + internal func markRendered(_ key: WorkspacePaneRenderKey) { + renderedKey = key + } + + /// Drops the record without touching the views. A workspace handed to another window keeps the + /// panes it already built, and every closure inside them calls back into the controller that + /// built them, which is no longer the one hosting it. + internal func invalidate() { + renderedKey = nil + } + /// Empties every pane and unparents it. A hosting controller retains its SwiftUI tree, which /// retains the `MainContentCoordinator`, which only leaves the app-wide coordinator registry /// when it deinits: a pane left behind keeps a dead session answering questions about open tabs @@ -60,6 +94,7 @@ internal final class WorkspacePanes { /// tears down a pane that is still on screen. A pane that was never parented has nothing /// mounted to drop, and the cleared `rootView` is enough for it. internal func teardown() { + renderedKey = nil for pane in panes { pane.rootView = AnyView(Color.clear) pane.view.layoutSubtreeIfNeeded() diff --git a/TableProTests/Core/Services/Infrastructure/MainSplitViewControllerPaneSynchronizationTests.swift b/TableProTests/Core/Services/Infrastructure/MainSplitViewControllerPaneSynchronizationTests.swift new file mode 100644 index 000000000..9c816835e --- /dev/null +++ b/TableProTests/Core/Services/Infrastructure/MainSplitViewControllerPaneSynchronizationTests.swift @@ -0,0 +1,209 @@ +import AppKit +import Foundation +@testable import TablePro +import Testing + +@Suite("Main split view background pane synchronization", .serialized) +@MainActor +struct MainSplitViewControllerPaneSynchronizationTests { + @Test("A connection completed in the background mounts content when selected") + func backgroundConnectionCompletionMountsContentWhenSelected() throws { + let harness = try Harness() + defer { harness.tearDown() } + + harness.injectSession(status: .connecting, driver: false) + harness.controller.refreshFromActiveSessions() + harness.injectSession(status: .connected, driver: true) + harness.controller.refreshFromActiveSessions() + + let coordinator = try #require(harness.background.sessionState?.coordinator) + #expect(harness.controller.workspaces.selectedConnectionId == harness.foreground.connectionId) + #expect(harness.background.phase == .connected) + #expect(harness.background.panes.renderedKey?.pane == .content) + #expect(!coordinator.isActivated) + #expect(coordinator.commandActions == nil) + + harness.controller.workspaces.select(harness.background.connectionId) + harness.settle { coordinator.isActivated } + + #expect(harness.controller.currentPane == .content) + #expect(coordinator.isActivated) + #expect(coordinator.commandActions != nil) + } + + @Test("A connect that fails in the background renders the unavailable pane") + func backgroundConnectFailureRendersUnavailablePane() throws { + let harness = try Harness() + defer { harness.tearDown() } + + harness.controller.transition(to: .connecting, for: harness.background.connectionId) + #expect(harness.background.panes.renderedKey?.pane == .connecting) + + let failure = ConnectionUnavailableReason.failed(ConnectionFailureInfo(message: "refused")) + harness.controller.transition(to: .unavailable(failure), for: harness.background.connectionId) + + #expect(harness.background.phase == .unavailable(failure)) + #expect(harness.background.panes.renderedKey?.pane == .unavailable(failure)) + } + + @Test("A session lost in the background renders the disconnected pane, never the empty one") + func backgroundSessionLossRendersDisconnectedPane() throws { + let harness = try Harness() + defer { harness.tearDown() } + + harness.injectSession(status: .connected, driver: true) + harness.controller.refreshFromActiveSessions() + #expect(harness.background.panes.renderedKey?.pane == .content) + + DatabaseManager.shared.removeSession(for: harness.background.connectionId) + harness.controller.refreshFromActiveSessions() + + #expect(harness.background.phase == .unavailable(.disconnected(nil))) + #expect(harness.background.panes.renderedKey?.pane == .unavailable(.disconnected(nil))) + } + + @Test("A reconnect in the background renders connecting, returns to content, and adopts the new driver") + func backgroundReconnectRendersConnectingThenContent() throws { + let harness = try Harness() + defer { harness.tearDown() } + + let first = harness.injectSession(status: .connected, driver: true) + harness.controller.refreshFromActiveSessions() + #expect(harness.background.panes.renderedKey?.pane == .content) + #expect(harness.background.session?.driver === first) + + harness.injectSession(status: .connecting, driver: false) + harness.controller.refreshFromActiveSessions() + #expect(harness.background.phase == .connecting) + #expect(harness.background.panes.renderedKey?.pane == .connecting) + + let replacement = harness.injectSession(status: .connected, driver: true) + harness.controller.refreshFromActiveSessions() + #expect(harness.background.phase == .connected) + #expect(harness.background.panes.renderedKey?.pane == .content) + /// A recovered tunnel hands over a session that draws identically and carries a different + /// driver. Adopting on the phase alone is what stops the workspace holding the one the + /// recovery already disconnected, along with its cached credentials. + #expect(replacement !== first) + #expect(harness.background.session?.driver === replacement) + } + + @Test("Repeated connected status events leave the rendered panes settled") + func repeatedConnectedEventsLeavePanesSettled() throws { + let harness = try Harness() + defer { harness.tearDown() } + + harness.injectSession(status: .connected, driver: true) + harness.controller.refreshFromActiveSessions() + let settled = try #require(harness.background.panes.renderedKey) + let coordinator = try #require(harness.background.sessionState?.coordinator) + + for _ in 0..<10 { + harness.controller.refreshFromActiveSessions() + } + + #expect(harness.background.panes.renderedKey == settled) + #expect(harness.background.paneRenderKey == settled) + #expect(harness.background.sessionState?.coordinator === coordinator) + } + + @Test("A connection adopted with a live session renders content instead of staying empty") + func adoptedWorkspaceWithLiveSessionRendersContent() throws { + let harness = try Harness() + defer { harness.tearDown() } + + let adopted = TestFixtures.makeConnection(name: "Adopted") + var session = ConnectionSession(connection: adopted, driver: MockDatabaseDriver(connection: adopted)) + session.status = .connected + DatabaseManager.shared.injectSession(session, for: adopted.id) + defer { DatabaseManager.shared.removeSession(for: adopted.id) } + + let workspace = try #require( + harness.controller.adoptWorkspace( + payload: EditorTabPayload(connectionId: adopted.id), + autoConnect: false + ) + ) + defer { workspace.teardown() } + + #expect(workspace.phase == .connected) + #expect(workspace.resolvedPane == .content) + #expect(workspace.panes.renderedKey?.pane == .content) + } + + /// One window hosting two connections, with the second one in the background. Every case here + /// asks what that background workspace's panes hold, which is the state the window shows the + /// moment the user switches to it. + @MainActor + private struct Harness { + let controller: MainSplitViewController + let foreground: ConnectionWorkspace + let background: ConnectionWorkspace + let backgroundConnection: DatabaseConnection + private let window: NSWindow + + init() throws { + let foregroundConnection = TestFixtures.makeConnection(name: "Foreground") + backgroundConnection = TestFixtures.makeConnection(name: "Background") + foreground = Self.makeWorkspace(connection: foregroundConnection, phase: .idle) + background = Self.makeWorkspace(connection: backgroundConnection, phase: .connecting) + + controller = MainSplitViewController(payload: nil, sessionState: nil, adopting: foreground) + controller.workspaces.insert(background, 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 + window.orderFront(nil) + } + + @discardableResult + func injectSession(status: ConnectionStatus, driver: Bool) -> MockDatabaseDriver? { + let mock = driver ? MockDatabaseDriver(connection: backgroundConnection) : nil + var session = ConnectionSession(connection: backgroundConnection, driver: mock) + session.status = status + DatabaseManager.shared.injectSession(session, for: backgroundConnection.id) + return mock + } + + /// SwiftUI mounts a pane on the next layout pass, so a test that asks whether it mounted has + /// to let the run loop reach one. + func settle(until isSatisfied: () -> Bool) { + let deadline = Date(timeIntervalSinceNow: 2) + while !isSatisfied(), Date() < deadline { + window.contentView?.layoutSubtreeIfNeeded() + controller.view.layoutSubtreeIfNeeded() + RunLoop.current.run(until: Date(timeIntervalSinceNow: 0.02)) + } + } + + func tearDown() { + window.orderOut(nil) + window.contentViewController = nil + background.teardown() + foreground.teardown() + DatabaseManager.shared.removeSession(for: backgroundConnection.id) + } + + private static func makeWorkspace( + connection: DatabaseConnection, + phase: ConnectionWindowPhase + ) -> ConnectionWorkspace { + ConnectionWorkspace( + connectionId: connection.id, + payload: nil, + autoConnect: false, + payloadConnection: connection, + session: nil, + sessionState: nil, + rightPanelState: nil, + phase: phase + ) + } + } +}