Offer Compare Plans across query sessions, not just within one (#447) - #449
Merged
Merged
Conversation
UpdateCompareButtonState counted plans in the session's OWN sub-tabs, so two queries in two separate sessions - one plan each - left the button disabled in both, which is precisely the comparison it exists for. The plans were always reachable. MainWindow.CollectAllPlanTabs spans sessions and labels them "Query 1 > Plan", and the file-mode Compare button has always used it. That is why the reporter's workaround worked: saving a plan and reopening it gave him a file tab, whose button looks at the window-wide collection. Only the session button was looking at the wrong one, in both its enablement and its own narrower picker. So the session button now asks the window for both, and hands off to the window's picker rather than keeping a second one that cannot see past its own session. The refresh is ONE subscription to MainTabControl.Items rather than a call added at each of the sixteen places that add or remove a tab. That is the same reasoning as #438, #439 and #440, and it matters more here than usual: a plan appearing in one session changes whether Compare is available in every OTHER session, so the refresh has to be window-wide and a call site that gets forgotten leaves a stale button somewhere the author never looked. Kept a fallback to the session's own count for when there is no owning MainWindow - the control not yet attached, or hosted somewhere else - so the button is never left in a stale state rather than throwing. Tooltip updated, because it said "Compare two plan tabs" and now means something wider. No automated test, and I would rather say so than pretend. This is UI wiring and the repo has no headless Avalonia harness; a test that would have caught it needs two constructed sessions and a window. Building that harness for one bug is disproportionate, and putting Avalonia into the test process is not something to do casually given the test-host wedge in #441. A test on the arithmetic would be hollow - count >= 2 was never the broken part, the SCOPE was. Verifiable without a SQL Server, which is worth recording because two sessions holding real plans otherwise need a live connection: open two .sqlplan files, then New Query. The session's Compare button is enabled and its picker lists both file plans. On dev it stays disabled, because the session has no plans of its own and IsEnabled="False" is the XAML default. Tested: 314 total, 312 passed, 0 failed - unchanged, since nothing here touches Core. App builds clean and runs without exceptions on two plan tabs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Reviewed. This is a clean, well-contained fix — no correctness issues found.
No blocking findings. |
This was referenced Sep 2, 2026
erikdarlingdata
added a commit
that referenced
this pull request
Sep 2, 2026
…447) (#481) Compare Plans came back disabled after running two queries, on a build containing the fix that was supposed to have sorted that out. #449 fixed the enablement rule and left the refresh: a plan produced by executing a query lands by having an existing tab's Content replaced, and #449 subscribed to the tab collection, which says nothing about that. The same shape sits at window level in Get Actual Plan on a file tab. Both tab controls now go through TabContentWatcher, which reports collection changes and content replacement, so the next path that produces a plan is correct without its author knowing the watcher exists. The five hand-written refreshes in Plans.cs go with it - there is one place that decides now. The owner lookup moves off TopLevel.GetTopLevel and onto the logical tree. A TabControl realises the selected tab and nothing else, so a session in a background tab could not see its own window, and a query left running while the user works elsewhere lands its plan in exactly that state - the fallback then reinstated the original bug. Erik's own table had Query Store down as broken; it is not. Its two plan-producing sites go through AddPlanTab, which did refresh. What was broken is both execution paths and the window-level actual plan. The tests now drive those paths rather than the file path they avoided last time. What still needs a SQL Server is producing plan XML, and only that. Claude-Session: https://claude.ai/code/session_016a1AnKAHwcALrwdYVVrpgR Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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.
Fixes #447.
The bug
QuerySessionControl.UpdateCompareButtonState()counted plans in the session's own sub-tabs:Two queries in two separate sessions means one plan each, so the button stayed disabled in both — which is exactly the comparison it exists for.
Why the workaround worked
The plans were always reachable.
MainWindow.CollectAllPlanTabs()spans sessions and labels themQuery 1 > Plan, and the file-mode Compare button has always used it. Saving a plan and reopening it gave joshdbe a file tab, whose button looks at the window-wide collection. Only the session button was looking at the wrong one — in both its enablement and its own narrower picker.The fix
The session button asks the window for both, and hands off to the window's picker rather than keeping a second one that can't see past its own session.
Refresh is one subscription to
MainTabControl.Itemscollection-changed, not a call added at each of the 16 places that add or remove a tab. That matters more here than usual: a plan appearing in one session changes whether Compare is available in every other session, so a forgotten call site leaves a stale button somewhere the author never looked.Kept a fallback to the session's own count for when there's no owning
MainWindow— control not yet attached, or hosted elsewhere — so the button is never stale rather than throwing.Tooltip updated; it said "Compare two plan tabs" and now means something wider.
Verifying without a SQL Server
Worth recording, because two sessions holding real plans otherwise need a live connection: open two
.sqlplanfiles, then New Query. The session's Compare button is enabled and its picker lists both file plans. Ondevit stays disabled — the session has no plans of its own andIsEnabled="False"is the XAML default.No automated test
UI wiring, and the repo has no headless Avalonia harness — a test that would have caught this needs two constructed sessions and a window. A test on the arithmetic would be hollow:
count >= 2was never the broken part, the scope was.314 tests unchanged, since nothing here touches Core.
🤖 Generated with Claude Code