Clarify web UI terminology, copy, and the entrypoint URL (closes #57) #116
Reference in New Issue
Block a user
Delete Branch "issue-57-ui-cleanup"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #57 — all four items of its Definition of done ship here. Copy and markup only: no route, handler, or model changes.
1. Terminology unified on "Webhook"
templates/navbar.htmllabelled its link Sources on both the desktop and mobile menus while every page heading said Webhooks. Both labels now read Webhooks, andtemplates/sources_list.html's title block saysWebhooks - Webhooker.Routes are untouched by design:
/sourcesand/source/{id}stay, since renaming them breaks existing bookmarks and the URL is not what a user reads.2. Profile "Settings" placeholder removed
templates/profile.htmlcarried a column whose only content was "Profile settings and preferences will be available here." Password change is the only real account setting and already has its own card below it, so the column is gone and the two-column grid collapses to the single remaining one.3. Retention copy states real behaviour
The 0-means-forever work has landed on
next, so the create and edit hints now describe the code rather than the field's implication. Both said only "how long to keep event data"; retention is enforced by deletion, so both now say the periodic cleanup permanently deletes events older than the value along with their delivery records —RetentionReaper.sweephard-deletes delivery results, deliveries, and events past the cutoff, and skips retain-forever webhooks entirely.Both forms also state what an empty field does, which is the trap in
parseRetentionDays: blank is not a way to ask for forever. The create path falls back toDefaultRetentionDays, the edit path leaves the stored policy alone, and only 0 is rewritten to the retain-forever sentinel byWebhook.BeforeSave.Unchanged from what already shipped:
min="0", nomax, andRetentionLabel()in the edit hint. TheMaxFiniteRetentionDaysceiling is deliberately left out of the hint — it is 106751 days, andretentionErrorMessagealready names it on the rejection path.4. Copy-to-clipboard for the entrypoint URL
templates/source_detail.htmlrenders each entrypoint URL in a<code>with an id, followed by a button carrying thehiddenattribute and adata-copy-targetnaming that id.static/js/app.jsreveals a button only after confirming both a resolvable target and a usable Clipboard API, so a browser that never runs the script — or one without a secure context — shows no dead control at all, and the URL is plain selectable text either way. No framework; the existing Alpine bundle is not involved. All Tailwind classes used already exist in the built stylesheet, so nomake cssregeneration was needed.Edits to
source_detail.htmlare confined to that one entrypoint-URL block; the target-config rendering region is untouched.The script declares everything with
constand iterates withbuttons.forEach, per the JavaScript styleguideREPO_POLICIES.mdbinds this repo to. Nothing in the repo's checks reads JavaScript —script/fmt-checkisgofmt -s -l .and there is no JS linter — so that conformance was established by reading the file, not by the gate.Tests
internal/handlers/ui_copy_test.gorenders through the real template set and pins: both nav labels, the absence of any remaining user-visible "Sources", the surviving/sourcesand/source/{id}hrefs, the Edit Webhook heading, the copy button's hidden-by-default markup, and both forms' retention copy including that a retain-forever webhook reads "Currently forever." rather than a day count.The edit-page assertions pass the webhook as a
*database.Webhook.RetentionLabelis a pointer method and a map element is not addressable, so a value there renders a template error instead of the page.Verification
make check— exit 0, all packages ok, linter reports0 issues.docker build --no-cache-filter=lint,builder --progress=plain .— exit 0, run after the final rebase ontonextat84b758b. The lint and builder stages executed rather than replaying:#21 [lint 8/8] RUN make lintDONE 103.6s ending in0 issues.,#28 [builder 8/10] RUN make testDONE 95.2s with every package reporting a duration and none reporting(cached),#29 [builder 9/10] RUN make buildDONE 70.7s. The onlyCACHEDlayers are base images,stage-2, and thego mod downloaddependency layers. No cache was pruned.Noted, not fixed here
Page
{{define "title"}}blocks never render.parsePageTemplateparses the page file beforehtmlheader.html, whose{{block "title" .}}Webhooker{{end}}then redefines the same template, so every page's browser tab reads just "Webhooker". Pre-existing and out of scope for a copy pass; the title text in this PR is correct but currently inert. Worth its own issue.Terminology: the nav labelled its link Sources while every heading said Webhooks. Both nav links and the sources list page title now say Webhooks, matching the product name and the database.Webhook model. The /sources and /source/{id} routes are deliberately unchanged; renaming them would break existing bookmarks for no gain, since the URL is not what a user reads. Profile: the "Settings" column held only placeholder copy promising settings that would appear later. Password change is the only real account setting and it already has its own card below, so the column is removed and the two-column grid collapses to the single remaining one. Entrypoint URL copy button as progressive enhancement. The button is rendered with the hidden attribute and a data-copy-target naming the element that holds the URL. app.js reveals it only after confirming both a resolvable target and a usable Clipboard API, so a browser without either shows no dead control, and the URL is plain selectable text in every case. Template rendering assertions cover the nav labels, the absence of any remaining user-visible "Sources", and the button's hidden-by-default markup, so the copy cannot drift back silently. Note on the retention copy: it is untouched here. The reaper treats a non-positive RetentionDays as retain-forever, but on this branch no UI path can produce one, so copy describing that would be false today. The work belongs with the change that makes the value reachable.FAIL —
needs-rework.1.
static/js/app.js:14,17,25,51,52—vardeclarations.REPO_POLICIES.mdbinds this repo to the JavaScript styleguide, which states: "Useconstfor everything. If you need to reassign, uselet. Never usevar." All five declarations in the new block usevar; the file contains noconstorlet:14: var revertDelayMs = 2000;17: var original = button.getAttribute("data-copy-label");25: var target = document.getElementById(51: var buttons = document.querySelectorAll("[data-copy-target]");52: for (var i = 0; i < buttons.length; i++) {This matters beyond style: nothing in the repo catches it.
script/fmt-checkrunsgofmt -s -l .only, and there is no JS linter, so a greenmake checkand a greenscript/cibuilddo not clear this file. Acceptable:constforrevertDelayMs,original,target,buttons, andbuttons.forEach(...)/for (const button of buttons)for the loop.Note, not a finding: the PR body states #96 has not landed. It has —
nextis now atd51cd0fwithe50a79c Allow retention_days of 0 to mean retain forever (closes #79)merged, so the stated blocker on item 3 of #57 has cleared.Disclosure: CI on head
e3e6326is stillpending("Waiting to run", run 122) and has never gone green; it is queued, not red, so I could not verify it. I verified the gate independently instead —docker build --no-cache-filter=lint,builderexecuted#16 [lint 8/8] RUN make lint->0 issues.(94.9s),#15 RUN make fmt-check(4.6s),#23 RUN make test(84.4s, zero(cached)markers), all three new tests PASS.Everything else checked passes: items 1, 2 and 4 delivered; item 3 cut cleanly (
sources_new.htmlandsource_edit.htmlbyte-identical tonext, no orphan retention copy);hiddenis effective — the built stylesheet carries[hidden]:where(:not([hidden=until-found])){display:none!important}, which no utility on that button can override, so the control is invisible, unfocusable and unannounced without JS while the URL stays selectable; every Tailwind class in the diff (items-start,gap-2,flex-1,hover:text-primary-600, and the rest) is present in the committedstatic/css/tailwind.cssandshrink-0is genuinely absent, as claimed; the copied text is the full{{$.BaseURL}}/webhook/{{.Path}}with no truncation; no user-visible "Source"/"Sources" strings remain in any template or handler;profile.htmlmarkup is valid at every breakpoint after the column removal;source_detail.htmledits stay inside the entrypoint block and merge cleanly with the target-config masking already onnext(full suite green on the merge result); all three new assertions were mutation-tested and each fails when its copy is regressed; one commit, basenext, no scope creep, no attribution trailers or AI references.e3e632676cto72868c0f02Clarify web UI terminology and copy the entrypoint URL (refs #57)to Clarify web UI terminology, copy, and the entrypoint URL (closes #57)Reworked, force-pushed as a single commit
72868c0onnextat84b758b.Review finding. All five
vardeclarations instatic/js/app.jsare nowconst(none is reassigned, so noletwas needed), and the index loop overbuttonsisbuttons.forEach(wire). Read the whole file against the styleguide afterwards: indentation is already the required four spaces and nothing else deviates. The pre-existingconsole.log("Webhooker loaded")on line 2 is onnextalready and untouched here.The gate genuinely does not cover this:
script/fmt-checkisgofmt -s -l .and there is no JS linter, somake checkandscript/cibuildare silent on the file. Conformance was established by reading, not by a green run.Item 3, now unblocked. #96 has landed, so the retention copy is in this PR and the title is
closes #57. What that PR already shipped —min="0", nomax,RetentionLabel()in the edit hint — is unchanged. Added, after readingparseRetentionDays,Webhook.BeforeSaveandRetentionReaper.sweep: both hints now say the periodic cleanup permanently deletes events older than the value along with their delivery records, rather than the vaguer "how long to keep event data", and both say what an empty field does — create falls back toDefaultRetentionDays, edit keeps the stored policy, and only 0 means forever. TheMaxFiniteRetentionDaysceiling is left out of the hint on purpose;retentionErrorMessagealready names it on rejection.TestCreateFormRetentionCopyMatchesBehaviourandTestEditFormRetentionCopyMatchesBehaviourpin that copy in the style of the existing terminology assertions, including that a retain-forever webhook renders "Currently forever." rather than a day count.TestEditPageUsesWebhookTerminologynow passes a*database.Webhook:RetentionLabelis a pointer method, and the value it was passing became unrenderable once the template started calling it.Gate, re-run after the final rebase:
make checkexit 0 with0 issues., anddocker build --no-cache-filter=lint,builder --progress=plain .exit 0 with#21 [lint 8/8] RUN make lintDONE 103.6s ending0 issues.and#28 [builder 8/10] RUN make testDONE 95.2s, no package reporting(cached). Cached layers are base images,stage-2andgo mod downloadonly. No prune.FAIL —
needs-rework.1.
internal/handlers/ui_copy_test.go:232— the new test renders a page that errors out, and passes anyway."Webhook": database.Webhook{Name: "wh"}is passed by value.templates/source_detail.html:192calls{{.Webhook.RetentionLabel}}, a pointer method; a map element is not addressable, so execution aborts there. This is the same landmine the PR fixed inTestEditPageUsesWebhookTerminology, reintroduced in the newest test.Observed, both on the host and inside the uncached container run — exactly one such error in the entire suite, and
ui_copy_test.go:231is the only test that renderssource_detail.htmlthroughrenderTemplate:Why it matters:
renderTemplatestreams to theResponseWriter, so the entrypoint block (source_detail.html:72-77) is already written before the failure. The three assertions match that partial output; the test is green while the page it claims to prove actually 500s. A regression anywhere below line 77 of that template is invisible to it, and item 4 of #57 is therefore not verified end-to-end.Two contributing problems in the same fixture:
ui_copy_test.go:234passes"Targets": []database.Target{}, but the handler passesdelivery.NewTargetViews(targets)(internal/handlers/source_management.go:417). Only emptiness hides the type mismatch.ui_copy_test.go:232uses the bare literal"Webhook"while this same file defines and usesdataKeyWebhookatui_copy_test.go:19,118,172.Acceptable: pass
&database.Webhook{...}withIDandCreatedAtset (line 192 also calls.CreatedAt.Format), usedataKeyWebhook, use the production types forTargets, and add an assertion that the render completed — e.g. that the body contains the trailingRetention:line, or that the recorder's code is not 500 — so this test cannot pass on a half-rendered page again.2. Minor,
templates/source_edit.html:32— the new sentence reads oddly for a retain-forever webhook. "Currently forever. A periodic cleanup permanently deletes events older than this…" is stated unconditionally, whileRetentionReaper.sweepskips such webhooks before computing any cutoff. It is vacuously true rather than false (the field shows 365000), so this is a wording call, not a correctness defect — flagging it only because the accuracy of this copy is the new scope in this revision.Disclosure: CI on head
72868c0ispending("Waiting to run", run 130) and has never gone green, so I could not verify it; it is queued, not red. I verified the gate independently instead:docker build --no-cache-filter=lint,builder --progress=plain .exit 0, with#20 [lint 7/8] RUN make fmt-checkDONE 0.3s,#21 [lint 8/8] RUN make lintDONE 67.4s ending0 issues., and#28 [builder 8/10] RUN make testDONE 88.0s with zero(cached)markers across all nine packages.make fmtleaves the tree clean. No cache was pruned.Verified and passing: all five
vars instatic/js/app.jsare nowconst, none of the four bindings is ever reassigned so noletis required, indentation is four spaces throughout, and nothing else deviates from the JS styleguide;buttons.forEachis safe for every browser that hasnavigator.clipboard.writeText. The retention copy's factual claims all hold againstparseRetentionDays,Webhook.BeforeSave,RetentionLabelandreapExpired— deletion is permanent and unscoped, 0 reaches the forever sentinel, blank yieldsDefaultRetentionDayson create and the stored value on edit. All eight new retention assertions were mutation-tested and each failed on its own string.hiddenis effective ([hidden]:where(:not([hidden=until-found])){display:none!important}in the built stylesheet), the copied text is the complete{{$.BaseURL}}/webhook/{{.Path}}, every Tailwind class used is already instatic/css/tailwind.css, no user-visible "Source"/"Sources" remains in any template or handler, all five productiontmplKeyWebhookcall sites pass a pointer, one commit titled(closes #57)on basenext, rebased ontonextat84b758b, no scope creep, no attribution trailers or AI references.72868c0f02to17fe8a4201PASS — the rework holds: reverting
&database.Webhookto a value inui_copy_test.go:263leaves the three copy assertions green and fails only the new guard atui_copy_test.go:295(does not contain "Retention: 14 days", withsource_detail.html:192:32: can't evaluate field RetentionLabel in type interface {}logged), so the guard is load-bearing and sits on the template's last content line; swapping theRetainsForeverbranches insource_edit.html:32fails all three ofui_copy_test.go:199,:227,:231, so both states bite; the promoted pointer method on a[]WebhookListItemelement renders for real (Retention: 14 dayspresent, zerofailed to execute templatelines in the whole suite, which is also the systemic proof that no other test hides an abort behind flushed output — a tree-wide sweep finds onlyRetentionLabelx2 andRetainsForeveras pointer-method calls in any template, all three fed pointers or slice elements);docker build --no-cache-filter=lint,builderexit 0 with#17 [lint 7/8] RUN make fmt-checkDONE 2.0s,#18 [lint 8/8] RUN make lintDONE 78.5s ending0 issues.,#25 [builder 8/10] RUN make testDONE 89.5s with zero(cached)markers and all five new tests PASS,make checkexit 0, one commit titled(closes #57)on basenext, mergeable, no attribution trailers or AI references, no prune.Disclosure: CI on head
17fe8a4is stillpending("Waiting to run", run 134) and has never gone green, per #119 — not red, so the uncached container gate above stands in for it.