fix(ui): stop repeated reply preview requests after lookup failures (#130737)

2026-08-27 21:07:01 -06:00 · 2026-08-26 23:50:03 -07:00
parent cf8b58cbd9
commit ee30064562
3 changed files with 306 additions and 24 deletions
@@ -0,0 +1,204 @@
+import { mkdir, writeFile } from "node:fs/promises";
+import path from "node:path";
+import { expect, it } from "vitest";
+import {
+  createChatFlowE2eSuite,
+  expectRequestCountStable,
+  installMockGateway,
+  waitForRequests,
+} from "./chat-flow.test-support.ts";
+
+const suite = createChatFlowE2eSuite();
+
+suite.define(() => {
+  it.each([
+    {
+      name: "temporary lookup failure",
+      artifact: "temporary-failure",
+      response: {
+        __mockError: {
+          code: "UNAVAILABLE",
+          message: "Session transcript projection is rebuilding: reply-preview-session",
+        },
+      },
+    },
+    {
+      name: "permanently unavailable source",
+      artifact: "unavailable-source",
+      response: { ok: false, unavailableReason: "not_found" },
+    },
+  ])(
+    "settles a $name without repeatedly loading the reply preview",
+    async ({ artifact, response }) => {
+      const artifactDir = process.env.OPENCLAW_UI_E2E_ARTIFACT_DIR?.trim();
+      if (artifactDir) {
+        await mkdir(artifactDir, { recursive: true });
+      }
+      const context = await suite.newBrowserContext({
+        locale: "en-US",
+        ...(artifactDir
+          ? { recordVideo: { dir: artifactDir, size: { height: 900, width: 1280 } } }
+          : {}),
+        serviceWorkers: "block",
+        viewport: { height: 900, width: 1280 },
+      });
+      const page = await context.newPage();
+      const gateway = await installMockGateway(page, {
+        historyMessages: [
+          {
+            role: "user",
+            content: [{ type: "text", text: "Follow up on the earlier answer." }],
+            timestamp: 1_800_000_000_000,
+            __openclaw: { id: "reply-message", seq: 101, replyToId: "older-answer" },
+          },
+        ],
+        methodResponses: { "chat.message.get": response },
+        sessionKey: "agent:main:reply-preview",
+      });
+
+      let firstCount = 0;
+      try {
+        await page.goto(`${suite.server.baseUrl}chat`);
+        const reply = page.locator(".chat-pane-cache__pane--active .chat-reply-preview--message");
+        await reply.waitFor({ state: "visible" });
+        expect(await reply.textContent()).toContain("Replying to message");
+        await gateway.waitForRequest("chat.message.get");
+        const composer = page.locator(
+          ".chat-pane-cache__pane--active .agent-chat__composer-combobox textarea",
+        );
+        await composer.fill("This draft remains usable.");
+        expect(await composer.inputValue()).toBe("This draft remains usable.");
+        firstCount = (await gateway.getRequests("chat.message.get")).length;
+        await expectRequestCountStable(gateway, "chat.message.get", 1);
+        if (artifact === "unavailable-source") {
+          await reply.click();
+          await page
+            .locator(".chat-pane-cache__pane--active")
+            .getByRole("alert")
+            .locator("summary")
+            .getByText("The original message is unavailable.", { exact: true })
+            .waitFor();
+          await expectRequestCountStable(gateway, "chat.message.get", 1);
+        }
+      } finally {
+        if (artifactDir) {
+          await page.screenshot({
+            animations: "disabled",
+            path: path.join(artifactDir, `${artifact}.png`),
+          });
+          await writeFile(
+            path.join(artifactDir, `${artifact}.json`),
+            JSON.stringify(
+              {
+                firstCount,
+                finalCount: (await gateway.getRequests("chat.message.get")).length,
+                replyText: await page.locator(".chat-reply-preview--message").allTextContents(),
+              },
+              null,
+              2,
+            ),
+          );
+        }
+        await suite.closeBrowserContext(context);
+      }
+    },
+  );
+
+  it.each(["temporary failure", "previous success", "not found"] as const)(
+    "refreshes a reply preview after reconnect from $0 and keeps source navigation working",
+    async (initial) => {
+      const context = await suite.newBrowserContext({
+        locale: "en-US",
+        serviceWorkers: "block",
+        viewport: { height: 900, width: 1280 },
+      });
+      const page = await context.newPage();
+      const source = {
+        role: "assistant",
+        content: [{ type: "text", text: "The current original answer." }],
+        timestamp: 1_800_000_000_000,
+        __openclaw: { id: "reconnect-source", seq: 1 },
+      };
+      const reply = {
+        role: "user",
+        content: [{ type: "text", text: "A follow-up question after reconnect." }],
+        timestamp: 1_800_000_000_017,
+        __openclaw: { id: "reconnect-reply", seq: 17, replyToId: "reconnect-source" },
+      };
+      const messages = [
+        ...Array.from({ length: 15 }, (_, index) => ({
+          role: index % 2 === 0 ? "assistant" : "user",
+          content: [{ type: "text", text: `Conversation entry ${index + 2}.` }],
+          timestamp: 1_800_000_000_001 + index,
+          __openclaw: { id: `intervening-${index}`, seq: index + 2 },
+        })),
+        reply,
+      ];
+      const gateway = await installMockGateway(page, {
+        historyMessages: messages,
+        methodResponses: {
+          "chat.message.get":
+            initial === "temporary failure"
+              ? {
+                  __mockError: {
+                    code: "UNAVAILABLE",
+                    message: "Transcript temporarily unavailable",
+                  },
+                }
+              : initial === "not found"
+                ? { ok: false, unavailableReason: "not_found" }
+                : {
+                    ok: true,
+                    message: { ...source, content: [{ type: "text", text: "Previous preview." }] },
+                  },
+          "chat.history": {
+            cases: [
+              {
+                match: { offset: messages.length },
+                response: {
+                  messages: [source],
+                  hasMore: false,
+                  totalMessages: 17,
+                  sessionId: "reply-preview-history",
+                },
+              },
+            ],
+          },
+          "chat.startup": {
+            messages,
+            hasMore: true,
+            nextOffset: messages.length,
+            totalMessages: 17,
+            sessionId: "reply-preview-history",
+          },
+        },
+        sessionKey: "agent:main:reply-reconnect",
+      });
+
+      try {
+        await page.goto(`${suite.server.baseUrl}chat`);
+        const preview = page.locator(".chat-pane-cache__pane--active .chat-reply-preview--message");
+        await preview.waitFor();
+        await gateway.waitForRequest("chat.message.get");
+        await expectRequestCountStable(gateway, "chat.message.get", 1);
+        if (initial === "previous success") {
+          expect(await preview.textContent()).toContain("Previous preview.");
+        }
+        await gateway.setMethodResponse("chat.message.get", { ok: true, message: source });
+        const connectCount = (await gateway.getRequests("connect")).length;
+        await gateway.closeLatest(1006, "reply preview recovery");
+        await waitForRequests(gateway, "connect", connectCount + 1);
+        await expect.poll(() => preview.textContent()).toContain("The current original answer.");
+        await expectRequestCountStable(gateway, "chat.message.get", 2);
+
+        await preview.click();
+        await page
+          .locator(".chat-pane-cache__pane--active .chat-text")
+          .getByText("The current original answer.", { exact: true })
+          .waitFor();
+      } finally {
+        await suite.closeBrowserContext(context);
+      }
+    },
+  );
+});
@@ -178,6 +178,87 @@ describe("chat pane native history pagination", () => {
    });
  });

