Popup follow-ups: teardown safety, entry pacing, sleep lint

.oxlintrc.json

@@ -5,5 +5,17 @@
     "middlewright/prefer-locator-waits": "error",
     "middlewright/prefer-positive-waits": "error",
     "middlewright/require-timeout-comment": "error"
-  }
+  },
+  "overrides": [
+    {
+      "files": [
+        "spec/video-mode.spec.ts",
+        "spec/video-mode-*.spec.ts",
+        "spec/scroll-pan-demo.spec.ts"
+      ],
+      "rules": {
+        "middlewright/require-timeout-comment": ["error", { "allowSleeps": true }]
+      }
+    }
+  ]
 }

spec/debug-mode.spec.ts

@@ -63,6 +63,7 @@ test("videoMode controls are inert when PWDEBUG is set", async ({ page: basePage
 
   page.videoMode.setStartTime();
   await page.videoMode.deadAir(async () => {
+    // timeout sleep gives deadAir a nonzero span to (not) record in debug mode; spinner-waiter n/a
     await page.waitForTimeout(20);
   });
   await page.getByRole("button", { name: "press" }).click();

spec/lint-plugin.spec.ts

@@ -432,6 +432,69 @@ test("reports static timeout properties without guessing dynamic shapes", async
   expect(await readFile(fixture.sourcePath, "utf8")).toBe(source);
 });
 
+test("reports bare waitForTimeout sleeps", async () => {
+  const source = `await page.waitForTimeout(500);\n`;
+  await using fixture = await lintFixture(source, requireTimeoutCommentRules);
+
+  const result = await execFileAsync("pnpm", [
+    "exec",
+    "oxlint",
+    "--config",
+    fixture.configPath,
+    fixture.sourcePath,
+  ]).catch((error: any) => error);
+
+  expect(result).toMatchObject({ code: 1 });
+  const output = `${result.stdout}\n${result.stderr}`;
+  expect(output).toContain("middlewright(require-timeout-comment)");
+  expect(output).toContain("a sleep waits whether or not the app is ready");
+  expect(await readFile(fixture.sourcePath, "utf8")).toBe(source);
+});
+
+test("allows justified waitForTimeout sleeps", async () => {
+  const source = [
+    `// timeout sleep paces footage for the render under test; spinner-waiter n/a`,
+    `await page.waitForTimeout(500);`,
+    ``,
+  ].join("\n");
+  await using fixture = await lintFixture(source, requireTimeoutCommentRules);
+
+  await execFileAsync("pnpm", [
+    "exec",
+    "oxlint",
+    "--config",
+    fixture.configPath,
+    fixture.sourcePath,
+  ]);
+
+  expect(await readFile(fixture.sourcePath, "utf8")).toBe(source);
+});
+
+test("allowSleeps exempts footage specs from the sleep check but not from timeout options", async () => {
+  const source = [
+    `await page.waitForTimeout(500);`,
+    `await page.getByText("Export").click({ timeout: 10_000 });`,
+    ``,
+  ].join("\n");
+  await using fixture = await lintFixture(source, {
+    "middlewright/require-timeout-comment": ["error", { allowSleeps: true }],
+  });
+
+  const result = await execFileAsync("pnpm", [
+    "exec",
+    "oxlint",
+    "--config",
+    fixture.configPath,
+    fixture.sourcePath,
+  ]).catch((error: any) => error);
+
+  expect(result).toMatchObject({ code: 1 });
+  expect(
+    `${result.stdout}\n${result.stderr}`.match(/middlewright\(require-timeout-comment\)/g),
+  ).toHaveLength(1);
+  expect(`${result.stdout}\n${result.stderr}`).not.toContain("a sleep waits");
+});
+
 async function lintFixture(source: string, rules: Record<string, any>) {
   const directory = await mkdtemp(join(tmpdir(), "middlewright-lint-"));
   const sourcePath = join(directory, "fixture.ts");
@@ -447,7 +510,9 @@ async function lintFixture(source: string, rules: Record<string, any>) {
   await writeFile(
     configPath,
     JSON.stringify({
-      jsPlugins: ["middlewright/lint-plugin"],
+      // A path (not the package's ./lint-plugin export) so the plugin loads
+      // from ./src via tsx, like the repo's own config — no build required.
+      jsPlugins: ["./node_modules/middlewright/lint-plugin.local.js"],
       rules,
     }),
   );

spec/popup-overlay-demo.spec.ts

@@ -17,24 +17,18 @@ test("auth popup demo", async ({ page: basePage, context }, testInfo) => {
     const popupPromise = basePage.waitForEvent("popup");
     await test.step("Open the sign-in popup", async () => {
       await page.goto("https://app.middlewright.test/");
-      // Real frames on each side of the popup span keep the composite honest
-      // (and the demo watchable) — an instant flow would land before the
-      // screencast's first frame.
-      await page.waitForTimeout(500);
       await page.getByRole("button", { name: "Sign in" }).click();
     });
 
     const popup = await popupPromise;
     await test.step("Sign in as mmkal", async () => {
-      await popup.waitForTimeout(500);
       await popup.getByLabel("Username").fill("mmkal");
       await popup.getByLabel("Password").fill("hunter2");
       await popup.getByRole("button", { name: "Sign in" }).click();
     });
 
     await test.step("Back on the app, signed in", async () => {
       await page.getByText("Signed in as mmkal").waitFor();
-      await page.waitForTimeout(500);
     });
   }
 

spec/popup-video.spec.ts

@@ -56,17 +56,14 @@ test("renders the popup as a dimmed overlay in one composed video", async ({
   {
     await using page = await addPlugins({ page: basePage, testInfo, plugins: [video] });
     await page.goto("https://app.middlewright.test/");
-    // Let the screencast capture real frames on each side of the popup span —
-    // an instant flow lands entirely before the recorder's first frame.
-    await page.waitForTimeout(500);
 
+    // No pacing waits: videoMode holds the popup's first action itself until
+    // the popup has painted and the enter animation window has real footage.
     const popupPromise = basePage.waitForEvent("popup");
     await page.getByRole("button", { name: "Sign in" }).click();
     const popup = await popupPromise;
-    await popup.waitForTimeout(500);
     await popup.getByRole("button", { name: "Approve" }).click();
     await page.getByText("Signed in as mmkal").waitFor();
-    await page.waitForTimeout(500);
   }
 
   await expect(video.metadata()).resolves.toMatchObject({

spec/popup.spec.ts

@@ -87,6 +87,41 @@ test("wrapping an already-wrapped page throws", async ({ page: basePage, context
   );
 });
 
+test("a failing popup plugin finalizer does not stop the parent finalizing", async ({
+  page: basePage,
+  context,
+}, testInfo) => {
+  await routeAuthDemoApp(context);
+  const events: string[] = [];
+  const plugin: Plugin = {
+    name: "flaky-on-popups",
+    forPopup: () => ({
+      name: "flaky-on-popups-child",
+      testLifecycle: (emitter) => {
+        emitter.on("afterTestFinalize", () => {
+          throw new Error("popup teardown exploded");
+        });
+      },
+    }),
+    testLifecycle: (emitter) => {
+      emitter.on("afterTestFinalize", () => {
+        events.push("parent finalized");
+      });
+    },
+  };
+  const page = await addPlugins({ page: basePage, testInfo, plugins: [plugin] });
+  await page.goto("https://app.middlewright.test/");
+  const popupPromise = basePage.waitForEvent("popup");
+  await page.getByRole("button", { name: "Sign in" }).click();
+  await (await popupPromise).getByRole("button", { name: "Approve" }).click();
+  await page.getByText("Signed in as mmkal").waitFor();
+
+  // The child's failure surfaces, but only after the parent finalized — a
+  // popup teardown hiccup must not drop the main page's artifacts.
+  await expect(page[Symbol.asyncDispose]()).rejects.toThrow("popup teardown exploded");
+  expect(events).toEqual(["parent finalized"]);
+});
+
 test("popups: false leaves popups unwrapped", async ({ page: basePage, context }, testInfo) => {
   await routeAuthDemoApp(context);
   const actions: string[] = [];

src/lint/plugin.ts

@@ -66,13 +66,34 @@ const preferLocatorWaits = {
   },
 };
 
+const requireTimeoutCommentSchema = [
+  {
+    type: "object",
+    properties: {
+      requiredPatterns: {
+        type: "array",
+        items: { type: "string" },
+        minItems: 1,
+      },
+      /**
+       * Allow bare waitForTimeout sleeps. For spec files whose subject IS the
+       * recorded footage (video-mode renders), sleeps are the test input —
+       * annotating every one would be noise. Everywhere else they need the
+       * same justification as explicit timeouts.
+       */
+      allowSleeps: { type: "boolean" },
+    },
+    additionalProperties: false,
+  },
+];
+
 const requireTimeoutComment = {
   meta: {
     type: "suggestion",
     docs: {
       description: "Require explicit timeout options to explain why the timeout is needed",
     },
-    schema: requiredPatternsSchema,
+    schema: requireTimeoutCommentSchema,
     messages: {
       unexplained: dedent`
         Avoid locator timeouts by using spinnerWaiter. Best ways to resolve:
@@ -83,6 +104,14 @@ const requireTimeoutComment = {
         - If it is truly impossible for there to be loading UI, add a nearby // comment matching every required pattern: {{patterns}}.
         - If you're in a block which has done \`await spinnerWaiter.settings.run({ disabled: true }, async () => ...)\`, you should probably *un-disable* for that block and apply the above suggestions to the inner code.
 
+        See https://github.com/iterate/middlewright#dont-fix-slow-tests-with-longer-timeouts for more details.
+      `,
+      sleep: dedent`
+        Avoid waitForTimeout — a sleep waits whether or not the app is ready. Best ways to resolve:
+        - Wait for positive UI instead: a locator wait covers readiness, and spinnerWaiter extends it while loading UI shows.
+        - If the sleep paces a recording, let video mode pace instead (it holds popup entry and settles the recorder itself); still-needed manual pacing is a library gap worth filing.
+        - If it is truly necessary, add a nearby // comment matching every required pattern: {{patterns}}.
+
         See https://github.com/iterate/middlewright#dont-fix-slow-tests-with-longer-timeouts for more details.
       `,
     },
@@ -97,11 +126,26 @@ const requireTimeoutComment = {
       "spinner.?waiter",
     ];
     const requiredPatterns = requiredPatternSources.map((source: string) => new RegExp(source, "i"));
+    const allowSleeps = context.options[0]?.allowSleeps === true;
 
     return {
       CallExpression(node: any) {
         if (node.callee.type !== "MemberExpression") return;
 
+        if (
+          !allowSleeps &&
+          !node.callee.computed &&
+          node.callee.property.type === "Identifier" &&
+          node.callee.property.name === "waitForTimeout" &&
+          !hasNearbyComment(node, node.callee.property, lineComments, requiredPatterns, sourceLines)
+        ) {
+          context.report({
+            node: node.callee.property,
+            messageId: "sleep",
+            data: { patterns: requiredPatternSources.join(", ") },
+          });
+        }
+
         for (const argument of node.arguments) {
           if (argument.type !== "ObjectExpression") continue;
 

src/plugin-system.ts

@@ -287,21 +287,28 @@ export const addPlugins = async <const Plugins extends readonly MaybePlugin[]>(p
       page.off("popup", onPopup);
     }
     // Children dispose first (newest first) so their plugins can finalize --
-    // and, later, feed facts to parent plugins -- before the parent's own
-    // lifecycle events run. A failed child wrap must not stop the parent
-    // finalizing; it rethrows below once cleanup is done.
+    // and feed facts to parent plugins -- before the parent's own lifecycle
+    // events run. A failed child wrap OR a throwing child dispose must not
+    // stop the parent finalizing (that would drop the main page's artifacts);
+    // failures rethrow below once the parent's own teardown has run.
+    const childFailures: unknown[] = [];
     const settledChildren = await Promise.allSettled(childWraps);
     for (const result of [...settledChildren].reverse()) {
-      if (result.status === "fulfilled") {
+      if (result.status === "rejected") {
+        childFailures.push(result.reason);
+        continue;
+      }
+      try {
         await result.value[Symbol.asyncDispose]();
+      } catch (error) {
+        childFailures.push(error);
       }
     }
     await state.lifecycleEmitter.emitSerial("afterTest", { page, testInfo });
     await state.lifecycleEmitter.emitSerial("afterTestFinalize", { page, testInfo });
     state.lifecycleCleanups.forEach((cleanup) => cleanup());
-    const failedChildWrap = settledChildren.find((result) => result.status === "rejected");
-    if (failedChildWrap) {
-      throw failedChildWrap.reason;
+    if (childFailures.length > 0) {
+      throw childFailures[0];
     }
   };