feat(connections): disconnect a connection without closing its window - #2056
Merged
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
…nnect # Conflicts: # CHANGELOG.md
This was referenced Aug 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds a way to disconnect an active connection, which the app had no UI for at all, and fixes four bugs found while tracing what a disconnect does to an open window. Two of them lose the user's tabs.
From user feedback: "Maybe I didn't find where, but a way to disconnect an active DB connection." They had not missed it.
DatabaseManager.disconnectSession(_:)did all the real work already, and the only callers were window-close teardown, the MCPdisconnecttool, and Reset Sample Database. The only way to end a session was to close the window, which took its tabs with it.Where it lives
A top-level
Connectionmenu withDisconnectandReconnect, plus the same Disconnect on the workspace rail's context menu and on a connection's row in the connection list. All three go through oneConnectionDisconnectAction, so the confirmation exists once and reads the same everywhere.The menu bar is the surface people look at first, and the HIG is explicit that leaving a command out of it "risks making them difficult for everyone to find". The File menu was the other candidate and was rejected on the HIG's own description of it: commands that manage the files an app supports, with no slot for ending a session with a server. Apple renames that File slot to
Connectionin Screen Sharing and toShellin Terminal, but neither app handles files and TablePro does, so the app-specific menu between View and Window is the sanctioned home.CommandMenuinserts exactly there, and declaring it beforeQuerysatisfies the HIG's most-general-first ordering.Both items stay visible with one enabled, per "if a menu bar item isn't actionable, disable the action instead of hiding it". The rail does the opposite and omits Disconnect on a row with no live session, because a context menu "displays only the actions that are relevant". That split is the whole reason the enable rule is a separate pure function,
ConnectionMenuPolicy, instead of a condition inline in the view controller: the rail's menu had no test coverage at all.Neither command takes a keyboard shortcut. Cmd+E is what Finder uses for Eject, but the HIG reserves it for "use the selection for a find operation" and only permits redefining a standard shortcut when "its action doesn't make sense in your experience". TablePro has a full editor with Find, so it does not qualify. Shift+Cmd+Q is the system Log Out. TablePlus, the closest benchmark, ships no shortcut for Disconnect either.
What a disconnect does
The window stays open. Every competitor surveyed agrees on that, and TablePlus shipped it as a bug fix ("TablePlus no longer close workspace after disconnected"). The window lands on the
ContentUnavailableViewpane the app already has, with Reconnect, and no error text: nothing failed, so it does not read like a failure.That needed a sixth
ConnectionUnavailableReason,.disconnectedByUser, rather than a flag on.disconnected. It answers two questions differently and the compiler makes every switch answer them. It drops restore intent, so a connection you disconnected is not replayed at the next launch, which puts a deliberate disconnect alongside a cancelled connect under the rule already written down: a database that was down is not a user who gave up, but a user who disconnected is. It also blocks the activation connect, so clicking back into the window cannot silently undo the disconnect. Reconnect still has to work from that same phase, which is a separate question, soallowsManualConnectnow answers it instead ofallowsActivationConnectdoing double duty.Intent reaches the phase machine as a defaulted
origin:parameter ondisconnectSession, not a flag set inside it. Window close and quit run through the same primitive, and recording those as deliberate would drop every connection out of "Reopen Last Session".There is no confirmation unless a window of that connection has unsaved changes or a query running. The HIG asks for an alert when data loss is "unexpected and irreversible" and against one when "data loss is the expected result", and none of TablePlus, DataGrip, Postico, Sequel Ace or Beekeeper prompt on disconnect. When there is something to lose it is a two-way sheet, shaped like Terminal's "Do you want to terminate running processes in this window?", which also fires only when there is work in flight. Save is not offered because
saveAndCloseis built around the window going away; Cancel is the way to save first. The check spans every window of the connection, since the rail and the connection list can both fire for a connection whose windows are all in the background.Bugs fixed
Each of these predates this change, and each is reachable from the new command.
MainContentCoordinator.teardown()clearedtabManager.tabsand the pending edits with no save, and only the window-close path saved on its way out. Ending a session while its window stayed open therefore discarded that window's tab state, including through the MCPdisconnecttool and Reset Sample Database. Every disconnect now writes the tabs first, through an injectedSessionTabStatePersistingsoDatabaseManagerstill cannot see the view layer. It uses a newsaveAggregatedSync(): synchronous like the close path, because the coordinators holding those tabs are torn down immediately after, and never-clearing likesaveAggregated(), because a disconnect is not the user closing their tabs.saveAggregated()alone could not serve, since it defers throughscheduleSave, which cancels the previous task and can drop the write.Reconnecting never restored anything, not even a single window.
WindowLifecycleMonitor.unregisterWindow(for:)has no callers, and a window's id belongs to the SwiftUI content mounted inside it, so rebuilding that content (which a reconnect does) registered the sameNSWindowa second time under a new id.hasOtherWindowswas then permanently true and the restore that repopulates the tabs stood down every time.registernow supersedes any entry for the same window. This was also givinghandleWindowClosea duplicate to miss.Closing a window that never loaded any tabs deleted the connection's saved tabs, because
saveOrClearAggregatedSync()read the empty aggregate as "the user closed everything". A reconnected window is exactly that case, so a disconnect followed by a reconnect and a window close erased the state the disconnect had just written. Only a window that has actually seen a tab can clear now.A native tab window that AppKit joined but never displayed keeps whatever phase it was born with, because
installObservers()runs fromviewWillAppear. Its frozen.connectedwas enough to vote a deliberately disconnected connection back intoLastOpenConnections.json.SessionRecoveryTrackernow excludes a connection the user ended, whatever its windows still report.Two smaller ones from the same pass: the rail's confirmation sheet anchored to the connection's most recent window, which can be behind the one the user is looking at or miniaturized, and now anchors to the rail's own window; and
disconnectSessiongained an in-flight guard so two overlapping disconnects cannot have the second one finish its teardown against a session that has since reconnected.Known limitation
A connection with several windows open still comes back empty on an in-place reconnect. Restore is a "one window fans out into N" operation, so with siblings present every window correctly stands down and none of them claims it. The tabs are safe on disk and come back the next time the connection is opened, and
docs/features/tabs.mdxsays so rather than promising otherwise. Fixing it means teaching the restore to hand tabs to windows that already exist instead of opening new ones, which is a change of its own shape.Testing
Build succeeds.
swiftlint lint --strictclean across 1294 files. 71 tests pass across 11 suites, including the pre-existing persistence, session, window-title and rail suites.DatabaseManagerDisconnectTestsis new. Its first case is the regression test for the tab loss: a spy persister records whether the session still existed when it was asked, so the save cannot drift back to after the teardown. It also pins that only a user-requested origin records intent, which is what keeps quit and window close out of it.WindowLifecycleMonitorRegistrationTestsis new and covers both directions: one window registered twice is one window, two genuine windows on a connection still see each other.TabPersistenceClearGuardTestsis new and covers the clear guard, including that an empty save neither writes nor earns the right to clear.ConnectionMenuPolicyTestsis new and covers the rail's hide-not-dim rule, which had no coverage.ConnectionWindowPhaseMachineTestsgains four cases: a deliberate session loss is distinct from losing one, it drops restore intent, it blocks the activation connect while still allowing Reconnect, and Reconnect is offered from every sessionless phase except a missing plugin.No
TableProUITestscoverage. The flow needs a nativeNSAlertsheet driven across several connection windows, and the assertion that matters ("the window is still open and its tabs came back") is about window-tab and disk state rather than anything the UI test harness reads today. The parts that carry the logic are pure functions with unit tests instead.