mirror of
https://github.com/OpenHands/OpenHands.git
synced 2026-10-06 15:03:43 +08:00
Fix browser panel screenshot flow (#963)
Stop clearing browser state when the Browser tab first mounts, and instead reset it only when switching to a different conversation. This preserves the URL and screenshot that were already emitted for the active conversation. Also update the canvas_ui browser guidance so agents capture a screenshot with browser_get_state(include_screenshot=true) before opening the Browser tab. browser_navigate alone only updates the URL, which left the panel blank even though navigation succeeded. Add regression coverage for both behaviors: - preloaded browser screenshots survive first Browser tab mount - browser state still resets on conversation change - canvas_ui browser instructions require screenshot capture before opening the Browser tab Co-authored-by: Tim O'Farrell <tofarr@gmail.com>
This commit is contained in:
committed by
GitHub
co-authored by
Tim O'Farrell
parent
38253e41ba
commit
efe35446d0
@@ -1,4 +1,4 @@
|
||||
import { describe, it, expect, afterEach, vi } from "vitest";
|
||||
import { describe, it, expect, afterEach, beforeEach, vi } from "vitest";
|
||||
import { screen, render } from "@testing-library/react";
|
||||
import React from "react";
|
||||
|
||||
@@ -31,7 +31,12 @@ import { BrowserPanel } from "#/components/features/browser/browser";
|
||||
import { useBrowserStore } from "#/stores/browser-store";
|
||||
|
||||
describe("Browser", () => {
|
||||
beforeEach(() => {
|
||||
useBrowserStore.getState().reset();
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
useBrowserStore.getState().reset();
|
||||
vi.clearAllMocks();
|
||||
});
|
||||
|
||||
@@ -39,7 +44,6 @@ describe("Browser", () => {
|
||||
useBrowserStore.setState({
|
||||
url: "https://example.com",
|
||||
screenshotSrc: "",
|
||||
reset: vi.fn(),
|
||||
});
|
||||
|
||||
render(<BrowserPanel />);
|
||||
@@ -52,7 +56,6 @@ describe("Browser", () => {
|
||||
url: "https://example.com",
|
||||
screenshotSrc:
|
||||
"data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mN0uGvyHwAFCAJS091fQwAAAABJRU5ErkJggg==",
|
||||
reset: vi.fn(),
|
||||
});
|
||||
|
||||
render(<BrowserPanel />);
|
||||
@@ -60,4 +63,20 @@ describe("Browser", () => {
|
||||
expect(screen.getByText("https://example.com")).toBeInTheDocument();
|
||||
expect(screen.getByAltText("BROWSER$SCREENSHOT_ALT")).toBeInTheDocument();
|
||||
});
|
||||
|
||||
it("does not clear a preloaded screenshot when the browser tab first mounts", () => {
|
||||
const screenshotSrc =
|
||||
"data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mN0uGvyHwAFCAJS091fQwAAAABJRU5ErkJggg==";
|
||||
|
||||
useBrowserStore.setState({
|
||||
url: "https://example.com",
|
||||
screenshotSrc,
|
||||
});
|
||||
|
||||
render(<BrowserPanel />);
|
||||
|
||||
expect(useBrowserStore.getState().screenshotSrc).toBe(screenshotSrc);
|
||||
expect(screen.getByAltText("BROWSER$SCREENSHOT_ALT")).toBeInTheDocument();
|
||||
expect(screen.queryByText("BROWSER$NO_PAGE_LOADED")).not.toBeInTheDocument();
|
||||
});
|
||||
});
|
||||
|
||||
@@ -5,6 +5,7 @@ import { createUserMessageEvent } from "test-utils";
|
||||
import { ConversationWebSocketProvider } from "#/contexts/conversation-websocket-context";
|
||||
import { useEventStore } from "#/stores/use-event-store";
|
||||
import { useOptimisticUserMessageStore } from "#/stores/optimistic-user-message-store";
|
||||
import { useBrowserStore } from "#/stores/browser-store";
|
||||
import { useUserConversation } from "#/hooks/query/use-user-conversation";
|
||||
import EventService from "#/api/event-service/event-service.api";
|
||||
import type { MessageEvent } from "#/types/agent-server/core";
|
||||
@@ -62,6 +63,7 @@ describe("ConversationWebSocketProvider — conversation-scoped event store", ()
|
||||
loadedConversationId: null,
|
||||
});
|
||||
useOptimisticUserMessageStore.setState({ pendingMessages: [] });
|
||||
useBrowserStore.getState().reset();
|
||||
|
||||
vi.mocked(useUserConversation).mockReturnValue({
|
||||
data: { conversation_url: "http://localhost/api", session_api_key: null },
|
||||
@@ -102,6 +104,32 @@ describe("ConversationWebSocketProvider — conversation-scoped event store", ()
|
||||
await waitFor(() => expect(eventIds()).toEqual(["user-msg-conv-b"]));
|
||||
});
|
||||
|
||||
it("resets browser-panel state when switching conversations", async () => {
|
||||
const { rerender } = renderProvider("conv-a");
|
||||
await waitFor(() => expect(eventIds()).toEqual(["user-msg-conv-a"]));
|
||||
|
||||
useBrowserStore.setState({
|
||||
url: "https://example.com",
|
||||
screenshotSrc: "data:image/png;base64,abc123",
|
||||
});
|
||||
|
||||
rerender(
|
||||
<QueryClientProvider client={queryClient}>
|
||||
<ConversationWebSocketProvider
|
||||
conversationId="conv-b"
|
||||
conversationUrl={null}
|
||||
>
|
||||
<div />
|
||||
</ConversationWebSocketProvider>
|
||||
</QueryClientProvider>,
|
||||
);
|
||||
|
||||
await waitFor(() =>
|
||||
expect(useBrowserStore.getState().screenshotSrc).toBe(""),
|
||||
);
|
||||
expect(useBrowserStore.getState().url).toBe("");
|
||||
});
|
||||
|
||||
it("keeps events that arrived after history when re-entering the same conversation", async () => {
|
||||
// Arrange: open conversation A, then receive an agent reply over the socket
|
||||
// that is not part of the cached REST history page.
|
||||
|
||||
@@ -0,0 +1,20 @@
|
||||
import { readFileSync } from "node:fs";
|
||||
import { dirname, resolve } from "node:path";
|
||||
import { fileURLToPath } from "node:url";
|
||||
import { describe, expect, it } from "vitest";
|
||||
|
||||
const repoRoot = resolve(dirname(fileURLToPath(import.meta.url)), "..", "..");
|
||||
const toolSource = readFileSync(resolve(repoRoot, "tools/canvas_ui_tool.py"), "utf8");
|
||||
|
||||
describe("canvas_ui browser guidance", () => {
|
||||
it("tells the agent to capture a browser screenshot before opening the browser tab", () => {
|
||||
const captureInstruction = "browser_get_state(include_screenshot=true)";
|
||||
const openBrowserInstruction = 'command="open_tab", tab="browser"';
|
||||
|
||||
expect(toolSource).toContain(captureInstruction);
|
||||
expect(toolSource).toContain(openBrowserInstruction);
|
||||
expect(toolSource.indexOf(captureInstruction)).toBeLessThan(
|
||||
toolSource.indexOf(openBrowserInstruction),
|
||||
);
|
||||
});
|
||||
});
|
||||
@@ -1,16 +1,9 @@
|
||||
import { useEffect } from "react";
|
||||
import { BrowserSnapshot } from "./browser-snapshot";
|
||||
import { EmptyBrowserMessage } from "./empty-browser-message";
|
||||
import { useConversationId } from "#/hooks/use-conversation-id";
|
||||
import { useBrowserStore } from "#/stores/browser-store";
|
||||
|
||||
export function BrowserPanel() {
|
||||
const { url, screenshotSrc, reset } = useBrowserStore();
|
||||
const { conversationId } = useConversationId();
|
||||
|
||||
useEffect(() => {
|
||||
reset();
|
||||
}, [conversationId, reset]);
|
||||
const { url, screenshotSrc } = useBrowserStore();
|
||||
|
||||
const imgSrc = screenshotSrc?.startsWith("data:image/png;base64,")
|
||||
? screenshotSrc
|
||||
|
||||
@@ -143,6 +143,7 @@ export function ConversationWebSocketProvider({
|
||||
);
|
||||
const { setExecutionStatus } = useConversationStateStore();
|
||||
const { appendInput, appendOutput } = useCommandStore();
|
||||
const resetBrowserStore = useBrowserStore((state) => state.reset);
|
||||
|
||||
// History loading state.
|
||||
// - Main conversation history is now loaded via REST (`useConversationHistory`),
|
||||
@@ -241,7 +242,8 @@ export function ConversationWebSocketProvider({
|
||||
// records the new loaded id in one `set`, so no subscriber can observe a
|
||||
// half-applied state (events gone but the old id still reported).
|
||||
clearEventsForConversation(nextId);
|
||||
}, [conversationId, clearEventsForConversation]);
|
||||
resetBrowserStore();
|
||||
}, [conversationId, clearEventsForConversation, resetBrowserStore]);
|
||||
|
||||
useLayoutEffect(() => {
|
||||
if (!preloadedHistory || preloadedHistory.events.length === 0) {
|
||||
|
||||
@@ -1,9 +1,9 @@
|
||||
import { create } from "zustand";
|
||||
|
||||
interface BrowserState {
|
||||
// URL of browser window (placeholder for now, will be replaced with the actual URL later)
|
||||
// URL of the last page the agent navigated to in the browser panel.
|
||||
url: string;
|
||||
// Base64-encoded screenshot of browser window (placeholder for now, will be replaced with the actual screenshot later)
|
||||
// Base64-encoded screenshot of the browser window, when the tool provides one.
|
||||
screenshotSrc: string;
|
||||
}
|
||||
|
||||
@@ -14,7 +14,7 @@ interface BrowserStore extends BrowserState {
|
||||
}
|
||||
|
||||
const initialState: BrowserState = {
|
||||
url: "https://github.com/OpenHands/OpenHands",
|
||||
url: "",
|
||||
screenshotSrc: "",
|
||||
};
|
||||
|
||||
|
||||
@@ -90,7 +90,11 @@ When to call (pick the most specific option that matches your last action):
|
||||
command="open_tab", tab="terminal"
|
||||
|
||||
* You browsed to a URL the user should see →
|
||||
First call browser_get_state(include_screenshot=true) after your final
|
||||
browser interaction so Agent Canvas has a screenshot to display, then call
|
||||
command="open_tab", tab="browser"
|
||||
(browser_navigate alone only updates the URL; without browser_get_state,
|
||||
the Browser tab will open without a screenshot.)
|
||||
|
||||
Call this BEFORE writing your chat-message summary of the change, so the
|
||||
artifact is visible while the user reads what you did. One canvas_ui call
|
||||
|
||||
Reference in New Issue
Block a user