From 8dd7a67ecf903842bb6fd35904379c6cc615dae3 Mon Sep 17 00:00:00 2001 From: guarzo Date: Mon, 10 Aug 2026 10:04:56 -0400 Subject: [PATCH 01/22] fix(payouts): make a rejected form reachable and Try again actually retry Two defects that shared a shape: a message the user could never get to. - The error boundary's "Try again" called `reset()` alone, which re-runs the segment from the client router cache -- still holding the payload that threw. A server-side failure therefore rendered the same error forever. Pairs it with `router.refresh()` so the cached payload is discarded first. - Native constraint validation runs before the `submit` event, and React's `
` runs from it, so `required` / `type="url"` / `max=` on the new-operation form made its server-side rejections unreachable. Removes the native attributes that shadowed a server message and corrects the copy: a bare hostname fails `new URL()` outright and yields `url_invalid`, not the `url_scheme` the old text described. --- e2e/error-boundary.spec.ts | 63 +++++++++++++++++ src/app/error.tsx | 81 ++++++++++++++++++---- src/app/payouts/actions.ts | 21 ++++++ src/app/payouts/errors.ts | 10 ++- src/app/payouts/new/new-operation-form.tsx | 39 ++++++++++- 5 files changed, 197 insertions(+), 17 deletions(-) diff --git a/e2e/error-boundary.spec.ts b/e2e/error-boundary.spec.ts index dc88d015..507c98a3 100644 --- a/e2e/error-boundary.spec.ts +++ b/e2e/error-boundary.spec.ts @@ -139,6 +139,69 @@ test("a retry that fails again re-announces instead of looking like a dead click }); }); +/* + * The other outcome, and the one that had no announcement at all: the retry + * works. `reset()` unmounts the boundary, so the button holding focus goes with + * it, the URL never changes, and nothing is left mounted to run a focus effect + * — focus fell to `` and a screen-reader user heard silence on the single + * press that succeeded. + * + * Note the shape of the setup: the table is restored BEFORE the click, by + * letting `breakPayoutsList` exit while the broken page is still on screen. + * That is what makes the retry deterministically succeed, and it is the only + * difference from the failing-retry test above. + * + * Writing it that way is also what exposed the larger defect this test now + * guards. With the table repaired and the press made, the boundary came back + * anyway: `reset()` alone re-runs the segment from the client router cache, + * which is still holding the payload that threw, so for a server-side failure + * the button could not recover anything at all. The fix pairs it with + * `router.refresh()` (see error.tsx). So the first assertion below — that the + * page is actually back — is the load-bearing one; the focus assertions ride + * on top of a retry that now works. + */ +test("a retry that succeeds hands focus to the recovered page", async ({ + page, + context, +}) => { + const member = await seedMember(db, { + name: "Retry Winner", + tier: "member", + status: "active", + }); + await context.addCookies([await sessionCookieFor(db, member.id)]); + + await breakPayoutsList(async () => { + await page.goto(BROKEN_ROUTE); + await expect(page.getByRole("heading", { name: "Something broke" })).toBeVisible(); + }); + + // The table is back; this press repairs the page. + const retry = page.getByRole("button", { name: /Try again|Trying/ }); + await retry.focus(); + await retry.click(); + + await expect(page.getByRole("heading", { name: "Operations" })).toBeVisible(); + await expect(page.getByRole("heading", { name: "Something broke" })).toHaveCount(0); + + // Focus is on the recovered page's own main — the skip link's target, and + // where a member skipping to content would have landed. Asserting the + // boundary marker is absent as well as the tag: both mains carry + // `id="main"`, and `data-error-boundary` is the attribute the fix uses to + // tell them apart, so a regression that focuses the wrong one still fails + // here rather than passing on the tag alone. + await expect + .poll( + async () => + page.evaluate(() => ({ + tag: document.activeElement?.tagName, + boundary: document.activeElement?.hasAttribute("data-error-boundary"), + })), + { timeout: 10_000 }, + ) + .toEqual({ tag: "MAIN", boundary: false }); +}); + test("the boundary keeps an admin inside the admin section", async ({ page, context, diff --git a/src/app/error.tsx b/src/app/error.tsx index 0c82f071..9ff88b38 100644 --- a/src/app/error.tsx +++ b/src/app/error.tsx @@ -1,7 +1,7 @@ "use client"; -import { useEffect, useState, useTransition } from "react"; -import { usePathname } from "next/navigation"; +import { useEffect, useRef, useState, useTransition } from "react"; +import { usePathname, useRouter } from "next/navigation"; import { FocusHeading } from "@/app/_components/focus-heading"; import { utcHhmm } from "@/app/_components/utc-time"; import { @@ -111,6 +111,7 @@ export default function Error({ reset: () => void; }) { const pathname = usePathname(); + const router = useRouter(); const section = sectionFor(pathname); // A client component cannot read config; the root layout's provider carries // the values down. `useBrand()` falls back to the generic defaults rather @@ -119,6 +120,61 @@ export default function Error({ const brand = useBrand(); const [retrying, startRetry] = useTransition(); + /** + * A retry that WORKS was, until now, the one outcome this page did not + * announce. `reset()` unmounts this boundary and the recovered page renders + * in its place — so the button holding focus disappears, focus falls to + * ``, the URL does not change, and no boundary is left to run an + * effect. A screen-reader user pressed a button and heard nothing, on the one + * press that actually succeeded. The failing path was already covered (the + * remount re-runs `FocusHeading`; e2e/error-boundary.spec.ts pins it), which + * is what made the gap easy to miss: the louder half was the one handled. + * + * Nothing inside this component can act after it unmounts, so the handoff is + * observed rather than scheduled — no rAF count, no timeout guess about when + * React commits. Watch for THIS main leaving the document, then focus + * whatever now answers to `#main`, which is the skip link's own target and so + * exactly where a member skipping to content would have landed. + * + * `data-error-boundary` is what keeps this off the failing path. A failed + * retry also removes this main and mounts another one carrying the same + * `id="main"`, and focusing that would race `FocusHeading` for the h1 and + * make the existing announcement nondeterministic. The attribute makes the + * two cases distinguishable at the moment of the swap: marked means the + * boundary came back and its own focus effect owns the announcement. + */ + const mainRef = useRef(null); + function retry() { + const leaving = mainRef.current; + if (leaving) { + const observer = new MutationObserver(() => { + if (leaving.isConnected) return; + observer.disconnect(); + const arrived = document.getElementById("main"); + if (arrived && !arrived.hasAttribute("data-error-boundary")) arrived.focus(); + }); + observer.observe(document.body, { childList: true, subtree: true }); + // Bounded so a retry that never resolves cannot leave an observer running + // over the rest of the session. Ten seconds is far longer than the swap + // and far shorter than a member's patience. + window.setTimeout(() => observer.disconnect(), 10_000); + } + startRetry(() => { + // `router.refresh()` BEFORE `reset()`, and the button does not work + // without it. Measured, on `/payouts` with its list query broken and then + // repaired before the press: `reset()` alone re-rendered the boundary and + // the boundary came back, every time. It re-runs the segment from the + // client router cache, and that cache is still holding the payload that + // failed — so for a server-side throw, which this boundary's own doc + // names as the case it exists for, "Try again" could never have recovered + // anything. It was reliably a no-op dressed as the page's only action. + // `refresh()` is what discards that cache entry and refetches; `reset()` + // then tears down the boundary so the refetched segment can render. + router.refresh(); + reset(); + }); + } + // Set after mount rather than during render. This boundary renders on the // server for a server-side throw, and a clock read in the render body would // be a hydration mismatch on every one of those. `null` until the effect @@ -159,7 +215,15 @@ export default function Error({ brandTagline={brand.tagline} brandMarkUrl={brand.markUrl} /> -
+
{/* Same mechanism and the same reason as the two 404 boundaries: the subtree is swapped in place with no document load, the control @@ -292,16 +356,7 @@ export default function Error({ is the thing that was missing. The visual half is the busy state in flight; it is brief against a local failure and grows with the round trip, which is the right way round. */} - {/* Stays an ``, not a ``: a full document load is the diff --git a/src/app/payouts/actions.ts b/src/app/payouts/actions.ts index 0c4f97e6..248e52a7 100644 --- a/src/app/payouts/actions.ts +++ b/src/app/payouts/actions.ts @@ -257,6 +257,27 @@ export async function createOperationAction( if (!name) return { ok: false, code: "name_required" }; const occurredAt = parseYmd(field(formData, "occurredAt")); if (occurredAt === null) return { ok: false, code: "date_invalid" }; + // `max={today}` on the form declares this rule; until now the browser was the + // only thing enforcing it, so a hand-built request could always date an + // operation into the future. That became load-bearing the moment this form + // took `noValidate` (see new-operation-form.tsx): with native validation off, + // the attribute stops being enforcement at all and this check is the rule. + // + // Compared against the same UTC-midnight boundary `parseYmd` produces, not + // against `now`: both sides are then EVE-day granular, so an operation + // recorded during today's downtime is not rejected for being "ahead" of an + // instant a few hours later in the same day. + // + // `/payouts/[id]`'s own date field is deliberately NOT changed to match. It + // still runs native validation, so its `max={today}` still holds for anyone + // using the page; it carries the same scripted-request gap this closes here, + // pre-existing and unchanged, and closing it there means a new code on + // `OPERATION_ERRORS` for a path this task did not touch. + const todayUtc = new Date(); + todayUtc.setUTCHours(0, 0, 0, 0); + if (occurredAt.getTime() > todayUtc.getTime()) { + return { ok: false, code: "date_future" }; + } // Checked before any network call, alongside name and date, so a bad scheme // never triggers an appraisal only to be thrown away. diff --git a/src/app/payouts/errors.ts b/src/app/payouts/errors.ts index c632cd9b..ec00f5a3 100644 --- a/src/app/payouts/errors.ts +++ b/src/app/payouts/errors.ts @@ -35,10 +35,18 @@ export const NEW_OPERATION_ERRORS = { name_required: "An operation needs a name. Everything else you typed is still here.", date_invalid: "Date must be a real calendar date. Everything else you typed is still here.", + date_future: + "An operation cannot be dated in the future. Everything else you typed is still here.", appraisal_failed: "Could not price that loot paste right now (triff.tools did not answer). Nothing was created — adjust the paste and try again, or leave it blank and price loot later.", + // Names the remedy, because this is the code a pasted zkillboard link + // actually lands on: `zkillboard.com/related/…` with no scheme fails + // `new URL()` outright, so it is `url_invalid` that an operator meets, not + // `url_scheme` — which only fires for something like `javascript:`, already + // a URL and merely the wrong kind. Saying "not a URL" and stopping is true + // and useless; the missing piece is always the `https://` on the front. url_invalid: - "That battle report is not a URL. Everything else you typed is still here.", + "That battle report is not a URL — it usually needs an https:// on the front. Everything else you typed is still here.", url_scheme: "Battle report links must start with http:// or https://. Everything else you typed is still here.", } as const; diff --git a/src/app/payouts/new/new-operation-form.tsx b/src/app/payouts/new/new-operation-form.tsx index 08bb00e7..729a1da1 100644 --- a/src/app/payouts/new/new-operation-form.tsx +++ b/src/app/payouts/new/new-operation-form.tsx @@ -74,8 +74,32 @@ export function NewOperationForm({ today }: { today: string }) { if (state && !state.ok) document.getElementById(ERROR_NOTICE_ID)?.focus(); }, [state]); + /* `noValidate` below, so this form's own rejection messages are the ones an + operator actually reads. Native constraint validation runs BEFORE the + `submit` event and React's `` runs FROM it, so with validation + on, `name_required`, `date_invalid`, `date_future`, `url_invalid` and + `url_scheme` were server backstops for scripted requests only — every one + of them ends "Everything else you typed is still here", a promise about a + screen nobody could reach. + + What the browser showed instead was a transient bubble ("Please enter a + URL.") that auto-dismisses, cannot be recalled, and never says the thing an + operator pasting a zkillboard link needs to hear — that the link wants an + http:// or https:// on the front. The replacement is the reserved `Notice` + slot below: persistent, re-readable, and a live region, which is what + SC 3.3.1/3.3.3 are asking for. + + The promise those messages make is already true and does not depend on this + change: every field here is controlled React state, so a rejected submit + re-renders with the pastes still in their textareas. + + The one thing native validation was really enforcing is `max={today}` — + nothing server-side rejected a future date. `createOperationAction` now + does; see the comment there. Do not remove `noValidate` without also + deciding what happens to that check, and do not remove that check while + this attribute is here. */ return ( - + {/* Mounted unconditionally, not behind `&&`: the reserved slot registers the live region before the text arrives, so AT announces a change to it rather than a region born holding its own message. */} @@ -140,10 +164,19 @@ export function NewOperationForm({ today }: { today: string }) { strict-mode violation that took out 22 payouts specs. Any wording added here has to stay clear of every other label on the form. */} Loot paste (optional: one line per item, quantity before or after) + {/* rows={6}, and the same six as Roster below. These are two sibling + optional pastes with the same job, and 10-then-8 reserved 18 rows of + empty box between the operator and "Create operation" while saying, + by the size difference alone, that loot matters more than roster. + Neither field is read at rest — both are pasted into, and a textarea + scrolls once it overflows — so the reserved height buys nothing the + scrollbar does not, and spends it on the page length of the app's + primary creation flow. Equal heights also let the two read as the + pair they are. */}