fix: resolve snapshot -C and screenshot --annotate hang over WSS (#842)

ctate · web-flow · commit 285eab46df63 · 2026-03-15T20:41:30.000-05:00
* fix: resolve snapshot -C and screenshot --annotate hang over WSS (#841) Root cause: sequential CDP round-trips per element in find_cursor_interactive_elements() and collect_annotations() caused timeouts over high-latency WSS connections (~200ms × 200+ elements exceeds the 30s CDP timeout). Fix: - snapshot -C: Replace per-element CDP calls with a single JS eval that detects cursor:pointer/onclick/tabindex elements in-browser, then batch-resolve via DOM.querySelectorAll + concurrent DOM.describeNode calls using join_all - screenshot --annotate: Replace sequential DOM.resolveNode + getRect calls with concurrent join_all, matching v0.19.0's Promise.all() pattern Behavioral parity with v0.19.0 (Node.js/Playwright): - cursor:pointer detection via getComputedStyle - Inherited cursor:pointer dedup (skip children of pointer parents) - interactiveTags and interactive ARIA roles exclusion - Role differentiation: clickable vs focusable - Text dedup against ARIA tree ref names and quoted strings - Edge case: -i -C shows cursor elements even when ARIA tree is empty Tests: - 5 unit tests for build_dedup_set() helper - 3 e2e regression tests: cursor-interactive detection, annotation scaling to 50 elements, cursor scaling to 100 elements * fix: add hidden/aria-hidden filtering, contentEditable support, and cleanup robustness - Restore hidden/aria-hidden element filtering in cursor-interactive JS (was present in old code, dropped during rewrite) - Add contentEditable detection with 'editable' role and hint - Replace fire-and-forget cleanup with warning on failure - Simplify build_dedup_set to use ref_map only (eliminates fragile tree-text quote parsing; ref_map already has all ref-bearing names) --------- Co-authored-by: ctate <366502+ctate@users.noreply.github.com>
diff --git a/cli/src/native/e2e_tests.rs b/cli/src/native/e2e_tests.rs
@@ -1982,3 +1982,232 @@ async fn e2e_material_checkbox_check_uncheck() {
     let resp = execute_command(&json!({ "id": "99", "action": "close" }), &mut state).await;
     assert_success(&resp);
 }
+
+// ---------------------------------------------------------------------------
+// Issue #841 – snapshot -C and screenshot --annotate must not hang over WSS
+// ---------------------------------------------------------------------------
+
+/// Verifies that `snapshot -C` (cursor-interactive mode) detects elements with
+/// cursor:pointer / onclick / tabindex, produces the correct v0.19.0-compatible
+/// output format, deduplicates against the ARIA tree, and completes in bounded
+/// time (no sequential CDP round-trip explosion).
+#[tokio::test]
+#[ignore]
+async fn e2e_snapshot_cursor_interactive() {
+    let mut state = DaemonState::new();
+
+    let resp = execute_command(
+        &json!({ "id": "1", "action": "launch", "headless": true }),
+        &mut state,
+    )
+    .await;
+    assert_success(&resp);
+
+    // Page with:
+    //  - <button> and <a> (standard interactive – ARIA tree, NOT in cursor section)
+    //  - <div cursor:pointer onclick> (clickable – cursor section)
+    //  - <div tabindex=0> (focusable – cursor section)
+    //  - <span cursor:pointer> (clickable – cursor section)
+    //  - <span cursor:pointer> child of <div cursor:pointer> (inherited – skip)
+    let html = concat!(
+        "<html><body>",
+        "<a href='#'>Link</a>",
+        "<button>Btn</button>",
+        "<div style='cursor:pointer' onclick='x()'>ClickDiv</div>",
+        "<div tabindex='0'>FocusDiv</div>",
+        "<span style='cursor:pointer'>PointerSpan</span>",
+        "<div style='cursor:pointer'><span>InheritChild</span></div>",
+        "</body></html>",
+    );
+
+    let resp = execute_command(
+        &json!({ "id": "2", "action": "setcontent", "html": html }),
+        &mut state,
+    )
+    .await;
+    assert_success(&resp);
+
+    // snapshot -i -C: interactive tree + cursor section
+    let start = std::time::Instant::now();
+    let resp = execute_command(
+        &json!({ "id": "3", "action": "snapshot", "interactive": true, "cursor": true }),
+        &mut state,
+    )
+    .await;
+    let elapsed = start.elapsed();
+    assert_success(&resp);
+
+    let snapshot = get_data(&resp)["snapshot"].as_str().unwrap();
+
+    // Cursor section must appear
+    assert!(
+        snapshot.contains("# Cursor-interactive elements:") || snapshot.contains("clickable"),
+        "Cursor section missing from snapshot -i -C:\n{}",
+        snapshot,
+    );
+
+    // v0.19.0 output format: role + hints
+    assert!(
+        snapshot.contains("clickable") && snapshot.contains("[cursor:pointer"),
+        "Expected v0.19.0-format cursor output with hints:\n{}",
+        snapshot,
+    );
+
+    // Role differentiation: tabindex-only → focusable
+    assert!(
+        snapshot.contains("focusable") && snapshot.contains("[tabindex]"),
+        "Expected focusable role for tabindex-only element:\n{}",
+        snapshot,
+    );
+
+    // Text dedup: "Link" and "Btn" are in the ARIA tree, so must NOT appear
+    // in the cursor section.
+    let cursor_section = snapshot
+        .split("# Cursor-interactive elements:")
+        .nth(1)
+        .unwrap_or(snapshot);
+    assert!(
+        !cursor_section.contains("\"Link\""),
+        "ARIA link text should be deduped from cursor section"
+    );
+    assert!(
+        !cursor_section.contains("\"Btn\""),
+        "ARIA button text should be deduped from cursor section"
+    );
+
+    // Must complete quickly (< 5s), not hit the 30s CDP timeout
+    assert!(
+        elapsed.as_secs() < 5,
+        "snapshot -C took {:?}, expected < 5s (Issue #841 regression)",
+        elapsed,
+    );
+
+    let resp = execute_command(&json!({ "id": "99", "action": "close" }), &mut state).await;
+    assert_success(&resp);
+}
+
+/// Verifies that `screenshot --annotate` completes in bounded time even with
+/// many interactive elements. Guards against the sequential CDP round-trip
+/// regression that caused hangs over high-latency WSS (Issue #841).
+#[tokio::test]
+#[ignore]
+async fn e2e_screenshot_annotate_many_elements() {
+    let mut state = DaemonState::new();
+
+    let resp = execute_command(
+        &json!({ "id": "1", "action": "launch", "headless": true }),
+        &mut state,
+    )
+    .await;
+    assert_success(&resp);
+
+    // 50 buttons: old sequential code would do 50×2×200ms ≈ 20s over WSS.
+    let mut html = String::from("<html><body>");
+    for i in 1..=50 {
+        html.push_str(&format!("<button>Button {}</button>", i));
+    }
+    html.push_str("</body></html>");
+
+    let resp = execute_command(
+        &json!({ "id": "2", "action": "setcontent", "html": html }),
+        &mut state,
+    )
+    .await;
+    assert_success(&resp);
+
+    let start = std::time::Instant::now();
+    let resp = execute_command(
+        &json!({ "id": "3", "action": "screenshot", "annotate": true }),
+        &mut state,
+    )
+    .await;
+    let elapsed = start.elapsed();
+    assert_success(&resp);
+
+    let annotations = get_data(&resp)["annotations"]
+        .as_array()
+        .expect("Annotated screenshot should return annotations");
+
+    assert!(
+        annotations.len() >= 50,
+        "Expected at least 50 annotations, got {}",
+        annotations.len(),
+    );
+
+    // Must complete quickly (< 10s), not hit the 30s CDP timeout
+    assert!(
+        elapsed.as_secs() < 10,
+        "screenshot --annotate with 50 elements took {:?}, expected < 10s (Issue #841)",
+        elapsed,
+    );
+
+    let resp = execute_command(&json!({ "id": "99", "action": "close" }), &mut state).await;
+    assert_success(&resp);
+}
+
+/// Verifies `snapshot -C` with many cursor-interactive elements completes in
+/// bounded time. Direct regression test for Issue #841's root cause: N×2
+/// sequential CDP round-trips per cursor-interactive element.
+#[tokio::test]
+#[ignore]
+async fn e2e_snapshot_cursor_many_elements() {
+    let mut state = DaemonState::new();
+
+    let resp = execute_command(
+        &json!({ "id": "1", "action": "launch", "headless": true }),
+        &mut state,
+    )
+    .await;
+    assert_success(&resp);
+
+    // 100 cursor-interactive divs: old code = 200 sequential CDP calls,
+    // at 200ms WSS latency = 40s timeout. New code must finish in seconds.
+    let mut html = String::from("<html><body>");
+    for i in 1..=100 {
+        html.push_str(&format!(
+            "<div style='cursor:pointer' onclick='x()'>Item {}</div>",
+            i,
+        ));
+    }
+    html.push_str("</body></html>");
+
+    let resp = execute_command(
+        &json!({ "id": "2", "action": "setcontent", "html": html }),
+        &mut state,
+    )
+    .await;
+    assert_success(&resp);
+
+    let start = std::time::Instant::now();
+    let resp = execute_command(
+        &json!({ "id": "3", "action": "snapshot", "interactive": true, "cursor": true }),
+        &mut state,
+    )
+    .await;
+    let elapsed = start.elapsed();
+    assert_success(&resp);
+
+    let snapshot = get_data(&resp)["snapshot"].as_str().unwrap();
+
+    // All 100 items should appear
+    assert!(
+        snapshot.contains("Item 1") && snapshot.contains("Item 100"),
+        "Expected all 100 cursor-interactive items in output",
+    );
+
+    // All should have v0.19.0-format hints
+    assert!(
+        snapshot.contains("[cursor:pointer, onclick]"),
+        "Expected v0.19.0-format hints",
+    );
+
+    // Must complete quickly
+    assert!(
+        elapsed.as_secs() < 10,
+        "snapshot -C with 100 cursor elements took {:?}, expected < 10s (Issue #841)",
+        elapsed,
+    );
+
+    let resp = execute_command(&json!({ "id": "99", "action": "close" }), &mut state).await;
+    assert_success(&resp);
+}
diff --git a/cli/src/native/screenshot.rs b/cli/src/native/screenshot.rs
@@ -223,35 +223,87 @@ async fn collect_annotations(
     session_id: &str,
     ref_map: &RefMap,
 ) -> Result<Vec<RawAnnotation>, String> {
-    let mut annotations = Vec::new();
+    let entries = ref_map.entries_sorted();
+    if entries.is_empty() {
+        return Ok(Vec::new());
+    }
 
-    for (ref_id, entry) in ref_map.entries_sorted() {
-        let object_id =
-            match super::element::resolve_element_object_id(client, session_id, ref_map, &ref_id)
-                .await
-            {
-                Ok(id) => id,
-                Err(_) => continue,
-            };
+    // Collect entries that have backend_node_ids for batch resolution.
+    let with_backend_ids: Vec<(String, super::element::RefEntry, i64)> = entries
+        .iter()
+        .filter_map(|(ref_id, entry)| {
+            entry
+                .backend_node_id
+                .map(|bid| (ref_id.clone(), entry.clone(), bid))
+        })
+        .collect();
 
-        let Some(rect) = get_rect_for_object(client, session_id, &object_id).await? else {
-            continue;
-        };
+    if with_backend_ids.is_empty() {
+        return Ok(Vec::new());
+    }
 
-        if rect.width <= 0.0 || rect.height <= 0.0 {
-            continue;
+    // Batch-resolve all backend_node_ids to object IDs using concurrent CDP calls.
+    let resolve_futures: Vec<_> = with_backend_ids
+        .iter()
+        .map(|(_, _, backend_node_id)| {
+            client.send_command(
+                "DOM.resolveNode",
+                Some(serde_json::json!({
+                    "backendNodeId": backend_node_id,
+                    "objectGroup": "agent-browser-annotate"
+                })),
+                Some(session_id),
+            )
+        })
+        .collect();
+
+    let resolve_results = futures_util::future::join_all(resolve_futures).await;
+
+    // Collect resolved object IDs paired with their ref info.
+    let mut resolved: Vec<(String, super::element::RefEntry, String)> = Vec::new();
+    for (i, result) in resolve_results.into_iter().enumerate() {
+        if let Ok(val) = result {
+            if let Some(oid) = val
+                .get("object")
+                .and_then(|o| o.get("objectId"))
+                .and_then(|v| v.as_str())
+            {
+                let (ref_id, entry, _) = &with_backend_ids[i];
+                resolved.push((ref_id.clone(), entry.clone(), oid.to_string()));
+            }
         }
+    }
+
+    if resolved.is_empty() {
+        return Ok(Vec::new());
+    }
+
+    // Batch-get bounding rects for all resolved elements using concurrent CDP calls.
+    let rect_futures: Vec<_> = resolved
+        .iter()
+        .map(|(_, _, object_id)| get_rect_for_object(client, session_id, object_id))
+        .collect();
+
+    let rect_results = futures_util::future::join_all(rect_futures).await;
+
+    let mut annotations = Vec::new();
+    for (i, rect_result) in rect_results.into_iter().enumerate() {
+        let rect = match rect_result {
+            Ok(Some(r)) if r.width > 0.0 && r.height > 0.0 => r,
+            _ => continue,
+        };
 
+        let (ref_id, entry, _) = &resolved[i];
         let number = ref_id
             .strip_prefix('e')
             .and_then(|n| n.parse::<u64>().ok())
             .unwrap_or(0);
 
         annotations.push(RawAnnotation {
-            ref_id,
+            ref_id: ref_id.clone(),
             number,
-            role: entry.role,
-            name: (!entry.name.is_empty()).then_some(entry.name),
+            role: entry.role.clone(),
+            name: (!entry.name.is_empty()).then_some(entry.name.clone()),
             rect,
         });
     }
diff --git a/cli/src/native/snapshot.rs b/cli/src/native/snapshot.rs
