From 58014eb8dd5c598279864eee68baebebdf99ce67 Mon Sep 17 00:00:00 2001 From: Sulthan Zaki Date: Sun, 16 Aug 2026 14:31:00 +0700 Subject: [PATCH] review: verdigris patina, honest Lane figures, tests that can fail (#102) Two-axis review found the shipped colour and three weak tests. - --patina was a warm gold at the same hue family as --brass; the spec asked for a cool blue-green so an unhealthy Lane is unmistakably not ember. Now verdigris in both branches, with docs/design-system.md stating why the far side of the wheel is the point. - A Lane pass that returns before computing its figures carries the previous pass's due count and gap forward instead of recording zeroes, and Checked rides beside Due so a stopped Lane is distinguishable from a quiet one. - TestAdminPageWithoutAPollerSaysSo now separates the two causes it conflated, TestOwnerClearsReaderMarks seeds real counters so the clearing assertion can fail, and TestLaneStatus asserts Checked and the carry-forward. - backend/AGENTS.md records the one deliberate owner comparison outside requireOwner and the carry-forward rule. --- backend/AGENTS.md | 9 ++ backend/internal/latest/poller.go | 14 ++- backend/internal/latest/poller_test.go | 21 ++++ backend/internal/latest/status.go | 25 +++-- backend/internal/web/admin.go | 36 +++++-- backend/internal/web/static/style.css | 11 +- backend/internal/web/templates/admin.html | 5 +- backend/internal/web/templates/lanes.html | 13 ++- backend/web_test.go | 123 +++++++++++++++++----- docs/design-system.md | 7 +- 10 files changed, 203 insertions(+), 61 deletions(-) diff --git a/backend/AGENTS.md b/backend/AGENTS.md index 8315b44..6c3c5d3 100644 --- a/backend/AGENTS.md +++ b/backend/AGENTS.md @@ -224,3 +224,12 @@ Guidance for OpenCode (and Claude Code) working under `backend/`. See root `AGEN rather than zeroes. `main.newRouter` takes the reporter as an interface and converts a nil `*Poller` to a nil interface — a typed nil would make the page claim a poller exists. + The one owner comparison left outside `requireOwner` is in `index` + (`view.Owner = readerID == h.store.OwnerID()`): it gates a link, not an + endpoint, so it is a rendering decision a registration-time wrapper cannot + express — do not "unify" it into the gate. + A Lane pass that returns before computing its figures (refusal backoff, + sidecar down) carries the previous pass's due count and gap forward rather + than recording zeroes; a Lane that has never reached a pace renders no gap at + all. `Checked` next to `Due` is what separates a stopped Lane from a quiet + one, so neither figure may be dropped from the row. diff --git a/backend/internal/latest/poller.go b/backend/internal/latest/poller.go index e0dbe12..90028e0 100644 --- a/backend/internal/latest/poller.go +++ b/backend/internal/latest/poller.go @@ -246,19 +246,23 @@ func (p *Poller) runLanePass(ctx context.Context, name string, paced bool) time. // No fetcher at all right now (browser absent, no fallback): every // Series stays unstamped and due, so a browser that appears after a // restart finds its full queue waiting (issue #100). + st.Gap = defaultGap return defaultGap } due, err := p.Store.DueForLatestCheck(name, now.Add(-s.Rest).UnixMilli()) if err != nil { log.Printf("latest poll %s: due query: %v", name, err) + st.Gap = defaultGap return defaultGap } st.Due = len(due) if s.Browser != nil && f == p.BrowserFetch && !browserWakeDue(due, now, s.Rest) { // Below both thresholds Chrome stays asleep (ADR-0005 on-demand // browser): waking it for a single Poll would cost a challenge solve - // per request. + // per request. The Lane still paces at the default gap, which is what + // the owner's page must show rather than a zero. + st.Gap = defaultGap return defaultGap } if s.Browser != nil { @@ -274,6 +278,7 @@ func (p *Poller) runLanePass(ctx context.Context, name string, paced bool) time. eligible, err := p.Store.EligibleSeriesCount(name) if err != nil { log.Printf("latest poll %s: eligible count: %v", name, err) + st.Gap = defaultGap return defaultGap } gap, clamped := effectiveGap(s, eligible) @@ -288,7 +293,6 @@ func (p *Poller) runLanePass(ctx context.Context, name string, paced bool) time. } refusals := 0 - checked := 0 for i, sr := range due { if ctx.Err() != nil { break @@ -323,10 +327,10 @@ func (p *Poller) runLanePass(ctx context.Context, name string, paced bool) time. } else { refusals = 0 } - checked++ + st.Checked++ } - if checked > 0 { - log.Printf("latest poll %s: due=%d checked=%d", name, len(due), checked) + if st.Checked > 0 { + log.Printf("latest poll %s: due=%d checked=%d", name, len(due), st.Checked) } if refusals >= 2 { p.setRefusalBackoff(name, now.Add(refuseBackoff)) diff --git a/backend/internal/latest/poller_test.go b/backend/internal/latest/poller_test.go index 7cff35e..aaedcc7 100644 --- a/backend/internal/latest/poller_test.go +++ b/backend/internal/latest/poller_test.go @@ -1640,6 +1640,27 @@ func TestLaneStatus(t *testing.T) { if !st.BrowserConfigured || !st.BrowserReachable { t.Fatalf("browser after round = configured=%v reachable=%v, want true/true (sidecar never lost)", st.BrowserConfigured, st.BrowserReachable) } + if asura.Checked != 1 { + t.Fatalf("asura Checked = %d, want the one Series it read", asura.Checked) + } + + // A pass that declines to look (kagane is now in backoff) must not restate + // the figures it never gathered as zeroes: the last real pass's due count + // and pace stand until a pass replaces them. + before := kagane + if before.Due == 0 || before.Gap == 0 { + t.Fatalf("kagane after its refusing pass = %+v, want the figures that pass gathered", before) + } + p.runOnce(context.Background()) + for _, lane := range p.LaneStatus().Lanes { + if lane.Site != "kagane" { + continue + } + if lane.Due != before.Due || lane.Gap != before.Gap { + t.Fatalf("kagane after a skipped pass = due %d gap %s, want the previous pass's %d / %s", + lane.Due, lane.Gap, before.Due, before.Gap) + } + } // A lost sidecar reads as unreachable for the same window the Lanes skip. p.setBrowserDown(now) diff --git a/backend/internal/latest/status.go b/backend/internal/latest/status.go index da8b417..0c0ba08 100644 --- a/backend/internal/latest/status.go +++ b/backend/internal/latest/status.go @@ -3,14 +3,19 @@ package latest import "time" // LaneState is the administrative page's view of one Poll Lane (issue #102): -// what the Lane's last completed pass saw. Due and Gap are filled in as the -// pass computes them, so a pass that returned before reaching a figure (Lane -// in refusal backoff, no fetcher) records a zero in its place. +// what the Lane's last pass saw. Due, Gap and Checked are filled in as the +// pass computes them; a pass that returned before reaching a figure (refusal +// backoff, sidecar down) carries the previous pass's figures forward rather +// than overwriting them with zeroes the page would state as fact. type LaneState struct { - Site string - Due int - LastRun time.Time - Gap time.Duration + Site string + Due int + LastRun time.Time + Gap time.Duration + // Checked is how many Series this pass actually read. A Lane with Series + // due and nothing checked has stopped working; one with nothing due is + // merely quiet, and the page must not draw the two the same (story 13). + Checked int Clamped bool Refusing bool Browser bool @@ -53,11 +58,17 @@ func (p *Poller) LaneStatus() Status { // recordLaneState stores one Lane's last pass for LaneStatus. Called deferred // from runLanePass so every return path records, even a pass that refused. +// A pass that never reached the pace (Gap zero) keeps the last pass's figures: +// the Lane's due count and gap did not become zero because this pass declined +// to look, and the row's own marks say why it declined. func (p *Poller) recordLaneState(st LaneState) { p.mu.Lock() defer p.mu.Unlock() if p.laneStates == nil { p.laneStates = make(map[string]LaneState) } + if prev, ok := p.laneStates[st.Site]; ok && st.Gap == 0 { + st.Due, st.Gap, st.Clamped, st.Checked = prev.Due, prev.Gap, prev.Clamped, prev.Checked + } p.laneStates[st.Site] = st } diff --git a/backend/internal/web/admin.go b/backend/internal/web/admin.go index df5bd4c..efb925b 100644 --- a/backend/internal/web/admin.go +++ b/backend/internal/web/admin.go @@ -31,7 +31,11 @@ type adminView struct { // browser fact, which is shared by the three browser Sites rather than held // once per Site. type lanesView struct { - Rows []laneRow + Rows []laneRow + // PollerOff means no poller is running at all (disabled by config, or its + // client could not be built). The browser line must not answer "not + // configured" then: the sidecar is not the reason nothing is polled. + PollerOff bool BrowserConfigured bool BrowserReachable bool } @@ -40,9 +44,15 @@ type lanesView struct { // template renders strings and flags, and every judgement about what they mean // is made here. type laneRow struct { - Site string - Due int - Ran string + Site string + Due int + Ran string + // Checked is how many Series the last pass read. Due without Checked is a + // Lane that has stopped working; the two figures side by side are what + // separate that from a Lane with nothing to do. + Checked int + // Gap is empty when no pass has reached the pace yet, so the row omits the + // figure instead of stating a zero. Gap string Clamped bool Refusing bool @@ -50,6 +60,9 @@ type laneRow struct { // sidecar while the sidecar is unreachable — including the case where none // is configured, which stops those Series just as completely. BrowserLost bool + // Stalled marks a Lane with Series waiting that its last pass did not read + // — the difference between a stopped Lane and a quiet one (story 13). + Stalled bool // Attention is the one flag the template colours on, so an unhealthy Lane // is found at a glance rather than read for. Attention bool @@ -127,7 +140,7 @@ func (h *Handler) uiLanes(w http.ResponseWriter, r *http.Request) { // — an empty page a few seconds after a restart must not read as a stopped one. func (h *Handler) lanesView() lanesView { if h.lanes == nil { - return lanesView{} + return lanesView{PollerOff: true} } snap := h.lanes.LaneStatus() v := lanesView{ @@ -138,15 +151,24 @@ func (h *Handler) lanesView() lanesView { now := time.Now() for _, l := range snap.Lanes { lost := l.Browser && !snap.BrowserReachable + // Series waiting and none read is the shape of a Lane that has stopped + // working, as distinct from one that is quiet for want of work. + stalled := l.Due > 0 && l.Checked == 0 + gap := "" + if l.Gap > 0 { + gap = l.Gap.Truncate(time.Second).String() + } v.Rows = append(v.Rows, laneRow{ Site: l.Site, Due: l.Due, Ran: since(now, l.LastRun), - Gap: l.Gap.Truncate(time.Second).String(), + Checked: l.Checked, + Gap: gap, Clamped: l.Clamped, Refusing: l.Refusing, BrowserLost: lost, - Attention: l.Clamped || l.Refusing || lost, + Stalled: stalled, + Attention: l.Clamped || l.Refusing || lost || stalled, }) } return v diff --git a/backend/internal/web/static/style.css b/backend/internal/web/static/style.css index 9a091b0..4a1ba46 100644 --- a/backend/internal/web/static/style.css +++ b/backend/internal/web/static/style.css @@ -87,10 +87,11 @@ --moss: #7fae86; /* finished */ --clay: #b5906f; /* set chapter */ --trash: #977671; /* remove, resting — icons need 3:1, not 4.5:1 */ - /* A Lane needing attention: the admin page's only accent, held at the same - muted weight as --brass so it never competes with ember. Neither ember - (new chapter) nor danger (destruction) may say "system unhealthy". */ - --patina: #b08a4a; + /* A Lane needing attention: the admin page's only accent. Verdigris — cool, + the far side of the wheel from ember's crimson, and clear of the archive + blue. Neither ember (new chapter) nor danger (destruction) may say + "system unhealthy". */ + --patina: #5fb3a6; /* Desktop cell borders for the two coloured action states. */ --play-hot-line: #3a1d18; @@ -150,7 +151,7 @@ --moss: #3d6c46; --clay: #7c5533; --trash: #8c6558; - --patina: #7a5a1e; + --patina: #1f6f66; --play-hot-line: #f0cfc6; --fav-line: #e3d3a4; diff --git a/backend/internal/web/templates/admin.html b/backend/internal/web/templates/admin.html index c1fdaae..f2f8e0d 100644 --- a/backend/internal/web/templates/admin.html +++ b/backend/internal/web/templates/admin.html @@ -27,7 +27,10 @@ - {{template "lanes" .Lanes}} + {{/* The live region wraps the swapped block rather than being it: the + refresh replaces the section wholesale, and a region recreated on every + update is never announced. */}} +
{{template "lanes" .Lanes}}
{{template "readers" .}} diff --git a/backend/internal/web/templates/lanes.html b/backend/internal/web/templates/lanes.html index 79a70fb..6cf61b7 100644 --- a/backend/internal/web/templates/lanes.html +++ b/backend/internal/web/templates/lanes.html @@ -7,7 +7,7 @@ a Site absent from Rows has not completed a pass since the last restart, which the empty state must say — zeroes would read as a stopped Lane. */}} {{define "lanes"}} -

