Follow the window a site opens, so a person can finish a popup sign-in

CHANGELOG.md

@@ -8,6 +8,22 @@ Newest first. `Unreleased` is what is on `main` and not yet tagged.
 
 ## Unreleased
 
+### A sign-in a site opens in a new window is shown, and can be clicked
+
+A Bot's browser was bound to the page it launched with, and to nothing the site opened afterwards.
+Anything arriving in a new window or tab was invisible on the live screen and unreachable by input,
+so the popup sign-ins that a person takes the wheel to complete were exactly the ones they could not
+complete. Worse than invisible: a click at the place the popup's button was drawn went to the page
+underneath it, so a person trying to finish a sign-in could navigate the page the Bot was working on
+without seeing either result.
+
+The browser now follows the window the site opens, and returns to the opener when it closes, which is
+what a sign-in popup does when it succeeds. A snapshot taken before the change of page is refused
+afterwards with the same "take a new snapshot" it already gives after a navigation, so a stale ref
+cannot act on the wrong document.
+
+Nothing to configure.
+
 ## 0.0.5
 
 ### One Bot can hand work to another, and reach a person when no Bot will do

agent-computer/src/index.ts

@@ -112,6 +112,8 @@ type BotSession = {
   control: Control;
   /** This Bot's snapshot generation. See the note above on staleness. */
   snapshotId: number;
+  /** The page this Bot was last handed, so a change of page can retire its refs. */
+  livePage?: Page;
   /** The one live screen viewer for this Bot, if a person is watching. */
   viewer?: {
     socket: unknown;
@@ -210,7 +212,13 @@ const DEFAULT_BOT_ID = (() => {
 })();
 
 async function currentPage(botId: string): Promise<Page> {
-  return profiles.page(botId);
+  const session = sessionFor(botId);
+  const page = await profiles.page(botId);
+  // A ref names an element on the page it was taken from, so moving to a window the site opened has
+  // to retire the outstanding ones exactly as a navigation does, or a click lands on the wrong document.
+  if (session.livePage && session.livePage !== page) session.snapshotId += 1;
+  session.livePage = page;
+  return page;
 }
 
 /**

agent-computer/src/live-page.ts

@@ -0,0 +1,16 @@
+// Its own file so a test can reach it without the Playwright import `profiles.ts` carries.
+
+/** Enough of a page for the choice. Keeps this file independent of what else is on one. */
+export type OpenPage = { isClosed(): boolean };
+
+// The newest open page wins, so a window the site opens becomes the one being watched and driven,
+// and closing it falls back to whatever is still open rather than to a page that has gone.
+export function chooseLivePage<T extends OpenPage>(
+  opened: readonly T[],
+): T | undefined {
+  for (let index = opened.length - 1; index >= 0; index -= 1) {
+    const page = opened[index];
+    if (page && !page.isClosed()) return page;
+  }
+  return undefined;
+}

agent-computer/src/profiles.ts

@@ -40,6 +40,7 @@ import { profileDirectoryFor } from "./bot-id";
 import { chooseEvictions, chooseIdle } from "./browser-eviction";
 import { egressFor, egressLabel } from "./egress";
 import { numberFromEnv } from "./env";
+import { chooseLivePage } from "./live-page";
 import { botIdsIn } from "./profile-listing";
 
 // Re-exported so callers that already import it from here do not change, while the test imports it
@@ -180,17 +181,17 @@ const IDLE_TIMEOUT_MS = numberFromEnv("COMPUTER_BROWSER_IDLE_MS", 30 * 60_000);
 const IDLE_SWEEP_MS = 60_000;
 
 export function createProfiles(root: string) {
+  type LiveBrowser = {
+    context: BrowserContext;
+    page: Page;
+    startedAt: string;
+    /** When this Bot last asked for its page. Decides what the cap and the sweep close. */
+    usedAt: number;
+    /** Point `page` at whatever is open now. Called on every page opening and closing. */
+    retarget: () => void;
+  };
   /** One running browser per Bot, up to {@link MAX_LIVE_BROWSERS}. */
-  const live = new Map<
-    string,
-    {
-      context: BrowserContext;
-      page: Page;
-      startedAt: string;
-      /** When this Bot last asked for its page. Decides what the cap and the sweep close. */
-      usedAt: number;
-    }
-  >();
+  const live = new Map<string, LiveBrowser>();
   /** Launches in flight, so a cold computer is started once however many callers ask at once. */
   const starting = new Map<string, Promise<Page>>();
 
@@ -274,6 +275,9 @@ export function createProfiles(root: string) {
       if (launching) return launching;
 
       const existing = live.get(botId);
+      // Asked before the page is judged, so a close that has not been delivered yet does not read as
+      // a browser that has gone.
+      existing?.retarget();
       if (
         existing?.context.browser()?.isConnected() &&
         !existing.page.isClosed()
@@ -309,16 +313,40 @@ export function createProfiles(root: string) {
         });
         // Persistent contexts open with a page already; reuse it rather than leaving an extra blank tab.
         const page = context.pages()[0] ?? (await context.newPage());
-        live.set(botId, {
+        const record: LiveBrowser = {
           context,
           page,
           startedAt: new Date().toISOString(),
           usedAt: Date.now(),
+          retarget: () => {},
+        };
+        record.retarget = () => {
+          const next = chooseLivePage(context.pages());
+          if (!next || next === record.page) return;
+          record.page = next;
+          console.info(
+            JSON.stringify({
+              type: "computer-page-changed",
+              botId,
+              url: next.url(),
+            }),
+          );
+        };
+        // Without this the Bot stays pinned to the page it launched with, so a sign-in the site opens
+        // in a new window is neither shown to the person taking the wheel nor reachable by input.
+        context.on("page", (opened) => {
+          record.retarget();
+          // A popup closes itself when it succeeds, and the record must move back to the opener
+          // rather than leave a closed page to be read as a dead browser.
+          opened.on("close", () => record.retarget());
         });
+        page.on("close", () => record.retarget());
+        live.set(botId, record);
         // After the new one is in the map, so the cap counts what is really running and the Bot that
         // just asked is the most recently used and therefore never the one closed.
         await enforceCap();
-        return page;
+        // Not `page`: a window opened while the browser was starting is already the live one.
+        return record.page;
       })();
 
       starting.set(botId, launch);

agent-computer/tests/follows-popup.test.ts

@@ -0,0 +1,84 @@
+import { mkdtemp, rm } from "node:fs/promises";
+import { tmpdir } from "node:os";
+import { join } from "node:path";
+import { afterAll, describe, expect, test } from "bun:test";
+
+/**
+ * Asked for by name, like the deployment journey at the repository root: it launches a real Chromium,
+ * which the machine running `bun test` is not required to have. The import is inside the test rather
+ * than at the top of the file for the same reason.
+ *
+ *   cd agent-computer && bunx playwright install chromium
+ *   OPENBOT_COMPUTER_BROWSER=1 bun test tests/follows-popup.test.ts
+ */
+const asked = process.env.OPENBOT_COMPUTER_BROWSER === "1";
+const LAUNCH_TIMEOUT_MS = 120_000;
+
+const html = (title: string, body: string) =>
+  new Response(`<!doctype html><title>${title}</title><body>${body}</body>`, {
+    headers: { "content-type": "text/html; charset=utf-8" },
+  });
+
+const site = Bun.serve({
+  port: 0,
+  fetch(request) {
+    const { pathname } = new URL(request.url);
+    if (pathname === "/popup") return html("POPUP", "<p>the popup</p>");
+    return html(
+      "OPENER",
+      `<button id="open" onclick="window.open('/popup','_blank','width=500,height=500')">open</button>`,
+    );
+  },
+});
+const origin = `http://127.0.0.1:${site.port}`;
+
+afterAll(() => {
+  site.stop(true);
+});
+
+/** Playwright delivers the opening and the closing as events, so the record moves a tick later. */
+async function settle(read: () => Promise<string>, wanted: string) {
+  for (let attempt = 0; attempt < 50; attempt += 1) {
+    if ((await read()).includes(wanted)) return;
+    await new Promise((resolve) => setTimeout(resolve, 100));
+  }
+}
+
+describe.skipIf(!asked)("the page a Bot is on", () => {
+  test(
+    "follows a window the site opens, and comes back when it closes",
+    async () => {
+      const { createProfiles } = await import("../src/profiles");
+      const root = await mkdtemp(join(tmpdir(), "openbot-profiles-"));
+      const profiles = createProfiles(root);
+      const bot = "popup-test";
+      const url = async () => (await profiles.page(bot)).url();
+
+      try {
+        const opener = await profiles.page(bot);
+        await opener.goto(`${origin}/opener`, {
+          waitUntil: "domcontentloaded",
+        });
+        expect(await url()).toContain("/opener");
+
+        await opener.click("#open");
+        await settle(url, "/popup");
+        // The defect this covers: the popup is open and rendering, and everything the Bot and the
+        // person taking the wheel can reach is still pointed at the page it launched with.
+        expect(await url()).toContain("/popup");
+
+        const popup = await profiles.page(bot);
+        await popup.close();
+        await settle(url, "/opener");
+        expect(await url()).toContain("/opener");
+        // A sign-in popup closes itself when it succeeds, and the profile that just received it must
+        // still be there.
+        expect(opener.isClosed()).toBeFalse();
+      } finally {
+        await profiles.stop(bot).catch(() => undefined);
+        await rm(root, { recursive: true, force: true }).catch(() => undefined);
+      }
+    },
+    LAUNCH_TIMEOUT_MS,
+  );
+});

agent-computer/tests/live-page.test.ts

@@ -0,0 +1,36 @@
+import { describe, expect, test } from "bun:test";
+import { chooseLivePage } from "../src/live-page";
+
+// A popup OAuth sign-in is the case this exists for: the page a Bot was pinned to at launch is not
+// the page the person taking the wheel needs to see, and the popup closes itself when it succeeds.
+const page = (closed = false) => ({ isClosed: () => closed });
+
+describe("choosing the page a Bot is on", () => {
+  test("one page is that page", () => {
+    const only = page();
+    expect(chooseLivePage([only])).toBe(only);
+  });
+
+  test("a window the site opens becomes the live one", () => {
+    const opener = page();
+    const popup = page();
+    expect(chooseLivePage([opener, popup])).toBe(popup);
+  });
+
+  test("closing that window falls back to the opener", () => {
+    const opener = page();
+    const popup = page(true);
+    expect(chooseLivePage([opener, popup])).toBe(opener);
+  });
+
+  test("a closed opener is not chosen while another page is open", () => {
+    const opener = page(true);
+    const popup = page();
+    expect(chooseLivePage([opener, popup])).toBe(popup);
+  });
+
+  test("nothing open is nothing to choose", () => {
+    expect(chooseLivePage([page(true), page(true)])).toBeUndefined();
+    expect(chooseLivePage([])).toBeUndefined();
+  });
+});