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