Release v1.7.1 — render benefit % and actionable fix#248
Release v1.7.1 — render benefit % and actionable fix#248erikdarlingdata merged 2 commits intomainfrom
Conversation
…246) If signing (or any step after "Create release") fails, the release + tag are created but empty. Previously every downstream step was guarded by EXISTS == 'false', so reruns short-circuited to a no-op and the only fix was to delete the empty release and tag before rerunning. Now only "Create release" itself is guarded; Setup .NET, Build and test, Publish, Sign, Velopack, and Package/upload always run. The final gh release upload uses --clobber, so rerunning against an existing release safely overwrites its assets. Happened during v1.7.0: first attempt timed out on SignPath approval, the rerun skipped all work because v1.7.0 already existed, and manual release+tag deletion was needed to unstick it. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.
What this does
v1.7.1 hotfix. ActionableFix was being dropped on the web warnings strip (Joe's report on v1.7.0); this PR renders it on web, HTML export, Avalonia properties panel, and plain-text output. Also backfills the up to N% benefit badge on the web strip, aligns web warning ordering with TextFormatter, and pulls in the idempotent-rerun workflow change from #246.
What's good
- Rendering change is symmetric across all four output surfaces — text, HTML, web, Avalonia. Previous gap was web-only; this fix covers the set.
- Null-safe guards (
!string.IsNullOrEmpty(w.ActionableFix)) on every surface — no emptyFix:rows when the field isn't populated. - Avalonia addition at
PlanViewerControl.axaml.cs:1757-1766uses the existingTooltipFgBrush(#E4E6EBon dark tooltip bg) — high contrast, no new dim-grey brush introduced. - Web CSS uses existing
--text-secondary/--bordertokens rather than inline colors — no new contrast regressions. gh release upload --clobberandvpk upload --mergemean the workflow's droppedEXISTS == 'false'guards don't break re-runs.- No PlanAnalyzer rule/scoring changes — no Monitor/Lite sync needed.
What needs attention
- No test coverage added.
HtmlExporter.csandTextFormatter.csboth changed insrc/, buttests/PlanViewer.Core.Tests/HtmlExporterTests.csand the text-formatting tests weren't updated. Given the regression being fixed is exactly "output surface dropped a field," a golden-output test on each would be the cheapest insurance. See inline comments. - Sort expression drift between text and web (
Index.razor:335-338vsTextFormatter.cs:166-168). Same result today, opposite ordering direction on the severity key. Easy to diverge on the next change. Inline comment suggests consolidating. - Workflow cost side-effect: removing the
EXISTSguards means every workflow re-run now submits a full SignPath signing request and full Velopack pack, not just the upload. Intentional for the recovery case, worth being explicit about.
Non-issues (pre-checked)
dev → mainbase is correct — the release workflow atrelease.yml:15gates onhead.ref == 'dev', so this is the required pattern for a release PR.- No AvaloniaEdit additions, no new
ComboBox, no new:pointeroverselectors, no MVVM creep. - CSS vars used on web (
--text-secondary: #666666) are darker than the rejected#6B7280/#808080range — contrast is fine.
Generated by Claude Code
| 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)) | ||
| sb.AppendLine($"<span class=\"warn-fix\">{Encode(w.ActionableFix)}</span>"); |
There was a problem hiding this comment.
New warn-fix span branch has no coverage in tests/PlanViewer.Core.Tests/HtmlExporterTests.cs. A snapshot-style assertion that a warning with ActionableFix set emits the span (and one with it null/empty does not) would guard against a regression like the one this PR is fixing.
Generated by Claude Code
| : ""; | ||
| writer.WriteLine($" [{w.Severity}] {w.Type}{benefitTag}: {EscapeNewlines(w.Message)}"); | ||
| if (!string.IsNullOrEmpty(w.ActionableFix)) | ||
| writer.WriteLine($" Fix: {EscapeNewlines(w.ActionableFix)}"); |
There was a problem hiding this comment.
Same gap on the text side — no test in tests/PlanViewer.Core.Tests/ asserts the Fix: ... line renders when ActionableFix is set. Text output is the easiest of the four surfaces to golden-test.
Generated by Claude Code
| @foreach (var w in GetAllWarnings(ActiveStmt!) | ||
| .OrderByDescending(x => x.MaxBenefitPercent ?? -1) | ||
| .ThenByDescending(x => x.Severity == "Critical" ? 3 : x.Severity == "Warning" ? 2 : 1) | ||
| .ThenBy(x => x.Type)) |
There was a problem hiding this comment.
Ordering here mirrors TextFormatter.cs:166-168, which is good — but the secondary key differs in direction. Text uses .ThenBy(w => w.Severity switch { "Critical" => 0, "Warning" => 1, _ => 2 }) (ascending over an ordinal where Critical is lowest). Web uses .ThenByDescending(x => x.Severity == "Critical" ? 3 : x.Severity == "Warning" ? 2 : 1). Both happen to yield Critical → Warning → Info, so behavior matches, but the inverted convention invites drift the next time someone touches one side and not the other. Worth normalizing to a single helper or identical expression.
Generated by Claude Code
| @@ -126,7 +121,6 @@ jobs: | |||
|
|
|||
| # ── Package and upload ──────────────────────────────────────────── | |||
| - name: Package and upload | |||
There was a problem hiding this comment.
Idempotency check: gh release upload ... --clobber (line 184) and vpk upload ... --merge (line 187) handle re-runs against an existing release, so dropping the EXISTS == 'false' guards on the build/sign/package steps is safe from a failure-mode standpoint. The trade-off is that every re-run now submits a fresh SignPath signing request and a full Velopack pack — intentional for the "partial failure on the first run" recovery case, just flagging the cost side for awareness.
Generated by Claude Code
Summary
Hotfix for the rendering gap Joe spotted on v1.7.0: the web warnings strip was dropping `MaxBenefitPercent` and `ActionableFix`. Fixed that, closed the same `ActionableFix` gap on HTML export / Avalonia / plain text, plus the idempotent-rerun workflow fix from PR #246.
See PR #247 and PR #246 for details.
Test plan
🤖 Generated with Claude Code