From c97e85448bea815abca481a403792e4dc7450645 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Flor=C3=AAncio?= Date: Fri, 7 Aug 2026 18:11:37 +0100 Subject: [PATCH] fix: restore browser inspect annotation submission (#2374) (#2381) --- .../agentctl/server/api/html_injector_test.go | 34 +++++ .../agentctl/server/api/scripts/inspector.js | 2 + .../preview/inspector-submission.spec.ts | 124 ++++++++++++++++++ .../browser-inspect-annotations-save/plan.md | 101 ++++++++++++++ .../task-01-inspector-state.md | 64 +++++++++ .../task-02-preview-submit-e2e.md | 62 +++++++++ docs/specs/INDEX.md | 1 + .../browser-inspect-annotations-save/spec.md | 56 ++++++++ 8 files changed, 444 insertions(+) create mode 100644 apps/web/e2e/tests/preview/inspector-submission.spec.ts create mode 100644 docs/plans/browser-inspect-annotations-save/plan.md create mode 100644 docs/plans/browser-inspect-annotations-save/task-01-inspector-state.md create mode 100644 docs/plans/browser-inspect-annotations-save/task-02-preview-submit-e2e.md create mode 100644 docs/specs/browser-inspect-annotations-save/spec.md diff --git a/apps/backend/internal/agentctl/server/api/html_injector_test.go b/apps/backend/internal/agentctl/server/api/html_injector_test.go index 5e6ddb788b..8276f9fd0d 100644 --- a/apps/backend/internal/agentctl/server/api/html_injector_test.go +++ b/apps/backend/internal/agentctl/server/api/html_injector_test.go @@ -52,6 +52,40 @@ func TestInspectorScript_UsesPreviewRouteForAnnotationPagePath(t *testing.T) { } } +func TestInspectorScript_PreservesPendingAnnotationAcrossPopupCleanup(t *testing.T) { + openStart := strings.Index(inspectorScript, "function openCommentPopup(") + if openStart < 0 { + t.Fatal("inspector should define openCommentPopup") + } + openEnd := strings.Index(inspectorScript[openStart:], "function commitAnnotation(") + if openEnd < 0 { + t.Fatal("inspector should define commitAnnotation after openCommentPopup") + } + openBody := inspectorScript[openStart : openStart+openEnd] + + keep := strings.Index(openBody, "var keep = pendingAnnotation;") + cleanup := strings.Index(openBody, "closePopup();") + restore := strings.Index(openBody, "pendingAnnotation = keep;") + if keep < 0 || cleanup < 0 || restore < 0 { + t.Fatal("openCommentPopup should preserve pendingAnnotation across preventive cleanup") + } + if keep >= cleanup || cleanup >= restore { + t.Fatal("openCommentPopup should preserve pendingAnnotation across preventive cleanup") + } + + closeStart := strings.Index(inspectorScript, "function closePopup()") + if closeStart < 0 { + t.Fatal("inspector should define closePopup") + } + closeEnd := strings.Index(inspectorScript[closeStart:], "function currentPagePath()") + if closeEnd < 0 { + t.Fatal("inspector should define currentPagePath after closePopup") + } + if !strings.Contains(inspectorScript[closeStart:closeStart+closeEnd], "pendingAnnotation = null;") { + t.Fatal("direct popup cleanup should still clear pendingAnnotation") + } +} + func TestStripIframeSecurityHeaders_RemovesBlockingHeaders(t *testing.T) { h := http.Header{} h.Set("Content-Security-Policy", "default-src 'none'") diff --git a/apps/backend/internal/agentctl/server/api/scripts/inspector.js b/apps/backend/internal/agentctl/server/api/scripts/inspector.js index 8a812420ab..41fb9d2bef 100644 --- a/apps/backend/internal/agentctl/server/api/scripts/inspector.js +++ b/apps/backend/internal/agentctl/server/api/scripts/inspector.js @@ -137,7 +137,9 @@ } function openCommentPopup(anchorX, anchorY, onSubmit) { + var keep = pendingAnnotation; closePopup(); + pendingAnnotation = keep; popup = document.createElement('div'); popup.style.cssText = 'position:fixed;left:' + anchorX + 'px;top:' + anchorY + 'px;z-index:' + (Z + 1) + ';' + 'background:#fff;border:1px solid #e5e7eb;border-radius:6px;box-shadow:0 4px 12px rgba(0,0,0,0.15);' diff --git a/apps/web/e2e/tests/preview/inspector-submission.spec.ts b/apps/web/e2e/tests/preview/inspector-submission.spec.ts new file mode 100644 index 0000000000..8298cf28ca --- /dev/null +++ b/apps/web/e2e/tests/preview/inspector-submission.spec.ts @@ -0,0 +1,124 @@ +import http from "node:http"; +import type { AddressInfo } from "node:net"; +import { expect } from "@playwright/test"; +import { test } from "../../fixtures/test-base"; +import { openTaskSession, createStandardProfile } from "../../helpers/git-helper"; + +const MOCK_HTML = ` + +Mock Dev Server + + + +`; + +async function startMockDevServer(): Promise<{ port: number; close: () => Promise }> { + const server = http.createServer((_req, res) => { + res.writeHead(200, { "Content-Type": "text/html; charset=utf-8" }); + res.end(MOCK_HTML); + }); + await new Promise((resolve) => server.listen(0, "127.0.0.1", resolve)); + const { port } = server.address() as AddressInfo; + return { + port, + close: () => + new Promise((resolve, reject) => + server.close((err) => (err ? reject(err) : resolve())), + ), + }; +} + +async function openPreview( + testPage: import("@playwright/test").Page, + apiClient: import("../../helpers/api-client").ApiClient, + seedData: { workspaceId: string; workflowId: string; startStepId: string; repositoryId: string }, + title: string, + port: number, +) { + await apiClient.updateRepository(seedData.repositoryId, { dev_script: "echo preview" }); + const profile = await createStandardProfile(apiClient, `inspector-${title.toLowerCase()}`); + await apiClient.createTaskWithAgent(seedData.workspaceId, title, profile.id, { + description: "preview inspector submission", + workflow_id: seedData.workflowId, + workflow_step_id: seedData.startStepId, + repository_ids: [seedData.repositoryId], + }); + + const session = await openTaskSession(testPage, title); + await session.waitForChatIdle({ timeout: 30_000 }); + const openBrowserPreview = testPage.getByRole("button", { name: /open browser preview/i }); + await expect(openBrowserPreview).toBeVisible(); + await openBrowserPreview.click(); + + const urlInput = testPage.locator('[placeholder*="localhost"], [placeholder*="3000"]').first(); + await expect(urlInput).toBeVisible(); + await urlInput.fill(`http://localhost:${port}`); + await urlInput.press("Enter"); + + const iframe = testPage.locator('iframe[title*="preview" i]'); + await expect(iframe).toBeVisible({ timeout: 15_000 }); + const preview = testPage.frameLocator('iframe[title*="preview" i]'); + await expect(preview.locator("#submit-btn")).toBeVisible(); + + await testPage.getByTestId("preview-inspect-button").click(); + await expect(preview.locator("#submit-btn")).toBeVisible(); + return preview; +} + +test.describe("Browser inspect annotation submission", () => { + test("Save submits a pin annotation", async ({ testPage, apiClient, seedData }) => { + const devServer = await startMockDevServer(); + try { + const preview = await openPreview( + testPage, + apiClient, + seedData, + "Inspector Save", + devServer.port, + ); + + await preview.locator("#submit-btn").click(); + const comment = preview.locator("textarea"); + await expect(comment).toBeVisible(); + await comment.fill("make this primary color"); + await preview.getByRole("button", { name: "Save" }).click(); + + await expect(preview.locator("textarea")).toHaveCount(0); + await expect(preview.locator('[data-kandev-marker="1"]')).toBeVisible(); + await expect(testPage.getByTestId("preview-annotation-item")).toContainText( + "make this primary color", + ); + } finally { + await apiClient.updateRepository(seedData.repositoryId, { dev_script: "" }); + await devServer.close(); + } + }); + + test("Enter submits a pin annotation", async ({ testPage, apiClient, seedData }) => { + const devServer = await startMockDevServer(); + try { + const preview = await openPreview( + testPage, + apiClient, + seedData, + "Inspector Enter", + devServer.port, + ); + + await preview.locator("#submit-btn").click(); + const comment = preview.locator("textarea"); + await expect(comment).toBeVisible(); + await comment.fill("increase font size"); + await comment.press("Enter"); + + await expect(preview.locator("textarea")).toHaveCount(0); + await expect(preview.locator('[data-kandev-marker="1"]')).toBeVisible(); + await expect(testPage.getByTestId("preview-annotation-item")).toContainText( + "increase font size", + ); + } finally { + await apiClient.updateRepository(seedData.repositoryId, { dev_script: "" }); + await devServer.close(); + } + }); +}); diff --git a/docs/plans/browser-inspect-annotations-save/plan.md b/docs/plans/browser-inspect-annotations-save/plan.md new file mode 100644 index 0000000000..57c0cb2e87 --- /dev/null +++ b/docs/plans/browser-inspect-annotations-save/plan.md @@ -0,0 +1,101 @@ +--- +spec: docs/specs/browser-inspect-annotations-save/spec.md +created: 2026-08-07 +status: complete +--- + +# Implementation Plan: Browser inspect annotation submission + +## Overview + +Repair the injected inspector's pending-annotation lifecycle with the smallest +possible change: preserve the selection while `openCommentPopup` performs its +defensive popup cleanup. Add a script-level regression test and focused browser +coverage for both Save and plain-Enter submission, while keeping Cancel/Escape +cleanup unchanged. + +The issue's root cause is confirmed in +`apps/backend/internal/agentctl/server/api/scripts/inspector.js`: `startPending` +sets `pendingAnnotation`, then `openCommentPopup` calls `closePopup`, which +unconditionally clears it before either submit handler runs. + +## Backend + +### Injected inspector state + +- `apps/backend/internal/agentctl/server/api/scripts/inspector.js` + - Save the current `pendingAnnotation` before the preventive + `closePopup()` call in `openCommentPopup`. + - Restore it immediately after cleanup and before wiring the popup handlers. + - Leave direct Cancel/Escape calls to `closePopup()` unchanged so dismissal + still discards the pending selection. + +## Frontend + +### Preview submission coverage + +- `apps/web/e2e/tests/preview/inspector-submission.spec.ts` + - Use the existing E2E fixtures and a local mock HTML server. + - Configure the seeded repository's dev script so the Browser panel action is + available, open the Browser panel, switch to Inspect mode, and interact + with the real injected iframe popup. + - Cover clicking Save and pressing plain Enter, asserting the popup closes, + the iframe marker exists, and the parent annotations panel contains the + submitted comment. + - Restore any seeded repository settings in teardown. + +This is a state-only repair in the existing preview surface. It does not alter +responsive composition, touch targets, scrolling, navigation, or viewport +geometry, so a separate mobile Playwright test is not required; the shared +injected state transition is the same on mobile-sized previews. + +## Tests + +- **What:** `openCommentPopup` does not erase the selection created by + `startPending`, while the existing dismissal path still clears it. + **File:** `apps/backend/internal/agentctl/server/api/html_injector_test.go`. + **How:** assert the embedded inspector script preserves the pending value + across the cleanup call and retains the direct Cancel/Escape cleanup paths. +- **What:** the proxy-served inspector script remains syntactically valid and + the existing HTML injection behavior remains intact. + **File:** existing API package tests. + **How:** run the targeted Go package test and `node --check` on the injected + script. + +## E2E Tests + +- **Scenario:** **GIVEN** a pin selection with the comment popup open, **WHEN** + the user types a comment and clicks **Save**, **THEN** the popup closes, a + marker is present in the iframe, and the Browser annotations panel shows the + comment. + **File:** `apps/web/e2e/tests/preview/inspector-submission.spec.ts`. +- **Scenario:** **GIVEN** a pin selection with the comment popup open, + **WHEN** the user types a comment and presses plain **Enter**, **THEN** the + same marker and annotations-panel outcomes occur. + **File:** `apps/web/e2e/tests/preview/inspector-submission.spec.ts`. + +## Verification Results + +Implemented and verified: + +- `cd apps/backend && go test ./internal/agentctl/server/api` — passed. +- `cd apps/backend && node --check internal/agentctl/server/api/scripts/inspector.js` — passed. +- `cd apps/backend && go test -run TestInspectorScript_PreservesPendingAnnotationAcrossPopupCleanup ./internal/agentctl/server/api` — passed after the expected red failure. +- `cd apps/web && pnpm e2e:run --project chromium tests/preview/inspector-submission.spec.ts` — 2 passed (Save and plain Enter). +- `git diff --check` — passed. + +## Implementation Waves And Parallel Candidates + +Wave 1 (sequential): + +- [x] [task-01-inspector-state](task-01-inspector-state.md) — done + +Wave 2 (sequential, depends on Wave 1): + +- [x] [task-02-preview-submit-e2e](task-02-preview-submit-e2e.md) — done + +## Open Questions + +- The existing `preview-annotations.spec.ts` suite remains skipped because its + preview fixture lacks a configured `dev_script`; the focused regression test + sets that prerequisite locally rather than changing unrelated scenarios. diff --git a/docs/plans/browser-inspect-annotations-save/task-01-inspector-state.md b/docs/plans/browser-inspect-annotations-save/task-01-inspector-state.md new file mode 100644 index 0000000000..5da5dc1b7d --- /dev/null +++ b/docs/plans/browser-inspect-annotations-save/task-01-inspector-state.md @@ -0,0 +1,64 @@ +--- +id: "01-inspector-state" +title: "Preserve inspector pending state" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +spec: "../../specs/browser-inspect-annotations-save/spec.md" +--- + +# Task 01: Preserve inspector pending state + +## Acceptance + +- A pin or area selection remains available to `commitAnnotation` after the + popup is opened, so Save/Enter can emit `annotation-added` and close the + popup. +- Cancel and Escape still clear the pending selection without emitting an + annotation. +- The existing API package tests and JavaScript syntax check pass. + +## Verification + +- Add and run the focused regression test: + `cd apps/backend && go test -run TestInspectorScript_PreservesPendingAnnotationAcrossPopupCleanup ./internal/agentctl/server/api` +- Run the full affected package: + `cd apps/backend && go test ./internal/agentctl/server/api` +- Validate the embedded script parses: + `cd apps/backend && node --check internal/agentctl/server/api/scripts/inspector.js` +- Run `git diff --check`. + +## Files likely touched + +- `apps/backend/internal/agentctl/server/api/scripts/inspector.js` +- `apps/backend/internal/agentctl/server/api/html_injector_test.go` + +## Dependencies + +None. + +## Parallelism + +Sequential. The test and the embedded script share the same behavior contract. + +## Inputs + +- `docs/specs/browser-inspect-annotations-save/spec.md`, especially the Save, + Enter, and dismissal scenarios. +- `docs/plans/browser-inspect-annotations-save/plan.md`. +- Existing `closePopup`, `openCommentPopup`, `startPending`, and + `commitAnnotation` implementations in `inspector.js`. + +## Output contract + +Report the changed files, the red/green test results, exact commands, any +remaining E2E dependency, and synchronized task/plan statuses. + +## Results + +- RED: `cd apps/backend && go test -run TestInspectorScript_PreservesPendingAnnotationAcrossPopupCleanup ./internal/agentctl/server/api` failed at the new assertion before the production fix. +- GREEN: the same focused test passed after preserving `pendingAnnotation` across `closePopup()`. +- `cd apps/backend && go test ./internal/agentctl/server/api` — passed. +- `cd apps/backend && node --check internal/agentctl/server/api/scripts/inspector.js` — passed. +- `git diff --check` — passed. diff --git a/docs/plans/browser-inspect-annotations-save/task-02-preview-submit-e2e.md b/docs/plans/browser-inspect-annotations-save/task-02-preview-submit-e2e.md new file mode 100644 index 0000000000..7813bf8ef2 --- /dev/null +++ b/docs/plans/browser-inspect-annotations-save/task-02-preview-submit-e2e.md @@ -0,0 +1,62 @@ +--- +id: "02-preview-submit-e2e" +title: "Cover preview annotation submission" +status: done +wave: 2 +depends_on: ["01-inspector-state"] +plan: "plan.md" +spec: "../../specs/browser-inspect-annotations-save/spec.md" +--- + +# Task 02: Cover preview annotation submission + +## Acceptance + +- A real preview iframe test proves clicking Save creates the marker and + Browser annotations-panel entry. +- A real preview iframe test proves plain Enter follows the same successful + path for a pin selection. +- The test restores seeded repository settings and runs in the desktop + Chromium project without enabling the currently unrelated skipped suite. + +## Verification + +- Bootstrap a fresh worktree if needed: + `cd apps && pnpm install --frozen-lockfile`. +- Run the focused desktop E2E test with a production build: + `cd apps/web && pnpm e2e:run --project chromium tests/preview/inspector-submission.spec.ts`. +- Run `git diff --check`. + +## Files likely touched + +- `apps/web/e2e/tests/preview/inspector-submission.spec.ts` + +## Dependencies + +Task 01 must pass first. + +## Parallelism + +Sequential. This test depends on the repaired injected script and the same +preview fixture state. + +## Inputs + +- `docs/specs/browser-inspect-annotations-save/spec.md` scenarios. +- `docs/plans/browser-inspect-annotations-save/plan.md` E2E section. +- `apps/web/e2e/README.md` preview-fixture guidance. +- `apps/web/components/task/dockview-header-actions.tsx` Browser panel action. +- `apps/web/e2e/tests/preview/preview-annotations.spec.ts` mock server and + preview setup patterns. + +## Output contract + +Report the exact E2E command, test count and result, any browser artifacts, +cleanup performed, and synchronized task/plan statuses. + +## Results + +- `cd apps && pnpm install --frozen-lockfile` — dependencies already up to date. +- `cd apps/web && pnpm e2e:run --project chromium tests/preview/inspector-submission.spec.ts` — 2 passed in Chromium (Save and plain Enter). +- The test uses a local mock HTTP server, restores the seeded repository's + `dev_script`, and closes the server in teardown. diff --git a/docs/specs/INDEX.md b/docs/specs/INDEX.md index b9b3b58de7..c6b7fadebe 100644 --- a/docs/specs/INDEX.md +++ b/docs/specs/INDEX.md @@ -230,6 +230,7 @@ System pages (Radarr/Sonarr-style) for status, disk usage, database maintenance, | [mobile-quick-chat-topbar](mobile-quick-chat-topbar/spec.md) | building | | [native-code-review](native-code-review/spec.md) | building | | [missing-task-route-recovery](missing-task-route-recovery/spec.md) | draft | +| [browser-inspect-annotations-save](browser-inspect-annotations-save/spec.md) | shipped | --- diff --git a/docs/specs/browser-inspect-annotations-save/spec.md b/docs/specs/browser-inspect-annotations-save/spec.md new file mode 100644 index 0000000000..4aaaefc1a8 --- /dev/null +++ b/docs/specs/browser-inspect-annotations-save/spec.md @@ -0,0 +1,56 @@ +--- +status: shipped +created: 2026-08-07 +owner: carlosflorencio +--- + +# Browser inspect annotation submission + +## Why + +Users can select a pin or area in the Browser panel's Inspect mode, but the +comment popup currently loses the selection before the user can submit it. +This makes the primary annotation workflow appear unresponsive and prevents +users from giving the agent visual change requests. + +## What + +- Selecting a pin or area opens the existing comment popup with that selection + pending until the user submits or cancels it. +- Clicking **Save** submits the pending annotation, closes the popup, places its + numbered marker in the preview, and adds it to the Browser panel's + annotations list. +- Pressing **Enter** without Shift has the same submission behavior as + clicking **Save**. Shift+Enter continues to insert a newline in the comment. +- The submitted annotation uses the existing `annotation-added` iframe message + contract; the parent continues to assign the display number. +- Clicking **Cancel** or pressing **Escape** closes the popup and discards the + pending selection without adding a marker or annotation. +- Empty comments remain valid submissions, matching the existing popup + behavior. + +## Scenarios + +- **GIVEN** Inspect mode is active and the user clicks an element, **WHEN** the + user enters a comment and clicks **Save**, **THEN** the popup closes, a + numbered marker appears at the clicked point, and the parent receives one + `annotation-added` message containing the pin and comment. +- **GIVEN** Inspect mode is active and the user clicks an element, **WHEN** the + user enters a comment and presses plain **Enter**, **THEN** the popup closes, + a numbered marker appears at the clicked point, and the parent receives one + `annotation-added` message containing the pin and comment. +- **GIVEN** a pin or area comment popup is open, **WHEN** the user presses + **Shift+Enter**, **THEN** no annotation is submitted and the popup remains + open with the comment newline inserted. +- **GIVEN** a pin or area comment popup is open, **WHEN** the user clicks + **Cancel** or presses **Escape**, **THEN** the popup closes and no marker or + `annotation-added` message is created. + +## Out of scope + +- Changing the parent React hook, annotation formatter, annotation panel, or + iframe message shape. +- Persisting annotations across iframe reloads, page reloads, or Kandev + restarts. +- Changing the Inspect button, preview layout, touch composition, or proxy + injection behavior.