Skip to content

Fix/query store grid usability#18

Merged
erikdarlingdata merged 10 commits into
erikdarlingdata:devfrom
ClaudioESSilva:fix/query-store-grid-usability
Mar 5, 2026
Merged

Fix/query store grid usability#18
erikdarlingdata merged 10 commits into
erikdarlingdata:devfrom
ClaudioESSilva:fix/query-store-grid-usability

Conversation

@ClaudioESSilva
Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes #14 and #15
Implements #16 and #17

Which component(s) does this affect?

  • Desktop App (PlanViewer.App)
  • Core Library (PlanViewer.Core)
  • CLI Tool (PlanViewer.Cli)
  • SSMS Extension (PlanViewer.Ssms)
  • Tests
  • Documentation

How was this tested?

Open the "Query Store" and navigate in the grid.

Before

image

After

image

Checklist

  • I have read the contributing guide
  • My code builds with zero warnings (dotnet build -c Debug)
  • All tests pass (dotnet test)
  • I have not introduced any hardcoded credentials or server names

Copy link
Copy Markdown
Owner

@erikdarlingdata erikdarlingdata left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

Good contribution — numeric sorting, column filtering, reordering, and horizontal scroll all add real value. Code is clean and well-organized. A few things to address before merge.


🔴 Required Fix: Cross-Platform Icon Font

The filter button uses Segoe Fluent Icons,Segoe MDL2 Assets for the filter icon (\uE71C) and active indicator (\uE16E). This font only exists on Windows. Performance Studio ships for linux-x64, osx-x64, and osx-arm64 — those platforms will show empty squares or nothing.

PerformanceMonitor Lite gets away with Segoe MDL2 because it's WPF (Windows-only). Performance Studio is Avalonia (cross-platform).

What to change:

In QueryStoreGridControl.axaml.cs, method SetColumnFilterButton(), replace:

Text = "\uE71C",
FontFamily = new Avalonia.Media.FontFamily("Segoe Fluent Icons,Segoe MDL2 Assets"),

with a platform-safe Unicode character (no FontFamily needed):

Text = "▽",

In UpdateFilterButtonStyles(), replace:

tb.Text = hasFilter ? "\uE16E" : "\uE71C";

with:

tb.Text = hasFilter ? "▼" : "▽";

