From 4df5a508e48b804a4e20c2728f0f9bfb18603d2e Mon Sep 17 00:00:00 2001 From: Derek Reynolds Date: Wed, 10 Jun 2026 06:52:17 -0700 Subject: [PATCH] refactor: clean up small Go idiom nits (P2.10) Batch of mechanical cleanups flagged in the prelaunch review: keepalive Timer+Reset -> Ticker; fail loudly on fs.Sub error; meaningful 500 messages with http.StatusInternalServerError; IdleTimeout on the public server (WriteTimeout deliberately omitted for SSE, now documented) and ReadHeaderTimeout on the admin listener; errors.New / strconv.FormatInt over fmt; CountMatches checks err before interpreting n; merged duplicate viewer doc comment; dropped no-op .Funcs; removed non-load-bearing cch drains; moved countResult next to its use in render.go. Co-Authored-By: Claude Fable 5 --- auth.go | 6 ++++-- db.go | 8 +++++--- main.go | 31 +++++++++++++++++-------------- render.go | 19 ++++++++++++------- tasks/prelaunch-checklist.md | 2 +- 5 files changed, 39 insertions(+), 27 deletions(-) diff --git a/auth.go b/auth.go index eca1344..fe7d1dc 100644 --- a/auth.go +++ b/auth.go @@ -9,6 +9,7 @@ import ( "encoding/base64" "encoding/hex" "encoding/json" + "errors" "fmt" "io" "log" @@ -16,6 +17,7 @@ import ( "net/url" "os" "path/filepath" + "strconv" "strings" "time" ) @@ -214,7 +216,7 @@ func (a *Auth) logout(w http.ResponseWriter, r *http.Request) { func (a *Auth) exchange(ctx context.Context, code string) (string, error) { if code == "" { - return "", fmt.Errorf("no code") + return "", errors.New("no code") } q := url.Values{ "client_id": {a.clientID}, @@ -268,7 +270,7 @@ func (a *Auth) fetchUser(ctx context.Context, token string) (*AuthUser, error) { if name == "" { name = gh.Login } - return &AuthUser{Provider: "github", ID: fmt.Sprint(gh.ID), Login: gh.Login, Name: name, Avatar: gh.Avatar}, nil + return &AuthUser{Provider: "github", ID: strconv.FormatInt(gh.ID, 10), Login: gh.Login, Name: name, Avatar: gh.Avatar}, nil } func randHex(n int) string { diff --git a/db.go b/db.go index 3355e00..be30956 100644 --- a/db.go +++ b/db.go @@ -318,11 +318,13 @@ func (s *store) CountMatches(ctx context.Context, q Query) (n int, capped bool, return 0, false, err } args = append(args, CountCap+1) - err = s.db.QueryRowContext(ctx, `SELECT COUNT(*) FROM (SELECT 1 FROM issues i `+where+` LIMIT ?)`, args...).Scan(&n) + if err := s.db.QueryRowContext(ctx, `SELECT COUNT(*) FROM (SELECT 1 FROM issues i `+where+` LIMIT ?)`, args...).Scan(&n); err != nil { + return 0, false, err + } if n > CountCap { - return CountCap, true, err + return CountCap, true, nil } - return n, false, err + return n, false, nil } // ListIssues returns the visible window (q.Limit rows). It fetches one extra row diff --git a/main.go b/main.go index 953eff5..a9836a9 100644 --- a/main.go +++ b/main.go @@ -213,7 +213,10 @@ func main() { // ---- public listener: the app, stream, and commands ---- pub := http.NewServeMux() - staticSub, _ := fs.Sub(staticFS, "static") + staticSub, err := fs.Sub(staticFS, "static") + if err != nil { + log.Fatal(err) + } staticFile := http.StripPrefix("/static/", http.FileServer(http.FS(staticSub))) pub.Handle("GET /static/", http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { // Embedded, self-hosted assets (incl. datastar.js — no CDN, no SRI gap). @@ -291,13 +294,21 @@ func main() { }) go func() { log.Printf("admin/debug on http://%s (local only)", *adminAddr) - if err := http.ListenAndServe(*adminAddr, admin); err != nil { + adminSrv := &http.Server{Addr: *adminAddr, Handler: admin, ReadHeaderTimeout: 10 * time.Second} + if err := adminSrv.ListenAndServe(); err != nil { log.Printf("admin listener: %v", err) } }() // ---- serve the public mux: HTTPS via autocert if a domain is set ---- - srv := &http.Server{Handler: a.withSecurity(pub), ReadHeaderTimeout: 10 * time.Second} + // WriteTimeout is deliberately omitted: it would kill the long-lived SSE + // streams. IdleTimeout only bounds keep-alive connections between requests, + // so it's safe alongside SSE. + srv := &http.Server{ + Handler: a.withSecurity(pub), + ReadHeaderTimeout: 10 * time.Second, + IdleTimeout: 120 * time.Second, + } serveErr := make(chan error, 1) if *tlsDomain != "" { mgr := &autocert.Manager{ @@ -492,7 +503,7 @@ func (a *app) renderPage(w http.ResponseWriter, r *http.Request, initial Session rs, err := a.renderRegions(r.Context(), initial, vw) if err != nil { log.Printf("page render: %v", err) - http.Error(w, "error", 500) + http.Error(w, "page render failed", http.StatusInternalServerError) return } w.Header().Set("Content-Type", "text/html; charset=utf-8") @@ -506,7 +517,7 @@ func (a *app) renderPage(w http.ResponseWriter, r *http.Request, initial Session }{vw, assetVer, seedSelect, template.HTML(rs.facets), template.HTML(rs.list), template.HTML(rs.detail)} if err := tmpl.ExecuteTemplate(w, "index.html", data); err != nil { log.Printf("page: %v", err) - http.Error(w, "error", 500) + http.Error(w, "page render failed", http.StatusInternalServerError) } } @@ -590,7 +601,7 @@ func (a *app) handleStream(w http.ResponseWriter, r *http.Request) { // no-op after each projection), and first paint is server-rendered, so no // connect-time burst is needed here. `_ka` is underscore-prefixed → never // sent back to the server nor included in the stream URL. - ka := time.NewTimer(15 * time.Second) + ka := time.NewTicker(15 * time.Second) defer ka.Stop() kaCount := 0 for { @@ -606,18 +617,10 @@ func (a *app) handleStream(w http.ResponseWriter, r *http.Request) { return } kaCount++ - ka.Reset(15 * time.Second) } } } -// countResult carries the concurrently-computed match count. -type countResult struct { - n int - capped bool - err error -} - // streamState remembers the per-region hash of what this connection last sent. // A region whose bytes are unchanged is left out of the next patch, so (a) idle // connections cost ~0 under fan-out, and (b) an actively-edited form in an diff --git a/render.go b/render.go index 4b68c88..08a4df4 100644 --- a/render.go +++ b/render.go @@ -16,9 +16,8 @@ import ( // viewer is the server-side auth context for a request/connection. It drives // auth-gated *rendering* (the server decides what to emit); enforcement still -// happens in the write handlers. No client signals involved. -// viewer is the server-side auth context. Exported fields so the index template -// can render the header directly. (Defined here next to the other view types.) +// happens in the write handlers. No client signals involved. Exported fields so +// the index template can render the header directly. type viewer struct { Login string // current identity ("" if none) Avatar string @@ -32,7 +31,7 @@ var tmplFS embed.FS //go:embed static/* var staticFS embed.FS -var tmpl = template.Must(template.New("").Funcs(template.FuncMap{}).ParseFS(tmplFS, "templates/*.html")) +var tmpl = template.Must(template.New("").ParseFS(tmplFS, "templates/*.html")) // ---- View models ---- @@ -475,12 +474,21 @@ type regionSet struct { facets, list, detail, stats, palette, help string } +// countResult carries the concurrently-computed match count. +type countResult struct { + n int + capped bool + err error +} + // renderRegions renders every projection region. Templating stays here; the // handler just dedups + sends. func (a *app) renderRegions(ctx context.Context, state SessionState, vw viewer) (regionSet, error) { q := state.toQuery() // The match count is the slow part; compute it concurrently with the rest. + // cch is buffered, so on the error returns below the goroutine still + // completes its send and exits — no drain needed. cch := make(chan countResult, 1) go func() { n, capped, err := a.counts.Get(ctx, q) @@ -489,12 +497,10 @@ func (a *app) renderRegions(ctx context.Context, state SessionState, vw viewer) list, shown, err := a.lists.Get(ctx, q) // shared cache; pre-rendered HTML if err != nil { - <-cch return regionSet{}, fmt.Errorf("list: %w", err) } dv, err := buildDetailView(ctx, a.reads, a.catalog, state, vw) if err != nil { - <-cch return regionSet{}, err } if dv.Selected { // realtime presence: who's looking at this issue right now @@ -502,7 +508,6 @@ func (a *app) renderRegions(ctx context.Context, state SessionState, vw viewer) } detail, err := renderToString("detail", dv) if err != nil { - <-cch return regionSet{}, err } cr := <-cch diff --git a/tasks/prelaunch-checklist.md b/tasks/prelaunch-checklist.md index e0ad418..af09a9d 100644 --- a/tasks/prelaunch-checklist.md +++ b/tasks/prelaunch-checklist.md @@ -82,7 +82,7 @@ Verified clean at review time: `go vet ./...`, `gofmt -l .`, `go test -race ./.. - [x] **P2.9 Dead code + tooling** — `db.go:17-21` (`type User` unused), `session.go:246-251` (`Hub.get` never called), `main.go:882` (`writerName`'s `state` param unused — also drop the `sess.snapshot()` at `main.go:994`). Install staticcheck and add it to `make check`. Also fix `Makefile:25`: `check` runs `gofmt -w` (mutates the tree); a gate should fail instead — `test -z "$$(gofmt -l .)"`. -- [ ] **P2.10 Small idiom nits (one commit)** — +- [x] **P2.10 Small idiom nits (one commit)** — - `main.go:563-579`: keepalive uses `time.Timer` + manual `Reset`; use `time.NewTicker(15*time.Second)` + `defer ticker.Stop()`. - `main.go:201`: `staticSub, _ := fs.Sub(...)` — fail loudly (`log.Fatal`) on error. - `main.go:466,479`: `http.Error(w, "error", 500)` → `http.StatusInternalServerError` with a meaningful message. -- 2.51.2