feat(viewer): add collapse-all and hand a finding to a coding agent

internal/viewer/agent_deeplink_test.go

@@ -0,0 +1,192 @@
+// SPDX-License-Identifier: Apache-2.0
+// Copyright 2026 alibaba/open-code-review Contributors
+
+package viewer
+
+import (
+	"strings"
+	"testing"
+)
+
+// A deep link is handed to the desktop only if the browser navigates to it
+// inside a real user gesture. Building the href in the menu's own click handler
+// and re-dispatching the click afterwards loses that gesture, and every current
+// browser then silently refuses the external protocol - the click appears to do
+// nothing, with no error to explain it. So the href must be built while the
+// menu opens, and the link's own click must be left to the browser.
+//
+// Confirmed in headless Chromium against this exact regression: zero
+// navigations requested before the fix, one after.
+func TestAgentScript_DoesNotRedispatchClicks(t *testing.T) {
+	src, err := assets.ReadFile("static/agent.js")
+	if err != nil {
+		t.Fatal(err)
+	}
+	js := string(src)
+
+	if strings.Contains(js, ".click()") {
+		t.Error("agent.js re-dispatches a click; the deep link will lose its user gesture and be dropped")
+	}
+	// The copy control is a <button> and does cancel its own default, which is
+	// correct. The link handler is the one that must not.
+	if strings.Contains(js, `item.addEventListener("click", (e)`) {
+		t.Error("a deep-link handler still takes an event; it should close the menu and let the browser navigate")
+	}
+	// The href has to be assigned from the open path, not the click path.
+	if !strings.Contains(js, "buildLinks") {
+		t.Error("agent.js no longer builds link hrefs; the open path must set them")
+	}
+}
+
+// A document-level listener per finding would mean a session with hundreds of
+// findings registers hundreds of them, all of which run on every click anywhere
+// in the viewer. The dismiss handler must be installed once for the whole page.
+func TestAgentScript_DismissListenerIsNotPerFinding(t *testing.T) {
+	src, err := assets.ReadFile("static/agent.js")
+	if err != nil {
+		t.Fatal(err)
+	}
+	js := string(src)
+
+	if strings.Count(js, `document.addEventListener("click"`) > 1 {
+		t.Errorf("agent.js registers %d document click listeners; it must register exactly one",
+			strings.Count(js, `document.addEventListener("click"`))
+	}
+	if !strings.Contains(js, "bindDismiss") {
+		t.Error("agent.js no longer installs the dismiss listener once per page")
+	}
+}
+
+// A link whose shipped href is "#" would scroll the page if the script ever
+// failed to replace it, and would look like a working link while doing nothing.
+func TestAgentMenu_PlaceholderHrefIsInert(t *testing.T) {
+	body := renderSessionPage(t,
+		`{"type":"session_start","timestamp":"2025-06-01T10:00:00Z","cwd":"/my/proj"}`,
+		`{"type":"review_item_done","filePath":"a.go","comments":[{"content":"c","category":"bug","severity":"high"}]}`,
+	)
+	if strings.Contains(body, `data-agent-open="codex" href="#"`) {
+		t.Error("agent menu ships a '#' placeholder href, which scrolls the page when followed")
+	}
+	// Every deep-link anchor must ship an href attribute at all, or it is not
+	// focusable and cannot be activated by keyboard.
+	if !strings.Contains(body, `data-agent-open="codex" href=`) {
+		t.Error("deep-link anchors ship without an href and are not keyboard reachable")
+	}
+}
+
+// The menu is absolutely positioned inside the card, and the card's group
+// clips its overflow to round its corners, so an open menu was cut off at the
+// group's bottom edge. The clip has to be lifted while a menu is open.
+func TestAgentMenu_IsNotClippedByItsGroup(t *testing.T) {
+	css, err := assets.ReadFile("static/style.css")
+	if err != nil {
+		t.Fatal(err)
+	}
+	style := string(css)
+	if !strings.Contains(style, ".comment-file-group:has(.agent-menu:not([hidden]))") {
+		t.Error("the comment group still clips the agent menu it contains")
+	}
+	if !strings.Contains(style, "overflow: visible") {
+		t.Error("the agent menu's group does not lift its clip while a menu is open")
+	}
+}
+
+// The prompt is truncated to fit the vendors' limits, and the vendors measure
+// the URL-encoded value. Truncating on the raw character count would let a
+// prompt of mostly non-ASCII text encode to several times the limit.
+func TestAgentScript_TruncatesOnTheEncodedLength(t *testing.T) {
+	src, err := assets.ReadFile("static/agent.js")
+	if err != nil {
+		t.Fatal(err)
+	}
+	js := string(src)
+	if !strings.Contains(js, "encodedLength") {
+		t.Error("agent.js does not measure the encoded prompt length")
+	}
+	if strings.Contains(js, "const MAX_PROMPT = 4000") {
+		t.Error("agent.js still truncates the prompt on its raw character count")
+	}
+}
+
+// The finding's text is read from the rendered card instead of being shipped
+// again in a data attribute, which roughly doubled the size of a session page.
+// The toggle's click used to stop propagating, which kept the page-level
+// dismiss handler from seeing it: opening a second menu then left the first
+// one open, so two dropdowns were on screen at once.
+func TestAgentScript_ToggleDoesNotStopPropagation(t *testing.T) {
+	src, err := assets.ReadFile("static/agent.js")
+	if err != nil {
+		t.Fatal(err)
+	}
+	js := string(src)
+	if strings.Contains(js, ".stopPropagation(") {
+		t.Error("agent.js stops the toggle's click from propagating, so the dismiss handler never sees it")
+	}
+}
+
+// The workspace parameter is a file URI, and building it by concatenating
+// "file://" with the path does not produce a valid URI for a Windows path
+// (backslashes, no third slash) nor for a path containing "#" or "?", which
+// would start a fragment or a query.
+func TestAgentScript_BuildsTheWorkspaceURIWithTheURLParser(t *testing.T) {
+	src, err := assets.ReadFile("static/agent.js")
+	if err != nil {
+		t.Fatal(err)
+	}
+	js := string(src)
+	if strings.Contains(js, `"file://" + dir`) {
+		t.Error("agent.js builds the workspace URI by concatenation, which is not a valid URI for every path")
+	}
+	if !strings.Contains(js, "new URL(") {
+		t.Error("agent.js does not build the workspace URI with the URL parser")
+	}
+}
+
+// Two copies of the dismissal can drift apart, and they already had: one
+// closed the menu the page-level handler was looking at, the other the menu
+// whose button was pressed.
+func TestAgentScript_HasOneDismissal(t *testing.T) {
+	src, err := assets.ReadFile("static/agent.js")
+	if err != nil {
+		t.Fatal(err)
+	}
+	js := string(src)
+	if !strings.Contains(js, "closeMenu(root)") {
+		t.Error("agent.js does not route the per-finding dismissal through closeMenu")
+	}
+	if strings.Contains(js, `menu.hidden = true;
+            toggle.setAttribute("aria-expanded", "false");`) {
+		t.Error("agent.js has a second copy of the dismissal logic")
+	}
+}
+
+func TestSessionPage_DoesNotDuplicateTheFindingText(t *testing.T) {
+	body := renderSessionPage(t,
+		`{"type":"session_start","timestamp":"2025-06-01T10:00:00Z","cwd":"/my/proj"}`,
+		`{"type":"review_item_done","filePath":"a.go","comments":[{"content":"a unique finding body","category":"bug","severity":"high"}]}`,
+	)
+	for _, unwanted := range []string{"data-content=", "data-existing=", "data-suggestion="} {
+		if strings.Contains(body, unwanted) {
+			t.Errorf("the session page ships the finding text twice, in %s", unwanted)
+		}
+	}
+	// The text still has to reach the page, or the handoff has nothing to send.
+	if !strings.Contains(body, "a unique finding body") {
+		t.Error("the finding body is not on the page for the handoff to read")
+	}
+}
+
+// The collapse-all control's label carries its state, so a toggle attribute
+// would announce the opposite state to the one the label describes.
+func TestCollapseAll_LabelCarriesTheState(t *testing.T) {
+	body := renderSessionPage(t,
+		`{"type":"session_start","timestamp":"2025-06-01T10:00:00Z","cwd":"/my/proj"}`,
+		`{"type":"review_item_done","filePath":"a.go","comments":[{"content":"c","category":"bug","severity":"high"}]}`,
+	)
+	if !strings.Contains(body, ">Collapse all</button>") {
+		t.Error("the collapse-all control does not name the action it performs")
+	}
+	if strings.Contains(body, `data-collapse-all aria-pressed`) {
+		t.Error("the collapse-all control has a toggle state that contradicts its label")
+	}
+}

