From d0e76dea4945aa286ca2dc8df805a26e5e287fad Mon Sep 17 00:00:00 2001 From: Shakker Date: Sun, 9 Aug 2026 06:14:43 +0200 Subject: [PATCH] fix: bound browser annotation composer lifecycle --- docs/web/control-ui.md | 2 + .../components/browser/browser-annotation.ts | 19 ++- .../browser/browser-panel-controller.test.ts | 35 +++++ .../browser/browser-panel-controller.ts | 13 +- .../browser/browser-panel-surface.test.ts | 44 +++++- .../browser/browser-panel-surface.ts | 3 +- ui/src/i18n/locales/en.ts | 4 + .../chat/browser-annotation-admission.ts | 31 +++++ .../chat/browser-annotation-removal.test.ts | 125 ++++++++++++------ .../pages/chat/browser-annotation-removal.ts | 13 +- .../chat/chat-pane-browser-annotation.test.ts | 123 +++++++++++++++++ .../chat/chat-pane-browser-annotation.ts | 75 +++++++++++ ui/src/pages/chat/chat-pane-lifecycle.test.ts | 53 +++++++- ui/src/pages/chat/chat-pane-lifecycle.ts | 47 ++----- ui/src/pages/chat/chat-pane-render.ts | 11 +- 15 files changed, 501 insertions(+), 97 deletions(-) create mode 100644 ui/src/pages/chat/browser-annotation-admission.ts create mode 100644 ui/src/pages/chat/chat-pane-browser-annotation.test.ts create mode 100644 ui/src/pages/chat/chat-pane-browser-annotation.ts diff --git a/docs/web/control-ui.md b/docs/web/control-ui.md index 3a4ae5e6ec8a..a2d6c96a68aa 100644 --- a/docs/web/control-ui.md +++ b/docs/web/control-ui.md @@ -424,6 +424,8 @@ Two capture modes package page context for the agent: - **Annotate (pencil)**: draw freehand markup over the page. **Send to chat** composites the strokes into the screenshot and adds one structured annotation card to the active chat composer. The card keeps its generated page and region context with the image instead of inserting it into your editable draft. - **Inspect (pointer)**: hover to see the element under the cursor (selector, accessible name, role, size); click to add those details and a highlighted screenshot through the same card flow. Removing an annotation card removes only that image and its generated context, preserves your draft and other attachments, and offers a short-lived **Undo**. Inspect, wheel scrolling, and back/forward need `browser.evaluateEnabled` (on by default). +One composer accepts up to four browser annotation cards and 8,000 total characters of generated annotation context. When it reaches either limit, the browser panel keeps the current capture so you can remove a card and retry; Undo also preserves the limit instead of evicting another card. + The macOS app keeps its native link-browser sidebar for links clicked in the dashboard; the browser panel works there too, and is the way to annotate pages on every other platform. ## Composer capability menu diff --git a/ui/src/components/browser/browser-annotation.ts b/ui/src/components/browser/browser-annotation.ts index c4de1d832efe..06764723b934 100644 --- a/ui/src/components/browser/browser-annotation.ts +++ b/ui/src/components/browser/browser-annotation.ts @@ -27,20 +27,27 @@ export type BrowserAnnotationDraft = { fileName: string; }; +export type BrowserAnnotationDispatchResult = "accepted" | "rejected" | "unhandled"; +export type BrowserAnnotationEvent = CustomEvent & { + /** Synchronous consumer rejection keeps capture state retryable in the browser panel. */ + rejection?: "limit"; +}; + export const BROWSER_ANNOTATION_EVENT = "openclaw:browser-annotation"; /** - * Hands an annotation to whichever chat pane is active. Returns false when no - * pane consumed it (chat not mounted), so the panel can surface a hint instead - * of silently dropping the user's markup. + * Hands an annotation to whichever chat pane is active. The result distinguishes + * a retryable admission rejection from the absence of a mounted chat target. */ -export function dispatchBrowserAnnotation(draft: BrowserAnnotationDraft): boolean { +export function dispatchBrowserAnnotation( + draft: BrowserAnnotationDraft, +): BrowserAnnotationDispatchResult { const event = new CustomEvent(BROWSER_ANNOTATION_EVENT, { detail: draft, cancelable: true, - }); + }) as BrowserAnnotationEvent; window.dispatchEvent(event); - return event.defaultPrevented; + return event.defaultPrevented ? "accepted" : event.rejection ? "rejected" : "unhandled"; } function clamp01(value: number): number { diff --git a/ui/src/components/browser/browser-panel-controller.test.ts b/ui/src/components/browser/browser-panel-controller.test.ts index 0e6d139816f0..f2314f838fa2 100644 --- a/ui/src/components/browser/browser-panel-controller.test.ts +++ b/ui/src/components/browser/browser-panel-controller.test.ts @@ -1,4 +1,5 @@ import { describe, expect, it, vi } from "vitest"; +import { BROWSER_ANNOTATION_EVENT, type BrowserAnnotationEvent } from "./browser-annotation.ts"; import { createBrowserClient, createBrowserPanelTestController, @@ -19,6 +20,40 @@ import { BrowserPanelController } from "./browser-panel-controller.ts"; setupBrowserPanelTestCleanup(); describe("BrowserPanelController tab and lifecycle ownership", () => { + it("keeps rejected capture state and replaces stale feedback with the limit error", async () => { + vi.spyOn(HTMLCanvasElement.prototype, "getContext").mockReturnValue({ + drawImage: vi.fn(), + beginPath: vi.fn(), + moveTo: vi.fn(), + lineTo: vi.fn(), + stroke: vi.fn(), + } as unknown as CanvasRenderingContext2D); + vi.spyOn(HTMLCanvasElement.prototype, "toDataURL").mockReturnValue( + "data:image/png;base64,annotated", + ); + const { client } = createBrowserClient(async () => ({ running: true, tabs: [] })); + const controller = createBrowserPanelTestController(client, "tab-a"); + controller.setMode("annotate"); + controller.strokes = [{ points: [{ x: 0.25, y: 0.5 }] }]; + controller.errorText = "Stale error"; + controller.noticeText = "Previously sent"; + const reject = (event: Event) => { + (event as BrowserAnnotationEvent).rejection = "limit"; + }; + window.addEventListener(BROWSER_ANNOTATION_EVENT, reject); + + try { + await controller.sendAnnotation({}); + } finally { + window.removeEventListener(BROWSER_ANNOTATION_EVENT, reject); + } + + expect(controller.mode).toBe("annotate"); + expect(controller.strokes).toHaveLength(1); + expect(controller.noticeText).toBeNull(); + expect(controller.errorText).toContain("maximum 4 cards and 8,000 characters"); + }); + it.each(["reject", "resolve"] as const)( "preserves pending new-tab ownership when the previous capture %ss", async (completion) => { diff --git a/ui/src/components/browser/browser-panel-controller.ts b/ui/src/components/browser/browser-panel-controller.ts index a4e2892e0ead..ebb6d6efc6b5 100644 --- a/ui/src/components/browser/browser-panel-controller.ts +++ b/ui/src/components/browser/browser-panel-controller.ts @@ -720,17 +720,24 @@ export class BrowserPanelController implements ReactiveController { return; } const highlight = element ? this.inspectHighlightRegion() : null; - let handled: boolean; + let result: ReturnType; try { - handled = dispatchCompositedBrowserAnnotation(view, tab, this.strokes, element, highlight); + result = dispatchCompositedBrowserAnnotation(view, tab, this.strokes, element, highlight); } catch (error) { this.reportError(error); return; } - if (!handled) { + if (result === "unhandled") { + this.setState("noticeText", null); this.setState("errorText", t("browser.noChatTarget")); return; } + if (result === "rejected") { + this.setState("noticeText", null); + this.setState("errorText", t("browser.annotationLimitReached")); + return; + } + this.setState("errorText", null); this.setState("noticeText", t("browser.annotationSent")); this.exitCaptureModes(); } diff --git a/ui/src/components/browser/browser-panel-surface.test.ts b/ui/src/components/browser/browser-panel-surface.test.ts index 58315b17a4e2..88f5459ef0a2 100644 --- a/ui/src/components/browser/browser-panel-surface.test.ts +++ b/ui/src/components/browser/browser-panel-surface.test.ts @@ -31,7 +31,9 @@ describe("dispatchCompositedBrowserAnnotation", () => { } satisfies BrowserPanelView; const strokes = [{ points: [{ x: 0.25, y: 0.5 }] }]; - expect(dispatchCompositedBrowserAnnotation(view, undefined, strokes, null, null)).toBe(false); + expect(dispatchCompositedBrowserAnnotation(view, undefined, strokes, null, null)).toBe( + "unhandled", + ); expect(drawImage).toHaveBeenCalledTimes(1); expect(toDataUrl).toHaveBeenCalledTimes(1); @@ -42,7 +44,9 @@ describe("dispatchCompositedBrowserAnnotation", () => { }; window.addEventListener(BROWSER_ANNOTATION_EVENT, consume); try { - expect(dispatchCompositedBrowserAnnotation(view, undefined, strokes, null, null)).toBe(true); + expect(dispatchCompositedBrowserAnnotation(view, undefined, strokes, null, null)).toBe( + "accepted", + ); } finally { window.removeEventListener(BROWSER_ANNOTATION_EVENT, consume); } @@ -62,4 +66,40 @@ describe("dispatchCompositedBrowserAnnotation", () => { }); expect(draft).not.toHaveProperty("text"); }); + + it("distinguishes a rejected capture from an unhandled one", () => { + vi.spyOn(HTMLCanvasElement.prototype, "getContext").mockReturnValue({ + drawImage: vi.fn(), + beginPath: vi.fn(), + moveTo: vi.fn(), + lineTo: vi.fn(), + stroke: vi.fn(), + } as unknown as CanvasRenderingContext2D); + vi.spyOn(HTMLCanvasElement.prototype, "toDataURL").mockReturnValue( + "data:image/png;base64,annotated", + ); + const reject = (event: Event) => { + (event as Event & { rejection?: "limit" }).rejection = "limit"; + }; + window.addEventListener(BROWSER_ANNOTATION_EVENT, reject); + try { + expect( + dispatchCompositedBrowserAnnotation( + { + targetId: "tab-1", + dataUrl: "data:image/png;base64,source", + image: { naturalWidth: 800, naturalHeight: 600 } as HTMLImageElement, + url: "https://example.com", + metrics: null, + }, + undefined, + [{ points: [{ x: 0.25, y: 0.5 }] }], + null, + null, + ), + ).toBe("rejected"); + } finally { + window.removeEventListener(BROWSER_ANNOTATION_EVENT, reject); + } + }); }); diff --git a/ui/src/components/browser/browser-panel-surface.ts b/ui/src/components/browser/browser-panel-surface.ts index 96b758545e9b..86efd76a9d38 100644 --- a/ui/src/components/browser/browser-panel-surface.ts +++ b/ui/src/components/browser/browser-panel-surface.ts @@ -1,6 +1,7 @@ import { t } from "../../i18n/index.ts"; import { buildBrowserAnnotationContent, + type BrowserAnnotationDispatchResult, composeAnnotatedImage, dispatchBrowserAnnotation, paintAnnotations, @@ -135,7 +136,7 @@ export function dispatchCompositedBrowserAnnotation( strokes: AnnotationStroke[], element: BrowserInspectedNode | null, highlight: AnnotationRegion | null, -): boolean { +): BrowserAnnotationDispatchResult { const url = view.metrics?.url || view.url || tab?.url || ""; const title = view.metrics?.title || tab?.title || ""; const content = buildBrowserAnnotationContent({ url, title, strokes, element }); diff --git a/ui/src/i18n/locales/en.ts b/ui/src/i18n/locales/en.ts index d2015f69c07b..ab0dcf6710a5 100644 --- a/ui/src/i18n/locales/en.ts +++ b/ui/src/i18n/locales/en.ts @@ -1878,6 +1878,8 @@ export const en: TranslationMap = { start: "Start browser", empty: "No open tabs. Enter a URL above to browse.", noChatTarget: "Open a chat session first so the annotation has somewhere to go.", + annotationLimitReached: + "Remove a browser annotation before retrying (maximum 4 cards and 8,000 characters of generated context).", inspectUnavailable: "Element inspection is disabled (browser.evaluateEnabled=false).", annotationSent: "Annotation added to the chat composer.", errors: { @@ -5036,6 +5038,8 @@ export const en: TranslationMap = { browserAnnotationRegions: "{count} marked regions", browserAnnotationInspectedElement: "Element inspected", browserAnnotationRemoved: "Browser annotation removed.", + browserAnnotationUndoUnavailable: + "Undo is unavailable because the browser annotation limit has been reached.", compactRecommendedContext: "Compact recommended session context", removeAttachment: "Remove attachment", removeBrowserAnnotation: "Remove browser annotation: {name}", diff --git a/ui/src/pages/chat/browser-annotation-admission.ts b/ui/src/pages/chat/browser-annotation-admission.ts new file mode 100644 index 000000000000..4475bb3196a7 --- /dev/null +++ b/ui/src/pages/chat/browser-annotation-admission.ts @@ -0,0 +1,31 @@ +import type { ChatAttachment } from "../../lib/chat/chat-types.ts"; + +const MAX_BROWSER_ANNOTATION_ATTACHMENTS = 4; +const MAX_BROWSER_ANNOTATION_CONTEXT_CHARS = 8_000; + +/** Enforces one aggregate bound for both a new annotation candidate and Undo. */ +export function canAdmitBrowserAnnotation( + attachments: readonly ChatAttachment[], + modelContext: string, +): boolean { + let annotationCount = 1; + let contextLength = modelContext.length; + if (contextLength > MAX_BROWSER_ANNOTATION_CONTEXT_CHARS) { + return false; + } + for (const attachment of attachments) { + const annotation = attachment.browserAnnotation; + if (!annotation) { + continue; + } + annotationCount += 1; + contextLength += annotation.modelContext.length; + if ( + annotationCount > MAX_BROWSER_ANNOTATION_ATTACHMENTS || + contextLength > MAX_BROWSER_ANNOTATION_CONTEXT_CHARS + ) { + return false; + } + } + return true; +} diff --git a/ui/src/pages/chat/browser-annotation-removal.test.ts b/ui/src/pages/chat/browser-annotation-removal.test.ts index 2800cc2850dc..e22957be0e23 100644 --- a/ui/src/pages/chat/browser-annotation-removal.test.ts +++ b/ui/src/pages/chat/browser-annotation-removal.test.ts @@ -3,6 +3,12 @@ import type { ChatAttachment } from "../../lib/chat/chat-types.ts"; import type { ToastOptions } from "../../lib/toast.ts"; import { removeBrowserAnnotationWithUndo } from "./browser-annotation-removal.ts"; +const labels = { + removed: "Removed", + undo: "Undo", + undoUnavailable: "Remove another annotation before undoing", +}; + function annotation(id: string): ChatAttachment { return { id, @@ -18,10 +24,12 @@ function annotation(id: string): ChatAttachment { } function createHost(initial: ChatAttachment[]) { + let owner = {}; let sessionKey = "agent:main"; let attachments = initial; return { host: { + getOwner: () => owner, getSessionKey: () => sessionKey, getAttachments: () => attachments, setAttachments: (next: ChatAttachment[]) => { @@ -35,6 +43,9 @@ function createHost(initial: ChatAttachment[]) { switchSession: (next: string) => { sessionKey = next; }, + replaceOwner: () => { + owner = {}; + }, }; } @@ -48,18 +59,13 @@ describe("browser annotation removal", () => { const releasePayload = vi.fn(); expect( - removeBrowserAnnotationWithUndo( - state.host, - first, - { removed: "Removed", undo: "Undo" }, - { - presentToast: (options) => { - toast = options; - return true; - }, - releasePayload, + removeBrowserAnnotationWithUndo(state.host, first, labels, { + presentToast: (options) => { + toast = options; + return true; }, - ), + releasePayload, + }), ).toBe(true); expect(state.attachments()).toEqual([ordinary, second]); @@ -79,18 +85,13 @@ describe("browser annotation removal", () => { const state = createHost([target]); let toast: ToastOptions | undefined; const releasePayload = vi.fn(); - removeBrowserAnnotationWithUndo( - state.host, - target, - { removed: "Removed", undo: "Undo" }, - { - presentToast: (options) => { - toast = options; - return true; - }, - releasePayload, + removeBrowserAnnotationWithUndo(state.host, target, labels, { + presentToast: (options) => { + toast = options; + return true; }, - ); + releasePayload, + }); toast?.onDismiss?.(reason); toast?.onDismiss?.(reason); @@ -105,18 +106,13 @@ describe("browser annotation removal", () => { const state = createHost([target]); let toast: ToastOptions | undefined; const releasePayload = vi.fn(); - removeBrowserAnnotationWithUndo( - state.host, - target, - { removed: "Removed", undo: "Undo" }, - { - presentToast: (options) => { - toast = options; - return true; - }, - releasePayload, + removeBrowserAnnotationWithUndo(state.host, target, labels, { + presentToast: (options) => { + toast = options; + return true; }, - ); + releasePayload, + }); state.switchSession("agent:other"); toast?.onDismiss?.("action"); @@ -127,20 +123,65 @@ describe("browser annotation removal", () => { expect(state.host.focusRestoredAnnotation).not.toHaveBeenCalled(); }); + it("never restores into a replacement composer owner", () => { + const target = annotation("target"); + const state = createHost([target]); + let toast: ToastOptions | undefined; + const releasePayload = vi.fn(); + removeBrowserAnnotationWithUndo(state.host, target, labels, { + presentToast: (options) => { + toast = options; + return true; + }, + releasePayload, + }); + state.replaceOwner(); + + toast?.onDismiss?.("action"); + toast?.onAction?.(); + + expect(state.attachments()).toEqual([]); + expect(releasePayload).toHaveBeenCalledOnce(); + expect(state.host.focusRestoredAnnotation).not.toHaveBeenCalled(); + }); + + it("releases instead of exceeding the bound when Undo follows a replacement", () => { + const target = annotation("target"); + const state = createHost([ + target, + annotation("second"), + annotation("third"), + annotation("fourth"), + ]); + const toasts: ToastOptions[] = []; + const releasePayload = vi.fn(); + removeBrowserAnnotationWithUndo(state.host, target, labels, { + presentToast: (options) => { + toasts.push(options); + return true; + }, + releasePayload, + }); + state.host.setAttachments([...state.attachments(), annotation("replacement")]); + + toasts[0]?.onDismiss?.("action"); + toasts[0]?.onAction?.(); + + expect(state.attachments()).toHaveLength(4); + expect(state.attachments()).not.toContain(target); + expect(releasePayload).toHaveBeenCalledOnce(); + expect(toasts[1]?.message).toBe(labels.undoUnavailable); + }); + it("releases immediately when no toast host can present Undo", () => { const target = annotation("target"); const state = createHost([target]); const releasePayload = vi.fn(); - removeBrowserAnnotationWithUndo( - state.host, - target, - { removed: "Removed", undo: "Undo" }, - { - presentToast: () => false, - releasePayload, - }, - ); + removeBrowserAnnotationWithUndo(state.host, target, labels, { + presentToast: () => false, + releasePayload, + }); expect(releasePayload).toHaveBeenCalledOnce(); }); diff --git a/ui/src/pages/chat/browser-annotation-removal.ts b/ui/src/pages/chat/browser-annotation-removal.ts index d84fb6e740f1..d5006b473718 100644 --- a/ui/src/pages/chat/browser-annotation-removal.ts +++ b/ui/src/pages/chat/browser-annotation-removal.ts @@ -1,8 +1,10 @@ import type { ChatAttachment } from "../../lib/chat/chat-types.ts"; import { showToast, type ToastOptions } from "../../lib/toast.ts"; import { releaseChatAttachmentPayload } from "./attachment-payload-store.ts"; +import { canAdmitBrowserAnnotation } from "./browser-annotation-admission.ts"; type BrowserAnnotationRemovalHost = { + getOwner: () => object | undefined; getSessionKey: () => string; getAttachments: () => ChatAttachment[]; setAttachments: (attachments: ChatAttachment[]) => void; @@ -20,12 +22,14 @@ type BrowserAnnotationRemovalDependencies = { export function removeBrowserAnnotationWithUndo( host: BrowserAnnotationRemovalHost, attachment: ChatAttachment, - labels: { removed: string; undo: string }, + labels: { removed: string; undo: string; undoUnavailable: string }, dependencies: BrowserAnnotationRemovalDependencies = {}, ): boolean { if (!attachment.browserAnnotation) { return false; } + const modelContext = attachment.browserAnnotation.modelContext; + const sourceOwner = host.getOwner(); const sourceSessionKey = host.getSessionKey(); const current = host.getAttachments(); const sourceIndex = current.findIndex((candidate) => candidate.id === attachment.id); @@ -54,7 +58,7 @@ export function removeBrowserAnnotationWithUndo( if (settled) { return; } - if (host.getSessionKey() !== sourceSessionKey) { + if (host.getOwner() !== sourceOwner || host.getSessionKey() !== sourceSessionKey) { finalizeRemoval(); return; } @@ -63,6 +67,11 @@ export function removeBrowserAnnotationWithUndo( settled = true; return; } + if (!canAdmitBrowserAnnotation(latest, modelContext)) { + finalizeRemoval(); + presentToast({ message: labels.undoUnavailable }); + return; + } settled = true; const insertionIndex = Math.min(sourceIndex, latest.length); host.setAttachments([ diff --git a/ui/src/pages/chat/chat-pane-browser-annotation.test.ts b/ui/src/pages/chat/chat-pane-browser-annotation.test.ts new file mode 100644 index 000000000000..b7e4bd301786 --- /dev/null +++ b/ui/src/pages/chat/chat-pane-browser-annotation.test.ts @@ -0,0 +1,123 @@ +/* @vitest-environment jsdom */ + +import { describe, expect, it, vi } from "vitest"; +import type { + BrowserAnnotationDraft, + BrowserAnnotationEvent, +} from "../../components/browser/browser-annotation.ts"; +import type { ChatAttachment } from "../../lib/chat/chat-types.ts"; +import { + getChatAttachmentDataUrl, + registerChatAttachmentPayload, + releaseChatAttachmentPayload, +} from "./attachment-payload-store.ts"; +import { canAdmitBrowserAnnotation } from "./browser-annotation-admission.ts"; +import { + receiveBrowserAnnotation, + releasePaneBrowserAnnotations, +} from "./chat-pane-browser-annotation.ts"; +import type { ChatPageHost } from "./chat-state-host.ts"; + +function annotation(id: string, modelContext = `Context ${id}`): ChatAttachment { + return { + id, + mimeType: "image/png", + browserAnnotation: { + modelContext, + title: `Page ${id}`, + displayUrl: "example.com", + markedRegionCount: 1, + inspectedElement: false, + }, + }; +} + +function draft(modelContext: string): BrowserAnnotationDraft { + return { + modelContext, + dataUrl: "data:image/png;base64,aGVsbG8=", + fileName: "annotated-page.png", + card: { + title: "Example", + displayUrl: "example.com", + markedRegionCount: 1, + inspectedElement: false, + }, + }; +} + +describe("browser annotation admission", () => { + it("includes the candidate in both the four-card and 8,000-character bounds", () => { + expect(canAdmitBrowserAnnotation([], "x".repeat(8_000))).toBe(true); + expect(canAdmitBrowserAnnotation([], "x".repeat(8_001))).toBe(false); + expect( + canAdmitBrowserAnnotation( + [annotation("one"), annotation("two"), annotation("three")], + "fourth", + ), + ).toBe(true); + expect( + canAdmitBrowserAnnotation( + [annotation("one"), annotation("two"), annotation("three"), annotation("four")], + "fifth", + ), + ).toBe(false); + }); + + it("marks an active-pane rejection without allocating or consuming the capture", () => { + const state = { + chatAttachments: [ + annotation("one"), + annotation("two"), + annotation("three"), + annotation("four"), + ], + requestUpdate: vi.fn(), + } as unknown as ChatPageHost; + const event = new CustomEvent("openclaw:browser-annotation", { + detail: draft("Rejected context"), + cancelable: true, + }); + expect(receiveBrowserAnnotation(state, true, event)).toBe(false); + expect(event.defaultPrevented).toBe(false); + expect((event as BrowserAnnotationEvent).rejection).toBe("limit"); + expect(state.chatAttachments).toHaveLength(4); + }); +}); + +describe("browser annotation pane teardown", () => { + it("deduplicates current and fallback annotations while preserving ordinary payloads", () => { + const stored = (attachment: ChatAttachment, payload: string) => + registerChatAttachmentPayload({ + attachment, + dataUrl: payload, + file: new File([payload], `${attachment.id}.png`, { type: attachment.mimeType }), + }); + const shared = stored(annotation("shared"), "data:image/png;base64,c2hhcmVk"); + const fallback = stored(annotation("fallback"), "data:image/png;base64,ZmFsbGJhY2s="); + const ordinary = stored( + { id: "ordinary", mimeType: "image/png" }, + "data:image/png;base64,b3JkaW5hcnk=", + ); + const state = { + chatAttachments: [shared, ordinary], + chatComposerFallbackByScope: { + fallback: { + attachments: [shared, fallback, ordinary], + message: "", + sequence: 1, + storageFailed: false, + }, + }, + } as unknown as ChatPageHost; + + const releasePayload = vi.fn((id: string) => releaseChatAttachmentPayload(id)); + releasePaneBrowserAnnotations(state, releasePayload); + + expect(releasePayload.mock.calls.map(([id]) => id)).toEqual(["shared", "fallback"]); + expect(getChatAttachmentDataUrl(shared)).toBeNull(); + expect(getChatAttachmentDataUrl(fallback)).toBeNull(); + expect(getChatAttachmentDataUrl(ordinary)).not.toBeNull(); + releaseChatAttachmentPayload(ordinary.id); + }); +}); diff --git a/ui/src/pages/chat/chat-pane-browser-annotation.ts b/ui/src/pages/chat/chat-pane-browser-annotation.ts new file mode 100644 index 000000000000..5d449a4453c5 --- /dev/null +++ b/ui/src/pages/chat/chat-pane-browser-annotation.ts @@ -0,0 +1,75 @@ +import type { + BrowserAnnotationDraft, + BrowserAnnotationEvent, +} from "../../components/browser/browser-annotation.ts"; +import type { ChatAttachment } from "../../lib/chat/chat-types.ts"; +import { releaseChatAttachmentPayload } from "./attachment-payload-store.ts"; +import { canAdmitBrowserAnnotation } from "./browser-annotation-admission.ts"; +import type { ChatPageHost } from "./chat-state-host.ts"; +import { chatAttachmentFromDataUrl } from "./components/chat-attachments.ts"; + +/** Adopts one complete browser annotation without mixing generated context into the user's draft. */ +export function receiveBrowserAnnotation( + state: ChatPageHost | null | undefined, + active: boolean, + event: Event, +): boolean { + if (!state || !active || event.defaultPrevented || !(event instanceof CustomEvent)) { + return false; + } + const detail = event.detail as BrowserAnnotationDraft | null; + if ( + !detail || + typeof detail.modelContext !== "string" || + typeof detail.dataUrl !== "string" || + !detail.card + ) { + return false; + } + if (!canAdmitBrowserAnnotation(state.chatAttachments, detail.modelContext)) { + // A rejected capture remains editable in the browser panel for a later retry. + (event as BrowserAnnotationEvent).rejection = "limit"; + return false; + } + const attachment = chatAttachmentFromDataUrl(detail.dataUrl, detail.fileName || "annotation"); + if (!attachment) { + return false; + } + event.preventDefault(); + state.chatAttachments = [ + ...state.chatAttachments, + { + ...attachment, + browserAnnotation: { + modelContext: detail.modelContext, + title: detail.card.title, + displayUrl: detail.card.displayUrl, + markedRegionCount: detail.card.markedRegionCount, + inspectedElement: detail.card.inspectedElement, + }, + }, + ]; + state.requestUpdate?.(); + return true; +} + +/** Releases only annotation-owned payloads when a pane's state is discarded. */ +export function releasePaneBrowserAnnotations( + state: ChatPageHost, + releasePayload = releaseChatAttachmentPayload, +): void { + const released = new Set(); + const release = (attachments: readonly ChatAttachment[]) => { + for (const attachment of attachments) { + if (!attachment.browserAnnotation || released.has(attachment.id)) { + continue; + } + released.add(attachment.id); + releasePayload(attachment.id); + } + }; + release(state.chatAttachments); + for (const fallback of Object.values(state.chatComposerFallbackByScope)) { + release(fallback.attachments); + } +} diff --git a/ui/src/pages/chat/chat-pane-lifecycle.test.ts b/ui/src/pages/chat/chat-pane-lifecycle.test.ts index 077fc2e9f55a..3ea7ce47f1c6 100644 --- a/ui/src/pages/chat/chat-pane-lifecycle.test.ts +++ b/ui/src/pages/chat/chat-pane-lifecycle.test.ts @@ -14,6 +14,11 @@ import type { ApplicationContext } from "../../app/context.ts"; import { createInitialUserMessageHandoff } from "../../app/initial-user-message-handoff.ts"; import type { BrowserAnnotationDraft } from "../../components/browser/browser-annotation.ts"; import type { SessionCapability } from "../../lib/sessions/index.ts"; +import { + getChatAttachmentDataUrl, + registerChatAttachmentPayload, + releaseChatAttachmentPayload, +} from "./attachment-payload-store.ts"; import { createTestChatPane, type TestChatPane } from "./chat-pane.test-support.ts"; import { applySelectedChatAgent } from "./chat-session.ts"; import type { ChatPageHost } from "./chat-state-host.ts"; @@ -27,7 +32,53 @@ import { prepareInitialUserMessageHandoff } from "./initial-turn-handoff.ts"; const SKIP_REWIND_CONFIRM_PREFERENCE = "openclaw:skip-rewind-confirm"; const confirmationOwners = new Set(); -describe("browser annotation composer handoff", () => { +describe("browser annotation composer adoption", () => { + it("releases annotation payloads before pane state is discarded on disconnect", () => { + const { pane, state } = createTestChatPane({ + client: {} as GatewayBrowserClient, + sessions: {} as SessionCapability, + }); + const stored = (id: string, browserAnnotation: boolean) => + registerChatAttachmentPayload({ + attachment: { + id, + mimeType: "image/png", + ...(browserAnnotation + ? { + browserAnnotation: { + modelContext: "Context", + title: "Page", + displayUrl: "example.com", + markedRegionCount: 1, + inspectedElement: false, + }, + } + : {}), + }, + dataUrl: `data:image/png;base64,${id}`, + file: new File([id], `${id}.png`, { type: "image/png" }), + }); + const shared = stored("shared-annotation", true); + const fallback = stored("fallback-annotation", true); + const ordinary = stored("ordinary", false); + state.chatAttachments = [shared, ordinary]; + state.chatComposerFallbackByScope = { + fallback: { + attachments: [shared, fallback, ordinary], + message: "", + sequence: 1, + storageFailed: false, + }, + }; + + pane.disconnectedCallback(); + + expect(getChatAttachmentDataUrl(shared)).toBeNull(); + expect(getChatAttachmentDataUrl(fallback)).toBeNull(); + expect(getChatAttachmentDataUrl(ordinary)).not.toBeNull(); + releaseChatAttachmentPayload(ordinary.id); + }); + it("keeps generated context on the attachment and leaves the user's draft unchanged", () => { const { pane, state } = createTestChatPane({ client: {} as GatewayBrowserClient, diff --git a/ui/src/pages/chat/chat-pane-lifecycle.ts b/ui/src/pages/chat/chat-pane-lifecycle.ts index 470ce268bb12..c33bd3743613 100644 --- a/ui/src/pages/chat/chat-pane-lifecycle.ts +++ b/ui/src/pages/chat/chat-pane-lifecycle.ts @@ -12,10 +12,7 @@ import { handleQuestionPromptEvent, } from "../../app/question-prompt.ts"; import { readPresenceEntries } from "../../app/user-profile.ts"; -import { - BROWSER_ANNOTATION_EVENT, - type BrowserAnnotationDraft, -} from "../../components/browser/browser-annotation.ts"; +import { BROWSER_ANNOTATION_EVENT } from "../../components/browser/browser-annotation.ts"; import { t } from "../../i18n/index.ts"; import { resolveAsciiShortcutKey } from "../../lib/keyboard-shortcuts.ts"; import { resolveChatPaneObserverRunId } from "../../lib/observer-digest.ts"; @@ -31,6 +28,10 @@ import { import { invalidateChatAvatarCache, refreshChatAvatar } from "./chat-avatar.ts"; import { clearChatHistory } from "./chat-history.ts"; import { ChatPaneBoard } from "./chat-pane-board.ts"; +import { + receiveBrowserAnnotation as admitBrowserAnnotation, + releasePaneBrowserAnnotations, +} from "./chat-pane-browser-annotation.ts"; import { CHAT_COMPOSER_TEXTAREA_SELECTOR, CHAT_MODAL_SELECTOR, @@ -49,7 +50,6 @@ import { createPageState } from "./chat-state-page.ts"; import { invalidateChatMetadataCache, refreshPageChat } from "./chat-state-refresh.ts"; import { selectedChatSessionRow, canCreateChatSession } from "./chat-state-route.ts"; import { resetChatViewState } from "./chat-view-state.ts"; -import { chatAttachmentFromDataUrl } from "./components/chat-attachments.ts"; import { dismissConfirmedActionPopovers } from "./components/chat-message.ts"; import { clearChatModelSearchOnEscape } from "./components/chat-model-picker.ts"; import { toggleSessionWorkspace } from "./components/chat-session-workspace.ts"; @@ -315,40 +315,10 @@ export abstract class ChatPaneLifecycle extends ChatPaneBoard { /** Receives one complete browser annotation without mixing generated context into the user's draft. */ protected receiveBrowserAnnotation(event: Event): void { - const state = this.state; - // Only the active pane consumes the annotation; defaultPrevented tells the - // browser panel it landed (and stops sibling panes from double-adding). - if (!state || !this.active || event.defaultPrevented || !(event instanceof CustomEvent)) { + const accepted = admitBrowserAnnotation(this.state, this.active, event); + if (!accepted) { return; } - const detail = event.detail as BrowserAnnotationDraft | null; - if ( - !detail || - typeof detail.modelContext !== "string" || - typeof detail.dataUrl !== "string" || - !detail.card - ) { - return; - } - const attachment = chatAttachmentFromDataUrl(detail.dataUrl, detail.fileName || "annotation"); - if (!attachment) { - return; - } - event.preventDefault(); - state.chatAttachments = [ - ...state.chatAttachments, - { - ...attachment, - browserAnnotation: { - modelContext: detail.modelContext, - title: detail.card.title, - displayUrl: detail.card.displayUrl, - markedRegionCount: detail.card.markedRegionCount, - inspectedElement: detail.card.inspectedElement, - }, - }, - ]; - state.requestUpdate?.(); void this.updateComplete.then(() => { this.querySelector(CHAT_COMPOSER_TEXTAREA_SELECTOR)?.focus({ preventScroll: true, @@ -711,6 +681,9 @@ export abstract class ChatPaneLifecycle extends ChatPaneBoard { } override disconnectedCallback() { + if (this.state) { + releasePaneBrowserAnnotations(this.state); + } this.clearComposerPrefillAttention(); this.retainedBoardSessionKey = ""; this.boardProviderLifecycleConnected = false; diff --git a/ui/src/pages/chat/chat-pane-render.ts b/ui/src/pages/chat/chat-pane-render.ts index 963856554340..fd72ac03d5e5 100644 --- a/ui/src/pages/chat/chat-pane-render.ts +++ b/ui/src/pages/chat/chat-pane-render.ts @@ -86,6 +86,7 @@ export class ChatPane extends ChatPaneHeader { const sourceSessionKey = state.sessionKey; removeBrowserAnnotationWithUndo( { + getOwner: () => this.state, getSessionKey: () => this.state?.sessionKey ?? "", getAttachments: () => this.state?.chatAttachments ?? [], setAttachments: (attachments) => { @@ -96,7 +97,7 @@ export class ChatPane extends ChatPaneHeader { requestUpdate: () => this.state?.requestUpdate?.(), focusComposer: () => { void this.updateComplete.then(() => { - if (this.state?.sessionKey !== sourceSessionKey) { + if (this.state !== state || this.state.sessionKey !== sourceSessionKey) { return; } this.querySelector(CHAT_COMPOSER_TEXTAREA_SELECTOR)?.focus({ @@ -106,7 +107,7 @@ export class ChatPane extends ChatPaneHeader { }, focusRestoredAnnotation: (attachmentId) => { void this.updateComplete.then(() => { - if (this.state?.sessionKey !== sourceSessionKey) { + if (this.state !== state || this.state.sessionKey !== sourceSessionKey) { return; } const card = [...this.querySelectorAll("[data-attachment-id]")].find( @@ -117,7 +118,11 @@ export class ChatPane extends ChatPaneHeader { }, }, attachment, - { removed: t("chat.composer.browserAnnotationRemoved"), undo: t("common.undo") }, + { + removed: t("chat.composer.browserAnnotationRemoved"), + undo: t("common.undo"), + undoUnavailable: t("chat.composer.browserAnnotationUndoUnavailable"), + }, ); };