diff --git a/.claude/skills/e2e/SKILL.md b/.claude/skills/e2e/SKILL.md index 241b569c4..cb6f803b2 100644 --- a/.claude/skills/e2e/SKILL.md +++ b/.claude/skills/e2e/SKILL.md @@ -85,6 +85,13 @@ When a failure needs full DOM snapshots to understand, re-run just that test wit ### Re-render races Live events run on an in-process event bus per worker server (SSE, see `app/features/events`), so cross-worker crosstalk cannot cause flakes. Google Fonts are also blocked at the context level so font swaps never reflow the page mid-test. Re-renders from the test's own action revalidations can still swallow a React Aria press (press start registers, press end never fires — no POST); `waitForPOSTResponse` retries for this, so route flows through it rather than adding sleeps. +Other known sources, each handled once in the helpers or page objects: +- YouTube is stubbed at the context level (a fake `iframe_api`, everything else aborted): the real player loads from the internet at its own pace and, arriving mid-test, closed an open select on the VoD form +- dnd-kit stops every click in the document for 50ms after a drop; drag helpers end with `waitForDropToSettle` so the next click lands +- A tab or link clicked before hydration (after a raw `page.reload()`, say) loads the target as a new document and the click after it is lost; reload through the page object's `reload()` which waits for hydration +- Popovers close on navigation in a passive effect, so for a frame the old panel and the new page both show the same names; scope locators to `main` where a name can appear in both +- React Router drops a fetcher's redirect when a navigation (a revalidation included) started after the submission; broadcast revalidations are jittered up to 1.5s after a live event, so one could land mid-submission and the action silently did nothing. `holdRevalidationsDuring` (used by `useActionSubmit` and `SendouForm`) defers them until the submission settles + ## Test pattern reference Every test builds its own data with factories and drives the UI through page objects: @@ -112,7 +119,7 @@ Key rules: - Use `navigate()` instead of `page.goto()` — it waits for hydration (page objects' `goto()` methods wrap it) - Use `submit()` instead of clicking submit buttons directly — it waits for the POST response - Use `impersonate(page, userId?)` to authenticate. Default is admin (ADMIN_ID); prefer N-ZAP (`NZAP_TEST_ID`) when the flow doesn't need admin rights -- Avoid `page.waitForTimeout` — use assertions or `waitFor` patterns instead +- Avoid `page.waitForTimeout` — use assertions or `waitFor` patterns instead (the one exception is `waitForDropToSettle`, dnd-kit's post-drop window has nothing observable to wait on) - Import `test` from `./helpers/playwright` (not from `@playwright/test`) — it includes worker port fixtures and the database reset - Factory writes must be followed by a helper that talks to the server (`navigate`, `impersonate`, `submit`) or the test fails with "writes the server never saw" diff --git a/app/features/chat/revalidation-scope.test.ts b/app/features/chat/revalidation-scope.test.ts index d4e9c5f0e..fa2cb6aa0 100644 --- a/app/features/chat/revalidation-scope.test.ts +++ b/app/features/chat/revalidation-scope.test.ts @@ -1,6 +1,7 @@ import type { ShouldRevalidateFunctionArgs } from "react-router"; import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; import { + holdRevalidationsDuring, isMatchResultsScopedRevalidation, revalidateWithScope, scheduleBroadcastRevalidation, @@ -175,3 +176,84 @@ describe("scheduleBroadcastRevalidation", () => { expect(isMatchResultsScopedRevalidation(revalidationArgs())).toBe(false); }); }); + +describe("holdRevalidationsDuring", () => { + test("a revalidation requested during a submission runs once it has settled", async () => { + const submission = deferred(); + const revalidate = vi.fn(() => Promise.resolve()); + + const held = holdRevalidationsDuring(() => submission.promise); + revalidateWithScope(revalidate, undefined); + expect(revalidate).not.toHaveBeenCalled(); + + submission.resolve(); + await held; + expect(revalidate).toHaveBeenCalledTimes(1); + }); + + test("revalidations requested during a submission collapse into one", async () => { + const submission = deferred(); + const revalidate = vi.fn(() => Promise.resolve()); + + const held = holdRevalidationsDuring(() => submission.promise); + revalidateWithScope(revalidate, "MATCH_RESULTS"); + revalidateWithScope(revalidate, "MATCH_RESULTS"); + + submission.resolve(); + await held; + expect(revalidate).toHaveBeenCalledTimes(1); + }); + + test("a deferred revalidation of another scope widens to unscoped", async () => { + const submission = deferred(); + const revalidation = deferred(); + + const held = holdRevalidationsDuring(() => submission.promise); + revalidateWithScope(() => revalidation.promise, "MATCH_RESULTS"); + revalidateWithScope(() => revalidation.promise, undefined); + + submission.resolve(); + await held; + expect(isMatchResultsScopedRevalidation(revalidationArgs())).toBe(false); + + revalidation.resolve(); + await flushMicrotasks(); + }); + + test("the hold lasts until every overlapping submission has settled", async () => { + const first = deferred(); + const second = deferred(); + const revalidate = vi.fn(() => Promise.resolve()); + + const firstHeld = holdRevalidationsDuring(() => first.promise); + const secondHeld = holdRevalidationsDuring(() => second.promise); + revalidateWithScope(revalidate, undefined); + + first.resolve(); + await firstHeld; + expect(revalidate).not.toHaveBeenCalled(); + + second.resolve(); + await secondHeld; + expect(revalidate).toHaveBeenCalledTimes(1); + }); + + test("a failing submission releases the hold", async () => { + const revalidate = vi.fn(() => Promise.resolve()); + + const held = holdRevalidationsDuring(() => + Promise.reject(new Error("network")), + ); + revalidateWithScope(revalidate, undefined); + + await expect(held).rejects.toThrow("network"); + expect(revalidate).toHaveBeenCalledTimes(1); + }); + + test("nothing is held outside a submission", () => { + const revalidate = vi.fn(() => Promise.resolve()); + + revalidateWithScope(revalidate, undefined); + expect(revalidate).toHaveBeenCalledTimes(1); + }); +}); diff --git a/app/features/chat/revalidation-scope.ts b/app/features/chat/revalidation-scope.ts index 77a2b766a..b1cd81a13 100644 --- a/app/features/chat/revalidation-scope.ts +++ b/app/features/chat/revalidation-scope.ts @@ -11,12 +11,49 @@ let pendingRevalidations = 0; let oldestPendingStartedAt: number | null = null; let revalidationGeneration = 0; let scheduledBroadcast: { scope: RevalidateScope | null } | null = null; +let heldSubmissions = 0; +let deferredRevalidation: { + revalidate: () => Promise; + scope: RevalidateScope | null; +} | null = null; + +/** + * Runs a fetcher submission, holding broadcast revalidations back until it has settled (its + * redirect followed). React Router drops a fetcher's redirect when a navigation started after + * the submission, and a revalidation is one: a broadcast landing mid-flight would leave the + * user on the page with nothing happening. The held revalidation runs once, afterwards. + */ +export async function holdRevalidationsDuring(submission: () => Promise) { + heldSubmissions++; + try { + await submission(); + } finally { + heldSubmissions--; + if (heldSubmissions === 0 && deferredRevalidation) { + const { revalidate, scope } = deferredRevalidation; + deferredRevalidation = null; + revalidateWithScope(revalidate, scope ?? undefined); + } + } +} /** Runs a broadcast triggered revalidation, remembering its scope while in flight so `shouldRevalidate` can skip loaders the broadcast cannot have changed. */ export function revalidateWithScope( revalidate: () => Promise, scope: RevalidateScope | undefined, ) { + if (heldSubmissions > 0) { + // like an absorbed broadcast, a deferred revalidation of a different scope widens to unscoped + const widens = + deferredRevalidation !== null && + deferredRevalidation.scope !== (scope ?? null); + deferredRevalidation = { + revalidate, + scope: widens ? null : (scope ?? null), + }; + return; + } + forgetStalePendingRevalidations(); if (!scope) { diff --git a/app/form/SendouForm.tsx b/app/form/SendouForm.tsx index ee00b30d1..ec2fbc3e6 100644 --- a/app/form/SendouForm.tsx +++ b/app/form/SendouForm.tsx @@ -9,6 +9,7 @@ import * as v from "valibot"; import type { SendouButtonProps } from "~/components/elements/Button"; import { FormMessage } from "~/components/FormMessage"; import { SubmitButton } from "~/components/SubmitButton"; +import { holdRevalidationsDuring } from "~/features/chat/revalidation-scope"; import { FormField as FormFieldComponent } from "./FormField"; import { getFormFieldMetadata } from "./fields"; import styles from "./SendouForm.module.css"; @@ -505,11 +506,13 @@ function createFormActions({ const submitted = revalidateRoot ? { ...values, revalidateRoot: true } : values; - fetcher.submit(submitted as Record, { - method: "post", - action, - encType: "application/json", - }); + void holdRevalidationsDuring(() => + fetcher.submit(submitted as Record, { + method: "post", + action, + encType: "application/json", + }), + ); }; const setClientError = (name: string, error: string | undefined) => { diff --git a/app/hooks/useActionSubmit.ts b/app/hooks/useActionSubmit.ts index 01756dc00..827f41db3 100644 --- a/app/hooks/useActionSubmit.ts +++ b/app/hooks/useActionSubmit.ts @@ -1,4 +1,5 @@ import { type FetcherWithComponents, useFetcher } from "react-router"; +import { holdRevalidationsDuring } from "~/features/chat/revalidation-scope"; import { type ActionsOf, type FieldsOf, @@ -36,9 +37,13 @@ export function useActionSubmit( const fields = (rest[0] ?? {}) as Record; if (opts?.encType === "application/json") { - fetcher.submit( - { _action: action, ...fields } as Parameters[0], - { method: "post", action: opts?.action, encType: "application/json" }, + void holdRevalidationsDuring(() => + fetcher.submit( + { _action: action, ...fields } as Parameters< + typeof fetcher.submit + >[0], + { method: "post", action: opts?.action, encType: "application/json" }, + ), ); return; } @@ -48,7 +53,9 @@ export function useActionSubmit( if (value === undefined || value === null) continue; payload[name] = serializeFieldValue(value); } - fetcher.submit(payload, { method: "post", action: opts?.action }); + void holdRevalidationsDuring(() => + fetcher.submit(payload, { method: "post", action: opts?.action }), + ); }; return { submit, fetcher, state: fetcher.state }; diff --git a/e2e/helpers/playwright.ts b/e2e/helpers/playwright.ts index bfb8628b9..eae26ea53 100644 --- a/e2e/helpers/playwright.ts +++ b/e2e/helpers/playwright.ts @@ -32,6 +32,17 @@ declare global { } } +/** `YT.Player` that never readies, so a VoD form behaves as it does before the real one loads. */ +const YOUTUBE_IFRAME_API_STUB = ` +window.YT = { + Player: class { + getCurrentTime() { return 0; } + destroy() {} + }, +}; +window.onYouTubeIframeAPIReady?.(); +`; + export const MOBILE_VIEWPORT = { width: 375, height: 667 }; export const TABLET_VIEWPORT = { width: 768, height: 1024 }; @@ -79,6 +90,18 @@ export const test = base.extend({ /^https:\/\/fonts\.(googleapis|gstatic)\.com\//, (route) => route.abort(), ); + // The VoD pages embed a YouTube player, which loads from the internet + // (player, ads, telemetry) at a pace of its own. Under load it landed + // mid-test, and the frame arriving closed the select being filled in. + // A stub player API keeps the pages off the network. + await context.route(/^https:\/\/www\.youtube\.com\//, (route) => + new URL(route.request().url()).pathname === "/iframe_api" + ? route.fulfill({ + contentType: "text/javascript", + body: YOUTUBE_IFRAME_API_STUB, + }) + : route.abort(), + ); await use(context); }, workerPort: [ @@ -470,6 +493,14 @@ async function expectRouterIdle(page: Page) { } } +/** dnd-kit stops every click in the document for this long after a drop (`PointerSensor.detach`). */ +const DND_KIT_CLICK_SUPPRESSION_MS = 50; + +/** Waits out dnd-kit's post-drop click suppression, which nothing observable marks the end of. Call after the `mouse.up()` of a drag. */ +export async function waitForDropToSettle(page: Page) { + await page.waitForTimeout(2 * DND_KIT_CLICK_SUPPRESSION_MS); +} + /** Asserts the page rendered rather than the error boundary catching something. */ export async function expectNoErrorPage(page: Page) { await expect(page.getByTestId("error-page")).toHaveCount(0); diff --git a/e2e/pages/friends/friends-page.ts b/e2e/pages/friends/friends-page.ts index 101729eda..66b8fa1f1 100644 --- a/e2e/pages/friends/friends-page.ts +++ b/e2e/pages/friends/friends-page.ts @@ -52,8 +52,9 @@ export class FriendsPage { ); } + /** Scoped to the page content: the mobile friends panel still shows the same name for a frame after navigating here from it. */ friendButton(name: string) { - return this.page.getByRole("button", { name }); + return this.page.getByRole("main").getByRole("button", { name }); } friend(name: string) { diff --git a/e2e/pages/layout/mobile-nav.ts b/e2e/pages/layout/mobile-nav.ts index 0c22233ea..bb1c1b160 100644 --- a/e2e/pages/layout/mobile-nav.ts +++ b/e2e/pages/layout/mobile-nav.ts @@ -72,7 +72,10 @@ export class MobileNav { private async settleAnimations() { await this.page.evaluate(() => Promise.all( - document.getAnimations().map((animation) => animation.finished), + document.getAnimations().map((animation) => + // a cancelled animation (its element gone, or another taking its place) is as settled as a finished one + animation.finished.catch(() => {}), + ), ), ); } diff --git a/e2e/pages/tier-list-maker/tier-list-maker-page.ts b/e2e/pages/tier-list-maker/tier-list-maker-page.ts index 2ba5387cb..b7491630f 100644 --- a/e2e/pages/tier-list-maker/tier-list-maker-page.ts +++ b/e2e/pages/tier-list-maker/tier-list-maker-page.ts @@ -1,7 +1,12 @@ import type { Page } from "@playwright/test"; import invariant from "~/utils/invariant"; import { TIER_LIST_MAKER_URL } from "~/utils/urls"; -import { expect, expectIsHydrated, navigate } from "../../helpers/playwright"; +import { + expect, + expectIsHydrated, + navigate, + waitForDropToSettle, +} from "../../helpers/playwright"; type ItemType = | "main-weapon" @@ -64,7 +69,9 @@ export class TierListMakerPage { } async openTab(type: ItemType) { - await this.page.getByRole("tab", { name: TAB_NAMES[type] }).click(); + const tab = this.page.getByRole("tab", { name: TAB_NAMES[type] }); + await tab.click(); + await expect(tab).toHaveAttribute("aria-selected", "true"); } async setPlacementMode(mode: "drag" | "click") { @@ -94,6 +101,7 @@ export class TierListMakerPage { await this.page.mouse.up(); await expect(emptyTiers).toHaveCount(emptyCountBefore - 1); + await waitForDropToSettle(this.page); } async clickFirstItem(type: ItemType) { diff --git a/e2e/pages/tournament/tournament-brackets-page.ts b/e2e/pages/tournament/tournament-brackets-page.ts index 904eb566b..53d877e89 100644 --- a/e2e/pages/tournament/tournament-brackets-page.ts +++ b/e2e/pages/tournament/tournament-brackets-page.ts @@ -2,6 +2,7 @@ import type { Page } from "@playwright/test"; import { tournamentBracketsPage } from "~/features/tournament-bracket/tournament-bracket-urls"; import { expect, + expectIsHydrated, modalClickConfirmButton, navigate, submit, @@ -49,6 +50,11 @@ export class TournamentBracketsPage { }); } + async reload() { + await this.page.reload(); + await expectIsHydrated(this.page); + } + teamName(name: string) { return this.page.getByText(name); } @@ -152,6 +158,8 @@ export class TournamentBracketsPage { this.page.getByTestId("back-to-bracket-button"), ).toBeVisible(); }).toPass(); + // a click that beat hydration loads the match page as a new document + await expectIsHydrated(this.page); return new TournamentMatchPage(this.page); } diff --git a/e2e/pages/tournament/tournament-seeds-page.ts b/e2e/pages/tournament/tournament-seeds-page.ts index d9649a584..35ee8a0c8 100644 --- a/e2e/pages/tournament/tournament-seeds-page.ts +++ b/e2e/pages/tournament/tournament-seeds-page.ts @@ -1,6 +1,10 @@ import type { Page } from "@playwright/test"; import { tournamentAdminPage } from "~/utils/urls"; -import { navigate, submit } from "../../helpers/playwright"; +import { + navigate, + submit, + waitForDropToSettle, +} from "../../helpers/playwright"; const DRAG_TARGET_Y = 500; @@ -34,6 +38,7 @@ export class TournamentSeedsPage { // the drag & drop library only registers the drop when moved in steps await this.page.mouse.move(0, DRAG_TARGET_Y, { steps: 10 }); await this.page.mouse.up(); + await waitForDropToSettle(this.page); } save() { diff --git a/e2e/tournament-bracket.spec.ts b/e2e/tournament-bracket.spec.ts index adb1d75f9..04c563082 100644 --- a/e2e/tournament-bracket.spec.ts +++ b/e2e/tournament-bracket.spec.ts @@ -182,7 +182,7 @@ test.describe("Tournament bracket", () => { // past the 26min limit of a Bo3 await page.clock.fastForward("30:00"); - await page.reload(); + await brackets.reload(); match = await brackets.openMatch(matchId);