From 018a82fc6b26f230df61746c9e435d847194a053 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Wed, 2 Sep 2026 18:15:44 +0100 Subject: [PATCH 01/26] Notice a plan arriving by every route, not just the one with a test (#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 --- .../Controls/QuerySessionControl.Execution.cs | 46 +++-- .../Controls/QuerySessionControl.Plans.cs | 20 +- .../QuerySessionControl.QueryStore.cs | 9 +- .../Controls/QuerySessionControl.axaml.cs | 8 + .../Helpers/TabContentWatcher.cs | 76 ++++++++ src/PlanViewer.App/MainWindow.PlanViewer.cs | 8 +- src/PlanViewer.App/MainWindow.axaml.cs | 11 +- .../ComparePlansAvailabilityTests.cs | 184 +++++++++++++++++- 8 files changed, 327 insertions(+), 35 deletions(-) create mode 100644 src/PlanViewer.App/Helpers/TabContentWatcher.cs diff --git a/src/PlanViewer.App/Controls/QuerySessionControl.Execution.cs b/src/PlanViewer.App/Controls/QuerySessionControl.Execution.cs index 9f9582a..1de145f 100644 --- a/src/PlanViewer.App/Controls/QuerySessionControl.Execution.cs +++ b/src/PlanViewer.App/Controls/QuerySessionControl.Execution.cs @@ -197,15 +197,7 @@ failure is reported. A SQL error is the one string in this app a user most needs // Replace loading content with the plan viewer SetStatus($"{planType} plan captured ({sw.Elapsed.TotalSeconds:F1}s)"); - var viewer = new PlanViewerControl(); - viewer.Metadata = _serverMetadata; - viewer.ConnectionString = _connectionString; - viewer.SetConnectionServices(_credentialService, _connectionStore); - if (_serverConnection != null) - viewer.SetConnectionStatus(_serverConnection.ServerName, _selectedDatabase); - viewer.OpenInEditorRequested += OnOpenInEditorRequested; - viewer.LoadPlan(planXml, tabLabel, queryText); - loadingTab.Content = viewer; + ShowCapturedPlan(loadingTab, planXml, tabLabel, queryText); HumanAdviceButton.IsEnabled = true; RobotAdviceButton.IsEnabled = true; } @@ -224,6 +216,32 @@ failure is reported. A SQL error is the one string in this app a user most needs } } + /// + /// Puts a captured plan into the tab that has been showing the progress spinner for it. + /// + /// Both execution paths end here — the estimated/actual capture and Get Actual Plan — and + /// this is the moment a plan appears in a session, so it is the moment Compare Plans has to be + /// re-decided. That happens through the sub-tab + /// rather than a call added below, because the assignment + /// on the last line is what it is watching for (#447). + /// + /// Internal so a test can drive the plan landing with XML it already has, rather than + /// needing a SQL Server to produce some. The half of these paths that reaches out to a server + /// is above this; everything that decides what the user ends up looking at is here. + /// + internal void ShowCapturedPlan(TabItem planTab, string planXml, string tabLabel, string queryText) + { + var viewer = new PlanViewerControl(); + viewer.Metadata = _serverMetadata; + viewer.ConnectionString = _connectionString; + viewer.SetConnectionServices(_credentialService, _connectionStore); + if (_serverConnection != null) + viewer.SetConnectionStatus(_serverConnection.ServerName, _selectedDatabase); + viewer.OpenInEditorRequested += OnOpenInEditorRequested; + viewer.LoadPlan(planXml, tabLabel, queryText); + planTab.Content = viewer; + } + /// /// Reports a query failure in the plan tab, in full (#448). /// @@ -402,15 +420,7 @@ private async void GetActualPlan_Click(object? sender, RoutedEventArgs e) } SetStatus($"Actual plan captured ({sw.Elapsed.TotalSeconds:F1}s)"); - var actualViewer = new PlanViewerControl(); - actualViewer.Metadata = _serverMetadata; - actualViewer.ConnectionString = _connectionString; - actualViewer.SetConnectionServices(_credentialService, _connectionStore); - if (_serverConnection != null) - actualViewer.SetConnectionStatus(_serverConnection.ServerName, _selectedDatabase); - actualViewer.OpenInEditorRequested += OnOpenInEditorRequested; - actualViewer.LoadPlan(actualPlanXml, tabLabel, queryText); - loadingTab.Content = actualViewer; + ShowCapturedPlan(loadingTab, actualPlanXml, tabLabel, queryText); } catch (OperationCanceledException) { diff --git a/src/PlanViewer.App/Controls/QuerySessionControl.Plans.cs b/src/PlanViewer.App/Controls/QuerySessionControl.Plans.cs index f230526..fa59d4e 100644 --- a/src/PlanViewer.App/Controls/QuerySessionControl.Plans.cs +++ b/src/PlanViewer.App/Controls/QuerySessionControl.Plans.cs @@ -13,12 +13,14 @@ using Avalonia.Input.Platform; using Avalonia.Interactivity; using Avalonia.Layout; +using Avalonia.LogicalTree; using Avalonia.Media; using AvaloniaEdit; using AvaloniaEdit.CodeCompletion; using AvaloniaEdit.TextMate; using Microsoft.Data.SqlClient; using PlanViewer.App.Dialogs; +using PlanViewer.App.Helpers; using PlanViewer.App.Services; using PlanViewer.Core.Interfaces; using PlanViewer.Core.Models; @@ -108,7 +110,6 @@ private bool AddPlanTab(string planXml, string queryText, bool estimated, string SubTabControl.Items.Add(tab); SubTabControl.SelectedItem = tab; - UpdateCompareButtonState(); return true; } @@ -159,7 +160,6 @@ private void ClosePlanTab_Click(object? sender, RoutedEventArgs e) if (tab.Content is PlanViewerControl viewer) viewer.Clear(); SubTabControl.Items.Remove(tab); - UpdateCompareButtonState(); } } @@ -180,7 +180,6 @@ private void PlanTabContextMenu_Click(object? sender, RoutedEventArgs e) if (tab.Content is PlanViewerControl closeViewer) closeViewer.Clear(); SubTabControl.Items.Remove(tab); - UpdateCompareButtonState(); } break; @@ -199,7 +198,6 @@ private void PlanTabContextMenu_Click(object? sender, RoutedEventArgs e) SubTabControl.Items.Remove(t); } SubTabControl.SelectedItem = keepTab; - UpdateCompareButtonState(); } break; @@ -215,7 +213,6 @@ private void PlanTabContextMenu_Click(object? sender, RoutedEventArgs e) SubTabControl.Items.Remove(t); } SubTabControl.SelectedIndex = 0; // back to Editor - UpdateCompareButtonState(); break; } } @@ -225,10 +222,21 @@ private void PlanTabContextMenu_Click(object? sender, RoutedEventArgs e) /// from another is the ordinary case, and counting only this session's own tabs left the button /// disabled in both — the reporter had to save a plan and reopen it to get at a comparison the /// app could already do. + /// + /// Called by the wired to this session's sub-tabs, and by + /// nothing else. It used to be called by hand at the five places that add or remove a plan tab, + /// which is why the paths that instead fill in an existing tab — every executed query — never + /// reached it. /// private void UpdateCompareButtonState() { - if (TopLevel.GetTopLevel(this) is MainWindow owner) + /* Logical tree, not 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 and cannot + see its own window — and a query started in one tab and left to run while the user works + in another lands its plan in exactly that state. GetTopLevel returned null there and the + fallback below silently reinstated the bug this method exists to fix. The logical parent + chain holds whether the tab is on screen or not. */ + if (this.FindLogicalAncestorOfType() is { } owner) { /* Refreshes every session, not just this one: a plan appearing here can be the second plan that makes Compare available over THERE. */ diff --git a/src/PlanViewer.App/Controls/QuerySessionControl.QueryStore.cs b/src/PlanViewer.App/Controls/QuerySessionControl.QueryStore.cs index fe2bf9b..ff786cb 100644 --- a/src/PlanViewer.App/Controls/QuerySessionControl.QueryStore.cs +++ b/src/PlanViewer.App/Controls/QuerySessionControl.QueryStore.cs @@ -261,7 +261,14 @@ family as #448. */ SubTabControl.SelectedItem = tab; } - private void OnQueryStorePlansSelected(object? sender, List plans) + /// + /// Opens a plan tab for each plan picked out of the Query Store grid. + /// + /// Internal so a test can hand it plans it already has. The grid is what fetches them from + /// a server; nothing below this line needs one, which is what makes the Query Store side of + /// #447 testable without a live instance. + /// + internal void OnQueryStorePlansSelected(object? sender, List plans) { int loaded = 0; foreach (var qsPlan in plans) diff --git a/src/PlanViewer.App/Controls/QuerySessionControl.axaml.cs b/src/PlanViewer.App/Controls/QuerySessionControl.axaml.cs index 8be55ab..566c656 100644 --- a/src/PlanViewer.App/Controls/QuerySessionControl.axaml.cs +++ b/src/PlanViewer.App/Controls/QuerySessionControl.axaml.cs @@ -18,6 +18,7 @@ using AvaloniaEdit.TextMate; using Microsoft.Data.SqlClient; using PlanViewer.App.Dialogs; +using PlanViewer.App.Helpers; using PlanViewer.App.Services; using PlanViewer.Core.Interfaces; using PlanViewer.Core.Models; @@ -131,6 +132,13 @@ public QuerySessionControl(ICredentialService credentialService, ConnectionStore _statusClearCts = null; }; + /* #447: a plan appearing in — or leaving — this session changes whether Compare Plans is + offered in every session in the window, not just this one. Watched here rather than + called at each site that produces a plan, because the sites that produce a plan are the + ones nobody remembers: executing a query fills in a tab that already exists, which is + neither an Add nor a Remove and is exactly the case the first fix missed. */ + TabContentWatcher.Watch(SubTabControl, UpdateCompareButtonState); + // Focus the editor when the Editor tab is selected; toggle plan-dependent buttons SubTabControl.SelectionChanged += (_, _) => { diff --git a/src/PlanViewer.App/Helpers/TabContentWatcher.cs b/src/PlanViewer.App/Helpers/TabContentWatcher.cs new file mode 100644 index 0000000..bd78cc3 --- /dev/null +++ b/src/PlanViewer.App/Helpers/TabContentWatcher.cs @@ -0,0 +1,76 @@ +using System; +using System.Collections.Generic; +using System.Collections.Specialized; +using System.Linq; +using Avalonia; +using Avalonia.Controls; + +namespace PlanViewer.App.Helpers; + +/// +/// Reports every change to what a is showing, so state derived from its +/// contents can be recomputed without a call planted at each site that changes them (#447). +/// +/// Why a collection subscription is not enough. A tab's content is changed two ways, +/// and only one of them is a collection change. Tabs are added and removed, which +/// Items raises; but a tab is also created holding a progress spinner and later has its +/// Content swapped for the finished article — which is how every plan produced by executing +/// a query arrives, and which the collection says nothing about. #449 watched the collection alone +/// and so fixed the file path while leaving the execution path exactly as broken as it was +/// reported. +/// +/// Both are watched here, which is the point: the next path that produces a plan is correct +/// without its author knowing this exists, because a plan cannot reach the screen without either +/// adding a tab or filling one in. +/// +internal static class TabContentWatcher +{ + /// + /// Invokes whenever a tab is added to or removed from + /// , or an existing tab's content is replaced. + /// + /// Meant to be called once, where the control is built. The subscriptions live as long as + /// the tab control does, which for both call sites is the lifetime of the window. + /// + internal static void Watch(TabControl tabs, Action onChanged) + { + /* Tracked rather than derived from the collection-changed args, because a Reset carries + neither OldItems nor NewItems and would otherwise leave stale subscriptions behind. */ + var watched = new HashSet(); + + void OnTabPropertyChanged(object? sender, AvaloniaPropertyChangedEventArgs e) + { + if (e.Property == ContentControl.ContentProperty) + onChanged(); + } + + void Resync() + { + var current = tabs.Items.OfType().ToHashSet(); + + foreach (var gone in watched.Except(current).ToList()) + { + gone.PropertyChanged -= OnTabPropertyChanged; + watched.Remove(gone); + } + + foreach (var arrived in current.Except(watched).ToList()) + { + arrived.PropertyChanged += OnTabPropertyChanged; + watched.Add(arrived); + } + } + + /* Tabs declared in XAML are already in the collection before anyone gets to watch it. */ + Resync(); + + if (tabs.Items is INotifyCollectionChanged observable) + { + observable.CollectionChanged += (_, _) => + { + Resync(); + onChanged(); + }; + } + } +} diff --git a/src/PlanViewer.App/MainWindow.PlanViewer.cs b/src/PlanViewer.App/MainWindow.PlanViewer.cs index abf53ac..a39b64e 100644 --- a/src/PlanViewer.App/MainWindow.PlanViewer.cs +++ b/src/PlanViewer.App/MainWindow.PlanViewer.cs @@ -27,7 +27,13 @@ namespace PlanViewer.App; public partial class MainWindow : Window { - private DockPanel CreatePlanTabContent(PlanViewerControl viewer) + /// + /// Wraps a loaded plan in the toolbar a window-level plan tab shows above it. + /// + /// Internal so a test can reproduce what Get Actual Plan does to a file tab — build this + /// and assign it over a tab that was holding a spinner — without executing anything (#447). + /// + internal DockPanel CreatePlanTabContent(PlanViewerControl viewer) { var humanBtn = new Button { diff --git a/src/PlanViewer.App/MainWindow.axaml.cs b/src/PlanViewer.App/MainWindow.axaml.cs index 466574c..6f748d1 100644 --- a/src/PlanViewer.App/MainWindow.axaml.cs +++ b/src/PlanViewer.App/MainWindow.axaml.cs @@ -1,6 +1,5 @@ using System; using System.Collections.Generic; -using System.Collections.Specialized; using System.IO; using System.IO.Pipes; using System.Linq; @@ -19,6 +18,7 @@ using Avalonia.Threading; using PlanViewer.App.Controls; using PlanViewer.App.Dialogs; +using PlanViewer.App.Helpers; using PlanViewer.App.Services; using PlanViewer.Core.Interfaces; using PlanViewer.Core.Models; @@ -79,9 +79,12 @@ public MainWindow() remove a tab. Compare Plans depends on how many plans exist across the WHOLE window, so opening or closing any tab can change whether it is available in every OTHER tab, and a refresh that has to be remembered at sixteen call sites is one that gets forgotten at the - seventeenth. */ - if (MainTabControl.Items is INotifyCollectionChanged tabs) - tabs.CollectionChanged += (_, _) => RefreshComparePlanAvailability(); + seventeenth. + + Watching content replacement as well as the collection is the part the first attempt at + this got wrong: Get Actual Plan opens a tab holding a spinner and later swaps in the plan, + so the tab that gains a plan is one this window already had. */ + TabContentWatcher.Watch(MainTabControl, RefreshComparePlanAvailability); // Global hotkeys via tunnel routing so they fire before AvaloniaEdit consumes them AddHandler(KeyDownEvent, (_, e) => diff --git a/tests/PlanViewer.Core.Tests/ComparePlansAvailabilityTests.cs b/tests/PlanViewer.Core.Tests/ComparePlansAvailabilityTests.cs index b8b70f9..591ace4 100644 --- a/tests/PlanViewer.Core.Tests/ComparePlansAvailabilityTests.cs +++ b/tests/PlanViewer.Core.Tests/ComparePlansAvailabilityTests.cs @@ -1,9 +1,11 @@ +using System.Collections.Generic; using System.IO; using System.Linq; using Avalonia.Controls; using Avalonia.Interactivity; using PlanViewer.App; using PlanViewer.App.Controls; +using PlanViewer.Core.Models; namespace PlanViewer.Core.Tests; @@ -14,13 +16,128 @@ namespace PlanViewer.Core.Tests; /// able to see across sessions. The reporter had to save a plan and reopen it to get at a comparison /// the app could already do. /// -/// The scenario is built from plan FILES rather than executed queries deliberately: getting a plan -/// into a session needs a live SQL Server, and the defect does not require one. What it requires is -/// plans existing somewhere OTHER than the session whose button is being judged, which two file tabs -/// provide exactly. +/// The first version of this file is why the issue was reopened. It built its scenario +/// out of plan FILES on purpose, reasoning that getting a plan into a session needed a live SQL +/// Server and that the defect did not require one. The second half of that was true and the first +/// half was the bug: a plan file opens a window-level tab, which is the one path that was already +/// recomputing. Every test passed against a build in which running two queries — the thing being +/// reported — still left both buttons dead. +/// +/// So the tests below drive the paths a plan actually arrives by. What still needs a server is +/// the round trip that produces plan XML, and only that: the landing step each path performs with +/// the XML in hand is reachable directly, and is where the whole defect lived. /// public class ComparePlansAvailabilityTests { + /// + /// The report, verbatim: a query run in one tab, another query run in a second tab, Compare + /// disabled in both. Driven through the same ShowCapturedPlan both execution paths use + /// once the server has answered. + /// + [Fact] + public void RunningAQueryInEachOfTwoSessionsOffersCompareInBoth() + { + HeadlessUi.Run(() => + { + var window = new MainWindow(); + + var first = NewSession(window); + var second = NewSession(window); + + RunQuery(first, "row_goal_plan.sqlplan", "Plan 1"); + + Assert.All(new[] { first, second }, session => Assert.False( + CompareButton(session).IsEnabled, + "one plan cannot be compared against anything")); + + RunQuery(second, "key_lookup_plan.sqlplan", "Plan 1"); + + Assert.All(new[] { first, second }, session => Assert.True( + CompareButton(session).IsEnabled, + "a query has now run in each session, which is the whole of the report")); + }); + } + + /// + /// Two queries run in the SAME session, which is the case the pre-#449 arithmetic did handle. + /// Here because that arithmetic no longer exists — the count comes from the window now, and a + /// window-wide count has its own way of getting a single session wrong. + /// + [Fact] + public void RunningTwoQueriesInOneSessionOffersCompareThere() + { + HeadlessUi.Run(() => + { + var window = new MainWindow(); + var session = NewSession(window); + + RunQuery(session, "row_goal_plan.sqlplan", "Plan 1"); + RunQuery(session, "key_lookup_plan.sqlplan", "Plan 2"); + + Assert.True(CompareButton(session).IsEnabled); + }); + } + + /// + /// The Query Store path, which opens its plans by adding tabs rather than filling one in, and a + /// plan tab being closed again. Both used to say so with a call written out at the site; they + /// now go through the same watcher as everything else, so they need pinning where they did not + /// before. + /// + [Fact] + public void QueryStorePlansOfferCompare_AndClosingOneTakesItAway() + { + HeadlessUi.Run(() => + { + var window = new MainWindow(); + var session = NewSession(window); + + session.OnQueryStorePlansSelected(null, new List + { + QueryStorePlanFrom(11, "row_goal_plan.sqlplan"), + QueryStorePlanFrom(22, "key_lookup_plan.sqlplan") + }); + + Assert.True(CompareButton(session).IsEnabled, + "two Query Store plans are open in this session"); + + ClosePlanTab(session); + + Assert.False(CompareButton(session).IsEnabled, + "one of the two was closed, so there is nothing left to compare against"); + }); + } + + /// + /// Get Actual Plan on a window-level plan tab, which produces its plan the same way an executed + /// query does — into a tab that has been showing a spinner since the query was sent, and so is + /// not an addition to anything. + /// + [Fact] + public void AnActualPlanArrivingInAnExistingWindowTabIsNoticed() + { + HeadlessUi.Run(() => + { + var window = new MainWindow(); + + window.LoadPlanFile(PlanPath("row_goal_plan.sqlplan")); + var session = NewSession(window); + + Assert.False(CompareButton(session).IsEnabled); + + var tabs = window.FindControl("MainTabControl")!; + var spinnerTab = new TabItem { Header = "Actual Plan", Content = new Grid() }; + tabs.Items.Add(spinnerTab); + + var viewer = new PlanViewerControl(); + Assert.True(viewer.LoadPlan(PlanXml("key_lookup_plan.sqlplan"), "Actual Plan")); + spinnerTab.Content = window.CreatePlanTabContent(viewer); + + Assert.True(CompareButton(session).IsEnabled, + "the window gained a second plan without gaining a tab"); + }); + } + [Fact] public void ASessionOffersCompareWhenThePlansAreElsewhereInTheWindow() { @@ -86,15 +203,72 @@ public void OpeningASecondPlanEnablesCompareInASessionThatAlreadyExisted() }); } + /// + /// Runs a query in a session as far as a test without a SQL Server can. + /// + /// The tab holding the progress spinner is opened first, exactly as the execution paths + /// open it, and the session is then handed plan XML from a fixture in place of what the server + /// would have returned. Everything after that — building the viewer, loading the plan into it, + /// and assigning it over the spinner — is the session's own code, and the assignment is the + /// statement #447 was about. + /// + private static void RunQuery(QuerySessionControl session, string planFileName, string tabLabel) + { + var subTabs = SubTabs(session); + var spinnerTab = new TabItem { Header = tabLabel, Content = new Grid() }; + subTabs.Items.Add(spinnerTab); + subTabs.SelectedItem = spinnerTab; + + session.ShowCapturedPlan(spinnerTab, PlanXml(planFileName), tabLabel, "select 1;"); + } + + /// + /// Closes the session's first plan tab through its own close button, rather than reaching into + /// the collection — the removal paths are production code too, and they lost their hand-written + /// refresh along with the paths that add. + /// + private static void ClosePlanTab(QuerySessionControl session) + { + var tab = SubTabs(session).Items + .OfType() + .First(t => t.Content is PlanViewerControl); + + var closeButton = ((StackPanel)tab.Header!).Children.OfType