diff --git a/e2e/run.mjs b/e2e/run.mjs index 966aaeb1..77d621f8 100644 --- a/e2e/run.mjs +++ b/e2e/run.mjs @@ -410,6 +410,7 @@ function startFixtureServer(geoIpFixture, geoIpAsnFixture) {

Donut E2E Browser Fixture

${url.pathname}

+ Open the linked page `); diff --git a/e2e/tests/browser.test.mjs b/e2e/tests/browser.test.mjs index b9ca2532..0c1bae16 100644 --- a/e2e/tests/browser.test.mjs +++ b/e2e/tests/browser.test.mjs @@ -691,6 +691,27 @@ test("real Wayfern fingerprinting, terms, API automation, CDP, cookies, and proc assert.ok(echo.userAgent.length > 20); assert.match(await cdp.evaluate("document.cookie"), /donut_e2e=browser-ok/); + // A click that only changes the page ends a moment after the click, not + // at the load timeout; a link click still waits for its load and says so. + const agentClick = async (locator) => { + const started = Date.now(); + const answer = await request( + `${base}/v1/profiles/${profile.id}/agent/click`, + { method: "POST", token: saved.api_token, body: { locator } }, + ); + assert.equal(answer.response.status, 200, JSON.stringify(answer.value)); + return { ...answer.value, ms: Date.now() - started }; + }; + const inPlace = await agentClick({ role: "button", name: "Clicked" }); + assert.equal(inPlace.navigated, false); + assert.ok(inPlace.ms < 5000, `an in-place click took ${inPlace.ms} ms`); + const link = await agentClick({ role: "link", name: "Open the linked page" }); + assert.equal(link.navigated, true); + assert.equal( + await cdp.evaluate("document.querySelector('#path').textContent"), + "/linked", + ); + const runningProfile = (await app.invoke("list_browser_profiles")).find( (item) => item.id === profile.id, ); diff --git a/src-tauri/src/cdp_target.rs b/src-tauri/src/cdp_target.rs index e4ca43c2..77ac61c8 100644 --- a/src-tauri/src/cdp_target.rs +++ b/src-tauri/src/cdp_target.rs @@ -541,6 +541,7 @@ impl CdpConnection { const RUN_PAGE_ENABLE_ID: u64 = 1; const RUN_COMMAND_ID: u64 = 2; const RUN_PAGE_DISABLE_ID: u64 = 3; +const RUN_FRAME_TREE_ID: u64 = 4; /// The attach handshake and the commands after it share one socket, so a /// handshake reply carrying a command's id would be handed back as that @@ -554,6 +555,8 @@ const _: () = { assert!(HANDSHAKE_ATTACH_ID != RUN_PAGE_ENABLE_ID); assert!(HANDSHAKE_ATTACH_ID != RUN_COMMAND_ID); assert!(HANDSHAKE_ATTACH_ID != RUN_PAGE_DISABLE_ID); + assert!(HANDSHAKE_GET_TARGETS_ID != RUN_FRAME_TREE_ID); + assert!(HANDSHAKE_ATTACH_ID != RUN_FRAME_TREE_ID); }; /// Run one command on a fresh connection and hand back its result. @@ -570,16 +573,19 @@ pub async fn run_command( /// Run one command, then wait for the page to finish loading. /// -/// Used for anything that might navigate: `Page.navigate` obviously, but also a -/// click or a script that turns out to follow a link. When nothing navigates, -/// the wait simply expires and the command's own result is returned — that is -/// the intended path, not a failure. +/// Used for anything that might navigate: `Page.navigate` obviously +/// (`expect_navigation`), but also a click or a script that turns out to +/// follow a link. When nothing starts to navigate, the wait ends a moment after +/// the reply (`navigation_watch`) and the command's own result is returned: +/// that is the intended path, not a failure. pub async fn run_command_awaiting_load( target: &CdpTarget, method: &str, params: Value, timeout_secs: u64, + expect_navigation: bool, ) -> Result { + use crate::navigation_watch::{main_frame_of, NavigationWatch}; let mut connection = target.connect().await?; // Page events have to be on before the command runs, or `loadEventFired` @@ -587,20 +593,31 @@ pub async fn run_command_awaiting_load( connection .call(RUN_PAGE_ENABLE_ID, "Page.enable", serde_json::json!({})) .await?; + let main_frame = connection + .call( + RUN_FRAME_TREE_ID, + "Page.getFrameTree", + serde_json::json!({}), + ) + .await + .ok() + .and_then(|tree| main_frame_of(&tree)); connection .send_command(RUN_COMMAND_ID, method, params) .await?; let mut command_result = None; let mut failure = None; + let mut watch = NavigationWatch::new(main_frame, expect_navigation); let deadline = tokio::time::Instant::now() + Duration::from_secs(timeout_secs); loop { - let remaining = deadline.saturating_duration_since(tokio::time::Instant::now()); - if remaining.is_zero() { + let now = tokio::time::Instant::now(); + if watch.done(now) || now >= deadline { break; } - let text = match tokio::time::timeout(remaining, connection.next_text()).await { + let text = match tokio::time::timeout_at(watch.wake_at(deadline), connection.next_text()).await + { Ok(Some(Ok(text))) => text, Ok(Some(Err(e))) => { // A dropped connection cannot unsay a command the browser already @@ -612,9 +629,11 @@ pub async fn run_command_awaiting_load( } break; } - // The peer hung up, or the wait expired. Either way whatever the command - // already answered is the best result available. - Ok(None) | Err(_) => break, + // The peer hung up. Whatever the command already answered is the best + // result available. + Ok(None) => break, + // Woken for the end of the grace or the deadline; the loop decides. + Err(_) => continue, }; let response: Value = serde_json::from_str(&text).unwrap_or_default(); @@ -630,12 +649,13 @@ pub async fn run_command_awaiting_load( .cloned() .unwrap_or_else(|| serde_json::json!({})), ); + watch.on_reply(tokio::time::Instant::now()); } // Flattened remote sessions keep `method` at the top level, so this match // is identical on both arms. - if response.get("method") == Some(&Value::from("Page.loadEventFired")) { - break; + if let Some(event) = response.get("method").and_then(Value::as_str) { + watch.on_event(event, response.get("params")); } } @@ -663,6 +683,7 @@ pub async fn navigate(target: &CdpTarget, url: &str, timeout_secs: u64) -> Resul "Page.navigate", serde_json::json!({ "url": url }), timeout_secs, + true, ) .await .map(|_| ()) diff --git a/src-tauri/src/lib.rs b/src-tauri/src/lib.rs index c568cf78..7d1da967 100644 --- a/src-tauri/src/lib.rs +++ b/src-tauri/src/lib.rs @@ -153,6 +153,7 @@ mod mcp_remote; mod mcp_server; mod mcp_toolsets; mod mcp_views; +mod navigation_watch; mod tag_manager; mod team_lock; mod totp; diff --git a/src-tauri/src/mcp_server.rs b/src-tauri/src/mcp_server.rs index 41cba55c..47264820 100644 --- a/src-tauri/src/mcp_server.rs +++ b/src-tauri/src/mcp_server.rs @@ -1801,12 +1801,7 @@ pub(crate) async fn agent_click_locator( "clickCount": request.click_count.unwrap_or(1), }); let (_, navigated) = session - .call_then_await_event( - "Input.dispatchMouseEvent", - release, - "Page.loadEventFired", - CLICK_LOAD_TIMEOUT, - ) + .call_then_await_navigation("Input.dispatchMouseEvent", release, CLICK_LOAD_TIMEOUT) .await?; Ok::<_, AgentError>(navigated) } @@ -8047,16 +8042,25 @@ impl McpServer { dispatch_keystrokes(target, events).await.map_err(cdp_error) } + /// `expect_navigation`: the command is a navigation, so the page's load is + /// always awaited; otherwise only a navigation that starts is. async fn send_cdp_and_wait_for_load( &self, target: &CdpTarget, method: &str, params: serde_json::Value, timeout_secs: u64, + expect_navigation: bool, ) -> Result { - crate::cdp_target::run_command_awaiting_load(target, method, params, timeout_secs) - .await - .map_err(cdp_error) + crate::cdp_target::run_command_awaiting_load( + target, + method, + params, + timeout_secs, + expect_navigation, + ) + .await + .map_err(cdp_error) } /// The profile a browser-interaction tool refers to. @@ -8120,6 +8124,7 @@ impl McpServer { "Page.navigate", serde_json::json!({ "url": url }), 30, + true, ) .await?; @@ -8235,7 +8240,7 @@ impl McpServer { let result = if wait_for_load { self - .send_cdp_and_wait_for_load(&target, "Runtime.evaluate", cdp_params, 30) + .send_cdp_and_wait_for_load(&target, "Runtime.evaluate", cdp_params, 30, false) .await? } else { self @@ -8317,8 +8322,8 @@ impl McpServer { ); // Use send_cdp_and_wait_for_load: if the click triggers navigation, - // we wait for the new page to load. If not, the 10s timeout expires - // and we return immediately. + // we wait for the new page to load; if no navigation starts, the wait + // ends a moment after the click. let result = self .send_cdp_and_wait_for_load( &ctx.target, @@ -8328,6 +8333,7 @@ impl McpServer { "returnByValue": true, }), 10, + false, ) .await?; @@ -8812,6 +8818,7 @@ impl McpServer { "returnByValue": true, }), 10, + false, ) .await?; diff --git a/src-tauri/src/navigation_watch.rs b/src-tauri/src/navigation_watch.rs new file mode 100644 index 00000000..7d9f13a6 --- /dev/null +++ b/src-tauri/src/navigation_watch.rs @@ -0,0 +1,188 @@ +//! When the wait after an action that may navigate can stop. +//! +//! The action's reply alone does not end the wait: a link click is answered +//! before the page it opens has loaded. But most clicks open a menu or change +//! a field and never load anything, and waiting the full load timeout after +//! each of them cost an agent seconds per click. So the wait ends: +//! +//! - when the main frame fires its load event, or navigates within the +//! document (a single-page app changing its route); or +//! - a short grace after the reply, when the main frame has not started to +//! navigate by then. +//! +//! A frame other than the main one (an ad, a widget) loading does not count: +//! its load is not the page the agent acted on. + +use serde_json::Value; +use std::time::Duration; +use tokio::time::Instant; + +/// How long after the reply a navigation may still start. A click handler +/// that navigates usually does so at once; some wait a moment (an animation, +/// a request) first. +pub const NAVIGATION_GRACE: Duration = Duration::from_millis(500); + +/// The CDP events that say the main frame is on its way to a new document. +const STARTED: [&str; 3] = [ + "Page.frameRequestedNavigation", + "Page.frameStartedLoading", + "Page.frameScheduledNavigation", +]; + +#[derive(Debug)] +pub struct NavigationWatch { + /// The page's top frame. `None` when it could not be read: then any frame + /// starting to load counts, which only ever waits longer. + main_frame: Option, + /// The action is a navigation (`Page.navigate`): always wait for the load. + expected: bool, + grace: Duration, + replied_at: Option, + started: bool, + loaded: bool, + same_document: bool, +} + +impl NavigationWatch { + pub fn new(main_frame: Option, expected: bool) -> Self { + Self { + main_frame, + expected, + grace: NAVIGATION_GRACE, + replied_at: None, + started: false, + loaded: false, + same_document: false, + } + } + + pub fn on_reply(&mut self, now: Instant) { + self.replied_at.get_or_insert(now); + } + + pub fn on_event(&mut self, method: &str, params: Option<&Value>) { + let in_main_frame = || match (&self.main_frame, params.and_then(|p| p.get("frameId"))) { + (Some(main), Some(frame)) => frame.as_str() == Some(main.as_str()), + _ => true, + }; + if method == "Page.loadEventFired" { + self.loaded = true; + } else if method == "Page.navigatedWithinDocument" && in_main_frame() { + self.same_document = true; + } else if STARTED.contains(&method) && in_main_frame() { + self.started = true; + } + } + + /// Whether the action took the page somewhere: a load, or a route change. + pub fn navigated(&self) -> bool { + self.loaded || self.same_document + } + + pub fn done(&self, now: Instant) -> bool { + let Some(replied_at) = self.replied_at else { + return false; + }; + if self.navigated() { + return true; + } + !self.expected && !self.started && now >= replied_at + self.grace + } + + /// When to look again: the deadline, or sooner when the grace ends first. + pub fn wake_at(&self, deadline: Instant) -> Instant { + match self.replied_at { + Some(replied_at) if !self.expected && !self.started => deadline.min(replied_at + self.grace), + _ => deadline, + } + } +} + +/// The top frame's id, from a `Page.getFrameTree` reply. +pub fn main_frame_of(tree: &Value) -> Option { + tree + .pointer("/frameTree/frame/id") + .and_then(Value::as_str) + .map(str::to_string) +} + +#[cfg(test)] +mod tests { + use super::*; + use serde_json::json; + + fn watch() -> NavigationWatch { + NavigationWatch::new(Some("main".to_string()), false) + } + + #[test] + fn a_click_that_loads_nothing_ends_after_the_grace() { + let start = Instant::now(); + let mut watch = watch(); + assert!(!watch.done(start), "never before the reply"); + watch.on_reply(start); + assert!(!watch.done(start + Duration::from_millis(100))); + assert_eq!( + watch.wake_at(start + Duration::from_secs(10)), + start + NAVIGATION_GRACE + ); + assert!(watch.done(start + NAVIGATION_GRACE)); + assert!(!watch.navigated()); + } + + #[test] + fn a_started_navigation_waits_for_its_load() { + let start = Instant::now(); + let mut watch = watch(); + watch.on_reply(start); + watch.on_event( + "Page.frameStartedLoading", + Some(&json!({ "frameId": "main" })), + ); + assert!(!watch.done(start + Duration::from_secs(5))); + assert_eq!( + watch.wake_at(start + Duration::from_secs(10)), + start + Duration::from_secs(10) + ); + watch.on_event("Page.loadEventFired", Some(&json!({ "timestamp": 1.0 }))); + assert!(watch.done(start + Duration::from_secs(5))); + assert!(watch.navigated()); + } + + #[test] + fn another_frame_loading_does_not_hold_the_click() { + let start = Instant::now(); + let mut watch = watch(); + watch.on_reply(start); + watch.on_event( + "Page.frameStartedLoading", + Some(&json!({ "frameId": "ad" })), + ); + assert!(watch.done(start + NAVIGATION_GRACE)); + } + + #[test] + fn a_route_change_is_a_navigation_with_no_load() { + let start = Instant::now(); + let mut watch = watch(); + watch.on_reply(start); + watch.on_event( + "Page.navigatedWithinDocument", + Some(&json!({ "frameId": "main" })), + ); + assert!(watch.done(start)); + assert!(watch.navigated()); + } + + #[test] + fn an_expected_navigation_always_waits_for_the_load() { + let start = Instant::now(); + let mut watch = NavigationWatch::new(None, true); + watch.on_reply(start); + assert!(!watch.done(start + Duration::from_secs(9))); + assert_eq!( + main_frame_of(&json!({ "frameTree": { "frame": { "id": "F1" } } })), + Some("F1".to_string()) + ); + } +} diff --git a/src-tauri/src/wayfern_cdp.rs b/src-tauri/src/wayfern_cdp.rs index ba7dfe61..aa2880c2 100644 --- a/src-tauri/src/wayfern_cdp.rs +++ b/src-tauri/src/wayfern_cdp.rs @@ -628,60 +628,59 @@ impl WayfernSession { Ok(self.connection.await_reply(id, timeout).await?) } - /// Send `method`, then keep reading until BOTH its reply and `event` have - /// arrived, or `timeout` passes. + /// Send an action that may navigate (a click), then wait as long as the + /// page needs and no longer: for its load when it navigates, a short grace + /// when it does not (`navigation_watch`). `Page` must be enabled. /// - /// The reply alone is never enough for an action that may navigate: the - /// browser answers `Vellum.strike` as soon as the release is dispatched, and - /// the load that follows is what the caller is waiting for. The reply alone - /// is, however, still the answer when no load ever comes; that is the normal - /// case for a click that only changed state on the page. - /// - /// Returns the reply and whether `event` was seen. An error reply is an + /// Returns the reply and whether the page navigated. An error reply is an /// error; a socket that dies before the reply is one too. - pub async fn call_then_await_event( + pub async fn call_then_await_navigation( &mut self, method: &str, params: Value, - event: &str, timeout: Duration, ) -> Result<(Value, bool), WayfernError> { + use crate::navigation_watch::{main_frame_of, NavigationWatch}; + let main_frame = self + .call("Page.getFrameTree", serde_json::json!({})) + .await + .ok() + .and_then(|tree| main_frame_of(&tree)); let id = self.allocate_id(); self.connection.send_command(id, method, params).await?; let deadline = tokio::time::Instant::now() + timeout; + let mut watch = NavigationWatch::new(main_frame, false); let mut reply: Option = None; - let mut event_seen = false; loop { - if reply.is_some() && event_seen { + let now = tokio::time::Instant::now(); + if watch.done(now) || now >= deadline { break; } - let remaining = deadline.saturating_duration_since(tokio::time::Instant::now()); - if remaining.is_zero() { - break; - } - let text = match tokio::time::timeout(remaining, self.connection.next_text()).await { - Ok(Some(Ok(text))) => text, - Ok(Some(Err(e))) => { - if reply.is_none() { - return Err(e.into()); + let text = + match tokio::time::timeout_at(watch.wake_at(deadline), self.connection.next_text()).await { + Ok(Some(Ok(text))) => text, + Ok(Some(Err(e))) => { + if reply.is_none() { + return Err(e.into()); + } + break; } - break; - } - Ok(None) => { - if reply.is_none() { - return Err( - self - .connection - .closed_error("no response received from CDP") - .into(), - ); + Ok(None) => { + if reply.is_none() { + return Err( + self + .connection + .closed_error("no response received from CDP") + .into(), + ); + } + break; } - break; - } - Err(_) => break, - }; + // Woken for the end of the grace; the loop decides. + Err(_) => continue, + }; let message: Value = serde_json::from_str(&text).unwrap_or_default(); if message.get("id") == Some(&Value::from(id)) { @@ -694,15 +693,16 @@ impl WayfernSession { .cloned() .unwrap_or_else(|| serde_json::json!({})), ); + watch.on_reply(tokio::time::Instant::now()); continue; } - if message.get("method").and_then(Value::as_str) == Some(event) { - event_seen = true; + if let Some(event) = message.get("method").and_then(Value::as_str) { + watch.on_event(event, message.get("params")); } } match reply { - Some(reply) => Ok((reply, event_seen)), + Some(reply) => Ok((reply, watch.navigated())), None => { Err(CdpError::Transport(format!("timed out waiting for the reply to {method}")).into()) } @@ -1232,10 +1232,9 @@ pub mod vellum { load_timeout: Duration, ) -> Result<(Strike, bool), WayfernError> { let (reply, navigated) = session - .call_then_await_event( + .call_then_await_navigation( "Vellum.strike", strike_params(pointer, button, click_count), - "Page.loadEventFired", load_timeout, ) .await?;