Release v1.21.0 - #454
Merged
Merged
Conversation
A warning did not record where it came from, so on a large plan there was no way to get from a finding to the thing that produced it. pgfiore's framing was exactly right: "if the plan is huge and the warning origin is murky, it would help to click a warning and be beamed up to a specific step." PlanWarning.OriginNodeIds carries it, and the interesting half of this feature is knowing when to say nothing. Three honest answers, not one: - A key lookup came from exactly one operator. - A table variable warning came from every operator that touched one, which on a real plan is several. That rule already walked the tree and knew precisely which ones; it threw the answer away before emitting. It does not any more. - "High Compile CPU" happened before a single row was read, and SQL Server reports "UDF Execution" at the statement level only. Those have NO operator origin and now say so, so the UI offers no link rather than one that goes somewhere arbitrary. Sending a reader to the wrong operator is worse than sending them nowhere, because they would believe it. Operator warnings are stamped in ONE place, at the end of AnalyzeNode, rather than at the 26 sites that add one - the same reasoning as the provenance stamp in #439 and the ceiling in #438. A rule you have to remember at every construction site is a rule that eventually gets forgotten, and the once it is forgotten the UI quietly drops a link that existed. It only fills what a rule left empty, so a rule that knows better keeps its own answer. The scope call worth reviewing. The pop-up the issue is about shows STATEMENT warnings, and few of those can attribute to an operator - so linking only those would not have served the huge-plan case that motivated the request at all. The warnings that do have origins are the operator ones, and until now the only way to see one was to have already clicked the operator carrying it, which is no help when you do not know which operator to click. So the statement panel also gains an "Operator Warnings" section indexing every warning in the tree, each one a link. Nothing is removed from the per-operator panel; this is an index into it. It is collapsed by default because on a large plan it is the longest section in the panel and expanding it would push the statement's own details off screen, which is the opposite of the problem being solved. The tree walk lives in Core as WarningIndex rather than beside the panel that renders it: it is a walk over Core's own models with nothing UI about it, and there it can be tested without standing up Avalonia. It uses an explicit stack rather than recursion, because a deep plan is precisely the case this feature exists for and #430 was a crash caused by assuming operator trees are shallow. CLI output contract: "origin_node_ids" is additive on every warning, so ExpectedCompactOutputSha256 is rolled - second time, both additive, both deliberate. Verified that origin_node_ids is the ONLY new key rather than assuming it: the full key set on a warning is otherwise unchanged. WarningBaseline.txt does not move. It digests type, severity and message, none of which changed, so no committed plan's verdict is affected. Tested: 314 total, 312 passed, 0 failed. The new tests pin both directions - that across every committed plan no operator warning is left without an origin or points away from its own node, that the table variable warning names the operators that touch one, and that High Compile CPU and UDF Execution claim none. The index is compared against an independent recursive walk rather than a hand-written count, so it cannot drift as fixtures are added. Not verified: the click itself. This box has no reachable display session, so screencapture fails and I could not watch a warning navigate. The app was launched on a plan carrying both kinds of warning and ran clean with no exceptions, and the logic underneath is covered, but someone with a screen should confirm the scroll lands where it should before this is trusted. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…#449) UpdateCompareButtonState counted plans in the session's OWN sub-tabs, so two queries in two separate sessions - one plan each - left the button disabled in both, which is precisely the comparison it exists for. The plans were always reachable. MainWindow.CollectAllPlanTabs spans sessions and labels them "Query 1 > Plan", and the file-mode Compare button has always used it. That is why the reporter's workaround worked: saving a plan and reopening it gave him a file tab, whose button looks at the window-wide collection. Only the session button was looking at the wrong one, in both its enablement and its own narrower picker. So the session button now asks the window for both, and hands off to the window's picker rather than keeping a second one that cannot see past its own session. The refresh is ONE subscription to MainTabControl.Items rather than a call added at each of the sixteen places that add or remove a tab. That is the same reasoning as #438, #439 and #440, and it matters more here than usual: a plan appearing in one session changes whether Compare is available in every OTHER session, so the refresh has to be window-wide and a call site that gets forgotten leaves a stale button somewhere the author never looked. Kept a fallback to the session's own count for when there is no owning MainWindow - the control not yet attached, or hosted somewhere else - so the button is never left in a stale state rather than throwing. Tooltip updated, because it said "Compare two plan tabs" and now means something wider. No automated test, and I would rather say so than pretend. This is UI wiring and the repo has no headless Avalonia harness; a test that would have caught it needs two constructed sessions and a window. Building that harness for one bug is disproportionate, and putting Avalonia into the test process is not something to do casually given the test-host wedge in #441. A test on the arithmetic would be hollow - count >= 2 was never the broken part, the SCOPE was. Verifiable without a SQL Server, which is worth recording because two sessions holding real plans otherwise need a live connection: open two .sqlplan files, then New Query. The session's Compare button is enabled and its picker lists both file plans. On dev it stays disabled, because the session has no plans of its own and IsEnabled="False" is the XAML default. Tested: 314 total, 312 passed, 0 failed - unchanged, since nothing here touches Core. App builds clean and runs without exceptions on two plan tabs. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…#450) A failed query reported an error that was cut off, and it was cut off three separate times on the way to the screen: - ex.Message[..100] + "..." at four sites in Execution.cs. A hundred characters does not even clear "Msg 208, Level 16, State 1, Server X, Line 1" before the sentence naming the actual problem starts, so the truncation reliably removed the only part worth reading. - statusLabel had no TextWrapping, so it defaulted to NoWrap and clipped whatever survived the truncation. - loadingPanel is a fixed Width = 300, sized for a spinner and a Cancel button rather than for prose. Any one of those alone would have cut a real SQL error. Together they made the message close to useless, which matches the report. All four catch sites now go through one helper rather than repeating the display logic - the same reasoning as #438, #439 and #440, and the reason it matters here is that the four sites were already identical and already wrong in the same way, which is what a copied line does over time. The helper widens the panel on failure (MaxWidth rather than Width, so a short error stays compact and a long one is bounded at a readable measure instead of running the whole window), wraps, and colours it as an error. The label is now a SelectableTextBlock. A SQL error is the one string in this app a user most needs to get out and paste somewhere else, and it could not be selected. Verified against a real server rather than by reading: SQL Server 2025 in Docker, and a query against a deliberately long object name produces a 191 character error where the old path stopped mid-word. Not fixed here, and flagged rather than folded in: QueryStore.cs:219 truncates at 80 characters before handing the text to a status bar that already does TextTrimming="CharacterEllipsis". Redundant and lossy, same family, but a different surface and no issue filed against it. No automated test. This is UI wiring and the repo has no headless Avalonia harness; a test that would have caught it needs a constructed control tree. Two bugs in one evening now sit in that gap, which is worth a decision about Avalonia.Headless rather than a hollow test asserting that a string is not truncated by code that no longer truncates it. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both of those were real bugs that no test in this suite could reach, because every
test here works on Core models and the defects were in the UI. Two in one evening
made it a gap worth closing rather than apologising for twice.
Avalonia.Headless.XUnit exists and would have been less code, but at 11.3.20 it
depends on xunit.core 2.4.0 - xunit v2 - and this suite runs xunit.v3. Two xunit
frameworks in one test project to obtain an attribute is a worse trade than owning
fifteen lines, so HeadlessUi drives HeadlessUnitTestSession directly, which is
runner-agnostic and is what that package wraps anyway.
Two things learned the hard way and written into the harness so nobody repeats
them:
- The session runs the REAL App, not a stub. A bare Application looked tidier but
does not load the application XAML, and MainWindow's toolbars resolve styles from
it - FindResource("AppButton") throws without them, which surfaces as an
unrelated-looking "Failed to open" dialog rather than anything resembling a
missing style. App.OnFrameworkInitializationCompleted only creates a window under
a classic desktop lifetime, which a headless session is not, so nothing is
spawned behind the tests.
- One session for the whole assembly, created lazily and never disposed. Avalonia
allows one Application per process, and tearing a session down while another test
class may still be queued is a good way to reintroduce the kind of wedge #441 was
about.
The tests were checked by reverting the fixes, which is the only way to know they
test anything. #448's two - full text, and panel no longer spinner-sized - fail
against the old truncation. #447's fail against the old scope.
Worth recording, because it nearly fooled me: my first attempt at reverting #447
reverted only UpdateCompareButtonState, and all three tests still passed. That was
not the tests being weak - it was the revert being incomplete, because the
window-level subscription was still doing the work. Reverting BOTH halves fails two
of the three, and the one that still passes is the one asserting a single plan is
never enough to compare, which should hold either way. A partial revert is not a
check.
Stability, since putting Avalonia into the test process is exactly the shape of
change that produced #441: three consecutive full runs, 320 passing, 0 failed,
15-16 seconds each, no wedges.
Two entry points became internal for this - MainWindow.LoadPlanFile and
NewQuery_Click, plus QuerySessionControl.ShowExecutionFailure. InternalsVisibleTo
to the test project already existed.
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…452) Same family as #448, different surface. QueryStore.cs cut the message to 80 characters and appended an ellipsis before handing it to StatusText, which already has TextTrimming="CharacterEllipsis". The trimming is the control's job and it does it against the actual available width; doing it again in code threw away text the control would otherwise have kept. It also cost the only way to recover the rest. The bar is one line, so a long message is readable on hover or not at all - SetStatus now sets the tip to the full text, which costs nothing when the message fits and is the difference between a truncated error and a recoverable one when it does not. That applies to every status, not just this one, because every status goes through the same bar with the same one-line constraint. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Minor rather than patch, on the same reasoning as 1.20.0: #440 adds "origin_node_ids" to every warning in the JSON and MCP output, and an output-shape change is not a patch even when it is additive. What ships, all of it reported by users of 1.20.0 in the two days since it went out: - #440 Warnings record which operator produced them. The ones that can be linked are clickable in the app and carry origin_node_ids in JSON; the ones that genuinely have no operator behind them - High Compile CPU happened before a row was read - deliberately have none, because a wrong link is worse than no link. The statement panel also gained an Operator Warnings index, since the warnings worth navigating to were the ones you could previously only reach by already having clicked the operator carrying them. - #447 Compare Plans is offered across query sessions. Two queries in two tabs is the ordinary case and the button was disabled for it, even though the picker behind it could always see both. - #448 A failed query shows its whole error. It was being cut three times over - truncated to 100 characters, clipped by a label that did not wrap, inside a panel fixed at 300px - and the error is now selectable, which matters for the one string in this app people most need to paste elsewhere. Also in, not user-facing: Query Store errors are no longer cut to 80 characters before reaching a status bar that already trims, and the status bar carries the full text as a tooltip so a long message is recoverable rather than lost. And the suite can now test the UI. #447 and #448 were both real bugs that nothing here could reach, so this release also carries a headless Avalonia harness and eight tests over the two defects, each checked by reverting the fix rather than assumed. Tested: 320 passing, 0 failed, across three consecutive full runs. dotnet build clean. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Release v1.21.0. Merging this fires
release.yml: signs the Windows binaries through SignPath and publishes the GitHub release.All three user-facing fixes were reported against 1.20.0 in the two days since it shipped.
User-facing
High Compile CPUhappened before a row was read — deliberately carry no link, because a wrong one is worse than none. The statement panel also gained an Operator Warnings index, since the warnings worth navigating to were previously reachable only by already having clicked the operator carrying them. Addsorigin_node_idsto JSON/MCP output.Also: Query Store errors are no longer cut to 80 characters before reaching a status bar that already trims, and the bar carries the full text as a tooltip.
The suite can test the UI now
#447 and #448 were both real bugs that no test could reach — every test worked on Core models and the defects were in the UI. This release carries a headless Avalonia harness and eight tests over those two defects, each verified by reverting the fix rather than assumed.
Verification
build-and-testgreen onubuntu-latestfor every constituent PR.dotnet buildclean; the 9 warnings are the pre-existingMCP9005obsolete-API uses inMcpSmokeTests.cs.No Windows probe this time:
release.yml's test command is unchanged since v1.20.0 and has now shipped successfully once.Closes on release
#440, #447, #448.
🤖 Generated with Claude Code