internal/viewer/agent_handoff_test.go

@@ -0,0 +1,173 @@
+// SPDX-License-Identifier: Apache-2.0
+// Copyright 2026 alibaba/open-code-review Contributors
+
+package viewer
+
+import (
+	"net/http"
+	"net/http/httptest"
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+)
+
+// writeSession creates a session file with the given JSONL lines under the
+// repo directory the viewer serves, and returns its path.
+func writeSession(t *testing.T, repoDir, id string, lines ...string) string {
+	t.Helper()
+	if err := os.MkdirAll(repoDir, 0755); err != nil {
+		t.Fatal(err)
+	}
+	path := filepath.Join(repoDir, id+".jsonl")
+	if err := os.WriteFile(path, []byte(strings.Join(lines, "\n")+"\n"), 0600); err != nil {
+		t.Fatal(err)
+	}
+	return path
+}
+
+// renderSessionPage writes a session file and renders its detail page.
+func renderSessionPage(t *testing.T, lines ...string) string {
+	t.Helper()
+	root := t.TempDir()
+	repoDir := filepath.Join(root, "repo")
+	writeSession(t, repoDir, "s1", lines...)
+
+	req := httptest.NewRequest("GET", "/r/repo/s1", nil)
+	rr := httptest.NewRecorder()
+	handleSession(rr, req, root, "repo", "s1")
+	if rr.Code != http.StatusOK {
+		t.Fatalf("status = %d, want 200", rr.Code)
+	}
+	return rr.Body.String()
+}
+
+// The agent handoff has to carry the finding's full context, or the agent it
+// opens cannot act on the finding without a round trip back to the page.
+func TestSessionPage_AgentActionsCarryContext(t *testing.T) {
+	body := renderSessionPage(t,
+		`{"type":"session_start","timestamp":"2025-06-01T10:00:00Z","cwd":"/my/proj","gitBranch":"feat/x"}`,
+		`{"type":"review_item_done","filePath":"src/app.js","comments":[{"content":"Null deref on empty input.","category":"bug","severity":"high","start_line":10,"end_line":20,"suggestion_code":"const a = x?.y;"}]}`,
+	)
+
+	for _, want := range []string{
+		`data-agent-actions`,
+		`data-file="src/app.js"`,
+		`data-start="10"`,
+		`data-end="20"`,
+		`data-category="bug"`,
+		`data-severity="high"`,
+		`data-branch="feat/x"`,
+		`data-repo="/my/proj"`,
+	} {
+		if !strings.Contains(body, want) {
+			t.Errorf("agent handoff missing %s", want)
+		}
+	}
+
+	// The finding's text is read from the rendered card rather than shipped
+	// again in an attribute, so it has to be on the page exactly once.
+	for _, want := range []string{
+		`Null deref on empty input.`,
+		`const a = x?.y;`,
+	} {
+		if !strings.Contains(body, want) {
+			t.Errorf("session page is missing the finding text %s", want)
+		}
+	}
+	for _, unwanted := range []string{
+		`data-content=`,
+		`data-existing=`,
+		`data-suggestion=`,
+	} {
+		if strings.Contains(body, unwanted) {
+			t.Errorf("session page duplicates the finding text in %s", unwanted)
+		}
+	}
+}
+
+// Targets are ordered app -> editor extension per vendor, because the app is
+// what a user reaches for first and the extension is the last resort.
+func TestSessionPage_AgentMenuOrdersAppBeforeExtension(t *testing.T) {
+	body := renderSessionPage(t,
+		`{"type":"session_start","timestamp":"2025-06-01T10:00:00Z","cwd":"/my/proj"}`,
+		`{"type":"review_item_done","filePath":"a.go","comments":[{"content":"c","category":"bug","severity":"high"}]}`,
+	)
+
+	appAt := strings.Index(body, `data-agent-open="claude-app"`)
+	extAt := strings.Index(body, `data-agent-open="claude-vscode"`)
+	if appAt < 0 || extAt < 0 {
+		t.Fatal("agent menu is missing a Claude Code app or extension target")
+	}
+	if appAt > extAt {
+		t.Error("app target should precede the editor extension target")
+	}
+
+	// The CLI has no prefilled-prompt scheme, so the copy action is the path
+	// and must be present rather than implied.
+	if !strings.Contains(body, `data-agent-copy`) {
+		t.Error("agent menu has no copy-prompt action")
+	}
+	if !strings.Contains(body, `data-agent-open="codex"`) || !strings.Contains(body, `data-agent-open="cursor"`) {
+		t.Error("agent menu is missing the Codex or Cursor target")
+	}
+}
+
+func TestSessionPage_CollapseAllControlPresent(t *testing.T) {
+	body := renderSessionPage(t,
+		`{"type":"session_start","timestamp":"2025-06-01T10:00:00Z","cwd":"/my/proj"}`,
+		`{"type":"review_item_done","filePath":"a.go","comments":[{"content":"c","category":"bug","severity":"high"}]}`,
+	)
+	if !strings.Contains(body, `data-collapse-all`) {
+		t.Error("session page has no collapse-all control")
+	}
+	// The label carries the state, so the control must not also carry a toggle
+	// attribute that would announce the opposite state.
+	if strings.Contains(body, `data-collapse-all aria-pressed`) {
+		t.Error("collapse-all control has an aria-pressed state that contradicts its label")
+	}
+}
+
+// The control belongs to the comments section, so a session with no findings
+// must not render a button that would do nothing.
+func TestSessionPage_NoCollapseAllWithoutComments(t *testing.T) {
+	body := renderSessionPage(t,
+		`{"type":"session_start","timestamp":"2025-06-01T10:00:00Z","cwd":"/my/proj"}`,
+	)
+	if strings.Contains(body, `data-collapse-all`) {
+		t.Error("collapse-all rendered on a session with no findings")
+	}
+}
+
+// An exported page has to keep the handoff working offline, which is the whole
+// point of the export: no server, but the same actions.
+func TestExportSession_InlinesAgentScript(t *testing.T) {
+	root := t.TempDir()
+	repoDir := filepath.Join(root, "repo")
+	writeSession(t, repoDir, "exp1",
+		`{"type":"session_start","timestamp":"2025-06-01T10:00:00Z","cwd":"/my/proj"}`,
+		`{"type":"review_item_done","filePath":"a.go","comments":[{"content":"c","category":"bug","severity":"high"}]}`,
+	)
+
+	var buf strings.Builder
+	if err := ExportSession(&buf, root, "repo", "exp1"); err != nil {
+		t.Fatal(err)
+	}
+	body := buf.String()
+
+	for _, want := range []string{
+		"claude-cli://open",
+		"codex://new",
+		"cursor://anysphere.cursor-deeplink/prompt",
+		"vscode://anthropic.claude-code/open",
+		`data-agent-actions`,
+	} {
+		if !strings.Contains(body, want) {
+			t.Errorf("exported page missing %s", want)
+		}
+	}
+	// The exported copy must not try to fetch /static/ assets it no longer has.
+	if strings.Contains(body, `src="/static/agent.js"`) {
+		t.Error("exported page still links agent.js instead of inlining it")
+	}
+}

internal/viewer/export.go

@@ -53,7 +53,16 @@ func ExportSession(out io.Writer, root, encodedRepo, sessionID string) error {
 	if err != nil {
 		return fmt.Errorf("read embedded pager script: %w", err)
 	}
+	// The per-finding agent handoff is part of the page, so an exported copy has
+	// to carry it too.
+	agentJS, err := assets.ReadFile("static/agent.js")
+	if err != nil {
+		return fmt.Errorf("read embedded agent script: %w", err)
+	}
+	// The order has to match the served page's template, so the exported page
+	// and the served one run the same scripts in the same sequence.
 	js := append(append(append([]byte(nil), a11y...), '\n'), pagerJS...)
+	js = append(append(js, '\n'), agentJS...)
 	js = append(append(js, '\n'), sessionJS...)
 
 	tmpl, err := parseTemplate("session.html")