fix(tabs): ask before closing a tab that holds unsaved work - #2238
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. |
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.
Closing a single tab destroyed unsaved work with no confirmation. Reported for two cases, both reproduced: a
.sqlfile 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 calledMainContentCoordinator.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:)readtab.pendingChanges, which is only written on switch-away (MainContentCoordinator+TabSwitch.swift:30-37). The selected tab's live grid edits sit in the coordinator'sDataChangeManager. 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:
TabChangeSnapshotis notCodableandPersistedTabcarries no change fields, soRecentlyClosedTabStoreand 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.hasUnsavedWork(in:)resolves live coordinator state when the tab is selected and its own snapshot when it is not, plusisFileDirtyeither way. It now also covers structure changes and a staged Create Table, whichhasAnyUnsavedWork()was missing entirely, so window close and Quit were silently destroying staged DDL too.RecentlyClosedTabStore, so it comes back. Postico, Sequel Ace and DataGrip all make the same distinction.closeTabAwaiting(id:)presentsAlertHelper.confirmSaveChangesas a window-modal sheet. Save proceeds with the close, perNSDocument.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.saveCompletionContinuationis 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
DataChangeManager.handleTabChangecannot 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 throughbrowseScopeand 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.confirmDiscardingUnsavedWorkandConnectionCloseAction.closeboth calledsaveChanges()and then returned without closing. The save half is nowsaveSelectedTabWork(), 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.showsUnsavedIndicatorignored 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).closeTabsByUsernever unregistered the tab's source file, so a.sqlfile opened from a linked favorite could not be reopened while its window lived:TabRouter.openSQLFiletrusts 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.confirmSaveChangessheet 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: PASSverify.sh test TabCloseProtectionTests CommandActionsBulkCloseTests QueryTabProtectionTests ConnectionCloseActionTests MainContentCoordinatorTabSwitchTests: PASS, 64 executed, 64 passed, 0 failedswiftlint --strictover every changed file: cleanTabCloseProtectionTestsis new and pins the reported bug directly: a selected table tab with live cell edits and no tab switch must report unsaved work while itspendingChangessnapshot 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
TableProUITestsautomation: the three sheet arms run throughAlertHelper.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.