Carlos Florêncio
@@ -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'")
|
||||
|
||||
@@ -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);'
|
||||
|
||||
@@ -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 = `<!DOCTYPE html>
|
||||
<html>
|
||||
<head><title>Mock Dev Server</title></head>
|
||||
<body>
|
||||
<button id="submit-btn" role="button" aria-label="Submit form">Submit</button>
|
||||
</body>
|
||||
</html>`;
|
||||
|
||||
async function startMockDevServer(): Promise<{ port: number; close: () => Promise<void> }> {
|
||||
const server = http.createServer((_req, res) => {
|
||||
res.writeHead(200, { "Content-Type": "text/html; charset=utf-8" });
|
||||
res.end(MOCK_HTML);
|
||||
});
|
||||
await new Promise<void>((resolve) => server.listen(0, "127.0.0.1", resolve));
|
||||
const { port } = server.address() as AddressInfo;
|
||||
return {
|
||||
port,
|
||||
close: () =>
|
||||
new Promise<void>((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();
|
||||
}
|
||||
});
|
||||
});
|
||||
@@ -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.
|
||||
@@ -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.
|
||||
@@ -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.
|
||||
@@ -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 |
|
||||
|
||||
---
|
||||
|
||||
|
||||
@@ -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.
|
||||
Reference in New Issue
Block a user