From da0b1e405dae453c6e4065133d8c8e67aaf4acf8 Mon Sep 17 00:00:00 2001 From: Misha Kaletsky <15040698+mmkal@users.noreply.github.com> Date: Thu, 13 Aug 2026 12:34:45 +0100 Subject: [PATCH 1/4] Add popup-plugins task: wrap popups with addPlugins, video-mode artifact gap Investigation findings: middleware plugins already support popups via a second addPlugins call (per-page dispatch with fall-through). videoMode needs work: a reused instance wipes the main timeline on beforeTest, and a fresh instance per popup collides on fixed artifact filenames in testInfo.outputDir. Spec for the intended behavior comes next. Co-Authored-By: Claude Fable 5 --- tasks/popup-plugins.md | 43 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) create mode 100644 tasks/popup-plugins.md diff --git a/tasks/popup-plugins.md b/tasks/popup-plugins.md new file mode 100644 index 0000000..6c0a83e --- /dev/null +++ b/tasks/popup-plugins.md @@ -0,0 +1,43 @@ +--- +status: in-progress +size: medium +branch: popup-plugins +--- + +# Popup support (auth popout windows) + +**Status summary**: investigation done. Wrapping a popup with `addPlugins` already works for middleware plugins — no code change needed there. Video mode has a real gap: two `videoMode()` instances in one test clobber each other's artifacts. Next step is a spec capturing the intended behavior; the fix (artifact namespacing) comes after. + +The pattern this should support: + +```ts +const popupPromise = page.waitForEvent("popup"); +await page.getByRole("button", { name: "Sign in" }).click(); +await using popup = await addPlugins({ + page: await popupPromise, + testInfo, + plugins: [spinnerWaiter(), videoMode()], // fresh instances for the popup +}); +``` + +## Findings: do we need a code change? + +- **Middleware plugins (spinnerWaiter, hydrationWaiter, uiErrorReporter, screenshot): no.** The `Locator.prototype` patch is global, but dispatch is per-page via `getPluginState(this.page())` (`src/plugin-system.ts`). An unwrapped popup falls through to original Playwright behavior; a second `addPlugins` call on the popup gives it its own plugin state. This is the designed seam. +- **videoMode: yes, sort of.** Two hazards, one per way you might wire it: + 1. **Reusing the same `videoMode()` instance** on page + popup is broken: the plugin holds one per-test `state` closure, and its `beforeTest` handler resets it — wrapping the popup mid-test wipes the main page's highlights/captions recorded so far. + 2. **A fresh `videoMode()` instance per popup** is the right model (Playwright screencasts each page separately; the instance's timebase is its creation time ≈ popup screencast start). But artifact filenames are fixed per `testInfo.outputDir` (`video-mode.json`, `video-raw.webm`, `video-mode-highlight-N.png`, …), so the two instances clobber each other: whichever page disposes last overwrites, and afterwards `metadata()`/`outputPaths()` on the popup instance read the *main page's* artifacts. +- A single combined video (main page + popup interleaved) would require compositing two screencasts and is out of scope. Separate videos per page is the natural model. + +## Checklist + +- [ ] spec: popup wrapped with `addPlugins` runs actions through its own plugins, timelines isolated in-memory during the test (should pass today) +- [ ] spec: after the test, each `videoMode` instance still owns its artifacts (intended-behavior spec, expected to fail today on the artifact collision) +- [ ] decide artifact namespacing for multiple `videoMode` instances per test (auto-index like `video-mode-2.json`, vs explicit `videoMode({ name: "popup" })`) +- [ ] implement the namespacing (metadata, raw/rendered video, highlight/pan images, player HTML, attachment names) +- [ ] guard against registering an already-active `videoMode` instance on a second page (clear error pointing at fresh-instance-per-page) +- [ ] ffmpeg-level spec with `video: "on"`: popup gets its own raw/rendered webm alongside the main page's +- [ ] README section on popups (fresh plugin instances per popup, `await using` so the popup finalizes) + +## Implementation log + +- 2026-08-13: investigated plugin-system + video-mode internals. Plan: `spec/popup.spec.ts` with an auth-popup demo app (`app.middlewright.test` opens `auth.middlewright.test`, Approve posts a message back to the opener). Assumption: intended behavior is *separate* artifacts per instance, not a composited single video. From db3f414bd9001999d9e799fc43f6da96968ca9ae Mon Sep 17 00:00:00 2001 From: Misha Kaletsky <15040698+mmkal@users.noreply.github.com> Date: Thu, 13 Aug 2026 12:35:50 +0100 Subject: [PATCH 2/4] Spec popup pages wrapped with addPlugins Two tests against an auth-popup demo app. The first passes today: wrapping the popup with a second addPlugins call runs its actions through its own plugins, with timelines isolated in-memory. The second is an intended-behavior spec that fails today: after both pages finalize, each videoMode instance should own its artifacts, but both resolve the same fixed filenames in testInfo.outputDir so the last finalize clobbers the first. Co-Authored-By: Claude Fable 5 --- spec/popup.spec.ts | 115 +++++++++++++++++++++++++++++++++++++++++ tasks/popup-plugins.md | 8 +-- 2 files changed, 119 insertions(+), 4 deletions(-) create mode 100644 spec/popup.spec.ts diff --git a/spec/popup.spec.ts b/spec/popup.spec.ts new file mode 100644 index 0000000..0fbb253 --- /dev/null +++ b/spec/popup.spec.ts @@ -0,0 +1,115 @@ +import { test, expect } from "@playwright/test"; +import type { BrowserContext } from "@playwright/test"; +import { addPlugins, videoMode } from "../src/index.ts"; + +test("a popup wrapped with addPlugins runs actions through its own plugins", async ({ + page: basePage, + context, +}, testInfo) => { + await routeAuthDemoApp(context); + const video = videoMode({ finalHold: 0, highlight: { mode: "outline", duration: 300 }, trimStart: "never" }); + await using page = await addPlugins({ page: basePage, testInfo, plugins: [video] }); + await page.goto("https://app.middlewright.test/"); + + const popupPromise = basePage.waitForEvent("popup"); + await page.getByRole("button", { name: "Sign in" }).click(); + const popupVideo = videoMode({ finalHold: 0, highlight: { mode: "outline", duration: 300 }, trimStart: "never" }); + await using popup = await addPlugins({ + page: await popupPromise, + testInfo, + plugins: [popupVideo], + }); + + await popup.getByRole("button", { name: "Approve" }).click(); + await page.getByText("Signed in as mmkal").waitFor(); + + // The popup's click went through the popup's own video-mode middleware... + await expect(popupVideo.metadata()).resolves.toMatchObject({ + highlights: [{ method: "click" }], + }); + // ...and the main page's timeline has only the main page's actions. + await expect(video.metadata()).resolves.toMatchObject({ + highlights: [{ method: "click" }, { method: "waitFor" }], + }); +}); + +test("each videoMode instance still owns its artifacts after the test", async ({ + page: basePage, + context, +}, testInfo) => { + await routeAuthDemoApp(context); + const video = videoMode({ finalHold: 0, highlight: { mode: "outline", duration: 300 }, trimStart: "never" }); + let popupVideo!: ReturnType; + { + await using page = await addPlugins({ page: basePage, testInfo, plugins: [video] }); + await page.goto("https://app.middlewright.test/"); + + const popupPromise = basePage.waitForEvent("popup"); + await page.getByRole("button", { name: "Sign in" }).click(); + popupVideo = videoMode({ finalHold: 0, highlight: { mode: "outline", duration: 300 }, trimStart: "never" }); + await using popup = await addPlugins({ + page: await popupPromise, + testInfo, + plugins: [popupVideo], + }); + + await popup.getByRole("button", { name: "Approve" }).click(); + await page.getByText("Signed in as mmkal").waitFor(); + } + + // Both pages have finalized. Each instance must write its own artifacts and + // read back its own timeline — not whichever page finalized last. + expect(popupVideo.outputPaths().metadata).not.toBe(video.outputPaths().metadata); + await expect(video.metadata()).resolves.toMatchObject({ + highlights: [{ method: "click" }, { method: "waitFor" }], + }); + await expect(popupVideo.metadata()).resolves.toMatchObject({ + highlights: [{ method: "click" }], + }); +}); + +/** + * app.middlewright.test shows a Sign in button that opens an auth popup on + * auth.middlewright.test; approving there posts a message back to the opener, + * which then shows who signed in. Routed on the context so the popup page is + * covered too. + */ +const routeAuthDemoApp = async (context: BrowserContext) => { + await context.route("https://app.middlewright.test/**", async (route) => { + await route.fulfill({ + body: ` +
+ + + +
+ `, + contentType: "text/html", + }); + }); + await context.route("https://auth.middlewright.test/**", async (route) => { + await route.fulfill({ + body: ` +
+

