Port Lite chart/tab polish to Dashboard + LSP diagnostics cleanup#863
Conversation
Dashboard polish (ports the same items merged to Lite in #862): - New Dashboard/Helpers/AxesExtensions.cs with DateTimeTicksBottomDateChange(), culture-aware (dd/MM for en-GB, dd.MM for de-DE, 24h clocks, etc.). All 52 call sites of DateTimeTicksBottom() across 10 files swapped to use it. - TabHelpers.ApplyTheme + ReapplyAxisColors bump chart tick label font from 12 to 13 so numbers read cleaner on wide charts. - SubTabItemStyle added to Dark / Light / CoolBreeze themes: thin accent underline + transparent background instead of filled cyan, so sub-tabs don't look identical to main tabs when selected. Wired via ItemContainerStyle on 11 sub-TabControls (Overview's inner tabs, Collection Health's inner tabs, Locking, ConfigChanges, CurrentConfig, FinOps, Memory, ResourceMetrics ×2, SystemEvents, QueryPerformance). LSP diagnostics cleanup (tracked work from chore/lsp-diagnostics-cleanup): - Small nullability/warning fixes across Dashboard and Lite services, analysis helpers, and BenefitScorer / PlanAnalyzer. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
erikdarlingdata
left a comment
There was a problem hiding this comment.
Review summary
What this does
Ports the chart-axis + sub-tab polish from Lite PR #862 into Dashboard, plus a round of LSP/nullability cleanup across both apps.
- New
Dashboard/Helpers/AxesExtensions.csmirroring the Lite helper from #862; 52 call sites ofDateTimeTicksBottom()swapped to the newDateTimeTicksBottomDateChange()across 10 Dashboard files. SubTabItemStyleadded to Dark / Light / CoolBreeze themes; wired viaItemContainerStyleon 11 sub-TabControls inServerTab.xaml,MemoryContent.xaml,QueryPerformanceContent.xaml,ResourceMetricsContent.xaml,SystemEventsContent.xaml,ConfigChangesContent.xaml,CurrentConfigContent.xaml,FinOpsContent.xaml.- Tick font 12 → 13 in
Dashboard/Helpers/TabHelpers.cs:203,205,239,241. - Null-guards and
StringComparison.Ordinalfixes acrossPlanAnalyzer,BenefitScorer,SqlServerBaselineProvider,CorrelatedCrosshairManager,RemoteCollectorService.QueryStore.
Good
- Base branch is
dev. ✓ - Lite-first ordering honored — Dashboard catches up to Lite, not the other way around. ✓
PlanAnalyzer.csrule bodies (Rule 12hasRecompile, Rule 28Row Count Spool, implicit-conversion warning promotion) changed identically in both Dashboard and Lite copies. ✓BenefitScorer.csStringComparison.Ordinalcleanup is byte-for-byte identical across both copies. ✓- No SQL / schema changes — no upgrade-script concerns.
- No
.github/workflows/build.ymlchanges — SignPath stays ontest-signing.
Needs attention
- PlanAnalyzer sync drift (minor):
Dashboard/Services/PlanAnalyzer.cs:1831is nowprivate sealed record ScanImpact(...)butLite/Services/PlanAnalyzer.cs:1828is stillprivate record ScanImpact(...). Mirror thesealedchange into Lite (or drop it from Dashboard) to keep the two copies aligned per the sync rule. Inline comment left. AxesExtensions.MonthDayPatternis culture-snapshot-once at static init — stale if the UI culture changes at runtime. Inherited from Lite's #862 copy, not introduced here, but worth a follow-up. Inline comment left.AxesExtensions.DateTimeTicksBottomDateChangerelies on the tick formatter being invoked in tick order. Fragile if ScottPlot reorders calls during pan/zoom redraws. Worth a manual pan+zoom verification on a chart crossing midnight. Inline comment left.RemoteCollectorService.QueryStore.cs:225null-propagation silently downgrades engine detection whenserverStatusis null. Matches line 33's pattern, so consistent — but see inline comment for the tradeoff.- No tests touched.
AxesExtensions.BuildMonthDayPatternis a pure string function and would be trivial to cover inDashboard.Tests/Lite.Tests(en-US, en-GB, de-DE, ja-JP at minimum). Consider adding a small fixture.
Not blocking anything — comments only.
Generated by Claude Code
| } | ||
|
|
||
| private record ScanImpact(double CostPct, double ElapsedPct, string? Summary); | ||
| private sealed record ScanImpact(double CostPct, double ElapsedPct, string? Summary); |
There was a problem hiding this comment.
ScanImpact was changed to sealed record here, but the mirrored Lite/Services/PlanAnalyzer.cs:1828 still has private record ScanImpact(...) (unsealed). Per the PlanAnalyzer sync rule, either seal it in Lite too or leave both alone — the two copies should stay byte-for-byte equivalent (minus namespace/using).
Generated by Claude Code
| { | ||
| /// <summary>Culture's short-date pattern with the year component removed (e.g. "M/d" en-US, "dd/MM" en-GB, "dd.MM" de-DE).</summary> | ||
| private static readonly string MonthDayPattern = BuildMonthDayPattern(); | ||
|
|
There was a problem hiding this comment.
MonthDayPattern is a static readonly resolved once at type-init against CultureInfo.CurrentCulture. If the user changes locale mid-session (or the UI thread runs under a different culture than the static init thread), the pattern will be stale. Not a blocker — this mirrors Lite's copy from #862 — but worth a TODO to either recompute per-call or cache by culture.
Generated by Claude Code
| var time = dt.ToString("t", culture); | ||
| if (lastDate is null || dt.Date != lastDate.Value) | ||
| { | ||
| lastDate = dt.Date; |
There was a problem hiding this comment.
The lastDate closure variable assumes the LabelFormatter delegate is invoked in left-to-right tick order within a render pass. If ScottPlot ever reorders tick-label resolution (zoom/pan/redraw), date labels can print on the wrong ticks or the first tick may lose its date line on redraw. Worth verifying with a pan+zoom test in the test plan — again, inherited from Lite, so fine to port as-is, but a known fragility.
Generated by Claude Code
| } | ||
|
|
||
| bool isNew = productVersion > 13 || serverStatus.SqlEngineEdition == 5 || serverStatus.SqlEngineEdition == 8; | ||
| bool isNew = productVersion > 13 || serverStatus?.SqlEngineEdition == 5 || serverStatus?.SqlEngineEdition == 8; |
There was a problem hiding this comment.
Behavior change: when serverStatus is null, isNew now silently falls through to productVersion > 13. That matches line 33's null-propagation pattern, so it's consistent — but note that a null serverStatus on Azure SQL DB (engine 5) previously threw NRE, and now quietly reports isNew=false if the product-version probe also failed. If serverStatus is never expected to be null post-connect, an early guard upstream would express that intent more clearly than silently downgrading the engine detection.
Generated by Claude Code
Summary
Two pieces of work bundled on the
chore/lsp-diagnostics-cleanupbranch:Dashboard polish (ports items already merged to Lite in #862)
Dashboard/Helpers/AxesExtensions.cswithDateTimeTicksBottomDateChange()— culture-aware (en-GBdd/MM, de-DEdd.MM, 24-hour clocks, etc.). All 52 call sites ofDateTimeTicksBottom()across 10 Dashboard files swapped to use it: dates now print on the first tick and on rollover ticks only; time-only on the rest.TabHelpers.ApplyThemeandReapplyAxisColorsfor readability.SubTabItemStyleadded to all three themes (Dark / Light / CoolBreeze): thin accent underline + transparent background instead of filled cyan. Wired viaItemContainerStyleon 11 sub-TabControls: Overview's inner tabs, Collection Health's inner tabs, Locking, ConfigChanges, CurrentConfig, FinOps, Memory, ResourceMetrics (×2), SystemEvents, QueryPerformance.LSP diagnostics cleanup
BenefitScorer/PlanAnalyzer.Test plan
dotnet buildclean with no regressions from the LSP cleanup edits🤖 Generated with Claude Code