Ask before a detached window throws away unsaved query changes (#473)

[erikdarlingdata]

Closes #473.

What was actually wrong

#462 gave query tabs an unsaved-changes prompt on tab close and on window close. Detach to Window walked out past both of them.

Two separate holes, one root cause — a detached session stops being a tab:

1. The window's own close asked nobody anything. DetachedWindowHelper.ShowDetached owns that path, and its Closing handler did exactly one thing: tell the content it was going away so a Query Store fetch could be cancelled. There was no question in it, because the helper is shared with plan and Query Store content and neither of those has anything to lose. A dirty query session went out the same door.

2. The shutdown prompt could not see it. TabsWithUnsavedChanges() is over MainTabControl.Items, and a detached session is not in MainTabControl.Items. So the app closed reporting nothing to save, honestly, while the edit sat in a window two inches to the left.

The second one is the nastier of the two. The first is at least a window you are looking at when you close it. The second discards work in a window you are not.

The guard

ShowDetached gains one parameter:

Func<Control, Window, Task<bool>?>? closeGuard = null

It is asked, synchronously, once, before anything is destroyed, and it answers in one of two shapes:

• null — close now, nothing asked, nothing delayed. That is read-only content, an unmodified session, and app shutdown. A plan window and a Query Store window take exactly the path they took before this change: no cancelled close, no dispatcher post, no detour. AGuardlessDetachIsUntouchedAndClosesOnTheFirstPass pins that, and the mutation that gives everyone the detour reddens it.
• a task — cancel the close and hold the window until that task answers. True re-issues it, false leaves the window open.

That is the same cancel-then-reissue MainWindow.OnClosing does, and for the same reason: Window.Closing is synchronous and a prompt is not. A closeConfirmed latch is what stops the second pass asking the question all over again. The re-issued Close() is posted rather than called, so a guard that answers without ever actually waiting cannot land Close() in the middle of the Closing handler that called it — safe to post precisely because the guard returns null during shutdown, so nothing is ever queued against a dispatcher that is going away.

Re-dock never reaches the guard. The redocked flag is set before Close(), and the handler checks it first: the content is being moved, not destroyed, so there is nothing to save it from.

The shutdown design decision, and why it is not where you would first put it

The obvious place to ask about detached windows is where they are closed — MainWindow.OnClosed, which force-closes every other window. That is the wrong place, and the issue says as much: by then the main window is already gone, the app is on its way out, and a modal raised there is at best a window nobody expects and at worst a shutdown that never finishes. A detached window that answers "Cancel" from OnClosed is a cancelled close on a window whose parent no longer exists.

So the question is asked one step earlier, from ConfirmWindowCloseAsync, while every window is still up. DetachedContentNeedsSavePrompt then returns false whenever IsShuttingDown, which is what lets OnClosed force those windows shut with no dialog and no cancelled close — the guard waves the force-close through because the question has already been asked and answered.

That inverts the usual reading of the flag: IsShuttingDown is not "skip the prompt to be safe", it is "the prompt already happened". The three ways DetachedContentNeedsSavePrompt answers no are all in that method with the reasoning attached, because the shutdown one is the one a future reader will otherwise delete.

The walk itself is now a single list:

internal List<(TabItem? Tab, QuerySessionControl Session, Window Owner)> UnsavedWorkOnClose()

Tabs first in tab order, then the detached windows. One list rather than two walks, because two walks is how the second one gets forgotten — which is the whole of #473. Tab is null for a detached session; there is no tab. Owner is the window that has to own its prompt.

No known gap on shutdown. It is not merely lossy-but-clean; the shutdown confirmation genuinely counts detached dirty sessions and cancels on any Cancel.

Which window the prompt belongs to

The prompt for a detached session is owned by the detached window, not the main one. A dialog parented to a window behind the one you are looking at is a question about something you cannot see, and at shutdown it is a question about a window that is closing.

The Save As picker had the same problem in a quieter way. SaveQueryAsync reached for StorageProvider — an unqualified this.StorageProvider, the main window's — so a detached window's Save As would have opened its picker on the wrong window. It now takes an IStorageProvider and the detached path hands it owner.StorageProvider.

SaveQueryToPath also had to learn that tab can be null. There is no tab to retitle. Everything else about the save is unchanged, including which side of the write settles the dirty state: the mutation that moves MarkClean() before the write is still red under #462's tests.

Red-then-green

Every test was written against the fix, then proven red by reverting the specific behaviour it covers, then green again. Run in the shared single-session harness the rest of the suite uses, not in isolation.

Mutation of the fix	Tests that went red
DetachTabToWindow passes no closeGuard (the pre-fix close path)	ClosingADetachedWindowWithUnsavedWorkDoesNotJustCloseIt
Detached session never goes on the register	5: dirty-detected, shutdown-count, close-refused, register-cleared, tabless-save
Register never cleared when the detached window closes	ClosingADetachedWindowTakesTheSessionOffTheRegister
Register never cleared on Re-dock	RedockingDoesNotPrompt
CloseNeedsConfirmation counts tabs only (the #462 state)	TheShutdownPromptCountsDetachedSessionsAsWellAsTabs
UnsavedWorkOnClose stops at the tab strip	2: shutdown-count, shutdown-walk
Detached prompt owned by the main window	TheShutdownWalkAsksAboutTabsAndDetachedWindowsAlike
DetachedContentNeedsSavePrompt ignores isShuttingDown	OnlyADirtySessionIsAskedAboutAndNotOnceTheAppIsShuttingDown
DetachedContentNeedsSavePrompt claims read-only content too	3: shutting-down, read-only, register-cleared
Re-dock consults the guard	RedockingDoesNotPrompt
No closeConfirmed latch — the re-issued close is questioned again	TheGuardCancelsACloseAndTheAnswerIsOnlyAskedForOnce
onClosing fires before the question is answered	2: guard-cancels, close-refused
Every detach gets the detour, guard or not	2: guardless-detach, register-cleared
SaveQueryToPath still insists on a tab	ADetachedSessionSavesWithNoTabToRetitle
Detached Save As uses the main window's picker	none — see below

Two of these are worth calling out because they caught me rather than confirming me.

Re-dock. My first version of RedockingDoesNotPrompt asserted the session came back to a tab with its edit intact — and it stayed green when I wired the guard into the re-dock path. Re-dock hands the content back whether or not the window agreed to close, so the tab coming back proves nothing on its own. What a firing guard actually leaves behind is the emptied window still open, with a prompt on it, asking about a session that is already somewhere else. The test asserts detached.IsVisible is false and detached.OwnedWindows is empty, and it reddens now.

The latch. Removing it makes the helper re-ask its way around the post-and-close loop forever, which is a hang, and a test that hangs tells you nothing. The test's stub guard carries a fuse — it refuses after three asks — so the failure is asked == 4 instead of a wedged runner. The fuse is scaffolding, not contract, and says so.

Tests

376 passed / 0 failed / 2 skipped → 385 passed / 0 failed / 2 skipped, measured on origin/dev at ba50e6d and on this branch. Run with the in-process runner; dotnet test reports zero tests under SDK 10.0.302 here, which is pre-existing and unrelated.

Covered: the dirty-detached walk in both directions, the shutdown list including its order and which window owns each prompt, the guard's three no-answers, the read-only path staying silent, Re-dock not asking, the cancel-and-keep-open behaviour, the ask-once latch, and a save with no tab behind it in both the written and the threw case.

The new tests put a real QuerySessionControl in a real detached window, and every one of them puts that window away from a finally — #474 is what a leaked window costs in a single-application headless session, and the run where cleanup matters is the run where an assertion above it failed. Nothing here puts a PlanViewerControl in a Window; read-only content is represented by a QueryStoreHistoryControl, which detaches through the same helper and takes the same silent path a plan does.

Known gaps

• Which IStorageProvider the Save As picker gets is not covered by a test. The mutation that reverts it to the main window's provider stays green. A file picker cannot be driven headlessly, so which window it parents to is not observable from a test — the same limit Warn about unsaved query changes, and mark modified tabs #462 hit and said so about. It rests on reading the code.
• Clicking one of the prompt's three buttons is still not testable. As in Warn about unsaved query changes, and mark modified tabs #462, that is why the decision is a value: DetachedContentNeedsSavePrompt for whether to ask, DecideClose for what to do with the answer. ClosingADetachedWindowWithUnsavedWorkDoesNotJustCloseIt drives a real close to a real prompt and asserts the window is still there with the edit intact, which is as far as headless goes.
• macOS app termination. Cmd+Q / Dock → Quit may not route through Window.Closing on every platform. Unchanged from Warn about unsaved query changes, and mark modified tabs #462, not chased here.
• A detached window's title does not follow a Save As. The window is closing at that point, so nothing sees the stale title. Not fixed.

🤖 Generated with Claude Code

https://claude.ai/code/session_017xj7HmCKrnsz2PWkRKT2Jx

[claude]

Reviewed the detached-window unsaved-changes fix. The design and test coverage are solid — the pure DetachedContentNeedsSavePrompt/DecideClose split, the single UnsavedWorkOnClose() walk, and the IsShuttingDown handoff between ConfirmWindowCloseAsync and OnClosed all check out under tracing (including the shutdown-ordering and re-issue-latch cases, which are the easiest parts of this to get subtly wrong).

One gap, same class of bug the PR explicitly fixes for the Save As picker:

SaveQueryToPath's failure path still parents the error dialog to the main window, not the detached one. src/PlanViewer.App/MainWindow.FileOps.cs:189 — the catch block in SaveQueryToPath calls ShowFileError(...), and ShowFileError (line 298) is hardcoded to dialog.ShowDialog(this), i.e. always the main window. When ConfirmDetachedCloseAsync drives a Save/Save As for a detached session (MainWindow.axaml.cs:371-376) and the write throws (permission error, disk full, path deleted out from under it), the error dialog comes up modal to MainWindow instead of the detached owner window that was just Activate()-ed and that the user is actually looking at — the exact problem the PR's own writeup calls out for StorageProvider ("a detached window's Save As would have opened its picker on the wrong window"), just one step later in the same flow. ShowFileError would need an optional owner parameter (mirroring the storage param added to SaveQueryAsync) threaded through SaveQueryToPath.

Low severity — it only surfaces on a save I/O failure during close — but worth a follow-up since the PR otherwise goes out of its way to get window ownership right everywhere else.