Authorize middlewright?

+ + +
+ `, + contentType: "text/html", + }); + }); +}; diff --git a/tasks/popup-plugins.md b/tasks/popup-plugins.md index 6c0a83e..81fdceb 100644 --- a/tasks/popup-plugins.md +++ b/tasks/popup-plugins.md @@ -6,7 +6,7 @@ branch: popup-plugins # Popup support (auth popout windows) -**Status summary**: investigation done. Wrapping a popup with `addPlugins` already works for middleware plugins — no code change needed there. Video mode has a real gap: two `videoMode()` instances in one test clobber each other's artifacts. Next step is a spec capturing the intended behavior; the fix (artifact namespacing) comes after. +**Status summary**: investigation done, spec written. Wrapping a popup with `addPlugins` already works for middleware plugins — no code change needed there, and a passing spec proves it. Video mode has a real gap: two `videoMode()` instances in one test clobber each other's artifacts; a failing intended-behavior spec captures it. The fix (artifact namespacing) is not implemented yet. The pattern this should support: @@ -30,8 +30,8 @@ await using popup = await addPlugins({ ## Checklist -- [ ] spec: popup wrapped with `addPlugins` runs actions through its own plugins, timelines isolated in-memory during the test (should pass today) -- [ ] spec: after the test, each `videoMode` instance still owns its artifacts (intended-behavior spec, expected to fail today on the artifact collision) +- [x] spec: popup wrapped with `addPlugins` runs actions through its own plugins, timelines isolated in-memory during the test _(`spec/popup.spec.ts`, passes — no code change needed for this half)_ +- [x] spec: after the test, each `videoMode` instance still owns its artifacts _(`spec/popup.spec.ts`, intended-behavior spec — fails today: both instances resolve `video-mode.json` in the same outputDir, and the two report attachments even share one content hash)_ - [ ] decide artifact namespacing for multiple `videoMode` instances per test (auto-index like `video-mode-2.json`, vs explicit `videoMode({ name: "popup" })`) - [ ] implement the namespacing (metadata, raw/rendered video, highlight/pan images, player HTML, attachment names) - [ ] guard against registering an already-active `videoMode` instance on a second page (clear error pointing at fresh-instance-per-page) @@ -40,4 +40,4 @@ await using popup = await addPlugins({ ## Implementation log -- 2026-08-13: investigated plugin-system + video-mode internals. Plan: `spec/popup.spec.ts` with an auth-popup demo app (`app.middlewright.test` opens `auth.middlewright.test`, Approve posts a message back to the opener). Assumption: intended behavior is *separate* artifacts per instance, not a composited single video. +- 2026-08-13: investigated plugin-system + video-mode internals. Wrote `spec/popup.spec.ts` with an auth-popup demo app (`app.middlewright.test` opens `auth.middlewright.test`, Approve posts a message back to the opener). Assumption: intended behavior is *separate* artifacts per instance, not a composited single video. Test run confirms: middleware test green, artifact test red at `outputPaths()` equality. From 94ef67c3ed22535ae6042dba9773164af7cc5b10 Mon Sep 17 00:00:00 2001 From: Misha Kaletsky <15040698+mmkal@users.noreply.github.com> Date: Thu, 13 Aug 2026 12:47:14 +0100 Subject: [PATCH 3/4] Namespace videoMode artifacts per instance to support popups Each videoMode() registered within a test now gets its own artifact namespace: the first instance keeps the legacy unsuffixed filenames, later ones get -2, -3, ... on metadata, raw/rendered video, player HTML, highlight/pan/fill images, dialog/final frames, .ass files, and attachment names. This lets an auth popup carry its own fresh videoMode instance without clobbering the main page's artifacts - Playwright screencasts each page separately, so separate videos per page is the natural model. Reusing one active instance on a second page now throws a clear error instead of silently wiping the first page's timeline in beforeTest. Specs: popup.spec.ts (artifact isolation now green, plus the reuse guard), popup-video.spec.ts (video: 'on' end to end - popup gets its own raw/rendered webm), shared demo app in auth-demo-app.ts. README gains a Popups section. Task moved to complete. Co-Authored-By: Claude Fable 5 --- README.md | 17 +++ spec/auth-demo-app.ts | 47 +++++++++ spec/popup-video.spec.ts | 48 +++++++++ spec/popup.spec.ts | 65 ++++-------- src/plugins/video-mode.ts | 117 ++++++++++++++++----- tasks/complete/2026-08-13-popup-plugins.md | 44 ++++++++ tasks/popup-plugins.md | 43 -------- 7 files changed, 264 insertions(+), 117 deletions(-) create mode 100644 spec/auth-demo-app.ts create mode 100644 spec/popup-video.spec.ts create mode 100644 tasks/complete/2026-08-13-popup-plugins.md delete mode 100644 tasks/popup-plugins.md diff --git a/README.md b/README.md index 909f4f2..fb18a14 100644 --- a/README.md +++ b/README.md @@ -351,6 +351,23 @@ export default defineConfig({ }); ``` +### Popups + +Pages you never wrap (popups, `context.newPage()`) fall through to plain Playwright. To get plugin behavior in a popup — an OAuth window, say — wrap it with a second `addPlugins` call, using **fresh plugin instances**: + +```ts +const popupPromise = page.waitForEvent("popup"); +await page.getByRole("button", { name: "Sign in" }).click(); +await using popup = await addPlugins({ + page: await popupPromise, + testInfo, + plugins: [spinnerWaiter(), videoMode()], +}); +await popup.getByRole("button", { name: "Approve" }).click(); +``` + +Playwright screencasts each page separately, so the popup's `videoMode` produces its own video; its artifacts get a `-2` suffix (`video-rendered-2.webm`, `video-mode-2.json`, …) so they sit next to the main page's in the same output dir. Reusing the main page's `videoMode` instance on the popup would wipe the main timeline, so it throws instead — one instance per page. See [spec/popup.spec.ts](spec/popup.spec.ts). + ## Writing your own plugin **Writing your own plugins is the intended way to use this package.** The bundled five exist because they were useful for one particular app; your app has its own loading conventions, error surfaces, and flake patterns. Each bundled plugin is one small self-contained file — use them as inspiration: [spinner-waiter](./src/plugins/spinner-waiter.ts) (conditional waiting + error enrichment + runtime settings via `AsyncLocalStorage`), [hydration-waiter](./src/plugins/hydration-waiter.ts) (the simplest one — start here), [ui-error-reporter](./src/plugins/ui-error-reporter.ts) (catch/enrich/rethrow), [video-mode](./src/plugins/video-mode.ts) (video annotations/artifacts + lifecycle hooks), [llm-recover](./src/plugins/llm-recover.ts) (recovery loops, artifacts, soft assertions). The source also ships inside the npm package, so it's right there in `node_modules/middlewright/src`. diff --git a/spec/auth-demo-app.ts b/spec/auth-demo-app.ts new file mode 100644 index 0000000..29d58f1 --- /dev/null +++ b/spec/auth-demo-app.ts @@ -0,0 +1,47 @@ +import type { BrowserContext } from "@playwright/test"; + +/** + * app.middlewright.test shows a Sign in button that opens an auth popup on + * auth.middlewright.test; approving there posts a message back to the opener, + * which then shows who signed in. Routed on the context so the popup page is + * covered too. + */ +export const routeAuthDemoApp = async (context: BrowserContext) => { + await context.route("https://app.middlewright.test/**", async (route) => { + await route.fulfill({ + body: ` +
+ + + +
+ `, + contentType: "text/html", + }); + }); + await context.route("https://auth.middlewright.test/**", async (route) => { + await route.fulfill({ + body: ` +
+

