Give the web dashboard's "+52 more" somewhere to go (#2437) - #2447
Conversation
The fleet page rendered the overflow as an inert muted div -- a number, a colour, and nothing to click. @ehaar asked the same question of the desktop twin in #2422 ("where do I find these warnings?") and the honest answer here was the same: nowhere. The 52 servers are not missing data. BuildRollup computes the whole problem set and then truncates it to DefaultWorstCount for display, so everything past the fifth is discarded rather than unavailable. The line is now the way to them. Activating it turns on a needs-attention filter over the card grid immediately below, which is the surface that actually answers the question because it is where the six metric chips live. #2429 settled two properties on the viewer and both port intact. The first is that the filter runs the SAME predicate the count was computed from. BuildRollup counts `Band != Healthy`, the payload carries that band per card already banded by the shared classifier, and cardNeedsAttention reads it off the card -- so the destination cannot disagree with the label that sent you there, and R1 still holds because no threshold is re-derived in the browser. A client-side approximation of "needs attention" is not a shortcut here, it is this defect wearing a new hat. The test pins that against both artifacts: it runs the reduction's own arithmetic, and it builds the literal the browser compares against out of the shipped serializer rather than typing it out, so renaming the band breaks the test instead of silently emptying the grid. The second is that an active filter is visible and clearable. The viewer can put its count beside the toggle and stop, because its roll-up header is docked and never scrolls; this page head scrolls away, so the notice rides with the cards instead and carries its own way out. It says what the desktop says, word for word -- "showing 12 of 57", and the all-clear arms too, since a filtered grid holding nothing is otherwise an empty page with no explanation. The colour follows the sentence rather than the filter, which is a review finding from #2429: painting an all-clear amber would be a colour contradicting its own text. The toggle sits beside Search and Sort because it is the same kind of thing, a view control over the same cards, and it is where the state lives -- the link turns it on rather than filtering behind its back. It is deliberately not persisted the way the sort and the grouped view are: a sort is a preference, this is a triage action tied to a moment, and a page that opens with 52 of 57 servers already hidden is a support ticket even with the box in plain sight. The empty-grid line had to learn which filter emptied it. Reusing the search wording unguarded would tell a reader whose fleet is simply healthy that nothing matches a term they never typed. This repository has no JavaScript test runner, so the pins text-scan the shipped module the way ViewerGridPayloadColumnOrderPinTests already scans server.js. Six of the seven fail against dev. Behaviour was verified for real by running that same file under a minimal DOM shim against a fabricated /api/fleet body: 23 assertions across a 57-server fleet, a mixed fleet, an all-healthy fleet and a one-server install, including the keyboard path in and out. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Reviewed. This is Darling-only (web dashboard) work with no T-SQL touched, so the collector/style conventions don't apply here, and the Lite half is explicitly out of scope for this PR (separate PR per the description) — no parity gap to flag. Checked and confirmed solid:
No correctness, security, or performance issues found. Nice test coverage pinning the JS module content against the shared C# artifacts (enum serialization, viewer wording) so a rename breaks the test instead of silently drifting. |
My own notice had the defect this PR is about. It read "showing N of 57" with the fleet as the denominator, which is fine on its own and wrong the moment a search term is also narrowing the grid: "showing 4 of 57" invites reading 4 as the fleet's problem count, and the other 53 were not judged healthy, they were never looked at. The label has to mean what the grid holds. The two filters now compose explicitly. The search runs first, its result is the denominator, and the notice names the term when there is one -- "Needs attention only, matching "off" -- showing 2 of 2." With no term the wording is unchanged, because searched.length is then the whole fleet. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Follow-up commit: my own notice had the defect this PR is about. The count read The two filters now compose explicitly: the search runs first, its result is the denominator, and the notice names the term when there is one — Harness is now 24 assertions, all passing against the branch; the C# pins still split 6 of 7 against |
|
Reviewed the diff (fleet.js, app.css, and the new What I checked:
Nit (non-blocking): when a search term matches zero servers and the attention filter is on, No correctness, security, or performance issues found. This is a well-tested, low-risk change. |
Review's nit, and it is right. With a search term AND the attention filter both on, an empty grid always said "No servers needing attention match X" -- which is vacuously true when X matched nothing at all, and points the reader at the wrong control. They would go looking for a filter to clear when the answer is that the term matches no server on the fleet, healthy or otherwise. The denominator commit already computed the thing that splits the two cases: searched is the term's result before the toggle sees it. So a zero there blames the search, a non-zero blames the filter, and the wording says which. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Took the nit — it is right, and the denominator commit had already computed the thing that fixes it. With both filters on, an empty grid always said
Harness is 26 assertions now, all passing; the C# pins still split 6 of 7 against |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Reviewed. This is a well-scoped, well-tested client-side change — no T-SQL involved, and the PR description correctly scopes it to the Darling web dashboard only (Lite gets its own PR, per #2437), so no parity drift to flag here. What holds up:
Left two inline comments on
Nothing here blocks merging as far as I can tell — both are polish items. |
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>
|
Reviewed the diff (test + CSS + Correctness / R1 discipline: Lite/Darling parity: not applicable — Lite has no web dashboard (WPF-only), and the PR description already scopes this to the web half with the Lite equivalent as a separate PR. The desktop Darling.Viewer wording ( Security: all new text (search term, sentence, tag names) goes through Left one inline note: a copy edge case where "the 1 server monitored is healthy" is reused for a search-narrowed result of 1, which reads as if the whole fleet has one server rather than "one match." Everything else looks solid — nice test coverage pinning the JS against the shipped serializer instead of transcribing it. |
Review again, and again on wording I ported without noticing the surface it landed on is different. AttentionFilterCountText comes from the viewer, whose Overview has no search box, so "the 1 server monitored is healthy" is simply true there -- total IS the fleet. Here total is what the SEARCH left. So on a 57-server fleet narrowed to one match, the notice read "matching "prod-07" -- the 1 server monitored is healthy", which claims the fleet holds one server while 56 others exist and were never looked at. The plural arm had it too: "all 20 servers are healthy" over a term that matched 20 of 57. The unnarrowed arms stay verbatim, so the cross-surface vocabulary pin still holds and still means something. With a term active the sentence says "matching" instead of "monitored", which is the fact that makes it true. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Reviewed the diff (Darling web dashboard only — no T-SQL, no Lite changes, both correctly out of scope per the PR description and #2437's split). Verified:
Left one inline finding: when the "Needs attention only" filter empties the flat (non-grouped) grid, the page shows two stacked messages that say the same thing (the |
Review, and it lands on the PR's own stated goal. With the filter on and nothing left to show, the page stacked two boxes: the notice above saying "all 57 servers are healthy", and the grid-area fallback below saying "No servers need attention." Same fact, different words, one under the other. The desktop viewer shows only its count and leaves the grid blank. The notice already explains an empty grid whenever it is showing, and more precisely -- it names the search term, the population, and whether anything was judged at all. So the fallback is suppressed under it, leaving the one case the notice does not cover: the filter off, where the search term is the only thing that can have emptied the grid. That collapses noMatchText back to a single question, which is the honest shape for it. Its three-way split existed only to disambiguate cases the notice now owns; keeping it would have meant maintaining two answers to a question only one of them is ever asked. The term-less arm stays, worded honestly, because it is unreachable today rather than impossible. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Reviewed the diff ( What I checked
No T-SQL in this PR, so the collector/style conventions don't apply here. One non-blocking observation, pre-existing and not introduced by this diff: |
1 participant
Part 1 of #2437 — the web dashboard half. (The Lite half is a separate PR: different language, different risk.)
fleet.js:168rendered the overflow as an inert muted div:A number, a colour, and nothing to click. @ehaar asked the same question of the desktop twin in #2422 — "where do I find these warnings?" — and the honest answer on this surface was the same: nowhere. Those 52 servers are not missing data.
BuildRollupcomputes the whole problem set and then truncates it toDefaultWorstCountfor display, so everything past the fifth is discarded rather than unavailable.The line is now the way to them
Activating it turns on a needs-attention filter over the card grid immediately below — the surface that actually answers the question, because it is where the six metric chips live. #2429 settled two properties on the viewer and both port intact.
The filter runs the same predicate the count was computed from
BuildRollupcountscards.Where(c => c.Band != FleetHealthBand.Healthy); the payload already carries that band per card, banded server-side by the sharedServerHealthClassifier;cardNeedsAttentionreads it off the card. So the destination cannot disagree with the label that sent you there, and R1 still holds — no threshold is re-derived in the browser. A client-side approximation of "needs attention" is not a shortcut here, it is this defect wearing a new hat.That is pinned against both artifacts rather than transcribed:
Renaming the band member now breaks the test instead of silently emptying the grid the link lands on.
An active filter is visible and clearable
The viewer can put its count beside the toggle and stop there, because its roll-up header is docked and never scrolls. This page head scrolls away, so the notice rides with the cards instead and carries its own way out (
Show all servers). Its wording is the desktop's, word for word —showing 12 of 57, and the all-clear arms too, because a filtered grid holding nothing is otherwise an empty page with no explanation.The colour follows the sentence, not the filter. That is a review finding from #2429 ported rather than rediscovered: this line says either a count of servers wanting attention or an all-clear, and painting an all-clear amber would be a colour contradicting its own text — the family of defect this whole change is about.
Needs attention only — showing 12 of 57.offalso typed in SearchNeeds attention only, matching “off” — showing 2 of 2.Needs attention only — all 57 servers are healthy.Needs attention only — the 1 server monitored is healthy.Needs attention only, matching “xyz” — nothing matches that term, so no server was judged.Needs attention only, matching “warn0” — all 10 matching servers are healthy.Rows 2 and 5 are follow-up commits, both of them this PR's own defect turned on itself. The first version counted against the fleet, which is fine alone and wrong once a search term is also narrowing the grid —
showing 4 of 57invites reading 4 as the fleet's problem count. And a term that matched nothing leftshownat 0, so it took the green all-clear arm: the colour that means everything is fine, over a fleet whose problem servers were never looked at. That is the same colour-contradicting-its-own-text finding #2429's review raised about the amber arm, ported without noticing it had a third case.Row 6 is the third:
AttentionFilterCountTextis ported from a surface where it is unconditionally true, because the viewer's Overview has no search box and itstotalis the fleet. Heretotalis what the search left, sothe 1 server monitored is healthyclaimed the fleet held one server while 56 others existed and were never looked at. The unnarrowed arms stay verbatim — the cross-surface vocabulary pin still holds and still means something — and with a term active the sentence names the search instead.The notice also carries
role="status", matchingutil.js'snoticeStrip— it appears and re-words itself with no page load, and a filter you can reach by keyboard whose result is never announced is a half-finished job.The toggle sits beside Search and Sort because it is the same kind of thing — a view control over the same cards — and it is where the state lives: the link turns it on rather than filtering behind its back, so either affordance can undo the other. It is deliberately not persisted the way the sort and the grouped view are (both are in
localStorage): a sort is a preference, this is a triage action tied to a moment, and a page that opens with 52 of 57 servers already hidden is a support ticket even with the box in plain sight.onActivaterather thanonClick, because that isutil.el's path that also installsrole="button", a tabindex and Enter/Space — the same treatment every other clickable div on this page already gets; a plain click handler would look identical and be invisible to a keyboard.One sentence per state, including the empty ones. The notice explains an empty grid whenever it is showing, so the grid-area fallback is suppressed under it rather than stacking a second box that says the same thing in different words — the desktop viewer shows only its count and leaves the grid blank, and this now matches. What remains is the case the notice does not cover: the filter off, where the search term is the only thing that can have emptied the grid.
Verification
This repository has no JavaScript test runner, so the pins text-scan the shipped module, the way
ViewerGridPayloadColumnOrderPinTestsalready scansserver.js. Six of the seven fail againstdev; the one that passes is the guard that finds the overflow line at all plus the server-side arithmetic, which has not changed.Behaviour was verified for real anyway, because a source scan cannot see whether the thing works. The shipped
fleet.jswas run under a minimal DOM shim (no npm, no jsdom —createElement/createTextNodeand a stubbedfetch) against a fabricated/api/fleetbody of the shapeBuildRollupemits: 31 assertions, all passing, over a 57-server fleet, a mixed fleet, an all-healthy fleet and a one-server install — including that the filtered grid holds exactly the 12 problem servers and no healthy card, that the five already ranked are still reachable in it, that search composes with the filter, and the keyboard path in and out. Againstdevthe same harness cannot get past the third assertion, because the affordance does not exist.One thing noted and not fixed here, raised in review and pre-existing:
renderGrouped's own empty-state branch is unreachable wheneverlastTags.length > 0, becausebuildTagGroupsemits a header per tag in the forest even when no card falls under it. So narrowing to zero in the grouped view renders a tree of zero-count headers rather than a message. That was already true of the search box; the attention filter is a second way to reach it. It is cosmetic rather than confusing here — the notice sits above the tree and says what happened — and unpicking it means changing how the grouped view treats empty groups, which is #2020's lane rather than this one.Whole solution builds, 0 errors. CHANGELOG deliberately untouched — #2395 is an open release-prep PR that owns that file for 3.5.1.
🤖 Generated with Claude Code