Add metric detail modal with history range picker#54
Conversation
Clicking a metric card (CPU, Load, Temperature, Memory) now opens a modal showing that metric's history in a larger chart, with 5m/15m/1h range buttons that re-scale the chart to the chosen span. The window is naturally bounded by the server's retained history (history_window_minutes), since points beyond that are never returned by /api/v1/metrics/history. Reuses the existing updates-modal pattern and the chart.js sparkline renderer; no API or config changes. The open modal stays in sync with freshly polled history and repaints on theme changes. Cards are keyboard-accessible (role=button, Enter/Space to open); the modal closes via the close button, backdrop click, or Escape.
There was a problem hiding this comment.
Reviewed against the project's review checklist (.claude/skills/review-pr).
Verification
go build ./...,go vet ./...,go test ./...all pass on the PR branchgolangci-lint run— 0 issues
Checklist results
- REST API stability: ✅ No changes to any
/api/v1/...response shape — the modal consumes the existingGET /api/v1/metrics/historyendpoint. I verified the JS reads the exact JSON keys the server emits (cpu_percent,load1,temperature,memory_used_percent, and per-pointt/vfromHistoryPoint). - Range semantics: ✅ Filtering back from the most recent sample's timestamp (Pi clock) rather than the browser clock is the right call and consistent with how
renderMetricstreatssnap.timestamp. The 5m/15m/1h buttons also align with the defaulthistory_window_minutes: 60retention. - Dependencies: ✅ None added; reuses the existing hand-rolled
drawSparklinerenderer, per the project's dependency philosophy. - Error handling/leaks/parsing/exec safety: N/A — no Go changes.
- Test coverage: Frontend-only; there is no JS test harness in the repo, and the existing
internal/webembed tests still pass. Headless-browser verification described in the PR is a reasonable substitute here. - Language: ✅ All new code and comments are in English.
Minor, non-blocking observations
- Focus management in the modal: opening doesn't move focus into the dialog and closing doesn't return focus to the invoking card, so keyboard users who open via Enter are left focused on the (now background) card. The existing updates modal has the same behavior, so this is consistent with the codebase pattern — fine to leave as a follow-up that fixes both modals together.
role="button"on the whole<section>: children of an element withrole="button"are treated as presentational by assistive tech, which flattens the card's<h2>and metric values into one long accessible name. A visually hidden "Show details" button inside the card (orrole="button"on the heading only) would preserve the card semantics. Minor a11y nit, non-blocking.- Single-sample edge: with exactly one point in the selected range, the stats line reports "1 sample" but the chart area is blank (
drawSparklineneeds ≥ 2 points). Trivial and self-resolving after the next poll. - There are now two document-level Escape handlers (updates modal + detail modal), each closing its own modal. Harmless since only one is open at a time — just noting it in case a shared helper ever makes sense.
Verdict
Everything on the checklist checks out — this is a clean, well-scoped frontend change that reuses the existing modal and sparkline patterns as intended. Good to merge as-is; the observations above are optional follow-ups.
- Move role="button"/tabindex/aria-label from the card <section> onto its heading. With the role on the whole section, assistive tech treated the card's children as presentational and flattened the heading and metric values into one accessible name; exposing the heading as the single control keeps the card's text readable. The full card stays clickable via mouse, and the card highlights on :focus-within so keyboard focus on the heading is still visible. - In the detail modal, a single sample in the selected range left the chart blank (drawSparkline needs >= 2 points) while the stats line still said "1 sample". Show a "collecting more samples to plot" hint instead so the empty chart is explained.
|
Thanks for the review! Addressed two of the observations in 6985b41:
Leaving #1 (focus management) and #4 (shared Escape handler) as a follow-up, since both touch the updates modal too and are better fixed for both modals together.
|
Follow-up to the review on both modals: - Move focus into a dialog when it opens (its close button) and restore focus to the invoking element when it closes, so keyboard users who open the detail view from a card land in the dialog and return to that card afterwards. Tab is now trapped within the open dialog, as an aria-modal dialog should behave. - Replace the two separate document-level Escape listeners (one per modal) with a single handler that dismisses whichever modal is open and also drives the Tab focus trap. Applies to both the updates modal and the metric detail modal via shared openModal/closeModal helpers.
|
Also did the two deferred follow-ups (7cf5cb8), for both the updates modal and the detail modal via shared
Verified in a headless browser: focus lands on the close button on open, Tab/Shift+Tab wrap within the dialog, and Escape returns focus to the opening card (detail) / button (updates), with no console errors. |
Summary
Implements the historical range picker / metric detail view (C1). Clicking a
metric card now opens a modal showing that metric's history in a larger chart,
with 5m / 15m / 1h range buttons that re-scale the chart to the chosen span.
Each is keyboard-accessible (
role="button",tabindex="0", Enter/Space toopen) with a hover/focus accent outline.
(
index.html/app.js/style.css) and the existingchart.jssparkline renderer — no new charting code.
from the most recent sample's timestamp, i.e. the Pi clock) and redraw the
chart. A stats line reports now / min / avg / max and the sample count. The
span is naturally bounded by the server's retained history
(
history_window_minutes), since points older than that are never returned byGET /api/v1/metrics/history.changes. It closes via the close button, backdrop click, or Escape.
No changes to the REST API or configuration — consumes the existing
GET /api/v1/metrics/historyendpoint.Verification
Built the binary and drove the live dashboard in a headless browser
(Chromium/Playwright):
range active; the enlarged chart renders the metric's history.
the chart and stats.
click, all work with no console errors.
go build ./...,go vet ./...,go test ./..., andgolangci-lint runare all clean.
Related Issue
Closes #9
Checklist
go test ./...passes locally) —change is frontend-only (HTML/CSS/JS); the existing
internal/webembedtests still pass and there is no JS test harness in the repo. Verified
end-to-end in a headless browser instead.
go vet ./...andgolangci-lint runare cleaninstallation/packaging — N/A, no such changes
/api/v1/...response shapes, or a new APIversion was introduced instead — no API changes