From 1414d27120d501db120779b06e9afcbf4bbe7514 Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Tue, 17 Mar 2026 10:02:57 -0400 Subject: [PATCH] Fix code review findings across Core, CLI, and UI layers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Core: - ServerMetadataService: using → await using for async disposal - KeychainCredentialService: fix potential deadlock on stdout/stderr - ShowPlanParser: add ParseError property instead of silent swallow - EstimatedPlanExecutor: null check on BatchSequence in merge - ServerConnection: throw when SQL auth credentials missing - QueryStoreService: fix PlanId filter applied after ROW_NUMBER CLI: - AnalyzeCommand: add missing return after credential check failure - AnalyzeCommand: wrap BuildServerConnection in try/catch - AnalyzeCommand: respect --output flag in live mode - Add --top/--hours-back/--timeout validation - QueryStoreCommand: add .env file support - Extract shared ConnectionHelper to eliminate duplication UI: - Fix MCP session leak: call viewer.Clear() on all tab close paths - Dispose CancellationTokenSource before creating new instances - Dispose TextMate.Installation on DetachedFromVisualTree - Consolidate duplicate Connect_Click / ShowConnectionDialogAsync - Cancel in-flight fetch on QS database change - Add try/catch to SavePlan_Click - Log MCP startup errors to Debug output - Validate numeric input in QS search (Query ID / Plan ID) - Remove dead QueryStoreDialog (superseded by QueryStoreGridControl) - Remove dead _currentPropertySection field Co-Authored-By: Claude Opus 4.6 (1M context) --- .../Controls/PlanViewerControl.axaml.cs | 18 +- .../Controls/QuerySessionControl.axaml.cs | 56 ++--- .../Controls/QueryStoreGridControl.axaml.cs | 9 + .../Dialogs/QueryStoreDialog.axaml | 89 -------- .../Dialogs/QueryStoreDialog.axaml.cs | 201 ------------------ src/PlanViewer.App/Mcp/McpHostService.cs | 4 +- src/PlanViewer.Cli/Commands/AnalyzeCommand.cs | 102 +++------ .../Commands/QueryStoreCommand.cs | 41 ++-- src/PlanViewer.Cli/ConnectionHelper.cs | 63 ++++++ src/PlanViewer.Core/Models/PlanModels.cs | 1 + .../Models/ServerConnection.cs | 10 +- .../Services/EstimatedPlanExecutor.cs | 4 +- .../Services/KeychainCredentialService.cs | 3 +- .../Services/QueryStoreService.cs | 3 +- .../Services/ServerMetadataService.cs | 8 +- .../Services/ShowPlanParser.cs | 3 +- 16 files changed, 179 insertions(+), 436 deletions(-) delete mode 100644 src/PlanViewer.App/Dialogs/QueryStoreDialog.axaml delete mode 100644 src/PlanViewer.App/Dialogs/QueryStoreDialog.axaml.cs create mode 100644 src/PlanViewer.Cli/ConnectionHelper.cs diff --git a/src/PlanViewer.App/Controls/PlanViewerControl.axaml.cs b/src/PlanViewer.App/Controls/PlanViewerControl.axaml.cs index 60045d0..6851fdb 100644 --- a/src/PlanViewer.App/Controls/PlanViewerControl.axaml.cs +++ b/src/PlanViewer.App/Controls/PlanViewerControl.axaml.cs @@ -90,8 +90,6 @@ public partial class PlanViewerControl : UserControl private static readonly SolidColorBrush OrangeBrush = new(Colors.Orange); - // Current property section for collapsible groups - private StackPanel? _currentPropertySection; // Track all property section grids for synchronized column resize private readonly List _sectionLabelColumns = new(); private double _propertyLabelWidth = 140; @@ -821,7 +819,6 @@ private async System.Threading.Tasks.Task SetClipboardTextAsync(string text) private void ShowPropertiesPanel(PlanNode node) { PropertiesContent.Children.Clear(); - _currentPropertySection = null; _sectionLabelColumns.Clear(); _currentSectionGrid = null; _currentSectionRowIndex = 0; @@ -1772,7 +1769,6 @@ private void AddPropertySection(string title) HorizontalContentAlignment = HorizontalAlignment.Stretch }; PropertiesContent.Children.Add(expander); - _currentPropertySection = null; // No longer used — rows go into _currentSectionGrid } private void AddPropertyRow(string label, string value, bool isCode = false, bool indent = false) @@ -2983,9 +2979,17 @@ private async void SavePlan_Click(object? sender, RoutedEventArgs e) if (file != null) { - await using var stream = await file.OpenWriteAsync(); - await using var writer = new StreamWriter(stream); - await writer.WriteAsync(_currentPlan.RawXml); + try + { + await using var stream = await file.OpenWriteAsync(); + await using var writer = new StreamWriter(stream); + await writer.WriteAsync(_currentPlan.RawXml); + } + catch (Exception ex) + { + System.Diagnostics.Debug.WriteLine($"SavePlan failed: {ex.Message}"); + CostText.Text = $"Save failed: {(ex.Message.Length > 60 ? ex.Message[..60] + "..." : ex.Message)}"; + } } } diff --git a/src/PlanViewer.App/Controls/QuerySessionControl.axaml.cs b/src/PlanViewer.App/Controls/QuerySessionControl.axaml.cs index 8a90a2e..76f6534 100644 --- a/src/PlanViewer.App/Controls/QuerySessionControl.axaml.cs +++ b/src/PlanViewer.App/Controls/QuerySessionControl.axaml.cs @@ -74,6 +74,8 @@ public QuerySessionControl(ICredentialService credentialService, ConnectionStore QueryEditor.TextArea.Focus(); }; + DetachedFromVisualTree += (_, _) => _textMateInstallation?.Dispose(); + // Focus the editor when the Editor tab is selected; toggle plan-dependent buttons SubTabControl.SelectionChanged += (_, _) => { @@ -320,6 +322,7 @@ private string GetTextFromCursor() private void SetStatus(string text, bool autoClear = true) { _statusClearCts?.Cancel(); + _statusClearCts?.Dispose(); StatusText.Text = text; if (autoClear && !string.IsNullOrEmpty(text)) @@ -335,44 +338,7 @@ private void SetStatus(string text, bool autoClear = true) private async void Connect_Click(object? sender, RoutedEventArgs e) { - var dialog = new ConnectionDialog(_credentialService, _connectionStore); - var result = await dialog.ShowDialog(GetParentWindow()); - - if (result == true && dialog.ResultConnection != null) - { - _serverConnection = dialog.ResultConnection; - _selectedDatabase = dialog.ResultDatabase; - _connectionString = _serverConnection.GetConnectionString(_credentialService, _selectedDatabase); - - ServerLabel.Text = _serverConnection.ServerName; - ServerLabel.Foreground = Brushes.LimeGreen; - ConnectButton.Content = "Reconnect"; - - // Populate database dropdown - await PopulateDatabases(); - - // Collect server metadata for advice output - await FetchServerMetadataAsync(); - - // Select the database chosen in the dialog - if (_selectedDatabase != null) - { - for (int i = 0; i < DatabaseBox.Items.Count; i++) - { - if (DatabaseBox.Items[i]?.ToString() == _selectedDatabase) - { - DatabaseBox.SelectedIndex = i; - break; - } - } - } - - // Collect database metadata for the initial database - await FetchDatabaseMetadataAsync(); - - ExecuteButton.IsEnabled = true; - ExecuteEstButton.IsEnabled = true; - } + await ShowConnectionDialogAsync(); } private async Task ShowConnectionDialogAsync() @@ -515,6 +481,7 @@ private async Task CaptureAndShowPlan(bool estimated, string? queryTextOverride } _executionCts?.Cancel(); + _executionCts?.Dispose(); _executionCts = new CancellationTokenSource(); var ct = _executionCts.Token; @@ -900,6 +867,8 @@ private void ClosePlanTab_Click(object? sender, RoutedEventArgs e) { if (sender is Button btn && btn.Tag is TabItem tab) { + if (tab.Content is PlanViewerControl viewer) + viewer.Clear(); SubTabControl.Items.Remove(tab); UpdateCompareButtonState(); } @@ -919,6 +888,8 @@ private void PlanTabContextMenu_Click(object? sender, RoutedEventArgs e) case "Close": if (item.Tag is TabItem tab) { + if (tab.Content is PlanViewerControl closeViewer) + closeViewer.Clear(); SubTabControl.Items.Remove(tab); UpdateCompareButtonState(); } @@ -933,7 +904,11 @@ private void PlanTabContextMenu_Click(object? sender, RoutedEventArgs e) .Where(t => t != keepTab && t.Content is PlanViewerControl) .ToList(); foreach (var t in others) + { + if (t.Content is PlanViewerControl otherViewer) + otherViewer.Clear(); SubTabControl.Items.Remove(t); + } SubTabControl.SelectedItem = keepTab; UpdateCompareButtonState(); } @@ -945,7 +920,11 @@ private void PlanTabContextMenu_Click(object? sender, RoutedEventArgs e) .Where(t => t.Content is PlanViewerControl) .ToList(); foreach (var t in planTabs) + { + if (t.Content is PlanViewerControl allViewer) + allViewer.Clear(); SubTabControl.Items.Remove(t); + } SubTabControl.SelectedIndex = 0; // back to Editor UpdateCompareButtonState(); break; @@ -1339,6 +1318,7 @@ private async void GetActualPlan_Click(object? sender, RoutedEventArgs e) if (!confirmed) return; _executionCts?.Cancel(); + _executionCts?.Dispose(); _executionCts = new CancellationTokenSource(); var ct = _executionCts.Token; diff --git a/src/PlanViewer.App/Controls/QueryStoreGridControl.axaml.cs b/src/PlanViewer.App/Controls/QueryStoreGridControl.axaml.cs index d6fc363..a05ef88 100644 --- a/src/PlanViewer.App/Controls/QueryStoreGridControl.axaml.cs +++ b/src/PlanViewer.App/Controls/QueryStoreGridControl.axaml.cs @@ -59,6 +59,8 @@ private async void QsDatabase_SelectionChanged(object? sender, SelectionChangedE { if (QsDatabaseBox.SelectedItem is not string db || db == _database) return; + _fetchCts?.Cancel(); + // Check if Query Store is enabled on the new database var newConnStr = _serverConnection.GetConnectionString(_credentialService, db); StatusText.Text = "Checking Query Store..."; @@ -92,6 +94,7 @@ private async void QsDatabase_SelectionChanged(object? sender, SelectionChangedE private async void Fetch_Click(object? sender, RoutedEventArgs e) { _fetchCts?.Cancel(); + _fetchCts?.Dispose(); _fetchCts = new CancellationTokenSource(); var ct = _fetchCts.Token; @@ -153,9 +156,15 @@ private async void Fetch_Click(object? sender, RoutedEventArgs e) case "query-id" when long.TryParse(searchValue, out var qid): filter.QueryId = qid; break; + case "query-id": + StatusText.Text = "Invalid Query ID"; + return null; case "plan-id" when long.TryParse(searchValue, out var pid): filter.PlanId = pid; break; + case "plan-id": + StatusText.Text = "Invalid Plan ID"; + return null; case "query-hash": filter.QueryHash = searchValue; break; diff --git a/src/PlanViewer.App/Dialogs/QueryStoreDialog.axaml b/src/PlanViewer.App/Dialogs/QueryStoreDialog.axaml deleted file mode 100644 index 1ccefae..0000000 --- a/src/PlanViewer.App/Dialogs/QueryStoreDialog.axaml +++ /dev/null @@ -1,89 +0,0 @@ - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -