Notice a plan arriving by every route, not just the one with a test (#447)

[erikdarlingdata]

Fixes #447, which I closed once already and should not have.

What was actually wrong

#449 fixed the enablement rule. It did not fix the refresh, and joshdbe's report was never about the rule.

A plan gets into a session's sub-tabs two ways, and they are shaped differently:

• Adding a tab. Query Store does this, through AddPlanTab.
• Filling in a tab that is already there. Executing a query does this. The tab opens the moment you hit F5, holding a progress spinner, and when the server answers loadingTab.Content = viewer swaps the plan in. The tab count never changes.

#449 subscribed to MainTabControl.Items collection-changed and called that the single seam. A collection subscription cannot see the second shape, so every plan produced by running a query landed without a word to anybody. Two queries in two tabs, Compare dead in both — exactly as reported, on a build containing the fix.

The same shape sits at window level: Get Actual Plan on a file tab opens a spinner tab and later assigns tab.Content = CreatePlanTabContent(actualViewer) over it. Second instance of the same bug, never reported because you need a server to reach it.

And a third, found by the new tests rather than by reading: UpdateCompareButtonState located its window with TopLevel.GetTopLevel. A TabControl realises the selected tab's content and nothing else, so a session sitting in a background tab has no visual root, cannot find its own window, and falls back to counting its own plans — which is the original bug, restored. Start a query in one tab, work in another while it runs, and its plan lands in precisely that state.

Correcting my own table

I posted a table on the issue. Two of its three rows are wrong, and following it would have fixed the wrong things:

What I said	What is true
Opening a .sqlplan file into a session refreshes, via Plans.cs:109	There is no such path. AddPlanTab has exactly two callers and both are Query Store. Opening a plan file opens a window-level tab (MainWindow.FileOps.cs), which the collection subscription did catch.
Query Store does not refresh, at four SubTabControl.Items.Add sites	Query Store did refresh. Its two plan-producing sites go through AddPlanTab, which called UpdateCompareButtonState. The four Items.Add sites I pointed at add the QS grid, the QS overview and the history tab — containers, not plans.
Executing a query does not refresh	Correct, and this was the whole defect.

The new Query Store test passes on dev unmodified. It is in here as a pin on the seam swap below, not as a reproduction of anything.

The seam

Helpers/TabContentWatcher.cs, wired once to MainTabControl and once to each session's SubTabControl. It reports collection changes and content replacement on any tab it holds.

I considered just adding the missing calls at the three sites. The reason I did not is the reason we are here: #449's call sites were correct for the paths their author had in mind, and the next author did not know to look. A plan cannot reach the screen without either adding a tab or filling one in, so watching both is the smallest thing that is also complete — the next plan-producing path is right without anyone remembering this file exists.

Given that, the five hand-written UpdateCompareButtonState() calls in Plans.cs are gone. Leaving them would have meant two mechanisms doing one job, and the whole argument is that there should be one place that decides.

Content replacement is watched through TabItem.PropertyChanged filtered to ContentProperty, with subscriptions tracked in a set rather than derived from the collection-changed args — a Reset carries neither OldItems nor NewItems and would leave stale subscriptions behind.

The test, which is the part I got wrong last time

The old test file built its scenario from plan files, with a comment saying executed queries need a live SQL Server and the defect does not require one. The second half was true. The first half was the bug: a plan file opens a window-level tab, which is the one path that was already recomputing. Every assertion passed against a build where the reported behaviour was unchanged. A test that cannot reach the broken code is not coverage, and I shipped it as though it were.

Four new tests, driving the paths a plan actually arrives by:

• RunningAQueryInEachOfTwoSessionsOffersCompareInBoth — the report, verbatim.
• RunningTwoQueriesInOneSessionOffersCompareThere — the case the pre-Offer Compare Plans across query sessions, not just within one (#447) #449 arithmetic did handle, now that the arithmetic is gone.
• QueryStorePlansOfferCompare_AndClosingOneTakesItAway — Query Store's add path, and closing a plan tab through its own close button.
• AnActualPlanArrivingInAnExistingWindowTabIsNoticed — window-level content replacement.

No SQL Server. What needs one is producing plan XML, and only that: ShowCapturedPlan is the landing step both execution paths end at, and it is now internal so a test can hand it a committed fixture in place of what the server would have returned. Same for OnQueryStorePlansSelected, which needs plans, not a connection.

Red first, three ways. With the extractions kept so the tests compile, and behaviour put back to dev:

Reverted	Failing
All of it (dev behaviour)	the two execution tests + the window-level test — 3 of 4
Sub-tab watcher only, hand-calls left deleted	both execution tests + Query Store
Owner lookup back to TopLevel.GetTopLevel	the cross-session execution test

All four green with the fix in.

What is still uncovered

• The SQL round trip. CaptureAndShowPlan and GetActualPlan_Click above the landing step — connecting, SET STATISTICS XML ON, cancellation, the error paths. Unchanged by this and still server-only.
• The loading tab. The tests open their own placeholder rather than the ~35 lines each execution path builds. Those two blocks are near-identical and want extracting; not in a bug fix I cannot run against a server.
• The Query Store history path. OnHistoryPlanLoadRequested goes through the same AddPlanTab as the grid path that is covered; one of the two is tested.
• Background-tab landing. The TopLevel hole is fixed and the tests happen to exercise the unrealised case, but nothing asserts it deliberately — no test switches tabs mid-flight.

Counts

388 tests on origin/dev before this, 0 failed, 2 skipped. 392 after, 0 failed, 2 skipped. Five consecutive full runs, stable, ~15s each.

🤖 Generated with Claude Code

https://claude.ai/code/session_016a1AnKAHwcALrwdYVVrpgR

[claude]

Reviewed the diff (QuerySessionControl.*, MainWindow.PlanViewer.cs/axaml.cs, new TabContentWatcher, and the expanded test file).

The root-cause diagnosis holds up: TabControl only realizes the selected tab's Content into the visual tree, so TopLevel.GetTopLevel(this) on a session sitting in a background tab legitimately returns null and silently fell back to the per-session plan count (the original #447 bug). Switching to FindLogicalAncestorOfType<MainWindow>() is correct because Content is logically parented to its ContentControl regardless of visual realization. Unifying the "tab added/removed" + "tab content replaced" cases behind TabContentWatcher is a sound way to stop this from being re-broken at some future call site.

Checked for issues and didn't find any:

• TabContentWatcher.Resync() correctly reconciles the watched set against Reset collection-changed args (which carry no Old/NewItems), and unsubscribes PropertyChanged for removed tabs, so no listener leak on tab close.
• No infinite loop between UpdateCompareButtonState → RefreshComparePlanAvailability → SetCompareAvailability (the latter doesn't call back into UpdateCompareButtonState).
• The detached-window fallback (CountOwnPlans() >= 2 when there's no MainWindow ancestor) is intentional and pre-existing behavior, not a regression.
• The new tests actually drive the two previously-broken paths (query executed in two separate sessions; a plan landing in a window-level tab that already existed) rather than the file-open path that was already working — matches the PR's own postmortem about why the original test suite passed against a broken build.
• No T-SQL, no version-bump files, no PlanViewer.Core files needing a Blazor <Compile Include> are touched, so the repo-convention checks don't apply here.

No inline comments — nothing rises to a real finding.