feat(diagnostics): dismiss + clear-all parity with Notifications panel#304
Conversation
…ifications panel Diagnostics panel entries only ever accumulated with no way to clear seen preflight/CLI-stderr warnings. Adds: - stable per-entry id (_nextDiagnosticId) so a rendered item can be located and removed - per-item dismiss (x) control -> dismissDiagnostic(id), removes from the diagnostics array + DOM, restores the empty-state placeholder when the list drains, refreshes the badge - Clear all button in the panel header -> clearAllDiagnostics() Client-side only: unlike Notifications (notification_dismiss / notification_dismiss_all over the websocket), diagnostics have no server-side dismiss contract, so this only mutates in-memory state + DOM. A dismissed diagnostic can legitimately reappear if the next settings-preflight run re-emits the same condition. Does not touch the lr-e901 dedup path (_isDuplicateDiagnostic, DEDUP_WINDOW_MS) or toast rendering — covered by the existing addDiagnostic dedup regression test, which still passes.
There was a problem hiding this comment.
PEACHES — clean (0 findings)
All review gates passed. Dismiss / clear-all implementation reaches parity with app-notifications.js pattern:
Correctness verified:
- Per-entry dismiss: stable incrementing
idstamped at entry creation, located viadata-diag-idin DOM, removed from array + DOM, badge updated. - Clear-all: array emptied, innerHTML cleared, empty placeholder restored, badge hidden (count → 0).
- Accessibility: aria-labels on both controls; focus restored to closeBtn after clear-all; focus-visible outlines on both buttons.
- Dedup contract (lr-e901): _isDuplicateDiagnostic check, early return, DEDUP_WINDOW_MS all untouched.
Code quality:
- refreshIcons() call repositioned after item append (was inside actionable-only block) — necessary fix, ensures dismiss button icon renders even when no actionable property.
- dismissDiagnostic() safely handles stale click (idx === -1 returns early).
- _restoreEmptyPlaceholder() guards against duplicate placeholders (checks existing before append).
Test coverage: 14 regression tests cover stable identity, per-entry dismiss mechanics, clear-all flow, dedup preservation, badge behavior, and CSS styling.
Brand: No violations; user-facing strings ("Clear all" button text, aria-labels) follow Clagentic: Console naming rules.
Cross-layer imports: None; diagnostics.js stays in lib/public/modules layer.
There was a problem hiding this comment.
BOBBIE — clean
Audited PR #304 (feat/lr-diag-dismiss-clear-all-parity -> main, head a31b2f4). Frontend-only change adding per-entry dismiss and clear-all controls to the Diagnostics panel.
DOM injection review (primary focus per assignment):
- lib/public/modules/diagnostics.js:311 and :327 — untrusted
entry.source/entry.scope/entry.message(settings-preflight + CLI stderr text) are assigned viatextContent, notinnerHTML. No markup injection surface. - lib/public/modules/diagnostics.js:316 — new
dismissBtn.innerHTML = iconHtml("x")call.iconHtml()(lib/public/modules/icons.js:25) builds its string from a hardcoded literal argument only ("x"), never from diagnostic data. Matches the pre-existing pattern at line 215 and line 339 (both unmodified). Not a new sink for untrusted data. - lib/public/modules/diagnostics.js:393 —
data-diag-idselector interpolatesid, which is_nextDiagnosticId(module-internal integer counter), not attacker-controlled. Not a selector-injection vector. - lib/public/modules/diagnostics.js:409 —
panelListEl.innerHTML = ""(clear-all) assigns an empty string; not an injection sink.
No new innerHTML sinks for user/data-derived strings were introduced.
Scanner results:
- gitleaks: 6 hits, all in unrelated
.claude/worktrees/*/lib/certs/privkey.pemdev-cert artifacts outside this PR's diff scope (diagnostics.js, index.html, diagnostics.css, new test file only). Not attributable to PR #304. - semgrep (auto config, 214 rules against the 4 changed files): 11 findings, all
html.security.audit.missing-integrityon CDN<script>/<link>tags at index.html lines 21 and 2069-2078. Confirmed viagit diffthat this PR's index.html change is scoped to a single replaced line (345, panel header markup) — none of the flagged lines are touched by this diff. Pre-existing, not introduced by this PR. - osv-scanner: not applicable — no package.json/package-lock.json changes in this diff.
- .crew/bobbie.yaml project rules (no-push-token-in-source, no-api-key-in-yoke, postinstall-safe): no matches in changed files.
early_exit_reason: low-surface frontend diff — DOM-only additions using safe textContent rendering consistent with existing codebase patterns; no reachable injection sink introduced.
scanners_run: gitleaks (completed, 0 in-scope findings), semgrep (completed, 0 in-scope findings), osv-scanner (not_applicable, no manifest delta)
Note: the SHA provided in the dispatch (a31b2f4 with an extra trailing digit, 41 hex chars) was malformed. Independently verified the correct 40-char head SHA via git log against the locally-checked-out commit and bobbie-github-security read: a31b2f4.
clagentic gate-note — authorized
Authorize rationale: PEACHES clean (0 findings, dedup path lr-5db5/lr-e901 preserved). BOBBIE clean (no new DOM-injection sink). CI 631/631 at HEAD. Authorized by naomi. |
What
Adds dismiss / clear-all behavior to the Diagnostics panel (
#diagnostics-panel), reaching parity with the existing Notifications/Alerts panel (app-notifications.js)..diagnostics-itemgets a dismiss (x) control. Clicking it removes the entry from the in-memorydiagnosticsarray and its DOM node, then refreshes the badge..diagnostics-emptyplaceholder.id(_nextDiagnosticId) added so dismiss can locate the exact DOM node via adata-diag-idattribute — the array previously had no unique key per entry.aria-labels, are real<button>elements (native keyboard operability), and clear-all moves focus to the panel's close button rather than a node about to be removed from the DOM.Client-side only (no server round-trip)
Diagnostics have no backend dismiss message type, unlike Notifications (
notification_dismiss/notification_dismiss_allover the websocket). This PR does not invent one. Dismiss/clear-all here only mutate the client-sidediagnosticsarray and the DOM. Trade-off, called out explicitly per the task brief: a dismissed diagnostic can legitimately reappear if the next settings-preflight run re-emits the same underlying settings.json condition — that is expected behavior and out of scope for this UI fix.Design source
Mirrors
lib/public/modules/app-notifications.js(dismissNotif,clearAllBanners,.notif-banner-clear-all) for interaction and a11y conventions, adapted to the Diagnostics panel's existing structure (no banner stack — dismiss/clear-all act directly on the persistent panel list).Untouched (per contract)
addDiagnosticdedup logic (_isDuplicateDiagnostic,DEDUP_WINDOW_MS, lr-e901) — unmodified._showDiagnosticToasttoast rendering — unmodified.Tests
test/diagnostics-dismiss-clear-all-parity.test.js— source-text regression checks following this project's existing DOM-harness-free convention (seediagnostics-panel-pointer-events-lr-b580.test.js,diagnostics-toast-dedup-placement-lr-e901.test.js). Covers: stable id assignment, per-item dismiss wiring + DOM/array removal + badge refresh + empty-state restore, clear-all wiring + array/DOM/empty-state/badge reset, explicit assertion that clear-all does NOT send a websocket message, and that the existing dedup/badge-hide contracts are untouched.npm test: 623/623 passing, 0 failures.Brand
No user-facing product-name strings touched; "Clear all" / dismiss labels only.