diff --git a/internal/api/incident_store.go b/internal/api/incident_store.go index 0900697..29f4802 100644 --- a/internal/api/incident_store.go +++ b/internal/api/incident_store.go @@ -261,14 +261,17 @@ func resolveIfSettled(ctx context.Context, q querier, incidentID int64) (bool, e } // acknowledgeIncident records that userID has picked an incident up, and reports -// whether it changed anything — an already-resolved incident is left alone. -// Shared by the authenticated handler and the Acknowledge button in a push -// notification, so both write the same state and the same timeline entry. +// whether it changed anything — an already-resolved or already-acknowledged +// incident is left alone, so a second acknowledge (a retried request, or a +// stale push notification tapped after the web UI already acked it) is a +// no-op rather than a second "acknowledged" timeline entry. Shared by the +// authenticated handler and the Acknowledge button in a push notification, +// so both write the same state and the same timeline entry. func acknowledgeIncident(ctx context.Context, q querier, incidentID, userID int64) (bool, error) { res, err := q.ExecContext(ctx, ` UPDATE incidents SET status = 'acknowledged', acknowledged_by = $1, acknowledged_at = $2 - WHERE id = $3 AND resolved_at IS NULL`, + WHERE id = $3 AND status = 'triggered'`, userID, time.Now().Unix(), incidentID) if err != nil { return false, err diff --git a/internal/api/incidents.go b/internal/api/incidents.go index 5287f17..18a0c84 100644 --- a/internal/api/incidents.go +++ b/internal/api/incidents.go @@ -197,10 +197,20 @@ func handleIncidentAcknowledge(db *sql.DB) http.HandlerFunc { return } if !acked { - if !incidentExists(w, r, db, id) { + // incidentIDParam above already confirmed the incident exists, so this + // is either resolved, or already acknowledged — the latter is now a + // no-op rather than an error, since the caller's desired state + // (acknowledged) already holds. + inc, err := fetchIncident(r.Context(), db, id) + if err != nil { + respond(w, http.StatusInternalServerError, errResp("internal error")) return } - respond(w, http.StatusConflict, errResp("incident is resolved")) + if inc.Status == "resolved" { + respond(w, http.StatusConflict, errResp("incident is resolved")) + return + } + respond(w, http.StatusOK, inc) return } respondIncident(w, r, db, id) diff --git a/internal/api/incidents_test.go b/internal/api/incidents_test.go index a98b0b5..f55f6e2 100644 --- a/internal/api/incidents_test.go +++ b/internal/api/incidents_test.go @@ -338,6 +338,48 @@ func TestIncident_Acknowledge(t *testing.T) { } } +// A second acknowledge — a retried request, or a stale push notification +// tapped after the web UI already acked it — must be a no-op: same state, +// no second "acknowledged" timeline entry. Regression test for the bug +// described in issue #26 ("two acknowledged entries look like a bug"). +func TestIncident_AcknowledgeTwiceIsIdempotent(t *testing.T) { + s := newTS(t) + postWebhook(t, s, []map[string]any{ + amAlert("fp-ack2", "Y", "firing", "2026-05-20T10:00:00Z", zeroTime, nil), + }) + + resp := s.req(t, http.MethodPost, "/api/incidents/1/acknowledge", nil) + if resp.StatusCode != http.StatusOK { + t.Fatalf("first acknowledge returned %d", resp.StatusCode) + } + var first map[string]any + decode(t, resp, &first) + + resp = s.req(t, http.MethodPost, "/api/incidents/1/acknowledge", nil) + if resp.StatusCode != http.StatusOK { + t.Fatalf("second acknowledge returned %d, want 200 (idempotent)", resp.StatusCode) + } + var second map[string]any + decode(t, resp, &second) + if second["status"] != "acknowledged" { + t.Errorf("expected status still acknowledged, got %v", second["status"]) + } + if second["acknowledged_by"] != first["acknowledged_by"] { + t.Errorf("expected the same acknowledged_by, got %v then %v", first["acknowledged_by"], second["acknowledged_by"]) + } + + types := eventTypes(timeline(t, s, 1)) + n := 0 + for _, ty := range types { + if ty == "acknowledged" { + n++ + } + } + if n != 1 { + t.Errorf("expected exactly one acknowledged event, got %d in %v", n, types) + } +} + func TestIncident_ManualResolveIsTerminal(t *testing.T) { s := newTS(t) postWebhook(t, s, []map[string]any{ diff --git a/internal/api/notify_ack.go b/internal/api/notify_ack.go index a23c43f..43c4507 100644 --- a/internal/api/notify_ack.go +++ b/internal/api/notify_ack.go @@ -66,12 +66,19 @@ func handleNotifyAck(db *sql.DB) http.HandlerFunc { return } if !acked { - // The incident closed between the page and the tap. Nothing to do, - // and nothing the responder did wrong — report the state, not an error, - // so ntfy shows a success toast rather than a failure. + // Either the incident closed between the page and the tap, or it was + // already acknowledged (e.g. from the web UI, or an earlier tap of + // the same button) — either way nothing the responder did wrong, so + // report the actual state rather than assuming "resolved", and let + // ntfy show a success toast rather than a failure. + inc, err := fetchIncident(r.Context(), db, incidentID) + if err != nil { + respond(w, http.StatusInternalServerError, errResp("internal error")) + return + } respond(w, http.StatusOK, map[string]any{ "incident_id": incidentID, - "status": "resolved", + "status": inc.Status, }) return } diff --git a/internal/api/notify_test.go b/internal/api/notify_test.go index c3b6ef0..55453b3 100644 --- a/internal/api/notify_test.go +++ b/internal/api/notify_test.go @@ -408,6 +408,47 @@ func TestNotify_AckButtonAcknowledgesIncident(t *testing.T) { } } +// The ack token isn't single-use (it stays valid for a day, in case the +// first tap never reaches the server), so tapping the same notification's +// Acknowledge button twice is a real scenario, not just a retried request. +// It must report the incident's actual state, not assume "resolved" — +// see handleNotifyAck's !acked branch — and must not log a second +// "acknowledged" event. +func TestNotify_AckButtonTwiceIsIdempotent(t *testing.T) { + s, f := notifyTS(t, api.NotifyConfig{PublicURL: "https://terdut.example.com"}) + + fireCritical(t, s) + s.sweepNotify(t) + + ackURL := f.messages()[0].Actions[0].URL + path := ackURL[strings.Index(ackURL, "/api/notify/ack/"):] + + for i := range 2 { + resp, err := http.Post(s.URL+path, "application/json", nil) + if err != nil { + t.Fatalf("ack %d: %v", i+1, err) + } + var body map[string]any + decode(t, resp, &body) + if resp.StatusCode != http.StatusOK { + t.Fatalf("ack %d returned %d", i+1, resp.StatusCode) + } + if body["status"] != "acknowledged" { + t.Errorf("ack %d: expected status acknowledged, got %v", i+1, body["status"]) + } + } + + n := 0 + for _, ty := range eventTypes(timeline(t, s, 1)) { + if ty == "acknowledged" { + n++ + } + } + if n != 1 { + t.Errorf("expected exactly one acknowledged event after two taps, got %d", n) + } +} + func TestNotify_AckRejectsUnknownToken(t *testing.T) { s, _ := notifyTS(t, api.NotifyConfig{PublicURL: "https://terdut.example.com"}) fireCritical(t, s) diff --git a/internal/web/static/app.css b/internal/web/static/app.css index 6090c77..1ce3324 100644 --- a/internal/web/static/app.css +++ b/internal/web/static/app.css @@ -47,12 +47,27 @@ --font: system-ui, -apple-system, "Segoe UI", Roboto, "Helvetica Neue", Arial, sans-serif; --mono: ui-monospace, SFMono-Regular, Menlo, Consolas, monospace; + /* A ~4-size type scale, per issue #26's "use one type scale" ask. This + file still has a dozen one-off font-size values below; migrating the + low-risk, purely cosmetic ones (standalone titles with no dimensional + or functional constraint) onto these tokens is a start, not the whole + job — the rest (sizes tied to a fixed shape like the avatar circle, to + a deliberately prominent display like a stat tile or the on-call name, + or to a non-negotiable constraint like the 16px that stops iOS zooming + into an input) stay as either their own pixel value or a documented + exception, since guessing at those without seeing them render risks + trading one inconsistency for a worse one. */ + --fs-xs: 12px; + --fs-sm: 13px; + --fs-base: 14px; + --fs-lg: 18px; + --fs-xl: 21px; + --topbar-h: 52px; - /* No bottom tab bar on any breakpoint any more — mobile uses the hamburger - menu in the topbar, desktop the sidebar — so this stays 0. Kept as a - variable rather than deleted since .view, .toast and .nav's own height - calc still read it. */ - --tabbar-h: 0px; + /* The phone-width bottom tab bar's height. Unused above 900px: the + desktop block overrides .nav/.view/.toast directly rather than reading + this back down to 0. */ + --tabbar-h: 58px; --safe-top: env(safe-area-inset-top, 0px); --safe-bottom: env(safe-area-inset-bottom, 0px); } @@ -212,9 +227,6 @@ input:focus, textarea:focus { outline: none; border-color: var(--accent); box-sh font-size: 18px; font-weight: 700; letter-spacing: -0.01em; overflow: hidden; text-overflow: ellipsis; white-space: nowrap; min-width: 0; } -#menu-btn { position: relative; } -.nav-badge.menu-btn-badge { top: 2px; left: auto; right: 2px; } - .open-pill { display: inline-flex; align-items: center; gap: 6px; padding: 3px 10px; border-radius: 999px; @@ -226,11 +238,11 @@ input:focus, textarea:focus { outline: none; border-color: var(--accent); box-sh .open-pill.has-triggered::before { background: var(--crit); } .open-pill.all-acked::before { background: var(--warn); } -/* Hidden on phones — mobile navigates through the hamburger menu in the - topbar instead (see #menu-btn / openNavMenu in app.js). Reappears as the - left sidebar from 900px, where the desktop block below redeclares display. */ +/* Bottom tab bar on phones (Queue, On-call, Alerts, Team, More); becomes the + left sidebar from 900px, where the desktop block below redeclares display + and shows every section flat, .nav-link-secondary included. */ .nav { - display: none; + display: flex; position: fixed; left: 0; right: 0; bottom: 0; z-index: 20; height: calc(var(--tabbar-h) + var(--safe-bottom)); padding-bottom: var(--safe-bottom); @@ -240,8 +252,15 @@ input:focus, textarea:focus { outline: none; border-color: var(--accent); box-sh border-top: 1px solid var(--border); } .nav-brand { display: none; } +/* Hidden here (shown from 900px below): on the phone bar the team switcher + lives in the topbar instead, as #team-selector-mobile. */ +.nav-team-selector { display: none; } +.nav-link-secondary { display: none; } .nav-link { position: relative; + /* flex: 1 spreads the tabs evenly across the bar's width; the desktop + block below cancels it back to a natural-width row item. */ + flex: 1 1 0; display: flex; flex-direction: column; align-items: center; justify-content: center; gap: 2px; color: var(--faint); font-size: 11px; font-weight: 600; /* min-width lets a column shrink below its label's natural width, which is @@ -276,6 +295,7 @@ input:focus, textarea:focus { outline: none; border-color: var(--accent); box-sh } .team-selector-mobile { padding: 4px 10px; font-size: 12px; max-width: 120px; } .team-selector-label { overflow: hidden; text-overflow: ellipsis; white-space: nowrap; } +.team-selector-chevron { width: 14px; height: 14px; flex: none; color: var(--faint); margin-left: -2px; } /* A team's identity colour — not a status, so never the severity palette. Six colours, then they repeat; teamColorClass() in format.js picks one by the @@ -318,6 +338,14 @@ input:focus, textarea:focus { outline: none; border-color: var(--accent); box-sh } .chip[aria-selected="true"] { background: var(--text); border-color: var(--text); color: var(--bg); } .chip .count { margin-left: 4px; opacity: 0.7; } +/* Pinned to the visible right edge of the scrolling row (sticky, not + absolute, so it tracks the scroll position rather than the content). */ +.chips-fade { + position: sticky; right: -1px; flex: none; + width: 24px; margin-left: -24px; + background: linear-gradient(to right, transparent, var(--bg)); + pointer-events: none; +} /* ---------- lists ---------- */ @@ -381,7 +409,7 @@ input:focus, textarea:focus { outline: none; border-color: var(--accent); box-sh .empty { padding: 48px 16px; text-align: center; color: var(--muted); } -.empty strong { display: block; color: var(--text); font-size: 16px; margin-bottom: 4px; } +.empty strong { display: block; color: var(--text); font-size: var(--fs-lg); margin-bottom: 4px; } .empty .icon { width: 36px; height: 36px; color: var(--ok); margin-bottom: 8px; } .load-error { @@ -434,8 +462,12 @@ input:focus, textarea:focus { outline: none; border-color: var(--accent); box-sh .detail-head .copy { margin-left: auto; } .clip-buffer { position: fixed; top: 0; left: 0; opacity: 0; pointer-events: none; } .detail-head .crumb { font-weight: 600; color: var(--muted); font-size: 14px; } -.detail-title { font-size: 21px; font-weight: 750; letter-spacing: -0.01em; margin: 16px 0 8px; overflow-wrap: anywhere; } +.detail-title { font-size: var(--fs-xl); font-weight: 750; letter-spacing: -0.01em; margin: 16px 0 8px; overflow-wrap: anywhere; } .detail-badges { display: flex; flex-wrap: wrap; gap: 6px; margin-bottom: 14px; } +/* A copy of the sticky actionbar's primary button, right under the status + it responds to — see quickActions() in incident.js. */ +.detail-quick-actions { margin-bottom: 14px; } +.detail-quick-actions .btn-primary { font-size: 16px; min-height: 44px; } .card { background: var(--surface); @@ -450,6 +482,9 @@ input:focus, textarea:focus { outline: none; border-color: var(--accent); box-sh .facts dt { color: var(--muted); } .facts dd { margin: 0; overflow-wrap: anywhere; } .facts .sub { color: var(--faint); } +.facts dd.fact-summary { display: flex; flex-wrap: wrap; align-items: center; gap: 8px; } +.fact-chip { display: inline-flex; align-items: center; gap: 4px; } +.fact-icon { width: 15px; height: 15px; color: var(--faint); } .section { margin-top: 22px; } .section-title { @@ -481,6 +516,17 @@ details > summary::before { content: "▸ "; } details[open] > summary::before { content: "▾ "; } details[open] > summary { margin-bottom: 8px; } +/* One .tl-phase per status the incident has been through (see + timelinePhases() in incident.js) — each with its own .timeline
    , so + the existing :first-child/:last-child rail-capping below gives each phase + its own self-contained connecting line rather than one running through + the headings. */ +.tl-phase + .tl-phase { border-top: 1px solid var(--border); } +.tl-phase-title { + padding: 10px 14px 0; + font-size: 11px; font-weight: 700; text-transform: uppercase; letter-spacing: 0.06em; + color: var(--faint); +} .timeline { list-style: none; margin: 0; padding: 4px 0; } .tl-item { position: relative; @@ -513,7 +559,7 @@ details[open] > summary { margin-bottom: 8px; } } .note-fix { background: var(--ok-soft); border-left: 3px solid var(--ok); } .similar { list-style: none; margin: 0; padding: 0; } -.similar-item { padding: 10px 0; } +.similar-item { padding: 10px 14px; } .similar-item + .similar-item { border-top: 1px solid var(--border, var(--surface-2)); } .check { display: flex; align-items: center; gap: 8px; font-size: 14px; color: var(--muted); } .note-actions { display: flex; justify-content: flex-end; } @@ -552,7 +598,7 @@ details[open] > summary { margin-bottom: 8px; } @keyframes sheet-up { from { transform: translateY(24px); opacity: 0.6; } } .sheet-inner { padding: 8px 16px calc(16px + var(--safe-bottom)); } .sheet-grab { width: 40px; height: 4px; margin: 0 auto 12px; border-radius: 2px; background: var(--border-strong); } -.sheet-title { font-size: 17px; font-weight: 700; margin: 0 0 4px; } +.sheet-title { font-size: var(--fs-lg); font-weight: 700; margin: 0 0 4px; } .sheet-text { color: var(--muted); margin: 0 0 14px; font-size: 14px; } .sheet-form { display: grid; gap: 12px; } .sheet-actions { display: flex; gap: 8px; margin-top: 16px; } @@ -604,9 +650,18 @@ details[open] > summary { margin-bottom: 8px; } .now-label { color: var(--muted); font-size: 13px; font-weight: 600; } .now-name { font-size: 20px; font-weight: 750; } .you { color: var(--accent); font-weight: 650; font-size: 13px; margin-left: 6px; } +/* Oncall's own "you" indicator only (see you() in oncall.js) — a pill badge + is easier to spot there than this plain accent-coloured text. */ +.you-badge { margin-left: 6px; } .week-nav { display: flex; align-items: center; gap: 4px; } .week-nav .label { font-size: 14px; font-weight: 650; min-width: 9em; text-align: center; } +/* The week-nav button that shows the date range, "28 Sep – 4 Oct", with the + ISO week number as secondary text inside it — a separate class from + .label above (team.js's month-nav uses that one) so its isn't + caught by the unrelated .label > span styling meant for label chips. */ +.week-nav .week-label { font-size: 14px; font-weight: 650; min-width: 11.5em; text-align: center; white-space: nowrap; } +.week-label small { color: var(--faint); font-weight: 600; font-size: 11px; margin-left: 2px; } .days { list-style: none; margin: 0; padding: 0; } .day { display: grid; grid-template-columns: 3.2em 4.2em 1fr; align-items: center; gap: 8px; min-height: 50px; padding: 0 14px; } .day + .day { border-top: 1px solid var(--border); } @@ -614,11 +669,18 @@ details[open] > summary { margin-bottom: 8px; } .day-date { color: var(--faint); font-size: 13px; } .day-who { overflow: hidden; text-overflow: ellipsis; white-space: nowrap; } .day-who.nobody { color: var(--faint); font-style: italic; } +/* A run of several days held by the same person (or left empty), replacing + what used to be one identical row per day — see weekRuns() in oncall.js. */ +.day.range { grid-template-columns: 1fr auto; } +.day-range { font-weight: 650; } .day.today { background: var(--accent-soft); } .day.today:first-child { border-radius: var(--radius) var(--radius) 0 0; } .day.today:last-child { border-radius: 0 0 var(--radius) var(--radius); } -.day.today .day-name { color: var(--accent); } +.day.today .day-name, .day.today .day-range { color: var(--accent); } .day.past { opacity: 0.6; } +/* Highlights whichever row is yours, same soft tint as .today — they already + read fine layered (today's own row is almost always one of yours too). */ +.day.mine { background: var(--accent-soft); } .shift-list { list-style: none; margin: 0; padding: 0; } .shift-list li { display: flex; justify-content: space-between; padding: 12px 14px; } @@ -675,9 +737,11 @@ kbd { display: flex; align-items: center; gap: 10px; padding: 4px 10px 18px; font-size: 18px; font-weight: 750; letter-spacing: -0.01em; } - .nav-team-selector { margin: -8px 10px 14px; width: calc(100% - 20px); } + .nav-team-selector { display: inline-flex; margin: -8px 10px 14px; width: calc(100% - 20px); } + .nav-link-secondary { display: flex; } + .nav-more-btn { display: none; } .nav-link { - flex-direction: row; justify-content: flex-start; gap: 12px; + flex: none; flex-direction: row; justify-content: flex-start; gap: 12px; min-height: 40px; padding: 0 10px; border-radius: var(--radius-sm); color: var(--muted); font-size: 14px; } @@ -701,6 +765,7 @@ kbd { is hidden, so the chips wrap here instead: Archived stays reachable. */ .pane-list .chips { flex-wrap: wrap; overflow-x: visible; } .pane-list .chip-sep { display: none; } + .chips-fade { display: none; } .view-queue:not(.has-detail) .pane-detail { display: block; } /* On desktop the list stays visible next to the detail. */ @@ -966,6 +1031,7 @@ button.rota-week:hover { background: var(--surface-2); color: var(--text); } .overview-head { display: flex; align-items: baseline; gap: 8px; } .overview-count { margin-left: auto; color: var(--muted); font-size: 18px; font-weight: 700; } .overview-item p { margin: 4px 0 0; } +.overview-note-icon { width: 13px; height: 13px; vertical-align: -2px; color: var(--ok); } /* ---------- stats page ---------- */ diff --git a/internal/web/static/index.html b/internal/web/static/index.html index 3dce7cc..ccbe355 100644 --- a/internal/web/static/index.html +++ b/internal/web/static/index.html @@ -102,7 +102,10 @@ Alerts - + + Stats @@ -113,22 +116,26 @@ - - + Account + +
    -

    Queue

    diff --git a/internal/web/static/js/alerts.js b/internal/web/static/js/alerts.js index c53ec2c..19ebb7f 100644 --- a/internal/web/static/js/alerts.js +++ b/internal/web/static/js/alerts.js @@ -60,7 +60,11 @@ function render() { let body; if (error && !items) body = h('div', { class: 'load-error', text: error }); else if (!items) body = spinner(); - else if (!items.length) body = emptyState(filter === 'firing' ? 'Nothing firing' : 'No alerts', '', filter === 'firing' ? 'checkCircle' : null); + else if (!items.length) { + body = filter === 'firing' + ? emptyState('Nothing firing', 'No alerts are currently firing.', 'checkCircle') + : emptyState('No alerts', 'None match this filter.', 'bell'); + } else { body = h('div', { class: 'list' }, error && h('div', { class: 'load-error', text: `Showing older data: ${error}` }), diff --git a/internal/web/static/js/app.js b/internal/web/static/js/app.js index ace9f8c..1f8bdfa 100644 --- a/internal/web/static/js/app.js +++ b/internal/web/static/js/app.js @@ -36,16 +36,17 @@ const SECTIONS = { device: { title: 'Sign in a terminal', view: device }, }; -// The mobile hamburger menu's contents — the same sections the desktop -// sidebar's .nav-link list carries in index.html, in the same order. +// The desktop sidebar's .nav-link list in index.html, in the same order. +// `secondary` marks the ones that fold into the phone bottom bar's "More" +// sheet (openNavMenu below) instead of getting a tab of their own there. const NAV_ITEMS = [ { path: '/', section: 'queue', label: 'Queue', icon: 'queueList' }, { path: '/oncall', section: 'oncall', label: 'On-call', icon: 'calendar' }, { path: '/alerts', section: 'alerts', label: 'Alerts', icon: 'bell' }, - { path: '/stats', section: 'stats', label: 'Stats', icon: 'chart' }, + { path: '/stats', section: 'stats', label: 'Stats', icon: 'chart', secondary: true }, { path: '/team', section: 'team', label: 'Team', icon: 'team' }, - { path: '/admin', section: 'admin', label: 'Admin', icon: 'shield', adminOnly: true }, - { path: '/more', section: 'more', label: 'Account', icon: 'user' }, + { path: '/admin', section: 'admin', label: 'Admin', icon: 'shield', adminOnly: true, secondary: true }, + { path: '/more', section: 'more', label: 'Account', icon: 'user', secondary: true }, ]; function parseRoute(pathname) { @@ -152,15 +153,16 @@ function render() { // ---------- nav menu ---------- -// The mobile hamburger menu: same shape as the sheet-based action menus in -// incident.js (openSheet + a