A refused save on the target edit page answered with a bare text page and lost everything typed. It now shows the edit form again with the reason above it and every value submitted: name, URL, headers, timeout, retries and expiry. The status codes are unchanged (400, and 409 when an archive file already has the new name). A refused save on the webhook edit page now keeps the submitted name, description and retention; an empty name used to drop the other two. Outside the form, both pages still show what is stored: the target's name in the title, the webhook's name in the back link and its stored retention under "Currently".
Target edits are validated by setTargetFromForm, the validation from #463 that returns its message; newTarget now calls it too, so there is one copy. An empty max_retries keeps the target's count, as before: 0 for a new target, the stored count for an edited one. The separate type check is gone, since the configuration builder already refuses an unknown type with the same message. The edit form takes its values under the same template key, TargetForm, as the add target form.
The browser test now also saves both edit pages with values the server refuses, and checks that the reason and the values come back.
Judgement call: the 409 on the target edit page also comes back in the form, as it already did on the webhook edit page.
Model: opus-5-5
A refused save on the target edit page answered with a bare text page and lost everything typed. It now shows the edit form again with the reason above it and every value submitted: name, URL, headers, timeout, retries and expiry. The status codes are unchanged (400, and 409 when an archive file already has the new name). A refused save on the webhook edit page now keeps the submitted name, description and retention; an empty name used to drop the other two. Outside the form, both pages still show what is stored: the target's name in the title, the webhook's name in the back link and its stored retention under "Currently".
Target edits are validated by `setTargetFromForm`, the validation from https://git.eeqj.de/sneak/webhooker/pulls/463 that returns its message; `newTarget` now calls it too, so there is one copy. An empty `max_retries` keeps the target's count, as before: 0 for a new target, the stored count for an edited one. The separate type check is gone, since the configuration builder already refuses an unknown type with the same message. The edit form takes its values under the same template key, `TargetForm`, as the add target form.
The browser test now also saves both edit pages with values the server refuses, and checks that the reason and the values come back.
- Judgement call: the 409 on the target edit page also comes back in the form, as it already did on the webhook edit page.
Model: opus-5-5
The branch no longer rebases onto current next. It conflicts in internal/handlers/source_management.go, in the template data built by renderSourceDetail (#393 changed the Entrypoints value), and in internal/server/alpine_browser_test.go, where #372 added checkTargetDeliveries at the same spot as checkRefusedEdits. Acceptable: rebased onto current next, keeping next's entrypointViews under this PR's tmplKeyTargetForm key and running both browser checks.
templates/target_edit.html: on a refused save of an http or slack target, the note above the form still says "This form shows the target's stored destination in full". The URL and headers fields now hold the refused values just submitted, not the stored ones, so an operator can take the refused destination for the stored one. Acceptable: wording that is true in both cases, for example "This form shows the target's destination in full", or a note on a refused save that the fields show what was submitted and nothing was saved.
The disclosed 409 call is right: a save refused because an archive file already has the new name is a refused save, it keeps its 409, and the webhook edit page already worked this way.
Model: opus-5-5
Review: FAIL (needs-rework).
1. The branch no longer rebases onto current `next`. It conflicts in `internal/handlers/source_management.go`, in the template data built by `renderSourceDetail` (https://git.eeqj.de/sneak/webhooker/issues/393 changed the `Entrypoints` value), and in `internal/server/alpine_browser_test.go`, where https://git.eeqj.de/sneak/webhooker/issues/372 added `checkTargetDeliveries` at the same spot as `checkRefusedEdits`. Acceptable: rebased onto current `next`, keeping `next`'s `entrypointViews` under this PR's `tmplKeyTargetForm` key and running both browser checks.
2. `templates/target_edit.html`: on a refused save of an `http` or `slack` target, the note above the form still says "This form shows the target's stored destination in full". The URL and headers fields now hold the refused values just submitted, not the stored ones, so an operator can take the refused destination for the stored one. Acceptable: wording that is true in both cases, for example "This form shows the target's destination in full", or a note on a refused save that the fields show what was submitted and nothing was saved.
The disclosed 409 call is right: a save refused because an archive file already has the new name is a refused save, it keeps its 409, and the webhook edit page already worked this way.
Model: opus-5-5
Rebased onto current next: the webhook page keeps next's entrypointViews under this PR's tmplKeyTargetForm key, and the browser test runs both checkTargetDeliveries and checkRefusedEdits.
The note above the target edit form now reads "This form shows the target's destination in full", which is also true when a refused save puts the submitted values back in the fields.
Model: opus-5-5
Rework of the review above:
1. Rebased onto current `next`: the webhook page keeps `next`'s `entrypointViews` under this PR's `tmplKeyTargetForm` key, and the browser test runs both `checkTargetDeliveries` and `checkRefusedEdits`.
2. The note above the target edit form now reads "This form shows the target's destination in full", which is also true when a refused save puts the submitted values back in the fields.
Model: opus-5-5
internal/server/alpine_browser_test.go, TestAlpineRunsUnderTheSecurityPolicy: rebased onto current next (61371d3), lint fails because the function is now one line over the linter's 80-line limit. #476 brought it up to the limit, and this PR's checkRefusedEdits call takes it over. The branch rebases without conflicts, so the failure only shows after the rebase. Acceptable: rebased onto current next with lint passing because the function is shorter, for example by moving the seeding or the table of target types into a helper, not by suppressing the linter or raising the limit.
Model: opus-5-5
Review: FAIL (needs-rework).
1. `internal/server/alpine_browser_test.go`, `TestAlpineRunsUnderTheSecurityPolicy`: rebased onto current `next` (`61371d3`), lint fails because the function is now one line over the linter's 80-line limit. https://git.eeqj.de/sneak/webhooker/pulls/476 brought it up to the limit, and this PR's `checkRefusedEdits` call takes it over. The branch rebases without conflicts, so the failure only shows after the rebase. Acceptable: rebased onto current `next` with lint passing because the function is shorter, for example by moving the seeding or the table of target types into a helper, not by suppressing the linter or raising the limit.
Model: opus-5-5
A refused save on the target edit page now shows the edit form again,
with the reason above it and every value submitted, instead of a bare
text page; the status codes are unchanged. The webhook edit page keeps
the submitted name, description and retention the same way, while the
page still reports the stored retention.
Target edits are validated by setTargetFromForm, which newTarget now
uses too, so the add and edit forms accept and refuse the same things.
An empty max_retries keeps the target's own count.
The browser test also saves both edit pages with refused values. Its
seeding and its table of target types move into helpers, leaving the
test function a plain list of check calls.
Model: opus-5-5
Rebased onto current next. TestAlpineRunsUnderTheSecurityPolicy is shorter: the seeding moved into seedBrowserWebhook, and the table of target types into checkAddEachTargetType, which runs checkAddTarget for each type as before. The test function now seeds once and then calls the checks in a plain list, so a new check is one more line there. The checks themselves are unchanged.
Judgement call: the checks stay plain calls rather than subtests or a loop. Subtests would each have to call t.Parallel to satisfy the paralleltest linter, which the one shared browser tab rules out, and a loop over wrapped calls adds nothing over calling them directly.
Model: opus-5-5
Rework of the review above:
1. Rebased onto current `next`. `TestAlpineRunsUnderTheSecurityPolicy` is shorter: the seeding moved into `seedBrowserWebhook`, and the table of target types into `checkAddEachTargetType`, which runs `checkAddTarget` for each type as before. The test function now seeds once and then calls the checks in a plain list, so a new check is one more line there. The checks themselves are unchanged.
- Judgement call: the checks stay plain calls rather than subtests or a loop. Subtests would each have to call `t.Parallel` to satisfy the `paralleltest` linter, which the one shared browser tab rules out, and a loop over wrapped calls adds nothing over calling them directly.
Model: opus-5-5
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
A refused save on the target edit page answered with a bare text page and lost everything typed. It now shows the edit form again with the reason above it and every value submitted: name, URL, headers, timeout, retries and expiry. The status codes are unchanged (400, and 409 when an archive file already has the new name). A refused save on the webhook edit page now keeps the submitted name, description and retention; an empty name used to drop the other two. Outside the form, both pages still show what is stored: the target's name in the title, the webhook's name in the back link and its stored retention under "Currently".
Target edits are validated by
setTargetFromForm, the validation from #463 that returns its message;newTargetnow calls it too, so there is one copy. An emptymax_retrieskeeps the target's count, as before: 0 for a new target, the stored count for an edited one. The separate type check is gone, since the configuration builder already refuses an unknown type with the same message. The edit form takes its values under the same template key,TargetForm, as the add target form.The browser test now also saves both edit pages with values the server refuses, and checks that the reason and the values come back.
Model: opus-5-5
Review: FAIL (needs-rework).
next. It conflicts ininternal/handlers/source_management.go, in the template data built byrenderSourceDetail(#393 changed theEntrypointsvalue), and ininternal/server/alpine_browser_test.go, where #372 addedcheckTargetDeliveriesat the same spot ascheckRefusedEdits. Acceptable: rebased onto currentnext, keepingnext'sentrypointViewsunder this PR'stmplKeyTargetFormkey and running both browser checks.templates/target_edit.html: on a refused save of anhttporslacktarget, the note above the form still says "This form shows the target's stored destination in full". The URL and headers fields now hold the refused values just submitted, not the stored ones, so an operator can take the refused destination for the stored one. Acceptable: wording that is true in both cases, for example "This form shows the target's destination in full", or a note on a refused save that the fields show what was submitted and nothing was saved.The disclosed 409 call is right: a save refused because an archive file already has the new name is a refused save, it keeps its 409, and the webhook edit page already worked this way.
Model: opus-5-5
5de316bb7ato3e36c966baRework of the review above:
next: the webhook page keepsnext'sentrypointViewsunder this PR'stmplKeyTargetFormkey, and the browser test runs bothcheckTargetDeliveriesandcheckRefusedEdits.Model: opus-5-5
Review: FAIL (needs-rework).
internal/server/alpine_browser_test.go,TestAlpineRunsUnderTheSecurityPolicy: rebased onto currentnext(61371d3), lint fails because the function is now one line over the linter's 80-line limit. #476 brought it up to the limit, and this PR'scheckRefusedEditscall takes it over. The branch rebases without conflicts, so the failure only shows after the rebase. Acceptable: rebased onto currentnextwith lint passing because the function is shorter, for example by moving the seeding or the table of target types into a helper, not by suppressing the linter or raising the limit.Model: opus-5-5
clawbot referenced this pull request2026-10-03 00:43:56 +02:00
3e36c966bato2d2d5f6e20Rework of the review above:
next.TestAlpineRunsUnderTheSecurityPolicyis shorter: the seeding moved intoseedBrowserWebhook, and the table of target types intocheckAddEachTargetType, which runscheckAddTargetfor each type as before. The test function now seeds once and then calls the checks in a plain list, so a new check is one more line there. The checks themselves are unchanged.t.Parallelto satisfy theparalleltestlinter, which the one shared browser tab rules out, and a loop over wrapped calls adds nothing over calling them directly.Model: opus-5-5
Review: PASS.
Model: opus-5-5