Authorize middlewright?

+ + +
+ `, + contentType: "text/html", + }); + }); +}; diff --git a/spec/popup-video.spec.ts b/spec/popup-video.spec.ts new file mode 100644 index 0000000..6f81c06 --- /dev/null +++ b/spec/popup-video.spec.ts @@ -0,0 +1,48 @@ +import { stat } from "node:fs/promises"; +import { test, expect } from "@playwright/test"; +import { addPlugins, videoMode } from "../src/index.ts"; +import { routeAuthDemoApp } from "./auth-demo-app.ts"; + +test.use({ video: "on" }); + +test("records separate videos for the main page and an auth popup", async ({ + page: basePage, + context, +}, testInfo) => { + await routeAuthDemoApp(context); + const video = videoMode({ finalHold: 0, highlight: { mode: "outline", duration: 300 }, trimStart: "never" }); + let popupVideo!: ReturnType; + { + await using page = await addPlugins({ page: basePage, testInfo, plugins: [video] }); + await page.goto("https://app.middlewright.test/"); + + const popupPromise = basePage.waitForEvent("popup"); + await page.getByRole("button", { name: "Sign in" }).click(); + popupVideo = videoMode({ finalHold: 0, highlight: { mode: "outline", duration: 300 }, trimStart: "never" }); + await using popup = await addPlugins({ + page: await popupPromise, + testInfo, + plugins: [popupVideo], + }); + + await popup.getByRole("button", { name: "Approve" }).click(); + await page.getByText("Signed in as mmkal").waitFor(); + } + + // Playwright screencasts each page separately, so each instance ends the + // test with its own raw recording and its own annotated render. + await expect(video.metadata()).resolves.toMatchObject({ + outputs: { raw: "video-raw.webm", rendered: "video-rendered.webm" }, + }); + await expect(popupVideo.metadata()).resolves.toMatchObject({ + outputs: { raw: "video-raw-2.webm", rendered: "video-rendered-2.webm" }, + }); + for (const path of [ + video.outputPaths().raw, + video.outputPaths().rendered, + popupVideo.outputPaths().raw, + popupVideo.outputPaths().rendered, + ]) { + expect((await stat(path)).size).toBeGreaterThan(0); + } +}); diff --git a/spec/popup.spec.ts b/spec/popup.spec.ts index 0fbb253..8f2fae3 100644 --- a/spec/popup.spec.ts +++ b/spec/popup.spec.ts @@ -1,6 +1,6 @@ import { test, expect } from "@playwright/test"; -import type { BrowserContext } from "@playwright/test"; import { addPlugins, videoMode } from "../src/index.ts"; +import { routeAuthDemoApp } from "./auth-demo-app.ts"; test("a popup wrapped with addPlugins runs actions through its own plugins", async ({ page: basePage, @@ -68,48 +68,21 @@ test("each videoMode instance still owns its artifacts after the test", async ({ }); }); -/** - * app.middlewright.test shows a Sign in button that opens an auth popup on - * auth.middlewright.test; approving there posts a message back to the opener, - * which then shows who signed in. Routed on the context so the popup page is - * covered too. - */ -const routeAuthDemoApp = async (context: BrowserContext) => { - await context.route("https://app.middlewright.test/**", async (route) => { - await route.fulfill({ - body: ` -
- - - -
- `, - contentType: "text/html", - }); - }); - await context.route("https://auth.middlewright.test/**", async (route) => { - await route.fulfill({ - body: ` -
-

