Say why the card is amber, and give "+52 more" somewhere to go (#2424) - #2429
Conversation
Both halves of #2422 are places where the Overview computes the answer and then declines to render it, and on the fleet size where this tab earns its keep they compound. The reporter had 57 servers, read "Warning" in amber on the first card and asked what it was warning him about, then read "+52 more need attention" and asked where he could find them. The reason was already built. FleetRollup.BuildReason turns a card into "CPU 94%, Blocking 12" out of that card's OWN metric displays -- deliberately, per its own comment, so it can never drift from what the card shows -- and it was spent entirely on the five-row Needs Attention ranking. The card, which is the thing being looked at, did not get it. The status line carries it now as a ToolTip, in the shape MainWindow.AlertBadges.cs already established for the sidebar badge: the breakdown, then how to act on it. It is BuildReason's output verbatim rather than a second derivation, because a second derivation would eventually disagree with the six metric rows sitting directly underneath it, and that is precisely the failure BuildReason exists to prevent. Two arms sit on top of it. A healthy card gets an all-clear instead of BuildReason's "Needs attention" fallback, which is written for a ranking that only ever holds problem servers and on a grid showing every server would tell each green card the opposite of the truth. Offline and awaiting-first-collection are not labelled twice, their reason already being a whole sentence that names the state. Background="Transparent" on that TextBlock is load-bearing rather than decorative: a TextBlock with a null Background hit-tests on its rendered glyphs alone, so the tooltip would have appeared over the letters of "Warning" and nowhere in the space around them. Worth naming, because it is the reporter's exact card. StatusDisplay renders "Warning" for one condition only -- an online server whose collection has gone stale -- and nothing on the card said so, while the amber reads naturally as a metric problem. That card's tooltip now says "Warning - collection stale". "+52 more need attention" is the harder half, because the honest answer to "where do I find these warnings?" was nowhere. FleetRollup computes the full problem set and truncates it for display, so those 52 were discarded rather than unavailable. The line is now a link into a needs-attention filter over the card grid, which is the option the issue ranked first and the one that actually answers the question, since the grid is where the six metric rows live. It filters on FleetRollup.NeedsAttention -- the same banding that counted the 52 -- so the destination cannot disagree with the label that sent you there. The filter is a toggle beside the existing Sort selector rather than a mode the link switches on invisibly, and the placement is the point: the roll-up header is docked to the top of the Overview and never scrolls, so a filter that is on can never be off screen while the grid it shrank is on it, and "showing 12 of 57" renders beside the toggle whenever it is active. A filtered list that looks unfiltered is a worse defect than the dead end it replaces. It is not persisted the way the sort mode is, because a sort is a preference and this is a triage action tied to a moment; an Overview that opens with 52 of 57 servers already hidden is a support ticket even with the box in plain sight. The cap of five stays, deliberately. It hides 52 servers on this fleet, which is the complaint, but the ranking renders inside that same non-scrolling docked panel: a list that grew with the fleet would push the cards it is pointing at off the screen, and would stop being a triage shortlist and become a second, worse copy of the grid without the metrics. The repair for an overflow is a destination, not a longer list, and it now has one. One structural consequence is worth flagging. OverviewItemsControl.ItemsSource was read back in two places to answer "what cards are there" -- the re-sort and the tag re-stamp -- which is safe only while the bound list is the complete list. That is the exact mistake FleetView was written to make impossible on the sidebar, so the full sorted set becomes the authority here too and the grid is projected from it through the filter in one place. Sorting the bound list under an active filter would have silently discarded every card the filter was hiding. The tests cover the tooltip's reuse of BuildReason, the healthy and offline arms, the filter agreeing with the roll-up's own overflow count, and the cap. The wiring half text-scans source, because a ToolTip attribute and a click handler live in XAML where no assertion about a C# object can reach them and removing either compiles perfectly clean. Fixes #2424 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| /// <para>Delegated rather than reimplemented on purpose: two independent derivations of "why is this amber" | ||
| /// would eventually disagree, and the one place they would disagree is a card the reader is staring at.</para> | ||
| /// </summary> | ||
| public string StatusTooltip => FleetRollup.BuildStatusTooltip(this); |
There was a problem hiding this comment.
Minor/non-blocking: StatusDisplay has an Unknown arm (IsOnline == null && !AwaitingFirstCollection, line 569) that BuildStatusTooltip/ClassifyBand don't special-case. For that state, ClassifyBand falls through to OverallMetricSeverity/HasCollectorErrors, so on an otherwise-clean summary the tooltip would read "Healthy — every metric on this card is inside its threshold" right next to a status word reading "Unknown" — the two would say opposite things.
Today this looks unreachable in practice: ApplyFreshness (the only place that sets IsOnline/AwaitingFirstCollection for Overview cards) always leaves the pair in one of the two handled states, never in the Unknown combination. But since StatusDisplay already carries a dedicated branch for it, it might be worth a one-line Unknown arm in DescribeBand/BuildStatusTooltip too, so the two stay in lockstep if a future caller ever constructs a summary without going through ApplyFreshness (e.g., a test fixture, or a new data path).
There was a problem hiding this comment.
Taken — 44ea480. You are right that ApplyFreshness cannot produce that pair today, and right that it is worth an arm anyway: the guard costs one branch, and the state it guards is one a fixture or a new data path can construct without going near ApplyFreshness, at which point the contradiction ships silently because nothing fails.
DescribeBand becomes Headline, since it is no longer describing a band — it answers "what does this card's status word mean", with the band as one of the answers. The unknown card still gets its reason when there is one: not knowing whether a server is reporting is no argument for withholding the CPU number that was collected, so an unknown card with a bad metric now reads Unknown — no collection status for this server; CPU 96%, and the healthy-card guard still sits underneath so the ranking's "Needs attention" fallback cannot leak through this arm either. Pinned by TheCardsTooltip_NeverContradictsAnUnknownStatus.
ReviewScope: C#/XAML only ( Lite/Darling parity: No drift. Correctness: Traced the two new code paths end to end against
Left one non-blocking inline note on a latent (currently unreachable) inconsistency between Security/perf: No I/O, no new external input parsing, no secrets. Filter/sort are O(n) in-memory list operations over per-fleet-refresh data — no regression risk at realistic fleet sizes. No blocking issues found. |
Review on #2429 caught the one state where the new tooltip could contradict the word it hangs off. StatusDisplay renders "Unknown" when IsOnline is null and nothing marks the card as awaiting its first collection, and ClassifyBand has no such arm -- it falls straight through to the metrics, so on an otherwise clean card the tooltip read "Healthy - every metric on this card is inside its threshold" beside a status word reading "Unknown". Two surfaces, one pixel apart, saying opposite things, which is a smaller copy of the defect this branch exists to remove rather than a different one. It is not reachable today. ApplyFreshness is the only thing that sets that pair on an Overview card, and its four arms never leave a card there. That is the argument for pinning it rather than filing it: the cost of the guard is one branch, and the thing being guarded is a state a test fixture or a future data path can construct without going anywhere near ApplyFreshness -- at which point the contradiction ships silently, because nothing would fail. The unknown card still gets its reason when there is one. Not knowing whether a server is reporting is no argument for withholding the CPU number that WAS collected, so a card in that state with a bad metric reads "Unknown - no collection status for this server; CPU 96%". The healthy-card guard still applies underneath it, so the ranking's "Needs attention" fallback cannot leak onto a card through this arm either. DescribeBand becomes Headline in the process, because it is no longer describing a band -- it is answering "what does this card's status word mean", with the band as one of the answers. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Reviewed the diff (Darling-only: Correctness: Traced the new Lite/Darling parity: No drift from this PR. The fleet-wide rollup ( Style/other: New file has the copyright header; XML docs present on all new public members; no T-SQL touched so the T-SQL style rules don't apply here. One tiny non-blocking nit: No blocking issues. |
Second review pass on #2429 caught that OverviewAttentionCountText paints all three of its sentences amber, and one of them is an all-clear. That line says either "showing 12 of 57" -- servers wanting attention, amber is right -- or "all 57 servers are healthy", where amber is a colour contradicting its own text. It is cosmetic, and it is also a small copy of the thing this branch exists to fix: a card that showed a colour and left the reader to guess what it was claiming. The brush is set through SetResourceReference rather than assigned, which is the alert badge's idiom in MainWindow.AlertBadges.cs, so the line still follows a theme change instead of freezing whichever palette happened to be loaded when the filter was switched on. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Both notes from the second pass are settled. The On Lite: agreed, and confirmed independently — |
| { | ||
| /* Offline and awaiting-first-collection come back from BuildReason as whole sentences that already | ||
| name the state, so putting a band label in front would only say "Offline" twice. */ | ||
| if (s.IsOnline == false || s.AwaitingFirstCollection) |
There was a problem hiding this comment.
This guard closes the IsOnline == null && !AwaitingFirstCollection contradiction (the one raised in the prior review round and pinned by TheCardsTooltip_NeverContradictsAnUnknownStatus), but there's a sibling gap it doesn't close: IsOnline == true && AwaitingFirstCollection == true.
AwaitingFirstCollection is a plain mutable { get; set; } (ViewerDataService.Overview.cs:389), so nothing stops a fixture or a future data path from setting it alongside IsOnline = true, just as nothing stopped the null/false combo this commit already guards against. If that happens:
StatusDisplaymatches onIsOnlinealone (true when HasCollectorErrors => "Warning",true => "Online") —AwaitingFirstCollectionnever enters that switch, so it reads "Online".Headlinehere short-circuits ons.IsOnline == false || s.AwaitingFirstCollection— the second half fires regardless ofIsOnline, soBuildReasonreturns "Awaiting first collection".
The card would show green "Online" with a tooltip reading "Awaiting first collection — Double-click the card to open this server's tab", which is exactly the class of contradiction this PR (and the prior review round's fix) exists to remove — just a different unreached-today combination than the one already pinned.
Worth mirroring StatusDisplay's own structure here: only consult AwaitingFirstCollection inside the IsOnline is null branch (where it's actually meaningful per ApplyFreshness), the same way the switch does, rather than checking it independently of IsOnline up front.
There was a problem hiding this comment.
Right, and taken further than the note — ae87120.
You are the second round to find a different pair of the same two flags disagreeing, and that is the point where it stops being a bug and starts being a category: the word, the colour, BuildReason and the tooltip were four independent readings of one settable pair, and nothing forced any of them to agree. Mirroring StatusDisplay's structure in Headline would have fixed your instance and left the next one to be found the same way.
So they now share a discriminant. ServerCardStatus is derived in exactly one place from the flag pair, and StatusDisplay / StatusBrush / BuildReason / the tooltip headline are renderings of it. Every rendered string and colour is byte-identical, so the existing StatusDisplay pins carry over untouched.
Then I wrote the test as a sweep over the whole product instead of a third guard, and it found two more nobody had reported: ClassifyBand was still reading AwaitingFirstCollection raw, so an online card with a stray marker banded Warning with nothing to report and rendered Warning — Needs attention; and the Unknown arm appended the reason unguarded, so a card outside Healthy with nothing nameable leaked the ranking-only fallback. Both go through one WithReason helper now, and the fallback string is a named constant.
The sweep walks 96 cards (three IsOnline values against awaiting, stale, a CPU number, a blocking max-wait and a failing collector) and asserts no tooltip names a state its own status word contradicts, the ranking fallback never reaches a card, and all five status words were actually exercised. The blocking dimension is in there because widening the sweep is what found it — a long max-wait with a zero event count bands Blocking Warning while BuildReason's gate wants a non-zero count.
|
Reviewed the diff (Darling.Tests + MainWindow.xaml/.xaml.cs + ViewerDataService.Fleet.cs/.Overview.cs). Overall this is well-reasoned and well-tested — the One remaining gap left an inline comment on ( Lite/Darling parity note (not a regression from this PR, flagging per review scope): Lite's No T-SQL in this change, so the collector-query style rules don't apply. No security-relevant surface (no new I/O, no untrusted input parsing) — this is pure client-side view-model/filter logic over already-loaded data. |
Two review rounds on #2429 each found a different pair of freshness flags where the card's status word and its tooltip said opposite things. The first was IsOnline null with no awaiting marker; the second was IsOnline TRUE with one, which would have put a green "Online" over a tooltip reading "Awaiting first collection". Both got guarded where they were found. That is the point at which the second instance stops being a bug and starts being a category: the word, the colour, the ranking's reason and the tooltip were four independent readings of the same two settable properties, and nothing forced any of them to agree. So they now share one discriminant. ServerCardStatus is derived in exactly one place, from the flag pair, and StatusDisplay, StatusBrush, BuildReason and the tooltip's headline are renderings OF IT rather than four opinions about it. Every rendered string and every colour is byte-for-byte what it was, so the existing StatusDisplay pins carry over untouched -- what changed is that there is no longer a combination left for two of them to disagree about, because they no longer each decide. Then the test was written as a sweep over the whole product of the flags rather than as a third guard, and it immediately found two more instances that nobody had reported. ClassifyBand was still reading AwaitingFirstCollection raw, so an online card carrying a stray marker banded Warning with nothing to report and rendered "Warning - Needs attention". And the Unknown arm appended the reason unguarded, which is the same append the band arm had already learned to guard -- so a card outside Healthy with nothing nameable leaked the ranking-only fallback onto a tooltip. Both now go through one WithReason helper that degrades to the headline alone, and BuildReason's fallback is a named constant so the two sides cannot drift on its spelling. That is four instances of one category, two of them found by the sweep rather than by a reviewer, which is the argument for the sweep. It walks 96 cards -- the three IsOnline values against awaiting, stale, a CPU number, a blocking max-wait and a failing collector -- and asserts that no tooltip names a state its own status word contradicts, that the ranking's fallback never reaches a card, and that all five status words were actually exercised, so a later rewrite that collapses one of them cannot leave the sweep passing over a smaller surface than it claims. The blocking dimension is in there because widening the sweep is what found it: a long max-wait with a zero event count bands Blocking as Warning while BuildReason's own gate wants a non-zero count. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ReviewRe-reviewed the latest diff ( Correctness: Traced
No new bugs found beyond what the last two review rounds already caught and the author fixed (the Lite/Darling parity: No drift introduced by this PR. Confirmed independently: Lite's Security/perf: No I/O, no external input, no secrets. Filter/sort/tooltip-build are O(n) in-memory operations over already-loaded per-refresh fleet data — no regression risk at realistic fleet sizes. No T-SQL in this change, so the collector-query style rules don't apply. No blocking issues. |
Review caught this and it is right, and it is worse than the doc-accuracy nit it was raised as. ServerCardStatus's comment claimed the (IsOnline, HasCollectorErrors) pair was read in one place; CardBorderBrush read IsOffline and HasCollectorErrors raw, so it agreed with the status word by coincidence rather than by construction. It already disagreed on one pair. A card that has never been connection-checked but carries a collector-error marker draws the amber "collectors failing" border -- verified against dev's own copy of the class, which returns #FFD54F there -- while its word reads "Unknown" and its tooltip says the server has not been checked yet. That is precisely the desync family this change exists to close, sitting in the one member it had not reached. The Overview loader only sets HasCollectorErrors when the connection check succeeded, so the pair is unreached in practice. That is the argument for making it unrepresentable rather than for leaving it alone: both flags are plain settable properties, and "no caller does this today" is what the viewer's two review rounds on #2429 each disproved. So the border reads CardStatus, and so does IsOffline, which the offline overlay binds. Precedence is unchanged -- a dark server, then the metric rows worst-first, then failing collectors -- and the amber border is now literally the same amber the status word is painted, because both render one state. The claim in the doc comment is now true, and a test asserts it rather than leaving it as prose for the next reviewer to re-check by hand: exactly one `IsOnline switch` in the file. That is the assertion that would have caught the overclaim when it was written. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both from review, and the first one is this PR's own defect turned on itself. The notice had two colours and three sentences. A search term that matches nothing leaves the attention filter with nothing to judge, so shown was 0, so it took the green all-clear arm and read "Needs attention only, matching "xyz" -- no servers to filter." in the colour that means everything is fine. The fleet's problem servers were not found healthy; they were never looked at. That is a colour contradicting its own text, which is the exact finding #2429's review raised about the amber arm and which I ported without noticing it had a third case. So the notice has three states. Amber is a count of servers wanting attention, green is a real all-clear over a real result set, and neutral is "nothing matches that term, so no server was judged" -- which says what happened instead of implying a verdict. The grid line underneath already named the search; the notice above it was the one still claiming health. Second: the notice appears and re-words itself with no page load, and carried no role="status". util.js's noticeStrip is the house idiom for exactly that kind of non-fatal live notice, so a screen reader now hears the count change rather than the grid silently shrinking. It matters more here than usual -- this PR is otherwise deliberate about the keyboard path, using onActivate on both affordances so they are reachable at all. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ng green Lite's Overview card carried five metric rows and a status word and none of them was collection freshness. The failure mode that leaves is not exotic: the connection check keeps passing, no collector is *erroring*, and the store takes no new rows for hours. IsOnline comes from ServerManager.GetConnectionStatus -- a live check that succeeds whether or not anything is being collected -- and HasCollectorErrors counts collectors with ConsecutiveErrors > 0, which a collector that has stopped being scheduled never increments. So the card showed a green "Online", a neutral border, and a "Last Collect" timestamp painted in the plain foreground brush, where four hours ago looked exactly like four seconds ago. Lite reaches that state through its own shipped code. A collector answering PERMISSIONS sets IsPermissionRestricted and RunCollectorAsync returns BEFORE LogCollectionAsync, so it writes no collection_log row at all -- and the PERMISSIONS arm of RecordCollectorResult deliberately does not touch ConsecutiveErrors, because a permission denial is not a transient failure. A login that loses rights across the board therefore stops the store dead with ErroringCollectors at zero. The health map is in-memory as well, so a restart zeroes every streak while the store stays as stale as the old session left it. This takes option 1 from the issue, in full. The band is ServerHealthClassifier.ClassifyFreshness from PerformanceMonitor.Common -- the same Fresh / Stale / Offline / NeverCollected ladder the Darling viewer bands, off the same ServerHealthThresholds numbers, and Lite already referenced that project. No threshold is added to Lite: its fastest configured collector runs every minute, which is the derivation CollectorCadence documents, so the shared constant is true here on its own terms rather than borrowed. Option 2 -- folding freshness into the status word -- is turned down, and the reason is written on CardStatus so the next reader has the argument in front of them. Lite's status word is a CONNECTION word and CollectorErrors already means one specific thing. Folding freshness in would make one amber word mean two unrelated failures, which is the conflation #2429 spent four review rounds untangling on the viewer and the reason #2422 was written. So freshness bands its own row and names its own band there: "08:00:00 (stopped)" in red, with a tooltip that says in words that this is about collection stopping rather than about the server's metrics. #2451 landed while this was open and it is what makes the rest of option 1 possible: the issue asked for the tooltip to "name it, the way it names CPU and Blocking today", and there is now a StatusReason built out of the card's own row displays to name it in. CollectionIsNotFresh joins the per-row concern gates beside CpuIsElevated and BlockingIsElevated, and the clause quotes LastCollectionDisplay verbatim like every other part there -- so the sentence and the row cannot render different things, and because the display already carries the band word the clause says which axis it is about without borrowing a metric's vocabulary. It goes FIRST in the reason. Collection is the row that says whether the other four can be believed at all: a card whose collection stopped four hours ago is showing four-hour-old CPU, and meeting "CPU 4%" before learning that is the wrong order to be told the two facts in. The card border escalates too, sharing the amber arm with CollectorErrors, because collector errors and collector silence are the same class of fault -- the monitoring of this server is not working -- and silence is the harder of the two to notice, which is the whole issue. It stays amber even for a stopped collection: red belongs to a dark server, and a red border under a green "Online" would be the loudest contradiction on the card. The band is stamped in GetServerSummaryAsync, the one place a ServerSummaryItem is built, so the two MCP reads get the same answer the Overview does rather than a fact that depends on which caller asked. Null means nobody classified it -- unreachable in the app, reachable from a fixture -- and renders exactly what the row rendered before: the bare stamp, the card's unknown grey, no tooltip. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ServerConnection.DotStatus computed "Unknown"/"Online"/"Warning"/"Offline" from its own (IsOnline, HasCollectorErrors) pair -- the same four-word ladder the Overview card derives, on a different type, on a different surface, from a different instance of the same flags. The two could say different things about one server and nothing would notice. That is the drift #2429 collapsed the viewer's card for and #2451 collapsed Lite's, and this is the last of them in Lite. Not the last of them anywhere, and an earlier draft of this message said so. Review caught it: DarlingServer.DotStatus in the Darling viewer is a fifth copy, and unlike this one it is not merely capable of disagreeing with its card -- on dev it already does. Both surfaces there classify the SAME ServerFreshness, and a never-collected server draws a grey "Unknown" dot beside an amber "Awaiting first collection" card. That is a different app, a different enum and a different vocabulary decision, so it is filed rather than folded in here. The collapse is not "ServerConnection switches on the pair too, but returns an enum". A second switch is a second ladder however it is typed, so the ladder itself moved into one function: ServerCardStatusRules.Classify, with Word() and Headline() beside it. ServerSummaryItem.CardStatus and ServerConnection.CardStatus both call it, and StatusDisplay/DotStatus and StatusHeadline/DotTooltip are renderings of the result. The card's brushes stay where they are -- hex colours are the card's business and the sidebar paints from theme resources -- but they still switch on CardStatus, not on the flags. The dot gets the tooltip the card's dot got in #2451, opening on the same sentence word for word. It is the thing a reader points at first, which is #2422's complaint one surface over from where it was reported. Its middle line is the part this surface specifically needs. #2457 deliberately kept collection freshness OUT of the status word and gave it its own banded row on the card; the sidebar has no such row and ServerConnection carries no last-collection time to build one from, so a green dot here is a connection answer read somewhere that offers no freshness answer at all. The tooltip says so and names where the freshness answer lives, rather than leaving a reader to infer one from the other -- which is the #2429/#2422 conflation in miniature. Classify takes two arguments and a test names that signature, so folding freshness in cannot happen by accident. #2451's pin moved with the ladder rather than being loosened, and it got wider, because the way it missed this copy is instructive. Assert.Equal(1, CountOccurrences(source, "IsOnline switch")) counted one literal in one file. The fourth copy was in another file AND was written as a chain of if statements rather than a switch, so it evaded that pin on both axes at once and went on evading it through #2451 and #2457. The count now spans both files and pins the classifier's own parameter list; and a new assertion holds the invariant the count was standing in for, syntax-agnostically -- the four words are string literals that appear in exactly one function, and none of them is written in ServerConnection.cs any more. Verification. Lite.Tests targets net10.0-windows and cannot run on macOS, so the logic was run for real anyway: a throwaway net10.0 harness splices ServerCardStatus, ServerCardStatusRules, ServerSummaryItem and the ServerConnection members straight out of the shipped files (a generator reads the source and emits them -- never retyped) behind a System.Windows.Media shim, with a real project reference to PerformanceMonitor.Common. All 35 checks pass, including every reachable (IsOnline, HasCollectorErrors) combination agreeing between the two surfaces. Against dev the strongly-typed harness does not compile at all, so the comparison is run as the identically-expressible subset with the new members reached reflectively: 7 passed / 6 failed on dev, 13 / 0 on the branch. The 7 that pass on both are the controls, and they are the ones a wrong fix breaks first -- the two ladders agree TODAY, which is why this defect is drift potential rather than a live disagreement, and why the assertions that separate the branches are about construction rather than output. The source- and XAML-scanning half was simulated in Python against both versions: 13 fail on dev and all 18 pass on the branch. Whole solution builds, 0 errors. CHANGELOG deliberately untouched -- #2395 is an open release-prep PR that owns that file for 3.5.1. One thing noted and not fixed here: Lite/Mcp/McpDiscoveryTools.cs renders a THIRD vocabulary for the same axis -- "Online"/"Offline"/"Status not checked" -- off ConnectionStatus.IsOnline. It is a different type, it never reads collector health so it has no "Warning" state, and the MCP text is a consumer API, so changing its words is a decision rather than a cleanup. Filed as a separate concern rather than folded in. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Gotcha 11 said a screen of Warning statuses "is not an incident" because status is "collection freshness, not health". That is wrong, and wrong in the worst available direction for a monitoring tool: it tells a new user their first reflex on seeing amber should be to disregard it. ServerHealthBands.ClassifyBand returns Warning for three unrelated causes. A metric at warning severity -- CPU, blocking, deadlocks, memory, threads -- returns Warning straight out of the overallMetricSeverity switch, and that IS a real condition on the server. Awaiting-first-collection returns Warning before that switch is reached. Stale collection returns it after, through the hasCollectorErrors arm the viewer re-uses to carry staleness. The advice survives in narrowed form, because the original observation was not baseless: on a fresh install every server is awaiting first collection at once, and at fleet scale staleness is routine. So an opening screen of amber usually is not an incident. What it must not become is a rule for every screen after that one. It now names the three causes and points at the tooltip #2429 added, which is built from the same metric rows shown underneath the status word, so it cannot disagree with them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
Fixes #2424, and answers both halves of @ehaar's #2422.
Both defects have the same shape: the Overview computes a rich health picture and then renders a lossy summary of it. On a 57-server fleet — which is where this tab earns its keep — they compound. The reporter read "Warning" in amber on the first card and asked what it was warning him about, then read "+52 more need attention" and asked where he could find them. Neither question needed new data; both needed the tab to show something it had already worked out.
The card says why
FleetRollup.BuildReasonalready turns a card into"CPU 94%, Blocking 12"out of that card's own metric displays — deliberately, per its own comment, so it can never drift from what the card shows — and every bit of that was spent on the five-row Needs Attention ranking. The card, which is the thing being looked at, never got it.The status line carries it now as a
ToolTip, in the shapeMainWindow.AlertBadges.csalready established for the sidebar badge: the breakdown, then how to act on it. It isBuildReason's output verbatim rather than a second derivation, because a second derivation would eventually disagree with the six metric rows sitting directly underneath it, which is the exact failureBuildReasonexists to prevent. Two arms sit on top of it:BuildReason's"Needs attention"fallback — that fallback is written for a ranking that only ever holds problem servers, and on a grid showing every server it would tell each green card the opposite of the truth;What the tooltips actually read, run against the shipped source:
Critical — CPU 96%, Blocking 6Warning — collection staleOffline — no recent collectionAwaiting first collectionHealthy — every metric on this card is inside its thresholdeach followed by
Double-click the card to open this server's tab.The second row is worth naming, because it is the reporter's exact card.
StatusDisplayrenders"Warning"for one condition only — an online server whose collection has gone stale — and nothing on the card said so, while the amber reads naturally as a metric problem.Background="Transparent"on thatTextBlockis load-bearing rather than decorative: aTextBlockwith a nullBackgroundhit-tests on its rendered glyphs alone, so the tooltip would have appeared over the letters of "Warning" and nowhere in the space around them."+52 more" reaches the 52
This took option 1 from the issue, the filter. The honest answer to "where do I find these warnings?" was nowhere;
FleetRollupcomputes the full problem set and truncates it for display, so those 52 were discarded rather than unavailable. The line is now a link into a needs-attention filter over the card grid — the surface that actually answers the question, because the grid is where the six metric rows live. It filters onFleetRollup.NeedsAttention, the same banding that counted the 52, so the destination cannot disagree with the label that sent you there.The filter is a toggle beside the existing Sort selector rather than a mode the link switches on invisibly, and the placement is the point: the roll-up header is docked to the top of the Overview and never scrolls, so a filter that is on can never be off screen while the grid it shrank is on it, and
showing 12 of 57renders beside the toggle whenever it is active. A filtered list that looks unfiltered is a worse defect than the dead end it replaces. It is deliberately not persisted the way the sort mode is — a sort is a preference, this is a triage action tied to a moment, and an Overview that opens with 52 of 57 servers already hidden is a support ticket even with the box in plain sight.The cap of five stays
Considered and kept, not left alone by default. It hides 52 servers on this fleet, which is the complaint — but the ranking renders inside that same non-scrolling docked panel. A list that grew with the fleet would push the cards it is pointing at off the screen, and would stop being a triage shortlist and become a second, worse copy of the grid without the metrics. The repair for an overflow is a destination rather than a longer list, and it now has one. The reasoning is written down on
DefaultWorstCountso the next person deciding this has the argument in front of them.One structural consequence
OverviewItemsControl.ItemsSourcewas read back in two places to answer "what cards are there" — the re-sort and the tag re-stamp — which is safe only while the bound list is the complete list. That is the exact mistakeFleetViewwas written to make impossible on the sidebar. The full sorted set is now the authority and the grid is projected from it through the filter in one place; sorting the bound list under an active filter would have silently discarded every card the filter was hiding. A test pins that there are exactly twoItemsSourcewriters and no readers.Verification
The Windows test suites cannot run on macOS, so the pure logic was run for real anyway: a throwaway
net10.0harness splicesFleetRollupandServerSummaryItemstraight out of the shipped files (never retyped) behind aSystem.Windows.Mediashim, and every assertion in the new test class passes against it — the table above is its output. Pointed atdev's copies of the same two files, it does not compile, because none of the members exist there.The four source-scanning facts were simulated in Python against both versions: 18 of 19 assertions fail on
devand all pass on the branch (the one that passes ondevis the guard that finds the statusTextBlockat all). The wiring half has to text-scan source, because aToolTipattribute and a click handler live in XAML where no assertion about a C# object can reach them, and removing either compiles perfectly clean.Whole solution builds with 0 warnings and 0 errors;
Darling.Testsbuilds. CHANGELOG deliberately untouched — #2395 is an open release-prep PR that owns that file for 3.5.1.🤖 Generated with Claude Code