fix(open): graceful load-timeout + --wait-until override for SPAs (issue #10)
`open` waits for the `load` event by default. SPAs whose `load` never fires (a long-pending XHR or a stuck sub-resource holds it open) made `open` hard-fail after the lifecycle timeout — even though the DOM was ready and eval/screenshot worked immediately right after. - Graceful degradation: if the lifecycle event times out but document.readyState is interactive/complete, navigate returns success carrying a `warning` in the response (the CLI prints it to stderr; --json keeps the field) instead of erroring. Only a still-loading document is a real failure. - `open/goto/navigate` now accept `--wait-until <load|domcontentloaded| networkidle|none>` so SPAs can return as soon as the DOM is parsed. The URL parser skips the --wait-until value so it isn't mistaken for the URL. - WaitUntil::as_str() for the warning label; output.rs surfaces response warnings. Verified live: --wait-until domcontentloaded returns immediately on a page whose load never fires; default load on the same page now succeeds at the timeout with a clear stderr warning instead of failing. Adds parse tests for both arg orders + bogus value.
This commit is contained in:
+76
-1
@@ -320,7 +320,27 @@ fn parse_command_inner(args: &[String], flags: &Flags) -> Result<Value, ParseErr
|
|||||||
// scripts before the first real navigation (see `batch`).
|
// scripts before the first real navigation (see `batch`).
|
||||||
// `goto` and `navigate` still require a URL since those verbs
|
// `goto` and `navigate` still require a URL since those verbs
|
||||||
// imply the navigation itself.
|
// imply the navigation itself.
|
||||||
let first_url = rest.iter().find(|a| !a.starts_with("--"));
|
// The URL is the first positional arg, skipping flags AND any value
|
||||||
|
// consumed by `--wait-until` (so it isn't mistaken for the URL).
|
||||||
|
let first_url = {
|
||||||
|
let mut url = None;
|
||||||
|
let mut skip_next = false;
|
||||||
|
for a in &rest {
|
||||||
|
if skip_next {
|
||||||
|
skip_next = false;
|
||||||
|
continue;
|
||||||
|
}
|
||||||
|
if *a == "--wait-until" {
|
||||||
|
skip_next = true;
|
||||||
|
continue;
|
||||||
|
}
|
||||||
|
if !a.starts_with("--") {
|
||||||
|
url = Some(a);
|
||||||
|
break;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
url
|
||||||
|
};
|
||||||
let url = match first_url {
|
let url = match first_url {
|
||||||
Some(u) => *u,
|
Some(u) => *u,
|
||||||
None if cmd == "open" => {
|
None if cmd == "open" => {
|
||||||
@@ -350,6 +370,23 @@ fn parse_command_inner(args: &[String], flags: &Flags) -> Result<Value, ParseErr
|
|||||||
if flags.provider.is_some() {
|
if flags.provider.is_some() {
|
||||||
nav_cmd["waitUntil"] = json!("none");
|
nav_cmd["waitUntil"] = json!("none");
|
||||||
}
|
}
|
||||||
|
// Explicit readiness override (issue #10): SPAs whose `load` event
|
||||||
|
// never fires (a long-lived XHR/websocket holds it open) hang out the
|
||||||
|
// load-event wait. `--wait-until domcontentloaded` returns as soon as
|
||||||
|
// the DOM is parsed.
|
||||||
|
if let Some(i) = rest.iter().position(|a| *a == "--wait-until") {
|
||||||
|
let val = rest.get(i + 1).ok_or(ParseError::MissingArguments {
|
||||||
|
context: "open --wait-until".to_string(),
|
||||||
|
usage: "open <url> --wait-until <load|domcontentloaded|networkidle|none>",
|
||||||
|
})?;
|
||||||
|
if !["load", "domcontentloaded", "networkidle", "none"].contains(val) {
|
||||||
|
return Err(ParseError::InvalidValue {
|
||||||
|
message: format!("Unknown --wait-until value: {}", val),
|
||||||
|
usage: "open <url> --wait-until <load|domcontentloaded|networkidle|none>",
|
||||||
|
});
|
||||||
|
}
|
||||||
|
nav_cmd["waitUntil"] = json!(val);
|
||||||
|
}
|
||||||
if let Some(ref headers_json) = flags.headers {
|
if let Some(ref headers_json) = flags.headers {
|
||||||
let headers =
|
let headers =
|
||||||
serde_json::from_str::<serde_json::Value>(headers_json).map_err(|_| {
|
serde_json::from_str::<serde_json::Value>(headers_json).map_err(|_| {
|
||||||
@@ -3643,6 +3680,44 @@ mod tests {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn test_open_wait_until_after_url() {
|
||||||
|
let cmd = parse_command(
|
||||||
|
&args("open https://x.com --wait-until domcontentloaded"),
|
||||||
|
&default_flags(),
|
||||||
|
)
|
||||||
|
.unwrap();
|
||||||
|
assert_eq!(cmd["action"], "navigate");
|
||||||
|
assert_eq!(cmd["url"], "https://x.com");
|
||||||
|
assert_eq!(cmd["waitUntil"], "domcontentloaded");
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn test_open_wait_until_before_url_not_mistaken_for_url() {
|
||||||
|
// The --wait-until value must not be picked up as the URL.
|
||||||
|
let cmd = parse_command(
|
||||||
|
&args("open --wait-until domcontentloaded https://x.com"),
|
||||||
|
&default_flags(),
|
||||||
|
)
|
||||||
|
.unwrap();
|
||||||
|
assert_eq!(cmd["url"], "https://x.com");
|
||||||
|
assert_eq!(cmd["waitUntil"], "domcontentloaded");
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn test_open_wait_until_rejects_bogus_value() {
|
||||||
|
let err = parse_command(
|
||||||
|
&args("open https://x.com --wait-until wat"),
|
||||||
|
&default_flags(),
|
||||||
|
)
|
||||||
|
.unwrap_err();
|
||||||
|
assert!(
|
||||||
|
err.format().contains("--wait-until"),
|
||||||
|
"got: {}",
|
||||||
|
err.format()
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn test_find_bare_value_suggests_text_locator() {
|
fn test_find_bare_value_suggests_text_locator() {
|
||||||
// `find "I'm not a robot" click` — value where a locator keyword was
|
// `find "I'm not a robot" click` — value where a locator keyword was
|
||||||
|
|||||||
@@ -265,6 +265,15 @@ impl WaitUntil {
|
|||||||
_ => Self::Load,
|
_ => Self::Load,
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
pub fn as_str(&self) -> &'static str {
|
||||||
|
match self {
|
||||||
|
Self::Load => "load",
|
||||||
|
Self::DomContentLoaded => "domcontentloaded",
|
||||||
|
Self::NetworkIdle => "networkidle",
|
||||||
|
Self::None => "none",
|
||||||
|
}
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
pub enum BrowserProcess {
|
pub enum BrowserProcess {
|
||||||
@@ -800,9 +809,38 @@ impl BrowserManager {
|
|||||||
// Only wait for lifecycle events if Chrome created a new loader (full navigation).
|
// Only wait for lifecycle events if Chrome created a new loader (full navigation).
|
||||||
// If loader_id is None, it was a same-document navigation (e.g., hash routing)
|
// If loader_id is None, it was a same-document navigation (e.g., hash routing)
|
||||||
// which does not fire Page.loadEventFired or Page.domContentEventFired.
|
// which does not fire Page.loadEventFired or Page.domContentEventFired.
|
||||||
|
let mut nav_warning: Option<String> = None;
|
||||||
if nav_result.loader_id.is_some() && wait_until != WaitUntil::None {
|
if nav_result.loader_id.is_some() && wait_until != WaitUntil::None {
|
||||||
self.wait_for_lifecycle(wait_until, &session_id, &mut lifecycle_rx)
|
if let Err(e) = self
|
||||||
.await?;
|
.wait_for_lifecycle(wait_until, &session_id, &mut lifecycle_rx)
|
||||||
|
.await
|
||||||
|
{
|
||||||
|
// The lifecycle event (e.g. `load`) didn't fire within the
|
||||||
|
// timeout. On SPAs this is common — a long-pending XHR or a stuck
|
||||||
|
// sub-resource holds `load` open long after the DOM is interactive
|
||||||
|
// and the page is usable, so `open` would hard-fail even though
|
||||||
|
// eval/screenshot work immediately (issue #10). If the DOM is
|
||||||
|
// already ready, treat navigation as done (with a warning, carried
|
||||||
|
// in the response so the CLI can surface it) instead of failing.
|
||||||
|
// Only a still-loading document is a real failure.
|
||||||
|
let ready = self
|
||||||
|
.evaluate_simple("document.readyState")
|
||||||
|
.await
|
||||||
|
.ok()
|
||||||
|
.and_then(|v| v.as_str().map(str::to_string))
|
||||||
|
.unwrap_or_default();
|
||||||
|
if ready == "interactive" || ready == "complete" {
|
||||||
|
nav_warning = Some(format!(
|
||||||
|
"`{}` didn't complete within the timeout, but the DOM is ready ({}) — \
|
||||||
|
continuing. Pass `--wait-until domcontentloaded` to skip this wait on \
|
||||||
|
SPAs with long-lived requests.",
|
||||||
|
wait_until.as_str(),
|
||||||
|
ready
|
||||||
|
));
|
||||||
|
} else {
|
||||||
|
return Err(e);
|
||||||
|
}
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
let page_url = self.get_url().await.unwrap_or_else(|_| url.to_string());
|
let page_url = self.get_url().await.unwrap_or_else(|_| url.to_string());
|
||||||
@@ -821,7 +859,11 @@ impl BrowserManager {
|
|||||||
page.title = title.clone();
|
page.title = title.clone();
|
||||||
}
|
}
|
||||||
|
|
||||||
Ok(json!({ "url": page_url, "title": title }))
|
let mut out = json!({ "url": page_url, "title": title });
|
||||||
|
if let Some(w) = nav_warning {
|
||||||
|
out["warning"] = json!(w);
|
||||||
|
}
|
||||||
|
Ok(out)
|
||||||
}
|
}
|
||||||
|
|
||||||
async fn wait_for_lifecycle(
|
async fn wait_for_lifecycle(
|
||||||
|
|||||||
@@ -256,6 +256,12 @@ pub fn print_response_with_opts(resp: &Response, action: Option<&str>, opts: &Ou
|
|||||||
// empty title line.
|
// empty title line.
|
||||||
None => println!("{} {}", color::success_indicator(), color::dim(url)),
|
None => println!("{} {}", color::success_indicator(), color::dim(url)),
|
||||||
}
|
}
|
||||||
|
// Soft warning carried in the response (e.g. the load event timed out
|
||||||
|
// but the DOM was ready — issue #10). Goes to stderr so it doesn't
|
||||||
|
// pollute the stdout url/title that scripts parse.
|
||||||
|
if let Some(w) = data.get("warning").and_then(|v| v.as_str()) {
|
||||||
|
eprintln!("⚠ navigation: {w}");
|
||||||
|
}
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
if let Some(cdp_url) = data.get("cdpUrl").and_then(|v| v.as_str()) {
|
if let Some(cdp_url) = data.get("cdpUrl").and_then(|v| v.as_str()) {
|
||||||
|
|||||||
Reference in New Issue
Block a user