Conversation
…ation - Add `--extra-headers` flag to `navigate` command for setting custom HTTP headers (e.g. Authorization) via Network.setExtraHTTPHeaders - Add `set-geolocation` command for geolocation emulation with --latitude, --longitude, --accuracy, and --clear flags using Emulation.setGeolocationOverride - Add unknown argument validation in daemon executor — returns structured error with expected argument list when unexpected fields are passed in a request - Update SKILL.md with examples for both new features
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds EmulateParams + emulate(); per-page emulation and extra-header support for navigate/new-page; centralizes JSON argument allowlists and early validation; updates pages API (close/select by target id); standardizes navigation timeout; refreshes docs/README/SKILL; and adds a known-args unit test. ChangesEmulation & Navigation Enhancements
sequenceDiagram
participant CLI
participant Executor
participant Emulation
participant NavigateCmd
participant PagesCmd
participant CdpClient
CLI->>Executor: JSON request (navigate / emulate / new-page)
Executor->>Emulation: emulate(params) [when present]
Emulation->>CdpClient: Emulation.clearDeviceMetricsOverride
Emulation->>CdpClient: Emulation.clearGeolocationOverride
CdpClient-->>Emulation: OK
Emulation->>CdpClient: Emulation.setDeviceMetricsOverride {width,height,deviceScaleFactor,mobile}
CdpClient-->>Emulation: OK
Emulation->>CdpClient: Emulation.setGeolocationOverride {latitude,longitude,accuracy}
CdpClient-->>Emulation: OK
Emulation-->>Executor: CommandResult(actions)
Executor->>NavigateCmd: navigate(..., extra_headers)
Executor->>PagesCmd: new_page(..., emulation?, extra_headers?)
PagesCmd->>CdpClient: create/attach target
PagesCmd->>CdpClient: Network.setExtraHTTPHeaders
NavigateCmd->>CdpClient: Page.navigate
NavigateCmd->>CdpClient: wait_for_load
CdpClient-->>NavigateCmd: navigated / loaded
NavigateCmd-->>Executor: CommandResult(navigated_to)
Executor-->>CLI: result
🎯 4 (Complex) | ⏱️ ~45 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new set-geolocation command and adds support for custom HTTP headers via the --extra-headers flag in the navigate command. It also implements a centralized argument validation system to detect unknown parameters. The review feedback focuses on improving the validation logic by including browser-level commands, handling the dialog_action parameter globally, and simplifying the geolocation implementation by removing redundant checks. All review comments provided actionable improvements and were retained.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/commands/emulation.rs`:
- Around line 26-38: Validate lat/lon/acc before calling client.send_to_target:
check that lat is between -90.0 and 90.0, lon is between -180.0 and 180.0, and
acc is non-negative (and optionally finite); if any check fails return an
Err(anyhow::anyhow!(...)) with a clear message (e.g., "latitude must be between
-90 and 90", "longitude must be between -180 and 180", "accuracy must be >= 0").
Place these checks right after computing lat, lon, acc (the variables in
question) and before the call to
client.send_to_target/Emulation.setGeolocationOverride so invalid inputs fail
fast locally.
In `@src/commands/executor.rs`:
- Line 130: Move the unknown-argument validation so it runs before any
browser-level early return: call validate_args(cmd, args)? at the start of the
outer execution path (before the browser shortcut) instead of only inside
inner_execute; update the function that currently returns early for browser
commands (the outer execute/execute_command wrapper) to invoke
validate_args(cmd, args)? first, and then proceed with the existing
browser-return logic and later call inner_execute as before.
In `@src/commands/navigate.rs`:
- Around line 17-31: The code applies extra headers via
client.send_to_target("Network.setExtraHTTPHeaders", ...) before verifying that
a navigation command will actually run, which can mutate session state on a
failing command; move the block that parses and sends extra_headers (the
extra_headers variable handling and the client.send_to_target call) so it
executes only after command intent is validated (i.e., after checking for url or
flags like --back/--forward/--reload), or alternatively perform the validation
first and return an error if no navigation intent is present before calling
Network.setExtraHTTPHeaders; keep references to session_id, extra_headers,
client.send_to_target and the Network.setExtraHTTPHeaders call so the fix
targets the same code paths.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d0c5497f-80c4-4713-b3dd-17175fa60015
📒 Files selected for processing (6)
skill/chrome-devtools/SKILL.mdsrc/commands/emulation.rssrc/commands/executor.rssrc/commands/mod.rssrc/commands/navigate.rssrc/main.rs
- Add lat/lon range validation (-90..90, -180..180) and non-negative finite accuracy check in set_geolocation before CDP call - Move validate_args to execute_command so browser-level commands (list-pages, new-page, close-page, select-page) are also validated - Add browser-level commands to known_args whitelist - Allow dialog_action globally for all page-level commands in validator - Simplify set-geolocation executor dispatch — pass options directly and let emulation.rs handle requirement logic (removes duplication) - Move extra-headers application after navigation intent validation in navigate.rs to avoid mutating session state on invalid input
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new set-geolocation command and adds support for custom HTTP headers in the navigate command via the --extra-headers flag. It also implements a centralized argument validation system to detect unknown parameters across all commands. Feedback highlights that the geolocation override may be lost due to session detachment in the current executor, suggesting a need for persistent sessions or integration into the navigate command. Additionally, the extra-headers implementation needs stricter validation to ensure all header values are strings and should explicitly enable the Network domain before use.
- Replace resize and set-geolocation with single `emulate` command: --viewport 1280x720, --geolocation lat,lon, --accuracy N, --clear-viewport, --clear-geolocation, --clear-all - `emulate` with no flags shows all active overrides (viewport via Emulation.getDeviceMetricsOverride, geolocation via JS) - Add --latitude/--longitude/--accuracy/--clear-geolocation flags to navigate so geolocation is set before page loads and persists after - Extra headers: validate all values are strings, call Network.enable before Network.setExtraHTTPHeaders - Move extra-headers application after navigation intent validation - Move validate_args to execute_command so browser-level commands are also validated; allow dialog_action globally for page commands - Add browser-level commands to known_args whitelist - Show subcommand-specific help on bad args instead of global help - Document viewport/geolocation as page-based in README and SKILL
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a unified emulate command, replacing the previous resize command, to manage viewport and geolocation overrides. It also enhances the navigate command with support for custom HTTP headers and geolocation settings. Additionally, a new argument validation layer was added to the command executor to catch unknown parameters, and CLI help output was improved to be subcommand-specific. Feedback focused on refactoring duplicated geolocation validation logic into a shared utility and addressing potential side effects or limitations in the emulate implementation, such as hardcoded viewport settings and the use of intrusive browser APIs for state retrieval.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/commands/emulation.rs`:
- Around line 135-137: The code tries to call the non-existent CDP method
"Emulation.getDeviceMetricsOverride" via client.send_to_target(session_id,
"Emulation.getDeviceMetricsOverride", ...) (assigned to vp), which will fail;
replace that read path by either treating the viewport override as None (i.e.,
assume no override) or derive effective metrics from the page using a
Runtime.evaluate call to read
window.innerWidth/window.innerHeight/devicePixelRatio, or maintain and consult
an internal last-known override state when you send setDeviceMetricsOverride;
specifically remove the client.send_to_target call for
"Emulation.getDeviceMetricsOverride", update the emulate command's logic that
reads vp to accept a None/default case or to use Runtime.evaluate (or a stored
field) to populate vp, and ensure any downstream code that used vp handles the
none/default case.
In `@src/commands/navigate.rs`:
- Around line 27-35: The current logic silently skips setting coordinates when
both clear_geolocation and latitude/longitude flags are passed; update the top
of the geolocation branch in navigate.rs to detect this conflict and return an
explicit error instead of proceeding: if clear_geolocation is true and either
latitude.is_some() or longitude.is_some(), return an
anyhow::anyhow!("conflicting flags: --clear-geolocation cannot be used with
--latitude/--longitude") (or equivalent) so the caller gets a clear failure;
leave the existing clear_geolocation path (client.send_to_target(session_id,
"Emulation.clearGeolocationOverride", ...)) and the coordinate-setting path
(resolving lat/lon/accuracy) unchanged otherwise.
- Around line 54-76: The code sets persistent CDP headers via
client.send_to_target(session_id, "Network.setExtraHTTPHeaders", ...) without
clearing them, which can leak sensitive headers; after the navigation flow that
uses these headers (i.e., after the navigation request/response handling in the
same function), send a subsequent client.send_to_target(session_id,
"Network.setExtraHTTPHeaders", json!({"headers": {}})). Ensure this cleanup runs
even on errors (e.g., in a finally/cleanup block or by using a guard) so headers
are always reset for the session_id.
In `@src/main.rs`:
- Around line 404-411: The current logic uses std::env::args().skip(1).find(|a|
!a.starts_with('-')).and_then(|name| cmd.find_subcommand_mut(&name)) which can
pick flag values (e.g. "red-snake") instead of an actual subcommand; change it
to scan non-flag args and return the first one that actually matches a
subcommand by replacing the chain with something like
std::env::args().skip(1).find_map(|name| if !name.starts_with('-') {
cmd.find_subcommand_mut(&name) } else { None }) so you call
cmd.find_subcommand_mut for each candidate and only pick a real subcommand (then
keep the existing match on sub and calls to
sub_cmd.print_help()/cmd.print_help()).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 96378c77-13ff-40f4-a267-5a3a10ced83b
📒 Files selected for processing (7)
README.mdskill/chrome-devtools/SKILL.mdsrc/commands/emulation.rssrc/commands/executor.rssrc/commands/navigate.rssrc/commands/pages.rssrc/main.rs
💤 Files with no reviewable changes (1)
- src/commands/pages.rs
- Consolidate viewport and geolocation overrides into a single `emulate` command. - Restore atomic emulation support in `navigate` and `new-page` commands. - Refactor `close-page` and `select-page` to support global `--target` and `--page` flags. - Update documentation in `README.md` and `SKILL.md` to reflect the new command structure.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a unified emulate command for managing viewport and geolocation settings, replacing the specific resize command. It also enables atomic emulation during navigation and new page creation, and adds support for custom HTTP headers via --extra-headers. A new argument validation mechanism has been implemented to catch unknown parameters. Review feedback points out that Emulation.getDeviceMetricsOverride is not a standard CDP method and should be replaced with JavaScript evaluation. Furthermore, the geolocation retrieval script requires better error handling for non-secure contexts, and the internal known_args list needs to be updated to reflect changes in the close-page and select-page command structures.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/main.rs (1)
600-619: 💤 Low valueDead code:
NewPageis already handled in the browser-level branch.
is_browser_level()returnstrueforCommands::NewPage(line 216), causing an early return at line 527. This match arm at lines 600-619 is unreachable.🧹 Remove unreachable arm
} - Commands::NewPage { - url, - viewport, - geolocation, - accuracy, - } => { - let params = if viewport.is_some() || geolocation.is_some() { - Some(commands::emulation::EmulateParams { - viewport: viewport.clone(), - geolocation: geolocation.clone(), - accuracy: *accuracy, - clear_viewport: false, - clear_geolocation: false, - clear_all: false, - }) - } else { - None - }; - commands::pages::new_page(&mut client, url, params).await - } Commands::Screenshot {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main.rs` around lines 600 - 619, The Commands::NewPage match arm in the main command dispatch is unreachable because is_browser_level() causes NewPage to be handled earlier; remove the NewPage arm (the match branch that constructs EmulateParams and calls commands::pages::new_page) from the Commands match to eliminate dead code, and ensure nothing else relies on that local params construction—if emulation params are needed at browser-level, move or reuse that logic in the browser-level handler rather than keeping this arm.src/commands/executor.rs (1)
117-125: 💤 Low value
known_argsforclose-page/select-pagelists"index"but args are not read.The new special case correctly uses
target_idfromresolve_page, butknown_argsat lines 15-16 still lists"index"as a valid argument. This means a client can pass{"index": 0}without validation error, yet it will be silently ignored since the code usesreq.target/req.pageinstead.If
indexis kept for backward compatibility with existing clients, consider documenting this behavior. Otherwise, updateknown_argsto&[]for these commands to reject the now-unused parameter.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/executor.rs` around lines 117 - 125, The review points out that the special-case branch for "close-page" and "select-page" uses target_id from resolve_page (and calls commands::pages::close_page / commands::pages::select_page) but known_args still lists "index", so a passed {"index":...} will be accepted but ignored; update the command metadata to reflect the new argument behavior by removing "index" from known_args for close-page and select-page (set known_args to &[]), or if you intend to keep "index" for backward compatibility, add a comment/documentation and explicitly read/validate req.target/req.page vs index; specifically change the known_args entry for the "close-page"/"select-page" command definitions and ensure resolve_page/target_id usage remains consistent with req.target/req.page handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/commands/emulation.rs`:
- Around line 73-80: The code assigns acc = params.accuracy.unwrap_or(100.0) but
does not validate it; add a check after that assignment to ensure acc is
non-negative (and optionally finite) and return an error if not—e.g. if
acc.is_sign_negative() || !acc.is_finite() { anyhow::bail!("accuracy must be a
non-negative finite value"); }—so you validate params.accuracy (and the local
acc variable) similarly to the existing latitude/longitude checks.
---
Nitpick comments:
In `@src/commands/executor.rs`:
- Around line 117-125: The review points out that the special-case branch for
"close-page" and "select-page" uses target_id from resolve_page (and calls
commands::pages::close_page / commands::pages::select_page) but known_args still
lists "index", so a passed {"index":...} will be accepted but ignored; update
the command metadata to reflect the new argument behavior by removing "index"
from known_args for close-page and select-page (set known_args to &[]), or if
you intend to keep "index" for backward compatibility, add a
comment/documentation and explicitly read/validate req.target/req.page vs index;
specifically change the known_args entry for the "close-page"/"select-page"
command definitions and ensure resolve_page/target_id usage remains consistent
with req.target/req.page handling.
In `@src/main.rs`:
- Around line 600-619: The Commands::NewPage match arm in the main command
dispatch is unreachable because is_browser_level() causes NewPage to be handled
earlier; remove the NewPage arm (the match branch that constructs EmulateParams
and calls commands::pages::new_page) from the Commands match to eliminate dead
code, and ensure nothing else relies on that local params construction—if
emulation params are needed at browser-level, move or reuse that logic in the
browser-level handler rather than keeping this arm.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ec2d1698-bb8b-4103-8e35-34f32cedef8e
📒 Files selected for processing (7)
README.mdskill/chrome-devtools/SKILL.mdsrc/commands/emulation.rssrc/commands/executor.rssrc/commands/navigate.rssrc/commands/pages.rssrc/main.rs
💤 Files with no reviewable changes (1)
- src/commands/navigate.rs
✅ Files skipped from review due to trivial changes (2)
- skill/chrome-devtools/SKILL.md
- README.md
- Consolidate viewport and geolocation overrides into a single `emulate` command (setter only). - Restore atomic emulation support in `navigate` and `new-page` commands. - Refactor `close-page` and `select-page` to support both positional arguments (ID or index) and global flags. - Remove non-existent `Emulation.getDeviceMetricsOverride` getter and `as_json` internal flag. - Add validation for geolocation `accuracy` parameter. - Clean up unreachable code and unused imports.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a unified emulation system for managing viewport and geolocation settings, replacing the previous resize command. It updates the navigate and new-page commands to support atomic emulation and custom HTTP headers, while also improving argument validation and allowing pages to be targeted by ID, name, or index. Feedback from the review identifies a potential session leak in the new_page function when emulation fails and suggests more robust parsing for viewport dimensions. Additionally, the reviewer recommended simplifying redundant logic in the emulation module and ensuring consistent page-load waiting behavior across all navigation-related commands.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/commands/pages.rs (2)
100-117:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDon't turn CDP failures into timeouts.
This loop drops every
Runtime.evaluateerror. If the tab closes or the session detaches,wait_forwill burn the full timeout and return the wrong error instead of surfacing the real failure.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/pages.rs` around lines 100 - 117, The loop is currently ignoring errors from client.send_to_target (the "Runtime.evaluate" call) which masks real failures (like closed tab/session) and makes wait_for return a timeout; change the match on result so that on Err you immediately propagate or return the underlying CDP error instead of continuing the loop—e.g. match the Result from client.send_to_target and on Err return Err(...) (or map_err and propagate) so failures from client.send_to_target/Runtime.evaluate surface to the caller of wait_for rather than being treated as a transient miss; keep the existing success branch that checks val["result"]["value"] and sleeps only when the call succeeded but returned false.
44-61:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClean up the temporary target on failure.
Any error after Line 46 leaves an orphan
about:blanktab and a still-attached session, because detach only runs on the happy path and the temporary target is never rolled back.Suggested fix
if let Some(params) = emulation { // Create an empty page first so we can apply emulation before it loads the real URL let target_id = client.create_target("about:blank").await?; let session_id = client.attach_to_target(&target_id).await?; - // Apply emulation to the new session - crate::commands::emulation::emulate(client, &session_id, params).await?; - - // Now navigate to the real URL - client - .send_to_target(&session_id, "Page.navigate", json!({ "url": url })) - .await?; + let result = async { + crate::commands::emulation::emulate(client, &session_id, params).await?; + client + .send_to_target(&session_id, "Page.navigate", json!({ "url": url })) + .await?; + Ok::<(), anyhow::Error>(()) + } + .await; // Optional: wait for load (consistent with navigate command) // Since we don't have wait_for_load easily available here (it's in navigate.rs), // we'll just return. The user can wait-for if needed. let _ = client.detach_from_target(&session_id).await; + if let Err(err) = result { + let _ = client.close_target(&target_id).await; + return Err(err); + } Ok(CommandResult::output(format!( "Opened new page with emulation: {url} (target: {target_id})" )))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/pages.rs` around lines 44 - 61, The temporary "about:blank" target and its session are not cleaned up on errors after create_target/attach_to_target, so ensure you always detach and close the created target when any subsequent step fails; capture target_id and session_id immediately after create_target and attach_to_target and add a cleanup path (using a drop guard, finally-like block, or explicit match/if let Err branch) that calls client.detach_from_target(&session_id).await and client.close_target(&target_id).await (or the appropriate close method) before returning the error; update the emulation block that calls create_target, attach_to_target, emulate, and send_to_target to use this guaranteed cleanup.src/main.rs (1)
522-533:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDon't silently drop
--accuracyin direct fallback.
new-page --accuracy ...andnavigate --accuracy ...never reach emulation unless some other emulation flag is set, so fallback mode ignores the flag instead of matching daemon behavior or rejecting the combination.Suggested fix
- let params = if viewport.is_some() || geolocation.is_some() { + let params = if viewport.is_some() || geolocation.is_some() || accuracy.is_some() { Some(commands::emulation::EmulateParams { viewport: viewport.clone(), geolocation: geolocation.clone(), accuracy: *accuracy, clear_viewport: false, clear_geolocation: false, clear_all: false, }) } else { None };- if viewport.is_some() || geolocation.is_some() || *clear_all { + if viewport.is_some() || geolocation.is_some() || accuracy.is_some() || *clear_all { commands::emulation::emulate( &mut client, &session_id, commands::emulation::EmulateParams { viewport: viewport.clone(), geolocation: geolocation.clone(), accuracy: *accuracy, clear_viewport: false, clear_geolocation: false, clear_all: *clear_all, }, ) .await?; }If
--accuracywithout--geolocationis intentionally invalid, return an error here instead of ignoring it.Also applies to: 595-610
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main.rs` around lines 522 - 533, The fallback logic builds EmulateParams but silently ignores --accuracy when geolocation is None; update the code that constructs params (the block creating commands::emulation::EmulateParams using viewport, geolocation, accuracy, clear_*) to detect when accuracy.is_some() && geolocation.is_none() and reject that combination by returning an appropriate error (or propagate an Err) instead of dropping the flag; apply the same change to the other analogous block around the second occurrence (the similar params construction at 595-610) so both fallback paths match daemon behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/commands/executor.rs`:
- Around line 115-124: The branch handling "close-page" / "select-page"
incorrectly only treats args.get("id_or_index").and_then(|v| v.as_str()) so
numeric JSON values (e.g. 1) are ignored; update the logic in the block that
computes target_id_arg and page_idx_arg (the code that reads id_or_index) to
also check for numeric variants (use Value::as_i64 / as_u64 or similar) and
convert to usize when present, falling back to treating strings as before —
ensure conversion is safe (non-negative) and returns (None, Some(idx)) for
numeric indexes and (Some(s), None) for string IDs.
---
Outside diff comments:
In `@src/commands/pages.rs`:
- Around line 100-117: The loop is currently ignoring errors from
client.send_to_target (the "Runtime.evaluate" call) which masks real failures
(like closed tab/session) and makes wait_for return a timeout; change the match
on result so that on Err you immediately propagate or return the underlying CDP
error instead of continuing the loop—e.g. match the Result from
client.send_to_target and on Err return Err(...) (or map_err and propagate) so
failures from client.send_to_target/Runtime.evaluate surface to the caller of
wait_for rather than being treated as a transient miss; keep the existing
success branch that checks val["result"]["value"] and sleeps only when the call
succeeded but returned false.
- Around line 44-61: The temporary "about:blank" target and its session are not
cleaned up on errors after create_target/attach_to_target, so ensure you always
detach and close the created target when any subsequent step fails; capture
target_id and session_id immediately after create_target and attach_to_target
and add a cleanup path (using a drop guard, finally-like block, or explicit
match/if let Err branch) that calls client.detach_from_target(&session_id).await
and client.close_target(&target_id).await (or the appropriate close method)
before returning the error; update the emulation block that calls create_target,
attach_to_target, emulate, and send_to_target to use this guaranteed cleanup.
In `@src/main.rs`:
- Around line 522-533: The fallback logic builds EmulateParams but silently
ignores --accuracy when geolocation is None; update the code that constructs
params (the block creating commands::emulation::EmulateParams using viewport,
geolocation, accuracy, clear_*) to detect when accuracy.is_some() &&
geolocation.is_none() and reject that combination by returning an appropriate
error (or propagate an Err) instead of dropping the flag; apply the same change
to the other analogous block around the second occurrence (the similar params
construction at 595-610) so both fallback paths match daemon behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 10ad6edc-efe3-4552-8ada-8a29acb10c53
📒 Files selected for processing (6)
README.mdskill/chrome-devtools/SKILL.mdsrc/commands/emulation.rssrc/commands/executor.rssrc/commands/pages.rssrc/main.rs
💤 Files with no reviewable changes (1)
- README.md
✅ Files skipped from review due to trivial changes (1)
- skill/chrome-devtools/SKILL.md
- Simplify action validation in `emulate` command. - Support numeric JSON values for `id_or_index` in page management commands. - Ensure consistent navigation behavior by making `wait_for_load` public and using it in `new_page`. - Guarantee cleanup of temporary CDP sessions and targets on failure in `new_page`. - Surface real CDP errors in `wait_for` instead of masking them as timeouts. - Add CLI-level validation to reject `--accuracy` without `--geolocation` in fallback mode.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a unified emulation system for viewport and geolocation overrides, enabling atomic application during navigation and new page creation. It also adds support for custom HTTP headers, enhances page targeting for tab management, and implements stricter argument validation for daemon requests. Feedback highlights the need to make the mobile emulation flag configurable, the maintenance risk of manually duplicating argument lists in the executor, a missing validation check for geolocation accuracy in the daemon, and the benefit of using shared constants for hardcoded timeouts.
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
skill/chrome-devtools/SKILL.md (1)
60-69:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDocument
navigate --extra-headersin the navigation reference.This section adds emulation-on-navigate examples but still omits the new custom-header flag, so one of the PR’s key features isn’t discoverable from SKILL.md.
📝 Suggested docs patch
### Navigation ```bash -chrome-devtools navigate <url> [--viewport WxH] [--geolocation lat,lon] +chrome-devtools navigate <url> [--viewport WxH] [--geolocation lat,lon] [--extra-headers '{"Authorization":"Bearer ..."}'] chrome-devtools navigate --back chrome-devtools navigate --forward chrome-devtools navigate --reload chrome-devtools new-page <url> [--viewport WxH] [--geolocation lat,lon] chrome-devtools close-page [id_or_index] chrome-devtools select-page [id_or_index] + +# Example: custom request headers during navigation +chrome-devtools navigate https://httpbin.org/headers --extra-headers '{"X-Debug":"1"}'</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@skill/chrome-devtools/SKILL.mdaround lines 60 - 69, Update the Navigation
section of SKILL.md to document the new --extra-headers flag for the
chrome-devtools navigate command: add [--extra-headers '{"Authorization":"Bearer
..."}'] to the chrome-devtools navigate synopsis, include a one-line
example showing usage (e.g. chrome-devtools navigate https://httpbin.org/headers
--extra-headers '{"X-Debug":"1"}'), and ensure the new flag appears alongside
the existing options in the list for navigate and new-page so users can discover
the custom-header feature.</details> </blockquote></details> <details> <summary>src/commands/navigate.rs (1)</summary><blockquote> `158-187`: _⚠️ Potential issue_ | _🟠 Major_ | _⚡ Quick win_ **Fail fast on CDP evaluation errors in `wait_for_load`.** On Line [178], `Runtime.evaluate` errors are ignored; after that, Line [166] can still return success on timeout. This can falsely report successful navigation when the session/target is failing. <details> <summary>Proposed fix</summary> ```diff pub async fn wait_for_load(client: &mut CdpClient, session_id: &str, timeout_ms: u64) -> Result<()> { @@ - let result = client + let result = client .send_to_target( session_id, "Runtime.evaluate", json!({ "expression": "document.readyState", "returnByValue": true, }), ) - .await; + .await?; - if let Ok(val) = result { - if val["result"]["value"].as_str() == Some("complete") { - return Ok(()); - } + if result["result"]["value"].as_str() == Some("complete") { + return Ok(()); } @@ } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/navigate.rs` around lines 158 - 187, The loop in wait_for_load swallows errors from client.send_to_target (Runtime.evaluate) so transient/target/session failures are ignored and the function can still return Ok on timeout; change the logic to fail fast: when client.send_to_target(session_id, "Runtime.evaluate", ...) returns Err, convert/propagate that error (e.g., return Err(anyhow::Error::new(...) or use ? with contextualize) instead of continuing the loop, and keep the existing readyState check on Ok responses; update the error message to include session_id and the original error for debugging.
♻️ Duplicate comments (1)
src/commands/executor.rs (1)
125-125:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInvalid numeric
id_or_indexvalues silently fall through to default page.When
id_or_indexis a number thatas_u64()cannot convert (negative, float, or too large), the result is(None, None), causingresolve_pageto select the default page instead of erroring. For example,{"id_or_index": -1}would close/select the default page rather than returning an error.🛡️ Proposed fix to validate numeric values
- Some(v) if v.is_number() => (None, v.as_u64().map(|idx| idx as usize)), + Some(v) if v.is_number() => { + let idx = v + .as_u64() + .ok_or_else(|| anyhow!("id_or_index must be a non-negative integer"))?; + (None, Some(idx as usize)) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/executor.rs` at line 125, The numeric match arm that currently returns (None, v.as_u64().map(|idx| idx as usize)) must validate numbers that cannot be converted (negative, float, too large) instead of falling through to default; update the match in the id_or_index handling (the arm matching Some(v) if v.is_number()) to explicitly detect invalid numeric conversions and return an error (or Result::Err) with a clear message (e.g., "invalid numeric id_or_index") rather than producing (None, None), so that resolve_page (or the caller) receives a failure for invalid numeric inputs.
🧹 Nitpick comments (1)
src/commands/executor.rs (1)
189-189: 💤 Low valueRemove unnecessary reference.
session_idis already&str; using&session_idcreates&&strwhich is immediately dereferenced. Line 205 correctly passessession_iddirectly.♻️ Proposed fix
commands::emulation::emulate( client, - &session_id, + session_id, commands::emulation::EmulateParams {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/executor.rs` at line 189, The call is passing &&str by using &session_id even though session_id is already a &str; update the call site that currently passes &session_id to pass session_id directly (remove the extra ampersand). Locate the invocation in executor.rs where session_id is supplied (same place referenced and consistent with the later correct call on line 205) and change the argument from &session_id to session_id so the function receives a &str rather than &&str.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/commands/pages.rs`:
- Around line 52-57: The Page.navigate call in the new_page flow uses
client.send_to_target but ignores the response's errorText, unlike the existing
navigate() path; update the new_page flow to capture the response from
client.send_to_target("Page.navigate", ...) and inspect the returned JSON for an
"errorText" (or equivalent) field, and if present return an Err (or propagate a
failure) with that message before calling
crate::commands::navigate::wait_for_load so failed navigations are detected the
same way as navigate().
---
Outside diff comments:
In `@skill/chrome-devtools/SKILL.md`:
- Around line 60-69: Update the Navigation section of SKILL.md to document the
new --extra-headers flag for the chrome-devtools navigate command: add
[--extra-headers '{"Authorization":"Bearer ..."}'] to the chrome-devtools
navigate <url> synopsis, include a one-line example showing usage (e.g.
chrome-devtools navigate https://httpbin.org/headers --extra-headers
'{"X-Debug":"1"}'), and ensure the new flag appears alongside the existing
options in the list for navigate and new-page so users can discover the
custom-header feature.
In `@src/commands/navigate.rs`:
- Around line 158-187: The loop in wait_for_load swallows errors from
client.send_to_target (Runtime.evaluate) so transient/target/session failures
are ignored and the function can still return Ok on timeout; change the logic to
fail fast: when client.send_to_target(session_id, "Runtime.evaluate", ...)
returns Err, convert/propagate that error (e.g., return
Err(anyhow::Error::new(...) or use ? with contextualize) instead of continuing
the loop, and keep the existing readyState check on Ok responses; update the
error message to include session_id and the original error for debugging.
---
Duplicate comments:
In `@src/commands/executor.rs`:
- Line 125: The numeric match arm that currently returns (None,
v.as_u64().map(|idx| idx as usize)) must validate numbers that cannot be
converted (negative, float, too large) instead of falling through to default;
update the match in the id_or_index handling (the arm matching Some(v) if
v.is_number()) to explicitly detect invalid numeric conversions and return an
error (or Result::Err) with a clear message (e.g., "invalid numeric
id_or_index") rather than producing (None, None), so that resolve_page (or the
caller) receives a failure for invalid numeric inputs.
---
Nitpick comments:
In `@src/commands/executor.rs`:
- Line 189: The call is passing &&str by using &session_id even though
session_id is already a &str; update the call site that currently passes
&session_id to pass session_id directly (remove the extra ampersand). Locate the
invocation in executor.rs where session_id is supplied (same place referenced
and consistent with the later correct call on line 205) and change the argument
from &session_id to session_id so the function receives a &str rather than
&&str.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d2959493-9edf-4b4f-96d2-f63465bba306
📒 Files selected for processing (7)
README.mdskill/chrome-devtools/SKILL.mdsrc/commands/emulation.rssrc/commands/executor.rssrc/commands/navigate.rssrc/commands/pages.rssrc/main.rs
💤 Files with no reviewable changes (1)
- README.md
… handling - Add --mobile and --device-scale-factor CLI flags to navigate, new-page, and emulate commands - Expose mobile/device_scale_factor in EmulateParams instead of hardcoding false/1.0 - Validate --accuracy requires --geolocation in daemon executor (consistent with CLI) - Fail fast in wait_for_load when Runtime.evaluate returns an error - Check Page.navigate response for errorText in new_page flow - Validate numeric id_or_index (reject negative/float/too-large values) - Remove extra & from session_id (&str already) in emulate call - Extract NAVIGATION_TIMEOUT_MS constant to replace hardcoded 30_000 - Update known_args with new mobile/device_scale_factor fields - Update SKILL.md and README.md with new flags and documentation
|
/gemini review |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main.rs (2)
560-577:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
--mobileand--device-scale-factorsilently ignored without--viewport.The condition at line 564 only checks
viewport.is_some() || geolocation.is_some(). If a user passes--mobileor--device-scale-factor 2.0without--viewport, the params remainNoneand these flags are silently ignored. This is inconsistent with theEmulatecommand which always forwards all params for validation.Consider adding upfront validation similar to the existing
--accuracy requires --geolocationcheck:Proposed validation
if accuracy.is_some() && geolocation.is_none() { anyhow::bail!("--accuracy requires --geolocation"); } + if (*mobile || device_scale_factor.is_some()) && viewport.is_none() { + anyhow::bail!("--mobile and --device-scale-factor require --viewport"); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main.rs` around lines 560 - 577, The current condition that builds params only when viewport or geolocation is present causes --mobile and --device_scale_factor to be ignored; update the validation and params construction so that if mobile.is_true() or device_scale_factor.is_some() are provided without a viewport, you either require --viewport (add checks similar to the existing accuracy->geolocation check) or always construct Some(commands::emulation::EmulateParams { ... }) to let EmulateParams validate them; specifically adjust the condition around creating params (where viewport, geolocation, device_scale_factor, mobile, accuracy are used) and add a pre-check that errors when mobile or device_scale_factor are supplied without viewport (or remove the dependency and always create EmulateParams) so that mobile and device_scale_factor are not silently ignored.
641-662:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSame issue:
--mobileand--device-scale-factorsilently ignored without--viewportin Navigate.The condition at line 646 doesn't account for
*mobileordevice_scale_factor.is_some(). Apply the same fix as suggested for NewPage.Proposed validation
if accuracy.is_some() && geolocation.is_none() { anyhow::bail!("--accuracy requires --geolocation"); } + if (*mobile || device_scale_factor.is_some()) && viewport.is_none() { + anyhow::bail!("--mobile and --device-scale-factor require --viewport"); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main.rs` around lines 641 - 662, The Navigate code currently ignores --mobile and --device-scale-factor when --viewport is not set; to fix, add validation checks similar to the accuracy/geolocation check: if device_scale_factor.is_some() && viewport.is_none() { anyhow::bail!("--device-scale-factor requires --viewport"); } and if *mobile && viewport.is_none() { anyhow::bail!("--mobile requires --viewport"); } and update the emulate invocation gating condition to include device_scale_factor.is_some() || *mobile (i.e., change the if to: if viewport.is_some() || geolocation.is_some() || device_scale_factor.is_some() || *mobile || *clear_all { ... } so commands::emulation::emulate(...) runs when those flags are provided).
🧹 Nitpick comments (3)
src/commands/navigate.rs (1)
165-168: 💤 Low valueTimeout returns success with warning; consider consistency with
wait_for.
wait_for_loadprints a warning and returnsOk(())on timeout, whilewait_forinpages.rsbails with an error. This might be intentional (navigation succeeded, just load didn't finish), but the inconsistent behavior could confuse callers expecting uniform timeout semantics.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/navigate.rs` around lines 165 - 168, The timeout branch in wait_for_load currently prints a warning and returns Ok(()), which is inconsistent with pages.rs::wait_for that returns an error on timeout; change wait_for_load to return a Result::Err on deadline expiry (or otherwise mirror pages.rs::wait_for semantics) so callers receive a failure instead of a silent warning, update the function signature/return path of wait_for_load accordingly, and adjust any callers to handle the error (or add explicit docs/tests if you intentionally keep the warning approach) — reference function names: wait_for_load and wait_for.src/commands/executor.rs (2)
307-323: ⚡ Quick winSame accuracy-requires-geolocation validation gap as
new-page.The standalone
emulatecommand also doesn't validate that--accuracyrequires--geolocation. For consistency, consider adding the same check here or centralizing the validation inemulation::emulateitself.🛠️ Suggested fix
"emulate" => { + let geolocation = args.get("geolocation").and_then(|v| v.as_str()); + let accuracy = args.get("accuracy").and_then(|v| v.as_f64()); + if accuracy.is_some() && geolocation.is_none() { + bail!("--accuracy requires --geolocation"); + } + commands::emulation::emulate( client, session_id, commands::emulation::EmulateParams { viewport: args.get("viewport").and_then(|v| v.as_str()).map(|s| s.to_string()), device_scale_factor: args.get("device_scale_factor").and_then(|v| v.as_f64()), mobile: args.get("mobile").and_then(|v| v.as_bool()).unwrap_or(false), - geolocation: args.get("geolocation").and_then(|v| v.as_str()).map(|s| s.to_string()), - accuracy: args.get("accuracy").and_then(|v| v.as_f64()), + geolocation: geolocation.map(|s| s.to_string()), + accuracy,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/executor.rs` around lines 307 - 323, The "emulate" match arm currently constructs EmulateParams without ensuring that accuracy is only provided when geolocation is present; add the same validation used for `new-page` so that when args contains "accuracy" but not "geolocation" you return an error before calling commands::emulation::emulate (or move/duplicate this check into commands::emulation::emulate for centralized validation). Specifically, check the parsed values derived for EmulateParams.accuracy and EmulateParams.geolocation and return an appropriate error/result if accuracy.is_some() && geolocation.is_none(), referencing the match arm handling "emulate" and the EmulateParams fields to locate where to add the guard.
95-108: ⚡ Quick winConsider adding accuracy-requires-geolocation validation for consistency with
navigate.The
navigatecommand validates that--accuracyrequires--geolocation(lines 192-195), butnew-pagedoes not. If a user provides--viewport 1280x720 --accuracy 50without--geolocation, the accuracy value is silently ignored whenemulation::emulateskips the geolocation block. This creates inconsistent UX across commands.🛠️ Suggested fix
let viewport = args.get("viewport").and_then(|v| v.as_str()); let geolocation = args.get("geolocation").and_then(|v| v.as_str()); +let accuracy = args.get("accuracy").and_then(|v| v.as_f64()); + +if accuracy.is_some() && geolocation.is_none() { + bail!("--accuracy requires --geolocation"); +} let params = if viewport.is_some() || geolocation.is_some() { Some(commands::emulation::EmulateParams { viewport: viewport.map(|s| s.to_string()), device_scale_factor: args.get("device_scale_factor").and_then(|v| v.as_f64()), mobile: args.get("mobile").and_then(|v| v.as_bool()).unwrap_or(false), geolocation: geolocation.map(|s| s.to_string()), - accuracy: args.get("accuracy").and_then(|v| v.as_f64()), + accuracy, clear_viewport: false,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/executor.rs` around lines 95 - 108, The new-page emulation block builds EmulateParams even when accuracy is provided but geolocation is missing, causing accuracy to be ignored; add the same validation used by navigate: if args.get("accuracy") is Some and geolocation.is_none() then return an error (or surface a user-facing validation message) instead of silently continuing. Locate the params construction that creates commands::emulation::EmulateParams and add a pre-check using args.get("accuracy") and the geolocation variable to enforce "accuracy requires geolocation" before creating EmulateParams.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/main.rs`:
- Around line 560-577: The current condition that builds params only when
viewport or geolocation is present causes --mobile and --device_scale_factor to
be ignored; update the validation and params construction so that if
mobile.is_true() or device_scale_factor.is_some() are provided without a
viewport, you either require --viewport (add checks similar to the existing
accuracy->geolocation check) or always construct
Some(commands::emulation::EmulateParams { ... }) to let EmulateParams validate
them; specifically adjust the condition around creating params (where viewport,
geolocation, device_scale_factor, mobile, accuracy are used) and add a pre-check
that errors when mobile or device_scale_factor are supplied without viewport (or
remove the dependency and always create EmulateParams) so that mobile and
device_scale_factor are not silently ignored.
- Around line 641-662: The Navigate code currently ignores --mobile and
--device-scale-factor when --viewport is not set; to fix, add validation checks
similar to the accuracy/geolocation check: if device_scale_factor.is_some() &&
viewport.is_none() { anyhow::bail!("--device-scale-factor requires --viewport");
} and if *mobile && viewport.is_none() { anyhow::bail!("--mobile requires
--viewport"); } and update the emulate invocation gating condition to include
device_scale_factor.is_some() || *mobile (i.e., change the if to: if
viewport.is_some() || geolocation.is_some() || device_scale_factor.is_some() ||
*mobile || *clear_all { ... } so commands::emulation::emulate(...) runs when
those flags are provided).
---
Nitpick comments:
In `@src/commands/executor.rs`:
- Around line 307-323: The "emulate" match arm currently constructs
EmulateParams without ensuring that accuracy is only provided when geolocation
is present; add the same validation used for `new-page` so that when args
contains "accuracy" but not "geolocation" you return an error before calling
commands::emulation::emulate (or move/duplicate this check into
commands::emulation::emulate for centralized validation). Specifically, check
the parsed values derived for EmulateParams.accuracy and
EmulateParams.geolocation and return an appropriate error/result if
accuracy.is_some() && geolocation.is_none(), referencing the match arm handling
"emulate" and the EmulateParams fields to locate where to add the guard.
- Around line 95-108: The new-page emulation block builds EmulateParams even
when accuracy is provided but geolocation is missing, causing accuracy to be
ignored; add the same validation used by navigate: if args.get("accuracy") is
Some and geolocation.is_none() then return an error (or surface a user-facing
validation message) instead of silently continuing. Locate the params
construction that creates commands::emulation::EmulateParams and add a pre-check
using args.get("accuracy") and the geolocation variable to enforce "accuracy
requires geolocation" before creating EmulateParams.
In `@src/commands/navigate.rs`:
- Around line 165-168: The timeout branch in wait_for_load currently prints a
warning and returns Ok(()), which is inconsistent with pages.rs::wait_for that
returns an error on timeout; change wait_for_load to return a Result::Err on
deadline expiry (or otherwise mirror pages.rs::wait_for semantics) so callers
receive a failure instead of a silent warning, update the function
signature/return path of wait_for_load accordingly, and adjust any callers to
handle the error (or add explicit docs/tests if you intentionally keep the
warning approach) — reference function names: wait_for_load and wait_for.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0f641fc9-df14-4434-ae92-2454f25f1183
📒 Files selected for processing (9)
README.mdskill/chrome-devtools/SKILL.mdsrc/commands/emulation.rssrc/commands/executor.rssrc/commands/navigate.rssrc/commands/pages.rssrc/constants.rssrc/lib.rssrc/main.rs
✅ Files skipped from review due to trivial changes (4)
- src/lib.rs
- src/constants.rs
- skill/chrome-devtools/SKILL.md
- README.md
✅ Actions performedReviews resumed. |
- Move attach_to_target inside result block in new_page so that close_target is reached if attach fails (prevents about:blank leak) - Replace `idx as usize` with usize::try_from to catch overflow on platforms where usize < u64 - Use args_os() + find_map for subcommand detection to avoid lossy UTF-8 conversion of argv
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive page emulation feature (viewport, mobile, device scale factor, and geolocation overrides), adds support for custom HTTP headers via --extra-headers, and refactors page targeting for closing and selecting pages. It also replaces the resize command with the new emulate command, adds command argument validation, and updates the documentation and Claude Code skill accordingly. The reviewer provided valuable feedback on making input parsing (viewport, geolocation, and extra headers) more robust against whitespace and type variations, as well as refactoring the CLI structure to allow dynamic, robust testing of command arguments against Clap definitions.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/commands/navigate.rs (1)
134-135: 💤 Low valueRedundant
with_navigated_tocall.
with_navigated_to(url.clone())is called on line 134, thenwith_navigated_to(url)is called again on the result at line 135. The second call overwrites the first with the same value.Suggested simplification
async fn do_reload( client: &mut CdpClient, session_id: &str, output: Option<&str>, ) -> Result<CommandResult> { client .send_to_target(session_id, "Page.reload", json!({})) .await?; wait_for_load(client, session_id, NAVIGATION_TIMEOUT_MS).await?; let url = client.current_url(session_id).await?; - let result = CommandResult::output("Reloaded page".to_string()).with_navigated_to(url.clone()); - Ok(result.save_output(output).await?.with_navigated_to(url)) + let result = CommandResult::output("Reloaded page".to_string()).with_navigated_to(url); + Ok(result.save_output(output).await?)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/navigate.rs` around lines 134 - 135, The code calls with_navigated_to twice on the same CommandResult (once when building result and again after save_output), which is redundant; remove the first call on CommandResult::output(...) and keep the single with_navigated_to invocation after save_output so the final returned value has the navigated URL. Update the construction around CommandResult::output, variable result, save_output, and with_navigated_to so only the post-save_output call remains.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/commands/navigate.rs`:
- Around line 134-135: The code calls with_navigated_to twice on the same
CommandResult (once when building result and again after save_output), which is
redundant; remove the first call on CommandResult::output(...) and keep the
single with_navigated_to invocation after save_output so the final returned
value has the navigated URL. Update the construction around
CommandResult::output, variable result, save_output, and with_navigated_to so
only the post-save_output call remains.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b0444fe2-c14f-48f7-8fcc-b12ae599e7c9
📒 Files selected for processing (12)
src/cdp.rssrc/commands/emulation.rssrc/commands/evaluate.rssrc/commands/executor.rssrc/commands/input.rssrc/commands/navigate.rssrc/commands/pages.rssrc/commands/screenshot.rssrc/commands/snapshot.rssrc/commands/third_party.rssrc/main.rssrc/protocol.rs
✅ Files skipped from review due to trivial changes (6)
- src/commands/evaluate.rs
- src/commands/snapshot.rs
- src/commands/screenshot.rs
- src/commands/third_party.rs
- src/cdp.rs
- src/protocol.rs
- Removed the CLI command definitions and related logic from `src/main.rs`, moving them to the `chrome_devtools_cli` crate. - Updated the main function to utilize the new command structure and error handling. - Simplified the command execution flow by integrating the `run` function from the `chrome_devtools_cli` crate. - Enhanced the test suite in `executor_tests.rs` to ensure `known_args` remains in sync with the CLI command definitions, improving maintainability and reducing potential discrepancies.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the CLI structure, moving the main logic to a library crate (src/lib.rs), and introduces a unified page emulation command (emulate) that supports viewport, mobile, device scale factor, and geolocation overrides. It also adds support for custom HTTP headers via --extra-headers, replaces hardcoded navigation timeouts with a central constant, and introduces dynamic argument validation backed by a new synchronization test. The documentation in README.md and SKILL.md has been updated accordingly. The review feedback suggests making the string parsing for both viewport (WxH) and geolocation (lat,lon) parameters more robust by trimming whitespace and handling case-insensitivity.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/commands/navigate.rs (1)
19-34:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReject conflicting navigation intents instead of silently picking first match.
At Line 26 onward,
backwins overforward/reload/urlby branch order. Please fail fast when multiple intents are supplied (e.g.,--back --reload, orURL + --back) to avoid surprising behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/navigate.rs` around lines 19 - 34, Detect conflicting navigation intents before mutating state by counting the requested intents (back, forward, reload, and url presence) and bail if more than one is set; modify the logic in navigate.rs (the block that currently validates intent and then calls super::pages::apply_extra_headers and branches to go_back, go_forward, do_reload) to compute an intent_count from back, forward, reload, and url.is_some() and return an error (bail!) when intent_count > 1 with a clear message about conflicting flags instead of picking the first match.
♻️ Duplicate comments (1)
src/commands/navigate.rs (1)
24-49:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClear
extra_headersafter command to avoid credential bleed across later requests.
Network.setExtraHTTPHeadersis session-persistent, and this path never resets it. A one-offnavigate --extra-headerscan leak headers (e.g.,Authorization) into subsequent commands on the same target/session.Suggested minimal fix
pub async fn navigate( @@ ) -> Result<CommandResult> { @@ - super::pages::apply_extra_headers(client, session_id, extra_headers).await?; + let headers_applied = extra_headers.is_some(); + super::pages::apply_extra_headers(client, session_id, extra_headers).await?; - if back { - return go_back(client, session_id, output).await; - } - if forward { - return go_forward(client, session_id, output).await; - } - if reload { - return do_reload(client, session_id, output).await; - } + let nav_result: Result<CommandResult> = async { + if back { + return go_back(client, session_id, output).await; + } + if forward { + return go_forward(client, session_id, output).await; + } + if reload { + return do_reload(client, session_id, output).await; + } - let url = url.unwrap(); // safe: validated above + let url = url.unwrap(); // safe: validated above - let result = client - .send_to_target(session_id, "Page.navigate", json!({"url": url})) - .await?; + let result = client + .send_to_target(session_id, "Page.navigate", json!({"url": url})) + .await?; - if let Some(err) = result.get("errorText").and_then(|v| v.as_str()) { - bail!("Navigation error: {err}"); - } + if let Some(err) = result.get("errorText").and_then(|v| v.as_str()) { + bail!("Navigation error: {err}"); + } - wait_for_load(client, session_id, NAVIGATION_TIMEOUT_MS).await?; - let result = - CommandResult::output(format!("Navigated to {url}")).with_navigated_to(url.to_string()); - Ok(result.save_output(output).await?) + wait_for_load(client, session_id, NAVIGATION_TIMEOUT_MS).await?; + let result = CommandResult::output(format!("Navigated to {url}")) + .with_navigated_to(url.to_string()); + Ok(result.save_output(output).await?) + } + .await; + + if headers_applied { + let _ = client + .send_to_target(session_id, "Network.setExtraHTTPHeaders", json!({"headers": {}})) + .await; + } + + nav_result }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/navigate.rs` around lines 24 - 49, The navigate path applies session-persistent extra headers via super::pages::apply_extra_headers but never clears them, risking credential leakage; update navigate.rs so that after apply_extra_headers you always clear the headers (e.g., call a clear function such as super::pages::clear_extra_headers or invoke Network.setExtraHTTPHeaders with an empty header map) and ensure that clear is executed on all exit paths (after successful navigation and before each early return from go_back, go_forward, do_reload), awaiting the clear call and treating clear failures as non-fatal (log them) so headers are removed regardless of outcome.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/commands/navigate.rs`:
- Around line 19-34: Detect conflicting navigation intents before mutating state
by counting the requested intents (back, forward, reload, and url presence) and
bail if more than one is set; modify the logic in navigate.rs (the block that
currently validates intent and then calls super::pages::apply_extra_headers and
branches to go_back, go_forward, do_reload) to compute an intent_count from
back, forward, reload, and url.is_some() and return an error (bail!) when
intent_count > 1 with a clear message about conflicting flags instead of picking
the first match.
---
Duplicate comments:
In `@src/commands/navigate.rs`:
- Around line 24-49: The navigate path applies session-persistent extra headers
via super::pages::apply_extra_headers but never clears them, risking credential
leakage; update navigate.rs so that after apply_extra_headers you always clear
the headers (e.g., call a clear function such as
super::pages::clear_extra_headers or invoke Network.setExtraHTTPHeaders with an
empty header map) and ensure that clear is executed on all exit paths (after
successful navigation and before each early return from go_back, go_forward,
do_reload), awaiting the clear call and treating clear failures as non-fatal
(log them) so headers are removed regardless of outcome.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 99522901-5f54-47d3-8ea5-c26bbe054c47
📒 Files selected for processing (4)
src/commands/navigate.rssrc/lib.rssrc/main.rstests/executor_tests.rs
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request restructures the CLI and daemon codebase, moving core logic from src/main.rs to src/lib.rs and introducing modular subcommands. Key additions include a unified emulate command to manage viewport, mobile, and geolocation overrides, support for custom HTTP headers via --extra-headers, and updating page-targeting commands to accept target IDs or friendly names. The documentation (README.md and SKILL.md) has been updated to reflect these changes, and a new integration test ensures that executor arguments stay in sync with Clap definitions. Feedback focuses on optimizing the handling of custom HTTP headers: only clearing headers when they were actually set to avoid unnecessary CDP round-trips, ensuring headers are cleared in new_page to prevent unexpected persistence, and disabling the Network domain after clearing headers to stop network event floods.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/commands/pages.rs (1)
95-116:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClear page extra HTTP headers before detaching the target session
apply_extra_headerscallsNetwork.setExtraHTTPHeaders, which persists for subsequent requests on the same CDP target/session until explicitly cleared/overwritten. Innew_page, the success path waits for navigation/load and then detaches without clearing, so headers (e.g.,Authorization) can remain applied for later requests the page makes. Callclear_extra_headersbeforedetach_from_target(using the same condition/inputs asapply_extra_headers).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/pages.rs` around lines 95 - 116, The Page.navigate flow currently calls apply_extra_headers but never clears them before detaching the CDP session; update the closure that uses apply_extra_headers to call clear_extra_headers(client, &session_id, extra_headers).await (using the same extra_headers condition/inputs as apply_extra_headers) immediately before calling client.detach_from_target(&session_id).await so headers are removed prior to detach—ensure the clear call is awaited and does not override/lose the original inner Result (i.e., handle/ignore clear errors or log them but return the original inner result).src/commands/emulation.rs (1)
47-52:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCall
EmulateParams::validate()at the top ofemulate().Lines 47-52 skip the cross-field validation defined on the type, so invalid combinations can be accepted or downgraded to a generic error. For example,
--clear-geolocationplus--accuracy, or--clear-viewportplus--mobile, will currently execute the clear action and silently ignore the invalid flag instead of failing fast.Suggested fix
pub async fn emulate( client: &mut CdpClient, session_id: &str, params: EmulateParams, ) -> Result<CommandResult> { + params.validate()?; let mut actions = Vec::new();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/emulation.rs` around lines 47 - 52, Call EmulateParams::validate() at the start of the emulate function (before constructing actions) and propagate any validation error instead of proceeding; in other words, add a call like params.validate()? (or equivalent error-handling) at the top of pub async fn emulate(...), so invalid cross-field combinations are rejected immediately rather than being downgraded to a generic error when actions are executed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/commands/navigate.rs`:
- Around line 58-63: The cleanup unconditionally calls
super::pages::clear_extra_headers(client, session_id) even when this navigate
call didn't set any headers; change the code so headers are only cleared if this
call actually applied them: introduce a boolean (e.g., applied_extra_headers)
that's set when extra_headers is Some and the code successfully sets those
headers (where navigation sets the headers), then replace the unconditional
clear_extra_headers invocation with a conditional that calls
clear_extra_headers(client, session_id).await only when applied_extra_headers is
true; reference functions/values clear_extra_headers, extra_headers,
navigate_result, client, and session_id to locate the relevant logic.
---
Outside diff comments:
In `@src/commands/emulation.rs`:
- Around line 47-52: Call EmulateParams::validate() at the start of the emulate
function (before constructing actions) and propagate any validation error
instead of proceeding; in other words, add a call like params.validate()? (or
equivalent error-handling) at the top of pub async fn emulate(...), so invalid
cross-field combinations are rejected immediately rather than being downgraded
to a generic error when actions are executed.
In `@src/commands/pages.rs`:
- Around line 95-116: The Page.navigate flow currently calls apply_extra_headers
but never clears them before detaching the CDP session; update the closure that
uses apply_extra_headers to call clear_extra_headers(client, &session_id,
extra_headers).await (using the same extra_headers condition/inputs as
apply_extra_headers) immediately before calling
client.detach_from_target(&session_id).await so headers are removed prior to
detach—ensure the clear call is awaited and does not override/lose the original
inner Result (i.e., handle/ignore clear errors or log them but return the
original inner result).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1bcf56a5-57b6-422b-aacf-44c39d5785e6
📒 Files selected for processing (3)
src/commands/emulation.rssrc/commands/navigate.rssrc/commands/pages.rs
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the CLI by moving the core logic and command definitions from src/main.rs into a library module (src/lib.rs). It introduces a unified emulate command for managing viewport, mobile, and geolocation overrides, and adds support for custom HTTP headers during navigation and page creation. Documentation and the Claude Code skill have been updated accordingly. Feedback on the changes suggests moving parameter validation for device_scale_factor and accuracy into EmulateParams::validate() to fail early, and filtering out built-in help and version arguments in the new sync test to prevent assertion failures.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request restructures the CLI by moving the core logic from main.rs into a library (lib.rs), introduces a new emulate command for viewport and geolocation overrides, supports custom HTTP headers via --extra-headers, and improves argument validation. Feedback suggests enhancing the usability of --extra-headers by automatically converting primitive scalar values (numbers and booleans) to strings instead of bailing when they are not explicitly quoted.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request restructures the project by moving CLI definitions and direct execution logic from src/main.rs to src/lib.rs, and introduces several new features. These include a unified emulate command to manage viewport, mobile, and geolocation overrides, support for custom HTTP headers via --extra-headers, and updating close-page and select-page to accept target IDs or friendly names. Additionally, argument validation has been added to executor.rs along with a test to keep arguments in sync with Clap. The review comments suggest improving the user-friendliness of the close_page and select_page commands by printing friendly names instead of raw hex target IDs, and setting the target_id field in the CommandResult for select_page to output the target helper.
| pub async fn close_page(client: &mut CdpClient, target_id: &str) -> Result<CommandResult> { | ||
| client.close_target(target_id).await?; | ||
| Ok(CommandResult::output(format!("Closed page: {target_id}"))) | ||
| } |
There was a problem hiding this comment.
The close_page command currently prints the raw 32-character hex target_id in its success message, which is not very user-friendly. Since the friendly name generator is available, we can print the friendly name instead to make the output much more readable.
pub async fn close_page(client: &mut CdpClient, target_id: &str) -> Result<CommandResult> {
client.close_target(target_id).await?;
let friendly_name = friendly::to_friendly(target_id);
Ok(CommandResult::output(format!("Closed page: {friendly_name}")))
}| pub async fn select_page(client: &mut CdpClient, target_id: &str) -> Result<CommandResult> { | ||
| client.activate_target(target_id).await?; | ||
| Ok(CommandResult::output(format!("Activated page: {target_id}"))) | ||
| } |
There was a problem hiding this comment.
Similar to close_page, select_page prints the raw target_id in its success message. Additionally, it does not set the target_id field in the returned CommandResult. Setting this field allows the CLI to automatically print the [target: friendly-name] helper at the end of the command, making it easy for users to copy and pin subsequent commands to this page.
pub async fn select_page(client: &mut CdpClient, target_id: &str) -> Result<CommandResult> {
client.activate_target(target_id).await?;
let friendly_name = friendly::to_friendly(target_id);
let mut result = CommandResult::output(format!("Activated page: {friendly_name}"));
result.target_id = Some(friendly_name);
Ok(result)
}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/commands/pages.rs (1)
106-123:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClear
extra_headersbefore detaching the temporary session innew_page
new_pageappliesNetwork.setExtraHTTPHeadersbeforePage.navigate, but the success path never clears it before detaching/closing the target. CDP extra headers persist across subsequent navigations/requests on the target until explicitly reset (e.g., with{}), so one-shot headers likeAuthorizationcan leak into later navigations.Suggested fix
let inner: Result<()> = async { if let Some(params) = emulation { crate::commands::emulation::emulate(client, &session_id, params).await?; } apply_extra_headers(client, &session_id, extra_headers).await?; let nav_result = client .send_to_target(&session_id, "Page.navigate", json!({ "url": url })) .await?; if let Some(error_text) = nav_result.get("errorText").and_then(|v| v.as_str()) { anyhow::bail!("Page.navigate failed: {error_text}"); } crate::commands::navigate::wait_for_load(client, &session_id, NAVIGATION_TIMEOUT_MS).await?; Ok(()) } .await; + if extra_headers.is_some() { + if let Err(e) = clear_extra_headers(client, &session_id).await { + eprintln!("Warning: failed to clear extra headers: {e}"); + } + } + let _ = client.detach_from_target(&session_id).await; inner🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/pages.rs` around lines 106 - 123, The code sets extra headers via apply_extra_headers (which calls Network.setExtraHTTPHeaders) before navigation but never clears them, so temporary headers like Authorization can leak after client.detach_from_target; modify new_page (the async block around apply_extra_headers, Page.navigate, and wait_for_load) to always reset/clear extra headers (e.g., call apply_extra_headers with an empty map or call Network.setExtraHTTPHeaders with {}) before detaching the target — ensure this cleanup runs in both success and error paths (for example in a finally-like block or immediately before client.detach_from_target) so the headers are cleared regardless of navigation outcome.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/commands/pages.rs`:
- Around line 106-123: The code sets extra headers via apply_extra_headers
(which calls Network.setExtraHTTPHeaders) before navigation but never clears
them, so temporary headers like Authorization can leak after
client.detach_from_target; modify new_page (the async block around
apply_extra_headers, Page.navigate, and wait_for_load) to always reset/clear
extra headers (e.g., call apply_extra_headers with an empty map or call
Network.setExtraHTTPHeaders with {}) before detaching the target — ensure this
cleanup runs in both success and error paths (for example in a finally-like
block or immediately before client.detach_from_target) so the headers are
cleared regardless of navigation outcome.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f80572fc-b7b6-41cf-a5ed-52c669935c3a
📒 Files selected for processing (1)
src/commands/pages.rs
that's what it meant to do, keep the page having the same extra headers all the time. |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the CLI by moving core logic from src/main.rs into a library structure in src/lib.rs. It introduces a unified emulate command supporting viewport, mobile, device scale factor, and geolocation overrides, and adds support for custom HTTP headers (--extra-headers) during navigation and page creation. Additionally, it updates close-page and select-page to target tabs by either ID or index, implements dynamic argument validation in executor.rs (with a corresponding sync test), and updates the documentation in README.md and SKILL.md to match these new capabilities. I have no feedback to provide as there are no review comments.
1 participant
--extra-headersflag tonavigatecommand for setting custom HTTP headers (e.g. Authorization) via Network.setExtraHTTPHeadersset-geolocationcommand for geolocation emulation with --latitude, --longitude, --accuracy, and --clear flags using Emulation.setGeolocationOverrideSummary by CodeRabbit
New Features
Bug Fixes
Documentation
Removed
Tests