From 0655796538c85412d42cad166c67688c626da75a Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Tue, 1 Sep 2026 21:20:24 +0100 Subject: [PATCH] Restore query tabs on restart, and let Copy Path see them (#463) Plan tabs came back after a restart. Query tabs did not, even when the query had been opened from a file and the path was sitting on the session control. GetTabFilePath knew exactly one tab shape: a DockPanel with a PlanViewerControl inside it. A query tab is a QuerySessionControl with no wrapper, and it has had a SourceFilePath of its own since #459. It was simply never asked, so SaveOpenPlans wrote nothing down and RestoreOpenPlans had nothing to bring back. The same blind spot hid Copy Path on the tab context menu, which is shown only when GetTabFilePath answers. It had never appeared on a query tab. With both kinds of file in one saved list, restore routes on extension through the existing OpenFileByExtension instead of assuming a plan. Handing a .sql file to LoadPlanFile produced an "XML is not valid" box where the user's query should have been. The setting is renamed open_plans -> open_tabs now that it holds both. A file written by the previous version is still read: the old key deserializes into a migration-only property that is merged into OpenTabs on load and then nulled, so it drops out of the file on the next save rather than taking a user's restored tabs with it. Scope: this restores query tabs that came from a file. A scratch tab that was never saved has no path and still does not come back; persisting unsaved buffers is #462. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_017xj7HmCKrnsz2PWkRKT2Jx --- .../Dialogs/SettingsWindow.axaml.cs | 2 +- src/PlanViewer.App/MainWindow.FileOps.cs | 38 ++- src/PlanViewer.App/MainWindow.Tabs.cs | 14 + .../Services/AppSettingsService.cs | 36 ++- .../RestoreQueryTabsTests.cs | 240 ++++++++++++++++++ 5 files changed, 319 insertions(+), 11 deletions(-) create mode 100644 tests/PlanViewer.Core.Tests/RestoreQueryTabsTests.cs diff --git a/src/PlanViewer.App/Dialogs/SettingsWindow.axaml.cs b/src/PlanViewer.App/Dialogs/SettingsWindow.axaml.cs index 9aaa24be..420746fc 100644 --- a/src/PlanViewer.App/Dialogs/SettingsWindow.axaml.cs +++ b/src/PlanViewer.App/Dialogs/SettingsWindow.axaml.cs @@ -639,7 +639,7 @@ private void ResetAll_Click(object? sender, RoutedEventArgs e) var fresh = new AppSettings { RecentPlans = _settings.RecentPlans, - OpenPlans = _settings.OpenPlans, + OpenTabs = _settings.OpenTabs, AccuracyRatioDivergenceLimit = _settings.AccuracyRatioDivergenceLimit }; _settings = fresh; diff --git a/src/PlanViewer.App/MainWindow.FileOps.cs b/src/PlanViewer.App/MainWindow.FileOps.cs index 99c56d6e..05b30bba 100644 --- a/src/PlanViewer.App/MainWindow.FileOps.cs +++ b/src/PlanViewer.App/MainWindow.FileOps.cs @@ -384,11 +384,16 @@ private bool ValidatePlanXml(string xml, string label) } /// - /// Saves the file paths of all currently open file-based plan tabs. + /// The file behind every open tab that has one, in tab order. Plans and queries both, + /// since answers for either shape. /// - private void SaveOpenPlans() + /// + /// Separate from so a test can assert what would be persisted + /// without writing over the user's real settings file. + /// + internal List CollectOpenTabPaths() { - _appSettings.OpenPlans.Clear(); + var paths = new List(); foreach (var item in MainTabControl.Items) { @@ -396,31 +401,46 @@ private void SaveOpenPlans() var path = GetTabFilePath(tab); if (!string.IsNullOrEmpty(path)) - _appSettings.OpenPlans.Add(path); + paths.Add(path); } + return paths; + } + + /// + /// Saves the file paths of all currently open file-based tabs, plans and queries alike. + /// + private void SaveOpenPlans() + { + _appSettings.OpenTabs.Clear(); + _appSettings.OpenTabs.AddRange(CollectOpenTabPaths()); + AppSettingsService.Save(_appSettings); } /// - /// Restores plan tabs from the previous session. Skips files that no longer exist. + /// Restores the tabs from the previous session. Skips files that no longer exist. /// Falls back to a new query tab if nothing was restored. + /// + /// The saved list holds queries as well as plans, so it routes on extension the + /// same way an ordinary file open does. Sending a .sql file to LoadPlanFile would greet + /// the user with "the XML is not valid" where their query used to be. /// private void RestoreOpenPlans() { var restored = false; - foreach (var path in _appSettings.OpenPlans) + foreach (var path in _appSettings.OpenTabs) { if (File.Exists(path)) { - LoadPlanFile(path); + OpenFileByExtension(path); restored = true; } } - // Clear the open plans list now that we've restored - _appSettings.OpenPlans.Clear(); + // Clear the restored list now that its tabs are back on screen + _appSettings.OpenTabs.Clear(); AppSettingsService.Save(_appSettings); if (!restored) diff --git a/src/PlanViewer.App/MainWindow.Tabs.cs b/src/PlanViewer.App/MainWindow.Tabs.cs index 029e8549..d47477d1 100644 --- a/src/PlanViewer.App/MainWindow.Tabs.cs +++ b/src/PlanViewer.App/MainWindow.Tabs.cs @@ -187,6 +187,15 @@ private static void SetTabLabel(TabItem tab, string label) text.Text = label; } + /// + /// The file a tab was opened from, or null when nothing on disk is behind it + /// (a pasted plan, a scratch query, a Query Store tab). + /// + /// Every tab shape that can carry a path has to be recognised here, because this one + /// method answers two questions: what writes down for the next + /// session, and whether Copy Path appears on the tab's context menu. A shape it does + /// not know about loses both without saying anything. + /// private static string? GetTabFilePath(TabItem tab) { // Plans opened from file are wrapped in a DockPanel with the viewer as the last child @@ -198,6 +207,11 @@ private static void SetTabLabel(TabItem tab, string label) return v.SourceFilePath; } } + + // Queries are the session control itself, with no wrapper around it + if (tab.Content is QuerySessionControl session) + return session.SourceFilePath; + return null; } diff --git a/src/PlanViewer.App/Services/AppSettingsService.cs b/src/PlanViewer.App/Services/AppSettingsService.cs index f62ae717..56dcd678 100644 --- a/src/PlanViewer.App/Services/AppSettingsService.cs +++ b/src/PlanViewer.App/Services/AppSettingsService.cs @@ -69,6 +69,11 @@ public static AppSettings Load() settings = JsonSerializer.Deserialize(json, JsonOptions) ?? new AppSettings(); } + // Settings written before the open-tab list held queries use the old key. + // Ahead of MigrateFormatSettings, which can Save mid-load and would otherwise + // write the old key straight back out. + MigrateOpenTabs(settings); + // Migrate legacy format settings file into unified settings MigrateFormatSettings(settings); @@ -117,6 +122,23 @@ public static void Save(AppSettings settings) } } + /// + /// Moves an "open_plans" list written by an older version onto , + /// so upgrading does not cost the user the tabs they had open. Only fills an empty OpenTabs: + /// if both keys are somehow present, the current one wins. + /// + /// + /// Nothing is written here. The old key disappears from disk on the next ordinary save, which + /// happens on the first restore, so a downgrade before that point still finds its list. + /// + internal static void MigrateOpenTabs(AppSettings settings) + { + if (settings.LegacyOpenPlans is { Count: > 0 } legacy && settings.OpenTabs.Count == 0) + settings.OpenTabs = legacy; + + settings.LegacyOpenPlans = null; + } + /// /// If the old perfstudio_format_settings.json exists, migrate it into AppSettings /// (when FormatOptions is not yet set) and delete the old file unconditionally. @@ -214,8 +236,20 @@ internal sealed class AppSettings [JsonPropertyName("recent_plans")] public List RecentPlans { get; set; } = new(); + /// + /// Paths of the tabs that were open when the app last closed, reopened on the next start. + /// Holds queries as well as plans, which is why it is no longer named for plans. + /// + [JsonPropertyName("open_tabs")] + public List OpenTabs { get; set; } = new(); + + /// + /// What was called before it held queries too. Read on load so an + /// upgrade does not throw away the tabs the previous version wrote down, then nulled — + /// nulls are not serialized, so the old key drops out of the file on the next save. + /// [JsonPropertyName("open_plans")] - public List OpenPlans { get; set; } = new(); + public List? LegacyOpenPlans { get; set; } /// /// Divergence limit for accuracy ratio coloring on plan links. Default 10. diff --git a/tests/PlanViewer.Core.Tests/RestoreQueryTabsTests.cs b/tests/PlanViewer.Core.Tests/RestoreQueryTabsTests.cs new file mode 100644 index 00000000..366001b9 --- /dev/null +++ b/tests/PlanViewer.Core.Tests/RestoreQueryTabsTests.cs @@ -0,0 +1,240 @@ +using System.Collections.Generic; +using System.IO; +using System.Linq; +using System.Text.Json; +using System.Threading.Tasks; +using Avalonia.Controls; +using Avalonia.Interactivity; +using Avalonia.Threading; +using PlanViewer.App; +using PlanViewer.App.Controls; +using PlanViewer.App.Services; + +namespace PlanViewer.Core.Tests; + +/// +/// #463: plan tabs came back after a restart and query tabs did not, even when the query had been +/// opened from a file and its path was sitting right there on the session. The saved-tab list was +/// built from GetTabFilePath, which knew how to look inside a plan tab and nothing else, so a query +/// tab was invisible to it — nothing was written down, so nothing could be restored. +/// +/// The same blind spot hid Copy Path on the tab context menu, which is shown only when +/// GetTabFilePath answers, and so had never once appeared on a query tab. Both halves are covered +/// here because they are one defect, and the menu half is the easier one to fix by accident and +/// never actually check. +/// +/// These drive LoadSqlFile and the MainWindow constructor rather than the menu handlers, for the +/// same reason does: a file picker cannot be answered headlessly. +/// +public class RestoreQueryTabsTests +{ + [Fact] + public void AQueryOpenedFromAFileIsWrittenDownForTheNextSession() + { + HeadlessUi.Run(() => + { + var path = TempSql("SELECT 1 AS restored;"); + try + { + var window = new MainWindow(); + window.LoadSqlFile(path); + + Assert.Contains(path, window.CollectOpenTabPaths()); + } + finally + { + File.Delete(path); + } + }); + } + + /// + /// The deliberate edge of the fix. A never-saved scratch buffer has no path, so there is + /// nothing to write down and it does not come back — persisting unsaved text is #462's job, + /// not this one's. + /// + [Fact] + public void AScratchQueryHasNoFileAndIsNotWrittenDown() + { + HeadlessUi.Run(() => + { + var window = new MainWindow(); + window.NewQuery_Click(window, new RoutedEventArgs()); + + var session = Sessions(window).Last(); + Assert.Null(session.SourceFilePath); + Assert.Empty(window.CollectOpenTabPaths()); + }); + } + + [Fact] + public void AQueryFileComesBackAsAQueryTabOnTheNextStart() + { + HeadlessUi.Run(() => + { + var path = TempSql("SELECT 1 AS came_back;"); + try + { + Seed(path); + + var window = new MainWindow(); + + var session = Sessions(window).SingleOrDefault(s => s.SourceFilePath == path); + Assert.NotNull(session); + Assert.Equal("SELECT 1 AS came_back;", session!.QueryEditor.Text); + } + finally + { + File.Delete(path); + } + }); + } + + /// + /// The saved list now holds both kinds of file, so restore routes on extension. This is the + /// half that would break if that routing sent everything to LoadSqlFile instead. + /// + [Fact] + public void APlanFileStillComesBackAsAPlanTab() + { + HeadlessUi.Run(() => + { + var path = Path.Combine(System.AppContext.BaseDirectory, "Plans", "row_goal_plan.sqlplan"); + Seed(path); + + var window = new MainWindow(); + + Assert.Contains(path, window.CollectOpenTabPaths()); + Assert.Contains(Viewers(window), v => v.SourceFilePath == path); + }); + } + + [Fact] + public void CopyPathIsOfferedOnAQueryTabAndCopiesTheFile() + { + HeadlessUi.Run(() => + { + var path = TempSql("SELECT 1 AS copied;"); + try + { + var window = new MainWindow(); + window.LoadSqlFile(path); + + var tab = window.MainTabControl.Items + .OfType() + .Last(t => t.Content is QuerySessionControl); + + var copyPath = ContextMenuItem(tab, "Copy Path"); + Assert.True(copyPath.IsVisible, + "the query came from a file, so there is a path to copy"); + + copyPath.RaiseEvent(new RoutedEventArgs(MenuItem.ClickEvent)); + + Assert.Equal(path, Pump(ClipboardHelper.TryGetTextAsync(window))); + } + finally + { + File.Delete(path); + } + }); + } + + /// + /// A scratch tab has no path, so the menu item stays hidden — the gate still gates. + /// + [Fact] + public void CopyPathStaysHiddenOnAQueryTabWithNoFile() + { + HeadlessUi.Run(() => + { + var window = new MainWindow(); + window.NewQuery_Click(window, new RoutedEventArgs()); + + var tab = window.MainTabControl.Items + .OfType() + .Last(t => t.Content is QuerySessionControl); + + Assert.False(ContextMenuItem(tab, "Copy Path").IsVisible); + }); + } + + /// + /// The list outgrew the name "open_plans" once it started holding queries. Renaming the key + /// is free for a new install and expensive for an existing one, so the old key is still read. + /// + [Fact] + public void TabsRecordedUnderTheOldSettingsKeyAreStillRestored() + { + var settings = JsonSerializer.Deserialize( + """{"open_plans":["/tmp/one.sqlplan","/tmp/two.sql"]}""")!; + + AppSettingsService.MigrateOpenTabs(settings); + + Assert.Equal(new[] { "/tmp/one.sqlplan", "/tmp/two.sql" }, settings.OpenTabs); + Assert.Null(settings.LegacyOpenPlans); + } + + /// + /// Both keys present means a downgrade wrote the old one after the new one already existed. + /// The current key is the one that reflects the last session. + /// + [Fact] + public void TheCurrentSettingsKeyWinsOverTheOldOne() + { + var settings = JsonSerializer.Deserialize( + """{"open_plans":["/tmp/stale.sqlplan"],"open_tabs":["/tmp/current.sql"]}""")!; + + AppSettingsService.MigrateOpenTabs(settings); + + Assert.Equal(new[] { "/tmp/current.sql" }, settings.OpenTabs); + Assert.Null(settings.LegacyOpenPlans); + } + + /// + /// Puts one path where the next MainWindow will look for the previous session's tabs. + /// Load returns the process-wide cached instance, which is the same object the window reads, + /// and restore clears it again on the way out — so this does not leak into other tests. + /// + private static void Seed(string path) + { + var settings = AppSettingsService.Load(); + settings.OpenTabs.Clear(); + settings.OpenTabs.Add(path); + } + + private static MenuItem ContextMenuItem(TabItem tab, string header) => + ((StackPanel)tab.Header!).ContextMenu!.Items + .OfType() + .Single(i => (i.Header as string) == header); + + /// + /// Drains the UI queue until the clipboard call finishes. Copy Path starts its write and + /// does not await it, so the read that follows can be a dispatcher turn early. Fails rather + /// than hangs if the headless clipboard never answers. + /// + private static T Pump(Task task) + { + for (var i = 0; i < 100 && !task.IsCompleted; i++) + Dispatcher.UIThread.RunJobs(); + + Assert.True(task.IsCompleted, "the clipboard call never completed"); + return task.GetAwaiter().GetResult(); + } + + private static string TempSql(string text) + { + var path = Path.Combine(Path.GetTempPath(), $"{Path.GetRandomFileName()}.sql"); + File.WriteAllText(path, text); + return path; + } + + private static IEnumerable Sessions(MainWindow window) => + window.MainTabControl.Items.OfType().Select(t => t.Content).OfType(); + + private static IEnumerable Viewers(MainWindow window) => + window.MainTabControl.Items.OfType() + .Select(t => t.Content) + .OfType() + .SelectMany(d => d.Children) + .OfType(); +}