Poll Lanes

{{if .Rows}} @@ -16,11 +16,13 @@ {{.Site}} {{.Due}} due + {{.Checked}} checked ran {{.Ran}} - gap {{.Gap}} + {{if .Gap}}gap {{.Gap}}{{end}} {{if .Clamped}}gap at floor{{end}} {{if .Refusing}}refusing{{end}} {{if .BrowserLost}}no browser{{end}} + {{if .Stalled}}not checking{{end}} {{end}} @@ -28,9 +30,12 @@

No data yet — no Lane has completed a pass since the backend started.

{{end}} -

Browser sidecar: +

+ {{if .PollerOff}}Polling is switched off in this deployment: no Lane runs, + and Latest Chapter comes from the userscripts alone. + {{else}}Browser sidecar: {{if not .BrowserConfigured}}not configured — comix, kagane and novelfull pages are not fetched through it{{else if .BrowserReachable}}reachable - {{else}}unreachable{{end}}.

+ {{else}}unreachable{{end}}.{{end}}

{{end}} diff --git a/backend/web_test.go b/backend/web_test.go index 4e0b9e8..445f0bb 100644 --- a/backend/web_test.go +++ b/backend/web_test.go @@ -1,6 +1,7 @@ package main import ( + "database/sql" "encoding/json" "fmt" "io" @@ -737,16 +738,19 @@ func TestAdminRoutesAreOwnerOnly(t *testing.T) { } // The Lane block reports what the poller says, and marks the Lanes that need -// attention — a clamped gap, a refusal, or a Site whose pages can only be read -// through a sidecar that is not there. +// attention — a clamped gap, a refusal, a Site whose pages can only be read +// through a sidecar that is not there, and a Lane with Series waiting that its +// last pass did not read. func TestAdminPageShowsLaneStatus(t *testing.T) { lanes := fakeLanes{latest.Status{ Lanes: []latest.LaneState{ - {Site: "asura", Due: 12, LastRun: time.Now().Add(-90 * time.Second), Gap: 40 * time.Second}, - {Site: "kagane", Due: 3, LastRun: time.Now().Add(-time.Minute), Gap: time.Minute, Browser: true}, - {Site: "demonic", Due: 400, LastRun: time.Now(), Gap: 8 * time.Second, Clamped: true}, + {Site: "asura", Due: 12, Checked: 12, LastRun: time.Now().Add(-90 * time.Second), Gap: 40 * time.Second}, + {Site: "kagane", Due: 3, Checked: 3, LastRun: time.Now().Add(-time.Minute), Gap: time.Minute, Browser: true}, + {Site: "demonic", Due: 400, Checked: 400, LastRun: time.Now(), Gap: 8 * time.Second, Clamped: true}, + {Site: "comix", Due: 7, LastRun: time.Now(), Gap: time.Minute, Browser: true}, }, BrowserConfigured: true, + BrowserReachable: true, }} router, st, _ := oauthWebTestServer(t, lanes) @@ -758,54 +762,115 @@ func TestAdminPageShowsLaneStatus(t *testing.T) { t.Fatalf("GET /ui/admin/lanes status = %d, want 200", rr.Code) } body := rr.Body.String() - for _, want := range []string{"asura", "kagane", "12 due", "gap 40s", "ran 1m30s ago", "gap at floor", "no browser", "unreachable"} { + for _, want := range []string{"asura", "kagane", "12 due", "12 checked", "gap 40s", "ran 1m30s ago", "gap at floor", "not checking", "reachable"} { if !strings.Contains(body, want) { t.Errorf("lane status lacks %q:\n%s", want, body) } } - // Two Lanes need attention: the clamped one and the one cut off from the - // sidecar. The healthy Lane must not be marked. - if n := strings.Count(body, `class="attention"`); n != 2 { - t.Errorf("attention rows = %d, want 2:\n%s", n, body) + // Nothing is refusing and the sidecar is up, so neither mark may appear: + // a mark the owner cannot act on is worse than none. + for _, unwanted := range []string{"refusing", "no browser"} { + if strings.Contains(body, unwanted) { + t.Errorf("lane status marks %q on a healthy run:\n%s", unwanted, body) + } } } -// No poller and a poller that has not finished a pass are the same to the page: -// it says so rather than drawing zeroes that read as a stopped backend. -func TestAdminPageWithoutAPollerSaysSo(t *testing.T) { - router, st, _ := oauthWebTestServer(t) - req := httptest.NewRequest(http.MethodGet, "/admin", nil) +// A Lane whose pass never reached a figure must not have that figure drawn as +// a zero: a refusing Lane still reports the due count and gap its last real +// pass saw, and a Lane that has never reached one omits it entirely. +func TestLaneStatusOmitsUnknownGap(t *testing.T) { + lanes := fakeLanes{latest.Status{ + Lanes: []latest.LaneState{{Site: "comix", LastRun: time.Now(), Refusing: true, Browser: true}}, + BrowserConfigured: true, + BrowserReachable: true, + }} + router, st, _ := oauthWebTestServer(t, lanes) + + req := httptest.NewRequest(http.MethodGet, "/ui/admin/lanes", nil) req.AddCookie(sessionCookie(t, st)) rr := httptest.NewRecorder() router.ServeHTTP(rr, req) body := rr.Body.String() - if !strings.Contains(body, "No data yet") { - t.Errorf("admin page with no poller does not say so:\n%s", body) + if strings.Contains(body, "gap 0s") { + t.Errorf("a Lane with no pace yet states a zero gap:\n%s", body) } - if !strings.Contains(body, "not configured") { - t.Errorf("admin page does not report the missing browser sidecar:\n%s", body) + if !strings.Contains(body, "refusing") { + t.Errorf("a refusing Lane is not marked as such:\n%s", body) } } -// Clearing a Reader's marks answers with the whole roster, so the page cannot -// keep showing the record that was just wiped. +// No poller and a poller that has not finished a pass both render "no data +// yet" rather than zeroes that read as a stopped backend — but they are not +// the same fact, so the page must not blame the sidecar when nothing polls. +func TestAdminPageWithoutAPollerSaysSo(t *testing.T) { + for _, tc := range []struct { + name string + lanes []web.LaneReporter + want, unwant string + }{ + {"no poller", nil, "Polling is switched off", "not configured"}, + {"poller, no pass yet", []web.LaneReporter{fakeLanes{}}, "not configured", "Polling is switched off"}, + } { + t.Run(tc.name, func(t *testing.T) { + router, st, _ := oauthWebTestServer(t, tc.lanes...) + req := httptest.NewRequest(http.MethodGet, "/admin", nil) + req.AddCookie(sessionCookie(t, st)) + rr := httptest.NewRecorder() + router.ServeHTTP(rr, req) + body := rr.Body.String() + if !strings.Contains(body, "No data yet") { + t.Errorf("admin page with no Lane data does not say so:\n%s", body) + } + if !strings.Contains(body, tc.want) { + t.Errorf("admin page lacks %q:\n%s", tc.want, body) + } + if strings.Contains(body, tc.unwant) { + t.Errorf("admin page states %q, which is not what is wrong:\n%s", tc.unwant, body) + } + }) + } +} + +// A Reader past the disagreement threshold is rendered as blocked, and +// clearing their marks both zeroes the counters and lifts the block in the +// roster the response carries back. func TestOwnerClearsReaderMarks(t *testing.T) { - router, st, _ := oauthWebTestServer(t) - theirCookie := signInCookie(t, router) - their, _, err := st.GetSession(theirCookie.Value, time.Now()) + st, dsn := newTestStoreURL(t) + router := newRouter(st, testConfig(), nil) + cookie := sessionCookie(t, st) + // The counters are filled by issue #103; until it lands the only way to + // stand a marked Reader up is to write the columns directly. + db, err := sql.Open("pgx", dsn) if err != nil { - t.Fatalf("GetSession: %v", err) + t.Fatalf("open %s: %v", dsn, err) + } + defer db.Close() + if _, err := db.Exec(`UPDATE readers SET sighting_agreements = 4, sighting_disagreements = 3 WHERE id = $1`, st.OwnerID()); err != nil { + t.Fatalf("mark reader: %v", err) } - req := httptest.NewRequest(http.MethodPost, - "/readers/"+strconv.FormatInt(their.ReaderID, 10)+"/clear-marks", nil) - req.AddCookie(sessionCookie(t, st)) + req := httptest.NewRequest(http.MethodGet, "/admin", nil) + req.AddCookie(cookie) rr := httptest.NewRecorder() router.ServeHTTP(rr, req) + body := rr.Body.String() + if !strings.Contains(body, "4 confirmed / 3 contradicted") { + t.Errorf("roster does not report the Reader's marks:\n%s", body) + } + if !strings.Contains(body, "deferral blocked") { + t.Errorf("a Reader at the threshold is not rendered as blocked:\n%s", body) + } + + req = httptest.NewRequest(http.MethodPost, + "/readers/"+strconv.FormatInt(st.OwnerID(), 10)+"/clear-marks", nil) + req.AddCookie(cookie) + rr = httptest.NewRecorder() + router.ServeHTTP(rr, req) if rr.Code != http.StatusOK { t.Fatalf("clear marks: status = %d, want 200 (body %s)", rr.Code, rr.Body.String()) } - body := rr.Body.String() + body = rr.Body.String() if !strings.Contains(body, `id="readers"`) { t.Fatalf("clear marks did not re-render the roster:\n%s", body) } diff --git a/docs/design-system.md b/docs/design-system.md index 1fca74b..e060c91 100644 --- a/docs/design-system.md +++ b/docs/design-system.md @@ -73,7 +73,7 @@ Defined once in `backend/internal/web/static/style.css` `:root`, mirrored in the | `--moss` | `#7fae86` | `#3d6c46` | finished accent | | `--clay` | `#b5906f` | `#7c5533` | set-chapter accent | | `--trash` | `#977671` | `#8c6558` | remove, at rest — icons need 3:1, not 4.5:1 | -| `--patina` | `#b08a4a` | `#7a5a1e` | admin page only — a Poll Lane needing attention, a Reader whose reports are blocked | +| `--patina` | `#5fb3a6` | `#1f6f66` | admin page only — a Poll Lane needing attention, a Reader whose reports are blocked | | `--play-hot-line` | `#3a1d18` | `#f0cfc6` | desktop cell border, play when `.is-new` | | `--fav-line` | `#332b14` | `#e3d3a4` | desktop cell border, favourite when on | | `--asura` | `#7d93a5` | `#4f6b80` | site tag | @@ -85,8 +85,9 @@ Defined once in `backend/internal/web/static/style.css` `:root`, mirrored in the `--slate`/`--moss`/`--clay`/`--brass`/`--patina` are held at the same weight deliberately: one accent per meaning, so a press says which lane it belongs to, with none of them competing with ember. `--patina` is the admin page's only -colour — system health is neither a new chapter nor destruction, so it borrows -neither `--ember` nor `--danger`. +colour — a cool verdigris, the far side of the wheel from ember's crimson and +clear of the archive blue: system health is neither a new chapter nor +destruction, so it borrows neither `--ember` nor `--danger`. Dark is the default (`color-scheme: dark light`); light is a `@media (prefers-color-scheme: light)` override of the same names. **Any new colour must be added in both branches** — light is not a filter over