Render benefit % and actionable fix in all warning surfaces (v1.7.1)#247
Conversation
Web viewer warnings strip was dropping both MaxBenefitPercent and ActionableFix — data made it through ResultMapper into the JSON but the strip only rendered Severity/Type/Message. Joe caught this on v1.7.0 (wait stat benefit missing from the rendered warning). While in the area, closed the same ActionableFix gap in the other surfaces that already render benefit: - Web viewer (Index.razor + app.css): add .warn-benefit badge + .warning-fix italic block. Sort strip by benefit desc, then severity, then type to match HTML export / Avalonia ordering. - HtmlExporter: append .warn-fix block after .warn-msg, style added. - PlanViewerControl (Avalonia): italic TextBlock under the message when ActionableFix is present. - TextFormatter: indented "Fix:" line after each warning. Version bump 1.7.0 -> 1.7.1. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
erikdarlingdata
left a comment
There was a problem hiding this comment.
Summary
Fixes the v1.7.0 regression where MaxBenefitPercent and ActionableFix were dropped from the web viewer's warnings strip, and closes the matching ActionableFix gap in the HTML export, Avalonia properties panel, and text formatter. Version bumped 1.7.0 → 1.7.1. Rendering-only change — no PlanAnalyzer rule or scoring changes, so no PerformanceMonitor/Dashboard/Lite sync needed.
Good
- Base branch is
dev✓ - No MVVM creep in the Avalonia path — still pure code-behind, new
TextBlockuses existingTooltipFgBrush(#E4E6EB), not in the rejected#6B7280/#808080range. - Sort order for the web strip (
benefit desc → severity desc → type asc, Index.razor:336) produces Critical-first and matches HtmlExporter.cs:446 and PlanViewerControl.axaml.cs:1730. Parity restored across all three surfaces. - Reuses existing CSS variables (
--text-muted,--text-secondary) rather than introducing new inline colors.
Needs attention
- Class naming inconsistency in Index.razor:349 — new
warn-benefitspan uses short prefix while all neighboring classes (warning-op,warning-type,warning-msg, newwarning-fix) use the fullwarning-prefix. Minor but jarring. See inline. - Low-contrast quote border on the fix block —
border-left: 2px solid var(--border)where--border: #e0e0e0will vanish on light surface. Same issue in HtmlExporter.cs:190. See inline. - No test coverage added.
HtmlExporterTests.csexists and wouldn't need much to assert the new spans emit;TextFormatterTests.csdoesn't exist at all. Given this PR is explicitly a "silently dropped field" regression fix, a contains-assertion is the cheapest insurance against it happening again. See inline.
Not blocking, just noting
- PR test plan still has the web + Avalonia visual checks unticked. Worth confirming those before shipping v1.7.1.
Generated by Claude Code
| <span class="warning-type">@w.Type</span> | ||
| @if (w.MaxBenefitPercent.HasValue) | ||
| { | ||
| <span class="warn-benefit">up to @w.MaxBenefitPercent.Value.ToString("N0")% benefit</span> |
There was a problem hiding this comment.
Class-name nit: the neighboring spans in this strip are severity, warning-op, warning-type, warning-msg (full warning- prefix), but this PR introduces warn-benefit (short prefix) alongside warning-fix (full prefix) at line 354. The short form matches the HtmlExporter copy but is inconsistent with every other class in this Razor block. Consider renaming to warning-benefit for consistency with the siblings in Index.razor — HtmlExporter.cs is a separate CSS namespace and doesn't need to match.
Generated by Claude Code
| margin-top: 0.25rem; | ||
| font-size: 0.75rem; | ||
| font-style: italic; | ||
| border-left: 2px solid var(--border); |
There was a problem hiding this comment.
--border: #e0e0e0 (defined at line 15) on the default light surface is barely visible — a 2px vertical rule at that color on white/near-white will effectively disappear, defeating the purpose of the quote-style indent. The HtmlExporter copy (src/PlanViewer.Core/Output/HtmlExporter.cs line 190) uses the same var(--border) and has the same issue. Consider var(--text-muted) for the border so the fix block actually reads as a distinct quoted region.
Generated by Claude Code
| if (w.MaxBenefitPercent.HasValue) | ||
| sb.AppendLine($"<span class=\"warn-benefit\">up to {w.MaxBenefitPercent:N0}% benefit</span>"); | ||
| sb.AppendLine($"<span class=\"warn-msg\">{Encode(w.Message)}</span>"); | ||
| if (!string.IsNullOrEmpty(w.ActionableFix)) |
There was a problem hiding this comment.
No test coverage added for the new warn-benefit / warn-fix spans. tests/PlanViewer.Core.Tests/HtmlExporterTests.cs already exercises Export_ProducesValidHtml_WithWarnings using key_lookup_plan.sqlplan; a one-liner Assert.Contains("warn-benefit", html) (and "warn-fix" if that fixture produces an ActionableFix) would lock in this regression — the whole reason for this PR is the strip silently dropped these fields.
Generated by Claude Code
| ? $" (up to {w.MaxBenefitPercent:N0}% benefit)" | ||
| : ""; | ||
| writer.WriteLine($" [{w.Severity}] {w.Type}{benefitTag}: {EscapeNewlines(w.Message)}"); | ||
| if (!string.IsNullOrEmpty(w.ActionableFix)) |
There was a problem hiding this comment.
No TextFormatterTests.cs exists under tests/PlanViewer.Core.Tests/ — the text output is effectively untested. Not a blocker for this PR, but a TextFormatter_Warning_IncludesBenefitAndFix test would be cheap insurance for the output the CLI/MCP surface depends on.
Generated by Claude Code
| Foreground = TooltipFgBrush, | ||
| TextWrapping = TextWrapping.Wrap, | ||
| Margin = new Thickness(16, 2, 0, 0) | ||
| }); |
There was a problem hiding this comment.
Confirmed TooltipFgBrush = #E4E6EB (line 90) — fine on the dark properties-panel surface, well out of the rejected #6B7280/#808080 dim range. Left margin (16) matches the message block above, indent reads correctly. No concerns here — leaving this so the inline record matches the summary.
Generated by Claude Code
Summary
Web viewer warnings strip was dropping `MaxBenefitPercent` and `ActionableFix` from every warning — the data flowed through `ResultMapper` into the JSON but the strip only rendered Severity/Type/Message. Joe caught the missing benefit % on v1.7.0 wait warnings.
Fixed the strip plus the same `ActionableFix` gap in the other surfaces that were already showing benefit (HTML export, Avalonia, plain text).
Changes
Index.razor+app.css):.warn-benefitbadge + italic.warning-fixblock. Strip now sorts by benefit desc, then severity, then type — matches HTML export and Avalonia..warn-fixblock after.warn-msg.PlanViewerControl): italic TextBlock under the message whenActionableFixis present.Test plan
🤖 Generated with Claude Code