Feature/Allow open/paste execution plans#397
Conversation
… in "Plan Viewer"
Adds a sub-multi tab layer where we can drag and drop or open sqlplan files or paste the XML.
Adds a sub-multi tab layer where we can drag and drop or open sqlplan files or paste the XML.
erikdarlingdata
left a comment
There was a problem hiding this comment.
PR #397 Review — Allow Open/Paste Execution Plans
Nice work, Cláudio! This is a clean, well-organized implementation. Here's my full review:
What works well
- Good UX: The dashed drop-zone border, multi-tab support with unique labels, and "+" add tab pattern are all polished.
- Proper XML validation: Every entry point (Ctrl+V, paste button, file open, drag & drop) validates XML before rendering — no path to crash on bad input.
- Doesn't break existing behavior: The per-server Plan Viewer tab still works exactly as before. The new main-window Plan Viewer is independent.
- Both apps covered: Dashboard and Lite get the same feature with appropriate structural differences.
- Smart empty-tab handling: When all sub-tabs are closed, a fresh empty one re-opens so you never end up staring at just a "+" tab.
Issues to address
1. Missing Ctrl+V on Lite's per-server Plan Viewer empty state
Dashboard adds Ctrl+V support to ServerTab_KeyDown (line ~628-640 in the diff) so you can paste a plan when the Plan Viewer tab is selected but no plan is loaded yet. Lite's ServerTab doesn't have a KeyDown handler at all, so this path is missing. When the Lite per-server Plan Viewer shows the empty "No Plan Loaded" state, Ctrl+V won't work.
This matters less since PlanViewerControl itself handles Ctrl+V once a plan IS loaded, but it's a UX gap for the initial paste.
2. Large code duplication (~400 lines near-identical)
AddNewEmptyPlanSubTab, LoadPlanIntoSubTab, GetUniqueSubTabLabel, GetActivePlanSubTab, IsPlanFile, drag/drop handlers, and KeyDown handlers are copy-pasted between Dashboard/MainWindow.xaml.cs and Lite/MainWindow.xaml.cs. This is consistent with how the codebase already works (Dashboard and Lite are separate projects), so it's acceptable — but worth noting for future maintenance. If we ever extract a shared project, this would be a good candidate.
3. Minor formatting
_mainPlanTabControl = new TabControl
{There's a stray blank line between = new TabControl and { in Dashboard/MainWindow.xaml.cs (around diff line 170-171).
Nice-to-haves (not blocking)
- Multi-file drag & drop: Currently only the first dropped file loads. Supporting multiple files (each in its own sub-tab) would be a natural enhancement but fine for a follow-up.
OpenMainWindowPlanTabis public but uncalled: Both MainWindow classes have this public method, but nothing in this PR calls it. Presumably it's a hook for future use (e.g., routing "View Plan" from a server tab into the main plan viewer). Worth a comment to clarify intent.
Verdict
The Lite Ctrl+V gap (#1) is the only functional issue. Everything else is clean and ready. Would be great to fix #1 and #3, then this is good to merge.
What does this PR do?
Implements #359
This is my suggestion for this. Let me know if it's ok or needs changes.
Which component(s) does this affect?
How was this tested?
Created a new button "🔍 Open Plan Viewer"
When we click on it, it will open the "Plan Viewer" at the same level as "Overview" and "Alert History".
It has, by default, a "New Plan" tab and a "+" in case you want to open others.
There, you'll be able to:
OpenPlanViewer.mp4
Also, the "Plan Viewer" under the server tab is still there as before.
Same screen on Dashboard
Checklist
dotnet build -c Debug)