+  it.each(["reconnect", "replacement client"] as const)(
+    "retires a resolved reply preview after %s",
+    async (transition) => {
+      const oldMessage = { role: "assistant", content: "Previous connection's answer" };
+      const newMessage = { role: "assistant", content: "Current connection's answer" };
+      const request = vi
+        .fn()
+        .mockResolvedValueOnce({ ok: true, message: oldMessage })
+        .mockResolvedValueOnce({ ok: true, message: newMessage });
+      const client = { request } as unknown as GatewayBrowserClient;
+      const { pane, state } = createTestChatPane({ client, sessions: {} as SessionCapability });
+
+      pane.requestReplyMessage("source-message");
+      await vi.waitFor(() => expect(pane.readReplyMessage("source-message")).toBe(oldMessage));
+      if (transition === "reconnect") {
+        pane.connectionGeneration += 1;
+        state.connectionEpoch = pane.connectionGeneration;
+      } else {
+        const replacement = { request } as unknown as GatewayBrowserClient;
+        state.client = replacement;
+        pane.connectedClient = replacement;
+        pane.context.gateway.snapshot.client = replacement;
+      }
+
+      expect(pane.readReplyMessage("source-message")).toBeUndefined();
+      pane.requestReplyMessage("source-message");
+      await vi.waitFor(() => expect(pane.readReplyMessage("source-message")).toBe(newMessage));
+      expect(request).toHaveBeenCalledTimes(2);
+    },
+  );
+
+  it.each(["success", "failure"] as const)(
+    "ignores an obsolete reply lookup %s while a reconnected lookup is pending",
+    async (outcome) => {
+      const stale = createDeferred<{ ok: true; message: unknown }>();
+      const fresh = createDeferred<{ ok: true; message: unknown }>();
+      const currentMessage = { role: "assistant", content: "Current answer" };
+      const request = vi.fn().mockReturnValueOnce(stale.promise).mockReturnValueOnce(fresh.promise);
+      const client = { request } as unknown as GatewayBrowserClient;
+      const { pane, state } = createTestChatPane({ client, sessions: {} as SessionCapability });
+
+      pane.requestReplyMessage("source-message");
+      pane.connectionGeneration += 1;
+      state.connectionEpoch = pane.connectionGeneration;
+      pane.requestReplyMessage("source-message");
+      expect(request).toHaveBeenCalledTimes(2);
+      if (outcome === "success") {
+        stale.resolve({ ok: true, message: { role: "assistant", content: "Obsolete answer" } });
+      } else {
+        stale.reject(new Error("Previous connection unavailable"));
+      }
+      await stale.promise.catch(() => {});
+      expect(pane.readReplyMessage("source-message")).toBeUndefined();
+      fresh.resolve({ ok: true, message: currentMessage });
+      await vi.waitFor(() => expect(pane.readReplyMessage("source-message")).toBe(currentMessage));
+      pane.requestReplyMessage("source-message");
+      expect(request).toHaveBeenCalledTimes(2);
+    },
+  );
+
+  it("retries a previously unavailable reply source after reconnect", async () => {
+    const message = { role: "assistant", content: "Source is available again" };
+    const request = vi
+      .fn()
+      .mockResolvedValueOnce({ ok: false, unavailableReason: "not_found" })
+      .mockResolvedValueOnce({ ok: true, message });
+    const client = { request } as unknown as GatewayBrowserClient;
+    const { pane, state } = createTestChatPane({ client, sessions: {} as SessionCapability });
+
+    pane.requestReplyMessage("source-message");
+    await Promise.resolve();
+    pane.requestReplyMessage("source-message");
+    expect(request).toHaveBeenCalledOnce();
+    pane.connectionGeneration += 1;
+    state.connectionEpoch = pane.connectionGeneration;
+    pane.requestReplyMessage("source-message");
+
+    await vi.waitFor(() => expect(pane.readReplyMessage("source-message")).toBe(message));
+    expect(request).toHaveBeenCalledTimes(2);
+  });
+
  it("pages backward until a clicked reply target is loaded, then reveals it", async () => {
    const target = {
      ...nativeHistoryMessage(1, "Original answer"),
@@ -14,7 +14,7 @@ export abstract class ChatPaneReplyNavigation extends ChatPaneSession {
  protected replyMessageRevision = 0;
  private readonly replyMessages = new Map<
    string,
-    { client: object; settled?: boolean; message?: unknown }
+    { client: object; generation: number; message?: unknown }
  >();

  protected abstract loadOlderMessages(): Promise<boolean>;
@@ -24,7 +24,10 @@ export abstract class ChatPaneReplyNavigation extends ChatPaneSession {
    if (!state) {
      return undefined;
    }
-    return this.replyMessages.get(this.replyMessageCacheKey(state.sessionKey, messageId))?.message;
+    const cached = this.replyMessages.get(this.replyMessageCacheKey(state.sessionKey, messageId));
+    return cached?.client === state.client && cached.generation === this.connectionGeneration
+      ? cached.message
+      : undefined;
  };

  protected readonly requestReplyMessage = (messageId: string): void => {
@@ -50,42 +53,36 @@ export abstract class ChatPaneReplyNavigation extends ChatPaneSession {
    const agentId = scopedAgentParamsForSession(scope.state, sessionKey).agentId;
    const cacheKey = this.replyMessageCacheKey(sessionKey, messageId);
    const cached = this.replyMessages.get(cacheKey);
-    if (cached && (cached.client === scope.client || cached.settled)) {
+    if (cached?.client === scope.client && cached.generation === scope.generation) {
      return;
    }
    while (this.replyMessages.size >= 256) {
      this.replyMessages.delete(this.replyMessages.keys().next().value!);
    }
-    this.replyMessages.set(cacheKey, { client: scope.client });
+    const attempt = { client: scope.client, generation: scope.generation };
+    this.replyMessages.set(cacheKey, attempt);
+    let result: ChatMessageGetResult;
    try {
-      const result = await scope.client.request<ChatMessageGetResult>("chat.message.get", {
+      result = await scope.client.request<ChatMessageGetResult>("chat.message.get", {
        sessionKey,
        ...(agentId ? { agentId } : {}),
        messageId,
        maxChars: 500,
      });
-      const pending = this.replyMessages.get(cacheKey);
-      if (pending?.client !== scope.client || pending.settled) {
-        return;
-      }
-      this.replyMessages.set(
-        cacheKey,
-        result.ok && result.message
-          ? { client: scope.client, settled: true, message: result.message }
-          : { client: scope.client, settled: true },
-      );
    } catch {
-      const pending = this.replyMessages.get(cacheKey);
-      if (pending?.client !== scope.client || pending.settled) {
-        return;
-      }
-      this.replyMessages.delete(cacheKey);
+      // Retain the failed attempt so rendering cannot retry it in a loop.
+      // A new logical connection owns a fresh attempt, even with the same client.
+      return;
    }
+    if (!this.isConnectionScopeCurrent(scope) || this.replyMessages.get(cacheKey) !== attempt) {
+      return;
+    }
+    if (!result.ok || !result.message) {
+      return;
+    }
+    this.replyMessages.set(cacheKey, { ...attempt, message: result.message });
    this.replyMessageRevision += 1;
-    if (
-      this.isConnectionScopeCurrent(scope) &&
-      areUiSessionKeysEquivalent(scope.state.sessionKey, sessionKey)
-    ) {
+    if (areUiSessionKeysEquivalent(scope.state.sessionKey, sessionKey)) {
      this.requestUpdate();
    }
  }