The gold color (#FFD700) for active filters is fine — that's just a foreground color and works everywhere.


🔴 Required Fix: LoadSelectedPlans_Click Should Use _filteredRows

SelectAll_Click and SelectNone_Click were correctly updated to operate on _filteredRows, but LoadSelectedPlans_Click still iterates _rows:

// Current (inconsistent):
var selected = _rows.Where(r => r.IsSelected).Select(r => r.Plan).ToList();

// Should be:
var selected = _filteredRows.Where(r => r.IsSelected).Select(r => r.Plan).ToList();

Without this, hidden-but-checked rows get loaded when a user checks rows, filters to hide some, then clicks "Load Selected".


🟡 Suggested: Query Text Column Width

Changed from Width="*" (fills remaining space) to Width="300" MinWidth="150". This means the grid won't fill available width on wide monitors — there will be dead space to the right.

Suggestion: In QueryStoreGridControl.axaml, use Width="*" with MinWidth="300" instead, so the column still stretches but has a guaranteed minimum:

<DataGridTemplateColumn Header="Query Text" Width="*" MinWidth="300">

🟡 Suggested: Remove Dead Code

ColumnFilterState.GetOperatorDisplayName() is defined but never called anywhere. The same display strings already exist in the Operators tuple array in ColumnFilterPopup.cs. Remove the unused method from ColumnFilterState.cs (lines 55-69) to keep things clean.


✅ What's Good

  • SortMemberPath properties — Clean pattern with 16 numeric sort properties exposing raw values while display properties show formatted strings. Exactly right.
  • Unit conversions in NumericAccessors — Filter values match display units (µs→ms for CPU/Duration, pages→MB for memory). Thoughtful.
  • Column index offset — Correctly starts at cols[1], skipping the checkbox column.
  • Filter popup — Reuses app theme resources (BackgroundDarkBrush, BorderBrush, AppButton). Enter/Escape keyboard shortcuts work naturally.
  • Status text — Shows "X / Y plans (filtered)" when filters are active. Nice touch.
  • Filter state UX — Active filters change icon color to gold with tooltip showing the expression. Clear signal.

Summary

Item Severity File(s)
Replace Segoe icon font with Unicode chars 🔴 Required QueryStoreGridControl.axaml.cs
LoadSelectedPlans_Click use _filteredRows 🔴 Required QueryStoreGridControl.axaml.cs
Query Text Width="*" MinWidth="300" 🟡 Suggested QueryStoreGridControl.axaml
Remove unused GetOperatorDisplayName 🟡 Suggested ColumnFilterState.cs

@ClaudioESSilva
Copy link
Copy Markdown
Contributor Author

@erikdarlingdata on the

Query Text Width="*" MinWidth="300"

This will make the grid lose its horizontal scroll bar, and worse than that, will create this problem on "smaller" screens.

image

A Width="*" column always stretches to consume whatever space remains, so the grid's total content width always equals the viewport width — horizontal scrolling never kicks in. With Width="300" the column has a fixed size, so when you expand any column beyond the viewport the scrollbar appears.

So if you agree, we should keep the 300 on the Width and then people can resize any column and use the horizontal scroll bar if needed.

All other 3 fixed.

Copy link
Copy Markdown
Owner

@erikdarlingdata erikdarlingdata left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All four items addressed. Cross-platform icons, _filteredRows fix, dead code removed, and Width=300 rationale makes sense — keeping it. LGTM.

@erikdarlingdata erikdarlingdata merged commit b4e6c9e into erikdarlingdata:dev Mar 5, 2026
1 check failed
erikdarlingdata added a commit that referenced this pull request Apr 9, 2026
- #17: Bump SubTreeCost threshold from 0.01 to 1.0 (Rules 3 & 20) — CTFP is an integer
- #18: Smarter MaxDOPSetToOne severity — check query text for MAXDOP 1, detect truncation
- #19: Already fixed by #20
- #20: Scope "allocated resources" message to Hash/Sort/Spool operators only
- #21: Fix non-SARGable false positive when function is on parameter side
- #22: Enrich parallelism warnings with targeted wait stats advice
- #23: Enrich scan-with-predicate with cost %, elapsed %, row selectivity; elevate to Critical

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
erikdarlingdata added a commit that referenced this pull request Apr 9, 2026
* Address issue #178 round 3 feedback (items 17-22)

- #17: Bump SubTreeCost threshold from 0.01 to 1.0 (Rules 3 & 20) — CTFP is an integer
- #18: Smarter MaxDOPSetToOne severity — check query text for MAXDOP 1, detect truncation
- #19: Already fixed by #20
- #20: Scope "allocated resources" message to Hash/Sort/Spool operators only
- #21: Fix non-SARGable false positive when function is on parameter side
- #22: Enrich parallelism warnings with targeted wait stats advice
- #23: Enrich scan-with-predicate with cost %, elapsed %, row selectivity; elevate to Critical

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* Fix parser skipping statements after DECLARE in multi-statement batches

A Batch element can contain multiple <Statements> children (e.g., one
for DECLARE and one for the SELECT). The parser used .Element() which
only reads the first, causing "no plan loaded" when the actual query
plan was in the second <Statements> block. Changed to .Elements() to
iterate all of them.

Reported by Joe Obbish via email (related to issue #178).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* Show DECLARE/ASSIGN statements in plan viewer with proper icon

Statements with no QueryPlan (like DECLARE/ASSIGN) were filtered from
the statement tab list because they had no RootNode. Now creates a
synthetic root node with the appropriate icon (assign/declare) so they
appear as tabs alongside the actual query plans.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* Add Rules 32 and 33: scan cardinality misestimate and CE guess detection

Rule 32 (actual plans): When an expensive scan (>= 50% of plan) has a
>= 10x row overestimate and < 10% selectivity, flag that the bad
estimate likely caused the optimizer to choose a scan over a seek.

Rule 33 (estimated plans): Detect well-known CE default selectivity
guesses (30%, 10%, 9%, ~16.4%, 1%) on expensive scans against large
tables (>= 100K rows). These patterns suggest the optimizer is using
a default guess instead of accurate statistics.

Addresses issue #178 items #24 and #25 (Joe Obbish feedback).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants