fix: coalesce approval page lookups (#178217)

Coalesce repeated approval-page lookups while preserving Retry feedback and retiring stale responses after scope or decision changes.

Fixes #127579.
Reviewed-by: @shakkernerd
Co-authored-by: Shakker <165377636+shakkernerd@users.noreply.github.com>

test/vitest-ui-e2e-config.test.ts

@@ -32,7 +32,7 @@ const qaLabFiles = [
   "extensions/qa-lab/src/session-host-command-state.real-gateway.e2e.test.ts",
   "extensions/qa-lab/src/control-ui-openclaw-delegation.real-gateway.e2e.test.ts",
 ] as const;
-const serialGatewayFiles = `chat-managed-media-order
+const serialGatewayFiles = `approval-lookup chat-managed-media-order
   chat-session-diff-folding chat-steer-deferred-custody chat-tts-supplement
   command-palette-search deepseek-search desktop-resize device-platform-family
   peer-followup-restart session-pr-reader-lifetime

test/vitest/vitest.ui-e2e.config.ts

@@ -29,6 +29,7 @@ export const uiE2ePrivateServerTestFiles = [
   "ui/src/e2e/agent-file-lifecycle.real-gateway.e2e.test.ts",
   "ui/src/e2e/agent-switch-roster.e2e.test.ts",
   "ui/src/e2e/approval-bootstrap.e2e.test.ts",
+  "ui/src/e2e/approval-lookup.real-gateway.e2e.test.ts",
   "ui/src/e2e/background-work.real-gateway.e2e.test.ts",
   "ui/src/e2e/boot-module-boundaries.e2e.test.ts",
   "ui/src/e2e/browser-auth-recovery.e2e.test.ts",

test/vitest/vitest.ui-paths.mjs

@@ -138,6 +138,7 @@ export function isUiTestTarget(relative) {
 }
 
 export const uiE2eRealGatewayTestFiles = [
+  "ui/src/e2e/approval-lookup.real-gateway.e2e.test.ts",
   "ui/src/e2e/deepseek-search.real-gateway.e2e.test.ts",
   "ui/src/e2e/peer-followup-restart.real-gateway.e2e.test.ts",
   "ui/src/e2e/chat-session-diff-folding.real-gateway.e2e.test.ts",

ui/src/e2e/approval-lookup.real-gateway.e2e.test.ts

@@ -0,0 +1,185 @@
+import { createHash } from "node:crypto";
+import { readFile, writeFile } from "node:fs/promises";
+import path from "node:path";
+import { isRecord } from "@openclaw/normalization-core/record-coerce";
+import { expect, it } from "vitest";
+import {
+  createOpenClawTestInstance,
+  type OpenClawTestInstance,
+} from "../../../test/helpers/openclaw-test-instance.ts";
+import { runQaGatewayFixture } from "../../../test/helpers/qa-gateway-cleanup.ts";
+import { pairControlUiPage } from "../test-helpers/control-ui-browser-pairing.ts";
+import { createControlUiE2eSuite } from "./control-ui-e2e-suite.test-support.ts";
+
+let instance: OpenClawTestInstance;
+const suite = createControlUiE2eSuite({
+  name: "Approval lookup admission through a real Gateway",
+  startServerBeforeBrowser: true,
+  async startServer() {
+    instance = await createOpenClawTestInstance({ name: "approval-lookup" });
+    try {
+      await instance.state.writeConfig({
+        gateway: {
+          port: instance.port,
+          auth: { mode: "token", token: instance.gatewayToken },
+          controlUi: { enabled: true },
+        },
+      });
+      await instance.startGateway();
+      return { baseUrl: `http://127.0.0.1:${instance.port}/`, close: () => instance.cleanup() };
+    } catch (error) {
+      return runQaGatewayFixture(
+        async () => {
+          throw error;
+        },
+        () => instance.cleanup(),
+      );
+    }
+  },
+});
+
+async function runCli(args: string[]) {
+  const result = await instance.cli(args);
+  expect(result.code).toBe(0);
+  return result.stdout;
+}
+
+suite.define(() => {
+  it("coalesces tab refreshes while a real approval lookup reply is held", async (test) => {
+    await suite.runScenario(test, {
+      retainedState: () => instance.stateDir,
+      run: async () => {
+        const context = await suite.newBrowserContext({
+          locale: "en-US",
+          viewport: { width: 1200, height: 800 },
+        });
+        const page = await context.newPage();
+        const errors: string[] = [];
+        page.on("pageerror", (error) => errors.push(error.message));
+        const inFlight = new Set<string>();
+        const release: (() => void)[] = [];
+        let holding = false;
+        let requests = 0;
+        let peak = 0;
+        await page.routeWebSocket("**", (socket) => {
+          const server = socket.connectToServer();
+          socket.onMessage((message) => {
+            const frame: unknown = JSON.parse(message.toString());
+            if (
+              isRecord(frame) &&
+              frame.method === "approval.get" &&
+              typeof frame.id === "string"
+            ) {
+              requests += 1;
+              inFlight.add(frame.id);
+              peak = Math.max(peak, inFlight.size);
+            }
+            server.send(message);
+          });
+          server.onMessage((message) => {
+            const frame: unknown = JSON.parse(message.toString());
+            const id = isRecord(frame) && typeof frame.id === "string" ? frame.id : undefined;
+            const deliver = () => {
+              if (id) {
+                inFlight.delete(id);
+              }
+              socket.send(message);
+            };
+            if (holding && id && inFlight.has(id)) {
+              release.push(deliver);
+            } else {
+              deliver();
+            }
+          });
+        });
+        try {
+          await pairControlUiPage(page, runCli);
+          const id = "approval-lookup-proof";
+          const accepted: unknown = JSON.parse(
+            await runCli([
+              "gateway",
+              "call",
+              "exec.approval.request",
+              "--json",
+              "--params",
+              JSON.stringify({
+                id,
+                command: "printf synthetic-approval",
+                host: "gateway",
+                twoPhase: true,
+                suppressDelivery: true,
+                requireDeliveryRoute: false,
+                timeoutMs: 60_000,
+              }),
+            ]),
+          );
+          expect(accepted).toMatchObject({ status: "accepted", id });
+          await page.goto(new URL(`/approve/${id}`, suite.server.baseUrl).href);
+          await page.getByText("Waiting for your decision", { exact: true }).waitFor();
+          const script = await page
+            .locator('script[type="module"][src]')
+            .first()
+            .getAttribute("src");
+          if (!script?.startsWith("/assets/")) {
+            throw new Error("Expected the built Control UI entrypoint");
+          }
+          const asset = await context.request.get(new URL(script, suite.server.baseUrl).href);
+          expect(asset.ok()).toBe(true);
+          const served = await asset.body();
+          expect(served.equals(await readFile(path.join("dist/control-ui", script.slice(1))))).toBe(
+            true,
+          );
+          const uiAssetSha256 = createHash("sha256").update(served).digest("hex");
+          await expect.poll(() => inFlight.size).toBe(0);
+          // Only the external visibility input and reply latency are simulated.
+          // Every request and response still traverses the real Gateway.
+          const visibility = async (value: DocumentVisibilityState) => {
+            await page.evaluate((state) => {
+              Object.defineProperty(document, "visibilityState", {
+                configurable: true,
+                value: state,
+              });
+              document.dispatchEvent(new Event("visibilitychange"));
+            }, value);
+          };
+          await visibility("hidden");
+          const initial = requests;
+          peak = 0;
+          holding = true;
+          await visibility("visible");
+          await expect.poll(() => release.length).toBe(1);
+          for (let cycle = 0; cycle < 4; cycle += 1) {
+            await visibility("hidden");
+            await visibility("visible");
+          }
+          expect(requests - initial).toBe(1);
+          expect(peak).toBe(1);
+          holding = false;
+          for (const deliver of release) {
+            deliver();
+          }
+          await expect.poll(() => requests - initial).toBe(2);
+          await visibility("hidden");
+          await expect.poll(() => inFlight.size).toBe(0);
+          await page.getByRole("button", { name: "Deny", exact: true }).click();
+          await page.getByRole("heading", { name: "Denied here", exact: true }).waitFor();
+          expect(requests - initial).toBe(2);
+          expect(peak).toBe(1);
+          expect(errors).toEqual([]);
+          await writeFile(
+            path.join(suite.artifactDir, "lookup-counts.json"),
+            JSON.stringify({
+              visibilityCycles: 5,
+              requests: requests - initial,
+              peak,
+              terminal: "Denied here",
+              uiAssetSha256,
+            }) + "\n",
+          );
+        } finally {
+          await suite.closeBrowserContext(context);
+        }
+      },
+    });
+  });
+});

ui/src/pages/approval/approval-page.test.ts

@@ -155,6 +155,8 @@ async function createPage(
   return { page, source };
 }
 
+type PageSource = Awaited<ReturnType<typeof createPage>>["source"];
+
 function operator(scopes: string[]): ApplicationGatewaySnapshot["hello"] {
   return { auth: { role: "operator", scopes } } as ApplicationGatewaySnapshot["hello"];
 }
@@ -666,6 +668,164 @@ describe("ApprovalPage", () => {
     expect(request).toHaveBeenCalledTimes(3);
   });
 
+  it("keeps one approval.get in flight across repeated visibility refreshes", async () => {
+    const pending = pendingApproval();
+    const { promise: heldLookup, resolve: releaseLookup } =
+      Promise.withResolvers<ApprovalGetResult>();
+    const request = vi
+      .fn()
+      .mockResolvedValueOnce({ approval: pending } satisfies ApprovalGetResult)
+      .mockReturnValueOnce(heldLookup)
+      .mockResolvedValue({ approval: pending } satisfies ApprovalGetResult);
+    const { page } = await createPage(request);
+    for (let cycle = 0; cycle < 5; cycle += 1) {
+      visibilityState = "hidden";
+      document.dispatchEvent(new Event("visibilitychange"));
+      visibilityState = "visible";
+      document.dispatchEvent(new Event("visibilitychange"));
+    }
+    await settle(page);
+    expect(request).toHaveBeenCalledTimes(2);
+    releaseLookup({ approval: pending });
+    await settle(page);
+    await settle(page);
+    expect(request).toHaveBeenCalledTimes(3);
+    expect(text(page, ".approval-page__preview")).toBe("printf safe");
+  });
+
+  it.each<
+    [string, (page: TestApprovalPage, source: PageSource, request: GatewayRequestHandler) => void]
+  >([
+    ["the approval ID changes", (page) => (page.approvalId = "exec:approval-2")],
+    [
+      "the client changes",
+      (_, source, request) => source.update({ client: createTestGatewayClient(request) }),
+    ],
+  ])("drops the queued follow-up when %s", async (_, retire) => {
+    const pending = pendingApproval();
+    const { promise: heldLookup, resolve: releaseLookup } =
+      Promise.withResolvers<ApprovalGetResult>();
+    const request = vi
+      .fn()
+      .mockResolvedValueOnce({ approval: pending } satisfies ApprovalGetResult)
+      .mockReturnValueOnce(heldLookup)
+      .mockImplementation(async (_method: string, params: unknown) => {
+        if (!isRecord(params) || typeof params.id !== "string") {
+          throw new Error("Expected an approval ID");
+        }
+        return { approval: pendingApproval({ id: params.id }) } satisfies ApprovalGetResult;
+      });
+    const { page, source } = await createPage(request, { hello: operator(["operator.approvals"]) });
+    // The first refresh sends the held lookup; the second queues its follow-up.
+    document.dispatchEvent(new Event("visibilitychange"));
+    document.dispatchEvent(new Event("visibilitychange"));
+    retire(page, source, request);
+    await settle(page);
+    expect(request).toHaveBeenCalledTimes(3);
+    releaseLookup({ approval: pending });
+    await settle(page);
+    await settle(page);
+    expect(request).toHaveBeenCalledTimes(3);
+    expect(text(page, ".approval-page__preview")).toBe("printf safe");
+    expect(decision(page).disabled).toBe(false);
+  });
+
+  it.each<[string, number, (lookup: PromiseWithResolvers<ApprovalGetResult>) => void]>([
+    [
+      "the tab is hidden again",
+      2,
+      ({ resolve }) => {
+        visibilityState = "hidden";
+        document.dispatchEvent(new Event("visibilitychange"));
+        resolve({ approval: pendingApproval() });
+      },
+    ],
+    ["the approval has expired", 2, ({ resolve }) => resolve({ approval: expiredApproval() })],
+    ["the lookup fails", 3, ({ reject }) => reject(new Error("temporary Gateway failure"))],
+  ])("runs the queued follow-up only when a poll would run after %s", async (_, calls, finish) => {
+    const lookup = Promise.withResolvers<ApprovalGetResult>();
+    const request = vi
+      .fn()
+      .mockResolvedValueOnce({ approval: pendingApproval() } satisfies ApprovalGetResult)
+      .mockReturnValueOnce(lookup.promise)
+      .mockResolvedValue({ approval: pendingApproval() } satisfies ApprovalGetResult);
+    const { page } = await createPage(request);
+    document.dispatchEvent(new Event("visibilitychange"));
+    document.dispatchEvent(new Event("visibilitychange"));
+    finish(lookup);
+    await settle(page);
+    await settle(page);
+    expect(request).toHaveBeenCalledTimes(calls);
+  });
+
+  it.each(["applied", "reconciled"] as const)(
+    "retires a queued lookup when a decision is %s",
+    async (outcome) => {
+      const staleLookup = Promise.withResolvers<ApprovalGetResult>();
+      const pending = pendingApproval();
+      const request = vi
+        .fn()
+        .mockResolvedValueOnce({ approval: pending } satisfies ApprovalGetResult)
+        .mockReturnValueOnce(staleLookup.promise)
+        .mockResolvedValueOnce({
+          applied: true,
+          approval: allowedApproval(),
+        } satisfies ApprovalResolveResult)
+        .mockResolvedValue({ approval: expiredApproval() } satisfies ApprovalGetResult);
+      const { page } = await createPage(request);
+      document.dispatchEvent(new Event("visibilitychange"));
+      document.dispatchEvent(new Event("visibilitychange"));
+      expect(request).toHaveBeenCalledTimes(2);
+      decision(page, outcome === "applied" ? "allow-once" : "deny").click();
+      const title = outcome === "applied" ? "Approved here" : "Expired";
+      await waitForSolid(() => expect(text(page, "h1")).toBe(title));
+      const calls = outcome === "applied" ? 3 : 4;
+      expect(request).toHaveBeenCalledTimes(calls);
+      expect(request.mock.calls.filter(([method]) => method === "approval.resolve")).toHaveLength(
+        1,
+      );
+      staleLookup.resolve({ approval: pending });
+      await settle(page);
+      await settle(page);
+      expect(request).toHaveBeenCalledTimes(calls);
+      expect(text(page, "h1")).toBe(title);
+      expect(page.querySelectorAll("[data-decision]")).toHaveLength(0);
+    },
+  );
+
+  it("shows Retry progress while joining a background lookup", async () => {
+    const pending = pendingApproval();
+    const lookup = Promise.withResolvers<ApprovalGetResult>();
+    const followUp = Promise.withResolvers<ApprovalGetResult>();
+    const request = vi
+      .fn()
+      .mockResolvedValueOnce({ approval: pending } satisfies ApprovalGetResult)
+      .mockRejectedValueOnce(new Error("temporary Gateway failure"))
+      .mockReturnValueOnce(lookup.promise)
+      .mockReturnValueOnce(followUp.promise);
+    const { page } = await createPage(request);
+    document.dispatchEvent(new Event("visibilitychange"));
+    await waitForSolid(() => expect(page.querySelector(".approval-page__callout")).not.toBeNull());
+    document.dispatchEvent(new Event("visibilitychange"));
+    await settle(page);
+    const retry = page.querySelector<HTMLButtonElement>(".approval-page__callout button")!;
+    expect(retry.disabled).toBe(false);
+    expect(page.querySelector(".approval-page__card")?.getAttribute("aria-busy")).toBe("false");
+    retry.click();
+    await waitForSolid(() => expect(retry.disabled).toBe(true));
+    expect(page.querySelector(".approval-page__card")?.getAttribute("aria-busy")).toBe("true");
+    expect(request).toHaveBeenCalledTimes(3);
+    lookup.resolve({ approval: pending });
+    await waitForSolid(() => expect(request).toHaveBeenCalledTimes(4));
+    await waitForSolid(() =>
+      expect(page.querySelector(".approval-page__card")?.getAttribute("aria-busy")).toBe("false"),
+    );
+    followUp.resolve({ approval: pending });
+    await settle(page);
+    expect(request).toHaveBeenCalledTimes(4);
+    expect(decision(page).disabled).toBe(false);
+  });
+
   it("keeps one connection alert and disabled decisions across failed background polls", async () => {
     vi.useFakeTimers();
     const pending = pendingApproval({ expiresAtMs: Date.now() + 60_000 });

ui/src/pages/approval/approval-page.tsx

@@ -95,7 +95,6 @@ function createApprovalPage(
     completion.resolve();
     completion = Promise.withResolvers<void>();
     setRequest({ completion: completion.promise });
-    return completion.resolve;
   }
   const [optimisticDecision, setOptimisticDecision] = createOptimistic<ApprovalDecision | null>(
     null,
@@ -119,6 +118,10 @@ function createApprovalPage(
   });
   let clientOwner: GatewayBrowserClient | null = null;
   let operationGeneration = 0;
+  // Refreshes during an in-flight approval.get queue one follow-up instead of another request.
+  let activeLookup:
+    | { client: GatewayBrowserClient; id: string; generation: number; followUp: boolean }
+    | undefined;
   let pollTimer: ReturnType<typeof globalThis.setTimeout> | undefined;
   let boundApprovalId: string | undefined;
   let heading: HTMLHeadingElement | undefined;
@@ -245,11 +248,26 @@ function createApprovalPage(
     if (!client || !state.connected || !id || !hasApprovalAccess()) {
       return;
     }
+    if (
+      activeLookup?.generation === operationGeneration &&
+      activeLookup.client === client &&
+      activeLookup.id === id
+    ) {
+      activeLookup.followUp = true;
+      if (!options.background) {
+        beginRead();
+        notify();
+      }
+      return;
+    }
     const generation = ++operationGeneration;
+    const lookup = (activeLookup = { client, id, generation, followUp: false });
     const previousStatus = state.approval?.status;
     let shouldFocusTerminal = false;
     clearPollTimer();
-    const completeRead = options.background ? completion.resolve : beginRead();
+    if (!options.background) {
+      beginRead();
+    }
     notify();
     try {
       const result = await client.request<ApprovalGetResult>("approval.get", { id });
@@ -280,12 +298,20 @@ function createApprovalPage(
         state.requestError = "connection";
       }
     } finally {
+      if (activeLookup === lookup) {
+        activeLookup = undefined;
+      }
       if (isCurrentOperation(client, generation, id)) {
-        completeRead();
+        // Retry can attach a foreground read receipt to this same lookup.
+        completion.resolve();
         schedulePoll();
         notify();
       }
     }
+    // Stale lookups returned above; a queued follow-up replaces the poll just armed.
+    if (lookup.followUp && pollTimer !== undefined) {
+      void loadApproval({ background: true });
+    }
     if (shouldFocusTerminal && isCurrentOperation(client, generation, id)) {
       await focusTerminalState();
     }