fix(tabs): keep a closed query tab's SQL and add Reopen Closed Tab (#1854)#1857
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4a22ea61b0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| internal static func reopen(id: UUID) { | ||
| guard let entry = RecentlyClosedTabStore.shared.consume(id: id) else { return } |
There was a problem hiding this comment.
When there is no open window for the entry's connection, this removes the recently closed entry (and consume deletes any overflow sidecar) before the async reconnect/open path has succeeded. If the reconnect fails, the password prompt is cancelled, or the saved connection is missing, LaunchIntentRouter only shows an error and the tab is already gone, so the user cannot retry Reopen Closed Tab and large unsaved SQL may be deleted. Keep the entry until the reopen path succeeds, or reinsert it on failure.
Useful? React with 👍 / 👎.
| QueryTab( | ||
| from: entry.tab, | ||
| defaultPageSize: AppSettingsManager.shared.dataGrid.defaultPageSize | ||
| ) |
There was a problem hiding this comment.
When the closed tab is file-backed, rebuilding it with QueryTab(from:) preserves sourceFileURL and the query text but leaves savedFileContent/loadMtime nil, unlike the normal disk-restore and open-file paths. After reopening foo.sql, TabQueryContent.isFileDirty always returns false because savedFileContent is nil, so subsequent edits do not show the dirty dot and the Save Changes path will not write back to the original file. Load the current file snapshot or otherwise initialize the saved-file state for reopened file-backed tabs.
Useful? React with 👍 / 👎.
1 participant
Closes #1854.
The problem
Closing a query editor was destructive by construction, in two ways that compounded.
The warning never fired for the common case.
closeTab()already has a proper Save / Cancel / Don't Save sheet, but it was fed the wrong signal.hasUnsavedChangestestedcontent.isFileDirty, andisFileDirtystarts withguard sourceFileURL != nil. A scratch "Query 1" tab was therefore never dirty, no matter how much SQL you typed into it. Same reason it never showed the native unsaved dot in the close button.The content was then actively deleted.
performClose()has three branches. The one that runs in the reported scenario (several tabs open, close one) was a barewindow.close()that touched no recovery path at all. The only recovery that existed,ClosedTabDraftStorage, was a single UserDefaults slot per connection: last write wins, consumed silently by the nextCmd+T. Close two tabs and the first one's SQL was gone. The tab state JSON could not help either, because it is recomputed from the surviving windows on every save, so a closed tab necessarily drops out of it.The approach
The reporter asked for a save/discard warning. Apple's guidance and every mature competitor point the other way: make closing non-destructive, and then no warning is needed.
NSDocument.autosavesInPlace).Shift+Cmd+T. TablePlus does the opposite and is the anti-pattern: it prompts on blank tabs, stays silent on genuinely modified ones, and still loses work.A modal also cannot save you from a crash, a force quit, or clicking "Don't Save" on autopilot. Persistence covers all three.
What changed
Closing preserves the tab. A new
RecentlyClosedTabStorecaptures the closing tab at the top ofperformClose(), before the branch dispatch, so all three branches are covered and the branching stops mattering for correctness. It is deliberately separate fromTabDiskActor: that store answers "what is open right now", this one is the append-and-prune log of what disappeared. Last 20 entries, 30 days, pruned by count and age.Reopen Closed Tab.
Shift+Cmd+Tand a File > Recently Closed submenu. Reopening routes through the sameRestorationGroupRegistrymechanism cold launch already uses, so a reopened table tab recovers its filters, sort, and column layout instead of reimplementing that. If the connection has no window left, a newLaunchIntent.reopenClosedTabreconnects first; if the connection was deleted, the existing "Connection Failed" path handles it. Reopening into the empty window left behind by closing the last tab fills that window in place, matching how New Tab already reuses it, so no blank tab is stranded beside the restored one.The unsaved dot now shows for a scratch query tab holding text, via a new
QueryTab.showsUnsavedIndicator.The Save sheet is narrowed to the one case where loss is unrecoverable: a file-backed
.sqltab with unsaved edits. No modal on throwaway scratch tabs.Predicates unified.
hasUnsavedChanges(close) andisActiveTabReusable(the tab replacement guard) had drifted apart, each blind to what the other checked. The query-tab facts now live inQueryTab+Protection.swiftand the coordinator-level "destructive work" inMainContentCoordinator+Protection.swift, which also feeds the quit check.ClosedTabDraftStorageis deleted.Cmd+Tno longer silently pre-fills a new tab with the last closed tab's query; that behaviour is replaced by the explicitShift+Cmd+T.Two bugs found along the way
Oversized queries would have come back empty.
toPersistedTab()blanks any query over 500 KB to keep the tab-state JSON small. Had the recovery store inherited that, a large query would have been silently truncated to nothing: data loss with extra steps. Oversized text goes to a sidecar file instead, and a test asserts a >500 KB query survives a store restart byte for byte.Quitting warned about almost nothing.
hasAnyUnsavedChanges()checked only cell edits and pending changes. It missedisFileDirty, sidebar edits, and pending truncates and deletes, so quitting with unsaved edits in a.sqlfile warned about nothing at all. Now wired to the shared predicate.Testing
25 new unit tests: the protection predicates, the store (LIFO order, count and age pruning, blank-tab rejection, oversized-query overflow round trip, per-connection removal), and
adoptTab.MainContentCommandActionshad zero coverage before this.OpenTableTabTests(the tab replacement guard invariant),TabPersistenceCoordinatorTests,CommandActionsDispatchTests,CoordinatorEditorLoadTests, andConnectionStorageSyncDeleteTestsall pass with the change.The local full suite reports pre-existing failures unrelated to this work:
SaveCompletionTestsfails identically on a cleanorigin/maincheckout, verified by stashing this branch and rerunning.No UI automation. The existing
TableProUITestsis a launch smoke test with no connection scaffolding, and scripting connect, type,Cmd+W,Shift+Cmd+Tneeds a live Sample Database session that does not run deterministically. Flagging this per the testing rule rather than shipping a flaky test.Manual check
Open three or four query tabs, type into one,
Cmd+Wit. It should close with no sheet.Shift+Cmd+Tbrings it back with its SQL and cursor position. Close every tab, thenShift+Cmd+T, and exactly one tab should come back.