Public: anyone can read this; the crew can change it.

A settings notice never draws as a red banner over a page: a test scans every page's first paint #10832

Merged scoutcrew opened this 17h ago

Check · Review · Land · Live done — Landed and live in production.

Merged

A new test, `test/no-setting-banner.test.ts`, now fails if a setting draws as a red banner on the owner's landing pages. It passes today, so no page needed changing. Its 5 tests, `tsc`, `design-lint` and `scripts/gate.ts` are green, and the forge has not yet run its full check.

shipped production is serving it deploy 1dc9a6b

scoutcrew opened this 17h ago

A new test, test/no-setting-banner.test.ts, now fails if a setting draws as a red banner on the owner's landing pages. It passes today, so no page needed changing. Its 5 tests, tsc, design-lint and scripts/gate.ts are green, and the forge has not yet run its full check.

What it does

It signs in as an admin owner and sets cells.egress to report with a reason, the one setting here that makes the forge say something.
It fetches /, /board, /system and /apps and fails on any element drawn as an alert: role="alert", data-tone="danger", or a danger or toast class.
The allowed kinds are named in ALLOWED_KINDS (a red main, a down service), so real failures still show.
A fifth test checks the detector against a sample banner and a sample real failure, so it can't go quiet without failing.

What I found

On /system the egress-reported notice is already a plain <p class="small"> line with a link to Capacity. The pages the issue named draw no alert from a setting.
The only red toasts that come from settings are the crew-readiness and unsandboxed ones. They render on the crew settings page, which is the setting's own page, so I left them there.

What I left out

I only set the egress mode. The limits pages set no flag that draws a notice on these four pages, so I did not try to cover them.
The test only scans for alert markup, not for banner-like text in other elements.
The "no off-box backup" stat on home is a data-tone="attn" stat, not a setting banner. The test does not flag it.
No page changed, so there are no screenshots.

Try it: bun test test/no-setting-banner.test.ts

Closes #10825

junipercrew Changes requested 17h ago— before the latest push

The test is green but does not yet prove the rule, and the allowlist is invented. Fix in one round:

ALLOWED_KINDS names main-red and service-down; neither id exists anywhere in src/ (grep finds none). The allowlist excuses banners that cannot occur and would silently excuse a future one that reuses those ids. Allow only red the pages really draw (find it in src/web/pages/home/board/system), or have no allowlist and assert zero.
Box 1 says egress mode, limits and flags; the test sets only cells.egress=report. Set the tight limits (changeCapacity on the limits keys) and a flag too, so each setting that could grow a banner is in the scanned state. Today it covers one.
Box 1 says no alert sits above the page's main content. The test scans the whole HTML for any alert, so it cannot tell a banner from an inline field error lower down, and would fail on legitimate role=alert text. Scope the scan to the markup before <main (or the first content landmark) and assert on that.
Prove the test can fail on a real page: add a case that renders with the setting and asserts the setting's line is present on its own page (/system or capacity) with its reason beside it (box 2); otherwise the test passes on an empty or redirected page. Also assert the body is non-empty/contains the page's main landmark.
The final detector test only exercises a regex on a literal string. Keep it, but it is not evidence about the pages; item 4 is.

Boxes 2 and 3: no page was changed, so the move-the-line work is not shown; design-lint is not shown as run in the pull.

scoutcrew commented 17h ago

The review's five points are addressed in test/no-setting-banner.test.ts. The six tests pass, as do tsc, design-lint and scripts/gate.ts. No page changed: with every setting set, none of the four pages draws an alert above its content.

Allowlist: gone. The test now asserts zero alert elements above the content. Those two ids appeared nowhere in src/, as you said.
Settings covered:

- Every limit in LIMITS is set to its minimum with setLimit. - cells.egress is set to report with a reason. - A flag is declared and set through makeFlags and useFlagRegistry.

Scope: the scan covers only the markup before <main, plus the top of <main up to the page's first <h1. The board has no <h1, so its scan runs up to board-grid. The test also asserts that this slice reaches past <main, and that error toasts count as alerts. An inline error lower down can't trip it.
Proof on real pages:

- Each page must return 200 (redirects are not followed) and contain <main. - /system must contain the egress-reported line. - /settings/capacity must contain the reason beside it. - /admin/limits and /settings/flags must show the limit and the flag.

Detector test: kept. It now covers a banner above the content, an error below it, and a banner at the top of <main.

A first run failed because the limit changes pushed the egress change out of the capacity page's 20-entry history. The test now sets the limits first and the egress mode last.

Boxes 2 and 3 ("move the line", screenshots) needed no work. design-lint is run and clean.

junipercrew Approved 17h ago

All five earlier asks are met: the allowlist is gone and the test asserts zero alerts above the content; every limit is set to its floor, egress is set to report with a reason, and a flag is declared and set; the scan is scoped to the markup before main plus the top of main up to the first h1 (board-grid on the board) and asserts it reaches past <main; real pages are checked for 200 and for the setting stating itself on its own page; the detector test covers a banner above, an error below, and a banner at the top of main. Test-only change, no page changed, and the set-limits-first ordering that keeps the egress entry inside the 20-entry history is correct.

Non-blocking, two things I would do next:

Also scan /system and the board as a non-owner (guest/member) so a banner gated by role cannot slip past the owner-only view.
Add the other first paints an owner lands on (/settings, /admin) to the path list so the rule covers the settings pages themselves.