Skip to content

Fix code review findings across Core, CLI, and UI #97

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
erikdarlingdata merged 1 commit into from
Mar 17, 2026
Merged

Fix code review findings across Core, CLI, and UI #97

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 11 additions & 7 deletions src/PlanViewer.App/Controls/PlanViewerControl.axaml.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<ColumnDefinition> _sectionLabelColumns = new();
private double _propertyLabelWidth = 140;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)}";
}
}
}

Expand Down
56 changes: 18 additions & 38 deletions src/PlanViewer.App/Controls/QuerySessionControl.axaml.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 += (_, _) =>
{
Expand Down Expand Up @@ -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))
Expand All @@ -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<bool?>(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()
Expand Down Expand Up @@ -515,6 +481,7 @@ private async Task CaptureAndShowPlan(bool estimated, string? queryTextOverride
}

_executionCts?.Cancel();
_executionCts?.Dispose();
_executionCts = new CancellationTokenSource();
var ct = _executionCts.Token;

Expand Down Expand Up @@ -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();
}
Expand All @@ -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();
}
Expand All @@ -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();
}
Expand All @@ -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;
Expand Down Expand Up @@ -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;

Expand Down
9 changes: 9 additions & 0 deletions src/PlanViewer.App/Controls/QueryStoreGridControl.axaml.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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...";
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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;
Expand Down
89 changes: 0 additions & 89 deletions src/PlanViewer.App/Dialogs/QueryStoreDialog.axaml
View file
Open in desktop

This file was deleted.

Loading
Loading