Render admin page errors in the normal layout (closes #382) #408

Merged
clawbot merged 1 commits from issue-382-error-pages into next 2026-10-02 08:15:13 +02:00
Collaborator

Every 400, 403, 404 and 500 on an admin page now answers with an error page in the normal layout: the status, one fixed line explaining it, and a link back to the webhook list, or to sign-in when nobody is signed in. Unknown paths reach it through the router's not-found handler, a refused form token through the CSRF middleware, and a panic through a recoverer each admin page route group now installs first, with error reporting inside it. Status codes are unchanged; the page always sends Cache-Control: no-store. The receiver, the healthcheck and /metrics keep their plain answers, after a panic too.

If the error page fails to render, the answer is the same status in plain text, with no loop; if it panics while rendering, the answer is a 500. A normal page that fails to render now gets the 500 page.

  • Judgement call: an error page served before a form token exists (unknown path outside every route group, or a panic) leaves Logout out of the navbar.
  • Judgement call: the line is fixed per status, so a refused form token and another user's profile share the 403 line; the profile page's busy answer (503) uses the page too.
  • Judgement call: with error reporting on, an admin page request passes through it twice, globally and in its group; only the group's sees a panic, so it is reported once.
  • Not changed: form validation messages (#381, #370) and the middleware's 413 and 429 answers.

Model: opus-5-5

Every 400, 403, 404 and 500 on an admin page now answers with an error page in the normal layout: the status, one fixed line explaining it, and a link back to the webhook list, or to sign-in when nobody is signed in. Unknown paths reach it through the router's not-found handler, a refused form token through the CSRF middleware, and a panic through a recoverer each admin page route group now installs first, with error reporting inside it. Status codes are unchanged; the page always sends `Cache-Control: no-store`. The receiver, the healthcheck and `/metrics` keep their plain answers, after a panic too. If the error page fails to render, the answer is the same status in plain text, with no loop; if it panics while rendering, the answer is a 500. A normal page that fails to render now gets the 500 page. - Judgement call: an error page served before a form token exists (unknown path outside every route group, or a panic) leaves Logout out of the navbar. - Judgement call: the line is fixed per status, so a refused form token and another user's profile share the 403 line; the profile page's busy answer (503) uses the page too. - Judgement call: with error reporting on, an admin page request passes through it twice, globally and in its group; only the group's sees a panic, so it is reported once. - Not changed: form validation messages (https://git.eeqj.de/sneak/webhooker/issues/381, https://git.eeqj.de/sneak/webhooker/issues/370) and the middleware's 413 and 429 answers. Model: opus-5-5
clawbot added the needs-review label 2026-10-01 23:07:13 +02:00
clawbot self-assigned this 2026-10-01 23:07:13 +02:00
Author
Collaborator

Review: FAIL (needs-rework)

  1. Does not build on current next. The branch rebases without conflicts, but HandleSourceDetail in internal/handlers/source_management.go now has two h.serverError(w, msg, err) calls from the recent-events change (#347) that use the old signature, so the handlers package no longer compiles. Acceptable: rebase onto current next and pass the request in both calls, so those two new 500s on the webhook page also render the error page.

  2. A 500 after a panic on an admin page is still plain text. The definition of done says every admin page route answers 500 with a rendered page, and the PR lists this case as not changed. It can be rendered safely: the recoverer already knows whether anything was written, and the page renders into a buffer with a plain-text fallback. Acceptable: on admin page routes, a panic caught before anything was written answers with the 500 error page; if that page fails or panics while rendering, the answer is the plain 500 as today; the receiver, the healthcheck and /metrics keep the plain 500; the panic is still logged and still reported to error tracking; a test sends a panicking admin page handler through the real router.

  3. Some error pages show the signed-in user but can be cached. The page for an unknown path outside the route groups, and the page for a refused form token (sent before NoCache runs), show the user's name and profile link without Cache-Control: no-store. Every other page that shows them sends that header. Acceptable: the error page always sends Cache-Control: no-store, for example set in renderError.

  • Judgement call: leaving the 413 and 429 answers plain meets the definition of done, which names only 400, 403, 404 and 500.
  • Judgement call: with metrics turned off, /metrics is an unknown path and now gets the HTML 404 page. The same goes for receiver-like paths that are not an entrypoint URL, such as /webhook/a/b. I read that as the rule for unknown paths, not a change to /metrics or the receiver.

Model: opus-5-5

Review: FAIL (needs-rework) 1. **Does not build on current `next`.** The branch rebases without conflicts, but `HandleSourceDetail` in `internal/handlers/source_management.go` now has two `h.serverError(w, msg, err)` calls from the recent-events change (https://git.eeqj.de/sneak/webhooker/issues/347) that use the old signature, so the handlers package no longer compiles. Acceptable: rebase onto current `next` and pass the request in both calls, so those two new 500s on the webhook page also render the error page. 2. **A 500 after a panic on an admin page is still plain text.** The definition of done says every admin page route answers 500 with a rendered page, and the PR lists this case as not changed. It can be rendered safely: the recoverer already knows whether anything was written, and the page renders into a buffer with a plain-text fallback. Acceptable: on admin page routes, a panic caught before anything was written answers with the 500 error page; if that page fails or panics while rendering, the answer is the plain 500 as today; the receiver, the healthcheck and `/metrics` keep the plain 500; the panic is still logged and still reported to error tracking; a test sends a panicking admin page handler through the real router. 3. **Some error pages show the signed-in user but can be cached.** The page for an unknown path outside the route groups, and the page for a refused form token (sent before `NoCache` runs), show the user's name and profile link without `Cache-Control: no-store`. Every other page that shows them sends that header. Acceptable: the error page always sends `Cache-Control: no-store`, for example set in `renderError`. - Judgement call: leaving the 413 and 429 answers plain meets the definition of done, which names only 400, 403, 404 and 500. - Judgement call: with metrics turned off, `/metrics` is an unknown path and now gets the HTML 404 page. The same goes for receiver-like paths that are not an entrypoint URL, such as `/webhook/a/b`. I read that as the rule for unknown paths, not a change to `/metrics` or the receiver. Model: opus-5-5
clawbot added needs-rework and removed needs-review labels 2026-10-02 00:14:56 +02:00
clawbot force-pushed issue-382-error-pages from 718865e075 to 2b303fe569 2026-10-02 01:16:49 +02:00 Compare
clawbot added needs-review and removed needs-rework labels 2026-10-02 01:16:59 +02:00
Author
Collaborator

Rework:

  1. Rebased onto current next; both h.serverError calls in HandleSourceDetail now pass the request, so those two 500s render the error page.
  2. Each admin page route group now installs its own recoverer, answering with the 500 error page, with the error reporting inside it, as its first middleware. A panic there is logged and reported once, inside the group, and answered with the page, or with the plain 500 if the page fails or panics. The global recoverer is unchanged and keeps the plain 500 for the receiver, the healthcheck and /metrics. A new test sends a panicking handler in the /pages route group through the real router with error reporting on; two more cover the page and its fallback in the recoverer itself.
  3. renderError now sets Cache-Control: no-store on every error page, its plain-text fallback included.
  • Judgement call: the error page after a panic is served before a form token exists, so its navbar leaves out Logout, as on the page for an unknown path outside every route group.
  • Judgement call: with error reporting on, an admin page request passes through it twice, globally and in its group; only the group's sees a panic.
  • The PR body is updated to match.

Model: opus-5-5

Rework: 1. Rebased onto current `next`; both `h.serverError` calls in `HandleSourceDetail` now pass the request, so those two 500s render the error page. 2. Each admin page route group now installs its own recoverer, answering with the 500 error page, with the error reporting inside it, as its first middleware. A panic there is logged and reported once, inside the group, and answered with the page, or with the plain 500 if the page fails or panics. The global recoverer is unchanged and keeps the plain 500 for the receiver, the healthcheck and `/metrics`. A new test sends a panicking handler in the `/pages` route group through the real router with error reporting on; two more cover the page and its fallback in the recoverer itself. 3. `renderError` now sets `Cache-Control: no-store` on every error page, its plain-text fallback included. - Judgement call: the error page after a panic is served before a form token exists, so its navbar leaves out Logout, as on the page for an unknown path outside every route group. - Judgement call: with error reporting on, an admin page request passes through it twice, globally and in its group; only the group's sees a panic. - The PR body is updated to match. Model: opus-5-5
Author
Collaborator

Review: FAIL (needs-rework)

  1. Two comments the PR edits are not true. In internal/server/routes.go, the maxFormBodySize comment now says every route group below installs MaxBodySize as its first middleware after the recoverer. With error reporting turned on, the group's error reporting sits between the two, and the /s, /api/v1 and /metrics groups install no MaxBodySize at all. In internal/middleware/recoverer.go, the Recoverer comment still ends "every other header is left to http.Error", but when a page is given, http.Error runs only if the page fails. Acceptable: the first comment names the four admin page route groups and says MaxBodySize comes right after their recoverer and error reporting; the second says what happens to the other headers when the page answers.

  2. The README's middleware section no longer describes the admin page route groups. Under Middleware Stack in README.md, the description of what the /pages, /user/*, /sources and /source/* groups apply, and the paragraph on where the Recoverer sits, do not mention that each of those groups now starts with its own recoverer and error reporting. They also don't say that this recoverer answers a panic with the error page, while the global one keeps the plain 500 for every other route. Acceptable: a short addition there saying so.

  3. The PR body is over the length limit (about 275 words, against about 250). Acceptable: 250 words or fewer, for example by dropping the last bullet, which the comment in SetupRoutes already states.

  • Judgement call: / only redirects, so I did not count it as an admin page route; a panic there still gets the plain 500.
  • Judgement call: the double pass through error reporting is acceptable. The group's pass reuses what the global pass started for the request, and only the group's pass reports a panic.
  • Deviation: the page after a panic was checked in the browser against a local build with a planted panic route, because the image has no route that can panic.

Model: opus-5-5

Review: FAIL (needs-rework) 1. **Two comments the PR edits are not true.** In `internal/server/routes.go`, the `maxFormBodySize` comment now says every route group below installs `MaxBodySize` as its first middleware after the recoverer. With error reporting turned on, the group's error reporting sits between the two, and the `/s`, `/api/v1` and `/metrics` groups install no `MaxBodySize` at all. In `internal/middleware/recoverer.go`, the `Recoverer` comment still ends "every other header is left to http.Error", but when a page is given, `http.Error` runs only if the page fails. Acceptable: the first comment names the four admin page route groups and says `MaxBodySize` comes right after their recoverer and error reporting; the second says what happens to the other headers when the page answers. 2. **The README's middleware section no longer describes the admin page route groups.** Under Middleware Stack in `README.md`, the description of what the `/pages`, `/user/*`, `/sources` and `/source/*` groups apply, and the paragraph on where the Recoverer sits, do not mention that each of those groups now starts with its own recoverer and error reporting. They also don't say that this recoverer answers a panic with the error page, while the global one keeps the plain 500 for every other route. Acceptable: a short addition there saying so. 3. **The PR body is over the length limit** (about 275 words, against about 250). Acceptable: 250 words or fewer, for example by dropping the last bullet, which the comment in `SetupRoutes` already states. - Judgement call: `/` only redirects, so I did not count it as an admin page route; a panic there still gets the plain 500. - Judgement call: the double pass through error reporting is acceptable. The group's pass reuses what the global pass started for the request, and only the group's pass reports a panic. - Deviation: the page after a panic was checked in the browser against a local build with a planted panic route, because the image has no route that can panic. Model: opus-5-5
clawbot added needs-rework and removed needs-review labels 2026-10-02 01:57:17 +02:00
clawbot force-pushed issue-382-error-pages from 2b303fe569 to 2de98cedbe 2026-10-02 02:32:38 +02:00 Compare
clawbot added needs-review and removed needs-rework labels 2026-10-02 02:32:44 +02:00
Author
Collaborator

Rework, rebased onto current next:

  1. The maxFormBodySize comment names the four admin page route groups and says MaxBodySize comes right after their recoverer and error reporting; the Recoverer comment says that when the page answers, every other header the handler set goes out with it, apart from those the page sets itself.
  2. The README's Middleware Stack section has a short paragraph saying each admin page route group starts with its own recoverer and error reporting, which answers a panic with the error page, while the global one keeps the plain 500 for every other route.
  3. The PR body is under 250 words; the bullet on a route group's middleware running twice is dropped, as the comment in SetupRoutes already says it.
  • Also fixed: the HandleErrorPage comment listed only the router and the CSRF middleware as its users; it now names the admin page route groups' recoverers too.

Model: opus-5-5

Rework, rebased onto current `next`: 1. The `maxFormBodySize` comment names the four admin page route groups and says `MaxBodySize` comes right after their recoverer and error reporting; the `Recoverer` comment says that when the page answers, every other header the handler set goes out with it, apart from those the page sets itself. 2. The README's Middleware Stack section has a short paragraph saying each admin page route group starts with its own recoverer and error reporting, which answers a panic with the error page, while the global one keeps the plain 500 for every other route. 3. The PR body is under 250 words; the bullet on a route group's middleware running twice is dropped, as the comment in `SetupRoutes` already says it. - Also fixed: the `HandleErrorPage` comment listed only the router and the CSRF middleware as its users; it now names the admin page route groups' recoverers too. Model: opus-5-5
Author
Collaborator

Review: FAIL (needs-rework)

  1. The PR body and the commit message overstate what happens when the error page panics. Both say that if the error page fails to render or panics, the answer is the same status in plain text, with no status change. That holds for a render failure, and for the 500 page panicking inside a route group's recoverer. It does not hold for the other pages: a panic while rendering the 404, 403, 400 or 503 page is caught by a recoverer and answered with a 500. For an unknown path outside every route group that 500 is plain text; inside a group it is the 500 page, or plain text if that page panics too. Acceptable: in both the PR body and the commit message, say that a render failure keeps the status in plain text and a panic in the error page is answered with a 500.
  • Judgement call: an unknown path inside an admin page route group runs that group's middleware twice, so a first visit gets two form-token cookies. The browser keeps the second, which matches the page, so I accepted the comment in SetupRoutes that calls this harmless.

Model: opus-5-5

Review: FAIL (needs-rework) 1. **The PR body and the commit message overstate what happens when the error page panics.** Both say that if the error page fails to render or panics, the answer is the same status in plain text, with no status change. That holds for a render failure, and for the 500 page panicking inside a route group's recoverer. It does not hold for the other pages: a panic while rendering the 404, 403, 400 or 503 page is caught by a recoverer and answered with a 500. For an unknown path outside every route group that 500 is plain text; inside a group it is the 500 page, or plain text if that page panics too. Acceptable: in both the PR body and the commit message, say that a render failure keeps the status in plain text and a panic in the error page is answered with a 500. - Judgement call: an unknown path inside an admin page route group runs that group's middleware twice, so a first visit gets two form-token cookies. The browser keeps the second, which matches the page, so I accepted the comment in `SetupRoutes` that calls this harmless. Model: opus-5-5
Author
Collaborator

Re-gate after #378: FAIL (needs-rebase, needs-rework)

  1. Does not rebase onto current next. It conflicts in README.md (the Middleware Stack paragraph on the admin page route groups) and in internal/server/routes.go (setupSourceRoutes, both route groups). Acceptable: rebase so the groups are /hooks and /hook/{sourceID}, each still starting with its own recoverer.

  2. The error page links back to a path that no longer exists. templates/error.html sends "Back to webhooks" to /sources, which is now an unknown path, so a signed-in user following it lands on the 404 page again. Acceptable: the link goes to /hooks.

  3. The new tests use the old paths. In internal/server/error_page_test.go, backToWebhooks expects /sources; the deleted webhook, deleted target, unknown path inside a route group and refused form token tests request /source/ID...; the receiver test posts to /webhook/no-such-entrypoint. On current next these requests never reach a route group or the receiver: the unknown path, form token and receiver tests fail, and the deleted webhook and deleted target tests pass without reaching the webhook or target pages. Acceptable: /hooks, /hook/ID, /hook/ID/edit, /hook/ID/targets/ID/edit, and /h/ followed by an unknown UUID for the receiver.

  4. Docs and a comment the PR adds name the old paths. The new README.md paragraph lists /sources and /source/*; the maxFormBodySize comment in internal/server/routes.go lists /sources and /source/{sourceID}. Acceptable: /hooks and /hook/* in the README, /hooks and /hook/{sourceID} in the comment.

Model: opus-5-5

Re-gate after https://git.eeqj.de/sneak/webhooker/pulls/378: FAIL (needs-rebase, needs-rework) 1. **Does not rebase onto current `next`.** It conflicts in `README.md` (the Middleware Stack paragraph on the admin page route groups) and in `internal/server/routes.go` (`setupSourceRoutes`, both route groups). Acceptable: rebase so the groups are `/hooks` and `/hook/{sourceID}`, each still starting with its own recoverer. 2. **The error page links back to a path that no longer exists.** `templates/error.html` sends "Back to webhooks" to `/sources`, which is now an unknown path, so a signed-in user following it lands on the 404 page again. Acceptable: the link goes to `/hooks`. 3. **The new tests use the old paths.** In `internal/server/error_page_test.go`, `backToWebhooks` expects `/sources`; the deleted webhook, deleted target, unknown path inside a route group and refused form token tests request `/source/ID...`; the receiver test posts to `/webhook/no-such-entrypoint`. On current `next` these requests never reach a route group or the receiver: the unknown path, form token and receiver tests fail, and the deleted webhook and deleted target tests pass without reaching the webhook or target pages. Acceptable: `/hooks`, `/hook/ID`, `/hook/ID/edit`, `/hook/ID/targets/ID/edit`, and `/h/` followed by an unknown UUID for the receiver. 4. **Docs and a comment the PR adds name the old paths.** The new `README.md` paragraph lists `/sources` and `/source/*`; the `maxFormBodySize` comment in `internal/server/routes.go` lists `/sources` and `/source/{sourceID}`. Acceptable: `/hooks` and `/hook/*` in the README, `/hooks` and `/hook/{sourceID}` in the comment. Model: opus-5-5
clawbot added needs-rebase and removed needs-review labels 2026-10-02 06:42:05 +02:00
clawbot force-pushed issue-382-error-pages from 2de98cedbe to ef59bf7554 2026-10-02 06:49:30 +02:00 Compare
clawbot added needs-review and removed needs-rebase labels 2026-10-02 06:49:43 +02:00
Author
Collaborator

Rework, rebased onto current next:

  1. Rebased onto #378 and #403; the /hooks and /hook/{sourceID} route groups each start with their own recoverer.
  2. The error page's "Back to webhooks" link goes to /hooks.
  3. The tests request /hooks, /hook/ID, /hook/ID/edit and /hook/ID/targets/ID/edit; the receiver test posts to /h/ followed by an unknown UUID.
  4. The README paragraph names /hooks and /hook/*; the maxFormBodySize comment names /hooks and /hook/{sourceID}.
  • Also fixed: the commit message now says a panic in the error page is answered with a 500, as the PR body already did (finding 1 of the 04:43 review).

Model: opus-5-5

Rework, rebased onto current `next`: 1. Rebased onto https://git.eeqj.de/sneak/webhooker/pulls/378 and https://git.eeqj.de/sneak/webhooker/pulls/403; the `/hooks` and `/hook/{sourceID}` route groups each start with their own recoverer. 2. The error page's "Back to webhooks" link goes to `/hooks`. 3. The tests request `/hooks`, `/hook/ID`, `/hook/ID/edit` and `/hook/ID/targets/ID/edit`; the receiver test posts to `/h/` followed by an unknown UUID. 4. The README paragraph names `/hooks` and `/hook/*`; the `maxFormBodySize` comment names `/hooks` and `/hook/{sourceID}`. - Also fixed: the commit message now says a panic in the error page is answered with a 500, as the PR body already did (finding 1 of the 04:43 review). Model: opus-5-5
Author
Collaborator

Review: FAIL (needs-rebase)

  1. Does not rebase onto current next. Since #406 landed, templates/navbar.html conflicts in both the desktop and the mobile menu: that change removed the Login link shown to a signed-out visitor, right where this PR wraps the Logout form in {{if .CSRFToken}}. Acceptable: rebase onto current next, keep the removal of the Login link from #406, and wrap only the Logout form in {{if .CSRFToken}} in both menus.
  • Deviation: the rest of the change was reviewed on a local resolution of that conflict made as described above; it raised no other finding.

Model: opus-5-5

Review: FAIL (needs-rebase) 1. **Does not rebase onto current `next`.** Since https://git.eeqj.de/sneak/webhooker/pulls/406 landed, `templates/navbar.html` conflicts in both the desktop and the mobile menu: that change removed the Login link shown to a signed-out visitor, right where this PR wraps the Logout form in `{{if .CSRFToken}}`. Acceptable: rebase onto current `next`, keep the removal of the Login link from https://git.eeqj.de/sneak/webhooker/pulls/406, and wrap only the Logout form in `{{if .CSRFToken}}` in both menus. - Deviation: the rest of the change was reviewed on a local resolution of that conflict made as described above; it raised no other finding. Model: opus-5-5
clawbot added needs-rebase and removed needs-review labels 2026-10-02 07:26:45 +02:00
clawbot added 1 commit 2026-10-02 07:38:39 +02:00
Every 400, 403, 404 and 500 on an admin page now answers with an
error page in the normal layout: one fixed line for the status and a
link back to the webhook list, or to sign-in when nobody is signed
in. The router's handler for unknown paths, the CSRF middleware's
refusal and a panic in an admin page route group use the same page;
each such group has its own recoverer and error reporting for that.
The page always sends Cache-Control: no-store. Status codes are
unchanged. The receiver, the healthcheck and /metrics keep their
plain answers. If the error page fails to render, the answer is the
same status in plain text; if it panics, the answer is a 500.

Model: opus-5-5
clawbot force-pushed issue-382-error-pages from ef59bf7554 to afe6e60b9e 2026-10-02 07:38:39 +02:00 Compare
clawbot added needs-review and removed needs-rebase labels 2026-10-02 07:38:44 +02:00
Author
Collaborator

Rebased onto current next. The only conflict was templates/navbar.html: in both the desktop and the mobile menu, the Login link removed by #406 stays removed, and only the Logout form is wrapped in {{if .CSRFToken}}. Nothing else changed.

Model: opus-5-5

Rebased onto current `next`. The only conflict was `templates/navbar.html`: in both the desktop and the mobile menu, the Login link removed by https://git.eeqj.de/sneak/webhooker/pulls/406 stays removed, and only the Logout form is wrapped in `{{if .CSRFToken}}`. Nothing else changed. Model: opus-5-5
Author
Collaborator

Re-gate passed on current next.

Model: opus-5-5

Re-gate passed on current next. Model: opus-5-5
Author
Collaborator

Re-gate passed on current next.

Model: opus-5-5

Re-gate passed on current next. Model: opus-5-5
clawbot merged commit 2416528b77 into next 2026-10-02 08:15:13 +02:00
clawbot deleted branch issue-382-error-pages 2026-10-02 08:15:13 +02:00
Sign in to join this conversation.
No Reviewers
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/webhooker#408