From 9f39e2efd87fa6b884b01d521cb340e70288a669 Mon Sep 17 00:00:00 2001 From: John O'Keefe Date: Wed, 28 Oct 2026 00:22:00 -0400 Subject: [PATCH] =?UTF-8?q?feat(settings):=20save=20feedback=20=E2=80=94?= =?UTF-8?q?=20success/error=20toasts,=20disabled=20buttons;=20fix=20scan-f?= =?UTF-8?q?orm=20raw-JSON=20swap?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit None of the three save surfaces on the admin settings page gave usable feedback: the Base URL form re-rendered into an identical-looking form (its only indicator was the Go-source twin's banner — markup that diverged from the templ original), the scan form was WORSE — its handler responds JSON, so htmx's outerHTML swap replaced the whole scan card with literal JSON text — and the tunable rows showed only a tiny per-row label. Client side (toasts are the sanctioned client-local island; no new inline scripts, CSP unchanged): - web/src/settings.ts: one delegated htmx:afterRequest/sendError listener keyed on a data-settings-save attribute stamped on each form. Success → green toast per surface ("Settings saved" / "Scan settings saved" / "Setting saved"); failure → error toast carrying the server's JSON error field, degrading to a status line for the tunable endpoint's HTML error body (markup must never reach a toast). Network-level failures (htmx:sendError) toast instead of silently re-enabling the button. - htmx 2.x quirk handled: for FORM submissions, afterRequest's detail.elt is the event-dispatch ancestor, not the form — the requesting element lives on detail.requestConfig.elt (verified empirically; saveLabelFor reads the right one). - hx-disabled-elt="this" on all save buttons: the click is acknowledged and double-clicks cannot double-save. Server side: - Scan form: hx-swap="none" (the inputs already hold the saved values; nothing needs re-rendering) — kills the JSON-over-the-card swap. - The Base URL form's save response is a Go-source copy of the templ form (handlers cannot import templates: templates/utils.go imports handlers types). The twin now carries the functional attributes (data-settings-save, hx-disabled-elt) so feedback survives the swap, with a drift-warning comment; the double-save e2e pins this. Dev-DB safety: every success save in the spec re-submits the value already on the page (zero drift), and the spec restores system_config.updated_by after itself — the save handler stamps the requesting admin's id there, and that FK (no cascade) otherwise blocks the integration harness's transient-user sweep (it broke TestUpdateProfileAdminMode/TestDeleteUserConsolidated on this run's first pass; pollution cleaned up by hand and now self-restoring). Tests: web/src/settings.test.ts pins the form→label map and the error-extraction contract (including the no-markup-in-toast rule); e2e/settings-feedback.spec.ts drives all three surfaces through the real admin UI including the error path and the twin-drift double-save. --- e2e/settings-feedback.spec.ts | 160 ++++++++++++++++++++++++++++++ internal/handlers/sidecar.go | 14 ++- templates/admin_settings.templ | 15 +-- templates/admin_settings_templ.go | 28 +++--- web/src/main.ts | 1 + web/src/settings.test.ts | 60 +++++++++++ web/src/settings.ts | 103 +++++++++++++++++++ 7 files changed, 357 insertions(+), 24 deletions(-) create mode 100644 e2e/settings-feedback.spec.ts create mode 100644 web/src/settings.test.ts create mode 100644 web/src/settings.ts diff --git a/e2e/settings-feedback.spec.ts b/e2e/settings-feedback.spec.ts new file mode 100644 index 00000000..0a935c88 --- /dev/null +++ b/e2e/settings-feedback.spec.ts @@ -0,0 +1,160 @@ +import { test, expect, type APIRequestContext, type Page } from "@playwright/test"; +import { execSync } from "child_process"; + +const BASE = "http://localhost:8765"; +const STAMP = Date.now(); +const EMAIL = `e2e-setfb-${STAMP}@example.com`; +const USERNAME = `e2e-setfb-${STAMP}`; +const PASSWORD = "E2e-Playwright-2026!"; + +// Settings-page save feedback: every save surface must acknowledge the +// click (toast) and the scan form must NOT swap the handler's JSON body +// over its card (the pre-toast behavior — raw JSON replaced the form). +// +// Dev-state safety: every success save re-submits the value ALREADY in +// the input (read from the page first), so nothing in the shared dev DB +// changes; the error-path request is rejected server-side before any +// write. + +let token = ""; +let page = null as Page | null; +// system_config.updated_by audit restore: the config save handler stamps +// the requesting admin's id onto system_config rows. The harness cleanup +// deletes transient example.com users, and this FK (no cascade) would +// block the delete and fail unrelated integration tests. Capture before +// any save, restore after. +let e2eUserID = ""; +let origUpdatedBy = ""; + +function psql(sql: string): string { + const flat = sql.replace(/\s+/g, " "); + return execSync( + `docker exec bookhoard_db psql -U postgres -d bookhoard -tAc ${JSON.stringify(flat)}`, + ) + .toString() + .trim(); +} + +async function apiLogin(request: APIRequestContext): Promise { + const res = await request.post("/api/auth/login", { + data: { login: EMAIL, password: PASSWORD }, + }); + expect(res.status(), "login should succeed").toBe(200); + return (await res.json()).access_token as string; +} + +test.describe.configure({ mode: "serial" }); + +test.describe("Settings save feedback (toasts + disabled buttons)", () => { + test.beforeAll(async ({ browser, request }) => { + // Fresh user, promoted to admin in the dev DB (the harness cleanup + // sweeps transient example.com users afterwards). + const reg = await request.post("/api/auth/register", { + data: { email: EMAIL, username: USERNAME, password: PASSWORD }, + }); + expect([201, 200, 409]).toContain(reg.status()); + psql(`UPDATE users SET role='admin' WHERE email='${EMAIL}'`); + token = await apiLogin(request); + expect(token).toBeTruthy(); + + e2eUserID = psql(`SELECT id FROM users WHERE email='${EMAIL}'`); + origUpdatedBy = psql(`SELECT COALESCE(updated_by::text, '') FROM system_config WHERE key='base_url'`); + + page = await browser.newPage(); + await page.context().addCookies([{ name: "token", value: token, url: BASE }]); + await page.goto("/admin/settings"); + await expect(page.locator("h1")).toHaveText("System Settings"); + }); + + test.afterAll(async () => { + await page?.close(); + // Hand system_config.updated_by back to whoever held it (or NULL), + // so the transient user can be swept. + if (e2eUserID) { + const restore = origUpdatedBy + ? `'${origUpdatedBy}'::uuid` + : "NULL"; + psql(`UPDATE system_config SET updated_by = ${restore} WHERE updated_by = '${e2eUserID}'::uuid`); + } + }); + + test("Base URL save shows a success toast — twice (twin-form drift pin)", async () => { + const input = page.locator("#settings-form input[name='base_url']"); + await expect(input).toBeVisible(); + + // First save goes through the templ form… + await page.locator("#settings-form button[type='submit']").first().click(); + await expect(page.getByText("Settings saved", { exact: true })).toBeVisible({ + timeout: 10_000, + }); + + // …the second through the Go-source twin the handler swaps in after + // the first save. If the twin ever drops data-settings-save (the + // handlers→templates import cycle is why the twin exists), THIS is + // the assertion that fails. + await page.waitForTimeout(300); // let the swap settle + await expect(page.locator("#settings-form")).toBeVisible(); + await page.locator("#settings-form button[type='submit']").first().click(); + await expect(page.getByText("Settings saved", { exact: true })).toBeVisible({ + timeout: 10_000, + }); + }); + + test("Scan settings save shows a toast and never swaps raw JSON over the card", async () => { + const section = page.locator("#scan-settings-section"); + await expect(section).toBeVisible(); + + await section.locator("button[type='submit']").click(); + await expect(page.getByText("Scan settings saved", { exact: true })).toBeVisible({ + timeout: 10_000, + }); + + // The regression: the scan handler responds JSON; with the old + // hx-swap="outerHTML" the card was replaced by the literal JSON text. + await expect(section).toBeVisible(); + await expect(section.locator("form")).toBeVisible(); + const body = await page.locator("body").innerText(); + expect(body).not.toContain("scan settings updated successfully"); + }); + + test("Tunable row save shows a toast and the per-row status span", async () => { + const row = page.locator("form[data-settings-save='tunable']").first(); + await expect(row).toBeVisible(); + const key = await row.locator("input[name='key']").getAttribute("value"); + expect(key).toBeTruthy(); + + await row.locator("button[type='submit']").click(); + await expect(page.getByText("Setting saved", { exact: true })).toBeVisible({ + timeout: 10_000, + }); + // Server-driven per-row status (kept alongside the toast; it carries + // the "restart required" nuance). + await expect(page.locator(`#status-${key}`)).toContainText("Saved", { + timeout: 10_000, + }); + }); + + test("Invalid value shows an error toast and the form survives", async () => { + // Inject a genuinely invalid option and select it: setting a select + // to a value with no matching option silently serializes as NO field + // at all (no selected option → nothing submitted), which would save + // fine instead of failing. With a real option present, the server + // rejects with 400 {"error":"invalid timezone"} before any write. + await page.evaluate(() => { + const select = document.getElementById("default_timezone") as HTMLSelectElement; + const opt = document.createElement("option"); + opt.value = "Not/AZone"; + opt.textContent = "E2E invalid"; + select.appendChild(opt); + select.value = "Not/AZone"; + }); + await page.locator("#settings-form button[type='submit']").first().click(); + + await expect(page.getByText("Save failed: invalid timezone")).toBeVisible({ + timeout: 10_000, + }); + // htmx does not swap non-2xx bodies: the form is intact. + await expect(page.locator("#settings-form")).toBeVisible(); + await expect(page.locator("#settings-form input[name='base_url']")).toBeVisible(); + }); +}); diff --git a/internal/handlers/sidecar.go b/internal/handlers/sidecar.go index 449c2d08..9b3ef676 100644 --- a/internal/handlers/sidecar.go +++ b/internal/handlers/sidecar.go @@ -489,12 +489,18 @@ func (h *SidecarHandler) UpdateSystemConfiguration(c *echo.Context) error { defaultTimezone = tz } - // Render success message with updated form + // Render success message with updated form. NOTE: this Go-source + // copy is the htmx-swapped twin of the templ form in + // templates/admin_settings.templ (handlers cannot import templates: + // templates/utils.go imports handlers types — cycle). It MUST keep + // the twin's functional attributes — id, hx-*, data-settings-save, + // hx-disabled-elt — or save feedback silently dies after the first + // swap. e2e/settings-feedback.spec.ts saves twice to pin this. return c.HTML(http.StatusOK, fmt.Sprintf(`

