feat(site): rename "Dismiss warnings" to "Mute warnings" and make health callouts dismissible - #27554
Conversation
…callouts dismissible The header button toggles whether error/warning health checks surface in the top-nav status indicator; it does not dismiss the in-page callout. Rename it to "Mute warnings" / "Unmute warnings" to reflect what it actually does, and make the warning/error callouts themselves dismissible via the Alert component's built-in close button.
|
/coder-agents-review |
|
Review starting... deep-review v0.9.0 | Round 4 | Last posted: Round 4, 9 findings (1 P2, 5 P3, 2 Nit, 1 Note), APPROVE. Review Finding inventoryFinding inventory: PR #27554Findings
Round logRound 1Netero-only first pass clean (Nit + Note, P3 floor). Panel of 11 (bisky, hisoka, mafu-san, mafuuu, pariston, gon, leorio, ging-ts, ging-react, nami, meruem + Netero). 1 P2, 4 P3, 2 Nit. ging-ts and ging-react: no findings. Reviewed against d57965e..bd3d7a4. Round 2 updateBLOCKED. Churn guard: 2 addressed (CRF-1, CRF-6), 1 acknowledged (CRF-2), 2 contested (CRF-5, CRF-7), 2 silent (CRF-3, CRF-4). No panel spawned. Reviewed against d57965e..55d9672. Round 3 updatePROCEED. Churn guard: 2 addressed (CRF-3 rename, CRF-4 DERPRegionPage dismissible), 2 contested carried (CRF-5, CRF-7). Rename verified clean: no stale DismissWarningButton refs, old file removed, 6 imports updated. Panel re-review of the delta. Reviewed against d57965e..a2afadb. Round 3 panel findingsPanel of 10 (bisky, hisoka, mafu-san, mafuuu, pariston, gon, leorio, nami, ging-react, kite) + Netero. Fixes verified: CRF-3 rename complete (grep clean, old file gone, 6 imports), CRF-4 DERPRegionPage dismissible added, CRF-1 holds (no error Alert dismissible), CRF-6 comment accurate. New: CRF-8 (P3, stale PR description), CRF-9 (Note, missing prominent). Contested CRF-5 evaluated: 5 reviewers re-raised on a "no-op on warnings / only consumer is getHealthErrors" premise that is empirically false; dismissed drives a 2nd consumer (HealthLayout:164 bell-off, any severity), so "Mute" is accurate and pre-existing scope. Defense holds on merits; not escalated; still needs a ticket or explicit human acceptance for the error-nav scope. CRF-7 closed as Nit (Alert dismiss storied; button-copy gap pre-existing). ging-react: no findings. Round 4 updatePROCEED. Churn guard: CRF-8 addressed (description rewritten), CRF-9 addressed (prominent added, c61c76b), CRF-5 deferred with ticket #27557. Delta 00d134e..c61c76b (rebased) is one code line: prominent on ProvisionerDaemonsPage warning Alert. Confirming panel. Round 4 panel resultPanel of 7 (bisky, hisoka, mafu-san, mafuuu, pariston, nami, zoro) + Netero. No findings from any reviewer. All prior fixes verified against current code: CRF-1 holds (4 error Alerts non-dismissible), CRF-3 holds (no stale DismissWarningButton refs, 7 imports), CRF-4 holds (DERPRegionPage dismissible), CRF-6 holds (comment accurate), CRF-8 fixed (description matches diff), CRF-9 fixed (all 7 warning callouts render prominent+dismissible). CRF-2 accepted, CRF-7 closed Nit, CRF-5 deferred (#27557). All 9 findings resolved. APPROVE. Contested and acknowledgedCRF-2 (P3, WebsocketPage.tsx:46) - two divergent close affordances
CRF-5 (P3, DismissWarningButton.tsx:66) - "Mute warnings" mutes errors
CRF-7 (Nit, DismissWarningButton.tsx:11) - no Storybook coverage
CRF-5 panel evaluation (R3)
About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
|
/coder-agents-review |
|
Chat: Review posted | View chat Review history
|
![coder-agents-review[bot]](https://avatars.githubusercontent.com/in/3454270?s=60&v=4){kind=small}
There was a problem hiding this comment.
The rename is clean and complete: no test, e2e, or code path references the old copy, so nothing breaks on the swap, and the mute/unmute verb with the Bell/BellOff icons is a clear metaphor for silencing a notification. The dismissible change reuses the existing Alert close button rather than adding a new primitive. ging-ts and ging-react found nothing. Severity count: 1 P2, 4 P3, 2 Nit across a +30/-21 diff.
The one finding worth a decision before merge is CRF-1: the error callouts are now dismissible, and on a health-diagnostics page a dismissed error stays hidden across the 30s background refetch. The description's "reappear on reload/refetch" is inaccurate for the refetch half, so the stated safety valve does not exist. This needs a human call, not an agent-accepted default.
Hisoka summed up the rest: "I came looking for a fight. Copy renames and a dismissible prop on seven Alerts... Nothing bites."
Process note: the chore: retrigger CI empty commit is permanent history noise; squash it on merge so the log keeps only the commit that says what happened.
site/src/pages/HealthPage/DismissWarningButton.tsx:10
P3 [CRF-3] The component and file are still named DismissWarningButton while every string and internal identifier now says "mute". (Gon P2, Netero/Hisoka/Mafu-san/Mafuuu/Pariston/Leorio/Nami/Meruem Nit)
Renamed: the labels ("Mute warnings"/"Unmute warnings"), the toasts, and the locals (
isMuted,muteMutation,unmuteMutation). Unchanged: the exported symbol, the filename, and the six imports.
This PR's thesis is that the label must match the behavior; leaving the component named DismissWarningButton re-creates that mismatch one layer down, and now "dismiss" separately names the Alert X. A reader greps "mute" and finds no component; greps "dismiss" and lands on the mute button. Rename to MuteWarningsButton and update the imports, or file a ticket. Gon rated this P2; I set P3 because it has no runtime effect and the rename is wider than this PR's scope. The server field dismissed_healthchecks is a wire contract and correctly stays; the frontend/API vocabulary split is worth a tracked follow-up.
🤖
site/src/pages/HealthPage/DERPRegionPage.tsx:81
P3 [CRF-4] DERPRegionPage renders the same warning callouts but was left non-dismissible. (Nami)
The region drill-down (reached by clicking a DERP region) maps warnings to
<Alert severity="warning" prominent>with aHealthMessageDocsLinkaction, the identical pattern made dismissible onDERPPage.tsx:162, but has nodismissible.
The PR description lists DERP as covered; the region view is part of DERP and was missed. A user closes a warning on the DERP page, drills into a region, and the same warning is back with no close button. Add dismissible here to match. (Line is outside the diff; this file was not touched.)
🤖
🤖 This review was automatically generated with Coder Agents.
…trim comment Address coder-agents-review feedback: - P2: error <Alert>s were dismissible with local useState. HealthLayout refetches every 30s and reuses the mounted subpage, so a dismissed error would suppress subsequent (possibly different) error messages until reload. Remove dismissible from all severity=error alerts; warning alerts stay dismissible. - Nit: trim the two-instance mutation comment to just the load-bearing detail.
|
/coder-agents-review |
There was a problem hiding this comment.
Round 2 is blocked pending a response on two round-1 findings that got no code change and no reply. They were folded into the round-1 review body (their lines were not on the diff), so there was no inline thread to answer, which is likely why they slipped past. The rest of round 1 was handled well.
Verified fixed:
- CRF-1 (P2):
dismissibleremoved from all fourseverity="error"health alerts in 55d9672; only warnings remain dismissible. The masking-across-refetch concern is resolved. - CRF-6 (Nit): the mutation comment was trimmed to the load-bearing version.
Responses noted and carried to the next round:
- CRF-2 (P3): acknowledged as a disclosed product call.
- CRF-5 (P3): contested; "Mute warnings" kept per the requester, with an offer to file a follow-up for the nav-indicator naming. No ticket linked yet.
- CRF-7 (Nit): contested; skipped per the shared-
Alertargument, matching Bisky's dissent.
Blocking (silent: no code change, no response):
- CRF-3 (P3)
DismissWarningButton.tsx:10: the component and file are stillDismissWarningButtonwhile every string and internal identifier now says "mute", and "dismiss" now separately names theAlertX. Rename toMuteWarningsButton(updating the six imports), or file a tracked follow-up and say so. - CRF-4 (P3)
DERPRegionPage.tsx:81: the DERP region drill-down still renders the same warning callouts withoutdismissible, which contradicts the PR's own claim that DERP is covered. Adddismissibleto matchDERPPage.tsx, or state why the region view is intentionally excluded.
The panel will not re-review until each of these two is addressed: fix it, file a ticket, or explain why it should not be fixed. A bare "won't fix" is not a resolution; the justification is weighed against the finding's consequence. Push a change or reply on the two points and re-request review.
🤖 This review was automatically generated with Coder Agents.
…uteWarningsButton, dismissible DERP region warnings Address round-2 coder-agents-review feedback: - CRF-3 (P3): file/component name still said Dismiss while every string and identifier had moved to mute; "dismiss" is now the callout X. Rename to MuteWarningsButton and update the six import sites. - CRF-4 (P3): DERPRegionPage renders the same warning callouts as DERPPage. Mark them dismissible to match.
|
/coder-agents-review Round-2 blockers addressed in a2afadb:
|
There was a problem hiding this comment.
Round 3 clears the round-2 blockers cleanly. The DismissWarningButton -> MuteWarningsButton rename is complete (no stale references under site/src, old file removed, all six imports updated), CRF-4 added dismissible to the DERPRegionPage warning callout, and the round-2 fixes hold: every error Alert stays non-dismissible (CRF-1) and the mutation comment is accurate (CRF-6). Nami: "The weather is clearing."
Severity this round: 1 new P3 (stale PR description), 1 new Note, plus the two contested findings evaluated. No P0/P1.
On the contested findings, one correction to the record. Five reviewers re-raised CRF-5 ("Mute warnings" mutes errors) arguing the button is a no-op on warning-severity sections because getHealthErrors is the only consumer of dismissed. That premise is wrong: dismissed has a second consumer, HealthLayout.tsx:164, which renders a bell-off icon next to any muted section regardless of severity. So muting a warning is not a no-op, and "Mute" (with the bell/bell-off metaphor) is an accurate, improved label over "Dismiss." Leorio (who first raised CRF-5) and Kite both found this and withdrew the finding. The panel does not escalate CRF-5. What remains is the pre-existing point that muting an error-severity section also drops it from the top-nav error banner; that is a product/copy call the requester made, unchanged by this PR. It stays open only because the offered follow-up has no ticket: please file one or state the mismatch is accepted as-is (a follow-up promise without a ticket is not a resolution). CRF-7 is closed as a Nit; the shared-Alert dismiss is storied and the button-copy gap is pre-existing.
Process note: the PR description now contradicts the shipped code (see CRF-8); update it before squash-merge since it becomes the commit body. The chore: retrigger CI empty commit is still there; squash it.
🤖 This review was automatically generated with Coder Agents.
…allout prominent Align with the other five Health pages, which already set prominent on their warning callouts. Addresses coder-agents-review CRF-9.
|
/coder-agents-review Round-3 items addressed:
Process notes: leaving the retrigger commit alone as it will disappear on squash-merge. |
There was a problem hiding this comment.
Round 4 resolves the last open items. The round-4 delta is a single line (adding prominent to the ProvisionerDaemonsPage warning callout, CRF-9), plus a PR-description rewrite (CRF-8). A panel of seven reviewers plus Netero re-verified the whole change and found nothing new.
All nine findings are resolved:
- CRF-1 (P2) fixed and holding: the four error-severity Alerts stay non-dismissible, so a dismissed callout can no longer mask an active fault across the 30s refetch.
- CRF-3 (P3) fixed:
DismissWarningButton->MuteWarningsButtonrename is complete (no stale references, old file removed, all seven imports updated). - CRF-4 (P3) fixed:
DERPRegionPagewarning callout is dismissible, matching its siblings. - CRF-6 (Nit) fixed: the mutation comment states the behavior it prevents.
- CRF-8 (P3) fixed: the description now matches the code (warning-only, DERP Region listed, refetch-survival called out, and the intentional no-error-dismiss rationale spelled out).
- CRF-9 (Note) fixed: all seven warning callouts now render
prominent+dismissibleconsistently. - CRF-2 (P3) accepted as a disclosed product call; CRF-7 (Nit) closed; CRF-5 (P3) deferred to #27557 with a linked ticket (a real deferral, not a drop).
The description now honestly documents the durability model: warning dismissal is client-side useState keyed by warning.code, so a dismissed warning survives a refetch and returns on reload/remount. Bisky: "A real gem stays a real gem when the setting is polished and nothing new is set into it." Nothing further from the panel. Approving.
🤖 This review was automatically generated with Coder Agents.
2 participants
Fixes a mismatch between the header button's label and its behavior on the Health pages. Today the button reads Dismiss warnings, which suggests it will close the in-page callout, but it actually toggles whether the health check surfaces in the top-nav status indicator and shows a bell-off icon in the sidebar. The callout itself has no way to be closed.
Changes
DismissWarningButtontoMuteWarningsButton.dismissibleon the warning<Alert>s across the Health pages (Access URL, Database, DERP, DERP region, Provisioner Daemons, Websocket, Workspace Proxy) so users can close the callout from the callout itself.Alertalready supports this via a built-in close button.<Alert>s are intentionally not dismissible:HealthLayoutrefetches every 30s and reuses the mounted subpage, so allowing dismissal would suppress subsequent (possibly different) error messages until reload. Diagnostics pages should not hide active faults.prominent.Notes
Alert's existinguseStatebehavior). Warning<Alert>s are keyed bywarning.code, so a dismissed warning reappears on reload/remount but survives a refetch. The mute toggle continues to persist server-side viadismissed_healthchecks.This PR was generated by Coder Agents on behalf of @tracyjohnsonux.