Skip to content

fix(tabs): ask before closing a tab that holds unsaved work - #2238

Merged
datlechin merged 1 commit into
mainfrom
fix/warn-before-closing-tab-with-unsaved-work
Aug 19, 2026
Merged

fix(tabs): ask before closing a tab that holds unsaved work#2238
datlechin merged 1 commit into
mainfrom
fix/warn-before-closing-tab-with-unsaved-work

Conversation

@datlechin

Copy link
Copy Markdown
Member

Closing a single tab destroyed unsaved work with no confirmation. Reported for two cases, both reproduced: a .sql file tab with unsaved edits, and a table tab with edited cells.

Root cause

Two defects that compose.

No gate. Every single-tab-close gesture funnels through MainContentCommandActions.closeTab(id:), which called MainContentCoordinator.closeTabsByUser(ids:) directly. Window close (closeWindowAwaiting) and the batch closes (runBatchClose) both gate on unsaved work; the single-tab path gated on nothing.

The per-tab predicate was blind to live edits. hasUnsavedWork(in:) read tab.pendingChanges, which is only written on switch-away (MainContentCoordinator+TabSwitch.swift:30-37). The selected tab's live grid edits sit in the coordinator's DataChangeManager. So even a gate using the existing predicate would still have missed the reported table-tab case, because nothing has switched away to write the snapshot.

Grid edits were the worst case: TabChangeSnapshot is not Codable and PersistedTab carries no change fields, so RecentlyClosedTabStore and tab persistence both drop them. Nothing brought them back.

The fix

Refactored the predicate layer rather than patching the call site, because the existing one is stale by design for the selected tab and its only selected-tab helper (hasUnsavedWorkInSelectedTab) was dead and mis-scoped.

  • One tab-scoped predicate. hasUnsavedWork(in:) resolves live coordinator state when the tab is selected and its own snapshot when it is not, plus isFileDirty either way. It now also covers structure changes and a staged Create Table, which hasAnyUnsavedWork() was missing entirely, so window close and Quit were silently destroying staged DDL too.
  • Connection-scoped work stays out of it. A staged TRUNCATE or an unsaved sidebar edit belongs to the connection, so it must not gate an unrelated tab: neither Save nor Don't Save could answer for the tab being closed.
  • Scratch query text stays unprompted. It is persisted with the tab and filed in RecentlyClosedTabStore, so it comes back. Postico, Sequel Ace and DataGrip all make the same distinction.
  • The gate itself. closeTabAwaiting(id:) presents AlertHelper.confirmSaveChanges as a window-modal sheet. Save proceeds with the close, per NSDocument.canCloseDocumentWithDelegate: "shouldClose will be YES if ... the user chose to discard modifications, or chose to save and the saving was successful". Closing a background tab selects it first, so the question is about work the user can see, and Cancel puts the selection back.
  • Reentrancy guard. saveCompletionContinuation is a single slot, so a second Cmd+W arriving before the first sheet resolved would overwrite the continuation the first was suspended on and hang that task.

Also fixed, because the primary fix is unsafe or incoherent without them

  • Orphaned change manager (blocking). Closing a tab never consumed the coordinator's DataChangeManager. handleTabChange cannot snapshot a tab that is already removed, and nothing else cleared it, so the edits outlived their tab: the connection went on reporting unsaved work with no tabs, and a later Save resolved its scope through browseScope and would run those statements against whatever database the sidebar had since moved to. Without this, Don't Save would have converted silent loss into a wrong-database write.
  • Save aborting the close. confirmDiscardingUnsavedWork and ConnectionCloseAction.close both called saveChanges() and then returned without closing. The save half is now saveSelectedTabWork(), shared by all three close paths so they cannot drift again. It keeps the one deliberate exception: user and role changes can only be applied after review, so Save opens the review sheet and stands the close down.
  • The unsaved dot. showsUnsavedIndicator ignored grid edits, so a table tab with pending cell edits was marked clean and would then ask to be saved. The dot now covers everything the gate covers, and stays deliberately broader (a scratch tab shows the dot without being gated).
  • Stale source-file registration. closeTabsByUser never unregistered the tab's source file, so a .sql file opened from a linked favorite could not be reopened while its window lived: TabRouter.openSQLFile trusts that registry without re-checking and just raised the window.

Before / After

There is no "before" screenshot to show: the defect is that no UI appeared at all. The "after" is the existing AlertHelper.confirmSaveChanges sheet already shipped for window close and batch close, now presented on one more path, so this introduces no new visual design.

Verification

Built and tested locally against the real app target.

  • verify.sh build: PASS
  • verify.sh test TabCloseProtectionTests CommandActionsBulkCloseTests QueryTabProtectionTests ConnectionCloseActionTests MainContentCoordinatorTabSwitchTests: PASS, 64 executed, 64 passed, 0 failed
  • swiftlint --strict over every changed file: clean

TabCloseProtectionTests is new and pins the reported bug directly: a selected table tab with live cell edits and no tab switch must report unsaved work while its pendingChanges snapshot is still empty. It also pins the background-tab case, the scratch-tab exclusion, the connection-scoped exclusion, the dot agreeing with the gate, and the three pieces of coordinator state a closed tab has to take with it.

No TableProUITests automation: the three sheet arms run through AlertHelper.run(_:in:), which has no injection seam, and the suite has no existing coverage of this sheet even for the already-gated window-close path. The predicate and the state cleanup, which is where the defect actually lived, are covered by unit tests.

@mintlify

mintlify Bot commented Aug 19, 2026

Copy link
Copy Markdown

Preview deployment for your docs. Learn more about Mintlify Previews.

Project Status Preview Updated (UTC)
TablePro 🟢 Ready View Preview Aug 19, 2026, 10:15 AM

💡 Tip: Enable Workflows to automatically generate PRs for you.

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@datlechin
datlechin merged commit b289799 into main Aug 19, 2026
9 checks passed
@datlechin
datlechin deleted the fix/warn-before-closing-tab-with-unsaved-work branch August 19, 2026 11:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant