Fix remaining SonarCloud MEDIUM/LOW severity issues - #87
Conversation
- godre:S8193 (Go): drop unnecessary variable declarations in two test
conditions where the value wasn't used in the assertion body.
- javascript:S2486: give the three silent catch blocks (localStorage/
theme access, all of which are expected to fail in private browsing
or with storage disabled) an explanatory comment instead of swallowing
the exception without a trace.
- javascript:S6582/S7755: replace manual `&&`-guarded property chains
with optional chaining, and `arr[arr.length - 1]` with `arr.at(-1)`.
- javascript:S3358: extract gauge.js's nested ternary into a small
`gaugeColorVar` helper.
- css:S1874: replace the deprecated `word-break: break-word` with its
modern equivalent `overflow-wrap: break-word`.
- Web:S6842/S6819/MouseEventWithoutKeyboardEquivalentCheck: the four
metric cards used an `<h2 role="button" tabindex="0">` with a
hand-rolled Enter/Space keydown handler to fake a clickable heading.
Replaced with a real `<button>` inside the `<h2>`, which gets keyboard
activation for free and let the manual keydown handler in app.js be
deleted. The whole card remains the mouse click target.
- Web:S6819: replaced the three `role="dialog"` modal-backdrop divs with
native `<dialog>` elements. showModal()/close() now drive visibility,
which also means the browser handles focus trapping and Escape-to-
dismiss natively, so the hand-rolled focusablesIn/trapTab/dismissModal/
visibleModal/wireModalKeys machinery in app.js could be deleted.
The API key prompt's "not dismissible" behavior is preserved by
preventDefault()-ing the dialog's native `cancel` event. Each dialog's
own `close` event now drives side effects (focus return, clearing the
open detail metric) so they fire consistently regardless of how the
dialog was dismissed.
- Web:S6819: replaced the range-picker's `role="group"` div with a
`<fieldset>` (with a screen-reader-only `<legend>`).
Verified in a real browser (Playwright/Chromium): card header buttons
open the detail modal via click and keyboard (Enter), all three dialogs
open/close via their trigger, Escape, and backdrop click (except the
API key prompt, which correctly ignores Escape/backdrop), focus returns
to the trigging element on close, native focus trapping keeps Tab
inside an open dialog, and the whole card (not just the header button)
remains clickable. No visual regressions in light or dark theme.
Caught and fixed one regression from the <dialog> conversion during
that verification: `dialog.modal { display: flex; }` unconditionally
overrode the UA's default `dialog:not([open]) { display: none }`,
which would have left every dialog visible (in-flow, at the bottom of
the page) even when closed. Scoped the rule to `dialog.modal[open]`.
There was a problem hiding this comment.
Verified on f101d12: go build ./..., go vet ./..., go test ./... -race -cover and golangci-lint run (0 issues) all pass. No dangling references to the deleted helpers (closeModal, dismissModal, visibleModal, trapTab, focusablesIn, wireModalKeys) or to the removed .modal-backdrop / role="button" markup, and no new dependencies. The Go, /proc-parsing, exec.Command, privilege-separation and /api/v1 stability parts of the checklist are untouched by this PR.
Two findings. (Submitting as a comment rather than "request changes" only because GitHub won't let a review do that on your own PR — the first finding is blocking by this repo's rules.)
Missing test coverage (blocking per this repo's policy)
The checklist note says the JS/HTML changes "have no existing JS test harness". That's true for a browser-level harness, but this repo already locks front-end invariants from Go, and that's the harness that applies here:
internal/web/embed_test.goassertsindex.htmlcontainsid="theme-toggle"and loadstheme-init.jsinternal/web/xss_test.goreadsassets/app.jsout ofassetsFSand greps it for a forbidden pattern (innerHTMLinterpolation, issue #19)
Per CLAUDE.md ("New or changed behavior must ship with tests — this is a hard requirement") and docs/TESTS.md, the accessibility rework should extend that pattern rather than rely on manual verification alone. The dialog.modal → dialog.modal[open] bug you caught by hand during browser testing is the strongest argument for it: a one-line assertion over the embedded style.css would stop exactly that regression from coming back, and it's the kind of bug that silently ships if nobody re-runs Playwright by hand.
Concretely, in internal/web and following the Test<Subject>_<Scenario> convention, reading from assetsFS (no real I/O):
TestIndexHTML_ModalsAreNativeDialogs—updates-modal/apikey-modal/detail-modalare<dialog>elements, and noclass="modal-backdrop"orrole="dialog"remainsTestStyleCSS_DialogRuleScopedToOpen—dialog.modal[open]is present and there is no unscopeddialog.modal {block that would defeatdialog:not([open]) { display: none }TestIndexHTML_ClickableCardHeadersUseButtons— eachdata-metriccard's<h2>wraps a.card-header-btn<button>, and no<h2 role="button"remainsTestAppJS_NoManualModalKeyHandling—app.jsno longer hand-rolls the Tab trap / Escape routing, mirroring the guard style ofxss_test.go
These are cheap string assertions against the embedded FS, in the same spirit as the tests already there. Happy to be told a subset is enough, but shipping this with zero automated coverage isn't consistent with the repo's own rule.
CSS: word-break: break-word → overflow-wrap: break-word is not a 1:1 swap
See the inline comment on style.css — overflow-wrap: anywhere is the exact equivalent, and the difference is observable in the updates table.
Everything else in the <dialog> conversion reads correctly to me: the cancel-preventDefault() for the non-dismissible API key prompt, routing side effects through the native close event, min-width: 0 on the <fieldset> so it lays out like the <div> it replaces, and keeping backdrop-dismiss off the API key dialog.
Per CSS Text L3, word-break: break-word is defined as word-break: normal + overflow-wrap: anywhere, not overflow-wrap: break-word. The two differ in intrinsic sizing: anywhere counts its soft wrap opportunities toward min-content width, break-word doesn't. That's observable on .updates-table, an auto-layout table whose column widths derive from min-content: with break-word a long unbreakable package name kept the column's min-content width at the full token width, pushing the table past the modal and triggering the .modal-body auto horizontal scrollbar instead of wrapping the name in place. Verified in-browser: a long unbreakable name now wraps within its column with no horizontal overflow on the table or modal body.
The vanilla JS/CSS/HTML in internal/web/assets has no test framework and is never covered by the Go coverage report (sonar.go.coverage.reportPaths), so New Code Coverage always shows 0% on any frontend-only change, tanking the quality gate's >= 80% requirement regardless of how well the actually-tested Go code is covered (this PR's own quality gate failed that way). sonar.coverage.exclusions removes these files from the coverage ratio only; they stay in sonar.sources, so SonarCloud still analyzes and reports bugs, code smells, and vulnerabilities in them (the accessibility and modernization rules fixed earlier in this PR were caught this way).
There was a problem hiding this comment.
Re-reviewed at b49317c. go build, go vet, go test ./... -race -cover and golangci-lint run (0 issues) all pass locally, and all 5 CI checks are green.
The CSS finding is fully resolved — all four sites now use overflow-wrap: anywhere, and 3fd595d's message captures the intrinsic-sizing reasoning accurately. No new findings in either follow-up commit.
One clarification so the green quality gate isn't read as closing the other item: the missing test coverage from my earlier review is still open.
sonar.coverage.exclusions=internal/web/assets/** is a correct fix for a real and separate problem — JS/CSS/HTML can never appear in a Go coverage report, so those files would always report 0% and drag New Code Coverage under the gate on any frontend-only change. Correctly scoped (coverage ratio only, files stay in sonar.sources), and the comment explains it well. No objection to it.
But it changes a metric, not the underlying gap. CLAUDE.md's rule is that new or changed behavior ships with tests, and this PR still rewrites the modal system, the card header controls, and the range picker with no automated test asserting any of it. Worth noting the exclusion doesn't get in the way of fixing that: the tests I suggested are Go tests in internal/web reading from assetsFS, and only internal/web/assets/** is excluded — internal/web/*_test.go is unaffected, exactly like the existing embed_test.go and xss_test.go.
Still a judgment call that's yours to make, and it's the last thing standing between this and a clean review from my side.
CLAUDE.md requires tests for new or changed behavior; the <dialog>/ <button> migration in this PR shipped with browser verification only, per review feedback. Add four static-asset guard tests (same pattern as xss_test.go: grep the embedded FS for a banned/required construct, no real browser or DOM needed): - TestIndexHTML_ModalsAreNativeDialogs: the three modals are <dialog> elements, not a role="dialog" div inside a .modal-backdrop wrapper. - TestStyleCSS_DialogRuleScopedToOpen: the dialog.modal rule is scoped to [open]. This is the exact regression caught by hand during manual browser testing (an unscoped rule overrides the UA default that hides a closed <dialog>, leaving every dialog permanently visible) and is the strongest case for automated coverage here. - TestIndexHTML_ClickableCardHeadersUseButtons: each clickable card's <h2> wraps a real <button>, not a role="button" on a non-interactive element. - TestAppJS_NoManualModalKeyHandling: the hand-rolled Tab trap/Escape routing and the manual Enter/Space keydown handler don't come back, now that native <dialog>/<button> provide the same behavior for free.
|
You're right, and I owe you an apology — I only checked inline review comments ( Added all four suggested tests in
|
|
2 participants
📖 Description
Follow-up to #86: fixes the 25 remaining open SonarCloud issues on
LarsLaskowski_PiMonitor(MEDIUM/LOW severity), now that the 8 HIGH-severity ones are merged.godre:S8193(2×, Go tests,sysinfo_test.go/middleware_test.go): dropped unnecessary variable declarations inifconditions where the value wasn't used in the assertion body.javascript:S2486(3×): the three silentcatchblocks aroundlocalStorage/theme access (expected to fail in private browsing or with storage disabled) now carry an explanatory comment instead of swallowing the exception without a trace.javascript:S6582/S7755(8×,app.js): manual&&-guarded property chains → optional chaining;arr[arr.length - 1]→arr.at(-1).javascript:S3358(1×,gauge.js): extracted a nested ternary into a smallgaugeColorVarhelper.css:S1874(4×): the deprecatedword-break: break-word→ its modern equivalentoverflow-wrap: break-word.Web:S6842/Web:S6819/MouseEventWithoutKeyboardEquivalentCheck(12× across the 4 metric card headers and 3 modals):<h2 role="button" tabindex="0">with a hand-rolled Enter/Space keydown handler to fake a clickable heading. Replaced with a real<button>inside the<h2>, which gets keyboard activation for free — the manual keydown handler inapp.jscould be deleted. The whole card remains the mouse click target (click listener stays on the card<section>, unaffected by the header markup change).role="dialog"modal-backdrop<div>s are now native<dialog>elements.showModal()/close()drive visibility, and the browser now handles focus trapping and Escape-to-dismiss natively, so the hand-rolledfocusablesIn/trapTab/dismissModal/visibleModal/wireModalKeysmachinery inapp.jscould all be deleted. The API key prompt's "not dismissible" behavior is preserved bypreventDefault()-ing the dialog's nativecancelevent. Each dialog's owncloseevent now drives side effects (focus return, clearing the open detail metric) so they fire consistently regardless of how the dialog was dismissed (button, Escape, or backdrop click).Web:S6819(1×): the range-picker'srole="group"<div>is now a<fieldset>with a screen-reader-only<legend>.🎫 Issues
N/A (SonarCloud housekeeping, follow-up to #86).
👩💻 Reviewer Notes
The
<dialog>/button conversion is the behaviorally-sensitive part of this PR, so I didn't rely on the Go test suite alone (it doesn't cover the JS/HTML) — I ran it in a real browser (Playwright/Chromium) and verified: card header buttons open the detail modal via click and keyboard (Enter), all three dialogs open/close via their trigger, Escape, and backdrop click (except the API key prompt, which correctly ignores both), focus returns to the triggering element on close, native focus trapping keeps Tab inside an open dialog, and the whole card — not just the header button — remains clickable. No visual regressions in light or dark theme (screenshots compared before/after).One real bug turned up during that verification and is already fixed here:
dialog.modal { display: flex; }unconditionally overrode the browser's defaultdialog:not([open]) { display: none }, which would have left every dialog permanently visible (in-flow, at the bottom of the page) even when closed. Scoped the rule todialog.modal[open].📑 Test Plan
go build ./...,go vet ./...,go test ./... -race -cover— all pass; the two Go test tweaks are style-only (same assertions, no new coverage needed).golangci-lint run— 0 issues.✅ Checklist
General
go test ./... -race -coverpasses locally). (No new automated tests — behavior-preserving refactor for the Go side; the JS/HTML accessibility changes have no existing JS test harness and were verified manually in a real browser, see Test Plan.)go vet ./...andgolangci-lint runare clean.ARCHITECTURE.mdif this changes a documented design decision. (N/A — no architectural decision changed.)⏭ Next Steps
None — this clears the SonarCloud backlog down to 0 open issues as of this PR.