✅ Settings saved successfully!

-
+

Base URL

@@ -513,7 +519,7 @@ func (h *SidecarHandler) UpdateSystemConfiguration(c *echo.Context) error {
-
@@ -551,7 +557,7 @@ func (h *SidecarHandler) UpdateSystemConfiguration(c *echo.Context) error {

Default timezone for users who haven't set their own.

-
diff --git a/templates/admin_settings.templ b/templates/admin_settings.templ index f5f3b06a..4f36160b 100644 --- a/templates/admin_settings.templ +++ b/templates/admin_settings.templ @@ -34,7 +34,7 @@ templ AdminSettings(user User, systemConfig map[string]string, scanSettings Scan { errorMessage } } - +
@Icon("globe", "h-5 w-5 shrink-0") @@ -53,7 +53,7 @@ templ AdminSettings(user User, systemConfig map[string]string, scanSettings Scan

The public URL of your Bookhoard instance (e.g., https://books.example.com). Used for device sync, OPDS, and API endpoints.

- @@ -128,9 +128,9 @@ templ ScanSettingsSection(scanSettings ScanSettingsData) {

Scanning

@@ -164,7 +164,7 @@ templ ScanSettingsSection(scanSettings ScanSettingsData) {

How often to poll libraries for changes (1–3600 seconds). Default: 60.

- @@ -219,6 +219,7 @@ templ TunableSettingRow(e SettingEntry) {
} - @@ -274,3 +275,5 @@ templ AboutSection() {
} + + diff --git a/templates/admin_settings_templ.go b/templates/admin_settings_templ.go index a5774bfd..8e9c42e7 100644 --- a/templates/admin_settings_templ.go +++ b/templates/admin_settings_templ.go @@ -103,7 +103,7 @@ func AdminSettings(user User, systemConfig map[string]string, scanSettings ScanS return templ_7745c5c3_Err } } - templ_7745c5c3_Err = templruntime.WriteString(templ_7745c5c3_Buffer, 9, "
") + templ_7745c5c3_Err = templruntime.WriteString(templ_7745c5c3_Buffer, 9, "
") if templ_7745c5c3_Err != nil { return templ_7745c5c3_Err } @@ -124,7 +124,7 @@ func AdminSettings(user User, systemConfig map[string]string, scanSettings ScanS if templ_7745c5c3_Err != nil { return templ_7745c5c3_Err } - templ_7745c5c3_Err = templruntime.WriteString(templ_7745c5c3_Buffer, 11, "\" placeholder=\"https://books.example.com\" class=\"input\" required>

The public URL of your Bookhoard instance (e.g., https://books.example.com). Used for device sync, OPDS, and API endpoints.

Watch libraries for file changes on startup

Watch libraries for file changes on startup

") + templ_7745c5c3_Err = templruntime.WriteString(templ_7745c5c3_Buffer, 106, "