Authorize middlewright?

- - -
- `, - contentType: "text/html", - }); - }); -}; +test("reusing one videoMode instance on a popup fails with a clear error", async ({ + page: basePage, + context, +}, testInfo) => { + await routeAuthDemoApp(context); + const video = videoMode({ finalHold: 0, highlight: { mode: "outline", duration: 300 }, trimStart: "never" }); + await using page = await addPlugins({ page: basePage, testInfo, plugins: [video] }); + await page.goto("https://app.middlewright.test/"); + + const popupPromise = basePage.waitForEvent("popup"); + await page.getByRole("button", { name: "Sign in" }).click(); + + // Wiring the same instance to a second page would wipe the main page's + // timeline, so it must fail loudly instead. + await expect( + addPlugins({ page: await popupPromise, testInfo, plugins: [video] }), + ).rejects.toThrow("create a fresh videoMode() instance for each page"); +}); diff --git a/src/plugins/video-mode.ts b/src/plugins/video-mode.ts index bc0e566..3f44357 100644 --- a/src/plugins/video-mode.ts +++ b/src/plugins/video-mode.ts @@ -265,6 +265,8 @@ export type VideoModeTrimStart = "auto" | "detect-blank" | "never" | ["selector" type VideoModeState = { addressBars: VideoModeAddressBar[]; + /** Distinguishes artifact filenames when a test has several instances. */ + artifactSuffix: string; captions: VideoModeCaption[]; deadAirDepth: number; deadAirSpans: VideoModeSpan[]; @@ -862,13 +864,35 @@ const recordCaption = async ( } }; -const videoModeOutputPaths = (testInfo: TestInfo): VideoModeOutputPaths => { +/** + * Insert a per-instance suffix before the extension: `video-mode.json` → + * `video-mode-2.json`. The first instance in a test keeps the unsuffixed + * names, so single-page tests are unaffected. + */ +const suffixArtifactFileName = (fileName: string, artifactSuffix: string) => { + if (!artifactSuffix) return fileName; + const extension = extname(fileName); + return `${fileName.slice(0, fileName.length - extension.length)}${artifactSuffix}${extension}`; +}; + +/** + * Registrations per testInfo.outputDir. Each videoMode instance added within + * one test (e.g. a fresh instance for a popup) gets its own artifact + * namespace so it can't clobber the main page's files. + */ +const videoModeRegistrationCounts = new Map(); + +const videoModeOutputPaths = ( + testInfo: TestInfo, + artifactSuffix: string, +): VideoModeOutputPaths => { + const name = (fileName: string) => suffixArtifactFileName(fileName, artifactSuffix); return { - metadata: join(testInfo.outputDir, VIDEO_MODE_METADATA_FILE), - player: join(testInfo.outputDir, VIDEO_MODE_PLAYER_FILE), - raw: join(testInfo.outputDir, VIDEO_MODE_RAW_FILE), - rendered: join(testInfo.outputDir, VIDEO_MODE_RENDERED_FILE), - reportPlayer: join(testInfo.outputDir, VIDEO_MODE_REPORT_PLAYER_FILE), + metadata: join(testInfo.outputDir, name(VIDEO_MODE_METADATA_FILE)), + player: join(testInfo.outputDir, name(VIDEO_MODE_PLAYER_FILE)), + raw: join(testInfo.outputDir, name(VIDEO_MODE_RAW_FILE)), + rendered: join(testInfo.outputDir, name(VIDEO_MODE_RENDERED_FILE)), + reportPlayer: join(testInfo.outputDir, name(VIDEO_MODE_REPORT_PLAYER_FILE)), }; }; @@ -1258,8 +1282,8 @@ const recordHighlight = async (options: { : undefined; const image = pan - ? `video-mode-pan-${options.state.highlightImageIndex}.png` - : `video-mode-highlight-${options.state.highlightImageIndex}.png`; + ? `video-mode-pan${options.state.artifactSuffix}-${options.state.highlightImageIndex}.png` + : `video-mode-highlight${options.state.artifactSuffix}-${options.state.highlightImageIndex}.png`; options.state.highlightImageIndex += 1; const imagePath = join(options.testInfo.outputDir, image); await mkdir(options.testInfo.outputDir, { recursive: true }); @@ -1546,7 +1570,7 @@ const recordFillReveal = async (options: { return; } - const image = `video-mode-fill-${options.state.highlightImageIndex}.png`; + const image = `video-mode-fill${options.state.artifactSuffix}-${options.state.highlightImageIndex}.png`; options.state.highlightImageIndex += 1; await mkdir(options.testInfo.outputDir, { recursive: true }); await options.locator.page().screenshot({ @@ -3580,7 +3604,7 @@ const playwrightReportAttachmentName = async (path: string) => { return `${createHash("sha1").update(data).digest("hex")}${extname(path)}`; }; -const videoModePlayerHtml = (options: { raw: string; rendered?: string }) => { +const videoModePlayerHtml = (options: { metadata: string; raw: string; rendered?: string }) => { const primary = options.rendered || options.raw; const primaryLabel = options.rendered ? "Rendered video" : "Raw video"; const primaryActiveKey = options.rendered ? "rendered" : "raw"; @@ -3724,7 +3748,7 @@ const videoModePlayerHtml = (options: { raw: string; rendered?: string }) => {
frame: 0
duration: ?s
Left/right steps one frame. Shift+left/right steps ten. Space toggles play.
-
${VIDEO_MODE_METADATA_FILE}
+
${options.metadata}