Make every clickable control look clickable, in two shared styles (closes #375)

Buttons keep btn-primary, btn-secondary and btn-danger and gain a
pointer cursor; the site name and the navigation links become
btn-secondary buttons. Every control that was plain coloured text
(Copy, both Add, the row actions, Resubmit, Replay, the back, footer
and download links) now uses btn-small, bordered at rest with hover
and focus states. Each card on the webhook list shows an Open label in
btn-small and takes its focus outline. Both styles are in
static/css/style.css, which the layout now loads.

The event log's clickable rows become buttons, so they work by
keyboard; Replay moves beside its delivery's row. Rows on the webhook
page and in the event log wrap at phone width. The browser test now
checks that Copy reads "Copied".

Model: opus-5-5

cmd/webhooker/main_test.go

@@ -14,7 +14,6 @@ import (
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
-	"sneak.berlin/go/webhooker/internal/config"
 	"sneak.berlin/go/webhooker/internal/datadir"
 	"sneak.berlin/go/webhooker/internal/resetpw"
 	"sneak.berlin/go/webhooker/internal/server"
@@ -37,7 +36,6 @@ const dockerStopGrace = 10 * time.Second
 // fx.New applies options before it executes invokes, so the timeout
 // is set whether or not the graph itself can be constructed here.
 func TestNewApp_StopTimeout(t *testing.T) {
-	config.ClearEnvForTest(t)
 	t.Setenv("DATA_DIR", t.TempDir())
 
 	got := newApp().StopTimeout()
@@ -75,7 +73,6 @@ func freePort(t *testing.T) int {
 // anything is built, and the run of logger.New, which happens before
 // the configuration sets the level.
 func TestNewApp_SendsFxEventsToTheLogger(t *testing.T) {
-	config.ClearEnvForTest(t)
 	t.Setenv("DATA_DIR", t.TempDir())
 	t.Setenv("PORT", strconv.Itoa(freePort(t)))
 	t.Setenv("DEBUG", "true")

internal/delivery/engine_integration_test.go

@@ -355,14 +355,9 @@ func TestProcessRetryTask_SuccessfulRetry(t *testing.T) {
 
 	s := newISetup(t)
 
-	var receivedBody string
-
 	ts := httptest.NewServer(
 		http.HandlerFunc(
-			func(w http.ResponseWriter, r *http.Request) {
+			func(w http.ResponseWriter, _ *http.Request) {
-				body, _ := io.ReadAll(r.Body)
-				receivedBody = string(body)
-
 				w.WriteHeader(http.StatusOK)
 			},
 		),
@@ -402,8 +397,6 @@ func TestProcessRetryTask_SuccessfulRetry(t *testing.T) {
 		context.TODO(), &task,
 	)
 
-	assert.Equal(t, event.Body, receivedBody)
-
 	iAssertStatus(t, s.WebhookDB, d.ID,
 		database.DeliveryStatusDelivered,
 	)
@@ -450,14 +443,9 @@ func TestProcessRetryTask_LargeBody_FetchFromDB(
 
 	s := newISetup(t)
 
-	var receivedBody string
-
 	ts := httptest.NewServer(
 		http.HandlerFunc(
-			func(w http.ResponseWriter, r *http.Request) {
+			func(w http.ResponseWriter, _ *http.Request) {
-				body, _ := io.ReadAll(r.Body)
-				receivedBody = string(body)
-
 				w.WriteHeader(http.StatusOK)
 			},
 		),
@@ -494,8 +482,6 @@ func TestProcessRetryTask_LargeBody_FetchFromDB(
 		context.TODO(), &task,
 	)
 
-	assert.Equal(t, largeBody, receivedBody)
-
 	iAssertStatus(t, s.WebhookDB, d.ID,
 		database.DeliveryStatusDelivered,
 	)

internal/logfield/logfield_test.go

@@ -5,7 +5,6 @@ import (
 	"io"
 	"log/slog"
 	"strings"
-	"sync"
 	"testing"
 	"unicode/utf8"
 
@@ -19,11 +18,15 @@ import (
 // width.
 const budget = 64
 
-// batchRunes is how many consecutive code points the charge test logs
+// sampleRunes is how many runes wide the values in the charge test
-// in one value from U+1000 up. Logging each of those on its own line
+// are. The handlers add a constant per field — a pair of quotes when
-// is too slow for the suite under the race detector; 4,096 at a time
+// the value needs quoting — so the per-rune charge is only visible
-// is 271 batches, each logged on two lines, so 542 lines per handler.
+// once it is amortised over a run of them.
-const batchRunes = 4096
+const sampleRunes = 64
+
+// quotingSlack is that constant: the pair of quotes a handler adds to
+// a value that needs them and omits from one that does not.
+const quotingSlack = 2
 
 // newHandlers are the two handlers internal/logger can install. Time
 // is dropped so a line's width is a function of its value alone —
@@ -63,48 +66,46 @@ func renderedWidth(
 	return buf.Len()
 }
 
-// emittedBytes is what a handler writes for the runes of s alone, in a
+// chargeTestRunes is the set of code points the charge test measures:
-// value that starts with prefix: the width of a line carrying prefix
+// every rune in the first two planes' worth of the BMP that the
-// and then s twice, less that of a line carrying prefix and s once.
+// handlers are most likely to treat specially, the separators that
-// Both values start the same way and hold the same runes, so the text
+// only slog's JSON handler escapes, and a stratified sample across
-// handler quotes both or neither, and the quotes cancel along with the
+// the rest of Unicode so the astral charge is exercised on more than
-// prefix and everything else on the line.
+// one hand-picked rune.
-func emittedBytes(
+func chargeTestRunes() []rune {
-	newHandler func(io.Writer) slog.Handler,
+	const (
-	prefix, s string,
+		denseCeiling = 0x800
-) int {
+		stride       = 1021
-	return renderedWidth(newHandler, prefix+s+s) -
+		surrogateLo  = 0xD800
-		renderedWidth(newHandler, prefix+s)
+		surrogateHi  = 0xDFFF
-}
+	)
 
-// firstUndercharged returns the first rune in s that the handler
+	var runes []rune
-// writes in more bytes than EncodedBytes charges for it, and how many
-// runes in s are undercharged that way. It measures one rune per line,
-// in a value of that rune alone and again after a space, which makes
-// the text handler quote the value. The charge test calls it on the
-// code points below U+1000, and from there up only on a batch that has
-// already failed, to name the code points rather than just their range.
-func firstUndercharged(
-	newHandler func(io.Writer) slog.Handler,
-	s string,
-) (rune, int) {
-	first, count := rune(-1), 0
 
-	for _, r := range s {
+	keep := func(r rune) {
-		charge := logfield.EncodedBytes(r)
+		if r >= surrogateLo && r <= surrogateHi {
-		if emittedBytes(newHandler, "", string(r)) <= charge &&
+			return
-			emittedBytes(newHandler, " ", string(r)) <= charge {
-			continue
 		}
 
-		if count == 0 {
+		runes = append(runes, r)
-			first = r
 	}
 
-		count++
+	for r := range rune(denseCeiling) {
+		keep(r)
 	}
 
-	return first, count
+	for _, r := range []rune{
+		0x2028, 0x2029, 0x200B, 0x4E00, 0xE000, 0xFFFD,
+		0x1000C, 0x1F600, 0xE0001, 0x10FFFF,
+	} {
+		keep(r)
+	}
+
+	for r := rune(denseCeiling); r <= utf8.MaxRune; r += stride {
+		keep(r)
+	}
+
+	return runes
 }
 
 // TestEncodedBytes_ChargesAtLeastWhatTheHandlersEmit is the property
@@ -113,93 +114,33 @@ func firstUndercharged(
 // how a stated ceiling becomes false without any test noticing, so
 // the charge is measured against what the handlers actually write
 // rather than against the escaping rules as read.
-//
-// Every code point below U+1000 is checked on its own, for both
-// handlers. That range holds the quote, the backslash and the control
-// characters the handlers escape, next to code points each handler
-// writes in fewer bytes than their charge, which in a sum would cover
-// a neighbour charged too little. Each is measured in a value of it
-// alone and again in one the text handler quotes, because that handler
-// writes U+007F as one raw byte in a value it leaves bare but as \x7f,
-// four bytes, in one it quotes.
-//
-// From U+1000 up the text handler writes every code point in exactly
-// its charge, so the rest of Unicode is checked batchRunes at a time:
-// each batch's summed charge must cover what the handler writes for
-// the whole batch. The sums there can miss the JSON handler alone
-// writing one code point in more bytes than its charge, when it writes
-// others in the same batch in fewer.
 func TestEncodedBytes_ChargesAtLeastWhatTheHandlersEmit(t *testing.T) {
 	t.Parallel()
 
-	var below strings.Builder
-	for r := range rune(0x1000) {
-		below.WriteRune(r)
-	}
-
-	var batches []string
-
-	for lo := rune(0x1000); lo <= utf8.MaxRune; lo += batchRunes {
-		var batch strings.Builder
-
-		for r := lo; r < lo+batchRunes; r++ {
-			// Surrogate halves are not runes a string can carry.
-			if utf8.ValidRune(r) {
-				batch.WriteRune(r)
-			}
-		}
-
-		batches = append(batches, batch.String())
-	}
-
-	// What EncodedBytes charges for each batch. Under -race -cover this
-	// takes longer than logging the batches, so it is worked out once,
-	// by whichever handler finishes logging first, while the other is
-	// still logging.
-	charged := sync.OnceValue(func() []int {
-		costs := make([]int, len(batches))
-
-		for i, batch := range batches {
-			for _, r := range batch {
-				costs[i] += logfield.EncodedBytes(r)
-			}
-		}
-
-		return costs
-	})
-
 	for name, newHandler := range newHandlers() {
 		t.Run(name, func(t *testing.T) {
 			t.Parallel()
 
-			if first, count := firstUndercharged(newHandler, below.String()); count > 0 {
+			// 'a' is a printable ASCII rune, charged exactly one
-				t.Errorf(
+			// byte, so it is the zero point the other runes are
-					"%d code points below U+1000 cost more than "+
+			// measured against.
-						"EncodedBytes charges, the first U+%04X",
+			base := renderedWidth(
-					count, first,
+				newHandler, strings.Repeat("a", sampleRunes),
 			)
-			}
 
-			emitted := make([]int, len(batches))
+			for _, r := range chargeTestRunes() {
-			for i, batch := range batches {
+				got := renderedWidth(
-				emitted[i] = emittedBytes(newHandler, "", batch)
+					newHandler,
-			}
+					strings.Repeat(string(r), sampleRunes),
-
-			for i, cost := range charged() {
-				if emitted[i] <= cost {
-					continue
-				}
-
-				lo := rune(0x1000 + i*batchRunes)
-				first, count := firstUndercharged(
-					newHandler, batches[i],
 				)
-				t.Errorf(
+				charged := sampleRunes *
-					"U+%04X to U+%04X emit %d bytes but are "+
+					(logfield.EncodedBytes(r) - 1)
-						"charged %d; %d of them cost more than "+
+
-						"EncodedBytes charges, the first U+%04X",
+				require.LessOrEqual(
-					lo, lo+batchRunes-1, emitted[i], cost,
+					t, got-base, charged+quotingSlack,
-					count, first,
+					"U+%04X costs more on the line than "+
+						"EncodedBytes charges for it",
+					r,
 				)
 			}
 		})

internal/middleware/middleware.go

@@ -600,10 +600,7 @@ func bodyLimitedMethod(method string) bool {
 }
 
 // MaxBodySize returns middleware that limits the size of
-// POST/PUT/PATCH request bodies to maxBytes. A request with any other
+// POST/PUT/PATCH request bodies to maxBytes. It must be registered
-// method passes through uncapped, deliberately: no route behind it
-// reads a body on GET, HEAD or DELETE. A handler that starts to needs
-// its method added to bodyLimitedMethod first. It must be registered
 // before any middleware that parses the body — notably CSRF, which
 // calls r.PostFormValue — so that form parsing happens under this
 // cap rather than net/http's 10 MB default.

internal/middleware/middleware_test.go

@@ -730,8 +730,10 @@ func TestNoCache_SetsHeaders(t *testing.T) {
 
 const testBodyLimit int64 = 64
 
-// maxBodySizeResult is what runMaxBodySize's sentinel handler saw,
+// maxBodySizeHandler wraps a sentinel handler in MaxBodySize with
-// together with the response.
+// testBodyLimit. The sentinel records whether it ran and how much of
+// the body it managed to read, so tests can distinguish "never
+// reached" from "reached but truncated".
 type maxBodySizeResult struct {
 	called   bool
 	read     int
@@ -739,10 +741,6 @@ type maxBodySizeResult struct {
 	response *httptest.ResponseRecorder
 }
 
-// runMaxBodySize wraps a sentinel handler in MaxBodySize with
-// testBodyLimit and serves req through it. The sentinel records
-// whether it ran and how much of the body it managed to read, so
-// tests can distinguish "never reached" from "reached but truncated".
 func runMaxBodySize(
 	t *testing.T,
 	req *http.Request,

internal/server/error_page_test.go

@@ -191,7 +191,7 @@ func TestErrorPage_PanicOnAdminPage(t *testing.T) {
 
 	w := serve(
 		server.NewRouterWithPageProbeForTest(
-			t, env.log, env.cfg, env.mw, env.hnd,
+			env.log.Get(), env.cfg, env.mw, env.hnd,
 			true, panicProbeHandler,
 		),
 		server.PageProbePattern,
@@ -200,7 +200,7 @@ func TestErrorPage_PanicOnAdminPage(t *testing.T) {
 
 	w = serve(
 		server.NewRouterWithProbeForTest(
-			t, env.log, env.cfg, env.mw, env.hnd,
+			env.log.Get(), env.cfg, env.mw, env.hnd,
 			true, panicProbeHandler,
 		),
 		server.ProbePattern,

internal/server/export_test.go

@@ -1,16 +1,13 @@
 package server
 
 import (
+	"log/slog"
 	"net/http"
-	"testing"
 
 	"github.com/getsentry/sentry-go"
 	"github.com/go-chi/chi"
-	"github.com/stretchr/testify/require"
-	"go.uber.org/fx/fxtest"
 	"sneak.berlin/go/webhooker/internal/config"
 	"sneak.berlin/go/webhooker/internal/handlers"
-	"sneak.berlin/go/webhooker/internal/logger"
 	"sneak.berlin/go/webhooker/internal/middleware"
 )
 
@@ -37,45 +34,23 @@ func SentryClientOptionsForTest(
 	return sentryClientOptions(dsn, release)
 }
 
-// newServerForTest builds a Server through New, as the application
-// does, on a lifecycle that is never started: the hooks New adds to
-// it never run, so nothing listens.
-func newServerForTest(
-	t *testing.T,
-	log *logger.Logger,
-	cfg *config.Config,
-	mw *middleware.Middleware,
-	h *handlers.Handlers,
-) *Server {
-	t.Helper()
-
-	s, err := New(fxtest.NewLifecycle(t), ServerParams{
-		Logger:     log,
-		Config:     cfg,
-		Middleware: mw,
-		Handlers:   h,
-	})
-	require.NoError(t, err)
-
-	return s
-}
-
 // NewRouterForTest builds the real route tree via SetupRoutes with
-// the supplied middleware and handlers, on a Server from New whose
+// the supplied middleware and handlers, bypassing the fx lifecycle
-// lifecycle is never started, so no HTTP listener runs. Tests use it
+// and the HTTP listener. Tests use it so that route-group middleware
-// so that route-group middleware registration order is exercised
+// registration order is exercised exactly as it ships, rather than
-// exactly as it ships, rather than against a hand-rebuilt chain that
+// against a hand-rebuilt chain that could drift from routes.go.
-// could drift from routes.go.
 func NewRouterForTest(
-	t *testing.T,
+	log *slog.Logger,
-	log *logger.Logger,
 	cfg *config.Config,
 	mw *middleware.Middleware,
 	h *handlers.Handlers,
 ) http.Handler {
-	t.Helper()
+	s := &Server{
-
+		log:    log,
-	s := newServerForTest(t, log, cfg, mw, h)
+		mw:     mw,
+		h:      h,
+		params: ServerParams{Config: cfg},
+	}
 	s.SetupRoutes()
 
 	return s.router
@@ -108,17 +83,19 @@ const ProbePattern = "/probe"
 // option and the recoverer registered outside it is the thing a test
 // has to be able to pin.
 func NewRouterWithProbeForTest(
-	t *testing.T,
+	log *slog.Logger,
-	log *logger.Logger,
 	cfg *config.Config,
 	mw *middleware.Middleware,
 	h *handlers.Handlers,
 	sentryEnabled bool,
 	probe http.HandlerFunc,
 ) http.Handler {
-	t.Helper()
+	s := &Server{
-
+		log:    log,
-	s := newServerForTest(t, log, cfg, mw, h)
+		mw:     mw,
+		h:      h,
+		params: ServerParams{Config: cfg},
+	}
 	s.sentryEnabled.Store(sentryEnabled)
 	s.SetupRoutes()
 	s.router.Handle(ProbePattern, probe)
@@ -136,17 +113,19 @@ const PageProbePattern = "/pages/probe"
 // it, so the probe runs behind that group's own middleware exactly as
 // the group's real routes do.
 func NewRouterWithPageProbeForTest(
-	t *testing.T,
+	log *slog.Logger,
-	log *logger.Logger,
 	cfg *config.Config,
 	mw *middleware.Middleware,
 	h *handlers.Handlers,
 	sentryEnabled bool,
 	probe http.HandlerFunc,
 ) http.Handler {
-	t.Helper()
+	s := &Server{
-
+		log:    log,
-	s := newServerForTest(t, log, cfg, mw, h)
+		mw:     mw,
+		h:      h,
+		params: ServerParams{Config: cfg},
+	}
 	s.sentryEnabled.Store(sentryEnabled)
 	s.SetupRoutes()
 

internal/server/recoverer_test.go

@@ -199,7 +199,7 @@ func TestPanicProbeChild(t *testing.T) {
 	env := newTestEnv(t)
 
 	router := server.NewRouterWithProbeForTest(
-		t, env.log, env.cfg, env.mw, env.hnd,
+		env.log.Get(), env.cfg, env.mw, env.hnd,
 		false, panicProbeHandler,
 	)
 
@@ -253,7 +253,7 @@ func TestSentryStillSeesAPanic(t *testing.T) {
 	require.NoError(t, err)
 
 	router := server.NewRouterWithProbeForTest(
-		t, env.log, env.cfg, env.mw, env.hnd,
+		env.log.Get(), env.cfg, env.mw, env.hnd,
 		true, panicProbeHandler,
 	)
 

internal/server/response_controller_test.go

@@ -67,11 +67,11 @@ func TestResponseControllerThroughProductionRouter(t *testing.T) {
 
 			routers := map[string]http.Handler{
 				server.ProbePattern: server.NewRouterWithProbeForTest(
-					t, env.log, env.cfg, env.mw, env.hnd,
+					env.log.Get(), env.cfg, env.mw, env.hnd,
 					tc.sentryEnabled, probe,
 				),
 				server.PageProbePattern: server.NewRouterWithPageProbeForTest(
-					t, env.log, env.cfg, env.mw, env.hnd,
+					env.log.Get(), env.cfg, env.mw, env.hnd,
 					tc.sentryEnabled, probe,
 				),
 			}

internal/server/routes_test.go

@@ -136,7 +136,7 @@ func newTestEnvWithConfig(
 	t.Cleanup(app.RequireStop)
 
 	return &testEnv{
-		router: server.NewRouterForTest(t, log, cfg, mw, hnd),
+		router: server.NewRouterForTest(log.Get(), cfg, mw, hnd),
 		sess:   sess,
 		db:     db,
 		dbMgr:  dbMgr,
@@ -531,48 +531,6 @@ func TestStaticServesOnlyGetAndHead(t *testing.T) {
 	}
 }
 
-// --- every page route group ---
-
-// TestPageRouteGroups_OversizeBody_RejectedBeforeCSRF pins the body
-// cap ahead of CSRF and RequireAuth in every page route group. The
-// requests carry no session and no CSRF token, so if either ran first
-// the answer would be a 403 or a redirect to the login page rather
-// than 413, and CSRF would issue its cookie (see
-// TestPagesLogin_UnderLimit_NoToken_CSRFRejects). /settings has no
-// POST route, but its group's middleware runs before the method is
-// matched, so a POST there still reaches CSRF's form parsing if the
-// cap moves after it. The user and webhook in the paths need not
-// exist: nothing after the cap runs.
-func TestPageRouteGroups_OversizeBody_RejectedBeforeCSRF(
-	t *testing.T,
-) {
-	t.Parallel()
-
-	env := newTestEnv(t)
-
-	form := url.Values{}
-	form.Set("name", oversizeValue())
-
-	for _, path := range []string{
-		"/pages/login",
-		"/user/nobody/password",
-		"/settings/",
-		"/hooks/new",
-		"/hook/nonexistent/edit",
-	} {
-		w := env.post(path, form, nil)
-
-		assert.Equal(
-			t, http.StatusRequestEntityTooLarge, w.Code, path,
-		)
-		assert.False(
-			t, csrfCookieSet(w),
-			"CSRF middleware must not run for an oversized body to %s",
-			path,
-		)
-	}
-}
-
 // --- /pages group ---
 
 // TestPagesLogin_OversizeBody_RejectedBeforeCSRF proves the cap runs
@@ -1652,7 +1610,7 @@ func TestTwoMetricsRoutersInOneProcess(t *testing.T) {
 	)
 	third := &testEnv{
 		router: server.NewRouterForTest(
-			t, first.log, first.cfg, first.mw, first.hnd,
+			first.log.Get(), first.cfg, first.mw, first.hnd,
 		),
 	}